On 06/07/2015 04:10, Alexey Kardashevskiy wrote: > This reworks finish_realize() which used to finalize DMA setup with > an assumption that it will not change later. > > New callbacks supports various window parameters such as page and > windows sizes. The new callback return error code rather than Error**. > > This is a mechanical change so no change in behaviour is expected. > This is a part of getting rid of spapr-pci-vfio-host-bridge type. > > Signed-off-by: Alexey Kardashevskiy <a...@ozlabs.ru> > Reviewed-by: David Gibson <da...@gibson.dropbear.id.au> > --- > Changes: > v8: > * moved spapr_phb_dma_capabilities_update() higher to avoid forward > declaration in following patches and keep DMA code together (i.e. next > to spapr_pci_dma_iommu()) > --- > hw/ppc/spapr_pci.c | 59 > ++++++++++++++++++++++++++------------------- > hw/ppc/spapr_pci_vfio.c | 53 ++++++++++++++++------------------------ > include/hw/pci-host/spapr.h | 8 +++++- > 3 files changed, 62 insertions(+), 58 deletions(-) > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index a8f79d8..c1ca13d 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -808,6 +808,28 @@ static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, > PCIDevice *pdev) > return buf; > } > > +static int spapr_phb_dma_capabilities_update(sPAPRPHBState *sphb) > +{ > + sphb->dma32_window_start = 0; > + sphb->dma32_window_size = SPAPR_PCI_DMA32_SIZE; > + > + return 0; > +} > + > +static int spapr_phb_dma_init_window(sPAPRPHBState *sphb, > + uint32_t liobn, uint32_t page_shift, > + uint64_t window_size) > +{ > + uint64_t bus_offset = sphb->dma32_window_start; > + sPAPRTCETable *tcet; > + > + tcet = spapr_tce_new_table(DEVICE(sphb), liobn, bus_offset, page_shift, > + window_size >> page_shift, > + false); > + > + return tcet ? 0 : -1; > +} > + > /* Macros to operate with address in OF binding to PCI */ > #define b_x(x, p, l) (((x) & ((1<<(l))-1)) << (p)) > #define b_n(x) b_x((x), 31, 1) /* 0 if relocatable */ > @@ -1220,6 +1242,7 @@ static void spapr_phb_realize(DeviceState *dev, Error > **errp) > int i; > PCIBus *bus; > uint64_t msi_window_size = 4096; > + sPAPRTCETable *tcet; > > if (sphb->index != (uint32_t)-1) { > hwaddr windows_base; > @@ -1369,33 +1392,18 @@ static void spapr_phb_realize(DeviceState *dev, Error > **errp) > } > } > > - if (!info->finish_realize) { > - error_setg(errp, "finish_realize not defined"); > - return; > - } > - > - info->finish_realize(sphb, errp); > - > - sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, > g_free); > -} > - > -static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp) > -{ > - sPAPRTCETable *tcet; > - uint32_t nb_table; > - > - nb_table = SPAPR_PCI_DMA32_SIZE >> SPAPR_TCE_PAGE_SHIFT; > - tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn, > - 0, SPAPR_TCE_PAGE_SHIFT, nb_table, false); > + info->dma_capabilities_update(sphb); > + info->dma_init_window(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT, > + sphb->dma32_window_size); > + tcet = spapr_tce_find_by_liobn(sphb->dma_liobn); > if (!tcet) { > - error_setg(errp, "Unable to create TCE table for %s", > - sphb->dtbusname); > - return ; > + error_setg(errp, "failed to create TCE table"); > + return; > } > - > - /* Register default 32bit DMA window */ > - memory_region_add_subregion(&sphb->iommu_root, 0, > + memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, > spapr_tce_get_iommu(tcet)); > + > + sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, > g_free); > } > > static int spapr_phb_children_reset(Object *child, void *opaque) > @@ -1543,9 +1551,10 @@ static void spapr_phb_class_init(ObjectClass *klass, > void *data) > dc->vmsd = &vmstate_spapr_pci; > set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); > dc->cannot_instantiate_with_device_add_yet = false; > - spc->finish_realize = spapr_phb_finish_realize; > hp->plug = spapr_phb_hot_plug_child; > hp->unplug = spapr_phb_hot_unplug_child; > + spc->dma_capabilities_update = spapr_phb_dma_capabilities_update; > + spc->dma_init_window = spapr_phb_dma_init_window; > } > > static const TypeInfo spapr_phb_info = { > diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c > index cca45ed..6e3e17b 100644 > --- a/hw/ppc/spapr_pci_vfio.c > +++ b/hw/ppc/spapr_pci_vfio.c > @@ -28,48 +28,36 @@ static Property spapr_phb_vfio_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > -static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp) > +static int spapr_phb_vfio_dma_capabilities_update(sPAPRPHBState *sphb) > { > sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); > struct vfio_iommu_spapr_tce_info info = { .argsz = sizeof(info) }; > int ret; > - sPAPRTCETable *tcet; > - uint32_t liobn = svphb->phb.dma_liobn; > > - if (svphb->iommugroupid == -1) { > - error_setg(errp, "Wrong IOMMU group ID %d", svphb->iommugroupid); > - return; > - } > - > - ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, > - VFIO_CHECK_EXTENSION, > - (void *) VFIO_SPAPR_TCE_IOMMU);
This ioctl() disappears completely, was it useless ? [ but I guess the following one will fail exactly in the same way ] > - if (ret != 1) { > - error_setg_errno(errp, -ret, > - "spapr-vfio: SPAPR extension is not supported"); > - return; > - } > - > - ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid, > + ret = vfio_container_ioctl(&sphb->iommu_as, svphb->iommugroupid, > VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info); > if (ret) { > - error_setg_errno(errp, -ret, > - "spapr-vfio: get info from container failed"); > - return; > + return ret; > } > > - tcet = spapr_tce_new_table(DEVICE(sphb), liobn, info.dma32_window_start, > - SPAPR_TCE_PAGE_SHIFT, > - info.dma32_window_size >> > SPAPR_TCE_PAGE_SHIFT, > + sphb->dma32_window_start = info.dma32_window_start; > + sphb->dma32_window_size = info.dma32_window_size; > + > + return ret; > +} > + > +static int spapr_phb_vfio_dma_init_window(sPAPRPHBState *sphb, > + uint32_t liobn, uint32_t > page_shift, > + uint64_t window_size) > +{ > + uint64_t bus_offset = sphb->dma32_window_start; > + sPAPRTCETable *tcet; > + > + tcet = spapr_tce_new_table(DEVICE(sphb), liobn, bus_offset, page_shift, > + window_size >> page_shift, > true); > - if (!tcet) { > - error_setg(errp, "spapr-vfio: failed to create VFIO TCE table"); > - return; > - } > > - /* Register default 32bit DMA window */ > - memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, > - spapr_tce_get_iommu(tcet)); > + return tcet ? 0 : -1; > } > > static void spapr_phb_vfio_eeh_reenable(sPAPRPHBVFIOState *svphb) > @@ -257,7 +245,8 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, > void *data) > > dc->props = spapr_phb_vfio_properties; > dc->reset = spapr_phb_vfio_reset; > - spc->finish_realize = spapr_phb_vfio_finish_realize; > + spc->dma_capabilities_update = spapr_phb_vfio_dma_capabilities_update; > + spc->dma_init_window = spapr_phb_vfio_dma_init_window; > spc->eeh_set_option = spapr_phb_vfio_eeh_set_option; > spc->eeh_get_state = spapr_phb_vfio_eeh_get_state; > spc->eeh_reset = spapr_phb_vfio_eeh_reset; > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h > index 5322b56..b6d5719 100644 > --- a/include/hw/pci-host/spapr.h > +++ b/include/hw/pci-host/spapr.h > @@ -48,7 +48,10 @@ typedef struct sPAPRPHBVFIOState sPAPRPHBVFIOState; > struct sPAPRPHBClass { > PCIHostBridgeClass parent_class; > > - void (*finish_realize)(sPAPRPHBState *sphb, Error **errp); > + int (*dma_capabilities_update)(sPAPRPHBState *sphb); > + int (*dma_init_window)(sPAPRPHBState *sphb, > + uint32_t liobn, uint32_t page_shift, > + uint64_t window_size); > int (*eeh_set_option)(sPAPRPHBState *sphb, unsigned int addr, int > option); > int (*eeh_get_state)(sPAPRPHBState *sphb, int *state); > int (*eeh_reset)(sPAPRPHBState *sphb, int option); > @@ -90,6 +93,9 @@ struct sPAPRPHBState { > int32_t msi_devs_num; > spapr_pci_msi_mig *msi_devs; > > + uint32_t dma32_window_start; > + uint32_t dma32_window_size; > + > QLIST_ENTRY(sPAPRPHBState) list; > }; > > Really nice work. Reviewed-by: Laurent Vivier <lviv...@redhat.com>