diff mbox

[for-4.7] xen/arm64: correctly emulate the {w, x}zr registers

Message ID 1460026438-10372-1-git-send-email-julien.grall@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Julien Grall April 7, 2016, 10:53 a.m. UTC
On AArch64, encoding 31 for an R<n> in the HSR is used to represent
either {w,x}sp or {w,x}zr (See C1.2.4 in ARM DDI 0486A.d) depending on
how the register field is interpreted by the instruction.

All the instructions trapped by Xen (either via a sysreg access or
data abort) interpret encoding 31 as {w,x}zr. Therefore we don't have
to worry about the possibility that a trap could refer to sp or about
decoding the instruction.

For example AArch64 LDR and STR can have zr in the source/target
register <Xt>, but never sp. sp can be present in the destination
pointer( i.e.  "[sp]"), but that would be represented by the value of
FAR_EL2, not in the HSR.

For AArch32 it is possible for a LDR to target the PC, but this would
not result in a valid ISS in the HSR register. However this could only
occur if loading or storing the PC to MMIO, which we simply choose not
to support for now.

Finally, features such as xenaccess can lead to us trapping on
arbitrary instructions accessing RAM and not just for MMIO. However in
many such cases HSR.ISS is not valid and in general features such as
xenaccess do not rely on the nature of the specific instruction, they
resolve the fault (via information found elsewhere e.g. FAR_EL2)
without needing to know anything about the instruction which triggered
the trap.

The register zr represents the zero register, i.e it will always
return 0 and write to it is ignored. To properly handle this property,
2 new helpers have been introduced {get,set}_user_reg to read/write a
value from/to a register. All the calls to select_user_reg have been
replaced by these 2 helpers.

Furthermore, the code to emulate encoding 31 in select_user_reg has been
dropped because it was invalid. For Aarch64 context, the encoding is
used for sp or zr. For AArch32 context, the ISS won't be valid for data
abort from AArch32 using r15 (i.e pc) as source/destination (See D7-1881
ARM DDI 0487A.d, note the validity is more restrictive than on ARMv7).
It's also not possible to use r15 in co-processor instructions.

This patch fixes setting MMIO register and sysreg to a random value
(actually PC) instead of zero by something like:

*((volatile int*)reg) = 0;

compilers tend to generate "str wzr, [xx]" here.

[ian: added BUG_ON to select_user_reg and clarified bits of the commit message]
Reported-by: Marc Zyngier <Marc.Zyngier@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Stefano, let me know the new helper corresponds to change you requested
(see [1])

This patch is a bug fix for Xen 4.7. Without it, a MMIO register and
sysreg will be set to a random value (actually PC) when the zero
register is used.

I'm not sure if we should consider this patch to be backported to Xen
4.6 and Xen 4.5. It depends on other patches and it would require some
rework to backport it alone.

[1] http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03100.html
---
 xen/arch/arm/io.c          |  34 ++++++++----
 xen/arch/arm/traps.c       | 126 ++++++++++++++++++++++++++++++---------------
 xen/arch/arm/vgic-v3.c     |   3 +-
 xen/arch/arm/vtimer.c      |  59 ++++++++++++++++-----
 xen/include/asm-arm/regs.h |   7 +--
 5 files changed, 158 insertions(+), 71 deletions(-)

Comments

