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

[Brendan Jackman <jackmanb@google.com>]

On Thu Oct 16, 2025 at 4:41 PM UTC, Sean Christopherson wrote:
> 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
> }

Yeah, just poking irq_stat directly seems fine to me.