Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-Id: <20510ce03cc9463f1c9e743c1d93b939de501b53.1566219503.git.christophe.leroy@c-s.fr>
In-Reply-To: <a6781075192afe0c909ce7d091de7931183a5d93.1566219503.git.christophe.leroy@c-s.fr>
References: <a6781075192afe0c909ce7d091de7931183a5d93.1566219503.git.christophe.leroy@c-s.fr>
From:   Christophe Leroy <christophe.leroy@c-s.fr>
Subject: [PATCH 3/3] powerpc: use __builtin_trap() in BUG/WARN macros.
To:     Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        Michael Ellerman <mpe@ellerman.id.au>,
        segher@kernel.crashing.org
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
Date:   Mon, 19 Aug 2019 13:06:31 +0000 (UTC)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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