[RFC PATCH 0/4] ARM: add support for hw-breakpoints [v2]

[RFC PATCH 0/4] ARM: add support for hw-breakpoints [v2]

[Will Deacon]

This is version 2 of the RFC originally posted here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2010-February/009084.html

Changes from the previous version include:

- Based on 2.6.33
- Removal of self-modifying code to access the debug registers
- Control and value registers now reset during init
- Removal of .name field from struct arch_hw_breakpoint and associated
  symbol resolution [following comments made about patches for PPC].

All comments welcome [the last submission didn't attract any!].

Cc: Frederic Weisbecker <fweisbec@gmail.com>

Will Deacon (4):
  ARM: hw-breakpoint: add mechanism for hooking into prefetch aborts
  ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework
  ARM: hw-breakpoint: add new ptrace requests for hw-breakpoint
    interaction
  ARM: hw-breakpoint: add HAVE_HW_BREAKPOINT to Kconfig

 arch/arm/Kconfig                     |    1 +
 arch/arm/include/asm/hw_breakpoint.h |  118 ++++++
 arch/arm/include/asm/processor.h     |    4 +
 arch/arm/include/asm/ptrace.h        |    2 +
 arch/arm/include/asm/system.h        |    3 +
 arch/arm/kernel/Makefile             |    1 +
 arch/arm/kernel/hw_breakpoint.c      |  711 ++++++++++++++++++++++++++++++++++
 arch/arm/kernel/ptrace.c             |  158 ++++++++
 arch/arm/mm/fault.c                  |   11 +
 9 files changed, 1009 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/hw_breakpoint.h
 create mode 100644 arch/arm/kernel/hw_breakpoint.c

[RFC PATCH 1/4] ARM: hw-breakpoint: add mechanism for hooking into prefetch aborts

[Will Deacon]

On ARM processors with hardware breakpoint and watchpoint support,
triggering these events results in a debug exception. These manifest
as prefetch and data aborts respectively.

arch/arm/mm/fault.c already provides hook_fault_code for hooking
into data aborts dependent on the DFSR. This patch adds a new function,
hook_ifault_code for hooking into prefetch aborts in the same manner.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/system.h |    3 +++
 arch/arm/mm/fault.c           |   11 +++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index 058e7e9..7aa53e3 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -83,6 +83,9 @@ void arm_notify_die(const char *str, struct pt_regs *regs, struct siginfo *info,
 void hook_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
 				       struct pt_regs *),
 		     int sig, const char *name);
+void hook_ifault_code(int nr, int (*fn)(unsigned long, unsigned int,
+				       struct pt_regs *),
+		     int sig, const char *name);
 
 #define xchg(ptr,x) \
 	((__typeof__(*(ptr)))__xchg((unsigned long)(x),(ptr),sizeof(*(ptr))))
diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 9d40c34..9a04584 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -567,6 +567,17 @@ static struct fsr_info ifsr_info[] = {
 	{ do_bad,		SIGBUS,  0,		"unknown 31"			   },
 };
 
