Re: [PATCH 1/1] powerpc: add "memory" attribute for mfmsr()

["tiejun.chen" <tiejun.chen@windriver.com>]

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")
> 
> 
> 
>