Skip to contentSkip to conversation

arm64: Use BRK instruction for generic BUG traps

PATCH New Dave Martin 2 participants 18 messages Jun 11, 2015 – Jun 17, 2015 arm64

lore
Share & tools
lore

Dave Martin

author
#0 lore raw

cover arm64: Use BRK instruction for generic BUG traps

Currently, the minimal default BUG() implementation from asm-generic is used for arm64.

This series uses the BRK software breakpoint instruction to generate a trap instead, similarly to most other arches, with the generic BUG code generating the dmesg boilerplate. This eliminates a fair amount of inlined code at BUG() and WARN() sites.

This work makes it look increasingly desirable to collect BRK immediates together in one place. Patches 1-7 do some refactoring to prepare for this, and patch 8 moves the definitions to a fresh header, <asm/brk.h>.

Patch 9 provides the BRK-based GENERIC_BUG support for arm64.

A side-effect of this change is that WARNs are now generated via a different bit of generic code (lib/bug.c:report_bug()) that no longer prints a backtrace (compare kernel/panic.c:warn_slowpath_common()).) I will post a separate mini-RFC series to address that in the generic code. Patch 10 hacks a backtrace back into the arm64 arch code in the meantime.

Comments and testing welcome.

Quick testing suggests a kernel size reduction of ~110K for arm64 defconfig.

Dave Martin (10):
arm64/debug: Eliminate magic number for size of BRK instruction arm64/debug: Mask off all reserved bits from generated ESR values
arm64: esr.h type fixes and cleanup
arm64/debug: Eliminate magic number from ESR template definition arm64/debug: More consistent naming for the BRK ESR template macro arm64/debug: Move BRK ESR template macro into <asm/esr.h> arm64/debug: Simplify BRK insn opcode declarations arm64/debug: Move BRK types to a separate header arm64/BUG: Use BRK instruction for generic BUG traps arm64/BUG: Show explicit backtrace for WARNs

 arch/arm64/Kconfig                      |    8 ++
 arch/arm64/include/asm/brk.h            |   32 ++++++++
 arch/arm64/include/asm/bug.h            |   64 +++++++++++++++
 arch/arm64/include/asm/debug-monitors.h |   45 +++--------
 arch/arm64/include/asm/esr.h            |  135 ++++++++++++++++---------------
 arch/arm64/include/asm/memory.h         |    3 +-
 arch/arm64/kernel/kgdb.c                |   12 +--
 arch/arm64/kernel/traps.c               |   60 +++++++++++++-
 arch/arm64/mm/fault.c                   |   12 ++-
 9 files changed, 262 insertions(+), 109 deletions(-)
 create mode 100644 arch/arm64/include/asm/brk.h
 create mode 100644 arch/arm64/include/asm/bug.h
Signature · 1 line
1.7.10.4
author
#1 lore raw

PATCH 1/10 arm64/debug: Eliminate magic number for size of BRK instruction

The size of an A64 BRK instruction is the same as the size of all other A64 instructions, because all A64 instructions are the same size.

BREAK_INSTR_SIZE is retained for readibility, but it should not be an independent constant from AARCH64_INSN_SIZE.

Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/include/asm/debug-monitors.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
arch/arm64/include/asm/debug-monitors.h +3 -1
index 40ec68a..f3d2dbd 100644
@@ -18,6 +18,8 @@
 
 #ifdef __KERNEL__
 
+#include <asm/insn.h>
+
 /* Low-level stepping controls. */
 #define DBG_MDSCR_SS		(1 << 0)
 #define DBG_SPSR_SS		(1 << 21)

@@ -38,7 +40,7 @@
 /*
  * Break point instruction encoding
  */
-#define BREAK_INSTR_SIZE		4
+#define BREAK_INSTR_SIZE		AARCH64_INSN_SIZE
 
 /*
  * ESR values expected for dynamic and compile time BRK instruction
Signature · 1 line
1.7.10.4
author
#2 lore raw

PATCH 2/10 arm64/debug: Mask off all reserved bits from generated ESR values

In reply to Dave Martin

There are only 16 comment bits in a BRK instruction, which correspond to ESR bits 15:0. Bits 24:16 of the ESR are RES0, and might have weird meanings in the future.

This code inserts 16 bits of comment in the ESR value instead of 20 (almost certainly a typo in the original code).

Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/include/asm/debug-monitors.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/include/asm/debug-monitors.h +1 -1
index f3d2dbd..ab7d5a8 100644
@@ -45,7 +45,7 @@
 /*
  * ESR values expected for dynamic and compile time BRK instruction
  */
-#define DBG_ESR_VAL_BRK(x)	(0xf2000000 | ((x) & 0xfffff))
+#define DBG_ESR_VAL_BRK(x)	(0xf2000000 | ((x) & 0xffff))
 
 /*
  * #imm16 values used for BRK instruction generation
Signature · 1 line
1.7.10.4
author
#3 lore raw

PATCH 3/10 arm64: esr.h type fixes and cleanup

In reply to Dave Martin

ESR_ELx is a 32-bit register, so it is not necessary for all the template values defined by esr.h to be forced to 64-bit (long).

This patch introduces a UINT() macro analogous to UL(), and applies it consistently. (Unfortunately, the more succinct U and UI names are already used in unrelated code, and cause conflicts since memory.h is widely included.)

Since this change touches many lines already, I've taken the opportunity to squash some redundant parentheses and bogus whitespace at the same time.

The missing include of <asm/memory.h> (for UL(), UINT() etc.) is also added.

No functional change.

Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/include/asm/esr.h    |  128 ++++++++++++++++++++-------------------
 arch/arm64/include/asm/memory.h |    3 +-
 2 files changed, 67 insertions(+), 64 deletions(-)
arch/arm64/include/asm/esr.h +65 -63
index 7052245..8dab2a9 100644
@@ -18,86 +18,88 @@
 #ifndef __ASM_ESR_H
 #define __ASM_ESR_H
 
-#define ESR_ELx_EC_UNKNOWN	(0x00)
-#define ESR_ELx_EC_WFx		(0x01)
+#include <asm/memory.h>
+
+#define ESR_ELx_EC_UNKNOWN	UINT(0x00)
+#define ESR_ELx_EC_WFx		UINT(0x01)
 /* Unallocated EC: 0x02 */
