[PATCH v8 0/2] MIPS: Override barrier_before_unreachable() to fix microMIPS

[PATCH v8 0/2] MIPS: Override barrier_before_unreachable() to fix microMIPS

[Paul Burton]

This series overrides barrier_before_unreachable() for MIPS to add a
.insn assembler directive.

Due to the subsequent __builtin_unreachable(), the assembler can't tell
that a label on the empty inline asm is code rather than data, so any
microMIPS branches targeting it (which sadly can't be removed) raise
errors due to the mismatching ISA mode, Adding the .insn in patch 2
tells the assembler that it should be treated as code.

Applies cleanly atop v4.18.

Changes in v8 (Paul):
- Try something different... including a header that might be an
  asm-generic wrapper in linux/compiler_types.h creates build ordering
  problems for any C file which can be built before the asm-generic
  target. Patch 1 changes tact to avoid asm-generic & the ordering
  problem.
- Commit message improvements in patch 2.

Changes in v7 (Paul):
- Elaborate on affected GCC versions in patch 4.

Changes in v6 (Paul):
- Fix patch 2 to find the generated headers in $(objtree).
- Remove CC's for defunct MIPS email addresses (Matthew & Robert).
- CC linux-um@lists.infradead.org.

Changes in v5 (Paul):
- Rebase atop v4.18-rc8.
- Comment & commit message tweaks.
- Add SPDX-License-Identifier to asm-generic/compiler.h.

Changes in v4 (James):
- Fix asm-generic/compiler.h include from check, compiler_types.h is
  included on the command line so linux/compiler.h may not be included
  (kbuild test robot).
- New patch 2 to fix um (kbuild test robot).

Changes in v3 (James):
- New patch 1.
- Rebase after 4.17 arch removal and update commit messages.
- Use asm/compiler.h instead of compiler-gcc.h (Arnd).
- Drop stable tag for now.

Changes in v2 (Paul):
- Add generic-y entries to arch Kbuild files. Oops!

Previous versions:
v1: https://www.linux-mips.org/archives/linux-mips/2016-05/msg00399.html
v2: https://www.linux-mips.org/archives/linux-mips/2016-05/msg00401.html
v3: https://lkml.org/lkml/2018/4/17/228
v4: https://www.linux-mips.org/archives/linux-mips/2018-05/msg00069.html
v5: https://www.spinics.net/lists/mips/msg74408.html
v6: https://www.spinics.net/lists/mips/msg74425.html
v7: https://www.spinics.net/lists/linux-arch/msg47934.html

Older #ifdef-based attempt:
https://marc.info/?l=linux-mips&m=145555921408274&w=2

Paul Burton (2):
  kbuild: Allow asm-specific compiler_types.h
  MIPS: Workaround GCC __builtin_unreachable reordering bug

 arch/mips/include/asm/compiler_types.h | 39 ++++++++++++++++++++++++++
 include/linux/compiler-gcc.h           |  2 ++
 scripts/Makefile.lib                   |  5 +++-
 3 files changed, 45 insertions(+), 1 deletion(-)
 create mode 100644 arch/mips/include/asm/compiler_types.h

-- 
2.18.0

[PATCH v8 1/2] kbuild: Allow asm-specific compiler_types.h

[Paul Burton]

We have a need to override the definition of
barrier_before_unreachable() for MIPS, which means we either need to add
architecture-specific code into linux/compiler-gcc.h or we need to allow
the architecture to provide a header that can define the macro before
the generic definition. The latter seems like the better approach.

A straightforward approach to the per-arch header is to make use of
asm-generic to provide a default empty header & adjust architectures
which don't need anything specific to make use of that by adding the
header to generic-y. Unfortunately this doesn't work so well due to
commit a95b37e20db9 ("kbuild: get <linux/compiler_types.h> out of
<linux/kconfig.h>") which moved the inclusion of linux/compiler.h to
cflags using the -include compiler flag.

Because the -include flag is present for all C files we compile, we need
the architecture-provided header to be present before any C files are
compiled. If any C files can be compiled prior to the asm-generic header
wrappers being generated then we hit a build failure due to missing
header. Such cases do exist - one pointed out by the kbuild test robot
is the compilation of arch/ia64/kernel/nr-irqs.c, which occurs as part
of the archprepare target [1].

This leaves us with a few options:

  1) Use generic-y & fix any build failures we find by enforcing
     ordering such that the asm-generic target occurs before any C
     compilation, such that linux/compiler_types.h can always include
     the generated asm-generic wrapper which in turn includes the empty
     asm-generic header. This would rely on us finding all the
     problematic cases - I don't know for sure that the ia64 issue is
     the only one.

  2) Add an actual empty header to each architecture, so that we don't
     need the generated asm-generic wrapper. This seems messy.

  3) Give up & add #ifdef CONFIG_MIPS or similar to
     linux/compiler_types.h. This seems messy too.

  4) Include the arch header only when it's actually needed, removing
     the need for the asm-generic wrapper for all other architectures.

