[PATCH] arm64: KVM: Save two instructions in __guest_enter()

Subsystems: arm64 port (aarch64 architecture), kernel virtual machine for arm64 (kvm/arm64), the rest

STALE3663d

5 messages, 2 authors, 2016-08-29 · open the first message on its own page

[PATCH] arm64: KVM: Save two instructions in __guest_enter()

From: Shanker Donthineni <hidden>
Date: 2016-08-10 01:15:53

We are doing an unnecessary stack push/pop operation when restoring
the guest registers x0-x18 in __guest_enter(). This patch saves the
two instructions by using x18 as a base register. No need to store
the vcpu context pointer in stack because it is redundant and not
being used anywhere, the same information is available in tpidr_el2.

Signed-off-by: Shanker Donthineni <redacted>
---
 arch/arm64/kvm/hyp/entry.S | 66 ++++++++++++++++++++++------------------------
 1 file changed, 32 insertions(+), 34 deletions(-)
diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index ce9e5e5..d2e09a1 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -55,37 +55,32 @@
  */
 ENTRY(__guest_enter)
 	// x0: vcpu
-	// x1: host/guest context
-	// x2-x18: clobbered by macros
+	// x1: host context
+	// x2-x17: clobbered by macros
+	// x18: guest context
 
 	// Store the host regs
 	save_callee_saved_regs x1
 