-#define ESR_ELx_EC_CP15_32	(0x03)
-#define ESR_ELx_EC_CP15_64	(0x04)
-#define ESR_ELx_EC_CP14_MR	(0x05)
-#define ESR_ELx_EC_CP14_LS	(0x06)
-#define ESR_ELx_EC_FP_ASIMD	(0x07)
-#define ESR_ELx_EC_CP10_ID	(0x08)
+#define ESR_ELx_EC_CP15_32	UINT(0x03)
+#define ESR_ELx_EC_CP15_64	UINT(0x04)
+#define ESR_ELx_EC_CP14_MR	UINT(0x05)
+#define ESR_ELx_EC_CP14_LS	UINT(0x06)
+#define ESR_ELx_EC_FP_ASIMD	UINT(0x07)
+#define ESR_ELx_EC_CP10_ID	UINT(0x08)
 /* Unallocated EC: 0x09 - 0x0B */
-#define ESR_ELx_EC_CP14_64	(0x0C)
+#define ESR_ELx_EC_CP14_64	UINT(0x0C)
 /* Unallocated EC: 0x0d */
-#define ESR_ELx_EC_ILL		(0x0E)
+#define ESR_ELx_EC_ILL		UINT(0x0E)
 /* Unallocated EC: 0x0F - 0x10 */
-#define ESR_ELx_EC_SVC32	(0x11)
-#define ESR_ELx_EC_HVC32	(0x12)
-#define ESR_ELx_EC_SMC32	(0x13)
+#define ESR_ELx_EC_SVC32	UINT(0x11)
+#define ESR_ELx_EC_HVC32	UINT(0x12)
+#define ESR_ELx_EC_SMC32	UINT(0x13)
 /* Unallocated EC: 0x14 */
-#define ESR_ELx_EC_SVC64	(0x15)
-#define ESR_ELx_EC_HVC64	(0x16)
-#define ESR_ELx_EC_SMC64	(0x17)
-#define ESR_ELx_EC_SYS64	(0x18)
+#define ESR_ELx_EC_SVC64	UINT(0x15)
+#define ESR_ELx_EC_HVC64	UINT(0x16)
+#define ESR_ELx_EC_SMC64	UINT(0x17)
+#define ESR_ELx_EC_SYS64	UINT(0x18)
 /* Unallocated EC: 0x19 - 0x1E */
-#define ESR_ELx_EC_IMP_DEF	(0x1f)
-#define ESR_ELx_EC_IABT_LOW	(0x20)
-#define ESR_ELx_EC_IABT_CUR	(0x21)
-#define ESR_ELx_EC_PC_ALIGN	(0x22)
+#define ESR_ELx_EC_IMP_DEF	UINT(0x1f)
+#define ESR_ELx_EC_IABT_LOW	UINT(0x20)
+#define ESR_ELx_EC_IABT_CUR	UINT(0x21)
+#define ESR_ELx_EC_PC_ALIGN	UINT(0x22)
 /* Unallocated EC: 0x23 */
-#define ESR_ELx_EC_DABT_LOW	(0x24)
-#define ESR_ELx_EC_DABT_CUR	(0x25)
-#define ESR_ELx_EC_SP_ALIGN	(0x26)
+#define ESR_ELx_EC_DABT_LOW	UINT(0x24)
+#define ESR_ELx_EC_DABT_CUR	UINT(0x25)
+#define ESR_ELx_EC_SP_ALIGN	UINT(0x26)
 /* Unallocated EC: 0x27 */
-#define ESR_ELx_EC_FP_EXC32	(0x28)
+#define ESR_ELx_EC_FP_EXC32	UINT(0x28)
 /* Unallocated EC: 0x29 - 0x2B */
-#define ESR_ELx_EC_FP_EXC64	(0x2C)
+#define ESR_ELx_EC_FP_EXC64	UINT(0x2C)
 /* Unallocated EC: 0x2D - 0x2E */
-#define ESR_ELx_EC_SERROR	(0x2F)
-#define ESR_ELx_EC_BREAKPT_LOW	(0x30)
-#define ESR_ELx_EC_BREAKPT_CUR	(0x31)
-#define ESR_ELx_EC_SOFTSTP_LOW	(0x32)
-#define ESR_ELx_EC_SOFTSTP_CUR	(0x33)
-#define ESR_ELx_EC_WATCHPT_LOW	(0x34)
-#define ESR_ELx_EC_WATCHPT_CUR	(0x35)
+#define ESR_ELx_EC_SERROR	UINT(0x2F)
+#define ESR_ELx_EC_BREAKPT_LOW	UINT(0x30)
+#define ESR_ELx_EC_BREAKPT_CUR	UINT(0x31)
+#define ESR_ELx_EC_SOFTSTP_LOW	UINT(0x32)
+#define ESR_ELx_EC_SOFTSTP_CUR	UINT(0x33)
+#define ESR_ELx_EC_WATCHPT_LOW	UINT(0x34)
+#define ESR_ELx_EC_WATCHPT_CUR	UINT(0x35)
 /* Unallocated EC: 0x36 - 0x37 */
-#define ESR_ELx_EC_BKPT32	(0x38)
+#define ESR_ELx_EC_BKPT32	UINT(0x38)
 /* Unallocated EC: 0x39 */
-#define ESR_ELx_EC_VECTOR32	(0x3A)
+#define ESR_ELx_EC_VECTOR32	UINT(0x3A)
 /* Unallocted EC: 0x3B */
-#define ESR_ELx_EC_BRK64	(0x3C)
+#define ESR_ELx_EC_BRK64	UINT(0x3C)
 /* Unallocated EC: 0x3D - 0x3F */
-#define ESR_ELx_EC_MAX		(0x3F)
+#define ESR_ELx_EC_MAX		UINT(0x3F)
 
-#define ESR_ELx_EC_SHIFT	(26)
-#define ESR_ELx_EC_MASK		(UL(0x3F) << ESR_ELx_EC_SHIFT)
+#define ESR_ELx_EC_SHIFT	26
+#define ESR_ELx_EC_MASK		(ESR_ELx_EC_MAX << ESR_ELx_EC_SHIFT)
 
-#define ESR_ELx_IL		(UL(1) << 25)
+#define ESR_ELx_IL		(UINT(1) << 25)
 #define ESR_ELx_ISS_MASK	(ESR_ELx_IL - 1)
-#define ESR_ELx_ISV		(UL(1) << 24)
-#define ESR_ELx_SAS_SHIFT	(22)
-#define ESR_ELx_SAS		(UL(3) << ESR_ELx_SAS_SHIFT)
-#define ESR_ELx_SSE		(UL(1) << 21)
-#define ESR_ELx_SRT_SHIFT	(16)
-#define ESR_ELx_SRT_MASK	(UL(0x1F) << ESR_ELx_SRT_SHIFT)
-#define ESR_ELx_SF 		(UL(1) << 15)
-#define ESR_ELx_AR 		(UL(1) << 14)
-#define ESR_ELx_EA 		(UL(1) << 9)
-#define ESR_ELx_CM 		(UL(1) << 8)
-#define ESR_ELx_S1PTW 		(UL(1) << 7)
-#define ESR_ELx_WNR		(UL(1) << 6)
-#define ESR_ELx_FSC		(0x3F)
-#define ESR_ELx_FSC_TYPE	(0x3C)
-#define ESR_ELx_FSC_EXTABT	(0x10)
-#define ESR_ELx_FSC_ACCESS	(0x08)
-#define ESR_ELx_FSC_FAULT	(0x04)
-#define ESR_ELx_FSC_PERM	(0x0C)
-#define ESR_ELx_CV		(UL(1) << 24)
-#define ESR_ELx_COND_SHIFT	(20)
-#define ESR_ELx_COND_MASK	(UL(0xF) << ESR_ELx_COND_SHIFT)
-#define ESR_ELx_WFx_ISS_WFE	(UL(1) << 0)
-#define ESR_ELx_xVC_IMM_MASK	((1UL << 16) - 1)
+#define ESR_ELx_ISV		(UINT(1) << 24)
+#define ESR_ELx_SAS_SHIFT	22
+#define ESR_ELx_SAS		(UINT(3) << ESR_ELx_SAS_SHIFT)
+#define ESR_ELx_SSE		(UINT(1) << 21)
+#define ESR_ELx_SRT_SHIFT	16
+#define ESR_ELx_SRT_MASK	(UINT(0x1F) << ESR_ELx_SRT_SHIFT)
+#define ESR_ELx_SF		(UINT(1) << 15)
+#define ESR_ELx_AR		(UINT(1) << 14)
+#define ESR_ELx_EA		(UINT(1) << 9)
+#define ESR_ELx_CM		(UINT(1) << 8)
+#define ESR_ELx_S1PTW		(UINT(1) << 7)
+#define ESR_ELx_WNR		(UINT(1) << 6)
+#define ESR_ELx_FSC		UINT(0x3F)
+#define ESR_ELx_FSC_TYPE	UINT(0x3C)
+#define ESR_ELx_FSC_EXTABT	UINT(0x10)
+#define ESR_ELx_FSC_ACCESS	UINT(0x08)
+#define ESR_ELx_FSC_FAULT	UINT(0x04)
+#define ESR_ELx_FSC_PERM	UINT(0x0C)
+#define ESR_ELx_CV		(UINT(1) << 24)
+#define ESR_ELx_COND_SHIFT	20
+#define ESR_ELx_COND_MASK	(UINT(0xF) << ESR_ELx_COND_SHIFT)
+#define ESR_ELx_WFx_ISS_WFE	(UINT(1) << 0)
+#define ESR_ELx_xVC_IMM_MASK	((UINT(1) << 16) - 1)
 
 #ifndef __ASSEMBLY__
 #include <asm/types.h>
arch/arm64/include/asm/memory.h +2 -1
index f800d45..c6a6592a 100644
@@ -28,9 +28,10 @@
 
 /*
  * Allow for constants defined here to be used from assembly code
- * by prepending the UL suffix only with actual C code compilation.
+ * by prepending type suffixes only with actual C code compilation.
  */
 #define UL(x) _AC(x, UL)
+#define UINT(x) _AC(x, U)
 
 /*
  * Size of the PCI I/O space. This must remain a power of two so that
Signature · 1 line
1.7.10.4
author
#4 lore raw

PATCH 4/10 arm64/debug: Eliminate magic number from ESR template definition

In reply to Dave Martin

<asm/esr.h> has perfectly good constants for defining ESR values already. Let's use them.

Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/include/asm/debug-monitors.h |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
arch/arm64/include/asm/debug-monitors.h +3 -1
index ab7d5a8..ff09058 100644
@@ -18,6 +18,7 @@
 
 #ifdef __KERNEL__
 
+#include <asm/esr.h>
 #include <asm/insn.h>
 
 /* Low-level stepping controls. */

@@ -45,7 +46,8 @@
 /*
  * ESR values expected for dynamic and compile time BRK instruction
  */
-#define DBG_ESR_VAL_BRK(x)	(0xf2000000 | ((x) & 0xffff))
+#define DBG_ESR_VAL_BRK(x) \
+	((ESR_ELx_EC_BRK64 << ESR_ELx_EC_SHIFT) | ESR_ELx_IL | ((x) & 0xffff))
 
 /*
  * #imm16 values used for BRK instruction generation
Signature · 1 line
1.7.10.4
author
#5 lore raw

PATCH 5/10 arm64/debug: More consistent naming for the BRK ESR template macro

In reply to Dave Martin

The naming of DBG_ESR_VAL_BRK is inconsistent with the way other similar macros are named.

This patch makes the naming more consistent, and appends "64" as a reminder that this ESR pattern only matches from AArch64 state.

Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/include/asm/debug-monitors.h |    5 +++--
 arch/arm64/kernel/kgdb.c                |    4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)
arch/arm64/include/asm/debug-monitors.h +3 -2
index ff09058..bb97e9d 100644
@@ -46,8 +46,9 @@
 /*
  * ESR values expected for dynamic and compile time BRK instruction
  */
