Received-SPF: pass (google.com: domain of linux-kernel+bounces-23887-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 11 Jan 2024 08:45:13 -0800
In-Reply-To: <20240111-delay-verw-v5-6-a3b234933ea6@linux.intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240111-delay-verw-v5-0-a3b234933ea6@linux.intel.com> <20240111-delay-verw-v5-6-a3b234933ea6@linux.intel.com>
Message-ID: <ZaAbGWFEfUt1PX66@google.com>
Subject: Re: [PATCH  v5 6/6] KVM: VMX: Move VERW closer to VMentry for MDS mitigation
From: Sean Christopherson <seanjc@google.com>
To: Pawan Gupta <pawan.kumar.gupta@linux.intel.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
Content-Type: text/plain; charset="us-ascii"

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.

> +	 */
> +	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;