[PATCH v2 13/16] powerpc/64s: Move hash MMU code under a new Kconfig name

Nicholas Piggin npiggin at gmail.com
Thu Oct 21 18:33:44 AEDT 2021


Excerpts from Christophe Leroy's message of October 21, 2021 3:43 pm:
> 
> 
> Le 21/10/2021 à 05:54, Nicholas Piggin a écrit :
>> Introduce a new option CONFIG_PPC_64S_HASH_MMU, and make 64s hash
>> code depend on it.
>> 
>> Signed-off-by: Nicholas Piggin <npiggin at gmail.com>
>> ---
>>   arch/powerpc/Kconfig                          |  2 +-
>>   arch/powerpc/include/asm/book3s/64/mmu.h      | 19 +++++++++--
>>   .../include/asm/book3s/64/tlbflush-hash.h     |  7 ++++
>>   arch/powerpc/include/asm/book3s/pgtable.h     |  4 +++
>>   arch/powerpc/include/asm/mmu.h                | 16 +++++++--
>>   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             | 14 +++++---
>>   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                |  5 +++
>>   arch/powerpc/kexec/core_64.c                  |  4 +--
>>   arch/powerpc/kexec/ranges.c                   |  4 +++
>>   arch/powerpc/mm/book3s64/Makefile             | 15 ++++----
>>   arch/powerpc/mm/book3s64/hugetlbpage.c        |  2 ++
>>   arch/powerpc/mm/book3s64/mmu_context.c        | 34 +++++++++++++++----
>>   arch/powerpc/mm/book3s64/radix_pgtable.c      |  4 +++
>>   arch/powerpc/mm/copro_fault.c                 |  2 ++
>>   arch/powerpc/mm/ioremap.c                     | 13 ++++---
>>   arch/powerpc/mm/pgtable.c                     | 10 ++++--
>>   arch/powerpc/mm/ptdump/Makefile               |  2 +-
>>   arch/powerpc/platforms/Kconfig.cputype        |  4 +++
>>   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 +++--
>>   drivers/misc/lkdtm/Makefile                   |  2 +-
>>   drivers/misc/lkdtm/core.c                     |  2 +-
>>   39 files changed, 219 insertions(+), 64 deletions(-)
> 
> I'm still unconfortable with the quantity of files impacted in that commit.

Hmm. Splitting it into N partial patches that have the same result
doesn't seem better to me. There's a few other little things that are
be better split out, but size of the patch will not shrink much.

>> diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h b/arch/powerpc/include/asm/book3s/64/mmu.h
>> index c02f42d1031e..d94ebae386b6 100644
>> --- a/arch/powerpc/include/asm/book3s/64/mmu.h
>> +++ b/arch/powerpc/include/asm/book3s/64/mmu.h
> 
>> @@ -193,8 +198,15 @@ static inline struct subpage_prot_table *mm_ctx_subpage_prot(mm_context_t *ctx)
>>   extern int mmu_linear_psize;
>>   extern int mmu_virtual_psize;
>>   extern int mmu_vmalloc_psize;
>> -extern int mmu_vmemmap_psize;
>>   extern int mmu_io_psize;
>> +#else /* CONFIG_PPC_64S_HASH_MMU */
>> +#ifdef CONFIG_PPC_64K_PAGES
> 
> Avoid nested #ifdefs and do
> 
> #elif defined(CONFIG_PPC_64K_PAGES)

I sort of like the nesting because this is the !HASH block. But I don't 
care much so I can do it your way if you prefer.

>> @@ -223,6 +226,13 @@ enum {
>>   #ifdef CONFIG_E500
>>   #define MMU_FTRS_ALWAYS		MMU_FTR_TYPE_FSL_E
>>   #endif
>> +#ifdef CONFIG_PPC_BOOK3S_64
> 
> No need of this CONFIG_PPC_BOOK3S_64 ifdef, it is necessarily defined if 
> either CONFIG_PPC_RADIX_MMU or CONFIG_PPC_64S_HASH_MMU is defined

Good point.

>> +#if defined(CONFIG_PPC_RADIX_MMU) && !defined(CONFIG_PPC_64S_HASH_MMU)
>> +#define MMU_FTRS_ALWAYS		MMU_FTR_TYPE_RADIX
>> +#elif !defined(CONFIG_PPC_RADIX_MMU) && defined(CONFIG_PPC_64S_HASH_MMU)
>> +#define MMU_FTRS_ALWAYS		MMU_FTR_HPTE_TABLE
>> +#endif
>> +#endif
>>   
>>   #ifndef MMU_FTRS_ALWAYS
>>   #define MMU_FTRS_ALWAYS		0
> 
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> index 046c99e31d01..65b695e9401e 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -1369,11 +1369,15 @@ EXC_COMMON_BEGIN(data_access_common)
>>   	addi	r3,r1,STACK_FRAME_OVERHEAD
>>   	andis.	r0,r4,DSISR_DABRMATCH at h
>>   	bne-	1f
>> +#ifdef CONFIG_PPC_64S_HASH_MMU
>>   BEGIN_MMU_FTR_SECTION
>>   	bl	do_hash_fault
>>   MMU_FTR_SECTION_ELSE
>>   	bl	do_page_fault
>>   ALT_MMU_FTR_SECTION_END_IFCLR(MMU_FTR_TYPE_RADIX)
>> +#else
>> +	bl	do_page_fault
>> +#endif
> 
> Maybe we could always branch to do_page_fault() and get redirected to 
> do_hash_fault() from do_page_fault() if BOOK3S64 && !radix_enabled() ?

Not trivial because of do_hash_fault().

A book3s_64_do_page_fault() raw hander which calls do_hash_fault or 
do_page_fault if necessary, but that's less optimal and we already
have this gunk so might as well keep it.

> 
> Another solution is to make it a GAS macro ?

For this I don't find it too bad. Another macro means another thing to 
look up. Maybe if we had more callers but for now this isn't too 
terrible.

>> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
>> index 7d4bcbc3124e..59b1a1833143 100644
>> --- a/arch/powerpc/kernel/setup_64.c
>> +++ b/arch/powerpc/kernel/setup_64.c
>> @@ -887,6 +887,8 @@ void __init setup_per_cpu_areas(void)
>>   	} else if (radix_enabled()) {
>>   		atom_size = PAGE_SIZE;
>>   	} else {
>> +#ifdef CONFIG_PPC_64S_HASH_MMU
> 
> Use IS_ENABLED()

Can't because of mmu_linear_psize.

>> diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
>> index cd16b407f47e..ab105d33e0b0 100644
>> --- a/arch/powerpc/mm/pgtable.c
>> +++ b/arch/powerpc/mm/pgtable.c
>> @@ -81,9 +81,6 @@ static struct page *maybe_pte_to_page(pte_t pte)
>>   
>>   static pte_t set_pte_filter_hash(pte_t pte)
>>   {
>> -	if (radix_enabled())
>> -		return pte;
>> -
>>   	pte = __pte(pte_val(pte) & ~_PAGE_HPTEFLAGS);
>>   	if (pte_looks_normal(pte) && !(cpu_has_feature(CPU_FTR_COHERENT_ICACHE) ||
>>   				       cpu_has_feature(CPU_FTR_NOEXECUTE))) {
>> @@ -112,6 +109,9 @@ static inline pte_t set_pte_filter(pte_t pte)
>>   {
>>   	struct page *pg;
>>   
>> +	if (radix_enabled())
>> +		return pte;
>> +
>>   	if (mmu_has_feature(MMU_FTR_HPTE_TABLE))
>>   		return set_pte_filter_hash(pte);
>>   
>> @@ -144,6 +144,10 @@ static pte_t set_access_flags_filter(pte_t pte, struct vm_area_struct *vma,
>>   {
>>   	struct page *pg;
>>   
>> +#ifdef CONFIG_PPC_BOOK3S_64
>> +	return pte;
>> +#endif
> 
> Aren't you changing the behaviour here for RADIX ?

No, 64s always has MMU_FTR_HPTE_TABLE until now. I should actually split 
this change out.

> Anyway, can use IS_ENABLED()

Good point.

> 
>> +
>>   	if (mmu_has_feature(MMU_FTR_HPTE_TABLE))
>>   		return pte;
>>   
>> diff --git a/arch/powerpc/mm/ptdump/Makefile b/arch/powerpc/mm/ptdump/Makefile
>> index 4050cbb55acf..b533caaf0910 100644
>> --- a/arch/powerpc/mm/ptdump/Makefile
>> +++ b/arch/powerpc/mm/ptdump/Makefile
>> @@ -10,5 +10,5 @@ obj-$(CONFIG_PPC_BOOK3S_64)	+= book3s64.o
>>   
>>   ifdef CONFIG_PTDUMP_DEBUGFS
>>   obj-$(CONFIG_PPC_BOOK3S_32)	+= bats.o segment_regs.o
>> -obj-$(CONFIG_PPC_BOOK3S_64)	+= hashpagetable.o
>> +obj-$(CONFIG_PPC_64S_HASH_MMU)	+= hashpagetable.o
>>   endif
>> diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
>> index a208997ade88..01726e7f2c7f 100644
>> --- a/arch/powerpc/platforms/Kconfig.cputype
>> +++ b/arch/powerpc/platforms/Kconfig.cputype
>> @@ -105,6 +105,7 @@ config PPC_BOOK3S_64
>>   	select HAVE_MOVE_PMD
>>   	select HAVE_MOVE_PUD
>>   	select IRQ_WORK
>> +	select PPC_64S_HASH_MMU
> 
> Not needed
> 
>>   	select PPC_MM_SLICES
>>   	select PPC_HAVE_KUEP
>>   	select PPC_HAVE_KUAP
>> @@ -364,6 +365,9 @@ config SPE
>>   
>>   	  If in doubt, say Y here.
>>   
>> +config PPC_64S_HASH_MMU
>> +	bool
> 
> Add
> 	depends on PPC_BOOK3S_64
> 	default y
> 
>> +
>>   config PPC_RADIX_MMU
>>   	bool "Radix MMU Support"
>>   	depends on PPC_BOOK3S_64
> 
>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> index dd8241c009e5..33de8d798c95 100644
>> --- a/arch/powerpc/xmon/xmon.c
>> +++ b/arch/powerpc/xmon/xmon.c
>> @@ -1160,9 +1160,11 @@ cmds(struct pt_regs *excp)
>>   			show_tasks();
>>   			break;
>>   #ifdef CONFIG_PPC_BOOK3S
>> +#ifdef CONFIG_PPC_64S_HASH_MMU
> 
> And BOOK3S/32 ???

Oops, good catch.

Thanks,
Nick

> 
>>   		case 'u':
>>   			dump_segments();
>>   			break;
>> +#endif
>>   #elif defined(CONFIG_44x)
>>   		case 'u':
>>   			dump_tlb_44x();


More information about the Linuxppc-dev mailing list