Add an new element scalable_mode in IntelIOMMUState to mark scalable
modern mode, this element will be exposed as an intel_iommu property
finally.
For now, it's only a placehholder and used for cap/ecap initialization,
compatibility check and block host device passthrough until nesting
is supported.
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
hw/i386/intel_iommu_internal.h | 2 ++
include/hw/i386/intel_iommu.h | 1 +
hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++-----------
3 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index c0ca7b372f..4e0331caba 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/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 1eb05c29fc..788ed42477 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -262,6 +262,7 @@ struct IntelIOMMUState {
bool caching_mode; /* RO - is cap CM enabled? */
bool scalable_mode; /* RO - is Scalable Mode supported? */
+ bool scalable_modern; /* RO - is modern SM supported? */
bool snoop_control; /* RO - is SNP filed supported? */
dma_addr_t root; /* Current root table pointer */
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 1cff8b00ae..40cbd4a0f4 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -755,16 +755,20 @@ static inline bool vtd_is_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)
{
+ X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
+
switch (VTD_PE_GET_TYPE(pe)) {
+ case VTD_SM_PASID_ENTRY_FLT:
+ return s->scalable_modern;
case VTD_SM_PASID_ENTRY_SLT:
- return true;
+ return !s->scalable_modern;
+ case VTD_SM_PASID_ENTRY_NESTED:
+ /* Not support NESTED page table type yet */
+ return false;
case VTD_SM_PASID_ENTRY_PT:
return x86_iommu->pt_supported;
- case VTD_SM_PASID_ENTRY_FLT:
- case VTD_SM_PASID_ENTRY_NESTED:
default:
/* Unknown type */
return false;
@@ -813,7 +817,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;
@@ -827,7 +830,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;
}
@@ -3861,7 +3864,13 @@ static bool vtd_check_hiod(IntelIOMMUState *s, HostIOMMUDevice *hiod,
return false;
}
- return true;
+ if (!s->scalable_modern) {
+ /* All checks requested by VTD non-modern mode pass */
+ return true;
+ }
+
+ error_setg(errp, "host device is unsupported in scalable modern mode yet");
+ return false;
}
static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn,
@@ -4084,7 +4093,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;
}
@@ -4251,9 +4263,9 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp)
}
}
- /* Currently only address widths supported are 39 and 48 bits */
if ((s->aw_bits != VTD_HOST_AW_39BIT) &&
- (s->aw_bits != VTD_HOST_AW_48BIT)) {
+ (s->aw_bits != VTD_HOST_AW_48BIT) &&
+ !s->scalable_modern) {
error_setg(errp, "Supported values for aw-bits are: %d, %d",
VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT);
return false;
--
2.34.1
On 18/07/2024 10:16, 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. > > > Add an new element scalable_mode in IntelIOMMUState to mark scalable > modern mode, this element will be exposed as an intel_iommu property > finally. > > For now, it's only a placehholder and used for cap/ecap initialization, > compatibility check and block host device passthrough until nesting > is supported. > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > hw/i386/intel_iommu_internal.h | 2 ++ > include/hw/i386/intel_iommu.h | 1 + > hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++----------- > 3 files changed, 26 insertions(+), 11 deletions(-) > > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h > index c0ca7b372f..4e0331caba 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/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 1eb05c29fc..788ed42477 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -262,6 +262,7 @@ struct IntelIOMMUState { > > bool caching_mode; /* RO - is cap CM enabled? */ > bool scalable_mode; /* RO - is Scalable Mode supported? */ > + bool scalable_modern; /* RO - is modern SM supported? */ > bool snoop_control; /* RO - is SNP filed supported? */ > > dma_addr_t root; /* Current root table pointer */ > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 1cff8b00ae..40cbd4a0f4 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -755,16 +755,20 @@ static inline bool vtd_is_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) > { What about using the cap/ecap registers to know if the translation types are supported or not. Otherwise, we could add a comment to explain why we expect s->scalable_modern to give us enough information. > + X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); > + > switch (VTD_PE_GET_TYPE(pe)) { > + case VTD_SM_PASID_ENTRY_FLT: > + return s->scalable_modern; > case VTD_SM_PASID_ENTRY_SLT: > - return true; > + return !s->scalable_modern; > + case VTD_SM_PASID_ENTRY_NESTED: > + /* Not support NESTED page table type yet */ > + return false; > case VTD_SM_PASID_ENTRY_PT: > return x86_iommu->pt_supported; > - case VTD_SM_PASID_ENTRY_FLT: > - case VTD_SM_PASID_ENTRY_NESTED: > default: > /* Unknown type */ > return false; > @@ -813,7 +817,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; > @@ -827,7 +830,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; > } > > @@ -3861,7 +3864,13 @@ static bool vtd_check_hiod(IntelIOMMUState *s, HostIOMMUDevice *hiod, > return false; > } > > - return true; > + if (!s->scalable_modern) { > + /* All checks requested by VTD non-modern mode pass */ > + return true; > + } > + > + error_setg(errp, "host device is unsupported in scalable modern mode yet"); > + return false; > } > > static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int devfn, > @@ -4084,7 +4093,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; > } > > @@ -4251,9 +4263,9 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error **errp) > } > } > > - /* Currently only address widths supported are 39 and 48 bits */ > if ((s->aw_bits != VTD_HOST_AW_39BIT) && > - (s->aw_bits != VTD_HOST_AW_48BIT)) { > + (s->aw_bits != VTD_HOST_AW_48BIT) && > + !s->scalable_modern) { > error_setg(errp, "Supported values for aw-bits are: %d, %d", > VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT); > return false; > -- > 2.34.1 >
>-----Original Message----- >From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >scalable modern mode > > > >On 18/07/2024 10:16, 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. >> >> >> Add an new element scalable_mode in IntelIOMMUState to mark scalable >> modern mode, this element will be exposed as an intel_iommu property >> finally. >> >> For now, it's only a placehholder and used for cap/ecap initialization, >> compatibility check and block host device passthrough until nesting >> is supported. >> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> hw/i386/intel_iommu_internal.h | 2 ++ >> include/hw/i386/intel_iommu.h | 1 + >> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++----------- >> 3 files changed, 26 insertions(+), 11 deletions(-) >> >> diff --git a/hw/i386/intel_iommu_internal.h >b/hw/i386/intel_iommu_internal.h >> index c0ca7b372f..4e0331caba 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/include/hw/i386/intel_iommu.h >b/include/hw/i386/intel_iommu.h >> index 1eb05c29fc..788ed42477 100644 >> --- a/include/hw/i386/intel_iommu.h >> +++ b/include/hw/i386/intel_iommu.h >> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >> >> bool caching_mode; /* RO - is cap CM enabled? */ >> bool scalable_mode; /* RO - is Scalable Mode supported? */ >> + bool scalable_modern; /* RO - is modern SM supported? */ >> bool snoop_control; /* RO - is SNP filed supported? */ >> >> dma_addr_t root; /* Current root table pointer */ >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index 1cff8b00ae..40cbd4a0f4 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -755,16 +755,20 @@ static inline bool >vtd_is_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) >> { >What about using the cap/ecap registers to know if the translation types >are supported or not. >Otherwise, we could add a comment to explain why we expect >s->scalable_modern to give us enough information. What about below: /* *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. *So it's simpler to check s->scalable_modern directly for a PASID entry type instead ecap bits. */ Thanks Zhenzhong >> + X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); >> + >> switch (VTD_PE_GET_TYPE(pe)) { >> + case VTD_SM_PASID_ENTRY_FLT: >> + return s->scalable_modern; >> case VTD_SM_PASID_ENTRY_SLT: >> - return true; >> + return !s->scalable_modern; >> + case VTD_SM_PASID_ENTRY_NESTED: >> + /* Not support NESTED page table type yet */ >> + return false; >> case VTD_SM_PASID_ENTRY_PT: >> return x86_iommu->pt_supported; >> - case VTD_SM_PASID_ENTRY_FLT: >> - case VTD_SM_PASID_ENTRY_NESTED: >> default: >> /* Unknown type */ >> return false; >> @@ -813,7 +817,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; >> @@ -827,7 +830,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; >> } >> >> @@ -3861,7 +3864,13 @@ static bool vtd_check_hiod(IntelIOMMUState >*s, HostIOMMUDevice *hiod, >> return false; >> } >> >> - return true; >> + if (!s->scalable_modern) { >> + /* All checks requested by VTD non-modern mode pass */ >> + return true; >> + } >> + >> + error_setg(errp, "host device is unsupported in scalable modern mode >yet"); >> + return false; >> } >> >> static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int >devfn, >> @@ -4084,7 +4093,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; >> } >> >> @@ -4251,9 +4263,9 @@ static bool vtd_decide_config(IntelIOMMUState >*s, Error **errp) >> } >> } >> >> - /* Currently only address widths supported are 39 and 48 bits */ >> if ((s->aw_bits != VTD_HOST_AW_39BIT) && >> - (s->aw_bits != VTD_HOST_AW_48BIT)) { >> + (s->aw_bits != VTD_HOST_AW_48BIT) && >> + !s->scalable_modern) { >> error_setg(errp, "Supported values for aw-bits are: %d, %d", >> VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT); >> return false; >> -- >> 2.34.1 >>
On 19/07/2024 04:47, 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. > > >> -----Original Message----- >> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >> scalable modern mode >> >> >> >> On 18/07/2024 10:16, 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. >>> >>> Add an new element scalable_mode in IntelIOMMUState to mark scalable >>> modern mode, this element will be exposed as an intel_iommu property >>> finally. >>> >>> For now, it's only a placehholder and used for cap/ecap initialization, >>> compatibility check and block host device passthrough until nesting >>> is supported. >>> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> hw/i386/intel_iommu_internal.h | 2 ++ >>> include/hw/i386/intel_iommu.h | 1 + >>> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++----------- >>> 3 files changed, 26 insertions(+), 11 deletions(-) >>> >>> diff --git a/hw/i386/intel_iommu_internal.h >> b/hw/i386/intel_iommu_internal.h >>> index c0ca7b372f..4e0331caba 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/include/hw/i386/intel_iommu.h >> b/include/hw/i386/intel_iommu.h >>> index 1eb05c29fc..788ed42477 100644 >>> --- a/include/hw/i386/intel_iommu.h >>> +++ b/include/hw/i386/intel_iommu.h >>> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >>> >>> bool caching_mode; /* RO - is cap CM enabled? */ >>> bool scalable_mode; /* RO - is Scalable Mode supported? */ >>> + bool scalable_modern; /* RO - is modern SM supported? */ >>> bool snoop_control; /* RO - is SNP filed supported? */ >>> >>> dma_addr_t root; /* Current root table pointer */ >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>> index 1cff8b00ae..40cbd4a0f4 100644 >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -755,16 +755,20 @@ static inline bool >> vtd_is_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) >>> { >> What about using the cap/ecap registers to know if the translation types >> are supported or not. >> Otherwise, we could add a comment to explain why we expect >> s->scalable_modern to give us enough information. > What about below: > > /* > *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. > *So it's simpler to check s->scalable_modern directly for a PASID entry type instead ecap bits. > */ Fine ;) > > Thanks > Zhenzhong > >>> + X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); >>> + >>> switch (VTD_PE_GET_TYPE(pe)) { >>> + case VTD_SM_PASID_ENTRY_FLT: >>> + return s->scalable_modern; >>> case VTD_SM_PASID_ENTRY_SLT: >>> - return true; >>> + return !s->scalable_modern; >>> + case VTD_SM_PASID_ENTRY_NESTED: >>> + /* Not support NESTED page table type yet */ >>> + return false; >>> case VTD_SM_PASID_ENTRY_PT: >>> return x86_iommu->pt_supported; >>> - case VTD_SM_PASID_ENTRY_FLT: >>> - case VTD_SM_PASID_ENTRY_NESTED: >>> default: >>> /* Unknown type */ >>> return false; >>> @@ -813,7 +817,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; >>> @@ -827,7 +830,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; >>> } >>> >>> @@ -3861,7 +3864,13 @@ static bool vtd_check_hiod(IntelIOMMUState >> *s, HostIOMMUDevice *hiod, >>> return false; >>> } >>> >>> - return true; >>> + if (!s->scalable_modern) { >>> + /* All checks requested by VTD non-modern mode pass */ >>> + return true; >>> + } >>> + >>> + error_setg(errp, "host device is unsupported in scalable modern mode >> yet"); >>> + return false; >>> } >>> >>> static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int >> devfn, >>> @@ -4084,7 +4093,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; >>> } >>> >>> @@ -4251,9 +4263,9 @@ static bool vtd_decide_config(IntelIOMMUState >> *s, Error **errp) >>> } >>> } >>> >>> - /* Currently only address widths supported are 39 and 48 bits */ >>> if ((s->aw_bits != VTD_HOST_AW_39BIT) && >>> - (s->aw_bits != VTD_HOST_AW_48BIT)) { >>> + (s->aw_bits != VTD_HOST_AW_48BIT) && >>> + !s->scalable_modern) { >>> error_setg(errp, "Supported values for aw-bits are: %d, %d", >>> VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT); >>> return false; >>> -- >>> 2.34.1 >>>
On 2024/7/19 10:47, Duan, Zhenzhong wrote: > > >> -----Original Message----- >> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >> scalable modern mode >> >> >> >> On 18/07/2024 10:16, 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. >>> >>> >>> Add an new element scalable_mode in IntelIOMMUState to mark scalable >>> modern mode, this element will be exposed as an intel_iommu property >>> finally. >>> >>> For now, it's only a placehholder and used for cap/ecap initialization, >>> compatibility check and block host device passthrough until nesting >>> is supported. >>> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> hw/i386/intel_iommu_internal.h | 2 ++ >>> include/hw/i386/intel_iommu.h | 1 + >>> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++----------- >>> 3 files changed, 26 insertions(+), 11 deletions(-) >>> >>> diff --git a/hw/i386/intel_iommu_internal.h >> b/hw/i386/intel_iommu_internal.h >>> index c0ca7b372f..4e0331caba 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/include/hw/i386/intel_iommu.h >> b/include/hw/i386/intel_iommu.h >>> index 1eb05c29fc..788ed42477 100644 >>> --- a/include/hw/i386/intel_iommu.h >>> +++ b/include/hw/i386/intel_iommu.h >>> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >>> >>> bool caching_mode; /* RO - is cap CM enabled? */ >>> bool scalable_mode; /* RO - is Scalable Mode supported? */ >>> + bool scalable_modern; /* RO - is modern SM supported? */ >>> bool snoop_control; /* RO - is SNP filed supported? */ >>> >>> dma_addr_t root; /* Current root table pointer */ >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>> index 1cff8b00ae..40cbd4a0f4 100644 >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -755,16 +755,20 @@ static inline bool >> vtd_is_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) >>> { >> What about using the cap/ecap registers to know if the translation types >> are supported or not. >> Otherwise, we could add a comment to explain why we expect >> s->scalable_modern to give us enough information. > > What about below: > > /* > *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. > *So it's simpler to check s->scalable_modern directly for a PASID entry type instead ecap bits. > */ Since this helper is for pasid entry check, so you can just return false if the pe's PGTT is SS-only. It might make more sense to check the ecap/cap here as anyhow the capability is listed in ecap/cap. This may also bring us some convenience. Say in the future, if we want to add a new mode (e.g. scalable mode 2.0) that supports both FS and SS for guest, we may need to update this helper as well if we check the scalable_modern. But if we check the ecap/cap, then the future change just needs to control the ecap/cap setting at the beginning of the vIOMMU init. To keep the code aligned, you may need to check ecap.PT bit for VTD_SM_PASID_ENTRY_PT. :) > >>> + X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s); >>> + >>> switch (VTD_PE_GET_TYPE(pe)) { >>> + case VTD_SM_PASID_ENTRY_FLT: >>> + return s->scalable_modern; >>> case VTD_SM_PASID_ENTRY_SLT: >>> - return true; >>> + return !s->scalable_modern; >>> + case VTD_SM_PASID_ENTRY_NESTED: >>> + /* Not support NESTED page table type yet */ >>> + return false; >>> case VTD_SM_PASID_ENTRY_PT: >>> return x86_iommu->pt_supported; >>> - case VTD_SM_PASID_ENTRY_FLT: >>> - case VTD_SM_PASID_ENTRY_NESTED: >>> default: >>> /* Unknown type */ >>> return false; >>> @@ -813,7 +817,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; >>> @@ -827,7 +830,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; >>> } >>> >>> @@ -3861,7 +3864,13 @@ static bool vtd_check_hiod(IntelIOMMUState >> *s, HostIOMMUDevice *hiod, >>> return false; >>> } >>> >>> - return true; >>> + if (!s->scalable_modern) { >>> + /* All checks requested by VTD non-modern mode pass */ >>> + return true; >>> + } >>> + >>> + error_setg(errp, "host device is unsupported in scalable modern mode >> yet"); >>> + return false; >>> } >>> >>> static bool vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int >> devfn, >>> @@ -4084,7 +4093,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; >>> } >>> >>> @@ -4251,9 +4263,9 @@ static bool vtd_decide_config(IntelIOMMUState >> *s, Error **errp) >>> } >>> } >>> >>> - /* Currently only address widths supported are 39 and 48 bits */ >>> if ((s->aw_bits != VTD_HOST_AW_39BIT) && >>> - (s->aw_bits != VTD_HOST_AW_48BIT)) { >>> + (s->aw_bits != VTD_HOST_AW_48BIT) && >>> + !s->scalable_modern) { >>> error_setg(errp, "Supported values for aw-bits are: %d, %d", >>> VTD_HOST_AW_39BIT, VTD_HOST_AW_48BIT); >>> return false; >>> -- >>> 2.34.1 >>> -- Regards, Yi Liu
>-----Original Message-----
>From: Liu, Yi L <yi.l.liu@intel.com>
>Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for
>scalable modern mode
>
>On 2024/7/19 10:47, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com>
>>> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable
>for
>>> scalable modern mode
>>>
>>>
>>>
>>> On 18/07/2024 10:16, 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.
>>>>
>>>>
>>>> Add an new element scalable_mode in IntelIOMMUState to mark
>scalable
>>>> modern mode, this element will be exposed as an intel_iommu property
>>>> finally.
>>>>
>>>> For now, it's only a placehholder and used for cap/ecap initialization,
>>>> compatibility check and block host device passthrough until nesting
>>>> is supported.
>>>>
>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>> ---
>>>> hw/i386/intel_iommu_internal.h | 2 ++
>>>> include/hw/i386/intel_iommu.h | 1 +
>>>> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++---------
>--
>>>> 3 files changed, 26 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/i386/intel_iommu_internal.h
>>> b/hw/i386/intel_iommu_internal.h
>>>> index c0ca7b372f..4e0331caba 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/include/hw/i386/intel_iommu.h
>>> b/include/hw/i386/intel_iommu.h
>>>> index 1eb05c29fc..788ed42477 100644
>>>> --- a/include/hw/i386/intel_iommu.h
>>>> +++ b/include/hw/i386/intel_iommu.h
>>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState {
>>>>
>>>> bool caching_mode; /* RO - is cap CM enabled? */
>>>> bool scalable_mode; /* RO - is Scalable Mode supported? */
>>>> + bool scalable_modern; /* RO - is modern SM supported? */
>>>> bool snoop_control; /* RO - is SNP filed supported? */
>>>>
>>>> dma_addr_t root; /* Current root table pointer */
>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>>> index 1cff8b00ae..40cbd4a0f4 100644
>>>> --- a/hw/i386/intel_iommu.c
>>>> +++ b/hw/i386/intel_iommu.c
>>>> @@ -755,16 +755,20 @@ static inline bool
>>> vtd_is_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)
>>>> {
>>> What about using the cap/ecap registers to know if the translation types
>>> are supported or not.
>>> Otherwise, we could add a comment to explain why we expect
>>> s->scalable_modern to give us enough information.
>>
>> What about below:
>>
>> /*
>> *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else
>VTD_ECAP_SLTS can be set or not depending on s->scalable_mode.
>> *So it's simpler to check s->scalable_modern directly for a PASID entry
>type instead ecap bits.
>> */
>
>Since this helper is for pasid entry check, so you can just return false
>if the pe's PGTT is SS-only.
It depends on which scalable mode is chosed.
In scalable legacy mode, PGTT is SS-only and we should return true.
>
>It might make more sense to check the ecap/cap here as anyhow the
>capability is listed in ecap/cap. This may also bring us some convenience.
>
>Say in the future, if we want to add a new mode (e.g. scalable mode 2.0)
>that supports both FS and SS for guest, we may need to update this helper
>as well if we check the scalable_modern. But if we check the ecap/cap, then
>the future change just needs to control the ecap/cap setting at the
>beginning of the vIOMMU init. To keep the code aligned, you may need to
>check ecap.PT bit for VTD_SM_PASID_ENTRY_PT. :)
OK, will be like below:
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -826,14 +826,14 @@ static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe)
switch (VTD_PE_GET_TYPE(pe)) {
case VTD_SM_PASID_ENTRY_FLT:
- return s->scalable_modern;
+ return !!(s->ecap & VTD_ECAP_FLTS);
case VTD_SM_PASID_ENTRY_SLT:
- return !s->scalable_modern;
+ return !!(s->ecap & VTD_ECAP_FLTS) || !(s->ecap & VTD_ECAP_SMTS);
case VTD_SM_PASID_ENTRY_NESTED:
/* Not support NESTED page table type yet */
return false;
case VTD_SM_PASID_ENTRY_PT:
- return x86_iommu->pt_supported;
+ return !!(s->ecap & VTD_ECAP_PT);
default:
/* Unknown type */
return false;
Thanks
Zhenzhong
>-----Original Message----- >From: Duan, Zhenzhong] >Subject: RE: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >scalable modern mode > > > >>-----Original Message----- >>From: Liu, Yi L <yi.l.liu@intel.com> >>Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >>scalable modern mode >> >>On 2024/7/19 10:47, Duan, Zhenzhong wrote: >>> >>> >>>> -----Original Message----- >>>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >>>> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable >>for >>>> scalable modern mode >>>> >>>> >>>> >>>> On 18/07/2024 10:16, 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. >>>>> >>>>> >>>>> Add an new element scalable_mode in IntelIOMMUState to mark >>scalable >>>>> modern mode, this element will be exposed as an intel_iommu >property >>>>> finally. >>>>> >>>>> For now, it's only a placehholder and used for cap/ecap initialization, >>>>> compatibility check and block host device passthrough until nesting >>>>> is supported. >>>>> >>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>> --- >>>>> hw/i386/intel_iommu_internal.h | 2 ++ >>>>> include/hw/i386/intel_iommu.h | 1 + >>>>> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++------- >-- >>-- >>>>> 3 files changed, 26 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/hw/i386/intel_iommu_internal.h >>>> b/hw/i386/intel_iommu_internal.h >>>>> index c0ca7b372f..4e0331caba 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/include/hw/i386/intel_iommu.h >>>> b/include/hw/i386/intel_iommu.h >>>>> index 1eb05c29fc..788ed42477 100644 >>>>> --- a/include/hw/i386/intel_iommu.h >>>>> +++ b/include/hw/i386/intel_iommu.h >>>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >>>>> >>>>> bool caching_mode; /* RO - is cap CM enabled? */ >>>>> bool scalable_mode; /* RO - is Scalable Mode supported? >*/ >>>>> + bool scalable_modern; /* RO - is modern SM supported? */ >>>>> bool snoop_control; /* RO - is SNP filed supported? */ >>>>> >>>>> dma_addr_t root; /* Current root table pointer */ >>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>>>> index 1cff8b00ae..40cbd4a0f4 100644 >>>>> --- a/hw/i386/intel_iommu.c >>>>> +++ b/hw/i386/intel_iommu.c >>>>> @@ -755,16 +755,20 @@ static inline bool >>>> vtd_is_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) >>>>> { >>>> What about using the cap/ecap registers to know if the translation types >>>> are supported or not. >>>> Otherwise, we could add a comment to explain why we expect >>>> s->scalable_modern to give us enough information. >>> >>> What about below: >>> >>> /* >>> *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else >>VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. >>> *So it's simpler to check s->scalable_modern directly for a PASID entry >>type instead ecap bits. >>> */ >> >>Since this helper is for pasid entry check, so you can just return false >>if the pe's PGTT is SS-only. > >It depends on which scalable mode is chosed. >In scalable legacy mode, PGTT is SS-only and we should return true. > >> >>It might make more sense to check the ecap/cap here as anyhow the >>capability is listed in ecap/cap. This may also bring us some convenience. >> >>Say in the future, if we want to add a new mode (e.g. scalable mode 2.0) >>that supports both FS and SS for guest, we may need to update this helper >>as well if we check the scalable_modern. But if we check the ecap/cap, then >>the future change just needs to control the ecap/cap setting at the >>beginning of the vIOMMU init. To keep the code aligned, you may need to >>check ecap.PT bit for VTD_SM_PASID_ENTRY_PT. :) > >OK, will be like below: > >--- a/hw/i386/intel_iommu.c >+++ b/hw/i386/intel_iommu.c >@@ -826,14 +826,14 @@ static inline bool >vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe) > > switch (VTD_PE_GET_TYPE(pe)) { > case VTD_SM_PASID_ENTRY_FLT: >- return s->scalable_modern; >+ return !!(s->ecap & VTD_ECAP_FLTS); > case VTD_SM_PASID_ENTRY_SLT: >- return !s->scalable_modern; >+ return !!(s->ecap & VTD_ECAP_FLTS) || !(s->ecap & VTD_ECAP_SMTS); Sorry typo err, should be: + return !!(s->ecap & VTD_ECAP_SLTS) || !(s->ecap & VTD_ECAP_SMTS); > case VTD_SM_PASID_ENTRY_NESTED: > /* Not support NESTED page table type yet */ > return false; > case VTD_SM_PASID_ENTRY_PT: >- return x86_iommu->pt_supported; >+ return !!(s->ecap & VTD_ECAP_PT); > default: > /* Unknown type */ > return false; > >Thanks >Zhenzhong
On 19/07/2024 05:39, 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. > > >> -----Original Message----- >> From: Duan, Zhenzhong] >> Subject: RE: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >> scalable modern mode >> >> >> >>> -----Original Message----- >>> From: Liu, Yi L <yi.l.liu@intel.com> >>> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable for >>> scalable modern mode >>> >>> On 2024/7/19 10:47, Duan, Zhenzhong wrote: >>>> >>>>> -----Original Message----- >>>>> From: CLEMENT MATHIEU--DRIF <clement.mathieu--drif@eviden.com> >>>>> Subject: Re: [PATCH v1 03/17] intel_iommu: Add a placeholder variable >>> for >>>>> scalable modern mode >>>>> >>>>> >>>>> >>>>> On 18/07/2024 10:16, 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. >>>>>> >>>>>> Add an new element scalable_mode in IntelIOMMUState to mark >>> scalable >>>>>> modern mode, this element will be exposed as an intel_iommu >> property >>>>>> finally. >>>>>> >>>>>> For now, it's only a placehholder and used for cap/ecap initialization, >>>>>> compatibility check and block host device passthrough until nesting >>>>>> is supported. >>>>>> >>>>>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>>> --- >>>>>> hw/i386/intel_iommu_internal.h | 2 ++ >>>>>> include/hw/i386/intel_iommu.h | 1 + >>>>>> hw/i386/intel_iommu.c | 34 +++++++++++++++++++++++------- >> -- >>> -- >>>>>> 3 files changed, 26 insertions(+), 11 deletions(-) >>>>>> >>>>>> diff --git a/hw/i386/intel_iommu_internal.h >>>>> b/hw/i386/intel_iommu_internal.h >>>>>> index c0ca7b372f..4e0331caba 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/include/hw/i386/intel_iommu.h >>>>> b/include/hw/i386/intel_iommu.h >>>>>> index 1eb05c29fc..788ed42477 100644 >>>>>> --- a/include/hw/i386/intel_iommu.h >>>>>> +++ b/include/hw/i386/intel_iommu.h >>>>>> @@ -262,6 +262,7 @@ struct IntelIOMMUState { >>>>>> >>>>>> bool caching_mode; /* RO - is cap CM enabled? */ >>>>>> bool scalable_mode; /* RO - is Scalable Mode supported? >> */ >>>>>> + bool scalable_modern; /* RO - is modern SM supported? */ >>>>>> bool snoop_control; /* RO - is SNP filed supported? */ >>>>>> >>>>>> dma_addr_t root; /* Current root table pointer */ >>>>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>>>>> index 1cff8b00ae..40cbd4a0f4 100644 >>>>>> --- a/hw/i386/intel_iommu.c >>>>>> +++ b/hw/i386/intel_iommu.c >>>>>> @@ -755,16 +755,20 @@ static inline bool >>>>> vtd_is_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) >>>>>> { >>>>> What about using the cap/ecap registers to know if the translation types >>>>> are supported or not. >>>>> Otherwise, we could add a comment to explain why we expect >>>>> s->scalable_modern to give us enough information. >>>> What about below: >>>> >>>> /* >>>> *VTD_ECAP_FLTS in ecap is set if s->scalable_modern is true, or else >>> VTD_ECAP_SLTS can be set or not depending on s->scalable_mode. >>>> *So it's simpler to check s->scalable_modern directly for a PASID entry >>> type instead ecap bits. >>>> */ >>> Since this helper is for pasid entry check, so you can just return false >>> if the pe's PGTT is SS-only. >> It depends on which scalable mode is chosed. >> In scalable legacy mode, PGTT is SS-only and we should return true. >> >>> It might make more sense to check the ecap/cap here as anyhow the >>> capability is listed in ecap/cap. This may also bring us some convenience. >>> >>> Say in the future, if we want to add a new mode (e.g. scalable mode 2.0) >>> that supports both FS and SS for guest, we may need to update this helper >>> as well if we check the scalable_modern. But if we check the ecap/cap, then >>> the future change just needs to control the ecap/cap setting at the >>> beginning of the vIOMMU init. To keep the code aligned, you may need to >>> check ecap.PT bit for VTD_SM_PASID_ENTRY_PT. :) >> OK, will be like below: >> >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -826,14 +826,14 @@ static inline bool >> vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe) >> >> switch (VTD_PE_GET_TYPE(pe)) { >> case VTD_SM_PASID_ENTRY_FLT: >> - return s->scalable_modern; >> + return !!(s->ecap & VTD_ECAP_FLTS); >> case VTD_SM_PASID_ENTRY_SLT: >> - return !s->scalable_modern; >> + return !!(s->ecap & VTD_ECAP_FLTS) || !(s->ecap & VTD_ECAP_SMTS); > Sorry typo err, should be: > > + return !!(s->ecap & VTD_ECAP_SLTS) || !(s->ecap & VTD_ECAP_SMTS); > I agree with Yi's point of view, this version looks good >> case VTD_SM_PASID_ENTRY_NESTED: >> /* Not support NESTED page table type yet */ >> return false; >> case VTD_SM_PASID_ENTRY_PT: >> - return x86_iommu->pt_supported; >> + return !!(s->ecap & VTD_ECAP_PT); >> default: >> /* Unknown type */ >> return false; >> >> Thanks >> Zhenzhong
© 2016 - 2024 Red Hat, Inc.