[PATCH v2 00/17] vfio: QOMify VFIOContainer

[PATCH v2 00/17] vfio: QOMify VFIOContainer

[Cédric Le Goater]

Hello,

The series starts with simple changes (patch 1-4). Two of which were
initially sent by Joao in a series adding VFIO migration support with
vIOMMU [1].

The changes following prepare VFIOContainer for QOMification, switch
the container models to QOM when ready and add some final cleanups.

Applies on top of :

 * [v7] Add a host IOMMU device abstraction to check with vIOMMU
   https://lore.kernel.org/all/20240605083043.317831-1-zhenzhong.duan@intel.com
 * [v4] VIRTIO-IOMMU/VFIO: Fix host iommu geometry
   https://lore.kernel.org/all/20240614095402.904691-1-eric.auger@redhat.com

Thanks,

C.

[1] https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.martins@oracle.com/


Changes in v2:
 - Used OBJECT_DECLARE_SIMPLE_TYPE
 - Introduced a instance_finalize() handler

Avihai Horon (1):
  vfio/common: Extract vIOMMU code from vfio_sync_dirty_bitmap()

Cédric Le Goater (15):
  vfio: Make vfio_devices_dma_logging_start() return bool
  vfio: Remove unused declarations from vfio-common.h
  vfio/container: Introduce vfio_address_space_insert()
  vfio/container: Simplify vfio_container_init()
  vfio/container: Modify vfio_get_iommu_type() to use a container fd
  vfio/container: Introduce vfio_get_iommu_class_name()
  vfio/container: Introduce vfio_create_container()
  vfio/container: Discover IOMMU type before creating the container
  vfio/container: Change VFIOContainerBase to use QOM
  vfio/container: Switch to QOM
  vfio/container: Introduce an instance_init() handler
  vfio/container: Remove VFIOContainerBase::ops
  vfio/container: Remove vfio_container_init()
  vfio/container: Introduce vfio_iommu_legacy_instance_init()
  vfio/container: Move vfio_container_destroy() to an
    instance_finalize() handler

Joao Martins (1):
  vfio/common: Move dirty tracking ranges update to helper

 include/hw/vfio/vfio-common.h         |  10 ++-
 include/hw/vfio/vfio-container-base.h |  19 +---
 hw/vfio/common.c                      | 124 ++++++++++++++++----------
 hw/vfio/container-base.c              |  70 +++++++++------
 hw/vfio/container.c                   | 106 ++++++++++++----------
 hw/vfio/iommufd.c                     |  13 ++-
 hw/vfio/pci.c                         |   4 +-
 hw/vfio/spapr.c                       |   3 +
 8 files changed, 196 insertions(+), 153 deletions(-)

-- 
2.45.2

[PATCH v2 01/17] vfio: Make vfio_devices_dma_logging_start() return bool

[Cédric Le Goater]

Since vfio_devices_dma_logging_start() takes an 'Error **' argument,
best practices suggest to return a bool. See the api/error.h Rules
section. It will simplify potential changes coming after.

vfio_container_set_dirty_page_tracking() could be modified in the same
way but the errno value can be saved in the migration stream when
called from vfio_listener_log_global_stop().

Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/common.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 9e4c0cc95ff90209d3e8184035af0806a2bf890b..d48cd9b9361a92d184e423ffc60aabaff40fb487 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1020,7 +1020,7 @@ static void vfio_device_feature_dma_logging_start_destroy(
     g_free(feature);
 }
 
-static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
+static bool vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
                                           Error **errp)
 {
     struct vfio_device_feature *feature;
@@ -1033,7 +1033,7 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
                                                            &ranges);
     if (!feature) {
         error_setg_errno(errp, errno, "Failed to prepare DMA logging");
-        return -errno;
+        return false;
     }
 
     QLIST_FOREACH(vbasedev, &bcontainer->device_list, container_next) {
@@ -1058,7 +1058,7 @@ out:
 
     vfio_device_feature_dma_logging_start_destroy(feature);
 
-    return ret;
+    return ret == 0;
 }
 
 static bool vfio_listener_log_global_start(MemoryListener *listener,
@@ -1067,18 +1067,18 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
     ERRP_GUARD();
     VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
                                                  listener);
-    int ret;
+    bool ret;
 
     if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
         ret = vfio_devices_dma_logging_start(bcontainer, errp);
     } else {
-        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
+        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp) == 0;
     }
 
-    if (ret) {
+    if (!ret) {
         error_prepend(errp, "vfio: Could not start dirty page tracking - ");
     }
-    return !ret;
+    return ret;
 }
 
 static void vfio_listener_log_global_stop(MemoryListener *listener)
-- 
2.45.2

[PATCH v2 02/17] vfio: Remove unused declarations from vfio-common.h

[Cédric Le Goater]

These were forgotten in the recent cleanups.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/vfio/vfio-common.h | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 776de8064f740784f95cab0311c5f15f50d60ffe..c19572f90b277193491020af28e8b5587f15bfd1 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -207,10 +207,6 @@ typedef struct VFIODisplay {
 VFIOAddressSpace *vfio_get_address_space(AddressSpace *as);
 void vfio_put_address_space(VFIOAddressSpace *space);
 
-/* SPAPR specific */
-int 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);
 void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
-- 
2.45.2

[PATCH v2 03/17] vfio/common: Move dirty tracking ranges update to helper

[Cédric Le Goater]

From: Joao Martins <joao.m.martins@oracle.com>

Separate the changes that updates the ranges from the listener, to
make it reusable in preparation to expand its use to vIOMMU support.

[ clg: - Rebased on upstream
       - Introduced vfio_dirty_tracking_update_range() ]

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/common.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index d48cd9b9361a92d184e423ffc60aabaff40fb487..fe215918bdf66ddbe3c5db803e10ce1aa9756b90 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -839,20 +839,11 @@ static bool vfio_section_is_vfio_pci(MemoryRegionSection *section,
     return false;
 }
 
-static void vfio_dirty_tracking_update(MemoryListener *listener,
-                                       MemoryRegionSection *section)
+static void vfio_dirty_tracking_update_range(VFIODirtyRanges *range,
+                                             hwaddr iova, hwaddr end,
+                                             bool update_pci)
 {
-    VFIODirtyRangesListener *dirty = container_of(listener,
-                                                  VFIODirtyRangesListener,
-                                                  listener);
-    VFIODirtyRanges *range = &dirty->ranges;
-    hwaddr iova, end, *min, *max;
-
-    if (!vfio_listener_valid_section(section, "tracking_update") ||
-        !vfio_get_section_iova_range(dirty->bcontainer, section,
-                                     &iova, &end, NULL)) {
-        return;
-    }
+    hwaddr *min, *max;
 
     /*
      * The address space passed to the dirty tracker is reduced to three ranges:
@@ -873,8 +864,7 @@ static void vfio_dirty_tracking_update(MemoryListener *listener,
      * The alternative would be an IOVATree but that has a much bigger runtime
      * overhead and unnecessary complexity.
      */
-    if (vfio_section_is_vfio_pci(section, dirty->bcontainer) &&
-        iova >= UINT32_MAX) {
+    if (update_pci && iova >= UINT32_MAX) {
         min = &range->minpci64;
         max = &range->maxpci64;
     } else {
@@ -889,7 +879,23 @@ static void vfio_dirty_tracking_update(MemoryListener *listener,
     }
 
     trace_vfio_device_dirty_tracking_update(iova, end, *min, *max);
-    return;
+}
+
+static void vfio_dirty_tracking_update(MemoryListener *listener,
+                                       MemoryRegionSection *section)
+{
+    VFIODirtyRangesListener *dirty =
+        container_of(listener, VFIODirtyRangesListener, listener);
+    hwaddr iova, end;
+
+    if (!vfio_listener_valid_section(section, "tracking_update") ||
+        !vfio_get_section_iova_range(dirty->bcontainer, section,
+                                     &iova, &end, NULL)) {
+        return;
+    }
+
+    vfio_dirty_tracking_update_range(&dirty->ranges, iova, end,
+                      vfio_section_is_vfio_pci(section, dirty->bcontainer));
 }
 
 static const MemoryListener vfio_dirty_tracking_listener = {
-- 
2.45.2

[PATCH v2 04/17] vfio/common: Extract vIOMMU code from vfio_sync_dirty_bitmap()

[Cédric Le Goater]

From: Avihai Horon <avihaih@nvidia.com>

Extract vIOMMU code from vfio_sync_dirty_bitmap() to a new function and
restructure the code.

This is done in preparation for optimizing vIOMMU deviice dirty page
tracking. No functional changes intended.

Signed-off-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
[ clg: - Rebased on upstream ]
Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/common.c | 63 +++++++++++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 25 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index fe215918bdf66ddbe3c5db803e10ce1aa9756b90..f28641bad5cf4b71fcdc0a6c9d42b24c8d786248 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1302,37 +1302,50 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer,
                                                 &vrdl);
 }
 
+static int vfio_sync_iommu_dirty_bitmap(VFIOContainerBase *bcontainer,
+                                        MemoryRegionSection *section)
+{
+    VFIOGuestIOMMU *giommu;
+    bool found = false;
+    Int128 llend;
+    vfio_giommu_dirty_notifier gdn;
+    int idx;
+
+    QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
+        if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
+            giommu->n.start == section->offset_within_region) {
+            found = true;
+            break;
+        }
+    }
+
+    if (!found) {
+        return 0;
+    }
+
+    gdn.giommu = giommu;
+    idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
+                                             MEMTXATTRS_UNSPECIFIED);
+
+    llend = int128_add(int128_make64(section->offset_within_region),
+                       section->size);
+    llend = int128_sub(llend, int128_one());
+
+    iommu_notifier_init(&gdn.n, vfio_iommu_map_dirty_notify, IOMMU_NOTIFIER_MAP,
+                        section->offset_within_region, int128_get64(llend),
+                        idx);
+    memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
+
+    return 0;
+}
+
 static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
                                   MemoryRegionSection *section, Error **errp)
 {
     ram_addr_t ram_addr;
 
     if (memory_region_is_iommu(section->mr)) {
-        VFIOGuestIOMMU *giommu;
-
-        QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
-            if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
-                giommu->n.start == section->offset_within_region) {
-                Int128 llend;
-                vfio_giommu_dirty_notifier gdn = { .giommu = giommu };
-                int idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
-                                                       MEMTXATTRS_UNSPECIFIED);
-
-                llend = int128_add(int128_make64(section->offset_within_region),
-                                   section->size);
-                llend = int128_sub(llend, int128_one());
-
-                iommu_notifier_init(&gdn.n,
-                                    vfio_iommu_map_dirty_notify,
-                                    IOMMU_NOTIFIER_MAP,
-                                    section->offset_within_region,
-                                    int128_get64(llend),
-                                    idx);
-                memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
-                break;
-            }
-        }
-        return 0;
+        return vfio_sync_iommu_dirty_bitmap(bcontainer, section);
     } else if (memory_region_has_ram_discard_manager(section->mr)) {
         int ret;
 
-- 
2.45.2

[PATCH v2 05/17] vfio/container: Introduce vfio_address_space_insert()

[Cédric Le Goater]

It will ease future changes.

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

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index c19572f90b277193491020af28e8b5587f15bfd1..825d80130bd435fe50830c8ae5b7905d18104dd6 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -206,6 +206,8 @@ typedef struct VFIODisplay {
 
 VFIOAddressSpace *vfio_get_address_space(AddressSpace *as);
 void vfio_put_address_space(VFIOAddressSpace *space);
+void vfio_address_space_insert(VFIOAddressSpace *space,
+                               VFIOContainerBase *bcontainer);
 
 void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
 void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index f28641bad5cf4b71fcdc0a6c9d42b24c8d786248..8cdf26c6f5a490cfa02bdf1087a91948709aaa33 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1508,6 +1508,12 @@ void vfio_put_address_space(VFIOAddressSpace *space)
     }
 }
 
