[PATCH v4 0/3] xen: add support for automatic debug key actions in case of crash

[PATCH v4 0/3] xen: add support for automatic debug key actions in case of crash

[Juergen Gross]

When the host crashes it would sometimes be nice to have additional
debug data available which could be produced via debug keys, but
halting the server for manual intervention might be impossible due to
the need to reboot/kexec rather sooner than later.

Add support for automatic debug key actions in case of crashes which
can be activated via boot- or runtime-parameter.

Changes in V4:
- addressed comments (now patch 3)
- added patches 1 and 2

Some further remarks to the new patches added:

Patch 1 adds Arm support for run_in_exception_handler(). Constructing
the related bug_frame is unfortunately not as easy as on x86.

I have verified that %c in inline assembly isn't supported by gcc 7 for
arm64, so this was the only way I've found to support this feature. In
theory it might be possible to add a variable referencing the called
function and to discard that variable again when linking, but I'd like
to add this more intrusive modification only if really wanted.

There is more potential for unifying struct bug_frame between x86 and
Arm, either by:
- using the Arm layout on x86, too (resulting in a grow of the bugframe
  data for the cases without messages)
- trying to construct the data in C instead of inline assembly, which
  will need to either keep the x86 assembler BUG_FRAME construction, or
  to add a few functions issuing the ASSERT/BUG/WARN which would then
  need to be called from *.S files.

Patch 2 opens up more potential for simplification: in theory there is
no need any more to call any key handler with the regs parameter,
allowing to use the same prototype for all handlers. The downside would
be to have an additional irq frame on the stack for the dump_registers()
and the do_debug_key() handlers. In case this is acceptable I'd be
happy to send a related cleanup patch.

Juergen Gross (3):
  xen/arm: add support for run_in_exception_handler()
  xen: enable keyhandlers to work without register set specified
  xen: add support for automatic debug key actions in case of crash

 docs/misc/xen-command-line.pandoc | 41 +++++++++++++++++
 xen/arch/arm/traps.c              | 10 ++++-
 xen/common/kexec.c                |  8 ++--
 xen/common/keyhandler.c           | 73 +++++++++++++++++++++++++++++--
 xen/common/shutdown.c             |  4 +-
 xen/drivers/char/console.c        |  2 +-
 xen/drivers/char/ns16550.c        |  3 +-
 xen/include/asm-arm/bug.h         | 32 +++++++++-----
 xen/include/xen/kexec.h           | 10 ++++-
 xen/include/xen/keyhandler.h      | 10 +++++
 10 files changed, 169 insertions(+), 24 deletions(-)

-- 
2.26.2

[PATCH v4 1/3] xen/arm: add support for run_in_exception_handler()

[Juergen Gross]

Add support to run a function in an exception handler for Arm. Do it
the same way as on x86 via a bug_frame.

Unfortunately inline assembly on Arm seems to be less capable than on
x86, leading to functions called via run_in_exception_handler() having
to be globally visible.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4:
- new patch

I have verified the created bugframe is correct by inspecting the
created binary.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/arch/arm/traps.c       | 10 +++++++++-
 xen/drivers/char/ns16550.c |  3 ++-
 xen/include/asm-arm/bug.h  | 32 +++++++++++++++++++++-----------
 3 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 22bd1bd4c6..6e677affe2 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1236,8 +1236,16 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
     if ( !bug )
         return -ENOENT;
 
+    if ( id == BUGFRAME_run_fn )
+    {
+        void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
+
+        fn(regs);
+        return 0;
+    }
+
     /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
