Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 28 Jan 2018 15:21:01 -0500
User-Agent: K-9 Mail for Android
In-Reply-To: <1517167750-23485-1-git-send-email-karahmed@amazon.de>
References: <1517167750-23485-1-git-send-email-karahmed@amazon.de>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PATCH] x86: vmx: Allow direct access to MSR_IA32_SPEC_CTRL
To:     KarimAllah Ahmed <karahmed@amazon.de>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
CC:     Asit Mallick <asit.k.mallick@intel.com>,
        Arjan Van De Ven <arjan.van.de.ven@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Andi Kleen <ak@linux.intel.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Dan Williams <dan.j.williams@intel.com>,
        Jun Nakajima <jun.nakajima@intel.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        David Woodhouse <dwmw@amazon.co.uk>,
        Greg KH <gregkh@linuxfoundation.org>,
        Andy Lutomirski <luto@kernel.org>,
        Ashok Raj <ashok.raj@intel.com>, daniel.kiper@oracle.com
From:   Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Message-ID: <4DCAF18F-C86A-4CBC-A9CC-CC01BF63313F@oracle.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On January 28, 2018 2:29:10 PM EST, KarimAllah Ahmed <karahmed@amazon=2Ede>=
 wrote:
>Add direct access to MSR_IA32_SPEC_CTRL for guests=2E This is needed for
>guests
>that will only mitigate Spectre V2 through IBRS+IBPB and will not be
>using a
>retpoline+IBPB based approach=2E
>
>To avoid the overhead of atomically saving and restoring the
>MSR_IA32_SPEC_CTRL
>for guests that do not actually use the MSR, only add_atomic_switch_msr
>when a
>non-zero is written to it=2E


We tried this and found that it was about 3% slower that doing the old way=
 of rdmsr and wrmsr=2E

But that was also with the host doing IBRS  as well=2E

On what type of hardware did you run this?

