[PATCH 00/19] xen: arm: cleanup traps.c

[PATCH 00/19] xen: arm: cleanup traps.c

[Ian Campbell]

While working on reenabling 32-bit user space on arm63 I concluded that
the trap handling in traps.c had grown into a twisty confusing mess.
Lets try and sort that out.

This series contains to halves (after a couple of preparatory cleanups).

First clean up the goto maze which we've found ourselves in, by
providing a selection of handle_* helpers e.g. for raz/ro etc and by
calling those and the existing inject_* helpers directly instead of
trying to have only one call to each of the latter by using goto. The
handle_* helpers can also deal with the minimum allowable exception
level, which simplifies things further.

To keep things simpler I've used "return handle_..." when the caller and
callee both return void, since that avoids the need for 3 more lines (2
braces and the return), I think this improves clarity.

Second go through init_traps and for each bit there consolidate the
handling for each type of trap (e.g. do_cp15_32, do_cp15_64, do_sysreg
etc) such that all the registers whose traps are associated with that
bit are kept together beneath a comment which documents why those bits
are trapped, references the appropriate section of the ARMv7 and ARMv8
ARM (the v8 one in particular has a series of very useful tables per
bit) and notes which registers are not explicitly handled (and therefore
take the default case).

For traps which have no explicit handling (i.e. those which trap
implementation defined registers) and which always hit the default case
add the comment above that instead.

Do the same for the GICv3 ICC traps and timer traps.

There is probably scope for doing more, i.e. refactoring related
functionality into subsystem helpers (like we do for vtimer) and even
moving into separate files, but I think this is a good start.

This is a lot of patches, sorry, because I wanted to mostly go through
the trap bits one at a time per patch to keep each one manageable,
although I did end up compressing some of the more obvious ones.

Ian.

[PATCH 01/19] xen: arm: constify union hsr and struct hsr_* where possible.

[Ian Campbell]

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index aaa9d93..69b9513 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -447,7 +447,7 @@ static vaddr_t exception_handler64(struct cpu_user_regs *regs, vaddr_t offset)
 static void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len)
 {
     vaddr_t handler;
-    union hsr esr = {
+    const union hsr esr = {
         .iss = 0,
         .len = instr_len,
         .ec = HSR_EC_UNKNOWN,
@@ -1141,7 +1141,7 @@ int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc)
 }
 
 #ifdef CONFIG_ARM_64
-static void do_trap_brk(struct cpu_user_regs *regs, union hsr hsr)
+static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
 {
     /* HCR_EL2.TGE and MDCR_EL2.TDE are not set so we never receive
      * software breakpoint exception for EL1 and EL0 here.
@@ -1488,7 +1488,8 @@ static const unsigned short cc_map[16] = {
         0                       /* NV                     */
 };
 
-static int check_conditional_instr(struct cpu_user_regs *regs, union hsr hsr)
+static int check_conditional_instr(struct cpu_user_regs *regs,
+                                   const union hsr hsr)
 {
     unsigned long cpsr, cpsr_cond;
     int cond;
@@ -1533,7 +1534,7 @@ static int check_conditional_instr(struct cpu_user_regs *regs, union hsr hsr)
     return 1;
 }
 
-static void advance_pc(struct cpu_user_regs *regs, union hsr hsr)
+static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
 {
     unsigned long itbits, cond, cpsr = regs->cpsr;
 
@@ -1574,9 +1575,9 @@ static void advance_pc(struct cpu_user_regs *regs, union hsr hsr)
 }
 
 static void do_cp15_32(struct cpu_user_regs *regs,
-                       union hsr hsr)
+                       const union hsr hsr)
 {
-    struct hsr_cp32 cp32 = hsr.cp32;
+    const struct hsr_cp32 cp32 = hsr.cp32;
     uint32_t *r = (uint32_t*)select_user_reg(regs, cp32.reg);
     struct vcpu *v = current;
 
@@ -1659,7 +1660,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
 }
 
 static void do_cp15_64(struct cpu_user_regs *regs,
-                       union hsr hsr)
+                       const union hsr hsr)
 {
     if ( !check_conditional_instr(regs, hsr) )
     {
@@ -1676,7 +1677,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,
         break;
     default:
         {
-            struct hsr_cp64 cp64 = hsr.cp64;
+            const struct hsr_cp64 cp64 = hsr.cp64;
 
             gdprintk(XENLOG_ERR,
                      "%s p15, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
@@ -1692,9 +1693,9 @@ static void do_cp15_64(struct cpu_user_regs *regs,
     advance_pc(regs, hsr);
 }
 
-static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
+static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
 {
-    struct hsr_cp32 cp32 = hsr.cp32;
+    const struct hsr_cp32 cp32 = hsr.cp32;
     uint32_t *r = (uint32_t *)select_user_reg(regs, cp32.reg);
     struct domain *d = current->domain;
 
@@ -1784,9 +1785,9 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
     advance_pc(regs, hsr);
 }
 
-static void do_cp14_dbg(struct cpu_user_regs *regs, union hsr hsr)
+static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
 {
-    struct hsr_cp64 cp64 = hsr.cp64;
+    const struct hsr_cp64 cp64 = hsr.cp64;
 
     if ( !check_conditional_instr(regs, hsr) )
     {
@@ -1804,9 +1805,9 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, union hsr hsr)
     inject_undef_exception(regs, hsr.len);
 }
 
-static void do_cp(struct cpu_user_regs *regs, union hsr hsr)
+static void do_cp(struct cpu_user_regs *regs, const union hsr hsr)
 {
-    struct hsr_cp cp = hsr.cp;
+    const struct hsr_cp cp = hsr.cp;
 
     if ( !check_conditional_instr(regs, hsr) )
     {
@@ -1821,7 +1822,7 @@ static void do_cp(struct cpu_user_regs *regs, union hsr hsr)
 
 #ifdef CONFIG_ARM_64
 static void do_sysreg(struct cpu_user_regs *regs,
-                      union hsr hsr)
+                      const union hsr hsr)
 {
     register_t *x = select_user_reg(regs, hsr.sysreg.reg);
 
@@ -1918,7 +1919,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
         inject_undef64_exception(regs, hsr.len);
     default:
         {
-            struct hsr_sysreg sysreg = hsr.sysreg;
+            const struct hsr_sysreg sysreg = hsr.sysreg;
 
             gdprintk(XENLOG_ERR,
                      "%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
@@ -1997,16 +1998,16 @@ done:
 }
 
 static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
-                                      union hsr hsr)
+                                      const union hsr hsr)
 {
     register_t addr = READ_SYSREG(FAR_EL2);
     inject_iabt_exception(regs, addr, hsr.len);
 }
 
 static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
-                                     union hsr hsr)
+                                     const union hsr hsr)
 {
-    struct hsr_dabt dabt = hsr.dabt;
+    const struct hsr_dabt dabt = hsr.dabt;
     int rc;
     mmio_info_t info;
 
@@ -2066,7 +2067,7 @@ static void enter_hypervisor_head(struct cpu_user_regs *regs)
 
 asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
 {
-    union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };
+    const union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };
 
     enter_hypervisor_head(regs);
 
-- 
1.7.10.4

Re: [PATCH 01/19] xen: arm: constify union hsr and struct hsr_* where possible.

[Julien Grall]

Hi Ian,

On 31/03/2015 11:07, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

> ---
>   xen/arch/arm/traps.c |   41 +++++++++++++++++++++--------------------
>   1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index aaa9d93..69b9513 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -447,7 +447,7 @@ static vaddr_t exception_handler64(struct cpu_user_regs *regs, vaddr_t offset)
>   static void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len)
>   {
>       vaddr_t handler;
> -    union hsr esr = {
> +    const union hsr esr = {
>           .iss = 0,
>           .len = instr_len,
>           .ec = HSR_EC_UNKNOWN,
> @@ -1141,7 +1141,7 @@ int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc)
>   }
>
>   #ifdef CONFIG_ARM_64
> -static void do_trap_brk(struct cpu_user_regs *regs, union hsr hsr)
> +static void do_trap_brk(struct cpu_user_regs *regs, const union hsr hsr)
>   {
>       /* HCR_EL2.TGE and MDCR_EL2.TDE are not set so we never receive
>        * software breakpoint exception for EL1 and EL0 here.
> @@ -1488,7 +1488,8 @@ static const unsigned short cc_map[16] = {
>           0                       /* NV                     */
>   };
>
> -static int check_conditional_instr(struct cpu_user_regs *regs, union hsr hsr)
> +static int check_conditional_instr(struct cpu_user_regs *regs,
> +                                   const union hsr hsr)
>   {
>       unsigned long cpsr, cpsr_cond;
>       int cond;
> @@ -1533,7 +1534,7 @@ static int check_conditional_instr(struct cpu_user_regs *regs, union hsr hsr)
>       return 1;
>   }
>
> -static void advance_pc(struct cpu_user_regs *regs, union hsr hsr)
> +static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
>   {
>       unsigned long itbits, cond, cpsr = regs->cpsr;
>
> @@ -1574,9 +1575,9 @@ static void advance_pc(struct cpu_user_regs *regs, union hsr hsr)
>   }
>
>   static void do_cp15_32(struct cpu_user_regs *regs,
> -                       union hsr hsr)
> +                       const union hsr hsr)
>   {
> -    struct hsr_cp32 cp32 = hsr.cp32;
> +    const struct hsr_cp32 cp32 = hsr.cp32;
>       uint32_t *r = (uint32_t*)select_user_reg(regs, cp32.reg);
>       struct vcpu *v = current;
>
> @@ -1659,7 +1660,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
>   }
>
>   static void do_cp15_64(struct cpu_user_regs *regs,
> -                       union hsr hsr)
> +                       const union hsr hsr)
>   {
>       if ( !check_conditional_instr(regs, hsr) )
>       {
> @@ -1676,7 +1677,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,
>           break;
>       default:
>           {
> -            struct hsr_cp64 cp64 = hsr.cp64;
> +            const struct hsr_cp64 cp64 = hsr.cp64;
>
>               gdprintk(XENLOG_ERR,
>                        "%s p15, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
> @@ -1692,9 +1693,9 @@ static void do_cp15_64(struct cpu_user_regs *regs,
>       advance_pc(regs, hsr);
>   }
>
> -static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
> +static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
>   {
> -    struct hsr_cp32 cp32 = hsr.cp32;
> +    const struct hsr_cp32 cp32 = hsr.cp32;
>       uint32_t *r = (uint32_t *)select_user_reg(regs, cp32.reg);
>       struct domain *d = current->domain;
>
> @@ -1784,9 +1785,9 @@ static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
>       advance_pc(regs, hsr);
>   }
>
> -static void do_cp14_dbg(struct cpu_user_regs *regs, union hsr hsr)
> +static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
>   {
> -    struct hsr_cp64 cp64 = hsr.cp64;
> +    const struct hsr_cp64 cp64 = hsr.cp64;
>
>       if ( !check_conditional_instr(regs, hsr) )
>       {
> @@ -1804,9 +1805,9 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, union hsr hsr)
>       inject_undef_exception(regs, hsr.len);
>   }
>
> -static void do_cp(struct cpu_user_regs *regs, union hsr hsr)
> +static void do_cp(struct cpu_user_regs *regs, const union hsr hsr)
>   {
> -    struct hsr_cp cp = hsr.cp;
> +    const struct hsr_cp cp = hsr.cp;
>
>       if ( !check_conditional_instr(regs, hsr) )
>       {
> @@ -1821,7 +1822,7 @@ static void do_cp(struct cpu_user_regs *regs, union hsr hsr)
>
>   #ifdef CONFIG_ARM_64
>   static void do_sysreg(struct cpu_user_regs *regs,
> -                      union hsr hsr)
> +                      const union hsr hsr)
>   {
>       register_t *x = select_user_reg(regs, hsr.sysreg.reg);
>
> @@ -1918,7 +1919,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
>           inject_undef64_exception(regs, hsr.len);
>       default:
>           {
> -            struct hsr_sysreg sysreg = hsr.sysreg;
> +            const struct hsr_sysreg sysreg = hsr.sysreg;
>
>               gdprintk(XENLOG_ERR,
>                        "%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
> @@ -1997,16 +1998,16 @@ done:
>   }
>
>   static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
> -                                      union hsr hsr)
> +                                      const union hsr hsr)
>   {
>       register_t addr = READ_SYSREG(FAR_EL2);
>       inject_iabt_exception(regs, addr, hsr.len);
>   }
>
>   static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
> -                                     union hsr hsr)
> +                                     const union hsr hsr)
>   {
> -    struct hsr_dabt dabt = hsr.dabt;
> +    const struct hsr_dabt dabt = hsr.dabt;
>       int rc;
>       mmio_info_t info;
>
> @@ -2066,7 +2067,7 @@ static void enter_hypervisor_head(struct cpu_user_regs *regs)
>
>   asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>   {
> -    union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };
> +    const union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };
>
>       enter_hypervisor_head(regs);
>
>

-- 
Julien Grall

Re: [PATCH 01/19] xen: arm: constify union hsr and struct hsr_* where possible.

[Julien Grall]

On 02/04/2015 11:44, Julien Grall wrote:
> Hi Ian,
>
> On 31/03/2015 11:07, Ian Campbell wrote:
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Reviewed-by: Julien Grall <julien.grall@linaro.org>

Hmmm... Sorry I'm not use to use my citrix mail anymore. It should be

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

-- 
Julien Grall

[PATCH 02/19] xen: arm: add missing break

[Ian Campbell]

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

xen: arm: Fix handling of ICC_{SGI1R,SGI0R,ASGI1R}_EL1

Having injected an undefined instruction we don't want to also advance
pc. So return.

THe ICC_{SGI0R,ASGI1R}_EL1 case was previously missing a break, so
would have fallen through to the default case and injected a second
undef, corrupting SPSR_EL1 and ELR_EL1 for the guest.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 69b9513..99ceaea 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1908,7 +1908,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
         {
             dprintk(XENLOG_WARNING,
                     "failed emulation of sysreg ICC_SGI1R_EL1 access\n");
-            inject_undef64_exception(regs, hsr.len);
+            return inject_undef64_exception(regs, hsr.len);
         }
         break;
     case HSR_SYSREG_ICC_SGI0R_EL1:
@@ -1916,7 +1916,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
         /* TBD: Implement to support secure grp0/1 SGI forwarding */
         dprintk(XENLOG_WARNING,
                 "Emulation of sysreg ICC_SGI0R_EL1/ASGI1R_EL1 not supported\n");
-        inject_undef64_exception(regs, hsr.len);
+        return inject_undef64_exception(regs, hsr.len);
     default:
         {
             const struct hsr_sysreg sysreg = hsr.sysreg;
-- 
1.7.10.4

Re: [PATCH 02/19] xen: arm: add missing break

[Julien Grall]

Hi Ian,

On 31/03/2015 11:07, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>
> xen: arm: Fix handling of ICC_{SGI1R,SGI0R,ASGI1R}_EL1
>
> Having injected an undefined instruction we don't want to also advance
> pc. So return.
>
> THe ICC_{SGI0R,ASGI1R}_EL1 case was previously missing a break, so
> would have fallen through to the default case and injected a second
> undef, corrupting SPSR_EL1 and ELR_EL1 for the guest.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com>

Regards,

> ---
>   xen/arch/arm/traps.c |    4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 69b9513..99ceaea 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1908,7 +1908,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
>           {
>               dprintk(XENLOG_WARNING,
>                       "failed emulation of sysreg ICC_SGI1R_EL1 access\n");
> -            inject_undef64_exception(regs, hsr.len);
> +            return inject_undef64_exception(regs, hsr.len);
>           }
>           break;
>       case HSR_SYSREG_ICC_SGI0R_EL1:
> @@ -1916,7 +1916,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
>           /* TBD: Implement to support secure grp0/1 SGI forwarding */
>           dprintk(XENLOG_WARNING,
>                   "Emulation of sysreg ICC_SGI0R_EL1/ASGI1R_EL1 not supported\n");
> -        inject_undef64_exception(regs, hsr.len);
> +        return inject_undef64_exception(regs, hsr.len);
>       default:
>           {
>               const struct hsr_sysreg sysreg = hsr.sysreg;
>

-- 
Julien Grall

Re: [PATCH 02/19] xen: arm: add missing break

[Ian Campbell]

On Thu, 2015-04-02 at 16:09 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 31/03/2015 11:07, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >
> > xen: arm: Fix handling of ICC_{SGI1R,SGI0R,ASGI1R}_EL1
> >
> > Having injected an undefined instruction we don't want to also advance
> > pc. So return.
> >
> > THe ICC_{SGI0R,ASGI1R}_EL1 case was previously missing a break, so
> > would have fallen through to the default case and injected a second
> > undef, corrupting SPSR_EL1 and ELR_EL1 for the guest.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Reviewed-by: Julien Grall <julien.grall@citrix.com>

Thanks. I seem to have two commit messages for this one, I think from a
botched patch squash.

I've dropped the "add missing break" commit message, since I've actually
written about it in the final paragraph. I've kept your R-by regardless,
which I hope is ok.

Ian.

Re: [PATCH 02/19] xen: arm: add missing break

[Julien Grall]

Hi Ian,

On 16/04/2015 17:08, Ian Campbell wrote:
> On Thu, 2015-04-02 at 16:09 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 31/03/2015 11:07, Ian Campbell wrote:
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> xen: arm: Fix handling of ICC_{SGI1R,SGI0R,ASGI1R}_EL1
>>>
>>> Having injected an undefined instruction we don't want to also advance
>>> pc. So return.
>>>
>>> THe ICC_{SGI0R,ASGI1R}_EL1 case was previously missing a break, so
>>> would have fallen through to the default case and injected a second
>>> undef, corrupting SPSR_EL1 and ELR_EL1 for the guest.
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> Reviewed-by: Julien Grall <julien.grall@citrix.com>
>
> Thanks. I seem to have two commit messages for this one, I think from a
> botched patch squash.
>
> I've dropped the "add missing break" commit message, since I've actually
> written about it in the final paragraph. I've kept your R-by regardless,
> which I hope is ok.

I'm fine with the change.

Regards,

-- 
Julien Grall

[PATCH 03/19] xen: arm: call inject_undef_exception directly

[Ian Campbell]

Reducing the amount of goto maze considerably.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 99ceaea..7270116 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -518,13 +518,13 @@ static void inject_iabt64_exception(struct cpu_user_regs *regs,
 #endif
 
 static void inject_undef_exception(struct cpu_user_regs *regs,
-                                   int instr_len)
+                                   const union hsr hsr)
 {
         if ( is_32bit_domain(current->domain) )
             inject_undef32_exception(regs);
 #ifdef CONFIG_ARM_64
         else
-            inject_undef64_exception(regs, instr_len);
+            inject_undef64_exception(regs, hsr.len);
 #endif
 }
 
@@ -1592,11 +1592,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,
     case HSR_CPREG32(CNTP_CTL):
     case HSR_CPREG32(CNTP_TVAL):
         if ( !vtimer_emulate(regs, hsr) )
-            goto undef_cp15_32;
+            return inject_undef_exception(regs, hsr);
         break;
     case HSR_CPREG32(ACTLR):
         if ( psr_mode_is_user(regs) )
-            goto undef_cp15_32;
+            return inject_undef_exception(regs, hsr);
         if ( cp32.read )
            *r = v->arch.actlr;
         break;
@@ -1612,14 +1612,14 @@ 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) && !hsr.cp32.read )
-            goto undef_cp15_32;
+            return inject_undef_exception(regs, hsr);
         goto cp15_32_raz_wi;
 
     case HSR_CPREG32(PMINTENSET):
     case HSR_CPREG32(PMINTENCLR):
         /* EL1 only, however MDCR_EL2.TPM==1 means EL0 may trap here also. */
         if ( psr_mode_is_user(regs) )
-            goto undef_cp15_32;
+            return inject_undef_exception(regs, hsr);
         goto cp15_32_raz_wi;
     case HSR_CPREG32(PMCR):
     case HSR_CPREG32(PMCNTENSET):
@@ -1638,7 +1638,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
          * emulate that register as 0 above.
          */
         if ( psr_mode_is_user(regs) )
-            goto undef_cp15_32;
+            return inject_undef_exception(regs, hsr);
  cp15_32_raz_wi:
         if ( cp32.read )
             *r = 0;
@@ -1652,8 +1652,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
                  cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);
         gdprintk(XENLOG_ERR, "unhandled 32-bit CP15 access %#x\n",
                  hsr.bits & HSR_CP32_REGS_MASK);
- undef_cp15_32:
-        inject_undef_exception(regs, hsr.len);
+        inject_undef_exception(regs, hsr);
         return;
     }
     advance_pc(regs, hsr);
@@ -1673,7 +1672,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,
     case HSR_CPREG64(CNTPCT):
     case HSR_CPREG64(CNTP_CVAL):
         if ( !vtimer_emulate(regs, hsr) )
-            goto undef_cp15_64;
+            return inject_undef_exception(regs, hsr);
         break;
     default:
         {
@@ -1685,8 +1684,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,
                      cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);
             gdprintk(XENLOG_ERR, "unhandled 64-bit CP15 access %#x\n",
                      hsr.bits & HSR_CP64_REGS_MASK);
- undef_cp15_64:
-            inject_undef_exception(regs, hsr.len);
+            inject_undef_exception(regs, hsr);
             return;
         }
     }
