[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC PATCH 3/4] xen/arm: Sanitize cpuinfo ID registers fields
Hi Julien, > On 12 Jul 2021, at 11:16, Julien Grall <julien@xxxxxxx> 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@xxxxxxx> >> --- >> 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |