[PATCH v1 00/11] Add a host IOMMU device abstraction

[PATCH v1 00/11] Add a host IOMMU device abstraction

[Zhenzhong Duan]

Hi,

Based on Joao's suggestion, the iommufd nesting prerequisite series [1]
is further splitted to host IOMMU device abstract part and vIOMMU
check/sync 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.

PATCH1-3: Introduce HostIOMMUDevice and two sub class
PATCH4: Define HostIOMMUDevice handle in VFIODevice
PATCH5-8: Introdcue host_iommu_device_create callback to allocate and intialize HostIOMMUDevice
PATCH9-10: Introdcue set/unset_iommu_device to pass HostIOMMUDevice to vIOMMU
PATCH11: a helper to get host IOMMU info

Because it's becoming clear on community's suggestion, I'd like to remove
rfc tag from this version.

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

[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:
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 (10):
  Introduce a common abstract struct HostIOMMUDevice
  backends/iommufd: Introduce IOMMUFDDevice
  vfio: Introduce IOMMULegacyDevice
  vfio: Add HostIOMMUDevice handle into VFIODevice
  vfio: Introduce host_iommu_device_create callback
  vfio/container: Implement host_iommu_device_create callback in legacy
    mode
  vfio/iommufd: Implement host_iommu_device_create callback in iommufd
    mode
  vfio/pci: Allocate and initialize HostIOMMUDevice after attachment
  vfio: Pass HostIOMMUDevice to vIOMMU
  backends/iommufd: Introduce helper function iommufd_device_get_info()

 include/hw/pci/pci.h                  | 38 +++++++++++++++-
 include/hw/vfio/vfio-common.h         |  8 ++++
 include/hw/vfio/vfio-container-base.h |  1 +
 include/sysemu/host_iommu_device.h    | 22 ++++++++++
 include/sysemu/iommufd.h              | 19 ++++++++
 backends/iommufd.c                    | 32 +++++++++++++-
 hw/pci/pci.c                          | 62 +++++++++++++++++++++++++--
 hw/vfio/common.c                      |  8 ++++
 hw/vfio/container.c                   |  9 ++++
 hw/vfio/iommufd.c                     | 10 +++++
 hw/vfio/pci.c                         | 24 ++++++++---
 11 files changed, 223 insertions(+), 10 deletions(-)
 create mode 100644 include/sysemu/host_iommu_device.h

-- 
2.34.1

[PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

[Zhenzhong Duan]

HostIOMMUDevice will be inherited by two sub classes,
legacy and iommufd currently.

Introduce a helper function host_iommu_base_device_init to initialize it.

Suggested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/sysemu/host_iommu_device.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 include/sysemu/host_iommu_device.h

diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
new file mode 100644
index 0000000000..fe80ab25fb
--- /dev/null
+++ b/include/sysemu/host_iommu_device.h
@@ -0,0 +1,22 @@
+#ifndef HOST_IOMMU_DEVICE_H
+#define HOST_IOMMU_DEVICE_H
+
+typedef enum HostIOMMUDevice_Type {
+    HID_LEGACY,
+    HID_IOMMUFD,
+    HID_MAX,
+} HostIOMMUDevice_Type;
+
+typedef struct HostIOMMUDevice {
+    HostIOMMUDevice_Type type;
+    size_t size;
+} HostIOMMUDevice;
+
+static inline void host_iommu_base_device_init(HostIOMMUDevice *dev,
+                                               HostIOMMUDevice_Type type,
+                                               size_t size)
+{
+    dev->type = type;
+    dev->size = size;
+}
+#endif
-- 
2.34.1

[PATCH v1 02/11] backends/iommufd: Introduce IOMMUFDDevice

[Zhenzhong Duan]

IOMMUFDDevice represents a device in iommufd and can be used as
a communication interface between devices (i.e., VFIO, VDPA) and
vIOMMU.

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

There will also be some elements in private field in future,
i.e., capability bits for dirty tracking; when nested translation
is supported in future, vIOMMU is going to have more iommufd related
operations like allocate hwpt for a device, attach/detach hwpt, etc.
So IOMMUFDDevice will be further extended with those needs.

IOMMUFDDevice is willingly not a QOM object because we don't want
it to be visible from the user interface.

Introduce a helper iommufd_device_init to initialize IOMMUFDDevice.

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 | 15 +++++++++++++++
 backends/iommufd.c       |  9 +++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
index 9af27ebd6c..d509ff88ef 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,18 @@ 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);
+
+
+/* Abstraction of host IOMMUFD device */
+typedef struct IOMMUFDDevice {
+    /* private: */
+    HostIOMMUDevice base;
+
+    /* public: */
+    IOMMUFDBackend *iommufd;
+    uint32_t devid;
+} IOMMUFDDevice;
+
+void iommufd_device_init(IOMMUFDDevice *idev,
+                         IOMMUFDBackend *iommufd, int devid);
 #endif
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 1ef683c7b0..6d280e4aea 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -231,3 +231,12 @@ static void register_types(void)
 }
 
 type_init(register_types);
+
+void iommufd_device_init(IOMMUFDDevice *idev,
+                         IOMMUFDBackend *iommufd, int devid)
+{
+    host_iommu_base_device_init(&idev->base, HID_IOMMUFD,
+                                sizeof(IOMMUFDDevice));
+    idev->iommufd = iommufd;
+    idev->devid = devid;
+}
-- 
2.34.1

[PATCH v1 03/11] vfio: Introduce IOMMULegacyDevice

[Zhenzhong Duan]

Similar as IOMMUFDDevice, IOMMULegacyDevice represents a device in
legacy mode and can be used as a communication interface between
devices (i.e., VFIO, VDPA) and vIOMMU.

Currently it includes nothing legacy specific, but could be extended
with any wanted info of legacy mode when necessary.

IOMMULegacyDevice is willingly not a QOM object because we don't want
it to be visible from the user interface.

Suggested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/vfio/vfio-common.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 9b7ef7d02b..8bfb9cbe94 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: "
 
@@ -97,6 +98,11 @@ typedef struct VFIOIOMMUFDContainer {
     uint32_t ioas_id;
 } VFIOIOMMUFDContainer;
 
+/* Abstraction of host IOMMU legacy device */
+typedef struct IOMMULegacyDevice {
+    HostIOMMUDevice base;
+} IOMMULegacyDevice;
+
 typedef struct VFIODeviceOps VFIODeviceOps;
 
 typedef struct VFIODevice {
-- 
2.34.1

[PATCH v1 04/11] vfio: Add HostIOMMUDevice handle into VFIODevice

[Zhenzhong Duan]

This handle points to either IOMMULegacyDevice or IOMMUFDDevice variant,
neither both.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/vfio/vfio-common.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 8bfb9cbe94..b6676c9f79 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -130,6 +130,7 @@ typedef struct VFIODevice {
     OnOffAuto pre_copy_dirty_page_tracking;
     bool dirty_pages_supported;
     bool dirty_tracking;
+    HostIOMMUDevice *base_hdev;
     int devid;
     IOMMUFDBackend *iommufd;
 } VFIODevice;
-- 
2.34.1

[PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback

[Zhenzhong Duan]

Introduce host_iommu_device_create callback and a wrapper for it.

This callback is used to allocate a host iommu device instance and
initialize it based on type.

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

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index b6676c9f79..9fefea4b89 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int fd);
 int vfio_attach_device(char *name, VFIODevice *vbasedev,
                        AddressSpace *as, Error **errp);
 void vfio_detach_device(VFIODevice *vbasedev);
+void host_iommu_device_create(VFIODevice *vbasedev);
 
 int vfio_kvm_device_add_fd(int fd, Error **errp);
 int vfio_kvm_device_del_fd(int fd, Error **errp);
diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
index b2813b0c11..dc003f6eb2 100644
--- a/include/hw/vfio/vfio-container-base.h
+++ b/include/hw/vfio/vfio-container-base.h
@@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
     int (*attach_device)(const char *name, VFIODevice *vbasedev,
                          AddressSpace *as, Error **errp);
     void (*detach_device)(VFIODevice *vbasedev);
+    void (*host_iommu_device_create)(VFIODevice *vbasedev);
     /* migration feature */
     int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
                                    bool start);
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 059bfdc07a..41e9031c59 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice *vbasedev)
     }
     vbasedev->bcontainer->ops->detach_device(vbasedev);
 }
+
+void host_iommu_device_create(VFIODevice *vbasedev)
+{
+    const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
+
+    assert(ops->host_iommu_device_create);
+    ops->host_iommu_device_create(vbasedev);
+}
-- 
2.34.1

[PATCH v1 06/11] vfio/container: Implement host_iommu_device_create callback in legacy mode

[Zhenzhong Duan]

This callback will be used to initialize base and public elements in
IOMMULegacyDevice.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/container.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index bd25b9fbad..2e8ff32284 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -1120,6 +1120,14 @@ out_single:
     return ret;
 }
 
+static void vfio_legacy_host_iommu_device_create(VFIODevice *vbasedev)
+{
+    vbasedev->base_hdev = g_malloc0(sizeof(IOMMULegacyDevice));
+
+    host_iommu_base_device_init(vbasedev->base_hdev, HID_LEGACY,
+                                sizeof(IOMMULegacyDevice));
+}
+
 static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
 {
     VFIOIOMMUClass *vioc = VFIO_IOMMU_CLASS(klass);
@@ -1132,6 +1140,7 @@ static void vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
     vioc->set_dirty_page_tracking = vfio_legacy_set_dirty_page_tracking;
     vioc->query_dirty_bitmap = vfio_legacy_query_dirty_bitmap;
     vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
+    vioc->host_iommu_device_create = vfio_legacy_host_iommu_device_create;
 };
 
 static const TypeInfo types[] = {
-- 
2.34.1

[PATCH v1 07/11] vfio/iommufd: Implement host_iommu_device_create callback in iommufd mode

[Zhenzhong Duan]

This callback will be used to initialize base and public elements
in IOMMUFDDevice.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/iommufd.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 9bfddc1360..1c2f5da0d0 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -619,6 +619,15 @@ out_single:
     return ret;
 }
 
+static void vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev)
+{
+    IOMMUFDDevice *idev = g_malloc0(sizeof(IOMMUFDDevice));
+
+    vbasedev->base_hdev = &idev->base;
+
+    iommufd_device_init(idev, vbasedev->iommufd, vbasedev->devid);
+}
+
 static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
 {
     VFIOIOMMUClass *vioc = VFIO_IOMMU_CLASS(klass);
@@ -628,6 +637,7 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data)
     vioc->attach_device = iommufd_cdev_attach;
     vioc->detach_device = iommufd_cdev_detach;
     vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset;