This patch allows us to use approach 4, by including an
asm/compiler_types.h header using the -include flag in the same way we
do for linux/compiler_types.h, but only if the header actually exists.

[1] https://lists.01.org/pipermail/kbuild-all/2018-August/051175.html

Signed-off-by: Paul Burton <paul.burton@mips.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: James Hogan <jhogan@kernel.org>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-arch@vger.kernel.org
Cc: linux-kbuild@vger.kernel.org
Cc: linux-mips@linux-mips.org

---
Any thoughts anyone?

This isn't the prettiest it could possibly be but it's a small change &
clearly shouldn't break anything, which are good qualities for a patch
fixing build failures that we'd ideally backport as far as 4.16.

Changes in v8:
- New patch.

 scripts/Makefile.lib | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 1bb594fcfe12..4e7b41ef029b 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -151,8 +151,11 @@ __a_flags	= $(call flags,_a_flags)
 __cpp_flags     = $(call flags,_cpp_flags)
 endif
 
+c_includes	= $(wildcard $(srctree)/arch/$(SRCARCH)/include/asm/compiler_types.h)
+c_includes	+= $(srctree)/include/linux/compiler_types.h
+
 c_flags        = -Wp,-MD,$(depfile) $(NOSTDINC_FLAGS) $(LINUXINCLUDE)     \
-		 -include $(srctree)/include/linux/compiler_types.h       \
+		 $(addprefix -include ,$(c_includes))                     \
 		 $(__c_flags) $(modkern_cflags)                           \
 		 $(basename_flags) $(modname_flags)
 
-- 
2.18.0

[PATCH v8 2/2] MIPS: Workaround GCC __builtin_unreachable reordering bug

[Paul Burton]

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.

Signed-off-by: Paul Burton <paul.burton@mips.com>
Fixes: 173a3efd3edb ("bug.h: work around GCC PR82365 in BUG()")
Cc: James Hogan <jhogan@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-mips@linux-mips.org

---

Changes in v8:
- Move to asm/compiler_types.h to suit patch 1.
- Commit message improvements.
- Drop James' SoB since this changed a fair bit since he added it.

Changes in v7:
- Elaborate on affected GCC versions in comment.

Changes in v6: None

Changes in v5:
- Comment & commit message tweaks.

Changes in v4: None

Changes in v3:
- Forward port to v4.17-rc and update commit message.
- Drop stable tag for now.

Changes in v2:
- Remove generic-y entry.

 arch/mips/include/asm/compiler_types.h | 39 ++++++++++++++++++++++++++
 include/linux/compiler-gcc.h           |  2 ++
 2 files changed, 41 insertions(+)
 create mode 100644 arch/mips/include/asm/compiler_types.h

diff --git a/arch/mips/include/asm/compiler_types.h b/arch/mips/include/asm/compiler_types.h
new file mode 100644
index 000000000000..cecd5dc48ce2
--- /dev/null
+++ b/arch/mips/include/asm/compiler_types.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __ASM_COMPILER_TYPES_H__
+#define __ASM_COMPILER_TYPES_H__
+
+/*
+ * With GCC 4.5 onwards we can use __builtin_unreachable to indicate to the
+ * compiler that a particular code path will never be hit. This allows it to be
+ * optimised out of the generated binary.
+ *
+ * Unfortunately at least GCC 4.6.3 through 7.3.0 inclusive suffer from a bug
+ * that can lead to instructions from beyond an unreachable statement being
+ * incorrectly reordered into earlier 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. See this potential GCC fix for details:
+ *
+ *   https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00360.html
+ *
+ * It is unclear whether GCC 8 onwards suffer from the same issue - nothing
+ * relevant is mentioned in GCC 8 release notes and nothing obviously relevant
+ * stands out in GCC commit logs, but these newer GCC versions generate very
+ * different code for the testcase which doesn't exhibit the bug.
+ *
+ * GCC also handles stack allocation suboptimally when calling noreturn
+ * functions or calling __builtin_unreachable():
+ *
+ *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
+ *
+ * We work around both of these issues by placing a volatile asm statement,
+ * which GCC is prevented from reordering past, prior to __builtin_unreachable
+ * calls.
+ *
+ * The .insn statement is required to ensure that any branches to the
+ * statement, which sadly must be kept due to the asm statement, are known to
+ * be branches to code and satisfy linker requirements for microMIPS kernels.
+ */
+#define barrier_before_unreachable() asm volatile(".insn")
+
+#endif /* __ASM_COMPILER_TYPES_H__ */
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index f1a7492a5cc8..354d40f7bf80 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -218,7 +218,9 @@
  *
  * Adding an empty inline assembly before it works around the problem
  */
