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

[Pawan Gupta <pawan.kumar.gupta@linux.intel.com>]

On Thu, Oct 30, 2025 at 09:06:41AM -0700, 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 :-)

Ya, it is tricky to handle per-guest mitigation for MMIO in a clean way.

> > 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

This approach looks better. I think we will be fine without ALTERNATIVE_2:

       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

I am not 100% sure, but I believe the _MMIO check needs to be before
X86_FEATURE_CLEAR_CPU_BUF_VM, because MMIO mitigation also sets _VM:

       ALTERNATIVE_2 "jmp .Lskip_clear_cpu_buffers",
                     "jz .Lskip_clear_cpu_buffers", X86_FEATURE_CLEAR_CPU_BUFFERS_MMIO
                     "", X86_FEATURE_CLEAR_CPU_BUF_VM

>         /* Clobbers EFLAGS.ZF */
>         VM_CLEAR_CPU_BUFFERS
>  .Lskip_clear_cpu_buffers:
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 303935882a9f..b9e7247e6b9a 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -903,16 +903,9 @@ unsigned int __vmx_vcpu_run_flags(struct vcpu_vmx *vmx)
>         if (!msr_write_intercepted(vmx, MSR_IA32_SPEC_CTRL))
>                 flags |= VMX_RUN_SAVE_SPEC_CTRL;
>  
> -       /*
> -        * When affected by MMIO Stale Data only (and not other data sampling
> -        * attacks) only clear for MMIO-capable guests.
> -        */
> -       if (static_branch_unlikely(&cpu_buf_vm_clear_mmio_only)) {
> -               if (kvm_vcpu_can_access_host_mmio(&vmx->vcpu))
> -                       flags |= VMX_RUN_CLEAR_CPU_BUFFERS;
> -       } else {
> -               flags |= VMX_RUN_CLEAR_CPU_BUFFERS;
> -       }
> +       if (cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUFFERS_MMIO) &&
> +           kvm_vcpu_can_access_host_mmio(&vmx->vcpu))
> +               flags |= VMX_RUN_CAN_ACCESS_HOST_MMIO;

Thanks Sean! This is much cleaner.