Re: [Qemu-devel] [RFC PATCH 01/10] vfio: Remove unneeded union from VFIOContainer

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 09/17/2015 11:09 PM, 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.
>
> 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              | 51 +++++++++++++++++--------------------------
>   include/hw/vfio/vfio-common.h | 14 +++---------
>   2 files changed, 23 insertions(+), 42 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 6d21311..e3152f6 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -316,7 +316,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
>                                        MemoryRegionSection *section)
>   {
>       VFIOContainer *container = container_of(listener, VFIOContainer,
> -                                            iommu_data.type1.listener);
> +                                            iommu_data.listener);
>       hwaddr iova, end;
>       Int128 llend;
>       void *vaddr;
> @@ -406,9 +406,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->iommu_data.initialized) {
> +            if (!container->iommu_data.error) {
> +                container->iommu_data.error = ret;
>               }
>           } else {
>               hw_error("vfio: DMA mapping failed, unable to continue");
> @@ -420,7 +420,7 @@ static void vfio_listener_region_del(MemoryListener *listener,
>                                        MemoryRegionSection *section)
>   {
>       VFIOContainer *container = container_of(listener, VFIOContainer,
> -                                            iommu_data.type1.listener);
> +                                            iommu_data.listener);
>       hwaddr iova, end;
>       int ret;
>
> @@ -485,7 +485,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->iommu_data.listener);
>   }
>
>   int vfio_mmap_region(Object *obj, VFIORegion *region,
> @@ -683,21 +683,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 +708,25 @@ 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->iommu_data.listener = vfio_memory_listener;
> +
> +    memory_listener_register(&container->iommu_data.listener,
> +                             container->space->as);
> +
> +    if (container->iommu_data.error) {
> +        ret = container->iommu_data.error;
> +        error_report("vfio: memory listener initialization failed for container");
> +        goto listener_release_exit;
> +    }
> +
> +    container->iommu_data.initialized = true;
> +
>       QLIST_INIT(&container->group_list);
>       QLIST_INSERT_HEAD(&space->containers, container, next);
>
> @@ -774,9 +765,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 59a321d..aff18cd 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -64,21 +64,13 @@ 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 *);
> +        MemoryListener listener;
> +        int error;
> +        bool initialized;
>       } iommu_data;

The only difference to my patch - "[RFC PATCH qemu 2/4] vfio: Generalize 
IOMMU memory listener" - is that you keep iommu_data struct which was 
supposed to have an union for type1/spapr/etc IOMMU data but this is gone. 
What is its purpose now?



>       QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
>       QLIST_HEAD(, VFIOGroup) group_list;
>


-- 
Alexey