On Wed, May 03 2023 at 18:41, Eric DeVolder wrote:yep
In the patch 'kexec: exclude elfcorehdr from the segment digest'
See reply to 8/8
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 53bab123a8ee..80538524c494 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2119,6 +2119,19 @@ config CRASH_DUMP
(CONFIG_RELOCATABLE=y).
For more details see Documentation/admin-guide/kdump/kdump.rst
+config CRASH_HOTPLUG
+ bool "Update the crash elfcorehdr on system configuration changes"
+ default y
+ depends on CRASH_DUMP && (HOTPLUG_CPU || MEMORY_HOTPLUG)
+ help
+ Enable direct update to the crash elfcorehdr (which contains
+ the list of CPUs and memory regions to be dumped upon a crash)
+ in response to hot plug/unplug or online/offline of CPUs or
+ memory. This is a much more advanced approach than userspace
+ attempting that.
+
+ If unsure, say Y.
Why is this config an X86 specific thing?
Neither CRASH_DUMP nor HOTPLUG_CPU nor MEMORY_HOTPLUG are in any way X86
specific at all. So why can't you stick that into a place where it can
be reused by other architectures?
It's not rocket science to do
+ depends on WANTS_CRASH_HOTPLUG && CRASH_DUMP && (HOTPLUG_CPU || MEMORY_HOTPLUG)
or something like that. It's so tiring to have x86 Kconfig be the dump
ground for the initial implementation, then having the sh*t copied to
every other architecture and the cleanup is left to the maintainers.
It's not rocket science to differentiate between a real architecture
specific option and a generally useful option in the first place, right?
+#ifdef CONFIG_CRASH_HOTPLUG
+ /*
+ * Ensure the elfcorehdr segment large enough for hotplug changes.
+ * Account for VMCOREINFO and kernel_map and maximum CPUs.
Neither the first line nor the second one qualifies as parseable sentences.
How about this:+/**
+ * arch_crash_handle_hotplug_event() - Handle hotplug elfcorehdr changes
+ * @image: the active struct kimage
What is an active struct kimage?
How about:+ *
+ * The new elfcorehdr is prepared in a kernel buffer, and then it is
+ * written on top of the existing/old elfcorehdr.
-ENOPARSE
An error in the crash elfcorehdr infrastructure introduced in this series+ */
+void arch_crash_handle_hotplug_event(struct kimage *image)
+{
+ void *elfbuf = NULL, *old_elfcorehdr;
+ unsigned long nr_mem_ranges;
+ unsigned long mem, memsz;
+ unsigned long elfsz = 0;
+
+ /*
+ * Create the new elfcorehdr reflecting the changes to CPU and/or
+ * memory resources.
+ */
+ if (prepare_elf_headers(image, &elfbuf, &elfsz, &nr_mem_ranges)) {
+ pr_err("unable to prepare elfcore headers");
+ goto out;
So this can fail. Why is there just a pr_err() and no return value which
tells the caller that this failed?
How about:
+ /*
+ * Copy new elfcorehdr over the old elfcorehdr at destination.
+ */
+ old_elfcorehdr = kmap_local_page(pfn_to_page(mem >> PAGE_SHIFT));
+ if (!old_elfcorehdr) {
+ pr_err("updating elfcorehdr failed\n");
How hard is it to write an error message which is clearly describing the
problem?
Thanks,Again, thanks for the fresh eyes!
tglx