Re: [PATCH v2 5/6] powerpc/crash: use generic crashkernel reservation

Sourabh Jain
2025-01-23T11:53:25Z

Hello Hari,

On 23/01/25 16:15, Hari Bathini wrote:
> Hi Sourabh,
>
> On 21/01/25 5:24 pm, Sourabh Jain wrote:
>> Commit 0ab97169aa05 ("crash_core: add generic function to do
>> reservation") added a generic function to reserve crashkernel memory.
>> So let's use the same function on powerpc and remove the
>> architecture-specific code that essentially does the same thing.
>>
>> The generic crashkernel reservation also provides a way to split the
>> crashkernel reservation into high and low memory reservations, which can
>> be enabled for powerpc in the future.
>>
>> Along with moving to the generic crashkernel reservation, the code
>> related to finding the base address for the crashkernel has been
>> separated into its own function name get_crash_base() for better
>> readability and maintainability.
>>
>> To prevent crashkernel memory from being added to iomem_resource, the
>> function arch_add_crash_res_to_iomem() has been introduced. For further
>> details on why this should not be done for the PowerPC architecture,
>> please refer to the previous commit titled "crash: let arch decide crash
>> memory export to iomem_resource.
>>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Baoquan he <bhe@redhat.com>
>> Cc: Hari Bathini <hbathini@linux.ibm.com>
>> CC: Madhavan Srinivasan <maddy@linux.ibm.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: kexec@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Reviewed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>> ---
>>   arch/powerpc/Kconfig                     |  3 +
>>   arch/powerpc/include/asm/crash_reserve.h | 18 +++++
>>   arch/powerpc/include/asm/kexec.h         |  4 +-
>>   arch/powerpc/kernel/prom.c               |  2 +-
>>   arch/powerpc/kexec/core.c                | 90 ++++++++++--------------
>>   5 files changed, 63 insertions(+), 54 deletions(-)
>>   create mode 100644 arch/powerpc/include/asm/crash_reserve.h
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index db9f7b2d07bf..880d35fadf40 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -718,6 +718,9 @@ config ARCH_SUPPORTS_CRASH_HOTPLUG
>>       def_bool y
>>       depends on PPC64
>>   +config ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
>> +    def_bool CRASH_RESERVE
>> +
>>   config FA_DUMP
>>       bool "Firmware-assisted dump"
>>       depends on CRASH_DUMP && PPC64 && (PPC_RTAS || PPC_POWERNV)
>> diff --git a/arch/powerpc/include/asm/crash_reserve.h
>> b/arch/powerpc/include/asm/crash_reserve.h
>> new file mode 100644
>> index 000000000000..f5e60721de41
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/crash_reserve.h
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_POWERPC_CRASH_RESERVE_H
>> +#define _ASM_POWERPC_CRASH_RESERVE_H
>> +
>> +/* crash kernel regions are Page size agliged */
>> +#define CRASH_ALIGN             PAGE_SIZE
>> +
>> +#ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
>> +
>
>> +static inline bool arch_add_crash_res_to_iomem(void)
>> +{
>> +    return false;
>> +}
>> +#define arch_add_crash_res_to_iomem arch_add_crash_res_to_iomem
>
> This is probably not needed if commit c40dd2f766440 can be reverted.
> Othewise..

Agree. Since I am reverting c40dd2f766440 in next series so I will the
above change.

>
> Acked-by: Hari Bathini <hbathini@linux.ibm.com>

Thanks for the Ack!

- Sourabh Jain

>
>> +#endif
>> +
>> +#endif /* _ASM_POWERPC_CRASH_RESERVE_H */
>> +
>> diff --git a/arch/powerpc/include/asm/kexec.h
>> b/arch/powerpc/include/asm/kexec.h
>> index 270ee93a0f7d..64741558071f 100644
>> --- a/arch/powerpc/include/asm/kexec.h
>> +++ b/arch/powerpc/include/asm/kexec.h
>> @@ -113,9 +113,9 @@ int setup_new_fdt_ppc64(const struct kimage
>> *image, void *fdt, struct crash_mem
>>     #ifdef CONFIG_CRASH_RESERVE
>>   int __init overlaps_crashkernel(unsigned long start, unsigned long
>> size);
>> -extern void reserve_crashkernel(void);
>> +extern void arch_reserve_crashkernel(void);
>>   #else
>> -static inline void reserve_crashkernel(void) {}
>> +static inline void arch_reserve_crashkernel(void) {}
>>   static inline int overlaps_crashkernel(unsigned long start,
>> unsigned long size) { return 0; }
>>   #endif
>>   diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> index e0059842a1c6..9ed9dde7d231 100644
>> --- a/arch/powerpc/kernel/prom.c
>> +++ b/arch/powerpc/kernel/prom.c
>> @@ -860,7 +860,7 @@ void __init early_init_devtree(void *params)
>>        */
>>       if (fadump_reserve_mem() == 0)
>>   #endif
>> -        reserve_crashkernel();
>> +        arch_reserve_crashkernel();
>>       early_reserve_mem();
>>         if (memory_limit > memblock_phys_mem_size())
>> diff --git a/arch/powerpc/kexec/core.c b/arch/powerpc/kexec/core.c
>> index 4945b33322ae..b21cfa814492 100644
>> --- a/arch/powerpc/kexec/core.c
>> +++ b/arch/powerpc/kexec/core.c
>> @@ -80,38 +80,20 @@ void machine_kexec(struct kimage *image)
>>   }
>>     #ifdef CONFIG_CRASH_RESERVE
>> -void __init reserve_crashkernel(void)
>> -{
>> -    unsigned long long crash_size, crash_base, total_mem_sz;
>> -    int ret;
>>   -    total_mem_sz = memory_limit ? memory_limit :
>> memblock_phys_mem_size();
>> -    /* use common parsing */
>> -    ret = parse_crashkernel(boot_command_line, total_mem_sz,
>> -            &crash_size, &crash_base, NULL, NULL);
>> -    if (ret == 0 && crash_size > 0) {
>> -        crashk_res.start = crash_base;
>> -        crashk_res.end = crash_base + crash_size - 1;
>> -    }
>> -
>> -    if (crashk_res.end == crashk_res.start) {
>> -        crashk_res.start = crashk_res.end = 0;
>> -        return;
>> -    }
>> -
>> -    /* We might have got these values via the command line or the
>> -     * device tree, either way sanitise them now. */
>> -
>> -    crash_size = resource_size(&crashk_res);
>> +static unsigned long long __init get_crash_base(unsigned long long
>> crash_base)
>> +{
>>     #ifndef CONFIG_NONSTATIC_KERNEL
>> -    if (crashk_res.start != KDUMP_KERNELBASE)
>> +    if (crash_base != KDUMP_KERNELBASE)
>>           printk("Crash kernel location must be 0x%x\n",
>>                   KDUMP_KERNELBASE);
>>   -    crashk_res.start = KDUMP_KERNELBASE;
>> +    return KDUMP_KERNELBASE;
>>   #else
>> -    if (!crashk_res.start) {
>> +    unsigned long long crash_base_align;
>> +
>> +    if (!crash_base) {
>>   #ifdef CONFIG_PPC64
>>           /*
>>            * On the LPAR platform place the crash kernel to mid of
>> @@ -123,45 +105,51 @@ void __init reserve_crashkernel(void)
>>            * kernel starts at 128MB offset on other platforms.
>>            */
>>           if (firmware_has_feature(FW_FEATURE_LPAR))
>> -            crashk_res.start = min_t(u64, ppc64_rma_size / 2, SZ_512M);
>> +            crash_base = min_t(u64, ppc64_rma_size / 2, SZ_512M);
>>           else
>> -            crashk_res.start = min_t(u64, ppc64_rma_size / 2, SZ_128M);
>> +            crash_base = min_t(u64, ppc64_rma_size / 2, SZ_128M);
>>   #else
>> -        crashk_res.start = KDUMP_KERNELBASE;
>> +        crash_base = KDUMP_KERNELBASE;
>>   #endif
>>       }
>>   -    crash_base = PAGE_ALIGN(crashk_res.start);
>> -    if (crash_base != crashk_res.start) {
>> -        printk("Crash kernel base must be aligned to 0x%lx\n",
>> -                PAGE_SIZE);
>> -        crashk_res.start = crash_base;
>> -    }
>> +    crash_base_align = PAGE_ALIGN(crash_base);
>> +    if (crash_base != crash_base_align)
>> +        pr_warn("Crash kernel base must be aligned to 0x%lx\n",
>> PAGE_SIZE);
>>   +    return crash_base_align;
>>   #endif
>> -    crash_size = PAGE_ALIGN(crash_size);
>> -    crashk_res.end = crashk_res.start + crash_size - 1;
>> +}
>>   -    /* The crash region must not overlap the current kernel */
>> -    if (overlaps_crashkernel(__pa(_stext), _end - _stext)) {
>> -        printk(KERN_WARNING
>> -            "Crash kernel can not overlap current kernel\n");
>> -        crashk_res.start = crashk_res.end = 0;
>> +void __init arch_reserve_crashkernel(void)
>> +{
>> +    unsigned long long crash_size, crash_base, crash_end;
>> +    unsigned long long kernel_start, kernel_size;
>> +    unsigned long long total_mem_sz;
>> +    int ret;
>> +
>> +    total_mem_sz = memory_limit ? memory_limit :
>> memblock_phys_mem_size();
>> +
>> +    /* use common parsing */
>> +    ret = parse_crashkernel(boot_command_line, total_mem_sz,
>> &crash_size,
>> +                &crash_base, NULL, NULL);
>> +
>> +    if (ret)
>>           return;
>> -    }
>>   -    printk(KERN_INFO "Reserving %ldMB of memory at %ldMB "
>> -            "for crashkernel (System RAM: %ldMB)\n",
>> -            (unsigned long)(crash_size >> 20),
>> -            (unsigned long)(crashk_res.start >> 20),
>> -            (unsigned long)(total_mem_sz >> 20));
>> +    crash_base = get_crash_base(crash_base);
>> +    crash_end = crash_base + crash_size - 1;
>>   -    if (!memblock_is_region_memory(crashk_res.start, crash_size) ||
>> -        memblock_reserve(crashk_res.start, crash_size)) {
>> -        pr_err("Failed to reserve memory for crashkernel!\n");
>> -        crashk_res.start = crashk_res.end = 0;
>> +    kernel_start = __pa(_stext);
>> +    kernel_size = _end - _stext;
>> +
>> +    /* The crash region must not overlap the current kernel */
>> +    if ((kernel_start + kernel_size > crash_base) && (kernel_start
>> <= crash_end)) {
>> +        pr_warn("Crash kernel can not overlap current kernel\n");
>>           return;
>>       }
>> +
>> +    reserve_crashkernel_generic(crash_size, crash_base, 0, false);
>>   }
>>     int __init overlaps_crashkernel(unsigned long start, unsigned
>> long size)
>