[PATCH v6 18/22] iommufd: Introduce a helper function to extract vendor capabilities

Zhenzhong Duan posted 22 patches 1 month, 3 weeks ago
Maintainers: Yi Liu <yi.l.liu@intel.com>, Eric Auger <eric.auger@redhat.com>, Zhenzhong Duan <zhenzhong.duan@intel.com>, "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, "Clément Mathieu--Drif" <clement.mathieu--drif@eviden.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Alex Williamson <alex.williamson@redhat.com>, "Cédric Le Goater" <clg@redhat.com>, Fabiano Rosas <farosas@suse.de>, Laurent Vivier <lvivier@redhat.com>
There is a newer version of this series
[PATCH v6 18/22] iommufd: Introduce a helper function to extract vendor capabilities
Posted by Zhenzhong Duan 1 month, 3 weeks ago
In VFIO core, we call iommufd_backend_get_device_info() to return vendor
specific hardware information data, but it's not good to extract this raw
data in VFIO core.

Introduce host_iommu_extract_vendor_caps() to help extracting the raw
data and return a bitmap in iommufd.c because it's the place defining
iommufd_backend_get_device_info().

The other choice is to put vendor data extracting code in vendor vIOMMU
emulation file, but that will make those files mixed with vIOMMU
emulation and host IOMMU extracting code, also need a new callback in
PCIIOMMUOps. So we choose a simpler way as above.

Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/iommu.h                 |  5 +++++
 include/system/host_iommu_device.h | 16 ++++++++++++++++
 backends/iommufd.c                 | 13 +++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/include/hw/iommu.h b/include/hw/iommu.h
index 65d652950a..9b343e64b0 100644
--- a/include/hw/iommu.h
+++ b/include/hw/iommu.h
@@ -16,4 +16,9 @@ enum {
      VIOMMU_FLAG_WANT_NESTING_PARENT = BIT_ULL(0),
 };
 
+enum {
+    /* Nesting parent HWPT shouldn't have readonly mapping, due to errata */
+     IOMMU_HW_NESTING_PARENT_BYPASS_RO = BIT_ULL(0),
+};
+
 #endif /* HW_IOMMU_H */
diff --git a/include/system/host_iommu_device.h b/include/system/host_iommu_device.h
index ab849a4a82..41c9159605 100644
--- a/include/system/host_iommu_device.h
+++ b/include/system/host_iommu_device.h
@@ -39,6 +39,22 @@ typedef struct HostIOMMUDeviceCaps {
     uint64_t hw_caps;
     VendorCaps vendor_caps;
 } HostIOMMUDeviceCaps;
+
+/**
+ * host_iommu_extract_vendor_caps: Extract vendor capabilities
+ *
+ * This function converts @type specific hardware information data
+ * into a standard bitmap format.
+ *
+ * @type: IOMMU Hardware Info Types
+ *
+ * @VendorCaps: IOMMU @type specific hardware information data
+ *
+ * Returns: 64bit bitmap with each bit represents a capability of host
+ * IOMMU that we want to expose. See IOMMU_HW_* in include/hw/iommu.h
+ * for all possible capabilities currently exposed.
+ */
+uint64_t host_iommu_extract_vendor_caps(uint32_t type, VendorCaps *caps);
 #endif
 
 #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device"
diff --git a/backends/iommufd.c b/backends/iommufd.c
index 2a33c7ab0b..0bb1ed40d3 100644
--- a/backends/iommufd.c
+++ b/backends/iommufd.c
@@ -19,6 +19,7 @@
 #include "migration/cpr.h"
 #include "monitor/monitor.h"
 #include "trace.h"
+#include "hw/iommu.h"
 #include "hw/vfio/vfio-device.h"
 #include <sys/ioctl.h>
 #include <linux/iommufd.h>
@@ -410,6 +411,18 @@ bool iommufd_backend_get_device_info(IOMMUFDBackend *be, uint32_t devid,
     return true;
 }
 
