Re: [PATCH v2 1/4] powerpc/mmu: do MMU type discovery before crashkernel reservation

On 26/07/26 16:24, Hari Bathini wrote:
> Couple of minor nits below..
>
> On 08/07/26 8:03 pm, Sourabh Jain wrote:
>> Crashkernel reservation on high memory depends on the MMU type, so
>> finalize the MMU type before calling arch_reserve_crashkernel().
>>
>> With the changes introduced here, early_radix_enabled() becomes usable
>> and will be used in arch_reserve_crashkernel() in the upcoming patch.
>>
>> early_radix_enabled() depends on cur_cpu_spec->mmu_features to find
>> out if the radix MMU is enabled. The radix MMU bit in mmu_features is
>> discovered from the FDT and kernel configs. To make sure the MMU type is
>> finalized before arch_reserve_crashkernel() is called, the function that
>> scans the FDT and sets mmu_features, along with some bits from
>> mmu_early_type_finalize(), has been moved above
>> arch_reserve_crashkernel().
>>
>> Signed-off-by: Sourabh Jain <[email protected]>
>> ---
>>   arch/powerpc/include/asm/book3s/64/mmu.h |  1 +
>>   arch/powerpc/include/asm/mmu.h           |  1 +
>>   arch/powerpc/kernel/prom.c               | 28 +++++++++++++-----------
>>   arch/powerpc/mm/init_64.c                | 27 ++++++++++++++---------
>>   4 files changed, 34 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h 
>> b/arch/powerpc/include/asm/book3s/64/mmu.h
>> index 48631365b48c..7a3b2ff02041 100644
>> --- a/arch/powerpc/include/asm/book3s/64/mmu.h
>> +++ b/arch/powerpc/include/asm/book3s/64/mmu.h
>> @@ -208,6 +208,7 @@ extern int mmu_vmemmap_psize;
>>     /* MMU initialization */
>>   void mmu_early_init_devtree(void);
>> +void mmu_early_type_finalize(void);
>>   void hash__early_init_devtree(void);
>>   void radix__early_init_devtree(void);
>>   #ifdef CONFIG_PPC_PKEY
>> diff --git a/arch/powerpc/include/asm/mmu.h 
>> b/arch/powerpc/include/asm/mmu.h
>> index 5f9c5d436e17..c40dc6349e55 100644
>> --- a/arch/powerpc/include/asm/mmu.h
>> +++ b/arch/powerpc/include/asm/mmu.h
>> @@ -384,6 +384,7 @@ extern void early_init_mmu_secondary(void);
>>   extern void setup_initial_memory_limit(phys_addr_t 
>> first_memblock_base,
>>                          phys_addr_t first_memblock_size);
>>   static inline void mmu_early_init_devtree(void) { }
>> +static inline void mmu_early_type_finalize(void) { }
>>     static inline void pkey_early_init_devtree(void) {}
>>   diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>> index 9ed9dde7d231..db1615f26075 100644
>> --- a/arch/powerpc/kernel/prom.c
>> +++ b/arch/powerpc/kernel/prom.c
>> @@ -853,6 +853,21 @@ void __init early_init_devtree(void *params)
>>       if (PHYSICAL_START > MEMORY_START)
>>           memblock_reserve(MEMORY_START, int_vector_size);
>>       reserve_kdump_trampoline();
>> +
>> +    DBG("Scanning CPUs ...\n");
>> +
>> +    dt_cpu_ftrs_scan();
>> +
>> +    /* Retrieve CPU related informations from the flat tree
>> +     * (altivec support, boot CPU ID, ...)
>> +     */
>> +    of_scan_flat_dt(early_init_dt_scan_cpus, NULL);
>> +    if (boot_cpuid < 0) {
>> +        printk("Failed to identify boot CPU !\n");
>> +        BUG();
>> +    }
>> +
>
>> +    mmu_early_type_finalize();
>>   #if defined(CONFIG_FA_DUMP) || defined(CONFIG_PRESERVE_FA_DUMP)
>
> Please add a blank line here
>
>>       /*
>>        * If we fail to reserve memory for firmware-assisted dump then
>> @@ -884,19 +899,6 @@ void __init early_init_devtree(void *params)
>>        * FIXME .. and the initrd too? */
>>       move_device_tree();
>>   -    DBG("Scanning CPUs ...\n");
>> -
>> -    dt_cpu_ftrs_scan();
>> -
>> -    /* Retrieve CPU related informations from the flat tree
>> -     * (altivec support, boot CPU ID, ...)
>> -     */
>> -    of_scan_flat_dt(early_init_dt_scan_cpus, NULL);
>> -    if (boot_cpuid < 0) {
>> -        printk("Failed to identify boot CPU !\n");
>> -        BUG();
>> -    }
>> -
>>       save_fscr_to_task();
>>     #if defined(CONFIG_SMP) && defined(CONFIG_PPC64)
>> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
>> index 64f0df5bb5cd..2fd533bc17cc 100644
>> --- a/arch/powerpc/mm/init_64.c
>> +++ b/arch/powerpc/mm/init_64.c
>> @@ -614,8 +614,10 @@ static void __init 
>> early_init_memory_block_size(void)
>>       of_scan_flat_dt(probe_memory_block_size, &memory_block_size);
>>   }
>>   -void __init mmu_early_init_devtree(void)
>> +
>
>> +void __init mmu_early_type_finalize(void)
>>   {
>> +
>
> Please remove this extra blank line
>
>>       bool hvmode = !!(mfmsr() & MSR_HV);
>>         /* Disable radix mode based on kernel command line. */
>> @@ -626,6 +628,20 @@ void __init mmu_early_init_devtree(void)
>>               pr_warn("WARNING: Ignoring cmdline option 
>> disable_radix\n");
>>       }
>>   +    /*
>> +     * Check /chosen/ibm,architecture-vec-5 if running as a guest.
>> +     * When running bare-metal, we can use radix if we like
>> +     * even though the ibm,architecture-vec-5 property created by
>> +     * skiboot doesn't have the necessary bits set.
>> +     */
>> +    if (!hvmode)
>> +        early_check_vec5();
>> +}
>> +
>> +void __init mmu_early_init_devtree(void)
>> +{
>> +    bool hvmode = !!(mfmsr() & MSR_HV);
>> +
>>       of_scan_flat_dt(dt_scan_mmu_pid_width, NULL);
>>       if (hvmode && !mmu_lpid_bits) {
>>           if (early_cpu_has_feature(CPU_FTR_ARCH_207S))
>> @@ -638,15 +654,6 @@ void __init mmu_early_init_devtree(void)
>>               mmu_pid_bits = 20; /* POWER9-10 */
>>       }
>>   -    /*
>> -     * Check /chosen/ibm,architecture-vec-5 if running as a guest.
>> -     * When running bare-metal, we can use radix if we like
>> -     * even though the ibm,architecture-vec-5 property created by
>> -     * skiboot doesn't have the necessary bits set.
>> -     */
>> -    if (!hvmode)
>> -        early_check_vec5();
>> -
>>       early_init_memory_block_size();
>>         if (early_radix_enabled()) {

I will address the above comments in the next version.

Thanks for the review Hari.

- Sourabh Jain