[PATCH v2 00/10] Add a host IOMMU device abstraction

[PATCH v2 00/10] Add a host IOMMU device abstraction

[Zhenzhong Duan]

Based on Joao's suggestion, the iommufd nesting prerequisite series [1]
is further splitted to host IOMMU device abstract part and vIOMMU
check part. This series implements the 1st part.

This split also faciliates the dirty tracking series [2] and virtio-iommu
series [3] to depend on 1st part.

The major change in this version is to use QOM, the class tree is as below:

                        HostIOMMUDevice
                               | .get_host_iommu_info()
                               |
                               |
            .------------------------------------.
            |                  |                 |
      HIODLegacyVFIO    [HIODLegacyVDPA]    HIODIOMMUFD
            | .vdev            | [.vdev]         | .iommufd
                                                 | .devid
                                                 | [.ioas_id]
                                                 | [.attach_hwpt()]
                                                 | [.detach_hwpt()]
                                                 |
                                    .----------------------.
                                    |                      |
                           HIODIOMMUFDVFIO         [HIODIOMMUFDVDPA]
                                    | .vdev                | [.vdev]

* The classes in [] will be implemented in future.
* .ioas_id, .attach/detach_hwpt() will be implemented in nesting series.
* .vdev in different class points to different agent device,
* i.e., for VFIO it points to VFIODevice.

PATCH1-4: Introduce HostIOMMUDevice and its sub classes
PATCH5-7: Implement get_host_iommu_info() callback
PATCH8-10: Create HostIOMMUDevice instance and pass to vIOMMU

Qemu code can be found at:
https://github.com/yiliu1765/qemu/tree/zhenzhong/iommufd_nesting_preq_part1_v2

[1] https://lore.kernel.org/qemu-devel/20240201072818.327930-1-zhenzhong.duan@intel.com/
[2] https://lore.kernel.org/qemu-devel/20240212135643.5858-1-joao.m.martins@oracle.com/
[3] https://lore.kernel.org/qemu-devel/20240117080414.316890-1-eric.auger@redhat.com/

Thanks
Zhenzhong

Changelog:
v2:
- use QOM to abstract host IOMMU device and its sub-classes (Cédric)
- move host IOMMU device creation in attach_device() (Cédric)
- refine pci_device_set/unset_iommu_device doc futher (Eric)
- define host IOMMU info format of different backend
- implement get_host_iommu_info() for different backend (Cédric)

v1:
- use HostIOMMUDevice handle instead of union in VFIODevice (Eric)
- change host_iommu_device_init to host_iommu_device_create
- allocate HostIOMMUDevice in host_iommu_device_create callback
  and set the VFIODevice base_hdev handle (Eric)
- refine pci_device_set/unset_iommu_device doc (Eric)
- use HostIOMMUDevice handle instead of union in VTDHostIOMMUDevice (Eric)

rfcv2:
- introduce common abstract HostIOMMUDevice and sub struct for different BEs (Eric, Cédric)
- remove iommufd_device.[ch] (Cédric)
- remove duplicate iommufd/devid define from VFIODevice (Eric)
- drop the p in aliased_pbus and aliased_pdevfn (Eric)
- assert devfn and iommu_bus in pci_device_get_iommu_bus_devfn (Cédric, Eric)
- use errp in iommufd_device_get_info (Eric)
- split and simplify cap/ecap check/sync code in intel_iommu.c (Cédric)
- move VTDHostIOMMUDevice declaration to intel_iommu_internal.h (Cédric)
- make '(vtd->cap_reg >> 16) & 0x3fULL' a MACRO and add missed '+1' (Cédric)
- block migration if vIOMMU cap/ecap updated based on host IOMMU cap/ecap
- add R-B


Yi Liu (1):
  hw/pci: Introduce pci_device_set/unset_iommu_device()

Zhenzhong Duan (9):
  backends: Introduce abstract HostIOMMUDevice
  vfio: Introduce HIODLegacyVFIO device
  backends/iommufd: Introduce abstract HIODIOMMUFD device
  vfio/iommufd: Introduce HIODIOMMUFDVFIO device
  vfio: Implement get_host_iommu_info() callback
  backends/iommufd: Introduce helper function
    iommufd_backend_get_device_info()
  backends/iommufd: Implement get_host_iommu_info() callback
  vfio: Create host IOMMU device instance
  vfio: Pass HostIOMMUDevice to vIOMMU

 MAINTAINERS                        |  2 +
 include/hw/pci/pci.h               | 40 +++++++++++++-
 include/hw/vfio/vfio-common.h      | 23 ++++++++
 include/sysemu/host_iommu_device.h | 29 ++++++++++
 include/sysemu/iommufd.h           | 33 ++++++++++++
 backends/host_iommu_device.c       | 19 +++++++
 backends/iommufd.c                 | 85 ++++++++++++++++++++++++------
 hw/pci/pci.c                       | 75 ++++++++++++++++++++++++--
 hw/vfio/container.c                | 40 +++++++++++++-
 hw/vfio/iommufd.c                  | 19 ++++++-
 hw/vfio/pci.c                      | 20 +++++--
 backends/Kconfig                   |  5 ++
 backends/meson.build               |  1 +
 13 files changed, 364 insertions(+), 27 deletions(-)
 create mode 100644 include/sysemu/host_iommu_device.h
 create mode 100644 backends/host_iommu_device.c

-- 
2.34.1

[PATCH v2 01/10] backends: Introduce abstract HostIOMMUDevice

[Zhenzhong Duan]

Introduce HostIOMMUDevice as an abstraction of host IOMMU device.

get_host_iommu_info() is used to get host IOMMU info, different
backends can have different implementations and result format.

Introduce a macro CONFIG_HOST_IOMMU_DEVICE to define the usage
for VFIO, and VDPA in the future.

Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 MAINTAINERS                        |  2 ++
 include/sysemu/host_iommu_device.h | 19 +++++++++++++++++++
 backends/host_iommu_device.c       | 19 +++++++++++++++++++
 backends/Kconfig                   |  5 +++++
 backends/meson.build               |  1 +
 5 files changed, 46 insertions(+)
 create mode 100644 include/sysemu/host_iommu_device.h
 create mode 100644 backends/host_iommu_device.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e71183eef9..22f71cbe02 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2202,6 +2202,8 @@ M: Zhenzhong Duan <zhenzhong.duan@intel.com>
 S: Supported
 F: backends/iommufd.c
 F: include/sysemu/iommufd.h
+F: backends/host_iommu_device.c
+F: include/sysemu/host_iommu_device.h
 F: include/qemu/chardev_open.h
 F: util/chardev_open.c
 F: docs/devel/vfio-iommufd.rst
diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
new file mode 100644
index 0000000000..22ccbe3a5d
--- /dev/null
+++ b/include/sysemu/host_iommu_device.h
@@ -0,0 +1,19 @@
+#ifndef HOST_IOMMU_DEVICE_H
+#define HOST_IOMMU_DEVICE_H
+
+#include "qom/object.h"
+
+#define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
+OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUDeviceClass, HOST_IOMMU_DEVICE)
+
+struct HostIOMMUDevice {
+    Object parent;
+};
+
+struct HostIOMMUDeviceClass {
+    ObjectClass parent_class;
+
+    int (*get_host_iommu_info)(HostIOMMUDevice *hiod, void *data, uint32_t len,
+                               Error **errp);
+};
+#endif
diff --git a/backends/host_iommu_device.c b/backends/host_iommu_device.c
new file mode 100644
index 0000000000..6cb6007d8c
--- /dev/null
+++ b/backends/host_iommu_device.c
@@ -0,0 +1,19 @@
+#include "qemu/osdep.h"
+#include "sysemu/host_iommu_device.h"
+
+OBJECT_DEFINE_ABSTRACT_TYPE(HostIOMMUDevice,
+                            host_iommu_device,
+                            HOST_IOMMU_DEVICE,
+                            OBJECT)
+
+static void host_iommu_device_class_init(ObjectClass *oc, void *data)
+{
+}
+
+static void host_iommu_device_init(Object *obj)
+{
+}
+
+static void host_iommu_device_finalize(Object *obj)
+{
+}
diff --git a/backends/Kconfig b/backends/Kconfig
index 2cb23f62fa..34ab29e994 100644
--- a/backends/Kconfig
+++ b/backends/Kconfig
@@ -3,3 +3,8 @@ source tpm/Kconfig
 config IOMMUFD
     bool
     depends on VFIO
+
+config HOST_IOMMU_DEVICE
+    bool
+    default y
+    depends on VFIO
diff --git a/backends/meson.build b/backends/meson.build
index 8b2b111497..2e975d641e 100644
--- a/backends/meson.build
+++ b/backends/meson.build
@@ -25,6 +25,7 @@ if have_vhost_user
 endif
 system_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('cryptodev-vhost.c'))
 system_ss.add(when: 'CONFIG_IOMMUFD', if_true: files('iommufd.c'))
+system_ss.add(when: 'CONFIG_HOST_IOMMU_DEVICE', if_true: files('host_iommu_device.c'))
 if have_vhost_user_crypto
   system_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('cryptodev-vhost-user.c'))
 endif
-- 
2.34.1

[PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

[Zhenzhong Duan]

HIODLegacyVFIO represents a host IOMMU device under VFIO legacy
container backend.

It includes a link to VFIODevice.

Suggested-by: Eric Auger <eric.auger@redhat.com>
Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/vfio/vfio-common.h | 11 +++++++++++
 hw/vfio/container.c           | 11 ++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index b9da6c08ef..f30772f534 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -31,6 +31,7 @@
 #endif
 #include "sysemu/sysemu.h"
 #include "hw/vfio/vfio-container-base.h"
+#include "sysemu/host_iommu_device.h"
 
 #define VFIO_MSG_PREFIX "vfio %s: "
 
@@ -147,6 +148,16 @@ typedef struct VFIOGroup {
     bool ram_block_discard_allowed;
 } VFIOGroup;
 
+#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy-vfio"
+OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
+
+/* Abstraction of VFIO legacy host IOMMU device */
+struct HIODLegacyVFIO {
+    /*< private >*/
+    HostIOMMUDevice parent;
+    VFIODevice *vdev;
+};
+
 typedef struct VFIODMABuf {
     QemuDmaBuf buf;
     uint32_t pos_x, pos_y, pos_updates;
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 77bdec276e..44018ef085 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -1143,12 +1143,21 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
     vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
 };
 
+static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
+{
+};
+
 static const TypeInfo types[] = {
     {
         .name = TYPE_VFIO_IOMMU_LEGACY,
         .parent = TYPE_VFIO_IOMMU,
         .class_init = vfio_iommu_legacy_class_init,
-    },
+    }, {
+        .name = TYPE_HIOD_LEGACY_VFIO,
+        .parent = TYPE_HOST_IOMMU_DEVICE,
+        .instance_size = sizeof(HIODLegacyVFIO),
+        .class_init = hiod_legacy_vfio_class_init,
+    }
 };
 
 DEFINE_TYPES(types)
-- 
2.34.1

[PATCH v2 03/10] backends/iommufd: Introduce abstract HIODIOMMUFD device

[Zhenzhong Duan]

HIODIOMMUFD represents a host IOMMU device under iommufd backend.

Currently it includes only public iommufd handle and device id.
which could be used to get hw IOMMU information.

When nested translation is supported in future, vIOMMU is going
to have iommufd related operations like attaching/detaching hwpt,
So IOMMUFDDevice interface will be further extended at that time.

VFIO and VDPA device have different way of attaching/detaching hwpt.
So HIODIOMMUFD is still an abstract class which will be inherited by
VFIO and VDPA device.

Introduce a helper hiod_iommufd_init() to initialize HIODIOMMUFD
device.

Suggested-by: Cédric Le Goater <clg@redhat.com>
Originally-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/sysemu/iommufd.h | 22 +++++++++++++++++++
 backends/iommufd.c       | 47 ++++++++++++++++++++++++++--------------
 2 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
index 9af27ebd6c..71c53cbb45 100644
--- a/include/sysemu/iommufd.h
+++ b/include/sysemu/iommufd.h
@@ -4,6 +4,7 @@
 #include "qom/object.h"
 #include "exec/hwaddr.h"
 #include "exec/cpu-common.h"
+#include "sysemu/host_iommu_device.h"
 
 #define TYPE_IOMMUFD_BACKEND "iommufd"
 OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, IOMMUFD_BACKEND)
@@ -33,4 +34,25 @@ int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id, hwaddr iova,
                             ram_addr_t size, void *vaddr, bool readonly);
 int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
                               hwaddr iova, ram_addr_t size);
+
+#define TYPE_HIOD_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
+OBJECT_DECLARE_TYPE(HIODIOMMUFD, HIODIOMMUFDClass, HIOD_IOMMUFD)
+
+struct HIODIOMMUFD {
+    /*< private >*/
+    HostIOMMUDevice parent;
+    void *opaque;
+
+    /*< public >*/
+    IOMMUFDBackend *iommufd;
+    uint32_t devid;
+};
+
+struct HIODIOMMUFDClass {
+    /*< private >*/
+    HostIOMMUDeviceClass parent_class;
+};
+
+void hiod_iommufd_init(HIODIOMMUFD *idev, IOMMUFDBackend *iommufd,
+                       uint32_t devid);
 #endif
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 62a79fa6b0..ef8b3a808b 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -212,23 +212,38 @@ int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
     return ret;
 }
 
-static const TypeInfo iommufd_backend_info = {
-    .name = TYPE_IOMMUFD_BACKEND,
-    .parent = TYPE_OBJECT,
-    .instance_size = sizeof(IOMMUFDBackend),
-    .instance_init = iommufd_backend_init,
-    .instance_finalize = iommufd_backend_finalize,
-    .class_size = sizeof(IOMMUFDBackendClass),
-    .class_init = iommufd_backend_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { TYPE_USER_CREATABLE },
-        { }
-    }
-};
+void hiod_iommufd_init(HIODIOMMUFD *idev, IOMMUFDBackend *iommufd,
+                       uint32_t devid)
+{
+    idev->iommufd = iommufd;
+    idev->devid = devid;
+}
 
-static void register_types(void)
+static void hiod_iommufd_class_init(ObjectClass *oc, void *data)
 {
-    type_register_static(&iommufd_backend_info);
 }
 
-type_init(register_types);
+static const TypeInfo types[] = {
+    {
+        .name = TYPE_IOMMUFD_BACKEND,
+        .parent = TYPE_OBJECT,
+        .instance_size = sizeof(IOMMUFDBackend),
+        .instance_init = iommufd_backend_init,
+        .instance_finalize = iommufd_backend_finalize,
+        .class_size = sizeof(IOMMUFDBackendClass),
+        .class_init = iommufd_backend_class_init,
+        .interfaces = (InterfaceInfo[]) {
+            { TYPE_USER_CREATABLE },
+            { }
+        }
+    }, {
+        .name = TYPE_HIOD_IOMMUFD,
+        .parent = TYPE_HOST_IOMMU_DEVICE,
+        .instance_size = sizeof(HIODIOMMUFD),
+        .class_size = sizeof(HIODIOMMUFDClass),
+        .class_init = hiod_iommufd_class_init,
+        .abstract = true,
+    }
+};
+
+DEFINE_TYPES(types)
-- 
2.34.1

