arm64: Implement parallel CPU onlining with PSCI v0.2+
Subsystems (6)
Will Deacon
maintainercover arm64: Implement parallel CPU onlining with PSCI v0.2+
Hi folks,
This series implements CONFIG_HOTPLUG_PARALLEL for arm64 by utilising the argument to CPU_ON introduced in PSCI v0.2. This supersedes the previous series from Jinjie [1], and I'm grateful to him for his support getting this alternative version in shape for posting.
I previously spoke at KVM forum about this work last year:
https://www.youtube.com/watch?v=Q6kOshnnQuE
There are three major benefits realised by these changes:
1. We ditch our home-brew secondary CPU synchronisation in favour of constructs in the core code.
2. They offer a performance advantage on some platforms, where getting a CPU into the kernel can take time. I'm hoping Jinjie can provide numbers in this case.
3. When used inside a confidential guest, they provide protection against a malicious VMM that puts secondary CPUs into the guest kernel after the onlining operation has timed out.
The first four patches are small changes to the generic code to make it a little more "Arm-shaped" but the rest of the series is largely confined to the arm64 architecture and PSCI driver code.
The series is based on -rc2, as it otherwise conflicts with Fuad's GMID fix that was recently merged upstream.
All feedback welcome,
Will
[1] https://lore.kernel.org/all/[email protected]/
--->8
Will Deacon (19):
cpu/hotplug: Clean up cmpxchg() logic in cpuhp_can_boot_ap() cpu/hotplug: Avoid trying to bring up CPUs that are already online cpu/hotplug: Avoid busy-polling on archs where cpu_relax() is a no-op cpu/hotplug: Propagate bring-up status to
arch_cpuhp_cleanup_kick_cpu()
arm64: smp: Tidy up smp_prepare_cpus()
arm64: smp: Tidy up cpuinfo init and cpufeature updates
arm64: smp: Defer update of secondary CPU capabilities
arm64: smp: Don't bother printing the I-cache policy for each CPU
arm64: smp: Defer RCU registration during secondary CPU bringup
arm64: smp: Use generic HOTPLUG_CORE_SYNC_FULL machinery for CPU
onlining
arm64: smp: Use generic HOTPLUG_SPLIT_STARTUP machinery for CPU
onlining
arm64: cpu_ops: Make 'cpu_operations' pointer global instead of
per-cpu
arm64: cpu_ops: Introduce get_secondary_cpu_ops() firmware/psci: Cache PSCI v0.2+ version number to avoid redundant SMCs firmware/psci: Extend ->cpu_on() callback to take an additional
argument
arm64: cpu_ops: Expose optional argument to target cpu in ->cpu_boot()
arm64: smp: Pass secondary CPU boot parameters via firmware if
possible
arm64: smp: Use generic HOTPLUG_PARALLEL machinery for CPU onlining
arm64: smp: Harden parallel CPU bringup against broken PSCI firmware
arch/arm/kernel/psci_smp.c | 4 +- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/cpu.h | 7 +- arch/arm64/include/asm/cpu_ops.h | 7 +- arch/arm64/include/asm/smp.h | 40 +-- arch/arm64/include/asm/topology.h | 2 + arch/arm64/kernel/acpi_parking_protocol.c | 3 +- arch/arm64/kernel/asm-offsets.c | 1 + arch/arm64/kernel/cpu_ops.c | 34 ++- arch/arm64/kernel/cpufeature.c | 322 ++++++++++++---------- arch/arm64/kernel/cpuinfo.c | 27 -- arch/arm64/kernel/head.S | 64 ++++- arch/arm64/kernel/psci.c | 13 +- arch/arm64/kernel/smp.c | 179 ++++++------ arch/arm64/kernel/smp_spin_table.c | 2 +- arch/arm64/mm/mmu.c | 2 +- arch/x86/kernel/smpboot.c | 6 +- drivers/firmware/psci/psci.c | 27 +- include/linux/cpuhotplug.h | 4 +- include/linux/psci.h | 3 +- include/linux/rcutree.h | 2 +- kernel/cpu.c | 28 +- 22 files changed, 429 insertions(+), 350 deletions(-)
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Will Deacon
maintainerPATCH 1/19 cpu/hotplug: Clean up cmpxchg() logic in cpuhp_can_boot_ap()
cpuhp_can_boot_ap() uses atomic_try_cmpxchg() to transition the sync state of the incoming CPU to SYNC_STATE_KICKED. However, this is unnecessary if the state is SYNC_STATE_DEAD, since there will not be any concurrent state modifications, and also if the state is already set to SYNC_STATE_KICKED.
Restrict the use of cmpxchg() to the case where the existing state is SYNC_STATE_ALIVE.
--- kernel/cpu.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
kernel/cpu.c +4 -4
@@ -408,22 +408,22 @@ static bool cpuhp_can_boot_ap(unsigned int cpu)switch (sync) {case SYNC_STATE_DEAD :/* CPU is properly dead */ +atomic_set (st,SYNC_STATE_KICKED );break ;case SYNC_STATE_KICKED :/* CPU did not come up in previous attempt */ break ;case SYNC_STATE_ALIVE :/* CPU is stuck cpuhp_ap_sync_alive(). */ +if (!atomic_try_cmpxchg_relaxed (st, &sync,SYNC_STATE_KICKED )) + goto again;break ;default :/* CPU failed to report online or dead and is in limbo state. */ return false; } -/* Prepare for booting */ -if (!atomic_try_cmpxchg (st, &sync,SYNC_STATE_KICKED )) - goto again; - +/* Continue with booting */ return true; }
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 01/19] cpu/hotplug: Clean up cmpxchg() logic in cpuhp_can_boot_ap()
Quoted 32 lines · Will Deacon
cpuhp_can_boot_ap()usesatomic_try_cmpxchg()to transition the sync state of the incoming CPU toSYNC_STATE_KICKED. However, this is unnecessary if the state isSYNC_STATE_DEAD, since there will not be any concurrent state modifications, and also if the state is already set toSYNC_STATE_KICKED.Restrict the use of
cmpxchg()to the case where the existing state isSYNC_STATE_ALIVE.Signed-off-by Will Deacon <[email protected]>--- kernel/cpu.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)kernel/cpu.c +3
-0@@ -408,22 +408,22 @@ static bool cpuhp_can_boot_ap(unsigned int cpu)switch (sync) {case SYNC_STATE_DEAD :/* CPU is properly dead */ +atomic_set (st,SYNC_STATE_KICKED );break ;case SYNC_STATE_KICKED :/* CPU did not come up in previous attempt */ break ;case SYNC_STATE_ALIVE :/* CPU is stuck cpuhp_ap_sync_alive(). */ +if (!atomic_try_cmpxchg_relaxed (st, &sync,SYNC_STATE_KICKED )) + goto again;break ;
kernel/cpu.c static bool cpuhp_can_boot_ap(unsigned int cpu) · line 426
Quoted 10 lines
kernel/cpu.c +1
-4default :/* CPU failed to report online or dead and is in limbo state. */ return false; } -/* Prepare for booting */ -if (!atomic_try_cmpxchg (st, &sync,SYNC_STATE_KICKED )) - goto again; - +/* Continue with booting */
Quoted 3 lines
kernel/cpu.c +0
-0return true; }
Will Deacon
maintainerPATCH 2/19 cpu/hotplug: Avoid trying to bring up CPUs that are already online
In reply to Will DeaconThere's little point trying to bring up a CPU that is already online. Although _cpu_up() handles this case by doing nothing (because the target state has already been reached), it's wasted effort when we can easily elide the call to cpu_up() in the first place.
Check that the target CPU isn't already online before invoking cpu_up() from cpuhp_bringup_mask().
--- kernel/cpu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/cpu.c +2 -1
@@ -1769,7 +1769,8 @@ static void __init cpuhp_bringup_mask(const struct cpumask *mask, unsigned int nfor_each_cpu (cpu, mask) {struct cpuhp_cpu_state *st =per_cpu_ptr (&cpuhp_state, cpu); -if (cpu_up (cpu, target) &&can_rollback_cpu (st)) { +if (!cpu_online (cpu) &&cpu_up (cpu, target) && +can_rollback_cpu (st)) {/* * If this failed then cpu_up() might have only * rolled back to CPUHP_BP_KICK_AP for the final
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 02/19] cpu/hotplug: Avoid trying to bring up CPUs that are already online
Quoted 18 lines · Will Deacon
There's little point trying to bring up a CPU that is already online. Although
_cpu_up()handles this case by doing nothing (because the target state has already been reached), it's wasted effort when we can easily elide the call tocpu_up()in the first place.Check that the target CPU isn't already online before invoking
cpu_up()fromcpuhp_bringup_mask().Signed-off-by Will Deacon <[email protected]>--- kernel/cpu.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/cpu.c +0
-0
kernel/cpu.c static void __init cpuhp_bringup_mask(const struct cpumask *mask, unsigned int n · line 1773
Quoted 6 lines
kernel/cpu.c +2
-1@@ -1769,7 +1769,8 @@ static void __init cpuhp_bringup_mask(const struct cpumask *mask, unsigned int nfor_each_cpu (cpu, mask) {struct cpuhp_cpu_state *st =per_cpu_ptr (&cpuhp_state, cpu); -if (cpu_up (cpu, target) &&can_rollback_cpu (st)) { +if (!cpu_online (cpu) &&cpu_up (cpu, target) && +can_rollback_cpu (st)) {
Make sense to me.
Quoted 3 lines
kernel/cpu.c +0
-0/* * If this failed then cpu_up() might have only * rolled back to CPUHP_BP_KICK_AP for the final
Will Deacon
maintainerPATCH 3/19 cpu/hotplug: Avoid busy-polling on archs where cpu_relax() is a no-op
In reply to Will DeaconOn some architectures (such as arm64), cpu_relax() is effectively a NOP and so isn't particularly efficient when used in a tight polling loop such as the CPU state synchronisation in cpuhp_wait_for_sync_state().
Once an incoming CPU has reached the SYNC_STATE_ALIVE state, we know that it is executing within the kernel and so we can use the more efficient polling mechanism provided by the atomic_cond_read* API.
Extend the generic implementation of arch_cpuhp_sync_state_poll() to take the state details as additional parameters and polling using atomic_cond_read_relaxed() instead of cpu_relax() once we have reached the alive state. No change on x86.
--- arch/x86/kernel/smpboot.c | 2 +- include/linux/cpuhotplug.h | 2 +- kernel/cpu.c | 13 +++++++++---- 3 files changed, 11 insertions(+), 6 deletions(-)
arch/x86/kernel/smpboot.c +1 -1
@@ -1138,7 +1138,7 @@ void arch_cpuhp_cleanup_dead_cpu(unsigned int cpu)pr_info ("CPU %u is now offline\n" , cpu); } -void arch_cpuhp_sync_state_poll (void ) +void arch_cpuhp_sync_state_poll (atomic_t *st,int old) {if (smp_ops.poll_sync_state) smp_ops.poll_sync_state ();
include/linux/cpuhotplug.h +1 -1
@@ -509,7 +509,7 @@ static inline void cpuhp_online_idle(enum cpuhp_state state) { }struct task_struct ;void cpuhp_ap_sync_alive (void ); -void arch_cpuhp_sync_state_poll (void ); +void arch_cpuhp_sync_state_poll (atomic_t *st,int old);void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu);int arch_cpuhp_kick_ap_alive (unsigned int cpu,struct task_struct *tidle);bool arch_cpuhp_init_parallel_bringup (void );
kernel/cpu.c +9 -4
@@ -303,7 +303,13 @@ static inline void cpuhp_ap_update_sync_state(enum cpuhp_sync_state state) (void )atomic_xchg (st, state); } -void __weak arch_cpuhp_sync_state_poll (void ) {cpu_relax (); } +void __weak arch_cpuhp_sync_state_poll (atomic_t *st,int old) +{ +if (old <SYNC_STATE_ALIVE ) +cpu_relax (); +else +atomic_cond_read_relaxed (st,VAL != old); +}static bool cpuhp_wait_for_sync_state (unsigned int cpu,enum cpuhp_sync_state state,enum cpuhp_sync_state next_state)
@@ -328,7 +334,7 @@ static bool cpuhp_wait_for_sync_state(unsigned int cpu, enum cpuhp_sync_state streturn false; }else if (now - start <NSEC_PER_MSEC ) {/* Poll for one millisecond */ -arch_cpuhp_sync_state_poll (); +arch_cpuhp_sync_state_poll (st, sync); }else {usleep_range (USEC_PER_MSEC ,2 *USEC_PER_MSEC ); }
@@ -395,8 +401,7 @@ void cpuhp_ap_sync_alive(void)cpuhp_ap_update_sync_state (SYNC_STATE_ALIVE );/* Wait for the control CPU to release it. */ -while (atomic_read (st) !=SYNC_STATE_SHOULD_ONLINE ) -cpu_relax (); +atomic_cond_read_acquire (st,VAL ==SYNC_STATE_SHOULD_ONLINE ); }static bool cpuhp_can_boot_ap (unsigned int cpu)
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 03/19] cpu/hotplug: Avoid busy-polling on archs where cpu_relax() is a no-op
Quoted 2 lines · Will Deacon
On some architectures (such as arm64),
cpu_relax()is effectively a NOP and so isn't particularly efficient when used in a tight polling loop
Yes, a "yield" instruction in aarch64 is essentially a nop, with no power optimization effect.
However, x86 "PAUSE" provides power optimization benefits.
Quoted 5 lines
such as the CPU state synchronisation in
cpuhp_wait_for_sync_state().Once an incoming CPU has reached the
SYNC_STATE_ALIVEstate, we know that it is executing within the kernel and so we can use the more efficient polling mechanism provided by the atomic_cond_read* API.
Right! Once the secondary CPU state transitions to SYNC_STATE_ALIVE, it is already executing kernel C code.
Quoted 5 lines
Extend the generic implementation of
arch_cpuhp_sync_state_poll()to take the state details as additional parameters and polling usingatomic_cond_read_relaxed()instead ofcpu_relax()once we have reached the alive state. No change on x86.
Right! x86 still use cpu_relax() for atomic_cond_read_relaxed().
And we can aslo mention that this also allow RISC-V to leverage the Zawrs extension for low-power stalling instead of busy-wasting cycles with cpu_relax().
Quoted 40 lines
Signed-off-by Will Deacon <[email protected]>--- arch/x86/kernel/smpboot.c | 2 +- include/linux/cpuhotplug.h | 2 +- kernel/cpu.c | 13 +++++++++---- 3 files changed, 11 insertions(+), 6 deletions(-)arch/x86/kernel/smpboot.c +1
-1@@ -1138,7 +1138,7 @@ void arch_cpuhp_cleanup_dead_cpu(unsigned int cpu)pr_info ("CPU %u is now offline\n" , cpu); } -void arch_cpuhp_sync_state_poll (void ) +void arch_cpuhp_sync_state_poll (atomic_t *st,int old) {if (smp_ops.poll_sync_state) smp_ops.poll_sync_state ();include/linux/cpuhotplug.h +1
-1@@ -509,7 +509,7 @@ static inline void cpuhp_online_idle(enum cpuhp_state state) { }struct task_struct ;void cpuhp_ap_sync_alive (void ); -void arch_cpuhp_sync_state_poll (void ); +void arch_cpuhp_sync_state_poll (atomic_t *st,int old);void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu);int arch_cpuhp_kick_ap_alive (unsigned int cpu,struct task_struct *tidle);bool arch_cpuhp_init_parallel_bringup (void );kernel/cpu.c +0
-0@@ -303,7 +303,13 @@ static inline void cpuhp_ap_update_sync_state(enum cpuhp_sync_state state) (void )atomic_xchg (st, state);
kernel/cpu.c static inline void cpuhp_ap_update_sync_state(enum cpuhp_sync_state state) · line 312
Quoted 10 lines
kernel/cpu.c +7
-1} -void __weak arch_cpuhp_sync_state_poll (void ) {cpu_relax (); } +void __weak arch_cpuhp_sync_state_poll (atomic_t *st,int old) +{ +if (old <SYNC_STATE_ALIVE ) +cpu_relax (); +else +atomic_cond_read_relaxed (st,VAL != old); +}
With atomic_cond_read_relaxed(), we can use WFE to improve the power for arm64.
Quoted 13 lines
kernel/cpu.c +1
-1static bool cpuhp_wait_for_sync_state (unsigned int cpu,enum cpuhp_sync_state state,enum cpuhp_sync_state next_state)@@ -328,7 +334,7 @@ static bool cpuhp_wait_for_sync_state(unsigned int cpu, enum cpuhp_sync_state streturn false; }else if (now - start <NSEC_PER_MSEC ) {/* Poll for one millisecond */ -arch_cpuhp_sync_state_poll (); +arch_cpuhp_sync_state_poll (st, sync); }else {usleep_range (USEC_PER_MSEC ,2 *USEC_PER_MSEC ); }
kernel/cpu.c void cpuhp_ap_sync_alive(void) · line 404
Quoted 6 lines
kernel/cpu.c +1
-2@@ -395,8 +401,7 @@ void cpuhp_ap_sync_alive(void)cpuhp_ap_update_sync_state (SYNC_STATE_ALIVE );/* Wait for the control CPU to release it. */ -while (atomic_read (st) !=SYNC_STATE_SHOULD_ONLINE ) -cpu_relax (); +atomic_cond_read_acquire (st,VAL ==SYNC_STATE_SHOULD_ONLINE );
LGTM
Quoted 3 lines
kernel/cpu.c +0
-0}static bool cpuhp_can_boot_ap (unsigned int cpu)
Re: [PATCH 03/19] cpu/hotplug: Avoid busy-polling on archs where cpu_relax() is a no-op
In reply to Will DeaconQuoted 52 lines · Will Deacon
On some architectures (such as arm64),
cpu_relax()is effectively a NOP and so isn't particularly efficient when used in a tight polling loop such as the CPU state synchronisation incpuhp_wait_for_sync_state().Once an incoming CPU has reached the
SYNC_STATE_ALIVEstate, we know that it is executing within the kernel and so we can use the more efficient polling mechanism provided by the atomic_cond_read* API.Extend the generic implementation of
arch_cpuhp_sync_state_poll()to take the state details as additional parameters and polling usingatomic_cond_read_relaxed()instead ofcpu_relax()once we have reached the alive state. No change on x86.Signed-off-by Will Deacon <[email protected]>--- arch/x86/kernel/smpboot.c | 2 +- include/linux/cpuhotplug.h | 2 +- kernel/cpu.c | 13 +++++++++---- 3 files changed, 11 insertions(+), 6 deletions(-)arch/x86/kernel/smpboot.c +1
-1@@ -1138,7 +1138,7 @@ void arch_cpuhp_cleanup_dead_cpu(unsigned int cpu)pr_info ("CPU %u is now offline\n" , cpu); } -void arch_cpuhp_sync_state_poll (void ) +void arch_cpuhp_sync_state_poll (atomic_t *st,int old) {if (smp_ops.poll_sync_state) smp_ops.poll_sync_state ();include/linux/cpuhotplug.h +1
-1@@ -509,7 +509,7 @@ static inline void cpuhp_online_idle(enum cpuhp_state state) { }struct task_struct ;void cpuhp_ap_sync_alive (void ); -void arch_cpuhp_sync_state_poll (void ); +void arch_cpuhp_sync_state_poll (atomic_t *st,int old);void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu);int arch_cpuhp_kick_ap_alive (unsigned int cpu,struct task_struct *tidle);bool arch_cpuhp_init_parallel_bringup (void );kernel/cpu.c +0
-0@@ -303,7 +303,13 @@ static inline void cpuhp_ap_update_sync_state(enum cpuhp_sync_state state) (void )atomic_xchg (st, state);
kernel/cpu.c static inline void cpuhp_ap_update_sync_state(enum cpuhp_sync_state state) · line 312
Quoted 10 lines
kernel/cpu.c +7
-1} -void __weak arch_cpuhp_sync_state_poll (void ) {cpu_relax (); } +void __weak arch_cpuhp_sync_state_poll (atomic_t *st,int old) +{ +if (old <SYNC_STATE_ALIVE ) +cpu_relax (); +else +atomic_cond_read_relaxed (st,VAL != old); +}
As sashiko pointed out, it seems that the else branch unreachable for the call of cpuhp_wait_for_sync_state(cpu, SYNC_STATE_ALIVE, SYNC_STATE_SHOULD_ONLINE), the old state can only be "SYNC_STATE_KICKED" or "SYNC_STATE_SHOULD_DIE", which go the if branch.
Link:
https://sashiko.dev/#/patchset/20260907164024.17164-1-will%40kernel.org
Quoted 22 lines
kernel/cpu.c +2
-3static bool cpuhp_wait_for_sync_state (unsigned int cpu,enum cpuhp_sync_state state,enum cpuhp_sync_state next_state)@@ -328,7 +334,7 @@ static bool cpuhp_wait_for_sync_state(unsigned int cpu, enum cpuhp_sync_state streturn false; }else if (now - start <NSEC_PER_MSEC ) {/* Poll for one millisecond */ -arch_cpuhp_sync_state_poll (); +arch_cpuhp_sync_state_poll (st, sync); }else {usleep_range (USEC_PER_MSEC ,2 *USEC_PER_MSEC ); }@@ -395,8 +401,7 @@ void cpuhp_ap_sync_alive(void)cpuhp_ap_update_sync_state (SYNC_STATE_ALIVE );/* Wait for the control CPU to release it. */ -while (atomic_read (st) !=SYNC_STATE_SHOULD_ONLINE ) -cpu_relax (); +atomic_cond_read_acquire (st,VAL ==SYNC_STATE_SHOULD_ONLINE ); }static bool cpuhp_can_boot_ap (unsigned int cpu)
Will Deacon
maintainerRe: [PATCH 03/19] cpu/hotplug: Avoid busy-polling on archs where cpu_relax() is a no-op
Quoted 9 lines · Jinjie Ruan
Quoted 8 lines · Will Deacon
kernel/cpu.c +0
-0@@ -303,7 +303,13 @@ static inline void cpuhp_ap_update_sync_state(enum cpuhp_sync_state state) (void )atomic_xchg (st, state); }
Quoted 13 lines
kernel/cpu.c static inline void cpuhp_ap_update_sync_state(enum cpuhp_sync_state state) · line 312
Quoted 8 lines
kernel/cpu.c +7
-1-void __weak arch_cpuhp_sync_state_poll (void ) {cpu_relax (); } +void __weak arch_cpuhp_sync_state_poll (atomic_t *st,int old) +{ +if (old <SYNC_STATE_ALIVE ) +cpu_relax (); +else +atomic_cond_read_relaxed (st,VAL != old); +}As sashiko pointed out, it seems that the else branch unreachable for the call of
cpuhp_wait_for_sync_state(cpu, SYNC_STATE_ALIVE, SYNC_STATE_SHOULD_ONLINE), the old state can only be "SYNC_STATE_KICKED" or "SYNC_STATE_SHOULD_DIE", which go the if branch.
Ah yes, it's a harmless leftover from my initial (failed) attempt to use cpuhp_wait_for_sync_state() in cpuhp_ap_sync_alive(). I can drop this part and reword the commit message.
Will
Re: [PATCH 03/19] cpu/hotplug: Avoid busy-polling on archs where cpu_relax() is a no-op
Quoted 24 lines · Will Deacon
Quoted 9 lines · Jinjie Ruan
Quoted 8 lines · Will Deacon
kernel/cpu.c +0
-0@@ -303,7 +303,13 @@ static inline void cpuhp_ap_update_sync_state(enum cpuhp_sync_state state) (void )atomic_xchg (st, state); }Quoted 13 lines
kernel/cpu.c static inline void cpuhp_ap_update_sync_state(enum cpuhp_sync_state state) · line 312
Quoted 8 lines
kernel/cpu.c +7
-1-void __weak arch_cpuhp_sync_state_poll (void ) {cpu_relax (); } +void __weak arch_cpuhp_sync_state_poll (atomic_t *st,int old) +{ +if (old <SYNC_STATE_ALIVE ) +cpu_relax (); +else +atomic_cond_read_relaxed (st,VAL != old); +}As sashiko pointed out, it seems that the else branch unreachable for the call of
cpuhp_wait_for_sync_state(cpu, SYNC_STATE_ALIVE, SYNC_STATE_SHOULD_ONLINE), the old state can only be "SYNC_STATE_KICKED" or "SYNC_STATE_SHOULD_DIE", which go the if branch.
Quoted 3 lines
Ah yes, it's a harmless leftover from my initial (failed) attempt to use
cpuhp_wait_for_sync_state()incpuhp_ap_sync_alive(). I can drop this part and reword the commit message.
Thanks, looking forward to your update — that will be nice.
Quoted 2 lines
Will
Will Deacon
maintainerPATCH 4/19 cpu/hotplug: Propagate bring-up status to arch_cpuhp_cleanup_kick_cpu()
In reply to Will DeaconIn preparation for enabling the generic CPU hotplug machinery on arm64, which has architecture-specific handling of early bringup failures, extend arch_cpuhp_cleanup_kick_cpu() to take an additional argument indicating whether or not the target AP reached the alive state.
--- arch/x86/kernel/smpboot.c | 4 ++-- include/linux/cpuhotplug.h | 2 +- kernel/cpu.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-)
arch/x86/kernel/smpboot.c +2 -2
@@ -1074,7 +1074,7 @@ static int do_boot_cpu(u32 apicid, unsigned int cpu, struct task_struct *idle)/* If the wakeup mechanism failed, cleanup the warm reset vector */ if (ret) -arch_cpuhp_cleanup_kick_cpu (cpu); +arch_cpuhp_cleanup_kick_cpu (cpu, false);return ret; }
@@ -1122,7 +1122,7 @@ int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *tidle)return smp_ops.kick_ap_alive (cpu, tidle); } -void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu) +void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu,bool is_alive) {/* Cleanup possible dangling ends... */ if (smp_ops.kick_ap_alive == native_kick_ap && x86_platform.legacy.warm_reset)
include/linux/cpuhotplug.h +1 -1
@@ -510,7 +510,7 @@ struct task_struct;void cpuhp_ap_sync_alive (void );void arch_cpuhp_sync_state_poll (atomic_t *st,int old); -void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu); +void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu,bool is_alive);int arch_cpuhp_kick_ap_alive (unsigned int cpu,struct task_struct *tidle);bool arch_cpuhp_init_parallel_bringup (void );
kernel/cpu.c +2 -2
@@ -432,7 +432,7 @@ static bool cpuhp_can_boot_ap(unsigned int cpu)return true; } -void __weak arch_cpuhp_cleanup_kick_cpu (unsigned int cpu) { } +void __weak arch_cpuhp_cleanup_kick_cpu (unsigned int cpu,bool is_alive) { }/* * Early CPU bringup synchronization point. Cannot use cpuhp_state::done_up
@@ -451,7 +451,7 @@ static int cpuhp_bp_sync_alive(unsigned int cpu) }/* Let the architecture cleanup the kick alive mechanics. */ -arch_cpuhp_cleanup_kick_cpu (cpu); +arch_cpuhp_cleanup_kick_cpu (cpu, !ret);return ret; }#else /* CONFIG_HOTPLUG_CORE_SYNC_FULL */
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 04/19] cpu/hotplug: Propagate bring-up status to arch_cpuhp_cleanup_kick_cpu()
Quoted 61 lines · Will Deacon
In preparation for enabling the generic CPU hotplug machinery on arm64, which has architecture-specific handling of early bringup failures, extend
arch_cpuhp_cleanup_kick_cpu()to take an additional argument indicating whether or not the target AP reached the alive state.Signed-off-by Will Deacon <[email protected]>--- arch/x86/kernel/smpboot.c | 4 ++-- include/linux/cpuhotplug.h | 2 +- kernel/cpu.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-)arch/x86/kernel/smpboot.c +2
-2@@ -1074,7 +1074,7 @@ static int do_boot_cpu(u32 apicid, unsigned int cpu, struct task_struct *idle)/* If the wakeup mechanism failed, cleanup the warm reset vector */ if (ret) -arch_cpuhp_cleanup_kick_cpu (cpu); +arch_cpuhp_cleanup_kick_cpu (cpu, false);return ret; }@@ -1122,7 +1122,7 @@ int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *tidle)return smp_ops.kick_ap_alive (cpu, tidle); } -void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu) +void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu,bool is_alive) {/* Cleanup possible dangling ends... */ if (smp_ops.kick_ap_alive == native_kick_ap && x86_platform.legacy.warm_reset)include/linux/cpuhotplug.h +1
-1@@ -510,7 +510,7 @@ struct task_struct;void cpuhp_ap_sync_alive (void );void arch_cpuhp_sync_state_poll (atomic_t *st,int old); -void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu); +void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu,bool is_alive);int arch_cpuhp_kick_ap_alive (unsigned int cpu,struct task_struct *tidle);bool arch_cpuhp_init_parallel_bringup (void );kernel/cpu.c +1
-1@@ -432,7 +432,7 @@ static bool cpuhp_can_boot_ap(unsigned int cpu)return true; } -void __weak arch_cpuhp_cleanup_kick_cpu (unsigned int cpu) { } +void __weak arch_cpuhp_cleanup_kick_cpu (unsigned int cpu,bool is_alive) { }/* * Early CPU bringup synchronization point. Cannot use cpuhp_state::done_up
kernel/cpu.c static int cpuhp_bp_sync_alive(unsigned int cpu) · line 454
Quoted 5 lines
kernel/cpu.c +1
-1@@ -451,7 +451,7 @@ static int cpuhp_bp_sync_alive(unsigned int cpu) }/* Let the architecture cleanup the kick alive mechanics. */ -arch_cpuhp_cleanup_kick_cpu (cpu); +arch_cpuhp_cleanup_kick_cpu (cpu, !ret);
LGTM
Quoted 3 lines
kernel/cpu.c +0
-0return ret; }#else /* CONFIG_HOTPLUG_CORE_SYNC_FULL */
Will Deacon
maintainerPATCH 5/19 arm64: smp: Tidy up smp_prepare_cpus()
In reply to Will Deaconsmp_prepare_cpus() is always run on the boot CPU (i.e. CPU 0) but goes to great lengths to support running on a CPU where smp_processor_id() is non-zero.
Clean up the code a little by hardcoding zero for the boot CPU ID.
--- arch/arm64/kernel/smp.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
arch/arm64/kernel/smp.c +5 -8
@@ -779,16 +779,14 @@ void __init smp_init_cpus(void)void __init smp_prepare_cpus (unsigned int max_cpus) {const struct cpu_operations *ops; -int err;unsigned int cpu; -unsigned int this_cpu; +int err;init_cpu_topology (); - this_cpu =smp_processor_id (); -store_cpu_topology (this_cpu); -numa_store_cpu_info (this_cpu); -numa_add_cpu (this_cpu); +store_cpu_topology (0 ); +numa_store_cpu_info (0 ); +numa_add_cpu (0 );/* * If UP is mandated by "nosmp" (which implies "maxcpus=0"), don't set
@@ -803,8 +801,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)* secondaries from the bootloader. */ for_each_possible_cpu (cpu) { - -if (cpu ==smp_processor_id ()) +if (cpu ==0 )continue ; ops =get_cpu_ops (cpu);
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 05/19] arm64: smp: Tidy up smp_prepare_cpus()
Quoted 2 lines · Will Deacon
smp_prepare_cpus()is always run on the boot CPU (i.e. CPU 0) but goes to great lengths to support running on a CPU wheresmp_processor_id()
Quoted 44 lines
is non-zero.
Clean up the code a little by hardcoding zero for the boot CPU ID.
Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/smp.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)arch/arm64/kernel/smp.c +5
-8@@ -779,16 +779,14 @@ void __init smp_init_cpus(void)void __init smp_prepare_cpus (unsigned int max_cpus) {const struct cpu_operations *ops; -int err;unsigned int cpu; -unsigned int this_cpu; +int err;init_cpu_topology (); - this_cpu =smp_processor_id (); -store_cpu_topology (this_cpu); -numa_store_cpu_info (this_cpu); -numa_add_cpu (this_cpu); +store_cpu_topology (0 ); +numa_store_cpu_info (0 ); +numa_add_cpu (0 );/* * If UP is mandated by "nosmp" (which implies "maxcpus=0"), don't set @@ -803,8 +801,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)* secondaries from the bootloader. */ for_each_possible_cpu (cpu) { - -if (cpu ==smp_processor_id ()) +if (cpu ==0 )continue ; ops =get_cpu_ops (cpu);
Will Deacon
maintainerPATCH 6/19 arm64: smp: Tidy up cpuinfo init and cpufeature updates
In reply to Will DeaconPopulating the 'cpuinfo_arm64' structure during CPU bringup and subsequently checking/updating cpufeature structures is slightly convoluted and differs unnecessarily between the boot CPU and secondary CPUs.
Rework the code so that cpuinfo_store_cpu() is used to populate the 'cpuinfo_arm64' structure for each CPU, with secondary CPUs then calling update_cpu_features() to update the global view of the available features. This allows us to internalise the 'boot_cpu_data' in cpufeature.c and paves the way for parallelising the ID register probing during bring-up of secondary CPUs.
--- arch/arm64/include/asm/cpu.h | 7 +++---- arch/arm64/kernel/cpufeature.c | 21 +++++++++++++++++---- arch/arm64/kernel/cpuinfo.c | 11 ----------- arch/arm64/kernel/smp.c | 3 ++- 4 files changed, 22 insertions(+), 20 deletions(-)
arch/arm64/include/asm/cpu.h +3 -4
@@ -73,11 +73,10 @@ struct cpuinfo_arm64 { DECLARE_PER_CPU(struct cpuinfo_arm64, cpu_data);void cpuinfo_store_cpu (void ); -void __init cpuinfo_store_boot_cpu (void ); -void __init init_cpu_features (struct cpuinfo_arm64 *info); -void update_cpu_features (int cpu,struct cpuinfo_arm64 *info, -struct cpuinfo_arm64 *boot); +void __init init_cpu_features (void ); +void update_cpu_features (int cpu); +bool gmid_el1_accessible (const struct cpuinfo_arm64 *info);#endif /* __ASM_CPU_H */
arch/arm64/kernel/cpufeature.c +17 -4
@@ -117,6 +117,7 @@ EXPORT_SYMBOL(system_cpucaps);static struct arm64_cpu_capabilities const __ro_after_init *cpucap_ptrs[ARM64_NCAPS ];DECLARE_BITMAP (boot_cpucaps,ARM64_NCAPS ); +static struct cpuinfo_arm64 boot_cpu_data;/* * arm64_use_ng_mappings must be placed in the .data section, otherwise it
@@ -1205,11 +1206,19 @@ bool gmid_el1_accessible(const struct cpuinfo_arm64 *info)return mte >=ID_AA64PFR1_EL1_MTE_MTE2 ; } -void __init init_cpu_features (struct cpuinfo_arm64 *info) +void __init init_cpu_features (void ) { +struct cpuinfo_arm64 *info = &per_cpu (cpu_data,0 ); +/* Before we start using the tables, make sure it is sorted */ sort_ftr_regs (); +/* +* We keep a copy of the boot CPU registers so that physical hotplug +* of CPU 0 can still be properly checked. +*/ + boot_cpu_data = *info; +init_cpu_ftr_reg (SYS_CTR_EL0 , info->reg_ctr);init_cpu_ftr_reg (SYS_DCZID_EL0 , info->reg_dczid);init_cpu_ftr_reg (SYS_CNTFRQ_EL0 , info->reg_cntfrq);
@@ -1404,12 +1413,14 @@ static int update_32bit_cpu_features(int cpu, struct cpuinfo_32bit *info,* non-boot CPU. Also performs SANITY checks to make sure that there * aren't any insane variations from that of the boot CPU. */ -void update_cpu_features (int cpu, -struct cpuinfo_arm64 *info, -struct cpuinfo_arm64 *boot) +void update_cpu_features (int cpu) { +struct cpuinfo_arm64 *boot, *info;int taint =0 ; + boot = &boot_cpu_data; + info =per_cpu_ptr (&cpu_data, cpu); +/* * The kernel can handle differing I-cache policies, but otherwise * caches should look identical. Userspace JITs will make use of
@@ -3978,6 +3989,8 @@ static void __init setup_boot_cpu_capabilities(void)void __init setup_boot_cpu_features (void ) { +init_cpu_features (); +/* * Initialize the indirect array of CPU capabilities pointers before we * handle the boot CPU.
arch/arm64/kernel/cpuinfo.c +0 -11
@@ -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 inline const char *icache_policy_str (int l1ip) {
@@ -531,14 +530,4 @@ void cpuinfo_store_cpu(void) {struct cpuinfo_arm64 *info =this_cpu_ptr (&cpu_data);__cpuinfo_store_cpu (info); -update_cpu_features (smp_processor_id (), info, &boot_cpu_data); -} - -void __init cpuinfo_store_boot_cpu (void ) -{ -struct cpuinfo_arm64 *info = &per_cpu (cpu_data,0 ); -__cpuinfo_store_cpu (info); - - boot_cpu_data = *info; -init_cpu_features (&boot_cpu_data); }
arch/arm64/kernel/smp.c +2 -1
@@ -235,6 +235,7 @@ asmlinkage notrace void secondary_start_kernel(void)* Log the CPU info before it is marked online and might get read. */ cpuinfo_store_cpu (); +update_cpu_features (cpu);store_cpu_topology (cpu);/*
@@ -455,7 +456,7 @@ void __init smp_prepare_boot_cpu(void)*/ set_my_cpu_offset (per_cpu_offset (smp_processor_id ())); -cpuinfo_store_boot_cpu (); +cpuinfo_store_cpu ();setup_boot_cpu_features ();/* Conditionally switch to GIC PMR for interrupt masking */
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 06/19] arm64: smp: Tidy up cpuinfo init and cpufeature updates
Quoted 25 lines · Will Deacon
Populating the '
cpuinfo_arm64' structure during CPU bringup and subsequently checking/updating cpufeature structures is slightly convoluted and differs unnecessarily between the boot CPU and secondary CPUs.Rework the code so that
cpuinfo_store_cpu()is used to populate the 'cpuinfo_arm64' structure for each CPU, with secondary CPUs then callingupdate_cpu_features()to update the global view of the available features. This allows us to internalise the 'boot_cpu_data' incpufeature.cand paves the way for parallelising the ID register probing during bring-up of secondary CPUs.Signed-off-by Will Deacon <[email protected]>--- arch/arm64/include/asm/cpu.h | 7 +++---- arch/arm64/kernel/cpufeature.c | 21 +++++++++++++++++---- arch/arm64/kernel/cpuinfo.c | 11 ----------- arch/arm64/kernel/smp.c | 3 ++- 4 files changed, 22 insertions(+), 20 deletions(-)arch/arm64/include/asm/cpu.h +0
-0
arch/arm64/include/asm/cpu.h struct cpuinfo_arm64 { · line 77
Quoted 9 lines
arch/arm64/include/asm/cpu.h +1
-4@@ -73,11 +73,10 @@ struct cpuinfo_arm64 { DECLARE_PER_CPU(struct cpuinfo_arm64, cpu_data);void cpuinfo_store_cpu (void ); -void __init cpuinfo_store_boot_cpu (void ); -void __init init_cpu_features (struct cpuinfo_arm64 *info); -void update_cpu_features (int cpu,struct cpuinfo_arm64 *info, -struct cpuinfo_arm64 *boot); +void __init init_cpu_features (void );
init_cpu_features() now only used in cpufeature.c, we can remove this declaration and make it static in cpufeature.c.
Quoted 18 lines
arch/arm64/include/asm/cpu.h +2
-0+void update_cpu_features (int cpu); +bool gmid_el1_accessible (const struct cpuinfo_arm64 *info);#endif /* __ASM_CPU_H */ arch/arm64/kernel/cpufeature.c +1
-0@@ -117,6 +117,7 @@ EXPORT_SYMBOL(system_cpucaps);static struct arm64_cpu_capabilities const __ro_after_init *cpucap_ptrs[ARM64_NCAPS ];DECLARE_BITMAP (boot_cpucaps,ARM64_NCAPS ); +static struct cpuinfo_arm64 boot_cpu_data;/* * arm64_use_ng_mappings must be placed in the .data section, otherwise it
arch/arm64/kernel/cpufeature.c bool gmid_el1_accessible(const struct cpuinfo_arm64 *info) · line 1210
Quoted 6 lines
arch/arm64/kernel/cpufeature.c +1
-1@@ -1205,11 +1206,19 @@ bool gmid_el1_accessible(const struct cpuinfo_arm64 *info)return mte >=ID_AA64PFR1_EL1_MTE_MTE2 ; } -void __init init_cpu_features (struct cpuinfo_arm64 *info) +void __init init_cpu_features (void ) {
Maybe rename to init_boot_cpu_features() ?
Otherwise LGTM
Quoted 89 lines
arch/arm64/kernel/cpufeature.c +15
-3+struct cpuinfo_arm64 *info = &per_cpu (cpu_data,0 ); +/* Before we start using the tables, make sure it is sorted */ sort_ftr_regs (); +/* +* We keep a copy of the boot CPU registers so that physical hotplug +* of CPU 0 can still be properly checked. +*/ + boot_cpu_data = *info; +init_cpu_ftr_reg (SYS_CTR_EL0 , info->reg_ctr);init_cpu_ftr_reg (SYS_DCZID_EL0 , info->reg_dczid);init_cpu_ftr_reg (SYS_CNTFRQ_EL0 , info->reg_cntfrq);@@ -1404,12 +1413,14 @@ static int update_32bit_cpu_features(int cpu, struct cpuinfo_32bit *info,* non-boot CPU. Also performs SANITY checks to make sure that there * aren't any insane variations from that of the boot CPU. */ -void update_cpu_features (int cpu, -struct cpuinfo_arm64 *info, -struct cpuinfo_arm64 *boot) +void update_cpu_features (int cpu) { +struct cpuinfo_arm64 *boot, *info;int taint =0 ; + boot = &boot_cpu_data; + info =per_cpu_ptr (&cpu_data, cpu); +/* * The kernel can handle differing I-cache policies, but otherwise * caches should look identical. Userspace JITs will make use of @@ -3978,6 +3989,8 @@ static void __init setup_boot_cpu_capabilities(void)void __init setup_boot_cpu_features (void ) { +init_cpu_features (); +/* * Initialize the indirect array of CPU capabilities pointers before we * handle the boot CPU. arch/arm64/kernel/cpuinfo.c +0
-11@@ -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 inline const char *icache_policy_str (int l1ip) {@@ -531,14 +530,4 @@ void cpuinfo_store_cpu(void) {struct cpuinfo_arm64 *info =this_cpu_ptr (&cpu_data);__cpuinfo_store_cpu (info); -update_cpu_features (smp_processor_id (), info, &boot_cpu_data); -} - -void __init cpuinfo_store_boot_cpu (void ) -{ -struct cpuinfo_arm64 *info = &per_cpu (cpu_data,0 ); -__cpuinfo_store_cpu (info); - - boot_cpu_data = *info; -init_cpu_features (&boot_cpu_data); }arch/arm64/kernel/smp.c +2
-1@@ -235,6 +235,7 @@ asmlinkage notrace void secondary_start_kernel(void)* Log the CPU info before it is marked online and might get read. */ cpuinfo_store_cpu (); +update_cpu_features (cpu);store_cpu_topology (cpu);/* @@ -455,7 +456,7 @@ void __init smp_prepare_boot_cpu(void)*/ set_my_cpu_offset (per_cpu_offset (smp_processor_id ())); -cpuinfo_store_boot_cpu (); +cpuinfo_store_cpu ();setup_boot_cpu_features ();/* Conditionally switch to GIC PMR for interrupt masking */
Will Deacon
maintainerPATCH 7/19 arm64: smp: Defer update of secondary CPU capabilities
In reply to Will Deaconcheck_local_cpu_capabilities() runs relatively early during the boot of each secondary CPU and, despite its name, calls update_cpu_capabilities() to manipulate the global 'system_cpucaps' based on the features detected by the incoming CPU.
In preparation for parallel bringup of secondary CPUs, move the call to update_cpu_capabilities() into update_cpu_features(), allowing check_local_cpu_capabilities() to run concurrently in future, as it now only performs local verification of the incoming CPU.
--- arch/arm64/kernel/cpufeature.c | 311 +++++++++++++++++---------------- 1 file changed, 157 insertions(+), 154 deletions(-)
arch/arm64/kernel/cpufeature.c +157 -154
@@ -1408,156 +1408,6 @@ static int update_32bit_cpu_features(int cpu, struct cpuinfo_32bit *info,return taint; } -/* -* Update system wide CPU feature registers with the values from a -* non-boot CPU. Also performs SANITY checks to make sure that there -* aren't any insane variations from that of the boot CPU. -*/ -void update_cpu_features (int cpu) -{ -struct cpuinfo_arm64 *boot, *info; -int taint =0 ; - - boot = &boot_cpu_data; - info =per_cpu_ptr (&cpu_data, cpu); - -/* -* The kernel can handle differing I-cache policies, but otherwise -* caches should look identical. Userspace JITs will make use of -* *minLine. -*/ - taint |=check_update_ftr_reg (SYS_CTR_EL0 , cpu, - info->reg_ctr, boot->reg_ctr); - -/* -* Userspace may perform DC ZVA instructions. Mismatched block sizes -* could result in too much or too little memory being zeroed if a -* process is preempted and migrated between CPUs. -*/ - taint |=check_update_ftr_reg (SYS_DCZID_EL0 , cpu, - info->reg_dczid, boot->reg_dczid); - -/* If different, timekeeping will be broken (especially with KVM) */ - taint |=check_update_ftr_reg (SYS_CNTFRQ_EL0 , cpu, - info->reg_cntfrq, boot->reg_cntfrq); - -/* -* The kernel uses self-hosted debug features and expects CPUs to -* support identical debug features. We presently need CTX_CMPs, WRPs, -* and BRPs to be identical. -* ID_AA64DFR1 is currently RES0. -*/ - taint |=check_update_ftr_reg (SYS_ID_AA64DFR0_EL1 , cpu, - info->reg_id_aa64dfr0, boot->reg_id_aa64dfr0); - taint |=check_update_ftr_reg (SYS_ID_AA64DFR1_EL1 , cpu, - info->reg_id_aa64dfr1, boot->reg_id_aa64dfr1); -/* -* Even in big.LITTLE, processors should be identical instruction-set -* wise. -*/ - taint |=check_update_ftr_reg (SYS_ID_AA64ISAR0_EL1 , cpu, - info->reg_id_aa64isar0, boot->reg_id_aa64isar0); - taint |=check_update_ftr_reg (SYS_ID_AA64ISAR1_EL1 , cpu, - info->reg_id_aa64isar1, boot->reg_id_aa64isar1); - taint |=check_update_ftr_reg (SYS_ID_AA64ISAR2_EL1 , cpu, - info->reg_id_aa64isar2, boot->reg_id_aa64isar2); - taint |=check_update_ftr_reg (SYS_ID_AA64ISAR3_EL1 , cpu, - info->reg_id_aa64isar3, boot->reg_id_aa64isar3); - -/* -* Differing PARange support is fine as long as all peripherals and -* memory are mapped within the minimum PARange of all CPUs. -* Linux should not care about secure memory. -*/ - taint |=check_update_ftr_reg (SYS_ID_AA64MMFR0_EL1 , cpu, - info->reg_id_aa64mmfr0, boot->reg_id_aa64mmfr0); - taint |=check_update_ftr_reg (SYS_ID_AA64MMFR1_EL1 , cpu, - info->reg_id_aa64mmfr1, boot->reg_id_aa64mmfr1); - taint |=check_update_ftr_reg (SYS_ID_AA64MMFR2_EL1 , cpu, - info->reg_id_aa64mmfr2, boot->reg_id_aa64mmfr2); - taint |=check_update_ftr_reg (SYS_ID_AA64MMFR3_EL1 , cpu, - info->reg_id_aa64mmfr3, boot->reg_id_aa64mmfr3); - taint |=check_update_ftr_reg (SYS_ID_AA64MMFR4_EL1 , cpu, - info->reg_id_aa64mmfr4, boot->reg_id_aa64mmfr4); - - taint |=check_update_ftr_reg (SYS_ID_AA64PFR0_EL1 , cpu, - info->reg_id_aa64pfr0, boot->reg_id_aa64pfr0); - taint |=check_update_ftr_reg (SYS_ID_AA64PFR1_EL1 , cpu, - info->reg_id_aa64pfr1, boot->reg_id_aa64pfr1); - taint |=check_update_ftr_reg (SYS_ID_AA64PFR2_EL1 , cpu, - info->reg_id_aa64pfr2, boot->reg_id_aa64pfr2); - - taint |=check_update_ftr_reg (SYS_ID_AA64ZFR0_EL1 , cpu, - info->reg_id_aa64zfr0, boot->reg_id_aa64zfr0); - - taint |=check_update_ftr_reg (SYS_ID_AA64SMFR0_EL1 , cpu, - info->reg_id_aa64smfr0, boot->reg_id_aa64smfr0); - - taint |=check_update_ftr_reg (SYS_ID_AA64FPFR0_EL1 , cpu, - info->reg_id_aa64fpfr0, boot->reg_id_aa64fpfr0); - -/* Probe vector lengths */ -if (IS_ENABLED (CONFIG_ARM64_SVE ) && -id_aa64pfr0_sve (read_sanitised_ftr_reg (SYS_ID_AA64PFR0_EL1 ))) { -if (!system_capabilities_finalized ()) { -unsigned long cpacr =cpacr_save_enable_kernel_sve (); - -vec_update_vq_map (ARM64_VEC_SVE ); - -cpacr_restore (cpacr); - } - } - -if (IS_ENABLED (CONFIG_ARM64_SME ) && -id_aa64pfr1_sme (read_sanitised_ftr_reg (SYS_ID_AA64PFR1_EL1 ))) { -unsigned long cpacr =cpacr_save_enable_kernel_sme (); - -/* Probe vector lengths */ -if (!system_capabilities_finalized ()) -vec_update_vq_map (ARM64_VEC_SME ); - -cpacr_restore (cpacr); - } - -if (detect_ftr_has_mpam ()) { - info->reg_mpamidr =read_cpuid (MPAMIDR_EL1 ); - taint |=check_update_ftr_reg (SYS_MPAMIDR_EL1 , cpu, - info->reg_mpamidr, boot->reg_mpamidr); - } - -/* -* The kernel uses the LDGM/STGM instructions and the number of tags -* they read/write depends on the GMID_EL1.BS field. Check that the -* value is the same on all CPUs. -*/ -if (gmid_el1_accessible (info)) - taint |=check_update_ftr_reg (SYS_GMID_EL1 , cpu, - info->reg_gmid, boot->reg_gmid); - -/* -* If we don't have AArch32 at all then skip the checks entirely -* as the register values may be UNKNOWN and we're not going to be -* using them for anything. -* -* This relies on a sanitised view of the AArch64 ID registers -* (e.g. SYS_ID_AA64PFR0_EL1), so we call it last. -*/ -if (id_aa64pfr0_32bit_el0 (info->reg_id_aa64pfr0)) { -lazy_init_32bit_cpu_features (info, boot); - taint |=update_32bit_cpu_features (cpu, &info->aarch32, - &boot->aarch32); - } - -/* -* Mismatched CPU features are a recipe for disaster. Don't even -* pretend to support them. -*/ -if (taint) { -pr_warn_once ("Unsupported CPU feature variation detected.\n" ); -add_taint (TAINT_CPU_OUT_OF_SPEC ,LOCKDEP_STILL_OK ); - } -} -u64 read_sanitised_ftr_reg (u32 id) {struct arm64_ftr_reg *regp =get_arm64_ftr_reg (id);
@@ -3902,16 +3752,169 @@ void check_local_cpu_capabilities(void)*/ check_early_cpu_features (); +/* +* Verify that this CPU has all the system advertised +* capabilities. +*/ +if (system_capabilities_finalized ()) +verify_local_cpu_capabilities (); +} + +/* +* Update system wide CPU feature registers with the values from a +* non-boot CPU. Also performs SANITY checks to make sure that there +* aren't any insane variations from that of the boot CPU. +*/ +void update_cpu_features (int cpu) +{ +struct cpuinfo_arm64 *boot, *info; +int taint =0 ; +/* * If we haven't finalised the system capabilities, this CPU gets * a chance to update the errata work arounds and local features. -* Otherwise, this CPU should verify that it has all the system -* advertised capabilities. */ if (!system_capabilities_finalized ())update_cpu_capabilities (SCOPE_LOCAL_CPU ); -else -verify_local_cpu_capabilities (); + + boot = &boot_cpu_data; + info =per_cpu_ptr (&cpu_data, cpu); + +/* +* The kernel can handle differing I-cache policies, but otherwise +* caches should look identical. Userspace JITs will make use of +* *minLine. +*/ + taint |=check_update_ftr_reg (SYS_CTR_EL0 , cpu, + info->reg_ctr, boot->reg_ctr); + +/* +* Userspace may perform DC ZVA instructions. Mismatched block sizes +* could result in too much or too little memory being zeroed if a +* process is preempted and migrated between CPUs. +*/ + taint |=check_update_ftr_reg (SYS_DCZID_EL0 , cpu, + info->reg_dczid, boot->reg_dczid); + +/* If different, timekeeping will be broken (especially with KVM) */ + taint |=check_update_ftr_reg (SYS_CNTFRQ_EL0 , cpu, + info->reg_cntfrq, boot->reg_cntfrq); + +/* +* The kernel uses self-hosted debug features and expects CPUs to +* support identical debug features. We presently need CTX_CMPs, WRPs, +* and BRPs to be identical. +* ID_AA64DFR1 is currently RES0. +*/ + taint |=check_update_ftr_reg (SYS_ID_AA64DFR0_EL1 , cpu, + info->reg_id_aa64dfr0, boot->reg_id_aa64dfr0); + taint |=check_update_ftr_reg (SYS_ID_AA64DFR1_EL1 , cpu, + info->reg_id_aa64dfr1, boot->reg_id_aa64dfr1); +/* +* Even in big.LITTLE, processors should be identical instruction-set +* wise. +*/ + taint |=check_update_ftr_reg (SYS_ID_AA64ISAR0_EL1 , cpu, + info->reg_id_aa64isar0, boot->reg_id_aa64isar0); + taint |=check_update_ftr_reg (SYS_ID_AA64ISAR1_EL1 , cpu, + info->reg_id_aa64isar1, boot->reg_id_aa64isar1); + taint |=check_update_ftr_reg (SYS_ID_AA64ISAR2_EL1 , cpu, + info->reg_id_aa64isar2, boot->reg_id_aa64isar2); + taint |=check_update_ftr_reg (SYS_ID_AA64ISAR3_EL1 , cpu, + info->reg_id_aa64isar3, boot->reg_id_aa64isar3); + +/* +* Differing PARange support is fine as long as all peripherals and +* memory are mapped within the minimum PARange of all CPUs. +* Linux should not care about secure memory. +*/ + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR0_EL1 , cpu, + info->reg_id_aa64mmfr0, boot->reg_id_aa64mmfr0); + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR1_EL1 , cpu, + info->reg_id_aa64mmfr1, boot->reg_id_aa64mmfr1); + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR2_EL1 , cpu, + info->reg_id_aa64mmfr2, boot->reg_id_aa64mmfr2); + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR3_EL1 , cpu, + info->reg_id_aa64mmfr3, boot->reg_id_aa64mmfr3); + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR4_EL1 , cpu, + info->reg_id_aa64mmfr4, boot->reg_id_aa64mmfr4); + + taint |=check_update_ftr_reg (SYS_ID_AA64PFR0_EL1 , cpu, + info->reg_id_aa64pfr0, boot->reg_id_aa64pfr0); + taint |=check_update_ftr_reg (SYS_ID_AA64PFR1_EL1 , cpu, + info->reg_id_aa64pfr1, boot->reg_id_aa64pfr1); + taint |=check_update_ftr_reg (SYS_ID_AA64PFR2_EL1 , cpu, + info->reg_id_aa64pfr2, boot->reg_id_aa64pfr2); + + taint |=check_update_ftr_reg (SYS_ID_AA64ZFR0_EL1 , cpu, + info->reg_id_aa64zfr0, boot->reg_id_aa64zfr0); + + taint |=check_update_ftr_reg (SYS_ID_AA64SMFR0_EL1 , cpu, + info->reg_id_aa64smfr0, boot->reg_id_aa64smfr0); + + taint |=check_update_ftr_reg (SYS_ID_AA64FPFR0_EL1 , cpu, + info->reg_id_aa64fpfr0, boot->reg_id_aa64fpfr0); + +/* Probe vector lengths */ +if (IS_ENABLED (CONFIG_ARM64_SVE ) && +id_aa64pfr0_sve (read_sanitised_ftr_reg (SYS_ID_AA64PFR0_EL1 ))) { +if (!system_capabilities_finalized ()) { +unsigned long cpacr =cpacr_save_enable_kernel_sve (); + +vec_update_vq_map (ARM64_VEC_SVE ); + +cpacr_restore (cpacr); + } + } + +if (IS_ENABLED (CONFIG_ARM64_SME ) && +id_aa64pfr1_sme (read_sanitised_ftr_reg (SYS_ID_AA64PFR1_EL1 ))) { +unsigned long cpacr =cpacr_save_enable_kernel_sme (); + +/* Probe vector lengths */ +if (!system_capabilities_finalized ()) +vec_update_vq_map (ARM64_VEC_SME ); + +cpacr_restore (cpacr); + } + +if (detect_ftr_has_mpam ()) { + info->reg_mpamidr =read_cpuid (MPAMIDR_EL1 ); + taint |=check_update_ftr_reg (SYS_MPAMIDR_EL1 , cpu, + info->reg_mpamidr, boot->reg_mpamidr); + } + +/* +* The kernel uses the LDGM/STGM instructions and the number of tags +* they read/write depends on the GMID_EL1.BS field. Check that the +* value is the same on all CPUs. +*/ +if (gmid_el1_accessible (info)) + taint |=check_update_ftr_reg (SYS_GMID_EL1 , cpu, + info->reg_gmid, boot->reg_gmid); + +/* +* If we don't have AArch32 at all then skip the checks entirely +* as the register values may be UNKNOWN and we're not going to be +* using them for anything. +* +* This relies on a sanitised view of the AArch64 ID registers +* (e.g. SYS_ID_AA64PFR0_EL1), so we call it last. +*/ +if (id_aa64pfr0_32bit_el0 (info->reg_id_aa64pfr0)) { +lazy_init_32bit_cpu_features (info, boot); + taint |=update_32bit_cpu_features (cpu, &info->aarch32, + &boot->aarch32); + } + +/* +* Mismatched CPU features are a recipe for disaster. Don't even +* pretend to support them. +*/ +if (taint) { +pr_warn_once ("Unsupported CPU feature variation detected.\n" ); +add_taint (TAINT_CPU_OUT_OF_SPEC ,LOCKDEP_STILL_OK ); + } }bool this_cpu_has_cap (unsigned int n)
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 07/19] arm64: smp: Defer update of secondary CPU capabilities
Quoted 177 lines · Will Deacon
check_local_cpu_capabilities()runs relatively early during the boot of each secondary CPU and, despite its name, callsupdate_cpu_capabilities()to manipulate the global 'system_cpucaps' based on the features detected by the incoming CPU.In preparation for parallel bringup of secondary CPUs, move the call to
update_cpu_capabilities()intoupdate_cpu_features(), allowingcheck_local_cpu_capabilities()to run concurrently in future, as it now only performs local verification of the incoming CPU.Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/cpufeature.c | 311 +++++++++++++++++---------------- 1 file changed, 157 insertions(+), 154 deletions(-)arch/arm64/kernel/cpufeature.c +0
-150@@ -1408,156 +1408,6 @@ static int update_32bit_cpu_features(int cpu, struct cpuinfo_32bit *info,return taint; } -/* -* Update system wide CPU feature registers with the values from a -* non-boot CPU. Also performs SANITY checks to make sure that there -* aren't any insane variations from that of the boot CPU. -*/ -void update_cpu_features (int cpu) -{ -struct cpuinfo_arm64 *boot, *info; -int taint =0 ; - - boot = &boot_cpu_data; - info =per_cpu_ptr (&cpu_data, cpu); - -/* -* The kernel can handle differing I-cache policies, but otherwise -* caches should look identical. Userspace JITs will make use of -* *minLine. -*/ - taint |=check_update_ftr_reg (SYS_CTR_EL0 , cpu, - info->reg_ctr, boot->reg_ctr); - -/* -* Userspace may perform DC ZVA instructions. Mismatched block sizes -* could result in too much or too little memory being zeroed if a -* process is preempted and migrated between CPUs. -*/ - taint |=check_update_ftr_reg (SYS_DCZID_EL0 , cpu, - info->reg_dczid, boot->reg_dczid); - -/* If different, timekeeping will be broken (especially with KVM) */ - taint |=check_update_ftr_reg (SYS_CNTFRQ_EL0 , cpu, - info->reg_cntfrq, boot->reg_cntfrq); - -/* -* The kernel uses self-hosted debug features and expects CPUs to -* support identical debug features. We presently need CTX_CMPs, WRPs, -* and BRPs to be identical. -* ID_AA64DFR1 is currently RES0. -*/ - taint |=check_update_ftr_reg (SYS_ID_AA64DFR0_EL1 , cpu, - info->reg_id_aa64dfr0, boot->reg_id_aa64dfr0); - taint |=check_update_ftr_reg (SYS_ID_AA64DFR1_EL1 , cpu, - info->reg_id_aa64dfr1, boot->reg_id_aa64dfr1); -/* -* Even in big.LITTLE, processors should be identical instruction-set -* wise. -*/ - taint |=check_update_ftr_reg (SYS_ID_AA64ISAR0_EL1 , cpu, - info->reg_id_aa64isar0, boot->reg_id_aa64isar0); - taint |=check_update_ftr_reg (SYS_ID_AA64ISAR1_EL1 , cpu, - info->reg_id_aa64isar1, boot->reg_id_aa64isar1); - taint |=check_update_ftr_reg (SYS_ID_AA64ISAR2_EL1 , cpu, - info->reg_id_aa64isar2, boot->reg_id_aa64isar2); - taint |=check_update_ftr_reg (SYS_ID_AA64ISAR3_EL1 , cpu, - info->reg_id_aa64isar3, boot->reg_id_aa64isar3); - -/* -* Differing PARange support is fine as long as all peripherals and -* memory are mapped within the minimum PARange of all CPUs. -* Linux should not care about secure memory. -*/ - taint |=check_update_ftr_reg (SYS_ID_AA64MMFR0_EL1 , cpu, - info->reg_id_aa64mmfr0, boot->reg_id_aa64mmfr0); - taint |=check_update_ftr_reg (SYS_ID_AA64MMFR1_EL1 , cpu, - info->reg_id_aa64mmfr1, boot->reg_id_aa64mmfr1); - taint |=check_update_ftr_reg (SYS_ID_AA64MMFR2_EL1 , cpu, - info->reg_id_aa64mmfr2, boot->reg_id_aa64mmfr2); - taint |=check_update_ftr_reg (SYS_ID_AA64MMFR3_EL1 , cpu, - info->reg_id_aa64mmfr3, boot->reg_id_aa64mmfr3); - taint |=check_update_ftr_reg (SYS_ID_AA64MMFR4_EL1 , cpu, - info->reg_id_aa64mmfr4, boot->reg_id_aa64mmfr4); - - taint |=check_update_ftr_reg (SYS_ID_AA64PFR0_EL1 , cpu, - info->reg_id_aa64pfr0, boot->reg_id_aa64pfr0); - taint |=check_update_ftr_reg (SYS_ID_AA64PFR1_EL1 , cpu, - info->reg_id_aa64pfr1, boot->reg_id_aa64pfr1); - taint |=check_update_ftr_reg (SYS_ID_AA64PFR2_EL1 , cpu, - info->reg_id_aa64pfr2, boot->reg_id_aa64pfr2); - - taint |=check_update_ftr_reg (SYS_ID_AA64ZFR0_EL1 , cpu, - info->reg_id_aa64zfr0, boot->reg_id_aa64zfr0); - - taint |=check_update_ftr_reg (SYS_ID_AA64SMFR0_EL1 , cpu, - info->reg_id_aa64smfr0, boot->reg_id_aa64smfr0); - - taint |=check_update_ftr_reg (SYS_ID_AA64FPFR0_EL1 , cpu, - info->reg_id_aa64fpfr0, boot->reg_id_aa64fpfr0); - -/* Probe vector lengths */ -if (IS_ENABLED (CONFIG_ARM64_SVE ) && -id_aa64pfr0_sve (read_sanitised_ftr_reg (SYS_ID_AA64PFR0_EL1 ))) { -if (!system_capabilities_finalized ()) { -unsigned long cpacr =cpacr_save_enable_kernel_sve (); - -vec_update_vq_map (ARM64_VEC_SVE ); - -cpacr_restore (cpacr); - } - } - -if (IS_ENABLED (CONFIG_ARM64_SME ) && -id_aa64pfr1_sme (read_sanitised_ftr_reg (SYS_ID_AA64PFR1_EL1 ))) { -unsigned long cpacr =cpacr_save_enable_kernel_sme (); - -/* Probe vector lengths */ -if (!system_capabilities_finalized ()) -vec_update_vq_map (ARM64_VEC_SME ); - -cpacr_restore (cpacr); - } - -if (detect_ftr_has_mpam ()) { - info->reg_mpamidr =read_cpuid (MPAMIDR_EL1 ); - taint |=check_update_ftr_reg (SYS_MPAMIDR_EL1 , cpu, - info->reg_mpamidr, boot->reg_mpamidr); - } - -/* -* The kernel uses the LDGM/STGM instructions and the number of tags -* they read/write depends on the GMID_EL1.BS field. Check that the -* value is the same on all CPUs. -*/ -if (gmid_el1_accessible (info)) - taint |=check_update_ftr_reg (SYS_GMID_EL1 , cpu, - info->reg_gmid, boot->reg_gmid); - -/* -* If we don't have AArch32 at all then skip the checks entirely -* as the register values may be UNKNOWN and we're not going to be -* using them for anything. -* -* This relies on a sanitised view of the AArch64 ID registers -* (e.g. SYS_ID_AA64PFR0_EL1), so we call it last. -*/ -if (id_aa64pfr0_32bit_el0 (info->reg_id_aa64pfr0)) { -lazy_init_32bit_cpu_features (info, boot); - taint |=update_32bit_cpu_features (cpu, &info->aarch32, - &boot->aarch32); - } - -/* -* Mismatched CPU features are a recipe for disaster. Don't even -* pretend to support them. -*/ -if (taint) { -pr_warn_once ("Unsupported CPU feature variation detected.\n" ); -add_taint (TAINT_CPU_OUT_OF_SPEC ,LOCKDEP_STILL_OK ); - } -} -u64 read_sanitised_ftr_reg (u32 id) {struct arm64_ftr_reg *regp =get_arm64_ftr_reg (id);
arch/arm64/kernel/cpufeature.c void check_local_cpu_capabilities(void) · line 3761
Quoted 10 lines
arch/arm64/kernel/cpufeature.c +7
-0@@ -3902,16 +3752,169 @@ void check_local_cpu_capabilities(void)*/ check_early_cpu_features (); +/* +* Verify that this CPU has all the system advertised +* capabilities. +*/ +if (system_capabilities_finalized ()) +verify_local_cpu_capabilities (); +}
As I commented below, this order avoids concurrency issues, as after cpuhp_ap_sync_alive(), the secondary CPUs are woken up serially by the boot CPU.
It also eliminates the problem of boot CPUs being stuck in deadlock wait for the secondary CPUs, because update_cpu_capabilities() does not call cpu_die_early() or cpu_panic_kernel().
secondary_start_kernel()
-> check_local_cpu_capabilities()
-> cpuhp_ap_sync_alive()
-> update_cpu_features()
-> update_cpu_capabilities()
So LGTM
Quoted 163 lines
arch/arm64/kernel/cpufeature.c +150
-4+ +/* +* Update system wide CPU feature registers with the values from a +* non-boot CPU. Also performs SANITY checks to make sure that there +* aren't any insane variations from that of the boot CPU. +*/ +void update_cpu_features (int cpu) +{ +struct cpuinfo_arm64 *boot, *info; +int taint =0 ; +/* * If we haven't finalised the system capabilities, this CPU gets * a chance to update the errata work arounds and local features. -* Otherwise, this CPU should verify that it has all the system -* advertised capabilities. */ if (!system_capabilities_finalized ())update_cpu_capabilities (SCOPE_LOCAL_CPU ); -else -verify_local_cpu_capabilities (); + + boot = &boot_cpu_data; + info =per_cpu_ptr (&cpu_data, cpu); + +/* +* The kernel can handle differing I-cache policies, but otherwise +* caches should look identical. Userspace JITs will make use of +* *minLine. +*/ + taint |=check_update_ftr_reg (SYS_CTR_EL0 , cpu, + info->reg_ctr, boot->reg_ctr); + +/* +* Userspace may perform DC ZVA instructions. Mismatched block sizes +* could result in too much or too little memory being zeroed if a +* process is preempted and migrated between CPUs. +*/ + taint |=check_update_ftr_reg (SYS_DCZID_EL0 , cpu, + info->reg_dczid, boot->reg_dczid); + +/* If different, timekeeping will be broken (especially with KVM) */ + taint |=check_update_ftr_reg (SYS_CNTFRQ_EL0 , cpu, + info->reg_cntfrq, boot->reg_cntfrq); + +/* +* The kernel uses self-hosted debug features and expects CPUs to +* support identical debug features. We presently need CTX_CMPs, WRPs, +* and BRPs to be identical. +* ID_AA64DFR1 is currently RES0. +*/ + taint |=check_update_ftr_reg (SYS_ID_AA64DFR0_EL1 , cpu, + info->reg_id_aa64dfr0, boot->reg_id_aa64dfr0); + taint |=check_update_ftr_reg (SYS_ID_AA64DFR1_EL1 , cpu, + info->reg_id_aa64dfr1, boot->reg_id_aa64dfr1); +/* +* Even in big.LITTLE, processors should be identical instruction-set +* wise. +*/ + taint |=check_update_ftr_reg (SYS_ID_AA64ISAR0_EL1 , cpu, + info->reg_id_aa64isar0, boot->reg_id_aa64isar0); + taint |=check_update_ftr_reg (SYS_ID_AA64ISAR1_EL1 , cpu, + info->reg_id_aa64isar1, boot->reg_id_aa64isar1); + taint |=check_update_ftr_reg (SYS_ID_AA64ISAR2_EL1 , cpu, + info->reg_id_aa64isar2, boot->reg_id_aa64isar2); + taint |=check_update_ftr_reg (SYS_ID_AA64ISAR3_EL1 , cpu, + info->reg_id_aa64isar3, boot->reg_id_aa64isar3); + +/* +* Differing PARange support is fine as long as all peripherals and +* memory are mapped within the minimum PARange of all CPUs. +* Linux should not care about secure memory. +*/ + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR0_EL1 , cpu, + info->reg_id_aa64mmfr0, boot->reg_id_aa64mmfr0); + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR1_EL1 , cpu, + info->reg_id_aa64mmfr1, boot->reg_id_aa64mmfr1); + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR2_EL1 , cpu, + info->reg_id_aa64mmfr2, boot->reg_id_aa64mmfr2); + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR3_EL1 , cpu, + info->reg_id_aa64mmfr3, boot->reg_id_aa64mmfr3); + taint |=check_update_ftr_reg (SYS_ID_AA64MMFR4_EL1 , cpu, + info->reg_id_aa64mmfr4, boot->reg_id_aa64mmfr4); + + taint |=check_update_ftr_reg (SYS_ID_AA64PFR0_EL1 , cpu, + info->reg_id_aa64pfr0, boot->reg_id_aa64pfr0); + taint |=check_update_ftr_reg (SYS_ID_AA64PFR1_EL1 , cpu, + info->reg_id_aa64pfr1, boot->reg_id_aa64pfr1); + taint |=check_update_ftr_reg (SYS_ID_AA64PFR2_EL1 , cpu, + info->reg_id_aa64pfr2, boot->reg_id_aa64pfr2); + + taint |=check_update_ftr_reg (SYS_ID_AA64ZFR0_EL1 , cpu, + info->reg_id_aa64zfr0, boot->reg_id_aa64zfr0); + + taint |=check_update_ftr_reg (SYS_ID_AA64SMFR0_EL1 , cpu, + info->reg_id_aa64smfr0, boot->reg_id_aa64smfr0); + + taint |=check_update_ftr_reg (SYS_ID_AA64FPFR0_EL1 , cpu, + info->reg_id_aa64fpfr0, boot->reg_id_aa64fpfr0); + +/* Probe vector lengths */ +if (IS_ENABLED (CONFIG_ARM64_SVE ) && +id_aa64pfr0_sve (read_sanitised_ftr_reg (SYS_ID_AA64PFR0_EL1 ))) { +if (!system_capabilities_finalized ()) { +unsigned long cpacr =cpacr_save_enable_kernel_sve (); + +vec_update_vq_map (ARM64_VEC_SVE ); + +cpacr_restore (cpacr); + } + } + +if (IS_ENABLED (CONFIG_ARM64_SME ) && +id_aa64pfr1_sme (read_sanitised_ftr_reg (SYS_ID_AA64PFR1_EL1 ))) { +unsigned long cpacr =cpacr_save_enable_kernel_sme (); + +/* Probe vector lengths */ +if (!system_capabilities_finalized ()) +vec_update_vq_map (ARM64_VEC_SME ); + +cpacr_restore (cpacr); + } + +if (detect_ftr_has_mpam ()) { + info->reg_mpamidr =read_cpuid (MPAMIDR_EL1 ); + taint |=check_update_ftr_reg (SYS_MPAMIDR_EL1 , cpu, + info->reg_mpamidr, boot->reg_mpamidr); + } + +/* +* The kernel uses the LDGM/STGM instructions and the number of tags +* they read/write depends on the GMID_EL1.BS field. Check that the +* value is the same on all CPUs. +*/ +if (gmid_el1_accessible (info)) + taint |=check_update_ftr_reg (SYS_GMID_EL1 , cpu, + info->reg_gmid, boot->reg_gmid); + +/* +* If we don't have AArch32 at all then skip the checks entirely +* as the register values may be UNKNOWN and we're not going to be +* using them for anything. +* +* This relies on a sanitised view of the AArch64 ID registers +* (e.g. SYS_ID_AA64PFR0_EL1), so we call it last. +*/ +if (id_aa64pfr0_32bit_el0 (info->reg_id_aa64pfr0)) { +lazy_init_32bit_cpu_features (info, boot); + taint |=update_32bit_cpu_features (cpu, &info->aarch32, + &boot->aarch32); + } + +/* +* Mismatched CPU features are a recipe for disaster. Don't even +* pretend to support them. +*/ +if (taint) { +pr_warn_once ("Unsupported CPU feature variation detected.\n" ); +add_taint (TAINT_CPU_OUT_OF_SPEC ,LOCKDEP_STILL_OK ); + } }bool this_cpu_has_cap (unsigned int n)
Will Deacon
maintainerPATCH 8/19 arm64: smp: Don't bother printing the I-cache policy for each CPU
In reply to Will DeaconThe I-cache policy isn't particularly interesting but printing it early can result in unnecessary serialisation of onlining CPUs.
Remove the pointless print.
--- arch/arm64/kernel/cpuinfo.c | 16 ---------------- 1 file changed, 16 deletions(-)
arch/arm64/kernel/cpuinfo.c +0 -16
@@ -31,19 +31,6 @@* values depending on configuration at or after reset. */ DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data); - -static inline const char *icache_policy_str (int l1ip) -{ -switch (l1ip) { -case CTR_EL0_L1Ip_VIPT: -return "VIPT" ; -case CTR_EL0_L1Ip_PIPT: -return "PIPT" ; -default : -return "RESERVED/UNKNOWN" ; - } -} -unsigned long __icache_flags;static const char *const hwcap_str[] = {
@@ -424,7 +411,6 @@ device_initcall(cpuinfo_regs_init);static void cpuinfo_detect_icache_policy (struct cpuinfo_arm64 *info) { -unsigned int cpu =smp_processor_id ();u32 l1ip =CTR_L1IP (info->reg_ctr);switch (l1ip) {
@@ -436,8 +422,6 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)set_bit (ICACHEF_ALIASING , &__icache_flags);break ; } - -pr_info ("Detected %s I-cache on CPU%d\n" ,icache_policy_str (l1ip), cpu); }static void __cpuinfo_store_cpu_32bit (struct cpuinfo_32bit *info)
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 08/19] arm64: smp: Don't bother printing the I-cache policy for each CPU
Quoted 43 lines · Will Deacon
The I-cache policy isn't particularly interesting but printing it early can result in unnecessary serialisation of onlining CPUs.
Remove the pointless print.
Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/cpuinfo.c | 16 ---------------- 1 file changed, 16 deletions(-)arch/arm64/kernel/cpuinfo.c +0
-14@@ -31,19 +31,6 @@* values depending on configuration at or after reset. */ DEFINE_PER_CPU(struct cpuinfo_arm64, cpu_data); - -static inline const char *icache_policy_str (int l1ip) -{ -switch (l1ip) { -case CTR_EL0_L1Ip_VIPT: -return "VIPT" ; -case CTR_EL0_L1Ip_PIPT: -return "PIPT" ; -default : -return "RESERVED/UNKNOWN" ; - } -} -unsigned long __icache_flags;static const char *const hwcap_str[] = {@@ -424,7 +411,6 @@ device_initcall(cpuinfo_regs_init);static void cpuinfo_detect_icache_policy (struct cpuinfo_arm64 *info) { -unsigned int cpu =smp_processor_id ();u32 l1ip =CTR_L1IP (info->reg_ctr);switch (l1ip) {
arch/arm64/kernel/cpuinfo.c static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info) · line 440
Quoted 5 lines
arch/arm64/kernel/cpuinfo.c +0
-2@@ -436,8 +422,6 @@ static void cpuinfo_detect_icache_policy(struct cpuinfo_arm64 *info)set_bit (ICACHEF_ALIASING , &__icache_flags);break ; } - -pr_info ("Detected %s I-cache on CPU%d\n" ,icache_policy_str (l1ip), cpu);
As we need to defer RCU registration during secondary CPU bringup, so we should avoid unnecessary early print during secondary CPU bringup, which will trigger a false-positive lockdep"suspicious RCU usage" splat during early lock acquisitions as commit ce3d31ad3cac ("arm64/smp: Move rcu_cpu_starting() earlier") pointed out.
LGTM
Quoted 3 lines
arch/arm64/kernel/cpuinfo.c +0
-0}static void __cpuinfo_store_cpu_32bit (struct cpuinfo_32bit *info)
Will Deacon
maintainerPATCH 9/19 arm64: smp: Defer RCU registration during secondary CPU bringup
In reply to Will DeaconCalling rcutree_report_cpu_starting() early during boot can lead to livelocks with the generic CPU hotplug mechanism if the boot CPU blocks on an RCU grace period while the CPU being onlined is spinning in cpuhp_ap_sync_alive().
In preparation for enabling the generic CPU hotplug code on arm64, split up the trace_hardirqs_off() call during secondary CPU bringup so that we update lockdep early but defer the tracing updates until after notify_cpu_starting() has registered the new CPU with RCU, allowing us to drop the explicit call to rcutree_report_cpu_starting() entirely.
--- arch/arm64/kernel/smp.c | 5 ++--- include/linux/rcutree.h | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-)
arch/arm64/kernel/smp.c +2 -3
@@ -217,8 +217,7 @@ asmlinkage notrace void secondary_start_kernel(void)if (system_uses_irq_prio_masking ())init_gic_priority_masking (); -rcutree_report_cpu_starting (cpu); -trace_hardirqs_off (); +lockdep_hardirqs_off (CALLER_ADDR0 );/* * If the system has established the capabilities, make sure
@@ -242,6 +241,7 @@ asmlinkage notrace void secondary_start_kernel(void)* Enable GIC and timers. */ notify_cpu_starting (cpu); +trace_hardirqs_off_finish ();ipi_setup (cpu);
@@ -411,7 +411,6 @@ void __noreturn cpu_die_early(void)/* Mark this CPU absent */ set_cpu_present (cpu,0 ); -rcutree_report_cpu_dead ();if (IS_ENABLED (CONFIG_HOTPLUG_CPU )) {update_cpu_boot_status (CPU_KILL_ME );
include/linux/rcutree.h +1 -1
@@ -116,7 +116,7 @@ int rcutree_offline_cpu(unsigned int cpu);void rcutree_migrate_callbacks (int cpu); -/* Called from hotplug and also arm64 early secondary boot failure */ +/* Called from hotplug */ void rcutree_report_cpu_dead (void );#endif /* __LINUX_RCUTREE_H */
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 09/19] arm64: smp: Defer RCU registration during secondary CPU bringup
Quoted 22 lines · Will Deacon
Calling
rcutree_report_cpu_starting()early during boot can lead to livelocks with the generic CPU hotplug mechanism if the boot CPU blocks on an RCU grace period while the CPU being onlined is spinning incpuhp_ap_sync_alive().In preparation for enabling the generic CPU hotplug code on arm64, split up the
trace_hardirqs_off()call during secondary CPU bringup so that we update lockdep early but defer the tracing updates until afternotify_cpu_starting()has registered the new CPU with RCU, allowing us to drop the explicit call torcutree_report_cpu_starting()entirely.Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/smp.c | 5 ++--- include/linux/rcutree.h | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-)arch/arm64/kernel/smp.c +0
-0
arch/arm64/kernel/smp.c asmlinkage notrace void secondary_start_kernel(void) · line 221
Quoted 5 lines
arch/arm64/kernel/smp.c +0
-2@@ -217,8 +217,7 @@ asmlinkage notrace void secondary_start_kernel(void)if (system_uses_irq_prio_masking ())init_gic_priority_masking (); -rcutree_report_cpu_starting (cpu); -trace_hardirqs_off ();
Hi Will,
I think we need to handle the printk problem before this patch as we discussed earlier.
Otherwise defer the rcutree_report_cpu_starting() will trigger a false-positive lockdep"suspicious RCU usage" splat during early lock acquisitions as commit ce3d31ad3cac ("arm64/smp: Move rcu_cpu_starting() earlier") pointed out.
Link:
https://lore.kernel.org/all/[email protected]/
Best regards,
Jinjie
Quoted 33 lines
arch/arm64/kernel/smp.c +2
-1+lockdep_hardirqs_off (CALLER_ADDR0 );/* * If the system has established the capabilities, make sure @@ -242,6 +241,7 @@ asmlinkage notrace void secondary_start_kernel(void)* Enable GIC and timers. */ notify_cpu_starting (cpu); +trace_hardirqs_off_finish ();ipi_setup (cpu);@@ -411,7 +411,6 @@ void __noreturn cpu_die_early(void)/* Mark this CPU absent */ set_cpu_present (cpu,0 ); -rcutree_report_cpu_dead ();if (IS_ENABLED (CONFIG_HOTPLUG_CPU )) {update_cpu_boot_status (CPU_KILL_ME );include/linux/rcutree.h +1
-1@@ -116,7 +116,7 @@ int rcutree_offline_cpu(unsigned int cpu);void rcutree_migrate_callbacks (int cpu); -/* Called from hotplug and also arm64 early secondary boot failure */ +/* Called from hotplug */ void rcutree_report_cpu_dead (void );#endif /* __LINUX_RCUTREE_H */
Will Deacon
maintainerRe: [PATCH 09/19] arm64: smp: Defer RCU registration during secondary CPU bringup
Hi Jinjie,
Quoted 32 lines · Jinjie Ruan
Quoted 22 lines · Will Deacon
Calling
rcutree_report_cpu_starting()early during boot can lead to livelocks with the generic CPU hotplug mechanism if the boot CPU blocks on an RCU grace period while the CPU being onlined is spinning incpuhp_ap_sync_alive().In preparation for enabling the generic CPU hotplug code on arm64, split up the
trace_hardirqs_off()call during secondary CPU bringup so that we update lockdep early but defer the tracing updates until afternotify_cpu_starting()has registered the new CPU with RCU, allowing us to drop the explicit call torcutree_report_cpu_starting()entirely.Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/smp.c | 5 ++--- include/linux/rcutree.h | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-)arch/arm64/kernel/smp.c +0
-0arch/arm64/kernel/smp.c asmlinkage notrace void secondary_start_kernel(void) · line 221
Quoted 5 lines
arch/arm64/kernel/smp.c +0
-2@@ -217,8 +217,7 @@ asmlinkage notrace void secondary_start_kernel(void)if (system_uses_irq_prio_masking ())init_gic_priority_masking (); -rcutree_report_cpu_starting (cpu); -trace_hardirqs_off ();I think we need to handle the printk problem before this patch as we discussed earlier.
Quoted 4 lines
Otherwise defer the
rcutree_report_cpu_starting()will trigger a false-positive lockdep"suspicious RCU usage" splat during early lock acquisitions as commitce3d31ad3cac("arm64/smp: Movercu_cpu_starting()earlier") pointed out.
Sorry, I meant to mention this in the cover letter but forgot about it. I'm not sure that ce3d31ad3cac ("arm64/smp: Move rcu_cpu_starting() earlier") is still relevant with the latest printk/console/lockdep code. I tried quite hard to trigger lockdep splats manually, but the only way I could do it was by using the "%pS" specifier to print the name of a symbol in a module, which would cause an RCU walk of the module symbols in the kallsyms code! Manually calling WARN() or even rcu_read_lock() / spin_lock() did _not_ trigger a splat.
Since that's not something I think we should be doing this early, I decided to leave the code as-is unless I have a way to trigger a lockdep splat with the relatively simple prints we have on the early error paths.
Will
Re: [PATCH 09/19] arm64: smp: Defer RCU registration during secondary CPU bringup
Quoted 40 lines · Will Deacon
Hi Jinjie,
Quoted 32 lines · Jinjie Ruan
Quoted 22 lines · Will Deacon
Calling
rcutree_report_cpu_starting()early during boot can lead to livelocks with the generic CPU hotplug mechanism if the boot CPU blocks on an RCU grace period while the CPU being onlined is spinning incpuhp_ap_sync_alive().In preparation for enabling the generic CPU hotplug code on arm64, split up the
trace_hardirqs_off()call during secondary CPU bringup so that we update lockdep early but defer the tracing updates until afternotify_cpu_starting()has registered the new CPU with RCU, allowing us to drop the explicit call torcutree_report_cpu_starting()entirely.Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/smp.c | 5 ++--- include/linux/rcutree.h | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-)arch/arm64/kernel/smp.c +0
-0arch/arm64/kernel/smp.c asmlinkage notrace void secondary_start_kernel(void) · line 221
Quoted 5 lines
arch/arm64/kernel/smp.c +0
-2@@ -217,8 +217,7 @@ asmlinkage notrace void secondary_start_kernel(void)if (system_uses_irq_prio_masking ())init_gic_priority_masking (); -rcutree_report_cpu_starting (cpu); -trace_hardirqs_off ();I think we need to handle the printk problem before this patch as we discussed earlier.
Quoted 4 lines
Otherwise defer the
rcutree_report_cpu_starting()will trigger a false-positive lockdep"suspicious RCU usage" splat during early lock acquisitions as commitce3d31ad3cac("arm64/smp: Movercu_cpu_starting()earlier") pointed out.
Quoted 8 lines
Sorry, I meant to mention this in the cover letter but forgot about it. I'm not sure that
ce3d31ad3cac("arm64/smp: Movercu_cpu_starting()earlier") is still relevant with the latest printk/console/lockdep code. I tried quite hard to trigger lockdep splats manually, but the only way I could do it was by using the "%pS" specifier to print the name of a symbol in a module, which would cause an RCU walk of the module symbols in the kallsyms code! Manually callingWARN()or evenrcu_read_lock()/spin_lock()did _not_ trigger a splat.
Hi Will,
Add "dyndbg="+p"" in cmdline, CONFIG_DEBUG_LOCK_ALLOC=y, CONFIG_PROVE_RCU_LIST=y, we can reproduce the warning as below:
I believe there is also a problem in the RISC-V code itself here as store_cpu_topology() is common for RISC-V.
[ 0.335162] smp: Bringing up secondary CPUs ...
[ 0.345495]
[ 0.345513] =============================
[ 0.345523] WARNING: suspicious RCU usage
[ 0.345621] 7.3.0-rc2-00010-g2311ba2cd56f #500 Tainted: G W
[ 0.345637] -----------------------------
[ 0.345646] kernel/locking/lockdep.c:3845 RCU-list traversed in
non-reader section!!
[ 0.345659]
[ 0.345659] other info that might help us debug this:
[ 0.345659]
[ 0.345680]
[ 0.345680] RCU used illegally from offline CPU!
[ 0.345680] rcu_scheduler_active = 1, debug_locks = 1
[ 0.345725] locks held by swapper/1/0: 0, last CPU#1
[ 0.345743]
[ 0.345743] stack backtrace:
[ 0.345834] CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Tainted: G W
7.3.0-rc2-00010-g2311ba2cd56f #500 PREEMPT(full)
[ 0.345885] Tainted: [W]=WARN
[ 0.346077] Call trace:
[ 0.346102] show_stack+0x20/0x38 (C)
[ 0.346153] dump_stack_lvl+0xc4/0x150
[ 0.346176] dump_stack+0x18/0x28
[ 0.346194] lockdep_rcu_suspicious+0x170/0x238
[ 0.346217] __lock_acquire+0xf08/0x1818
[ 0.346237] lock_acquire+0x1e0/0x450
[ 0.346256] _raw_spin_lock_irqsave+0x70/0xc0
[ 0.346277] down_trylock+0x20/0x60
[ 0.346293] __down_trylock_console_sem+0x4c/0x118
[ 0.346316] vprintk_emit+0x2d8/0x3f8
[ 0.346333] vprintk_default+0x40/0x58
[ 0.346350] vprintk+0x3c/0x80
[ 0.346366] _printk+0x64/0x98
[ 0.346386] __dynamic_pr_debug+0x90/0xd8
[ 0.346406] acpi_get_cache_info+0x140/0x1a0
[ 0.346430] init_cache_level+0xec/0x110
[ 0.346450] detect_cache_attributes+0x74/0x7c0
[ 0.346473] update_siblings_masks+0x30/0x300
[ 0.346495] store_cpu_topology+0x70/0xf0
[ 0.346515] secondary_start_kernel+0xe0/0x178
[ 0.346535] __secondary_switched+0xc0/0xc8Quoted 6 lines
Since that's not something I think we should be doing this early, I decided to leave the code as-is unless I have a way to trigger a lockdep splat with the relatively simple prints we have on the early error paths.
Will
Will Deacon
maintainerRe: [PATCH 09/19] arm64: smp: Defer RCU registration during secondary CPU bringup
Quoted 59 lines · Jinjie Ruan
Quoted 9 lines · Will Deacon
Quoted 7 lines · Jinjie Ruan
I think we need to handle the printk problem before this patch as we discussed earlier.
Otherwise defer the
rcutree_report_cpu_starting()will trigger a false-positive lockdep"suspicious RCU usage" splat during early lock acquisitions as commitce3d31ad3cac("arm64/smp: Movercu_cpu_starting()earlier") pointed out.Quoted 8 lines
Sorry, I meant to mention this in the cover letter but forgot about it. I'm not sure that
ce3d31ad3cac("arm64/smp: Movercu_cpu_starting()earlier") is still relevant with the latest printk/console/lockdep code. I tried quite hard to trigger lockdep splats manually, but the only way I could do it was by using the "%pS" specifier to print the name of a symbol in a module, which would cause an RCU walk of the module symbols in the kallsyms code! Manually callingWARN()or evenrcu_read_lock()/spin_lock()did _not_ trigger a splat.Add "dyndbg="+p"" in cmdline,
CONFIG_DEBUG_LOCK_ALLOC=y,CONFIG_PROVE_RCU_LIST=y, we can reproduce the warning as below:I believe there is also a problem in the RISC-V code itself here as
store_cpu_topology()is common for RISC-V.[ 0.335162] smp: Bringing up secondary CPUs ... [ 0.345495] [ 0.345513] ============================= [ 0.345523] WARNING: suspicious RCU usage [ 0.345621] 7.3.0-rc2-00010-g2311ba2cd56f #500 Tainted: G W [ 0.345637] ----------------------------- [ 0.345646] kernel/locking/lockdep.c:3845 RCU-list traversed in non-reader section!! [ 0.345659] [ 0.345659] other info that might help us debug this: [ 0.345659] [ 0.345680] [ 0.345680] RCU used illegally from offline CPU! [ 0.345680] rcu_scheduler_active = 1, debug_locks = 1 [ 0.345725] locks held by swapper/1/0: 0, last CPU#1 [ 0.345743] [ 0.345743] stack backtrace: [ 0.345834] CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Tainted: G W 7.3.0-rc2-00010-g2311ba2cd56f #500 PREEMPT(full) [ 0.345885] Tainted: [W]=WARN [ 0.346077] Call trace: [ 0.346102] show_stack+0x20/0x38 (C) [ 0.346153] dump_stack_lvl+0xc4/0x150 [ 0.346176] dump_stack+0x18/0x28 [ 0.346194] lockdep_rcu_suspicious+0x170/0x238 [ 0.346217] __lock_acquire+0xf08/0x1818 [ 0.346237] lock_acquire+0x1e0/0x450 [ 0.346256] _raw_spin_lock_irqsave+0x70/0xc0 [ 0.346277] down_trylock+0x20/0x60 [ 0.346293] __down_trylock_console_sem+0x4c/0x118 [ 0.346316] vprintk_emit+0x2d8/0x3f8 [ 0.346333] vprintk_default+0x40/0x58 [ 0.346350] vprintk+0x3c/0x80 [ 0.346366] _printk+0x64/0x98
Quoted 8 lines
[ 0.346386] __dynamic_pr_debug+0x90/0xd8 [ 0.346406] acpi_get_cache_info+0x140/0x1a0 [ 0.346430] init_cache_level+0xec/0x110 [ 0.346450] detect_cache_attributes+0x74/0x7c0 [ 0.346473] update_siblings_masks+0x30/0x300 [ 0.346495] store_cpu_topology+0x70/0xf0 [ 0.346515] secondary_start_kernel+0xe0/0x178 [ 0.346535] __secondary_switched+0xc0/0xc8
I was about to say "don't do this" but then I realised two things:
1. update_siblings_masks() can trigger lockdep splats outside of pr_debug() if RCU isn't up and running, e.g.:
[ 0.524042] show_stack+0x18/0x24 (C)
[ 0.524519] __dump_stack+0x28/0x38
[ 0.524546] dump_stack_lvl+0x64/0x84
[ 0.524562] dump_stack+0x18/0x24
[ 0.524576] lockdep_rcu_suspicious+0x134/0x1cc
[ 0.524591] __lock_acquire+0xee8/0x2cb0
[ 0.524606] lock_acquire+0x11c/0x2fc
[ 0.524621] _raw_spin_lock_irqsave+0x64/0x84
[ 0.524641] of_find_property+0x2c/0x8c
[ 0.524659] detect_cache_attributes+0x1c0/0x6d0
[ 0.524676] update_siblings_masks+0x38/0x288
[ 0.524692] store_cpu_topology+0x4c/0x58
[ 0.524706] secondary_start_kernel+0xdc/0x1c8
[ 0.524722] __secondary_switched+0x120/0x1242. This code is running _after_ cpuhp_ap_sync_alive().
So for the next version, I'll reintroduce the call to rcutree_report_cpu_starting(), but move it immediately after the call to cpuhp_ap_sync_alive(). I think that will solve these issues, without causing issues with the concurrent part of early boot and also without reintroducing the early call to rcutree_report_cpu_dead().
Cheers,
Will
Re: [PATCH 09/19] arm64: smp: Defer RCU registration during secondary CPU bringup
Quoted 89 lines · Will Deacon
Quoted 59 lines · Jinjie Ruan
Quoted 9 lines · Will Deacon
Quoted 7 lines · Jinjie Ruan
I think we need to handle the printk problem before this patch as we discussed earlier.
Otherwise defer the
rcutree_report_cpu_starting()will trigger a false-positive lockdep"suspicious RCU usage" splat during early lock acquisitions as commitce3d31ad3cac("arm64/smp: Movercu_cpu_starting()earlier") pointed out.Quoted 8 lines
Sorry, I meant to mention this in the cover letter but forgot about it. I'm not sure that
ce3d31ad3cac("arm64/smp: Movercu_cpu_starting()earlier") is still relevant with the latest printk/console/lockdep code. I tried quite hard to trigger lockdep splats manually, but the only way I could do it was by using the "%pS" specifier to print the name of a symbol in a module, which would cause an RCU walk of the module symbols in the kallsyms code! Manually callingWARN()or evenrcu_read_lock()/spin_lock()did _not_ trigger a splat.Add "dyndbg="+p"" in cmdline,
CONFIG_DEBUG_LOCK_ALLOC=y,CONFIG_PROVE_RCU_LIST=y, we can reproduce the warning as below:I believe there is also a problem in the RISC-V code itself here as
store_cpu_topology()is common for RISC-V.[ 0.335162] smp: Bringing up secondary CPUs ... [ 0.345495] [ 0.345513] ============================= [ 0.345523] WARNING: suspicious RCU usage [ 0.345621] 7.3.0-rc2-00010-g2311ba2cd56f #500 Tainted: G W [ 0.345637] ----------------------------- [ 0.345646] kernel/locking/lockdep.c:3845 RCU-list traversed in non-reader section!! [ 0.345659] [ 0.345659] other info that might help us debug this: [ 0.345659] [ 0.345680] [ 0.345680] RCU used illegally from offline CPU! [ 0.345680] rcu_scheduler_active = 1, debug_locks = 1 [ 0.345725] locks held by swapper/1/0: 0, last CPU#1 [ 0.345743] [ 0.345743] stack backtrace: [ 0.345834] CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Tainted: G W 7.3.0-rc2-00010-g2311ba2cd56f #500 PREEMPT(full) [ 0.345885] Tainted: [W]=WARN [ 0.346077] Call trace: [ 0.346102] show_stack+0x20/0x38 (C) [ 0.346153] dump_stack_lvl+0xc4/0x150 [ 0.346176] dump_stack+0x18/0x28 [ 0.346194] lockdep_rcu_suspicious+0x170/0x238 [ 0.346217] __lock_acquire+0xf08/0x1818 [ 0.346237] lock_acquire+0x1e0/0x450 [ 0.346256] _raw_spin_lock_irqsave+0x70/0xc0 [ 0.346277] down_trylock+0x20/0x60 [ 0.346293] __down_trylock_console_sem+0x4c/0x118 [ 0.346316] vprintk_emit+0x2d8/0x3f8 [ 0.346333] vprintk_default+0x40/0x58 [ 0.346350] vprintk+0x3c/0x80 [ 0.346366] _printk+0x64/0x98Quoted 8 lines
[ 0.346386] __dynamic_pr_debug+0x90/0xd8 [ 0.346406] acpi_get_cache_info+0x140/0x1a0 [ 0.346430] init_cache_level+0xec/0x110 [ 0.346450] detect_cache_attributes+0x74/0x7c0 [ 0.346473] update_siblings_masks+0x30/0x300 [ 0.346495] store_cpu_topology+0x70/0xf0 [ 0.346515] secondary_start_kernel+0xe0/0x178 [ 0.346535] __secondary_switched+0xc0/0xc8I was about to say "don't do this" but then I realised two things:
1.
update_siblings_masks()can trigger lockdep splats outside ofpr_debug()if RCU isn't up and running, e.g.:[ 0.524042] show_stack+0x18/0x24 (C) [ 0.524519] __dump_stack+0x28/0x38 [ 0.524546] dump_stack_lvl+0x64/0x84 [ 0.524562] dump_stack+0x18/0x24 [ 0.524576] lockdep_rcu_suspicious+0x134/0x1cc [ 0.524591] __lock_acquire+0xee8/0x2cb0 [ 0.524606] lock_acquire+0x11c/0x2fc [ 0.524621] _raw_spin_lock_irqsave+0x64/0x84 [ 0.524641] of_find_property+0x2c/0x8c [ 0.524659] detect_cache_attributes+0x1c0/0x6d0 [ 0.524676] update_siblings_masks+0x38/0x288 [ 0.524692] store_cpu_topology+0x4c/0x58 [ 0.524706] secondary_start_kernel+0xdc/0x1c8 [ 0.524722] __secondary_switched+0x120/0x124
Quoted 5 lines
2. This code is running _after_
cpuhp_ap_sync_alive().So for the next version, I'll reintroduce the call to
rcutree_report_cpu_starting(), but move it immediately after the call tocpuhp_ap_sync_alive(). I think that will solve these issues, without
pr_crit() and pr_warn() (such as vec_verify_vq_map()) in check_local_cpu_capabilities() can also trigger lockdep splats as below.
But I think this is not common on the failure path, so it seems to have little impact..
[ 0.158619] smp: Bringing up secondary CPUs ...
[ 0.173958] CPU1: missing HWCAP.
[ 0.174071]
[ 0.174088] =============================
[ 0.174099] WARNING: suspicious RCU usage
[ 0.174197] 7.3.0-rc2-00020-gef0bd63bdd97-dirty #504 Tainted: G W
[ 0.174217] -----------------------------
[ 0.174226] kernel/locking/lockdep.c:3845 RCU-list traversed in
non-reader section!!
[ 0.174240]
[ 0.174240] other info that might help us debug this:
[ 0.174240]
[ 0.174262]
[ 0.174262] RCU used illegally from offline CPU!
[ 0.174262] rcu_scheduler_active = 1, debug_locks = 1
[ 0.174306] locks held by swapper/1/0: 0, last CPU#1
[ 0.174326]
[ 0.174326] stack backtrace:
[ 0.174412] CPU: 1 UID: 0 PID: 0 Comm: swapper/1 Tainted: G W
7.3.0-rc2-00020-gef0bd63bdd97-dirty #504 PREEMPT(full)
[ 0.174462] Tainted: [W]=WARN
[ 0.174488] Call trace:
[ 0.174513] show_stack+0x20/0x38 (C)
[ 0.174568] dump_stack_lvl+0xc4/0x150
[ 0.174594] dump_stack+0x18/0x28
[ 0.174615] lockdep_rcu_suspicious+0x170/0x238
[ 0.174641] __lock_acquire+0xf08/0x1818
[ 0.174664] lock_acquire+0x1e0/0x450
[ 0.174687] _raw_spin_lock_irqsave+0x70/0xc0
[ 0.174709] down_trylock+0x20/0x60
[ 0.174728] __down_trylock_console_sem+0x4c/0x118
[ 0.174748] vprintk_emit+0x2d8/0x3f8
[ 0.174769] vprintk_default+0x40/0x58
[ 0.174788] vprintk+0x3c/0x80
[ 0.174808] _printk+0x64/0x98
[ 0.174832] secondary_start_kernel+0xc8/0x190
[ 0.174857] __secondary_switched+0x120/0x128Quoted 6 lines
causing issues with the concurrent part of early boot and also without reintroducing the early call to
rcutree_report_cpu_dead().Cheers,
Will
Will Deacon
maintainerRe: [PATCH 09/19] arm64: smp: Defer RCU registration during secondary CPU bringup
Quoted 27 lines · Jinjie Ruan
Quoted 20 lines · Will Deacon
I was about to say "don't do this" but then I realised two things:
1.
update_siblings_masks()can trigger lockdep splats outside ofpr_debug()if RCU isn't up and running, e.g.:[ 0.524042] show_stack+0x18/0x24 (C) [ 0.524519] __dump_stack+0x28/0x38 [ 0.524546] dump_stack_lvl+0x64/0x84 [ 0.524562] dump_stack+0x18/0x24 [ 0.524576] lockdep_rcu_suspicious+0x134/0x1cc [ 0.524591] __lock_acquire+0xee8/0x2cb0 [ 0.524606] lock_acquire+0x11c/0x2fc [ 0.524621] _raw_spin_lock_irqsave+0x64/0x84 [ 0.524641] of_find_property+0x2c/0x8c [ 0.524659] detect_cache_attributes+0x1c0/0x6d0 [ 0.524676] update_siblings_masks+0x38/0x288 [ 0.524692] store_cpu_topology+0x4c/0x58 [ 0.524706] secondary_start_kernel+0xdc/0x1c8 [ 0.524722] __secondary_switched+0x120/0x124Quoted 5 lines
2. This code is running _after_
cpuhp_ap_sync_alive().So for the next version, I'll reintroduce the call to
rcutree_report_cpu_starting(), but move it immediately after the call tocpuhp_ap_sync_alive(). I think that will solve these issues, without
Quoted 5 lines
pr_crit()andpr_warn()(such asvec_verify_vq_map()) incheck_local_cpu_capabilities()can also trigger lockdep splats as below.But I think this is not common on the failure path, so it seems to have little impact..
I agree that we shouldn't bend the code out-of-shape to squash a debug warning on an error path (ideally, printk would handle this internally), especially as the original message does seem to get printed in the end.
Quoted 9 lines
[ 0.158619] smp: Bringing up secondary CPUs ... [ 0.173958] CPU1: missing HWCAP. [ 0.174071] [ 0.174088] ============================= [ 0.174099] WARNING: suspicious RCU usage [ 0.174197] 7.3.0-rc2-00020-gef0bd63bdd97-dirty #504 Tainted: G W [ 0.174217] ----------------------------- [ 0.174226] kernel/locking/lockdep.c:3845 RCU-list traversed innon-reader section!!
However, if this is caused by vec_verify_vq_map(), then how are you getting that to run on the early boot path? check_local_cpu_capabilities() only calls verify_local_cpu_capabilities() if system_capabilities_finalized().
Or is the backtrace for something else?
Will
Re: [PATCH 09/19] arm64: smp: Defer RCU registration during secondary CPU bringup
Quoted 48 lines · Will Deacon
Quoted 27 lines · Jinjie Ruan
Quoted 20 lines · Will Deacon
I was about to say "don't do this" but then I realised two things:
1.
update_siblings_masks()can trigger lockdep splats outside ofpr_debug()if RCU isn't up and running, e.g.:[ 0.524042] show_stack+0x18/0x24 (C) [ 0.524519] __dump_stack+0x28/0x38 [ 0.524546] dump_stack_lvl+0x64/0x84 [ 0.524562] dump_stack+0x18/0x24 [ 0.524576] lockdep_rcu_suspicious+0x134/0x1cc [ 0.524591] __lock_acquire+0xee8/0x2cb0 [ 0.524606] lock_acquire+0x11c/0x2fc [ 0.524621] _raw_spin_lock_irqsave+0x64/0x84 [ 0.524641] of_find_property+0x2c/0x8c [ 0.524659] detect_cache_attributes+0x1c0/0x6d0 [ 0.524676] update_siblings_masks+0x38/0x288 [ 0.524692] store_cpu_topology+0x4c/0x58 [ 0.524706] secondary_start_kernel+0xdc/0x1c8 [ 0.524722] __secondary_switched+0x120/0x124Quoted 5 lines
2. This code is running _after_
cpuhp_ap_sync_alive().So for the next version, I'll reintroduce the call to
rcutree_report_cpu_starting(), but move it immediately after the call tocpuhp_ap_sync_alive(). I think that will solve these issues, withoutQuoted 5 lines
pr_crit()andpr_warn()(such asvec_verify_vq_map()) incheck_local_cpu_capabilities()can also trigger lockdep splats as below.But I think this is not common on the failure path, so it seems to have little impact..
I agree that we shouldn't bend the code out-of-shape to squash a debug warning on an error path (ideally, printk would handle this internally), especially as the original message does seem to get printed in the end.
Quoted 9 lines
[ 0.158619] smp: Bringing up secondary CPUs ... [ 0.173958] CPU1: missing HWCAP. [ 0.174071] [ 0.174088] ============================= [ 0.174099] WARNING: suspicious RCU usage [ 0.174197] 7.3.0-rc2-00020-gef0bd63bdd97-dirty #504 Tainted: G W [ 0.174217] ----------------------------- [ 0.174226] kernel/locking/lockdep.c:3845 RCU-list traversed innon-reader section!!
Quoted 2 lines
However, if this is caused by
vec_verify_vq_map(), then how are you getting that to run on the early boot path?check_local_cpu_capabilities()only
Sorry, based on the fact that pr_crit() or pr_warn() is printed before rcutree_report_cpu_starting() is called, I modified the code to add print tests to simulate this situation.
Quoted 5 lines
calls
verify_local_cpu_capabilities()ifsystem_capabilities_finalized().Or is the backtrace for something else?
Will
Will Deacon
maintainerPATCH 10/19 arm64: smp: Use generic HOTPLUG_CORE_SYNC_FULL machinery for CPU onlining
In reply to Will DeaconSelect HOTPLUG_CORE_SYNC_FULL on arm64 to replace the 'cpu_running' completion with the generic code for synchronising with secondary CPUs during boot.
--- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/smp.h | 1 - arch/arm64/kernel/smp.c | 39 +++++++++++++++++------------------- 3 files changed, 19 insertions(+), 23 deletions(-)
arch/arm64/Kconfig +1 -1
@@ -230,7 +230,7 @@ config ARM64 select HAVE_SYSCALL_TRACEPOINTS select HAVE_KPROBES select HAVE_KRETPROBES - select HOTPLUG_CORE_SYNC_DEAD if HOTPLUG_CPU + select HOTPLUG_CORE_SYNC_FULL select HOTPLUG_SMT if HOTPLUG_CPU select IRQ_DOMAIN select IRQ_FORCED_THREADING
arch/arm64/include/asm/smp.h +0 -1
@@ -12,7 +12,6 @@#define CPU_BOOT_STATUS_MASK ((UL(1) << CPU_STUCK_REASON_SHIFT) - 1)#define CPU_MMU_OFF (-1) -#define CPU_BOOT_SUCCESS (0)/* The cpu invoked ops->cpu_die, synchronise it with cpu_kill */ #define CPU_KILL_ME (1)/* The cpu couldn't die gracefully and is looping in the kernel */
arch/arm64/kernel/smp.c +18 -21
@@ -108,12 +108,9 @@ static int boot_secondary(unsigned int cpu, struct task_struct *idle)return -EOPNOTSUPP ; } -static DECLARE_COMPLETION (cpu_running); -int __cpu_up (unsigned int cpu,struct task_struct *idle) {int ret; -long status;/* * We need to tell the secondary core where to find its stack and the
@@ -124,27 +121,24 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)/* Now bring the CPU into our world */ ret =boot_secondary (cpu, idle); -if (ret) { -if (ret != -EPERM ) -pr_err ("CPU%u: failed to boot: %d\n" , cpu, ret); -return ret; - } +if (ret && ret != -EPERM ) +pr_err ("CPU%u: failed to boot: %d\n" , cpu, ret); +return ret; +} -/* -* CPU was successfully started, wait for it to come online or -* time out. -*/ -wait_for_completion_timeout (&cpu_running, -msecs_to_jiffies (5000 )); -if (cpu_online (cpu)) -return 0 ; +void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu,bool is_alive) +{ +long status; + +if (is_alive) +return ; -pr_crit ("CPU%u: failed to come online\n" , cpu); secondary_data.task =NULL ; status =READ_ONCE (secondary_data.status);if (status ==CPU_MMU_OFF ) status =READ_ONCE (__early_cpu_boot_status); +/* A CPU has failed to boot. Try to figure out what happened. */ switch (status &CPU_BOOT_STATUS_MASK ) {default : pr_err("CPU%u: failed in unknown state : 0x%lx\n",
@@ -171,8 +165,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)case CPU_PANIC_KERNEL :panic ("CPU%u detected unsupported configuration\n" , cpu); } - -return -EIO ; }static void init_gic_priority_masking (void )
@@ -234,6 +226,13 @@ asmlinkage notrace void secondary_start_kernel(void)* Log the CPU info before it is marked online and might get read. */ cpuinfo_store_cpu (); + +/* +* Synchronise with the core bringing us online so that it knows +* we made it into the kernel. We're still not 'online'. +*/ +cpuhp_ap_sync_alive (); +update_cpu_features (cpu);store_cpu_topology (cpu);
@@ -255,9 +254,7 @@ asmlinkage notrace void secondary_start_kernel(void)pr_info ("CPU%u: Booted secondary processor 0x%010lx [0x%08x]\n" , cpu, (unsigned long )mpidr,read_cpuid_id ()); -update_cpu_boot_status (CPU_BOOT_SUCCESS );set_cpu_online (cpu, true); -complete (&cpu_running);/* * Secondary CPUs enter the kernel with all DAIF exceptions masked.
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 10/19] arm64: smp: Use generic HOTPLUG_CORE_SYNC_FULL machinery for CPU onlining
Quoted 116 lines · Will Deacon
Select
HOTPLUG_CORE_SYNC_FULLon arm64 to replace the 'cpu_running' completion with the generic code for synchronising with secondary CPUs during boot.Signed-off-by Will Deacon <[email protected]>--- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/smp.h | 1 - arch/arm64/kernel/smp.c | 39 +++++++++++++++++------------------- 3 files changed, 19 insertions(+), 23 deletions(-)arch/arm64/Kconfig +1
-1@@ -230,7 +230,7 @@ config ARM64 select HAVE_SYSCALL_TRACEPOINTS select HAVE_KPROBES select HAVE_KRETPROBES - select HOTPLUG_CORE_SYNC_DEAD if HOTPLUG_CPU + select HOTPLUG_CORE_SYNC_FULL select HOTPLUG_SMT if HOTPLUG_CPU select IRQ_DOMAIN select IRQ_FORCED_THREADINGarch/arm64/include/asm/smp.h +0
-1@@ -12,7 +12,6 @@#define CPU_BOOT_STATUS_MASK ((UL(1) << CPU_STUCK_REASON_SHIFT) - 1)#define CPU_MMU_OFF (-1) -#define CPU_BOOT_SUCCESS (0)/* The cpu invoked ops->cpu_die, synchronise it with cpu_kill */ #define CPU_KILL_ME (1)/* The cpu couldn't die gracefully and is looping in the kernel */ arch/arm64/kernel/smp.c +18
-19@@ -108,12 +108,9 @@ static int boot_secondary(unsigned int cpu, struct task_struct *idle)return -EOPNOTSUPP ; } -static DECLARE_COMPLETION (cpu_running); -int __cpu_up (unsigned int cpu,struct task_struct *idle) {int ret; -long status;/* * We need to tell the secondary core where to find its stack and the @@ -124,27 +121,24 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)/* Now bring the CPU into our world */ ret =boot_secondary (cpu, idle); -if (ret) { -if (ret != -EPERM ) -pr_err ("CPU%u: failed to boot: %d\n" , cpu, ret); -return ret; - } +if (ret && ret != -EPERM ) +pr_err ("CPU%u: failed to boot: %d\n" , cpu, ret); +return ret; +} -/* -* CPU was successfully started, wait for it to come online or -* time out. -*/ -wait_for_completion_timeout (&cpu_running, -msecs_to_jiffies (5000 )); -if (cpu_online (cpu)) -return 0 ; +void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu,bool is_alive) +{ +long status; + +if (is_alive) +return ; -pr_crit ("CPU%u: failed to come online\n" , cpu); secondary_data.task =NULL ; status =READ_ONCE (secondary_data.status);if (status ==CPU_MMU_OFF ) status =READ_ONCE (__early_cpu_boot_status); +/* A CPU has failed to boot. Try to figure out what happened. */ switch (status &CPU_BOOT_STATUS_MASK ) {default : pr_err("CPU%u: failed in unknown state : 0x%lx\n",@@ -171,8 +165,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)case CPU_PANIC_KERNEL :panic ("CPU%u detected unsupported configuration\n" , cpu); } - -return -EIO ; }static void init_gic_priority_masking (void )@@ -234,6 +226,13 @@ asmlinkage notrace void secondary_start_kernel(void)* Log the CPU info before it is marked online and might get read. */ cpuinfo_store_cpu (); + +/* +* Synchronise with the core bringing us online so that it knows +* we made it into the kernel. We're still not 'online'. +*/ +cpuhp_ap_sync_alive (); +update_cpu_features (cpu);store_cpu_topology (cpu);
arch/arm64/kernel/smp.c asmlinkage notrace void secondary_start_kernel(void) · line 260
Quoted 6 lines
arch/arm64/kernel/smp.c +0
-2@@ -255,9 +254,7 @@ asmlinkage notrace void secondary_start_kernel(void)pr_info ("CPU%u: Booted secondary processor 0x%010lx [0x%08x]\n" , cpu, (unsigned long )mpidr,read_cpuid_id ()); -update_cpu_boot_status (CPU_BOOT_SUCCESS );set_cpu_online (cpu, true); -complete (&cpu_running);
Quoted 3 lines
arch/arm64/kernel/smp.c +0
-0/* * Secondary CPUs enter the kernel with all DAIF exceptions masked.
Will Deacon
maintainerPATCH 11/19 arm64: smp: Use generic HOTPLUG_SPLIT_STARTUP machinery for CPU onlining
In reply to Will DeaconIn preparation for enabling parallel bringup of secondary CPUs on arm64, take the baby step of moving from HOTPLUG_CORE_SYNC_FULL to HOTPLUG_SPLIT_STARTUP.
Rework the cpu_die_early() path to use a private cpumask, otherwise clearing the incoming CPU from the present mask in the 'kick' stage will prevent the hotplug stage machine from progressing and arch_cpuhp_cleanup_kick_cpu() will not be called.
--- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/smp.h | 1 + arch/arm64/kernel/smp.c | 9 +++++---- 3 files changed, 7 insertions(+), 5 deletions(-)
arch/arm64/Kconfig +1 -1
@@ -230,7 +230,7 @@ config ARM64 select HAVE_SYSCALL_TRACEPOINTS select HAVE_KPROBES select HAVE_KRETPROBES - select HOTPLUG_CORE_SYNC_FULL + select HOTPLUG_SPLIT_STARTUP select HOTPLUG_SMT if HOTPLUG_CPU select IRQ_DOMAIN select IRQ_FORCED_THREADING
arch/arm64/include/asm/smp.h +1 -0
@@ -89,6 +89,7 @@ asmlinkage void secondary_start_kernel(void);struct secondary_data {struct task_struct *task;long status; +cpumask_t cpu_died_early_mask; };extern struct secondary_data secondary_data;
arch/arm64/kernel/smp.c +5 -4
@@ -62,7 +62,7 @@* so we need some other way of telling a new secondary core * where to place its SVC stack */ -struct secondary_data secondary_data; +struct secondary_data secondary_data = {};/* Number of CPUs which aren't online, but looping in kernel text. */ static int cpus_stuck_in_kernel;
@@ -108,7 +108,7 @@ static int boot_secondary(unsigned int cpu, struct task_struct *idle)return -EOPNOTSUPP ; } -int __cpu_up (unsigned int cpu,struct task_struct *idle) +int arch_cpuhp_kick_ap_alive (unsigned int cpu,struct task_struct *idle) {int ret;
@@ -146,6 +146,8 @@ void arch_cpuhp_cleanup_kick_cpu(unsigned int cpu, bool is_alive) cpus_stuck_in_kernel++;break ;case CPU_KILL_ME : +if (cpumask_test_cpu (cpu, &secondary_data.cpu_died_early_mask)) +set_cpu_present (cpu, false);if (!op_cpu_kill (cpu)) {pr_crit ("CPU%u: died during early boot\n" , cpu);break ;
@@ -406,8 +408,7 @@ void __noreturn cpu_die_early(void)pr_crit ("CPU%d: will not boot\n" , cpu); -/* Mark this CPU absent */ -set_cpu_present (cpu,0 ); +cpumask_set_cpu (cpu, &secondary_data.cpu_died_early_mask);if (IS_ENABLED (CONFIG_HOTPLUG_CPU )) {update_cpu_boot_status (CPU_KILL_ME );
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 11/19] arm64: smp: Use generic HOTPLUG_SPLIT_STARTUP machinery for CPU onlining
Quoted 64 lines · Will Deacon
In preparation for enabling parallel bringup of secondary CPUs on arm64, take the baby step of moving from
HOTPLUG_CORE_SYNC_FULLtoHOTPLUG_SPLIT_STARTUP.Rework the
cpu_die_early()path to use a private cpumask, otherwise clearing the incoming CPU from the present mask in the 'kick' stage will prevent the hotplug stage machine from progressing andarch_cpuhp_cleanup_kick_cpu()will not be called.Signed-off-by Will Deacon <[email protected]>--- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/smp.h | 1 + arch/arm64/kernel/smp.c | 9 +++++---- 3 files changed, 7 insertions(+), 5 deletions(-)arch/arm64/Kconfig +1
-1@@ -230,7 +230,7 @@ config ARM64 select HAVE_SYSCALL_TRACEPOINTS select HAVE_KPROBES select HAVE_KRETPROBES - select HOTPLUG_CORE_SYNC_FULL + select HOTPLUG_SPLIT_STARTUP select HOTPLUG_SMT if HOTPLUG_CPU select IRQ_DOMAIN select IRQ_FORCED_THREADINGarch/arm64/include/asm/smp.h +1
-0@@ -89,6 +89,7 @@ asmlinkage void secondary_start_kernel(void);struct secondary_data {struct task_struct *task;long status; +cpumask_t cpu_died_early_mask; };extern struct secondary_data secondary_data;arch/arm64/kernel/smp.c +2
-2@@ -62,7 +62,7 @@* so we need some other way of telling a new secondary core * where to place its SVC stack */ -struct secondary_data secondary_data; +struct secondary_data secondary_data = {};/* Number of CPUs which aren't online, but looping in kernel text. */ static int cpus_stuck_in_kernel;@@ -108,7 +108,7 @@ static int boot_secondary(unsigned int cpu, struct task_struct *idle)return -EOPNOTSUPP ; } -int __cpu_up (unsigned int cpu,struct task_struct *idle) +int arch_cpuhp_kick_ap_alive (unsigned int cpu,struct task_struct *idle) {int ret;
arch/arm64/kernel/smp.c void arch_cpuhp_cleanup_kick_cpu(unsigned int cpu, bool is_alive) · line 150
Quoted 5 lines
arch/arm64/kernel/smp.c +2
-0@@ -146,6 +146,8 @@ void arch_cpuhp_cleanup_kick_cpu(unsigned int cpu, bool is_alive) cpus_stuck_in_kernel++;break ;case CPU_KILL_ME : +if (cpumask_test_cpu (cpu, &secondary_data.cpu_died_early_mask)) +set_cpu_present (cpu, false);
Quoted 13 lines
arch/arm64/kernel/smp.c +1
-2if (!op_cpu_kill (cpu)) {pr_crit ("CPU%u: died during early boot\n" , cpu);break ;@@ -406,8 +408,7 @@ void __noreturn cpu_die_early(void)pr_crit ("CPU%d: will not boot\n" , cpu); -/* Mark this CPU absent */ -set_cpu_present (cpu,0 ); +cpumask_set_cpu (cpu, &secondary_data.cpu_died_early_mask);if (IS_ENABLED (CONFIG_HOTPLUG_CPU )) {update_cpu_boot_status (CPU_KILL_ME );
Re: [PATCH 11/19] arm64: smp: Use generic HOTPLUG_SPLIT_STARTUP machinery for CPU onlining
In reply to Will DeaconQuoted 64 lines · Will Deacon
In preparation for enabling parallel bringup of secondary CPUs on arm64, take the baby step of moving from
HOTPLUG_CORE_SYNC_FULLtoHOTPLUG_SPLIT_STARTUP.Rework the
cpu_die_early()path to use a private cpumask, otherwise clearing the incoming CPU from the present mask in the 'kick' stage will prevent the hotplug stage machine from progressing andarch_cpuhp_cleanup_kick_cpu()will not be called.Signed-off-by Will Deacon <[email protected]>--- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/smp.h | 1 + arch/arm64/kernel/smp.c | 9 +++++---- 3 files changed, 7 insertions(+), 5 deletions(-)arch/arm64/Kconfig +1
-1@@ -230,7 +230,7 @@ config ARM64 select HAVE_SYSCALL_TRACEPOINTS select HAVE_KPROBES select HAVE_KRETPROBES - select HOTPLUG_CORE_SYNC_FULL + select HOTPLUG_SPLIT_STARTUP select HOTPLUG_SMT if HOTPLUG_CPU select IRQ_DOMAIN select IRQ_FORCED_THREADINGarch/arm64/include/asm/smp.h +1
-0@@ -89,6 +89,7 @@ asmlinkage void secondary_start_kernel(void);struct secondary_data {struct task_struct *task;long status; +cpumask_t cpu_died_early_mask; };extern struct secondary_data secondary_data;arch/arm64/kernel/smp.c +2
-2@@ -62,7 +62,7 @@* so we need some other way of telling a new secondary core * where to place its SVC stack */ -struct secondary_data secondary_data; +struct secondary_data secondary_data = {};/* Number of CPUs which aren't online, but looping in kernel text. */ static int cpus_stuck_in_kernel;@@ -108,7 +108,7 @@ static int boot_secondary(unsigned int cpu, struct task_struct *idle)return -EOPNOTSUPP ; } -int __cpu_up (unsigned int cpu,struct task_struct *idle) +int arch_cpuhp_kick_ap_alive (unsigned int cpu,struct task_struct *idle) {int ret;
arch/arm64/kernel/smp.c void arch_cpuhp_cleanup_kick_cpu(unsigned int cpu, bool is_alive) · line 150
Quoted 5 lines
arch/arm64/kernel/smp.c +2
-0@@ -146,6 +146,8 @@ void arch_cpuhp_cleanup_kick_cpu(unsigned int cpu, bool is_alive) cpus_stuck_in_kernel++;break ;case CPU_KILL_ME : +if (cpumask_test_cpu (cpu, &secondary_data.cpu_died_early_mask)) +set_cpu_present (cpu, false);
Should we set_cpu_present(cpu, false) in CPU_STUCK_IN_KERNEL case, as we do not update to CPU_KILL_ME but CPU_STUCK_IN_KERNEL if CONFIG_HOTPLUG_CPU is not enabled.
Quoted 13 lines
arch/arm64/kernel/smp.c +1
-2if (!op_cpu_kill (cpu)) {pr_crit ("CPU%u: died during early boot\n" , cpu);break ;@@ -406,8 +408,7 @@ void __noreturn cpu_die_early(void)pr_crit ("CPU%d: will not boot\n" , cpu); -/* Mark this CPU absent */ -set_cpu_present (cpu,0 ); +cpumask_set_cpu (cpu, &secondary_data.cpu_died_early_mask);if (IS_ENABLED (CONFIG_HOTPLUG_CPU )) {update_cpu_boot_status (CPU_KILL_ME );
Will Deacon
maintainerRe: [PATCH 11/19] arm64: smp: Use generic HOTPLUG_SPLIT_STARTUP machinery for CPU onlining
Quoted 66 lines · Jinjie Ruan
Quoted 63 lines · Will Deacon
In preparation for enabling parallel bringup of secondary CPUs on arm64, take the baby step of moving from
HOTPLUG_CORE_SYNC_FULLtoHOTPLUG_SPLIT_STARTUP.Rework the
cpu_die_early()path to use a private cpumask, otherwise clearing the incoming CPU from the present mask in the 'kick' stage will prevent the hotplug stage machine from progressing andarch_cpuhp_cleanup_kick_cpu()will not be called.Signed-off-by Will Deacon <[email protected]>--- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/smp.h | 1 + arch/arm64/kernel/smp.c | 9 +++++---- 3 files changed, 7 insertions(+), 5 deletions(-)arch/arm64/Kconfig +1
-1@@ -230,7 +230,7 @@ config ARM64 select HAVE_SYSCALL_TRACEPOINTS select HAVE_KPROBES select HAVE_KRETPROBES - select HOTPLUG_CORE_SYNC_FULL + select HOTPLUG_SPLIT_STARTUP select HOTPLUG_SMT if HOTPLUG_CPU select IRQ_DOMAIN select IRQ_FORCED_THREADINGarch/arm64/include/asm/smp.h +1
-0@@ -89,6 +89,7 @@ asmlinkage void secondary_start_kernel(void);struct secondary_data {struct task_struct *task;long status; +cpumask_t cpu_died_early_mask; };extern struct secondary_data secondary_data;arch/arm64/kernel/smp.c +2
-2@@ -62,7 +62,7 @@* so we need some other way of telling a new secondary core * where to place its SVC stack */ -struct secondary_data secondary_data; +struct secondary_data secondary_data = {};/* Number of CPUs which aren't online, but looping in kernel text. */ static int cpus_stuck_in_kernel;@@ -108,7 +108,7 @@ static int boot_secondary(unsigned int cpu, struct task_struct *idle)return -EOPNOTSUPP ; } -int __cpu_up (unsigned int cpu,struct task_struct *idle) +int arch_cpuhp_kick_ap_alive (unsigned int cpu,struct task_struct *idle) {int ret;
Quoted 10 lines
arch/arm64/kernel/smp.c void arch_cpuhp_cleanup_kick_cpu(unsigned int cpu, bool is_alive) · line 150
Quoted 6 lines
arch/arm64/kernel/smp.c +2
-0@@ -146,6 +146,8 @@ void arch_cpuhp_cleanup_kick_cpu(unsigned int cpu, bool is_alive) cpus_stuck_in_kernel++;break ;case CPU_KILL_ME : +if (cpumask_test_cpu (cpu, &secondary_data.cpu_died_early_mask)) +set_cpu_present (cpu, false);Should we
set_cpu_present(cpu, false)inCPU_STUCK_IN_KERNELcase, as we do not update toCPU_KILL_MEbutCPU_STUCK_IN_KERNELifCONFIG_HOTPLUG_CPUis not enabled
That's where we get to at the end of the series, but I can hoist this check on 'secondary_data.cpu_died_early_mask' above the switch here to keep that behaviour for bisection.
Will
Will Deacon
maintainerPATCH 12/19 arm64: cpu_ops: Make 'cpu_operations' pointer global instead of per-cpu
In reply to Will Deacon'cpu_ops' is an NR_CPUS-length array of 'cpu_operations' pointers, which theoretically allows for different CPUs to have different bringup and hotplug backends.
In reality, this complexity exists only to deal with the case where CPU0 is not hotpluggable, so replace the array with a single, global pointer and record separately whether or not they apply to the boot CPU. Update the logic in init_cpu_ops() to enforce that only a single set of 'cpu_ops' is required.
--- arch/arm64/kernel/cpu_ops.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)
arch/arm64/kernel/cpu_ops.c +20 -9
@@ -20,7 +20,8 @@ extern const struct cpu_operations acpi_parking_protocol_ops;#endif extern const struct cpu_operations cpu_psci_ops; -static const struct cpu_operations *cpu_ops[NR_CPUS ]__ro_after_init ; +static const struct cpu_operations *cpu_ops__ro_after_init ; +static bool boot_cpu_has_enable_method__ro_after_init ;static const struct cpu_operations *const dt_supported_cpu_ops[]__initconst = { &smp_spin_table_ops,
@@ -40,6 +41,9 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) {const struct cpu_operations *const *ops; +if (!name) +return NULL ; + ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops;while (*ops) {
@@ -49,6 +53,7 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) ops++; } +pr_warn ("Unsupported enable-method: %s\n" , name);return NULL ; }
@@ -94,25 +99,31 @@ static const char *__init cpu_read_enable_method(int cpu)return enable_method; }/* -* Read a cpu's enable method and record it in cpu_ops. +* Read a cpu's enable method and update/check cpu_ops. */ int __init init_cpu_ops (int cpu) {const char *enable_method =cpu_read_enable_method (cpu); +const struct cpu_operations *ops =cpu_get_ops (enable_method); -if (!enable_method) +if (!ops)return -ENODEV ; - cpu_ops[cpu] =cpu_get_ops (enable_method); -if (!cpu_ops[cpu]) { -pr_warn ("Unsupported enable-method: %s\n" , enable_method); -return -EOPNOTSUPP ; - } +if (!cpu_ops) + cpu_ops = ops; +else if (cpu_ops != ops) +return -EBUSY ; + +if (cpu ==0 ) + boot_cpu_has_enable_method = true;return 0 ; }const struct cpu_operations *get_cpu_ops (int cpu) { -return cpu_ops[cpu]; +if (cpu || boot_cpu_has_enable_method) +return cpu_ops; + +return NULL ; }
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 12/19] arm64: cpu_ops: Make 'cpu_operations' pointer global instead of per-cpu
Quoted 62 lines · Will Deacon
'
cpu_ops' is an NR_CPUS-length array of 'cpu_operations' pointers, which theoretically allows for different CPUs to have different bringup and hotplug backends.In reality, this complexity exists only to deal with the case where CPU0 is not hotpluggable, so replace the array with a single, global pointer and record separately whether or not they apply to the boot CPU. Update the logic in
init_cpu_ops()to enforce that only a single set of 'cpu_ops' is required.Signed-off-by Will Deacon <[email protected]>--- arch/arm64/kernel/cpu_ops.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)arch/arm64/kernel/cpu_ops.c +9
-3@@ -20,7 +20,8 @@ extern const struct cpu_operations acpi_parking_protocol_ops;#endif extern const struct cpu_operations cpu_psci_ops; -static const struct cpu_operations *cpu_ops[NR_CPUS ]__ro_after_init ; +static const struct cpu_operations *cpu_ops__ro_after_init ; +static bool boot_cpu_has_enable_method__ro_after_init ;static const struct cpu_operations *const dt_supported_cpu_ops[]__initconst = { &smp_spin_table_ops,@@ -40,6 +41,9 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) {const struct cpu_operations *const *ops; +if (!name) +return NULL ; + ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops;while (*ops) {@@ -49,6 +53,7 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) ops++; } +pr_warn ("Unsupported enable-method: %s\n" , name);return NULL ; }@@ -94,25 +99,31 @@ static const char *__init cpu_read_enable_method(int cpu)return enable_method; }/* -* Read a cpu's enable method and record it in cpu_ops. +* Read a cpu's enable method and update/check cpu_ops. */ int __init init_cpu_ops (int cpu) {const char *enable_method =cpu_read_enable_method (cpu); +const struct cpu_operations *ops =cpu_get_ops (enable_method); -if (!enable_method) +if (!ops)return -ENODEV ;
arch/arm64/kernel/cpu_ops.c static const char *__init cpu_read_enable_method(int cpu) · line 115
Quoted 10 lines
arch/arm64/kernel/cpu_ops.c +4
-5- cpu_ops[cpu] =cpu_get_ops (enable_method); -if (!cpu_ops[cpu]) { -pr_warn ("Unsupported enable-method: %s\n" , enable_method); -return -EOPNOTSUPP ; - } +if (!cpu_ops) + cpu_ops = ops; +else if (cpu_ops != ops) +return -EBUSY ;
Should we return the original error code of init_cpu_ops() in smp_cpu_setup()?
487 static int __init smp_cpu_setup(int cpu)
488 {
489 >-------const struct cpu_operations *ops;
490
491 >-------if (init_cpu_ops(cpu))
492 >------->-------return -ENODEV;
493
494 >-------ops = get_cpu_ops(cpu);
495 >-------if (ops->cpu_init(cpu))
496 >------->-------return -ENODEV;
Otherwise LGTM
Quoted 16 lines
arch/arm64/kernel/cpu_ops.c +7
-1+ +if (cpu ==0 ) + boot_cpu_has_enable_method = true;return 0 ; }const struct cpu_operations *get_cpu_ops (int cpu) { -return cpu_ops[cpu]; +if (cpu || boot_cpu_has_enable_method) +return cpu_ops; + +return NULL ;}
Will Deacon
maintainerRe: [PATCH 12/19] arm64: cpu_ops: Make 'cpu_operations' pointer global instead of per-cpu
Quoted 49 lines · Jinjie Ruan
Quoted 48 lines · Will Deacon
arch/arm64/kernel/cpu_ops.c +9
-3@@ -20,7 +20,8 @@ extern const struct cpu_operations acpi_parking_protocol_ops;#endif extern const struct cpu_operations cpu_psci_ops; -static const struct cpu_operations *cpu_ops[NR_CPUS ]__ro_after_init ; +static const struct cpu_operations *cpu_ops__ro_after_init ; +static bool boot_cpu_has_enable_method__ro_after_init ;static const struct cpu_operations *const dt_supported_cpu_ops[]__initconst = { &smp_spin_table_ops,@@ -40,6 +41,9 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) {const struct cpu_operations *const *ops; +if (!name) +return NULL ; + ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops;while (*ops) {@@ -49,6 +53,7 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) ops++; } +pr_warn ("Unsupported enable-method: %s\n" , name);return NULL ; }@@ -94,25 +99,31 @@ static const char *__init cpu_read_enable_method(int cpu)return enable_method; }/* -* Read a cpu's enable method and record it in cpu_ops. +* Read a cpu's enable method and update/check cpu_ops. */ int __init init_cpu_ops (int cpu) {const char *enable_method =cpu_read_enable_method (cpu); +const struct cpu_operations *ops =cpu_get_ops (enable_method); -if (!enable_method) +if (!ops)return -ENODEV ;
Quoted 12 lines
arch/arm64/kernel/cpu_ops.c static const char *__init cpu_read_enable_method(int cpu) · line 115
Quoted 9 lines
arch/arm64/kernel/cpu_ops.c +4
-5- cpu_ops[cpu] =cpu_get_ops (enable_method); -if (!cpu_ops[cpu]) { -pr_warn ("Unsupported enable-method: %s\n" , enable_method); -return -EOPNOTSUPP ; - } +if (!cpu_ops) + cpu_ops = ops; +else if (cpu_ops != ops) +return -EBUSY ;Should we return the original error code of
init_cpu_ops()insmp_cpu_setup()?
I don't think it matters (the only caller of smp_cpu_setup() just cares about 0 vs !0) and I don't see a reason to change it as part of this series.
Quoted 2 lines
Otherwise LGTM
Reviewed-by Jinjie Ruan <[email protected]>
Thanks,
Will
Re: [PATCH 12/19] arm64: cpu_ops: Make 'cpu_operations' pointer global instead of per-cpu
Quoted 63 lines · Will Deacon
Quoted 49 lines · Jinjie Ruan
Quoted 48 lines · Will Deacon
arch/arm64/kernel/cpu_ops.c +9
-3@@ -20,7 +20,8 @@ extern const struct cpu_operations acpi_parking_protocol_ops;#endif extern const struct cpu_operations cpu_psci_ops; -static const struct cpu_operations *cpu_ops[NR_CPUS ]__ro_after_init ; +static const struct cpu_operations *cpu_ops__ro_after_init ; +static bool boot_cpu_has_enable_method__ro_after_init ;static const struct cpu_operations *const dt_supported_cpu_ops[]__initconst = { &smp_spin_table_ops,@@ -40,6 +41,9 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) {const struct cpu_operations *const *ops; +if (!name) +return NULL ; + ops = acpi_disabled ? dt_supported_cpu_ops : acpi_supported_cpu_ops;while (*ops) {@@ -49,6 +53,7 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) ops++; } +pr_warn ("Unsupported enable-method: %s\n" , name);return NULL ; }@@ -94,25 +99,31 @@ static const char *__init cpu_read_enable_method(int cpu)return enable_method; }/* -* Read a cpu's enable method and record it in cpu_ops. +* Read a cpu's enable method and update/check cpu_ops. */ int __init init_cpu_ops (int cpu) {const char *enable_method =cpu_read_enable_method (cpu); +const struct cpu_operations *ops =cpu_get_ops (enable_method); -if (!enable_method) +if (!ops)return -ENODEV ;Quoted 12 lines
arch/arm64/kernel/cpu_ops.c static const char *__init cpu_read_enable_method(int cpu) · line 115
Quoted 9 lines
arch/arm64/kernel/cpu_ops.c +4
-5- cpu_ops[cpu] =cpu_get_ops (enable_method); -if (!cpu_ops[cpu]) { -pr_warn ("Unsupported enable-method: %s\n" , enable_method); -return -EOPNOTSUPP ; - } +if (!cpu_ops) + cpu_ops = ops; +else if (cpu_ops != ops) +return -EBUSY ;Should we return the original error code of
init_cpu_ops()insmp_cpu_setup()?
Quoted 3 lines
I don't think it matters (the only caller of
smp_cpu_setup()just cares about 0 vs !0) and I don't see a reason to change it as part of this series.
That's indeed the case — currently there's only one caller, and it doesn't care what the return value is.
Quoted 7 lines
Quoted 3 lines
Otherwise LGTM
Reviewed-by Jinjie Ruan <[email protected]>Thanks,
Will
Will Deacon
maintainerPATCH 13/19 arm64: cpu_ops: Introduce get_secondary_cpu_ops()
In reply to Will DeaconIntroduce get_secondary_cpu_ops() to retrieve a pointer to the 'cpu_operations' structure for the non-boot CPUs and use it instead of get_cpu_ops() where we are dealing with secondary CPUs.
This is a pre-requisite for enabling parallel CPU bring-up.
--- arch/arm64/include/asm/cpu_ops.h | 1 + arch/arm64/kernel/cpu_ops.c | 5 +++++ arch/arm64/kernel/smp.c | 19 +++++++------------ 3 files changed, 13 insertions(+), 12 deletions(-)
arch/arm64/include/asm/cpu_ops.h +1 -0
@@ -48,6 +48,7 @@ struct cpu_operations {int __init init_cpu_ops (int cpu);extern const struct cpu_operations *get_cpu_ops (int cpu); +extern const struct cpu_operations *get_secondary_cpu_ops (void );static inline void __init init_bootcpu_ops (void ) {
arch/arm64/kernel/cpu_ops.c +5 -0
@@ -127,3 +127,8 @@ const struct cpu_operations *get_cpu_ops(int cpu)return NULL ; } + +const struct cpu_operations *get_secondary_cpu_ops (void ) +{ +return cpu_ops; +}
arch/arm64/kernel/smp.c +7 -12
@@ -100,7 +100,7 @@ static inline int op_cpu_kill(unsigned int cpu)*/ static int boot_secondary (unsigned int cpu,struct task_struct *idle) { -const struct cpu_operations *ops =get_cpu_ops (cpu); +const struct cpu_operations *ops =get_secondary_cpu_ops ();if (ops->cpu_boot)return ops->cpu_boot (cpu);
@@ -220,7 +220,7 @@ asmlinkage notrace void secondary_start_kernel(void)*/ check_local_cpu_capabilities (); - ops =get_cpu_ops (cpu); + ops =get_secondary_cpu_ops ();if (ops->cpu_postboot) ops->cpu_postboot ();
@@ -327,7 +327,7 @@ int __cpu_disable(void)static int op_cpu_kill (unsigned int cpu) { -const struct cpu_operations *ops =get_cpu_ops (cpu); +const struct cpu_operations *ops =get_secondary_cpu_ops ();/* * If we have no means of synchronising with the dying CPU, then assume
@@ -368,7 +368,7 @@ void arch_cpuhp_cleanup_dead_cpu(unsigned int cpu)void __noreturn cpu_die (void ) {unsigned int cpu =smp_processor_id (); -const struct cpu_operations *ops =get_cpu_ops (cpu); +const struct cpu_operations *ops =get_secondary_cpu_ops ();idle_task_exit ();
@@ -492,7 +492,7 @@ static int __init smp_cpu_setup(int cpu)if (init_cpu_ops (cpu))return -ENODEV ; - ops =get_cpu_ops (cpu); + ops =get_secondary_cpu_ops ();if (ops->cpu_init (cpu))return -ENODEV ;
@@ -776,7 +776,7 @@ void __init smp_init_cpus(void)void __init smp_prepare_cpus (unsigned int max_cpus) { -const struct cpu_operations *ops; +const struct cpu_operations *ops =get_secondary_cpu_ops ();unsigned int cpu;int err;
@@ -802,10 +802,6 @@ void __init smp_prepare_cpus(unsigned int max_cpus)if (cpu ==0 )continue ; - ops =get_cpu_ops (cpu); -if (!ops) -continue ; - err = ops->cpu_prepare (cpu);if (err)continue ;
@@ -1341,8 +1337,7 @@ bool smp_crash_stop_failed(void)static bool have_cpu_die (void ) {#ifdef CONFIG_HOTPLUG_CPU -int any_cpu =raw_smp_processor_id (); -const struct cpu_operations *ops =get_cpu_ops (any_cpu); +const struct cpu_operations *ops =get_secondary_cpu_ops ();if (ops && ops->cpu_die)return true;
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 13/19] arm64: cpu_ops: Introduce get_secondary_cpu_ops()
Quoted 97 lines · Will Deacon
Introduce
get_secondary_cpu_ops()to retrieve a pointer to the 'cpu_operations' structure for the non-boot CPUs and use it instead ofget_cpu_ops()where we are dealing with secondary CPUs.This is a pre-requisite for enabling parallel CPU bring-up.
Signed-off-by Will Deacon <[email protected]>--- arch/arm64/include/asm/cpu_ops.h | 1 + arch/arm64/kernel/cpu_ops.c | 5 +++++ arch/arm64/kernel/smp.c | 19 +++++++------------ 3 files changed, 13 insertions(+), 12 deletions(-)arch/arm64/include/asm/cpu_ops.h +1
-0@@ -48,6 +48,7 @@ struct cpu_operations {int __init init_cpu_ops (int cpu);extern const struct cpu_operations *get_cpu_ops (int cpu); +extern const struct cpu_operations *get_secondary_cpu_ops (void );static inline void __init init_bootcpu_ops (void ) {arch/arm64/kernel/cpu_ops.c +5
-0@@ -127,3 +127,8 @@ const struct cpu_operations *get_cpu_ops(int cpu)return NULL ; } + +const struct cpu_operations *get_secondary_cpu_ops (void ) +{ +return cpu_ops; +}arch/arm64/kernel/smp.c +6
-6@@ -100,7 +100,7 @@ static inline int op_cpu_kill(unsigned int cpu)*/ static int boot_secondary (unsigned int cpu,struct task_struct *idle) { -const struct cpu_operations *ops =get_cpu_ops (cpu); +const struct cpu_operations *ops =get_secondary_cpu_ops ();if (ops->cpu_boot)return ops->cpu_boot (cpu);@@ -220,7 +220,7 @@ asmlinkage notrace void secondary_start_kernel(void)*/ check_local_cpu_capabilities (); - ops =get_cpu_ops (cpu); + ops =get_secondary_cpu_ops ();if (ops->cpu_postboot) ops->cpu_postboot ();@@ -327,7 +327,7 @@ int __cpu_disable(void)static int op_cpu_kill (unsigned int cpu) { -const struct cpu_operations *ops =get_cpu_ops (cpu); +const struct cpu_operations *ops =get_secondary_cpu_ops ();/* * If we have no means of synchronising with the dying CPU, then assume @@ -368,7 +368,7 @@ void arch_cpuhp_cleanup_dead_cpu(unsigned int cpu)void __noreturn cpu_die (void ) {unsigned int cpu =smp_processor_id (); -const struct cpu_operations *ops =get_cpu_ops (cpu); +const struct cpu_operations *ops =get_secondary_cpu_ops ();idle_task_exit ();@@ -492,7 +492,7 @@ static int __init smp_cpu_setup(int cpu)if (init_cpu_ops (cpu))return -ENODEV ; - ops =get_cpu_ops (cpu); + ops =get_secondary_cpu_ops ();if (ops->cpu_init (cpu))return -ENODEV ;@@ -776,7 +776,7 @@ void __init smp_init_cpus(void)void __init smp_prepare_cpus (unsigned int max_cpus) { -const struct cpu_operations *ops; +const struct cpu_operations *ops =get_secondary_cpu_ops ();unsigned int cpu;int err;
arch/arm64/kernel/smp.c void __init smp_prepare_cpus(unsigned int max_cpus) · line 808
Quoted 7 lines
arch/arm64/kernel/smp.c +0
-4@@ -802,10 +802,6 @@ void __init smp_prepare_cpus(unsigned int max_cpus)if (cpu ==0 )continue ; - ops =get_cpu_ops (cpu); -if (!ops) -continue ; -
Quoted 13 lines
arch/arm64/kernel/smp.c +1
-2err = ops->cpu_prepare (cpu);if (err)continue ;@@ -1341,8 +1337,7 @@ bool smp_crash_stop_failed(void)static bool have_cpu_die (void ) {#ifdef CONFIG_HOTPLUG_CPU -int any_cpu =raw_smp_processor_id (); -const struct cpu_operations *ops =get_cpu_ops (any_cpu); +const struct cpu_operations *ops =get_secondary_cpu_ops ();if (ops && ops->cpu_die)return true;
Will Deacon
maintainerPATCH 14/19 firmware/psci: Cache PSCI v0.2+ version number to avoid redundant SMCs
In reply to Will DeaconSecure monitor calls to EL3 aren't necessarily cheap, so cache the PSCI version number in memory to avoid asking firmware the same question over and over again.
--- drivers/firmware/psci/psci.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/firmware/psci/psci.c +6 -1
@@ -155,7 +155,12 @@ static u32 psci_0_1_get_version(void)static u32 psci_0_2_get_version (void ) { -return invoke_psci_fn (PSCI_0_2_FN_PSCI_VERSION ,0 ,0 ,0 ); +static u32 version; + +if (unlikely (!version)) + version =invoke_psci_fn (PSCI_0_2_FN_PSCI_VERSION ,0 ,0 ,0 ); + +return version; }int psci_set_osi_mode (bool enable)
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 14/19] firmware/psci: Cache PSCI v0.2+ version number to avoid redundant SMCs
Quoted 14 lines · Will Deacon
Secure monitor calls to EL3 aren't necessarily cheap, so cache the PSCI version number in memory to avoid asking firmware the same question over and over again.
Signed-off-by Will Deacon <[email protected]>--- drivers/firmware/psci/psci.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/firmware/psci/psci.c +0
-0
drivers/firmware/psci/psci.c static u32 psci_0_1_get_version(void) · line 163
Quoted 10 lines
drivers/firmware/psci/psci.c +6
-1@@ -155,7 +155,12 @@ static u32 psci_0_1_get_version(void)static u32 psci_0_2_get_version (void ) { -return invoke_psci_fn (PSCI_0_2_FN_PSCI_VERSION ,0 ,0 ,0 ); +static u32 version; + +if (unlikely (!version)) + version =invoke_psci_fn (PSCI_0_2_FN_PSCI_VERSION ,0 ,0 ,0 ); + +return version;
Quoted 3 lines
drivers/firmware/psci/psci.c +0
-0}int psci_set_osi_mode (bool enable)
Will Deacon
maintainerPATCH 15/19 firmware/psci: Extend ->cpu_on() callback to take an additional argument
In reply to Will DeaconIn preparation for hooking up the 'context ID' parameter introduced by PSCI v0.2 to the CPU_ON call, extend the ->cpu_on() callback to take an additional argument and modify all callers to pass 0 for now.
--- arch/arm/kernel/psci_smp.c | 4 ++-- arch/arm64/kernel/psci.c | 2 +- drivers/firmware/psci/psci.c | 20 ++++++++++++++------ include/linux/psci.h | 3 ++- 4 files changed, 19 insertions(+), 10 deletions(-)
arch/arm/kernel/psci_smp.c +2 -2
@@ -49,10 +49,10 @@ static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)return psci_ops.cpu_on (cpu_logical_map (cpu), ((phys_addr_t)(&secondary_startup) -XIP_VIRT_ADDR (CONFIG_XIP_PHYS_ADDR ) - +CONFIG_XIP_PHYS_ADDR )); + +CONFIG_XIP_PHYS_ADDR ),0 );#else return psci_ops.cpu_on (cpu_logical_map (cpu), -virt_to_idmap (&secondary_startup)); +virt_to_idmap (&secondary_startup),0 );#endif return -ENODEV ; }
arch/arm64/kernel/psci.c +1 -1
@@ -39,7 +39,7 @@ static int __init cpu_psci_cpu_prepare(unsigned int cpu)static int cpu_psci_cpu_boot (unsigned int cpu) {phys_addr_t pa_secondary_entry =__pa_symbol (secondary_entry); -int err = psci_ops.cpu_on (cpu_logical_map (cpu), pa_secondary_entry); +int err = psci_ops.cpu_on (cpu_logical_map (cpu), pa_secondary_entry,0 );if (err && err != -EPERM )pr_err ("failed to boot CPU%d (%d)\n" , cpu, err);
drivers/firmware/psci/psci.c +14 -6
@@ -219,22 +219,30 @@ static int psci_0_2_cpu_off(u32 state)return __psci_cpu_off (PSCI_0_2_FN_CPU_OFF , state); } -static int __psci_cpu_on (u32 fn,unsigned long cpuid,unsigned long entry_point) +static int __psci_cpu_on (u32 fn,unsigned long cpuid,unsigned long entry_point, +unsigned long context) {int err; - err =invoke_psci_fn (fn, cpuid, entry_point,0 ); + err =invoke_psci_fn (fn, cpuid, entry_point, context);return psci_to_linux_errno (err); } -static int psci_0_1_cpu_on (unsigned long cpuid,unsigned long entry_point) +static int psci_0_1_cpu_on (unsigned long cpuid,unsigned long entry_point, +unsigned long mbz) { -return __psci_cpu_on (psci_0_1_function_ids.cpu_on, cpuid, entry_point); +if (mbz) +return -EINVAL ; + +return __psci_cpu_on (psci_0_1_function_ids.cpu_on, cpuid, entry_point, +0 ); } -static int psci_0_2_cpu_on (unsigned long cpuid,unsigned long entry_point) +static int psci_0_2_cpu_on (unsigned long cpuid,unsigned long entry_point, +unsigned long context) { - return __psci_cpu_on(PSCI_FN_NATIVE(0_2, CPU_ON), cpuid, entry_point); + return __psci_cpu_on(PSCI_FN_NATIVE(0_2, CPU_ON), cpuid, entry_point, + context); }static int __psci_migrate (u32 fn,unsigned long cpuid)
include/linux/psci.h +2 -1
@@ -25,7 +25,8 @@ struct psci_operations {u32 (*get_version)(void);int (*cpu_suspend)(u32 state,unsigned long entry_point);int (*cpu_off)(u32 state); -int (*cpu_on)(unsigned long cpuid,unsigned long entry_point); +int (*cpu_on)(unsigned long cpuid,unsigned long entry_point, +unsigned long context);int (*migrate)(unsigned long cpuid);int (*affinity_info)(unsigned long target_affinity,unsigned long lowest_affinity_level);
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 15/19] firmware/psci: Extend ->cpu_on() callback to take an additional argument
Quoted 97 lines · Will Deacon
In preparation for hooking up the 'context ID' parameter introduced by PSCI v0.2 to the
CPU_ONcall, extend the ->cpu_on() callback to take an additional argument and modify all callers to pass 0 for now.Signed-off-by Will Deacon <[email protected]>--- arch/arm/kernel/psci_smp.c | 4 ++-- arch/arm64/kernel/psci.c | 2 +- drivers/firmware/psci/psci.c | 20 ++++++++++++++------ include/linux/psci.h | 3 ++- 4 files changed, 19 insertions(+), 10 deletions(-)arch/arm/kernel/psci_smp.c +2
-2@@ -49,10 +49,10 @@ static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)return psci_ops.cpu_on (cpu_logical_map (cpu), ((phys_addr_t)(&secondary_startup) -XIP_VIRT_ADDR (CONFIG_XIP_PHYS_ADDR ) - +CONFIG_XIP_PHYS_ADDR )); + +CONFIG_XIP_PHYS_ADDR ),0 );#else return psci_ops.cpu_on (cpu_logical_map (cpu), -virt_to_idmap (&secondary_startup)); +virt_to_idmap (&secondary_startup),0 );#endif return -ENODEV ; }arch/arm64/kernel/psci.c +1
-1@@ -39,7 +39,7 @@ static int __init cpu_psci_cpu_prepare(unsigned int cpu)static int cpu_psci_cpu_boot (unsigned int cpu) {phys_addr_t pa_secondary_entry =__pa_symbol (secondary_entry); -int err = psci_ops.cpu_on (cpu_logical_map (cpu), pa_secondary_entry); +int err = psci_ops.cpu_on (cpu_logical_map (cpu), pa_secondary_entry,0 );if (err && err != -EPERM )pr_err ("failed to boot CPU%d (%d)\n" , cpu, err);drivers/firmware/psci/psci.c +14
-6@@ -219,22 +219,30 @@ static int psci_0_2_cpu_off(u32 state)return __psci_cpu_off (PSCI_0_2_FN_CPU_OFF , state); } -static int __psci_cpu_on (u32 fn,unsigned long cpuid,unsigned long entry_point) +static int __psci_cpu_on (u32 fn,unsigned long cpuid,unsigned long entry_point, +unsigned long context) {int err; - err =invoke_psci_fn (fn, cpuid, entry_point,0 ); + err =invoke_psci_fn (fn, cpuid, entry_point, context);return psci_to_linux_errno (err); } -static int psci_0_1_cpu_on (unsigned long cpuid,unsigned long entry_point) +static int psci_0_1_cpu_on (unsigned long cpuid,unsigned long entry_point, +unsigned long mbz) { -return __psci_cpu_on (psci_0_1_function_ids.cpu_on, cpuid, entry_point); +if (mbz) +return -EINVAL ; + +return __psci_cpu_on (psci_0_1_function_ids.cpu_on, cpuid, entry_point, +0 ); } -static int psci_0_2_cpu_on (unsigned long cpuid,unsigned long entry_point) +static int psci_0_2_cpu_on (unsigned long cpuid,unsigned long entry_point, +unsigned long context) { - return __psci_cpu_on(PSCI_FN_NATIVE(0_2, CPU_ON), cpuid, entry_point); + return __psci_cpu_on(PSCI_FN_NATIVE(0_2, CPU_ON), cpuid, entry_point, + context); }static int __psci_migrate (u32 fn,unsigned long cpuid)include/linux/psci.h +2
-1@@ -25,7 +25,8 @@ struct psci_operations {u32 (*get_version)(void);int (*cpu_suspend)(u32 state,unsigned long entry_point);int (*cpu_off)(u32 state); -int (*cpu_on)(unsigned long cpuid,unsigned long entry_point); +int (*cpu_on)(unsigned long cpuid,unsigned long entry_point, +unsigned long context);int (*migrate)(unsigned long cpuid);int (*affinity_info)(unsigned long target_affinity,unsigned long lowest_affinity_level);
Will Deacon
maintainerPATCH 16/19 arm64: cpu_ops: Expose optional argument to target cpu in ->cpu_boot()
In reply to Will DeaconSome backend implementations of 'struct cpu_ops', notably PSCI v0.2+, allow an optional argument to be passed in register X0 to the target CPU during boot.
Expose this functionality by extending the ->cpu_boot() CPU operation to take an additional argument which is ignored unless the new optional ->cpu_boot_has_arg() callback is present and returns 'true'. For now, we continue to pass zero.
--- arch/arm64/include/asm/cpu_ops.h | 6 +++++- arch/arm64/kernel/acpi_parking_protocol.c | 3 ++- arch/arm64/kernel/psci.c | 11 +++++++++-- arch/arm64/kernel/smp.c | 2 +- arch/arm64/kernel/smp_spin_table.c | 2 +- 5 files changed, 18 insertions(+), 6 deletions(-)
arch/arm64/include/asm/cpu_ops.h +5 -1
@@ -21,6 +21,9 @@* mechanism for doing so, tests whether it is possible to boot * the given CPU. * @cpu_boot: Boots a cpu into the kernel. +* @cpu_boot_has_arg: Optionally determines whether @cpu_boot passes its +* (non-zero) second argument to the booting CPU in +* register x0. * @cpu_postboot: Optionally, perform any post-boot cleanup or necessary * synchronisation. Called from the cpu being booted. * @cpu_can_disable: Determines whether a CPU can be disabled based on
@@ -36,7 +39,8 @@ struct cpu_operations {const char *name;int (*cpu_init)(unsigned int );int (*cpu_prepare)(unsigned int ); -int (*cpu_boot)(unsigned int ); +int (*cpu_boot)(unsigned int ,unsigned long ); +bool (*cpu_boot_has_arg)(void );void (*cpu_postboot)(void );#ifdef CONFIG_HOTPLUG_CPU bool (*cpu_can_disable)(unsigned int cpu);
arch/arm64/kernel/acpi_parking_protocol.c +2 -1
@@ -56,7 +56,8 @@ static int acpi_parking_protocol_cpu_prepare(unsigned int cpu)return 0 ; } -static int acpi_parking_protocol_cpu_boot (unsigned int cpu) +static int acpi_parking_protocol_cpu_boot (unsigned int cpu, +unsigned long ignored) {struct cpu_mailbox_entry *cpu_entry = &cpu_mailbox_entries[cpu];struct parking_protocol_mailbox __iomem *mailbox;
arch/arm64/kernel/psci.c +9 -2
@@ -36,16 +36,22 @@ static int __init cpu_psci_cpu_prepare(unsigned int cpu)return 0 ; } -static int cpu_psci_cpu_boot (unsigned int cpu) +static int cpu_psci_cpu_boot (unsigned int cpu,unsigned long context) {phys_addr_t pa_secondary_entry =__pa_symbol (secondary_entry); -int err = psci_ops.cpu_on (cpu_logical_map (cpu), pa_secondary_entry,0 ); +int err = psci_ops.cpu_on (cpu_logical_map (cpu), pa_secondary_entry, + context);if (err && err != -EPERM )pr_err ("failed to boot CPU%d (%d)\n" , cpu, err);return err; } +static bool cpu_psci_cpu_boot_has_context (void ) +{ +return psci_ops.get_version () >=PSCI_VERSION (0 ,2 ); +} +#ifdef CONFIG_HOTPLUG_CPU static bool cpu_psci_cpu_can_disable (unsigned int cpu) {
@@ -114,6 +120,7 @@ const struct cpu_operations cpu_psci_ops = { .cpu_init = cpu_psci_cpu_init, .cpu_prepare = cpu_psci_cpu_prepare, .cpu_boot = cpu_psci_cpu_boot, + .cpu_boot_has_arg = cpu_psci_cpu_boot_has_context, #ifdef CONFIG_HOTPLUG_CPU .cpu_can_disable = cpu_psci_cpu_can_disable, .cpu_disable = cpu_psci_cpu_disable,
arch/arm64/kernel/smp.c +1 -1
@@ -103,7 +103,7 @@ static int boot_secondary(unsigned int cpu, struct task_struct *idle)const struct cpu_operations *ops =get_secondary_cpu_ops ();if (ops->cpu_boot) -return ops->cpu_boot (cpu); +return ops->cpu_boot (cpu,0 );return -EOPNOTSUPP ; }
arch/arm64/kernel/smp_spin_table.c +1 -1
@@ -104,7 +104,7 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu)return 0 ; } -static int smp_spin_table_cpu_boot (unsigned int cpu) +static int smp_spin_table_cpu_boot (unsigned int cpu,unsigned long ignored) {/* * Update the pen release flag.
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 16/19] arm64: cpu_ops: Expose optional argument to target cpu in ->cpu_boot()
Quoted 9 lines · Will Deacon
Some backend implementations of 'struct
cpu_ops', notably PSCI v0.2+, allow an optional argument to be passed in register X0 to the target CPU during boot.Expose this functionality by extending the ->cpu_boot() CPU operation to take an additional argument which is ignored unless the new optional ->cpu_boot_has_arg() callback is present and returns 'true'. For now, we continue to pass zero.
Quoted 8 lines
Signed-off-by Will Deacon <[email protected]>--- arch/arm64/include/asm/cpu_ops.h | 6 +++++- arch/arm64/kernel/acpi_parking_protocol.c | 3 ++- arch/arm64/kernel/psci.c | 11 +++++++++-- arch/arm64/kernel/smp.c | 2 +- arch/arm64/kernel/smp_spin_table.c | 2 +- 5 files changed, 18 insertions(+), 6 deletions(-)
Quoted 102 lines
arch/arm64/include/asm/cpu_ops.h +5
-1@@ -21,6 +21,9 @@* mechanism for doing so, tests whether it is possible to boot * the given CPU. * @cpu_boot: Boots a cpu into the kernel. +* @cpu_boot_has_arg: Optionally determines whether @cpu_boot passes its +* (non-zero) second argument to the booting CPU in +* register x0. * @cpu_postboot: Optionally, perform any post-boot cleanup or necessary * synchronisation. Called from the cpu being booted. * @cpu_can_disable: Determines whether a CPU can be disabled based on @@ -36,7 +39,8 @@ struct cpu_operations {const char *name;int (*cpu_init)(unsigned int );int (*cpu_prepare)(unsigned int ); -int (*cpu_boot)(unsigned int ); +int (*cpu_boot)(unsigned int ,unsigned long ); +bool (*cpu_boot_has_arg)(void );void (*cpu_postboot)(void );#ifdef CONFIG_HOTPLUG_CPU bool (*cpu_can_disable)(unsigned int cpu);arch/arm64/kernel/acpi_parking_protocol.c +2
-1@@ -56,7 +56,8 @@ static int acpi_parking_protocol_cpu_prepare(unsigned int cpu)return 0 ; } -static int acpi_parking_protocol_cpu_boot (unsigned int cpu) +static int acpi_parking_protocol_cpu_boot (unsigned int cpu, +unsigned long ignored) {struct cpu_mailbox_entry *cpu_entry = &cpu_mailbox_entries[cpu];struct parking_protocol_mailbox __iomem *mailbox;arch/arm64/kernel/psci.c +9
-2@@ -36,16 +36,22 @@ static int __init cpu_psci_cpu_prepare(unsigned int cpu)return 0 ; } -static int cpu_psci_cpu_boot (unsigned int cpu) +static int cpu_psci_cpu_boot (unsigned int cpu,unsigned long context) {phys_addr_t pa_secondary_entry =__pa_symbol (secondary_entry); -int err = psci_ops.cpu_on (cpu_logical_map (cpu), pa_secondary_entry,0 ); +int err = psci_ops.cpu_on (cpu_logical_map (cpu), pa_secondary_entry, + context);if (err && err != -EPERM )pr_err ("failed to boot CPU%d (%d)\n" , cpu, err);return err; } +static bool cpu_psci_cpu_boot_has_context (void ) +{ +return psci_ops.get_version () >=PSCI_VERSION (0 ,2 ); +} +#ifdef CONFIG_HOTPLUG_CPU static bool cpu_psci_cpu_can_disable (unsigned int cpu) {@@ -114,6 +120,7 @@ const struct cpu_operations cpu_psci_ops = { .cpu_init = cpu_psci_cpu_init, .cpu_prepare = cpu_psci_cpu_prepare, .cpu_boot = cpu_psci_cpu_boot, + .cpu_boot_has_arg = cpu_psci_cpu_boot_has_context, #ifdef CONFIG_HOTPLUG_CPU .cpu_can_disable = cpu_psci_cpu_can_disable, .cpu_disable = cpu_psci_cpu_disable,arch/arm64/kernel/smp.c +1
-1@@ -103,7 +103,7 @@ static int boot_secondary(unsigned int cpu, struct task_struct *idle)const struct cpu_operations *ops =get_secondary_cpu_ops ();if (ops->cpu_boot) -return ops->cpu_boot (cpu); +return ops->cpu_boot (cpu,0 );return -EOPNOTSUPP ; }arch/arm64/kernel/smp_spin_table.c +1
-1@@ -104,7 +104,7 @@ static int smp_spin_table_cpu_prepare(unsigned int cpu)return 0 ; } -static int smp_spin_table_cpu_boot (unsigned int cpu) +static int smp_spin_table_cpu_boot (unsigned int cpu,unsigned long ignored) {/* * Update the pen release flag.
Will Deacon
maintainerPATCH 17/19 arm64: smp: Pass secondary CPU boot parameters via firmware if possible
In reply to Will DeaconIn preparation for parallel bringup of secondary CPUs, the global 'secondary_data' structure used for initial paramater passing must be localised.
Pass the idle 'task_struct' pointer for secondary CPUs directly to ->cpu_boot() if the backend supports it.
--- arch/arm64/include/asm/smp.h | 1 + arch/arm64/kernel/head.S | 12 +++++++++++- arch/arm64/kernel/psci.c | 4 ++-- arch/arm64/kernel/smp.c | 25 +++++++++++-------------- 4 files changed, 25 insertions(+), 17 deletions(-)
arch/arm64/include/asm/smp.h +1 -0
@@ -95,6 +95,7 @@ struct secondary_data {extern struct secondary_data secondary_data;extern long __early_cpu_boot_status;extern void secondary_entry (void ); +extern void secondary_entry_with_arg (void );extern void arch_send_call_function_single_ipi (int cpu);extern void arch_send_call_function_ipi_mask (const struct cpumask *mask);
arch/arm64/kernel/head.S +11 -1
@@ -335,6 +335,7 @@ SYM_FUNC_END(init_kernel_el) * cores are held until we're ready for them to initialise. */ SYM_FUNC_START(secondary_holding_pen) + mov x19, xzr mov x0, xzr bl init_kernel_el // w0=cpu_boot_mode mrs x2, mpidr_el1
@@ -353,10 +354,16 @@ SYM_FUNC_END(secondary_holding_pen) * be used where CPUs are brought online dynamically by the kernel. */ SYM_FUNC_START(secondary_entry) + mov x0, xzr + b secondary_entry_with_arg +SYM_FUNC_END(secondary_entry) + +SYM_FUNC_START(secondary_entry_with_arg) + mov x19, x0 mov x0, xzr bl init_kernel_el // w0=cpu_boot_mode b secondary_startup -SYM_FUNC_END(secondary_entry) +SYM_FUNC_END(secondary_entry_with_arg) SYM_FUNC_START_LOCAL(secondary_startup) /*
@@ -391,10 +398,13 @@ SYM_FUNC_START_LOCAL(__secondary_switched) msr vbar_el1, x5 isb + mov x2, x19 + cbnz x2, 1f adr_l x0, secondary_data ldr x2, [x0, #CPU_BOOT_TASK] cbz x2, __secondary_too_slow +1: init_cpu_task x2, x1, x3 #ifdef CONFIG_ARM64_PTR_AUTH
arch/arm64/kernel/psci.c +2 -2
@@ -38,8 +38,8 @@ static int __init cpu_psci_cpu_prepare(unsigned int cpu)static int cpu_psci_cpu_boot (unsigned int cpu,unsigned long context) { -phys_addr_t pa_secondary_entry =__pa_symbol (secondary_entry); -int err = psci_ops.cpu_on (cpu_logical_map (cpu), pa_secondary_entry, +void *entry_va = context ? secondary_entry_with_arg : secondary_entry; +int err = psci_ops.cpu_on (cpu_logical_map (cpu),__pa_symbol (entry_va), context);if (err && err != -EPERM )pr_err ("failed to boot CPU%d (%d)\n" , cpu, err);
arch/arm64/kernel/smp.c +11 -14
@@ -93,34 +93,31 @@ static inline int op_cpu_kill(unsigned int cpu) }#endif -/* * Boot a secondary CPU, and assign it the specified idle task. * This also gives us the initial stack to use for this CPU. */ -static int boot_secondary (unsigned int cpu,struct task_struct *idle) -{ -const struct cpu_operations *ops =get_secondary_cpu_ops (); - -if (ops->cpu_boot) -return ops->cpu_boot (cpu,0 ); - -return -EOPNOTSUPP ; -} -int arch_cpuhp_kick_ap_alive (unsigned int cpu,struct task_struct *idle) { -int ret; +const struct cpu_operations *ops =get_secondary_cpu_ops (); +int ret = -EOPNOTSUPP ; +void *arg =NULL ;/* * We need to tell the secondary core where to find its stack and the * page tables. */ - secondary_data.task = idle; +if (ops->cpu_boot_has_arg && ops->cpu_boot_has_arg ()) + arg = idle; +else + secondary_data.task = idle; +update_cpu_boot_status (CPU_MMU_OFF );/* Now bring the CPU into our world */ - ret =boot_secondary (cpu, idle); +if (ops->cpu_boot) + ret = ops->cpu_boot (cpu, (unsigned long )arg); +if (ret && ret != -EPERM )pr_err ("CPU%u: failed to boot: %d\n" , cpu, ret);return ret;
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 17/19] arm64: smp: Pass secondary CPU boot parameters via firmware if possible
Quoted 6 lines · Will Deacon
In preparation for parallel bringup of secondary CPUs, the global '
secondary_data' structure used for initial paramater passing must be localised.Pass the idle '
task_struct' pointer for secondary CPUs directly to ->cpu_boot() if the backend supports it.
Quoted 130 lines
Signed-off-by Will Deacon <[email protected]>--- arch/arm64/include/asm/smp.h | 1 + arch/arm64/kernel/head.S | 12 +++++++++++- arch/arm64/kernel/psci.c | 4 ++-- arch/arm64/kernel/smp.c | 25 +++++++++++-------------- 4 files changed, 25 insertions(+), 17 deletions(-)arch/arm64/include/asm/smp.h +1
-0@@ -95,6 +95,7 @@ struct secondary_data {extern struct secondary_data secondary_data;extern long __early_cpu_boot_status;extern void secondary_entry (void ); +extern void secondary_entry_with_arg (void );extern void arch_send_call_function_single_ipi (int cpu);extern void arch_send_call_function_ipi_mask (const struct cpumask *mask);arch/arm64/kernel/head.S +11
-1@@ -335,6 +335,7 @@ SYM_FUNC_END(init_kernel_el) * cores are held until we're ready for them to initialise. */ SYM_FUNC_START(secondary_holding_pen) + mov x19, xzr mov x0, xzr bl init_kernel_el // w0=cpu_boot_mode mrs x2, mpidr_el1@@ -353,10 +354,16 @@ SYM_FUNC_END(secondary_holding_pen) * be used where CPUs are brought online dynamically by the kernel. */ SYM_FUNC_START(secondary_entry) + mov x0, xzr + b secondary_entry_with_arg +SYM_FUNC_END(secondary_entry) + +SYM_FUNC_START(secondary_entry_with_arg) + mov x19, x0 mov x0, xzr bl init_kernel_el // w0=cpu_boot_mode b secondary_startup -SYM_FUNC_END(secondary_entry) +SYM_FUNC_END(secondary_entry_with_arg) SYM_FUNC_START_LOCAL(secondary_startup) /*@@ -391,10 +398,13 @@ SYM_FUNC_START_LOCAL(__secondary_switched) msr vbar_el1, x5 isb + mov x2, x19 + cbnz x2, 1f adr_l x0, secondary_data ldr x2, [x0, #CPU_BOOT_TASK] cbz x2, __secondary_too_slow +1: init_cpu_task x2, x1, x3 #ifdef CONFIG_ARM64_PTR_AUTHarch/arm64/kernel/psci.c +2
-2@@ -38,8 +38,8 @@ static int __init cpu_psci_cpu_prepare(unsigned int cpu)static int cpu_psci_cpu_boot (unsigned int cpu,unsigned long context) { -phys_addr_t pa_secondary_entry =__pa_symbol (secondary_entry); -int err = psci_ops.cpu_on (cpu_logical_map (cpu), pa_secondary_entry, +void *entry_va = context ? secondary_entry_with_arg : secondary_entry; +int err = psci_ops.cpu_on (cpu_logical_map (cpu),__pa_symbol (entry_va), context);if (err && err != -EPERM )pr_err ("failed to boot CPU%d (%d)\n" , cpu, err);arch/arm64/kernel/smp.c +11
-14@@ -93,34 +93,31 @@ static inline int op_cpu_kill(unsigned int cpu) }#endif -/* * Boot a secondary CPU, and assign it the specified idle task. * This also gives us the initial stack to use for this CPU. */ -static int boot_secondary (unsigned int cpu,struct task_struct *idle) -{ -const struct cpu_operations *ops =get_secondary_cpu_ops (); - -if (ops->cpu_boot) -return ops->cpu_boot (cpu,0 ); - -return -EOPNOTSUPP ; -} -int arch_cpuhp_kick_ap_alive (unsigned int cpu,struct task_struct *idle) { -int ret; +const struct cpu_operations *ops =get_secondary_cpu_ops (); +int ret = -EOPNOTSUPP ; +void *arg =NULL ;/* * We need to tell the secondary core where to find its stack and the * page tables. */ - secondary_data.task = idle; +if (ops->cpu_boot_has_arg && ops->cpu_boot_has_arg ()) + arg = idle; +else + secondary_data.task = idle; +update_cpu_boot_status (CPU_MMU_OFF );/* Now bring the CPU into our world */ - ret =boot_secondary (cpu, idle); +if (ops->cpu_boot) + ret = ops->cpu_boot (cpu, (unsigned long )arg); +if (ret && ret != -EPERM )pr_err ("CPU%u: failed to boot: %d\n" , cpu, ret);return ret;
Will Deacon
maintainerPATCH 18/19 arm64: smp: Use generic HOTPLUG_PARALLEL machinery for CPU onlining
In reply to Will DeaconMake the move from HOTPLUG_SPLIT_STARTUP to HOTPLUG_PARALLEL and enable parallel CPU bringup on systems with PSCI v0.2 or later.
The fiddly part of all this is the error handling if a CPU fails to come up, as we can no longer rely on a single global 'status' flag to capture the details. Instead, the secondary_data::status field is replaced with a zero-initialised byte array, with each byte representing an error reason, so the total set of failures can be accurately captured by the primary CPU.
--- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/smp.h | 35 +++++++------ arch/arm64/include/asm/topology.h | 2 + arch/arm64/kernel/head.S | 15 +++--- arch/arm64/kernel/smp.c | 81 ++++++++++++++++--------------- arch/arm64/mm/mmu.c | 2 +- 6 files changed, 72 insertions(+), 65 deletions(-)
arch/arm64/Kconfig +1 -1
@@ -230,7 +230,7 @@ config ARM64 select HAVE_SYSCALL_TRACEPOINTS select HAVE_KPROBES select HAVE_KRETPROBES - select HOTPLUG_SPLIT_STARTUP + select HOTPLUG_PARALLEL select HOTPLUG_SMT if HOTPLUG_CPU select IRQ_DOMAIN select IRQ_FORCED_THREADING
arch/arm64/include/asm/smp.h +20 -15
@@ -7,20 +7,15 @@#include <linux/const.h> -/* Values for secondary_data.status */ -#define CPU_STUCK_REASON_SHIFT (8) -#define CPU_BOOT_STATUS_MASK ((UL(1) << CPU_STUCK_REASON_SHIFT) - 1) +/* Offsets for early CPU boot reasons */ +#define EARLY_CPU_STUCK_REASON_52_BIT_VA (0) +#define EARLY_CPU_STUCK_REASON_NO_GRAN (1) +#define EARLY_CPU_STUCK_REASON_MAX (2) -#define CPU_MMU_OFF (-1) -/* The cpu invoked ops->cpu_die, synchronise it with cpu_kill */ -#define CPU_KILL_ME (1) -/* The cpu couldn't die gracefully and is looping in the kernel */ -#define CPU_STUCK_IN_KERNEL (2) +/* Offsets for late (i.e. MMU-enabled) CPU boot reasons */ /* Fatal system error detected by secondary CPU, crash the system */ -#define CPU_PANIC_KERNEL (3) - -#define CPU_STUCK_REASON_52_BIT_VA (UL(1) << CPU_STUCK_REASON_SHIFT) -#define CPU_STUCK_REASON_NO_GRAN (UL(2) << CPU_STUCK_REASON_SHIFT) +#define CPU_PANIC_KERNEL (0) +#define CPU_STATUS_FLAGS_MAX (1)#ifndef __ASSEMBLER__
@@ -81,6 +76,14 @@ static inline void set_smp_ipi_range(int ipi_base, int n)*/ asmlinkage void secondary_start_kernel(void); +union secondary_status { +u64 val; +union { +u8 flags[CPU_STATUS_FLAGS_MAX ]; +u8 early_flags[EARLY_CPU_STUCK_REASON_MAX ]; + }; +}; +/* * Initial data for bringing up a secondary CPU. * @status - Result passed back from the secondary CPU to
@@ -88,7 +91,7 @@ asmlinkage void secondary_start_kernel(void);*/ struct secondary_data {struct task_struct *task; -long status; +union secondary_status status;cpumask_t cpu_died_early_mask; };
@@ -123,9 +126,11 @@ static inline void __noreturn cpu_park_loop(void) } } -static inline void update_cpu_boot_status (int val) +static inline void update_cpu_boot_status (const unsigned int val) { -WRITE_ONCE (secondary_data.status, val); +BUILD_BUG_ON (val >=CPU_STATUS_FLAGS_MAX ); + +WRITE_ONCE (secondary_data.status.flags[val],1 );/* Ensure the visibility of the status update */ dsb (ishst); }
arch/arm64/include/asm/topology.h +2 -0
@@ -41,4 +41,6 @@ void update_freq_counters_refs(void);#include <asm-generic/topology.h> +#define cpu_primary_thread_mask cpu_none_mask +#endif /* _ASM_ARM_TOPOLOGY_H */
arch/arm64/kernel/head.S +6 -9
@@ -393,7 +393,6 @@ SYM_FUNC_START_LOCAL(__secondary_switched) mov x0, x20 bl finalise_el2 - str_l xzr, __early_cpu_boot_status, x3 adr_l x5, vectors msr vbar_el1, x5 isb
@@ -439,15 +438,15 @@ SYM_FUNC_END(set_cpu_boot_mode_flag) * with MMU turned off. * * update_early_cpu_boot_status tmp, status - * - Corrupts tmp1, tmp2 - * - Writes 'status' to __early_cpu_boot_status and makes sure + * - Corrupts tmp1 + * - Writes 1 to the 'status' field of __early_cpu_boot_status and makes sure * it is committed to memory. */ .macro update_early_cpu_boot_status status, tmp1, tmp2 - mov \tmp2, #\status adr_l \tmp1, __early_cpu_boot_status - str \tmp2, [\tmp1] + mov \tmp2, #1 + strb w\tmp2, [\tmp1, #\status] dmb sy dc ivac, \tmp1 // Invalidate potentially stale cache line .endm
@@ -495,8 +494,7 @@ SYM_FUNC_START(__cpu_secondary_check52bitva) b.ge 2f #endif - update_early_cpu_boot_status \ - CPU_STUCK_IN_KERNEL | CPU_STUCK_REASON_52_BIT_VA, x0, x1 + update_early_cpu_boot_status EARLY_CPU_STUCK_REASON_52_BIT_VA, x0, x1 1: wfe wfi b 1b
@@ -507,8 +505,7 @@ SYM_FUNC_END(__cpu_secondary_check52bitva) SYM_FUNC_START_LOCAL(__no_granule_support) /* Indicate that this CPU can't boot and is stuck in the kernel */ - update_early_cpu_boot_status \ - CPU_STUCK_IN_KERNEL | CPU_STUCK_REASON_NO_GRAN, x1, x2 + update_early_cpu_boot_status EARLY_CPU_STUCK_REASON_NO_GRAN, x1, x2 1: wfe wfi
arch/arm64/kernel/smp.c +42 -39
125 lines not on this page · raw message
arch/arm64/mm/mmu.c +1 -1
@@ -61,7 +61,7 @@ static bool rodata_is_rw __ro_after_init = true;* The booting CPU updates the failed status @__early_cpu_boot_status, * with MMU turned off. */ -long __section(".mmuoff.data.write") __early_cpu_boot_status; +long __section(".mmuoff.data.write") __early_cpu_boot_status = 0;static DEFINE_SPINLOCK (swapper_pgdir_lock);static DEFINE_MUTEX (fixmap_lock);
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Re: [PATCH 18/19] arm64: smp: Use generic HOTPLUG_PARALLEL machinery for CPU onlining
Quoted 127 lines · Will Deacon
Make the move from
HOTPLUG_SPLIT_STARTUPtoHOTPLUG_PARALLELand enable parallel CPU bringup on systems with PSCI v0.2 or later.The fiddly part of all this is the error handling if a CPU fails to come up, as we can no longer rely on a single global 'status' flag to capture the details. Instead, the
secondary_data::statusfield is replaced with a zero-initialised byte array, with each byte representing an error reason, so the total set of failures can be accurately captured by the primary CPU.Signed-off-by Will Deacon <[email protected]>--- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/smp.h | 35 +++++++------ arch/arm64/include/asm/topology.h | 2 + arch/arm64/kernel/head.S | 15 +++--- arch/arm64/kernel/smp.c | 81 ++++++++++++++++--------------- arch/arm64/mm/mmu.c | 2 +- 6 files changed, 72 insertions(+), 65 deletions(-)arch/arm64/Kconfig +1
-1@@ -230,7 +230,7 @@ config ARM64 select HAVE_SYSCALL_TRACEPOINTS select HAVE_KPROBES select HAVE_KRETPROBES - select HOTPLUG_SPLIT_STARTUP + select HOTPLUG_PARALLEL select HOTPLUG_SMT if HOTPLUG_CPU select IRQ_DOMAIN select IRQ_FORCED_THREADINGarch/arm64/include/asm/smp.h +20
-15@@ -7,20 +7,15 @@#include <linux/const.h> -/* Values for secondary_data.status */ -#define CPU_STUCK_REASON_SHIFT (8) -#define CPU_BOOT_STATUS_MASK ((UL(1) << CPU_STUCK_REASON_SHIFT) - 1) +/* Offsets for early CPU boot reasons */ +#define EARLY_CPU_STUCK_REASON_52_BIT_VA (0) +#define EARLY_CPU_STUCK_REASON_NO_GRAN (1) +#define EARLY_CPU_STUCK_REASON_MAX (2) -#define CPU_MMU_OFF (-1) -/* The cpu invoked ops->cpu_die, synchronise it with cpu_kill */ -#define CPU_KILL_ME (1) -/* The cpu couldn't die gracefully and is looping in the kernel */ -#define CPU_STUCK_IN_KERNEL (2) +/* Offsets for late (i.e. MMU-enabled) CPU boot reasons */ /* Fatal system error detected by secondary CPU, crash the system */ -#define CPU_PANIC_KERNEL (3) - -#define CPU_STUCK_REASON_52_BIT_VA (UL(1) << CPU_STUCK_REASON_SHIFT) -#define CPU_STUCK_REASON_NO_GRAN (UL(2) << CPU_STUCK_REASON_SHIFT) +#define CPU_PANIC_KERNEL (0) +#define CPU_STATUS_FLAGS_MAX (1)#ifndef __ASSEMBLER__@@ -81,6 +76,14 @@ static inline void set_smp_ipi_range(int ipi_base, int n)*/ asmlinkage void secondary_start_kernel(void); +union secondary_status { +u64 val; +union { +u8 flags[CPU_STATUS_FLAGS_MAX ]; +u8 early_flags[EARLY_CPU_STUCK_REASON_MAX ]; + }; +}; +/* * Initial data for bringing up a secondary CPU. * @status - Result passed back from the secondary CPU to @@ -88,7 +91,7 @@ asmlinkage void secondary_start_kernel(void);*/ struct secondary_data {struct task_struct *task; -long status; +union secondary_status status;cpumask_t cpu_died_early_mask; };@@ -123,9 +126,11 @@ static inline void __noreturn cpu_park_loop(void) } } -static inline void update_cpu_boot_status (int val) +static inline void update_cpu_boot_status (const unsigned int val) { -WRITE_ONCE (secondary_data.status, val); +BUILD_BUG_ON (val >=CPU_STATUS_FLAGS_MAX ); + +WRITE_ONCE (secondary_data.status.flags[val],1 );/* Ensure the visibility of the status update */ dsb (ishst); }arch/arm64/include/asm/topology.h +2
-0@@ -41,4 +41,6 @@ void update_freq_counters_refs(void);#include <asm-generic/topology.h> +#define cpu_primary_thread_mask cpu_none_mask +#endif /* _ASM_ARM_TOPOLOGY_H */ arch/arm64/kernel/head.S +0
-1@@ -393,7 +393,6 @@ SYM_FUNC_START_LOCAL(__secondary_switched) mov x0, x20 bl finalise_el2 - str_l xzr, __early_cpu_boot_status, x3 adr_l x5, vectors msr vbar_el1, x5 isb
arch/arm64/kernel/head.S SYM_FUNC_END(set_cpu_boot_mode_flag) · line 441
Quoted 6 lines
arch/arm64/kernel/head.S +1
-2@@ -439,15 +438,15 @@ SYM_FUNC_END(set_cpu_boot_mode_flag) * with MMU turned off. * * update_early_cpu_boot_status tmp, status - * - Corrupts tmp1, tmp2 - * - Writes 'status' to __early_cpu_boot_status and makes sure + * - Corrupts tmp1
Corrupts tmp1, tmp2 ?
Quoted 97 lines
arch/arm64/kernel/head.S +5
-6+ * - Writes 1 to the 'status' field of __early_cpu_boot_status and makes sure * it is committed to memory. */ .macro update_early_cpu_boot_status status, tmp1, tmp2 - mov \tmp2, #\status adr_l \tmp1, __early_cpu_boot_status - str \tmp2, [\tmp1] + mov \tmp2, #1 + strb w\tmp2, [\tmp1, #\status] dmb sy dc ivac, \tmp1 // Invalidate potentially stale cache line .endm@@ -495,8 +494,7 @@ SYM_FUNC_START(__cpu_secondary_check52bitva) b.ge 2f #endif - update_early_cpu_boot_status \ - CPU_STUCK_IN_KERNEL | CPU_STUCK_REASON_52_BIT_VA, x0, x1 + update_early_cpu_boot_status EARLY_CPU_STUCK_REASON_52_BIT_VA, x0, x1 1: wfe wfi b 1b@@ -507,8 +505,7 @@ SYM_FUNC_END(__cpu_secondary_check52bitva) SYM_FUNC_START_LOCAL(__no_granule_support) /* Indicate that this CPU can't boot and is stuck in the kernel */ - update_early_cpu_boot_status \ - CPU_STUCK_IN_KERNEL | CPU_STUCK_REASON_NO_GRAN, x1, x2 + update_early_cpu_boot_status EARLY_CPU_STUCK_REASON_NO_GRAN, x1, x2 1: wfe wfiarch/arm64/kernel/smp.c +15
-11@@ -64,7 +64,7 @@*/ struct secondary_data secondary_data = {};/* Number of CPUs which aren't online, but looping in kernel text. */ -static int cpus_stuck_in_kernel; +static bool cpus_stuck_in_kernel;static int ipi_irq_base__ro_after_init ;static int nr_ipi__ro_after_init =NR_IPI ;@@ -93,6 +93,18 @@ static inline int op_cpu_kill(unsigned int cpu) }#endif +static bool smp_parallel_bringup; + +bool arch_cpuhp_init_parallel_bringup (void ) +{ +const struct cpu_operations *ops =get_secondary_cpu_ops (); + + smp_parallel_bringup = ops && + ops->cpu_boot_has_arg && + ops->cpu_boot_has_arg (); +return smp_parallel_bringup; +} +/* * Boot a secondary CPU, and assign it the specified idle task. * This also gives us the initial stack to use for this CPU. @@ -107,13 +119,11 @@ int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *idle)* We need to tell the secondary core where to find its stack and the * page tables. */ -if (ops->cpu_boot_has_arg && ops->cpu_boot_has_arg ()) +if (smp_parallel_bringup) arg = idle;else secondary_data.task = idle; -update_cpu_boot_status (CPU_MMU_OFF ); -/* Now bring the CPU into our world */ if (ops->cpu_boot) ret = ops->cpu_boot (cpu, (unsigned long )arg);@@ -125,45 +135,42 @@ int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *idle)void arch_cpuhp_cleanup_kick_cpu (unsigned int cpu,bool is_alive) { -long status; +union secondary_status status;if (is_alive)return ; - secondary_data.task =NULL ; - status =READ_ONCE (secondary_data.status); -if (status ==CPU_MMU_OFF ) - status =READ_ONCE (__early_cpu_boot_status); -/* A CPU has failed to boot. Try to figure out what happened. */ -switch (status &CPU_BOOT_STATUS_MASK ) {
arch/arm64/kernel/smp.c int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *idle) · line 145
Quoted 10 lines
arch/arm64/kernel/smp.c +2
-8-default : -pr_err ("CPU%u: failed in unknown state : 0x%lx\n" , - cpu, status); - cpus_stuck_in_kernel++; -break ; -case CPU_KILL_ME : -if (cpumask_test_cpu (cpu, &secondary_data.cpu_died_early_mask)) -set_cpu_present (cpu, false); +if (smp_parallel_bringup) +pr_warn_once ("Parallel CPU bringup failed; consider passing \"cpuhp.parallel=off\" for a more accurate diagnosis.\n" );
For some production systems, restarting to reproduce the issue may be troublesome.
Quoted 32 lines
arch/arm64/kernel/smp.c +21
-8+else + secondary_data.task =NULL ; + + status.val =READ_ONCE (__early_cpu_boot_status); +if (status.early_flags[EARLY_CPU_STUCK_REASON_52_BIT_VA ]) { +pr_crit_once ("CPU%u detected lack of support for 52-bit VAs\n" , + cpu); + } + +if (status.early_flags[EARLY_CPU_STUCK_REASON_NO_GRAN ]) { +pr_crit_once ("CPU%u detected lack of support for %luK granules\n" , + cpu,PAGE_SIZE /SZ_1K ); + } + + status =READ_ONCE (secondary_data.status); +if (status.flags[CPU_PANIC_KERNEL ]) +panic ("CPU%u detected unsupported configuration\n" , cpu); + +if (cpumask_test_cpu (cpu, &secondary_data.cpu_died_early_mask)) { +set_cpu_present (cpu, false);if (!op_cpu_kill (cpu)) {pr_crit ("CPU%u: died during early boot\n" , cpu); -break ; +return ; } -pr_crit ("CPU%u: may not have shut down cleanly\n" , cpu); - fallthrough; -case CPU_STUCK_IN_KERNEL : -pr_crit ("CPU%u: is stuck in kernel\n" , cpu); -if (status &CPU_STUCK_REASON_52_BIT_VA ) -pr_crit ("CPU%u: does not support 52-bit VAs\n" , cpu); -if (status &CPU_STUCK_REASON_NO_GRAN ) {
arch/arm64/kernel/smp.c int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *idle) · line 172
Quoted 10 lines
arch/arm64/kernel/smp.c +2
-7-pr_crit ("CPU%u: does not support %luK granule\n" , - cpu,PAGE_SIZE /SZ_1K ); - } - cpus_stuck_in_kernel++; -break ; -case CPU_PANIC_KERNEL : -panic ("CPU%u detected unsupported configuration\n" , cpu); } + +pr_crit_once ("CPUs may be stuck in kernel\n" );
Need we print the "cpu"
arch/arm64/kernel/smp.c void __noreturn cpu_die_early(void) · line 415
Quoted 7 lines
arch/arm64/kernel/smp.c +1
-0+ cpus_stuck_in_kernel = true; }static void init_gic_priority_masking (void )@@ -407,12 +414,8 @@ void __noreturn cpu_die_early(void)cpumask_set_cpu (cpu, &secondary_data.cpu_died_early_mask);
It seems unsafe for multiple secondary CPUs to update cpu_died_early_mask concurrently.
Quoted 24 lines
arch/arm64/kernel/smp.c +1
-5-if (IS_ENABLED (CONFIG_HOTPLUG_CPU )) { -update_cpu_boot_status (CPU_KILL_ME ); +if (IS_ENABLED (CONFIG_HOTPLUG_CPU ))__cpu_try_die (cpu); - } - -update_cpu_boot_status (CPU_STUCK_IN_KERNEL );cpu_park_loop (); }arch/arm64/mm/mmu.c +1
-1@@ -61,7 +61,7 @@ static bool rodata_is_rw __ro_after_init = true;* The booting CPU updates the failed status @__early_cpu_boot_status, * with MMU turned off. */ -long __section(".mmuoff.data.write") __early_cpu_boot_status; +long __section(".mmuoff.data.write") __early_cpu_boot_status = 0;static DEFINE_SPINLOCK (swapper_pgdir_lock);static DEFINE_MUTEX (fixmap_lock);
Will Deacon
maintainerRe: [PATCH 18/19] arm64: smp: Use generic HOTPLUG_PARALLEL machinery for CPU onlining
Quoted 9 lines · Jinjie Ruan
Quoted 8 lines · Will Deacon
arch/arm64/kernel/head.S +0
-0@@ -393,7 +393,6 @@ SYM_FUNC_START_LOCAL(__secondary_switched) mov x0, x20 bl finalise_el2
Quoted 13 lines
arch/arm64/kernel/head.S SYM_FUNC_END(set_cpu_boot_mode_flag) · line 441
Quoted 11 lines
arch/arm64/kernel/head.S +1
-3- str_l xzr, __early_cpu_boot_status, x3 adr_l x5, vectors msr vbar_el1, x5 isb@@ -439,15 +438,15 @@ SYM_FUNC_END(set_cpu_boot_mode_flag) * with MMU turned off. * * update_early_cpu_boot_status tmp, status - * - Corrupts tmp1, tmp2 - * - Writes 'status' to __early_cpu_boot_status and makes sure + * - Corrupts tmp1Corrupts tmp1, tmp2 ?
Well spotted, thanks.
Quoted 22 lines
Quoted 22 lines
arch/arm64/kernel/head.S +1
-12@@ -125,45 +135,42 @@ int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *idle) void arch_cpuhp_cleanup_kick_cpu(unsigned int cpu, bool is_alive) { - long status; + union secondary_status status; if (is_alive) return; - secondary_data.task = NULL; - status = READ_ONCE(secondary_data.status); - if (status == CPU_MMU_OFF) - status = READ_ONCE(__early_cpu_boot_status); - /* A CPU has failed to boot. Try to figure out what happened. */ - switch (status & CPU_BOOT_STATUS_MASK) { - default: - pr_err("CPU%u: failed in unknown state : 0x%lx\n", - cpu, status); - cpus_stuck_in_kernel++; - break;
Quoted 8 lines
arch/arm64/kernel/head.S int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *idle) · line 145
Quoted 5 lines
arch/arm64/kernel/head.S +2
-3- case CPU_KILL_ME: - if (cpumask_test_cpu(cpu, &secondary_data.cpu_died_early_mask)) - set_cpu_present(cpu, false); + if (smp_parallel_bringup) + pr_warn_once("Parallel CPU bringup failed; consider passing \"cpuhp.parallel=off\" for a more accurate diagnosis.\n");For some production systems, restarting to reproduce the issue may be troublesome.
These errors _really_ shouldn't happen with production systems. They are caused by critical, deterministic errors such as the secondary CPU not supporting the system page size. If you can't reboot in that situation, then you have no system!
The parallel bringup code will detect the issue, it just won't be able to tell you which CPU caused which issue.
Quoted 18 lines
Quoted 18 lines
arch/arm64/kernel/head.S +3
-12+ if (cpumask_test_cpu(cpu, &secondary_data.cpu_died_early_mask)) { + set_cpu_present(cpu, false); if (!op_cpu_kill(cpu)) { pr_crit("CPU%u: died during early boot\n", cpu); - break; + return; } - pr_crit("CPU%u: may not have shut down cleanly\n", cpu); - fallthrough; - case CPU_STUCK_IN_KERNEL: - pr_crit("CPU%u: is stuck in kernel\n", cpu); - if (status & CPU_STUCK_REASON_52_BIT_VA) - pr_crit("CPU%u: does not support 52-bit VAs\n", cpu); - if (status & CPU_STUCK_REASON_NO_GRAN) { - pr_crit("CPU%u: does not support %luK granule\n", - cpu, PAGE_SIZE / SZ_1K); - } - cpus_stuck_in_kernel++;
Quoted 8 lines
arch/arm64/kernel/head.S int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *idle) · line 154
Quoted 6 lines
arch/arm64/kernel/head.S +2
-3- break; - case CPU_PANIC_KERNEL: - panic("CPU%u detected unsupported configuration\n", cpu); } + + pr_crit_once("CPUs may be stuck in kernel\n");Need we print the "cpu"
You should already get something like:
CPUn: will not boot
so I don't think we need anything extra (as, as above, we don't know exactly which CPUs are stuck).
Quoted 7 lines
arch/arm64/kernel/head.S void __noreturn cpu_die_early(void) · line 415
Quoted 4 lines
arch/arm64/kernel/head.S +0
-0static void init_gic_priority_masking(void)@@ -407,12 +414,8 @@ void __noreturn cpu_die_early(void) cpumask_set_cpu(cpu, &secondary_data.cpu_died_early_mask);It seems unsafe for multiple secondary CPUs to update
cpu_died_early_maskconcurrently.
Why? cpumask_set_cpu() is atomic and each CPU only sets their own bit.
Will
Re: [PATCH 18/19] arm64: smp: Use generic HOTPLUG_PARALLEL machinery for CPU onlining
Quoted 57 lines · Will Deacon
Quoted 9 lines · Jinjie Ruan
Quoted 8 lines · Will Deacon
arch/arm64/kernel/head.S +0
-0@@ -393,7 +393,6 @@ SYM_FUNC_START_LOCAL(__secondary_switched) mov x0, x20 bl finalise_el2Quoted 13 lines
arch/arm64/kernel/head.S SYM_FUNC_END(set_cpu_boot_mode_flag) · line 441
Quoted 11 lines
arch/arm64/kernel/head.S +1
-3- str_l xzr, __early_cpu_boot_status, x3 adr_l x5, vectors msr vbar_el1, x5 isb@@ -439,15 +438,15 @@ SYM_FUNC_END(set_cpu_boot_mode_flag) * with MMU turned off. * * update_early_cpu_boot_status tmp, status - * - Corrupts tmp1, tmp2 - * - Writes 'status' to __early_cpu_boot_status and makes sure + * - Corrupts tmp1Corrupts tmp1, tmp2 ?
Well spotted, thanks.
Quoted 22 lines
Quoted 22 lines
arch/arm64/kernel/head.S +1
-12@@ -125,45 +135,42 @@ int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *idle) void arch_cpuhp_cleanup_kick_cpu(unsigned int cpu, bool is_alive) { - long status; + union secondary_status status; if (is_alive) return; - secondary_data.task = NULL; - status = READ_ONCE(secondary_data.status); - if (status == CPU_MMU_OFF) - status = READ_ONCE(__early_cpu_boot_status); - /* A CPU has failed to boot. Try to figure out what happened. */ - switch (status & CPU_BOOT_STATUS_MASK) { - default: - pr_err("CPU%u: failed in unknown state : 0x%lx\n", - cpu, status); - cpus_stuck_in_kernel++; - break;Quoted 8 lines
arch/arm64/kernel/head.S int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *idle) · line 145
Quoted 5 lines
arch/arm64/kernel/head.S +2
-3- case CPU_KILL_ME: - if (cpumask_test_cpu(cpu, &secondary_data.cpu_died_early_mask)) - set_cpu_present(cpu, false); + if (smp_parallel_bringup) + pr_warn_once("Parallel CPU bringup failed; consider passing \"cpuhp.parallel=off\" for a more accurate diagnosis.\n");For some production systems, restarting to reproduce the issue may be troublesome.
Quoted 4 lines
These errors _really_ shouldn't happen with production systems. They are caused by critical, deterministic errors such as the secondary CPU not supporting the system page size. If you can't reboot in that situation, then you have no system!
That does seem to be the case — this kind of situation is usually more common with firmware issues or during the testing phase before a chip is commercially available, and these boot problems should already have been resolved before production systems.
Quoted 3 lines
The parallel bringup code will detect the issue, it just won't be able to tell you which CPU caused which issue.
Indeed.
Quoted 28 lines
Quoted 19 lines
Quoted 19 lines
arch/arm64/kernel/head.S +3
-12+ if (cpumask_test_cpu(cpu, &secondary_data.cpu_died_early_mask)) { + set_cpu_present(cpu, false); if (!op_cpu_kill(cpu)) { pr_crit("CPU%u: died during early boot\n", cpu); - break; + return; } - pr_crit("CPU%u: may not have shut down cleanly\n", cpu); - fallthrough; - case CPU_STUCK_IN_KERNEL: - pr_crit("CPU%u: is stuck in kernel\n", cpu); - if (status & CPU_STUCK_REASON_52_BIT_VA) - pr_crit("CPU%u: does not support 52-bit VAs\n", cpu); - if (status & CPU_STUCK_REASON_NO_GRAN) { - pr_crit("CPU%u: does not support %luK granule\n", - cpu, PAGE_SIZE / SZ_1K); - } - cpus_stuck_in_kernel++;Quoted 8 lines
arch/arm64/kernel/head.S int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *idle) · line 154
Quoted 6 lines
arch/arm64/kernel/head.S +2
-3- break; - case CPU_PANIC_KERNEL: - panic("CPU%u detected unsupported configuration\n", cpu); } + + pr_crit_once("CPUs may be stuck in kernel\n");Need we print the "cpu"
Quoted 3 lines
You should already get something like:
CPUn: will not boot
"will not boot" is only for "cpu_died_early_mask" case, I meant the "unknown state" case in the original default branch of the switch.
Quoted 7 lines
so I don't think we need anything extra (as, as above, we don't know exactly which CPUs are stuck).
Quoted 3 lines
Quoted 3 lines
arch/arm64/kernel/head.S +0
-0static void init_gic_priority_masking(void)@@ -407,12 +414,8 @@ void __noreturn cpu_die_early(void)
Quoted 6 lines
Quoted 4 lines
arch/arm64/kernel/head.S void __noreturn cpu_die_early(void) · line 415
Quoted 1 line
arch/arm64/kernel/head.S +0
-0cpumask_set_cpu(cpu, &secondary_data.cpu_died_early_mask);It seems unsafe for multiple secondary CPUs to update
cpu_died_early_maskconcurrently.Why?
cpumask_set_cpu()is atomic and each CPU only sets their own bit.
Oh! Sorry, I misread it — the underlying implementation uses atomic LSE or LDXR/STXR instructions.
Quoted 2 lines
Will
Will Deacon
maintainerRe: [PATCH 18/19] arm64: smp: Use generic HOTPLUG_PARALLEL machinery for CPU onlining
Quoted 11 lines · Jinjie Ruan
Quoted 7 lines · Will Deacon
Quoted 3 lines
arch/arm64/kernel/smp.c int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *idle) · line 172
Quoted 1 line
arch/arm64/kernel/smp.c +1
-0+pr_crit_once ("CPUs may be stuck in kernel\n" );Need we print the "cpu"
You should already get something like:
CPUn: will not boot
"will not boot" is only for "
cpu_died_early_mask" case, I meant the "unknown state" case in the original default branch of the switch.
Oh, in that case the core code should now print:
"CPU%u failed to report alive state\n"
Will
Will Deacon
maintainerPATCH 19/19 arm64: smp: Harden parallel CPU bringup against broken PSCI firmware
In reply to Will DeaconSince firmware has occasionally been known to get things wrong, harden our parallel CPU bringup code against a PSCI implementation that passes the CPU_ON argument to an incorrect CPU.
The primary CPU writes the MPIDR of the incoming CPU to the end of its task stack and this is then checked against the MPIDR_EL1 register during early kernel entry. A mismatch is reported via the existing failure reporting mechanism and the CPU is not brought online.
--- arch/arm64/include/asm/smp.h | 4 +++- arch/arm64/kernel/asm-offsets.c | 1 + arch/arm64/kernel/head.S | 39 ++++++++++++++++++++++++++++----- arch/arm64/kernel/smp.c | 11 ++++++++-- 4 files changed, 46 insertions(+), 9 deletions(-)
arch/arm64/include/asm/smp.h +3 -1
@@ -15,7 +15,9 @@/* Offsets for late (i.e. MMU-enabled) CPU boot reasons */ /* Fatal system error detected by secondary CPU, crash the system */ #define CPU_PANIC_KERNEL (0) -#define CPU_STATUS_FLAGS_MAX (1) +/* The PSCI v0.2+ implementation passed the wrong argument */ +#define CPU_BROKEN_PSCI_ARG (1) +#define CPU_STATUS_FLAGS_MAX (2)#ifndef __ASSEMBLER__
arch/arm64/kernel/asm-offsets.c +1 -0
@@ -97,6 +97,7 @@ int main(void)BLANK ();#endif DEFINE (CPU_BOOT_TASK , offsetof(struct secondary_data , task)); + DEFINE(CPU_BOOT_STATUS_FLAGS, offsetof(struct secondary_data, status.flags));BLANK ();DEFINE (FTR_OVR_VAL_OFFSET , offsetof(struct arm64_ftr_override , val));DEFINE (FTR_OVR_MASK_OFFSET , offsetof(struct arm64_ftr_override , mask));
arch/arm64/kernel/head.S +33 -6
@@ -192,11 +192,21 @@ SYM_CODE_END(preserve_boot_args) * its location in the task stack. We reserve the entire pt_regs space * for consistency with user tasks and kthreads. */ - .macro init_cpu_task tsk, tmp1, tmp2 + .macro init_cpu_task tsk, tmp1, tmp2, check_mpidr= msr sp_el0, \tsk ldr \tmp1, [\tsk, #TSK_STACK] - add sp, \tmp1, #THREAD_SIZE + mov sp, \tmp1 + .ifnb \check_mpidr + mov_q \tmp1, MPIDR_HWID_BITMASK + mrs \tmp2, mpidr_el1 + and \tmp2, \tmp2, \tmp1 + ldr \tmp1, [sp] + sub \tmp1, \tmp1, \tmp2 + cbnz \tmp1, __cpu_secondary_broken_psci_arg + .endif + + add sp, sp, #THREAD_SIZE sub sp, sp, #PT_REGS_SIZE stp xzr, xzr, [sp, #S_STACKFRAME]
@@ -401,11 +411,12 @@ SYM_FUNC_START_LOCAL(__secondary_switched) cbnz x2, 1f adr_l x0, secondary_data ldr x2, [x0, #CPU_BOOT_TASK] - cbz x2, __secondary_too_slow - -1: init_cpu_task x2, x1, x3 - + cbnz x2, 2f + b __secondary_too_slow +1: + init_cpu_task x2, x1, x3, check_mpidr=1 +2: #ifdef CONFIG_ARM64_PTR_AUTH ptrauth_keys_init_cpu x2, x3, x4, x5 #endif
@@ -451,6 +462,22 @@ SYM_FUNC_END(set_cpu_boot_mode_flag) dc ivac, \tmp1 // Invalidate potentially stale cache line .endm + .macro update_cpu_boot_status status, tmp1, tmp2 + adr_l \tmp1, secondary_data + add \tmp1, \tmp1, #CPU_BOOT_STATUS_FLAGS + \status + mov \tmp2, #1 + strb w\tmp2, [\tmp1] + .endm + +SYM_FUNC_START_LOCAL(__cpu_secondary_broken_psci_arg) + update_cpu_boot_status CPU_BROKEN_PSCI_ARG, x0, x1 +1: + wfe + wfi + b 1b +SYM_FUNC_END(__cpu_secondary_broken_psci_arg) + + /* * Enable the MMU. *
arch/arm64/kernel/smp.c +9 -2
@@ -119,10 +119,12 @@ int arch_cpuhp_kick_ap_alive(unsigned int cpu, struct task_struct *idle)* We need to tell the secondary core where to find its stack and the * page tables. */ -if (smp_parallel_bringup) +if (smp_parallel_bringup) { + *((u64 *)idle->stack) =cpu_logical_map (cpu); arg = idle; -else + }else { secondary_data.task = idle; + }/* Now bring the CPU into our world */ if (ops->cpu_boot)
@@ -158,6 +160,11 @@ void arch_cpuhp_cleanup_kick_cpu(unsigned int cpu, bool is_alive) } status =READ_ONCE (secondary_data.status); +if (status.flags[CPU_BROKEN_PSCI_ARG ]) { +pr_crit_once ("CPU%u detected broken PSCI v0.2+ CPU_ON argument passing\n" , + cpu); + } +if (status.flags[CPU_PANIC_KERNEL ])panic ("CPU%u detected unsupported configuration\n" , cpu);
Signature · 1 line
2.55.0.979.g7e5102b832-goog
Will Deacon
maintainerRe: [PATCH 19/19] arm64: smp: Harden parallel CPU bringup against broken PSCI firmware
Quoted 17 lines · Will Deacon
Since firmware has occasionally been known to get things wrong, harden our parallel CPU bringup code against a PSCI implementation that passes the
CPU_ONargument to an incorrect CPU.The primary CPU writes the MPIDR of the incoming CPU to the end of its task stack and this is then checked against the
MPIDR_EL1register during early kernel entry. A mismatch is reported via the existing failure reporting mechanism and the CPU is not brought online.Suggested-by David Woodhouse <[email protected]>Signed-off-by Will Deacon <[email protected]>--- arch/arm64/include/asm/smp.h | 4 +++- arch/arm64/kernel/asm-offsets.c | 1 + arch/arm64/kernel/head.S | 39 ++++++++++++++++++++++++++++----- arch/arm64/kernel/smp.c | 11 ++++++++-- 4 files changed, 46 insertions(+), 9 deletions(-)
[...]
Quoted 29 lines
arch/arm64/kernel/head.S +12
-2@@ -192,11 +192,21 @@ SYM_CODE_END(preserve_boot_args) * its location in the task stack. We reserve the entire pt_regs space * for consistency with user tasks and kthreads. */ - .macro init_cpu_task tsk, tmp1, tmp2 + .macro init_cpu_task tsk, tmp1, tmp2, check_mpidr= msr sp_el0, \tsk ldr \tmp1, [\tsk, #TSK_STACK] - add sp, \tmp1, #THREAD_SIZE + mov sp, \tmp1 + .ifnb \check_mpidr + mov_q \tmp1, MPIDR_HWID_BITMASK + mrs \tmp2, mpidr_el1 + and \tmp2, \tmp2, \tmp1 + ldr \tmp1, [sp] + sub \tmp1, \tmp1, \tmp2 + cbnz \tmp1, __cpu_secondary_broken_psci_arg + .endif + + add sp, sp, #THREAD_SIZE sub sp, sp, #PT_REGS_SIZE stp xzr, xzr, [sp, #S_STACKFRAME]
arch/arm64/kernel/head.S SYM_FUNC_START_LOCAL(__secondary_switched) · line 416
Quoted 10 lines
arch/arm64/kernel/head.S +2
-4@@ -401,11 +411,12 @@ SYM_FUNC_START_LOCAL(__secondary_switched) cbnz x2, 1f adr_l x0, secondary_data ldr x2, [x0, #CPU_BOOT_TASK] - cbz x2, __secondary_too_slow - -1: init_cpu_task x2, x1, x3 - + cbnz x2, 2f + b __secondary_too_slow
Bah, Sashiko points out that this is too late, as x2 has already been dereferenced on the non-PSCI 0.2+ path. I'll rework this for v2.
Will
Quoted 11 lines · Will Deacon
Hi folks,
This series implements
CONFIG_HOTPLUG_PARALLELfor arm64 by utilising the argument toCPU_ONintroduced in PSCI v0.2. This supersedes the previous series from Jinjie [1], and I'm grateful to him for his support getting this alternative version in shape for posting.I previously spoke at KVM forum about this work last year:
Quoted 9 lines
There are three major benefits realised by these changes:
1. We ditch our home-brew secondary CPU synchronisation in favour of constructs in the core code.
2. They offer a performance advantage on some platforms, where getting a CPU into the kernel can take time. I'm hoping Jinjie can provide numbers in this case.
Tested on Glymur-CRD - 18 core qcom,oryon-2-2 CPU, saw ~6% improvement in smp bringup and ~12% improvement in cpu hotplug kselftest
Quoted 7 lines
3. When used inside a confidential guest, they provide protection against a malicious VMM that puts secondary CPUs into the guest kernel after the onlining operation has timed out.
The first four patches are small changes to the generic code to make it a little more "Arm-shaped" but the rest of the series is largely confined to the arm64 architecture and PSCI driver code.[...]
Will Deacon
maintainerQuoted 22 lines · Pankaj Patil
Quoted 11 lines · Will Deacon
Hi folks,
This series implements
CONFIG_HOTPLUG_PARALLELfor arm64 by utilising the argument toCPU_ONintroduced in PSCI v0.2. This supersedes the previous series from Jinjie [1], and I'm grateful to him for his support getting this alternative version in shape for posting.I previously spoke at KVM forum about this work last year:
Quoted 9 lines
There are three major benefits realised by these changes:
1. We ditch our home-brew secondary CPU synchronisation in favour of constructs in the core code.
2. They offer a performance advantage on some platforms, where getting a CPU into the kernel can take time. I'm hoping Jinjie can provide numbers in this case.
Quoted 4 lines
Tested on Glymur-CRD - 18 core qcom,oryon-2-2 CPU, saw ~6% improvement in smp bringup and ~12% improvement in cpu hotplug kselftest
Tested-by Pankaj Patil <[email protected]>
Nice, thanks for giving this a go, Pankaj! I'll post a v2 soon, so it would be great if you can try that as well.
Cheers,
Will
Quoted 24 lines · Will Deacon
Hi folks,
This series implements
CONFIG_HOTPLUG_PARALLELfor arm64 by utilising the argument toCPU_ONintroduced in PSCI v0.2. This supersedes the previous series from Jinjie [1], and I'm grateful to him for his support getting this alternative version in shape for posting.I previously spoke at KVM forum about this work last year:
https://www.youtube.com/watch?v=Q6kOshnnQuE
There are three major benefits realised by these changes:
1. We ditch our home-brew secondary CPU synchronisation in favour of constructs in the core code.
2. They offer a performance advantage on some platforms, where getting a CPU into the kernel can take time. I'm hoping Jinjie can provide numbers in this case.
3. When used inside a confidential guest, they provide protection against a malicious VMM that puts secondary CPUs into the guest kernel after the onlining operation has timed out.
Quoted 8 lines
The first four patches are small changes to the generic code to make it a little more "Arm-shaped" but the rest of the series is largely confined to the arm64 architecture and PSCI driver code.
The series is based on -rc2, as it otherwise conflicts with Fuad's GMID fix that was recently merged upstream.
All feedback welcome,
Thanks both of you for working on this. It was always on my list to come back and do it for arm64, but that list is long. Also on my list FWIW is parallelising the *later* stages of CPU hotplug¹, not just the early path into start_secondary()/secondary_start_kernel(). IIRC there was more win to be had, but we had to prove that a lot of once- serialized code could safely run concurrently. Ideally without just naïvely adding locking and serializing it all again.
I gave your series a quick spin across a range of EC2 systems, and it gives a ~40% win on systems with 192 cores — at least, as a microbenchmark of the CPU onlining. For these systems, PSCI is fairly fast to bring the CPUs online and it isn't a huge proportion of the *overall* kexec time.
However, your code in serial mode (cpuhp.parallel=0) is up to 40% *slower* than before. A/B testing results, all values in milliseconds:
┌───────────┬──────┬───────────┬────────────────┬─────────────────┐ │ platform │ ncpu │ A 7.3-rc2 │ A' Will-serial │ B Will-parallel │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ a1-metal │ 16 │ 5.7 │ 6.7 (+16.8%) │ 5.2 (−8.6%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ a1-virt │ 16 │ 13.6 │ 15.7 (+15.4%) │ 10.5 (−22.5%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ m6g-metal │ 64 │ 21.7 │ 29.8 (+37.3%) │ 15.5 (−28.8%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ m6g-virt │ 64 │ 27.0 │ 30.9 (+14.5%) │ 23.0 (−14.6%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ c7g-metal │ 64 │ 20.6 │ 26.0 (+26.3%) │ 11.9 (−41.9%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ c7g-virt │ 64 │ 26.8 │ 31.0 (+15.4%) │ 21.7 (−19.1%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ c8g-metal │ 192 │ 81.7 │ 111.2 (+36.0%) │ 53.0 (−35.2%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ c8g-virt │ 192 │ 110.7 │ 120.4 (+8.8%) │ 96.4 (−12.9%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ m9g-metal │ 192 │ 69.0 │ 97.2 (+40.9%) │ 43.9 (−36.3%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ m9g-virt │ 192 │ 128.6 │ 138.9 (+8.0%) │ 112.2 (−12.7%) │ └───────────┴──────┴───────────┴────────────────┴─────────────────┘
We also tried it on another system where the firmware takes about 9ms to bring each CPU up (after CPU_ON returns fairly quickly). Fanning out the CPU_ON calls didn't make any difference either; the CPUs came online, one at a time, about 9ms apart. The parallel onlining saved only about 20ms out of 820ms (→800ms) here.
The real answer for such platforms (at least for kexec) is *not* to do the CPU_OFF/CPU_ON thing at all. Pasha's Caretaker work² still does so for the reclaim at hotplug time in the next kernel; I'm experimenting with eliding that, which should give the biggest improvement on such platforms. So during kexec the APs just spin and wait to be asked to come back, instead of going completely offline.
Once I have that working in its upstreamable Caretaker-compatible form, I'll post the full 3x2 matrix of results for each platform.
¹ https://lore.kernel.org/all/[email protected]/t/#u
² https://lore.kernel.org/all/[email protected]/
Will Deacon
maintainerHi David,
Thanks for replying with so much data! It looks like I sent my v2 out at the same time.
Quoted 7 lines · David Woodhouse
Thanks both of you for working on this. It was always on my list to come back and do it for arm64, but that list is long. Also on my list FWIW is parallelising the *later* stages of CPU hotplug¹, not just the early path into start_secondary()/secondary_start_kernel(). IIRC there was more win to be had, but we had to prove that a lot of once- serialized code could safely run concurrently. Ideally without just naïvely adding locking and serializing it all again.
That gets _really_ hard because you need RCU up and running pretty early.
Quoted 8 lines
I gave your series a quick spin across a range of EC2 systems, and it gives a ~40% win on systems with 192 cores — at least, as a microbenchmark of the CPU onlining. For these systems, PSCI is fairly fast to bring the CPUs online and it isn't a huge proportion of the *overall* kexec time.
However, your code in serial mode (cpuhp.parallel=0) is up to 40% *slower* than before. A/B testing results, all values in milliseconds:
Oh, that's unexpected.
Quoted 15 lines
┌───────────┬──────┬───────────┬────────────────┬─────────────────┐ │ platform │ ncpu │ A 7.3-rc2 │ A' Will-serial │ B Will-parallel │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ a1-metal │ 16 │ 5.7 │ 6.7 (+16.8%) │ 5.2 (−8.6%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ a1-virt │ 16 │ 13.6 │ 15.7 (+15.4%) │ 10.5 (−22.5%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ m6g-metal │ 64 │ 21.7 │ 29.8 (+37.3%) │ 15.5 (−28.8%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ m6g-virt │ 64 │ 27.0 │ 30.9 (+14.5%) │ 23.0 (−14.6%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ c7g-metal │ 64 │ 20.6 │ 26.0 (+26.3%) │ 11.9 (−41.9%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ c7g-virt │ 64 │ 26.8 │ 31.0 (+15.4%) │ 21.7 (−19.1%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤
Quoted 8 lines
│ c8g-metal │ 192 │ 81.7 │ 111.2 (+36.0%) │ 53.0 (−35.2%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ c8g-virt │ 192 │ 110.7 │ 120.4 (+8.8%) │ 96.4 (−12.9%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ m9g-metal │ 192 │ 69.0 │ 97.2 (+40.9%) │ 43.9 (−36.3%) │ ├───────────┼──────┼───────────┼────────────────┼─────────────────┤ │ m9g-virt │ 192 │ 128.6 │ 138.9 (+8.0%) │ 112.2 (−12.7%) │ └───────────┴──────┴───────────┴────────────────┴─────────────────┘
Would it be possible for you to pick one of these platforms and bisect the serial regression, please? I'm not really sure where to look, but serial boot should work all the way through the series so if you can identify the point at which it regresses (and presumably doesn't recover) then that would hopefully point me in the right direction. One possibility is that the extra atomics in the new state machine logic are slowing things down. Another possibility is the timeout logic in cpuhp_wait_for_sync_state() ends up sleeping in your tests.
Probably worth using my v2 just in case one of the fixes there helps, but I'm not hopeful.
Quoted 5 lines
We also tried it on another system where the firmware takes about 9ms to bring each CPU up (after
CPU_ONreturns fairly quickly). Fanning out theCPU_ONcalls didn't make any difference either; the CPUs came online, one at a time, about 9ms apart. The parallel onlining saved only about 20ms out of 820ms (→800ms) here.
Damn, I guess the firmware has some serialisation in that case?
Quoted 6 lines
The real answer for such platforms (at least for kexec) is *not* to do the CPU_OFF/CPU_ON thing at all. Pasha's Caretaker work² still does so for the reclaim at hotplug time in the next kernel; I'm experimenting with eliding that, which should give the biggest improvement on such platforms. So during kexec the APs just spin and wait to be asked to come back, instead of going completely offline.
I was talking to Tarun about that at LPC. I was envisaging a form of CPU_OFF that would leave the MMU enabled so we could make use of the existing EFI boot logic, but I hadn't thought about it beyond that.
Will
Quoted 4 lines · Will Deacon
Hi David,
Thanks for replying with so much data! It looks like I sent my v2 out at the same time.
Yeah. I'm redoing the full set of tests on that, along with the spinwait variant.
Quoted 10 lines · David Woodhouse
Quoted 7 lines
Thanks both of you for working on this. It was always on my list to come back and do it for arm64, but that list is long. Also on my list FWIW is parallelising the *later* stages of CPU hotplug¹, not just the early path into start_secondary()/secondary_start_kernel(). IIRC there was more win to be had, but we had to prove that a lot of once- serialized code could safely run concurrently. Ideally without just naïvely adding locking and serializing it all again.
That gets _really_ hard because you need RCU up and running pretty early.
RCU should be OK, as long as a CPU which is registered doesn't *then* block in a spin-wait, like the traditional CPU hotplug machinery does.
Quoted 12 lines
Quoted 8 lines
I gave your series a quick spin across a range of EC2 systems, and it gives a ~40% win on systems with 192 cores — at least, as a microbenchmark of the CPU onlining. For these systems, PSCI is fairly fast to bring the CPUs online and it isn't a huge proportion of the *overall* kexec time.
However, your code in serial mode (cpuhp.parallel=0) is up to 40% *slower* than before. A/B testing results, all values in milliseconds:
Oh, that's unexpected.
...
Quoted 8 lines
Would it be possible for you to pick one of these platforms and bisect the serial regression, please? I'm not really sure where to look, but serial boot should work all the way through the series so if you can identify the point at which it regresses (and presumably doesn't recover) then that would hopefully point me in the right direction. One possibility is that the extra atomics in the new state machine logic are slowing things down. Another possibility is the timeout logic in
cpuhp_wait_for_sync_state()ends up sleeping in your tests.
Looks like the latter, falling back to usleep_range(1000, 2000). Goes away if you make it atomic_cond_read_relaxed():
https://git.infradead.org/?p=users/dwmw2/linux.git;a=commitdiff;h=cba8bdecff74
Of course, if we weren't synchronously *waiting* for the APs and they just turned up later, the world would be a better place. And the ones with slow firmware wouldn't matter either.
Unless we're actually doing parallel initcalls, we don't even *need* the secondary CPUs until much later, do we?
Quoted 10 lines
Probably worth using my v2 just in case one of the fixes there helps, but I'm not hopeful.
Quoted 5 lines
We also tried it on another system where the firmware takes about 9ms to bring each CPU up (after
CPU_ONreturns fairly quickly). Fanning out theCPU_ONcalls didn't make any difference either; the CPUs came online, one at a time, about 9ms apart. The parallel onlining saved only about 20ms out of 820ms (→800ms) here.Damn, I guess the firmware has some serialisation in that case?
$DEITY knows, but I guess so. We don't get to write the firmware for those ones, AIUI.
Quoted 10 lines
Quoted 6 lines
The real answer for such platforms (at least for kexec) is *not* to do the CPU_OFF/CPU_ON thing at all. Pasha's Caretaker work² still does so for the reclaim at hotplug time in the next kernel; I'm experimenting with eliding that, which should give the biggest improvement on such platforms. So during kexec the APs just spin and wait to be asked to come back, instead of going completely offline.
I was talking to Tarun about that at LPC. I was envisaging a form of
CPU_OFFthat would leave the MMU enabled so we could make use of the existing EFI boot logic, but I hadn't thought about it beyond that.
I think it ties closely into the caretaker thing. We don't take the CPU offline at all (no firmware, no CPU_OFF of any kind). Instead, we just leave it parked somewhere.
The first stage is leaving it parked and just spinning waiting to come back. Which is all we care about here.
The next stages involve making it do useful *work* instead of just spinning. Like Pasha's KVM vCPU caretaker, and the thing I'm working on which runs a minimal C runtime driving an e1000 through VFIO, transitioning onto bare metal on the "offline" CPUs across kexec, then back to running in a Linux thread after kexec. All in QEMU while being flood-pinged from the host across the kexec.
Quoted 8 lines · David Woodhouse
Quoted 4 lines · Will Deacon
Hi David,
Thanks for replying with so much data! It looks like I sent my v2 out at the same time.
Yeah. I'm redoing the full set of tests on that, along with the spinwait variant.
With cpuhp_wait_for_sync_state() using atomic_cond_read_relaxed() the performance hit of your series in serial mode is fixed to match vanilla, so I dropped that column.
And while spinwait alone is a modest win, it doesn't buy us anything on top of parallel bringup on *this* hardware. In fact it's *slightly* slower on the biggest hardware, because I haven't yet batched some of the flushes properly. I'll look at that, and rerun on the machines with slow CPU_ON which is where the spinwait will come into its own.
https://git.infradead.org/?p=users/dwmw2/linux.git;a=shortlog;h=refs/heads/spinwait-will2fix
┌───────────┬──────┬─────────┬──────────┬──────────┬───────────────────┐ │ platform │ ncpu │ vanilla │ spinwait │ parallel │ parallel+spinwait │ ├───────────┼──────┼─────────┼──────────┼──────────┼───────────────────┤ │ a1-metal │ 16 │ 5.6 │ 5.3 │ 5.2 │ 5.2 │ ├───────────┼──────┼─────────┼──────────┼──────────┼───────────────────┤ │ a1-virt │ 16 │ 13.5 │ 13.2 │ 10.6 │ 10.6 │ ├───────────┼──────┼─────────┼──────────┼──────────┼───────────────────┤ │ m6g-metal │ 64 │ 21.7 │ 16.5 │ 15.4 │ 15.2 │ ├───────────┼──────┼─────────┼──────────┼──────────┼───────────────────┤ │ m6g-virt │ 64 │ 27.1 │ 26.5 │ 22.9 │ 22.7 │ ├───────────┼──────┼─────────┼──────────┼──────────┼───────────────────┤ │ c7g-metal │ 64 │ 20.6 │ 13.6 │ 11.9 │ 11.8 │ ├───────────┼──────┼─────────┼──────────┼──────────┼───────────────────┤ │ c7g-virt │ 64 │ 27.0 │ 26.8 │ 21.7 │ 21.8 │ ├───────────┼──────┼─────────┼──────────┼──────────┼───────────────────┤ │ c8g-metal │ 192 │ 81.9 │ 60.5 │ 52.7 │ 58.7 │ ├───────────┼──────┼─────────┼──────────┼──────────┼───────────────────┤ │ c8g-virt │ 192 │ 111.0 │ (noisy) │ 96.4 │ 101.8 │ ├───────────┼──────┼─────────┼──────────┼──────────┼───────────────────┤ │ m9g-metal │ 192 │ 68.9 │ 56.0 │ 44.7 │ 50.0 │ ├───────────┼──────┼─────────┼──────────┼──────────┼───────────────────┤ │ m9g-virt │ 192 │ 128.5 │ 123.6 │ 112.8 │ 111.7 │ └───────────┴──────┴─────────┴──────────┴──────────┴───────────────────┘
On Fri, 2026-10-09 at 17:04 +0100, David Woodhouse wrote
Quoted 5 lines
And while spinwait alone is a modest win, it doesn't buy us anything on top of parallel bringup on *this* hardware. In fact it's *slightly* slower on the biggest hardware, because I haven't yet batched some of the flushes properly. I'll look at that, and rerun on the machines with slow
CPU_ONwhich is where the spinwait will come into its own.
Retested there. For the CPU onlining:
Upstream: 820ms
Parallel: 800ms
Spinwait: 35-40ms
So I think we definitely need both.
A trivial spinwait with MMU *off* also causes the BSP to spend an extra 200ms in paging_init() on this hardware. And we want the MMU on for all the interesting caretaker use cases anyway, so I'll do that.
We *already* wanted exception handling in arm64_relocate_new_kernel() before any of this. So I'll do that first, as I did for x86 in
https://lore.kernel.org/all/[email protected]/
Shape of the upcoming series:
• Exception handling and serial print
• Simple handover to spinwaiter, and handback protocol
• More complex handover to caretaker workloads (like my proof of concept e1000 ping responder, which is now working)
... and then it devolves into Pasha's caretaker series.
I'll give some thought to whether the spinwait should be a LUO thing, gated on the incoming kernel advertising *support* for such, using the feature negotiation we talked about at LPC. I'll probably start with punting the decision to userspace for now.