[RFC v4 PATCH 4/5] powerpc/crash hp: add crash hotplug support for kexec_file_load

[Eric DeVolder <eric.devolder@oracle.com>]

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@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;