-    filename = bug_file(bug);
+    filename = bug_ptr(bug);
     if ( !is_kernel(filename) )
         return -EINVAL;
     fixup = strlen(filename);
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 9235d854fe..dd6500acc8 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -192,7 +192,8 @@ static void ns16550_interrupt(
 /* Safe: ns16550_poll() runs as softirq so not reentrant on a given CPU. */
 static DEFINE_PER_CPU(struct serial_port *, poll_port);
 
-static void __ns16550_poll(struct cpu_user_regs *regs)
+/* run_in_exception_handler() on Arm requires globally visible symbol. */
+void __ns16550_poll(struct cpu_user_regs *regs)
 {
     struct serial_port *port = this_cpu(poll_port);
     struct ns16550 *uart = port->uart;
diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h
index 36c803357c..a7da2c306f 100644
--- a/xen/include/asm-arm/bug.h
+++ b/xen/include/asm-arm/bug.h
@@ -15,34 +15,38 @@
 
 struct bug_frame {
     signed int loc_disp;    /* Relative address to the bug address */
-    signed int file_disp;   /* Relative address to the filename */
+    signed int ptr_disp;    /* Relative address to the filename or function */
     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_ptr(b) ((const void *)(b) + (b)->ptr_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
+#define BUGFRAME_run_fn 0
+#define BUGFRAME_warn   1
+#define BUGFRAME_bug    2
+#define BUGFRAME_assert 3
 
-#define BUGFRAME_NR     3
+#define BUGFRAME_NR     4
 
 /* Many versions 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 {                      \
+#define BUG_FRAME(type, line, ptr, ptr_is_file, has_msg, msg) do {          \
     BUILD_BUG_ON((line) >> 16);                                             \
     BUILD_BUG_ON((type) >= BUGFRAME_NR);                                    \
     asm ("1:"BUG_INSTR"\n"                                                  \
          ".pushsection .rodata.str, \"aMS\", %progbits, 1\n"                \
-         "2:\t.asciz " __stringify(file) "\n"                               \
+         "2:\n"                                                             \
+         ".if " #ptr_is_file "\n"                                           \
+         "\t.asciz " __stringify(ptr) "\n"                                  \
+         ".endif\n"                                                         \
          "3:\n"                                                             \
          ".if " #has_msg "\n"                                               \
          "\t.asciz " #msg "\n"                                              \
@@ -52,21 +56,27 @@ struct bug_frame {
          "4:\n"                                                             \
          ".p2align 2\n"                                                     \
          ".long (1b - 4b)\n"                                                \
+         ".if " #ptr_is_file "\n"                                           \
          ".long (2b - 4b)\n"                                                \
+         ".else\n"                                                          \
+         ".long (" #ptr " - 4b)\n"                                          \
+         ".endif\n"                                                         \
          ".long (3b - 4b)\n"                                                \
          ".hword " __stringify(line) ", 0\n"                                \
          ".popsection");                                                    \
 } while (0)
 
-#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")
+#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, 0, "")
+
+#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 1, 0, "")
 
 #define BUG() do {                                              \
-    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, "");        \
+    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 1, 0, "");        \
     unreachable();                                              \
 } while (0)
 
 #define assert_failed(msg) do {                                 \
-    BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg);     \
+    BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, 1, msg);     \
     unreachable();                                              \
 } while (0)
 
-- 
2.26.2

[PATCH v4 2/3] xen: enable keyhandlers to work without register set specified

[Juergen Gross]

There are only two keyhandlers which make use of the cpu_user_regs
struct passed to them. In order to be able to call any keyhandler in
non-interrupt contexts, too, modify those two handlers to copy with a
NULL regs pointer by using run_in_exception_handler() in that case.

Suggested-by: Julien Grall <julien@xen.org>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V4:
- new patch
---
 xen/common/keyhandler.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 68364e987d..de120fa092 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -181,7 +181,10 @@ static void dump_registers(unsigned char key, struct cpu_user_regs *regs)
     cpumask_copy(&dump_execstate_mask, &cpu_online_map);
 
     /* Get local execution state out immediately, in case we get stuck. */
-    dump_execstate(regs);
+    if ( regs )
+        dump_execstate(regs);
+    else
+        run_in_exception_handler(dump_execstate);
 
     /* Alt. handling: remaining CPUs are dumped asynchronously one-by-one. */
     if ( alt_key_handling )
@@ -481,15 +484,24 @@ static void run_all_keyhandlers(unsigned char key, struct cpu_user_regs *regs)
     tasklet_schedule(&run_all_keyhandlers_tasklet);
 }
 
-static void do_debug_key(unsigned char key, struct cpu_user_regs *regs)
+/* run_in_exception_handler() on Arm requires globally visible symbol. */
+void do_debugger_trap_fatal(struct cpu_user_regs *regs)
 {
-    printk("'%c' pressed -> trapping into debugger\n", key);
     (void)debugger_trap_fatal(0xf001, regs);
 
     /* Prevent tail call optimisation, which confuses xendbg. */
     barrier();
 }
 
+static void do_debug_key(unsigned char key, struct cpu_user_regs *regs)
+{
+    printk("'%c' pressed -> trapping into debugger\n", key);
+    if ( regs )
+        do_debugger_trap_fatal(regs);
+    else
+        run_in_exception_handler(do_debugger_trap_fatal);
+}
+
 static void do_toggle_alt_key(unsigned char key, struct cpu_user_regs *regs)
 {
     alt_key_handling = !alt_key_handling;
-- 
2.26.2

[PATCH v4 3/3] xen: add support for automatic debug key actions in case of crash

[Juergen Gross]

When the host crashes it would sometimes be nice to have additional
debug data available which could be produced via debug keys, but
halting the server for manual intervention might be impossible due to
the need to reboot/kexec rather sooner than later.

Add support for automatic debug key actions in case of crashes which
can be activated via boot- or runtime-parameter.

Depending on the type of crash the desired data might be different, so
support different settings for the possible types of crashes.

The parameter is "crash-debug" with the following syntax:

  crash-debug-<type>=<string>

with <type> being one of:

  panic, hwdom, watchdog, kexeccmd, debugkey

and <string> a sequence of debug key characters with '+' having the
special semantics of a 10 millisecond pause.

So "crash-debug-watchdog=0+0qr" would result in special output in case
of watchdog triggered crash (dom0 state, 10 ms pause, dom0 state,
domain info, run queues).

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- switched special character '.' to '+' (Jan Beulich)
- 10 ms instead of 1 s pause (Jan Beulich)
- added more text to the boot parameter description (Jan Beulich)

V3:
- added const (Jan Beulich)
- thorough test of crash reason parameter (Jan Beulich)
- kexeccmd case should depend on CONFIG_KEXEC (Jan Beulich)
- added dummy get_irq_regs() helper on Arm

V4:
- call keyhandlers with NULL for regs
- use ARRAY_SIZE() (Jan Beulich)
- don't activate handlers in early boot (Jan Beulich)
- avoid recursion
- extend documentation a bit

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/misc/xen-command-line.pandoc | 41 +++++++++++++++++++++++
 xen/common/kexec.c                |  8 +++--
 xen/common/keyhandler.c           | 55 +++++++++++++++++++++++++++++++
 xen/common/shutdown.c             |  4 +--
 xen/drivers/char/console.c        |  2 +-
 xen/include/xen/kexec.h           | 10 ++++--
 xen/include/xen/keyhandler.h      | 10 ++++++
 7 files changed, 122 insertions(+), 8 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index b4a0d60c11..e4c0a144fc 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -574,6 +574,47 @@ reduction of features at Xen's disposal to manage guests.
 ### cpuinfo (x86)
 > `= <boolean>`
 
+### crash-debug-debugkey
+### crash-debug-hwdom
+### crash-debug-kexeccmd
+### crash-debug-panic
+### crash-debug-watchdog
+> `= <string>`
+
+> Can be modified at runtime
+
+Specify debug-key actions in cases of crashes. Each of the parameters applies
+to a different crash reason. The `<string>` is a sequence of debug key
+characters, with `+` having the special meaning of a 10 millisecond pause.
+
+`crash-debug-debugkey` will be used for crashes induced by the `C` debug
+key (i.e. manually induced crash).
+
+`crash-debug-hwdom` denotes a crash of dom0.
+
+`crash-debug-kexeccmd` is an explicit request of dom0 to continue with the
+kdump kernel via kexec. Only available on hypervisors built with CONFIG_KEXEC.
+
+`crash-debug-panic` is a crash of the hypervisor.
+
+`crash-debug-watchdog` is a crash due to the watchdog timer expiring.
+
+It should be noted that dumping diagnosis data to the console can fail in
+multiple ways (missing data, hanging system, ...) depending on the reason
+of the crash, which might have left the hypervisor in a bad state. In case
+a debug-key action leads to another crash recursion will be avoided, so no
+additional debug-key actions will be performed in this case. A crash in the
+early boot phase will not result in any debug-key action, as the system
+might not yet be in a state where the handlers can work.
+
+So e.g. `crash-debug-watchdog=0+0r` would dump dom0 state twice with 10
+milliseconds between the two state dumps, followed by the run queues of the
+hypervisor, if the system crashes due to a watchdog timeout.
+
+Depending on the reason of the system crash it might happen that triggering
+some debug key action will result in a hang instead of dumping data and then
+doing a reboot or crash dump.
+
 ### crashinfo_maxaddr
 > `= <size>`
 
diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index 52cdc4ebc3..ebeee6405a 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -373,10 +373,12 @@ static int kexec_common_shutdown(void)
     return 0;
 }
 
-void kexec_crash(void)
+void kexec_crash(enum crash_reason reason)
 {
     int pos;
 
+    keyhandler_crash_action(reason);
+
     pos = (test_bit(KEXEC_FLAG_CRASH_POS, &kexec_flags) != 0);
     if ( !test_bit(KEXEC_IMAGE_CRASH_BASE + pos, &kexec_flags) )
         return;
@@ -409,7 +411,7 @@ static long kexec_reboot(void *_image)
 static void do_crashdump_trigger(unsigned char key)
 {
     printk("'%c' pressed -> triggering crashdump\n", key);
-    kexec_crash();
+    kexec_crash(CRASHREASON_DEBUGKEY);
     printk(" * no crash kernel loaded!\n");
 }
 
@@ -840,7 +842,7 @@ static int kexec_exec(XEN_GUEST_HANDLE_PARAM(void) uarg)
         ret = continue_hypercall_on_cpu(0, kexec_reboot, image);
         break;
     case KEXEC_TYPE_CRASH:
-        kexec_crash(); /* Does not return */
+        kexec_crash(CRASHREASON_KEXECCMD); /* Does not return */
         break;
     }
 
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index de120fa092..806355ed8b 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -3,7 +3,9 @@
  */
 
 #include <asm/regs.h>
+#include <xen/delay.h>
 #include <xen/keyhandler.h>
+#include <xen/param.h>
 #include <xen/shutdown.h>
 #include <xen/event.h>
 #include <xen/console.h>
@@ -519,6 +521,59 @@ void __init initialize_keytable(void)
     }
 }
 
+#define CRASHACTION_SIZE  32
+static char crash_debug_panic[CRASHACTION_SIZE];
+string_runtime_param("crash-debug-panic", crash_debug_panic);
+static char crash_debug_hwdom[CRASHACTION_SIZE];
+string_runtime_param("crash-debug-hwdom", crash_debug_hwdom);
+static char crash_debug_watchdog[CRASHACTION_SIZE];
+string_runtime_param("crash-debug-watchdog", crash_debug_watchdog);
+#ifdef CONFIG_KEXEC
+static char crash_debug_kexeccmd[CRASHACTION_SIZE];
+string_runtime_param("crash-debug-kexeccmd", crash_debug_kexeccmd);
+#else
+#define crash_debug_kexeccmd NULL
+#endif
+static char crash_debug_debugkey[CRASHACTION_SIZE];
+string_runtime_param("crash-debug-debugkey", crash_debug_debugkey);
+
+void keyhandler_crash_action(enum crash_reason reason)
+{
+    static const char *const crash_action[] = {
+        [CRASHREASON_PANIC] = crash_debug_panic,
+        [CRASHREASON_HWDOM] = crash_debug_hwdom,
+        [CRASHREASON_WATCHDOG] = crash_debug_watchdog,
+        [CRASHREASON_KEXECCMD] = crash_debug_kexeccmd,
+        [CRASHREASON_DEBUGKEY] = crash_debug_debugkey,
+    };
+    static bool ignore;
+    const char *action;
+
+    /* Some handlers are not functional too early. */
+    if ( system_state < SYS_STATE_smp_boot )
+        return;
+
+    /* Avoid recursion. */
+    if ( ignore )
+        return;
+    ignore = true;
+
+    if ( (unsigned int)reason >= ARRAY_SIZE(crash_action) )
+        return;
+    action = crash_action[reason];
+    if ( !action )
+        return;
+
+    while ( *action )
+    {
+        if ( *action == '+' )
+            mdelay(10);
+        else
+            handle_keypress(*action, NULL);
+        action++;
+    }
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c
index 912593915b..abde48aa4c 100644
--- a/xen/common/shutdown.c
+++ b/xen/common/shutdown.c
@@ -43,7 +43,7 @@ void hwdom_shutdown(u8 reason)
     case SHUTDOWN_crash:
         debugger_trap_immediate();
         printk("Hardware Dom%u crashed: ", hardware_domain->domain_id);
-        kexec_crash();
+        kexec_crash(CRASHREASON_HWDOM);
         maybe_reboot();
         break; /* not reached */
 
@@ -56,7 +56,7 @@ void hwdom_shutdown(u8 reason)
     case SHUTDOWN_watchdog:
         printk("Hardware Dom%u shutdown: watchdog rebooting machine\n",
                hardware_domain->domain_id);
-        kexec_crash();
+        kexec_crash(CRASHREASON_WATCHDOG);
         machine_restart(0);
         break; /* not reached */
 
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 861ad53a8f..acec277f5e 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1271,7 +1271,7 @@ void panic(const char *fmt, ...)
 
     debugger_trap_immediate();
 
-    kexec_crash();
+    kexec_crash(CRASHREASON_PANIC);
 
     if ( opt_noreboot )
         machine_halt();
diff --git a/xen/include/xen/kexec.h b/xen/include/xen/kexec.h
index e85ba16405..9f7a912e97 100644
--- a/xen/include/xen/kexec.h
+++ b/xen/include/xen/kexec.h
@@ -1,6 +1,8 @@
 #ifndef __XEN_KEXEC_H__
 #define __XEN_KEXEC_H__
 
+#include <xen/keyhandler.h>
+
 #ifdef CONFIG_KEXEC
 
 #include <public/kexec.h>
@@ -48,7 +50,7 @@ void machine_kexec_unload(struct kexec_image *image);
 void machine_kexec_reserved(xen_kexec_reserve_t *reservation);
 void machine_reboot_kexec(struct kexec_image *image);
 void machine_kexec(struct kexec_image *image);
-void kexec_crash(void);
+void kexec_crash(enum crash_reason reason);
 void kexec_crash_save_cpu(void);
 struct crash_xen_info *kexec_crash_save_info(void);
 void machine_crash_shutdown(void);
@@ -82,7 +84,11 @@ void vmcoreinfo_append_str(const char *fmt, ...)
 #define kexecing 0
 
 static inline void kexec_early_calculations(void) {}
-static inline void kexec_crash(void) {}
+static inline void kexec_crash(enum crash_reason reason)
+{
+    keyhandler_crash_action(reason);
+}
+
 static inline void kexec_crash_save_cpu(void) {}
 static inline void set_kexec_crash_area_size(u64 system_ram) {}
 
diff --git a/xen/include/xen/keyhandler.h b/xen/include/xen/keyhandler.h
index 5131e86cbc..9c5830a037 100644
--- a/xen/include/xen/keyhandler.h
+++ b/xen/include/xen/keyhandler.h
@@ -48,4 +48,14 @@ void register_irq_keyhandler(unsigned char key,
 /* Inject a keypress into the key-handling subsystem. */
 extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs);
 
+enum crash_reason {
+    CRASHREASON_PANIC,
+    CRASHREASON_HWDOM,
+    CRASHREASON_WATCHDOG,
+    CRASHREASON_KEXECCMD,
+    CRASHREASON_DEBUGKEY,
+};
+
+void keyhandler_crash_action(enum crash_reason reason);
+
 #endif /* __XEN_KEYHANDLER_H__ */
-- 
2.26.2

Re: [PATCH v4 1/3] xen/arm: add support for run_in_exception_handler()

[Jan Beulich]

On 14.12.2020 08:56, Juergen Gross wrote:
> Add support to run a function in an exception handler for Arm. Do it
> the same way as on x86 via a bug_frame.
> 
> Unfortunately inline assembly on Arm seems to be less capable than on
> x86, leading to functions called via run_in_exception_handler() having
> to be globally visible.

Could you extend on this? I don't understand what the relevant
difference is, from just looking at the changes.

> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V4:
> - new patch
> 
> I have verified the created bugframe is correct by inspecting the
> created binary.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  xen/arch/arm/traps.c       | 10 +++++++++-
>  xen/drivers/char/ns16550.c |  3 ++-
>  xen/include/asm-arm/bug.h  | 32 +++++++++++++++++++++-----------
>  3 files changed, 32 insertions(+), 13 deletions(-)

Aiui you also need to modify xen.lds.S to cover the new (or really
the last renamed) section.

Jan

Re: [PATCH v4 0/3] xen: add support for automatic debug key actions in case of crash

[Jan Beulich]

On 14.12.2020 08:56, Juergen Gross wrote:
> Patch 2 opens up more potential for simplification: in theory there is
> no need any more to call any key handler with the regs parameter,
> allowing to use the same prototype for all handlers. The downside would
> be to have an additional irq frame on the stack for the dump_registers()
> and the do_debug_key() handlers.

This isn't the only downside, is it? We'd then also need to be able
to (sufficiently cleanly) unwind through the new frame to reach the
prior one, in order to avoid logging less reliable information. Plus
decompose the prior frame as well to avoid logging less easy to
consume data.

Jan

Re: [PATCH v4 1/3] xen/arm: add support for run_in_exception_handler()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1524 bytes --]

On 14.12.20 10:03, Jan Beulich wrote:
> On 14.12.2020 08:56, Juergen Gross wrote:
>> Add support to run a function in an exception handler for Arm. Do it
>> the same way as on x86 via a bug_frame.
>>
>> Unfortunately inline assembly on Arm seems to be less capable than on
>> x86, leading to functions called via run_in_exception_handler() having
>> to be globally visible.
> 
> Could you extend on this? I don't understand what the relevant
> difference is, from just looking at the changes.

The problem seems to be that a static symbol referenced from the inline
asm seems not to silence the error that this static symbol isn't being
used. On x86 the bug_frame is constructed using the %c modifier, which
is not supported for Arm (at least gcc 7 used in my compile test
complained), but seems to be enough for gcc on x86 to not complain.

> 
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V4:
>> - new patch
>>
>> I have verified the created bugframe is correct by inspecting the
>> created binary.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   xen/arch/arm/traps.c       | 10 +++++++++-
>>   xen/drivers/char/ns16550.c |  3 ++-
>>   xen/include/asm-arm/bug.h  | 32 +++++++++++++++++++++-----------
>>   3 files changed, 32 insertions(+), 13 deletions(-)
> 
> Aiui you also need to modify xen.lds.S to cover the new (or really
> the last renamed) section.

Oh, right. I thought of that before, but forgot again. Thanks for the
reminder.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 491 bytes --]

