>-----Original Message-----
>From: Liu, Yi L <yi.l....@intel.com>
>Sent: Monday, November 4, 2024 3:23 PM
>Subject: Re: [PATCH v4 15/17] intel_iommu: Introduce a property x-fls for
>scalable modern mode
>
>On 2024/11/4 14:25, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Liu, Yi L <yi.l....@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....@intel.com>
>>>> Signed-off-by: Yi Sun <yi.y....@linux.intel.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
>>>
>>> Maybe a Suggested-by tag can help to understand where this idea comes. :)
>>
>> Will add:
>> Suggested-by: Jason Wang <jasow...@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?
So first level support(fls) vs. first level translation(flt) or first level
translation support(flts),
looks same for me, but I can change to x-flt or x-flts if you prefer.
Thanks
Zhenzhong
