Fix issues with ARMv6+v6k+v7 kernels

Add additional instructions to our assembly bitops functions to ensure that they only operate on word-aligned pointers. This will be necessary when we switch these operations to use the word-based exclusive operations.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/lib/bitops.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

arch/arm/lib/bitops.h +8 -0

index d422529..910d599 100644
@@ -1,6 +1,8 @@
 
 #if __LINUX_ARM_ARCH__ >= 6 && defined(CONFIG_CPU_32v6K)
 	.macro	bitop, instr
+	tst	r1, #3
+	strne	r1, [r1, -r1]		@ assert word-aligned
 	mov	r2, #1
 	and	r3, r0, #7		@ Get bit offset
 	add	r1, r1, r0, lsr #3	@ Get byte offset

@@ -14,6 +16,8 @@
 	.endm
 
 	.macro	testop, instr, store
+	tst	r1, #3
+	strne	r1, [r1, -r1]		@ assert word-aligned
 	and	r3, r0, #7		@ Get bit offset
 	mov	r2, #1
 	add	r1, r1, r0, lsr #3	@ Get byte offset

@@ -32,6 +36,8 @@
 	.endm
 #else
 	.macro	bitop, instr
+	tst	r1, #3
+	strne	r1, [r1, -r1]		@ assert word-aligned
 	and	r2, r0, #7
 	mov	r3, #1
 	mov	r3, r3, lsl r2

@@ -52,6 +58,8 @@
  * to avoid dirtying the data cache.
  */
 	.macro	testop, instr, store
+	tst	r1, #3
+	strne	r1, [r1, -r1]		@ assert word-aligned
 	add	r1, r1, r0, lsr #3
 	and	r3, r0, #7
 	mov	r0, #1

Signature · 7 lines

1.6.2.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

I also wonder what happens with a misaligned ldrex/strex... Does the alignment trap get invoked? If so, the assertion could be put there instead if that's not done already, removing this overhead from bitops calls. In the pre-ARMv6 case the alignment trap fixup would be applied as usual since the ldr/str are not special, but the previous code allowed for misaligned pointer so result would be no worse than before in that case. Testing appears to indicate those misaligned bitops are rather nonexistent so far.

Quoted 8 lines · Nicolas Pitre

I also wonder what happens with a misaligned ldrex/strex... Does the alignment trap get invoked? If so, the assertion could be put there instead if that's not done already, removing this overhead from bitops calls. In the pre-ARMv6 case the alignment trap fixup would be applied as usual since the ldr/str are not special, but the previous code allowed for misaligned pointer so result would be no worse than before in that case. Testing appears to indicate those misaligned bitops are rather nonexistent so far.

Well, whether LDREX/STREX always fault for misalignment is well open to debate. The ARM ARM for ARMv7 seems to imply they will always fault for misaligned accesses. For ARMv6, the situation is less clear - the way the manual is arranged, it suggests that they will if SCTLR.U is set. When it's clear, the ARM ARM doesn't specify what the outcome is.

I also don't know how our unaligned fixup will behave with an unaligned LDREX/STREX without checking the bit patterns.

So... I think that's too many unknowns to rely on it, especially when filesystem data is at stake.

Maybe changing it to:

	ands	ip, r1, #3
	strneb	r1, [ip]

instead?

Quoted 6 lines · Russell King - ARM Linux

Add additional instructions to our assembly bitops functions to ensure that they only operate on word-aligned pointers. ?This will be necessary when we switch these operations to use the word-based exclusive operations.

Signed-off-by Russell King <[email protected]>

This breaks the Thumb2 kernel build:

?AS ? ? ?arch/arm/lib/changebit.o
arch/arm/lib/changebit.S: Assembler messages:
arch/arm/lib/changebit.S:16: Error: Thumb does not support negative register indexing -- `strne r1,[r1,-r1]'

I also wonder what happens with a misaligned ldrex/strex... Does the alignment trap get invoked?

Add additional instructions to our assembly bitops functions to ensure that they only operate on word-aligned pointers. This will be necessary when we switch these operations to use the word-based exclusive operations.

Switch the set/clear/change bitops to use the word-based exclusive operations, which are only present in a wider range of ARM architectures than the byte-based exclusive operations.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/include/asm/bitops.h |   60 +++++++++++++++--------------------------
 arch/arm/kernel/armksyms.c    |   18 ++++--------
 arch/arm/lib/bitops.h         |   38 +++++++++++++------------
 arch/arm/lib/changebit.S      |   10 +-----
 arch/arm/lib/clearbit.S       |   11 +------
 arch/arm/lib/setbit.S         |   11 +------
 arch/arm/lib/testchangebit.S  |    9 ++----
 arch/arm/lib/testclearbit.S   |    9 ++----
 arch/arm/lib/testsetbit.S     |    9 ++----
 9 files changed, 63 insertions(+), 112 deletions(-)

arch/arm/include/asm/bitops.h +22 -38

index 7b1bb2b..af54ed1 100644
@@ -149,14 +149,18 @@ ____atomic_test_and_change_bit(unsigned int bit, volatile unsigned long *p)
  */
 
 /*
+ * Native endian assembly bitops.  nr = 0 -> word 0 bit 0.
+ */
+extern void _set_bit(int nr, volatile unsigned long * p);
+extern void _clear_bit(int nr, volatile unsigned long * p);
+extern void _change_bit(int nr, volatile unsigned long * p);
+extern int _test_and_set_bit(int nr, volatile unsigned long * p);
+extern int _test_and_clear_bit(int nr, volatile unsigned long * p);
+extern int _test_and_change_bit(int nr, volatile unsigned long * p);
+
+/*
  * Little endian assembly bitops.  nr = 0 -> byte 0 bit 0.
  */
-extern void _set_bit_le(int nr, volatile unsigned long * p);
-extern void _clear_bit_le(int nr, volatile unsigned long * p);
-extern void _change_bit_le(int nr, volatile unsigned long * p);
-extern int _test_and_set_bit_le(int nr, volatile unsigned long * p);
-extern int _test_and_clear_bit_le(int nr, volatile unsigned long * p);
-extern int _test_and_change_bit_le(int nr, volatile unsigned long * p);
 extern int _find_first_zero_bit_le(const void * p, unsigned size);
 extern int _find_next_zero_bit_le(const void * p, int size, int offset);
 extern int _find_first_bit_le(const unsigned long *p, unsigned size);

@@ -165,12 +169,6 @@ extern int _find_next_bit_le(const unsigned long *p, int size, int offset);
 /*
  * Big endian assembly bitops.  nr = 0 -> byte 3 bit 0.
  */
-extern void _set_bit_be(int nr, volatile unsigned long * p);
-extern void _clear_bit_be(int nr, volatile unsigned long * p);
-extern void _change_bit_be(int nr, volatile unsigned long * p);
-extern int _test_and_set_bit_be(int nr, volatile unsigned long * p);
-extern int _test_and_clear_bit_be(int nr, volatile unsigned long * p);
-extern int _test_and_change_bit_be(int nr, volatile unsigned long * p);
 extern int _find_first_zero_bit_be(const void * p, unsigned size);
 extern int _find_next_zero_bit_be(const void * p, int size, int offset);
 extern int _find_first_bit_be(const unsigned long *p, unsigned size);

@@ -180,33 +178,26 @@ extern int _find_next_bit_be(const unsigned long *p, int size, int offset);
 /*
  * The __* form of bitops are non-atomic and may be reordered.
  */
-#define	ATOMIC_BITOP_LE(name,nr,p)		\
-	(__builtin_constant_p(nr) ?		\
-	 ____atomic_##name(nr, p) :		\
-	 _##name##_le(nr,p))
-
-#define	ATOMIC_BITOP_BE(name,nr,p)		\
-	(__builtin_constant_p(nr) ?		\
-	 ____atomic_##name(nr, p) :		\
-	 _##name##_be(nr,p))
+#define ATOMIC_BITOP(name,nr,p)			\
+	(__builtin_constant_p(nr) ? ____atomic_##name(nr, p) : _##name(nr,p))
 #else
-#define ATOMIC_BITOP_LE(name,nr,p)	_##name##_le(nr,p)
-#define ATOMIC_BITOP_BE(name,nr,p)	_##name##_be(nr,p)
+#define ATOMIC_BITOP(name,nr,p)		_##name(nr,p)
 #endif
 
-#define NONATOMIC_BITOP(name,nr,p)		\
-	(____nonatomic_##name(nr, p))
+/*
+ * Native endian atomic definitions.
+ */
+#define set_bit(nr,p)			ATOMIC_BITOP(set_bit,nr,p)
+#define clear_bit(nr,p)			ATOMIC_BITOP(clear_bit,nr,p)
+#define change_bit(nr,p)		ATOMIC_BITOP(change_bit,nr,p)
+#define test_and_set_bit(nr,p)		ATOMIC_BITOP(test_and_set_bit,nr,p)
+#define test_and_clear_bit(nr,p)	ATOMIC_BITOP(test_and_clear_bit,nr,p)
+#define test_and_change_bit(nr,p)	ATOMIC_BITOP(test_and_change_bit,nr,p)
 
 #ifndef __ARMEB__
 /*
  * These are the little endian, atomic definitions.
  */
-#define set_bit(nr,p)			ATOMIC_BITOP_LE(set_bit,nr,p)
-#define clear_bit(nr,p)			ATOMIC_BITOP_LE(clear_bit,nr,p)
-#define change_bit(nr,p)		ATOMIC_BITOP_LE(change_bit,nr,p)
-#define test_and_set_bit(nr,p)		ATOMIC_BITOP_LE(test_and_set_bit,nr,p)
-#define test_and_clear_bit(nr,p)	ATOMIC_BITOP_LE(test_and_clear_bit,nr,p)
-#define test_and_change_bit(nr,p)	ATOMIC_BITOP_LE(test_and_change_bit,nr,p)
 #define find_first_zero_bit(p,sz)	_find_first_zero_bit_le(p,sz)
 #define find_next_zero_bit(p,sz,off)	_find_next_zero_bit_le(p,sz,off)
 #define find_first_bit(p,sz)		_find_first_bit_le(p,sz)

@@ -215,16 +206,9 @@ extern int _find_next_bit_be(const unsigned long *p, int size, int offset);
 #define WORD_BITOFF_TO_LE(x)		((x))
 
 #else
-
 /*
  * These are the big endian, atomic definitions.
  */
-#define set_bit(nr,p)			ATOMIC_BITOP_BE(set_bit,nr,p)
-#define clear_bit(nr,p)			ATOMIC_BITOP_BE(clear_bit,nr,p)
-#define change_bit(nr,p)		ATOMIC_BITOP_BE(change_bit,nr,p)
-#define test_and_set_bit(nr,p)		ATOMIC_BITOP_BE(test_and_set_bit,nr,p)
-#define test_and_clear_bit(nr,p)	ATOMIC_BITOP_BE(test_and_clear_bit,nr,p)
-#define test_and_change_bit(nr,p)	ATOMIC_BITOP_BE(test_and_change_bit,nr,p)
 #define find_first_zero_bit(p,sz)	_find_first_zero_bit_be(p,sz)
 #define find_next_zero_bit(p,sz,off)	_find_next_zero_bit_be(p,sz,off)
 #define find_first_bit(p,sz)		_find_first_bit_be(p,sz)

arch/arm/kernel/armksyms.c +6 -12

index e5e1e53..d5d4185 100644
@@ -140,24 +140,18 @@ EXPORT_SYMBOL(__aeabi_ulcmp);
 #endif
 
 	/* bitops */
-EXPORT_SYMBOL(_set_bit_le);
-EXPORT_SYMBOL(_test_and_set_bit_le);
-EXPORT_SYMBOL(_clear_bit_le);
-EXPORT_SYMBOL(_test_and_clear_bit_le);
-EXPORT_SYMBOL(_change_bit_le);
-EXPORT_SYMBOL(_test_and_change_bit_le);
+EXPORT_SYMBOL(_set_bit);
+EXPORT_SYMBOL(_test_and_set_bit);
+EXPORT_SYMBOL(_clear_bit);
+EXPORT_SYMBOL(_test_and_clear_bit);
+EXPORT_SYMBOL(_change_bit);
+EXPORT_SYMBOL(_test_and_change_bit);
 EXPORT_SYMBOL(_find_first_zero_bit_le);
 EXPORT_SYMBOL(_find_next_zero_bit_le);
 EXPORT_SYMBOL(_find_first_bit_le);
 EXPORT_SYMBOL(_find_next_bit_le);
 
 #ifdef __ARMEB__
-EXPORT_SYMBOL(_set_bit_be);
-EXPORT_SYMBOL(_test_and_set_bit_be);
-EXPORT_SYMBOL(_clear_bit_be);
-EXPORT_SYMBOL(_test_and_clear_bit_be);
-EXPORT_SYMBOL(_change_bit_be);
-EXPORT_SYMBOL(_test_and_change_bit_be);
 EXPORT_SYMBOL(_find_first_zero_bit_be);
 EXPORT_SYMBOL(_find_next_zero_bit_be);
 EXPORT_SYMBOL(_find_first_bit_be);

arch/arm/lib/bitops.h +20 -18

index 910d599..f8a2bd3 100644
@@ -1,15 +1,15 @@
-
-#if __LINUX_ARM_ARCH__ >= 6 && defined(CONFIG_CPU_32v6K)
+#if __LINUX_ARM_ARCH__ >= 6
 	.macro	bitop, instr
 	tst	r1, #3
 	strne	r1, [r1, -r1]		@ assert word-aligned
 	mov	r2, #1
-	and	r3, r0, #7		@ Get bit offset
-	add	r1, r1, r0, lsr #3	@ Get byte offset
+	and	r3, r0, #31		@ Get bit offset
+	mov	r0, r0, lsr #5
+	add	r1, r1, r0, lsl #2	@ Get word offset
 	mov	r3, r2, lsl r3
-1:	ldrexb	r2, [r1]
+1:	ldrex	r2, [r1]
 	\instr	r2, r2, r3
-	strexb	r0, r2, [r1]
+	strex	r0, r2, [r1]
 	cmp	r0, #0
 	bne	1b
 	mov	pc, lr

@@ -18,15 +18,16 @@
 	.macro	testop, instr, store
 	tst	r1, #3
 	strne	r1, [r1, -r1]		@ assert word-aligned
-	and	r3, r0, #7		@ Get bit offset
 	mov	r2, #1
-	add	r1, r1, r0, lsr #3	@ Get byte offset
+	and	r3, r0, #31		@ Get bit offset
+	mov	r0, r0, lsr #5
+	add	r1, r1, r0, lsl #2	@ Get word offset
 	mov	r3, r2, lsl r3		@ create mask
 	smp_dmb
-1:	ldrexb	r2, [r1]
+1:	ldrex	r2, [r1]
 	ands	r0, r2, r3		@ save old value of bit
-	\instr	r2, r2, r3			@ toggle bit
-	strexb	ip, r2, [r1]
+	\instr	r2, r2, r3		@ toggle bit
+	strex	ip, r2, [r1]
 	cmp	ip, #0
 	bne	1b
 	smp_dmb

@@ -38,13 +39,14 @@
 	.macro	bitop, instr
 	tst	r1, #3
 	strne	r1, [r1, -r1]		@ assert word-aligned
-	and	r2, r0, #7
+	and	r2, r0, #31
+	mov	r0, r0, lsr #5
 	mov	r3, #1
 	mov	r3, r3, lsl r2
 	save_and_disable_irqs ip
-	ldrb	r2, [r1, r0, lsr #3]
+	ldr	r2, [r1, r0, lsl #2]
 	\instr	r2, r2, r3
-	strb	r2, [r1, r0, lsr #3]
+	str	r2, [r1, r0, lsl #2]
 	restore_irqs ip
 	mov	pc, lr
 	.endm

@@ -60,11 +62,11 @@
 	.macro	testop, instr, store
 	tst	r1, #3
 	strne	r1, [r1, -r1]		@ assert word-aligned
-	add	r1, r1, r0, lsr #3
-	and	r3, r0, #7
-	mov	r0, #1
+	and	r3, r0, #31
+	mov	r0, r0, lsr #5
 	save_and_disable_irqs ip
-	ldrb	r2, [r1]
+	ldr	r2, [r1, r0, lsl #2]!
+	mov	r0, #1
 	tst	r2, r0, lsl r3
 	\instr	r2, r2, r0, lsl r3
 	\store	r2, [r1]

arch/arm/lib/changebit.S +2 -8

index 80f3115..68ed5b6 100644
@@ -12,12 +12,6 @@
 #include "bitops.h"
                 .text
 
-/* Purpose  : Function to change a bit
- * Prototype: int change_bit(int bit, void *addr)
- */
-ENTRY(_change_bit_be)
-		eor	r0, r0, #0x18		@ big endian byte ordering
-ENTRY(_change_bit_le)
+ENTRY(_change_bit)
 	bitop	eor
-ENDPROC(_change_bit_be)
-ENDPROC(_change_bit_le)
+ENDPROC(_change_bit)

arch/arm/lib/clearbit.S +2 -9

index 1a63e43..4c04c3b 100644
@@ -12,13 +12,6 @@
 #include "bitops.h"
                 .text
 
-/*
- * Purpose  : Function to clear a bit
- * Prototype: int clear_bit(int bit, void *addr)
- */
-ENTRY(_clear_bit_be)
-		eor	r0, r0, #0x18		@ big endian byte ordering
-ENTRY(_clear_bit_le)
+ENTRY(_clear_bit)
 	bitop	bic
-ENDPROC(_clear_bit_be)
-ENDPROC(_clear_bit_le)
+ENDPROC(_clear_bit)

arch/arm/lib/setbit.S +2 -9

index 1dd7176..bbee5c6 100644
@@ -12,13 +12,6 @@
 #include "bitops.h"
 		.text
 
-/*
- * Purpose  : Function to set a bit
- * Prototype: int set_bit(int bit, void *addr)
- */
-ENTRY(_set_bit_be)
-		eor	r0, r0, #0x18		@ big endian byte ordering
-ENTRY(_set_bit_le)
+ENTRY(_set_bit)
 	bitop	orr
-ENDPROC(_set_bit_be)
-ENDPROC(_set_bit_le)
+ENDPROC(_set_bit)

arch/arm/lib/testchangebit.S +3 -6

index 5c98dc5..15a4d43 100644
@@ -12,9 +12,6 @@
 #include "bitops.h"
                 .text
 
-ENTRY(_test_and_change_bit_be)
-		eor	r0, r0, #0x18		@ big endian byte ordering
-ENTRY(_test_and_change_bit_le)
-	testop	eor, strb
-ENDPROC(_test_and_change_bit_be)
-ENDPROC(_test_and_change_bit_le)
+ENTRY(_test_and_change_bit)
+	testop	eor, str
+ENDPROC(_test_and_change_bit)

arch/arm/lib/testclearbit.S +3 -6

index 543d709..521b66b 100644
@@ -12,9 +12,6 @@
 #include "bitops.h"
                 .text
 
-ENTRY(_test_and_clear_bit_be)
-		eor	r0, r0, #0x18		@ big endian byte ordering
-ENTRY(_test_and_clear_bit_le)
-	testop	bicne, strneb
-ENDPROC(_test_and_clear_bit_be)
-ENDPROC(_test_and_clear_bit_le)
+ENTRY(_test_and_clear_bit)
+	testop	bicne, strne
+ENDPROC(_test_and_clear_bit)

arch/arm/lib/testsetbit.S +3 -6

index 0b3f390..1c98cc2 100644
@@ -12,9 +12,6 @@
 #include "bitops.h"
                 .text
 
-ENTRY(_test_and_set_bit_be)
-		eor	r0, r0, #0x18		@ big endian byte ordering
-ENTRY(_test_and_set_bit_le)
-	testop	orreq, streqb
-ENDPROC(_test_and_set_bit_be)
-ENDPROC(_test_and_set_bit_le)
+ENTRY(_test_and_set_bit)
+	testop	orreq, streq
+ENDPROC(_test_and_set_bit)

Signature · 7 lines

1.6.2.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Switch the set/clear/change bitops to use the word-based exclusive operations, which are only present in a wider range of ARM architectures than the byte-based exclusive operations.

Signed-off-by Russell King <[email protected]>

Quoted 5 lines · Russell King - ARM Linux

Switch the set/clear/change bitops to use the word-based exclusive operations, which are only present in a wider range of ARM architectures than the byte-based exclusive operations.

Signed-off-by Russell King <[email protected]>

As only one person replied to this patch with test information, I'm not happy to push this into -rc, and hence that prevents it going into -stable too.

That means omap2plus_defconfig .38 mainline kernels (including -stable) will remain potentially dangerous when run on SMP capable hardware.

At least another person did post results:

http://mid.gmane.org/20110117094602.GA2622 at pulham.picochip.com
http://mid.gmane.org/20110117110308.GC2622 at pulham.picochip.com

Quoted 3 lines

That means omap2plus_defconfig .38 mainline kernels (including -stable) will remain potentially dangerous when run on SMP capable hardware.

I must admit that this series looks a bit large for stable IMHO. I think that the fix for stable should limit itself only to prevent SMP from being selected if anything else than CPU_32v6K is selected.

Quoted 4 lines · Nicolas Pitre

At least another person did post results:

http://mid.gmane.org/20110117094602.GA2622 at pulham.picochip.com
http://mid.gmane.org/20110117110308.GC2622 at pulham.picochip.com

Slightly different patch - there were three revisions. ?I can't attach a tested-by given to a different patch to this one.

Quoted 4 lines

Quoted 3 lines

That means omap2plus_defconfig .38 mainline kernels (including -stable) will remain potentially dangerous when run on SMP capable hardware.

Quoted 3 lines

I must admit that this series looks a bit large for stable IMHO. ?I think that the fix for stable should limit itself only to prevent SMP from being selected if anything else than CPU_32v6K is selected.

The first three are the bare minimum required for -stable.

Quoted 13 lines · Russell King - ARM Linux

Quoted 4 lines · Nicolas Pitre

At least another person did post results:

http://mid.gmane.org/20110117094602.GA2622 at pulham.picochip.com
http://mid.gmane.org/20110117110308.GC2622 at pulham.picochip.com

Slightly different patch - there were three revisions. ?I can't attach a tested-by given to a different patch to this one.

Quoted 4 lines

Quoted 3 lines

That means omap2plus_defconfig .38 mainline kernels (including -stable) will remain potentially dangerous when run on SMP capable hardware.

Quoted 6 lines

Quoted 3 lines

I must admit that this series looks a bit large for stable IMHO. ?I think that the fix for stable should limit itself only to prevent SMP from being selected if anything else than CPU_32v6K is selected.

The first three are the bare minimum required for -stable.

Quoted 15 lines · Sourav

Quoted 13 lines · Russell King - ARM Linux

Quoted 4 lines · Nicolas Pitre

At least another person did post results:

http://mid.gmane.org/20110117094602.GA2622 at pulham.picochip.com
http://mid.gmane.org/20110117110308.GC2622 at pulham.picochip.com

Slightly different patch - there were three revisions. ?I can't attach a tested-by given to a different patch to this one.

Quoted 4 lines

Quoted 3 lines

That means omap2plus_defconfig .38 mainline kernels (including -stable) will remain potentially dangerous when run on SMP capable hardware.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

I must admit that this series looks a bit large for stable IMHO. ?I think that the fix for stable should limit itself only to prevent SMP from being selected if anything else than CPU_32v6K is selected.

The first three are the bare minimum required for -stable.

Boot tested the 14 patch series ?with CONFIG_SWP_EMULATE enabled, on the following boards :

1. Omap2420 SDP

2. Omap2430 SDP

3. Omap3430 SDP

4. Omap4 Blaze

Tested-by Sourav Poddar <[email protected]>

Quoted 32 lines · Sourav

Quoted 15 lines · Sourav

Quoted 13 lines · Russell King - ARM Linux

Quoted 4 lines · Nicolas Pitre

At least another person did post results:

http://mid.gmane.org/20110117094602.GA2622 at pulham.picochip.com
http://mid.gmane.org/20110117110308.GC2622 at pulham.picochip.com

Slightly different patch - there were three revisions. ?I can't attach a tested-by given to a different patch to this one.

Quoted 4 lines

Quoted 3 lines

That means omap2plus_defconfig .38 mainline kernels (including -stable) will remain potentially dangerous when run on SMP capable hardware.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

I must admit that this series looks a bit large for stable IMHO. ?I think that the fix for stable should limit itself only to prevent SMP from being selected if anything else than CPU_32v6K is selected.

The first three are the bare minimum required for -stable.

?Boot tested the 14 patch series ?with CONFIG_SWP_EMULATE enabled, on ?the following boards :

? 1. Omap2420 SDP

? 2. Omap2430 SDP

Quoted 5 lines

? 3. Omap3430 SDP

? 4. Omap4 Blaze

? Tested-by: Sourav Poddar <[email protected]>

Thanks. ?It's also important to ascertain which filesystems were tested - could you let me know please?

Quoted 37 lines · Russell King - ARM Linux

Quoted 36 lines · Sourav

Quoted 15 lines · Sourav

Quoted 13 lines · Russell King - ARM Linux

Quoted 4 lines · Nicolas Pitre

At least another person did post results:

http://mid.gmane.org/20110117094602.GA2622 at pulham.picochip.com
http://mid.gmane.org/20110117110308.GC2622 at pulham.picochip.com

Slightly different patch - there were three revisions. ?I can't attach a tested-by given to a different patch to this one.

Quoted 4 lines

Quoted 3 lines

That means omap2plus_defconfig .38 mainline kernels (including -stable) will remain potentially dangerous when run on SMP capable hardware.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

I must admit that this series looks a bit large for stable IMHO. ?I think that the fix for stable should limit itself only to prevent SMP from being selected if anything else than CPU_32v6K is selected.

The first three are the bare minimum required for -stable.

?Boot tested the 14 patch series ?with CONFIG_SWP_EMULATE enabled, on ?the following boards :

? 1. Omap2420 SDP

? 2. Omap2430 SDP

? 3. Omap3430 SDP

? 4. Omap4 Blaze

Quoted 5 lines

Quoted 1 line

? Tested-by: Sourav Poddar <[email protected]>

Thanks. ?It's also important to ascertain which filesystems were tested - could you let me know please?

It is a custom filesystem on BusyBox v1.17.2.

Please let me know if you need some other
information.

Quoted 41 lines · Sourav

Quoted 39 lines · Russell King - ARM Linux

Quoted 37 lines · Sourav

Quoted 15 lines · Sourav

Quoted 13 lines · Russell King - ARM Linux

Quoted 4 lines · Nicolas Pitre

At least another person did post results:

http://mid.gmane.org/20110117094602.GA2622 at pulham.picochip.com
http://mid.gmane.org/20110117110308.GC2622 at pulham.picochip.com

Slightly different patch - there were three revisions. ?I can't attach a tested-by given to a different patch to this one.

Quoted 4 lines

Quoted 3 lines

That means omap2plus_defconfig .38 mainline kernels (including -stable) will remain potentially dangerous when run on SMP capable hardware.

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

I must admit that this series looks a bit large for stable IMHO. ?I think that the fix for stable should limit itself only to prevent SMP from being selected if anything else than CPU_32v6K is selected.

The first three are the bare minimum required for -stable.

?Boot tested the 14 patch series ?with CONFIG_SWP_EMULATE enabled, on ?the following boards :

? 1. Omap2420 SDP

? 2. Omap2430 SDP

? 3. Omap3430 SDP

? 4. Omap4 Blaze

? Tested-by: Sourav Poddar <[email protected]>

Quoted 5 lines

Quoted 3 lines

Thanks. ?It's also important to ascertain which filesystems were tested - could you let me know please?

It is a custom filesystem on BusyBox v1.17.2.

So you wrote code under fs/ for this filesystem?

Quoted 2 lines

Please let me know if you need some other
information.

I'm trying to find out what _type_ of filesystem. ?ext2, ext3, nfs, cramfs, etc.

nfs filesystem.

Quoted 5 lines

Switch the set/clear/change bitops to use the word-based exclusive operations, which are only present in a wider range of ARM architectures than the byte-based exclusive operations.

Signed-off-by Russell King <[email protected]>

As only one person replied to this patch with test information, I'm not happy to push this into -rc, and hence that prevents it going into -stable too. That means omap2plus_defconfig .38 mainline kernels (including -stable) will remain potentially dangerous when run on SMP capable hardware.

Therefore, I see no alternative at the moment but to purposely cause builds which cover ARMv6, and ARMv6K or ARMv7 CPUs to save people from data corruption.

Please get your tested-by's to me for at least the first three patches of this series ASAP.

Hi Russell,

Hopefully this isn't too late to be useful.

Quoted 13 lines · Russell King - ARM Linux

Quoted 5 lines

Switch the set/clear/change bitops to use the word-based exclusive operations, which are only present in a wider range of ARM architectures than the byte-based exclusive operations.

Signed-off-by Russell King <[email protected]>

As only one person replied to this patch with test information, I'm not happy to push this into -rc, and hence that prevents it going into -stable too. That means omap2plus_defconfig .38 mainline kernels (including -stable) will remain potentially dangerous when run on SMP capable hardware.

Quoted 6 lines

Therefore, I see no alternative at the moment but to purposely cause builds which cover ARMv6, and ARMv6K or ARMv7 CPUs to save people from data corruption.

Please get your tested-by's to me for at least the first three patches of this series ASAP.

I've tested this on the Versatile Express (ca9x4, little endian) with a USB hard drive and ext3 (rw) filesystem. I did a bunch of parallel dds of a 2GB file containing random data whilst the system was fairly loaded. File checksums all passed and the kernel didn't do anything untowards.

Tested-by Will Deacon <[email protected]>

I appreciate this is the same test configuration as the one Nicolas did, but it at least replicates his findings.

Quoted 9 lines

I've tested this on the Versatile Express (ca9x4, little endian) with a USB hard drive and ext3 (rw) filesystem. I did a bunch of parallel dds of a 2GB file containing random data whilst the system was fairly loaded. File checksums all passed and the kernel didn't do anything untowards.

Tested-by Will Deacon <[email protected]>

I appreciate this is the same test configuration as the one Nicolas did, but it at least replicates his findings.

Was this for just the first three or the complete set?

Quoted 9 lines

I've tested this on the Versatile Express (ca9x4, little endian) with a USB hard drive and ext3 (rw) filesystem. I did a bunch of parallel dds of a 2GB file containing random data whilst the system was fairly loaded. File checksums all passed and the kernel didn't do anything untowards.

Tested-by Will Deacon <[email protected]>

I appreciate this is the same test configuration as the one Nicolas did, but it at least replicates his findings.

Was this for just the first three or the complete set?

Quoted 5 lines

Switch the set/clear/change bitops to use the word-based exclusive operations, which are only present in a wider range of ARM architectures than the byte-based exclusive operations.

Signed-off-by Russell King <[email protected]>

As only one person replied to this patch with test information, I'm not happy to push this into -rc, and hence that prevents it going into -stable too. That means omap2plus_defconfig .38 mainline kernels (including -stable) will remain potentially dangerous when run on SMP capable hardware.

Therefore, I see no alternative at the moment but to purposely cause builds which cover ARMv6, and ARMv6K or ARMv7 CPUs to save people from data corruption.

Please get your tested-by's to me for at least the first three patches of this series ASAP.

Hi Russell,

Hopefully this isn't too late to be useful.

Quoted 13 lines · Russell King - ARM Linux

Quoted 5 lines

Switch the set/clear/change bitops to use the word-based exclusive operations, which are only present in a wider range of ARM architectures than the byte-based exclusive operations.

Signed-off-by Russell King <[email protected]>

As only one person replied to this patch with test information, I'm not happy to push this into -rc, and hence that prevents it going into -stable too. That means omap2plus_defconfig .38 mainline kernels (including -stable) will remain potentially dangerous when run on SMP capable hardware.

Quoted 6 lines

Therefore, I see no alternative at the moment but to purposely cause builds which cover ARMv6, and ARMv6K or ARMv7 CPUs to save people from data corruption.

Please get your tested-by's to me for at least the first three patches of this series ASAP.

I've tested this on the Versatile Express (ca9x4, little endian) with a USB hard drive and ext3 (rw) filesystem. I did a bunch of parallel dds of a 2GB file containing random data whilst the system was fairly loaded. File checksums all passed and the kernel didn't do anything untowards.

Tested-by Will Deacon <[email protected]>

I appreciate this is the same test configuration as the one Nicolas did, but it at least replicates his findings.

Switch the set/clear/change bitops to use the word-based exclusive operations, which are only present in a wider range of ARM architectures than the byte-based exclusive operations.

SMP requires at least the ARMv6K extensions to be present, so if we're running on SMP, the WFE and SEV instructions must be available.

However, when we run on UP, the v6K extensions may not be available, and so we don't want WFE/SEV to be in the instruction stream. Use the SMP alternatives infrastructure to replace these instructions with NOPs if we build for SMP but run on UP.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/include/asm/spinlock.h |   37 +++++++++++++++++++++++++------------
 1 files changed, 25 insertions(+), 12 deletions(-)

arch/arm/include/asm/spinlock.h +25 -12

index 17eb355..da1af52 100644
@@ -5,17 +5,36 @@
 #error SMP not supported on pre-ARMv6 CPUs
 #endif
 
+/*
+ * sev and wfe are ARMv6K extensions.  Uniprocessor ARMv6 may not have the K
+ * extensions, so when running on UP, we have to patch these instructions away.
+ */
+#define ALT_SMP(smp, up)					\
+	"9998:	" smp "\n"					\
+	"	.pushsection \".alt.smp.init\", \"a\"\n"	\
+	"	.long	9998b\n"				\
+	"	" up "\n"					\
+	"	.popsection\n"
+
+#ifdef CONFIG_THUMB2_KERNEL
+#define SEV		ALT_SMP("sev.w", "nop.w")
+#define WFE(cond)	ALT_SMP("wfe" cond ".w", "nop.w")
+#else
+#define SEV		ALT_SMP("sev", "nop")
+#define WFE(cond)	ALT_SMP("wfe" cond, "nop")
+#endif
+
 static inline void dsb_sev(void)
 {
 #if __LINUX_ARM_ARCH__ >= 7
 	__asm__ __volatile__ (
 		"dsb\n"
-		"sev"
+		SEV
 	);
-#elif defined(CONFIG_CPU_32v6K)
+#else
 	__asm__ __volatile__ (
 		"mcr p15, 0, %0, c7, c10, 4\n"
-		"sev"
+		SEV
 		: : "r" (0)
 	);
 #endif

@@ -46,9 +65,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%1]\n"
 "	teq	%0, #0\n"
-#ifdef CONFIG_CPU_32v6K
-"	wfene\n"
-#endif
+	WFE("ne")
 "	strexeq	%0, %2, [%1]\n"
 "	teqeq	%0, #0\n"
 "	bne	1b"

@@ -107,9 +124,7 @@ static inline void arch_write_lock(arch_rwlock_t *rw)
 	__asm__ __volatile__(
 "1:	ldrex	%0, [%1]\n"
 "	teq	%0, #0\n"
-#ifdef CONFIG_CPU_32v6K
-"	wfene\n"
-#endif
+	WFE("ne")
 "	strexeq	%0, %2, [%1]\n"
 "	teq	%0, #0\n"
 "	bne	1b"

@@ -176,9 +191,7 @@ static inline void arch_read_lock(arch_rwlock_t *rw)
 "1:	ldrex	%0, [%2]\n"
 "	adds	%0, %0, #1\n"
 "	strexpl	%1, %0, [%2]\n"
-#ifdef CONFIG_CPU_32v6K
-"	wfemi\n"
-#endif
+	WFE("mi")
 "	rsbpls	%0, %1, #0\n"
 "	bmi	1b"
 	: "=&r" (tmp), "=&r" (tmp2)

Signature · 6 lines

1.6.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

SMP requires at least the ARMv6K extensions to be present, so if we're running on SMP, the WFE and SEV instructions must be available.

However, when we run on UP, the v6K extensions may not be available, and so we don't want WFE/SEV to be in the instruction stream. ?Use the SMP alternatives infrastructure to replace these instructions with NOPs if we build for SMP but run on UP.

Signed-off-by Russell King <[email protected]>

---
?arch/arm/include/asm/spinlock.h | ? 37 +++++++++++++++++++++++++------------ ?1 files changed, 25 insertions(+), 12 deletions(-)

arch/arm/include/asm/spinlock.h +0 -0

index 17eb355..da1af52 100644
@@ -5,17 +5,36 @@

?#error SMP not supported on pre-ARMv6 CPUs
?#endif

arch/arm/include/asm/spinlock.h +19 -0

+/*
+ * sev and wfe are ARMv6K extensions. ?Uniprocessor ARMv6 may not have the K
+ * extensions, so when running on UP, we have to patch these instructions away.
+ */
+#define ALT_SMP(smp, up) ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
+ ? ? ? "9998: ?" smp "\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
+ ? ? ? " ? ? ? .pushsection \".alt.smp.init\", \"a\"\n" ? ? ? ?\
+ ? ? ? " ? ? ? .long ? 9998b\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?\
+ ? ? ? " ? ? ? " up "\n" ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? \
+ ? ? ? " ? ? ? .popsection\n"
+
+#ifdef CONFIG_THUMB2_KERNEL
+#define SEV ? ? ? ? ? ?ALT_SMP("sev.w", "nop.w")
+#define WFE(cond) ? ? ?ALT_SMP("wfe" cond ".w", "nop.w")
+#else
+#define SEV ? ? ? ? ? ?ALT_SMP("sev", "nop")
+#define WFE(cond) ? ? ?ALT_SMP("wfe" cond, "nop")
+#endif
+

?static inline void dsb_sev(void)
?{
?#if __LINUX_ARM_ARCH__ >= 7
? ? ? ?__asm__ __volatile__ (
? ? ? ? ? ? ? ?"dsb\n"
- ? ? ? ? ? ? ? "sev"
+ ? ? ? ? ? ? ? SEV
? ? ? ?);

arch/arm/include/asm/spinlock.h +1 -1

-#elif defined(CONFIG_CPU_32v6K)
+#else

? ? ? ?__asm__ __volatile__ (
? ? ? ? ? ? ? ?"mcr p15, 0, %0, c7, c10, 4\n"
- ? ? ? ? ? ? ? "sev"
+ ? ? ? ? ? ? ? SEV
? ? ? ? ? ? ? ?: : "r" (0)
? ? ? ?);
?#endif

arch/arm/include/asm/spinlock.h +0 -0

@@ -46,9 +65,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)

? ? ? ?__asm__ __volatile__(
?"1: ? ?ldrex ? %0, [%1]\n"
?" ? ? ?teq ? ? %0, #0\n"

arch/arm/include/asm/spinlock.h +1 -3

-#ifdef CONFIG_CPU_32v6K
-" ? ? ?wfene\n"
-#endif
+ ? ? ? WFE("ne")

?" ? ? ?strexeq %0, %2, [%1]\n"
?" ? ? ?teqeq ? %0, #0\n"
?" ? ? ?bne ? ? 1b"

arch/arm/include/asm/spinlock.h +0 -0

@@ -107,9 +124,7 @@ static inline void arch_write_lock(arch_rwlock_t *rw)

? ? ? ?__asm__ __volatile__(
?"1: ? ?ldrex ? %0, [%1]\n"
?" ? ? ?teq ? ? %0, #0\n"

arch/arm/include/asm/spinlock.h +1 -3

-#ifdef CONFIG_CPU_32v6K
-" ? ? ?wfene\n"
-#endif
+ ? ? ? WFE("ne")

?" ? ? ?strexeq %0, %2, [%1]\n"
?" ? ? ?teq ? ? %0, #0\n"
?" ? ? ?bne ? ? 1b"

arch/arm/include/asm/spinlock.h +0 -0

@@ -176,9 +191,7 @@ static inline void arch_read_lock(arch_rwlock_t *rw)

?"1: ? ?ldrex ? %0, [%2]\n"
?" ? ? ?adds ? ?%0, %0, #1\n"
?" ? ? ?strexpl %1, %0, [%2]\n"

arch/arm/include/asm/spinlock.h +1 -3

-#ifdef CONFIG_CPU_32v6K
-" ? ? ?wfemi\n"
-#endif
+ ? ? ? WFE("mi")

?" ? ? ?rsbpls ?%0, %1, #0\n"
?" ? ? ?bmi ? ? 1b"
? ? ? ?: "=&r" (tmp), "=&r" (tmp2)
--
1.6.2.5

A couple of questions on this:

1) I notice these spinlock functions are generally marked inline.

Is that likely to happen in modules? If so, there would be a need to do SMP_ON_UP fixups at module load time -- I don't think that's currently implemented.

2) When building with this patch and CONFIG_SMP_ON_UP=y, I've seen vmlinux link errors like this:

LD .tmp_vmlinux1
.exit.text' referenced in section .alt.smp.init' of drivers/built-in.o: defined in discarded section `.exit.text' of drivers/built-in.o
make: *** [.tmp_vmlinux1] Error 1