[PATCH v2 04/10] vfio/iommufd: Introduce HIODIOMMUFDVFIO device

[Zhenzhong Duan]

HIODIOMMUFDVFIO represents a host IOMMU device under VFIO iommufd
backend. It will be created during VFIO device attaching and passed
to vIOMMU.

It includes a link to VFIODevice so that we can do VFIO device
specific hwpt attaching/detaching.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/vfio/vfio-common.h | 11 +++++++++++
 hw/vfio/iommufd.c             | 11 ++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index f30772f534..d382b12ec1 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -32,6 +32,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/vfio/vfio-container-base.h"
 #include "sysemu/host_iommu_device.h"
+#include "sysemu/iommufd.h"
 
 #define VFIO_MSG_PREFIX "vfio %s: "
 
@@ -158,6 +159,16 @@ struct HIODLegacyVFIO {
     VFIODevice *vdev;
 };
 
+#define TYPE_HIOD_IOMMUFD_VFIO TYPE_HIOD_IOMMUFD "-vfio"
+OBJECT_DECLARE_SIMPLE_TYPE(HIODIOMMUFDVFIO, HIOD_IOMMUFD_VFIO)
+
+/* Abstraction of VFIO IOMMUFD host IOMMU device */
+struct HIODIOMMUFDVFIO {
+    /*< private >*/
+    HIODIOMMUFD parent;
+    VFIODevice *vdev;
+};
+
 typedef struct VFIODMABuf {
     QemuDmaBuf buf;
     uint32_t pos_x, pos_y, pos_updates;
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 8827ffe636..115b9f8e7f 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -634,12 +634,21 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
     vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset;
 };
 
+static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data)
+{
+};
+
 static const TypeInfo types[] = {
     {
         .name = TYPE_VFIO_IOMMU_IOMMUFD,
         .parent = TYPE_VFIO_IOMMU,
         .class_init = vfio_iommu_iommufd_class_init,
-    },
+    }, {
+        .name = TYPE_HIOD_IOMMUFD_VFIO,
+        .parent = TYPE_HIOD_IOMMUFD,
+        .instance_size = sizeof(HIODIOMMUFDVFIO),
+        .class_init = hiod_iommufd_vfio_class_init,
+    }
 };
 
 DEFINE_TYPES(types)
-- 
2.34.1

[PATCH v2 05/10] vfio: Implement get_host_iommu_info() callback

[Zhenzhong Duan]

Utilize iova_ranges to calculate host IOMMU address width and
package it in HIOD_LEGACY_INFO for vIOMMU usage.

HIOD_LEGACY_INFO will be used by both VFIO and VDPA so declare
it in host_iommu_device.h.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/sysemu/host_iommu_device.h | 10 ++++++++++
 hw/vfio/container.c                | 24 ++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
index 22ccbe3a5d..beb8be8231 100644
--- a/include/sysemu/host_iommu_device.h
+++ b/include/sysemu/host_iommu_device.h
@@ -16,4 +16,14 @@ struct HostIOMMUDeviceClass {
     int (*get_host_iommu_info)(HostIOMMUDevice *hiod, void *data, uint32_t len,
                                Error **errp);
 };
+
+/*
+ * Define the format of host IOMMU related info that current VFIO
+ * or VDPA can privode to vIOMMU.
+ *
+ * @aw_bits: Host IOMMU address width. 0xff if no limitation.
+ */
+typedef struct HIOD_LEGACY_INFO {
+    uint8_t aw_bits;
+} HIOD_LEGACY_INFO;
 #endif
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 44018ef085..ba0ad4a41b 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -1143,8 +1143,32 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
     vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
 };
 
+static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod,
+                                                void *data, uint32_t len,
+                                                Error **errp)
+{
+    VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev;
+    /* iova_ranges is a sorted list */
+    GList *l = g_list_last(vbasedev->bcontainer->iova_ranges);
+    HIOD_LEGACY_INFO *info = data;
+
+    assert(sizeof(HIOD_LEGACY_INFO) <= len);
+
+    if (l) {
+        Range *range = l->data;
+        info->aw_bits = find_last_bit(&range->upb, BITS_PER_LONG) + 1;
+    } else {
+        info->aw_bits = 0xff;
+    }
+
+    return 0;
+}
+
 static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
 {
+    HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
+
+    hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
 };
 
 static const TypeInfo types[] = {
-- 
2.34.1

[PATCH v2 06/10] backends/iommufd: Introduce helper function iommufd_backend_get_device_info()

[Zhenzhong Duan]

Introduce a helper function iommufd_backend_get_device_info() to get
host IOMMU related information through iommufd uAPI.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/sysemu/iommufd.h |  4 ++++
 backends/iommufd.c       | 23 ++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
index 71c53cbb45..fa1a866237 100644
--- a/include/sysemu/iommufd.h
+++ b/include/sysemu/iommufd.h
@@ -4,6 +4,7 @@
 #include "qom/object.h"
 #include "exec/hwaddr.h"
 #include "exec/cpu-common.h"
+#include <linux/iommufd.h>
 #include "sysemu/host_iommu_device.h"
 
 #define TYPE_IOMMUFD_BACKEND "iommufd"
@@ -34,6 +35,9 @@ int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id, hwaddr iova,
                             ram_addr_t size, void *vaddr, bool readonly);
 int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
                               hwaddr iova, ram_addr_t size);
+int iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
+                                    enum iommu_hw_info_type *type,
+                                    void *data, uint32_t len, Error **errp);
 
 #define TYPE_HIOD_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
 OBJECT_DECLARE_TYPE(HIODIOMMUFD, HIODIOMMUFDClass, HIOD_IOMMUFD)
diff --git a/backends/iommufd.c b/backends/iommufd.c
index ef8b3a808b..559affa9ec 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -20,7 +20,6 @@
 #include "monitor/monitor.h"
 #include "trace.h"
 #include <sys/ioctl.h>
-#include <linux/iommufd.h>
 
 static void iommufd_backend_init(Object *obj)
 {
@@ -212,6 +211,28 @@ int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
     return ret;
 }
 
+int iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
+                                    enum iommu_hw_info_type *type,
+                                    void *data, uint32_t len, Error **errp)
+{
+    struct iommu_hw_info info = {
+        .size = sizeof(info),
+        .dev_id = devid,
+        .data_len = len,
+        .data_uptr = (uintptr_t)data,
+    };
+    int ret;
+
+    ret = ioctl(be->fd, IOMMU_GET_HW_INFO, &info);
+    if (ret) {
+        error_setg_errno(errp, errno, "Failed to get hardware info");
+    } else {
+        *type = info.out_data_type;
+    }
+
+    return ret;
+}
+
 void hiod_iommufd_init(HIODIOMMUFD *idev, IOMMUFDBackend *iommufd,
                        uint32_t devid)
 {
-- 
2.34.1

[PATCH v2 07/10] backends/iommufd: Implement get_host_iommu_info() callback

[Zhenzhong Duan]

It calls iommufd_backend_get_device_info() to get host IOMMU
related information.

Define a common structure HIOD_IOMMUFD_INFO to describe the info
returned from kernel. Currently only vtd, but easy to add arm smmu
when kernel supports.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/sysemu/iommufd.h |  7 +++++++
 backends/iommufd.c       | 17 +++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
index fa1a866237..44ec1335b2 100644
--- a/include/sysemu/iommufd.h
+++ b/include/sysemu/iommufd.h
@@ -39,6 +39,13 @@ int iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
                                     enum iommu_hw_info_type *type,
                                     void *data, uint32_t len, Error **errp);
 
+typedef struct HIOD_IOMMUFD_INFO {
+    enum iommu_hw_info_type type;
+    union {
+        struct iommu_hw_info_vtd vtd;
+    } data;
+} HIOD_IOMMUFD_INFO;
+
 #define TYPE_HIOD_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
 OBJECT_DECLARE_TYPE(HIODIOMMUFD, HIODIOMMUFDClass, HIOD_IOMMUFD)
 
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 559affa9ec..1e9c469e65 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -240,8 +240,25 @@ void hiod_iommufd_init(HIODIOMMUFD *idev, IOMMUFDBackend *iommufd,
     idev->devid = devid;
 }
 
+static int hiod_iommufd_get_host_iommu_info(HostIOMMUDevice *hiod,
+                                            void *data, uint32_t len,
+                                            Error **errp)
+{
+    HIODIOMMUFD *idev = HIOD_IOMMUFD(hiod);
+    HIOD_IOMMUFD_INFO *info = data;
+
+    assert(sizeof(HIOD_IOMMUFD_INFO) <= len);
+
+    return iommufd_backend_get_device_info(idev->iommufd, idev->devid,
+                                           &info->type, &info->data,
+                                           sizeof(info->data), errp);
+}
+
 static void hiod_iommufd_class_init(ObjectClass *oc, void *data)
 {
+    HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc);
+
+    hiodc->get_host_iommu_info = hiod_iommufd_get_host_iommu_info;
 }
 
 static const TypeInfo types[] = {
-- 
2.34.1

[PATCH v2 08/10] vfio: Create host IOMMU device instance

[Zhenzhong Duan]

Create host IOMMU device instance and initialize it based on backend.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/vfio/vfio-common.h | 1 +
 hw/vfio/container.c           | 5 +++++
 hw/vfio/iommufd.c             | 8 ++++++++
 3 files changed, 14 insertions(+)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index d382b12ec1..4fbba85018 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -126,6 +126,7 @@ typedef struct VFIODevice {
     OnOffAuto pre_copy_dirty_page_tracking;
     bool dirty_pages_supported;
     bool dirty_tracking;
+    HostIOMMUDevice *hiod;
     int devid;
     IOMMUFDBackend *iommufd;
 } VFIODevice;
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index ba0ad4a41b..fc0c027501 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -915,6 +915,7 @@ static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
     VFIODevice *vbasedev_iter;
     VFIOGroup *group;
     VFIOContainerBase *bcontainer;
+    HIODLegacyVFIO *hiod_vfio;
     int ret;
 
     if (groupid < 0) {
@@ -945,6 +946,9 @@ static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
     vbasedev->bcontainer = bcontainer;
     QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev, container_next);
     QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
+    hiod_vfio = HIOD_LEGACY_VFIO(object_new(TYPE_HIOD_LEGACY_VFIO));
+    hiod_vfio->vdev = vbasedev;
+    vbasedev->hiod = HOST_IOMMU_DEVICE(hiod_vfio);
 
     return ret;
 }
@@ -959,6 +963,7 @@ static void vfio_legacy_detach_device(VFIODevice *vbasedev)
     trace_vfio_detach_device(vbasedev->name, group->groupid);
     vfio_put_base_device(vbasedev);
     vfio_put_group(group);
+    object_unref(vbasedev->hiod);
 }
 
 static int vfio_legacy_pci_hot_reset(VFIODevice *vbasedev, bool single)
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 115b9f8e7f..b6d058339b 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -308,6 +308,7 @@ static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
     VFIOIOMMUFDContainer *container;
     VFIOAddressSpace *space;
     struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) };
+    HIODIOMMUFDVFIO *hiod_vfio;
     int ret, devfd;
     uint32_t ioas_id;
     Error *err = NULL;
@@ -431,6 +432,12 @@ found_container:
     QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev, container_next);
     QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
 
+    hiod_vfio = HIOD_IOMMUFD_VFIO(object_new(TYPE_HIOD_IOMMUFD_VFIO));
+    hiod_iommufd_init(HIOD_IOMMUFD(hiod_vfio), vbasedev->iommufd,
+                      vbasedev->devid);
+    hiod_vfio->vdev = vbasedev;
+    vbasedev->hiod = HOST_IOMMU_DEVICE(hiod_vfio);
+
     trace_iommufd_cdev_device_info(vbasedev->name, devfd, vbasedev->num_irqs,
                                    vbasedev->num_regions, vbasedev->flags);
     return 0;
@@ -468,6 +475,7 @@ static void iommufd_cdev_detach(VFIODevice *vbasedev)
     iommufd_cdev_detach_container(vbasedev, container);
     iommufd_cdev_container_destroy(container);
     vfio_put_address_space(space);
+    object_unref(vbasedev->hiod);
 
     iommufd_cdev_unbind_and_disconnect(vbasedev);
     close(vbasedev->fd);
-- 
2.34.1

[PATCH v2 09/10] hw/pci: Introduce pci_device_set/unset_iommu_device()

[Zhenzhong Duan]

From: Yi Liu <yi.l.liu@intel.com>

This adds pci_device_set/unset_iommu_device() to set/unset
HostIOMMUDevice for a given PCI device. Caller of set
should fail if set operation fails.

Extract out pci_device_get_iommu_bus_devfn() to facilitate
implementation of pci_device_set/unset_iommu_device().

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/pci/pci.h | 40 ++++++++++++++++++++++-
 hw/pci/pci.c         | 75 ++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 111 insertions(+), 4 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index eaa3fc99d8..4ae7fe6f3f 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -3,6 +3,7 @@
 
 #include "exec/memory.h"
 #include "sysemu/dma.h"
+#include "sysemu/host_iommu_device.h"
 
 /* PCI includes legacy ISA access.  */
 #include "hw/isa/isa.h"
@@ -383,10 +384,47 @@ typedef struct PCIIOMMUOps {
      *
      * @devfn: device and function number
      */
-   AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int devfn);
+    AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int devfn);
+    /**
+     * @set_iommu_device: attach a HostIOMMUDevice to a vIOMMU
+     *
+     * Optional callback, if not implemented in vIOMMU, then vIOMMU can't
+     * retrieve host information from the associated HostIOMMUDevice.
+     *
+     * Return true if HostIOMMUDevice is attached, or else return false
+     * with errp set.
+     *
+     * @bus: the #PCIBus of the PCI device.
+     *
+     * @opaque: the data passed to pci_setup_iommu().
+     *
+     * @devfn: device and function number of the PCI device.
+     *
+     * @dev: the data structure representing host IOMMU device.
+     *
+     * @errp: pass an Error out only when return false
+     *
+     */
+    int (*set_iommu_device)(PCIBus *bus, void *opaque, int devfn,
+                            HostIOMMUDevice *dev, Error **errp);
+    /**
+     * @unset_iommu_device: detach a HostIOMMUDevice from a vIOMMU
+     *
+     * Optional callback.
+     *
+     * @bus: the #PCIBus of the PCI device.
+     *
+     * @opaque: the data passed to pci_setup_iommu().
+     *
+     * @devfn: device and function number of the PCI device.
+     */
+    void (*unset_iommu_device)(PCIBus *bus, void *opaque, int devfn);
 } PCIIOMMUOps;
 
 AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
