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

["Michael S. Tsirkin" <mst@redhat.com>]

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.

> >
> >Get your point. But I have some concerns about this:
> >1.Exact version matching will bring vast of version check in the code,
> >   especially when we support more versions.
> >2. There are some missed features before we can update version number to
> >3.0,
> >    i.e., nested translation, Accessed/Dirty (A/D) bits, 5 level page table, etc.
> >3. Some features are removed in future versions, but we still need to
> >   implement them for intermediate version,
> >   i.e., ExecuteRequested (ER), Advanced Fault Logging, etc.
> >
> >>
> >>When migration to the old qemu, mgmt can specify e.g vtd=3.0 for
> >>backward migration compatibility.
> >
> >Yes, that makes sense for such migration.
> >But I'm not sure if there is a real scenario migrating to old qemu,
> >why not just update qemu on destination?
> >
> >Thanks
> >Zhenzhong
>