+    vioc->host_iommu_device_create = vfio_cdev_host_iommu_device_create;
 };
 
 static const TypeInfo types[] = {
-- 
2.34.1

[PATCH v1 08/11] vfio/pci: Allocate and initialize HostIOMMUDevice after attachment

[Zhenzhong Duan]

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/vfio/pci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 4fa387f043..6cc7de5d10 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3006,6 +3006,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         goto error;
     }
 
+    /* Allocate and initialize HostIOMMUDevice after attachment succeed */
+    host_iommu_device_create(vbasedev);
+
     vfio_populate_device(vdev, &err);
     if (err) {
         error_propagate(errp, err);
@@ -3244,6 +3247,7 @@ static void vfio_instance_finalize(Object *obj)
 
     vfio_display_finalize(vdev);
     vfio_bars_finalize(vdev);
+    g_free(vdev->vbasedev.base_hdev);
     g_free(vdev->emulated_config_bits);
     g_free(vdev->rom);
     /*
-- 
2.34.1

[PATCH v1 09/11] 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 PCIe 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 | 38 ++++++++++++++++++++++++++-
 hw/pci/pci.c         | 62 +++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 96 insertions(+), 4 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index fa6313aabc..8fe6f746d7 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"
@@ -384,10 +385,45 @@ 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.
+     *
+     */
+    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 *base_dev,
+                                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 76080af580..8078307963 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2672,11 +2672,14 @@ static void pci_device_class_base_init(ObjectClass *klass, void *data)
     }
 }
 
-AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
+static void pci_device_get_iommu_bus_devfn(PCIDevice *dev,
+                                           PCIBus **aliased_bus,
+                                           PCIBus **piommu_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);
@@ -2717,13 +2720,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, &bus, &iommu_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 *base_dev,
+                                Error **errp)
+{
+    PCIBus *iommu_bus;
+
+    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, 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, base_dev,
+                                                      errp);
+    }
+    return 0;
+}
+
+void pci_device_unset_iommu_device(PCIDevice *dev)
+{
+    PCIBus *iommu_bus;
+
+    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, 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 v1 10/11] vfio: Pass HostIOMMUDevice to vIOMMU

[Zhenzhong Duan]

Support both iommufd and legacy backend.

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 6cc7de5d10..ed9f386fde 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3112,11 +3112,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->base_hdev, 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);
     }
@@ -3133,7 +3139,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,
@@ -3142,13 +3148,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;
         }
     }
 
@@ -3234,6 +3240,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);
@@ -3263,6 +3271,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);
@@ -3277,7 +3286,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

[PATCH v1 11/11] backends/iommufd: Introduce helper function iommufd_device_get_info()

[Zhenzhong Duan]

Introduce a helper function iommufd_device_get_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 d509ff88ef..518b97bfed 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"
@@ -48,4 +49,7 @@ typedef struct IOMMUFDDevice {
 
 void iommufd_device_init(IOMMUFDDevice *idev,
                          IOMMUFDBackend *iommufd, int devid);
+int iommufd_device_get_info(IOMMUFDDevice *idev,
+                            enum iommu_hw_info_type *type,
+                            uint32_t len, void *data, Error **errp);
 #endif
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 6d280e4aea..69f3f75ea5 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)
 {
@@ -240,3 +239,25 @@ void iommufd_device_init(IOMMUFDDevice *idev,
     idev->iommufd = iommufd;
     idev->devid = devid;
 }
+
+int iommufd_device_get_info(IOMMUFDDevice *idev,
+                            enum iommu_hw_info_type *type,
+                            uint32_t len, void *data, Error **errp)
+{
+    struct iommu_hw_info info = {
+        .size = sizeof(info),
+        .dev_id = idev->devid,
+        .data_len = len,
+        .data_uptr = (uintptr_t)data,
+    };
+    int ret;
+
+    ret = ioctl(idev->iommufd->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;
+}
-- 
2.34.1

Re: [PATCH v1 09/11] hw/pci: Introduce pci_device_set/unset_iommu_device()

[Michael S. Tsirkin]

On Wed, Feb 28, 2024 at 11:58:58AM +0800, 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 PCIe 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 | 38 ++++++++++++++++++++++++++-
>  hw/pci/pci.c         | 62 +++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 96 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index fa6313aabc..8fe6f746d7 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"
> @@ -384,10 +385,45 @@ 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.
> +     *
> +     */
> +    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;


So I expected someone to implement these new callbacks but I see
no implementation in this patchset.

Is this just infrastructure that will be used later?
A bit hard to judge without a user ...



>  AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
> +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *base_dev,
> +                                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 76080af580..8078307963 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2672,11 +2672,14 @@ static void pci_device_class_base_init(ObjectClass *klass, void *data)
>      }
>  }
>  
> -AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> +static void pci_device_get_iommu_bus_devfn(PCIDevice *dev,
> +                                           PCIBus **aliased_bus,
> +                                           PCIBus **piommu_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);
> @@ -2717,13 +2720,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, &bus, &iommu_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 *base_dev,
> +                                Error **errp)
> +{
> +    PCIBus *iommu_bus;
> +
> +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, 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, base_dev,
> +                                                      errp);
> +    }
> +    return 0;
> +}
> +
> +void pci_device_unset_iommu_device(PCIDevice *dev)
> +{
> +    PCIBus *iommu_bus;
> +
> +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, 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

Re: [PATCH v1 09/11] hw/pci: Introduce pci_device_set/unset_iommu_device()

[Michael S. Tsirkin]

On Tue, Mar 12, 2024 at 12:55:30PM -0400, Michael S. Tsirkin wrote:
> On Wed, Feb 28, 2024 at 11:58:58AM +0800, 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 PCIe 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 | 38 ++++++++++++++++++++++++++-
> >  hw/pci/pci.c         | 62 +++++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 96 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index fa6313aabc..8fe6f746d7 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"
> > @@ -384,10 +385,45 @@ 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.
> > +     *
> > +     */
> > +    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;
> 
> 
> So I expected someone to implement these new callbacks but I see
> no implementation in this patchset.
> 
> Is this just infrastructure that will be used later?
> A bit hard to judge without a user ...
> 

Looked at the second part of the patchset now (dealing with VTD).
Let's continue the discussion there.

> 
> >  AddressSpace *pci_device_iommu_address_space(PCIDevice *dev);
> > +int pci_device_set_iommu_device(PCIDevice *dev, HostIOMMUDevice *base_dev,
> > +                                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 76080af580..8078307963 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -2672,11 +2672,14 @@ static void pci_device_class_base_init(ObjectClass *klass, void *data)
> >      }
> >  }
> >  
> > -AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> > +static void pci_device_get_iommu_bus_devfn(PCIDevice *dev,
> > +                                           PCIBus **aliased_bus,
> > +                                           PCIBus **piommu_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);
> > @@ -2717,13 +2720,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, &bus, &iommu_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 *base_dev,
> > +                                Error **errp)
> > +{
> > +    PCIBus *iommu_bus;
> > +
> > +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, 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, base_dev,
> > +                                                      errp);
> > +    }
> > +    return 0;
> > +}
> > +
> > +void pci_device_unset_iommu_device(PCIDevice *dev)
> > +{
> > +    PCIBus *iommu_bus;
> > +
> > +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, 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

Re: [PATCH v1 04/11] vfio: Add HostIOMMUDevice handle into VFIODevice

[Eric Auger]

On 2/28/24 04:58, Zhenzhong Duan wrote:
> This handle points to either IOMMULegacyDevice or IOMMUFDDevice variant,
> neither both.
I would reword into:
store an handle to the HostIOMMUDevice the VFIODevice is associated with
. Its actual nature depends on the backend in use (VFIO or IOMMUFD).

Thanks

Eric
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  include/hw/vfio/vfio-common.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 8bfb9cbe94..b6676c9f79 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -130,6 +130,7 @@ typedef struct VFIODevice {
>      OnOffAuto pre_copy_dirty_page_tracking;
>      bool dirty_pages_supported;
>      bool dirty_tracking;
> +    HostIOMMUDevice *base_hdev;
>      int devid;
>      IOMMUFDBackend *iommufd;
>  } VFIODevice;

Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback

[Eric Auger]

Hi ZHenzhong,

On 2/28/24 04:58, Zhenzhong Duan wrote:
> Introduce host_iommu_device_create callback and a wrapper for it.
>
> This callback is used to allocate a host iommu device instance and
> initialize it based on type.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  include/hw/vfio/vfio-common.h         | 1 +
>  include/hw/vfio/vfio-container-base.h | 1 +
>  hw/vfio/common.c                      | 8 ++++++++
>  3 files changed, 10 insertions(+)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b6676c9f79..9fefea4b89 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int fd);
>  int vfio_attach_device(char *name, VFIODevice *vbasedev,
>                         AddressSpace *as, Error **errp);
>  void vfio_detach_device(VFIODevice *vbasedev);
> +void host_iommu_device_create(VFIODevice *vbasedev);
>  
>  int vfio_kvm_device_add_fd(int fd, Error **errp);
>  int vfio_kvm_device_del_fd(int fd, Error **errp);
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index b2813b0c11..dc003f6eb2 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>      int (*attach_device)(const char *name, VFIODevice *vbasedev,
>                           AddressSpace *as, Error **errp);
>      void (*detach_device)(VFIODevice *vbasedev);
> +    void (*host_iommu_device_create)(VFIODevice *vbasedev);
>      /* migration feature */
>      int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>                                     bool start);
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 059bfdc07a..41e9031c59 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice *vbasedev)
>      }
>      vbasedev->bcontainer->ops->detach_device(vbasedev);
>  }
> +
> +void host_iommu_device_create(VFIODevice *vbasedev)
> +{
> +    const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
> +
> +    assert(ops->host_iommu_device_create);
at this stage ops actual implementation do not exist yet so this will
break the bisection

Eric
> +    ops->host_iommu_device_create(vbasedev);
> +}

Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback

[Eric Auger]