Jan Beulich April 7, 2016, 5:18 p.m. UTC | #1
>>> On 07.04.16 at 12:53, <julien.grall@arm.com> wrote:
> On AArch64, encoding 31 for an R<n> in the HSR is used to represent
> either {w,x}sp or {w,x}zr (See C1.2.4 in ARM DDI 0486A.d) depending on
> how the register field is interpreted by the instruction.
> 
> All the instructions trapped by Xen (either via a sysreg access or
> data abort) interpret encoding 31 as {w,x}zr. Therefore we don't have
> to worry about the possibility that a trap could refer to sp or about
> decoding the instruction.
> 
> For example AArch64 LDR and STR can have zr in the source/target
> register <Xt>, but never sp. sp can be present in the destination
> pointer( i.e.  "[sp]"), but that would be represented by the value of
> FAR_EL2, not in the HSR.
> 
> For AArch32 it is possible for a LDR to target the PC, but this would
> not result in a valid ISS in the HSR register. However this could only
> occur if loading or storing the PC to MMIO, which we simply choose not
> to support for now.
> 
> Finally, features such as xenaccess can lead to us trapping on
> arbitrary instructions accessing RAM and not just for MMIO. However in
> many such cases HSR.ISS is not valid and in general features such as
> xenaccess do not rely on the nature of the specific instruction, they
> resolve the fault (via information found elsewhere e.g. FAR_EL2)
> without needing to know anything about the instruction which triggered
> the trap.
> 
> The register zr represents the zero register, i.e it will always
> return 0 and write to it is ignored. To properly handle this property,
> 2 new helpers have been introduced {get,set}_user_reg to read/write a
> value from/to a register. All the calls to select_user_reg have been
> replaced by these 2 helpers.
> 
> Furthermore, the code to emulate encoding 31 in select_user_reg has been
> dropped because it was invalid. For Aarch64 context, the encoding is
> used for sp or zr. For AArch32 context, the ISS won't be valid for data
> abort from AArch32 using r15 (i.e pc) as source/destination (See D7-1881
> ARM DDI 0487A.d, note the validity is more restrictive than on ARMv7).
> It's also not possible to use r15 in co-processor instructions.
> 
> This patch fixes setting MMIO register and sysreg to a random value
> (actually PC) instead of zero by something like:
> 
> *((volatile int*)reg) = 0;
> 
> compilers tend to generate "str wzr, [xx]" here.
> 
> [ian: added BUG_ON to select_user_reg and clarified bits of the commit 
> message]
> Reported-by: Marc Zyngier <Marc.Zyngier@arm.com>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> Stefano, let me know the new helper corresponds to change you requested
> (see [1])
> 
> This patch is a bug fix for Xen 4.7. Without it, a MMIO register and
> sysreg will be set to a random value (actually PC) when the zero
> register is used.
> 
> I'm not sure if we should consider this patch to be backported to Xen
> 4.6 and Xen 4.5. It depends on other patches and it would require some
> rework to backport it alone.
> 
> [1] 
> http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03100.html 

So the tags and alike would suggest this is ready to be committed,
but the lack of a version number or version history don't really
help support this. Could you please clarify the state of this patch?

Jan
Julien Grall April 7, 2016, 6:49 p.m. UTC | #2
Hi Jan,

On 07/04/2016 18:18, Jan Beulich wrote:
>>>> On 07.04.16 at 12:53, <julien.grall@arm.com> wrote:
>> ---
>>
>> Stefano, let me know the new helper corresponds to change you requested
>> (see [1])
>>
>> This patch is a bug fix for Xen 4.7. Without it, a MMIO register and
>> sysreg will be set to a random value (actually PC) when the zero
>> register is used.
>>
>> I'm not sure if we should consider this patch to be backported to Xen
>> 4.6 and Xen 4.5. It depends on other patches and it would require some
>> rework to backport it alone.
>>
>> [1]
>> http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03100.html
>
> So the tags and alike would suggest this is ready to be committed,
> but the lack of a version number or version history don't really
> help support this. Could you please clarify the state of this patch?

Sorry I forgot to mention the changes I made. This is a resend with the 
modification suggested by Stefano.

I've kept Stefano's reviewed-by as he was fine with and without the 
helpers. Although, I would like to give Stefano a chance to object. Can 
you wait a bit before committing?

Regards,
Konrad Rzeszutek Wilk April 8, 2016, 4:02 p.m. UTC | #3
> >>http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg03100.html
> >
> >So the tags and alike would suggest this is ready to be committed,
> >but the lack of a version number or version history don't really
> >help support this. Could you please clarify the state of this patch?
> 
> Sorry I forgot to mention the changes I made. This is a resend with the
> modification suggested by Stefano.
> 
> I've kept Stefano's reviewed-by as he was fine with and without the helpers.
> Although, I would like to give Stefano a chance to object. Can you wait a
> bit before committing?

On IRC Julien asked it to be applied, so it has been done.
diff mbox

Patch

diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 7e29943..0156755 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -24,12 +24,19 @@ 
 #include <asm/mmio.h>
 
 static int handle_read(const struct mmio_handler *handler, struct vcpu *v,
-                       mmio_info_t *info, register_t *r)
+                       mmio_info_t *info)
 {
     const struct hsr_dabt dabt = info->dabt;
+    struct cpu_user_regs *regs = guest_cpu_user_regs();
+    /*
+     * Initialize to zero to avoid leaking data if there is an
+     * implementation error in the emulation (such as not correctly
+     * setting r).
+     */
+    register_t r = 0;
     uint8_t size = (1 << dabt.size) * 8;
 
-    if ( !handler->ops->read(v, info, r, handler->priv) )
+    if ( !handler->ops->read(v, info, &r, handler->priv) )
         return 0;
 
     /*
@@ -37,7 +44,7 @@  static int handle_read(const struct mmio_handler *handler, struct vcpu *v,
      * Note that we expect the read handler to have zeroed the bits
      * outside the requested access size.
      */
-    if ( dabt.sign && (*r & (1UL << (size - 1))) )
+    if ( dabt.sign && (r & (1UL << (size - 1))) )
     {
         /*
          * We are relying on register_t using the same as
@@ -45,21 +52,30 @@  static int handle_read(const struct mmio_handler *handler, struct vcpu *v,
          * code smaller.
          */
         BUILD_BUG_ON(sizeof(register_t) != sizeof(unsigned long));
-        *r |= (~0UL) << size;
+        r |= (~0UL) << size;
     }
 
+    set_user_reg(regs, dabt.reg, r);
+
     return 1;
 }
 
+static int handle_write(const struct mmio_handler *handler, struct vcpu *v,
+                        mmio_info_t *info)
+{
+    const struct hsr_dabt dabt = info->dabt;
+    struct cpu_user_regs *regs = guest_cpu_user_regs();
+
+    return handler->ops->write(v, info, get_user_reg(regs, dabt.reg),
+                               handler->priv);
+}
+
 int handle_mmio(mmio_info_t *info)
 {
     struct vcpu *v = current;
     int i;
     const struct mmio_handler *handler = NULL;
     const struct vmmio *vmmio = &v->domain->arch.vmmio;
-    struct hsr_dabt dabt = info->dabt;
-    struct cpu_user_regs *regs = guest_cpu_user_regs();
-    register_t *r = select_user_reg(regs, dabt.reg);
 
     for ( i = 0; i < vmmio->num_entries; i++ )
     {
@@ -74,9 +90,9 @@  int handle_mmio(mmio_info_t *info)
         return 0;
 
     if ( info->dabt.write )
-        return handler->ops->write(v, info, *r, handler->priv);
+        return handle_write(handler, v, info);
     else
-        return handle_read(handler, v, info, r);
+        return handle_read(handler, v, info);
 }
 
 void register_mmio_handler(struct domain *d,
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 31d2115..1516abd 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -149,7 +149,31 @@  static void print_xen_info(void)
            debug_build() ? 'y' : 'n', print_tainted(taint_str));
 }
 
-register_t *select_user_reg(struct cpu_user_regs *regs, int reg)
+#ifdef CONFIG_ARM_32
+static inline bool_t is_zero_register(int reg)
+{
+    /* There is no zero register for ARM32 */
+    return 0;
+}
+#else
+static inline bool_t is_zero_register(int reg)
+{
+    /*
+     * For store/load and sysreg instruction, the encoding 31 always
+     * corresponds to {w,x}zr which is the zero register.
+     */
+    return (reg == 31);
+}
+#endif
+
+/*
+ * Returns a pointer to the given register value in regs, taking the
+ * processor mode (CPSR) into account.
+ *
+ * Note that this function should not be used directly but via
+ * {get,set}_user_reg.
+ */
+static register_t *select_user_reg(struct cpu_user_regs *regs, int reg)
 {
     BUG_ON( !guest_mode(regs) );
 
@@ -207,16 +231,31 @@  register_t *select_user_reg(struct cpu_user_regs *regs, int reg)
     }
 #undef REGOFFS
 #else
-    /* In 64 bit the syndrome register contains the AArch64 register
-     * number even if the trap was from AArch32 mode. Except that
-     * AArch32 R15 (PC) is encoded as 0b11111.
+    /*
+     * On 64-bit the syndrome register contains the register index as
+     * viewed in AArch64 state even if the trap was from AArch32 mode.
      */
-    if ( reg == 0x1f /* && is aarch32 guest */)
-        return &regs->pc;
+    BUG_ON(is_zero_register(reg)); /* Cannot be {w,x}zr */
     return &regs->x0 + reg;
 #endif
 }
 
