Re: [PATCH 18/19] arm64: smp: Use generic HOTPLUG_PARALLEL machinery for CPU onlining

[Jinjie Ruan]

在 2026/9/11 20:56, Will Deacon 写道:
> On Tue, Sep 08, 2026 at 09:05:18PM +0800, Jinjie Ruan wrote:
>> 在 2026/9/8 0:40, Will Deacon 写道:
>>> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
>>> index 17868b497d7c..bec4bc1b12db 100644
>>> --- a/arch/arm64/kernel/head.S
>>> +++ b/arch/arm64/kernel/head.S
>>> @@ -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
>>
>> Corrupts tmp1, tmp2 ?
> 
> Well spotted, thanks.
> 
>>> @@ -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;
>>> -	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!

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.

> 
> The parallel bringup code will detect the issue, it just won't be able
> to tell you which CPU caused which issue.

Indeed.

> 
>>> +	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++;
>>> -		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

"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.

> 
> so I don't think we need anything extra (as, as above, we don't know
> exactly which CPUs are stuck).
> 
>>>  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.
> 
> 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.

> 
> Will