Xen devel - Feb 2012 - [PATCH] arm: restore ELR_hyp and SPSR_hyp on return from hypervisor to hypervisor.

This is necessary to handle nested traps to the hypervisor more than one deep.

I''ve not seen an actually failure relating to this but I''m not
quite sure how
we''ve managed to get away with not doing it (I suppose multiply nested
traps
are uncommon).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/entry.S |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/xen/arch/arm/entry.S b/xen/arch/arm/entry.S
index 0b9cce5..36f1119 100644
--- a/xen/arch/arm/entry.S
+++ b/xen/arch/arm/entry.S
@@ -102,6 +102,10 @@ ENTRY(return_to_guest)
 
 ENTRY(return_to_hypervisor)
 	ldr lr, [sp, #UREGS_lr]
+	ldr r11, [sp, #UREGS_pc]
+	msr ELR_hyp, r11
+	ldr r11, [sp, #UREGS_cpsr]
+	msr SPSR_hyp, r11
 	pop {r0-r12}
 	add sp, #(UREGS_R8_fiq - UREGS_sp); /* SP, LR, SPSR, PC */
 	eret
-- 
1.7.2.5

On Mon, 2012-02-20 at 15:22 +0000, Ian Campbell wrote:
> This is necessary to handle nested traps to the hypervisor more than one
deep.
A sort of corollary to this patch is the following.

The situation with the LR register when in hyp mode is a bit odd, since
this is normally a banked register but for hyp mode it actually accesses
LR_usr instead.

However, although I think the following is correct I''m not sure if it
is
useful to combine LR_usr and LR like this -- in particular I fear it
might be more confusing than helpful. Making the return-to-user case a
proper superset of return-to-hypervisor (as is the case on the save
path) is nice though.

What do people think?

8<---------------------------------------------------------------

From 3132c2976ae4c83d6d9b94ad80ee04c4880f13da Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Mon, 20 Feb 2012 15:07:09 +0000
Subject: [PATCH] arm: lr register in hyp mode is really LR_usr.

Save and restore it in the same way for both hypervisor and user stack frames
rather than saving both individually in the user stack frame.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/entry.S          |   14 ++------------
 xen/include/public/arch-arm.h |   15 ++++++++++++---
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/entry.S b/xen/arch/arm/entry.S
index 36f1119..0e85c3c 100644
--- a/xen/arch/arm/entry.S
+++ b/xen/arch/arm/entry.S
@@ -29,8 +29,6 @@
 	blne save_guest_regs
 
 save_guest_regs:
-	ldr r11, [sp, #UREGS_lr]
-	str r11, [sp, #UREGS_LR_usr]
 	ldr r11, =0xffffffff  /* Clobber SP which is only valid for hypervisor frames.
*/
 	str r11, [sp, #UREGS_sp]
 	SAVE_ONE_BANKED(SP_usr)
@@ -78,15 +76,11 @@ ENTRY(return_from_trap)
 	and r11, #PSR_MODE_MASK
 	cmp r11, #PSR_MODE_HYP
 	beq return_to_hypervisor
-
+	/* Fall thru */
 ENTRY(return_to_guest)
 	mov r11, sp
 	bic sp, #7 /* Align the stack pointer */
 	bl leave_hypervisor_tail
-	ldr r11, [sp, #UREGS_pc]
-	msr ELR_hyp, r11
-	ldr r11, [sp, #UREGS_cpsr]
-	msr SPSR_hyp, r11
 	RESTORE_ONE_BANKED(SP_usr)
 	RESTORE_BANKED(svc)
 	RESTORE_BANKED(abt)
@@ -95,11 +89,7 @@ ENTRY(return_to_guest)
 	RESTORE_BANKED(fiq)
 	RESTORE_ONE_BANKED(R8_fiq); RESTORE_ONE_BANKED(R9_fiq);
RESTORE_ONE_BANKED(R10_fiq)
 	RESTORE_ONE_BANKED(R11_fiq); RESTORE_ONE_BANKED(R12_fiq);
-	ldr lr, [sp, #UREGS_LR_usr]
-	pop {r0-r12}
-	add sp, #(UREGS_R8_fiq - UREGS_sp); /* SP, LR, SPSR, PC */
-	eret
-
+	/* Fall thru */
 ENTRY(return_to_hypervisor)
 	ldr lr, [sp, #UREGS_lr]
 	ldr r11, [sp, #UREGS_pc]
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index e3d5c08..edb78b4 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -63,7 +63,12 @@ struct cpu_user_regs
     uint32_t r12;
 
     uint32_t sp; /* r13 - SP: Valid for Hyp. frames only, o/w banked (see
below) */
-    uint32_t lr; /* r14 - LR: Valid for Hyp. Same physical register as lr_usr.
*/
+
+    /* r14 - LR: is the same physical register as LR_usr */
+    union {
+        uint32_t lr; /* r14 - LR: Valid for Hyp. Same physical register as
lr_usr. */
+        uint32_t lr_usr;
+    };
 
     uint32_t pc; /* Return IP */
     uint32_t cpsr; /* Return mode */
@@ -73,10 +78,14 @@ struct cpu_user_regs
 
     uint32_t r8_fiq, r9_fiq, r10_fiq, r11_fiq, r12_fiq;
 
-    uint32_t sp_usr, sp_svc, sp_abt, sp_und, sp_irq, sp_fiq;
-    uint32_t lr_usr, lr_svc, lr_abt, lr_und, lr_irq, lr_fiq;
+    uint32_t sp_usr; /* LR_usr is the same register as LR, see above */
+
+    uint32_t sp_svc, sp_abt, sp_und, sp_irq, sp_fiq;
+    uint32_t lr_svc, lr_abt, lr_und, lr_irq, lr_fiq;
 
     uint32_t spsr_svc, spsr_abt, spsr_und, spsr_irq, spsr_fiq;
+
+    uint32_t pad1; /* Doubleword-align the user half of the frame */
 };
 typedef struct cpu_user_regs cpu_user_regs_t;
 DEFINE_XEN_GUEST_HANDLE(cpu_user_regs_t);
-- 
1.7.2.5

On Mon, 20 Feb 2012, Ian Campbell wrote:
> On Mon, 2012-02-20 at 15:22 +0000, Ian Campbell wrote:
> > This is necessary to handle nested traps to the hypervisor more than
one deep.
> 
> A sort of corollary to this patch is the following.
> 
> The situation with the LR register when in hyp mode is a bit odd, since
> this is normally a banked register but for hyp mode it actually accesses
> LR_usr instead.
> 
> However, although I think the following is correct I''m not sure if
it is
> useful to combine LR_usr and LR like this -- in particular I fear it
> might be more confusing than helpful. Making the return-to-user case a
> proper superset of return-to-hypervisor (as is the case on the save
> path) is nice though.
> 
> What do people think?
I am OK with this.
> 8<---------------------------------------------------------------
> 
> >From 3132c2976ae4c83d6d9b94ad80ee04c4880f13da Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Mon, 20 Feb 2012 15:07:09 +0000
> Subject: [PATCH] arm: lr register in hyp mode is really LR_usr.
> 
> Save and restore it in the same way for both hypervisor and user stack
frames
> rather than saving both individually in the user stack frame.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/entry.S          |   14 ++------------
>  xen/include/public/arch-arm.h |   15 ++++++++++++---
>  2 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/entry.S b/xen/arch/arm/entry.S
> index 36f1119..0e85c3c 100644
> --- a/xen/arch/arm/entry.S
> +++ b/xen/arch/arm/entry.S
> @@ -29,8 +29,6 @@
>  	blne save_guest_regs
>  
>  save_guest_regs:
> -	ldr r11, [sp, #UREGS_lr]
> -	str r11, [sp, #UREGS_LR_usr]
>  	ldr r11, =0xffffffff  /* Clobber SP which is only valid for hypervisor
frames. */
>  	str r11, [sp, #UREGS_sp]
It would be nice to have a comment here saying "no need to save LR again
because LR_usr and LR are the same physical register"

On Tue, 2012-02-21 at 10:53 +0000, Stefano Stabellini
wrote:
> On Mon, 20 Feb 2012, Ian Campbell wrote:
> > On Mon, 2012-02-20 at 15:22 +0000, Ian Campbell wrote:
> > > This is necessary to handle nested traps to the hypervisor more
than one deep.
> > 
> > A sort of corollary to this patch is the following.
> > 
> > The situation with the LR register when in hyp mode is a bit odd,
since
> > this is normally a banked register but for hyp mode it actually
accesses
> > LR_usr instead.
> > 
> > However, although I think the following is correct I''m not
sure if it is
> > useful to combine LR_usr and LR like this -- in particular I fear it
> > might be more confusing than helpful. Making the return-to-user case a
> > proper superset of return-to-hypervisor (as is the case on the save
> > path) is nice though.
> > 
> > What do people think?
> 
> I am OK with this.
Is that an Acked-by?
> 
> > 8<---------------------------------------------------------------
> > 
> > >From 3132c2976ae4c83d6d9b94ad80ee04c4880f13da Mon Sep 17 00:00:00
2001
> > From: Ian Campbell <ian.campbell@citrix.com>
> > Date: Mon, 20 Feb 2012 15:07:09 +0000
> > Subject: [PATCH] arm: lr register in hyp mode is really LR_usr.
> > 
> > Save and restore it in the same way for both hypervisor and user stack
frames
> > rather than saving both individually in the user stack frame.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/entry.S          |   14 ++------------
> >  xen/include/public/arch-arm.h |   15 ++++++++++++---
> >  2 files changed, 14 insertions(+), 15 deletions(-)
> > 
> > diff --git a/xen/arch/arm/entry.S b/xen/arch/arm/entry.S
> > index 36f1119..0e85c3c 100644
> > --- a/xen/arch/arm/entry.S
> > +++ b/xen/arch/arm/entry.S
> > @@ -29,8 +29,6 @@
> >  	blne save_guest_regs
> >  
> >  save_guest_regs:
> > -	ldr r11, [sp, #UREGS_lr]
> > -	str r11, [sp, #UREGS_LR_usr]
> >  	ldr r11, =0xffffffff  /* Clobber SP which is only valid for
hypervisor frames. */
> >  	str r11, [sp, #UREGS_sp]
> 
> It would be nice to have a comment here saying "no need to save LR
again
> because LR_usr and LR are the same physical register"
I''ll add something next to both the save and restore of SP_usr saying
why LR_usr isn''t also saved/restored there.

On Tue, 21 Feb 2012, Ian Campbell wrote:
> On Tue, 2012-02-21 at 10:53 +0000, Stefano Stabellini wrote:
> > On Mon, 20 Feb 2012, Ian Campbell wrote:
> > > On Mon, 2012-02-20 at 15:22 +0000, Ian Campbell wrote:
> > > > This is necessary to handle nested traps to the hypervisor
more than one deep.
> > > 
> > > A sort of corollary to this patch is the following.
> > > 
> > > The situation with the LR register when in hyp mode is a bit odd,
since
> > > this is normally a banked register but for hyp mode it actually
accesses
> > > LR_usr instead.
> > > 
> > > However, although I think the following is correct I''m
not sure if it is
> > > useful to combine LR_usr and LR like this -- in particular I fear
it
> > > might be more confusing than helpful. Making the return-to-user
case a
> > > proper superset of return-to-hypervisor (as is the case on the
save
> > > path) is nice though.
> > > 
> > > What do people think?
> > 
> > I am OK with this.
> 
> Is that an Acked-by?
with the comments, yes

On Mon, 20 Feb 2012, Ian Campbell wrote:
> This is necessary to handle nested traps to the hypervisor more than one
deep.
> 
> I''ve not seen an actually failure relating to this but
I''m not quite sure how
> we''ve managed to get away with not doing it (I suppose multiply
nested traps
> are uncommon).
ack
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/entry.S |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/arm/entry.S b/xen/arch/arm/entry.S
> index 0b9cce5..36f1119 100644
> --- a/xen/arch/arm/entry.S
> +++ b/xen/arch/arm/entry.S
> @@ -102,6 +102,10 @@ ENTRY(return_to_guest)
>  
>  ENTRY(return_to_hypervisor)
>  	ldr lr, [sp, #UREGS_lr]
> +	ldr r11, [sp, #UREGS_pc]
> +	msr ELR_hyp, r11
> +	ldr r11, [sp, #UREGS_cpsr]
> +	msr SPSR_hyp, r11
>  	pop {r0-r12}
>  	add sp, #(UREGS_R8_fiq - UREGS_sp); /* SP, LR, SPSR, PC */
>  	eret
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

On Wed, 2012-02-22 at 14:23 +0000, Stefano Stabellini
wrote:
> On Mon, 20 Feb 2012, Ian Campbell wrote:
> > This is necessary to handle nested traps to the hypervisor more than
one deep.
> > 
> > I''ve not seen an actually failure relating to this but
I''m not quite sure how
> > we''ve managed to get away with not doing it (I suppose
multiply nested traps
> > are uncommon).
> 
> ack
Thanks, I have committed both patches from this thread:
        arm: restore ELR_hyp and SPSR_hyp on return from hypervisor to
hypervisor.
        arm: lr register in hyp mode is really LR_usr.

Ian.