| Message ID | 20230913163823.7880-7-james.morse@arm.com |
|---|---|
| State | Accepted |
| Commit | d127db1a23c94a876557b5bf8ca8bea49e8debb6 |
| Headers | show |
| Series | ACPI/arm64: add support for virtual cpuhotplug | expand |
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>
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) > {
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!
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 --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) {
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(-)