+void vfio_address_space_insert(VFIOAddressSpace *space,
+                               VFIOContainerBase *bcontainer)
+{
+    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
+}
+
 struct vfio_device_info *vfio_get_device_info(int fd)
 {
     struct vfio_device_info *info;
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index c48749c089a67ee4d0e6b8dd975562e2938500cd..0237c216987ff64a6d11bef8688bb000d93a7f09 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -637,7 +637,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     vfio_kvm_device_add_group(group);
 
     QLIST_INIT(&container->group_list);
-    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
+    vfio_address_space_insert(space, bcontainer);
 
     group->container = container;
     QLIST_INSERT_HEAD(&container->group_list, group, container_next);
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index e502081c2ad9eda31769176f875fef60a77e2b43..9f8f33e383a38827ceca0f73cb77f5ca6b123198 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -358,7 +358,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
 
     bcontainer = &container->bcontainer;
     vfio_container_init(bcontainer, space, iommufd_vioc);
-    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
+    vfio_address_space_insert(space, bcontainer);
 
     if (!iommufd_cdev_attach_container(vbasedev, container, errp)) {
         goto err_attach_container;
-- 
2.45.2

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

[Cédric Le Goater]

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

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

Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@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)) {
-- 
2.45.2

[PATCH v2 07/17] vfio/container: Modify vfio_get_iommu_type() to use a container fd

[Cédric Le Goater]

The 'container' pointer has no other use than its 'fd' attribute.
Simplify the prototype to ease future changes.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/container.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index dc85a79cb9e62b72312f79da994c53608b6cef48..589f37bc6d68dae18f9e46371f14d6952b2240c0 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -354,7 +354,7 @@ static void vfio_kvm_device_del_group(VFIOGroup *group)
 /*
  * vfio_get_iommu_type - selects the richest iommu_type (v2 first)
  */
-static int vfio_get_iommu_type(VFIOContainer *container,
+static int vfio_get_iommu_type(int container_fd,
                                Error **errp)
 {
     int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
@@ -362,7 +362,7 @@ static int vfio_get_iommu_type(VFIOContainer *container,
     int i;
 
     for (i = 0; i < ARRAY_SIZE(iommu_types); i++) {
-        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu_types[i])) {
+        if (ioctl(container_fd, VFIO_CHECK_EXTENSION, iommu_types[i])) {
             return iommu_types[i];
         }
     }
@@ -399,7 +399,7 @@ static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
     int iommu_type;
     const VFIOIOMMUClass *vioc;
 
-    iommu_type = vfio_get_iommu_type(container, errp);
+    iommu_type = vfio_get_iommu_type(container->fd, errp);
     if (iommu_type < 0) {
         return false;
     }
-- 
2.45.2

[PATCH v2 08/17] vfio/container: Introduce vfio_get_iommu_class_name()

[Cédric Le Goater]

Rework vfio_get_iommu_class() to return a literal class name instead
of a class object. We will need this name to instantiate the object
later on. Since the default case asserts, remove the error report as
QEMU will simply abort before.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/container.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 589f37bc6d68dae18f9e46371f14d6952b2240c0..bb6abe60ee29d5b69b494523c9002f53e1b2a3c8 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -373,24 +373,20 @@ static int vfio_get_iommu_type(int container_fd,
 /*
  * vfio_get_iommu_ops - get a VFIOIOMMUClass associated with a type
  */
-static const VFIOIOMMUClass *vfio_get_iommu_class(int iommu_type, Error **errp)
+static const char *vfio_get_iommu_class_name(int iommu_type)
 {
-    ObjectClass *klass = NULL;
-
     switch (iommu_type) {
     case VFIO_TYPE1v2_IOMMU:
     case VFIO_TYPE1_IOMMU:
-        klass = object_class_by_name(TYPE_VFIO_IOMMU_LEGACY);
+        return TYPE_VFIO_IOMMU_LEGACY;
         break;
     case VFIO_SPAPR_TCE_v2_IOMMU:
     case VFIO_SPAPR_TCE_IOMMU:
-        klass = object_class_by_name(TYPE_VFIO_IOMMU_SPAPR);
+        return TYPE_VFIO_IOMMU_SPAPR;
         break;
     default:
         g_assert_not_reached();
     };
-
-    return VFIO_IOMMU_CLASS(klass);
 }
 
 static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
@@ -398,6 +394,7 @@ static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
 {
     int iommu_type;
     const VFIOIOMMUClass *vioc;
+    const char *vioc_name;
 
     iommu_type = vfio_get_iommu_type(container->fd, errp);
     if (iommu_type < 0) {
@@ -426,11 +423,8 @@ static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
 
     container->iommu_type = iommu_type;
 
-    vioc = vfio_get_iommu_class(iommu_type, errp);
-    if (!vioc) {
-        error_setg(errp, "No available IOMMU models");
-        return false;
-    }
+    vioc_name = vfio_get_iommu_class_name(iommu_type);
+    vioc = VFIO_IOMMU_CLASS(object_class_by_name(vioc_name));
 
     vfio_container_init(&container->bcontainer, vioc);
     return true;
-- 
2.45.2

[PATCH v2 09/17] vfio/container: Introduce vfio_create_container()

[Cédric Le Goater]

This routine allocates the QEMU struct type representing the VFIO
container. It is minimal currently and future changes will do more
initialization.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/container.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index bb6abe60ee29d5b69b494523c9002f53e1b2a3c8..a8691942791006f44f7a3c34b32c67ca51766182 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -430,6 +430,16 @@ static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
     return true;
 }
 
+static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
+                                            Error **errp)
+{
+    VFIOContainer *container;
+
+    container = g_malloc0(sizeof(*container));
+    container->fd = fd;
+    return container;
+}
+
 static int vfio_get_iommu_info(VFIOContainer *container,
                                struct vfio_iommu_type1_info **info)
 {
@@ -604,13 +614,14 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
         goto close_fd_exit;
     }
 
-    container = g_malloc0(sizeof(*container));
-    container->fd = fd;
-    bcontainer = &container->bcontainer;
-
+    container = vfio_create_container(fd, group, errp);
+    if (!container) {
+        goto close_fd_exit;
+    }
     if (!vfio_set_iommu(container, group->fd, errp)) {
         goto free_container_exit;
     }
+    bcontainer = &container->bcontainer;
 
     if (!vfio_cpr_register_container(bcontainer, errp)) {
         goto free_container_exit;
-- 
2.45.2

[PATCH v2 10/17] vfio/container: Discover IOMMU type before creating the container

[Cédric Le Goater]

Since the QEMU struct type representing the VFIO container is deduced
from the IOMMU type exposed by the host, this type should be well
defined *before* creating the container struct. This will be necessary
to instantiate a QOM object of the correct type in future changes.

Rework vfio_set_iommu() to extract the part doing the container
initialization and move it under vfio_create_container().

Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/container.c | 47 ++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index a8691942791006f44f7a3c34b32c67ca51766182..31bdc46a96d1626b237227a25007957e1d472757 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -389,54 +389,56 @@ static const char *vfio_get_iommu_class_name(int iommu_type)
     };
 }
 
-static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
-                           Error **errp)
+static bool vfio_set_iommu(int container_fd, int group_fd,
+                           int *iommu_type, Error **errp)
 {
-    int iommu_type;
-    const VFIOIOMMUClass *vioc;
-    const char *vioc_name;
-
-    iommu_type = vfio_get_iommu_type(container->fd, errp);
-    if (iommu_type < 0) {
-        return false;
-    }
-
-    if (ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
+    if (ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container_fd)) {
         error_setg_errno(errp, errno, "Failed to set group container");
         return false;
     }
 
-    while (ioctl(container->fd, VFIO_SET_IOMMU, iommu_type)) {
-        if (iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
+    while (ioctl(container_fd, VFIO_SET_IOMMU, *iommu_type)) {
+        if (*iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
             /*
              * On sPAPR, despite the IOMMU subdriver always advertises v1 and
              * v2, the running platform may not support v2 and there is no
              * way to guess it until an IOMMU group gets added to the container.
              * So in case it fails with v2, try v1 as a fallback.
              */
-            iommu_type = VFIO_SPAPR_TCE_IOMMU;
+            *iommu_type = VFIO_SPAPR_TCE_IOMMU;
             continue;
         }
         error_setg_errno(errp, errno, "Failed to set iommu for container");
         return false;
     }
 
-    container->iommu_type = iommu_type;
-
-    vioc_name = vfio_get_iommu_class_name(iommu_type);
-    vioc = VFIO_IOMMU_CLASS(object_class_by_name(vioc_name));
-
-    vfio_container_init(&container->bcontainer, vioc);
     return true;
 }
 
 static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
                                             Error **errp)
 {
+    int iommu_type;
+    const VFIOIOMMUClass *vioc;
+    const char *vioc_name;
     VFIOContainer *container;
 
+    iommu_type = vfio_get_iommu_type(fd, errp);
+    if (iommu_type < 0) {
+        return NULL;
+    }
+
+    if (!vfio_set_iommu(fd, group->fd, &iommu_type, errp)) {
+        return NULL;
+    }
+
+    vioc_name = vfio_get_iommu_class_name(iommu_type);
+    vioc = VFIO_IOMMU_CLASS(object_class_by_name(vioc_name));
+
     container = g_malloc0(sizeof(*container));
     container->fd = fd;
+    container->iommu_type = iommu_type;
+    vfio_container_init(&container->bcontainer, vioc);
     return container;
 }
 
@@ -618,9 +620,6 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     if (!container) {
         goto close_fd_exit;
     }
-    if (!vfio_set_iommu(container, group->fd, errp)) {
-        goto free_container_exit;
-    }
     bcontainer = &container->bcontainer;
 
     if (!vfio_cpr_register_container(bcontainer, errp)) {
-- 
2.45.2

[PATCH v2 11/17] vfio/container: Change VFIOContainerBase to use QOM

[Cédric Le Goater]

VFIOContainerBase was made a QOM interface because we believed that a
QOM object would expose all the IOMMU backends to the QEMU machine and
human interface. This only applies to user creatable devices or objects.

Change the VFIOContainerBase nature from interface to object and make
the necessary adjustments in the VFIO_IOMMU hierarchy.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/vfio/vfio-common.h         |  4 ++++
 include/hw/vfio/vfio-container-base.h | 12 +++---------
 hw/vfio/container-base.c              |  4 +++-
 hw/vfio/container.c                   |  1 +
 hw/vfio/iommufd.c                     |  1 +
 hw/vfio/spapr.c                       |  3 +++
 6 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 825d80130bd435fe50830c8ae5b7905d18104dd6..e8ddf92bb18547f0d3b811b3d757cbae7fec8b8d 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -84,6 +84,8 @@ typedef struct VFIOContainer {
     QLIST_HEAD(, VFIOGroup) group_list;
 } VFIOContainer;
 
+OBJECT_DECLARE_SIMPLE_TYPE(VFIOContainer, VFIO_IOMMU_LEGACY);
+
 typedef struct VFIOHostDMAWindow {
     hwaddr min_iova;
     hwaddr max_iova;
@@ -99,6 +101,8 @@ typedef struct VFIOIOMMUFDContainer {
     uint32_t ioas_id;
 } VFIOIOMMUFDContainer;
 
+OBJECT_DECLARE_SIMPLE_TYPE(VFIOIOMMUFDContainer, VFIO_IOMMU_IOMMUFD);
+
 typedef struct VFIODeviceOps VFIODeviceOps;
 
 typedef struct VFIODevice {
diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index d505f63607ec40e6aa44aeb3e20848ac780562a1..b079b76f68975c5701a289ce9012e912a8e44fc6 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -34,6 +34,7 @@ typedef struct VFIOAddressSpace {
  * This is the base object for vfio container backends
  */
 typedef struct VFIOContainerBase {
+    Object parent;
     const VFIOIOMMUClass *ops;
     VFIOAddressSpace *space;
     MemoryListener listener;
@@ -96,17 +97,10 @@ void vfio_container_destroy(VFIOContainerBase *bcontainer);
 #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr"
 #define TYPE_VFIO_IOMMU_IOMMUFD TYPE_VFIO_IOMMU "-iommufd"
 
-/*
- * VFIOContainerBase is not an abstract QOM object because it felt
- * unnecessary to expose all the IOMMU backends to the QEMU machine
- * and human interface. However, we can still abstract the IOMMU
- * backend handlers using a QOM interface class. This provides more
- * flexibility when referencing the various implementations.
- */
-DECLARE_CLASS_CHECKERS(VFIOIOMMUClass, VFIO_IOMMU, TYPE_VFIO_IOMMU)
+OBJECT_DECLARE_TYPE(VFIOContainerBase, VFIOIOMMUClass, VFIO_IOMMU)
 
 struct VFIOIOMMUClass {
-    InterfaceClass parent_class;
+    ObjectClass parent_class;
 
     /* Properties */
     const char *hiod_typename;
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 280f0dd2db1fc3939fe9925ce00a2c50d0e14196..98c15e174dd78df5146ee83c05c98f3ea9c1e52c 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -102,8 +102,10 @@ void vfio_container_destroy(VFIOContainerBase *bcontainer)
 static const TypeInfo types[] = {
     {
         .name = TYPE_VFIO_IOMMU,
-        .parent = TYPE_INTERFACE,
+        .parent = TYPE_OBJECT,
+        .instance_size = sizeof(VFIOContainerBase),
         .class_size = sizeof(VFIOIOMMUClass),
+        .abstract = true,
     },
 };
 
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 31bdc46a96d1626b237227a25007957e1d472757..3ae52530a9b500bd53ec9f9e66c73253d97c9aba 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -1196,6 +1196,7 @@ static const TypeInfo types[] = {
     {
         .name = TYPE_VFIO_IOMMU_LEGACY,
         .parent = TYPE_VFIO_IOMMU,
+        .instance_size = sizeof(VFIOContainer),
         .class_init = vfio_iommu_legacy_class_init,
     }, {
         .name = TYPE_HOST_IOMMU_DEVICE_LEGACY_VFIO,
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index e5d9334142418514215528b9523f12c031792c7f..3e9d642034c2d2234ea701952c94a78ab32e9147 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -672,6 +672,7 @@ static const TypeInfo types[] = {
     {
         .name = TYPE_VFIO_IOMMU_IOMMUFD,
         .parent = TYPE_VFIO_IOMMU,
+        .instance_size = sizeof(VFIOIOMMUFDContainer),
         .class_init = vfio_iommu_iommufd_class_init,
     }, {
         .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO,
diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
index 47b040f1bcca7dd0b5cf052d941b43541e98a3c5..018bd2048194a6a2db83ed740025a7060181698f 100644
--- a/hw/vfio/spapr.c
+++ b/hw/vfio/spapr.c
@@ -30,6 +30,8 @@ typedef struct VFIOSpaprContainer {
     QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
 } VFIOSpaprContainer;
 
+OBJECT_DECLARE_SIMPLE_TYPE(VFIOSpaprContainer, VFIO_IOMMU_SPAPR);
+
 static bool vfio_prereg_listener_skipped_section(MemoryRegionSection *section)
 {
     if (memory_region_is_iommu(section->mr)) {
@@ -548,6 +550,7 @@ static const TypeInfo types[] = {
     {
         .name = TYPE_VFIO_IOMMU_SPAPR,
         .parent = TYPE_VFIO_IOMMU_LEGACY,
+        .instance_size = sizeof(VFIOSpaprContainer),
         .class_init = vfio_iommu_spapr_class_init,
     },
 };
-- 
2.45.2

[PATCH v2 12/17] vfio/container: Switch to QOM

[Cédric Le Goater]

Instead of allocating the container struct, create a QOM object of the
appropriate type.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/container.c | 6 +++---
 hw/vfio/iommufd.c   | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 3ae52530a9b500bd53ec9f9e66c73253d97c9aba..ff3a6831da83c0fe11060cd57918c4d87b10197c 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -435,7 +435,7 @@ static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
     vioc_name = vfio_get_iommu_class_name(iommu_type);
     vioc = VFIO_IOMMU_CLASS(object_class_by_name(vioc_name));
 
-    container = g_malloc0(sizeof(*container));
+    container = VFIO_IOMMU_LEGACY(object_new(vioc_name));
     container->fd = fd;
     container->iommu_type = iommu_type;
     vfio_container_init(&container->bcontainer, vioc);
@@ -674,7 +674,7 @@ unregister_container_exit:
     vfio_cpr_unregister_container(bcontainer);
 
 free_container_exit:
-    g_free(container);
+    object_unref(container);
 
 close_fd_exit:
     close(fd);
@@ -718,7 +718,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
         trace_vfio_disconnect_container(container->fd);
         vfio_cpr_unregister_container(bcontainer);
         close(container->fd);
-        g_free(container);
+        object_unref(container);
 
         vfio_put_address_space(space);
     }
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 3e9d642034c2d2234ea701952c94a78ab32e9147..d59df858407f3cadb9405386ad673c99cdad61d0 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -239,7 +239,7 @@ static void iommufd_cdev_container_destroy(VFIOIOMMUFDContainer *container)
     memory_listener_unregister(&bcontainer->listener);
     vfio_container_destroy(bcontainer);
     iommufd_backend_free_id(container->be, container->ioas_id);
-    g_free(container);
+    object_unref(container);
 }
 
 static int iommufd_cdev_ram_block_discard_disable(bool state)
@@ -352,7 +352,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
 
     trace_iommufd_cdev_alloc_ioas(vbasedev->iommufd->fd, ioas_id);
 
-    container = g_malloc0(sizeof(*container));
+    container = VFIO_IOMMU_IOMMUFD(object_new(TYPE_VFIO_IOMMU_IOMMUFD));
     container->be = vbasedev->iommufd;
     container->ioas_id = ioas_id;
 
-- 
2.45.2

[PATCH v2 13/17] vfio/container: Introduce an instance_init() handler

[Cédric Le Goater]

This allows us to move the initialization code from vfio_container_init(),
which we will soon remove.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/container-base.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 98c15e174dd78df5146ee83c05c98f3ea9c1e52c..3858f5ab1d68e897f9013161d7c5c20c0553029d 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -75,12 +75,6 @@ void vfio_container_init(VFIOContainerBase *bcontainer,
                          const VFIOIOMMUClass *ops)
 {
     bcontainer->ops = ops;
-    bcontainer->error = NULL;
-    bcontainer->dirty_pages_supported = false;
-    bcontainer->dma_max_mappings = 0;
-    bcontainer->iova_ranges = NULL;
-    QLIST_INIT(&bcontainer->giommu_list);
-    QLIST_INIT(&bcontainer->vrdl_list);
 }
 
 void vfio_container_destroy(VFIOContainerBase *bcontainer)
@@ -99,10 +93,23 @@ void vfio_container_destroy(VFIOContainerBase *bcontainer)
     g_list_free_full(bcontainer->iova_ranges, g_free);
 }
 
+static void vfio_container_instance_init(Object *obj)
+{
+    VFIOContainerBase *bcontainer = VFIO_IOMMU(obj);
+
+    bcontainer->error = NULL;
+    bcontainer->dirty_pages_supported = false;
+    bcontainer->dma_max_mappings = 0;
+    bcontainer->iova_ranges = NULL;
+    QLIST_INIT(&bcontainer->giommu_list);
+    QLIST_INIT(&bcontainer->vrdl_list);
+}
+
 static const TypeInfo types[] = {
     {
         .name = TYPE_VFIO_IOMMU,
         .parent = TYPE_OBJECT,
+        .instance_init = vfio_container_instance_init,
         .instance_size = sizeof(VFIOContainerBase),
         .class_size = sizeof(VFIOIOMMUClass),
         .abstract = true,
-- 
2.45.2

[PATCH v2 14/17] vfio/container: Remove VFIOContainerBase::ops

[Cédric Le Goater]

Instead, use VFIO_IOMMU_GET_CLASS() to get the class pointer.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/vfio/vfio-container-base.h |  1 -
 hw/vfio/common.c                      |  2 +-
 hw/vfio/container-base.c              | 37 +++++++++++++++++----------
 hw/vfio/container.c                   | 15 ++++++-----
 hw/vfio/iommufd.c                     |  4 +--
 hw/vfio/pci.c                         |  4 +--
 6 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index b079b76f68975c5701a289ce9012e912a8e44fc6..6b57cd8e7f5d7d2817f6e3b96ce4566d2630bb12 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -35,7 +35,6 @@ typedef struct VFIOAddressSpace {
  */
 typedef struct VFIOContainerBase {
     Object parent;
-    const VFIOIOMMUClass *ops;
     VFIOAddressSpace *space;
     MemoryListener listener;
     Error *error;
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 1686a0bed23bd95467bfb00a0c39a4d966e49cae..7cdb969fd396ae3815cb175ad631d93d7cca7006 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1573,5 +1573,5 @@ void vfio_detach_device(VFIODevice *vbasedev)
         return;
     }
     object_unref(vbasedev->hiod);
-    vbasedev->bcontainer->ops->detach_device(vbasedev);
+    VFIO_IOMMU_GET_CLASS(vbasedev->bcontainer)->detach_device(vbasedev);
 }
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 3858f5ab1d68e897f9013161d7c5c20c0553029d..24669d4d7472f49ac3adf2618a32bf7d82c5c344 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -19,62 +19,73 @@ int vfio_container_dma_map(VFIOContainerBase *bcontainer,
                            hwaddr iova, ram_addr_t size,
                            void *vaddr, bool readonly)
 {
-    g_assert(bcontainer->ops->dma_map);
-    return bcontainer->ops->dma_map(bcontainer, iova, size, vaddr, readonly);
+    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
+
+    g_assert(vioc->dma_map);
+    return vioc->dma_map(bcontainer, iova, size, vaddr, readonly);
 }
 
 int vfio_container_dma_unmap(VFIOContainerBase *bcontainer,
                              hwaddr iova, ram_addr_t size,
                              IOMMUTLBEntry *iotlb)
 {
-    g_assert(bcontainer->ops->dma_unmap);
-    return bcontainer->ops->dma_unmap(bcontainer, iova, size, iotlb);
+    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
+
+    g_assert(vioc->dma_unmap);
+    return vioc->dma_unmap(bcontainer, iova, size, iotlb);
 }
 
 bool vfio_container_add_section_window(VFIOContainerBase *bcontainer,
                                        MemoryRegionSection *section,
                                        Error **errp)
 {
-    if (!bcontainer->ops->add_window) {
+    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
+
+    if (!vioc->add_window) {
         return true;
     }
 
-    return bcontainer->ops->add_window(bcontainer, section, errp);
+    return vioc->add_window(bcontainer, section, errp);
 }
 
 void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
                                        MemoryRegionSection *section)
 {
-    if (!bcontainer->ops->del_window) {
+    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
+
+    if (!vioc->del_window) {
         return;
     }
 
-    return bcontainer->ops->del_window(bcontainer, section);
+    return vioc->del_window(bcontainer, section);
 }
 
 int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
                                            bool start, Error **errp)
 {
+    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
+
     if (!bcontainer->dirty_pages_supported) {
         return 0;
     }
 
-    g_assert(bcontainer->ops->set_dirty_page_tracking);
-    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp);
+    g_assert(vioc->set_dirty_page_tracking);
+    return vioc->set_dirty_page_tracking(bcontainer, start, errp);
 }
 
 int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
                    VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp)
 {
-    g_assert(bcontainer->ops->query_dirty_bitmap);
-    return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size,
+    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
+
+    g_assert(vioc->query_dirty_bitmap);
+    return vioc->query_dirty_bitmap(bcontainer, vbmap, iova, size,
                                                errp);
 }
 
 void vfio_container_init(VFIOContainerBase *bcontainer,
                          const VFIOIOMMUClass *ops)
 {
-    bcontainer->ops = ops;
 }
 
 void vfio_container_destroy(VFIOContainerBase *bcontainer)
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index ff3a6831da83c0fe11060cd57918c4d87b10197c..a2f5fbad00cd228e27a47df5cd683dbb34296113 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -548,6 +548,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
     VFIOContainerBase *bcontainer;
     int ret, fd;
     VFIOAddressSpace *space;
+    VFIOIOMMUClass *vioc;
 
     space = vfio_get_address_space(as);
 
@@ -632,9 +633,10 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
         goto unregister_container_exit;
     }
 
-    assert(bcontainer->ops->setup);
+    vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
+    assert(vioc->setup);
 
-    if (!bcontainer->ops->setup(bcontainer, errp)) {
+    if (!vioc->setup(bcontainer, errp)) {
         goto enable_discards_exit;
     }
 
@@ -663,8 +665,8 @@ listener_release_exit:
     QLIST_REMOVE(bcontainer, next);
     vfio_kvm_device_del_group(group);
     memory_listener_unregister(&bcontainer->listener);
-    if (bcontainer->ops->release) {
-        bcontainer->ops->release(bcontainer);
+    if (vioc->release) {
+        vioc->release(bcontainer);
     }
 
 enable_discards_exit:
@@ -689,6 +691,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
 {
     VFIOContainer *container = group->container;
     VFIOContainerBase *bcontainer = &container->bcontainer;
+    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
 
     QLIST_REMOVE(group, container_next);
     group->container = NULL;
@@ -700,8 +703,8 @@ static void vfio_disconnect_container(VFIOGroup *group)
      */
     if (QLIST_EMPTY(&container->group_list)) {
         memory_listener_unregister(&bcontainer->listener);
-        if (bcontainer->ops->release) {
-            bcontainer->ops->release(bcontainer);
+        if (vioc->release) {
+            vioc->release(bcontainer);
         }
     }
 
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index d59df858407f3cadb9405386ad673c99cdad61d0..7bc76f80b48ea5422e68fd4d4cb3f5bca90993f6 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -324,7 +324,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
     /* try to attach to an existing container in this space */
     QLIST_FOREACH(bcontainer, &space->containers, next) {
         container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
-        if (bcontainer->ops != iommufd_vioc ||
+        if (VFIO_IOMMU_GET_CLASS(bcontainer) != iommufd_vioc ||
             vbasedev->iommufd != container->be) {
             continue;
         }
@@ -465,7 +465,7 @@ static VFIODevice *iommufd_cdev_pci_find_by_devid(__u32 devid)
         VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
 
     QLIST_FOREACH(vbasedev_iter, &vfio_device_list, global_next) {
-        if (vbasedev_iter->bcontainer->ops != iommufd_vioc) {
+        if (VFIO_IOMMU_GET_CLASS(vbasedev_iter->bcontainer) != iommufd_vioc) {
             continue;
         }
         if (devid == vbasedev_iter->devid) {
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d8a76c1ee003e6f5669e8390271836fd9d839a8a..e03d9f3ba5461f55f6351d937aba5d522a9128ec 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2511,9 +2511,9 @@ int vfio_pci_get_pci_hot_reset_info(VFIOPCIDevice *vdev,
 static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
 {
     VFIODevice *vbasedev = &vdev->vbasedev;
-    const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
+    const VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(vbasedev->bcontainer);
 
-    return ops->pci_hot_reset(vbasedev, single);
+    return vioc->pci_hot_reset(vbasedev, single);
 }
 
 /*
-- 
2.45.2

[PATCH v2 15/17] vfio/container: Remove vfio_container_init()

[Cédric Le Goater]

It's now empty.

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

diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index 6b57cd8e7f5d7d2817f6e3b96ce4566d2630bb12..6242a62771caa8cf19440a53ad6f4db862ca12d7 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -86,8 +86,6 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
 int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
                    VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp);
 
-void vfio_container_init(VFIOContainerBase *bcontainer,
-                         const VFIOIOMMUClass *ops);
 void vfio_container_destroy(VFIOContainerBase *bcontainer);
 
 
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 24669d4d7472f49ac3adf2618a32bf7d82c5c344..970ae2356a92f87df44e1dd58ff8c67045a24ef1 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -83,11 +83,6 @@ int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
                                                errp);
 }
 
-void vfio_container_init(VFIOContainerBase *bcontainer,
-                         const VFIOIOMMUClass *ops)
-{
-}
-
 void vfio_container_destroy(VFIOContainerBase *bcontainer)
 {
     VFIOGuestIOMMU *giommu, *tmp;
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index a2f5fbad00cd228e27a47df5cd683dbb34296113..3f2032d5c496de078c277ebacc49d7db89f4cc65 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -419,7 +419,6 @@ static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
                                             Error **errp)
 {
     int iommu_type;
-    const VFIOIOMMUClass *vioc;
     const char *vioc_name;
     VFIOContainer *container;
 
@@ -433,12 +432,10 @@ static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
     }
 
     vioc_name = vfio_get_iommu_class_name(iommu_type);
-    vioc = VFIO_IOMMU_CLASS(object_class_by_name(vioc_name));
 
     container = VFIO_IOMMU_LEGACY(object_new(vioc_name));
     container->fd = fd;
     container->iommu_type = iommu_type;
-    vfio_container_init(&container->bcontainer, vioc);
     return container;
 }
 
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 7bc76f80b48ea5422e68fd4d4cb3f5bca90993f6..09b71a6617807c621275c74b924cfd39eb643961 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -357,7 +357,6 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
     container->ioas_id = ioas_id;
 
     bcontainer = &container->bcontainer;
-    vfio_container_init(bcontainer, iommufd_vioc);
     vfio_address_space_insert(space, bcontainer);
 
     if (!iommufd_cdev_attach_container(vbasedev, container, errp)) {
-- 
2.45.2

[PATCH v2 16/17] vfio/container: Introduce vfio_iommu_legacy_instance_init()

[Cédric Le Goater]

Just as we did for the VFIOContainerBase object, introduce an
instance_init() handler for the legacy VFIOContainer object and do the
specific initialization there.

Signed-off-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/container.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 3f2032d5c496de078c277ebacc49d7db89f4cc65..45123acbdd6a681f4ce7cae7aa2509100ea225ab 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -639,7 +639,6 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
 
     vfio_kvm_device_add_group(group);
 
-    QLIST_INIT(&container->group_list);
     vfio_address_space_insert(space, bcontainer);
 
     group->container = container;
@@ -1183,6 +1182,13 @@ hiod_legacy_vfio_get_iova_ranges(HostIOMMUDevice *hiod, Error **errp)
     return l;
 }
 
+static void vfio_iommu_legacy_instance_init(Object *obj)
+{
+    VFIOContainer *container = VFIO_IOMMU_LEGACY(obj);
+
+    QLIST_INIT(&container->group_list);
+}
+
 static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
 {
     HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
@@ -1196,6 +1202,7 @@ static const TypeInfo types[] = {
     {
         .name = TYPE_VFIO_IOMMU_LEGACY,
         .parent = TYPE_VFIO_IOMMU,
+        .instance_init = vfio_iommu_legacy_instance_init,
         .instance_size = sizeof(VFIOContainer),
         .class_init = vfio_iommu_legacy_class_init,
     }, {
-- 
2.45.2

[PATCH v2 17/17] vfio/container: Move vfio_container_destroy() to an instance_finalize() handler

[Cédric Le Goater]

vfio_container_destroy() clears the resources allocated
VFIOContainerBase object. Now that VFIOContainerBase is a QOM object,
add an instance_finalize() handler to do the cleanup. It will be
called through object_unref().

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

diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index 6242a62771caa8cf19440a53ad6f4db862ca12d7..419e45ee7a5ac960dae4a993127fc9ee66d48db2 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -86,9 +86,6 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
 int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
                    VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp);
 
-void vfio_container_destroy(VFIOContainerBase *bcontainer);
-
-
 #define TYPE_VFIO_IOMMU "vfio-iommu"
 #define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy"
 #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr"
diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
index 970ae2356a92f87df44e1dd58ff8c67045a24ef1..50b1664f89a8192cf4021498e59f2a92cd2f6e89 100644
--- a/hw/vfio/container-base.c
+++ b/hw/vfio/container-base.c
@@ -83,8 +83,9 @@ int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
                                                errp);
 }
 
-void vfio_container_destroy(VFIOContainerBase *bcontainer)
+static void vfio_container_instance_finalize(Object *obj)
 {
+    VFIOContainerBase *bcontainer = VFIO_IOMMU(obj);
     VFIOGuestIOMMU *giommu, *tmp;
 
     QLIST_REMOVE(bcontainer, next);
@@ -116,6 +117,7 @@ static const TypeInfo types[] = {
         .name = TYPE_VFIO_IOMMU,
         .parent = TYPE_OBJECT,
         .instance_init = vfio_container_instance_init,
+        .instance_finalize = vfio_container_instance_finalize,
         .instance_size = sizeof(VFIOContainerBase),
         .class_size = sizeof(VFIOIOMMUClass),
         .abstract = true,
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 45123acbdd6a681f4ce7cae7aa2509100ea225ab..2e7ecdf10edc4d84963a45ae9507096965da64fc 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -712,8 +712,6 @@ static void vfio_disconnect_container(VFIOGroup *group)
     if (QLIST_EMPTY(&container->group_list)) {
         VFIOAddressSpace *space = bcontainer->space;
 
-        vfio_container_destroy(bcontainer);
-
         trace_vfio_disconnect_container(container->fd);
         vfio_cpr_unregister_container(bcontainer);
         close(container->fd);
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 09b71a6617807c621275c74b924cfd39eb643961..c2f158e60386502eef267769ac9bce1effb67033 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -237,7 +237,6 @@ static void iommufd_cdev_container_destroy(VFIOIOMMUFDContainer *container)
         return;
     }
     memory_listener_unregister(&bcontainer->listener);
-    vfio_container_destroy(bcontainer);
     iommufd_backend_free_id(container->be, container->ioas_id);
     object_unref(container);
 }
-- 
2.45.2

RE: [PATCH v2 17/17] vfio/container: Move vfio_container_destroy() to an instance_finalize() handler

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: [PATCH v2 17/17] vfio/container: Move vfio_container_destroy() to
>an instance_finalize() handler
>
>vfio_container_destroy() clears the resources allocated
>VFIOContainerBase object. Now that VFIOContainerBase is a QOM object,
>add an instance_finalize() handler to do the cleanup. It will be
>called through object_unref().
>
>Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>Signed-off-by: Cédric Le Goater <clg@redhat.com>

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>---
> include/hw/vfio/vfio-container-base.h | 3 ---
> hw/vfio/container-base.c              | 4 +++-
> hw/vfio/container.c                   | 2 --
> hw/vfio/iommufd.c                     | 1 -
> 4 files changed, 3 insertions(+), 7 deletions(-)
>
>diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>index
>6242a62771caa8cf19440a53ad6f4db862ca12d7..419e45ee7a5ac960dae4a9
>93127fc9ee66d48db2 100644
>--- a/include/hw/vfio/vfio-container-base.h
>+++ b/include/hw/vfio/vfio-container-base.h
>@@ -86,9 +86,6 @@ int
>vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
> int vfio_container_query_dirty_bitmap(const VFIOContainerBase
>*bcontainer,
>                    VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp);
>
>-void vfio_container_destroy(VFIOContainerBase *bcontainer);
>-
>-
> #define TYPE_VFIO_IOMMU "vfio-iommu"
> #define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy"
> #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr"
>diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
>index
>970ae2356a92f87df44e1dd58ff8c67045a24ef1..50b1664f89a8192cf402149
>8e59f2a92cd2f6e89 100644
>--- a/hw/vfio/container-base.c
>+++ b/hw/vfio/container-base.c
>@@ -83,8 +83,9 @@ int vfio_container_query_dirty_bitmap(const
>VFIOContainerBase *bcontainer,
>                                                errp);
> }
>
>-void vfio_container_destroy(VFIOContainerBase *bcontainer)
>+static void vfio_container_instance_finalize(Object *obj)
> {
>+    VFIOContainerBase *bcontainer = VFIO_IOMMU(obj);
>     VFIOGuestIOMMU *giommu, *tmp;
>
>     QLIST_REMOVE(bcontainer, next);
>@@ -116,6 +117,7 @@ static const TypeInfo types[] = {
>         .name = TYPE_VFIO_IOMMU,
>         .parent = TYPE_OBJECT,
>         .instance_init = vfio_container_instance_init,
>+        .instance_finalize = vfio_container_instance_finalize,
>         .instance_size = sizeof(VFIOContainerBase),
>         .class_size = sizeof(VFIOIOMMUClass),
>         .abstract = true,
>diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>index
>45123acbdd6a681f4ce7cae7aa2509100ea225ab..2e7ecdf10edc4d84963a45
>ae9507096965da64fc 100644
>--- a/hw/vfio/container.c
>+++ b/hw/vfio/container.c
>@@ -712,8 +712,6 @@ static void vfio_disconnect_container(VFIOGroup
>*group)
>     if (QLIST_EMPTY(&container->group_list)) {
>         VFIOAddressSpace *space = bcontainer->space;
>
>-        vfio_container_destroy(bcontainer);
>-
>         trace_vfio_disconnect_container(container->fd);
>         vfio_cpr_unregister_container(bcontainer);
>         close(container->fd);
>diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>index
>09b71a6617807c621275c74b924cfd39eb643961..c2f158e60386502eef267
>769ac9bce1effb67033 100644
>--- a/hw/vfio/iommufd.c
>+++ b/hw/vfio/iommufd.c
>@@ -237,7 +237,6 @@ static void
>iommufd_cdev_container_destroy(VFIOIOMMUFDContainer *container)
>         return;
>     }
>     memory_listener_unregister(&bcontainer->listener);
>-    vfio_container_destroy(bcontainer);
>     iommufd_backend_free_id(container->be, container->ioas_id);
>     object_unref(container);
> }
>--
>2.45.2

Re: [PATCH v2 01/17] vfio: Make vfio_devices_dma_logging_start() return bool

[Eric Auger]

Hi Cédric,

On 6/17/24 08:33, Cédric Le Goater wrote:
> Since vfio_devices_dma_logging_start() takes an 'Error **' argument,
> best practices suggest to return a bool. See the api/error.h Rules
> section. It will simplify potential changes coming after.


As I already mentionned the Rules section does not say that, as far as I
understand:
It is allowed to either return a bool, a pointer-value, an integer,
along with an error handle:

"
 *   • bool-valued functions return true on success / false on failure,
 *   • pointer-valued functions return non-null / null pointer, and
 *   • integer-valued functions return non-negative / negative.
"

Personally I don't like much returning a bool as I think it rather
complexifies the code and to me that kind of change is error prone.



>
> vfio_container_set_dirty_page_tracking() could be modified in the same
> way but the errno value can be saved in the migration stream when
> called from vfio_listener_log_global_stop().
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/common.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 9e4c0cc95ff90209d3e8184035af0806a2bf890b..d48cd9b9361a92d184e423ffc60aabaff40fb487 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1020,7 +1020,7 @@ static void vfio_device_feature_dma_logging_start_destroy(
>      g_free(feature);
>  }
>  
> -static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
> +static bool vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
>                                            Error **errp)
>  {
>      struct vfio_device_feature *feature;
> @@ -1033,7 +1033,7 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
>                                                             &ranges);
>      if (!feature) {
>          error_setg_errno(errp, errno, "Failed to prepare DMA logging");
> -        return -errno;
> +        return false;
>      }
>  
>      QLIST_FOREACH(vbasedev, &bcontainer->device_list, container_next) {
> @@ -1058,7 +1058,7 @@ out:
>  
>      vfio_device_feature_dma_logging_start_destroy(feature);
>  
> -    return ret;
> +    return ret == 0;
>  }
>  
>  static bool vfio_listener_log_global_start(MemoryListener *listener,
> @@ -1067,18 +1067,18 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
>      ERRP_GUARD();
>      VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>                                                   listener);
> -    int ret;
> +    bool ret;
>  
>      if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>          ret = vfio_devices_dma_logging_start(bcontainer, errp);
>      } else {
> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp) == 0;

why vfio_container_set_dirty_page_tracking(bcontainer, true, errp) doesn't return a bool then?

Eric

>      }
>  
> -    if (ret) {
> +    if (!ret) {
>          error_prepend(errp, "vfio: Could not start dirty page tracking - ");
>      }
> -    return !ret;
> +    return ret;
>  }
>  
>  static void vfio_listener_log_global_stop(MemoryListener *listener)

Re: [PATCH v2 02/17] vfio: Remove unused declarations from vfio-common.h

[Eric Auger]

On 6/17/24 08:33, Cédric Le Goater wrote:
> These were forgotten in the recent cleanups.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric


> ---
>  include/hw/vfio/vfio-common.h | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 776de8064f740784f95cab0311c5f15f50d60ffe..c19572f90b277193491020af28e8b5587f15bfd1 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -207,10 +207,6 @@ typedef struct VFIODisplay {
>  VFIOAddressSpace *vfio_get_address_space(AddressSpace *as);
>  void vfio_put_address_space(VFIOAddressSpace *space);
>  
> -/* SPAPR specific */
> -int 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);
>  void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);

Re: [PATCH v2 03/17] vfio/common: Move dirty tracking ranges update to helper

[Eric Auger]

Hi Cédric,
On 6/17/24 08:33, Cédric Le Goater wrote:
> From: Joao Martins <joao.m.martins@oracle.com>
>
> Separate the changes that updates the ranges from the listener, to
s/updates/update
> make it reusable in preparation to expand its use to vIOMMU support.
>
> [ clg: - Rebased on upstream
>        - Introduced vfio_dirty_tracking_update_range() ]
>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/common.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index d48cd9b9361a92d184e423ffc60aabaff40fb487..fe215918bdf66ddbe3c5db803e10ce1aa9756b90 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -839,20 +839,11 @@ static bool vfio_section_is_vfio_pci(MemoryRegionSection *section,
>      return false;
>  }
>  
> -static void vfio_dirty_tracking_update(MemoryListener *listener,
> -                                       MemoryRegionSection *section)
> +static void vfio_dirty_tracking_update_range(VFIODirtyRanges *range,
> +                                             hwaddr iova, hwaddr end,
> +                                             bool update_pci)
>  {
> -    VFIODirtyRangesListener *dirty = container_of(listener,
> -                                                  VFIODirtyRangesListener,
> -                                                  listener);
> -    VFIODirtyRanges *range = &dirty->ranges;
> -    hwaddr iova, end, *min, *max;
> -
> -    if (!vfio_listener_valid_section(section, "tracking_update") ||
> -        !vfio_get_section_iova_range(dirty->bcontainer, section,
> -                                     &iova, &end, NULL)) {
> -        return;
> -    }
> +    hwaddr *min, *max;
>  
>      /*
>       * The address space passed to the dirty tracker is reduced to three ranges:
> @@ -873,8 +864,7 @@ static void vfio_dirty_tracking_update(MemoryListener *listener,
>       * The alternative would be an IOVATree but that has a much bigger runtime
>       * overhead and unnecessary complexity.
>       */
> -    if (vfio_section_is_vfio_pci(section, dirty->bcontainer) &&
> -        iova >= UINT32_MAX) {
> +    if (update_pci && iova >= UINT32_MAX) {
>          min = &range->minpci64;
>          max = &range->maxpci64;
>      } else {
> @@ -889,7 +879,23 @@ static void vfio_dirty_tracking_update(MemoryListener *listener,
>      }
>  
>      trace_vfio_device_dirty_tracking_update(iova, end, *min, *max);
don't you want to update the name of the trace point?
> -    return;
> +}
> +
> +static void vfio_dirty_tracking_update(MemoryListener *listener,
> +                                       MemoryRegionSection *section)
> +{
> +    VFIODirtyRangesListener *dirty =
> +        container_of(listener, VFIODirtyRangesListener, listener);
> +    hwaddr iova, end;
> +
> +    if (!vfio_listener_valid_section(section, "tracking_update") ||
> +        !vfio_get_section_iova_range(dirty->bcontainer, section,
> +                                     &iova, &end, NULL)) {
> +        return;
> +    }
> +
> +    vfio_dirty_tracking_update_range(&dirty->ranges, iova, end,
> +                      vfio_section_is_vfio_pci(section, dirty->bcontainer));
>  }
>  
>  static const MemoryListener vfio_dirty_tracking_listener = {
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

Re: [PATCH v2 01/17] vfio: Make vfio_devices_dma_logging_start() return bool

[Cédric Le Goater]

Hello Eric,

On 6/17/24 1:31 PM, Eric Auger wrote:
> Hi Cédric,
> 
> On 6/17/24 08:33, Cédric Le Goater wrote:
>> Since vfio_devices_dma_logging_start() takes an 'Error **' argument,
>> best practices suggest to return a bool. See the api/error.h Rules
>> section. It will simplify potential changes coming after.
> 
> 
> As I already mentionned the Rules section does not say that, as far as I
> understand:
> It is allowed to either return a bool, a pointer-value, an integer,
> along with an error handle:
> 
> "
>   *   • bool-valued functions return true on success / false on failure,
>   *   • pointer-valued functions return non-null / null pointer, and
>   *   • integer-valued functions return non-negative / negative.
> "
>
> Personally I don't like much returning a bool as I think it rather
> complexifies the code and to me that kind of change is error prone.

Returning an int could be misleading too, since there are multiple ways
it could be interpreted. You can find in QEMU routines returning -1 for
error which is later used as an errno :/

The error framework in QEMU provides a way to to save and return any
kind of errors, using the error_seg_*() routines. I tend to prefer
the basic approach: return fail or pass and use the Error parameter
for details.

Now, the problem, as always, is doing the conversion in all the
code. This is probably why people, with a kernel background, find
it confusing.


> 
> 
>>
>> vfio_container_set_dirty_page_tracking() could be modified in the same
>> way but the errno value can be saved in the migration stream when
>> called from vfio_listener_log_global_stop().
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/vfio/common.c | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 9e4c0cc95ff90209d3e8184035af0806a2bf890b..d48cd9b9361a92d184e423ffc60aabaff40fb487 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1020,7 +1020,7 @@ static void vfio_device_feature_dma_logging_start_destroy(
>>       g_free(feature);
>>   }
>>   
>> -static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
>> +static bool vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
>>                                             Error **errp)
>>   {
>>       struct vfio_device_feature *feature;
>> @@ -1033,7 +1033,7 @@ static int vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
>>                                                              &ranges);
>>       if (!feature) {
>>           error_setg_errno(errp, errno, "Failed to prepare DMA logging");
>> -        return -errno;
>> +        return false;
>>       }
>>   
>>       QLIST_FOREACH(vbasedev, &bcontainer->device_list, container_next) {
>> @@ -1058,7 +1058,7 @@ out:
>>   
>>       vfio_device_feature_dma_logging_start_destroy(feature);
>>   
>> -    return ret;
>> +    return ret == 0;
>>   }
>>   
>>   static bool vfio_listener_log_global_start(MemoryListener *listener,
>> @@ -1067,18 +1067,18 @@ static bool vfio_listener_log_global_start(MemoryListener *listener,
>>       ERRP_GUARD();
>>       VFIOContainerBase *bcontainer = container_of(listener, VFIOContainerBase,
>>                                                    listener);
>> -    int ret;
>> +    bool ret;
>>   
>>       if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>           ret = vfio_devices_dma_logging_start(bcontainer, errp);
>>       } else {
>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp);
>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer, true, errp) == 0;
> 
> why vfio_container_set_dirty_page_tracking(bcontainer, true, errp) doesn't return a bool then?

The errno value can be saved in the migration stream when called from
vfio_listener_log_global_stop(). So this would require changes in the
migration subsystem, like we did for vfio_listener_log_global_start().

Thanks,

C.





> 
> Eric
> 
>>       }
>>   
>> -    if (ret) {
>> +    if (!ret) {
>>           error_prepend(errp, "vfio: Could not start dirty page tracking - ");
>>       }
>> -    return !ret;
>> +    return ret;
>>   }
>>   
>>   static void vfio_listener_log_global_stop(MemoryListener *listener)
> 

Re: [PATCH v2 01/17] vfio: Make vfio_devices_dma_logging_start() return bool

[Eric Auger]

Hi Cédric,

On 6/17/24 14:34, Cédric Le Goater wrote:
> Hello Eric,
>
> On 6/17/24 1:31 PM, Eric Auger wrote:
>> Hi Cédric,
>>
>> On 6/17/24 08:33, Cédric Le Goater wrote:
>>> Since vfio_devices_dma_logging_start() takes an 'Error **' argument,
>>> best practices suggest to return a bool. See the api/error.h Rules
>>> section. It will simplify potential changes coming after.
>>
>>
>> As I already mentionned the Rules section does not say that, as far as I
>> understand:
>> It is allowed to either return a bool, a pointer-value, an integer,
>> along with an error handle:
>>
>> "
>>   *   • bool-valued functions return true on success / false on failure,
>>   *   • pointer-valued functions return non-null / null pointer, and
>>   *   • integer-valued functions return non-negative / negative.
>> "
>>
>> Personally I don't like much returning a bool as I think it rather
>> complexifies the code and to me that kind of change is error prone.
>
> Returning an int could be misleading too, since there are multiple ways
> it could be interpreted. You can find in QEMU routines returning -1 for
> error which is later used as an errno :/
yes no good!
>
> The error framework in QEMU provides a way to to save and return any
> kind of errors, using the error_seg_*() routines. I tend to prefer
> the basic approach: return fail or pass and use the Error parameter
> for details.
>
> Now, the problem, as always, is doing the conversion in all the
> code. This is probably why people, with a kernel background, find
> it confusing.
>
>
>>
>>
>>>
>>> vfio_container_set_dirty_page_tracking() could be modified in the same
>>> way but the errno value can be saved in the migration stream when
>>> called from vfio_listener_log_global_stop().
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>   hw/vfio/common.c | 14 +++++++-------
>>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index
>>> 9e4c0cc95ff90209d3e8184035af0806a2bf890b..d48cd9b9361a92d184e423ffc60aabaff40fb487
>>> 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -1020,7 +1020,7 @@ static void
>>> vfio_device_feature_dma_logging_start_destroy(
>>>       g_free(feature);
>>>   }
>>>   -static int vfio_devices_dma_logging_start(VFIOContainerBase
>>> *bcontainer,
>>> +static bool vfio_devices_dma_logging_start(VFIOContainerBase
>>> *bcontainer,
>>>                                             Error **errp)
>>>   {
>>>       struct vfio_device_feature *feature;
>>> @@ -1033,7 +1033,7 @@ static int
>>> vfio_devices_dma_logging_start(VFIOContainerBase *bcontainer,
>>>                                                              &ranges);
>>>       if (!feature) {
>>>           error_setg_errno(errp, errno, "Failed to prepare DMA
>>> logging");
>>> -        return -errno;
>>> +        return false;
>>>       }
>>>         QLIST_FOREACH(vbasedev, &bcontainer->device_list,
>>> container_next) {
>>> @@ -1058,7 +1058,7 @@ out:
>>>         vfio_device_feature_dma_logging_start_destroy(feature);
>>>   -    return ret;
>>> +    return ret == 0;
>>>   }
>>>     static bool vfio_listener_log_global_start(MemoryListener
>>> *listener,
>>> @@ -1067,18 +1067,18 @@ static bool
>>> vfio_listener_log_global_start(MemoryListener *listener,
>>>       ERRP_GUARD();
>>>       VFIOContainerBase *bcontainer = container_of(listener,
>>> VFIOContainerBase,
>>>                                                    listener);
>>> -    int ret;
>>> +    bool ret;
>>>         if (vfio_devices_all_device_dirty_tracking(bcontainer)) {
>>>           ret = vfio_devices_dma_logging_start(bcontainer, errp);
>>>       } else {
>>> -        ret = vfio_container_set_dirty_page_tracking(bcontainer,
>>> true, errp);
>>> +        ret = vfio_container_set_dirty_page_tracking(bcontainer,
>>> true, errp) == 0;
>>
>> why vfio_container_set_dirty_page_tracking(bcontainer, true, errp)
>> doesn't return a bool then?
>
> The errno value can be saved in the migration stream when called from
> vfio_listener_log_global_stop(). So this would require changes in the
> migration subsystem, like we did for vfio_listener_log_global_start().

OK

so although I am not a big fan of that kind of change and once
highlighted this is not mandated by the error.h doc, please feel free to
add my
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
>
> Thanks,
>
> C.
>
>
>
>
>
>>
>> Eric
>>
>>>       }
>>>   -    if (ret) {
>>> +    if (!ret) {
>>>           error_prepend(errp, "vfio: Could not start dirty page
>>> tracking - ");
>>>       }
>>> -    return !ret;
>>> +    return ret;
>>>   }
>>>     static void vfio_listener_log_global_stop(MemoryListener *listener)
>>
>

Re: [PATCH v2 04/17] vfio/common: Extract vIOMMU code from vfio_sync_dirty_bitmap()

[Eric Auger]

Hi Cédric,

On 6/17/24 08:33, Cédric Le Goater wrote:
> From: Avihai Horon <avihaih@nvidia.com>
>
> Extract vIOMMU code from vfio_sync_dirty_bitmap() to a new function and
> restructure the code.
>
> This is done in preparation for optimizing vIOMMU deviice dirty page
device
> tracking. No functional changes intended.
>
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> [ clg: - Rebased on upstream ]
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/common.c | 63 +++++++++++++++++++++++++++++-------------------
>  1 file changed, 38 insertions(+), 25 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index fe215918bdf66ddbe3c5db803e10ce1aa9756b90..f28641bad5cf4b71fcdc0a6c9d42b24c8d786248 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1302,37 +1302,50 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer,
>                                                  &vrdl);
>  }
>  
> +static int vfio_sync_iommu_dirty_bitmap(VFIOContainerBase *bcontainer,
> +                                        MemoryRegionSection *section)
> +{
> +    VFIOGuestIOMMU *giommu;
> +    bool found = false;
> +    Int128 llend;
> +    vfio_giommu_dirty_notifier gdn;
> +    int idx;
> +
> +    QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
> +        if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
> +            giommu->n.start == section->offset_within_region) {
> +            found = true;
> +            break;
> +        }
> +    }
> +
> +    if (!found) {
> +        return 0;
> +    }
> +
> +    gdn.giommu = giommu;
> +    idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
> +                                             MEMTXATTRS_UNSPECIFIED);
> +
> +    llend = int128_add(int128_make64(section->offset_within_region),
> +                       section->size);
> +    llend = int128_sub(llend, int128_one());
> +
> +    iommu_notifier_init(&gdn.n, vfio_iommu_map_dirty_notify, IOMMU_NOTIFIER_MAP,
> +                        section->offset_within_region, int128_get64(llend),
> +                        idx);
> +    memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
> +
> +    return 0;
if this does not return anything else than 0, shouldn't it be void?

Thanks

Eric
> +}
> +
>  static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
>                                    MemoryRegionSection *section, Error **errp)
>  {
>      ram_addr_t ram_addr;
>  
>      if (memory_region_is_iommu(section->mr)) {
> -        VFIOGuestIOMMU *giommu;
> -
> -        QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
> -            if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
> -                giommu->n.start == section->offset_within_region) {
> -                Int128 llend;
> -                vfio_giommu_dirty_notifier gdn = { .giommu = giommu };
> -                int idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
> -                                                       MEMTXATTRS_UNSPECIFIED);
> -
> -                llend = int128_add(int128_make64(section->offset_within_region),
> -                                   section->size);
> -                llend = int128_sub(llend, int128_one());
> -
> -                iommu_notifier_init(&gdn.n,
> -                                    vfio_iommu_map_dirty_notify,
> -                                    IOMMU_NOTIFIER_MAP,
> -                                    section->offset_within_region,
> -                                    int128_get64(llend),
> -                                    idx);
> -                memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
> -                break;
> -            }
> -        }
> -        return 0;
> +        return vfio_sync_iommu_dirty_bitmap(bcontainer, section);
>      } else if (memory_region_has_ram_discard_manager(section->mr)) {
>          int ret;
>  

Re: [PATCH v2 05/17] vfio/container: Introduce vfio_address_space_insert()

[Eric Auger]

On 6/17/24 08:33, Cédric Le Goater wrote:
> It will ease future changes.
Does it, really?

Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  include/hw/vfio/vfio-common.h | 2 ++
>  hw/vfio/common.c              | 6 ++++++
>  hw/vfio/container.c           | 2 +-
>  hw/vfio/iommufd.c             | 2 +-
>  4 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index c19572f90b277193491020af28e8b5587f15bfd1..825d80130bd435fe50830c8ae5b7905d18104dd6 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -206,6 +206,8 @@ typedef struct VFIODisplay {
>  
>  VFIOAddressSpace *vfio_get_address_space(AddressSpace *as);
>  void vfio_put_address_space(VFIOAddressSpace *space);
> +void vfio_address_space_insert(VFIOAddressSpace *space,
> +                               VFIOContainerBase *bcontainer);
>  
>  void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
>  void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index f28641bad5cf4b71fcdc0a6c9d42b24c8d786248..8cdf26c6f5a490cfa02bdf1087a91948709aaa33 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1508,6 +1508,12 @@ void vfio_put_address_space(VFIOAddressSpace *space)
>      }
>  }
>  
> +void vfio_address_space_insert(VFIOAddressSpace *space,
> +                               VFIOContainerBase *bcontainer)
> +{
> +    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
> +}
> +
>  struct vfio_device_info *vfio_get_device_info(int fd)
>  {
>      struct vfio_device_info *info;
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index c48749c089a67ee4d0e6b8dd975562e2938500cd..0237c216987ff64a6d11bef8688bb000d93a7f09 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -637,7 +637,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>      vfio_kvm_device_add_group(group);
>  
>      QLIST_INIT(&container->group_list);
> -    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
> +    vfio_address_space_insert(space, bcontainer);
>  
>      group->container = container;
>      QLIST_INSERT_HEAD(&container->group_list, group, container_next);
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index e502081c2ad9eda31769176f875fef60a77e2b43..9f8f33e383a38827ceca0f73cb77f5ca6b123198 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -358,7 +358,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>  
>      bcontainer = &container->bcontainer;
>      vfio_container_init(bcontainer, space, iommufd_vioc);
> -    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
> +    vfio_address_space_insert(space, bcontainer);
>  
>      if (!iommufd_cdev_attach_container(vbasedev, container, errp)) {
>          goto err_attach_container;

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

[Eric Auger]

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.

>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@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

Re: [PATCH v2 07/17] vfio/container: Modify vfio_get_iommu_type() to use a container fd

[Eric Auger]

On 6/17/24 08:33, Cédric Le Goater wrote:
> The 'container' pointer has no other use than its 'fd' attribute.
> Simplify the prototype to ease future changes.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/vfio/container.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index dc85a79cb9e62b72312f79da994c53608b6cef48..589f37bc6d68dae18f9e46371f14d6952b2240c0 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -354,7 +354,7 @@ static void vfio_kvm_device_del_group(VFIOGroup *group)
>  /*
>   * vfio_get_iommu_type - selects the richest iommu_type (v2 first)
>   */
> -static int vfio_get_iommu_type(VFIOContainer *container,
> +static int vfio_get_iommu_type(int container_fd,
>                                 Error **errp)
>  {
>      int iommu_types[] = { VFIO_TYPE1v2_IOMMU, VFIO_TYPE1_IOMMU,
> @@ -362,7 +362,7 @@ static int vfio_get_iommu_type(VFIOContainer *container,
>      int i;
>  
>      for (i = 0; i < ARRAY_SIZE(iommu_types); i++) {
> -        if (ioctl(container->fd, VFIO_CHECK_EXTENSION, iommu_types[i])) {
> +        if (ioctl(container_fd, VFIO_CHECK_EXTENSION, iommu_types[i])) {
>              return iommu_types[i];
>          }
>      }
> @@ -399,7 +399,7 @@ static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
>      int iommu_type;
>      const VFIOIOMMUClass *vioc;
>  
> -    iommu_type = vfio_get_iommu_type(container, errp);
> +    iommu_type = vfio_get_iommu_type(container->fd, errp);
>      if (iommu_type < 0) {
>          return false;
>      }

Re: [PATCH v2 08/17] vfio/container: Introduce vfio_get_iommu_class_name()

[Eric Auger]

On 6/17/24 08:34, Cédric Le Goater wrote:
> Rework vfio_get_iommu_class() to return a literal class name instead
> of a class object. We will need this name to instantiate the object
> later on. Since the default case asserts, remove the error report as
> QEMU will simply abort before.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Reviewed-by: Eric Auger <eric.auger@redhat.com>


Eric
> ---
>  hw/vfio/container.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 589f37bc6d68dae18f9e46371f14d6952b2240c0..bb6abe60ee29d5b69b494523c9002f53e1b2a3c8 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -373,24 +373,20 @@ static int vfio_get_iommu_type(int container_fd,
>  /*
>   * vfio_get_iommu_ops - get a VFIOIOMMUClass associated with a type
>   */
> -static const VFIOIOMMUClass *vfio_get_iommu_class(int iommu_type, Error **errp)
> +static const char *vfio_get_iommu_class_name(int iommu_type)
>  {
> -    ObjectClass *klass = NULL;
> -
>      switch (iommu_type) {
>      case VFIO_TYPE1v2_IOMMU:
>      case VFIO_TYPE1_IOMMU:
> -        klass = object_class_by_name(TYPE_VFIO_IOMMU_LEGACY);
> +        return TYPE_VFIO_IOMMU_LEGACY;
>          break;
>      case VFIO_SPAPR_TCE_v2_IOMMU:
>      case VFIO_SPAPR_TCE_IOMMU:
> -        klass = object_class_by_name(TYPE_VFIO_IOMMU_SPAPR);
> +        return TYPE_VFIO_IOMMU_SPAPR;
>          break;
>      default:
>          g_assert_not_reached();
>      };
> -
> -    return VFIO_IOMMU_CLASS(klass);
>  }
>  
>  static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
> @@ -398,6 +394,7 @@ static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
>  {
>      int iommu_type;
>      const VFIOIOMMUClass *vioc;
> +    const char *vioc_name;
>  
>      iommu_type = vfio_get_iommu_type(container->fd, errp);
>      if (iommu_type < 0) {
> @@ -426,11 +423,8 @@ static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
>  
>      container->iommu_type = iommu_type;
>  
> -    vioc = vfio_get_iommu_class(iommu_type, errp);
> -    if (!vioc) {
> -        error_setg(errp, "No available IOMMU models");
> -        return false;
> -    }
> +    vioc_name = vfio_get_iommu_class_name(iommu_type);
> +    vioc = VFIO_IOMMU_CLASS(object_class_by_name(vioc_name));
>  
>      vfio_container_init(&container->bcontainer, vioc);
>      return true;

Re: [PATCH v2 09/17] vfio/container: Introduce vfio_create_container()

[Eric Auger]

On 6/17/24 08:34, Cédric Le Goater wrote:
> This routine allocates the QEMU struct type representing the VFIO
> container. It is minimal currently and future changes will do more
> initialization.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/vfio/container.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index bb6abe60ee29d5b69b494523c9002f53e1b2a3c8..a8691942791006f44f7a3c34b32c67ca51766182 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -430,6 +430,16 @@ static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
>      return true;
>  }
>  
> +static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
> +                                            Error **errp)
> +{
> +    VFIOContainer *container;
> +
> +    container = g_malloc0(sizeof(*container));
> +    container->fd = fd;
> +    return container;
> +}
> +
>  static int vfio_get_iommu_info(VFIOContainer *container,
>                                 struct vfio_iommu_type1_info **info)
>  {
> @@ -604,13 +614,14 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>          goto close_fd_exit;
>      }
>  
> -    container = g_malloc0(sizeof(*container));
> -    container->fd = fd;
> -    bcontainer = &container->bcontainer;
> -
> +    container = vfio_create_container(fd, group, errp);
> +    if (!container) {
> +        goto close_fd_exit;
> +    }
>      if (!vfio_set_iommu(container, group->fd, errp)) {
>          goto free_container_exit;
>      }
> +    bcontainer = &container->bcontainer;
>  
>      if (!vfio_cpr_register_container(bcontainer, errp)) {
>          goto free_container_exit;

Re: [PATCH v2 10/17] vfio/container: Discover IOMMU type before creating the container

[Eric Auger]

On 6/17/24 08:34, Cédric Le Goater wrote:
> Since the QEMU struct type representing the VFIO container is deduced
> from the IOMMU type exposed by the host, this type should be well
> defined *before* creating the container struct. This will be necessary
> to instantiate a QOM object of the correct type in future changes.
>
> Rework vfio_set_iommu() to extract the part doing the container
> initialization and move it under vfio_create_container().
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/vfio/container.c | 47 ++++++++++++++++++++++-----------------------
>  1 file changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index a8691942791006f44f7a3c34b32c67ca51766182..31bdc46a96d1626b237227a25007957e1d472757 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -389,54 +389,56 @@ static const char *vfio_get_iommu_class_name(int iommu_type)
>      };
>  }
>  
> -static bool vfio_set_iommu(VFIOContainer *container, int group_fd,
> -                           Error **errp)
> +static bool vfio_set_iommu(int container_fd, int group_fd,
> +                           int *iommu_type, Error **errp)
>  {
> -    int iommu_type;
> -    const VFIOIOMMUClass *vioc;
> -    const char *vioc_name;
> -
> -    iommu_type = vfio_get_iommu_type(container->fd, errp);
> -    if (iommu_type < 0) {
> -        return false;
> -    }
> -
> -    if (ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
> +    if (ioctl(group_fd, VFIO_GROUP_SET_CONTAINER, &container_fd)) {
>          error_setg_errno(errp, errno, "Failed to set group container");
>          return false;
>      }
>  
> -    while (ioctl(container->fd, VFIO_SET_IOMMU, iommu_type)) {
> -        if (iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
> +    while (ioctl(container_fd, VFIO_SET_IOMMU, *iommu_type)) {
> +        if (*iommu_type == VFIO_SPAPR_TCE_v2_IOMMU) {
>              /*
>               * On sPAPR, despite the IOMMU subdriver always advertises v1 and
>               * v2, the running platform may not support v2 and there is no
>               * way to guess it until an IOMMU group gets added to the container.
>               * So in case it fails with v2, try v1 as a fallback.
>               */
> -            iommu_type = VFIO_SPAPR_TCE_IOMMU;
> +            *iommu_type = VFIO_SPAPR_TCE_IOMMU;
>              continue;
>          }
>          error_setg_errno(errp, errno, "Failed to set iommu for container");
>          return false;
>      }
>  
> -    container->iommu_type = iommu_type;
> -
> -    vioc_name = vfio_get_iommu_class_name(iommu_type);
> -    vioc = VFIO_IOMMU_CLASS(object_class_by_name(vioc_name));
> -
> -    vfio_container_init(&container->bcontainer, vioc);
>      return true;
>  }
>  
>  static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
>                                              Error **errp)
>  {
> +    int iommu_type;
> +    const VFIOIOMMUClass *vioc;
> +    const char *vioc_name;
>      VFIOContainer *container;
>  
> +    iommu_type = vfio_get_iommu_type(fd, errp);
> +    if (iommu_type < 0) {
> +        return NULL;
> +    }
> +
> +    if (!vfio_set_iommu(fd, group->fd, &iommu_type, errp)) {
> +        return NULL;
> +    }
> +
> +    vioc_name = vfio_get_iommu_class_name(iommu_type);
> +    vioc = VFIO_IOMMU_CLASS(object_class_by_name(vioc_name));
> +
>      container = g_malloc0(sizeof(*container));
>      container->fd = fd;
> +    container->iommu_type = iommu_type;
> +    vfio_container_init(&container->bcontainer, vioc);
>      return container;
>  }
>  
> @@ -618,9 +620,6 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>      if (!container) {
>          goto close_fd_exit;
>      }
> -    if (!vfio_set_iommu(container, group->fd, errp)) {
> -        goto free_container_exit;
> -    }
>      bcontainer = &container->bcontainer;
>  
>      if (!vfio_cpr_register_container(bcontainer, errp)) {

Re: [PATCH v2 11/17] vfio/container: Change VFIOContainerBase to use QOM

[Eric Auger]

On 6/17/24 08:34, Cédric Le Goater wrote:
> VFIOContainerBase was made a QOM interface because we believed that a
> QOM object would expose all the IOMMU backends to the QEMU machine and
> human interface. This only applies to user creatable devices or objects.
>
> Change the VFIOContainerBase nature from interface to object and make
> the necessary adjustments in the VFIO_IOMMU hierarchy.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  include/hw/vfio/vfio-common.h         |  4 ++++
>  include/hw/vfio/vfio-container-base.h | 12 +++---------
>  hw/vfio/container-base.c              |  4 +++-
>  hw/vfio/container.c                   |  1 +
>  hw/vfio/iommufd.c                     |  1 +
>  hw/vfio/spapr.c                       |  3 +++
>  6 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 825d80130bd435fe50830c8ae5b7905d18104dd6..e8ddf92bb18547f0d3b811b3d757cbae7fec8b8d 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -84,6 +84,8 @@ typedef struct VFIOContainer {
>      QLIST_HEAD(, VFIOGroup) group_list;
>  } VFIOContainer;
>  
> +OBJECT_DECLARE_SIMPLE_TYPE(VFIOContainer, VFIO_IOMMU_LEGACY);
> +
>  typedef struct VFIOHostDMAWindow {
>      hwaddr min_iova;
>      hwaddr max_iova;
> @@ -99,6 +101,8 @@ typedef struct VFIOIOMMUFDContainer {
>      uint32_t ioas_id;
>  } VFIOIOMMUFDContainer;
>  
> +OBJECT_DECLARE_SIMPLE_TYPE(VFIOIOMMUFDContainer, VFIO_IOMMU_IOMMUFD);
> +
>  typedef struct VFIODeviceOps VFIODeviceOps;
>  
>  typedef struct VFIODevice {
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index d505f63607ec40e6aa44aeb3e20848ac780562a1..b079b76f68975c5701a289ce9012e912a8e44fc6 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -34,6 +34,7 @@ typedef struct VFIOAddressSpace {
>   * This is the base object for vfio container backends
>   */
>  typedef struct VFIOContainerBase {
> +    Object parent;
>      const VFIOIOMMUClass *ops;
>      VFIOAddressSpace *space;
>      MemoryListener listener;
> @@ -96,17 +97,10 @@ void vfio_container_destroy(VFIOContainerBase *bcontainer);
>  #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr"
>  #define TYPE_VFIO_IOMMU_IOMMUFD TYPE_VFIO_IOMMU "-iommufd"
>  
> -/*
> - * VFIOContainerBase is not an abstract QOM object because it felt
> - * unnecessary to expose all the IOMMU backends to the QEMU machine
> - * and human interface. However, we can still abstract the IOMMU
> - * backend handlers using a QOM interface class. This provides more
> - * flexibility when referencing the various implementations.
> - */
> -DECLARE_CLASS_CHECKERS(VFIOIOMMUClass, VFIO_IOMMU, TYPE_VFIO_IOMMU)
> +OBJECT_DECLARE_TYPE(VFIOContainerBase, VFIOIOMMUClass, VFIO_IOMMU)
>  
>  struct VFIOIOMMUClass {
> -    InterfaceClass parent_class;
> +    ObjectClass parent_class;
>  
>      /* Properties */
>      const char *hiod_typename;
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 280f0dd2db1fc3939fe9925ce00a2c50d0e14196..98c15e174dd78df5146ee83c05c98f3ea9c1e52c 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -102,8 +102,10 @@ void vfio_container_destroy(VFIOContainerBase *bcontainer)
>  static const TypeInfo types[] = {
>      {
>          .name = TYPE_VFIO_IOMMU,
> -        .parent = TYPE_INTERFACE,
> +        .parent = TYPE_OBJECT,
> +        .instance_size = sizeof(VFIOContainerBase),
>          .class_size = sizeof(VFIOIOMMUClass),
> +        .abstract = true,
>      },
>  };
>  
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 31bdc46a96d1626b237227a25007957e1d472757..3ae52530a9b500bd53ec9f9e66c73253d97c9aba 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -1196,6 +1196,7 @@ static const TypeInfo types[] = {
>      {
>          .name = TYPE_VFIO_IOMMU_LEGACY,
>          .parent = TYPE_VFIO_IOMMU,
> +        .instance_size = sizeof(VFIOContainer),
>          .class_init = vfio_iommu_legacy_class_init,
>      }, {
>          .name = TYPE_HOST_IOMMU_DEVICE_LEGACY_VFIO,
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index e5d9334142418514215528b9523f12c031792c7f..3e9d642034c2d2234ea701952c94a78ab32e9147 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -672,6 +672,7 @@ static const TypeInfo types[] = {
>      {
>          .name = TYPE_VFIO_IOMMU_IOMMUFD,
>          .parent = TYPE_VFIO_IOMMU,
> +        .instance_size = sizeof(VFIOIOMMUFDContainer),
>          .class_init = vfio_iommu_iommufd_class_init,
>      }, {
>          .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO,
> diff --git a/hw/vfio/spapr.c b/hw/vfio/spapr.c
> index 47b040f1bcca7dd0b5cf052d941b43541e98a3c5..018bd2048194a6a2db83ed740025a7060181698f 100644
> --- a/hw/vfio/spapr.c
> +++ b/hw/vfio/spapr.c
> @@ -30,6 +30,8 @@ typedef struct VFIOSpaprContainer {
>      QLIST_HEAD(, VFIOHostDMAWindow) hostwin_list;
>  } VFIOSpaprContainer;
>  
> +OBJECT_DECLARE_SIMPLE_TYPE(VFIOSpaprContainer, VFIO_IOMMU_SPAPR);
> +
>  static bool vfio_prereg_listener_skipped_section(MemoryRegionSection *section)
>  {
>      if (memory_region_is_iommu(section->mr)) {
> @@ -548,6 +550,7 @@ static const TypeInfo types[] = {
>      {
>          .name = TYPE_VFIO_IOMMU_SPAPR,
>          .parent = TYPE_VFIO_IOMMU_LEGACY,
> +        .instance_size = sizeof(VFIOSpaprContainer),
>          .class_init = vfio_iommu_spapr_class_init,
>      },
>  };

Re: [PATCH v2 12/17] vfio/container: Switch to QOM

[Eric Auger]

On 6/17/24 08:34, Cédric Le Goater wrote:
> Instead of allocating the container struct, create a QOM object of the
> appropriate type.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric

> ---
>  hw/vfio/container.c | 6 +++---
>  hw/vfio/iommufd.c   | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 3ae52530a9b500bd53ec9f9e66c73253d97c9aba..ff3a6831da83c0fe11060cd57918c4d87b10197c 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -435,7 +435,7 @@ static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
>      vioc_name = vfio_get_iommu_class_name(iommu_type);
>      vioc = VFIO_IOMMU_CLASS(object_class_by_name(vioc_name));
>  
> -    container = g_malloc0(sizeof(*container));
> +    container = VFIO_IOMMU_LEGACY(object_new(vioc_name));
>      container->fd = fd;
>      container->iommu_type = iommu_type;
>      vfio_container_init(&container->bcontainer, vioc);
> @@ -674,7 +674,7 @@ unregister_container_exit:
>      vfio_cpr_unregister_container(bcontainer);
>  
>  free_container_exit:
> -    g_free(container);
> +    object_unref(container);
>  
>  close_fd_exit:
>      close(fd);
> @@ -718,7 +718,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
>          trace_vfio_disconnect_container(container->fd);
>          vfio_cpr_unregister_container(bcontainer);
>          close(container->fd);
> -        g_free(container);
> +        object_unref(container);
>  
>          vfio_put_address_space(space);
>      }
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 3e9d642034c2d2234ea701952c94a78ab32e9147..d59df858407f3cadb9405386ad673c99cdad61d0 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -239,7 +239,7 @@ static void iommufd_cdev_container_destroy(VFIOIOMMUFDContainer *container)
>      memory_listener_unregister(&bcontainer->listener);
>      vfio_container_destroy(bcontainer);
>      iommufd_backend_free_id(container->be, container->ioas_id);
> -    g_free(container);
> +    object_unref(container);
>  }
>  
>  static int iommufd_cdev_ram_block_discard_disable(bool state)
> @@ -352,7 +352,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>  
>      trace_iommufd_cdev_alloc_ioas(vbasedev->iommufd->fd, ioas_id);
>  
> -    container = g_malloc0(sizeof(*container));
> +    container = VFIO_IOMMU_IOMMUFD(object_new(TYPE_VFIO_IOMMU_IOMMUFD));
>      container->be = vbasedev->iommufd;
>      container->ioas_id = ioas_id;
>  

Re: [PATCH v2 13/17] vfio/container: Introduce an instance_init() handler

[Eric Auger]

On 6/17/24 08:34, Cédric Le Goater wrote:
> This allows us to move the initialization code from vfio_container_init(),
> which we will soon remove.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/vfio/container-base.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 98c15e174dd78df5146ee83c05c98f3ea9c1e52c..3858f5ab1d68e897f9013161d7c5c20c0553029d 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -75,12 +75,6 @@ void vfio_container_init(VFIOContainerBase *bcontainer,
>                           const VFIOIOMMUClass *ops)
>  {
>      bcontainer->ops = ops;
> -    bcontainer->error = NULL;
> -    bcontainer->dirty_pages_supported = false;
> -    bcontainer->dma_max_mappings = 0;
> -    bcontainer->iova_ranges = NULL;
> -    QLIST_INIT(&bcontainer->giommu_list);
> -    QLIST_INIT(&bcontainer->vrdl_list);
>  }
>  
>  void vfio_container_destroy(VFIOContainerBase *bcontainer)
> @@ -99,10 +93,23 @@ void vfio_container_destroy(VFIOContainerBase *bcontainer)
>      g_list_free_full(bcontainer->iova_ranges, g_free);
>  }
>  
> +static void vfio_container_instance_init(Object *obj)
> +{
> +    VFIOContainerBase *bcontainer = VFIO_IOMMU(obj);
> +
> +    bcontainer->error = NULL;
> +    bcontainer->dirty_pages_supported = false;
> +    bcontainer->dma_max_mappings = 0;
> +    bcontainer->iova_ranges = NULL;
> +    QLIST_INIT(&bcontainer->giommu_list);
> +    QLIST_INIT(&bcontainer->vrdl_list);
> +}
> +
>  static const TypeInfo types[] = {
>      {
>          .name = TYPE_VFIO_IOMMU,
>          .parent = TYPE_OBJECT,
> +        .instance_init = vfio_container_instance_init,
>          .instance_size = sizeof(VFIOContainerBase),
>          .class_size = sizeof(VFIOIOMMUClass),
>          .abstract = true,

Re: [PATCH v2 14/17] vfio/container: Remove VFIOContainerBase::ops

[Eric Auger]

On 6/17/24 08:34, Cédric Le Goater wrote:
> Instead, use VFIO_IOMMU_GET_CLASS() to get the class pointer.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  include/hw/vfio/vfio-container-base.h |  1 -
>  hw/vfio/common.c                      |  2 +-
>  hw/vfio/container-base.c              | 37 +++++++++++++++++----------
>  hw/vfio/container.c                   | 15 ++++++-----
>  hw/vfio/iommufd.c                     |  4 +--
>  hw/vfio/pci.c                         |  4 +--
>  6 files changed, 38 insertions(+), 25 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index b079b76f68975c5701a289ce9012e912a8e44fc6..6b57cd8e7f5d7d2817f6e3b96ce4566d2630bb12 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -35,7 +35,6 @@ typedef struct VFIOAddressSpace {
>   */
>  typedef struct VFIOContainerBase {
>      Object parent;
> -    const VFIOIOMMUClass *ops;
>      VFIOAddressSpace *space;
>      MemoryListener listener;
>      Error *error;
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 1686a0bed23bd95467bfb00a0c39a4d966e49cae..7cdb969fd396ae3815cb175ad631d93d7cca7006 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1573,5 +1573,5 @@ void vfio_detach_device(VFIODevice *vbasedev)
>          return;
>      }
>      object_unref(vbasedev->hiod);
> -    vbasedev->bcontainer->ops->detach_device(vbasedev);
> +    VFIO_IOMMU_GET_CLASS(vbasedev->bcontainer)->detach_device(vbasedev);
>  }
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 3858f5ab1d68e897f9013161d7c5c20c0553029d..24669d4d7472f49ac3adf2618a32bf7d82c5c344 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -19,62 +19,73 @@ int vfio_container_dma_map(VFIOContainerBase *bcontainer,
>                             hwaddr iova, ram_addr_t size,
>                             void *vaddr, bool readonly)
>  {
> -    g_assert(bcontainer->ops->dma_map);
> -    return bcontainer->ops->dma_map(bcontainer, iova, size, vaddr, readonly);
> +    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
> +
> +    g_assert(vioc->dma_map);
> +    return vioc->dma_map(bcontainer, iova, size, vaddr, readonly);
>  }
>  
>  int vfio_container_dma_unmap(VFIOContainerBase *bcontainer,
>                               hwaddr iova, ram_addr_t size,
>                               IOMMUTLBEntry *iotlb)
>  {
> -    g_assert(bcontainer->ops->dma_unmap);
> -    return bcontainer->ops->dma_unmap(bcontainer, iova, size, iotlb);
> +    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
> +
> +    g_assert(vioc->dma_unmap);
> +    return vioc->dma_unmap(bcontainer, iova, size, iotlb);
>  }
>  
>  bool vfio_container_add_section_window(VFIOContainerBase *bcontainer,
>                                         MemoryRegionSection *section,
>                                         Error **errp)
>  {
> -    if (!bcontainer->ops->add_window) {
> +    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
> +
> +    if (!vioc->add_window) {
>          return true;
>      }
>  
> -    return bcontainer->ops->add_window(bcontainer, section, errp);
> +    return vioc->add_window(bcontainer, section, errp);
>  }
>  
>  void vfio_container_del_section_window(VFIOContainerBase *bcontainer,
>                                         MemoryRegionSection *section)
>  {
> -    if (!bcontainer->ops->del_window) {
> +    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
> +
> +    if (!vioc->del_window) {
>          return;
>      }
>  
> -    return bcontainer->ops->del_window(bcontainer, section);
> +    return vioc->del_window(bcontainer, section);
>  }
>  
>  int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>                                             bool start, Error **errp)
>  {
> +    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
> +
>      if (!bcontainer->dirty_pages_supported) {
>          return 0;
>      }
>  
> -    g_assert(bcontainer->ops->set_dirty_page_tracking);
> -    return bcontainer->ops->set_dirty_page_tracking(bcontainer, start, errp);
> +    g_assert(vioc->set_dirty_page_tracking);
> +    return vioc->set_dirty_page_tracking(bcontainer, start, errp);
>  }
>  
>  int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>                     VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp)
>  {
> -    g_assert(bcontainer->ops->query_dirty_bitmap);
> -    return bcontainer->ops->query_dirty_bitmap(bcontainer, vbmap, iova, size,
> +    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
> +
> +    g_assert(vioc->query_dirty_bitmap);
> +    return vioc->query_dirty_bitmap(bcontainer, vbmap, iova, size,
>                                                 errp);
>  }
>  
>  void vfio_container_init(VFIOContainerBase *bcontainer,
>                           const VFIOIOMMUClass *ops)
>  {
> -    bcontainer->ops = ops;
>  }
>  
>  void vfio_container_destroy(VFIOContainerBase *bcontainer)
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index ff3a6831da83c0fe11060cd57918c4d87b10197c..a2f5fbad00cd228e27a47df5cd683dbb34296113 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -548,6 +548,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>      VFIOContainerBase *bcontainer;
>      int ret, fd;
>      VFIOAddressSpace *space;
> +    VFIOIOMMUClass *vioc;
>  
>      space = vfio_get_address_space(as);
>  
> @@ -632,9 +633,10 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>          goto unregister_container_exit;
>      }
>  
> -    assert(bcontainer->ops->setup);
> +    vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
> +    assert(vioc->setup);
>  
> -    if (!bcontainer->ops->setup(bcontainer, errp)) {
> +    if (!vioc->setup(bcontainer, errp)) {
>          goto enable_discards_exit;
>      }
>  
> @@ -663,8 +665,8 @@ listener_release_exit:
>      QLIST_REMOVE(bcontainer, next);
>      vfio_kvm_device_del_group(group);
>      memory_listener_unregister(&bcontainer->listener);
> -    if (bcontainer->ops->release) {
> -        bcontainer->ops->release(bcontainer);
> +    if (vioc->release) {
> +        vioc->release(bcontainer);
>      }
>  
>  enable_discards_exit:
> @@ -689,6 +691,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
>  {
>      VFIOContainer *container = group->container;
>      VFIOContainerBase *bcontainer = &container->bcontainer;
> +    VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(bcontainer);
>  
>      QLIST_REMOVE(group, container_next);
>      group->container = NULL;
> @@ -700,8 +703,8 @@ static void vfio_disconnect_container(VFIOGroup *group)
>       */
>      if (QLIST_EMPTY(&container->group_list)) {
>          memory_listener_unregister(&bcontainer->listener);
> -        if (bcontainer->ops->release) {
> -            bcontainer->ops->release(bcontainer);
> +        if (vioc->release) {
> +            vioc->release(bcontainer);
>          }
>      }
>  
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index d59df858407f3cadb9405386ad673c99cdad61d0..7bc76f80b48ea5422e68fd4d4cb3f5bca90993f6 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -324,7 +324,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>      /* try to attach to an existing container in this space */
>      QLIST_FOREACH(bcontainer, &space->containers, next) {
>          container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
> -        if (bcontainer->ops != iommufd_vioc ||
> +        if (VFIO_IOMMU_GET_CLASS(bcontainer) != iommufd_vioc ||
>              vbasedev->iommufd != container->be) {
>              continue;
>          }
> @@ -465,7 +465,7 @@ static VFIODevice *iommufd_cdev_pci_find_by_devid(__u32 devid)
>          VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
>  
>      QLIST_FOREACH(vbasedev_iter, &vfio_device_list, global_next) {
> -        if (vbasedev_iter->bcontainer->ops != iommufd_vioc) {
> +        if (VFIO_IOMMU_GET_CLASS(vbasedev_iter->bcontainer) != iommufd_vioc) {
>              continue;
>          }
>          if (devid == vbasedev_iter->devid) {
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d8a76c1ee003e6f5669e8390271836fd9d839a8a..e03d9f3ba5461f55f6351d937aba5d522a9128ec 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2511,9 +2511,9 @@ int vfio_pci_get_pci_hot_reset_info(VFIOPCIDevice *vdev,
>  static int vfio_pci_hot_reset(VFIOPCIDevice *vdev, bool single)
>  {
>      VFIODevice *vbasedev = &vdev->vbasedev;
> -    const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
> +    const VFIOIOMMUClass *vioc = VFIO_IOMMU_GET_CLASS(vbasedev->bcontainer);
>  
> -    return ops->pci_hot_reset(vbasedev, single);
> +    return vioc->pci_hot_reset(vbasedev, single);
>  }
>  
>  /*

Re: [PATCH v2 15/17] vfio/container: Remove vfio_container_init()

[Eric Auger]

On 6/17/24 08:34, Cédric Le Goater wrote:
> It's now empty.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  include/hw/vfio/vfio-container-base.h | 2 --
>  hw/vfio/container-base.c              | 5 -----
>  hw/vfio/container.c                   | 3 ---
>  hw/vfio/iommufd.c                     | 1 -
>  4 files changed, 11 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index 6b57cd8e7f5d7d2817f6e3b96ce4566d2630bb12..6242a62771caa8cf19440a53ad6f4db862ca12d7 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -86,8 +86,6 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>  int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>                     VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp);
>  
> -void vfio_container_init(VFIOContainerBase *bcontainer,
> -                         const VFIOIOMMUClass *ops);
>  void vfio_container_destroy(VFIOContainerBase *bcontainer);
>  
>  
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 24669d4d7472f49ac3adf2618a32bf7d82c5c344..970ae2356a92f87df44e1dd58ff8c67045a24ef1 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -83,11 +83,6 @@ int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>                                                 errp);
>  }
>  
> -void vfio_container_init(VFIOContainerBase *bcontainer,
> -                         const VFIOIOMMUClass *ops)
> -{
> -}
> -
>  void vfio_container_destroy(VFIOContainerBase *bcontainer)
>  {
>      VFIOGuestIOMMU *giommu, *tmp;
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index a2f5fbad00cd228e27a47df5cd683dbb34296113..3f2032d5c496de078c277ebacc49d7db89f4cc65 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -419,7 +419,6 @@ static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
>                                              Error **errp)
>  {
>      int iommu_type;
> -    const VFIOIOMMUClass *vioc;
>      const char *vioc_name;
>      VFIOContainer *container;
>  
> @@ -433,12 +432,10 @@ static VFIOContainer *vfio_create_container(int fd, VFIOGroup *group,
>      }
>  
>      vioc_name = vfio_get_iommu_class_name(iommu_type);
> -    vioc = VFIO_IOMMU_CLASS(object_class_by_name(vioc_name));
>  
>      container = VFIO_IOMMU_LEGACY(object_new(vioc_name));
>      container->fd = fd;
>      container->iommu_type = iommu_type;
> -    vfio_container_init(&container->bcontainer, vioc);
>      return container;
>  }
>  
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 7bc76f80b48ea5422e68fd4d4cb3f5bca90993f6..09b71a6617807c621275c74b924cfd39eb643961 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -357,7 +357,6 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>      container->ioas_id = ioas_id;
>  
>      bcontainer = &container->bcontainer;
> -    vfio_container_init(bcontainer, iommufd_vioc);
>      vfio_address_space_insert(space, bcontainer);
>  
>      if (!iommufd_cdev_attach_container(vbasedev, container, errp)) {

Re: [PATCH v2 16/17] vfio/container: Introduce vfio_iommu_legacy_instance_init()

[Eric Auger]

On 6/17/24 08:34, Cédric Le Goater wrote:
> Just as we did for the VFIOContainerBase object, introduce an
> instance_init() handler for the legacy VFIOContainer object and do the
> specific initialization there.
>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/vfio/container.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 3f2032d5c496de078c277ebacc49d7db89f4cc65..45123acbdd6a681f4ce7cae7aa2509100ea225ab 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -639,7 +639,6 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>  
>      vfio_kvm_device_add_group(group);
>  
> -    QLIST_INIT(&container->group_list);
>      vfio_address_space_insert(space, bcontainer);
>  
>      group->container = container;
> @@ -1183,6 +1182,13 @@ hiod_legacy_vfio_get_iova_ranges(HostIOMMUDevice *hiod, Error **errp)
>      return l;
>  }
>  
> +static void vfio_iommu_legacy_instance_init(Object *obj)
> +{
> +    VFIOContainer *container = VFIO_IOMMU_LEGACY(obj);
> +
> +    QLIST_INIT(&container->group_list);
> +}
> +
>  static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>  {
>      HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
> @@ -1196,6 +1202,7 @@ static const TypeInfo types[] = {
>      {
>          .name = TYPE_VFIO_IOMMU_LEGACY,
>          .parent = TYPE_VFIO_IOMMU,
> +        .instance_init = vfio_iommu_legacy_instance_init,
>          .instance_size = sizeof(VFIOContainer),
>          .class_init = vfio_iommu_legacy_class_init,
>      }, {

Re: [PATCH v2 17/17] vfio/container: Move vfio_container_destroy() to an instance_finalize() handler

[Eric Auger]

Hi Cédric,

On 6/17/24 08:34, Cédric Le Goater wrote:
> vfio_container_destroy() clears the resources allocated
> VFIOContainerBase object. Now that VFIOContainerBase is a QOM object,
> add an instance_finalize() handler to do the cleanup. It will be
> called through object_unref().
>
> Suggested-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
>  include/hw/vfio/vfio-container-base.h | 3 ---
>  hw/vfio/container-base.c              | 4 +++-
>  hw/vfio/container.c                   | 2 --
>  hw/vfio/iommufd.c                     | 1 -
>  4 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index 6242a62771caa8cf19440a53ad6f4db862ca12d7..419e45ee7a5ac960dae4a993127fc9ee66d48db2 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -86,9 +86,6 @@ int vfio_container_set_dirty_page_tracking(VFIOContainerBase *bcontainer,
>  int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>                     VFIOBitmap *vbmap, hwaddr iova, hwaddr size, Error **errp);
>  
> -void vfio_container_destroy(VFIOContainerBase *bcontainer);
> -
> -
>  #define TYPE_VFIO_IOMMU "vfio-iommu"
>  #define TYPE_VFIO_IOMMU_LEGACY TYPE_VFIO_IOMMU "-legacy"
>  #define TYPE_VFIO_IOMMU_SPAPR TYPE_VFIO_IOMMU "-spapr"
> diff --git a/hw/vfio/container-base.c b/hw/vfio/container-base.c
> index 970ae2356a92f87df44e1dd58ff8c67045a24ef1..50b1664f89a8192cf4021498e59f2a92cd2f6e89 100644
> --- a/hw/vfio/container-base.c
> +++ b/hw/vfio/container-base.c
> @@ -83,8 +83,9 @@ int vfio_container_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>                                                 errp);
>  }
>  
> -void vfio_container_destroy(VFIOContainerBase *bcontainer)
> +static void vfio_container_instance_finalize(Object *obj)
>  {
> +    VFIOContainerBase *bcontainer = VFIO_IOMMU(obj);
>      VFIOGuestIOMMU *giommu, *tmp;
>  
>      QLIST_REMOVE(bcontainer, next);
> @@ -116,6 +117,7 @@ static const TypeInfo types[] = {
>          .name = TYPE_VFIO_IOMMU,
>          .parent = TYPE_OBJECT,
>          .instance_init = vfio_container_instance_init,
> +        .instance_finalize = vfio_container_instance_finalize,
>          .instance_size = sizeof(VFIOContainerBase),
>          .class_size = sizeof(VFIOIOMMUClass),
>          .abstract = true,
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 45123acbdd6a681f4ce7cae7aa2509100ea225ab..2e7ecdf10edc4d84963a45ae9507096965da64fc 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -712,8 +712,6 @@ static void vfio_disconnect_container(VFIOGroup *group)
>      if (QLIST_EMPTY(&container->group_list)) {
>          VFIOAddressSpace *space = bcontainer->space;
>  
> -        vfio_container_destroy(bcontainer);
> -
>          trace_vfio_disconnect_container(container->fd);
>          vfio_cpr_unregister_container(bcontainer);
>          close(container->fd);
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 09b71a6617807c621275c74b924cfd39eb643961..c2f158e60386502eef267769ac9bce1effb67033 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -237,7 +237,6 @@ static void iommufd_cdev_container_destroy(VFIOIOMMUFDContainer *container)
>          return;
>      }
>      memory_listener_unregister(&bcontainer->listener);
> -    vfio_container_destroy(bcontainer);
>      iommufd_backend_free_id(container->be, container->ioas_id);
>      object_unref(container);
>  }

Re: [PATCH v2 00/17] vfio: QOMify VFIOContainer

[Eric Auger]

Hi Cédric,

On 6/17/24 08:33, Cédric Le Goater wrote:
> Hello,
>
> The series starts with simple changes (patch 1-4). Two of which were
> initially sent by Joao in a series adding VFIO migration support with
> vIOMMU [1].
>
> The changes following prepare VFIOContainer for QOMification, switch
> the container models to QOM when ready and add some final cleanups.
>
> Applies on top of :
>
>  * [v7] Add a host IOMMU device abstraction to check with vIOMMU
>    https://lore.kernel.org/all/20240605083043.317831-1-zhenzhong.duan@intel.com
>  * [v4] VIRTIO-IOMMU/VFIO: Fix host iommu geometry
>    https://lore.kernel.org/all/20240614095402.904691-1-eric.auger@redhat.com

For the whole series, feel free to add my

Tested-by: Eric Auger <eric.auger@redhat.com>

I tested with legacy/iommufd BE and with/without virtio-iommu

Eric

>
> Thanks,
>
> C.
>
> [1] https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.martins@oracle.com/
>
>
> Changes in v2:
>  - Used OBJECT_DECLARE_SIMPLE_TYPE
>  - Introduced a instance_finalize() handler
>
> Avihai Horon (1):
>   vfio/common: Extract vIOMMU code from vfio_sync_dirty_bitmap()
>
> Cédric Le Goater (15):
>   vfio: Make vfio_devices_dma_logging_start() return bool
>   vfio: Remove unused declarations from vfio-common.h
>   vfio/container: Introduce vfio_address_space_insert()
>   vfio/container: Simplify vfio_container_init()
>   vfio/container: Modify vfio_get_iommu_type() to use a container fd
>   vfio/container: Introduce vfio_get_iommu_class_name()
>   vfio/container: Introduce vfio_create_container()
>   vfio/container: Discover IOMMU type before creating the container
>   vfio/container: Change VFIOContainerBase to use QOM
>   vfio/container: Switch to QOM
>   vfio/container: Introduce an instance_init() handler
>   vfio/container: Remove VFIOContainerBase::ops
>   vfio/container: Remove vfio_container_init()
>   vfio/container: Introduce vfio_iommu_legacy_instance_init()
>   vfio/container: Move vfio_container_destroy() to an
>     instance_finalize() handler
>
> Joao Martins (1):
>   vfio/common: Move dirty tracking ranges update to helper
>
>  include/hw/vfio/vfio-common.h         |  10 ++-
>  include/hw/vfio/vfio-container-base.h |  19 +---
>  hw/vfio/common.c                      | 124 ++++++++++++++++----------
>  hw/vfio/container-base.c              |  70 +++++++++------
>  hw/vfio/container.c                   | 106 ++++++++++++----------
>  hw/vfio/iommufd.c                     |  13 ++-
>  hw/vfio/pci.c                         |   4 +-
>  hw/vfio/spapr.c                       |   3 +
>  8 files changed, 196 insertions(+), 153 deletions(-)
>

Re: [PATCH v2 03/17] vfio/common: Move dirty tracking ranges update to helper

[Cédric Le Goater]

On 6/17/24 1:39 PM, Eric Auger wrote:
> Hi Cédric,
> On 6/17/24 08:33, Cédric Le Goater wrote:
>> From: Joao Martins <joao.m.martins@oracle.com>
>>
>> Separate the changes that updates the ranges from the listener, to
> s/updates/update

fixed.

>> make it reusable in preparation to expand its use to vIOMMU support.
>>
>> [ clg: - Rebased on upstream
>>         - Introduced vfio_dirty_tracking_update_range() ]
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/vfio/common.c | 38 ++++++++++++++++++++++----------------
>>   1 file changed, 22 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index d48cd9b9361a92d184e423ffc60aabaff40fb487..fe215918bdf66ddbe3c5db803e10ce1aa9756b90 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -839,20 +839,11 @@ static bool vfio_section_is_vfio_pci(MemoryRegionSection *section,
>>       return false;
>>   }
>>   
>> -static void vfio_dirty_tracking_update(MemoryListener *listener,
>> -                                       MemoryRegionSection *section)
>> +static void vfio_dirty_tracking_update_range(VFIODirtyRanges *range,
>> +                                             hwaddr iova, hwaddr end,
>> +                                             bool update_pci)
>>   {
>> -    VFIODirtyRangesListener *dirty = container_of(listener,
>> -                                                  VFIODirtyRangesListener,
>> -                                                  listener);
>> -    VFIODirtyRanges *range = &dirty->ranges;
>> -    hwaddr iova, end, *min, *max;
>> -
>> -    if (!vfio_listener_valid_section(section, "tracking_update") ||
>> -        !vfio_get_section_iova_range(dirty->bcontainer, section,
>> -                                     &iova, &end, NULL)) {
>> -        return;
>> -    }
>> +    hwaddr *min, *max;
>>   
>>       /*
>>        * The address space passed to the dirty tracker is reduced to three ranges:
>> @@ -873,8 +864,7 @@ static void vfio_dirty_tracking_update(MemoryListener *listener,
>>        * The alternative would be an IOVATree but that has a much bigger runtime
>>        * overhead and unnecessary complexity.
>>        */
>> -    if (vfio_section_is_vfio_pci(section, dirty->bcontainer) &&
>> -        iova >= UINT32_MAX) {
>> +    if (update_pci && iova >= UINT32_MAX) {
>>           min = &range->minpci64;
>>           max = &range->maxpci64;
>>       } else {
>> @@ -889,7 +879,23 @@ static void vfio_dirty_tracking_update(MemoryListener *listener,
>>       }
>>   
>>       trace_vfio_device_dirty_tracking_update(iova, end, *min, *max);
> don't you want to update the name of the trace point?

There is only one caller. Let's reconsider when there are more users of this
routine.

Thanks,

C.

  
>> -    return;
>> +}
>> +
>> +static void vfio_dirty_tracking_update(MemoryListener *listener,
>> +                                       MemoryRegionSection *section)
>> +{
>> +    VFIODirtyRangesListener *dirty =
>> +        container_of(listener, VFIODirtyRangesListener, listener);
>> +    hwaddr iova, end;
>> +
>> +    if (!vfio_listener_valid_section(section, "tracking_update") ||
>> +        !vfio_get_section_iova_range(dirty->bcontainer, section,
>> +                                     &iova, &end, NULL)) {
>> +        return;
>> +    }
>> +
>> +    vfio_dirty_tracking_update_range(&dirty->ranges, iova, end,
>> +                      vfio_section_is_vfio_pci(section, dirty->bcontainer));
>>   }
>>   
>>   static const MemoryListener vfio_dirty_tracking_listener = {
> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> Eric
> 

Re: [PATCH v2 04/17] vfio/common: Extract vIOMMU code from vfio_sync_dirty_bitmap()

[Cédric Le Goater]

On 6/17/24 4:00 PM, Eric Auger wrote:
> Hi Cédric,
> 
> On 6/17/24 08:33, Cédric Le Goater wrote:
>> From: Avihai Horon <avihaih@nvidia.com>
>>
>> Extract vIOMMU code from vfio_sync_dirty_bitmap() to a new function and
>> restructure the code.
>>
>> This is done in preparation for optimizing vIOMMU deviice dirty page
> device

fixed.


>> tracking. No functional changes intended.
>>
>> Signed-off-by: Avihai Horon <avihaih@nvidia.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> [ clg: - Rebased on upstream ]
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/vfio/common.c | 63 +++++++++++++++++++++++++++++-------------------
>>   1 file changed, 38 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index fe215918bdf66ddbe3c5db803e10ce1aa9756b90..f28641bad5cf4b71fcdc0a6c9d42b24c8d786248 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1302,37 +1302,50 @@ vfio_sync_ram_discard_listener_dirty_bitmap(VFIOContainerBase *bcontainer,
>>                                                   &vrdl);
>>   }
>>   
>> +static int vfio_sync_iommu_dirty_bitmap(VFIOContainerBase *bcontainer,
>> +                                        MemoryRegionSection *section)
>> +{
>> +    VFIOGuestIOMMU *giommu;
>> +    bool found = false;
>> +    Int128 llend;
>> +    vfio_giommu_dirty_notifier gdn;
>> +    int idx;
>> +
>> +    QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
>> +        if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
>> +            giommu->n.start == section->offset_within_region) {
>> +            found = true;
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (!found) {
>> +        return 0;
>> +    }
>> +
>> +    gdn.giommu = giommu;
>> +    idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
>> +                                             MEMTXATTRS_UNSPECIFIED);
>> +
>> +    llend = int128_add(int128_make64(section->offset_within_region),
>> +                       section->size);
>> +    llend = int128_sub(llend, int128_one());
>> +
>> +    iommu_notifier_init(&gdn.n, vfio_iommu_map_dirty_notify, IOMMU_NOTIFIER_MAP,
>> +                        section->offset_within_region, int128_get64(llend),
>> +                        idx);
>> +    memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
>> +
>> +    return 0;
> if this does not return anything else than 0, shouldn't it be void?

Yes it could. The return value is practical for the caller below though. Let's
keep it that way for now, it's harmless.


Thanks,

C.


> 
> Thanks
> 
> Eric
>> +}
>> +
>>   static int vfio_sync_dirty_bitmap(VFIOContainerBase *bcontainer,
>>                                     MemoryRegionSection *section, Error **errp)
>>   {
>>       ram_addr_t ram_addr;
>>   
>>       if (memory_region_is_iommu(section->mr)) {
>> -        VFIOGuestIOMMU *giommu;
>> -
>> -        QLIST_FOREACH(giommu, &bcontainer->giommu_list, giommu_next) {
>> -            if (MEMORY_REGION(giommu->iommu_mr) == section->mr &&
>> -                giommu->n.start == section->offset_within_region) {
>> -                Int128 llend;
>> -                vfio_giommu_dirty_notifier gdn = { .giommu = giommu };
>> -                int idx = memory_region_iommu_attrs_to_index(giommu->iommu_mr,
>> -                                                       MEMTXATTRS_UNSPECIFIED);
>> -
>> -                llend = int128_add(int128_make64(section->offset_within_region),
>> -                                   section->size);
>> -                llend = int128_sub(llend, int128_one());
>> -
>> -                iommu_notifier_init(&gdn.n,
>> -                                    vfio_iommu_map_dirty_notify,
>> -                                    IOMMU_NOTIFIER_MAP,
>> -                                    section->offset_within_region,
>> -                                    int128_get64(llend),
>> -                                    idx);
>> -                memory_region_iommu_replay(giommu->iommu_mr, &gdn.n);
>> -                break;
>> -            }
>> -        }
>> -        return 0;
>> +        return vfio_sync_iommu_dirty_bitmap(bcontainer, section);
>>       } else if (memory_region_has_ram_discard_manager(section->mr)) {
>>           int ret;
>>   
> 

Re: [PATCH v2 05/17] vfio/container: Introduce vfio_address_space_insert()

[Cédric Le Goater]

On 6/17/24 4:04 PM, Eric Auger wrote:
> 
> 
> On 6/17/24 08:33, Cédric Le Goater wrote:
>> It will ease future changes.
> Does it, really?

Changed to :

   It prepares gound for a future change initializing the 'space' pointer
   of VFIOContainerBase. The goal is to replace vfio_container_init() by
   an .instance_init() handler when VFIOContainerBase is QOMified.

Thanks,

C.


> 
> Besides
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> Eric
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/hw/vfio/vfio-common.h | 2 ++
>>   hw/vfio/common.c              | 6 ++++++
>>   hw/vfio/container.c           | 2 +-
>>   hw/vfio/iommufd.c             | 2 +-
>>   4 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index c19572f90b277193491020af28e8b5587f15bfd1..825d80130bd435fe50830c8ae5b7905d18104dd6 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -206,6 +206,8 @@ typedef struct VFIODisplay {
>>   
>>   VFIOAddressSpace *vfio_get_address_space(AddressSpace *as);
>>   void vfio_put_address_space(VFIOAddressSpace *space);
>> +void vfio_address_space_insert(VFIOAddressSpace *space,
>> +                               VFIOContainerBase *bcontainer);
>>   
>>   void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
>>   void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index f28641bad5cf4b71fcdc0a6c9d42b24c8d786248..8cdf26c6f5a490cfa02bdf1087a91948709aaa33 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1508,6 +1508,12 @@ void vfio_put_address_space(VFIOAddressSpace *space)
>>       }
>>   }
>>   
>> +void vfio_address_space_insert(VFIOAddressSpace *space,
>> +                               VFIOContainerBase *bcontainer)
>> +{
>> +    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
>> +}
>> +
>>   struct vfio_device_info *vfio_get_device_info(int fd)
>>   {
>>       struct vfio_device_info *info;
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index c48749c089a67ee4d0e6b8dd975562e2938500cd..0237c216987ff64a6d11bef8688bb000d93a7f09 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -637,7 +637,7 @@ static bool vfio_connect_container(VFIOGroup *group, AddressSpace *as,
>>       vfio_kvm_device_add_group(group);
>>   
>>       QLIST_INIT(&container->group_list);
>> -    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
>> +    vfio_address_space_insert(space, bcontainer);
>>   
>>       group->container = container;
>>       QLIST_INSERT_HEAD(&container->group_list, group, container_next);
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index e502081c2ad9eda31769176f875fef60a77e2b43..9f8f33e383a38827ceca0f73cb77f5ca6b123198 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -358,7 +358,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>>   
>>       bcontainer = &container->bcontainer;
>>       vfio_container_init(bcontainer, space, iommufd_vioc);
>> -    QLIST_INSERT_HEAD(&space->containers, bcontainer, next);
>> +    vfio_address_space_insert(space, bcontainer);
>>   
>>       if (!iommufd_cdev_attach_container(vbasedev, container, errp)) {
>>           goto err_attach_container;
> 

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 <clg@redhat.com>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@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
> 

Re: [PATCH v2 00/17] vfio: QOMify VFIOContainer

[Cédric Le Goater]

On 6/17/24 6:22 PM, Eric Auger wrote:
> Hi Cédric,
> 
> On 6/17/24 08:33, Cédric Le Goater wrote:
>> Hello,
>>
>> The series starts with simple changes (patch 1-4). Two of which were
>> initially sent by Joao in a series adding VFIO migration support with
>> vIOMMU [1].
>>
>> The changes following prepare VFIOContainer for QOMification, switch
>> the container models to QOM when ready and add some final cleanups.
>>
>> Applies on top of :
>>
>>   * [v7] Add a host IOMMU device abstraction to check with vIOMMU
>>     https://lore.kernel.org/all/20240605083043.317831-1-zhenzhong.duan@intel.com
>>   * [v4] VIRTIO-IOMMU/VFIO: Fix host iommu geometry
>>     https://lore.kernel.org/all/20240614095402.904691-1-eric.auger@redhat.com
> 
> For the whole series, feel free to add my
> 
> Tested-by: Eric Auger <eric.auger@redhat.com>
> 
> I tested with legacy/iommufd BE and with/without virtio-iommu

Thanks !

Here is an updated version, with the new commit logs :

   https://github.com/legoater/qemu/commits/vfio-9.1

I hope to push on vfio-next soon, without the series "vfio: VFIO
migration support with vIOMMU".

C.

Re: [PATCH v2 00/17] vfio: QOMify VFIOContainer

[Cédric Le Goater]

On 6/17/24 8:33 AM, Cédric Le Goater wrote:
> Hello,
> 
> The series starts with simple changes (patch 1-4). Two of which were
> initially sent by Joao in a series adding VFIO migration support with
> vIOMMU [1].
> 
> The changes following prepare VFIOContainer for QOMification, switch
> the container models to QOM when ready and add some final cleanups.
> 
> Applies on top of :
> 
>   * [v7] Add a host IOMMU device abstraction to check with vIOMMU
>     https://lore.kernel.org/all/20240605083043.317831-1-zhenzhong.duan@intel.com
>   * [v4] VIRTIO-IOMMU/VFIO: Fix host iommu geometry
>     https://lore.kernel.org/all/20240614095402.904691-1-eric.auger@redhat.com
> 
> Thanks,
> 
> C.
> 
> [1] https://lore.kernel.org/qemu-devel/20230622214845.3980-1-joao.m.martins@oracle.com/
> 
> 
> Changes in v2:
>   - Used OBJECT_DECLARE_SIMPLE_TYPE
>   - Introduced a instance_finalize() handler
> 
> Avihai Horon (1):
>    vfio/common: Extract vIOMMU code from vfio_sync_dirty_bitmap()
> 
> Cédric Le Goater (15):
>    vfio: Make vfio_devices_dma_logging_start() return bool
>    vfio: Remove unused declarations from vfio-common.h
>    vfio/container: Introduce vfio_address_space_insert()
>    vfio/container: Simplify vfio_container_init()
>    vfio/container: Modify vfio_get_iommu_type() to use a container fd
>    vfio/container: Introduce vfio_get_iommu_class_name()
>    vfio/container: Introduce vfio_create_container()
>    vfio/container: Discover IOMMU type before creating the container
>    vfio/container: Change VFIOContainerBase to use QOM
>    vfio/container: Switch to QOM
>    vfio/container: Introduce an instance_init() handler
>    vfio/container: Remove VFIOContainerBase::ops
>    vfio/container: Remove vfio_container_init()
>    vfio/container: Introduce vfio_iommu_legacy_instance_init()
>    vfio/container: Move vfio_container_destroy() to an
>      instance_finalize() handler
> 
> Joao Martins (1):
>    vfio/common: Move dirty tracking ranges update to helper
> 
>   include/hw/vfio/vfio-common.h         |  10 ++-
>   include/hw/vfio/vfio-container-base.h |  19 +---
>   hw/vfio/common.c                      | 124 ++++++++++++++++----------
>   hw/vfio/container-base.c              |  70 +++++++++------
>   hw/vfio/container.c                   | 106 ++++++++++++----------
>   hw/vfio/iommufd.c                     |  13 ++-
>   hw/vfio/pci.c                         |   4 +-
>   hw/vfio/spapr.c                       |   3 +
>   8 files changed, 196 insertions(+), 153 deletions(-)
> 

Applied to vfio-next.

Thanks,

C.