Re: [PATCH v2 3/6] crash: let arch decide crash memory export to iomem_resource

[Sourabh Jain]

Hello Hari,


On 23/01/25 15:56, Hari Bathini wrote:
> Hi Sourabh,
>
> On 21/01/25 5:24 pm, Sourabh Jain wrote:
> > insert_crashkernel_resources() adds crash memory to iomem_resource if
> > generic crashkernel reservation is enabled on an architecture.
>
> > On PowerPC, system RAM is added to iomem_resource. See commit
> > c40dd2f766440 ("powerpc: Add System RAM to /proc/iomem").
>
> The changelog clearly says the patch was added for kdump. But 
> kexec-tools or kernel code for kdump on powerpc don't seem to have relied
> on that. Device-tree memory nodes were used instead? Wondering if
> dropping commit c40dd2f766440 is better..

Hmm. Even I thought of removing that patch for the same reason.
But I noticed the below comment:

/*
 * System memory should not be in /proc/iomem but various tools expect it
 * (eg kdump).
 */

Since they said eg kdump. I thought maybe it is needed for tools other 
then kdump.
But I think this was only added for kdump. And as we know this is not
used in kexec/kdump tool anymore.

I will revert `c40dd2f766440 ("powerpc: Add System RAM to /proc/iomem"). `
change in the next series and take feedback from the community.

Thanks for the review.

- Sourabh Jain

> > Enabling generic crashkernel reservation on PowerPC leads to a conflict
> > when system RAM is added to iomem_resource because a part of the system
> > RAM, the crashkernel memory, has already been added to iomem_resource.
> >
> > The next commit in the series "powerpc/crash: use generic crashkernel
> > reservation" enables generic crashkernel reservation on PowerPC. If the
> > crashkernel is added to iomem_resource, the kernel fails to add
> > system RAM to /proc/iomem and prints the following traces:
> >
> > CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.13.0-rc2+
> > snip...
> > NIP [c000000002016b3c] add_system_ram_resources+0xf0/0x15c
> > LR [c000000002016b34] add_system_ram_resources+0xe8/0x15c
> > Call Trace:
> > [c00000000484bbc0] [c000000002016b34] 
> > add_system_ram_resources+0xe8/0x15c
> > [c00000000484bc20] [c000000000010a4c] do_one_initcall+0x7c/0x39c
> > [c00000000484bd00] [c000000002005418] do_initcalls+0x144/0x18c
> > [c00000000484bd90] [c000000002005714] kernel_init_freeable+0x21c/0x290
> > [c00000000484bdf0] [c0000000000110f4] kernel_init+0x2c/0x1b8
> > [c00000000484be50] [c00000000000dd3c] 
> > ret_from_kernel_user_thread+0x14/0x1c
> >
> > To avoid this, an architecture hook is added in
> > insert_crashkernel_resources(), allowing the architecture to decide
> > whether crashkernel memory should be added to iomem_resource.
> >
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Baoquan he <bhe@xxxxxxxxxx>
> > Cc: Hari Bathini <hbathini@xxxxxxxxxxxxx>
> > Cc: Madhavan Srinivasan <maddy@xxxxxxxxxxxxx>
> > Cc: Mahesh Salgaonkar <mahesh@xxxxxxxxxxxxx>
> > Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
> > Cc: kexec@xxxxxxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Signed-off-by: Sourabh Jain <sourabhjain@xxxxxxxxxxxxx>
> > ---
> >   include/linux/crash_reserve.h | 11 +++++++++++
> >   kernel/crash_reserve.c        |  3 +++
> >   2 files changed, 14 insertions(+)
> >
> > diff --git a/include/linux/crash_reserve.h 
> > b/include/linux/crash_reserve.h
> > index 1fe7e7d1b214..f1205d044dae 100644
> > --- a/include/linux/crash_reserve.h
> > +++ b/include/linux/crash_reserve.h
> > @@ -18,6 +18,17 @@ int __init parse_crashkernel(char *cmdline, 
> > unsigned long long system_ram,
> >           unsigned long long *crash_size, unsigned long long 
> > *crash_base,
> >           unsigned long long *low_size, bool *high);
> >   +#ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
> > +
> > +#ifndef arch_add_crash_res_to_iomem
> > +static inline bool arch_add_crash_res_to_iomem(void)
> > +{
> > +    return true;
> > +}
> > +#endif
> > +
> > +#endif
> > +
> >   #ifdef CONFIG_ARCH_HAS_GENERIC_CRASHKERNEL_RESERVATION
> >   #ifndef DEFAULT_CRASH_KERNEL_LOW_SIZE
> >   #define DEFAULT_CRASH_KERNEL_LOW_SIZE    (128UL << 20)
> > diff --git a/kernel/crash_reserve.c b/kernel/crash_reserve.c
> > index aff7c0fdbefa..190104f32fe1 100644
> > --- a/kernel/crash_reserve.c
> > +++ b/kernel/crash_reserve.c
> > @@ -460,6 +460,9 @@ void __init reserve_crashkernel_generic(unsigned 
> > long long crash_size,
> >   #ifndef HAVE_ARCH_ADD_CRASH_RES_TO_IOMEM_EARLY
> >   static __init int insert_crashkernel_resources(void)
> >   {
> > +    if (!arch_add_crash_res_to_iomem())
> > +        return 0;
> > +
> >       if (crashk_res.start < crashk_res.end)
> >           insert_resource(&iomem_resource, &crashk_res);