diff mbox

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

Message ID 1404240719-13164-1-git-send-email-julien.grall@linaro.org
State Superseded, archived
Headers show

Commit Message

Julien Grall July 1, 2014, 6:51 p.m. UTC
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.

TODO:
I haven't yet hook the code on ARM64 as I'm not sure how undefined instruction
are handled. It looks like there is multiple way to get it via HSR.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/arm32/traps.c      |    2 +-
 xen/arch/arm/traps.c            |   96 +++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/xen.lds.S          |    8 ++++
 xen/include/asm-arm/bug.h       |   72 +++++++++++++++++++++++++++--
 xen/include/asm-arm/debugger.h  |    2 +-
 xen/include/asm-arm/processor.h |    2 +
 6 files changed, 177 insertions(+), 5 deletions(-)

Comments

Ian Campbell July 3, 2014, 10:30 a.m. UTC | #1
On Tue, 2014-07-01 at 19:51 +0100, Julien Grall wrote:

> TODO:
> I haven't yet hook the code on ARM64 as I'm not sure how undefined
> instruction are handled. It looks like there is multiple way to get it
> via HSR.

I think EC==0x00 (the catchall) is where it will end up "The attempted
execution of an instruction bit patterns that has no allocated
instruction at the current Exception level".

> +/* ARM doesn't provide any undefined instruction, make our own based on
> + * a combination of bits which is not used by the instruction set
> + */
> +#define BUG_INSTR 0xe7f000f0

Linux uses 0xe7f001f2. I'm not sure what the difference is but it seems
safer to follow suit.

Ian.
Julien Grall July 3, 2014, 12:33 p.m. UTC | #2
Hi Ian,

On 07/03/2014 11:30 AM, Ian Campbell wrote:
> On Tue, 2014-07-01 at 19:51 +0100, Julien Grall wrote:
> 
>> TODO:
>> I haven't yet hook the code on ARM64 as I'm not sure how undefined
>> instruction are handled. It looks like there is multiple way to get it
>> via HSR.
> 
> I think EC==0x00 (the catchall) is where it will end up "The attempted
> execution of an instruction bit patterns that has no allocated
> instruction at the current Exception level".

So if EC==0x00 I can safely assume this is a undefined instruction
exception, right?

>> +/* ARM doesn't provide any undefined instruction, make our own based on
>> + * a combination of bits which is not used by the instruction set
>> + */
>> +#define BUG_INSTR 0xe7f000f0
> 
> Linux uses 0xe7f001f2. I'm not sure what the difference is but it seems
> safer to follow suit.

Linux uses multiple different undefined opcode (bug, debugger,...). This
value is already used in Xen by dump_execution_state (see
include/asm-arm/processor.h).

I don't see why we should change it.

Regards,
Ian Campbell July 3, 2014, 1:44 p.m. UTC | #3
On Thu, 2014-07-03 at 13:33 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 07/03/2014 11:30 AM, Ian Campbell wrote:
> > On Tue, 2014-07-01 at 19:51 +0100, Julien Grall wrote:
> > 
> >> TODO:
> >> I haven't yet hook the code on ARM64 as I'm not sure how undefined
> >> instruction are handled. It looks like there is multiple way to get it
> >> via HSR.
> > 
> > I think EC==0x00 (the catchall) is where it will end up "The attempted
> > execution of an instruction bit patterns that has no allocated
> > instruction at the current Exception level".
> 
> So if EC==0x00 I can safely assume this is a undefined instruction
> exception, right?

No, EC=0x00 is a catch all thing, I think you would need to work out
which reason it was. It might be that having consulted the ARM ARM you
decide that an undefined instruction is the only cause which applies to
us.

> 
> >> +/* ARM doesn't provide any undefined instruction, make our own based on
> >> + * a combination of bits which is not used by the instruction set
> >> + */
> >> +#define BUG_INSTR 0xe7f000f0
> > 
> > Linux uses 0xe7f001f2. I'm not sure what the difference is but it seems
> > safer to follow suit.
> 
> Linux uses multiple different undefined opcode (bug, debugger,...). This
> value is already used in Xen by dump_execution_state (see
> include/asm-arm/processor.h).
> 
> I don't see why we should change it.

OK.
Ian Campbell July 10, 2014, 3:06 p.m. UTC | #4
On Tue, 2014-07-01 at 19:51 +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.
> 
> TODO:
> I haven't yet hook the code on ARM64 as I'm not sure how undefined instruction
> are handled. It looks like there is multiple way to get it via HSR.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Apart from the lack of arm64 support this looks good to me.

