ARM64: More flexible HW watchpoint

ARM64 hardware supports watchpoint at any double word aligned address. However, it can select any consecutive bytes from offset 0 to 7 from that base address. For example, if base address is programmed as 0x420030 and byte select is 0x1C, then access of 0x420032,0x420033 and 0x420034 will generate a watchpoint exception.

Currently, we do not have such modularity. We can only program byte, halfword, word and double word access exception from any base address.

This patch adds support to overcome above limitations.

Signed-off-by Pratyush Anand <[email protected]>

---
 arch/arm64/include/asm/hw_breakpoint.h |  2 +-
 arch/arm64/kernel/hw_breakpoint.c      | 45 ++++++++++++++++------------------
 arch/arm64/kernel/ptrace.c             |  5 ++--
 3 files changed, 25 insertions(+), 27 deletions(-)

arch/arm64/include/asm/hw_breakpoint.h +1 -1

index 115ea2a64520..4f4e58bee9bc 100644
@@ -118,7 +118,7 @@ struct perf_event;
 struct pmu;
 
 extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
-				  int *gen_len, int *gen_type);
+				  int *gen_len, int *gen_type, int *offset);
 extern int arch_check_bp_in_kernelspace(struct perf_event *bp);
 extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,

arch/arm64/kernel/hw_breakpoint.c +1 -1

index 26a6bf77d272..3c2b96803eba 100644
@@ -349,7 +349,7 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
  * to generic breakpoint descriptions.
  */
 int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
