[PATCH 0/4] xen/arm32: Follow-up XSA-93

[PATCH 0/4] xen/arm32: Follow-up XSA-93

[Julien Grall]

Hello all,

This patch series is a follow-up of XSA-93. We have disabled performance
monitors and hardware debug on ARM32 because we don't context switch
this registers. The chosen solution was to inject undefined instruction
when the guest is trying to access to this registers.

Unfortunately when I did my test, I used a tiny config file which doesn't
reflect real distribution.

The image provided by Linaro (based on Ubuntu) enable CONFIG_PERF_EVENTS on
every board. This will result to dom0 crash during the boot.

This can also happen with other distributions if they try to have a generic
configuration. Therefore it might be interesting to backport patch #2-#3
(also #1 for message clarity) on Xen 4.4. Otherwise people may not be able
to try to boot Xen with a non-modifed Linux image.

This series implement a dummy performance monitors and HW debug. Linux is
able to boot without crash and HW debug is disabled.

I've tested this series on Midway with CONFIG_PERF_EVENTS=y.

Comments and questions are welcomed.

Sincerely yours,

Julien Grall (4):
  xen/arm: Add missing newline after commit 60f7376
  xen/arm: Implement a dummy Performance Monitor for ARM32
  xen/arm: Implement a dummy debug monitor for ARM32
  xen/arm: Add some useful debug in coprocessor trapping

 xen/arch/arm/traps.c            |  129 +++++++++++++++++++++++++++++++++++++--
 xen/include/asm-arm/cpregs.h    |   31 +++++++++-
 xen/include/asm-arm/processor.h |   15 ++++-
 3 files changed, 167 insertions(+), 8 deletions(-)

-- 
1.7.10.4

[PATCH 1/4] xen/arm: Add missing newline after commit 60f7376

[Julien Grall]

Commit 60f7376 "xen/arm: Inject an undefined instruction when the coproc/sysreg
is not handled" replaced panic by gdprintk.

Unfortunately panic message string doesn't need newline, rather than gdprintk
will request one.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/traps.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index d674a15..700665c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1393,7 +1393,7 @@ static void do_cp15_32(struct cpu_user_regs *regs,
                  "%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
                  cp32.read ? "mrc" : "mcr",
                  cp32.op1, cp32.reg, cp32.crn, cp32.crm, cp32.op2, regs->pc);
-        gdprintk(XENLOG_ERR, "unhandled 32-bit CP15 access %#x",
+        gdprintk(XENLOG_ERR, "unhandled 32-bit CP15 access %#x\n",
                  hsr.bits & HSR_CP32_REGS_MASK);
 #endif
         inject_undef32_exception(regs);
@@ -1430,7 +1430,7 @@ static void do_cp15_64(struct cpu_user_regs *regs,
                      "%s p15, %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 CP15 access %#x",
+            gdprintk(XENLOG_ERR, "unhandled 64-bit CP15 access %#x\n",
                      hsr.bits & HSR_CP64_REGS_MASK);
 #endif
             inject_undef32_exception(regs);
@@ -1529,7 +1529,7 @@ static void do_sysreg(struct cpu_user_regs *regs,
                      sysreg.op2,
                      sysreg.read ? "=>" : "<=",
                      sysreg.reg, regs->pc);
-            gdprintk(XENLOG_ERR, "unhandled 64-bit sysreg access %#x",
+            gdprintk(XENLOG_ERR, "unhandled 64-bit sysreg access %#x\n",
                      hsr.bits & HSR_SYSREG_REGS_MASK);
 #endif
             inject_undef64_exception(regs, sysreg.len);
-- 
1.7.10.4

Re: [PATCH 1/4] xen/arm: Add missing newline after commit 60f7376

[Ian Campbell]

On Thu, 2014-04-24 at 23:45 +0100, Julien Grall wrote:
> Commit 60f7376 "xen/arm: Inject an undefined instruction when the coproc/sysreg
> is not handled" replaced panic by gdprintk.
> 
> Unfortunately panic message string doesn't need newline, rather than gdprintk
> will request one.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked + applied.

Re: [PATCH 1/4] xen/arm: Add missing newline after commit 60f7376

[Julien Grall]

Hi Ian,

On 05/02/2014 01:58 PM, Ian Campbell wrote:
> 
> On Thu, 2014-04-24 at 23:45 +0100, Julien Grall wrote:
>> Commit 60f7376 "xen/arm: Inject an undefined instruction when the coproc/sysreg
>> is not handled" replaced panic by gdprintk.
>>
>> Unfortunately panic message string doesn't need newline, rather than gdprintk
>> will request one.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked + applied.

Thanks. Is it possible to backport this patch for Xen 4.4?

Regards,

-- 
Julien Grall

[PATCH 2/4] xen/arm: Implement a dummy Performance Monitor for ARM32

[Julien Grall]

XSA-93 (commit 0b18220 "xen/arm: Don't let guess access to Debug and Performance
Monitor registers") disable Performance Monitor.

When CONFIG_PERF_EVENTS is enabled in the Linux Kernel, regardless the
ID_DFR0 (which tell if Perfomance Monitors Extension is implemented) the
kernel will try to access to PMCR.

Therefore we tell the guest we have 0 counters. Unfortunately we must always
support PMCCNTR (the cycle counter): we just RAZ/WI for all PM register,
which doesn't crash the kernel at least.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
    This commit is candidate for backporting on Xen 4.4. Some distribution
    (such as Linaro Ubuntu image) enable Perf Events (CONFIG_PERF_EVENTS=y).
    Linux will unconditionally access to theses registers and therefore will
    crash.
---
 xen/arch/arm/traps.c         |   28 ++++++++++++++++++++++++++++
 xen/include/asm-arm/cpregs.h |   17 ++++++++++++++++-
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 700665c..319bbe9 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1387,6 +1387,34 @@ static void do_cp15_32(struct cpu_user_regs *regs,
         if ( cp32.read )
            *r = v->arch.actlr;
         break;
+
+    /* 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.
+     *
+     * We tell the guest we have 0 counters. Unfortunately we must
+     * always support PMCCNTR (the cyle counter): we just RAZ/WI for all
+     * PM register, which doesn't crash the kernel at least
+     */
+    case HSR_CPREG32(PMCR):
+    case HSR_CPREG32(PMCNTENSET):
+    case HSR_CPREG32(PMCNTENCLR):
+    case HSR_CPREG32(PMOVSR):
+    case HSR_CPREG32(PMSWINC):
+    case HSR_CPREG32(PMSELR):
+    case HSR_CPREG32(PMCEID0):
+    case HSR_CPREG32(PMCEID1):
+    case HSR_CPREG32(PMCCNTR):
+    case HSR_CPREG32(PMXEVCNTR):
+    case HSR_CPREG32(PMXEVCNR):
+    case HSR_CPREG32(PMUSERENR):
+    case HSR_CPREG32(PMINTENSET):
+    case HSR_CPREG32(PMINTENCLR):
+    case HSR_CPREG32(PMOVSSET):
+        if ( cp32.read )
+            *r = 0;
+        break;
+
     default:
 #ifndef NDEBUG
         gdprintk(XENLOG_ERR,
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index bf8133e..f44e3b5 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -198,7 +198,22 @@
 #define TLBIMVAH        p15,4,c8,c7,1   /* Invalidate Unified Hyp. TLB by MVA */
 #define TLBIALLNSNH     p15,4,c8,c7,4   /* Invalidate Entire Non-Secure Non-Hyp. Unified TLB */
 
-/* CP15 CR9: */
+/* CP15 CR9: Performance monitors */
+#define PMCR            p15,0,c9,c12,0  /* Perf. Mon. Control Register */
+#define PMCNTENSET      p15,0,c9,c12,1  /* Perf. Mon. Count Enable Set register */
+#define PMCNTENCLR      p15,0,c9,c12,2  /* Perf. Mon. Count Enable Clear register */
+#define PMOVSR          p15,0,c9,c12,3  /* Perf. Mon. Overflow Flag Status Register */
+#define PMSWINC         p15,0,c9,c12,4  /* Perf. Mon. Software Increment register */
+#define PMSELR          p15,0,c9,c12,5  /* Perf. Mon. Event Counter Selection Register */
+#define PMCEID0         p15,0,c9,c12,6  /* Perf. Mon. Common Event Identification register 0 */
+#define PMCEID1         p15,0,c9,c12,7  /* Perf. Mon. Common Event Identification register 1 */
+#define PMCCNTR         p15,0,c9,c13,0  /* Perf. Mon. Cycle Count Register */
+#define PMXEVCNTR       p15,0,c9,c13,1  /* Perf. Mon. Event Type Select Register */
+#define PMXEVCNR        p15,0,c9,c13,2  /* Perf. Mon. Event Count Register */
+#define PMUSERENR       p15,0,c9,c14,0  /* Perf. Mon. User Enable Register */
+#define PMINTENSET      p15,0,c9,c14,1  /* Perf. Mon. Interrupt Enable Set Register */
+#define PMINTENCLR      p15,0,c9,c14,2  /* Perf. Mon. Interrupt Enable Clear Register */
+#define PMOVSSET        p15,0,c9,c14,3  /* Perf. Mon. Overflow Flag Status Set register */
 
 /* CP15 CR10: */
 #define MAIR0           p15,0,c10,c2,0  /* Memory Attribute Indirection Register 0 AKA PRRR */
-- 
1.7.10.4

Re: [PATCH 2/4] xen/arm: Implement a dummy Performance Monitor for ARM32

[Ian Campbell]

On Thu, 2014-04-24 at 23:45 +0100, Julien Grall wrote:
> XSA-93 (commit 0b18220 "xen/arm: Don't let guess access to Debug and Performance
> Monitor registers") disable Performance Monitor.
> 
> When CONFIG_PERF_EVENTS is enabled in the Linux Kernel, regardless the
> ID_DFR0 (which tell if Perfomance Monitors Extension is implemented) the
> kernel will try to access to PMCR.
> 
> Therefore we tell the guest we have 0 counters. Unfortunately we must always
> support PMCCNTR (the cycle counter): we just RAZ/WI for all PM register,
> which doesn't crash the kernel at least.

How often does this trap occur in practice? Once at start of day? Only
if you run perf? Or on every guest context switch? (obviously the last
one would be bad...)

Re: [PATCH 2/4] xen/arm: Implement a dummy Performance Monitor for ARM32

[Julien Grall]

On 05/02/2014 12:01 PM, Ian Campbell wrote:
> On Thu, 2014-04-24 at 23:45 +0100, Julien Grall wrote:
>> XSA-93 (commit 0b18220 "xen/arm: Don't let guess access to Debug and Performance
>> Monitor registers") disable Performance Monitor.
>>
>> When CONFIG_PERF_EVENTS is enabled in the Linux Kernel, regardless the
>> ID_DFR0 (which tell if Perfomance Monitors Extension is implemented) the
>> kernel will try to access to PMCR.
>>
>> Therefore we tell the guest we have 0 counters. Unfortunately we must always
>> support PMCCNTR (the cycle counter): we just RAZ/WI for all PM register,
>> which doesn't crash the kernel at least.
> 
> How often does this trap occur in practice? Once at start of day? Only
> if you run perf? Or on every guest context switch? (obviously the last
> one would be bad...)

There is few calls to the perf registers during the boot (when Linux is
compiled with CONFIG_PERF_EVENTS=y).

I didn't see any usage during guest context switch. I haven't try perf.

Regards,

-- 
Julien Grall

Re: [PATCH 2/4] xen/arm: Implement a dummy Performance Monitor for ARM32

[Ian Campbell]

On Fri, 2014-05-02 at 13:41 +0100, Julien Grall wrote:
> On 05/02/2014 12:01 PM, Ian Campbell wrote:
> > On Thu, 2014-04-24 at 23:45 +0100, Julien Grall wrote:
> >> XSA-93 (commit 0b18220 "xen/arm: Don't let guess access to Debug and Performance
> >> Monitor registers") disable Performance Monitor.
> >>
> >> When CONFIG_PERF_EVENTS is enabled in the Linux Kernel, regardless the
> >> ID_DFR0 (which tell if Perfomance Monitors Extension is implemented) the
> >> kernel will try to access to PMCR.
> >>
> >> Therefore we tell the guest we have 0 counters. Unfortunately we must always
> >> support PMCCNTR (the cycle counter): we just RAZ/WI for all PM register,
> >> which doesn't crash the kernel at least.
> > 
> > How often does this trap occur in practice? Once at start of day? Only
> > if you run perf? Or on every guest context switch? (obviously the last
> > one would be bad...)
> 
> There is few calls to the perf registers during the boot (when Linux is
> compiled with CONFIG_PERF_EVENTS=y).
> 
> I didn't see any usage during guest context switch. I haven't try perf.

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

[PATCH 3/4] xen/arm: Implement a dummy debug monitor for ARM32

[Julien Grall]

XSA-93 (commit 0b18220 "xen/arm: Don't let guess access to Debug and Performance
Monitors registers") disable Debug Registers access.

When CONFIG_PERF_EVENTS is enabled in the Linux Kernel, it will try to
initialize the debug monitors. If an error occured Linux won't use this
feature.

The implementation made Xen expose a minimal set of registers which let think
the guest (i.e.) thinks HW debug won't work.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    This commit is candidate for backporting on Xen 4.4. Distribution may
    enable HW DEBUG (e.g. Linaro Ubuntu image), therefore Linux will try
    to initialize it.
---
 xen/arch/arm/traps.c         |   77 ++++++++++++++++++++++++++++++++++++++++--
 xen/include/asm-arm/cpregs.h |   14 ++++++++
 2 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 319bbe9..1f61e6e 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1468,7 +1468,76 @@ static void do_cp15_64(struct cpu_user_regs *regs,
     advance_pc(regs, hsr);
 }
 
-static void do_cp14(struct cpu_user_regs *regs, union hsr hsr)
+static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
+{
+    struct hsr_cp32 cp32 = hsr.cp32;
+    uint32_t *r = (uint32_t *)select_user_reg(regs, cp32.reg);
+    struct domain *d = current->domain;
+
+    if ( !check_conditional_instr(regs, hsr) )
+    {
+        advance_pc(regs, hsr);
+        return;
+    }
+
+    switch ( hsr.bits & HSR_CP32_REGS_MASK )
+    {
+    case HSR_CPREG32(DBGDIDR):
+
+        /* Read-only register */
+        if ( !cp32.read )
+            goto bad_cp;
+
+        /* Implement the minimum requirements:
+         *  - Number of watchpoints: 1
+         *  - Number of breakpoints: 2
+         *  - Version: ARMv7 v7.1
+         *  - Variant and Revision bits match MDIR
+         */
+        *r = (1 << 24) | (5 << 16);
+        *r |= ((d->arch.vpidr >> 20) & 0xf) | (d->arch.vpidr & 0xf);
+        break;
+
+    case HSR_CPREG32(DBGDSCRINT):
+    case HSR_CPREG32(DBGDSCREXT):
+        /* 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;
+    case HSR_CPREG32(DBGVCR):
+    case HSR_CPREG32(DBGOSLAR):
+    case HSR_CPREG32(DBGBVR0):
+    case HSR_CPREG32(DBGCR0):
+    case HSR_CPREG32(DBGWVR0):
+    case HSR_CPREG32(DBGWCR0):
+    case HSR_CPREG32(DBGBVR1):
+    case HSR_CPREG32(DBGCR1):
+    case HSR_CPREG32(DBGOSDLR):
+        /* RAZ/WI */
+        if ( cp32.read )
+            *r = 0;
+        break;
+
+    default:
+bad_cp:
+#ifndef NDEBUG
+        gdprintk(XENLOG_ERR,
+                 "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
+                  cp32.read ? "mrc" : "mcr",
+                  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);
+#endif
+        inject_undef32_exception(regs);
+        return;
+    }
+
+    advance_pc(regs, hsr);
+}
+
+static void do_cp14_dbg(struct cpu_user_regs *regs, union hsr hsr)
 {
     if ( !check_conditional_instr(regs, hsr) )
     {
@@ -1720,10 +1789,14 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
         do_cp15_64(regs, hsr);
         break;
     case HSR_EC_CP14_32:
+        if ( !is_32bit_domain(current->domain) )
+            goto bad_trap;
+        do_cp14_32(regs, hsr);
+        break;
     case HSR_EC_CP14_DBG:
         if ( !is_32bit_domain(current->domain) )
             goto bad_trap;
-        do_cp14(regs, hsr);
+        do_cp14_dbg(regs, hsr);
         break;
     case HSR_EC_CP:
         if ( !is_32bit_domain(current->domain) )
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index f44e3b5..306e506 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -71,6 +71,20 @@
 
 /* Coprocessor 14 */
 
+/* CP14 0: Debug Register interface */
+#define DBGDIDR         p14,0,c0,c0,0   /* Debug ID Register */
+#define DBGDSCRINT      p14,0,c0,c1,0   /* Debug Status and Control Internal */
+#define DBGDSCREXT      p14,0,c0,c2,2   /* Debug Status and Control External */
+#define DBGVCR          p14,0,c0,c7,0   /* Vector Catch */
+#define DBGBVR0         p14,0,c0,c0,4   /* Breakpoint Value 0 */
+#define DBGCR0          p14,0,c0,c0,5   /* Breakpoint Control 0 */
+#define DBGWVR0         p14,0,c0,c0,6   /* Watchpoint Value 0 */
+#define DBGWCR0         p14,0,c0,c0,7   /* Watchpoint Control 0 */
+#define DBGBVR1         p14,0,c0,c1,4   /* Breakpoint Value 1 */
+#define DBGCR1          p14,0,c0,c1,5   /* Breakpoint Control 1 */
+#define DBGOSLAR        p14,0,c1,c0,4   /* OS Lock Access */
+#define DBGOSDLR        p14,0,c1,c3,4   /* OS Double Lock */
+
 /* CP14 CR0: */
 #define TEECR           p14,6,c0,c0,0   /* ThumbEE Configuration Register */
 
-- 
1.7.10.4

Re: [PATCH 3/4] xen/arm: Implement a dummy debug monitor for ARM32

[Ian Campbell]

On Thu, 2014-04-24 at 23:45 +0100, Julien Grall wrote:
> XSA-93 (commit 0b18220 "xen/arm: Don't let guess access to Debug and Performance
> Monitors registers") disable Debug Registers access.
> 
> When CONFIG_PERF_EVENTS is enabled in the Linux Kernel, it will try to
> initialize the debug monitors. If an error occured Linux won't use this
> feature.
> 
> The implementation made Xen expose a minimal set of registers which let think
> the guest (i.e.) thinks HW debug won't work.

Why only for arm32?

I think arm64 makes more use than arm32 (unconditionally touches
MDSCR_EL1 on the ctx switch path).

I think we should be considering allow the guest to access these and
context switching them instead.

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     This commit is candidate for backporting on Xen 4.4. Distribution may
>     enable HW DEBUG (e.g. Linaro Ubuntu image), therefore Linux will try
>     to initialize it.
> ---
>  xen/arch/arm/traps.c         |   77 ++++++++++++++++++++++++++++++++++++++++--
>  xen/include/asm-arm/cpregs.h |   14 ++++++++
>  2 files changed, 89 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 319bbe9..1f61e6e 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1468,7 +1468,76 @@ static void do_cp15_64(struct cpu_user_regs *regs,
>      advance_pc(regs, hsr);
>  }
>  
> -static void do_cp14(struct cpu_user_regs *regs, union hsr hsr)
> +static void do_cp14_32(struct cpu_user_regs *regs, union hsr hsr)
> +{
> +    struct hsr_cp32 cp32 = hsr.cp32;
> +    uint32_t *r = (uint32_t *)select_user_reg(regs, cp32.reg);
> +    struct domain *d = current->domain;
> +
> +    if ( !check_conditional_instr(regs, hsr) )
> +    {
> +        advance_pc(regs, hsr);
> +        return;
> +    }
> +
> +    switch ( hsr.bits & HSR_CP32_REGS_MASK )
> +    {
> +    case HSR_CPREG32(DBGDIDR):
> +
> +        /* Read-only register */
> +        if ( !cp32.read )
> +            goto bad_cp;
> +
> +        /* Implement the minimum requirements:
> +         *  - Number of watchpoints: 1
> +         *  - Number of breakpoints: 2
> +         *  - Version: ARMv7 v7.1
> +         *  - Variant and Revision bits match MDIR
> +         */
> +        *r = (1 << 24) | (5 << 16);
> +        *r |= ((d->arch.vpidr >> 20) & 0xf) | (d->arch.vpidr & 0xf);
> +        break;
> +
> +    case HSR_CPREG32(DBGDSCRINT):
> +    case HSR_CPREG32(DBGDSCREXT):
> +        /* 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;
> +    case HSR_CPREG32(DBGVCR):
> +    case HSR_CPREG32(DBGOSLAR):
> +    case HSR_CPREG32(DBGBVR0):
> +    case HSR_CPREG32(DBGCR0):
> +    case HSR_CPREG32(DBGWVR0):
> +    case HSR_CPREG32(DBGWCR0):
> +    case HSR_CPREG32(DBGBVR1):
> +    case HSR_CPREG32(DBGCR1):
> +    case HSR_CPREG32(DBGOSDLR):
> +        /* RAZ/WI */
> +        if ( cp32.read )
> +            *r = 0;
> +        break;
> +
> +    default:
> +bad_cp:
> +#ifndef NDEBUG
> +        gdprintk(XENLOG_ERR,
> +                 "%s p14, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n",
> +                  cp32.read ? "mrc" : "mcr",
> +                  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);
> +#endif
> +        inject_undef32_exception(regs);
> +        return;
> +    }
> +
> +    advance_pc(regs, hsr);
> +}
> +
> +static void do_cp14_dbg(struct cpu_user_regs *regs, union hsr hsr)
>  {
>      if ( !check_conditional_instr(regs, hsr) )
>      {
> @@ -1720,10 +1789,14 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
>          do_cp15_64(regs, hsr);
>          break;
>      case HSR_EC_CP14_32:
> +        if ( !is_32bit_domain(current->domain) )
> +            goto bad_trap;
> +        do_cp14_32(regs, hsr);
> +        break;
>      case HSR_EC_CP14_DBG:
>          if ( !is_32bit_domain(current->domain) )
>              goto bad_trap;
> -        do_cp14(regs, hsr);
> +        do_cp14_dbg(regs, hsr);
>          break;
>      case HSR_EC_CP:
>          if ( !is_32bit_domain(current->domain) )
> diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
> index f44e3b5..306e506 100644
> --- a/xen/include/asm-arm/cpregs.h
> +++ b/xen/include/asm-arm/cpregs.h
> @@ -71,6 +71,20 @@
>  
>  /* Coprocessor 14 */
>  
> +/* CP14 0: Debug Register interface */
> +#define DBGDIDR         p14,0,c0,c0,0   /* Debug ID Register */
> +#define DBGDSCRINT      p14,0,c0,c1,0   /* Debug Status and Control Internal */
> +#define DBGDSCREXT      p14,0,c0,c2,2   /* Debug Status and Control External */
> +#define DBGVCR          p14,0,c0,c7,0   /* Vector Catch */
> +#define DBGBVR0         p14,0,c0,c0,4   /* Breakpoint Value 0 */
> +#define DBGCR0          p14,0,c0,c0,5   /* Breakpoint Control 0 */
> +#define DBGWVR0         p14,0,c0,c0,6   /* Watchpoint Value 0 */
> +#define DBGWCR0         p14,0,c0,c0,7   /* Watchpoint Control 0 */
> +#define DBGBVR1         p14,0,c0,c1,4   /* Breakpoint Value 1 */
> +#define DBGCR1          p14,0,c0,c1,5   /* Breakpoint Control 1 */
> +#define DBGOSLAR        p14,0,c1,c0,4   /* OS Lock Access */
> +#define DBGOSDLR        p14,0,c1,c3,4   /* OS Double Lock */
> +
>  /* CP14 CR0: */
>  #define TEECR           p14,6,c0,c0,0   /* ThumbEE Configuration Register */
>  

Re: [PATCH 3/4] xen/arm: Implement a dummy debug monitor for ARM32

[Julien Grall]

On 05/02/2014 12:09 PM, Ian Campbell wrote:
> On Thu, 2014-04-24 at 23:45 +0100, Julien Grall wrote:
>> XSA-93 (commit 0b18220 "xen/arm: Don't let guess access to Debug and Performance
>> Monitors registers") disable Debug Registers access.
>>
>> When CONFIG_PERF_EVENTS is enabled in the Linux Kernel, it will try to
>> initialize the debug monitors. If an error occured Linux won't use this
>> feature.
>>
>> The implementation made Xen expose a minimal set of registers which let think
>> the guest (i.e.) thinks HW debug won't work.
> 
> Why only for arm32?

Because, if I'm not mistaken, you've already implemented a dummy HW
debug for arm64 in commit 0b182202 "xen/arm: Don't let guess access to
Debug and Performance Monitor registers".

> I think arm64 makes more use than arm32 (unconditionally touches
> MDSCR_EL1 on the ctx switch path).
> 
> I think we should be considering allow the guest to access these and
> context switching them instead.

Disabling HW breakpoint don't disable debug. Linux will only use
software breakpoing (which is of course a bit slower).

I wrote this series to allow Distribution kernel (such as Linaro Ubuntu
kernel) boots correctly on Xen 4.4 and onwards.

I don't plan to more spend time to write a correct emulation (i.e
context switching) to support HW debug.

Regards,

-- 
Julien Grall

Re: [PATCH 3/4] xen/arm: Implement a dummy debug monitor for ARM32

[Ian Campbell]

On Fri, 2014-05-02 at 13:53 +0100, Julien Grall wrote:
> On 05/02/2014 12:09 PM, Ian Campbell wrote:
> > On Thu, 2014-04-24 at 23:45 +0100, Julien Grall wrote:
> >> XSA-93 (commit 0b18220 "xen/arm: Don't let guess access to Debug and Performance
> >> Monitors registers") disable Debug Registers access.
> >>
> >> When CONFIG_PERF_EVENTS is enabled in the Linux Kernel, it will try to
> >> initialize the debug monitors. If an error occured Linux won't use this
> >> feature.
> >>
> >> The implementation made Xen expose a minimal set of registers which let think
> >> the guest (i.e.) thinks HW debug won't work.
> > 
> > Why only for arm32?
> 
> Because, if I'm not mistaken, you've already implemented a dummy HW
> debug for arm64 in commit 0b182202 "xen/arm: Don't let guess access to
> Debug and Performance Monitor registers".

That's a RAZ/WI thing, I thought this was something cleverer (returing
values to make the guest think nothing was there).

> 
> > I think arm64 makes more use than arm32 (unconditionally touches
> > MDSCR_EL1 on the ctx switch path).
> > 
> > I think we should be considering allow the guest to access these and
> > context switching them instead.
> 
> Disabling HW breakpoint don't disable debug. Linux will only use
> software breakpoing (which is of course a bit slower).
> 
> I wrote this series to allow Distribution kernel (such as Linaro Ubuntu
> kernel) boots correctly on Xen 4.4 and onwards.
> 
> I don't plan to more spend time to write a correct emulation (i.e
> context switching) to support HW debug.
> 
> Regards,
> 

Re: [PATCH 3/4] xen/arm: Implement a dummy debug monitor for ARM32

[Julien Grall]

On 05/02/2014 02:14 PM, Ian Campbell wrote:
> On Fri, 2014-05-02 at 13:53 +0100, Julien Grall wrote:
>> On 05/02/2014 12:09 PM, Ian Campbell wrote:
>>> On Thu, 2014-04-24 at 23:45 +0100, Julien Grall wrote:
>>>> XSA-93 (commit 0b18220 "xen/arm: Don't let guess access to Debug and Performance
>>>> Monitors registers") disable Debug Registers access.
>>>>
>>>> When CONFIG_PERF_EVENTS is enabled in the Linux Kernel, it will try to
>>>> initialize the debug monitors. If an error occured Linux won't use this
>>>> feature.
>>>>
>>>> The implementation made Xen expose a minimal set of registers which let think
>>>> the guest (i.e.) thinks HW debug won't work.
>>>
>>> Why only for arm32?
>>
>> Because, if I'm not mistaken, you've already implemented a dummy HW
>> debug for arm64 in commit 0b182202 "xen/arm: Don't let guess access to
>> Debug and Performance Monitor registers".
> 
> That's a RAZ/WI thing, I thought this was something cleverer (returing
> values to make the guest think nothing was there).

Most of the time RAZ/WI is enough.  The arm32 implementation makes the
life harder.

I didn't check the arm64 implementation as I don't use it every day... I
need to set up the Foundation Model on my computer.

-- 
Julien Grall

Re: [PATCH 3/4] xen/arm: Implement a dummy debug monitor for ARM32

[Ian Campbell]

On Fri, 2014-05-02 at 14:29 +0100, Julien Grall wrote:
> On 05/02/2014 02:14 PM, Ian Campbell wrote:
> > On Fri, 2014-05-02 at 13:53 +0100, Julien Grall wrote:
> >> On 05/02/2014 12:09 PM, Ian Campbell wrote:
> >>> On Thu, 2014-04-24 at 23:45 +0100, Julien Grall wrote:
> >>>> XSA-93 (commit 0b18220 "xen/arm: Don't let guess access to Debug and Performance
> >>>> Monitors registers") disable Debug Registers access.
> >>>>
> >>>> When CONFIG_PERF_EVENTS is enabled in the Linux Kernel, it will try to
> >>>> initialize the debug monitors. If an error occured Linux won't use this
> >>>> feature.
> >>>>
> >>>> The implementation made Xen expose a minimal set of registers which let think
> >>>> the guest (i.e.) thinks HW debug won't work.
> >>>
> >>> Why only for arm32?
> >>
> >> Because, if I'm not mistaken, you've already implemented a dummy HW
> >> debug for arm64 in commit 0b182202 "xen/arm: Don't let guess access to
> >> Debug and Performance Monitor registers".
> > 
> > That's a RAZ/WI thing, I thought this was something cleverer (returing
> > values to make the guest think nothing was there).

I think we've got a bit sidetracked here (especially in the other
subthread), sorry about that.

> Most of the time RAZ/WI is enough.  The arm32 implementation makes the
> life harder.

Looking at it with fresh eyes this morning I see what you mean now, the
essential difference is that with arm32 DBGDIDR is trapped by
MDCR_EL2.TDA, whereas the the arm64 equivalent (IDAA64DFR*) are not
(it's trapped as part of the ID register group, which we don't bother
trapping). The rest of the registers are RAZ/WI which is consistent with
arm64.

So in the end you've convinced me that this is the right thing to do for
now and to backport to 4.4.

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

I've committed this and the previous (perfc) one but not the next one
(useful debug for coproc traps) which had comments.

I did s/DBGCR/DBGBCR/ to match the name used in both the v7 and v8 ARM
ARMs, I think it was just a typo? Hope that's ok.

Also DBGOSLAR is supposed to be WO but you've implemented it as RAZ/WI,
I didn't think that mattered enough to bother with though.

Sorry that it took so long to get my head around this.

> I didn't check the arm64 implementation as I don't use it every day... I
> need to set up the Foundation Model on my computer.

You made me notice that arm64 doesn't handle OSDLR_EL1 traps. I think
that's harmless (unless a guest kernel tries to use it) but I'll send
out a patch shortly.

Ian.

Re: [PATCH 3/4] xen/arm: Implement a dummy debug monitor for ARM32

[Julien Grall]

Hi Ian,

On 13/06/14 13:02, Ian Campbell wrote:
> Looking at it with fresh eyes this morning I see what you mean now, the
> essential difference is that with arm32 DBGDIDR is trapped by
> MDCR_EL2.TDA, whereas the the arm64 equivalent (IDAA64DFR*) are not
> (it's trapped as part of the ID register group, which we don't bother
> trapping). The rest of the registers are RAZ/WI which is consistent with
> arm64.

Right, this patch is mostly here to let Linux think there is not debug 
hardware registers.

As you said on another mail, the long term goal is to context switch 
correctly those register to allow perf and debug working in the guest.

> So in the end you've convinced me that this is the right thing to do for
> now and to backport to 4.4.
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> I've committed this and the previous (perfc) one but not the next one
> (useful debug for coproc traps) which had comments.
>
> I did s/DBGCR/DBGBCR/ to match the name used in both the v7 and v8 ARM
> ARMs, I think it was just a typo? Hope that's ok.

Yes, I forgot the B by mistake.

>
> Also DBGOSLAR is supposed to be WO but you've implemented it as RAZ/WI,
> I didn't think that mattered enough to bother with though.

I was lazy to add 2 more lines to handle this register WO.

I'm not sure what is behavior when a guest is trying to read a WO 
register. I guess an undefined instruction.

I can send a follow-up to use this behavior for Xen 4.5.

Regards,

-- 
Julien Grall

Re: [PATCH 3/4] xen/arm: Implement a dummy debug monitor for ARM32

[Ian Campbell]

On Fri, 2014-05-02 at 13:53 +0100, Julien Grall wrote:
> I don't plan to more spend time to write a correct emulation (i.e
> context switching) to support HW debug.

I'm not going to ack a patch which causes arm32 to diverge from arm64 in
this area, especially not when the correct solution (more critical on
arm64 than arm32) is to properly context switch these registers.

Ian.

Re: [PATCH 3/4] xen/arm: Implement a dummy debug monitor for ARM32

[Julien Grall]

On 05/02/2014 02:26 PM, Ian Campbell wrote:
> On Fri, 2014-05-02 at 13:53 +0100, Julien Grall wrote:
>> I don't plan to more spend time to write a correct emulation (i.e
>> context switching) to support HW debug.
> 
> I'm not going to ack a patch which causes arm32 to diverge from arm64 in
> this area, especially not when the correct solution (more critical on
> arm64 than arm32) is to properly context switch these registers.

We don't diverge... The Linux HW debug arm32 implementation doesn't
permit to use RAZ/WI on some registers.

Currently arm64 HW debug may or may not work but it won't crash the
guest. It's not the case on arm32. So the current Xen already diverge.

As said earlier, the HW debug is not essential. Writing a proper
emulation will take some time and I don't have time for writing and
testing it correctly.

IHMO, this solution is perfect for Xen 4.4, otherwise Xen 4.4.1 will
breaks support with lots of distribution.

For Xen 4.5, it's intermediate solution to allow guest working correctly
and people playing their shiny distribution on top of Xen. When someone
will care about HW debug, then we will have to support it correctly.

Regards,

-- 
Julien Grall

Re: [PATCH 3/4] xen/arm: Implement a dummy debug monitor for ARM32

[Ian Campbell]

On Fri, 2014-05-02 at 14:39 +0100, Julien Grall wrote:
> On 05/02/2014 02:26 PM, Ian Campbell wrote:
> > On Fri, 2014-05-02 at 13:53 +0100, Julien Grall wrote:
> >> I don't plan to more spend time to write a correct emulation (i.e
> >> context switching) to support HW debug.
> > 
> > I'm not going to ack a patch which causes arm32 to diverge from arm64 in
> > this area, especially not when the correct solution (more critical on
> > arm64 than arm32) is to properly context switch these registers.
> 
> We don't diverge... The Linux HW debug arm32 implementation doesn't
> permit to use RAZ/WI on some registers.
> 
> Currently arm64 HW debug may or may not work but it won't crash the
> guest. It's not the case on arm32. So the current Xen already diverge.

I think that was a mistake (albeit made under the pressure of a security
embargo), we shouldn't diverge further.

> As said earlier, the HW debug is not essential. Writing a proper
> emulation will take some time and I don't have time for writing and
> testing it correctly.

This is not about writing any sort of emulation AFAICT. It is about
context switching a couple of dozen new registers, of which 80% are
multiple instances of the same type of register.

The proper solution won't involve any trapping at all. (Maybe we will do
lazy context switching at some point, but that's another thing).

Ian.

> 
> IHMO, this solution is perfect for Xen 4.4, otherwise Xen 4.4.1 will
> breaks support with lots of distribution.
> 
> For Xen 4.5, it's intermediate solution to allow guest working correctly
> and people playing their shiny distribution on top of Xen. When someone
> will care about HW debug, then we will have to support it correctly.
> 
> Regards,
> 

Re: [PATCH 3/4] xen/arm: Implement a dummy debug monitor for ARM32

[Julien Grall]

On 05/02/2014 03:18 PM, Ian Campbell wrote:
> On Fri, 2014-05-02 at 14:39 +0100, Julien Grall wrote:
>> On 05/02/2014 02:26 PM, Ian Campbell wrote:
>>> On Fri, 2014-05-02 at 13:53 +0100, Julien Grall wrote:
>>>> I don't plan to more spend time to write a correct emulation (i.e
>>>> context switching) to support HW debug.
>>>
>>> I'm not going to ack a patch which causes arm32 to diverge from arm64 in
>>> this area, especially not when the correct solution (more critical on
>>> arm64 than arm32) is to properly context switch these registers.
>>
>> We don't diverge... The Linux HW debug arm32 implementation doesn't
>> permit to use RAZ/WI on some registers.
>>
>> Currently arm64 HW debug may or may not work but it won't crash the
>> guest. It's not the case on arm32. So the current Xen already diverge.
> 
> I think that was a mistake (albeit made under the pressure of a security
> embargo), we shouldn't diverge further.
> 
>> As said earlier, the HW debug is not essential. Writing a proper
>> emulation will take some time and I don't have time for writing and
>> testing it correctly.
> 
> This is not about writing any sort of emulation AFAICT. It is about
> context switching a couple of dozen new registers, of which 80% are
> multiple instances of the same type of register.
> 
> The proper solution won't involve any trapping at all. (Maybe we will do
> lazy context switching at some point, but that's another thing).

Are we sure that context switching won't lead to another security issue?
It's not clear to me how debugging behave with virtualization.

-- 
Julien Grall

Re: [PATCH 3/4] xen/arm: Implement a dummy debug monitor for ARM32

[Ian Campbell]

On Fri, 2014-05-02 at 15:22 +0100, Julien Grall wrote:
> On 05/02/2014 03:18 PM, Ian Campbell wrote:
> > On Fri, 2014-05-02 at 14:39 +0100, Julien Grall wrote:
> >> On 05/02/2014 02:26 PM, Ian Campbell wrote:
> >>> On Fri, 2014-05-02 at 13:53 +0100, Julien Grall wrote:
> >>>> I don't plan to more spend time to write a correct emulation (i.e
> >>>> context switching) to support HW debug.
> >>>
> >>> I'm not going to ack a patch which causes arm32 to diverge from arm64 in
> >>> this area, especially not when the correct solution (more critical on
> >>> arm64 than arm32) is to properly context switch these registers.
> >>
> >> We don't diverge... The Linux HW debug arm32 implementation doesn't
> >> permit to use RAZ/WI on some registers.
> >>
> >> Currently arm64 HW debug may or may not work but it won't crash the
> >> guest. It's not the case on arm32. So the current Xen already diverge.
> > 
> > I think that was a mistake (albeit made under the pressure of a security
> > embargo), we shouldn't diverge further.
> > 
> >> As said earlier, the HW debug is not essential. Writing a proper
> >> emulation will take some time and I don't have time for writing and
> >> testing it correctly.
> > 
> > This is not about writing any sort of emulation AFAICT. It is about
> > context switching a couple of dozen new registers, of which 80% are
> > multiple instances of the same type of register.
> > 
> > The proper solution won't involve any trapping at all. (Maybe we will do
> > lazy context switching at some point, but that's another thing).
> 
> Are we sure that context switching won't lead to another security issue?

Not if we do it right ;-)

> It's not clear to me how debugging behave with virtualization.

Me neither, yet.

Ian.

[PATCH 4/4] xen/arm: Add some useful debug in coprocessor trapping

[Julien Grall]

XSA-93 adds a couple of new functions to trap coprocessor registers. They
unconditonally inject an undefined instruction to guest.

When debugging an OS at early stage, it may be hard to know why the guest
received an UNDEFINED. Add some debug message to help the developper when Xen
is built in debug mode.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/traps.c            |   18 ++++++++++++++++++
 xen/include/asm-arm/processor.h |   15 +++++++++++++--
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 1f61e6e..c04f53f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1539,23 +1539,41 @@ bad_cp:
 
 static void do_cp14_dbg(struct cpu_user_regs *regs, union hsr hsr)
 {
+    struct hsr_cp64 cp64 = hsr.cp64;
+
     if ( !check_conditional_instr(regs, hsr) )
     {
         advance_pc(regs, hsr);
         return;
     }
 
+#ifndef NDEBUG
+    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);
+#endif
     inject_undef32_exception(regs);
 }
 
 static void do_cp(struct cpu_user_regs *regs, union hsr hsr)
 {
+#ifndef NDEBUG
+    struct hsr_cp cp = hsr.cp;
+#endif
+
     if ( !check_conditional_instr(regs, hsr) )
     {
         advance_pc(regs, hsr);
         return;
     }
 
+#ifndef NDEBUG
+    ASSERT(!cp.tas); /* We don't trap SIMD instruction */
+    gdprintk(XENLOG_ERR, "unhandled CP%d access\n", cp.coproc);
+#endif
     inject_undef32_exception(regs);
 }
 
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 9267c1b..bc29de1 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -289,12 +289,23 @@ union hsr {
         unsigned long reg2:5;   /* Rt2 */
         unsigned long sbzp2:1;
         unsigned long op1:4;    /* Op1 */
-        unsigned long cc:4;     /* Condition Code */
-        unsigned long ccvalid:1;/* CC Valid */
+        unsigned long cc:4;     /* condition code */
+        unsigned long ccvalid:1;/* cc valid */
         unsigned long len:1;    /* Instruction length */
         unsigned long ec:6;     /* Exception Class */
     } cp64; /* HSR_EC_CP15_64, HSR_EC_CP14_64 */
 
+    struct hsr_cp {
+        unsigned long coproc:4; /* Number of coproc accessed */
+        unsigned long sbz0p:1;
+        unsigned long tas:1;    /* Trapped Advanced SIMD */
+        unsigned long res0:14;
+        unsigned long cc:4;     /* condition code */
+        unsigned long ccvalid:1;/* cc valid */
+        unsigned long len:1;    /* Instruction length */
+        unsigned long ec:6;     /* Exception Class */
+    } cp; /* HSR_EC_CP */
+
 #ifdef CONFIG_ARM_64
     struct hsr_sysreg {
         unsigned long read:1;   /* Direction */
-- 
1.7.10.4

Re: [PATCH 4/4] xen/arm: Add some useful debug in coprocessor trapping

[Ian Campbell]

On Thu, 2014-04-24 at 23:45 +0100, Julien Grall wrote:
> XSA-93 adds a couple of new functions to trap coprocessor registers. They
> unconditonally inject an undefined instruction to guest.

"unconditionally"

> When debugging an OS at early stage, it may be hard to know why the guest
> received an UNDEFINED. Add some debug message to help the developper when Xen

"developer"

> is built in debug mode.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/traps.c            |   18 ++++++++++++++++++
>  xen/include/asm-arm/processor.h |   15 +++++++++++++--
>  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 1f61e6e..c04f53f 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1539,23 +1539,41 @@ bad_cp:
>  
>  static void do_cp14_dbg(struct cpu_user_regs *regs, union hsr hsr)
>  {
> +    struct hsr_cp64 cp64 = hsr.cp64;

Won't this be unused in debug=n builds and therefore not build?

> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 9267c1b..bc29de1 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -289,12 +289,23 @@ union hsr {
>          unsigned long reg2:5;   /* Rt2 */
>          unsigned long sbzp2:1;
>          unsigned long op1:4;    /* Op1 */
> -        unsigned long cc:4;     /* Condition Code */
> -        unsigned long ccvalid:1;/* CC Valid */
> +        unsigned long cc:4;     /* condition code */
> +        unsigned long ccvalid:1;/* cc valid */

This seems a bit gratuitous, especially given it appears 3 times and you
only change one. I'd prefer if you just made the new version match the
existing ones than change everything.

>          unsigned long len:1;    /* Instruction length */
>          unsigned long ec:6;     /* Exception Class */
>      } cp64; /* HSR_EC_CP15_64, HSR_EC_CP14_64 */
>  
> +    struct hsr_cp {
> +        unsigned long coproc:4; /* Number of coproc accessed */
> +        unsigned long sbz0p:1;
> +        unsigned long tas:1;    /* Trapped Advanced SIMD */
> +        unsigned long res0:14;
> +        unsigned long cc:4;     /* condition code */
> +        unsigned long ccvalid:1;/* cc valid */
> +        unsigned long len:1;    /* Instruction length */
> +        unsigned long ec:6;     /* Exception Class */
> +    } cp; /* HSR_EC_CP */
> +
>  #ifdef CONFIG_ARM_64
>      struct hsr_sysreg {
>          unsigned long read:1;   /* Direction */

Re: [PATCH 4/4] xen/arm: Add some useful debug in coprocessor trapping

[Julien Grall]

On 05/02/2014 12:12 PM, Ian Campbell wrote:
> On Thu, 2014-04-24 at 23:45 +0100, Julien Grall wrote:
>> XSA-93 adds a couple of new functions to trap coprocessor registers. They
>> unconditonally inject an undefined instruction to guest.
> 
> "unconditionally"
> 
>> When debugging an OS at early stage, it may be hard to know why the guest
>> received an UNDEFINED. Add some debug message to help the developper when Xen
> 
> "developer"

I will fix both typo on the next version.

>> is built in debug mode.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/traps.c            |   18 ++++++++++++++++++
>>  xen/include/asm-arm/processor.h |   15 +++++++++++++--
>>  2 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 1f61e6e..c04f53f 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1539,23 +1539,41 @@ bad_cp:
>>  
>>  static void do_cp14_dbg(struct cpu_user_regs *regs, union hsr hsr)
>>  {
>> +    struct hsr_cp64 cp64 = hsr.cp64;
> 
> Won't this be unused in debug=n builds and therefore not build?

Right. I will add #ifndef NDEBUG

>> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
>> index 9267c1b..bc29de1 100644
>> --- a/xen/include/asm-arm/processor.h
>> +++ b/xen/include/asm-arm/processor.h
>> @@ -289,12 +289,23 @@ union hsr {
>>          unsigned long reg2:5;   /* Rt2 */
>>          unsigned long sbzp2:1;
>>          unsigned long op1:4;    /* Op1 */
>> -        unsigned long cc:4;     /* Condition Code */
>> -        unsigned long ccvalid:1;/* CC Valid */
>> +        unsigned long cc:4;     /* condition code */
>> +        unsigned long ccvalid:1;/* cc valid */
> 
> This seems a bit gratuitous, especially given it appears 3 times and you
> only change one. I'd prefer if you just made the new version match the
> existing ones than change everything.

This change has been added by mistake. I will remove it in next version.

Regards,

-- 
Julien Grall