+uint64_t host_iommu_extract_vendor_caps(uint32_t type, VendorCaps *caps)
+{
+    uint64_t vendor_caps = 0;
+
+    if (type == IOMMU_HW_INFO_TYPE_INTEL_VTD &&
+        caps->vtd.flags & IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17) {
+        vendor_caps |= IOMMU_HW_NESTING_PARENT_BYPASS_RO;
+    }
+
+    return vendor_caps;
+}
+
 bool iommufd_backend_invalidate_cache(IOMMUFDBackend *be, uint32_t id,
                                       uint32_t data_type, uint32_t entry_len,
                                       uint32_t *entry_num, void *data,
-- 
2.47.1
Re: [PATCH v6 18/22] iommufd: Introduce a helper function to extract vendor capabilities
Posted by Nicolin Chen 1 month, 3 weeks ago
On Thu, Sep 18, 2025 at 04:57:57AM -0400, Zhenzhong Duan wrote:
> In VFIO core, we call iommufd_backend_get_device_info() to return vendor
> specific hardware information data, but it's not good to extract this raw
> data in VFIO core.
> 
> Introduce host_iommu_extract_vendor_caps() to help extracting the raw
> data and return a bitmap in iommufd.c because it's the place defining
> iommufd_backend_get_device_info().
> 
> The other choice is to put vendor data extracting code in vendor vIOMMU
> emulation file, but that will make those files mixed with vIOMMU
> emulation and host IOMMU extracting code, also need a new callback in
> PCIIOMMUOps. So we choose a simpler way as above.
> 
> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

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

With some nits:

> +enum {
> +    /* Nesting parent HWPT shouldn't have readonly mapping, due to errata */
> +     IOMMU_HW_NESTING_PARENT_BYPASS_RO = BIT_ULL(0),
> +};

I would put a name here too. And given this is defined generically:

/* Host IOMMU quirks. Extracted from host IOMMU capabilities */
enum host_iommu_quirks {
	HOST_IOMMU_QUIRK_NESTING_PARENT_BYPASS_RO = BIT_ULL(0),
};

> +/**
> + * host_iommu_extract_vendor_caps: Extract vendor capabilities

Then:

 * host_iommu_extract_quirks: Extract host IOMMU quirks

> + * This function converts @type specific hardware information data
> + * into a standard bitmap format.
> + *
> + * @type: IOMMU Hardware Info Types
> + *
> + * @VendorCaps: IOMMU @type specific hardware information data
> + *
> + * Returns: 64bit bitmap with each bit represents a capability of host
> + * IOMMU that we want to expose. See IOMMU_HW_* in include/hw/iommu.h
> + * for all possible capabilities currently exposed.

And simplify this:

 * Returns: bitmap with each representing a host IOMMU quirk defined in
 * enum host_iommu_quirks

> +uint64_t host_iommu_extract_vendor_caps(uint32_t type, VendorCaps *caps)
> +{
> +    uint64_t vendor_caps = 0;
> +
> +    if (type == IOMMU_HW_INFO_TYPE_INTEL_VTD &&
> +        caps->vtd.flags & IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17) {
> +        vendor_caps |= IOMMU_HW_NESTING_PARENT_BYPASS_RO;
> +    }
> +
> +    return vendor_caps;
> +}

uint64_t host_iommu_extract_quirks(enum iommu_hw_info_type, VendorCaps *caps)
{
    uint64_t quirks = 0;

#if defined(CONFIG_VTD)
    if (type == IOMMU_HW_INFO_TYPE_INTEL_VTD) {
        if (caps->vtd.flags & IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17) {
            quirks |= HOST_IOMMU_QUIRK_NESTING_PARENT_BYPASS_RO;
        }
    }
#endif

    return quirks;
}

Thanks
Nicolin
RE: [PATCH v6 18/22] iommufd: Introduce a helper function to extract vendor capabilities
Posted by Duan, Zhenzhong 1 month, 3 weeks ago

>-----Original Message-----
>From: Nicolin Chen <nicolinc@nvidia.com>
>Subject: Re: [PATCH v6 18/22] iommufd: Introduce a helper function to
>extract vendor capabilities
>
>On Thu, Sep 18, 2025 at 04:57:57AM -0400, Zhenzhong Duan wrote:
>> In VFIO core, we call iommufd_backend_get_device_info() to return vendor
>> specific hardware information data, but it's not good to extract this raw
>> data in VFIO core.
>>
>> Introduce host_iommu_extract_vendor_caps() to help extracting the raw
>> data and return a bitmap in iommufd.c because it's the place defining
>> iommufd_backend_get_device_info().
>>
>> The other choice is to put vendor data extracting code in vendor vIOMMU
>> emulation file, but that will make those files mixed with vIOMMU
>> emulation and host IOMMU extracting code, also need a new callback in
>> PCIIOMMUOps. So we choose a simpler way as above.
>>
>> Suggested-by: Nicolin Chen <nicolinc@nvidia.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
>Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>
>With some nits:
>
>> +enum {
>> +    /* Nesting parent HWPT shouldn't have readonly mapping, due to
>errata */
>> +     IOMMU_HW_NESTING_PARENT_BYPASS_RO = BIT_ULL(0),
>> +};
>
>I would put a name here too. And given this is defined generically:
>
>/* Host IOMMU quirks. Extracted from host IOMMU capabilities */
>enum host_iommu_quirks {
>	HOST_IOMMU_QUIRK_NESTING_PARENT_BYPASS_RO = BIT_ULL(0),
>};
>
>> +/**
>> + * host_iommu_extract_vendor_caps: Extract vendor capabilities
>
>Then:
>
> * host_iommu_extract_quirks: Extract host IOMMU quirks
>
>> + * This function converts @type specific hardware information data
>> + * into a standard bitmap format.
>> + *
>> + * @type: IOMMU Hardware Info Types
>> + *
>> + * @VendorCaps: IOMMU @type specific hardware information data
>> + *
>> + * Returns: 64bit bitmap with each bit represents a capability of host
>> + * IOMMU that we want to expose. See IOMMU_HW_* in
>include/hw/iommu.h
>> + * for all possible capabilities currently exposed.
>
>And simplify this:
>
> * Returns: bitmap with each representing a host IOMMU quirk defined in
> * enum host_iommu_quirks
>
>> +uint64_t host_iommu_extract_vendor_caps(uint32_t type, VendorCaps
>*caps)
>> +{
>> +    uint64_t vendor_caps = 0;
>> +
>> +    if (type == IOMMU_HW_INFO_TYPE_INTEL_VTD &&
>> +        caps->vtd.flags &
>IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17) {
>> +        vendor_caps |= IOMMU_HW_NESTING_PARENT_BYPASS_RO;
>> +    }
>> +
>> +    return vendor_caps;
>> +}
>
>uint64_t host_iommu_extract_quirks(enum iommu_hw_info_type,
>VendorCaps *caps)
>{
>    uint64_t quirks = 0;
>
>#if defined(CONFIG_VTD)

I have applied all suggested change except CONFIG_VTD here as it's a device config and iommufd.c is device agnostic, it doesn't recognize CONFIG_VTD.

../backends/iommufd.c:419:13: error: attempt to use poisoned "CONFIG_VTD"

I thought this is trivial and OK for not having CONFIG_VTD?

Thanks
Zhenzhong

>    if (type == IOMMU_HW_INFO_TYPE_INTEL_VTD) {
>        if (caps->vtd.flags &
>IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17) {
>            quirks |=
>HOST_IOMMU_QUIRK_NESTING_PARENT_BYPASS_RO;
>        }
>    }
>#endif
>
>    return quirks;
>}
>
>Thanks
>Nicolin
Re: [PATCH v6 18/22] iommufd: Introduce a helper function to extract vendor capabilities
Posted by Nicolin Chen 1 month, 3 weeks ago
On Wed, Sep 24, 2025 at 08:05:36AM +0000, Duan, Zhenzhong wrote:
> >uint64_t host_iommu_extract_quirks(enum iommu_hw_info_type,
> >VendorCaps *caps)
> >{
> >    uint64_t quirks = 0;
> >
> >#if defined(CONFIG_VTD)
> 
> I have applied all suggested change except CONFIG_VTD here as it's a device config and iommufd.c is device agnostic, it doesn't recognize CONFIG_VTD.
> 
> ../backends/iommufd.c:419:13: error: attempt to use poisoned "CONFIG_VTD"
> 
> I thought this is trivial and OK for not having CONFIG_VTD?

Hmm.. I didn't expect that. It seems that QEMU does encourage
moving all vendor specific code to vendor specific file :-/

Anyway, I think it's fine to drop the ifdef. The VTD type and
cap structure are defined in the shared uAPI header.

Nicolin
RE: [PATCH v6 18/22] iommufd: Introduce a helper function to extract vendor capabilities
Posted by Duan, Zhenzhong 1 month, 2 weeks ago

>-----Original Message-----
>From: Nicolin Chen <nicolinc@nvidia.com>
>Subject: Re: [PATCH v6 18/22] iommufd: Introduce a helper function to
>extract vendor capabilities
>
>On Wed, Sep 24, 2025 at 08:05:36AM +0000, Duan, Zhenzhong wrote:
>> >uint64_t host_iommu_extract_quirks(enum iommu_hw_info_type,
>> >VendorCaps *caps)
>> >{
>> >    uint64_t quirks = 0;
>> >
>> >#if defined(CONFIG_VTD)
>>
>> I have applied all suggested change except CONFIG_VTD here as it's a
>device config and iommufd.c is device agnostic, it doesn't recognize
>CONFIG_VTD.
>>
>> ../backends/iommufd.c:419:13: error: attempt to use poisoned
>"CONFIG_VTD"
>>
>> I thought this is trivial and OK for not having CONFIG_VTD?
>
>Hmm.. I didn't expect that. It seems that QEMU does encourage
>moving all vendor specific code to vendor specific file :-/

This make me think CONFIG_VTD should not be used here, it controls
intel_iommu emulation, we may have a custom virtual machine without
virtual intel_iommu but still need to check host IOMMU.

If we do want a control, maybe kind of CONFIG_HOST_VTD rather than CONFIG_VTD.

>
>Anyway, I think it's fine to drop the ifdef. The VTD type and
>cap structure are defined in the shared uAPI header.

Agree.

Thanks
Zhenzhong