1. Patchew
  2. Xen
  3. View series

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

Juergen Gross posted 1 patch 3 years, 4 months ago
Test gitlab-ci passed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20201126080340.6154-1-jgross@suse.com
There is a newer version of this series
docs/misc/xen-command-line.pandoc | 39 ++++++++++++++++++++++++++
xen/common/kexec.c                |  8 ++++--
xen/common/keyhandler.c           | 46 +++++++++++++++++++++++++++++++
xen/common/shutdown.c             |  4 +--
xen/drivers/char/console.c        |  2 +-
xen/include/asm-arm/irq.h         |  5 ++++
xen/include/xen/kexec.h           | 10 +++++--
xen/include/xen/keyhandler.h      | 11 ++++++++
8 files changed, 117 insertions(+), 8 deletions(-)
[PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Juergen Gross 3 years, 4 months ago 
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

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 docs/misc/xen-command-line.pandoc | 39 ++++++++++++++++++++++++++
 xen/common/kexec.c                |  8 ++++--
 xen/common/keyhandler.c           | 46 +++++++++++++++++++++++++++++++
 xen/common/shutdown.c             |  4 +--
 xen/drivers/char/console.c        |  2 +-
 xen/include/asm-arm/irq.h         |  5 ++++
 xen/include/xen/kexec.h           | 10 +++++--
 xen/include/xen/keyhandler.h      | 11 ++++++++
 8 files changed, 117 insertions(+), 8 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index b4a0d60c11..294be31abb 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -574,6 +574,45 @@ 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.
+
+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.
+
+These parameters should be used carefully, as e.g. specifying
+`crash-debug-debugkey=C` would result in an endless loop. 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 68364e987d..11ab6ecd59 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>
@@ -507,6 +509,50 @@ 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);
+#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_N] = {
+        [CRASHREASON_PANIC] = crash_debug_panic,
+        [CRASHREASON_HWDOM] = crash_debug_hwdom,
+        [CRASHREASON_WATCHDOG] = crash_debug_watchdog,
+#ifdef CONFIG_KEXEC
+        [CRASHREASON_KEXECCMD] = crash_debug_kexeccmd,
+#endif
+        [CRASHREASON_DEBUGKEY] = crash_debug_debugkey,
+    };
+    const char *action;
+    struct cpu_user_regs *regs = get_irq_regs() ? : guest_cpu_user_regs();
+
+    if ( (unsigned int)reason >= CRASHREASON_N )
+        return;
+    action = crash_action[reason];
+    if ( !action )
+        return;
+
+    while ( *action )
+    {
+        if ( *action == '+' )
+            mdelay(10);
+        else
+            handle_keypress(*action, regs);
+        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/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index e45d574598..01d5a7cb02 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -98,6 +98,11 @@ void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
  */
 bool irq_type_set_by_domain(const struct domain *d);
 
+static inline struct cpu_user_regs *get_irq_regs(void)
+{
+    return NULL;
+}
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:
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..dbf797a8b4 100644
--- a/xen/include/xen/keyhandler.h
+++ b/xen/include/xen/keyhandler.h
@@ -48,4 +48,15 @@ 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,
+    CRASHREASON_N
+};
+
+void keyhandler_crash_action(enum crash_reason reason);
+
 #endif /* __XEN_KEYHANDLER_H__ */
-- 
2.26.2
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Jan Beulich 3 years, 4 months ago 
On 26.11.2020 09:03, Juergen Gross wrote:
> 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
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Except for the Arm aspect, where I'm not sure using
guest_cpu_user_regs() is correct in all cases,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Nevertheless ...

> @@ -507,6 +509,50 @@ 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);
> +#endif

... to limit #ifdef-ary I'd have suggested to put

#else
# define crash_debug_kexeccmd NULL

right above here and ...

> +void keyhandler_crash_action(enum crash_reason reason)
> +{
> +    static const char *const crash_action[CRASHREASON_N] = {
> +        [CRASHREASON_PANIC] = crash_debug_panic,
> +        [CRASHREASON_HWDOM] = crash_debug_hwdom,
> +        [CRASHREASON_WATCHDOG] = crash_debug_watchdog,
> +#ifdef CONFIG_KEXEC
> +        [CRASHREASON_KEXECCMD] = crash_debug_kexeccmd,
> +#endif
> +        [CRASHREASON_DEBUGKEY] = crash_debug_debugkey,
> +    };
> +    const char *action;
> +    struct cpu_user_regs *regs = get_irq_regs() ? : guest_cpu_user_regs();
> +
> +    if ( (unsigned int)reason >= CRASHREASON_N )

... I'd have preferred ARRAY_SIZE() here, at which point the
array dimension also wouldn't need explicitly specifying.

Jan
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Stefano Stabellini 3 years, 4 months ago 
On Thu, 26 Nov 2020, Jan Beulich wrote:
> On 26.11.2020 09:03, Juergen Gross wrote:
> > 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
> > 
> > Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Except for the Arm aspect, where I'm not sure using
> guest_cpu_user_regs() is correct in all cases,
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 

for the trivial ARM bit:

Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Julien Grall 3 years, 4 months ago 
Hi,

Sorry for jumping late in the discussion.

On 26/11/2020 11:20, Jan Beulich wrote:
> On 26.11.2020 09:03, Juergen Gross wrote:
>> 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
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Except for the Arm aspect, where I'm not sure using
> guest_cpu_user_regs() is correct in all cases,

I am not entirely sure to understand what get_irq_regs() is supposed to 
returned on x86. Is it the registers saved from the most recent exception?

Cheers,


-- 
Julien Grall
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Jan Beulich 3 years, 4 months ago 
On 09.12.2020 13:11, Julien Grall wrote:
> On 26/11/2020 11:20, Jan Beulich wrote:
>> On 26.11.2020 09:03, Juergen Gross wrote:
>>> 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
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> Except for the Arm aspect, where I'm not sure using
>> guest_cpu_user_regs() is correct in all cases,
> 
> I am not entirely sure to understand what get_irq_regs() is supposed to 
> returned on x86. Is it the registers saved from the most recent exception?

An interrupt (not an exception) sets the underlying per-CPU
variable, such that interested parties will know the real
context is not guest or "normal" Xen code, but an IRQ.

Jan
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Julien Grall 3 years, 4 months ago 
Hi Jan,

On 09/12/2020 14:29, Jan Beulich wrote:
> On 09.12.2020 13:11, Julien Grall wrote:
>> On 26/11/2020 11:20, Jan Beulich wrote:
>>> On 26.11.2020 09:03, Juergen Gross wrote:
>>>> 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
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> Except for the Arm aspect, where I'm not sure using
>>> guest_cpu_user_regs() is correct in all cases,
>>
>> I am not entirely sure to understand what get_irq_regs() is supposed to
>> returned on x86. Is it the registers saved from the most recent exception?
> 
> An interrupt (not an exception) sets the underlying per-CPU
> variable, such that interested parties will know the real
> context is not guest or "normal" Xen code, but an IRQ.

Thanks for the explanation. I am a bit confused to why we need to give a 
regs to handle_keypress() because no-one seems to use it. Do you have an 
explanation?

To add to the confusion, it looks like that get_irqs_regs() may return 
NULL. So sometimes we may pass guest_cpu_regs() (which may contain 
garbagge or a set too far).

I guess providing the wrong information to handle_keypress() is not 
going to matter that much because no-one use it (?). Although, I'd like 
to make sure this is not going to bite us in the future.

Cheers,

-- 
Julien Grall
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Jürgen Groß 3 years, 4 months ago 
On 10.12.20 21:51, Julien Grall wrote:
> Hi Jan,
> 
> On 09/12/2020 14:29, Jan Beulich wrote:
>> On 09.12.2020 13:11, Julien Grall wrote:
>>> On 26/11/2020 11:20, Jan Beulich wrote:
>>>> On 26.11.2020 09:03, Juergen Gross wrote:
>>>>> 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
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>
>>>> Except for the Arm aspect, where I'm not sure using
>>>> guest_cpu_user_regs() is correct in all cases,
>>>
>>> I am not entirely sure to understand what get_irq_regs() is supposed to
>>> returned on x86. Is it the registers saved from the most recent 
>>> exception?
>>
>> An interrupt (not an exception) sets the underlying per-CPU
>> variable, such that interested parties will know the real
>> context is not guest or "normal" Xen code, but an IRQ.
> 
> Thanks for the explanation. I am a bit confused to why we need to give a 
> regs to handle_keypress() because no-one seems to use it. Do you have an 
> explanation?

dump_registers() (key 'd') is using it.

> 
> To add to the confusion, it looks like that get_irqs_regs() may return 
> NULL. So sometimes we may pass guest_cpu_regs() (which may contain 
> garbagge or a set too far).

I guess this is a best effort approach.

> 
> I guess providing the wrong information to handle_keypress() is not 
> going to matter that much because no-one use it (?). Although, I'd like 
> to make sure this is not going to bite us in the future.

TBH using the 'd' handler isn't making that much sense, as the
information delivered would be of interest only in case of a panic(),
which is already printing that information.


Juergen
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Jan Beulich 3 years, 4 months ago 
On 11.12.2020 08:02, Jürgen Groß wrote:
> On 10.12.20 21:51, Julien Grall wrote:
>> Hi Jan,
>>
>> On 09/12/2020 14:29, Jan Beulich wrote:
>>> On 09.12.2020 13:11, Julien Grall wrote:
>>>> On 26/11/2020 11:20, Jan Beulich wrote:
>>>>> On 26.11.2020 09:03, Juergen Gross wrote:
>>>>>> 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
>>>>>>
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>
>>>>> Except for the Arm aspect, where I'm not sure using
>>>>> guest_cpu_user_regs() is correct in all cases,
>>>>
>>>> I am not entirely sure to understand what get_irq_regs() is supposed to
>>>> returned on x86. Is it the registers saved from the most recent 
>>>> exception?
>>>
>>> An interrupt (not an exception) sets the underlying per-CPU
>>> variable, such that interested parties will know the real
>>> context is not guest or "normal" Xen code, but an IRQ.
>>
>> Thanks for the explanation. I am a bit confused to why we need to give a 
>> regs to handle_keypress() because no-one seems to use it. Do you have an 
>> explanation?
> 
> dump_registers() (key 'd') is using it.
> 
>>
>> To add to the confusion, it looks like that get_irqs_regs() may return 
>> NULL. So sometimes we may pass guest_cpu_regs() (which may contain 
>> garbagge or a set too far).
> 
> I guess this is a best effort approach.

Indeed. If there are ways to make it "more best", we should of
course follow them. (Except before Dom0 starts, I'm afraid I
don't see though where garbage would come from. And even then,
just like for the idle vCPU-s, it shouldn't really be garbage,
or else this suggests missing initialization somewhere.)

>> I guess providing the wrong information to handle_keypress() is not 
>> going to matter that much because no-one use it (?). Although, I'd like 
>> to make sure this is not going to bite us in the future.
> 
> TBH using the 'd' handler isn't making that much sense, as the
> information delivered would be of interest only in case of a panic(),
> which is already printing that information.

I disagree. I've had numerous cases where I found this key very
useful. Or do you mean what you say just for the new purpose
added here?

Jan
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Jürgen Groß 3 years, 4 months ago 
On 11.12.20 08:24, Jan Beulich wrote:
> On 11.12.2020 08:02, Jürgen Groß wrote:
>> On 10.12.20 21:51, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 09/12/2020 14:29, Jan Beulich wrote:
>>>> On 09.12.2020 13:11, Julien Grall wrote:
>>>>> On 26/11/2020 11:20, Jan Beulich wrote:
>>>>>> On 26.11.2020 09:03, Juergen Gross wrote:
>>>>>>> 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
>>>>>>>
>>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>>
>>>>>> Except for the Arm aspect, where I'm not sure using
>>>>>> guest_cpu_user_regs() is correct in all cases,
>>>>>
>>>>> I am not entirely sure to understand what get_irq_regs() is supposed to
>>>>> returned on x86. Is it the registers saved from the most recent
>>>>> exception?
>>>>
>>>> An interrupt (not an exception) sets the underlying per-CPU
>>>> variable, such that interested parties will know the real
>>>> context is not guest or "normal" Xen code, but an IRQ.
>>>
>>> Thanks for the explanation. I am a bit confused to why we need to give a
>>> regs to handle_keypress() because no-one seems to use it. Do you have an
>>> explanation?
>>
>> dump_registers() (key 'd') is using it.
>>
>>>
>>> To add to the confusion, it looks like that get_irqs_regs() may return
>>> NULL. So sometimes we may pass guest_cpu_regs() (which may contain
>>> garbagge or a set too far).
>>
>> I guess this is a best effort approach.
> 
> Indeed. If there are ways to make it "more best", we should of
> course follow them. (Except before Dom0 starts, I'm afraid I
> don't see though where garbage would come from. And even then,
> just like for the idle vCPU-s, it shouldn't really be garbage,
> or else this suggests missing initialization somewhere.)
> 
>>> I guess providing the wrong information to handle_keypress() is not
>>> going to matter that much because no-one use it (?). Although, I'd like
>>> to make sure this is not going to bite us in the future.
>>
>> TBH using the 'd' handler isn't making that much sense, as the
>> information delivered would be of interest only in case of a panic(),
>> which is already printing that information.
> 
> I disagree. I've had numerous cases where I found this key very
> useful. Or do you mean what you say just for the new purpose
> added here?

Just for the new purpose.


Juergen
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Julien Grall 3 years, 4 months ago 
Hi,

On 11/12/2020 07:24, Jan Beulich wrote:
> On 11.12.2020 08:02, Jürgen Groß wrote:
>> On 10.12.20 21:51, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 09/12/2020 14:29, Jan Beulich wrote:
>>>> On 09.12.2020 13:11, Julien Grall wrote:
>>>>> On 26/11/2020 11:20, Jan Beulich wrote:
>>>>>> On 26.11.2020 09:03, Juergen Gross wrote:
>>>>>>> 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
>>>>>>>
>>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>>
>>>>>> Except for the Arm aspect, where I'm not sure using
>>>>>> guest_cpu_user_regs() is correct in all cases,
>>>>>
>>>>> I am not entirely sure to understand what get_irq_regs() is supposed to
>>>>> returned on x86. Is it the registers saved from the most recent
>>>>> exception?
>>>>
>>>> An interrupt (not an exception) sets the underlying per-CPU
>>>> variable, such that interested parties will know the real
>>>> context is not guest or "normal" Xen code, but an IRQ.
>>>
>>> Thanks for the explanation. I am a bit confused to why we need to give a
>>> regs to handle_keypress() because no-one seems to use it. Do you have an
>>> explanation?
>>
>> dump_registers() (key 'd') is using it.
>>
>>>
>>> To add to the confusion, it looks like that get_irqs_regs() may return
>>> NULL. So sometimes we may pass guest_cpu_regs() (which may contain
>>> garbagge or a set too far).
>>
>> I guess this is a best effort approach.
> 
> Indeed. If there are ways to make it "more best", we should of
> course follow them. (Except before Dom0 starts, I'm afraid I
> don't see though where garbage would come from. And even then,
> just like for the idle vCPU-s, it shouldn't really be garbage,
> or else this suggests missing initialization somewhere.)

So I decided to mimick what 'd' does to see what happen if this is 
called early boot.


diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 7fcff9af2a7e..9d33507a26eb 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -857,6 +857,8 @@ void __init start_xen(unsigned long boot_phys_offset,
       */
      system_state = SYS_STATE_boot;

+    dump_execstate(guest_cpu_user_regs());
+
      vm_init();

      if ( acpi_disabled )
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 30d6f375a3af..50fcf2e8d70e 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1678,6 +1678,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
          end_boot_allocator();

      system_state = SYS_STATE_boot;
+    dump_execstate(guest_cpu_user_regs());
      /*
       * No calls involving ACPI code should go between the setting of
       * SYS_STATE_boot and vm_init() (or else acpi_os_{,un}map_memory()

It leads to crash on both Arm and x86.

For the Arm crash:

(XEN) Data Abort Trap. Syndrome=0x1c08006
(XEN) Walking Hypervisor VA 0x10 on CPU0 via TTBR 0x0000000065a7f000
(XEN) 0TH[0x0] = 0x0000000065a7ef7f
(XEN) 1ST[0x0] = 0x0000000065a7bf7f
(XEN) 2ND[0x0] = 0x0000000000000000
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.15-unstable  arm64  debug=y   Not tainted ]----
(XEN) CPU:    0
(XEN) PC:     0000000000219674 dump_execstate+0x58/0x1ec
(XEN) LR:     00000000002d77dc
(XEN) SP:     000000000030fdc0
(XEN) CPSR:   800003c9 MODE:64-bit EL2h (Hypervisor, handler)
(XEN)      X0: 0000000000000000  X1: 0000000000000000  X2: 0000000000007fff
(XEN)      X3: 00000000002b7198  X4: 0000000000000080  X5: 00000000002e9a68
(XEN)      X6: 0080808080808080  X7: fefefefefefeff09  X8: 7f7f7f7f7f7f7f7f
(XEN)      X9: 717164616f726051 X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
(XEN)     X12: 0000000000000008 X13: 00000000002b9a48 X14: 0000000000000000
(XEN)     X15: 0000000000400000 X16: 00000000002ba000 X17: 00000000002b9000
(XEN)     X18: 00000000002b9000 X19: 0000000000000000 X20: 000000000030feb0
(XEN)     X21: 0000000080000000 X22: 00000000002f0d30 X23: 00000000002f1d68
(XEN)     X24: 00000000002f0eb8 X25: 0000000040000000 X26: 0000000080000000
(XEN)     X27: 0000000000000018 X28: 000000000003f970  FP: 000000000030fdc0
(XEN)
(XEN)   VTCR_EL2: 00000000
(XEN)  VTTBR_EL2: 0000000000000000
(XEN)
(XEN)  SCTLR_EL2: 30cd183d
(XEN)    HCR_EL2: 0000000000000038
(XEN)  TTBR0_EL2: 0000000065a7f000
(XEN)
(XEN)    ESR_EL2: 97c08006
(XEN)  HPFAR_EL2: 0000000000000000
(XEN)    FAR_EL2: 0000000000000010
(XEN)
(XEN) Xen stack trace from sp=000000000030fdc0:
(XEN)    000000000030fdf0 00000000002d77dc 0000000000080000 000000007fffc000
(XEN)    0000000080000000 00000000002f0d30 000000007f68b250 00000000002001b8
(XEN)    0000000065932000 0000000065732000 00000000784f9000 0000000000000000
(XEN)    0000000000400000 0000000065a2ad30 0000000000000630 0000000000000001
(XEN)    0000000000000001 0000000000000001 0000000000000000 0000000000003000
(XEN)    00000000784f9000 00000000002bc8e4 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000300000000 0000000000000000 00000040ffffffff
(XEN)    00000000ffffffff 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) Xen call trace:
(XEN)    [<0000000000219674>] dump_execstate+0x58/0x1ec (PC)
(XEN)    [<00000000002d77dc>] start_xen+0x3d0/0xcf8 (LR)
(XEN)    [<00000000002d77dc>] start_xen+0x3d0/0xcf8
(XEN)    [<00000000002001b8>] arm64/head.o#primary_switched+0x10/0x30
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ****************************************

For the x86 crash:

(XEN) Early fatal page fault at e008:ffff82d0402188b4 
(cr2=0000000000000010, ec=0000)
(XEN) ----[ Xen-4.15-unstable  x86_64  debug=y   Tainted:   C   ]----
(XEN) CPU:    0
(XEN) RIP:    e008:[<ffff82d0402188b4>] dump_execstate+0x42/0x167
(XEN) RFLAGS: 0000000000010086   CONTEXT: hypervisor
(XEN) rax: 0000000000000000   rbx: 0000000000000000   rcx: 0000000000000000
(XEN) rdx: ffff82d0404affff   rsi: 000000000000000a   rdi: ffff82d0404afef8
(XEN) rbp: ffff82d0404afd90   rsp: ffff82d0404afd80   r8:  0000000000000004
(XEN) r9:  0101010101010101   r10: 0f0f0f0f0f0f0f0f   r11: 5555555555555555
(XEN) r12: ffff82d0404afef8   r13: 0000000000800163   r14: ffff83000009dfb0
(XEN) r15: 0000000000000002   cr0: 0000000080050033   cr4: 00000000000000a0
(XEN) cr3: 00000000bfa9e000   cr2: 0000000000000010
(XEN) fsb: 0000000000000000   gsb: 0000000000000000   gss: 0000000000000000
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: 0000   cs: e008
(XEN) Xen code around <ffff82d0402188b4> (dump_execstate+0x42/0x167):
(XEN)  ff 7f 00 00 48 8b 40 c9 <48> 8b 40 10 66 81 38 ff 7f 75 49 3b 1d 
23 18 27
(XEN) Xen stack trace from rsp=ffff82d0404afd80:
(XEN)    000000000023ffff 00000000000005ed ffff82d0404afee8 ffff82d0404378cb
(XEN)    0000000000000002 0000000000000002 0000000000000002 0000000000000001
(XEN)    0000000000000001 0000000000000001 0000000000000001 0000000000000000
(XEN)    00000000000001ff 0000000002a45fff 0000000000240000 0000000002a45000
(XEN)    0000000000100000 0000000000000000 00000000000001ff ffff82d040475c80
(XEN)    ffff82d000800163 ffff83000009dee0 ffff83000009dfb0 0000000000200001
(XEN)    0000000100000000 0000000100000000 ffff83000009df80 642ded38bf9fe4f3
(XEN)    bf9fed3500000000 bfaafe980009df73 0009df73bf9fe7ea 00000004bf9fed31
(XEN)    bfaafeb00009df01 0000000800000000 000000010000006e 0000000000000003
(XEN)    00000000000002f8 ffff82d0405b0000 ffff82d0404b0000 0000000000000002
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 ffff82d04020012f 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000e01000000000 0000000000000000 0000000000000000 00000000000000a0
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN) Xen call trace:
(XEN)    [<ffff82d0402188b4>] R dump_execstate+0x42/0x167
(XEN)    [<ffff82d0404378cb>] F __start_xen+0x1e10/0x2906
(XEN)    [<ffff82d04020012f>] F __high_start+0x8f/0x91
(XEN)
(XEN) Pagetable walk from 0000000000000010:
(XEN)  L4[0x000] = 00000000bfa54063 ffffffffffffffff
(XEN)  L3[0x000] = 00000000bfa50063 ffffffffffffffff
(XEN)  L2[0x000] = 00000000bfa4f063 ffffffffffffffff
(XEN)  L1[0x000] = 0000000000000000 ffffffffffffffff
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) FATAL TRAP: vec 14, #PF[0000] IN INTERRUPT CONTEXT
(XEN) ****************************************
(XEN)
(XEN) Reboot in five seconds...

So I think guest_cpu_user_regs() is not quite yet ready to be called 
from panic().

A different approach my be to generate an exception and call the 
keyhandler from there. At least you know that the register would always 
be accurate.

Cheers,

-- 
Julien Grall
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Jürgen Groß 3 years, 4 months ago 
On 11.12.20 11:22, Julien Grall wrote:
> Hi,
> 
> On 11/12/2020 07:24, Jan Beulich wrote:
>> On 11.12.2020 08:02, Jürgen Groß wrote:
>>> On 10.12.20 21:51, Julien Grall wrote:
>>>> Hi Jan,
>>>>
>>>> On 09/12/2020 14:29, Jan Beulich wrote:
>>>>> On 09.12.2020 13:11, Julien Grall wrote:
>>>>>> On 26/11/2020 11:20, Jan Beulich wrote:
>>>>>>> On 26.11.2020 09:03, Juergen Gross wrote:
>>>>>>>> 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
>>>>>>>>
>>>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>>>
>>>>>>> Except for the Arm aspect, where I'm not sure using
>>>>>>> guest_cpu_user_regs() is correct in all cases,
>>>>>>
>>>>>> I am not entirely sure to understand what get_irq_regs() is 
>>>>>> supposed to
>>>>>> returned on x86. Is it the registers saved from the most recent
>>>>>> exception?
>>>>>
>>>>> An interrupt (not an exception) sets the underlying per-CPU
>>>>> variable, such that interested parties will know the real
>>>>> context is not guest or "normal" Xen code, but an IRQ.
>>>>
>>>> Thanks for the explanation. I am a bit confused to why we need to 
>>>> give a
>>>> regs to handle_keypress() because no-one seems to use it. Do you 
>>>> have an
>>>> explanation?
>>>
>>> dump_registers() (key 'd') is using it.
>>>
>>>>
>>>> To add to the confusion, it looks like that get_irqs_regs() may return
>>>> NULL. So sometimes we may pass guest_cpu_regs() (which may contain
>>>> garbagge or a set too far).
>>>
>>> I guess this is a best effort approach.
>>
>> Indeed. If there are ways to make it "more best", we should of
>> course follow them. (Except before Dom0 starts, I'm afraid I
>> don't see though where garbage would come from. And even then,
>> just like for the idle vCPU-s, it shouldn't really be garbage,
>> or else this suggests missing initialization somewhere.)
> 
> So I decided to mimick what 'd' does to see what happen if this is 
> called early boot.
> 
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 7fcff9af2a7e..9d33507a26eb 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -857,6 +857,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>        */
>       system_state = SYS_STATE_boot;
> 
> +    dump_execstate(guest_cpu_user_regs());
> +
>       vm_init();
> 
>       if ( acpi_disabled )
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 30d6f375a3af..50fcf2e8d70e 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1678,6 +1678,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>           end_boot_allocator();
> 
>       system_state = SYS_STATE_boot;
> +    dump_execstate(guest_cpu_user_regs());
>       /*
>        * No calls involving ACPI code should go between the setting of
>        * SYS_STATE_boot and vm_init() (or else acpi_os_{,un}map_memory()
> 
> It leads to crash on both Arm and x86.
> 
> For the Arm crash:
> 
> (XEN) Data Abort Trap. Syndrome=0x1c08006
> (XEN) Walking Hypervisor VA 0x10 on CPU0 via TTBR 0x0000000065a7f000
> (XEN) 0TH[0x0] = 0x0000000065a7ef7f
> (XEN) 1ST[0x0] = 0x0000000065a7bf7f
> (XEN) 2ND[0x0] = 0x0000000000000000
> (XEN) CPU0: Unexpected Trap: Data Abort
> (XEN) ----[ Xen-4.15-unstable  arm64  debug=y   Not tainted ]----
> (XEN) CPU:    0
> (XEN) PC:     0000000000219674 dump_execstate+0x58/0x1ec
> (XEN) LR:     00000000002d77dc
> (XEN) SP:     000000000030fdc0
> (XEN) CPSR:   800003c9 MODE:64-bit EL2h (Hypervisor, handler)
> (XEN)      X0: 0000000000000000  X1: 0000000000000000  X2: 0000000000007fff
> (XEN)      X3: 00000000002b7198  X4: 0000000000000080  X5: 00000000002e9a68
> (XEN)      X6: 0080808080808080  X7: fefefefefefeff09  X8: 7f7f7f7f7f7f7f7f
> (XEN)      X9: 717164616f726051 X10: 7f7f7f7f7f7f7f7f X11: 0101010101010101
> (XEN)     X12: 0000000000000008 X13: 00000000002b9a48 X14: 0000000000000000
> (XEN)     X15: 0000000000400000 X16: 00000000002ba000 X17: 00000000002b9000
> (XEN)     X18: 00000000002b9000 X19: 0000000000000000 X20: 000000000030feb0
> (XEN)     X21: 0000000080000000 X22: 00000000002f0d30 X23: 00000000002f1d68
> (XEN)     X24: 00000000002f0eb8 X25: 0000000040000000 X26: 0000000080000000
> (XEN)     X27: 0000000000000018 X28: 000000000003f970  FP: 000000000030fdc0
> (XEN)
> (XEN)   VTCR_EL2: 00000000
> (XEN)  VTTBR_EL2: 0000000000000000
> (XEN)
> (XEN)  SCTLR_EL2: 30cd183d
> (XEN)    HCR_EL2: 0000000000000038
> (XEN)  TTBR0_EL2: 0000000065a7f000
> (XEN)
> (XEN)    ESR_EL2: 97c08006
> (XEN)  HPFAR_EL2: 0000000000000000
> (XEN)    FAR_EL2: 0000000000000010
> (XEN)
> (XEN) Xen stack trace from sp=000000000030fdc0:
> (XEN)    000000000030fdf0 00000000002d77dc 0000000000080000 
> 000000007fffc000
> (XEN)    0000000080000000 00000000002f0d30 000000007f68b250 
> 00000000002001b8
> (XEN)    0000000065932000 0000000065732000 00000000784f9000 
> 0000000000000000
> (XEN)    0000000000400000 0000000065a2ad30 0000000000000630 
> 0000000000000001
> (XEN)    0000000000000001 0000000000000001 0000000000000000 
> 0000000000003000
> (XEN)    00000000784f9000 00000000002bc8e4 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000300000000 0000000000000000 
> 00000040ffffffff
> (XEN)    00000000ffffffff 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN) Xen call trace:
> (XEN)    [<0000000000219674>] dump_execstate+0x58/0x1ec (PC)
> (XEN)    [<00000000002d77dc>] start_xen+0x3d0/0xcf8 (LR)
> (XEN)    [<00000000002d77dc>] start_xen+0x3d0/0xcf8
> (XEN)    [<00000000002001b8>] arm64/head.o#primary_switched+0x10/0x30
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) CPU0: Unexpected Trap: Data Abort
> (XEN) ****************************************
> 
> For the x86 crash:
> 
> (XEN) Early fatal page fault at e008:ffff82d0402188b4 
> (cr2=0000000000000010, ec=0000)
> (XEN) ----[ Xen-4.15-unstable  x86_64  debug=y   Tainted:   C   ]----
> (XEN) CPU:    0
> (XEN) RIP:    e008:[<ffff82d0402188b4>] dump_execstate+0x42/0x167
> (XEN) RFLAGS: 0000000000010086   CONTEXT: hypervisor
> (XEN) rax: 0000000000000000   rbx: 0000000000000000   rcx: 0000000000000000
> (XEN) rdx: ffff82d0404affff   rsi: 000000000000000a   rdi: ffff82d0404afef8
> (XEN) rbp: ffff82d0404afd90   rsp: ffff82d0404afd80   r8:  0000000000000004
> (XEN) r9:  0101010101010101   r10: 0f0f0f0f0f0f0f0f   r11: 5555555555555555
> (XEN) r12: ffff82d0404afef8   r13: 0000000000800163   r14: ffff83000009dfb0
> (XEN) r15: 0000000000000002   cr0: 0000000080050033   cr4: 00000000000000a0
> (XEN) cr3: 00000000bfa9e000   cr2: 0000000000000010
> (XEN) fsb: 0000000000000000   gsb: 0000000000000000   gss: 0000000000000000
> (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: 0000   cs: e008
> (XEN) Xen code around <ffff82d0402188b4> (dump_execstate+0x42/0x167):
> (XEN)  ff 7f 00 00 48 8b 40 c9 <48> 8b 40 10 66 81 38 ff 7f 75 49 3b 1d 
> 23 18 27
> (XEN) Xen stack trace from rsp=ffff82d0404afd80:
> (XEN)    000000000023ffff 00000000000005ed ffff82d0404afee8 
> ffff82d0404378cb
> (XEN)    0000000000000002 0000000000000002 0000000000000002 
> 0000000000000001
> (XEN)    0000000000000001 0000000000000001 0000000000000001 
> 0000000000000000
> (XEN)    00000000000001ff 0000000002a45fff 0000000000240000 
> 0000000002a45000
> (XEN)    0000000000100000 0000000000000000 00000000000001ff 
> ffff82d040475c80
> (XEN)    ffff82d000800163 ffff83000009dee0 ffff83000009dfb0 
> 0000000000200001
> (XEN)    0000000100000000 0000000100000000 ffff83000009df80 
> 642ded38bf9fe4f3
> (XEN)    bf9fed3500000000 bfaafe980009df73 0009df73bf9fe7ea 
> 00000004bf9fed31
> (XEN)    bfaafeb00009df01 0000000800000000 000000010000006e 
> 0000000000000003
> (XEN)    00000000000002f8 ffff82d0405b0000 ffff82d0404b0000 
> 0000000000000002
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 ffff82d04020012f 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN)    0000e01000000000 0000000000000000 0000000000000000 
> 00000000000000a0
> (XEN)    0000000000000000 0000000000000000 0000000000000000 
> 0000000000000000
> (XEN) Xen call trace:
> (XEN)    [<ffff82d0402188b4>] R dump_execstate+0x42/0x167
> (XEN)    [<ffff82d0404378cb>] F __start_xen+0x1e10/0x2906
> (XEN)    [<ffff82d04020012f>] F __high_start+0x8f/0x91
> (XEN)
> (XEN) Pagetable walk from 0000000000000010:
> (XEN)  L4[0x000] = 00000000bfa54063 ffffffffffffffff
> (XEN)  L3[0x000] = 00000000bfa50063 ffffffffffffffff
> (XEN)  L2[0x000] = 00000000bfa4f063 ffffffffffffffff
> (XEN)  L1[0x000] = 0000000000000000 ffffffffffffffff
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) FATAL TRAP: vec 14, #PF[0000] IN INTERRUPT CONTEXT
> (XEN) ****************************************
> (XEN)
> (XEN) Reboot in five seconds...
> 
> So I think guest_cpu_user_regs() is not quite yet ready to be called 
> from panic().

guest_cpu_user_regs() isn't the problem, but dump_execstate().

This is one of the caveats from the added boot parameter doc: some debug
keys might lead to problems. 'd' seems to be such a key when used for
the panic() case and the panic() happens in early boot.

> 
> A different approach my be to generate an exception and call the 
> keyhandler from there. At least you know that the register would always 
> be accurate.

Or dump_execstate() is modified to accept NULL for regs and it will do
nothing in case guest_cpu_user_regs() isn't valid (a test for idle vcpu
might be the easiest way to determine that).


Juergen
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Julien Grall 3 years, 4 months ago 

On 11/12/2020 10:38, Jürgen Groß wrote:
> On 11.12.20 11:22, Julien Grall wrote:
>> Hi,
>>
>> On 11/12/2020 07:24, Jan Beulich wrote:
>>> On 11.12.2020 08:02, Jürgen Groß wrote:
>>>> On 10.12.20 21:51, Julien Grall wrote:
>>>>> Hi Jan,
>>>>>
>>>>> On 09/12/2020 14:29, Jan Beulich wrote:
>>>>>> On 09.12.2020 13:11, Julien Grall wrote:
>>>>>>> On 26/11/2020 11:20, Jan Beulich wrote:
>>>>>>>> On 26.11.2020 09:03, Juergen Gross wrote:
>>>>>>>>> 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
>>>>>>>>>
>>>>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>>>>
>>>>>>>> Except for the Arm aspect, where I'm not sure using
>>>>>>>> guest_cpu_user_regs() is correct in all cases,
>>>>>>>
>>>>>>> I am not entirely sure to understand what get_irq_regs() is 
>>>>>>> supposed to
>>>>>>> returned on x86. Is it the registers saved from the most recent
>>>>>>> exception?
>>>>>>
>>>>>> An interrupt (not an exception) sets the underlying per-CPU
>>>>>> variable, such that interested parties will know the real
>>>>>> context is not guest or "normal" Xen code, but an IRQ.
>>>>>
>>>>> Thanks for the explanation. I am a bit confused to why we need to 
>>>>> give a
>>>>> regs to handle_keypress() because no-one seems to use it. Do you 
>>>>> have an
>>>>> explanation?
>>>>
>>>> dump_registers() (key 'd') is using it.
>>>>
>>>>>
>>>>> To add to the confusion, it looks like that get_irqs_regs() may return
>>>>> NULL. So sometimes we may pass guest_cpu_regs() (which may contain
>>>>> garbagge or a set too far).
>>>>
>>>> I guess this is a best effort approach.
>>>
>>> Indeed. If there are ways to make it "more best", we should of
>>> course follow them. (Except before Dom0 starts, I'm afraid I
>>> don't see though where garbage would come from. And even then,
>>> just like for the idle vCPU-s, it shouldn't really be garbage,
>>> or else this suggests missing initialization somewhere.)
>>
>> So I decided to mimick what 'd' does to see what happen if this is 
>> called early boot.
>>
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 7fcff9af2a7e..9d33507a26eb 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -857,6 +857,8 @@ void __init start_xen(unsigned long boot_phys_offset,
>>        */
>>       system_state = SYS_STATE_boot;
>>
>> +    dump_execstate(guest_cpu_user_regs());
>> +
>>       vm_init();
>>
>>       if ( acpi_disabled )
>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index 30d6f375a3af..50fcf2e8d70e 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1678,6 +1678,7 @@ void __init noreturn __start_xen(unsigned long 
>> mbi_p)
>>           end_boot_allocator();
>>
>>       system_state = SYS_STATE_boot;
>> +    dump_execstate(guest_cpu_user_regs());
>>       /*
>>        * No calls involving ACPI code should go between the setting of
>>        * SYS_STATE_boot and vm_init() (or else acpi_os_{,un}map_memory()
>>
>> It leads to crash on both Arm and x86.
>>
>> For the Arm crash:
>>
>> (XEN) Data Abort Trap. Syndrome=0x1c08006
>> (XEN) Walking Hypervisor VA 0x10 on CPU0 via TTBR 0x0000000065a7f000
>> (XEN) 0TH[0x0] = 0x0000000065a7ef7f
>> (XEN) 1ST[0x0] = 0x0000000065a7bf7f
>> (XEN) 2ND[0x0] = 0x0000000000000000
>> (XEN) CPU0: Unexpected Trap: Data Abort
>> (XEN) ----[ Xen-4.15-unstable  arm64  debug=y   Not tainted ]----
>> (XEN) CPU:    0
>> (XEN) PC:     0000000000219674 dump_execstate+0x58/0x1ec
>> (XEN) LR:     00000000002d77dc
>> (XEN) SP:     000000000030fdc0
>> (XEN) CPSR:   800003c9 MODE:64-bit EL2h (Hypervisor, handler)
>> (XEN)      X0: 0000000000000000  X1: 0000000000000000  X2: 
>> 0000000000007fff
>> (XEN)      X3: 00000000002b7198  X4: 0000000000000080  X5: 
>> 00000000002e9a68
>> (XEN)      X6: 0080808080808080  X7: fefefefefefeff09  X8: 
>> 7f7f7f7f7f7f7f7f
>> (XEN)      X9: 717164616f726051 X10: 7f7f7f7f7f7f7f7f X11: 
>> 0101010101010101
>> (XEN)     X12: 0000000000000008 X13: 00000000002b9a48 X14: 
>> 0000000000000000
>> (XEN)     X15: 0000000000400000 X16: 00000000002ba000 X17: 
>> 00000000002b9000
>> (XEN)     X18: 00000000002b9000 X19: 0000000000000000 X20: 
>> 000000000030feb0
>> (XEN)     X21: 0000000080000000 X22: 00000000002f0d30 X23: 
>> 00000000002f1d68
>> (XEN)     X24: 00000000002f0eb8 X25: 0000000040000000 X26: 
>> 0000000080000000
>> (XEN)     X27: 0000000000000018 X28: 000000000003f970  FP: 
>> 000000000030fdc0
>> (XEN)
>> (XEN)   VTCR_EL2: 00000000
>> (XEN)  VTTBR_EL2: 0000000000000000
>> (XEN)
>> (XEN)  SCTLR_EL2: 30cd183d
>> (XEN)    HCR_EL2: 0000000000000038
>> (XEN)  TTBR0_EL2: 0000000065a7f000
>> (XEN)
>> (XEN)    ESR_EL2: 97c08006
>> (XEN)  HPFAR_EL2: 0000000000000000
>> (XEN)    FAR_EL2: 0000000000000010
>> (XEN)
>> (XEN) Xen stack trace from sp=000000000030fdc0:
>> (XEN)    000000000030fdf0 00000000002d77dc 0000000000080000 
>> 000000007fffc000
>> (XEN)    0000000080000000 00000000002f0d30 000000007f68b250 
>> 00000000002001b8
>> (XEN)    0000000065932000 0000000065732000 00000000784f9000 
>> 0000000000000000
>> (XEN)    0000000000400000 0000000065a2ad30 0000000000000630 
>> 0000000000000001
>> (XEN)    0000000000000001 0000000000000001 0000000000000000 
>> 0000000000003000
>> (XEN)    00000000784f9000 00000000002bc8e4 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000300000000 0000000000000000 
>> 00000040ffffffff
>> (XEN)    00000000ffffffff 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN) Xen call trace:
>> (XEN)    [<0000000000219674>] dump_execstate+0x58/0x1ec (PC)
>> (XEN)    [<00000000002d77dc>] start_xen+0x3d0/0xcf8 (LR)
>> (XEN)    [<00000000002d77dc>] start_xen+0x3d0/0xcf8
>> (XEN)    [<00000000002001b8>] arm64/head.o#primary_switched+0x10/0x30
>> (XEN)
>> (XEN)
>> (XEN) ****************************************
>> (XEN) Panic on CPU 0:
>> (XEN) CPU0: Unexpected Trap: Data Abort
>> (XEN) ****************************************
>>
>> For the x86 crash:
>>
>> (XEN) Early fatal page fault at e008:ffff82d0402188b4 
>> (cr2=0000000000000010, ec=0000)
>> (XEN) ----[ Xen-4.15-unstable  x86_64  debug=y   Tainted:   C   ]----
>> (XEN) CPU:    0
>> (XEN) RIP:    e008:[<ffff82d0402188b4>] dump_execstate+0x42/0x167
>> (XEN) RFLAGS: 0000000000010086   CONTEXT: hypervisor
>> (XEN) rax: 0000000000000000   rbx: 0000000000000000   rcx: 
>> 0000000000000000
>> (XEN) rdx: ffff82d0404affff   rsi: 000000000000000a   rdi: 
>> ffff82d0404afef8
>> (XEN) rbp: ffff82d0404afd90   rsp: ffff82d0404afd80   r8:  
>> 0000000000000004
>> (XEN) r9:  0101010101010101   r10: 0f0f0f0f0f0f0f0f   r11: 
>> 5555555555555555
>> (XEN) r12: ffff82d0404afef8   r13: 0000000000800163   r14: 
>> ffff83000009dfb0
>> (XEN) r15: 0000000000000002   cr0: 0000000080050033   cr4: 
>> 00000000000000a0
>> (XEN) cr3: 00000000bfa9e000   cr2: 0000000000000010
>> (XEN) fsb: 0000000000000000   gsb: 0000000000000000   gss: 
>> 0000000000000000
>> (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: 0000   cs: e008
>> (XEN) Xen code around <ffff82d0402188b4> (dump_execstate+0x42/0x167):
>> (XEN)  ff 7f 00 00 48 8b 40 c9 <48> 8b 40 10 66 81 38 ff 7f 75 49 3b 
>> 1d 23 18 27
>> (XEN) Xen stack trace from rsp=ffff82d0404afd80:
>> (XEN)    000000000023ffff 00000000000005ed ffff82d0404afee8 
>> ffff82d0404378cb
>> (XEN)    0000000000000002 0000000000000002 0000000000000002 
>> 0000000000000001
>> (XEN)    0000000000000001 0000000000000001 0000000000000001 
>> 0000000000000000
>> (XEN)    00000000000001ff 0000000002a45fff 0000000000240000 
>> 0000000002a45000
>> (XEN)    0000000000100000 0000000000000000 00000000000001ff 
>> ffff82d040475c80
>> (XEN)    ffff82d000800163 ffff83000009dee0 ffff83000009dfb0 
>> 0000000000200001
>> (XEN)    0000000100000000 0000000100000000 ffff83000009df80 
>> 642ded38bf9fe4f3
>> (XEN)    bf9fed3500000000 bfaafe980009df73 0009df73bf9fe7ea 
>> 00000004bf9fed31
>> (XEN)    bfaafeb00009df01 0000000800000000 000000010000006e 
>> 0000000000000003
>> (XEN)    00000000000002f8 ffff82d0405b0000 ffff82d0404b0000 
>> 0000000000000002
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 ffff82d04020012f 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN)    0000e01000000000 0000000000000000 0000000000000000 
>> 00000000000000a0
>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>> 0000000000000000
>> (XEN) Xen call trace:
>> (XEN)    [<ffff82d0402188b4>] R dump_execstate+0x42/0x167
>> (XEN)    [<ffff82d0404378cb>] F __start_xen+0x1e10/0x2906
>> (XEN)    [<ffff82d04020012f>] F __high_start+0x8f/0x91
>> (XEN)
>> (XEN) Pagetable walk from 0000000000000010:
>> (XEN)  L4[0x000] = 00000000bfa54063 ffffffffffffffff
>> (XEN)  L3[0x000] = 00000000bfa50063 ffffffffffffffff
>> (XEN)  L2[0x000] = 00000000bfa4f063 ffffffffffffffff
>> (XEN)  L1[0x000] = 0000000000000000 ffffffffffffffff
>> (XEN)
>> (XEN) ****************************************
>> (XEN) Panic on CPU 0:
>> (XEN) FATAL TRAP: vec 14, #PF[0000] IN INTERRUPT CONTEXT
>> (XEN) ****************************************
>> (XEN)
>> (XEN) Reboot in five seconds...
>>
>> So I think guest_cpu_user_regs() is not quite yet ready to be called 
>> from panic().
> 
> guest_cpu_user_regs() isn't the problem, but dump_execstate().
> 
> This is one of the caveats from the added boot parameter doc: some debug
> keys might lead to problems. 'd' seems to be such a key when used for
> the panic() case and the panic() happens in early boot.

Right, I think we should be clearer in the documentation about the keys 
we know works.

> 
>>
>> A different approach my be to generate an exception and call the 
>> keyhandler from there. At least you know that the register would 
>> always be accurate.
> 
> Or dump_execstate() is modified to accept NULL for regs and it will do
> nothing in case guest_cpu_user_regs() isn't valid (a test for idle vcpu
> might be the easiest way to determine that).

So Jan pointed out that current may not be initialized properly in early 
boot. So we possibly want to use "system_state" instead.

Cheers,

-- 
Julien Grall
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Jürgen Groß 3 years, 4 months ago 
On 11.12.20 11:49, Julien Grall wrote:
> 
> 
> On 11/12/2020 10:38, Jürgen Groß wrote:
>> On 11.12.20 11:22, Julien Grall wrote:
>>> Hi,
>>>
>>> On 11/12/2020 07:24, Jan Beulich wrote:
>>>> On 11.12.2020 08:02, Jürgen Groß wrote:
>>>>> On 10.12.20 21:51, Julien Grall wrote:
>>>>>> Hi Jan,
>>>>>>
>>>>>> On 09/12/2020 14:29, Jan Beulich wrote:
>>>>>>> On 09.12.2020 13:11, Julien Grall wrote:
>>>>>>>> On 26/11/2020 11:20, Jan Beulich wrote:
>>>>>>>>> On 26.11.2020 09:03, Juergen Gross wrote:
>>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>>>>>
>>>>>>>>> Except for the Arm aspect, where I'm not sure using
>>>>>>>>> guest_cpu_user_regs() is correct in all cases,
>>>>>>>>
>>>>>>>> I am not entirely sure to understand what get_irq_regs() is 
>>>>>>>> supposed to
>>>>>>>> returned on x86. Is it the registers saved from the most recent
>>>>>>>> exception?
>>>>>>>
>>>>>>> An interrupt (not an exception) sets the underlying per-CPU
>>>>>>> variable, such that interested parties will know the real
>>>>>>> context is not guest or "normal" Xen code, but an IRQ.
>>>>>>
>>>>>> Thanks for the explanation. I am a bit confused to why we need to 
>>>>>> give a
>>>>>> regs to handle_keypress() because no-one seems to use it. Do you 
>>>>>> have an
>>>>>> explanation?
>>>>>
>>>>> dump_registers() (key 'd') is using it.
>>>>>
>>>>>>
>>>>>> To add to the confusion, it looks like that get_irqs_regs() may 
>>>>>> return
>>>>>> NULL. So sometimes we may pass guest_cpu_regs() (which may contain
>>>>>> garbagge or a set too far).
>>>>>
>>>>> I guess this is a best effort approach.
>>>>
>>>> Indeed. If there are ways to make it "more best", we should of
>>>> course follow them. (Except before Dom0 starts, I'm afraid I
>>>> don't see though where garbage would come from. And even then,
>>>> just like for the idle vCPU-s, it shouldn't really be garbage,
>>>> or else this suggests missing initialization somewhere.)
>>>
>>> So I decided to mimick what 'd' does to see what happen if this is 
>>> called early boot.
>>>
>>>
>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>>> index 7fcff9af2a7e..9d33507a26eb 100644
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -857,6 +857,8 @@ void __init start_xen(unsigned long 
>>> boot_phys_offset,
>>>        */
>>>       system_state = SYS_STATE_boot;
>>>
>>> +    dump_execstate(guest_cpu_user_regs());
>>> +
>>>       vm_init();
>>>
>>>       if ( acpi_disabled )
>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>> index 30d6f375a3af..50fcf2e8d70e 100644
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1678,6 +1678,7 @@ void __init noreturn __start_xen(unsigned long 
>>> mbi_p)
>>>           end_boot_allocator();
>>>
>>>       system_state = SYS_STATE_boot;
>>> +    dump_execstate(guest_cpu_user_regs());
>>>       /*
>>>        * No calls involving ACPI code should go between the setting of
>>>        * SYS_STATE_boot and vm_init() (or else acpi_os_{,un}map_memory()
>>>
>>> It leads to crash on both Arm and x86.
>>>
>>> For the Arm crash:
>>>
>>> (XEN) Data Abort Trap. Syndrome=0x1c08006
>>> (XEN) Walking Hypervisor VA 0x10 on CPU0 via TTBR 0x0000000065a7f000
>>> (XEN) 0TH[0x0] = 0x0000000065a7ef7f
>>> (XEN) 1ST[0x0] = 0x0000000065a7bf7f
>>> (XEN) 2ND[0x0] = 0x0000000000000000
>>> (XEN) CPU0: Unexpected Trap: Data Abort
>>> (XEN) ----[ Xen-4.15-unstable  arm64  debug=y   Not tainted ]----
>>> (XEN) CPU:    0
>>> (XEN) PC:     0000000000219674 dump_execstate+0x58/0x1ec
>>> (XEN) LR:     00000000002d77dc
>>> (XEN) SP:     000000000030fdc0
>>> (XEN) CPSR:   800003c9 MODE:64-bit EL2h (Hypervisor, handler)
>>> (XEN)      X0: 0000000000000000  X1: 0000000000000000  X2: 
>>> 0000000000007fff
>>> (XEN)      X3: 00000000002b7198  X4: 0000000000000080  X5: 
>>> 00000000002e9a68
>>> (XEN)      X6: 0080808080808080  X7: fefefefefefeff09  X8: 
>>> 7f7f7f7f7f7f7f7f
>>> (XEN)      X9: 717164616f726051 X10: 7f7f7f7f7f7f7f7f X11: 
>>> 0101010101010101
>>> (XEN)     X12: 0000000000000008 X13: 00000000002b9a48 X14: 
>>> 0000000000000000
>>> (XEN)     X15: 0000000000400000 X16: 00000000002ba000 X17: 
>>> 00000000002b9000
>>> (XEN)     X18: 00000000002b9000 X19: 0000000000000000 X20: 
>>> 000000000030feb0
>>> (XEN)     X21: 0000000080000000 X22: 00000000002f0d30 X23: 
>>> 00000000002f1d68
>>> (XEN)     X24: 00000000002f0eb8 X25: 0000000040000000 X26: 
>>> 0000000080000000
>>> (XEN)     X27: 0000000000000018 X28: 000000000003f970  FP: 
>>> 000000000030fdc0
>>> (XEN)
>>> (XEN)   VTCR_EL2: 00000000
>>> (XEN)  VTTBR_EL2: 0000000000000000
>>> (XEN)
>>> (XEN)  SCTLR_EL2: 30cd183d
>>> (XEN)    HCR_EL2: 0000000000000038
>>> (XEN)  TTBR0_EL2: 0000000065a7f000
>>> (XEN)
>>> (XEN)    ESR_EL2: 97c08006
>>> (XEN)  HPFAR_EL2: 0000000000000000
>>> (XEN)    FAR_EL2: 0000000000000010
>>> (XEN)
>>> (XEN) Xen stack trace from sp=000000000030fdc0:
>>> (XEN)    000000000030fdf0 00000000002d77dc 0000000000080000 
>>> 000000007fffc000
>>> (XEN)    0000000080000000 00000000002f0d30 000000007f68b250 
>>> 00000000002001b8
>>> (XEN)    0000000065932000 0000000065732000 00000000784f9000 
>>> 0000000000000000
>>> (XEN)    0000000000400000 0000000065a2ad30 0000000000000630 
>>> 0000000000000001
>>> (XEN)    0000000000000001 0000000000000001 0000000000000000 
>>> 0000000000003000
>>> (XEN)    00000000784f9000 00000000002bc8e4 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000300000000 0000000000000000 
>>> 00000040ffffffff
>>> (XEN)    00000000ffffffff 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN) Xen call trace:
>>> (XEN)    [<0000000000219674>] dump_execstate+0x58/0x1ec (PC)
>>> (XEN)    [<00000000002d77dc>] start_xen+0x3d0/0xcf8 (LR)
>>> (XEN)    [<00000000002d77dc>] start_xen+0x3d0/0xcf8
>>> (XEN)    [<00000000002001b8>] arm64/head.o#primary_switched+0x10/0x30
>>> (XEN)
>>> (XEN)
>>> (XEN) ****************************************
>>> (XEN) Panic on CPU 0:
>>> (XEN) CPU0: Unexpected Trap: Data Abort
>>> (XEN) ****************************************
>>>
>>> For the x86 crash:
>>>
>>> (XEN) Early fatal page fault at e008:ffff82d0402188b4 
>>> (cr2=0000000000000010, ec=0000)
>>> (XEN) ----[ Xen-4.15-unstable  x86_64  debug=y   Tainted:   C   ]----
>>> (XEN) CPU:    0
>>> (XEN) RIP:    e008:[<ffff82d0402188b4>] dump_execstate+0x42/0x167
>>> (XEN) RFLAGS: 0000000000010086   CONTEXT: hypervisor
>>> (XEN) rax: 0000000000000000   rbx: 0000000000000000   rcx: 
>>> 0000000000000000
>>> (XEN) rdx: ffff82d0404affff   rsi: 000000000000000a   rdi: 
>>> ffff82d0404afef8
>>> (XEN) rbp: ffff82d0404afd90   rsp: ffff82d0404afd80   r8: 
>>> 0000000000000004
>>> (XEN) r9:  0101010101010101   r10: 0f0f0f0f0f0f0f0f   r11: 
>>> 5555555555555555
>>> (XEN) r12: ffff82d0404afef8   r13: 0000000000800163   r14: 
>>> ffff83000009dfb0
>>> (XEN) r15: 0000000000000002   cr0: 0000000080050033   cr4: 
>>> 00000000000000a0
>>> (XEN) cr3: 00000000bfa9e000   cr2: 0000000000000010
>>> (XEN) fsb: 0000000000000000   gsb: 0000000000000000   gss: 
>>> 0000000000000000
>>> (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: 0000   cs: e008
>>> (XEN) Xen code around <ffff82d0402188b4> (dump_execstate+0x42/0x167):
>>> (XEN)  ff 7f 00 00 48 8b 40 c9 <48> 8b 40 10 66 81 38 ff 7f 75 49 3b 
>>> 1d 23 18 27
>>> (XEN) Xen stack trace from rsp=ffff82d0404afd80:
>>> (XEN)    000000000023ffff 00000000000005ed ffff82d0404afee8 
>>> ffff82d0404378cb
>>> (XEN)    0000000000000002 0000000000000002 0000000000000002 
>>> 0000000000000001
>>> (XEN)    0000000000000001 0000000000000001 0000000000000001 
>>> 0000000000000000
>>> (XEN)    00000000000001ff 0000000002a45fff 0000000000240000 
>>> 0000000002a45000
>>> (XEN)    0000000000100000 0000000000000000 00000000000001ff 
>>> ffff82d040475c80
>>> (XEN)    ffff82d000800163 ffff83000009dee0 ffff83000009dfb0 
>>> 0000000000200001
>>> (XEN)    0000000100000000 0000000100000000 ffff83000009df80 
>>> 642ded38bf9fe4f3
>>> (XEN)    bf9fed3500000000 bfaafe980009df73 0009df73bf9fe7ea 
>>> 00000004bf9fed31
>>> (XEN)    bfaafeb00009df01 0000000800000000 000000010000006e 
>>> 0000000000000003
>>> (XEN)    00000000000002f8 ffff82d0405b0000 ffff82d0404b0000 
>>> 0000000000000002
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 ffff82d04020012f 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN)    0000e01000000000 0000000000000000 0000000000000000 
>>> 00000000000000a0
>>> (XEN)    0000000000000000 0000000000000000 0000000000000000 
>>> 0000000000000000
>>> (XEN) Xen call trace:
>>> (XEN)    [<ffff82d0402188b4>] R dump_execstate+0x42/0x167
>>> (XEN)    [<ffff82d0404378cb>] F __start_xen+0x1e10/0x2906
>>> (XEN)    [<ffff82d04020012f>] F __high_start+0x8f/0x91
>>> (XEN)
>>> (XEN) Pagetable walk from 0000000000000010:
>>> (XEN)  L4[0x000] = 00000000bfa54063 ffffffffffffffff
>>> (XEN)  L3[0x000] = 00000000bfa50063 ffffffffffffffff
>>> (XEN)  L2[0x000] = 00000000bfa4f063 ffffffffffffffff
>>> (XEN)  L1[0x000] = 0000000000000000 ffffffffffffffff
>>> (XEN)
>>> (XEN) ****************************************
>>> (XEN) Panic on CPU 0:
>>> (XEN) FATAL TRAP: vec 14, #PF[0000] IN INTERRUPT CONTEXT
>>> (XEN) ****************************************
>>> (XEN)
>>> (XEN) Reboot in five seconds...
>>>
>>> So I think guest_cpu_user_regs() is not quite yet ready to be called 
>>> from panic().
>>
>> guest_cpu_user_regs() isn't the problem, but dump_execstate().
>>
>> This is one of the caveats from the added boot parameter doc: some debug
>> keys might lead to problems. 'd' seems to be such a key when used for
>> the panic() case and the panic() happens in early boot.
> 
> Right, I think we should be clearer in the documentation about the keys 
> we know works.
> 
>>
>>>
>>> A different approach my be to generate an exception and call the 
>>> keyhandler from there. At least you know that the register would 
>>> always be accurate.
>>
>> Or dump_execstate() is modified to accept NULL for regs and it will do
>> nothing in case guest_cpu_user_regs() isn't valid (a test for idle vcpu
>> might be the easiest way to determine that).
> 
> So Jan pointed out that current may not be initialized properly in early 
> boot. So we possibly want to use "system_state" instead.

Fine with me. Will send an updated patch.


Juergen
Re: [PATCH v3] xen: add support for automatic debug key actions in case of crash
Posted by Jan Beulich 3 years, 4 months ago 
On 11.12.2020 11:22, Julien Grall wrote:
> Hi,
> 
> On 11/12/2020 07:24, Jan Beulich wrote:
>> On 11.12.2020 08:02, Jürgen Groß wrote:
>>> On 10.12.20 21:51, Julien Grall wrote:
>>>> Hi Jan,
>>>>
>>>> On 09/12/2020 14:29, Jan Beulich wrote:
>>>>> On 09.12.2020 13:11, Julien Grall wrote:
>>>>>> On 26/11/2020 11:20, Jan Beulich wrote:
>>>>>>> On 26.11.2020 09:03, Juergen Gross wrote:
>>>>>>>> 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
>>>>>>>>
>>>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>>>
>>>>>>> Except for the Arm aspect, where I'm not sure using
>>>>>>> guest_cpu_user_regs() is correct in all cases,
>>>>>>
>>>>>> I am not entirely sure to understand what get_irq_regs() is supposed to
>>>>>> returned on x86. Is it the registers saved from the most recent
>>>>>> exception?
>>>>>
>>>>> An interrupt (not an exception) sets the underlying per-CPU
>>>>> variable, such that interested parties will know the real
>>>>> context is not guest or "normal" Xen code, but an IRQ.
>>>>
>>>> Thanks for the explanation. I am a bit confused to why we need to give a
>>>> regs to handle_keypress() because no-one seems to use it. Do you have an
>>>> explanation?
>>>
>>> dump_registers() (key 'd') is using it.
>>>
>>>>
>>>> To add to the confusion, it looks like that get_irqs_regs() may return
>>>> NULL. So sometimes we may pass guest_cpu_regs() (which may contain
>>>> garbagge or a set too far).
>>>
>>> I guess this is a best effort approach.
>>
>> Indeed. If there are ways to make it "more best", we should of
>> course follow them. (Except before Dom0 starts, I'm afraid I
>> don't see though where garbage would come from. And even then,
>> just like for the idle vCPU-s, it shouldn't really be garbage,
>> or else this suggests missing initialization somewhere.)
> 
> So I decided to mimick what 'd' does to see what happen if this is 
> called early boot.

But this isn't really relevant here: If you need to deal with a
crash during boot, just don't specify these command line options
(and that's on top of Jürgen's indication that 'd' may not be
very useful to specify here anyway, albeit now that I think
about this I'm not so sure anymore - panic() only logs the local
CPUs registers iirc, while 'd' would log everyone's). Of course
Jürgen could go and limit honoring of the option to sufficiently
high SYS_STATE_*. In particular at least the x86 crash you've
observed is - afaict - from the is_idle_vcpu(current) check in
dump_execstate(), which requires init_idle_domain() to have run
before.

Jan

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.