-	// Preserve vcpu & host_ctxt for use at exit time
-	stp	x0, x1, [sp, #-16]!
+	// Preserve the host_ctxt for use at exit time
+	str	x1, [sp, #-16]!
 
-	add	x1, x0, #VCPU_CONTEXT
+	add	x18, x0, #VCPU_CONTEXT
 
-	// Prepare x0-x1 for later restore by pushing them onto the stack
-	ldp	x2, x3, [x1, #CPU_XREG_OFFSET(0)]
-	stp	x2, x3, [sp, #-16]!
+	// Restore guest regs x19-x29, lr
+	restore_callee_saved_regs x18
 
-	// x2-x18
-	ldp	x2, x3,   [x1, #CPU_XREG_OFFSET(2)]
-	ldp	x4, x5,   [x1, #CPU_XREG_OFFSET(4)]
-	ldp	x6, x7,   [x1, #CPU_XREG_OFFSET(6)]
-	ldp	x8, x9,   [x1, #CPU_XREG_OFFSET(8)]
-	ldp	x10, x11, [x1, #CPU_XREG_OFFSET(10)]
-	ldp	x12, x13, [x1, #CPU_XREG_OFFSET(12)]
-	ldp	x14, x15, [x1, #CPU_XREG_OFFSET(14)]
-	ldp	x16, x17, [x1, #CPU_XREG_OFFSET(16)]
-	ldr	x18,      [x1, #CPU_XREG_OFFSET(18)]
-
-	// x19-x29, lr
-	restore_callee_saved_regs x1
-
-	// Last bits of the 64bit state
-	ldp	x0, x1, [sp], #16
+	// Restore guest regs x0-x18
+	ldp	x0, x1,   [x18, #CPU_XREG_OFFSET(0)]
+	ldp	x2, x3,   [x18, #CPU_XREG_OFFSET(2)]
+	ldp	x4, x5,   [x18, #CPU_XREG_OFFSET(4)]
+	ldp	x6, x7,   [x18, #CPU_XREG_OFFSET(6)]
+	ldp	x8, x9,   [x18, #CPU_XREG_OFFSET(8)]
+	ldp	x10, x11, [x18, #CPU_XREG_OFFSET(10)]
+	ldp	x12, x13, [x18, #CPU_XREG_OFFSET(12)]
+	ldp	x14, x15, [x18, #CPU_XREG_OFFSET(14)]
+	ldp	x16, x17, [x18, #CPU_XREG_OFFSET(16)]
+	ldr	x18,      [x18, #CPU_XREG_OFFSET(18)]
 
 	// Do not touch any register after this!
 	eret
@@ -100,6 +95,16 @@ ENTRY(__guest_exit)
 
 	add	x2, x0, #VCPU_CONTEXT
 
+	// Store the guest regs x19-x29, lr
+	save_callee_saved_regs x2
+
+	// Retrieve the guest regs x0-x3 from the stack
+	ldp	x21, x22, [sp], #16	// x2, x3
+	ldp	x19, x20, [sp], #16	// x0, x1
+
+	// Store the guest regs x0-x18
+	stp	x19, x20, [x2, #CPU_XREG_OFFSET(0)]
+	stp	x21, x22, [x2, #CPU_XREG_OFFSET(2)]
 	stp	x4, x5,   [x2, #CPU_XREG_OFFSET(4)]
 	stp	x6, x7,   [x2, #CPU_XREG_OFFSET(6)]
 	stp	x8, x9,   [x2, #CPU_XREG_OFFSET(8)]
@@ -109,20 +114,13 @@ ENTRY(__guest_exit)
 	stp	x16, x17, [x2, #CPU_XREG_OFFSET(16)]
 	str	x18,      [x2, #CPU_XREG_OFFSET(18)]
 
-	ldp	x6, x7, [sp], #16	// x2, x3
-	ldp	x4, x5, [sp], #16	// x0, x1
+	// Restore the host_ctxt from the stack
+	ldr	x2, [sp], #16
 
-	stp	x4, x5, [x2, #CPU_XREG_OFFSET(0)]
-	stp	x6, x7, [x2, #CPU_XREG_OFFSET(2)]
-
-	save_callee_saved_regs x2
-
-	// Restore vcpu & host_ctxt from the stack
-	// (preserving return code in x1)
-	ldp	x0, x2, [sp], #16
 	// Now restore the host regs
 	restore_callee_saved_regs x2
 
+	// Preserving return code (x1)
 	mov	x0, x1
 	ret
 ENDPROC(__guest_exit)
-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] arm64: KVM: Save two instructions in __guest_enter()

From: Christoffer Dall <hidden>
Date: 2016-08-25 13:31:25

Hi Shanker,

On Tue, Aug 09, 2016 at 08:15:36PM -0500, Shanker Donthineni wrote:
We are doing an unnecessary stack push/pop operation when restoring
the guest registers x0-x18 in __guest_enter(). This patch saves the
two instructions by using x18 as a base register. No need to store
the vcpu context pointer in stack because it is redundant and not
being used anywhere, the same information is available in tpidr_el2.
Does this have any measureable benefit?

Thanks,
-Christoffer

Re: [PATCH] arm64: KVM: Save two instructions in __guest_enter()

From: Shanker Donthineni <hidden>
Date: 2016-08-29 15:22:11

Hi Christoffer,

This is change may not provide the measurable performance improvement, 
but still we can
save a few cpu cycles on vCPU context switch and also improves the code 
readability.


On 08/25/2016 08:31 AM, Christoffer Dall wrote:
Hi Shanker,

On Tue, Aug 09, 2016 at 08:15:36PM -0500, Shanker Donthineni wrote:
quoted
We are doing an unnecessary stack push/pop operation when restoring
the guest registers x0-x18 in __guest_enter(). This patch saves the
two instructions by using x18 as a base register. No need to store
the vcpu context pointer in stack because it is redundant and not
being used anywhere, the same information is available in tpidr_el2.
Does this have any measureable benefit?

Thanks,
-Christoffer
-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] arm64: KVM: Save two instructions in __guest_enter()

From: Christoffer Dall <hidden>
Date: 2016-08-29 18:11:51

On Tue, Aug 09, 2016 at 08:15:36PM -0500, Shanker Donthineni wrote:
quoted hunk
We are doing an unnecessary stack push/pop operation when restoring
the guest registers x0-x18 in __guest_enter(). This patch saves the
two instructions by using x18 as a base register. No need to store
the vcpu context pointer in stack because it is redundant and not
being used anywhere, the same information is available in tpidr_el2.

Signed-off-by: Shanker Donthineni <redacted>
---
 arch/arm64/kvm/hyp/entry.S | 66 ++++++++++++++++++++++------------------------
 1 file changed, 32 insertions(+), 34 deletions(-)
diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index ce9e5e5..d2e09a1 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -55,37 +55,32 @@
  */
 ENTRY(__guest_enter)
 	// x0: vcpu
-	// x1: host/guest context
-	// x2-x18: clobbered by macros
+	// x1: host context
+	// x2-x17: clobbered by macros
+	// x18: guest context
 
 	// Store the host regs
 	save_callee_saved_regs x1
 
-	// Preserve vcpu & host_ctxt for use at exit time
-	stp	x0, x1, [sp, #-16]!
+	// Preserve the host_ctxt for use at exit time
+	str	x1, [sp, #-16]!
 
-	add	x1, x0, #VCPU_CONTEXT
+	add	x18, x0, #VCPU_CONTEXT
 
-	// Prepare x0-x1 for later restore by pushing them onto the stack
-	ldp	x2, x3, [x1, #CPU_XREG_OFFSET(0)]
-	stp	x2, x3, [sp, #-16]!
+	// Restore guest regs x19-x29, lr
+	restore_callee_saved_regs x18
couldn't moving this load here be bad for prefetching?
quoted hunk
 
-	// x2-x18
-	ldp	x2, x3,   [x1, #CPU_XREG_OFFSET(2)]
-	ldp	x4, x5,   [x1, #CPU_XREG_OFFSET(4)]
-	ldp	x6, x7,   [x1, #CPU_XREG_OFFSET(6)]
-	ldp	x8, x9,   [x1, #CPU_XREG_OFFSET(8)]
-	ldp	x10, x11, [x1, #CPU_XREG_OFFSET(10)]
-	ldp	x12, x13, [x1, #CPU_XREG_OFFSET(12)]
-	ldp	x14, x15, [x1, #CPU_XREG_OFFSET(14)]
-	ldp	x16, x17, [x1, #CPU_XREG_OFFSET(16)]
-	ldr	x18,      [x1, #CPU_XREG_OFFSET(18)]
-
-	// x19-x29, lr
-	restore_callee_saved_regs x1
-
-	// Last bits of the 64bit state
-	ldp	x0, x1, [sp], #16
+	// Restore guest regs x0-x18
+	ldp	x0, x1,   [x18, #CPU_XREG_OFFSET(0)]
+	ldp	x2, x3,   [x18, #CPU_XREG_OFFSET(2)]
+	ldp	x4, x5,   [x18, #CPU_XREG_OFFSET(4)]
+	ldp	x6, x7,   [x18, #CPU_XREG_OFFSET(6)]
+	ldp	x8, x9,   [x18, #CPU_XREG_OFFSET(8)]
+	ldp	x10, x11, [x18, #CPU_XREG_OFFSET(10)]
+	ldp	x12, x13, [x18, #CPU_XREG_OFFSET(12)]
+	ldp	x14, x15, [x18, #CPU_XREG_OFFSET(14)]
+	ldp	x16, x17, [x18, #CPU_XREG_OFFSET(16)]
+	ldr	x18,      [x18, #CPU_XREG_OFFSET(18)]
 
 	// Do not touch any register after this!
 	eret
@@ -100,6 +95,16 @@ ENTRY(__guest_exit)
 
 	add	x2, x0, #VCPU_CONTEXT
 
+	// Store the guest regs x19-x29, lr
+	save_callee_saved_regs x2
same question here (although with a different weight as we were already
'jumping back' with the memory address in our store sequence.

If this is a real concern, a better approach would be to override x0
with the vcpu context pointer, do two pairs of load/stores using x2,x3
for the vcpu x0-x3, and then proceed with the rest of the registers.
quoted hunk
+
+	// Retrieve the guest regs x0-x3 from the stack
+	ldp	x21, x22, [sp], #16	// x2, x3
+	ldp	x19, x20, [sp], #16	// x0, x1
+
+	// Store the guest regs x0-x18
+	stp	x19, x20, [x2, #CPU_XREG_OFFSET(0)]
+	stp	x21, x22, [x2, #CPU_XREG_OFFSET(2)]
 	stp	x4, x5,   [x2, #CPU_XREG_OFFSET(4)]
 	stp	x6, x7,   [x2, #CPU_XREG_OFFSET(6)]
 	stp	x8, x9,   [x2, #CPU_XREG_OFFSET(8)]
@@ -109,20 +114,13 @@ ENTRY(__guest_exit)
 	stp	x16, x17, [x2, #CPU_XREG_OFFSET(16)]
 	str	x18,      [x2, #CPU_XREG_OFFSET(18)]
 
-	ldp	x6, x7, [sp], #16	// x2, x3
-	ldp	x4, x5, [sp], #16	// x0, x1
+	// Restore the host_ctxt from the stack
+	ldr	x2, [sp], #16
 
-	stp	x4, x5, [x2, #CPU_XREG_OFFSET(0)]
-	stp	x6, x7, [x2, #CPU_XREG_OFFSET(2)]
-
-	save_callee_saved_regs x2
-
-	// Restore vcpu & host_ctxt from the stack
-	// (preserving return code in x1)
-	ldp	x0, x2, [sp], #16
 	// Now restore the host regs
 	restore_callee_saved_regs x2
 
+	// Preserving return code (x1)
nit: preserve is a strange word to choose to describe what you do here.

if you want to do what I suggested above, you could change the two
callers to return the return code in x0, and the vcpu pointer in x1 and
then you can save this instruction as well.
 	mov	x0, x1
 	ret
 ENDPROC(__guest_exit)
-- 
Thanks,
-Christoffer

Re: [PATCH] arm64: KVM: Save two instructions in __guest_enter()

From: Christoffer Dall <hidden>
Date: 2016-08-29 18:12:55

On Mon, Aug 29, 2016 at 10:22:06AM -0500, Shanker Donthineni wrote:
Hi Christoffer,

This is change may not provide the measurable performance
improvement, but still we can
save a few cpu cycles on vCPU context switch and also improves the
code readability.
I'm not so convinced about saving CPU cycles, but now when I've reviewed
your patch I think it has potential to slightly improve the code.  See
my review of your patch.

-Christoffer
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help