diff mbox series

[v5,19/19] intel_iommu: Check compatibility with host IOMMU capabilities

Message ID 20240508090354.1815561-20-zhenzhong.duan@intel.com (mailing list archive)
State New, archived
Headers show
Series Add a host IOMMU device abstraction to check with vIOMMU | expand

Commit Message

Duan, Zhenzhong May 8, 2024, 9:03 a.m. UTC 
If check fails, host device (either VFIO or VDPA device) is not
compatible with current vIOMMU config and should not be passed to
guest.

Only aw_bits is checked for now, we don't care other capabilities
before scalable modern mode is introduced.

Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 hw/i386/intel_iommu.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Comments

Cédric Le Goater May 27, 2024, 1:18 p.m. UTC | #1 
On 5/8/24 11:03, Zhenzhong Duan wrote:
> If check fails, host device (either VFIO or VDPA device) is not
> compatible with current vIOMMU config and should not be passed to
> guest.
> 
> Only aw_bits is checked for now, we don't care other capabilities
> before scalable modern mode is introduced.
> 
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   hw/i386/intel_iommu.c | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 747c988bc4..07bfd4f99e 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -20,6 +20,7 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include CONFIG_DEVICES /* CONFIG_HOST_IOMMU_DEVICE */
>   #include "qemu/error-report.h"
>   #include "qemu/main-loop.h"
>   #include "qapi/error.h"
> @@ -3819,6 +3820,26 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>       return vtd_dev_as;
>   }
>   
> +static bool vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice *vtd_hdev,
> +                           Error **errp)
> +{
> +#ifdef CONFIG_HOST_IOMMU_DEVICE
> +    HostIOMMUDevice *hiod = vtd_hdev->dev;
> +    int ret;
> +
> +    /* Common checks */
> +    ret = host_iommu_device_get_cap(hiod, HOST_IOMMU_DEVICE_CAP_AW_BITS, errp);

To avoid CONFIG_HOST_IOMMU_DEVICE, host_iommu_device_get_cap() could be
open coded.

> +    if (ret < 0) {
> +        return false;
> +    }
> +    if (s->aw_bits > ret) {
> +        error_setg(errp, "aw-bits %d > host aw-bits %d", s->aw_bits, ret);
> +        return false;
> +    }
> +#endif
> +    return true;
> +}
> +
>   static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>                                        HostIOMMUDevice *hiod, Error **errp)
>   {
> @@ -3848,6 +3869,12 @@ static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>       vtd_hdev->iommu_state = s;
>       vtd_hdev->dev = hiod;
>   
> +    if (!vtd_check_hdev(s, vtd_hdev, errp)) {
> +        g_free(vtd_hdev);
> +        vtd_iommu_unlock(s);
> +        return false;
> +    }

This check could be first done before allocating vtd_hdev.


Thanks,

C.



>       new_key = g_malloc(sizeof(*new_key));
>       new_key->bus = bus;
>       new_key->devfn = devfn;
Duan, Zhenzhong May 28, 2024, 3 a.m. UTC | #2 
>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v5 19/19] intel_iommu: Check compatibility with host
>IOMMU capabilities
>
>On 5/8/24 11:03, Zhenzhong Duan wrote:
>> If check fails, host device (either VFIO or VDPA device) is not
>> compatible with current vIOMMU config and should not be passed to
>> guest.
>>
>> Only aw_bits is checked for now, we don't care other capabilities
>> before scalable modern mode is introduced.
>>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>   hw/i386/intel_iommu.c | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>> index 747c988bc4..07bfd4f99e 100644
>> --- a/hw/i386/intel_iommu.c
>> +++ b/hw/i386/intel_iommu.c
>> @@ -20,6 +20,7 @@
>>    */
>>
>>   #include "qemu/osdep.h"
>> +#include CONFIG_DEVICES /* CONFIG_HOST_IOMMU_DEVICE */
>>   #include "qemu/error-report.h"
>>   #include "qemu/main-loop.h"
>>   #include "qapi/error.h"
>> @@ -3819,6 +3820,26 @@ VTDAddressSpace
>*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>       return vtd_dev_as;
>>   }
>>
>> +static bool vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice
>*vtd_hdev,
>> +                           Error **errp)
>> +{
>> +#ifdef CONFIG_HOST_IOMMU_DEVICE
>> +    HostIOMMUDevice *hiod = vtd_hdev->dev;
>> +    int ret;
>> +
>> +    /* Common checks */
>> +    ret = host_iommu_device_get_cap(hiod,
>HOST_IOMMU_DEVICE_CAP_AW_BITS, errp);
>
>To avoid CONFIG_HOST_IOMMU_DEVICE, host_iommu_device_get_cap()
>could be
>open coded.

Thanks for suggesting, it works for build on both windows and linux.

>
>> +    if (ret < 0) {
>> +        return false;
>> +    }
>> +    if (s->aw_bits > ret) {
>> +        error_setg(errp, "aw-bits %d > host aw-bits %d", s->aw_bits, ret);
>> +        return false;
>> +    }
>> +#endif
>> +    return true;
>> +}
>> +
>>   static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int
>devfn,
>>                                        HostIOMMUDevice *hiod, Error **errp)
>>   {
>> @@ -3848,6 +3869,12 @@ static bool
>vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
>>       vtd_hdev->iommu_state = s;
>>       vtd_hdev->dev = hiod;
>>
>> +    if (!vtd_check_hdev(s, vtd_hdev, errp)) {
>> +        g_free(vtd_hdev);
>> +        vtd_iommu_unlock(s);
>> +        return false;
>> +    }
>
>This check could be first done before allocating vtd_hdev.

OK, will do.
I made it that way to facilitate this patch:
https://github.com/yiliu1765/qemu/commit/d589a470002ccf607b5743b2951612f7b4790833

Thanks
Zhenzhong
diff mbox series

Patch

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 747c988bc4..07bfd4f99e 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -20,6 +20,7 @@ 
  */
 
 #include "qemu/osdep.h"
+#include CONFIG_DEVICES /* CONFIG_HOST_IOMMU_DEVICE */
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
 #include "qapi/error.h"
@@ -3819,6 +3820,26 @@  VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
     return vtd_dev_as;
 }
 
+static bool vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice *vtd_hdev,
+                           Error **errp)
+{
+#ifdef CONFIG_HOST_IOMMU_DEVICE
+    HostIOMMUDevice *hiod = vtd_hdev->dev;
+    int ret;
+
+    /* Common checks */
+    ret = host_iommu_device_get_cap(hiod, HOST_IOMMU_DEVICE_CAP_AW_BITS, errp);
+    if (ret < 0) {
+        return false;
+    }
+    if (s->aw_bits > ret) {
+        error_setg(errp, "aw-bits %d > host aw-bits %d", s->aw_bits, ret);
+        return false;
+    }
+#endif
+    return true;
+}
+
 static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
                                      HostIOMMUDevice *hiod, Error **errp)
 {
@@ -3848,6 +3869,12 @@  static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
     vtd_hdev->iommu_state = s;
     vtd_hdev->dev = hiod;
 
+    if (!vtd_check_hdev(s, vtd_hdev, errp)) {
+        g_free(vtd_hdev);
+        vtd_iommu_unlock(s);
+        return false;
+    }
+
     new_key = g_malloc(sizeof(*new_key));
     new_key->bus = bus;
     new_key->devfn = devfn;