-#define DBG_ESR_VAL_BRK(x) \
-	((ESR_ELx_EC_BRK64 << ESR_ELx_EC_SHIFT) | ESR_ELx_IL | ((x) & 0xffff))
+#define ESR_ELx_VAL_BRK64(imm)					\
+	((ESR_ELx_EC_BRK64 << ESR_ELx_EC_SHIFT) | ESR_ELx_IL |	\
+	 ((imm) & 0xffff))
 
 /*
  * #imm16 values used for BRK instruction generation
arch/arm64/kernel/kgdb.c +2 -2
index a0d10c5..a5a838e 100644
@@ -235,13 +235,13 @@ static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
 
 static struct break_hook kgdb_brkpt_hook = {
 	.esr_mask	= 0xffffffff,
-	.esr_val	= DBG_ESR_VAL_BRK(KGDB_DYN_DBG_BRK_IMM),
+	.esr_val	= ESR_ELx_VAL_BRK64(KGDB_DYN_DBG_BRK_IMM),
 	.fn		= kgdb_brk_fn
 };
 
 static struct break_hook kgdb_compiled_brkpt_hook = {
 	.esr_mask	= 0xffffffff,
-	.esr_val	= DBG_ESR_VAL_BRK(KGDB_COMPILED_DBG_BRK_IMM),
+	.esr_val	= ESR_ELx_VAL_BRK64(KGDB_COMPILED_DBG_BRK_IMM),
 	.fn		= kgdb_compiled_brk_fn
 };
Signature · 1 line
1.7.10.4
author
#6 lore raw

PATCH 6/10 arm64/debug: Move BRK ESR template macro into <asm/esr.h>

In reply to Dave Martin

It makes sense to keep all the architectural exception syndrome definitions in the same place.

Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/include/asm/debug-monitors.h |    7 -------
 arch/arm64/include/asm/esr.h            |    7 +++++++
 2 files changed, 7 insertions(+), 7 deletions(-)
arch/arm64/include/asm/debug-monitors.h +0 -7
index bb97e9d..e28b1dd 100644
@@ -44,13 +44,6 @@
 #define BREAK_INSTR_SIZE		AARCH64_INSN_SIZE
 
 /*
- * ESR values expected for dynamic and compile time BRK instruction
- */
-#define ESR_ELx_VAL_BRK64(imm)					\
-	((ESR_ELx_EC_BRK64 << ESR_ELx_EC_SHIFT) | ESR_ELx_IL |	\
-	 ((imm) & 0xffff))
-
-/*
  * #imm16 values used for BRK instruction generation
  * Allowed values for kgbd are 0x400 - 0x7ff
  * 0x100: for triggering a fault on purpose (reserved)
arch/arm64/include/asm/esr.h +7 -0
index 8dab2a9..7568d14 100644
@@ -101,6 +101,13 @@
 #define ESR_ELx_WFx_ISS_WFE	(UINT(1) << 0)
 #define ESR_ELx_xVC_IMM_MASK	((UINT(1) << 16) - 1)
 
+/* ESR value templates for specific events */
+
+/* BRK instruction trap from AArch64 state */
+#define ESR_ELx_VAL_BRK64(imm)					\
+	((ESR_ELx_EC_BRK64 << ESR_ELx_EC_SHIFT) | ESR_ELx_IL |	\
+	 ((imm) & 0xffff))
+
 #ifndef __ASSEMBLY__
 #include <asm/types.h>
Signature · 1 line
1.7.10.4
author
#7 lore raw

PATCH 7/10 arm64/debug: Simplify BRK insn opcode declarations

In reply to Dave Martin

The way the KGDB_DYN_BRK_INS_BYTEx macros are declared is more complex than it needs to be. Also, the macros are only used in one place, which is arch-specific anyway.

This patch refactors the macros to simplify them, and exposes an argument so that we can have a single macro instead of 4.

As a side effect, this patch also fixes some anomalous spellings of "KGDB".

Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/include/asm/debug-monitors.h |   23 ++++-------------------
 arch/arm64/kernel/kgdb.c                |    8 ++++----
 2 files changed, 8 insertions(+), 23 deletions(-)
arch/arm64/include/asm/debug-monitors.h +4 -19
index e28b1dd..6a17fb8 100644
@@ -66,25 +66,10 @@
  */
 #define AARCH64_BREAK_FAULT	(AARCH64_BREAK_MON | (FAULT_BRK_IMM << 5))
 
-/*
- * Extract byte from BRK instruction
- */
-#define KGDB_DYN_DBG_BRK_INS_BYTE(x) \
-	((((AARCH64_BREAK_MON) & 0xffe0001f) >> (x * 8)) & 0xff)
-
-/*
- * Extract byte from BRK #imm16
- */
-#define KGBD_DYN_DBG_BRK_IMM_BYTE(x) \
-	(((((KGDB_DYN_DBG_BRK_IMM) & 0xffff) << 5) >> (x * 8)) & 0xff)
-
-#define KGDB_DYN_DBG_BRK_BYTE(x) \
-	(KGDB_DYN_DBG_BRK_INS_BYTE(x) | KGBD_DYN_DBG_BRK_IMM_BYTE(x))
-
-#define  KGDB_DYN_BRK_INS_BYTE0  KGDB_DYN_DBG_BRK_BYTE(0)
-#define  KGDB_DYN_BRK_INS_BYTE1  KGDB_DYN_DBG_BRK_BYTE(1)
-#define  KGDB_DYN_BRK_INS_BYTE2  KGDB_DYN_DBG_BRK_BYTE(2)
-#define  KGDB_DYN_BRK_INS_BYTE3  KGDB_DYN_DBG_BRK_BYTE(3)
+#define AARCH64_BREAK_KGDB_DYN_DBG	\
+	(AARCH64_BREAK_MON | (KGDB_DYN_DBG_BRK_IMM << 5))
+#define KGDB_DYN_BRK_INS_BYTE(x)	\
+	((AARCH64_BREAK_KGDB_DYN_DBG >> (8 * (x))) & 0xff)
 
 #define CACHE_FLUSH_IS_SAFE		1
arch/arm64/kernel/kgdb.c +4 -4
index a5a838e..4f78b8a 100644
@@ -328,9 +328,9 @@ void kgdb_arch_exit(void)
  */
 struct kgdb_arch arch_kgdb_ops = {
 	.gdb_bpt_instr = {
-		KGDB_DYN_BRK_INS_BYTE0,
-		KGDB_DYN_BRK_INS_BYTE1,
-		KGDB_DYN_BRK_INS_BYTE2,
-		KGDB_DYN_BRK_INS_BYTE3,
+		KGDB_DYN_BRK_INS_BYTE(0),
+		KGDB_DYN_BRK_INS_BYTE(1),
+		KGDB_DYN_BRK_INS_BYTE(2),
+		KGDB_DYN_BRK_INS_BYTE(3),
 	}
 };
Signature · 1 line
1.7.10.4
author
#8 lore raw

PATCH 8/10 arm64/debug: Move BRK types to a separate header

In reply to Dave Martin

This patch creates a new header <asm/brk.h> for the purpose of enumerating all breakpoint types used by the kernel.

Having a clear place for this information should reduce maintenance headaches.

Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/include/asm/brk.h            |   31 +++++++++++++++++++++++++++++++
 arch/arm64/include/asm/debug-monitors.h |   12 +-----------
 2 files changed, 32 insertions(+), 11 deletions(-)
 create mode 100644 arch/arm64/include/asm/brk.h
