ARM: Correct WFE() in asm/spinlock.h for Thumb-2
Subsystems (2)
PATCH 1/1 ARM: Correct WFE() in asm/spinlock.h for Thumb-2
The content for ALT_SMP() in the definition of WFE() expands to 6 bytes (IT cc ; WFEcc.W), which breaks the assumptions of the fixup code, leading to lockups when the affected code gets run.
This patch works around the problem by explicitly using an IT + WFEcc.N pair.
--- arch/arm/include/asm/spinlock.h | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
arch/arm/include/asm/spinlock.h +6 -1
@@ -18,7 +18,12 @@#ifdef CONFIG_THUMB2_KERNEL #define SEV ALT_SMP("sev.w", "nop.w") -#define WFE (cond) ALT_SMP("wfe" cond ".w", "nop.w") +#define WFE (cond) ALT_SMP( \ + "it " cond "\n\t" \ + "wfe" cond ".n", \ + \ + "nop.w" \ +)#else #define SEV ALT_SMP("sev", "nop")#define WFE (cond) ALT_SMP("wfe" cond, "nop")
Signature · 1 line
1.7.1
PATCH 1/1 ARM: Correct WFE() in asm/spinlock.h for Thumb-2
Quoted 3 lines · Dave Martin
The content for
ALT_SMP()in the definition ofWFE()expands to 6 bytes (IT cc ; WFEcc.W), which breaks the assumptions of the fixup code, leading to lockups when the affected code gets run.
Why does it? My understanding is that the .w suffix forces the assembler to use the ARM encoding for the instruction.
Thumb2 is just far too weird to understand. I think I'll just stick to writing good ARM code and let people sort out the Thumb2 weirdnesses.
PATCH 1/1 ARM: Correct WFE() in asm/spinlock.h for Thumb-2
Hi,
Quoted 7 lines · Russell King - ARM Linux
Quoted 3 lines · Dave Martin
The content for
ALT_SMP()in the definition ofWFE()expands to 6 bytes (IT cc ; WFEcc.W), which breaks the assumptions of the fixup code, leading to lockups when the affected code gets run.Why does it? ?My understanding is that the .w suffix forces the assembler to use the ARM encoding for the instruction.
The .w suffix forces a 32-bit encoding of Thumb-2 instructions for which a 16-bit encoding is available. Unfortunately, the 32-bit Thumb-2 encodings are different from the ARM encodings (though there are some odd similarities ... for example, all MCR and MRC encodings are the same ... but with the halfwords swapped (!))
Like the 16-bit forms, most 32-bit Thumb-2 instructions don't contain a condition field, so the assembler -mimplicit-it feature will need insert an IT instruction if you write something conditional. This applies to everything except branches.
wfe: wfene.w becomes:
00000000 <wfe>:
0: bf18 it ne
2: f3af 8002 wfene.w
Fortunately, wfe also has a 16-bit form, so we can squeeze the IT and the WFE into the 4 bytes expected by ALT_SMP(). For safety, I write it out explicitly: ALT_SMP("wfene", ...) would work sometimes but might not work as expected if the previous instruction has an EQ or NE condition, because then the assembler wouldn't need to insert an extra IT when it generates its output...
Quoted 2 lines
Thumb2 is just far too weird to understand. ?I think I'll just stick to writing good ARM code and let people sort out the Thumb2 weirdnesses.
Certainly life is much easier in ARM-land :)
I'm hoping that getting this working for OMAP will allow more people to play with it though.
Cheers
---Dave
PATCH 1/1 ARM: Correct WFE() in asm/spinlock.h for Thumb-2
Hi Dave,
Quoted 12 lines
Quoted 7 lines · Russell King - ARM Linux
Quoted 3 lines · Dave Martin
The content for
ALT_SMP()in the definition ofWFE()expands to 6 bytes (IT cc ; WFEcc.W), which breaks the assumptions of the fixup code, leading to lockups when the affected code gets run.Why does it? My understanding is that the .w suffix forces the assembler to use the ARM encoding for the instruction.
The .w suffix forces a 32-bit encoding of Thumb-2 instructions for which a 16-bit encoding is available. Unfortunately, the 32-bit
[...]
Quoted 5 lines
the WFE into the 4 bytes expected by
ALT_SMP(). For safety, I write it out explicitly:ALT_SMP("wfene", ...)would work sometimes but might not work as expected if the previous instruction has an EQ or NE condition, because then the assembler wouldn't need to insert an extra IT when it generates its output...
That's really horrible! It might be worth commenting the explicit IT in your WFE macro to explain why we can't rely on the assembler to generate it for us.
With that minor change:
Will
PATCH 1/1 ARM: Correct WFE() in asm/spinlock.h for Thumb-2
Quoted 9 lines · Will Deacon
Quoted 5 lines
the WFE into the 4 bytes expected by
ALT_SMP(). For safety, I write it out explicitly:ALT_SMP("wfene", ...)would work sometimes but might not work as expected if the previous instruction has an EQ or NE condition, because then the assembler wouldn't need to insert an extra IT when it generates its output...That's really horrible! It might be worth commenting the explicit IT in your WFE macro to explain why we can't rely on the assembler to generate it for us.
Yes, it's really really horrible. As I say, I think in future I'm just going to stick to writing good ARM code and let others worry about how to make it work properly on T2.
PATCH 1/1 ARM: Correct WFE() in asm/spinlock.h for Thumb-2
In reply to Dave MartinQuoted 17 lines · Dave Martin
The content for
ALT_SMP()in the definition ofWFE()expands to 6 bytes (IT cc ; WFEcc.W), which breaks the assumptions of the fixup code, leading to lockups when the affected code gets run.This patch works around the problem by explicitly using an IT + WFEcc.N pair.
Signed-off-by Dave Martin <[email protected]>--- arch/arm/include/asm/spinlock.h | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)arch/arm/include/asm/spinlock.h +0
-0
arch/arm/include/asm/spinlock.h line 26
Quoted 10 lines
arch/arm/include/asm/spinlock.h +6
-1@@ -18,7 +18,12 @@#ifdef CONFIG_THUMB2_KERNEL #define SEV ALT_SMP("sev.w", "nop.w") -#define WFE (cond) ALT_SMP("wfe" cond ".w", "nop.w") +#define WFE (cond) ALT_SMP( \ + "it " cond "\n\t" \ + "wfe" cond ".n", \ + \ + "nop.w" \ +)
In addition to a comment inline for this as others have suggested already, I'd write it differently to save some realestate:
#define WFE(cond) ALT_SMP("it " cond " ; wfe" cond ".n", "nop.w")
Nicolas
PATCH 1/1 ARM: Correct WFE() in asm/spinlock.h for Thumb-2
Quoted 33 lines · Nicolas Pitre
Quoted 20 lines · Dave Martin
The content for
ALT_SMP()in the definition ofWFE()expands to 6 bytes (IT cc ; WFEcc.W), which breaks the assumptions of the fixup code, leading to lockups when the affected code gets run.This patch works around the problem by explicitly using an IT + WFEcc.N pair.
Signed-off-by Dave Martin <[email protected]>---
?arch/arm/include/asm/spinlock.h | ? ?7 ++++++-
?1 files changed, 6 insertions(+), 1 deletions(-)arch/arm/include/asm/spinlock.h +0
-0@@ -18,7 +18,12 @@?#ifdef
CONFIG_THUMB2_KERNEL
?#define SEV ? ? ? ? ?ALT_SMP("sev.w", "nop.w")arch/arm/include/asm/spinlock.h
Quoted 7 lines
arch/arm/include/asm/spinlock.h +6
-1-#define WFE (cond) ? ?ALT_SMP("wfe" cond ".w", "nop.w") +#define WFE (cond) ? ?ALT_SMP( ? ? ? ? ? ? ? ?\ + ? ? "it " cond "\n\t" ? ? ? ? ? ? ? ? ? ? ? \ + ? ? "wfe" cond ".n", ? ? ? ? ? ? ? ? ? ? ? ?\ + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \ + ? ? "nop.w" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \ +)In addition to a comment inline for this as others have suggested already, I'd write it differently to save some realestate:
Quoted 1 line
#define WFE(cond) ? ? ?
ALT_SMP("it " cond " ; wfe" cond ".n", "nop.w")
Because of the subtlety of this, I wanted it to be presented in as clear a way as possible.
Russell, the patch is in your patch system--- if you prefer the shortened form, fell free to reject the existing patch and I'll repost.
Cheers
---Dave