Re: [PATCH 8/9] KVM/VMX: Allow direct access to MSR_IA32_SPEC_CTRL

2018-02-15 Thread Jim Mattson
On Tue, Feb 6, 2018 at 9:29 AM, David Woodhouse  wrote:

> @@ -8946,6 +9017,27 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
> *vcpu)
>  #endif
>   );
>
> +   /*
> +* We do not use IBRS in the kernel. If this vCPU has used the
> +* SPEC_CTRL MSR it may have left it on; save the value and
> +* turn it off. This is much more efficient than blindly adding
> +* it to the atomic save/restore list. Especially as the former
> +* (Saving guest MSRs on vmexit) doesn't even exist in KVM.
> +*
> +* For non-nested case:
> +* If the L01 MSR bitmap does not intercept the MSR, then we need to
> +* save it.
> +*
> +* For nested case:
> +* If the L02 MSR bitmap does not intercept the MSR, then we need to
> +* save it.
> +*/
> +   if (!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL))
> +   rdmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +
> +   if (vmx->spec_ctrl)
> +   wrmsrl(MSR_IA32_SPEC_CTRL, 0);
> +

Again, we haven't verified host support for this MSR. Perhaps this
should be something like:

if (!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL)
&& !rdmsrl_safe(MSR_IA32_SPEC_CTRL, &vmx->spec_ctrl) && vmx->spec_ctrl)
wrmsrl(MSR_IA32_SPEC_CTRL, 0);


Re: [PATCH 8/9] KVM/VMX: Allow direct access to MSR_IA32_SPEC_CTRL

2018-02-15 Thread Jim Mattson
On Tue, Feb 6, 2018 at 9:29 AM, David Woodhouse  wrote:

> @@ -8828,6 +8890,15 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu 
> *vcpu)
>
> vmx_arm_hv_timer(vcpu);
>
> +   /*
> +* If this vCPU has touched SPEC_CTRL, restore the guest's value if
> +* it's non-zero. Since vmentry is serialising on affected CPUs, there
> +* is no need to worry about the conditional branch over the wrmsr
> +* being speculatively taken.
> +*/
> +   if (vmx->spec_ctrl)
> +   wrmsrl(MSR_IA32_SPEC_CTRL, vmx->spec_ctrl);
> +

Shouldn't this be wrmsrl_safe? Userspace can make an ioctl to set
vmx->spec_ctrl to non-zero even if the MSR is not supported on the
host.


[PATCH 8/9] KVM/VMX: Allow direct access to MSR_IA32_SPEC_CTRL

2018-02-06 Thread David Woodhouse
From: KarimAllah Ahmed 

[ Based on a patch from Ashok Raj  ]

Add direct access to MSR_IA32_SPEC_CTRL for guests. This is needed for
guests that will only mitigate Spectre V2 through IBRS+IBPB and will not
be using a retpoline+IBPB based approach.

To avoid the overhead of saving and restoring the MSR_IA32_SPEC_CTRL for
guests that do not actually use the MSR, only start saving and restoring
when a non-zero is written to it.

No attempt is made to handle STIBP here, intentionally. Filtering STIBP
may be added in a future patch, which may require trapping all writes
if we don't want to pass it through directly to the guest.

[dwmw2: Clean up CPUID bits, save/restore manually, handle reset]

Signed-off-by: KarimAllah Ahmed 
Signed-off-by: David Woodhouse 
Signed-off-by: Thomas Gleixner 
Reviewed-by: Darren Kenny 
Reviewed-by: Konrad Rzeszutek Wilk 
Reviewed-by: Jim Mattson 
Cc: Andrea Arcangeli 
Cc: Andi Kleen 
Cc: Jun Nakajima 
Cc: k...@vger.kernel.org
Cc: Dave Hansen 
Cc: Tim Chen 
Cc: Andy Lutomirski 
Cc: Asit Mallick 
Cc: Arjan Van De Ven 
Cc: Greg KH 
Cc: Paolo Bonzini 
Cc: Dan Williams 
Cc: Linus Torvalds 
Cc: Ashok Raj 
Link: 
https://lkml.kernel.org/r/1517522386-18410-5-git-send-email-karah...@amazon.de

