Re: [PATCH v2] xen: arm64: more useful logging on bad trap.

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/02/15 17:01, Ian Campbell wrote:
> Dump the register state before panicing so we have some clue where the
> issue occurred. Also decode the ESR register a bit to save having to
> grab a pen and paper.
>
> ESR_EL2 is a 32-bit register, so use SYSREG_READ32 not ..._READ64, as
> we already do correctly in the main trap handler.
>
> While here notice that do_trap_serror is never called and remove it.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Reviewed-by: Julien Grall <julien.grall@linaro.org>
> Tested-by: Jintack Lim <jintack@cs.columbia.edu>
> Cc: jintack@cs.columbia.edu
> ---
> Jintack, since you have a system which is exhibiting SError issues I
> wonder if I could prevail on you to give this patch a try on your
> system and report on the output. I've only compile tested this myself.
>
> v2: Added blank line after variable declaration
>     Split log message into two lines.
>     s/code/ESR/ and reformat a little.
> ---
>  xen/arch/arm/arm64/traps.c |   14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c
> index 1693b5d..31a3ca5 100644
> --- a/xen/arch/arm/arm64/traps.c
> +++ b/xen/arch/arm/arm64/traps.c
> @@ -24,11 +24,6 @@
>  
>  #include <public/xen.h>
>  
> -asmlinkage void do_trap_serror(struct cpu_user_regs *regs)
> -{
> -    panic("Unhandled serror trap");
> -}
> -
>  static const char *handler[]= {
>          "Synchronous Abort",
>          "IRQ",
> @@ -38,11 +33,14 @@ static const char *handler[]= {
>  
>  asmlinkage void do_bad_mode(struct cpu_user_regs *regs, int reason)
>  {
> -    uint64_t esr = READ_SYSREG64(ESR_EL2);
> -    printk("Bad mode in %s handler detected, code 0x%08"PRIx64"\n",
> -           handler[reason], esr);
> +    union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };
> +
> +    printk("Bad mode in %s handler detected", handler[reason]);
> +    printk("ESR=0x%08"PRIx32":  EC=%"PRIx32", IL=%"PRIx32", ISS=%"PRIx32"\n",
> +           hsr.bits, hsr.ec, hsr.len, hsr.iss);

This would be better as a single printk() call, otherwise a different
cpu issuing a printk() could interleave in the middle of the line.

Also, you appear to have dropped the space between "detected" and "ESR"

~Andrew