Re: [PATCH v1 2/8] x86/smp: Remove unnecessary WBINVD from mwait_play_dead()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] x86/smp: memory corruption during suspend if package offlined at runtime
- [High] x86/smp: offline CPUs miss wbinvd_on_all_cpus() flush causing data corruption
--

commit 2579aa1d01a44c8d96bb166fd2ce0dff4493a42a
Author: Pasha Tatashin <pasha.tatashin@soleen.com>

x86/smp: Remove unnecessary WBINVD from mwait_play_dead()

The commit aims to avoid time-consuming WBINVDs during runtime CPU offlining
by only executing it when cpuhp_tasks_frozen is set (i.e. system sleep).

> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index ba01a9e919b78..60a781dac9991 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
[ ... ]
> @@ -1426,15 +1422,21 @@ void __noreturn mwait_play_dead(unsigned int eax_hint)
>  	md->status = CPUDEAD_MWAIT_WAIT;
>  	md->control = CPUDEAD_MWAIT_WAIT;
>  
> -	wbinvd();
> +	/*
> +	 * MWAIT C-states keep the caches coherent, so runtime CPU offline
> +	 * does not need to flush them. System sleep, however, may remove
> +	 * power from this package's caches while all of its CPUs are
> +	 * offline, and the flush done by the boot CPU before entering the
> +	 * sleep state only covers the boot CPU's own package.
> +	 */
> +	if (cpuhp_tasks_frozen)
> +		wbinvd();

[Severity: Critical]
Could this result in memory corruption during a system suspend if an entire
package is offlined at runtime prior to the suspend?

If a package is fully offlined at runtime (so cpuhp_tasks_frozen is false),
its CPUs enter MWAIT with dirty caches. If the system later suspends,
freeze_secondary_cpus() skips CPUs that are already offline:

kernel/cpu.c:freeze_secondary_cpus() {
	...
	for (cpu = nr_cpu_ids - 1; cpu >= 0; cpu--) {
		if (!cpu_online(cpu) || cpu == primary)
			continue;
	...
}

Because these CPUs are skipped, they never awaken to check cpuhp_tasks_frozen
and perform the flush. Since the sleep-entry cache flush on the boot CPU
only covers its own package, won't the dirty cache lines of the un-flushed,
offlined package be permanently lost when power is removed?

[Severity: High]
Does skipping the flush during runtime offline break the system-wide cache
flush guarantees of wbinvd_on_all_cpus()?

If a package is completely offlined at runtime and retains dirty cache lines
in its LLC, wbinvd_on_all_cpus() will miss them entirely because it only
targets online CPUs:

arch/x86/lib/cache-smp.c:wbinvd_on_all_cpus() {
	on_each_cpu(__wbinvd, NULL, 1);
}

If a system-wide non-coherent operation is later triggered (such as mapping
memory as Uncacheable or persisting PMEM), the missing flush could lead to
data corruption or Machine Check Exceptions from dirty line evictions.

Furthermore, during Kexec, offline CPUs are kicked out of MWAIT:

arch/x86/kernel/smpboot.c:smp_kick_mwait_play_dead() {
	...
	for_each_cpu_andnot(cpu, cpu_present_mask, cpu_online_mask) {
		...
		WRITE_ONCE(md->control, newstate);
	...
}

Will this cause the offlined CPUs to carry unflushed dirty cache lines into
the new kernel's execution context?

>  
>  	while (1) {
>  		/*
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015153331.1872641-1-pasha.tatashin@soleen.com?part=2