[03/29] KVM: VMX: Modify only RSP when creating a placeholder for guest's RCX
diff mbox series

Message ID 20190118212037.24412-4-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
In the vCPU-run asm blob, the guest's RCX is temporarily saved onto the
stack after VM-Exit as the exit flow must first load a register with a
pointer to the vCPU's save area in order to save the guest's registers.
RCX is arbitrarily designated as the scratch register.

Since the stack usage is to (1)save host, (2)save guest, (3)load host
and (4)load guest, the code can't conform to the stack's natural FIFO
semantics, i.e. it can't simply do PUSH/POP.  Regardless of whether it
is done for the host's value or guest's value, at some point the code
needs to access the stack using a non-traditional method, e.g. MOV
instead of POP.  vCPU-run opts to create a placeholder on the stack for
guest's RCX (by adjusting RSP) and saves RCX to its place immediately
after VM-Exit (via MOV).

In other words, the purpose of the first 'PUSH RCX' at the start of
the vCPU-run asm blob  is to adjust RSP down, i.e. there's no need to
actually access memory.  Use 'SUB $wordsize, RSP' instead of 'PUSH RCX'
to make it more obvious that the intent is simply to create a gap on
the stack for the guest's RCX.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jim Mattson Jan. 18, 2019, 10:09 p.m. UTC | #1
On Fri, Jan 18, 2019 at 1:22 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> In the vCPU-run asm blob, the guest's RCX is temporarily saved onto the
> stack after VM-Exit as the exit flow must first load a register with a
> pointer to the vCPU's save area in order to save the guest's registers.
> RCX is arbitrarily designated as the scratch register.
>
> Since the stack usage is to (1)save host, (2)save guest, (3)load host
> and (4)load guest, the code can't conform to the stack's natural FIFO
> semantics, i.e. it can't simply do PUSH/POP.  Regardless of whether it
> is done for the host's value or guest's value, at some point the code
> needs to access the stack using a non-traditional method, e.g. MOV
> instead of POP.  vCPU-run opts to create a placeholder on the stack for
> guest's RCX (by adjusting RSP) and saves RCX to its place immediately
> after VM-Exit (via MOV).
>
> In other words, the purpose of the first 'PUSH RCX' at the start of
> the vCPU-run asm blob  is to adjust RSP down, i.e. there's no need to
> actually access memory.  Use 'SUB $wordsize, RSP' instead of 'PUSH RCX'
> to make it more obvious that the intent is simply to create a gap on
> the stack for the guest's RCX.
>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
Konrad Rzeszutek Wilk Jan. 22, 2019, 8:35 a.m. UTC | #2
On Fri, Jan 18, 2019 at 01:20:11PM -0800, Sean Christopherson wrote:
> In the vCPU-run asm blob, the guest's RCX is temporarily saved onto the
> stack after VM-Exit as the exit flow must first load a register with a
> pointer to the vCPU's save area in order to save the guest's registers.
> RCX is arbitrarily designated as the scratch register.
> 
> Since the stack usage is to (1)save host, (2)save guest, (3)load host
> and (4)load guest, the code can't conform to the stack's natural FIFO
> semantics, i.e. it can't simply do PUSH/POP.  Regardless of whether it
> is done for the host's value or guest's value, at some point the code
> needs to access the stack using a non-traditional method, e.g. MOV
> instead of POP.  vCPU-run opts to create a placeholder on the stack for
> guest's RCX (by adjusting RSP) and saves RCX to its place immediately
> after VM-Exit (via MOV).
> 
> In other words, the purpose of the first 'PUSH RCX' at the start of
> the vCPU-run asm blob  is to adjust RSP down, i.e. there's no need to
> actually access memory.  Use 'SUB $wordsize, RSP' instead of 'PUSH RCX'
> to make it more obvious that the intent is simply to create a gap on
> the stack for the guest's RCX.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thank you!
> ---
>  arch/x86/kvm/vmx/vmx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 23e5fa58751a..adf59fd23a6c 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6377,7 +6377,7 @@ static void __vmx_vcpu_run(struct kvm_vcpu *vcpu, struct vcpu_vmx *vmx)
>  	asm(
>  		/* Store host registers */
>  		"push %%" _ASM_DX "; push %%" _ASM_BP ";"
> -		"push %%" _ASM_CX " \n\t" /* placeholder for guest rcx */
> +		"sub $%c[wordsize], %%" _ASM_SP "\n\t" /* placeholder for guest RCX */
>  		"push %%" _ASM_CX " \n\t"
>  		"sub $%c[wordsize], %%" _ASM_SP "\n\t" /* temporarily adjust RSP for CALL */
>  		"cmp %%" _ASM_SP ", %c[host_rsp](%%" _ASM_CX ") \n\t"
> -- 
> 2.20.1
>

Patch
diff mbox series

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 23e5fa58751a..adf59fd23a6c 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6377,7 +6377,7 @@  static void __vmx_vcpu_run(struct kvm_vcpu *vcpu, struct vcpu_vmx *vmx)
 	asm(
 		/* Store host registers */
 		"push %%" _ASM_DX "; push %%" _ASM_BP ";"
-		"push %%" _ASM_CX " \n\t" /* placeholder for guest rcx */
+		"sub $%c[wordsize], %%" _ASM_SP "\n\t" /* placeholder for guest RCX */
 		"push %%" _ASM_CX " \n\t"
 		"sub $%c[wordsize], %%" _ASM_SP "\n\t" /* temporarily adjust RSP for CALL */
 		"cmp %%" _ASM_SP ", %c[host_rsp](%%" _ASM_CX ") \n\t"