[PATCH 0/8] ARM: hw_breakpoint: fixes and improvements (v2)

[PATCH 0/8] ARM: hw_breakpoint: fixes and improvements (v2)

[Will Deacon]

Hello,

This is version 2 of the patchset originally posted here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2010-November/032717.html

There are a substantial number of changes:

* Support for CONFIG_PREEMPT kernels.

* Hugely improved watchpoint single-step capabilities without violating
  RCU requirements.

* sparse no longer generates warnings for hw_breakpoint.c.

* We no longer advertise reserved breakpoints to GDB.

All feedback welcome. I would especially value feedback on patch [4/8]
(`disable preemption during debug exception handling') to know if there's
a better solution.


Will Deacon (8):
  ARM: hw_breakpoint: ensure OS lock is clear before writing to debug
    registers
  ARM: hw_breakpoint: reset control registers in hotplug path
  ARM: hw_breakpoint: correct and simplify alignment fixup code
  ARM: hw_breakpoint: disable preemption during debug exception
    handling
  ARM: hw_breakpoint: don't advertise reserved breakpoints
  ARM: hw_breakpoint: do not allocate new breakpoints with
    rcu_read_lock held
  ARM: ptrace: fix style issue with hw_breakpoint interface
  ARM: hw_breakpoint: fix warnings generated by sparse

 arch/arm/include/asm/hw_breakpoint.h |    2 +-
 arch/arm/kernel/entry-armv.S         |    4 +
 arch/arm/kernel/entry-header.S       |   19 ++
 arch/arm/kernel/hw_breakpoint.c      |  464 ++++++++++++++++++++--------------
 arch/arm/kernel/ptrace.c             |    4 +-
 5 files changed, 305 insertions(+), 188 deletions(-)

[PATCH 1/8] ARM: hw_breakpoint: ensure OS lock is clear before writing to debug registers

[Will Deacon]

ARMv7 architects a system for saving and restoring the debug registers
across low-power modes. At the heart of this system is a lock register
which, when set, forbids writes to the debug registers. While locked,
writes to debug registers via the co-processor interface will result
in undefined instruction traps. Linux currently doesn't make use of
this feature because we update the debug registers on context switch
anyway, however the status of the lock is IMPLEMENTATION DEFINED on
reset.

This patch ensures that the lock is cleared during boot so that we
can write to the debug registers safely.

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

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 21e3a4a..793959e 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -768,6 +768,23 @@ static void __init reset_ctrl_regs(void *unused)
 {
 	int i;
 
+	/*
+	 * v7 debug contains save and restore registers so that debug state
+	 * can be maintained across low-power modes without leaving
+	 * the debug logic powered up. It is IMPLEMENTATION DEFINED whether
+	 * we can write to the debug registers out of reset, so we must
+	 * unlock the OS Lock Access Register to avoid taking undefined
+	 * instruction exceptions later on.
+	 */
+	if (debug_arch >= ARM_DEBUG_ARCH_V7_ECP14) {
+		/*
+		 * Unconditionally clear the lock by writing a value
+		 * other than 0xC5ACCE55 to the access register.
+		 */
+		asm volatile("mcr p14, 0, %0, c1, c0, 4" : : "r" (0));
+		isb();
+	}
+
 	if (enable_monitor_mode())
 		return;
 
@@ -810,17 +827,17 @@ static int __init arch_hw_breakpoint_init(void)
 		pr_warning("halting debug mode enabled. Assuming maximum "
 				"watchpoint size of 4 bytes.");
 	} 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);
-
 		/*
 		 * Reset the breakpoint resources. We assume that a halting
 		 * debugger will leave the world in a nice state for us.
 		 */
 		smp_call_function(reset_ctrl_regs, NULL, 1);
 		reset_ctrl_regs(NULL);
+
+		/* 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. */
-- 
1.7.0.4

[PATCH 2/8] ARM: hw_breakpoint: reset control registers in hotplug path

[Will Deacon]

The ARMv7 debug architecture doesn't make any guarantees about the
contents of debug control registers following a debug logic reset.

This patch ensures that we reset the control registers when a cpu
comes ONLINE (for example, with hotplug) so that when we enable
monitor mode while inserting a breakpoint we won't exhibit random
behaviour.

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

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 793959e..515a3c4 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -764,7 +764,7 @@ out:
 /*
  * One-time initialisation.
  */
-static void __init reset_ctrl_regs(void *unused)
+static void reset_ctrl_regs(void *unused)
 {
 	int i;
 
@@ -799,6 +799,18 @@ static void __init reset_ctrl_regs(void *unused)
 	}
 }
 
+static int __cpuinit dbg_reset_notify(struct notifier_block *self,
+				      unsigned long action, void *cpu)
+{
+	if (action == CPU_ONLINE)
+		smp_call_function_single((int)cpu, reset_ctrl_regs, NULL, 1);
+	return NOTIFY_OK;
+}
+
+static struct notifier_block __cpuinitdata dbg_reset_nb = {
+	.notifier_call = dbg_reset_notify,
+};
+
 static int __init arch_hw_breakpoint_init(void)
 {
 	int ret = 0;
@@ -846,6 +858,8 @@ static int __init arch_hw_breakpoint_init(void)
 	hook_ifault_code(2, hw_breakpoint_pending, SIGTRAP, TRAP_HWBKPT,
 			"breakpoint debug exception");
 
+	/* Register hotplug notifier. */
+	register_cpu_notifier(&dbg_reset_nb);
 out:
 	return ret;
 }
-- 
1.7.0.4

[PATCH 3/8] ARM: hw_breakpoint: correct and simplify alignment fixup code

[Will Deacon]

The current hw_breakpoint code tries to fix up the alignment of
breakpoints so that we can make use of sparse byte-address-select
bits in the control register and give the illusion that we can
set breakpoints on unaligned addresses.

Although this works on v6 cores, v7 forbids this behaviour, instead
requiring breakpoints to be set on aligned addresses and have contiguous
byte-address-select ranges depending on the instruction set in use.
For ARM the only supported size is 4 bytes, whilst Thumb-2 also permits
2 byte breakpoints (watchpoints can be of 1, 2, 4 or 8 bytes long).

This patch simplifies the alignment fixup code so that we require
addresses to be aligned to the size of the corresponding breakpoint.
This allows us to handle the common case of breaking on a half-word
aligned Thumb-2 instruction and also allows us to set byte watchpoints
on arbitrary addresses.

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

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 515a3c4..550d949 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -537,6 +537,17 @@ static int arch_build_bp_info(struct perf_event *bp)
 		return -EINVAL;
 	}
 
+	/*
+	 * Breakpoints must be of length 2 (thumb) or 4 (ARM) bytes.
+	 * Watchpoints can be of length 1, 2, 4 or 8 bytes if supported
+	 * by the hardware and must be aligned to the appropriate number of
+	 * bytes.
+	 */
+	if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE &&
+	    info->ctrl.len != ARM_BREAKPOINT_LEN_2 &&
+	    info->ctrl.len != ARM_BREAKPOINT_LEN_4)
+		return -EINVAL;
+
 	/* Address */
 	info->address = bp->attr.bp_addr;
 
@@ -561,7 +572,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 {
 	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
 	int ret = 0;
-	u32 bytelen, max_len, offset, alignment_mask = 0x3;
+	u32 offset, alignment_mask = 0x3;
 
 	/* Build the arch_hw_breakpoint. */
 	ret = arch_build_bp_info(bp);
@@ -571,32 +582,27 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 	/* Check address alignment. */
 	if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
 		alignment_mask = 0x7;
-	if (info->address & alignment_mask) {
-		/*
-		 * Try to fix the alignment. This may result in a length
-		 * that is too large, so we must check for that.
-		 */
-		bytelen = get_hbp_len(info->ctrl.len);
-		max_len = info->ctrl.type == ARM_BREAKPOINT_EXECUTE ? 4 :
-				max_watchpoint_len;
-
-		if (max_len >= 8)
-			offset = info->address & 0x7;
-		else
-			offset = info->address & 0x3;
-
-		if (bytelen > (1 << ((max_len - (offset + 1)) >> 1))) {
-			ret = -EFBIG;
-			goto out;
-		}
-
-		info->ctrl.len <<= offset;
-		info->address &= ~offset;
-
-		pr_debug("breakpoint alignment fixup: length = 0x%x, "
-			"address = 0x%x\n", info->ctrl.len, info->address);
+	offset = info->address & alignment_mask;
+	switch (offset) {
+	case 0:
+		/* Aligned */
+		break;
+	case 1:
+		/* Allow single byte watchpoint. */
+		if (info->ctrl.len == ARM_BREAKPOINT_LEN_1)
+			break;
+	case 2:
+		/* Allow halfword watchpoints and breakpoints. */
+		if (info->ctrl.len == ARM_BREAKPOINT_LEN_2)
+			break;
+	default:
+		ret = -EINVAL;
+		goto out;
 	}
 
+	info->address &= ~alignment_mask;
+	info->ctrl.len <<= offset;
+
 	/*
 	 * Currently we rely on an overflow handler to take
 	 * care of single-stepping the breakpoint when it fires.
-- 
1.7.0.4

[PATCH 4/8] ARM: hw_breakpoint: disable preemption during debug exception handling

[Will Deacon]

On ARM, debug exceptions occur in the form of data or prefetch aborts.
One difference is that debug exceptions require access to per-cpu banked
registers and data structures which are not saved in the low-level exception
code. For kernels built with CONFIG_PREEMPT, there is an unlikely scenario
that the debug handler ends up running on a different CPU from the one
that originally signalled the event, resulting in random data being read
from the wrong registers.

This patch adds a debug_entry macro to the low-level exception handling
code which checks whether the taken exception is a debug exception. If
it is, the preempt count for the faulting process is incremented. After
the debug handler has finished, the count is decremented.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/kernel/entry-armv.S    |    4 ++++
 arch/arm/kernel/entry-header.S  |   19 +++++++++++++++++++
 arch/arm/kernel/hw_breakpoint.c |   14 +++++++++-----
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index c09e357..34bbef0 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -198,6 +198,7 @@ __dabt_svc:
 	@
 	@ set desired IRQ state, then call main handler
 	@
+	debug_entry r1
 	msr	cpsr_c, r9
 	mov	r2, sp
 	bl	do_DataAbort
@@ -324,6 +325,7 @@ __pabt_svc:
 #else
 	bl	CPU_PABORT_HANDLER
 #endif
+	debug_entry r1
 	msr	cpsr_c, r9			@ Maybe enable interrupts
 	mov	r2, sp				@ regs
 	bl	do_PrefetchAbort		@ call abort handler
@@ -439,6 +441,7 @@ __dabt_usr:
 	@
 	@ IRQs on, then call the main handler
 	@
+	debug_entry r1
 	enable_irq
 	mov	r2, sp
 	adr	lr, BSYM(ret_from_exception)
@@ -703,6 +706,7 @@ __pabt_usr:
 #else
 	bl	CPU_PABORT_HANDLER
 #endif
+	debug_entry r1
 	enable_irq				@ Enable interrupts
 	mov	r2, sp				@ regs
 	bl	do_PrefetchAbort		@ call abort handler
diff --git a/arch/arm/kernel/entry-header.S b/arch/arm/kernel/entry-header.S
index d93f976..ae94649 100644
--- a/arch/arm/kernel/entry-header.S
+++ b/arch/arm/kernel/entry-header.S
@@ -165,6 +165,25 @@
 	.endm
 #endif	/* !CONFIG_THUMB2_KERNEL */
 
+	@
+	@ Debug exceptions are taken as prefetch or data aborts.
+	@ We must disable preemption during the handler so that
+	@ we can access the debug registers safely.
+	@
+	.macro	debug_entry, fsr
+#if defined(CONFIG_HAVE_HW_BREAKPOINT) && defined(CONFIG_PREEMPT)
+	ldr	r4, =0x40f		@ mask out fsr.fs
+	and	r5, r4, \fsr
+	cmp	r5, #2			@ debug exception
+	bne	1f
+	get_thread_info r10
+	ldr	r6, [r10, #TI_PREEMPT]	@ get preempt count
+	add	r11, r6, #1		@ increment it
+	str	r11, [r10, #TI_PREEMPT]
+1:
+#endif
+	.endm
+
 /*
  * These are the registers used in the syscall handler, and allow us to
  * have in theory up to 7 arguments to a function - r0 to r6.
diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 550d949..61eeb41 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -737,12 +737,12 @@ unlock:
 
 /*
  * Called from either the Data Abort Handler [watchpoint] or the
- * Prefetch Abort Handler [breakpoint].
+ * Prefetch Abort Handler [breakpoint] with preemption disabled.
  */
 static int hw_breakpoint_pending(unsigned long addr, unsigned int fsr,
 				 struct pt_regs *regs)
 {
-	int ret = 1; /* Unhandled fault. */
+	int ret = 0;
 	u32 dscr;
 
 	/* We only handle watchpoints and hardware breakpoints. */
@@ -759,11 +759,15 @@ static int hw_breakpoint_pending(unsigned long addr, unsigned int fsr,
 		watchpoint_handler(addr, regs);
 		break;
 	default:
-		goto out;
+		ret = 1; /* Unhandled fault. */
 	}
 
-	ret = 0;
-out:
+	/*
+	 * Re-enable preemption after it was disabled in the
+	 * low-level exception handling code.
+	 */
+	preempt_enable();
+
 	return ret;
 }
 
-- 
1.7.0.4

[PATCH 5/8] ARM: hw_breakpoint: don't advertise reserved breakpoints

[Will Deacon]

To permit handling of watchpoint exceptions without signalling a
debugger, it is necessary to reserve breakpoint registers for in-kernel
use only.

This patch ensures that we record and subtract the number of reserved
breakpoints from the number of usable breakpoint registers that we
advertise to userspace via the ptrace API.

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

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 61eeb41..b8acfbc 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -44,6 +44,7 @@ static DEFINE_PER_CPU(struct perf_event *, wp_on_reg[ARM_MAX_WRP]);
 
 /* Number of BRP/WRP registers on this CPU. */
 static int core_num_brps;
+static int core_num_reserved_brps;
 static int core_num_wrps;
 
 /* Debug architecture version. */
@@ -52,87 +53,6 @@ static u8 debug_arch;
 /* Maximum supported watchpoint length. */
 static u8 max_watchpoint_len;
 
-/* Determine number of BRP registers available. */
-static int get_num_brps(void)
-{
-	u32 didr;
-	ARM_DBG_READ(c0, 0, didr);
-	return ((didr >> 24) & 0xf) + 1;
-}
-
-/* Determine number of WRP registers available. */
-static int get_num_wrps(void)
-{
-	/*
-	 * FIXME: When a watchpoint fires, the only way to work out which
-	 * watchpoint it was is by disassembling the faulting instruction
-	 * and working out the address of the memory access.
-	 *
-	 * Furthermore, we can only do this if the watchpoint was precise
-	 * since imprecise watchpoints prevent us from calculating register
-	 * based addresses.
-	 *
-	 * For the time being, we only report 1 watchpoint register so we
-	 * always know which watchpoint fired. In the future we can either
-	 * add a disassembler and address generation emulator, or we can
-	 * insert a check to see if the DFAR is set on watchpoint exception
-	 * entry [the ARM ARM states that the DFAR is UNKNOWN, but
-	 * experience shows that it is set on some implementations].
-	 */
-
-#if 0
-	u32 didr, wrps;
-	ARM_DBG_READ(c0, 0, didr);
-	return ((didr >> 28) & 0xf) + 1;
-#endif
-
-	return 1;
-}
-
-int hw_breakpoint_slots(int type)
-{
-	/*
-	 * We can be called early, so don't rely on
-	 * our static variables being initialised.
-	 */
-	switch (type) {
-	case TYPE_INST:
-		return get_num_brps();
-	case TYPE_DATA:
-		return get_num_wrps();
-	default:
-		pr_warning("unknown slot type: %d\n", type);
-		return 0;
-	}
-}
-
-/* Determine debug architecture. */
-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");
-		return ARM_DEBUG_ARCH_V6;
-	}
-
-	ARM_DBG_READ(c0, 0, didr);
-	return (didr >> 16) & 0xf;
-}
-
-/* Does this core support mismatch breakpoints? */
-static int core_has_mismatch_bps(void)
-{
-	return debug_arch >= ARM_DEBUG_ARCH_V7_ECP14 && core_num_brps > 1;
-}
-
-u8 arch_get_debug_arch(void)
-{
-	return debug_arch;
-}
-
 #define READ_WB_REG_CASE(OP2, M, VAL)		\
 	case ((OP2 << 4) + M):			\
 		ARM_DBG_READ(c ## M, OP2, VAL); \
@@ -210,6 +130,110 @@ static void write_wb_reg(int n, u32 val)
 	isb();
 }
 
+/* Determine debug architecture. */
+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");
+		return ARM_DEBUG_ARCH_V6;
+	}
+
+	ARM_DBG_READ(c0, 0, didr);
+	return (didr >> 16) & 0xf;
+}
+
+u8 arch_get_debug_arch(void)
+{
+	return debug_arch;
+}
+
+/* Determine number of BRP register available. */
+static int get_num_brp_resources(void)
+{
+	u32 didr;
+	ARM_DBG_READ(c0, 0, didr);
+	return ((didr >> 24) & 0xf) + 1;
+}
+
+/* Does this core support mismatch breakpoints? */
+static int core_has_mismatch_brps(void)
+{
+	return (get_debug_arch() >= ARM_DEBUG_ARCH_V7_ECP14 &&
+		get_num_brp_resources() > 1);
+}
+
+/* Determine number of usable WRPs available. */
+static int get_num_wrps(void)
+{
+	/*
+	 * FIXME: When a watchpoint fires, the only way to work out which
+	 * watchpoint it was is by disassembling the faulting instruction
+	 * and working out the address of the memory access.
+	 *
+	 * Furthermore, we can only do this if the watchpoint was precise
+	 * since imprecise watchpoints prevent us from calculating register
+	 * based addresses.
+	 *
+	 * For the time being, we only report 1 watchpoint register so we
+	 * always know which watchpoint fired. In the future we can either
+	 * add a disassembler and address generation emulator, or we can
+	 * insert a check to see if the DFAR is set on watchpoint exception
+	 * entry [the ARM ARM states that the DFAR is UNKNOWN, but
+	 * experience shows that it is set on some implementations].
+	 */
+
+#if 0
+	int wrps;
+	u32 didr;
+	ARM_DBG_READ(c0, 0, didr);
+	wrps = ((didr >> 28) & 0xf) + 1;
+#endif
+	int wrps = 1;
+
+	if (core_has_mismatch_brps() && wrps >= get_num_brp_resources())
+		wrps = get_num_brp_resources() - 1;
+
+	return wrps;
+}
+
+/* We reserve one breakpoint for each watchpoint. */
+static int get_num_reserved_brps(void)
+{
+	if (core_has_mismatch_brps())
+		return get_num_wrps();
+	return 0;
+}
+
+/* Determine number of usable BRPs available. */
+static int get_num_brps(void)
+{
+	int brps = get_num_brp_resources();
+	if (core_has_mismatch_brps())
+		brps -= get_num_reserved_brps();
+	return brps;
+}
+
+int hw_breakpoint_slots(int type)
+{
+	/*
+	 * We can be called early, so don't rely on
+	 * our static variables being initialised.
+	 */
+	switch (type) {
+	case TYPE_INST:
+		return get_num_brps();
+	case TYPE_DATA:
+		return get_num_wrps();
+	default:
+		pr_warning("unknown slot type: %d\n", type);
+		return 0;
+	}
+}
+
 /*
  * In order to access the breakpoint/watchpoint control registers,
  * we must be running in debug monitor mode. Unfortunately, we can
@@ -325,7 +349,7 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 		ctrl_base = ARM_BASE_BCR;
 		val_base = ARM_BASE_BVR;
 		slots = __get_cpu_var(bp_on_reg);
-		max_slots = core_num_brps - 1;
+		max_slots = core_num_brps;
 
 		if (bp_is_single_step(bp)) {
 			info->ctrl.mismatch = 1;
@@ -376,7 +400,7 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 		/* Breakpoint */
 		base = ARM_BASE_BCR;
 		slots = __get_cpu_var(bp_on_reg);
-		max_slots = core_num_brps - 1;
+		max_slots = core_num_brps;
 
 		if (bp_is_single_step(bp)) {
 			i = max_slots;
@@ -610,7 +634,7 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 	 * we can use the mismatch feature as a poor-man's hardware single-step.
 	 */
 	if (WARN_ONCE(!bp->overflow_handler &&
-		(arch_check_bp_in_kernelspace(bp) || !core_has_mismatch_bps()),
+		(arch_check_bp_in_kernelspace(bp) || !core_has_mismatch_brps()),
 			"overflow handler required but none found")) {
 		ret = -EINVAL;
 		goto out;
@@ -698,7 +722,7 @@ static void breakpoint_handler(unsigned long unknown, struct pt_regs *regs)
 	/* The exception entry code places the amended lr in the PC. */
 	addr = regs->ARM_pc;
 
-	for (i = 0; i < core_num_brps; ++i) {
+	for (i = 0; i < core_num_brps + core_num_reserved_brps; ++i) {
 		rcu_read_lock();
 
 		bp = slots[i];
@@ -798,7 +822,8 @@ static void reset_ctrl_regs(void *unused)
 	if (enable_monitor_mode())
 		return;
 
-	for (i = 0; i < core_num_brps; ++i) {
+	/* We must also reset any reserved registers. */
+	for (i = 0; i < core_num_brps + core_num_reserved_brps; ++i) {
 		write_wb_reg(ARM_BASE_BCR + i, 0UL);
 		write_wb_reg(ARM_BASE_BVR + i, 0UL);
 	}
@@ -836,13 +861,15 @@ static int __init arch_hw_breakpoint_init(void)
 
 	/* Determine how many BRPs/WRPs are available. */
 	core_num_brps = get_num_brps();
+	core_num_reserved_brps = get_num_reserved_brps();
 	core_num_wrps = get_num_wrps();
 
 	pr_info("found %d breakpoint and %d watchpoint registers.\n",
-			core_num_brps, core_num_wrps);
+		core_num_brps + core_num_reserved_brps, core_num_wrps);
 
-	if (core_has_mismatch_bps())
-		pr_info("1 breakpoint reserved for watchpoint single-step.\n");
+	if (core_num_reserved_brps)
+		pr_info("%d breakpoint(s) reserved for watchpoint "
+				"single-step.\n", core_num_reserved_brps);
 
 	ARM_DBG_READ(c1, 0, dscr);
 	if (dscr & ARM_DSCR_HDBGEN) {
-- 
1.7.0.4

[PATCH 6/8] ARM: hw_breakpoint: do not allocate new breakpoints with rcu_read_lock held

[Will Deacon]

The watchpoint single-stepping code calls register_user_hw_breakpoint to
register a mismatch breakpoint for stepping over the watchpoint. This is
performed while the rcu_read_lock is held and is therefore unsafe.

This patch reworks the watchpoint stepping code so that we don't require
another perf_event for the mismatch breakpoint. Instead, we hold a separate
arch_hw_breakpoint_ctrl struct inside the watchpoint which is used exclusively
for stepping. We can check whether or not stepping is enabled when installing
or uninstalling the watchpoint and operate on the breakpoint accordingly.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/hw_breakpoint.h |    2 +-
 arch/arm/kernel/hw_breakpoint.c      |  134 +++++++++++++++++++--------------
 2 files changed, 78 insertions(+), 58 deletions(-)

diff --git a/arch/arm/include/asm/hw_breakpoint.h b/arch/arm/include/asm/hw_breakpoint.h
index 4d8ae9d..881429d 100644
--- a/arch/arm/include/asm/hw_breakpoint.h
+++ b/arch/arm/include/asm/hw_breakpoint.h
@@ -20,7 +20,7 @@ struct arch_hw_breakpoint_ctrl {
 struct arch_hw_breakpoint {
 	u32	address;
 	u32	trigger;
-	struct perf_event *suspended_wp;
+	struct arch_hw_breakpoint_ctrl step_ctrl;
 	struct arch_hw_breakpoint_ctrl ctrl;
 };
 
diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index b8acfbc..4ee9a7d 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -314,23 +314,6 @@ u8 arch_get_max_wp_len(void)
 }
 
 /*
- * Handler for reactivating a suspended watchpoint when the single
- * step `mismatch' breakpoint is triggered.
- */
-static void wp_single_step_handler(struct perf_event *bp, int unused,
-				   struct perf_sample_data *data,
-				   struct pt_regs *regs)
-{
-	perf_event_enable(counter_arch_bp(bp)->suspended_wp);
-	unregister_hw_breakpoint(bp);
-}
-
-static int bp_is_single_step(struct perf_event *bp)
-{
-	return bp->overflow_handler == wp_single_step_handler;
-}
-
-/*
  * Install a perf counter breakpoint.
  */
 int arch_install_hw_breakpoint(struct perf_event *bp)
@@ -338,29 +321,35 @@ 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;
+	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;
+
 	if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE) {
 		/* Breakpoint */
 		ctrl_base = ARM_BASE_BCR;
 		val_base = ARM_BASE_BVR;
 		slots = __get_cpu_var(bp_on_reg);
 		max_slots = core_num_brps;
-
-		if (bp_is_single_step(bp)) {
-			info->ctrl.mismatch = 1;
-			i = max_slots;
-			slots[i] = bp;
-			goto setup;
-		}
 	} else {
 		/* Watchpoint */
-		ctrl_base = ARM_BASE_WCR;
-		val_base = ARM_BASE_WVR;
+		if (info->step_ctrl.enabled) {
+			/* Install into the reserved breakpoint region. */
+			ctrl_base = ARM_BASE_BCR + core_num_brps;
+			val_base = ARM_BASE_BVR + core_num_brps;
+			/* Override the watchpoint data with the step data. */
+			addr = info->trigger & ~0x3;
+			ctrl = encode_ctrl_reg(info->step_ctrl);
+		} else {
+			ctrl_base = ARM_BASE_WCR;
+			val_base = ARM_BASE_WVR;
+		}
 		slots = __get_cpu_var(wp_on_reg);
 		max_slots = core_num_wrps;
 	}
@@ -379,12 +368,11 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 		goto out;
 	}
 
-setup:
 	/* Setup the address register. */
-	write_wb_reg(val_base + i, info->address);
+	write_wb_reg(val_base + i, addr);
 
 	/* Setup the control register. */
-	write_wb_reg(ctrl_base + i, encode_ctrl_reg(info->ctrl) | 0x1);
+	write_wb_reg(ctrl_base + i, ctrl);
 
 out:
 	return ret;
@@ -401,15 +389,12 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 		base = ARM_BASE_BCR;
 		slots = __get_cpu_var(bp_on_reg);
 		max_slots = core_num_brps;
-
-		if (bp_is_single_step(bp)) {
-			i = max_slots;
-			slots[i] = NULL;
-			goto reset;
-		}
 	} else {
 		/* Watchpoint */
-		base = ARM_BASE_WCR;
+		if (info->step_ctrl.enabled)
+			base = ARM_BASE_BCR + core_num_brps;
+		else
+			base = ARM_BASE_WCR;
 		slots = __get_cpu_var(wp_on_reg);
 		max_slots = core_num_wrps;
 	}
@@ -427,7 +412,6 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 	if (WARN_ONCE(i == max_slots, "Can't find any breakpoint slot"))
 		return;
 
-reset:
 	/* Reset the control register. */
 	write_wb_reg(base + i, 0);
 }
@@ -577,7 +561,7 @@ static int arch_build_bp_info(struct perf_event *bp)
 
 	/* Privilege */
 	info->ctrl.privilege = ARM_BREAKPOINT_USER;
-	if (arch_check_bp_in_kernelspace(bp) && !bp_is_single_step(bp))
+	if (arch_check_bp_in_kernelspace(bp))
 		info->ctrl.privilege |= ARM_BREAKPOINT_PRIV;
 
 	/* Enabled? */
@@ -663,22 +647,18 @@ static void update_mismatch_flag(int idx, int flag)
 static void watchpoint_handler(unsigned long unknown, struct pt_regs *regs)
 {
 	int i;
-	struct perf_event *bp, **slots = __get_cpu_var(wp_on_reg);
+	struct perf_event *wp, **slots = __get_cpu_var(wp_on_reg);
 	struct arch_hw_breakpoint *info;
-	struct perf_event_attr attr;
 
 	/* Without a disassembler, we can only handle 1 watchpoint. */
 	BUG_ON(core_num_wrps > 1);
 
-	hw_breakpoint_init(&attr);
-	attr.bp_addr	= regs->ARM_pc & ~0x3;
-	attr.bp_len	= HW_BREAKPOINT_LEN_4;
-	attr.bp_type	= HW_BREAKPOINT_X;
-
 	for (i = 0; i < core_num_wrps; ++i) {
 		rcu_read_lock();
 
-		if (slots[i] == NULL) {
+		wp = slots[i];
+
+		if (wp == NULL) {
 			rcu_read_unlock();
 			continue;
 		}
@@ -688,24 +668,60 @@ static void watchpoint_handler(unsigned long unknown, struct pt_regs *regs)
 		 * single watchpoint, we can set the trigger to the lowest
 		 * possible faulting address.
 		 */
-		info = counter_arch_bp(slots[i]);
-		info->trigger = slots[i]->attr.bp_addr;
+		info = counter_arch_bp(wp);
+		info->trigger = wp->attr.bp_addr;
 		pr_debug("watchpoint fired: address = 0x%x\n", info->trigger);
-		perf_bp_event(slots[i], regs);
+		perf_bp_event(wp, regs);
 
 		/*
 		 * If no overflow handler is present, insert a temporary
 		 * mismatch breakpoint so we can single-step over the
 		 * watchpoint trigger.
 		 */
-		if (!slots[i]->overflow_handler) {
-			bp = register_user_hw_breakpoint(&attr,
-							 wp_single_step_handler,
-							 current);
-			counter_arch_bp(bp)->suspended_wp = slots[i];
-			perf_event_disable(slots[i]);
+		if (!wp->overflow_handler) {
+			arch_uninstall_hw_breakpoint(wp);
+			info->step_ctrl.mismatch  = 1;
+			info->step_ctrl.len	  = ARM_BREAKPOINT_LEN_4;
+			info->step_ctrl.type	  = ARM_BREAKPOINT_EXECUTE;
+			info->step_ctrl.privilege = info->ctrl.privilege;
+			info->step_ctrl.enabled	  = 1;
+			info->trigger		  = regs->ARM_pc;
+			arch_install_hw_breakpoint(wp);
+		}
+
+		rcu_read_unlock();
+	}
+}
+
+static void watchpoint_single_step_handler(unsigned long pc)
+{
+	int i;
+	struct perf_event *wp, **slots = __get_cpu_var(wp_on_reg);
+	struct arch_hw_breakpoint *info;
+
+	for (i = 0; i < core_num_reserved_brps; ++i) {
+		rcu_read_lock();
+
+		wp = slots[i];
+
+		if (wp == NULL)
+			goto unlock;
+
+		info = counter_arch_bp(wp);
+		if (!info->step_ctrl.enabled)
+			goto unlock;
+
+		/*
+		 * Restore the original watchpoint if we've completed the
+		 * single-step.
+		 */
+		if (info->trigger != pc) {
+			arch_uninstall_hw_breakpoint(wp);
+			info->step_ctrl.enabled = 0;
+			arch_install_hw_breakpoint(wp);
 		}
 
+unlock:
 		rcu_read_unlock();
 	}
 }
@@ -722,7 +738,8 @@ static void breakpoint_handler(unsigned long unknown, struct pt_regs *regs)
 	/* The exception entry code places the amended lr in the PC. */
 	addr = regs->ARM_pc;
 
-	for (i = 0; i < core_num_brps + core_num_reserved_brps; ++i) {
+	/* Check the currently installed breakpoints first. */
+	for (i = 0; i < core_num_brps; ++i) {
 		rcu_read_lock();
 
 		bp = slots[i];
@@ -749,7 +766,7 @@ static void breakpoint_handler(unsigned long unknown, struct pt_regs *regs)
 		}
 
 unlock:
-		if ((mismatch && !info->ctrl.mismatch) || bp_is_single_step(bp)) {
+		if (mismatch && !info->ctrl.mismatch) {
 			pr_debug("breakpoint fired: address = 0x%x\n", addr);
 			perf_bp_event(bp, regs);
 		}
@@ -757,6 +774,9 @@ unlock:
 		update_mismatch_flag(i, mismatch);
 		rcu_read_unlock();
 	}
+
+	/* Handle any pending watchpoint single-step breakpoints. */
+	watchpoint_single_step_handler(addr);
 }
 
 /*
-- 
1.7.0.4

[PATCH 7/8] ARM: ptrace: fix style issue with hw_breakpoint interface

[Will Deacon]

This patch fixes a trivial style issue in ptrace.c.

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

diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index 3e97483..19c6816 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -1060,8 +1060,8 @@ static int ptrace_sethbpregs(struct task_struct *tsk, long num,
 			goto out;
 
 		if ((gen_type & implied_type) != gen_type) {
-				ret = -EINVAL;
-				goto out;
+			ret = -EINVAL;
+			goto out;
 		}
 
 		attr.bp_len	= gen_len;
-- 
1.7.0.4

[PATCH 8/8] ARM: hw_breakpoint: fix warnings generated by sparse

[Will Deacon]

sparse doesn't like per-cpu accesses such as:

static DEFINE_PER_CPU(struct perf_event *, foo[MAXLEN]);
struct perf_event **bar = __get_cpu_var(foo);

and shouts quite loudly about it:

| warning: incorrect type in assignment (different modifiers)
|    expected struct perf_event **slots
|    got struct perf_event *[noderef] *<noident>

This patch adds casts to these sorts of assignments in hw_breakpoint.c
in order to silence the warnings.

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

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index 4ee9a7d..97dc36a 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -335,7 +335,7 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 		/* Breakpoint */
 		ctrl_base = ARM_BASE_BCR;
 		val_base = ARM_BASE_BVR;
-		slots = __get_cpu_var(bp_on_reg);
+		slots = (struct perf_event **)__get_cpu_var(bp_on_reg);
 		max_slots = core_num_brps;
 	} else {
 		/* Watchpoint */
@@ -350,7 +350,7 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 			ctrl_base = ARM_BASE_WCR;
 			val_base = ARM_BASE_WVR;
 		}
-		slots = __get_cpu_var(wp_on_reg);
+		slots = (struct perf_event **)__get_cpu_var(wp_on_reg);
 		max_slots = core_num_wrps;
 	}
 
@@ -387,7 +387,7 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 	if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE) {
 		/* Breakpoint */
 		base = ARM_BASE_BCR;
-		slots = __get_cpu_var(bp_on_reg);
+		slots = (struct perf_event **)__get_cpu_var(bp_on_reg);
 		max_slots = core_num_brps;
 	} else {
 		/* Watchpoint */
@@ -395,7 +395,7 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 			base = ARM_BASE_BCR + core_num_brps;
 		else
 			base = ARM_BASE_WCR;
-		slots = __get_cpu_var(wp_on_reg);
+		slots = (struct perf_event **)__get_cpu_var(wp_on_reg);
 		max_slots = core_num_wrps;
 	}
 
@@ -647,9 +647,11 @@ static void update_mismatch_flag(int idx, int flag)
 static void watchpoint_handler(unsigned long unknown, struct pt_regs *regs)
 {
 	int i;
-	struct perf_event *wp, **slots = __get_cpu_var(wp_on_reg);
+	struct perf_event *wp, **slots;
 	struct arch_hw_breakpoint *info;
 
+	slots = (struct perf_event **)__get_cpu_var(wp_on_reg);
+
 	/* Without a disassembler, we can only handle 1 watchpoint. */
 	BUG_ON(core_num_wrps > 1);
 
@@ -696,9 +698,11 @@ static void watchpoint_handler(unsigned long unknown, struct pt_regs *regs)
 static void watchpoint_single_step_handler(unsigned long pc)
 {
 	int i;
-	struct perf_event *wp, **slots = __get_cpu_var(wp_on_reg);
+	struct perf_event *wp, **slots;
 	struct arch_hw_breakpoint *info;
 
+	slots = (struct perf_event **)__get_cpu_var(wp_on_reg);
+
 	for (i = 0; i < core_num_reserved_brps; ++i) {
 		rcu_read_lock();
 
@@ -731,10 +735,12 @@ static void breakpoint_handler(unsigned long unknown, struct pt_regs *regs)
 	int i;
 	int mismatch;
 	u32 ctrl_reg, val, addr;
-	struct perf_event *bp, **slots = __get_cpu_var(bp_on_reg);
+	struct perf_event *bp, **slots;
 	struct arch_hw_breakpoint *info;
 	struct arch_hw_breakpoint_ctrl ctrl;
 
+	slots = (struct perf_event **)__get_cpu_var(bp_on_reg);
+
 	/* The exception entry code places the amended lr in the PC. */
 	addr = regs->ARM_pc;
 
-- 
1.7.0.4

[PATCH 5/8] ARM: hw_breakpoint: don't advertise reserved breakpoints

[Jamie Iles]

On Mon, Nov 29, 2010 at 05:34:45PM +0000, Will Deacon wrote:
> To permit handling of watchpoint exceptions without signalling a
> debugger, it is necessary to reserve breakpoint registers for in-kernel
> use only.
> 
> This patch ensures that we record and subtract the number of reserved
> breakpoints from the number of usable breakpoint registers that we
> advertise to userspace via the ptrace API.
> 
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm/kernel/hw_breakpoint.c |  205 ++++++++++++++++++++++-----------------
>  1 files changed, 116 insertions(+), 89 deletions(-)
> 
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index 61eeb41..b8acfbc 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -44,6 +44,7 @@ static DEFINE_PER_CPU(struct perf_event *, wp_on_reg[ARM_MAX_WRP]);
>  
[...]
> +/* Determine number of BRP register available. */
> +static int get_num_brp_resources(void)
> +{
> +	u32 didr;
> +	ARM_DBG_READ(c0, 0, didr);
> +	return ((didr >> 24) & 0xf) + 1;
> +}
> +
> +/* Does this core support mismatch breakpoints? */
> +static int core_has_mismatch_brps(void)
> +{
> +	return (get_debug_arch() >= ARM_DEBUG_ARCH_V7_ECP14 &&
> +		get_num_brp_resources() > 1);
> +}
> +
> +/* Determine number of usable WRPs available. */
> +static int get_num_wrps(void)
> +{
> +	/*
> +	 * FIXME: When a watchpoint fires, the only way to work out which
> +	 * watchpoint it was is by disassembling the faulting instruction
> +	 * and working out the address of the memory access.
> +	 *
> +	 * Furthermore, we can only do this if the watchpoint was precise
> +	 * since imprecise watchpoints prevent us from calculating register
> +	 * based addresses.
> +	 *
> +	 * For the time being, we only report 1 watchpoint register so we
> +	 * always know which watchpoint fired. In the future we can either
> +	 * add a disassembler and address generation emulator, or we can
> +	 * insert a check to see if the DFAR is set on watchpoint exception
> +	 * entry [the ARM ARM states that the DFAR is UNKNOWN, but
> +	 * experience shows that it is set on some implementations].
> +	 */
> +
> +#if 0
> +	int wrps;
> +	u32 didr;
> +	ARM_DBG_READ(c0, 0, didr);
> +	wrps = ((didr >> 28) & 0xf) + 1;
> +#endif
> +	int wrps = 1;
> +
> +	if (core_has_mismatch_brps() && wrps >= get_num_brp_resources())
> +		wrps = get_num_brp_resources() - 1;
> +
> +	return wrps;
> +}
Hi Will,

Minor nitpick, is the comment above still valid? It looks like this could 
return something other than 1. Is this to handle the case when there aren't 
any watchpoint registers?

Jamie

[PATCH 5/8] ARM: hw_breakpoint: don't advertise reserved breakpoints

[Will Deacon]

Hi Jamie,

> > +	/*
> > +	 * FIXME: When a watchpoint fires, the only way to work out which
> > +	 * watchpoint it was is by disassembling the faulting instruction
> > +	 * and working out the address of the memory access.
> > +	 *
> > +	 * Furthermore, we can only do this if the watchpoint was precise
> > +	 * since imprecise watchpoints prevent us from calculating register
> > +	 * based addresses.
> > +	 *
> > +	 * For the time being, we only report 1 watchpoint register so we
> > +	 * always know which watchpoint fired. In the future we can either
> > +	 * add a disassembler and address generation emulator, or we can
> > +	 * insert a check to see if the DFAR is set on watchpoint exception
> > +	 * entry [the ARM ARM states that the DFAR is UNKNOWN, but
> > +	 * experience shows that it is set on some implementations].
> > +	 */
> > +
> > +#if 0
> > +	int wrps;
> > +	u32 didr;
> > +	ARM_DBG_READ(c0, 0, didr);
> > +	wrps = ((didr >> 28) & 0xf) + 1;
> > +#endif
> > +	int wrps = 1;
> > +
> > +	if (core_has_mismatch_brps() && wrps >= get_num_brp_resources())
> > +		wrps = get_num_brp_resources() - 1;
> > +
> > +	return wrps;
> > +}
> Hi Will,
> 
> Minor nitpick, is the comment above still valid? It looks like this could
> return something other than 1. Is this to handle the case when there aren't
> any watchpoint registers?

The comment still stands because we can't determine which watchpoint fired
if we allow more than one. Since we must reserve a breakpoint to handle stepping
over the watchpoint, we need to ensure that we truncate the number of usable
watchpoints to be the number of breakpoints - 1 (so that there is always 1
hardware breakpoint available).

Currently, all the code above ends up doing is checking that we have more than
1 breakpoint available if we want watchpoints.

I could update the comment to say that we might advertise 0 watchpoints in the
case that only 1 breakpoint is available if you like?

Will

[PATCH 5/8] ARM: hw_breakpoint: don't advertise reserved breakpoints

[Jamie Iles]

On Tue, Nov 30, 2010 at 10:12:51AM -0000, Will Deacon wrote:
> Hi Jamie,
> 
> > > +	/*
> > > +	 * FIXME: When a watchpoint fires, the only way to work out which
> > > +	 * watchpoint it was is by disassembling the faulting instruction
> > > +	 * and working out the address of the memory access.
> > > +	 *
> > > +	 * Furthermore, we can only do this if the watchpoint was precise
> > > +	 * since imprecise watchpoints prevent us from calculating register
> > > +	 * based addresses.
> > > +	 *
> > > +	 * For the time being, we only report 1 watchpoint register so we
> > > +	 * always know which watchpoint fired. In the future we can either
> > > +	 * add a disassembler and address generation emulator, or we can
> > > +	 * insert a check to see if the DFAR is set on watchpoint exception
> > > +	 * entry [the ARM ARM states that the DFAR is UNKNOWN, but
> > > +	 * experience shows that it is set on some implementations].
> > > +	 */
> > > +
> > > +#if 0
> > > +	int wrps;
> > > +	u32 didr;
> > > +	ARM_DBG_READ(c0, 0, didr);
> > > +	wrps = ((didr >> 28) & 0xf) + 1;
> > > +#endif
> > > +	int wrps = 1;
> > > +
> > > +	if (core_has_mismatch_brps() && wrps >= get_num_brp_resources())
> > > +		wrps = get_num_brp_resources() - 1;
> > > +
> > > +	return wrps;
> > > +}
> > Hi Will,
> > 
> > Minor nitpick, is the comment above still valid? It looks like this could
> > return something other than 1. Is this to handle the case when there aren't
> > any watchpoint registers?
> 
> The comment still stands because we can't determine which watchpoint fired
> if we allow more than one. Since we must reserve a breakpoint to handle stepping
> over the watchpoint, we need to ensure that we truncate the number of usable
> watchpoints to be the number of breakpoints - 1 (so that there is always 1
> hardware breakpoint available).
> 
> Currently, all the code above ends up doing is checking that we have more than
> 1 breakpoint available if we want watchpoints.
> 
> I could update the comment to say that we might advertise 0 watchpoints in the
> case that only 1 breakpoint is available if you like?
It was more out of curiosity really! If you're in there again it might be 
worth updating the comment otherwise it's not obvious why the code doesn't 
just return a static 1.

Jamie

[PATCH 5/8] ARM: hw_breakpoint: don't advertise reserved breakpoints

[Will Deacon]

> > Currently, all the code above ends up doing is checking that we have more than
> > 1 breakpoint available if we want watchpoints.
> >
> > I could update the comment to say that we might advertise 0 watchpoints in the
> > case that only 1 breakpoint is available if you like?
> It was more out of curiosity really! If you're in there again it might be
> worth updating the comment otherwise it's not obvious why the code doesn't
> just return a static 1.

I've updated the comment so you should see it in v3!

Will