Ian.
Julien Grall July 10, 2014, 3:07 p.m. UTC | #5
On 10/07/14 16:06, Ian Campbell wrote:
> On Tue, 2014-07-01 at 19:51 +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.
>>
>> TODO:
>> I haven't yet hook the code on ARM64 as I'm not sure how undefined instruction
>> are handled. It looks like there is multiple way to get it via HSR.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>
> Apart from the lack of arm64 support this looks good to me.

I will try to send a new version with the arm64 support next week.

Regards,
diff mbox

Patch

diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c
index ff0b945..f79edcb 100644
--- a/xen/arch/arm/arm32/traps.c
+++ b/xen/arch/arm/arm32/traps.c
@@ -25,7 +25,7 @@ 
 
 asmlinkage void do_trap_undefined_instruction(struct cpu_user_regs *regs)
 {
-    do_unexpected_trap("Undefined Instruction", regs);
+    do_undefined_instruction(regs);
 }
 
 asmlinkage void do_trap_supervisor_call(struct cpu_user_regs *regs)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 686d8b7..921ae54 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -32,6 +32,7 @@ 
 #include <xen/domain_page.h>
 #include <public/sched.h>
 #include <public/xen.h>
+#include <asm/debugger.h>
 #include <asm/event.h>
 #include <asm/regs.h>
 #include <asm/cpregs.h>
@@ -1002,6 +1003,101 @@  void do_unexpected_trap(const char *msg, struct cpu_user_regs *regs)
     panic("CPU%d: Unexpected Trap: %s\n", smp_processor_id(), msg);
 }
 
