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

["Cédric Le Goater" <clg@redhat.com>]

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]


Thanks,

C.


> Then legacy and iommufd host device have different implementation of .check_hdev()
> and calls into one of the two interface handlers.
> 
> Let me know if I misunderstand any of your point.
> 
> Thanks
> Zhenzhong
> 
>>
>>
>> Thanks,
>>
>> C.
>>
>>
>>> That the reason I have two functions to do different thing.
>>> See:
>>>
>> https://github.com/yiliu1765/qemu/blob/zhenzhong/iommufd_nesting_rfc
>> v2/hw/i386/intel_iommu.c#L5472
>>>
>>> Meanwhile in vtd_check_legacy_hdev(), when legacy VFIO device attaches
>> to modern vIOMMU,
>>> this is unsupported and error out early, it will not
>> call .get_host_iommu_info().
>>> I mean we don't need to unconditionally call .get_host_iommu_info() in
>> some cases.
>>>
>>> Thanks
>>> Zhenzhong
>