[PATCH 0/7] ARM: hw_breakpoint: fix reset sequence for debug arch v7.1

[PATCH 0/7] ARM: hw_breakpoint: fix reset sequence for debug arch v7.1

[Will Deacon]

Hello,

This set of patches fixes the hw_breakpoint debug register reset sequence
for ARM debug architecture v7.1. This involves:

	- Probing of the save/restore registers on v7
	- Avoiding the use of DBGDSCRint.HDBGen, which is now UNKNOWN
	- Only attempt to enable monitor mode during boot, then check
	  whether it is still enabled during breakpoint validation
	- Some minor changes to remain compatible with v6 processors

All feedback welcome,

Will


Dietmar Eggemann (1):
  ARM: hw_breakpoint: use CRn as argument for debug reg accessor macros

Will Deacon (6):
  ARM: hw_breakpoint: only clear OS lock when implemented on v7
  ARM: hw_breakpoint: fix monitor mode detection with v7.1
  ARM: hw_breakpoint: fix ordering of debug register reset sequence
  ARM: hw_breakpoint: don't try to clear v6 debug registers during boot
  ARM: hw_breakpoint: make boot quieter without CPUID feature registers
  ARM: hw_breakpoint: check if monitor mode is enabled during
    validation

 arch/arm/include/asm/hw_breakpoint.h |    8 +-
 arch/arm/kernel/hw_breakpoint.c      |  146 ++++++++++++++++++----------------
 2 files changed, 82 insertions(+), 72 deletions(-)

-- 
1.7.4.1

[PATCH 1/7] ARM: hw_breakpoint: only clear OS lock when implemented on v7

[Will Deacon]

The OS save and restore register are optional in debug architecture v7,
so check the status register before attempting to clear the OS lock.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/hw_breakpoint.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 281bf33..ec16ada 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -929,6 +929,13 @@ static void reset_ctrl_regs(void *unused)
 		asm volatile("mrc p14, 0, %0, c1, c5, 4" : "=r" (dbg_power));
 		if ((dbg_power & 0x1) == 0)
 			err = -EPERM;
+
+		/*
+		 * Check whether we implement OS save and restore.
+		 */
+		asm volatile("mrc p14, 0, %0, c1, c1, 4" : "=r" (dbg_power));
+		if ((dbg_power & 0x9) == 0)
+			goto clear_vcr;
 		break;
 	case ARM_DEBUG_ARCH_V7_1:
 		/*
@@ -947,7 +954,7 @@ static void reset_ctrl_regs(void *unused)
 	}
 
 	/*
-	 * Unconditionally clear the lock by writing a value
+	 * Unconditionally clear the OS lock by writing a value
 	 * other than 0xC5ACCE55 to the access register.
 	 */
 	asm volatile("mcr p14, 0, %0, c1, c0, 4" : : "r" (0));
@@ -957,6 +964,7 @@ static void reset_ctrl_regs(void *unused)
 	 * Clear any configured vector-catch events before
 	 * enabling monitor mode.
 	 */
+clear_vcr:
 	asm volatile("mcr p14, 0, %0, c0, c7, 0" : : "r" (0));
 	isb();
 
-- 
1.7.4.1

[PATCH 2/7] ARM: hw_breakpoint: fix monitor mode detection with v7.1

[Will Deacon]

Detecting whether halting debug is enabled is no longer possible via
the DBGDSCR in v7.1, returning an UNKNOWN value for the HDBGen bit via
CP14 when the OS lock is clear.

This patch removes the halting mode check and ensures that accesses to
the internal and external views of the DBGDSCR are serialised with an
instruction barrier.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/hw_breakpoint.c |   25 +++++--------------------
 1 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index ec16ada..169dcce 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -235,13 +235,6 @@ static int enable_monitor_mode(void)
 
 	ARM_DBG_READ(c1, 0, dscr);
 
-	/* Ensure that halting mode is disabled. */
-	if (WARN_ONCE(dscr & ARM_DSCR_HDBGEN,
-		"halting debug mode enabled. Unable to access hardware resources.\n")) {
-		ret = -EPERM;
-		goto out;
-	}
-
 	/* If monitor mode is already enabled, just return. */
 	if (dscr & ARM_DSCR_MDBGEN)
 		goto out;
