[PATCH 0/4] xen/arm64: Emulate correctly the register {w, x}zr

[PATCH 0/4] xen/arm64: Emulate correctly the register {w, x}zr

[Julien Grall]

Hi all,

This patch series aims to fix the emulation of register {w,x}zr for ARM64.

A branch is available with all the patches applied:

git://xenbits.xen.org/people/julieng/xen-unstable.git branch handle-xzr-v1

Regards,

Julien Grall (4):
  xen/arm: io: handle_read: Use a local variable to store dabt
  xen/arm64: Document the register mapping aarch64 <-> aarch32
  xen/arm: vtimer: Introduce vtimer_emulate_sysreg{32,64}
  xen/arm64: Emulate correctly the register {w,x}zr

 xen/arch/arm/io.c                     |  37 ++++++++---
 xen/arch/arm/traps.c                  | 121 ++++++++++++++++++++++------------
 xen/arch/arm/vgic-v3.c                |   3 +-
 xen/arch/arm/vtimer.c                 |  96 ++++++++++++++++++++-------
 xen/include/asm-arm/arm64/processor.h |   7 +-
 xen/include/asm-arm/regs.h            |   7 +-
 6 files changed, 189 insertions(+), 82 deletions(-)

-- 
2.1.4

[PATCH 1/4] xen/arm: io: handle_read: Use a local variable to store dabt

[Julien Grall]

Rather than getting dabt every time through info->dabt, introduce a
local variable and use it.

