The VM entry/exit path is a giant inline assembly statement. Simplify it
by doing CR2 context switching in plain C. Move CR2 restore behind IBRS
clearing, so we reduce the amount of code we execute with IBRS on.
Signed-off-by: Julian Stecklina <[email protected]>
Reviewed-by: Jan H. Schönherr <[email protected]>
Reviewed-by: Konrad Jan Miller <[email protected]>
Reviewed-by: Jim Mattson <[email protected]>
---
arch/x86/kvm/vmx.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index ccc6a01..a6e5a5c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11212,6 +11212,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
evmcs_rsp = static_branch_unlikely(&enable_evmcs) ?
(unsigned long)¤t_evmcs->host_rsp : 0;
+ if (read_cr2() != vcpu->arch.cr2)
+ write_cr2(vcpu->arch.cr2);
+
if (static_branch_unlikely(&vmx_l1d_should_flush))
vmx_l1d_flush(vcpu);
@@ -11231,13 +11234,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
"2: \n\t"
__ex("vmwrite %%" _ASM_SP ", %%" _ASM_DX) "\n\t"
"1: \n\t"
- /* Reload cr2 if changed */
- "mov %c[cr2](%0), %%" _ASM_AX " \n\t"
- "mov %%cr2, %%" _ASM_DX " \n\t"
- "cmp %%" _ASM_AX ", %%" _ASM_DX " \n\t"
- "je 3f \n\t"
- "mov %%" _ASM_AX", %%cr2 \n\t"
- "3: \n\t"
/* Check if vmlaunch of vmresume is needed */
"cmpl $0, %c[launched](%0) \n\t"
/* Load guest registers. Don't clobber flags. */
@@ -11298,8 +11294,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
"xor %%r14d, %%r14d \n\t"
"xor %%r15d, %%r15d \n\t"
#endif
- "mov %%cr2, %%" _ASM_AX " \n\t"
- "mov %%" _ASM_AX ", %c[cr2](%0) \n\t"
"xor %%eax, %%eax \n\t"
"xor %%ebx, %%ebx \n\t"
@@ -11331,7 +11325,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
[r14]"i"(offsetof(struct vcpu_vmx, vcpu.arch.regs[VCPU_REGS_R14])),
[r15]"i"(offsetof(struct vcpu_vmx, vcpu.arch.regs[VCPU_REGS_R15])),
#endif
- [cr2]"i"(offsetof(struct vcpu_vmx, vcpu.arch.cr2)),
[wordsize]"i"(sizeof(ulong))
: "cc", "memory"
#ifdef CONFIG_X86_64
@@ -11365,6 +11358,8 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
/* Eliminate branch target predictions from guest mode */
vmexit_fill_RSB();
+ vcpu->arch.cr2 = read_cr2();
+
/* All fields are clean at this point */
if (static_branch_unlikely(&enable_evmcs))
current_evmcs->hv_clean_fields |=
--
2.7.4
Split the security related register clearing out of the large inline
assembly VM entry path. This results in two slightly less complicated
inline assembly statements, where it is clearer what each one does.
Signed-off-by: Julian Stecklina <[email protected]>
Reviewed-by: Jan H. Schönherr <[email protected]>
Reviewed-by: Konrad Jan Miller <[email protected]>
Reviewed-by: Jim Mattson <[email protected]>
---
arch/x86/kvm/vmx.c | 45 ++++++++++++++++++++++++++++-----------------
1 file changed, 28 insertions(+), 17 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index a6e5a5c..29a2ee7 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11281,24 +11281,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
"mov %%r13, %c[r13](%0) \n\t"
"mov %%r14, %c[r14](%0) \n\t"
"mov %%r15, %c[r15](%0) \n\t"
- /*
- * Clear host registers marked as clobbered to prevent
- * speculative use.
- */
- "xor %%r8d, %%r8d \n\t"
- "xor %%r9d, %%r9d \n\t"
- "xor %%r10d, %%r10d \n\t"
- "xor %%r11d, %%r11d \n\t"
- "xor %%r12d, %%r12d \n\t"
- "xor %%r13d, %%r13d \n\t"
- "xor %%r14d, %%r14d \n\t"
- "xor %%r15d, %%r15d \n\t"
#endif
-
- "xor %%eax, %%eax \n\t"
- "xor %%ebx, %%ebx \n\t"
- "xor %%esi, %%esi \n\t"
- "xor %%edi, %%edi \n\t"
"pop %%" _ASM_BP "; pop %%" _ASM_DX " \n\t"
".pushsection .rodata \n\t"
".global vmx_return \n\t"
@@ -11336,6 +11319,34 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
);
/*
+ * Don't let guest register values survive. Registers that cannot
+ * contain guest values anymore are not touched.
+ */
+ asm volatile (
+ "xor %%eax, %%eax \n\t"
+ "xor %%ebx, %%ebx \n\t"
+ "xor %%esi, %%esi \n\t"
+ "xor %%edi, %%edi \n\t"
+#ifdef CONFIG_X86_64
+ "xor %%r8d, %%r8d \n\t"
+ "xor %%r9d, %%r9d \n\t"
+ "xor %%r10d, %%r10d \n\t"
+ "xor %%r11d, %%r11d \n\t"
+ "xor %%r12d, %%r12d \n\t"
+ "xor %%r13d, %%r13d \n\t"
+ "xor %%r14d, %%r14d \n\t"
+ "xor %%r15d, %%r15d \n\t"
+#endif
+ ::: "cc"
+#ifdef CONFIG_X86_64
+ , "rax", "rbx", "rsi", "rdi"
+ , "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15"
+#else
+ , "eax", "ebx", "esi", "edi"
+#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
--
2.7.4
The code violated the coding style. Fixed by using tabs instead of
spaces. There are only whitespace changes here.
Signed-off-by: Julian Stecklina <[email protected]>
Reviewed-by: Jan H. Schönherr <[email protected]>
Reviewed-by: Konrad Jan Miller <[email protected]>
---
arch/x86/kvm/vmx.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 29a2ee7..9bd7c6c 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2100,16 +2100,16 @@ static int __find_msr_index(struct vcpu_vmx *vmx, u32 msr)
static inline void __invvpid(unsigned long ext, u16 vpid, gva_t gva)
{
- struct {
- u64 vpid : 16;
- u64 rsvd : 48;
- u64 gva;
- } operand = { vpid, 0, gva };
- bool error;
-
- asm volatile (__ex("invvpid %2, %1") CC_SET(na)
- : CC_OUT(na) (error) : "r"(ext), "m"(operand));
- BUG_ON(error);
+ struct {
+ u64 vpid : 16;
+ u64 rsvd : 48;
+ u64 gva;
+ } operand = { vpid, 0, gva };
+ bool error;
+
+ asm volatile (__ex("invvpid %2, %1") CC_SET(na)
+ : CC_OUT(na) (error) : "r"(ext), "m"(operand));
+ BUG_ON(error);
}
static inline void __invept(unsigned long ext, u64 eptp, gpa_t gpa)
--
2.7.4
On Mon, Oct 29, 2018 at 04:40:42PM +0100, Julian Stecklina wrote:
> The VM entry/exit path is a giant inline assembly statement. Simplify it
> by doing CR2 context switching in plain C. Move CR2 restore behind IBRS
> clearing, so we reduce the amount of code we execute with IBRS on.
I think it's worth documenting two things in the changelog:
- Using {read,write}_cr2() means KVM will use pv_mmu_ops instead of
open coding native_{read,write}_cr2().
- The CR2 code has been done in assembly since KVM's genesis[1],
which predates the addition of the paravirt ops[2], i.e. KVM isn't
deliberately avoiding the paravirt ops.
The above info makes it trivially easy to review this patch from a
correctness standpoint. With that:
Reviewed-by: Sean Christopherson <[email protected]>
[1] Commit 6aa8b732ca01 ("[PATCH] kvm: userspace interface")
[2] Commit d3561b7fa0fb ("[PATCH] paravirt: header and stubs for paravirtualisation")
> Signed-off-by: Julian Stecklina <[email protected]>
> Reviewed-by: Jan H. Sch?nherr <[email protected]>
> Reviewed-by: Konrad Jan Miller <[email protected]>
> Reviewed-by: Jim Mattson <[email protected]>
> ---
> arch/x86/kvm/vmx.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ccc6a01..a6e5a5c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11212,6 +11212,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> evmcs_rsp = static_branch_unlikely(&enable_evmcs) ?
> (unsigned long)¤t_evmcs->host_rsp : 0;
>
> + if (read_cr2() != vcpu->arch.cr2)
> + write_cr2(vcpu->arch.cr2);
> +
> if (static_branch_unlikely(&vmx_l1d_should_flush))
> vmx_l1d_flush(vcpu);
>
> @@ -11231,13 +11234,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> "2: \n\t"
> __ex("vmwrite %%" _ASM_SP ", %%" _ASM_DX) "\n\t"
> "1: \n\t"
> - /* Reload cr2 if changed */
> - "mov %c[cr2](%0), %%" _ASM_AX " \n\t"
> - "mov %%cr2, %%" _ASM_DX " \n\t"
> - "cmp %%" _ASM_AX ", %%" _ASM_DX " \n\t"
> - "je 3f \n\t"
> - "mov %%" _ASM_AX", %%cr2 \n\t"
> - "3: \n\t"
> /* Check if vmlaunch of vmresume is needed */
> "cmpl $0, %c[launched](%0) \n\t"
> /* Load guest registers. Don't clobber flags. */
> @@ -11298,8 +11294,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> "xor %%r14d, %%r14d \n\t"
> "xor %%r15d, %%r15d \n\t"
> #endif
> - "mov %%cr2, %%" _ASM_AX " \n\t"
> - "mov %%" _ASM_AX ", %c[cr2](%0) \n\t"
>
> "xor %%eax, %%eax \n\t"
> "xor %%ebx, %%ebx \n\t"
> @@ -11331,7 +11325,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> [r14]"i"(offsetof(struct vcpu_vmx, vcpu.arch.regs[VCPU_REGS_R14])),
> [r15]"i"(offsetof(struct vcpu_vmx, vcpu.arch.regs[VCPU_REGS_R15])),
> #endif
> - [cr2]"i"(offsetof(struct vcpu_vmx, vcpu.arch.cr2)),
> [wordsize]"i"(sizeof(ulong))
> : "cc", "memory"
> #ifdef CONFIG_X86_64
> @@ -11365,6 +11358,8 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> /* Eliminate branch target predictions from guest mode */
> vmexit_fill_RSB();
>
> + vcpu->arch.cr2 = read_cr2();
> +
> /* All fields are clean at this point */
> if (static_branch_unlikely(&enable_evmcs))
> current_evmcs->hv_clean_fields |=
> --
> 2.7.4
>
On Mon, Oct 29, 2018 at 04:40:43PM +0100, Julian Stecklina wrote:
> Split the security related register clearing out of the large inline
> assembly VM entry path. This results in two slightly less complicated
> inline assembly statements, where it is clearer what each one does.
>
> Signed-off-by: Julian Stecklina <[email protected]>
> Reviewed-by: Jan H. Sch?nherr <[email protected]>
> Reviewed-by: Konrad Jan Miller <[email protected]>
> Reviewed-by: Jim Mattson <[email protected]>
> ---
> arch/x86/kvm/vmx.c | 45 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index a6e5a5c..29a2ee7 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11281,24 +11281,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> "mov %%r13, %c[r13](%0) \n\t"
> "mov %%r14, %c[r14](%0) \n\t"
> "mov %%r15, %c[r15](%0) \n\t"
> - /*
> - * Clear host registers marked as clobbered to prevent
> - * speculative use.
> - */
> - "xor %%r8d, %%r8d \n\t"
> - "xor %%r9d, %%r9d \n\t"
> - "xor %%r10d, %%r10d \n\t"
> - "xor %%r11d, %%r11d \n\t"
> - "xor %%r12d, %%r12d \n\t"
> - "xor %%r13d, %%r13d \n\t"
> - "xor %%r14d, %%r14d \n\t"
> - "xor %%r15d, %%r15d \n\t"
> #endif
> -
> - "xor %%eax, %%eax \n\t"
> - "xor %%ebx, %%ebx \n\t"
> - "xor %%esi, %%esi \n\t"
> - "xor %%edi, %%edi \n\t"
> "pop %%" _ASM_BP "; pop %%" _ASM_DX " \n\t"
> ".pushsection .rodata \n\t"
> ".global vmx_return \n\t"
> @@ -11336,6 +11319,34 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> );
>
> /*
> + * Don't let guest register values survive. Registers that cannot
> + * contain guest values anymore are not touched.
I think it's a good idea to explicitly call out that clearing the GPRs
is done to prevent speculative use. Simply stating that we don't want
to let guest register values survive doesn't explain *why*.
What about:
/*
* Explicitly clear (in addition to marking them as clobbered) all GPRs
* that have not been loaded with host state to prevent speculatively
* using the guest's values.
*/
> + */
> + asm volatile (
> + "xor %%eax, %%eax \n\t"
> + "xor %%ebx, %%ebx \n\t"
> + "xor %%esi, %%esi \n\t"
> + "xor %%edi, %%edi \n\t"
> +#ifdef CONFIG_X86_64
> + "xor %%r8d, %%r8d \n\t"
> + "xor %%r9d, %%r9d \n\t"
> + "xor %%r10d, %%r10d \n\t"
> + "xor %%r11d, %%r11d \n\t"
> + "xor %%r12d, %%r12d \n\t"
> + "xor %%r13d, %%r13d \n\t"
> + "xor %%r14d, %%r14d \n\t"
> + "xor %%r15d, %%r15d \n\t"
> +#endif
> + ::: "cc"
> +#ifdef CONFIG_X86_64
> + , "rax", "rbx", "rsi", "rdi"
> + , "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15"
> +#else
> + , "eax", "ebx", "esi", "edi"
> +#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
> --
> 2.7.4
>
On Mon, 2018-10-29 at 10:26 -0700, Sean Christopherson wrote:
> I think it's a good idea to explicitly call out that clearing the
> GPRs
> is done to prevent speculative use. Simply stating that we don't
> want
> to let guest register values survive doesn't explain *why*.
>
> What about:
>
> /*
> * Explicitly clear (in addition to marking them as
> clobbered) all GPRs
> * that have not been loaded with host state to prevent
> speculatively
> * using the guest's values.
> */
Sounds good. I've also updated the commit message for the register
clearing commit. I'll repost the patches.
Julian
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B