[PATCH 0/5] cleanup interfaces

[PATCH 0/5] cleanup interfaces

[Zhenzhong Duan]

Hi,

This series addresses Cédric's suggestion[1] and Donald's suggestion[2] to
move realize() call after attach_device().

Also addresses Eric and Nicolin's suggestion[3] to use a union to hold different
vendor capabilities.

[1] https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg01211.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg00898.html
[3] https://lists.gnu.org/archive/html/qemu-devel/2025-03/msg01552.html

Test:
net card passthrough and ping test
hotplug/unplug

Based on vfio-next(b9d42a878b).

Thanks
Zhenzhong


Zhenzhong Duan (5):
  vfio/iommufd: Save host iommu capabilities in VFIODevice.caps
  vfio: Move realize() after attach_device()
  vfio/iommufd: Implement .get_cap() in
    TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO sub-class
  backends/iommufd: Drop hiod_iommufd_get_cap()
  vfio/iommufd: Drop HostIOMMUDeviceCaps from HostIOMMUDevice

 include/hw/vfio/vfio-device.h      |  2 +-
 include/system/host_iommu_device.h | 14 -------
 include/system/iommufd.h           | 26 ++++++++++++
 backends/iommufd.c                 | 63 +++++++++++++++++++-----------
 hw/vfio/container.c                |  4 --
 hw/vfio/device.c                   | 28 ++++++-------
 hw/vfio/iommufd.c                  | 39 ++++++++++--------
 7 files changed, 100 insertions(+), 76 deletions(-)

-- 
2.34.1

[PATCH 1/5] vfio/iommufd: Save host iommu capabilities in VFIODevice.caps

[Zhenzhong Duan]

The saved caps copy can be used to check dirty tracking capability.

The capabilities is gotten through IOMMUFD interface, so define a
new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor
caps raw data in "include/system/iommufd.h".

This is a prepare work for moving .realize() after .attach_device().

Suggested-by: Cédric Le Goater <clg@redhat.com>
Suggested-by: Eric Auger <eric.auger@redhat.com>
Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/vfio/vfio-device.h |  1 +
 include/system/iommufd.h      | 22 ++++++++++++++++++++++
 hw/vfio/iommufd.c             | 10 +++++++++-
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index 66797b4c92..09a7af891a 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -77,6 +77,7 @@ typedef struct VFIODevice {
     bool dirty_tracking; /* Protected by BQL */
     bool iommu_dirty_tracking;
     HostIOMMUDevice *hiod;
+    HostIOMMUDeviceIOMMUFDCaps caps;
     int devid;
     IOMMUFDBackend *iommufd;
     VFIOIOASHwpt *hwpt;
diff --git a/include/system/iommufd.h b/include/system/iommufd.h
index cbab75bfbf..0f337585c9 100644
--- a/include/system/iommufd.h
+++ b/include/system/iommufd.h
@@ -18,6 +18,9 @@
 #include "exec/hwaddr.h"
 #include "exec/cpu-common.h"
 #include "system/host_iommu_device.h"
+#ifdef CONFIG_LINUX
+#include <linux/iommufd.h>
+#endif
 
 #define TYPE_IOMMUFD_BACKEND "iommufd"
 OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, IOMMUFD_BACKEND)
@@ -63,4 +66,23 @@ bool iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id,
                                       Error **errp);
 
 #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
+
+typedef union VendorCaps {
+    struct iommu_hw_info_vtd vtd;
+    struct iommu_hw_info_arm_smmuv3 smmuv3;
+} VendorCaps;
+
+/**
+ * struct HostIOMMUDeviceIOMMUFDCaps - Define host IOMMU device capabilities.
+ *
+ * @type: host platform IOMMU type.
+ *
+ * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents
+ *           the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl)
+ */
+typedef struct HostIOMMUDeviceIOMMUFDCaps {
+    uint32_t type;
+    uint64_t hw_caps;
+    VendorCaps vendor_caps;
+} HostIOMMUDeviceIOMMUFDCaps;
 #endif
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 48db105422..530cde6740 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -324,7 +324,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
      * vfio_migration_realize() may decide to use VF dirty tracking
      * instead.
      */
-    if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
+    if (vbasedev->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
         flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
     }
 
@@ -475,6 +475,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
     int ret, devfd;
     uint32_t ioas_id;
     Error *err = NULL;
+    HostIOMMUDeviceIOMMUFDCaps *caps = &vbasedev->caps;
     const VFIOIOMMUClass *iommufd_vioc =
         VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
 
@@ -505,6 +506,13 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
         goto err_alloc_ioas;
     }
 
+    if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid,
+                                         &caps->type, &caps->vendor_caps,
+                                         sizeof(VendorCaps), &caps->hw_caps,
+                                         errp)) {
+        goto err_alloc_ioas;
+    }
+
     /* try to attach to an existing container in this space */
     QLIST_FOREACH(bcontainer, &space->containers, next) {
         container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);
-- 
2.34.1

Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in VFIODevice.caps

[Joao Martins]

On 11/04/2025 11:17, Zhenzhong Duan wrote:
> The saved caps copy can be used to check dirty tracking capability.
> 
> The capabilities is gotten through IOMMUFD interface, so define a
> new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor
> caps raw data in "include/system/iommufd.h".
> 
> This is a prepare work for moving .realize() after .attach_device().
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  include/hw/vfio/vfio-device.h |  1 +
>  include/system/iommufd.h      | 22 ++++++++++++++++++++++
>  hw/vfio/iommufd.c             | 10 +++++++++-
>  3 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index 66797b4c92..09a7af891a 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -77,6 +77,7 @@ typedef struct VFIODevice {
>      bool dirty_tracking; /* Protected by BQL */
>      bool iommu_dirty_tracking;
>      HostIOMMUDevice *hiod;
> +    HostIOMMUDeviceIOMMUFDCaps caps;
>      int devid;
>      IOMMUFDBackend *iommufd;
>      VFIOIOASHwpt *hwpt;
> diff --git a/include/system/iommufd.h b/include/system/iommufd.h
> index cbab75bfbf..0f337585c9 100644
> --- a/include/system/iommufd.h
> +++ b/include/system/iommufd.h
> @@ -18,6 +18,9 @@
>  #include "exec/hwaddr.h"
>  #include "exec/cpu-common.h"
>  #include "system/host_iommu_device.h"
> +#ifdef CONFIG_LINUX
> +#include <linux/iommufd.h>
> +#endif
>  
>  #define TYPE_IOMMUFD_BACKEND "iommufd"
>  OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, IOMMUFD_BACKEND)
> @@ -63,4 +66,23 @@ bool iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id,
>                                        Error **errp);
>  
>  #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
> +
> +typedef union VendorCaps {
> +    struct iommu_hw_info_vtd vtd;
> +    struct iommu_hw_info_arm_smmuv3 smmuv3;
> +} VendorCaps;
> +
> +/**
> + * struct HostIOMMUDeviceIOMMUFDCaps - Define host IOMMU device capabilities.
> + *
> + * @type: host platform IOMMU type.
> + *
> + * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents
> + *           the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl)
> + */
> +typedef struct HostIOMMUDeviceIOMMUFDCaps {
> +    uint32_t type;
> +    uint64_t hw_caps;
> +    VendorCaps vendor_caps;
> +} HostIOMMUDeviceIOMMUFDCaps;
>  #endif
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 48db105422..530cde6740 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -324,7 +324,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>       * vfio_migration_realize() may decide to use VF dirty tracking
>       * instead.
>       */
> -    if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
> +    if (vbasedev->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
>          flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>      }
>  
> @@ -475,6 +475,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>      int ret, devfd;
>      uint32_t ioas_id;
>      Error *err = NULL;
> +    HostIOMMUDeviceIOMMUFDCaps *caps = &vbasedev->caps;
>      const VFIOIOMMUClass *iommufd_vioc =
>          VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
>  
> @@ -505,6 +506,13 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>          goto err_alloc_ioas;
>      }
>  
> +    if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid,
> +                                         &caps->type, &caps->vendor_caps,
> +                                         sizeof(VendorCaps), &caps->hw_caps,
> +                                         errp)) {
> +        goto err_alloc_ioas;
> +    }
> +