Re: [PATCH v4 3/3] xen: add support for automatic debug key actions in case of crash

[Jan Beulich]

On 14.12.2020 08:56, Juergen Gross wrote:
> @@ -519,6 +521,59 @@ void __init initialize_keytable(void)
>      }
>  }
>  
> +#define CRASHACTION_SIZE  32
> +static char crash_debug_panic[CRASHACTION_SIZE];
> +string_runtime_param("crash-debug-panic", crash_debug_panic);
> +static char crash_debug_hwdom[CRASHACTION_SIZE];
> +string_runtime_param("crash-debug-hwdom", crash_debug_hwdom);
> +static char crash_debug_watchdog[CRASHACTION_SIZE];
> +string_runtime_param("crash-debug-watchdog", crash_debug_watchdog);
> +#ifdef CONFIG_KEXEC
> +static char crash_debug_kexeccmd[CRASHACTION_SIZE];
> +string_runtime_param("crash-debug-kexeccmd", crash_debug_kexeccmd);
> +#else
> +#define crash_debug_kexeccmd NULL
> +#endif
> +static char crash_debug_debugkey[CRASHACTION_SIZE];
> +string_runtime_param("crash-debug-debugkey", crash_debug_debugkey);
> +
> +void keyhandler_crash_action(enum crash_reason reason)
> +{
> +    static const char *const crash_action[] = {
> +        [CRASHREASON_PANIC] = crash_debug_panic,
> +        [CRASHREASON_HWDOM] = crash_debug_hwdom,
> +        [CRASHREASON_WATCHDOG] = crash_debug_watchdog,
> +        [CRASHREASON_KEXECCMD] = crash_debug_kexeccmd,
> +        [CRASHREASON_DEBUGKEY] = crash_debug_debugkey,
> +    };
> +    static bool ignore;
> +    const char *action;
> +
> +    /* Some handlers are not functional too early. */
> +    if ( system_state < SYS_STATE_smp_boot )
> +        return;
> +
> +    /* Avoid recursion. */
> +    if ( ignore )
> +        return;
> +    ignore = true;
> +
> +    if ( (unsigned int)reason >= ARRAY_SIZE(crash_action) )
> +        return;
> +    action = crash_action[reason];
> +    if ( !action )
> +        return;

If we consider either of the last two "return"s to possibly be
taken, I think the "ignore" logic wants to live below them, not
above, avoiding no output at all when a recursive invocation
turns out to be a "good" one. Then, as before,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v4 3/3] xen: add support for automatic debug key actions in case of crash

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2161 bytes --]

