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
>
>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.

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