I think this will fail on mdev (and thus fail the attachment mistakengly as
there's no IOMMUFDDevice with mdev) ? In case it fails, you can just do:

	if (!vbasedev->mdev && !iommufd_backend_get_device_info(...)) {

RE: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in VFIODevice.caps

[Duan, Zhenzhong]

>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Subject: Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in
>VFIODevice.caps
>
>On 11/04/2025 11:17, Zhenzhong Duan wrote:
>> The saved caps copy can be used to check dirty tracking capability.
>>
>> The capabilities is gotten through IOMMUFD interface, so define a
>> new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor
>> caps raw data in "include/system/iommufd.h".
>>
>> This is a prepare work for moving .realize() after .attach_device().
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  include/hw/vfio/vfio-device.h |  1 +
>>  include/system/iommufd.h      | 22 ++++++++++++++++++++++
>>  hw/vfio/iommufd.c             | 10 +++++++++-
>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
>> index 66797b4c92..09a7af891a 100644
>> --- a/include/hw/vfio/vfio-device.h
>> +++ b/include/hw/vfio/vfio-device.h
>> @@ -77,6 +77,7 @@ typedef struct VFIODevice {
>>      bool dirty_tracking; /* Protected by BQL */
>>      bool iommu_dirty_tracking;
>>      HostIOMMUDevice *hiod;
>> +    HostIOMMUDeviceIOMMUFDCaps caps;
>>      int devid;
>>      IOMMUFDBackend *iommufd;
>>      VFIOIOASHwpt *hwpt;
>> diff --git a/include/system/iommufd.h b/include/system/iommufd.h
>> index cbab75bfbf..0f337585c9 100644
>> --- a/include/system/iommufd.h
>> +++ b/include/system/iommufd.h
>> @@ -18,6 +18,9 @@
>>  #include "exec/hwaddr.h"
>>  #include "exec/cpu-common.h"
>>  #include "system/host_iommu_device.h"
>> +#ifdef CONFIG_LINUX
>> +#include <linux/iommufd.h>
>> +#endif
>>
>>  #define TYPE_IOMMUFD_BACKEND "iommufd"
>>  OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass,
>IOMMUFD_BACKEND)
>> @@ -63,4 +66,23 @@ bool
>iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id,
>>                                        Error **errp);
>>
>>  #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD
>TYPE_HOST_IOMMU_DEVICE "-iommufd"
>> +
>> +typedef union VendorCaps {
>> +    struct iommu_hw_info_vtd vtd;
>> +    struct iommu_hw_info_arm_smmuv3 smmuv3;
>> +} VendorCaps;
>> +
>> +/**
>> + * struct HostIOMMUDeviceIOMMUFDCaps - Define host IOMMU device
>capabilities.
>> + *
>> + * @type: host platform IOMMU type.
>> + *
>> + * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this
>represents
>> + *           the @out_capabilities value returned from IOMMU_GET_HW_INFO
>ioctl)
>> + */
>> +typedef struct HostIOMMUDeviceIOMMUFDCaps {
>> +    uint32_t type;
>> +    uint64_t hw_caps;
>> +    VendorCaps vendor_caps;
>> +} HostIOMMUDeviceIOMMUFDCaps;
>>  #endif
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index 48db105422..530cde6740 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -324,7 +324,7 @@ static bool
>iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>>       * vfio_migration_realize() may decide to use VF dirty tracking
>>       * instead.
>>       */
>> -    if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
>> +    if (vbasedev->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
>>          flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>>      }
>>
>> @@ -475,6 +475,7 @@ static bool iommufd_cdev_attach(const char *name,
>VFIODevice *vbasedev,
>>      int ret, devfd;
>>      uint32_t ioas_id;
>>      Error *err = NULL;
>> +    HostIOMMUDeviceIOMMUFDCaps *caps = &vbasedev->caps;
>>      const VFIOIOMMUClass *iommufd_vioc =
>>
>VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
>>
>> @@ -505,6 +506,13 @@ static bool iommufd_cdev_attach(const char *name,
>VFIODevice *vbasedev,
>>          goto err_alloc_ioas;
>>      }
>>
>> +    if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev-
>>devid,
>> +                                         &caps->type, &caps->vendor_caps,
>> +                                         sizeof(VendorCaps), &caps->hw_caps,
>> +                                         errp)) {
>> +        goto err_alloc_ioas;
>> +    }
>> +
>
>I think this will fail on mdev (and thus fail the attachment mistakengly as
>there's no IOMMUFDDevice with mdev) ? In case it fails, you can just do:
>
>	if (!vbasedev->mdev && !iommufd_backend_get_device_info(...)) {

Indeed, thanks for caching this.

BRs,
Zhenzhong

Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in VFIODevice.caps

[Cédric Le Goater]

On 4/11/25 12:17, Zhenzhong Duan wrote:
> The saved caps copy can be used to check dirty tracking capability.
> 
> The capabilities is gotten through IOMMUFD interface, so define a
> new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor
> caps raw data in "include/system/iommufd.h".
> 
> This is a prepare work for moving .realize() after .attach_device().
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/hw/vfio/vfio-device.h |  1 +
>   include/system/iommufd.h      | 22 ++++++++++++++++++++++
>   hw/vfio/iommufd.c             | 10 +++++++++-
>   3 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index 66797b4c92..09a7af891a 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -77,6 +77,7 @@ typedef struct VFIODevice {
>       bool dirty_tracking; /* Protected by BQL */
>       bool iommu_dirty_tracking;
>       HostIOMMUDevice *hiod;
> +    HostIOMMUDeviceIOMMUFDCaps caps;

IMO, these capabilities belong to HostIOMMUDevice and not VFIODevice.

I would simply call iommufd_backend_get_device_info() twice where needed :
iommufd_cdev_autodomains_get() and  hiod_iommufd_vfio_realize()


Thanks,

C.



>       int devid;
>       IOMMUFDBackend *iommufd;
>       VFIOIOASHwpt *hwpt;
> diff --git a/include/system/iommufd.h b/include/system/iommufd.h
> index cbab75bfbf..0f337585c9 100644
> --- a/include/system/iommufd.h
> +++ b/include/system/iommufd.h
> @@ -18,6 +18,9 @@
>   #include "exec/hwaddr.h"
>   #include "exec/cpu-common.h"
>   #include "system/host_iommu_device.h"
> +#ifdef CONFIG_LINUX
> +#include <linux/iommufd.h>
> +#endif
>   
>   #define TYPE_IOMMUFD_BACKEND "iommufd"
>   OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, IOMMUFD_BACKEND)
> @@ -63,4 +66,23 @@ bool iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id,
>                                         Error **errp);
>   
>   #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
> +
> +typedef union VendorCaps {
> +    struct iommu_hw_info_vtd vtd;
> +    struct iommu_hw_info_arm_smmuv3 smmuv3;
> +} VendorCaps;
> +
> +/**
> + * struct HostIOMMUDeviceIOMMUFDCaps - Define host IOMMU device capabilities.
> + *
> + * @type: host platform IOMMU type.
> + *
> + * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents
> + *           the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl)
> + */
> +typedef struct HostIOMMUDeviceIOMMUFDCaps {
> +    uint32_t type;
> +    uint64_t hw_caps;
> +    VendorCaps vendor_caps;
> +} HostIOMMUDeviceIOMMUFDCaps;
>   #endif
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 48db105422..530cde6740 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -324,7 +324,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>        * vfio_migration_realize() may decide to use VF dirty tracking
>        * instead.
>        */
> -    if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
> +    if (vbasedev->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
>           flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>       }
>   
> @@ -475,6 +475,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>       int ret, devfd;
>       uint32_t ioas_id;
>       Error *err = NULL;
> +    HostIOMMUDeviceIOMMUFDCaps *caps = &vbasedev->caps;
>       const VFIOIOMMUClass *iommufd_vioc =
>           VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
>   
> @@ -505,6 +506,13 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>           goto err_alloc_ioas;
>       }
>   
> +    if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid,
> +                                         &caps->type, &caps->vendor_caps,
> +                                         sizeof(VendorCaps), &caps->hw_caps,
> +                                         errp)) {
> +        goto err_alloc_ioas;
> +    }
> +
>       /* try to attach to an existing container in this space */
>       QLIST_FOREACH(bcontainer, &space->containers, next) {
>           container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);