-			   int *gen_len, int *gen_type)
+			   int *gen_len, int *gen_type, int *offset)
 {
 	/* Type */
 	switch (ctrl.type) {

arch/arm64/kernel/hw_breakpoint.c +1 -4

 	/* Len */
-	switch (ctrl.len) {
+	switch (ctrl.len >> *offset) {
 	case ARM_BREAKPOINT_LEN_1:
 		*gen_len = HW_BREAKPOINT_LEN_1;
 		break;

@@ -517,18 +519,17 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 		default:
 			return -EINVAL;
 		}
-
-		info->address &= ~alignment_mask;
-		info->ctrl.len <<= offset;
 	} else {
 		if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE)

arch/arm64/kernel/hw_breakpoint.c +3 -12

 	/*
 	 * Disallow per-task kernel breakpoints since these would
 	 * complicate the stepping code.

@@ -665,8 +666,8 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 			      struct pt_regs *regs)
 {
 	int i, step = 0, *kernel_step, access;
-	u32 ctrl_reg;
-	u64 val, alignment_mask;
+	u32 ctrl_reg, lens, lene;
+	u64 val;
 	struct perf_event *wp, **slots;
 	struct debug_info *debug_info;
 	struct arch_hw_breakpoint *info;

@@ -684,25 +685,21 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 			goto unlock;
 
 		info = counter_arch_bp(wp);
-		/* AArch32 watchpoints are either 4 or 8 bytes aligned. */
-		if (is_compat_task()) {
-			if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
-				alignment_mask = 0x7;
-			else
-				alignment_mask = 0x3;
-		} else {
-			alignment_mask = 0x7;
-		}
 
-		/* Check if the watchpoint value matches. */
+		/* Check if the watchpoint value and byte select match. */

arch/arm64/kernel/hw_breakpoint.c +8 -0

+		/*
+		 * FIXME: reported address can be anywhere between "the
+		 * lowest address accessed by the memory access that
+		 * triggered the watchpoint" and "the highest watchpointed
+		 * address accessed by the memory access". So, it may not
+		 * lie in the interval of watchpoint address range.
+		 */
+		if (addr < val + lens || addr > val + lene)
 			goto unlock;
 
 		/*

arch/arm64/kernel/ptrace.c +2 -2

index e0c81da60f76..0eb366a94382 100644
@@ -327,13 +327,13 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
 				     struct arch_hw_breakpoint_ctrl ctrl,
 				     struct perf_event_attr *attr)
 {
-	int err, len, type, disabled = !ctrl.enabled;
+	int err, len, type, offset, disabled = !ctrl.enabled;
 
 	attr->disabled = disabled;
 	if (disabled)
 		return 0;
 
-	err = arch_bp_generic_fields(ctrl, &len, &type);
+	err = arch_bp_generic_fields(ctrl, &len, &type, &offset);
 	if (err)
 		return err;
 

Hi Pratyush,

Quoted 48 lines · Pratyush Anand

ARM64 hardware supports watchpoint at any double word aligned address. However, it can select any consecutive bytes from offset 0 to 7 from that base address. For example, if base address is programmed as 0x420030 and byte select is 0x1C, then access of 0x420032,0x420033 and 0x420034 will generate a watchpoint exception.

Currently, we do not have such modularity. We can only program byte, halfword, word and double word access exception from any base address.

This patch adds support to overcome above limitations.

Signed-off-by Pratyush Anand <[email protected]>

---
 arch/arm64/include/asm/hw_breakpoint.h |  2 +-
 arch/arm64/kernel/hw_breakpoint.c      | 45 ++++++++++++++++------------------
 arch/arm64/kernel/ptrace.c             |  5 ++--
 3 files changed, 25 insertions(+), 27 deletions(-)

arch/arm64/include/asm/hw_breakpoint.h +1 -1

index 115ea2a64520..4f4e58bee9bc 100644
@@ -118,7 +118,7 @@ struct perf_event;
 struct pmu;
 
 extern int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
-				  int *gen_len, int *gen_type);
+				  int *gen_len, int *gen_type, int *offset);
 extern int arch_check_bp_in_kernelspace(struct perf_event *bp);
 extern int arch_validate_hwbkpt_settings(struct perf_event *bp);
 extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,

arch/arm64/kernel/hw_breakpoint.c +1 -1

index 26a6bf77d272..3c2b96803eba 100644
@@ -349,7 +349,7 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
  * to generic breakpoint descriptions.
  */
 int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
-			   int *gen_len, int *gen_type)
+			   int *gen_len, int *gen_type, int *offset)
 {
 	/* Type */
 	switch (ctrl.type) {

@@ -369,8 +369,10 @@ int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl,
 		return -EINVAL;
 	}
 

arch/arm64/kernel/hw_breakpoint.c int arch_bp_generic_fields(struct arch_hw_breakpoint_ctrl ctrl, · line 372

Quoted 1 line

arch/arm64/kernel/hw_breakpoint.c +1 -0

+	*offset = ffs(ctrl.len) - 1;

Do we want to fail the length == 0 case early? If you do add that check, then you can use __ffs here instead.

Quoted 24 lines

arch/arm64/kernel/hw_breakpoint.c +2 -6

 	/* Len */
-	switch (ctrl.len) {
+	switch (ctrl.len >> *offset) {
 	case ARM_BREAKPOINT_LEN_1:
 		*gen_len = HW_BREAKPOINT_LEN_1;
 		break;

@@ -517,18 +519,17 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
 		default:
 			return -EINVAL;
 		}
-
-		info->address &= ~alignment_mask;
-		info->ctrl.len <<= offset;
 	} else {
 		if (info->ctrl.type == ARM_BREAKPOINT_EXECUTE)
 			alignment_mask = 0x3;
 		else
 			alignment_mask = 0x7;
-		if (info->address & alignment_mask)
-			return -EINVAL;
+		offset = info->address & alignment_mask;
 	}
 

arch/arm64/kernel/hw_breakpoint.c int arch_validate_hwbkpt_settings(struct perf_event *bp) · line 531

Quoted 2 lines

arch/arm64/kernel/hw_breakpoint.c +2 -0

+	info->address &= ~alignment_mask;
+	info->ctrl.len <<= offset;

Urgh, I really hate all this converting to and fro to keep perf happy. FWIW, I'm at the point where I would seriously consider ripping out the hw_breakpoint code and replacing it with a ptrace-specific backend so we just drop all this crap. The only people that seem to use the perf interface are those running the (failing) selftests. Given that we have to have a bloody thread switch notifier *anyway*, all perf seems to give us is complexity and restrictions.

Quoted 29 lines

arch/arm64/kernel/hw_breakpoint.c +2 -11

 	/*
 	 * Disallow per-task kernel breakpoints since these would
 	 * complicate the stepping code.

@@ -665,8 +666,8 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 			      struct pt_regs *regs)
 {
 	int i, step = 0, *kernel_step, access;
-	u32 ctrl_reg;
-	u64 val, alignment_mask;
+	u32 ctrl_reg, lens, lene;
+	u64 val;
 	struct perf_event *wp, **slots;
 	struct debug_info *debug_info;
 	struct arch_hw_breakpoint *info;

@@ -684,25 +685,21 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 			goto unlock;
 
 		info = counter_arch_bp(wp);
-		/* AArch32 watchpoints are either 4 or 8 bytes aligned. */
-		if (is_compat_task()) {
-			if (info->ctrl.len == ARM_BREAKPOINT_LEN_8)
-				alignment_mask = 0x7;
-			else
-				alignment_mask = 0x3;
-		} else {
-			alignment_mask = 0x7;
-		}
 

arch/arm64/kernel/hw_breakpoint.c static int watchpoint_handler(unsigned long addr, unsigned int esr, · line 694

Quoted 12 lines

arch/arm64/kernel/hw_breakpoint.c +3 -6

-		/* Check if the watchpoint value matches. */
+		/* Check if the watchpoint value and byte select match. */
 		val = read_wb_reg(AARCH64_DBG_REG_WVR, i);
-		if (val != (addr & ~alignment_mask))
-			goto unlock;
-
-		/* Possible match, check the byte address select to confirm. */
 		ctrl_reg = read_wb_reg(AARCH64_DBG_REG_WCR, i);
 		decode_ctrl_reg(ctrl_reg, &ctrl);
-		if (!((1 << (addr & alignment_mask)) & ctrl.len))
+		lens = ffs(ctrl.len) - 1;
+		lene = fls(ctrl.len) - 1;

Again, you can use the '__' variants to avoid the '- 1'.

Quoted 33 lines

arch/arm64/kernel/hw_breakpoint.c +8 -0

+		/*
+		 * FIXME: reported address can be anywhere between "the
+		 * lowest address accessed by the memory access that
+		 * triggered the watchpoint" and "the highest watchpointed
+		 * address accessed by the memory access". So, it may not
+		 * lie in the interval of watchpoint address range.
+		 */
+		if (addr < val + lens || addr > val + lene)
 			goto unlock;
 
 		/*

arch/arm64/kernel/ptrace.c +2 -2

index e0c81da60f76..0eb366a94382 100644
@@ -327,13 +327,13 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
 				     struct arch_hw_breakpoint_ctrl ctrl,
 				     struct perf_event_attr *attr)
 {
-	int err, len, type, disabled = !ctrl.enabled;
+	int err, len, type, offset, disabled = !ctrl.enabled;
 
 	attr->disabled = disabled;
 	if (disabled)
 		return 0;
 
-	err = arch_bp_generic_fields(ctrl, &len, &type);
+	err = arch_bp_generic_fields(ctrl, &len, &type, &offset);
 	if (err)
 		return err;
 

arch/arm64/kernel/ptrace.c static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type, · line 355

Quoted 4 lines

arch/arm64/kernel/ptrace.c +1 -0

@@ -352,6 +352,7 @@ static int ptrace_hbp_fill_attr_ctrl(unsigned int note_type,
 
 	attr->bp_len	= len;
 	attr->bp_type	= type;
+	attr->bp_addr	+= offset;

That's going to look pretty bizarre from userspace if it decides to read back the address registers to find that they've mysteriously changed.

Perhaps ptrace_hbp_get_addr needs to retrieve the address from the arch_hw_breakpoint, like we do for the ctrl register. What do you think?

From: Pavel Labath <[email protected]>

Arm64 hardware does not always report a watchpoint hit address that matches one of the watchpoints set. It can also report an address "near" the watchpoint if a single instruction access both watched and unwatched addresses. There is no straight-forward way, short of disassembling the offending instruction, to map that address back to the watchpoint.

Previously, when the hardware reported a watchpoint hit on an address that did not match our watchpoint (this happens in case of instructions which access large chunks of memory such as "stp") the process would enter a loop where we would be continually resuming it (because we did not recognise that watchpoint hit) and it would keep hitting the watchpoint again and again. The tracing process would never get notified of the watchpoint hit.

This commit fixes the problem by looking at the watchpoints near the address reported by the hardware. If the address does not exactly match one of the watchpoints we have set, it attributes the hit to the nearest watchpoint we have. This heuristic is a bit dodgy, but I don't think we can do much more, given the hardware limitations.

[panand: reworked to rebase on his patches]

Signed-off-by Pavel Labath <[email protected]>

Signed-off-by Pratyush Anand <[email protected]>

---
 arch/arm64/kernel/hw_breakpoint.c | 94 +++++++++++++++++++++++++++------------
 1 file changed, 66 insertions(+), 28 deletions(-)

arch/arm64/kernel/hw_breakpoint.c +58 -26

index 3c2b96803eba..c57bc90b8286 100644
@@ -662,11 +662,46 @@ unlock:
 }
 NOKPROBE_SYMBOL(breakpoint_handler);
 
+/*
+ * Arm64 hardware does not always report a watchpoint hit address that matches
+ * one of the watchpoints set. It can also report an address "near" the
+ * watchpoint if a single instruction access both watched and unwatched
+ * addresses. There is no straight-forward way, short of disassembling the
+ * offending instruction, to map that address back to the watchpoint. This
+ * function computes the distance of the memory access from the watchpoint as a
+ * heuristic for the likelyhood that a given access triggered the watchpoint.
+ *
+ * See Section D2.10.5 "Determining the memory location that caused a Watchpoint
+ * exception" of ARMv8 Architecture Reference Manual for details.
+ *
+ * The function returns the distance of the address from the bytes watched by
+ * the watchpoint. In case of an exact match, it returns 0.
+ */
+static u64 get_distance_from_watchpoint(unsigned long addr, u64 val,
+					struct arch_hw_breakpoint_ctrl *ctrl)
+{
+	u64 wp_low, wp_high;
+	u32 lens, lene;
+
+	lens = ffs(ctrl->len) - 1;
+	lene = fls(ctrl->len) - 1;
+
+	wp_low = val + lens;
+	wp_high = val + lene;
+	if (addr < wp_low)
+		return wp_low - addr;
+	else if (addr > wp_high)
+		return addr - wp_high;
+	else
+		return 0;
+}
+
 static int watchpoint_handler(unsigned long addr, unsigned int esr,
 			      struct pt_regs *regs)
 {
-	int i, step = 0, *kernel_step, access;
-	u32 ctrl_reg, lens, lene;
+	int i, step = 0, *kernel_step, access, closest_match = 0;
+	u64 min_dist = -1, dist;
+	u32 ctrl_reg;
 	u64 val;
 	struct perf_event *wp, **slots;
 	struct debug_info *debug_info;

@@ -676,31 +711,15 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 	slots = this_cpu_ptr(wp_on_reg);
 	debug_info = &current->thread.debug;
 
+	/*
+	 * Find all watchpoints that match the reported address. If no exact
+	 * match is found. Attribute the hit to the closest watchpoint.
+	 */
+	rcu_read_lock();
 	for (i = 0; i < core_num_wrps; ++i) {
-		rcu_read_lock();
-
 		wp = slots[i];
-
 		if (wp == NULL)
-			goto unlock;
-
-		info = counter_arch_bp(wp);
-
-		/* Check if the watchpoint value and byte select match. */
-		val = read_wb_reg(AARCH64_DBG_REG_WVR, i);
-		ctrl_reg = read_wb_reg(AARCH64_DBG_REG_WCR, i);
-		decode_ctrl_reg(ctrl_reg, &ctrl);
-		lens = ffs(ctrl.len) - 1;
-		lene = fls(ctrl.len) - 1;
-		/*
-		 * FIXME: reported address can be anywhere between "the
-		 * lowest address accessed by the memory access that
-		 * triggered the watchpoint" and "the highest watchpointed
-		 * address accessed by the memory access". So, it may not
-		 * lie in the interval of watchpoint address range.
-		 */
-		if (addr < val + lens || addr > val + lene)
-			goto unlock;
+			continue;
 
 		/*
 		 * Check that the access type matches.

@@ -709,18 +728,37 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 		access = (esr & AARCH64_ESR_ACCESS_MASK) ? HW_BREAKPOINT_W :
 			 HW_BREAKPOINT_R;
 		if (!(access & hw_breakpoint_type(wp)))
-			goto unlock;
+			continue;
+
+		/* Check if the watchpoint value and byte select match. */
+		val = read_wb_reg(AARCH64_DBG_REG_WVR, i);
+		ctrl_reg = read_wb_reg(AARCH64_DBG_REG_WCR, i);
+		decode_ctrl_reg(ctrl_reg, &ctrl);
+		dist = get_distance_from_watchpoint(addr, val, &ctrl);
+		if (dist < min_dist) {
+			min_dist = dist;
+			closest_match = i;
+		}
+		/* Is this an exact match? */
+		if (dist != 0)
+			continue;
 
+		info = counter_arch_bp(wp);
 		info->trigger = addr;
 		perf_bp_event(wp, regs);
 
 		/* Do we need to handle the stepping? */
 		if (is_default_overflow_handler(wp))
 			step = 1;
-

Quoted 10 lines

              /* Do we need to handle the stepping? */
              if (is_default_overflow_handler(wp))
                      step = 1;

(quoted diff) +2 -3

-
-unlock:
-             rcu_read_unlock();
      }
+     if (min_dist > 0 && min_dist != -1) {
+             /* No exact match found. */

Review

Quoted 5 lines

(quoted diff) +5 -0

+             wp = slots[closest_match];
+             info = counter_arch_bp(wp);
+             info->trigger = addr;
+             perf_bp_event(wp, regs);
+     }

Why don't we need to bother with the stepping in the case of a non-exact match?

Quoted 8 lines

Quoted 8 lines

+ if (min_dist > 0 && min_dist != -1) {

Quoted 6 lines

Quoted 6 lines

(quoted diff) +6 -0

+             /* No exact match found. */
+             wp = slots[closest_match];
+             info = counter_arch_bp(wp);
+             info->trigger = addr;
+             perf_bp_event(wp, regs);
+     }

Quoted 2 lines

Quoted 2 lines

Why don't we need to bother with the stepping in the case of a non-exact match?

Good catch. I think we do. I must have dropped that part somehow.

Pratyush, could you include the attached fixup in the next batch?