Also fix a coding style error in the if condition.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/io.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index de5765a..7e29943 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -26,7 +26,8 @@
 static int handle_read(const struct mmio_handler *handler, struct vcpu *v,
                        mmio_info_t *info, register_t *r)
 {
-    uint8_t size = (1 << info->dabt.size) * 8;
+    const struct hsr_dabt dabt = info->dabt;
+    uint8_t size = (1 << dabt.size) * 8;
 
     if ( !handler->ops->read(v, info, r, handler->priv) )
         return 0;
@@ -36,7 +37,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 ( info->dabt.sign && (*r & (1UL << (size - 1)) ))
+    if ( dabt.sign && (*r & (1UL << (size - 1))) )
     {
         /*
          * We are relying on register_t using the same as
-- 
2.1.4

Re: [PATCH 1/4] xen/arm: io: handle_read: Use a local variable to store dabt

[Ian Campbell]

On Fri, 2015-12-11 at 15:28 +0000, Julien Grall wrote:
> Rather than getting dabt every time through info->dabt, introduce a
> local variable and use it.
> 
> Also fix a coding style error in the if condition.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  xen/arch/arm/io.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index de5765a..7e29943 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -26,7 +26,8 @@
>  static int handle_read(const struct mmio_handler *handler, struct vcpu
> *v,
>                         mmio_info_t *info, register_t *r)
>  {
> -    uint8_t size = (1 << info->dabt.size) * 8;
> +    const struct hsr_dabt dabt = info->dabt;
> +    uint8_t size = (1 << dabt.size) * 8;
>  
>      if ( !handler->ops->read(v, info, r, handler->priv) )
>          return 0;
> @@ -36,7 +37,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 ( info->dabt.sign && (*r & (1UL << (size - 1)) ))
> +    if ( dabt.sign && (*r & (1UL << (size - 1))) )
>      {
>          /*
>           * We are relying on register_t using the same as

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2/4] xen/arm64: Document the register mapping aarch64 <-> aarch32

[Julien Grall]

The mapping between aarch64 and aarch32 has not been chosen in random.
It's based on D1.20.1 in ARM DDI 0487A.d.

The section is not obvious to find in the spec, so make it clear for the
anyone else.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/include/asm-arm/arm64/processor.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/include/asm-arm/arm64/processor.h b/xen/include/asm-arm/arm64/processor.h
index 3a9c0cd..fef35a5 100644
--- a/xen/include/asm-arm/arm64/processor.h
+++ b/xen/include/asm-arm/arm64/processor.h
@@ -15,7 +15,12 @@
 /* On stack VCPU state */
 struct cpu_user_regs
 {
-    /*         Aarch64       Aarch32 */
+    /*
+     * The mapping AArch64 <-> AArch32 is based on D1.20.1 in ARM DDI
+     * 0487A.d.
+     *
+     *         AArch64       AArch32
+     */
     __DECL_REG(x0,           r0/*_usr*/);
     __DECL_REG(x1,           r1/*_usr*/);
     __DECL_REG(x2,           r2/*_usr*/);
-- 
2.1.4

Re: [PATCH 2/4] xen/arm64: Document the register mapping aarch64 <-> aarch32

[Ian Campbell]

On Fri, 2015-12-11 at 15:28 +0000, Julien Grall wrote:
> The mapping between aarch64 and aarch32 has not been chosen in random.
> It's based on D1.20.1 in ARM DDI 0487A.d.
> 
> The section is not obvious to find in the spec, so make it clear for the
> anyone else.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  xen/include/asm-arm/arm64/processor.h | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/arm64/processor.h b/xen/include/asm-
> arm/arm64/processor.h
> index 3a9c0cd..fef35a5 100644
> --- a/xen/include/asm-arm/arm64/processor.h
> +++ b/xen/include/asm-arm/arm64/processor.h
> @@ -15,7 +15,12 @@
>  /* On stack VCPU state */
>  struct cpu_user_regs
>  {
> -    /*         Aarch64       Aarch32 */
> +    /*
> +     * The mapping AArch64 <-> AArch32 is based on D1.20.1 in ARM DDI
> +     * 0487A.d.
> +     *
> +     *         AArch64       AArch32
> +     */
>      __DECL_REG(x0,           r0/*_usr*/);
>      __DECL_REG(x1,           r1/*_usr*/);
>      __DECL_REG(x2,           r2/*_usr*/);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 3/4] xen/arm: vtimer: Introduce vtimer_emulate_sysreg{32, 64}

[Julien Grall]

Factorize the code to emulate a a 32-bit/64-bit sysreg in specific
helpers.

While this is currently not necessary, it will be helpful in a following
patch to handle properly some registers.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/vtimer.c | 47 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 629feb4..b9c0b41 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -302,11 +302,39 @@ static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
 }
 
 #ifdef CONFIG_ARM_64
-static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
+typedef int (*vtimer_sysreg32_fn_t)(struct cpu_user_regs *regs, uint32_t *r,
+                                    int read);
+typedef int (*vtimer_sysreg64_fn_t)(struct cpu_user_regs *regs, uint64_t *r,
+                                    int read);
+
+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 = (uint32_t)*x;
+    uint32_t r = *x;
+    int ret;
+
+    ret = fn(regs, &r, sysreg.read);
+
+    if ( ret && sysreg.read )
+        *x = r;
+
+    return ret;
+}
+
+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);
+
+    return fn(regs, x, sysreg.read);
+}
+
+static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
+{
+    struct hsr_sysreg sysreg = hsr.sysreg;
 
     if ( sysreg.read )
         perfc_incr(vtimer_sysreg_reads);
@@ -316,20 +344,11 @@ static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
     switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
     {
     case HSR_SYSREG_CNTP_CTL_EL0:
-        if ( !vtimer_cntp_ctl(regs, &r, sysreg.read) )
-            return 0;
-        if ( sysreg.read )
-            *x = r;
-        return 1;
+        return vtimer_emulate_sysreg32(regs, hsr, vtimer_cntp_ctl);
     case HSR_SYSREG_CNTP_TVAL_EL0:
-        if ( !vtimer_cntp_tval(regs, &r, sysreg.read) )
-            return 0;
-        if ( sysreg.read )
-            *x = r;
-        return 1;
-
+        return vtimer_emulate_sysreg32(regs, hsr, vtimer_cntp_tval);
     case HSR_SYSREG_CNTP_CVAL_EL0:
-        return vtimer_cntp_cval(regs, x, sysreg.read);
+        return vtimer_emulate_sysreg64(regs, hsr, vtimer_cntp_cval);
 
     default:
         return 0;
-- 
2.1.4

Re: [PATCH 3/4] xen/arm: vtimer: Introduce vtimer_emulate_sysreg{32, 64}

[Ian Campbell]

On Fri, 2015-12-11 at 15:28 +0000, Julien Grall wrote:
> Factorize the code to emulate a a 32-bit/64-bit sysreg in specific
> helpers.
> 
> While this is currently not necessary, it will be helpful in a following
> patch to handle properly some registers.

Looks like it would be potentially be usefully more generic too, but not
now.

> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 3/4] xen/arm: vtimer: Introduce vtimer_emulate_sysreg{32, 64}

[Julien Grall]

Hi Ian,

On 15/12/15 10:38, Ian Campbell wrote:
> On Fri, 2015-12-11 at 15:28 +0000, Julien Grall wrote:
>> Factorize the code to emulate a a 32-bit/64-bit sysreg in specific
>> helpers.
>>
>> While this is currently not necessary, it will be helpful in a following
>> patch to handle properly some registers.
> 
> Looks like it would be potentially be usefully more generic too, but not
> now.

I though about it and wanted to rework the sysreg emulation at the same
time. Although it was too much for this series.

>> Signed-off-by: Julien Grall <julien.grall@citrix.com>

> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Thanks!

Regards,

-- 
Julien Grall

[PATCH 4/4] xen/arm64: Emulate correctly the register {w, x}zr

[Julien Grall]

On AArch64, the encoding 31 could be used to represent {x,w}sp or
{x,w}zr (See C1.2.4 in ARM DDI 0486A.d). The choice will depend how the
register field is interpreted by the instruction.

All the instructions trapped by Xen (either via a sysreg access or data
abort) interprets the encoding 31 as zr. Therefore we don't have to
worry about decoding the instruction.

The register zr reprents the zero register, i.e it will always
return 0 and write to it is ignored. To handle properly this properties,
2 new helpers have been introduced {get,set}_user_reg to read/write a
value from/to a register. All the call 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.

Reported-by: Marc Zyngier <Marc.Zyngier@arm.com>
Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/io.c          |  34 +++++++++----
 xen/arch/arm/traps.c       | 121 +++++++++++++++++++++++++++++----------------
 xen/arch/arm/vgic-v3.c     |   3 +-
 xen/arch/arm/vtimer.c      |  59 +++++++++++++++++-----
 xen/include/asm-arm/regs.h |   7 +--
 5 files changed, 153 insertions(+), 71 deletions(-)

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 c49bd3f..6e2e50e 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -149,7 +149,14 @@ 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)
+/*
+ * 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 +214,42 @@ 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;
     return &regs->x0 + reg;
 #endif
 }
 
+register_t get_user_reg(struct cpu_user_regs *regs, int reg)
+{
+#ifdef CONFIG_ARM_64
+    /*
+     * For store/load and sysreg instruction, the encoding 31 always
+     * correspond to {w,x}zr which is the zero register.
+     */
+    if ( reg == 31 )
+        return 0;
+#endif
+
+    return *select_user_reg(regs, reg);
+}
+
+void set_user_reg(struct cpu_user_regs *regs, int reg, register_t value)
+{
+#ifdef CONFIG_ARM_64
+    /*
+     * For store/load and sysreg instruction, the encoding 31 always
+     * correspond to {w,x}zr which is the zero register.
+     */
+    if ( reg == 31 )
+        return;
+#endif
+
+    *select_user_reg(regs, reg) = value;
+}
+
 static const char *decode_fsc(uint32_t fsc, int *level)
 {
     const char *msg = NULL;
@@ -1240,22 +1273,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);
@@ -1613,7 +1643,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)
@@ -1624,7 +1654,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);
@@ -1632,7 +1662,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)
@@ -1651,7 +1681,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)
@@ -1665,7 +1695,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);
 }
