Re: [PATCH 3/3] x86/mmio: Unify VERW mitigation for guests

[Brendan Jackman <jackmanb@google.com>]

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

>  
>         /* 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?

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". So basically we moved cpu_buf_vm_clear_mmio_only into a
CPU feature to make it accessible from asm?

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