[RFC v4 PATCH 4/5] powerpc/crash hp: add crash hotplug support for kexec_file_load
Eric DeVolder
eric.devolder at oracle.com
Fri Apr 22 01:33:26 AEST 2022
On 4/19/22 03:31, Sourabh Jain wrote:
>
> On 14/04/22 22:02, Laurent Dufour wrote:
>> On 11/04/2022, 10:43:56, Sourabh Jain wrote:
>>> Two major changes are done to enable the crash CPU hotplug handler.
>>> Firstly, updated the kexec load path to prepare kimage for hotplug
>>> changes, and secondly, implemented the crash hotplug handler.
>>>
>>> On the kexec load path, the memsz allocation of the crash FDT segment
>>> is updated to ensure that it has sufficient buffer space to accommodate
>>> future hot add CPUs. Initialized the kimage members to track the kexec
>>> FDT segment.
>>>
>>> The crash hotplug handler updates the cpus node of crash FDT. While we
>>> update crash FDT the kexec_crash_image is marked invalid and restored
>>> after FDT update to avoid race.
>>>
>>> Since memory crash hotplug support is not there yet the crash hotplug
>>> the handler simply warns the user and returns.
>>>
>>> Signed-off-by: Sourabh Jain <sourabhjain at linux.ibm.com>
>>> ---
>>> arch/powerpc/kexec/core_64.c | 46 ++++++++++++++++++++++
>>> arch/powerpc/kexec/elf_64.c | 74 ++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 120 insertions(+)
>>>
>>> diff --git a/arch/powerpc/kexec/core_64.c b/arch/powerpc/kexec/core_64.c
>>> index 249d2632526d..62f77cc86407 100644
>>> --- a/arch/powerpc/kexec/core_64.c
>>> +++ b/arch/powerpc/kexec/core_64.c
>>> @@ -466,6 +466,52 @@ int update_cpus_node(void *fdt)
>>> return ret;
>>> }
>>> +#ifdef CONFIG_CRASH_HOTPLUG
>>> +/**
>>> + * arch_crash_hotplug_handler() - Handle hotplug FDT changes
>>> + * @image: the active struct kimage
>>> + * @hp_action: the hot un/plug action being handled
>>> + * @a: first parameter dependent upon hp_action
>>> + * @b: first parameter dependent upon hp_action
>>> + *
>>> + * To accurately reflect CPU hot un/plug changes, the FDT
>>> + * must be updated with the new list of CPUs and memories.
>>> + */
>>> +void arch_crash_hotplug_handler(struct kimage *image, unsigned int hp_action,
>>> + unsigned long a, unsigned long b)
>>> +{
>>> + void *fdt;
>>> +
>>> + /* No action needed for CPU hot-unplug */
>>> + if (hp_action == KEXEC_CRASH_HP_REMOVE_CPU)
>>> + return;
>> I should have report since in the previous version too, why nothing is done
>> when CPU are removed?
>
> Since CPU note addresses are already available for all possible CPUs
> (regardless they are online or not) the PHDR is allocated for all possible
> CPUs during elfcorehdr creation. At least for the kexec_load system call.
>
> Now on the crash path, the crashing CPU initiates an IPI call to update
> the CPU data of all online CPUs and jumps to the purgatory. Hence no
> action is needed for the remove case.
>
> With the above logic, we shouldn't be taking any action for the CPU add
> case too, right? But on PowerPC early boot path there is validation that
> checks the boot CPU is part of the Flattened Device Tree (FDT) or not.
> If the boot CPU is not found in FDT the boot fails. Hence FDT needs to be
> updated for every new CPU added to the system but not needed when
> CPU is removed.
>
>
> Thanks
> Sourabh Jain
>
>>
>>> +
>>> + /* crash update on memory hotplug is not support yet */
>>> + if (hp_action == KEXEC_CRASH_HP_REMOVE_MEMORY || hp_action == KEXEC_CRASH_HP_ADD_MEMORY) {
>>> + pr_info_once("crash hp: crash update is not supported with memory hotplug\n");
>>> + return;
>>> + }
Sourabh,
Baoquan's preference is to do away with the hp_action (and a and b) parameters to
this function. As x86_64 has no need for them, I have no reason to argue for keeping
them.
If you really need hp_action, then please respond to Baoquan's concern in the
"[PATCH v7 4/8] crash: add generic infrastructure for crash hotplug support"
thread.
Thanks,
eric
>>> +
>>> + /* Must have valid FDT index */
>>> + if (!image->arch.fdt_index_valid) {
>>> + pr_err("crash hp: unable to locate FDT segment");
>>> + return;
>>> + }
>>> +
>>> + fdt = __va((void *)image->segment[image->arch.fdt_index].mem);
>>> +
>>> + /* Temporarily invalidate the crash image while it is replaced */
>>> + xchg(&kexec_crash_image, NULL);
>>> +
>>> + /* update FDT to refelect changes to CPU resrouces */
>>> + if (update_cpus_node(fdt))
>>> + pr_err("crash hp: failed to update crash FDT");
>>> +
>>> + /* The crash image is now valid once again */
>>> + xchg(&kexec_crash_image, image);
>>> +}
>>> +#endif /* CONFIG_CRASH_HOTPLUG */
>>> +
>>> #ifdef CONFIG_PPC_64S_HASH_MMU
>>> /* Values we need to export to the second kernel via the device tree. */
>>> static unsigned long htab_base;
>>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
>>> index eeb258002d1e..9dc774548ce4 100644
>>> --- a/arch/powerpc/kexec/elf_64.c
>>> +++ b/arch/powerpc/kexec/elf_64.c
>>> @@ -24,6 +24,67 @@
>>> #include <linux/slab.h>
>>> #include <linux/types.h>
>>> +#include <asm/kvm_book3s.h>
>>> +#include <asm/kvm_ppc.h>
>>> +
>>> +#ifdef CONFIG_CRASH_HOTPLUG
>>> +
>>> +/**
>>> + * get_cpu_node_sz() - Calculate the space needed to store a CPU device type node
>>> + * in FDT. The calculation is done based on the existing CPU
>>> + * node in unflatten device tree. Loop through all the
>>> + * properties of the very first CPU type device node found in
>>> + * unflatten device tree and returns the sum of the property
>>> + * length and property string size of all properties of a CPU
>>> + * node.
>>> + */
>> I don't think this is following the indent rules.
>>
>>> +static int get_cpu_node_sz(void) {
>>> + struct device_node *dn = NULL;
>>> + struct property *pp;
>>> + int cpu_node_size = 0;
>>> +
>>> + dn = of_find_node_by_type(NULL, "cpu");
>>> +
>>> + if (!dn) {
>>> + pr_warn("Unable to locate cpu device_type node.\n");
>>> + goto out;
>>> + }
>>> +
>>> + /* Every node in FDT starts with FDT_BEGIN_NODE and ends with
>>> + * FDT_END_NODE that takes one byte each.
>>> + */
>>> + cpu_node_size = 2;
>>> +
>>> + for_each_property_of_node(dn, pp) {
>>> + /* For each property add two bytes extra. One for string null
>>> + * character for property name and other for FDT property start
>>> + * tag FDT_PROP.
>>> + */
>> Coding style request to start with a blank comment line for multiple
>> comment lines.
>>
>>> + cpu_node_size = cpu_node_size + pp->length + strlen(pp->name) + 2;
>>> + }
>>> +
>>> +out:
>>> + return cpu_node_size;
>>> +}
>>> +
>>> +/**
>>> + * get_crash_fdt_mem_sz() - calcuate mem size for crash kernel FDT
>>> + * @fdt: pointer to crash kernel FDT
>>> + *
>>> + * Calculate the buffer space needed to add more CPU nodes in crash FDT
>>> + * post capture kenrel load due to CPU hotplug events.
>>> + */
>>> +static unsigned int get_crash_fdt_mem_sz(void *fdt)
>>> +{
>>> + int fdt_cpu_nodes_sz, offline_cpu_cnt;
>>> +
>>> + offline_cpu_cnt = (num_possible_cpus() - num_present_cpus()) / MAX_SMT_THREADS;
>>> + fdt_cpu_nodes_sz = get_cpu_node_sz() * offline_cpu_cnt;
>>> +
>>> + return fdt_totalsize(fdt) + fdt_cpu_nodes_sz;
>>> +}
>>> +#endif
>>> +
>>> static void *elf64_load(struct kimage *image, char *kernel_buf,
>>> unsigned long kernel_len, char *initrd,
>>> unsigned long initrd_len, char *cmdline,
>>> @@ -123,6 +184,19 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>>> kbuf.buf_align = PAGE_SIZE;
>>> kbuf.top_down = true;
>>> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>>> +
>>> +#ifdef CONFIG_CRASH_HOTPLUG
>>> + if (image->type == KEXEC_TYPE_CRASH) {
>>> + kbuf.memsz = get_crash_fdt_mem_sz(fdt);
>>> + fdt_set_totalsize(fdt, kbuf.memsz);
>>> + image->arch.fdt_index = image->nr_segments;
>>> + image->arch.fdt_index_valid = true;
>>> + } else
>>> +#endif
>>> + {
>>> + kbuf.memsz = fdt_totalsize(fdt);
>>> + }
>>> +
>>> ret = kexec_add_buffer(&kbuf);
>>> if (ret)
>>> goto out_free_fdt;
More information about the Linuxppc-dev
mailing list