* [PATCH v2 00/10] arm64: Use BRK instruction for generic BUG traps
@ 2015-07-13 13:25 Dave Martin
2015-07-13 13:25 ` [PATCH v2 01/10] arm64/debug: Eliminate magic number for size of BRK instruction Dave Martin
` (9 more replies)
0 siblings, 10 replies; 34+ messages in thread
From: Dave Martin @ 2015-07-13 13:25 UTC (permalink / raw)
To: linux-arm-kernel
Changes since v1:
* Modified BRK immediate for BUG so that it doesn't overlap the range
allocated for KGDB.
* Typo fixes.
* Don't assume that BUG() kills the thread, so that catching BUGs in
kgdb works again.
* Separate header for the BRK immediates removed, at Will's request.
I've retained the other refactoring since it contains useful tidy-
ups, but some of that could go away if desired.
Original cover letter:
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: Add missing #include
arm64/BUG: Use BRK instruction for generic BUG traps
arm64/BUG: Show explicit backtrace for WARNs
arch/arm64/Kconfig | 8 ++
arch/arm64/include/asm/bug.h | 64 +++++++++++++++
arch/arm64/include/asm/debug-monitors.h | 36 +++------
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 | 61 +++++++++++++-
arch/arm64/mm/fault.c | 12 ++-
8 files changed, 233 insertions(+), 98 deletions(-)
create mode 100644 arch/arm64/include/asm/bug.h
--
1.7.10.4
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 01/10] arm64/debug: Eliminate magic number for size of BRK instruction
2015-07-13 13:25 [PATCH v2 00/10] arm64: Use BRK instruction for generic BUG traps Dave Martin
@ 2015-07-13 13:25 ` Dave Martin
2015-07-13 14:47 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 02/10] arm64/debug: Mask off all reserved bits from generated ESR values Dave Martin
` (8 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Dave Martin @ 2015-07-13 13:25 UTC (permalink / raw)
To: linux-arm-kernel
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 <Dave.Martin@arm.com>
---
arch/arm64/include/asm/debug-monitors.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 40ec68a..f3d2dbd 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -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
--
1.7.10.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v2 02/10] arm64/debug: Mask off all reserved bits from generated ESR values
2015-07-13 13:25 [PATCH v2 00/10] arm64: Use BRK instruction for generic BUG traps Dave Martin
2015-07-13 13:25 ` [PATCH v2 01/10] arm64/debug: Eliminate magic number for size of BRK instruction Dave Martin
@ 2015-07-13 13:25 ` Dave Martin
2015-07-13 14:14 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 03/10] arm64: esr.h type fixes and cleanup Dave Martin
` (7 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Dave Martin @ 2015-07-13 13:25 UTC (permalink / raw)
To: linux-arm-kernel
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 <Dave.Martin@arm.com>
---
arch/arm64/include/asm/debug-monitors.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index f3d2dbd..ab7d5a8 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -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
--
1.7.10.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v2 03/10] arm64: esr.h type fixes and cleanup
2015-07-13 13:25 [PATCH v2 00/10] arm64: Use BRK instruction for generic BUG traps Dave Martin
2015-07-13 13:25 ` [PATCH v2 01/10] arm64/debug: Eliminate magic number for size of BRK instruction Dave Martin
2015-07-13 13:25 ` [PATCH v2 02/10] arm64/debug: Mask off all reserved bits from generated ESR values Dave Martin
@ 2015-07-13 13:25 ` Dave Martin
2015-07-14 15:54 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 04/10] arm64/debug: Eliminate magic number from ESR template definition Dave Martin
` (6 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Dave Martin @ 2015-07-13 13:25 UTC (permalink / raw)
To: linux-arm-kernel
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 <Dave.Martin@arm.com>
---
arch/arm64/include/asm/esr.h | 128 ++++++++++++++++++++-------------------
arch/arm64/include/asm/memory.h | 3 +-
2 files changed, 67 insertions(+), 64 deletions(-)
diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 7052245..8dab2a9 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -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>
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index f800d45..c6a6592a 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -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
--
1.7.10.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v2 04/10] arm64/debug: Eliminate magic number from ESR template definition
2015-07-13 13:25 [PATCH v2 00/10] arm64: Use BRK instruction for generic BUG traps Dave Martin
` (2 preceding siblings ...)
2015-07-13 13:25 ` [PATCH v2 03/10] arm64: esr.h type fixes and cleanup Dave Martin
@ 2015-07-13 13:25 ` Dave Martin
2015-07-13 14:16 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 05/10] arm64/debug: More consistent naming for the BRK ESR template macro Dave Martin
` (5 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Dave Martin @ 2015-07-13 13:25 UTC (permalink / raw)
To: linux-arm-kernel
<asm/esr.h> has perfectly good constants for defining ESR values
already. Let's use them.
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
arch/arm64/include/asm/debug-monitors.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index ab7d5a8..ff09058 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -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
--
1.7.10.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v2 05/10] arm64/debug: More consistent naming for the BRK ESR template macro
2015-07-13 13:25 [PATCH v2 00/10] arm64: Use BRK instruction for generic BUG traps Dave Martin
` (3 preceding siblings ...)
2015-07-13 13:25 ` [PATCH v2 04/10] arm64/debug: Eliminate magic number from ESR template definition Dave Martin
@ 2015-07-13 13:25 ` Dave Martin
2015-07-13 14:19 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 06/10] arm64/debug: Move BRK ESR template macro into <asm/esr.h> Dave Martin
` (4 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Dave Martin @ 2015-07-13 13:25 UTC (permalink / raw)
To: linux-arm-kernel
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 <Dave.Martin@arm.com>
---
arch/arm64/include/asm/debug-monitors.h | 5 +++--
arch/arm64/kernel/kgdb.c | 4 ++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index ff09058..bb97e9d 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -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
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index a0d10c5..a5a838e 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -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
};
--
1.7.10.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v2 06/10] arm64/debug: Move BRK ESR template macro into <asm/esr.h>
2015-07-13 13:25 [PATCH v2 00/10] arm64: Use BRK instruction for generic BUG traps Dave Martin
` (4 preceding siblings ...)
2015-07-13 13:25 ` [PATCH v2 05/10] arm64/debug: More consistent naming for the BRK ESR template macro Dave Martin
@ 2015-07-13 13:25 ` Dave Martin
2015-07-13 14:18 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 07/10] arm64/debug: Simplify BRK insn opcode declarations Dave Martin
` (3 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Dave Martin @ 2015-07-13 13:25 UTC (permalink / raw)
To: linux-arm-kernel
It makes sense to keep all the architectural exception syndrome
definitions in the same place.
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
arch/arm64/include/asm/debug-monitors.h | 7 -------
arch/arm64/include/asm/esr.h | 7 +++++++
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index bb97e9d..e28b1dd 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -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)
diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 8dab2a9..7568d14 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -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>
--
1.7.10.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v2 07/10] arm64/debug: Simplify BRK insn opcode declarations
2015-07-13 13:25 [PATCH v2 00/10] arm64: Use BRK instruction for generic BUG traps Dave Martin
` (5 preceding siblings ...)
2015-07-13 13:25 ` [PATCH v2 06/10] arm64/debug: Move BRK ESR template macro into <asm/esr.h> Dave Martin
@ 2015-07-13 13:25 ` Dave Martin
2015-07-13 14:38 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 08/10] arm64/debug: Add missing #include Dave Martin
` (2 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Dave Martin @ 2015-07-13 13:25 UTC (permalink / raw)
To: linux-arm-kernel
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 <Dave.Martin@arm.com>
---
arch/arm64/include/asm/debug-monitors.h | 23 ++++-------------------
arch/arm64/kernel/kgdb.c | 8 ++++----
2 files changed, 8 insertions(+), 23 deletions(-)
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index e28b1dd..6a17fb8 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -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
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index a5a838e..4f78b8a 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -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),
}
};
--
1.7.10.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v2 08/10] arm64/debug: Add missing #include
2015-07-13 13:25 [PATCH v2 00/10] arm64: Use BRK instruction for generic BUG traps Dave Martin
` (6 preceding siblings ...)
2015-07-13 13:25 ` [PATCH v2 07/10] arm64/debug: Simplify BRK insn opcode declarations Dave Martin
@ 2015-07-13 13:25 ` Dave Martin
2015-07-13 14:34 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps Dave Martin
2015-07-13 13:25 ` [PATCH v2 10/10] arm64/BUG: Show explicit backtrace for WARNs Dave Martin
9 siblings, 1 reply; 34+ messages in thread
From: Dave Martin @ 2015-07-13 13:25 UTC (permalink / raw)
To: linux-arm-kernel
<asm/debug-monitors.h> relies on <asm/ptrace.h>, but doesn't
declare this dependency. This becomes a problem once
debug-monitors.h starts getting included all over the place to get
the BRK immedates.
This patch adds the missing #include.
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
arch/arm64/include/asm/debug-monitors.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 6a17fb8..b5715cb 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -20,6 +20,7 @@
#include <asm/esr.h>
#include <asm/insn.h>
+#include <asm/ptrace.h>
/* Low-level stepping controls. */
#define DBG_MDSCR_SS (1 << 0)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps
2015-07-13 13:25 [PATCH v2 00/10] arm64: Use BRK instruction for generic BUG traps Dave Martin
` (7 preceding siblings ...)
2015-07-13 13:25 ` [PATCH v2 08/10] arm64/debug: Add missing #include Dave Martin
@ 2015-07-13 13:25 ` Dave Martin
2015-07-13 16:43 ` Mark Rutland
2015-07-14 16:11 ` Catalin Marinas
2015-07-13 13:25 ` [PATCH v2 10/10] arm64/BUG: Show explicit backtrace for WARNs Dave Martin
9 siblings, 2 replies; 34+ messages in thread
From: Dave Martin @ 2015-07-13 13:25 UTC (permalink / raw)
To: linux-arm-kernel
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 <Dave.Martin@arm.com>
---
arch/arm64/Kconfig | 8 ++++
arch/arm64/include/asm/bug.h | 64 +++++++++++++++++++++++++++++++
arch/arm64/include/asm/debug-monitors.h | 2 +
arch/arm64/kernel/traps.c | 59 +++++++++++++++++++++++++++-
arch/arm64/mm/fault.c | 12 +++++-
5 files changed, 142 insertions(+), 3 deletions(-)
create mode 100644 arch/arm64/include/asm/bug.h
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 318175f..b918286 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -113,6 +113,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
diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h
new file mode 100644
index 0000000..4a748ce
--- /dev/null
+++ b/arch/arm64/include/asm/bug.h
@@ -0,0 +1,64 @@
+/*
+ * Copyright (C) 2015 ARM Limited
+ * Author: Dave Martin <Dave.Martin@arm.com>
+ *
+ * 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_BUG_H
+#define _ARCH_ARM64_ASM_BUG_H
+
+#include <asm/debug-monitors.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 */
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index b5715cb..c37c70e 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -50,10 +50,12 @@
* 0x100: for triggering a fault on purpose (reserved)
* 0x400: for dynamic BRK instruction
* 0x401: for compile time BRK instruction
+ * 0x800: kernel-mode BUG() and WARN() traps
*/
#define FAULT_BRK_IMM 0x100
#define KGDB_DYN_DBG_BRK_IMM 0x400
#define KGDB_COMPILED_DBG_BRK_IMM 0x401
+#define BUG_BRK_IMM 0x800
/*
* BRK instruction encoding
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 566bc4c..b10f4bf 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -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,10 @@
#include <linux/syscalls.h>
#include <asm/atomic.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>
@@ -459,7 +462,61 @@ 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 BRK #BUG_BRK_IMM.
+ * 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;
+}
+
+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);
+ break;
+
+ case BUG_TRAP_TYPE_WARN:
+ break;
+
+ default:
+ /* unknown/unrecognised bug trap type */
+ return DBG_HOOK_ERROR;
+ }
+
+ /* If thread survives, skip over the BUG instruction and continue: */
+ regs->pc += AARCH64_INSN_SIZE; /* skip BRK and resume */
+ return DBG_HOOK_HANDLED;
+}
+
+static struct break_hook bug_break_hook = {
+ .esr_val = 0xf2000000 | BUG_BRK_IMM,
+ .esr_mask = 0xffffffff,
+ .fn = bug_handler,
+};
+
+/*
+ * Initial handler for AArch64 BRK exceptions
+ * This handler only used until debug_traps_init().
+ */
+int __init early_brk64(unsigned long addr, unsigned int esr,
+ struct pt_regs *regs)
+{
+ return bug_handler(regs, esr) != DBG_HOOK_HANDLED;
+}
+
+/* This registration must happen early, before debug_traps_init(). */
void __init trap_init(void)
{
- return;
+ register_break_hook(&bug_break_hook);
}
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 94d98cd..49da50d 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -492,14 +492,22 @@ asmlinkage void __exception do_sp_pc_abort(unsigned long addr,
arm64_notify_die("Oops - SP/PC alignment exception", 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" },
};
--
1.7.10.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v2 10/10] arm64/BUG: Show explicit backtrace for WARNs
2015-07-13 13:25 [PATCH v2 00/10] arm64: Use BRK instruction for generic BUG traps Dave Martin
` (8 preceding siblings ...)
2015-07-13 13:25 ` [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps Dave Martin
@ 2015-07-13 13:25 ` Dave Martin
9 siblings, 0 replies; 34+ messages in thread
From: Dave Martin @ 2015-07-13 13:25 UTC (permalink / raw)
To: linux-arm-kernel
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 <Dave.Martin@arm.com>
---
arch/arm64/kernel/traps.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index b10f4bf..bc3dfc5 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -487,6 +487,8 @@ static int bug_handler(struct pt_regs *regs, unsigned int esr)
break;
case BUG_TRAP_TYPE_WARN:
+ /* Ideally, report_bug() should backtrace for us... but no. */
+ dump_backtrace(regs, NULL);
break;
default:
--
1.7.10.4
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v2 02/10] arm64/debug: Mask off all reserved bits from generated ESR values
2015-07-13 13:25 ` [PATCH v2 02/10] arm64/debug: Mask off all reserved bits from generated ESR values Dave Martin
@ 2015-07-13 14:14 ` Mark Rutland
2015-07-13 14:22 ` Dave Martin
0 siblings, 1 reply; 34+ messages in thread
From: Mark Rutland @ 2015-07-13 14:14 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 02:25:49PM +0100, Dave P Martin wrote:
> 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).
I'd guess that someone got confused with the instruction encoding where
the immediate lives in bits 20:5, though the mask would be wrong anyway.
>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Looks sane to me.
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/include/asm/debug-monitors.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index f3d2dbd..ab7d5a8 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -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
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 04/10] arm64/debug: Eliminate magic number from ESR template definition
2015-07-13 13:25 ` [PATCH v2 04/10] arm64/debug: Eliminate magic number from ESR template definition Dave Martin
@ 2015-07-13 14:16 ` Mark Rutland
0 siblings, 0 replies; 34+ messages in thread
From: Mark Rutland @ 2015-07-13 14:16 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 02:25:51PM +0100, Dave P Martin wrote:
> <asm/esr.h> has perfectly good constants for defining ESR values
> already. Let's use them.
>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
> arch/arm64/include/asm/debug-monitors.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index ab7d5a8..ff09058 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -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))
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
>
> /*
> * #imm16 values used for BRK instruction generation
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 06/10] arm64/debug: Move BRK ESR template macro into <asm/esr.h>
2015-07-13 13:25 ` [PATCH v2 06/10] arm64/debug: Move BRK ESR template macro into <asm/esr.h> Dave Martin
@ 2015-07-13 14:18 ` Mark Rutland
0 siblings, 0 replies; 34+ messages in thread
From: Mark Rutland @ 2015-07-13 14:18 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 02:25:53PM +0100, Dave P Martin wrote:
> It makes sense to keep all the architectural exception syndrome
> definitions in the same place.
>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
It might make sense to merge this with the previous patch, but either
way:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/include/asm/debug-monitors.h | 7 -------
> arch/arm64/include/asm/esr.h | 7 +++++++
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index bb97e9d..e28b1dd 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -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)
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 8dab2a9..7568d14 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -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>
>
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 05/10] arm64/debug: More consistent naming for the BRK ESR template macro
2015-07-13 13:25 ` [PATCH v2 05/10] arm64/debug: More consistent naming for the BRK ESR template macro Dave Martin
@ 2015-07-13 14:19 ` Mark Rutland
0 siblings, 0 replies; 34+ messages in thread
From: Mark Rutland @ 2015-07-13 14:19 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 02:25:52PM +0100, Dave P Martin wrote:
> 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 <Dave.Martin@arm.com>
Looks sane to me, though I think it should probably be merged with the
subsequenty patch moving the definition out to esr.h.
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/include/asm/debug-monitors.h | 5 +++--
> arch/arm64/kernel/kgdb.c | 4 ++--
> 2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index ff09058..bb97e9d 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -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
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index a0d10c5..a5a838e 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -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
> };
>
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 02/10] arm64/debug: Mask off all reserved bits from generated ESR values
2015-07-13 14:14 ` Mark Rutland
@ 2015-07-13 14:22 ` Dave Martin
0 siblings, 0 replies; 34+ messages in thread
From: Dave Martin @ 2015-07-13 14:22 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 03:14:40PM +0100, Mark Rutland wrote:
> On Mon, Jul 13, 2015 at 02:25:49PM +0100, Dave P Martin wrote:
> > 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).
>
> I'd guess that someone got confused with the instruction encoding where
> the immediate lives in bits 20:5, though the mask would be wrong anyway.
>
> >
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>
> Looks sane to me.
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
Cheers.
This was harmless up to now, anyway. It will only bite if someone
passes x >= 0x10000, which they're not supposed to do.
---Dave
>
> Mark.
>
> > ---
> > arch/arm64/include/asm/debug-monitors.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> > index f3d2dbd..ab7d5a8 100644
> > --- a/arch/arm64/include/asm/debug-monitors.h
> > +++ b/arch/arm64/include/asm/debug-monitors.h
> > @@ -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
> > --
> > 1.7.10.4
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 08/10] arm64/debug: Add missing #include
2015-07-13 13:25 ` [PATCH v2 08/10] arm64/debug: Add missing #include Dave Martin
@ 2015-07-13 14:34 ` Mark Rutland
2015-07-13 14:44 ` Dave Martin
0 siblings, 1 reply; 34+ messages in thread
From: Mark Rutland @ 2015-07-13 14:34 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 02:25:55PM +0100, Dave P Martin wrote:
> <asm/debug-monitors.h> relies on <asm/ptrace.h>, but doesn't
> declare this dependency. This becomes a problem once
> debug-monitors.h starts getting included all over the place to get
> the BRK immedates.
I take it that's for struct pt_regs down in the ifndef __ASSEMBLY__
portion?
>
> This patch adds the missing #include.
>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> ---
> arch/arm64/include/asm/debug-monitors.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 6a17fb8..b5715cb 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -20,6 +20,7 @@
>
> #include <asm/esr.h>
> #include <asm/insn.h>
> +#include <asm/ptrace.h>
We're also missing <linux/types.h> (for struct list_head, u8, and u32),
and <linux/errno.h> (for ENODEV in the reinstall_suspended_bps stub).
Would you be able to fold those in?
With those:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
>
> /* Low-level stepping controls. */
> #define DBG_MDSCR_SS (1 << 0)
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 07/10] arm64/debug: Simplify BRK insn opcode declarations
2015-07-13 13:25 ` [PATCH v2 07/10] arm64/debug: Simplify BRK insn opcode declarations Dave Martin
@ 2015-07-13 14:38 ` Mark Rutland
0 siblings, 0 replies; 34+ messages in thread
From: Mark Rutland @ 2015-07-13 14:38 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 02:25:54PM +0100, Dave P Martin wrote:
> 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 <Dave.Martin@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/include/asm/debug-monitors.h | 23 ++++-------------------
> arch/arm64/kernel/kgdb.c | 8 ++++----
> 2 files changed, 8 insertions(+), 23 deletions(-)
>
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index e28b1dd..6a17fb8 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -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
>
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index a5a838e..4f78b8a 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -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),
> }
> };
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 08/10] arm64/debug: Add missing #include
2015-07-13 14:34 ` Mark Rutland
@ 2015-07-13 14:44 ` Dave Martin
2015-07-13 14:45 ` Mark Rutland
0 siblings, 1 reply; 34+ messages in thread
From: Dave Martin @ 2015-07-13 14:44 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 03:34:36PM +0100, Mark Rutland wrote:
> On Mon, Jul 13, 2015 at 02:25:55PM +0100, Dave P Martin wrote:
> > <asm/debug-monitors.h> relies on <asm/ptrace.h>, but doesn't
> > declare this dependency. This becomes a problem once
> > debug-monitors.h starts getting included all over the place to get
> > the BRK immedates.
>
> I take it that's for struct pt_regs down in the ifndef __ASSEMBLY__
> portion?
Yes ... I suppose I could move the #include in there, but it seems
hermless. I prefer to keep them together.
>
> >
> > This patch adds the missing #include.
> >
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > ---
> > arch/arm64/include/asm/debug-monitors.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> > index 6a17fb8..b5715cb 100644
> > --- a/arch/arm64/include/asm/debug-monitors.h
> > +++ b/arch/arm64/include/asm/debug-monitors.h
> > @@ -20,6 +20,7 @@
> >
> > #include <asm/esr.h>
> > #include <asm/insn.h>
> > +#include <asm/ptrace.h>
>
> We're also missing <linux/types.h> (for struct list_head, u8, and u32),
> and <linux/errno.h> (for ENODEV in the reinstall_suspended_bps stub).
> Would you be able to fold those in?
Sure, can do.
> With those:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
Cheers
---Dave
>
> Mark.
>
> >
> > /* Low-level stepping controls. */
> > #define DBG_MDSCR_SS (1 << 0)
> > --
> > 1.7.10.4
> >
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 08/10] arm64/debug: Add missing #include
2015-07-13 14:44 ` Dave Martin
@ 2015-07-13 14:45 ` Mark Rutland
0 siblings, 0 replies; 34+ messages in thread
From: Mark Rutland @ 2015-07-13 14:45 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 03:44:34PM +0100, Dave P Martin wrote:
> On Mon, Jul 13, 2015 at 03:34:36PM +0100, Mark Rutland wrote:
> > On Mon, Jul 13, 2015 at 02:25:55PM +0100, Dave P Martin wrote:
> > > <asm/debug-monitors.h> relies on <asm/ptrace.h>, but doesn't
> > > declare this dependency. This becomes a problem once
> > > debug-monitors.h starts getting included all over the place to get
> > > the BRK immedates.
> >
> > I take it that's for struct pt_regs down in the ifndef __ASSEMBLY__
> > portion?
>
> Yes ... I suppose I could move the #include in there, but it seems
> hermless. I prefer to keep them together.
I was just checking that I hadn't missed something -- no need to move
it.
Mark.
>
> >
> > >
> > > This patch adds the missing #include.
> > >
> > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > ---
> > > arch/arm64/include/asm/debug-monitors.h | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> > > index 6a17fb8..b5715cb 100644
> > > --- a/arch/arm64/include/asm/debug-monitors.h
> > > +++ b/arch/arm64/include/asm/debug-monitors.h
> > > @@ -20,6 +20,7 @@
> > >
> > > #include <asm/esr.h>
> > > #include <asm/insn.h>
> > > +#include <asm/ptrace.h>
> >
> > We're also missing <linux/types.h> (for struct list_head, u8, and u32),
> > and <linux/errno.h> (for ENODEV in the reinstall_suspended_bps stub).
> > Would you be able to fold those in?
>
> Sure, can do.
>
> > With those:
> >
> > Acked-by: Mark Rutland <mark.rutland@arm.com>
>
> Cheers
> ---Dave
>
> >
> > Mark.
> >
> > >
> > > /* Low-level stepping controls. */
> > > #define DBG_MDSCR_SS (1 << 0)
> > > --
> > > 1.7.10.4
> > >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 01/10] arm64/debug: Eliminate magic number for size of BRK instruction
2015-07-13 13:25 ` [PATCH v2 01/10] arm64/debug: Eliminate magic number for size of BRK instruction Dave Martin
@ 2015-07-13 14:47 ` Mark Rutland
0 siblings, 0 replies; 34+ messages in thread
From: Mark Rutland @ 2015-07-13 14:47 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 02:25:48PM +0100, Dave P Martin wrote:
> 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.
Given BREAK_INSTR_SIZE seems to be used by the core of the KGDB code I
guess we have no choice. ;)
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/include/asm/debug-monitors.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 40ec68a..f3d2dbd 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -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
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps
2015-07-13 13:25 ` [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps Dave Martin
@ 2015-07-13 16:43 ` Mark Rutland
2015-07-13 16:51 ` Dave Martin
2015-07-14 16:11 ` Catalin Marinas
1 sibling, 1 reply; 34+ messages in thread
From: Mark Rutland @ 2015-07-13 16:43 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Mon, Jul 13, 2015 at 02:25:56PM +0100, Dave P Martin wrote:
> 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 <Dave.Martin@arm.com>
FWIW I've given this a spin and it seems to work, so:
Tested-by: Mark Rutland <mark.rutland@arm.com>
I have one concern with this below.
> +#ifndef _ARCH_ARM64_ASM_BUG_H
> +#define _ARCH_ARM64_ASM_BUG_H
> +
> +#include <asm/debug-monitors.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
Given the reliance on the labels in the caller, I think it might make
more sense to fold this into __BUG_FLAGS, and just have an #ifdef in the
middle.
That would also mean passing file and line to the macro for the general
case, like on arch/arm (even if !CONFIG_DEBUG_BUGVERBOSE), and moving
the double-indirection of those out to the caller.
Otherwise this looks good to me.
As an aside, it looks to me like the arch/arm implementation never
allocates space for flags in each bug_entry, which I would have expected
to mess up the bug table. I must be missing something there.
Thanks,
Mark.
> +
> +#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 */
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps
2015-07-13 16:43 ` Mark Rutland
@ 2015-07-13 16:51 ` Dave Martin
2015-07-13 16:56 ` Mark Rutland
0 siblings, 1 reply; 34+ messages in thread
From: Dave Martin @ 2015-07-13 16:51 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 05:43:15PM +0100, Mark Rutland wrote:
> Hi,
>
> On Mon, Jul 13, 2015 at 02:25:56PM +0100, Dave P Martin wrote:
> > 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.
[...]
> FWIW I've given this a spin and it seems to work, so:
>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
Thanks for testing.
> I have one concern with this below.
>
> > +#ifndef _ARCH_ARM64_ASM_BUG_H
> > +#define _ARCH_ARM64_ASM_BUG_H
> > +
> > +#include <asm/debug-monitors.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
>
> Given the reliance on the labels in the caller, I think it might make
Not sure what you mean here, can you elaborate?
> more sense to fold this into __BUG_FLAGS, and just have an #ifdef in the
> middle.
>
> That would also mean passing file and line to the macro for the general
> case, like on arch/arm (even if !CONFIG_DEBUG_BUGVERBOSE), and moving
> the double-indirection of those out to the caller.
I'll take a look -- it is probably more complex at present than it needs
to be.
[...]
Cheers
---Dave
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps
2015-07-13 16:51 ` Dave Martin
@ 2015-07-13 16:56 ` Mark Rutland
2015-07-13 17:05 ` Dave P Martin
2015-07-14 10:20 ` Dave Martin
0 siblings, 2 replies; 34+ messages in thread
From: Mark Rutland @ 2015-07-13 16:56 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 05:51:51PM +0100, Dave P Martin wrote:
> On Mon, Jul 13, 2015 at 05:43:15PM +0100, Mark Rutland wrote:
> > Hi,
> >
> > On Mon, Jul 13, 2015 at 02:25:56PM +0100, Dave P Martin wrote:
> > > 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.
>
> [...]
>
> > FWIW I've given this a spin and it seems to work, so:
> >
> > Tested-by: Mark Rutland <mark.rutland@arm.com>
>
> Thanks for testing.
>
> > I have one concern with this below.
> >
> > > +#ifndef _ARCH_ARM64_ASM_BUG_H
> > > +#define _ARCH_ARM64_ASM_BUG_H
> > > +
> > > +#include <asm/debug-monitors.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
> >
> > Given the reliance on the labels in the caller, I think it might make
>
> Not sure what you mean here, can you elaborate?
We're relying on label "0:" in the caller/user of the macro when we emit
".long 2b - 0b".
I think it would be clearer if folded into the caller (even with the
inline ifdef this necessitates).
Though I could be missing something here that renders that impossible.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps
2015-07-13 16:56 ` Mark Rutland
@ 2015-07-13 17:05 ` Dave P Martin
2015-07-14 10:20 ` Dave Martin
1 sibling, 0 replies; 34+ messages in thread
From: Dave P Martin @ 2015-07-13 17:05 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 05:56:39PM +0100, Mark Rutland wrote:
> On Mon, Jul 13, 2015 at 05:51:51PM +0100, Dave P Martin wrote:
> > On Mon, Jul 13, 2015 at 05:43:15PM +0100, Mark Rutland wrote:
> > > Hi,
> > >
> > > On Mon, Jul 13, 2015 at 02:25:56PM +0100, Dave P Martin wrote:
[...]
> > > > +#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
> > >
> > > Given the reliance on the labels in the caller, I think it might make
> >
> > Not sure what you mean here, can you elaborate?
>
> We're relying on label "0:" in the caller/user of the macro when we emit
> ".long 2b - 0b".
>
> I think it would be clearer if folded into the caller (even with the
> inline ifdef this necessitates).
Ah, I see what you're getting at. I think there is room for improvement
there.
> Though I could be missing something here that renders that impossible.
Well, you can't put an #ifdef in the middle of a macro definition as
such, but there are other ways around that. I'll see what I can do.
Cheers
---Dave
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps
2015-07-13 16:56 ` Mark Rutland
2015-07-13 17:05 ` Dave P Martin
@ 2015-07-14 10:20 ` Dave Martin
2015-07-14 11:09 ` Mark Rutland
1 sibling, 1 reply; 34+ messages in thread
From: Dave Martin @ 2015-07-14 10:20 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 05:56:39PM +0100, Mark Rutland wrote:
> On Mon, Jul 13, 2015 at 05:51:51PM +0100, Dave P Martin wrote:
> > On Mon, Jul 13, 2015 at 05:43:15PM +0100, Mark Rutland wrote:
[...]
> > > Given the reliance on the labels in the caller, I think it might make
> >
> > Not sure what you mean here, can you elaborate?
>
> We're relying on label "0:" in the caller/user of the macro when we emit
> ".long 2b - 0b".
>
> I think it would be clearer if folded into the caller (even with the
> inline ifdef this necessitates).
>
> Though I could be missing something here that renders that impossible.
Before I respin the series, can you cast your eye over this
alternative?
I'm testing it now, but in any case it should make the conditional
structure of the code clearer as per your suggestion.
Cheers
---Dave
[...]
#ifdef CONFIG_DEBUG_BUGVERBOSE
#define __BUGVERBOSE(x...) x
#else
#define __BUGVERBOSE(x...)
#endif
#define _BUG_FLAGS(flags, file, line) __BUG_FLAGS(flags, file, line)
#define __BUG_FLAGS(flags, file, line) asm volatile ( \
__BUGVERBOSE( ".pushsection .rodata.str,\"aMS\", at progbits,1\n" \
"2: .string \"" file "\"\n\t" \
".popsection\n\t" ) \
\
".pushsection __bug_table,\"a\"\n\t" \
".align 2\n\t" \
"0: .long 1f - 0b\n\t" \
__BUGVERBOSE( ".long 2b - 0b\n\t" \
".short " #line "\n\t" ) \
".short " #flags "\n\t" \
".popsection\n" \
\
"1: brk %[imm]" \
:: [imm] "i" (BUG_BRK_IMM) \
)
#define BUG() do { \
_BUG_FLAGS(0, __FILE__, __LINE__); \
unreachable(); \
} while (0)
#define __WARN_TAINT(taint) \
_BUG_FLAGS(BUGFLAG_TAINT(taint), __FILE__, __LINE__)
[...]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps
2015-07-14 10:20 ` Dave Martin
@ 2015-07-14 11:09 ` Mark Rutland
2015-07-14 11:34 ` Dave Martin
0 siblings, 1 reply; 34+ messages in thread
From: Mark Rutland @ 2015-07-14 11:09 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jul 14, 2015 at 11:20:27AM +0100, Dave P Martin wrote:
> On Mon, Jul 13, 2015 at 05:56:39PM +0100, Mark Rutland wrote:
> > On Mon, Jul 13, 2015 at 05:51:51PM +0100, Dave P Martin wrote:
> > > On Mon, Jul 13, 2015 at 05:43:15PM +0100, Mark Rutland wrote:
>
> [...]
>
> > > > Given the reliance on the labels in the caller, I think it might make
> > >
> > > Not sure what you mean here, can you elaborate?
> >
> > We're relying on label "0:" in the caller/user of the macro when we emit
> > ".long 2b - 0b".
> >
> > I think it would be clearer if folded into the caller (even with the
> > inline ifdef this necessitates).
> >
> > Though I could be missing something here that renders that impossible.
>
> Before I respin the series, can you cast your eye over this
> alternative?
>
> I'm testing it now, but in any case it should make the conditional
> structure of the code clearer as per your suggestion.
I think that style of macro is certainly more legible when applied to
single lines, but I got a little confused when reading this before I
noticed that each __BUGVERBOSE use covered a few lines.
Thanks,
Mark.
>
> Cheers
> ---Dave
>
>
> [...]
>
> #ifdef CONFIG_DEBUG_BUGVERBOSE
> #define __BUGVERBOSE(x...) x
> #else
> #define __BUGVERBOSE(x...)
> #endif
>
> #define _BUG_FLAGS(flags, file, line) __BUG_FLAGS(flags, file, line)
>
> #define __BUG_FLAGS(flags, file, line) asm volatile ( \
> __BUGVERBOSE( ".pushsection .rodata.str,\"aMS\", at progbits,1\n" \
> "2: .string \"" file "\"\n\t" \
> ".popsection\n\t" ) \
> \
> ".pushsection __bug_table,\"a\"\n\t" \
> ".align 2\n\t" \
> "0: .long 1f - 0b\n\t" \
> __BUGVERBOSE( ".long 2b - 0b\n\t" \
> ".short " #line "\n\t" ) \
> ".short " #flags "\n\t" \
> ".popsection\n" \
> \
> "1: brk %[imm]" \
> :: [imm] "i" (BUG_BRK_IMM) \
> )
>
> #define BUG() do { \
> _BUG_FLAGS(0, __FILE__, __LINE__); \
> unreachable(); \
> } while (0)
>
> #define __WARN_TAINT(taint) \
> _BUG_FLAGS(BUGFLAG_TAINT(taint), __FILE__, __LINE__)
>
> [...]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps
2015-07-14 11:09 ` Mark Rutland
@ 2015-07-14 11:34 ` Dave Martin
2015-07-14 15:51 ` Mark Rutland
0 siblings, 1 reply; 34+ messages in thread
From: Dave Martin @ 2015-07-14 11:34 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jul 14, 2015 at 12:09:05PM +0100, Mark Rutland wrote:
> On Tue, Jul 14, 2015 at 11:20:27AM +0100, Dave P Martin wrote:
> > On Mon, Jul 13, 2015 at 05:56:39PM +0100, Mark Rutland wrote:
> > > On Mon, Jul 13, 2015 at 05:51:51PM +0100, Dave P Martin wrote:
> > > > On Mon, Jul 13, 2015 at 05:43:15PM +0100, Mark Rutland wrote:
> >
> > [...]
> >
> > > > > Given the reliance on the labels in the caller, I think it might make
> > > >
> > > > Not sure what you mean here, can you elaborate?
> > >
> > > We're relying on label "0:" in the caller/user of the macro when we emit
> > > ".long 2b - 0b".
> > >
> > > I think it would be clearer if folded into the caller (even with the
> > > inline ifdef this necessitates).
> > >
> > > Though I could be missing something here that renders that impossible.
> >
> > Before I respin the series, can you cast your eye over this
> > alternative?
> >
> > I'm testing it now, but in any case it should make the conditional
> > structure of the code clearer as per your suggestion.
>
> I think that style of macro is certainly more legible when applied to
> single lines, but I got a little confused when reading this before I
> noticed that each __BUGVERBOSE use covered a few lines.
True, but that's as short a name as I like to define, given that it has
to stay #defined and this gets included absolutely everywhere.
I could repeat it on every line and rearrange/reindent things so that
__BUGVERBOSE doesn't get in the way of the labels, but IMHO that's
uglier and not really more readable.
Are we getting into dimishing returns here?
Cheers
---Dave
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps
2015-07-14 11:34 ` Dave Martin
@ 2015-07-14 15:51 ` Mark Rutland
2015-07-14 16:53 ` Dave Martin
0 siblings, 1 reply; 34+ messages in thread
From: Mark Rutland @ 2015-07-14 15:51 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jul 14, 2015 at 12:34:35PM +0100, Dave P Martin wrote:
> On Tue, Jul 14, 2015 at 12:09:05PM +0100, Mark Rutland wrote:
> > On Tue, Jul 14, 2015 at 11:20:27AM +0100, Dave P Martin wrote:
> > > On Mon, Jul 13, 2015 at 05:56:39PM +0100, Mark Rutland wrote:
> > > > On Mon, Jul 13, 2015 at 05:51:51PM +0100, Dave P Martin wrote:
> > > > > On Mon, Jul 13, 2015 at 05:43:15PM +0100, Mark Rutland wrote:
> > >
> > > [...]
> > >
> > > > > > Given the reliance on the labels in the caller, I think it might make
> > > > >
> > > > > Not sure what you mean here, can you elaborate?
> > > >
> > > > We're relying on label "0:" in the caller/user of the macro when we emit
> > > > ".long 2b - 0b".
> > > >
> > > > I think it would be clearer if folded into the caller (even with the
> > > > inline ifdef this necessitates).
> > > >
> > > > Though I could be missing something here that renders that impossible.
> > >
> > > Before I respin the series, can you cast your eye over this
> > > alternative?
> > >
> > > I'm testing it now, but in any case it should make the conditional
> > > structure of the code clearer as per your suggestion.
> >
> > I think that style of macro is certainly more legible when applied to
> > single lines, but I got a little confused when reading this before I
> > noticed that each __BUGVERBOSE use covered a few lines.
>
> True, but that's as short a name as I like to define, given that it has
> to stay #defined and this gets included absolutely everywhere.
>
> I could repeat it on every line and rearrange/reindent things so that
> __BUGVERBOSE doesn't get in the way of the labels, but IMHO that's
> uglier and not really more readable.
>
> Are we getting into dimishing returns here?
I guess so.
For any of the variants posted in this thread so far:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Thanks,
Mark.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 03/10] arm64: esr.h type fixes and cleanup
2015-07-13 13:25 ` [PATCH v2 03/10] arm64: esr.h type fixes and cleanup Dave Martin
@ 2015-07-14 15:54 ` Mark Rutland
2015-07-14 16:53 ` Dave Martin
0 siblings, 1 reply; 34+ messages in thread
From: Mark Rutland @ 2015-07-14 15:54 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 02:25:50PM +0100, Dave P Martin wrote:
> 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).
While it's potentially misleading, does forcing these to be 64-bit cause
a real issue? If so, it would be good to mention in the commit message.
Mark.
> 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 <Dave.Martin@arm.com>
> ---
> arch/arm64/include/asm/esr.h | 128 ++++++++++++++++++++-------------------
> arch/arm64/include/asm/memory.h | 3 +-
> 2 files changed, 67 insertions(+), 64 deletions(-)
>
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 7052245..8dab2a9 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -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>
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index f800d45..c6a6592a 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -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
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps
2015-07-13 13:25 ` [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps Dave Martin
2015-07-13 16:43 ` Mark Rutland
@ 2015-07-14 16:11 ` Catalin Marinas
2015-07-14 16:55 ` Dave Martin
1 sibling, 1 reply; 34+ messages in thread
From: Catalin Marinas @ 2015-07-14 16:11 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jul 13, 2015 at 02:25:56PM +0100, Dave P Martin wrote:
> 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 <Dave.Martin@arm.com>
The patch looks fine to me and since I gave you feedback on it before,
you can add:
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
For the rest of the series, feel free to add:
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 03/10] arm64: esr.h type fixes and cleanup
2015-07-14 15:54 ` Mark Rutland
@ 2015-07-14 16:53 ` Dave Martin
0 siblings, 0 replies; 34+ messages in thread
From: Dave Martin @ 2015-07-14 16:53 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jul 14, 2015 at 04:54:46PM +0100, Mark Rutland wrote:
> On Mon, Jul 13, 2015 at 02:25:50PM +0100, Dave P Martin wrote:
> > 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).
>
> While it's potentially misleading, does forcing these to be 64-bit cause
> a real issue? If so, it would be good to mention in the commit message.
>
> Mark.
GCC (at least, >= 4.9.2) generates warnings when these values, or
expressions involving them, are assigned to 32-bit structure members.
However, Will wasn't a big fan of my original solution to this problem,
so I'll take a final look before posting the final series to see if
there's a less invasive approach.
An explicit cast at the site of the truncating assignment(s) might do
the trick.
Cheers
---Dave
[...]
> > 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.
[...]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps
2015-07-14 15:51 ` Mark Rutland
@ 2015-07-14 16:53 ` Dave Martin
0 siblings, 0 replies; 34+ messages in thread
From: Dave Martin @ 2015-07-14 16:53 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jul 14, 2015 at 04:51:27PM +0100, Mark Rutland wrote:
> On Tue, Jul 14, 2015 at 12:34:35PM +0100, Dave P Martin wrote:
[...]
> > I could repeat it on every line and rearrange/reindent things so that
> > __BUGVERBOSE doesn't get in the way of the labels, but IMHO that's
> > uglier and not really more readable.
> >
> > Are we getting into dimishing returns here?
>
> I guess so.
>
> For any of the variants posted in this thread so far:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>
OK, thanks.
---Dave
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps
2015-07-14 16:11 ` Catalin Marinas
@ 2015-07-14 16:55 ` Dave Martin
0 siblings, 0 replies; 34+ messages in thread
From: Dave Martin @ 2015-07-14 16:55 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jul 14, 2015 at 05:11:04PM +0100, Catalin Marinas wrote:
> On Mon, Jul 13, 2015 at 02:25:56PM +0100, Dave P Martin wrote:
> > 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 <Dave.Martin@arm.com>
>
> The patch looks fine to me and since I gave you feedback on it before,
> you can add:
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>
> For the rest of the series, feel free to add:
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
OK, thanks.
I have a final respin to do, but most stuff won't change -- I'll flag
the differences.
Cheers
---Dave
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2015-07-14 16:55 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-13 13:25 [PATCH v2 00/10] arm64: Use BRK instruction for generic BUG traps Dave Martin
2015-07-13 13:25 ` [PATCH v2 01/10] arm64/debug: Eliminate magic number for size of BRK instruction Dave Martin
2015-07-13 14:47 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 02/10] arm64/debug: Mask off all reserved bits from generated ESR values Dave Martin
2015-07-13 14:14 ` Mark Rutland
2015-07-13 14:22 ` Dave Martin
2015-07-13 13:25 ` [PATCH v2 03/10] arm64: esr.h type fixes and cleanup Dave Martin
2015-07-14 15:54 ` Mark Rutland
2015-07-14 16:53 ` Dave Martin
2015-07-13 13:25 ` [PATCH v2 04/10] arm64/debug: Eliminate magic number from ESR template definition Dave Martin
2015-07-13 14:16 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 05/10] arm64/debug: More consistent naming for the BRK ESR template macro Dave Martin
2015-07-13 14:19 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 06/10] arm64/debug: Move BRK ESR template macro into <asm/esr.h> Dave Martin
2015-07-13 14:18 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 07/10] arm64/debug: Simplify BRK insn opcode declarations Dave Martin
2015-07-13 14:38 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 08/10] arm64/debug: Add missing #include Dave Martin
2015-07-13 14:34 ` Mark Rutland
2015-07-13 14:44 ` Dave Martin
2015-07-13 14:45 ` Mark Rutland
2015-07-13 13:25 ` [PATCH v2 09/10] arm64/BUG: Use BRK instruction for generic BUG traps Dave Martin
2015-07-13 16:43 ` Mark Rutland
2015-07-13 16:51 ` Dave Martin
2015-07-13 16:56 ` Mark Rutland
2015-07-13 17:05 ` Dave P Martin
2015-07-14 10:20 ` Dave Martin
2015-07-14 11:09 ` Mark Rutland
2015-07-14 11:34 ` Dave Martin
2015-07-14 15:51 ` Mark Rutland
2015-07-14 16:53 ` Dave Martin
2015-07-14 16:11 ` Catalin Marinas
2015-07-14 16:55 ` Dave Martin
2015-07-13 13:25 ` [PATCH v2 10/10] arm64/BUG: Show explicit backtrace for WARNs Dave Martin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.