Sourabh Jain <sourabhj...@linux.ibm.com> writes:
> From: Hari Bathini <hbath...@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 <venka...@linux.vnet.ibm.com>
> Closes:
> https://lore.kernel.org/lkml/a70e4064-a040-447b-8556-1fd02f193...@linux.vnet.ibm.com/T/#u
> Cc: Mahesh Salgaonkar <mah...@linux.ibm.com>
> Cc: Michael Ellerman <m...@ellerman.id.au>
> Signed-off-by: Hari Bathini <hbath...@linux.ibm.com>
> Signed-off-by: Sourabh Jain <sourabhj...@linux.ibm.com>
> ---
>
> Changelog:
>
> Since v1:
> https://lore.kernel.org/all/20241104083528.99520-1-sourabhj...@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.l...@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
