* [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).