linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Avoid unpaired stwcx. on some processors
@ 2007-11-09 22:08 Becky Bruce
  2007-11-09 23:19 ` Olof Johansson
  0 siblings, 1 reply; 7+ messages in thread
From: Becky Bruce @ 2007-11-09 22:08 UTC (permalink / raw)
  To: linuxppc-dev

The context switch code in the kernel issues a dummy stwcx. to clear the
reservation, as recommended by the architecture. However, some processors
can have issues if this stwcx to address A occurs while the reservation
is already held to a different address B.  To avoid this problem, the dummy
stwcx. needs to be paired with a dummy lwarx to the same address.

This patch adds the dummy lwarx, and creates a cpu feature bit to indicate
which cpus are affected. Tested on mpc8641_hpcn_defconfig in arch/powerpc;
build tested in arch/ppc.

<Signed-off-by> Becky Bruce <becky.bruce@freescale.com>
---
 arch/powerpc/kernel/entry_32.S |    6 ++++++
 arch/ppc/kernel/entry.S        |    6 ++++++
 include/asm-powerpc/cputable.h |   22 ++++++++++++----------
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 21d889e..6461dca 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -244,6 +244,9 @@ syscall_exit_cont:
 	andis.	r10,r0,DBCR0_IC@h
 	bnel-	load_dbcr0
 #endif
+BEGIN_FTR_SECTION
+	lwarx	r7,0,r1
+END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
 	stwcx.	r0,0,r1			/* to clear the reservation */
 	lwz	r4,_LINK(r1)
 	lwz	r5,_CCR(r1)
@@ -694,6 +697,9 @@ restore:
 	mtctr	r11
 
 	PPC405_ERR77(0,r1)
+BEGIN_FTR_SECTION
+	lwarx	r11,0,r1
+END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
 	stwcx.	r0,0,r1			/* to clear the reservation */
 
 #if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
diff --git a/arch/ppc/kernel/entry.S b/arch/ppc/kernel/entry.S
index fba7ca1..aa57dee 100644
--- a/arch/ppc/kernel/entry.S
+++ b/arch/ppc/kernel/entry.S
@@ -244,6 +244,9 @@ syscall_exit_cont:
 	andis.	r10,r0,DBCR0_IC@h
 	bnel-	load_dbcr0
 #endif
+BEGIN_FTR_SECTION
+	lwarx	r7,0,r1
+END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
 	stwcx.	r0,0,r1			/* to clear the reservation */
 	lwz	r4,_LINK(r1)
 	lwz	r5,_CCR(r1)
@@ -690,6 +693,9 @@ restore:
 	mtctr	r11
 
 	PPC405_ERR77(0,r1)
+BEGIN_FTR_SECTION
+	lwarx	r11,0,r1
+END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
 	stwcx.	r0,0,r1			/* to clear the reservation */
 
 #if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
diff --git a/include/asm-powerpc/cputable.h b/include/asm-powerpc/cputable.h
index 9d74338..4525c78 100644
--- a/include/asm-powerpc/cputable.h
+++ b/include/asm-powerpc/cputable.h
@@ -138,6 +138,7 @@ extern void do_feature_fixups(unsigned long value, void *fixup_start,
 #define CPU_FTR_FPU_UNAVAILABLE		ASM_CONST(0x0000000000800000)
 #define CPU_FTR_UNIFIED_ID_CACHE	ASM_CONST(0x0000000001000000)
 #define CPU_FTR_SPE			ASM_CONST(0x0000000002000000)
+#define CPU_FTR_NEED_PAIRED_STWCX	ASM_CONST(0x0000000004000000)
 
 /*
  * Add the 64-bit processor unique features in the top half of the word;
@@ -261,25 +262,25 @@ extern void do_feature_fixups(unsigned long value, void *fixup_start,
 #define CPU_FTRS_7450_20	(CPU_FTR_COMMON | \
 	    CPU_FTR_USE_TB | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
-	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
+	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_7450_21	(CPU_FTR_COMMON | \
 	    CPU_FTR_USE_TB | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_L3_DISABLE_NAP | \
-	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
+	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_7450_23	(CPU_FTR_COMMON | \
-	    CPU_FTR_USE_TB | \
+	    CPU_FTR_USE_TB | CPU_FTR_NEED_PAIRED_STWCX | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
 #define CPU_FTRS_7455_1	(CPU_FTR_COMMON | \
-	    CPU_FTR_USE_TB | \
+	    CPU_FTR_USE_TB | CPU_FTR_NEED_PAIRED_STWCX | \
 	    CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | CPU_FTR_L3CR | \
 	    CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | CPU_FTR_HAS_HIGH_BATS | \
 	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
 #define CPU_FTRS_7455_20	(CPU_FTR_COMMON | \
-	    CPU_FTR_USE_TB | \
+	    CPU_FTR_USE_TB | CPU_FTR_NEED_PAIRED_STWCX | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_L3_DISABLE_NAP | \
@@ -289,31 +290,32 @@ extern void do_feature_fixups(unsigned long value, void *fixup_start,
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_HAS_HIGH_BATS | \
-	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
+	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_7447_10	(CPU_FTR_COMMON | \
 	    CPU_FTR_USE_TB | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_HAS_HIGH_BATS | \
-	    CPU_FTR_NEED_COHERENT | CPU_FTR_NO_BTIC | CPU_FTR_PPC_LE)
+	    CPU_FTR_NEED_COHERENT | CPU_FTR_NO_BTIC | CPU_FTR_PPC_LE | \
+	    CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_7447	(CPU_FTR_COMMON | \
 	    CPU_FTR_USE_TB | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_HAS_HIGH_BATS | \
-	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
+	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_7447A	(CPU_FTR_COMMON | \
 	    CPU_FTR_USE_TB | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_HAS_HIGH_BATS | \
-	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
+	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_7448	(CPU_FTR_COMMON | \
 	    CPU_FTR_USE_TB | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_HAS_HIGH_BATS | \
-	    CPU_FTR_PPC_LE)
+	    CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_82XX	(CPU_FTR_COMMON | \
 	    CPU_FTR_MAYBE_CAN_DOZE | CPU_FTR_USE_TB)
 #define CPU_FTRS_G2_LE	(CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | \
-- 
1.5.3

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH] Avoid unpaired stwcx. on some processors
@ 2007-11-09 22:17 Becky Bruce
  0 siblings, 0 replies; 7+ messages in thread
From: Becky Bruce @ 2007-11-09 22:17 UTC (permalink / raw)
  To: linuxppc-dev

The context switch code in the kernel issues a dummy stwcx. to clear the
reservation, as recommended by the architecture. However, some processors
can have issues if this stwcx to address A occurs while the reservation
is already held to a different address B.  To avoid this problem, the dummy
stwcx. needs to be paired with a dummy lwarx to the same address.

This patch adds the dummy lwarx, and creates a cpu feature bit to indicate
which cpus are affected. Tested on mpc8641_hpcn_defconfig in arch/powerpc;
build tested in arch/ppc.

Signed-off-by: Becky Bruce <becky.bruce@freescale.com>
---
Resend with correct signed-off-by line!

 arch/powerpc/kernel/entry_32.S |    6 ++++++
 arch/ppc/kernel/entry.S        |    6 ++++++
 include/asm-powerpc/cputable.h |   22 ++++++++++++----------
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 21d889e..6461dca 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -244,6 +244,9 @@ syscall_exit_cont:
 	andis.	r10,r0,DBCR0_IC@h
 	bnel-	load_dbcr0
 #endif
+BEGIN_FTR_SECTION
+	lwarx	r7,0,r1
+END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
 	stwcx.	r0,0,r1			/* to clear the reservation */
 	lwz	r4,_LINK(r1)
 	lwz	r5,_CCR(r1)
@@ -694,6 +697,9 @@ restore:
 	mtctr	r11
 
 	PPC405_ERR77(0,r1)
+BEGIN_FTR_SECTION
+	lwarx	r11,0,r1
+END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
 	stwcx.	r0,0,r1			/* to clear the reservation */
 
 #if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
diff --git a/arch/ppc/kernel/entry.S b/arch/ppc/kernel/entry.S
index fba7ca1..aa57dee 100644
--- a/arch/ppc/kernel/entry.S
+++ b/arch/ppc/kernel/entry.S
@@ -244,6 +244,9 @@ syscall_exit_cont:
 	andis.	r10,r0,DBCR0_IC@h
 	bnel-	load_dbcr0
 #endif
+BEGIN_FTR_SECTION
+	lwarx	r7,0,r1
+END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
 	stwcx.	r0,0,r1			/* to clear the reservation */
 	lwz	r4,_LINK(r1)
 	lwz	r5,_CCR(r1)
@@ -690,6 +693,9 @@ restore:
 	mtctr	r11
 
 	PPC405_ERR77(0,r1)
+BEGIN_FTR_SECTION
+	lwarx	r11,0,r1
+END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
 	stwcx.	r0,0,r1			/* to clear the reservation */
 
 #if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
diff --git a/include/asm-powerpc/cputable.h b/include/asm-powerpc/cputable.h
index 9d74338..4525c78 100644
--- a/include/asm-powerpc/cputable.h
+++ b/include/asm-powerpc/cputable.h
@@ -138,6 +138,7 @@ extern void do_feature_fixups(unsigned long value, void *fixup_start,
 #define CPU_FTR_FPU_UNAVAILABLE		ASM_CONST(0x0000000000800000)
 #define CPU_FTR_UNIFIED_ID_CACHE	ASM_CONST(0x0000000001000000)
 #define CPU_FTR_SPE			ASM_CONST(0x0000000002000000)
+#define CPU_FTR_NEED_PAIRED_STWCX	ASM_CONST(0x0000000004000000)
 
 /*
  * Add the 64-bit processor unique features in the top half of the word;
@@ -261,25 +262,25 @@ extern void do_feature_fixups(unsigned long value, void *fixup_start,
 #define CPU_FTRS_7450_20	(CPU_FTR_COMMON | \
 	    CPU_FTR_USE_TB | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
-	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
+	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_7450_21	(CPU_FTR_COMMON | \
 	    CPU_FTR_USE_TB | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_L3_DISABLE_NAP | \
-	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
+	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_7450_23	(CPU_FTR_COMMON | \
-	    CPU_FTR_USE_TB | \
+	    CPU_FTR_USE_TB | CPU_FTR_NEED_PAIRED_STWCX | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
 #define CPU_FTRS_7455_1	(CPU_FTR_COMMON | \
-	    CPU_FTR_USE_TB | \
+	    CPU_FTR_USE_TB | CPU_FTR_NEED_PAIRED_STWCX | \
 	    CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | CPU_FTR_L3CR | \
 	    CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | CPU_FTR_HAS_HIGH_BATS | \
 	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
 #define CPU_FTRS_7455_20	(CPU_FTR_COMMON | \
-	    CPU_FTR_USE_TB | \
+	    CPU_FTR_USE_TB | CPU_FTR_NEED_PAIRED_STWCX | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_L3_DISABLE_NAP | \
@@ -289,31 +290,32 @@ extern void do_feature_fixups(unsigned long value, void *fixup_start,
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_HAS_HIGH_BATS | \
-	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
+	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_7447_10	(CPU_FTR_COMMON | \
 	    CPU_FTR_USE_TB | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_HAS_HIGH_BATS | \
-	    CPU_FTR_NEED_COHERENT | CPU_FTR_NO_BTIC | CPU_FTR_PPC_LE)
+	    CPU_FTR_NEED_COHERENT | CPU_FTR_NO_BTIC | CPU_FTR_PPC_LE | \
+	    CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_7447	(CPU_FTR_COMMON | \
 	    CPU_FTR_USE_TB | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_L3CR | CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_HAS_HIGH_BATS | \
-	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
+	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_7447A	(CPU_FTR_COMMON | \
 	    CPU_FTR_USE_TB | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_HAS_HIGH_BATS | \
-	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE)
+	    CPU_FTR_NEED_COHERENT | CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_7448	(CPU_FTR_COMMON | \
 	    CPU_FTR_USE_TB | \
 	    CPU_FTR_MAYBE_CAN_NAP | CPU_FTR_L2CR | CPU_FTR_ALTIVEC_COMP | \
 	    CPU_FTR_HPTE_TABLE | CPU_FTR_SPEC7450 | \
 	    CPU_FTR_NAP_DISABLE_L2_PR | CPU_FTR_HAS_HIGH_BATS | \
-	    CPU_FTR_PPC_LE)
+	    CPU_FTR_PPC_LE | CPU_FTR_NEED_PAIRED_STWCX)
 #define CPU_FTRS_82XX	(CPU_FTR_COMMON | \
 	    CPU_FTR_MAYBE_CAN_DOZE | CPU_FTR_USE_TB)
 #define CPU_FTRS_G2_LE	(CPU_FTR_COMMON | CPU_FTR_MAYBE_CAN_DOZE | \
-- 
1.5.3

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] Avoid unpaired stwcx. on some processors
  2007-11-09 22:08 [PATCH] Avoid unpaired stwcx. on some processors Becky Bruce
@ 2007-11-09 23:19 ` Olof Johansson
  2007-11-09 23:52   ` Becky Bruce
  2007-11-11  4:46   ` Benjamin Herrenschmidt
  0 siblings, 2 replies; 7+ messages in thread
From: Olof Johansson @ 2007-11-09 23:19 UTC (permalink / raw)
  To: Becky Bruce; +Cc: linuxppc-dev

On Fri, Nov 09, 2007 at 04:08:06PM -0600, Becky Bruce wrote:
> The context switch code in the kernel issues a dummy stwcx. to clear the
> reservation, as recommended by the architecture. However, some processors
> can have issues if this stwcx to address A occurs while the reservation
> is already held to a different address B.  To avoid this problem, the dummy
> stwcx. needs to be paired with a dummy lwarx to the same address.
> 
> This patch adds the dummy lwarx, and creates a cpu feature bit to indicate
> which cpus are affected. Tested on mpc8641_hpcn_defconfig in arch/powerpc;
> build tested in arch/ppc.

You're still exposed even with this patch. The stwcx is there to protect
from the cases where process 1 does lwarx and get context switched
out to process 2 that by pure random chance does a stwcx. to the same
reservation granule and succeeds, in spite of not having done the lwarx
(on this side of the context switch).

In exactly that case, process 2 will instead of doing a store to a
reservation it didn't take on it's own, do a dangling stwcx, which is
your erratum. Right?

Seems like a "better" (but still ugly) workaround would be to create a
_new_ reservation to a RA that's unavailable to any userspace process,
so that they could never do a successful store to it. That way you would
have stray reservations, but never dangling stwcx:es. No?