I don't know whether this is caused by the patch directly or as a side-effect -- I've only noticed it in the linaro-2.6.37 tree so far. git bisect indentified this above patch as the first one with the error in that case.

I don't understand the section discarding logic too well, so I'm not sure how to fix it for now...

Quoted 7 lines · Dave Martin

A couple of questions on this:

1) I notice these spinlock functions are generally marked inline.

Is that likely to happen in modules? ?If so, there would be a need to do SMP_ON_UP fixups at module load time -- I don't think that's currently implemented.

No one should be using the arch_* spinlocks directly. ?The spinlocks are implemented in out of line code in kernel/spinlock.c

Quoted 15 lines

2) When building with this patch and CONFIG_SMP_ON_UP=y, I've seen vmlinux link errors like this:

? LD ? ? ?.tmp_vmlinux1
.exit.text' referenced in section .alt.smp.init' of drivers/built-in.o: defined in discarded section `.exit.text' of drivers/built-in.o
make: *** [.tmp_vmlinux1] Error 1

I don't know whether this is caused by the patch directly or as a side-effect -- I've only noticed it in the linaro-2.6.37 tree so far. git bisect indentified this above patch as the first one with the error in that case.

Quoted 2 lines

I don't understand the section discarding logic too well, so I'm not sure how to fix it for now...

Hmm. ?I don't see how that could happen, unless some driver is going behind the spinlock APIs, or using our dsb_sev() directly.

I think you need to first track down what's responsible for inserting architecture spinlock code into drivers.

Quoted 9 lines · Russell King - ARM Linux

Quoted 7 lines · Dave Martin

A couple of questions on this:

1) I notice these spinlock functions are generally marked inline.

Is that likely to happen in modules? ?If so, there would be a need to do SMP_ON_UP fixups at module load time -- I don't think that's currently implemented.

Quoted 2 lines

No one should be using the arch_* spinlocks directly. ?The spinlocks are implemented in out of line code in kernel/spinlock.c

OK--- do think this is something we need a sanity-check for, or does this fall into to a category of bad driver implementation which will get thrown out during peer review?

Quoted 11 lines · Dave Martin

Quoted 9 lines · Russell King - ARM Linux

Quoted 7 lines · Dave Martin

A couple of questions on this:

1) I notice these spinlock functions are generally marked inline.

Is that likely to happen in modules? ?If so, there would be a need to do SMP_ON_UP fixups at module load time -- I don't think that's currently implemented.

Quoted 6 lines

Quoted 2 lines

No one should be using the arch_* spinlocks directly. ?The spinlocks are implemented in out of line code in kernel/spinlock.c

OK--- do think this is something we need a sanity-check for, or does this fall into to a category of bad driver implementation which will get thrown out during peer review?

Hmm, actually it looks like you can end up with configurations where the spinlocks are inlined.

That means we'll have to get rid of the link-time discarding of the .exit sections, and discard them along with the .init sections.

Quoted 22 lines · Russell King - ARM Linux

Quoted 11 lines · Dave Martin

Quoted 9 lines · Russell King - ARM Linux

Quoted 7 lines · Dave Martin

A couple of questions on this:

1) I notice these spinlock functions are generally marked inline.

Is that likely to happen in modules? ?If so, there would be a need to do SMP_ON_UP fixups at module load time -- I don't think that's currently implemented.

Quoted 6 lines

Quoted 2 lines

No one should be using the arch_* spinlocks directly. ?The spinlocks are implemented in out of line code in kernel/spinlock.c

OK--- do think this is something we need a sanity-check for, or does this fall into to a category of bad driver implementation which will get thrown out during peer review?

Hmm, actually it looks like you can end up with configurations where the spinlocks are inlined.

Quoted 2 lines

That means we'll have to get rid of the link-time discarding of the .exit sections, and discard them along with the .init sections.

... but only when CONFIG_SMP_ON_UP=y.

Nicolas

Do we need to consider any other discarded sections? In vmlinux.lds, I have:

/* Default discards */
/DISCARD/ : { *(.exit.text) *(.cpuexit.text) *(.memexit.text)
*(.exit.data) *(.cpuexit.data) *(.cpuexit.rodata) *(.memexit.data) *(.memexit.rodata) *(.exitcall.exit) *(.discard) *(.discard.*) }

If we put every fixup in its own _section_ we might be able to collapse those out of the list at link time too. However, that seems hard to achieve, since in order to do the correct link-time filtering we would need the fixup's section name to be based on the referenced section's name ... due to preprocessor limitations I don't think we can do that automatically (and manual maintenance would be a bad plan).

Quoted 6 lines · Dave Martin

Do we need to consider any other discarded sections? In vmlinux.lds, I have:

/* Default discards */
/DISCARD/ : { *(.exit.text) *(.cpuexit.text) *(.memexit.text)
*(.exit.data) *(.cpuexit.data) *(.cpuexit.rodata) *(.memexit.data) *(.memexit.rodata) *(.exitcall.exit) *(.discard) *(.discard.*) }

We need to keep all the text exit stuff, and that could well want data exit stuff, so I think we need ot keep most of that. Maybe not the .discard* stuff though.

Quoted 7 lines

If we put every fixup in its own _section_ we might be able to collapse those out of the list at link time too. However, that seems hard to achieve, since in order to do the correct link-time filtering we would need the fixup's section name to be based on the referenced section's name ... due to preprocessor limitations I don't think we can do that automatically (and manual maintenance would be a bad plan).

I suspect if it was sanely possible to do, then x86 would already be doing it as they've had this problem for a lot longer than we have. It might be worth talking to toolchain people to see if it can be made to work somehow - but we really don't want to get rid of all discarded section warnings.

IOW, if we have a text segment which is included in the image and it references discarded .exit text, we really do want it to fail at link time.

SMP_ON_UP fixups lead to vmlinux link errors if those sections are discarded at link-time. In particular this may happen for built-in __exit stuff.

This patch modifies the vmlinux linker script to reduce the amount of discarded sections, and tries to make sure that __exit sections are kept in.

This is a hack and probably wrong! Further discussion is needed.

Signed-off-by Dave Martin <[email protected]>

Since discarded sections are by definition not used, we should at least put them into the .init section so to discard them at run time. And only the EXIT_TEXT and EXIT_DATA would need to be kept (see x86 for example).

Nicolas

Quoted 7 lines · Nicolas Pitre

Since discarded sections are by definition not used, we should at least put them into the .init section so to discard them at run time. And only the EXIT_TEXT and EXIT_DATA would need to be kept (see x86 for example).

Nicolas

I'll check up on whether we need to care about writable data. For now I've just attempted to move stuff to .init.

SMP_ON_UP fixups lead to vmlinux link errors if those sections are discarded at link-time. In particular this may happen for built-in __exit stuff.

This patch modifies the vmlinux linker script to reduce the amount of discarded sections, and tries to make sure that __exit sections are kept in.

This is a hack and probably wrong! Further discussion is needed.

Quoted 8 lines · Dave P. Martin

SMP_ON_UP fixups lead to vmlinux link errors if those sections are discarded at link-time. In particular this may happen for built-in __exit stuff.

This patch modifies the vmlinux linker script to reduce the amount of discarded sections, and tries to make sure that __exit sections are kept in.

Quoted 1 line

This is a hack and probably wrong! Further discussion is needed.

Can you send the configuration which you see this problem with? I've tried to build a kernel which inlines the spinlocks, but I find that's not possible, so I'm doubting whether any fix is required for mainline.

Quoted 9 lines · Russell King - ARM Linux

Quoted 8 lines · Dave P. Martin

SMP_ON_UP fixups lead to vmlinux link errors if those sections are discarded at link-time. ?In particular this may happen for built-in __exit stuff.

This patch modifies the vmlinux linker script to reduce the amount of discarded sections, and tries to make sure that __exit sections are kept in.

Quoted 6 lines

Quoted 1 line

This is a hack and probably wrong! ?Further discussion is needed.

Can you send the configuration which you see this problem with? I've tried to build a kernel which inlines the spinlocks, but I find that's not possible, so I'm doubting whether any fix is required for mainline.

Any news on this, or can it not be reproduced?

Quoted 10 lines · Russell King - ARM Linux

Quoted 9 lines · Russell King - ARM Linux

Quoted 8 lines · Dave P. Martin

SMP_ON_UP fixups lead to vmlinux link errors if those sections are discarded at link-time. ?In particular this may happen for built-in __exit stuff.

This patch modifies the vmlinux linker script to reduce the amount of discarded sections, and tries to make sure that __exit sections are kept in.

Quoted 9 lines

Quoted 6 lines

Quoted 1 line

This is a hack and probably wrong! ?Further discussion is needed.

Can you send the configuration which you see this problem with? I've tried to build a kernel which inlines the spinlocks, but I find that's not possible, so I'm doubting whether any fix is required for mainline.

Any news on this, or can it not be reproduced?

Hi, apologies-- didn't see my mail for a bit.

I get the problem with this tree and config (which builds with SMP_ON_UP and THUMB2_KERNEL for omap3/4):

Tree: git://git.linaro.org/people/dmart/linux-2.6-arm.git
dirty/arm/omap-thumb2+merged
Config: http://people.linaro.org/~dmart/arm_omap-thumb2+v2_config

Quoted 8 lines · Dave Martin

Hi, apologies-- didn't see my mail for a bit.

I get the problem with this tree and config (which builds with SMP_ON_UP and THUMB2_KERNEL for omap3/4):

Tree: git://git.linaro.org/people/dmart/linux-2.6-arm.git
dirty/arm/omap-thumb2+merged
Config: http://people.linaro.org/~dmart/arm_omap-thumb2+v2_config

Note that this tree contains some extra patches (though I believe nothing is there which should cause the problem), and does not contain everything from rmk/devel -- so it's possible this has been fixed in the meantime or no longer occurs for some other reason...

Quoted 9 lines · Dave Martin

Quoted 8 lines · Dave Martin

Hi, apologies-- didn't see my mail for a bit.

I get the problem with this tree and config (which builds with SMP_ON_UP and THUMB2_KERNEL for omap3/4):

Tree: git://git.linaro.org/people/dmart/linux-2.6-arm.git
dirty/arm/omap-thumb2+merged
Config: http://people.linaro.org/~dmart/arm_omap-thumb2+v2_config

Thanks for the config.

Quoted 4 lines

Note that this tree contains some extra patches (though I believe nothing is there which should cause the problem), and does not contain everything from rmk/devel -- so it's possible this has been fixed in the meantime or no longer occurs for some other reason...

No need - I now have some modules which contain SMP alternatives, so we do need to fix the module loader for this, and keep the SMP alternatives replacement code around. ?It's not worth eliminating the code as if we include an informative printk() to say why we're refusing to load the module, the string would be larger than the code it eliminates. ?So, I think we need to merge the patch below to avoid run-time problems.

Quoted 6 lines · Russell King - ARM Linux

Quoted 4 lines · Dave Martin

Note that this tree contains some extra patches (though I believe nothing is there which should cause the problem), and does not contain everything from rmk/devel -- so it's possible this has been fixed in the meantime or no longer occurs for some other reason...

Quoted 6 lines

No need - I now have some modules which contain SMP alternatives, so we do need to fix the module loader for this, and keep the SMP alternatives replacement code around. ?It's not worth eliminating the code as if we include an informative printk() to say why we're refusing to load the module, the string would be larger than the code it eliminates. ?So, I think we need to merge the patch below to avoid run-time problems.

I thought the problem was not caused by removal of the fixup code, but rather by the removal of code referenced by fixups?

Quoted 15 lines · Dave Martin

Quoted 6 lines · Russell King - ARM Linux

Quoted 4 lines · Dave Martin

Note that this tree contains some extra patches (though I believe nothing is there which should cause the problem), and does not contain everything from rmk/devel -- so it's possible this has been fixed in the meantime or no longer occurs for some other reason...

Quoted 6 lines

No need - I now have some modules which contain SMP alternatives, so we do need to fix the module loader for this, and keep the SMP alternatives replacement code around. ?It's not worth eliminating the code as if we include an informative printk() to say why we're refusing to load the module, the string would be larger than the code it eliminates. ?So, I think we need to merge the patch below to avoid run-time problems.

Quoted 2 lines

I thought the problem was not caused by removal of the fixup code, but rather by the removal of code referenced by fixups?

It is. ?But:

arm-linux-objdump -h sound/core/snd-timer.ko

sound/core/snd-timer.ko: ? ? file format elf32-littlearm

Sections:
Idx Name ? ? ? ? ?Size ? ? ?VMA ? ? ? LMA ? ? ? File off ?Algn ?0 .text ? ? ? ? 00002f34 ?00000000 ?00000000 ?00000034 ?2**2 ? ? ? ? ? ? ? ? ?CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE ?1 .exit.text ? ?00000070 ?00000000 ?00000000 ?00002f68 ?2**2 ? ? ? ? ? ? ? ? ?CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE ?2 .init.text ? ?000001b4 ?00000000 ?00000000 ?00002fd8 ?2**2 ? ? ? ? ? ? ? ? ?CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE ?3 .rodata ? ? ? 000000bc ?00000000 ?00000000 ?0000318c ?2**2 ? ? ? ? ? ? ? ? ?CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA ?4 .rodata.str1.1 00000156 ?00000000 ?00000000 ?00003248 ?2**0 ? ? ? ? ? ? ? ? ?CONTENTS, ALLOC, LOAD, READONLY, DATA ?5 .alt.smp.init 00000088 ?00000000 ?00000000 ?0000339e ?2**0 ? ? ? ? ? ? ? ? ?CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
...

Here we have a module which contains the spin_unlock assembly, which contains the SMP alternatives. ?If we insert that into a kernel also built with SMP alternatives support, but which is running on a UP system, we need to run these fixups as well when the module is loaded.

Agreed -- actually, I suspected we might need to support this. But I don't think solving this problem (= keeping the fixup implementation in memory and enhancing the module loader) solved the fixups-referencing-sections-discarded-from-vmlinux problem. These seem to be two separate issues. I am filing to understand something?

Quoted 5 lines · Dave Martin

Agreed -- actually, I suspected we might need to support this. ?But I don't think solving this problem (= keeping the fixup implementation in memory and enhancing the module loader) solved the fixups-referencing-sections-discarded-from-vmlinux problem. ?These seem to be two separate issues. ?I am filing to understand something?

They are separate, but related issues. ?They both ultimately have the same cause - the placement of the spinlock code inline rather than out of line, resulting in fixups appearing all over the place rather than just in kernel/spinlock.o.

Quoted 7 lines · Russell King - ARM Linux

Quoted 5 lines · Dave Martin

Agreed -- actually, I suspected we might need to support this. ?But I don't think solving this problem (= keeping the fixup implementation in memory and enhancing the module loader) solved the fixups-referencing-sections-discarded-from-vmlinux problem. ?These seem to be two separate issues. ?I am filing to understand something?

Quoted 4 lines

They are separate, but related issues. ?They both ultimately have the same cause - the placement of the spinlock code inline rather than out of line, resulting in fixups appearing all over the place rather than just in kernel/spinlock.o.

I guess what I want to understand is whether I (or someone) still need(s) to sort out the vmlinux.lds issue.

If we're keeping inline spinlocks (I currently assume "yes"), then the vmlinux.lds issue still needs fixing. Is that correct? However, if we get rid of inline spinlocks we won't have the problem, though there may be some performance impact -- hard to judge how significant.

Quoted 14 lines · Dave Martin

Quoted 11 lines · Russell King - ARM Linux

Quoted 5 lines · Dave Martin

Agreed -- actually, I suspected we might need to support this. ?But I don't think solving this problem (= keeping the fixup implementation in memory and enhancing the module loader) solved the fixups-referencing-sections-discarded-from-vmlinux problem. ?These seem to be two separate issues. ?I am filing to understand something?

They are separate, but related issues. ?They both ultimately have the same cause - the placement of the spinlock code inline rather than out of line, resulting in fixups appearing all over the place rather than just in kernel/spinlock.o.

Quoted 2 lines

I guess what I want to understand is whether I (or someone) still need(s) to sort out the vmlinux.lds issue.

Yes we do - if you build your kernel you should find that your link fails because of discarded sections being referenced.

Quoted 5 lines

If we're keeping inline spinlocks (I currently assume "yes"), then the vmlinux.lds issue still needs fixing. ?Is that correct? ?However, if we get rid of inline spinlocks we won't have the problem, though there may be some performance impact -- hard to judge how significant.

I don't see that we can get rid of inline spinlocks - it's controlled by stuff external to the arch.

Yep -- I'm still applying my original patch to work around that, but it sounds like I need to tidy that up. Can you elaborate on what you meant by defining a "KEEP_EXIT" macro to handle this?

arch/arm/kernel/vmlinux.lds.S +9 -0

index 45b5651..343d29f 100644
@@ -21,6 +21,12 @@
 #define ARM_CPU_KEEP(x)
 #endif
 
+#if defined(CONFIG_SMP_ON_UP) && !defined(CONFIG_DEBUG_SPINLOCK)
+#define ARM_EXIT_KEEP(x)	x
+#else
+#define ARM_EXIT_KEEP(x)
+#endif
+
 OUTPUT_ARCH(arm)
 ENTRY(stext)
 

@@ -43,6 +49,7 @@ SECTIONS
 		_sinittext = .;
 			HEAD_TEXT
 			INIT_TEXT
+			ARM_EXIT_KEEP(EXIT_TEXT)
 		_einittext = .;
 		ARM_CPU_DISCARD(PROC_INFO)
 		__arch_info_begin = .;

@@ -71,6 +78,7 @@ SECTIONS
 #ifndef CONFIG_XIP_KERNEL
 		__init_begin = _stext;
 		INIT_DATA
+		ARM_EXIT_KEEP(EXIT_DATA)
 #endif
 	}
 

@@ -166,6 +174,7 @@ SECTIONS
 		. = ALIGN(PAGE_SIZE);
 		__init_begin = .;
 		INIT_DATA
+		ARM_EXIT_KEEP(EXIT_DATA)
 		. = ALIGN(PAGE_SIZE);
 		__init_end = .;
 #endif

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Hi,

On Fri, Feb 11, 2011 at 04:32:47PM +0000, Russell King - ARM Linux wrote:

[...]

Quoted 42 lines

arch/arm/kernel/vmlinux.lds.S +0 -0

index 45b5651..343d29f 100644
@@ -21,6 +21,12 @@
 ?#define ARM_CPU_KEEP(x)

?#endif

arch/arm/kernel/vmlinux.lds.S +6 -0

+#if defined(CONFIG_SMP_ON_UP) && !defined(CONFIG_DEBUG_SPINLOCK)
+#define ARM_EXIT_KEEP(x) ? ? x
+#else
+#define ARM_EXIT_KEEP(x)
+#endif
+

?OUTPUT_ARCH(arm)
?ENTRY(stext)

arch/arm/kernel/vmlinux.lds.S +0 -0

@@ -43,6 +49,7 @@ SECTIONS
 ? ? ? ? ? ? ? _sinittext = .;

? ? ? ? ? ? ? ? ? ? ? HEAD_TEXT
? ? ? ? ? ? ? ? ? ? ? INIT_TEXT
+ ? ? ? ? ? ? ? ? ? ? ARM_EXIT_KEEP(EXIT_TEXT)
? ? ? ? ? ? ? _einittext = .;
? ? ? ? ? ? ? ARM_CPU_DISCARD(PROC_INFO)
? ? ? ? ? ? ? __arch_info_begin = .;

arch/arm/kernel/vmlinux.lds.S +0 -0

@@ -71,6 +78,7 @@ SECTIONS

?#ifndef CONFIG_XIP_KERNEL
? ? ? ? ? ? ? __init_begin = _stext;
? ? ? ? ? ? ? INIT_DATA
+ ? ? ? ? ? ? ARM_EXIT_KEEP(EXIT_DATA)
?#endif
? ? ? }

arch/arm/kernel/vmlinux.lds.S +0 -0

@@ -166,6 +174,7 @@ SECTIONS
 ? ? ? ? ? ? ? . = ALIGN(PAGE_SIZE);
 ? ? ? ? ? ? ? __init_begin = .;

? ? ? ? ? ? ? INIT_DATA
+ ? ? ? ? ? ? ARM_EXIT_KEEP(EXIT_DATA)
? ? ? ? ? ? ? . = ALIGN(PAGE_SIZE);
? ? ? ? ? ? ? __init_end = .;
?#endif

Quoted 4 lines

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

This works for me in a case known to fail without the patch.

Tested-by Dave Martin <[email protected]>

I still don't quite understand the intricacies of how vmlinux is laid out, but the patch looks sensible.

Do you need anything more from me on this? ?If not, I will throw away my patch and replace it with yours.

Quoted 9 lines · Nicolas Pitre

Quoted 5 lines · Russell King - ARM Linux

Hmm, actually it looks like you can end up with configurations where the spinlocks are inlined.

That means we'll have to get rid of the link-time discarding of the .exit sections, and discard them along with the .init sections.

... but only when CONFIG_SMP_ON_UP=y.

Do we need to consider any other discarded sections? In vmlinux.lds, I have:

/* Default discards */
/DISCARD/ : { *(.exit.text) *(.cpuexit.text) *(.memexit.text)
*(.exit.data) *(.cpuexit.data) *(.cpuexit.rodata) *(.memexit.data) *(.memexit.rodata) *(.exitcall.exit) *(.discard) *(.discard.*) }

Alternatively, I wonder .alt.smp.init could make weak references instead of normal references: then if the referenced section is discarded, the reference will revert to 0 and the fixup code can ignore it (at the expense of wasting a bit of space in the fixup table ... but it's no worse than the current situation). From experimentation, it seems this doesn't work if the referenced symbol is defined in the same file, since the linker resolves the reference before discarding sections, but maybe there's a way around it...

Quoted 8 lines · Dave Martin

Alternatively, I wonder .alt.smp.init could make weak references instead of normal references: then if the referenced section is discarded, the reference will revert to 0 and the fixup code can ignore it (at the expense of wasting a bit of space in the fixup table ... but it's no worse than the current situation). From experimentation, it seems this doesn't work if the referenced symbol is defined in the same file, since the linker resolves the reference before discarding sections, but maybe there's a way around it...

That's strange. How can the linker resolve the reference if the referenced symbol is not in the same section as the reference source? What happens when the final link completes?

Quoted 10 lines · Nicolas Pitre

Quoted 8 lines · Dave Martin

Alternatively, I wonder .alt.smp.init could make weak references instead of normal references: then if the referenced section is discarded, the reference will revert to 0 and the fixup code can ignore it (at the expense of wasting a bit of space in the fixup table ... but it's no worse than the current situation). ?From experimentation, it seems this doesn't work if the referenced symbol is defined in the same file, since the linker resolves the reference before discarding sections, but maybe there's a way around it...

Quoted 3 lines

That's strange. ?How can the linker resolve the reference if the referenced symbol is not in the same section as the reference source? What happens when the final link completes?

$ cat t.s
? ? ? ?.section ".exit.text", "ax", %progbits
? ? ? ?mov ? ? r0, r0
a: ? ? ?mov ? ? r0, r0

? ? ? ?.pushsection ".alt.smp.fixup", "a"
? ? ? ?.weak ? a
? ? ? ?.long ? a
? ? ? ?mov ? ? r0, r1
? ? ? ?.popsection

$ arm-linux-as -o t.o t.s
$ arm-linux-nm t.o
00000004 W a
$ arm-linux-objdump -Dr t.o

Disassembly of section .exit.text:

00000000 <a-0x4>:
? 0: ? e1a00000 ? ? ? ?nop ? ? ? ? ? ? ? ? ? ? (mov r0,r0)

00000004 <a>:
? 4: ? e1a00000 ? ? ? ?.word ? 0xe1a00000

Disassembly of section .alt.smp.fixup:

00000000 <.alt.smp.fixup>:
? 0: ? 00000000 ? ? ? ?.word ? 0x00000000
? ? ? ? ? ? ? ? ? ? ? ?0: R_ARM_ABS32 ?a
? 4: ? e1a00001 ? ? ? ?mov ? ? r0, r1

which when linked with:

SECTIONS
{
? ? ? ?.alt.smp.init : { *(.alt.smp.init) }

? ? ? ?/DISCARD/ : { *(.exit.text) }
}

$ arm-linux-ld -T t.lds -r -o t.oo t.o
`a' referenced in section `.alt.smp.fixup' of t.o: defined in discarded section `.exit.text' of t.o

So I don't think weak symbols work like we want them to.

Quoted 2 lines · Russell King - ARM Linux

So I don't think weak symbols work like we want them to.

That was the conclusion I came to also ... the linker seems to resolve references in each object before discarding sections, so the weak reference has already become concrete and section discard breaks it.

Quoted 8 lines · Dave Martin

Quoted 2 lines · Russell King - ARM Linux

So I don't think weak symbols work like we want them to.

That was the conclusion I came to also ... the linker seems to resolve references in each object before discarding sections, so the weak reference has already become concrete and section discard breaks it.

Well, I must add to the chorus.

And it seems to be impossible to obtain the name of the currently active section from gas either, which would have made the conditional omission of the fixup possible, or similar.

Bummer.

Nicolas

Introduce a CPU_V6K configuration option for platforms to select if they have a V6K CPU core. This allows us to identify whether we need to support ARMv6 CPUs without the V6K SMP extensions at build time.

Currently CPU_V6K is just an alias for CPU_V6, and all places which reference CPU_V6 are replaced by (CPU_V6 || CPU_V6K).

Select CPU_V6K from platforms which are known to be V6K-only.

Signed-off-by Russell King <[email protected]>

Introduce a CPU_V6K configuration option for platforms to select if they have a V6K CPU core. This allows us to identify whether we need to support ARMv6 CPUs without the V6K SMP extensions at build time.

Currently CPU_V6K is just an alias for CPU_V6, and all places which reference CPU_V6 are replaced by (CPU_V6 || CPU_V6K).

Select CPU_V6K from platforms which are known to be V6K-only.

Signed-off-by Russell King <[email protected]>

arch/arm/Makefile +0 -0

index c22c1ad..9c43052 100644
@@ -89,6 +89,7 @@ tune-$(CONFIG_CPU_XSCALE)	:=$(call cc-option,-mtune=xscale,-mtune=strongarm110)
 tune-$(CONFIG_CPU_XSC3)		:=$(call cc-option,-mtune=xscale,-mtune=strongarm110) -Wa,-

mcpu=xscale
tune-$(CONFIG_CPU_FEROCEON) :=$(call cc-option,-mtune=marvell-f,-mtune=xscale)
tune-$(CONFIG_CPU_V6) :=$(call cc-option,-mtune=arm1136j-s,-mtune=strongarm)
+tune-$(CONFIG_CPU_V6K) :=$(call cc-option,-mtune=arm1136j-s,-mtune=strongarm)

Quoted 4 lines

arch/arm/Makefile +0 -0

index c22c1ad..9c43052 100644

Quoted 6 lines

arch/arm/Makefile +0 -0

@@ -89,6 +89,7 @@ tune-$(CONFIG_CPU_XSCALE)	:=$(call cc-option,-mtune=xscale,-mtune=strongarm110)
 tune-$(CONFIG_CPU_XSC3)		:=$(call cc-option,-mtune=xscale,-mtune=strongarm110) -Wa,-

mcpu=xscale
tune-$(CONFIG_CPU_FEROCEON) :=$(call cc-option,-mtune=marvell-f,-mtune=xscale)
tune-$(CONFIG_CPU_V6) :=$(call cc-option,-mtune=arm1136j-s,-mtune=strongarm)
+tune-$(CONFIG_CPU_V6K) :=$(call cc-option,-mtune=arm1136j-s,-mtune=strongarm)

Can we change the tune target to 1176 for v6k?

arch/arm/Makefile

Quoted 7 lines

config DMA_CACHE_RWFO

arch/arm/Makefile +1 -1

 	bool "Enable read/write for ownership DMA cache maintenance"
-	depends on CPU_V6 && SMP
+	depends on (CPU_V6 || CPU_V6K) && SMP
 	default y
 	help
 	  The Snoop Control Unit on ARM11MPCore does not detect the

Can we lose the CPU_V6 check here? RWFO is only required for 11MPCore so I think we just need to check CPU_V6K && SMP.

I'd rather not in this patch - this patch adds CPU_V6K as an alias for CPU_V6 - so eveywhere which referenced CPU_V6 becomes (CPU_V6 || CPU_V6K). We could remove it in a later patch though.

Quoted 3 lines

I'd rather not in this patch - this patch adds CPU_V6K as an alias for CPU_V6 - so eveywhere which referenced CPU_V6 becomes (CPU_V6 || CPU_V6K). We could remove it in a later patch though.

Here's a follow-up patch to do that. There's a few other things which could do with updating now that we have CPU_Vx* straightened out. I've also recently noticed that the v6 and v7 cache stuff doesn't use the direct-call optimization (see commented out bits in cacheflush.h).

That was because my original V6 support used the block cache operations, which had to be run-time tested for presence, and so modified the global cpu_cache methods directly. As the block cache stuff was dropped, we should re-enable this optimization.

That said, I'd rather not add too much to this series as I think it needs to go into mainline ASAP - and probably stable as well, even though it's on the large side - once properly and fully tested.

Subject: [PATCH] ARM: v6k: DMA_CACHE_RWFO isn't appropriate for non-v6k CPUs

Limit DMA_CACHE_RWFO to only v6k SMP CPUs - V6 CPUs aren't SMP capable, so the read/write for ownership work-around doesn't apply to them.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/mm/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/mm/Kconfig +0 -0

index 843bc8c..808b832 100644

Quoted 3 lines

I'd rather not in this patch - this patch adds CPU_V6K as an alias for CPU_V6 - so eveywhere which referenced CPU_V6 becomes (CPU_V6 || CPU_V6K). We could remove it in a later patch though.

Here's a follow-up patch to do that. There's a few other things which could do with updating now that we have CPU_Vx* straightened out. I've also recently noticed that the v6 and v7 cache stuff doesn't use the direct-call optimization (see commented out bits in cacheflush.h).

That was because my original V6 support used the block cache operations, which had to be run-time tested for presence, and so modified the global cpu_cache methods directly. As the block cache stuff was dropped, we should re-enable this optimization.

That said, I'd rather not add too much to this series as I think it needs to go into mainline ASAP - and probably stable as well, even though it's on the large side - once properly and fully tested.

Subject: [PATCH] ARM: v6k: DMA_CACHE_RWFO isn't appropriate for non-v6k CPUs

Limit DMA_CACHE_RWFO to only v6k SMP CPUs - V6 CPUs aren't SMP capable, so the read/write for ownership work-around doesn't apply to them.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/mm/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/mm/Kconfig +0 -0

index 843bc8c..808b832 100644

Introduce a CPU_V6K configuration option for platforms to select if they have a V6K CPU core. This allows us to identify whether we need to support ARMv6 CPUs without the V6K SMP extensions at build time.

Currently CPU_V6K is just an alias for CPU_V6, and all places which reference CPU_V6 are replaced by (CPU_V6 || CPU_V6K).

Select CPU_V6K from platforms which are known to be V6K-only.

Signed-off-by Russell King <[email protected]>

arch/arm/Makefile +0 -0

index c22c1ad..9c43052 100644
@@ -89,6 +89,7 @@ tune-$(CONFIG_CPU_XSCALE)	:=$(call cc-option,-mtune=xscale,-mtune=strongarm110)
 tune-$(CONFIG_CPU_XSC3)		:=$(call cc-option,-mtune=xscale,-mtune=strongarm110) -Wa,-

mcpu=xscale
tune-$(CONFIG_CPU_FEROCEON) :=$(call cc-option,-mtune=marvell-f,-mtune=xscale)
tune-$(CONFIG_CPU_V6) :=$(call cc-option,-mtune=arm1136j-s,-mtune=strongarm)
+tune-$(CONFIG_CPU_V6K) :=$(call cc-option,-mtune=arm1136j-s,-mtune=strongarm)

Make Dove platforms select the new V6K CPU option.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/mach-dove/Kconfig |    4 +++-
 arch/arm/mm/Kconfig        |    4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

arch/arm/mach-dove/Kconfig +3 -1

index a4ed390..822439a 100644
@@ -5,12 +5,14 @@ menu "Marvell Dove Implementations"
 config MACH_DOVE_DB
 	bool "Marvell DB-MV88AP510 Development Board"
 	select I2C_BOARDINFO
+	select CPU_V6K
 	help
 	  Say 'Y' here if you want your kernel to support the
 	  Marvell DB-MV88AP510 Development Board.
 
- config MACH_CM_A510
+config MACH_CM_A510
 	bool "CompuLab CM-A510 Board"
+	select CPU_V6K
 	help
 	  Say 'Y' here if you want your kernel to support the
 	  CompuLab CM-A510 Board.

arch/arm/mm/Kconfig +2 -2

index 559e933..22a3f4a 100644
@@ -390,7 +390,7 @@ config CPU_PJ4
 
 # ARMv6
 config CPU_V6
-	bool "Support ARM V6 processor" if ARCH_INTEGRATOR || MACH_REALVIEW_EB || MACH_REALVIEW_PBX || ARCH_DOVE
+	bool "Support ARM V6 processor" if ARCH_INTEGRATOR || MACH_REALVIEW_EB || MACH_REALVIEW_PBX
 	select CPU_32v6
 	select CPU_ABRT_EV6
 	select CPU_PABRT_V6

@@ -403,7 +403,7 @@ config CPU_V6
 
 # ARMv6k
 config CPU_V6K
-	bool "Support ARM V6K processor" if ARCH_INTEGRATOR || MACH_REALVIEW_EB || MACH_REALVIEW_PBX || ARCH_DOVE
+	bool "Support ARM V6K processor" if ARCH_INTEGRATOR || MACH_REALVIEW_EB || MACH_REALVIEW_PBX
 	select CPU_32v6
 	select CPU_32v6K if !ARCH_OMAP2
 	select CPU_ABRT_EV6

Signature · 7 lines

1.6.2.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

If CONFIG_CPU_V6 is enabled, then the kernel must support ARMv6 CPUs which don't have the V6K extensions implemented. Always use the dummy store-exclusive method to ensure that the exclusive monitors are cleared.

If CONFIG_CPU_V6 is not set, but CONFIG_CPU_32v6K is enabled, then we have the K extensions available on all CPUs we're building support for, so we can use the new clear-exclusive instruction.

Signed-off-by Russell King <[email protected]>

If CONFIG_CPU_V6 is enabled, we must avoid the byte/halfword/doubleword exclusive operations, which aren't implemented before V6K. Use the generic versions (or omit them) instead.

If CONFIG_CPU_V6 is not set, but CONFIG_CPU_32v6K is enabled, we have the K extnesions, so use these new instructions.

Signed-off-by Russell King <[email protected]>

If CONFIG_CPU_V6 is enabled, avoid using the double-word exclusive instructions in the kernel's atomic implementations as these are not supported. Fall back to the generic spinlock code instead.

Signed-off-by Russell King <[email protected]>

If CONFIG_CPU_V6 is enabled, avoid using the double-word exclusive instructions in the kernel's atomic implementations as these are not supported. Fall back to the generic spinlock code instead.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/Kconfig +0 -0

index 95ba92f..283d7b9 100644

If CONFIG_CPU_V6 is enabled, avoid using the double-word exclusive instructions in the kernel's atomic implementations as these are not supported. Fall back to the generic spinlock code instead.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/Kconfig +0 -0

index 95ba92f..283d7b9 100644

If CONFIG_CPU_V6 is enabled, we may or may not have the TLS register. Use the conditional code which copes with this variability. Otherwise, if CONFIG_CPU_32v6K is set, we know we have the TLS register on all supported CPUs, so use it unconditionally.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/include/asm/tls.h |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

arch/arm/include/asm/tls.h +5 -6

index e71d6ff..60843eb 100644
@@ -28,15 +28,14 @@
 #define tls_emu		1
 #define has_tls_reg		1
 #define set_tls		set_tls_none
-#elif __LINUX_ARM_ARCH__ >= 7 ||					\
-	(__LINUX_ARM_ARCH__ == 6 && defined(CONFIG_CPU_32v6K))
-#define tls_emu		0
-#define has_tls_reg		1
-#define set_tls		set_tls_v6k
-#elif __LINUX_ARM_ARCH__ == 6
+#elif defined(CONFIG_CPU_V6)
 #define tls_emu		0
 #define has_tls_reg		(elf_hwcap & HWCAP_TLS)
 #define set_tls		set_tls_v6
+#elif defined(CONFIG_CPU_32v6K)
+#define tls_emu		0
+#define has_tls_reg		1
+#define set_tls		set_tls_v6k
 #else
 #define tls_emu		0
 #define has_tls_reg		0

Signature · 7 lines

1.6.2.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 5 lines · Russell King - ARM Linux

If CONFIG_CPU_V6 is enabled, we may or may not have the TLS register. Use the conditional code which copes with this variability. Otherwise, if CONFIG_CPU_32v6K is set, we know we have the TLS register on all supported CPUs, so use it unconditionally.

Quoted 1 line

Signed-off-by Russell King <[email protected]>

Acked-by Nicolas Pitre <[email protected]>

Note: this is becoming a bit confusing and prone to mistake when we have:

	CONFIG_CPU_V6
	CONFIG_CPU_V6K
	CONFIG_CPU_32v6
	CONFIG_CPU_32v6K

I don't know what to suggest for a less similar naming scheme between the CPU design and the allowed instruction set though.

It may be better at some point to get rid of the CPU_32v* and replace them with CPU_ARCH_V* instead, which makes it clear that these ones definitely refer to the architecture versions.

Quoted 3 lines · Russell King - ARM Linux

It may be better at some point to get rid of the CPU_32v* and replace them with CPU_ARCH_V* instead, which makes it clear that these ones definitely refer to the architecture versions.

Last point on this is that I think we want to sanitize PJ4.

If we think about CPU_V7 as being the selection of ARM CPUs which are V7 architectures, CPU_PJ4 should be a separate config option selecting all the same symbols which CPU_V7 does - and that should also select proc-v7.S for building.

I'm not sure it makes sense to have (eg) the V7 perf events available on PJ4 or the ARM errata for their V7 cores?

Does PJ4 have VFPv3? Is Neon available on PJ4? What about the DCC debug registers?

Quoted 1 line · Nicolas Pitre

Note: this is becoming a bit confusing and prone to mistake when we have:

Agreed.

Quoted 4 lines

	CONFIG_CPU_V6
	CONFIG_CPU_V6K
	CONFIG_CPU_32v6
	CONFIG_CPU_32v6K

It may be better at some point to get rid of the CPU_32v* and replace them with CPU_ARCH_V* instead, which makes it clear that these ones definitely refer to the architecture versions.

The last point while we're here is I don't think it helps to have the conditionals spread between the Kconfig files and this file - I'd much rather see them all in Kconfig so we can see exactly how we end up with each option (TLS_EMUL, TLS_V6, TLS_V6K) in one place.

Quoted 6 lines · Russell King - ARM Linux

If CONFIG_CPU_V6 is enabled, we may or may not have the TLS register. Use the conditional code which copes with this variability. Otherwise, if CONFIG_CPU_32v6K is set, we know we have the TLS register on all supported CPUs, so use it unconditionally.

Signed-off-by Russell King <[email protected]>

Acked-by Nicolas Pitre <[email protected]>

Rather than turning off CPU domain switching when the build architecture includes ARMv6K, thereby causing problems for ARMv6-supporting kernels, turn it on when it's required to support a CPU architecture.

Signed-off-by Russell King <[email protected]>

---
 arch/arm/mm/Kconfig |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

arch/arm/mm/Kconfig +5 -2

index 22a3f4a..29215f5 100644
@@ -435,25 +435,30 @@ config CPU_32v3
 	bool
 	select TLS_REG_EMUL if SMP || !MMU
 	select NEEDS_SYSCALL_FOR_CMPXCHG if SMP
+	select CPU_USE_DOMAINS if MMU
 
 config CPU_32v4
 	bool
 	select TLS_REG_EMUL if SMP || !MMU
 	select NEEDS_SYSCALL_FOR_CMPXCHG if SMP
+	select CPU_USE_DOMAINS if MMU
 
 config CPU_32v4T
 	bool
 	select TLS_REG_EMUL if SMP || !MMU
 	select NEEDS_SYSCALL_FOR_CMPXCHG if SMP
+	select CPU_USE_DOMAINS if MMU
 
 config CPU_32v5
 	bool
 	select TLS_REG_EMUL if SMP || !MMU
 	select NEEDS_SYSCALL_FOR_CMPXCHG if SMP
+	select CPU_USE_DOMAINS if MMU
 
 config CPU_32v6
 	bool
 	select TLS_REG_EMUL if !CPU_32v6K && !MMU
+	select CPU_USE_DOMAINS if CPU_V6 && MMU
 
 config CPU_32v6K
 	bool "Support ARM V6K processor extensions" if !SMP

@@ -620,8 +625,6 @@ config CPU_CP15_MPU
 
 config CPU_USE_DOMAINS
 	bool
-	depends on MMU
-	default y if !CPU_32v6K
 	help
 	  This option enables or disables the use of domain switching
 	  via the set_fs() function.

Signature · 7 lines

1.6.2.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 5 lines · Russell King - ARM Linux

Rather than turning off CPU domain switching when the build architecture includes ARMv6K, thereby causing problems for ARMv6-supporting kernels, turn it on when it's required to support a CPU architecture.

Signed-off-by Russell King <[email protected]>

Acked-by Nicolas Pitre <[email protected]>

Rather than turning off CPU domain switching when the build architecture includes ARMv6K, thereby causing problems for ARMv6-supporting kernels, turn it on when it's required to support a CPU architecture.

Quoted 3 lines · Russell King - ARM Linux

Rather than turning off CPU domain switching when the build architecture includes ARMv6K, thereby causing problems for ARMv6-supporting kernels, turn it on when it's required to support a CPU architecture.

(sorry, I'm well behind list messages, didn't reply earlier)

Does it mean that we get domains enabled even for ARMv7 when ARMv6 is built in the same kernel? The side effect is that user read-only addresses are writable by the kernel and SWP emulation no longer works.

Quoted 6 lines · Catalin Marinas

Quoted 3 lines · Russell King - ARM Linux

Rather than turning off CPU domain switching when the build architecture includes ARMv6K, thereby causing problems for ARMv6-supporting kernels, turn it on when it's required to support a CPU architecture.

Quoted 6 lines

(sorry, I'm well behind list messages, didn't reply earlier)

Does it mean that we get domains enabled even for ARMv7 when ARMv6 is built in the same kernel? The side effect is that user read-only addresses are writable by the kernel and SWP emulation no longer works.

It means that a kernel with ARMv6 through to ARMv7 will have CPU domains enabled. So maybe we want:

config CPU_32v6

        select CPU_USE_DOMAINS if !CPU_V6K && MMU

instead?

Does this mean that we could still configure a kernel to run on CPU_V6 with domains disabled? The vectors page becomes read-only and setting the TLS would fail.

My point is that we may want SWP_EMULATE disabled (or depending on ! CPU_USE_DOMAINS). With domains enabled every read-only user page is writeable by the kernel. This has the side-effect that SWP emulation using LDREX/STREX breaks COW.

We could change early_trap_init() to always use the vectors_page rather than the high mapping and determine at run-time (patch the code) whether set_pte_ext sets R/W kernel permissions for RO user pages.

Quoted 4 lines · Catalin Marinas

My point is that we may want SWP_EMULATE disabled (or depending on ! CPU_USE_DOMAINS). With domains enabled every read-only user page is writeable by the kernel. This has the side-effect that SWP emulation using LDREX/STREX breaks COW.

Yes, and maybe we should instead just enable the SWP instruction by default on ARMv7, and if SWP emulation is built, disable it at that point.

Quoted 8 lines · Russell King - ARM Linux

Quoted 4 lines · Catalin Marinas

My point is that we may want SWP_EMULATE disabled (or depending on ! CPU_USE_DOMAINS). With domains enabled every read-only user page is writeable by the kernel. This has the side-effect that SWP emulation using LDREX/STREX breaks COW.

Yes, and maybe we should instead just enable the SWP instruction by default on ARMv7, and if SWP emulation is built, disable it at that point.

We can't disable the SWP instruction as long as domains are enabled (COW not working for in-kernel STREX).

On ARMv7 we could always force R/O kernel/user pages in set_pte_ext independent of the domains setting and have early_trap_init() use vectors_page() if cpu_architecture() >= 7 (this would actually catch ARM11MPCore as well because of the way we interpret CPUID).

Quoted 13 lines · Catalin Marinas

Quoted 8 lines · Russell King - ARM Linux

Quoted 4 lines · Catalin Marinas

My point is that we may want SWP_EMULATE disabled (or depending on ! CPU_USE_DOMAINS). With domains enabled every read-only user page is writeable by the kernel. This has the side-effect that SWP emulation using LDREX/STREX breaks COW.

Yes, and maybe we should instead just enable the SWP instruction by default on ARMv7, and if SWP emulation is built, disable it at that point.

We can't disable the SWP instruction as long as domains are enabled (COW not working for in-kernel STREX).

Quoted 4 lines

On ARMv7 we could always force R/O kernel/user pages in set_pte_ext independent of the domains setting and have early_trap_init() use vectors_page() if cpu_architecture() >= 7 (this would actually catch ARM11MPCore as well because of the way we interpret CPUID).

What about a kernel covering ARMv6 too? Writing to an aliased mapping of the vectors page (as required for TLS emulation) will require additional cache maintainence on every context switch.

1. SWP emulation requires domain support turned off

2. We can't turn domains off without creating a vectors page alias

3. We can't have a separate vectors alias with ARMv6 VIPT aliasing caches without additional cache maintainence.

I don't think overloading the context switch with yet more conditionals based on yet more TLS combinations is a practical solution.

So, ARMv6 TLS emulation, and SWP emulation are incompatible with each other.

So either we don't have the SWP instruction at all on ARMv6+, or we have it enabled in hardware, or we don't and use the emulation where possible.

Quoted 19 lines · Russell King - ARM Linux

Quoted 17 lines · Catalin Marinas

Quoted 8 lines · Russell King - ARM Linux

Quoted 4 lines · Catalin Marinas

My point is that we may want SWP_EMULATE disabled (or depending on ! CPU_USE_DOMAINS). With domains enabled every read-only user page is writeable by the kernel. This has the side-effect that SWP emulation using LDREX/STREX breaks COW.

Yes, and maybe we should instead just enable the SWP instruction by default on ARMv7, and if SWP emulation is built, disable it at that point.

We can't disable the SWP instruction as long as domains are enabled (COW not working for in-kernel STREX).

On ARMv7 we could always force R/O kernel/user pages in set_pte_ext independent of the domains setting and have early_trap_init() use vectors_page() if cpu_architecture() >= 7 (this would actually catch ARM11MPCore as well because of the way we interpret CPUID).

Quoted 3 lines

What about a kernel covering ARMv6 too? Writing to an aliased mapping of the vectors page (as required for TLS emulation) will require additional cache maintainence on every context switch.

With your latest patches, do we use the TLS emulation on ARMv7 (UP) if v6 is compiled in? If that's the case, we may have a problem - I talked to the toolchain guys and it looks like code optimised for ARMv7 reads the TLS register directly without going through the kuser helper.