[MODERATED] Re: [patch V11 05/16] SSB 5

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, May 10, 2018 at 03:25:33PM -0700, speck for Andi Kleen wrote:
> > It is actually slower. I tried doing it last time with the spectre/meltdown
> > and the performance was way slower than doing it this way. I can dig up the patches
> >  - as I think we did the tests on Broadwell but hadn't tried Skylake or such
> > (or maybe it was the other way around).
> 
> Was this with MSR lists unconditionally, or with MSR list combined with 
> the "wait for the first write" approach?

It was the unconditional. The patch went through a bunch of iterations and this
is what we ended up testing - but it showed around 2-3% performance degradation
when doing TPCC-like workloads. I am trying to track down exactly what hardware
that was done against.

It won't apply to 'master' as this was against our heavily backported 4.1 kernel
but you get the idea. Also ignore some of the commit description as we did
eventually figure out the 'very fragile' case.


From: Daniel Kiper <daniel.kiper@oracle.com>

x86/kvm: Initialize guest MSR_IA32_SPEC_CTRL from VMCS VM-entry MSR load area

Current solution, wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl) before VMLAUNCH,
is very fragile and it may not restore MSR_IA32_SPEC_CTRL guest value in some
cases, e.g. during rescheduling processes (it looks that it is separate bug which
have to be fixed; anyway...). So, let's initialize guest MSR_IA32_SPEC_CTRL from
VMCS VM-entry MSR load area which is designed to do such things. Additionally,
save guest MSR_IA32_SPEC_CTRL into VM-exit MSR store area before VM-exit instead
of rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl) after VM-exit. Later may not be
reliable too.

This patch replaces commit bc5d49f (x86/spec: Always set IBRS to guest value on
VMENTER and host on VMEXIT) and 51b5f53 (x86/spec: Always set IBRS to guest value
on VMENTER and host on VMEXIT (redux)).

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>

---
 arch/x86/kvm/vmx.c |   56 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index aa9bc4f..e7c0f8b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -162,8 +162,10 @@ module_param(ple_window_max, int, S_IRUGO);
 
 extern const ulong vmx_return;
 
-#define NR_AUTOLOAD_MSRS 8
-#define VMCS02_POOL_SIZE 1
+#define NR_AUTOLOAD_MSRS	8
+#define NR_AUTOSTORE_MSRS	NR_AUTOLOAD_MSRS
+
+#define VMCS02_POOL_SIZE	1
 
 struct vmcs {
 	u32 revision_id;
@@ -504,6 +506,10 @@ struct vcpu_vmx {
 		struct vmx_msr_entry guest[NR_AUTOLOAD_MSRS];
 		struct vmx_msr_entry host[NR_AUTOLOAD_MSRS];
 	} msr_autoload;
+	struct msr_autostore {
+		unsigned nr;
+		struct vmx_msr_entry guest[NR_AUTOSTORE_MSRS];
+	} msr_autostore;
 	struct {
 		int           loaded;
 		u16           fs_sel, gs_sel, ldt_sel;
@@ -1704,6 +1710,37 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
 	m->host[i].value = host_val;
 }
 
+static void add_atomic_store_msr(struct vcpu_vmx *vmx, unsigned msr)
+{
+	unsigned i;
+	struct msr_autostore *m = &vmx->msr_autostore;
+
+	for (i = 0; i < m->nr; ++i)
+		if (m->guest[i].index == msr)
+			return;
+
+	if (i == NR_AUTOSTORE_MSRS) {
+		pr_err("Not enough msr store entries. Can't add msr %x\n", msr);
+		BUG();
+	}
+
+	m->guest[i].index = msr;
+	vmcs_write32(VM_EXIT_MSR_STORE_COUNT, ++m->nr);
+}
+
+static u64 get_msr_vmcs_store(struct vcpu_vmx *vmx, unsigned msr)
+{
+	unsigned i;
+	struct msr_autostore *m = &vmx->msr_autostore;
+
+	for (i = 0; i < m->nr; ++i)
+		if (m->guest[i].index == msr)
+			return m->guest[i].value;
+
+	pr_err("Can't find msr %x in VMCS store\n", msr);
+	BUG();
+}
+
 static void reload_tss(void)
 {
 	/*
@@ -4716,6 +4753,7 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
 #endif
 
 	vmcs_write32(VM_EXIT_MSR_STORE_COUNT, 0);
+	vmcs_write64(VM_EXIT_MSR_STORE_ADDR, __pa(vmx->msr_autostore.guest));
 	vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, 0);
 	vmcs_write64(VM_EXIT_MSR_LOAD_ADDR, __pa(vmx->msr_autoload.host));
 	vmcs_write32(VM_ENTRY_MSR_LOAD_COUNT, 0);
@@ -8192,8 +8230,11 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 
 	vmx->__launched = vmx->loaded_vmcs->launched;
 
-	if (ibrs_inuse)
-		wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
+	if (ibrs_inuse) {
+		add_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL, vmx->spec_ctrl,
+				      SPEC_CTRL_FEATURE_ENABLE_IBRS);
+		add_atomic_store_msr(vmx, MSR_IA32_SPEC_CTRL);
+	}
 
 	asm(
 		/* Store host registers */
@@ -8317,12 +8358,11 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 #endif
 	      );
 
-	if (ibrs_inuse) {
-		rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
-		wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
-	}
 	stuff_RSB();
 
+	if (ibrs_inuse)
+		vmx->spec_ctrl = get_msr_vmcs_store(vmx, MSR_IA32_SPEC_CTRL);
+
 	/* MSR_IA32_DEBUGCTLMSR is zeroed on vmexit. Restore it if needed */
 	if (debugctlmsr)
 		update_debugctlmsr(debugctlmsr);
-- 
1.7.10.4