Message-ID: <20251030180621.c4jk3isi3pmzeolw@desk>
Date: Thu, 30 Oct 2025 11:06:21 -0700
From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
To: Brendan Jackman <jackmanb@...gle.com>
Cc: Sean Christopherson <seanjc@...gle.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Borislav Petkov <bp@...en8.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Josh Poimboeuf <jpoimboe@...nel.org>,
	Ingo Molnar <mingo@...hat.com>,
	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, Tao Zhang <tao1.zhang@...el.com>,
	Jim Mattson <jmattson@...gle.com>
Subject: Re: [PATCH 3/3] x86/mmio: Unify VERW mitigation for guests

On Thu, Oct 30, 2025 at 04:26:10PM +0000, Brendan Jackman wrote:
> On Thu Oct 30, 2025 at 4:06 PM UTC, Sean Christopherson wrote:
> > On Thu, Oct 30, 2025, Brendan Jackman wrote:
> >> > @@ -160,6 +163,8 @@ SYM_FUNC_START(__vmx_vcpu_run)
> >> >  	/* Load guest RAX.  This kills the @regs pointer! */
> >> >  	mov VCPU_RAX(%_ASM_AX), %_ASM_AX
> >> >  
> >> > +	/* Check EFLAGS.ZF from the VMX_RUN_CLEAR_CPU_BUFFERS bit test above */
> >> > +	jz .Lskip_clear_cpu_buffers
> >> 
> >> Hm, it's a bit weird that we have the "alternative" inside
> >> VM_CLEAR_CPU_BUFFERS, but then we still keep the test+jz
> >> unconditionally. 
> >
> > Yeah, I had the same reaction, but couldn't come up with a clean-ish solution
> > and so ignored it :-)
> >
> >> If we really want to super-optimise the no-mitigations-needed case,
> >> shouldn't we want to avoid the conditional in the asm if it never
> >> actually leads to a flush?
> >> 
> >> On the other hand, if we don't mind a couple of extra instructions,
> >> shouldn't we be fine with just having the whole asm code based solely
> >> on VMX_RUN_CLEAR_CPU_BUFFERS and leaving the
> >> X86_FEATURE_CLEAR_CPU_BUF_VM to the C code?
> >> 
> >> I guess the issue is that in the latter case we'd be back to having
> >> unnecessary inconsistency with AMD code while in the former case... well
> >> that would just be really annoying asm code - am I on the right
> >> wavelength there? So I'm not necessarily asking for changes here, just
> >> probing in case it prompts any interesting insights on your side.
> >> 
> >> (Also, maybe this test+jz has a similar cost to the nops that the
> >> "alternative" would inject anyway...?)
> >
> > It's not at all expensive.  My bigger objection is that it's hard to follow what's
> > happening.
> >
> > Aha!  Idea.  IIUC, only the MMIO Stale Data is conditional based on the properties
> > of the vCPU, so we should track _that_ in a KVM_RUN flag.  And then if we add yet
> > another X86_FEATURE for MMIO Stale Data flushing (instead of a static branch),
> > this path can use ALTERNATIVE_2.  The use of ALTERNATIVE_2 isn't exactly pretty,
> > but IMO this is much more intuitive.
> >
> > diff --git a/arch/x86/kvm/vmx/run_flags.h b/arch/x86/kvm/vmx/run_flags.h
> > index 004fe1ca89f0..b9651960e069 100644
> > --- a/arch/x86/kvm/vmx/run_flags.h
> > +++ b/arch/x86/kvm/vmx/run_flags.h
> > @@ -4,10 +4,10 @@
> >  
> >  #define VMX_RUN_VMRESUME_SHIFT                 0
> >  #define VMX_RUN_SAVE_SPEC_CTRL_SHIFT           1
> > -#define VMX_RUN_CLEAR_CPU_BUFFERS_SHIFT                2
> > +#define VMX_RUN_CAN_ACCESS_HOST_MMIO_SHIT      2
> >  
> >  #define VMX_RUN_VMRESUME               BIT(VMX_RUN_VMRESUME_SHIFT)
> >  #define VMX_RUN_SAVE_SPEC_CTRL         BIT(VMX_RUN_SAVE_SPEC_CTRL_SHIFT)
> > -#define VMX_RUN_CLEAR_CPU_BUFFERS      BIT(VMX_RUN_CLEAR_CPU_BUFFERS_SHIFT)
> > +#define VMX_RUN_CAN_ACCESS_HOST_MMIO   BIT(VMX_RUN_CAN_ACCESS_HOST_MMIO_SHIT)
> >  
> >  #endif /* __KVM_X86_VMX_RUN_FLAGS_H */
> > diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> > index ec91f4267eca..50a748b489b4 100644
> > --- a/arch/x86/kvm/vmx/vmenter.S
> > +++ b/arch/x86/kvm/vmx/vmenter.S
> > @@ -137,8 +137,10 @@ SYM_FUNC_START(__vmx_vcpu_run)
> >         /* Load @regs to RAX. */
> >         mov (%_ASM_SP), %_ASM_AX
> >  
> > -       /* jz .Lskip_clear_cpu_buffers below relies on this */
> > -       test $VMX_RUN_CLEAR_CPU_BUFFERS, %ebx
> > +       /* Check if jz .Lskip_clear_cpu_buffers below relies on this */
> > +       ALTERNATIVE_2 "",
> > +                     "", X86_FEATURE_CLEAR_CPU_BUF
> > +                     "test $VMX_RUN_CAN_ACCESS_HOST_MMIO, %ebx", X86_FEATURE_CLEAR_CPU_BUFFERS_MMIO
> 
> Er, I don't understand the ALTERNATIVE_2 here, don't we just need this?
> 
> ALTERNATIVE "" "test $VMX_RUN_CAN_ACCESS_HOST_MMIO, %ebx", 
> 	    X86_FEATURE_CLEAR_CPU_BUFFERS_MMIO