On 14.12.20 10:16, Jan Beulich wrote:
> On 14.12.2020 08:56, Juergen Gross wrote:
>> @@ -519,6 +521,59 @@ void __init initialize_keytable(void)
>>       }
>>   }
>>   
>> +#define CRASHACTION_SIZE  32
>> +static char crash_debug_panic[CRASHACTION_SIZE];
>> +string_runtime_param("crash-debug-panic", crash_debug_panic);
>> +static char crash_debug_hwdom[CRASHACTION_SIZE];
>> +string_runtime_param("crash-debug-hwdom", crash_debug_hwdom);
>> +static char crash_debug_watchdog[CRASHACTION_SIZE];
>> +string_runtime_param("crash-debug-watchdog", crash_debug_watchdog);
>> +#ifdef CONFIG_KEXEC
>> +static char crash_debug_kexeccmd[CRASHACTION_SIZE];
>> +string_runtime_param("crash-debug-kexeccmd", crash_debug_kexeccmd);
>> +#else
>> +#define crash_debug_kexeccmd NULL
>> +#endif
>> +static char crash_debug_debugkey[CRASHACTION_SIZE];
>> +string_runtime_param("crash-debug-debugkey", crash_debug_debugkey);
>> +
>> +void keyhandler_crash_action(enum crash_reason reason)
>> +{
>> +    static const char *const crash_action[] = {
>> +        [CRASHREASON_PANIC] = crash_debug_panic,
>> +        [CRASHREASON_HWDOM] = crash_debug_hwdom,
>> +        [CRASHREASON_WATCHDOG] = crash_debug_watchdog,
>> +        [CRASHREASON_KEXECCMD] = crash_debug_kexeccmd,
>> +        [CRASHREASON_DEBUGKEY] = crash_debug_debugkey,
>> +    };
>> +    static bool ignore;
>> +    const char *action;
>> +
>> +    /* Some handlers are not functional too early. */
>> +    if ( system_state < SYS_STATE_smp_boot )
>> +        return;
>> +
>> +    /* Avoid recursion. */
>> +    if ( ignore )
>> +        return;
>> +    ignore = true;
>> +
>> +    if ( (unsigned int)reason >= ARRAY_SIZE(crash_action) )
>> +        return;
>> +    action = crash_action[reason];
>> +    if ( !action )
>> +        return;
> 
> If we consider either of the last two "return"s to possibly be
> taken, I think the "ignore" logic wants to live below them, not
> above, avoiding no output at all when a recursive invocation
> turns out to be a "good" one. Then, as before,
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Fine with me.


Juergen


[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v4 0/3] xen: add support for automatic debug key actions in case of crash

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 803 bytes --]

On 14.12.20 10:09, Jan Beulich wrote:
> On 14.12.2020 08:56, Juergen Gross wrote:
>> Patch 2 opens up more potential for simplification: in theory there is
>> no need any more to call any key handler with the regs parameter,
>> allowing to use the same prototype for all handlers. The downside would
>> be to have an additional irq frame on the stack for the dump_registers()
>> and the do_debug_key() handlers.
> 
> This isn't the only downside, is it? We'd then also need to be able
> to (sufficiently cleanly) unwind through the new frame to reach the
> prior one, in order to avoid logging less reliable information. Plus
> decompose the prior frame as well to avoid logging less easy to
> consume data.

Yes, this was implied by the "additional irq frame on the stack".


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v4 1/3] xen/arm: add support for run_in_exception_handler()

[Jan Beulich]

On 14.12.2020 10:15, Jürgen Groß wrote:
> On 14.12.20 10:03, Jan Beulich wrote:
>> On 14.12.2020 08:56, Juergen Gross wrote:
>>> Add support to run a function in an exception handler for Arm. Do it
>>> the same way as on x86 via a bug_frame.
>>>
>>> Unfortunately inline assembly on Arm seems to be less capable than on
>>> x86, leading to functions called via run_in_exception_handler() having
>>> to be globally visible.
>>
>> Could you extend on this? I don't understand what the relevant
>> difference is, from just looking at the changes.
> 
> The problem seems to be that a static symbol referenced from the inline
> asm seems not to silence the error that this static symbol isn't being
> used. On x86 the bug_frame is constructed using the %c modifier, which
> is not supported for Arm (at least gcc 7 used in my compile test
> complained), but seems to be enough for gcc on x86 to not complain.

But this isn't tied to %c not working on older gcc, is it? It looks
instead to be tied to you not specifying the function pointer as an
input in the asm(). The compiler would know the symbol is referenced
even if the input wasn't used at all in any of the asm()'s operands
(but of course it would be better to use the operand once you have
it, if it can be used in some way despite %c not being possible to
use).

Jan

Re: [PATCH v4 0/3] xen: add support for automatic debug key actions in case of crash

[Jan Beulich]

On 14.12.2020 10:21, Jürgen Groß wrote:
> On 14.12.20 10:09, Jan Beulich wrote:
>> On 14.12.2020 08:56, Juergen Gross wrote:
>>> Patch 2 opens up more potential for simplification: in theory there is
>>> no need any more to call any key handler with the regs parameter,
>>> allowing to use the same prototype for all handlers. The downside would
>>> be to have an additional irq frame on the stack for the dump_registers()
>>> and the do_debug_key() handlers.
>>
>> This isn't the only downside, is it? We'd then also need to be able
>> to (sufficiently cleanly) unwind through the new frame to reach the
>> prior one, in order to avoid logging less reliable information. Plus
>> decompose the prior frame as well to avoid logging less easy to
>> consume data.
> 
> Yes, this was implied by the "additional irq frame on the stack".

Oh, okay - I read it as just referring to the possible concern of
more of the not overly large stack to get used.

Jan

