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

[Duan, Zhenzhong]

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