diff mbox series

[RFC,v2,06/35] arm64: setup: Switch over to GENERIC_CPU_DEVICES using arch_register_cpu()

Message ID 20230913163823.7880-7-james.morse@arm.com (mailing list archive)
State Handled Elsewhere
Headers show
Series ACPI/arm64: add support for virtual cpuhotplug | expand

Checks

Context Check Description
conchuod/cover_letter success Series has a cover letter
conchuod/tree_selection success Guessed tree name to be for-next at HEAD 0bb80ecc33a8
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 5 and now 5
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 9 this patch: 9
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 9 this patch: 9
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 25 this patch: 25
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 37 lines checked
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

James Morse Sept. 13, 2023, 4:37 p.m. UTC
To allow ACPI's _STA value to hide CPUs that are present, but not
available to online right now due to VMM or firmware policy, the
register_cpu() call needs to be made by the ACPI machinery when ACPI
is in use. This allows it to hide CPUs that are unavailable from sysfs.

Switching to GENERIC_CPU_DEVICES is an intermediate step to allow all
five ACPI architectures to be modified at once.

Switch over to GENERIC_CPU_DEVICES, and provide an arch_register_cpu()
that populates the hotpluggable flag. arch_register_cpu() is also the
interface the ACPI machinery expects.

The struct cpu in struct cpuinfo_arm64 is never used directly, remove
it to use the one GENERIC_CPU_DEVICES provides.

This changes the CPUs visible in sysfs from possible to present, but
on arm64 smp_prepare_cpus() ensures these are the same.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/arm64/Kconfig           |  1 +
 arch/arm64/include/asm/cpu.h |  1 -
 arch/arm64/kernel/setup.c    | 13 ++++---------
 3 files changed, 5 insertions(+), 10 deletions(-)

Comments

Russell King (Oracle) Sept. 14, 2023, 9:56 a.m. UTC | #1
On Wed, Sep 13, 2023 at 04:37:54PM +0000, James Morse wrote:
> To allow ACPI's _STA value to hide CPUs that are present, but not
> available to online right now due to VMM or firmware policy, the
> register_cpu() call needs to be made by the ACPI machinery when ACPI
> is in use. This allows it to hide CPUs that are unavailable from sysfs.
> 
> Switching to GENERIC_CPU_DEVICES is an intermediate step to allow all
> five ACPI architectures to be modified at once.
> 
> Switch over to GENERIC_CPU_DEVICES, and provide an arch_register_cpu()
> that populates the hotpluggable flag. arch_register_cpu() is also the
> interface the ACPI machinery expects.
> 
> The struct cpu in struct cpuinfo_arm64 is never used directly, remove
> it to use the one GENERIC_CPU_DEVICES provides.
> 
> This changes the CPUs visible in sysfs from possible to present, but
> on arm64 smp_prepare_cpus() ensures these are the same.
> 
> Signed-off-by: James Morse <james.morse@arm.com>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Jonathan Cameron Sept. 14, 2023, 11:27 a.m. UTC | #2
On Wed, 13 Sep 2023 16:37:54 +0000
James Morse <james.morse@arm.com> wrote:

> To allow ACPI's _STA value to hide CPUs that are present, but not
> available to online right now due to VMM or firmware policy, the
> register_cpu() call needs to be made by the ACPI machinery when ACPI
> is in use. This allows it to hide CPUs that are unavailable from sysfs.
> 
> Switching to GENERIC_CPU_DEVICES is an intermediate step to allow all
> five ACPI architectures to be modified at once.
> 
> Switch over to GENERIC_CPU_DEVICES, and provide an arch_register_cpu()
> that populates the hotpluggable flag. arch_register_cpu() is also the
> interface the ACPI machinery expects.
> 
> The struct cpu in struct cpuinfo_arm64 is never used directly, remove
> it to use the one GENERIC_CPU_DEVICES provides.
> 
> This changes the CPUs visible in sysfs from possible to present, but
> on arm64 smp_prepare_cpus() ensures these are the same.
> 
> Signed-off-by: James Morse <james.morse@arm.com>

After this the earlier question about ordering of cpu_dev_init()
and node_dev_init() is relevant.   

Why won't node_dev_init() call
get_cpu_devce() which queries per_cpu(cpu_sys_devices)
and get NULL as we haven't yet filled that in?

Or does it do so but that doesn't matter as well create the
relevant links later?

I've not had enough coffee yet today so might be missing the
obvious!