+int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *hiod,
+                                Error **errp);
+void pci_device_unset_iommu_device(PCIDevice *dev);
 
 /**
  * pci_setup_iommu: Initialize specific IOMMU handlers for a PCIBus
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index e7a39cb203..8ece617673 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2648,11 +2648,27 @@ static void pci_device_class_base_init(ObjectClass *klass, void *data)
     }
 }
 
-AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
+/*
+ * Get IOMMU root bus, aliased bus and devfn of a PCI device
+ *
+ * IOMMU root bus is needed by all call sites to call into iommu_ops.
+ * For call sites which don't need aliased BDF, passing NULL to
+ * aliased_[bus/devfn] is allowed.
+ *
+ * @piommu_bus: return root #PCIBus backed by an IOMMU for the PCI device.
+ *
+ * @aliased_bus: return aliased #PCIBus of the PCI device, optional.
+ *
+ * @aliased_devfn: return aliased devfn of the PCI device, optional.
+ */
+static void pci_device_get_iommu_bus_devfn(PCIDevice *dev,
+                                           PCIBus **piommu_bus,
+                                           PCIBus **aliased_bus,
+                                           int *aliased_devfn)
 {
     PCIBus *bus = pci_get_bus(dev);
     PCIBus *iommu_bus = bus;
-    uint8_t devfn = dev->devfn;
+    int devfn = dev->devfn;
 
     while (iommu_bus && !iommu_bus->iommu_ops && iommu_bus->parent_dev) {
         PCIBus *parent_bus = pci_get_bus(iommu_bus->parent_dev);
@@ -2693,13 +2709,66 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
 
         iommu_bus = parent_bus;
     }
-    if (!pci_bus_bypass_iommu(bus) && iommu_bus->iommu_ops) {
+
+    assert(0 <= devfn && devfn < PCI_DEVFN_MAX);
+    assert(iommu_bus);
+
+    if (pci_bus_bypass_iommu(bus) || !iommu_bus->iommu_ops) {
+        iommu_bus = NULL;
+    }
+
+    *piommu_bus = iommu_bus;
+
+    if (aliased_bus) {
+        *aliased_bus = bus;
+    }
+
+    if (aliased_devfn) {
+        *aliased_devfn = devfn;
+    }
+}
+
+AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
+{
+    PCIBus *bus;
+    PCIBus *iommu_bus;
+    int devfn;
+
+    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
+    if (iommu_bus) {
         return iommu_bus->iommu_ops->get_address_space(bus,
                                  iommu_bus->iommu_opaque, devfn);
     }
     return &address_space_memory;
 }
 
+int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *hiod,
+                                Error **errp)
+{
+    PCIBus *iommu_bus;
+
+    /* set_iommu_device requires device's direct BDF instead of aliased BDF */
+    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, NULL, NULL);
+    if (iommu_bus && iommu_bus->iommu_ops->set_iommu_device) {
+        return iommu_bus->iommu_ops->set_iommu_device(pci_get_bus(dev),
+                                                      iommu_bus->iommu_opaque,
+                                                      dev->devfn, hiod, errp);
+    }
+    return 0;
+}
+
+void pci_device_unset_iommu_device(PCIDevice *dev)
+{
+    PCIBus *iommu_bus;
+
+    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, NULL, NULL);
+    if (iommu_bus && iommu_bus->iommu_ops->unset_iommu_device) {
+        return iommu_bus->iommu_ops->unset_iommu_device(pci_get_bus(dev),
+                                                        iommu_bus->iommu_opaque,
+                                                        dev->devfn);
+    }
+}
+
 void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
 {
     /*
-- 
2.34.1

[PATCH v2 10/10] vfio: Pass HostIOMMUDevice to vIOMMU

[Zhenzhong Duan]

With HostIOMMUDevice passed, vIOMMU can check compatibility with host
IOMMU, call into IOMMUFD specific methods, etc.

Originally-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/pci.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 64780d1b79..224501a86e 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3111,11 +3111,17 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
 
     vfio_bars_register(vdev);
 
-    ret = vfio_add_capabilities(vdev, errp);
+    ret = pci_device_set_iommu_device(pdev, vbasedev->hiod, errp);
     if (ret) {
+        error_prepend(errp, "Failed to set iommu_device: ");
         goto out_teardown;
     }
 
+    ret = vfio_add_capabilities(vdev, errp);
+    if (ret) {
+        goto out_unset_idev;
+    }
+
     if (vdev->vga) {
         vfio_vga_quirk_setup(vdev);
     }
@@ -3132,7 +3138,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
             error_setg(errp,
                        "cannot support IGD OpRegion feature on hotplugged "
                        "device");
-            goto out_teardown;
+            goto out_unset_idev;
         }
 
         ret = vfio_get_dev_region_info(vbasedev,
@@ -3141,13 +3147,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         if (ret) {
             error_setg_errno(errp, -ret,
                              "does not support requested IGD OpRegion feature");
-            goto out_teardown;
+            goto out_unset_idev;
         }
 
         ret = vfio_pci_igd_opregion_init(vdev, opregion, errp);
         g_free(opregion);
         if (ret) {
-            goto out_teardown;
+            goto out_unset_idev;
         }
     }
 
@@ -3233,6 +3239,8 @@ out_deregister:
     if (vdev->intx.mmap_timer) {
         timer_free(vdev->intx.mmap_timer);
     }
+out_unset_idev:
+    pci_device_unset_iommu_device(pdev);
 out_teardown:
     vfio_teardown_msi(vdev);
     vfio_bars_exit(vdev);
@@ -3261,6 +3269,7 @@ static void vfio_instance_finalize(Object *obj)
 static void vfio_exitfn(PCIDevice *pdev)
 {
     VFIOPCIDevice *vdev = VFIO_PCI(pdev);
+    VFIODevice *vbasedev = &vdev->vbasedev;
 
     vfio_unregister_req_notifier(vdev);
     vfio_unregister_err_notifier(vdev);
@@ -3275,7 +3284,8 @@ static void vfio_exitfn(PCIDevice *pdev)
     vfio_teardown_msi(vdev);
     vfio_pci_disable_rp_atomics(vdev);
     vfio_bars_exit(vdev);
-    vfio_migration_exit(&vdev->vbasedev);
+    vfio_migration_exit(vbasedev);
+    pci_device_unset_iommu_device(pdev);
 }
 
 static void vfio_pci_reset(DeviceState *dev)
-- 
2.34.1

RE: [PATCH v2 03/10] backends/iommufd: Introduce abstract HIODIOMMUFD device

[Duan, Zhenzhong]

Hi All,

>-----Original Message-----
>From: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>Subject: [PATCH v2 03/10] backends/iommufd: Introduce abstract
>HIODIOMMUFD device
>
>HIODIOMMUFD represents a host IOMMU device under iommufd backend.
>
>Currently it includes only public iommufd handle and device id.
>which could be used to get hw IOMMU information.
>
>When nested translation is supported in future, vIOMMU is going
>to have iommufd related operations like attaching/detaching hwpt,
>So IOMMUFDDevice interface will be further extended at that time.
>
>VFIO and VDPA device have different way of attaching/detaching hwpt.
>So HIODIOMMUFD is still an abstract class which will be inherited by
>VFIO and VDPA device.
>
>Introduce a helper hiod_iommufd_init() to initialize HIODIOMMUFD
>device.
>
>Suggested-by: Cédric Le Goater <clg@redhat.com>
>Originally-by: Yi Liu <yi.l.liu@intel.com>
>Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>---
> include/sysemu/iommufd.h | 22 +++++++++++++++++++
> backends/iommufd.c       | 47 ++++++++++++++++++++++++++--------------
> 2 files changed, 53 insertions(+), 16 deletions(-)
>
>diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>index 9af27ebd6c..71c53cbb45 100644
>--- a/include/sysemu/iommufd.h
>+++ b/include/sysemu/iommufd.h
>@@ -4,6 +4,7 @@
> #include "qom/object.h"
> #include "exec/hwaddr.h"
> #include "exec/cpu-common.h"
>+#include "sysemu/host_iommu_device.h"
>
> #define TYPE_IOMMUFD_BACKEND "iommufd"
> OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass,
>IOMMUFD_BACKEND)
>@@ -33,4 +34,25 @@ int iommufd_backend_map_dma(IOMMUFDBackend
>*be, uint32_t ioas_id, hwaddr iova,
>                             ram_addr_t size, void *vaddr, bool readonly);
> int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t
>ioas_id,
>                               hwaddr iova, ram_addr_t size);
>+
>+#define TYPE_HIOD_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
>+OBJECT_DECLARE_TYPE(HIODIOMMUFD, HIODIOMMUFDClass,
>HIOD_IOMMUFD)
>+
>+struct HIODIOMMUFD {
>+    /*< private >*/
>+    HostIOMMUDevice parent;
>+    void *opaque;

Please ignore above line "void *opaque;", it's totally useless, I forgot to remove it. Sorry for noise.

Thanks
Zhenzhong

Re: [PATCH v2 01/10] backends: Introduce abstract HostIOMMUDevice

[Cédric Le Goater]

On 4/8/24 10:12, Zhenzhong Duan wrote:
> Introduce HostIOMMUDevice as an abstraction of host IOMMU device.
> 
> get_host_iommu_info() is used to get host IOMMU info, different
> backends can have different implementations and result format.
> 
> Introduce a macro CONFIG_HOST_IOMMU_DEVICE to define the usage
> for VFIO, and VDPA in the future.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

LGTM,

> ---
>   MAINTAINERS                        |  2 ++
>   include/sysemu/host_iommu_device.h | 19 +++++++++++++++++++
>   backends/host_iommu_device.c       | 19 +++++++++++++++++++
>   backends/Kconfig                   |  5 +++++
>   backends/meson.build               |  1 +
>   5 files changed, 46 insertions(+)
>   create mode 100644 include/sysemu/host_iommu_device.h
>   create mode 100644 backends/host_iommu_device.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e71183eef9..22f71cbe02 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2202,6 +2202,8 @@ M: Zhenzhong Duan <zhenzhong.duan@intel.com>
>   S: Supported
>   F: backends/iommufd.c
>   F: include/sysemu/iommufd.h
> +F: backends/host_iommu_device.c
> +F: include/sysemu/host_iommu_device.h
>   F: include/qemu/chardev_open.h
>   F: util/chardev_open.c
>   F: docs/devel/vfio-iommufd.rst
> diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
> new file mode 100644
> index 0000000000..22ccbe3a5d
> --- /dev/null
> +++ b/include/sysemu/host_iommu_device.h
> @@ -0,0 +1,19 @@
> +#ifndef HOST_IOMMU_DEVICE_H
> +#define HOST_IOMMU_DEVICE_H
> +
> +#include "qom/object.h"
> +
> +#define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
> +OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUDeviceClass, HOST_IOMMU_DEVICE)
> +
> +struct HostIOMMUDevice {
> +    Object parent;
> +};
> +
> +struct HostIOMMUDeviceClass {
> +    ObjectClass parent_class;

Could you please document the struct and its handlers ? This is more for
the future reader to understand the VFIO concepts than for the generated
docs. Anyhow, it could be useful for the docs also. Overall, the QEMU VFIO
susbsytem suffers from a lack of documentation and we should try to improve
that in the next cycle.

Thanks,

C.



> +    int (*get_host_iommu_info)(HostIOMMUDevice *hiod, void *data, uint32_t len,
> +                               Error **errp);
> +};
> +#endif
> diff --git a/backends/host_iommu_device.c b/backends/host_iommu_device.c
> new file mode 100644
> index 0000000000..6cb6007d8c
> --- /dev/null
> +++ b/backends/host_iommu_device.c
> @@ -0,0 +1,19 @@
> +#include "qemu/osdep.h"
> +#include "sysemu/host_iommu_device.h"
> +
> +OBJECT_DEFINE_ABSTRACT_TYPE(HostIOMMUDevice,
> +                            host_iommu_device,
> +                            HOST_IOMMU_DEVICE,
> +                            OBJECT)
> +
> +static void host_iommu_device_class_init(ObjectClass *oc, void *data)
> +{
> +}
> +
> +static void host_iommu_device_init(Object *obj)
> +{
> +}
> +
> +static void host_iommu_device_finalize(Object *obj)
> +{
> +}
> diff --git a/backends/Kconfig b/backends/Kconfig
> index 2cb23f62fa..34ab29e994 100644
> --- a/backends/Kconfig
> +++ b/backends/Kconfig
> @@ -3,3 +3,8 @@ source tpm/Kconfig
>   config IOMMUFD
>       bool
>       depends on VFIO
> +
> +config HOST_IOMMU_DEVICE
> +    bool
> +    default y
> +    depends on VFIO
> diff --git a/backends/meson.build b/backends/meson.build
> index 8b2b111497..2e975d641e 100644
> --- a/backends/meson.build
> +++ b/backends/meson.build
> @@ -25,6 +25,7 @@ if have_vhost_user
>   endif
>   system_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('cryptodev-vhost.c'))
>   system_ss.add(when: 'CONFIG_IOMMUFD', if_true: files('iommufd.c'))
> +system_ss.add(when: 'CONFIG_HOST_IOMMU_DEVICE', if_true: files('host_iommu_device.c'))
>   if have_vhost_user_crypto
>     system_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('cryptodev-vhost-user.c'))
>   endif

Re: [PATCH v2 01/10] backends: Introduce abstract HostIOMMUDevice

[Philippe Mathieu-Daudé]

Hi Zhenzhong,

On 8/4/24 10:12, Zhenzhong Duan wrote:
> Introduce HostIOMMUDevice as an abstraction of host IOMMU device.
> 
> get_host_iommu_info() is used to get host IOMMU info, different
> backends can have different implementations and result format.
> 
> Introduce a macro CONFIG_HOST_IOMMU_DEVICE to define the usage
> for VFIO, and VDPA in the future.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   MAINTAINERS                        |  2 ++
>   include/sysemu/host_iommu_device.h | 19 +++++++++++++++++++
>   backends/host_iommu_device.c       | 19 +++++++++++++++++++
>   backends/Kconfig                   |  5 +++++
>   backends/meson.build               |  1 +
>   5 files changed, 46 insertions(+)
>   create mode 100644 include/sysemu/host_iommu_device.h
>   create mode 100644 backends/host_iommu_device.c


> diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
> new file mode 100644
> index 0000000000..22ccbe3a5d
> --- /dev/null
> +++ b/include/sysemu/host_iommu_device.h
> @@ -0,0 +1,19 @@
> +#ifndef HOST_IOMMU_DEVICE_H
> +#define HOST_IOMMU_DEVICE_H
> +
> +#include "qom/object.h"
> +
> +#define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
> +OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUDeviceClass, HOST_IOMMU_DEVICE)
> +
> +struct HostIOMMUDevice {
> +    Object parent;
> +};
> +
> +struct HostIOMMUDeviceClass {
> +    ObjectClass parent_class;
> +
> +    int (*get_host_iommu_info)(HostIOMMUDevice *hiod, void *data, uint32_t len,
> +                               Error **errp);

Please document this new method (in particular return value and @data).

Since @len is sizeof(data), can we use the size_t type?

Thanks,

Phil.

> +};
> +#endif

Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

