From: Thomas Gleixner <tglx@linutronix.de>
To: Eric DeVolder <eric.devolder@oracle.com>,
linux-kernel@vger.kernel.org, x86@kernel.org,
kexec@lists.infradead.org, ebiederm@xmission.com,
dyoung@redhat.com, bhe@redhat.com, vgoyal@redhat.com
Cc: mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com,
hpa@zytor.com, nramas@linux.microsoft.com,
thomas.lendacky@amd.com, robh@kernel.org, efault@gmx.de,
rppt@kernel.org, david@redhat.com, sourabhjain@linux.ibm.com,
konrad.wilk@oracle.com, boris.ostrovsky@oracle.com,
eric.devolder@oracle.com
Subject: Re: [PATCH v22 5/8] x86/crash: add x86 crash hotplug support
Date: Wed, 10 May 2023 00:52:36 +0200 [thread overview]
Message-ID: <875y91yv63.ffs@tglx> (raw)
In-Reply-To: <20230503224145.7405-6-eric.devolder@oracle.com>
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
> 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.
> +/**
> + * arch_crash_handle_hotplug_event() - Handle hotplug elfcorehdr changes
> + * @image: the active struct kimage
What is an active struct kimage?
> + *
> + * The new elfcorehdr is prepared in a kernel buffer, and then it is
> + * written on top of the existing/old elfcorehdr.
-ENOPARSE
> + */
> +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?
> + /*
> + * 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,
tglx
next prev parent reply other threads:[~2023-05-09 22:52 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-03 22:41 [PATCH v22 0/8] crash: Kernel handling of CPU and memory hot un/plug Eric DeVolder
2023-05-03 22:41 ` [PATCH v22 1/8] crash: move a few code bits to setup support of crash hotplug Eric DeVolder
2023-05-03 22:41 ` [PATCH v22 2/8] crash: add generic infrastructure for crash hotplug support Eric DeVolder
2023-05-03 22:41 ` [PATCH v22 3/8] kexec: exclude elfcorehdr from the segment digest Eric DeVolder
2023-05-03 22:41 ` [PATCH v22 4/8] crash: memory and CPU hotplug sysfs attributes Eric DeVolder
2023-05-03 22:41 ` [PATCH v22 5/8] x86/crash: add x86 crash hotplug support Eric DeVolder
2023-05-09 22:52 ` Thomas Gleixner [this message]
2023-05-10 22:49 ` Eric DeVolder
2023-05-03 22:41 ` [PATCH v22 6/8] crash: hotplug support for kexec_load() Eric DeVolder
2023-05-08 5:13 ` Baoquan He
2023-05-09 6:15 ` Sourabh Jain
2023-05-10 22:52 ` Eric DeVolder
2023-05-09 6:56 ` Sourabh Jain
2023-05-09 20:35 ` Eric DeVolder
2023-05-03 22:41 ` [PATCH v22 7/8] crash: change crash_prepare_elf64_headers() to for_each_possible_cpu() Eric DeVolder
2023-05-03 22:41 ` [PATCH v22 8/8] x86/crash: optimize CPU changes Eric DeVolder
2023-05-09 22:39 ` Thomas Gleixner
2023-05-10 22:49 ` Eric DeVolder
2023-05-04 6:22 ` [PATCH v22 0/8] crash: Kernel handling of CPU and memory hot un/plug Hari Bathini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=875y91yv63.ffs@tglx \
--to=tglx@linutronix.de \
--cc=bhe@redhat.com \
--cc=boris.ostrovsky@oracle.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=david@redhat.com \
--cc=dyoung@redhat.com \
--cc=ebiederm@xmission.com \
--cc=efault@gmx.de \
--cc=eric.devolder@oracle.com \
--cc=hpa@zytor.com \
--cc=kexec@lists.infradead.org \
--cc=konrad.wilk@oracle.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=nramas@linux.microsoft.com \
--cc=robh@kernel.org \
--cc=rppt@kernel.org \
--cc=sourabhjain@linux.ibm.com \
--cc=thomas.lendacky@amd.com \
--cc=vgoyal@redhat.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).