[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 at 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 at linux.ibm.com>
>> Cc: Michael Ellerman <mpe at ellerman.id.au>
>> Acked-by: Hari Bathini <hbathini at linux.ibm.com>
>> Signed-off-by: Sourabh Jain <sourabhjain at 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