Re: [PATCH v4 15/17] intel_iommu: Introduce a property x-fls for scalable modern mode

[Yi Liu <yi.l.liu@intel.com>]

On 2024/11/4 14:25, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Monday, November 4, 2024 12:25 PM
>> Subject: Re: [PATCH v4 15/17] intel_iommu: Introduce a property x-fls for
>> scalable modern mode
>>
>> On 2024/9/30 17:26, Zhenzhong Duan 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-fls".
>>> 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().
>>>
>>> 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>
>>
>> Maybe a Suggested-by tag can help to understand where this idea comes. :)
> 
> Will add:
> Suggested-by: Jason Wang <jasowang@redhat.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 2702edd27f..f13576d334 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 068a08f522..14578655e1 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;
>>>        }
>>>
>>> @@ -3779,6 +3780,7 @@ static Property vtd_properties[] = {
>>>                          VTD_HOST_AW_AUTO),
>>>        DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode,
>> FALSE),
>>>        DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode,
>> FALSE),
>>> +    DEFINE_PROP_BOOL("x-fls", IntelIOMMUState, scalable_modern, FALSE),
>>>        DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control,
>> false),
>>
>> a question: is there any requirement on the layout of this array? Should
>> new fields added in the end?
> 
> Looked over the history, seems we didn't have an explicit rule in vtd_properties.
> I put "x-fls" just under "x-scalable-mode" as stage-1 is a sub-feature of scalable mode.
> Let me know if you have preference to add in the end.

I don't have a preference for now as long as it does not break any
functionality. BTW. Will x-flt or x-flts better?

> 
>>
>>>        DEFINE_PROP_BOOL("x-pasid-mode", IntelIOMMUState, pasid, false),
>>>        DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
>>> @@ -4509,7 +4511,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;
>>>        }
>>>
>>> @@ -4683,6 +4688,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-fls=on");
>>> +        return false;
>>> +    }
>>> +
>>>        if (s->aw_bits == VTD_HOST_AW_AUTO) {
>>>            if (s->scalable_modern) {
>>>                s->aw_bits = VTD_HOST_AW_48BIT;
>>
>> --
>> Regards,
>> Yi Liu

-- 
Regards,
Yi Liu