[PATCH RFC v2 0/3] riscv: support for hardware breakpoints/watchpoints

[PATCH RFC v2 0/3] riscv: support for hardware breakpoints/watchpoints

[Sergey Matyukevich]

RISC-V Debug specification includes Sdtrig ISA extension. This extension
describes Trigger Module. Triggers can cause a breakpoint exception,
entry into Debug Mode, or a trace action without having to execute a
special instruction. For native debugging triggers can be used to
implement hardware breakpoints and watchpoints.

Software support for triggers consists of the following
major components:
 - U-mode: gdb support for hw breakpoints/watchpoints
 - S-mode: hardware breakpoints framework in Linux kernel
 - M-mode: SBI firmware code to handle triggers

SBI Debug Trigger extension proposal has been posted by Anup Patel
to lists.riscv.org tech-debug mailing list, see:
https://lists.riscv.org/g/tech-debug/topic/92375492

This patch provides initial Linux support for RISC-V hardware breakpoints
and watchpoints based on the proposed SBI Debug Trigger extension. The
accompanying OpenSBI and GDB changes has also been posted for review:

- https://patchwork.ozlabs.org/project/opensbi/patch/20221203213929.206429-3-geomatsi@gmail.com/
- https://patchwork.sourceware.org/project/gdb/patch/20221130182605.1905317-1-yuly.tarasov@syntacore.com/

Current revision has the following limitations:
- two trigger types are supported: mcontrol, mcontrol6
- no support for chained triggers
- no support for virtualization
- only build test for RV32

The functionality has been tested on QEMU together with the mentioned
opensbi and gdb patches, including both target gdb and remote debug
using gdbserver. Hardware breakpoints work just fine on upstream QEMU.
However this is not the case for watchpoints since there is no way to
figure out which watchpoint triggered. IIUC there are two possible
options for doing this: using 'hit' bit in tdata1 or reading faulting
virtual address from STVAL. QEMU implements neither of them. Current
implementation opts for STVAL. The following experimental QEMU patch
is required to make hw-watchpoints work:

: diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
: index 278d163803..8858be7411 100644
: --- a/target/riscv/cpu_helper.c
: +++ b/target/riscv/cpu_helper.c
: @@ -1639,6 +1639,10 @@ void riscv_cpu_do_interrupt(CPUState *cs)
:          case RISCV_EXCP_VIRT_INSTRUCTION_FAULT:
:              tval = env->bins;
:              break;
: +        case RISCV_EXCP_BREAKPOINT:
: +            tval = env->badaddr;
: +            env->badaddr = 0x0;
: +            break;
:          default:
:              break;
:          }
: diff --git a/target/riscv/debug.c b/target/riscv/debug.c
: index 26ea764407..b4d1d566ab 100644
: --- a/target/riscv/debug.c
: +++ b/target/riscv/debug.c
: @@ -560,6 +560,7 @@ void riscv_cpu_debug_excp_handler(CPUState *cs)
:
:      if (cs->watchpoint_hit) {
:          if (cs->watchpoint_hit->flags & BP_CPU) {
: +            env->badaddr = cs->watchpoint_hit->hitaddr;
:              cs->watchpoint_hit = NULL;
:              do_trigger_action(env, DBG_ACTION_BP);


Changes v1 -> v2:
- switched to per-cpu buffers to exchange data with SBI firmware
- added support for type 2 (mcounter) triggers
- added ptrace interface to expose hw-breakpoints to debuggers


Sergey Matyukevich (3):
  riscv: add support for hardware breakpoints/watchpoints
  riscv: ptrace: expose hardware breakpoints to debuggers
  riscv: hw-breakpoints: add more trigger controls

 arch/riscv/Kconfig                     |   2 +
 arch/riscv/include/asm/hw_breakpoint.h | 172 ++++++++
 arch/riscv/include/asm/kdebug.h        |   3 +-
 arch/riscv/include/asm/processor.h     |   5 +
 arch/riscv/include/asm/sbi.h           |  24 ++
 arch/riscv/include/uapi/asm/ptrace.h   |   9 +
 arch/riscv/kernel/Makefile             |   1 +
 arch/riscv/kernel/hw_breakpoint.c      | 540 +++++++++++++++++++++++++
 arch/riscv/kernel/process.c            |   3 +
 arch/riscv/kernel/ptrace.c             | 188 +++++++++
 arch/riscv/kernel/traps.c              |   5 +
 11 files changed, 951 insertions(+), 1 deletion(-)
 create mode 100644 arch/riscv/include/asm/hw_breakpoint.h
 create mode 100644 arch/riscv/kernel/hw_breakpoint.c

-- 
2.38.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH RFC v2 1/3] riscv: add support for hardware breakpoints/watchpoints

[Sergey Matyukevich]

From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>

RISC-V backend for hw-breakpoint framework is built on top of SBI Debug
Trigger extension. Architecture specific hooks are implemented as kernel
wrappers around ecalls to SBI functions. This patch implements only a
minimal set of hooks required to support user-space debug via ptrace.

Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
---
 arch/riscv/Kconfig                     |   2 +
 arch/riscv/include/asm/hw_breakpoint.h | 157 +++++++++
 arch/riscv/include/asm/kdebug.h        |   3 +-
 arch/riscv/include/asm/processor.h     |   5 +
 arch/riscv/include/asm/sbi.h           |  24 ++
 arch/riscv/kernel/Makefile             |   1 +
 arch/riscv/kernel/hw_breakpoint.c      | 432 +++++++++++++++++++++++++
 arch/riscv/kernel/process.c            |   1 +
 arch/riscv/kernel/traps.c              |   5 +
 9 files changed, 629 insertions(+), 1 deletion(-)
 create mode 100644 arch/riscv/include/asm/hw_breakpoint.h
 create mode 100644 arch/riscv/kernel/hw_breakpoint.c

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 593cf09264d8..fe7f63928235 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -95,10 +95,12 @@ config RISCV
 	select HAVE_FUNCTION_ERROR_INJECTION
 	select HAVE_GCC_PLUGINS
 	select HAVE_GENERIC_VDSO if MMU && 64BIT
+	select HAVE_HW_BREAKPOINT if PERF_EVENTS
 	select HAVE_IRQ_TIME_ACCOUNTING
 	select HAVE_KPROBES if !XIP_KERNEL
 	select HAVE_KPROBES_ON_FTRACE if !XIP_KERNEL
 	select HAVE_KRETPROBES if !XIP_KERNEL
+	select HAVE_MIXED_BREAKPOINTS_REGS
 	select HAVE_MOVE_PMD
 	select HAVE_MOVE_PUD
 	select HAVE_PCI
diff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h
new file mode 100644
index 000000000000..5bb3b55cd464
--- /dev/null
+++ b/arch/riscv/include/asm/hw_breakpoint.h
@@ -0,0 +1,157 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __RISCV_HW_BREAKPOINT_H
+#define __RISCV_HW_BREAKPOINT_H
+
+struct task_struct;
+
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+
+#include <uapi/linux/hw_breakpoint.h>
+
+#if __riscv_xlen == 64
+#define cpu_to_lle cpu_to_le64
+#define lle_to_cpu le64_to_cpu
+#elif __riscv_xlen == 32
+#define cpu_to_lle cpu_to_le32
+#define lle_to_cpu le32_to_cpu
+#else
+#error "Unexpected __riscv_xlen"
+#endif
+
+enum {
+	RISCV_DBTR_BREAKPOINT	= 0,
+	RISCV_DBTR_WATCHPOINT	= 1,
+};
+
+enum {
+	RISCV_DBTR_TRIG_NONE = 0,
+	RISCV_DBTR_TRIG_LEGACY,
+	RISCV_DBTR_TRIG_MCONTROL,
+	RISCV_DBTR_TRIG_ICOUNT,
+	RISCV_DBTR_TRIG_ITRIGGER,
+	RISCV_DBTR_TRIG_ETRIGGER,
+	RISCV_DBTR_TRIG_MCONTROL6,
+};
+
+union riscv_dbtr_tdata1 {
+	unsigned long value;
+	struct {
+#if __riscv_xlen == 64
+		unsigned long data:59;
+#elif __riscv_xlen == 32
+		unsigned long data:27;
+#else
+#error "Unexpected __riscv_xlen"
+#endif
+		unsigned long dmode:1;
+		unsigned long type:4;
+	};
+};
+
+union riscv_dbtr_tdata1_mcontrol {
+	unsigned long value;
+	struct {
+		unsigned long load:1;
+		unsigned long store:1;
+		unsigned long execute:1;
+		unsigned long u:1;
+		unsigned long s:1;
+		unsigned long _res2:1;
+		unsigned long m:1;
+		unsigned long match:4;
+		unsigned long chain:1;
+		unsigned long action:4;
+		unsigned long sizelo:2;
+		unsigned long timing:1;
+		unsigned long select:1;
+		unsigned long hit:1;
+#if __riscv_xlen >= 64
+		unsigned long sizehi:2;
+		unsigned long _res1:30;
+#endif
+		unsigned long maskmax:6;
+		unsigned long dmode:1;
+		unsigned long type:4;
+	};
+};
+
+union riscv_dbtr_tdata1_mcontrol6 {
+	unsigned long value;
+	struct {
+		unsigned long load:1;
+		unsigned long store:1;
+		unsigned long execute:1;
+		unsigned long u:1;
+		unsigned long s:1;
+		unsigned long _res2:1;
+		unsigned long m:1;
+		unsigned long match:4;
+		unsigned long chain:1;
+		unsigned long action:4;
+		unsigned long size:4;
+		unsigned long timing:1;
+		unsigned long select:1;
+		unsigned long hit:1;
+		unsigned long vu:1;
+		unsigned long vs:1;
+#if __riscv_xlen == 64
+		unsigned long _res1:34;
+#elif __riscv_xlen == 32
+		unsigned long _res1:2;
+#else
+#error "Unexpected __riscv_xlen"
+#endif
+		unsigned long dmode:1;
+		unsigned long type:4;
+	};
+};
+
+struct arch_hw_breakpoint {
+	unsigned long address;
+	unsigned long len;
+	unsigned int type;
+
+	union {
+		unsigned long value;
+		union riscv_dbtr_tdata1_mcontrol mcontrol;
+		union riscv_dbtr_tdata1_mcontrol6 mcontrol6;
+	} trig_data1;
+	unsigned long trig_data2;
+	unsigned long trig_data3;
+};
+
+/* Max supported HW breakpoints */
+#define HBP_NUM_MAX 32
+
+struct perf_event_attr;
+struct notifier_block;
+struct perf_event;
+struct pt_regs;
+
+int hw_breakpoint_slots(int type);
+int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+			     const struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw);
+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 clear_ptrace_hw_breakpoint(struct task_struct *tsk);
+
+#else
+
+int hw_breakpoint_slots(int type)
+{
+	return 0;
+}
+
+static inline void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
+{
+}
+
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
+#endif /* __RISCV_HW_BREAKPOINT_H */
diff --git a/arch/riscv/include/asm/kdebug.h b/arch/riscv/include/asm/kdebug.h
index 85ac00411f6e..53e989781aa1 100644
--- a/arch/riscv/include/asm/kdebug.h
+++ b/arch/riscv/include/asm/kdebug.h
@@ -6,7 +6,8 @@
 enum die_val {
 	DIE_UNUSED,
 	DIE_TRAP,
-	DIE_OOPS
+	DIE_OOPS,
+	DIE_DEBUG
 };
 
 #endif
diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index 94a0590c6971..10c87fba2548 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -11,6 +11,7 @@
 #include <vdso/processor.h>
 
 #include <asm/ptrace.h>
+#include <asm/hw_breakpoint.h>
 
 /*
  * This decides where the kernel will search for a free chunk of vm
@@ -29,6 +30,7 @@
 #ifndef __ASSEMBLY__
 
 struct task_struct;
+struct perf_event;
 struct pt_regs;
 
 /* CPU-specific state of a task */
@@ -39,6 +41,9 @@ struct thread_struct {
 	unsigned long s[12];	/* s[0]: frame pointer */
 	struct __riscv_d_ext_state fstate;
 	unsigned long bad_cause;
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	struct perf_event *ptrace_bps[HBP_NUM_MAX];
+#endif
 };
 
 /* Whitelist the fstate from the task_struct for hardened usercopy */
diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 2a0ef738695e..ef41d60a5ed3 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -31,6 +31,9 @@ enum sbi_ext_id {
 	SBI_EXT_SRST = 0x53525354,
 	SBI_EXT_PMU = 0x504D55,
 
+	/* Experimental: Debug Trigger Extension */
+	SBI_EXT_DBTR = 0x44425452,
+
 	/* Experimentals extensions must lie within this range */
 	SBI_EXT_EXPERIMENTAL_START = 0x08000000,
 	SBI_EXT_EXPERIMENTAL_END = 0x08FFFFFF,
@@ -113,6 +116,27 @@ enum sbi_srst_reset_reason {
 	SBI_SRST_RESET_REASON_SYS_FAILURE,
 };
 
+enum sbi_ext_dbtr_fid {
+	SBI_EXT_DBTR_NUM_TRIGGERS = 0,
+	SBI_EXT_DBTR_TRIGGER_READ,
+	SBI_EXT_DBTR_TRIGGER_INSTALL,
+	SBI_EXT_DBTR_TRIGGER_UNINSTALL,
+	SBI_EXT_DBTR_TRIGGER_ENABLE,
+	SBI_EXT_DBTR_TRIGGER_UPDATE,
+	SBI_EXT_DBTR_TRIGGER_DISABLE,
+};
+
+struct sbi_dbtr_data_msg {
+	unsigned long tstate;
+	unsigned long tdata1;
+	unsigned long tdata2;
+	unsigned long tdata3;
+};
+
+struct sbi_dbtr_id_msg {
+	unsigned long idx;
+};
+
 enum sbi_ext_pmu_fid {
 	SBI_EXT_PMU_NUM_COUNTERS = 0,
 	SBI_EXT_PMU_COUNTER_GET_INFO,
diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index db6e4b1294ba..116697d0ca1d 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -72,6 +72,7 @@ obj-$(CONFIG_TRACE_IRQFLAGS)	+= trace_irq.o
 
 obj-$(CONFIG_PERF_EVENTS)	+= perf_callchain.o
 obj-$(CONFIG_HAVE_PERF_REGS)	+= perf_regs.o
+obj-$(CONFIG_HAVE_HW_BREAKPOINT)	+= hw_breakpoint.o
 obj-$(CONFIG_RISCV_SBI)		+= sbi.o
 ifeq ($(CONFIG_RISCV_SBI), y)
 obj-$(CONFIG_SMP) += cpu_ops_sbi.o
diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
new file mode 100644
index 000000000000..8eddf512cd03
--- /dev/null
+++ b/arch/riscv/kernel/hw_breakpoint.c
@@ -0,0 +1,432 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/hw_breakpoint.h>
+#include <linux/perf_event.h>
+#include <linux/percpu.h>
+#include <linux/kdebug.h>
+
+#include <asm/sbi.h>
+
+/* bps/wps currently set on each debug trigger for each cpu */
+static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM_MAX]);
+
+static struct sbi_dbtr_data_msg __percpu *sbi_xmit;
+static struct sbi_dbtr_id_msg __percpu *sbi_recv;
+
+/* number of debug triggers on this cpu . */
+static int dbtr_total_num __ro_after_init;
+static int dbtr_type __ro_after_init;
+static int dbtr_init __ro_after_init;
+
+void arch_hw_breakpoint_init_sbi(void)
+{
+	union riscv_dbtr_tdata1 tdata1;
+	struct sbiret ret;
+
+	if (sbi_probe_extension(SBI_EXT_DBTR) <= 0) {
+		pr_info("%s: SBI_EXT_DBTR is not supported\n", __func__);
+		dbtr_total_num = 0;
+		goto done;
+	}
+
+	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
+			0, 0, 0, 0, 0, 0);
+	if (ret.error) {
+		pr_warn("%s: failed to detect triggers\n", __func__);
+		dbtr_total_num = 0;
+		goto done;
+	}
+
+	tdata1.value = 0;
+	tdata1.type = RISCV_DBTR_TRIG_MCONTROL6;
+
+	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
+			tdata1.value, 0, 0, 0, 0, 0);
+	if (ret.error) {
+		pr_warn("%s: failed to detect mcontrol6 triggers\n", __func__);
+	} else if (!ret.value) {
+		pr_warn("%s: type 6 triggers not available\n", __func__);
+	} else {
+		dbtr_total_num = ret.value;
+		dbtr_type = RISCV_DBTR_TRIG_MCONTROL6;
+		goto done;
+	}
+
+	/* fallback to type 2 triggers if type 6 is not available */
+
+	tdata1.value = 0;
+	tdata1.type = RISCV_DBTR_TRIG_MCONTROL;
+
+	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
+			tdata1.value, 0, 0, 0, 0, 0);
+	if (ret.error) {
+		pr_warn("%s: failed to detect mcontrol triggers\n", __func__);
+	} else if (!ret.value) {
+		pr_warn("%s: type 2 triggers not available\n", __func__);
+	} else {
+		dbtr_total_num = ret.value;
+		dbtr_type = RISCV_DBTR_TRIG_MCONTROL;
+		goto done;
+	}
+
+done:
+	dbtr_init = 1;
+}
+
+int hw_breakpoint_slots(int type)
+{
+	/*
+	 * We can be called early, so don't rely on
+	 * static variables being initialised.
+	 */
+
+	if (!dbtr_init)
+		arch_hw_breakpoint_init_sbi();
+
+	return dbtr_total_num;
+}
+
+int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw)
+{
+	unsigned int len;
+	unsigned long va;
+
+	va = hw->address;
+	len = hw->len;
+
+	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
+}
+
+int arch_build_type2_trigger(const struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw)
+{
+	/* type */
+	switch (attr->bp_type) {
+	case HW_BREAKPOINT_X:
+		hw->type = RISCV_DBTR_BREAKPOINT;
+		hw->trig_data1.mcontrol.execute = 1;
+		break;
+	case HW_BREAKPOINT_R:
+		hw->type = RISCV_DBTR_WATCHPOINT;
+		hw->trig_data1.mcontrol.load = 1;
+		break;
+	case HW_BREAKPOINT_W:
+		hw->type = RISCV_DBTR_WATCHPOINT;
+		hw->trig_data1.mcontrol.store = 1;
+		break;
+	case HW_BREAKPOINT_RW:
+		hw->type = RISCV_DBTR_WATCHPOINT;
+		hw->trig_data1.mcontrol.store = 1;
+		hw->trig_data1.mcontrol.load = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* length */
+	switch (attr->bp_len) {
+	case HW_BREAKPOINT_LEN_1:
+		hw->len = 1;
+		hw->trig_data1.mcontrol.sizelo = 1;
+		break;
+	case HW_BREAKPOINT_LEN_2:
+		hw->len = 2;
+		hw->trig_data1.mcontrol.sizelo = 2;
+		break;
+	case HW_BREAKPOINT_LEN_4:
+		hw->len = 4;
+		hw->trig_data1.mcontrol.sizelo = 3;
+		break;
+#if __riscv_xlen >= 64
+	case HW_BREAKPOINT_LEN_8:
+		hw->len = 8;
+		hw->trig_data1.mcontrol.sizelo = 1;
+		hw->trig_data1.mcontrol.sizehi = 1;
+		break;
+#endif
+	default:
+		return -EINVAL;
+	}
+
+	hw->trig_data1.mcontrol.type = RISCV_DBTR_TRIG_MCONTROL;
+	hw->trig_data1.mcontrol.dmode = 0;
+	hw->trig_data1.mcontrol.timing = 0;
+	hw->trig_data1.mcontrol.select = 0;
+	hw->trig_data1.mcontrol.action = 0;
+	hw->trig_data1.mcontrol.chain = 0;
+	hw->trig_data1.mcontrol.match = 0;
+
+	hw->trig_data1.mcontrol.m = 0;
+	hw->trig_data1.mcontrol.s = 1;
+	hw->trig_data1.mcontrol.u = 1;
+
+	return 0;
+}
+
+int arch_build_type6_trigger(const struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw)
+{
+	/* type */
+	switch (attr->bp_type) {
+	case HW_BREAKPOINT_X:
+		hw->type = RISCV_DBTR_BREAKPOINT;
+		hw->trig_data1.mcontrol6.execute = 1;
+		break;
+	case HW_BREAKPOINT_R:
+		hw->type = RISCV_DBTR_WATCHPOINT;
+		hw->trig_data1.mcontrol6.load = 1;
+		break;
+	case HW_BREAKPOINT_W:
+		hw->type = RISCV_DBTR_WATCHPOINT;
+		hw->trig_data1.mcontrol6.store = 1;
+		break;
+	case HW_BREAKPOINT_RW:
+		hw->type = RISCV_DBTR_WATCHPOINT;
+		hw->trig_data1.mcontrol6.store = 1;
+		hw->trig_data1.mcontrol6.load = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* length */
+	switch (attr->bp_len) {
+	case HW_BREAKPOINT_LEN_1:
+		hw->len = 1;
+		hw->trig_data1.mcontrol6.size = 1;
+		break;
+	case HW_BREAKPOINT_LEN_2:
+		hw->len = 2;
+		hw->trig_data1.mcontrol6.size = 2;
+		break;
+	case HW_BREAKPOINT_LEN_4:
+		hw->len = 4;
+		hw->trig_data1.mcontrol6.size = 3;
+		break;
+	case HW_BREAKPOINT_LEN_8:
+		hw->len = 8;
+		hw->trig_data1.mcontrol6.size = 5;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	hw->trig_data1.mcontrol6.type = RISCV_DBTR_TRIG_MCONTROL6;
+	hw->trig_data1.mcontrol6.dmode = 0;
+	hw->trig_data1.mcontrol6.timing = 0;
+	hw->trig_data1.mcontrol6.select = 0;
+	hw->trig_data1.mcontrol6.action = 0;
+	hw->trig_data1.mcontrol6.chain = 0;
+	hw->trig_data1.mcontrol6.match = 0;
+
+	hw->trig_data1.mcontrol6.m = 0;
+	hw->trig_data1.mcontrol6.s = 1;
+	hw->trig_data1.mcontrol6.u = 1;
+	hw->trig_data1.mcontrol6.vs = 0;
+	hw->trig_data1.mcontrol6.vu = 0;
+
+	return 0;
+}
+
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+			     const struct perf_event_attr *attr,
+			     struct arch_hw_breakpoint *hw)
+{
+	int ret;
+
+	/* address */
+	hw->address = attr->bp_addr;
+	hw->trig_data2 = attr->bp_addr;
+	hw->trig_data3 = 0x0;
+
+	switch (dbtr_type) {
+	case RISCV_DBTR_TRIG_MCONTROL:
+		ret = arch_build_type2_trigger(attr, hw);
+		break;
+	case RISCV_DBTR_TRIG_MCONTROL6:
+		ret = arch_build_type6_trigger(attr, hw);
+		break;
+	default:
+		pr_warn("unsupported trigger type\n");
+		ret = -EOPNOTSUPP;
+		break;
+	}
+
+	return ret;
+}
+
+/*
+ * Handle debug exception notifications.
+ */
+static int hw_breakpoint_handler(struct die_args *args)
+{
+	int ret = NOTIFY_DONE;
+	struct arch_hw_breakpoint *info;
+	struct perf_event *bp;
+	int i;
+
+	for (i = 0; i < dbtr_total_num; ++i) {
+		bp = this_cpu_read(bp_per_reg[i]);
+		if (!bp)
+			continue;
+
+		info = counter_arch_bp(bp);
+		switch (info->type) {
+		case RISCV_DBTR_BREAKPOINT:
+			if (info->address == args->regs->epc) {
+				pr_debug("%s: breakpoint fired: pc[0x%lx]\n",
+					 __func__, args->regs->epc);
+				perf_bp_event(bp, args->regs);
+				ret = NOTIFY_STOP;
+			}
+
+			break;
+		case RISCV_DBTR_WATCHPOINT:
+			if (info->address == csr_read(CSR_STVAL)) {
+				pr_debug("%s: watchpoint fired: addr[0x%lx]\n",
+					 __func__, info->address);
+				perf_bp_event(bp, args->regs);
+				ret = NOTIFY_STOP;
+			}
+
+			break;
+		default:
+			pr_warn("%s: unexpected breakpoint type: %u\n",
+				__func__, info->type);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
+				    unsigned long val, void *data)
+{
+	if (val != DIE_DEBUG)
+		return NOTIFY_DONE;
+
+	return hw_breakpoint_handler(data);
+}
+
+/* atomic: counter->ctx->lock is held */
+int arch_install_hw_breakpoint(struct perf_event *bp)
+{
+	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	struct sbi_dbtr_data_msg *xmit = this_cpu_ptr(sbi_xmit);
+	struct sbi_dbtr_id_msg *recv = this_cpu_ptr(sbi_recv);
+	struct perf_event **slot;
+	unsigned long idx;
+	struct sbiret ret;
+
+	xmit->tdata1 = cpu_to_lle(info->trig_data1.value);
+	xmit->tdata2 = cpu_to_lle(info->trig_data2);
+	xmit->tdata3 = cpu_to_lle(info->trig_data3);
+
+	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_INSTALL,
+			1, __pa(xmit) >> 4, __pa(recv) >> 4,
+			0, 0, 0);
+	if (ret.error) {
+		pr_warn("%s: failed to install trigger\n", __func__);
+		return -EIO;
+	}
+
+	idx = lle_to_cpu(recv->idx);
+
+	if (idx >= dbtr_total_num) {
+		pr_warn("%s: invalid trigger index %lu\n", __func__, idx);
+		return -EINVAL;
+	}
+
+	slot = this_cpu_ptr(&bp_per_reg[idx]);
+	if (*slot) {
+		pr_warn("%s: slot %lu is in use\n", __func__, idx);
+		return -EBUSY;
+	}
+
+	*slot = bp;
+
+	return 0;
+}
+
+/* atomic: counter->ctx->lock is held */
+void arch_uninstall_hw_breakpoint(struct perf_event *bp)
+{
+	struct sbiret ret;
+	int i;
+
+	for (i = 0; i < dbtr_total_num; i++) {
+		struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
+
+		if (*slot == bp) {
+			*slot = NULL;
+			break;
+		}
+	}
+
+	if (i == dbtr_total_num) {
+		pr_warn("%s: unknown breakpoint\n", __func__);
+		return;
+	}
+
+	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_UNINSTALL,
+			i, 1, 0, 0, 0, 0);
+	if (ret.error)
+		pr_warn("%s: failed to uninstall trigger %d\n", __func__, i);
+}
+
+void hw_breakpoint_pmu_read(struct perf_event *bp)
+{
+}
+
+/*
+ * Set ptrace breakpoint pointers to zero for this task.
+ * This is required in order to prevent child processes from unregistering
+ * breakpoints held by their parent.
+ */
+void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
+{
+	memset(tsk->thread.ptrace_bps, 0, sizeof(tsk->thread.ptrace_bps));
+}
+
+/*
+ * Unregister breakpoints from this task and reset the pointers in
+ * the thread_struct.
+ */
+void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
+{
+	int i;
+	struct thread_struct *t = &tsk->thread;
+
+	for (i = 0; i < dbtr_total_num; i++) {
+		unregister_hw_breakpoint(t->ptrace_bps[i]);
+		t->ptrace_bps[i] = NULL;
+	}
+}
+
+static int __init arch_hw_breakpoint_init(void)
+{
+	sbi_xmit = __alloc_percpu(sizeof(*sbi_xmit), SZ_16);
+	if (!sbi_xmit) {
+		pr_warn("failed to allocate SBI xmit message buffer\n");
+		return -ENOMEM;
+	}
+
+	sbi_recv = __alloc_percpu(sizeof(*sbi_recv), SZ_16);
+	if (!sbi_recv) {
+		pr_warn("failed to allocate SBI recv message buffer\n");
+		return -ENOMEM;
+	}
+
+	if (!dbtr_init)
+		arch_hw_breakpoint_init_sbi();
+
+	if (dbtr_total_num)
+		pr_info("%s: total number of type %d triggers: %u\n",
+			__func__, dbtr_type, dbtr_total_num);
+	else
+		pr_info("%s: no hardware triggers available\n", __func__);
+
+	return 0;
+}
+arch_initcall(arch_hw_breakpoint_init);
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index 8955f2432c2d..cd99bececed8 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -187,5 +187,6 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
 		p->thread.ra = (unsigned long)ret_from_fork;
 	}
 	p->thread.sp = (unsigned long)childregs; /* kernel sp */
+	clear_ptrace_hw_breakpoint(p);
 	return 0;
 }
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 7abd8e4c4df6..34c93d2f159e 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -174,6 +174,11 @@ asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs)
 
 	if (uprobe_breakpoint_handler(regs))
 		return;
+#endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	if (notify_die(DIE_DEBUG, "EBREAK", regs, 0, regs->cause, SIGTRAP)
+						       == NOTIFY_STOP)
+		return;
 #endif
 	current->thread.bad_cause = regs->cause;
 
-- 
2.38.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH RFC v2 1/3] riscv: add support for hardware breakpoints/watchpoints

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1327 bytes --]

On Sun, Dec 04, 2022 at 12:55:33AM +0300, Sergey Matyukevich wrote:
> From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
> 
> RISC-V backend for hw-breakpoint framework is built on top of SBI Debug
> Trigger extension. Architecture specific hooks are implemented as kernel
> wrappers around ecalls to SBI functions. This patch implements only a
> minimal set of hooks required to support user-space debug via ptrace.
> 
> Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 2a0ef738695e..ef41d60a5ed3 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -31,6 +31,9 @@ enum sbi_ext_id {
>  	SBI_EXT_SRST = 0x53525354,
>  	SBI_EXT_PMU = 0x504D55,
>  
> +	/* Experimental: Debug Trigger Extension */
> +	SBI_EXT_DBTR = 0x44425452,
> +
>  	/* Experimentals extensions must lie within this range */
>  	SBI_EXT_EXPERIMENTAL_START = 0x08000000,
>  	SBI_EXT_EXPERIMENTAL_END = 0x08FFFFFF,

This is an RFC for something I know nothing about, so was just scrolling
mindlessly... This caught my eye as odd - There's an explicit comment
about the range for experimental stuff but you've not used it? I guess
there must be some reason for that?

Confused,
Conor.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH RFC v2 1/3] riscv: add support for hardware breakpoints/watchpoints

[Sergey Matyukevich]

> > RISC-V backend for hw-breakpoint framework is built on top of SBI Debug
> > Trigger extension. Architecture specific hooks are implemented as kernel
> > wrappers around ecalls to SBI functions. This patch implements only a
> > minimal set of hooks required to support user-space debug via ptrace.
> > 
> > Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 2a0ef738695e..ef41d60a5ed3 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -31,6 +31,9 @@ enum sbi_ext_id {
> >  	SBI_EXT_SRST = 0x53525354,
> >  	SBI_EXT_PMU = 0x504D55,
> >  
> > +	/* Experimental: Debug Trigger Extension */
> > +	SBI_EXT_DBTR = 0x44425452,
> > +
> >  	/* Experimentals extensions must lie within this range */
> >  	SBI_EXT_EXPERIMENTAL_START = 0x08000000,
> >  	SBI_EXT_EXPERIMENTAL_END = 0x08FFFFFF,
> 
> This is an RFC for something I know nothing about, so was just scrolling
> mindlessly... This caught my eye as odd - There's an explicit comment
> about the range for experimental stuff but you've not used it? I guess
> there must be some reason for that?

IIUC it is not so experimental. This SBI extension accompanies the debug
spec v1.0 (frozen but not yet ratified). So sooner or later is going
to become a part of SBI spec.

I am using EID suggested in the draft v4 for this extension
posted at lists.riscv.org tech-debug mailing list, see:
https://lists.riscv.org/g/tech-debug/topic/92375492

Regards,
Sergey

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH RFC v2 2/3] riscv: ptrace: expose hardware breakpoints to debuggers

[Sergey Matyukevich]

From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>

Implement regset-based ptrace interface that exposes hardware
breakpoints to user-space debuggers.

Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
---
 arch/riscv/include/uapi/asm/ptrace.h |   9 ++
 arch/riscv/kernel/process.c          |   2 +
 arch/riscv/kernel/ptrace.c           | 188 +++++++++++++++++++++++++++
 3 files changed, 199 insertions(+)

diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
index 882547f6bd5c..e7a5e1b9ea58 100644
--- a/arch/riscv/include/uapi/asm/ptrace.h
+++ b/arch/riscv/include/uapi/asm/ptrace.h
@@ -77,6 +77,15 @@ union __riscv_fp_state {
 	struct __riscv_q_ext_state q;
 };
 
+struct user_hwdebug_state {
+	__u64     dbg_info;
+	struct {
+		__u64 addr;
+		__u64 type;
+		__u64 len;
+	}       dbg_regs[16];
+};
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* _UAPI_ASM_RISCV_PTRACE_H */
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index cd99bececed8..b66936b02caf 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -15,6 +15,7 @@
 #include <linux/tick.h>
 #include <linux/ptrace.h>
 #include <linux/uaccess.h>
+#include <linux/hw_breakpoint.h>
 
 #include <asm/unistd.h>
 #include <asm/processor.h>
@@ -148,6 +149,7 @@ void flush_thread(void)
 	fstate_off(current, task_pt_regs(current));
 	memset(&current->thread.fstate, 0, sizeof(current->thread.fstate));
 #endif
+	flush_ptrace_hw_breakpoint(current);
 }
 
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
index 2ae8280ae475..9bdc70ad7819 100644
--- a/arch/riscv/kernel/ptrace.c
+++ b/arch/riscv/kernel/ptrace.c
@@ -18,6 +18,7 @@
 #include <linux/regset.h>
 #include <linux/sched.h>
 #include <linux/sched/task_stack.h>
+#include <linux/hw_breakpoint.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/syscalls.h>
@@ -27,6 +28,10 @@ enum riscv_regset {
 #ifdef CONFIG_FPU
 	REGSET_F,
 #endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	REGSET_HW_BREAK,
+	REGSET_HW_WATCH,
+#endif
 };
 
 static int riscv_gpr_get(struct task_struct *target,
@@ -83,6 +88,170 @@ static int riscv_fpr_set(struct task_struct *target,
 }
 #endif
 
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+static void ptrace_hbptriggered(struct perf_event *bp,
+				struct perf_sample_data *data,
+				struct pt_regs *regs)
+{
+	struct arch_hw_breakpoint *bkpt = counter_arch_bp(bp);
+
+	force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)bkpt->address);
+}
+
+static int hw_break_get(struct task_struct *target,
+			const struct user_regset *regset,
+			struct membuf to)
+{
+	/* send total number of h/w debug triggers */
+	u64 count = hw_breakpoint_slots(regset->core_note_type);
+
+	membuf_write(&to, &count, sizeof(count));
+	return 0;
+}
+
+static inline int hw_break_empty(u64 addr, u64 type, u64 size)
+{
+	/* TODO: for now adjusted to current riscv-gdb behavior */
+	return (!addr && !size);
+}
+
+static int hw_break_setup_trigger(struct task_struct *target, u64 addr,
+				  u64 type, u64 size, int idx)
+{
+	struct perf_event *bp = ERR_PTR(-EINVAL);
+	struct perf_event_attr attr;
+	u32 bp_type;
+	u64 bp_len;
+
+	if (!hw_break_empty(addr, type, size)) {
+		/* bp size: gdb to kernel */
+		switch (size) {
+		case 2:
+			bp_len = HW_BREAKPOINT_LEN_2;
+			break;
+		case 4:
+			bp_len = HW_BREAKPOINT_LEN_4;
+			break;
+		case 8:
+			bp_len = HW_BREAKPOINT_LEN_8;
+			break;
+		default:
+			pr_warn("%s: unsupported size: %llu\n", __func__, size);
+			return -EINVAL;
+		}
+
+		/* bp type: gdb to kernel */
+		switch (type) {
+		case 0:
+			bp_type = HW_BREAKPOINT_X;
+			break;
+		case 1:
+			bp_type = HW_BREAKPOINT_R;
+			break;
+		case 2:
+			bp_type = HW_BREAKPOINT_W;
+			break;
+		case 3:
+			bp_type = HW_BREAKPOINT_RW;
+			break;
+		default:
+			pr_warn("%s: unsupported type: %llu\n", __func__, type);
+			return -EINVAL;
+		}
+	}
+
+	bp = target->thread.ptrace_bps[idx];
+	if (bp) {
+		attr = bp->attr;
+
+		if (hw_break_empty(addr, type, size)) {
+			attr.disabled = 1;
+		} else {
+			attr.bp_type = bp_type;
+			attr.bp_addr = addr;
+			attr.bp_len = bp_len;
+			attr.disabled = 0;
+		}
+
+		return modify_user_hw_breakpoint(bp, &attr);
+	}
+
+	if (hw_break_empty(addr, type, size))
+		return 0;
+
+	ptrace_breakpoint_init(&attr);
+	attr.bp_type = bp_type;
+	attr.bp_addr = addr;
+	attr.bp_len = bp_len;
+
+	bp = register_user_hw_breakpoint(&attr, ptrace_hbptriggered,
+					 NULL, target);
+	if (IS_ERR(bp))
+		return PTR_ERR(bp);
+
+	target->thread.ptrace_bps[idx] = bp;
+	return 0;
+}
+
+static int hw_break_set(struct task_struct *target,
+			const struct user_regset *regset,
+			unsigned int pos, unsigned int count,
+			const void *kbuf, const void __user *ubuf)
+{
+	int ret, idx = 0, offset, limit;
+	u64 addr;
+	u64 type;
+	u64 size;
+
+#define PTRACE_HBP_ADDR_SZ	sizeof(u64)
+#define PTRACE_HBP_TYPE_SZ	sizeof(u64)
+#define PTRACE_HBP_SIZE_SZ	sizeof(u64)
+
+	/* Resource info and pad */
+	offset = offsetof(struct user_hwdebug_state, dbg_regs);
+	ret = user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0, offset);
+	if (ret)
+		return ret;
+
+	/* trigger settings */
+	limit = regset->n * regset->size;
+	while (count && offset < limit) {
+		if (count < PTRACE_HBP_ADDR_SZ)
+			return -EINVAL;
+		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &addr,
+					 offset, offset + PTRACE_HBP_ADDR_SZ);
+		if (ret)
+			return ret;
+
+		offset += PTRACE_HBP_ADDR_SZ;
+
+		if (!count)
+			break;
+		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &type,
+					 offset, offset + PTRACE_HBP_TYPE_SZ);
+		if (ret)
+			return ret;
+
+		offset += PTRACE_HBP_TYPE_SZ;
+
+		ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &size,
+					 offset, offset + PTRACE_HBP_SIZE_SZ);
+		if (ret)
+			return ret;
+
+		offset += PTRACE_HBP_SIZE_SZ;
+
+		ret = hw_break_setup_trigger(target, addr, type, size, idx);
+		if (ret)
+			return ret;
+
+		idx++;
+	}
+
+	return 0;
+}
+#endif
+
 static const struct user_regset riscv_user_regset[] = {
 	[REGSET_X] = {
 		.core_note_type = NT_PRSTATUS,
@@ -102,6 +271,25 @@ static const struct user_regset riscv_user_regset[] = {
 		.set = riscv_fpr_set,
 	},
 #endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	[REGSET_HW_BREAK] = {
+		.core_note_type = NT_ARM_HW_BREAK,
+		.n = sizeof(struct user_hwdebug_state) / sizeof(u32),
+		.size = sizeof(u32),
+		.align = sizeof(u32),
+		.regset_get = hw_break_get,
+		.set = hw_break_set,
+	},
+	[REGSET_HW_WATCH] = {
+		.core_note_type = NT_ARM_HW_WATCH,
+		.n = sizeof(struct user_hwdebug_state) / sizeof(u32),
+		.size = sizeof(u32),
+		.align = sizeof(u32),
+		.regset_get = hw_break_get,
+		.set = hw_break_set,
+	},
+#endif
+
 };
 
 static const struct user_regset_view riscv_user_native_view = {
-- 
2.38.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH RFC v2 3/3] riscv: hw-breakpoints: add more trigger controls

[Sergey Matyukevich]

From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>

