[PATCH v2 1/2] powerpc/fadump: allocate memory for additional parameters early

[PATCH v2 1/2] powerpc/fadump: allocate memory for additional parameters early

[Sourabh Jain]

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

[PATCH v2 2/2] fadump: reserve param area if below boot_mem_top

[Sourabh Jain]

The param area is a memory region where the kernel places additional
command-line arguments for fadump kernel. Currently, the param memory
area is reserved in fadump kernel if it is above boot_mem_top. However,
it should be reserved if it is below boot_mem_top because the fadump
kernel already reserves memory from boot_mem_top to the end of DRAM.

Currently, there is no impact from not reserving param memory if it is
below boot_mem_top, as it is not used after the early boot phase of the
fadump kernel. However, if this changes in the future, it could lead to
issues in the fadump kernel.

Fixes: 3416c9daa6b1 ("powerpc/fadump: pass additional parameters when fadump is active")
Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Acked-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/
  - Include Fixes and Acked-by tag in the commit message
  - No functional changes

---
 arch/powerpc/kernel/fadump.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 3a2863307863..3f3674060164 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -143,7 +143,7 @@ void __init fadump_append_bootargs(void)
 	if (!fw_dump.dump_active || !fw_dump.param_area_supported || !fw_dump.param_area)
 		return;
 
-	if (fw_dump.param_area >= fw_dump.boot_mem_top) {
+	if (fw_dump.param_area < fw_dump.boot_mem_top) {
 		if (memblock_reserve(fw_dump.param_area, COMMAND_LINE_SIZE)) {
 			pr_warn("WARNING: Can't use additional parameters area!\n");
 			fw_dump.param_area = 0;
-- 
2.46.2

Re: [PATCH v2 1/2] powerpc/fadump: allocate memory for additional parameters early

[Venkat]

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
> 

Re: [PATCH v2 1/2] powerpc/fadump: allocate memory for additional parameters early

[Venkat Rao Bagalkote]

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);

Re: [PATCH v2 1/2] powerpc/fadump: allocate memory for additional parameters early

[Sourabh Jain]

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);

Re: [PATCH v2 2/2] fadump: reserve param area if below boot_mem_top

[Ritesh Harjani (IBM)]

Sourabh Jain <sourabhjain@linux.ibm.com> writes:

> The param area is a memory region where the kernel places additional
> command-line arguments for fadump kernel. Currently, the param memory
> area is reserved in fadump kernel if it is above boot_mem_top. However,
> it should be reserved if it is below boot_mem_top because the fadump
> kernel already reserves memory from boot_mem_top to the end of DRAM.

did you mean s/reserves/preserves ?

>
> Currently, there is no impact from not reserving param memory if it is
> below boot_mem_top, as it is not used after the early boot phase of the
> fadump kernel. However, if this changes in the future, it could lead to
> issues in the fadump kernel.

This will only affect Hash and not radix correct? Because for radix your
param_area is somewhere within [memblock_end_of_DRAM() / 2, memblock_end_of_DRAM()]
which is anyway above boot_mem_top so it is anyway preserved as is... 

... On second thoughts since param_area during normal kernel boot anyway
comes from memblock now. And irrespective of where it falls (above or below
boot_mem_top), we anyway append the bootargs to that. So we don't really
preserve the original contents :) right? So why not just always call for
memblock_reserve() on param_area during capture kernel run?

Thoughts?

