All of lore.kernel.org
 help / color / mirror / Atom feed
From: will.deacon@arm.com (Will Deacon)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 5/5] arm64: Ensure the secondary CPUs have safe ASIDBits size
Date: Mon, 23 Nov 2015 17:29:20 +0000	[thread overview]
Message-ID: <20151123172920.GB1702@arm.com> (raw)
In-Reply-To: <1447866540-23207-5-git-send-email-suzuki.poulose@arm.com>

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@arm.com>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  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

WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will.deacon@arm.com>
To: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
Cc: linux-arm-kernel@lists.infradead.org, catalin.marinas@arm.com,
	mark.rutland@arm.com, ard.biesheuvel@linaro.org,
	linux-kernel@vger.kernel.org, takahiro.akashi@linaro.org
Subject: Re: [PATCH 5/5] arm64: Ensure the secondary CPUs have safe ASIDBits size
Date: Mon, 23 Nov 2015 17:29:20 +0000	[thread overview]
Message-ID: <20151123172920.GB1702@arm.com> (raw)
In-Reply-To: <1447866540-23207-5-git-send-email-suzuki.poulose@arm.com>

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@arm.com>
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  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

  reply	other threads:[~2015-11-23 17:29 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-11-18 17:03 [PATCH 0/5] arm64: cpufeature: Fixes for 4.4-rc1 Suzuki K. Poulose
2015-11-18 17:03 ` Suzuki K. Poulose
2015-11-18 17:08 ` [PATCH 1/5] arm64: cpufeature: Add helpers for extracting unsigned values Suzuki K. Poulose
2015-11-18 17:08   ` Suzuki K. Poulose
2015-11-18 17:08   ` [PATCH 2/5] arm64: cpufeature: Track unsigned fields Suzuki K. Poulose
2015-11-18 17:08     ` Suzuki K. Poulose
2015-11-19  4:57     ` AKASHI Takahiro
2015-11-19  4:57       ` AKASHI Takahiro
2015-11-19 10:03       ` Suzuki K. Poulose
2015-11-19 10:03         ` Suzuki K. Poulose
2015-11-19 10:06         ` Suzuki K. Poulose
2015-11-19 10:06           ` Suzuki K. Poulose
2015-11-19 18:45         ` Catalin Marinas
2015-11-19 18:45           ` Catalin Marinas
2015-11-20 13:37           ` Suzuki K. Poulose
2015-11-20 13:37             ` Suzuki K. Poulose
2015-11-18 17:08   ` [PATCH 3/5] arm64: debug: Treat the BRPs/WRPs as unsigned Suzuki K. Poulose
2015-11-18 17:08     ` Suzuki K. Poulose
2015-11-23 17:29     ` Will Deacon
2015-11-23 17:29       ` Will Deacon
2015-11-18 17:08   ` [PATCH 4/5] arm64: Make fail_incapable_cpu reusable Suzuki K. Poulose
2015-11-18 17:08     ` Suzuki K. Poulose
2015-11-18 17:09   ` [PATCH 5/5] arm64: Ensure the secondary CPUs have safe ASIDBits size Suzuki K. Poulose
2015-11-18 17:09     ` Suzuki K. Poulose
2015-11-23 17:29     ` Will Deacon [this message]
2015-11-23 17:29       ` Will Deacon
2015-11-23 23:48       ` Suzuki K. Poulose
2015-11-23 23:48         ` Suzuki K. Poulose
2015-11-26 18:13 ` [PATCH 0/5] arm64: cpufeature: Fixes for 4.4-rc1 Catalin Marinas
2015-11-26 18:13   ` Catalin Marinas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20151123172920.GB1702@arm.com \
    --to=will.deacon@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.