Re: [PATCH v5 18/20] intel_iommu: Introduce a property x-flts for scalable modern mode

[Jason Wang]

On Wed, Dec 11, 2024 at 10:50 AM Duan, Zhenzhong
<zhenzhong.duan@intel.com> wrote:
>
> Hi Jason, Clement,
>
> Sorry for late reply, just back from vacation.
>
> >-----Original Message-----
> >From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
> >Subject: Re: [PATCH v5 18/20] intel_iommu: Introduce a property x-flts for
> >scalable modern mode
> >
> >
> >
> >
> >On 09/12/2024 07:24, Jason Wang wrote:
> >> Caution: External email. Do not open attachments or click links, unless 
> >> this
> >email comes from a known sender and you know the content is safe.
> >>
> >>
> >> On Mon, Dec 9, 2024 at 2:15 PM CLEMENT MATHIEU--DRIF
> >> <clement.mathieu--drif@eviden.com> wrote:
> >>>
> >>>
> >>> On 09/12/2024 04:13, Jason Wang wrote:
> >>>> Caution: External email. Do not open attachments or click links, unless 
> >>>> this
> >email comes from a known sender and you know the content is safe.
> >>>>
> >>>>
> >>>> On Wed, Dec 4, 2024 at 2:14 PM CLEMENT MATHIEU--DRIF
> >>>> <clement.mathieu--drif@eviden.com> wrote:
> >>>>>
> >>>>>
> >>>>> On 04/12/2024 04:34, Jason Wang wrote:
> >>>>>> Caution: External email. Do not open attachments or click links, 
> >>>>>> unless this
> >email comes from a known sender and you know the content is safe.
> >>>>>>
> >>>>>>
> >>>>>> On Mon, Nov 11, 2024 at 4:39 PM Zhenzhong Duan
> ><zhenzhong.duan@intel.com> wrote:
> >>>>>>> Intel VT-d 3.0 introduces scalable mode, and it has a bunch of 
> >>>>>>> capabilities
> >>>>>>> related to scalable mode translation, thus there are multiple
> >combinations.
> >>>>>>>
> >>>>>>> This vIOMMU implementation wants to simplify it with a new property 
> >>>>>>> "x-
> >flts".
> >>>>>>> When enabled in scalable mode, first stage translation also known as
> >scalable
> >>>>>>> modern mode is supported. When enabled in legacy mode, throw out
> >error.
> >>>>>>>
> >>>>>>> With scalable modern mode exposed to user, also accurate the pasid
> >entry
> >>>>>>> check in vtd_pe_type_check().
> >>>>>>>
> >>>>>>> Suggested-by: Jason Wang <jasowang@redhat.com>
> >>>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> >>>>>>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> >>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> >>>>>>> ---
> >>>>>>>     hw/i386/intel_iommu_internal.h |  2 ++
> >>>>>>>     hw/i386/intel_iommu.c          | 28 +++++++++++++++++++---------
> >>>>>>>     2 files changed, 21 insertions(+), 9 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/hw/i386/intel_iommu_internal.h
> >b/hw/i386/intel_iommu_internal.h
> >>>>>>> index 2c977aa7da..e8b211e8b0 100644
> >>>>>>> --- a/hw/i386/intel_iommu_internal.h
> >>>>>>> +++ b/hw/i386/intel_iommu_internal.h
> ...
> >>>>>>> @@ -4737,6 +4742,11 @@ static bool
> >vtd_decide_config(IntelIOMMUState *s, Error **errp)
> >>>>>>>             }
> >>>>>>>         }
> >>>>>>>
> >>>>>>> +    if (!s->scalable_mode && s->scalable_modern) {
> >>>>>>> +        error_setg(errp, "Legacy mode: not support x-flts=on");
> >>>>>> This seems to be wired, should we say "scalable mode is needed for
> >>>>>> scalable modern mode"?
> >>>>> Hi Jason,
> >>>>>
> >>>>> We agreed to use the following sentence: "x-flts is only available in
> >>>>> scalable mode"
> >>>>>
> >>>>> Does it look goot to you?
> >>>> Better but if we add more features to the scalable modern, we need to
> >>>> change the error message here.
> >>> Hi Jason
> >>>
> >>> Maybe the weirdness comes from the fact that x-flts on the command line
> >>> is mapped to scalable_modern in the code?
> >> Yes, actually the code checks if scalable mode is enabled if scalable
> >> modern is enabled. But this is inconsistent with the error message
> >> (though x-flts was implied there probably).
> >
> >Would you rename s->scalable_modern to s->flts?
>
> Starting from v4, we replace x-scalable-mode=modern with flts=on on QEMU 
> cmdline.
> Scalable modern mode is an alias of stage-1 page table, so I reuse 
> s->scalable_modern
> in code, I'm fine to rename to s->flts if that's preferred. In that case, 
> maybe we should
> also drop the concept of 'scalable modern mode' totally?

I think so, it helps to reduce the confusion.

Thanks

>
> Thanks
> Zhenzhong