| Message ID | 20230913163823.7880-25-james.morse@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI/arm64: add support for virtual cpuhotplug | expand |
On Wed, 13 Sep 2023 16:38:12 +0000 James Morse <james.morse@arm.com> wrote: > Add arch_unregister_cpu() to allow the ACPI machinery to call > unregister_cpu(). This is enough for arm64, riscv and loongarch, but > needs to be overridden by x86 and ia64 who need to do more work. > > CC: Jean-Philippe Brucker <jean-philippe@linaro.org> > Signed-off-by: James Morse <james.morse@arm.com> Ah. Was thinking this should happen in an earlier patch. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > Changes since v1: > * Added CONFIG_HOTPLUG_CPU ifdeffery around unregister_cpu > --- > arch/ia64/include/asm/cpu.h | 4 ---- > arch/loongarch/include/asm/cpu.h | 6 ------ > arch/x86/include/asm/cpu.h | 1 - > drivers/base/cpu.c | 9 ++++++++- > 4 files changed, 8 insertions(+), 12 deletions(-) > > diff --git a/arch/ia64/include/asm/cpu.h b/arch/ia64/include/asm/cpu.h > index a3e690e685e5..642d71675ddb 100644 > --- a/arch/ia64/include/asm/cpu.h > +++ b/arch/ia64/include/asm/cpu.h > @@ -15,8 +15,4 @@ DECLARE_PER_CPU(struct ia64_cpu, cpu_devices); > > DECLARE_PER_CPU(int, cpu_state); > > -#ifdef CONFIG_HOTPLUG_CPU > -extern void arch_unregister_cpu(int); > -#endif > - > #endif /* _ASM_IA64_CPU_H_ */ > diff --git a/arch/loongarch/include/asm/cpu.h b/arch/loongarch/include/asm/cpu.h > index b8568e637420..48b9f7168bcc 100644 > --- a/arch/loongarch/include/asm/cpu.h > +++ b/arch/loongarch/include/asm/cpu.h > @@ -128,10 +128,4 @@ enum cpu_type_enum { > #define LOONGARCH_CPU_HYPERVISOR BIT_ULL(CPU_FEATURE_HYPERVISOR) > #define LOONGARCH_CPU_PTW BIT_ULL(CPU_FEATURE_PTW) > > -#if !defined(__ASSEMBLY__) > -#ifdef CONFIG_HOTPLUG_CPU > -void arch_unregister_cpu(int cpu); > -#endif > -#endif /* ! __ASSEMBLY__ */ > - > #endif /* _ASM_CPU_H */ > diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h > index f349c94510e8..91867a6a9f8e 100644 > --- a/arch/x86/include/asm/cpu.h > +++ b/arch/x86/include/asm/cpu.h > @@ -24,7 +24,6 @@ static inline void prefill_possible_map(void) {} > #endif /* CONFIG_SMP */ > > #ifdef CONFIG_HOTPLUG_CPU > -extern void arch_unregister_cpu(int); > extern void soft_restart_cpu(void); > #endif > > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > index 677f963e02ce..c709747c4a18 100644 > --- a/drivers/base/cpu.c > +++ b/drivers/base/cpu.c > @@ -531,7 +531,14 @@ int __weak arch_register_cpu(int cpu) > { > return register_cpu(&per_cpu(cpu_devices, cpu), cpu); > } > -#endif > + > +#ifdef CONFIG_HOTPLUG_CPU > +void __weak arch_unregister_cpu(int num) > +{ > + unregister_cpu(&per_cpu(cpu_devices, num)); > +} > +#endif /* CONFIG_HOTPLUG_CPU */ > +#endif /* CONFIG_GENERIC_CPU_DEVICES */ > > static void __init cpu_dev_register_generic(void) > {
On 9/14/23 02:38, James Morse wrote: > Add arch_unregister_cpu() to allow the ACPI machinery to call > unregister_cpu(). This is enough for arm64, riscv and loongarch, but > needs to be overridden by x86 and ia64 who need to do more work. > > CC: Jean-Philippe Brucker <jean-philippe@linaro.org> > Signed-off-by: James Morse <james.morse@arm.com> > --- > Changes since v1: > * Added CONFIG_HOTPLUG_CPU ifdeffery around unregister_cpu > --- > arch/ia64/include/asm/cpu.h | 4 ---- > arch/loongarch/include/asm/cpu.h | 6 ------ > arch/x86/include/asm/cpu.h | 1 - > drivers/base/cpu.c | 9 ++++++++- > 4 files changed, 8 insertions(+), 12 deletions(-) > I agree with Jonathan this patch needs to come early. Maybe move this before the following one: [RFC PATCH v2 19/35] ACPI: Move acpi_bus_trim_one() before acpi_scan_hot_remove() > diff --git a/arch/ia64/include/asm/cpu.h b/arch/ia64/include/asm/cpu.h > index a3e690e685e5..642d71675ddb 100644 > --- a/arch/ia64/include/asm/cpu.h > +++ b/arch/ia64/include/asm/cpu.h > @@ -15,8 +15,4 @@ DECLARE_PER_CPU(struct ia64_cpu, cpu_devices); > > DECLARE_PER_CPU(int, cpu_state); > > -#ifdef CONFIG_HOTPLUG_CPU > -extern void arch_unregister_cpu(int); > -#endif > - > #endif /* _ASM_IA64_CPU_H_ */ > diff --git a/arch/loongarch/include/asm/cpu.h b/arch/loongarch/include/asm/cpu.h > index b8568e637420..48b9f7168bcc 100644 > --- a/arch/loongarch/include/asm/cpu.h > +++ b/arch/loongarch/include/asm/cpu.h > @@ -128,10 +128,4 @@ enum cpu_type_enum { > #define LOONGARCH_CPU_HYPERVISOR BIT_ULL(CPU_FEATURE_HYPERVISOR) > #define LOONGARCH_CPU_PTW BIT_ULL(CPU_FEATURE_PTW) > > -#if !defined(__ASSEMBLY__) > -#ifdef CONFIG_HOTPLUG_CPU > -void arch_unregister_cpu(int cpu); > -#endif > -#endif /* ! __ASSEMBLY__ */ > - > #endif /* _ASM_CPU_H */ > diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h > index f349c94510e8..91867a6a9f8e 100644 > --- a/arch/x86/include/asm/cpu.h > +++ b/arch/x86/include/asm/cpu.h > @@ -24,7 +24,6 @@ static inline void prefill_possible_map(void) {} > #endif /* CONFIG_SMP */ > > #ifdef CONFIG_HOTPLUG_CPU > -extern void arch_unregister_cpu(int); > extern void soft_restart_cpu(void); > #endif > > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > index 677f963e02ce..c709747c4a18 100644 > --- a/drivers/base/cpu.c > +++ b/drivers/base/cpu.c > @@ -531,7 +531,14 @@ int __weak arch_register_cpu(int cpu) > { > return register_cpu(&per_cpu(cpu_devices, cpu), cpu); > } > -#endif > + > +#ifdef CONFIG_HOTPLUG_CPU > +void __weak arch_unregister_cpu(int num) > +{ > + unregister_cpu(&per_cpu(cpu_devices, num)); > +} > +#endif /* CONFIG_HOTPLUG_CPU */ > +#endif /* CONFIG_GENERIC_CPU_DEVICES */ > It seems conflicting with its declaration in include/linux/cpu.h. Besides, the function is still needed by drivers/acpi/acpi_processor.c::acpi_processor_make_not_present() even both CONFIG_HOTPLUG_CPU and CONFIG_GENERIC_CPU_DEVICES are disabled? > static void __init cpu_dev_register_generic(void) > { Thanks, Gavin
On Tue, Sep 19, 2023 at 10:59:23AM +1000, Gavin Shan wrote: > On 9/14/23 02:38, James Morse wrote: > > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > > index 677f963e02ce..c709747c4a18 100644 > > --- a/drivers/base/cpu.c > > +++ b/drivers/base/cpu.c > > @@ -531,7 +531,14 @@ int __weak arch_register_cpu(int cpu) > > { > > return register_cpu(&per_cpu(cpu_devices, cpu), cpu); > > } > > -#endif > > + > > +#ifdef CONFIG_HOTPLUG_CPU > > +void __weak arch_unregister_cpu(int num) > > +{ > > + unregister_cpu(&per_cpu(cpu_devices, num)); > > +} > > +#endif /* CONFIG_HOTPLUG_CPU */ > > +#endif /* CONFIG_GENERIC_CPU_DEVICES */ > > It seems conflicting with its declaration in include/linux/cpu.h. How so? The declaration is: extern void arch_unregister_cpu(int cpu); So: void __weak arch_unregister_cpu(int num) is compatible. > Besides, the function is still needed by > drivers/acpi/acpi_processor.c::acpi_processor_make_not_present() > even both CONFIG_HOTPLUG_CPU and CONFIG_GENERIC_CPU_DEVICES are disabled? Yes, I agree - it needs to be present when ACPI is built, so I'm thinking the right solution is to move it out from under at least CONFIG_HOTPLUG_CPU. It can't be moved out from under CONFIG_GENERIC_CPU_DEVICES because then we end up referencing the per-cpu variable cpu_devices which only exists when CONFIG_GENERIC_CPU_DEVICES is enabled. Is that a problem though, because in the case of !CONFIG_GENERIC_CPU_DEVICES, aren't architectures required to provide both arch_.*register_cpu() functions? Thanks.
On Mon, Oct 23, 2023 at 09:44:50AM +0100, Russell King (Oracle) wrote: > On Tue, Sep 19, 2023 at 10:59:23AM +1000, Gavin Shan wrote: > > On 9/14/23 02:38, James Morse wrote: > > > diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c > > > index 677f963e02ce..c709747c4a18 100644 > > > --- a/drivers/base/cpu.c > > > +++ b/drivers/base/cpu.c > > > @@ -531,7 +531,14 @@ int __weak arch_register_cpu(int cpu) > > > { > > > return register_cpu(&per_cpu(cpu_devices, cpu), cpu); > > > } > > > -#endif > > > + > > > +#ifdef CONFIG_HOTPLUG_CPU > > > +void __weak arch_unregister_cpu(int num) > > > +{ > > > + unregister_cpu(&per_cpu(cpu_devices, num)); > > > +} > > > +#endif /* CONFIG_HOTPLUG_CPU */ > > > +#endif /* CONFIG_GENERIC_CPU_DEVICES */ > > > > It seems conflicting with its declaration in include/linux/cpu.h. > > How so? The declaration is: > > extern void arch_unregister_cpu(int cpu); > > So: > > void __weak arch_unregister_cpu(int num) > > is compatible. > > > Besides, the function is still needed by > > drivers/acpi/acpi_processor.c::acpi_processor_make_not_present() > > even both CONFIG_HOTPLUG_CPU and CONFIG_GENERIC_CPU_DEVICES are disabled? > > Yes, I agree - it needs to be present when ACPI is built, so I'm > thinking the right solution is to move it out from under at least > CONFIG_HOTPLUG_CPU. > > It can't be moved out from under CONFIG_GENERIC_CPU_DEVICES because > then we end up referencing the per-cpu variable cpu_devices which only > exists when CONFIG_GENERIC_CPU_DEVICES is enabled. Is that a problem > though, because in the case of !CONFIG_GENERIC_CPU_DEVICES, aren't > architectures required to provide both arch_.*register_cpu() functions? I'm also wondering why this patch isn't part of: "drivers: base: Allow parts of GENERIC_CPU_DEVICES to be overridden" because it seems to be doing something very similar. The commit I refer to introduces a weak version of arch_register_cpu(), and it seems it would also be appropriate to introduce a weak version of its unregister paired function at the same time. Any existing definitions of non-weak arch_unregister_cpu() would override it so it shouldn't cause any issues. Thanks.
diff --git a/arch/ia64/include/asm/cpu.h b/arch/ia64/include/asm/cpu.h index a3e690e685e5..642d71675ddb 100644 --- a/arch/ia64/include/asm/cpu.h +++ b/arch/ia64/include/asm/cpu.h @@ -15,8 +15,4 @@ DECLARE_PER_CPU(struct ia64_cpu, cpu_devices); DECLARE_PER_CPU(int, cpu_state); -#ifdef CONFIG_HOTPLUG_CPU -extern void arch_unregister_cpu(int); -#endif - #endif /* _ASM_IA64_CPU_H_ */ diff --git a/arch/loongarch/include/asm/cpu.h b/arch/loongarch/include/asm/cpu.h index b8568e637420..48b9f7168bcc 100644 --- a/arch/loongarch/include/asm/cpu.h +++ b/arch/loongarch/include/asm/cpu.h @@ -128,10 +128,4 @@ enum cpu_type_enum { #define LOONGARCH_CPU_HYPERVISOR BIT_ULL(CPU_FEATURE_HYPERVISOR) #define LOONGARCH_CPU_PTW BIT_ULL(CPU_FEATURE_PTW) -#if !defined(__ASSEMBLY__) -#ifdef CONFIG_HOTPLUG_CPU -void arch_unregister_cpu(int cpu); -#endif -#endif /* ! __ASSEMBLY__ */ - #endif /* _ASM_CPU_H */ diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h index f349c94510e8..91867a6a9f8e 100644 --- a/arch/x86/include/asm/cpu.h +++ b/arch/x86/include/asm/cpu.h @@ -24,7 +24,6 @@ static inline void prefill_possible_map(void) {} #endif /* CONFIG_SMP */ #ifdef CONFIG_HOTPLUG_CPU -extern void arch_unregister_cpu(int); extern void soft_restart_cpu(void); #endif diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c index 677f963e02ce..c709747c4a18 100644 --- a/drivers/base/cpu.c +++ b/drivers/base/cpu.c @@ -531,7 +531,14 @@ int __weak arch_register_cpu(int cpu) { return register_cpu(&per_cpu(cpu_devices, cpu), cpu); } -#endif + +#ifdef CONFIG_HOTPLUG_CPU +void __weak arch_unregister_cpu(int num) +{ + unregister_cpu(&per_cpu(cpu_devices, num)); +} +#endif /* CONFIG_HOTPLUG_CPU */ +#endif /* CONFIG_GENERIC_CPU_DEVICES */ static void __init cpu_dev_register_generic(void) {
Add arch_unregister_cpu() to allow the ACPI machinery to call unregister_cpu(). This is enough for arm64, riscv and loongarch, but needs to be overridden by x86 and ia64 who need to do more work. CC: Jean-Philippe Brucker <jean-philippe@linaro.org> Signed-off-by: James Morse <james.morse@arm.com> --- Changes since v1: * Added CONFIG_HOTPLUG_CPU ifdeffery around unregister_cpu --- arch/ia64/include/asm/cpu.h | 4 ---- arch/loongarch/include/asm/cpu.h | 6 ------ arch/x86/include/asm/cpu.h | 1 - drivers/base/cpu.c | 9 ++++++++- 4 files changed, 8 insertions(+), 12 deletions(-)