[Philippe Mathieu-Daudé]

On 8/4/24 10:12, Zhenzhong Duan wrote:
> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy
> container backend.
> 
> It includes a link to VFIODevice.
> 
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/hw/vfio/vfio-common.h | 11 +++++++++++
>   hw/vfio/container.c           | 11 ++++++++++-
>   2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b9da6c08ef..f30772f534 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -31,6 +31,7 @@
>   #endif
>   #include "sysemu/sysemu.h"
>   #include "hw/vfio/vfio-container-base.h"
> +#include "sysemu/host_iommu_device.h"
>   
>   #define VFIO_MSG_PREFIX "vfio %s: "
>   
> @@ -147,6 +148,16 @@ typedef struct VFIOGroup {
>       bool ram_block_discard_allowed;
>   } VFIOGroup;
>   
> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy-vfio"
> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
> +
> +/* Abstraction of VFIO legacy host IOMMU device */
> +struct HIODLegacyVFIO {
> +    /*< private >*/

Please drop this comment.

> +    HostIOMMUDevice parent;

Please name 'parent_obj'.

> +    VFIODevice *vdev;
> +};

RE: [PATCH v2 01/10] backends: Introduce abstract HostIOMMUDevice

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 01/10] backends: Introduce abstract
>HostIOMMUDevice
>
>On 4/8/24 10:12, Zhenzhong Duan wrote:
>> Introduce HostIOMMUDevice as an abstraction of host IOMMU device.
>>
>> get_host_iommu_info() is used to get host IOMMU info, different
>> backends can have different implementations and result format.
>>
>> Introduce a macro CONFIG_HOST_IOMMU_DEVICE to define the usage
>> for VFIO, and VDPA in the future.
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
>LGTM,
>
>> ---
>>   MAINTAINERS                        |  2 ++
>>   include/sysemu/host_iommu_device.h | 19 +++++++++++++++++++
>>   backends/host_iommu_device.c       | 19 +++++++++++++++++++
>>   backends/Kconfig                   |  5 +++++
>>   backends/meson.build               |  1 +
>>   5 files changed, 46 insertions(+)
>>   create mode 100644 include/sysemu/host_iommu_device.h
>>   create mode 100644 backends/host_iommu_device.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e71183eef9..22f71cbe02 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2202,6 +2202,8 @@ M: Zhenzhong Duan
><zhenzhong.duan@intel.com>
>>   S: Supported
>>   F: backends/iommufd.c
>>   F: include/sysemu/iommufd.h
>> +F: backends/host_iommu_device.c
>> +F: include/sysemu/host_iommu_device.h
>>   F: include/qemu/chardev_open.h
>>   F: util/chardev_open.c
>>   F: docs/devel/vfio-iommufd.rst
>> diff --git a/include/sysemu/host_iommu_device.h
>b/include/sysemu/host_iommu_device.h
>> new file mode 100644
>> index 0000000000..22ccbe3a5d
>> --- /dev/null
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -0,0 +1,19 @@
>> +#ifndef HOST_IOMMU_DEVICE_H
>> +#define HOST_IOMMU_DEVICE_H
>> +
>> +#include "qom/object.h"
>> +
>> +#define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
>> +OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUDeviceClass,
>HOST_IOMMU_DEVICE)
>> +
>> +struct HostIOMMUDevice {
>> +    Object parent;
>> +};
>> +
>> +struct HostIOMMUDeviceClass {
>> +    ObjectClass parent_class;
>
>Could you please document the struct and its handlers ? This is more for
>the future reader to understand the VFIO concepts than for the generated
>docs. Anyhow, it could be useful for the docs also. Overall, the QEMU VFIO
>susbsytem suffers from a lack of documentation and we should try to
>improve that in the next cycle.

Sure, will doc struct and handlers in v3.

Thanks
Zhenzhong

>
>Thanks,
>
>C.
>
>
>
>> +    int (*get_host_iommu_info)(HostIOMMUDevice *hiod, void *data,
>uint32_t len,
>> +                               Error **errp);
>> +};
>> +#endif
>> diff --git a/backends/host_iommu_device.c
>b/backends/host_iommu_device.c
>> new file mode 100644
>> index 0000000000..6cb6007d8c
>> --- /dev/null
>> +++ b/backends/host_iommu_device.c
>> @@ -0,0 +1,19 @@
>> +#include "qemu/osdep.h"
>> +#include "sysemu/host_iommu_device.h"
>> +
>> +OBJECT_DEFINE_ABSTRACT_TYPE(HostIOMMUDevice,
>> +                            host_iommu_device,
>> +                            HOST_IOMMU_DEVICE,
>> +                            OBJECT)
>> +
>> +static void host_iommu_device_class_init(ObjectClass *oc, void *data)
>> +{
>> +}
>> +
>> +static void host_iommu_device_init(Object *obj)
>> +{
>> +}
>> +
>> +static void host_iommu_device_finalize(Object *obj)
>> +{
>> +}
>> diff --git a/backends/Kconfig b/backends/Kconfig
>> index 2cb23f62fa..34ab29e994 100644
>> --- a/backends/Kconfig
>> +++ b/backends/Kconfig
>> @@ -3,3 +3,8 @@ source tpm/Kconfig
>>   config IOMMUFD
>>       bool
>>       depends on VFIO
>> +
>> +config HOST_IOMMU_DEVICE
>> +    bool
>> +    default y
>> +    depends on VFIO
>> diff --git a/backends/meson.build b/backends/meson.build
>> index 8b2b111497..2e975d641e 100644
>> --- a/backends/meson.build
>> +++ b/backends/meson.build
>> @@ -25,6 +25,7 @@ if have_vhost_user
>>   endif
>>   system_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true: files('cryptodev-
>vhost.c'))
>>   system_ss.add(when: 'CONFIG_IOMMUFD', if_true: files('iommufd.c'))
>> +system_ss.add(when: 'CONFIG_HOST_IOMMU_DEVICE', if_true:
>files('host_iommu_device.c'))
>>   if have_vhost_user_crypto
>>     system_ss.add(when: 'CONFIG_VIRTIO_CRYPTO', if_true:
>files('cryptodev-vhost-user.c'))
>>   endif

RE: [PATCH v2 01/10] backends: Introduce abstract HostIOMMUDevice

[Duan, Zhenzhong]

>-----Original Message-----
>From: Philippe Mathieu-Daudé <philmd@linaro.org>
>Subject: Re: [PATCH v2 01/10] backends: Introduce abstract
>HostIOMMUDevice
>
>Hi Zhenzhong,
>
>On 8/4/24 10:12, Zhenzhong Duan wrote:
>> Introduce HostIOMMUDevice as an abstraction of host IOMMU device.
>>
>> get_host_iommu_info() is used to get host IOMMU info, different
>> backends can have different implementations and result format.
>>
>> Introduce a macro CONFIG_HOST_IOMMU_DEVICE to define the usage
>> for VFIO, and VDPA in the future.
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   MAINTAINERS                        |  2 ++
>>   include/sysemu/host_iommu_device.h | 19 +++++++++++++++++++
>>   backends/host_iommu_device.c       | 19 +++++++++++++++++++
>>   backends/Kconfig                   |  5 +++++
>>   backends/meson.build               |  1 +
>>   5 files changed, 46 insertions(+)
>>   create mode 100644 include/sysemu/host_iommu_device.h
>>   create mode 100644 backends/host_iommu_device.c
>
>
>> diff --git a/include/sysemu/host_iommu_device.h
>b/include/sysemu/host_iommu_device.h
>> new file mode 100644
>> index 0000000000..22ccbe3a5d
>> --- /dev/null
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -0,0 +1,19 @@
>> +#ifndef HOST_IOMMU_DEVICE_H
>> +#define HOST_IOMMU_DEVICE_H
>> +
>> +#include "qom/object.h"
>> +
>> +#define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
>> +OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUDeviceClass,
>HOST_IOMMU_DEVICE)
>> +
>> +struct HostIOMMUDevice {
>> +    Object parent;
>> +};
>> +
>> +struct HostIOMMUDeviceClass {
>> +    ObjectClass parent_class;
>> +
>> +    int (*get_host_iommu_info)(HostIOMMUDevice *hiod, void *data,
>uint32_t len,
>> +                               Error **errp);
>
>Please document this new method (in particular return value and @data).
>
>Since @len is sizeof(data), can we use the size_t type?

Sure, will do.

Thanks
Zhenzhong

RE: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

[Duan, Zhenzhong]

Hi Philippe,

>-----Original Message-----
>From: Philippe Mathieu-Daudé <philmd@linaro.org>
>Sent: Monday, April 15, 2024 5:20 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>devel@nongnu.org
>Cc: alex.williamson@redhat.com; clg@redhat.com; eric.auger@redhat.com;
>peterx@redhat.com; jasowang@redhat.com; mst@redhat.com;
>jgg@nvidia.com; nicolinc@nvidia.com; joao.m.martins@oracle.com; Tian,
>Kevin <kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao P
><chao.p.peng@intel.com>
>Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>
>On 8/4/24 10:12, Zhenzhong Duan wrote:
>> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy
>> container backend.
>>
>> It includes a link to VFIODevice.
>>
>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/hw/vfio/vfio-common.h | 11 +++++++++++
>>   hw/vfio/container.c           | 11 ++++++++++-
>>   2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>> index b9da6c08ef..f30772f534 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -31,6 +31,7 @@
>>   #endif
>>   #include "sysemu/sysemu.h"
>>   #include "hw/vfio/vfio-container-base.h"
>> +#include "sysemu/host_iommu_device.h"
>>
>>   #define VFIO_MSG_PREFIX "vfio %s: "
>>
>> @@ -147,6 +148,16 @@ typedef struct VFIOGroup {
>>       bool ram_block_discard_allowed;
>>   } VFIOGroup;
>>
>> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy-
>vfio"
>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
>> +
>> +/* Abstraction of VFIO legacy host IOMMU device */
>> +struct HIODLegacyVFIO {
>> +    /*< private >*/
>
>Please drop this comment.

Will do. But may I ask the rules when to use that comment and when not?
I see some QOM use that comment to mark private vs. public, for example:

struct AccelState {
    /*< private >*/
    Object parent_obj;
};

typedef struct AccelClass {
    /*< private >*/
    ObjectClass parent_class;
    /*< public >*/

>
>> +    HostIOMMUDevice parent;
>
>Please name 'parent_obj'.

Will do.

Thanks
Zhenzhong

>
>> +    VFIODevice *vdev;
>> +};

Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

[Philippe Mathieu-Daudé]

On 15/4/24 12:10, Duan, Zhenzhong wrote:
> Hi Philippe,
> 
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Sent: Monday, April 15, 2024 5:20 PM
>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>> devel@nongnu.org
>> Cc: alex.williamson@redhat.com; clg@redhat.com; eric.auger@redhat.com;
>> peterx@redhat.com; jasowang@redhat.com; mst@redhat.com;
>> jgg@nvidia.com; nicolinc@nvidia.com; joao.m.martins@oracle.com; Tian,
>> Kevin <kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao P
>> <chao.p.peng@intel.com>
>> Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>>
>> On 8/4/24 10:12, Zhenzhong Duan wrote:
>>> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy
>>> container backend.
>>>
>>> It includes a link to VFIODevice.
>>>
>>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    include/hw/vfio/vfio-common.h | 11 +++++++++++
>>>    hw/vfio/container.c           | 11 ++++++++++-
>>>    2 files changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>> common.h
>>> index b9da6c08ef..f30772f534 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -31,6 +31,7 @@
>>>    #endif
>>>    #include "sysemu/sysemu.h"
>>>    #include "hw/vfio/vfio-container-base.h"
>>> +#include "sysemu/host_iommu_device.h"
>>>
>>>    #define VFIO_MSG_PREFIX "vfio %s: "
>>>
>>> @@ -147,6 +148,16 @@ typedef struct VFIOGroup {
>>>        bool ram_block_discard_allowed;
>>>    } VFIOGroup;
>>>
>>> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy-
>> vfio"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
>>> +
>>> +/* Abstraction of VFIO legacy host IOMMU device */
>>> +struct HIODLegacyVFIO {
>>> +    /*< private >*/
>>
>> Please drop this comment.
> 
> Will do. But may I ask the rules when to use that comment and when not?

Sure, see 
https://www.qemu.org/docs/master/devel/style.html#qemu-object-model-declarations

> I see some QOM use that comment to mark private vs. public, for example:
> 
> struct AccelState {
>      /*< private >*/
>      Object parent_obj;

This is old style which might be cleaned some day...

> };
> 
> typedef struct AccelClass {
>      /*< private >*/
>      ObjectClass parent_class;
>      /*< public >*/
> 
>>
>>> +    HostIOMMUDevice parent;
>>
>> Please name 'parent_obj'.
> 
> Will do.

Thanks,

Phil.

> 
> Thanks
> Zhenzhong
> 
>>
>>> +    VFIODevice *vdev;
>>> +};
> 

RE: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

[Duan, Zhenzhong]

>-----Original Message-----
>From: Philippe Mathieu-Daudé <philmd@linaro.org>
>Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>
>On 15/4/24 12:10, Duan, Zhenzhong wrote:
>> Hi Philippe,
>>
>>> -----Original Message-----
>>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Sent: Monday, April 15, 2024 5:20 PM
>>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>>> devel@nongnu.org
>>> Cc: alex.williamson@redhat.com; clg@redhat.com;
>eric.auger@redhat.com;
>>> peterx@redhat.com; jasowang@redhat.com; mst@redhat.com;
>>> jgg@nvidia.com; nicolinc@nvidia.com; joao.m.martins@oracle.com; Tian,
>>> Kevin <kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; Peng, Chao P
>>> <chao.p.peng@intel.com>
>>> Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>>>
>>> On 8/4/24 10:12, Zhenzhong Duan wrote:
>>>> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy
>>>> container backend.
>>>>
>>>> It includes a link to VFIODevice.
>>>>
>>>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>>>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>>    include/hw/vfio/vfio-common.h | 11 +++++++++++
>>>>    hw/vfio/container.c           | 11 ++++++++++-
>>>>    2 files changed, 21 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>>> common.h
>>>> index b9da6c08ef..f30772f534 100644
>>>> --- a/include/hw/vfio/vfio-common.h
>>>> +++ b/include/hw/vfio/vfio-common.h
>>>> @@ -31,6 +31,7 @@
>>>>    #endif
>>>>    #include "sysemu/sysemu.h"
>>>>    #include "hw/vfio/vfio-container-base.h"
>>>> +#include "sysemu/host_iommu_device.h"
>>>>
>>>>    #define VFIO_MSG_PREFIX "vfio %s: "
>>>>
>>>> @@ -147,6 +148,16 @@ typedef struct VFIOGroup {
>>>>        bool ram_block_discard_allowed;
>>>>    } VFIOGroup;
>>>>
>>>> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-
>legacy-
>>> vfio"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
>>>> +
>>>> +/* Abstraction of VFIO legacy host IOMMU device */
>>>> +struct HIODLegacyVFIO {
>>>> +    /*< private >*/
>>>
>>> Please drop this comment.
>>
>> Will do. But may I ask the rules when to use that comment and when not?
>
>Sure, see
>https://www.qemu.org/docs/master/devel/style.html#qemu-object-model-
>declarations

Learned, thanks Philippe.

BRs.
Zhenzhong

Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

[Cédric Le Goater]

On 4/8/24 10:12, Zhenzhong Duan wrote:
> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy
> container backend.
> 
> It includes a link to VFIODevice.
> 
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/hw/vfio/vfio-common.h | 11 +++++++++++
>   hw/vfio/container.c           | 11 ++++++++++-
>   2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b9da6c08ef..f30772f534 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -31,6 +31,7 @@
>   #endif
>   #include "sysemu/sysemu.h"
>   #include "hw/vfio/vfio-container-base.h"
> +#include "sysemu/host_iommu_device.h"
>   
>   #define VFIO_MSG_PREFIX "vfio %s: "
>   
> @@ -147,6 +148,16 @@ typedef struct VFIOGroup {
>       bool ram_block_discard_allowed;
>   } VFIOGroup;
>   
> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy-vfio"

I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE.

> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
> +
> +/* Abstraction of VFIO legacy host IOMMU device */
> +struct HIODLegacyVFIO {

same here

> +    /*< private >*/
> +    HostIOMMUDevice parent;
> +    VFIODevice *vdev;

It seems to me that the back pointer should be on the container instead.
Looks more correct conceptually.


> +};
> +
>   typedef struct VFIODMABuf {
>       QemuDmaBuf buf;
>       uint32_t pos_x, pos_y, pos_updates;
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 77bdec276e..44018ef085 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -1143,12 +1143,21 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>       vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>   };
>   
> +static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
> +{
> +};

Is it preferable to introduce routines when they are actually useful.
Please drop the .class_init definition.

Thanks,

C.


> +
>   static const TypeInfo types[] = {
>       {
>           .name = TYPE_VFIO_IOMMU_LEGACY,
>           .parent = TYPE_VFIO_IOMMU,
>           .class_init = vfio_iommu_legacy_class_init,
> -    },
> +    }, {
> +        .name = TYPE_HIOD_LEGACY_VFIO,
> +        .parent = TYPE_HOST_IOMMU_DEVICE,
> +        .instance_size = sizeof(HIODLegacyVFIO),
> +        .class_init = hiod_legacy_vfio_class_init,
> +    }
>   };
>   
>   DEFINE_TYPES(types)

Re: [PATCH v2 03/10] backends/iommufd: Introduce abstract HIODIOMMUFD device

[Cédric Le Goater]

On 4/8/24 10:12, Zhenzhong Duan wrote:
> HIODIOMMUFD represents a host IOMMU device under iommufd backend.
> 
> Currently it includes only public iommufd handle and device id.
> which could be used to get hw IOMMU information.
> 
> When nested translation is supported in future, vIOMMU is going
> to have iommufd related operations like attaching/detaching hwpt,
> So IOMMUFDDevice interface will be further extended at that time.
> 
> VFIO and VDPA device have different way of attaching/detaching hwpt.
> So HIODIOMMUFD is still an abstract class which will be inherited by
> VFIO and VDPA device.
> 
> Introduce a helper hiod_iommufd_init() to initialize HIODIOMMUFD
> device.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Originally-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/sysemu/iommufd.h | 22 +++++++++++++++++++
>   backends/iommufd.c       | 47 ++++++++++++++++++++++++++--------------
>   2 files changed, 53 insertions(+), 16 deletions(-)
> 
> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
> index 9af27ebd6c..71c53cbb45 100644
> --- a/include/sysemu/iommufd.h
> +++ b/include/sysemu/iommufd.h
> @@ -4,6 +4,7 @@
>   #include "qom/object.h"
>   #include "exec/hwaddr.h"
>   #include "exec/cpu-common.h"
> +#include "sysemu/host_iommu_device.h"
>   
>   #define TYPE_IOMMUFD_BACKEND "iommufd"
>   OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, IOMMUFD_BACKEND)
> @@ -33,4 +34,25 @@ int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id, hwaddr iova,
>                               ram_addr_t size, void *vaddr, bool readonly);
>   int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
>                                 hwaddr iova, ram_addr_t size);
> +
> +#define TYPE_HIOD_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"