@@ -1674,7 +1704,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) )
@@ -1707,7 +1737,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;
 
     /*
@@ -1739,13 +1769,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):
@@ -1762,7 +1792,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
@@ -1803,6 +1833,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
         inject_undef_exception(regs, hsr);
         return;
     }
+
     advance_pc(regs, hsr);
 }
 
@@ -1865,7 +1896,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) )
@@ -1887,9 +1918,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
@@ -1914,6 +1945,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.
@@ -1927,23 +1961,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):
@@ -1952,7 +1989,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
@@ -2076,7 +2113,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 )
@@ -2090,7 +2127,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;
 
     /*
@@ -2099,7 +2136,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
@@ -2111,9 +2148,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
@@ -2133,18 +2170,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
@@ -2168,13 +2205,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:
@@ -2191,7 +2228,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 985e866..56f98ef 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1305,7 +1305,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);
 
@@ -1319,7 +1318,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
 
-- 
2.1.4

Re: [PATCH 4/4] xen/arm64: Emulate correctly the register {w, x}zr

[Ian Campbell]

On Fri, 2015-12-11 at 15:28 +0000, Julien Grall wrote:
> On AArch64, the encoding 31 could be used to represent {x,w}sp or
> {x,w}zr (See C1.2.4 in ARM DDI 0486A.d). The choice will depend how the
> register field is interpreted by the instruction.
> 
> All the instructions trapped by Xen (either via a sysreg access or data
> abort) interprets the encoding 31 as zr. Therefore we don't have to
> worry about decoding the instruction.

Is decoding the only way we could tell? i.e. there's no indication in the
syndrome reg?

Is there no way a data abort could result from an access which involved SP
rather than XZ? e.g. if the OS was to (stupidly) point SP at an MMIO area
and then try a stp x0, x1, [sp] for example? Ah, perhaps in that case we
would get a trap with x0 or x1 but sp would be in the FAR and not in the
HSR?

Or maybe a less lunatic case, could xenaccess not result in a faulting
stack access? I suppose the answer there is that xenaccess faults are
handled without actually caring which register it was and then the
instruction is replayed in guest context?

Aside: if an stp traps, does the h/v get two exits, one for each register?
I suppose it must.

> The register zr reprents the zero register, i.e it will always

"represents"

> return 0 and write to it is ignored. To handle properly this properties,

"these properties"

> 2 new helpers have been introduced {get,set}_user_reg to read/write a
> value from/to a register. All the call to select_user_reg have been

calls

> 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.

Does this "Just Work" for arm32 Xen then?

What happens if an aarch32 guest accesses r15 in one of these ways on an
aarch64 hypervisor? Is it verboten in ARM v8 or something else? E1.2.3
describes it as deprecated, but not forbidden, but I suspect that "and the
source address of the ldr is in MMIO space" is not covered there or
something.

> @@ -207,16 +214,42 @@ 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;
>      return &regs->x0 + reg;

If reg were == 0x1f here then it would end up accessing regs->sp, which is
only valid for traps from Xen to Xen (i.e. interrupt stack frames), whichif
it were to somehow happen would lead to some rather interesting behaviour I
fear. I'm pretty sure it can't happen, but I'd be happier with an explicit
assert/BUG_ON.

Alternatively we could return NULL here to represent XZR and handle that
appropriately in the {get,set}_user_reg, replacing the check for reg == 31 in both places.

(FWIW the actual guest stack pointer state is in either SP_EL{0,1} or the
aarch sp_* in x13, x17 etc.)

>  #endif
>  }
>  
> +register_t get_user_reg(struct cpu_user_regs *regs, int reg)
> +{
> +#ifdef CONFIG_ARM_64
> +    /*
> +     * For store/load and sysreg instruction, the encoding 31 always
> +     * correspond to {w,x}zr which is the zero register.
> +     */
> +    if ( reg == 31 )
> +        return 0;
> +#endif
> +
> +    return *select_user_reg(regs, reg);
> +}
> +
> +void set_user_reg(struct cpu_user_regs *regs, int reg, register_t value)
> +{
> +#ifdef CONFIG_ARM_64
> +    /*
> +     * For store/load and sysreg instruction, the encoding 31 always
> +     * correspond to {w,x}zr which is the zero register.
> +     */
> +    if ( reg == 31 )
> +        return;
> +#endif
> +
> +    *select_user_reg(regs, reg) = value;
> +}
> +
> [...]