+#ifndef barrier_before_unreachable
 #define barrier_before_unreachable() asm volatile("")
+#endif
 
 /*
  * Mark a position in code as unreachable.  This can be used to
-- 
2.18.0

Re: [PATCH v8 1/2] kbuild: Allow asm-specific compiler_types.h

[Masahiro Yamada]

Hi Paul,


2018-08-19 3:10 GMT+09:00 Paul Burton <paul.burton@mips.com>:
> We have a need to override the definition of
> barrier_before_unreachable() for MIPS, which means we either need to add
> architecture-specific code into linux/compiler-gcc.h or we need to allow
> the architecture to provide a header that can define the macro before
> the generic definition. The latter seems like the better approach.
>
> A straightforward approach to the per-arch header is to make use of
> asm-generic to provide a default empty header & adjust architectures
> which don't need anything specific to make use of that by adding the
> header to generic-y. Unfortunately this doesn't work so well due to
> commit a95b37e20db9 ("kbuild: get <linux/compiler_types.h> out of
> <linux/kconfig.h>") which moved the inclusion of linux/compiler.h to
> cflags using the -include compiler flag.
>
> Because the -include flag is present for all C files we compile, we need
> the architecture-provided header to be present before any C files are
> compiled. If any C files can be compiled prior to the asm-generic header
> wrappers being generated then we hit a build failure due to missing
> header. Such cases do exist - one pointed out by the kbuild test robot
> is the compilation of arch/ia64/kernel/nr-irqs.c, which occurs as part
> of the archprepare target [1].
>
> This leaves us with a few options:
>
>   1) Use generic-y & fix any build failures we find by enforcing
>      ordering such that the asm-generic target occurs before any C
>      compilation, such that linux/compiler_types.h can always include
>      the generated asm-generic wrapper which in turn includes the empty
>      asm-generic header. This would rely on us finding all the
>      problematic cases - I don't know for sure that the ia64 issue is
>      the only one.
>
>   2) Add an actual empty header to each architecture, so that we don't
>      need the generated asm-generic wrapper. This seems messy.
>
>   3) Give up & add #ifdef CONFIG_MIPS or similar to
>      linux/compiler_types.h. This seems messy too.
>
>   4) Include the arch header only when it's actually needed, removing
>      the need for the asm-generic wrapper for all other architectures.
>
> This patch allows us to use approach 4, by including an
> asm/compiler_types.h header using the -include flag in the same way we
> do for linux/compiler_types.h, but only if the header actually exists.


I agree with the approach 4),
but I am of two minds about how to implement it.


I guess the cost of evaluating 'wildcard' for each C file
is unnoticeable level, but I am slightly in favor of
including <asm/compilr_types.h> from <linux/compiler_types.h>
conditionally.

I am not sure about the CONFIG name, but for example, like this.

#ifdef CONFIG_HAVE_ARCH_COMPILER_TYPES
#include <asm/compiler_types.h>
#endif


What do you think?






> [1] https://lists.01.org/pipermail/kbuild-all/2018-August/051175.html
>
> Signed-off-by: Paul Burton <paul.burton@mips.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: James Hogan <jhogan@kernel.org>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-arch@vger.kernel.org
> Cc: linux-kbuild@vger.kernel.org
> Cc: linux-mips@linux-mips.org
>
> ---
> Any thoughts anyone?
>
> This isn't the prettiest it could possibly be but it's a small change &
> clearly shouldn't break anything, which are good qualities for a patch
> fixing build failures that we'd ideally backport as far as 4.16.
>
> Changes in v8:
> - New patch.
>
>  scripts/Makefile.lib | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 1bb594fcfe12..4e7b41ef029b 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -151,8 +151,11 @@ __a_flags  = $(call flags,_a_flags)
>  __cpp_flags     = $(call flags,_cpp_flags)
>  endif
>
> +c_includes     = $(wildcard $(srctree)/arch/$(SRCARCH)/include/asm/compiler_types.h)
> +c_includes     += $(srctree)/include/linux/compiler_types.h
> +
>  c_flags        = -Wp,-MD,$(depfile) $(NOSTDINC_FLAGS) $(LINUXINCLUDE)     \
> -                -include $(srctree)/include/linux/compiler_types.h       \
> +                $(addprefix -include ,$(c_includes))                     \
>                  $(__c_flags) $(modkern_cflags)                           \
>                  $(basename_flags) $(modname_flags)
>
> --
> 2.18.0
>



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v8 1/2] kbuild: Allow asm-specific compiler_types.h

[Paul Burton]

Hi Masahiro,

On Mon, Aug 20, 2018 at 02:04:24PM +0900, Masahiro Yamada wrote:
> 2018-08-19 3:10 GMT+09:00 Paul Burton <paul.burton@mips.com>:
> > We have a need to override the definition of
> > barrier_before_unreachable() for MIPS, which means we either need to add
> > architecture-specific code into linux/compiler-gcc.h or we need to allow
> > the architecture to provide a header that can define the macro before
> > the generic definition. The latter seems like the better approach.
> >
> > A straightforward approach to the per-arch header is to make use of
> > asm-generic to provide a default empty header & adjust architectures
> > which don't need anything specific to make use of that by adding the
> > header to generic-y. Unfortunately this doesn't work so well due to
> > commit a95b37e20db9 ("kbuild: get <linux/compiler_types.h> out of
> > <linux/kconfig.h>") which moved the inclusion of linux/compiler.h to
> > cflags using the -include compiler flag.
> >
> > Because the -include flag is present for all C files we compile, we need
> > the architecture-provided header to be present before any C files are
> > compiled. If any C files can be compiled prior to the asm-generic header
> > wrappers being generated then we hit a build failure due to missing
> > header. Such cases do exist - one pointed out by the kbuild test robot
> > is the compilation of arch/ia64/kernel/nr-irqs.c, which occurs as part
> > of the archprepare target [1].
> >
> > This leaves us with a few options:
> >
> >   1) Use generic-y & fix any build failures we find by enforcing
> >      ordering such that the asm-generic target occurs before any C
> >      compilation, such that linux/compiler_types.h can always include
> >      the generated asm-generic wrapper which in turn includes the empty
> >      asm-generic header. This would rely on us finding all the
> >      problematic cases - I don't know for sure that the ia64 issue is
> >      the only one.
> >
> >   2) Add an actual empty header to each architecture, so that we don't
> >      need the generated asm-generic wrapper. This seems messy.
> >
> >   3) Give up & add #ifdef CONFIG_MIPS or similar to
> >      linux/compiler_types.h. This seems messy too.
> >
> >   4) Include the arch header only when it's actually needed, removing
> >      the need for the asm-generic wrapper for all other architectures.
> >
> > This patch allows us to use approach 4, by including an
> > asm/compiler_types.h header using the -include flag in the same way we
> > do for linux/compiler_types.h, but only if the header actually exists.
> 
> I agree with the approach 4),
> but I am of two minds about how to implement it.
> 
> I guess the cost of evaluating 'wildcard' for each C file
> is unnoticeable level, but I am slightly in favor of
> including <asm/compilr_types.h> from <linux/compiler_types.h>
> conditionally.
> 
> I am not sure about the CONFIG name, but for example, like this.
> 
> #ifdef CONFIG_HAVE_ARCH_COMPILER_TYPES
> #include <asm/compiler_types.h>
> #endif
> 
> What do you think?

That sounds fine to me - I'll submit a v9 shortly :)

Thanks,
    Paul

[PATCH v9 0/2] MIPS: Override barrier_before_unreachable() to fix microMIPS

[Paul Burton]

This series overrides barrier_before_unreachable() for MIPS to add a
.insn assembler directive.

Due to the subsequent __builtin_unreachable(), the assembler can't tell
that a label on the empty inline asm is code rather than data, so any
microMIPS branches targeting it (which sadly can't be removed) raise
errors due to the mismatching ISA mode, Adding the .insn in patch 2
tells the assembler that it should be treated as code.

Applies cleanly atop v4.18.

Changes in v9 (Paul):
- Use a simple #include conditional upon a Kconfig symbol, as suggested
  by Masahiro.
- Move back to asm/compiler.h instead of asm/compiler_types.h.

Changes in v8 (Paul):
- Try something different... including a header that might be an
  asm-generic wrapper in linux/compiler_types.h creates build ordering
  problems for any C file which can be built before the asm-generic
  target. Patch 1 changes tact to avoid asm-generic & the ordering
  problem.
- Commit message improvements in patch 2.

Changes in v7 (Paul):
- Elaborate on affected GCC versions in patch 4.

Changes in v6 (Paul):
- Fix patch 2 to find the generated headers in $(objtree).
- Remove CC's for defunct MIPS email addresses (Matthew & Robert).
- CC linux-um@lists.infradead.org.

Changes in v5 (Paul):
- Rebase atop v4.18-rc8.
- Comment & commit message tweaks.
- Add SPDX-License-Identifier to asm-generic/compiler.h.

Changes in v4 (James):
- Fix asm-generic/compiler.h include from check, compiler_types.h is
  included on the command line so linux/compiler.h may not be included
  (kbuild test robot).
- New patch 2 to fix um (kbuild test robot).

Changes in v3 (James):
- New patch 1.
- Rebase after 4.17 arch removal and update commit messages.
- Use asm/compiler.h instead of compiler-gcc.h (Arnd).
- Drop stable tag for now.

Changes in v2 (Paul):
- Add generic-y entries to arch Kbuild files. Oops!

Previous versions:
v1: https://www.linux-mips.org/archives/linux-mips/2016-05/msg00399.html
v2: https://www.linux-mips.org/archives/linux-mips/2016-05/msg00401.html
v3: https://lkml.org/lkml/2018/4/17/228
v4: https://www.linux-mips.org/archives/linux-mips/2018-05/msg00069.html
v5: https://www.spinics.net/lists/mips/msg74408.html
v6: https://www.spinics.net/lists/mips/msg74425.html
v7: https://www.spinics.net/lists/linux-arch/msg47934.html
v8: https://www.spinics.net/lists/mips/msg74562.html

Older #ifdef-based attempt:
https://marc.info/?l=linux-mips&m=145555921408274&w=2

Paul Burton (2):
  kbuild: Allow arch-specific asm/compiler.h
  MIPS: Workaround GCC __builtin_unreachable reordering bug

 arch/Kconfig                     |  8 ++++++++
 arch/mips/Kconfig                |  1 +
 arch/mips/include/asm/compiler.h | 35 ++++++++++++++++++++++++++++++++
 include/linux/compiler_types.h   | 12 +++++++++++
 4 files changed, 56 insertions(+)

-- 
2.18.0

[PATCH v9 1/2] kbuild: Allow arch-specific asm/compiler.h

[Paul Burton]

We have a need to override the definition of
barrier_before_unreachable() for MIPS, which means we either need to add
architecture-specific code into linux/compiler-gcc.h or we need to allow
the architecture to provide a header that can define the macro before
the generic definition. The latter seems like the better approach.

A straightforward approach to the per-arch header is to make use of
asm-generic to provide a default empty header & adjust architectures
which don't need anything specific to make use of that by adding the
header to generic-y. Unfortunately this doesn't work so well due to
commit a95b37e20db9 ("kbuild: get <linux/compiler_types.h> out of
<linux/kconfig.h>") which moved the inclusion of linux/compiler.h to
cflags using the -include compiler flag.

Because the -include flag is present for all C files we compile, we need
the architecture-provided header to be present before any C files are
compiled. If any C files can be compiled prior to the asm-generic header
wrappers being generated then we hit a build failure due to missing
header. Such cases do exist - one pointed out by the kbuild test robot
is the compilation of arch/ia64/kernel/nr-irqs.c, which occurs as part
of the archprepare target [1].

This leaves us with a few options:

  1) Use generic-y & fix any build failures we find by enforcing
     ordering such that the asm-generic target occurs before any C
     compilation, such that linux/compiler_types.h can always include
     the generated asm-generic wrapper which in turn includes the empty
     asm-generic header. This would rely on us finding all the
     problematic cases - I don't know for sure that the ia64 issue is
     the only one.

  2) Add an actual empty header to each architecture, so that we don't
     need the generated asm-generic wrapper. This seems messy.

  3) Give up & add #ifdef CONFIG_MIPS or similar to
     linux/compiler_types.h. This seems messy too.

  4) Include the arch header only when it's actually needed, removing
     the need for the asm-generic wrapper for all other architectures.

This patch allows us to use approach 4, by including an asm/compiler.h
header from linux/compiler_types.h after the inclusion of the
compiler-specific linux/compiler-*.h header(s). We do this
conditionally, only when CONFIG_HAVE_ARCH_COMPILER_H is selected, in
order to avoid the need for asm-generic wrappers & the associated build
ordering issue described above. The asm/compiler.h header is included
after the generic linux/compiler-*.h header(s) for consistency with the
way linux/compiler-intel.h & linux/compiler-clang.h are included after
the linux/compiler-gcc.h header that they override.

[1] https://lists.01.org/pipermail/kbuild-all/2018-August/051175.html

Signed-off-by: Paul Burton <paul.burton@mips.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: James Hogan <jhogan@kernel.org>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: linux-arch@vger.kernel.org
Cc: linux-kbuild@vger.kernel.org
Cc: linux-mips@linux-mips.org

---

Changes in v9:
- Use Kconfig & a #include directive as Masahiro suggested.
- Go with asm/compiler.h rather than asm/compiler_types.h as it's really
  definitions from linux/compiler-*.h that we want to override & the
  conditional include means we don't need to worry about existing
  asm/compiler.h headers.
- Tweak subject & commit message to reflect the changes above.

Changes in v8:
- New patch.

 arch/Kconfig                   |  8 ++++++++
 include/linux/compiler_types.h | 12 ++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/arch/Kconfig b/arch/Kconfig
index c6148166a7b4..c0b56b0d86b0 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -841,6 +841,14 @@ config REFCOUNT_FULL
 	  against various use-after-free conditions that can be used in
 	  security flaw exploits.
 
+config HAVE_ARCH_COMPILER_H
+	bool
+	help
+	  An architecture can select this if it provides an
+	  asm/compiler.h header that should be included after
+	  linux/compiler-*.h in order to override macro definitions that those
+	  headers generally provide.
+
 source "kernel/gcov/Kconfig"
 
 source "scripts/gcc-plugins/Kconfig"
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index fbf337933fd8..66239549d240 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -78,6 +78,18 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 #include <linux/compiler-clang.h>
 #endif
 
+/*
+ * Some architectures need to provide custom definitions of macros provided
+ * by linux/compiler-*.h, and can do so using asm/compiler.h. We include that
+ * conditionally rather than using an asm-generic wrapper in order to avoid
+ * build failures if any C compilation, which will include this file via an
+ * -include argument in c_flags, occurs prior to the asm-generic wrappers being
+ * generated.
+ */
+#ifdef CONFIG_HAVE_ARCH_COMPILER_H
+#include <asm/compiler.h>
+#endif
+
 /*
  * Generic compiler-dependent macros required for kernel
  * build go below this comment. Actual compiler/compiler version
-- 
2.18.0

[PATCH v9 2/2] MIPS: Workaround GCC __builtin_unreachable reordering bug

[Paul Burton]

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@mips.com>
Fixes: 173a3efd3edb ("bug.h: work around GCC PR82365 in BUG()")
Cc: James Hogan <jhogan@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-mips@linux-mips.org

---

Changes in v9:
- Move back to asm/compiler.h to match changes in patch 1.
- #undef before #define since we're now including this after
  linux/compiler-gcc.h.

Changes in v8:
- Move to asm/compiler_types.h to suit patch 1.
- Commit message improvements.
- Drop James' SoB since this changed a fair bit since he added it.

Changes in v7:
- Elaborate on affected GCC versions in comment.

Changes in v6: None

Changes in v5:
- Comment & commit message tweaks.

Changes in v4: None

Changes in v3:
- Forward port to v4.17-rc and update commit message.
- Drop stable tag for now.

Changes in v2:
- Remove generic-y entry.

 arch/mips/Kconfig                |  1 +
 arch/mips/include/asm/compiler.h | 35 ++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 2af13b162e5e..35511999156a 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -33,6 +33,7 @@ config MIPS
 	select GENERIC_SMP_IDLE_THREAD
 	select GENERIC_TIME_VSYSCALL
 	select HANDLE_DOMAIN_IRQ
+	select HAVE_ARCH_COMPILER_H
 	select HAVE_ARCH_JUMP_LABEL
 	select HAVE_ARCH_KGDB
 	select HAVE_ARCH_MMAP_RND_BITS if MMU
diff --git a/arch/mips/include/asm/compiler.h b/arch/mips/include/asm/compiler.h
index e081a265f422..cc2eb1b06050 100644
--- a/arch/mips/include/asm/compiler.h
+++ b/arch/mips/include/asm/compiler.h
@@ -8,6 +8,41 @@
 #ifndef _ASM_COMPILER_H
 #define _ASM_COMPILER_H
 
+/*
+ * With GCC 4.5 onwards we can use __builtin_unreachable to indicate to the
+ * compiler that a particular code path will never be hit. This allows it to be
+ * optimised out of the generated binary.
+ *
+ * Unfortunately at least GCC 4.6.3 through 7.3.0 inclusive suffer from a bug
+ * that can lead to instructions from beyond an unreachable statement being
+ * incorrectly reordered into earlier 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. See this potential GCC fix for details:
+ *
+ *   https://gcc.gnu.org/ml/gcc-patches/2015-09/msg00360.html
+ *
+ * It is unclear whether GCC 8 onwards suffer from the same issue - nothing
+ * relevant is mentioned in GCC 8 release notes and nothing obviously relevant
+ * stands out in GCC commit logs, but these newer GCC versions generate very
+ * different code for the testcase which doesn't exhibit the bug.
+ *
+ * GCC also handles stack allocation suboptimally when calling noreturn
+ * functions or calling __builtin_unreachable():
+ *
+ *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
+ *
+ * We work around both of these issues by placing a volatile asm statement,
+ * which GCC is prevented from reordering past, prior to __builtin_unreachable
+ * calls.
+ *
+ * The .insn statement is required to ensure that any branches to the
+ * statement, which sadly must be kept due to the asm statement, are known to
+ * be branches to code and satisfy linker requirements for microMIPS kernels.
+ */
+#undef barrier_before_unreachable
+#define barrier_before_unreachable() asm volatile(".insn")
+
 #if __GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 4)
 #define GCC_IMM_ASM() "n"
 #define GCC_REG_ACCUM "$0"
