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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
> ---
> 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@xxxxxxxx>

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@xxxxxxxx>

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