All of the other mostly mechanical changes look OK from a quick glance.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 4/4] xen/arm64: Emulate correctly the register {w, x}zr

[Julien Grall]

Hi Ian,

On 15/12/15 11:10, Ian Campbell wrote:
> On Fri, 2015-12-11 at 15:28 +0000, Julien Grall wrote:
>> On AArch64, the encoding 31 could be used to represent {x,w}sp or
>> {x,w}zr (See C1.2.4 in ARM DDI 0486A.d). The choice will depend how the
>> register field is interpreted by the instruction.
>>
>> All the instructions trapped by Xen (either via a sysreg access or data
>> abort) interprets the encoding 31 as zr. Therefore we don't have to
>> worry about decoding the instruction.
> 
> Is decoding the only way we could tell? i.e. there's no indication in the
> syndrome reg?

Unfortunately yes, the syndrome reg will only contain the encoding of
the transferred register.

However, by looking to the list of instruction that could be trapped by
Xen and contain valid ISS, the encoding 31 will always mean zr.

> Is there no way a data abort could result from an access which involved SP
> rather than XZ? e.g. if the OS was to (stupidly) point SP at an MMIO area
> and then try a stp x0, x1, [sp] for example? Ah, perhaps in that case we
> would get a trap with x0 or x1 but sp would be in the FAR and not in the
> HSR?

