* [PATCH 1/1] powerpc: add "memory" attribute for mfmsr() @ 2012-07-10 7:59 Tiejun Chen 2012-07-10 8:19 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 4+ messages in thread From: Tiejun Chen @ 2012-07-10 7:59 UTC (permalink / raw) To: benh; +Cc: linuxppc-dev Add "memory" attribute in inline assembly language as a compiler barrier to make sure 4.6.x GCC don't reorder mfmsr(). Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> --- arch/powerpc/include/asm/reg.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h index 559da19..578e5a0 100644 --- a/arch/powerpc/include/asm/reg.h +++ b/arch/powerpc/include/asm/reg.h @@ -1016,7 +1016,8 @@ /* Macros for setting and retrieving special purpose registers */ #ifndef __ASSEMBLY__ #define mfmsr() ({unsigned long rval; \ - asm volatile("mfmsr %0" : "=r" (rval)); rval;}) + asm volatile("mfmsr %0" : "=r" (rval) : \ + : "memory"); rval;}) #ifdef CONFIG_PPC_BOOK3S_64 #define __mtmsrd(v, l) asm volatile("mtmsrd %0," __stringify(l) \ : : "r" (v) : "memory") -- 1.5.6 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] powerpc: add "memory" attribute for mfmsr() 2012-07-10 7:59 [PATCH 1/1] powerpc: add "memory" attribute for mfmsr() Tiejun Chen @ 2012-07-10 8:19 ` Benjamin Herrenschmidt 2012-07-10 8:22 ` tiejun.chen 0 siblings, 1 reply; 4+ messages in thread From: Benjamin Herrenschmidt @ 2012-07-10 8:19 UTC (permalink / raw) To: Tiejun Chen; +Cc: linuxppc-dev On Tue, 2012-07-10 at 15:59 +0800, Tiejun Chen wrote: > Add "memory" attribute in inline assembly language as a compiler > barrier to make sure 4.6.x GCC don't reorder mfmsr(). Out of curiosity, did you see a case where it was re-ordered improperly ? Cheers, Ben. > Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> > --- > arch/powerpc/include/asm/reg.h | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h > index 559da19..578e5a0 100644 > --- a/arch/powerpc/include/asm/reg.h > +++ b/arch/powerpc/include/asm/reg.h > @@ -1016,7 +1016,8 @@ > /* Macros for setting and retrieving special purpose registers */ > #ifndef __ASSEMBLY__ > #define mfmsr() ({unsigned long rval; \ > - asm volatile("mfmsr %0" : "=r" (rval)); rval;}) > + asm volatile("mfmsr %0" : "=r" (rval) : \ > + : "memory"); rval;}) > #ifdef CONFIG_PPC_BOOK3S_64 > #define __mtmsrd(v, l) asm volatile("mtmsrd %0," __stringify(l) \ > : : "r" (v) : "memory") ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] powerpc: add "memory" attribute for mfmsr() 2012-07-10 8:19 ` Benjamin Herrenschmidt @ 2012-07-10 8:22 ` tiejun.chen 2012-07-10 8:27 ` tiejun.chen 0 siblings, 1 reply; 4+ messages in thread From: tiejun.chen @ 2012-07-10 8:22 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: linuxppc-dev On 07/10/2012 04:19 PM, Benjamin Herrenschmidt wrote: > On Tue, 2012-07-10 at 15:59 +0800, Tiejun Chen wrote: >> Add "memory" attribute in inline assembly language as a compiler >> barrier to make sure 4.6.x GCC don't reorder mfmsr(). > > Out of curiosity, did you see a case where it was re-ordered > improperly ? Yes. In my scenario, there's one simple wrapper from local_irq_save(). ------ uint32_t XX_DisableAllIntr(void) { unsigned long flags; local_irq_save(flags); return (uint32_t)flags; } When CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled, kernel would expand local_irq_save(). Firstly, please refer to the follows: #define local_irq_save(flags) \ do { \ raw_local_irq_save(flags); \ trace_hardirqs_off(); \ } while (0) #define raw_local_irq_save(flags) \ do { \ typecheck(unsigned long, flags); \ flags = arch_local_irq_save(); \ } while (0) static inline unsigned long arch_local_irq_save(void) { unsigned long flags = arch_local_save_flags(); #ifdef CONFIG_BOOKE asm volatile("wrteei 0" : : : "memory"); #else SET_MSR_EE(flags & ~MSR_EE); #endif return flags; } static inline unsigned long arch_local_save_flags(void) { return mfmsr(); } So local_irq_save(flags) is extended as something like: { #1 flags = mfmsr(); #2 asm volatile("wrteei 0" : : : "memory"); #3 trace_hardirqs_off(); } But build kernel with our 5.0 toolchain (with/without --with-toolchain-dir=wr-toolchain), (gdb) disassemble XX_DisableAllIntr Dump of assembler code for function XX_DisableAllIntr: 0xc04550c0 <+0>: mflr r0 0xc04550c4 <+4>: stw r0,4(r1) 0xc04550c8 <+8>: bl 0xc000f060 <mcount> 0xc04550cc <+12>: stwu r1,-16(r1) 0xc04550d0 <+16>: mflr r0 0xc04550d4 <+20>: stw r0,20(r1) 0xc04550d8 <+24>: wrteei 0 0xc04550dc <+28>: bl 0xc00c6c10 <trace_hardirqs_off> 0xc04550e0 <+32>: mfmsr r3 0xc04550e4 <+36>: lwz r0,20(r1) 0xc04550e8 <+40>: mtlr r0 0xc04550ec <+44>: addi r1,r1,16 0xc04550f0 <+48>: blr End of assembler dump. You should notice #2 and #3 is reorganized before #1. This means irq is always disabled before we check irq status via reading msr. Then kernel would work as abnormal sometimes. But with old toolchain (4.5.x) for this same kernel, everything is fine. Tiejun > > Cheers, > Ben. > >> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> >> --- >> arch/powerpc/include/asm/reg.h | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h >> index 559da19..578e5a0 100644 >> --- a/arch/powerpc/include/asm/reg.h >> +++ b/arch/powerpc/include/asm/reg.h >> @@ -1016,7 +1016,8 @@ >> /* Macros for setting and retrieving special purpose registers */ >> #ifndef __ASSEMBLY__ >> #define mfmsr() ({unsigned long rval; \ >> - asm volatile("mfmsr %0" : "=r" (rval)); rval;}) >> + asm volatile("mfmsr %0" : "=r" (rval) : \ >> + : "memory"); rval;}) >> #ifdef CONFIG_PPC_BOOK3S_64 >> #define __mtmsrd(v, l) asm volatile("mtmsrd %0," __stringify(l) \ >> : : "r" (v) : "memory") > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] powerpc: add "memory" attribute for mfmsr() 2012-07-10 8:22 ` tiejun.chen @ 2012-07-10 8:27 ` tiejun.chen 0 siblings, 0 replies; 4+ messages in thread From: tiejun.chen @ 2012-07-10 8:27 UTC (permalink / raw) To: tiejun.chen; +Cc: linuxppc-dev On 07/10/2012 04:22 PM, tiejun.chen wrote: > On 07/10/2012 04:19 PM, Benjamin Herrenschmidt wrote: >> On Tue, 2012-07-10 at 15:59 +0800, Tiejun Chen wrote: >>> Add "memory" attribute in inline assembly language as a compiler >>> barrier to make sure 4.6.x GCC don't reorder mfmsr(). >> >> Out of curiosity, did you see a case where it was re-ordered >> improperly ? > > Yes. > > In my scenario, there's one simple wrapper from local_irq_save(). > ------ > uint32_t XX_DisableAllIntr(void) > { > unsigned long flags; > > local_irq_save(flags); > > return (uint32_t)flags; > } > > When CONFIG_TRACE_IRQFLAGS_SUPPORT is enabled, kernel would expand > local_irq_save(). Firstly, please refer to the follows: > > #define local_irq_save(flags) \ > do { \ > raw_local_irq_save(flags); \ > trace_hardirqs_off(); \ > } while (0) > > #define raw_local_irq_save(flags) \ > do { \ > typecheck(unsigned long, flags); \ > flags = arch_local_irq_save(); \ > } while (0) > > static inline unsigned long arch_local_irq_save(void) > { > unsigned long flags = arch_local_save_flags(); > #ifdef CONFIG_BOOKE > asm volatile("wrteei 0" : : : "memory"); > #else > SET_MSR_EE(flags & ~MSR_EE); > #endif > return flags; > } > > static inline unsigned long arch_local_save_flags(void) > { > return mfmsr(); > } > > So local_irq_save(flags) is extended as something like: > > { > #1 flags = mfmsr(); > #2 asm volatile("wrteei 0" : : : "memory"); > #3 trace_hardirqs_off(); > } > > But build kernel with our 5.0 toolchain (with/without Note here this toolchain is based on 4.6.3. Tiejun > --with-toolchain-dir=wr-toolchain), > > (gdb) disassemble XX_DisableAllIntr > Dump of assembler code for function XX_DisableAllIntr: > 0xc04550c0 <+0>: mflr r0 > 0xc04550c4 <+4>: stw r0,4(r1) > 0xc04550c8 <+8>: bl 0xc000f060 <mcount> > 0xc04550cc <+12>: stwu r1,-16(r1) > 0xc04550d0 <+16>: mflr r0 > 0xc04550d4 <+20>: stw r0,20(r1) > 0xc04550d8 <+24>: wrteei 0 > 0xc04550dc <+28>: bl 0xc00c6c10 <trace_hardirqs_off> > 0xc04550e0 <+32>: mfmsr r3 > 0xc04550e4 <+36>: lwz r0,20(r1) > 0xc04550e8 <+40>: mtlr r0 > 0xc04550ec <+44>: addi r1,r1,16 > 0xc04550f0 <+48>: blr > End of assembler dump. > > You should notice #2 and #3 is reorganized before #1. This means irq is always > disabled before we check irq status via reading msr. Then kernel would work as > abnormal sometimes. > > But with old toolchain (4.5.x) for this same kernel, everything is fine. > > Tiejun > >> >> Cheers, >> Ben. >> >>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com> >>> --- >>> arch/powerpc/include/asm/reg.h | 3 ++- >>> 1 files changed, 2 insertions(+), 1 deletions(-) >>> >>> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h >>> index 559da19..578e5a0 100644 >>> --- a/arch/powerpc/include/asm/reg.h >>> +++ b/arch/powerpc/include/asm/reg.h >>> @@ -1016,7 +1016,8 @@ >>> /* Macros for setting and retrieving special purpose registers */ >>> #ifndef __ASSEMBLY__ >>> #define mfmsr() ({unsigned long rval; \ >>> - asm volatile("mfmsr %0" : "=r" (rval)); rval;}) >>> + asm volatile("mfmsr %0" : "=r" (rval) : \ >>> + : "memory"); rval;}) >>> #ifdef CONFIG_PPC_BOOK3S_64 >>> #define __mtmsrd(v, l) asm volatile("mtmsrd %0," __stringify(l) \ >>> : : "r" (v) : "memory") >> >> >> >> > > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-07-10 8:30 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-07-10 7:59 [PATCH 1/1] powerpc: add "memory" attribute for mfmsr() Tiejun Chen 2012-07-10 8:19 ` Benjamin Herrenschmidt 2012-07-10 8:22 ` tiejun.chen 2012-07-10 8:27 ` tiejun.chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).