+register_t get_user_reg(struct cpu_user_regs *regs, int reg)
+{
+    if ( is_zero_register(reg) )
+        return 0;
+
+    return *select_user_reg(regs, reg);
+}
+
+void set_user_reg(struct cpu_user_regs *regs, int reg, register_t value)
+{
+    if ( is_zero_register(reg) )
+        return;
+
+    *select_user_reg(regs, reg) = value;
+}
+
 static const char *decode_fsc(uint32_t fsc, int *level)
 {
     const char *msg = NULL;
@@ -1241,22 +1280,19 @@  static arm_hypercall_t arm_hypercall_table[] = {
 #ifndef NDEBUG
 static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 {
-    register_t *r;
     uint32_t reg;
     uint32_t domid = current->domain->domain_id;
     switch ( code ) {
     case 0xe0 ... 0xef:
         reg = code - 0xe0;
-        r = select_user_reg(regs, reg);
         printk("DOM%d: R%d = 0x%"PRIregister" at 0x%"PRIvaddr"\n",
-               domid, reg, *r, regs->pc);
+               domid, reg, get_user_reg(regs, reg), regs->pc);
         break;
     case 0xfd:
         printk("DOM%d: Reached %"PRIvaddr"\n", domid, regs->pc);
         break;
     case 0xfe:
-        r = select_user_reg(regs, 0);
-        printk("%c", (char)(*r & 0xff));
+        printk("%c", (char)(get_user_reg(regs, 0) & 0xff));
         break;
     case 0xff:
         printk("DOM%d: DEBUG\n", domid);
@@ -1614,7 +1650,7 @@  static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
 
 /* Read as zero and write ignore */
 static void handle_raz_wi(struct cpu_user_regs *regs,
-                          register_t *reg,
+                          int regidx,
                           bool_t read,
                           const union hsr hsr,
                           int min_el)
@@ -1625,7 +1661,7 @@  static void handle_raz_wi(struct cpu_user_regs *regs,
         return inject_undef_exception(regs, hsr);
 
     if ( read )
-        *reg = 0;
+        set_user_reg(regs, regidx, 0);
     /* else: write ignored */
 
     advance_pc(regs, hsr);
@@ -1633,7 +1669,7 @@  static void handle_raz_wi(struct cpu_user_regs *regs,
 
 /* Write only as write ignore */
 static void handle_wo_wi(struct cpu_user_regs *regs,
-                         register_t *reg,
+                         int regidx,
                          bool_t read,
                          const union hsr hsr,
                          int min_el)
@@ -1652,7 +1688,7 @@  static void handle_wo_wi(struct cpu_user_regs *regs,
 
 /* Read only as read as zero */
 static void handle_ro_raz(struct cpu_user_regs *regs,
-                          register_t *reg,
+                          int regidx,
                           bool_t read,
                           const union hsr hsr,
                           int min_el)
@@ -1666,7 +1702,7 @@  static void handle_ro_raz(struct cpu_user_regs *regs,
         return inject_undef_exception(regs, hsr);
     /* else: raz */
 
-    *reg = 0;
+    set_user_reg(regs, regidx, 0);
 
     advance_pc(regs, hsr);
 }
@@ -1675,7 +1711,7 @@  static void do_cp15_32(struct cpu_user_regs *regs,
                        const union hsr hsr)
 {
     const struct hsr_cp32 cp32 = hsr.cp32;
-    register_t *r = select_user_reg(regs, cp32.reg);
+    int regidx = cp32.reg;
     struct vcpu *v = current;
 
     if ( !check_conditional_instr(regs, hsr) )
@@ -1708,7 +1744,7 @@  static void do_cp15_32(struct cpu_user_regs *regs,
         if ( psr_mode_is_user(regs) )
             return inject_undef_exception(regs, hsr);
         if ( cp32.read )
-           *r = v->arch.actlr;
+            set_user_reg(regs, regidx, v->arch.actlr);
         break;
 
     /*
@@ -1740,13 +1776,13 @@  static void do_cp15_32(struct cpu_user_regs *regs,
     case HSR_CPREG32(PMUSERENR):
         /* RO at EL0. RAZ/WI at EL1 */
         if ( psr_mode_is_user(regs) )
-            return handle_ro_raz(regs, r, cp32.read, hsr, 0);
+            return handle_ro_raz(regs, regidx, cp32.read, hsr, 0);
         else
-            return handle_raz_wi(regs, r, cp32.read, hsr, 1);
+            return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
     case HSR_CPREG32(PMINTENSET):
     case HSR_CPREG32(PMINTENCLR):
         /* EL1 only, however MDCR_EL2.TPM==1 means EL0 may trap here also. */
-        return handle_raz_wi(regs, r, cp32.read, hsr, 1);
+        return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
     case HSR_CPREG32(PMCR):
     case HSR_CPREG32(PMCNTENSET):
     case HSR_CPREG32(PMCNTENCLR):
@@ -1763,7 +1799,7 @@  static void do_cp15_32(struct cpu_user_regs *regs,
          * Accessible at EL0 only if PMUSERENR_EL0.EN is set. We
          * emulate that register as 0 above.
          */
-        return handle_raz_wi(regs, r, cp32.read, hsr, 1);
+        return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
 
     /*
      * HCR_EL2.TIDCP
@@ -1866,7 +1902,7 @@  static void do_cp15_64(struct cpu_user_regs *regs,
 static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
 {
     const struct hsr_cp32 cp32 = hsr.cp32;
-    register_t *r = select_user_reg(regs, cp32.reg);
+    int regidx = cp32.reg;
     struct domain *d = current->domain;
 
     if ( !check_conditional_instr(regs, hsr) )
@@ -1888,9 +1924,9 @@  static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
      *    DBGPRCR
      */
     case HSR_CPREG32(DBGOSLAR):
-        return handle_wo_wi(regs, r, cp32.read, hsr, 1);
+        return handle_wo_wi(regs, regidx, cp32.read, hsr, 1);
     case HSR_CPREG32(DBGOSDLR):
-        return handle_raz_wi(regs, r, cp32.read, hsr, 1);
+        return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
 
     /*
      * MDCR_EL2.TDA
@@ -1915,6 +1951,9 @@  static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
      *    DBGOSECCR
      */
     case HSR_CPREG32(DBGDIDR):
+    {
+        uint32_t val;
+
         /*
          * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
          * is set to 0, which we emulated below.
@@ -1928,23 +1967,26 @@  static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
          *  - Version: ARMv7 v7.1
          *  - Variant and Revision bits match MDIR
          */
-        *r = (1 << 24) | (5 << 16);
-        *r |= ((d->arch.vpidr >> 20) & 0xf) | (d->arch.vpidr & 0xf);
+        val = (1 << 24) | (5 << 16);
+        val |= ((d->arch.vpidr >> 20) & 0xf) | (d->arch.vpidr & 0xf);
+        set_user_reg(regs, regidx, val);
+
         break;
+    }
 
     case HSR_CPREG32(DBGDSCRINT):
         /*
          * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
          * is set to 0, which we emulated below.
          */
-        return handle_ro_raz(regs, r, cp32.read, hsr, 1);
+        return handle_ro_raz(regs, regidx, cp32.read, hsr, 1);
 
     case HSR_CPREG32(DBGDSCREXT):
         /*
          * Implement debug status and control register as RAZ/WI.
          * The OS won't use Hardware debug if MDBGen not set.
          */
-        return handle_raz_wi(regs, r, cp32.read, hsr, 1);
+        return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
 
     case HSR_CPREG32(DBGVCR):
     case HSR_CPREG32(DBGBVR0):
@@ -1953,7 +1995,7 @@  static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
     case HSR_CPREG32(DBGWCR0):
     case HSR_CPREG32(DBGBVR1):
     case HSR_CPREG32(DBGBCR1):
-        return handle_raz_wi(regs, r, cp32.read, hsr, 1);
+        return handle_raz_wi(regs, regidx, cp32.read, hsr, 1);
 
     /*
      * CPTR_EL2.TTA
@@ -2077,7 +2119,7 @@  static void do_cp(struct cpu_user_regs *regs, const union hsr hsr)
 static void do_sysreg(struct cpu_user_regs *regs,
                       const union hsr hsr)
 {
-    register_t *x = select_user_reg(regs, hsr.sysreg.reg);
+    int regidx = hsr.sysreg.reg;
     struct vcpu *v = current;
 
     switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
@@ -2091,7 +2133,7 @@  static void do_sysreg(struct cpu_user_regs *regs,
         if ( psr_mode_is_user(regs) )
             return inject_undef_exception(regs, hsr);
         if ( hsr.sysreg.read )
-           *x = v->arch.actlr;
+            set_user_reg(regs, regidx, v->arch.actlr);
         break;
 
     /*
@@ -2100,7 +2142,7 @@  static void do_sysreg(struct cpu_user_regs *regs,
      * ARMv8 (DDI 0487A.d): D1-1508 Table D1-57
      */
     case HSR_SYSREG_MDRAR_EL1:
-        return handle_ro_raz(regs, x, hsr.sysreg.read, hsr, 1);
+        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 1);
 
     /*
      * MDCR_EL2.TDOSA
@@ -2112,9 +2154,9 @@  static void do_sysreg(struct cpu_user_regs *regs,
      *    DBGPRCR_EL1
      */
     case HSR_SYSREG_OSLAR_EL1:
-        return handle_wo_wi(regs, x, hsr.sysreg.read, hsr, 1);
+        return handle_wo_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
     case HSR_SYSREG_OSDLR_EL1:
-        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
+        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
 
     /*
      * MDCR_EL2.TDA
@@ -2134,18 +2176,18 @@  static void do_sysreg(struct cpu_user_regs *regs,
      *    DBGAUTHSTATUS_EL1
      */
     case HSR_SYSREG_MDSCR_EL1:
-        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
+        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
     case HSR_SYSREG_MDCCSR_EL0:
         /*
          * Accessible at EL0 only if MDSCR_EL1.TDCC is set to 0. We emulate that
          * register as RAZ/WI above. So RO at both EL0 and EL1.
          */
-        return handle_ro_raz(regs, x, hsr.sysreg.read, hsr, 0);
+        return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
     HSR_SYSREG_DBG_CASES(DBGBVR):
     HSR_SYSREG_DBG_CASES(DBGBCR):
     HSR_SYSREG_DBG_CASES(DBGWVR):
     HSR_SYSREG_DBG_CASES(DBGWCR):
-        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
+        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
 
     /*
      * MDCR_EL2.TPM
@@ -2169,13 +2211,13 @@  static void do_sysreg(struct cpu_user_regs *regs,
          * Accessible from EL1 only, but if EL0 trap happens handle as
          * undef.
          */
-        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
+        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
     case HSR_SYSREG_PMUSERENR_EL0:
         /* RO at EL0. RAZ/WI at EL1 */
         if ( psr_mode_is_user(regs) )
-            return handle_ro_raz(regs, x, hsr.sysreg.read, hsr, 0);
+            return handle_ro_raz(regs, regidx, hsr.sysreg.read, hsr, 0);
         else
-            return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
+            return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
     case HSR_SYSREG_PMCR_EL0:
     case HSR_SYSREG_PMCNTENSET_EL0:
     case HSR_SYSREG_PMCNTENCLR_EL0:
@@ -2192,7 +2234,7 @@  static void do_sysreg(struct cpu_user_regs *regs,
          * Accessible at EL0 only if PMUSERENR_EL0.EN is set. We
          * emulate that register as 0 above.
          */
-        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
+        return handle_raz_wi(regs, regidx, hsr.sysreg.read, hsr, 1);
 
     /*
      * !CNTHCTL_EL2.EL1PCEN
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index dba2449..b37a7c0 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1304,7 +1304,6 @@  static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
 {
     struct vcpu *v = current;
     struct hsr_sysreg sysreg = hsr.sysreg;
-    register_t *r = select_user_reg(regs, sysreg.reg);
 
     ASSERT (hsr.ec == HSR_EC_SYSREG);
 
@@ -1318,7 +1317,7 @@  static int vgic_v3_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
     case HSR_SYSREG_ICC_SGI1R_EL1:
         /* WO */
         if ( !sysreg.read )
-            return vgic_v3_to_sgi(v, *r);
+            return vgic_v3_to_sgi(v, get_user_reg(regs, sysreg.reg));
         else
         {
             gprintk(XENLOG_WARNING, "Reading SGI1R_EL1 - WO register\n");
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index b9c0b41..f636705 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -249,32 +249,49 @@  static int vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_t *r, int read)
 static int vtimer_emulate_cp32(struct cpu_user_regs *regs, union hsr hsr)
 {
     struct hsr_cp32 cp32 = hsr.cp32;
-    uint32_t *r = (uint32_t *)select_user_reg(regs, cp32.reg);
+    /*
+     * Initialize to zero to avoid leaking data if there is an
+     * implementation error in the emulation (such as not correctly
+     * setting r).
+     */
+    uint32_t r = 0;
+    int res;
+
 
     if ( cp32.read )
         perfc_incr(vtimer_cp32_reads);
     else
         perfc_incr(vtimer_cp32_writes);
 
+    if ( !cp32.read )
+        r = get_user_reg(regs, cp32.reg);
+
     switch ( hsr.bits & HSR_CP32_REGS_MASK )
     {
     case HSR_CPREG32(CNTP_CTL):
-        return vtimer_cntp_ctl(regs, r, cp32.read);
+        res = vtimer_cntp_ctl(regs, &r, cp32.read);
+        break;
 
     case HSR_CPREG32(CNTP_TVAL):
-        return vtimer_cntp_tval(regs, r, cp32.read);
+        res = vtimer_cntp_tval(regs, &r, cp32.read);
+        break;
 
     default:
         return 0;
     }
+
+    if ( res && cp32.read )
+        set_user_reg(regs, cp32.reg, r);
+
+    return res;
 }
 
 static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
 {
     struct hsr_cp64 cp64 = hsr.cp64;
-    uint32_t *r1 = (uint32_t *)select_user_reg(regs, cp64.reg1);
-    uint32_t *r2 = (uint32_t *)select_user_reg(regs, cp64.reg2);
-    uint64_t x = (uint64_t)(*r1) | ((uint64_t)(*r2) << 32);
+    uint32_t r1 = get_user_reg(regs, cp64.reg1);
+    uint32_t r2 = get_user_reg(regs, cp64.reg2);
+    uint64_t x = (uint64_t)r1 | ((uint64_t)r2 << 32);
 
     if ( cp64.read )
         perfc_incr(vtimer_cp64_reads);
@@ -294,8 +311,8 @@  static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
 
     if ( cp64.read )
     {
-        *r1 = (uint32_t)(x & 0xffffffff);
-        *r2 = (uint32_t)(x >> 32);
+        set_user_reg(regs, cp64.reg1, x & 0xffffffff);
+        set_user_reg(regs, cp64.reg2, x >> 32);
     }
 
     return 1;
@@ -311,14 +328,16 @@  static int vtimer_emulate_sysreg32(struct cpu_user_regs *regs, union hsr hsr,
                                    vtimer_sysreg32_fn_t fn)
 {
     struct hsr_sysreg sysreg = hsr.sysreg;
-    register_t *x = select_user_reg(regs, sysreg.reg);
-    uint32_t r = *x;
+    uint32_t r = 0;
     int ret;
 
+    if ( !sysreg.read )
+        r = get_user_reg(regs, sysreg.reg);
+
     ret = fn(regs, &r, sysreg.read);
 
     if ( ret && sysreg.read )
-        *x = r;
+        set_user_reg(regs, sysreg.reg, r);
 
     return ret;
 }
@@ -327,9 +346,23 @@  static int vtimer_emulate_sysreg64(struct cpu_user_regs *regs, union hsr hsr,
                                    vtimer_sysreg64_fn_t fn)
 {
     struct hsr_sysreg sysreg = hsr.sysreg;
-    uint64_t *x = select_user_reg(regs, sysreg.reg);
+    /*
+     * Initialize to zero to avoid leaking data if there is an
+     * implementation error in the emulation (such as not correctly
+     * setting x).
+     */
+    uint64_t x = 0;
+    int ret;
+
+    if ( !sysreg.read )
+        x = get_user_reg(regs, sysreg.reg);
 
-    return fn(regs, x, sysreg.read);
+    ret = fn(regs, &x, sysreg.read);
+
+    if ( ret && sysreg.read )
+        set_user_reg(regs, sysreg.reg, x);
+
+    return ret;
 }
 
 static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
diff --git a/xen/include/asm-arm/regs.h b/xen/include/asm-arm/regs.h
index 56d53d6..2440edb 100644
--- a/xen/include/asm-arm/regs.h
+++ b/xen/include/asm-arm/regs.h
@@ -50,11 +50,8 @@ 
 
 #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 register_t *select_user_reg(struct cpu_user_regs *regs, int reg);
+register_t get_user_reg(struct cpu_user_regs *regs, int reg);
+void set_user_reg(struct cpu_user_regs *regs, int reg, register_t val);
 
 #endif