First if the instruction stp trap into Xen, the ISS would be invalid.

For AArch64, only loads and store of a single general-purpose register
would result to a valid ISS (see D7-1880 in ARM DDI 0487A.d).

If we take an instruction producing a valid ISS:

ldr x0, [x2]
str x0, [x2]

The ISS will always contains the transfered register, i.e x0 here. The
faulting address (x2) will be in FAR.

> 
> Or maybe a less lunatic case, could xenaccess not result in a faulting
> stack access? I suppose the answer there is that xenaccess faults are
> handled without actually caring which register it was and then the
> instruction is replayed in guest context?

xenaccess can't rely on the instruction syndrome because the fault may
happen with instruction producing invalid ISS.

If we ever need the register in xenaccess we have to decode by hand
every instruction.

> 
> Aside: if an stp traps, does the h/v get two exits, one for each register?
> I suppose it must.
> 
>> The register zr reprents the zero register, i.e it will always
> 
> "represents"
> 
>> return 0 and write to it is ignored. To handle properly this properties,
> 
> "these properties"
> 
>> 2 new helpers have been introduced {get,set}_user_reg to read/write a
>> value from/to a register. All the call to select_user_reg have been
> 
> calls
> 
>> 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.
> 
> Does this "Just Work" for arm32 Xen then?
> 
> What happens if an aarch32 guest accesses r15 in one of these ways on an
> aarch64 hypervisor? Is it verboten in ARM v8 or something else? E1.2.3
> describes it as deprecated, but not forbidden, but I suspect that "and the
> source address of the ldr is in MMIO space" is not covered there or
> something.

Based on the description of the ISS encoding for data abort, the
syndrome would be invalid.

Our data abort handler will inject a data abort to the guest for any
invalid instruction.

