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

[Sourabh Jain <sourabhjain@linux.ibm.com>]

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