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

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

On 29.10.20 15:49, Jan Beulich wrote:
> On 29.10.2020 15:40, Jürgen Groß wrote:
> > On 29.10.20 15:22, Jan Beulich wrote:
> > > On 22.10.2020 16:39, Juergen Gross wrote:
> > > > @@ -507,6 +509,41 @@ 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] = {
> > > > +    [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);
> > > > +string_runtime_param("crash-debug-debugkey", crash_debug_debugkey);
> > > In general I'm not in favor of this (or similar) needing
> > > five new command line options, instead of just one. The problem
> > > with e.g.
> > >
> > > crash-debug=panic:rq,watchdog:0d
> > >
> > > is that ',' (or any other separator chosen) could in principle
> > > also be a debug key. It would still be possible to use
> > >
> > > crash-debug=panic:rq crash-debug=watchdog:0d
> > >
> > > though. Thoughts?
> > OTOH the runtime parameters are much easier addressable that way.
> Ah yes, I can see this as a reason. Would make we wonder whether
> command line and runtime handling may want disconnecting in this
> specific case then. (But I can also see the argument of this
> being too much overhead then.)
>
> > > > +void keyhandler_crash_action(enum crash_reason reason)
> > > > +{
> > > > +    const char *action = crash_action[reason];
> > > > +    struct cpu_user_regs *regs = get_irq_regs() ? : guest_cpu_user_regs();
> > > > +
> > > > +    while ( *action ) {
> > > > +        if ( *action == '.' )
> > > > +            mdelay(1000);
> > > > +        else
> > > > +            handle_keypress(*action, regs);
> > > > +        action++;
> > > > +    }
> > > > +}
> > > I think only diagnostic keys should be permitted here.
> > While I understand that other keys could produce nonsense or do harm,
> > I'm not sure we should really prohibit them. Allowing them would e.g.
> > allow to do just a reboot without kdump for one type of crashes.
> Ah yes, that's a fair point.
>
> > > > --- 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>
> > > Could we go without this, utilizing the gcc extension of forward
> > > declared enums? Otoh ...
> > >
> > > > @@ -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);
> > > > +}
> > > ... if this is to be an inline function and not just a #define,
> > > it'll need the declaration of the function to have been seen.
> > And even being a #define all users of kexec_crash() would need to
> > #include keyhandler.h (and I'm not sure there are any source files
> > including kexec.h which don't use kexec_crash()).
> About as many which do as ones which don't. But there's no
> generally accessible header which includes xen/kexec.h, so perhaps
> the extra dependency indeed isn't all this problematic.
Any further comments, or even better, Acks?


Juergen

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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature