RE: [PATCH v2 3/5] intel_iommu: Add a framework to do compatibility check with host IOMMU cap/ecap

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>compatibility check with host IOMMU cap/ecap
>
>On 4/17/24 06:21, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Cédric Le Goater <clg@redhat.com>
>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>>> compatibility check with host IOMMU cap/ecap
>>>
>>> Hello,
>>>
>>> On 4/16/24 09:09, Duan, Zhenzhong wrote:
>>>> Hi Cédric,
>>>>
>>>>> -----Original Message-----
>>>>> From: Cédric Le Goater <clg@redhat.com>
>>>>> Subject: Re: [PATCH v2 3/5] intel_iommu: Add a framework to do
>>>>> compatibility check with host IOMMU cap/ecap
>>>>>
>>>>> On 4/8/24 10:44, Zhenzhong Duan wrote:
>>>>>> From: Yi Liu <yi.l.liu@intel.com>
>>>>>>
>>>>>> If check fails, the host side device(either vfio or vdpa device) should
>not
>>>>>> be passed to guest.
>>>>>>
>>>>>> Implementation details for different backends will be in following
>>> patches.
>>>>>>
>>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>>>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>>>> ---
>>>>>>     hw/i386/intel_iommu.c | 35
>>>>> +++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 35 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>>>> index 4f84e2e801..a49b587c73 100644
>>>>>> --- a/hw/i386/intel_iommu.c
>>>>>> +++ b/hw/i386/intel_iommu.c
>>>>>> @@ -35,6 +35,7 @@
>>>>>>     #include "sysemu/kvm.h"
>>>>>>     #include "sysemu/dma.h"
>>>>>>     #include "sysemu/sysemu.h"
>>>>>> +#include "sysemu/iommufd.h"
>>>>>>     #include "hw/i386/apic_internal.h"
>>>>>>     #include "kvm/kvm_i386.h"
>>>>>>     #include "migration/vmstate.h"
>>>>>> @@ -3819,6 +3820,32 @@ VTDAddressSpace
>>>>> *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
>>>>>>         return vtd_dev_as;
>>>>>>     }
>>>>>>
>>>>>> +static int vtd_check_legacy_hdev(IntelIOMMUState *s,
>>>>>> +                                 HostIOMMUDevice *hiod,
>>>>>> +                                 Error **errp)
>>>>>> +{
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
>>>>>> +                                  HostIOMMUDevice *hiod,
>>>>>> +                                  Error **errp)
>>>>>> +{
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vtd_check_hdev(IntelIOMMUState *s,
>>> VTDHostIOMMUDevice
>>>>> *vtd_hdev,
>>>>>> +                          Error **errp)
>>>>>> +{
>>>>>> +    HostIOMMUDevice *hiod = vtd_hdev->dev;
>>>>>> +
>>>>>> +    if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD)) {
>>>>>> +        return vtd_check_iommufd_hdev(s, hiod, errp);
>>>>>> +    }
>>>>>> +
>>>>>> +    return vtd_check_legacy_hdev(s, hiod, errp);
>>>>>> +}
>>>>>
>>>>>
>>>>> I think we should be using the .get_host_iommu_info() class handler
>>>>> instead. Can we refactor the code slightly to avoid this check on
>>>>> the type ?
>>>>
>>>> There is some difficulty ini avoiding this check, the behavior of
>>> vtd_check_legacy_hdev
>>>> and vtd_check_iommufd_hdev are different especially after nesting
>>> support introduced.
>>>> vtd_check_iommufd_hdev() has much wider check over cap/ecap bits
>>> besides aw_bits.
>>>
>>> I think it is important to fully separate the vIOMMU model from the
>>> host IOMMU backing device. Could we introduce a new
>>> HostIOMMUDeviceClass
>>> handler .check_hdev() handler, which would call .get_host_iommu_info() ?
>>
>> Understood, besides the new .check_hdev() handler, I think we also need a
>new interface
>> class TYPE_IOMMU_CHECK_HDEV which has two handlers
>check_[legacy|iommufd]_hdev(),
>> and different vIOMMUs have different implementation.
>
>I am not sure to understand. Which class hierarchy would implement this
>new "TYPE_IOMMU_CHECK_HDEV" interface ? vIOMMU or host iommu  ?
>
>Could you please explain with an update of your diagram :
>
>                         HostIOMMUDevice
>                                | .get_host_iommu_info()
>                                |
>                                |
>             .------------------------------------.
>             |                  |                 |
>       HIODLegacyVFIO    [HIODLegacyVDPA]    HIODIOMMUFD
>             | .vdev            | [.vdev]         | .iommufd
>                                                  | .devid
>                                                  | [.ioas_id]
>                                                  | [.attach_hwpt()]
>                                                  | [.detach_hwpt()]
>                                                  |
>                                     .----------------------.
>                                     |                      |
>                            HIODIOMMUFDVFIO         [HIODIOMMUFDVDPA]
>                                     | .vdev                | [.vdev]
>

