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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aPEgNdjr0j4LdSYq@google.com>
Date: Thu, 16 Oct 2025 09:41:25 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Brendan Jackman <jackmanb@...gle.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>, Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org, 
	kvm@...r.kernel.org
Subject: Re: [PATCH v2] KVM: x86: Unify L1TF flushing under per-CPU variable

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@...gle.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
}