Please keep TYPE_HOST_IOMMU_DEVICE

> +OBJECT_DECLARE_TYPE(HIODIOMMUFD, HIODIOMMUFDClass, HIOD_IOMMUFD)
> +
> +struct HIODIOMMUFD {
> +    /*< private >*/
> +    HostIOMMUDevice parent;
> +    void *opaque;
> +
> +    /*< public >*/
> +    IOMMUFDBackend *iommufd;
> +    uint32_t devid;
> +};
> +
> +struct HIODIOMMUFDClass {
> +    /*< private >*/
> +    HostIOMMUDeviceClass parent_class;
> +};

This new class doesn't seem useful. Do you have plans for handlers ?

> +
> +void hiod_iommufd_init(HIODIOMMUFD *idev, IOMMUFDBackend *iommufd,
> +                       uint32_t devid);
>   #endif
> diff --git a/backends/iommufd.c b/backends/iommufd.c
> index 62a79fa6b0..ef8b3a808b 100644
> --- a/backends/iommufd.c
> +++ b/backends/iommufd.c
> @@ -212,23 +212,38 @@ int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
>       return ret;
>   }
>   
> -static const TypeInfo iommufd_backend_info = {
> -    .name = TYPE_IOMMUFD_BACKEND,
> -    .parent = TYPE_OBJECT,
> -    .instance_size = sizeof(IOMMUFDBackend),
> -    .instance_init = iommufd_backend_init,
> -    .instance_finalize = iommufd_backend_finalize,
> -    .class_size = sizeof(IOMMUFDBackendClass),
> -    .class_init = iommufd_backend_class_init,
> -    .interfaces = (InterfaceInfo[]) {
> -        { TYPE_USER_CREATABLE },
> -        { }
> -    }
> -};
> +void hiod_iommufd_init(HIODIOMMUFD *idev, IOMMUFDBackend *iommufd,
> +                       uint32_t devid)
> +{
> +    idev->iommufd = iommufd;
> +    idev->devid = devid;
> +}

This routine doesn't seem useful. I wonder if we shouldn't introduce
properties. I'm not sure this is useful either.


> -static void register_types(void)
> +static void hiod_iommufd_class_init(ObjectClass *oc, void *data)
>   {
> -    type_register_static(&iommufd_backend_info);
>   }
>   
> -type_init(register_types);
> +static const TypeInfo types[] = {
> +    {
> +        .name = TYPE_IOMMUFD_BACKEND,
> +        .parent = TYPE_OBJECT,
> +        .instance_size = sizeof(IOMMUFDBackend),
> +        .instance_init = iommufd_backend_init,
> +        .instance_finalize = iommufd_backend_finalize,
> +        .class_size = sizeof(IOMMUFDBackendClass),
> +        .class_init = iommufd_backend_class_init,
> +        .interfaces = (InterfaceInfo[]) {
> +            { TYPE_USER_CREATABLE },
> +            { }
> +        }
> +    }, {
> +        .name = TYPE_HIOD_IOMMUFD,
> +        .parent = TYPE_HOST_IOMMU_DEVICE,
> +        .instance_size = sizeof(HIODIOMMUFD),
> +        .class_size = sizeof(HIODIOMMUFDClass),
> +        .class_init = hiod_iommufd_class_init,
> +        .abstract = true,
> +    }
> +};
> +
> +DEFINE_TYPES(types)

Re: [PATCH v2 05/10] vfio: Implement get_host_iommu_info() callback

[Cédric Le Goater]

