Re: [PATCH v5 6/6] KVM: VMX: Move VERW closer to VMentry for MDS mitigation

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

On Thu, Jan 11, 2024 at 08:45:13AM -0800, Sean Christopherson wrote:
> On Thu, Jan 11, 2024, Pawan Gupta wrote:
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index bdcf2c041e0c..8defba8e417b 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -387,6 +387,17 @@ static __always_inline void vmx_enable_fb_clear(struct vcpu_vmx *vmx)
> >  
> >  static void vmx_update_fb_clear_dis(struct kvm_vcpu *vcpu, struct vcpu_vmx *vmx)
> >  {
> > +	/*
> > +	 * FB_CLEAR_CTRL is to optimize VERW latency in guests when host is
> > +	 * affected by MMIO Stale Data, but not by MDS/TAA. When
> > +	 * X86_FEATURE_CLEAR_CPU_BUF is enabled, system is likely affected by
> > +	 * MDS/TAA. Skip the optimization for such a case.
> 
> This is unnecessary speculation (ha!), and it'll also be confusing for many readers
> as the code below explicitly checks for MDS/TAA.  We have no idea why the host
> admin forced the mitigation to be enabled, and it doesn't matter.  The important
> thing to capture is that the intent is to keep the mitigation enabled when it
> was forcefully enabled, that should be self-explanatory and doesn't require
> speculating on _why_ the mitigation was forced on.

Agree.

> > +	 */
> > +	if (cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF)) {
> > +		vmx->disable_fb_clear = false;
> > +		return;
> > +	}
> > +
> >  	vmx->disable_fb_clear = (host_arch_capabilities & ARCH_CAP_FB_CLEAR_CTRL) &&
> >  				!boot_cpu_has_bug(X86_BUG_MDS) &&
> >  				!boot_cpu_has_bug(X86_BUG_TAA);
> 
> I would rather include the X86_FEATURE_CLEAR_CPU_BUF check along with all the
> other checks, and then add a common early return. E.g.
> 
> 	/*
> 	 * Disable VERW's behavior of clearing CPU buffers for the guest if the
> 	 * CPU isn't affected MDS/TAA, and the host hasn't forcefully enabled
> 	 * the mitigation.  Disabing the clearing provides a performance boost
> 	 * for guests that aren't aware that manually clearing CPU buffers is
> 	 * unnecessary, at the cost of MSR accesses on VM-Entry and VM-Exit.
> 	 */
> 	vmx->disable_fb_clear = !cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF) &&
> 				(host_arch_capabilities & ARCH_CAP_FB_CLEAR_CTRL) &&
> 				!boot_cpu_has_bug(X86_BUG_MDS) &&
> 				!boot_cpu_has_bug(X86_BUG_TAA);
> 
> 	if (!vmx->disable_fb_clear)
> 		return;

This is better. Thanks.