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