On 4/8/24 10:12, Zhenzhong Duan wrote:
> Utilize iova_ranges to calculate host IOMMU address width and
> package it in HIOD_LEGACY_INFO for vIOMMU usage.
> 
> HIOD_LEGACY_INFO will be used by both VFIO and VDPA so declare
> it in host_iommu_device.h.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/sysemu/host_iommu_device.h | 10 ++++++++++
>   hw/vfio/container.c                | 24 ++++++++++++++++++++++++
>   2 files changed, 34 insertions(+)
> 
> diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
> index 22ccbe3a5d..beb8be8231 100644
> --- a/include/sysemu/host_iommu_device.h
> +++ b/include/sysemu/host_iommu_device.h
> @@ -16,4 +16,14 @@ struct HostIOMMUDeviceClass {
>       int (*get_host_iommu_info)(HostIOMMUDevice *hiod, void *data, uint32_t len,
>                                  Error **errp);
>   };
> +
> +/*
> + * Define the format of host IOMMU related info that current VFIO
> + * or VDPA can privode to vIOMMU.
> + *
> + * @aw_bits: Host IOMMU address width. 0xff if no limitation.
> + */
> +typedef struct HIOD_LEGACY_INFO {

Please use CamelCase names.

> +    uint8_t aw_bits;
> +} HIOD_LEGACY_INFO;
>   #endif
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 44018ef085..ba0ad4a41b 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -1143,8 +1143,32 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>       vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>   };
>   
> +static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod,
> +                                                void *data, uint32_t len,
> +                                                Error **errp)
> +{
> +    VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev;
> +    /* iova_ranges is a sorted list */
> +    GList *l = g_list_last(vbasedev->bcontainer->iova_ranges);
> +    HIOD_LEGACY_INFO *info = data;
> +
> +    assert(sizeof(HIOD_LEGACY_INFO) <= len);
> +
> +    if (l) {
> +        Range *range = l->data;
> +        info->aw_bits = find_last_bit(&range->upb, BITS_PER_LONG) + 1;

There is a comment in range.h saying:

     /*
      * Do not access members directly, use the functions!

Please introduce a new helper.


Thanks,

C.



> +    } else {
> +        info->aw_bits = 0xff;
> +    }
> +
> +    return 0;
> +}
> +
>   static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>   {
> +    HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
> +
> +    hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
>   };
>   
>   static const TypeInfo types[] = {

Re: [PATCH v2 06/10] backends/iommufd: Introduce helper function iommufd_backend_get_device_info()

[Cédric Le Goater]

On 4/8/24 10:12, Zhenzhong Duan wrote:
> Introduce a helper function iommufd_backend_get_device_info() to get
> host IOMMU related information through iommufd uAPI.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/sysemu/iommufd.h |  4 ++++
>   backends/iommufd.c       | 23 ++++++++++++++++++++++-
>   2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
> index 71c53cbb45..fa1a866237 100644
> --- a/include/sysemu/iommufd.h
> +++ b/include/sysemu/iommufd.h
> @@ -4,6 +4,7 @@
>   #include "qom/object.h"
>   #include "exec/hwaddr.h"
>   #include "exec/cpu-common.h"
> +#include <linux/iommufd.h>
>   #include "sysemu/host_iommu_device.h"
>   
>   #define TYPE_IOMMUFD_BACKEND "iommufd"
> @@ -34,6 +35,9 @@ int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id, hwaddr iova,
>                               ram_addr_t size, void *vaddr, bool readonly);
>   int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
>                                 hwaddr iova, ram_addr_t size);
> +int iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
> +                                    enum iommu_hw_info_type *type,
> +                                    void *data, uint32_t len, Error **errp);
>   
>   #define TYPE_HIOD_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
>   OBJECT_DECLARE_TYPE(HIODIOMMUFD, HIODIOMMUFDClass, HIOD_IOMMUFD)
> diff --git a/backends/iommufd.c b/backends/iommufd.c
> index ef8b3a808b..559affa9ec 100644
> --- a/backends/iommufd.c
> +++ b/backends/iommufd.c
> @@ -20,7 +20,6 @@
>   #include "monitor/monitor.h"
>   #include "trace.h"
>   #include <sys/ioctl.h>
> -#include <linux/iommufd.h>
>   
>   static void iommufd_backend_init(Object *obj)
>   {
> @@ -212,6 +211,28 @@ int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
>       return ret;
>   }
>   
> +int iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
> +                                    enum iommu_hw_info_type *type,
> +                                    void *data, uint32_t len, Error **errp)
> +{
> +    struct iommu_hw_info info = {
> +        .size = sizeof(info),
> +        .dev_id = devid,
> +        .data_len = len,
> +        .data_uptr = (uintptr_t)data,
> +    };
> +    int ret;
> +
> +    ret = ioctl(be->fd, IOMMU_GET_HW_INFO, &info);
> +    if (ret) {
> +        error_setg_errno(errp, errno, "Failed to get hardware info");
> +    } else {
> +        *type = info.out_data_type;

type should not be NULL.
> +    }
> +
> +    return ret;
> +}
> +
>   void hiod_iommufd_init(HIODIOMMUFD *idev, IOMMUFDBackend *iommufd,
>                          uint32_t devid)
>   {

Re: [PATCH v2 07/10] backends/iommufd: Implement get_host_iommu_info() callback

[Cédric Le Goater]

On 4/8/24 10:12, Zhenzhong Duan wrote:
> It calls iommufd_backend_get_device_info() to get host IOMMU
> related information.
> 
> Define a common structure HIOD_IOMMUFD_INFO to describe the info
> returned from kernel. Currently only vtd, but easy to add arm smmu
> when kernel supports.

I think you can merge the previous patch and this one.
  

> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/sysemu/iommufd.h |  7 +++++++
>   backends/iommufd.c       | 17 +++++++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
> index fa1a866237..44ec1335b2 100644
> --- a/include/sysemu/iommufd.h
> +++ b/include/sysemu/iommufd.h

I just noticed that include/sysemu/iommufd.h lacks a header.  Could you fix
that please ?

> @@ -39,6 +39,13 @@ int iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
>                                       enum iommu_hw_info_type *type,
>                                       void *data, uint32_t len, Error **errp);
>   
> +typedef struct HIOD_IOMMUFD_INFO {

Please use CamelCase names.


Thanks,

C.


> +    enum iommu_hw_info_type type;
> +    union {
> +        struct iommu_hw_info_vtd vtd;
> +    } data;
> +} HIOD_IOMMUFD_INFO;
> +
>   #define TYPE_HIOD_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
>   OBJECT_DECLARE_TYPE(HIODIOMMUFD, HIODIOMMUFDClass, HIOD_IOMMUFD)
>   
> diff --git a/backends/iommufd.c b/backends/iommufd.c
> index 559affa9ec..1e9c469e65 100644
> --- a/backends/iommufd.c
> +++ b/backends/iommufd.c
> @@ -240,8 +240,25 @@ void hiod_iommufd_init(HIODIOMMUFD *idev, IOMMUFDBackend *iommufd,
>       idev->devid = devid;
>   }
>   
> +static int hiod_iommufd_get_host_iommu_info(HostIOMMUDevice *hiod,
> +                                            void *data, uint32_t len,
> +                                            Error **errp)
> +{
> +    HIODIOMMUFD *idev = HIOD_IOMMUFD(hiod);
> +    HIOD_IOMMUFD_INFO *info = data;
> +
> +    assert(sizeof(HIOD_IOMMUFD_INFO) <= len);
> +
> +    return iommufd_backend_get_device_info(idev->iommufd, idev->devid,
> +                                           &info->type, &info->data,
> +                                           sizeof(info->data), errp);
> +}
> +
>   static void hiod_iommufd_class_init(ObjectClass *oc, void *data)
>   {
> +    HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc);
> +
> +    hiodc->get_host_iommu_info = hiod_iommufd_get_host_iommu_info;
>   }
>   
>   static const TypeInfo types[] = {

Re: [PATCH v2 08/10] vfio: Create host IOMMU device instance

[Cédric Le Goater]

On 4/8/24 10:12, Zhenzhong Duan wrote:
> Create host IOMMU device instance and initialize it based on backend.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/hw/vfio/vfio-common.h | 1 +
>   hw/vfio/container.c           | 5 +++++
>   hw/vfio/iommufd.c             | 8 ++++++++
>   3 files changed, 14 insertions(+)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index d382b12ec1..4fbba85018 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -126,6 +126,7 @@ typedef struct VFIODevice {
>       OnOffAuto pre_copy_dirty_page_tracking;
>       bool dirty_pages_supported;
>       bool dirty_tracking;
> +    HostIOMMUDevice *hiod;
>       int devid;
>       IOMMUFDBackend *iommufd;
>   } VFIODevice;
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index ba0ad4a41b..fc0c027501 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -915,6 +915,7 @@ static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
>       VFIODevice *vbasedev_iter;
>       VFIOGroup *group;
>       VFIOContainerBase *bcontainer;
> +    HIODLegacyVFIO *hiod_vfio;

s/hiod_vfio/hiod/ please. Same below.


Thanks,

C.



>       int ret;
>   
>       if (groupid < 0) {
> @@ -945,6 +946,9 @@ static int vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
>       vbasedev->bcontainer = bcontainer;
>       QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev, container_next);
>       QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
> +    hiod_vfio = HIOD_LEGACY_VFIO(object_new(TYPE_HIOD_LEGACY_VFIO));
> +    hiod_vfio->vdev = vbasedev;
> +    vbasedev->hiod = HOST_IOMMU_DEVICE(hiod_vfio);
>   
>       return ret;
>   }
> @@ -959,6 +963,7 @@ static void vfio_legacy_detach_device(VFIODevice *vbasedev)
>       trace_vfio_detach_device(vbasedev->name, group->groupid);
>       vfio_put_base_device(vbasedev);
>       vfio_put_group(group);
> +    object_unref(vbasedev->hiod);
>   }
>   
>   static int vfio_legacy_pci_hot_reset(VFIODevice *vbasedev, bool single)
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 115b9f8e7f..b6d058339b 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -308,6 +308,7 @@ static int iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>       VFIOIOMMUFDContainer *container;
>       VFIOAddressSpace *space;
>       struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) };
> +    HIODIOMMUFDVFIO *hiod_vfio;
>       int ret, devfd;
>       uint32_t ioas_id;
>       Error *err = NULL;
> @@ -431,6 +432,12 @@ found_container:
>       QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev, container_next);
>       QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
>   
> +    hiod_vfio = HIOD_IOMMUFD_VFIO(object_new(TYPE_HIOD_IOMMUFD_VFIO));
> +    hiod_iommufd_init(HIOD_IOMMUFD(hiod_vfio), vbasedev->iommufd,
> +                      vbasedev->devid);
> +    hiod_vfio->vdev = vbasedev;
> +    vbasedev->hiod = HOST_IOMMU_DEVICE(hiod_vfio);
> +
>       trace_iommufd_cdev_device_info(vbasedev->name, devfd, vbasedev->num_irqs,
>                                      vbasedev->num_regions, vbasedev->flags);
>       return 0;
> @@ -468,6 +475,7 @@ static void iommufd_cdev_detach(VFIODevice *vbasedev)
>       iommufd_cdev_detach_container(vbasedev, container);
>       iommufd_cdev_container_destroy(container);
>       vfio_put_address_space(space);
> +    object_unref(vbasedev->hiod);
>   
>       iommufd_cdev_unbind_and_disconnect(vbasedev);
>       close(vbasedev->fd);

Re: [PATCH v2 09/10] hw/pci: Introduce pci_device_set/unset_iommu_device()

[Cédric Le Goater]

On 4/8/24 10:12, Zhenzhong Duan wrote:
> From: Yi Liu <yi.l.liu@intel.com>
> 
> This adds pci_device_set/unset_iommu_device() to set/unset
> HostIOMMUDevice for a given PCI device. Caller of set
> should fail if set operation fails.
> 
> Extract out pci_device_get_iommu_bus_devfn() to facilitate

I would separate this change in a prereq patch.


Thanks,

C.


> implementation of pci_device_set/unset_iommu_device().
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/hw/pci/pci.h | 40 ++++++++++++++++++++++-
>   hw/pci/pci.c         | 75 ++++++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 111 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index eaa3fc99d8..4ae7fe6f3f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -3,6 +3,7 @@
>   
>   #include "exec/memory.h"
>   #include "sysemu/dma.h"
> +#include "sysemu/host_iommu_device.h"
>   
>   /* PCI includes legacy ISA access.  */
>   #include "hw/isa/isa.h"
> @@ -383,10 +384,47 @@ typedef struct PCIIOMMUOps {
>        *
>        * @devfn: device and function number
>        */
> -   AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int devfn);
> +    AddressSpace * (*get_address_space)(PCIBus *bus, void *opaque, int devfn);
> +    /**
> +     * @set_iommu_device: attach a HostIOMMUDevice to a vIOMMU
> +     *
> +     * Optional callback, if not implemented in vIOMMU, then vIOMMU can't
> +     * retrieve host information from the associated HostIOMMUDevice.
> +     *
> +     * Return true if HostIOMMUDevice is attached, or else return false
> +     * with errp set.
> +     *
> +     * @bus: the #PCIBus of the PCI device.
> +     *
> +     * @opaque: the data passed to pci_setup_iommu().
> +     *
> +     * @devfn: device and function number of the PCI device.
> +     *
> +     * @dev: the data structure representing host IOMMU device.
> +     *
> +     * @errp: pass an Error out only when return false
> +     *
> +     */
> +    int (*set_iommu_device)(PCIBus *bus, void *opaque, int devfn,
> +                            HostIOMMUDevice *dev, Error **errp);
> +    /**
> +     * @unset_iommu_device: detach a HostIOMMUDevice from a vIOMMU
> +     *
> +     * Optional callback.
> +     *
> +     * @bus: the #PCIBus of the PCI device.
> +     *
> +     * @opaque: the data passed to pci_setup_iommu().
> +     *
> +     * @devfn: device and function number of the PCI device.
> +     */
> +    void (*unset_iommu_device)(PCIBus *bus, void *opaque, int devfn);
>   } PCIIOMMUOps;
>   
>   AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
> +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *hiod,
> +                                Error **errp);
> +void pci_device_unset_iommu_device(PCIDevice *dev);
>   
>   /**
>    * pci_setup_iommu: Initialize specific IOMMU handlers for a PCIBus
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index e7a39cb203..8ece617673 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2648,11 +2648,27 @@ static void pci_device_class_base_init(ObjectClass *klass, void *data)
>       }
>   }
>   
> -AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> +/*
> + * Get IOMMU root bus, aliased bus and devfn of a PCI device
> + *
> + * IOMMU root bus is needed by all call sites to call into iommu_ops.
> + * For call sites which don't need aliased BDF, passing NULL to
> + * aliased_[bus/devfn] is allowed.
> + *
> + * @piommu_bus: return root #PCIBus backed by an IOMMU for the PCI device.
> + *
> + * @aliased_bus: return aliased #PCIBus of the PCI device, optional.
> + *
> + * @aliased_devfn: return aliased devfn of the PCI device, optional.
> + */
> +static void pci_device_get_iommu_bus_devfn(PCIDevice *dev,
> +                                           PCIBus **piommu_bus,
> +                                           PCIBus **aliased_bus,
> +                                           int *aliased_devfn)
>   {
>       PCIBus *bus = pci_get_bus(dev);
>       PCIBus *iommu_bus = bus;
> -    uint8_t devfn = dev->devfn;
> +    int devfn = dev->devfn;
>   
>       while (iommu_bus && !iommu_bus->iommu_ops && iommu_bus->parent_dev) {
>           PCIBus *parent_bus = pci_get_bus(iommu_bus->parent_dev);
> @@ -2693,13 +2709,66 @@ AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>   
>           iommu_bus = parent_bus;
>       }
> -    if (!pci_bus_bypass_iommu(bus) && iommu_bus->iommu_ops) {
> +
> +    assert(0 <= devfn && devfn < PCI_DEVFN_MAX);
> +    assert(iommu_bus);
> +
> +    if (pci_bus_bypass_iommu(bus) || !iommu_bus->iommu_ops) {
> +        iommu_bus = NULL;
> +    }
> +
> +    *piommu_bus = iommu_bus;
> +
> +    if (aliased_bus) {
> +        *aliased_bus = bus;
> +    }
> +
> +    if (aliased_devfn) {
> +        *aliased_devfn = devfn;
> +    }
> +}
> +
> +AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> +{
> +    PCIBus *bus;
> +    PCIBus *iommu_bus;
> +    int devfn;
> +
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, &bus, &devfn);
> +    if (iommu_bus) {
>           return iommu_bus->iommu_ops->get_address_space(bus,
>                                    iommu_bus->iommu_opaque, devfn);
>       }
>       return &address_space_memory;
>   }
>   
> +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *hiod,
> +                                Error **errp)
> +{
> +    PCIBus *iommu_bus;
> +
> +    /* set_iommu_device requires device's direct BDF instead of aliased BDF */
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, NULL, NULL);
> +    if (iommu_bus && iommu_bus->iommu_ops->set_iommu_device) {
> +        return iommu_bus->iommu_ops->set_iommu_device(pci_get_bus(dev),
> +                                                      iommu_bus->iommu_opaque,
> +                                                      dev->devfn, hiod, errp);
> +    }
> +    return 0;
> +}
> +
> +void pci_device_unset_iommu_device(PCIDevice *dev)
> +{
> +    PCIBus *iommu_bus;
> +
> +    pci_device_get_iommu_bus_devfn(dev, &iommu_bus, NULL, NULL);
> +    if (iommu_bus && iommu_bus->iommu_ops->unset_iommu_device) {
> +        return iommu_bus->iommu_ops->unset_iommu_device(pci_get_bus(dev),
> +                                                        iommu_bus->iommu_opaque,
> +                                                        dev->devfn);
> +    }
> +}
> +
>   void pci_setup_iommu(PCIBus *bus, const PCIIOMMUOps *ops, void *opaque)
>   {
>       /*

Re: [PATCH v2 10/10] vfio: Pass HostIOMMUDevice to vIOMMU

[Cédric Le Goater]

On 4/8/24 10:12, Zhenzhong Duan wrote:
> With HostIOMMUDevice passed, vIOMMU can check compatibility with host
> IOMMU, call into IOMMUFD specific methods, etc.
> 
> Originally-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

LGTM, waiting v3.


Thanks,

C.




> ---
>   hw/vfio/pci.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 64780d1b79..224501a86e 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3111,11 +3111,17 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>   
>       vfio_bars_register(vdev);
>   
> -    ret = vfio_add_capabilities(vdev, errp);
> +    ret = pci_device_set_iommu_device(pdev, vbasedev->hiod, errp);
>       if (ret) {
> +        error_prepend(errp, "Failed to set iommu_device: ");
>           goto out_teardown;
>       }
>   
> +    ret = vfio_add_capabilities(vdev, errp);
> +    if (ret) {
> +        goto out_unset_idev;
> +    }
> +
>       if (vdev->vga) {
>           vfio_vga_quirk_setup(vdev);
>       }
> @@ -3132,7 +3138,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>               error_setg(errp,
>                          "cannot support IGD OpRegion feature on hotplugged "
>                          "device");
> -            goto out_teardown;
> +            goto out_unset_idev;
>           }
>   
>           ret = vfio_get_dev_region_info(vbasedev,
> @@ -3141,13 +3147,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>           if (ret) {
>               error_setg_errno(errp, -ret,
>                                "does not support requested IGD OpRegion feature");
> -            goto out_teardown;
> +            goto out_unset_idev;
>           }
>   
>           ret = vfio_pci_igd_opregion_init(vdev, opregion, errp);
>           g_free(opregion);
>           if (ret) {
> -            goto out_teardown;
> +            goto out_unset_idev;
>           }
>       }
>   
> @@ -3233,6 +3239,8 @@ out_deregister:
>       if (vdev->intx.mmap_timer) {
>           timer_free(vdev->intx.mmap_timer);
>       }
> +out_unset_idev:
> +    pci_device_unset_iommu_device(pdev);
>   out_teardown:
>       vfio_teardown_msi(vdev);
>       vfio_bars_exit(vdev);
> @@ -3261,6 +3269,7 @@ static void vfio_instance_finalize(Object *obj)
>   static void vfio_exitfn(PCIDevice *pdev)
>   {
>       VFIOPCIDevice *vdev = VFIO_PCI(pdev);
> +    VFIODevice *vbasedev = &vdev->vbasedev;
>   
>       vfio_unregister_req_notifier(vdev);
>       vfio_unregister_err_notifier(vdev);
> @@ -3275,7 +3284,8 @@ static void vfio_exitfn(PCIDevice *pdev)
>       vfio_teardown_msi(vdev);
>       vfio_pci_disable_rp_atomics(vdev);
>       vfio_bars_exit(vdev);
> -    vfio_migration_exit(&vdev->vbasedev);
> +    vfio_migration_exit(vbasedev);
> +    pci_device_unset_iommu_device(pdev);
>   }
>   
>   static void vfio_pci_reset(DeviceState *dev)

RE: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

[Duan, Zhenzhong]

Hi Cédric,

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>
>On 4/8/24 10:12, Zhenzhong Duan wrote:
>> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy
>> container backend.
>>
>> It includes a link to VFIODevice.
>>
>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/hw/vfio/vfio-common.h | 11 +++++++++++
>>   hw/vfio/container.c           | 11 ++++++++++-
>>   2 files changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>> index b9da6c08ef..f30772f534 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -31,6 +31,7 @@
>>   #endif
>>   #include "sysemu/sysemu.h"
>>   #include "hw/vfio/vfio-container-base.h"
>> +#include "sysemu/host_iommu_device.h"
>>
>>   #define VFIO_MSG_PREFIX "vfio %s: "
>>
>> @@ -147,6 +148,16 @@ typedef struct VFIOGroup {
>>       bool ram_block_discard_allowed;
>>   } VFIOGroup;
>>
>> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy-
>vfio"
>
>I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE.

Will do.

>
>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
>> +
>> +/* Abstraction of VFIO legacy host IOMMU device */
>> +struct HIODLegacyVFIO {
>
>same here

Should I do the same for all the HostIOMMUDevice and HostIOMMUDeviceClass sub-structures?

The reason I used 'HIOD' abbreviation is some function names become extremely long
and exceed 80 characters. E.g.:

@@ -1148,9 +1148,9 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
     vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
 };

-static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod,
-                                                void *data, uint32_t len,
-                                                Error **errp)
+static int host_iommu_device_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod,
+                                                             void *data, uint32_t len,
+                                                             Error **errp)
 {
     VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev;
     /* iova_ranges is a sorted list */
@@ -1173,7 +1173,7 @@ static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
 {
     HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);

-    hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
+    hioc->get_host_iommu_info = host_iommu_device_legacy_vfio_get_host_iommu_info;
 };

