[PATCH 0/4] ARM: hw_breakpoint: fixes and improvements

[PATCH 0/4] ARM: hw_breakpoint: fixes and improvements

[Will Deacon]

Hello,

This patch series contains a number of fixes and improvements to the
hw_breakpoint implementation for ARM. A quick summary:

* Support for cores that use the OS lock for debug register save/restore
  out of reset.

* A vastly simplified alignment model to match the v7 architecture.
  Breakpoints must be the same length as an instruction and be aligned
  to the same boundary. Watchpoints can be 1, 2, 4 or 8 bytes long and
  aligned to the appropriate datasize (byte, half-word, word, double-word).

* Minor style fix in ptrace.c

All feedback welcome!

Will Deacon (4):
  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: ptrace: fix style issue with hw_breakpoint interface

 arch/arm/kernel/hw_breakpoint.c |   99 ++++++++++++++++++++++++++------------
 arch/arm/kernel/ptrace.c        |    4 +-
 2 files changed, 70 insertions(+), 33 deletions(-)

[PATCH 1/4] 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/4] 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/4] 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/4] 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 0/4] ARM: hw_breakpoint: fixes and improvements

[Russell King - ARM Linux]

On Thu, Nov 25, 2010 at 05:08:15PM +0000, Will Deacon wrote:
> All feedback welcome!

While in the subject of fixes, sparse seems to have a problem with
hw_breakpoint's usage of percpu stuff:

  CHECK   arch/arm/kernel/hw_breakpoint.c
arch/arm/kernel/hw_breakpoint.c:326:23: warning: incorrect type in assignment (different modifiers)
arch/arm/kernel/hw_breakpoint.c:326:23:    expected struct perf_event **slots
arch/arm/kernel/hw_breakpoint.c:326:23:    got struct perf_event *[noderef] *<noident>
arch/arm/kernel/hw_breakpoint.c:339:23: warning: incorrect type in assignment (different modifiers)
arch/arm/kernel/hw_breakpoint.c:339:23:    expected struct perf_event **slots
arch/arm/kernel/hw_breakpoint.c:339:23:    got struct perf_event *[noderef] *<noident>
arch/arm/kernel/hw_breakpoint.c:377:23: warning: incorrect type in assignment (different modifiers)
arch/arm/kernel/hw_breakpoint.c:377:23:    expected struct perf_event **slots
arch/arm/kernel/hw_breakpoint.c:377:23:    got struct perf_event *[noderef] *<noident>
arch/arm/kernel/hw_breakpoint.c:388:23: warning: incorrect type in assignment (different modifiers)
arch/arm/kernel/hw_breakpoint.c:388:23:    expected struct perf_event **slots t perf_event *[noderef] *<noident>
arch/arm/kernel/hw_breakpoint.c:635:42: warning: incorrect type in initializer (different modifiers)
arch/arm/kernel/hw_breakpoint.c:635:42:    expected struct perf_event **slots
arch/arm/kernel/hw_breakpoint.c:635:42:    got struct perf_event *[noderef] *<noident>
arch/arm/kernel/hw_breakpoint.c:687:42: warning: incorrect type in initializer (different modifiers)
arch/arm/kernel/hw_breakpoint.c:687:42:    expected struct perf_event **slots
arch/arm/kernel/hw_breakpoint.c:687:42:    got struct perf_event *[noderef] *<noident>

perf_event.c seems to also have problems too:

  CHECK   arch/arm/kernel/perf_event.c
arch/arm/kernel/perf_event.c:37:1: warning: symbol 'pmu_lock' was not declared. Should it be static?
arch/arm/kernel/perf_event.c:70:1: warning: symbol 'cpu_hw_events' was not declared. Should it be static?
arch/arm/kernel/perf_event.c:1006:1: warning: symbol 'armv6pmu_enable_event' was not declared. Should it be static?
arch/arm/kernel/perf_event.c:1113:1: warning: symbol 'armv6pmu_stop' was not declared. Should it be static?
arch/arm/kernel/perf_event.c:1956:6: warning: symbol 'armv7pmu_enable_event' was not declared. Should it be static?
arch/arm/kernel/perf_event.c:3072:14: warning: incorrect type in argument 1 (different address spaces)
arch/arm/kernel/perf_event.c:3072:14:    expected void const volatile [noderef] <asn:1>*<noident>
arch/arm/kernel/perf_event.c:3072:14:    got struct frame_tail *tail
arch/arm/kernel/perf_event.c:3074:49: warning: incorrect type in argument 2 (different address spaces)
arch/arm/kernel/perf_event.c:3074:49:    expected void const [noderef] <asn:1>*from
arch/arm/kernel/perf_event.c:3074:49:    got struct frame_tail *tail

[PATCH 0/4] ARM: hw_breakpoint: fixes and improvements

[Will Deacon]

Hi Russell,

On Sat, 2010-11-27 at 12:39 +0000, Russell King - ARM Linux wrote:
> On Thu, Nov 25, 2010 at 05:08:15PM +0000, Will Deacon wrote:
> > All feedback welcome!
> 
> While in the subject of fixes, sparse seems to have a problem with
> hw_breakpoint's usage of percpu stuff:
> 
[...]

This appears to be because I access something declared as perf_event *[]
using something of type perf_event **. I plan on re-rolling these fixes
anyway as I found a problem with CONFIG_PREEMPT, so I'll add the
necessary casts to silence sparse to the series.

> perf_event.c seems to also have problems too:
> 
I don't think you've pulled my perf-split branch yet, so if you hold
fire on that I'll add some fixes to get rid of these warnings [I think I
noticed one of the issues before] and send another pull request in the
week.

Thanks!

Will