-Olof

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Avoid unpaired stwcx. on some processors
  2007-11-09 23:19 ` Olof Johansson
@ 2007-11-09 23:52   ` Becky Bruce
  2007-11-10  0:18     ` Olof Johansson
  2007-11-11  4:46   ` Benjamin Herrenschmidt
  1 sibling, 1 reply; 7+ messages in thread
From: Becky Bruce @ 2007-11-09 23:52 UTC (permalink / raw)
  To: Olof Johansson; +Cc: linuxppc-dev


On Nov 9, 2007, at 5:19 PM, Olof Johansson wrote:

> On Fri, Nov 09, 2007 at 04:08:06PM -0600, Becky Bruce wrote:
>> The context switch code in the kernel issues a dummy stwcx. to  
>> clear the
>> reservation, as recommended by the architecture. However, some  
>> processors
>> can have issues if this stwcx to address A occurs while the  
>> reservation
>> is already held to a different address B.  To avoid this problem,  
>> the dummy
>> stwcx. needs to be paired with a dummy lwarx to the same address.
>>
>> This patch adds the dummy lwarx, and creates a cpu feature bit to  
>> indicate
>> which cpus are affected. Tested on mpc8641_hpcn_defconfig in arch/ 
>> powerpc;
>> build tested in arch/ppc.
>
> You're still exposed even with this patch. The stwcx is there to  
> protect
> from the cases where process 1 does lwarx and get context switched
> out to process 2 that by pure random chance does a stwcx. to the same
> reservation granule and succeeds, in spite of not having done the  
> lwarx
> (on this side of the context switch).