>
> Fixes: 3416c9daa6b1 ("powerpc/fadump: pass additional parameters when fadump is active")
> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Acked-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/
>   - Include Fixes and Acked-by tag in the commit message
>   - No functional changes
>
> ---
>  arch/powerpc/kernel/fadump.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index 3a2863307863..3f3674060164 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -143,7 +143,7 @@ void __init fadump_append_bootargs(void)
>  	if (!fw_dump.dump_active || !fw_dump.param_area_supported || !fw_dump.param_area)
>  		return;
>  
> -	if (fw_dump.param_area >= fw_dump.boot_mem_top) {
> +	if (fw_dump.param_area < fw_dump.boot_mem_top) {
>  		if (memblock_reserve(fw_dump.param_area, COMMAND_LINE_SIZE)) {
>  			pr_warn("WARNING: Can't use additional parameters area!\n");
>  			fw_dump.param_area = 0;
> -- 
> 2.46.2

Re: [PATCH v2 1/2] powerpc/fadump: allocate memory for additional parameters early

[Ritesh Harjani (IBM)]

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

Re: [PATCH v2 1/2] powerpc/fadump: allocate memory for additional parameters early

[Sourabh Jain]

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

Re: [PATCH v2 2/2] fadump: reserve param area if below boot_mem_top

[Sourabh Jain]

Hello Ritesh,


On 12/11/24 11:51, Ritesh Harjani (IBM) wrote:
> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>
>> The param area is a memory region where the kernel places additional
>> command-line arguments for fadump kernel. Currently, the param memory
>> area is reserved in fadump kernel if it is above boot_mem_top. However,
>> it should be reserved if it is below boot_mem_top because the fadump
>> kernel already reserves memory from boot_mem_top to the end of DRAM.
> did you mean s/reserves/preserves ?

Yeah, preserves is better.

>
>> Currently, there is no impact from not reserving param memory if it is
>> below boot_mem_top, as it is not used after the early boot phase of the
>> fadump kernel. However, if this changes in the future, it could lead to
>> issues in the fadump kernel.
> This will only affect Hash and not radix correct? Because for radix your
> param_area is somewhere within [memblock_end_of_DRAM() / 2, memblock_end_of_DRAM()]
> which is anyway above boot_mem_top so it is anyway preserved as is...

Yes.

>
> ... On second thoughts since param_area during normal kernel boot anyway
> comes from memblock now. And irrespective of where it falls (above or below
> boot_mem_top), we anyway append the bootargs to that. So we don't really
> preserve the original contents :) right?

Sorry I didn't get it. We append strings from param_area to 
boot_command_line
not the other way.


> So why not just always call for
> memblock_reserve() on param_area during capture kernel run?
>
> Thoughts?

Yes, there is no harm in calling memblock_reserve regardless of whether 
param_area
is below or above boot_mem_top. However, calling it when param_area is 
higher than
boot_mem_top is redundant, as we know fadump preserves memory from 
boot_mem_top
to the end of DRAM during early boot.

According to the memblock documentation, when reserving memory regions, 
the new
regions can overlap with existing ones, but I don't see any advantage in 
calling memblock_reserve
for param_area if it falls above boot_mem_top.

Regardless, I don’t have a strong opinion. If you think we should call 
memblock_reserve regardless
of where param_area is placed, I can do that. Please let me know your 
opinion.

Sourabh Jain



>
>> Fixes: 3416c9daa6b1 ("powerpc/fadump: pass additional parameters when fadump is active")
>> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Acked-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/
>>    - Include Fixes and Acked-by tag in the commit message
>>    - No functional changes
>>
>> ---
>>   arch/powerpc/kernel/fadump.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
>> index 3a2863307863..3f3674060164 100644
>> --- a/arch/powerpc/kernel/fadump.c
>> +++ b/arch/powerpc/kernel/fadump.c
>> @@ -143,7 +143,7 @@ void __init fadump_append_bootargs(void)
>>   	if (!fw_dump.dump_active || !fw_dump.param_area_supported || !fw_dump.param_area)
>>   		return;
>>   
>> -	if (fw_dump.param_area >= fw_dump.boot_mem_top) {
>> +	if (fw_dump.param_area < fw_dump.boot_mem_top) {
>>   		if (memblock_reserve(fw_dump.param_area, COMMAND_LINE_SIZE)) {
>>   			pr_warn("WARNING: Can't use additional parameters area!\n");
>>   			fw_dump.param_area = 0;
>> -- 
>> 2.46.2

Re: [PATCH v2 2/2] fadump: reserve param area if below boot_mem_top

[Ritesh Harjani]

Sourabh Jain <sourabhjain@linux.ibm.com> writes:

> Hello Ritesh,
>
>
> On 12/11/24 11:51, Ritesh Harjani (IBM) wrote:
>> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>>
>>> The param area is a memory region where the kernel places additional
>>> command-line arguments for fadump kernel. Currently, the param memory
>>> area is reserved in fadump kernel if it is above boot_mem_top. However,
>>> it should be reserved if it is below boot_mem_top because the fadump
>>> kernel already reserves memory from boot_mem_top to the end of DRAM.
>> did you mean s/reserves/preserves ?
>
> Yeah, preserves is better.
>
>>
>>> Currently, there is no impact from not reserving param memory if it is
>>> below boot_mem_top, as it is not used after the early boot phase of the
>>> fadump kernel. However, if this changes in the future, it could lead to
>>> issues in the fadump kernel.
>> This will only affect Hash and not radix correct? Because for radix your
>> param_area is somewhere within [memblock_end_of_DRAM() / 2, memblock_end_of_DRAM()]
>> which is anyway above boot_mem_top so it is anyway preserved as is...
>
> Yes.
>
>>
>> ... On second thoughts since param_area during normal kernel boot anyway
>> comes from memblock now. And irrespective of where it falls (above or below
>> boot_mem_top), we anyway append the bootargs to that. So we don't really
>> preserve the original contents :) right?
>
> Sorry I didn't get it. We append strings from param_area to 
> boot_command_line
> not the other way.
>
>

Right. My bad. 

>> So why not just always call for
>> memblock_reserve() on param_area during capture kernel run?
>>
>> Thoughts?
>
> Yes, there is no harm in calling memblock_reserve regardless of whether 
> param_area
> is below or above boot_mem_top. However, calling it when param_area is 
> higher than
> boot_mem_top is redundant, as we know fadump preserves memory from 
> boot_mem_top
> to the end of DRAM during early boot.

So if we don't reserve the param_area then the kernel may use it for
some other purposes once memory is released to buddy, right. But I guess,
given we anyway copied the param_area in fadump_append_bootargs() during
early boot to cmdline (before parse_early_param()), we anyway don't need
it for later, right?

In that case we don't need for Hash too (i.e when param_area falls under
boot_mem_top), right? Since we anyway copied the param_area before
parse_early_param() in fadump_append_bootargs. So what is the point in
calling memblock_reserve() on that? Maybe I am missing something, can
you please help explain.

-ritesh

>
> According to the memblock documentation, when reserving memory regions, 
> the new
> regions can overlap with existing ones, but I don't see any advantage in 
> calling memblock_reserve
> for param_area if it falls above boot_mem_top.
>
> Regardless, I don’t have a strong opinion. If you think we should call 
> memblock_reserve regardless
> of where param_area is placed, I can do that. Please let me know your 
> opinion.
>
> Sourabh Jain
>
>
>
>>
>>> Fixes: 3416c9daa6b1 ("powerpc/fadump: pass additional parameters when fadump is active")
>>> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>> Acked-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/
>>>    - Include Fixes and Acked-by tag in the commit message
>>>    - No functional changes
>>>
>>> ---
>>>   arch/powerpc/kernel/fadump.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
>>> index 3a2863307863..3f3674060164 100644
>>> --- a/arch/powerpc/kernel/fadump.c
>>> +++ b/arch/powerpc/kernel/fadump.c
>>> @@ -143,7 +143,7 @@ void __init fadump_append_bootargs(void)
>>>   	if (!fw_dump.dump_active || !fw_dump.param_area_supported || !fw_dump.param_area)
>>>   		return;
>>>   
>>> -	if (fw_dump.param_area >= fw_dump.boot_mem_top) {
>>> +	if (fw_dump.param_area < fw_dump.boot_mem_top) {
>>>   		if (memblock_reserve(fw_dump.param_area, COMMAND_LINE_SIZE)) {
>>>   			pr_warn("WARNING: Can't use additional parameters area!\n");
>>>   			fw_dump.param_area = 0;
>>> -- 
>>> 2.46.2

Re: [PATCH v2 2/2] fadump: reserve param area if below boot_mem_top

[Ritesh Harjani]

Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:

> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>
>> Hello Ritesh,
>>
>>
>> On 12/11/24 11:51, Ritesh Harjani (IBM) wrote:
>>> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>>>
>>>> The param area is a memory region where the kernel places additional
>>>> command-line arguments for fadump kernel. Currently, the param memory
>>>> area is reserved in fadump kernel if it is above boot_mem_top. However,
>>>> it should be reserved if it is below boot_mem_top because the fadump
>>>> kernel already reserves memory from boot_mem_top to the end of DRAM.
>>> did you mean s/reserves/preserves ?
>>
>> Yeah, preserves is better.
>>
>>>
>>>> Currently, there is no impact from not reserving param memory if it is
>>>> below boot_mem_top, as it is not used after the early boot phase of the
>>>> fadump kernel. However, if this changes in the future, it could lead to
>>>> issues in the fadump kernel.
>>> This will only affect Hash and not radix correct? Because for radix your
>>> param_area is somewhere within [memblock_end_of_DRAM() / 2, memblock_end_of_DRAM()]
>>> which is anyway above boot_mem_top so it is anyway preserved as is...
>>
>> Yes.
>>
>>>
>>> ... On second thoughts since param_area during normal kernel boot anyway
>>> comes from memblock now. And irrespective of where it falls (above or below
>>> boot_mem_top), we anyway append the bootargs to that. So we don't really
>>> preserve the original contents :) right?
>>
>> Sorry I didn't get it. We append strings from param_area to 
>> boot_command_line
>> not the other way.
>>
>>
>
> Right. My bad. 
>
>>> So why not just always call for
>>> memblock_reserve() on param_area during capture kernel run?
>>>
>>> Thoughts?
>>
>> Yes, there is no harm in calling memblock_reserve regardless of whether 
>> param_area
>> is below or above boot_mem_top. However, calling it when param_area is 
>> higher than
>> boot_mem_top is redundant, as we know fadump preserves memory from 
>> boot_mem_top
>> to the end of DRAM during early boot.
>
> So if we don't reserve the param_area then the kernel may use it for
> some other purposes once memory is released to buddy, right. But I guess,
> given we anyway copied the param_area in fadump_append_bootargs() during
> early boot to cmdline (before parse_early_param()), we anyway don't need
> it for later, right?
>
> In that case we don't need for Hash too (i.e when param_area falls under
> boot_mem_top), right? Since we anyway copied the param_area before
> parse_early_param() in fadump_append_bootargs. So what is the point in
> calling memblock_reserve() on that? Maybe I am missing something, can
> you please help explain.
>

Ok. I think I got it now. You did mention in the changelog - 

"Currently, there is no impact from not reserving param memory if it is
below boot_mem_top, as it is not used after the early boot phase of the
fadump kernel. However, if this changes in the future, it could lead to
issues in the fadump kernel."


So it is not an issue now, since the param area is not used after the
contents is copied over. So I think today we anyway don't need to call
memblock_reserve() on the param area - but if we are making it future
proof then we might as well just call memblock_reserve() on param_area
irrespective because otherwise once the kernel starts up it might re-use
that area for other purposes. So isn't it better to reserve for fadump
use of the param_area for either during early boot or during late kernel
boot phase of the capture kernel?

But now that I understand I don't have a strong opinion too (since it is
just future proofing). But I would prefer the safer approach of doing
memblock_reserve() always for param_area. So I will leave it upto you
and others. 

>>
>> According to the memblock documentation, when reserving memory regions, 
>> the new
>> regions can overlap with existing ones, but I don't see any advantage in 
>> calling memblock_reserve
>> for param_area if it falls above boot_mem_top.
>>
>> Regardless, I don’t have a strong opinion. If you think we should call 
>> memblock_reserve regardless
>> of where param_area is placed, I can do that. Please let me know your 
>> opinion.
>>
>> Sourabh Jain
>>
>>
>>
>>>
>>>> Fixes: 3416c9daa6b1 ("powerpc/fadump: pass additional parameters when fadump is active")

Not really IIUC, this is not really a fix but a future proofing of if
fadump ever tries to use param_area later during early boot or during 
late kernel boot.

-ritesh

>>>> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>>> Acked-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/
>>>>    - Include Fixes and Acked-by tag in the commit message
>>>>    - No functional changes
>>>>
>>>> ---
>>>>   arch/powerpc/kernel/fadump.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
>>>> index 3a2863307863..3f3674060164 100644
>>>> --- a/arch/powerpc/kernel/fadump.c
>>>> +++ b/arch/powerpc/kernel/fadump.c
>>>> @@ -143,7 +143,7 @@ void __init fadump_append_bootargs(void)
>>>>   	if (!fw_dump.dump_active || !fw_dump.param_area_supported || !fw_dump.param_area)
>>>>   		return;
>>>>   
>>>> -	if (fw_dump.param_area >= fw_dump.boot_mem_top) {
>>>> +	if (fw_dump.param_area < fw_dump.boot_mem_top) {
>>>>   		if (memblock_reserve(fw_dump.param_area, COMMAND_LINE_SIZE)) {
>>>>   			pr_warn("WARNING: Can't use additional parameters area!\n");
>>>>   			fw_dump.param_area = 0;
>>>> -- 
>>>> 2.46.2

Re: [PATCH v2 2/2] fadump: reserve param area if below boot_mem_top

[Sourabh Jain]

Hello Ritesh,


On 12/11/24 17:23, Ritesh Harjani (IBM) wrote:
> Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:
>
>> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>>
>>> Hello Ritesh,
>>>
>>>
>>> On 12/11/24 11:51, Ritesh Harjani (IBM) wrote:
>>>> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>>>>
>>>>> The param area is a memory region where the kernel places additional
>>>>> command-line arguments for fadump kernel. Currently, the param memory
>>>>> area is reserved in fadump kernel if it is above boot_mem_top. However,
>>>>> it should be reserved if it is below boot_mem_top because the fadump
>>>>> kernel already reserves memory from boot_mem_top to the end of DRAM.
>>>> did you mean s/reserves/preserves ?
>>> Yeah, preserves is better.
>>>
>>>>> Currently, there is no impact from not reserving param memory if it is
>>>>> below boot_mem_top, as it is not used after the early boot phase of the
>>>>> fadump kernel. However, if this changes in the future, it could lead to
>>>>> issues in the fadump kernel.
>>>> This will only affect Hash and not radix correct? Because for radix your
>>>> param_area is somewhere within [memblock_end_of_DRAM() / 2, memblock_end_of_DRAM()]
>>>> which is anyway above boot_mem_top so it is anyway preserved as is...
>>> Yes.
>>>
>>>> ... On second thoughts since param_area during normal kernel boot anyway
>>>> comes from memblock now. And irrespective of where it falls (above or below
>>>> boot_mem_top), we anyway append the bootargs to that. So we don't really
>>>> preserve the original contents :) right?
>>> Sorry I didn't get it. We append strings from param_area to
>>> boot_command_line
>>> not the other way.
>>>
>>>
>> Right. My bad.
>>
>>>> So why not just always call for
>>>> memblock_reserve() on param_area during capture kernel run?
>>>>
>>>> Thoughts?
>>> Yes, there is no harm in calling memblock_reserve regardless of whether
>>> param_area
>>> is below or above boot_mem_top. However, calling it when param_area is
>>> higher than
>>> boot_mem_top is redundant, as we know fadump preserves memory from
>>> boot_mem_top
>>> to the end of DRAM during early boot.
>> So if we don't reserve the param_area then the kernel may use it for
>> some other purposes once memory is released to buddy, right. But I guess,
>> given we anyway copied the param_area in fadump_append_bootargs() during
>> early boot to cmdline (before parse_early_param()), we anyway don't need
>> it for later, right?
>>
>> In that case we don't need for Hash too (i.e when param_area falls under
>> boot_mem_top), right? Since we anyway copied the param_area before
>> parse_early_param() in fadump_append_bootargs. So what is the point in
>> calling memblock_reserve() on that? Maybe I am missing something, can
>> you please help explain.
>>
> Ok. I think I got it now. You did mention in the changelog -
>
> "Currently, there is no impact from not reserving param memory if it is
> below boot_mem_top, as it is not used after the early boot phase of the
> fadump kernel. However, if this changes in the future, it could lead to
> issues in the fadump kernel."
>
>
> So it is not an issue now, since the param area is not used after the
> contents is copied over. So I think today we anyway don't need to call
> memblock_reserve() on the param area - but if we are making it future
> proof then we might as well just call memblock_reserve() on param_area
> irrespective because otherwise once the kernel starts up it might re-use
> that area for other purposes. So isn't it better to reserve for fadump
> use of the param_area for either during early boot or during late kernel
> boot phase of the capture kernel?

Seems like there is some confusion. Here is the full picture with the 
current patch:

First kernel boot: Reserve param_area during early boot and let it 
remain reserved.

First kernel crashed

Fadump/second kernel boot

fadump_reserve_mem() does memblock_reserve() from boot_mem_top to 
end_of_dram().
This covers param_area if it is above boot_mem_top.

fadump_setup_param_area() does memblock_reserve() for param_area only if 
it falls below
boot_mem_top. This ensures it is covered if param_area falls below 
boot_mem_top.

This way, we make sure that param_area is preserved during the fadump 
kernel's early boot in both cases.

Note: fadump_reserve_mem() is executed before fadump_setup_param_area().

IIUC, you are suggesting doing memblock_reserve() for param_area in 
fadump_setup_param_area() regardless
of its location. If we do this, memblock_reserve() will be called twice 
for the case where it falls above
boot_mem_top. I agree there is no harm in doing so, but do we have to?

Again, I don't have a strong opinion on this but just wanted to put 
things forward to make sure we are on the
same page. Let me know your opinion.

Thanks,
Sourabh Jain


>>> According to the memblock documentation, when reserving memory regions,
>>> the new
>>> regions can overlap with existing ones, but I don't see any advantage in
>>> calling memblock_reserve
>>> for param_area if it falls above boot_mem_top.
>>>
>>> Regardless, I don’t have a strong opinion. If you think we should call
>>> memblock_reserve regardless
>>> of where param_area is placed, I can do that. Please let me know your
>>> opinion.
>>>
>>> Sourabh Jain
>>>
>>>
>>>
>>>>> Fixes: 3416c9daa6b1 ("powerpc/fadump: pass additional parameters when fadump is active")
> Not really IIUC, this is not really a fix but a future proofing of if
> fadump ever tries to use param_area later during early boot or during
> late kernel boot.

The reason I put the Fixes tags because we mistakenly put the wrong 
condition there.
The intention was to reserve memory if it below boot_mem_top.
But yes if see this patch as future proofing the Fixes tag can be avoided.

- Sourabh Jain

>
>>>>> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
>>>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>>>> Acked-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/
>>>>>     - Include Fixes and Acked-by tag in the commit message
>>>>>     - No functional changes
>>>>>
>>>>> ---
>>>>>    arch/powerpc/kernel/fadump.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
>>>>> index 3a2863307863..3f3674060164 100644
>>>>> --- a/arch/powerpc/kernel/fadump.c
>>>>> +++ b/arch/powerpc/kernel/fadump.c
>>>>> @@ -143,7 +143,7 @@ void __init fadump_append_bootargs(void)
>>>>>    	if (!fw_dump.dump_active || !fw_dump.param_area_supported || !fw_dump.param_area)
>>>>>    		return;
>>>>>    
>>>>> -	if (fw_dump.param_area >= fw_dump.boot_mem_top) {
>>>>> +	if (fw_dump.param_area < fw_dump.boot_mem_top) {
>>>>>    		if (memblock_reserve(fw_dump.param_area, COMMAND_LINE_SIZE)) {
>>>>>    			pr_warn("WARNING: Can't use additional parameters area!\n");
>>>>>    			fw_dump.param_area = 0;
>>>>> -- 
>>>>> 2.46.2

Re: [PATCH v2 2/2] fadump: reserve param area if below boot_mem_top

[Ritesh Harjani]

Sourabh Jain <sourabhjain@linux.ibm.com> writes:

> Hello Ritesh,
>
>
> On 12/11/24 17:23, Ritesh Harjani (IBM) wrote:
>> Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:
>>
>>> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>>>
>>>> Hello Ritesh,
>>>>
>>>>
>>>> On 12/11/24 11:51, Ritesh Harjani (IBM) wrote:
>>>>> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>>>>>
>>>>>> The param area is a memory region where the kernel places additional
>>>>>> command-line arguments for fadump kernel. Currently, the param memory
>>>>>> area is reserved in fadump kernel if it is above boot_mem_top. However,
>>>>>> it should be reserved if it is below boot_mem_top because the fadump
>>>>>> kernel already reserves memory from boot_mem_top to the end of DRAM.
>>>>> did you mean s/reserves/preserves ?
>>>> Yeah, preserves is better.
>>>>
>>>>>> Currently, there is no impact from not reserving param memory if it is
>>>>>> below boot_mem_top, as it is not used after the early boot phase of the
>>>>>> fadump kernel. However, if this changes in the future, it could lead to
>>>>>> issues in the fadump kernel.
>>>>> This will only affect Hash and not radix correct? Because for radix your
>>>>> param_area is somewhere within [memblock_end_of_DRAM() / 2, memblock_end_of_DRAM()]
>>>>> which is anyway above boot_mem_top so it is anyway preserved as is...
>>>> Yes.
>>>>
>>>>> ... On second thoughts since param_area during normal kernel boot anyway
>>>>> comes from memblock now. And irrespective of where it falls (above or below
>>>>> boot_mem_top), we anyway append the bootargs to that. So we don't really
>>>>> preserve the original contents :) right?
>>>> Sorry I didn't get it. We append strings from param_area to
>>>> boot_command_line
>>>> not the other way.
>>>>
>>>>
>>> Right. My bad.
>>>
>>>>> So why not just always call for
>>>>> memblock_reserve() on param_area during capture kernel run?
>>>>>
>>>>> Thoughts?
>>>> Yes, there is no harm in calling memblock_reserve regardless of whether
>>>> param_area
>>>> is below or above boot_mem_top. However, calling it when param_area is
>>>> higher than
>>>> boot_mem_top is redundant, as we know fadump preserves memory from
>>>> boot_mem_top
>>>> to the end of DRAM during early boot.
>>> So if we don't reserve the param_area then the kernel may use it for
>>> some other purposes once memory is released to buddy, right. But I guess,
>>> given we anyway copied the param_area in fadump_append_bootargs() during
>>> early boot to cmdline (before parse_early_param()), we anyway don't need
>>> it for later, right?
>>>
>>> In that case we don't need for Hash too (i.e when param_area falls under
>>> boot_mem_top), right? Since we anyway copied the param_area before
>>> parse_early_param() in fadump_append_bootargs. So what is the point in
>>> calling memblock_reserve() on that? Maybe I am missing something, can
>>> you please help explain.
>>>
>> Ok. I think I got it now. You did mention in the changelog -
>>
>> "Currently, there is no impact from not reserving param memory if it is
>> below boot_mem_top, as it is not used after the early boot phase of the
>> fadump kernel. However, if this changes in the future, it could lead to
>> issues in the fadump kernel."
>>
>>
>> So it is not an issue now, since the param area is not used after the
>> contents is copied over. So I think today we anyway don't need to call
>> memblock_reserve() on the param area - but if we are making it future
>> proof then we might as well just call memblock_reserve() on param_area
>> irrespective because otherwise once the kernel starts up it might re-use
>> that area for other purposes. So isn't it better to reserve for fadump
>> use of the param_area for either during early boot or during late kernel
>> boot phase of the capture kernel?
>
> Seems like there is some confusion. Here is the full picture with the 
> current patch:
>
> First kernel boot: Reserve param_area during early boot and let it 
> remain reserved.
>
> First kernel crashed
>
> Fadump/second kernel boot
>
> fadump_reserve_mem() does memblock_reserve() from boot_mem_top to 
> end_of_dram().
> This covers param_area if it is above boot_mem_top.
>
> fadump_setup_param_area() does memblock_reserve() for param_area only if 
> it falls below
> boot_mem_top. This ensures it is covered if param_area falls below 
> boot_mem_top.
>
> This way, we make sure that param_area is preserved during the fadump 
> kernel's early boot in both cases.
>
> Note: fadump_reserve_mem() is executed before fadump_setup_param_area().
>

Ohk. I think I missd to look into the dump_active portion of the code
which is where the fadump calls fadump_reserve_mem() -> fadump_reserve_crash_area().
I will be pay more attention to these details the next time :) 

> IIUC, you are suggesting doing memblock_reserve() for param_area in 
> fadump_setup_param_area() regardless
> of its location. If we do this, memblock_reserve() will be called twice 
> for the case where it falls above
> boot_mem_top. I agree there is no harm in doing so, but do we have to?
>
> Again, I don't have a strong opinion on this but just wanted to put 
> things forward to make sure we are on the
> same page. Let me know your opinion.

No. The current patch is doing the right thing. Thanks for taking time
and answering my queries. I agree with the approach and logic taken in
this patch including that of the "Fixes" tag. 

The patch looks good to me. Please feel free to add - 

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

-ritesh

Re: [PATCH v2 2/2] fadump: reserve param area if below boot_mem_top

[Sourabh Jain]

Hello Ritesh,

On 12/11/24 18:40, Ritesh Harjani (IBM) wrote:
> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>
>> Hello Ritesh,
>>
>>
>> On 12/11/24 17:23, Ritesh Harjani (IBM) wrote:
>>> Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:
>>>
>>>> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>>>>
>>>>> Hello Ritesh,
>>>>>
>>>>>
>>>>> On 12/11/24 11:51, Ritesh Harjani (IBM) wrote:
>>>>>> Sourabh Jain <sourabhjain@linux.ibm.com> writes:
>>>>>>
>>>>>>> The param area is a memory region where the kernel places additional
>>>>>>> command-line arguments for fadump kernel. Currently, the param memory
>>>>>>> area is reserved in fadump kernel if it is above boot_mem_top. However,
>>>>>>> it should be reserved if it is below boot_mem_top because the fadump
>>>>>>> kernel already reserves memory from boot_mem_top to the end of DRAM.
>>>>>> did you mean s/reserves/preserves ?
>>>>> Yeah, preserves is better.
>>>>>
>>>>>>> Currently, there is no impact from not reserving param memory if it is
>>>>>>> below boot_mem_top, as it is not used after the early boot phase of the
>>>>>>> fadump kernel. However, if this changes in the future, it could lead to
>>>>>>> issues in the fadump kernel.
>>>>>> This will only affect Hash and not radix correct? Because for radix your
>>>>>> param_area is somewhere within [memblock_end_of_DRAM() / 2, memblock_end_of_DRAM()]
>>>>>> which is anyway above boot_mem_top so it is anyway preserved as is...
>>>>> Yes.
>>>>>
>>>>>> ... On second thoughts since param_area during normal kernel boot anyway
>>>>>> comes from memblock now. And irrespective of where it falls (above or below
>>>>>> boot_mem_top), we anyway append the bootargs to that. So we don't really
>>>>>> preserve the original contents :) right?
>>>>> Sorry I didn't get it. We append strings from param_area to
>>>>> boot_command_line
>>>>> not the other way.
>>>>>
>>>>>
>>>> Right. My bad.
>>>>
>>>>>> So why not just always call for
>>>>>> memblock_reserve() on param_area during capture kernel run?
>>>>>>
>>>>>> Thoughts?
>>>>> Yes, there is no harm in calling memblock_reserve regardless of whether
>>>>> param_area
>>>>> is below or above boot_mem_top. However, calling it when param_area is
>>>>> higher than
>>>>> boot_mem_top is redundant, as we know fadump preserves memory from
>>>>> boot_mem_top
>>>>> to the end of DRAM during early boot.
>>>> So if we don't reserve the param_area then the kernel may use it for
>>>> some other purposes once memory is released to buddy, right. But I guess,
>>>> given we anyway copied the param_area in fadump_append_bootargs() during
>>>> early boot to cmdline (before parse_early_param()), we anyway don't need
>>>> it for later, right?
>>>>
>>>> In that case we don't need for Hash too (i.e when param_area falls under
>>>> boot_mem_top), right? Since we anyway copied the param_area before
>>>> parse_early_param() in fadump_append_bootargs. So what is the point in
>>>> calling memblock_reserve() on that? Maybe I am missing something, can
>>>> you please help explain.
>>>>
>>> Ok. I think I got it now. You did mention in the changelog -
>>>
>>> "Currently, there is no impact from not reserving param memory if it is
>>> below boot_mem_top, as it is not used after the early boot phase of the
>>> fadump kernel. However, if this changes in the future, it could lead to
>>> issues in the fadump kernel."
>>>
>>>
>>> So it is not an issue now, since the param area is not used after the
>>> contents is copied over. So I think today we anyway don't need to call
>>> memblock_reserve() on the param area - but if we are making it future
>>> proof then we might as well just call memblock_reserve() on param_area
>>> irrespective because otherwise once the kernel starts up it might re-use
>>> that area for other purposes. So isn't it better to reserve for fadump
>>> use of the param_area for either during early boot or during late kernel
>>> boot phase of the capture kernel?
>> Seems like there is some confusion. Here is the full picture with the
>> current patch:
>>
>> First kernel boot: Reserve param_area during early boot and let it
>> remain reserved.
>>
>> First kernel crashed
>>
>> Fadump/second kernel boot
>>
>> fadump_reserve_mem() does memblock_reserve() from boot_mem_top to
>> end_of_dram().
>> This covers param_area if it is above boot_mem_top.
>>
>> fadump_setup_param_area() does memblock_reserve() for param_area only if
>> it falls below
>> boot_mem_top. This ensures it is covered if param_area falls below
>> boot_mem_top.
>>
>> This way, we make sure that param_area is preserved during the fadump
>> kernel's early boot in both cases.
>>
>> Note: fadump_reserve_mem() is executed before fadump_setup_param_area().
>>
> Ohk. I think I missd to look into the dump_active portion of the code
> which is where the fadump calls fadump_reserve_mem() -> fadump_reserve_crash_area().
> I will be pay more attention to these details the next time :)
>
>> IIUC, you are suggesting doing memblock_reserve() for param_area in
>> fadump_setup_param_area() regardless
>> of its location. If we do this, memblock_reserve() will be called twice
>> for the case where it falls above
>> boot_mem_top. I agree there is no harm in doing so, but do we have to?
>>
>> Again, I don't have a strong opinion on this but just wanted to put
>> things forward to make sure we are on the
>> same page. Let me know your opinion.
> No. The current patch is doing the right thing. Thanks for taking time
> and answering my queries. I agree with the approach and logic taken in
> this patch including that of the "Fixes" tag.
>
> The patch looks good to me. Please feel free to add -
>
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Thank you for putting in the effort to review that patch.

- Sourabh Jain

Re: [PATCH v2 1/2] powerpc/fadump: allocate memory for additional parameters early

[Michael Ellerman]

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