ok, feel free to add my RB if this is the only change Thanks cmd
On 19/11/2024 10:25, Duan, Zhenzhong 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. Clear, will use "x-flts is only available in scalable mode". Thanks Clement. From: CLEMENT MATHIEU--DRIF <clement.mathieu--d...@eviden.com><mailto:clement.mathieu--d...@eviden.com> Sent: Tuesday, November 19, 2024 5:00 PM To: Duan, Zhenzhong <zhenzhong.d...@intel.com><mailto:zhenzhong.d...@intel.com>; qemu-devel@nongnu.org<mailto:qemu-devel@nongnu.org> Cc: alex.william...@redhat.com<mailto:alex.william...@redhat.com>; c...@redhat.com<mailto:c...@redhat.com>; eric.au...@redhat.com<mailto:eric.au...@redhat.com>; m...@redhat.com<mailto:m...@redhat.com>; pet...@redhat.com<mailto:pet...@redhat.com>; jasow...@redhat.com<mailto:jasow...@redhat.com>; j...@nvidia.com<mailto:j...@nvidia.com>; nicol...@nvidia.com<mailto:nicol...@nvidia.com>; joao.m.mart...@oracle.com<mailto:joao.m.mart...@oracle.com>; Tian, Kevin <kevin.t...@intel.com><mailto:kevin.t...@intel.com>; Liu, Yi L <yi.l....@intel.com><mailto:yi.l....@intel.com>; Peng, Chao P <chao.p.p...@intel.com><mailto:chao.p.p...@intel.com>; Yi Sun <yi.y....@linux.intel.com><mailto:yi.y....@linux.intel.com>; Marcel Apfelbaum <marcel.apfelb...@gmail.com><mailto:marcel.apfelb...@gmail.com>; Paolo Bonzini <pbonz...@redhat.com><mailto:pbonz...@redhat.com>; Richard Henderson <richard.hender...@linaro.org><mailto:richard.hender...@linaro.org>; Eduardo Habkost <edua...@habkost.net><mailto:edua...@habkost.net> Subject: Re: [PATCH v5 18/20] intel_iommu: Introduce a property x-flts for scalable modern mode On 19/11/2024 08:28, Duan, Zhenzhong 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. Hi Clement, -----Original Message----- From: CLEMENT MATHIEU--DRIF <clement.mathieu--d...@eviden.com><mailto:clement.mathieu--d...@eviden.com> Subject: Re: [PATCH v5 18/20] intel_iommu: Introduce a property x-flts for scalable modern mode Hi zhenzhong, Just one comment but you can add Reviewed-by: Clément Mathieu-- Drif<clement.mathieu--d...@eviden.com><mailto:clement.mathieu--d...@eviden.com> On 11/11/2024 09:34, Zhenzhong Duan 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. 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 <jasow...@redhat.com><mailto:jasow...@redhat.com> Signed-off-by: Yi Liu <yi.l....@intel.com><mailto:yi.l....@intel.com> Signed-off-by: Yi Sun <yi.y....@linux.intel.com><mailto:yi.y....@linux.intel.com> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com><mailto:zhenzhong.d...@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"); The error message should be "x-flts=on not supported in legacy mode" or even "x-flts is only available in scalable mode" as there is no FLT in legacy mode OK, will do. But I'm not quite clear of the difference between "Legacy mode: not support x-flts=on" and "x-flts=on not supported in legacy mode". Is it because the later looks more formal or the former has ambiguity? It's just because the former looks more natural. But I think the most appropriate would be : "x-flts only available in scalable mode" because the issue is about "availability", not "support" Thanks >cmd Thanks Zhenzhong