> In exactly that case, process 2 will instead of doing a store to a
> reservation it didn't take on it's own, do a dangling stwcx, which is
> your erratum. Right?

I don't think so. It's not plain old dangling stwcx that's the  
problem.  It's dangling stwcx when the reservation is held to another  
address.  The lwarx that I've added prevents the normally dangling  
stwcx. in the context switch/syscall  path from meeting this  
condition.  Any process that gets swapped in and executes stwcx.  
first thing is fine, because the reservation was previously cleared  
by the stwcx. in the context switch path.

BTW, I think you're missing a key point here, which is this:  
Architecturally, there is a single reservation per core.  On e600 and  
other parts, the stwcx. does *not* take the address into account for  
success.  If you stwcx, and the reservation is held, it succeeds  
regardless of the address.   Fun, no?   That's one of the reasons  
it's so important that the kernel have the dummy stwcx. in place.

Does that make sense?
-B

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Avoid unpaired stwcx. on some processors
  2007-11-09 23:52   ` Becky Bruce
@ 2007-11-10  0:18     ` Olof Johansson
  0 siblings, 0 replies; 7+ messages in thread
From: Olof Johansson @ 2007-11-10  0:18 UTC (permalink / raw)
  To: Becky Bruce; +Cc: linuxppc-dev

