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

[Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Baoquan he <bhe@xxxxxxxxxx>
> > Cc: Hari Bathini <hbathini@xxxxxxxxxxxxx>
> > CC: Madhavan Srinivasan <maddy@xxxxxxxxxxxxx>
> > Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> > Cc: kexec@xxxxxxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Reviewed-by: Mahesh Salgaonkar <mahesh@xxxxxxxxxxxxx>
> > Signed-off-by: Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxx>

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)