Re: [PATCH v2] KVM: x86: Unify L1TF flushing under per-CPU variable

[Sean Christopherson <seanjc@google.com>]

On Thu, Oct 16, 2025, Brendan Jackman wrote:
> On Thu Oct 16, 2025 at 3:50 PM UTC, Sean Christopherson wrote:
> > On Wed, Oct 15, 2025, Brendan Jackman wrote:
> >> Currently the tracking of the need to flush L1D for L1TF is tracked by
> >> two bits: one per-CPU and one per-vCPU.
> >> 
> >> The per-vCPU bit is always set when the vCPU shows up on a core, so
> >> there is no interesting state that's truly per-vCPU. Indeed, this is a
> >> requirement, since L1D is a part of the physical CPU.
> >> 
> >> So simplify this by combining the two bits.
> >> 
> >> The vCPU bit was being written from preemption-enabled regions. For
> >> those cases, use raw_cpu_write() (via a variant of the setter function)
> >> to avoid DEBUG_PREEMPT failures. If the vCPU is getting migrated, the
> >> CPU that gets its bit set in these paths is not important; vcpu_load()
> >> must always set it on the destination CPU before the guest is resumed.
> >> 
> >> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> >> ---
> >
> > ...
> >
> >> @@ -78,6 +79,11 @@ static __always_inline void kvm_set_cpu_l1tf_flush_l1d(void)
> >>  	__this_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, 1);
> >>  }
> >>  
> >> +static __always_inline void kvm_set_cpu_l1tf_flush_l1d_raw(void)
> >> +{
> >> +	raw_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, 1);
> >> +}
> >
> > TL;DR: I'll post a v3 with a slightly tweaked version of this patch at the end.
> >
> > Rather than add a "raw" variant, I would rather have a wrapper in arch/x86/kvm/x86.h
> > that disables preemption, with a comment explaining why it's ok to enable preemption
> > after setting the per-CPU flag.  Without such a comment, choosing between the two
> > variants looks entirely random
> >
> > Alternatively, all writes could be raw, but that
> > feels wrong/weird, and in practice disabling preemption in the relevant paths is a
> > complete non-issue.
> 
> Hm, why does making every write _raw feel weird but adding
> preempt_disable() to every write doesn't? Both feel equally weird to me.

I completely agree that both approaches are odd/weird.

> But the latter has the additional weirdness of using preempt_disable()
> as a way to signal "I know what I'm doing", when that signal is already
> explicitly documented as the purpose of raw_cpu_write().

True.  Aha!

With the #ifdefs in place, KVM doesn't need arch/x86/include/asm/hardirq.h to
provide a wrapper.  irq_stat is already exported, the wrapper exists purely so
that kvm_set_cpu_l1tf_flush_l1d() can be invoked without callers having to check
CONFIG_KVM_INTEL.

Not yet tested, but how about this?

static __always_inline void kvm_request_l1tf_flush_l1d(void)
{
#if IS_ENABLED(CONFIG_CPU_MITIGATIONS) && IS_ENABLED(CONFIG_KVM_INTEL)
	/*
	 * Use a raw write to set the per-CPU flag, as KVM will ensure a flush
	 * even if preemption is currently enabled..  If the current vCPU task
	 * is migrated to a different CPU (or userspace runs the vCPU on a
	 * different task) before the next VM-Entry, then kvm_arch_vcpu_load()
	 * will request a flush on the new CPU.
	 */
	raw_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, 1);
#endif
}