[PATCH 0/3] xen/x86: Improvements to asm assertions

[PATCH 0/3] xen/x86: Improvements to asm assertions

[Andrew Cooper]

In XenServer, we have a single server which occasionally finds interrupts
unexpectedtly in the wrong state (I suspect the SMM handler).  However,
asserts from ASSERT_INTERRUPTS_{EN,DIS}ABLED are quite opaque, being an
unqualified fatal #UD.

Andrew Cooper (3):
  x86/bug: Infrastructure to create BUG_FRAMES in asm code
  xen/x86: Use real assert frames for ASSERT_INTERRUPTS_{EN,DIS}ABLED
  DO NOT APPLY - test code for this series

 xen/arch/x86/traps.c            |   66 +++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/x86_64/entry.S     |   26 +++++++++++++++
 xen/include/asm-x86/asm_defns.h |   25 +++++++++------
 xen/include/asm-x86/bug.h       |   48 +++++++++++++++++++++++++---
 4 files changed, 151 insertions(+), 14 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] xen/x86: Infrastructure to create BUG_FRAMES in asm code

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/include/asm-x86/bug.h |   48 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h
index cd862e3..365c6b8 100644
--- a/xen/include/asm-x86/bug.h
+++ b/xen/include/asm-x86/bug.h
@@ -5,6 +5,13 @@
 #define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
 #define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
 
+#define BUGFRAME_run_fn 0
+#define BUGFRAME_warn   1
+#define BUGFRAME_bug    2
+#define BUGFRAME_assert 3
+
+#ifndef __ASSEMBLY__
+
 struct bug_frame {
     signed int loc_disp:BUG_DISP_WIDTH;
     unsigned int line_hi:BUG_LINE_HI_WIDTH;
@@ -22,11 +29,6 @@ struct bug_frame {
                       ((1 << BUG_LINE_LO_WIDTH) - 1)))
 #define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
 
-#define BUGFRAME_run_fn 0
-#define BUGFRAME_warn   1
-#define BUGFRAME_bug    2
-#define BUGFRAME_assert 3
-
 #define BUG_FRAME(type, line, ptr, second_frame, msg) do {                   \
     BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH));         \
     asm volatile ( ".Lbug%=: ud2\n"                                          \
@@ -66,4 +68,40 @@ struct bug_frame {
                               __stop_bug_frames_2[],
                               __stop_bug_frames_3[];
 
+#else  /* !__ASSEMBLY__ */
+
+/*
+ * Construct a bugframe, suitable for using in assembly code.  Should always
+ * match the C version above.  One complication is having to stash the strings
+ * in .rodata (TODO - figure out how to get GAS to elide duplicate file_str's)
+ */
+.macro BUG_FRAME type, line, file_str, second_frame, msg
+92: ud2a
+
+.pushsection .rodata
+94: .asciz "\file_str"
+.popsection
+
+.pushsection .bug_frames.\type, "a", @progbits
+93:
+.long (92b - 93b) + ((\line >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
+.long (94b - 93b) + ((\line & ((1 << BUG_LINE_LO_WIDTH) - 1)) << BUG_DISP_WIDTH)
+
+.if \second_frame
+     .pushsection .rodata
+     95: .asciz "\msg"
+     .popsection
+.long 0, (95b - 93b)
+.endif
+.popsection
+.endm
+
+#define WARN() BUG_FRAME BUGFRAME_warn, __LINE__, __FILE__, 0, 0
+#define BUG()  BUG_FRAME BUGFRAME_bug,  __LINE__, __FILE__, 0, 0
+
+#define ASSERT_FAILED(msg)                                      \
+     BUG_FRAME BUGFRAME_assert, __LINE__, __FILE__, 1, msg
+
+#endif /* !__ASSEMBLY__ */
+
 #endif /* __X86_BUG_H__ */
-- 
1.7.10.4

[PATCH 2/3] xen/x86: Use real assert frames for ASSERT_INTERRUPTS_{EN, DIS}ABLED

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/include/asm-x86/asm_defns.h |   25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index 1674c7c..e8a678e 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -6,6 +6,7 @@
 /* NB. Auto-generated from arch/.../asm-offsets.c */
 #include <asm/asm-offsets.h>
 #endif
+#include <asm/bug.h>
 #include <asm/processor.h>
 #include <asm/percpu.h>
 #include <xen/stringify.h>
@@ -26,18 +27,24 @@
 #endif
 
 #ifndef NDEBUG
-#define ASSERT_INTERRUPT_STATUS(x)              \
-        pushf;                                  \
-        testb $X86_EFLAGS_IF>>8,1(%rsp);        \
-        j##x  1f;                               \
-        ud2a;                                   \
-1:      addq  $8,%rsp;
+#define ASSERT_INTERRUPTS_ENABLED               \
+    pushf;                                      \
+    testb $X86_EFLAGS_IF>>8,1(%rsp);            \
+    jnz   1f;                                   \
+    ASSERT_FAILED("INTERRUPTS ENABLED");        \
+1:  addq  $8,%rsp;
+
+#define ASSERT_INTERRUPTS_DISABLED              \
+    pushf;                                      \
+    testb $X86_EFLAGS_IF>>8,1(%rsp);            \
+    jz    1f;                                   \
+    ASSERT_FAILED("INTERRUPTS DISABLED");       \
+1:  addq  $8,%rsp;
 #else
-#define ASSERT_INTERRUPT_STATUS(x)
+#define ASSERT_INTERRUPTS_ENABLED
+#define ASSERT_INTERRUPTS_DISABLED
 #endif
 
-#define ASSERT_INTERRUPTS_ENABLED  ASSERT_INTERRUPT_STATUS(nz)
-#define ASSERT_INTERRUPTS_DISABLED ASSERT_INTERRUPT_STATUS(z)
 
 /*
  * This flag is set in an exception frame when registers R12-R15 did not get
-- 
1.7.10.4

[PATCH 3/3] DO NOT APPLY - test code for this series

[Andrew Cooper]

---
 xen/arch/x86/traps.c        |   66 +++++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/x86_64/entry.S |   26 +++++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 22cdfc4..c562842 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3981,6 +3981,72 @@ void asm_domain_crash_synchronous(unsigned long addr)
     __domain_crash_synchronous();
 }
 
+#include <xen/keyhandler.h>
+
+void asm_test_warnframe(void);
+void asm_test_bugframe(void);
+void asm_test_assertframe(void);
+void asm_test_assert_enabled(void);
+void asm_test_assert_disabled(void);
+
+static void do_extreme_debug(unsigned char key, struct cpu_user_regs *regs)
+{
+    printk("'%c' pressed -> Extreme debugging in progress...\n", key);
+
+    switch ( key )
+    {
+    case '1':
+        printk("WARN frame:\n");
+        asm_test_warnframe();
+        printk("Done\n");
+        break;
+
+    case '2':
+        printk("BUG frame:\n");
+        asm_test_bugframe();
+        printk("Done\n");
+        break;
+
+    case '3':
+        printk("ASSERT frame:\n");
+        asm_test_assertframe();
+        printk("Done\n");
+        break;
+
+    case '4':
+        printk("Trip ASSERT_IRQ_ENABLED:\n");
+        asm_test_assert_enabled();
+        printk("Done\n");
+        break;
+
+    case '5':
+        printk("Trip ASSERT_IRQ_DISABLED:\n");
+        asm_test_assert_disabled();
+        printk("Done\n");
+        break;
+
+    default:
+        printk("Nothing\n");
+    }
+}
+
+static struct keyhandler extreme_debug_keyhandler = {
+    .irq_callback = 1,
+    .u.irq_fn = do_extreme_debug,
+    .desc = "Extreme debugging"
+};
+
+static int __init extreme_debug_keyhandler_init(void)
+{
+    register_keyhandler('1', &extreme_debug_keyhandler);
+    register_keyhandler('2', &extreme_debug_keyhandler);
+    register_keyhandler('3', &extreme_debug_keyhandler);
+    register_keyhandler('4', &extreme_debug_keyhandler);
+    register_keyhandler('5', &extreme_debug_keyhandler);
+    return 0;
+}
+__initcall(extreme_debug_keyhandler_init);
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 2d25d57..233483b 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -13,6 +13,32 @@
 #include <public/xen.h>
 #include <irq_vectors.h>
 
+ENTRY(asm_test_warnframe)
+        WARN()
+        ret
+
+ENTRY(asm_test_bugframe)
+        BUG()
+        ret
+
+ENTRY(asm_test_assertframe)
+	ASSERT_FAILED("asm assert frame")
+	ret
+
+ENTRY(asm_test_assert_enabled)
+        pushfq
+        cli
+	ASSERT_INTERRUPTS_ENABLED
+        popfq
+	ret
+
+ENTRY(asm_test_assert_disabled)
+        pushfq
+        sti
+	ASSERT_INTERRUPTS_DISABLED
+        popfq
+	ret
+
         ALIGN
 /* %rbx: struct vcpu */
 switch_to_kernel:
-- 
1.7.10.4

Re: [PATCH 1/3] xen/x86: Infrastructure to create BUG_FRAMES in asm code

[Jan Beulich]

>>> On 09.04.15 at 22:06, <andrew.cooper3@citrix.com> wrote:
> @@ -66,4 +68,40 @@ struct bug_frame {
>                                __stop_bug_frames_2[],
>                                __stop_bug_frames_3[];
>  
> +#else  /* !__ASSEMBLY__ */
> +
> +/*
> + * Construct a bugframe, suitable for using in assembly code.  Should always
> + * match the C version above.  One complication is having to stash the strings
> + * in .rodata (TODO - figure out how to get GAS to elide duplicate file_str's)

Use the 'M' and 'S' section flags (and perhaps the section name gcc
also uses for this purpose, .rodata.str1 or some such).

> +.macro BUG_FRAME type, line, file_str, second_frame, msg

Can we please avoid spreading the bad habit of starting directives
in the first column? The only thing formally allowed to start there is
a label.

> +92: ud2a

Instead of using number labels that can conflict with the context
the macro is used in, please use .L prefixed ones together with
\@ to reference the macro invocation count (as a unique number).

> +.if \second_frame
> +     .pushsection .rodata
> +     95: .asciz "\msg"
> +     .popsection
> +.long 0, (95b - 93b)
> +.endif
> +.popsection
> +.endm

Please be consistent with indentation.

> +#define WARN() BUG_FRAME BUGFRAME_warn, __LINE__, __FILE__, 0, 0
> +#define BUG()  BUG_FRAME BUGFRAME_bug,  __LINE__, __FILE__, 0, 0

I don't think the parentheses are particularly well suited for
assembly code.

Jan

Re: [PATCH 2/3] xen/x86: Use real assert frames for ASSERT_INTERRUPTS_{EN, DIS}ABLED

[Jan Beulich]

>>> On 09.04.15 at 22:06, <andrew.cooper3@citrix.com> wrote:
> @@ -26,18 +27,24 @@
>  #endif
>  
>  #ifndef NDEBUG
> -#define ASSERT_INTERRUPT_STATUS(x)              \
> -        pushf;                                  \
> -        testb $X86_EFLAGS_IF>>8,1(%rsp);        \
> -        j##x  1f;                               \
> -        ud2a;                                   \
> -1:      addq  $8,%rsp;
> +#define ASSERT_INTERRUPTS_ENABLED               \
> +    pushf;                                      \
> +    testb $X86_EFLAGS_IF>>8,1(%rsp);            \
> +    jnz   1f;                                   \
> +    ASSERT_FAILED("INTERRUPTS ENABLED");        \
> +1:  addq  $8,%rsp;
> +
> +#define ASSERT_INTERRUPTS_DISABLED              \
> +    pushf;                                      \
> +    testb $X86_EFLAGS_IF>>8,1(%rsp);            \
> +    jz    1f;                                   \
> +    ASSERT_FAILED("INTERRUPTS DISABLED");       \
> +1:  addq  $8,%rsp;
>  #else
> -#define ASSERT_INTERRUPT_STATUS(x)
> +#define ASSERT_INTERRUPTS_ENABLED
> +#define ASSERT_INTERRUPTS_DISABLED
>  #endif
>  
> -#define ASSERT_INTERRUPTS_ENABLED  ASSERT_INTERRUPT_STATUS(nz)
> -#define ASSERT_INTERRUPTS_DISABLED ASSERT_INTERRUPT_STATUS(z)

So what's the point of deleting these and duplicating most of the
macro definition text above?

Jan