I didn't find other way to make it meet the 80 chars limitation. Any suggestions on this?

>
>> +    /*< private >*/
>> +    HostIOMMUDevice parent;
>> +    VFIODevice *vdev;
>
>It seems to me that the back pointer should be on the container instead.
>Looks more correct conceptually.

Yes, that makes sense for legacy VFIO, as iova_ranges, pgsizes etc are all saved in bcontainer.

>
>
>> +};
>> +
>>   typedef struct VFIODMABuf {
>>       QemuDmaBuf buf;
>>       uint32_t pos_x, pos_y, pos_updates;
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index 77bdec276e..44018ef085 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -1143,12 +1143,21 @@ static void
>vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>>       vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>>   };
>>
>> +static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>> +{
>> +};
>
>Is it preferable to introduce routines when they are actually useful.
>Please drop the .class_init definition.

Sure.

Thanks
Zhenzhong

>
>Thanks,
>
>C.
>
>
>> +
>>   static const TypeInfo types[] = {
>>       {
>>           .name = TYPE_VFIO_IOMMU_LEGACY,
>>           .parent = TYPE_VFIO_IOMMU,
>>           .class_init = vfio_iommu_legacy_class_init,
>> -    },
>> +    }, {
>> +        .name = TYPE_HIOD_LEGACY_VFIO,
>> +        .parent = TYPE_HOST_IOMMU_DEVICE,
>> +        .instance_size = sizeof(HIODLegacyVFIO),
>> +        .class_init = hiod_legacy_vfio_class_init,
>> +    }
>>   };
>>
>>   DEFINE_TYPES(types)

RE: [PATCH v2 03/10] backends/iommufd: Introduce abstract HIODIOMMUFD device

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 03/10] backends/iommufd: Introduce abstract
>HIODIOMMUFD device
>
>On 4/8/24 10:12, Zhenzhong Duan wrote:
>> HIODIOMMUFD represents a host IOMMU device under iommufd backend.
>>
>> Currently it includes only public iommufd handle and device id.
>> which could be used to get hw IOMMU information.
>>
>> When nested translation is supported in future, vIOMMU is going
>> to have iommufd related operations like attaching/detaching hwpt,
>> So IOMMUFDDevice interface will be further extended at that time.
>>
>> VFIO and VDPA device have different way of attaching/detaching hwpt.
>> So HIODIOMMUFD is still an abstract class which will be inherited by
>> VFIO and VDPA device.
>>
>> Introduce a helper hiod_iommufd_init() to initialize HIODIOMMUFD
>> device.
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Originally-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/sysemu/iommufd.h | 22 +++++++++++++++++++
>>   backends/iommufd.c       | 47 ++++++++++++++++++++++++++--------------
>>   2 files changed, 53 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>> index 9af27ebd6c..71c53cbb45 100644
>> --- a/include/sysemu/iommufd.h
>> +++ b/include/sysemu/iommufd.h
>> @@ -4,6 +4,7 @@
>>   #include "qom/object.h"
>>   #include "exec/hwaddr.h"
>>   #include "exec/cpu-common.h"
>> +#include "sysemu/host_iommu_device.h"
>>
>>   #define TYPE_IOMMUFD_BACKEND "iommufd"
>>   OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass,
>IOMMUFD_BACKEND)
>> @@ -33,4 +34,25 @@ int
>iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t ioas_id,
>hwaddr iova,
>>                               ram_addr_t size, void *vaddr, bool readonly);
>>   int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t
>ioas_id,
>>                                 hwaddr iova, ram_addr_t size);
>> +
>> +#define TYPE_HIOD_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
>
>Please keep TYPE_HOST_IOMMU_DEVICE

Sure.

>
>> +OBJECT_DECLARE_TYPE(HIODIOMMUFD, HIODIOMMUFDClass,
>HIOD_IOMMUFD)
>> +
>> +struct HIODIOMMUFD {
>> +    /*< private >*/
>> +    HostIOMMUDevice parent;
>> +    void *opaque;
>> +
>> +    /*< public >*/
>> +    IOMMUFDBackend *iommufd;
>> +    uint32_t devid;
>> +};
>> +
>> +struct HIODIOMMUFDClass {
>> +    /*< private >*/
>> +    HostIOMMUDeviceClass parent_class;
>> +};
>
>This new class doesn't seem useful. Do you have plans for handlers ?

Yes, In nesting series https://github.com/yiliu1765/qemu/commits/zhenzhong/iommufd_nesting_rfcv2/
This commit https://github.com/yiliu1765/qemu/commit/581fc900aa296988eaa48abee6d68d3670faf8c9
implement [at|de]tach_hwpt handlers.

So I add an extra layer of abstract HIODIOMMUFDClass.

>
>> +
>> +void hiod_iommufd_init(HIODIOMMUFD *idev, IOMMUFDBackend
>*iommufd,
>> +                       uint32_t devid);
>>   #endif
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index 62a79fa6b0..ef8b3a808b 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -212,23 +212,38 @@ int
>iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
>>       return ret;
>>   }
>>
>> -static const TypeInfo iommufd_backend_info = {
>> -    .name = TYPE_IOMMUFD_BACKEND,
>> -    .parent = TYPE_OBJECT,
>> -    .instance_size = sizeof(IOMMUFDBackend),
>> -    .instance_init = iommufd_backend_init,
>> -    .instance_finalize = iommufd_backend_finalize,
>> -    .class_size = sizeof(IOMMUFDBackendClass),
>> -    .class_init = iommufd_backend_class_init,
>> -    .interfaces = (InterfaceInfo[]) {
>> -        { TYPE_USER_CREATABLE },
>> -        { }
>> -    }
>> -};
>> +void hiod_iommufd_init(HIODIOMMUFD *idev, IOMMUFDBackend
>*iommufd,
>> +                       uint32_t devid)
>> +{
>> +    idev->iommufd = iommufd;
>> +    idev->devid = devid;
>> +}
>
>This routine doesn't seem useful. I wonder if we shouldn't introduce
>properties. I'm not sure this is useful either.

This routine is called in patch8 to initialize iommu, devid and ioas(in future nesting series).
I didn't choose properties as HIODIOMMUFD is not user creatable, property is a bit heavy
here. But I'm fine to use it if you prefer.

Thanks
Zhenzhong

>
>
>> -static void register_types(void)
>> +static void hiod_iommufd_class_init(ObjectClass *oc, void *data)
>>   {
>> -    type_register_static(&iommufd_backend_info);
>>   }
>>
>> -type_init(register_types);
>> +static const TypeInfo types[] = {
>> +    {
>> +        .name = TYPE_IOMMUFD_BACKEND,
>> +        .parent = TYPE_OBJECT,
>> +        .instance_size = sizeof(IOMMUFDBackend),
>> +        .instance_init = iommufd_backend_init,
>> +        .instance_finalize = iommufd_backend_finalize,
>> +        .class_size = sizeof(IOMMUFDBackendClass),
>> +        .class_init = iommufd_backend_class_init,
>> +        .interfaces = (InterfaceInfo[]) {
>> +            { TYPE_USER_CREATABLE },
>> +            { }
>> +        }
>> +    }, {
>> +        .name = TYPE_HIOD_IOMMUFD,
>> +        .parent = TYPE_HOST_IOMMU_DEVICE,
>> +        .instance_size = sizeof(HIODIOMMUFD),
>> +        .class_size = sizeof(HIODIOMMUFDClass),
>> +        .class_init = hiod_iommufd_class_init,
>> +        .abstract = true,
>> +    }
>> +};
>> +
>> +DEFINE_TYPES(types)