Re: [PATCH v4 1/3] xen/arm: add support for run_in_exception_handler()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 1454 bytes --]

On 14.12.20 10:22, Jan Beulich wrote:
> On 14.12.2020 10:15, Jürgen Groß wrote:
>> On 14.12.20 10:03, Jan Beulich wrote:
>>> On 14.12.2020 08:56, Juergen Gross wrote:
>>>> Add support to run a function in an exception handler for Arm. Do it
>>>> the same way as on x86 via a bug_frame.
>>>>
>>>> Unfortunately inline assembly on Arm seems to be less capable than on
>>>> x86, leading to functions called via run_in_exception_handler() having
>>>> to be globally visible.
>>>
>>> Could you extend on this? I don't understand what the relevant
>>> difference is, from just looking at the changes.
>>
>> The problem seems to be that a static symbol referenced from the inline
>> asm seems not to silence the error that this static symbol isn't being
>> used. On x86 the bug_frame is constructed using the %c modifier, which
>> is not supported for Arm (at least gcc 7 used in my compile test
>> complained), but seems to be enough for gcc on x86 to not complain.
> 
> But this isn't tied to %c not working on older gcc, is it? It looks
> instead to be tied to you not specifying the function pointer as an
> input in the asm(). The compiler would know the symbol is referenced
> even if the input wasn't used at all in any of the asm()'s operands
> (but of course it would be better to use the operand once you have
> it, if it can be used in some way despite %c not being possible to
> use).

Let me check that.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v4 1/3] xen/arm: add support for run_in_exception_handler()

[Julien Grall]

Hi Juergen,

On 14/12/2020 07:56, Juergen Gross wrote:
> Add support to run a function in an exception handler for Arm. Do it
> the same way as on x86 via a bug_frame.
> 
> Unfortunately inline assembly on Arm seems to be less capable than on
> x86, leading to functions called via run_in_exception_handler() having
> to be globally visible.

Jan already commented on this, so I am not going to comment again.

> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V4:
> - new patch
> 
> I have verified the created bugframe is correct by inspecting the
> created binary.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>   xen/arch/arm/traps.c       | 10 +++++++++-
>   xen/drivers/char/ns16550.c |  3 ++-
>   xen/include/asm-arm/bug.h  | 32 +++++++++++++++++++++-----------
>   3 files changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 22bd1bd4c6..6e677affe2 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1236,8 +1236,16 @@ int do_bug_frame(const struct cpu_user_regs *regs, vaddr_t pc)
>       if ( !bug )
>           return -ENOENT;
>   
> +    if ( id == BUGFRAME_run_fn )
> +    {
> +        void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> +
> +        fn(regs);
> +        return 0;
> +    }
> +
>       /* WARN, BUG or ASSERT: decode the filename pointer and line number. */
> -    filename = bug_file(bug);
> +    filename = bug_ptr(bug);
>       if ( !is_kernel(filename) )
>           return -EINVAL;
>       fixup = strlen(filename);
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 9235d854fe..dd6500acc8 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -192,7 +192,8 @@ static void ns16550_interrupt(
>   /* Safe: ns16550_poll() runs as softirq so not reentrant on a given CPU. */
>   static DEFINE_PER_CPU(struct serial_port *, poll_port);
>   
> -static void __ns16550_poll(struct cpu_user_regs *regs)
> +/* run_in_exception_handler() on Arm requires globally visible symbol. */
> +void __ns16550_poll(struct cpu_user_regs *regs)
>   {
>       struct serial_port *port = this_cpu(poll_port);
>       struct ns16550 *uart = port->uart;
> diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h
> index 36c803357c..a7da2c306f 100644
> --- a/xen/include/asm-arm/bug.h
> +++ b/xen/include/asm-arm/bug.h
> @@ -15,34 +15,38 @@
>   
>   struct bug_frame {
>       signed int loc_disp;    /* Relative address to the bug address */
> -    signed int file_disp;   /* Relative address to the filename */
> +    signed int ptr_disp;    /* Relative address to the filename or function */
>       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_ptr(b) ((const void *)(b) + (b)->ptr_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
> +#define BUGFRAME_run_fn 0
> +#define BUGFRAME_warn   1
> +#define BUGFRAME_bug    2
> +#define BUGFRAME_assert 3

Why did you renumber it? IOW, why can't BUGFRAME_run_fn be defined as 3?

>   
> -#define BUGFRAME_NR     3
> +#define BUGFRAME_NR     4
>   
>   /* Many versions 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 {                      \
> +#define BUG_FRAME(type, line, ptr, ptr_is_file, has_msg, msg) do {          \
>       BUILD_BUG_ON((line) >> 16);                                             \
>       BUILD_BUG_ON((type) >= BUGFRAME_NR);                                    \
>       asm ("1:"BUG_INSTR"\n"                                                  \
>            ".pushsection .rodata.str, \"aMS\", %progbits, 1\n"                \
> -         "2:\t.asciz " __stringify(file) "\n"                               \
> +         "2:\n"                                                             \
> +         ".if " #ptr_is_file "\n"                                           \
> +         "\t.asciz " __stringify(ptr) "\n"                                  \
> +         ".endif\n"                                                         \
>            "3:\n"                                                             \
>            ".if " #has_msg "\n"                                               \
>            "\t.asciz " #msg "\n"                                              \
> @@ -52,21 +56,27 @@ struct bug_frame {
>            "4:\n"                                                             \
>            ".p2align 2\n"                                                     \
>            ".long (1b - 4b)\n"                                                \
> +         ".if " #ptr_is_file "\n"                                           \
>            ".long (2b - 4b)\n"                                                \
> +         ".else\n"                                                          \
> +         ".long (" #ptr " - 4b)\n"                                          \
> +         ".endif\n"                                                         \
>            ".long (3b - 4b)\n"                                                \
>            ".hword " __stringify(line) ", 0\n"                                \
>            ".popsection");                                                    \
>   } while (0)
>   
> -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")
> +#define run_in_exception_handler(fn) BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, 0, "")
> +
> +#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 1, 0, "")
>   
>   #define BUG() do {                                              \
> -    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, "");        \
> +    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 1, 0, "");        \
>       unreachable();                                              \
>   } while (0)
>   
>   #define assert_failed(msg) do {                                 \
> -    BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg);     \
> +    BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, 1, msg);     \
>       unreachable();                                              \
>   } while (0)
>   
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v4 3/3] xen: add support for automatic debug key actions in case of crash

[Julien Grall]

Hi Juergen,

On 14/12/2020 07:56, Juergen Gross wrote:
> diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> index de120fa092..806355ed8b 100644
> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -3,7 +3,9 @@
>    */
>   
>   #include <asm/regs.h>
> +#include <xen/delay.h>
>   #include <xen/keyhandler.h>
> +#include <xen/param.h>
>   #include <xen/shutdown.h>
>   #include <xen/event.h>
>   #include <xen/console.h>
> @@ -519,6 +521,59 @@ void __init initialize_keytable(void)
>       }
>   }
>   
> +#define CRASHACTION_SIZE  32
> +static char crash_debug_panic[CRASHACTION_SIZE];
> +string_runtime_param("crash-debug-panic", crash_debug_panic);
> +static char crash_debug_hwdom[CRASHACTION_SIZE];
> +string_runtime_param("crash-debug-hwdom", crash_debug_hwdom);
> +static char crash_debug_watchdog[CRASHACTION_SIZE];
> +string_runtime_param("crash-debug-watchdog", crash_debug_watchdog);
> +#ifdef CONFIG_KEXEC
> +static char crash_debug_kexeccmd[CRASHACTION_SIZE];
> +string_runtime_param("crash-debug-kexeccmd", crash_debug_kexeccmd);
> +#else
> +#define crash_debug_kexeccmd NULL
> +#endif
> +static char crash_debug_debugkey[CRASHACTION_SIZE];
> +string_runtime_param("crash-debug-debugkey", crash_debug_debugkey);
> +
> +void keyhandler_crash_action(enum crash_reason reason)
> +{
> +    static const char *const crash_action[] = {
> +        [CRASHREASON_PANIC] = crash_debug_panic,
> +        [CRASHREASON_HWDOM] = crash_debug_hwdom,
> +        [CRASHREASON_WATCHDOG] = crash_debug_watchdog,
> +        [CRASHREASON_KEXECCMD] = crash_debug_kexeccmd,
> +        [CRASHREASON_DEBUGKEY] = crash_debug_debugkey,
> +    };
> +    static bool ignore;
> +    const char *action;
> +
> +    /* Some handlers are not functional too early. */

Can you explain in the commit message why this is necessary (An example 
would be useful)?

Cheers,

-- 
Julien Grall

Re: [PATCH v4 1/3] xen/arm: add support for run_in_exception_handler()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 5288 bytes --]

On 14.12.20 11:17, Julien Grall wrote:
> Hi Juergen,
> 
> On 14/12/2020 07:56, Juergen Gross wrote:
>> Add support to run a function in an exception handler for Arm. Do it
>> the same way as on x86 via a bug_frame.
>>
>> Unfortunately inline assembly on Arm seems to be less capable than on
>> x86, leading to functions called via run_in_exception_handler() having
>> to be globally visible.
> 
> Jan already commented on this, so I am not going to comment again.

Maybe I can ask some Arm specific question related to this:

In my experiments the only working solution was using the "i" constraint
for the function pointer. Do you know whether this is supported for all
gcc versions we care about?

Or is there another way to achieve the desired functionality? I'm using
now the following macros:

#define BUG_FRAME_run_fn(fn) do {                                      \
     asm ("1:"BUG_INSTR"\n"                                             \
          ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn)      \
                        ", \"a\", %%progbits\n"                         \
          "2:\n"                                                        \
          ".p2align 2\n"                                                \
          ".long (1b - 2b)\n"                                           \
          ".long (%0 - 2b)\n"                                           \
          ".long 0\n"                                                   \
          ".hword 0, 0\n"                                               \
          ".popsection" :: "i" (fn));                                   \
} while (0)

