>-----Original Message----- >From: Cédric Le Goater <c...@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 <c...@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....@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....@intel.com> >>>> Signed-off-by: Yi Sun <yi.y....@linux.intel.com> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@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