[v2] VFIO and IOMMU prerequisite stuff for IOMMU nesting support

[PATCH v2 4/4] vfio/iommufd: Save vendor specific device info

Posted by Zhenzhong Duan 5 months, 2 weeks ago

Some device information returned by ioctl(IOMMU_GET_HW_INFO) are vendor
specific. Save them as raw data in a union supporting different vendors,
then vendor IOMMU can query the raw data with its fixed format for
capability directly.

Because IOMMU_GET_HW_INFO is only supported in linux, so declare those
capability related structures with CONFIG_LINUX.

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/system/host_iommu_device.h | 11 +++++++++++
 hw/vfio/iommufd.c                  |  8 +++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/include/system/host_iommu_device.h b/include/system/host_iommu_device.h
index 809cced4ba..10fccc10be 100644
--- a/include/system/host_iommu_device.h
+++ b/include/system/host_iommu_device.h
@@ -14,6 +14,13 @@
 
 #include "qom/object.h"
 #include "qapi/error.h"
+#ifdef CONFIG_LINUX
+#include "linux/iommufd.h"
+
+typedef union VendorCaps {
+    struct iommu_hw_info_vtd vtd;
+    struct iommu_hw_info_arm_smmuv3 smmuv3;
+} VendorCaps;
 
 /**
  * struct HostIOMMUDeviceCaps - Define host IOMMU device capabilities.
@@ -26,7 +33,9 @@
 typedef struct HostIOMMUDeviceCaps {
     uint32_t type;
     uint64_t hw_caps;
+    VendorCaps vendor_caps;
 } HostIOMMUDeviceCaps;
+#endif
 
 #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
 OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUDeviceClass, HOST_IOMMU_DEVICE)
@@ -38,7 +47,9 @@ struct HostIOMMUDevice {
     void *agent; /* pointer to agent device, ie. VFIO or VDPA device */
     PCIBus *aliased_bus;
     int aliased_devfn;
+#ifdef CONFIG_LINUX
     HostIOMMUDeviceCaps caps;
