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: <DDJVZU914RVD.1HXRX01BELY4L@google.com>
Date: Thu, 16 Oct 2025 16:28:31 +0000
From: Brendan Jackman <jackmanb@...gle.com>
To: Sean Christopherson <seanjc@...gle.com>, 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 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.
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().

> <me rummages around>
>
> Gah, I followed a tangential thought about the "cost" of disabling/enabling preemtion
> and ended up with a 4-patch series.  All of this code really should be conditioned
> on CONFIG_CPU_MITIGATIONS=y.  With that, the wrapper can be:
>
> static __always_inline void kvm_request_l1tf_flush_l1d(void)
> {
> #if IS_ENABLED(CONFIG_CPU_MITIGATIONS) && IS_ENABLED(CONFIG_KVM_INTEL)
> 	/*
> 	 * Temporarily disable preemption (if necessary) as the tracking is
> 	 * per-CPU.  If the current vCPU task is migrated to a different CPU
> 	 * before the next VM-Entry, then kvm_arch_vcpu_load() will pend a
> 	 * flush on the new CPU.
> 	 */
> 	guard(preempt)();
> 	kvm_set_cpu_l1tf_flush_l1d();
> #endif
> }

Having a nice place to hang the comment definitely seems worthwhile, but
couldn't we just put it in the _raw varant?

> and kvm_set_cpu_l1tf_flush_l1d() and irq_cpustat_t.kvm_cpu_l1tf_flush_l1d can
> likewise be gated on CONFIG_CPU_MITIGATIONS && CONFIG_KVM_INTEL.
>
>
>> +
>>  static __always_inline void kvm_clear_cpu_l1tf_flush_l1d(void)
>>  {
>>  	__this_cpu_write(irq_stat.kvm_cpu_l1tf_flush_l1d, 0);
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 48598d017d6f3f07263a2ffffe670be2658eb9cb..fcdc65ab13d8383018577aacf19e832e6c4ceb0b 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1055,9 +1055,6 @@ struct kvm_vcpu_arch {
>>  	/* be preempted when it's in kernel-mode(cpl=0) */
>>  	bool preempted_in_kernel;
>>  
>> -	/* Flush the L1 Data cache for L1TF mitigation on VMENTER */
>> -	bool l1tf_flush_l1d;
>> -
>>  	/* Host CPU on which VM-entry was most recently attempted */
>>  	int last_vmentry_cpu;
>>  
>> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
>> index 667d66cf76d5e52c22f9517914307244ae868eea..8c0dce401a42d977756ca82d249bb33c858b9c9f 100644
>> --- a/arch/x86/kvm/mmu/mmu.c
>> +++ b/arch/x86/kvm/mmu/mmu.c
>> @@ -4859,7 +4859,7 @@ int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
>>  	 */
>>  	BUILD_BUG_ON(lower_32_bits(PFERR_SYNTHETIC_MASK));
>>  
>> -	vcpu->arch.l1tf_flush_l1d = true;
>> +	kvm_set_cpu_l1tf_flush_l1d();
>
> This is wrong, kvm_handle_page_fault() runs with preemption enabled.

Ah, thanks.