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

[Duan, Zhenzhong]

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?

Thanks
Zhenzhong