RISC-V SBI Debug Trigger extension proposal defines multiple functions
to control debug triggers. The pair of install/uninstall functions was
enough to implement ptrace interface for user-space debug. This patch
implements kernel wrappers for start/update/stop SBI functions. The
intended users of these control wrappers are kgdb and kernel modules
that make use of kernel-wide hardware breakpoints and watchpoints.

Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
---
 arch/riscv/include/asm/hw_breakpoint.h |  15 ++++
 arch/riscv/kernel/hw_breakpoint.c      | 116 ++++++++++++++++++++++++-
 2 files changed, 127 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h
index 5bb3b55cd464..afc59f8e034e 100644
--- a/arch/riscv/include/asm/hw_breakpoint.h
+++ b/arch/riscv/include/asm/hw_breakpoint.h
@@ -137,6 +137,9 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
 int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 				    unsigned long val, void *data);
 
+void arch_enable_hw_breakpoint(struct perf_event *bp);
+void arch_update_hw_breakpoint(struct perf_event *bp);
+void arch_disable_hw_breakpoint(struct perf_event *bp);
 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);
@@ -153,5 +156,17 @@ static inline void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
 {
 }
 
+void arch_enable_hw_breakpoint(struct perf_event *bp)
+{
+}
+
+void arch_update_hw_breakpoint(struct perf_event *bp)
+{
+}
+
+void arch_disable_hw_breakpoint(struct perf_event *bp)
+{
+}
+
 #endif /* CONFIG_HAVE_HW_BREAKPOINT */
 #endif /* __RISCV_HW_BREAKPOINT_H */
diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
index 8eddf512cd03..ca7df02830c2 100644
--- a/arch/riscv/kernel/hw_breakpoint.c
+++ b/arch/riscv/kernel/hw_breakpoint.c
@@ -2,6 +2,7 @@
 
 #include <linux/hw_breakpoint.h>
 #include <linux/perf_event.h>
+#include <linux/spinlock.h>
 #include <linux/percpu.h>
 #include <linux/kdebug.h>
 
@@ -9,6 +10,8 @@
 
 /* bps/wps currently set on each debug trigger for each cpu */
 static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM_MAX]);
+static DEFINE_PER_CPU(unsigned long, msg_lock_flags);
+static DEFINE_PER_CPU(raw_spinlock_t, msg_lock);
 
 static struct sbi_dbtr_data_msg __percpu *sbi_xmit;
 static struct sbi_dbtr_id_msg __percpu *sbi_recv;
@@ -318,6 +321,10 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 	struct perf_event **slot;
 	unsigned long idx;
 	struct sbiret ret;
+	int err = 0;
+
+	raw_spin_lock_irqsave(this_cpu_ptr(&msg_lock),
+			  *this_cpu_ptr(&msg_lock_flags));
 
 	xmit->tdata1 = cpu_to_lle(info->trig_data1.value);
 	xmit->tdata2 = cpu_to_lle(info->trig_data2);
@@ -328,25 +335,31 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 			0, 0, 0);
 	if (ret.error) {
 		pr_warn("%s: failed to install trigger\n", __func__);
-		return -EIO;
+		err = -EIO;
+		goto done;
 	}
 
 	idx = lle_to_cpu(recv->idx);
 
 	if (idx >= dbtr_total_num) {
 		pr_warn("%s: invalid trigger index %lu\n", __func__, idx);
-		return -EINVAL;
+		err = -EINVAL;
+		goto done;
 	}
 
 	slot = this_cpu_ptr(&bp_per_reg[idx]);
 	if (*slot) {
 		pr_warn("%s: slot %lu is in use\n", __func__, idx);
-		return -EBUSY;
+		err = -EBUSY;
+		goto done;
 	}
 
 	*slot = bp;
 
-	return 0;
+done:
+	raw_spin_unlock_irqrestore(this_cpu_ptr(&msg_lock),
+			       *this_cpu_ptr(&msg_lock_flags));
+	return err;
 }
 
 /* atomic: counter->ctx->lock is held */
@@ -375,6 +388,96 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 		pr_warn("%s: failed to uninstall trigger %d\n", __func__, i);
 }
 
+void arch_enable_hw_breakpoint(struct perf_event *bp)
+{
+	struct sbiret ret;
+	int i;
+
+	for (i = 0; i < dbtr_total_num; i++) {
+		struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
+
+		if (*slot == bp)
+			break;
+	}
+
+	if (i == dbtr_total_num) {
+		pr_warn("%s: unknown breakpoint\n", __func__);
+		return;
+	}
+
+	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_ENABLE,
+			i, 1, 0, 0, 0, 0);
+	if (ret.error) {
+		pr_warn("%s: failed to install trigger %d\n", __func__, i);
+		return;
+	}
+}
+EXPORT_SYMBOL_GPL(arch_enable_hw_breakpoint);
+
+void arch_update_hw_breakpoint(struct perf_event *bp)
+{
+	struct arch_hw_breakpoint *info = counter_arch_bp(bp);
+	struct sbi_dbtr_data_msg *xmit = this_cpu_ptr(sbi_xmit);
+	struct perf_event **slot;
+	struct sbiret ret;
+	int i;
+
+	for (i = 0; i < dbtr_total_num; i++) {
+		slot = this_cpu_ptr(&bp_per_reg[i]);
+
+		if (*slot == bp)
+			break;
+	}
+
+	if (i == dbtr_total_num) {
+		pr_warn("%s: unknown breakpoint\n", __func__);
+		return;
+	}
+
+	raw_spin_lock_irqsave(this_cpu_ptr(&msg_lock),
+			  *this_cpu_ptr(&msg_lock_flags));
+
+	xmit->tdata1 = cpu_to_lle(info->trig_data1.value);
+	xmit->tdata2 = cpu_to_lle(info->trig_data2);
+	xmit->tdata3 = cpu_to_lle(info->trig_data3);
+
+	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_UPDATE,
+			i, 1, __pa(xmit) >> 4,
+			0, 0, 0);
+	if (ret.error)
+		pr_warn("%s: failed to update trigger %d\n", __func__, i);
+
+	raw_spin_unlock_irqrestore(this_cpu_ptr(&msg_lock),
+			       *this_cpu_ptr(&msg_lock_flags));
+}
+EXPORT_SYMBOL_GPL(arch_update_hw_breakpoint);
+
+void arch_disable_hw_breakpoint(struct perf_event *bp)
+{
+	struct sbiret ret;
+	int i;
+
+	for (i = 0; i < dbtr_total_num; i++) {
+		struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
+
+		if (*slot == bp)
+			break;
+	}
+
+	if (i == dbtr_total_num) {
+		pr_warn("%s: unknown breakpoint\n", __func__);
+		return;
+	}
+
+	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_DISABLE,
+			i, 1, 0, 0, 0, 0);
+	if (ret.error) {
+		pr_warn("%s: failed to uninstall trigger %d\n", __func__, i);
+		return;
+	}
+}
+EXPORT_SYMBOL_GPL(arch_disable_hw_breakpoint);
+
 void hw_breakpoint_pmu_read(struct perf_event *bp)
 {
 }
@@ -406,6 +509,8 @@ void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
 
 static int __init arch_hw_breakpoint_init(void)
 {
+	unsigned int cpu;
+
 	sbi_xmit = __alloc_percpu(sizeof(*sbi_xmit), SZ_16);
 	if (!sbi_xmit) {
 		pr_warn("failed to allocate SBI xmit message buffer\n");
@@ -418,6 +523,9 @@ static int __init arch_hw_breakpoint_init(void)
 		return -ENOMEM;
 	}
 
+	for_each_possible_cpu(cpu)
+		raw_spin_lock_init(&per_cpu(msg_lock, cpu));
+
 	if (!dbtr_init)
 		arch_hw_breakpoint_init_sbi();
 
-- 
2.38.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH RFC v2 3/3] riscv: hw-breakpoints: add more trigger controls

[Andrew Bresticker]

Hi Sergey,

On Sat, Dec 3, 2022 at 4:55 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
>
> From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
>
> RISC-V SBI Debug Trigger extension proposal defines multiple functions
> to control debug triggers. The pair of install/uninstall functions was
> enough to implement ptrace interface for user-space debug. This patch
> implements kernel wrappers for start/update/stop SBI functions. The
> intended users of these control wrappers are kgdb and kernel modules
> that make use of kernel-wide hardware breakpoints and watchpoints.
>
> Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>

<snip>