#define run_in_exception_handler(fn) BUG_FRAME_run_fn(fn)

> 
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V4:
>> - new patch
>>
>> I have verified the created bugframe is correct by inspecting the
>> created binary.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   xen/arch/arm/traps.c       | 10 +++++++++-
>>   xen/drivers/char/ns16550.c |  3 ++-
>>   xen/include/asm-arm/bug.h  | 32 +++++++++++++++++++++-----------
>>   3 files changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 22bd1bd4c6..6e677affe2 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1236,8 +1236,16 @@ int do_bug_frame(const struct cpu_user_regs 
>> *regs, vaddr_t pc)
>>       if ( !bug )
>>           return -ENOENT;
>> +    if ( id == BUGFRAME_run_fn )
>> +    {
>> +        void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
>> +
>> +        fn(regs);
>> +        return 0;
>> +    }
>> +
>>       /* WARN, BUG or ASSERT: decode the filename pointer and line 
>> number. */
>> -    filename = bug_file(bug);
>> +    filename = bug_ptr(bug);
>>       if ( !is_kernel(filename) )
>>           return -EINVAL;
>>       fixup = strlen(filename);
>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>> index 9235d854fe..dd6500acc8 100644
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -192,7 +192,8 @@ static void ns16550_interrupt(
>>   /* Safe: ns16550_poll() runs as softirq so not reentrant on a given 
>> CPU. */
>>   static DEFINE_PER_CPU(struct serial_port *, poll_port);
>> -static void __ns16550_poll(struct cpu_user_regs *regs)
>> +/* run_in_exception_handler() on Arm requires globally visible 
>> symbol. */
>> +void __ns16550_poll(struct cpu_user_regs *regs)
>>   {
>>       struct serial_port *port = this_cpu(poll_port);
>>       struct ns16550 *uart = port->uart;
>> diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h
>> index 36c803357c..a7da2c306f 100644
>> --- a/xen/include/asm-arm/bug.h
>> +++ b/xen/include/asm-arm/bug.h
>> @@ -15,34 +15,38 @@
>>   struct bug_frame {
>>       signed int loc_disp;    /* Relative address to the bug address */
>> -    signed int file_disp;   /* Relative address to the filename */
>> +    signed int ptr_disp;    /* Relative address to the filename or 
>> function */
>>       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_ptr(b) ((const void *)(b) + (b)->ptr_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
>> +#define BUGFRAME_run_fn 0
>> +#define BUGFRAME_warn   1
>> +#define BUGFRAME_bug    2
>> +#define BUGFRAME_assert 3
> 
> Why did you renumber it? IOW, why can't BUGFRAME_run_fn be defined as 3?

This matches x86 definition. IMO there is no reason to have a different
definition and this will make it more obvious that it might be a good
idea to have a common include/xen/bug.h header.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v4 3/3] xen: add support for automatic debug key actions in case of crash

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 2091 bytes --]

On 14.12.20 11:24, Julien Grall wrote:
> Hi Juergen,
> 
> On 14/12/2020 07:56, Juergen Gross wrote:
>> diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
>> index de120fa092..806355ed8b 100644
>> --- a/xen/common/keyhandler.c
>> +++ b/xen/common/keyhandler.c
>> @@ -3,7 +3,9 @@
>>    */
>>   #include <asm/regs.h>
>> +#include <xen/delay.h>
>>   #include <xen/keyhandler.h>
>> +#include <xen/param.h>
>>   #include <xen/shutdown.h>
>>   #include <xen/event.h>
>>   #include <xen/console.h>
>> @@ -519,6 +521,59 @@ void __init initialize_keytable(void)
>>       }
>>   }
>> +#define CRASHACTION_SIZE  32
>> +static char crash_debug_panic[CRASHACTION_SIZE];
>> +string_runtime_param("crash-debug-panic", crash_debug_panic);
>> +static char crash_debug_hwdom[CRASHACTION_SIZE];
>> +string_runtime_param("crash-debug-hwdom", crash_debug_hwdom);
>> +static char crash_debug_watchdog[CRASHACTION_SIZE];
>> +string_runtime_param("crash-debug-watchdog", crash_debug_watchdog);
>> +#ifdef CONFIG_KEXEC
>> +static char crash_debug_kexeccmd[CRASHACTION_SIZE];
>> +string_runtime_param("crash-debug-kexeccmd", crash_debug_kexeccmd);
>> +#else
>> +#define crash_debug_kexeccmd NULL
>> +#endif
>> +static char crash_debug_debugkey[CRASHACTION_SIZE];
>> +string_runtime_param("crash-debug-debugkey", crash_debug_debugkey);
>> +
>> +void keyhandler_crash_action(enum crash_reason reason)
>> +{
>> +    static const char *const crash_action[] = {
>> +        [CRASHREASON_PANIC] = crash_debug_panic,
>> +        [CRASHREASON_HWDOM] = crash_debug_hwdom,
>> +        [CRASHREASON_WATCHDOG] = crash_debug_watchdog,
>> +        [CRASHREASON_KEXECCMD] = crash_debug_kexeccmd,
>> +        [CRASHREASON_DEBUGKEY] = crash_debug_debugkey,
>> +    };
>> +    static bool ignore;
>> +    const char *action;
>> +
>> +    /* Some handlers are not functional too early. */
> 
> Can you explain in the commit message why this is necessary (An example 
> would be useful)?

Okay.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v4 1/3] xen/arm: add support for run_in_exception_handler()

[Julien Grall]

Hi Juergen,

