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

[Paul Burton]

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