+void __init
+hook_ifault_code(int nr, int (*fn)(unsigned long, unsigned int, struct pt_regs *),
+			int sig, const char *name)
+{
+	if (nr >= 0 && nr < ARRAY_SIZE(ifsr_info)) {
+		ifsr_info[nr].fn   = fn;
+		ifsr_info[nr].sig  = sig;
+		ifsr_info[nr].name = name;
+	}
+}
+
 asmlinkage void __exception
 do_PrefetchAbort(unsigned long addr, unsigned int ifsr, struct pt_regs *regs)
 {
-- 
1.6.3.3

[RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Will Deacon]

The hw-breakpoint framework in the kernel requires architecture-specific
support in order to install, remove, validate and manage hardware
breakpoints.

This patch adds preliminary support for this framework to the ARM
architecture, but restricts the number of watchpoints to a single resource
to get around the fact that the Data Fault Address Register is unpredictable
when a watchpoint debug exception is taken.

Additionally, the memory-mapped extended debug interface is unsupported
due to its unreliability in real implementations.

Cc: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/hw_breakpoint.h |  118 ++++++
 arch/arm/kernel/hw_breakpoint.c      |  711 ++++++++++++++++++++++++++++++++++
 2 files changed, 829 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/include/asm/hw_breakpoint.h
 create mode 100644 arch/arm/kernel/hw_breakpoint.c

diff --git a/arch/arm/include/asm/hw_breakpoint.h b/arch/arm/include/asm/hw_breakpoint.h
new file mode 100644
index 0000000..d81cc67
--- /dev/null
+++ b/arch/arm/include/asm/hw_breakpoint.h
@@ -0,0 +1,118 @@
+#ifndef _ARM_HW_BREAKPOINT_H
+#define _ARM_HW_BREAKPOINT_H
+
+#ifdef __KERNEL__
+struct arch_hw_breakpoint {
+	u32	address;
+	u32	trigger;
+	u8	len;
+	u8	type;
+	u8	privilege;
+};
+
+/* Debug architecture numbers. */
+#define ARM_DEBUG_ARCH_V6	1
+#define ARM_DEBUG_ARCH_V6_1	2
+#define ARM_DEBUG_ARCH_V7_ECP14	3
+#define ARM_DEBUG_ARCH_V7_MM	4
+
+/* Breakpoint */
+#define ARM_BREAKPOINT_EXECUTE	0
+
+/* Watchpoints */
+#define ARM_BREAKPOINT_LOAD	1
+#define ARM_BREAKPOINT_STORE	2
+
+/* Privilege Levels */
+#define ARM_BREAKPOINT_SUPER	1
+#define ARM_BREAKPOINT_USER	2
+
+/* Lengths */
+#define ARM_BREAKPOINT_LEN_1	0x1
+#define ARM_BREAKPOINT_LEN_2	0x3
+#define ARM_BREAKPOINT_LEN_4	0xf
+#define ARM_BREAKPOINT_LEN_8	0xff
+
+/* Limits */
+#define ARM_MAX_BRP		16
+#define ARM_MAX_WRP		16
+#define HBP_NUM			(ARM_MAX_BRP + ARM_MAX_WRP)
+
+/* DSCR method of entry bits. */
+#define ARM_DSCR_MOE(x)			((x >> 2) & 0xf)
+#define ARM_ENTRY_BREAKPOINT		0x1
+#define ARM_ENTRY_ASYNC_WATCHPOINT	0x2
+#define ARM_ENTRY_SYNC_WATCHPOINT	0xa
+
+/* DSCR monitor/halting bits. */
+#define ARM_DSCR_HDBGEN		(1 << 14)
+#define ARM_DSCR_MDBGEN		(1 << 15)
+
+/* opcode2 numbers for the co-processor instructions. */
+#define ARM_OP2_BVR		4
+#define ARM_OP2_BCR		5
+#define ARM_OP2_WVR		6
+#define ARM_OP2_WCR		7
+
+/* Base register numbers for the debug registers. */
+#define ARM_BASE_BVR		64
+#define ARM_BASE_BCR		80
+#define ARM_BASE_WVR		96
+#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));\
+} while (0)
+
+#define ARM_DBG_WRITE(M, OP2, VAL) do {\
+	asm volatile("mcr p14, 0, %0, c0," #M ", " #OP2 : : "r" (VAL));\
+} while (0)
+
+/* ptrace interface macros and constants.
+ *
+ * command layout:
+ *
+ * 31    30 29 28   11 10  7 6   3 2  0
+ * +-------+--+-------+-----+-----+----+
+ * |Ver[00]|Op|  SBZ  | Num | Len |Type|
+ * +-------+--+-------+-----+-----+----+
+ *
+ * Op: 1 => insert, 0 => remove.
+ * Num: index into hbp field for thread_struct debug.
+ * Len: HW_BREAKPOINT_LEN_{1,2,4,8}.
+ * Type: HW_BREAKPOINT_{R,W,X}.
+ */
+#define PTRACE_HWBREAK_VER(x)		((x >> 30) & 3)
+#define PTRACE_HWBREAK_OP(x)		((x >> 29) & 1)
+#define PTRACE_HWBREAK_NUM(x)		((x >> 7) & 0xf)
+#define PTRACE_HWBREAK_LEN(x)		((x >> 3) & 0xf)
+#define PTRACE_HWBREAK_TYPE(x)		(x & 7)
+
+#define PTRACE_HWBREAK_OP_REMOVE	0
+#define PTRACE_HWBREAK_OP_INSERT	1
+
+#define PTRACE_HWBREAK_GET_NUM_BRPS	0
+#define PTRACE_HWBREAK_GET_NUM_WRPS	1
+
+struct notifier_block;
+struct perf_event;
+struct pmu;
+struct task_struct;
+
+extern struct pmu perf_ops_bp;
+extern int arch_check_va_in_userspace(unsigned long va, u8 hbp_len);
+extern int arch_validate_hwbkpt_settings(struct perf_event *bp,
+					 struct task_struct *tsk);
+extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
+					   unsigned long val, void *data);
+
+int arch_install_hw_breakpoint(struct perf_event *bp);
+void arch_uninstall_hw_breakpoint(struct perf_event *bp);
+void hw_breakpoint_pmu_read(struct perf_event *bp);
+void hw_breakpoint_pmu_unthrottle(struct perf_event *bp);
+
+unsigned int ptrace_get_num_brps(void);
+unsigned int ptrace_get_num_wrps(void);
+#endif	/* __KERNEL__ */
+#endif	/* _ARM_HW_BREAKPOINT_H */
diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
new file mode 100644
index 0000000..f950b60
--- /dev/null
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -0,0 +1,711 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ *
+ * Copyright (C) 2009,2010 ARM Limited
+ *
+ * Author: Will Deacon <will.deacon@arm.com>
+ */
+
+/*
+ * HW_breakpoint: a unified kernel/user-space hardware breakpoint facility,
+ * using the CPU's debug registers.
+ */
+#include <linux/errno.h>
+#include <linux/perf_event.h>
+#include <linux/hw_breakpoint.h>
+#include <linux/smp.h>
+
+#include <asm/cacheflush.h>
+#include <asm/cputype.h>
+#include <asm/current.h>
+#include <asm/hw_breakpoint.h>
+#include <asm/kdebug.h>
+#include <asm/system.h>
+#include <asm/thread_notify.h>
+#include <asm/traps.h>
+
+/* Breakpoint currently in use for each BRP. */
+static DEFINE_PER_CPU(struct perf_event *, bp_on_reg[ARM_MAX_BRP]);
+
+/* Watchpoint currently in use for each WRP. */
+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_wrps;
+
+/* Debug architecture version. */
+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 UNPREDICTABLE, but
+	 * experience shows that it is set on some implementations].
+	 */
+
+	/*
+	u32 didr, wrps;
+	ARM_DBG_READ(c0, 0, didr);
+	return ((didr >> 28) & 0xf) + 1;
+	*/
+
+	return 1;
+}
+
+unsigned int ptrace_get_num_brps(void)
+{
+	return core_num_brps;
+}
+
+unsigned int ptrace_get_num_wrps(void)
+{
+	return core_num_wrps;
+}
+
+/* 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("hw-breakpoint: 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);
+}
+
+#define READ_WB_REG_CASE(OP2, M, VAL)		\
+	case ((OP2 << 4) + M):			\
+		ARM_DBG_READ(c ## M, OP2, VAL); \
+		break
+
+#define WRITE_WB_REG_CASE(OP2, M, VAL)		\
+	case ((OP2 << 4) + M):			\
+		ARM_DBG_WRITE(c ## M, OP2, VAL);\
+		break
+
+#define GEN_READ_WB_REG_CASES(OP2, VAL)		\
+	READ_WB_REG_CASE(OP2, 0, VAL);		\
+	READ_WB_REG_CASE(OP2, 1, VAL);		\
+	READ_WB_REG_CASE(OP2, 2, VAL);		\
+	READ_WB_REG_CASE(OP2, 3, VAL);		\
+	READ_WB_REG_CASE(OP2, 4, VAL);		\
+	READ_WB_REG_CASE(OP2, 5, VAL);		\
+	READ_WB_REG_CASE(OP2, 6, VAL);		\
+	READ_WB_REG_CASE(OP2, 7, VAL);		\
+	READ_WB_REG_CASE(OP2, 8, VAL);		\
+	READ_WB_REG_CASE(OP2, 9, VAL);		\
+	READ_WB_REG_CASE(OP2, 10, VAL);		\
+	READ_WB_REG_CASE(OP2, 11, VAL);		\
+	READ_WB_REG_CASE(OP2, 12, VAL);		\
+	READ_WB_REG_CASE(OP2, 13, VAL);		\
+	READ_WB_REG_CASE(OP2, 14, VAL);		\
+	READ_WB_REG_CASE(OP2, 15, VAL)
+
+#define GEN_WRITE_WB_REG_CASES(OP2, VAL)	\
+	WRITE_WB_REG_CASE(OP2, 0, VAL);		\
+	WRITE_WB_REG_CASE(OP2, 1, VAL);		\
+	WRITE_WB_REG_CASE(OP2, 2, VAL);		\
+	WRITE_WB_REG_CASE(OP2, 3, VAL);		\
+	WRITE_WB_REG_CASE(OP2, 4, VAL);		\
+	WRITE_WB_REG_CASE(OP2, 5, VAL);		\
+	WRITE_WB_REG_CASE(OP2, 6, VAL);		\
+	WRITE_WB_REG_CASE(OP2, 7, VAL);		\
+	WRITE_WB_REG_CASE(OP2, 8, VAL);		\
+	WRITE_WB_REG_CASE(OP2, 9, VAL);		\
+	WRITE_WB_REG_CASE(OP2, 10, VAL);	\
+	WRITE_WB_REG_CASE(OP2, 11, VAL);	\
+	WRITE_WB_REG_CASE(OP2, 12, VAL);	\
+	WRITE_WB_REG_CASE(OP2, 13, VAL);	\
+	WRITE_WB_REG_CASE(OP2, 14, VAL);	\
+	WRITE_WB_REG_CASE(OP2, 15, VAL)
+
+static u32 read_wb_reg(int n)
+{
+	u32 val = 0;
+
+	switch (n) {
+	GEN_READ_WB_REG_CASES(ARM_OP2_BVR, val);
+	GEN_READ_WB_REG_CASES(ARM_OP2_BCR, val);
+	GEN_READ_WB_REG_CASES(ARM_OP2_WVR, val);
+	GEN_READ_WB_REG_CASES(ARM_OP2_WCR, val);
+	default:
+		pr_warning("hw-breakpoint: attempt to read from unknown "
+				"breakpoint register %d\n", n);
+	}
+
+	return val;
+}
+
+static void write_wb_reg(int n, u32 val)
+{
+	switch (n) {
+	GEN_WRITE_WB_REG_CASES(ARM_OP2_BVR, val);
+	GEN_WRITE_WB_REG_CASES(ARM_OP2_BCR, val);
+	GEN_WRITE_WB_REG_CASES(ARM_OP2_WVR, val);
+	GEN_WRITE_WB_REG_CASES(ARM_OP2_WCR, val);
+	default:
+		pr_warning("hw-breakpoint: attempt to write to unknown "
+				"breakpoint register %d\n", n);
+	}
+}
+
+/*
+ * In order to access the breakpoint/watchpoint control registers,
+ * we must be running in debug monitor mode. Unfortunately, we can
+ * be put into halting debug mode@any time by an external debugger
+ * but there is nothing we can do to prevent that.
+ */
+static int enable_monitor_mode(void)
+{
+	u32 dscr;
+	int ret = 0;
+
+	ARM_DBG_READ(c1, 0, dscr);
+
+	/* Ensure that halting mode is disabled. */
+	if (dscr & ARM_DSCR_HDBGEN) {
+		pr_info("hw-breakpoint: halting debug mode enabled. "
+			"Unable to access hardware resources.\n");
+		ret = -EPERM;
+		goto out;
+	}
+
+	/* Write to the corresponding DSCR. */
+	switch (debug_arch) {
+	case ARM_DEBUG_ARCH_V6:
+	case ARM_DEBUG_ARCH_V6_1:
+		ARM_DBG_WRITE(c1, 0, (dscr | ARM_DSCR_MDBGEN));
+		break;
+	case ARM_DEBUG_ARCH_V7_ECP14:
+		ARM_DBG_WRITE(c2, 2, (dscr | ARM_DSCR_MDBGEN));
+		break;
+	default:
+		ret = -ENODEV;
+		goto out;
+	}
+
+	/* Check that the write made it through. */
+	ARM_DBG_READ(c1, 0, dscr);
+	if (!(dscr & ARM_DSCR_MDBGEN)) {
+		pr_info("hw-breakpoint: failed to enable monitor mode. ");
+		ret = -EPERM;
+	}
+
+out:
+	return ret;
+}
+
+/*
+ * Check if 8-bit byte-address select is available.
+ * This clobbers WRP 0.
+ */
+static u8 get_max_watchpoint_len(void)
+{
+	u32 ctrl;
+	u8 size = 4;
+
+	if (debug_arch < ARM_DEBUG_ARCH_V7_ECP14)
+		goto out;
+
+	if (enable_monitor_mode())
+		goto out;
+
+	ctrl = ARM_BREAKPOINT_LEN_8 << 5;
+	write_wb_reg(ARM_BASE_WVR, 0);
+	write_wb_reg(ARM_BASE_WCR, ctrl);
+	if ((read_wb_reg(ARM_BASE_WCR) & ctrl) == ctrl)
+		size = 8;
+
+out:
+	return size;
+}
+
+/*
+ * Install a perf counter breakpoint.
+ */
+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;
+
+	/* Ensure that we are in monitor mode and halting mode is disabled. */
+	ret = enable_monitor_mode();
+	if (ret)
+		goto out;
+
+	if (info->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;
+	} else {
+		/* Watchpoint */
+		ctrl_base = ARM_BASE_WCR;
+		val_base = ARM_BASE_WVR;
+		slots = __get_cpu_var(wp_on_reg);
+		max_slots = core_num_wrps;
+	}
+
+	for (i = 0; i < max_slots; ++i) {
+		slot = &slots[i];
+
+		if (!*slot) {
+			*slot = bp;
+			break;
+		}
+	}
+
+	if (WARN_ONCE(i == max_slots, "Can't find any breakpoint slot")) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	/* Setup the address register. */
+	write_wb_reg(val_base + i, info->address);
+
+	/* Setup the control register. */
+	write_wb_reg(ctrl_base + i, (info->type << 3) |
+			(info->privilege << 1) | (info->len << 5) | 1);
+
+out:
+	return ret;
+}
+
+void arch_uninstall_hw_breakpoint(struct perf_event *bp)
+{
+	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	struct perf_event **slot, **slots;
+	int i, max_slots, base;
+
+	if (info->type == ARM_BREAKPOINT_EXECUTE) {
+		/* Breakpoint */
+		base = ARM_BASE_BCR;
+		slots = __get_cpu_var(bp_on_reg);
+		max_slots = core_num_brps;
+	} else {
+		/* Watchpoint */
+		base = ARM_BASE_WCR;
+		slots = __get_cpu_var(wp_on_reg);
+		max_slots = core_num_wrps;
+	}
+
+	/* Remove the breakpoint. */
+	for (i = 0; i < max_slots; ++i) {
+		slot = &slots[i];
+
+		if (*slot == bp) {
+			*slot = NULL;
+			break;
+		}
+	}
+
+	if (WARN_ONCE(i == max_slots, "Can't find any breakpoint slot"))
+		return;
+
+	/* Reset the control register. */
+	write_wb_reg(base + i, 0);
+}
+
+static int get_hbp_len(u8 hbp_len)
+{
+	unsigned int len_in_bytes = 0;
+
+	switch (hbp_len) {
+	case ARM_BREAKPOINT_LEN_1:
+		len_in_bytes = 1;
+		break;
+	case ARM_BREAKPOINT_LEN_2:
+		len_in_bytes = 2;
+		break;
+	case ARM_BREAKPOINT_LEN_4:
+		len_in_bytes = 4;
+		break;
+	case ARM_BREAKPOINT_LEN_8:
+		len_in_bytes = 8;
+		break;
+	}
+
+	return len_in_bytes;
+}
+
+/*
+ * Check for virtual address in user space.
+ */
+int arch_check_va_in_userspace(unsigned long va, u8 hbp_len)
+{
+	unsigned int len;
+
+	len = get_hbp_len(hbp_len);
+
+	return (va <= TASK_SIZE - len);
+}
+
+/*
+ * Check for virtual address in kernel space.
+ */
+static int arch_check_va_in_kernelspace(unsigned long va, u8 hbp_len)
+{
+	unsigned int len;
+
+	len = get_hbp_len(hbp_len);
+
+	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
+}
+
+/*
+ * Construct an arch_hw_breakpoint from a perf_event.
+ */
+static int arch_build_bp_info(struct perf_event *bp)
+{
+	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+
+	/* Type */
+	switch (bp->attr.bp_type) {
+	case HW_BREAKPOINT_X:
+		info->type = ARM_BREAKPOINT_EXECUTE;
+		break;
+	case HW_BREAKPOINT_R:
+		info->type = ARM_BREAKPOINT_LOAD;
+		break;
+	case HW_BREAKPOINT_W:
+		info->type = ARM_BREAKPOINT_STORE;
+		break;
+	case HW_BREAKPOINT_R | HW_BREAKPOINT_W:
+		info->type = ARM_BREAKPOINT_LOAD | ARM_BREAKPOINT_STORE;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Len */
+	switch (bp->attr.bp_len) {
+	case HW_BREAKPOINT_LEN_1:
+		info->len = ARM_BREAKPOINT_LEN_1;
+		break;
+	case HW_BREAKPOINT_LEN_2:
+		info->len = ARM_BREAKPOINT_LEN_2;
+		break;
+	case HW_BREAKPOINT_LEN_4:
+		info->len = ARM_BREAKPOINT_LEN_4;
+		break;
+	case HW_BREAKPOINT_LEN_8:
+		info->len = ARM_BREAKPOINT_LEN_8;
+		if ((info->type != ARM_BREAKPOINT_EXECUTE)
+			&& max_watchpoint_len == 8)
+			break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Privilege */
+	if (arch_check_va_in_userspace(info->address, info->len))
+		info->privilege = ARM_BREAKPOINT_USER;
+	else if (arch_check_va_in_kernelspace(info->address, info->len))
+		info->privilege = ARM_BREAKPOINT_SUPER;
+	else
+		return -EINVAL;
+
+	/* Address */
+	info->address = bp->attr.bp_addr;
+
+	if (!info->address)
+		return -EINVAL;
+
+	/*
+	 * Don't allow kernel watchpoints or kernel breakpoints in the
+	 * exception section.
+	 */
+	if (info->privilege == ARM_BREAKPOINT_SUPER &&
+		(info->type != ARM_BREAKPOINT_EXECUTE ||
+		 in_exception_text(info->address)))
+			return -EINVAL;
+
+	return 0;
+}
+
+/*
+ * Validate the arch-specific HW Breakpoint register settings.
+ */
+int arch_validate_hwbkpt_settings(struct perf_event *bp,
+				  struct task_struct *tsk)
+{
+	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	int ret = 0;
+	u32 alignment_mask = 0x3;
+
+	/* Build the arch_hw_breakpoint. */
+	ret = arch_build_bp_info(bp);
+	if (ret)
+		goto out;
+
+	/* Check address alignment. */
+	if (info->len == ARM_BREAKPOINT_LEN_8)
+		alignment_mask = 0x7;
+	if (info->address & alignment_mask) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Check that the virtual address is in the proper range. */
+	if (tsk) {
+		if (!(info->privilege & ARM_BREAKPOINT_USER)) {
+			ret = -EFAULT;
+			goto out;
+		}
+	} else {
+		if (!(info->privilege & ARM_BREAKPOINT_SUPER)) {
+			ret = -EFAULT;
+			goto out;
+		}
+	}
+
+out:
+	return ret;
+}
+
+/*
+ * Release the user breakpoints used by ptrace.
+ */
+void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
+{
+	int i;
+	struct thread_struct *t = &tsk->thread;
+
+	for (i = 0; i < HBP_NUM; i++) {
+		unregister_hw_breakpoint(t->debug.hbp[i]);
+		t->debug.hbp[i] = NULL;
+	}
+}
+
+static int hw_breakpoint_thread_notify(struct notifier_block *self,
+					unsigned long cmd, void *t)
+{
+	if (cmd == THREAD_NOTIFY_FLUSH)
+		flush_ptrace_hw_breakpoint(current);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block hw_breakpoint_thread_notifier_block = {
+	.notifier_call = hw_breakpoint_thread_notify,
+};
+
+static void watchpoint_handler(unsigned long unpredictable,
+				struct pt_regs *regs)
+{
+	int i;
+	struct perf_event **slots = __get_cpu_var(wp_on_reg);
+	struct arch_hw_breakpoint *info;
+
+	for (i = 0; i < core_num_wrps; ++i) {
+		rcu_read_lock();
+
+		if (slots[i] == NULL) {
+			rcu_read_unlock();
+			break;
+		}
+
+		/* TODO: Check if this watchpoint actually fired. */
+
+		/*
+		 * The DFAR is unpredictable. Since we only allow a
+		 * single watchpoint, we can set the trigger to the lowest
+		 * possible faulting address.
+		 */
+		info = counter_arch_bp(slots[i]);
+		info->trigger = info->address;
+		perf_bp_event(slots[i], regs);
+
+		rcu_read_unlock();
+	}
+}
+
+static void breakpoint_handler(unsigned long unpredictable,
+				struct pt_regs *regs)
+{
+	int i;
+	u32 ctrl, val, addr;
+	struct perf_event **slots = __get_cpu_var(bp_on_reg);
+	struct arch_hw_breakpoint *info;
+
+	/* The exception entry code places the amended lr in the PC. */
+	addr = regs->ARM_pc;
+
+	for (i = 0; i < core_num_brps; ++i) {
+		rcu_read_lock();
+
+		if (slots[i] == NULL) {
+			rcu_read_unlock();
+			break;
+		}
+
+		/* Check if the breakpoint value matches. */
+		val = read_wb_reg(ARM_BASE_BVR + i);
+		if (val != (addr & ~0x3))
+			continue;
+
+		/* Possible match, check the byte address select to confirm. */
+		ctrl = read_wb_reg(ARM_BASE_BCR + i);
+		if ((1 << (addr & 0x3)) & ((ctrl >> 5) & 0xf)) {
+			info = counter_arch_bp(slots[i]);
+			info->trigger = addr;
+			perf_bp_event(slots[i], regs);
+		}
+
+		rcu_read_unlock();
+	}
+}
+
+/*
+ * Called from either the Data Abort Handler [watchpoint] or the
+ * Prefetch Abort Handler [breakpoint].
+ */
+static int hw_breakpoint_pending(unsigned long addr, unsigned int fsr,
+					struct pt_regs *regs)
+{
+	int ret = 1; /* Unhandled fault. */
+	u32 dscr;
+
+	/* We only handle watchpoints and hardware breakpoints. */
+	ARM_DBG_READ(c1, 0, dscr);
+
+	/* Perform perf callbacks. */
+	switch (ARM_DSCR_MOE(dscr)) {
+	case ARM_ENTRY_BREAKPOINT:
+		breakpoint_handler(addr, regs);
+		break;
+	case ARM_ENTRY_ASYNC_WATCHPOINT:
+	case ARM_ENTRY_SYNC_WATCHPOINT:
+		watchpoint_handler(addr, regs);
+		break;
+	default:
+		goto out;
+	}
+
+	ret = 0;
+out:
+	return ret;
+}
+
+/*
+ * One-time initialisation.
+ */
+static int __init arch_hw_breakpoint_init(void)
+{
+	int ret = 0, i;
+	u32 dscr;
+
+	debug_arch = get_debug_arch();
+
+	if (debug_arch > ARM_DEBUG_ARCH_V7_ECP14) {
+		pr_info("hw-breakpoint: debug architecture "
+				"0x%x unsupported.\n", debug_arch);
+		ret = -ENODEV;
+		goto out;
+	}
+
+	/* Determine how many BRPs/WRPs are available. */
+	core_num_brps = get_num_brps();
+	core_num_wrps = get_num_wrps();
+
+	pr_info("hw-breakpoint: found %d breakpoint "
+		"and %d watchpoint registers.\n", core_num_brps, core_num_wrps);
+
+	ARM_DBG_READ(c1, 0, dscr);
+	if (dscr & ARM_DSCR_HDBGEN) {
+		pr_warning("hw-breakpoint: halting debug mode enabled. "
+			"Assuming maximum watchpoint size of 4 bytes.");
+	} else {
+		/* Work out the maximum supported watchpoint length. */
+		max_watchpoint_len = get_max_watchpoint_len();
+		pr_info("hw-breakpoint: 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.
+		 */
+		for (i = 0; i < core_num_brps; ++i) {
+			write_wb_reg(ARM_BASE_BCR + i, 0UL);
+			write_wb_reg(ARM_BASE_BVR + i, 0UL);
+		}
+
+		for (i = 0; i < core_num_wrps; ++i) {
+			write_wb_reg(ARM_BASE_WCR + i, 0UL);
+			write_wb_reg(ARM_BASE_WVR + i, 0UL);
+		}
+	}
+
+	/* Register thread flush notifier. */
+	thread_register_notifier(&hw_breakpoint_thread_notifier_block);
+
+	/* Register debug fault handler. */
+	hook_fault_code(2, hw_breakpoint_pending, SIGTRAP, "debug exception");
+	hook_ifault_code(2, hw_breakpoint_pending, SIGTRAP, "debug exception");
+
+out:
+	return ret;
+}
+arch_initcall(arch_hw_breakpoint_init);
+
+void hw_breakpoint_pmu_read(struct perf_event *bp)
+{
+	/* TODO */
+}
+
+void hw_breakpoint_pmu_unthrottle(struct perf_event *bp)
+{
+	/* TODO */
+}
+
+/*
+ * Dummy function to register with die_notifier.
+ */
+int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
+					unsigned long val, void *data)
+{
+	return NOTIFY_DONE;
+}
-- 
1.6.3.3

[RFC PATCH 3/4] ARM: hw-breakpoint: add new ptrace requests for hw-breakpoint interaction

[Will Deacon]

For debuggers to take advantage of the hw-breakpoint framework in the kernel,
it is necessary to expose the API calls via a ptrace interface.

This patch adds support for a debugger to insert, modify and remove hardware
breakpoints using PTRACE_SET_HWBKPT requests. The breakpoints are stored in
the debug_info struct of the running thread. A further request,
PTRACE_GET_HWBKPT can be used to query the number of hardware resources
available.

The ptrace interaction is controlled using a 32-bit word, as described in
arch/arm/include/asm/hw_breakpoint.h.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/include/asm/processor.h |    4 +
 arch/arm/include/asm/ptrace.h    |    2 +
 arch/arm/kernel/ptrace.c         |  158 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/processor.h b/arch/arm/include/asm/processor.h
index 6a89567..ddb0953 100644
--- a/arch/arm/include/asm/processor.h
+++ b/arch/arm/include/asm/processor.h
@@ -19,6 +19,7 @@
 
 #ifdef __KERNEL__
 
+#include <asm/hw_breakpoint.h>
 #include <asm/ptrace.h>
 #include <asm/types.h>
 
@@ -41,6 +42,9 @@ struct debug_entry {
 struct debug_info {
 	int			nsaved;
 	struct debug_entry	bp[2];
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	struct perf_event	*hbp[HBP_NUM];
+#endif
 };
 
 struct thread_struct {
diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
index eec6e89..ea35e4e 100644
--- a/arch/arm/include/asm/ptrace.h
+++ b/arch/arm/include/asm/ptrace.h
@@ -29,6 +29,8 @@
 #define PTRACE_SETCRUNCHREGS	26
 #define PTRACE_GETVFPREGS	27
 #define PTRACE_SETVFPREGS	28
+#define PTRACE_GET_HWBKPT	29
+#define PTRACE_SET_HWBKPT	30
 
 /*
  * PSR bits
diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
index a2ea385..1e02bc5 100644
--- a/arch/arm/kernel/ptrace.c
+++ b/arch/arm/kernel/ptrace.c
@@ -19,6 +19,8 @@
 #include <linux/init.h>
 #include <linux/signal.h>
 #include <linux/uaccess.h>
+#include <linux/perf_event.h>
+#include <linux/hw_breakpoint.h>
 
 #include <asm/pgtable.h>
 #include <asm/system.h>
@@ -707,6 +709,152 @@ static int ptrace_setvfpregs(struct task_struct *tsk, void __user *data)
 }
 #endif
 
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+/*
+ * Handle hitting a HW-breakpoint.
+ */
+static void ptrace_hwbreak_triggered(struct perf_event *bp, int unused,
+					struct perf_sample_data *data,
+					struct pt_regs *regs)
+{
+	struct arch_hw_breakpoint *bkpt = counter_arch_bp(bp);
+	siginfo_t info;
+
+	info.si_signo	= SIGTRAP;
+	info.si_errno	= bp->attr.bp_type;
+	info.si_code	= TRAP_HWBKPT;
+	info.si_addr	= (void __user *)(bkpt->trigger);
+
+	force_sig_info(SIGTRAP, &info, current);
+}
+
+static int ptrace_insert_hwbkpt(struct task_struct *tsk, unsigned long addr,
+				u8 n, u8 len, u8 type)
+{
+	struct perf_event *bp;
+	struct thread_struct *t = &tsk->thread;
+	struct perf_event_attr attr;
+	int ret = 0;
+
+	/* Watchpoints live in the upper indices. */
+	if ((type & HW_BREAKPOINT_X) == 0)
+		n += ARM_MAX_BRP;
+
+	if (n >= HBP_NUM) {
+		ret = -ENOSPC;
+		goto out;
+	}
+
+	if (!t->debug.hbp[n]) {
+		/* New breakpoint. */
+		hw_breakpoint_init(&attr);
+		attr.bp_addr = addr;
+		attr.bp_len = len;
+		attr.bp_type = type;
+
+		bp = register_user_hw_breakpoint(&attr,
+				ptrace_hwbreak_triggered, tsk);
+		if (IS_ERR(bp))
+			ret = PTR_ERR(bp);
+		else
+			t->debug.hbp[n] = bp;
+	} else {
+		/* Existing breakpoint. */
+		bp = t->debug.hbp[n];
+		attr = bp->attr;
+		attr.bp_addr = addr;
+		attr.bp_len = len;
+		attr.disabled = 0;
+		ret = modify_user_hw_breakpoint(bp, &attr);
+	}
+
+out:
+	return ret;
+}
+
+static int ptrace_remove_hwbkpt(struct task_struct *tsk, u8 n, u8 type)
+{
+	struct perf_event *bp;
+	struct thread_struct *t = &tsk->thread;
+	struct perf_event_attr attr;
+	int ret = -EINVAL;
+
+	if ((type & HW_BREAKPOINT_X) == 0)
+		n += ARM_MAX_BRP;
+
+	if (n < HBP_NUM) {
+		bp = t->debug.hbp[n];
+		if (bp == NULL)
+			goto out;
+		attr = bp->attr;
+		attr.disabled = 1;
+		ret = modify_user_hw_breakpoint(bp, &attr);
+	} else {
+		ret = -ENOSPC;
+	}
+
+out:
+	return ret;
+}
+
+static int ptrace_get_hwbkpt(struct task_struct *tsk, long addr,
+				unsigned long __user *data)
+{
+	unsigned long num;
+	int ret = 0;
+
+	switch (addr) {
+	case PTRACE_HWBREAK_GET_NUM_BRPS:
+		num = ptrace_get_num_brps();
+		break;
+	case PTRACE_HWBREAK_GET_NUM_WRPS:
+		num = ptrace_get_num_wrps();
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	if (!ret && put_user(num, data))
+		ret = -EFAULT;
+
+	return ret;
+}
+
+static int ptrace_set_hwbkpt(struct task_struct *tsk, long addr,
+				unsigned long data)
+{
+	int ret;
+	u8 num, len, type;
+
+	/* Check version field. */
+	if (PTRACE_HWBREAK_VER(data) != 0) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Extract data fields. */
+	num = PTRACE_HWBREAK_NUM(data);
+	len = PTRACE_HWBREAK_LEN(data);
+	type = PTRACE_HWBREAK_TYPE(data);
+
+	/* Insert or remove the breakpoint. */
+	switch (PTRACE_HWBREAK_OP(data)) {
+	case PTRACE_HWBREAK_OP_REMOVE:
+		ret = ptrace_remove_hwbkpt(tsk, num, type);
+		break;
+	case PTRACE_HWBREAK_OP_INSERT:
+		ret = ptrace_insert_hwbkpt(tsk, addr, num, len, type);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+out:
+	return ret;
+}
+
+#endif
+
 long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 {
 	int ret;
@@ -839,6 +987,16 @@ long arch_ptrace(struct task_struct *child, long request, long addr, long data)
 			break;
 #endif
 
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+		case PTRACE_GET_HWBKPT:
+			ret = ptrace_get_hwbkpt(child, addr,
+						(unsigned long __user *)data);
+			break;
+		case PTRACE_SET_HWBKPT:
+			ret = ptrace_set_hwbkpt(child, addr, data);
+			break;
+#endif
+
 		default:
 			ret = ptrace_request(child, request, addr, data);
 			break;
-- 
1.6.3.3

[RFC PATCH 4/4] ARM: hw-breakpoint: add HAVE_HW_BREAKPOINT to Kconfig

[Will Deacon]

If we're targetting a v6 or v7 core and have at least software perf events
available, then automatically add support for hardware breakpoints.

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

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 5e723bc..c6cb103 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -23,6 +23,7 @@ config ARM
 	select HAVE_PERF_EVENTS
 	select PERF_USE_VMALLOC
 	select GENERIC_ATOMIC64 if (!CPU_32v6K)
+	select HAVE_HW_BREAKPOINT if (PERF_EVENTS && (CPU_V6 || CPU_V7))
 	help
 	  The ARM series is a line of low-power-consumption RISC chip designs
 	  licensed by ARM Ltd and targeted at embedded applications and
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index c76e6d2..a06c8cc 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -38,6 +38,7 @@ obj-$(CONFIG_ARM_THUMBEE)	+= thumbee.o
 obj-$(CONFIG_KGDB)		+= kgdb.o
 obj-$(CONFIG_ARM_UNWIND)	+= unwind.o
 obj-$(CONFIG_HAVE_TCM)		+= tcm.o
+obj-$(CONFIG_HAVE_HW_BREAKPOINT)	+= hw_breakpoint.o
 
 obj-$(CONFIG_CRUNCH)		+= crunch.o crunch-bits.o
 AFLAGS_crunch-bits.o		:= -Wa,-mcpu=ep9312
-- 
1.6.3.3

[RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Frederic Weisbecker]

(WARNING: I've browsed the ARMv7 breakpoints implementation
but I may have an erratic/incomplete understanding, then parts
of this review might make little sense)


2010/3/10 Will Deacon <will.deacon@arm.com>:
> The hw-breakpoint framework in the kernel requires architecture-specific
> support in order to install, remove, validate and manage hardware
> breakpoints.
>
> This patch adds preliminary support for this framework to the ARM
> architecture, but restricts the number of watchpoints to a single resource
> to get around the fact that the Data Fault Address Register is unpredictable
> when a watchpoint debug exception is taken.


What do you mean here by unpredictable? It would be a pity to limit the
resources to one register.



> +}
> +
> +/* 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.



Doh! That must explain the problem with DFAR...
That's really not convenient :-(



> +        *
> +        * 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 UNPREDICTABLE, but
> +        * experience shows that it is set on some implementations].


Ok...



> +/*
> + * Install a perf counter breakpoint.
> + */
> +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;
> +
> +       /* Ensure that we are in monitor mode and halting mode is disabled. */
> +       ret = enable_monitor_mode();
> +       if (ret)
> +               goto out;
> +
> +       if (info->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;
> +       } else {
> +               /* Watchpoint */
> +               ctrl_base = ARM_BASE_WCR;
> +               val_base = ARM_BASE_WVR;
> +               slots = __get_cpu_var(wp_on_reg);
> +               max_slots = core_num_wrps;
> +       }
> +
> +       for (i = 0; i < max_slots; ++i) {
> +               slot = &slots[i];
> +
> +               if (!*slot) {
> +                       *slot = bp;
> +                       break;
> +               }
> +       }
> +
> +       if (WARN_ONCE(i == max_slots, "Can't find any breakpoint slot")) {
> +               ret = -EBUSY;
> +               goto out;
> +       }
> +
> +       /* Setup the address register. */
> +       write_wb_reg(val_base + i, info->address);
> +
> +       /* Setup the control register. */
> +       write_wb_reg(ctrl_base + i, (info->type << 3) |
> +                       (info->privilege << 1) | (info->len << 5) | 1);



Ok, alternatively, why not having the control register in the arch
breakpoint structure:

u32   __reserved : 19,
        len             :  7,
        type           :  2,
        privilege      :  2,
        enabled      :  1;

It will avoid you to play with bitwise operations that may scale into dirty
and error prone as you may support more features from the control register
(link, mask, etc...).

So in the future if you want to support more things, you can just split out
the __reserved field into the features it has, depending on the ARM versions.

Ah and it will make ptrace support easier: the user writes into the val/ctrl
fields directly as if they were the true registers, then you can just validate
the fields you want without bitwise ops.



> +/*
> + * Validate the arch-specific HW Breakpoint register settings.
> + */
> +int arch_validate_hwbkpt_settings(struct perf_event *bp,
> +                                 struct task_struct *tsk)
> +{
> +       struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> +       int ret = 0;
> +       u32 alignment_mask = 0x3;
> +
> +       /* Build the arch_hw_breakpoint. */
> +       ret = arch_build_bp_info(bp);
> +       if (ret)
> +               goto out;
> +
> +       /* Check address alignment. */
> +       if (info->len == ARM_BREAKPOINT_LEN_8)
> +               alignment_mask = 0x7;
> +       if (info->address & alignment_mask) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       /* Check that the virtual address is in the proper range. */
> +       if (tsk) {
> +               if (!(info->privilege & ARM_BREAKPOINT_USER)) {
> +                       ret = -EFAULT;
> +                       goto out;
> +               }
> +       } else {
> +               if (!(info->privilege & ARM_BREAKPOINT_SUPER)) {
> +                       ret = -EFAULT;
> +                       goto out;
> +               }
> +       }


You seem to make a wrong assumption here.
This is not because the breakpoint is task-bound that we don't
want it to trigger on the kernel. We may want to trigger breakpoints
on tasklist_lock accesses from a given task.

The only case for which we don't want it to trigger on the kernel
is for ptrace breakpoints.

I guess I should remove this tsk parameter as it makes the things
only confusing. We should simply create ptrace breakpoints with
bp->attr.exclude_kernel set to 1.

Because when bp->attr.exclude_kernel is set to 1, then it truly makes sense
to think about user and supervisor privileges, just to avoid to filter the
event in the software level. Note we do this filtering on software level,
but it going to be less costly if done from hardware.

Even if you don't support ptrace right now, we can define a kernel exluded
breakpoint from perf syscall.

(Note for myself: set exclude_kernel to ptrace breakpoints in x86)


> +/*
> + * Release the user breakpoints used by ptrace.
> + */
> +void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
> +{
> +       int i;
> +       struct thread_struct *t = &tsk->thread;
> +
> +       for (i = 0; i < HBP_NUM; i++) {
> +               unregister_hw_breakpoint(t->debug.hbp[i]);
> +               t->debug.hbp[i] = NULL;
> +       }
> +}


Do you actually support ptrace breakpoints?



> +
> +void hw_breakpoint_pmu_unthrottle(struct perf_event *bp)
> +{
> +       /* TODO */
> +}


We don't need this callback anymore, it has been removed because
of ptrace corner cases...


Thanks!

[RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Will Deacon]

Hi Frederic,

Many thanks for taking a look at the these patches, I appreciate the
feedback.

On Tue, 2010-04-13 at 14:32 +0100, Frederic Weisbecker wrote:
> (WARNING: I've browsed the ARMv7 breakpoints implementation
> but I may have an erratic/incomplete understanding, then parts
> of this review might make little sense)
> 
It looks like you've understood it correctly. Actually, I have
a disclaimer of my own; I'm using a new mail client in the hope
that LKML won't drop my messages. Anyway...
> 
> 2010/3/10 Will Deacon <will.deacon@arm.com>:
> > The hw-breakpoint framework in the kernel requires architecture-specific
> > support in order to install, remove, validate and manage hardware
> > breakpoints.
> >
> > This patch adds preliminary support for this framework to the ARM
> > architecture, but restricts the number of watchpoints to a single resource
> > to get around the fact that the Data Fault Address Register is unpredictable
> > when a watchpoint debug exception is taken.
> 
> What do you mean here by unpredictable? It would be a pity to limit the
> resources to one register.
> 
By unpredictable I mean that the value in the DFAR is not defined to
correspond to the causative address in any way. This isn't the case
for a standard data abort, but it is in the case of a watchpoint
exception.
> 
> > +}
> > +
> > +/* 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.
> 
> Doh! That must explain the problem with DFAR...
> That's really not convenient :-(
> 
I know, it makes life a lot harder for us. The most annoying thing is that
I'm yet to find a real implementation that *doesn't* set the DFAR to what
we want - we just can't rely on that!
> 
<snip>

> > +       /* Setup the address register. */
> > +       write_wb_reg(val_base + i, info->address);
> > +
> > +       /* Setup the control register. */
> > +       write_wb_reg(ctrl_base + i, (info->type << 3) |
> > +                       (info->privilege << 1) | (info->len << 5) | 1);

> Ok, alternatively, why not having the control register in the arch
> breakpoint structure:
> 
> u32   __reserved : 19,
>         len             :  7,
>         type           :  2,
>         privilege      :  2,
>         enabled      :  1;
> 
> It will avoid you to play with bitwise operations that may scale into dirty
> and error prone as you may support more features from the control register
> (link, mask, etc...).
> 
Nice, I like that a lot!

> Ah and it will make ptrace support easier: the user writes into the val/ctrl
> fields directly as if they were the true registers, then you can just validate
> the fields you want without bitwise ops.
> 
I'm unsure about this. As mainline stands, ARM has no ptrace support for the
hardware breakpoint registers. This means that we could expose the
hardware resources in an idealised fashion via ptrace so that if the
interface varies between CPUs, userspace doesn't need to care. I had a
crack at this with another patch in the series here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011169.html

> > +       /* Check that the virtual address is in the proper range. */
> > +       if (tsk) {
> > +               if (!(info->privilege & ARM_BREAKPOINT_USER)) {
> > +                       ret = -EFAULT;
> > +                       goto out;
> > +               }
> > +       } else {
> > +               if (!(info->privilege & ARM_BREAKPOINT_SUPER)) {
> > +                       ret = -EFAULT;
> > +                       goto out;
> > +               }
> > +       }
> 
> 
> You seem to make a wrong assumption here.
> This is not because the breakpoint is task-bound that we don't
> want it to trigger on the kernel. We may want to trigger breakpoints
> on tasklist_lock accesses from a given task.

> The only case for which we don't want it to trigger on the kernel
> is for ptrace breakpoints.
> 
Surely we can't have breakpoints triggering on *any* part of the
breakpoint handling code path? Otherwise we'll just get stuck. That's
why I disallow breakpoints in the exception section.

> I guess I should remove this tsk parameter as it makes the things
> only confusing. We should simply create ptrace breakpoints with
> bp->attr.exclude_kernel set to 1.

Sounds good to me. That also means that it's one less thing for the arch
to worry about!
> 
> > +/*
> > + * Release the user breakpoints used by ptrace.
> > + */
> > +void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
> > +{
> > +       int i;
> > +       struct thread_struct *t = &tsk->thread;
> > +
> > +       for (i = 0; i < HBP_NUM; i++) {
> > +               unregister_hw_breakpoint(t->debug.hbp[i]);
> > +               t->debug.hbp[i] = NULL;
> > +       }
> > +}
> 
> 
> Do you actually support ptrace breakpoints?
> 
Yep. See the link mentioned above.
> 
> > +
> > +void hw_breakpoint_pmu_unthrottle(struct perf_event *bp)
> > +{
> > +       /* TODO */
> > +}
> 
> 
> We don't need this callback anymore, it has been removed because
> of ptrace corner cases...
> 
Ok, I'll remove it.

I'll take a look at the new constraints and allocation patches you
posted this morning and then adjust these patches accordingly.

Thanks,

Will

[RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Frederic Weisbecker]

On Tue, Apr 13, 2010 at 04:52:45PM +0100, Will Deacon wrote:
> On Tue, 2010-04-13 at 14:32 +0100, Frederic Weisbecker wrote:
> > (WARNING: I've browsed the ARMv7 breakpoints implementation
> > but I may have an erratic/incomplete understanding, then parts
> > of this review might make little sense)
> > 
> It looks like you've understood it correctly. Actually, I have
> a disclaimer of my own; I'm using a new mail client in the hope
> that LKML won't drop my messages. Anyway...


Looks like that worked as well :)



> > 2010/3/10 Will Deacon <will.deacon@arm.com>:
> > > The hw-breakpoint framework in the kernel requires architecture-specific
> > > support in order to install, remove, validate and manage hardware
> > > breakpoints.
> > >
> > > This patch adds preliminary support for this framework to the ARM
> > > architecture, but restricts the number of watchpoints to a single resource
> > > to get around the fact that the Data Fault Address Register is unpredictable
> > > when a watchpoint debug exception is taken.
> > 
> > What do you mean here by unpredictable? It would be a pity to limit the
> > resources to one register.
> > 
> By unpredictable I mean that the value in the DFAR is not defined to
> correspond to the causative address in any way. This isn't the case
> for a standard data abort, but it is in the case of a watchpoint
> exception.



Ok.



> > 
> > > +}
> > > +
> > > +/* 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.
> > 
> > Doh! That must explain the problem with DFAR...
> > That's really not convenient :-(
> > 
> I know, it makes life a lot harder for us. The most annoying thing is that
> I'm yet to find a real implementation that *doesn't* set the DFAR to what
> we want - we just can't rely on that!


You mean that sometimes DFAR doesn't have the true ip origin of the
exception?
May be you can check the trapped regs to find the instruction pointer
that caused the exception?


 
> > Ah and it will make ptrace support easier: the user writes into the val/ctrl
> > fields directly as if they were the true registers, then you can just validate
> > the fields you want without bitwise ops.
> > 
> I'm unsure about this. As mainline stands, ARM has no ptrace support for the
> hardware breakpoint registers. This means that we could expose the
> hardware resources in an idealised fashion via ptrace so that if the
> interface varies between CPUs, userspace doesn't need to care. I had a
> crack at this with another patch in the series here:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011169.html



Ah, indeed if you need to abstract out various ARM versions, that's quite sensitive.
I'm going to look at this thread too.



 
> > You seem to make a wrong assumption here.
> > This is not because the breakpoint is task-bound that we don't
> > want it to trigger on the kernel. We may want to trigger breakpoints
> > on tasklist_lock accesses from a given task.
> 
> > The only case for which we don't want it to trigger on the kernel
> > is for ptrace breakpoints.
> > 
> Surely we can't have breakpoints triggering on *any* part of the
> breakpoint handling code path? Otherwise we'll just get stuck. That's
> why I disallow breakpoints in the exception section.



Ah indeed you really need to protect against recursion.

But I'm unclear about the difference between Supervisor and System.
May be System means the common kernel ring? And you enter into
Supervisor when an exception triggers?

Do these exceptions also concern page faults and not only debug
exceptions?

 
> > I guess I should remove this tsk parameter as it makes the things
> > only confusing. We should simply create ptrace breakpoints with
> > bp->attr.exclude_kernel set to 1.
> 
> Sounds good to me. That also means that it's one less thing for the arch
> to worry about!


Yeah!



> I'll take a look at the new constraints and allocation patches you
> posted this morning and then adjust these patches accordingly.


Thanks.

[RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Will Deacon]

Hello Frederic,

On Tue, 2010-04-13 at 21:40 +0100, Frederic Weisbecker wrote:
> > > > +/* 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.
> > >
> > > Doh! That must explain the problem with DFAR...
> > > That's really not convenient :-(
> > >
> > I know, it makes life a lot harder for us. The most annoying thing is that
> > I'm yet to find a real implementation that *doesn't* set the DFAR to what
> > we want - we just can't rely on that!
> 
> You mean that sometimes DFAR doesn't have the true ip origin of the
> exception?
> May be you can check the trapped regs to find the instruction pointer
> that caused the exception?

There are two related issues here:

1.) A watchpoint can be synchronous or asynchronous, you can determine which when
    you take the exception. If the watchpoint is synchronous, then yes, we can look
    at the trapped regs to find the causative instruction. Then we would need to
    disassemble the instruction and emulate the address generation part [which may
    also need the trapped regs] in order to calculate the faulting data address.
    If the watchpoint exception is asynchronous, we can read the WFAR register to
    find the address of the causative instruction. Unfortunately, we could still
    be screwed in this case because we won't be able to emulate the address
    calculation for a register-relative load/store. Thankfully, v7 cores only
    generate synchronous watchpoints [apart from some very early revisions which
    you won't see in the wild] but v6 cores are the opposite; they only generate
    asynchronous watchpoint exceptions.

2.) For a standard data abort, the DFAR is updated to contain the data address which
    caused the fault. Ideally this would also be the case for watchpoint exceptions
    because then we wouldn't have to disassemble anything [and as I mentioned
    previously, I'm yet to find a real implementation that doesn't set the DFAR on
    taking a watchpoint, but it's not something that the architecture guarantees].
    
> > > Ah and it will make ptrace support easier: the user writes into the val/ctrl
> > > fields directly as if they were the true registers, then you can just validate
> > > the fields you want without bitwise ops.
> > >
> > I'm unsure about this. As mainline stands, ARM has no ptrace support for the
> > hardware breakpoint registers. This means that we could expose the
> > hardware resources in an idealised fashion via ptrace so that if the
> > interface varies between CPUs, userspace doesn't need to care. I had a
> > crack at this with another patch in the series here:
> >
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011169.html
> 
> 
> 
> Ah, indeed if you need to abstract out various ARM versions, that's quite sensitive.
> I'm going to look at this thread too.

It's not so much about necessity, but I think it would be nice if userspace
didn't have to care so much about the breakpoint/watchpoint debug hardware on the
target. Since we don't have any backwards compatibility issues [because ptrace in
mainline has never supported hardware breakpoints on ARM] we can define a stable
API regardless of the hardware.

> > > You seem to make a wrong assumption here.
> > > This is not because the breakpoint is task-bound that we don't
> > > want it to trigger on the kernel. We may want to trigger breakpoints
> > > on tasklist_lock accesses from a given task.
> >
> > > The only case for which we don't want it to trigger on the kernel
> > > is for ptrace breakpoints.
> > >
> > Surely we can't have breakpoints triggering on *any* part of the
> > breakpoint handling code path? Otherwise we'll just get stuck. That's
> > why I disallow breakpoints in the exception section.
> 
> 
> 
> Ah indeed you really need to protect against recursion.

I'm unsure about how to provide full protection though. You could still
set a breakpoint on the breakpoint handling code that exists outside of the
exception text. Do other archs suffer from this problem? I'm tempted
just to disallow Kernel breakpoints for now, but that would be a shame.

> But I'm unclear about the difference between Supervisor and System.
> May be System means the common kernel ring? And you enter into
> Supervisor when an exception triggers?

Sorry, I think my ARM_BREAKPOINT_SUPER #define is confusing. I'll rename it
to ARM_BREAKPOINT_PRIV because the hardware only distinguishes between
privileged and unprivileged accesses, regardless of the mode.

> Do these exceptions also concern page faults and not only debug
> exceptions?

I think so.

One thing that has hit me is the behaviour of the framework when a breakpoint
is hit. If the breakpoint is synchronous, after the exception has been handled
[and perf_bp_event has been called], execution will restart at the faulting
instruction. It is down to the client program [traditionally a debugger] to
remove the breakpoint, single-step and re-insert the breakpoint. Obviously the
perf tool doesn't do this, so if you try to perf stat a breakpoint event you
will livelock [potentially another reason to disable Kernel breakpoints?]

Cheers,

Will

[RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Frederic Weisbecker]

On Wed, Apr 14, 2010 at 11:27:01AM +0100, Will Deacon wrote:
> Hello Frederic,
> 
> On Tue, 2010-04-13 at 21:40 +0100, Frederic Weisbecker wrote:
> > > > > +/* 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.
> > > >
> > > > Doh! That must explain the problem with DFAR...
> > > > That's really not convenient :-(
> > > >
> > > I know, it makes life a lot harder for us. The most annoying thing is that
> > > I'm yet to find a real implementation that *doesn't* set the DFAR to what
> > > we want - we just can't rely on that!
> > 
> > You mean that sometimes DFAR doesn't have the true ip origin of the
> > exception?
> > May be you can check the trapped regs to find the instruction pointer
> > that caused the exception?
> 
> There are two related issues here:
> 
> 1.) A watchpoint can be synchronous or asynchronous, you can determine which when
>     you take the exception. If the watchpoint is synchronous, then yes, we can look
>     at the trapped regs to find the causative instruction. Then we would need to
>     disassemble the instruction and emulate the address generation part [which may
>     also need the trapped regs] in order to calculate the faulting data address.
>     If the watchpoint exception is asynchronous, we can read the WFAR register to
>     find the address of the causative instruction. Unfortunately, we could still
>     be screwed in this case because we won't be able to emulate the address
>     calculation for a register-relative load/store. Thankfully, v7 cores only
>     generate synchronous watchpoints [apart from some very early revisions which
>     you won't see in the wild] but v6 cores are the opposite; they only generate
>     asynchronous watchpoint exceptions.
> 
> 2.) For a standard data abort, the DFAR is updated to contain the data address which
>     caused the fault. Ideally this would also be the case for watchpoint exceptions
>     because then we wouldn't have to disassemble anything [and as I mentioned
>     previously, I'm yet to find a real implementation that doesn't set the DFAR on
>     taking a watchpoint, but it's not something that the architecture guarantees].



It's a cruel architecture...


     
> > > > Ah and it will make ptrace support easier: the user writes into the val/ctrl
> > > > fields directly as if they were the true registers, then you can just validate
> > > > the fields you want without bitwise ops.
> > > >
> > > I'm unsure about this. As mainline stands, ARM has no ptrace support for the
> > > hardware breakpoint registers. This means that we could expose the
> > > hardware resources in an idealised fashion via ptrace so that if the
> > > interface varies between CPUs, userspace doesn't need to care. I had a
> > > crack at this with another patch in the series here:
> > >
> > > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011169.html
> > 
> > 
> > 
> > Ah, indeed if you need to abstract out various ARM versions, that's quite sensitive.
> > I'm going to look at this thread too.
> 
> It's not so much about necessity, but I think it would be nice if userspace
> didn't have to care so much about the breakpoint/watchpoint debug hardware on the
> target. Since we don't have any backwards compatibility issues [because ptrace in
> mainline has never supported hardware breakpoints on ARM] we can define a stable
> API regardless of the hardware.


Yeah, looking at how this is implemented across versions, it seems
that they all share the same registers, but early versions have
unfeatured reserved bytes in control registers that are filled
with new features over time in subsequent versions.

It looks like the things are then forward compatible but not
backward.

Isn't that enough to provide a ptrace interface? I mean you can't do
much magic here. Having something that works everywhere would consist
in implementing only the minimal set of features only.



> > > > You seem to make a wrong assumption here.
> > > > This is not because the breakpoint is task-bound that we don't
> > > > want it to trigger on the kernel. We may want to trigger breakpoints
> > > > on tasklist_lock accesses from a given task.
> > >
> > > > The only case for which we don't want it to trigger on the kernel
> > > > is for ptrace breakpoints.
> > > >
> > > Surely we can't have breakpoints triggering on *any* part of the
> > > breakpoint handling code path? Otherwise we'll just get stuck. That's
> > > why I disallow breakpoints in the exception section.
> > 
> > 
> > 
> > Ah indeed you really need to protect against recursion.
> 
> I'm unsure about how to provide full protection though. You could still
> set a breakpoint on the breakpoint handling code that exists outside of the
> exception text. Do other archs suffer from this problem? I'm tempted
> just to disallow Kernel breakpoints for now, but that would be a shame.


In x86 we just write the control register to disable the breakpoints
in the exception handler. (I suspect we don't do it early enough though).

You can do the same in ARM.

Currently, there is no true security flow because if a breakpoint
is task bound, x86 refuse breakpoints in kernel addresses.
But as I noticed before, this is a wrong behaviour as we also
want to be able to trace kernel var accesses while in a given task
context, except for ptrace.

I should just add a CAP_SYS_ADMIN() && !ptrace check for kernel
addresses.



> > But I'm unclear about the difference between Supervisor and System.
> > May be System means the common kernel ring? And you enter into
> > Supervisor when an exception triggers?
> 
> Sorry, I think my ARM_BREAKPOINT_SUPER #define is confusing. I'll rename it
> to ARM_BREAKPOINT_PRIV because the hardware only distinguishes between
> privileged and unprivileged accesses, regardless of the mode.


Ok.



> > Do these exceptions also concern page faults and not only debug
> > exceptions?
> 
> I think so.
> 
> One thing that has hit me is the behaviour of the framework when a breakpoint
> is hit. If the breakpoint is synchronous, after the exception has been handled
> [and perf_bp_event has been called], execution will restart at the faulting
> instruction. It is down to the client program [traditionally a debugger] to
> remove the breakpoint, single-step and re-insert the breakpoint. Obviously the
> perf tool doesn't do this, so if you try to perf stat a breakpoint event you
> will livelock [potentially another reason to disable Kernel breakpoints?]


If I understand you correctly,

sync = breakpoint triggers before the instruction is executed,
       trap return to the faulting instruction
async = breakpoint triggers after the instruction is executed

Then yeah, the current breakpoint framework only supports async
ones.

If an arch has no choice but using sync breakpoints, it needs
to handle that. It seems PowerPc has a similar problem, may be
have a look at Prasad's patches.

If there is something that can be handled from the generic
layer to help solving this problem, I'll try something.

[RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Will Deacon]

Hi Frederic,

Thanks for addressing some of these problems.

> > There are two related issues here:
> >
> > 1.) A watchpoint can be synchronous or asynchronous, you can determine which when
> >     you take the exception. If the watchpoint is synchronous, then yes, we can look
> >     at the trapped regs to find the causative instruction. Then we would need to
> >     disassemble the instruction and emulate the address generation part [which may
> >     also need the trapped regs] in order to calculate the faulting data address.
> >     If the watchpoint exception is asynchronous, we can read the WFAR register to
> >     find the address of the causative instruction. Unfortunately, we could still
> >     be screwed in this case because we won't be able to emulate the address
> >     calculation for a register-relative load/store. Thankfully, v7 cores only
> >     generate synchronous watchpoints [apart from some very early revisions which
> >     you won't see in the wild] but v6 cores are the opposite; they only generate
> >     asynchronous watchpoint exceptions.
> >
> > 2.) For a standard data abort, the DFAR is updated to contain the data address which
> >     caused the fault. Ideally this would also be the case for watchpoint exceptions
> >     because then we wouldn't have to disassemble anything [and as I mentioned
> >     previously, I'm yet to find a real implementation that doesn't set the DFAR on
> >     taking a watchpoint, but it's not something that the architecture guarantees].
> 
> 
> 
> It's a cruel architecture...
> 

It grows on you :)

> > > > > Ah and it will make ptrace support easier: the user writes into the val/ctrl
> > > > > fields directly as if they were the true registers, then you can just validate
> > > > > the fields you want without bitwise ops.
> > > > >
> > > > I'm unsure about this. As mainline stands, ARM has no ptrace support for the
> > > > hardware breakpoint registers. This means that we could expose the
> > > > hardware resources in an idealised fashion via ptrace so that if the
> > > > interface varies between CPUs, userspace doesn't need to care. I had a
> > > > crack at this with another patch in the series here:
> > > >
> > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-March/011169.html
> > >
> > >
> > >
> > > Ah, indeed if you need to abstract out various ARM versions, that's quite sensitive.
> > > I'm going to look at this thread too.
> >
> > It's not so much about necessity, but I think it would be nice if userspace
> > didn't have to care so much about the breakpoint/watchpoint debug hardware on the
> > target. Since we don't have any backwards compatibility issues [because ptrace in
> > mainline has never supported hardware breakpoints on ARM] we can define a stable
> > API regardless of the hardware.
> 
> 
> Yeah, looking at how this is implemented across versions, it seems
> that they all share the same registers, but early versions have
> unfeatured reserved bytes in control registers that are filled
> with new features over time in subsequent versions.
> 
> It looks like the things are then forward compatible but not
> backward.
> 
> Isn't that enough to provide a ptrace interface? I mean you can't do
> much magic here. Having something that works everywhere would consist
> in implementing only the minimal set of features only.

I suppose the ptrace interface will involve bit-packing at some point anyway,
so mirroring the hardware configuration might not be as bad as I initially thought.
The hardware has some slightly more esoteric features [breakpoint linking, context
matching etc] that won't be supported in software for now so I'll ignore those bits.

> > I'm unsure about how to provide full protection though. You could still
> > set a breakpoint on the breakpoint handling code that exists outside of the
> > exception text. Do other archs suffer from this problem? I'm tempted
> > just to disallow Kernel breakpoints for now, but that would be a shame.
> 
> 
> In x86 we just write the control register to disable the breakpoints
> in the exception handler. (I suspect we don't do it early enough though).
> 
> You can do the same in ARM.

Ok.
 
> > One thing that has hit me is the behaviour of the framework when a breakpoint
> > is hit. If the breakpoint is synchronous, after the exception has been handled
> > [and perf_bp_event has been called], execution will restart at the faulting
> > instruction. It is down to the client program [traditionally a debugger] to
> > remove the breakpoint, single-step and re-insert the breakpoint. Obviously the
> > perf tool doesn't do this, so if you try to perf stat a breakpoint event you
> > will livelock [potentially another reason to disable Kernel breakpoints?]
> 
> 
> If I understand you correctly,
> 
> sync = breakpoint triggers before the instruction is executed,
>        trap return to the faulting instruction
> async = breakpoint triggers after the instruction is executed

That's right.
 
> Then yeah, the current breakpoint framework only supports async
> ones.

Oh no!

> If an arch has no choice but using sync breakpoints, it needs
> to handle that. It seems PowerPc has a similar problem, may be
> have a look at Prasad's patches.

Could you give me a pointer to these patches please [I can't see them on LKML]?
I *think* PowerPC is lucky enough to have hardware single step, so handling
synchronous breakpoints isn't *too* difficult. In ARM we have to work out the
address of the following instruction, which is quite tricky because it could be a
branch instruction [from a selection of instruction sets] and also be predicated.
GDB can handle this, but the Kernel doesn't have the capability to do so [there's
some code in arch/arm/ptrace.c but it's not feature complete].

> If there is something that can be handled from the generic
> layer to help solving this problem, I'll try something.

Unfortunately this is very much an arch problem. As a half-way house, I might
implement only the ptrace part of hw-breakpoint for ARM. That way we can
force the client to handle stepping over the breakpoint and we also won't have
to worry about recursive breakpoints in the Kernel.

Cheers,

Will

[RFC PATCH 2/4] ARM: hw-breakpoint: add ARM backend for the hw-breakpoint framework

[Frederic Weisbecker]

On Fri, Apr 16, 2010 at 03:17:59PM +0100, Will Deacon wrote:
> Hi Frederic,
> > It's a cruel architecture...
> > 
> 
> It grows on you :)


:-)))


 
> > Yeah, looking at how this is implemented across versions, it seems
> > that they all share the same registers, but early versions have
> > unfeatured reserved bytes in control registers that are filled
> > with new features over time in subsequent versions.
> > 
> > It looks like the things are then forward compatible but not
> > backward.
> > 
> > Isn't that enough to provide a ptrace interface? I mean you can't do
> > much magic here. Having something that works everywhere would consist
> > in implementing only the minimal set of features only.
> 
> I suppose the ptrace interface will involve bit-packing at some point anyway,
> so mirroring the hardware configuration might not be as bad as I initially thought.
> The hardware has some slightly more esoteric features [breakpoint linking, context
> matching etc] that won't be supported in software for now so I'll ignore those bits.



In fact, I guess watchpoint to breakpoint linking is something that userspace
can use. But the context ID linking....well I haven't understood this part
in ARM documentation :)


 
> > > I'm unsure about how to provide full protection though. You could still
> > > set a breakpoint on the breakpoint handling code that exists outside of the
> > > exception text. Do other archs suffer from this problem? I'm tempted
> > > just to disallow Kernel breakpoints for now, but that would be a shame.
> > 
> > 
> > In x86 we just write the control register to disable the breakpoints
> > in the exception handler. (I suspect we don't do it early enough though).
> > 
> > You can do the same in ARM.
> 
> Ok.



Speaking about this made me wonder about the current state in x86.
For now it's safe because task bound breakpoint only allow userspace
addresses.
But this is not right because we want task bound to be able to trigger
on kernel. Once we'll allow this, we need a bit of policy.

Because yeah we can protect against recursion, but always partially.
The only solution to really protect would be disabling the breakpoint
as the very first operation in the trap handler.

But that's ugly and may be costly as well.

So what I'm going to do is to simply check attr.exclude_kernel on
breakpoint creation and look if the address is in the kernel space.

So that if you want to crash your machine why lurking at the very
tight window of trap handler possible recursion, you need to be
root :)

Because when a perf event registers and doesn't have exclude_kernel,
you need to be admin.

Fortunately we don't have this security hole with the current rules,
but I'll try to make this check more generic (will require archs that
support breakpoints to implement arch_check_va_in_kernelspace()).


> > If an arch has no choice but using sync breakpoints, it needs
> > to handle that. It seems PowerPc has a similar problem, may be
> > have a look at Prasad's patches.
> 
> Could you give me a pointer to these patches please [I can't see them on LKML]?
> I *think* PowerPC is lucky enough to have hardware single step, so handling
> synchronous breakpoints isn't *too* difficult. In ARM we have to work out the
> address of the following instruction, which is quite tricky because it could be a
> branch instruction [from a selection of instruction sets] and also be predicated.
> GDB can handle this, but the Kernel doesn't have the capability to do so [there's
> some code in arch/arm/ptrace.c but it's not feature complete].


I don't remember exactly how this is handled. I remember a story about
a one-shot breakpoint, ie: that triggers only once.

Anyway, I have yet to review the latest version:

http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-April/081714.html
http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-April/081715.html
http://lists.ozlabs.org/pipermail/linuxppc-dev/2010-April/081716.html



> 
> > If there is something that can be handled from the generic
> > layer to help solving this problem, I'll try something.
> 
> Unfortunately this is very much an arch problem. As a half-way house, I might
> implement only the ptrace part of hw-breakpoint for ARM. That way we can
> force the client to handle stepping over the breakpoint and we also won't have
> to worry about recursive breakpoints in the Kernel.


:-s

Yeah, we'll see what we can do.