Yeah, right.

> >         /* Check if vmlaunch or vmresume is needed */
> >         bt   $VMX_RUN_VMRESUME_SHIFT, %ebx
> > @@ -163,8 +165,9 @@ SYM_FUNC_START(__vmx_vcpu_run)
> >         /* Load guest RAX.  This kills the @regs pointer! */
> >         mov VCPU_RAX(%_ASM_AX), %_ASM_AX
> >  
> > -       /* Check EFLAGS.ZF from the VMX_RUN_CLEAR_CPU_BUFFERS bit test above */
> > -       jz .Lskip_clear_cpu_buffers
> > +       ALTERNATIVE_2 "jmp .Lskip_clear_cpu_buffers",
> > +                     "", X86_FEATURE_CLEAR_CPU_BUF
> > +                     "jz .Lskip_clear_cpu_buffers", X86_FEATURE_CLEAR_CPU_BUFFERS_MMIO
> 
> To fit with the rest of Pawan's code this would need
> s/X86_FEATURE_CLEAR_CPU_BUF/X86_FEATURE_CLEAR_CPU_BUF_VM/, right?

Yes.

> In case it reveals that I just don't understand ALTERNATIVE_2 at all,
> I'm reading this second one as saying:
> 
> if cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUFFERS_MMIO)
>    "jz .Lskip_clear_cpu_buffers "
> else if !cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF_VM)
>    "jmp .Lskip_clear_cpu_buffers"
>
> I.e. I'm understanding X86_FEATURE_CLEAR_CPU_BUFFERS_MMIO as mutually
> exclusive with X86_FEATURE_CLEAR_CPU_BUF_VM, it means "you _only_ need
> to verw MMIO".

Yes, that's also my understanding.

> So basically we moved cpu_buf_vm_clear_mmio_only into a
> CPU feature to make it accessible from asm?

Essentially, yes.

> (Also let's use BUF instead of BUFFERS in the name, for consistency)

Agree.

Powered by blists - more mailing lists