Re: [Qemu-devel] [PATCH 1/7] vfio: Remove unneeded union from VFIOContainer

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 2015-09-24 at 14:33 +1000, David Gibson wrote:
> Currently the VFIOContainer iommu_data field contains a union with
> different information for different host iommu types.  However:
>    * It only actually contains information for the x86-like "Type1" iommu
>    * Because we have a common listener the Type1 fields are actually used
> on all IOMMU types, including the SPAPR TCE type as well
>    * There's no tag in the VFIOContainer to tell you which union member is
> valid anyway.

FWIW, this last point isn't valid.  The IOMMU setup determines which
union member is active and the listener and release functions are
specific to the union member.  There's no need whatsoever for a tag to
keep track of the union member in use.  The only problem is that the
union solved a problem that never really came to exist, so we can now
remove it and simplify things.

I'll remove this last bullet point unless there's some objection.
Thanks,

Alex


> In fact we now have a general structure for the listener which is unlikely
> to ever need per-iommu-type information, so this patch removes the union.
> 
> In a similar way we can unify the setup of the vfio memory listener in
> vfio_connect_container() that is currently split across a switch on iommu
> type, but is effectively the same in both cases.
> 
> The iommu_data.release pointer was only needed as a cleanup function
> which would handle potentially different data in the union.  With the
> union gone, it too can be removed.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/vfio/common.c              | 52 ++++++++++++++++---------------------------
>  include/hw/vfio/vfio-common.h | 16 +++----------
>  2 files changed, 22 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 0d341a3..1545f62 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -315,8 +315,7 @@ out:
>  static void vfio_listener_region_add(MemoryListener *listener,
>                                       MemoryRegionSection *section)
>  {
> -    VFIOContainer *container = container_of(listener, VFIOContainer,
> -                                            iommu_data.type1.listener);
> +    VFIOContainer *container = container_of(listener, VFIOContainer, listener);
>      hwaddr iova, end;
>      Int128 llend;
>      void *vaddr;
> @@ -406,9 +405,9 @@ static void vfio_listener_region_add(MemoryListener *listener,
>           * can gracefully fail.  Runtime, there's not much we can do other
>           * than throw a hardware error.
>           */
> -        if (!container->iommu_data.type1.initialized) {
> -            if (!container->iommu_data.type1.error) {
> -                container->iommu_data.type1.error = ret;
> +        if (!container->initialized) {
> +            if (!container->error) {
> +                container->error = ret;
>              }
>          } else {
>              hw_error("vfio: DMA mapping failed, unable to continue");
> @@ -419,8 +418,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>  static void vfio_listener_region_del(MemoryListener *listener,
>                                       MemoryRegionSection *section)
>  {
> -    VFIOContainer *container = container_of(listener, VFIOContainer,
> -                                            iommu_data.type1.listener);
> +    VFIOContainer *container = container_of(listener, VFIOContainer, listener);
>      hwaddr iova, end;
>      int ret;
>  
> @@ -485,7 +483,7 @@ static const MemoryListener vfio_memory_listener = {
>  
>  static void vfio_listener_release(VFIOContainer *container)
>  {
> -    memory_listener_unregister(&container->iommu_data.type1.listener);
> +    memory_listener_unregister(&container->listener);
>  }
>  
>  int vfio_mmap_region(Object *obj, VFIORegion *region,
> @@ -683,21 +681,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>              ret = -errno;
>              goto free_container_exit;
>          }
> -
> -        container->iommu_data.type1.listener = vfio_memory_listener;
> -        container->iommu_data.release = vfio_listener_release;
> -
> -        memory_listener_register(&container->iommu_data.type1.listener,
> -                                 container->space->as);
> -
> -        if (container->iommu_data.type1.error) {
> -            ret = container->iommu_data.type1.error;
> -            error_report("vfio: memory listener initialization failed for container");
> -            goto listener_release_exit;
> -        }
> -
> -        container->iommu_data.type1.initialized = true;
> -
>      } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) {
>          ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
>          if (ret) {
> @@ -723,19 +706,24 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>              ret = -errno;
>              goto free_container_exit;
>          }
> -
> -        container->iommu_data.type1.listener = vfio_memory_listener;
> -        container->iommu_data.release = vfio_listener_release;
> -
> -        memory_listener_register(&container->iommu_data.type1.listener,
> -                                 container->space->as);
> -
>      } else {
>          error_report("vfio: No available IOMMU models");
>          ret = -EINVAL;
>          goto free_container_exit;
>      }
>  
> +    container->listener = vfio_memory_listener;
> +
> +    memory_listener_register(&container->listener, container->space->as);
> +
> +    if (container->error) {
> +        ret = container->error;
> +        error_report("vfio: memory listener initialization failed for container");
> +        goto listener_release_exit;
> +    }
> +
> +    container->initialized = true;
> +
>      QLIST_INIT(&container->group_list);
>      QLIST_INSERT_HEAD(&space->containers, container, next);
>  
> @@ -774,9 +762,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
>          VFIOAddressSpace *space = container->space;
>          VFIOGuestIOMMU *giommu, *tmp;
>  
> -        if (container->iommu_data.release) {
> -            container->iommu_data.release(container);
> -        }
> +        vfio_listener_release(container);
>          QLIST_REMOVE(container, next);
>  
>          QLIST_FOREACH_SAFE(giommu, &container->giommu_list, giommu_next, tmp) {
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 9b9901f..fbbe6de 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -59,22 +59,12 @@ typedef struct VFIOAddressSpace {
>  
>  struct VFIOGroup;
>  
> -typedef struct VFIOType1 {
> -    MemoryListener listener;
> -    int error;
> -    bool initialized;
> -} VFIOType1;
> -
>  typedef struct VFIOContainer {
>      VFIOAddressSpace *space;
>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
> -    struct {
> -        /* enable abstraction to support various iommu backends */
> -        union {
> -            VFIOType1 type1;
> -        };
> -        void (*release)(struct VFIOContainer *);
> -    } iommu_data;
> +    MemoryListener listener;
> +    int error;
> +    bool initialized;
>      QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
>      QLIST_HEAD(, VFIOGroup) group_list;
>      QLIST_ENTRY(VFIOContainer) next;