[PATCH] arm64/cpuinfo: Drop boot_cpu_data

[PATCH] arm64/cpuinfo: Drop boot_cpu_data

[Anshuman Khandual]

A global boot_cpu_data is not really required. Lets drop this. While
here, rename the local variable as boot_cpu_info when it is fetched
for the boot cpu.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Suzuki Poulose <suzuki.poulose@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
Based on 5.7-rc4

 arch/arm64/kernel/cpuinfo.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index a515d8f3639e..dabcdc132e56 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -31,7 +31,6 @@
  * values depending on configuration at or after reset.
  */
 DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
-static struct cpuinfo_arm64 boot_cpu_data;
 
 static const char *icache_policy_str[] = {
 	[0 ... ICACHE_POLICY_PIPT]	= "RESERVED/UNKNOWN",
@@ -393,15 +392,16 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
 void cpuinfo_store_cpu(void)
 {
 	struct cpuinfo_arm64 *info = this_cpu_ptr(&cpu_data);
+	struct cpuinfo_arm64 *boot_cpu_info = &per_cpu(cpu_data, 0);
+
 	__cpuinfo_store_cpu(info);
-	update_cpu_features(smp_processor_id(), info, &boot_cpu_data);
+	update_cpu_features(smp_processor_id(), info, boot_cpu_info);
 }
 
 void __init cpuinfo_store_boot_cpu(void)
 {
-	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, 0);
-	__cpuinfo_store_cpu(info);
+	struct cpuinfo_arm64 *boot_cpu_info = &per_cpu(cpu_data, 0);
 
-	boot_cpu_data = *info;
-	init_cpu_features(&boot_cpu_data);
+	__cpuinfo_store_cpu(boot_cpu_info);
+	init_cpu_features(boot_cpu_info);
 }
-- 
2.20.1

Re: [PATCH] arm64/cpuinfo: Drop boot_cpu_data

[Mark Rutland]

On Mon, May 04, 2020 at 06:00:00PM +0530, Anshuman Khandual wrote:
> A global boot_cpu_data is not really required. Lets drop this.

I don't think it's true that this isn't required today.

One reason that we have both boot_cpu_data and a cpu_data variable for
CPU0 is that CPU0 itself can be hotplugged out then back in, and this
allows us to detect if CPU0's features have changed (e.g. due to FW
failing to configure it appropriately, or real physical hotplug
occurring).

So NAK to the patch as it stands. If we're certain we capture all of
those details even without boot_cpu_data, then we should make other
changes to make that clear (e.g. removing it as an argument to
update_cpu_features()).

Thanks,
Mark.

> While here, rename the local variable as boot_cpu_info when it is
> fetched for the boot cpu.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Suzuki Poulose <suzuki.poulose@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Based on 5.7-rc4
> 
>  arch/arm64/kernel/cpuinfo.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index a515d8f3639e..dabcdc132e56 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -31,7 +31,6 @@
>   * values depending on configuration at or after reset.
>   */
>  DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data);
> -static struct cpuinfo_arm64 boot_cpu_data;
>  
>  static const char *icache_policy_str[] = {
>  	[0 ... ICACHE_POLICY_PIPT]	= "RESERVED/UNKNOWN",
> @@ -393,15 +392,16 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
>  void cpuinfo_store_cpu(void)
>  {
>  	struct cpuinfo_arm64 *info = this_cpu_ptr(&cpu_data);
> +	struct cpuinfo_arm64 *boot_cpu_info = &per_cpu(cpu_data, 0);
> +
>  	__cpuinfo_store_cpu(info);
> -	update_cpu_features(smp_processor_id(), info, &boot_cpu_data);
> +	update_cpu_features(smp_processor_id(), info, boot_cpu_info);
>  }
>  
>  void __init cpuinfo_store_boot_cpu(void)
>  {
> -	struct cpuinfo_arm64 *info = &per_cpu(cpu_data, 0);
> -	__cpuinfo_store_cpu(info);
> +	struct cpuinfo_arm64 *boot_cpu_info = &per_cpu(cpu_data, 0);
>  
> -	boot_cpu_data = *info;
> -	init_cpu_features(&boot_cpu_data);
> +	__cpuinfo_store_cpu(boot_cpu_info);
> +	init_cpu_features(boot_cpu_info);
>  }
> -- 
> 2.20.1
> 

Re: [PATCH] arm64/cpuinfo: Drop boot_cpu_data

[Anshuman Khandual]

On 05/04/2020 06:13 PM, Mark Rutland wrote:
> On Mon, May 04, 2020 at 06:00:00PM +0530, Anshuman Khandual wrote:
>> A global boot_cpu_data is not really required. Lets drop this.
> 
> I don't think it's true that this isn't required today.
> 
> One reason that we have both boot_cpu_data and a cpu_data variable for
> CPU0 is that CPU0 itself can be hotplugged out then back in, and this
> allows us to detect if CPU0's features have changed (e.g. due to FW
> failing to configure it appropriately, or real physical hotplug
> occurring).

