diff mbox series

[v3,10/37] vfio/container: Move space field to base container

Message ID 20231026103104.1686921-11-zhenzhong.duan@intel.com
State Handled Elsewhere
Headers show
Series [v3,01/37] vfio/container: Move IBM EEH related functions into spapr_pci_vfio.c | expand

Commit Message

Duan, Zhenzhong Oct. 26, 2023, 10:30 a.m. UTC 
From: Eric Auger <eric.auger@redhat.com>

Move the space field to the base object. Also the VFIOAddressSpace
now contains a list of base containers.

No fucntional change intended.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
[ clg: context changes ]
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 include/hw/vfio/vfio-common.h         |  8 --------
 include/hw/vfio/vfio-container-base.h |  9 +++++++++
 hw/ppc/spapr_pci_vfio.c               | 10 +++++-----
 hw/vfio/common.c                      |  4 ++--
 hw/vfio/container-base.c              |  6 +++++-
 hw/vfio/container.c                   | 18 +++++++++---------
 6 files changed, 30 insertions(+), 25 deletions(-)

Comments

Cédric Le Goater Oct. 27, 2023, 2:46 p.m. UTC | #1 
On 10/26/23 12:30, Zhenzhong Duan wrote:
> From: Eric Auger <eric.auger@redhat.com>
> 
> Move the space field to the base object. Also the VFIOAddressSpace
> now contains a list of base containers.
> 
> No fucntional change intended.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> [ clg: context changes ]
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   include/hw/vfio/vfio-common.h         |  8 --------
>   include/hw/vfio/vfio-container-base.h |  9 +++++++++
>   hw/ppc/spapr_pci_vfio.c               | 10 +++++-----
>   hw/vfio/common.c                      |  4 ++--
>   hw/vfio/container-base.c              |  6 +++++-
>   hw/vfio/container.c                   | 18 +++++++++---------
>   6 files changed, 30 insertions(+), 25 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index fcb4003a21..857d2b8076 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -73,17 +73,10 @@ typedef struct VFIOMigration {
>       bool initial_data_sent;
>   } VFIOMigration;
>   
> -typedef struct VFIOAddressSpace {
> -    AddressSpace *as;
> -    QLIST_HEAD(, VFIOContainer) containers;
> -    QLIST_ENTRY(VFIOAddressSpace) list;
> -} VFIOAddressSpace;
> -
>   struct VFIOGroup;
>   
>   typedef struct VFIOContainer {
>       VFIOContainerBase bcontainer;
> -    VFIOAddressSpace *space;
>       int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>       MemoryListener listener;
>       MemoryListener prereg_listener;
> @@ -98,7 +91,6 @@ typedef struct VFIOContainer {
>       QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
>       QLIST_HEAD(, VFIOGroup) group_list;
>       QLIST_HEAD(, VFIORamDiscardListener) vrdl_list;
> -    QLIST_ENTRY(VFIOContainer) next;
>       QLIST_HEAD(, VFIODevice) device_list;
>       GList *iova_ranges;
>   } VFIOContainer;
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index 71e1e4324e..a5fef3e6a8 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -38,12 +38,20 @@ typedef struct {
>       hwaddr pages;
>   } VFIOBitmap;
>   
> +typedef struct VFIOAddressSpace {
> +    AddressSpace *as;
> +    QLIST_HEAD(, VFIOContainerBase) containers;
> +    QLIST_ENTRY(VFIOAddressSpace) list;
> +} VFIOAddressSpace;
> +
>   /*
>    * This is the base object for vfio container backends
>    */
>   typedef struct VFIOContainerBase {
>       const VFIOIOMMUOps *ops;
> +    VFIOAddressSpace *space;
>       QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
> +    QLIST_ENTRY(VFIOContainerBase) next;
>   } VFIOContainerBase;
>   
>   typedef struct VFIOGuestIOMMU {
> @@ -62,6 +70,7 @@ int vfio_container_dma_unmap(VFIOContainerBase *bcontainer,
>                                IOMMUTLBEntry *iotlb);
>   
>   void vfio_container_init(VFIOContainerBase *bcontainer,
> +                         VFIOAddressSpace *space,
>                            const VFIOIOMMUOps *ops);
>   void vfio_container_destroy(VFIOContainerBase *bcontainer);
>   
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index f283f7e38d..d1d07bec46 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -84,27 +84,27 @@ static int vfio_eeh_container_op(VFIOContainer *container, uint32_t op)
>   static VFIOContainer *vfio_eeh_as_container(AddressSpace *as)
>   {
>       VFIOAddressSpace *space = vfio_get_address_space(as);
> -    VFIOContainer *container = NULL;
> +    VFIOContainerBase *bcontainer = NULL;
>   
>       if (QLIST_EMPTY(&space->containers)) {
>           /* No containers to act on */
>           goto out;
>       }
>   
> -    container = QLIST_FIRST(&space->containers);
> +    bcontainer = QLIST_FIRST(&space->containers);
>   
> -    if (QLIST_NEXT(container, next)) {
> +    if (QLIST_NEXT(bcontainer, next)) {
>           /*
>            * We don't yet have logic to synchronize EEH state across
>            * multiple containers
>            */
> -        container = NULL;
> +        bcontainer = NULL;
>           goto out;
>       }
>   
>   out:
>       vfio_put_address_space(space);
> -    return container;
> +    return container_of(bcontainer, VFIOContainer, bcontainer);
>   }
>   
>   static bool vfio_eeh_as_ok(AddressSpace *as)
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 4f130ad87c..f87a0dcec3 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -145,7 +145,7 @@ void vfio_unblock_multiple_devices_migration(void)
>   
>   bool vfio_viommu_preset(VFIODevice *vbasedev)
>   {
> -    return vbasedev->container->space->as != &address_space_memory;
> +    return vbasedev->container->bcontainer.space->as != &address_space_memory;
>   }
>   
>   static void vfio_set_migration_error(int err)
> @@ -924,7 +924,7 @@ static void vfio_dirty_tracking_init(VFIOContainer *container,
>       dirty.container = container;
>   
>       memory_listener_register(&dirty.listener,
> -                             container->space->as);
> +                             container->bcontainer.space->as);

Could we introduce an helper returning :

   container->bcontainer.space->as

The rest looks OK.

Thanks,

C.

>   
>       *ranges = dirty.ranges;
>   
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 6be7ce65ad..99b2957d7b 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -48,9 +48,11 @@ int vfio_container_dma_unmap(VFIOContainerBase *bcontainer,
>       return bcontainer->ops->dma_unmap(bcontainer, iova, size, iotlb);
>   }
>   
> -void vfio_container_init(VFIOContainerBase *bcontainer, const VFIOIOMMUOps *ops)
> +void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space,
> +                         const VFIOIOMMUOps *ops)
>   {
>       bcontainer->ops = ops;
> +    bcontainer->space = space;
>       QLIST_INIT(&bcontainer->giommu_list);
>   }
>   
> @@ -58,6 +60,8 @@ void vfio_container_destroy(VFIOContainerBase *bcontainer)
>   {
>       VFIOGuestIOMMU *giommu, *tmp;
>   
> +    QLIST_REMOVE(bcontainer, next);
> +
>       QLIST_FOREACH_SAFE(giommu, &bcontainer->giommu_list, giommu_next, tmp) {
>           memory_region_unregister_iommu_notifier(
>                   MEMORY_REGION(giommu->iommu_mr), &giommu->n);
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index b2b6e05287..761310fa51 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -514,7 +514,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>        * details once we know which type of IOMMU we are using.
>        */
>   
> -    QLIST_FOREACH(container, &space->containers, next) {
> +    QLIST_FOREACH(bcontainer, &space->containers, next) {
> +        container = container_of(bcontainer, VFIOContainer, bcontainer);
>           if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
>               ret = vfio_ram_block_discard_disable(container, true);
>               if (ret) {
> @@ -550,7 +551,6 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>       }
>   
>       container = g_malloc0(sizeof(*container));
> -    container->space = space;
>       container->fd = fd;
>       container->error = NULL;
>       container->dirty_pages_supported = false;
> @@ -558,7 +558,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>       container->iova_ranges = NULL;
>       QLIST_INIT(&container->vrdl_list);
>       bcontainer = &container->bcontainer;
> -    vfio_container_init(bcontainer, &vfio_legacy_ops);
> +    vfio_container_init(bcontainer, space, &vfio_legacy_ops);
>   
>       ret = vfio_init_container(container, group->fd, errp);
>       if (ret) {
> @@ -613,14 +613,15 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>       vfio_kvm_device_add_group(group);
>   
>       QLIST_INIT(&container->group_list);
> -    QLIST_INSERT_HEAD(&space->containers, container, next);
> +    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
>   
>       group->container = container;
>       QLIST_INSERT_HEAD(&container->group_list, group, container_next);
>   
>       container->listener = vfio_memory_listener;
>   
> -    memory_listener_register(&container->listener, container->space->as);
> +    memory_listener_register(&container->listener,
> +                             container->bcontainer.space->as);
>   
>       if (container->error) {
>           ret = -1;
> @@ -634,7 +635,7 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>       return 0;
>   listener_release_exit:
>       QLIST_REMOVE(group, container_next);
> -    QLIST_REMOVE(container, next);
> +    QLIST_REMOVE(bcontainer, next);
>       vfio_kvm_device_del_group(group);
>       memory_listener_unregister(&container->listener);
>       if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU ||
> @@ -684,9 +685,8 @@ static void vfio_disconnect_container(VFIOGroup *group)
>       }
>   
>       if (QLIST_EMPTY(&container->group_list)) {
> -        VFIOAddressSpace *space = container->space;
> +        VFIOAddressSpace *space = container->bcontainer.space;
>   
> -        QLIST_REMOVE(container, next);
>           vfio_container_destroy(bcontainer);
>   
>           trace_vfio_disconnect_container(container->fd);
> @@ -706,7 +706,7 @@ static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
>       QLIST_FOREACH(group, &vfio_group_list, next) {
>           if (group->groupid == groupid) {
>               /* Found it.  Now is it already in the right context? */
> -            if (group->container->space->as == as) {
> +            if (group->container->bcontainer.space->as == as) {
>                   return group;
>               } else {
>                   error_setg(errp, "group %d used in multiple address spaces",
Duan, Zhenzhong Oct. 30, 2023, 2:51 a.m. UTC | #2 
>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Friday, October 27, 2023 10:46 PM
>Subject: Re: [PATCH v3 10/37] vfio/container: Move space field to base container
>
>On 10/26/23 12:30, Zhenzhong Duan wrote:
>> From: Eric Auger <eric.auger@redhat.com>
>>
>> Move the space field to the base object. Also the VFIOAddressSpace
>> now contains a list of base containers.
>>
>> No fucntional change intended.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> [ clg: context changes ]
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   include/hw/vfio/vfio-common.h         |  8 --------
>>   include/hw/vfio/vfio-container-base.h |  9 +++++++++
>>   hw/ppc/spapr_pci_vfio.c               | 10 +++++-----
>>   hw/vfio/common.c                      |  4 ++--
>>   hw/vfio/container-base.c              |  6 +++++-
>>   hw/vfio/container.c                   | 18 +++++++++---------
>>   6 files changed, 30 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index fcb4003a21..857d2b8076 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -73,17 +73,10 @@ typedef struct VFIOMigration {
>>       bool initial_data_sent;
>>   } VFIOMigration;
>>
>> -typedef struct VFIOAddressSpace {
>> -    AddressSpace *as;
>> -    QLIST_HEAD(, VFIOContainer) containers;
>> -    QLIST_ENTRY(VFIOAddressSpace) list;
>> -} VFIOAddressSpace;
>> -
>>   struct VFIOGroup;
>>
>>   typedef struct VFIOContainer {
>>       VFIOContainerBase bcontainer;
>> -    VFIOAddressSpace *space;
>>       int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>       MemoryListener listener;
>>       MemoryListener prereg_listener;
>> @@ -98,7 +91,6 @@ typedef struct VFIOContainer {
>>       QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
>>       QLIST_HEAD(, VFIOGroup) group_list;
>>       QLIST_HEAD(, VFIORamDiscardListener) vrdl_list;
>> -    QLIST_ENTRY(VFIOContainer) next;
>>       QLIST_HEAD(, VFIODevice) device_list;
>>       GList *iova_ranges;
>>   } VFIOContainer;
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>> index 71e1e4324e..a5fef3e6a8 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -38,12 +38,20 @@ typedef struct {
>>       hwaddr pages;
>>   } VFIOBitmap;
>>
>> +typedef struct VFIOAddressSpace {
>> +    AddressSpace *as;
>> +    QLIST_HEAD(, VFIOContainerBase) containers;
>> +    QLIST_ENTRY(VFIOAddressSpace) list;
>> +} VFIOAddressSpace;
>> +
>>   /*
>>    * This is the base object for vfio container backends
>>    */
>>   typedef struct VFIOContainerBase {
>>       const VFIOIOMMUOps *ops;
>> +    VFIOAddressSpace *space;
>>       QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
>> +    QLIST_ENTRY(VFIOContainerBase) next;
>>   } VFIOContainerBase;
>>
>>   typedef struct VFIOGuestIOMMU {
>> @@ -62,6 +70,7 @@ int vfio_container_dma_unmap(VFIOContainerBase
>*bcontainer,
>>                                IOMMUTLBEntry *iotlb);
>>
>>   void vfio_container_init(VFIOContainerBase *bcontainer,
>> +                         VFIOAddressSpace *space,
>>                            const VFIOIOMMUOps *ops);
>>   void vfio_container_destroy(VFIOContainerBase *bcontainer);
>>
>> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
>> index f283f7e38d..d1d07bec46 100644
>> --- a/hw/ppc/spapr_pci_vfio.c
>> +++ b/hw/ppc/spapr_pci_vfio.c
>> @@ -84,27 +84,27 @@ static int vfio_eeh_container_op(VFIOContainer
>*container, uint32_t op)
>>   static VFIOContainer *vfio_eeh_as_container(AddressSpace *as)
>>   {
>>       VFIOAddressSpace *space = vfio_get_address_space(as);
>> -    VFIOContainer *container = NULL;
>> +    VFIOContainerBase *bcontainer = NULL;
>>
>>       if (QLIST_EMPTY(&space->containers)) {
>>           /* No containers to act on */
>>           goto out;
>>       }
>>
>> -    container = QLIST_FIRST(&space->containers);
>> +    bcontainer = QLIST_FIRST(&space->containers);
>>
>> -    if (QLIST_NEXT(container, next)) {
>> +    if (QLIST_NEXT(bcontainer, next)) {
>>           /*
>>            * We don't yet have logic to synchronize EEH state across
>>            * multiple containers
>>            */
>> -        container = NULL;
>> +        bcontainer = NULL;
>>           goto out;
>>       }
>>
>>   out:
>>       vfio_put_address_space(space);
>> -    return container;
>> +    return container_of(bcontainer, VFIOContainer, bcontainer);
>>   }
>>
>>   static bool vfio_eeh_as_ok(AddressSpace *as)
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 4f130ad87c..f87a0dcec3 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -145,7 +145,7 @@ void vfio_unblock_multiple_devices_migration(void)
>>
>>   bool vfio_viommu_preset(VFIODevice *vbasedev)
>>   {
>> -    return vbasedev->container->space->as != &address_space_memory;
>> +    return vbasedev->container->bcontainer.space->as !=
>&address_space_memory;
>>   }
>>
>>   static void vfio_set_migration_error(int err)
>> @@ -924,7 +924,7 @@ static void vfio_dirty_tracking_init(VFIOContainer
>*container,
>>       dirty.container = container;
>>
>>       memory_listener_register(&dirty.listener,
>> -                             container->space->as);
>> +                             container->bcontainer.space->as);
>
>Could we introduce an helper returning :
>
>   container->bcontainer.space->as

Guess you mean this is too long? Then I would like to use
bcontainer->space->as except you have a strong opinion on that.

Thanks
Zhenzhong

>
>The rest looks OK.
>
>Thanks,
>
>C.
>
>>
>>       *ranges = dirty.ranges;
>>
>> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
>> index 6be7ce65ad..99b2957d7b 100644
>> --- a/hw/vfio/container-base.c
>> +++ b/hw/vfio/container-base.c
>> @@ -48,9 +48,11 @@ int vfio_container_dma_unmap(VFIOContainerBase
>*bcontainer,
>>       return bcontainer->ops->dma_unmap(bcontainer, iova, size, iotlb);
>>   }
>>
>> -void vfio_container_init(VFIOContainerBase *bcontainer, const
>VFIOIOMMUOps *ops)
>> +void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace
>*space,
>> +                         const VFIOIOMMUOps *ops)
>>   {
>>       bcontainer->ops = ops;
>> +    bcontainer->space = space;
>>       QLIST_INIT(&bcontainer->giommu_list);
>>   }
>>
>> @@ -58,6 +60,8 @@ void vfio_container_destroy(VFIOContainerBase
>*bcontainer)
>>   {
>>       VFIOGuestIOMMU *giommu, *tmp;
>>
>> +    QLIST_REMOVE(bcontainer, next);
>> +
>>       QLIST_FOREACH_SAFE(giommu, &bcontainer->giommu_list, giommu_next,
>tmp) {
>>           memory_region_unregister_iommu_notifier(
>>                   MEMORY_REGION(giommu->iommu_mr), &giommu->n);
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index b2b6e05287..761310fa51 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -514,7 +514,8 @@ static int vfio_connect_container(VFIOGroup *group,
>AddressSpace *as,
>>        * details once we know which type of IOMMU we are using.
>>        */
>>
>> -    QLIST_FOREACH(container, &space->containers, next) {
>> +    QLIST_FOREACH(bcontainer, &space->containers, next) {
>> +        container = container_of(bcontainer, VFIOContainer, bcontainer);
>>           if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
>>               ret = vfio_ram_block_discard_disable(container, true);
>>               if (ret) {
>> @@ -550,7 +551,6 @@ static int vfio_connect_container(VFIOGroup *group,
>AddressSpace *as,
>>       }
>>
>>       container = g_malloc0(sizeof(*container));
>> -    container->space = space;
>>       container->fd = fd;
>>       container->error = NULL;
>>       container->dirty_pages_supported = false;
>> @@ -558,7 +558,7 @@ static int vfio_connect_container(VFIOGroup *group,
>AddressSpace *as,
>>       container->iova_ranges = NULL;
>>       QLIST_INIT(&container->vrdl_list);
>>       bcontainer = &container->bcontainer;
>> -    vfio_container_init(bcontainer, &vfio_legacy_ops);
>> +    vfio_container_init(bcontainer, space, &vfio_legacy_ops);
>>
>>       ret = vfio_init_container(container, group->fd, errp);
>>       if (ret) {
>> @@ -613,14 +613,15 @@ static int vfio_connect_container(VFIOGroup *group,
>AddressSpace *as,
>>       vfio_kvm_device_add_group(group);
>>
>>       QLIST_INIT(&container->group_list);
>> -    QLIST_INSERT_HEAD(&space->containers, container, next);
>> +    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
>>
>>       group->container = container;
>>       QLIST_INSERT_HEAD(&container->group_list, group, container_next);
>>
>>       container->listener = vfio_memory_listener;
>>
>> -    memory_listener_register(&container->listener, container->space->as);
>> +    memory_listener_register(&container->listener,
>> +                             container->bcontainer.space->as);
>>
>>       if (container->error) {
>>           ret = -1;
>> @@ -634,7 +635,7 @@ static int vfio_connect_container(VFIOGroup *group,
>AddressSpace *as,
>>       return 0;
>>   listener_release_exit:
>>       QLIST_REMOVE(group, container_next);
>> -    QLIST_REMOVE(container, next);
>> +    QLIST_REMOVE(bcontainer, next);
>>       vfio_kvm_device_del_group(group);
>>       memory_listener_unregister(&container->listener);
>>       if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU ||
>> @@ -684,9 +685,8 @@ static void vfio_disconnect_container(VFIOGroup
>*group)
>>       }
>>
>>       if (QLIST_EMPTY(&container->group_list)) {
>> -        VFIOAddressSpace *space = container->space;
>> +        VFIOAddressSpace *space = container->bcontainer.space;
>>
>> -        QLIST_REMOVE(container, next);
>>           vfio_container_destroy(bcontainer);
>>
>>           trace_vfio_disconnect_container(container->fd);
>> @@ -706,7 +706,7 @@ static VFIOGroup *vfio_get_group(int groupid,
>AddressSpace *as, Error **errp)
>>       QLIST_FOREACH(group, &vfio_group_list, next) {
>>           if (group->groupid == groupid) {
>>               /* Found it.  Now is it already in the right context? */
>> -            if (group->container->space->as == as) {
>> +            if (group->container->bcontainer.space->as == as) {
>>                   return group;
>>               } else {
>>                   error_setg(errp, "group %d used in multiple address spaces",
Cédric Le Goater Oct. 31, 2023, 8:24 a.m. UTC | #3 
On 10/30/23 03:51, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Friday, October 27, 2023 10:46 PM
>> Subject: Re: [PATCH v3 10/37] vfio/container: Move space field to base container
>>
>> On 10/26/23 12:30, Zhenzhong Duan wrote:
>>> From: Eric Auger <eric.auger@redhat.com>
>>>
>>> Move the space field to the base object. Also the VFIOAddressSpace
>>> now contains a list of base containers.
>>>
>>> No fucntional change intended.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> [ clg: context changes ]
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>    include/hw/vfio/vfio-common.h         |  8 --------
>>>    include/hw/vfio/vfio-container-base.h |  9 +++++++++
>>>    hw/ppc/spapr_pci_vfio.c               | 10 +++++-----
>>>    hw/vfio/common.c                      |  4 ++--
>>>    hw/vfio/container-base.c              |  6 +++++-
>>>    hw/vfio/container.c                   | 18 +++++++++---------
>>>    6 files changed, 30 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>> index fcb4003a21..857d2b8076 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -73,17 +73,10 @@ typedef struct VFIOMigration {
>>>        bool initial_data_sent;
>>>    } VFIOMigration;
>>>
>>> -typedef struct VFIOAddressSpace {
>>> -    AddressSpace *as;
>>> -    QLIST_HEAD(, VFIOContainer) containers;
>>> -    QLIST_ENTRY(VFIOAddressSpace) list;
>>> -} VFIOAddressSpace;
>>> -
>>>    struct VFIOGroup;
>>>
>>>    typedef struct VFIOContainer {
>>>        VFIOContainerBase bcontainer;
>>> -    VFIOAddressSpace *space;
>>>        int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>>        MemoryListener listener;
>>>        MemoryListener prereg_listener;
>>> @@ -98,7 +91,6 @@ typedef struct VFIOContainer {
>>>        QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
>>>        QLIST_HEAD(, VFIOGroup) group_list;
>>>        QLIST_HEAD(, VFIORamDiscardListener) vrdl_list;
>>> -    QLIST_ENTRY(VFIOContainer) next;
>>>        QLIST_HEAD(, VFIODevice) device_list;
>>>        GList *iova_ranges;
>>>    } VFIOContainer;
>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>> container-base.h
>>> index 71e1e4324e..a5fef3e6a8 100644
>>> --- a/include/hw/vfio/vfio-container-base.h
>>> +++ b/include/hw/vfio/vfio-container-base.h
>>> @@ -38,12 +38,20 @@ typedef struct {
>>>        hwaddr pages;
>>>    } VFIOBitmap;
>>>
>>> +typedef struct VFIOAddressSpace {
>>> +    AddressSpace *as;
>>> +    QLIST_HEAD(, VFIOContainerBase) containers;
>>> +    QLIST_ENTRY(VFIOAddressSpace) list;
>>> +} VFIOAddressSpace;
>>> +
>>>    /*
>>>     * This is the base object for vfio container backends
>>>     */
>>>    typedef struct VFIOContainerBase {
>>>        const VFIOIOMMUOps *ops;
>>> +    VFIOAddressSpace *space;
>>>        QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
>>> +    QLIST_ENTRY(VFIOContainerBase) next;
>>>    } VFIOContainerBase;
>>>
>>>    typedef struct VFIOGuestIOMMU {
>>> @@ -62,6 +70,7 @@ int vfio_container_dma_unmap(VFIOContainerBase
>> *bcontainer,
>>>                                 IOMMUTLBEntry *iotlb);
>>>
>>>    void vfio_container_init(VFIOContainerBase *bcontainer,
>>> +                         VFIOAddressSpace *space,
>>>                             const VFIOIOMMUOps *ops);
>>>    void vfio_container_destroy(VFIOContainerBase *bcontainer);
>>>
>>> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
>>> index f283f7e38d..d1d07bec46 100644
>>> --- a/hw/ppc/spapr_pci_vfio.c
>>> +++ b/hw/ppc/spapr_pci_vfio.c
>>> @@ -84,27 +84,27 @@ static int vfio_eeh_container_op(VFIOContainer
>> *container, uint32_t op)
>>>    static VFIOContainer *vfio_eeh_as_container(AddressSpace *as)
>>>    {
>>>        VFIOAddressSpace *space = vfio_get_address_space(as);
>>> -    VFIOContainer *container = NULL;
>>> +    VFIOContainerBase *bcontainer = NULL;
>>>
>>>        if (QLIST_EMPTY(&space->containers)) {
>>>            /* No containers to act on */
>>>            goto out;
>>>        }
>>>
>>> -    container = QLIST_FIRST(&space->containers);
>>> +    bcontainer = QLIST_FIRST(&space->containers);
>>>
>>> -    if (QLIST_NEXT(container, next)) {
>>> +    if (QLIST_NEXT(bcontainer, next)) {
>>>            /*
>>>             * We don't yet have logic to synchronize EEH state across
>>>             * multiple containers
>>>             */
>>> -        container = NULL;
>>> +        bcontainer = NULL;
>>>            goto out;
>>>        }
>>>
>>>    out:
>>>        vfio_put_address_space(space);
>>> -    return container;
>>> +    return container_of(bcontainer, VFIOContainer, bcontainer);
>>>    }
>>>
>>>    static bool vfio_eeh_as_ok(AddressSpace *as)
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 4f130ad87c..f87a0dcec3 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -145,7 +145,7 @@ void vfio_unblock_multiple_devices_migration(void)
>>>
>>>    bool vfio_viommu_preset(VFIODevice *vbasedev)
>>>    {
>>> -    return vbasedev->container->space->as != &address_space_memory;
>>> +    return vbasedev->container->bcontainer.space->as !=
>> &address_space_memory;
>>>    }
>>>
>>>    static void vfio_set_migration_error(int err)
>>> @@ -924,7 +924,7 @@ static void vfio_dirty_tracking_init(VFIOContainer
>> *container,
>>>        dirty.container = container;
>>>
>>>        memory_listener_register(&dirty.listener,
>>> -                             container->space->as);
>>> +                             container->bcontainer.space->as);
>>
>> Could we introduce an helper returning :
>>
>>    container->bcontainer.space->as
> 
> Guess you mean this is too long? 

yes and I like helpers but it is a personal taste.

> Then I would like to use
> bcontainer->space->as except you have a strong opinion on that.

I don't mind.

Thanks,

C.



> 
> Thanks
> Zhenzhong
> 
>>
>> The rest looks OK.
>>
>> Thanks,
>>
>> C.
>>
>>>
>>>        *ranges = dirty.ranges;
>>>
>>> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
>>> index 6be7ce65ad..99b2957d7b 100644
>>> --- a/hw/vfio/container-base.c
>>> +++ b/hw/vfio/container-base.c
>>> @@ -48,9 +48,11 @@ int vfio_container_dma_unmap(VFIOContainerBase
>> *bcontainer,
>>>        return bcontainer->ops->dma_unmap(bcontainer, iova, size, iotlb);
>>>    }
>>>
>>> -void vfio_container_init(VFIOContainerBase *bcontainer, const
>> VFIOIOMMUOps *ops)
>>> +void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace
>> *space,
>>> +                         const VFIOIOMMUOps *ops)
>>>    {
>>>        bcontainer->ops = ops;
>>> +    bcontainer->space = space;
>>>        QLIST_INIT(&bcontainer->giommu_list);
>>>    }
>>>
>>> @@ -58,6 +60,8 @@ void vfio_container_destroy(VFIOContainerBase
>> *bcontainer)
>>>    {
>>>        VFIOGuestIOMMU *giommu, *tmp;
>>>
>>> +    QLIST_REMOVE(bcontainer, next);
>>> +
>>>        QLIST_FOREACH_SAFE(giommu, &bcontainer->giommu_list, giommu_next,
>> tmp) {
>>>            memory_region_unregister_iommu_notifier(
>>>                    MEMORY_REGION(giommu->iommu_mr), &giommu->n);
>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>> index b2b6e05287..761310fa51 100644
>>> --- a/hw/vfio/container.c
>>> +++ b/hw/vfio/container.c
>>> @@ -514,7 +514,8 @@ static int vfio_connect_container(VFIOGroup *group,
>> AddressSpace *as,
>>>         * details once we know which type of IOMMU we are using.
>>>         */
>>>
>>> -    QLIST_FOREACH(container, &space->containers, next) {
>>> +    QLIST_FOREACH(bcontainer, &space->containers, next) {
>>> +        container = container_of(bcontainer, VFIOContainer, bcontainer);
>>>            if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
>>>                ret = vfio_ram_block_discard_disable(container, true);
>>>                if (ret) {
>>> @@ -550,7 +551,6 @@ static int vfio_connect_container(VFIOGroup *group,
>> AddressSpace *as,
>>>        }
>>>
>>>        container = g_malloc0(sizeof(*container));
>>> -    container->space = space;
>>>        container->fd = fd;
>>>        container->error = NULL;
>>>        container->dirty_pages_supported = false;
>>> @@ -558,7 +558,7 @@ static int vfio_connect_container(VFIOGroup *group,
>> AddressSpace *as,
>>>        container->iova_ranges = NULL;
>>>        QLIST_INIT(&container->vrdl_list);
>>>        bcontainer = &container->bcontainer;
>>> -    vfio_container_init(bcontainer, &vfio_legacy_ops);
>>> +    vfio_container_init(bcontainer, space, &vfio_legacy_ops);
>>>
>>>        ret = vfio_init_container(container, group->fd, errp);
>>>        if (ret) {
>>> @@ -613,14 +613,15 @@ static int vfio_connect_container(VFIOGroup *group,
>> AddressSpace *as,
>>>        vfio_kvm_device_add_group(group);
>>>
>>>        QLIST_INIT(&container->group_list);
>>> -    QLIST_INSERT_HEAD(&space->containers, container, next);
>>> +    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
>>>
>>>        group->container = container;
>>>        QLIST_INSERT_HEAD(&container->group_list, group, container_next);
>>>
>>>        container->listener = vfio_memory_listener;
>>>
>>> -    memory_listener_register(&container->listener, container->space->as);
>>> +    memory_listener_register(&container->listener,
>>> +                             container->bcontainer.space->as);
>>>
>>>        if (container->error) {
>>>            ret = -1;
>>> @@ -634,7 +635,7 @@ static int vfio_connect_container(VFIOGroup *group,
>> AddressSpace *as,
>>>        return 0;
>>>    listener_release_exit:
>>>        QLIST_REMOVE(group, container_next);
>>> -    QLIST_REMOVE(container, next);
>>> +    QLIST_REMOVE(bcontainer, next);
>>>        vfio_kvm_device_del_group(group);
>>>        memory_listener_unregister(&container->listener);
>>>        if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU ||
>>> @@ -684,9 +685,8 @@ static void vfio_disconnect_container(VFIOGroup
>> *group)
>>>        }
>>>
>>>        if (QLIST_EMPTY(&container->group_list)) {
>>> -        VFIOAddressSpace *space = container->space;
>>> +        VFIOAddressSpace *space = container->bcontainer.space;
>>>
>>> -        QLIST_REMOVE(container, next);
>>>            vfio_container_destroy(bcontainer);
>>>
>>>            trace_vfio_disconnect_container(container->fd);
>>> @@ -706,7 +706,7 @@ static VFIOGroup *vfio_get_group(int groupid,
>> AddressSpace *as, Error **errp)
>>>        QLIST_FOREACH(group, &vfio_group_list, next) {
>>>            if (group->groupid == groupid) {
>>>                /* Found it.  Now is it already in the right context? */
>>> -            if (group->container->space->as == as) {
>>> +            if (group->container->bcontainer.space->as == as) {
>>>                    return group;
>>>                } else {
>>>                    error_setg(errp, "group %d used in multiple address spaces",
>
diff mbox series

Patch

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index fcb4003a21..857d2b8076 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -73,17 +73,10 @@  typedef struct VFIOMigration {
     bool initial_data_sent;
 } VFIOMigration;
 
-typedef struct VFIOAddressSpace {
-    AddressSpace *as;
-    QLIST_HEAD(, VFIOContainer) containers;
-    QLIST_ENTRY(VFIOAddressSpace) list;
-} VFIOAddressSpace;
-
 struct VFIOGroup;
 
 typedef struct VFIOContainer {
     VFIOContainerBase bcontainer;
-    VFIOAddressSpace *space;
     int fd; /* /dev/vfio/vfio, empowered by the attached groups */
     MemoryListener listener;
     MemoryListener prereg_listener;
@@ -98,7 +91,6 @@  typedef struct VFIOContainer {
     QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
     QLIST_HEAD(, VFIOGroup) group_list;
     QLIST_HEAD(, VFIORamDiscardListener) vrdl_list;
-    QLIST_ENTRY(VFIOContainer) next;
     QLIST_HEAD(, VFIODevice) device_list;
     GList *iova_ranges;
 } VFIOContainer;
diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index 71e1e4324e..a5fef3e6a8 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -38,12 +38,20 @@  typedef struct {
     hwaddr pages;
 } VFIOBitmap;
 
+typedef struct VFIOAddressSpace {
+    AddressSpace *as;
+    QLIST_HEAD(, VFIOContainerBase) containers;
+    QLIST_ENTRY(VFIOAddressSpace) list;
+} VFIOAddressSpace;
+
 /*
  * This is the base object for vfio container backends
  */
 typedef struct VFIOContainerBase {
     const VFIOIOMMUOps *ops;
+    VFIOAddressSpace *space;
     QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
+    QLIST_ENTRY(VFIOContainerBase) next;
 } VFIOContainerBase;
 
 typedef struct VFIOGuestIOMMU {
@@ -62,6 +70,7 @@  int vfio_container_dma_unmap(VFIOContainerBase *bcontainer,
                              IOMMUTLBEntry *iotlb);
 
 void vfio_container_init(VFIOContainerBase *bcontainer,
+                         VFIOAddressSpace *space,
                          const VFIOIOMMUOps *ops);
 void vfio_container_destroy(VFIOContainerBase *bcontainer);
 
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index f283f7e38d..d1d07bec46 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -84,27 +84,27 @@  static int vfio_eeh_container_op(VFIOContainer *container, uint32_t op)
 static VFIOContainer *vfio_eeh_as_container(AddressSpace *as)
 {
     VFIOAddressSpace *space = vfio_get_address_space(as);
-    VFIOContainer *container = NULL;
+    VFIOContainerBase *bcontainer = NULL;
 
     if (QLIST_EMPTY(&space->containers)) {
         /* No containers to act on */
         goto out;
     }
 
-    container = QLIST_FIRST(&space->containers);
+    bcontainer = QLIST_FIRST(&space->containers);
 
-    if (QLIST_NEXT(container, next)) {
+    if (QLIST_NEXT(bcontainer, next)) {
         /*
          * We don't yet have logic to synchronize EEH state across
          * multiple containers
          */
-        container = NULL;
+        bcontainer = NULL;
         goto out;
     }
 
 out:
     vfio_put_address_space(space);
-    return container;
+    return container_of(bcontainer, VFIOContainer, bcontainer);
 }
 
 static bool vfio_eeh_as_ok(AddressSpace *as)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 4f130ad87c..f87a0dcec3 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -145,7 +145,7 @@  void vfio_unblock_multiple_devices_migration(void)
 
 bool vfio_viommu_preset(VFIODevice *vbasedev)
 {
-    return vbasedev->container->space->as != &address_space_memory;
+    return vbasedev->container->bcontainer.space->as != &address_space_memory;
 }
 
 static void vfio_set_migration_error(int err)
@@ -924,7 +924,7 @@  static void vfio_dirty_tracking_init(VFIOContainer *container,
     dirty.container = container;
 
     memory_listener_register(&dirty.listener,
-                             container->space->as);
+                             container->bcontainer.space->as);
 
     *ranges = dirty.ranges;
 
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 6be7ce65ad..99b2957d7b 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -48,9 +48,11 @@  int vfio_container_dma_unmap(VFIOContainerBase *bcontainer,
     return bcontainer->ops->dma_unmap(bcontainer, iova, size, iotlb);
 }
 
-void vfio_container_init(VFIOContainerBase *bcontainer, const VFIOIOMMUOps *ops)
+void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space,
+                         const VFIOIOMMUOps *ops)
 {
     bcontainer->ops = ops;
+    bcontainer->space = space;
     QLIST_INIT(&bcontainer->giommu_list);
 }
 
@@ -58,6 +60,8 @@  void vfio_container_destroy(VFIOContainerBase *bcontainer)
 {
     VFIOGuestIOMMU *giommu, *tmp;
 
+    QLIST_REMOVE(bcontainer, next);
+
     QLIST_FOREACH_SAFE(giommu, &bcontainer->giommu_list, giommu_next, tmp) {
         memory_region_unregister_iommu_notifier(
                 MEMORY_REGION(giommu->iommu_mr), &giommu->n);
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index b2b6e05287..761310fa51 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -514,7 +514,8 @@  static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
      * details once we know which type of IOMMU we are using.
      */
 
-    QLIST_FOREACH(container, &space->containers, next) {
+    QLIST_FOREACH(bcontainer, &space->containers, next) {
+        container = container_of(bcontainer, VFIOContainer, bcontainer);
         if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
             ret = vfio_ram_block_discard_disable(container, true);
             if (ret) {
@@ -550,7 +551,6 @@  static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     }
 
     container = g_malloc0(sizeof(*container));
-    container->space = space;
     container->fd = fd;
     container->error = NULL;
     container->dirty_pages_supported = false;
@@ -558,7 +558,7 @@  static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     container->iova_ranges = NULL;
     QLIST_INIT(&container->vrdl_list);
     bcontainer = &container->bcontainer;
-    vfio_container_init(bcontainer, &vfio_legacy_ops);
+    vfio_container_init(bcontainer, space, &vfio_legacy_ops);
 
     ret = vfio_init_container(container, group->fd, errp);
     if (ret) {
@@ -613,14 +613,15 @@  static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     vfio_kvm_device_add_group(group);
 
     QLIST_INIT(&container->group_list);
-    QLIST_INSERT_HEAD(&space->containers, container, next);
+    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
 
     group->container = container;
     QLIST_INSERT_HEAD(&container->group_list, group, container_next);
 
     container->listener = vfio_memory_listener;
 
-    memory_listener_register(&container->listener, container->space->as);
+    memory_listener_register(&container->listener,
+                             container->bcontainer.space->as);
 
     if (container->error) {
         ret = -1;
@@ -634,7 +635,7 @@  static int vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     return 0;
 listener_release_exit:
     QLIST_REMOVE(group, container_next);
-    QLIST_REMOVE(container, next);
+    QLIST_REMOVE(bcontainer, next);
     vfio_kvm_device_del_group(group);
     memory_listener_unregister(&container->listener);
     if (container->iommu_type == VFIO_SPAPR_TCE_v2_IOMMU ||
@@ -684,9 +685,8 @@  static void vfio_disconnect_container(VFIOGroup *group)
     }
 
     if (QLIST_EMPTY(&container->group_list)) {
-        VFIOAddressSpace *space = container->space;
+        VFIOAddressSpace *space = container->bcontainer.space;
 
-        QLIST_REMOVE(container, next);
         vfio_container_destroy(bcontainer);
 
         trace_vfio_disconnect_container(container->fd);
@@ -706,7 +706,7 @@  static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
     QLIST_FOREACH(group, &vfio_group_list, next) {
         if (group->groupid == groupid) {
             /* Found it.  Now is it already in the right context? */
-            if (group->container->space->as == as) {
+            if (group->container->bcontainer.space->as == as) {
                 return group;
             } else {
                 error_setg(errp, "group %d used in multiple address spaces",