On Monday 25 July 2022 16:54:16 Segher Boessenkool wrote: > On Mon, Jul 25, 2022 at 10:10:09PM +0200, Pali Rohár wrote: > > On Monday 25 July 2022 16:20:49 Christophe Leroy wrote: > > Now I did again clean test with same Debian 10 cross compiler. > > > > $ git clone > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git && cd > > linux > > $ git checkout v5.15 > > $ make mpc85xx_smp_defconfig ARCH=powerpc > > CROSS_COMPILE=powerpc-linux-gnuspe- > > $ make vmlinux ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnuspe- > > $ cp -a vmlinux vmlinux.v5.15 > > $ git revert 9401f4e46cf6965e23738f70e149172344a01eef > > $ make vmlinux ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnuspe- > > $ cp -a vmlinux vmlinux.revert > > $ powerpc-linux-gnuspe-objdump -d vmlinux.revert > vmlinux.revert.dump > > $ powerpc-linux-gnuspe-objdump -d vmlinux.v5.15 > vmlinux.v5.15.dump > > $ diff -Naurp vmlinux.v5.15.dump vmlinux.revert.dump > > > > And there are: > > > > -c000c304: 7d 20 f8 29 lwarx r9,0,r31,1 > > +c000c304: 7d 20 f8 28 lwarx r9,0,r31 > > > > I guess it must be reproducible this issue as I'm using regular > > toolchain from distribution. > > The kernel had > > #define PPC_RAW_LWARX(t, a, b, eh) (0x7c000028 | ___PPC_RT(t) | > ___PPC_RA(a) | ___PPC_RB(b) | __PPC_EH(eh)) > > and > > #define PPC_LWARX(t, a, b, eh) stringify_in_c(.long PPC_RAW_LWARX(t, a, b, > eh)) > > and > > #ifdef CONFIG_PPC64 > #define __PPC_EH(eh) (((eh) & 0x1) << 0) > #else > #define __PPC_EH(eh) 0 > #endif > > but Christophe's 9401f4e46cf6 changed > > -"1: " PPC_LWARX(%0,0,%2,1) "\n\ > +"1: lwarx %0,0,%2,1\n\ > > no longer checking CONFIG_PPC64. That appears to be the bug.
Nice catch! Now I have tried to apply following change on master (without reverting anything) diff --git a/arch/powerpc/include/asm/simple_spinlock.h b/arch/powerpc/include/asm/simple_spinlock.h index 7ae6aeef8464..72d3657fd2f7 100644 --- a/arch/powerpc/include/asm/simple_spinlock.h +++ b/arch/powerpc/include/asm/simple_spinlock.h @@ -51,7 +51,7 @@ static inline unsigned long __arch_spin_trylock(arch_spinlock_t *lock) token = LOCK_TOKEN; __asm__ __volatile__( -"1: lwarx %0,0,%2,1\n\ +"1: lwarx %0,0,%2,0\n\ cmpwi 0,%0,0\n\ bne- 2f\n\ stwcx. %1,0,%2\n\ @@ -158,7 +158,7 @@ static inline long __arch_read_trylock(arch_rwlock_t *rw) long tmp; __asm__ __volatile__( -"1: lwarx %0,0,%1,1\n" +"1: lwarx %0,0,%1,0\n" __DO_SIGN_EXTEND " addic. %0,%0,1\n\ ble- 2f\n" @@ -182,7 +182,7 @@ static inline long __arch_write_trylock(arch_rwlock_t *rw) token = WRLOCK_TOKEN; __asm__ __volatile__( -"1: lwarx %0,0,%2,1\n\ +"1: lwarx %0,0,%2,0\n\ cmpwi 0,%0,0\n\ bne- 2f\n" " stwcx. %1,0,%2\n\ and with this change, objdump showed exactly same result as if I revert that problematic commit on top of master branch. I guess that simple_spinlock.h should be fixed to pass 1 to lwarx for CONFIG_PPC64 and 0 otherwise. Christophe, are you going to look at it? > The EH field in larx insns is new since ISA 2.05, and some ISA 1.x cpu > implementations actually raise an illegal insn exception on EH=1. It > appears P2020 is one of those. > > > Segher P2020 has e500 cores. e500 cores uses ISA 2.03. So this may be reason. But in official Freescale/NXP documentation for e500 is documented that lwarx supports also eh=1. Maybe it is not really supported. https://www.nxp.com/files-static/32bit/doc/ref_manual/EREF_RM.pdf (page 562) At least there is NOTE: Some older processors may treat EH=1 as an illegal instruction.