Received-SPF: pass (google.com: domain of linux-kernel+bounces-24157-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 11 Jan 2024 16:02:06 -0800
From: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
	"H. Peter Anvin" <hpa@zytor.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Josh Poimboeuf <jpoimboe@kernel.org>,
	Andy Lutomirski <luto@kernel.org>, Jonathan Corbet <corbet@lwn.net>,
	Paolo Bonzini <pbonzini@redhat.com>, tony.luck@intel.com,
	ak@linux.intel.com, tim.c.chen@linux.intel.com,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Nikolay Borisov <nik.borisov@suse.com>,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	kvm@vger.kernel.org,
	Alyssa Milburn <alyssa.milburn@linux.intel.com>,
	Daniel Sneddon <daniel.sneddon@linux.intel.com>,
	antonio.gomez.iglesias@linux.intel.com
Subject: Re: [PATCH  v5 6/6] KVM: VMX: Move VERW closer to VMentry for MDS
 mitigation
Message-ID: <20240112000206.ur5ub5bf5noesvc3@desk>
References: <20240111-delay-verw-v5-0-a3b234933ea6@linux.intel.com>
 <20240111-delay-verw-v5-6-a3b234933ea6@linux.intel.com>
 <ZaAbGWFEfUt1PX66@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZaAbGWFEfUt1PX66@google.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.