On 3/18/24 14:52, Eric Auger wrote:
> Hi ZHenzhong,
> 
> On 2/28/24 04:58, Zhenzhong Duan wrote:
>> Introduce host_iommu_device_create callback and a wrapper for it.
>>
>> This callback is used to allocate a host iommu device instance and
>> initialize it based on type.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  include/hw/vfio/vfio-common.h         | 1 +
>>  include/hw/vfio/vfio-container-base.h | 1 +
>>  hw/vfio/common.c                      | 8 ++++++++
>>  3 files changed, 10 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index b6676c9f79..9fefea4b89 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int fd);
>>  int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>                         AddressSpace *as, Error **errp);
>>  void vfio_detach_device(VFIODevice *vbasedev);
>> +void host_iommu_device_create(VFIODevice *vbasedev);
>>  
>>  int vfio_kvm_device_add_fd(int fd, Error **errp);
>>  int vfio_kvm_device_del_fd(int fd, Error **errp);
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
>> index b2813b0c11..dc003f6eb2 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>>      int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>                           AddressSpace *as, Error **errp);
>>      void (*detach_device)(VFIODevice *vbasedev);
>> +    void (*host_iommu_device_create)(VFIODevice *vbasedev);
>>      /* migration feature */
>>      int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>>                                     bool start);
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 059bfdc07a..41e9031c59 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice *vbasedev)
>>      }
>>      vbasedev->bcontainer->ops->detach_device(vbasedev);
>>  }
>> +
>> +void host_iommu_device_create(VFIODevice *vbasedev)
>> +{
>> +    const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
>> +
>> +    assert(ops->host_iommu_device_create);
> at this stage ops actual implementation do not exist yet so this will
> break the bisection

Sorry it is OK at the function only is called in
[PATCH v1 08/11] vfio/pci: Allocate and initialize HostIOMMUDevice after
attachment

Sorry for the noise

Eric
> 
> Eric
>> +    ops->host_iommu_device_create(vbasedev);
>> +}
> 

Re: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

[Eric Auger]

Hi Zhenzhong,
On 2/28/24 04:58, Zhenzhong Duan wrote:
> HostIOMMUDevice will be inherited by two sub classes,
> legacy and iommufd currently.
As this patch introduces the object, you describe what the object is
meant for and used for. Maybe reuse text from the cover letter

Thanks

Eric
>
> Introduce a helper function host_iommu_base_device_init to initialize it.
>
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  include/sysemu/host_iommu_device.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 include/sysemu/host_iommu_device.h
>
> diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
> new file mode 100644
> index 0000000000..fe80ab25fb
> --- /dev/null
> +++ b/include/sysemu/host_iommu_device.h
> @@ -0,0 +1,22 @@
> +#ifndef HOST_IOMMU_DEVICE_H
> +#define HOST_IOMMU_DEVICE_H
> +
> +typedef enum HostIOMMUDevice_Type {
> +    HID_LEGACY,
> +    HID_IOMMUFD,
> +    HID_MAX,
> +} HostIOMMUDevice_Type;
> +
> +typedef struct HostIOMMUDevice {
> +    HostIOMMUDevice_Type type;
> +    size_t size;
> +} HostIOMMUDevice;
> +
> +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev,
> +                                               HostIOMMUDevice_Type type,
> +                                               size_t size)
> +{
> +    dev->type = type;
> +    dev->size = size;
> +}
> +#endif

Re: [PATCH v1 08/11] vfio/pci: Allocate and initialize HostIOMMUDevice after attachment

[Eric Auger]

On 2/28/24 04:58, Zhenzhong Duan wrote:
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/vfio/pci.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 4fa387f043..6cc7de5d10 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3006,6 +3006,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          goto error;
>      }
>  
> +    /* Allocate and initialize HostIOMMUDevice after attachment succeed */
after successful attachment?
> +    host_iommu_device_create(vbasedev);
> +
you shall free on error: as well

Eric
>      vfio_populate_device(vdev, &err);
>      if (err) {
>          error_propagate(errp, err);
> @@ -3244,6 +3247,7 @@ static void vfio_instance_finalize(Object *obj)
>  
>      vfio_display_finalize(vdev);
>      vfio_bars_finalize(vdev);
> +    g_free(vdev->vbasedev.base_hdev);
>      g_free(vdev->emulated_config_bits);
>      g_free(vdev->rom);
>      /*

Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback

[Eric Auger]

On 2/28/24 04:58, Zhenzhong Duan wrote:
> Introduce host_iommu_device_create callback and a wrapper for it.
>
> This callback is used to allocate a host iommu device instance and
> initialize it based on type.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  include/hw/vfio/vfio-common.h         | 1 +
>  include/hw/vfio/vfio-container-base.h | 1 +
>  hw/vfio/common.c                      | 8 ++++++++
>  3 files changed, 10 insertions(+)
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b6676c9f79..9fefea4b89 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int fd);
>  int vfio_attach_device(char *name, VFIODevice *vbasedev,
>                         AddressSpace *as, Error **errp);
>  void vfio_detach_device(VFIODevice *vbasedev);
> +void host_iommu_device_create(VFIODevice *vbasedev);
>  
>  int vfio_kvm_device_add_fd(int fd, Error **errp);
>  int vfio_kvm_device_del_fd(int fd, Error **errp);
> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-container-base.h
> index b2813b0c11..dc003f6eb2 100644
> --- a/include/hw/vfio/vfio-container-base.h
> +++ b/include/hw/vfio/vfio-container-base.h
> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>      int (*attach_device)(const char *name, VFIODevice *vbasedev,
>                           AddressSpace *as, Error **errp);
>      void (*detach_device)(VFIODevice *vbasedev);
> +    void (*host_iommu_device_create)(VFIODevice *vbasedev);
Maybe return an int instead. It is common the allocation can fail and
the deallocation cannot. While at it I would also pass an errp in case
it fails

Eric
>      /* migration feature */
>      int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>                                     bool start);
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 059bfdc07a..41e9031c59 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice *vbasedev)
>      }
>      vbasedev->bcontainer->ops->detach_device(vbasedev);
>  }
> +
> +void host_iommu_device_create(VFIODevice *vbasedev)
> +{
> +    const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
> +
> +    assert(ops->host_iommu_device_create);
> +    ops->host_iommu_device_create(vbasedev);
> +}

Re: [PATCH v1 09/11] hw/pci: Introduce pci_device_set/unset_iommu_device()

[Eric Auger]

Hi Zhenzhong,

