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.