Jonathan

> ---
>  arch/arm64/Kconfig           |  1 +
>  arch/arm64/include/asm/cpu.h |  1 -
>  arch/arm64/kernel/setup.c    | 13 ++++---------
>  3 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index b10515c0200b..7b3990abf87a 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -132,6 +132,7 @@ config ARM64
>  	select GENERIC_ARCH_TOPOLOGY
>  	select GENERIC_CLOCKEVENTS_BROADCAST
>  	select GENERIC_CPU_AUTOPROBE
> +	select GENERIC_CPU_DEVICES
>  	select GENERIC_CPU_VULNERABILITIES
>  	select GENERIC_EARLY_IOREMAP
>  	select GENERIC_IDLE_POLL_SETUP
> diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
> index e749838b9c5d..887bd0d992bb 100644
> --- a/arch/arm64/include/asm/cpu.h
> +++ b/arch/arm64/include/asm/cpu.h
> @@ -38,7 +38,6 @@ struct cpuinfo_32bit {
>  };
>  
>  struct cpuinfo_arm64 {
> -	struct cpu	cpu;
>  	struct kobject	kobj;
>  	u64		reg_ctr;
>  	u64		reg_cntfrq;
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 417a8a86b2db..165bd2c0dd5a 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -402,19 +402,14 @@ static inline bool cpu_can_disable(unsigned int cpu)
>  	return false;
>  }
>  
> -static int __init topology_init(void)
> +int arch_register_cpu(int num)
>  {
> -	int i;
> +	struct cpu *cpu = &per_cpu(cpu_devices, num);
>  
> -	for_each_possible_cpu(i) {
> -		struct cpu *cpu = &per_cpu(cpu_data.cpu, i);
> -		cpu->hotpluggable = cpu_can_disable(i);
> -		register_cpu(cpu, i);
> -	}
> +	cpu->hotpluggable = cpu_can_disable(num);
>  
> -	return 0;
> +	return register_cpu(cpu, num);
>  }
> -subsys_initcall(topology_init);
>  
>  static void dump_kernel_offset(void)
>  {
Russell King (Oracle) Sept. 14, 2023, 2:07 p.m. UTC | #3
On Thu, Sep 14, 2023 at 12:27:15PM +0100, Jonathan Cameron wrote:
> On Wed, 13 Sep 2023 16:37:54 +0000
> James Morse <james.morse@arm.com> wrote:
> 
> > To allow ACPI's _STA value to hide CPUs that are present, but not
> > available to online right now due to VMM or firmware policy, the
> > register_cpu() call needs to be made by the ACPI machinery when ACPI
> > is in use. This allows it to hide CPUs that are unavailable from sysfs.
> > 
> > Switching to GENERIC_CPU_DEVICES is an intermediate step to allow all
> > five ACPI architectures to be modified at once.
> > 
> > Switch over to GENERIC_CPU_DEVICES, and provide an arch_register_cpu()
> > that populates the hotpluggable flag. arch_register_cpu() is also the
> > interface the ACPI machinery expects.
> > 
> > The struct cpu in struct cpuinfo_arm64 is never used directly, remove
> > it to use the one GENERIC_CPU_DEVICES provides.
> > 
> > This changes the CPUs visible in sysfs from possible to present, but
> > on arm64 smp_prepare_cpus() ensures these are the same.
> > 
> > Signed-off-by: James Morse <james.morse@arm.com>
> 
> After this the earlier question about ordering of cpu_dev_init()
> and node_dev_init() is relevant.   
> 
> Why won't node_dev_init() call
> get_cpu_devce() which queries per_cpu(cpu_sys_devices)
> and get NULL as we haven't yet filled that in?
> 
> Or does it do so but that doesn't matter as well create the
> relevant links later?

node_dev_init() will walk through the nodes calling register_one_node()
on each. This will trickle down to __register_one_node() which walks
all present CPUs, calling register_cpu_under_node() on each.

register_cpu_under_node() will call get_cpu_device(cpu) for each and
will return NULL until the CPU is registered using register_cpu(),
which will now happen _after_ node_dev_init().

So, at this point, CPUs won't get registered, and initially one might
think that's a problem.

However, register_cpu() will itself call register_cpu_under_node(),
where get_cpu_device() will return the now populated entry, and the
sysfs links will be created.

So, I think what you've spotted is a potential chunk of code that
isn't necessary when using GENERIC_CPU_DEVICES after this change!
Jonathan Cameron Sept. 14, 2023, 2:56 p.m. UTC | #4
On Thu, 14 Sep 2023 15:07:22 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Thu, Sep 14, 2023 at 12:27:15PM +0100, Jonathan Cameron wrote:
> > On Wed, 13 Sep 2023 16:37:54 +0000
> > James Morse <james.morse@arm.com> wrote:
> >   
> > > To allow ACPI's _STA value to hide CPUs that are present, but not
> > > available to online right now due to VMM or firmware policy, the
> > > register_cpu() call needs to be made by the ACPI machinery when ACPI
> > > is in use. This allows it to hide CPUs that are unavailable from sysfs.
> > > 
> > > Switching to GENERIC_CPU_DEVICES is an intermediate step to allow all
> > > five ACPI architectures to be modified at once.
> > > 
> > > Switch over to GENERIC_CPU_DEVICES, and provide an arch_register_cpu()
> > > that populates the hotpluggable flag. arch_register_cpu() is also the
> > > interface the ACPI machinery expects.
> > > 
> > > The struct cpu in struct cpuinfo_arm64 is never used directly, remove
> > > it to use the one GENERIC_CPU_DEVICES provides.
> > > 
> > > This changes the CPUs visible in sysfs from possible to present, but
> > > on arm64 smp_prepare_cpus() ensures these are the same.
> > > 
> > > Signed-off-by: James Morse <james.morse@arm.com>  
> > 
> > After this the earlier question about ordering of cpu_dev_init()
> > and node_dev_init() is relevant.   
> > 
> > Why won't node_dev_init() call
> > get_cpu_devce() which queries per_cpu(cpu_sys_devices)
> > and get NULL as we haven't yet filled that in?
> > 
> > Or does it do so but that doesn't matter as well create the
> > relevant links later?  
> 
> node_dev_init() will walk through the nodes calling register_one_node()
> on each. This will trickle down to __register_one_node() which walks
> all present CPUs, calling register_cpu_under_node() on each.
> 
> register_cpu_under_node() will call get_cpu_device(cpu) for each and
> will return NULL until the CPU is registered using register_cpu(),
> which will now happen _after_ node_dev_init().
> 
> So, at this point, CPUs won't get registered, and initially one might
> think that's a problem.
> 
> However, register_cpu() will itself call register_cpu_under_node(),
> where get_cpu_device() will return the now populated entry, and the
> sysfs links will be created.
> 
> So, I think what you've spotted is a potential chunk of code that
> isn't necessary when using GENERIC_CPU_DEVICES after this change!
> 

Makes sense thanks. I was just being too lazy to check and bouncing it back
at James! *looks guilty*

Jonathan
diff mbox series

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index b10515c0200b..7b3990abf87a 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -132,6 +132,7 @@  config ARM64
 	select GENERIC_ARCH_TOPOLOGY
 	select GENERIC_CLOCKEVENTS_BROADCAST
 	select GENERIC_CPU_AUTOPROBE
+	select GENERIC_CPU_DEVICES
 	select GENERIC_CPU_VULNERABILITIES
 	select GENERIC_EARLY_IOREMAP
 	select GENERIC_IDLE_POLL_SETUP
diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
index e749838b9c5d..887bd0d992bb 100644
--- a/arch/arm64/include/asm/cpu.h
+++ b/arch/arm64/include/asm/cpu.h
@@ -38,7 +38,6 @@  struct cpuinfo_32bit {
 };
 
 struct cpuinfo_arm64 {
-	struct cpu	cpu;
 	struct kobject	kobj;
 	u64		reg_ctr;
 	u64		reg_cntfrq;
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 417a8a86b2db..165bd2c0dd5a 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -402,19 +402,14 @@  static inline bool cpu_can_disable(unsigned int cpu)
 	return false;
 }
 
-static int __init topology_init(void)
+int arch_register_cpu(int num)
 {
-	int i;
+	struct cpu *cpu = &per_cpu(cpu_devices, num);
 
-	for_each_possible_cpu(i) {
-		struct cpu *cpu = &per_cpu(cpu_data.cpu, i);
-		cpu->hotpluggable = cpu_can_disable(i);
-		register_cpu(cpu, i);
-	}
+	cpu->hotpluggable = cpu_can_disable(num);
 
-	return 0;
+	return register_cpu(cpu, num);
 }
-subsys_initcall(topology_init);
 
 static void dump_kernel_offset(void)
 {