Understood. After hotplug, CPU0 will come back via secondary_start_kernel()
where it's current register values will be checked against earlier captured
values i.e boot_cpu_data.

But wondering why should CPU0 be treated like any other secondary CPU. IOW
in case the fresh boot CPU register values dont match with boot_cpu_data,
should not the online process just be declined ? AFAICS, current approach
will let the kernel run with taint in case of a mismatch.

> 
> So NAK to the patch as it stands. If we're certain we capture all of
> those details even without boot_cpu_data, then we should make other
> changes to make that clear (e.g. removing it as an argument to
> update_cpu_features()).

There might not be another way, unless we can override CPU0's cpu_data
variable when the boot CPU comes back in after vetting against existing
values. Is there any particular reason to store the very first boot CPU0
info for ever ?

Passing on CPU0's cpu_data variable in update_cpu_features() for secondary
CPUs during boot still make sense. It helps in finalizing register values.
Re-entering CPU0's test against boot_cpu_data seems different.

Re: [PATCH] arm64/cpuinfo: Drop boot_cpu_data

[Mark Rutland]

On Mon, May 04, 2020 at 08:23:08PM +0530, Anshuman Khandual wrote:
> 
> 
> On 05/04/2020 06:13 PM, Mark Rutland wrote:
> > On Mon, May 04, 2020 at 06:00:00PM +0530, Anshuman Khandual wrote:
> >> A global boot_cpu_data is not really required. Lets drop this.
> > 
> > I don't think it's true that this isn't required today.
> > 
> > One reason that we have both boot_cpu_data and a cpu_data variable for
> > CPU0 is that CPU0 itself can be hotplugged out then back in, and this
> > allows us to detect if CPU0's features have changed (e.g. due to FW
> > failing to configure it appropriately, or real physical hotplug
> > occurring).
> 
> Understood. After hotplug, CPU0 will come back via secondary_start_kernel()
> where it's current register values will be checked against earlier captured
> values i.e boot_cpu_data.
> 
> But wondering why should CPU0 be treated like any other secondary CPU. IOW
> in case the fresh boot CPU register values dont match with boot_cpu_data,
> should not the online process just be declined ? AFAICS, current approach
> will let the kernel run with taint in case of a mismatch.

I don't follow. When CPU0 is hotplguged back in it'll follow the
secondary boot path, so it can be rejected as with any other secondary
CPU.

If I'm missing a case, could you please point that out more
specifically?

> > So NAK to the patch as it stands. If we're certain we capture all of
> > those details even without boot_cpu_data, then we should make other
> > changes to make that clear (e.g. removing it as an argument to
> > update_cpu_features()).
> 
> There might not be another way, unless we can override CPU0's cpu_data
> variable when the boot CPU comes back in after vetting against existing
> values. Is there any particular reason to store the very first boot CPU0
> info for ever ?

The reason is so that we can log the values for comparison. Otherwise
we'll have to choose some arbitrary CPU's value in order to do so.

> Passing on CPU0's cpu_data variable in update_cpu_features() for secondary
> CPUs during boot still make sense. It helps in finalizing register values.
> Re-entering CPU0's test against boot_cpu_data seems different.

I think that practically this means we should leave this as-is. If we
need to keep it around for CPU, then we may as well keep it around and
use it consitently for all secondary CPUs.

I'd prefer to leave this as-is given it's simple to reason about.

Thanks,
Mark.