-- 
2.18.0

Re: [PATCH v9 1/2] kbuild: Allow arch-specific asm/compiler.h

[Masahiro Yamada]

Hi Paul,


The code diff looks good to me.

Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com>


Just comments in the commit description.   See below.


2018-08-21 7:36 GMT+09:00 Paul Burton <paul.burton@mips.com>:
> We have a need to override the definition of
> barrier_before_unreachable() for MIPS, which means we either need to add
> architecture-specific code into linux/compiler-gcc.h or we need to allow
> the architecture to provide a header that can define the macro before
> the generic definition. The latter seems like the better approach.
>
> A straightforward approach to the per-arch header is to make use of
> asm-generic to provide a default empty header & adjust architectures
> which don't need anything specific to make use of that by adding the
> header to generic-y. Unfortunately this doesn't work so well due to
> commit a95b37e20db9 ("kbuild: get <linux/compiler_types.h> out of
> <linux/kconfig.h>") which moved the inclusion of linux/compiler.h to
> cflags using the -include compiler flag.


I doubt this statement.

Commit a95b37e20db9 is not the cause of the problem.

include/linux/kconfig.h is also included
by using the -include compiler flag.


See the top-level Makefile.

USERINCLUDE    := \
                -I$(srctree)/arch/$(SRCARCH)/include/uapi \
                -I$(objtree)/arch/$(SRCARCH)/include/generated/uapi \
                -I$(srctree)/include/uapi \
                -I$(objtree)/include/generated/uapi \
                -include $(srctree)/include/linux/kconfig.h


So, <linux/compiler_types.h> (then, <asm/compiler.h>)
would be also required for all C files
in the archprepare stage regardless of commit a95b37e20db9.


The change happened in commit 28128c61e08e.



One more thing, you are not touching any makefile in this version.

Maybe, you can prefix the subject with "compiler.h:" or something
instead of "kbuild:".





> Because the -include flag is present for all C files we compile, we need
> the architecture-provided header to be present before any C files are
> compiled. If any C files can be compiled prior to the asm-generic header
> wrappers being generated then we hit a build failure due to missing
> header. Such cases do exist - one pointed out by the kbuild test robot
> is the compilation of arch/ia64/kernel/nr-irqs.c, which occurs as part
> of the archprepare target [1].
>
> This leaves us with a few options:
>
>   1) Use generic-y & fix any build failures we find by enforcing
>      ordering such that the asm-generic target occurs before any C
>      compilation, such that linux/compiler_types.h can always include
>      the generated asm-generic wrapper which in turn includes the empty
>      asm-generic header. This would rely on us finding all the
>      problematic cases - I don't know for sure that the ia64 issue is
>      the only one.
>
>   2) Add an actual empty header to each architecture, so that we don't
>      need the generated asm-generic wrapper. This seems messy.
>
>   3) Give up & add #ifdef CONFIG_MIPS or similar to
>      linux/compiler_types.h. This seems messy too.
>
>   4) Include the arch header only when it's actually needed, removing
>      the need for the asm-generic wrapper for all other architectures.
>
> This patch allows us to use approach 4, by including an asm/compiler.h
> header from linux/compiler_types.h after the inclusion of the
> compiler-specific linux/compiler-*.h header(s). We do this
> conditionally, only when CONFIG_HAVE_ARCH_COMPILER_H is selected, in
> order to avoid the need for asm-generic wrappers & the associated build
> ordering issue described above. The asm/compiler.h header is included
> after the generic linux/compiler-*.h header(s) for consistency with the
> way linux/compiler-intel.h & linux/compiler-clang.h are included after
> the linux/compiler-gcc.h header that they override.
>
> [1] https://lists.01.org/pipermail/kbuild-all/2018-August/051175.html
>
> Signed-off-by: Paul Burton <paul.burton@mips.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: James Hogan <jhogan@kernel.org>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: linux-arch@vger.kernel.org
> Cc: linux-kbuild@vger.kernel.org
> Cc: linux-mips@linux-mips.org

