Sourabh Jain <sourabhj...@linux.ibm.com> writes:
> Hello Ritesh,
>
>
> On 12/11/24 11:51, Ritesh Harjani (IBM) wrote:
>> Sourabh Jain <sourabhj...@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 <mah...@linux.ibm.com>
>>> Cc: Michael Ellerman <m...@ellerman.id.au>
>>> Acked-by: Hari Bathini <hbath...@linux.ibm.com>
>>> Signed-off-by: Sourabh Jain <sourabhj...@linux.ibm.com>
>>> ---
>>>
>>> Changelog:
>>>
>>> Since v1:
>>> https://lore.kernel.org/all/20241104083528.99520-1-sourabhj...@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