On 14/12/2020 10:51, Jürgen Groß wrote:
> On 14.12.20 11:17, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 14/12/2020 07:56, Juergen Gross wrote:
>>> Add support to run a function in an exception handler for Arm. Do it
>>> the same way as on x86 via a bug_frame.
>>>
>>> Unfortunately inline assembly on Arm seems to be less capable than on
>>> x86, leading to functions called via run_in_exception_handler() having
>>> to be globally visible.
>>
>> Jan already commented on this, so I am not going to comment again.
> 
> Maybe I can ask some Arm specific question related to this:
> 
> In my experiments the only working solution was using the "i" constraint
> for the function pointer. Do you know whether this is supported for all
> gcc versions we care about?

I don't know for sure. However, Linux has been using "i" since 2012. So 
I would assume it ought to be fine for all the version we care.

> 
> Or is there another way to achieve the desired functionality? I'm using
> now the following macros:
> 
> #define BUG_FRAME_run_fn(fn) do {                                      \
>      asm ("1:"BUG_INSTR"\n"                                             \
>           ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn)      \
>                         ", \"a\", %%progbits\n"                         \
>           "2:\n"                                                        \
>           ".p2align 2\n"                                                \
>           ".long (1b - 2b)\n"                                           \
>           ".long (%0 - 2b)\n"                                           \
>           ".long 0\n"                                                   \
>           ".hword 0, 0\n"                                               \
>           ".popsection" :: "i" (fn));                                   \
> } while (0)

May I ask why we need a new macro?

> 
> #define run_in_exception_handler(fn) BUG_FRAME_run_fn(fn)
> 
>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V4:
>>> - new patch
>>>
>>> I have verified the created bugframe is correct by inspecting the
>>> created binary.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>   xen/arch/arm/traps.c       | 10 +++++++++-
>>>   xen/drivers/char/ns16550.c |  3 ++-
>>>   xen/include/asm-arm/bug.h  | 32 +++++++++++++++++++++-----------
>>>   3 files changed, 32 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index 22bd1bd4c6..6e677affe2 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -1236,8 +1236,16 @@ int do_bug_frame(const struct cpu_user_regs 
>>> *regs, vaddr_t pc)
>>>       if ( !bug )
>>>           return -ENOENT;
>>> +    if ( id == BUGFRAME_run_fn )
>>> +    {
>>> +        void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
>>> +
>>> +        fn(regs);
>>> +        return 0;
>>> +    }
>>> +
>>>       /* WARN, BUG or ASSERT: decode the filename pointer and line 
>>> number. */
>>> -    filename = bug_file(bug);
>>> +    filename = bug_ptr(bug);
>>>       if ( !is_kernel(filename) )
>>>           return -EINVAL;
>>>       fixup = strlen(filename);
>>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>>> index 9235d854fe..dd6500acc8 100644
>>> --- a/xen/drivers/char/ns16550.c
>>> +++ b/xen/drivers/char/ns16550.c
>>> @@ -192,7 +192,8 @@ static void ns16550_interrupt(
>>>   /* Safe: ns16550_poll() runs as softirq so not reentrant on a given 
>>> CPU. */
>>>   static DEFINE_PER_CPU(struct serial_port *, poll_port);
>>> -static void __ns16550_poll(struct cpu_user_regs *regs)
>>> +/* run_in_exception_handler() on Arm requires globally visible 
>>> symbol. */
>>> +void __ns16550_poll(struct cpu_user_regs *regs)
>>>   {
>>>       struct serial_port *port = this_cpu(poll_port);
>>>       struct ns16550 *uart = port->uart;
>>> diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h
>>> index 36c803357c..a7da2c306f 100644
>>> --- a/xen/include/asm-arm/bug.h
>>> +++ b/xen/include/asm-arm/bug.h
>>> @@ -15,34 +15,38 @@
>>>   struct bug_frame {
>>>       signed int loc_disp;    /* Relative address to the bug address */
>>> -    signed int file_disp;   /* Relative address to the filename */
>>> +    signed int ptr_disp;    /* Relative address to the filename or 
>>> function */
>>>       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_ptr(b) ((const void *)(b) + (b)->ptr_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
>>> +#define BUGFRAME_run_fn 0
>>> +#define BUGFRAME_warn   1
>>> +#define BUGFRAME_bug    2
>>> +#define BUGFRAME_assert 3
>>
>> Why did you renumber it? IOW, why can't BUGFRAME_run_fn be defined as 3?
> 
> This matches x86 definition. IMO there is no reason to have a different
> definition and this will make it more obvious that it might be a good
> idea to have a common include/xen/bug.h header.

I agree that common header would be nice. Although, I am not sure if 
this is achievable. However, my point here is this change would have 
deserved half-sentence in the commit message because to me this look 
like unwanted churn.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 1/3] xen/arm: add support for run_in_exception_handler()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 6926 bytes --]

On 14.12.20 12:14, Julien Grall wrote:
> Hi Juergen,
> 
> On 14/12/2020 10:51, Jürgen Groß wrote:
>> On 14.12.20 11:17, Julien Grall wrote:
>>> Hi Juergen,
>>>
>>> On 14/12/2020 07:56, Juergen Gross wrote:
>>>> Add support to run a function in an exception handler for Arm. Do it
>>>> the same way as on x86 via a bug_frame.
>>>>
>>>> Unfortunately inline assembly on Arm seems to be less capable than on
>>>> x86, leading to functions called via run_in_exception_handler() having
>>>> to be globally visible.
>>>
>>> Jan already commented on this, so I am not going to comment again.
>>
>> Maybe I can ask some Arm specific question related to this:
>>
>> In my experiments the only working solution was using the "i" constraint
>> for the function pointer. Do you know whether this is supported for all
>> gcc versions we care about?
> 
> I don't know for sure. However, Linux has been using "i" since 2012. So 
> I would assume it ought to be fine for all the version we care.
> 
>>
>> Or is there another way to achieve the desired functionality? I'm using
>> now the following macros:
>>
>> #define BUG_FRAME_run_fn(fn) do {                                      \
>>      asm ("1:"BUG_INSTR"\n"                                             \
>>           ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn)      \
>>                         ", \"a\", %%progbits\n"                         \
>>           "2:\n"                                                        \
>>           ".p2align 2\n"                                                \
>>           ".long (1b - 2b)\n"                                           \
>>           ".long (%0 - 2b)\n"                                           \
>>           ".long 0\n"                                                   \
>>           ".hword 0, 0\n"                                               \
>>           ".popsection" :: "i" (fn));                                   \
>> } while (0)
> 
> May I ask why we need a new macro?

Using a common one might be possible, but not with the current way how
BUG_FRAME() is defined: gcc complained about the input parameter in case
of ASSERT() and WARN().

I might be missing something, but this was the fastest way to at least
confirm the scheme is working for Arm.

