Hi Michael, Jason, Based on Yi's analysis, is keeping current VERSION value acceptable for you? Look forward to your comments, currently this open blocks us from sending the next version.
Thanks Zhenzhong >-----Original Message----- >From: Liu, Yi L <yi.l....@intel.com> >Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons defined by >spec > >Hi Michael, Jason, > >On 2024/5/28 11:03, Jason Wang wrote: >> On Mon, May 27, 2024 at 2:50 PM Michael S. Tsirkin <m...@redhat.com> >wrote: >>> >>> On Mon, May 27, 2024 at 06:44:58AM +0000, Duan, Zhenzhong wrote: >>>> Hi Jason, >>>> >>>>> -----Original Message----- >>>>> From: Duan, Zhenzhong >>>>> Subject: RE: [PATCH] intel_iommu: Use the latest fault reasons defined >by >>>>> spec >>>>> >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Jason Wang <jasow...@redhat.com> >>>>>> Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons >defined by >>>>>> spec >>>>>> >>>>>> On Fri, May 24, 2024 at 4:41 PM Duan, Zhenzhong >>>>>> <zhenzhong.d...@intel.com> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Jason Wang <jasow...@redhat.com> >>>>>>>> Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons >defined >>>>> by >>>>>>>> spec >>>>>>>> >>>>>>>> On Tue, May 21, 2024 at 6:25 PM Duan, Zhenzhong >>>>>>>> <zhenzhong.d...@intel.com> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: Jason Wang <jasow...@redhat.com> >>>>>>>>>> Subject: Re: [PATCH] intel_iommu: Use the latest fault reasons >>>>> defined >>>>>> by >>>>>>>>>> spec >>>>>>>>>> >>>>>>>>>> On Mon, May 20, 2024 at 12:15 PM Liu, Yi L <yi.l....@intel.com> >>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> From: Duan, Zhenzhong <zhenzhong.d...@intel.com> >>>>>>>>>>>> Sent: Monday, May 20, 2024 11:41 AM >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> -----Original Message----- >>>>>>>>>>>>> From: Jason Wang <jasow...@redhat.com> >>>>>>>>>>>>> Sent: Monday, May 20, 2024 8:44 AM >>>>>>>>>>>>> To: Duan, Zhenzhong <zhenzhong.d...@intel.com> >>>>>>>>>>>>> Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l....@intel.com>; >Peng, >>>>>>>> Chao >>>>>>>>>> P >>>>>>>>>>>>> <chao.p.p...@intel.com>; Yu Zhang >>>>>> <yu.c.zh...@linux.intel.com>; >>>>>>>>>> Michael >>>>>>>>>>>>> S. Tsirkin <m...@redhat.com>; Paolo Bonzini >>>>>>>> <pbonz...@redhat.com>; >>>>>>>>>>>>> Richard Henderson <richard.hender...@linaro.org>; >Eduardo >>>>>>>> Habkost >>>>>>>>>>>>> <edua...@habkost.net>; Marcel Apfelbaum >>>>>>>>>> <marcel.apfelb...@gmail.com> >>>>>>>>>>>>> Subject: Re: [PATCH] intel_iommu: Use the latest fault >reasons >>>>>>>> defined >>>>>>>>>> by >>>>>>>>>>>>> spec >>>>>>>>>>>>> >>>>>>>>>>>>> On Fri, May 17, 2024 at 6:26 PM Zhenzhong Duan >>>>>>>>>>>>> <zhenzhong.d...@intel.com> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> From: Yu Zhang <yu.c.zh...@linux.intel.com> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Currently we use only VTD_FR_PASID_TABLE_INV as fault >>>>>> reason. >>>>>>>>>>>>>> Update with more detailed fault reasons listed in VT-d spec >>>>>> 7.2.3. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Signed-off-by: Yu Zhang <yu.c.zh...@linux.intel.com> >>>>>>>>>>>>>> Signed-off-by: Zhenzhong Duan ><zhenzhong.d...@intel.com> >>>>>>>>>>>>>> --- >>>>>>>>>>>>> >>>>>>>>>>>>> I wonder if this could be noticed by the guest or not. If yes >>>>> should >>>>>>>>>>>>> we consider starting to add thing like version to vtd >emulation >>>>>> code? >>>>>>>>>>>> >>>>>>>>>>>> Kernel only dumps the reason like below: >>>>>>>>>>>> >>>>>>>>>>>> DMAR: [DMA Write NO_PASID] Request device [20:00.0] fault >>>>> addr >>>>>>>>>> 0x1234600000 >>>>>>>>>>>> [fault reason 0x71] SM: Present bit in first-level paging entry >is >>>>>> clear >>>>>>>>>>> >>>>>>>>>>> Yes, guest kernel would notice it as the fault would be injected >to >>>>> vm. >>>>>>>>>>> >>>>>>>>>>>> Maybe bump 1.0 -> 1.1? >>>>>>>>>>>> My understanding version number is only informational and >is >>>>> far >>>>>>>> from >>>>>>>>>>>> accurate to mark if a feature supported. Driver should check >>>>>> cap/ecap >>>>>>>>>>>> bits instead. >>>>>>>>>>> >>>>>>>>>>> Should the version ID here be aligned with VT-d spec? >>>>>>>>>> >>>>>>>>>> Probably, this might be something that could be noticed by the >>>>>>>>>> management to migration compatibility. >>>>>>>>> >>>>>>>>> Could you elaborate what we need to do for migration >compatibility? >>>>>>>>> I see version is already exported so libvirt can query it, see: >>>>>>>>> >>>>>>>>> DEFINE_PROP_UINT32("version", IntelIOMMUState, version, 0), >>>>>>>> >>>>>>>> It is the Qemu command line parameters not the version of the >vmstate. >>>>>>>> >>>>>>>> For example -device intel-iommu,version=3.0 >>>>>>>> >>>>>>>> Qemu then knows it should behave as 3.0. >>>>>>> >>>>>>> So you want to bump vtd_vmstate.version? >>>>>> >>>>>> Well, as I said, it's not a direct bumping. >>>>>> >>>>>>> >>>>>>> In fact, this series change intel_iommu property from x-scalable- >>>>>> mode=["on"|"off"]" >>>>>>> to x-scalable-mode=["legacy"|"modern"|"off"]". >>>>>>> >>>>>>> My understanding management app should use same qemu cmdline >>>>>>> in source and destination, so compatibility is already guaranteed >even if >>>>>>> we don't bump vtd_vmstate.version. >>>>>> >>>>>> Exactly, so the point is to >>>>>> >>>>>> vtd=3.0, the device works exactly as vtd spec 3.0. >>>>>> vtd=3.3, the device works exactly as vtd spec 3.3. >>>> >>>> Yi just found version ID stored in VT-d VER_REG is not aligned with the >VT-d spec version. >>>> For example, we see a local hw with vtd version 6.0 which is beyond VT- >d spec version. >>>> We are asking VTD arch, will get back soon. >>>> >>>> Or will you plan qemu vVT-d having its own version policy? >>>> >>>> Thanks >>>> Zhenzhong >>> >>> Not unless there's a good reason to do this. >> >> +1 > >Had more studying in the spec. Bumping up to 3.0 might not be trivial. :( > >The VT-d spec has some requirements based on the version number. Below >is a >list of it. Although they are defined for hardware, but vVT-d may need to >respect it as the same iommu driver runs for both the vVT-d and the hw >VT-d. Per 1), if bumping up the major value to be 6 or higher, the vVT-d >needs to ensure the register-based invalidation interface is not available. >Per 2), if bump up the major version to be 2 or higher, the vVT-d needs to >by default drain write and read requests no matter the software requests it >or not. > >1) Chapter 6.5 Invalidation of Translation Caches > >For software to invalidate the various caching structures, the architecture >supports the following two >types of invalidation interfaces: >• Register-based invalidation interface: A legacy invalidation interface >with limited capabilities. >This interface is supported by hardware implementations of this >architecture with Major Version 5 >or lower (VER_REG). In all other hardware implementations, all requests are >treated as invalid >requests and will be ignored (for details see the CAIG field in the Context >Command Register and >the IAIG field in the IOTLB Invalidate Register). > >2) Chapter 6.5.2.3 IOTLB Invalidate >• Drain Reads (DR): Software sets this flag to indicate hardware must drain >read requests that are >already processed by the remapping hardware, but queued within the >Root-Complex to be >completed. When the value of this flag is 1, hardware must drain the >relevant reads before the >next Invalidation Wait Descriptor (see Section 6.5.2.8) is completed. >Section 6.5.4 describes >hardware support for draining. Hardware implementations with Major >Version >2 or higher >(VER_REG) will ignore this flag and always drain relevant reads before the >next Invalidation Wait >Descriptor is completed. >• Drain Writes (DW): Software sets this flag to indicate hardware must >drain relevant write >requests that are already processed by the remapping hardware, but queued >within the RootComplex to be completed. When the value of this flag is 1, >hardware must drain the relevant >writes before the next Invalidation Wait Descriptor is completed. Section >6.5.4 describes >hardware support for draining. Hardware implementations with Major >Version >2 or higher >(VER_REG) will ignore this flag and always drain relevant writes before the >next Invalidation Wait >Descriptor is completed. > >3) Chapter 11.4.2 Capability Register >Hardware implementations with Major Version 6 or higher >(VER_REG) reporting the second-stage translation support (SSTS) >field as Clear also report this field as 0. > >4) Chapter 11.4.8.1 Protected Memory Enable Register >• Hardware implementations with Major Version 2 or higher >(VER_REG) block all DMA requests accessing protected >memory regions whether or not DMA remapping is >enabled. > >Also, as replied in the prior email, the VT-d architecture reports >capability via the cap/ecap registers. The new fault reasons in this >patch is meaningful only when the ecap.SMTS bit is set. So bumping version >does not mean too much about the introduction of new capabilities. @Jason, >given the above statements, can we reconsider if it is necessary to bump >up the version number? > >-- >Regards, >Yi Liu