[v2] powerpc/32: Fix unpaired stwcx. on interrupt exit

[PATCH v2] powerpc/32: Fix unpaired stwcx. on interrupt exit

Posted by Christophe Leroy 4 months, 3 weeks ago

Commit b96bae3ae2cb ("powerpc/32: Replace ASM exception exit by C
exception exit from ppc64") erroneouly copied to powerpc/32 the logic
from powerpc/64 based on feature CPU_FTR_STCX_CHECKS_ADDRESS which is
always 0 on powerpc/32.

Re-instate the logic implemented by commit b64f87c16f3c ("[POWERPC]
Avoid unpaired stwcx. on some processors") which is based on
CPU_FTR_NEED_PAIRED_STWCX feature.

Fixes: b96bae3ae2cb ("powerpc/32: Replace ASM exception exit by C exception exit from ppc64")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: Fixed the commit message (Wrong patch reference in the beginning)
---
 arch/powerpc/kernel/entry_32.S | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 61ffd2989e7b..16f8ee6cb2cd 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -275,10 +275,9 @@ interrupt_return:
 	mtspr	SPRN_SRR1,r12
 
 BEGIN_FTR_SECTION
+	lwarx   r0,0,r1
+END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
 	stwcx.	r0,0,r1		/* to clear the reservation */
-FTR_SECTION_ELSE
-	lwarx	r0,0,r1
-ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
 
 	lwz	r3,_CCR(r1)
 	lwz	r4,_LINK(r1)
@@ -319,10 +318,9 @@ ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
 	mtspr	SPRN_SRR1,r12
 
 BEGIN_FTR_SECTION
+	lwarx   r0,0,r1
+END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
 	stwcx.	r0,0,r1		/* to clear the reservation */
-FTR_SECTION_ELSE
-	lwarx	r0,0,r1
-ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
 
 	lwz	r3,_LINK(r1)
 	lwz	r4,_CTR(r1)
-- 
2.49.0

Re: [PATCH v2] powerpc/32: Fix unpaired stwcx. on interrupt exit

Posted by Madhavan Srinivasan 2 months, 2 weeks ago

On Fri, 12 Sep 2025 10:37:34 +0200, Christophe Leroy wrote:
> Commit b96bae3ae2cb ("powerpc/32: Replace ASM exception exit by C
> exception exit from ppc64") erroneouly copied to powerpc/32 the logic
> from powerpc/64 based on feature CPU_FTR_STCX_CHECKS_ADDRESS which is
> always 0 on powerpc/32.
> 
> Re-instate the logic implemented by commit b64f87c16f3c ("[POWERPC]
> Avoid unpaired stwcx. on some processors") which is based on
> CPU_FTR_NEED_PAIRED_STWCX feature.
> 
> [...]

Applied to powerpc/next.

[1/1] powerpc/32: Fix unpaired stwcx. on interrupt exit
      https://git.kernel.org/powerpc/c/10e1c77c3636d815db802ceef588522c2d2d947c

Thanks

Re: [PATCH v2] powerpc/32: Fix unpaired stwcx. on interrupt exit

Posted by Segher Boessenkool 4 months, 3 weeks ago

Hi!

On Fri, Sep 12, 2025 at 10:37:34AM +0200, Christophe Leroy wrote:
>  BEGIN_FTR_SECTION
> +	lwarx   r0,0,r1
> +END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
>  	stwcx.	r0,0,r1		/* to clear the reservation */
> -FTR_SECTION_ELSE
> -	lwarx	r0,0,r1
> -ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)

Hrm.  So this is for V'ger (mpc7450)?  What kind of issues does that
old thing have with unpaired larx/stcx., some performance impact?

The extra "dummy" larx has serious performance impact itself, on most
implementations (also on all newer stuff).

Segher

Re: [PATCH v2] powerpc/32: Fix unpaired stwcx. on interrupt exit

Posted by Christophe Leroy 4 months, 3 weeks ago

Hi Segher,

Le 12/09/2025 à 15:24, Segher Boessenkool a écrit :
> Hi!
> 
> On Fri, Sep 12, 2025 at 10:37:34AM +0200, Christophe Leroy wrote:
>>   BEGIN_FTR_SECTION
>> +	lwarx   r0,0,r1
>> +END_FTR_SECTION_IFSET(CPU_FTR_NEED_PAIRED_STWCX)
>>   	stwcx.	r0,0,r1		/* to clear the reservation */
>> -FTR_SECTION_ELSE
>> -	lwarx	r0,0,r1
>> -ALT_FTR_SECTION_END_IFCLR(CPU_FTR_STCX_CHECKS_ADDRESS)
> 
> Hrm.  So this is for V'ger (mpc7450)?  What kind of issues does that
> old thing have with unpaired larx/stcx., some performance impact?

No idea, The original commit (from 2007) only says "some processors can 
have issues if this stwcx to address A occurs while the reservation is 
already held to a different address B".

> 
> The extra "dummy" larx has serious performance impact itself, on most
> implementations (also on all newer stuff).

To be honnest I don't know. I just discovered I made a mistake when I 
implemented C interrupt exit and this patch is aiming at restoring the 
previous behaviour.

If you think this is pointless then no problem, I can instead just 
remove the impossible case and that's it.

What is odd to begin with is that we have two features that seems to 
address the same problem but in a slightly different way
- CPU_FTR_NEED_PAIRED_STWCX for PPC32
- CPU_FTR_STCX_CHECKS_ADDRESS for PPC64

What do you recommend ?

Christophe