@@ -1713,7 +1711,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
          * is set to 0, which we emulated below.
          */
         if ( !cp32.read )
-            goto undef_cp14_32;
+            return inject_undef_exception(regs, hsr);
 
         /* Implement the minimum requirements:
          *  - Number of watchpoints: 1
@@ -1731,14 +1729,14 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
          * is set to 0, which we emulated below.
          */
         if ( !cp32.read )
-            goto undef_cp14_32;
+            return inject_undef_exception(regs, hsr);
 
         *r = 0;
         break;
 
     case HSR_CPREG32(DBGDSCREXT):
         if ( usr_mode(regs) )
-            goto undef_cp14_32;
+            return inject_undef_exception(regs, hsr);
 
         /* Implement debug status and control register as RAZ/WI.
          * The OS won't use Hardware debug if MDBGen not set
@@ -1756,7 +1754,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
     case HSR_CPREG32(DBGBCR1):
     case HSR_CPREG32(DBGOSDLR):
         if ( usr_mode(regs) )
-            goto undef_cp14_32;
+            return inject_undef_exception(regs, hsr);
         /* RAZ/WI */
         if ( cp32.read )
             *r = 0;
@@ -1764,10 +1762,10 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
 
     case HSR_CPREG32(DBGOSLAR):
         if ( usr_mode(regs) )
-            goto undef_cp14_32;
+            return inject_undef_exception(regs, hsr);
         /* WO */
         if ( cp32.read )
-            goto undef_cp14_32;
+            return inject_undef_exception(regs, hsr);
         /* else: ignore */
         break;
     default:
@@ -1777,8 +1775,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
                   cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);
         gdprintk(XENLOG_ERR, "unhandled 32-bit cp14 access %#x\n",
                  hsr.bits & HSR_CP32_REGS_MASK);
- undef_cp14_32:
-        inject_undef_exception(regs, hsr.len);
+        inject_undef_exception(regs, hsr);
         return;
     }
 
@@ -1802,7 +1799,7 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
     gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 access %#x\n",
              hsr.bits & HSR_CP64_REGS_MASK);
 
-    inject_undef_exception(regs, hsr.len);
+    inject_undef_exception(regs, hsr);
 }
 
 static void do_cp(struct cpu_user_regs *regs, const union hsr hsr)
@@ -1817,7 +1814,7 @@ static void do_cp(struct cpu_user_regs *regs, const union hsr hsr)
 
     ASSERT(!cp.tas); /* We don't trap SIMD instruction */
     gdprintk(XENLOG_ERR, "unhandled CP%d access\n", cp.coproc);
-    inject_undef_exception(regs, hsr.len);
+    inject_undef_exception(regs, hsr);
 }
 
 #ifdef CONFIG_ARM_64
@@ -1847,7 +1844,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
          * undef.
          */
         if ( psr_mode_is_user(regs) )
-            goto undef_sysreg;
+            return inject_undef_exception(regs, hsr);
         goto sysreg_raz_wi;
 
     case HSR_SYSREG_MDCCSR_EL0:
@@ -1856,7 +1853,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
          * register as RAZ/WI above. So RO at both EL0 and EL1.
          */
         if ( !hsr.sysreg.read )
-            goto undef_sysreg;
+            return inject_undef_exception(regs, hsr);
 
         *x = 0;
         break;
@@ -1865,7 +1862,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
     case HSR_SYSREG_PMUSERENR_EL0:
         /* RO at EL0. RAZ/WI at EL1 */
         if ( psr_mode_is_user(regs) && !hsr.sysreg.read )
-            goto undef_sysreg;
+            return inject_undef_exception(regs, hsr);
         goto sysreg_raz_wi;
     case HSR_SYSREG_PMCR_EL0:
     case HSR_SYSREG_PMCNTENSET_EL0:
@@ -1884,7 +1881,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
          * emulate that register as 0 above.
          */
         if ( psr_mode_is_user(regs) )
-            goto undef_sysreg;
+            return inject_undef_exception(regs, hsr);
  sysreg_raz_wi:
         if ( hsr.sysreg.read )
             *x = 0;
@@ -1894,14 +1891,14 @@ static void do_sysreg(struct cpu_user_regs *regs,
     /* Write only, Write ignore registers: */
     case HSR_SYSREG_OSLAR_EL1:
         if ( hsr.sysreg.read )
-            goto undef_sysreg;
+            return inject_undef_exception(regs, hsr);
         /* else: write ignored */
         break;
     case HSR_SYSREG_CNTP_CTL_EL0:
     case HSR_SYSREG_CNTP_TVAL_EL0:
     case HSR_SYSREG_CNTP_CVAL_EL0:
         if ( !vtimer_emulate(regs, hsr) )
-            goto undef_sysreg;
+            return inject_undef_exception(regs, hsr);
         break;
     case HSR_SYSREG_ICC_SGI1R_EL1:
         if ( !vgic_emulate(regs, hsr) )
@@ -1931,8 +1928,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
                      sysreg.reg, regs->pc);
             gdprintk(XENLOG_ERR, "unhandled 64-bit sysreg access %#x\n",
                      hsr.bits & HSR_SYSREG_REGS_MASK);
- undef_sysreg:
-            inject_undef_exception(regs, hsr.sysreg.len);
+            inject_undef_exception(regs, hsr);
             return;
         }
     }
-- 
1.7.10.4

Re: [PATCH 03/19] xen: arm: call inject_undef_exception directly

[Julien Grall]

Hi Ian,

On 31/03/2015 11:07, Ian Campbell wrote:
> Reducing the amount of goto maze considerably.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com>

Regards,

-- 
Julien Grall

[PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper

[Ian Campbell]

Reduces the use of goto in the trap handlers to none.

Some explcitily 32-bit types become register_t here, but that's OK, on
32-bit they are 32-bit already and on 64-bit it is fine/harmless to
set the larger register, a 32-bit guest won't see the top half in any
case.

Unlike the previous code the advancing of PC is handled within the
helper, rather than after the end of the switch as before. So return
as the handler is called.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 7270116..8b1846a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1574,11 +1574,24 @@ static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
     regs->pc += hsr.len ? 4 : 2;
 }
 
+/* Read as zero + write ignore */
+static void handle_raz_wi(struct cpu_user_regs *regs,
+                          register_t *reg,
+                          bool_t read,
+                          const union hsr hsr)
+{
+    if ( read )
+        *reg = 0;
+    /* else: write ignored */
+
+    advance_pc(regs, hsr);
+}
+
 static void do_cp15_32(struct cpu_user_regs *regs,
                        const union hsr hsr)
 {
     const struct hsr_cp32 cp32 = hsr.cp32;
-    uint32_t *r = (uint32_t*)select_user_reg(regs, cp32.reg);
+    register_t *r = select_user_reg(regs, cp32.reg);
     struct vcpu *v = current;
 
     if ( !check_conditional_instr(regs, hsr) )
@@ -1613,14 +1626,14 @@ static void do_cp15_32(struct cpu_user_regs *regs,
         /* RO at EL0. RAZ/WI at EL1 */
         if ( psr_mode_is_user(regs) && !hsr.cp32.read )
             return inject_undef_exception(regs, hsr);
-        goto cp15_32_raz_wi;
+        return handle_raz_wi(regs, r, cp32.read, hsr);
 
     case HSR_CPREG32(PMINTENSET):
     case HSR_CPREG32(PMINTENCLR):
         /* EL1 only, however MDCR_EL2.TPM==1 means EL0 may trap here also. */
         if ( psr_mode_is_user(regs) )
             return inject_undef_exception(regs, hsr);
-        goto cp15_32_raz_wi;
+        return handle_raz_wi(regs, r, cp32.read, hsr);
     case HSR_CPREG32(PMCR):
     case HSR_CPREG32(PMCNTENSET):
     case HSR_CPREG32(PMCNTENCLR):
@@ -1639,11 +1652,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
          */
         if ( psr_mode_is_user(regs) )
             return inject_undef_exception(regs, hsr);
- cp15_32_raz_wi:
-        if ( cp32.read )
-            *r = 0;
-        /* else: write ignored */
-        break;
+        return handle_raz_wi(regs, r, cp32.read, hsr);
 
     default:
         gdprintk(XENLOG_ERR,
@@ -1694,7 +1703,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;
-    uint32_t *r = (uint32_t *)select_user_reg(regs, cp32.reg);
+    register_t *r = select_user_reg(regs, cp32.reg);
     struct domain *d = current->domain;
 
     if ( !check_conditional_instr(regs, hsr) )
@@ -1738,12 +1747,11 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
         if ( usr_mode(regs) )
             return inject_undef_exception(regs, hsr);
 
-        /* Implement debug status and control register as RAZ/WI.
-         * The OS won't use Hardware debug if MDBGen not set
+        /*
+         * Implement debug status and control register as RAZ/WI.
+         * The OS won't use Hardware debug if MDBGen not set.
          */
-        if ( cp32.read )
-           *r = 0;
-        break;
+        return handle_raz_wi(regs, r, cp32.read, hsr);
 
     case HSR_CPREG32(DBGVCR):
     case HSR_CPREG32(DBGBVR0):
@@ -1755,10 +1763,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
     case HSR_CPREG32(DBGOSDLR):
         if ( usr_mode(regs) )
             return inject_undef_exception(regs, hsr);
-        /* RAZ/WI */
-        if ( cp32.read )
-            *r = 0;
-        break;
+        return handle_raz_wi(regs, r, cp32.read, hsr);
 
     case HSR_CPREG32(DBGOSLAR):
         if ( usr_mode(regs) )
@@ -1845,7 +1850,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
          */
         if ( psr_mode_is_user(regs) )
             return inject_undef_exception(regs, hsr);
-        goto sysreg_raz_wi;
+        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr);
 
     case HSR_SYSREG_MDCCSR_EL0:
         /*
@@ -1863,7 +1868,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
         /* RO at EL0. RAZ/WI at EL1 */
         if ( psr_mode_is_user(regs) && !hsr.sysreg.read )
             return inject_undef_exception(regs, hsr);
-        goto sysreg_raz_wi;
+        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr);
     case HSR_SYSREG_PMCR_EL0:
     case HSR_SYSREG_PMCNTENSET_EL0:
     case HSR_SYSREG_PMCNTENCLR_EL0:
@@ -1882,11 +1887,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
          */
         if ( psr_mode_is_user(regs) )
             return inject_undef_exception(regs, hsr);
- sysreg_raz_wi:
-        if ( hsr.sysreg.read )
-            *x = 0;
-        /* else: write ignored */
-        break;
+        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr);
 
     /* Write only, Write ignore registers: */
     case HSR_SYSREG_OSLAR_EL1:
-- 
1.7.10.4

Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper

[Julien Grall]

Hi Ian,

On 31/03/2015 11:07, Ian Campbell wrote:
> Reduces the use of goto in the trap handlers to none.
>
> Some explcitily 32-bit types become register_t here, but that's OK, on

s/explcitily/explicitly/

> 32-bit they are 32-bit already and on 64-bit it is fine/harmless to
> set the larger register, a 32-bit guest won't see the top half in any
> case.

What about 32-bit userspace on 64-bit kernel? Are we sure that a guest 
kernel won't only save the bottom half of the register?

Regards,

-- 
Julien Grall

Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper

[Ian Campbell]

On Thu, 2015-04-02 at 16:14 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 31/03/2015 11:07, Ian Campbell wrote:
> > Reduces the use of goto in the trap handlers to none.
> >
> > Some explcitily 32-bit types become register_t here, but that's OK, on
> 
> s/explcitily/explicitly/
> 
> > 32-bit they are 32-bit already and on 64-bit it is fine/harmless to
> > set the larger register, a 32-bit guest won't see the top half in any
> > case.
> 
> What about 32-bit userspace on 64-bit kernel? Are we sure that a guest 
> kernel won't only save the bottom half of the register?

That would be fine, since the userspace couldn't see the top half anyway
so not saving it doesn't hurt.

In any case, the trap here has been talking from 32-bit mode and that is
where we will return, so I'm not sure the guest kernel enters the
picture, does it?

Ian.

Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper

[Julien Grall]

On 02/04/2015 16:31, Ian Campbell wrote:
> On Thu, 2015-04-02 at 16:14 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 31/03/2015 11:07, Ian Campbell wrote:
>>> Reduces the use of goto in the trap handlers to none.
>>>
>>> Some explcitily 32-bit types become register_t here, but that's OK, on
>>
>> s/explcitily/explicitly/
>>
>>> 32-bit they are 32-bit already and on 64-bit it is fine/harmless to
>>> set the larger register, a 32-bit guest won't see the top half in any
>>> case.
>>
>> What about 32-bit userspace on 64-bit kernel? Are we sure that a guest
>> kernel won't only save the bottom half of the register?
>
> That would be fine, since the userspace couldn't see the top half anyway
> so not saving it doesn't hurt.
>
> In any case, the trap here has been talking from 32-bit mode and that is
> where we will return, so I'm not sure the guest kernel enters the
> picture, does it?

It's possible for the kernel to access only a part of the 64 bit 
registers and preserve the other part with a valid data to use later.

AFAICT, nothing prevent a guest to use the top half of the registers for 
his own purpose. It would only need to save/restore the bottom half of a 
64 bit registers.

By resetting the 64-bit register, we will corrupt the top half of the 
registers and potentially (if the use case is valid) crash the kernel.

Although I didn't say that I would write a such guest ;)

Regards,

-- 
Julien Grall

Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper

[Ian Campbell]

On Thu, 2015-04-02 at 16:45 +0100, Julien Grall wrote:
> 
> On 02/04/2015 16:31, Ian Campbell wrote:
> > On Thu, 2015-04-02 at 16:14 +0100, Julien Grall wrote:
> >> Hi Ian,
> >>
> >> On 31/03/2015 11:07, Ian Campbell wrote:
> >>> Reduces the use of goto in the trap handlers to none.
> >>>
> >>> Some explcitily 32-bit types become register_t here, but that's OK, on
> >>
> >> s/explcitily/explicitly/
> >>
> >>> 32-bit they are 32-bit already and on 64-bit it is fine/harmless to
> >>> set the larger register, a 32-bit guest won't see the top half in any
> >>> case.
> >>
> >> What about 32-bit userspace on 64-bit kernel? Are we sure that a guest
> >> kernel won't only save the bottom half of the register?
> >
> > That would be fine, since the userspace couldn't see the top half anyway
> > so not saving it doesn't hurt.
> >
> > In any case, the trap here has been talking from 32-bit mode and that is
> > where we will return, so I'm not sure the guest kernel enters the
> > picture, does it?
> 
> It's possible for the kernel to access only a part of the 64 bit 
> registers and preserve the other part with a valid data to use later.
> 
> AFAICT, nothing prevent a guest to use the top half of the registers for 
> his own purpose. It would only need to save/restore the bottom half of a 
> 64 bit registers.