+#endif
 };
 
 /**
diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index d661737c17..fbf47cab09 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -834,16 +834,14 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
     VFIODevice *vdev = opaque;
     HostIOMMUDeviceIOMMUFD *idev;
     HostIOMMUDeviceCaps *caps = &hiod->caps;
+    VendorCaps *vendor_caps = &caps->vendor_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),
+    if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid, &type,
+                                         vendor_caps, sizeof(*vendor_caps),
                                          &hw_caps, errp)) {
         return false;
     }
-- 
2.34.1

Re: [PATCH v2 4/4] vfio/iommufd: Save vendor specific device info

Posted by Eric Auger 5 months, 2 weeks ago

Hi Zhenzhong,

On 5/30/25 11:35 AM, Zhenzhong Duan wrote:
> Some device information returned by ioctl(IOMMU_GET_HW_INFO) are vendor
> specific. Save them as raw data in a union supporting different vendors,
> then vendor IOMMU can query the raw data with its fixed format for
> capability directly.
>
> Because IOMMU_GET_HW_INFO is only supported in linux, so declare those
> capability related structures with CONFIG_LINUX.
>
> 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/system/host_iommu_device.h | 11 +++++++++++
>  hw/vfio/iommufd.c                  |  8 +++-----
>  2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/include/system/host_iommu_device.h b/include/system/host_iommu_device.h
> index 809cced4ba..10fccc10be 100644
> --- a/include/system/host_iommu_device.h
> +++ b/include/system/host_iommu_device.h
> @@ -14,6 +14,13 @@
>  
>  #include "qom/object.h"
>  #include "qapi/error.h"
> +#ifdef CONFIG_LINUX
> +#include "linux/iommufd.h"
> +
> +typedef union VendorCaps {
> +    struct iommu_hw_info_vtd vtd;
> +    struct iommu_hw_info_arm_smmuv3 smmuv3;
> +} VendorCaps;
>  
>  /**
>   * struct HostIOMMUDeviceCaps - Define host IOMMU device capabilities.
> @@ -26,7 +33,9 @@
>  typedef struct HostIOMMUDeviceCaps {
>      uint32_t type;
>      uint64_t hw_caps;
> +    VendorCaps vendor_caps;
missing the doc comment update for new field vendor_caps

Otherwise

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

Thanks

Eric


>  } HostIOMMUDeviceCaps;
> +#endif
>  
>  #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
>  OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUDeviceClass, HOST_IOMMU_DEVICE)
> @@ -38,7 +47,9 @@ struct HostIOMMUDevice {
>      void *agent; /* pointer to agent device, ie. VFIO or VDPA device */
>      PCIBus *aliased_bus;
>      int aliased_devfn;
> +#ifdef CONFIG_LINUX
>      HostIOMMUDeviceCaps caps;
> +#endif
>  };
>  
>  /**
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index d661737c17..fbf47cab09 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -834,16 +834,14 @@ static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>      VFIODevice *vdev = opaque;
>      HostIOMMUDeviceIOMMUFD *idev;
>      HostIOMMUDeviceCaps *caps = &hiod->caps;
> +    VendorCaps *vendor_caps = &caps->vendor_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),
> +    if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid, &type,
> +                                         vendor_caps, sizeof(*vendor_caps),
>                                           &hw_caps, errp)) {
>          return false;
>      }

RE: [PATCH v2 4/4] vfio/iommufd: Save vendor specific device info

Posted by Duan, Zhenzhong 5 months, 2 weeks ago

Hi Eric,

>-----Original Message-----
>From: Eric Auger <eric.auger@redhat.com>
>Subject: Re: [PATCH v2 4/4] vfio/iommufd: Save vendor specific device info
>
>Hi Zhenzhong,
>
>On 5/30/25 11:35 AM, Zhenzhong Duan wrote:
>> Some device information returned by ioctl(IOMMU_GET_HW_INFO) are vendor
>> specific. Save them as raw data in a union supporting different vendors,
>> then vendor IOMMU can query the raw data with its fixed format for
>> capability directly.
>>
>> Because IOMMU_GET_HW_INFO is only supported in linux, so declare those
>> capability related structures with CONFIG_LINUX.
>>
>> 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/system/host_iommu_device.h | 11 +++++++++++
>>  hw/vfio/iommufd.c                  |  8 +++-----
>>  2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/system/host_iommu_device.h
>b/include/system/host_iommu_device.h
>> index 809cced4ba..10fccc10be 100644
>> --- a/include/system/host_iommu_device.h
>> +++ b/include/system/host_iommu_device.h
>> @@ -14,6 +14,13 @@
>>
>>  #include "qom/object.h"
>>  #include "qapi/error.h"
>> +#ifdef CONFIG_LINUX
>> +#include "linux/iommufd.h"
>> +
>> +typedef union VendorCaps {
>> +    struct iommu_hw_info_vtd vtd;
>> +    struct iommu_hw_info_arm_smmuv3 smmuv3;
>> +} VendorCaps;
>>
>>  /**
>>   * struct HostIOMMUDeviceCaps - Define host IOMMU device capabilities.
>> @@ -26,7 +33,9 @@
>>  typedef struct HostIOMMUDeviceCaps {
>>      uint32_t type;
>>      uint64_t hw_caps;
>> +    VendorCaps vendor_caps;
>missing the doc comment update for new field vendor_caps

Good catch, will add. Thanks

Zhenzhong

>
>Otherwise
>
>Reviewed-by: Eric Auger <eric.auger@redhat.com>
>
>Thanks
>
>Eric
>
>
>>  } HostIOMMUDeviceCaps;
>> +#endif
>>
>>  #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
>>  OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUDeviceClass,
>HOST_IOMMU_DEVICE)
>> @@ -38,7 +47,9 @@ struct HostIOMMUDevice {
>>      void *agent; /* pointer to agent device, ie. VFIO or VDPA device */
>>      PCIBus *aliased_bus;
>>      int aliased_devfn;
>> +#ifdef CONFIG_LINUX
>>      HostIOMMUDeviceCaps caps;
>> +#endif
>>  };
>>
>>  /**
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index d661737c17..fbf47cab09 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -834,16 +834,14 @@ static bool
>hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque,
>>      VFIODevice *vdev = opaque;
>>      HostIOMMUDeviceIOMMUFD *idev;
>>      HostIOMMUDeviceCaps *caps = &hiod->caps;
>> +    VendorCaps *vendor_caps = &caps->vendor_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),
>> +    if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid, &type,
>> +                                         vendor_caps, sizeof(*vendor_caps),
>>                                           &hw_caps, errp)) {
>>          return false;
>>      }

Re: [PATCH v2 4/4] vfio/iommufd: Save vendor specific device info

Posted by Nicolin Chen 5 months, 2 weeks ago

On Fri, May 30, 2025 at 05:35:12PM +0800, Zhenzhong Duan wrote:
> Some device information returned by ioctl(IOMMU_GET_HW_INFO) are vendor
> specific. Save them as raw data in a union supporting different vendors,
> then vendor IOMMU can query the raw data with its fixed format for
> capability directly.
> 
> Because IOMMU_GET_HW_INFO is only supported in linux, so declare those
> capability related structures with CONFIG_LINUX.
> 
> Suggested-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

I like this that we eventually moved all vendor stuff back to the
vendor vIOMMU code.

> +typedef union VendorCaps {
> +    struct iommu_hw_info_vtd vtd;
> +    struct iommu_hw_info_arm_smmuv3 smmuv3;
> +} VendorCaps;

Nit: can it be just:

typedef union {
	...
} VendorCaps; 

?

>  typedef struct HostIOMMUDeviceCaps {
>      uint32_t type;
>      uint64_t hw_caps;
> +    VendorCaps vendor_caps;
>  } HostIOMMUDeviceCaps;

Ditto.

RE: [PATCH v2 4/4] vfio/iommufd: Save vendor specific device info

Posted by Duan, Zhenzhong 5 months, 2 weeks ago


>-----Original Message-----
>From: Nicolin Chen <nicolinc@nvidia.com>
>Subject: Re: [PATCH v2 4/4] vfio/iommufd: Save vendor specific device info
>
>On Fri, May 30, 2025 at 05:35:12PM +0800, Zhenzhong Duan wrote:
>> Some device information returned by ioctl(IOMMU_GET_HW_INFO) are vendor
>> specific. Save them as raw data in a union supporting different vendors,
>> then vendor IOMMU can query the raw data with its fixed format for
>> capability directly.
>>
>> Because IOMMU_GET_HW_INFO is only supported in linux, so declare those
>> capability related structures with CONFIG_LINUX.
>>
>> Suggested-by: Eric Auger <eric.auger@redhat.com>
>> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
>Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>
>I like this that we eventually moved all vendor stuff back to the
>vendor vIOMMU code.
>
>> +typedef union VendorCaps {
>> +    struct iommu_hw_info_vtd vtd;
>> +    struct iommu_hw_info_arm_smmuv3 smmuv3;
>> +} VendorCaps;
>
>Nit: can it be just:
>
>typedef union {
>	...
>} VendorCaps;
>
>?

Any special reason?
I didn't see anonymous union is preferred over named union in docs/devel/style.rst

Thanks
Zhenzhong

>
>>  typedef struct HostIOMMUDeviceCaps {
>>      uint32_t type;
>>      uint64_t hw_caps;
>> +    VendorCaps vendor_caps;
>>  } HostIOMMUDeviceCaps;
>
>Ditto.