RE: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in VFIODevice.caps

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in
>VFIODevice.caps
>
>On 4/11/25 12:17, Zhenzhong Duan wrote:
>> The saved caps copy can be used to check dirty tracking capability.
>>
>> The capabilities is gotten through IOMMUFD interface, so define a
>> new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor
>> caps raw data in "include/system/iommufd.h".
>>
>> This is a prepare work for moving .realize() after .attach_device().
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/hw/vfio/vfio-device.h |  1 +
>>   include/system/iommufd.h      | 22 ++++++++++++++++++++++
>>   hw/vfio/iommufd.c             | 10 +++++++++-
>>   3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
>> index 66797b4c92..09a7af891a 100644
>> --- a/include/hw/vfio/vfio-device.h
>> +++ b/include/hw/vfio/vfio-device.h
>> @@ -77,6 +77,7 @@ typedef struct VFIODevice {
>>       bool dirty_tracking; /* Protected by BQL */
>>       bool iommu_dirty_tracking;
>>       HostIOMMUDevice *hiod;
>> +    HostIOMMUDeviceIOMMUFDCaps caps;
>
>IMO, these capabilities belong to HostIOMMUDevice and not VFIODevice.

This was trying to address suggestions in [1], caps is generated by IOMMUFD backend
and is only used by hiod_iommufd_get_cap(), hiod_legacy_vfio_get_cap() never
check it. By putting it in VFIODevice, I can save vendor caps in a union and raw
data format, hiod_iommufd_get_cap() recognizes the raw data format and can
check it for a cap support.

If keep caps in HostIOMMUDevice, I can think of a change like below to address Eric and Nicolin's suggestion:
https://github.com/yiliu1765/qemu/commit/e05f91b2a724cefa8356969cb43284f7c3ec11d1
https://github.com/yiliu1765/qemu/commit/e05f91b2a724cefa8356969cb43284f7c3ec11d1

Does the change make sense for you?

[1] https://lists.gnu.org/archive/html/qemu-devel/2025-03/msg01552.html

>
>I would simply call iommufd_backend_get_device_info() twice where needed :
>iommufd_cdev_autodomains_get() and  hiod_iommufd_vfio_realize()

OK, will do, that's simpler than current change.

Thanks
Zhenzhong

RE: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in VFIODevice.caps

[Duan, Zhenzhong]

>-----Original Message-----
>From: Nicolin Chen <nicolinc@nvidia.com>
>Subject: Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in
>VFIODevice.caps
>
>On Mon, Apr 14, 2025 at 09:30:41AM +0000, Duan, Zhenzhong wrote:
>> >-----Original Message-----
>> >From: Cédric Le Goater <clg@redhat.com>
>> >> @@ -77,6 +77,7 @@ typedef struct VFIODevice {
>> >>       bool dirty_tracking; /* Protected by BQL */
>> >>       bool iommu_dirty_tracking;
>> >>       HostIOMMUDevice *hiod;
>> >> +    HostIOMMUDeviceIOMMUFDCaps caps;
>> >
>> >IMO, these capabilities belong to HostIOMMUDevice and not VFIODevice.
>
>It's a bit complicated now, as a cap like PASID more belongs to
>the VFIO device than the IOMMU device, and should be read by
>the PCI VFIO code to set the PASID cap field. With this being
>said...

Yes, and ATS and PRQ capabilities.

>
>> This was trying to address suggestions in [1], caps is generated by IOMMUFD
>backend
>> and is only used by hiod_iommufd_get_cap(), hiod_legacy_vfio_get_cap() never
>> check it. By putting it in VFIODevice, I can save vendor caps in a union and raw
>> data format, hiod_iommufd_get_cap() recognizes the raw data format and can
>> check it for a cap support.
>
>It could still get hiod->caps?

No, in this series, I want to move hiod->caps to VFIODevice->caps,
only interface to get a cap is .get_cap() interface.

>
>I think the legacy pathway could have a NULL hiod or NULL caps,
>so both get_cap() functions could work.

If NULL hiod for legacy pathway, then vIOMMU could not call
hiod_legacy_vfio_get_cap() in any way for VFIO device with legacy backend.

> And I was expecting the
>vIOMMU could provide some callback for the core to use so it'll
>return a valid caps pointer.

Any reason for that?
iommufd_backend_get_device_info() need iommufd fd and devid which are
all provided by VFIO, why not let VFIO call it directly and let vIOMMU query
cap through .get_cap().

Thanks
Zhenzhong

RE: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in VFIODevice.caps

[Duan, Zhenzhong]

>-----Original Message-----
>From: Nicolin Chen <nicolinc@nvidia.com>
>Subject: Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in
>VFIODevice.caps
>
>On Wed, Apr 16, 2025 at 05:49:09AM +0000, Duan, Zhenzhong wrote:
>> >-----Original Message-----
>> >From: Nicolin Chen <nicolinc@nvidia.com>
>
>> > And I was expecting the
>> >vIOMMU could provide some callback for the core to use so it'll
>> >return a valid caps pointer.
>>
>> Any reason for that?
>> iommufd_backend_get_device_info() need iommufd fd and devid which are
>> all provided by VFIO, why not let VFIO call it directly and let vIOMMU query
>> cap through .get_cap().
>
>I thought we had an agreement on letting the core code stay away
>from vendor types and handlers, which seems to be a common sense?
>At least the kernel side never handles "VTD" and "SMMU" types, so
>shouldn't we do the same in QEMU?

As Cedric suggested in [1] "The IOMMU backend implementation could be anything, legacy, iommufd, iommufd v2, some other framework and the vIOMMU shouldn't be aware of its implementation.", so now we have core code handle the vendor data and provide a general capability query through .get_cap() callback.

>
>IMHO, it's okay to do the ioctl in the core code since it has all
>the inputs as you said. Yet, the core code should only decode the
>IOMMU_HW_CAP_PCI_PASID_ caps out of the out_capabilities, as they
>were filled by the core code in the kernel. For the type/data, it
>can store them in a cap structure, and forward to the vendor code
>via a callback function to decode them there for some common bits
>(e.g. nesting).

This is exactly what I had ever done in an old version, VFIO provides hiodc->get_host_iommu_info() callback for vIOMMU to get host IOMMU info.
See https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg00770.html
and https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg00784.html

Cedric suggested a more general interface between VFIO and vIOMMU:

"we could introduce in the vIOMMU <-> HostIOMMUDevice
interface capabilities, or features, to check more precisely the level
of compatibility between the vIOMMU and the host IOMMU device. This is
similar to what is done between QEMU and KVM"

See [1] for discussion details.

[1] https://lists.gnu.org/archive/html/qemu-devel/2024-04/msg02658.html

Thanks
Zhenzhong

>
>The other way around is to do the ioctl in the vendor code via a
>callback function that returns the cap structure. I think this is
>slightly better, since the vendor code knows the exact data size
>and it can validate the returned "type" immediately.
>
>Thanks
>Nicolin

Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in VFIODevice.caps

[Eric Auger]

Hi Zhenzhong,

On 4/11/25 1:28 PM, Cédric Le Goater wrote:
> On 4/11/25 12:17, Zhenzhong Duan wrote:
>> The saved caps copy can be used to check dirty tracking capability.
>>
>> The capabilities is gotten through IOMMUFD interface, so define a
>> new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor
>> caps raw data in "include/system/iommufd.h".
>>
>> This is a prepare work for moving .realize() after .attach_device().
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/hw/vfio/vfio-device.h |  1 +
>>   include/system/iommufd.h      | 22 ++++++++++++++++++++++
>>   hw/vfio/iommufd.c             | 10 +++++++++-
>>   3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/hw/vfio/vfio-device.h
>> b/include/hw/vfio/vfio-device.h
>> index 66797b4c92..09a7af891a 100644
>> --- a/include/hw/vfio/vfio-device.h
>> +++ b/include/hw/vfio/vfio-device.h
>> @@ -77,6 +77,7 @@ typedef struct VFIODevice {
>>       bool dirty_tracking; /* Protected by BQL */
>>       bool iommu_dirty_tracking;
>>       HostIOMMUDevice *hiod;
>> +    HostIOMMUDeviceIOMMUFDCaps caps;
>
> IMO, these capabilities belong to HostIOMMUDevice and not VFIODevice.
I do agree with Cédric that it looks a wrong place to put this caps. I
feel this somehow breaks the abstraction layering.

Now "[PATCH v2 0/5] vfio: Move realize after attach_dev" has landed, I
think it would help if you could respin with a clear functional goal
such as the one targeted in[PATCH v2 0/5] Check host IOMMU compatilibity
with vIOMMU
<https://lore.kernel.org/all/20240408084404.1111628-1-zhenzhong.duan@intel.com/>

Thanks

Eric
>
> I would simply call iommufd_backend_get_device_info() twice where
> needed :
> iommufd_cdev_autodomains_get() and  hiod_iommufd_vfio_realize()
>
>
> Thanks,
>
> C.
>
>
>
>>       int devid;
>>       IOMMUFDBackend *iommufd;
>>       VFIOIOASHwpt *hwpt;
>> diff --git a/include/system/iommufd.h b/include/system/iommufd.h
>> index cbab75bfbf..0f337585c9 100644
>> --- a/include/system/iommufd.h
>> +++ b/include/system/iommufd.h
>> @@ -18,6 +18,9 @@
>>   #include "exec/hwaddr.h"
>>   #include "exec/cpu-common.h"
>>   #include "system/host_iommu_device.h"
>> +#ifdef CONFIG_LINUX
>> +#include <linux/iommufd.h>
>> +#endif
>>     #define TYPE_IOMMUFD_BACKEND "iommufd"
>>   OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass,
>> IOMMUFD_BACKEND)
>> @@ -63,4 +66,23 @@ bool
>> iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id,
>>                                         Error **errp);
>>     #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE
>> "-iommufd"
>> +
>> +typedef union VendorCaps {
>> +    struct iommu_hw_info_vtd vtd;
>> +    struct iommu_hw_info_arm_smmuv3 smmuv3;
>> +} VendorCaps;
>> +
>> +/**
>> + * struct HostIOMMUDeviceIOMMUFDCaps - Define host IOMMU device
>> capabilities.
>> + *
>> + * @type: host platform IOMMU type.
>> + *
>> + * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this
>> represents
>> + *           the @out_capabilities value returned from
>> IOMMU_GET_HW_INFO ioctl)
>> + */
>> +typedef struct HostIOMMUDeviceIOMMUFDCaps {
>> +    uint32_t type;
>> +    uint64_t hw_caps;
>> +    VendorCaps vendor_caps;
>> +} HostIOMMUDeviceIOMMUFDCaps;
>>   #endif
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index 48db105422..530cde6740 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -324,7 +324,7 @@ static bool
>> iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>>        * vfio_migration_realize() may decide to use VF dirty tracking
>>        * instead.
>>        */
>> -    if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
>> +    if (vbasedev->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
>>           flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>>       }
>>   @@ -475,6 +475,7 @@ static bool iommufd_cdev_attach(const char
>> *name, VFIODevice *vbasedev,
>>       int ret, devfd;
>>       uint32_t ioas_id;
>>       Error *err = NULL;
>> +    HostIOMMUDeviceIOMMUFDCaps *caps = &vbasedev->caps;
>>       const VFIOIOMMUClass *iommufd_vioc =
>>          
>> VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
>>   @@ -505,6 +506,13 @@ static bool iommufd_cdev_attach(const char
>> *name, VFIODevice *vbasedev,
>>           goto err_alloc_ioas;
>>       }
>>   +    if (!iommufd_backend_get_device_info(vbasedev->iommufd,
>> vbasedev->devid,
>> +                                         &caps->type,
>> &caps->vendor_caps,
>> +                                         sizeof(VendorCaps),
>> &caps->hw_caps,
>> +                                         errp)) {
>> +        goto err_alloc_ioas;
>> +    }
>> +
>>       /* try to attach to an existing container in this space */
>>       QLIST_FOREACH(bcontainer, &space->containers, next) {
>>           container = container_of(bcontainer, VFIOIOMMUFDContainer,
>> bcontainer);
>

RE: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in VFIODevice.caps

[Duan, Zhenzhong]

Hi Eric,

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Sent: Tuesday, May 6, 2025 12:15 AM
>To: Cédric Le Goater <clg@redhat.com>; Duan, Zhenzhong
><zhenzhong.duan@intel.com>; qemu-devel@nongnu.org
>Cc: alex.williamson@redhat.com; nicolinc@nvidia.com;
>joao.m.martins@oracle.com; Peng, Chao P <chao.p.peng@intel.com>; Liu, Yi L
><yi.l.liu@intel.com>
>Subject: Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in
>VFIODevice.caps
>
>Hi Zhenzhong,
>
>On 4/11/25 1:28 PM, Cédric Le Goater wrote:
>> On 4/11/25 12:17, Zhenzhong Duan wrote:
>>> The saved caps copy can be used to check dirty tracking capability.
>>>
>>> The capabilities is gotten through IOMMUFD interface, so define a
>>> new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor
>>> caps raw data in "include/system/iommufd.h".
>>>
>>> This is a prepare work for moving .realize() after .attach_device().
>>>
>>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>>> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> ---
>>>   include/hw/vfio/vfio-device.h |  1 +
>>>   include/system/iommufd.h      | 22 ++++++++++++++++++++++
>>>   hw/vfio/iommufd.c             | 10 +++++++++-
>>>   3 files changed, 32 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/hw/vfio/vfio-device.h
>>> b/include/hw/vfio/vfio-device.h
>>> index 66797b4c92..09a7af891a 100644
>>> --- a/include/hw/vfio/vfio-device.h
>>> +++ b/include/hw/vfio/vfio-device.h
>>> @@ -77,6 +77,7 @@ typedef struct VFIODevice {
>>>       bool dirty_tracking; /* Protected by BQL */
>>>       bool iommu_dirty_tracking;
>>>       HostIOMMUDevice *hiod;
>>> +    HostIOMMUDeviceIOMMUFDCaps caps;
>>
>> IMO, these capabilities belong to HostIOMMUDevice and not VFIODevice.
>I do agree with Cédric that it looks a wrong place to put this caps. I
>feel this somehow breaks the abstraction layering.

This change was dropped in "[PATCH v2 0/5] vfio: Move realize after attach_dev".

>
>Now "[PATCH v2 0/5] vfio: Move realize after attach_dev" has landed, I
>think it would help if you could respin with a clear functional goal
>such as the one targeted in[PATCH v2 0/5] Check host IOMMU compatilibity
>with vIOMMU
><https://lore.kernel.org/all/20240408084404.1111628-1-
>zhenzhong.duan@intel.com/>

See a rfcv3 candidate at link https://github.com/yiliu1765/qemu/commits/zhenzhong/iommufd_nesting_rfcv3.wip/ for example implementation.

In this example, I used .get_cap() interface for vIOMMU to get cap. 
vIOMMU could also access HostIOMMUDevice::HostIOMMUDeviceCaps
directly as it's passed from VFIO to vIOMMU along with HostIOMMUDevice.

Thanks
Zhenzhong

Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in VFIODevice.caps

[Eric Auger]

On 4/11/25 12:17 PM, Zhenzhong Duan wrote:
> The saved caps copy can be used to check dirty tracking capability.
>
> The capabilities is gotten through IOMMUFD interface, so define a
> new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor
> caps raw data in "include/system/iommufd.h".
>
> This is a prepare work for moving .realize() after .attach_device().
>
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  include/hw/vfio/vfio-device.h |  1 +
>  include/system/iommufd.h      | 22 ++++++++++++++++++++++
>  hw/vfio/iommufd.c             | 10 +++++++++-
>  3 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index 66797b4c92..09a7af891a 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -77,6 +77,7 @@ typedef struct VFIODevice {
>      bool dirty_tracking; /* Protected by BQL */
>      bool iommu_dirty_tracking;
>      HostIOMMUDevice *hiod;
> +    HostIOMMUDeviceIOMMUFDCaps caps;
>      int devid;
>      IOMMUFDBackend *iommufd;
>      VFIOIOASHwpt *hwpt;
> diff --git a/include/system/iommufd.h b/include/system/iommufd.h
> index cbab75bfbf..0f337585c9 100644
> --- a/include/system/iommufd.h
> +++ b/include/system/iommufd.h
> @@ -18,6 +18,9 @@
>  #include "exec/hwaddr.h"
>  #include "exec/cpu-common.h"
>  #include "system/host_iommu_device.h"
> +#ifdef CONFIG_LINUX
> +#include <linux/iommufd.h>
> +#endif
>  
>  #define TYPE_IOMMUFD_BACKEND "iommufd"
>  OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, IOMMUFD_BACKEND)
> @@ -63,4 +66,23 @@ bool iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id,
>                                        Error **errp);
>  
>  #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
> +
> +typedef union VendorCaps {
> +    struct iommu_hw_info_vtd vtd;
> +    struct iommu_hw_info_arm_smmuv3 smmuv3;
> +} VendorCaps;
> +
> +/**
> + * struct HostIOMMUDeviceIOMMUFDCaps - Define host IOMMU device capabilities.
> + *
> + * @type: host platform IOMMU type.
> + *
> + * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents
> + *           the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl)
> + */
> +typedef struct HostIOMMUDeviceIOMMUFDCaps {
> +    uint32_t type;
> +    uint64_t hw_caps;
> +    VendorCaps vendor_caps;
> +} HostIOMMUDeviceIOMMUFDCaps;
Why can't we extend the existing HostIOMMUDeviceCaps in host_iommu_device.h?

Eric
>  #endif
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 48db105422..530cde6740 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -324,7 +324,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>       * vfio_migration_realize() may decide to use VF dirty tracking
>       * instead.
>       */
> -    if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
> +    if (vbasedev->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
>          flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>      }
>  
> @@ -475,6 +475,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>      int ret, devfd;
>      uint32_t ioas_id;
>      Error *err = NULL;
> +    HostIOMMUDeviceIOMMUFDCaps *caps = &vbasedev->caps;
>      const VFIOIOMMUClass *iommufd_vioc =
>          VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
>  
> @@ -505,6 +506,13 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>          goto err_alloc_ioas;
>      }
>  
> +    if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid,
> +                                         &caps->type, &caps->vendor_caps,
> +                                         sizeof(VendorCaps), &caps->hw_caps,
> +                                         errp)) {
> +        goto err_alloc_ioas;
> +    }
> +
>      /* try to attach to an existing container in this space */
>      QLIST_FOREACH(bcontainer, &space->containers, next) {
>          container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);

Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in VFIODevice.caps

