| Message ID | b9c86a28df2bddca095ae02511ced09585dce164.1624974370.git.bertrand.marquis@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: Sanitize cpuinfo | expand |
Hi Bertrand, On 29/06/2021 18:08, Bertrand Marquis wrote: > Define a sanitize_cpu function to be called on secondary cores to > sanitize the cpuinfo structure from the boot CPU. > > The safest value is taken when possible and the system is marked tainted > if we encounter values which are incompatible with each other. > > Call the sanitize_cpu function on all secondary cores and remove the > code disabling secondary cores if they are not the same as the boot core > as we are now able to handle this use case. > > This is only supported on arm64 so cpu_sanitize is an empty static > inline on arm32. > > This patch also removes the code imported from Linux that will not be > used by Xen. > > Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> > --- > xen/arch/arm/arm64/cpusanitize.c | 236 ++++++++++++++++++++++++------- > xen/arch/arm/smpboot.c | 5 +- > xen/include/asm-arm/cpufeature.h | 9 ++ > xen/include/xen/lib.h | 1 + > 4 files changed, 199 insertions(+), 52 deletions(-) > > diff --git a/xen/arch/arm/arm64/cpusanitize.c b/xen/arch/arm/arm64/cpusanitize.c > index 4cc8378c14..744006ca7c 100644 > --- a/xen/arch/arm/arm64/cpusanitize.c > +++ b/xen/arch/arm/arm64/cpusanitize.c > @@ -97,10 +97,6 @@ struct arm64_ftr_bits { > .width = 0, \ > } > > -static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap); > - > -static bool __system_matches_cap(unsigned int n); > - > /* > * NOTE: Any changes to the visibility of features should be kept in > * sync with the documentation of the CPU feature register ABI. > @@ -277,31 +273,6 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = { > ARM64_FTR_END, > }; > > -static const struct arm64_ftr_bits ftr_ctr[] = { > - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, 31, 1, 1), /* RES1 */ > - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DIC_SHIFT, 1, 1), > - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IDC_SHIFT, 1, 1), > - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_CWG_SHIFT, 4, 0), > - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_ERG_SHIFT, 4, 0), > - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DMINLINE_SHIFT, 4, 1), > - /* > - * Linux can handle differing I-cache policies. Userspace JITs will > - * make use of *minLine. > - * If we have differing I-cache policies, report it as the weakest - VIPT. > - */ > - ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_EXACT, CTR_L1IP_SHIFT, 2, ICACHE_POLICY_VIPT), /* L1Ip */ > - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IMINLINE_SHIFT, 4, 0), > - ARM64_FTR_END, > -}; I don't think this is should be dropped. Xen will need to know the safest cacheline size that can be used for cache maintenance instructions. > - > -static struct arm64_ftr_override __ro_after_init no_override = { }; > - > -struct arm64_ftr_reg arm64_ftr_reg_ctrel0 = { > - .name = "SYS_CTR_EL0", > - .ftr_bits = ftr_ctr, > - .override = &no_override, > -}; > - > static const struct arm64_ftr_bits ftr_id_mmfr0[] = { > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_MMFR0_INNERSHR_SHIFT, 4, 0xf), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_MMFR0_FCSE_SHIFT, 4, 0), > @@ -335,12 +306,6 @@ static const struct arm64_ftr_bits ftr_mvfr2[] = { > ARM64_FTR_END, > }; > > -static const struct arm64_ftr_bits ftr_dczid[] = { > - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, DCZID_DZP_SHIFT, 1, 1), > - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, DCZID_BS_SHIFT, 4, 0), > - ARM64_FTR_END, > -}; Why is this dropped? > - > static const struct arm64_ftr_bits ftr_id_isar0[] = { > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR0_DIVIDE_SHIFT, 4, 0), > ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR0_DEBUG_SHIFT, 4, 0), > @@ -454,12 +419,6 @@ static const struct arm64_ftr_bits ftr_id_dfr1[] = { > ARM64_FTR_END, > }; > > -static const struct arm64_ftr_bits ftr_zcr[] = { > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, > - ZCR_ELx_LEN_SHIFT, ZCR_ELx_LEN_SIZE, 0), /* LEN */ > - ARM64_FTR_END, > -}; At some point we will support SVE in Xen. So any reason we would not want to keep the code? > - > /* > * Common ftr bits for a 32bit register with all hidden, strict > * attributes, with 4bit feature fields and a default safe value of > @@ -478,17 +437,192 @@ static const struct arm64_ftr_bits ftr_generic_32bits[] = { > ARM64_FTR_END, > }; > > -/* Table for a single 32bit feature value */ > -static const struct arm64_ftr_bits ftr_single32[] = { > - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 0, 32, 0), > - ARM64_FTR_END, > -}; > - > -static const struct arm64_ftr_bits ftr_raz[] = { > - ARM64_FTR_END, > -}; > - > /* > * End of imported linux structures > */ > > +static inline int __attribute_const__ > +cpuid_feature_extract_signed_field_width(u64 features, int field, int width) > +{ > + return (s64)(features << (64 - width - field)) >> (64 - width); > +} Please avoid to mix Xen and Linux coding style in the same file. Also, this function and some others below seems to have been taken as-is from Linux. So this should at least be mentionned in the commit message. I would also consider to import them in a separate patch (or maybe in patch#2) so it is clear this is not "new" code. > + > +static inline int __attribute_const__ > +cpuid_feature_extract_signed_field(u64 features, int field) > +{ > + return cpuid_feature_extract_signed_field_width(features, field, 4); > +} > + > +static inline unsigned int __attribute_const__ > +cpuid_feature_extract_unsigned_field_width(u64 features, int field, int width) > +{ > + return (u64)(features << (64 - width - field)) >> (64 - width); > +} > + > +static inline unsigned int __attribute_const__ > +cpuid_feature_extract_unsigned_field(u64 features, int field) > +{ > + return cpuid_feature_extract_unsigned_field_width(features, field, 4); > +} > + > +static inline u64 arm64_ftr_mask(const struct arm64_ftr_bits *ftrp) > +{ > + return (u64)GENMASK(ftrp->shift + ftrp->width - 1, ftrp->shift); > +} > + > +static inline int __attribute_const__ > +cpuid_feature_extract_field_width(u64 features, int field, int width, > + bool sign) > +{ > + return (sign) ? > + cpuid_feature_extract_signed_field_width(features, field, width) : > + cpuid_feature_extract_unsigned_field_width(features, field, width); > +} > + > +static inline int __attribute_const__ > +cpuid_feature_extract_field(u64 features, int field, bool sign) > +{ > + return cpuid_feature_extract_field_width(features, field, 4, sign); > +} > + > +static inline s64 arm64_ftr_value(const struct arm64_ftr_bits *ftrp, u64 val) > +{ > + return (s64)cpuid_feature_extract_field_width(val, ftrp->shift, > + ftrp->width, ftrp->sign); > +} > + > +static s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new, > + s64 cur) > +{ > + s64 ret = 0; > + > + switch ( ftrp->type ) { > + case FTR_EXACT: > + ret = ftrp->safe_val; > + break; > + case FTR_LOWER_SAFE: > + ret = new < cur ? new : cur; > + break; > + case FTR_HIGHER_OR_ZERO_SAFE: > + if (!cur || !new) > + break; > + fallthrough; > + case FTR_HIGHER_SAFE: > + ret = new > cur ? new : cur; We have a macro max() defined in kernel.h. > + break; > + default: > + BUG(); > + } > + > + return ret; > +} > + > +static void sanitize_reg(u64 *cur_reg, u64 new_reg, const char *reg_name, > + const struct arm64_ftr_bits *ftrp) > +{ > + int taint = 0; > + u64 old_reg = *cur_reg; > + > + for ( ; ftrp->width !=0 ; ftrp++ ) > + { > + u64 mask; > + s64 cur_field = arm64_ftr_value(ftrp, *cur_reg); > + s64 new_field = arm64_ftr_value(ftrp, new_reg); > + > + if ( cur_field == new_field ) > + continue; > + > + if ( ftrp->strict ) > + taint = 1; > + > + mask = arm64_ftr_mask(ftrp); > + > + *cur_reg &= ~mask; > + *cur_reg |= (arm64_ftr_safe_value(ftrp, new_field, cur_field) > + << ftrp->shift) & mask; > + } > + > + if ( old_reg != new_reg ) > + printk(XENLOG_DEBUG "SANITY DIF: %s 0x%"PRIx64" -> 0x%"PRIx64"\n", > + reg_name, old_reg, new_reg); > + if ( old_reg != *cur_reg ) > + printk(XENLOG_DEBUG "SANITY FIX: %s 0x%"PRIx64" -> 0x%"PRIx64"\n", > + reg_name, old_reg, *cur_reg); > + > + if ( taint ) > + { > + printk(XENLOG_WARNING "SANITY CHECK: Unexpected variation in %s.\n", > + reg_name); > + add_taint(TAINT_CPU_OUT_OF_SPEC); > + } > +} > + > + > +/* > + * This function should be called on secondary cores to sanitize the boot cpu > + * cpuinfo. Can we renamed boot_cpu_data to system_cpuinfo (or something similar)? This would make clear this is not only the features for the boot CPU? > + */ > +void sanitize_cpu(const struct cpuinfo_arm *new) How about naming it to "update_system_features()"? > +{ > + > +#define SANITIZE_REG(field, num, reg) \ > + sanitize_reg(&boot_cpu_data.field.bits[num], new->field.bits[num], \ > + #reg, ftr_##reg) > + > +#define SANITIZE_ID_REG(field, num, reg) \ > + sanitize_reg(&boot_cpu_data.field.bits[num], new->field.bits[num], \ > + #reg, ftr_id_##reg) > + > +#define SANITIZE_GENERIC_REG(field, num, reg) \ > + sanitize_reg(&boot_cpu_data.field.bits[num], new->field.bits[num], \ > + #reg, ftr_generic_32bits) > + > + SANITIZE_ID_REG(pfr64, 0, aa64pfr0); > + SANITIZE_ID_REG(pfr64, 1, aa64pfr1); > + > + SANITIZE_ID_REG(dbg64, 0, aa64dfr0); > + > + /* aux64 x2 */ > + > + SANITIZE_ID_REG(mm64, 0, aa64mmfr0); > + SANITIZE_ID_REG(mm64, 1, aa64mmfr1); > + SANITIZE_ID_REG(mm64, 2, aa64mmfr2); > + > + SANITIZE_ID_REG(isa64, 0, aa64isar0); > + SANITIZE_ID_REG(isa64, 1, aa64isar1); > + > + SANITIZE_ID_REG(zfr64, 0, aa64zfr0); > + > + if ( cpu_feature64_has_el0_32(&boot_cpu_data) ) > + { > + SANITIZE_ID_REG(pfr32, 0, pfr0); > + SANITIZE_ID_REG(pfr32, 1, pfr1); > + SANITIZE_ID_REG(pfr32, 2, pfr2); > + > + SANITIZE_ID_REG(dbg32, 0, dfr0); > + SANITIZE_ID_REG(dbg32, 1, dfr1); > + > + /* aux32 x1 */ What does this comment mean? > + > + SANITIZE_ID_REG(mm32, 0, mmfr0); > + SANITIZE_GENERIC_REG(mm32, 1, mmfr1); > + SANITIZE_GENERIC_REG(mm32, 2, mmfr2); > + SANITIZE_GENERIC_REG(mm32, 3, mmfr3); > + SANITIZE_ID_REG(mm32, 4, mmfr4); > + SANITIZE_ID_REG(mm32, 5, mmfr5); > + > + SANITIZE_ID_REG(isa32, 0, isar0); > + SANITIZE_GENERIC_REG(isa32, 1, isar1); > + SANITIZE_GENERIC_REG(isa32, 2, isar2); > + SANITIZE_GENERIC_REG(isa32, 3, isar3); > + SANITIZE_ID_REG(isa32, 4, isar4); > + SANITIZE_ID_REG(isa32, 5, isar5); > + SANITIZE_ID_REG(isa32, 6, isar6); > + > + SANITIZE_GENERIC_REG(mvfr, 0, mvfr0); > + SANITIZE_GENERIC_REG(mvfr, 1, mvfr1); > +#ifndef MVFR2_MAYBE_UNDEFINED > + SANITIZE_REG(mvfr, 2, mvfr2); > +#endif > + } > +} > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index a1ee3146ef..8fdf5e70d3 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -307,12 +307,14 @@ void start_secondary(void) > set_processor_id(cpuid); > > identify_cpu(¤t_cpu_data); > + sanitize_cpu(¤t_cpu_data); > processor_setup(); > > init_traps(); > > +#ifndef CONFIG_ARM_64 > /* > - * Currently Xen assumes the platform has only one kind of CPUs. > + * Currently Xen assumes the platform has only one kind of CPUs on ARM32. > * This assumption does not hold on big.LITTLE platform and may > * result to instability and insecure platform (unless cpu affinity > * is manually specified for all domains). Better to park them for > @@ -328,6 +330,7 @@ void start_secondary(void) > boot_cpu_data.midr.bits); > stop_cpu(); > } > +#endif > > if ( dcache_line_bytes != read_dcache_line_bytes() ) > { Any plan to drop this check? > diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h > index ba48db3eac..ad34e55cc8 100644 > --- a/xen/include/asm-arm/cpufeature.h > +++ b/xen/include/asm-arm/cpufeature.h > @@ -330,6 +330,15 @@ extern struct cpuinfo_arm boot_cpu_data; > > extern void identify_cpu(struct cpuinfo_arm *); > > +#ifdef CONFIG_ARM_64 > +extern void sanitize_cpu(const struct cpuinfo_arm *); > +#else > +static inline void sanitize_cpu(const struct cpuinfo_arm *cpuinfo) > +{ > + /* Not supported on arm32 */ > +} > +#endif > + > extern struct cpuinfo_arm cpu_data[]; > #define current_cpu_data cpu_data[smp_processor_id()] > > diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h > index 1198c7c0b2..c6987973bf 100644 > --- a/xen/include/xen/lib.h > +++ b/xen/include/xen/lib.h > @@ -192,6 +192,7 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c); > #define TAINT_ERROR_INJECT (1u << 2) > #define TAINT_HVM_FEP (1u << 3) > #define TAINT_MACHINE_UNSECURE (1u << 4) > +#define TAINT_CPU_OUT_OF_SPEC (1u << 5) You want to also update at least print_tainted(). > extern unsigned int tainted; > #define TAINT_STRING_MAX_LEN 20 > extern char *print_tainted(char *str); >
Hi Julien, > On 12 Jul 2021, at 11:16, Julien Grall <julien@xen.org> wrote: > > Hi Bertrand, > > On 29/06/2021 18:08, Bertrand Marquis wrote: >> Define a sanitize_cpu function to be called on secondary cores to >> sanitize the cpuinfo structure from the boot CPU. >> The safest value is taken when possible and the system is marked tainted >> if we encounter values which are incompatible with each other. >> Call the sanitize_cpu function on all secondary cores and remove the >> code disabling secondary cores if they are not the same as the boot core >> as we are now able to handle this use case. >> This is only supported on arm64 so cpu_sanitize is an empty static >> inline on arm32. >> This patch also removes the code imported from Linux that will not be >> used by Xen. >> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >> --- >> xen/arch/arm/arm64/cpusanitize.c | 236 ++++++++++++++++++++++++------- >> xen/arch/arm/smpboot.c | 5 +- >> xen/include/asm-arm/cpufeature.h | 9 ++ >> xen/include/xen/lib.h | 1 + >> 4 files changed, 199 insertions(+), 52 deletions(-) >> diff --git a/xen/arch/arm/arm64/cpusanitize.c b/xen/arch/arm/arm64/cpusanitize.c >> index 4cc8378c14..744006ca7c 100644 >> --- a/xen/arch/arm/arm64/cpusanitize.c >> +++ b/xen/arch/arm/arm64/cpusanitize.c >> @@ -97,10 +97,6 @@ struct arm64_ftr_bits { >> .width = 0, \ >> } >> -static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap); >> - >> -static bool __system_matches_cap(unsigned int n); >> - >> /* >> * NOTE: Any changes to the visibility of features should be kept in >> * sync with the documentation of the CPU feature register ABI. >> @@ -277,31 +273,6 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = { >> ARM64_FTR_END, >> }; >> -static const struct arm64_ftr_bits ftr_ctr[] = { >> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, 31, 1, 1), /* RES1 */ >> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DIC_SHIFT, 1, 1), >> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IDC_SHIFT, 1, 1), >> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_CWG_SHIFT, 4, 0), >> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_ERG_SHIFT, 4, 0), >> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DMINLINE_SHIFT, 4, 1), >> - /* >> - * Linux can handle differing I-cache policies. Userspace JITs will >> - * make use of *minLine. >> - * If we have differing I-cache policies, report it as the weakest - VIPT. >> - */ >> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_EXACT, CTR_L1IP_SHIFT, 2, ICACHE_POLICY_VIPT), /* L1Ip */ >> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IMINLINE_SHIFT, 4, 0), >> - ARM64_FTR_END, >> -}; > > I don't think this is should be dropped. Xen will need to know the safest cacheline size that can be used for cache maintenance instructions. I will surround those entries by #if 0 instead > >> - >> -static struct arm64_ftr_override __ro_after_init no_override = { }; >> - >> -struct arm64_ftr_reg arm64_ftr_reg_ctrel0 = { >> - .name = "SYS_CTR_EL0", >> - .ftr_bits = ftr_ctr, >> - .override = &no_override, >> -}; >> - >> static const struct arm64_ftr_bits ftr_id_mmfr0[] = { >> S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_MMFR0_INNERSHR_SHIFT, 4, 0xf), >> ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_MMFR0_FCSE_SHIFT, 4, 0), >> @@ -335,12 +306,6 @@ static const struct arm64_ftr_bits ftr_mvfr2[] = { >> ARM64_FTR_END, >> }; >> -static const struct arm64_ftr_bits ftr_dczid[] = { >> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, DCZID_DZP_SHIFT, 1, 1), >> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, DCZID_BS_SHIFT, 4, 0), >> - ARM64_FTR_END, >> -}; > > Why is this dropped? Same I will keep that with #if 0 > >> - >> static const struct arm64_ftr_bits ftr_id_isar0[] = { >> ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR0_DIVIDE_SHIFT, 4, 0), >> ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR0_DEBUG_SHIFT, 4, 0), >> @@ -454,12 +419,6 @@ static const struct arm64_ftr_bits ftr_id_dfr1[] = { >> ARM64_FTR_END, >> }; >> -static const struct arm64_ftr_bits ftr_zcr[] = { >> - ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, >> - ZCR_ELx_LEN_SHIFT, ZCR_ELx_LEN_SIZE, 0), /* LEN */ >> - ARM64_FTR_END, >> -}; > > At some point we will support SVE in Xen. So any reason we would not want to keep the code? Same I will keep that with #if 0 > >> - >> /* >> * Common ftr bits for a 32bit register with all hidden, strict >> * attributes, with 4bit feature fields and a default safe value of >> @@ -478,17 +437,192 @@ static const struct arm64_ftr_bits ftr_generic_32bits[] = { >> ARM64_FTR_END, >> }; >> -/* Table for a single 32bit feature value */ >> -static const struct arm64_ftr_bits ftr_single32[] = { >> - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 0, 32, 0), >> - ARM64_FTR_END, >> -}; >> - >> -static const struct arm64_ftr_bits ftr_raz[] = { >> - ARM64_FTR_END, >> -}; >> - >> /* >> * End of imported linux structures >> */ >> +static inline int __attribute_const__ >> +cpuid_feature_extract_signed_field_width(u64 features, int field, int width) >> +{ >> + return (s64)(features << (64 - width - field)) >> (64 - width); >> +} > > Please avoid to mix Xen and Linux coding style in the same file. > > Also, this function and some others below seems to have been taken as-is from Linux. So this should at least be mentionned in the commit message. > > I would also consider to import them in a separate patch (or maybe in patch#2) so it is clear this is not "new" code. I will move those in patch 2 and keep Linux code. > >> + >> +static inline int __attribute_const__ >> +cpuid_feature_extract_signed_field(u64 features, int field) >> +{ >> + return cpuid_feature_extract_signed_field_width(features, field, 4); >> +} >> + >> +static inline unsigned int __attribute_const__ >> +cpuid_feature_extract_unsigned_field_width(u64 features, int field, int width) >> +{ >> + return (u64)(features << (64 - width - field)) >> (64 - width); >> +} >> + >> +static inline unsigned int __attribute_const__ >> +cpuid_feature_extract_unsigned_field(u64 features, int field) >> +{ >> + return cpuid_feature_extract_unsigned_field_width(features, field, 4); >> +} >> + >> +static inline u64 arm64_ftr_mask(const struct arm64_ftr_bits *ftrp) >> +{ >> + return (u64)GENMASK(ftrp->shift + ftrp->width - 1, ftrp->shift); >> +} >> + >> +static inline int __attribute_const__ >> +cpuid_feature_extract_field_width(u64 features, int field, int width, >> + bool sign) >> +{ >> + return (sign) ? >> + cpuid_feature_extract_signed_field_width(features, field, width) : >> + cpuid_feature_extract_unsigned_field_width(features, field, width); >> +} >> + >> +static inline int __attribute_const__ >> +cpuid_feature_extract_field(u64 features, int field, bool sign) >> +{ >> + return cpuid_feature_extract_field_width(features, field, 4, sign); >> +} >> + >> +static inline s64 arm64_ftr_value(const struct arm64_ftr_bits *ftrp, u64 val) >> +{ >> + return (s64)cpuid_feature_extract_field_width(val, ftrp->shift, >> + ftrp->width, ftrp->sign); >> +} >> + >> +static s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new, >> + s64 cur) >> +{ >> + s64 ret = 0; >> + >> + switch ( ftrp->type ) { >> + case FTR_EXACT: >> + ret = ftrp->safe_val; >> + break; >> + case FTR_LOWER_SAFE: >> + ret = new < cur ? new : cur; >> + break; >> + case FTR_HIGHER_OR_ZERO_SAFE: >> + if (!cur || !new) >> + break; >> + fallthrough; >> + case FTR_HIGHER_SAFE: >> + ret = new > cur ? new : cur; > > We have a macro max() defined in kernel.h. Right I will use that instead. > >> + break; >> + default: >> + BUG(); >> + } >> + >> + return ret; >> +} >> + >> +static void sanitize_reg(u64 *cur_reg, u64 new_reg, const char *reg_name, >> + const struct arm64_ftr_bits *ftrp) >> +{ >> + int taint = 0; >> + u64 old_reg = *cur_reg; >> + >> + for ( ; ftrp->width !=0 ; ftrp++ ) >> + { >> + u64 mask; >> + s64 cur_field = arm64_ftr_value(ftrp, *cur_reg); >> + s64 new_field = arm64_ftr_value(ftrp, new_reg); >> + >> + if ( cur_field == new_field ) >> + continue; >> + >> + if ( ftrp->strict ) >> + taint = 1; >> + >> + mask = arm64_ftr_mask(ftrp); >> + >> + *cur_reg &= ~mask; >> + *cur_reg |= (arm64_ftr_safe_value(ftrp, new_field, cur_field) >> + << ftrp->shift) & mask; >> + } >> + >> + if ( old_reg != new_reg ) >> + printk(XENLOG_DEBUG "SANITY DIF: %s 0x%"PRIx64" -> 0x%"PRIx64"\n", >> + reg_name, old_reg, new_reg); >> + if ( old_reg != *cur_reg ) >> + printk(XENLOG_DEBUG "SANITY FIX: %s 0x%"PRIx64" -> 0x%"PRIx64"\n", >> + reg_name, old_reg, *cur_reg); >> + >> + if ( taint ) >> + { >> + printk(XENLOG_WARNING "SANITY CHECK: Unexpected variation in %s.\n", >> + reg_name); >> + add_taint(TAINT_CPU_OUT_OF_SPEC); >> + } >> +} >> + >> + >> +/* >> + * This function should be called on secondary cores to sanitize the boot cpu >> + * cpuinfo. > > Can we renamed boot_cpu_data to system_cpuinfo (or something similar)? This would make clear this is not only the features for the boot CPU? Ok I will do that. > >> + */ >> +void sanitize_cpu(const struct cpuinfo_arm *new) > > How about naming it to "update_system_features()"? Ok > >> +{ >> + >> +#define SANITIZE_REG(field, num, reg) \ >> + sanitize_reg(&boot_cpu_data.field.bits[num], new->field.bits[num], \ >> + #reg, ftr_##reg) >> + >> +#define SANITIZE_ID_REG(field, num, reg) \ >> + sanitize_reg(&boot_cpu_data.field.bits[num], new->field.bits[num], \ >> + #reg, ftr_id_##reg) >> + >> +#define SANITIZE_GENERIC_REG(field, num, reg) \ >> + sanitize_reg(&boot_cpu_data.field.bits[num], new->field.bits[num], \ >> + #reg, ftr_generic_32bits) >> + >> + SANITIZE_ID_REG(pfr64, 0, aa64pfr0); >> + SANITIZE_ID_REG(pfr64, 1, aa64pfr1); >> + >> + SANITIZE_ID_REG(dbg64, 0, aa64dfr0); >> + >> + /* aux64 x2 */ >> + >> + SANITIZE_ID_REG(mm64, 0, aa64mmfr0); >> + SANITIZE_ID_REG(mm64, 1, aa64mmfr1); >> + SANITIZE_ID_REG(mm64, 2, aa64mmfr2); >> + >> + SANITIZE_ID_REG(isa64, 0, aa64isar0); >> + SANITIZE_ID_REG(isa64, 1, aa64isar1); >> + >> + SANITIZE_ID_REG(zfr64, 0, aa64zfr0); >> + >> + if ( cpu_feature64_has_el0_32(&boot_cpu_data) ) >> + { >> + SANITIZE_ID_REG(pfr32, 0, pfr0); >> + SANITIZE_ID_REG(pfr32, 1, pfr1); >> + SANITIZE_ID_REG(pfr32, 2, pfr2); >> + >> + SANITIZE_ID_REG(dbg32, 0, dfr0); >> + SANITIZE_ID_REG(dbg32, 1, dfr1); >> + >> + /* aux32 x1 */ > > What does this comment mean? Leftover during dev that I should turn in proper comment. It was there as I am not sanitizing one aux32 register. Same goes for the comment before for aux64. I will remove them. > >> + >> + SANITIZE_ID_REG(mm32, 0, mmfr0); >> + SANITIZE_GENERIC_REG(mm32, 1, mmfr1); >> + SANITIZE_GENERIC_REG(mm32, 2, mmfr2); >> + SANITIZE_GENERIC_REG(mm32, 3, mmfr3); >> + SANITIZE_ID_REG(mm32, 4, mmfr4); >> + SANITIZE_ID_REG(mm32, 5, mmfr5); >> + >> + SANITIZE_ID_REG(isa32, 0, isar0); >> + SANITIZE_GENERIC_REG(isa32, 1, isar1); >> + SANITIZE_GENERIC_REG(isa32, 2, isar2); >> + SANITIZE_GENERIC_REG(isa32, 3, isar3); >> + SANITIZE_ID_REG(isa32, 4, isar4); >> + SANITIZE_ID_REG(isa32, 5, isar5); >> + SANITIZE_ID_REG(isa32, 6, isar6); >> + >> + SANITIZE_GENERIC_REG(mvfr, 0, mvfr0); >> + SANITIZE_GENERIC_REG(mvfr, 1, mvfr1); >> +#ifndef MVFR2_MAYBE_UNDEFINED >> + SANITIZE_REG(mvfr, 2, mvfr2); >> +#endif >> + } >> +} >> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c >> index a1ee3146ef..8fdf5e70d3 100644 >> --- a/xen/arch/arm/smpboot.c >> +++ b/xen/arch/arm/smpboot.c >> @@ -307,12 +307,14 @@ void start_secondary(void) >> set_processor_id(cpuid); >> identify_cpu(¤t_cpu_data); >> + sanitize_cpu(¤t_cpu_data); >> processor_setup(); >> init_traps(); >> +#ifndef CONFIG_ARM_64 >> /* >> - * Currently Xen assumes the platform has only one kind of CPUs. >> + * Currently Xen assumes the platform has only one kind of CPUs on ARM32. >> * This assumption does not hold on big.LITTLE platform and may >> * result to instability and insecure platform (unless cpu affinity >> * is manually specified for all domains). Better to park them for >> @@ -328,6 +330,7 @@ void start_secondary(void) >> boot_cpu_data.midr.bits); >> stop_cpu(); >> } >> +#endif >> if ( dcache_line_bytes != read_dcache_line_bytes() ) >> { > > Any plan to drop this check? Yes this should be done as a next patch in the serie once we valeted The way to go for the base. > >> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h >> index ba48db3eac..ad34e55cc8 100644 >> --- a/xen/include/asm-arm/cpufeature.h >> +++ b/xen/include/asm-arm/cpufeature.h >> @@ -330,6 +330,15 @@ extern struct cpuinfo_arm boot_cpu_data; >> extern void identify_cpu(struct cpuinfo_arm *); >> +#ifdef CONFIG_ARM_64 >> +extern void sanitize_cpu(const struct cpuinfo_arm *); >> +#else >> +static inline void sanitize_cpu(const struct cpuinfo_arm *cpuinfo) >> +{ >> + /* Not supported on arm32 */ >> +} >> +#endif >> + >> extern struct cpuinfo_arm cpu_data[]; >> #define current_cpu_data cpu_data[smp_processor_id()] >> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h >> index 1198c7c0b2..c6987973bf 100644 >> --- a/xen/include/xen/lib.h >> +++ b/xen/include/xen/lib.h >> @@ -192,6 +192,7 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c); >> #define TAINT_ERROR_INJECT (1u << 2) >> #define TAINT_HVM_FEP (1u << 3) >> #define TAINT_MACHINE_UNSECURE (1u << 4) >> +#define TAINT_CPU_OUT_OF_SPEC (1u << 5) > > You want to also update at least print_tainted(). Oh yes I will fix that in the next version. Cheers Bertrand > >> extern unsigned int tainted; >> #define TAINT_STRING_MAX_LEN 20 >> extern char *print_tainted(char *str); > > -- > Julien Grall
On 12/07/2021 12:03, Bertrand Marquis wrote: > Hi Julien, Hi Bertrand, >> On 12 Jul 2021, at 11:16, Julien Grall <julien@xen.org> wrote: >> >> Hi Bertrand, >> >> On 29/06/2021 18:08, Bertrand Marquis wrote: >>> Define a sanitize_cpu function to be called on secondary cores to >>> sanitize the cpuinfo structure from the boot CPU. >>> The safest value is taken when possible and the system is marked tainted >>> if we encounter values which are incompatible with each other. >>> Call the sanitize_cpu function on all secondary cores and remove the >>> code disabling secondary cores if they are not the same as the boot core >>> as we are now able to handle this use case. >>> This is only supported on arm64 so cpu_sanitize is an empty static >>> inline on arm32. >>> This patch also removes the code imported from Linux that will not be >>> used by Xen. >>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >>> --- >>> xen/arch/arm/arm64/cpusanitize.c | 236 ++++++++++++++++++++++++------- >>> xen/arch/arm/smpboot.c | 5 +- >>> xen/include/asm-arm/cpufeature.h | 9 ++ >>> xen/include/xen/lib.h | 1 + >>> 4 files changed, 199 insertions(+), 52 deletions(-) >>> diff --git a/xen/arch/arm/arm64/cpusanitize.c b/xen/arch/arm/arm64/cpusanitize.c >>> index 4cc8378c14..744006ca7c 100644 >>> --- a/xen/arch/arm/arm64/cpusanitize.c >>> +++ b/xen/arch/arm/arm64/cpusanitize.c >>> @@ -97,10 +97,6 @@ struct arm64_ftr_bits { >>> .width = 0, \ >>> } >>> -static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap); >>> - >>> -static bool __system_matches_cap(unsigned int n); >>> - >>> /* >>> * NOTE: Any changes to the visibility of features should be kept in >>> * sync with the documentation of the CPU feature register ABI. >>> @@ -277,31 +273,6 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = { >>> ARM64_FTR_END, >>> }; >>> -static const struct arm64_ftr_bits ftr_ctr[] = { >>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, 31, 1, 1), /* RES1 */ >>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DIC_SHIFT, 1, 1), >>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IDC_SHIFT, 1, 1), >>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_CWG_SHIFT, 4, 0), >>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_ERG_SHIFT, 4, 0), >>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DMINLINE_SHIFT, 4, 1), >>> - /* >>> - * Linux can handle differing I-cache policies. Userspace JITs will >>> - * make use of *minLine. >>> - * If we have differing I-cache policies, report it as the weakest - VIPT. >>> - */ >>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_EXACT, CTR_L1IP_SHIFT, 2, ICACHE_POLICY_VIPT), /* L1Ip */ >>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IMINLINE_SHIFT, 4, 0), >>> - ARM64_FTR_END, >>> -}; >> >> I don't think this is should be dropped. Xen will need to know the safest cacheline size that can be used for cache maintenance instructions. > > I will surround those entries by #if 0 instead But, why can't this just be sanitized as the other registers? If this is just a matter of missing structure in Xen, then we should add one. The same goes for the other 2 below. Cheers,
Hi Julien, > On 12 Jul 2021, at 12:13, Julien Grall <julien@xen.org> wrote: > > > > On 12/07/2021 12:03, Bertrand Marquis wrote: >> Hi Julien, > > Hi Bertrand, > >>> On 12 Jul 2021, at 11:16, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Bertrand, >>> >>> On 29/06/2021 18:08, Bertrand Marquis wrote: >>>> Define a sanitize_cpu function to be called on secondary cores to >>>> sanitize the cpuinfo structure from the boot CPU. >>>> The safest value is taken when possible and the system is marked tainted >>>> if we encounter values which are incompatible with each other. >>>> Call the sanitize_cpu function on all secondary cores and remove the >>>> code disabling secondary cores if they are not the same as the boot core >>>> as we are now able to handle this use case. >>>> This is only supported on arm64 so cpu_sanitize is an empty static >>>> inline on arm32. >>>> This patch also removes the code imported from Linux that will not be >>>> used by Xen. >>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >>>> --- >>>> xen/arch/arm/arm64/cpusanitize.c | 236 ++++++++++++++++++++++++------- >>>> xen/arch/arm/smpboot.c | 5 +- >>>> xen/include/asm-arm/cpufeature.h | 9 ++ >>>> xen/include/xen/lib.h | 1 + >>>> 4 files changed, 199 insertions(+), 52 deletions(-) >>>> diff --git a/xen/arch/arm/arm64/cpusanitize.c b/xen/arch/arm/arm64/cpusanitize.c >>>> index 4cc8378c14..744006ca7c 100644 >>>> --- a/xen/arch/arm/arm64/cpusanitize.c >>>> +++ b/xen/arch/arm/arm64/cpusanitize.c >>>> @@ -97,10 +97,6 @@ struct arm64_ftr_bits { >>>> .width = 0, \ >>>> } >>>> -static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap); >>>> - >>>> -static bool __system_matches_cap(unsigned int n); >>>> - >>>> /* >>>> * NOTE: Any changes to the visibility of features should be kept in >>>> * sync with the documentation of the CPU feature register ABI. >>>> @@ -277,31 +273,6 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = { >>>> ARM64_FTR_END, >>>> }; >>>> -static const struct arm64_ftr_bits ftr_ctr[] = { >>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, 31, 1, 1), /* RES1 */ >>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DIC_SHIFT, 1, 1), >>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IDC_SHIFT, 1, 1), >>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_CWG_SHIFT, 4, 0), >>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_ERG_SHIFT, 4, 0), >>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DMINLINE_SHIFT, 4, 1), >>>> - /* >>>> - * Linux can handle differing I-cache policies. Userspace JITs will >>>> - * make use of *minLine. >>>> - * If we have differing I-cache policies, report it as the weakest - VIPT. >>>> - */ >>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_EXACT, CTR_L1IP_SHIFT, 2, ICACHE_POLICY_VIPT), /* L1Ip */ >>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IMINLINE_SHIFT, 4, 0), >>>> - ARM64_FTR_END, >>>> -}; >>> >>> I don't think this is should be dropped. Xen will need to know the safest cacheline size that can be used for cache maintenance instructions. >> I will surround those entries by #if 0 instead > > But, why can't this just be sanitized as the other registers? If this is just a matter of missing structure in Xen, then we should add one. > > The same goes for the other 2 below. The point of the RFC was to validate the way to go and do the base. Those require changes out of the cpuinfo and are on my todo list to. Regards Bertrand > > Cheers, > > -- > Julien Grall
Hi Bertrand, On 12/07/2021 17:29, Bertrand Marquis wrote: >> On 12 Jul 2021, at 12:13, Julien Grall <julien@xen.org> wrote: >> >> >> >> On 12/07/2021 12:03, Bertrand Marquis wrote: >>> Hi Julien, >> >> Hi Bertrand, >> >>>> On 12 Jul 2021, at 11:16, Julien Grall <julien@xen.org> wrote: >>>> >>>> Hi Bertrand, >>>> >>>> On 29/06/2021 18:08, Bertrand Marquis wrote: >>>>> Define a sanitize_cpu function to be called on secondary cores to >>>>> sanitize the cpuinfo structure from the boot CPU. >>>>> The safest value is taken when possible and the system is marked tainted >>>>> if we encounter values which are incompatible with each other. >>>>> Call the sanitize_cpu function on all secondary cores and remove the >>>>> code disabling secondary cores if they are not the same as the boot core >>>>> as we are now able to handle this use case. >>>>> This is only supported on arm64 so cpu_sanitize is an empty static >>>>> inline on arm32. >>>>> This patch also removes the code imported from Linux that will not be >>>>> used by Xen. >>>>> Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> >>>>> --- >>>>> xen/arch/arm/arm64/cpusanitize.c | 236 ++++++++++++++++++++++++------- >>>>> xen/arch/arm/smpboot.c | 5 +- >>>>> xen/include/asm-arm/cpufeature.h | 9 ++ >>>>> xen/include/xen/lib.h | 1 + >>>>> 4 files changed, 199 insertions(+), 52 deletions(-) >>>>> diff --git a/xen/arch/arm/arm64/cpusanitize.c b/xen/arch/arm/arm64/cpusanitize.c >>>>> index 4cc8378c14..744006ca7c 100644 >>>>> --- a/xen/arch/arm/arm64/cpusanitize.c >>>>> +++ b/xen/arch/arm/arm64/cpusanitize.c >>>>> @@ -97,10 +97,6 @@ struct arm64_ftr_bits { >>>>> .width = 0, \ >>>>> } >>>>> -static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap); >>>>> - >>>>> -static bool __system_matches_cap(unsigned int n); >>>>> - >>>>> /* >>>>> * NOTE: Any changes to the visibility of features should be kept in >>>>> * sync with the documentation of the CPU feature register ABI. >>>>> @@ -277,31 +273,6 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = { >>>>> ARM64_FTR_END, >>>>> }; >>>>> -static const struct arm64_ftr_bits ftr_ctr[] = { >>>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, 31, 1, 1), /* RES1 */ >>>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DIC_SHIFT, 1, 1), >>>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IDC_SHIFT, 1, 1), >>>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_CWG_SHIFT, 4, 0), >>>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_ERG_SHIFT, 4, 0), >>>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DMINLINE_SHIFT, 4, 1), >>>>> - /* >>>>> - * Linux can handle differing I-cache policies. Userspace JITs will >>>>> - * make use of *minLine. >>>>> - * If we have differing I-cache policies, report it as the weakest - VIPT. >>>>> - */ >>>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_EXACT, CTR_L1IP_SHIFT, 2, ICACHE_POLICY_VIPT), /* L1Ip */ >>>>> - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IMINLINE_SHIFT, 4, 0), >>>>> - ARM64_FTR_END, >>>>> -}; >>>> >>>> I don't think this is should be dropped. Xen will need to know the safest cacheline size that can be used for cache maintenance instructions. >>> I will surround those entries by #if 0 instead >> >> But, why can't this just be sanitized as the other registers? If this is just a matter of missing structure in Xen, then we should add one. >> >> The same goes for the other 2 below. > > The point of the RFC was to validate the way to go and do the base. Right... I was commenting on your suggestion to switch to #if 0 for the next version. I assume this will be a non-RFC and... > Those require changes out of the cpuinfo and are on my todo list to ... it wasn't clear to me that the change on the cpuinfo was on your TODO list for the next version. So I prefer to ask before you spend time working on a change that I may disagree with ;). Cheers,
Hi Julien […] >> >> + >> + if ( old_reg != new_reg ) >> + printk(XENLOG_DEBUG "SANITY DIF: %s 0x%"PRIx64" -> 0x%"PRIx64"\n", >> + reg_name, old_reg, new_reg); >> + if ( old_reg != *cur_reg ) >> + printk(XENLOG_DEBUG "SANITY FIX: %s 0x%"PRIx64" -> 0x%"PRIx64"\n", >> + reg_name, old_reg, *cur_reg); >> + >> + if ( taint ) >> + { >> + printk(XENLOG_WARNING "SANITY CHECK: Unexpected variation in %s.\n", >> + reg_name); >> + add_taint(TAINT_CPU_OUT_OF_SPEC); >> + } >> +} >> + >> + >> +/* >> + * This function should be called on secondary cores to sanitize the boot cpu >> + * cpuinfo. > > Can we renamed boot_cpu_data to system_cpuinfo (or something similar)? This would make clear this is not only the features for the boot CPU? While looking at this request, I checked a bit how boot_cpu_data and cpu_data overall are used: - boot_cpu_data is only used in setup.c, by boot_cpu_features macros, in smpboot to retrieve the bootcpu midr, in p2m and by cpufeatures - cpu_data[] is used in smpboot, in errata handling to test for csv2, and in vcpreg to access the midr So we have a bunch of cpuinfo structures as global variables but most of them are not really used or did I miss something ? So I am wondering if we should not reduce a bit the amount of global data and: - introduce a global system_cpuinfo - remove cpu_data[] and use a temp structure in the stack of the cpu booting - read midr directly in vcpreg - use boot_cpu_data in errata for csv2 - use system_cpuinfo in p2m It could even be possible to remove boot_cpu_data and put it on the stack of the boot cpu and only use system_cpuinfo but I did not investigate this. Cheers Bertrand
On 16/07/2021 18:14, Bertrand Marquis wrote: > Hi Julien Hi Bertrand, > […] >>> >>> + >>> + if ( old_reg != new_reg ) >>> + printk(XENLOG_DEBUG "SANITY DIF: %s 0x%"PRIx64" -> 0x%"PRIx64"\n", >>> + reg_name, old_reg, new_reg); >>> + if ( old_reg != *cur_reg ) >>> + printk(XENLOG_DEBUG "SANITY FIX: %s 0x%"PRIx64" -> 0x%"PRIx64"\n", >>> + reg_name, old_reg, *cur_reg); >>> + >>> + if ( taint ) >>> + { >>> + printk(XENLOG_WARNING "SANITY CHECK: Unexpected variation in %s.\n", >>> + reg_name); >>> + add_taint(TAINT_CPU_OUT_OF_SPEC); >>> + } >>> +} >>> + >>> + >>> +/* >>> + * This function should be called on secondary cores to sanitize the boot cpu >>> + * cpuinfo. >> >> Can we renamed boot_cpu_data to system_cpuinfo (or something similar)? This would make clear this is not only the features for the boot CPU? > > While looking at this request, I checked a bit how boot_cpu_data and cpu_data overall are used: > - boot_cpu_data is only used in setup.c, by boot_cpu_features macros, in smpboot to retrieve the bootcpu midr, in p2m and by cpufeatures > - cpu_data[] is used in smpboot, in errata handling to test for csv2, and in vcpreg to access the midr > > So we have a bunch of cpuinfo structures as global variables but most of them are not really used or did I miss something ? While I agree this is not useful today, the idea is we can find easily what features each processor supports. This could be useful if we wanted to expose big.LITTLE to the guest. For instance, imagine you have a system where some processor may support 32-bit EL1 only on some processor. With a global approach, we would say "32-bit EL1 is not supported". That would prevent a user to use the system to its full advantage. Note that I am not asking to implement such things today... This is more to show that we will likely want to keep the per-CPU info around. The system_cpuinfo could be used for system wide decision in Xen (e.g. P2M size, cacheline size....) while the per-CPU could be used to enable features only used by a couple of CPUs. > > So I am wondering if we should not reduce a bit the amount of global data and: How much are we talking? > - introduce a global system_cpuinfo > - remove cpu_data[] and use a temp structure in the stack of the cpu booting > - read midr directly in vcpreg > - use boot_cpu_data in errata for csv2 This would not be quite the same. You may have a system where not all processors have ID_AA64PFR0_EL1.CSV2 is set, yet we want to avoid setting the hardening vector on process with the bit set to reduce the overhead. Cheers,
Hi Julien, > On 19 Jul 2021, at 09:58, Julien Grall <julien@xen.org> wrote: > > > > On 16/07/2021 18:14, Bertrand Marquis wrote: >> Hi Julien > > Hi Bertrand, > >> […] >>>> >>>> + >>>> + if ( old_reg != new_reg ) >>>> + printk(XENLOG_DEBUG "SANITY DIF: %s 0x%"PRIx64" -> 0x%"PRIx64"\n", >>>> + reg_name, old_reg, new_reg); >>>> + if ( old_reg != *cur_reg ) >>>> + printk(XENLOG_DEBUG "SANITY FIX: %s 0x%"PRIx64" -> 0x%"PRIx64"\n", >>>> + reg_name, old_reg, *cur_reg); >>>> + >>>> + if ( taint ) >>>> + { >>>> + printk(XENLOG_WARNING "SANITY CHECK: Unexpected variation in %s.\n", >>>> + reg_name); >>>> + add_taint(TAINT_CPU_OUT_OF_SPEC); >>>> + } >>>> +} >>>> + >>>> + >>>> +/* >>>> + * This function should be called on secondary cores to sanitize the boot cpu >>>> + * cpuinfo. >>> >>> Can we renamed boot_cpu_data to system_cpuinfo (or something similar)? This would make clear this is not only the features for the boot CPU? >> While looking at this request, I checked a bit how boot_cpu_data and cpu_data overall are used: >> - boot_cpu_data is only used in setup.c, by boot_cpu_features macros, in smpboot to retrieve the bootcpu midr, in p2m and by cpufeatures >> - cpu_data[] is used in smpboot, in errata handling to test for csv2, and in vcpreg to access the midr >> So we have a bunch of cpuinfo structures as global variables but most of them are not really used or did I miss something ? > > While I agree this is not useful today, the idea is we can find easily what features each processor supports. This could be useful if we wanted to expose big.LITTLE to the guest. > > For instance, imagine you have a system where some processor may support 32-bit EL1 only on some processor. With a global approach, we would say "32-bit EL1 is not supported". That would prevent a user to use the system to its full advantage. > > Note that I am not asking to implement such things today... This is more to show that we will likely want to keep the per-CPU info around. > > The system_cpuinfo could be used for system wide decision in Xen (e.g. P2M size, cacheline size....) while the per-CPU could be used to enable features only used by a couple of CPUs. I understand the potential need (even though supporting to assign guest CPUs depending on features needed would be something complex to achieve). > >> So I am wondering if we should not reduce a bit the amount of global data and: > > How much are we talking? We are declaring cpuinfo for all possible cores in smpboot: struct cpuinfo_arm cpu_data[NR_CPUS]; And default value for NR_CPUS is 128 so that makes around 9k of data. This is not that much I agree. > >> - introduce a global system_cpuinfo >> - remove cpu_data[] and use a temp structure in the stack of the cpu booting >> - read midr directly in vcpreg >> - use boot_cpu_data in errata for csv2 > > This would not be quite the same. You may have a system where not all processors have ID_AA64PFR0_EL1.CSV2 is set, yet we want to avoid setting the hardening vector on process with the bit set to reduce the overhead. Agree and with the actual size reduction being quite small this does not make sense. Thanks a lot for the feedback, I will go on with the system_cpuinfo and is. Cheers Bertrand > > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/arm64/cpusanitize.c b/xen/arch/arm/arm64/cpusanitize.c index 4cc8378c14..744006ca7c 100644 --- a/xen/arch/arm/arm64/cpusanitize.c +++ b/xen/arch/arm/arm64/cpusanitize.c @@ -97,10 +97,6 @@ struct arm64_ftr_bits { .width = 0, \ } -static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap); - -static bool __system_matches_cap(unsigned int n); - /* * NOTE: Any changes to the visibility of features should be kept in * sync with the documentation of the CPU feature register ABI. @@ -277,31 +273,6 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = { ARM64_FTR_END, }; -static const struct arm64_ftr_bits ftr_ctr[] = { - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, 31, 1, 1), /* RES1 */ - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DIC_SHIFT, 1, 1), - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IDC_SHIFT, 1, 1), - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_CWG_SHIFT, 4, 0), - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_HIGHER_OR_ZERO_SAFE, CTR_ERG_SHIFT, 4, 0), - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_DMINLINE_SHIFT, 4, 1), - /* - * Linux can handle differing I-cache policies. Userspace JITs will - * make use of *minLine. - * If we have differing I-cache policies, report it as the weakest - VIPT. - */ - ARM64_FTR_BITS(FTR_VISIBLE, FTR_NONSTRICT, FTR_EXACT, CTR_L1IP_SHIFT, 2, ICACHE_POLICY_VIPT), /* L1Ip */ - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, CTR_IMINLINE_SHIFT, 4, 0), - ARM64_FTR_END, -}; - -static struct arm64_ftr_override __ro_after_init no_override = { }; - -struct arm64_ftr_reg arm64_ftr_reg_ctrel0 = { - .name = "SYS_CTR_EL0", - .ftr_bits = ftr_ctr, - .override = &no_override, -}; - static const struct arm64_ftr_bits ftr_id_mmfr0[] = { S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_MMFR0_INNERSHR_SHIFT, 4, 0xf), ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_MMFR0_FCSE_SHIFT, 4, 0), @@ -335,12 +306,6 @@ static const struct arm64_ftr_bits ftr_mvfr2[] = { ARM64_FTR_END, }; -static const struct arm64_ftr_bits ftr_dczid[] = { - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_EXACT, DCZID_DZP_SHIFT, 1, 1), - ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, DCZID_BS_SHIFT, 4, 0), - ARM64_FTR_END, -}; - static const struct arm64_ftr_bits ftr_id_isar0[] = { ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR0_DIVIDE_SHIFT, 4, 0), ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_ISAR0_DEBUG_SHIFT, 4, 0), @@ -454,12 +419,6 @@ static const struct arm64_ftr_bits ftr_id_dfr1[] = { ARM64_FTR_END, }; -static const struct arm64_ftr_bits ftr_zcr[] = { - ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, - ZCR_ELx_LEN_SHIFT, ZCR_ELx_LEN_SIZE, 0), /* LEN */ - ARM64_FTR_END, -}; - /* * Common ftr bits for a 32bit register with all hidden, strict * attributes, with 4bit feature fields and a default safe value of @@ -478,17 +437,192 @@ static const struct arm64_ftr_bits ftr_generic_32bits[] = { ARM64_FTR_END, }; -/* Table for a single 32bit feature value */ -static const struct arm64_ftr_bits ftr_single32[] = { - ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_EXACT, 0, 32, 0), - ARM64_FTR_END, -}; - -static const struct arm64_ftr_bits ftr_raz[] = { - ARM64_FTR_END, -}; - /* * End of imported linux structures */ +static inline int __attribute_const__ +cpuid_feature_extract_signed_field_width(u64 features, int field, int width) +{ + return (s64)(features << (64 - width - field)) >> (64 - width); +} + +static inline int __attribute_const__ +cpuid_feature_extract_signed_field(u64 features, int field) +{ + return cpuid_feature_extract_signed_field_width(features, field, 4); +} + +static inline unsigned int __attribute_const__ +cpuid_feature_extract_unsigned_field_width(u64 features, int field, int width) +{ + return (u64)(features << (64 - width - field)) >> (64 - width); +} + +static inline unsigned int __attribute_const__ +cpuid_feature_extract_unsigned_field(u64 features, int field) +{ + return cpuid_feature_extract_unsigned_field_width(features, field, 4); +} + +static inline u64 arm64_ftr_mask(const struct arm64_ftr_bits *ftrp) +{ + return (u64)GENMASK(ftrp->shift + ftrp->width - 1, ftrp->shift); +} + +static inline int __attribute_const__ +cpuid_feature_extract_field_width(u64 features, int field, int width, + bool sign) +{ + return (sign) ? + cpuid_feature_extract_signed_field_width(features, field, width) : + cpuid_feature_extract_unsigned_field_width(features, field, width); +} + +static inline int __attribute_const__ +cpuid_feature_extract_field(u64 features, int field, bool sign) +{ + return cpuid_feature_extract_field_width(features, field, 4, sign); +} + +static inline s64 arm64_ftr_value(const struct arm64_ftr_bits *ftrp, u64 val) +{ + return (s64)cpuid_feature_extract_field_width(val, ftrp->shift, + ftrp->width, ftrp->sign); +} + +static s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new, + s64 cur) +{ + s64 ret = 0; + + switch ( ftrp->type ) { + case FTR_EXACT: + ret = ftrp->safe_val; + break; + case FTR_LOWER_SAFE: + ret = new < cur ? new : cur; + break; + case FTR_HIGHER_OR_ZERO_SAFE: + if (!cur || !new) + break; + fallthrough; + case FTR_HIGHER_SAFE: + ret = new > cur ? new : cur; + break; + default: + BUG(); + } + + return ret; +} + +static void sanitize_reg(u64 *cur_reg, u64 new_reg, const char *reg_name, + const struct arm64_ftr_bits *ftrp) +{ + int taint = 0; + u64 old_reg = *cur_reg; + + for ( ; ftrp->width !=0 ; ftrp++ ) + { + u64 mask; + s64 cur_field = arm64_ftr_value(ftrp, *cur_reg); + s64 new_field = arm64_ftr_value(ftrp, new_reg); + + if ( cur_field == new_field ) + continue; + + if ( ftrp->strict ) + taint = 1; + + mask = arm64_ftr_mask(ftrp); + + *cur_reg &= ~mask; + *cur_reg |= (arm64_ftr_safe_value(ftrp, new_field, cur_field) + << ftrp->shift) & mask; + } + + if ( old_reg != new_reg ) + printk(XENLOG_DEBUG "SANITY DIF: %s 0x%"PRIx64" -> 0x%"PRIx64"\n", + reg_name, old_reg, new_reg); + if ( old_reg != *cur_reg ) + printk(XENLOG_DEBUG "SANITY FIX: %s 0x%"PRIx64" -> 0x%"PRIx64"\n", + reg_name, old_reg, *cur_reg); + + if ( taint ) + { + printk(XENLOG_WARNING "SANITY CHECK: Unexpected variation in %s.\n", + reg_name); + add_taint(TAINT_CPU_OUT_OF_SPEC); + } +} + + +/* + * This function should be called on secondary cores to sanitize the boot cpu + * cpuinfo. + */ +void sanitize_cpu(const struct cpuinfo_arm *new) +{ + +#define SANITIZE_REG(field, num, reg) \ + sanitize_reg(&boot_cpu_data.field.bits[num], new->field.bits[num], \ + #reg, ftr_##reg) + +#define SANITIZE_ID_REG(field, num, reg) \ + sanitize_reg(&boot_cpu_data.field.bits[num], new->field.bits[num], \ + #reg, ftr_id_##reg) + +#define SANITIZE_GENERIC_REG(field, num, reg) \ + sanitize_reg(&boot_cpu_data.field.bits[num], new->field.bits[num], \ + #reg, ftr_generic_32bits) + + SANITIZE_ID_REG(pfr64, 0, aa64pfr0); + SANITIZE_ID_REG(pfr64, 1, aa64pfr1); + + SANITIZE_ID_REG(dbg64, 0, aa64dfr0); + + /* aux64 x2 */ + + SANITIZE_ID_REG(mm64, 0, aa64mmfr0); + SANITIZE_ID_REG(mm64, 1, aa64mmfr1); + SANITIZE_ID_REG(mm64, 2, aa64mmfr2); + + SANITIZE_ID_REG(isa64, 0, aa64isar0); + SANITIZE_ID_REG(isa64, 1, aa64isar1); + + SANITIZE_ID_REG(zfr64, 0, aa64zfr0); + + if ( cpu_feature64_has_el0_32(&boot_cpu_data) ) + { + SANITIZE_ID_REG(pfr32, 0, pfr0); + SANITIZE_ID_REG(pfr32, 1, pfr1); + SANITIZE_ID_REG(pfr32, 2, pfr2); + + SANITIZE_ID_REG(dbg32, 0, dfr0); + SANITIZE_ID_REG(dbg32, 1, dfr1); + + /* aux32 x1 */ + + SANITIZE_ID_REG(mm32, 0, mmfr0); + SANITIZE_GENERIC_REG(mm32, 1, mmfr1); + SANITIZE_GENERIC_REG(mm32, 2, mmfr2); + SANITIZE_GENERIC_REG(mm32, 3, mmfr3); + SANITIZE_ID_REG(mm32, 4, mmfr4); + SANITIZE_ID_REG(mm32, 5, mmfr5); + + SANITIZE_ID_REG(isa32, 0, isar0); + SANITIZE_GENERIC_REG(isa32, 1, isar1); + SANITIZE_GENERIC_REG(isa32, 2, isar2); + SANITIZE_GENERIC_REG(isa32, 3, isar3); + SANITIZE_ID_REG(isa32, 4, isar4); + SANITIZE_ID_REG(isa32, 5, isar5); + SANITIZE_ID_REG(isa32, 6, isar6); + + SANITIZE_GENERIC_REG(mvfr, 0, mvfr0); + SANITIZE_GENERIC_REG(mvfr, 1, mvfr1); +#ifndef MVFR2_MAYBE_UNDEFINED + SANITIZE_REG(mvfr, 2, mvfr2); +#endif + } +} diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index a1ee3146ef..8fdf5e70d3 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -307,12 +307,14 @@ void start_secondary(void) set_processor_id(cpuid); identify_cpu(¤t_cpu_data); + sanitize_cpu(¤t_cpu_data); processor_setup(); init_traps(); +#ifndef CONFIG_ARM_64 /* - * Currently Xen assumes the platform has only one kind of CPUs. + * Currently Xen assumes the platform has only one kind of CPUs on ARM32. * This assumption does not hold on big.LITTLE platform and may * result to instability and insecure platform (unless cpu affinity * is manually specified for all domains). Better to park them for @@ -328,6 +330,7 @@ void start_secondary(void) boot_cpu_data.midr.bits); stop_cpu(); } +#endif if ( dcache_line_bytes != read_dcache_line_bytes() ) { diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h index ba48db3eac..ad34e55cc8 100644 --- a/xen/include/asm-arm/cpufeature.h +++ b/xen/include/asm-arm/cpufeature.h @@ -330,6 +330,15 @@ extern struct cpuinfo_arm boot_cpu_data; extern void identify_cpu(struct cpuinfo_arm *); +#ifdef CONFIG_ARM_64 +extern void sanitize_cpu(const struct cpuinfo_arm *); +#else +static inline void sanitize_cpu(const struct cpuinfo_arm *cpuinfo) +{ + /* Not supported on arm32 */ +} +#endif + extern struct cpuinfo_arm cpu_data[]; #define current_cpu_data cpu_data[smp_processor_id()] diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h index 1198c7c0b2..c6987973bf 100644 --- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -192,6 +192,7 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c); #define TAINT_ERROR_INJECT (1u << 2) #define TAINT_HVM_FEP (1u << 3) #define TAINT_MACHINE_UNSECURE (1u << 4) +#define TAINT_CPU_OUT_OF_SPEC (1u << 5) extern unsigned int tainted; #define TAINT_STRING_MAX_LEN 20 extern char *print_tainted(char *str);
Define a sanitize_cpu function to be called on secondary cores to sanitize the cpuinfo structure from the boot CPU. The safest value is taken when possible and the system is marked tainted if we encounter values which are incompatible with each other. Call the sanitize_cpu function on all secondary cores and remove the code disabling secondary cores if they are not the same as the boot core as we are now able to handle this use case. This is only supported on arm64 so cpu_sanitize is an empty static inline on arm32. This patch also removes the code imported from Linux that will not be used by Xen. Signed-off-by: Bertrand Marquis <bertrand.marquis@arm.com> --- xen/arch/arm/arm64/cpusanitize.c | 236 ++++++++++++++++++++++++------- xen/arch/arm/smpboot.c | 5 +- xen/include/asm-arm/cpufeature.h | 9 ++ xen/include/xen/lib.h | 1 + 4 files changed, 199 insertions(+), 52 deletions(-)