Re: [Xen-devel] [PATCH v2] xen/arm: Correctly support WARN_ON

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-08-05 at 17:32 +0100, Julien Grall wrote:
> Currently the hypervisor will hang if it hits a WARN_ON.
> 
> The implemention uses an undefined instruction, made ourself because ARM
> doesn't provide one, to implement BUG/ASSERT/WARN_ON, and sets up the
> different tables (one for each type) which contain useful information.
> 
> This is based on the x86 implementation (include/asm-x86/bug.h). Unfortunately
> the structure can't be shared because many ARM{32,64} gcc versions doesn't
> correctly support %c. The support of executing a function in an exception 
> handler
> is also keep unimplemented on ARM. Therefore, dump_execution_state is
> implement as WARN()
> 
> The current opcode used to go in exception mode may not be undefined on ARM64.
> Use the instruction "brk" to generate a software debug exception.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> 
> ---
>     Changes in v2:
>         - Missing static in do_bug_frame prototype
>         - Add support for ARM64 by using the instruction brk
>         - Implement dump_execution_state as WARN
> ---
>  xen/arch/arm/arm32/traps.c      |   22 ++++++++-
>  xen/arch/arm/traps.c            |  103 
> +++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/xen.lds.S          |    8 +++
>  xen/include/asm-arm/arm32/bug.h |   13 +++++
>  xen/include/asm-arm/arm64/bug.h |   10 ++++
>  xen/include/asm-arm/bug.h       |   77 +++++++++++++++++++++++++++--
>  xen/include/asm-arm/debugger.h  |    2 +-
>  xen/include/asm-arm/processor.h |   17 ++++++-
>  8 files changed, 246 insertions(+), 6 deletions(-)
>  create mode 100644 xen/include/asm-arm/arm32/bug.h
>  create mode 100644 xen/include/asm-arm/arm64/bug.h
> 
> diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
> index ff0b945..baf8c41 100644
> --- a/xen/arch/arm/arm32/traps.c
> +++ b/xen/arch/arm/arm32/traps.c
> @@ -18,6 +18,7 @@
>  
>  #include <xen/config.h>
>  #include <xen/lib.h>
> +#include <xen/kernel.h>
>  
>  #include <public/xen.h>
>  
> @@ -25,7 +26,26 @@
>  
>  asmlinkage void do_trap_undefined_instruction(struct cpu_user_regs *regs)
>  {
> -    do_unexpected_trap("Undefined Instruction", regs);
> +    uint64_t pc = regs->pc;
> +    uint32_t instr;
> +
> +    if ( !is_kernel_text(pc) &&
> +         (system_state >= SYS_STATE_active || !is_kernel_inittext(pc)) )
> +        goto die;
> +
> +    /* PC is always 4-byte align, as Xen is using ARM instruction set */

"aligned"

Is it worth a check here? I presume the nested fault if PC were
misaligned would be pretty exciting, print+goto die would seem
appropriate.

> +    instr = *((uint32_t *)regs->pc);
> +    if ( instr != BUG_OPCODE )
> +        goto die;
> +
> +    if ( do_bug_frame(regs, regs->pc) )
> +        goto die;
> +
> +    regs->pc += 4;
> +    return;
> +
> +die:
> +    do_unexpected_trap("undefined instruction", regs);

No need to change the case of this message.

>  }> +#ifdef CONFIG_ARM_64
> +static void do_trap_brk(struct cpu_user_regs *regs, 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
> +     */

BUG_ON? ;-O

> +die:
> +        do_unexpected_trap("undefined breakpoint value", regs);

Please can you capitilise this to match the other callers.


> +/* Many version of GCC doesn't support the asm %c parameter which would

"versions".

> + * be preferable to this unpleasantness. We use mergeable string
> + * sections to avoid multiple copies of the string appearing in the
> + * Xen image.

OOI what is the size increase of the final (stripped) binary with this patch?

Ian.


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