If we ever want to support the guest to write pc in an emulated MMIO, we
would have to decode the instruction.

> 
>> @@ -207,16 +214,42 @@ 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;
>>      return &regs->x0 + reg;
> 
> If reg were == 0x1f here then it would end up accessing regs->sp, which is
> only valid for traps from Xen to Xen (i.e. interrupt stack frames), whichif
> it were to somehow happen would lead to some rather interesting behaviour I
> fear. I'm pretty sure it can't happen, but I'd be happier with an explicit
> assert/BUG_ON.

I will had a BUG_ON Here.

> 
> Alternatively we could return NULL here to represent XZR and handle that
> appropriately in the {get,set}_user_reg, replacing the check for reg == 31 in both places.

I would rather avoid select_user_regs to return NULL. The explicit
encoding check in {get,set}_user_reg is better.

Regards,

-- 
Julien Grall

Re: [PATCH 4/4] xen/arm64: Emulate correctly the register {w, x}zr

[Ian Campbell]

On Tue, 2015-12-15 at 11:41 +0000, Julien Grall wrote:
> Hi Ian,

Thanks for the replies, I think mostly adding the further exposition from
this thread to the commit message would make this patch OK.
> 
> On 15/12/15 11:10, Ian Campbell wrote:
> > On Fri, 2015-12-11 at 15:28 +0000, Julien Grall wrote:
> > > On AArch64, the encoding 31 could be used to represent {x,w}sp or
> > > {x,w}zr (See C1.2.4 in ARM DDI 0486A.d). The choice will depend how
> > > the
> > > register field is interpreted by the instruction.
> > > 
> > > All the instructions trapped by Xen (either via a sysreg access or
> > > data
> > > abort) interprets the encoding 31 as zr. Therefore we don't have to
> > > worry about decoding the instruction.
> > 
> > Is decoding the only way we could tell? i.e. there's no indication in
> > the
> > syndrome reg?
> 
> Unfortunately yes, the syndrome reg will only contain the encoding of
> the transferred register.
> 
> However, by looking to the list of instruction that could be trapped by
> Xen and contain valid ISS, the encoding 31 will always mean zr.

Great.

> > Is there no way a data abort could result from an access which involved
> > SP
> > rather than XZ? e.g. if the OS was to (stupidly) point SP at an MMIO
> > area
> > and then try a stp x0, x1, [sp] for example? Ah, perhaps in that case
> > we
> > would get a trap with x0 or x1 but sp would be in the FAR and not in
> > the
> > HSR?
> 
> First if the instruction stp trap into Xen, the ISS would be invalid.
> 
> For AArch64, only loads and store of a single general-purpose register
> would result to a valid ISS (see D7-1880 in ARM DDI 0487A.d).
> 
> If we take an instruction producing a valid ISS:
> 
> ldr x0, [x2]
> str x0, [x2]
> 
> The ISS will always contains the transfered register, i.e x0 here. The
> faulting address (x2) will be in FAR.

Great. And the SP can only appear as the [sp] slot, so ldr x0 [sp] but
never ldr sp [x0], so that's ok for us too.
> 
> > Or maybe a less lunatic case, could xenaccess not result in a faulting
> > stack access? I suppose the answer there is that xenaccess faults are
> > handled without actually caring which register it was and then the
> > instruction is replayed in guest context?
> 
> xenaccess can't rely on the instruction syndrome because the fault may
> happen with instruction producing invalid ISS.

Right, this was my theory too.

> > What happens if an aarch32 guest accesses r15 in one of these ways on an
> > aarch64 hypervisor? Is it verboten in ARM v8 or something else? E1.2.3
> > describes it as deprecated, but not forbidden, but I suspect that "and the
> > source address of the ldr is in MMIO space" is not covered there or
> > something.
> 
> Based on the description of the ISS encoding for data abort, the
> syndrome would be invalid.
> 
> Our data abort handler will inject a data abort to the guest for any
> invalid instruction.
> 
> If we ever want to support the guest to write pc in an emulated MMIO, we
> would have to decode the instruction.