Sure.

                         HostIOMMUDevice
                                | .get_host_iommu_info()
                                | .check_hdev()
                                |
                   .------------------------------.
                   |                              |
               HIODLegacy                    HIODIOMMUFD
                   |                              | .iommufd
             .--------------.                     | .devid
             |              |                     | [.ioas_id]
       HIODLegacyVFIO    [HIODLegacyVDPA]         | [.attach_hwpt()]
             | .vdev            | [.vdev]         | [.detach_hwpt()]
                                                  |
                                     .----------------------.
                                     |                      |
                            HIODIOMMUFDVFIO         [HIODIOMMUFDVDPA]
                                     | .vdev                | [.vdev]


HostIOMMUDevice only declare .check_hdev(), but
HIODLegacy and HIODIOMMUFD will implement .check_hdev().
E.g., hiod_legacy_check_hdev() and hiod_iommufd_check_hdev().

int hiod_legacy_check_hdev(HostIOMMUDevice *hiod, IOMMUCheckHDev *viommu, Error 
**errp)
{
    IOMMUCheckHDevClass *chdc = IOMMU_CHECK_HDEV_GET_CLASS(viommu);

    return chdc->check_legacy_hdev(viommu, hiod, errp);
}

int hiod_iommufd_check_hdev(HostIOMMUDevice *hiod, IOMMUCheckHDev *viommu, 
Error **errp)
{
    IOMMUCheckHDevClass *chdc = IOMMU_CHECK_HDEV_GET_CLASS(viommu);

    return chdc->check_iommufd_hdev(viommu, hiod, errp);
}

And we implement interface TYPE_IOMMU_CHECK_HDEV in intel-iommu module.
Certainly, we can also implement the same in other vIOMMUs we want.
See below pseudo change:

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 68380d50ca..173c702b9f 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -5521,12 +5521,9 @@ static int vtd_check_hdev(IntelIOMMUState *s, 
VTDHostIOMMUDevice *vtd_hdev,
                           Error **errp)
 {
     HostIOMMUDevice *hiod = vtd_hdev->dev;
+    HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod);

-    if (object_dynamic_cast(OBJECT(hiod), TYPE_HIOD_IOMMUFD)) {
-        return vtd_check_iommufd_hdev(s, vtd_hdev, errp);
-    }
-
-    return vtd_check_legacy_hdev(s, hiod, errp);
+    return hiodc->check_hdev(IOMMU_CHECK_HDEV(s), hiod, errp);
 }

 static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
@@ -6076,6 +6073,7 @@ static void vtd_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     X86IOMMUClass *x86_class = X86_IOMMU_DEVICE_CLASS(klass);
+    IOMMUCheckHDevClass *chdc = IOMMU_CHECK_HDEV_CLASS(klass);

     dc->reset = vtd_reset;
     dc->vmsd = &vtd_vmstate;
@@ -6087,6 +6085,8 @@ static void vtd_class_init(ObjectClass *klass, void *data)
     dc->user_creatable = true;
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     dc->desc = "Intel IOMMU (VT-d) DMA Remapping device";
+    chdc->check_legacy_hdev = vtd_check_legacy_hdev;
+    chdc->check_iommufd_hdev = vtd_check_iommufd_hdev;
 }

 static const TypeInfo vtd_info = {
@@ -6094,6 +6094,10 @@ static const TypeInfo vtd_info = {
     .parent        = TYPE_X86_IOMMU_DEVICE,
     .instance_size = sizeof(IntelIOMMUState),
     .class_init    = vtd_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_IOMMU_CHECK_HDEV },
+        { }
+    }
 };

Thanks
Zhenzhong