From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: AH8x2241g5PEsVxNI9yUxUIIsKXnsIEK7/vFCUrBclYH90o4mFRkN4dQVqtedH5DI5d7GoX9Ywxp ARC-Seal: i=1; a=rsa-sha256; t=1517262695; cv=none; d=google.com; s=arc-20160816; b=JBnhPS0J3paY5o1SS/wJCkwK5qtz76Bne6IkEeuFgbgHXeuwKRHZ1gBR8T6I+bv8C0 cQq2mI2qNWr4Z4hpjo00+VS80eu69jFif6IYtOhONoYpupY8JBKk2qiw74S/MFKO8vYe K3elNPr1zbOJjflhN0WNyb89VcWiFN6jokNs9352xcMJ5wTlv03hlwDJxA0fYfO1pt6M tGjD8fV8PGMFGX98ubj2PDlPYHIZwBjvTf9wezMupQJsfbmUwreELX2suZs47/q6cHx2 dPKogkE/TFHv9Hx68K5r6q1oAp+/KZiFniWyKajp/juvsQ3YG77zAZXPkGMu3wasuCtF vcQQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:dkim-signature :arc-authentication-results; bh=oVL7HqjRGeUWaY+Mo900ryXXrOmwUdops4W1o57vjcg=; b=MY0/etI8y7uAorAoT0rpePjx0TngQ7708gf0fweDZtUzp66Zj4/BqUDS4v8dYbdLOK aBqNU2GEsJcSCoMP+cAI48gtFUszl4eW/uGQr+ms0z8chQAHG+x048G/ndyoM1xwZ2s6 8Es9DReKW6a2W1uwiWDQL57TIEbA7/UVkk+NQrfDc8J8hrT3HhKKlgEdk92dC7m7MG0Y eSL7QL6wVm7crDrWlH0euQQmcJUzb1bapzOTws1m0NTWwXa2/8F2U6Jc87XkMGra9BSn MOlVBpEQyrhD+k8OW66oyKUwVeLc1KSH+k1BgmfWSt8g6YKzOSINLIbdBQ0U1KVoo3if CNUw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@oracle.com header.s=corp-2017-10-26 header.b=oC/u1+2U; spf=pass (google.com: domain of daniel.kiper@oracle.com designates 156.151.31.85 as permitted sender) smtp.mailfrom=daniel.kiper@oracle.com; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=oracle.com Authentication-Results: mx.google.com; dkim=pass header.i=@oracle.com header.s=corp-2017-10-26 header.b=oC/u1+2U; spf=pass (google.com: domain of daniel.kiper@oracle.com designates 156.151.31.85 as permitted sender) smtp.mailfrom=daniel.kiper@oracle.com; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=oracle.com Date: Mon, 29 Jan 2018 22:49:52 +0100 From: Daniel Kiper To: Konrad Rzeszutek Wilk Cc: David Woodhouse , Mihai Carabas , Liran Alon , luto@kernel.org, tglx@linutronix.de, torvalds@linux-foundation.org, gregkh@linuxfoundation.org, asit.k.mallick@intel.com, dave.hansen@intel.com, karahmed@amazon.de, jun.nakajima@intel.com, dan.j.williams@intel.com, ashok.raj@intel.com, arjan.van.de.ven@intel.com, tim.c.chen@linux.intel.com, pbonzini@redhat.com, linux-kernel@vger.kernel.org, ak@linux.intel.com, kvm@vger.kernel.org, aarcange@redhat.com Subject: Re: [PATCH] x86: vmx: Allow direct access to MSR_IA32_SPEC_CTRL Message-ID: <20180129214952.GI16863@olila.local.net-space.pl> References: <6b9a1ec2-5ebd-4624-a825-3f31db5cefb5@default> <1517215563.6624.118.camel@infradead.org> <20180129173113.GO22045@char.us.oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180129173113.GO22045@char.us.oracle.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=8789 signatures=668655 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1801290278 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1590865729989295970?= X-GMAIL-MSGID: =?utf-8?q?1590965248204305380?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: On Mon, Jan 29, 2018 at 12:31:13PM -0500, Konrad Rzeszutek Wilk wrote: > On Mon, Jan 29, 2018 at 08:46:03AM +0000, David Woodhouse wrote: > > On Sun, 2018-01-28 at 16:39 -0800, Liran Alon wrote: > > > > > > Windows use IBRS and Microsoft don't have any plans to switch to retpoline. > > > Running a Windows guest should be a pretty common use-case no? > > > > > > In addition, your handle of the first WRMSR intercept could be different. > > > It could signal you to start doing the following: > > > 1. Disable intercept on SPEC_CTRL MSR. > > > 2. On VMEntry, Write vCPU SPEC_CTRL value into physical MSR. > > > 3. On VMExit, read physical MSR into vCPU SPEC_CTRL value. > > > (And if IBRS is used at host, also set physical SPEC_CTRL MSR here to 1) > > > > > > That way, you will both have fastest option as long as guest don't use IBRS > > > and also won't have the 3% performance hit compared to Konrad's proposal. > > > > > > Am I missing something? > > > > Reads from the SPEC_CTRL MSR are strangely slow. I suspect a large part > > of the 3% speedup you observe is because in the above, the vmentry path > > doesn't need to *read* the host's value and store it; the host is > > expected to restore it for itself anyway? > > Yes for at least the purpose of correctness. That is based on what I have > heard is that you when you transition to a higher ring you have to write 1, then write zero > when you transition back to lower rings. That is it is like a knob. > > But then I heard that on some CPUs it is more like reset button and > just writting 1 to IBRS is fine. But again, correctness here. > > > > > I'd actually quite like to repeat the benchmark on the new fixed > > microcode, if anyone has it yet, to see if that read/swap slowness is > > still quite as excessive. I'm certainly not ruling this out, but I'm > > just a little wary of premature optimisation, and I'd like to make sure > > we have everything *else* in the KVM patches right first. > > > > The fact that the save-and-restrict macros I have in the tip of my > > working tree at the moment are horrid and causing 0-day nastygrams, > > probably doesn't help persuade me to favour the approach ;) > > > > ... hm, the CPU actually has separate MSR save/restore lists for > > entry/exit, doesn't it? Is there any way to sanely make use of that and > > do the restoration manually on vmentry but let it be automatic on > > vmexit, by having it *only* in the guest's MSR-store area to be saved > > on exit and restored on exit, but *not* in the host's MSR-store area? s/on exit and restored on exit/on exit and restored on entry/? Additionally, AIUI there is no "host's MSR-store area". > Oh. That sounds sounds interesting That is possible but you have to split add_atomic_switch_msr() accordingly. > > Reading the code and comparing with the SDM, I can't see where we're > > ever setting VM_EXIT_MSR_STORE_{ADDR,COUNT} except in the nested > > case... > > Right. We (well Daniel Kiper, CC-ed) implemented it for this and > that is where we got the numbers. > > Daniel, you OK posting it here? Granted with the caveats thta it won't even > compile against upstream as it was based on a distro kernel. Please look below... 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); By the way, I saw in one version of patches that you use own rdmsrl()/wrmsrl() functions to save/restore IBRS instead of using common ones (I mean native_rdmsrl()/native_wrmsrl(), safe__rdmsrl()/safe_wrmsrl(), etc.) available in the kernel. Could you explain why do you do that? Daniel