Re: [PATCH v3 03/37] vfio/container: Move spapr specific init/deinit into spapr.c

["Cédric Le Goater" <clg@redhat.com>]

On 10/26/23 12:30, Zhenzhong Duan wrote:
> Move spapr specific init/deinit code into spapr.c and wrap
> them with vfio_spapr_container_init/deinit, this way footprint
> of spapr is further reduced, vfio_prereg_listener could also
> be made static.
> 
> vfio_listener_release is unnecessary when prereg_listener is
> moved out, so have it removed.
> 
> No functional changes intended.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   include/hw/vfio/vfio-common.h |  4 +-
>   hw/vfio/container.c           | 82 +++++------------------------------
>   hw/vfio/spapr.c               | 80 +++++++++++++++++++++++++++++++++-
>   3 files changed, 94 insertions(+), 72 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 0c3d390e8b..8d1d4f0a89 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -225,11 +225,14 @@ int vfio_set_dirty_page_tracking(VFIOContainer *container, bool start);
>   int vfio_query_dirty_bitmap(VFIOContainer *container, VFIOBitmap *vbmap,
>                               hwaddr iova, hwaddr size);
>   
> +/* SPAPR specific */
>   int vfio_container_add_section_window(VFIOContainer *container,
>                                         MemoryRegionSection *section,
>                                         Error **errp);
>   void vfio_container_del_section_window(VFIOContainer *container,
>                                          MemoryRegionSection *section);
> +bool vfio_spapr_container_init(VFIOContainer *container, Error **errp);
> +void vfio_spapr_container_deinit(VFIOContainer *container);
>   
>   void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
>   void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
> @@ -289,7 +292,6 @@ vfio_get_device_info_cap(struct vfio_device_info *info, uint16_t id);
>   struct vfio_info_cap_header *
>   vfio_get_cap(void *ptr, uint32_t cap_offset, uint16_t id);
>   #endif
> -extern const MemoryListener vfio_prereg_listener;
>   
>   int vfio_spapr_create_window(VFIOContainer *container,
>                                MemoryRegionSection *section,
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 7a3f005d1b..204b244b11 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -264,14 +264,6 @@ int vfio_query_dirty_bitmap(VFIOContainer *container, VFIOBitmap *vbmap,
>       return ret;
>   }
>   
> -static void vfio_listener_release(VFIOContainer *container)
> -{
> -    memory_listener_unregister(&container->listener);
> -    if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
> -        memory_listener_unregister(&container->prereg_listener);
> -    }
> -}
> -
>   static struct vfio_info_cap_header *
>   vfio_get_iommu_type1_info_cap(struct vfio_iommu_type1_info *info, uint16_t id)
>   {
> @@ -612,69 +604,11 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>       case VFIO_SPAPR_TCE_v2_IOMMU:
>       case VFIO_SPAPR_TCE_IOMMU:
>       {
> -        struct vfio_iommu_spapr_tce_info info;
> -        bool v2 = container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU;
> -
> -        /*
> -         * The host kernel code implementing VFIO_IOMMU_DISABLE is called
> -         * when container fd is closed so we do not call it explicitly
> -         * in this file.
> -         */
> -        if (!v2) {
> -            ret = ioctl(fd, VFIO_IOMMU_ENABLE);
> -            if (ret) {
> -                error_setg_errno(errp, errno, "failed to enable container");
> -                ret = -errno;
> -                goto enable_discards_exit;
> -            }
> -        } else {
> -            container->prereg_listener = vfio_prereg_listener;
> -
> -            memory_listener_register(&container->prereg_listener,
> -                                     &address_space_memory);
> -            if (container->error) {
> -                memory_listener_unregister(&container->prereg_listener);
> -                ret = -1;
> -                error_propagate_prepend(errp, container->error,
> -                    "RAM memory listener initialization failed: ");
> -                goto enable_discards_exit;
> -            }
> -        }
> -
> -        info.argsz = sizeof(info);
> -        ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info);
> +        ret = vfio_spapr_container_init(container, errp);
>           if (ret) {
> -            error_setg_errno(errp, errno,
> -                             "VFIO_IOMMU_SPAPR_TCE_GET_INFO failed");
> -            ret = -errno;
> -            if (v2) {
> -                memory_listener_unregister(&container->prereg_listener);
> -            }
>               goto enable_discards_exit;
>           }
> -
> -        if (v2) {
> -            container->pgsizes = info.ddw.pgsizes;
> -            /*
> -             * There is a default window in just created container.
> -             * To make region_add/del simpler, we better remove this
> -             * window now and let those iommu_listener callbacks
> -             * create/remove them when needed.
> -             */
> -            ret = vfio_spapr_remove_window(container, info.dma32_window_start);
> -            if (ret) {
> -                error_setg_errno(errp, -ret,
> -                                 "failed to remove existing window");
> -                goto enable_discards_exit;
> -            }
> -        } else {
> -            /* The default table uses 4K pages */
> -            container->pgsizes = 0x1000;
> -            vfio_host_win_add(container, info.dma32_window_start,
> -                              info.dma32_window_start +
> -                              info.dma32_window_size - 1,
> -                              0x1000);
> -        }
> +        break;
>       }
>       }
>   
> @@ -704,7 +638,11 @@ listener_release_exit:
>       QLIST_REMOVE(group, container_next);
>       QLIST_REMOVE(container, next);
>       vfio_kvm_device_del_group(group);
> -    vfio_listener_release(container);
> +    memory_listener_unregister(&container->listener);
> +    if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU ||
> +        container->iommu_type == VFIO_SPAPR_TCE_IOMMU) {
> +        vfio_spapr_container_deinit(container);
> +    }
>   
>   enable_discards_exit:
>       vfio_ram_block_discard_disable(container, false);
> @@ -734,7 +672,11 @@ static void vfio_disconnect_container(VFIOGroup *group)
>        * group.
>        */
>       if (QLIST_EMPTY(&container->group_list)) {
> -        vfio_listener_release(container);
> +        memory_listener_unregister(&container->listener);
> +        if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU ||
> +            container->iommu_type == VFIO_SPAPR_TCE_IOMMU) {
> +            vfio_spapr_container_deinit(container);
> +        }
>       }
>   
>       if (ioctl(group->fd, VFIO_GROUP_UNSET_CONTAINER, &container->fd)) {
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 9a7517c042..fd0613dfde 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -15,6 +15,7 @@
>   #include <linux/kvm.h>
>   #endif
>   #include "sysemu/kvm.h"
> +#include "exec/address-spaces.h"
>   
>   #include "hw/vfio/vfio-common.h"
>   #include "hw/hw.h"
> @@ -139,7 +140,7 @@ static void vfio_prereg_listener_region_del(MemoryListener *listener,
>       trace_vfio_prereg_unregister(reg.vaddr, reg.size, ret ? -errno : 0);
>   }
>   
> -const MemoryListener vfio_prereg_listener = {
> +static const MemoryListener vfio_prereg_listener = {
>       .name = "vfio-pre-reg",
>       .region_add = vfio_prereg_listener_region_add,
>       .region_del = vfio_prereg_listener_region_del,
> @@ -343,3 +344,80 @@ void vfio_container_del_section_window(VFIOContainer *container,
>                    __func__, section->offset_within_address_space);
>       }
>   }
> +
> +bool vfio_spapr_container_init(VFIOContainer *container, Error **errp)
> +{
> +
> +    struct vfio_iommu_spapr_tce_info info;
> +    bool v2 = container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU;
> +    int ret, fd = container->fd;
> +
> +    /*
> +     * The host kernel code implementing VFIO_IOMMU_DISABLE is called
> +     * when container fd is closed so we do not call it explicitly
> +     * in this file.
> +     */
> +    if (!v2) {
> +        ret = ioctl(fd, VFIO_IOMMU_ENABLE);
> +        if (ret) {
> +            error_setg_errno(errp, errno, "failed to enable container");
> +            return -errno;
> +        }
> +    } else {
> +        container->prereg_listener = vfio_prereg_listener;
> +
> +        memory_listener_register(&container->prereg_listener,
> +                                 &address_space_memory);
> +        if (container->error) {
> +            ret = -1;
> +            error_propagate_prepend(errp, container->error,
> +                    "RAM memory listener initialization failed: ");
> +            goto listener_unregister_exit;
> +        }
> +    }
> +
> +    info.argsz = sizeof(info);
> +    ret = ioctl(fd, VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info);
> +    if (ret) {
> +        error_setg_errno(errp, errno,
> +                         "VFIO_IOMMU_SPAPR_TCE_GET_INFO failed");
> +        ret = -errno;
> +        goto listener_unregister_exit;
> +    }
> +
> +    if (v2) {
> +        container->pgsizes = info.ddw.pgsizes;
> +        /*
> +         * There is a default window in just created container.
> +         * To make region_add/del simpler, we better remove this
> +         * window now and let those iommu_listener callbacks
> +         * create/remove them when needed.
> +         */
> +        ret = vfio_spapr_remove_window(container, info.dma32_window_start);
> +        if (ret) {
> +            error_setg_errno(errp, -ret,
> +                             "failed to remove existing window");
> +            goto listener_unregister_exit;
> +        }
> +    } else {
> +        /* The default table uses 4K pages */
> +        container->pgsizes = 0x1000;
> +        vfio_host_win_add(container, info.dma32_window_start,
> +                          info.dma32_window_start +
> +                          info.dma32_window_size - 1,
> +                          0x1000);
> +    }
> +
> +listener_unregister_exit:
> +    if (v2) {
> +        memory_listener_unregister(&container->prereg_listener);
> +    }
> +    return ret;
> +}
> +
> +void vfio_spapr_container_deinit(VFIOContainer *container)
> +{
> +    if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
> +        memory_listener_unregister(&container->prereg_listener);
> +    }
> +}