[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 at linux.ibm.com> writes:
>
>> Hello Ritesh,
>>
>>
>> On 12/11/24 17:23, Ritesh Harjani (IBM) wrote:
>>> Ritesh Harjani (IBM) <ritesh.list at gmail.com> writes:
>>>
>>>> Sourabh Jain <sourabhjain at linux.ibm.com> writes:
>>>>
>>>>> 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.
>>>>>
>>>>>
>>>> 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 at gmail.com>

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

- Sourabh Jain