Re: [PATCH v22 5/8] x86/crash: add x86 crash hotplug support

[Eric DeVolder]

On 5/9/23 17:52, Thomas Gleixner wrote:
> On Wed, May 03 2023 at 18:41, Eric DeVolder wrote:
> > In the patch 'kexec: exclude elfcorehdr from the segment digest'
>
> See reply to 8/8
yep

> > 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?

Right. To your point, CRASH_DUMP has been copied in all the archs:
arch/arm/Kconfig:config CRASH_DUMP
arch/arm64/Kconfig:config CRASH_DUMP
arch/ia64/Kconfig:config CRASH_DUMP
arch/mips/Kconfig:config CRASH_DUMP
arch/powerpc/Kconfig:config CRASH_DUMP
arch/riscv/Kconfig:config CRASH_DUMP
arch/s390/Kconfig:config CRASH_DUMP
arch/sh/Kconfig:config CRASH_DUMP
arch/x86/Kconfig:config CRASH_DUMP
arch/loongarch/Kconfig:config CRASH_DUMP

Likewise for KEXEC and KEXEC_FILE.

I've looked into this in the past, and looking again today, I don't see a natural
place to put the option. Perhaps starting a kernel/Kconfig.kexec?

> > +#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.

What about:
Ensure the elfcorehdr segment is large enough for hotplug changes.
The segment size accounts for VMCOREINFO, kernel_map, maximum CPUs
and maximum memory ranges.


> > +/**
> > + * arch_crash_handle_hotplug_event() - Handle hotplug elfcorehdr changes
> > + * @image: the active struct kimage
>
> What is an active struct kimage?
How about this:
@image: a pointer to kexec_crash_image

> > + *
> > + * The new elfcorehdr is prepared in a kernel buffer, and then it is
> > + * written on top of the existing/old elfcorehdr.
>
> -ENOPARSE
How about:
Prepare the new elfcorehdr and replace the existing elfcorehdr.

> > + */
> > +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?
An error in the crash elfcorehdr infrastructure introduced in this series
is not a reason to rollback state. The cpuhp and memory notifier callbacks
always return an OK.
The primary errors that might occur are failure to obtain the kexec_lock,
and failure to obtain a temporary kernel buffer to stage the new elfcorehdr.
How about:
pr_err("prepare_elf_headers() failed");

> > +	/*
> > +	 * 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?
How about:
pr_err("mapping elfcorehdr segment failed");

> Thanks,
>
>          tglx
Again, thanks for the fresh eyes!
eric