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
