| Message ID | 1448010842-22345-1-git-send-email-chenhui.zhao@freescale.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Scott Wood |
| Headers | show |
Hi Scott, Any comment on these pathes? Thanks, Chenhui On Fri, Nov 20, 2015 at 5:13 PM, Chenhui Zhao <chenhui.zhao@freescale.com> wrote: > On e6500, in the case of cpu hotplug, either thread in one core > may be the first thread initilzing the TLB1. The subsequent threads > must not setup it again. > > The code is derived from the comment of Scott Wood. > > Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com> > --- > arch/powerpc/include/asm/cputhreads.h | 7 +++++++ > arch/powerpc/mm/tlb_nohash.c | 4 +--- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/include/asm/cputhreads.h > b/arch/powerpc/include/asm/cputhreads.h > index ba42e46..b56cece 100644 > --- a/arch/powerpc/include/asm/cputhreads.h > +++ b/arch/powerpc/include/asm/cputhreads.h > @@ -94,6 +94,13 @@ static inline int cpu_last_thread_sibling(int cpu) > return cpu | (threads_per_core - 1); > } > > +static inline u32 get_tensr(void) > +{ > + if (cpu_has_feature(CPU_FTR_SMT)) > + return mfspr(SPRN_TENSR); > + else > + return 1; > +} > > > #endif /* _ASM_POWERPC_CPUTHREADS_H */ > diff --git a/arch/powerpc/mm/tlb_nohash.c > b/arch/powerpc/mm/tlb_nohash.c > index bb04e4d..f466848 100644 > --- a/arch/powerpc/mm/tlb_nohash.c > +++ b/arch/powerpc/mm/tlb_nohash.c > @@ -640,9 +640,7 @@ static void early_init_this_mmu(void) > * transient mapping would cause problems. > */ > #ifdef CONFIG_SMP > - if (cpu != boot_cpuid && > - (cpu != cpu_first_thread_sibling(cpu) || > - cpu == cpu_first_thread_sibling(boot_cpuid))) > + if (hweight32(get_tensr()) > 1) > map = false; > #endif > > -- > 1.9.1 >
On 11/20/15, Chenhui Zhao <chenhui.zhao@freescale.com> wrote: > On e6500, in the case of cpu hotplug, either thread in one core > may be the first thread initilzing the TLB1. The subsequent threads > must not setup it again. > > The code is derived from the comment of Scott Wood. > > Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com> > --- > arch/powerpc/include/asm/cputhreads.h | 7 +++++++ > arch/powerpc/mm/tlb_nohash.c | 4 +--- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/include/asm/cputhreads.h > b/arch/powerpc/include/asm/cputhreads.h > index ba42e46..b56cece 100644 > --- a/arch/powerpc/include/asm/cputhreads.h > +++ b/arch/powerpc/include/asm/cputhreads.h > @@ -94,6 +94,13 @@ static inline int cpu_last_thread_sibling(int cpu) > return cpu | (threads_per_core - 1); > } > > +static inline u32 get_tensr(void) > +{ > + if (cpu_has_feature(CPU_FTR_SMT)) > + return mfspr(SPRN_TENSR); > + else > + return 1; > +} If i get it right, SPRN_TENSR used in the code only if CONFIG_PPC64 is defined. Then we can make it noop on ppc32. Thanks! > > #endif /* _ASM_POWERPC_CPUTHREADS_H */ > diff --git a/arch/powerpc/mm/tlb_nohash.c b/arch/powerpc/mm/tlb_nohash.c > index bb04e4d..f466848 100644 > --- a/arch/powerpc/mm/tlb_nohash.c > +++ b/arch/powerpc/mm/tlb_nohash.c > @@ -640,9 +640,7 @@ static void early_init_this_mmu(void) > * transient mapping would cause problems. > */ > #ifdef CONFIG_SMP > - if (cpu != boot_cpuid && > - (cpu != cpu_first_thread_sibling(cpu) || > - cpu == cpu_first_thread_sibling(boot_cpuid))) > + if (hweight32(get_tensr()) > 1) > map = false; > #endif > > -- > 1.9.1 > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev
On Wed, Dec 2, 2015 at 8:12 PM, Denis Kirjanov <kda@linux-powerpc.org> wrote: > On 11/20/15, Chenhui Zhao <chenhui.zhao@freescale.com> wrote: >> On e6500, in the case of cpu hotplug, either thread in one core >> may be the first thread initilzing the TLB1. The subsequent threads >> must not setup it again. >> >> The code is derived from the comment of Scott Wood. >> >> Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com> >> --- >> arch/powerpc/include/asm/cputhreads.h | 7 +++++++ >> arch/powerpc/mm/tlb_nohash.c | 4 +--- >> 2 files changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/cputhreads.h >> b/arch/powerpc/include/asm/cputhreads.h >> index ba42e46..b56cece 100644 >> --- a/arch/powerpc/include/asm/cputhreads.h >> +++ b/arch/powerpc/include/asm/cputhreads.h >> @@ -94,6 +94,13 @@ static inline int cpu_last_thread_sibling(int >> cpu) >> return cpu | (threads_per_core - 1); >> } >> >> +static inline u32 get_tensr(void) >> +{ >> + if (cpu_has_feature(CPU_FTR_SMT)) >> + return mfspr(SPRN_TENSR); >> + else >> + return 1; >> +} > If i get it right, SPRN_TENSR used in the code only if CONFIG_PPC64 > is defined. Then we can make it noop on ppc32. > > Thanks! Yeah, SPRN_TENSR is defined when CONFIG_BOOKE or CONFIG_40x is enabled. I'd like to change the code like: static inline u32 get_tensr(void) { #ifdef CONFIG_BOOKE if (cpu_has_feature(CPU_FTR_SMT)) return mfspr(SPRN_TENSR); #endif return 1; } Thanks, Chenhui
On Wed, 2015-12-02 at 15:12 +0300, Denis Kirjanov wrote: > On 11/20/15, Chenhui Zhao <chenhui.zhao@freescale.com> wrote: > > On e6500, in the case of cpu hotplug, either thread in one core > > may be the first thread initilzing the TLB1. The subsequent threads > > must not setup it again. > > > > The code is derived from the comment of Scott Wood. > > > > Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com> > > --- > > arch/powerpc/include/asm/cputhreads.h | 7 +++++++ > > arch/powerpc/mm/tlb_nohash.c | 4 +--- > > 2 files changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/arch/powerpc/include/asm/cputhreads.h > > b/arch/powerpc/include/asm/cputhreads.h > > index ba42e46..b56cece 100644 > > --- a/arch/powerpc/include/asm/cputhreads.h > > +++ b/arch/powerpc/include/asm/cputhreads.h > > @@ -94,6 +94,13 @@ static inline int cpu_last_thread_sibling(int cpu) > > return cpu | (threads_per_core - 1); > > } > > > > +static inline u32 get_tensr(void) > > +{ > > + if (cpu_has_feature(CPU_FTR_SMT)) > > + return mfspr(SPRN_TENSR); > > + else > > + return 1; > > +} > If i get it right, SPRN_TENSR used in the code only if CONFIG_PPC64 > is defined. Then we can make it noop on ppc32. Please don't. It accomplishes nothing other than adding an obstacle to supporting this on ppc32. -Scott
On 12/3/15, Scott Wood <scottwood@freescale.com> wrote: > On Wed, 2015-12-02 at 15:12 +0300, Denis Kirjanov wrote: >> On 11/20/15, Chenhui Zhao <chenhui.zhao@freescale.com> wrote: >> > On e6500, in the case of cpu hotplug, either thread in one core >> > may be the first thread initilzing the TLB1. The subsequent threads >> > must not setup it again. >> > >> > The code is derived from the comment of Scott Wood. >> > >> > Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com> >> > --- >> > arch/powerpc/include/asm/cputhreads.h | 7 +++++++ >> > arch/powerpc/mm/tlb_nohash.c | 4 +--- >> > 2 files changed, 8 insertions(+), 3 deletions(-) >> > >> > diff --git a/arch/powerpc/include/asm/cputhreads.h >> > b/arch/powerpc/include/asm/cputhreads.h >> > index ba42e46..b56cece 100644 >> > --- a/arch/powerpc/include/asm/cputhreads.h >> > +++ b/arch/powerpc/include/asm/cputhreads.h >> > @@ -94,6 +94,13 @@ static inline int cpu_last_thread_sibling(int cpu) >> > return cpu | (threads_per_core - 1); >> > } >> > >> > +static inline u32 get_tensr(void) >> > +{ >> > + if (cpu_has_feature(CPU_FTR_SMT)) >> > + return mfspr(SPRN_TENSR); >> > + else >> > + return 1; >> > +} >> If i get it right, SPRN_TENSR used in the code only if CONFIG_PPC64 >> is defined. Then we can make it noop on ppc32. > > Please don't. It accomplishes nothing other than adding an obstacle to > supporting this on ppc32. The idea is make it noop since the function defined in header file and some core parts include it like: arch/powerpc/kernel/smp.c arch/powerpc/kernel/setup-common.c > > -Scott > >
On Fri, 2015-12-04 at 11:04 +0300, Denis Kirjanov wrote: > On 12/3/15, Scott Wood <scottwood@freescale.com> wrote: > > On Wed, 2015-12-02 at 15:12 +0300, Denis Kirjanov wrote: > > > On 11/20/15, Chenhui Zhao <chenhui.zhao@freescale.com> wrote: > > > > On e6500, in the case of cpu hotplug, either thread in one core > > > > may be the first thread initilzing the TLB1. The subsequent threads > > > > must not setup it again. > > > > > > > > The code is derived from the comment of Scott Wood. > > > > > > > > Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com> > > > > --- > > > > arch/powerpc/include/asm/cputhreads.h | 7 +++++++ > > > > arch/powerpc/mm/tlb_nohash.c | 4 +--- > > > > 2 files changed, 8 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/arch/powerpc/include/asm/cputhreads.h > > > > b/arch/powerpc/include/asm/cputhreads.h > > > > index ba42e46..b56cece 100644 > > > > --- a/arch/powerpc/include/asm/cputhreads.h > > > > +++ b/arch/powerpc/include/asm/cputhreads.h > > > > @@ -94,6 +94,13 @@ static inline int cpu_last_thread_sibling(int cpu) > > > > return cpu | (threads_per_core - 1); > > > > } > > > > > > > > +static inline u32 get_tensr(void) > > > > +{ > > > > + if (cpu_has_feature(CPU_FTR_SMT)) > > > > + return mfspr(SPRN_TENSR); > > > > + else > > > > + return 1; > > > > +} > > > If i get it right, SPRN_TENSR used in the code only if CONFIG_PPC64 > > > is defined. Then we can make it noop on ppc32. > > > > Please don't. It accomplishes nothing other than adding an obstacle to > > supporting this on ppc32. > > The idea is make it noop since the function defined in header file and > some core parts include it like: > > arch/powerpc/kernel/smp.c > arch/powerpc/kernel/setup-common.c What does that have to do with making it a no-op on 32-bit? I understand ifdeffing on CONFIG_BOOKE due to the build issue, but not CONFIG_PPC64. -Scott
On Thu, 2015-12-03 at 19:27 +0800, Chenhui Zhao wrote: > > On Wed, Dec 2, 2015 at 8:12 PM, Denis Kirjanov <kda@linux-powerpc.org> > wrote: > > On 11/20/15, Chenhui Zhao <chenhui.zhao@freescale.com> wrote: > > > On e6500, in the case of cpu hotplug, either thread in one core > > > may be the first thread initilzing the TLB1. The subsequent threads > > > must not setup it again. > > > > > > The code is derived from the comment of Scott Wood. > > > > > > Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com> > > > --- > > > arch/powerpc/include/asm/cputhreads.h | 7 +++++++ > > > arch/powerpc/mm/tlb_nohash.c | 4 +--- > > > 2 files changed, 8 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/powerpc/include/asm/cputhreads.h > > > b/arch/powerpc/include/asm/cputhreads.h > > > index ba42e46..b56cece 100644 > > > --- a/arch/powerpc/include/asm/cputhreads.h > > > +++ b/arch/powerpc/include/asm/cputhreads.h > > > @@ -94,6 +94,13 @@ static inline int cpu_last_thread_sibling(int > > > cpu) > > > return cpu | (threads_per_core - 1); > > > } > > > > > > +static inline u32 get_tensr(void) > > > +{ > > > + if (cpu_has_feature(CPU_FTR_SMT)) > > > + return mfspr(SPRN_TENSR); > > > + else > > > + return 1; > > > +} > > If i get it right, SPRN_TENSR used in the code only if CONFIG_PPC64 > > is defined. Then we can make it noop on ppc32. > > > > Thanks! > > Yeah, SPRN_TENSR is defined when CONFIG_BOOKE or CONFIG_40x is enabled. > I'd like to change the code like: > > static inline u32 get_tensr(void) > { > #ifdef CONFIG_BOOKE > if (cpu_has_feature(CPU_FTR_SMT)) > return mfspr(SPRN_TENSR); > #endif > return 1; > } Are you going to respin? -Scott
Hi Scott, I updated the patch a moment ago at http://patchwork.ozlabs.org/patch/560771/ . Thanks, Chenhui
diff --git a/arch/powerpc/include/asm/cputhreads.h b/arch/powerpc/include/asm/cputhreads.h index ba42e46..b56cece 100644 --- a/arch/powerpc/include/asm/cputhreads.h +++ b/arch/powerpc/include/asm/cputhreads.h @@ -94,6 +94,13 @@ static inline int cpu_last_thread_sibling(int cpu) return cpu | (threads_per_core - 1); } +static inline u32 get_tensr(void) +{ + if (cpu_has_feature(CPU_FTR_SMT)) + return mfspr(SPRN_TENSR); + else + return 1; +} #endif /* _ASM_POWERPC_CPUTHREADS_H */ diff --git a/arch/powerpc/mm/tlb_nohash.c b/arch/powerpc/mm/tlb_nohash.c index bb04e4d..f466848 100644 --- a/arch/powerpc/mm/tlb_nohash.c +++ b/arch/powerpc/mm/tlb_nohash.c @@ -640,9 +640,7 @@ static void early_init_this_mmu(void) * transient mapping would cause problems. */ #ifdef CONFIG_SMP - if (cpu != boot_cpuid && - (cpu != cpu_first_thread_sibling(cpu) || - cpu == cpu_first_thread_sibling(boot_cpuid))) + if (hweight32(get_tensr()) > 1) map = false; #endif
On e6500, in the case of cpu hotplug, either thread in one core may be the first thread initilzing the TLB1. The subsequent threads must not setup it again. The code is derived from the comment of Scott Wood. Signed-off-by: Chenhui Zhao <chenhui.zhao@freescale.com> --- arch/powerpc/include/asm/cputhreads.h | 7 +++++++ arch/powerpc/mm/tlb_nohash.c | 4 +--- 2 files changed, 8 insertions(+), 3 deletions(-)