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

[CLEMENT MATHIEU--DRIF]

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
>>>> @@ -195,6 +195,7 @@
>>>>    #define VTD_ECAP_PASID              (1ULL << 40)
>>>>    #define VTD_ECAP_SMTS               (1ULL << 43)
>>>>    #define VTD_ECAP_SLTS               (1ULL << 46)
>>>> +#define VTD_ECAP_FLTS               (1ULL << 47)
>>>>
>>>>    /* CAP_REG */
>>>>    /* (offset >> 4) << 24 */
>>>> @@ -211,6 +212,7 @@
>>>>    #define VTD_CAP_SLLPS               ((1ULL << 34) | (1ULL << 35))
>>>>    #define VTD_CAP_DRAIN_WRITE         (1ULL << 54)
>>>>    #define VTD_CAP_DRAIN_READ          (1ULL << 55)
>>>> +#define VTD_CAP_FS1GP               (1ULL << 56)
>>>>    #define VTD_CAP_DRAIN               (VTD_CAP_DRAIN_READ | 
>>>> VTD_CAP_DRAIN_WRITE)
>>>>    #define VTD_CAP_CM                  (1ULL << 7)
>>>>    #define VTD_PASID_ID_SHIFT          20
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index b921793c3a..a7a81aebee 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -803,16 +803,18 @@ static inline bool 
>>>> vtd_is_fl_level_supported(IntelIOMMUState *s, uint32_t level)
>>>>    }
>>>>
>>>>    /* Return true if check passed, otherwise false */
>>>> -static inline bool vtd_pe_type_check(X86IOMMUState *x86_iommu,
>>>> -                                     VTDPASIDEntry *pe)
>>>> +static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry 
>>>> *pe)
>>>>    {
>>>>        switch (VTD_PE_GET_TYPE(pe)) {
>>>> -    case VTD_SM_PASID_ENTRY_SLT:
>>>> -        return true;
>>>> -    case VTD_SM_PASID_ENTRY_PT:
>>>> -        return x86_iommu->pt_supported;
>>>>        case VTD_SM_PASID_ENTRY_FLT:
>>>> +        return !!(s->ecap & VTD_ECAP_FLTS);
>>>> +    case VTD_SM_PASID_ENTRY_SLT:
>>>> +        return !!(s->ecap & VTD_ECAP_SLTS);
>>>>        case VTD_SM_PASID_ENTRY_NESTED:
>>>> +        /* Not support NESTED page table type yet */
>>>> +        return false;
>>>> +    case VTD_SM_PASID_ENTRY_PT:
>>>> +        return !!(s->ecap & VTD_ECAP_PT);
>>>>        default:
>>>>            /* Unknown type */
>>>>            return false;
>>>> @@ -861,7 +863,6 @@ static int 
>>>> vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>>>>        uint8_t pgtt;
>>>>        uint32_t index;
>>>>        dma_addr_t entry_size;
>>>> -    X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>>>>
>>>>        index = VTD_PASID_TABLE_INDEX(pasid);
>>>>        entry_size = VTD_PASID_ENTRY_SIZE;
>>>> @@ -875,7 +876,7 @@ static int 
>>>> vtd_get_pe_in_pasid_leaf_table(IntelIOMMUState *s,
>>>>        }
>>>>
>>>>        /* Do translation type check */
>>>> -    if (!vtd_pe_type_check(x86_iommu, pe)) {
>>>> +    if (!vtd_pe_type_check(s, pe)) {
>>>>            return -VTD_FR_PASID_TABLE_ENTRY_INV;
>>>>        }
>>>>
>>>> @@ -3827,6 +3828,7 @@ static Property vtd_properties[] = {
>>>>                          VTD_HOST_ADDRESS_WIDTH),
>>>>        DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, 
>>>> FALSE),
>>>>        DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, 
>>>> FALSE),
>>>> +    DEFINE_PROP_BOOL("x-flts", IntelIOMMUState, scalable_modern, FALSE),
>>>>        DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, 
>>>> false),
>>>>        DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false),
>>>>        DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
>>>> @@ -4558,7 +4560,10 @@ static void vtd_cap_init(IntelIOMMUState *s)
>>>>        }
>>>>
>>>>        /* TODO: read cap/ecap from host to decide which cap to be exposed. 
>>>> */
>>>> -    if (s->scalable_mode) {
>>>> +    if (s->scalable_modern) {
>>>> +        s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_FLTS;
>>>> +        s->cap |= VTD_CAP_FS1GP;
>>>> +    } else if (s->scalable_mode) {
>>>>            s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
>>>>        }
>>>>
>>>> @@ -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?

Thanks
 >cmd

> 
> Thanks
> 
>>
>> Thanks
>> cmd
>>
>>>
>>>> +        return false;
>>>> +    }
>>>> +
>>>>        if (!s->scalable_modern && s->aw_bits != VTD_HOST_AW_39BIT &&
>>>>            s->aw_bits != VTD_HOST_AW_48BIT) {
>>>>            error_setg(errp, "%s mode: supported values for aw-bits are: 
>>>> %d, %d",
>>>> --
>>>> 2.34.1
>>>>
>>>
>>> Thanks
>>>
>