Re: [PATCH] intel_iommu: Use the latest fault reasons defined by spec

[Michael S. Tsirkin]

On Wed, Jul 17, 2024 at 03:30:03AM +0000, Duan, Zhenzhong wrote:
> Hi Michael, Jason,
> 
> Based on Yi's analysis, is keeping current VERSION value acceptable for you?

I think it's fine.

> 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.liu@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 <mst@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 <jasowang@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.duan@intel.com> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>> -----Original Message-----
> >>>>>>>> From: Jason Wang <jasowang@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.duan@intel.com> wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>> -----Original Message-----
> >>>>>>>>>> From: Jason Wang <jasowang@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.liu@intel.com>
> >>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> From: Duan, Zhenzhong <zhenzhong.duan@intel.com>
> >>>>>>>>>>>> Sent: Monday, May 20, 2024 11:41 AM
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>> -----Original Message-----
> >>>>>>>>>>>>> From: Jason Wang <jasowang@redhat.com>
> >>>>>>>>>>>>> Sent: Monday, May 20, 2024 8:44 AM
> >>>>>>>>>>>>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
> >>>>>>>>>>>>> Cc: qemu-devel@nongnu.org; Liu, Yi L <yi.l.liu@intel.com>;
> >Peng,
> >>>>>>>> Chao
> >>>>>>>>>> P
> >>>>>>>>>>>>> <chao.p.peng@intel.com>; Yu Zhang
> >>>>>> <yu.c.zhang@linux.intel.com>;
> >>>>>>>>>> Michael
> >>>>>>>>>>>>> S. Tsirkin <mst@redhat.com>; Paolo Bonzini
> >>>>>>>> <pbonzini@redhat.com>;
> >>>>>>>>>>>>> Richard Henderson <richard.henderson@linaro.org>;
> >Eduardo
> >>>>>>>> Habkost
> >>>>>>>>>>>>> <eduardo@habkost.net>; Marcel Apfelbaum
> >>>>>>>>>> <marcel.apfelbaum@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.duan@intel.com> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> From: Yu Zhang <yu.c.zhang@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.zhang@linux.intel.com>
> >>>>>>>>>>>>>> Signed-off-by: Zhenzhong Duan
> ><zhenzhong.duan@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