[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.