@@ -255,6 +248,7 @@ static int enable_monitor_mode(void)
 	case ARM_DEBUG_ARCH_V7_ECP14:
 	case ARM_DEBUG_ARCH_V7_1:
 		ARM_DBG_WRITE(c2, 2, (dscr | ARM_DSCR_MDBGEN));
+		isb();
 		break;
 	default:
 		ret = -ENODEV;
@@ -1000,8 +994,6 @@ static struct notifier_block __cpuinitdata dbg_reset_nb = {
 
 static int __init arch_hw_breakpoint_init(void)
 {
-	u32 dscr;
-
 	debug_arch = get_debug_arch();
 
 	if (!debug_arch_supported()) {
@@ -1036,17 +1028,10 @@ static int __init arch_hw_breakpoint_init(void)
 		core_num_brps, core_has_mismatch_brps() ? "(+1 reserved) " :
 		"", core_num_wrps);
 
-	ARM_DBG_READ(c1, 0, dscr);
-	if (dscr & ARM_DSCR_HDBGEN) {
-		max_watchpoint_len = 4;
-		pr_warning("halting debug mode enabled. Assuming maximum watchpoint size of %u bytes.\n",
-			   max_watchpoint_len);
-	} else {
-		/* Work out the maximum supported watchpoint length. */
-		max_watchpoint_len = get_max_wp_len();
-		pr_info("maximum watchpoint size is %u bytes.\n",
-				max_watchpoint_len);
-	}
+	/* Work out the maximum supported watchpoint length. */
+	max_watchpoint_len = get_max_wp_len();
+	pr_info("maximum watchpoint size is %u bytes.\n",
+			max_watchpoint_len);
 
 	/* Register debug fault handler. */
 	hook_fault_code(FAULT_CODE_DEBUG, hw_breakpoint_pending, SIGTRAP,
-- 
1.7.4.1

[PATCH 3/7] ARM: hw_breakpoint: fix ordering of debug register reset sequence

[Will Deacon]

The debug register reset sequence for v7 and v7.1 is congruent with
tap-dancing through a minefield.

Rather than wait until we've blown ourselves to pieces, this patch
instead checks the debug_err_mask after each potentially faulting
operation. We also move the enabling of monitor_mode to the end of the
sequence in order to prevent spurious debug events generated by UNKNOWN
register values.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/hw_breakpoint.c |   36 ++++++++++++++++++++++++++----------
 1 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 169dcce..e76cf1a 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -231,8 +231,6 @@ static int get_num_brps(void)
 static int enable_monitor_mode(void)
 {
 	u32 dscr;
-	int ret = 0;
-
 	ARM_DBG_READ(c1, 0, dscr);
 
 	/* If monitor mode is already enabled, just return. */
@@ -251,17 +249,18 @@ static int enable_monitor_mode(void)
 		isb();
 		break;
 	default:
-		ret = -ENODEV;
-		goto out;
+		return -ENODEV;
 	}
 
 	/* Check that the write made it through. */
 	ARM_DBG_READ(c1, 0, dscr);
-	if (!(dscr & ARM_DSCR_MDBGEN))
-		ret = -EPERM;
+	if (WARN_ONCE(!(dscr & ARM_DSCR_MDBGEN),
+		"Failed to enable monitor mode on CPU %d.\n",
+		smp_processor_id()))
+		return -EPERM;
 
 out:
-	return ret;
+	return 0;
 }
 
 int hw_breakpoint_slots(int type)
@@ -962,11 +961,16 @@ clear_vcr:
 	asm volatile("mcr p14, 0, %0, c0, c7, 0" : : "r" (0));
 	isb();
 
-reset_regs:
-	if (enable_monitor_mode())
+	if (cpumask_intersects(&debug_err_mask, cpumask_of(cpu))) {
+		pr_warning("CPU %d failed to disable vector catch\n", cpu);
 		return;
+	}
 
-	/* We must also reset any reserved registers. */
+reset_regs:
+	/*
+	 * The control/value register pairs are UNKNOWN out of reset so
+	 * clear them to avoid spurious debug events.
+	 */
 	raw_num_brps = get_num_brp_resources();
 	for (i = 0; i < raw_num_brps; ++i) {
 		write_wb_reg(ARM_BASE_BCR + i, 0UL);
@@ -977,6 +981,18 @@ reset_regs:
 		write_wb_reg(ARM_BASE_WCR + i, 0UL);
 		write_wb_reg(ARM_BASE_WVR + i, 0UL);
 	}
+
+	if (cpumask_intersects(&debug_err_mask, cpumask_of(cpu))) {
+		pr_warning("CPU %d failed to clear debug register pairs\n", cpu);
+		return;
+	}
+
+	/*
+	 * Have a crack@enabling monitor mode. We don't actually need
+	 * it yet, but reporting an error early is useful if it fails.
+	 */
+	if (enable_monitor_mode())
+		cpumask_or(&debug_err_mask, &debug_err_mask, cpumask_of(cpu));
 }
 
 static int __cpuinit dbg_reset_notify(struct notifier_block *self,
-- 
1.7.4.1

[PATCH 4/7] ARM: hw_breakpoint: don't try to clear v6 debug registers during boot

[Will Deacon]

v6 cores do not provide a way to clear the debug registers without first
enabling monitor mode, meaning that we could take spurious debug
exceptions. Instead, rely on the registers being in a sane state when we
boot as they are defined to be disabled out of reset anyway.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/hw_breakpoint.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index e76cf1a..b6f4aec 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -912,8 +912,8 @@ static void reset_ctrl_regs(void *unused)
 	switch (debug_arch) {
 	case ARM_DEBUG_ARCH_V6:
 	case ARM_DEBUG_ARCH_V6_1:
-		/* ARMv6 cores just need to reset the registers. */
-		goto reset_regs;
+		/* ARMv6 cores clear the registers out of reset. */
+		goto out_mdbgen;
 	case ARM_DEBUG_ARCH_V7_ECP14:
 		/*
 		 * Ensure sticky power-down is clear (i.e. debug logic is
@@ -966,7 +966,6 @@ clear_vcr:
 		return;
 	}
 
-reset_regs:
 	/*
 	 * The control/value register pairs are UNKNOWN out of reset so
 	 * clear them to avoid spurious debug events.
@@ -991,6 +990,7 @@ reset_regs:
 	 * Have a crack at enabling monitor mode. We don't actually need
 	 * it yet, but reporting an error early is useful if it fails.
 	 */
+out_mdbgen:
 	if (enable_monitor_mode())
 		cpumask_or(&debug_err_mask, &debug_err_mask, cpumask_of(cpu));
 }
-- 
1.7.4.1

[PATCH 5/7] ARM: hw_breakpoint: make boot quieter without CPUID feature registers

[Will Deacon]

Booting on a v6 core without the CPUID feature registers (e.g. 1136)
leads to a noisy dmesg complaining about their absence.

This patch changes the pr_warning into a WARN_ONCE to keep the log
quieter.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/hw_breakpoint.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index b6f4aec..f267120 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -135,11 +135,10 @@ static u8 get_debug_arch(void)
 	u32 didr;
 
 	/* Do we implement the extended CPUID interface? */
-	if (((read_cpuid_id() >> 16) & 0xf) != 0xf) {
-		pr_warning("CPUID feature registers not supported. "
-			   "Assuming v6 debug is present.\n");
+	if (WARN_ONCE(((read_cpuid_id() >> 16) & 0xf) != 0xf,
+		"CPUID feature registers not supported. "
+		"Assuming v6 debug is present.\n"))
 		return ARM_DEBUG_ARCH_V6;
-	}
 
 	ARM_DBG_READ(c0, 0, didr);
 	return (didr >> 16) & 0xf;
-- 
1.7.4.1

[PATCH 6/7] ARM: hw_breakpoint: check if monitor mode is enabled during validation

[Will Deacon]

Rather than attempt to enable monitor mode explicitly when scheduling in
a breakpoint event (which could raise an undefined exception trap when
accessing DBGDSCRext), instead check that DBGDSCRint.MDBGen is set
during event validation and report an error to the caller if not.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/hw_breakpoint.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index f267120..3127ae4 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -227,6 +227,13 @@ static int get_num_brps(void)
  * be put into halting debug mode at any time by an external debugger
  * but there is nothing we can do to prevent that.
  */
+static int monitor_mode_enabled(void)
+{
+	u32 dscr;
+	ARM_DBG_READ(c1, 0, dscr);
+	return !!(dscr & ARM_DSCR_MDBGEN);
+}
+
 static int enable_monitor_mode(void)
 {
 	u32 dscr;
@@ -320,14 +327,9 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 	struct perf_event **slot, **slots;
-	int i, max_slots, ctrl_base, val_base, ret = 0;
+	int i, max_slots, ctrl_base, val_base;
 	u32 addr, ctrl;
 
-	/* Ensure that we are in monitor mode and halting mode is disabled. */
-	ret = enable_monitor_mode();
-	if (ret)
-		goto out;
-
 	addr = info->address;
 	ctrl = encode_ctrl_reg(info->ctrl) | 0x1;
 
@@ -354,10 +356,8 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 		}
 	}
 
-	if (WARN_ONCE(i == max_slots, "Can't find any breakpoint slot\n")) {
-		ret = -EBUSY;
-		goto out;
-	}
+	if (WARN_ONCE(i == max_slots, "Can't find any breakpoint slot\n"))
+		return -EBUSY;
 
 	/* Override the breakpoint data with the step data. */
 	if (info->step_ctrl.enabled) {
@@ -375,9 +375,7 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 
 	/* Setup the control register. */
 	write_wb_reg(ctrl_base + i, ctrl);
-
-out:
-	return ret;
+	return 0;
 }
 
 void arch_uninstall_hw_breakpoint(struct perf_event *bp)
@@ -588,6 +586,10 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 	int ret = 0;
 	u32 offset, alignment_mask = 0x3;
 
+	/* Ensure that we are in monitor debug mode. */
+	if (!monitor_mode_enabled())
+		return -ENODEV;
+
 	/* Build the arch_hw_breakpoint. */
 	ret = arch_build_bp_info(bp);
 	if (ret)
-- 
1.7.4.1

[PATCH 7/7] ARM: hw_breakpoint: use CRn as argument for debug reg accessor macros

[Will Deacon]

From: Dietmar Eggemann <dietmar.eggemann@arm.com>

The coprocessor register CRn for accesses to the debug register can be a
different one than C0. Take this into account for the ARM_DBG_READ and
the ARM_DBG_WRITE macro.

The inline assembler calls which used a coprocessor register CRn other
than C0 are replaced by the ARM_DBG_READ or ARM_DBG_WRITE macro.

Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/hw_breakpoint.h |    8 +++---
 arch/arm/kernel/hw_breakpoint.c      |   40 +++++++++++++++++-----------------
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/arch/arm/include/asm/hw_breakpoint.h b/arch/arm/include/asm/hw_breakpoint.h
index c190bc9..01169dd 100644
--- a/arch/arm/include/asm/hw_breakpoint.h
+++ b/arch/arm/include/asm/hw_breakpoint.h
@@ -98,12 +98,12 @@ static inline void decode_ctrl_reg(u32 reg,
 #define ARM_BASE_WCR		112
 
 /* Accessor macros for the debug registers. */
-#define ARM_DBG_READ(M, OP2, VAL) do {\
-	asm volatile("mrc p14, 0, %0, c0," #M ", " #OP2 : "=r" (VAL));\
+#define ARM_DBG_READ(N, M, OP2, VAL) do {\
+	asm volatile("mrc p14, 0, %0, " #N "," #M ", " #OP2 : "=r" (VAL));\
 } while (0)
 
-#define ARM_DBG_WRITE(M, OP2, VAL) do {\
-	asm volatile("mcr p14, 0, %0, c0," #M ", " #OP2 : : "r" (VAL));\
+#define ARM_DBG_WRITE(N, M, OP2, VAL) do {\
+	asm volatile("mcr p14, 0, %0, " #N "," #M ", " #OP2 : : "r" (VAL));\
 } while (0)
 
 struct notifier_block;
diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 3127ae4..05fdd68 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -52,14 +52,14 @@ static u8 debug_arch;
 /* Maximum supported watchpoint length. */
 static u8 max_watchpoint_len;
 
-#define READ_WB_REG_CASE(OP2, M, VAL)		\
-	case ((OP2 << 4) + M):			\
-		ARM_DBG_READ(c ## M, OP2, VAL); \
+#define READ_WB_REG_CASE(OP2, M, VAL)			\
+	case ((OP2 << 4) + M):				\
+		ARM_DBG_READ(c0, c ## M, OP2, VAL);	\
 		break
 
-#define WRITE_WB_REG_CASE(OP2, M, VAL)		\
-	case ((OP2 << 4) + M):			\
-		ARM_DBG_WRITE(c ## M, OP2, VAL);\
+#define WRITE_WB_REG_CASE(OP2, M, VAL)			\
+	case ((OP2 << 4) + M):				\
+		ARM_DBG_WRITE(c0, c ## M, OP2, VAL);	\
 		break
 
 #define GEN_READ_WB_REG_CASES(OP2, VAL)		\
@@ -140,7 +140,7 @@ static u8 get_debug_arch(void)
 		"Assuming v6 debug is present.\n"))
 		return ARM_DEBUG_ARCH_V6;
 
-	ARM_DBG_READ(c0, 0, didr);
+	ARM_DBG_READ(c0, c0, 0, didr);
 	return (didr >> 16) & 0xf;
 }
 
@@ -168,7 +168,7 @@ static int debug_exception_updates_fsr(void)
 static int get_num_wrp_resources(void)
 {
 	u32 didr;
-	ARM_DBG_READ(c0, 0, didr);
+	ARM_DBG_READ(c0, c0, 0, didr);
 	return ((didr >> 28) & 0xf) + 1;
 }
 
@@ -176,7 +176,7 @@ static int get_num_wrp_resources(void)
 static int get_num_brp_resources(void)
 {
 	u32 didr;
-	ARM_DBG_READ(c0, 0, didr);
+	ARM_DBG_READ(c0, c0, 0, didr);
 	return ((didr >> 24) & 0xf) + 1;
 }
 
@@ -230,14 +230,14 @@ static int get_num_brps(void)
 static int monitor_mode_enabled(void)
 {
 	u32 dscr;
-	ARM_DBG_READ(c1, 0, dscr);
+	ARM_DBG_READ(c0, c1, 0, dscr);
 	return !!(dscr & ARM_DSCR_MDBGEN);
 }
 
 static int enable_monitor_mode(void)
 {
 	u32 dscr;
-	ARM_DBG_READ(c1, 0, dscr);
+	ARM_DBG_READ(c0, c1, 0, dscr);
 
 	/* If monitor mode is already enabled, just return. */
 	if (dscr & ARM_DSCR_MDBGEN)
@@ -247,11 +247,11 @@ static int enable_monitor_mode(void)
 	switch (get_debug_arch()) {
 	case ARM_DEBUG_ARCH_V6:
 	case ARM_DEBUG_ARCH_V6_1:
-		ARM_DBG_WRITE(c1, 0, (dscr | ARM_DSCR_MDBGEN));
+		ARM_DBG_WRITE(c0, c1, 0, (dscr | ARM_DSCR_MDBGEN));
 		break;
 	case ARM_DEBUG_ARCH_V7_ECP14:
 	case ARM_DEBUG_ARCH_V7_1:
-		ARM_DBG_WRITE(c2, 2, (dscr | ARM_DSCR_MDBGEN));
+		ARM_DBG_WRITE(c0, c2, 2, (dscr | ARM_DSCR_MDBGEN));
 		isb();
 		break;
 	default:
@@ -259,7 +259,7 @@ static int enable_monitor_mode(void)
 	}
 
 	/* Check that the write made it through. */
-	ARM_DBG_READ(c1, 0, dscr);
+	ARM_DBG_READ(c0, c1, 0, dscr);
 	if (WARN_ONCE(!(dscr & ARM_DSCR_MDBGEN),
 		"Failed to enable monitor mode on CPU %d.\n",
 		smp_processor_id()))
@@ -852,7 +852,7 @@ static int hw_breakpoint_pending(unsigned long addr, unsigned int fsr,
 		local_irq_enable();
 
 	/* We only handle watchpoints and hardware breakpoints. */
-	ARM_DBG_READ(c1, 0, dscr);
+	ARM_DBG_READ(c0, c1, 0, dscr);
 
 	/* Perform perf callbacks. */
 	switch (ARM_DSCR_MOE(dscr)) {
@@ -920,14 +920,14 @@ static void reset_ctrl_regs(void *unused)
 		 * Ensure sticky power-down is clear (i.e. debug logic is
 		 * powered up).
 		 */
-		asm volatile("mrc p14, 0, %0, c1, c5, 4" : "=r" (dbg_power));
+		ARM_DBG_READ(c1, c5, 4, dbg_power);
 		if ((dbg_power & 0x1) == 0)
 			err = -EPERM;
 
 		/*
 		 * Check whether we implement OS save and restore.
 		 */
-		asm volatile("mrc p14, 0, %0, c1, c1, 4" : "=r" (dbg_power));
+		ARM_DBG_READ(c1, c1, 4, dbg_power);
 		if ((dbg_power & 0x9) == 0)
 			goto clear_vcr;
 		break;
@@ -935,7 +935,7 @@ static void reset_ctrl_regs(void *unused)
 		/*
 		 * Ensure the OS double lock is clear.
 		 */
-		asm volatile("mrc p14, 0, %0, c1, c3, 4" : "=r" (dbg_power));
+		ARM_DBG_READ(c1, c3, 4, dbg_power);
 		if ((dbg_power & 0x1) == 1)
 			err = -EPERM;
 		break;
@@ -951,7 +951,7 @@ static void reset_ctrl_regs(void *unused)
 	 * Unconditionally clear the OS lock by writing a value
 	 * other than 0xC5ACCE55 to the access register.
 	 */
-	asm volatile("mcr p14, 0, %0, c1, c0, 4" : : "r" (0));
+	ARM_DBG_WRITE(c1, c0, 4, 0);
 	isb();
 
 	/*
@@ -959,7 +959,7 @@ static void reset_ctrl_regs(void *unused)
 	 * enabling monitor mode.
 	 */
 clear_vcr:
-	asm volatile("mcr p14, 0, %0, c0, c7, 0" : : "r" (0));
+	ARM_DBG_WRITE(c0, c7, 0, 0);
 	isb();
 
 	if (cpumask_intersects(&debug_err_mask, cpumask_of(cpu))) {
-- 
1.7.4.1

[PATCH 1/7] ARM: hw_breakpoint: only clear OS lock when implemented on v7

[Stephen Boyd]

On 10/17/12 08:31, Will Deacon wrote:
> The OS save and restore register are optional in debug architecture v7,
> so check the status register before attempting to clear the OS lock.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Tested-by: Stephen Boyd <sboyd@codeaurora.org>

> ---
>  arch/arm/kernel/hw_breakpoint.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index 281bf33..ec16ada 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -929,6 +929,13 @@ static void reset_ctrl_regs(void *unused)
>  		asm volatile("mrc p14, 0, %0, c1, c5, 4" : "=r" (dbg_power));
>  		if ((dbg_power & 0x1) == 0)
>  			err = -EPERM;
> +
> +		/*
> +		 * Check whether we implement OS save and restore.
> +		 */
> +		asm volatile("mrc p14, 0, %0, c1, c1, 4" : "=r" (dbg_power));

minor nit for this series. dbg_power has become a catch-all variable in
this code. It would be nice if we named the variables used to hold the
read register the same as the register or if we made the name of the
variable generic like 'val'.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 2/7] ARM: hw_breakpoint: fix monitor mode detection with v7.1

[Stephen Boyd]

On 10/17/12 08:31, Will Deacon wrote:
> Detecting whether halting debug is enabled is no longer possible via
> the DBGDSCR in v7.1, returning an UNKNOWN value for the HDBGen bit via
> CP14 when the OS lock is clear.
>
> This patch removes the halting mode check and ensures that accesses to
> the internal and external views of the DBGDSCR are serialised with an
> instruction barrier.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Tested-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 3/7] ARM: hw_breakpoint: fix ordering of debug register reset sequence

[Stephen Boyd]

On 10/17/12 08:31, Will Deacon wrote:
> The debug register reset sequence for v7 and v7.1 is congruent with
> tap-dancing through a minefield.
>
> Rather than wait until we've blown ourselves to pieces, this patch
> instead checks the debug_err_mask after each potentially faulting
> operation. We also move the enabling of monitor_mode to the end of the
> sequence in order to prevent spurious debug events generated by UNKNOWN
> register values.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Reported-by: Stephen Boyd <sboyd@codeaurora.org>
Tested-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 4/7] ARM: hw_breakpoint: don't try to clear v6 debug registers during boot

[Stephen Boyd]

On 10/17/12 08:31, Will Deacon wrote:
> v6 cores do not provide a way to clear the debug registers without first
> enabling monitor mode, meaning that we could take spurious debug
> exceptions. Instead, rely on the registers being in a sane state when we
> boot as they are defined to be disabled out of reset anyway.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>

Tested-by: Stephen Boyd <sboyd@codeaurora.org>

> ---
>  arch/arm/kernel/hw_breakpoint.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index e76cf1a..b6f4aec 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -912,8 +912,8 @@ static void reset_ctrl_regs(void *unused)
>  	switch (debug_arch) {
>  	case ARM_DEBUG_ARCH_V6:
>  	case ARM_DEBUG_ARCH_V6_1:
> -		/* ARMv6 cores just need to reset the registers. */
> -		goto reset_regs;
> +		/* ARMv6 cores clear the registers out of reset. */
> +		goto out_mdbgen;
>  	case ARM_DEBUG_ARCH_V7_ECP14:
>  		/*
>  		 * Ensure sticky power-down is clear (i.e. debug logic is
> @@ -966,7 +966,6 @@ clear_vcr:
>  		return;
>  	}
>  
> -reset_regs:
>  	/*
>  	 * The control/value register pairs are UNKNOWN out of reset so
>  	 * clear them to avoid spurious debug events.
> @@ -991,6 +990,7 @@ reset_regs:
>  	 * Have a crack at enabling monitor mode. We don't actually need
>  	 * it yet, but reporting an error early is useful if it fails.
>  	 */
> +out_mdbgen:
>  	if (enable_monitor_mode())
>  		cpumask_or(&debug_err_mask, &debug_err_mask, cpumask_of(cpu));
>  }


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 5/7] ARM: hw_breakpoint: make boot quieter without CPUID feature registers

[Stephen Boyd]

On 10/17/12 08:31, Will Deacon wrote:
> Booting on a v6 core without the CPUID feature registers (e.g. 1136)
> leads to a noisy dmesg complaining about their absence.
>
> This patch changes the pr_warning into a WARN_ONCE to keep the log
> quieter.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> @@ -135,11 +135,10 @@ static u8 get_debug_arch(void)
>  	u32 didr;
>  
>  	/* Do we implement the extended CPUID interface? */
> -	if (((read_cpuid_id() >> 16) & 0xf) != 0xf) {
> -		pr_warning("CPUID feature registers not supported. "
> -			   "Assuming v6 debug is present.\n");
> +	if (WARN_ONCE(((read_cpuid_id() >> 16) & 0xf) != 0xf,
> +		"CPUID feature registers not supported. "
> +		"Assuming v6 debug is present.\n"))

Won't this print a big stack trace instead of a one liner? Perhaps you
want pr_warn_once()?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 7/7] ARM: hw_breakpoint: use CRn as argument for debug reg accessor macros

[Stephen Boyd]

On 10/17/12 08:31, Will Deacon wrote:
> From: Dietmar Eggemann <dietmar.eggemann@arm.com>
>
> The coprocessor register CRn for accesses to the debug register can be a
> different one than C0. Take this into account for the ARM_DBG_READ and
> the ARM_DBG_WRITE macro.
>
> The inline assembler calls which used a coprocessor register CRn other
> than C0 are replaced by the ARM_DBG_READ or ARM_DBG_WRITE macro.
>
> Signed-off-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
>

Tested-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 1/7] ARM: hw_breakpoint: only clear OS lock when implemented on v7

[Will Deacon]

On Wed, Oct 24, 2012 at 11:53:29PM +0100, Stephen Boyd wrote:
> On 10/17/12 08:31, Will Deacon wrote:
> > The OS save and restore register are optional in debug architecture v7,
> > so check the status register before attempting to clear the OS lock.
> >
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> 
> Tested-by: Stephen Boyd <sboyd@codeaurora.org>
> 
> > ---
> >  arch/arm/kernel/hw_breakpoint.c |   10 +++++++++-
> >  1 files changed, 9 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> > index 281bf33..ec16ada 100644
> > --- a/arch/arm/kernel/hw_breakpoint.c
> > +++ b/arch/arm/kernel/hw_breakpoint.c
> > @@ -929,6 +929,13 @@ static void reset_ctrl_regs(void *unused)
> >  		asm volatile("mrc p14, 0, %0, c1, c5, 4" : "=r" (dbg_power));
> >  		if ((dbg_power & 0x1) == 0)
> >  			err = -EPERM;
> > +
> > +		/*
> > +		 * Check whether we implement OS save and restore.
> > +		 */
> > +		asm volatile("mrc p14, 0, %0, c1, c1, 4" : "=r" (dbg_power));
> 
> minor nit for this series. dbg_power has become a catch-all variable in
> this code. It would be nice if we named the variables used to hold the
> read register the same as the register or if we made the name of the
> variable generic like 'val'.

Thanks for your feedback and tested-bys on this series Stephen, I really
appreciate it.

I'll send out a v2 and update the branch I'm sending to -next.

Cheers,

Will