Re: [Qemu-devel] [PATCH qemu v10 03/14] spapr_pci: Convert finish_realize() to dma_capabilities_update()+dma_init_window()

[Laurent Vivier <lvivier@redhat.com>]

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 <aik@ozlabs.ru>
> Reviewed-by: David Gibson <david@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 <lvivier@redhat.com>