[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <36c4a86d-7b91-24c7-bdad-0f3ee1c2b7e7@oracle.com>
Date: Tue, 15 Aug 2023 09:40:32 -0500
From: Eric DeVolder <eric.devolder@...cle.com>
To: Sourabh Jain <sourabhjain@...ux.ibm.com>,
linux-kernel@...r.kernel.org, david@...hat.com, osalvador@...e.de,
corbet@....net, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, x86@...nel.org, bhe@...hat.com,
ebiederm@...ssion.com, kexec@...ts.infradead.org
Cc: hpa@...or.com, gregkh@...uxfoundation.org, rafael@...nel.org,
vgoyal@...hat.com, dyoung@...hat.com, lf32.dev@...il.com,
akpm@...ux-foundation.org, naveen.n.rao@...ux.vnet.ibm.com,
zohar@...ux.ibm.com, bhelgaas@...gle.com, vbabka@...e.cz,
tiwai@...e.de, seanjc@...gle.com, linux@...ssschuh.net,
vschneid@...hat.com, linux-mm@...ck.org, linux-doc@...r.kernel.org,
konrad.wilk@...cle.com, boris.ostrovsky@...cle.com
Subject: Re: [PATCH v27 2/8] crash: add generic infrastructure for crash
hotplug support
On 8/12/23 05:47, Sourabh Jain wrote:
> Hello Eric,
>
> On 11/08/23 22:36, Eric DeVolder wrote:
>> To support crash hotplug, a mechanism is needed to update the crash
>> elfcorehdr upon CPU or memory changes (eg. hot un/plug or off/
>> onlining). The crash elfcorehdr describes the CPUs and memory to
>> be written into the vmcore.
>>
>> To track CPU changes, callbacks are registered with the cpuhp
>> mechanism via cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN). The
>> crash hotplug elfcorehdr update has no explicit ordering requirement
>> (relative to other cpuhp states), so meets the criteria for
>> utilizing CPUHP_BP_PREPARE_DYN. CPUHP_BP_PREPARE_DYN is a dynamic
>> state and avoids the need to introduce a new state for crash
>> hotplug. Also, CPUHP_BP_PREPARE_DYN is the last state in the PREPARE
>> group, just prior to the STARTING group, which is very close to the
>> CPU starting up in a plug/online situation, or stopping in a unplug/
>> offline situation. This minimizes the window of time during an
>> actual plug/online or unplug/offline situation in which the
>> elfcorehdr would be inaccurate. Note that for a CPU being unplugged
>> or offlined, the CPU will still be present in the list of CPUs
>> generated by crash_prepare_elf64_headers(). However, there is no
>> need to explicitly omit the CPU, see justification in
>> 'crash: change crash_prepare_elf64_headers() to for_each_possible_cpu()'.
>>
>> To track memory changes, a notifier is registered to capture the
>> memblock MEM_ONLINE and MEM_OFFLINE events via register_memory_notifier().
>>
>> The CPU callbacks and memory notifiers invoke crash_handle_hotplug_event()
>> which performs needed tasks and then dispatches the event to the
>> architecture specific arch_crash_handle_hotplug_event() to update the
>> elfcorehdr with the current state of CPUs and memory. During the
>> process, the kexec_lock is held.
>>
>> Signed-off-by: Eric DeVolder <eric.devolder@...cle.com>
>> Reviewed-by: Sourabh Jain <sourabhjain@...ux.ibm.com>
>> Acked-by: Hari Bathini <hbathini@...ux.ibm.com>
>> Acked-by: Baoquan He <bhe@...hat.com>
>> ---
>> include/linux/crash_core.h | 9 +++
>> include/linux/kexec.h | 11 +++
>> kernel/Kconfig.kexec | 31 ++++++++
>> kernel/crash_core.c | 142 +++++++++++++++++++++++++++++++++++++
>> kernel/kexec_core.c | 6 ++
>> 5 files changed, 199 insertions(+)
>>
>> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
>> index de62a722431e..e14345cc7a22 100644
>> --- a/include/linux/crash_core.h
>> +++ b/include/linux/crash_core.h
>> @@ -84,4 +84,13 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
>> int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
>> unsigned long long *crash_size, unsigned long long *crash_base);
>> +#define KEXEC_CRASH_HP_NONE 0
>> +#define KEXEC_CRASH_HP_ADD_CPU 1
>> +#define KEXEC_CRASH_HP_REMOVE_CPU 2
>> +#define KEXEC_CRASH_HP_ADD_MEMORY 3
>> +#define KEXEC_CRASH_HP_REMOVE_MEMORY 4
>> +#define KEXEC_CRASH_HP_INVALID_CPU -1U
>> +
>> +struct kimage;
>> +
>> #endif /* LINUX_CRASH_CORE_H */
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index 811a90e09698..b9903dd48e24 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -33,6 +33,7 @@ extern note_buf_t __percpu *crash_notes;
>> #include <linux/compat.h>
>> #include <linux/ioport.h>
>> #include <linux/module.h>
>> +#include <linux/highmem.h>
>> #include <asm/kexec.h>
>> /* Verify architecture specific macros are defined */
>> @@ -360,6 +361,12 @@ struct kimage {
>> struct purgatory_info purgatory_info;
>> #endif
>> +#ifdef CONFIG_CRASH_HOTPLUG
>> + int hp_action;
>> + int elfcorehdr_index;
>> + bool elfcorehdr_updated;
>> +#endif
>> +
>> #ifdef CONFIG_IMA_KEXEC
>> /* Virtual address of IMA measurement buffer for kexec syscall */
>> void *ima_buffer;
>> @@ -490,6 +497,10 @@ static inline int arch_kexec_post_alloc_pages(void *vaddr, unsigned int pages, g
>> static inline void arch_kexec_pre_free_pages(void *vaddr, unsigned int pages) { }
>> #endif
>> +#ifndef arch_crash_handle_hotplug_event
>> +static inline void arch_crash_handle_hotplug_event(struct kimage *image) { }
>> +#endif
>> +
>
> Isn't the above function should be declare under CONFIG_CRASH_HOTPLUG?
>
> Thanks,
> Sourabh
>
There are no compiler warnings/errors, due to the nature of being declared static inline.
And most of the other functions defined in a similar way in this file are not guard banded
by CONFIG ifdefs. I'm inclined to leave it this way.
Thanks!
eric
>> #else /* !CONFIG_KEXEC_CORE */
>> struct pt_regs;
>> struct task_struct;
>> diff --git a/kernel/Kconfig.kexec b/kernel/Kconfig.kexec
>> index ff72e45cfaef..d0a9a5392035 100644
>> --- a/kernel/Kconfig.kexec
>> +++ b/kernel/Kconfig.kexec
>> @@ -113,4 +113,35 @@ config CRASH_DUMP
>> For s390, this option also enables zfcpdump.
>> See also <file:Documentation/s390/zfcpdump.rst>
>> +config CRASH_HOTPLUG
>> + bool "Update the crash elfcorehdr on system configuration changes"
>> + default y
>> + depends on CRASH_DUMP && (HOTPLUG_CPU || MEMORY_HOTPLUG)
>> + depends on ARCH_SUPPORTS_CRASH_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.
>> +
>> +config CRASH_MAX_MEMORY_RANGES
>> + int "Specify the maximum number of memory regions for the elfcorehdr"
>> + default 8192
>> + depends on CRASH_HOTPLUG
>> + help
>> + For the kexec_file_load() syscall path, specify the maximum number of
>> + memory regions that the elfcorehdr buffer/segment can accommodate.
>> + These regions are obtained via walk_system_ram_res(); eg. the
>> + 'System RAM' entries in /proc/iomem.
>> + This value is combined with NR_CPUS_DEFAULT and multiplied by
>> + sizeof(Elf64_Phdr) to determine the final elfcorehdr memory buffer/
>> + segment size.
>> + The value 8192, for example, covers a (sparsely populated) 1TiB system
>> + consisting of 128MiB memblocks, while resulting in an elfcorehdr
>> + memory buffer/segment size under 1MiB. This represents a sane choice
>> + to accommodate both baremetal and virtual machine configurations.
>> +
>> endmenu
>> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
>> index b7c30b748a16..53d211c690a1 100644
>> --- a/kernel/crash_core.c
>> +++ b/kernel/crash_core.c
>> @@ -11,6 +11,8 @@
>> #include <linux/vmalloc.h>
>> #include <linux/sizes.h>
>> #include <linux/kexec.h>
>> +#include <linux/memory.h>
>> +#include <linux/cpuhotplug.h>
>> #include <asm/page.h>
>> #include <asm/sections.h>
>> @@ -18,6 +20,7 @@
>> #include <crypto/sha1.h>
>> #include "kallsyms_internal.h"
>> +#include "kexec_internal.h"
>> /* vmcoreinfo stuff */
>> unsigned char *vmcoreinfo_data;
>> @@ -697,3 +700,142 @@ static int __init crash_save_vmcoreinfo_init(void)
>> }
>> subsys_initcall(crash_save_vmcoreinfo_init);
>> +
>> +#ifdef CONFIG_CRASH_HOTPLUG
>> +#undef pr_fmt
>> +#define pr_fmt(fmt) "crash hp: " fmt
>> +/*
>> + * To accurately reflect hot un/plug changes of cpu and memory resources
>> + * (including onling and offlining of those resources), the elfcorehdr
>> + * (which is passed to the crash kernel via the elfcorehdr= parameter)
>> + * must be updated with the new list of CPUs and memories.
>> + *
>> + * In order to make changes to elfcorehdr, two conditions are needed:
>> + * First, the segment containing the elfcorehdr must be large enough
>> + * to permit a growing number of resources; the elfcorehdr memory size
>> + * is based on NR_CPUS_DEFAULT and CRASH_MAX_MEMORY_RANGES.
>> + * Second, purgatory must explicitly exclude the elfcorehdr from the
>> + * list of segments it checks (since the elfcorehdr changes and thus
>> + * would require an update to purgatory itself to update the digest).
>> + */
>> +static void crash_handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
>> +{
>> + struct kimage *image;
>> +
>> + /* Obtain lock while changing crash information */
>> + if (!kexec_trylock()) {
>> + pr_info("kexec_trylock() failed, elfcorehdr may be inaccurate\n");
>> + return;
>> + }
>> +
>> + /* Check kdump is not loaded */
>> + if (!kexec_crash_image)
>> + goto out;
>> +
>> + image = kexec_crash_image;
>> +
>> + if (hp_action == KEXEC_CRASH_HP_ADD_CPU ||
>> + hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
>> + pr_debug("hp_action %u, cpu %u\n", hp_action, cpu);
>> + else
>> + pr_debug("hp_action %u\n", hp_action);
>> +
>> + /*
>> + * The elfcorehdr_index is set to -1 when the struct kimage
>> + * is allocated. Find the segment containing the elfcorehdr,
>> + * if not already found.
>> + */
>> + if (image->elfcorehdr_index < 0) {
>> + unsigned long mem;
>> + unsigned char *ptr;
>> + unsigned int n;
>> +
>> + for (n = 0; n < image->nr_segments; n++) {
>> + mem = image->segment[n].mem;
>> + ptr = kmap_local_page(pfn_to_page(mem >> PAGE_SHIFT));
>> + if (ptr) {
>> + /* The segment containing elfcorehdr */
>> + if (memcmp(ptr, ELFMAG, SELFMAG) == 0)
>> + image->elfcorehdr_index = (int)n;
>> + kunmap_local(ptr);
>> + }
>> + }
>> + }
>> +
>> + if (image->elfcorehdr_index < 0) {
>> + pr_err("unable to locate elfcorehdr segment");
>> + goto out;
>> + }
>> +
>> + /* Needed in order for the segments to be updated */
>> + arch_kexec_unprotect_crashkres();
>> +
>> + /* Differentiate between normal load and hotplug update */
>> + image->hp_action = hp_action;
>> +
>> + /* Now invoke arch-specific update handler */
>> + arch_crash_handle_hotplug_event(image);
>> +
>> + /* No longer handling a hotplug event */
>> + image->hp_action = KEXEC_CRASH_HP_NONE;
>> + image->elfcorehdr_updated = true;
>> +
>> + /* Change back to read-only */
>> + arch_kexec_protect_crashkres();
>> +
>> + /* Errors in the callback is not a reason to rollback state */
>> +out:
>> + /* Release lock now that update complete */
>> + kexec_unlock();
>> +}
>> +
>> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
>> +{
>> + switch (val) {
>> + case MEM_ONLINE:
>> + crash_handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY,
>> + KEXEC_CRASH_HP_INVALID_CPU);
>> + break;
>> +
>> + case MEM_OFFLINE:
>> + crash_handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY,
>> + KEXEC_CRASH_HP_INVALID_CPU);
>> + break;
>> + }
>> + return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block crash_memhp_nb = {
>> + .notifier_call = crash_memhp_notifier,
>> + .priority = 0
>> +};
>> +
>> +static int crash_cpuhp_online(unsigned int cpu)
>> +{
>> + crash_handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
>> + return 0;
>> +}
>> +
>> +static int crash_cpuhp_offline(unsigned int cpu)
>> +{
>> + crash_handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
>> + return 0;
>> +}
>> +
>> +static int __init crash_hotplug_init(void)
>> +{
>> + int result = 0;
>> +
>> + if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
>> + register_memory_notifier(&crash_memhp_nb);
>> +
>> + if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) {
>> + result = cpuhp_setup_state_nocalls(CPUHP_BP_PREPARE_DYN,
>> + "crash/cpuhp", crash_cpuhp_online, crash_cpuhp_offline);
>> + }
>> +
>> + return result;
>> +}
>> +
>> +subsys_initcall(crash_hotplug_init);
>> +#endif
>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
>> index e2f2574d8b74..5d323255862a 100644
>> --- a/kernel/kexec_core.c
>> +++ b/kernel/kexec_core.c
>> @@ -277,6 +277,12 @@ struct kimage *do_kimage_alloc_init(void)
>> /* Initialize the list of unusable pages */
>> INIT_LIST_HEAD(&image->unusable_pages);
>> +#ifdef CONFIG_CRASH_HOTPLUG
>> + image->hp_action = KEXEC_CRASH_HP_NONE;
>> + image->elfcorehdr_index = -1;
>> + image->elfcorehdr_updated = false;
>> +#endif
>> +
>> return image;
>> }
>
Powered by blists - more mailing lists