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

[CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>]

On 11/12/2024 04:03, 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 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

Yep, at this stage dropping mentions to "modern" is clearer.

Thanks
 >cmd
>
>> Thanks
>> Zhenzhong