Sourabh Jain <[email protected]> writes:
> 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().
>
That is correct. mmu_features may as well get set from this path too...
early_init_dt_scan_cpus() ->
if (!dt_cpu_ftrs_in_use())
-> check_cpu_features(node, "ibm_pa_features",...
cur_cpu_spec->mmu_features |= fp->mmu_features
...which I guess is controlled using CONFIG_PPC_DT_CPU_FTRS.
so it make sense to move those dependent paths above.
Overall the patch looks good to me. Added few minor nits below.
> Cc: Baoquan he <[email protected]>
> Cc: Hari Bathini <[email protected]>
> Cc: Madhavan Srinivasan <[email protected]>
> Cc: Mahesh Salgaonkar <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: Ritesh Harjani (IBM) <[email protected]>
> Cc: Shivang Upadhyay <[email protected]>
> 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);
Minor nit:
Can we please rename this function to - mmu_early_init_vec5()?
Your naming isn't wrong, but it's just known that after vec5 call, we
finalize the mmu early init type.. So keeping this function name as
"mmu_early_init_vec5()" makes slightly more sense to me.
And then the order of function declarations can also be kept like below -
/* MMU initialization */
+void mmu_early_init_vec5(void);
void mmu_early_init_devtree(void);
diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
+static inline void mmu_early_init_vec5(void) { }
static inline void mmu_early_init_devtree(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)
> /*
> * 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 b6f3ae03ca9e..cd52c1baa3bc 100644
> --- a/arch/powerpc/mm/init_64.c
> +++ b/arch/powerpc/mm/init_64.c
> @@ -622,8 +622,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)
Let's also add a comment here to be more explicit perhaps. Because it has
caused confusion in past.
/*
* mmu_early_init_vec5(): For non-hv mode (Pseries LPAR), whether we can do
* radix or not depend upon hypervisor vec5 values. This functions checks
* ibm,architecture-vec-5 and updates cur_cpu_spec->mmu_features bits
* accordingly.
* After this function returns, early_radix_enabled() can be used
* to check if radix is supported.
*/
> +
> +void __init mmu_early_type_finalize(void)
> {
> +
> bool hvmode = !!(mfmsr() & MSR_HV);
>
> /* Disable radix mode based on kernel command line. */
> @@ -634,6 +636,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();
> +}
-ritesh
