diff mbox series

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

Message ID 20240111-delay-verw-v5-6-a3b234933ea6@linux.intel.com
State New
Headers
Series Delay VERW |

Commit Message

Pawan Gupta Jan. 11, 2024, 8:56 a.m. UTC 
  During VMentry VERW is executed to mitigate MDS. After VERW, any memory
access like register push onto stack may put host data in MDS affected
CPU buffers. A guest can then use MDS to sample host data.

Although likelihood of secrets surviving in registers at current VERW
callsite is less, but it can't be ruled out. Harden the MDS mitigation
by moving the VERW mitigation late in VMentry path.

Note that VERW for MMIO Stale Data mitigation is unchanged because of
the complexity of per-guest conditional VERW which is not easy to handle
that late in asm with no GPRs available. If the CPU is also affected by
MDS, VERW is unconditionally executed late in asm regardless of guest
having MMIO access.

Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
---
 arch/x86/kvm/vmx/vmenter.S |  3 +++
 arch/x86/kvm/vmx/vmx.c     | 20 +++++++++++++++++---
 2 files changed, 20 insertions(+), 3 deletions(-)

Comments

Sean Christopherson Jan. 11, 2024, 4:45 p.m. UTC | #1 
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;
Pawan Gupta Jan. 12, 2024, 12:02 a.m. UTC | #2 
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.
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index b3b13ec04bac..139960deb736 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -161,6 +161,9 @@  SYM_FUNC_START(__vmx_vcpu_run)
 	/* Load guest RAX.  This kills the @regs pointer! */
 	mov VCPU_RAX(%_ASM_AX), %_ASM_AX
 
+	/* Clobbers EFLAGS.ZF */
+	CLEAR_CPU_BUFFERS
+
 	/* Check EFLAGS.CF from the VMX_RUN_VMRESUME bit test above. */
 	jnc .Lvmlaunch
 
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.
+	 */
+	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);
@@ -7226,11 +7237,14 @@  static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
 
 	guest_state_enter_irqoff();
 
-	/* L1D Flush includes CPU buffer clear to mitigate MDS */
+	/*
+	 * L1D Flush includes CPU buffer clear to mitigate MDS, but VERW
+	 * mitigation for MDS is done late in VMentry and is still
+	 * executed in spite of L1D Flush. This is because an extra VERW
+	 * should not matter much after the big hammer L1D Flush.
+	 */
 	if (static_branch_unlikely(&vmx_l1d_should_flush))
 		vmx_l1d_flush(vcpu);
-	else if (cpu_feature_enabled(X86_FEATURE_CLEAR_CPU_BUF))
-		mds_clear_cpu_buffers();
 	else if (static_branch_unlikely(&mmio_stale_data_clear) &&
 		 kvm_arch_has_assigned_device(vcpu->kvm))
 		mds_clear_cpu_buffers();