On 2/28/24 04:58, 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 PCIe 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 | 38 ++++++++++++++++++++++++++-
>  hw/pci/pci.c         | 62 +++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 96 insertions(+), 4 deletions(-)
>
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index fa6313aabc..8fe6f746d7 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"
> @@ -384,10 +385,45 @@ 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 is missing
> +     *
> +     */
> +    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 *base_dev,
> +                                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 76080af580..8078307963 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2672,11 +2672,14 @@ static void pci_device_class_base_init(ObjectClass *klass, void *data)
>      }
>  }
>  
I would write some comments describing the output params and also
explicitly saying some are optional
> -AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
> +static void pci_device_get_iommu_bus_devfn(PCIDevice *dev,
> +                                           PCIBus **aliased_bus,
> +                                           PCIBus **piommu_bus,

piommu_bus is not an optional parameter. I would put it before 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);
> @@ -2717,13 +2720,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, &bus, &iommu_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 *base_dev,
> +                                Error **errp)
> +{
> +    PCIBus *iommu_bus;
> +
> +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
I would add a comment explaining why you don't care about aliased bus
and devfn
> +    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, base_dev,
> +                                                      errp);
> +    }
> +    return 0;
> +}
> +
> +void pci_device_unset_iommu_device(PCIDevice *dev)
> +{
> +    PCIBus *iommu_bus;
> +
> +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, 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)
>  {
>      /*
Thanks

Eric

Re: [PATCH v1 11/11] backends/iommufd: Introduce helper function iommufd_device_get_info()

[Eric Auger]

Hi Zhenzhong,

On 2/28/24 04:59, Zhenzhong Duan wrote:
> Introduce a helper function iommufd_device_get_info() to get
> host IOMMU related information through iommufd uAPI.
Looks strange to have this patch in this series. I Would rather put it
in your second series alongs with its user.

Eric
>
> 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 d509ff88ef..518b97bfed 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"
> @@ -48,4 +49,7 @@ typedef struct IOMMUFDDevice {
>  
>  void iommufd_device_init(IOMMUFDDevice *idev,
>                           IOMMUFDBackend *iommufd, int devid);
> +int iommufd_device_get_info(IOMMUFDDevice *idev,
> +                            enum iommu_hw_info_type *type,
> +                            uint32_t len, void *data, Error **errp);
>  #endif
> diff --git a/backends/iommufd.c b/backends/iommufd.c
> index 6d280e4aea..69f3f75ea5 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)
>  {
> @@ -240,3 +239,25 @@ void iommufd_device_init(IOMMUFDDevice *idev,
>      idev->iommufd = iommufd;
>      idev->devid = devid;
>  }
> +
> +int iommufd_device_get_info(IOMMUFDDevice *idev,
> +                            enum iommu_hw_info_type *type,
> +                            uint32_t len, void *data, Error **errp)
> +{
> +    struct iommu_hw_info info = {
> +        .size = sizeof(info),
> +        .dev_id = idev->devid,
> +        .data_len = len,
> +        .data_uptr = (uintptr_t)data,
> +    };
> +    int ret;
> +
> +    ret = ioctl(idev->iommufd->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;
> +}

Re: [PATCH v1 00/11] Add a host IOMMU device abstraction

[Eric Auger]

On 2/28/24 04:58, Zhenzhong Duan wrote:
> Hi,
>
> Based on Joao's suggestion, the iommufd nesting prerequisite series [1]
> is further splitted to host IOMMU device abstract part and vIOMMU
> check/sync 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.
>
> PATCH1-3: Introduce HostIOMMUDevice and two sub class
> PATCH4: Define HostIOMMUDevice handle in VFIODevice
> PATCH5-8: Introdcue host_iommu_device_create callback to allocate and intialize HostIOMMUDevice
Introduce, here and below

Eric
> PATCH9-10: Introdcue set/unset_iommu_device to pass HostIOMMUDevice to vIOMMU
> PATCH11: a helper to get host IOMMU info
>
> Because it's becoming clear on community's suggestion, I'd like to remove
> rfc tag from this version.
>
> Qemu code can be found at:
> https://github.com/yiliu1765/qemu/tree/zhenzhong/iommufd_nesting_preq_part1_v1
>
> [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:
> 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 (10):
>   Introduce a common abstract struct HostIOMMUDevice
>   backends/iommufd: Introduce IOMMUFDDevice
>   vfio: Introduce IOMMULegacyDevice
>   vfio: Add HostIOMMUDevice handle into VFIODevice
>   vfio: Introduce host_iommu_device_create callback
>   vfio/container: Implement host_iommu_device_create callback in legacy
>     mode
>   vfio/iommufd: Implement host_iommu_device_create callback in iommufd
>     mode
>   vfio/pci: Allocate and initialize HostIOMMUDevice after attachment
>   vfio: Pass HostIOMMUDevice to vIOMMU
>   backends/iommufd: Introduce helper function iommufd_device_get_info()
>
>  include/hw/pci/pci.h                  | 38 +++++++++++++++-
>  include/hw/vfio/vfio-common.h         |  8 ++++
>  include/hw/vfio/vfio-container-base.h |  1 +
>  include/sysemu/host_iommu_device.h    | 22 ++++++++++
>  include/sysemu/iommufd.h              | 19 ++++++++
>  backends/iommufd.c                    | 32 +++++++++++++-
>  hw/pci/pci.c                          | 62 +++++++++++++++++++++++++--
>  hw/vfio/common.c                      |  8 ++++
>  hw/vfio/container.c                   |  9 ++++
>  hw/vfio/iommufd.c                     | 10 +++++
>  hw/vfio/pci.c                         | 24 ++++++++---
>  11 files changed, 223 insertions(+), 10 deletions(-)
>  create mode 100644 include/sysemu/host_iommu_device.h
>

Re: [PATCH v1 11/11] backends/iommufd: Introduce helper function iommufd_device_get_info()

[Joao Martins]

On 18/03/2024 07:54, Eric Auger wrote:
> Hi Zhenzhong,
> 
> On 2/28/24 04:59, Zhenzhong Duan wrote:
>> Introduce a helper function iommufd_device_get_info() to get
>> host IOMMU related information through iommufd uAPI.
> Looks strange to have this patch in this series. I Would rather put it
> in your second series alongs with its user.
> 

The reason it was here was to use this helper for this patch:

https://lore.kernel.org/qemu-devel/20240212135643.5858-2-joao.m.martins@oracle.com/

Instead of me having my own alternate helper.

Though at the same time, Zhenzhong will also make use of it in his second series.

> Eric
>>
>> 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 d509ff88ef..518b97bfed 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"
>> @@ -48,4 +49,7 @@ typedef struct IOMMUFDDevice {
>>  
>>  void iommufd_device_init(IOMMUFDDevice *idev,
>>                           IOMMUFDBackend *iommufd, int devid);
>> +int iommufd_device_get_info(IOMMUFDDevice *idev,
>> +                            enum iommu_hw_info_type *type,
>> +                            uint32_t len, void *data, Error **errp);
>>  #endif
>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>> index 6d280e4aea..69f3f75ea5 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)
>>  {
>> @@ -240,3 +239,25 @@ void iommufd_device_init(IOMMUFDDevice *idev,
>>      idev->iommufd = iommufd;
>>      idev->devid = devid;
>>  }
>> +
>> +int iommufd_device_get_info(IOMMUFDDevice *idev,
>> +                            enum iommu_hw_info_type *type,
>> +                            uint32_t len, void *data, Error **errp)
>> +{
>> +    struct iommu_hw_info info = {
>> +        .size = sizeof(info),
>> +        .dev_id = idev->devid,
>> +        .data_len = len,
>> +        .data_uptr = (uintptr_t)data,
>> +    };
>> +    int ret;
>> +
>> +    ret = ioctl(idev->iommufd->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;
>> +}
> 

Re: [PATCH v1 11/11] backends/iommufd: Introduce helper function iommufd_device_get_info()

[Eric Auger]

Hi Joao,

On 3/18/24 16:09, Joao Martins wrote:
> On 18/03/2024 07:54, Eric Auger wrote:
>> Hi Zhenzhong,
>>
>> On 2/28/24 04:59, Zhenzhong Duan wrote:
>>> Introduce a helper function iommufd_device_get_info() to get
>>> host IOMMU related information through iommufd uAPI.
>> Looks strange to have this patch in this series. I Would rather put it
>> in your second series alongs with its user.
>>
> The reason it was here was to use this helper for this patch:
>
> https://lore.kernel.org/qemu-devel/20240212135643.5858-2-joao.m.martins@oracle.com/
>
> Instead of me having my own alternate helper.
>
> Though at the same time, Zhenzhong will also make use of it in his second series.
OK I understand now. Maybe with extra comment in the coverletter then

Thanks

Eric
>
>> Eric
>>> 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 d509ff88ef..518b97bfed 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"
>>> @@ -48,4 +49,7 @@ typedef struct IOMMUFDDevice {
>>>  
>>>  void iommufd_device_init(IOMMUFDDevice *idev,
>>>                           IOMMUFDBackend *iommufd, int devid);
>>> +int iommufd_device_get_info(IOMMUFDDevice *idev,
>>> +                            enum iommu_hw_info_type *type,
>>> +                            uint32_t len, void *data, Error **errp);
>>>  #endif
>>> diff --git a/backends/iommufd.c b/backends/iommufd.c
>>> index 6d280e4aea..69f3f75ea5 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)
>>>  {
>>> @@ -240,3 +239,25 @@ void iommufd_device_init(IOMMUFDDevice *idev,
>>>      idev->iommufd = iommufd;
>>>      idev->devid = devid;
>>>  }
>>> +
>>> +int iommufd_device_get_info(IOMMUFDDevice *idev,
>>> +                            enum iommu_hw_info_type *type,
>>> +                            uint32_t len, void *data, Error **errp)
>>> +{
>>> +    struct iommu_hw_info info = {
>>> +        .size = sizeof(info),
>>> +        .dev_id = idev->devid,
>>> +        .data_len = len,
>>> +        .data_uptr = (uintptr_t)data,
>>> +    };
>>> +    int ret;
>>> +
>>> +    ret = ioctl(idev->iommufd->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;
>>> +}

RE: [PATCH v1 04/11] vfio: Add HostIOMMUDevice handle into VFIODevice

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 04/11] vfio: Add HostIOMMUDevice handle into
>VFIODevice
>
>
>
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> This handle points to either IOMMULegacyDevice or IOMMUFDDevice
>variant,
>> neither both.
>I would reword into:
>store an handle to the HostIOMMUDevice the VFIODevice is associated with
>. Its actual nature depends on the backend in use (VFIO or IOMMUFD).

More clear, thanks Eric, will use it.

Zhenzhong

>
>Thanks
>
>Eric
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  include/hw/vfio/vfio-common.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>> index 8bfb9cbe94..b6676c9f79 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -130,6 +130,7 @@ typedef struct VFIODevice {
>>      OnOffAuto pre_copy_dirty_page_tracking;
>>      bool dirty_pages_supported;
>>      bool dirty_tracking;
>> +    HostIOMMUDevice *base_hdev;
>>      int devid;
>>      IOMMUFDBackend *iommufd;
>>  } VFIODevice;

RE: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback

[Duan, Zhenzhong]

Hi Eric,

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create
>callback
>
>Hi ZHenzhong,
>
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> Introduce host_iommu_device_create callback and a wrapper for it.
>>
>> This callback is used to allocate a host iommu device instance and
>> initialize it based on type.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  include/hw/vfio/vfio-common.h         | 1 +
>>  include/hw/vfio/vfio-container-base.h | 1 +
>>  hw/vfio/common.c                      | 8 ++++++++
>>  3 files changed, 10 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>> index b6676c9f79..9fefea4b89 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int
>fd);
>>  int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>                         AddressSpace *as, Error **errp);
>>  void vfio_detach_device(VFIODevice *vbasedev);
>> +void host_iommu_device_create(VFIODevice *vbasedev);
>>
>>  int vfio_kvm_device_add_fd(int fd, Error **errp);
>>  int vfio_kvm_device_del_fd(int fd, Error **errp);
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>> index b2813b0c11..dc003f6eb2 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>>      int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>                           AddressSpace *as, Error **errp);
>>      void (*detach_device)(VFIODevice *vbasedev);
>> +    void (*host_iommu_device_create)(VFIODevice *vbasedev);
>>      /* migration feature */
>>      int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>>                                     bool start);
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 059bfdc07a..41e9031c59 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice
>*vbasedev)
>>      }
>>      vbasedev->bcontainer->ops->detach_device(vbasedev);
>>  }
>> +
>> +void host_iommu_device_create(VFIODevice *vbasedev)
>> +{
>> +    const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
>> +
>> +    assert(ops->host_iommu_device_create);
>at this stage ops actual implementation do not exist yet so this will
>break the bisection

This patch only introcudes host_iommu_device_create but no one call
into it. Patch6-7 implement callback for different backend,
patch8 call host_iommu_device_create(), so I think the order is ok.
Let me know if I missed your points.

Thanks
Zhenzhong

>
>Eric
>> +    ops->host_iommu_device_create(vbasedev);
>> +}

RE: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create
>callback
>
>
>
>On 3/18/24 14:52, Eric Auger wrote:
>> Hi ZHenzhong,
>>
>> On 2/28/24 04:58, Zhenzhong Duan wrote:
>>> Introduce host_iommu_device_create callback and a wrapper for it.
>>>
>>> This callback is used to allocate a host iommu device instance and
>>> initialize it based on type.
>>>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>  include/hw/vfio/vfio-common.h         | 1 +
>>>  include/hw/vfio/vfio-container-base.h | 1 +
>>>  hw/vfio/common.c                      | 8 ++++++++
>>>  3 files changed, 10 insertions(+)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>>> index b6676c9f79..9fefea4b89 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int
>fd);
>>>  int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>>                         AddressSpace *as, Error **errp);
>>>  void vfio_detach_device(VFIODevice *vbasedev);
>>> +void host_iommu_device_create(VFIODevice *vbasedev);
>>>
>>>  int vfio_kvm_device_add_fd(int fd, Error **errp);
>>>  int vfio_kvm_device_del_fd(int fd, Error **errp);
>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>>> index b2813b0c11..dc003f6eb2 100644
>>> --- a/include/hw/vfio/vfio-container-base.h
>>> +++ b/include/hw/vfio/vfio-container-base.h
>>> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>>>      int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>>                           AddressSpace *as, Error **errp);
>>>      void (*detach_device)(VFIODevice *vbasedev);
>>> +    void (*host_iommu_device_create)(VFIODevice *vbasedev);
>>>      /* migration feature */
>>>      int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>>>                                     bool start);
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 059bfdc07a..41e9031c59 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice
>*vbasedev)
>>>      }
>>>      vbasedev->bcontainer->ops->detach_device(vbasedev);
>>>  }
>>> +
>>> +void host_iommu_device_create(VFIODevice *vbasedev)
>>> +{
>>> +    const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
>>> +
>>> +    assert(ops->host_iommu_device_create);
>> at this stage ops actual implementation do not exist yet so this will
>> break the bisection
>
>Sorry it is OK at the function only is called in
>[PATCH v1 08/11] vfio/pci: Allocate and initialize HostIOMMUDevice after
>attachment
>
>Sorry for the noise

Ah, send too quickly. No problem.

Thanks
Zhenzhong

RE: [PATCH v1 08/11] vfio/pci: Allocate and initialize HostIOMMUDevice after attachment

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 08/11] vfio/pci: Allocate and initialize
>HostIOMMUDevice after attachment
>
>
>
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  hw/vfio/pci.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 4fa387f043..6cc7de5d10 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3006,6 +3006,9 @@ static void vfio_realize(PCIDevice *pdev, Error
>**errp)
>>          goto error;
>>      }
>>
>> +    /* Allocate and initialize HostIOMMUDevice after attachment succeed
>*/
>after successful attachment?
>> +    host_iommu_device_create(vbasedev);
>> +
>you shall free on error: as well

