Re: [PATCH v2 06/17] vfio/container: Simplify vfio_container_init()

[Cédric Le Goater]

On 6/17/24 4:25 PM, Eric Auger wrote:

Hi Cédric,

On 6/17/24 08:33, Cédric Le Goater wrote:

Assign the base container VFIOAddressSpace 'space' pointer in
vfio_address_space_insert().

OK I get it now. Maybe in the previous patch, say that the

vfio_address_space_insert() will be enhanced with additional initializations.

To be noted that vfio_connect_container() will assign the 'space'
pointer later in the execution flow. This should not have any
consequence.

You do not explain the motivation of that change.

Changed to:

  Assign the base container VFIOAddressSpace 'space' pointer in
  vfio_address_space_insert(). The ultimate goal is to remove
  vfio_container_init() and instead rely on an .instance_init() handler
  to perfom the initialization of VFIOContainerBase.

  To be noted that vfio_connect_container() will assign the 'space'
  pointer later in the execution flow. This should not have any
  consequence.

Thanks,

C.

Signed-off-by: Cédric Le Goater <c...@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
---
  include/hw/vfio/vfio-container-base.h | 1 -
  hw/vfio/common.c                      | 1 +
  hw/vfio/container-base.c              | 3 +--
  hw/vfio/container.c                   | 6 +++---
  hw/vfio/iommufd.c                     | 2 +-
  5 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/hw/vfio/vfio-container-base.h 
b/include/hw/vfio/vfio-container-base.h
index 
442c0dfc4c1774753c239c2c8360dcd1540d44fa..d505f63607ec40e6aa44aeb3e20848ac780562a1
 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -87,7 +87,6 @@ int vfio_container_query_dirty_bitmap(const VFIOContainerBase 
*bcontainer,
                     VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp);

void vfio_container_init(VFIOContainerBase *bcontainer,

-                         VFIOAddressSpace *space,
                           const VFIOIOMMUClass *ops);
  void vfio_container_destroy(VFIOContainerBase *bcontainer);

diff --git a/hw/vfio/common.c b/hw/vfio/common.c

index 
8cdf26c6f5a490cfa02bdf1087a91948709aaa33..1686a0bed23bd95467bfb00a0c39a4d966e49cae
 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1512,6 +1512,7 @@ void vfio_address_space_insert(VFIOAddressSpace *space,
                                 VFIOContainerBase *bcontainer)
  {
      QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
+    bcontainer->space = space;
  }

struct vfio_device_info *vfio_get_device_info(int fd)

diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 
760d9d0622b2e847ecb3368c88df772efb06043f..280f0dd2db1fc3939fe9925ce00a2c50d0e14196
 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -71,11 +71,10 @@ int vfio_container_query_dirty_bitmap(const 
VFIOContainerBase *bcontainer,
                                                 errp);
  }

-void vfio_container_init(VFIOContainerBase *bcontainer, VFIOAddressSpace *space,

+void vfio_container_init(VFIOContainerBase *bcontainer,
                           const VFIOIOMMUClass *ops)
  {
      bcontainer->ops = ops;
-    bcontainer->space = space;
      bcontainer->error = NULL;
      bcontainer->dirty_pages_supported = false;
      bcontainer->dma_max_mappings = 0;
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 
0237c216987ff64a6d11bef8688bb000d93a7f09..dc85a79cb9e62b72312f79da994c53608b6cef48
 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -394,7 +394,7 @@ static const VFIOIOMMUClass *vfio_get_iommu_class(int 
iommu_type, Error **errp)
  }

static bool vfio_set_iommu(VFIOContainer *container, int group_fd,

-                           VFIOAddressSpace *space, Error **errp)
+                           Error **errp)
  {
      int iommu_type;
      const VFIOIOMMUClass *vioc;
@@ -432,7 +432,7 @@ static bool vfio_set_iommu(VFIOContainer *container, int 
group_fd,
          return false;
      }

- vfio_container_init(&container->bcontainer, space, vioc);

+    vfio_container_init(&container->bcontainer, vioc);
      return true;
  }

@@ -614,7 +614,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,

      container->fd = fd;
      bcontainer = &container->bcontainer;

- if (!vfio_set_iommu(container, group->fd, space, errp)) {

+    if (!vfio_set_iommu(container, group->fd, errp)) {
          goto free_container_exit;
      }

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c

index 
9f8f33e383a38827ceca0f73cb77f5ca6b123198..e5d9334142418514215528b9523f12c031792c7f
 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -357,7 +357,7 @@ static bool iommufd_cdev_attach(const char *name, 
VFIODevice *vbasedev,
      container->ioas_id = ioas_id;

bcontainer = &container->bcontainer;

-    vfio_container_init(bcontainer, space, iommufd_vioc);
+    vfio_container_init(bcontainer, iommufd_vioc);
      vfio_address_space_insert(space, bcontainer);

if (!iommufd_cdev_attach_container(vbasedev, container, errp)) {

Thanks

Eric