| Message ID | 20180820223618.22319-2-paul.burton@mips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MIPS: Override barrier_before_unreachable() to fix microMIPS | expand |
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
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; > } >
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
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.
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
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(+)