Re: CONFIG_OPTIMIZE_INLINING breaks atomic64 test on arm64 - Russell King

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Will Deacon <will.deacon@arm.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Laura Abbott <labbott@redhat.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	Andrew.Murray@arm.com
Subject: Re: CONFIG_OPTIMIZE_INLINING breaks atomic64 test on arm64
Date: Wed, 29 May 2019 12:53:51 +0100	[thread overview]
Message-ID: <20190529115351.k3k5kcwpdopk2tck@shell.armlinux.org.uk> (raw)
In-Reply-To: <20190529112956.GE4485@fuggles.cambridge.arm.com>

On Wed, May 29, 2019 at 12:29:56PM +0100, Will Deacon wrote:
> Hi Laura,
> 
> Thanks for the report -- I've managed to reproduce the issue locally.
> 
> On Tue, May 28, 2019 at 05:42:04PM -0400, Laura Abbott wrote:
> > CONFIG_OPTIMIZE_INLINING is a selectable option on arm64 now. It currently
> > triggers a bug on the CONFIG_ATOMIC64_SELFTEST:
> 
> [...]
> 
> > ffff0000104901d0 <arch_atomic64_dec_if_positive>:
> > ffff0000104901d0:       a9bf7bfd        stp     x29, x30, [sp, #-16]!
> > ffff0000104901d4:       aa0003e1        mov     x1, x0
> > ffff0000104901d8:       910003fd        mov     x29, sp
> > ffff0000104901dc:       9412c9d5        bl      ffff000010942930 <__ll_sc_arch_atomic64_dec_if_positive>
> > ffff0000104901e0:       d503201f        nop
> > ffff0000104901e4:       d503201f        nop
> > ffff0000104901e8:       d503201f        nop
> > ffff0000104901ec:       d503201f        nop
> > ffff0000104901f0:       d503201f        nop
> > ffff0000104901f4:       d503201f        nop
> > ffff0000104901f8:       aa0103e0        mov     x0, x1
> > ffff0000104901fc:       a8c17bfd        ldp     x29, x30, [sp], #16
> > ffff000010490200:       d65f03c0        ret
> > 
> > ...which seems to be coming from this buggy looking code.
> 
> The problem here is that GCC has allocated the '%[ret]' input operand in x1
> for the out-of-line version of the function, despite us using the register
> keyword explicitly:
> 
> static inline long arch_atomic64_dec_if_positive(atomic64_t *v)
> {
> 	register long x0 asm ("x0") = (long)v;
> 
> 	asm volatile(ARM64_LSE_ATOMIC_INSN(
> 	/* LL/SC */
> 	__LL_SC_ATOMIC64(dec_if_positive)
> 	__nops(6),
> 	/* LSE atomics */
> 	"1:	ldr	x30, %[v]\n"
> 	"	subs	%[ret], x30, #1\n"
> 	"	b.lt	2f\n"
> 	"	casal	x30, %[ret], %[v]\n"
> 	"	sub	x30, x30, #1\n"
> 	"	sub	x30, x30, %[ret]\n"
> 	"	cbnz	x30, 1b\n"
> 	"2:")
> 	: [ret] "+&r" (x0), [v] "+Q" (v->counter)
> 	:
> 	: __LL_SC_CLOBBERS, "cc", "memory");
> 
> 	return x0;
> }
> 
> My reading of:
> 
>   https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html#Local-Register-Variables
> 
> is that this should work, although it's fair to say that we are pulling
> a bunch of other tricks in this code.

This is why 32-bit ARM has the __asmeq() macro, after a very similar bug
afflicted that toolchain.  Nico decided it wasn't worth the pain of
debugging these issues, so we added checks to inline assembly that
depends on registers in certain hardware registers.  That's kept us
safe.

I would encourage aarch64 to adopt this approach, especially as you
now have a similar instance of this.  It guarantees that the register
allocation for assembly code is in fact what you expect it to be.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

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