RE: [PATCH v2 05/10] vfio: Implement get_host_iommu_info() callback

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 05/10] vfio: Implement get_host_iommu_info()
>callback
>
>On 4/8/24 10:12, Zhenzhong Duan wrote:
>> Utilize iova_ranges to calculate host IOMMU address width and
>> package it in HIOD_LEGACY_INFO for vIOMMU usage.
>>
>> HIOD_LEGACY_INFO will be used by both VFIO and VDPA so declare
>> it in host_iommu_device.h.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/sysemu/host_iommu_device.h | 10 ++++++++++
>>   hw/vfio/container.c                | 24 ++++++++++++++++++++++++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/include/sysemu/host_iommu_device.h
>b/include/sysemu/host_iommu_device.h
>> index 22ccbe3a5d..beb8be8231 100644
>> --- a/include/sysemu/host_iommu_device.h
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -16,4 +16,14 @@ struct HostIOMMUDeviceClass {
>>       int (*get_host_iommu_info)(HostIOMMUDevice *hiod, void *data,
>uint32_t len,
>>                                  Error **errp);
>>   };
>> +
>> +/*
>> + * Define the format of host IOMMU related info that current VFIO
>> + * or VDPA can privode to vIOMMU.
>> + *
>> + * @aw_bits: Host IOMMU address width. 0xff if no limitation.
>> + */
>> +typedef struct HIOD_LEGACY_INFO {
>
>Please use CamelCase names.

Sure.

>
>> +    uint8_t aw_bits;
>> +} HIOD_LEGACY_INFO;
>>   #endif
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index 44018ef085..ba0ad4a41b 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -1143,8 +1143,32 @@ static void
>vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>>       vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>>   };
>>
>> +static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice
>*hiod,
>> +                                                void *data, uint32_t len,
>> +                                                Error **errp)
>> +{
>> +    VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev;
>> +    /* iova_ranges is a sorted list */
>> +    GList *l = g_list_last(vbasedev->bcontainer->iova_ranges);
>> +    HIOD_LEGACY_INFO *info = data;
>> +
>> +    assert(sizeof(HIOD_LEGACY_INFO) <= len);
>> +
>> +    if (l) {
>> +        Range *range = l->data;
>> +        info->aw_bits = find_last_bit(&range->upb, BITS_PER_LONG) + 1;
>
>There is a comment in range.h saying:
>
>     /*
>      * Do not access members directly, use the functions!
>
>Please introduce a new helper.

Sure, thanks for point out.

BRs.
Zhenzhong

>
>
>Thanks,
>
>C.
>
>
>
>> +    } else {
>> +        info->aw_bits = 0xff;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>>   {
>> +    HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
>> +
>> +    hioc->get_host_iommu_info =
>hiod_legacy_vfio_get_host_iommu_info;
>>   };
>>
>>   static const TypeInfo types[] = {

RE: [PATCH v2 06/10] backends/iommufd: Introduce helper function iommufd_backend_get_device_info()

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 06/10] backends/iommufd: Introduce helper
>function iommufd_backend_get_device_info()
>
>On 4/8/24 10:12, Zhenzhong Duan wrote:
>> Introduce a helper function iommufd_backend_get_device_info() to get
>> host IOMMU related information through iommufd uAPI.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/sysemu/iommufd.h |  4 ++++
>>   backends/iommufd.c       | 23 ++++++++++++++++++++++-
>>   2 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>> index 71c53cbb45..fa1a866237 100644
>> --- a/include/sysemu/iommufd.h
>> +++ b/include/sysemu/iommufd.h
>> @@ -4,6 +4,7 @@
>>   #include "qom/object.h"
>>   #include "exec/hwaddr.h"
>>   #include "exec/cpu-common.h"
>> +#include <linux/iommufd.h>
>>   #include "sysemu/host_iommu_device.h"
>>
>>   #define TYPE_IOMMUFD_BACKEND "iommufd"
>> @@ -34,6 +35,9 @@ int iommufd_backend_map_dma(IOMMUFDBackend
>*be, uint32_t ioas_id, hwaddr iova,
>>                               ram_addr_t size, void *vaddr, bool readonly);
>>   int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t
>ioas_id,
>>                                 hwaddr iova, ram_addr_t size);
>> +int iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t
>devid,
>> +                                    enum iommu_hw_info_type *type,
>> +                                    void *data, uint32_t len, Error **errp);
>>
>>   #define TYPE_HIOD_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
>>   OBJECT_DECLARE_TYPE(HIODIOMMUFD, HIODIOMMUFDClass,
>HIOD_IOMMUFD)
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index ef8b3a808b..559affa9ec 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -20,7 +20,6 @@
>>   #include "monitor/monitor.h"
>>   #include "trace.h"
>>   #include <sys/ioctl.h>
>> -#include <linux/iommufd.h>
>>
>>   static void iommufd_backend_init(Object *obj)
>>   {
>> @@ -212,6 +211,28 @@ int
>iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
>>       return ret;
>>   }
>>
>> +int iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t
>devid,
>> +                                    enum iommu_hw_info_type *type,
>> +                                    void *data, uint32_t len, Error **errp)
>> +{
>> +    struct iommu_hw_info info = {
>> +        .size = sizeof(info),
>> +        .dev_id = devid,
>> +        .data_len = len,
>> +        .data_uptr = (uintptr_t)data,
>> +    };
>> +    int ret;
>> +
>> +    ret = ioctl(be->fd, IOMMU_GET_HW_INFO, &info);
>> +    if (ret) {
>> +        error_setg_errno(errp, errno, "Failed to get hardware info");
>> +    } else {
>> +        *type = info.out_data_type;
>
>type should not be NULL.

Yes, will add g_assert(type);

Thanks
Zhenzhong

RE: [PATCH v2 07/10] backends/iommufd: Implement get_host_iommu_info() callback

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 07/10] backends/iommufd: Implement
>get_host_iommu_info() callback
>
>On 4/8/24 10:12, Zhenzhong Duan wrote:
>> It calls iommufd_backend_get_device_info() to get host IOMMU
>> related information.
>>
>> Define a common structure HIOD_IOMMUFD_INFO to describe the info
>> returned from kernel. Currently only vtd, but easy to add arm smmu
>> when kernel supports.
>
>I think you can merge the previous patch and this one.

Sure.

>
>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/sysemu/iommufd.h |  7 +++++++
>>   backends/iommufd.c       | 17 +++++++++++++++++
>>   2 files changed, 24 insertions(+)
>>
>> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
>> index fa1a866237..44ec1335b2 100644
>> --- a/include/sysemu/iommufd.h
>> +++ b/include/sysemu/iommufd.h
>
>I just noticed that include/sysemu/iommufd.h lacks a header.  Could you fix
>that please ?

Sure. Presume you means the copyright header. Fix me if you mean others.

>
>> @@ -39,6 +39,13 @@ int
>iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
>>                                       enum iommu_hw_info_type *type,
>>                                       void *data, uint32_t len, Error **errp);
>>
>> +typedef struct HIOD_IOMMUFD_INFO {
>
>Please use CamelCase names.

Sure.

Thanks
Zhenzhong

>
>
>Thanks,
>
>C.
>
>
>> +    enum iommu_hw_info_type type;
>> +    union {
>> +        struct iommu_hw_info_vtd vtd;
>> +    } data;
>> +} HIOD_IOMMUFD_INFO;
>> +
>>   #define TYPE_HIOD_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
>>   OBJECT_DECLARE_TYPE(HIODIOMMUFD, HIODIOMMUFDClass,
>HIOD_IOMMUFD)
>>
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index 559affa9ec..1e9c469e65 100644
>> --- a/backends/iommufd.c
>> +++ b/backends/iommufd.c
>> @@ -240,8 +240,25 @@ void hiod_iommufd_init(HIODIOMMUFD *idev,
>IOMMUFDBackend *iommufd,
>>       idev->devid = devid;
>>   }
>>
>> +static int hiod_iommufd_get_host_iommu_info(HostIOMMUDevice
>*hiod,
>> +                                            void *data, uint32_t len,
>> +                                            Error **errp)
>> +{
>> +    HIODIOMMUFD *idev = HIOD_IOMMUFD(hiod);
>> +    HIOD_IOMMUFD_INFO *info = data;
>> +
>> +    assert(sizeof(HIOD_IOMMUFD_INFO) <= len);
>> +
>> +    return iommufd_backend_get_device_info(idev->iommufd, idev-
>>devid,
>> +                                           &info->type, &info->data,
>> +                                           sizeof(info->data), errp);
>> +}
>> +
>>   static void hiod_iommufd_class_init(ObjectClass *oc, void *data)
>>   {
>> +    HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc);
>> +
>> +    hiodc->get_host_iommu_info = hiod_iommufd_get_host_iommu_info;
>>   }
>>
>>   static const TypeInfo types[] = {

RE: [PATCH v2 08/10] vfio: Create host IOMMU device instance

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 08/10] vfio: Create host IOMMU device instance
>
>On 4/8/24 10:12, Zhenzhong Duan wrote:
>> Create host IOMMU device instance and initialize it based on backend.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/hw/vfio/vfio-common.h | 1 +
>>   hw/vfio/container.c           | 5 +++++
>>   hw/vfio/iommufd.c             | 8 ++++++++
>>   3 files changed, 14 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>> index d382b12ec1..4fbba85018 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -126,6 +126,7 @@ typedef struct VFIODevice {
>>       OnOffAuto pre_copy_dirty_page_tracking;
>>       bool dirty_pages_supported;
>>       bool dirty_tracking;
>> +    HostIOMMUDevice *hiod;
>>       int devid;
>>       IOMMUFDBackend *iommufd;
>>   } VFIODevice;
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index ba0ad4a41b..fc0c027501 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -915,6 +915,7 @@ static int vfio_legacy_attach_device(const char
>*name, VFIODevice *vbasedev,
>>       VFIODevice *vbasedev_iter;
>>       VFIOGroup *group;
>>       VFIOContainerBase *bcontainer;
>> +    HIODLegacyVFIO *hiod_vfio;
>
>s/hiod_vfio/hiod/ please. Same below.

Will do.

Thanks
Zhenzhong

>
>
>Thanks,
>
>C.
>
>
>
>>       int ret;
>>
>>       if (groupid < 0) {
>> @@ -945,6 +946,9 @@ static int vfio_legacy_attach_device(const char
>*name, VFIODevice *vbasedev,
>>       vbasedev->bcontainer = bcontainer;
>>       QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev,
>container_next);
>>       QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
>> +    hiod_vfio =
>HIOD_LEGACY_VFIO(object_new(TYPE_HIOD_LEGACY_VFIO));
>> +    hiod_vfio->vdev = vbasedev;
>> +    vbasedev->hiod = HOST_IOMMU_DEVICE(hiod_vfio);
>>
>>       return ret;
>>   }
>> @@ -959,6 +963,7 @@ static void vfio_legacy_detach_device(VFIODevice
>*vbasedev)
>>       trace_vfio_detach_device(vbasedev->name, group->groupid);
>>       vfio_put_base_device(vbasedev);
>>       vfio_put_group(group);
>> +    object_unref(vbasedev->hiod);
>>   }
>>
>>   static int vfio_legacy_pci_hot_reset(VFIODevice *vbasedev, bool single)
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index 115b9f8e7f..b6d058339b 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -308,6 +308,7 @@ static int iommufd_cdev_attach(const char *name,
>VFIODevice *vbasedev,
>>       VFIOIOMMUFDContainer *container;
>>       VFIOAddressSpace *space;
>>       struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) };
>> +    HIODIOMMUFDVFIO *hiod_vfio;
>>       int ret, devfd;
>>       uint32_t ioas_id;
>>       Error *err = NULL;
>> @@ -431,6 +432,12 @@ found_container:
>>       QLIST_INSERT_HEAD(&bcontainer->device_list, vbasedev,
>container_next);
>>       QLIST_INSERT_HEAD(&vfio_device_list, vbasedev, global_next);
>>
>> +    hiod_vfio =
>HIOD_IOMMUFD_VFIO(object_new(TYPE_HIOD_IOMMUFD_VFIO));
>> +    hiod_iommufd_init(HIOD_IOMMUFD(hiod_vfio), vbasedev->iommufd,
>> +                      vbasedev->devid);
>> +    hiod_vfio->vdev = vbasedev;
>> +    vbasedev->hiod = HOST_IOMMU_DEVICE(hiod_vfio);
>> +
>>       trace_iommufd_cdev_device_info(vbasedev->name, devfd, vbasedev-
>>num_irqs,
>>                                      vbasedev->num_regions, vbasedev->flags);
>>       return 0;
>> @@ -468,6 +475,7 @@ static void iommufd_cdev_detach(VFIODevice
>*vbasedev)
>>       iommufd_cdev_detach_container(vbasedev, container);
>>       iommufd_cdev_container_destroy(container);
>>       vfio_put_address_space(space);
>> +    object_unref(vbasedev->hiod);
>>
>>       iommufd_cdev_unbind_and_disconnect(vbasedev);
>>       close(vbasedev->fd);

RE: [PATCH v2 09/10] hw/pci: Introduce pci_device_set/unset_iommu_device()

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 09/10] hw/pci: Introduce
>pci_device_set/unset_iommu_device()
>
>On 4/8/24 10:12, Zhenzhong Duan wrote:
>> From: Yi Liu <yi.l.liu@intel.com>
>>
>> This adds pci_device_set/unset_iommu_device() to set/unset
>> HostIOMMUDevice for a given PCI device. Caller of set
>> should fail if set operation fails.
>>
>> Extract out pci_device_get_iommu_bus_devfn() to facilitate
>
>I would separate this change in a prereq patch.

Will do.

Thanks
Zhenzhong

Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

[Cédric Le Goater]

Hello,

On 4/16/24 05:41, Duan, Zhenzhong wrote:
> Hi Cédric,
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>>
>> On 4/8/24 10:12, Zhenzhong Duan wrote:
>>> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy
>>> container backend.
>>>
>>> It includes a link to VFIODevice.
>>>
>>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    include/hw/vfio/vfio-common.h | 11 +++++++++++
>>>    hw/vfio/container.c           | 11 ++++++++++-
>>>    2 files changed, 21 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>> common.h
>>> index b9da6c08ef..f30772f534 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -31,6 +31,7 @@
>>>    #endif
>>>    #include "sysemu/sysemu.h"
>>>    #include "hw/vfio/vfio-container-base.h"
>>> +#include "sysemu/host_iommu_device.h"
>>>
>>>    #define VFIO_MSG_PREFIX "vfio %s: "
>>>
>>> @@ -147,6 +148,16 @@ typedef struct VFIOGroup {
>>>        bool ram_block_discard_allowed;
>>>    } VFIOGroup;
>>>
>>> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy-
>> vfio"
>>
>> I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE.
> 
> Will do.
> 
>>
>>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
>>> +
>>> +/* Abstraction of VFIO legacy host IOMMU device */
>>> +struct HIODLegacyVFIO {
>>
>> same here
> 
> Should I do the same for all the HostIOMMUDevice and HostIOMMUDeviceClass sub-structures?

I would for type names. The main reason is for naming consistency, which is
useful for grep and code analysis.

> 
> The reason I used 'HIOD' abbreviation is some function names become extremely long
> and exceed 80 characters. E.g.:
> 
> @@ -1148,9 +1148,9 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>       vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>   };
> 
> -static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod,
> -                                                void *data, uint32_t len,
> -                                                Error **errp)
> +static int host_iommu_device_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod,
> +                                                             void *data, uint32_t len,
> +                                                             Error **errp)
>   {
>       VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev;
>       /* iova_ranges is a sorted list */
> @@ -1173,7 +1173,7 @@ static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>   {
>       HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
> 
> -    hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
> +    hioc->get_host_iommu_info = host_iommu_device_legacy_vfio_get_host_iommu_info;
>   };
> 
> I didn't find other way to make it meet the 80 chars limitation. Any suggestions on this?

Try :

@@ -1177,7 +1177,8 @@ static void hiod_legacy_vfio_class_init(
  {
      HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
  
-    hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
+    hioc->get_host_iommu_info =
+        host_iommu_device_legacy_vfio_get_host_iommu_info;
  };
  
  static const TypeInfo types[] = {

That said, I agree that 'host_iommu_device_legacy_vfio' routine prefix
could be shortened to 'hiod_legacy_vfio'.


Thanks,

C.



> 
>>
>>> +    /*< private >*/
>>> +    HostIOMMUDevice parent;
>>> +    VFIODevice *vdev;
>>
>> It seems to me that the back pointer should be on the container instead.
>> Looks more correct conceptually.
> 
> Yes, that makes sense for legacy VFIO, as iova_ranges, pgsizes etc are all saved in bcontainer.
> 
>>
>>
>>> +};
>>> +
>>>    typedef struct VFIODMABuf {
>>>        QemuDmaBuf buf;
>>>        uint32_t pos_x, pos_y, pos_updates;
>>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>>> index 77bdec276e..44018ef085 100644
>>> --- a/hw/vfio/container.c
>>> +++ b/hw/vfio/container.c
>>> @@ -1143,12 +1143,21 @@ static void
>> vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>>>        vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>>>    };
>>>
>>> +static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +};
>>
>> Is it preferable to introduce routines when they are actually useful.
>> Please drop the .class_init definition.
> 
> Sure.
> 
> Thanks
> Zhenzhong
> 
>>
>> Thanks,
>>
>> C.
>>
>>
>>> +
>>>    static const TypeInfo types[] = {
>>>        {
>>>            .name = TYPE_VFIO_IOMMU_LEGACY,
>>>            .parent = TYPE_VFIO_IOMMU,
>>>            .class_init = vfio_iommu_legacy_class_init,
>>> -    },
>>> +    }, {
>>> +        .name = TYPE_HIOD_LEGACY_VFIO,
>>> +        .parent = TYPE_HOST_IOMMU_DEVICE,
>>> +        .instance_size = sizeof(HIODLegacyVFIO),
>>> +        .class_init = hiod_legacy_vfio_class_init,
>>> +    }
>>>    };
>>>
>>>    DEFINE_TYPES(types)
> 

RE: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>
>Hello,
>
>On 4/16/24 05:41, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device
>>>
>>> On 4/8/24 10:12, Zhenzhong Duan wrote:
>>>> HIODLegacyVFIO represents a host IOMMU device under VFIO legacy
>>>> container backend.
>>>>
>>>> It includes a link to VFIODevice.
>>>>
>>>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>>>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>>    include/hw/vfio/vfio-common.h | 11 +++++++++++
>>>>    hw/vfio/container.c           | 11 ++++++++++-
>>>>    2 files changed, 21 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>>> common.h
>>>> index b9da6c08ef..f30772f534 100644
>>>> --- a/include/hw/vfio/vfio-common.h
>>>> +++ b/include/hw/vfio/vfio-common.h
>>>> @@ -31,6 +31,7 @@
>>>>    #endif
>>>>    #include "sysemu/sysemu.h"
>>>>    #include "hw/vfio/vfio-container-base.h"
>>>> +#include "sysemu/host_iommu_device.h"
>>>>
>>>>    #define VFIO_MSG_PREFIX "vfio %s: "
>>>>
>>>> @@ -147,6 +148,16 @@ typedef struct VFIOGroup {
>>>>        bool ram_block_discard_allowed;
>>>>    } VFIOGroup;
>>>>
>>>> +#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-
>legacy-
>>> vfio"
>>>
>>> I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE.
>>
>> Will do.
>>
>>>
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
>>>> +
>>>> +/* Abstraction of VFIO legacy host IOMMU device */
>>>> +struct HIODLegacyVFIO {
>>>
>>> same here
>>
>> Should I do the same for all the HostIOMMUDevice and
>HostIOMMUDeviceClass sub-structures?
>
>I would for type names. The main reason is for naming consistency, which is
>useful for grep and code analysis.

Got it.

>
>>
>> The reason I used 'HIOD' abbreviation is some function names become
>extremely long
>> and exceed 80 characters. E.g.:
>>
>> @@ -1148,9 +1148,9 @@ static void
>vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
>>       vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
>>   };
>>
>> -static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice
>*hiod,
>> -                                                void *data, uint32_t len,
>> -                                                Error **errp)
>> +static int
>host_iommu_device_legacy_vfio_get_host_iommu_info(HostIOMMUDevice
>*hiod,
>> +                                                             void *data, uint32_t len,
>> +                                                             Error **errp)
>>   {
>>       VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev;
>>       /* iova_ranges is a sorted list */
>> @@ -1173,7 +1173,7 @@ static void
>hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
>>   {
>>       HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
>>
>> -    hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
>> +    hioc->get_host_iommu_info =
>host_iommu_device_legacy_vfio_get_host_iommu_info;
>>   };
>>
>> I didn't find other way to make it meet the 80 chars limitation. Any
>suggestions on this?
>
>Try :
>
>@@ -1177,7 +1177,8 @@ static void hiod_legacy_vfio_class_init(
>  {
>      HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
>
>-    hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
>+    hioc->get_host_iommu_info =
>+        host_iommu_device_legacy_vfio_get_host_iommu_info;
>  };
>
>  static const TypeInfo types[] = {
>
>That said, I agree that 'host_iommu_device_legacy_vfio' routine prefix
>could be shortened to 'hiod_legacy_vfio'.

Got it.

Thanks
Zhenzhong