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

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

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