Re: [Qemu-devel] [PATCH qemu v5 10/12] spapr_pci: Rework finish_realize()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6827 bytes --]

On Tue, Mar 31, 2015 at 04:28:45PM +1100, Alexey Kardashevskiy wrote:
> This renames and reworks finish_realize() which used to finalize DMA
> setup with an assumption that it will not change later.
> 
> The new callback supports multiple windows and supports various
> parameters such as page and windows sizes.
> 
> This is a mechanical change so no change in behaviour is expected.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

In terms of the basic concept anyway.  I'd prefer to see it moved
earlier in the series though, so we don't have the now confusingly
named "finish_realize" callbacks floating about.

> ---
>  hw/ppc/spapr_pci.c          | 14 +++++++++-----
>  hw/ppc/spapr_pci_vfio.c     | 24 +++++++++++++-----------
>  include/hw/pci-host/spapr.h |  3 ++-
>  3 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 57bbc82..b8c2488 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -729,10 +729,9 @@ int spapr_phb_dma_reset(sPAPRPHBState *sphb)
>      const uint32_t liobn = SPAPR_PCI_LIOBN(sphb->index, 0);
>      sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn);
>      sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> -    Error *err = NULL;
>  
>      spapr_tce_table_disable(tcet);
> -    spc->finish_realize(sphb, &err);
> +    spc->dma_init_window(sphb, liobn, SPAPR_TCE_PAGE_SHIFT, 0);
>  
>      return 0;
>  }
> @@ -904,15 +903,20 @@ static void spapr_phb_realize(DeviceState *dev, Error **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)
> +static int spapr_phb_dma_init_window(sPAPRPHBState *sphb, uint32_t liobn,
> +                                     uint32_t page_shift,
> +                                     uint64_t window_size_hint)
>  {
>      sPAPRTCETable *tcet;
>      uint32_t nb_table;
> +    uint64_t bus_offset = 0;
>  
>      tcet = spapr_tce_find_by_liobn(sphb->dma_liobn);
>      nb_table = SPAPR_PCI_DMA32_SIZE >> SPAPR_TCE_PAGE_SHIFT;
> -    spapr_tce_set_props(tcet, 0, SPAPR_TCE_PAGE_SHIFT, nb_table, false);
> +    spapr_tce_set_props(tcet, bus_offset, page_shift, nb_table, false);
>      spapr_tce_table_enable(tcet);
> +
> +    return 0;
>  }
>  
>  static int spapr_phb_children_reset(Object *child, void *opaque)
> @@ -1065,7 +1069,7 @@ 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;
> +    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 1657f6b..a7e32f6 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -21,43 +21,45 @@
>  #include "hw/pci-host/spapr.h"
>  #include "linux/vfio.h"
>  #include "hw/vfio/vfio.h"
> +#include "qemu/error-report.h"
>  
>  static Property spapr_phb_vfio_properties[] = {
>      DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
> +static int spapr_phb_vfio_dma_init_window(sPAPRPHBState *sphb, uint32_t liobn,
> +                                          uint32_t page_shift,
> +                                          uint64_t window_size_hint)
>  {
>      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;
>      uint32_t nb_table;
> +    sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn);
>  
>      ret = vfio_container_ioctl(&svphb->phb.iommu_as,
>                                 VFIO_CHECK_EXTENSION,
>                                 (void *) VFIO_SPAPR_TCE_IOMMU);
>      if (ret != 1) {
> -        error_setg_errno(errp, -ret,
> -                         "spapr-vfio: SPAPR extension is not supported");
> -        return;
> +        error_report("spapr-vfio: SPAPR extension is not supported: %m");
> +        return ret;
>      }
>  
>      ret = vfio_container_ioctl(&sphb->iommu_as,
>                                 VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info);
>      if (ret) {
> -        error_setg_errno(errp, -ret,
> -                         "spapr-vfio: get info from container failed");
> -        return;
> +        error_report("spapr-vfio: get info from container failed: %m");
> +        return ret;
>      }
>  
>      tcet = spapr_tce_find_by_liobn(liobn);
>      nb_table = info.dma32_window_size >> SPAPR_TCE_PAGE_SHIFT;
> -    spapr_tce_set_props(tcet, info.dma32_window_start, SPAPR_TCE_PAGE_SHIFT,
> +    spapr_tce_set_props(tcet, info.dma32_window_start, page_shift,
>                          nb_table, true);
>      spapr_tce_table_enable(tcet);
> +
> +    return ret;
>  }
>  
>  static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
> @@ -177,7 +179,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
>      sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
>  
>      dc->props = spapr_phb_vfio_properties;
> -    spc->finish_realize = spapr_phb_vfio_finish_realize;
> +    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 f592276..f556a41 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -48,7 +48,8 @@ typedef struct sPAPRPHBVFIOState sPAPRPHBVFIOState;
>  struct sPAPRPHBClass {
>      PCIHostBridgeClass parent_class;
>  
> -    void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
> +    int (*dma_init_window)(sPAPRPHBState *sphb, uint32_t liobn,
> +                           uint32_t page_shift, uint64_t window_size_hint);
>      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);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]