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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] x86/smp: skipping wbinvd() in mwait_play_dead() breaks KVM wbinvd_dirty_mask
--

commit 52dafb51edf021a7aaa6017af5d2ddd2b493e51b
Author: Pasha Tatashin <pasha.tatashin@soleen.com>

x86/smp: Remove unnecessary WBINVD from mwait_play_dead()

This commit skips the wbinvd() call in mwait_play_dead() when entering an
MWAIT C-state deeper than C1/C1E while another CPU sharing the same
package and LLC remains online.

> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index ba01a9e919b78..0dccd2f927bb6 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1414,10 +1414,28 @@ void play_dead_common(void)
>  	local_irq_disable();
>  }
>  
> -/*
> - * We need to flush the caches before going to sleep, lest we have
> - * dirty data in our caches when we come back up.
> - */
> +static bool mwait_play_dead_needs_wbinvd(unsigned int eax_hint)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	u32 pkg_id = topology_physical_package_id(cpu);
> +	u32 llc_id = per_cpu_llc_id(cpu);
> +	unsigned int i;
> +
> +	if (cpuhp_tasks_frozen || MWAIT_HINT2CSTATE(eax_hint) < 2 ||
> +	    llc_id == BAD_APICID || this_cpu_read(cache_state_incoherent)) {
> +		return true;
> +	}
> +
> +	for_each_online_cpu(i) {
> +		if (topology_physical_package_id(i) == pkg_id &&
> +		    per_cpu_llc_id(i) == llc_id) {
> +			return false;
> +		}
> +	}

[Severity: High]
Can skipping wbinvd() in mwait_play_dead_needs_wbinvd() when another CPU in
the same LLC is online leave a KVM guest's dirty cache lines unflushed in
the shared LLC when emulating guest WBINVD?

For KVM vCPUs with non-coherent DMA enabled (need_emulate_wbinvd(vcpu)),
KVM does not use wbinvd_on_all_cpus(). Instead, kvm_arch_vcpu_load() tracks
only the specific physical CPU where the vCPU ran in
vcpu->arch.wbinvd_dirty_mask (or flushes vcpu->cpu via wbinvd_on_cpu()):

arch/x86/kvm/x86.c:kvm_arch_vcpu_load() {
    ...
	if (need_emulate_wbinvd(vcpu)) {
		if (kvm_x86_call(has_wbinvd_exit)())
			cpumask_set_cpu(cpu, vcpu->arch.wbinvd_dirty_mask);
		else if (vcpu->cpu != -1 && vcpu->cpu != cpu)
			wbinvd_on_cpu(vcpu->cpu);
	}
    ...
}

When the guest later executes WBINVD, kvm_emulate_wbinvd_noskip() flushes
only the CPUs recorded in vcpu->arch.wbinvd_dirty_mask and clears the mask:

arch/x86/kvm/x86.c:kvm_emulate_wbinvd_noskip() {
    ...
	if (kvm_x86_call(has_wbinvd_exit)()) {
		int cpu = get_cpu();

		cpumask_set_cpu(cpu, vcpu->arch.wbinvd_dirty_mask);
		wbinvd_on_cpus_mask(vcpu->arch.wbinvd_dirty_mask);
		put_cpu();
		cpumask_clear(vcpu->arch.wbinvd_dirty_mask);
	} else
		wbinvd();
    ...
}

wbinvd_on_cpus_mask() calls on_each_cpu_mask() ->
smp_call_function_many_cond(), which filters the target mask against
cpu_online_mask:

kernel/smp.c:smp_call_function_many_cond() {
    ...
	if (cpumask_any_and_but(mask, cpu_online_mask, this_cpu) < nr_cpu_ids) {
		cpumask_and(cpumask, mask, cpu_online_mask);
    ...
}

If a vCPU runs on physical CPU A (in LLC 0), migrates to CPU B (in LLC 1),
and CPU A goes offline via mwait_play_dead() while sibling CPU C in LLC 0
remains online, CPU A will now skip wbinvd() and only flush its core
caches into LLC 0 (or leave lines in shared L1/L2 if CPU C is an SMT
sibling).

If the vCPU never ran on CPU C, CPU C is not in
vcpu->arch.wbinvd_dirty_mask. When the guest subsequently executes WBINVD,
wbinvd_on_cpus_mask() ignores the offline CPU A and never sends an IPI to
CPU C, leaving the guest's dirty lines in LLC 0 unflushed to DRAM before
clearing vcpu->arch.wbinvd_dirty_mask.

> +
> +	return true;
> +}
[ ... ]

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