* [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON() @ 2019-08-19 13:06 Christophe Leroy 2019-08-19 13:06 ` [PATCH 2/3] powerpc: refactoring BUG/WARN macros Christophe Leroy ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Christophe Leroy @ 2019-08-19 13:06 UTC (permalink / raw) To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, segher, Drew Davenport, Kees Cook, Andrew Morton Cc: linux-kernel, linuxppc-dev __WARN() used to just call __WARN_TAINT(TAINT_WARN) But a call to printk() has been added in the commit identified below to print a "---- cut here ----" line. This change only applies to warnings using __WARN(), which means WARN_ON() where the condition is constant at compile time. For WARN_ON() with a non constant condition, the additional line is not printed. In addition, adding a call to printk() forces GCC to add a stack frame and save volatile registers. Powerpc has been using traps to implement warnings in order to avoid that. So, call __WARN_TAINT(TAINT_WARN) directly instead of using __WARN() in order to restore the previous behaviour. If one day powerpc wants the decorative "---- cut here ----" line, it has to be done in the trap handler, not in the WARN_ON() macro. Fixes: 6b15f678fb7d ("include/asm-generic/bug.h: fix "cut here" for WARN_ON for __WARN_TAINT architectures") Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> --- arch/powerpc/include/asm/bug.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h index fed7e6241349..3928fdaebb71 100644 --- a/arch/powerpc/include/asm/bug.h +++ b/arch/powerpc/include/asm/bug.h @@ -99,7 +99,7 @@ int __ret_warn_on = !!(x); \ if (__builtin_constant_p(__ret_warn_on)) { \ if (__ret_warn_on) \ - __WARN(); \ + __WARN_TAINT(TAINT_WARN); \ } else { \ __asm__ __volatile__( \ "1: "PPC_TLNEI" %4,0\n" \ -- 2.13.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] powerpc: refactoring BUG/WARN macros 2019-08-19 13:06 [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON() Christophe Leroy @ 2019-08-19 13:06 ` Christophe Leroy 2019-11-25 10:46 ` Michael Ellerman 2019-08-19 13:06 ` [PATCH 3/3] powerpc: use __builtin_trap() in " Christophe Leroy 2019-08-19 16:28 ` [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON() Kees Cook 2 siblings, 1 reply; 12+ messages in thread From: Christophe Leroy @ 2019-08-19 13:06 UTC (permalink / raw) To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, segher Cc: linux-kernel, linuxppc-dev BUG(), WARN() and friends are using a similar inline assembly to implement various traps with various flags. Lets refactor via a new BUG_ENTRY() macro. Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> --- arch/powerpc/include/asm/bug.h | 41 +++++++++++++++-------------------------- 1 file changed, 15 insertions(+), 26 deletions(-) diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h index 3928fdaebb71..dbf7da90f507 100644 --- a/arch/powerpc/include/asm/bug.h +++ b/arch/powerpc/include/asm/bug.h @@ -57,6 +57,15 @@ ".previous\n" #endif +#define BUG_ENTRY(insn, flags, ...) \ + __asm__ __volatile__( \ + "1: " insn "\n" \ + _EMIT_BUG_ENTRY \ + : : "i" (__FILE__), "i" (__LINE__), \ + "i" (flags), \ + "i" (sizeof(struct bug_entry)), \ + ##__VA_ARGS__) + /* * BUG_ON() and WARN_ON() do their best to cooperate with compile-time * optimisations. However depending on the complexity of the condition @@ -64,11 +73,7 @@ */ #define BUG() do { \ - __asm__ __volatile__( \ - "1: twi 31,0,0\n" \ - _EMIT_BUG_ENTRY \ - : : "i" (__FILE__), "i" (__LINE__), \ - "i" (0), "i" (sizeof(struct bug_entry))); \ + BUG_ENTRY("twi 31, 0, 0", 0); \ unreachable(); \ } while (0) @@ -77,23 +82,11 @@ if (x) \ BUG(); \ } else { \ - __asm__ __volatile__( \ - "1: "PPC_TLNEI" %4,0\n" \ - _EMIT_BUG_ENTRY \ - : : "i" (__FILE__), "i" (__LINE__), "i" (0), \ - "i" (sizeof(struct bug_entry)), \ - "r" ((__force long)(x))); \ + BUG_ENTRY(PPC_TLNEI " %4, 0", 0, "r" ((__force long)(x))); \ } \ } while (0) -#define __WARN_FLAGS(flags) do { \ - __asm__ __volatile__( \ - "1: twi 31,0,0\n" \ - _EMIT_BUG_ENTRY \ - : : "i" (__FILE__), "i" (__LINE__), \ - "i" (BUGFLAG_WARNING|(flags)), \ - "i" (sizeof(struct bug_entry))); \ -} while (0) +#define __WARN_FLAGS(flags) BUG_ENTRY("twi 31, 0, 0", BUGFLAG_WARNING | (flags)) #define WARN_ON(x) ({ \ int __ret_warn_on = !!(x); \ @@ -101,13 +94,9 @@ if (__ret_warn_on) \ __WARN_TAINT(TAINT_WARN); \ } else { \ - __asm__ __volatile__( \ - "1: "PPC_TLNEI" %4,0\n" \ - _EMIT_BUG_ENTRY \ - : : "i" (__FILE__), "i" (__LINE__), \ - "i" (BUGFLAG_WARNING|BUGFLAG_TAINT(TAINT_WARN)),\ - "i" (sizeof(struct bug_entry)), \ - "r" (__ret_warn_on)); \ + BUG_ENTRY(PPC_TLNEI " %4, 0", \ + BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \ + "r" (__ret_warn_on)); \ } \ unlikely(__ret_warn_on); \ }) -- 2.13.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] powerpc: refactoring BUG/WARN macros 2019-08-19 13:06 ` [PATCH 2/3] powerpc: refactoring BUG/WARN macros Christophe Leroy @ 2019-11-25 10:46 ` Michael Ellerman 0 siblings, 0 replies; 12+ messages in thread From: Michael Ellerman @ 2019-11-25 10:46 UTC (permalink / raw) To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras, segher Cc: linuxppc-dev, linux-kernel On Mon, 2019-08-19 at 13:06:30 UTC, Christophe Leroy wrote: > BUG(), WARN() and friends are using a similar inline > assembly to implement various traps with various flags. > > Lets refactor via a new BUG_ENTRY() macro. > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/43f003bb74b9b27da6e719cfc2f7630f5652665a cheers ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros. 2019-08-19 13:06 [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON() Christophe Leroy 2019-08-19 13:06 ` [PATCH 2/3] powerpc: refactoring BUG/WARN macros Christophe Leroy @ 2019-08-19 13:06 ` Christophe Leroy 2019-08-19 13:23 ` Segher Boessenkool 2019-08-19 16:28 ` [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON() Kees Cook 2 siblings, 1 reply; 12+ messages in thread From: Christophe Leroy @ 2019-08-19 13:06 UTC (permalink / raw) To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, segher Cc: linux-kernel, linuxppc-dev The below exemples of use of WARN_ON() show that the result is sub-optimal in regard of the capabilities of powerpc. void test_warn1(unsigned long long a) { WARN_ON(a); } void test_warn2(unsigned long a) { WARN_ON(a); } void test_warn3(unsigned long a, unsigned long b) { WARN_ON(a < b); } void test_warn4(unsigned long a, unsigned long b) { WARN_ON(!a); } void test_warn5(unsigned long a, unsigned long b) { WARN_ON(!a && b); } 00000000 <test_warn1>: 0: 7c 64 23 78 or r4,r3,r4 4: 31 24 ff ff addic r9,r4,-1 8: 7c 89 21 10 subfe r4,r9,r4 c: 0f 04 00 00 twnei r4,0 10: 4e 80 00 20 blr 00000014 <test_warn2>: 14: 31 23 ff ff addic r9,r3,-1 18: 7c 69 19 10 subfe r3,r9,r3 1c: 0f 03 00 00 twnei r3,0 20: 4e 80 00 20 blr 00000024 <test_warn3>: 24: 7c 84 18 10 subfc r4,r4,r3 28: 7d 29 49 10 subfe r9,r9,r9 2c: 7d 29 00 d0 neg r9,r9 30: 0f 09 00 00 twnei r9,0 34: 4e 80 00 20 blr 00000038 <test_warn4>: 38: 7c 63 00 34 cntlzw r3,r3 3c: 54 63 d9 7e rlwinm r3,r3,27,5,31 40: 0f 03 00 00 twnei r3,0 44: 4e 80 00 20 blr 00000048 <test_warn5>: 48: 2f 83 00 00 cmpwi cr7,r3,0 4c: 39 20 00 00 li r9,0 50: 41 9e 00 0c beq cr7,5c <test_warn5+0x14> 54: 7c 84 00 34 cntlzw r4,r4 58: 54 89 d9 7e rlwinm r9,r4,27,5,31 5c: 0f 09 00 00 twnei r9,0 60: 4e 80 00 20 blr RELOCATION RECORDS FOR [__bug_table]: OFFSET TYPE VALUE 00000000 R_PPC_ADDR32 .text+0x0000000c 0000000c R_PPC_ADDR32 .text+0x0000001c 00000018 R_PPC_ADDR32 .text+0x00000030 00000018 R_PPC_ADDR32 .text+0x00000030 00000024 R_PPC_ADDR32 .text+0x00000040 00000030 R_PPC_ADDR32 .text+0x0000005c Using __builtin_trap() instead of inline assembly of twnei/tdnei provides a far better result: 00000000 <test_warn1>: 0: 7c 64 23 78 or r4,r3,r4 4: 0f 04 00 00 twnei r4,0 8: 4e 80 00 20 blr 0000000c <test_warn2>: c: 0f 03 00 00 twnei r3,0 10: 4e 80 00 20 blr 00000014 <test_warn3>: 14: 7c 43 20 08 twllt r3,r4 18: 4e 80 00 20 blr 0000001c <test_warn4>: 1c: 0c 83 00 00 tweqi r3,0 20: 4e 80 00 20 blr 00000024 <test_warn5>: 24: 2f 83 00 00 cmpwi cr7,r3,0 28: 41 9e 00 08 beq cr7,30 <test_warn5+0xc> 2c: 0c 84 00 00 tweqi r4,0 30: 4e 80 00 20 blr RELOCATION RECORDS FOR [__bug_table]: OFFSET TYPE VALUE 00000000 R_PPC_ADDR32 .text+0x00000004 0000000c R_PPC_ADDR32 .text+0x0000000c 00000018 R_PPC_ADDR32 .text+0x00000014 00000024 R_PPC_ADDR32 .text+0x0000001c 00000030 R_PPC_ADDR32 .text+0x0000002c Note that we keep using an assembly text using "twi 31, 0, 0" for inconditional traps because GCC drops all code after __builtin_trap() when the condition is always true at build time. In addition, this patch also fixes bugs in the BUG_ON(x) macro which unlike WARN_ON(x) uses (x) directly as the condition by forcing it to long instead of using !!(x). This leads to upper part of an unsigned long long being ignored on PPC32 and may produce bugs on PPC64 if (x) is a smaller type like an int. Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> --- arch/powerpc/include/asm/bug.h | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h index dbf7da90f507..a229130ffcf9 100644 --- a/arch/powerpc/include/asm/bug.h +++ b/arch/powerpc/include/asm/bug.h @@ -44,14 +44,14 @@ #ifdef CONFIG_DEBUG_BUGVERBOSE #define _EMIT_BUG_ENTRY \ ".section __bug_table,\"aw\"\n" \ - "2:\t" PPC_LONG "1b, %0\n" \ + "2:\t" PPC_LONG "1b - 4, %0\n" \ "\t.short %1, %2\n" \ ".org 2b+%3\n" \ ".previous\n" #else #define _EMIT_BUG_ENTRY \ ".section __bug_table,\"aw\"\n" \ - "2:\t" PPC_LONG "1b\n" \ + "2:\t" PPC_LONG "1b - 4\n" \ "\t.short %2\n" \ ".org 2b+%3\n" \ ".previous\n" @@ -59,7 +59,8 @@ #define BUG_ENTRY(insn, flags, ...) \ __asm__ __volatile__( \ - "1: " insn "\n" \ + insn "\n" \ + "1:\n" \ _EMIT_BUG_ENTRY \ : : "i" (__FILE__), "i" (__LINE__), \ "i" (flags), \ @@ -82,7 +83,9 @@ if (x) \ BUG(); \ } else { \ - BUG_ENTRY(PPC_TLNEI " %4, 0", 0, "r" ((__force long)(x))); \ + if (x) \ + __builtin_trap(); \ + BUG_ENTRY("", 0); \ } \ } while (0) @@ -94,9 +97,9 @@ if (__ret_warn_on) \ __WARN_TAINT(TAINT_WARN); \ } else { \ - BUG_ENTRY(PPC_TLNEI " %4, 0", \ - BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), \ - "r" (__ret_warn_on)); \ + if (__ret_warn_on) \ + __builtin_trap(); \ + BUG_ENTRY("", BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN)); \ } \ unlikely(__ret_warn_on); \ }) -- 2.13.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros. 2019-08-19 13:06 ` [PATCH 3/3] powerpc: use __builtin_trap() in " Christophe Leroy @ 2019-08-19 13:23 ` Segher Boessenkool 2019-08-19 14:08 ` Christophe Leroy 0 siblings, 1 reply; 12+ messages in thread From: Segher Boessenkool @ 2019-08-19 13:23 UTC (permalink / raw) To: Christophe Leroy Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, linux-kernel, linuxppc-dev On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote: > Note that we keep using an assembly text using "twi 31, 0, 0" for > inconditional traps because GCC drops all code after > __builtin_trap() when the condition is always true at build time. As I said, it can also do this for conditional traps, if it can prove the condition is always true. Can you put the bug table asm *before* the __builtin_trap maybe? That should make it all work fine... If you somehow can tell what machine instruction is that trap, anyway. Segher ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros. 2019-08-19 13:23 ` Segher Boessenkool @ 2019-08-19 14:08 ` Christophe Leroy 2019-08-19 14:37 ` Segher Boessenkool 0 siblings, 1 reply; 12+ messages in thread From: Christophe Leroy @ 2019-08-19 14:08 UTC (permalink / raw) To: Segher Boessenkool Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, linux-kernel, linuxppc-dev Le 19/08/2019 à 15:23, Segher Boessenkool a écrit : > On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote: >> Note that we keep using an assembly text using "twi 31, 0, 0" for >> inconditional traps because GCC drops all code after >> __builtin_trap() when the condition is always true at build time. > > As I said, it can also do this for conditional traps, if it can prove > the condition is always true. But we have another branch for 'always true' and 'always false' using __builtin_constant_p(), which don't use __builtin_trap(). Is there anything wrong with that ?: #define BUG_ON(x) do { \ if (__builtin_constant_p(x)) { \ if (x) \ BUG(); \ } else { \ if (x) \ __builtin_trap(); \ BUG_ENTRY("", 0); \ } \ } while (0) #define WARN_ON(x) ({ \ int __ret_warn_on = !!(x); \ if (__builtin_constant_p(__ret_warn_on)) { \ if (__ret_warn_on) \ __WARN_TAINT(TAINT_WARN); \ } else { \ if (__ret_warn_on) \ __builtin_trap(); \ BUG_ENTRY("", BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN)); \ } \ unlikely(__ret_warn_on); \ }) > > Can you put the bug table asm *before* the __builtin_trap maybe? That > should make it all work fine... If you somehow can tell what machine > instruction is that trap, anyway. And how can I tell that ? When I put it *after*, it always points to the trap instruction. When I put it *before* it usually points on the first instruction used to prepare the registers for the trap condition. Christophe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros. 2019-08-19 14:08 ` Christophe Leroy @ 2019-08-19 14:37 ` Segher Boessenkool 2019-08-19 15:05 ` Christophe Leroy 0 siblings, 1 reply; 12+ messages in thread From: Segher Boessenkool @ 2019-08-19 14:37 UTC (permalink / raw) To: Christophe Leroy Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, linux-kernel, linuxppc-dev On Mon, Aug 19, 2019 at 04:08:43PM +0200, Christophe Leroy wrote: > Le 19/08/2019 à 15:23, Segher Boessenkool a écrit : > >On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote: > >>Note that we keep using an assembly text using "twi 31, 0, 0" for > >>inconditional traps because GCC drops all code after > >>__builtin_trap() when the condition is always true at build time. > > > >As I said, it can also do this for conditional traps, if it can prove > >the condition is always true. > > But we have another branch for 'always true' and 'always false' using > __builtin_constant_p(), which don't use __builtin_trap(). Is there > anything wrong with that ?: The compiler might not realise it is constant when it evaluates the __builtin_constant_p, but only realises it later. As the documentation for the builtin says: A return of 0 does not indicate that the value is _not_ a constant, but merely that GCC cannot prove it is a constant with the specified value of the '-O' option. (and there should be many more and more serious warnings here). > #define BUG_ON(x) do { \ > if (__builtin_constant_p(x)) { \ > if (x) \ > BUG(); \ > } else { \ > if (x) \ > __builtin_trap(); \ > BUG_ENTRY("", 0); \ > } \ > } while (0) I think it may work if you do #define BUG_ON(x) do { \ if (__builtin_constant_p(x)) { \ if (x) \ BUG(); \ } else { \ BUG_ENTRY("", 0); \ if (x) \ __builtin_trap(); \ } \ } while (0) or even just #define BUG_ON(x) do { \ BUG_ENTRY("", 0); \ if (x) \ __builtin_trap(); \ } \ } while (0) if BUG_ENTRY can work for the trap insn *after* it. > >Can you put the bug table asm *before* the __builtin_trap maybe? That > >should make it all work fine... If you somehow can tell what machine > >instruction is that trap, anyway. > > And how can I tell that ? I don't know how BUG_ENTRY works exactly. Segher ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros. 2019-08-19 14:37 ` Segher Boessenkool @ 2019-08-19 15:05 ` Christophe Leroy 2019-08-19 15:45 ` Segher Boessenkool 0 siblings, 1 reply; 12+ messages in thread From: Christophe Leroy @ 2019-08-19 15:05 UTC (permalink / raw) To: Segher Boessenkool Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, linux-kernel, linuxppc-dev Le 19/08/2019 à 16:37, Segher Boessenkool a écrit : > On Mon, Aug 19, 2019 at 04:08:43PM +0200, Christophe Leroy wrote: >> Le 19/08/2019 à 15:23, Segher Boessenkool a écrit : >>> On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote: >>>> Note that we keep using an assembly text using "twi 31, 0, 0" for >>>> inconditional traps because GCC drops all code after >>>> __builtin_trap() when the condition is always true at build time. >>> >>> As I said, it can also do this for conditional traps, if it can prove >>> the condition is always true. >> >> But we have another branch for 'always true' and 'always false' using >> __builtin_constant_p(), which don't use __builtin_trap(). Is there >> anything wrong with that ?: > > The compiler might not realise it is constant when it evaluates the > __builtin_constant_p, but only realises it later. As the documentation > for the builtin says: > A return of 0 does not indicate that the > value is _not_ a constant, but merely that GCC cannot prove it is a > constant with the specified value of the '-O' option. So you mean GCC would not be able to prove that __builtin_constant_p(cond) is always true but it would be able to prove that if (cond) is always true ? And isn't there a away to tell GCC that '__builtin_trap()' is recoverable in our case ? > > (and there should be many more and more serious warnings here). > >> #define BUG_ON(x) do { \ >> if (__builtin_constant_p(x)) { \ >> if (x) \ >> BUG(); \ >> } else { \ >> if (x) \ >> __builtin_trap(); \ >> BUG_ENTRY("", 0); \ >> } \ >> } while (0) > > I think it may work if you do > > #define BUG_ON(x) do { \ > if (__builtin_constant_p(x)) { \ > if (x) \ > BUG(); \ > } else { \ > BUG_ENTRY("", 0); \ > if (x) \ > __builtin_trap(); \ > } \ > } while (0) It doesn't work: void test_bug1(unsigned long long a) { BUG_ON(a); } 00000090 <test_bug1>: 90: 7c 63 23 78 or r3,r3,r4 94: 0f 03 00 00 twnei r3,0 98: 4e 80 00 20 blr RELOCATION RECORDS FOR [__bug_table]: OFFSET TYPE VALUE 00000084 R_PPC_ADDR32 .text+0x00000090 As you see, the relocation in __bug_table points to the 'or' and not to the 'twnei'. > > or even just > > #define BUG_ON(x) do { \ > BUG_ENTRY("", 0); \ > if (x) \ > __builtin_trap(); \ > } \ > } while (0) > > if BUG_ENTRY can work for the trap insn *after* it. > >>> Can you put the bug table asm *before* the __builtin_trap maybe? That >>> should make it all work fine... If you somehow can tell what machine >>> instruction is that trap, anyway. >> >> And how can I tell that ? > > I don't know how BUG_ENTRY works exactly. It's basic, maybe too basic: it adds an inline asm with a label, and adds a .long in the __bug_table section with the address of that label. When putting it after the __builtin_trap(), I changed it to using the address before the one of the label which is always the twxx instruction as far as I can see. #define BUG_ENTRY(insn, flags, ...) \ __asm__ __volatile__( \ "1: " insn "\n" \ ".section __bug_table,\"aw\"\n" \ "2:\t" PPC_LONG "1b, %0\n" \ "\t.short %1, %2\n" \ ".org 2b+%3\n" \ ".previous\n" \ : : "i" (__FILE__), "i" (__LINE__), \ "i" (flags), \ "i" (sizeof(struct bug_entry)), \ ##__VA_ARGS__) Christophe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros. 2019-08-19 15:05 ` Christophe Leroy @ 2019-08-19 15:45 ` Segher Boessenkool 2019-08-23 15:35 ` Christophe Leroy 0 siblings, 1 reply; 12+ messages in thread From: Segher Boessenkool @ 2019-08-19 15:45 UTC (permalink / raw) To: Christophe Leroy Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, linux-kernel, linuxppc-dev On Mon, Aug 19, 2019 at 05:05:46PM +0200, Christophe Leroy wrote: > Le 19/08/2019 à 16:37, Segher Boessenkool a écrit : > >On Mon, Aug 19, 2019 at 04:08:43PM +0200, Christophe Leroy wrote: > >>Le 19/08/2019 à 15:23, Segher Boessenkool a écrit : > >>>On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote: > >>>>Note that we keep using an assembly text using "twi 31, 0, 0" for > >>>>inconditional traps because GCC drops all code after > >>>>__builtin_trap() when the condition is always true at build time. > >>> > >>>As I said, it can also do this for conditional traps, if it can prove > >>>the condition is always true. > >> > >>But we have another branch for 'always true' and 'always false' using > >>__builtin_constant_p(), which don't use __builtin_trap(). Is there > >>anything wrong with that ?: > > > >The compiler might not realise it is constant when it evaluates the > >__builtin_constant_p, but only realises it later. As the documentation > >for the builtin says: > > A return of 0 does not indicate that the > > value is _not_ a constant, but merely that GCC cannot prove it is a > > constant with the specified value of the '-O' option. > > So you mean GCC would not be able to prove that > __builtin_constant_p(cond) is always true but it would be able to prove > that if (cond) is always true ? Not sure what you mean, sorry. > And isn't there a away to tell GCC that '__builtin_trap()' is > recoverable in our case ? No, GCC knows that a trap will never fall through. > >I think it may work if you do > > > >#define BUG_ON(x) do { \ > > if (__builtin_constant_p(x)) { \ > > if (x) \ > > BUG(); \ > > } else { \ > > BUG_ENTRY("", 0); \ > > if (x) \ > > __builtin_trap(); \ > > } \ > >} while (0) > > It doesn't work: You need to make a BUG_ENTRY so that it refers to the *following* trap instruction, if you go this way. > >I don't know how BUG_ENTRY works exactly. > > It's basic, maybe too basic: it adds an inline asm with a label, and > adds a .long in the __bug_table section with the address of that label. > > When putting it after the __builtin_trap(), I changed it to using the > address before the one of the label which is always the twxx instruction > as far as I can see. > > #define BUG_ENTRY(insn, flags, ...) \ > __asm__ __volatile__( \ > "1: " insn "\n" \ > ".section __bug_table,\"aw\"\n" \ > "2:\t" PPC_LONG "1b, %0\n" \ > "\t.short %1, %2\n" \ > ".org 2b+%3\n" \ > ".previous\n" \ > : : "i" (__FILE__), "i" (__LINE__), \ > "i" (flags), \ > "i" (sizeof(struct bug_entry)), \ > ##__VA_ARGS__) #define MY_BUG_ENTRY(lab, flags) \ __asm__ __volatile__( \ ".section __bug_table,\"aw\"\n" \ "2:\t" PPC_LONG "%4, %0\n" \ "\t.short %1, %2\n" \ ".org 2b+%3\n" \ ".previous\n" \ : : "i" (__FILE__), "i" (__LINE__), \ "i" (flags), \ "i" (sizeof(struct bug_entry)), \ "i" (lab)) called as #define BUG_ON(x) do { \ MY_BUG_ENTRY(&&lab, 0); \ lab: if (x) \ __builtin_trap(); \ } while (0) not sure how reliable that works -- *if* it works, I just typed that in without testing or anything -- but hopefully you get the idea. Segher ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros. 2019-08-19 15:45 ` Segher Boessenkool @ 2019-08-23 15:35 ` Christophe Leroy 0 siblings, 0 replies; 12+ messages in thread From: Christophe Leroy @ 2019-08-23 15:35 UTC (permalink / raw) To: Segher Boessenkool Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, linux-kernel, linuxppc-dev On 08/19/2019 03:45 PM, Segher Boessenkool wrote: > On Mon, Aug 19, 2019 at 05:05:46PM +0200, Christophe Leroy wrote: >> Le 19/08/2019 à 16:37, Segher Boessenkool a écrit : >>> On Mon, Aug 19, 2019 at 04:08:43PM +0200, Christophe Leroy wrote: >>>> Le 19/08/2019 à 15:23, Segher Boessenkool a écrit : >>>>> On Mon, Aug 19, 2019 at 01:06:31PM +0000, Christophe Leroy wrote: >>>>>> Note that we keep using an assembly text using "twi 31, 0, 0" for >>>>>> inconditional traps because GCC drops all code after >>>>>> __builtin_trap() when the condition is always true at build time. >>>>> >>>>> As I said, it can also do this for conditional traps, if it can prove >>>>> the condition is always true. >>>> >>>> But we have another branch for 'always true' and 'always false' using >>>> __builtin_constant_p(), which don't use __builtin_trap(). Is there >>>> anything wrong with that ?: >>> >>> The compiler might not realise it is constant when it evaluates the >>> __builtin_constant_p, but only realises it later. As the documentation >>> for the builtin says: >>> A return of 0 does not indicate that the >>> value is _not_ a constant, but merely that GCC cannot prove it is a >>> constant with the specified value of the '-O' option. >> >> So you mean GCC would not be able to prove that >> __builtin_constant_p(cond) is always true but it would be able to prove >> that if (cond) is always true ? > > Not sure what you mean, sorry. > >> And isn't there a away to tell GCC that '__builtin_trap()' is >> recoverable in our case ? > > No, GCC knows that a trap will never fall through. > >>> I think it may work if you do >>> >>> #define BUG_ON(x) do { \ >>> if (__builtin_constant_p(x)) { \ >>> if (x) \ >>> BUG(); \ >>> } else { \ >>> BUG_ENTRY("", 0); \ >>> if (x) \ >>> __builtin_trap(); \ >>> } \ >>> } while (0) >> >> It doesn't work: > > You need to make a BUG_ENTRY so that it refers to the *following* trap > instruction, if you go this way. > >>> I don't know how BUG_ENTRY works exactly. >> >> It's basic, maybe too basic: it adds an inline asm with a label, and >> adds a .long in the __bug_table section with the address of that label. >> >> When putting it after the __builtin_trap(), I changed it to using the >> address before the one of the label which is always the twxx instruction >> as far as I can see. >> >> #define BUG_ENTRY(insn, flags, ...) \ >> __asm__ __volatile__( \ >> "1: " insn "\n" \ >> ".section __bug_table,\"aw\"\n" \ >> "2:\t" PPC_LONG "1b, %0\n" \ >> "\t.short %1, %2\n" \ >> ".org 2b+%3\n" \ >> ".previous\n" \ >> : : "i" (__FILE__), "i" (__LINE__), \ >> "i" (flags), \ >> "i" (sizeof(struct bug_entry)), \ >> ##__VA_ARGS__) > > #define MY_BUG_ENTRY(lab, flags) \ > __asm__ __volatile__( \ > ".section __bug_table,\"aw\"\n" \ > "2:\t" PPC_LONG "%4, %0\n" \ > "\t.short %1, %2\n" \ > ".org 2b+%3\n" \ > ".previous\n" \ > : : "i" (__FILE__), "i" (__LINE__), \ > "i" (flags), \ > "i" (sizeof(struct bug_entry)), \ > "i" (lab)) > > called as > > #define BUG_ON(x) do { \ > MY_BUG_ENTRY(&&lab, 0); \ > lab: if (x) \ > __builtin_trap(); \ > } while (0) > > not sure how reliable that works -- *if* it works, I just typed that in > without testing or anything -- but hopefully you get the idea. > I've not been able to make it work. GCC puts the label (.L2 and .L6) outside of the function, so the instruction preceding the label is blr, not the trap. #define _EMIT_BUG_ENTRY \ ".section __bug_table,\"aw\"\n" \ "2:\t" PPC_LONG "%4, %0\n" \ "\t.short %1, %2\n" \ ".org 2b+%3\n" \ ".previous\n" #define BUG_ENTRY(flags, label) \ __asm__ __volatile__( \ _EMIT_BUG_ENTRY \ : : "i" (__FILE__), "i" (__LINE__), \ "i" (flags), \ "i" (sizeof(struct bug_entry)), \ "i" (label - 4)) #define __recoverable_trap() asm volatile ("twi 31, 0, 0;"); #define __WARN_FLAGS(flags) do { \ __label__ label; \ BUG_ENTRY(BUGFLAG_WARNING | (flags), &&label); \ __recoverable_trap(); \ label: ; \ } while (0) #define WARN_ON(x) ({ \ int __ret_warn_on = !!(x); \ if (__builtin_constant_p(__ret_warn_on)) { \ if (__ret_warn_on) \ __WARN_TAINT(TAINT_WARN); \ } else { \ __label__ label; \ BUG_ENTRY(BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN), &&label); \ if (__ret_warn_on) \ __builtin_trap(); \ label: ; \ } \ unlikely(__ret_warn_on); \ }) void test_warn1(unsigned long long a) { WARN_ON(a); } void test_warn2(unsigned long a) { WARN_ON(a); } 00000000 <test_warn1>: 0: 7c 63 23 78 or r3,r3,r4 4: 0f 03 00 00 twnei r3,0 8: 4e 80 00 20 blr 0000000c <test_warn2>: c: 0f 03 00 00 twnei r3,0 10: 4e 80 00 20 blr RELOCATION RECORDS FOR [__bug_table]: OFFSET TYPE VALUE 00000000 R_PPC_ADDR32 .text+0x00000008 00000004 R_PPC_ADDR32 .rodata.str1.4 0000000c R_PPC_ADDR32 .text+0x00000010 00000010 R_PPC_ADDR32 .rodata.str1.4 .file "test.c" .section ".text" .Ltext0: .align 2 .globl test_warn1 .type test_warn1, @function test_warn1: .LFB598: .file 1 "arch/powerpc/mm/test.c" .loc 1 34 0 .LBB2: .LBB3: .loc 1 35 0 #APP # 35 "arch/powerpc/mm/test.c" 1 .section __bug_table,"aw" 2: .long .L2-4, .LC0 .short 35, 2305 .org 2b+12 .previous # 0 "" 2 #NO_APP or 3,3,4 twnei 3,0 blr .L3: .L2: .LBE3: .LBE2: .LFE598: .size test_warn1, .-test_warn1 .align 2 .globl test_warn2 .type test_warn2, @function test_warn2: .LFB599: .loc 1 39 0 .LBB4: .LBB5: .loc 1 40 0 #APP # 40 "arch/powerpc/mm/test.c" 1 .section __bug_table,"aw" 2: .long .L6-4, .LC0 .short 40, 2305 .org 2b+12 .previous # 0 "" 2 #NO_APP twnei 3,0 blr .L7: .L6: .LBE5: .LBE4: .LFE599: Any idea ? Christophe ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON() 2019-08-19 13:06 [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON() Christophe Leroy 2019-08-19 13:06 ` [PATCH 2/3] powerpc: refactoring BUG/WARN macros Christophe Leroy 2019-08-19 13:06 ` [PATCH 3/3] powerpc: use __builtin_trap() in " Christophe Leroy @ 2019-08-19 16:28 ` Kees Cook 2019-08-19 17:29 ` Clean up cut-here even harder (was Re: [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON()) Kees Cook 2 siblings, 1 reply; 12+ messages in thread From: Kees Cook @ 2019-08-19 16:28 UTC (permalink / raw) To: Christophe Leroy Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, segher, Drew Davenport, Andrew Morton, linux-kernel, linuxppc-dev On Mon, Aug 19, 2019 at 01:06:28PM +0000, Christophe Leroy wrote: > __WARN() used to just call __WARN_TAINT(TAINT_WARN) > > But a call to printk() has been added in the commit identified below > to print a "---- cut here ----" line. > > This change only applies to warnings using __WARN(), which means > WARN_ON() where the condition is constant at compile time. > For WARN_ON() with a non constant condition, the additional line is > not printed. > > In addition, adding a call to printk() forces GCC to add a stack frame > and save volatile registers. Powerpc has been using traps to implement > warnings in order to avoid that. > > So, call __WARN_TAINT(TAINT_WARN) directly instead of using __WARN() > in order to restore the previous behaviour. > > If one day powerpc wants the decorative "---- cut here ----" line, it > has to be done in the trap handler, not in the WARN_ON() macro. > > Fixes: 6b15f678fb7d ("include/asm-generic/bug.h: fix "cut here" for WARN_ON for __WARN_TAINT architectures") > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> Ah! Hmpf. Yeah, that wasn't an intended side-effect of this fix. It seems PPC is not alone in this situation of making this code much noisier. It looks like there needs to be a way to indicate to the trap handler that a message was delivered or not. Perhaps we can add another taint flag? -kees > --- > arch/powerpc/include/asm/bug.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h > index fed7e6241349..3928fdaebb71 100644 > --- a/arch/powerpc/include/asm/bug.h > +++ b/arch/powerpc/include/asm/bug.h > @@ -99,7 +99,7 @@ > int __ret_warn_on = !!(x); \ > if (__builtin_constant_p(__ret_warn_on)) { \ > if (__ret_warn_on) \ > - __WARN(); \ > + __WARN_TAINT(TAINT_WARN); \ > } else { \ > __asm__ __volatile__( \ > "1: "PPC_TLNEI" %4,0\n" \ > -- > 2.13.3 > -- Kees Cook ^ permalink raw reply [flat|nested] 12+ messages in thread
* Clean up cut-here even harder (was Re: [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON()) 2019-08-19 16:28 ` [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON() Kees Cook @ 2019-08-19 17:29 ` Kees Cook 0 siblings, 0 replies; 12+ messages in thread From: Kees Cook @ 2019-08-19 17:29 UTC (permalink / raw) To: Christophe Leroy Cc: Peter Zijlstra, Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, segher, Drew Davenport, Andrew Morton, linux-kernel, linuxppc-dev On Mon, Aug 19, 2019 at 09:28:03AM -0700, Kees Cook wrote: > On Mon, Aug 19, 2019 at 01:06:28PM +0000, Christophe Leroy wrote: > > __WARN() used to just call __WARN_TAINT(TAINT_WARN) > > > > But a call to printk() has been added in the commit identified below > > to print a "---- cut here ----" line. > > > > This change only applies to warnings using __WARN(), which means > > WARN_ON() where the condition is constant at compile time. > > For WARN_ON() with a non constant condition, the additional line is > > not printed. > > > > In addition, adding a call to printk() forces GCC to add a stack frame > > and save volatile registers. Powerpc has been using traps to implement > > warnings in order to avoid that. > > > > So, call __WARN_TAINT(TAINT_WARN) directly instead of using __WARN() > > in order to restore the previous behaviour. > > > > If one day powerpc wants the decorative "---- cut here ----" line, it > > has to be done in the trap handler, not in the WARN_ON() macro. > > > > Fixes: 6b15f678fb7d ("include/asm-generic/bug.h: fix "cut here" for WARN_ON for __WARN_TAINT architectures") > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr> > > Ah! Hmpf. Yeah, that wasn't an intended side-effect of this fix. > > It seems PPC is not alone in this situation of making this code much > noisier. It looks like there needs to be a way to indicate to the trap > handler that a message was delivered or not. Perhaps we can add another > taint flag? I meant "bug flag" here, not taint. Here's a stab at it. This tries to remove redundant defines, and moves the "cut here" up into the slow path explicitly (out of _warn()) and creates a flag so the trap handler can actually detect if things were already reported... Thoughts? diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index aa6c093d9ce9..c2b79878f24c 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -10,6 +10,7 @@ #define BUGFLAG_WARNING (1 << 0) #define BUGFLAG_ONCE (1 << 1) #define BUGFLAG_DONE (1 << 2) +#define BUGFLAG_PRINTK (1 << 3) #define BUGFLAG_TAINT(taint) ((taint) << 8) #define BUG_GET_TAINT(bug) ((bug)->flags >> 8) #endif @@ -62,13 +63,11 @@ struct bug_entry { #endif #ifdef __WARN_FLAGS -#define __WARN_TAINT(taint) __WARN_FLAGS(BUGFLAG_TAINT(taint)) -#define __WARN_ONCE_TAINT(taint) __WARN_FLAGS(BUGFLAG_ONCE|BUGFLAG_TAINT(taint)) - #define WARN_ON_ONCE(condition) ({ \ int __ret_warn_on = !!(condition); \ if (unlikely(__ret_warn_on)) \ - __WARN_ONCE_TAINT(TAINT_WARN); \ + __WARN_FLAGS(BUGFLAG_ONCE | \ + BUGFLAG_TAINT(TAINT_WARN)); \ unlikely(__ret_warn_on); \ }) #endif @@ -89,7 +88,7 @@ struct bug_entry { * * Use the versions with printk format strings to provide better diagnostics. */ -#ifndef __WARN_TAINT +#ifndef __WARN_FLAGS extern __printf(3, 4) void warn_slowpath_fmt(const char *file, const int line, const char *fmt, ...); @@ -104,12 +103,12 @@ extern void warn_slowpath_null(const char *file, const int line); warn_slowpath_fmt_taint(__FILE__, __LINE__, taint, arg) #else extern __printf(1, 2) void __warn_printk(const char *fmt, ...); -#define __WARN() do { \ - printk(KERN_WARNING CUT_HERE); __WARN_TAINT(TAINT_WARN); \ -} while (0) +#define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN)) #define __WARN_printf(arg...) __WARN_printf_taint(TAINT_WARN, arg) -#define __WARN_printf_taint(taint, arg...) \ - do { __warn_printk(arg); __WARN_TAINT(taint); } while (0) +#define __WARN_printf_taint(taint, arg...) do { \ + __warn_printk(arg); __WARN_FLAGS(BUGFLAG_PRINTK | \ + BUGFLAG_TAINT(taint)); \ + } while (0) #endif /* used internally by panic.c */ diff --git a/kernel/panic.c b/kernel/panic.c index 057540b6eee9..03c98da6e3f7 100644 --- a/kernel/panic.c +++ b/kernel/panic.c @@ -551,9 +551,6 @@ void __warn(const char *file, int line, void *caller, unsigned taint, { disable_trace_on_warning(); - if (args) - pr_warn(CUT_HERE); - if (file) pr_warn("WARNING: CPU: %d PID: %d at %s:%d %pS\n", raw_smp_processor_id(), current->pid, file, line, @@ -596,6 +593,7 @@ void warn_slowpath_fmt(const char *file, int line, const char *fmt, ...) { struct warn_args args; + pr_warn(CUT_HERE); args.fmt = fmt; va_start(args.args, fmt); __warn(file, line, __builtin_return_address(0), TAINT_WARN, NULL, @@ -609,6 +607,7 @@ void warn_slowpath_fmt_taint(const char *file, int line, { struct warn_args args; + pr_warn(CUT_HERE); args.fmt = fmt; va_start(args.args, fmt); __warn(file, line, __builtin_return_address(0), taint, NULL, &args); diff --git a/lib/bug.c b/lib/bug.c index 1077366f496b..73ce8f9d9eff 100644 --- a/lib/bug.c +++ b/lib/bug.c @@ -181,6 +181,10 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) } } + /* Did this trap already report a printk line with "cut here"? */ + if ((bug->flags & BUGFLAG_PRINTK) == 0) + printk(KERN_DEFAULT CUT_HERE); + if (warning) { /* this is a WARN_ON rather than BUG/BUG_ON */ __warn(file, line, (void *)bugaddr, BUG_GET_TAINT(bug), regs, @@ -188,8 +192,6 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) return BUG_TRAP_TYPE_WARN; } - printk(KERN_DEFAULT CUT_HERE); - if (file) pr_crit("kernel BUG at %s:%u!\n", file, line); else -- Kees Cook ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-11-25 10:46 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-19 13:06 [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON() Christophe Leroy 2019-08-19 13:06 ` [PATCH 2/3] powerpc: refactoring BUG/WARN macros Christophe Leroy 2019-11-25 10:46 ` Michael Ellerman 2019-08-19 13:06 ` [PATCH 3/3] powerpc: use __builtin_trap() in " Christophe Leroy 2019-08-19 13:23 ` Segher Boessenkool 2019-08-19 14:08 ` Christophe Leroy 2019-08-19 14:37 ` Segher Boessenkool 2019-08-19 15:05 ` Christophe Leroy 2019-08-19 15:45 ` Segher Boessenkool 2019-08-23 15:35 ` Christophe Leroy 2019-08-19 16:28 ` [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON() Kees Cook 2019-08-19 17:29 ` Clean up cut-here even harder (was Re: [PATCH 1/3] powerpc: don't use __WARN() for WARN_ON()) Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).