linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/3] kvm, vmx: move CR2 context switch out of assembly path
@ 2018-10-29 15:40 Julian Stecklina
  2018-10-29 15:40 ` [PATCH v2 2/3] kvm, vmx: move register clearing " Julian Stecklina
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Julian Stecklina @ 2018-10-29 15:40 UTC (permalink / raw)
  To: kvm, Paolo Bonzini; +Cc: Julian Stecklina, Julian Stecklina, linux-kernel

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 <jsteckli@amazon.de>
Reviewed-by: Jan H. Schönherr <jschoenh@amazon.de>
Reviewed-by: Konrad Jan Miller <kjm@amazon.de>
Reviewed-by: Jim Mattson <jmattson@google.com>
---
 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)&current_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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/3] kvm, vmx: move register clearing out of assembly path
  2018-10-29 15:40 [PATCH v2 1/3] kvm, vmx: move CR2 context switch out of assembly path Julian Stecklina
@ 2018-10-29 15:40 ` Julian Stecklina
  2018-10-29 17:26   ` Sean Christopherson
  2018-10-29 15:40 ` [PATCH v2 3/3] kvm, vmx: fix __invvpid style Julian Stecklina
  2018-10-29 17:14 ` [PATCH v2 1/3] kvm, vmx: move CR2 context switch out of assembly path Sean Christopherson
  2 siblings, 1 reply; 6+ messages in thread
From: Julian Stecklina @ 2018-10-29 15:40 UTC (permalink / raw)
  To: kvm, Paolo Bonzini; +Cc: Julian Stecklina, Julian Stecklina, linux-kernel

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 <jsteckli@amazon.de>
Reviewed-by: Jan H. Schönherr <jschoenh@amazon.de>
Reviewed-by: Konrad Jan Miller <kjm@amazon.de>
Reviewed-by: Jim Mattson <jmattson@google.com>
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 3/3] kvm, vmx: fix __invvpid style
  2018-10-29 15:40 [PATCH v2 1/3] kvm, vmx: move CR2 context switch out of assembly path Julian Stecklina
  2018-10-29 15:40 ` [PATCH v2 2/3] kvm, vmx: move register clearing " Julian Stecklina
@ 2018-10-29 15:40 ` Julian Stecklina
  2018-10-29 17:14 ` [PATCH v2 1/3] kvm, vmx: move CR2 context switch out of assembly path Sean Christopherson
  2 siblings, 0 replies; 6+ messages in thread
From: Julian Stecklina @ 2018-10-29 15:40 UTC (permalink / raw)
  To: kvm, Paolo Bonzini; +Cc: Julian Stecklina, Julian Stecklina, linux-kernel

The code violated the coding style. Fixed by using tabs instead of
spaces. There are only whitespace changes here.

Signed-off-by: Julian Stecklina <jsteckli@amazon.de>
Reviewed-by: Jan H. Schönherr <jschoenh@amazon.de>
Reviewed-by: Konrad Jan Miller <kjm@amazon.de>
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/3] kvm, vmx: move CR2 context switch out of assembly path
  2018-10-29 15:40 [PATCH v2 1/3] kvm, vmx: move CR2 context switch out of assembly path Julian Stecklina
  2018-10-29 15:40 ` [PATCH v2 2/3] kvm, vmx: move register clearing " Julian Stecklina
  2018-10-29 15:40 ` [PATCH v2 3/3] kvm, vmx: fix __invvpid style Julian Stecklina
@ 2018-10-29 17:14 ` Sean Christopherson
  2 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2018-10-29 17:14 UTC (permalink / raw)
  To: Julian Stecklina; +Cc: kvm, Paolo Bonzini, Julian Stecklina, linux-kernel

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 <sean.j.christopherson@intel.com>


[1] Commit 6aa8b732ca01 ("[PATCH] kvm: userspace interface")
[2] Commit d3561b7fa0fb ("[PATCH] paravirt: header and stubs for paravirtualisation")
 
> Signed-off-by: Julian Stecklina <jsteckli@amazon.de>
> Reviewed-by: Jan H. Schönherr <jschoenh@amazon.de>
> Reviewed-by: Konrad Jan Miller <kjm@amazon.de>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> ---
>  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)&current_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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/3] kvm, vmx: move register clearing out of assembly path
  2018-10-29 15:40 ` [PATCH v2 2/3] kvm, vmx: move register clearing " Julian Stecklina
@ 2018-10-29 17:26   ` Sean Christopherson
  2018-11-01 13:40     ` Stecklina, Julian
  0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2018-10-29 17:26 UTC (permalink / raw)
  To: Julian Stecklina; +Cc: kvm, Paolo Bonzini, Julian Stecklina, linux-kernel

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 <jsteckli@amazon.de>
> Reviewed-by: Jan H. Schönherr <jschoenh@amazon.de>
> Reviewed-by: Konrad Jan Miller <kjm@amazon.de>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> ---
>  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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/3] kvm, vmx: move register clearing out of assembly path
  2018-10-29 17:26   ` Sean Christopherson
@ 2018-11-01 13:40     ` Stecklina, Julian
  0 siblings, 0 replies; 6+ messages in thread
From: Stecklina, Julian @ 2018-11-01 13:40 UTC (permalink / raw)
  To: sean.j.christopherson; +Cc: linux-kernel, kvm, js, pbonzini

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-11-01 13:40 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-29 15:40 [PATCH v2 1/3] kvm, vmx: move CR2 context switch out of assembly path Julian Stecklina
2018-10-29 15:40 ` [PATCH v2 2/3] kvm, vmx: move register clearing " Julian Stecklina
2018-10-29 17:26   ` Sean Christopherson
2018-11-01 13:40     ` Stecklina, Julian
2018-10-29 15:40 ` [PATCH v2 3/3] kvm, vmx: fix __invvpid style Julian Stecklina
2018-10-29 17:14 ` [PATCH v2 1/3] kvm, vmx: move CR2 context switch out of assembly path Sean Christopherson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).