> 
>>
>> #define run_in_exception_handler(fn) BUG_FRAME_run_fn(fn)
>>
>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>> V4:
>>>> - new patch
>>>>
>>>> I have verified the created bugframe is correct by inspecting the
>>>> created binary.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>>   xen/arch/arm/traps.c       | 10 +++++++++-
>>>>   xen/drivers/char/ns16550.c |  3 ++-
>>>>   xen/include/asm-arm/bug.h  | 32 +++++++++++++++++++++-----------
>>>>   3 files changed, 32 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index 22bd1bd4c6..6e677affe2 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -1236,8 +1236,16 @@ int do_bug_frame(const struct cpu_user_regs 
>>>> *regs, vaddr_t pc)
>>>>       if ( !bug )
>>>>           return -ENOENT;
>>>> +    if ( id == BUGFRAME_run_fn )
>>>> +    {
>>>> +        void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
>>>> +
>>>> +        fn(regs);
>>>> +        return 0;
>>>> +    }
>>>> +
>>>>       /* WARN, BUG or ASSERT: decode the filename pointer and line 
>>>> number. */
>>>> -    filename = bug_file(bug);
>>>> +    filename = bug_ptr(bug);
>>>>       if ( !is_kernel(filename) )
>>>>           return -EINVAL;
>>>>       fixup = strlen(filename);
>>>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>>>> index 9235d854fe..dd6500acc8 100644
>>>> --- a/xen/drivers/char/ns16550.c
>>>> +++ b/xen/drivers/char/ns16550.c
>>>> @@ -192,7 +192,8 @@ static void ns16550_interrupt(
>>>>   /* Safe: ns16550_poll() runs as softirq so not reentrant on a 
>>>> given CPU. */
>>>>   static DEFINE_PER_CPU(struct serial_port *, poll_port);
>>>> -static void __ns16550_poll(struct cpu_user_regs *regs)
>>>> +/* run_in_exception_handler() on Arm requires globally visible 
>>>> symbol. */
>>>> +void __ns16550_poll(struct cpu_user_regs *regs)
>>>>   {
>>>>       struct serial_port *port = this_cpu(poll_port);
>>>>       struct ns16550 *uart = port->uart;
>>>> diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h
>>>> index 36c803357c..a7da2c306f 100644
>>>> --- a/xen/include/asm-arm/bug.h
>>>> +++ b/xen/include/asm-arm/bug.h
>>>> @@ -15,34 +15,38 @@
>>>>   struct bug_frame {
>>>>       signed int loc_disp;    /* Relative address to the bug address */
>>>> -    signed int file_disp;   /* Relative address to the filename */
>>>> +    signed int ptr_disp;    /* Relative address to the filename or 
>>>> function */
>>>>       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_ptr(b) ((const void *)(b) + (b)->ptr_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
>>>> +#define BUGFRAME_run_fn 0
>>>> +#define BUGFRAME_warn   1
>>>> +#define BUGFRAME_bug    2
>>>> +#define BUGFRAME_assert 3
>>>
>>> Why did you renumber it? IOW, why can't BUGFRAME_run_fn be defined as 3?
>>
>> This matches x86 definition. IMO there is no reason to have a different
>> definition and this will make it more obvious that it might be a good
>> idea to have a common include/xen/bug.h header.
> 
> I agree that common header would be nice. Although, I am not sure if 
> this is achievable. However, my point here is this change would have 
> deserved half-sentence in the commit message because to me this look 
> like unwanted churn.

Okay.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH v4 1/3] xen/arm: add support for run_in_exception_handler()

[Julien Grall]

Hi Juergen,

On 14/12/2020 11:21, Jürgen Groß wrote:
> On 14.12.20 12:14, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 14/12/2020 10:51, Jürgen Groß wrote:
>>> On 14.12.20 11:17, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> On 14/12/2020 07:56, Juergen Gross wrote:
>>>>> Add support to run a function in an exception handler for Arm. Do it
>>>>> the same way as on x86 via a bug_frame.
>>>>>
>>>>> Unfortunately inline assembly on Arm seems to be less capable than on
>>>>> x86, leading to functions called via run_in_exception_handler() having
>>>>> to be globally visible.
>>>>
>>>> Jan already commented on this, so I am not going to comment again.
>>>
>>> Maybe I can ask some Arm specific question related to this:
>>>
>>> In my experiments the only working solution was using the "i" constraint
>>> for the function pointer. Do you know whether this is supported for all
>>> gcc versions we care about?
>>
>> I don't know for sure. However, Linux has been using "i" since 2012. 
>> So I would assume it ought to be fine for all the version we care.
>>
>>>
>>> Or is there another way to achieve the desired functionality? I'm using
>>> now the following macros:
>>>
>>> #define BUG_FRAME_run_fn(fn) do {                                      \
>>>      asm 
>>> ("1:"BUG_INSTR"\n"                                             \
>>>           ".pushsection .bug_frames." 
>>> __stringify(BUGFRAME_run_fn)      \
>>>                         ", \"a\", 
>>> %%progbits\n"                         \
>>>           
>>> "2:\n"                                                        \
>>>           ".p2align 
>>> 2\n"                                                \
>>>           ".long (1b - 
>>> 2b)\n"                                           \
>>>           ".long (%0 - 
>>> 2b)\n"                                           \
>>>           ".long 
>>> 0\n"                                                   \
>>>           ".hword 0, 
>>> 0\n"                                               \
>>>           ".popsection" :: "i" 
>>> (fn));                                   \
>>> } while (0)
>>
>> May I ask why we need a new macro?
> 
> Using a common one might be possible, but not with the current way how
> BUG_FRAME() is defined: gcc complained about the input parameter in case
> of ASSERT() and WARN().

Could you share the code and the error message?

> 
> I might be missing something, but this was the fastest way to at least
> confirm the scheme is working for Arm.

Make senses. I also don't have much time to invest in trying to have a 
common macro. So I am happy with a new macro.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 1/3] xen/arm: add support for run_in_exception_handler()

[Jürgen Groß]

[-- Attachment #1.1.1: Type: text/plain, Size: 4080 bytes --]

On 14.12.20 12:21, Jürgen Groß wrote:
> On 14.12.20 12:14, Julien Grall wrote:
>> Hi Juergen,
>>
>> On 14/12/2020 10:51, Jürgen Groß wrote:
>>> On 14.12.20 11:17, Julien Grall wrote:
>>>> Hi Juergen,
>>>>
>>>> On 14/12/2020 07:56, Juergen Gross wrote:
>>>>> Add support to run a function in an exception handler for Arm. Do it
>>>>> the same way as on x86 via a bug_frame.
>>>>>
>>>>> Unfortunately inline assembly on Arm seems to be less capable than on
>>>>> x86, leading to functions called via run_in_exception_handler() having
>>>>> to be globally visible.
>>>>
>>>> Jan already commented on this, so I am not going to comment again.
>>>
>>> Maybe I can ask some Arm specific question related to this:
>>>
>>> In my experiments the only working solution was using the "i" constraint
>>> for the function pointer. Do you know whether this is supported for all
>>> gcc versions we care about?
>>
>> I don't know for sure. However, Linux has been using "i" since 2012. 
>> So I would assume it ought to be fine for all the version we care.
>>
>>>
>>> Or is there another way to achieve the desired functionality? I'm using
>>> now the following macros:
>>>
>>> #define BUG_FRAME_run_fn(fn) do {                                      \
>>>      asm 
>>> ("1:"BUG_INSTR"\n"                                             \
>>>           ".pushsection .bug_frames." 
>>> __stringify(BUGFRAME_run_fn)      \
>>>                         ", \"a\", 
>>> %%progbits\n"                         \
>>>           
>>> "2:\n"                                                        \
>>>           ".p2align 
>>> 2\n"                                                \
>>>           ".long (1b - 
>>> 2b)\n"                                           \
>>>           ".long (%0 - 
>>> 2b)\n"                                           \
>>>           ".long 
>>> 0\n"                                                   \
>>>           ".hword 0, 
>>> 0\n"                                               \
>>>           ".popsection" :: "i" 
>>> (fn));                                   \
>>> } while (0)
>>
>> May I ask why we need a new macro?
> 
> Using a common one might be possible, but not with the current way how
> BUG_FRAME() is defined: gcc complained about the input parameter in case
> of ASSERT() and WARN().
> 
> I might be missing something, but this was the fastest way to at least
> confirm the scheme is working for Arm.

Okay, I think I have found a way to use a common macro, which seems to
be even more simple than the original one:

#define BUG_FRAME(type, line, ptr, msg) do {                        \
     BUILD_BUG_ON((line) >> 16);                                     \
     BUILD_BUG_ON((type) >= BUGFRAME_NR);                            \
     asm ("1:"BUG_INSTR"\n"                                          \
          ".pushsection .bug_frames." __stringify(type)              \
                        ", \"a\", %%progbits\n"                      \
          "2:\n"                                                     \
          ".p2align 2\n"                                             \
          ".long (1b - 2b)\n"                                        \
          ".long (%0 - 2b)\n"                                        \
          ".long (%1 - 2b)\n"                                        \
          ".hword " __stringify(line) ", 0\n"                        \
          ".popsection" :: "i" (ptr), "i" (msg)); i                  \
} while (0)


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]