From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from userp2130.oracle.com ([156.151.31.86]) by Galois.linutronix.de with esmtps (TLS1.2:RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1fGvKz-0006S9-Bq for speck@linutronix.de; Fri, 11 May 2018 01:51:06 +0200 Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w4ANkKgm051843 for ; Thu, 10 May 2018 23:50:58 GMT Received: from userv0021.oracle.com (userv0021.oracle.com [156.151.31.71]) by userp2130.oracle.com with ESMTP id 2hvth91jaf-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Thu, 10 May 2018 23:50:58 +0000 Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by userv0021.oracle.com (8.14.4/8.14.4) with ESMTP id w4ANovOC007238 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Thu, 10 May 2018 23:50:58 GMT Received: from abhmp0001.oracle.com (abhmp0001.oracle.com [141.146.116.7]) by aserv0122.oracle.com (8.14.4/8.14.4) with ESMTP id w4ANovYs018416 for ; Thu, 10 May 2018 23:50:57 GMT Date: Thu, 10 May 2018 19:50:56 -0400 From: Konrad Rzeszutek Wilk Subject: [MODERATED] Re: [patch V11 05/16] SSB 5 Message-ID: <20180510235056.GA27882@char.us.oracle.com> References: <20180502215102.192655950@linutronix.de> <20180502215416.459915781@linutronix.de> <20180510175257.GD13616@tassilo.jf.intel.com> <20180510183058.GJ27358@char.us.oracle.com> <20180510190850.GE13616@tassilo.jf.intel.com> <20180510212232.GT27358@char.us.oracle.com> <20180510222533.GH13616@tassilo.jf.intel.com> MIME-Version: 1.0 In-Reply-To: <20180510222533.GH13616@tassilo.jf.intel.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: 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 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 --- 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