(cherry picked from commit d28b387fb74da95d69d2615732f50cceb38e9a4d)
Signed-off-by: David Woodhouse 
---
 arch/x86/kvm/cpuid.c |   8 ++--
 arch/x86/kvm/cpuid.h |  11 ++
 arch/x86/kvm/vmx.c   | 103 ++-
 arch/x86/kvm/x86.c   |   2 +-
 4 files changed, 118 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 9c6493f..93f924d 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -357,7 +357,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 
*entry, u32 function,
 
/* cpuid 0x8008.ebx */
const u32 kvm_cpuid_8000_0008_ebx_x86_features =
-   F(IBPB);
+   F(IBPB) | F(IBRS);
 
/* cpuid 0xC001.edx */
const u32 kvm_cpuid_C000_0001_edx_x86_features =
@@ -382,7 +382,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 
*entry, u32 function,
 
/* cpuid 7.0.edx*/
const u32 kvm_cpuid_7_0_edx_x86_features =
-   F(ARCH_CAPABILITIES);
+   F(SPEC_CTRL) | F(ARCH_CAPABILITIES);
 
/* all calls to cpuid_count() should be made on the same cpu */
get_cpu();
@@ -618,9 +618,11 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 
*entry, u32 function,
g_phys_as = phys_as;
entry->eax = g_phys_as | (virt_as << 8);
entry->edx = 0;
-   /* IBPB isn't necessarily present in hardware cpuid */
+   /* IBRS and IBPB aren't necessarily present in hardware cpuid */
if (boot_cpu_has(X86_FEATURE_IBPB))
entry->ebx |= F(IBPB);
+   if (boot_cpu_has(X86_FEATURE_IBRS))
+   entry->ebx |= F(IBRS);
entry->ebx &= kvm_cpuid_8000_0008_ebx_x86_features;
cpuid_mask(&entry->ebx, CPUID_8000_0008_EBX);
break;
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 8719997..d1beb71 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -171,6 +171,17 @@ static inline bool guest_cpuid_has_ibpb(struct kvm_vcpu 
*vcpu)
return best && (best->edx & bit(X86_FEATURE_SPEC_CTRL));
 }
 
+static inline bool guest_cpuid_has_ibrs(struct kvm_vcpu *vcpu)
+{
+   struct kvm_cpuid_entry2 *best;
+
+   best = kvm_find_cpuid_entry(vcpu, 0x8008, 0);
+   if (best && (best->ebx & bit(X86_FEATURE_IBRS)))
+   return true;
+   best = kvm_find_cpuid_entry(vcpu, 7, 0);
+   return best && (best->edx & bit(X86_FEATURE_SPEC_CTRL));
+}
+
 static inline bool guest_cpuid_has_arch_capabilities(struct kvm_vcpu *vcpu)
 {
struct kvm_cpuid_entry2 *best;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 92bf61f..764cb7c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -552,6 +552,7 @@ struct vcpu_vmx {
 #endif
 
u64   arch_capabilities;
+   u64   spec_ctrl;
 
u32 vm_entry_controls_shadow;
u32 vm_exit_controls_shadow;
@@ -1852,6 +1853,29 @@ static void update_exception_bitmap(struct kvm_vcpu 
*vcpu)
 }
 
 /*
+ * Check if MSR is intercepted for currently loaded MSR bitmap.
+ */
+static bool msr_write_intercepted(struct kvm_vcpu *vcpu, u32 msr)
+{
+   unsigned long *msr_bitmap;
+   int f = sizeof(unsigned long);
+
+   if (!cpu_has_vmx_msr_bitmap())
+   return true;
+
+   msr_bitmap = to_vmx(vcpu)->loaded_vmcs->msr_bitmap;
+
+   if (msr <= 0x1fff) {
+   return !!test_bit(msr, msr_bitmap + 0x800 / f);
+   } else if ((msr >= 0xc000) && (msr <= 0xc0001fff)) {
+   msr &= 0x1fff;
+   return !!test_bit(msr, msr_bitmap + 0xc00 / f);
+   }
+
+