+int do_bug_frame(struct cpu_user_regs *regs, vaddr_t pc)
+{
+    const struct bug_frame *bug;
+    const char *prefix = "", *filename, *predicate;
+    unsigned long fixup;
+    int id, lineno;
+    static const struct bug_frame *const stop_frames[] = {
+        __stop_bug_frames_0,
+        __stop_bug_frames_1,
+        __stop_bug_frames_2,
+        NULL
+    };
+
+    for ( bug = __start_bug_frames, id = 0; stop_frames[id]; ++bug )
+    {
+        while ( unlikely(bug == stop_frames[id]) )
+            ++id;
+
+        if ( ((vaddr_t)bug_loc(bug)) == pc )
+            break;
+    }
+
+    if ( !stop_frames[id] )
+        return -ENOENT;
+
+    /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
+    filename = bug_file(bug);
+    if ( !is_kernel(filename) )
+        return -EINVAL;
+    fixup = strlen(filename);
+    if ( fixup > 50 )
+    {
+        filename += fixup - 47;
+        prefix = "...";
+    }
+    lineno = bug_line(bug);
+
+    switch ( id )
+    {
+    case BUGFRAME_warn:
+        printk("Xen WARN at %s%s:%d\n", prefix, filename, lineno);
+        show_execution_state(regs);
+        return 0;
+
+    case BUGFRAME_bug:
+        printk("Xen BUG at %s%s:%d\n", prefix, filename, lineno);
+
+        if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
+            return 0;
+
+        show_execution_state(regs);
+        panic("Xen BUG at %s%s:%d", prefix, filename, lineno);
+
+    case BUGFRAME_assert:
+        /* ASSERT: decode the predicate string pointer. */
+        predicate = bug_msg(bug);
+        if ( !is_kernel(predicate) )
+            predicate = "<unknown>";
+
+        printk("Assertion '%s' failed at %s%s:%d\n",
+               predicate, prefix, filename, lineno);
+        if ( debugger_trap_fatal(TRAP_invalid_op, regs) )
+            return 0;
+        show_execution_state(regs);
+        panic("Assertion '%s' failed at %s%s:%d",
+              predicate, prefix, filename, lineno);
+    }
+
+    return -EINVAL;
+}
+
+void do_undefined_instruction(struct cpu_user_regs *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 */
+    instr = *((uint32_t *)regs->pc);
+    if ( instr != BUG_INSTR )
+        goto die;
+
+    if ( do_bug_frame(regs, regs->pc) )
+        goto die;
+
+    regs->pc += 4;
+    return;
+
+die:
+    do_unexpected_trap("undefined instruction", regs);
+}
+
 typedef register_t (*arm_hypercall_fn_t)(
     register_t, register_t, register_t, register_t, register_t);
 
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index be55dad..c7a47c2 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -40,6 +40,14 @@  SECTIONS
   . = ALIGN(PAGE_SIZE);
   .rodata : {
         _srodata = .;          /* Read-only data */
+        /* Bug frames table */
+       __start_bug_frames = .;
+       *(.bug_frames.0)
+       __stop_bug_frames_0 = .;
+       *(.bug_frames.1)
+       __stop_bug_frames_1 = .;
+       *(.bug_frames.2)
+       __stop_bug_frames_2 = .;
        *(.rodata)
        *(.rodata.*)
         _erodata = .;          /* End of read-only data */
diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h
index 458c818..0038df4 100644
--- a/xen/include/asm-arm/bug.h
+++ b/xen/include/asm-arm/bug.h
@@ -1,10 +1,76 @@ 
 #ifndef __ARM_BUG_H__
 #define __ARM_BUG_H__
 
-#define BUG() __bug(__FILE__, __LINE__)
-#define WARN() __warn(__FILE__, __LINE__)
+#include <xen/stringify.h>
 
-#endif /* __X86_BUG_H__ */
+#define BUG_DISP_WIDTH    24
+#define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
+#define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
+
+struct bug_frame {
+    signed int loc_disp;    /* Relative address to the bug address */
+    signed int file_disp;   /* Relative address to the filename */
+    signed int msg_disp;    /* Relative address to the predicate (for ASSERT) */
+    uint16_t line;          /* Line number */
+    uint32_t pad0:16;       /* Padding for 8-bytes align */
+};
+
+#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
+#define bug_file(b) ((const void *)(b) + (b)->file_disp);
+#define bug_line(b) ((b)->line)
+#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
+
+#define BUGFRAME_warn   0
+#define BUGFRAME_bug    1
+#define BUGFRAME_assert 2
+
+/* ARM doesn't provide any undefined instruction, make our own based on
+ * a combination of bits which is not used by the instruction set
+ */
+#define BUG_INSTR 0xe7f000f0
+
+/* Many version of GCC doesn't support the asm %c parameter which would
+ * be preferable to this unpleasantness. We use mergeable string
+ * sections to avoid multiple copies of the string appearing in the
+ * Xen image.
+ */
+#define BUG_FRAME(type, line, file, has_msg, msg) do {                      \
+    BUILD_BUG_ON((line) >> 16);                                             \
+    asm ("1:.word "__stringify(BUG_INSTR)"\n"                               \
+         ".pushsection .rodata.str, \"aMS\", %progbits, 1\n"                \
+         "2:\t.asciz " __stringify(file) "\n"                               \
+         "3:\n"                                                             \
+         ".if " #has_msg "\n"                                               \
+         "\t.asciz " #msg "\n"                                              \
+         ".endif\n"                                                         \
+         ".popsection\n"                                                    \
+         ".pushsection .bug_frames." __stringify(type) ", \"a\", %progbits\n"\
+         "4:\n"                                                             \
+         ".long (1b - 4b)\n"                                                \
+         ".long (2b - 4b)\n"                                                \
+         ".long (3b - 4b)\n"                                                \
+         ".hword " __stringify(line) ", 0\n"                                \
+         ".popsection");                                                    \
+} while (0)
+
+#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")
+
+#define BUG() do {                                              \
+    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, "");        \
+    unreachable();                                              \
+} while (0)
+
+#define assert_failed(msg) do {                                 \
+    BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg);     \
+    unreachable();                                              \
+} while (0)
+
+extern const struct bug_frame __start_bug_frames[],
+                              __stop_bug_frames_0[],
+                              __stop_bug_frames_1[],
+                              __stop_bug_frames_2[];
+
+#endif /* __ARM_BUG_H__ */
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/debugger.h b/xen/include/asm-arm/debugger.h
index 916860b..ac776ef 100644
--- a/xen/include/asm-arm/debugger.h
+++ b/xen/include/asm-arm/debugger.h
@@ -1,7 +1,7 @@ 
 #ifndef __ARM_DEBUGGER_H__
 #define __ARM_DEBUGGER_H__
 
-#define debugger_trap_fatal(v, r) ((void) 0)
+#define debugger_trap_fatal(v, r) (0)
 #define debugger_trap_immediate() ((void) 0)
 
 #endif /* __ARM_DEBUGGER_H__ */
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 3be86f1..ff24ef0 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -480,6 +480,8 @@  void show_registers(struct cpu_user_regs *regs);
 
 void do_unexpected_trap(const char *msg, struct cpu_user_regs *regs);
 
+void do_undefined_instruction(struct cpu_user_regs *regs);
+
 void vcpu_regs_hyp_to_user(const struct vcpu *vcpu,
                            struct vcpu_guest_core_regs *regs);
 void vcpu_regs_user_to_hyp(struct vcpu *vcpu,