On Fri, Nov 09, 2007 at 05:52:30PM -0600, Becky Bruce wrote:

> I don't think so. It's not plain old dangling stwcx that's the  
> problem.  It's dangling stwcx when the reservation is held to another  
> address. 

Gack, I misread the description. My bad.

> The lwarx that I've added prevents the normally dangling  
> stwcx. in the context switch/syscall  path from meeting this  
> condition.  Any process that gets swapped in and executes stwcx.  
> first thing is fine, because the reservation was previously cleared  
> by the stwcx. in the context switch path.
>
> BTW, I think you're missing a key point here, which is this:  
> Architecturally, there is a single reservation per core.  On e600 and  
> other parts, the stwcx. does *not* take the address into account for  
> success.  If you stwcx, and the reservation is held, it succeeds  
> regardless of the address.   Fun, no?   That's one of the reasons  
> it's so important that the kernel have the dummy stwcx. in place.
>
> Does that make sense?

Yep, it does, doesn't seem to be a better way to work around it.


-Olof

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Avoid unpaired stwcx. on some processors
  2007-11-09 23:19 ` Olof Johansson
  2007-11-09 23:52   ` Becky Bruce
@ 2007-11-11  4:46   ` Benjamin Herrenschmidt
  2007-11-11  7:14     ` Olof Johansson
  1 sibling, 1 reply; 7+ messages in thread
From: Benjamin Herrenschmidt @ 2007-11-11  4:46 UTC (permalink / raw)
  To: Olof Johansson; +Cc: linuxppc-dev


> Seems like a "better" (but still ugly) workaround would be to create a
> _new_ reservation to a RA that's unavailable to any userspace process,
> so that they could never do a successful store to it. That way you would
> have stray reservations, but never dangling stwcx:es. No?

Many processors don't compare the reservation address locally. If
there's any valid reservation held by that processor, a subsequent
stwcx. will always succeed. That would make you scheme dangerous :-)

Ben.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Avoid unpaired stwcx. on some processors
  2007-11-11  4:46   ` Benjamin Herrenschmidt
@ 2007-11-11  7:14     ` Olof Johansson
  0 siblings, 0 replies; 7+ messages in thread
From: Olof Johansson @ 2007-11-11  7:14 UTC (permalink / raw)
  To: Benjamin Herrenschmidt; +Cc: linuxppc-dev

On Sun, Nov 11, 2007 at 03:46:05PM +1100, Benjamin Herrenschmidt wrote:
> 
> > Seems like a "better" (but still ugly) workaround would be to create a
> > _new_ reservation to a RA that's unavailable to any userspace process,
> > so that they could never do a successful store to it. That way you would
> > have stray reservations, but never dangling stwcx:es. No?
> 
> Many processors don't compare the reservation address locally. If
> there's any valid reservation held by that processor, a subsequent
> stwcx. will always succeed. That would make you scheme dangerous :-)

Yeah, I had missed that arch detail. Becky already explained it in her
reply, but thanks for doing it again. :)


-Olof

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2007-11-11  7:14 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-11-09 22:08 [PATCH] Avoid unpaired stwcx. on some processors Becky Bruce
2007-11-09 23:19 ` Olof Johansson
2007-11-09 23:52   ` Becky Bruce
2007-11-10  0:18     ` Olof Johansson
2007-11-11  4:46   ` Benjamin Herrenschmidt
2007-11-11  7:14     ` Olof Johansson
  -- strict thread matches above, loose matches on Subject: below --
2007-11-09 22:17 Becky Bruce

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