[28/29] KVM: VMX: Make the vCPU-run asm routine callable from C
diff mbox series

Message ID 20190118212037.24412-29-sean.j.christopherson@intel.com
State New
Headers show
Series
  • KVM: VMX: Move vCPU-run to proper asm sub-routine
Related show

Commit Message

Sean Christopherson Jan. 18, 2019, 9:20 p.m. UTC
...and of course actually call it from C now that the assembly code is
in a dedicated sub-routine and has been cleansed of any quirks that
would break compliance with the kernel's x86 calling conventions.

Aside from saving/restoring registers instead of clobbering them, the
only real function difference is that VM-Fail is propagated to EAX
instead of EBX.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/vmenter.S | 53 ++++++++++++++++++++++++++------------
 arch/x86/kvm/vmx/vmx.c     | 23 +++--------------
 2 files changed, 40 insertions(+), 36 deletions(-)

Comments

Paolo Bonzini Jan. 22, 2019, 12:35 p.m. UTC | #1
On 18/01/19 22:20, Sean Christopherson wrote:
> ...and of course actually call it from C now that the assembly code is
> in a dedicated sub-routine and has been cleansed of any quirks that
> would break compliance with the kernel's x86 calling conventions.

I'm not sure about removing the clobbering.  If the stack is outside L1,
could this lead to speculative execution with the guest values in the
registers?

No problem with patches 1-26 though.

Paolo
Sean Christopherson Jan. 22, 2019, 2:59 p.m. UTC | #2
On Tue, Jan 22, 2019 at 01:35:41PM +0100, Paolo Bonzini wrote:
> On 18/01/19 22:20, Sean Christopherson wrote:
> > ...and of course actually call it from C now that the assembly code is
> > in a dedicated sub-routine and has been cleansed of any quirks that
> > would break compliance with the kernel's x86 calling conventions.
> 
> I'm not sure about removing the clobbering.  If the stack is outside L1,
> could this lead to speculative execution with the guest values in the
> registers?

The stack belongs to the L0 kernel.  Or did I misunderstand the comment?

> 
> No problem with patches 1-26 though.
> 
> Paolo
Paolo Bonzini Jan. 24, 2019, 8:21 p.m. UTC | #3
On 22/01/19 15:59, Sean Christopherson wrote:
> On Tue, Jan 22, 2019 at 01:35:41PM +0100, Paolo Bonzini wrote:
>> On 18/01/19 22:20, Sean Christopherson wrote:
>>> ...and of course actually call it from C now that the assembly code is
>>> in a dedicated sub-routine and has been cleansed of any quirks that
>>> would break compliance with the kernel's x86 calling conventions.
>>
>> I'm not sure about removing the clobbering.  If the stack is outside L1,
>> could this lead to speculative execution with the guest values in the
>> registers?
> 
> The stack belongs to the L0 kernel.  Or did I misunderstand the comment?

I meant outside L1 cache (overloaded terms... :)).

Paolo
Sean Christopherson Jan. 24, 2019, 9:23 p.m. UTC | #4
On Thu, Jan 24, 2019 at 09:21:31PM +0100, Paolo Bonzini wrote:
> On 22/01/19 15:59, Sean Christopherson wrote:
> > On Tue, Jan 22, 2019 at 01:35:41PM +0100, Paolo Bonzini wrote:
> >> On 18/01/19 22:20, Sean Christopherson wrote:
> >>> ...and of course actually call it from C now that the assembly code is
> >>> in a dedicated sub-routine and has been cleansed of any quirks that
> >>> would break compliance with the kernel's x86 calling conventions.
> >>
> >> I'm not sure about removing the clobbering.  If the stack is outside L1,
> >> could this lead to speculative execution with the guest values in the
> >> registers?
> > 
> > The stack belongs to the L0 kernel.  Or did I misunderstand the comment?
> 
> I meant outside L1 cache (overloaded terms... :)).

Alternatively, what about zeroing out the callee-save registers prior to
restoring them?  That'd allow the function to be called from C, and the
patch could be introduced earlier in the series, e.g. to apply the logic
to RBP, which is currently only saved/restored.  A few extra zeroing
XORs is dirt cheap, especially on CPUs with move elimination.
Paolo Bonzini Jan. 25, 2019, 11:36 a.m. UTC | #5
On 24/01/19 22:23, Sean Christopherson wrote:
> On Thu, Jan 24, 2019 at 09:21:31PM +0100, Paolo Bonzini wrote:
>> On 22/01/19 15:59, Sean Christopherson wrote:
>>> On Tue, Jan 22, 2019 at 01:35:41PM +0100, Paolo Bonzini wrote:
>>>> On 18/01/19 22:20, Sean Christopherson wrote:
>>>>> ...and of course actually call it from C now that the assembly code is
>>>>> in a dedicated sub-routine and has been cleansed of any quirks that
>>>>> would break compliance with the kernel's x86 calling conventions.
>>>>
>>>> I'm not sure about removing the clobbering.  If the stack is outside L1,
>>>> could this lead to speculative execution with the guest values in the
>>>> registers?
>>>
>>> The stack belongs to the L0 kernel.  Or did I misunderstand the comment?
>>
>> I meant outside L1 cache (overloaded terms... :)).
> 
> Alternatively, what about zeroing out the callee-save registers prior to
> restoring them?  That'd allow the function to be called from C, and the
> patch could be introduced earlier in the series, e.g. to apply the logic
> to RBP, which is currently only saved/restored.  A few extra zeroing
> XORs is dirt cheap, especially on CPUs with move elimination.

That's a good idea, it gets the best of both worlds.

Paolo

Patch
diff mbox series

diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index 45e3e381d41d..28c9034773b8 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -87,17 +87,34 @@  ENDPROC(vmx_vmexit)
  * __vmx_vcpu_run - Run a vCPU via a transition to VMX guest mode
  * @vmx:	struct vcpu_vmx *
  * @regs:	unsigned long * (to guest registers)
- * %RBX:	VMCS launched status (non-zero indicates already launched)
+ * @launched:	%true if the VMCS has been launched
  *
  * Returns:
- *	%RBX is 0 on VM-Exit, 1 on VM-Fail
+ *	0 on VM-Exit, 1 on VM-Fail
  */
 ENTRY(__vmx_vcpu_run)
-	/* Create a stack frame and save @regs. */
 	push %_ASM_BP
-	mov %_ASM_SP, %_ASM_BP
+	mov  %_ASM_SP, %_ASM_BP
+#ifdef CONFIG_X86_64
+	push %r15
+	push %r14
+	push %r13
+	push %r12
+#else
+	push %edi
+	push %esi
+#endif
+	push %_ASM_BX
+
+	/*
+	 * Save @regs, its register may be modified by vmx_update_host_rsp()
+	 * and it's also needed after VM-Exit.
+	 */
 	push %_ASM_ARG2
 
+	/* Copy @launched to BL, _ASM_ARG3 is volatile. */
+	mov %_ASM_ARG3B, %bl
+
 	/* Adjust RSP to account for the CALL to vmx_vmenter(). */
 	lea -WORD_SIZE(%_ASM_SP), %_ASM_ARG2
 	call vmx_update_host_rsp
@@ -159,8 +176,8 @@  ENTRY(__vmx_vcpu_run)
 	mov %r15, VCPU_R15(%_ASM_AX)
 #endif
 
-	/* Clear EBX to indicate VM-Exit (as opposed to VM-Fail). */
-	xor %ebx, %ebx
+	/* Clear RAX to indicate VM-Exit (as opposed to VM-Fail). */
+	xor %eax, %eax
 
 	/*
 	 * Clear registers that contain guest values and will not be
@@ -172,10 +189,6 @@  ENTRY(__vmx_vcpu_run)
 	xor %r9d,  %r9d
 	xor %r10d, %r10d
 	xor %r11d, %r11d
-	xor %r12d, %r12d
-	xor %r13d, %r13d
-	xor %r14d, %r14d
-	xor %r15d, %r15d
 #endif
 	xor %ecx, %ecx
 	xor %edx, %edx
@@ -184,15 +197,21 @@  ENTRY(__vmx_vcpu_run)
 
 	/* "POP" @regs. */
 	add $WORD_SIZE, %_ASM_SP
-	pop  %_ASM_BP
+	pop %_ASM_BX
+
+#ifdef CONFIG_X86_64
+	pop %r12
+	pop %r13
+	pop %r14
+	pop %r15
+#else
+	pop %esi
+	pop %edi
+#endif
+	pop %_ASM_BP
 	ret
 
 	/* VM-Fail.  Out-of-line to avoid a taken Jcc after VM-Exit. */
-2:	mov $1, %ebx
-	/*
-	 * RAX holds a guest value and it's not cleared in the common
-	 * exit path as VM-Exit reloads it with the vcpu_vmx pointer.
-	 */
-	xor %eax, %eax
+2:	mov $1, %eax
 	jmp 1b
 ENDPROC(__vmx_vcpu_run)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c4c6d6ccd53b..d840810ca91f 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6370,6 +6370,8 @@  void vmx_update_host_rsp(struct vcpu_vmx *vmx, unsigned long host_rsp)
 	}
 }
 
+bool __vmx_vcpu_run(struct vcpu_vmx *vmx, unsigned long *regs, bool launched);
+
 static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -6443,25 +6445,8 @@  static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	if (vcpu->arch.cr2 != read_cr2())
 		write_cr2(vcpu->arch.cr2);
 
-	asm(
-		"call __vmx_vcpu_run \n\t"
-	      : ASM_CALL_CONSTRAINT, "=ebx"(vmx->fail),
-#ifdef CONFIG_X86_64
-		"=D"((int){0}), "=S"((int){0})
-	      : "D"(vmx), "S"(&vcpu->arch.regs),
-#else
-		"=a"((int){0}), "=d"((int){0})
-	      : "a"(vmx), "d"(&vcpu->arch.regs),
-#endif
-		"bl"(vmx->loaded_vmcs->launched)
-	      : "cc", "memory"
-#ifdef CONFIG_X86_64
-		, "rax", "rcx", "rdx"
-		, "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15"
-#else
-		, "ecx", "edi", "esi"
-#endif
-	      );
+	vmx->fail = __vmx_vcpu_run(vmx, (unsigned long *)&vcpu->arch.regs,
+				   vmx->loaded_vmcs->launched);
 
 	vcpu->arch.cr2 = read_cr2();