I free it in vfio_instance_finalize().
Vfio-pci's design is special, it didn't free all allocated resources in realize's error path,
They are freed in _finalize(). e.g., vdev->emulated_config_bits, vdev->rom, devices and group resources(vfio_detach_device).
I'm following the same way. I'm fine to free it as you suggested something like below:

--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3246,6 +3246,7 @@ out_teardown:
     vfio_teardown_msi(vdev);
     vfio_bars_exit(vdev);
 error:
+    g_free(vdev->vbasedev.base_hdev);
     error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name);
 }

@@ -3288,6 +3289,7 @@ static void vfio_exitfn(PCIDevice *pdev)
     vfio_bars_exit(vdev);
     vfio_migration_exit(vbasedev);
     pci_device_unset_iommu_device(pdev);
+    g_free(vdev->vbasedev.base_hdev);
 }

>
>Eric
>>      vfio_populate_device(vdev, &err);
>>      if (err) {
>>          error_propagate(errp, err);
>> @@ -3244,6 +3247,7 @@ static void vfio_instance_finalize(Object *obj)
>>
>>      vfio_display_finalize(vdev);
>>      vfio_bars_finalize(vdev);
>> +    g_free(vdev->vbasedev.base_hdev);

I free it here.

Thanks
Zhenzhong

>>      g_free(vdev->emulated_config_bits);
>>      g_free(vdev->rom);
>>      /*

RE: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>HostIOMMUDevice
>
>Hi Zhenzhong,
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> HostIOMMUDevice will be inherited by two sub classes,
>> legacy and iommufd currently.
>As this patch introduces the object, you describe what the object is
>meant for and used for. Maybe reuse text from the cover letter

Sure, will do.

Thanks
Zhenzhong

>
>Thanks
>
>Eric
>>
>> Introduce a helper function host_iommu_base_device_init to initialize it.
>>
>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  include/sysemu/host_iommu_device.h | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>  create mode 100644 include/sysemu/host_iommu_device.h
>>
>> diff --git a/include/sysemu/host_iommu_device.h
>b/include/sysemu/host_iommu_device.h
>> new file mode 100644
>> index 0000000000..fe80ab25fb
>> --- /dev/null
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -0,0 +1,22 @@
>> +#ifndef HOST_IOMMU_DEVICE_H
>> +#define HOST_IOMMU_DEVICE_H
>> +
>> +typedef enum HostIOMMUDevice_Type {
>> +    HID_LEGACY,
>> +    HID_IOMMUFD,
>> +    HID_MAX,
>> +} HostIOMMUDevice_Type;
>> +
>> +typedef struct HostIOMMUDevice {
>> +    HostIOMMUDevice_Type type;
>> +    size_t size;
>> +} HostIOMMUDevice;
>> +
>> +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev,
>> +                                               HostIOMMUDevice_Type type,
>> +                                               size_t size)
>> +{
>> +    dev->type = type;
>> +    dev->size = size;
>> +}
>> +#endif

RE: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create
>callback
>
>
>
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> Introduce host_iommu_device_create callback and a wrapper for it.
>>
>> This callback is used to allocate a host iommu device instance and
>> initialize it based on type.
>>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  include/hw/vfio/vfio-common.h         | 1 +
>>  include/hw/vfio/vfio-container-base.h | 1 +
>>  hw/vfio/common.c                      | 8 ++++++++
>>  3 files changed, 10 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>common.h
>> index b6676c9f79..9fefea4b89 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int
>fd);
>>  int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>                         AddressSpace *as, Error **errp);
>>  void vfio_detach_device(VFIODevice *vbasedev);
>> +void host_iommu_device_create(VFIODevice *vbasedev);
>>
>>  int vfio_kvm_device_add_fd(int fd, Error **errp);
>>  int vfio_kvm_device_del_fd(int fd, Error **errp);
>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>container-base.h
>> index b2813b0c11..dc003f6eb2 100644
>> --- a/include/hw/vfio/vfio-container-base.h
>> +++ b/include/hw/vfio/vfio-container-base.h
>> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>>      int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>                           AddressSpace *as, Error **errp);
>>      void (*detach_device)(VFIODevice *vbasedev);
>> +    void (*host_iommu_device_create)(VFIODevice *vbasedev);
>Maybe return an int instead. It is common the allocation can fail and
>the deallocation cannot. While at it I would also pass an errp in case
>it fails

Currently host_iommu_device_create implementation only calls g_malloc0,
so never fails, so I returned void.

I'm fine to return an int, will be like below, take iommufd for example:

--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -651,7 +651,7 @@ static IOMMUFDDeviceOps vfio_iommufd_device_ops = {
     .detach_hwpt = vfio_iommufd_device_detach_hwpt,
 };

-static void vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev)
+static int vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev, Error **errp)
 {
     IOMMUFDDevice *idev = g_malloc0(sizeof(IOMMUFDDevice));
     VFIOIOMMUFDContainer *container = container_of(vbasedev->bcontainer,
@@ -661,6 +661,8 @@ static void vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev)

     iommufd_device_init(idev, vbasedev->iommufd, vbasedev->devid,
                         container->ioas_id, vbasedev, &vfio_iommufd_device_ops);
+
+    return 0;
 }

Thanks
Zhenzhong

>
>Eric
>>      /* migration feature */
>>      int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>>                                     bool start);
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 059bfdc07a..41e9031c59 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice
>*vbasedev)
>>      }
>>      vbasedev->bcontainer->ops->detach_device(vbasedev);
>>  }
>> +
>> +void host_iommu_device_create(VFIODevice *vbasedev)
>> +{
>> +    const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
>> +
>> +    assert(ops->host_iommu_device_create);
>> +    ops->host_iommu_device_create(vbasedev);
>> +}

RE: [PATCH v1 09/11] hw/pci: Introduce pci_device_set/unset_iommu_device()

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 09/11] hw/pci: Introduce
>pci_device_set/unset_iommu_device()
>
>Hi Zhenzhong,
>
>On 2/28/24 04:58, 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 PCIe 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 | 38 ++++++++++++++++++++++++++-
>>  hw/pci/pci.c         | 62
>+++++++++++++++++++++++++++++++++++++++++---
>>  2 files changed, 96 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
>> index fa6313aabc..8fe6f746d7 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"
>> @@ -384,10 +385,45 @@ 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 is missing

Will add.

>> +     *
>> +     */
>> +    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
>*base_dev,
>> +                                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 76080af580..8078307963 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -2672,11 +2672,14 @@ static void
>pci_device_class_base_init(ObjectClass *klass, void *data)
>>      }
>>  }
>>
>I would write some comments describing the output params and also
>explicitly saying some are optional

Sure.

>> -AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
>> +static void pci_device_get_iommu_bus_devfn(PCIDevice *dev,
>> +                                           PCIBus **aliased_bus,
>> +                                           PCIBus **piommu_bus,
>
>piommu_bus is not an optional parameter. I would put it before aliased_bus.

Good suggestion, will do.

>
>> +                                           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);
>> @@ -2717,13 +2720,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, &bus, &iommu_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
>*base_dev,
>> +                                Error **errp)
>> +{
>> +    PCIBus *iommu_bus;
>> +
>> +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, NULL);
>I would add a comment explaining why you don't care about aliased bus
>and devfn

Sure.

Thanks
Zhenzhong

