Excerpts from Christophe Leroy's message of October 19, 2021 6:05 pm:
>
>
> Le 15/10/2021 à 17:46, Nicholas Piggin a écrit :
>> Introduce a new option CONFIG_PPC_64S_HASH_MMU which allows the 64s hash
>> MMU code to be compiled out if radix is selected and the minimum
>> supported CPU type is POWER9 or higher, and KVM is not selected.
>>
>> This saves 128kB kernel image size (90kB text) on powernv_defconfig
>> minus KVM, 350kB on pseries_defconfig minus KVM, 40kB on a tiny config.
>
> This patch is huge, it could be split in several smaller patches ?
>
> I'm sure at least the Kconfig stuff can be do as a second step. In first
> step just make CONFIG_PPC_64S_HASH_MMU always y.
I can do that.
>
> I'm wondering if we could also reduce the amount of #ifdefs in C files,
> by using IS_ENABLED() and/or stubs defined in H files.
I didn't see a lot of low hanging things there. A lot of it is struct
members and globals and things. E.g., something like this -
@@ -175,7 +181,9 @@ static int radix__init_new_context(struct mm_struct *mm)
*/
asm volatile("ptesync;isync" : : : "memory");
+#ifdef CONFIG_PPC_64S_HASH_MMU
mm->context.hash_context = NULL;
+#endif
return index;
}
In theory we could add a mm_set_hash_context(mm, NULL) function to do
this for us, but if it is only required in this one bit of radix init
code then I would say the new function actually adds a reading burden
on all the rest of the code that uses it (or if we only use it in
this one place then it's pretty pointless).
>
>>
>> Signed-off-by: Nicholas Piggin <npig...@gmail.com>
>> ---
>> arch/powerpc/Kconfig | 1 +
>> arch/powerpc/include/asm/book3s/64/mmu.h | 22 ++++++++++++++++++-
>> .../include/asm/book3s/64/tlbflush-hash.h | 7 ++++++
>> arch/powerpc/include/asm/book3s/pgtable.h | 4 ++++
>> arch/powerpc/include/asm/mmu.h | 14 +++++++++---
>> arch/powerpc/include/asm/mmu_context.h | 2 ++
>> arch/powerpc/include/asm/paca.h | 8 +++++++
>> arch/powerpc/kernel/asm-offsets.c | 2 ++
>> arch/powerpc/kernel/dt_cpu_ftrs.c | 8 ++++++-
>> arch/powerpc/kernel/entry_64.S | 4 ++--
>> arch/powerpc/kernel/exceptions-64s.S | 16 ++++++++++++++
>> arch/powerpc/kernel/mce.c | 2 +-
>> arch/powerpc/kernel/mce_power.c | 10 ++++++---
>> arch/powerpc/kernel/paca.c | 18 ++++++---------
>> arch/powerpc/kernel/process.c | 13 ++++++-----
>> arch/powerpc/kernel/prom.c | 2 ++
>> arch/powerpc/kernel/setup_64.c | 4 ++++
>> arch/powerpc/kexec/core_64.c | 4 ++--
>> arch/powerpc/kexec/ranges.c | 4 ++++
>> arch/powerpc/kvm/Kconfig | 1 +
>> arch/powerpc/mm/book3s64/Makefile | 17 ++++++++------
>> arch/powerpc/mm/book3s64/hash_utils.c | 10 ---------
>> .../{hash_hugetlbpage.c => hugetlbpage.c} | 6 +++++
>> arch/powerpc/mm/book3s64/mmu_context.c | 16 ++++++++++++++
>> arch/powerpc/mm/book3s64/pgtable.c | 12 ++++++++++
>> arch/powerpc/mm/book3s64/radix_pgtable.c | 4 ++++
>> arch/powerpc/mm/copro_fault.c | 2 ++
>> arch/powerpc/mm/pgtable.c | 10 ++++++---
>> arch/powerpc/platforms/Kconfig.cputype | 21 +++++++++++++++++-
>> arch/powerpc/platforms/cell/Kconfig | 1 +
>> arch/powerpc/platforms/maple/Kconfig | 1 +
>> arch/powerpc/platforms/microwatt/Kconfig | 2 +-
>> arch/powerpc/platforms/pasemi/Kconfig | 1 +
>> arch/powerpc/platforms/powermac/Kconfig | 1 +
>> arch/powerpc/platforms/powernv/Kconfig | 2 +-
>> arch/powerpc/platforms/powernv/idle.c | 2 ++
>> arch/powerpc/platforms/powernv/setup.c | 2 ++
>> arch/powerpc/platforms/pseries/lpar.c | 11 ++++++++--
>> arch/powerpc/platforms/pseries/lparcfg.c | 2 +-
>> arch/powerpc/platforms/pseries/mobility.c | 6 +++++
>> arch/powerpc/platforms/pseries/ras.c | 2 ++
>> arch/powerpc/platforms/pseries/reconfig.c | 2 ++
>> arch/powerpc/platforms/pseries/setup.c | 6 +++--
>> arch/powerpc/xmon/xmon.c | 8 +++++--
>> 44 files changed, 233 insertions(+), 60 deletions(-)
>> rename arch/powerpc/mm/book3s64/{hash_hugetlbpage.c => hugetlbpage.c} (95%)
>>
>
>> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
>> index 8abe8e42e045..0f89fcab834d 100644
>> --- a/arch/powerpc/include/asm/mmu.h
>> +++ b/arch/powerpc/include/asm/mmu.h
>> @@ -157,7 +157,7 @@ DECLARE_PER_CPU(int, next_tlbcam_idx);
>>
>> enum {
>> MMU_FTRS_POSSIBLE =
>> -#if defined(CONFIG_PPC_BOOK3S_64) || defined(CONFIG_PPC_BOOK3S_604)
>> +#if defined(CONFIG_PPC_BOOK3S_604)
>> MMU_FTR_HPTE_TABLE |
>> #endif
>> #ifdef CONFIG_PPC_8xx
>> @@ -184,15 +184,18 @@ enum {
>> MMU_FTR_USE_TLBRSRV | MMU_FTR_USE_PAIRED_MAS |
>> #endif
>> #ifdef CONFIG_PPC_BOOK3S_64
>> + MMU_FTR_KERNEL_RO |
>> +#ifdef CONFIG_PPC_64S_HASH_MMU
>> MMU_FTR_NO_SLBIE_B | MMU_FTR_16M_PAGE | MMU_FTR_TLBIEL |
>> MMU_FTR_LOCKLESS_TLBIE | MMU_FTR_CI_LARGE_PAGE |
>> MMU_FTR_1T_SEGMENT | MMU_FTR_TLBIE_CROP_VA |
>> - MMU_FTR_KERNEL_RO | MMU_FTR_68_BIT_VA |
>> + MMU_FTR_68_BIT_VA | MMU_FTR_HPTE_TABLE |
>> #endif
>> #ifdef CONFIG_PPC_RADIX_MMU
>> MMU_FTR_TYPE_RADIX |
>> MMU_FTR_GTSE |
>> #endif /* CONFIG_PPC_RADIX_MMU */
>> +#endif
>> #ifdef CONFIG_PPC_KUAP
>> MMU_FTR_BOOK3S_KUAP |
>> #endif /* CONFIG_PPC_KUAP */
>> @@ -223,6 +226,11 @@ enum {
>> #ifdef CONFIG_E500
>> #define MMU_FTRS_ALWAYS MMU_FTR_TYPE_FSL_E
>> #endif
>> +#ifdef CONFIG_PPC_BOOK3S_64
>> +#if defined(CONFIG_PPC_RADIX_MMU) && !defined(CONFIG_PPC_64S_HASH_MMU)
>> +#define MMU_FTRS_ALWAYS MMU_FTR_TYPE_RADIX
>> +#endif
>> +#endif
>
> Should you also set MMU_FTR_HPTE_TABLE in MMU_FTRS_ALWAYS when HAS_MMU
> && !RADIX ?
Yeah, good point.
>>
>> #ifndef MMU_FTRS_ALWAYS
>> #define MMU_FTRS_ALWAYS 0
>> @@ -329,7 +337,7 @@ static __always_inline bool radix_enabled(void)
>> return mmu_has_feature(MMU_FTR_TYPE_RADIX);
>> }
>>
>> -static inline bool early_radix_enabled(void)
>> +static __always_inline bool early_radix_enabled(void)
>> {
>> return early_mmu_has_feature(MMU_FTR_TYPE_RADIX);
>> }
>
>> diff --git a/arch/powerpc/mm/book3s64/mmu_context.c
>> b/arch/powerpc/mm/book3s64/mmu_context.c
>> index c10fc8a72fb3..642cabc25e99 100644
>> --- a/arch/powerpc/mm/book3s64/mmu_context.c
>> +++ b/arch/powerpc/mm/book3s64/mmu_context.c
>> @@ -31,6 +31,7 @@ static int alloc_context_id(int min_id, int max_id)
>> return ida_alloc_range(&mmu_context_ida, min_id, max_id, GFP_KERNEL);
>> }
>>
>> +#ifdef CONFIG_PPC_64S_HASH_MMU
>> void hash__reserve_context_id(int id)
>> {
>> int result = ida_alloc_range(&mmu_context_ida, id, id, GFP_KERNEL);
>> @@ -50,7 +51,9 @@ int hash__alloc_context_id(void)
>> return alloc_context_id(MIN_USER_CONTEXT, max);
>> }
>> EXPORT_SYMBOL_GPL(hash__alloc_context_id);
>> +#endif
>>
>> +#ifdef CONFIG_PPC_64S_HASH_MMU
>> static int realloc_context_ids(mm_context_t *ctx)
>> {
>> int i, id;
>> @@ -144,12 +147,15 @@ static int hash__init_new_context(struct mm_struct *mm)
>> return index;
>> }
>>
>> +void slb_setup_new_exec(void);
>> +
>> void hash__setup_new_exec(void)
>> {
>> slice_setup_new_exec();
>>
>> slb_setup_new_exec();
>> }
>> +#endif
>>
>> static int radix__init_new_context(struct mm_struct *mm)
>> {
>> @@ -175,7 +181,9 @@ static int radix__init_new_context(struct mm_struct *mm)
>> */
>> asm volatile("ptesync;isync" : : : "memory");
>>
>> +#ifdef CONFIG_PPC_64S_HASH_MMU
>> mm->context.hash_context = NULL;
>> +#endif
>>
>> return index;
>> }
>> @@ -186,8 +194,10 @@ int init_new_context(struct task_struct *tsk, struct
>> mm_struct *mm)
>>
>> if (radix_enabled())
>> index = radix__init_new_context(mm);
>> +#ifdef CONFIG_PPC_64S_HASH_MMU
>> else
>> index = hash__init_new_context(mm);
>> +#endif
>
> I really dislike #ifdef nested in if/else.
>
> Can you do something like
>
> if (radix_enabled()
> index = radix__init_new_context(mm);
> else if (mmu_has_feature(MMU_FTR_HPTE_TABLE))
> index = hash__init_new_context(mm);
If radix_enabled() constant folds properly then even the 2nd if AFAIKS
should not be required. Maybe it does now after some of your patches.
I'll check.
>
>
>>
>> if (index < 0)
>> return index;
>> @@ -211,6 +221,7 @@ void __destroy_context(int context_id)
>> }
>> EXPORT_SYMBOL_GPL(__destroy_context);
>>
>> +#ifdef CONFIG_PPC_64S_HASH_MMU
>> static void destroy_contexts(mm_context_t *ctx)
>> {
>> int index, context_id;
>> @@ -222,6 +233,7 @@ static void destroy_contexts(mm_context_t *ctx)
>> }
>> kfree(ctx->hash_context);
>> }
>> +#endif
>>
>> static void pmd_frag_destroy(void *pmd_frag)
>> {
>> @@ -274,7 +286,11 @@ void destroy_context(struct mm_struct *mm)
>> process_tb[mm->context.id].prtb0 = 0;
>> else
>> subpage_prot_free(mm);
>> +#ifdef CONFIG_PPC_64S_HASH_MMU
>> destroy_contexts(&mm->context);
>> +#else
>> + ida_free(&mmu_context_ida, mm->context.id);
>
> Is that correct ? Was it done somewhere else before ?
Yeah in destroy_contexts. hash has a extended_id union member that
covers id.
I could just move this into destroy_contexts though, at least remove the
ifdef here.
Thanks,
Nick