> diff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h
> index 5bb3b55cd464..afc59f8e034e 100644
> --- a/arch/riscv/include/asm/hw_breakpoint.h
> +++ b/arch/riscv/include/asm/hw_breakpoint.h
> @@ -137,6 +137,9 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>  int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
>                                     unsigned long val, void *data);
>
> +void arch_enable_hw_breakpoint(struct perf_event *bp);
> +void arch_update_hw_breakpoint(struct perf_event *bp);
> +void arch_disable_hw_breakpoint(struct perf_event *bp);
>  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);
> @@ -153,5 +156,17 @@ static inline void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
>  {
>  }
>
> +void arch_enable_hw_breakpoint(struct perf_event *bp)
> +{
> +}
> +
> +void arch_update_hw_breakpoint(struct perf_event *bp)
> +{
> +}
> +
> +void arch_disable_hw_breakpoint(struct perf_event *bp)
> +{
> +}

I don't see any references to {enable,update,disable}_hw_breakpoint in
common kernel code, nor do any other architectures define these
functions. Who are the intended users? Do we even need them for kgdb
on RISC-V?

> diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
> index 8eddf512cd03..ca7df02830c2 100644
> --- a/arch/riscv/kernel/hw_breakpoint.c
> +++ b/arch/riscv/kernel/hw_breakpoint.c
> @@ -2,6 +2,7 @@
>
>  #include <linux/hw_breakpoint.h>
>  #include <linux/perf_event.h>
> +#include <linux/spinlock.h>
>  #include <linux/percpu.h>
>  #include <linux/kdebug.h>
>
> @@ -9,6 +10,8 @@
>
>  /* bps/wps currently set on each debug trigger for each cpu */
>  static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM_MAX]);
> +static DEFINE_PER_CPU(unsigned long, msg_lock_flags);
> +static DEFINE_PER_CPU(raw_spinlock_t, msg_lock);

I'm not sure I understand the point of this per-CPU spinlock. Just
disable preemption (and interrupts, if necessary).

Also, arch_{install,uninstall}_hw_breakpoint are already expected to
be called from atomic context; is the intention here that they be
callable from outside atomic context?

-Andrew

>
>  static struct sbi_dbtr_data_msg __percpu *sbi_xmit;
>  static struct sbi_dbtr_id_msg __percpu *sbi_recv;
> @@ -318,6 +321,10 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
>         struct perf_event **slot;
>         unsigned long idx;
>         struct sbiret ret;
> +       int err = 0;
> +
> +       raw_spin_lock_irqsave(this_cpu_ptr(&msg_lock),
> +                         *this_cpu_ptr(&msg_lock_flags));
>
>         xmit->tdata1 = cpu_to_lle(info->trig_data1.value);
>         xmit->tdata2 = cpu_to_lle(info->trig_data2);
> @@ -328,25 +335,31 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
>                         0, 0, 0);
>         if (ret.error) {
>                 pr_warn("%s: failed to install trigger\n", __func__);
> -               return -EIO;
> +               err = -EIO;
> +               goto done;
>         }
>
>         idx = lle_to_cpu(recv->idx);
>
>         if (idx >= dbtr_total_num) {
>                 pr_warn("%s: invalid trigger index %lu\n", __func__, idx);
> -               return -EINVAL;
> +               err = -EINVAL;
> +               goto done;
>         }
>
>         slot = this_cpu_ptr(&bp_per_reg[idx]);
>         if (*slot) {
>                 pr_warn("%s: slot %lu is in use\n", __func__, idx);
> -               return -EBUSY;
> +               err = -EBUSY;
> +               goto done;
>         }
>
>         *slot = bp;
>
> -       return 0;
> +done:
> +       raw_spin_unlock_irqrestore(this_cpu_ptr(&msg_lock),
> +                              *this_cpu_ptr(&msg_lock_flags));
> +       return err;
>  }
>
>  /* atomic: counter->ctx->lock is held */
> @@ -375,6 +388,96 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
>                 pr_warn("%s: failed to uninstall trigger %d\n", __func__, i);
>  }
>
> +void arch_enable_hw_breakpoint(struct perf_event *bp)
> +{
> +       struct sbiret ret;
> +       int i;
> +
> +       for (i = 0; i < dbtr_total_num; i++) {
> +               struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
> +
> +               if (*slot == bp)
> +                       break;
> +       }
> +
> +       if (i == dbtr_total_num) {
> +               pr_warn("%s: unknown breakpoint\n", __func__);
> +               return;
> +       }
> +
> +       ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_ENABLE,
> +                       i, 1, 0, 0, 0, 0);
> +       if (ret.error) {
> +               pr_warn("%s: failed to install trigger %d\n", __func__, i);
> +               return;
> +       }
> +}
> +EXPORT_SYMBOL_GPL(arch_enable_hw_breakpoint);
> +
> +void arch_update_hw_breakpoint(struct perf_event *bp)
> +{
> +       struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> +       struct sbi_dbtr_data_msg *xmit = this_cpu_ptr(sbi_xmit);
> +       struct perf_event **slot;
> +       struct sbiret ret;
> +       int i;
> +
> +       for (i = 0; i < dbtr_total_num; i++) {
> +               slot = this_cpu_ptr(&bp_per_reg[i]);
> +
> +               if (*slot == bp)
> +                       break;
> +       }
> +
> +       if (i == dbtr_total_num) {
> +               pr_warn("%s: unknown breakpoint\n", __func__);
> +               return;
> +       }
> +
> +       raw_spin_lock_irqsave(this_cpu_ptr(&msg_lock),
> +                         *this_cpu_ptr(&msg_lock_flags));
> +
> +       xmit->tdata1 = cpu_to_lle(info->trig_data1.value);
> +       xmit->tdata2 = cpu_to_lle(info->trig_data2);
> +       xmit->tdata3 = cpu_to_lle(info->trig_data3);
> +
> +       ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_UPDATE,
> +                       i, 1, __pa(xmit) >> 4,
> +                       0, 0, 0);
> +       if (ret.error)
> +               pr_warn("%s: failed to update trigger %d\n", __func__, i);
> +
> +       raw_spin_unlock_irqrestore(this_cpu_ptr(&msg_lock),
> +                              *this_cpu_ptr(&msg_lock_flags));
> +}
> +EXPORT_SYMBOL_GPL(arch_update_hw_breakpoint);
> +
> +void arch_disable_hw_breakpoint(struct perf_event *bp)
> +{
> +       struct sbiret ret;
> +       int i;
> +
> +       for (i = 0; i < dbtr_total_num; i++) {
> +               struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
> +
> +               if (*slot == bp)
> +                       break;
> +       }
> +
> +       if (i == dbtr_total_num) {
> +               pr_warn("%s: unknown breakpoint\n", __func__);
> +               return;
> +       }
> +
> +       ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIGGER_DISABLE,
> +                       i, 1, 0, 0, 0, 0);
> +       if (ret.error) {
> +               pr_warn("%s: failed to uninstall trigger %d\n", __func__, i);
> +               return;
> +       }
> +}
> +EXPORT_SYMBOL_GPL(arch_disable_hw_breakpoint);
> +
>  void hw_breakpoint_pmu_read(struct perf_event *bp)
>  {
>  }
> @@ -406,6 +509,8 @@ void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
>
>  static int __init arch_hw_breakpoint_init(void)
>  {
> +       unsigned int cpu;
> +
>         sbi_xmit = __alloc_percpu(sizeof(*sbi_xmit), SZ_16);
>         if (!sbi_xmit) {
>                 pr_warn("failed to allocate SBI xmit message buffer\n");
> @@ -418,6 +523,9 @@ static int __init arch_hw_breakpoint_init(void)
>                 return -ENOMEM;
>         }
>
> +       for_each_possible_cpu(cpu)
> +               raw_spin_lock_init(&per_cpu(msg_lock, cpu));
> +
>         if (!dbtr_init)
>                 arch_hw_breakpoint_init_sbi();
>
> --
> 2.38.1
>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH RFC v2 3/3] riscv: hw-breakpoints: add more trigger controls

[Sergey Matyukevich]

Hi Andrew,


