>-----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.
Thanks
Zhenzhong
>
>> 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