arch/arm64/include/asm/brk.h new file +31 -0
new file mode 100644
index 0000000..99b8dfb
@@ -0,0 +1,31 @@
+/*
+ * Enumeration of AArch64 software breakpoint types
+ * Copyright (C) 2012-2015  ARM Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef _ARCH_ARM64_ASM_BRK_H
+#define _ARCH_ARM64_ASM_BRK_H
+
+/*
+ * #imm16 values used for BRK instruction generation
+ * Allowed values for kgbd are 0x400 - 0x7ff
+ * 0x100: for triggering a fault on purpose (reserved)
+ * 0x400: for dynamic BRK instruction
+ * 0x401: for compile time BRK instruction
+ */
+#define FAULT_BRK_IMM			0x100
+#define KGDB_DYN_DBG_BRK_IMM		0x400
+#define KGDB_COMPILED_DBG_BRK_IMM	0x401
+
+#endif /* ! _ARCH_ARM64_ASM_BRK_H */
arch/arm64/include/asm/debug-monitors.h +1 -11
index 6a17fb8..78611ee 100644
@@ -18,6 +18,7 @@
 
 #ifdef __KERNEL__
 
+#include <asm/brk.h>
 #include <asm/esr.h>
 #include <asm/insn.h>
 

@@ -44,17 +45,6 @@
 #define BREAK_INSTR_SIZE		AARCH64_INSN_SIZE
 
 /*
- * #imm16 values used for BRK instruction generation
- * Allowed values for kgbd are 0x400 - 0x7ff
- * 0x100: for triggering a fault on purpose (reserved)
- * 0x400: for dynamic BRK instruction
- * 0x401: for compile time BRK instruction
- */
-#define FAULT_BRK_IMM			0x100
-#define KGDB_DYN_DBG_BRK_IMM		0x400
-#define KGDB_COMPILED_DBG_BRK_IMM	0x401
-
-/*
  * BRK instruction encoding
  * The #imm16 value should be placed at bits[20:5] within BRK ins
  */
Signature · 1 line
1.7.10.4
author
#9 lore raw

PATCH 9/10 arm64/BUG: Use BRK instruction for generic BUG traps

In reply to Dave Martin

Currently, the minimal default BUG() implementation from asm- generic is used for arm64.

This patch uses the BRK software breakpoint instruction to generate a trap instead, similarly to most other arches, with the generic BUG code generating the dmesg boilerplate.

This allows bug metadata to be moved to a separate table and reduces the amount of inline code at BUG and WARN sites. This also avoids clobbering any registers before they can be dumped.

To mitigate the size of the bug table further, this patch makes use of the existing infrastructure for encoding addresses within the bug table as 32-bit offsets instead of absolute pointers. (Note that this limits the kernel size to 2GB.)

Traps are registered at arch_initcall time for aarch64, but BUG has minimal real dependencies and it is desirable to be able to generate bug splats as early as possible. This patch redirects all debug exceptions caused by BRK directly to bug_handler() until the full debug exception support has been initialised.

Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/Kconfig           |    8 ++++++
 arch/arm64/include/asm/brk.h |    1 +
 arch/arm64/include/asm/bug.h |   64 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/traps.c    |   57 ++++++++++++++++++++++++++++++++++++-
 arch/arm64/mm/fault.c        |   12 ++++++--
 5 files changed, 139 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm64/include/asm/bug.h
arch/arm64/Kconfig +8 -0
index 7796af4..aedda42 100644
@@ -110,6 +110,14 @@ config TRACE_IRQFLAGS_SUPPORT
 config RWSEM_XCHGADD_ALGORITHM
 	def_bool y
 
+config GENERIC_BUG
+	def_bool y
+	depends on BUG
+
+config GENERIC_BUG_RELATIVE_POINTERS
+	def_bool y
+	depends on GENERIC_BUG
+
 config GENERIC_HWEIGHT
 	def_bool y
arch/arm64/include/asm/brk.h +1 -0
index 99b8dfb..f4d5894 100644
@@ -27,5 +27,6 @@
 #define FAULT_BRK_IMM			0x100
 #define KGDB_DYN_DBG_BRK_IMM		0x400
 #define KGDB_COMPILED_DBG_BRK_IMM	0x401
+#define BUG_BRK_IMM			0x7ff
 
 #endif /* ! _ARCH_ARM64_ASM_BRK_H */
arch/arm64/include/asm/bug.h new file +64 -0

64 lines not on this page · raw message

arch/arm64/kernel/traps.c +56 -1

77 lines not on this page · raw message

arch/arm64/mm/fault.c +10 -2
index 96da131..c7c5eeb 100644
@@ -486,14 +486,22 @@ asmlinkage void __exception do_sp_pc_abort(unsigned long addr,
 	arm64_notify_die("", regs, &info, esr);
 }
 
