From: Nicholas Piggin <npiggin@gmail.com>
To: Christophe Leroy <christophe.leroy@csgroup.eu>,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v2 13/16] powerpc/64s: Move hash MMU code under a new Kconfig name
Date: Thu, 21 Oct 2021 17:33:44 +1000 [thread overview]
Message-ID: <1634800508.2xc9upolsy.astroid@bobo.none> (raw)
In-Reply-To: <affad7bd-64c5-d9eb-c80a-81c4ff67d6e2@csgroup.eu>
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@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@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();
next prev parent reply other threads:[~2021-10-21 7:34 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-21 3:54 [PATCH v2 00/16] powerpc: Make hash MMU code build configurable Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 01/16] powerpc: Remove unused FW_FEATURE_NATIVE references Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 02/16] powerpc: Rename PPC_NATIVE to PPC_HASH_MMU_NATIVE Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 03/16] powerpc/pseries: Stop selecting PPC_HASH_MMU_NATIVE Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 04/16] powerpc/64s: Move and rename do_bad_slb_fault as it is not hash specific Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 05/16] powerpc/pseries: move pseries_lpar_register_process_table() out from hash specific code Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 06/16] powerpc/pseries: lparcfg don't include slb_size line in radix mode Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 07/16] powerpc/64s: move THP trace point creation out of hash specific file Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 08/16] powerpc/64s: Make flush_and_reload_slb a no-op when radix is enabled Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 09/16] powerpc/64s: move page size definitions from hash specific file Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 10/16] powerpc/64s: Rename hash_hugetlbpage.c to hugetlbpage.c Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 11/16] powerpc/64: pcpu setup avoid reading mmu_linear_psize on 64e or radix Nicholas Piggin
2021-10-21 4:52 ` Christophe Leroy
2021-10-21 7:13 ` Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 12/16] powerpc/64e: remove mmu_linear_psize Nicholas Piggin
2021-10-21 5:03 ` Christophe Leroy
2021-10-21 7:15 ` Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 13/16] powerpc/64s: Move hash MMU code under a new Kconfig name Nicholas Piggin
2021-10-21 5:43 ` Christophe Leroy
2021-10-21 7:33 ` Nicholas Piggin [this message]
2021-10-21 3:54 ` [PATCH v2 14/16] powerpc/64s: Make hash MMU support configurable Nicholas Piggin
2021-10-21 5:44 ` Christophe Leroy
2021-10-21 3:54 ` [PATCH v2 15/16] powerpc/configs/microwatt: add POWER9_CPU Nicholas Piggin
2021-10-21 3:54 ` [PATCH v2 16/16] powerpc/microwatt: Don't select the hash MMU code Nicholas Piggin
2021-10-21 5:19 ` Joel Stanley
2021-10-21 7:38 ` Nicholas Piggin
2021-10-21 5:47 ` Christophe Leroy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1634800508.2xc9upolsy.astroid@bobo.none \
--to=npiggin@gmail.com \
--cc=christophe.leroy@csgroup.eu \
--cc=linuxppc-dev@lists.ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.