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.c...@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
>
>
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
