Re: [PATCH v3] xen/arm: Correctly support WARN_ON

[Ian Campbell <Ian.Campbell@citrix.com>]

On Wed, 2014-09-10 at 16:33 -0700, Julien Grall wrote:
> Currently the hypervisor will hang if it hits a WARN_ON.
> 
> The implemention uses an undefined instruction, made ourself because ARM

"implementation"

> 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

s/doesn't/don't/

> correctly support %c. The support of executing a function in an exception handler

s/of/for/

> is also keep unimplemented on ARM. Therefore, dump_execution_state is
> implement as WARN()

"implemented".

> +#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
> +     */
> +    /* It's not possible to use BUG_ON here, because we would recurse */
> +    if ( unlikely(READ_SYSREG(HCR_EL2) & HCR_TGE) ||
> +         unlikely(READ_SYSREG(MDCR_EL2) & HDCR_TDE) )
> +        panic("Unable to handle brk exception from EL1/EL0");

Either of those bits being set doesn't imply that *this* trap came from
EL1/EL0.

The correct check would be BUG_ON(!hyp_mode(regs)) which I think won't
recurse because the resulting BRK will pass the check the second time,
although if you want to if (!hyp_mode(regs)) panic(...) instead that's
fine too.

Ian.