-static struct fault_info debug_fault_info[] = {
+int __init early_brk64(unsigned long addr, unsigned int esr,
+		       struct pt_regs *regs);
+
+/*
+ * __refdata because early_brk64 is __init, but the reference to it is
+ * clobbered at arch_initcall time.
+ * See traps.c and debug-monitors.c:debug_traps_init().
+ */
+static struct fault_info __refdata debug_fault_info[] = {
 	{ do_bad,	SIGTRAP,	TRAP_HWBKPT,	"hardware breakpoint"	},
 	{ do_bad,	SIGTRAP,	TRAP_HWBKPT,	"hardware single-step"	},
 	{ do_bad,	SIGTRAP,	TRAP_HWBKPT,	"hardware watchpoint"	},
 	{ do_bad,	SIGBUS,		0,		"unknown 3"		},
 	{ do_bad,	SIGTRAP,	TRAP_BRKPT,	"aarch32 BKPT"		},
 	{ do_bad,	SIGTRAP,	0,		"aarch32 vector catch"	},
-	{ do_bad,	SIGTRAP,	TRAP_BRKPT,	"aarch64 BRK"		},
+	{ early_brk64,	SIGTRAP,	TRAP_BRKPT,	"aarch64 BRK"		},
 	{ do_bad,	SIGBUS,		0,		"unknown 7"		},
 };
Signature · 1 line
1.7.10.4
#10 lore raw

PATCH 9/10 arm64/BUG: Use BRK instruction for generic BUG traps

Hi Dave,

Just a few comments, inline.

Quoted 56 lines · Dave P Martin

Currently, the minimal default BUG() implementation from asm- generic is used for arm64.

This patch uses the BRK software breakpoint instruction to generate a trap instead, similarly to most other arches, with the generic BUG code generating the dmesg boilerplate.

This allows bug metadata to be moved to a separate table and reduces the amount of inline code at BUG and WARN sites. This also avoids clobbering any registers before they can be dumped.

To mitigate the size of the bug table further, this patch makes use of the existing infrastructure for encoding addresses within the bug table as 32-bit offsets instead of absolute pointers. (Note that this limits the kernel size to 2GB.)

Traps are registered at arch_initcall time for aarch64, but BUG has minimal real dependencies and it is desirable to be able to generate bug splats as early as possible. This patch redirects all debug exceptions caused by BRK directly to bug_handler() until the full debug exception support has been initialised.

Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/Kconfig           |    8 ++++++
 arch/arm64/include/asm/brk.h |    1 +
 arch/arm64/include/asm/bug.h |   64 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/traps.c    |   57 ++++++++++++++++++++++++++++++++++++-
 arch/arm64/mm/fault.c        |   12 ++++++--
 5 files changed, 139 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm64/include/asm/bug.h
arch/arm64/Kconfig +8 -0
index 7796af4..aedda42 100644
@@ -110,6 +110,14 @@ config TRACE_IRQFLAGS_SUPPORT
 config RWSEM_XCHGADD_ALGORITHM
 	def_bool y
 
+config GENERIC_BUG
+	def_bool y
+	depends on BUG
+
+config GENERIC_BUG_RELATIVE_POINTERS
+	def_bool y
+	depends on GENERIC_BUG
+
 config GENERIC_HWEIGHT
 	def_bool y
arch/arm64/include/asm/brk.h +0 -0
index 99b8dfb..f4d5894 100644

arch/arm64/include/asm/brk.h line 30

Quoted 4 lines
arch/arm64/include/asm/brk.h +1 -0
@@ -27,5 +27,6 @@
 #define FAULT_BRK_IMM			0x100
 #define KGDB_DYN_DBG_BRK_IMM		0x400
 #define KGDB_COMPILED_DBG_BRK_IMM	0x401
+#define BUG_BRK_IMM			0x7ff

Just curious, but how did you come up with this number?

Quoted 16 lines
arch/arm64/include/asm/brk.h +0 -0
 #endif /* ! _ARCH_ARM64_ASM_BRK_H */
arch/arm64/include/asm/bug.h new file +9 -0
new file mode 100644
index 0000000..0429c7b
@@ -0,0 +1,64 @@
+/*
+ * Copyright (C) 2015  ARM Limited
+ * Author: Dave Martin <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,

arch/arm64/include/asm/bug.h line 19

Quoted 10 lines
arch/arm64/include/asm/bug.h new file +10 -0
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _ARCH_ARM64_ASM_BUG_H
+#define _ARCH_ARM64_ASM_BUG_H

Please follow the standard header guard pattern we use for arm64 (__ASM_).

Quoted 70 lines
arch/arm64/include/asm/bug.h new file +45 -0
+
+#include <asm/brk.h>
+
+#ifdef CONFIG_GENERIC_BUG
+#define HAVE_ARCH_BUG
+
+#ifdef CONFIG_DEBUG_BUGVERBOSE
+#define _BUGVERBOSE_LOCATION(file, line) __BUGVERBOSE_LOCATION(file, line)
+#define __BUGVERBOSE_LOCATION(file, line)				\
+		".pushsection .rodata.str,\"aMS\", at progbits,1\n"	\
+	"2:	.string \"" file "\"\n\t"				\
+		".popsection\n\t"					\
+									\
+		".long 2b - 0b\n\t"					\
+		".short " #line "\n\t"
+#else
+#define _BUGVERBOSE_LOCATION(file, line)
+#endif
+
+#define _BUG_FLAGS(flags) __BUG_FLAGS(flags)
+
+#define __BUG_FLAGS(flags) asm volatile (		\
+		".pushsection __bug_table,\"a\"\n\t"	\
+		".align 2\n\t"				\
+	"0:	.long 1f - 0b\n\t"			\
+_BUGVERBOSE_LOCATION(__FILE__, __LINE__)		\
+		".short " #flags "\n\t"			\
+		".popsection\n"				\
+							\
+	"1:	brk %[imm]"				\
+		:: [imm] "i" (BUG_BRK_IMM)		\
+)
+
+#define BUG() do {				\
+	_BUG_FLAGS(0);				\
+	unreachable();				\
+} while (0)
+
+#define __WARN_TAINT(taint) _BUG_FLAGS(BUGFLAG_TAINT(taint))
+
+#endif /* ! CONFIG_GENERIC_BUG */
+
+#include <asm-generic/bug.h>
+
+#endif /* ! _ARCH_ARM64_ASM_BUG_H */
arch/arm64/kernel/traps.c +4 -0
index 1ef2940..5fdf776 100644
@@ -17,6 +17,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/bug.h>
 #include <linux/signal.h>
 #include <linux/personality.h>
 #include <linux/kallsyms.h>

@@ -32,8 +33,11 @@
 #include <linux/syscalls.h>
 
 #include <asm/atomic.h>
+#include <asm/brk.h>
+#include <asm/bug.h>
 #include <asm/debug-monitors.h>
 #include <asm/esr.h>
+#include <asm/insn.h>
 #include <asm/traps.h>
 #include <asm/stacktrace.h>
 #include <asm/exception.h>

arch/arm64/kernel/traps.c void __pgd_error(const char *file, int line, unsigned long val) · line 472

Quoted 9 lines
arch/arm64/kernel/traps.c +6 -0
@@ -460,7 +464,58 @@ void __pgd_error(const char *file, int line, unsigned long val)
 	pr_crit("%s:%d: bad pgd %016lx.\n", file, line, val);
 }
 
+/* GENERIC_BUG traps */
+
+int is_valid_bugaddr(unsigned long addr)
+{
+	/*
+	 * bug_handler() only called for BUG #BUG_BRK_IMM.

s/BUG/BRK/ ?

arch/arm64/kernel/traps.c void __pgd_error(const char *file, int line, unsigned long val) · line 479

Quoted 7 lines
arch/arm64/kernel/traps.c +7 -0
+	 * So the answer is trivial -- any spurious instances with no
+	 * bug table entry will be rejected by report_bug() and passed
+	 * back to the debug-monitors code and handled as a fatal
+	 * unexpected debug exception.
+	 */
+	return 1;
+}

Could we define is_valid_bugaddr as a macro in the header file and avoid the potential out-of-line call?

arch/arm64/kernel/traps.c void __pgd_error(const char *file, int line, unsigned long val) · line 489