Re: [Skiboot] [PATCH] opal/hmi: Wakeup the cpu before reading core_fir

[Nicholas Piggin]

On Mon, 20 Aug 2018 19:36:05 +0530
Vaibhav Jain <vaibhav@linux.ibm.com> wrote:

> When stop state 5 is enabled, reading the core_fir during an HMI can
> result in a xscom read error with xscom_read() returning the
> OPAL_XSCOM_PARTIAL_GOOD error code and core_fir value of all FFs. At
> present this return error code is not handled in decode_core_fir()
> hence the invalid core_fir value is sent to the kernel where it
> interprets it as a FATAL hmi causing a system check-stop.
> 
> This can be prevented by forcing the core to wake-up using before
> reading the core_fir. Hence this patch wraps the call to
> read_core_fir() within calls to dctl_set_special_wakeup() and
> dctl_clear_special_wakeup().

Would it be feasible to enumerate the ranges of scoms that require
special wakeup and check for those in xscom_read/write, and warn if
spwkup was not set?

Thanks,
Nick

> 
> Suggested-by: Michael Neuling <mikey@neuling.org>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> Signed-off-by: Mahesh J Salgaonkar <mahesh@linux.vnet.ibm.com>
> ---
>  core/hmi.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/core/hmi.c b/core/hmi.c
> index 1f665a2f..67c520a0 100644
> --- a/core/hmi.c
> +++ b/core/hmi.c
> @@ -379,7 +379,7 @@ static bool decode_core_fir(struct cpu_thread *cpu,
>  {
>  	uint64_t core_fir;
>  	uint32_t core_id;
> -	int i;
> +	int i, swkup_rc = OPAL_UNSUPPORTED;
>  	bool found = false;
>  	int64_t ret;
>  	const char *loc;
> @@ -390,14 +390,15 @@ static bool decode_core_fir(struct cpu_thread *cpu,
>  
>  	core_id = pir_to_core_id(cpu->pir);
>  
> +	/* Force the core to wakeup, otherwise reading core_fir is unrealiable
> +	 * if stop-state 5 is enabled.
> +	 */
> +	swkup_rc = dctl_set_special_wakeup(cpu);
> +
>  	/* Get CORE FIR register value. */
>  	ret = read_core_fir(cpu->chip_id, core_id, &core_fir);
>  
> -	if (ret == OPAL_HARDWARE) {
> -		prerror("XSCOM error reading CORE FIR\n");
> -		/* If the FIR can't be read, we should checkstop. */
> -		return true;
> -	} else if (ret == OPAL_WRONG_STATE) {
> +	if (ret == OPAL_WRONG_STATE) {
>  		/*
>  		 * CPU is asleep, so it probably didn't cause the checkstop.
>  		 * If no other HMI cause is found a "catchall" checkstop
> @@ -407,11 +408,16 @@ static bool decode_core_fir(struct cpu_thread *cpu,
>  		prlog(PR_DEBUG,
>  		      "FIR read failed, chip %d core %d asleep\n",
>  		      cpu->chip_id, core_id);
> -		return false;
> +		goto out;
> +	} else if (ret != OPAL_SUCCESS) {
> +		prerror("XSCOM error reading CORE FIR\n");
> +		/* If the FIR can't be read, we should checkstop. */
> +		found = true;
> +		goto out;
>  	}
>  
>  	if (!core_fir)
> -		return false;
> +		goto out;
>  
>  	loc = chip_loc_code(cpu->chip_id);
>  	prlog(PR_INFO, "[Loc: %s]: CHIP ID: %x, CORE ID: %x, FIR: %016llx\n",
> @@ -426,6 +432,9 @@ static bool decode_core_fir(struct cpu_thread *cpu,
>  						|= xstop_bits[i].reason;
>  		}
>  	}
> +out:
> +	if (!swkup_rc)
> +		dctl_clear_special_wakeup(cpu);
>  	return found;
>  }
>  

Re: [PATCH v9 1/2] kbuild: Allow arch-specific asm/compiler.h

[Paul Burton]

Hi Masahiro,

On Tue, Aug 21, 2018 at 11:49:48AM +0900, Masahiro Yamada wrote:
> The code diff looks good to me.
> 
> Reviewed-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Thanks :)

> > A straightforward approach to the per-arch header is to make use of
> > asm-generic to provide a default empty header & adjust architectures
> > which don't need anything specific to make use of that by adding the
> > header to generic-y. Unfortunately this doesn't work so well due to
> > commit a95b37e20db9 ("kbuild: get <linux/compiler_types.h> out of
> > <linux/kconfig.h>") which moved the inclusion of linux/compiler.h to
> > cflags using the -include compiler flag.
>
> I doubt this statement.
> 
> Commit a95b37e20db9 is not the cause of the problem.
> 
>%
> 
> The change happened in commit 28128c61e08e.

You're correct - I'll fix that up.

> One more thing, you are not touching any makefile in this version.
> 
> Maybe, you can prefix the subject with "compiler.h:" or something
> instead of "kbuild:".

Will do.

Thanks,
    Paul

Re: [Skiboot] [PATCH] opal/hmi: Wakeup the cpu before reading core_fir

[Vaibhav Jain]

Thanks for reviewing this patch Nick

Nicholas Piggin <npiggin@gmail.com> writes:
>
> Would it be feasible to enumerate the ranges of scoms that require
> special wakeup and check for those in xscom_read/write, and warn if
> spwkup was not set?
>
I think that might be racy (Vaidy please correct if I am wrong) as a
core can change its state when we read its sleep state and when we do
the actual xscom to read the core_fir.

-- 
Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.