Writing to the bottom half (e.g. w0) of a register implicitly clears the
top half, IIRC, so I think a kernel is unlikely to want to do this, even
if it could (which I'm not quite convinced of).

> 
> By resetting the 64-bit register, we will corrupt the top half of the 
> registers and potentially (if the use case is valid) crash the kernel.
> 
> Although I didn't say that I would write a such guest ;)
> 
> Regards,
> 

Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper

[Ian Campbell]

On Thu, 2015-04-02 at 16:50 +0100, Ian Campbell wrote:

> Writing to the bottom half (e.g. w0) of a register implicitly clears the
> top half, IIRC, so I think a kernel is unlikely to want to do this, even
> if it could (which I'm not quite convinced of).

That said, I'll see if I can make something work with the handle_*
taking the reg number instead of a pointer and calling select_user_reg
in each.

Ian.

Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper

[Ian Campbell]

On Thu, 2015-04-02 at 17:01 +0100, Ian Campbell wrote:
> On Thu, 2015-04-02 at 16:50 +0100, Ian Campbell wrote:
> 
> > Writing to the bottom half (e.g. w0) of a register implicitly clears the
> > top half, IIRC, so I think a kernel is unlikely to want to do this, even
> > if it could (which I'm not quite convinced of).
> 
> That said, I'll see if I can make something work with the handle_*
> taking the reg number instead of a pointer and calling select_user_reg
> in each.

Actually don't even need that, I think the following does what is
needed. I'm not 100% convinced it is needed though, but it's simple
enough, and I can't find anything in the ARM ARM right now which rules
out what you are suggesting, even if it is unlikely.

Ian.

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 71e349a..61a2106 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1585,7 +1585,14 @@ static void handle_raz_wi(struct cpu_user_regs *regs,
         return inject_undef_exception(regs, hsr);
 
     if ( read )
-        *reg = 0;
+    {
+#ifdef CONFIG_ARM_64
+        if ( psr_mode_is_32bit(regs->cpsr) )
+            *reg &= ~0xffffffffUL;
+        else
+#endif
+            *reg = 0;
+    }
     /* else: write ignored */
 
     advance_pc(regs, hsr);
@@ -1622,7 +1629,12 @@ static void handle_ro_raz(struct cpu_user_regs *regs,
         return inject_undef_exception(regs, hsr);
     /* else: raz */
 
-    *reg = 0;
+#ifdef CONFIG_ARM_64
+    if ( psr_mode_is_32bit(regs->cpsr) )
+        *reg &= ~0xffffffffUL;
+    else
+#endif
+        *reg = 0;
 
     advance_pc(regs, hsr);
 }

Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper

[Julien Grall]

On 02/04/2015 18:19, Ian Campbell wrote:
> On Thu, 2015-04-02 at 17:01 +0100, Ian Campbell wrote:
>> On Thu, 2015-04-02 at 16:50 +0100, Ian Campbell wrote:
>>
>>> Writing to the bottom half (e.g. w0) of a register implicitly clears the
>>> top half, IIRC, so I think a kernel is unlikely to want to do this, even
>>> if it could (which I'm not quite convinced of).
>>
>> That said, I'll see if I can make something work with the handle_*
>> taking the reg number instead of a pointer and calling select_user_reg
>> in each.
>
> Actually don't even need that, I think the following does what is
> needed. I'm not 100% convinced it is needed though, but it's simple
> enough, and I can't find anything in the ARM ARM right now which rules
> out what you are suggesting, even if it is unlikely.

The paragraph "Pseudocode description of registers in AArch64 state" in 
section B1.2.1 (ARMv8 DDI0487 A.d) confirms your previous mail. I.e 
"writing to the bottom half (e.g. w0) of a register implicitly clears 
the top half".

I think it may be worth to mention the paragraph somewhere in the patch.

Regards,

-- 
Julien Grall

Re: [PATCH 04/19] xen: arm: provide and use a handle_raz_wi helper

[Ian Campbell]

On Fri, 2015-04-03 at 14:39 +0200, Julien Grall wrote:
> 
> On 02/04/2015 18:19, Ian Campbell wrote:
> > On Thu, 2015-04-02 at 17:01 +0100, Ian Campbell wrote:
> >> On Thu, 2015-04-02 at 16:50 +0100, Ian Campbell wrote:
> >>
> >>> Writing to the bottom half (e.g. w0) of a register implicitly clears the
> >>> top half, IIRC, so I think a kernel is unlikely to want to do this, even
> >>> if it could (which I'm not quite convinced of).
> >>
> >> That said, I'll see if I can make something work with the handle_*
> >> taking the reg number instead of a pointer and calling select_user_reg
> >> in each.
> >
> > Actually don't even need that, I think the following does what is
> > needed. I'm not 100% convinced it is needed though, but it's simple
> > enough, and I can't find anything in the ARM ARM right now which rules
> > out what you are suggesting, even if it is unlikely.
> 
> The paragraph "Pseudocode description of registers in AArch64 state" in 
> section B1.2.1 (ARMv8 DDI0487 A.d) confirms your previous mail. I.e 
> "writing to the bottom half (e.g. w0) of a register implicitly clears 
> the top half".
> 
> I think it may be worth to mention the paragraph somewhere in the patch.

Yes, I shall, in the commit log most likely.

Ian.

[PATCH 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers

[Ian Campbell]

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 8b1846a..ebc09f9 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs,
     advance_pc(regs, hsr);
 }
 
+/* Write only + write ignore */
+static void handle_wo_wi(struct cpu_user_regs *regs,
+                         register_t *reg,
+                         bool_t read,
+                         const union hsr hsr)
+{
+    if ( read )
+        return inject_undef_exception(regs, hsr);
+    /* else: ignore */
+
+    advance_pc(regs, hsr);
+}
+
+/* Read only + read as zero */
+static void handle_ro_raz(struct cpu_user_regs *regs,
+                          register_t *reg,
+                          bool_t read,
+                          const union hsr hsr)
+{
+    if ( !read )
+        return inject_undef_exception(regs, hsr);
+    /* else: raz */
+
+    *reg = 0;
+
+    advance_pc(regs, hsr);
+}
+
 static void do_cp15_32(struct cpu_user_regs *regs,
                        const union hsr hsr)
 {
@@ -1737,11 +1765,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
          * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
          * is set to 0, which we emulated below.
          */
-        if ( !cp32.read )
-            return inject_undef_exception(regs, hsr);
-
-        *r = 0;
-        break;
+        return handle_ro_raz(regs, r, cp32.read, hsr, 1);
 
     case HSR_CPREG32(DBGDSCREXT):
         if ( usr_mode(regs) )
@@ -1768,11 +1792,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
     case HSR_CPREG32(DBGOSLAR):
         if ( usr_mode(regs) )
             return inject_undef_exception(regs, hsr);
-        /* WO */
-        if ( cp32.read )
-            return inject_undef_exception(regs, hsr);
-        /* else: ignore */
-        break;
+        return handle_wo_wi(regs, r, cp32.read, hsr);
     default:
         gdprintk(XENLOG_ERR,
                  "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
@@ -1857,11 +1877,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
          * 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.
          */
-        if ( !hsr.sysreg.read )
-            return inject_undef_exception(regs, hsr);
-
-        *x = 0;
-        break;
+        return handle_ro_raz(regs, x, hsr.sysreg.read, hsr);
 
     /* - Perf monitors */
     case HSR_SYSREG_PMUSERENR_EL0:
@@ -1891,10 +1907,8 @@ static void do_sysreg(struct cpu_user_regs *regs,
 
     /* Write only, Write ignore registers: */
     case HSR_SYSREG_OSLAR_EL1:
-        if ( hsr.sysreg.read )
-            return inject_undef_exception(regs, hsr);
-        /* else: write ignored */
-        break;
+        return handle_wo_wi(regs, x, hsr.sysreg.read, hsr);
+
     case HSR_SYSREG_CNTP_CTL_EL0:
     case HSR_SYSREG_CNTP_TVAL_EL0:
     case HSR_SYSREG_CNTP_CVAL_EL0:
-- 
1.7.10.4

Re: [PATCH 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers

[Julien Grall]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   xen/arch/arm/traps.c |   52 ++++++++++++++++++++++++++++++++------------------
>   1 file changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 8b1846a..ebc09f9 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs,
>       advance_pc(regs, hsr);
>   }
>
> +/* Write only + write ignore */
> +static void handle_wo_wi(struct cpu_user_regs *regs,
> +                         register_t *reg,

This helper handles WO and WI which doesn't require to modify the register.

I would pass a register_t rather than register_t* in order to make clear 
that the register of the guest won't change.

> +                         bool_t read,
> +                         const union hsr hsr)
> +{
> +    if ( read )
> +        return inject_undef_exception(regs, hsr);
> +    /* else: ignore */
> +
> +    advance_pc(regs, hsr);
> +}
> +
> +/* Read only + read as zero */

This comment may confuse developer who wants to implement RO register 
which another value than 0.

I got confuse too. It would be nice to expand the comment for the RO case.

> +static void handle_ro_raz(struct cpu_user_regs *regs,
> +                          register_t *reg,
> +                          bool_t read,
> +                          const union hsr hsr)
> +{
> +    if ( !read )
> +        return inject_undef_exception(regs, hsr);
> +    /* else: raz */
> +
> +    *reg = 0;
> +
> +    advance_pc(regs, hsr);
> +}
> +
>   static void do_cp15_32(struct cpu_user_regs *regs,
>                          const union hsr hsr)
>   {
> @@ -1737,11 +1765,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
>            * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
>            * is set to 0, which we emulated below.
>            */
> -        if ( !cp32.read )
> -            return inject_undef_exception(regs, hsr);
> -
> -        *r = 0;
> -        break;
> +        return handle_ro_raz(regs, r, cp32.read, hsr, 1);

The function call has too many argument.

I guess the last argument is the minimum level exception, we should be 
part of the next patch (#6).

Regards,


-- 
Julien Grall

Re: [PATCH 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers

[Ian Campbell]

On Fri, 2015-04-03 at 14:51 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 31/03/2015 12:07, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >   xen/arch/arm/traps.c |   52 ++++++++++++++++++++++++++++++++------------------
> >   1 file changed, 33 insertions(+), 19 deletions(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 8b1846a..ebc09f9 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs,
> >       advance_pc(regs, hsr);
> >   }
> >
> > +/* Write only + write ignore */
> > +static void handle_wo_wi(struct cpu_user_regs *regs,
> > +                         register_t *reg,
> 
> This helper handles WO and WI which doesn't require to modify the register.
> 
> I would pass a register_t rather than register_t* in order to make clear 
> that the register of the guest won't change.

I deliberately made handle_* all have the same prototype for
consistency.

> > +/* Read only + read as zero */
> 
> This comment may confuse developer who wants to implement RO register 
> which another value than 0.
> 
> I got confuse too. It would be nice to expand the comment for the RO case.

I'm afraid I don't understand the confusion so I'm not sure how to
clarify. What did you think this comment was saying?

> > +        return handle_ro_raz(regs, r, cp32.read, hsr, 1);
> 
> The function call has too many argument.
> 
> I guess the last argument is the minimum level exception, we should be 
> part of the next patch (#6).

Fixed, thanks.

Ian.

Re: [PATCH 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers

[Julien Grall]

Hi Ian,

On 16/04/2015 17:22, Ian Campbell wrote:
> On Fri, 2015-04-03 at 14:51 +0200, Julien Grall wrote:
>> On 31/03/2015 12:07, Ian Campbell wrote:
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>    xen/arch/arm/traps.c |   52 ++++++++++++++++++++++++++++++++------------------
>>>    1 file changed, 33 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index 8b1846a..ebc09f9 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -1587,6 +1587,34 @@ static void handle_raz_wi(struct cpu_user_regs *regs,
>>>        advance_pc(regs, hsr);
>>>    }
>>>
>>> +/* Write only + write ignore */
>>> +static void handle_wo_wi(struct cpu_user_regs *regs,
>>> +                         register_t *reg,
>>
>> This helper handles WO and WI which doesn't require to modify the register.
>>
>> I would pass a register_t rather than register_t* in order to make clear
>> that the register of the guest won't change.
>
> I deliberately made handle_* all have the same prototype for
> consistency.

Ok.


>>> +/* Read only + read as zero */
>>
>> This comment may confuse developer who wants to implement RO register
>> which another value than 0.
>>
>> I got confuse too. It would be nice to expand the comment for the RO case.
>
> I'm afraid I don't understand the confusion so I'm not sure how to
> clarify. What did you think this comment was saying?

When I read the comment I though you were implemented two distinct part:
RO and RAZ.

As the value return by RO may not always be 0 (we have a handful of 
cases in traps.c), I didn't understand why you were implementing both 
within the same helper.

Although this helper choose to implement RO as RAZ. So I think it would 
be good to mention it in order to avoid confusion later.

Regards,

-- 
Julien Grall

Re: [PATCH 05/19] xen: arm: Add and use r/o+raz and w/o+wi helpers

[Ian Campbell]

On Fri, 2015-04-17 at 07:18 +0100, Julien Grall wrote:
> >>> +/* Read only + read as zero */
> >>
> >> This comment may confuse developer who wants to implement RO register
> >> which another value than 0.
> >>
> >> I got confuse too. It would be nice to expand the comment for the RO case.
> >
> > I'm afraid I don't understand the confusion so I'm not sure how to
> > clarify. What did you think this comment was saying?
> 
> When I read the comment I though you were implemented two distinct part:
> RO and RAZ.

Well, in a sense I am ;-)

> As the value return by RO may not always be 0 (we have a handful of 
> cases in traps.c), I didn't understand why you were implementing both 
> within the same helper.
> 
> Although this helper choose to implement RO as RAZ. So I think it would 
> be good to mention it in order to avoid confusion later.

Perhaps rather than:
/* Read as zero + write ignore */
/* Write only + write ignore */
/* Read only + read as zero */

this would be clearer:
/* Read as zero and write ignore */
/* Write only as write ignore */
/* Read only as read as zero */
?

IOW I suspect it is the + which has confused you, especially since it is
used differently in the first vs the second two.

Ian.

[PATCH 06/19] xen: arm: add minimum exception level argument to trap handler helpers

[Ian Campbell]

Removes a load of boiler plate.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ebc09f9..c9c98d3 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1578,8 +1578,12 @@ static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
 static void handle_raz_wi(struct cpu_user_regs *regs,
                           register_t *reg,
                           bool_t read,
-                          const union hsr hsr)
+                          const union hsr hsr,
+                          int min_el)
 {
+    if ( min_el > 0 && psr_mode_is_user(regs) )
+        return inject_undef_exception(regs, hsr);
+
     if ( read )
         *reg = 0;
     /* else: write ignored */
@@ -1591,8 +1595,12 @@ static void handle_raz_wi(struct cpu_user_regs *regs,
 static void handle_wo_wi(struct cpu_user_regs *regs,
                          register_t *reg,
                          bool_t read,
-                         const union hsr hsr)
+                         const union hsr hsr,
+                         int min_el)
 {
+    if ( min_el > 0 && psr_mode_is_user(regs) )
+        return inject_undef_exception(regs, hsr);
+
     if ( read )
         return inject_undef_exception(regs, hsr);
     /* else: ignore */
@@ -1604,8 +1612,12 @@ static void handle_wo_wi(struct cpu_user_regs *regs,
 static void handle_ro_raz(struct cpu_user_regs *regs,
                           register_t *reg,
                           bool_t read,
-                          const union hsr hsr)
+                          const union hsr hsr,
+                          int min_el)
 {
+    if ( min_el > 0 && psr_mode_is_user(regs) )
+        return inject_undef_exception(regs, hsr);
+
     if ( !read )
         return inject_undef_exception(regs, hsr);
     /* else: raz */
@@ -1652,16 +1664,15 @@ 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) && !hsr.cp32.read )
-            return inject_undef_exception(regs, hsr);
-        return handle_raz_wi(regs, r, cp32.read, hsr);
+        if ( psr_mode_is_user(regs) )
+            return handle_ro_raz(regs, r, cp32.read, hsr, 0);
+        else
+            return handle_raz_wi(regs, r, 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. */
-        if ( psr_mode_is_user(regs) )
-            return inject_undef_exception(regs, hsr);
-        return handle_raz_wi(regs, r, cp32.read, hsr);
+        return handle_raz_wi(regs, r, cp32.read, hsr, 1);
     case HSR_CPREG32(PMCR):
     case HSR_CPREG32(PMCNTENSET):
     case HSR_CPREG32(PMCNTENCLR):
@@ -1678,9 +1689,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.
          */
-        if ( psr_mode_is_user(regs) )
-            return inject_undef_exception(regs, hsr);
-        return handle_raz_wi(regs, r, cp32.read, hsr);
+        return handle_raz_wi(regs, r, cp32.read, hsr, 1);
 
     default:
         gdprintk(XENLOG_ERR,
@@ -1768,14 +1777,11 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
         return handle_ro_raz(regs, r, cp32.read, hsr, 1);
 
     case HSR_CPREG32(DBGDSCREXT):
-        if ( usr_mode(regs) )
-            return inject_undef_exception(regs, hsr);
-
         /*
          * 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);
+        return handle_raz_wi(regs, r, cp32.read, hsr, 1);
 
     case HSR_CPREG32(DBGVCR):
     case HSR_CPREG32(DBGBVR0):
@@ -1785,14 +1791,10 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
     case HSR_CPREG32(DBGBVR1):
     case HSR_CPREG32(DBGBCR1):
     case HSR_CPREG32(DBGOSDLR):
-        if ( usr_mode(regs) )
-            return inject_undef_exception(regs, hsr);
-        return handle_raz_wi(regs, r, cp32.read, hsr);
+        return handle_raz_wi(regs, r, cp32.read, hsr, 1);
 
     case HSR_CPREG32(DBGOSLAR):
-        if ( usr_mode(regs) )
-            return inject_undef_exception(regs, hsr);
-        return handle_wo_wi(regs, r, cp32.read, hsr);
+        return handle_wo_wi(regs, r, cp32.read, hsr, 1);
     default:
         gdprintk(XENLOG_ERR,
                  "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
@@ -1868,23 +1870,22 @@ static void do_sysreg(struct cpu_user_regs *regs,
          * Accessible from EL1 only, but if EL0 trap happens handle as
          * undef.
          */
-        if ( psr_mode_is_user(regs) )
-            return inject_undef_exception(regs, hsr);
-        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr);
+        return handle_raz_wi(regs, x, 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);
+        return handle_ro_raz(regs, x, hsr.sysreg.read, hsr, 0);
 
     /* - Perf monitors */
     case HSR_SYSREG_PMUSERENR_EL0:
         /* RO at EL0. RAZ/WI at EL1 */
-        if ( psr_mode_is_user(regs) && !hsr.sysreg.read )
-            return inject_undef_exception(regs, hsr);
-        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr);
+        if ( psr_mode_is_user(regs) )
+            return handle_ro_raz(regs, x, hsr.sysreg.read, hsr, 0);
+        else
+            return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
     case HSR_SYSREG_PMCR_EL0:
     case HSR_SYSREG_PMCNTENSET_EL0:
     case HSR_SYSREG_PMCNTENCLR_EL0:
@@ -1901,13 +1902,11 @@ 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.
          */
-        if ( psr_mode_is_user(regs) )
-            return inject_undef_exception(regs, hsr);
-        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr);
+        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
 
     /* Write only, Write ignore registers: */
     case HSR_SYSREG_OSLAR_EL1:
-        return handle_wo_wi(regs, x, hsr.sysreg.read, hsr);
+        return handle_wo_wi(regs, x, hsr.sysreg.read, hsr, 1);
 
     case HSR_SYSREG_CNTP_CTL_EL0:
     case HSR_SYSREG_CNTP_TVAL_EL0:
-- 
1.7.10.4

Re: [PATCH 06/19] xen: arm: add minimum exception level argument to trap handler helpers

[Julien Grall]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> Removes a load of boiler plate.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   xen/arch/arm/traps.c |   65 +++++++++++++++++++++++++-------------------------
>   1 file changed, 32 insertions(+), 33 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ebc09f9..c9c98d3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1578,8 +1578,12 @@ static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
>   static void handle_raz_wi(struct cpu_user_regs *regs,
>                             register_t *reg,
>                             bool_t read,
> -                          const union hsr hsr)
> +                          const union hsr hsr,
> +                          int min_el)
>   {

I would add an ASSERT((min_el == 0) || (min_el == 1)) in order to make 
clear that min_el should be either EL0 or EL1.

> +    if ( min_el > 0 && psr_mode_is_user(regs) )
> +        return inject_undef_exception(regs, hsr);
> +
>       if ( read )
>           *reg = 0;
>       /* else: write ignored */
> @@ -1591,8 +1595,12 @@ static void handle_raz_wi(struct cpu_user_regs *regs,
>   static void handle_wo_wi(struct cpu_user_regs *regs,
>                            register_t *reg,
>                            bool_t read,
> -                         const union hsr hsr)
> +                         const union hsr hsr,
> +                         int min_el)
>   {

Ditto

> +    if ( min_el > 0 && psr_mode_is_user(regs) )
> +        return inject_undef_exception(regs, hsr);
> +
>       if ( read )
>           return inject_undef_exception(regs, hsr);
>       /* else: ignore */
> @@ -1604,8 +1612,12 @@ static void handle_wo_wi(struct cpu_user_regs *regs,
>   static void handle_ro_raz(struct cpu_user_regs *regs,
>                             register_t *reg,
>                             bool_t read,
> -                          const union hsr hsr)
> +                          const union hsr hsr,
> +                          int min_el)
>   {

Ditto

> +    if ( min_el > 0 && psr_mode_is_user(regs) )
> +        return inject_undef_exception(regs, hsr);
> +
>       if ( !read )
>           return inject_undef_exception(regs, hsr);
>       /* else: raz */

Regards,

-- 
Julien Grall

Re: [PATCH 06/19] xen: arm: add minimum exception level argument to trap handler helpers

[Ian Campbell]

On Fri, 2015-04-03 at 14:58 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 31/03/2015 12:07, Ian Campbell wrote:
> > Removes a load of boiler plate.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >   xen/arch/arm/traps.c |   65 +++++++++++++++++++++++++-------------------------
> >   1 file changed, 32 insertions(+), 33 deletions(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index ebc09f9..c9c98d3 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1578,8 +1578,12 @@ static void advance_pc(struct cpu_user_regs *regs, const union hsr hsr)
> >   static void handle_raz_wi(struct cpu_user_regs *regs,
> >                             register_t *reg,
> >                             bool_t read,
> > -                          const union hsr hsr)
> > +                          const union hsr hsr,
> > +                          int min_el)
> >   {
> 
> I would add an ASSERT((min_el == 0) || (min_el == 1)) in order to make 
> clear that min_el should be either EL0 or EL1.

Done for all cases, thanks.

[PATCH 07/19] xen: arm: Annotate trap handler for HCR_EL2.{TWI, TWE, TSC}

[Ian Campbell]

Reference the bit which enables the trap and the section/page which
describes what that bit enables.

These ones are pretty trivial, included for completeness.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index c9c98d3..70e1b4d 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2083,6 +2083,12 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
 
     switch (hsr.ec) {
     case HSR_EC_WFI_WFE:
+        /*
+         * HSR_EL2.TWI, HSR_EL2.TWE
+         *
+         * ARMv7 (DDI 0406C.b): B1.14.9
+         * ARMv8 (DDI 0487A.d): D1-1505 Table D1-51
+         */
         if ( !check_conditional_instr(regs, hsr) )
         {
             advance_pc(regs, hsr);
@@ -2125,6 +2131,12 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         do_cp(regs, hsr);
         break;
     case HSR_EC_SMC32:
+        /*
+         * HSR_EL2.TSC
+         *
+         * ARMv7 (DDI 0406C.b): B1.14.8
+         * ARMv8 (DDI 0487A.d): D1-1501 Table D1-44
+         */
         GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_smc32);
         inject_undef32_exception(regs);
@@ -2153,6 +2165,11 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         do_trap_hypercall(regs, &regs->x16, hsr.iss);
         break;
     case HSR_EC_SMC64:
+        /*
+         * HSR_EL2.TSC
+         *
+         * ARMv8 (DDI 0487A.d): D1-1501 Table D1-44
+         */
         GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_smc64);
         inject_undef64_exception(regs, hsr.len);
-- 
1.7.10.4

Re: [PATCH 07/19] xen: arm: Annotate trap handler for HCR_EL2.{TWI, TWE, TSC}

[Julien Grall]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> Reference the bit which enables the trap and the section/page which
> describes what that bit enables.
>
> These ones are pretty trivial, included for completeness.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   xen/arch/arm/traps.c |   17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index c9c98d3..70e1b4d 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2083,6 +2083,12 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>
>       switch (hsr.ec) {
>       case HSR_EC_WFI_WFE:
> +        /*
> +         * HSR_EL2.TWI, HSR_EL2.TWE

Typo: I should be HCR not HSR.

> +         *
> +         * ARMv7 (DDI 0406C.b): B1.14.9
> +         * ARMv8 (DDI 0487A.d): D1-1505 Table D1-51

It's a bit confusing that you are using section for ARMv7 and page for 
ARMv8.

> +         */
>           if ( !check_conditional_instr(regs, hsr) )
>           {
>               advance_pc(regs, hsr);
> @@ -2125,6 +2131,12 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>           do_cp(regs, hsr);
>           break;
>       case HSR_EC_SMC32:
> +        /*
> +         * HSR_EL2.TSC

HCR

> +         *
> +         * ARMv7 (DDI 0406C.b): B1.14.8
> +         * ARMv8 (DDI 0487A.d): D1-1501 Table D1-44
> +         */
>           GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
>           perfc_incr(trap_smc32);
>           inject_undef32_exception(regs);
> @@ -2153,6 +2165,11 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>           do_trap_hypercall(regs, &regs->x16, hsr.iss);
>           break;
>       case HSR_EC_SMC64:
> +        /*
> +         * HSR_EL2.TSC

HCR

> +         *
> +         * ARMv8 (DDI 0487A.d): D1-1501 Table D1-44
> +         */
>           GUEST_BUG_ON(psr_mode_is_32bit(regs->cpsr));
>           perfc_incr(trap_smc64);
>           inject_undef64_exception(regs, hsr.len);
>

Regards,

-- 
Julien Grall

Re: [PATCH 07/19] xen: arm: Annotate trap handler for HCR_EL2.{TWI, TWE, TSC}

[Ian Campbell]

On Fri, 2015-04-03 at 15:05 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 31/03/2015 12:07, Ian Campbell wrote:
> > Reference the bit which enables the trap and the section/page which
> > describes what that bit enables.
> >
> > These ones are pretty trivial, included for completeness.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >   xen/arch/arm/traps.c |   17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index c9c98d3..70e1b4d 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -2083,6 +2083,12 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
> >
> >       switch (hsr.ec) {
> >       case HSR_EC_WFI_WFE:
> > +        /*
> > +         * HSR_EL2.TWI, HSR_EL2.TWE
> 
> Typo: I should be HCR not HSR.

Gah, at least I was consistent pretty much throughout!

> 
> > +         *
> > +         * ARMv7 (DDI 0406C.b): B1.14.9
> > +         * ARMv8 (DDI 0487A.d): D1-1505 Table D1-51
> 
> It's a bit confusing that you are using section for ARMv7 and page for 
> ARMv8.

I wanted to use section numbers everywhere, since in the ARMv7 ARM they
are nice and precise. But the ARMv8 ARM doesn't really use them in the
same way.

In this case D1-1505 is in section D1.15.3 which starts on page D1-1495
and covers the whole of HCR_EL2, which isn't a useful reference since it
would be the same for 2/3 of the patches (AKA bits in HCR_EL2) in this
series. D1-1505 isn't perfect since there is other stuff on that page,
but it's as close I can get.

Bit in the ARMv7 ARM B1.14.9 is exactly about these two traps.

Part of my goal here is to make it so that people (especially me) don't
spend half their life searching for the doc associated with a given
HCR_EL2 trap bit.

Ian.

Re: [PATCH 07/19] xen: arm: Annotate trap handler for HCR_EL2.{TWI, TWE, TSC}

[Julien Grall]

Hi Ian,

On 16/04/2015 17:34, Ian Campbell wrote:
> On Fri, 2015-04-03 at 15:05 +0200, Julien Grall wrote:
>> Hi Ian,
>>
>> On 31/03/2015 12:07, Ian Campbell wrote:
>>> Reference the bit which enables the trap and the section/page which
>>> describes what that bit enables.
>>>
>>> These ones are pretty trivial, included for completeness.
>>>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>    xen/arch/arm/traps.c |   17 +++++++++++++++++
>>>    1 file changed, 17 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index c9c98d3..70e1b4d 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -2083,6 +2083,12 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>>>
>>>        switch (hsr.ec) {
>>>        case HSR_EC_WFI_WFE:
>>> +        /*
>>> +         * HSR_EL2.TWI, HSR_EL2.TWE
>>
>> Typo: I should be HCR not HSR.
>
> Gah, at least I was consistent pretty much throughout!
>
>>
>>> +         *
>>> +         * ARMv7 (DDI 0406C.b): B1.14.9
>>> +         * ARMv8 (DDI 0487A.d): D1-1505 Table D1-51
>>
>> It's a bit confusing that you are using section for ARMv7 and page for
>> ARMv8.
>
> I wanted to use section numbers everywhere, since in the ARMv7 ARM they
> are nice and precise. But the ARMv8 ARM doesn't really use them in the
> same way.
>
> In this case D1-1505 is in section D1.15.3 which starts on page D1-1495
> and covers the whole of HCR_EL2, which isn't a useful reference since it
> would be the same for 2/3 of the patches (AKA bits in HCR_EL2) in this
> series. D1-1505 isn't perfect since there is other stuff on that page,
> but it's as close I can get.

Right, it's better to have a specific reference. I was surprised to see 
that ARMv8 didn't have number for small section.

> Bit in the ARMv7 ARM B1.14.9 is exactly about these two traps.
>
> Part of my goal here is to make it so that people (especially me) don't
> spend half their life searching for the doc associated with a given
> HCR_EL2 trap bit.

Thank you for the explanation. When I reviewed the series, it was very 
handy to have a reference to the docs.

Regards,

-- 
Julien Grall

[PATCH 08/19] xen: arm: implement handling of ACTLR_EL1 trap

[Ian Campbell]

While annotating ACTLR I noticed that we don't appear to handle the
64-bit version of this trap. Do so and annotate everything.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 70e1b4d..ca43f79 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1647,6 +1647,13 @@ static void do_cp15_32(struct cpu_user_regs *regs,
         if ( !vtimer_emulate(regs, hsr) )
             return inject_undef_exception(regs, hsr);
         break;
+
+    /*
+     * HSR_EL2.TASC / HSR.TAC
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.6
+     * ARMv8 (DDI 0487A.d): G6.2.1
+     */
     case HSR_CPREG32(ACTLR):
         if ( psr_mode_is_user(regs) )
             return inject_undef_exception(regs, hsr);
@@ -1849,9 +1856,22 @@ static void do_sysreg(struct cpu_user_regs *regs,
                       const union hsr hsr)
 {
     register_t *x = select_user_reg(regs, hsr.sysreg.reg);
+    struct vcpu *v = current;
 
     switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
     {
+    /*
+     * HSR_EL2.TASC
+     *
+     * ARMv8 (DDI 0487A.d): D7.2.1
+     */
+    case HSR_SYSREG_ACTLR_EL1:
+        if ( psr_mode_is_user(regs) )
+            return inject_undef_exception(regs, hsr);
+        if ( hsr.sysreg.read )
+           *x = v->arch.actlr;
+        break;
+
     /* RAZ/WI registers: */
     /*  - Debug */
     case HSR_SYSREG_MDSCR_EL1:
diff --git a/xen/include/asm-arm/sysregs.h b/xen/include/asm-arm/sysregs.h
index 2284fd7..d75e154 100644
--- a/xen/include/asm-arm/sysregs.h
+++ b/xen/include/asm-arm/sysregs.h
@@ -72,6 +72,7 @@
                                   case HSR_SYSREG_##REG##n_EL1(15)
 
 #define HSR_SYSREG_SCTLR_EL1      HSR_SYSREG(3,0,c1, c0,0)
+#define HSR_SYSREG_ACTLR_EL1      HSR_SYSREG(3,0,c1, c0,1)
 #define HSR_SYSREG_TTBR0_EL1      HSR_SYSREG(3,0,c2, c0,0)
 #define HSR_SYSREG_TTBR1_EL1      HSR_SYSREG(3,0,c2, c0,1)
 #define HSR_SYSREG_TCR_EL1        HSR_SYSREG(3,0,c2, c0,2)
-- 
1.7.10.4

Re: [PATCH 08/19] xen: arm: implement handling of ACTLR_EL1 trap

[Julien Grall]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> While annotating ACTLR I noticed that we don't appear to handle the
> 64-bit version of this trap. Do so and annotate everything.

While Linux doesn't use ACTLR_EL1 on aarch64, another OS may use it.

I'm not sure if we should consider it as a possible security issue as at 
least the Cortex A53 implements the register RES0.

> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   xen/arch/arm/traps.c          |   20 ++++++++++++++++++++
>   xen/include/asm-arm/sysregs.h |    1 +
>   2 files changed, 21 insertions(+)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 70e1b4d..ca43f79 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1647,6 +1647,13 @@ static void do_cp15_32(struct cpu_user_regs *regs,
>           if ( !vtimer_emulate(regs, hsr) )
>               return inject_undef_exception(regs, hsr);
>           break;
> +
> +    /*
> +     * HSR_EL2.TASC / HSR.TAC

I don't find any TASC in the ARMv8 doc. Did you intend to say TACR?

Also it's not HSR but HCR.

> +     *
> +     * ARMv7 (DDI 0406C.b): B1.14.6
> +     * ARMv8 (DDI 0487A.d): G6.2.1
> +     */
>       case HSR_CPREG32(ACTLR):
>           if ( psr_mode_is_user(regs) )
>               return inject_undef_exception(regs, hsr);
> @@ -1849,9 +1856,22 @@ static void do_sysreg(struct cpu_user_regs *regs,
>                         const union hsr hsr)
>   {
>       register_t *x = select_user_reg(regs, hsr.sysreg.reg);
> +    struct vcpu *v = current;
>
>       switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
>       {
> +    /*
> +     * HSR_EL2.TASC

Same question here for TASC.

Regards,

-- 
Julien Grall

Re: [PATCH 08/19] xen: arm: implement handling of ACTLR_EL1 trap

[Ian Campbell]

On Fri, 2015-04-03 at 15:42 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 31/03/2015 12:07, Ian Campbell wrote:
> > While annotating ACTLR I noticed that we don't appear to handle the
> > 64-bit version of this trap. Do so and annotate everything.
> 
> While Linux doesn't use ACTLR_EL1 on aarch64, another OS may use it.
> 
> I'm not sure if we should consider it as a possible security issue as at 
> least the Cortex A53 implements the register RES0.

Without this patch we would end up logging a debug message and injecting
undef into the guest. Since this is an EL1 register all a malicious
guest can do is send itself exceptions.

> 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >   xen/arch/arm/traps.c          |   20 ++++++++++++++++++++
> >   xen/include/asm-arm/sysregs.h |    1 +
> >   2 files changed, 21 insertions(+)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 70e1b4d..ca43f79 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1647,6 +1647,13 @@ static void do_cp15_32(struct cpu_user_regs *regs,
> >           if ( !vtimer_emulate(regs, hsr) )
> >               return inject_undef_exception(regs, hsr);
> >           break;
> > +
> > +    /*
> > +     * HSR_EL2.TASC / HSR.TAC
> 
> I don't find any TASC in the ARMv8 doc. Did you intend to say TACR?

Indeed, I did.

> Also it's not HSR but HCR.

Yes, sigh :-(

Ian.

[PATCH 09/19] xen: arm: Annotate registers trapped by HSR_EL1.TIDCP

[Ian Campbell]

This traps variety of implementation defined registers, so add a note
to the default case of the respective handler.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ca43f79..e26e673 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1698,6 +1698,14 @@ static void do_cp15_32(struct cpu_user_regs *regs,
          */
         return handle_raz_wi(regs, r, cp32.read, hsr, 1);
 
+    /*
+     * HCR_EL2.TIDCP
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.3
+     * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43
+     *
+     * And all other unknown registers.
+     */
     default:
         gdprintk(XENLOG_ERR,
                  "%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
@@ -1948,6 +1956,14 @@ static void do_sysreg(struct cpu_user_regs *regs,
         dprintk(XENLOG_WARNING,
                 "Emulation of sysreg ICC_SGI0R_EL1/ASGI1R_EL1 not supported\n");
         return inject_undef64_exception(regs, hsr.len);
+
+    /*
+     * HCR_EL2.TIDCP
+     *
+     * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43
+     *
+     * And all other unknown registers.
+     */
     default:
         {
             const struct hsr_sysreg sysreg = hsr.sysreg;
-- 
1.7.10.4

Re: [PATCH 09/19] xen: arm: Annotate registers trapped by HSR_EL1.TIDCP

[Julien Grall]

Hi Ian,

Subject: s/HSR/HCR/

On 31/03/2015 12:07, Ian Campbell wrote:
> This traps variety of implementation defined registers, so add a note
> to the default case of the respective handler.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Other than the typo in the subject:

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

Regards,

> ---
>   xen/arch/arm/traps.c |   16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ca43f79..e26e673 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1698,6 +1698,14 @@ static void do_cp15_32(struct cpu_user_regs *regs,
>            */
>           return handle_raz_wi(regs, r, cp32.read, hsr, 1);
>
> +    /*
> +     * HCR_EL2.TIDCP
> +     *
> +     * ARMv7 (DDI 0406C.b): B1.14.3
> +     * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43
> +     *
> +     * And all other unknown registers.
> +     */
>       default:
>           gdprintk(XENLOG_ERR,
>                    "%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
> @@ -1948,6 +1956,14 @@ static void do_sysreg(struct cpu_user_regs *regs,
>           dprintk(XENLOG_WARNING,
>                   "Emulation of sysreg ICC_SGI0R_EL1/ASGI1R_EL1 not supported\n");
>           return inject_undef64_exception(regs, hsr.len);
> +
> +    /*
> +     * HCR_EL2.TIDCP
> +     *
> +     * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43
> +     *
> +     * And all other unknown registers.
> +     */
>       default:
>           {
>               const struct hsr_sysreg sysreg = hsr.sysreg;
>

-- 
Julien Grall

[PATCH 10/19] xen: arm: Annotate registers trapped by CPTR_EL2.TTA

[Ian Campbell]

Add explicit handler for 64-bit CP14 accesses, with more relevant
debug message (as per other handlers) and to provide a place for the
comment.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index e26e673..9cdbda8 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1810,6 +1810,13 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
 
     case HSR_CPREG32(DBGOSLAR):
         return handle_wo_wi(regs, r, cp32.read, hsr, 1);
+
+    /*
+     * CPTR_EL2.TTA
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.16
+     * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54
+     */
     default:
         gdprintk(XENLOG_ERR,
                  "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
@@ -1824,7 +1831,7 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
     advance_pc(regs, hsr);
 }
 
-static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
+static void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr)
 {
     const struct hsr_cp64 cp64 = hsr.cp64;
 
@@ -1834,12 +1841,37 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
         return;
     }
 
+    /*
+     * CPTR_EL2.TTA
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.16
+     * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54
+     */
     gdprintk(XENLOG_ERR,
              "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
              cp64.read ? "mrrc" : "mcrr",
              cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);
     gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 access %#x\n",
              hsr.bits & HSR_CP64_REGS_MASK);
+    inject_undef_exception(regs, hsr);
+}
+
+static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    struct hsr_cp64 cp64 = hsr.cp64;
+
+    if ( !check_conditional_instr(regs, hsr) )
+    {
+        advance_pc(regs, hsr);
+        return;
+    }
+
+    gdprintk(XENLOG_ERR,
+             "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
+             cp64.read ? "mrrc" : "mcrr",
+             cp64.op1, cp64.reg1, cp64.reg2, cp64.crm, regs->pc);
+    gdprintk(XENLOG_ERR, "unhandled 64-bit CP14 DBG access %#x\n",
+             hsr.bits & HSR_CP64_REGS_MASK);
 
     inject_undef_exception(regs, hsr);
 }
@@ -1962,6 +1994,10 @@ static void do_sysreg(struct cpu_user_regs *regs,
      *
      * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43
      *
+     * CPTR_EL2.TTA
+     *
+     * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54
+     *
      * And all other unknown registers.
      */
     default:
@@ -2156,6 +2192,11 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         perfc_incr(trap_cp14_32);
         do_cp14_32(regs, hsr);
         break;
+    case HSR_EC_CP14_64:
+        GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
+        perfc_incr(trap_cp14_64);
+        do_cp14_64(regs, hsr);
+        break;
     case HSR_EC_CP14_DBG:
         GUEST_BUG_ON(!psr_mode_is_32bit(regs->cpsr));
         perfc_incr(trap_cp14_dbg);
diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h
index 46015f5..69fabe7 100644
--- a/xen/include/asm-arm/perfc_defn.h
+++ b/xen/include/asm-arm/perfc_defn.h
@@ -9,6 +9,7 @@ PERFCOUNTER(trap_wfe,      "trap: wfe")
 PERFCOUNTER(trap_cp15_32,  "trap: cp15 32-bit access")
 PERFCOUNTER(trap_cp15_64,  "trap: cp15 64-bit access")
 PERFCOUNTER(trap_cp14_32,  "trap: cp14 32-bit access")
+PERFCOUNTER(trap_cp14_64,  "trap: cp14 64-bit access")
 PERFCOUNTER(trap_cp14_dbg, "trap: cp14 dbg access")
 PERFCOUNTER(trap_cp,       "trap: cp access")
 PERFCOUNTER(trap_smc32,    "trap: 32-bit smc")
-- 
1.7.10.4

Re: [PATCH 10/19] xen: arm: Annotate registers trapped by CPTR_EL2.TTA

[Julien Grall]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> Add explicit handler for 64-bit CP14 accesses, with more relevant
> debug message (as per other handlers) and to provide a place for the
> comment.

It's a bit strange to name the patch "Annotate..." while the main change 
is 64-bit CP14 accesses.

AFAICT, this was a bug in Xen implementation. Although, I'm not sure if 
the current platform we support have Trace registers (maybe the Arndale?).

Regards,

-- 
Julien Grall

Re: [PATCH 10/19] xen: arm: Annotate registers trapped by CPTR_EL2.TTA

[Ian Campbell]

On Mon, 2015-04-06 at 13:10 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 31/03/2015 12:07, Ian Campbell wrote:
> > Add explicit handler for 64-bit CP14 accesses, with more relevant
> > debug message (as per other handlers) and to provide a place for the
> > comment.
> 
> It's a bit strange to name the patch "Annotate..." while the main change 
> is 64-bit CP14 accesses.

I mostly started off annotating things, which lead me to notice things
which are missing. I've retitled to "xen: arm: implement handling of
registers trapped by CPTR_EL2.TTA"

> AFAICT, this was a bug in Xen implementation. Although, I'm not sure if 
> the current platform we support have Trace registers (maybe the Arndale?).

Hrm, good point. I'm not sure really.

Ian.

[PATCH 11/19] xen: arm: Annotate handlers for PCTR_EL2.Tx

[Ian Campbell]

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 9cdbda8..ba120e5 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1704,6 +1704,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,
      * ARMv7 (DDI 0406C.b): B1.14.3
      * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43
      *
+     * CPTR_EL2.T{0..9,12..13}
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.12
+     * ARMv8 (DDI 0487A.d): N/A
+     *
      * And all other unknown registers.
      */
     default:
@@ -1735,6 +1740,15 @@ static void do_cp15_64(struct cpu_user_regs *regs,
         if ( !vtimer_emulate(regs, hsr) )
             return inject_undef_exception(regs, hsr);
         break;
+
+    /*
+     * CPTR_EL2.T{0..9,12..13}
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.12
+     * ARMv8 (DDI 0487A.d): N/A
+     *
+     * And all other unknown registers.
+     */
     default:
         {
             const struct hsr_cp64 cp64 = hsr.cp64;
-- 
1.7.10.4

Re: [PATCH 11/19] xen: arm: Annotate handlers for PCTR_EL2.Tx

[Julien Grall]

Hi Ian,

Subject: s/PCTR/CPTR/

On 31/03/2015 12:07, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   xen/arch/arm/traps.c |   14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 9cdbda8..ba120e5 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1704,6 +1704,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,
>        * ARMv7 (DDI 0406C.b): B1.14.3
>        * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43
>        *
> +     * CPTR_EL2.T{0..9,12..13}
> +     *
> +     * ARMv7 (DDI 0406C.b): B1.14.12
> +     * ARMv8 (DDI 0487A.d): N/A

I would also update the comment on top of WRITE_SYSREG(..., CPTR_EL2) to 
make clear that CP0..CP9 & CP12..CP13 are only traps for ARMv7.

Regards,

-- 
Julien Grall

Re: [PATCH 11/19] xen: arm: Annotate handlers for PCTR_EL2.Tx

[Ian Campbell]

On Mon, 2015-04-06 at 13:18 +0200, Julien Grall wrote:
> Hi Ian,
> 
> Subject: s/PCTR/CPTR/
> 
> On 31/03/2015 12:07, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >   xen/arch/arm/traps.c |   14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 9cdbda8..ba120e5 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1704,6 +1704,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,
> >        * ARMv7 (DDI 0406C.b): B1.14.3
> >        * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43
> >        *
> > +     * CPTR_EL2.T{0..9,12..13}
> > +     *
> > +     * ARMv7 (DDI 0406C.b): B1.14.12
> > +     * ARMv8 (DDI 0487A.d): N/A
> 
> I would also update the comment on top of WRITE_SYSREG(..., CPTR_EL2) to 
> make clear that CP0..CP9 & CP12..CP13 are only traps for ARMv7.

On v8 the corresponding bits are RES1, i.e. they always trap. I wrote:

    /* Trap all coprocessor registers (0-13) except cp10 and
     * cp11 for VFP.
     *
     * /!\ All coprocessors except cp10 and cp11 cannot be used in Xen.
     *
     * On ARM64 the TCPx bits which we set here (0..9,12,13) are all
     * RES1, i.e. they would trap whether we did this write or not.
     */

Ian.

Re: [PATCH 11/19] xen: arm: Annotate handlers for PCTR_EL2.Tx

[Julien Grall]

Hi Ian,

On 16/04/2015 17:53, Ian Campbell wrote:
> On Mon, 2015-04-06 at 13:18 +0200, Julien Grall wrote:
>> Hi Ian,
>>
>> Subject: s/PCTR/CPTR/
>>
>> On 31/03/2015 12:07, Ian Campbell wrote:
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>    xen/arch/arm/traps.c |   14 ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index 9cdbda8..ba120e5 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -1704,6 +1704,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,
>>>         * ARMv7 (DDI 0406C.b): B1.14.3
>>>         * ARMv8 (DDI 0487A.d): D1-1501 Table D1-43
>>>         *
>>> +     * CPTR_EL2.T{0..9,12..13}
>>> +     *
>>> +     * ARMv7 (DDI 0406C.b): B1.14.12
>>> +     * ARMv8 (DDI 0487A.d): N/A
>>
>> I would also update the comment on top of WRITE_SYSREG(..., CPTR_EL2) to
>> make clear that CP0..CP9 & CP12..CP13 are only traps for ARMv7.
>
> On v8 the corresponding bits are RES1, i.e. they always trap. I wrote:

Somehow I read RES0 in the spec.

>
>      /* Trap all coprocessor registers (0-13) except cp10 and
>       * cp11 for VFP.
>       *
>       * /!\ All coprocessors except cp10 and cp11 cannot be used in Xen.
>       *
>       * On ARM64 the TCPx bits which we set here (0..9,12,13) are all
>       * RES1, i.e. they would trap whether we did this write or not.
>       */

I'm fine with this change.

Regards,

-- 
Julien Grall

[PATCH 12/19] xen: arm: Annotate the handlers for HSTR_EL2.Tx

[Ian Campbell]

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ba120e5..21bef01 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1709,6 +1709,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,
      * ARMv7 (DDI 0406C.b): B1.14.12
      * ARMv8 (DDI 0487A.d): N/A
      *
+     * HSTR_EL2.Tx
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.14
+     * ARMv8 (DDI 0487A.d): D1-1507 Table D1-55
+     *
      * And all other unknown registers.
      */
     default:
@@ -1747,6 +1752,11 @@ static void do_cp15_64(struct cpu_user_regs *regs,
      * ARMv7 (DDI 0406C.b): B1.14.12
      * ARMv8 (DDI 0487A.d): N/A
      *
+     * HSTR_EL2.Tx
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.14
+     * ARMv8 (DDI 0487A.d): D1-1507 Table D1-55
+     *
      * And all other unknown registers.
      */
     default:
-- 
1.7.10.4

Re: [PATCH 12/19] xen: arm: Annotate the handlers for HSTR_EL2.Tx

[Julien Grall]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   xen/arch/arm/traps.c |   10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ba120e5..21bef01 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1709,6 +1709,11 @@ static void do_cp15_32(struct cpu_user_regs *regs,
>        * ARMv7 (DDI 0406C.b): B1.14.12
>        * ARMv8 (DDI 0487A.d): N/A
>        *
> +     * HSTR_EL2.Tx

I would prefer if you use T15 instead of Tx. This is less confusing as 
we only trap c15 and the bit T14 exists on ARMv8 (even though it's RES0).

Regards,

-- 
Julien Grall

[PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/traps.c          |   32 ++++++++++++++++++++++++++++++++
 xen/include/asm-arm/cpregs.h  |    4 ++++
 xen/include/asm-arm/sysregs.h |    1 +
 3 files changed, 37 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 21bef01..7c37cec 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1790,6 +1790,17 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
 
     switch ( hsr.bits & HSR_CP32_REGS_MASK )
     {
+    /*
+     * MDCR_EL2.TDRA
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.15
+     * ARMv8 (DDI 0487A.d): D1-1508 Table D1-57
+     *
+     * Unhandled:
+     *    DBGDRAR
+     *    DBGDSAR
+     */
+
     case HSR_CPREG32(DBGDIDR):
         /*
          * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
@@ -1840,6 +1851,8 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
      *
      * ARMv7 (DDI 0406C.b): B1.14.16
      * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54
+     *
+     * And all other unknown registers.
      */
     default:
         gdprintk(XENLOG_ERR,
@@ -1870,6 +1883,17 @@ static void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr)
      *
      * ARMv7 (DDI 0406C.b): B1.14.16
      * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54
+     *
+     * MDCR_EL2.TDRA
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.15
+     * ARMv8 (DDI 0487A.d): D1-1508 Table D1-57
+     *
+     * Unhandled:
+     *    DBGDRAR64
+     *    DBGDSAR64
+     *
+     * And all other unknown registers.
      */
     gdprintk(XENLOG_ERR,
              "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
@@ -1936,6 +1960,14 @@ static void do_sysreg(struct cpu_user_regs *regs,
            *x = v->arch.actlr;
         break;
 
+    /*
+     * MDCR_EL2.TDRA
+     *
+     * 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);
+
     /* RAZ/WI registers: */
     /*  - Debug */
     case HSR_SYSREG_MDSCR_EL1:
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index afe9148..9db8cfd 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -89,10 +89,14 @@
 #define TEECR           p14,6,c0,c0,0   /* ThumbEE Configuration Register */
 
 /* CP14 CR1: */
+#define DBGDRAR64       p14,0,c1        /* Debug ROM Address Register (64-bit access) */
+#define DBGDRAR         p14,0,c1,c0,0   /* Debug ROM Address Register (32-bit access) */
 #define TEEHBR          p14,6,c1,c0,0   /* ThumbEE Handler Base Register */
 #define JOSCR           p14,7,c1,c0,0   /* Jazelle OS Control Register */
 
 /* CP14 CR2: */
+#define DBGDSAR64       p14,0,c2        /* Debug Self Address Offset Register (64-bit access) */
+#define DBGDSAR         p14,0,c2,c0,0   /* Debug Self Address Offset Register (32-bit access) */
 #define JMCR            p14,7,c2,c0,0   /* Jazelle Main Configuration Register */
 
 
diff --git a/xen/include/asm-arm/sysregs.h b/xen/include/asm-arm/sysregs.h
index d75e154..55457fd 100644
--- a/xen/include/asm-arm/sysregs.h
+++ b/xen/include/asm-arm/sysregs.h
@@ -45,6 +45,7 @@
 #define HSR_SYSREG_DCCISW         HSR_SYSREG(1,0,c7,c14,2)
 
 #define HSR_SYSREG_MDSCR_EL1      HSR_SYSREG(2,0,c0,c2,2)
+#define HSR_SYSREG_MDRAR_EL1      HSR_SYSREG(2,0,c1,c0,0)
 #define HSR_SYSREG_OSLAR_EL1      HSR_SYSREG(2,0,c1,c0,4)
 #define HSR_SYSREG_OSDLR_EL1      HSR_SYSREG(2,0,c1,c3,4)
 #define HSR_SYSREG_MDCCSR_EL0     HSR_SYSREG(2,3,c0,c1,0)
-- 
1.7.10.4

Re: [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA

[Julien Grall]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   xen/arch/arm/traps.c          |   32 ++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/cpregs.h  |    4 ++++
>   xen/include/asm-arm/sysregs.h |    1 +
>   3 files changed, 37 insertions(+)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 21bef01..7c37cec 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1790,6 +1790,17 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
>
>       switch ( hsr.bits & HSR_CP32_REGS_MASK )
>       {
> +    /*
> +     * MDCR_EL2.TDRA
> +     *
> +     * ARMv7 (DDI 0406C.b): B1.14.15
> +     * ARMv8 (DDI 0487A.d): D1-1508 Table D1-57
> +     *
> +     * Unhandled:
> +     *    DBGDRAR
> +     *    DBGDSAR
> +     */
> +

Why did you put the comment here? For AArch32, only DBGDRAR and DBGSAR 
are trapped with this bit.

I think this should be moved above the label default.

>       case HSR_CPREG32(DBGDIDR):
>           /*
>            * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
> @@ -1840,6 +1851,8 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
>        *
>        * ARMv7 (DDI 0406C.b): B1.14.16
>        * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54
> +     *
> +     * And all other unknown registers.
>        */
>       default:
>           gdprintk(XENLOG_ERR,
> @@ -1870,6 +1883,17 @@ static void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr)
>        *
>        * ARMv7 (DDI 0406C.b): B1.14.16
>        * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54
> +     *
> +     * MDCR_EL2.TDRA
> +     *
> +     * ARMv7 (DDI 0406C.b): B1.14.15
> +     * ARMv8 (DDI 0487A.d): D1-1508 Table D1-57
> +     *
> +     * Unhandled:
> +     *    DBGDRAR64
> +     *    DBGDSAR64

This is confusing. The real name of the register is DBGDRAR. I would say 
"DBGDRAR 64-bit".

Furthermore, this is the only registers not handled on AArch32 for this 
bit. This is rather strange to list them while you didn't do it for the 
trace registers.

> +     *
> +     * And all other unknown registers.

For consistency, I would have add this part of the comment in patch #10 
(where the comment has been added).

Anyway, the patch is already written so I'm fine with it.
>        */
>       gdprintk(XENLOG_ERR,
>                "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
> @@ -1936,6 +1960,14 @@ static void do_sysreg(struct cpu_user_regs *regs,
>              *x = v->arch.actlr;
>           break;
>
> +    /*
> +     * MDCR_EL2.TDRA
> +     *
> +     * 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);

This change should be in a separate patch or mention in the commit message.

Regards,

-- 
Julien Grall

Re: [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA

[Ian Campbell]

On Mon, 2015-04-06 at 15:24 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 31/03/2015 12:07, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >   xen/arch/arm/traps.c          |   32 ++++++++++++++++++++++++++++++++
> >   xen/include/asm-arm/cpregs.h  |    4 ++++
> >   xen/include/asm-arm/sysregs.h |    1 +
> >   3 files changed, 37 insertions(+)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 21bef01..7c37cec 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1790,6 +1790,17 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
> >
> >       switch ( hsr.bits & HSR_CP32_REGS_MASK )
> >       {
> > +    /*
> > +     * MDCR_EL2.TDRA
> > +     *
> > +     * ARMv7 (DDI 0406C.b): B1.14.15
> > +     * ARMv8 (DDI 0487A.d): D1-1508 Table D1-57
> > +     *
> > +     * Unhandled:
> > +     *    DBGDRAR
> > +     *    DBGDSAR
> > +     */
> > +
> 
> Why did you put the comment here? For AArch32, only DBGDRAR and DBGSAR 
> are trapped with this bit.
> 
> I think this should be moved above the label default.

Yes, not sure how it got out of place here. Moved.

> 
> >       case HSR_CPREG32(DBGDIDR):
> >           /*
> >            * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
> > @@ -1840,6 +1851,8 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
> >        *
> >        * ARMv7 (DDI 0406C.b): B1.14.16
> >        * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54
> > +     *
> > +     * And all other unknown registers.
> >        */
> >       default:
> >           gdprintk(XENLOG_ERR,
> > @@ -1870,6 +1883,17 @@ static void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr)
> >        *
> >        * ARMv7 (DDI 0406C.b): B1.14.16
> >        * ARMv8 (DDI 0487A.d): D1-1507 Table D1-54
> > +     *
> > +     * MDCR_EL2.TDRA
> > +     *
> > +     * ARMv7 (DDI 0406C.b): B1.14.15
> > +     * ARMv8 (DDI 0487A.d): D1-1508 Table D1-57
> > +     *
> > +     * Unhandled:
> > +     *    DBGDRAR64
> > +     *    DBGDSAR64
> 
> This is confusing. The real name of the register is DBGDRAR. I would say 
> "DBGDRAR 64-bit".

Yes, this was a tricky one because the register is actually two (a 32
and 64 version), hence in the #define I had to add a suffix and I
duplicated that here.

Instead I'll make both of them "DBGDRAR (NN-bit accesses)", similar to
what I did for the #defines

> 
> Furthermore, this is the only registers not handled on AArch32 for this 
> bit. This is rather strange to list them while you didn't do it for the 
> trace registers.

My intention was that every register trapped by a bit which we set be
listed somewhere, to make it easier to cross reference with the docs and
check we haven't accidentally forgotten something (as opposed to
deliberately ignoring as indicated by these comments).

You seem to be saying I've missed some trace registers, which ones?

> 
> > +     *
> > +     * And all other unknown registers.
> 
> For consistency, I would have add this part of the comment in patch #10 
> (where the comment has been added).
> 
> Anyway, the patch is already written so I'm fine with it.

I was rebasing any way so I did this.

> >        */
> >       gdprintk(XENLOG_ERR,
> >                "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
> > @@ -1936,6 +1960,14 @@ static void do_sysreg(struct cpu_user_regs *regs,
> >              *x = v->arch.actlr;
> >           break;
> >
> > +    /*
> > +     * MDCR_EL2.TDRA
> > +     *
> > +     * 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);
> 
> This change should be in a separate patch or mention in the commit message.

I just added a mention of it since it is the AArch64 version of DBGDRAR.
I can't remember why I made this raz instead of trapping like with
DBGDRAR, but since I already wrote it I left them as is.

Ian

Re: [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA

[Julien Grall]

Hi Ian,

On 17/04/2015 16:51, Ian Campbell wrote:
>> Furthermore, this is the only registers not handled on AArch32 for this
>> bit. This is rather strange to list them while you didn't do it for the
>> trace registers.
>
> My intention was that every register trapped by a bit which we set be
> listed somewhere, to make it easier to cross reference with the docs and
> check we haven't accidentally forgotten something (as opposed to
> deliberately ignoring as indicated by these comments).
>
> You seem to be saying I've missed some trace registers, which ones?

I meant that you didn't list the trace registers trapped but unhandled. 
Although I wasn't able to find a list, is it trace module specific? If 
so maybe a comment would be good?

Regards,

-- 
Julien Grall

Re: [PATCH 13/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDRA

[Ian Campbell]

On Tue, 2015-04-21 at 13:26 +0500, Julien Grall wrote:
> Hi Ian,
> 
> On 17/04/2015 16:51, Ian Campbell wrote:
> >> Furthermore, this is the only registers not handled on AArch32 for this
> >> bit. This is rather strange to list them while you didn't do it for the
> >> trace registers.
> >
> > My intention was that every register trapped by a bit which we set be
> > listed somewhere, to make it easier to cross reference with the docs and
> > check we haven't accidentally forgotten something (as opposed to
> > deliberately ignoring as indicated by these comments).
> >
> > You seem to be saying I've missed some trace registers, which ones?
> 
> I meant that you didn't list the trace registers trapped but unhandled. 
> Although I wasn't able to find a list, is it trace module specific? If 
> so maybe a comment would be good?

I think maybe you are talking about the things trapped by CPTR_EL2.TTA
rather than MDCR_EL2.TDRA (the subject of this patch)?

The table referenced for CPTR_EL2.TTA just says "All implemented trace
registers", rather than listing anything specific. I could add a similar
comment to the relevant patch.

Looks like HCR_EL2.TIDCP is similarly lacking a comment for the
unhandled ones. I'll add one.

> 
> Regards,
> 

[PATCH 14/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDOSA

[Ian Campbell]

Gather the affected handlers in a single place per trap type.

Add some HSR_SYSREG and AArch32 defines for those registers (because
I'd already typed them in when I realised I didn't need them).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/traps.c          |   37 +++++++++++++++++++++++++++++--------
 xen/include/asm-arm/cpregs.h  |    2 ++
 xen/include/asm-arm/sysregs.h |    2 ++
 3 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 7c37cec..518f047 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1801,6 +1801,21 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
      *    DBGDSAR
      */
 
+    /*
+     * MDCR_EL2.TDOSA
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.15
+     * ARMv8 (DDI 0487A.d): D1-1509 Table D1-58
+     *
+     * Unhandled:
+     *    DBGOSLSR
+     *    DBGPRCR
+     */
+    case HSR_CPREG32(DBGOSLAR):
+        return handle_wo_wi(regs, r, cp32.read, hsr, 1);
+    case HSR_CPREG32(DBGOSDLR):
+        return handle_raz_wi(regs, r, cp32.read, hsr, 1);
+
     case HSR_CPREG32(DBGDIDR):
         /*
          * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
@@ -1840,12 +1855,8 @@ 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):
-    case HSR_CPREG32(DBGOSDLR):
         return handle_raz_wi(regs, r, cp32.read, hsr, 1);
 
-    case HSR_CPREG32(DBGOSLAR):
-        return handle_wo_wi(regs, r, cp32.read, hsr, 1);
-
     /*
      * CPTR_EL2.TTA
      *
@@ -1968,6 +1979,20 @@ static void do_sysreg(struct cpu_user_regs *regs,
     case HSR_SYSREG_MDRAR_EL1:
         return handle_ro_raz(regs, x, hsr.sysreg.read, hsr, 1);
 
+    /*
+     * MDCR_EL2.TDOSA
+     *
+     * ARMv8 (DDI 0487A.d): D1-1509 Table D1-58
+     *
+     * Unhandled:
+     *    OSLSR_EL1
+     *    DBGPRCR_EL1
+     */
+    case HSR_SYSREG_OSLAR_EL1:
+        return handle_wo_wi(regs, x, hsr.sysreg.read, hsr, 1);
+    case HSR_SYSREG_OSDLR_EL1:
+        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
+
     /* RAZ/WI registers: */
     /*  - Debug */
     case HSR_SYSREG_MDSCR_EL1:
@@ -1977,8 +2002,6 @@ static void do_sysreg(struct cpu_user_regs *regs,
     /*  - Watchpoints */
     HSR_SYSREG_DBG_CASES(DBGWVR):
     HSR_SYSREG_DBG_CASES(DBGWCR):
-    /*  - Double Lock Register */
-    case HSR_SYSREG_OSDLR_EL1:
     /*  - Perf monitors */
     case HSR_SYSREG_PMINTENSET_EL1:
     case HSR_SYSREG_PMINTENCLR_EL1:
@@ -2021,8 +2044,6 @@ static void do_sysreg(struct cpu_user_regs *regs,
         return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
 
     /* Write only, Write ignore registers: */
-    case HSR_SYSREG_OSLAR_EL1:
-        return handle_wo_wi(regs, x, hsr.sysreg.read, hsr, 1);
 
     case HSR_SYSREG_CNTP_CTL_EL0:
     case HSR_SYSREG_CNTP_TVAL_EL0:
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index 9db8cfd..e5cb00c 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -83,7 +83,9 @@
 #define DBGBVR1         p14,0,c0,c1,4   /* Breakpoint Value 1 */
 #define DBGBCR1         p14,0,c0,c1,5   /* Breakpoint Control 1 */
 #define DBGOSLAR        p14,0,c1,c0,4   /* OS Lock Access */
+#define DBGOSLSR        p14,0,c1,c1,4   /* OS Lock Status Register */
 #define DBGOSDLR        p14,0,c1,c3,4   /* OS Double Lock */
+#define DBGPRCR         p14,0,c1,c4,4   /* Debug Power Control Register */
 
 /* CP14 CR0: */
 #define TEECR           p14,6,c0,c0,0   /* ThumbEE Configuration Register */
diff --git a/xen/include/asm-arm/sysregs.h b/xen/include/asm-arm/sysregs.h
index 55457fd..570f43e 100644
--- a/xen/include/asm-arm/sysregs.h
+++ b/xen/include/asm-arm/sysregs.h
@@ -47,7 +47,9 @@
 #define HSR_SYSREG_MDSCR_EL1      HSR_SYSREG(2,0,c0,c2,2)
 #define HSR_SYSREG_MDRAR_EL1      HSR_SYSREG(2,0,c1,c0,0)
 #define HSR_SYSREG_OSLAR_EL1      HSR_SYSREG(2,0,c1,c0,4)
+#define HSR_SYSREG_OSLSR_EL1      HSR_SYSREG(2,0,c1,c1,4)
 #define HSR_SYSREG_OSDLR_EL1      HSR_SYSREG(2,0,c1,c3,4)
+#define HSR_SYSREG_DBGPRCR_EL1    HSR_SYSREG(2,0,c1,c4,4)
 #define HSR_SYSREG_MDCCSR_EL0     HSR_SYSREG(2,3,c0,c1,0)
 
 #define HSR_SYSREG_DBGBVRn_EL1(n) HSR_SYSREG(2,0,c0,c##n,4)
-- 
1.7.10.4

[PATCH 15/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDA

[Ian Campbell]

Gather the affected handlers in a single place per trap type.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 518f047..0ab5e56 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1816,6 +1816,28 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
     case HSR_CPREG32(DBGOSDLR):
         return handle_raz_wi(regs, r, cp32.read, hsr, 1);
 
+    /*
+     * MDCR_EL2.TDA
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.15
+     * ARMv8 (DDI 0487A.d): D1-1510 Table D1-59
+     *
+     * Unhandled:
+     *    DBGDCCINT
+     *    DBGDTRRXint
+     *    DBGDTRTXint
+     *    DBGWFAR
+     *    DBGDTRTXext
+     *    DBGDTRRXext,
+     *    DBGBXVR<n>
+     *    DBGCLAIMSET
+     *    DBGCLAIMCLR
+     *    DBGAUTHSTATUS
+     *    DBGDEVID
+     *    DBGDEVID1
+     *    DBGDEVID2
+     *    DBGOSECCR
+     */
     case HSR_CPREG32(DBGDIDR):
         /*
          * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
@@ -1925,6 +1947,17 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
         return;
     }
 
+    /*
+     * MDCR_EL2.TDOSA
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.15
+     * ARMv8 (DDI 0487A.d): D1-1509 Table D1-58
+     *
+     * Unhandled:
+     *    DBGDTRTXint
+     *    DBGDTRRXint
+     */
+
     gdprintk(XENLOG_ERR,
              "%s p14, %d, r%d, r%d, cr%d @ 0x%"PRIregister"\n",
              cp64.read ? "mrrc" : "mcrr",
@@ -1993,15 +2026,38 @@ static void do_sysreg(struct cpu_user_regs *regs,
     case HSR_SYSREG_OSDLR_EL1:
         return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
 
-    /* RAZ/WI registers: */
-    /*  - Debug */
+    /*
+     * MDCR_EL2.TDA
+     *
+     * ARMv8 (DDI 0487A.d): D1-1510 Table D1-59
+     *
+     * Unhandled:
+     *    MDCCINT_EL1
+     *    DBGDTR_EL0
+     *    DBGDTRRX_EL0
+     *    DBGDTRTX_EL0
+     *    OSDTRRX_EL1
+     *    OSDTRTX_EL1
+     *    OSECCR_EL1
+     *    DBGCLAIMSET_EL1
+     *    DBGCLAIMCLR_EL1
+     *    DBGAUTHSTATUS_EL1
+     */
     case HSR_SYSREG_MDSCR_EL1:
-    /*  - Breakpoints */
+        return handle_raz_wi(regs, x, 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);
     HSR_SYSREG_DBG_CASES(DBGBVR):
     HSR_SYSREG_DBG_CASES(DBGBCR):
-    /*  - Watchpoints */
     HSR_SYSREG_DBG_CASES(DBGWVR):
     HSR_SYSREG_DBG_CASES(DBGWCR):
+        return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
+
+    /* RAZ/WI registers: */
     /*  - Perf monitors */
     case HSR_SYSREG_PMINTENSET_EL1:
     case HSR_SYSREG_PMINTENCLR_EL1:
@@ -2011,13 +2067,6 @@ static void do_sysreg(struct cpu_user_regs *regs,
          */
         return handle_raz_wi(regs, x, 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);
-
     /* - Perf monitors */
     case HSR_SYSREG_PMUSERENR_EL0:
         /* RO at EL0. RAZ/WI at EL1 */
-- 
1.7.10.4

Re: [PATCH 15/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDA

[Julien Grall]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> Gather the affected handlers in a single place per trap type.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   xen/arch/arm/traps.c |   71 ++++++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 60 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 518f047..0ab5e56 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1816,6 +1816,28 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
>       case HSR_CPREG32(DBGOSDLR):
>           return handle_raz_wi(regs, r, cp32.read, hsr, 1);
>
> +    /*
> +     * MDCR_EL2.TDA
> +     *
> +     * ARMv7 (DDI 0406C.b): B1.14.15
> +     * ARMv8 (DDI 0487A.d): D1-1510 Table D1-59
> +     *
> +     * Unhandled:
> +     *    DBGDCCINT
> +     *    DBGDTRRXint
> +     *    DBGDTRTXint
> +     *    DBGWFAR
> +     *    DBGDTRTXext
> +     *    DBGDTRRXext,
> +     *    DBGBXVR<n>
> +     *    DBGCLAIMSET
> +     *    DBGCLAIMCLR
> +     *    DBGAUTHSTATUS
> +     *    DBGDEVID
> +     *    DBGDEVID1
> +     *    DBGDEVID2
> +     *    DBGOSECCR

Listing unhandled registers doesn't bring much information. It's more 
useful to explain that we choose to implement a dummy Debug support.

Also, it's very confusing that you are mixing AArch64 name with AArch32 
name. The former is used for the traps register while the latter is used 
for register name.

> +     */
>       case HSR_CPREG32(DBGDIDR):
>           /*
>            * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
> @@ -1925,6 +1947,17 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
>           return;
>       }
>
> +    /*
> +     * MDCR_EL2.TDOSA

Did you mean MDCR_EL2.TDA?

[..]

>       case HSR_SYSREG_MDSCR_EL1:
> -    /*  - Breakpoints */

[..]

> -    /*  - Watchpoints */

I think this 2 comments was useful in order to split the list of registers.

regards,

-- 
Julien Grall

Re: [PATCH 15/19] xen: arm: Annotate registers trapped by MDCR_EL2.TDA

[Ian Campbell]

On Mon, 2015-04-06 at 15:41 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 31/03/2015 12:07, Ian Campbell wrote:
> > Gather the affected handlers in a single place per trap type.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >   xen/arch/arm/traps.c |   71 ++++++++++++++++++++++++++++++++++++++++++--------
> >   1 file changed, 60 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 518f047..0ab5e56 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1816,6 +1816,28 @@ static void do_cp14_32(struct cpu_user_regs *regs, const union hsr hsr)
> >       case HSR_CPREG32(DBGOSDLR):
> >           return handle_raz_wi(regs, r, cp32.read, hsr, 1);
> >
> > +    /*
> > +     * MDCR_EL2.TDA
> > +     *
> > +     * ARMv7 (DDI 0406C.b): B1.14.15
> > +     * ARMv8 (DDI 0487A.d): D1-1510 Table D1-59
> > +     *
> > +     * Unhandled:
> > +     *    DBGDCCINT
> > +     *    DBGDTRRXint
> > +     *    DBGDTRTXint
> > +     *    DBGWFAR
> > +     *    DBGDTRTXext
> > +     *    DBGDTRRXext,
> > +     *    DBGBXVR<n>
> > +     *    DBGCLAIMSET
> > +     *    DBGCLAIMCLR
> > +     *    DBGAUTHSTATUS
> > +     *    DBGDEVID
> > +     *    DBGDEVID1
> > +     *    DBGDEVID2
> > +     *    DBGOSECCR
> 
> Listing unhandled registers doesn't bring much information.

Actually it does and it was quite deliberate and part of the purpose of
the series. By listing these unhandled registers you can now look at the
docs (in particular the ARMv8 table is very good for this) and see
immediately that every register is either handled or listed as
unhandled.

Without that you need to remember why certain things aren't handled in
order to distinguish them from things which have been accidentally
forgotten.

> Also, it's very confusing that you are mixing AArch64 name with AArch32 
> name. The former is used for the traps register while the latter is used 
> for register name.

For the traps register where it is similar enough I chose to only list
the Aarch64 name, since that is what we use when we write it. I could
write both I suppose but it didn't seem especially useful.

For the registers themselves it is easier to correlate with the docs if
the name which is relevant to the context (e.g. an arm32 vs arm64
handler). Comparing two lists where one is Aarch64 names and the other
Aarch32 is hard, especially in the dbg registers where the names don't
always correlate especially obviously.

The only change I'd be willing to make here would be to list HDCR.TDA
after MDCR_EL2.TDA.

> > +     */
> >       case HSR_CPREG32(DBGDIDR):
> >           /*
> >            * Read-only register. Accessible by EL0 if DBGDSCRext.UDCCdis
> > @@ -1925,6 +1947,17 @@ static void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr)
> >           return;
> >       }
> >
> > +    /*
> > +     * MDCR_EL2.TDOSA
> 
> Did you mean MDCR_EL2.TDA?
> 
> [..]

No, this hunk should have been in the previous patch. Moved.

> 
> >       case HSR_SYSREG_MDSCR_EL1:
> > -    /*  - Breakpoints */
> 
> [..]
> 
> > -    /*  - Watchpoints */
> 
> I think this 2 comments was useful in order to split the list of registers.

In the new code this is just:
    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);

I don't think the comments were adding much any more, especially given
we aren't actually doing anything with them.

Ian.

[PATCH 16/19] xen: arm: Annotate registers trapped by MDCR_EL2.TPM and TPMCR

[Ian Campbell]

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 0ab5e56..64d55dc 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1661,6 +1661,24 @@ static void do_cp15_32(struct cpu_user_regs *regs,
            *r = v->arch.actlr;
         break;
 
+    /*
+     * MDCR_EL2.TPM
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.17
+     * ARMv8 (DDI 0487A.d): D1-1511 Table D1-61
+     *
+     * Unhandled:
+     *    PMEVCNTR<n>
+     *    PMEVTYPER<n>
+     *    PMCCFILTR
+     *
+     * MDCR_EL2.TPMCR
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.17
+     * ARMv8 (DDI 0487A.d): D1-1511 Table D1-62
+     *
+     * NB: Both MDCR_EL2.TPM and MDCR_EL2.TPMCR cause trapping of PMCR.
+     */
     /* We could trap ID_DFR0 and tell the guest we don't support
      * performance monitoring, but Linux doesn't check the ID_DFR0.
      * Therefore it will read PMCR.
@@ -1675,7 +1693,6 @@ static void do_cp15_32(struct cpu_user_regs *regs,
             return handle_ro_raz(regs, r, cp32.read, hsr, 0);
         else
             return handle_raz_wi(regs, r, 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. */
@@ -2057,8 +2074,22 @@ static void do_sysreg(struct cpu_user_regs *regs,
     HSR_SYSREG_DBG_CASES(DBGWCR):
         return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
 
-    /* RAZ/WI registers: */
-    /*  - Perf monitors */
+    /*
+     * MDCR_EL2.TPM
+     *
+     * ARMv8 (DDI 0487A.d): D1-1511 Table D1-61
+     *
+     * Unhandled:
+     *    PMEVCNTR<n>_EL0
+     *    PMEVTYPER<n>_EL0
+     *    PMCCFILTR_EL0
+     * MDCR_EL2.TPMCR
+     *
+     * ARMv7 (DDI 0406C.b): B1.14.17
+     * ARMv8 (DDI 0487A.d): D1-1511 Table D1-62
+     *
+     * NB: Both MDCR_EL2.TPM and MDCR_EL2.TPMCR cause trapping of PMCR.
+     */
     case HSR_SYSREG_PMINTENSET_EL1:
     case HSR_SYSREG_PMINTENCLR_EL1:
         /*
@@ -2066,8 +2097,6 @@ static void do_sysreg(struct cpu_user_regs *regs,
          * undef.
          */
         return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
-
-    /* - Perf monitors */
     case HSR_SYSREG_PMUSERENR_EL0:
         /* RO at EL0. RAZ/WI at EL1 */
         if ( psr_mode_is_user(regs) )
-- 
1.7.10.4

[PATCH 17/19] xen: arm: Remove CNTPCT_EL0 trap handling.

[Ian Campbell]

We set CNTHCTL_EL2.EL1PCTEN and therefore according to ARMv8 (DDI
0487A.d) D1-1510 Table D1-60 we are not trapping this.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 64d55dc..1c9cf21 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1757,7 +1757,6 @@ static void do_cp15_64(struct cpu_user_regs *regs,
 
     switch ( hsr.bits & HSR_CP64_REGS_MASK )
     {
-    case HSR_CPREG64(CNTPCT):
     case HSR_CPREG64(CNTP_CVAL):
         if ( !vtimer_emulate(regs, hsr) )
             return inject_undef_exception(regs, hsr);
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index be65c9f..685bfea 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -243,28 +243,6 @@ static int vtimer_cntp_cval(struct cpu_user_regs *regs, uint64_t *r, int read)
     }
     return 1;
 }
-static int vtimer_cntpct(struct cpu_user_regs *regs, uint64_t *r, int read)
-{
-    struct vcpu *v = current;
-    uint64_t ticks;
-    s_time_t now;
-
-    if ( read )
-    {
-        if ( !ACCESS_ALLOWED(regs, EL0PCTEN) )
-            return 0;
-        now = NOW() - v->domain->arch.phys_timer_base.offset;
-        ticks = ns_to_ticks(now);
-        *r = ticks;
-        return 1;
-    }
-    else
-    {
-        gprintk(XENLOG_DEBUG, "WRITE to R/O CNTPCT\n");
-        return 0;
-    }
-}
-
 
 static int vtimer_emulate_cp32(struct cpu_user_regs *regs, union hsr hsr)
 {
@@ -303,11 +281,6 @@ static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
 
     switch ( hsr.bits & HSR_CP64_REGS_MASK )
     {
-    case HSR_CPREG64(CNTPCT):
-        if ( !vtimer_cntpct(regs, &x, cp64.read) )
-            return 0;
-        break;
-
     case HSR_CPREG64(CNTP_CVAL):
         if ( !vtimer_cntp_cval(regs, &x, cp64.read) )
             return 0;
@@ -356,9 +329,6 @@ static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
     case HSR_SYSREG_CNTP_CVAL_EL0:
         return vtimer_cntp_cval(regs, x, sysreg.read);
 
-    case HSR_SYSREG_CNTPCT_EL0:
-        return vtimer_cntpct(regs, x, sysreg.read);
-
     default:
         return 0;
     }
-- 
1.7.10.4

Re: [PATCH 17/19] xen: arm: Remove CNTPCT_EL0 trap handling.

[Julien Grall]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> We set CNTHCTL_EL2.EL1PCTEN and therefore according to ARMv8 (DDI
> 0487A.d) D1-1510 Table D1-60 we are not trapping this.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Julien Grall <julien.grall@citrix.com>

Regards,

-- 
Julien Grall

[PATCH 18/19] xen: arm: Annotate registers trapped when CNTHCTL_EL2.EL1PCEN == 0

[Ian Campbell]

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 1c9cf21..cc5b8dd 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1642,6 +1642,12 @@ static void do_cp15_32(struct cpu_user_regs *regs,
 
     switch ( hsr.bits & HSR_CP32_REGS_MASK )
     {
+    /*
+     * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
+     *
+     * ARMv7 (DDI 0406C.b): B4.1.22
+     * ARMv8 (DDI 0487A.d): D1-1510 Table D1-60
+     */
     case HSR_CPREG32(CNTP_CTL):
     case HSR_CPREG32(CNTP_TVAL):
         if ( !vtimer_emulate(regs, hsr) )
@@ -1757,6 +1763,12 @@ static void do_cp15_64(struct cpu_user_regs *regs,
 
     switch ( hsr.bits & HSR_CP64_REGS_MASK )
     {
+    /*
+     * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
+     *
+     * ARMv7 (DDI 0406C.b): B4.1.22
+     * ARMv8 (DDI 0487A.d): D1-1510 Table D1-60
+     */
     case HSR_CPREG64(CNTP_CVAL):
         if ( !vtimer_emulate(regs, hsr) )
             return inject_undef_exception(regs, hsr);
@@ -2120,14 +2132,18 @@ static void do_sysreg(struct cpu_user_regs *regs,
          */
         return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
 
-    /* Write only, Write ignore registers: */
-
+    /*
+     * !CNTHCTL_EL2.EL1PCEN
+     *
+     * ARMv8 (DDI 0487A.d): D1-1510 Table D1-60
+     */
     case HSR_SYSREG_CNTP_CTL_EL0:
     case HSR_SYSREG_CNTP_TVAL_EL0:
     case HSR_SYSREG_CNTP_CVAL_EL0:
         if ( !vtimer_emulate(regs, hsr) )
             return inject_undef_exception(regs, hsr);
         break;
+
     case HSR_SYSREG_ICC_SGI1R_EL1:
         if ( !vgic_emulate(regs, hsr) )
         {
-- 
1.7.10.4

Re: [PATCH 18/19] xen: arm: Annotate registers trapped when CNTHCTL_EL2.EL1PCEN == 0

[Julien Grall]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   xen/arch/arm/traps.c |   20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 1c9cf21..cc5b8dd 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1642,6 +1642,12 @@ static void do_cp15_32(struct cpu_user_regs *regs,
>
>       switch ( hsr.bits & HSR_CP32_REGS_MASK )
>       {
> +    /*
> +     * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN

I will be picky. The listing here is ARMv8 (AArch64) / ARMv7, but below 
it's ARMv7 / ARMv8.

> +     *
> +     * ARMv7 (DDI 0406C.b): B4.1.22
> +     * ARMv8 (DDI 0487A.d): D1-1510 Table D1-60
> +     */
>       case HSR_CPREG32(CNTP_CTL):
>       case HSR_CPREG32(CNTP_TVAL):
>           if ( !vtimer_emulate(regs, hsr) )
> @@ -1757,6 +1763,12 @@ static void do_cp15_64(struct cpu_user_regs *regs,
>
>       switch ( hsr.bits & HSR_CP64_REGS_MASK )
>       {
> +    /*
> +     * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
> +     *
> +     * ARMv7 (DDI 0406C.b): B4.1.22
> +     * ARMv8 (DDI 0487A.d): D1-1510 Table D1-60
> +     */
>       case HSR_CPREG64(CNTP_CVAL):
>           if ( !vtimer_emulate(regs, hsr) )
>               return inject_undef_exception(regs, hsr);
> @@ -2120,14 +2132,18 @@ static void do_sysreg(struct cpu_user_regs *regs,
>            */
>           return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
>
> -    /* Write only, Write ignore registers: */
> -

This comment should have been dropped in patch #14.

Regards,

-- 
Julien Grall

Re: [PATCH 18/19] xen: arm: Annotate registers trapped when CNTHCTL_EL2.EL1PCEN == 0

[Ian Campbell]

On Mon, 2015-04-06 at 15:58 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 31/03/2015 12:07, Ian Campbell wrote:
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >   xen/arch/arm/traps.c |   20 ++++++++++++++++++--
> >   1 file changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 1c9cf21..cc5b8dd 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1642,6 +1642,12 @@ static void do_cp15_32(struct cpu_user_regs *regs,
> >
> >       switch ( hsr.bits & HSR_CP32_REGS_MASK )
> >       {
> > +    /*
> > +     * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
> 
> I will be picky. The listing here is ARMv8 (AArch64) / ARMv7, but below 
> it's ARMv7 / ARMv8.

I'm not spotting the "below", which one did you mean?

I was trying to use the AArch64 names only when they were reasonably
similar (i.e. just differing in H prefix vs _EL2 suffix or otherwise
fairly obvious) and only use both names if there was some potentially
confusing difference in the naming.

> 
> > +     *
> > +     * ARMv7 (DDI 0406C.b): B4.1.22
> > +     * ARMv8 (DDI 0487A.d): D1-1510 Table D1-60
> > +     */
> >       case HSR_CPREG32(CNTP_CTL):
> >       case HSR_CPREG32(CNTP_TVAL):
> >           if ( !vtimer_emulate(regs, hsr) )
> > @@ -1757,6 +1763,12 @@ static void do_cp15_64(struct cpu_user_regs *regs,
> >
> >       switch ( hsr.bits & HSR_CP64_REGS_MASK )
> >       {
> > +    /*
> > +     * !CNTHCTL_EL2.EL1PCEN / !CNTHCTL.PL1PCEN
> > +     *
> > +     * ARMv7 (DDI 0406C.b): B4.1.22
> > +     * ARMv8 (DDI 0487A.d): D1-1510 Table D1-60
> > +     */
> >       case HSR_CPREG64(CNTP_CVAL):
> >           if ( !vtimer_emulate(regs, hsr) )
> >               return inject_undef_exception(regs, hsr);
> > @@ -2120,14 +2132,18 @@ static void do_sysreg(struct cpu_user_regs *regs,
> >            */
> >           return handle_raz_wi(regs, x, hsr.sysreg.read, hsr, 1);
> >
> > -    /* Write only, Write ignore registers: */
> > -
> 
> This comment should have been dropped in patch #14.

Moved.

Ian.

[PATCH 19/19] xen: arm: Annotate source of ICC SGI register trapping

[Ian Campbell]

I was unable to find an ARMv8 ARM reference to this, so refer to the
GIC Architecture Specification instead.

ARMv8 ARM does cover other ways of trapping these accesses via
ICH_HCR_EL2 but we don't use those and they trap additional registers
as well.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index cc5b8dd..71e349a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2144,6 +2144,12 @@ static void do_sysreg(struct cpu_user_regs *regs,
             return inject_undef_exception(regs, hsr);
         break;
 
+    /*
+     * HCR_EL2.FMO or HCR_EL2.IMO
+     *
+     * ARMv8: GIC Architecture Specification (PRD03-GENC-010745 24.0)
+     *        Section 4.6.8.
+     */
     case HSR_SYSREG_ICC_SGI1R_EL1:
         if ( !vgic_emulate(regs, hsr) )
         {
-- 
1.7.10.4

Re: [PATCH 19/19] xen: arm: Annotate source of ICC SGI register trapping

[Julien Grall]

Hi Ian,

On 31/03/2015 12:07, Ian Campbell wrote:
> I was unable to find an ARMv8 ARM reference to this, so refer to the
> GIC Architecture Specification instead.
>
> ARMv8 ARM does cover other ways of trapping these accesses via
> ICH_HCR_EL2 but we don't use those and they trap additional registers
> as well.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

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

Regards,

-- 
Julien Grall