|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 4/7] xen/arm: Sanitize cpuinfo ID registers fields
On Thu, 16 Sep 2021, Bertrand Marquis wrote:
> Define a sanitize_cpu function to be called on secondary cores to
> sanitize the system cpuinfo structure.
>
> 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 update_system_features function on all secondary cores that are
> kept running and taint the system if different midr are found between
> cores but hmp-unsafe=true was passed on Xen command line.
>
> This is only supported on arm64 so update_system_features is an empty
> static inline on arm32.
>
> The patch is adding a new TAINT_CPU_OUT_OF_SPEC to warn the user if
> Xen is running on a system with features differences between cores which
> are not supported.
>
> The patch is disabling CTR_EL0, DCZID_EL0 and ZCRusing #if 0 with a TODO
> as this patch is not handling sanitization of those registers.
> CTR_EL0/DCZID will be handled in a future patch to properly handle
> different cache attributes when possible.
> ZCR should be sanitize once we add support for SVE in Xen.
>
> Signed-off-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> ---
> Changes in v4:
> - arm64_ftr_set_value
> Changes in v3:
> - in case of different midr but hmp-unsafe passed on the command line,
> enable all cores anyway but taint Xen with CPU_OUT_OF_SPEC.
> - use current core info to sanitize cpu only if we keep it on
> Changes in v2:
> - add compilation of cpufeature.c in this patch instead of previous one
> - remove functions reused from linux code and moved to header
> - rename sanitize_cpu to update_system_features
> - change to Linux coding style
> - remove dev comments
> - surround currently not used Linux structures with #if 0 and adapt the
> commit message
> - add missing aa64dfr1 register
> - add TODO for CTR, DCZID and ZCR
> - add CPU_OUT_OF_SPEC support to print_taint
> - use system_cpuinfo instead of boot_cpu_data
> ---
> xen/arch/arm/arm64/Makefile | 1 +
> xen/arch/arm/arm64/cpufeature.c | 117 +++++++++++++++++++++++++++++++
> xen/arch/arm/smpboot.c | 34 ++++++---
> xen/common/kernel.c | 6 +-
> xen/include/asm-arm/cpufeature.h | 9 +++
> xen/include/xen/lib.h | 1 +
> 6 files changed, 158 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
> index 40642ff574..701d66883d 100644
> --- a/xen/arch/arm/arm64/Makefile
> +++ b/xen/arch/arm/arm64/Makefile
> @@ -1,6 +1,7 @@
> obj-y += lib/
>
> obj-y += cache.o
> +obj-y += cpufeature.o
> obj-$(CONFIG_HARDEN_BRANCH_PREDICTOR) += bpi.o
> obj-$(CONFIG_EARLY_PRINTK) += debug.o
> obj-y += domctl.o
> diff --git a/xen/arch/arm/arm64/cpufeature.c b/xen/arch/arm/arm64/cpufeature.c
> index 152086dc9d..58596495a8 100644
> --- a/xen/arch/arm/arm64/cpufeature.c
> +++ b/xen/arch/arm/arm64/cpufeature.c
> @@ -275,6 +275,9 @@ static const struct arm64_ftr_bits ftr_id_aa64mmfr2[] = {
> ARM64_FTR_END,
> };
>
> +#if 0
> +/* TODO: use this to sanitize the cache line size among cores */
> +
> 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),
> @@ -291,6 +294,7 @@ static const struct arm64_ftr_bits ftr_ctr[] = {
> ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE,
> CTR_IMINLINE_SHIFT, 4, 0),
> ARM64_FTR_END,
> };
> +#endif
>
> 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),
> @@ -325,11 +329,14 @@ static const struct arm64_ftr_bits ftr_mvfr2[] = {
> ARM64_FTR_END,
> };
>
> +#if 0
> +/* TODO: handle this when sanitizing cache related registers */
> 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,
> };
> +#endif
>
> 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),
> @@ -444,11 +451,15 @@ static const struct arm64_ftr_bits ftr_id_dfr1[] = {
> ARM64_FTR_END,
> };
>
> +#if 0
> +/* TODO: use this to sanitize SVE once we support it */
> +
> 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,
> };
> +#endif
>
> /*
> * Common ftr bits for a 32bit register with all hidden, strict
> @@ -512,3 +523,109 @@ static s64 arm64_ftr_safe_value(const struct
> arm64_ftr_bits *ftrp, s64 new,
> * End of imported linux structures and code
> */
>
> +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++)
> + {
> + 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;
> +
> + *cur_reg = arm64_ftr_set_value(ftrp, *cur_reg,
> +
> arm64_ftr_safe_value(ftrp, new_field, cur_field));
> + }
> +
> + 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 update_system_features(const struct cpuinfo_arm *new)
> +{
> +
> +#define SANITIZE_REG(field, num, reg) \
> + sanitize_reg(&system_cpuinfo.field.bits[num], new->field.bits[num], \
> + #reg, ftr_##reg)
> +
> +#define SANITIZE_ID_REG(field, num, reg) \
> + sanitize_reg(&system_cpuinfo.field.bits[num], new->field.bits[num], \
> + #reg, ftr_id_##reg)
> +
> +#define SANITIZE_RAZ_REG(field, num, reg) \
> + sanitize_reg(&system_cpuinfo.field.bits[num], new->field.bits[num], \
> + #reg, ftr_raz)
> +
> +#define SANITIZE_GENERIC_REG(field, num, reg) \
> + sanitize_reg(&system_cpuinfo.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);
> + SANITIZE_RAZ_REG(dbg64, 1, aa64dfr1);
> +
> + 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(&system_cpuinfo) )
> + {
> + 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);
> +
> + 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 c9f2827d56..60c0e82fc5 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -318,15 +318,26 @@ void start_secondary(void)
> * is manually specified for all domains). Better to park them for
> * now.
> */
> - if ( !opt_hmp_unsafe &&
> - current_cpu_data.midr.bits != system_cpuinfo.midr.bits )
> + if ( current_cpu_data.midr.bits != system_cpuinfo.midr.bits )
> {
> - printk(XENLOG_ERR
> - "CPU%u MIDR (0x%"PRIregister") does not match boot CPU MIDR
> (0x%"PRIregister"),\n"
> - XENLOG_ERR "disable cpu (see big.LITTLE.txt under docs/).\n",
> - smp_processor_id(), current_cpu_data.midr.bits,
> - system_cpuinfo.midr.bits);
> - stop_cpu();
> + if ( !opt_hmp_unsafe )
> + {
> + printk(XENLOG_ERR
> + "CPU%u MIDR (0x%"PRIregister") does not match boot CPU
> MIDR (0x%"PRIregister"),\n"
> + XENLOG_ERR "disable cpu (see big.LITTLE.txt under
> docs/).\n",
> + smp_processor_id(), current_cpu_data.midr.bits,
> + system_cpuinfo.midr.bits);
> + stop_cpu();
> + }
> + else
> + {
> + printk(XENLOG_ERR
> + "CPU%u MIDR (0x%"PRIregister") does not match boot CPU
> MIDR (0x%"PRIregister"),\n"
> + XENLOG_ERR "hmp-unsafe turned on so tainting Xen and keep
> core on!!\n",
> + smp_processor_id(), current_cpu_data.midr.bits,
> + system_cpuinfo.midr.bits);
> + add_taint(TAINT_CPU_OUT_OF_SPEC);
> + }
> }
>
> if ( dcache_line_bytes != read_dcache_line_bytes() )
> @@ -337,6 +348,13 @@ void start_secondary(void)
> stop_cpu();
> }
>
> + /*
> + * system features must be updated only if we do not stop the core or
> + * we might disable features due to a non used core (for example when
> + * booting on big cores on a big.LITTLE system with hmp_unsafe)
> + */
> + update_system_features(¤t_cpu_data);
> +
> mmu_init_secondary_cpu();
>
> gic_init_secondary_cpu();
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index d77756a81e..e119e5401f 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -327,6 +327,7 @@ unsigned int tainted;
> * 'H' - HVM forced emulation prefix is permitted.
> * 'M' - Machine had a machine check experience.
> * 'U' - Platform is unsecure (usually due to an errata on the platform).
> + * 'S' - Out of spec CPU (One core has a feature incompatible with others).
> *
> * The string is overwritten by the next call to print_taint().
> */
> @@ -334,12 +335,13 @@ char *print_tainted(char *str)
> {
> if ( tainted )
> {
> - snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c",
> + snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c%c",
> tainted & TAINT_MACHINE_UNSECURE ? 'U' : ' ',
> tainted & TAINT_MACHINE_CHECK ? 'M' : ' ',
> tainted & TAINT_SYNC_CONSOLE ? 'C' : ' ',
> tainted & TAINT_ERROR_INJECT ? 'E' : ' ',
> - tainted & TAINT_HVM_FEP ? 'H' : ' ');
> + tainted & TAINT_HVM_FEP ? 'H' : ' ',
> + tainted & TAINT_CPU_OUT_OF_SPEC ? 'S' : ' ');
> }
> else
> {
> diff --git a/xen/include/asm-arm/cpufeature.h
> b/xen/include/asm-arm/cpufeature.h
> index 8f2b8e7830..52cb3133e0 100644
> --- a/xen/include/asm-arm/cpufeature.h
> +++ b/xen/include/asm-arm/cpufeature.h
> @@ -330,6 +330,15 @@ extern struct cpuinfo_arm system_cpuinfo;
>
> extern void identify_cpu(struct cpuinfo_arm *);
>
> +#ifdef CONFIG_ARM_64
> +extern void update_system_features(const struct cpuinfo_arm *);
> +#else
> +static inline void update_system_features(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);
> --
> 2.17.1
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |