From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yw0-x241.google.com (mail-yw0-x241.google.com [IPv6:2607:f8b0:4002:c05::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3rTng80cz0zDqh5 for ; Wed, 15 Jun 2016 10:38:12 +1000 (AEST) Received: by mail-yw0-x241.google.com with SMTP id h185so805680ywc.0 for ; Tue, 14 Jun 2016 17:38:11 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <87shwfaf3i.fsf@skywalker.in.ibm.com> References: <87shwfaf3i.fsf@skywalker.in.ibm.com> From: Balbir Singh Date: Wed, 15 Jun 2016 10:38:09 +1000 Message-ID: Subject: Re: [PATCH] New MMU feature for new hpte format in ISA3 To: "Aneesh Kumar K.V" Cc: linuxppc-dev@lists.ozlabs.org, Michael Ellerman , Paul Mackerras , Michael Neuling Content-Type: text/plain; charset=UTF-8 List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Wed, Jun 15, 2016 at 2:51 AM, Aneesh Kumar K.V wrote: > Balbir Singh writes: > >> This patch has been lightly tested and needs testing >> on additional platforms and different page sizes, largely >> due to the mmu_features changes noted below >> >> Commit 50de596de introduced support for the new PTE >> format in the functions hpte_encode_r and hpte_encode_avpn. >> This patch abstracts the change under >> MMU_FTR_ISA3_HPTE_FORMAT. This bit is provided by the >> ibm,pa-feature mechanism; hypervisor for guests >> and via firmware for powernv. > > > IIUC, ISA 3.0 require new hpte format. So not sure why would need a mmu > feature bit for it. For a guest running in P8 compat mode, which is > negotiated via ibm,client-architecture-support , the HCALL interface > should do the necessary mapping from the HPTE format we use with p8 > kernel to the new ISA 3.0 format. > The point is that this capability needs to be advertised. It provides greater flexibility. You are right in that the PTE needs to be stored in the new format, but it should be done if advertised. >> >> The patch has a larger impact as the new feature bit >> changes the definition of mmu_features from unsigned int >> to unsigned long and also changes the print in setup_64.c >> of the same feature. >> >> This patch needs complementary changes to the >> ibm,client-architecture in arch/powerpc/kernel/prom_init.c. >> This is noted as a TODO >> >> Cc: Paul Mackerras >> Cc: Michael Ellerman >> Cc: Aneesh Kumar K.V >> >> Signed-off-by: Balbir Singh >> --- >> arch/powerpc/include/asm/book3s/64/mmu-hash.h | 5 ++--- >> arch/powerpc/include/asm/cputable.h | 2 +- >> arch/powerpc/include/asm/mmu.h | 11 ++++++++++- >> arch/powerpc/kernel/prom.c | 1 + >> arch/powerpc/kernel/setup_64.c | 2 +- >> 5 files changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h >> index 290157e..373f765 100644 >> --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h >> +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h >> @@ -229,7 +229,7 @@ static inline unsigned long hpte_encode_avpn(unsigned long vpn, int psize, >> */ >> v = (vpn >> (23 - VPN_SHIFT)) & ~(mmu_psize_defs[psize].avpnm); >> v <<= HPTE_V_AVPN_SHIFT; >> - if (!cpu_has_feature(CPU_FTR_ARCH_300)) >> + if (!mmu_has_feature(MMU_FTR_ISA3_HPTE_FORMAT)) >> v |= ((unsigned long) ssize) << HPTE_V_SSIZE_SHIFT; >> return v; >> } >> @@ -256,8 +256,7 @@ static inline unsigned long hpte_encode_v(unsigned long vpn, int base_psize, >> static inline unsigned long hpte_encode_r(unsigned long pa, int base_psize, >> int actual_psize, int ssize) >> { >> - >> - if (cpu_has_feature(CPU_FTR_ARCH_300)) >> + if (mmu_has_feature(MMU_FTR_ISA3_HPTE_FORMAT)) >> pa |= ((unsigned long) ssize) << HPTE_R_3_0_SSIZE_SHIFT; >> >> /* A 4K page needs no special encoding */ >> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h >> index df4fb5f..469c9be 100644 >> --- a/arch/powerpc/include/asm/cputable.h >> +++ b/arch/powerpc/include/asm/cputable.h >> @@ -58,7 +58,7 @@ struct cpu_spec { >> unsigned long cpu_features; /* Kernel features */ >> unsigned int cpu_user_features; /* Userland features */ >> unsigned int cpu_user_features2; /* Userland features v2 */ >> - unsigned int mmu_features; /* MMU features */ >> + unsigned long mmu_features; /* MMU features */ >> >> /* cache line sizes */ >> unsigned int icache_bsize; >> diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h >> index e53ebeb..e121db1 100644 >> --- a/arch/powerpc/include/asm/mmu.h >> +++ b/arch/powerpc/include/asm/mmu.h >> @@ -92,6 +92,12 @@ >> * Radix page table available >> */ >> #define MMU_FTR_RADIX ASM_CONST(0x80000000) >> +/* >> + * Support of new PTE format as defined in ISA 3.0 >> + */ >> +#ifdef CONFIG_PPC_STD_MMU_64 >> +#define MMU_FTR_ISA3_HPTE_FORMAT ASM_CONST(0x100000000) >> +#endif >> >> /* MMU feature bit sets for various CPUs */ >> #define MMU_FTRS_DEFAULT_HPTE_ARCH_V2 \ >> @@ -128,10 +134,13 @@ enum { >> #ifdef CONFIG_PPC_RADIX_MMU >> MMU_FTR_RADIX | >> #endif >> +#ifdef CONFIG_PPC_STD_MMU_64 >> + MMU_FTR_ISA3_HPTE_FORMAT | >> +#endif >> 0, >> }; >> >> -static inline int mmu_has_feature(unsigned long feature) >> +static inline unsigned long mmu_has_feature(unsigned long feature) >> { >> return (MMU_FTRS_POSSIBLE & cur_cpu_spec->mmu_features & feature); >> } >> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c >> index 946e34f..f9cb4a9 100644 >> --- a/arch/powerpc/kernel/prom.c >> +++ b/arch/powerpc/kernel/prom.c >> @@ -169,6 +169,7 @@ static struct ibm_pa_feature { >> {CPU_FTR_TM_COMP, 0, 0, >> PPC_FEATURE2_HTM_COMP|PPC_FEATURE2_HTM_NOSC_COMP, 22, 0, 0}, >> {0, MMU_FTR_RADIX, 0, 0, 40, 0, 0}, >> + {0, MMU_FTR_ISA3_HPTE_FORMAT, 0, 0, 58, 0, 0}, > > > IIUC, that bit indicate the usage of segment table. Not really new HPTE > format. With ISA 3.0, hash mode always need the new hpte format. Hence it > is not a something we could disable. ie, if we are on ISA 3.0 and using > hash mode, hash page table entries should have new format. > The bit is for new hashing facility as per ISA 3.0. The plan of the overall implementation is This bit is advertised when new hashing capabilities are supported via pa-features. The client the selects whether it supports it or not and the hypervisor does the right thing for legacy guests. > >> }; >> >> static void __init scan_features(unsigned long node, const unsigned char *ftrs, >> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c >> index 96d4a2b..2e15490 100644 >> --- a/arch/powerpc/kernel/setup_64.c >> +++ b/arch/powerpc/kernel/setup_64.c >> @@ -561,7 +561,7 @@ void __init setup_system(void) >> pr_info(" always = 0x%016lx\n", CPU_FTRS_ALWAYS); >> pr_info("cpu_user_features = 0x%08x 0x%08x\n", cur_cpu_spec->cpu_user_features, >> cur_cpu_spec->cpu_user_features2); >> - pr_info("mmu_features = 0x%08x\n", cur_cpu_spec->mmu_features); >> + pr_info("mmu_features = 0x%016lx\n", cur_cpu_spec->mmu_features); >> pr_info("firmware_features = 0x%016lx\n", powerpc_firmware_features); >> >> #ifdef CONFIG_PPC_STD_MMU_64 >> -- >> 2.5.5 > Thanks for a review, I'll do a V2 with some changes Balbir