| Message ID | 20241107055817.489795-1-sourabhjain@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f4892c68ecc1cf45e41a78820dd2eebccc945b66 |
| Headers | show |
| Series | [v2,1/2] powerpc/fadump: allocate memory for additional parameters early | expand |
Applied the below patch to 6.12.0-rc6-next20241106 and issue is not seen. Results look good to me. [root@ltcden8-lp5 ~]# uname -r 6.12.0-rc6-next-20241106-00002-gf374fbb4ee1a [root@ltcden8-lp5 ~]# Please add below tag. > Tested-by: Venkat Rao Bagalkote <venkat88@linux.vnet.ibm.com> > On 7 Nov 2024, at 11:28 AM, Sourabh Jain <sourabhjain@linux.ibm.com> wrote: > > From: Hari Bathini <hbathini@linux.ibm.com> > > Memory for passing additional parameters to fadump capture kernel > is allocated during subsys_initcall level, using memblock. But > as slab is already available by this time, allocation happens via > the buddy allocator. This may work for radix MMU but is likely to > fail in most cases for hash MMU as hash MMU needs this memory in > the first memory block for it to be accessible in real mode in the > capture kernel (second boot). So, allocate memory for additional > parameters area as soon as MMU mode is obvious. > > Fixes: 683eab94da75 ("powerpc/fadump: setup additional parameters for dump capture kernel") > Reported-by: Venkat Rao Bagalkote <venkat88@linux.vnet.ibm.com> > Closes: https://lore.kernel.org/lkml/a70e4064-a040-447b-8556-1fd02f19383d@linux.vnet.ibm.com/T/#u > Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> > Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> > --- > > Changelog: > > Since v1: https://lore.kernel.org/all/20241104083528.99520-1-sourabhjain@linux.ibm.com/ > - Drop extern keyword from fadump_setup_param_area function declaration > - Don't use __va() while resetting param memory area > > --- > arch/powerpc/include/asm/fadump.h | 2 ++ > arch/powerpc/kernel/fadump.c | 15 ++++++++++----- > arch/powerpc/kernel/prom.c | 3 +++ > 3 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h > index ef40c9b6972a..7b3473e05273 100644 > --- a/arch/powerpc/include/asm/fadump.h > +++ b/arch/powerpc/include/asm/fadump.h > @@ -19,6 +19,7 @@ extern int is_fadump_active(void); > extern int should_fadump_crash(void); > extern void crash_fadump(struct pt_regs *, const char *); > extern void fadump_cleanup(void); > +void fadump_setup_param_area(void); > extern void fadump_append_bootargs(void); > > #else /* CONFIG_FA_DUMP */ > @@ -26,6 +27,7 @@ static inline int is_fadump_active(void) { return 0; } > static inline int should_fadump_crash(void) { return 0; } > static inline void crash_fadump(struct pt_regs *regs, const char *str) { } > static inline void fadump_cleanup(void) { } > +static inline void fadump_setup_param_area(void) { } > static inline void fadump_append_bootargs(void) { } > #endif /* !CONFIG_FA_DUMP */ > > diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c > index a612e7513a4f..3a2863307863 100644 > --- a/arch/powerpc/kernel/fadump.c > +++ b/arch/powerpc/kernel/fadump.c > @@ -1586,6 +1586,12 @@ static void __init fadump_init_files(void) > return; > } > > + if (fw_dump.param_area) { > + rc = sysfs_create_file(fadump_kobj, &bootargs_append_attr.attr); > + if (rc) > + pr_err("unable to create bootargs_append sysfs file (%d)\n", rc); > + } > + > debugfs_create_file("fadump_region", 0444, arch_debugfs_dir, NULL, > &fadump_region_fops); > > @@ -1740,7 +1746,7 @@ static void __init fadump_process(void) > * Reserve memory to store additional parameters to be passed > * for fadump/capture kernel. > */ > -static void __init fadump_setup_param_area(void) > +void __init fadump_setup_param_area(void) > { > phys_addr_t range_start, range_end; > > @@ -1748,7 +1754,7 @@ static void __init fadump_setup_param_area(void) > return; > > /* This memory can't be used by PFW or bootloader as it is shared across kernels */ > - if (radix_enabled()) { > + if (early_radix_enabled()) { > /* > * Anywhere in the upper half should be good enough as all memory > * is accessible in real mode. > @@ -1776,12 +1782,12 @@ static void __init fadump_setup_param_area(void) > COMMAND_LINE_SIZE, > range_start, > range_end); > - if (!fw_dump.param_area || sysfs_create_file(fadump_kobj, &bootargs_append_attr.attr)) { > + if (!fw_dump.param_area) { > pr_warn("WARNING: Could not setup area to pass additional parameters!\n"); > return; > } > > - memset(phys_to_virt(fw_dump.param_area), 0, COMMAND_LINE_SIZE); > + memset((void *)fw_dump.param_area, 0, COMMAND_LINE_SIZE); > } > > /* > @@ -1807,7 +1813,6 @@ int __init setup_fadump(void) > } > /* Initialize the kernel dump memory structure and register with f/w */ > else if (fw_dump.reserve_dump_area_size) { > - fadump_setup_param_area(); > fw_dump.ops->fadump_init_mem_struct(&fw_dump); > register_fadump(); > } > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c > index 0be07ed407c7..47db1b1aef25 100644 > --- a/arch/powerpc/kernel/prom.c > +++ b/arch/powerpc/kernel/prom.c > @@ -908,6 +908,9 @@ void __init early_init_devtree(void *params) > > mmu_early_init_devtree(); > > + /* Setup param area for passing additional parameters to fadump capture kernel. */ > + fadump_setup_param_area(); > + > #ifdef CONFIG_PPC_POWERNV > /* Scan and build the list of machine check recoverable ranges */ > of_scan_flat_dt(early_init_dt_scan_recoverable_ranges, NULL); > -- > 2.46.2 >
Applied the below patch to 6.12.0-rc6-next20241106 and issue is not seen. Results look good to me. [root@ltcden8-lp5 ~]# uname -r 6.12.0-rc6-next-20241106-00002-gf374fbb4ee1a [root@ltcden8-lp5 ~]# Please add below tag. Tested-by: Venkat Rao Bagalkote <venkat88@linux.vnet.ibm.com> On 07/11/24 11:28 am, Sourabh Jain wrote: > From: Hari Bathini <hbathini@linux.ibm.com> > > Memory for passing additional parameters to fadump capture kernel > is allocated during subsys_initcall level, using memblock. But > as slab is already available by this time, allocation happens via > the buddy allocator. This may work for radix MMU but is likely to > fail in most cases for hash MMU as hash MMU needs this memory in > the first memory block for it to be accessible in real mode in the > capture kernel (second boot). So, allocate memory for additional > parameters area as soon as MMU mode is obvious. > > Fixes: 683eab94da75 ("powerpc/fadump: setup additional parameters for dump capture kernel") > Reported-by: Venkat Rao Bagalkote <venkat88@linux.vnet.ibm.com> > Closes: https://lore.kernel.org/lkml/a70e4064-a040-447b-8556-1fd02f19383d@linux.vnet.ibm.com/T/#u > Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> > Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> > --- > > Changelog: > > Since v1: https://lore.kernel.org/all/20241104083528.99520-1-sourabhjain@linux.ibm.com/ > - Drop extern keyword from fadump_setup_param_area function declaration > - Don't use __va() while resetting param memory area > > --- > arch/powerpc/include/asm/fadump.h | 2 ++ > arch/powerpc/kernel/fadump.c | 15 ++++++++++----- > arch/powerpc/kernel/prom.c | 3 +++ > 3 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h > index ef40c9b6972a..7b3473e05273 100644 > --- a/arch/powerpc/include/asm/fadump.h > +++ b/arch/powerpc/include/asm/fadump.h > @@ -19,6 +19,7 @@ extern int is_fadump_active(void); > extern int should_fadump_crash(void); > extern void crash_fadump(struct pt_regs *, const char *); > extern void fadump_cleanup(void); > +void fadump_setup_param_area(void); > extern void fadump_append_bootargs(void); > > #else /* CONFIG_FA_DUMP */ > @@ -26,6 +27,7 @@ static inline int is_fadump_active(void) { return 0; } > static inline int should_fadump_crash(void) { return 0; } > static inline void crash_fadump(struct pt_regs *regs, const char *str) { } > static inline void fadump_cleanup(void) { } > +static inline void fadump_setup_param_area(void) { } > static inline void fadump_append_bootargs(void) { } > #endif /* !CONFIG_FA_DUMP */ > > diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c > index a612e7513a4f..3a2863307863 100644 > --- a/arch/powerpc/kernel/fadump.c > +++ b/arch/powerpc/kernel/fadump.c > @@ -1586,6 +1586,12 @@ static void __init fadump_init_files(void) > return; > } > > + if (fw_dump.param_area) { > + rc = sysfs_create_file(fadump_kobj, &bootargs_append_attr.attr); > + if (rc) > + pr_err("unable to create bootargs_append sysfs file (%d)\n", rc); > + } > + > debugfs_create_file("fadump_region", 0444, arch_debugfs_dir, NULL, > &fadump_region_fops); > > @@ -1740,7 +1746,7 @@ static void __init fadump_process(void) > * Reserve memory to store additional parameters to be passed > * for fadump/capture kernel. > */ > -static void __init fadump_setup_param_area(void) > +void __init fadump_setup_param_area(void) > { > phys_addr_t range_start, range_end; > > @@ -1748,7 +1754,7 @@ static void __init fadump_setup_param_area(void) > return; > > /* This memory can't be used by PFW or bootloader as it is shared across kernels */ > - if (radix_enabled()) { > + if (early_radix_enabled()) { > /* > * Anywhere in the upper half should be good enough as all memory > * is accessible in real mode. > @@ -1776,12 +1782,12 @@ static void __init fadump_setup_param_area(void) > COMMAND_LINE_SIZE, > range_start, > range_end); > - if (!fw_dump.param_area || sysfs_create_file(fadump_kobj, &bootargs_append_attr.attr)) { > + if (!fw_dump.param_area) { > pr_warn("WARNING: Could not setup area to pass additional parameters!\n"); > return; > } > > - memset(phys_to_virt(fw_dump.param_area), 0, COMMAND_LINE_SIZE); > + memset((void *)fw_dump.param_area, 0, COMMAND_LINE_SIZE); > } > > /* > @@ -1807,7 +1813,6 @@ int __init setup_fadump(void) > } > /* Initialize the kernel dump memory structure and register with f/w */ > else if (fw_dump.reserve_dump_area_size) { > - fadump_setup_param_area(); > fw_dump.ops->fadump_init_mem_struct(&fw_dump); > register_fadump(); > } > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c > index 0be07ed407c7..47db1b1aef25 100644 > --- a/arch/powerpc/kernel/prom.c > +++ b/arch/powerpc/kernel/prom.c > @@ -908,6 +908,9 @@ void __init early_init_devtree(void *params) > > mmu_early_init_devtree(); > > + /* Setup param area for passing additional parameters to fadump capture kernel. */ > + fadump_setup_param_area(); > + > #ifdef CONFIG_PPC_POWERNV > /* Scan and build the list of machine check recoverable ranges */ > of_scan_flat_dt(early_init_dt_scan_recoverable_ranges, NULL);
Hello Venkat, Thanks for testing the patch series. - Sourabh Jain On 07/11/24 19:25, Venkat Rao Bagalkote wrote: > Applied the below patch to 6.12.0-rc6-next20241106 and issue is not > seen. Results look good to me. > > [root@ltcden8-lp5 ~]# uname -r > 6.12.0-rc6-next-20241106-00002-gf374fbb4ee1a > [root@ltcden8-lp5 ~]# > > Please add below tag. > > Tested-by: Venkat Rao Bagalkote <venkat88@linux.vnet.ibm.com> > > On 07/11/24 11:28 am, Sourabh Jain wrote: >> From: Hari Bathini <hbathini@linux.ibm.com> >> >> Memory for passing additional parameters to fadump capture kernel >> is allocated during subsys_initcall level, using memblock. But >> as slab is already available by this time, allocation happens via >> the buddy allocator. This may work for radix MMU but is likely to >> fail in most cases for hash MMU as hash MMU needs this memory in >> the first memory block for it to be accessible in real mode in the >> capture kernel (second boot). So, allocate memory for additional >> parameters area as soon as MMU mode is obvious. >> >> Fixes: 683eab94da75 ("powerpc/fadump: setup additional parameters for >> dump capture kernel") >> Reported-by: Venkat Rao Bagalkote <venkat88@linux.vnet.ibm.com> >> Closes: >> https://lore.kernel.org/lkml/a70e4064-a040-447b-8556-1fd02f19383d@linux.vnet.ibm.com/T/#u >> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com> >> Cc: Michael Ellerman <mpe@ellerman.id.au> >> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> >> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> >> --- >> >> Changelog: >> >> Since v1: >> https://lore.kernel.org/all/20241104083528.99520-1-sourabhjain@linux.ibm.com/ >> - Drop extern keyword from fadump_setup_param_area function >> declaration >> - Don't use __va() while resetting param memory area >> >> --- >> arch/powerpc/include/asm/fadump.h | 2 ++ >> arch/powerpc/kernel/fadump.c | 15 ++++++++++----- >> arch/powerpc/kernel/prom.c | 3 +++ >> 3 files changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/fadump.h >> b/arch/powerpc/include/asm/fadump.h >> index ef40c9b6972a..7b3473e05273 100644 >> --- a/arch/powerpc/include/asm/fadump.h >> +++ b/arch/powerpc/include/asm/fadump.h >> @@ -19,6 +19,7 @@ extern int is_fadump_active(void); >> extern int should_fadump_crash(void); >> extern void crash_fadump(struct pt_regs *, const char *); >> extern void fadump_cleanup(void); >> +void fadump_setup_param_area(void); >> extern void fadump_append_bootargs(void); >> >> #else /* CONFIG_FA_DUMP */ >> @@ -26,6 +27,7 @@ static inline int is_fadump_active(void) { return 0; } >> static inline int should_fadump_crash(void) { return 0; } >> static inline void crash_fadump(struct pt_regs *regs, const char >> *str) { } >> static inline void fadump_cleanup(void) { } >> +static inline void fadump_setup_param_area(void) { } >> static inline void fadump_append_bootargs(void) { } >> #endif /* !CONFIG_FA_DUMP */ >> >> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c >> index a612e7513a4f..3a2863307863 100644 >> --- a/arch/powerpc/kernel/fadump.c >> +++ b/arch/powerpc/kernel/fadump.c >> @@ -1586,6 +1586,12 @@ static void __init fadump_init_files(void) >> return; >> } >> >> + if (fw_dump.param_area) { >> + rc = sysfs_create_file(fadump_kobj, >> &bootargs_append_attr.attr); >> + if (rc) >> + pr_err("unable to create bootargs_append sysfs file >> (%d)\n", rc); >> + } >> + >> debugfs_create_file("fadump_region", 0444, arch_debugfs_dir, NULL, >> &fadump_region_fops); >> >> @@ -1740,7 +1746,7 @@ static void __init fadump_process(void) >> * Reserve memory to store additional parameters to be passed >> * for fadump/capture kernel. >> */ >> -static void __init fadump_setup_param_area(void) >> +void __init fadump_setup_param_area(void) >> { >> phys_addr_t range_start, range_end; >> >> @@ -1748,7 +1754,7 @@ static void __init fadump_setup_param_area(void) >> return; >> >> /* This memory can't be used by PFW or bootloader as it is >> shared across kernels */ >> - if (radix_enabled()) { >> + if (early_radix_enabled()) { >> /* >> * Anywhere in the upper half should be good enough as all >> memory >> * is accessible in real mode. >> @@ -1776,12 +1782,12 @@ static void __init fadump_setup_param_area(void) >> COMMAND_LINE_SIZE, >> range_start, >> range_end); >> - if (!fw_dump.param_area || sysfs_create_file(fadump_kobj, >> &bootargs_append_attr.attr)) { >> + if (!fw_dump.param_area) { >> pr_warn("WARNING: Could not setup area to pass additional >> parameters!\n"); >> return; >> } >> >> - memset(phys_to_virt(fw_dump.param_area), 0, COMMAND_LINE_SIZE); >> + memset((void *)fw_dump.param_area, 0, COMMAND_LINE_SIZE); >> } >> >> /* >> @@ -1807,7 +1813,6 @@ int __init setup_fadump(void) >> } >> /* Initialize the kernel dump memory structure and register >> with f/w */ >> else if (fw_dump.reserve_dump_area_size) { >> - fadump_setup_param_area(); >> fw_dump.ops->fadump_init_mem_struct(&fw_dump); >> register_fadump(); >> } >> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c >> index 0be07ed407c7..47db1b1aef25 100644 >> --- a/arch/powerpc/kernel/prom.c >> +++ b/arch/powerpc/kernel/prom.c >> @@ -908,6 +908,9 @@ void __init early_init_devtree(void *params) >> >> mmu_early_init_devtree(); >> >> + /* Setup param area for passing additional parameters to fadump >> capture kernel. */ >> + fadump_setup_param_area(); >> + >> #ifdef CONFIG_PPC_POWERNV >> /* Scan and build the list of machine check recoverable ranges */ >> of_scan_flat_dt(early_init_dt_scan_recoverable_ranges, NULL);
Sourabh Jain <sourabhjain@linux.ibm.com> writes: > From: Hari Bathini <hbathini@linux.ibm.com> > > Memory for passing additional parameters to fadump capture kernel > is allocated during subsys_initcall level, using memblock. But > as slab is already available by this time, allocation happens via > the buddy allocator. This may work for radix MMU but is likely to > fail in most cases for hash MMU as hash MMU needs this memory in > the first memory block for it to be accessible in real mode in the > capture kernel (second boot). So, allocate memory for additional > parameters area as soon as MMU mode is obvious. But looks like this was only caught due to the WARN_ON_ONCE emitted from mm/memblock.c which detected accidental use of memblock APIs when slab is available. That begs a question on why didn't we see the issue on Hash? Are we not using the "param_area_supported" feature that often is it? > > Fixes: 683eab94da75 ("powerpc/fadump: setup additional parameters for dump capture kernel") > Reported-by: Venkat Rao Bagalkote <venkat88@linux.vnet.ibm.com> > Closes: https://lore.kernel.org/lkml/a70e4064-a040-447b-8556-1fd02f19383d@linux.vnet.ibm.com/T/#u > Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com> > Cc: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> > Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> > --- > > Changelog: > > Since v1: https://lore.kernel.org/all/20241104083528.99520-1-sourabhjain@linux.ibm.com/ > - Drop extern keyword from fadump_setup_param_area function declaration > - Don't use __va() while resetting param memory area > > --- > arch/powerpc/include/asm/fadump.h | 2 ++ > arch/powerpc/kernel/fadump.c | 15 ++++++++++----- > arch/powerpc/kernel/prom.c | 3 +++ > 3 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h > index ef40c9b6972a..7b3473e05273 100644 > --- a/arch/powerpc/include/asm/fadump.h > +++ b/arch/powerpc/include/asm/fadump.h > @@ -19,6 +19,7 @@ extern int is_fadump_active(void); > extern int should_fadump_crash(void); > extern void crash_fadump(struct pt_regs *, const char *); > extern void fadump_cleanup(void); > +void fadump_setup_param_area(void); > extern void fadump_append_bootargs(void); > > #else /* CONFIG_FA_DUMP */ > @@ -26,6 +27,7 @@ static inline int is_fadump_active(void) { return 0; } > static inline int should_fadump_crash(void) { return 0; } > static inline void crash_fadump(struct pt_regs *regs, const char *str) { } > static inline void fadump_cleanup(void) { } > +static inline void fadump_setup_param_area(void) { } > static inline void fadump_append_bootargs(void) { } > #endif /* !CONFIG_FA_DUMP */ > > diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c > index a612e7513a4f..3a2863307863 100644 > --- a/arch/powerpc/kernel/fadump.c > +++ b/arch/powerpc/kernel/fadump.c > @@ -1586,6 +1586,12 @@ static void __init fadump_init_files(void) > return; > } > > + if (fw_dump.param_area) { > + rc = sysfs_create_file(fadump_kobj, &bootargs_append_attr.attr); > + if (rc) > + pr_err("unable to create bootargs_append sysfs file (%d)\n", rc); > + } > + > debugfs_create_file("fadump_region", 0444, arch_debugfs_dir, NULL, > &fadump_region_fops); > > @@ -1740,7 +1746,7 @@ static void __init fadump_process(void) > * Reserve memory to store additional parameters to be passed > * for fadump/capture kernel. > */ > -static void __init fadump_setup_param_area(void) > +void __init fadump_setup_param_area(void) > { > phys_addr_t range_start, range_end; > > @@ -1748,7 +1754,7 @@ static void __init fadump_setup_param_area(void) > return; > > /* This memory can't be used by PFW or bootloader as it is shared across kernels */ > - if (radix_enabled()) { > + if (early_radix_enabled()) { > /* > * Anywhere in the upper half should be good enough as all memory > * is accessible in real mode. > @@ -1776,12 +1782,12 @@ static void __init fadump_setup_param_area(void) > COMMAND_LINE_SIZE, > range_start, > range_end); > - if (!fw_dump.param_area || sysfs_create_file(fadump_kobj, &bootargs_append_attr.attr)) { > + if (!fw_dump.param_area) { > pr_warn("WARNING: Could not setup area to pass additional parameters!\n"); > return; > } > > - memset(phys_to_virt(fw_dump.param_area), 0, COMMAND_LINE_SIZE); > + memset((void *)fw_dump.param_area, 0, COMMAND_LINE_SIZE); > } > > /* > @@ -1807,7 +1813,6 @@ int __init setup_fadump(void) > } > /* Initialize the kernel dump memory structure and register with f/w */ > else if (fw_dump.reserve_dump_area_size) { > - fadump_setup_param_area(); > fw_dump.ops->fadump_init_mem_struct(&fw_dump); > register_fadump(); > } > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c > index 0be07ed407c7..47db1b1aef25 100644 > --- a/arch/powerpc/kernel/prom.c > +++ b/arch/powerpc/kernel/prom.c > @@ -908,6 +908,9 @@ void __init early_init_devtree(void *params) > > mmu_early_init_devtree(); > > + /* Setup param area for passing additional parameters to fadump capture kernel. */ > + fadump_setup_param_area(); > + Maybe we should add a comment here saying this needs to be done after mmu_early_init_devtree() because for pseries LPARs we need to be able to reliably use early_radix_enabled() helper within fadump_setup_param_area(). Either ways the patch looks good to me. So please feel free to add - Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> > #ifdef CONFIG_PPC_POWERNV > /* Scan and build the list of machine check recoverable ranges */ > of_scan_flat_dt(early_init_dt_scan_recoverable_ranges, NULL); > -- > 2.46.2
Hello Ritesh On 12/11/24 12:33, Ritesh Harjani (IBM) wrote: > Sourabh Jain <sourabhjain@linux.ibm.com> writes: > >> From: Hari Bathini <hbathini@linux.ibm.com> >> >> Memory for passing additional parameters to fadump capture kernel >> is allocated during subsys_initcall level, using memblock. But >> as slab is already available by this time, allocation happens via >> the buddy allocator. This may work for radix MMU but is likely to >> fail in most cases for hash MMU as hash MMU needs this memory in >> the first memory block for it to be accessible in real mode in the >> capture kernel (second boot). So, allocate memory for additional >> parameters area as soon as MMU mode is obvious. > But looks like this was only caught due to the WARN_ON_ONCE emitted from > mm/memblock.c which detected accidental use of memblock APIs when slab is > available. That begs a question on why didn't we see the issue on Hash? We do see the issues with the hash as mentioned in the commit message. > Are we not using the "param_area_supported" feature that often is it? Yes, because it is a relatively new feature, and the userspace changes required to make use of this feature using kdump service are not part of the distro yet. > >> Fixes: 683eab94da75 ("powerpc/fadump: setup additional parameters for dump capture kernel") >> Reported-by: Venkat Rao Bagalkote <venkat88@linux.vnet.ibm.com> >> Closes: https://lore.kernel.org/lkml/a70e4064-a040-447b-8556-1fd02f19383d@linux.vnet.ibm.com/T/#u >> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com> >> Cc: Michael Ellerman <mpe@ellerman.id.au> >> Signed-off-by: Hari Bathini <hbathini@linux.ibm.com> >> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com> >> --- >> >> Changelog: >> >> Since v1: https://lore.kernel.org/all/20241104083528.99520-1-sourabhjain@linux.ibm.com/ >> - Drop extern keyword from fadump_setup_param_area function declaration >> - Don't use __va() while resetting param memory area >> >> --- >> arch/powerpc/include/asm/fadump.h | 2 ++ >> arch/powerpc/kernel/fadump.c | 15 ++++++++++----- >> arch/powerpc/kernel/prom.c | 3 +++ >> 3 files changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h >> index ef40c9b6972a..7b3473e05273 100644 >> --- a/arch/powerpc/include/asm/fadump.h >> +++ b/arch/powerpc/include/asm/fadump.h >> @@ -19,6 +19,7 @@ extern int is_fadump_active(void); >> extern int should_fadump_crash(void); >> extern void crash_fadump(struct pt_regs *, const char *); >> extern void fadump_cleanup(void); >> +void fadump_setup_param_area(void); >> extern void fadump_append_bootargs(void); >> >> #else /* CONFIG_FA_DUMP */ >> @@ -26,6 +27,7 @@ static inline int is_fadump_active(void) { return 0; } >> static inline int should_fadump_crash(void) { return 0; } >> static inline void crash_fadump(struct pt_regs *regs, const char *str) { } >> static inline void fadump_cleanup(void) { } >> +static inline void fadump_setup_param_area(void) { } >> static inline void fadump_append_bootargs(void) { } >> #endif /* !CONFIG_FA_DUMP */ >> >> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c >> index a612e7513a4f..3a2863307863 100644 >> --- a/arch/powerpc/kernel/fadump.c >> +++ b/arch/powerpc/kernel/fadump.c >> @@ -1586,6 +1586,12 @@ static void __init fadump_init_files(void) >> return; >> } >> >> + if (fw_dump.param_area) { >> + rc = sysfs_create_file(fadump_kobj, &bootargs_append_attr.attr); >> + if (rc) >> + pr_err("unable to create bootargs_append sysfs file (%d)\n", rc); >> + } >> + >> debugfs_create_file("fadump_region", 0444, arch_debugfs_dir, NULL, >> &fadump_region_fops); >> >> @@ -1740,7 +1746,7 @@ static void __init fadump_process(void) >> * Reserve memory to store additional parameters to be passed >> * for fadump/capture kernel. >> */ >> -static void __init fadump_setup_param_area(void) >> +void __init fadump_setup_param_area(void) >> { >> phys_addr_t range_start, range_end; >> >> @@ -1748,7 +1754,7 @@ static void __init fadump_setup_param_area(void) >> return; >> >> /* This memory can't be used by PFW or bootloader as it is shared across kernels */ >> - if (radix_enabled()) { >> + if (early_radix_enabled()) { >> /* >> * Anywhere in the upper half should be good enough as all memory >> * is accessible in real mode. >> @@ -1776,12 +1782,12 @@ static void __init fadump_setup_param_area(void) >> COMMAND_LINE_SIZE, >> range_start, >> range_end); >> - if (!fw_dump.param_area || sysfs_create_file(fadump_kobj, &bootargs_append_attr.attr)) { >> + if (!fw_dump.param_area) { >> pr_warn("WARNING: Could not setup area to pass additional parameters!\n"); >> return; >> } >> >> - memset(phys_to_virt(fw_dump.param_area), 0, COMMAND_LINE_SIZE); >> + memset((void *)fw_dump.param_area, 0, COMMAND_LINE_SIZE); >> } >> >> /* >> @@ -1807,7 +1813,6 @@ int __init setup_fadump(void) >> } >> /* Initialize the kernel dump memory structure and register with f/w */ >> else if (fw_dump.reserve_dump_area_size) { >> - fadump_setup_param_area(); >> fw_dump.ops->fadump_init_mem_struct(&fw_dump); >> register_fadump(); >> } >> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c >> index 0be07ed407c7..47db1b1aef25 100644 >> --- a/arch/powerpc/kernel/prom.c >> +++ b/arch/powerpc/kernel/prom.c >> @@ -908,6 +908,9 @@ void __init early_init_devtree(void *params) >> >> mmu_early_init_devtree(); >> >> + /* Setup param area for passing additional parameters to fadump capture kernel. */ >> + fadump_setup_param_area(); >> + > Maybe we should add a comment here saying this needs to be done after > mmu_early_init_devtree() because for pseries LPARs we need to be able to > reliably use early_radix_enabled() helper within fadump_setup_param_area(). Sure, I will update the comment to indicate that the fadump_setup_param_area() function must be called once the MMU is finalized. > > Either ways the patch looks good to me. So please feel free to add - > Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com> Thanks for the review Ritesh. - Sourabh Jain
On Thu, 07 Nov 2024 11:28:16 +0530, Sourabh Jain wrote: > Memory for passing additional parameters to fadump capture kernel > is allocated during subsys_initcall level, using memblock. But > as slab is already available by this time, allocation happens via > the buddy allocator. This may work for radix MMU but is likely to > fail in most cases for hash MMU as hash MMU needs this memory in > the first memory block for it to be accessible in real mode in the > capture kernel (second boot). So, allocate memory for additional > parameters area as soon as MMU mode is obvious. > > [...] Applied to powerpc/next. [1/2] powerpc/fadump: allocate memory for additional parameters early https://git.kernel.org/powerpc/c/f4892c68ecc1cf45e41a78820dd2eebccc945b66 [2/2] fadump: reserve param area if below boot_mem_top https://git.kernel.org/powerpc/c/fb90dca828b6070709093934c6dec56489a2d91d cheers
diff --git a/arch/powerpc/include/asm/fadump.h b/arch/powerpc/include/asm/fadump.h index ef40c9b6972a..7b3473e05273 100644 --- a/arch/powerpc/include/asm/fadump.h +++ b/arch/powerpc/include/asm/fadump.h @@ -19,6 +19,7 @@ extern int is_fadump_active(void); extern int should_fadump_crash(void); extern void crash_fadump(struct pt_regs *, const char *); extern void fadump_cleanup(void); +void fadump_setup_param_area(void); extern void fadump_append_bootargs(void); #else /* CONFIG_FA_DUMP */ @@ -26,6 +27,7 @@ static inline int is_fadump_active(void) { return 0; } static inline int should_fadump_crash(void) { return 0; } static inline void crash_fadump(struct pt_regs *regs, const char *str) { } static inline void fadump_cleanup(void) { } +static inline void fadump_setup_param_area(void) { } static inline void fadump_append_bootargs(void) { } #endif /* !CONFIG_FA_DUMP */ diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c index a612e7513a4f..3a2863307863 100644 --- a/arch/powerpc/kernel/fadump.c +++ b/arch/powerpc/kernel/fadump.c @@ -1586,6 +1586,12 @@ static void __init fadump_init_files(void) return; } + if (fw_dump.param_area) { + rc = sysfs_create_file(fadump_kobj, &bootargs_append_attr.attr); + if (rc) + pr_err("unable to create bootargs_append sysfs file (%d)\n", rc); + } + debugfs_create_file("fadump_region", 0444, arch_debugfs_dir, NULL, &fadump_region_fops); @@ -1740,7 +1746,7 @@ static void __init fadump_process(void) * Reserve memory to store additional parameters to be passed * for fadump/capture kernel. */ -static void __init fadump_setup_param_area(void) +void __init fadump_setup_param_area(void) { phys_addr_t range_start, range_end; @@ -1748,7 +1754,7 @@ static void __init fadump_setup_param_area(void) return; /* This memory can't be used by PFW or bootloader as it is shared across kernels */ - if (radix_enabled()) { + if (early_radix_enabled()) { /* * Anywhere in the upper half should be good enough as all memory * is accessible in real mode. @@ -1776,12 +1782,12 @@ static void __init fadump_setup_param_area(void) COMMAND_LINE_SIZE, range_start, range_end); - if (!fw_dump.param_area || sysfs_create_file(fadump_kobj, &bootargs_append_attr.attr)) { + if (!fw_dump.param_area) { pr_warn("WARNING: Could not setup area to pass additional parameters!\n"); return; } - memset(phys_to_virt(fw_dump.param_area), 0, COMMAND_LINE_SIZE); + memset((void *)fw_dump.param_area, 0, COMMAND_LINE_SIZE); } /* @@ -1807,7 +1813,6 @@ int __init setup_fadump(void) } /* Initialize the kernel dump memory structure and register with f/w */ else if (fw_dump.reserve_dump_area_size) { - fadump_setup_param_area(); fw_dump.ops->fadump_init_mem_struct(&fw_dump); register_fadump(); } diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c index 0be07ed407c7..47db1b1aef25 100644 --- a/arch/powerpc/kernel/prom.c +++ b/arch/powerpc/kernel/prom.c @@ -908,6 +908,9 @@ void __init early_init_devtree(void *params) mmu_early_init_devtree(); + /* Setup param area for passing additional parameters to fadump capture kernel. */ + fadump_setup_param_area(); + #ifdef CONFIG_PPC_POWERNV /* Scan and build the list of machine check recoverable ranges */ of_scan_flat_dt(early_init_dt_scan_recoverable_ranges, NULL);