I doubt we would want to, but so long as the worst which can happen is that
we kill the guest (rather than the host) then I'm happy.

> > Alternatively we could return NULL here to represent XZR and handle that
> > appropriately in the {get,set}_user_reg, replacing the check for reg == 31 in both places.
> 
> I would rather avoid select_user_regs to return NULL. The explicit
> encoding check in {get,set}_user_reg is better.

OK.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/4] xen/arm64: Emulate correctly the register {w, x}zr

[Julien Grall]

Hi Ian,

On 11/12/15 15:28, Julien Grall wrote:
> Hi all,
> 
> This patch series aims to fix the emulation of register {w,x}zr for ARM64.
> 
> A branch is available with all the patches applied:
> 
> git://xenbits.xen.org/people/julieng/xen-unstable.git branch handle-xzr-v1
> 
> Regards,
> 
> Julien Grall (4):
>   xen/arm: io: handle_read: Use a local variable to store dabt
>   xen/arm64: Document the register mapping aarch64 <-> aarch32
>   xen/arm: vtimer: Introduce vtimer_emulate_sysreg{32,64}

The first 3 patches are acked. Would it be possible to commit them so I
only need to resent patch #4?

Regards,

-- 
Julien Grall

Re: [PATCH 0/4] xen/arm64: Emulate correctly the register {w, x}zr

[Ian Campbell]

On Thu, 2015-12-17 at 17:17 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 11/12/15 15:28, Julien Grall wrote:
> > Hi all,
> > 
> > This patch series aims to fix the emulation of register {w,x}zr for
> > ARM64.
> > 
> > A branch is available with all the patches applied:
> > 
> > git://xenbits.xen.org/people/julieng/xen-unstable.git branch handle-
> > xzr-v1
> > 
> > Regards,
> > 
> > Julien Grall (4):
> >   xen/arm: io: handle_read: Use a local variable to store dabt
> >   xen/arm64: Document the register mapping aarch64 <-> aarch32
> >   xen/arm: vtimer: Introduce vtimer_emulate_sysreg{32,64}
> 
> The first 3 patches are acked. Would it be possible to commit them so I
> only need to resent patch #4?

I've not got time to commit anything today, but please just resend #4 (as a
reply to v1) and I'll try and DTRT upon commit.

Ian.

> 
> Regards,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/4] xen/arm64: Emulate correctly the register {w, x}zr

[Ian Campbell]

On Thu, 2015-12-17 at 17:36 +0000, Ian Campbell wrote:
> On Thu, 2015-12-17 at 17:17 +0000, Julien Grall wrote:
> > Hi Ian,
> > 
> > On 11/12/15 15:28, Julien Grall wrote:
> > > Hi all,
> > > 
> > > This patch series aims to fix the emulation of register {w,x}zr for
> > > ARM64.
> > > 
> > > A branch is available with all the patches applied:
> > > 
> > > git://xenbits.xen.org/people/julieng/xen-unstable.git branch handle-
> > > xzr-v1
> > > 
> > > Regards,
> > > 
> > > Julien Grall (4):
> > >   xen/arm: io: handle_read: Use a local variable to store dabt
> > >   xen/arm64: Document the register mapping aarch64 <-> aarch32
> > >   xen/arm: vtimer: Introduce vtimer_emulate_sysreg{32,64}
> > 
> > The first 3 patches are acked. Would it be possible to commit them so I
> > only need to resent patch #4?
> 
> I've not got time to commit anything today, but please just resend #4 (as a
> reply to v1) and I'll try and DTRT upon commit.

I've now applied the first three. However since you have since moved on I
won't expect a resend of #4 (or even that you will see this mail other than
in the archives) so I'll add this to my own TODO list.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel