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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.11.2020 14:13, Juergen Gross wrote:
> @@ -507,6 +509,42 @@ void __init initialize_keytable(void)
>      }
>  }
>  
> +#define CRASHACTION_SIZE  32
> +static char crash_debug_panic[CRASHACTION_SIZE];
> +static char crash_debug_hwdom[CRASHACTION_SIZE];
> +static char crash_debug_watchdog[CRASHACTION_SIZE];
> +static char crash_debug_kexeccmd[CRASHACTION_SIZE];
> +static char crash_debug_debugkey[CRASHACTION_SIZE];
> +
> +static char *crash_action[CRASHREASON_N] = {

Considering the sole use below, I think there can be two "const"
added here. With this single use I also wonder whether this
array wouldn't better be private to that function.

> +    [CRASHREASON_PANIC] = crash_debug_panic,
> +    [CRASHREASON_HWDOM] = crash_debug_hwdom,
> +    [CRASHREASON_WATCHDOG] = crash_debug_watchdog,
> +    [CRASHREASON_KEXECCMD] = crash_debug_kexeccmd,
> +    [CRASHREASON_DEBUGKEY] = crash_debug_debugkey,
> +};
> +
> +string_runtime_param("crash-debug-panic", crash_debug_panic);
> +string_runtime_param("crash-debug-hwdom", crash_debug_hwdom);
> +string_runtime_param("crash-debug-watchdog", crash_debug_watchdog);
> +string_runtime_param("crash-debug-kexeccmd", crash_debug_kexeccmd);

This one probably wants a CONFIG_KEXEC conditional around it,
such that requests to set it won't appear to be "okay" on !KEXEC
builds. At which point the doc probably also wants to mention the
conditional availability of this option.

> +string_runtime_param("crash-debug-debugkey", crash_debug_debugkey);
> +
> +void keyhandler_crash_action(enum crash_reason reason)
> +{
> +    const char *action = crash_action[reason];

In order to avoid cascade problems when the system's already in
trouble, maybe better to bounds check "reason" before using as
array index and, also with the CONFIG_KEXEC related adjustment
requested above in mind, ...

> +    struct cpu_user_regs *regs = get_irq_regs() ? : guest_cpu_user_regs();
> +
> +    while ( *action )

... perhaps also better to check action against NULL before
de-referencing?

Jan