Xen devel - Dec 2012 - [PATCH V2] xen: arm: fix guest register access.

We weren''t taking the guest mode (CPSR) into account and would always
access the user version of the registers.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Fix r8 vs r8_fiq thinko.
---
 xen/arch/arm/traps.c       |   62 ++++++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/vgic.c        |    4 +-
 xen/arch/arm/vpl011.c      |    4 +-
 xen/arch/arm/vtimer.c      |    7 +++--
 xen/include/asm-arm/regs.h |    6 ++++
 5 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index bddd7d4..f42e4e9 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -73,6 +73,64 @@ static void print_xen_info(void)
            debug, print_tainted(taint_str));
 }
 
+uint32_t *select_user_reg(struct cpu_user_regs *regs, int reg)
+{
+    BUG_ON( guest_mode(regs) );
+
+    /*
+     * We rely heavily on the layout of cpu_user_regs to avoid having
+     * to handle all of the registers individually. Use BUILD_BUG_ON to
+     * ensure that things which expect are contiguous actually are.
+     */
+#define REGOFFS(R) offsetof(struct cpu_user_regs, R)
+
+    switch ( reg ) {
+    case 0 ... 7: /* Unbanked registers */
+        BUILD_BUG_ON(REGOFFS(r0) + 7*sizeof(uint32_t) != REGOFFS(r7));
+        return &regs->r0 + reg;
+    case 8 ... 12: /* Register banked in FIQ mode */
+        BUILD_BUG_ON(REGOFFS(r8_fiq) + 4*sizeof(uint32_t) != REGOFFS(r12_fiq));
+        if ( fiq_mode(regs) )
+            return &regs->r8_fiq + reg - 8;
+        else
+            return &regs->r8 + reg - 8;
+    case 13 ... 14: /* Banked SP + LR registers */
+        BUILD_BUG_ON(REGOFFS(sp_fiq) + 1*sizeof(uint32_t) != REGOFFS(lr_fiq));
+        BUILD_BUG_ON(REGOFFS(sp_irq) + 1*sizeof(uint32_t) != REGOFFS(lr_irq));
+        BUILD_BUG_ON(REGOFFS(sp_svc) + 1*sizeof(uint32_t) != REGOFFS(lr_svc));
+        BUILD_BUG_ON(REGOFFS(sp_abt) + 1*sizeof(uint32_t) != REGOFFS(lr_abt));
+        BUILD_BUG_ON(REGOFFS(sp_und) + 1*sizeof(uint32_t) != REGOFFS(lr_und));
+        switch ( regs->cpsr & PSR_MODE_MASK )
+        {
+        case PSR_MODE_USR:
+        case PSR_MODE_SYS: /* Sys regs are the usr regs */
+            if ( reg == 13 )
+                return &regs->sp_usr;
+            else /* lr_usr == lr in a user frame */
+                return &regs->lr;
+        case PSR_MODE_FIQ:
+            return &regs->sp_fiq + reg - 13;
+        case PSR_MODE_IRQ:
+            return &regs->sp_irq + reg - 13;
+        case PSR_MODE_SVC:
+            return &regs->sp_svc + reg - 13;
+        case PSR_MODE_ABT:
+            return &regs->sp_abt + reg - 13;
+        case PSR_MODE_UND:
+            return &regs->sp_und + reg - 13;
+        case PSR_MODE_MON:
+        case PSR_MODE_HYP:
+        default:
+            BUG();
+        }
+    case 15: /* PC */
+        return &regs->pc;
+    default:
+        BUG();
+    }
+#undef REGOFFS
+}
+
 static const char *decode_fsc(uint32_t fsc, int *level)
 {
     const char *msg = NULL;
@@ -448,7 +506,7 @@ static void do_debug_trap(struct cpu_user_regs *regs,
unsigned int code)
     switch ( code ) {
     case 0xe0 ... 0xef:
         reg = code - 0xe0;
-        r = &regs->r0 + reg;
+        r = select_user_reg(regs, reg);
         printk("DOM%d: R%d = %#010"PRIx32" at
%#010"PRIx32"\n",
                domid, reg, *r, regs->pc);
         break;
@@ -518,7 +576,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
                        union hsr hsr)
 {
     struct hsr_cp32 cp32 = hsr.cp32;
-    uint32_t *r = &regs->r0 + cp32.reg;
+    uint32_t *r = select_user_reg(regs, cp32.reg);
 
     if ( !cp32.ccvalid ) {
         dprintk(XENLOG_ERR, "cp_15(32): need to handle invalid condition
codes\n");
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 7d1a5ad..39b9775 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -160,7 +160,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t
*info)
 {
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
-    uint32_t *r = &regs->r0 + dabt.reg;
+    uint32_t *r = select_user_reg(regs, dabt.reg);
     struct vgic_irq_rank *rank;
     int offset = (int)(info->gpa - VGIC_DISTR_BASE_ADDRESS);
     int gicd_reg = REG(offset);
@@ -372,7 +372,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t
*info)
 {
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
-    uint32_t *r = &regs->r0 + dabt.reg;
+    uint32_t *r = select_user_reg(regs, dabt.reg);
     struct vgic_irq_rank *rank;
     int offset = (int)(info->gpa - VGIC_DISTR_BASE_ADDRESS);
     int gicd_reg = REG(offset);
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 1522667..7dcee90 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -92,7 +92,7 @@ static int uart0_mmio_read(struct vcpu *v, mmio_info_t *info)
 {
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
-    uint32_t *r = &regs->r0 + dabt.reg;
+    uint32_t *r = select_user_reg(regs, dabt.reg);
     int offset = (int)(info->gpa - UART0_START);
 
     switch ( offset )
@@ -114,7 +114,7 @@ static int uart0_mmio_write(struct vcpu *v, mmio_info_t
*info)
 {
     struct hsr_dabt dabt = info->dabt;
     struct cpu_user_regs *regs = guest_cpu_user_regs();
-    uint32_t *r = &regs->r0 + dabt.reg;
+    uint32_t *r = select_user_reg(regs, dabt.reg);
     int offset = (int)(info->gpa - UART0_START);
 
     switch ( offset )
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 1c45f4a..fc452e3 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -22,6 +22,7 @@
 #include <xen/timer.h>
 #include <xen/sched.h>
 #include <asm/gic.h>
+#include <asm/regs.h>
 
 extern s_time_t ticks_to_ns(uint64_t ticks);
 extern uint64_t ns_to_ticks(s_time_t ns);
@@ -49,7 +50,7 @@ static int vtimer_emulate_32(struct cpu_user_regs *regs, union
hsr hsr)
 {
     struct vcpu *v = current;
     struct hsr_cp32 cp32 = hsr.cp32;
-    uint32_t *r = &regs->r0 + cp32.reg;
+    uint32_t *r = select_user_reg(regs, cp32.reg);
     s_time_t now;
 
     switch ( hsr.bits & HSR_CP32_REGS_MASK )
@@ -101,8 +102,8 @@ static int vtimer_emulate_64(struct cpu_user_regs *regs,
union hsr hsr)
 {
     struct vcpu *v = current;
     struct hsr_cp64 cp64 = hsr.cp64;
-    uint32_t *r1 = &regs->r0 + cp64.reg1;
-    uint32_t *r2 = &regs->r0 + cp64.reg2;
+    uint32_t *r1 = select_user_reg(regs, cp64.reg1);
+    uint32_t *r2 = select_user_reg(regs, cp64.reg2);
     uint64_t ticks;
     s_time_t now;
 
diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h
index 54f6ed8..7486944 100644
--- a/xen/include/asm-arm/regs.h
+++ b/xen/include/asm-arm/regs.h
@@ -30,6 +30,12 @@
 
 #define return_reg(v) ((v)->arch.cpu_info->guest_cpu_user_regs.r0)
 
+/*
+ * Returns a pointer to the given register value in regs, taking the
+ * processor mode (CPSR) into account.
+ */
+extern uint32_t *select_user_reg(struct cpu_user_regs *regs, int reg);
+
 #endif /* __ARM_REGS_H__ */
 /*
  * Local variables:
-- 
1.7.2.5

At 14:56 +0000 on 19 Dec (1355929001), Ian Campbell
wrote:
> We weren''t taking the guest mode (CPSR) into account and would
always
> access the user version of the registers.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>

On Thu, 2012-12-20 at 11:30 +0000, Tim Deegan wrote:
> At 14:56 +0000 on 19 Dec (1355929001), Ian Campbell wrote:
> > We weren''t taking the guest mode (CPSR) into account and
would always
> > access the user version of the registers.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Tim Deegan <tim@xen.org>
Applied, thanks.

On Thu, 2012-12-20 at 11:30 +0000, Tim Deegan wrote:
> At 14:56 +0000 on 19 Dec (1355929001), Ian Campbell wrote:
> > We weren''t taking the guest mode (CPSR) into account and
would always
> > access the user version of the registers.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Tim Deegan <tim@xen.org>
Thanks.

I applied this last year and forgot to push. Now done.

Ian.

On Wed, 2012-12-19 at 14:56 +0000, Ian Campbell wrote:
> @@ -73,6 +73,64 @@ static void print_xen_info(void)
>             debug, print_tainted(taint_str));
>  }
>  
> +uint32_t *select_user_reg(struct cpu_user_regs *regs, int reg)
> +{
> +    BUG_ON( guest_mode(regs) );
I was obviously on crack here.

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

From a53486a9aa6c0bd9512ac5e213119c6bda27e3b6 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Tue, 15 Jan 2013 11:25:05 +0000
Subject: [PATCH] xen: arm: fix assert in select_user_reg

The condition was inverted.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f42e4e9..8afcf0c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -75,7 +75,7 @@ static void print_xen_info(void)
 
 uint32_t *select_user_reg(struct cpu_user_regs *regs, int reg)
 {
-    BUG_ON( guest_mode(regs) );
+    BUG_ON( !guest_mode(regs) );
 
     /*
      * We rely heavily on the layout of cpu_user_regs to avoid having
-- 
1.7.2.5

On Tue, 15 Jan 2013, Ian Campbell wrote:
> On Wed, 2012-12-19 at 14:56 +0000, Ian Campbell wrote:
> > @@ -73,6 +73,64 @@ static void print_xen_info(void)
> >             debug, print_tainted(taint_str));
> >  }
> >  
> > +uint32_t *select_user_reg(struct cpu_user_regs *regs, int reg)
> > +{
> > +    BUG_ON( guest_mode(regs) );
> 
> I was obviously on crack here.
> 
> 8<--------------------------
> 
> From a53486a9aa6c0bd9512ac5e213119c6bda27e3b6 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Tue, 15 Jan 2013 11:25:05 +0000
> Subject: [PATCH] xen: arm: fix assert in select_user_reg
> 
> The condition was inverted.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

>  xen/arch/arm/traps.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index f42e4e9..8afcf0c 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -75,7 +75,7 @@ static void print_xen_info(void)
>  
>  uint32_t *select_user_reg(struct cpu_user_regs *regs, int reg)
>  {
> -    BUG_ON( guest_mode(regs) );
> +    BUG_ON( !guest_mode(regs) );
>  
>      /*
>       * We rely heavily on the layout of cpu_user_regs to avoid having
> -- 
> 1.7.2.5
> 
> 
> 
>

> > From a53486a9aa6c0bd9512ac5e213119c6bda27e3b6 Mon Sep 17 00:00:00 2001
> > From: Ian Campbell <ian.campbell@citrix.com>
> > Date: Tue, 15 Jan 2013 11:25:05 +0000
> > Subject: [PATCH] xen: arm: fix assert in select_user_reg
> > 
> > The condition was inverted.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Thanks, applied.