[Eric Auger]

Hi Zhenzhong,

On 4/11/25 12:17 PM, Zhenzhong Duan wrote:
> The saved caps copy can be used to check dirty tracking capability.
>
> The capabilities is gotten through IOMMUFD interface, so define a
> new structure HostIOMMUDeviceIOMMUFDCaps which contains vendor
> caps raw data in "include/system/iommufd.h".
>
> This is a prepare work for moving .realize() after .attach_device().
>
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  include/hw/vfio/vfio-device.h |  1 +
>  include/system/iommufd.h      | 22 ++++++++++++++++++++++
>  hw/vfio/iommufd.c             | 10 +++++++++-
>  3 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index 66797b4c92..09a7af891a 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -77,6 +77,7 @@ typedef struct VFIODevice {
>      bool dirty_tracking; /* Protected by BQL */
>      bool iommu_dirty_tracking;
>      HostIOMMUDevice *hiod;
> +    HostIOMMUDeviceIOMMUFDCaps caps;
>      int devid;
>      IOMMUFDBackend *iommufd;
>      VFIOIOASHwpt *hwpt;
> diff --git a/include/system/iommufd.h b/include/system/iommufd.h
> index cbab75bfbf..0f337585c9 100644
> --- a/include/system/iommufd.h
> +++ b/include/system/iommufd.h
> @@ -18,6 +18,9 @@
>  #include "exec/hwaddr.h"
>  #include "exec/cpu-common.h"
>  #include "system/host_iommu_device.h"
> +#ifdef CONFIG_LINUX
> +#include <linux/iommufd.h>
> +#endif
>  
>  #define TYPE_IOMMUFD_BACKEND "iommufd"
>  OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, IOMMUFD_BACKEND)
> @@ -63,4 +66,23 @@ bool iommufd_backend_get_dirty_bitmap(IOMMUFDBackend *be, uint32_t hwpt_id,
>                                        Error **errp);
>  
>  #define TYPE_HOST_IOMMU_DEVICE_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
> +
> +typedef union VendorCaps {
> +    struct iommu_hw_info_vtd vtd;
> +    struct iommu_hw_info_arm_smmuv3 smmuv3;
> +} VendorCaps;
> +
> +/**
> + * struct HostIOMMUDeviceIOMMUFDCaps - Define host IOMMU device capabilities.
> + *
> + * @type: host platform IOMMU type.
> + *
> + * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents
> + *           the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl)
> + */
> +typedef struct HostIOMMUDeviceIOMMUFDCaps {
> +    uint32_t type;
> +    uint64_t hw_caps;
> +    VendorCaps vendor_caps;
can't we store the raw data in the caps and let the vIOMMU code
interpret it via a PCIIOMMUOps callback?

If my understanding is correct this is also Nicolin's initial
suggestion, no?

Eric
> +} HostIOMMUDeviceIOMMUFDCaps;
>  #endif
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 48db105422..530cde6740 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -324,7 +324,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>       * vfio_migration_realize() may decide to use VF dirty tracking
>       * instead.
>       */
> -    if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
> +    if (vbasedev->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
>          flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>      }
>  
> @@ -475,6 +475,7 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>      int ret, devfd;
>      uint32_t ioas_id;
>      Error *err = NULL;
> +    HostIOMMUDeviceIOMMUFDCaps *caps = &vbasedev->caps;
>      const VFIOIOMMUClass *iommufd_vioc =
>          VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
>  
> @@ -505,6 +506,13 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>          goto err_alloc_ioas;
>      }
>  
> +    if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid,
> +                                         &caps->type, &caps->vendor_caps,
> +                                         sizeof(VendorCaps), &caps->hw_caps,
> +                                         errp)) {
> +        goto err_alloc_ioas;
> +    }
> +
>      /* try to attach to an existing container in this space */
>      QLIST_FOREACH(bcontainer, &space->containers, next) {
>          container = container_of(bcontainer, VFIOIOMMUFDContainer, bcontainer);

Re: [PATCH 1/5] vfio/iommufd: Save host iommu capabilities in VFIODevice.caps

[Nicolin Chen]

On Mon, May 05, 2025 at 06:38:17PM +0200, Eric Auger wrote:
> > +/**
> > + * struct HostIOMMUDeviceIOMMUFDCaps - Define host IOMMU device capabilities.
> > + *
> > + * @type: host platform IOMMU type.
> > + *
> > + * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents
> > + *           the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl)
> > + */
> > +typedef struct HostIOMMUDeviceIOMMUFDCaps {
> > +    uint32_t type;
> > +    uint64_t hw_caps;
> > +    VendorCaps vendor_caps;

> can't we store the raw data in the caps and let the vIOMMU code
> interpret it via a PCIIOMMUOps callback?
> 
> If my understanding is correct this is also Nicolin's initial
> suggestion, no?

It was, until Cedric suggested to do a further isolation between
the iommufd uAPIs/structures and vIOMMU code, so vIOMMU wouldn't
need to deal with any iommufd uAPIs/structures.

So, what Zhenzhong did is kinda creating another vIOMMU specific
iommufd driver(s) in backend/iommufd, which for now only unpacks
the hw_caps and vendor_caps, and likely will further forward the
caps via another non-iommufd structure (?) to vIOMMU.

It's slightly different than what we do in the kernel, where all
the vendor data isn't touched by the core, but still makes sense
in QEMU world I think?

Thanks
Nicolin

[PATCH 2/5] vfio: Move realize() after attach_device()

[Zhenzhong Duan]

Previously device attaching depends on realize() getting host iommu
capabilities to check dirty tracking support.

Now we save a caps copy in VFIODevice and check that copy for dirty
tracking support, there is no dependency any more, move realize()
call after attach_device() call in vfio_device_attach().

Drop vfio_device_hiod_realize() which looks redundant now.

Suggested-by: Cédric Le Goater <clg@redhat.com>
Suggested-by: Donald Dutile <ddutile@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/vfio/vfio-device.h |  1 -
 hw/vfio/container.c           |  4 ----
 hw/vfio/device.c              | 28 +++++++++++-----------------
 hw/vfio/iommufd.c             |  4 ----
 4 files changed, 11 insertions(+), 26 deletions(-)

diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
index 09a7af891a..14559733c6 100644
--- a/include/hw/vfio/vfio-device.h
+++ b/include/hw/vfio/vfio-device.h
@@ -124,7 +124,6 @@ bool vfio_device_irq_set_signaling(VFIODevice *vbasedev, int index, int subindex
 
 void vfio_device_reset_handler(void *opaque);
 bool vfio_device_is_mdev(VFIODevice *vbasedev);
-bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);
 bool vfio_device_attach(char *name, VFIODevice *vbasedev,
                         AddressSpace *as, Error **errp);
 void vfio_device_detach(VFIODevice *vbasedev);
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 23a3373470..676e88cef4 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -883,10 +883,6 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
 
     trace_vfio_device_attach(vbasedev->name, groupid);
 
-    if (!vfio_device_hiod_realize(vbasedev, errp)) {
-        return false;
-    }
-
     group = vfio_group_get(groupid, as, errp);
     if (!group) {
         return false;
diff --git a/hw/vfio/device.c b/hw/vfio/device.c
index 4de6948cf4..6154d3f443 100644
--- a/hw/vfio/device.c
+++ b/hw/vfio/device.c
@@ -347,17 +347,6 @@ bool vfio_device_is_mdev(VFIODevice *vbasedev)
     return subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
 }
 
-bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp)
-{
-    HostIOMMUDevice *hiod = vbasedev->hiod;
-
-    if (!hiod) {
-        return true;
-    }
-
-    return HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp);
-}
-
 VFIODevice *vfio_get_vfio_device(Object *obj)
 {
     if (object_dynamic_cast(obj, TYPE_VFIO_PCI)) {
@@ -372,6 +361,7 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
 {
     const VFIOIOMMUClass *ops =
         VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
+    HostIOMMUDeviceClass *hiodc;
     HostIOMMUDevice *hiod = NULL;
 
     if (vbasedev->iommufd) {
@@ -380,16 +370,20 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
 
     assert(ops);
 
+    if (!ops->attach_device(name, vbasedev, as, errp)) {
+        return false;
+    }
 
     if (!vbasedev->mdev) {
         hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
-        vbasedev->hiod = hiod;
-    }
+        hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod);
 
-    if (!ops->attach_device(name, vbasedev, as, errp)) {
-        object_unref(hiod);
-        vbasedev->hiod = NULL;
-        return false;
+        if (!hiodc->realize(hiod, vbasedev, errp)) {
+            object_unref(hiod);
+            ops->detach_device(vbasedev);
+            return false;
+        }
+        vbasedev->hiod = hiod;
     }
 
     return true;
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 530cde6740..e05b472e35 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -502,10 +502,6 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
      * FD to be connected and having a devid to be able to successfully call
      * iommufd_backend_get_device_info().
      */
-    if (!vfio_device_hiod_realize(vbasedev, errp)) {
-        goto err_alloc_ioas;
-    }
-
     if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid,
                                          &caps->type, &caps->vendor_caps,
                                          sizeof(VendorCaps), &caps->hw_caps,
-- 
2.34.1

Re: [PATCH 2/5] vfio: Move realize() after attach_device()

[Philippe Mathieu-Daudé]

Hi,

On 11/4/25 12:17, Zhenzhong Duan wrote:
> Previously device attaching depends on realize() getting host iommu
> capabilities to check dirty tracking support.
> 
> Now we save a caps copy in VFIODevice and check that copy for dirty
> tracking support, there is no dependency any more, move realize()
> call after attach_device() call in vfio_device_attach().
> 
> Drop vfio_device_hiod_realize() which looks redundant now.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Suggested-by: Donald Dutile <ddutile@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/hw/vfio/vfio-device.h |  1 -
>   hw/vfio/container.c           |  4 ----
>   hw/vfio/device.c              | 28 +++++++++++-----------------
>   hw/vfio/iommufd.c             |  4 ----
>   4 files changed, 11 insertions(+), 26 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index 09a7af891a..14559733c6 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -124,7 +124,6 @@ bool vfio_device_irq_set_signaling(VFIODevice *vbasedev, int index, int subindex
>   
>   void vfio_device_reset_handler(void *opaque);
>   bool vfio_device_is_mdev(VFIODevice *vbasedev);
> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);

Pre-existing, but can we add documentation about what vfio_device_attach
does, in particular in which state is the device once attached (or if
attachment failed)?

>   bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>                           AddressSpace *as, Error **errp);
>   void vfio_device_detach(VFIODevice *vbasedev);

RE: [PATCH 2/5] vfio: Move realize() after attach_device()

[Duan, Zhenzhong]

>-----Original Message-----
>From: Philippe Mathieu-Daudé <philmd@linaro.org>
>Subject: Re: [PATCH 2/5] vfio: Move realize() after attach_device()
>
>Hi,
>
>On 11/4/25 12:17, Zhenzhong Duan wrote:
>> Previously device attaching depends on realize() getting host iommu
>> capabilities to check dirty tracking support.
>>
>> Now we save a caps copy in VFIODevice and check that copy for dirty
>> tracking support, there is no dependency any more, move realize()
>> call after attach_device() call in vfio_device_attach().
>>
>> Drop vfio_device_hiod_realize() which looks redundant now.
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Suggested-by: Donald Dutile <ddutile@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/hw/vfio/vfio-device.h |  1 -
>>   hw/vfio/container.c           |  4 ----
>>   hw/vfio/device.c              | 28 +++++++++++-----------------
>>   hw/vfio/iommufd.c             |  4 ----
>>   4 files changed, 11 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
>> index 09a7af891a..14559733c6 100644
>> --- a/include/hw/vfio/vfio-device.h
>> +++ b/include/hw/vfio/vfio-device.h
>> @@ -124,7 +124,6 @@ bool vfio_device_irq_set_signaling(VFIODevice
>*vbasedev, int index, int subindex
>>
>>   void vfio_device_reset_handler(void *opaque);
>>   bool vfio_device_is_mdev(VFIODevice *vbasedev);
>> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);
>
>Pre-existing, but can we add documentation about what vfio_device_attach
>does, in particular in which state is the device once attached (or if
>attachment failed)?

Sure, it can be in a separate patch.

Thanks
Zhenzhong

>
>>   bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>>                           AddressSpace *as, Error **errp);
>>   void vfio_device_detach(VFIODevice *vbasedev);

Re: [PATCH 2/5] vfio: Move realize() after attach_device()

[Cédric Le Goater]

On 4/11/25 12:17, Zhenzhong Duan wrote:
> Previously device attaching depends on realize() getting host iommu
> capabilities to check dirty tracking support.
> 
> Now we save a caps copy in VFIODevice and check that copy for dirty
> tracking support, there is no dependency any more, move realize()
> call after attach_device() call in vfio_device_attach().
> 
> Drop vfio_device_hiod_realize() which looks redundant now.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Suggested-by: Donald Dutile <ddutile@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/hw/vfio/vfio-device.h |  1 -
>   hw/vfio/container.c           |  4 ----
>   hw/vfio/device.c              | 28 +++++++++++-----------------
>   hw/vfio/iommufd.c             |  4 ----
>   4 files changed, 11 insertions(+), 26 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index 09a7af891a..14559733c6 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -124,7 +124,6 @@ bool vfio_device_irq_set_signaling(VFIODevice *vbasedev, int index, int subindex
>   
>   void vfio_device_reset_handler(void *opaque);
>   bool vfio_device_is_mdev(VFIODevice *vbasedev);
> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);
>   bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>                           AddressSpace *as, Error **errp);
>   void vfio_device_detach(VFIODevice *vbasedev);
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 23a3373470..676e88cef4 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -883,10 +883,6 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
>   
>       trace_vfio_device_attach(vbasedev->name, groupid);
>   
> -    if (!vfio_device_hiod_realize(vbasedev, errp)) {
> -        return false;
> -    }
> -
>       group = vfio_group_get(groupid, as, errp);
>       if (!group) {
>           return false;
> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
> index 4de6948cf4..6154d3f443 100644
> --- a/hw/vfio/device.c
> +++ b/hw/vfio/device.c
> @@ -347,17 +347,6 @@ bool vfio_device_is_mdev(VFIODevice *vbasedev)
>       return subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
>   }
>   
> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp)
> -{
> -    HostIOMMUDevice *hiod = vbasedev->hiod;
> -
> -    if (!hiod) {
> -        return true;
> -    }
> -
> -    return HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp);
> -}
> -
>   VFIODevice *vfio_get_vfio_device(Object *obj)
>   {
>       if (object_dynamic_cast(obj, TYPE_VFIO_PCI)) {
> @@ -372,6 +361,7 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>   {
>       const VFIOIOMMUClass *ops =
>           VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
> +    HostIOMMUDeviceClass *hiodc;
>       HostIOMMUDevice *hiod = NULL;
>   
>       if (vbasedev->iommufd) {
> @@ -380,16 +370,20 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>   
>       assert(ops);
>   
> +    if (!ops->attach_device(name, vbasedev, as, errp)) {
> +        return false;
> +    }
>   
>       if (!vbasedev->mdev) {
>           hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
> -        vbasedev->hiod = hiod;
> -    }
> +        hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod);
>   
> -    if (!ops->attach_device(name, vbasedev, as, errp)) {
> -        object_unref(hiod);
> -        vbasedev->hiod = NULL;
> -        return false;
> +        if (!hiodc->realize(hiod, vbasedev, errp)) {
> +            object_unref(hiod);
> +            ops->detach_device(vbasedev);
> +            return false;
> +        }
> +        vbasedev->hiod = hiod;

This is not what I meant. I was not clear enough. Sorry about that.

hiodc->realize can be called under each container backend: legacy
and iommufd. I don't see much much value to make it common and
it would remove the unref/detach sequence to handle errors.

Thanks,

C.



>       }
>   
>       return true;
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 530cde6740..e05b472e35 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -502,10 +502,6 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>        * FD to be connected and having a devid to be able to successfully call
>        * iommufd_backend_get_device_info().
>        */
> -    if (!vfio_device_hiod_realize(vbasedev, errp)) {
> -        goto err_alloc_ioas;
> -    }
> -
>       if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid,
>                                            &caps->type, &caps->vendor_caps,
>                                            sizeof(VendorCaps), &caps->hw_caps,

RE: [PATCH 2/5] vfio: Move realize() after attach_device()

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH 2/5] vfio: Move realize() after attach_device()
>
>On 4/11/25 12:17, Zhenzhong Duan wrote:
>> Previously device attaching depends on realize() getting host iommu
>> capabilities to check dirty tracking support.
>>
>> Now we save a caps copy in VFIODevice and check that copy for dirty
>> tracking support, there is no dependency any more, move realize()
>> call after attach_device() call in vfio_device_attach().
>>
>> Drop vfio_device_hiod_realize() which looks redundant now.
>>
>> Suggested-by: Cédric Le Goater <clg@redhat.com>
>> Suggested-by: Donald Dutile <ddutile@redhat.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   include/hw/vfio/vfio-device.h |  1 -
>>   hw/vfio/container.c           |  4 ----
>>   hw/vfio/device.c              | 28 +++++++++++-----------------
>>   hw/vfio/iommufd.c             |  4 ----
>>   4 files changed, 11 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
>> index 09a7af891a..14559733c6 100644
>> --- a/include/hw/vfio/vfio-device.h
>> +++ b/include/hw/vfio/vfio-device.h
>> @@ -124,7 +124,6 @@ bool vfio_device_irq_set_signaling(VFIODevice
>*vbasedev, int index, int subindex
>>
>>   void vfio_device_reset_handler(void *opaque);
>>   bool vfio_device_is_mdev(VFIODevice *vbasedev);
>> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);
>>   bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>>                           AddressSpace *as, Error **errp);
>>   void vfio_device_detach(VFIODevice *vbasedev);
>> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
>> index 23a3373470..676e88cef4 100644
>> --- a/hw/vfio/container.c
>> +++ b/hw/vfio/container.c
>> @@ -883,10 +883,6 @@ static bool vfio_legacy_attach_device(const char
>*name, VFIODevice *vbasedev,
>>
>>       trace_vfio_device_attach(vbasedev->name, groupid);
>>
>> -    if (!vfio_device_hiod_realize(vbasedev, errp)) {
>> -        return false;
>> -    }
>> -
>>       group = vfio_group_get(groupid, as, errp);
>>       if (!group) {
>>           return false;
>> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
>> index 4de6948cf4..6154d3f443 100644
>> --- a/hw/vfio/device.c
>> +++ b/hw/vfio/device.c
>> @@ -347,17 +347,6 @@ bool vfio_device_is_mdev(VFIODevice *vbasedev)
>>       return subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
>>   }
>>
>> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp)
>> -{
>> -    HostIOMMUDevice *hiod = vbasedev->hiod;
>> -
>> -    if (!hiod) {
>> -        return true;
>> -    }
>> -
>> -    return HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev,
>errp);
>> -}
>> -
>>   VFIODevice *vfio_get_vfio_device(Object *obj)
>>   {
>>       if (object_dynamic_cast(obj, TYPE_VFIO_PCI)) {
>> @@ -372,6 +361,7 @@ bool vfio_device_attach(char *name, VFIODevice
>*vbasedev,
>>   {
>>       const VFIOIOMMUClass *ops =
>>
>VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
>> +    HostIOMMUDeviceClass *hiodc;
>>       HostIOMMUDevice *hiod = NULL;
>>
>>       if (vbasedev->iommufd) {
>> @@ -380,16 +370,20 @@ bool vfio_device_attach(char *name, VFIODevice
>*vbasedev,
>>
>>       assert(ops);
>>
>> +    if (!ops->attach_device(name, vbasedev, as, errp)) {
>> +        return false;
>> +    }
>>
>>       if (!vbasedev->mdev) {
>>           hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
>> -        vbasedev->hiod = hiod;
>> -    }
>> +        hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod);
>>
>> -    if (!ops->attach_device(name, vbasedev, as, errp)) {
>> -        object_unref(hiod);
>> -        vbasedev->hiod = NULL;
>> -        return false;
>> +        if (!hiodc->realize(hiod, vbasedev, errp)) {
>> +            object_unref(hiod);
>> +            ops->detach_device(vbasedev);
>> +            return false;
>> +        }
>> +        vbasedev->hiod = hiod;
>
>This is not what I meant. I was not clear enough. Sorry about that.
>
>hiodc->realize can be called under each container backend: legacy
>and iommufd. I don't see much much value to make it common and
>it would remove the unref/detach sequence to handle errors.

OK, I'll switch to per backend change.
I was trying to avoid duplicate change for both backend, also realize() will not fail normally and unref/detach sequence is untouched.

Thanks
Zhenzhong

Re: [PATCH 2/5] vfio: Move realize() after attach_device()

[Donald Dutile]

On 4/11/25 6:17 AM, Zhenzhong Duan wrote:
> Previously device attaching depends on realize() getting host iommu capabilities to check dirty tracking support. Now we save a caps copy in VFIODevice and check that copy for dirty tracking support, there is no dependency any more, move realize() call after attach_device() call in vfio_device_attach(). Drop vfio_device_hiod_realize() which looks redundant now. Suggested-by: Cédric Le Goater <clg@redhat.com> Suggested-by: Donald Dutile <ddutile@redhat.com> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Reviewed-by: Donald Dutile <ddutile@redhat.com>

[PATCH 3/5] vfio/iommufd: Implement .get_cap() in TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO sub-class

[Zhenzhong Duan]

Now we have saved a copy of host iommu capabilities in VFIODevice, implemented
hiod_iommufd_vfio_get_cap() by querying the caps copy in sub-class. This
overrides .get_cap() implementation hiod_iommufd_vfio_get_cap() in
TYPE_HOST_IOMMU_DEVICE_IOMMUFD parent class.

Vendor caps are checked for a specific capability, e.g., for vtd, checking
code will be in hiod_iommufd_get_vtd_cap().

This also fixes an issue that calling vfio_device_get_aw_bits() in
TYPE_HOST_IOMMU_DEVICE_IOMMUFD parent class .get_cap().

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/system/iommufd.h |  4 ++++
 backends/iommufd.c       | 40 ++++++++++++++++++++++++++++++++++++++++
 hw/vfio/iommufd.c        | 16 ++++++++++++++++
 3 files changed, 60 insertions(+)

diff --git a/include/system/iommufd.h b/include/system/iommufd.h
index 0f337585c9..baba5ec1d8 100644
--- a/include/system/iommufd.h
+++ b/include/system/iommufd.h
@@ -85,4 +85,8 @@ typedef struct HostIOMMUDeviceIOMMUFDCaps {
     uint64_t hw_caps;
     VendorCaps vendor_caps;
 } HostIOMMUDeviceIOMMUFDCaps;
+
+int hiod_iommufd_get_common_cap(HostIOMMUDevice *hiod,
+                                HostIOMMUDeviceIOMMUFDCaps *caps,
+                                int cap, Error **errp);
 #endif
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 9587e4d99b..54fa3174d0 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -355,3 +355,43 @@ static const TypeInfo types[] = {
 };
 
 DEFINE_TYPES(types)
+
+static int hiod_iommufd_get_vtd_cap(HostIOMMUDevice *hiod,
+                                    struct iommu_hw_info_vtd *vtd,
+                                    int cap, Error **errp)
+{
+    /* TODO: Check vtd->cap_reg/ecap_reg for capability */
+    error_setg(errp, "%s: unsupported capability %x", hiod->name, cap);
+    return -EINVAL;
+}
+
+static int hiod_iommufd_get_vendor_cap(HostIOMMUDevice *hiod,
+                                       HostIOMMUDeviceIOMMUFDCaps *caps,
+                                       int cap, Error **errp)
+{
+    enum iommu_hw_info_type type = caps->type;
+
+    switch (type) {
+    case IOMMU_HW_INFO_TYPE_INTEL_VTD:
+        return hiod_iommufd_get_vtd_cap(hiod, &caps->vendor_caps.vtd,
+                                        cap, errp);
+    case IOMMU_HW_INFO_TYPE_ARM_SMMUV3:
+    case IOMMU_HW_INFO_TYPE_NONE:
+        break;
+    }
+
+    error_setg(errp, "%s: unsupported capability type %x", hiod->name, type);
+    return -EINVAL;
+}
+
+int hiod_iommufd_get_common_cap(HostIOMMUDevice *hiod,
+                                HostIOMMUDeviceIOMMUFDCaps *caps,
+                                int cap, Error **errp)
+{
+    switch (cap) {
+    case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE:
+        return caps->type;
+    default:
+        return hiod_iommufd_get_vendor_cap(hiod, caps, cap, errp);
+    }
+}
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index e05b472e35..e7ca92f81f 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -833,6 +833,21 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
     return true;
 }
 
+static int hiod_iommufd_vfio_get_cap(HostIOMMUDevice *hiod, int cap,
+                                     Error **errp)
+{
+    VFIODevice *vdev = hiod->agent;
+    HostIOMMUDeviceIOMMUFDCaps *caps = &vdev->caps;
+
+    /* VFIO has its own way to get aw_bits which may be different from VDPA */
+    switch (cap) {
+    case HOST_IOMMU_DEVICE_CAP_AW_BITS:
+        return vfio_device_get_aw_bits(hiod->agent);
+    default:
+        return hiod_iommufd_get_common_cap(hiod, caps, cap, errp);
+    }
+}
+
 static GList *
 hiod_iommufd_vfio_get_iova_ranges(HostIOMMUDevice *hiod)
 {
@@ -857,6 +872,7 @@ static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data)
     HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc);
 
     hiodc->realize = hiod_iommufd_vfio_realize;
+    hiodc->get_cap = hiod_iommufd_vfio_get_cap;
     hiodc->get_iova_ranges = hiod_iommufd_vfio_get_iova_ranges;
     hiodc->get_page_size_mask = hiod_iommufd_vfio_get_page_size_mask;
 };
-- 
2.34.1

RE: [PATCH 3/5] vfio/iommufd: Implement .get_cap() in TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO sub-class

[Duan, Zhenzhong]

>-----Original Message-----
>From: Nicolin Chen <nicolinc@nvidia.com>
>Subject: Re: [PATCH 3/5] vfio/iommufd: Implement .get_cap() in
>TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO sub-class
>
>On Fri, Apr 11, 2025 at 06:17:05PM +0800, Zhenzhong Duan wrote:
>> +static int hiod_iommufd_get_vtd_cap(HostIOMMUDevice *hiod,
>> +                                    struct iommu_hw_info_vtd *vtd,
>> +                                    int cap, Error **errp)
>> +{
>> +    /* TODO: Check vtd->cap_reg/ecap_reg for capability */
>> +    error_setg(errp, "%s: unsupported capability %x", hiod->name, cap);
>> +    return -EINVAL;
>> +}
>> +
>> +static int hiod_iommufd_get_vendor_cap(HostIOMMUDevice *hiod,
>> +                                       HostIOMMUDeviceIOMMUFDCaps *caps,
>> +                                       int cap, Error **errp)
>> +{
>> +    enum iommu_hw_info_type type = caps->type;
>> +
>> +    switch (type) {
>> +    case IOMMU_HW_INFO_TYPE_INTEL_VTD:
>> +        return hiod_iommufd_get_vtd_cap(hiod, &caps->vendor_caps.vtd,
>> +                                        cap, errp);
>> +    case IOMMU_HW_INFO_TYPE_ARM_SMMUV3:
>> +    case IOMMU_HW_INFO_TYPE_NONE:
>
>Do we have to keep vendor types in the core?

Either HostIOMMUDevice or VFIODevice, or you have other suggestion?

>
>Can we have another PCI callback like the set_iommu/unset_iommu
>ops to ask vendor code get the cap and return it?

As you said, vendor code can't generate all caps i.e. PASID, any benefit for a new callback?

Thanks
Zhenzhong 

[PATCH 4/5] backends/iommufd: Drop hiod_iommufd_get_cap()

[Zhenzhong Duan]

Because sub-class TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO has it's own
implementation of .get_cap(), hiod_iommufd_get_cap() isn't used
any more, drop it.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 backends/iommufd.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/backends/iommufd.c b/backends/iommufd.c
index 54fa3174d0..d2ecdc9c82 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -311,28 +311,6 @@ bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
     return true;
 }
 
-static int hiod_iommufd_get_cap(HostIOMMUDevice *hiod, int cap, Error **errp)
-{
-    HostIOMMUDeviceCaps *caps = &hiod->caps;
-
-    switch (cap) {
-    case HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE:
-        return caps->type;
-    case HOST_IOMMU_DEVICE_CAP_AW_BITS:
-        return vfio_device_get_aw_bits(hiod->agent);
-    default:
-        error_setg(errp, "%s: unsupported capability %x", hiod->name, cap);
-        return -EINVAL;
-    }
-}
-
-static void hiod_iommufd_class_init(ObjectClass *oc, void *data)
-{
-    HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
-
-    hioc->get_cap = hiod_iommufd_get_cap;
-};
-
 static const TypeInfo types[] = {
     {
         .name = TYPE_IOMMUFD_BACKEND,
@@ -349,7 +327,6 @@ static const TypeInfo types[] = {
     }, {
         .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD,
         .parent = TYPE_HOST_IOMMU_DEVICE,
-        .class_init = hiod_iommufd_class_init,
         .abstract = true,
     }
 };
-- 
2.34.1

[PATCH 5/5] vfio/iommufd: Drop HostIOMMUDeviceCaps from HostIOMMUDevice

[Zhenzhong Duan]

Because hiod_iommufd_get_cap() was dropped, HostIOMMUDeviceCaps is not
useful any more, drop it.

This also hides HostIOMMUDeviceCaps from vIOMMU so the only way to check
cap is through .get_cap() interface. This makes HostIOMMUDevice exposing
data to vIOMMU as small as possible.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/system/host_iommu_device.h | 14 --------------
 hw/vfio/iommufd.c                  | 15 ---------------
 2 files changed, 29 deletions(-)

diff --git a/include/system/host_iommu_device.h b/include/system/host_iommu_device.h
index 809cced4ba..6f10bea25f 100644
--- a/include/system/host_iommu_device.h
+++ b/include/system/host_iommu_device.h
@@ -15,19 +15,6 @@
 #include "qom/object.h"
 #include "qapi/error.h"
 
-/**
- * struct HostIOMMUDeviceCaps - Define host IOMMU device capabilities.
- *
- * @type: host platform IOMMU type.
- *
- * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents
- *           the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl)
- */
-typedef struct HostIOMMUDeviceCaps {
-    uint32_t type;
-    uint64_t hw_caps;
-} HostIOMMUDeviceCaps;
-
 #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
 OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUDeviceClass, HOST_IOMMU_DEVICE)
 
@@ -38,7 +25,6 @@ struct HostIOMMUDevice {
     void *agent; /* pointer to agent device, ie. VFIO or VDPA device */
     PCIBus *aliased_bus;
     int aliased_devfn;
-    HostIOMMUDeviceCaps caps;
 };
 
 /**
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index e7ca92f81f..947c5456d8 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -811,24 +811,9 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
                                       Error **errp)
 {
     VFIODevice *vdev = opaque;
-    HostIOMMUDeviceCaps *caps = &hiod->caps;
-    enum iommu_hw_info_type type;
-    union {
-        struct iommu_hw_info_vtd vtd;
-    } data;
-    uint64_t hw_caps;
 
     hiod->agent = opaque;
-
-    if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid,
-                                         &type, &data, sizeof(data),
-                                         &hw_caps, errp)) {
-        return false;
-    }
-
     hiod->name = g_strdup(vdev->name);
-    caps->type = type;
-    caps->hw_caps = hw_caps;
 
     return true;
 }
-- 
2.34.1