>> +    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, base_dev,
>> +                                                      errp);
>> +    }
>> +    return 0;
>> +}
>> +
>> +void pci_device_unset_iommu_device(PCIDevice *dev)
>> +{
>> +    PCIBus *iommu_bus;
>> +
>> +    pci_device_get_iommu_bus_devfn(dev, NULL, &iommu_bus, 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)
>>  {
>>      /*
>Thanks
>
>Eric

RE: [PATCH v1 11/11] backends/iommufd: Introduce helper function iommufd_device_get_info()

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 11/11] backends/iommufd: Introduce helper
>function iommufd_device_get_info()
>
>Hi Joao,
>
>On 3/18/24 16:09, Joao Martins wrote:
>> On 18/03/2024 07:54, Eric Auger wrote:
>>> Hi Zhenzhong,
>>>
>>> On 2/28/24 04:59, Zhenzhong Duan wrote:
>>>> Introduce a helper function iommufd_device_get_info() to get
>>>> host IOMMU related information through iommufd uAPI.
>>> Looks strange to have this patch in this series. I Would rather put it
>>> in your second series alongs with its user.
>>>
>> The reason it was here was to use this helper for this patch:
>>
>> https://lore.kernel.org/qemu-devel/20240212135643.5858-2-
>joao.m.martins@oracle.com/
>>
>> Instead of me having my own alternate helper.
>>
>> Though at the same time, Zhenzhong will also make use of it in his second
>series.
>OK I understand now. Maybe with extra comment in the coverletter then

Will add.

Thanks
Zhenzhong

RE: [PATCH v1 00/11] Add a host IOMMU device abstraction

[Duan, Zhenzhong]

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v1 00/11] Add a host IOMMU device abstraction
>
>
>
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> Hi,
>>
>> Based on Joao's suggestion, the iommufd nesting prerequisite series [1]
>> is further splitted to host IOMMU device abstract part and vIOMMU
>> check/sync 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.
>>
>> PATCH1-3: Introduce HostIOMMUDevice and two sub class
>> PATCH4: Define HostIOMMUDevice handle in VFIODevice
>> PATCH5-8: Introdcue host_iommu_device_create callback to allocate and
>intialize HostIOMMUDevice
>Introduce, here and below

Good catch, will fix.

Thanks
Zhenzhong

>
>Eric
>> PATCH9-10: Introdcue set/unset_iommu_device to pass
>HostIOMMUDevice to vIOMMU
>> PATCH11: a helper to get host IOMMU info
>>
>> Because it's becoming clear on community's suggestion, I'd like to remove
>> rfc tag from this version.
>>
>> Qemu code can be found at:
>>
>https://github.com/yiliu1765/qemu/tree/zhenzhong/iommufd_nesting_pre
>q_part1_v1
>>
>> [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:
>> 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 (10):
>>   Introduce a common abstract struct HostIOMMUDevice
>>   backends/iommufd: Introduce IOMMUFDDevice
>>   vfio: Introduce IOMMULegacyDevice
>>   vfio: Add HostIOMMUDevice handle into VFIODevice
>>   vfio: Introduce host_iommu_device_create callback
>>   vfio/container: Implement host_iommu_device_create callback in legacy
>>     mode
>>   vfio/iommufd: Implement host_iommu_device_create callback in
>iommufd
>>     mode
>>   vfio/pci: Allocate and initialize HostIOMMUDevice after attachment
>>   vfio: Pass HostIOMMUDevice to vIOMMU
>>   backends/iommufd: Introduce helper function iommufd_device_get_info()
>>
>>  include/hw/pci/pci.h                  | 38 +++++++++++++++-
>>  include/hw/vfio/vfio-common.h         |  8 ++++
>>  include/hw/vfio/vfio-container-base.h |  1 +
>>  include/sysemu/host_iommu_device.h    | 22 ++++++++++
>>  include/sysemu/iommufd.h              | 19 ++++++++
>>  backends/iommufd.c                    | 32 +++++++++++++-
>>  hw/pci/pci.c                          | 62 +++++++++++++++++++++++++--
>>  hw/vfio/common.c                      |  8 ++++
>>  hw/vfio/container.c                   |  9 ++++
>>  hw/vfio/iommufd.c                     | 10 +++++
>>  hw/vfio/pci.c                         | 24 ++++++++---
>>  11 files changed, 223 insertions(+), 10 deletions(-)
>>  create mode 100644 include/sysemu/host_iommu_device.h
>>

Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create callback

[Eric Auger]

Hi Zhenzhong,

On 3/19/24 06:44, Duan, Zhenzhong wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: Re: [PATCH v1 05/11] vfio: Introduce host_iommu_device_create
>> callback
>>
>>
>>
>> On 2/28/24 04:58, Zhenzhong Duan wrote:
>>> Introduce host_iommu_device_create callback and a wrapper for it.
>>>
>>> This callback is used to allocate a host iommu device instance and
>>> initialize it based on type.
>>>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>  include/hw/vfio/vfio-common.h         | 1 +
>>>  include/hw/vfio/vfio-container-base.h | 1 +
>>>  hw/vfio/common.c                      | 8 ++++++++
>>>  3 files changed, 10 insertions(+)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>> common.h
>>> index b6676c9f79..9fefea4b89 100644
>>> --- a/include/hw/vfio/vfio-common.h
>>> +++ b/include/hw/vfio/vfio-common.h
>>> @@ -208,6 +208,7 @@ struct vfio_device_info *vfio_get_device_info(int
>> fd);
>>>  int vfio_attach_device(char *name, VFIODevice *vbasedev,
>>>                         AddressSpace *as, Error **errp);
>>>  void vfio_detach_device(VFIODevice *vbasedev);
>>> +void host_iommu_device_create(VFIODevice *vbasedev);
>>>
>>>  int vfio_kvm_device_add_fd(int fd, Error **errp);
>>>  int vfio_kvm_device_del_fd(int fd, Error **errp);
>>> diff --git a/include/hw/vfio/vfio-container-base.h b/include/hw/vfio/vfio-
>> container-base.h
>>> index b2813b0c11..dc003f6eb2 100644
>>> --- a/include/hw/vfio/vfio-container-base.h
>>> +++ b/include/hw/vfio/vfio-container-base.h
>>> @@ -120,6 +120,7 @@ struct VFIOIOMMUClass {
>>>      int (*attach_device)(const char *name, VFIODevice *vbasedev,
>>>                           AddressSpace *as, Error **errp);
>>>      void (*detach_device)(VFIODevice *vbasedev);
>>> +    void (*host_iommu_device_create)(VFIODevice *vbasedev);
>> Maybe return an int instead. It is common the allocation can fail and
>> the deallocation cannot. While at it I would also pass an errp in case
>> it fails
> Currently host_iommu_device_create implementation only calls g_malloc0,
> so never fails, so I returned void.
>
> I'm fine to return an int, will be like below, take iommufd for example:
>
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -651,7 +651,7 @@ static IOMMUFDDeviceOps vfio_iommufd_device_ops = {
>      .detach_hwpt = vfio_iommufd_device_detach_hwpt,
>  };
>
> -static void vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev)
> +static int vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev, Error **errp)
>  {
>      IOMMUFDDevice *idev = g_malloc0(sizeof(IOMMUFDDevice));
>      VFIOIOMMUFDContainer *container = container_of(vbasedev->bcontainer,
> @@ -661,6 +661,8 @@ static void vfio_cdev_host_iommu_device_create(VFIODevice *vbasedev)
>
>      iommufd_device_init(idev, vbasedev->iommufd, vbasedev->devid,
>                          container->ioas_id, vbasedev, &vfio_iommufd_device_ops);
> +
> +    return 0;
>  }
I think the rationale behind introducing an error handle is to propagate
it down to the callbacks. This is were I anticipated errors to occur.
But if you don't foresee any maybe we can just wait and postpone this
addition.

Thanks

Eric
>
> Thanks
> Zhenzhong


>
>> Eric
>>>      /* migration feature */
>>>      int (*set_dirty_page_tracking)(const VFIOContainerBase *bcontainer,
>>>                                     bool start);
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 059bfdc07a..41e9031c59 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -1521,3 +1521,11 @@ void vfio_detach_device(VFIODevice
>> *vbasedev)
>>>      }
>>>      vbasedev->bcontainer->ops->detach_device(vbasedev);
>>>  }
>>> +
>>> +void host_iommu_device_create(VFIODevice *vbasedev)
>>> +{
>>> +    const VFIOIOMMUClass *ops = vbasedev->bcontainer->ops;
>>> +
>>> +    assert(ops->host_iommu_device_create);
>>> +    ops->host_iommu_device_create(vbasedev);
>>> +}

Re: [PATCH v1 08/11] vfio/pci: Allocate and initialize HostIOMMUDevice after attachment

[Eric Auger]

On 3/19/24 04:46, Duan, Zhenzhong wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: Re: [PATCH v1 08/11] vfio/pci: Allocate and initialize
>> HostIOMMUDevice after attachment
>>
>>
>>
>> On 2/28/24 04:58, Zhenzhong Duan wrote:
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>  hw/vfio/pci.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index 4fa387f043..6cc7de5d10 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -3006,6 +3006,9 @@ static void vfio_realize(PCIDevice *pdev, Error
>> **errp)
>>>          goto error;
>>>      }
>>>
>>> +    /* Allocate and initialize HostIOMMUDevice after attachment succeed
>> */
>> after successful attachment?
>>> +    host_iommu_device_create(vbasedev);
>>> +
>> you shall free on error: as well
> I free it in vfio_instance_finalize().
> Vfio-pci's design is special, it didn't free all allocated resources in realize's error path,
> They are freed in _finalize(). e.g., vdev->emulated_config_bits, vdev->rom, devices and group resources(vfio_detach_device).
> I'm following the same way. I'm fine to free it as you suggested something like below:

Oh yes I remember now. I had exactly the same question some months ago
:-/ So that's fine then

