Re: [PATCH RESEND v1 4/5] powerpc/crash: use generic crashkernel reservation

[Sourabh Jain <sourabhjain@linux.ibm.com>]

Hello Mahesh,

On 08/01/25 22:35, Mahesh J Salgaonkar wrote:
> On 2025-01-08 15:44:57 Wed, Sourabh Jain wrote:
>> Commit 0ab97169aa05 ("crash_core: add generic function to do
>> reservation") added a generic function to reserve crashkernel memory.
>> So let's use the same function on powerpc and remove the
>> architecture-specific code that essentially does the same thing.
>>
>> The generic crashkernel reservation also provides a way to split the
>> crashkernel reservation into high and low memory reservations, which can
>> be enabled for powerpc in the future.
>>
>> Along with moving to the generic crashkernel reservation, the code
>> related to finding the base address for the crashkernel has been
>> separated into its own function name get_crash_base() for better
>> readability and maintainability.
>>
>> To prevent crashkernel memory from being added to iomem_resource, the Mahesh.
>> function arch_add_crash_res_to_iomem() has been introduced. For further
>> details on why this should not be done for the PowerPC architecture,
>> please refer to the previous commit titled "crash: let arch decide crash
>> memory export to iomem_resource.
>>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Baoquan he <bhe@redhat.com>
>> Cc: Hari Bathini <hbathini@linux.ibm.com>
>> CC: Madhavan Srinivasan <maddy@linux.ibm.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: kexec@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Sourabh Jain <sourabhjain@linux.ibm.com>
>> ---
>>   arch/powerpc/Kconfig                     |  3 +
>>   arch/powerpc/include/asm/crash_reserve.h | 18 +++++
>>   arch/powerpc/include/asm/kexec.h         |  4 +-
>>   arch/powerpc/kernel/prom.c               |  2 +-
>>   arch/powerpc/kexec/core.c                | 90 ++++++++++--------------
>>   5 files changed, 63 insertions(+), 54 deletions(-)
>>   create mode 100644 arch/powerpc/include/asm/crash_reserve.h
> [...]
>> @@ -113,9 +113,9 @@ int setup_new_fdt_ppc64(const struct kimage *image, void *fdt, struct crash_mem
>>   
>>   #ifdef CONFIG_CRASH_RESERVE
>>   int __init overlaps_crashkernel(unsigned long start, unsigned long size);
>> -extern void reserve_crashkernel(void);
>> +extern void arch_reserve_crashkernel(void); Mahesh. Mahesh
> Do we really need to rename this ? it is still called from powepc arch
> and not from the common code.

You are right, we don’t. However, all architectures (x86, RISC-V, 
LoongArch, ARM64) that use the
generic crash kernel reservation have named their architecture-specific 
function `arch_reserve_crashkernel()`.
So, I did the same for PowerPC, and this helps sometimes.

Maybe I should justify the name change in the commit to avoid confusion.

Please let me know your opinion.

>
>>   #else
>> -static inline void reserve_crashkernel(void) {}
>> +static inline void arch_reserve_crashkernel(void) {}
>>   static inline int overlaps_crashkernel(unsigned long start, unsigned long size) { return 0; }
>>   #endif
>>   
>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> index e0059842a1c6..9ed9dde7d231 100644
>> --- a/arch/powerpc/kernel/prom.c
>> +++ b/arch/powerpc/kernel/prom.c
>> @@ -860,7 +860,7 @@ void __init early_init_devtree(void *params)
>>   	 */
>>   	if (fadump_reserve_mem() == 0)
>>   #endif
>> -		reserve_crashkernel();
>> +		arch_reserve_crashkernel();
>>   	early_reserve_mem();
>>   
>>   	if (memory_limit > memblock_phys_mem_size())
> Rest looks good to me.
>
> Reviewed-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>

Thank you for reviewing this.

- Sourabh Jain