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

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() ?


Thanks,

C.


> That the reason I have two functions to do different thing.
> See:
> https://github.com/yiliu1765/qemu/blob/zhenzhong/iommufd_nesting_rfcv2/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