Eric
>
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3246,6 +3246,7 @@ out_teardown:
>      vfio_teardown_msi(vdev);
>      vfio_bars_exit(vdev);
>  error:
> +    g_free(vdev->vbasedev.base_hdev);
>      error_prepend(errp, VFIO_MSG_PREFIX, vbasedev->name);
>  }
>
> @@ -3288,6 +3289,7 @@ static void vfio_exitfn(PCIDevice *pdev)
>      vfio_bars_exit(vdev);
>      vfio_migration_exit(vbasedev);
>      pci_device_unset_iommu_device(pdev);
> +    g_free(vdev->vbasedev.base_hdev);
>  }
>
>> Eric
>>>      vfio_populate_device(vdev, &err);
>>>      if (err) {
>>>          error_propagate(errp, err);
>>> @@ -3244,6 +3247,7 @@ static void vfio_instance_finalize(Object *obj)
>>>
>>>      vfio_display_finalize(vdev);
>>>      vfio_bars_finalize(vdev);
>>> +    g_free(vdev->vbasedev.base_hdev);
> I free it here.
>
> Thanks
> Zhenzhong
>
>>>      g_free(vdev->emulated_config_bits);
>>>      g_free(vdev->rom);
>>>      /*

Re: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

[Cédric Le Goater]

Hello Zhenzhong,

On 2/28/24 04:58, Zhenzhong Duan wrote:
> HostIOMMUDevice will be inherited by two sub classes,
> legacy and iommufd currently.
> 
> Introduce a helper function host_iommu_base_device_init to initialize it.
> 
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/sysemu/host_iommu_device.h | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>   create mode 100644 include/sysemu/host_iommu_device.h
> 
> diff --git a/include/sysemu/host_iommu_device.h b/include/sysemu/host_iommu_device.h
> new file mode 100644
> index 0000000000..fe80ab25fb
> --- /dev/null
> +++ b/include/sysemu/host_iommu_device.h
> @@ -0,0 +1,22 @@
> +#ifndef HOST_IOMMU_DEVICE_H
> +#define HOST_IOMMU_DEVICE_H
> +
> +typedef enum HostIOMMUDevice_Type {
> +    HID_LEGACY,
> +    HID_IOMMUFD,
> +    HID_MAX,
> +} HostIOMMUDevice_Type;
> +
> +typedef struct HostIOMMUDevice {
> +    HostIOMMUDevice_Type type;

A type field is not a good sign and that's where QOM is useful.

Is vtd_check_hdev() the only use of this field ? If so, can we simplify
with a QOM interface in any way ?

Thanks,

C.

  


> +    size_t size;
> +} HostIOMMUDevice;
> +
> +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev,
> +                                               HostIOMMUDevice_Type type,
> +                                               size_t size)
> +{
> +    dev->type = type;
> +    dev->size = size;
> +}
> +#endif

RE: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

[Duan, Zhenzhong]

Hi Cédric,

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Sent: Tuesday, March 19, 2024 4:17 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>devel@nongnu.org
>Cc: alex.williamson@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>; Sun, Yi Y
><yi.y.sun@intel.com>; Peng, Chao P <chao.p.peng@intel.com>
>Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>HostIOMMUDevice
>
>Hello Zhenzhong,
>
>On 2/28/24 04:58, Zhenzhong Duan wrote:
>> HostIOMMUDevice will be inherited by two sub classes,
>> legacy and iommufd currently.
>>
>> Introduce a helper function host_iommu_base_device_init to initialize it.
>>
>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/sysemu/host_iommu_device.h | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>   create mode 100644 include/sysemu/host_iommu_device.h
>>
>> diff --git a/include/sysemu/host_iommu_device.h
>b/include/sysemu/host_iommu_device.h
>> new file mode 100644
>> index 0000000000..fe80ab25fb
>> --- /dev/null
>> +++ b/include/sysemu/host_iommu_device.h
>> @@ -0,0 +1,22 @@
>> +#ifndef HOST_IOMMU_DEVICE_H
>> +#define HOST_IOMMU_DEVICE_H
>> +
>> +typedef enum HostIOMMUDevice_Type {
>> +    HID_LEGACY,
>> +    HID_IOMMUFD,
>> +    HID_MAX,
>> +} HostIOMMUDevice_Type;
>> +
>> +typedef struct HostIOMMUDevice {
>> +    HostIOMMUDevice_Type type;
>
>A type field is not a good sign and that's where QOM is useful.

Yes, agree.
I didn't choose QOM because in iommufd-cdev series, VFIOContainer chooses not using QOM model.
See the discussion: https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/
I thought HostIOMMUDevice need to follow same rule.

But after further digging into this, I think it may be ok to use QOM model as long as we don't expose
HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE interface. Your thoughts?

>
>Is vtd_check_hdev() the only use of this field ?

Currently yes. virtio-iommu may have similar usage.

> If so, can we simplify with a QOM interface in any way ?

QOM interface is a set of callbacks, guess you mean QOM class,
saying HostIOMMUDevice class, IOMMULegacyDevice class and IOMMUFDDevice class?

Thanks
Zhenzhong

>
>Thanks,
>
>C.
>
>
>
>
>> +    size_t size;
>> +} HostIOMMUDevice;
>> +
>> +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev,
>> +                                               HostIOMMUDevice_Type type,
>> +                                               size_t size)
>> +{
>> +    dev->type = type;
>> +    dev->size = size;
>> +}
>> +#endif

Re: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

[Cédric Le Goater]

Hello Zhenzhong,

On 3/19/24 12:58, Duan, Zhenzhong wrote:
> Hi Cédric,
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Sent: Tuesday, March 19, 2024 4:17 PM
>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>> devel@nongnu.org
>> Cc: alex.williamson@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>; Sun, Yi Y
>> <yi.y.sun@intel.com>; Peng, Chao P <chao.p.peng@intel.com>
>> Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>> HostIOMMUDevice
>>
>> Hello Zhenzhong,
>>
>> On 2/28/24 04:58, Zhenzhong Duan wrote:
>>> HostIOMMUDevice will be inherited by two sub classes,
>>> legacy and iommufd currently.
>>>
>>> Introduce a helper function host_iommu_base_device_init to initialize it.
>>>
>>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>    include/sysemu/host_iommu_device.h | 22 ++++++++++++++++++++++
>>>    1 file changed, 22 insertions(+)
>>>    create mode 100644 include/sysemu/host_iommu_device.h
>>>
>>> diff --git a/include/sysemu/host_iommu_device.h
>> b/include/sysemu/host_iommu_device.h
>>> new file mode 100644
>>> index 0000000000..fe80ab25fb
>>> --- /dev/null
>>> +++ b/include/sysemu/host_iommu_device.h
>>> @@ -0,0 +1,22 @@
>>> +#ifndef HOST_IOMMU_DEVICE_H
>>> +#define HOST_IOMMU_DEVICE_H
>>> +
>>> +typedef enum HostIOMMUDevice_Type {
>>> +    HID_LEGACY,
>>> +    HID_IOMMUFD,
>>> +    HID_MAX,
>>> +} HostIOMMUDevice_Type;
>>> +
>>> +typedef struct HostIOMMUDevice {
>>> +    HostIOMMUDevice_Type type;
>>
>> A type field is not a good sign and that's where QOM is useful.
> 
> Yes, agree.
> I didn't choose QOM because in iommufd-cdev series, VFIOContainer chooses not using QOM model.
> See the discussion: https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/
> I thought HostIOMMUDevice need to follow same rule.
> 
> But after further digging into this, I think it may be ok to use QOM model as long as we don't expose
> HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE interface. Your thoughts?

yes. Can we change a bit this series to use QOM ? something like :

     typedef struct HostIOMMUDevice {
         Object parent;
     } HostIOMMUDevice;
     
     #define TYPE_HOST_IOMMU "host.iommu"
     OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUClass, HOST_IOMMU)
     
     struct HostIOMMUClass {
         ObjectClass parent_class;
     
         int (*get_type)(HostIOMMUDevice *hiod, uint64_t *type, Error **errp);
         int (*get_cap)(HostIOMMUDevice *hiod, uint64_t *cap, Error **errp);
     };

Inherited objects would be TYPE_HOST_IOMMU_IOMMUFD and TYPE_HOST_IOMMU_LEGACY.
Each class implementing the handlers or not (legacy mode).

The class handlers are introduced for the intel-iommu helper vtd_check_hdev()
in order to avoid using iommufd routines directly. HostIOMMUDevice is supposed
to abstract the Host IOMMU device, so we need to abstract also all the
interfaces to this object.

The .host_iommu_device_create() handler could be merged in .attach_device()
possibly. Anyhow, please use now object_new() and object_unref() instead.
host_iommu_base_device_init() is useless IMHO.

> 
>>
>> Is vtd_check_hdev() the only use of this field ?
> 
> Currently yes. virtio-iommu may have similar usage.
> 
>> If so, can we simplify with a QOM interface in any way ?
> 
> QOM interface is a set of callbacks, guess you mean QOM class,
> saying HostIOMMUDevice class, IOMMULegacyDevice class and IOMMUFDDevice class?

See above proposal. it should work fine.

Also, I think it is better to use a IOMMUFDBackend* parameter for
iommufd_device_get_info() to be consistent with the other routines.

Then It would interesting to see how this applies to Eric's series.

Thanks,

C.

RE: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

[Duan, Zhenzhong]

Hi Cédric,

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>HostIOMMUDevice
>
>Hello Zhenzhong,
>
>On 3/19/24 12:58, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Sent: Tuesday, March 19, 2024 4:17 PM
>>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>>> devel@nongnu.org
>>> Cc: alex.williamson@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>; Sun, Yi Y
>>> <yi.y.sun@intel.com>; Peng, Chao P <chao.p.peng@intel.com>
>>> Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>>> HostIOMMUDevice
>>>
>>> Hello Zhenzhong,
>>>
>>> On 2/28/24 04:58, Zhenzhong Duan wrote:
>>>> HostIOMMUDevice will be inherited by two sub classes,
>>>> legacy and iommufd currently.
>>>>
>>>> Introduce a helper function host_iommu_base_device_init to initialize it.
>>>>
>>>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>>    include/sysemu/host_iommu_device.h | 22
>++++++++++++++++++++++
>>>>    1 file changed, 22 insertions(+)
>>>>    create mode 100644 include/sysemu/host_iommu_device.h
>>>>
>>>> diff --git a/include/sysemu/host_iommu_device.h
>>> b/include/sysemu/host_iommu_device.h
>>>> new file mode 100644
>>>> index 0000000000..fe80ab25fb
>>>> --- /dev/null
>>>> +++ b/include/sysemu/host_iommu_device.h
>>>> @@ -0,0 +1,22 @@
>>>> +#ifndef HOST_IOMMU_DEVICE_H
>>>> +#define HOST_IOMMU_DEVICE_H
>>>> +
>>>> +typedef enum HostIOMMUDevice_Type {
>>>> +    HID_LEGACY,
>>>> +    HID_IOMMUFD,
>>>> +    HID_MAX,
>>>> +} HostIOMMUDevice_Type;
>>>> +
>>>> +typedef struct HostIOMMUDevice {
>>>> +    HostIOMMUDevice_Type type;
>>>
>>> A type field is not a good sign and that's where QOM is useful.
>>
>> Yes, agree.
>> I didn't choose QOM because in iommufd-cdev series, VFIOContainer
>chooses not using QOM model.
>> See the discussion:
>https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/
>> I thought HostIOMMUDevice need to follow same rule.
>>
>> But after further digging into this, I think it may be ok to use QOM model
>as long as we don't expose
>> HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE
>interface. Your thoughts?
>
>yes. Can we change a bit this series to use QOM ? something like :
>
>     typedef struct HostIOMMUDevice {
>         Object parent;
>     } HostIOMMUDevice;
>
>     #define TYPE_HOST_IOMMU "host.iommu"
>     OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUClass,
>HOST_IOMMU)
>
>     struct HostIOMMUClass {
>         ObjectClass parent_class;
>
>         int (*get_type)(HostIOMMUDevice *hiod, uint64_t *type, Error **errp);
>         int (*get_cap)(HostIOMMUDevice *hiod, uint64_t *cap, Error **errp);
>     };
>
>Inherited objects would be TYPE_HOST_IOMMU_IOMMUFD and
>TYPE_HOST_IOMMU_LEGACY.
>Each class implementing the handlers or not (legacy mode).

Understood, thanks for your guide.

>
>The class handlers are introduced for the intel-iommu helper
>vtd_check_hdev()
>in order to avoid using iommufd routines directly. HostIOMMUDevice is
>supposed
>to abstract the Host IOMMU device, so we need to abstract also all the
>interfaces to this object.

I'd like to have a minimal adjustment to class handers. Just let me know if you have strong
preference.

Cap/ecap is intel_iommu specific, I'd like to make it a bit generic also for arm smmu usage,
and merge get_type and get_cap into one function as they both calls ioctl(IOMMU_GET_HW_INFO),
something like:
get_info(HostIOMMUDevice *hiod, enum iommu_hw_info_type *type, void **data, void **len,  Error **errp);

and let iommu emulater to extract content of *data. For intel_iommu, it's:

struct iommu_hw_info_vtd {
        __u32 flags;
        __u32 __reserved;
        __aligned_u64 cap_reg;
        __aligned_u64 ecap_reg;
};

>
>The .host_iommu_device_create() handler could be merged
>in .attach_device()
>possibly. Anyhow, please use now object_new() and object_unref() instead.
>host_iommu_base_device_init() is useless IMHO.

Good idea, will do.

>
>>
>>>
>>> Is vtd_check_hdev() the only use of this field ?
>>
>> Currently yes. virtio-iommu may have similar usage.
>>
>>> If so, can we simplify with a QOM interface in any way ?
>>
>> QOM interface is a set of callbacks, guess you mean QOM class,
>> saying HostIOMMUDevice class, IOMMULegacyDevice class and
>IOMMUFDDevice class?
>
>See above proposal. it should work fine.
>
>Also, I think it is better to use a IOMMUFDBackend* parameter for
>iommufd_device_get_info() to be consistent with the other routines.

Sure, then I'd like to also rename it to iommufd_backend_get_device_info().

Thanks
Zhenzhong

>
>Then It would interesting to see how this applies to Eric's series.
>
>Thanks,
>
>C.
>
>

Re: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

[Cédric Le Goater]

Hello Zhenzhong,

On 3/28/24 04:06, Duan, Zhenzhong wrote:
> Hi Cédric,
> 
>> -----Original Message-----
>> From: Cédric Le Goater <clg@redhat.com>
>> Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>> HostIOMMUDevice
>>
>> Hello Zhenzhong,
>>
>> On 3/19/24 12:58, Duan, Zhenzhong wrote:
>>> Hi Cédric,
>>>
>>>> -----Original Message-----
>>>> From: Cédric Le Goater <clg@redhat.com>
>>>> Sent: Tuesday, March 19, 2024 4:17 PM
>>>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>>>> devel@nongnu.org
>>>> Cc: alex.williamson@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>; Sun, Yi Y
>>>> <yi.y.sun@intel.com>; Peng, Chao P <chao.p.peng@intel.com>
>>>> Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>>>> HostIOMMUDevice
>>>>
>>>> Hello Zhenzhong,
>>>>
>>>> On 2/28/24 04:58, Zhenzhong Duan wrote:
>>>>> HostIOMMUDevice will be inherited by two sub classes,
>>>>> legacy and iommufd currently.
>>>>>
>>>>> Introduce a helper function host_iommu_base_device_init to initialize it.
>>>>>
>>>>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>> ---
>>>>>     include/sysemu/host_iommu_device.h | 22
>> ++++++++++++++++++++++
>>>>>     1 file changed, 22 insertions(+)
>>>>>     create mode 100644 include/sysemu/host_iommu_device.h
>>>>>
>>>>> diff --git a/include/sysemu/host_iommu_device.h
>>>> b/include/sysemu/host_iommu_device.h
>>>>> new file mode 100644
>>>>> index 0000000000..fe80ab25fb
>>>>> --- /dev/null
>>>>> +++ b/include/sysemu/host_iommu_device.h
>>>>> @@ -0,0 +1,22 @@
>>>>> +#ifndef HOST_IOMMU_DEVICE_H
>>>>> +#define HOST_IOMMU_DEVICE_H
>>>>> +
>>>>> +typedef enum HostIOMMUDevice_Type {
>>>>> +    HID_LEGACY,
>>>>> +    HID_IOMMUFD,
>>>>> +    HID_MAX,
>>>>> +} HostIOMMUDevice_Type;
>>>>> +
>>>>> +typedef struct HostIOMMUDevice {
>>>>> +    HostIOMMUDevice_Type type;
>>>>
>>>> A type field is not a good sign and that's where QOM is useful.
>>>
>>> Yes, agree.
>>> I didn't choose QOM because in iommufd-cdev series, VFIOContainer
>> chooses not using QOM model.
>>> See the discussion:
>> https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/
>>> I thought HostIOMMUDevice need to follow same rule.
>>>
>>> But after further digging into this, I think it may be ok to use QOM model
>> as long as we don't expose
>>> HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE
>> interface. Your thoughts?
>>
>> yes. Can we change a bit this series to use QOM ? something like :
>>
>>      typedef struct HostIOMMUDevice {
>>          Object parent;
>>      } HostIOMMUDevice;
>>
>>      #define TYPE_HOST_IOMMU "host.iommu"
>>      OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUClass,
>> HOST_IOMMU)
>>
>>      struct HostIOMMUClass {
>>          ObjectClass parent_class;
>>
>>          int (*get_type)(HostIOMMUDevice *hiod, uint64_t *type, Error **errp);
>>          int (*get_cap)(HostIOMMUDevice *hiod, uint64_t *cap, Error **errp);
>>      };
>>
>> Inherited objects would be TYPE_HOST_IOMMU_IOMMUFD and
>> TYPE_HOST_IOMMU_LEGACY.
>> Each class implementing the handlers or not (legacy mode).
> 
> Understood, thanks for your guide.
> 
>>
>> The class handlers are introduced for the intel-iommu helper
>> vtd_check_hdev()
>> in order to avoid using iommufd routines directly. HostIOMMUDevice is
>> supposed
>> to abstract the Host IOMMU device, so we need to abstract also all the
>> interfaces to this object.
> 
> I'd like to have a minimal adjustment to class handers. Just let me know if you have strong
> preference.
> 
> Cap/ecap is intel_iommu specific, I'd like to make it a bit generic also for arm smmu usage,
> and merge get_type and get_cap into one function as they both calls ioctl(IOMMU_GET_HW_INFO),
> something like:
> get_info(HostIOMMUDevice *hiod, enum iommu_hw_info_type *type, void **data, void **len,  Error **errp);

OK. Let's see how it goes. Having more users of this new object Host
IOMMU device is important to get a better feeling of the interface.
As of today, it doesn't have not much value. The iommufd object could
be QOM linked to the vIOMMU when available and we could get the bind
devid in some other ways I suppose. Anyhow, please keep it simple and
let's explore.

Thanks,

C.



> 
> and let iommu emulater to extract content of *data. For intel_iommu, it's:
> 
> struct iommu_hw_info_vtd {
>          __u32 flags;
>          __u32 __reserved;
>          __aligned_u64 cap_reg;
>          __aligned_u64 ecap_reg;
> };
> 
>>
>> The .host_iommu_device_create() handler could be merged
>> in .attach_device()
>> possibly. Anyhow, please use now object_new() and object_unref() instead.
>> host_iommu_base_device_init() is useless IMHO.
> 
> Good idea, will do.
> 
>>
>>>
>>>>
>>>> Is vtd_check_hdev() the only use of this field ?
>>>
>>> Currently yes. virtio-iommu may have similar usage.
>>>
>>>> If so, can we simplify with a QOM interface in any way ?
>>>
>>> QOM interface is a set of callbacks, guess you mean QOM class,
>>> saying HostIOMMUDevice class, IOMMULegacyDevice class and
>> IOMMUFDDevice class?
>>
>> See above proposal. it should work fine.
>>
>> Also, I think it is better to use a IOMMUFDBackend* parameter for
>> iommufd_device_get_info() to be consistent with the other routines.
> 
> Sure, then I'd like to also rename it to iommufd_backend_get_device_info().
> 
> Thanks
> Zhenzhong
> 
>>
>> Then It would interesting to see how this applies to Eric's series.
>>
>> Thanks,
>>
>> C.
>>
>>
> 

RE: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>HostIOMMUDevice
>
>Hello Zhenzhong,
>
>On 3/28/24 04:06, Duan, Zhenzhong wrote:
>> Hi Cédric,
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>>> HostIOMMUDevice
>>>
>>> Hello Zhenzhong,
>>>
>>> On 3/19/24 12:58, Duan, Zhenzhong wrote:
>>>> Hi Cédric,
>>>>
>>>>> -----Original Message-----
>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>> Sent: Tuesday, March 19, 2024 4:17 PM
>>>>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>>>>> devel@nongnu.org
>>>>> Cc: alex.williamson@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>; Sun, Yi Y
>>>>> <yi.y.sun@intel.com>; Peng, Chao P <chao.p.peng@intel.com>
>>>>> Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct
>>>>> HostIOMMUDevice
>>>>>
>>>>> Hello Zhenzhong,
>>>>>
>>>>> On 2/28/24 04:58, Zhenzhong Duan wrote:
>>>>>> HostIOMMUDevice will be inherited by two sub classes,
>>>>>> legacy and iommufd currently.
>>>>>>
>>>>>> Introduce a helper function host_iommu_base_device_init to
>initialize it.
>>>>>>
>>>>>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>>> ---
>>>>>>     include/sysemu/host_iommu_device.h | 22
>>> ++++++++++++++++++++++
>>>>>>     1 file changed, 22 insertions(+)
>>>>>>     create mode 100644 include/sysemu/host_iommu_device.h
>>>>>>
>>>>>> diff --git a/include/sysemu/host_iommu_device.h
>>>>> b/include/sysemu/host_iommu_device.h
>>>>>> new file mode 100644
>>>>>> index 0000000000..fe80ab25fb
>>>>>> --- /dev/null
>>>>>> +++ b/include/sysemu/host_iommu_device.h
>>>>>> @@ -0,0 +1,22 @@
>>>>>> +#ifndef HOST_IOMMU_DEVICE_H
>>>>>> +#define HOST_IOMMU_DEVICE_H
>>>>>> +
>>>>>> +typedef enum HostIOMMUDevice_Type {
>>>>>> +    HID_LEGACY,
>>>>>> +    HID_IOMMUFD,
>>>>>> +    HID_MAX,
>>>>>> +} HostIOMMUDevice_Type;
>>>>>> +
>>>>>> +typedef struct HostIOMMUDevice {
>>>>>> +    HostIOMMUDevice_Type type;
>>>>>
>>>>> A type field is not a good sign and that's where QOM is useful.
>>>>
>>>> Yes, agree.
>>>> I didn't choose QOM because in iommufd-cdev series, VFIOContainer
>>> chooses not using QOM model.
>>>> See the discussion:
>>> https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/
>>>> I thought HostIOMMUDevice need to follow same rule.
>>>>
>>>> But after further digging into this, I think it may be ok to use QOM
>model
>>> as long as we don't expose
>>>> HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE
>>> interface. Your thoughts?
>>>
>>> yes. Can we change a bit this series to use QOM ? something like :
>>>
>>>      typedef struct HostIOMMUDevice {
>>>          Object parent;
>>>      } HostIOMMUDevice;
>>>
>>>      #define TYPE_HOST_IOMMU "host.iommu"
>>>      OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUClass,
>>> HOST_IOMMU)
>>>
>>>      struct HostIOMMUClass {
>>>          ObjectClass parent_class;
>>>
>>>          int (*get_type)(HostIOMMUDevice *hiod, uint64_t *type, Error
>**errp);
>>>          int (*get_cap)(HostIOMMUDevice *hiod, uint64_t *cap, Error
>**errp);
>>>      };
>>>
>>> Inherited objects would be TYPE_HOST_IOMMU_IOMMUFD and
>>> TYPE_HOST_IOMMU_LEGACY.
>>> Each class implementing the handlers or not (legacy mode).
>>
>> Understood, thanks for your guide.
>>
>>>
>>> The class handlers are introduced for the intel-iommu helper
>>> vtd_check_hdev()
>>> in order to avoid using iommufd routines directly. HostIOMMUDevice is
>>> supposed
>>> to abstract the Host IOMMU device, so we need to abstract also all the
>>> interfaces to this object.
>>
>> I'd like to have a minimal adjustment to class handers. Just let me know if
>you have strong
>> preference.
>>
>> Cap/ecap is intel_iommu specific, I'd like to make it a bit generic also for
>arm smmu usage,
>> and merge get_type and get_cap into one function as they both calls
>ioctl(IOMMU_GET_HW_INFO),
>> something like:
>> get_info(HostIOMMUDevice *hiod, enum iommu_hw_info_type *type,
>void **data, void **len,  Error **errp);
>
>OK. Let's see how it goes. Having more users of this new object Host
>IOMMU device is important to get a better feeling of the interface.
>As of today, it doesn't have not much value. The iommufd object could
>be QOM linked to the vIOMMU when available and we could get the bind
>devid in some other ways I suppose. Anyhow, please keep it simple and
>let's explore.

Got it, thanks Cédric!

BRs.
Zhenzhong