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

[Jürgen Groß <jgross@xxxxxxxx>]

On 24.11.20 12:27, Jan Beulich wrote:
> 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.
Both fine with me.

> > +    [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.
Yes.

> > +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?
Okay (to both).

And I only now realized that get_irq_regs() is x86 only. I'll add a
dummy Arm function always returning NULL.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature