Re: [PATCH 5/5] arm64: Ensure the secondary CPUs have safe ASIDBits size

[Will Deacon]

On Wed, Nov 18, 2015 at 05:09:00PM +0000, Suzuki K. Poulose wrote:
> The ID_AA64MMFR0_EL1:ASIDBits determines the size of the mm context
> id and is used in the early boot to make decisions. The value is
> picked up from the Boot CPU and cannot be delayed until other CPUs
> are up. If a secondary CPU has a smaller size than that of the Boot
> CPU, things will break horribly and the usual SANITY check is not good
> enough to prevent the system from crashing. Prevent this by failing CPUs with
> ASID smaller than that of the boot CPU.
> 
> Also moves the fail_incapable_cpu() out of the CONFIG_HOTPLUG_CPU.
> 
> Cc: Will Deacon <will.deacon@xxxxxxx>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@xxxxxxx>
> ---
>  arch/arm64/kernel/cpufeature.c |   81 +++++++++++++++++++++++++++++-----------
>  1 file changed, 59 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 5629f2c..769782a 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -293,6 +293,28 @@ static struct arm64_ftr_reg arm64_ftr_regs[] = {
>  	ARM64_FTR_REG(SYS_CNTFRQ_EL0, ftr_generic32),
>  };
>  
> +/*
> + * Park the calling CPU which doesn't have the capability
> + * as advertised by the system.
> + */
> +static void fail_incapable_cpu(void)
> +{
> +	int cpu = smp_processor_id();
> +
> +	pr_crit("CPU%d: will not boot\n", cpu);

This is less informative than the current message (whcih describes the
missing capability).

> +
> +	/* Mark this CPU absent */
> +	set_cpu_present(cpu, 0);
> +
> +	/* Check if we can park ourselves */
> +	if (cpu_ops[cpu] && cpu_ops[cpu]->cpu_die)
> +		cpu_ops[cpu]->cpu_die(cpu);
> +	asm(
> +	"1:	wfe\n"
> +	"	wfi\n"
> +	"	b	1b");
> +}
> +
>  static int search_cmp_ftr_reg(const void *id, const void *regp)
>  {
>  	return (int)(unsigned long)id - (int)((const struct arm64_ftr_reg *)regp)->sys_id;
> @@ -459,6 +481,40 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
>  }
>  
>  /*
> + * The asid_bits, which determine the width of the mm context
> + * id, is based on the boot CPU value. If the new CPU doesn't
> + * have an ASID >= boot CPU, we are in trouble. Fail this CPU.
> + */
> +static void check_cpu_asid_bits(int cpu,
> +				struct cpuinfo_arm64 *info,
> +				struct cpuinfo_arm64 *boot)
> +{
> +	u32 asid_boot = cpuid_feature_extract_unsigned_field(boot->reg_id_aa64mmfr0,
> +							ID_AA64MMFR0_ASID_SHIFT);
> +	u32 asid_cur = cpuid_feature_extract_unsigned_field(info->reg_id_aa64mmfr0,
> +							ID_AA64MMFR0_ASID_SHIFT);
> +	if (asid_cur < asid_boot) {
> +		pr_crit("CPU%d: has incompatible ASIDBits: %u vs Boot CPU:%u\n",
> +				cpu, asid_cur, asid_boot);
> +		fail_incapable_cpu();
> +	}

Hmm. Whilst we want to ensure that secondary CPUs don't have a smaller
ASID size than the boot CPU, can we actually guarantee that a smaller
value for ID_AA64MMFR0.ASIDBits corresponds to fewer bits? We're
probably better off assuming 8-bit ASIDs unless ASIDBits == 2 (which is
what the ASID allocator does).

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/