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