Re: [PATCH AUTOSEL 4.14 33/46] MIPS: Workaround GCC __builtin_unreachable reordering bug

From: Paul Burton
Date: Thu Oct 25 2018 - 15:53:03 EST


Hi Sasha,

On Thu, Oct 25, 2018 at 10:10:40AM -0400, Sasha Levin wrote:
> From: Paul Burton <paul.burton@xxxxxxxx>
>
> [ Upstream commit 906d441febc0de974b2a6ef848a8f058f3bfada3 ]
>
> Some versions of GCC for the MIPS architecture suffer from a bug which
> can lead to instructions from beyond an unreachable statement being
> incorrectly reordered into earlier branch delay slots if the unreachable
> statement is the only content of a case in a switch statement. This can
> lead to seemingly random behaviour, such as invalid memory accesses from
> incorrectly reordered loads or stores, and link failures on microMIPS
> builds.
>
> See this potential GCC fix for details:
>
> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00360.html
>
> Runtime problems resulting from this bug were initially observed using a
> maltasmvp_defconfig v4.4 kernel built using GCC 4.9.2 (from a Codescape
> SDK 2015.06-05 toolchain), with the result being an address exception
> taken after log messages about the L1 caches (during probe of the L2
> cache):
>
> Initmem setup node 0 [mem 0x0000000080000000-0x000000009fffffff]
> VPE topology {2,2} total 4
> Primary instruction cache 64kB, VIPT, 4-way, linesize 32 bytes.
> Primary data cache 64kB, 4-way, PIPT, no aliases, linesize 32 bytes
> <AdEL exception here>
>
> This is early enough that the kernel exception vectors are not in use,
> so any further output depends upon the bootloader. This is reproducible
> in QEMU where no further output occurs - ie. the system hangs here.
> Given the nature of the bug it may potentially be hit with differing
> symptoms. The bug is known to affect GCC versions as recent as 7.3, and
> it is unclear whether GCC 8 fixed it or just happens not to encounter
> the bug in the testcase found at the link above due to differing
> optimizations.
>
> This bug can be worked around by placing a volatile asm statement, which
> GCC is prevented from reordering past, prior to the
> __builtin_unreachable call.
>
> That was actually done already for other reasons by commit 173a3efd3edb
> ("bug.h: work around GCC PR82365 in BUG()"), but creates problems for
> microMIPS builds due to the lack of a .insn directive. The microMIPS ISA
> allows for interlinking with regular MIPS32 code by repurposing bit 0 of
> the program counter as an ISA mode bit. To switch modes one changes the
> value of this bit in the PC. However typical branch instructions encode
> their offsets as multiples of 2-byte instruction halfwords, which means
> they cannot change ISA mode - this must be done using either an indirect
> branch (a jump-register in MIPS terminology) or a dedicated jalx
> instruction. In order to ensure that regular branches don't attempt to
> target code in a different ISA which they can't actually switch to, the
> linker will check that branch targets are code in the same ISA as the
> branch.
>
> Unfortunately our empty asm volatile statements don't qualify as code,
> and the link for microMIPS builds fails with errors such as:
>
> arch/mips/mm/dma-default.s:3265: Error: branch to a symbol in another ISA mode
> arch/mips/mm/dma-default.s:5027: Error: branch to a symbol in another ISA mode
>
> Resolve this by adding a .insn directive within the asm statement which
> declares that what comes next is code. This may or may not be true,
> since we don't really know what comes next, but as this code is in an
> unreachable path anyway that doesn't matter since we won't execute it.
>
> We do this in asm/compiler.h & select CONFIG_HAVE_ARCH_COMPILER_H in
> order to have this included by linux/compiler_types.h after
> linux/compiler-gcc.h. This will result in asm/compiler.h being included
> in all C compilations via the -include linux/compiler_types.h argument
> in c_flags, which should be harmless.
>
> Signed-off-by: Paul Burton <paul.burton@xxxxxxxx>
> Fixes: 173a3efd3edb ("bug.h: work around GCC PR82365 in BUG()")
> Patchwork: https://patchwork.linux-mips.org/patch/20270/
> Cc: James Hogan <jhogan@xxxxxxxxxx>
> Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: linux-mips@xxxxxxxxxxxxxx
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> ---
> arch/mips/Kconfig | 1 +
> arch/mips/include/asm/compiler.h | 35 ++++++++++++++++++++++++++++++++
> 2 files changed, 36 insertions(+)

In principle I'm fine with backporting this - it does fix broken builds.

It's only going to be of any use though if you also backport commit
04f264d3a8b0 ("compiler.h: Allow arch-specific asm/compiler.h"). I'd
recommend backporting both or neither.

In practice I think it's unlikely anyone will need a microMIPS kernel &
be tied to the particular versions affected by the bug this patch fixed,
so I don't think it's a problem to backport neither.

Thanks,
Paul