Ccing Daniel=2E
>
>Cc: Asit Mallick <asit=2Ek=2Emallick@intel=2Ecom>
>Cc: Arjan Van De Ven <arjan=2Evan=2Ede=2Even@intel=2Ecom>
>Cc: Dave Hansen <dave=2Ehansen@intel=2Ecom>
>Cc: Andi Kleen <ak@linux=2Eintel=2Ecom>
>Cc: Andrea Arcangeli <aarcange@redhat=2Ecom>
>Cc: Linus Torvalds <torvalds@linux-foundation=2Eorg>
>Cc: Tim Chen <tim=2Ec=2Echen@linux=2Eintel=2Ecom>
>Cc: Thomas Gleixner <tglx@linutronix=2Ede>
>Cc: Dan Williams <dan=2Ej=2Ewilliams@intel=2Ecom>
>Cc: Jun Nakajima <jun=2Enakajima@intel=2Ecom>
>Cc: Paolo Bonzini <pbonzini@redhat=2Ecom>
>Cc: David Woodhouse <dwmw@amazon=2Eco=2Euk>
>Cc: Greg KH <gregkh@linuxfoundation=2Eorg>
>Cc: Andy Lutomirski <luto@kernel=2Eorg>
>Signed-off-by: KarimAllah Ahmed <karahmed@amazon=2Ede>
>Signed-off-by: Ashok Raj <ashok=2Eraj@intel=2Ecom>
>---
> arch/x86/kvm/cpuid=2Ec |  4 +++-
> arch/x86/kvm/cpuid=2Eh |  1 +
>arch/x86/kvm/vmx=2Ec   | 63
>++++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 67 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/kvm/cpuid=2Ec b/arch/x86/kvm/cpuid=2Ec
>index 0099e10=2E=2Edc78095 100644
>--- a/arch/x86/kvm/cpuid=2Ec
>+++ b/arch/x86/kvm/cpuid=2Ec
>@@ -70,6 +70,7 @@ u64 kvm_supported_xcr0(void)
> /* These are scattered features in cpufeatures=2Eh=2E */
> #define KVM_CPUID_BIT_AVX512_4VNNIW     2
> #define KVM_CPUID_BIT_AVX512_4FMAPS     3
>+#define KVM_CPUID_BIT_SPEC_CTRL         26
> #define KF(x) bit(KVM_CPUID_BIT_##x)
>=20
> int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>@@ -392,7 +393,8 @@ static inline int __do_cpuid_ent(struct
>kvm_cpuid_entry2 *entry, u32 function,
>=20
> 	/* cpuid 7=2E0=2Eedx*/
> 	const u32 kvm_cpuid_7_0_edx_x86_features =3D
>-		KF(AVX512_4VNNIW) | KF(AVX512_4FMAPS);
>+		KF(AVX512_4VNNIW) | KF(AVX512_4FMAPS) | \
>+		(boot_cpu_has(X86_FEATURE_SPEC_CTRL) ? KF(SPEC_CTRL) : 0);
>=20
> 	/* all calls to cpuid_count() should be made on the same cpu */
> 	get_cpu();
>diff --git a/arch/x86/kvm/cpuid=2Eh b/arch/x86/kvm/cpuid=2Eh
>index cdc70a3=2E=2Edcfe227 100644
>--- a/arch/x86/kvm/cpuid=2Eh
>+++ b/arch/x86/kvm/cpuid=2Eh
>@@ -54,6 +54,7 @@ static const struct cpuid_reg reverse_cpuid[] =3D {
> 	[CPUID_8000_000A_EDX] =3D {0x8000000a, 0, CPUID_EDX},
> 	[CPUID_7_ECX]         =3D {         7, 0, CPUID_ECX},
> 	[CPUID_8000_0007_EBX] =3D {0x80000007, 0, CPUID_EBX},
>+	[CPUID_7_EDX]         =3D {         7, 0, CPUID_EDX},
> };
>=20
>static __always_inline struct cpuid_reg x86_feature_cpuid(unsigned
>x86_feature)
>diff --git a/arch/x86/kvm/vmx=2Ec b/arch/x86/kvm/vmx=2Ec
>index aa8638a=2E=2E1b743a0 100644
>--- a/arch/x86/kvm/vmx=2Ec
>+++ b/arch/x86/kvm/vmx=2Ec
>@@ -920,6 +920,9 @@ static void vmx_set_nmi_mask(struct kvm_vcpu *vcpu,
>bool masked);
> static bool nested_vmx_is_page_fault_vmexit(struct vmcs12 *vmcs12,
> 					    u16 error_code);
> static void vmx_update_msr_bitmap(struct kvm_vcpu *vcpu);
>+static void __always_inline vmx_disable_intercept_for_msr(unsigned
>long *msr_bitmap,
>+							  u32 msr, int type);
>+
>=20
> static DEFINE_PER_CPU(struct vmcs *, vmxarea);
> static DEFINE_PER_CPU(struct vmcs *, current_vmcs);
>@@ -2007,6 +2010,28 @@ static void add_atomic_switch_msr(struct
>vcpu_vmx *vmx, unsigned msr,
> 	m->host[i]=2Evalue =3D host_val;
> }
>=20
>+/* do not touch guest_val and host_val if the msr is not found */
>+static int read_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
>+				  u64 *guest_val, u64 *host_val)
>+{
>+	unsigned i;
>+	struct msr_autoload *m =3D &vmx->msr_autoload;
>+
>+	for (i =3D 0; i < m->nr; ++i)
>+		if (m->guest[i]=2Eindex =3D=3D msr)
>+			break;
>+
>+	if (i =3D=3D m->nr)
>+		return 1;
>+
>+	if (guest_val)
>+		*guest_val =3D m->guest[i]=2Evalue;
>+	if (host_val)
>+		*host_val =3D m->host[i]=2Evalue;
>+
>+	return 0;
>+}
>+
>static bool update_transition_efer(struct vcpu_vmx *vmx, int
>efer_offset)
> {
> 	u64 guest_efer =3D vmx->vcpu=2Earch=2Eefer;
>@@ -3203,7 +3228,9 @@ static inline bool
>vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu,
>  */
>static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data
>*msr_info)
> {
>+	u64 spec_ctrl =3D 0;
> 	struct shared_msr_entry *msr;
>+	struct vcpu_vmx *vmx =3D to_vmx(vcpu);
>=20
> 	switch (msr_info->index) {
> #ifdef CONFIG_X86_64
>@@ -3223,6 +3250,19 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu,
>struct msr_data *msr_info)
> 	case MSR_IA32_TSC:
> 		msr_info->data =3D guest_read_tsc(vcpu);
> 		break;
>+	case MSR_IA32_SPEC_CTRL:
>+		if (!msr_info->host_initiated &&
>+		    !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
>+			return 1;
>+
>+		/*
>+		 * If the MSR is not in the atomic list yet, then it was never
>+		 * written to=2E So the MSR value will be '0'=2E
>+		 */
>+		read_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL, &spec_ctrl, NULL);
>+
>+		msr_info->data =3D spec_ctrl;
>+		break;
> 	case MSR_IA32_SYSENTER_CS:
> 		msr_info->data =3D vmcs_read32(GUEST_SYSENTER_CS);
> 		break;
>@@ -3289,6 +3329,13 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu,
>struct msr_data *msr_info)
> 	int ret =3D 0;
> 	u32 msr_index =3D msr_info->index;
> 	u64 data =3D msr_info->data;
>+	unsigned long *msr_bitmap;
>+
>+	/*
>+	 * IBRS is not used (yet) to protect the host=2E Once it does, this
>+	 * variable needs to be a bit smarter=2E
>+	 */
>+	u64 host_spec_ctrl =3D 0;
>=20
> 	switch (msr_index) {
> 	case MSR_EFER:
>@@ -3330,6 +3377,22 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu,
>struct msr_data *msr_info)
> 	case MSR_IA32_TSC:
> 		kvm_write_tsc(vcpu, msr_info);
> 		break;
>+	case MSR_IA32_SPEC_CTRL:
>+		if (!msr_info->host_initiated &&
>+		    !guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
>+			return 1;
>+
>+		/*
>+		 * Now we know that the guest is actually using the MSR, so
>+		 * atomically load and save the SPEC_CTRL MSR and pass it
>+		 * through to the guest=2E
>+		 */
>+		add_atomic_switch_msr(vmx, MSR_IA32_SPEC_CTRL, msr_info->data,
>+				      host_spec_ctrl);
>+		msr_bitmap =3D vmx->vmcs01=2Emsr_bitmap;
>+		vmx_disable_intercept_for_msr(msr_bitmap, MSR_FS_BASE, MSR_TYPE_RW);
>+
>+		break;
> 	case MSR_IA32_CR_PAT:
> 		if (vmcs_config=2Evmentry_ctrl & VM_ENTRY_LOAD_IA32_PAT) {
> 			if (!kvm_mtrr_valid(vcpu, MSR_IA32_CR_PAT, data))