> Hi Sergey,
> 
> On Sat, Dec 3, 2022 at 4:55 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
> >
> > From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
> >
> > RISC-V SBI Debug Trigger extension proposal defines multiple functions
> > to control debug triggers. The pair of install/uninstall functions was
> > enough to implement ptrace interface for user-space debug. This patch
> > implements kernel wrappers for start/update/stop SBI functions. The
> > intended users of these control wrappers are kgdb and kernel modules
> > that make use of kernel-wide hardware breakpoints and watchpoints.
> >
> > Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
> 
> <snip>
> 
> > diff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h
> > index 5bb3b55cd464..afc59f8e034e 100644
> > --- a/arch/riscv/include/asm/hw_breakpoint.h
> > +++ b/arch/riscv/include/asm/hw_breakpoint.h
> > @@ -137,6 +137,9 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
> >  int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
> >                                     unsigned long val, void *data);
> >
> > +void arch_enable_hw_breakpoint(struct perf_event *bp);
> > +void arch_update_hw_breakpoint(struct perf_event *bp);
> > +void arch_disable_hw_breakpoint(struct perf_event *bp);
> >  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);
> > @@ -153,5 +156,17 @@ static inline void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
> >  {
> >  }
> >
> > +void arch_enable_hw_breakpoint(struct perf_event *bp)
> > +{
> > +}
> > +
> > +void arch_update_hw_breakpoint(struct perf_event *bp)
> > +{
> > +}
> > +
> > +void arch_disable_hw_breakpoint(struct perf_event *bp)
> > +{
> > +}
> 
> I don't see any references to {enable,update,disable}_hw_breakpoint in
> common kernel code, nor do any other architectures define these
> functions. Who are the intended users? Do we even need them for kgdb
> on RISC-V?

SBI Debug Trigger extension proposal defines more functions than just
install/uninstall required for ptrace hw-breakpoints API. So this patch
is an attempt to expose some additional SBI features to the rest of the
kernel.

For instance, I have been using these stop/update/start functions for
managing kernel-wide hw-breakpoints when experimenting with a sample
module from samples/hw_breakpoint. It can be convenient to modify a
breakpoint or watchpoint when it fires, e.g. to perform single-step
before proceeding execution. Full-fledged unregister/register cycle is
not suitable for this task. And this is where disable/update/enable
sequence can help.

I haven't yet tried to implement hw-breakpoint support in RISC-V kgdb,
so I don't know for sure if these custom calls are needed there.

> > diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
> > index 8eddf512cd03..ca7df02830c2 100644
> > --- a/arch/riscv/kernel/hw_breakpoint.c
> > +++ b/arch/riscv/kernel/hw_breakpoint.c
> > @@ -2,6 +2,7 @@
> >
> >  #include <linux/hw_breakpoint.h>
> >  #include <linux/perf_event.h>
> > +#include <linux/spinlock.h>
> >  #include <linux/percpu.h>
> >  #include <linux/kdebug.h>
> >
> > @@ -9,6 +10,8 @@
> >
> >  /* bps/wps currently set on each debug trigger for each cpu */
> >  static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM_MAX]);
> > +static DEFINE_PER_CPU(unsigned long, msg_lock_flags);
> > +static DEFINE_PER_CPU(raw_spinlock_t, msg_lock);
> 
> I'm not sure I understand the point of this per-CPU spinlock. Just
> disable preemption (and interrupts, if necessary).
> 
> Also, arch_{install,uninstall}_hw_breakpoint are already expected to
> be called from atomic context; is the intention here that they be
> callable from outside atomic context?

These additional locsk are not needed for install/uninstall pair due to
the reason that you mentioned: those calls are called by kernel event
core in atomic context with ctx->lock held. These locks are needed only
to make sure that new 'arch_update_hw_breakpoint' function can use the
same message buffers as install/uninstall.

Regards,
Sergey

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH RFC v2 3/3] riscv: hw-breakpoints: add more trigger controls

[Andrew Bresticker]

On Mon, Dec 5, 2022 at 3:25 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
>
> Hi Andrew,
>
>
> > Hi Sergey,
> >
> > On Sat, Dec 3, 2022 at 4:55 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
> > >
> > > From: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
> > >
> > > RISC-V SBI Debug Trigger extension proposal defines multiple functions
> > > to control debug triggers. The pair of install/uninstall functions was
> > > enough to implement ptrace interface for user-space debug. This patch
> > > implements kernel wrappers for start/update/stop SBI functions. The
> > > intended users of these control wrappers are kgdb and kernel modules
> > > that make use of kernel-wide hardware breakpoints and watchpoints.
> > >
> > > Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com>
> >
> > <snip>
> >
> > > diff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h
> > > index 5bb3b55cd464..afc59f8e034e 100644
> > > --- a/arch/riscv/include/asm/hw_breakpoint.h
> > > +++ b/arch/riscv/include/asm/hw_breakpoint.h
> > > @@ -137,6 +137,9 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
> > >  int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
> > >                                     unsigned long val, void *data);
> > >
> > > +void arch_enable_hw_breakpoint(struct perf_event *bp);
> > > +void arch_update_hw_breakpoint(struct perf_event *bp);
> > > +void arch_disable_hw_breakpoint(struct perf_event *bp);
> > >  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);
> > > @@ -153,5 +156,17 @@ static inline void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
> > >  {
> > >  }
> > >
> > > +void arch_enable_hw_breakpoint(struct perf_event *bp)
> > > +{
> > > +}
> > > +
> > > +void arch_update_hw_breakpoint(struct perf_event *bp)
> > > +{
> > > +}
> > > +
> > > +void arch_disable_hw_breakpoint(struct perf_event *bp)
> > > +{
> > > +}
> >
> > I don't see any references to {enable,update,disable}_hw_breakpoint in
> > common kernel code, nor do any other architectures define these
> > functions. Who are the intended users? Do we even need them for kgdb
> > on RISC-V?
>
> SBI Debug Trigger extension proposal defines more functions than just
> install/uninstall required for ptrace hw-breakpoints API. So this patch
> is an attempt to expose some additional SBI features to the rest of the
> kernel.

Got it. IMHO it's generally best not to add dead code unless a user
for it is imminent. It's harder to review something if you don't know
exactly how it'll be used.

> For instance, I have been using these stop/update/start functions for
> managing kernel-wide hw-breakpoints when experimenting with a sample
> module from samples/hw_breakpoint. It can be convenient to modify a
> breakpoint or watchpoint when it fires, e.g. to perform single-step
> before proceeding execution. Full-fledged unregister/register cycle is
> not suitable for this task. And this is where disable/update/enable
> sequence can help.
>
> I haven't yet tried to implement hw-breakpoint support in RISC-V kgdb,
> so I don't know for sure if these custom calls are needed there.
>
> > > diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
> > > index 8eddf512cd03..ca7df02830c2 100644
> > > --- a/arch/riscv/kernel/hw_breakpoint.c
> > > +++ b/arch/riscv/kernel/hw_breakpoint.c
> > > @@ -2,6 +2,7 @@
> > >
> > >  #include <linux/hw_breakpoint.h>
> > >  #include <linux/perf_event.h>
> > > +#include <linux/spinlock.h>
> > >  #include <linux/percpu.h>
> > >  #include <linux/kdebug.h>
> > >
> > > @@ -9,6 +10,8 @@
> > >
> > >  /* bps/wps currently set on each debug trigger for each cpu */
> > >  static DEFINE_PER_CPU(struct perf_event *, bp_per_reg[HBP_NUM_MAX]);
> > > +static DEFINE_PER_CPU(unsigned long, msg_lock_flags);
> > > +static DEFINE_PER_CPU(raw_spinlock_t, msg_lock);
> >
> > I'm not sure I understand the point of this per-CPU spinlock. Just
> > disable preemption (and interrupts, if necessary).
> >
> > Also, arch_{install,uninstall}_hw_breakpoint are already expected to
> > be called from atomic context; is the intention here that they be
> > callable from outside atomic context?
>
> These additional locsk are not needed for install/uninstall pair due to
> the reason that you mentioned: those calls are called by kernel event
> core in atomic context with ctx->lock held. These locks are needed only
> to make sure that new 'arch_update_hw_breakpoint' function can use the
> same message buffers as install/uninstall.

Sure, but you can achieve the same by disabling preemption. No need
for an actual spinlock.

-Andrew

>
> Regards,
> Sergey

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv