kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] KVM/nVMX: Move nested_vmx_check_vmentry_hw inline assembly to vmenter.S
@ 2020-10-07 14:43 Uros Bizjak
  2020-10-13 15:19 ` Sean Christopherson
  0 siblings, 1 reply; 3+ messages in thread
From: Uros Bizjak @ 2020-10-07 14:43 UTC (permalink / raw)
  To: kvm; +Cc: Uros Bizjak, Paolo Bonzini, Sean Christopherson

Move the big inline assembly block from nested_vmx_check_vmentry_hw
to vmenter.S assembly file, taking into account all ABI requirements.

The new function is modelled after __vmx_vcpu_run, and also calls
vmx_update_host_rsp instead of open-coding the function in assembly.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
---
 arch/x86/kvm/vmx/nested.c  | 32 +++-----------------------------
 arch/x86/kvm/vmx/vmenter.S | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 1bb6b31eb646..7b26e983e31c 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3012,6 +3012,8 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+bool __nested_vmx_check_vmentry_hw(struct vcpu_vmx *vmx, bool launched);
+
 static int nested_vmx_check_vmentry_hw(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -3050,35 +3052,7 @@ static int nested_vmx_check_vmentry_hw(struct kvm_vcpu *vcpu)
 		vmx->loaded_vmcs->host_state.cr4 = cr4;
 	}
 
-	asm(
-		"sub $%c[wordsize], %%" _ASM_SP "\n\t" /* temporarily adjust RSP for CALL */
-		"cmp %%" _ASM_SP ", %c[host_state_rsp](%[loaded_vmcs]) \n\t"
-		"je 1f \n\t"
-		__ex("vmwrite %%" _ASM_SP ", %[HOST_RSP]") "\n\t"
-		"mov %%" _ASM_SP ", %c[host_state_rsp](%[loaded_vmcs]) \n\t"
-		"1: \n\t"
-		"add $%c[wordsize], %%" _ASM_SP "\n\t" /* un-adjust RSP */
-
-		/* Check if vmlaunch or vmresume is needed */
-		"cmpb $0, %c[launched](%[loaded_vmcs])\n\t"
-
-		/*
-		 * VMLAUNCH and VMRESUME clear RFLAGS.{CF,ZF} on VM-Exit, set
-		 * RFLAGS.CF on VM-Fail Invalid and set RFLAGS.ZF on VM-Fail
-		 * Valid.  vmx_vmenter() directly "returns" RFLAGS, and so the
-		 * results of VM-Enter is captured via CC_{SET,OUT} to vm_fail.
-		 */
-		"call vmx_vmenter\n\t"
-
-		CC_SET(be)
-	      : ASM_CALL_CONSTRAINT, CC_OUT(be) (vm_fail)
-	      :	[HOST_RSP]"r"((unsigned long)HOST_RSP),
-		[loaded_vmcs]"r"(vmx->loaded_vmcs),
-		[launched]"i"(offsetof(struct loaded_vmcs, launched)),
-		[host_state_rsp]"i"(offsetof(struct loaded_vmcs, host_state.rsp)),
-		[wordsize]"i"(sizeof(ulong))
-	      : "memory"
-	);
+	vm_fail = __nested_vmx_check_vmentry_hw(vmx, vmx->loaded_vmcs->launched);
 
 	if (vmx->msr_autoload.host.nr)
 		vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, vmx->msr_autoload.host.nr);
diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index 799db084a336..9fdcbd9320dc 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -234,6 +234,42 @@ SYM_FUNC_START(__vmx_vcpu_run)
 	jmp 1b
 SYM_FUNC_END(__vmx_vcpu_run)
 
+/**
+ * __nested_vmx_check_vmentry_hw - Run a vCPU via a transition to
+ *				   a nested VMX guest mode
+ * @vmx:	struct vcpu_vmx * (forwarded to vmx_update_host_rsp)
+ * @launched:	%true if the VMCS has been launched
+ *
+ * Returns:
+ *	0 on VM-Exit, 1 on VM-Fail
+ */
+SYM_FUNC_START(__nested_vmx_check_vmentry_hw)
+	push %_ASM_BP
+	mov  %_ASM_SP, %_ASM_BP
+
+	push %_ASM_BX
+
+	/* Copy @launched to BL, _ASM_ARG2 is volatile. */
+	mov %_ASM_ARG2B, %bl
+
+	/* Adjust RSP to account for the CALL to vmx_vmenter(). */
+	lea -WORD_SIZE(%_ASM_SP), %_ASM_ARG2
+	call vmx_update_host_rsp
+
+	/* Check if vmlaunch or vmresume is needed */
+	cmpb $0, %bl
+
+	/* Enter guest mode */
+	call vmx_vmenter
+
+	/* Return 0 on VM-Exit, 1 on VM-Fail */
+	setbe %al
+
+	pop %_ASM_BX
+
+	pop %_ASM_BP
+	ret
+SYM_FUNC_END(__nested_vmx_check_vmentry_hw)
 
 .section .text, "ax"
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] KVM/nVMX: Move nested_vmx_check_vmentry_hw inline assembly to vmenter.S
  2020-10-07 14:43 [PATCH] KVM/nVMX: Move nested_vmx_check_vmentry_hw inline assembly to vmenter.S Uros Bizjak
@ 2020-10-13 15:19 ` Sean Christopherson
  2020-10-14 13:33   ` Uros Bizjak
  0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2020-10-13 15:19 UTC (permalink / raw)
  To: Uros Bizjak; +Cc: kvm, Paolo Bonzini

On Wed, Oct 07, 2020 at 04:43:12PM +0200, Uros Bizjak wrote:
> Move the big inline assembly block from nested_vmx_check_vmentry_hw
> to vmenter.S assembly file, taking into account all ABI requirements.
> 
> The new function is modelled after __vmx_vcpu_run, and also calls
> vmx_update_host_rsp instead of open-coding the function in assembly.

Is there specific motivation for this change?  The inline asm is ugly, but
it's contained.

If we really want to get rid of the inline asm, I'd probably vote to simply
use __vmx_vcpu_run() instead of adding another assembly helper.  The (double)
GPR save/restore is wasteful, but this flow is basically anti-performance
anyways.  Outside of KVM developers, I doubt anyone actually enables this path.

> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> ---
>  arch/x86/kvm/vmx/nested.c  | 32 +++-----------------------------
>  arch/x86/kvm/vmx/vmenter.S | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 1bb6b31eb646..7b26e983e31c 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -3012,6 +3012,8 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> +bool __nested_vmx_check_vmentry_hw(struct vcpu_vmx *vmx, bool launched);
> +
>  static int nested_vmx_check_vmentry_hw(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> @@ -3050,35 +3052,7 @@ static int nested_vmx_check_vmentry_hw(struct kvm_vcpu *vcpu)
>  		vmx->loaded_vmcs->host_state.cr4 = cr4;
>  	}
>  
> -	asm(
> -		"sub $%c[wordsize], %%" _ASM_SP "\n\t" /* temporarily adjust RSP for CALL */
> -		"cmp %%" _ASM_SP ", %c[host_state_rsp](%[loaded_vmcs]) \n\t"
> -		"je 1f \n\t"
> -		__ex("vmwrite %%" _ASM_SP ", %[HOST_RSP]") "\n\t"
> -		"mov %%" _ASM_SP ", %c[host_state_rsp](%[loaded_vmcs]) \n\t"
> -		"1: \n\t"
> -		"add $%c[wordsize], %%" _ASM_SP "\n\t" /* un-adjust RSP */
> -
> -		/* Check if vmlaunch or vmresume is needed */
> -		"cmpb $0, %c[launched](%[loaded_vmcs])\n\t"
> -
> -		/*
> -		 * VMLAUNCH and VMRESUME clear RFLAGS.{CF,ZF} on VM-Exit, set
> -		 * RFLAGS.CF on VM-Fail Invalid and set RFLAGS.ZF on VM-Fail
> -		 * Valid.  vmx_vmenter() directly "returns" RFLAGS, and so the
> -		 * results of VM-Enter is captured via CC_{SET,OUT} to vm_fail.
> -		 */
> -		"call vmx_vmenter\n\t"
> -
> -		CC_SET(be)
> -	      : ASM_CALL_CONSTRAINT, CC_OUT(be) (vm_fail)
> -	      :	[HOST_RSP]"r"((unsigned long)HOST_RSP),
> -		[loaded_vmcs]"r"(vmx->loaded_vmcs),
> -		[launched]"i"(offsetof(struct loaded_vmcs, launched)),
> -		[host_state_rsp]"i"(offsetof(struct loaded_vmcs, host_state.rsp)),
> -		[wordsize]"i"(sizeof(ulong))
> -	      : "memory"
> -	);
> +	vm_fail = __nested_vmx_check_vmentry_hw(vmx, vmx->loaded_vmcs->launched);
>  
>  	if (vmx->msr_autoload.host.nr)
>  		vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, vmx->msr_autoload.host.nr);
> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> index 799db084a336..9fdcbd9320dc 100644
> --- a/arch/x86/kvm/vmx/vmenter.S
> +++ b/arch/x86/kvm/vmx/vmenter.S
> @@ -234,6 +234,42 @@ SYM_FUNC_START(__vmx_vcpu_run)
>  	jmp 1b
>  SYM_FUNC_END(__vmx_vcpu_run)
>  
> +/**
> + * __nested_vmx_check_vmentry_hw - Run a vCPU via a transition to
> + *				   a nested VMX guest mode

This function comment is incorrect, this helper doesn't run the vCPU, it
simply executes VMLAUNCH or VMRESUME, which are expected to fail (and we're
in BUG_ON territory if they don't).

> + * @vmx:	struct vcpu_vmx * (forwarded to vmx_update_host_rsp)
> + * @launched:	%true if the VMCS has been launched
> + *
> + * Returns:
> + *	0 on VM-Exit, 1 on VM-Fail
> + */
> +SYM_FUNC_START(__nested_vmx_check_vmentry_hw)
> +	push %_ASM_BP
> +	mov  %_ASM_SP, %_ASM_BP
> +
> +	push %_ASM_BX
> +
> +	/* Copy @launched to BL, _ASM_ARG2 is volatile. */
> +	mov %_ASM_ARG2B, %bl
> +
> +	/* Adjust RSP to account for the CALL to vmx_vmenter(). */
> +	lea -WORD_SIZE(%_ASM_SP), %_ASM_ARG2
> +	call vmx_update_host_rsp
> +
> +	/* Check if vmlaunch or vmresume is needed */
> +	cmpb $0, %bl
> +
> +	/* Enter guest mode */
> +	call vmx_vmenter
> +
> +	/* Return 0 on VM-Exit, 1 on VM-Fail */
> +	setbe %al
> +
> +	pop %_ASM_BX
> +
> +	pop %_ASM_BP
> +	ret
> +SYM_FUNC_END(__nested_vmx_check_vmentry_hw)
>  
>  .section .text, "ax"
>  
> -- 
> 2.26.2
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] KVM/nVMX: Move nested_vmx_check_vmentry_hw inline assembly to vmenter.S
  2020-10-13 15:19 ` Sean Christopherson
@ 2020-10-14 13:33   ` Uros Bizjak
  0 siblings, 0 replies; 3+ messages in thread
From: Uros Bizjak @ 2020-10-14 13:33 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, Paolo Bonzini

On Tue, Oct 13, 2020 at 5:19 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Wed, Oct 07, 2020 at 04:43:12PM +0200, Uros Bizjak wrote:
> > Move the big inline assembly block from nested_vmx_check_vmentry_hw
> > to vmenter.S assembly file, taking into account all ABI requirements.
> >
> > The new function is modelled after __vmx_vcpu_run, and also calls
> > vmx_update_host_rsp instead of open-coding the function in assembly.
>
> Is there specific motivation for this change?  The inline asm is ugly, but
> it's contained.

The motivation was mostly the removal of open-coded asm implementation
of vmx_update_host_rsp, with corresponding asm arguments. Later, I
also noticed, that the resulting asm is surprisingly similar to
__vmx_vcpu_run, so I assumed that it would be nice to put the whole
low-level function to vmenter.S
>
> If we really want to get rid of the inline asm, I'd probably vote to simply
> use __vmx_vcpu_run() instead of adding another assembly helper.  The (double)
> GPR save/restore is wasteful, but this flow is basically anti-performance
> anyways.  Outside of KVM developers, I doubt anyone actually enables this path.

If this is the case, then the removal of a bunch of bytes at the
expense of a few extra cycles is a good deal.

Uros.

> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> > Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> > ---
> >  arch/x86/kvm/vmx/nested.c  | 32 +++-----------------------------
> >  arch/x86/kvm/vmx/vmenter.S | 36 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 39 insertions(+), 29 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 1bb6b31eb646..7b26e983e31c 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -3012,6 +3012,8 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> >       return 0;
> >  }
> >
> > +bool __nested_vmx_check_vmentry_hw(struct vcpu_vmx *vmx, bool launched);
> > +
> >  static int nested_vmx_check_vmentry_hw(struct kvm_vcpu *vcpu)
> >  {
> >       struct vcpu_vmx *vmx = to_vmx(vcpu);
> > @@ -3050,35 +3052,7 @@ static int nested_vmx_check_vmentry_hw(struct kvm_vcpu *vcpu)
> >               vmx->loaded_vmcs->host_state.cr4 = cr4;
> >       }
> >
> > -     asm(
> > -             "sub $%c[wordsize], %%" _ASM_SP "\n\t" /* temporarily adjust RSP for CALL */
> > -             "cmp %%" _ASM_SP ", %c[host_state_rsp](%[loaded_vmcs]) \n\t"
> > -             "je 1f \n\t"
> > -             __ex("vmwrite %%" _ASM_SP ", %[HOST_RSP]") "\n\t"
> > -             "mov %%" _ASM_SP ", %c[host_state_rsp](%[loaded_vmcs]) \n\t"
> > -             "1: \n\t"
> > -             "add $%c[wordsize], %%" _ASM_SP "\n\t" /* un-adjust RSP */
> > -
> > -             /* Check if vmlaunch or vmresume is needed */
> > -             "cmpb $0, %c[launched](%[loaded_vmcs])\n\t"
> > -
> > -             /*
> > -              * VMLAUNCH and VMRESUME clear RFLAGS.{CF,ZF} on VM-Exit, set
> > -              * RFLAGS.CF on VM-Fail Invalid and set RFLAGS.ZF on VM-Fail
> > -              * Valid.  vmx_vmenter() directly "returns" RFLAGS, and so the
> > -              * results of VM-Enter is captured via CC_{SET,OUT} to vm_fail.
> > -              */
> > -             "call vmx_vmenter\n\t"
> > -
> > -             CC_SET(be)
> > -           : ASM_CALL_CONSTRAINT, CC_OUT(be) (vm_fail)
> > -           : [HOST_RSP]"r"((unsigned long)HOST_RSP),
> > -             [loaded_vmcs]"r"(vmx->loaded_vmcs),
> > -             [launched]"i"(offsetof(struct loaded_vmcs, launched)),
> > -             [host_state_rsp]"i"(offsetof(struct loaded_vmcs, host_state.rsp)),
> > -             [wordsize]"i"(sizeof(ulong))
> > -           : "memory"
> > -     );
> > +     vm_fail = __nested_vmx_check_vmentry_hw(vmx, vmx->loaded_vmcs->launched);
> >
> >       if (vmx->msr_autoload.host.nr)
> >               vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, vmx->msr_autoload.host.nr);
> > diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> > index 799db084a336..9fdcbd9320dc 100644
> > --- a/arch/x86/kvm/vmx/vmenter.S
> > +++ b/arch/x86/kvm/vmx/vmenter.S
> > @@ -234,6 +234,42 @@ SYM_FUNC_START(__vmx_vcpu_run)
> >       jmp 1b
> >  SYM_FUNC_END(__vmx_vcpu_run)
> >
> > +/**
> > + * __nested_vmx_check_vmentry_hw - Run a vCPU via a transition to
> > + *                              a nested VMX guest mode
>
> This function comment is incorrect, this helper doesn't run the vCPU, it
> simply executes VMLAUNCH or VMRESUME, which are expected to fail (and we're
> in BUG_ON territory if they don't).
>
> > + * @vmx:     struct vcpu_vmx * (forwarded to vmx_update_host_rsp)
> > + * @launched:        %true if the VMCS has been launched
> > + *
> > + * Returns:
> > + *   0 on VM-Exit, 1 on VM-Fail
> > + */
> > +SYM_FUNC_START(__nested_vmx_check_vmentry_hw)
> > +     push %_ASM_BP
> > +     mov  %_ASM_SP, %_ASM_BP
> > +
> > +     push %_ASM_BX
> > +
> > +     /* Copy @launched to BL, _ASM_ARG2 is volatile. */
> > +     mov %_ASM_ARG2B, %bl
> > +
> > +     /* Adjust RSP to account for the CALL to vmx_vmenter(). */
> > +     lea -WORD_SIZE(%_ASM_SP), %_ASM_ARG2
> > +     call vmx_update_host_rsp
> > +
> > +     /* Check if vmlaunch or vmresume is needed */
> > +     cmpb $0, %bl
> > +
> > +     /* Enter guest mode */
> > +     call vmx_vmenter
> > +
> > +     /* Return 0 on VM-Exit, 1 on VM-Fail */
> > +     setbe %al
> > +
> > +     pop %_ASM_BX
> > +
> > +     pop %_ASM_BP
> > +     ret
> > +SYM_FUNC_END(__nested_vmx_check_vmentry_hw)
> >
> >  .section .text, "ax"
> >
> > --
> > 2.26.2
> >

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-14 13:34 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 14:43 [PATCH] KVM/nVMX: Move nested_vmx_check_vmentry_hw inline assembly to vmenter.S Uros Bizjak
2020-10-13 15:19 ` Sean Christopherson
2020-10-14 13:33   ` Uros Bizjak

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).