Quoted 10 lines
arch/arm64/kernel/traps.c +10 -0
+
+static int bug_handler(struct pt_regs *regs, unsigned int esr)
+{
+	if (user_mode(regs))
+		return DBG_HOOK_ERROR;
+
+	switch (report_bug(regs->pc, regs)) {
+	case BUG_TRAP_TYPE_BUG:
+		die("Oops - BUG", regs, 0);
+		/* die() does not return */

Are you sure about that? A quick glance at the code didn't convince me...

Will

author
#11 lore raw

PATCH 9/10 arm64/BUG: Use BRK instruction for generic BUG traps

Quoted 36 lines · Will Deacon

Hi Dave,

Just a few comments, inline.

Quoted 31 lines · Dave P Martin

Currently, the minimal default BUG() implementation from asm- generic is used for arm64.

This patch uses the BRK software breakpoint instruction to generate a trap instead, similarly to most other arches, with the generic BUG code generating the dmesg boilerplate.

This allows bug metadata to be moved to a separate table and reduces the amount of inline code at BUG and WARN sites. This also avoids clobbering any registers before they can be dumped.

To mitigate the size of the bug table further, this patch makes use of the existing infrastructure for encoding addresses within the bug table as 32-bit offsets instead of absolute pointers. (Note that this limits the kernel size to 2GB.)

Traps are registered at arch_initcall time for aarch64, but BUG has minimal real dependencies and it is desirable to be able to generate bug splats as early as possible. This patch redirects all debug exceptions caused by BRK directly to bug_handler() until the full debug exception support has been initialised.

Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/Kconfig           |    8 ++++++
 arch/arm64/include/asm/brk.h |    1 +
 arch/arm64/include/asm/bug.h |   64 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/traps.c    |   57 ++++++++++++++++++++++++++++++++++++-
 arch/arm64/mm/fault.c        |   12 ++++++--
 5 files changed, 139 insertions(+), 3 deletions(-)
 create mode 100644 arch/arm64/include/asm/bug.h

[...]

Quoted 11 lines

arch/arm64/include/asm/brk.h line 30

Quoted 9 lines
arch/arm64/include/asm/brk.h +1 -0
index 99b8dfb..f4d5894 100644
@@ -27,5 +27,6 @@
 #define FAULT_BRK_IMM			0x100
 #define KGDB_DYN_DBG_BRK_IMM		0x400
 #define KGDB_COMPILED_DBG_BRK_IMM	0x401
+#define BUG_BRK_IMM			0x7ff

Just curious, but how did you come up with this number?

There's a comment in debug-monitors.h that the "allowed values for kgbd" (sic) are 0x400..0x7ff.

So 0x7ff seemed unlikely to clash with any other use, and well out of the way of the values that kgbd currently uses.

It's debatable that the BUG value shouldn't be in the range at all. However, BUG_BRK_IMM is a contract between the kernel and itself for EL1 only, so it can be changed at any time in the future with minimal impact.

What's your view?

[...]

Quoted 6 lines
Quoted 6 lines
arch/arm64/include/asm/bug.h new file +0 -0
new file mode 100644
index 0000000..0429c7b
@@ -0,0 +1,64 @@

[...]

Quoted 4 lines

arch/arm64/include/asm/bug.h line 19

Quoted 2 lines
arch/arm64/include/asm/bug.h new file +2 -0
+#ifndef _ARCH_ARM64_ASM_BUG_H
+#define _ARCH_ARM64_ASM_BUG_H

Please follow the standard header guard pattern we use for arm64 (__ASM_).

Hmmm, I would say I picked a bad example to copy from, but I can't see another header matching what I have.

__ASM_ is certainly the most common.
There are also a few _ASM_ (mostly ACPI/EFI) and a few _ASM_ARM64.

I'll follow the crowd and change to __ASM_.

[...]

Quoted 4 lines
Quoted 4 lines
arch/arm64/kernel/traps.c +0 -0
index 1ef2940..5fdf776 100644

[...]

Quoted 4 lines
Quoted 4 lines
arch/arm64/kernel/traps.c +0 -0
@@ -460,7 +464,58 @@ void __pgd_error(const char *file, int line, unsigned long val)
 	pr_crit("%s:%d: bad pgd %016lx.\n", file, line, val);
 }
Quoted 8 lines

arch/arm64/kernel/traps.c void __pgd_error(const char *file, int line, unsigned long val) · line 472

Quoted 6 lines
arch/arm64/kernel/traps.c +6 -0
+/* GENERIC_BUG traps */
+
+int is_valid_bugaddr(unsigned long addr)
+{
+	/*
+	 * bug_handler() only called for BUG #BUG_BRK_IMM.

s/BUG/BRK/ ?

Hmmm, yes.

Quoted 11 lines

arch/arm64/kernel/traps.c void __pgd_error(const char *file, int line, unsigned long val) · line 479

Quoted 8 lines
arch/arm64/kernel/traps.c +7 -0
+	 * So the answer is trivial -- any spurious instances with no
+	 * bug table entry will be rejected by report_bug() and passed
+	 * back to the debug-monitors code and handled as a fatal
+	 * unexpected debug exception.
+	 */
+	return 1;
+}

Could we define is_valid_bugaddr as a macro in the header file and avoid the potential out-of-line call?

We could, but it would require a change to <linux/bug.h>. Since this is slowpath anyway, I wasn't sure it was worth it.

Happy to add that and try to push it as part of this series if you like -- let me know.

Quoted 13 lines

arch/arm64/kernel/traps.c void __pgd_error(const char *file, int line, unsigned long val) · line 489

Quoted 11 lines
arch/arm64/kernel/traps.c +10 -0
+
+static int bug_handler(struct pt_regs *regs, unsigned int esr)
+{
+	if (user_mode(regs))
+		return DBG_HOOK_ERROR;
+
+	switch (report_bug(regs->pc, regs)) {
+	case BUG_TRAP_TYPE_BUG:
+		die("Oops - BUG", regs, 0);
+		/* die() does not return */

Are you sure about that? A quick glance at the code didn't convince me...

Ouch. That explains why attempting to continue a BUGged thread using kgdb wasn't working too well...

I'll fix that the re-test this behaviour.

Thanks for the review.

Cheers
---Dave

#12 lore raw

PATCH 9/10 arm64/BUG: Use BRK instruction for generic BUG traps

Hi Dave,

Quoted 20 lines · Dave P Martin
Quoted 12 lines · Will Deacon

arch/arm64/include/asm/brk.h line 30

Quoted 9 lines · Dave P Martin
arch/arm64/include/asm/brk.h +1 -0
index 99b8dfb..f4d5894 100644
@@ -27,5 +27,6 @@
 #define FAULT_BRK_IMM			0x100
 #define KGDB_DYN_DBG_BRK_IMM		0x400
 #define KGDB_COMPILED_DBG_BRK_IMM	0x401
+#define BUG_BRK_IMM			0x7ff

Just curious, but how did you come up with this number?

There's a comment in debug-monitors.h that the "allowed values for kgbd" (sic) are 0x400..0x7ff.

So 0x7ff seemed unlikely to clash with any other use, and well out of the way of the values that kgbd currently uses.

Quoted 6 lines

It's debatable that the BUG value shouldn't be in the range at all. However, BUG_BRK_IMM is a contract between the kernel and itself for EL1 only, so it can be changed at any time in the future with minimal impact.

What's your view?

I wonder if that kgdb range is inclusive? Maybe best to use 0x800 to be sure.

Quoted 8 lines
Quoted 2 lines

Could we define is_valid_bugaddr as a macro in the header file and avoid the potential out-of-line call?

We could, but it would require a change to <linux/bug.h>. Since this is slowpath anyway, I wasn't sure it was worth it.

Happy to add that and try to push it as part of this series if you like -- let me know.

Nah, it's fine. I hadn't spotted the declaration.

Will

author
#13 lore raw

PATCH 10/10 arm64/BUG: Show explicit backtrace for WARNs

In reply to Dave Martin

The generic slowpath WARN implementation prints a backtrace, but the report_bug() based implementation does not, opting to print the registers instead which is generally not as useful.

Ideally, report_bug() should be fixed to make the behaviour more consistent, but in the meantime this patch generates a backtrace directly from the arm64 backend instead so that this functionality is not lost with the migration to report_bug().

As a side-effect, the backtrace will be outside the oops end marker, but that's hard to avoid without modifying generic code.

This patch can go away if report_bug() grows the ability in the future to generate a backtrace directly or call an arch hook at the appropriate time.

Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/kernel/traps.c |    3 +++
 1 file changed, 3 insertions(+)
arch/arm64/kernel/traps.c +3 -0
index 5fdf776..8929c16 100644
@@ -489,6 +489,9 @@ static int bug_handler(struct pt_regs *regs, unsigned int esr)
 		/* die() does not return */
 
 	case BUG_TRAP_TYPE_WARN:
+		/* Ideally, report_bug() should backtrace for us... but no. */
+		dump_backtrace(regs, NULL);
+
 		regs->pc += AARCH64_INSN_SIZE;	/* skip BRK and resume */
 		return DBG_HOOK_HANDLED;
Signature · 1 line
1.7.10.4
#14 lore raw

PATCH 10/10 arm64/BUG: Show explicit backtrace for WARNs

Quoted 9 lines · Dave P Martin

The generic slowpath WARN implementation prints a backtrace, but the report_bug() based implementation does not, opting to print the registers instead which is generally not as useful.

Ideally, report_bug() should be fixed to make the behaviour more consistent, but in the meantime this patch generates a backtrace directly from the arm64 backend instead so that this functionality is not lost with the migration to report_bug().

Quoted 6 lines

As a side-effect, the backtrace will be outside the oops end marker, but that's hard to avoid without modifying generic code.

This patch can go away if report_bug() grows the ability in the future to generate a backtrace directly or call an arch hook at the appropriate time.

Could you propose a patch to the core code, please? I'm happy to merge this in the interim, but it sounds like a simple oversight.

Will

Quoted 22 lines
Signed-off-by Dave Martin <[email protected]>
---
 arch/arm64/kernel/traps.c |    3 +++
 1 file changed, 3 insertions(+)
arch/arm64/kernel/traps.c +3 -0
index 5fdf776..8929c16 100644
@@ -489,6 +489,9 @@ static int bug_handler(struct pt_regs *regs, unsigned int esr)
 		/* die() does not return */
 
 	case BUG_TRAP_TYPE_WARN:
+		/* Ideally, report_bug() should backtrace for us... but no. */
+		dump_backtrace(regs, NULL);
+
 		regs->pc += AARCH64_INSN_SIZE;	/* skip BRK and resume */
 		return DBG_HOOK_HANDLED;
Signature · 1 line
1.7.10.4
author
#15 lore raw

PATCH 10/10 arm64/BUG: Show explicit backtrace for WARNs

Quoted 13 lines · Will Deacon
Quoted 12 lines · Dave P Martin

The generic slowpath WARN implementation prints a backtrace, but the report_bug() based implementation does not, opting to print the registers instead which is generally not as useful.

Ideally, report_bug() should be fixed to make the behaviour more consistent, but in the meantime this patch generates a backtrace directly from the arm64 backend instead so that this functionality is not lost with the migration to report_bug().

As a side-effect, the backtrace will be outside the oops end marker, but that's hard to avoid without modifying generic code.

Quoted 6 lines
Quoted 3 lines

This patch can go away if report_bug() grows the ability in the future to generate a backtrace directly or call an arch hook at the appropriate time.

Could you propose a patch to the core code, please? I'm happy to merge this in the interim, but it sounds like a simple oversight.

Already done[1], but it's late in the cycle and people are understandably busy with other stuff.

The purpose of this arm64-specific patch is simply to decouple the dependency.

None of this is urgent -- I plan to repost everything after the merge window when people are less swamped.

[1] http://marc.info/?l=linux-kernel&m=143403763612397&w=2

[...]

Cheers
---Dave

#16 lore raw

cover arm64: Use BRK instruction for generic BUG traps

In reply to Dave Martin

Hi Dave,

Quoted 7 lines · Dave P Martin

Currently, the minimal default BUG() implementation from asm-generic is used for arm64.

This series uses the BRK software breakpoint instruction to generate a trap instead, similarly to most other arches, with the generic BUG code generating the dmesg boilerplate. This eliminates a fair amount of inlined code at BUG() and WARN() sites.

I think patches 3 and 8 are a bit OTT and don't really bring us much. However, for the rest of the series (modulo my minor comments):

Reviewed-by Will Deacon <[email protected]>

Cheers,

Will

author
#17 lore raw

cover arm64: Use BRK instruction for generic BUG traps

Quoted 11 lines · Will Deacon

Hi Dave,

Quoted 7 lines · Dave P Martin

Currently, the minimal default BUG() implementation from asm-generic is used for arm64.

This series uses the BRK software breakpoint instruction to generate a trap instead, similarly to most other arches, with the generic BUG code generating the dmesg boilerplate. This eliminates a fair amount of inlined code at BUG() and WARN() sites.

Quoted 5 lines

I think patches 3 and 8 are a bit OTT and don't really bring us much.

However, for the rest of the series (modulo my minor comments):

Reviewed-by Will Deacon <[email protected]>

Patch 3 is needed to silence compiler warnings that happen as a side- effect of the redefinition of DBG_ESR_VAL_BRK in patch 4.

A point hack in DBG_ESR_VAL_BRK could have fixed this, but the existing ESR definitions are the real cause -- simply stripping all the type information didn't appear safe due to the use of shifts and masking. Unfortunately, attempting to fix this properly touched more code than I would have liked...

Patch 8 was requested by Mark Rutland, who preferred a separate header for those definitions rather than allowing them to sprawl in place. It seemed a fair point, but I'm happy either way.

I can still drop the affected patches if you want -- let me know.

Cheers
---Dave