Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
Nathan Chancellor writes:
> On Thu, Aug 26, 2021 at 11:54:17AM -0700, Nathan Chancellor wrote:
>> On Thu, Aug 26, 2021 at 01:21:39PM +1000, Michael Ellerman wrote:
>> > Nathan Chancellor writes:
>> > > On Tue, Apr 13, 2021 at 04:38:10PM +, Christophe Leroy wrote:
>> > >> Using asm goto in __WARN_FLAGS() and WARN_ON() allows more
>> > >> flexibility to GCC.
>> > ...
>> > >
>> > > This patch as commit 1e688dd2a3d6 ("powerpc/bug: Provide better
>> > > flexibility to WARN_ON/__WARN_FLAGS() with asm goto") cause a WARN_ON in
>> > > klist_add_tail to trigger over and over on boot when compiling with
>> > > clang:
>> > >
...
>> >
>> > This patch seems to fix it. Not sure if that's just papering over it
>> > though.
>> >
>> > diff --git a/arch/powerpc/include/asm/bug.h
>> > b/arch/powerpc/include/asm/bug.h
>> > index 1ee0f22313ee..75fcb4370d96 100644
>> > --- a/arch/powerpc/include/asm/bug.h
>> > +++ b/arch/powerpc/include/asm/bug.h
>> > @@ -119,7 +119,7 @@ __label_warn_on:
>> > \
>> >\
>> >WARN_ENTRY(PPC_TLNEI " %4, 0", \
>> > BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN),
>> > \
>> > - __label_warn_on, "r" (x)); \
>> > + __label_warn_on, "r" (!!(x))); \
>> >break; \
>> > __label_warn_on: \
>> >__ret_warn_on = true; \
>> >
>>
>> Thank you for the in-depth explanation and triage! I have gone ahead and
>> created a standalone reproducer that shows this based on the
>> preprocessed file and opened https://bugs.llvm.org/show_bug.cgi?id=51634
>> so the LLVM developers can take a look.
>
> Based on Eli Friedman's comment in the bug, would something like this
> work and not regress GCC? I noticed that the BUG_ON macro does a cast as
> well. Nick pointed out to me privately that we have run into what seems
> like a similar issue before in commit 6c58f25e6938 ("riscv/atomic: Fix
> sign extension for RV64I").
Yes, I read that comment this morning, and then landed at the same fix
via digging through the history of our bug code.
We in fact fixed a similar bug 16 years ago :}
32818c2eb6b8 ("[PATCH] ppc64: Fix issue with gcc 4.0 compiled kernels")
Which is when we first started adding the cast to long.
> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
> index 1ee0f22313ee..35022667f57d 100644
> --- a/arch/powerpc/include/asm/bug.h
> +++ b/arch/powerpc/include/asm/bug.h
> @@ -119,7 +119,7 @@ __label_warn_on:
> \
> \
> WARN_ENTRY(PPC_TLNEI " %4, 0", \
>BUGFLAG_WARNING |
> BUGFLAG_TAINT(TAINT_WARN), \
> - __label_warn_on, "r" (x)); \
> + __label_warn_on, "r" ((__force long)(x)));
> \
> break; \
> __label_warn_on: \
> __ret_warn_on = true; \
Yeah that fixes the clang build for me.
For GCC it seems to generate the same code in the simple cases:
void test_warn_on_ulong(unsigned long b)
{
WARN_ON(b);
}
void test_warn_on_int(int b)
{
WARN_ON(b);
}
I get:
0020 <.test_warn_on_ulong>:
20: 0b 03 00 00 tdnei r3,0
24: 4e 80 00 20 blr
28: 60 00 00 00 nop
2c: 60 00 00 00 nop
0030 <.test_warn_on_int>:
30: 0b 03 00 00 tdnei r3,0
34: 4e 80 00 20 blr
Both before and after the change. So that's good.
For:
void test_warn_on_int_addition(int b)
{
WARN_ON(b+1);
}
Before I get:
0040 <.test_warn_on_int_addition>:
40: 38 63 00 01 addir3,r3,1
44: 0b 03 00 00 tdnei r3,0
48: 4e 80 00 20 blr
vs after:
0040 <.test_warn_on_int_addition>:
40: 38 63 00 01 addir3,r3,1
44: 7c 63 07 b4 extsw r3,r3
48: 0b 03 00 00 tdnei r3,0
4c: 4e 80 00 20 blr
So there's an extra sign extension we don't need, but I guess we can
probably live with that.
cheers
Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
On Thu, Aug 26, 2021 at 11:54:17AM -0700, Nathan Chancellor wrote:
> On Thu, Aug 26, 2021 at 01:21:39PM +1000, Michael Ellerman wrote:
> > Nathan Chancellor writes:
> > > On Tue, Apr 13, 2021 at 04:38:10PM +, Christophe Leroy wrote:
> > >> Using asm goto in __WARN_FLAGS() and WARN_ON() allows more
> > >> flexibility to GCC.
> > ...
> > >
> > > This patch as commit 1e688dd2a3d6 ("powerpc/bug: Provide better
> > > flexibility to WARN_ON/__WARN_FLAGS() with asm goto") cause a WARN_ON in
> > > klist_add_tail to trigger over and over on boot when compiling with
> > > clang:
> > >
> > > [2.177416][T1] WARNING: CPU: 0 PID: 1 at lib/klist.c:62
> > > .klist_add_tail+0x3c/0x110
> > > [2.177456][T1] Modules linked in:
> > > [2.177481][T1] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW
> > >5.14.0-rc7-next-20210825 #1
> > > [2.177520][T1] NIP: c07ff81c LR: c090a038 CTR:
> > >
> > > [2.177557][T1] REGS: c73c32a0 TRAP: 0700 Tainted: G
> > >W (5.14.0-rc7-next-20210825)
> > > [2.177593][T1] MSR: 82029032
> > > CR: 22000a40 XER:
> > > [2.177667][T1] CFAR: c090a034 IRQMASK: 0
> > > [2.177667][T1] GPR00: c090a038 c73c3540
> > > c1be3200 0001
> > > [2.177667][T1] GPR04: c72d65c0
> > > c91ba798 c91bb0a0
> > > [2.177667][T1] GPR08: 0001
> > > c8581918 fc00
> > > [2.177667][T1] GPR12: 44000240 c1dd
> > > c0012300
> > > [2.177667][T1] GPR16:
> > >
> > > [2.177667][T1] GPR20:
> > >
> > > [2.177667][T1] GPR24: c17e3200
> > > c1a0e778
> > > [2.177667][T1] GPR28: c72d65b0 c72d65a8
> > > c7de72c8 c73c35d0
> > > [2.178019][T1] NIP [c07ff81c] .klist_add_tail+0x3c/0x110
> > > [2.178058][T1] LR [c090a038] .bus_add_driver+0x148/0x290
> > > [2.178088][T1] Call Trace:
> > > [2.178105][T1] [c73c3540] [c73c35d0]
> > > 0xc73c35d0 (unreliable)
> > > [2.178150][T1] [c73c35d0] [c090a038]
> > > .bus_add_driver+0x148/0x290
> > > [2.178190][T1] [c73c3670] [c090fae8]
> > > .driver_register+0xb8/0x190
> > > [2.178234][T1] [c73c3700] [c0be55c0]
> > > .__hid_register_driver+0x70/0xd0
> > > [2.178275][T1] [c73c37a0] [c116955c]
> > > .redragon_driver_init+0x34/0x58
> > > [2.178314][T1] [c73c3820] [c0011ae0]
> > > .do_one_initcall+0x130/0x3b0
> > > [2.178357][T1] [c73c3bb0] [c11065e0]
> > > .do_initcall_level+0xd8/0x188
> > > [2.178403][T1] [c73c3c50] [c11064a8]
> > > .do_initcalls+0x7c/0xdc
> > > [2.178445][T1] [c73c3ce0] [c1106238]
> > > .kernel_init_freeable+0x178/0x21c
> > > [2.178491][T1] [c73c3d90] [c0012334]
> > > .kernel_init+0x34/0x220
> > > [2.178530][T1] [c73c3e10] [c000cf50]
> > > .ret_from_kernel_thread+0x58/0x60
> > > [2.178569][T1] Instruction dump:
> > > [2.178592][T1] fba10078 7c7d1b78 3861 fb810070 3b9d0008
> > > fbc10080 7c9e2378 389d0018
> > > [2.178662][T1] fb9d0008 fb9d0010 9064 fbdd <0b1e>
> > > e87e0018 2823 41820024
> > > [2.178728][T1] ---[ end trace 52ed3431f58f1847 ]---
> > >
> > > Is this a bug with clang or is there something wrong with the patch? The
> > > vmlinux image is available at [1] if you want to inspect it and our QEMU
> > > command and the warning at boot can be viewed at [2]. If there is any
> > > other information I can provide, please let me know.
> > >
> > > [1] https://builds.tuxbuild.com/1xDcmp3Tvno0TTGxDVPedRKIKM2/
> > > [2]
> > > https://github.com/ClangBuiltLinux/continuous-integration2/commit/cee159b66a58eb57fa2359e7888074b9da24126c/checks/3422232736/logs
> >
> > Thanks.
> >
> > This is the generated assembly:
> >
> > c07ff600 <.klist_add_tail>:
> > c07ff600: 7c 08 02 a6 mflrr0
> > c07ff604: f8 01 00 10 std r0,16(r1)
> > c07ff608: f8 21 ff 71 stdur1,-144(r1) ^ prolog
> > c07ff60c: fb a1 00 78 std r29,120(r1) save r29 to
> > stack
> > c07ff610: 7c 7d 1b 78 mr r29,r3 r29 =
> > struct klist_node *n
> > c07ff614: 38 60 00 01 li r3,1r3 = 1
> > c07ff618: fb 81 00 70 std r28,112(r1) save r28 to
> > stack
> > c07ff61c:
Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
On Thu, Aug 26, 2021 at 01:21:39PM +1000, Michael Ellerman wrote:
> Nathan Chancellor writes:
> > On Tue, Apr 13, 2021 at 04:38:10PM +, Christophe Leroy wrote:
> >> Using asm goto in __WARN_FLAGS() and WARN_ON() allows more
> >> flexibility to GCC.
> ...
> >
> > This patch as commit 1e688dd2a3d6 ("powerpc/bug: Provide better
> > flexibility to WARN_ON/__WARN_FLAGS() with asm goto") cause a WARN_ON in
> > klist_add_tail to trigger over and over on boot when compiling with
> > clang:
> >
> > [2.177416][T1] WARNING: CPU: 0 PID: 1 at lib/klist.c:62
> > .klist_add_tail+0x3c/0x110
> > [2.177456][T1] Modules linked in:
> > [2.177481][T1] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW
> > 5.14.0-rc7-next-20210825 #1
> > [2.177520][T1] NIP: c07ff81c LR: c090a038 CTR:
> >
> > [2.177557][T1] REGS: c73c32a0 TRAP: 0700 Tainted: G
> > W (5.14.0-rc7-next-20210825)
> > [2.177593][T1] MSR: 82029032 CR:
> > 22000a40 XER:
> > [2.177667][T1] CFAR: c090a034 IRQMASK: 0
> > [2.177667][T1] GPR00: c090a038 c73c3540
> > c1be3200 0001
> > [2.177667][T1] GPR04: c72d65c0
> > c91ba798 c91bb0a0
> > [2.177667][T1] GPR08: 0001
> > c8581918 fc00
> > [2.177667][T1] GPR12: 44000240 c1dd
> > c0012300
> > [2.177667][T1] GPR16:
> >
> > [2.177667][T1] GPR20:
> >
> > [2.177667][T1] GPR24: c17e3200
> > c1a0e778
> > [2.177667][T1] GPR28: c72d65b0 c72d65a8
> > c7de72c8 c73c35d0
> > [2.178019][T1] NIP [c07ff81c] .klist_add_tail+0x3c/0x110
> > [2.178058][T1] LR [c090a038] .bus_add_driver+0x148/0x290
> > [2.178088][T1] Call Trace:
> > [2.178105][T1] [c73c3540] [c73c35d0]
> > 0xc73c35d0 (unreliable)
> > [2.178150][T1] [c73c35d0] [c090a038]
> > .bus_add_driver+0x148/0x290
> > [2.178190][T1] [c73c3670] [c090fae8]
> > .driver_register+0xb8/0x190
> > [2.178234][T1] [c73c3700] [c0be55c0]
> > .__hid_register_driver+0x70/0xd0
> > [2.178275][T1] [c73c37a0] [c116955c]
> > .redragon_driver_init+0x34/0x58
> > [2.178314][T1] [c73c3820] [c0011ae0]
> > .do_one_initcall+0x130/0x3b0
> > [2.178357][T1] [c73c3bb0] [c11065e0]
> > .do_initcall_level+0xd8/0x188
> > [2.178403][T1] [c73c3c50] [c11064a8]
> > .do_initcalls+0x7c/0xdc
> > [2.178445][T1] [c73c3ce0] [c1106238]
> > .kernel_init_freeable+0x178/0x21c
> > [2.178491][T1] [c73c3d90] [c0012334]
> > .kernel_init+0x34/0x220
> > [2.178530][T1] [c73c3e10] [c000cf50]
> > .ret_from_kernel_thread+0x58/0x60
> > [2.178569][T1] Instruction dump:
> > [2.178592][T1] fba10078 7c7d1b78 3861 fb810070 3b9d0008
> > fbc10080 7c9e2378 389d0018
> > [2.178662][T1] fb9d0008 fb9d0010 9064 fbdd <0b1e>
> > e87e0018 2823 41820024
> > [2.178728][T1] ---[ end trace 52ed3431f58f1847 ]---
> >
> > Is this a bug with clang or is there something wrong with the patch? The
> > vmlinux image is available at [1] if you want to inspect it and our QEMU
> > command and the warning at boot can be viewed at [2]. If there is any
> > other information I can provide, please let me know.
> >
> > [1] https://builds.tuxbuild.com/1xDcmp3Tvno0TTGxDVPedRKIKM2/
> > [2]
> > https://github.com/ClangBuiltLinux/continuous-integration2/commit/cee159b66a58eb57fa2359e7888074b9da24126c/checks/3422232736/logs
>
> Thanks.
>
> This is the generated assembly:
>
> c07ff600 <.klist_add_tail>:
> c07ff600: 7c 08 02 a6 mflrr0
> c07ff604: f8 01 00 10 std r0,16(r1)
> c07ff608: f8 21 ff 71 stdur1,-144(r1) ^ prolog
> c07ff60c: fb a1 00 78 std r29,120(r1) save r29 to
> stack
> c07ff610: 7c 7d 1b 78 mr r29,r3r29 =
> struct klist_node *n
> c07ff614: 38 60 00 01 li r3,1 r3 = 1
> c07ff618: fb 81 00 70 std r28,112(r1) save r28 to
> stack
> c07ff61c: 3b 9d 00 08 addir28,r29,8 r28 = >n_node
> c07ff620: fb c1 00 80 std r30,128(r1) save r30 to
> stack
> c07ff624: 7c 9e 23 78 mr r30,r4r30 =
> struct klist *k
> c07ff628:
Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
Le 26/08/2021 à 16:45, Michael Ellerman a écrit :
Christophe Leroy writes:
Le 26/08/2021 à 05:21, Michael Ellerman a écrit :
Nathan Chancellor writes:
On Tue, Apr 13, 2021 at 04:38:10PM +, Christophe Leroy wrote:
Using asm goto in __WARN_FLAGS() and WARN_ON() allows more
flexibility to GCC.
...
This patch as commit 1e688dd2a3d6 ("powerpc/bug: Provide better
flexibility to WARN_ON/__WARN_FLAGS() with asm goto") cause a WARN_ON in
klist_add_tail to trigger over and over on boot when compiling with
clang:
...
This patch seems to fix it. Not sure if that's just papering over it though.
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 1ee0f22313ee..75fcb4370d96 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -119,7 +119,7 @@ __label_warn_on:
\
\
WARN_ENTRY(PPC_TLNEI " %4, 0",\
BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN),
\
- __label_warn_on, "r" (x)); \
+ __label_warn_on, "r" (!!(x))); \
break; \
__label_warn_on: \
__ret_warn_on = true; \
But for a simple WARN_ON() call:
void test(unsigned long b)
{
WARN_ON(b);
}
Without your change with GCC you get:
12d0 <.test>:
12d0: 0b 03 00 00 tdnei r3,0
12d4: 4e 80 00 20 blr
With the !! change you get:
12d0 <.test>:
12d0: 31 23 ff ff addic r9,r3,-1
12d4: 7d 29 19 10 subfe r9,r9,r3
12d8: 0b 09 00 00 tdnei r9,0
12dc: 4e 80 00 20 blr
Yeah that's a pity.
We could do something like below, which is ugly, but would be better
than having to revert the whole thing.
Yes looks nice, we already had that kind of stuff in the past, even more ugly.
Although this doesn't fix the strange warning in drivers/net/ethernet/sfc.
What's the warning ?
So possibly we need a CLANG ifdef around the whole thing, and use the
old style warn for clang.
Christophe
Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
Christophe Leroy writes:
> Le 26/08/2021 à 05:21, Michael Ellerman a écrit :
>> Nathan Chancellor writes:
>>> On Tue, Apr 13, 2021 at 04:38:10PM +, Christophe Leroy wrote:
Using asm goto in __WARN_FLAGS() and WARN_ON() allows more
flexibility to GCC.
>> ...
>>>
>>> This patch as commit 1e688dd2a3d6 ("powerpc/bug: Provide better
>>> flexibility to WARN_ON/__WARN_FLAGS() with asm goto") cause a WARN_ON in
>>> klist_add_tail to trigger over and over on boot when compiling with
>>> clang:
>
> ...
>
>>
>> This patch seems to fix it. Not sure if that's just papering over it though.
>>
>> diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
>> index 1ee0f22313ee..75fcb4370d96 100644
>> --- a/arch/powerpc/include/asm/bug.h
>> +++ b/arch/powerpc/include/asm/bug.h
>> @@ -119,7 +119,7 @@ __label_warn_on:
>> \
>> \
>> WARN_ENTRY(PPC_TLNEI " %4, 0", \
>> BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN),
>> \
>> - __label_warn_on, "r" (x)); \
>> + __label_warn_on, "r" (!!(x))); \
>> break; \
>> __label_warn_on: \
>> __ret_warn_on = true; \
>
> But for a simple WARN_ON() call:
>
> void test(unsigned long b)
> {
> WARN_ON(b);
> }
>
> Without your change with GCC you get:
>
> 12d0 <.test>:
> 12d0:0b 03 00 00 tdnei r3,0
> 12d4:4e 80 00 20 blr
>
>
> With the !! change you get:
>
> 12d0 <.test>:
> 12d0:31 23 ff ff addic r9,r3,-1
> 12d4:7d 29 19 10 subfe r9,r9,r3
> 12d8:0b 09 00 00 tdnei r9,0
> 12dc:4e 80 00 20 blr
Yeah that's a pity.
We could do something like below, which is ugly, but would be better
than having to revert the whole thing.
Although this doesn't fix the strange warning in drivers/net/ethernet/sfc.
So possibly we need a CLANG ifdef around the whole thing, and use the
old style warn for clang.
cheers
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 1ee0f22313ee..d978d9004d0d 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -106,6 +106,12 @@ __label_warn_on:
\
} \
} while (0)
+#ifdef CONFIG_CC_IS_CLANG
+#define __clang_warn_hack(x) (!!(x))
+#else
+#define __clang_warn_hack(x) (x)
+#endif
+
#define WARN_ON(x) ({ \
bool __ret_warn_on = false; \
do {\
@@ -119,7 +125,8 @@ __label_warn_on:
\
\
WARN_ENTRY(PPC_TLNEI " %4, 0", \
BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN),
\
- __label_warn_on, "r" (x)); \
+ __label_warn_on, \
+ "r" __clang_warn_hack(x)); \
break; \
__label_warn_on: \
__ret_warn_on = true; \
Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
On Thu, Aug 26, 2021 at 01:21:39PM +1000, Michael Ellerman wrote: > So there seems to be some misunderstanding with clang, it doesn't like > us passing an expression to the inline asm. > > AFAIK it is legal to pass expressions as inputs to inline asm, ie. it > doesn't have to just be a variable name. It certainly is. That is the whole point of inline asm! This way, all of the C code "around" the asm can be optimised. > This patch seems to fix it. Not sure if that's just papering over it though. It is, and it makes less optimised code (also on GCC), as Christophe points out. Segher
Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
On Thu, Aug 26, 2021 at 08:37:09AM +0200, Christophe Leroy wrote:
> Le 26/08/2021 à 05:21, Michael Ellerman a écrit :
> >This patch seems to fix it. Not sure if that's just papering over it
> >though.
> >
> >diff --git a/arch/powerpc/include/asm/bug.h
> >b/arch/powerpc/include/asm/bug.h
> >index 1ee0f22313ee..75fcb4370d96 100644
> >--- a/arch/powerpc/include/asm/bug.h
> >+++ b/arch/powerpc/include/asm/bug.h
> >@@ -119,7 +119,7 @@ __label_warn_on: \
> > \
> > WARN_ENTRY(PPC_TLNEI " %4, 0", \
> >BUGFLAG_WARNING |
> >BUGFLAG_TAINT(TAINT_WARN), \
> >- __label_warn_on, "r" (x)); \
> >+ __label_warn_on, "r" (!!(x))); \
> > break; \
> > __label_warn_on: \
> > __ret_warn_on = true; \
>
> But for a simple WARN_ON() call:
>
> void test(unsigned long b)
> {
> WARN_ON(b);
> }
>
> Without your change with GCC you get:
>
> 12d0 <.test>:
> 12d0: 0b 03 00 00 tdnei r3,0
> 12d4: 4e 80 00 20 blr
>
>
> With the !! change you get:
>
> 12d0 <.test>:
> 12d0: 31 23 ff ff addic r9,r3,-1
> 12d4: 7d 29 19 10 subfe r9,r9,r3
> 12d8: 0b 09 00 00 tdnei r9,0
> 12dc: 4e 80 00 20 blr
That is because the asm (unlike the builtin) cannot be optimised by the
compiler.
Segher
Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
Le 26/08/2021 à 05:21, Michael Ellerman a écrit :
Nathan Chancellor writes:
On Tue, Apr 13, 2021 at 04:38:10PM +, Christophe Leroy wrote:
Using asm goto in __WARN_FLAGS() and WARN_ON() allows more
flexibility to GCC.
...
This patch as commit 1e688dd2a3d6 ("powerpc/bug: Provide better
flexibility to WARN_ON/__WARN_FLAGS() with asm goto") cause a WARN_ON in
klist_add_tail to trigger over and over on boot when compiling with
clang:
...
This patch seems to fix it. Not sure if that's just papering over it though.
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 1ee0f22313ee..75fcb4370d96 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -119,7 +119,7 @@ __label_warn_on:
\
\
WARN_ENTRY(PPC_TLNEI " %4, 0",\
BUGFLAG_WARNING | BUGFLAG_TAINT(TAINT_WARN),
\
- __label_warn_on, "r" (x)); \
+ __label_warn_on, "r" (!!(x))); \
break; \
__label_warn_on: \
__ret_warn_on = true; \
But for a simple WARN_ON() call:
void test(unsigned long b)
{
WARN_ON(b);
}
Without your change with GCC you get:
12d0 <.test>:
12d0: 0b 03 00 00 tdnei r3,0
12d4: 4e 80 00 20 blr
With the !! change you get:
12d0 <.test>:
12d0: 31 23 ff ff addic r9,r3,-1
12d4: 7d 29 19 10 subfe r9,r9,r3
12d8: 0b 09 00 00 tdnei r9,0
12dc: 4e 80 00 20 blr
Christophe
Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
Nathan Chancellor writes:
> On Tue, Apr 13, 2021 at 04:38:10PM +, Christophe Leroy wrote:
>> Using asm goto in __WARN_FLAGS() and WARN_ON() allows more
>> flexibility to GCC.
...
>
> This patch as commit 1e688dd2a3d6 ("powerpc/bug: Provide better
> flexibility to WARN_ON/__WARN_FLAGS() with asm goto") cause a WARN_ON in
> klist_add_tail to trigger over and over on boot when compiling with
> clang:
>
> [2.177416][T1] WARNING: CPU: 0 PID: 1 at lib/klist.c:62
> .klist_add_tail+0x3c/0x110
> [2.177456][T1] Modules linked in:
> [2.177481][T1] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW
>5.14.0-rc7-next-20210825 #1
> [2.177520][T1] NIP: c07ff81c LR: c090a038 CTR:
>
> [2.177557][T1] REGS: c73c32a0 TRAP: 0700 Tainted: G
> W (5.14.0-rc7-next-20210825)
> [2.177593][T1] MSR: 82029032 CR:
> 22000a40 XER:
> [2.177667][T1] CFAR: c090a034 IRQMASK: 0
> [2.177667][T1] GPR00: c090a038 c73c3540
> c1be3200 0001
> [2.177667][T1] GPR04: c72d65c0
> c91ba798 c91bb0a0
> [2.177667][T1] GPR08: 0001
> c8581918 fc00
> [2.177667][T1] GPR12: 44000240 c1dd
> c0012300
> [2.177667][T1] GPR16:
>
> [2.177667][T1] GPR20:
>
> [2.177667][T1] GPR24: c17e3200
> c1a0e778
> [2.177667][T1] GPR28: c72d65b0 c72d65a8
> c7de72c8 c73c35d0
> [2.178019][T1] NIP [c07ff81c] .klist_add_tail+0x3c/0x110
> [2.178058][T1] LR [c090a038] .bus_add_driver+0x148/0x290
> [2.178088][T1] Call Trace:
> [2.178105][T1] [c73c3540] [c73c35d0]
> 0xc73c35d0 (unreliable)
> [2.178150][T1] [c73c35d0] [c090a038]
> .bus_add_driver+0x148/0x290
> [2.178190][T1] [c73c3670] [c090fae8]
> .driver_register+0xb8/0x190
> [2.178234][T1] [c73c3700] [c0be55c0]
> .__hid_register_driver+0x70/0xd0
> [2.178275][T1] [c73c37a0] [c116955c]
> .redragon_driver_init+0x34/0x58
> [2.178314][T1] [c73c3820] [c0011ae0]
> .do_one_initcall+0x130/0x3b0
> [2.178357][T1] [c73c3bb0] [c11065e0]
> .do_initcall_level+0xd8/0x188
> [2.178403][T1] [c73c3c50] [c11064a8]
> .do_initcalls+0x7c/0xdc
> [2.178445][T1] [c73c3ce0] [c1106238]
> .kernel_init_freeable+0x178/0x21c
> [2.178491][T1] [c73c3d90] [c0012334]
> .kernel_init+0x34/0x220
> [2.178530][T1] [c73c3e10] [c000cf50]
> .ret_from_kernel_thread+0x58/0x60
> [2.178569][T1] Instruction dump:
> [2.178592][T1] fba10078 7c7d1b78 3861 fb810070 3b9d0008 fbc10080
> 7c9e2378 389d0018
> [2.178662][T1] fb9d0008 fb9d0010 9064 fbdd <0b1e>
> e87e0018 2823 41820024
> [2.178728][T1] ---[ end trace 52ed3431f58f1847 ]---
>
> Is this a bug with clang or is there something wrong with the patch? The
> vmlinux image is available at [1] if you want to inspect it and our QEMU
> command and the warning at boot can be viewed at [2]. If there is any
> other information I can provide, please let me know.
>
> [1] https://builds.tuxbuild.com/1xDcmp3Tvno0TTGxDVPedRKIKM2/
> [2]
> https://github.com/ClangBuiltLinux/continuous-integration2/commit/cee159b66a58eb57fa2359e7888074b9da24126c/checks/3422232736/logs
Thanks.
This is the generated assembly:
c07ff600 <.klist_add_tail>:
c07ff600: 7c 08 02 a6 mflrr0
c07ff604: f8 01 00 10 std r0,16(r1)
c07ff608: f8 21 ff 71 stdur1,-144(r1) ^ prolog
c07ff60c: fb a1 00 78 std r29,120(r1) save r29 to
stack
c07ff610: 7c 7d 1b 78 mr r29,r3 r29 = struct
klist_node *n
c07ff614: 38 60 00 01 li r3,1r3 = 1
c07ff618: fb 81 00 70 std r28,112(r1) save r28 to
stack
c07ff61c: 3b 9d 00 08 addir28,r29,8 r28 = >n_node
c07ff620: fb c1 00 80 std r30,128(r1) save r30 to
stack
c07ff624: 7c 9e 23 78 mr r30,r4 r30 = struct
klist *k
c07ff628: 38 9d 00 18 addir4,r29,24 r4 = >n_ref
c07ff62c: fb 9d 00 08 std r28,8(r29) n->n_node.next
= >n_node INIT_LIST_HEAD
c07ff630: fb 9d 00 10 std r28,16(r29) n->n_node.prev
= >n_node
Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
Hi Christophe,
On Tue, Apr 13, 2021 at 04:38:10PM +, Christophe Leroy wrote:
> Using asm goto in __WARN_FLAGS() and WARN_ON() allows more
> flexibility to GCC.
>
> For that add an entry to the exception table so that
> program_check_exception() knowns where to resume execution
> after a WARNING.
>
> Here are two exemples. The first one is done on PPC32 (which
> benefits from the previous patch), the second is on PPC64.
>
> unsigned long test(struct pt_regs *regs)
> {
> int ret;
>
> WARN_ON(regs->msr & MSR_PR);
>
> return regs->gpr[3];
> }
>
> unsigned long test9w(unsigned long a, unsigned long b)
> {
> if (WARN_ON(!b))
> return 0;
> return a / b;
> }
>
> Before the patch:
>
> 03a8 :
>3a8: 81 23 00 84 lwz r9,132(r3)
>3ac: 71 29 40 00 andi. r9,r9,16384
>3b0: 40 82 00 0c bne 3bc
>3b4: 80 63 00 0c lwz r3,12(r3)
>3b8: 4e 80 00 20 blr
>
>3bc: 0f e0 00 00 twuir0,0
>3c0: 80 63 00 0c lwz r3,12(r3)
>3c4: 4e 80 00 20 blr
>
> 0bf0 <.test9w>:
>bf0: 7c 89 00 74 cntlzd r9,r4
>bf4: 79 29 d1 82 rldicl r9,r9,58,6
>bf8: 0b 09 00 00 tdnei r9,0
>bfc: 2c 24 00 00 cmpdi r4,0
>c00: 41 82 00 0c beq c0c <.test9w+0x1c>
>c04: 7c 63 23 92 divdu r3,r3,r4
>c08: 4e 80 00 20 blr
>
>c0c: 38 60 00 00 li r3,0
>c10: 4e 80 00 20 blr
>
> After the patch:
>
> 03a8 :
>3a8: 81 23 00 84 lwz r9,132(r3)
>3ac: 71 29 40 00 andi. r9,r9,16384
>3b0: 40 82 00 0c bne 3bc
>3b4: 80 63 00 0c lwz r3,12(r3)
>3b8: 4e 80 00 20 blr
>
>3bc: 0f e0 00 00 twuir0,0
>
> 0c50 <.test9w>:
>c50: 7c 89 00 74 cntlzd r9,r4
>c54: 79 29 d1 82 rldicl r9,r9,58,6
>c58: 0b 09 00 00 tdnei r9,0
>c5c: 7c 63 23 92 divdu r3,r3,r4
>c60: 4e 80 00 20 blr
>
>c70: 38 60 00 00 li r3,0
>c74: 4e 80 00 20 blr
>
> In the first exemple, we see GCC doesn't need to duplicate what
> happens after the trap.
>
> In the second exemple, we see that GCC doesn't need to emit a test
> and a branch in the likely path in addition to the trap.
>
> We've got some WARN_ON() in .softirqentry.text section so it needs
> to be added in the OTHER_TEXT_SECTIONS in modpost.c
>
> Signed-off-by: Christophe Leroy
This patch as commit 1e688dd2a3d6 ("powerpc/bug: Provide better
flexibility to WARN_ON/__WARN_FLAGS() with asm goto") cause a WARN_ON in
klist_add_tail to trigger over and over on boot when compiling with
clang:
[2.177416][T1] WARNING: CPU: 0 PID: 1 at lib/klist.c:62
.klist_add_tail+0x3c/0x110
[2.177456][T1] Modules linked in:
[2.177481][T1] CPU: 0 PID: 1 Comm: swapper/0 Tainted: GW
5.14.0-rc7-next-20210825 #1
[2.177520][T1] NIP: c07ff81c LR: c090a038 CTR:
[2.177557][T1] REGS: c73c32a0 TRAP: 0700 Tainted: GW
(5.14.0-rc7-next-20210825)
[2.177593][T1] MSR: 82029032 CR:
22000a40 XER:
[2.177667][T1] CFAR: c090a034 IRQMASK: 0
[2.177667][T1] GPR00: c090a038 c73c3540
c1be3200 0001
[2.177667][T1] GPR04: c72d65c0
c91ba798 c91bb0a0
[2.177667][T1] GPR08: 0001
c8581918 fc00
[2.177667][T1] GPR12: 44000240 c1dd
c0012300
[2.177667][T1] GPR16:
[2.177667][T1] GPR20:
[2.177667][T1] GPR24: c17e3200
c1a0e778
[2.177667][T1] GPR28: c72d65b0 c72d65a8
c7de72c8 c73c35d0
[2.178019][T1] NIP [c07ff81c] .klist_add_tail+0x3c/0x110
[2.178058][T1] LR [c090a038] .bus_add_driver+0x148/0x290
[2.178088][T1] Call Trace:
[2.178105][T1] [c73c3540] [c73c35d0] 0xc73c35d0
(unreliable)
[2.178150][T1] [c73c35d0] [c090a038]
.bus_add_driver+0x148/0x290
[2.178190][T1] [c73c3670] [c090fae8]
.driver_register+0xb8/0x190
[2.178234][T1] [c73c3700] [c0be55c0]
.__hid_register_driver+0x70/0xd0
[2.178275][T1] [c73c37a0] [c116955c]
.redragon_driver_init+0x34/0x58
[2.178314][T1]
Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
Christophe Leroy writes: > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c > index 24725e50c7b4..34745f239208 100644 > --- a/scripts/mod/modpost.c > +++ b/scripts/mod/modpost.c > @@ -926,7 +926,7 @@ static void check_section(const char *modname, struct > elf_info *elf, > ".kprobes.text", ".cpuidle.text", ".noinstr.text" > #define OTHER_TEXT_SECTIONS ".ref.text", ".head.text", ".spinlock.text", \ > ".fixup", ".entry.text", ".exception.text", ".text.*", \ > - ".coldtext" > + ".coldtext .softirqentry.text" This wasn't working, I updated it to: ".coldtext", ".softirqentry.text" Which works. cheers
Re: [PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
Excerpts from Christophe Leroy's message of April 14, 2021 2:38 am:
> Using asm goto in __WARN_FLAGS() and WARN_ON() allows more
> flexibility to GCC.
>
> For that add an entry to the exception table so that
> program_check_exception() knowns where to resume execution
> after a WARNING.
Nice idea. How much does it bloat the exception table?
How easy would it be to make a different exception table for
unimportant stuff like WARN_ON faults?
>
> Here are two exemples. The first one is done on PPC32 (which
> benefits from the previous patch), the second is on PPC64.
>
> unsigned long test(struct pt_regs *regs)
> {
> int ret;
>
> WARN_ON(regs->msr & MSR_PR);
>
> return regs->gpr[3];
> }
>
> unsigned long test9w(unsigned long a, unsigned long b)
> {
> if (WARN_ON(!b))
> return 0;
> return a / b;
> }
>
> Before the patch:
>
> 03a8 :
>3a8: 81 23 00 84 lwz r9,132(r3)
>3ac: 71 29 40 00 andi. r9,r9,16384
>3b0: 40 82 00 0c bne 3bc
>3b4: 80 63 00 0c lwz r3,12(r3)
>3b8: 4e 80 00 20 blr
>
>3bc: 0f e0 00 00 twuir0,0
>3c0: 80 63 00 0c lwz r3,12(r3)
>3c4: 4e 80 00 20 blr
>
> 0bf0 <.test9w>:
>bf0: 7c 89 00 74 cntlzd r9,r4
>bf4: 79 29 d1 82 rldicl r9,r9,58,6
>bf8: 0b 09 00 00 tdnei r9,0
>bfc: 2c 24 00 00 cmpdi r4,0
>c00: 41 82 00 0c beq c0c <.test9w+0x1c>
>c04: 7c 63 23 92 divdu r3,r3,r4
>c08: 4e 80 00 20 blr
>
>c0c: 38 60 00 00 li r3,0
>c10: 4e 80 00 20 blr
>
> After the patch:
>
> 03a8 :
>3a8: 81 23 00 84 lwz r9,132(r3)
>3ac: 71 29 40 00 andi. r9,r9,16384
>3b0: 40 82 00 0c bne 3bc
>3b4: 80 63 00 0c lwz r3,12(r3)
>3b8: 4e 80 00 20 blr
>
>3bc: 0f e0 00 00 twuir0,0
>
> 0c50 <.test9w>:
>c50: 7c 89 00 74 cntlzd r9,r4
>c54: 79 29 d1 82 rldicl r9,r9,58,6
>c58: 0b 09 00 00 tdnei r9,0
>c5c: 7c 63 23 92 divdu r3,r3,r4
>c60: 4e 80 00 20 blr
>
>c70: 38 60 00 00 li r3,0
>c74: 4e 80 00 20 blr
One thing that would be nice for WARN_ON_ONCE is to patch out the trap
after the program interrupt. I've seen sometimes the WARN_ON_ONCE starts
firing a lot and slows down the kernel. That gets harder to do if the
trap is now part of the control flow.
I guess that's less important case for performance though. And in theory
you might be able to replace the trap with an equivalent cmp and branch
(but that's probably going too over engineering it).
Thanks,
Nick
[PATCH v2 2/2] powerpc/bug: Provide better flexibility to WARN_ON/__WARN_FLAGS() with asm goto
Using asm goto in __WARN_FLAGS() and WARN_ON() allows more
flexibility to GCC.
For that add an entry to the exception table so that
program_check_exception() knowns where to resume execution
after a WARNING.
Here are two exemples. The first one is done on PPC32 (which
benefits from the previous patch), the second is on PPC64.
unsigned long test(struct pt_regs *regs)
{
int ret;
WARN_ON(regs->msr & MSR_PR);
return regs->gpr[3];
}
unsigned long test9w(unsigned long a, unsigned long b)
{
if (WARN_ON(!b))
return 0;
return a / b;
}
Before the patch:
03a8 :
3a8: 81 23 00 84 lwz r9,132(r3)
3ac: 71 29 40 00 andi. r9,r9,16384
3b0: 40 82 00 0c bne 3bc
3b4: 80 63 00 0c lwz r3,12(r3)
3b8: 4e 80 00 20 blr
3bc: 0f e0 00 00 twuir0,0
3c0: 80 63 00 0c lwz r3,12(r3)
3c4: 4e 80 00 20 blr
0bf0 <.test9w>:
bf0: 7c 89 00 74 cntlzd r9,r4
bf4: 79 29 d1 82 rldicl r9,r9,58,6
bf8: 0b 09 00 00 tdnei r9,0
bfc: 2c 24 00 00 cmpdi r4,0
c00: 41 82 00 0c beq c0c <.test9w+0x1c>
c04: 7c 63 23 92 divdu r3,r3,r4
c08: 4e 80 00 20 blr
c0c: 38 60 00 00 li r3,0
c10: 4e 80 00 20 blr
After the patch:
03a8 :
3a8: 81 23 00 84 lwz r9,132(r3)
3ac: 71 29 40 00 andi. r9,r9,16384
3b0: 40 82 00 0c bne 3bc
3b4: 80 63 00 0c lwz r3,12(r3)
3b8: 4e 80 00 20 blr
3bc: 0f e0 00 00 twuir0,0
0c50 <.test9w>:
c50: 7c 89 00 74 cntlzd r9,r4
c54: 79 29 d1 82 rldicl r9,r9,58,6
c58: 0b 09 00 00 tdnei r9,0
c5c: 7c 63 23 92 divdu r3,r3,r4
c60: 4e 80 00 20 blr
c70: 38 60 00 00 li r3,0
c74: 4e 80 00 20 blr
In the first exemple, we see GCC doesn't need to duplicate what
happens after the trap.
In the second exemple, we see that GCC doesn't need to emit a test
and a branch in the likely path in addition to the trap.
We've got some WARN_ON() in .softirqentry.text section so it needs
to be added in the OTHER_TEXT_SECTIONS in modpost.c
Signed-off-by: Christophe Leroy
---
v2: Fix build failure when CONFIG_BUG is not selected.
---
arch/powerpc/include/asm/book3s/64/kup.h | 2 +-
arch/powerpc/include/asm/bug.h | 54
arch/powerpc/include/asm/extable.h | 14 ++
arch/powerpc/include/asm/ppc_asm.h | 11 +
arch/powerpc/kernel/entry_64.S | 2 +-
arch/powerpc/kernel/exceptions-64e.S | 2 +-
arch/powerpc/kernel/misc_32.S| 2 +-
arch/powerpc/kernel/traps.c | 9 +++-
scripts/mod/modpost.c| 2 +-
9 files changed, 72 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/kup.h
b/arch/powerpc/include/asm/book3s/64/kup.h
index 9700da3a4093..a22839cba32e 100644
--- a/arch/powerpc/include/asm/book3s/64/kup.h
+++ b/arch/powerpc/include/asm/book3s/64/kup.h
@@ -90,7 +90,7 @@
/* Prevent access to userspace using any key values */
LOAD_REG_IMMEDIATE(\gpr2, AMR_KUAP_BLOCKED)
999: tdne\gpr1, \gpr2
- EMIT_BUG_ENTRY 999b, __FILE__, __LINE__, (BUGFLAG_WARNING |
BUGFLAG_ONCE)
+ EMIT_WARN_ENTRY 999b, __FILE__, __LINE__, (BUGFLAG_WARNING |
BUGFLAG_ONCE)
END_MMU_FTR_SECTION_NESTED_IFSET(MMU_FTR_BOOK3S_KUAP, 67)
#endif
.endm
diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h
index 101dea4eec8d..e22dc503fb2f 100644
--- a/arch/powerpc/include/asm/bug.h
+++ b/arch/powerpc/include/asm/bug.h
@@ -4,6 +4,7 @@
#ifdef __KERNEL__
#include
+#include
#ifdef CONFIG_BUG
@@ -30,6 +31,11 @@
.endm
#endif /* verbose */
+.macro EMIT_WARN_ENTRY addr,file,line,flags
+ EX_TABLE(\addr,\addr+4)
+ EMIT_BUG_ENTRY \addr,\file,\line,\flags
+.endm
+
#else /* !__ASSEMBLY__ */
/* _EMIT_BUG_ENTRY expects args %0,%1,%2,%3 to be FILE, LINE, flags and
sizeof(struct bug_entry), respectively */
@@ -58,6 +64,16 @@
"i" (sizeof(struct bug_entry)), \
##__VA_ARGS__)
+#define WARN_ENTRY(insn, flags, label, ...)\
+ asm_volatile_goto( \
+ "1: " insn "\n" \
+ EX_TABLE(1b, %l[label]) \
+ _EMIT_BUG_ENTRY \
+ : : "i" (__FILE__), "i" (__LINE__), \
+ "i" (flags), \
+ "i" (sizeof(struct bug_entry)), \
+
