On Fri, Apr 26, 2024 at 03:10:14AM +0000, Duan, Zhenzhong wrote: > > > >-----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/25/24 10:46, 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 > >>> > >>> Hello Zhenzhong, > >>> > >>> On 4/18/24 10:42, 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 > >>>>> > >>>>> Hello Zhenzhong > >>>>> > >>>>> On 4/17/24 11:24, Duan, Zhenzhong wrote: > >>>>>> > >>>>>> > >>>>>>> -----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/17/24 06:21, Duan, Zhenzhong wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>>> -----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. > >>>>> > >>>>> This comment is true for the structures also. > >>>>> > >>>>>>>>> Could we introduce a new HostIOMMUDeviceClass > >>>>>>>>> handler .check_hdev() handler, which would > >>>>> call .get_host_iommu_info() ? > >>>>> > >>>>> This means that HIOD_LEGACY_INFO and HIOD_IOMMUFD_INFO > >should > >>> be > >>>>> a common structure 'HostIOMMUDeviceInfo' holding all attributes > >>>>> for the different backends. Each .get_host_iommu_info() > >implementation > >>>>> would translate the specific host iommu device data presentation > >>>>> into the common 'HostIOMMUDeviceInfo', this is true for > >host_aw_bits. > >>>> > >>>> I see, it's just not easy to define the unified elements in > >>> HostIOMMUDeviceInfo > >>>> so that they maps to bits or fields in host return IOMMU info. > >>> > >>> The proposal is adding a vIOMMU <-> HostIOMMUDevice interface and a > >>> new > >>> API needs to be completely defined for it. The IOMMU backend > >>> implementation > >>> could be anything, legacy, iommufd, iommufd v2, some other framework > >>> and > >>> the vIOMMU shouldn't be aware of its implementation. > >>> > >>> Exposing the kernel structures as done below should be avoided because > >>> they are part of the QEMU <-> kernel IOMMUFD interface. > >>> > >>> > >>>> Different platform returned host IOMMU info is platform specific. > >>>> For vtd and siommu: > >>>> > >>>> struct iommu_hw_info_vtd { > >>>> __u32 flags; > >>>> __u32 __reserved; > >>>> __aligned_u64 cap_reg; > >>>> __aligned_u64 ecap_reg; > >>>> }; > >>>> > >>>> struct iommu_hw_info_arm_smmuv3 { > >>>> __u32 flags; > >>>> __u32 __reserved; > >>>> __u32 idr[6]; > >>>> __u32 iidr; > >>>> __u32 aidr; > >>>> }; > >>>> > >>>> I can think of two kinds of declaration of HostIOMMUDeviceInfo: > >>>> > >>>> struct HostIOMMUDeviceInfo { > >>>> uint8_t aw_bits; > >>>> enum iommu_hw_info_type type; > >>>> union { > >>>> struct iommu_hw_info_vtd vtd; > >>>> struct iommu_hw_info_arm_smmuv3; > >>>> ...... > >>>> } data; > >>>> } > >>>> > >>>> or > >>>> > >>>> struct HostIOMMUDeviceInfo { > >>>> uint8_t aw_bits; > >>>> enum iommu_hw_info_type type; > >>>> __u32 flags; > >>>> __aligned_u64 cap_reg; > >>>> __aligned_u64 ecap_reg; > >>>> __u32 idr[6]; > >>>> __u32 iidr; > >>>> __u32 aidr; > >>>> ...... > >>>> } > >>>> > >>>> Not clear if any is your expected format. > >>>> > >>>>> 'type' could be handled the same way, with a 'HostIOMMUDeviceInfo' > >>>>> type attribute and host iommu device type definitions, or as you > >>>>> suggested with a QOM interface. This is more complex however. In > >>>>> this case, I would suggest to implement a .compatible() handler to > >>>>> compare the host iommu device type with the vIOMMU type. > >>>>> > >>>>> The resulting check_hdev routine would look something like : > >>>>> > >>>>> 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); > >>>>> HostIOMMUDevice info; > >>>>> int host_aw_bits, ret; > >>>>> > >>>>> ret = hiodc->get_host_iommu_info(hiod, &info, sizeof(info), errp); > >>>>> if (ret) { > >>>>> return ret; > >>>>> } > >>>>> > >>>>> ret = hiodc->is_compatible(hiod, VIOMMU_INTERFACE(s)); > >>>>> if (ret) { > >>>>> return ret; > >>>>> } > >>>>> > >>>>> if (s->aw_bits > info.aw_bits) { > >>>>> error_setg(errp, "aw-bits %d > host aw-bits %d", > >>>>> s->aw_bits, info.aw_bits); > >>>>> return -EINVAL; > >>>>> } > >>>>> } > >>>>> > >>>>> and the HostIOMMUDeviceClass::is_compatible() handler would call a > >>>>> vIOMMUInterface::compatible() handler simply returning > >>>>> IOMMU_HW_INFO_TYPE_INTEL_VTD. How does that sound ? > >>>> > >>>> Not quite get what HostIOMMUDeviceClass::is_compatible() does. > >>> > >>> HostIOMMUDeviceClass::is_compatible() calls in the host IOMMU > >backend > >>> to determine which IOMMU types are exposed by the host, then calls the > >>> vIOMMUInterface::compatible() handler to do the compare. API is to be > >>> defined. > >>> > >>> As a refinement, we could introduce in the vIOMMU <-> > >HostIOMMUDevice > >>> interface capabilities, or features, to check more precisely the level > >>> of compatibility between the vIOMMU and the host IOMMU device. This > >is > >>> similar to what is done between QEMU and KVM. > >>> > >>> If you think this is too complex, include type in HostIOMMUDeviceInfo. > >>> > >>>> Currently legacy and IOMMUFD host device has different check logic, > >how > >>> it can help > >>>> in merging vtd_check_legacy_hdev() and vtd_check_iommufd_hdev() > >into > >>> a single vtd_check_hdev()? > >>> > >>> IMHO, IOMMU shouldn't be aware of the IOMMU backend > >implementation, > >>> but > >>> if you think the Intel vIOMMU should access directly the iommufd > >backend > >>> when available, then we should drop this proposal and revisit the design > >>> to take a different approach. > >> > >> I implemented a draft following your suggestions so we could explore > >further. > >> See > >https://github.com/yiliu1765/qemu/tree/zhenzhong/iommufd_nesting_pre > >q_v3_tmp > >> > >> In this draft, it uses .check_cap() to query HOST_IOMMU_DEVICE_CAP_xxx > >> just like KVM CAPs. > >> A common HostIOMMUDeviceCaps structure is introduced to be used by > >> both legacy and iommufd backend. > >> > >> It indeed is cleaner. Only problem is I failed to implement .compatible() > >> as all the check could go ahead by just calling check_cap(). > >> Could you help a quick check to see if I misunderstood any of your > >suggestion? > > > >Thanks for the changes. It looks cleaner and simpler ! Some comments, > > > >* HostIOMMUDeviceIOMMUFDClass seems useless as it is empty. I don't > > remember if you told me already you had plans for future changes. > > Sorry about that if this is the case. I forgot. > > Never mind😊, reason is: > > In nesting series > https://github.com/yiliu1765/qemu/commits/zhenzhong/iommufd_nesting_rfcv2/ > This commit > https://github.com/yiliu1765/qemu/commit/581fc900aa296988eaa48abee6d68d3670faf8c9 > implement [at|de]tach_hwpt handlers. > > So I add an extra layer of abstract HostIOMMUDeviceIOMMUFDClass to define > [at|de]tach_hwpt handlers. > > > > >* I would use the 'host_iommu_device_' prefix for external routines > > which are part of the HostIOMMUDevice API and use 'hiod_' for > > internal routines where it makes sense, to limit the name length for > > instance. > > Good idea, will do. > > > > >* I would rename HOST_IOMMU_DEVICE_CAP_IOMMUFD_V1 to > > HOST_IOMMU_DEVICE_CAP_IOMMUFD. I mentioned IOMMUFD v2 as a > > theoretical example of a different IOMMU interface. I don't think we > > need to anticipate yet :) > > Will do. > > > > >* HostIOMMUDeviceCaps is using 'enum iommu_hw_info_type' from > > 'linux/iommufd.h', that's not my preferred choice but I won't > > object. The result looks good. > > Ok, will keep it for now. We can change when you want in future. > > > > >* HostIOMMUDevice now has a realize() routine to query the host IOMMU > > capability for later usage. This is a good idea. However, you could > > change the return value to bool and avoid the ERRP_GUARD() prologue. > > Will do. > > > > >* Beware of : > > > > struct Range { > > /* > > * Do not access members directly, use the functions! > > * A non-empty range has @lob <= @upb. > > * An empty range has @lob == @upb + 1. > > */ > > uint64_t lob; /* inclusive lower bound */ > > uint64_t upb; /* inclusive upper bound */ > > }; > > I remember😊, will add the change in formal version. > > > > > > >* I think you could introduce a new VFIOIOMMUClass attribute. Let's > > call it VFIOIOMMUClass::hiod_typename. The creation of > >HostIOMMUDevice > > would become generic and you could move : > > > > hiod= > >HOST_IOMMU_DEVICE(object_new(TYPE_HOST_IOMMU_DEVICE_LEGACY_V > >FIO)); > > HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp); > > if (*errp) { > > object_unref(hiod); > > return -EINVAL; > > } > > vbasedev->hiod = hiod; > > > > at the end of vfio_attach_device(). > > Good suggestion! Will do. > > Thanks > Zhenzhong
So I'm expecting v3 of this.