Hello Ritesh,

On 12/11/24 18:40, Ritesh Harjani (IBM) wrote:

Sourabh Jain [off-list ref] writes:

quoted

Hello Ritesh,


On 12/11/24 17:23, Ritesh Harjani (IBM) wrote:

quoted

Ritesh Harjani (IBM) [off-list ref] writes:

quoted

Sourabh Jain [off-list ref] writes:

quoted

Hello Ritesh,


On 12/11/24 11:51, Ritesh Harjani (IBM) wrote:

quoted

Sourabh Jain [off-list ref] writes:

quoted

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.

quoted

quoted

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.

quoted

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

quoted

quoted

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

quoted

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>

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

- Sourabh Jain