* [PATCH v8 1/8] riscv: Introduce support for hardware break/watchpoints
2026-11-07 2:39 [PATCH v8 0/8] riscv: Introduce support for hardware break/watchpoints Himanshu Chauhan
@ 2026-11-07 2:39 ` Himanshu Chauhan
2026-11-08 2:41 ` sashiko-bot
2026-11-07 2:39 ` [PATCH v8 2/8] riscv: Add breakpoint and watchpoint test for riscv Himanshu Chauhan
` (6 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Himanshu Chauhan @ 2026-11-07 2:39 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-kernel,
linux-mm, linux-perf-users, linux-kselftest, Himanshu Chauhan
RISC-V hardware breakpoints and watchpoints are implemented through the
perf hardware breakpoint framework. Use the SBI debug trigger extension
to install, uninstall, update, enable, and disable Sdtrig triggers.
When handling a debug exception, use the trigger hit bits reported by
hardware to decide whether a trigger fired. Clear those bits after they
are read so later exceptions see fresh state.
Signed-off-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
---
MAINTAINERS | 9 +
arch/riscv/Kconfig | 2 +
arch/riscv/include/asm/hw_breakpoint.h | 49 ++
arch/riscv/include/asm/kdebug.h | 3 +-
arch/riscv/kernel/Makefile | 1 +
arch/riscv/kernel/hw_breakpoint.c | 897 +++++++++++++++++++++++++
arch/riscv/kernel/traps.c | 6 +
7 files changed, 966 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/MAINTAINERS b/MAINTAINERS
index 93abbb315065..73b69e8dcba1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24163,6 +24163,15 @@ F: Documentation/devicetree/bindings/power/riscv,rpmi-device-power.yaml
F: Documentation/devicetree/bindings/power/riscv,rpmi-mpxy-device-power.yaml
F: drivers/pmdomain/riscv/
+RISC-V DEBUGGING
+M: Himanshu Chauhan <himanshu@thechauhan.dev>
+L: linux-riscv@lists.infradead.org
+F: arch/riscv/include/asm/hw_breakpoint.h
+F: arch/riscv/include/asm/perf_event.h
+F: arch/riscv/kernel/hw_breakpoint.c
+F: arch/riscv/kernel/perf_*
+F: drivers/perf/
+
RISC-V SPACEMIT SoC Support
M: Yixun Lan <dlan@kernel.org>
R: Troy Mitchell <troy.mitchell@linux.spacemit.com>
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index e7891a2a89da..2cfd9f369534 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -173,6 +173,8 @@ config RISCV
select HAVE_FUNCTION_ARG_ACCESS_API
select HAVE_FUNCTION_ERROR_INJECTION
select HAVE_GCC_PLUGINS
+ select HAVE_HW_BREAKPOINT if PERF_EVENTS && RISCV_SBI
+ select HAVE_MIXED_BREAKPOINTS_REGS if PERF_EVENTS
select HAVE_IRQ_TIME_ACCOUNTING
select HAVE_KERNEL_BZIP2 if !EFI_ZBOOT
select HAVE_KERNEL_GZIP if !EFI_ZBOOT
diff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h
new file mode 100644
index 000000000000..dcce20ba3faa
--- /dev/null
+++ b/arch/riscv/include/asm/hw_breakpoint.h
@@ -0,0 +1,49 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2026 Qualcomm Technologies, Inc.
+ */
+
+#ifndef __RISCV_HW_BREAKPOINT_H
+#define __RISCV_HW_BREAKPOINT_H
+
+struct task_struct;
+
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+
+#include <uapi/linux/hw_breakpoint.h>
+
+struct arch_hw_breakpoint {
+ unsigned long address;
+ unsigned long len;
+ unsigned int type;
+
+ /* Trigger configuration data */
+ unsigned long tdata1;
+ unsigned long tdata2;
+ unsigned long tdata3;
+};
+
+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);
+
+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);
+
+#else
+
+#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/kernel/Makefile b/arch/riscv/kernel/Makefile
index f54c336bf547..ede0cddddd31 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -100,6 +100,7 @@ obj-$(CONFIG_DYNAMIC_FTRACE) += mcount-dyn.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 sbi_ecall.o
ifeq ($(CONFIG_RISCV_SBI), y)
obj-$(CONFIG_SMP) += sbi-ipi.o
diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
new file mode 100644
index 000000000000..421ecd8549d9
--- /dev/null
+++ b/arch/riscv/kernel/hw_breakpoint.c
@@ -0,0 +1,897 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2026 Qualcomm Technologies, Inc.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/hw_breakpoint.h>
+#include <linux/perf_event.h>
+#include <linux/spinlock.h>
+#include <linux/percpu.h>
+#include <linux/kdebug.h>
+#include <linux/bitops.h>
+#include <linux/bitfield.h>
+#include <linux/cpu.h>
+#include <linux/cpuhotplug.h>
+
+#include <asm/sbi.h>
+
+#define RISCV_HW_BP_NUM_MAX 32
+
+#if __riscv_xlen == 64
+#define cpu_to_le cpu_to_le64
+#define le_to_cpu le64_to_cpu
+#elif __riscv_xlen == 32
+#define cpu_to_le cpu_to_le32
+#define le_to_cpu le32_to_cpu
+#else
+#error "Unexpected __riscv_xlen"
+#endif
+
+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,
+};
+
+#define RISCV_DBTR_TDATA1_TYPE_MASK GENMASK(__riscv_xlen - 1, __riscv_xlen - 4)
+
+#define RISCV_DBTR_MC_LOAD BIT(0)
+#define RISCV_DBTR_MC_STORE BIT(1)
+#define RISCV_DBTR_MC_EXEC BIT(2)
+#define RISCV_DBTR_MC_U BIT(3)
+#define RISCV_DBTR_MC_S BIT(4)
+#define RISCV_DBTR_MC_HIT BIT(20)
+#define RISCV_DBTR_MC_SIZELO_MASK GENMASK(17, 16)
+#if __riscv_xlen >= 64
+#define RISCV_DBTR_MC_SIZEHI_MASK GENMASK(22, 21)
+#endif
+
+#define RISCV_DBTR_MC6_LOAD BIT(0)
+#define RISCV_DBTR_MC6_STORE BIT(1)
+#define RISCV_DBTR_MC6_EXEC BIT(2)
+#define RISCV_DBTR_MC6_U BIT(3)
+#define RISCV_DBTR_MC6_S BIT(4)
+#define RISCV_DBTR_MC6_SIZE_MASK GENMASK(19, 16)
+#define RISCV_DBTR_MC6_HIT BIT(22)
+#define RISCV_DBTR_MC6_HIT1 BIT(25)
+#define RISCV_DBTR_MC6_HIT_MASK (RISCV_DBTR_MC6_HIT | RISCV_DBTR_MC6_HIT1)
+
+#define RISCV_DBTR_MC6_HIT_FALSE 0
+#define RISCV_DBTR_MC6_HIT_BEFORE 1
+#define RISCV_DBTR_MC6_HIT_AFTER 2
+#define RISCV_DBTR_MC6_HIT_IMM_AFTER 3
+
+/* Registered per-cpu bp/wp */
+static DEFINE_PER_CPU(struct perf_event *, pcpu_hw_bp_events[RISCV_HW_BP_NUM_MAX]);
+static DEFINE_PER_CPU(raw_spinlock_t, ecall_lock);
+
+/* Per-cpu shared memory between S and M mode */
+static union sbi_dbtr_shmem_entry __percpu *sbi_dbtr_shmem;
+
+/* number of debug triggers on this cpu . */
+static int dbtr_total_num __ro_after_init;
+static int dbtr_type __ro_after_init;
+static unsigned long dbtr_init __ro_after_init;
+
+#define MEM_HI(_m) \
+ (IS_ENABLED(CONFIG_32BIT) ? upper_32_bits((u64)(_m)) : 0UL)
+#define MEM_LO(_m) ((unsigned long)(_m))
+
+static unsigned long dbtr_tdata1_type(unsigned int type)
+{
+ return FIELD_PREP(RISCV_DBTR_TDATA1_TYPE_MASK, type);
+}
+
+static unsigned long dbtr_mcontrol_size(unsigned int lo, unsigned int hi)
+{
+ unsigned long tdata1 = FIELD_PREP(RISCV_DBTR_MC_SIZELO_MASK, lo);
+
+#if __riscv_xlen >= 64
+ tdata1 |= FIELD_PREP(RISCV_DBTR_MC_SIZEHI_MASK, hi);
+#else
+ (void)hi;
+#endif
+ return tdata1;
+}
+
+static unsigned long dbtr_mcontrol6_size(unsigned int size)
+{
+ return FIELD_PREP(RISCV_DBTR_MC6_SIZE_MASK, size);
+}
+
+static unsigned int dbtr_mcontrol6_hit(unsigned long tdata1)
+{
+ return ((tdata1 & RISCV_DBTR_MC6_HIT1) ? 2 : 0) |
+ ((tdata1 & RISCV_DBTR_MC6_HIT) ? 1 : 0);
+}
+
+static bool ranges_overlap(unsigned long start1, unsigned long len1,
+ unsigned long start2, unsigned long len2)
+{
+ unsigned long end1 = start1 + len1 - 1;
+ unsigned long end2 = start2 + len2 - 1;
+
+ if (end1 < start1)
+ end1 = ~0UL;
+ if (end2 < start2)
+ end2 = ~0UL;
+
+ return start1 <= end2 && start2 <= end1;
+}
+
+static int arch_smp_setup_sbi_shmem(unsigned int cpu)
+{
+ union sbi_dbtr_shmem_entry *dbtr_shmem;
+ phys_addr_t shmem_pa;
+ struct sbiret ret;
+
+ dbtr_shmem = per_cpu_ptr(sbi_dbtr_shmem, cpu);
+
+ shmem_pa = per_cpu_ptr_to_phys(dbtr_shmem);
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_SETUP_SHMEM,
+ MEM_LO(shmem_pa), MEM_HI(shmem_pa), 0, 0, 0, 0);
+
+ if (ret.error) {
+ pr_warn("%s: failed to setup shared memory. error: %ld\n",
+ __func__, ret.error);
+ return sbi_err_map_linux_errno(ret.error);
+ }
+
+ pr_info("CPU %d: HW Breakpoint shared memory registered.\n", cpu);
+
+ return 0;
+}
+
+static int arch_smp_teardown_sbi_shmem(unsigned int cpu)
+{
+ struct sbiret ret;
+
+ /* Disable shared memory */
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_SETUP_SHMEM,
+ SBI_SHMEM_DISABLE, SBI_SHMEM_DISABLE, 0, 0, 0, 0);
+
+ if (ret.error)
+ pr_warn("%s: failed to disable shared memory. error: %ld\n",
+ __func__, ret.error);
+ else
+ pr_info("CPU %d: HW Breakpoint shared memory disabled.\n", cpu);
+
+ return 0;
+}
+
+static void init_sbi_dbtr(void)
+{
+ unsigned long tdata1;
+ struct sbiret ret;
+
+ /*
+ * Called by hw_breakpoint_slots() and arch_hw_breakpoint_init().
+ * Only the first caller should run SBI discovery; later callers may
+ * arrive after __ro_after_init data is read-only, so avoid a locked
+ * write if the bit is already set.
+ */
+ if (test_bit(0, &dbtr_init) || test_and_set_bit(0, &dbtr_init))
+ return;
+
+ if (sbi_probe_extension(SBI_EXT_DBTR) <= 0) {
+ pr_warn("SBI_EXT_DBTR is not supported\n");
+ dbtr_total_num = 0;
+ return;
+ }
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
+ 0, 0, 0, 0, 0, 0);
+ if (ret.error) {
+ pr_warn("Failed to detect triggers\n");
+ dbtr_total_num = 0;
+ return;
+ }
+
+ tdata1 = dbtr_tdata1_type(RISCV_DBTR_TRIG_MCONTROL6);
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
+ tdata1, 0, 0, 0, 0, 0);
+ if (ret.error) {
+ pr_warn("Failed to detect mcontrol6 triggers\n");
+ } else if (!ret.value) {
+ pr_warn("Type 6 triggers not available\n");
+ } else {
+ dbtr_total_num = min_t(unsigned long, ret.value,
+ RISCV_HW_BP_NUM_MAX);
+ dbtr_type = RISCV_DBTR_TRIG_MCONTROL6;
+ pr_warn("Mcontrol6 trigger available.\n");
+ return;
+ }
+
+ /* fallback to type 2 triggers if type 6 is not available */
+
+ tdata1 = dbtr_tdata1_type(RISCV_DBTR_TRIG_MCONTROL);
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
+ tdata1, 0, 0, 0, 0, 0);
+ if (ret.error) {
+ pr_warn("Failed to detect mcontrol triggers\n");
+ } else if (!ret.value) {
+ pr_warn("Type 2 triggers not available\n");
+ } else {
+ dbtr_total_num = min_t(unsigned long, ret.value,
+ RISCV_HW_BP_NUM_MAX);
+ dbtr_type = RISCV_DBTR_TRIG_MCONTROL;
+ }
+}
+
+int hw_breakpoint_slots(int type)
+{
+ /*
+ * We can be called early, so don't rely on
+ * static variables being initialised.
+ */
+
+ init_sbi_dbtr();
+
+ return dbtr_total_num;
+}
+
+int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw)
+{
+ unsigned long va, len, end;
+
+ va = hw->address;
+ /* Execute breakpoints encode "match any instruction size" as len 0. */
+ len = hw->len ?: 1;
+ end = va + len - 1;
+
+ if (end < va)
+ end = ~0UL;
+
+ return (va >= TASK_SIZE) || (end >= TASK_SIZE);
+}
+
+static int rv_init_mcontrol_trigger(const struct perf_event_attr *attr,
+ struct arch_hw_breakpoint *hw)
+{
+ unsigned long tdata1 = dbtr_tdata1_type(RISCV_DBTR_TRIG_MCONTROL);
+
+ switch (attr->bp_type) {
+ case HW_BREAKPOINT_X:
+ tdata1 |= RISCV_DBTR_MC_EXEC;
+ break;
+ case HW_BREAKPOINT_R:
+ tdata1 |= RISCV_DBTR_MC_LOAD;
+ break;
+ case HW_BREAKPOINT_W:
+ tdata1 |= RISCV_DBTR_MC_STORE;
+ break;
+ case HW_BREAKPOINT_RW:
+ tdata1 |= RISCV_DBTR_MC_LOAD | RISCV_DBTR_MC_STORE;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (attr->bp_type == HW_BREAKPOINT_X) {
+ /*
+ * Userspace debuggers can request execute breakpoints with
+ * bp_len == 2 for compressed/non-aligned instruction
+ * addresses. Program execute triggers with "match any size"
+ * to avoid missing valid instruction fetches.
+ */
+ hw->len = 0;
+ } else {
+ switch (attr->bp_len) {
+ case HW_BREAKPOINT_LEN_1:
+ hw->len = 1;
+ tdata1 |= dbtr_mcontrol_size(1, 0);
+ break;
+ case HW_BREAKPOINT_LEN_2:
+ hw->len = 2;
+ tdata1 |= dbtr_mcontrol_size(2, 0);
+ break;
+ case HW_BREAKPOINT_LEN_4:
+ hw->len = 4;
+ tdata1 |= dbtr_mcontrol_size(3, 0);
+ break;
+#if __riscv_xlen >= 64
+ case HW_BREAKPOINT_LEN_8:
+ hw->len = 8;
+ tdata1 |= dbtr_mcontrol_size(1, 1);
+ break;
+#endif
+ /* Set to match any size */
+ default:
+ hw->len = 0;
+ break;
+ }
+ }
+
+ /*
+ * Match only the trigger's actual privilege-mode target. Setting
+ * both bits makes the kernel's own S-mode uaccess (e.g. SUM-enabled
+ * loads in strncpy_from_user()) spuriously fire a user-space watch.
+ */
+ if (arch_check_bp_in_kernelspace(hw))
+ tdata1 |= RISCV_DBTR_MC_S;
+ else
+ tdata1 |= RISCV_DBTR_MC_U;
+
+ hw->tdata1 = tdata1;
+ return 0;
+}
+
+static int rv_init_mcontrol6_trigger(const struct perf_event_attr *attr,
+ struct arch_hw_breakpoint *hw)
+{
+ unsigned long tdata1 = dbtr_tdata1_type(RISCV_DBTR_TRIG_MCONTROL6);
+
+ switch (attr->bp_type) {
+ case HW_BREAKPOINT_X:
+ tdata1 |= RISCV_DBTR_MC6_EXEC;
+ break;
+ case HW_BREAKPOINT_R:
+ tdata1 |= RISCV_DBTR_MC6_LOAD;
+ break;
+ case HW_BREAKPOINT_W:
+ tdata1 |= RISCV_DBTR_MC6_STORE;
+ break;
+ case HW_BREAKPOINT_RW:
+ tdata1 |= RISCV_DBTR_MC6_STORE | RISCV_DBTR_MC6_LOAD;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (attr->bp_type == HW_BREAKPOINT_X) {
+ /* See rv_init_mcontrol_trigger() for rationale. */
+ hw->len = 0;
+ } else {
+ switch (attr->bp_len) {
+ case HW_BREAKPOINT_LEN_1:
+ hw->len = 1;
+ tdata1 |= dbtr_mcontrol6_size(1);
+ break;
+ case HW_BREAKPOINT_LEN_2:
+ hw->len = 2;
+ tdata1 |= dbtr_mcontrol6_size(2);
+ break;
+ case HW_BREAKPOINT_LEN_4:
+ hw->len = 4;
+ tdata1 |= dbtr_mcontrol6_size(3);
+ break;
+#if __riscv_xlen >= 64
+ case HW_BREAKPOINT_LEN_8:
+ hw->len = 8;
+ tdata1 |= dbtr_mcontrol6_size(5);
+ break;
+#endif
+ /* Set to match any size */
+ default:
+ hw->len = 0;
+ }
+ }
+
+ /* See rv_init_mcontrol_trigger() for rationale. */
+ if (arch_check_bp_in_kernelspace(hw))
+ tdata1 |= RISCV_DBTR_MC6_S;
+ else
+ tdata1 |= RISCV_DBTR_MC6_U;
+
+ hw->tdata1 = tdata1;
+ return 0;
+}
+
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+ const struct perf_event_attr *attr,
+ struct arch_hw_breakpoint *hw)
+{
+ int ret;
+
+ /* Breakpoint address */
+ hw->address = attr->bp_addr;
+ hw->tdata1 = 0;
+ hw->tdata2 = attr->bp_addr;
+ hw->tdata3 = 0x0;
+
+ switch (dbtr_type) {
+ case RISCV_DBTR_TRIG_MCONTROL:
+ ret = rv_init_mcontrol_trigger(attr, hw);
+ break;
+ case RISCV_DBTR_TRIG_MCONTROL6:
+ ret = rv_init_mcontrol6_trigger(attr, hw);
+ break;
+ default:
+ pr_warn("Unsupported trigger type\n");
+ ret = -EOPNOTSUPP;
+ break;
+ }
+
+ return ret;
+}
+
+static void dbtr_encode_update(union sbi_dbtr_shmem_entry *shmem,
+ unsigned long idx, unsigned long tdata1,
+ unsigned long tdata2, unsigned long tdata3)
+{
+ struct sbi_dbtr_id_msg *id = &shmem->id;
+ struct sbi_dbtr_data_msg *data = &shmem->data;
+
+ /*
+ * SBI_EXT_DBTR_TRIG_UPDATE takes the number of entries in a0. The
+ * trigger index for each entry lives in the first shared-memory word;
+ * that word is named tstate only when the same union is used for
+ * TRIG_READ replies.
+ */
+ id->idx = cpu_to_le(idx);
+ data->tdata1 = cpu_to_le(tdata1);
+ data->tdata2 = cpu_to_le(tdata2);
+ data->tdata3 = cpu_to_le(tdata3);
+}
+
+static struct sbiret dbtr_update_trigger_locked(unsigned long idx,
+ unsigned long tdata1,
+ unsigned long tdata2,
+ unsigned long tdata3)
+{
+ union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(sbi_dbtr_shmem);
+
+ dbtr_encode_update(shmem, idx, tdata1, tdata2, tdata3);
+
+ return sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_UPDATE,
+ 1, 0, 0, 0, 0, 0);
+}
+
+static int dbtr_update_event_tdata(struct perf_event *event,
+ unsigned long tdata1,
+ unsigned long tdata2,
+ unsigned long tdata3, bool wait)
+{
+ struct perf_event **slot;
+ unsigned long flags;
+ struct sbiret ret;
+ int i;
+
+ if (wait)
+ raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock), flags);
+ else if (!raw_spin_trylock_irqsave(this_cpu_ptr(&ecall_lock), flags))
+ return -EBUSY;
+
+ for (i = 0; i < dbtr_total_num; i++) {
+ slot = this_cpu_ptr(&pcpu_hw_bp_events[i]);
+
+ if (*slot == event)
+ break;
+ }
+
+ if (i == dbtr_total_num) {
+ raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock), flags);
+ return -ENOENT;
+ }
+
+ ret = dbtr_update_trigger_locked(i, tdata1, tdata2, tdata3);
+ raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock), flags);
+
+ if (ret.error)
+ return sbi_err_map_linux_errno(ret.error);
+
+ return 0;
+}
+
+static struct sbiret dbtr_uninstall_trigger_locked(unsigned long idx)
+{
+ return sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_UNINSTALL,
+ idx, 1, 0, 0, 0, 0);
+}
+
+static bool mc_read_and_clear_hit(int idx)
+{
+ union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(sbi_dbtr_shmem);
+ unsigned long flags, tdata1, tdata2, tdata3;
+ struct sbiret ret;
+ bool hit;
+
+ /*
+ * This runs from DIE_DEBUG. If the exception interrupted another DBTR
+ * SBI call on this CPU, blocking on the same raw spinlock would deadlock;
+ * the handler can still fall back to address-based matching.
+ */
+ if (!raw_spin_trylock_irqsave(this_cpu_ptr(&ecall_lock), flags))
+ return false;
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_READ,
+ idx, 1, 0, 0, 0, 0);
+ if (ret.error) {
+ pr_warn("Failed to read trigger %d. error: %ld\n",
+ idx, ret.error);
+ hit = false;
+ goto out;
+ }
+
+ tdata1 = le_to_cpu(shmem->data.tdata1);
+ hit = !!(tdata1 & RISCV_DBTR_MC_HIT);
+ if (!hit)
+ goto out;
+
+ tdata2 = le_to_cpu(shmem->data.tdata2);
+ tdata3 = le_to_cpu(shmem->data.tdata3);
+ tdata1 &= ~RISCV_DBTR_MC_HIT;
+
+ ret = dbtr_update_trigger_locked(idx, tdata1, tdata2, tdata3);
+ if (ret.error)
+ pr_warn("Failed to clear hit bit on trigger %d. error: %ld\n",
+ idx, ret.error);
+
+out:
+ raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock), flags);
+ return hit;
+}
+
+/*
+ * Read mcontrol6's hit1:hit0 field for trigger @idx. If either bit is set,
+ * clear it in hardware (the Debug spec requires the trigger user to do
+ * this; hardware only ever sets it). Returns the RISCV_DBTR_MC6_HIT_*
+ * value read (RISCV_DBTR_MC6_HIT_FALSE if unimplemented or unfired).
+ */
+static unsigned int mc6_read_and_clear_hit(int idx)
+{
+ union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(sbi_dbtr_shmem);
+ unsigned long flags, tdata1, tdata2, tdata3;
+ struct sbiret ret;
+ unsigned int hit;
+
+ /* See mc_read_and_clear_hit() for the trylock rationale. */
+ if (!raw_spin_trylock_irqsave(this_cpu_ptr(&ecall_lock), flags))
+ return RISCV_DBTR_MC6_HIT_FALSE;
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_READ, idx, 1, 0, 0, 0, 0);
+ if (ret.error) {
+ pr_warn("Failed to read trigger %d. error: %ld\n", idx, ret.error);
+ hit = RISCV_DBTR_MC6_HIT_FALSE;
+ goto out;
+ }
+
+ tdata1 = le_to_cpu(shmem->data.tdata1);
+ hit = dbtr_mcontrol6_hit(tdata1);
+ if (hit == RISCV_DBTR_MC6_HIT_FALSE)
+ goto out;
+
+ tdata2 = le_to_cpu(shmem->data.tdata2);
+ tdata3 = le_to_cpu(shmem->data.tdata3);
+ tdata1 &= ~RISCV_DBTR_MC6_HIT_MASK;
+
+ ret = dbtr_update_trigger_locked(idx, tdata1, tdata2, tdata3);
+ if (ret.error)
+ pr_warn("Failed to clear hit bits on trigger %d. error: %ld\n",
+ idx, ret.error);
+
+out:
+ raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock), flags);
+ return hit;
+}
+
+/*
+ * HW Breakpoint/watchpoint handler
+ */
+static int hw_breakpoint_handler(struct die_args *args)
+{
+ int i, bp_ret = NOTIFY_DONE;
+ struct arch_hw_breakpoint *bp;
+ struct perf_event *event;
+
+ for (i = 0; i < dbtr_total_num; i++) {
+ event = this_cpu_read(pcpu_hw_bp_events[i]);
+ if (!event)
+ continue;
+
+ bp = counter_arch_bp(event);
+
+ switch (event->attr.bp_type) {
+ /* Breakpoint */
+ case HW_BREAKPOINT_X:
+ {
+ bool hit = bp->address == args->regs->epc;
+
+ if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL)
+ hit |= mc_read_and_clear_hit(i);
+ else if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL6)
+ hit |= mc6_read_and_clear_hit(i) !=
+ RISCV_DBTR_MC6_HIT_FALSE;
+
+ if (hit) {
+ perf_bp_event(event, args->regs);
+ bp_ret = NOTIFY_STOP;
+ goto exit;
+ }
+ }
+ break;
+
+ /* Watchpoint */
+ case HW_BREAKPOINT_W:
+ case HW_BREAKPOINT_R:
+ case HW_BREAKPOINT_RW:
+ {
+ unsigned long stval = args->regs->badaddr;
+ unsigned long bp_start = bp->address;
+ unsigned long bp_len = bp->len ?: 1;
+ bool hit = false;
+
+ /*
+ * Prefer tdata1.hit from SBI trigger readout whenever
+ * possible. Fall back to address-based matching if HIT
+ * isn't observed/supported.
+ */
+ if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL)
+ hit = mc_read_and_clear_hit(i);
+ else if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL6)
+ hit = mc6_read_and_clear_hit(i) !=
+ RISCV_DBTR_MC6_HIT_FALSE;
+
+ /*
+ * Sdtrig may report STVAL as the lowest accessed
+ * address while the watchpoint can match a higher byte
+ * in the same access. Check for full range overlap
+ * rather than comparing only the two start addresses.
+ */
+ hit |= ranges_overlap(bp_start, bp_len, stval,
+ sizeof(long));
+ if (hit) {
+ perf_bp_event(event, args->regs);
+ bp_ret = NOTIFY_STOP;
+ goto exit;
+ }
+ }
+ break;
+
+ default:
+ pr_warn("Unknown type: %u\n", event->attr.bp_type);
+ goto exit;
+ }
+ }
+
+exit:
+ return bp_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 *event)
+{
+ struct arch_hw_breakpoint *bp = counter_arch_bp(event);
+ union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(sbi_dbtr_shmem);
+ struct sbi_dbtr_data_msg *xmit;
+ struct sbi_dbtr_id_msg *recv;
+ struct perf_event **slot;
+ unsigned long idx;
+ unsigned long flags;
+ struct sbiret ret;
+ int err = 0;
+
+ raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock), flags);
+
+ xmit = &shmem->data;
+ recv = &shmem->id;
+ /* Avoid sending stale union contents from a previous id reply. */
+ xmit->tstate = cpu_to_le(0UL);
+ xmit->tdata1 = cpu_to_le(bp->tdata1);
+ xmit->tdata2 = cpu_to_le(bp->tdata2);
+ xmit->tdata3 = cpu_to_le(bp->tdata3);
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_INSTALL,
+ 1, 0, 0, 0, 0, 0);
+
+ if (ret.error) {
+ pr_warn("Failed to install trigger\n");
+ err = sbi_err_map_linux_errno(ret.error);
+ goto done;
+ }
+
+ idx = le_to_cpu(recv->idx);
+ if (idx >= dbtr_total_num) {
+ pr_warn("Invalid trigger index %lu\n", idx);
+ ret = dbtr_uninstall_trigger_locked(idx);
+ if (ret.error)
+ pr_warn("Failed to unwind trigger %lu. error: %ld\n",
+ idx, ret.error);
+ err = -EINVAL;
+ goto done;
+ }
+
+ slot = this_cpu_ptr(&pcpu_hw_bp_events[idx]);
+ if (*slot) {
+ pr_warn("Slot %lu is in use\n", idx);
+ ret = dbtr_uninstall_trigger_locked(idx);
+ if (ret.error)
+ pr_warn("Failed to unwind trigger %lu. error: %ld\n",
+ idx, ret.error);
+ err = -EBUSY;
+ goto done;
+ }
+
+ /* Save the event - to be looked up in handler */
+ *slot = event;
+
+done:
+ raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock), flags);
+ return err;
+}
+
+/* atomic: counter->ctx->lock is held */
+void arch_uninstall_hw_breakpoint(struct perf_event *event)
+{
+ struct perf_event **slot = NULL;
+ unsigned long flags;
+ struct sbiret ret;
+ int i;
+
+ raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock), flags);
+
+ for (i = 0; i < dbtr_total_num; i++) {
+ struct perf_event **candidate;
+
+ candidate = this_cpu_ptr(&pcpu_hw_bp_events[i]);
+ if (*candidate == event) {
+ slot = candidate;
+ break;
+ }
+ }
+
+ if (i == dbtr_total_num) {
+ pr_warn("Breakpoint not installed.\n");
+ goto out;
+ }
+
+ /*
+ * Keep the software slot populated until the hardware trigger is gone;
+ * otherwise a last debug exception would look unhandled.
+ */
+ ret = dbtr_uninstall_trigger_locked(i);
+
+ if (ret.error) {
+ pr_warn("Failed to uninstall trigger %d.\n", i);
+ goto out;
+ }
+
+ *slot = NULL;
+
+ out:
+ raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock), flags);
+}
+
+void arch_enable_hw_breakpoint(struct perf_event *event)
+{
+ struct sbiret ret;
+ int i;
+ struct perf_event **slot;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock), flags);
+
+ for (i = 0; i < dbtr_total_num; i++) {
+ slot = this_cpu_ptr(&pcpu_hw_bp_events[i]);
+
+ if (*slot == event)
+ break;
+ }
+
+ if (i == dbtr_total_num) {
+ pr_warn("Breakpoint not installed.\n");
+ goto out;
+ }
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_ENABLE,
+ i, 1, 0, 0, 0, 0);
+
+ if (ret.error) {
+ pr_warn("Failed to install trigger %d\n", i);
+ goto out;
+ }
+
+ out:
+ raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock), flags);
+}
+EXPORT_SYMBOL_GPL(arch_enable_hw_breakpoint);
+
+void arch_update_hw_breakpoint(struct perf_event *event)
+{
+ struct arch_hw_breakpoint *bp = counter_arch_bp(event);
+ int ret;
+
+ ret = dbtr_update_event_tdata(event, bp->tdata1, bp->tdata2, bp->tdata3,
+ true);
+ if (ret)
+ pr_warn("Failed to update trigger. error: %d\n", ret);
+}
+EXPORT_SYMBOL_GPL(arch_update_hw_breakpoint);
+
+void arch_disable_hw_breakpoint(struct perf_event *event)
+{
+ struct perf_event **slot;
+ struct sbiret ret;
+ int i;
+
+ for (i = 0; i < dbtr_total_num; i++) {
+ slot = this_cpu_ptr(&pcpu_hw_bp_events[i]);
+
+ if (*slot == event)
+ break;
+ }
+
+ if (i == dbtr_total_num) {
+ pr_warn("Breakpoint not installed.\n");
+ return;
+ }
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_DISABLE,
+ i, 1, 0, 0, 0, 0);
+
+ if (ret.error) {
+ pr_warn("Failed to uninstall trigger %d.\n", i);
+ return;
+ }
+}
+EXPORT_SYMBOL_GPL(arch_disable_hw_breakpoint);
+
+void hw_breakpoint_pmu_read(struct perf_event *bp) { }
+
+void flush_ptrace_hw_breakpoint(struct task_struct *tsk) { }
+
+static int __init arch_hw_breakpoint_init(void)
+{
+ unsigned int cpu;
+ int rc = 0;
+
+ for_each_possible_cpu(cpu)
+ raw_spin_lock_init(&per_cpu(ecall_lock, cpu));
+
+ init_sbi_dbtr();
+
+ if (dbtr_total_num) {
+ pr_info("Total number of type %d triggers: %u\n",
+ dbtr_type, dbtr_total_num);
+ } else {
+ pr_info("No hardware triggers available\n");
+ goto out;
+ }
+
+ /* Allocate per-cpu shared memory */
+ sbi_dbtr_shmem = __alloc_percpu(sizeof(*sbi_dbtr_shmem) * dbtr_total_num,
+ PAGE_SIZE);
+
+ if (!sbi_dbtr_shmem) {
+ pr_warn("Failed to allocate shared memory.\n");
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ /* Hotplug handler to register/unregister shared memory with SBI */
+ rc = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+ "riscv/hw_breakpoint:prepare",
+ arch_smp_setup_sbi_shmem,
+ arch_smp_teardown_sbi_shmem);
+
+ if (rc < 0) {
+ pr_warn("Failed to setup CPU hotplug state\n");
+ free_percpu(sbi_dbtr_shmem);
+ sbi_dbtr_shmem = NULL;
+ dbtr_total_num = 0;
+ dbtr_type = RISCV_DBTR_TRIG_NONE;
+ return rc;
+ }
+ rc = 0;
+ out:
+ return rc;
+}
+arch_initcall(arch_hw_breakpoint_init);
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index f8292adda099..1b9ccc0b6b8d 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -287,6 +287,12 @@ void handle_break(struct pt_regs *regs)
if (probe_breakpoint_handler(regs))
return;
+#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;
if (user_mode(regs))
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v8 1/8] riscv: Introduce support for hardware break/watchpoints
2026-11-07 2:39 ` [PATCH v8 1/8] " Himanshu Chauhan
@ 2026-11-08 2:41 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-11-08 2:41 UTC (permalink / raw)
To: Himanshu Chauhan, Paul Walmsley, Palmer Dabbelt, Albert Ou
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-mm,
linux-perf-users
Thank you for your contribution! Sashiko AI review found 11 potential
issues to consider:
- [High] riscv: hw_breakpoint: unconditional overlap match and early
exit in handler
- [High] riscv/hw_breakpoint: handle_break() runs DIE_DEBUG with IRQs
enabled
- [High] riscv: hw_breakpoint: missing trigger step-over in
hw_breakpoint_handler()
- [High] riscv: hw_breakpoint: invalid bp_len handling and watchpoint
size programming
- [Medium] riscv: hw_breakpoint: NULL sbi_dbtr_shmem dereference on init
alloc failure
- [Medium] riscv: hw_breakpoint: wrong cpuhp state ordering for SBI
shared memory
- [Medium] riscv: hw_breakpoint: missing ecall_lock in
arch_disable_hw_breakpoint()
- [Medium] riscv: hw_breakpoint: stale slot pointer in
arch_uninstall_hw_breakpoint()
- [Low] riscv: Kconfig: unmet HAVE_HW_BREAKPOINT dependency for mixed
breakpoints
- [Low] MAINTAINERS: overbroad drivers/perf/ path and missing S: tag in
RISC-V entry
- [Low] riscv/hw_breakpoint: sparse endianness mismatch on sbi_dbtr
shmem fields
--
commit dde98179a500199d237defeb10f1d800c10ddb91
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
riscv: Introduce support for hardware break/watchpoints
This commit introduces hardware breakpoint and watchpoint support on
RISC-V using the perf hardware breakpoint framework and the SBI debug
trigger extension (Sdtrig).
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 93abbb315065d..73b69e8dcba15 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -24163,6 +24163,15 @@ F: Documentation/devicetree/bindings/power/riscv,rpmi-device-power.yaml
> F: Documentation/devicetree/bindings/power/riscv,rpmi-mpxy-device-power.yaml
> F: drivers/pmdomain/riscv/
>
> +RISC-V DEBUGGING
> +M: Himanshu Chauhan <himanshu@thechauhan.dev>
> +L: linux-riscv@lists.infradead.org
> +F: arch/riscv/include/asm/hw_breakpoint.h
> +F: arch/riscv/include/asm/perf_event.h
> +F: arch/riscv/kernel/hw_breakpoint.c
> +F: arch/riscv/kernel/perf_*
> +F: drivers/perf/
[Severity: Low]
Should this MAINTAINERS entry drop F: drivers/perf/ (which claims the
entire multi-architecture perf driver directory already covered for RISC-V
by RISC-V PMU DRIVERS), include the required S: status tag, and be placed
in alphabetical order before RISC-V RPMI DEVICE POWER DRIVER?
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index e7891a2a89da6..2cfd9f3695346 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -173,6 +173,8 @@ config RISCV
> select HAVE_FUNCTION_ARG_ACCESS_API
> select HAVE_FUNCTION_ERROR_INJECTION
> select HAVE_GCC_PLUGINS
> + select HAVE_HW_BREAKPOINT if PERF_EVENTS && RISCV_SBI
> + select HAVE_MIXED_BREAKPOINTS_REGS if PERF_EVENTS
[Severity: Low]
Does selecting HAVE_MIXED_BREAKPOINTS_REGS without gating on RISCV_SBI
cause an unmet direct dependency warning when CONFIG_PERF_EVENTS=y and
CONFIG_RISCV_SBI=n (such as on M-mode builds)? In arch/Kconfig,
HAVE_MIXED_BREAKPOINTS_REGS depends on HAVE_HW_BREAKPOINT.
> diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
> new file mode 100644
> index 0000000000000..421ecd8549d94
> --- /dev/null
> +++ b/arch/riscv/kernel/hw_breakpoint.c
[ ... ]
> + if (attr->bp_type == HW_BREAKPOINT_X) {
> + /*
> + * Userspace debuggers can request execute breakpoints with
> + * bp_len == 2 for compressed/non-aligned instruction
> + * addresses. Program execute triggers with "match any size"
> + * to avoid missing valid instruction fetches.
> + */
> + hw->len = 0;
> + } else {
> + switch (attr->bp_len) {
> + case HW_BREAKPOINT_LEN_1:
> + hw->len = 1;
> + tdata1 |= dbtr_mcontrol_size(1, 0);
> + break;
> + case HW_BREAKPOINT_LEN_2:
> + hw->len = 2;
> + tdata1 |= dbtr_mcontrol_size(2, 0);
> + break;
> + case HW_BREAKPOINT_LEN_4:
> + hw->len = 4;
> + tdata1 |= dbtr_mcontrol_size(3, 0);
> + break;
> +#if __riscv_xlen >= 64
> + case HW_BREAKPOINT_LEN_8:
> + hw->len = 8;
> + tdata1 |= dbtr_mcontrol_size(1, 1);
> + break;
> +#endif
> + /* Set to match any size */
> + default:
> + hw->len = 0;
> + break;
> + }
> + }
[Severity: High]
Does programming attr->bp_len into tdata1's size field with match = 0 cause
watchpoints in rv_init_mcontrol_trigger() and rv_init_mcontrol6_trigger()
to miss valid memory accesses?
In Sdtrig mcontrol and mcontrol6, tdata1.size filters the access width of
the load or store instruction itself, while match = 0 matches only the
exact address in tdata2. A watchpoint of length N will therefore ignore
accesses of any other width as well as accesses to non-zero byte offsets
within [bp_addr, bp_addr + bp_len) (for example, a 1-byte store to offset 1
of a 2-byte watchpoint).
Also, the default branch accepts arbitrary invalid attr->bp_len values by
setting hw->len = 0 instead of returning -EINVAL, which causes
arch_check_bp_in_kernelspace() to check only a 1-byte range against
TASK_SIZE.
[ ... ]
> +static void dbtr_encode_update(union sbi_dbtr_shmem_entry *shmem,
> + unsigned long idx, unsigned long tdata1,
> + unsigned long tdata2, unsigned long tdata3)
> +{
> + struct sbi_dbtr_id_msg *id = &shmem->id;
> + struct sbi_dbtr_data_msg *data = &shmem->data;
> +
> + /*
> + * SBI_EXT_DBTR_TRIG_UPDATE takes the number of entries in a0. The
> + * trigger index for each entry lives in the first shared-memory word;
> + * that word is named tstate only when the same union is used for
> + * TRIG_READ replies.
> + */
> + id->idx = cpu_to_le(idx);
> + data->tdata1 = cpu_to_le(tdata1);
> + data->tdata2 = cpu_to_le(tdata2);
> + data->tdata3 = cpu_to_le(tdata3);
> +}
[Severity: Low]
Does converting these shared-memory fields with cpu_to_le() and le_to_cpu()
trigger sparse endianness warnings? In arch/riscv/include/asm/sbi.h,
struct sbi_dbtr_data_msg and struct sbi_dbtr_id_msg declare idx, tstate,
and tdata1..tdata3 as unsigned long rather than __le64 / __le32, so reading
and writing them with cpu_to_le() and le_to_cpu() here, in
mc_read_and_clear_hit(), mc6_read_and_clear_hit(), and
arch_install_hw_breakpoint() mismatches restricted bitwise types.
[ ... ]
> +static int hw_breakpoint_handler(struct die_args *args)
> +{
> + int i, bp_ret = NOTIFY_DONE;
> + struct arch_hw_breakpoint *bp;
> + struct perf_event *event;
> +
> + for (i = 0; i < dbtr_total_num; i++) {
> + event = this_cpu_read(pcpu_hw_bp_events[i]);
> + if (!event)
> + continue;
> +
> + bp = counter_arch_bp(event);
> +
> + switch (event->attr.bp_type) {
> + /* Breakpoint */
> + case HW_BREAKPOINT_X:
> + {
> + bool hit = bp->address == args->regs->epc;
> +
> + if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL)
> + hit |= mc_read_and_clear_hit(i);
> + else if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL6)
> + hit |= mc6_read_and_clear_hit(i) !=
> + RISCV_DBTR_MC6_HIT_FALSE;
> +
> + if (hit) {
> + perf_bp_event(event, args->regs);
> + bp_ret = NOTIFY_STOP;
> + goto exit;
> + }
> + }
> + break;
> +
> + /* Watchpoint */
> + case HW_BREAKPOINT_W:
> + case HW_BREAKPOINT_R:
> + case HW_BREAKPOINT_RW:
> + {
> + unsigned long stval = args->regs->badaddr;
> + unsigned long bp_start = bp->address;
> + unsigned long bp_len = bp->len ?: 1;
> + bool hit = false;
> +
> + /*
> + * Prefer tdata1.hit from SBI trigger readout whenever
> + * possible. Fall back to address-based matching if HIT
> + * isn't observed/supported.
> + */
> + if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL)
> + hit = mc_read_and_clear_hit(i);
> + else if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL6)
> + hit = mc6_read_and_clear_hit(i) !=
> + RISCV_DBTR_MC6_HIT_FALSE;
> +
> + /*
> + * Sdtrig may report STVAL as the lowest accessed
> + * address while the watchpoint can match a higher byte
> + * in the same access. Check for full range overlap
> + * rather than comparing only the two start addresses.
> + */
> + hit |= ranges_overlap(bp_start, bp_len, stval,
> + sizeof(long));
> + if (hit) {
> + perf_bp_event(event, args->regs);
> + bp_ret = NOTIFY_STOP;
> + goto exit;
> + }
> + }
> + break;
[Severity: High]
Does unconditionally ORing the hardware hit readout with software address
comparisons and jumping to exit on the first match misattribute events,
leave stale HIT bits set in higher slots, and swallow software EBREAK
traps?
When a trigger in slot j > i fires at an address within sizeof(long) bytes
of a watchpoint in slot i (or with a different access type), slot i's false
hardware hit readout is overridden by ranges_overlap(), attributing the
event to slot i. Because the loop then jumps to exit,
mc_read_and_clear_hit(j) / mc6_read_and_clear_hit(j) never runs for slot j,
leaving its hardware HIT bit set for the next debug exception.
Furthermore, because handle_break() invokes DIE_DEBUG before checking for
kernel WARN_ON()/BUG() traps and hw_breakpoint_handler() does not check
that a hardware trigger fired or that user_mode(args->regs) matches the
trigger's privilege mode, a user watchpoint covering address 0..7 will
match stval == 0 on a kernel WARN_ON() EBREAK and return NOTIFY_STOP
without advancing regs->epc, livelocking the CPU on the same EBREAK.
[Severity: High]
In addition, how does execution make forward progress after a breakpoint or
watchpoint hits?
Both rv_init_mcontrol_trigger() and rv_init_mcontrol6_trigger() configure
before-execution triggers (timing = 0), so the exception is taken before
the instruction at regs->epc retires. Clearing the hit bit and returning
NOTIFY_STOP without stepping over the instruction or disabling the trigger
causes sret to return to the same instruction and re-trigger the exception
in an infinite loop.
Even with commit "riscv: Add software supported single stepping with mc/mc6
triggers" later in the series, when setup_singlestep() fails (such as for
CPU-wide breakpoints where event->hw.target is NULL),
disable_unhandled_singlestep() only queues asynchronous irq_work via
perf_event_disable_inatomic() without disabling or uninstalling the
hardware trigger before sret.
[ ... ]
> + ret = dbtr_uninstall_trigger_locked(i);
> +
> + if (ret.error) {
> + pr_warn("Failed to uninstall trigger %d.\n", i);
> + goto out;
> + }
> +
> + *slot = NULL;
> +
> + out:
> + raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock), flags);
> +}
[Severity: Medium]
If dbtr_uninstall_trigger_locked() fails in arch_uninstall_hw_breakpoint(),
does jumping to out without clearing *slot leave a dangling perf_event
pointer in pcpu_hw_bp_events[i]? Because arch_uninstall_hw_breakpoint()
returns void to hw_breakpoint_del(), the perf core still finishes tearing
down and freeing the perf_event, and later debug exceptions on that CPU
will dereference the freed event in hw_breakpoint_handler().
[ ... ]
> +void arch_disable_hw_breakpoint(struct perf_event *event)
> +{
> + struct perf_event **slot;
> + struct sbiret ret;
> + int i;
> +
> + for (i = 0; i < dbtr_total_num; i++) {
> + slot = this_cpu_ptr(&pcpu_hw_bp_events[i]);
> +
> + if (*slot == event)
> + break;
> + }
> +
> + if (i == dbtr_total_num) {
> + pr_warn("Breakpoint not installed.\n");
> + return;
> + }
> +
> + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_DISABLE,
> + i, 1, 0, 0, 0, 0);
> +
> + if (ret.error) {
> + pr_warn("Failed to uninstall trigger %d.\n", i);
> + return;
> + }
> +}
> +EXPORT_SYMBOL_GPL(arch_disable_hw_breakpoint);
[Severity: Medium]
Should arch_disable_hw_breakpoint() acquire ecall_lock with
raw_spin_lock_irqsave() like arch_enable_hw_breakpoint() and
arch_uninstall_hw_breakpoint() do? Without it, reading pcpu_hw_bp_events[]
and issuing the SBI_EXT_DBTR_TRIG_DISABLE ecall can migrate CPUs mid-call
or race with concurrent trigger operations on the same CPU.
Also, arch_enable_hw_breakpoint(), arch_update_hw_breakpoint(), and
arch_disable_hw_breakpoint() have no callers in the tree, and the error
messages in arch_enable_hw_breakpoint() and arch_disable_hw_breakpoint()
say "install" and "uninstall" instead of "enable" and "disable".
> +
> +void hw_breakpoint_pmu_read(struct perf_event *bp) { }
> +
> +void flush_ptrace_hw_breakpoint(struct task_struct *tsk) { }
> +
> +static int __init arch_hw_breakpoint_init(void)
> +{
> + unsigned int cpu;
> + int rc = 0;
> +
> + for_each_possible_cpu(cpu)
> + raw_spin_lock_init(&per_cpu(ecall_lock, cpu));
> +
> + init_sbi_dbtr();
> +
> + if (dbtr_total_num) {
> + pr_info("Total number of type %d triggers: %u\n",
> + dbtr_type, dbtr_total_num);
> + } else {
> + pr_info("No hardware triggers available\n");
> + goto out;
> + }
> +
> + /* Allocate per-cpu shared memory */
> + sbi_dbtr_shmem = __alloc_percpu(sizeof(*sbi_dbtr_shmem) * dbtr_total_num,
> + PAGE_SIZE);
> +
> + if (!sbi_dbtr_shmem) {
> + pr_warn("Failed to allocate shared memory.\n");
> + rc = -ENOMEM;
> + goto out;
> + }
[Severity: Medium]
If __alloc_percpu() fails here, does leaving dbtr_total_num and dbtr_type
set allow later breakpoint registrations to dereference a NULL
sbi_dbtr_shmem via this_cpu_ptr(sbi_dbtr_shmem) in
arch_install_hw_breakpoint()?
hw_breakpoint_slots() already ran earlier during perf_event_init() and
cached the non-zero slot count, and the same NULL sbi_dbtr_shmem
dereference can occur if an in-kernel caller registers a breakpoint between
perf_event_init() and arch_hw_breakpoint_init(). In addition,
init_sbi_dbtr() discards the total trigger count returned by
SBI_EXT_DBTR_NUM_TRIGGERS(0) and bounds-checks recv->idx in
arch_install_hw_breakpoint() against only the type-specific trigger count
dbtr_total_num, which will reject valid trigger indices >= dbtr_total_num
if triggers of other types precede them on the hart.
> +
> + /* Hotplug handler to register/unregister shared memory with SBI */
> + rc = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> + "riscv/hw_breakpoint:prepare",
> + arch_smp_setup_sbi_shmem,
> + arch_smp_teardown_sbi_shmem);
[Severity: Medium]
Does registering the SBI shared-memory callbacks at CPUHP_AP_ONLINE_DYN
disable shared memory too early during CPU hotplug teardown and enable it
too late during bringup?
In enum cpuhp_state, CPUHP_AP_ONLINE_DYN runs after CPUHP_AP_PERF_ONLINE
and CPUHP_AP_SCHED_WAIT_EMPTY on bringup, and before them on teardown.
When a CPU goes offline, arch_smp_teardown_sbi_shmem() disables SBI shared
memory while per-CPU and per-task breakpoints are still active on that CPU,
causing trigger reads, updates, or installations before
CPUHP_AP_PERF_ONLINE and CPUHP_AP_SCHED_WAIT_EMPTY to fail with
SBI_ERR_NO_SHMEM.
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index f8292adda0999..1b9ccc0b6b8d8 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -287,6 +287,12 @@ void handle_break(struct pt_regs *regs)
> if (probe_breakpoint_handler(regs))
> return;
>
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> + if (notify_die(DIE_DEBUG, "EBREAK", regs, 0, regs->cause, SIGTRAP)
> + == NOTIFY_STOP)
> + return;
> +#endif
[Severity: High]
Does calling notify_die(DIE_DEBUG, ...) from handle_break() invoke
hw_breakpoint_handler() and perf_bp_event() with local interrupts and
preemption enabled on user-mode traps?
do_trap_break() calls local_irq_enable() before handle_break(regs) when
user_mode(regs) is true:
arch/riscv/kernel/traps.c:do_trap_break() {
if (user_mode(regs)) {
irqentry_enter_from_user_mode(regs);
local_irq_enable();
handle_break(regs);
perf_bp_event() requires local interrupts and preemption to be disabled
(asserted via lockdep_assert_irqs_disabled() in kernel/events/core.c), and
hw_breakpoint_handler(), mc_read_and_clear_hit(), and
mc6_read_and_clear_hit() also call this_cpu_read() and this_cpu_ptr()
before acquiring ecall_lock. Running with interrupts and preemption
enabled triggers lockdep and CONFIG_DEBUG_PREEMPT warnings and allows task
migration or cross-CPU perf IPIs to race with trigger readout and event
delivery.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261107023917.40606-1-himanshu.chauhan@oss.qualcomm.com?part=1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v8 2/8] riscv: Add breakpoint and watchpoint test for riscv
2026-11-07 2:39 [PATCH v8 0/8] riscv: Introduce support for hardware break/watchpoints Himanshu Chauhan
2026-11-07 2:39 ` [PATCH v8 1/8] " Himanshu Chauhan
@ 2026-11-07 2:39 ` Himanshu Chauhan
2026-11-08 2:41 ` sashiko-bot
2026-11-07 2:39 ` [PATCH v8 3/8] riscv: add ptrace requests for hardware breakpoints Himanshu Chauhan
` (5 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Himanshu Chauhan @ 2026-11-07 2:39 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-kernel,
linux-mm, linux-perf-users, linux-kselftest, Himanshu Chauhan
Add self test for riscv architecture. It uses ptrace to ptrace framework
to set/unset break/watchpoint and uses signals to check triggers.
Also add $(KHDR_INCLUDES) to CFLAGS for the riscv test build so the
UAPI headers exercised by the test can be located during compilation.
Signed-off-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
---
MAINTAINERS | 1 +
tools/testing/selftests/breakpoints/Makefile | 8 +
.../breakpoints/breakpoint_test_riscv.c | 215 ++++++++++++++++++
3 files changed, 224 insertions(+)
create mode 100644 tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 73b69e8dcba1..baf97e8c39f6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24171,6 +24171,7 @@ F: arch/riscv/include/asm/perf_event.h
F: arch/riscv/kernel/hw_breakpoint.c
F: arch/riscv/kernel/perf_*
F: drivers/perf/
+F: tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
RISC-V SPACEMIT SoC Support
M: Yixun Lan <dlan@kernel.org>
diff --git a/tools/testing/selftests/breakpoints/Makefile b/tools/testing/selftests/breakpoints/Makefile
index 0b8f5acf7c78..1f28f50e8f85 100644
--- a/tools/testing/selftests/breakpoints/Makefile
+++ b/tools/testing/selftests/breakpoints/Makefile
@@ -12,5 +12,13 @@ ifneq (,$(filter $(ARCH),aarch64 arm64))
TEST_GEN_PROGS += breakpoint_test_arm64
endif
+ifneq (,$(filter $(ARCH),riscv riscv64))
+TEST_GEN_PROGS += breakpoint_test_riscv
+endif
+
include ../lib.mk
+ifneq (,$(filter $(ARCH),riscv riscv64))
+$(OUTPUT)/breakpoint_test_riscv: CFLAGS += -static $(KHDR_INCLUDES)
+endif
+
diff --git a/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
new file mode 100644
index 000000000000..a8a94a788142
--- /dev/null
+++ b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2026 Qualcomm Technologies, Inc.
+ *
+ * Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
+ */
+
+#define _GNU_SOURCE
+#include <linux/perf_event.h> /* Definition of PERF_* constants */
+#include <linux/hw_breakpoint.h> /* Definition of HW_* constants */
+#include <sys/syscall.h> /* Definition of SYS_* constants */
+#include <unistd.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/ioctl.h>
+#include <time.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <sys/mman.h>
+#include <string.h>
+#include <semaphore.h>
+#include <errno.h>
+
+static int gfd;
+sem_t ib_mtx, wp_mtx;
+static int bp_triggered, wp_triggered;
+static int test_func_sink;
+static const int wait_timeout_sec = 5;
+
+int setup_bp(bool is_x, void *addr, int sig)
+{
+ struct perf_event_attr pe;
+ int fd;
+
+ memset(&pe, 0, sizeof(struct perf_event_attr));
+ pe.type = PERF_TYPE_BREAKPOINT;
+ pe.size = sizeof(struct perf_event_attr);
+
+ pe.config = 0;
+ pe.bp_type = is_x ? HW_BREAKPOINT_X : HW_BREAKPOINT_W;
+ pe.bp_addr = (unsigned long)addr;
+ pe.bp_len = sizeof(long);
+
+ pe.sample_period = 1;
+ pe.sample_type = PERF_SAMPLE_IP;
+ pe.wakeup_events = 1;
+
+ pe.disabled = 1;
+ pe.exclude_kernel = 1;
+ pe.exclude_hv = 1;
+
+ fd = syscall(SYS_perf_event_open, &pe, 0, -1, -1, 0);
+ if (fd < 0) {
+ printf("Failed to open event: %llx\n", pe.config);
+ return -1;
+ }
+
+ fcntl(fd, F_SETFL, O_RDWR | O_NONBLOCK | O_ASYNC);
+ fcntl(fd, F_SETSIG, sig);
+ fcntl(fd, F_SETOWN, getpid());
+
+ ioctl(fd, PERF_EVENT_IOC_RESET, 0);
+
+ return fd;
+}
+
+static void sig_handler_bp(int signum, siginfo_t *oh, void *uc)
+{
+ int ret;
+
+ bp_triggered++;
+
+ printf("Breakpoint triggered!\n");
+ ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
+ ret = sem_post(&ib_mtx);
+ if (ret) {
+ printf("Failed to report BP success\n");
+ return;
+ }
+}
+
+static void sig_handler_wp(int signum, siginfo_t *oh, void *uc)
+{
+ int ret;
+
+ printf("Watchpoint triggered!\n");
+ ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
+ wp_triggered++;
+
+ ret = sem_post(&wp_mtx);
+
+ if (ret) {
+ printf("Failed to report WP success\n");
+ return;
+ }
+}
+
+/*
+ * Keep a real instruction address for HW execute breakpoints: prevent inlining
+ * and force a visible side effect so the function can't be optimized away.
+ */
+static __attribute_noinline__ void test_func(void)
+{
+ test_func_sink++;
+ __asm__ __volatile__("" : : "g" (test_func_sink));
+}
+
+static int trigger_bp(void)
+{
+ struct sigaction sa;
+
+ memset(&sa, 0, sizeof(struct sigaction));
+ sa.sa_sigaction = (void *)sig_handler_bp;
+ sa.sa_flags = SA_SIGINFO;
+
+ if (sigaction(SIGIO, &sa, NULL) < 0) {
+ printf("Failed to setup signal handler\n");
+ return -1;
+ }
+
+ gfd = setup_bp(1, test_func, SIGIO);
+
+ if (gfd < 0) {
+ printf("Failed to setup breakpoint.\n");
+ return -1;
+ }
+
+ ioctl(gfd, PERF_EVENT_IOC_ENABLE, 0);
+
+ test_func();
+
+ ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
+
+ close(gfd);
+
+ return 0;
+}
+
+static int trigger_wp(void)
+{
+ struct sigaction sa;
+ unsigned long test_data;
+
+ memset(&sa, 0, sizeof(struct sigaction));
+ sa.sa_sigaction = (void *)sig_handler_wp;
+ sa.sa_flags = SA_SIGINFO;
+
+ if (sigaction(SIGUSR1, &sa, NULL) < 0) {
+ printf("Failed to setup signal handler\n");
+ return -1;
+ }
+
+ gfd = setup_bp(0, &test_data, SIGUSR1);
+
+ if (gfd < 0) {
+ printf("Failed to setup watchpoint\n");
+ return -1;
+ }
+
+ ioctl(gfd, PERF_EVENT_IOC_ENABLE, 0);
+ test_data = 0xdeadbeef;
+ ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
+
+ return 0;
+}
+
+static int wait_event(sem_t *sem, const char *name)
+{
+ struct timespec ts;
+
+ if (clock_gettime(CLOCK_REALTIME, &ts)) {
+ printf("%s: Failed to get current time\n", name);
+ return -1;
+ }
+
+ /*
+ * Deadlock fix: avoid blocking forever on sem_wait() if the breakpoint/
+ * watchpoint signal never arrives. Use a bounded wait and fail the test
+ * on timeout instead.
+ */
+ ts.tv_sec += wait_timeout_sec;
+ if (!sem_timedwait(sem, &ts))
+ return 0;
+
+ if (errno == ETIMEDOUT)
+ printf("%s: Timed out waiting for event\n", name);
+ else
+ printf("%s: sem_timedwait() failed with %d\n", name, errno);
+
+ return -1;
+}
+
+int main(int argc, char *argv[])
+{
+ sem_init(&ib_mtx, 0, 0);
+ if (trigger_bp() < 0)
+ return -1;
+ if (wait_event(&ib_mtx, "Breakpoint") < 0)
+ return -1;
+
+ if (bp_triggered)
+ printf("Breakpoint test passed!\n");
+
+ sem_init(&wp_mtx, 0, 0);
+ if (trigger_wp() < 0)
+ return -1;
+ if (wait_event(&wp_mtx, "Watchpoint") < 0)
+ return -1;
+
+ if (wp_triggered)
+ printf("Watchpoint test passed!\n");
+
+ return 0;
+}
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v8 2/8] riscv: Add breakpoint and watchpoint test for riscv
2026-11-07 2:39 ` [PATCH v8 2/8] riscv: Add breakpoint and watchpoint test for riscv Himanshu Chauhan
@ 2026-11-08 2:41 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-11-08 2:41 UTC (permalink / raw)
To: Himanshu Chauhan, Paul Walmsley, Palmer Dabbelt, Albert Ou
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-mm,
linux-perf-users
Thank you for your contribution! Sashiko AI review found 7 potential
issues to consider:
- [Medium] selftests/breakpoints: missing kselftest API and skip logic
in setup_bp()
- [Medium] selftests/breakpoints: premature close(gfd) and dead store in
trigger_wp()
- [Medium] selftests/breakpoints: non-volatile test_data in trigger_wp()
can be optimized out
- [Low] selftests/breakpoints: forced -static CFLAGS and missing
.gitignore entry
- [Low] selftests/breakpoints: commit message claims ptrace usage and
.gitignore misses binary
- [Low] selftests/breakpoints: non-portable glibc macro
__attribute_noinline__ in test_func()
- [Low] selftests/breakpoints: unhandled EINTR in wait_event()
sem_timedwait()
--
commit 1561777f5a1538f9c1adcbe99231ade440297bf1
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
riscv: Add breakpoint and watchpoint test for riscv
This commit adds a RISC-V kselftest for hardware execute breakpoints and
write watchpoints using perf_event_open() and asynchronous signal delivery.
> Add self test for riscv architecture. It uses ptrace to ptrace framework
> to set/unset break/watchpoint and uses signals to check triggers.
[Severity: Low]
As reported by Jesse Taube on v5 [1], does this test use the perf_event_open()
interface rather than ptrace? In addition, "ptrace to ptrace" is a typo in
this sentence, and the ptrace-based tests are only added later in commit
"selftests/breakpoints: extend riscv test for ptrace hw break/watchpoints".
Also, breakpoint_test_riscv is not added to
tools/testing/selftests/breakpoints/.gitignore.
> diff --git a/tools/testing/selftests/breakpoints/Makefile b/tools/testing/selftests/breakpoints/Makefile
> index 0b8f5acf7c78f..1f28f50e8f85f 100644
> --- a/tools/testing/selftests/breakpoints/Makefile
> +++ b/tools/testing/selftests/breakpoints/Makefile
> @@ -12,5 +12,13 @@ ifneq (,$(filter $(ARCH),aarch64 arm64))
> TEST_GEN_PROGS += breakpoint_test_arm64
> endif
>
> +ifneq (,$(filter $(ARCH),riscv riscv64))
> +TEST_GEN_PROGS += breakpoint_test_riscv
> +endif
> +
> include ../lib.mk
>
> +ifneq (,$(filter $(ARCH),riscv riscv64))
> +$(OUTPUT)/breakpoint_test_riscv: CFLAGS += -static $(KHDR_INCLUDES)
> +endif
> +
[Severity: Low]
Will forcing -static without -pthread fail to link on build environments
without static libc/libpthread archives or where sem_init(), sem_post(), and
sem_timedwait() require explicit pthread linkage? Also, the $(ARCH) filter
omits riscv32 on 32-bit RISC-V hosts, and tools/testing/selftests/breakpoints/.gitignore
is not updated with breakpoint_test_riscv. In breakpoint_test_riscv.c,
ib_mtx, wp_mtx, and setup_bp() can be static, and bp_triggered and
wp_triggered should be volatile sig_atomic_t since they are updated in signal
handlers.
> diff --git a/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
> new file mode 100644
> index 0000000000000..a8a94a788142d
> --- /dev/null
> +++ b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
[ ... ]
> +int setup_bp(bool is_x, void *addr, int sig)
> +{
> + struct perf_event_attr pe;
> + int fd;
[ ... ]
> + fd = syscall(SYS_perf_event_open, &pe, 0, -1, -1, 0);
> + if (fd < 0) {
> + printf("Failed to open event: %llx\n", pe.config);
> + return -1;
> + }
> +
> + fcntl(fd, F_SETFL, O_RDWR | O_NONBLOCK | O_ASYNC);
> + fcntl(fd, F_SETSIG, sig);
> + fcntl(fd, F_SETOWN, getpid());
> +
> + ioctl(fd, PERF_EVENT_IOC_RESET, 0);
> +
> + return fd;
> +}
[Severity: Medium]
Should setup_bp() and main() use the kselftest.h helpers and skip via
ksft_exit_skip() when hardware breakpoints are unsupported or restricted?
On RISC-V platforms without SBI_EXT_DBTR / Sdtrig support or when perf events
are disabled or restricted, syscall(SYS_perf_event_open, ...) fails and causes
main() to fail the test run with exit code 255 instead of skipping, while
printing pe.config (which is always 0) instead of errno. Also, the fcntl()
and ioctl() return values are unchecked, and in main(), if (bp_triggered) and
if (wp_triggered) only guard printf() calls, so main() still returns 0 if
wait_event() succeeds while either counter is 0.
[ ... ]
> +static __attribute_noinline__ void test_func(void)
> +{
> + test_func_sink++;
> + __asm__ __volatile__("" : : "g" (test_func_sink));
> +}
[Severity: Low]
Will __attribute_noinline__ fail to compile against non-glibc C libraries
such as musl? __attribute_noinline__ is a glibc <sys/cdefs.h> macro; using
__attribute__((noinline)) avoids breaking musl builds.
> +
> +static int trigger_bp(void)
> +{
[ ... ]
> + ioctl(gfd, PERF_EVENT_IOC_ENABLE, 0);
> +
> + test_func();
> +
> + ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
> +
> + close(gfd);
> +
> + return 0;
> +}
[Severity: Medium]
Does closing gfd here before main() calls wait_event(&ib_mtx, "Breakpoint")
race with asynchronous signal delivery?
Because perf event O_ASYNC signals are queued via irq_work, if signal delivery
is delayed past test_func(), close(gfd) tears down the perf event and its
fasync registration before the signal is sent, or races with sig_handler_bp()
calling ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0) on a closed descriptor. Also,
sig_handler_bp() and sig_handler_wp() call printf(), which is not
async-signal-safe, and trigger_wp() never closes gfd before returning (note
that commit "selftests/breakpoints: extend riscv test for ptrace hw
break/watchpoints" later in the series adds close(gfd) to trigger_wp(),
though still before wait_event()).
> +
> +static int trigger_wp(void)
> +{
> + struct sigaction sa;
> + unsigned long test_data;
[ ... ]
> + gfd = setup_bp(0, &test_data, SIGUSR1);
> +
> + if (gfd < 0) {
> + printf("Failed to setup watchpoint\n");
> + return -1;
> + }
> +
> + ioctl(gfd, PERF_EVENT_IOC_ENABLE, 0);
> + test_data = 0xdeadbeef;
> + ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
> +
> + return 0;
> +}
[Severity: Medium]
Can the compiler eliminate the store to test_data as a dead store?
test_data is a non-volatile local variable that is never read before
trigger_wp() returns, so under -O2 the store between the enable and disable
ioctl() calls can be optimized away and the watchpoint will never fire.
Qualifying test_data as volatile prevents this, and gfd also remains open on
the stack address &test_data when trigger_wp() returns in this patch
(close(gfd) is added later in commit "selftests/breakpoints: extend riscv
test for ptrace hw break/watchpoints").
> +
> +static int wait_event(sem_t *sem, const char *name)
> +{
[ ... ]
> + ts.tv_sec += wait_timeout_sec;
> + if (!sem_timedwait(sem, &ts))
> + return 0;
> +
> + if (errno == ETIMEDOUT)
> + printf("%s: Timed out waiting for event\n", name);
> + else
> + printf("%s: sem_timedwait() failed with %d\n", name, errno);
> +
> + return -1;
> +}
[Severity: Low]
Does sem_timedwait() fail with EINTR if SIGIO or SIGUSR1 is delivered while
wait_event() is blocked? POSIX sem_timedwait() does not automatically restart
when interrupted by a signal handler, so when the signal arrives during the
wait and the handler posts the semaphore, sem_timedwait() returns -1 with
errno == EINTR unless retried in a loop.
[1] https://lore.kernel.org/r/CADRr4bdFr+cu-1L0cSAJUwN-ehHiyb=4WHmwh3xS2s6q33ZCVw@mail.gmail.com
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261107023917.40606-1-himanshu.chauhan@oss.qualcomm.com?part=2
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v8 3/8] riscv: add ptrace requests for hardware breakpoints
2026-11-07 2:39 [PATCH v8 0/8] riscv: Introduce support for hardware break/watchpoints Himanshu Chauhan
2026-11-07 2:39 ` [PATCH v8 1/8] " Himanshu Chauhan
2026-11-07 2:39 ` [PATCH v8 2/8] riscv: Add breakpoint and watchpoint test for riscv Himanshu Chauhan
@ 2026-11-07 2:39 ` Himanshu Chauhan
2026-11-07 7:04 ` kernel test robot
2026-11-08 2:41 ` sashiko-bot
2026-11-07 2:39 ` [PATCH v8 4/8] riscv: add regset access " Himanshu Chauhan
` (4 subsequent siblings)
7 siblings, 2 replies; 17+ messages in thread
From: Himanshu Chauhan @ 2026-11-07 2:39 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-kernel,
linux-mm, linux-perf-users, linux-kselftest, Himanshu Chauhan
Add support for the RISC-V PTRACE_GETHBPREGS and PTRACE_SETHBPREGS
requests. A tracer can use these requests to read or program one
hardware breakpoint or watchpoint slot at a time.
Store the ptrace-owned breakpoints in the traced task and use perf
hardware breakpoint events to program the trigger. Report hits as
SIGTRAP/TRAP_HWBKPT. Clear the events when a thread is flushed or copied
so breakpoints do not survive execve() or fork().
Signed-off-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
---
arch/riscv/include/asm/hw_breakpoint.h | 13 +
arch/riscv/include/asm/processor.h | 15 ++
arch/riscv/include/uapi/asm/ptrace.h | 9 +
arch/riscv/kernel/hw_breakpoint.c | 2 -
arch/riscv/kernel/process.c | 2 +
arch/riscv/kernel/ptrace.c | 321 +++++++++++++++++++++++++
6 files changed, 360 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h
index dcce20ba3faa..6e8913d69df2 100644
--- a/arch/riscv/include/asm/hw_breakpoint.h
+++ b/arch/riscv/include/asm/hw_breakpoint.h
@@ -12,10 +12,17 @@ struct task_struct;
#include <uapi/linux/hw_breakpoint.h>
+/* Maximum number of hardware breakpoints supported */
+#define RISCV_MAX_BP 16
+
struct arch_hw_breakpoint {
unsigned long address;
unsigned long len;
unsigned int type;
+ unsigned int match;
+ unsigned int chain;
+ unsigned int select;
+ unsigned int time;
/* Trigger configuration data */
unsigned long tdata1;
@@ -42,8 +49,14 @@ 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);
+void clear_ptrace_hw_breakpoint(struct task_struct *tsk);
+void flush_ptrace_hw_breakpoint(struct task_struct *tsk);
+void ptrace_hw_copy_thread(struct task_struct *task);
#else
+static inline void flush_ptrace_hw_breakpoint(struct task_struct *task) { }
+static inline void ptrace_hw_copy_thread(struct task_struct *task) { }
+
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
#endif /* __RISCV_HW_BREAKPOINT_H */
diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index e9d339bcb7e9..a124ea48d805 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -15,6 +15,7 @@
#include <asm/ptrace.h>
#include <asm/insn-def.h>
#include <asm/alternative-macros.h>
+#include <asm/hw_breakpoint.h>
#include <asm/hwcap.h>
#include <asm/usercfi.h>
#include <asm/errata_list.h>
@@ -111,6 +112,19 @@ struct pt_regs;
#define RISCV_PREEMPT_V_NEED_RESTORE 0x40000000
#define RISCV_PREEMPT_V_IN_SCHEDULE 0x20000000
+struct debug_info {
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+ /* Have we suspended stepping by a debugger? */
+ int suspended_step;
+ /* Allow breakpoints and watchpoints to be disabled for this thread. */
+ int bp_disabled;
+ int wp_disabled;
+ /* Hardware breakpoints pinned to this task. */
+ struct perf_event *hbp_break[RISCV_MAX_BP];
+ struct perf_event *hbp_watch[RISCV_MAX_BP];
+#endif
+};
+
/* CPU-specific state of a task */
struct thread_struct {
/* Callee-saved registers */
@@ -135,6 +149,7 @@ struct thread_struct {
#ifdef CONFIG_RISCV_ISA_SSQOSID
u32 srmcfg;
#endif
+ struct debug_info debug;
};
/* Whitelist the fstate from the task_struct for hardened usercopy */
diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
index 3de2b7124aff..373a1c14f207 100644
--- a/arch/riscv/include/uapi/asm/ptrace.h
+++ b/arch/riscv/include/uapi/asm/ptrace.h
@@ -15,6 +15,8 @@
#define PTRACE_GETFDPIC_EXEC 0
#define PTRACE_GETFDPIC_INTERP 1
+#define PTRACE_GETHBPREGS 0x4210
+#define PTRACE_SETHBPREGS 0x4211
/*
* User-mode register state for core dumps, ptrace, sigcontext
@@ -164,6 +166,13 @@ struct user_cfi_state {
__u64 shstk_ptr;
};
+struct __riscv_hwdebug_state {
+ __u64 addr;
+ __u64 type;
+ __u64 len;
+ __u64 ctrl;
+};
+
#endif /* __ASSEMBLER__ */
#endif /* _UAPI_ASM_RISCV_PTRACE_H */
diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
index 421ecd8549d9..4231170d4901 100644
--- a/arch/riscv/kernel/hw_breakpoint.c
+++ b/arch/riscv/kernel/hw_breakpoint.c
@@ -846,8 +846,6 @@ EXPORT_SYMBOL_GPL(arch_disable_hw_breakpoint);
void hw_breakpoint_pmu_read(struct perf_event *bp) { }
-void flush_ptrace_hw_breakpoint(struct task_struct *tsk) { }
-
static int __init arch_hw_breakpoint_init(void)
{
unsigned int cpu;
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index 34c1d498d1ca..083c45e657b6 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -204,6 +204,7 @@ void flush_thread(void)
if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM))
envcfg_update_bits(current, ENVCFG_PMM, ENVCFG_PMM_PMLEN_0);
#endif
+ flush_ptrace_hw_breakpoint(current);
}
void arch_release_task_struct(struct task_struct *tsk)
@@ -283,6 +284,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
p->thread.riscv_v_flags = 0;
if (has_vector() || has_xtheadvector())
riscv_v_thread_alloc(p);
+ ptrace_hw_copy_thread(p);
p->thread.sp = (unsigned long)childregs; /* kernel sp */
return 0;
}
diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
index a0055743d3ba..be076865d02d 100644
--- a/arch/riscv/kernel/ptrace.c
+++ b/arch/riscv/kernel/ptrace.c
@@ -18,8 +18,10 @@
#include <linux/elf.h>
#include <linux/regset.h>
#include <linux/sched.h>
+#include <linux/sched/signal.h>
#include <linux/sched/task_stack.h>
#include <asm/usercfi.h>
+#include <linux/hw_breakpoint.h>
enum riscv_regset {
REGSET_X,
@@ -380,6 +382,152 @@ static int riscv_cfi_set(struct task_struct *target,
}
#endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+enum riscv_hbp_type {
+ RISCV_HBP_BREAK,
+ RISCV_HBP_WATCH,
+};
+
+/*
+ * Handle hitting a HW-breakpoint.
+ */
+static void riscv_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);
+}
+
+/*
+ * 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 < RISCV_MAX_BP; i++) {
+ if (t->debug.hbp_break[i]) {
+ unregister_hw_breakpoint(t->debug.hbp_break[i]);
+ t->debug.hbp_break[i] = NULL;
+ }
+ }
+
+ for (i = 0; i < RISCV_MAX_BP; i++) {
+ if (t->debug.hbp_watch[i]) {
+ unregister_hw_breakpoint(t->debug.hbp_watch[i]);
+ t->debug.hbp_watch[i] = NULL;
+ }
+ }
+}
+
+void ptrace_hw_copy_thread(struct task_struct *tsk)
+{
+ memset(&tsk->thread.debug, 0, sizeof(struct debug_info));
+}
+
+static struct perf_event *ptrace_hbp_get_event(enum riscv_hbp_type hbp_type,
+ struct task_struct *tsk,
+ unsigned long idx)
+{
+ struct perf_event *bp = ERR_PTR(-EINVAL);
+
+ switch (hbp_type) {
+ case RISCV_HBP_BREAK:
+ if (idx >= RISCV_MAX_BP)
+ goto out;
+ idx = array_index_nospec(idx, RISCV_MAX_BP);
+ bp = tsk->thread.debug.hbp_break[idx];
+ break;
+ case RISCV_HBP_WATCH:
+ if (idx >= RISCV_MAX_BP)
+ goto out;
+ idx = array_index_nospec(idx, RISCV_MAX_BP);
+ bp = tsk->thread.debug.hbp_watch[idx];
+ break;
+ }
+
+out:
+ return bp;
+}
+
+static int ptrace_hbp_set_event(enum riscv_hbp_type hbp_type,
+ struct task_struct *tsk,
+ unsigned long idx,
+ struct perf_event *bp)
+{
+ int err = -EINVAL;
+
+ switch (hbp_type) {
+ case RISCV_HBP_BREAK:
+ if (idx >= RISCV_MAX_BP)
+ goto out;
+ idx = array_index_nospec(idx, RISCV_MAX_BP);
+ tsk->thread.debug.hbp_break[idx] = bp;
+ err = 0;
+ break;
+ case RISCV_HBP_WATCH:
+ if (idx >= RISCV_MAX_BP)
+ goto out;
+ idx = array_index_nospec(idx, RISCV_MAX_BP);
+ tsk->thread.debug.hbp_watch[idx] = bp;
+ err = 0;
+ break;
+ }
+
+out:
+ return err;
+}
+
+static struct perf_event *ptrace_hbp_create(enum riscv_hbp_type hbp_type,
+ struct task_struct *tsk,
+ unsigned long idx)
+{
+ struct perf_event *bp;
+ struct perf_event_attr attr;
+ int err, type;
+
+ switch (hbp_type) {
+ case RISCV_HBP_BREAK:
+ type = HW_BREAKPOINT_X;
+ break;
+ case RISCV_HBP_WATCH:
+ type = HW_BREAKPOINT_RW;
+ break;
+ default:
+ return ERR_PTR(-EINVAL);
+ }
+
+ ptrace_breakpoint_init(&attr);
+
+ /*
+ * Initialise fields to sane defaults
+ * (i.e. values that will pass validation).
+ */
+ attr.bp_addr = 0;
+ attr.bp_len = HW_BREAKPOINT_LEN_4;
+ attr.bp_type = type;
+ attr.disabled = 1;
+
+ bp = register_user_hw_breakpoint(&attr, riscv_ptrace_hbptriggered, NULL, tsk);
+ if (IS_ERR(bp))
+ return bp;
+
+ err = ptrace_hbp_set_event(hbp_type, tsk, idx, bp);
+ if (err) {
+ /* Drop the event if it cannot be published in the task slot. */
+ unregister_hw_breakpoint(bp);
+ return ERR_PTR(err);
+ }
+
+ return bp;
+}
+
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
+
static struct user_regset riscv_user_regset[] __ro_after_init = {
[REGSET_X] = {
USER_REGSET_NOTE_TYPE(PRSTATUS),
@@ -549,12 +697,173 @@ void ptrace_disable(struct task_struct *child)
{
}
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+static int riscv_ptrace_bp_type_to_hbp(u64 type, enum riscv_hbp_type *hbp_type)
+{
+ switch (type) {
+ case HW_BREAKPOINT_X:
+ *hbp_type = RISCV_HBP_BREAK;
+ return 0;
+ case HW_BREAKPOINT_R:
+ case HW_BREAKPOINT_W:
+ case HW_BREAKPOINT_RW:
+ *hbp_type = RISCV_HBP_WATCH;
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int riscv_ptrace_bp_clear_event(struct task_struct *child,
+ enum riscv_hbp_type hbp_type,
+ unsigned long idx)
+{
+ struct perf_event *bp = ptrace_hbp_get_event(hbp_type, child, idx);
+
+ if (IS_ERR(bp))
+ return PTR_ERR(bp);
+ if (!bp)
+ return 0;
+
+ unregister_hw_breakpoint(bp);
+ return ptrace_hbp_set_event(hbp_type, child, idx, NULL);
+}
+
+static int riscv_ptrace_bp_get(struct task_struct *child, unsigned long idx,
+ struct __riscv_hwdebug_state *state)
+{
+ struct perf_event *bp;
+
+ if (idx >= RISCV_MAX_BP)
+ return -EINVAL;
+
+ idx = array_index_nospec(idx, RISCV_MAX_BP);
+
+ /*
+ * PTRACE_GETHBPREGS is a compact view keyed only by index. The real
+ * storage is split by type, so search both arrays.
+ */
+ bp = ptrace_hbp_get_event(RISCV_HBP_BREAK, child, idx);
+ if (IS_ERR(bp))
+ return PTR_ERR(bp);
+ if (!bp) {
+ bp = ptrace_hbp_get_event(RISCV_HBP_WATCH, child, idx);
+ if (IS_ERR(bp))
+ return PTR_ERR(bp);
+ }
+ if (!bp)
+ return -ENOENT;
+
+ state->addr = bp->attr.bp_addr;
+ state->len = bp->attr.bp_len;
+ state->type = bp->attr.bp_type;
+ state->ctrl = bp->attr.disabled == 1;
+
+ return 0;
+}
+
+static int riscv_ptrace_bp_set(struct task_struct *child, unsigned long idx,
+ struct __riscv_hwdebug_state *state)
+{
+ struct perf_event *bp;
+ struct perf_event_attr attr;
+ enum riscv_hbp_type hbp_type, other_hbp_type;
+ int ret;
+ bool created = false;
+
+ if (idx >= RISCV_MAX_BP)
+ return -EINVAL;
+
+ idx = array_index_nospec(idx, RISCV_MAX_BP);
+
+ ret = riscv_ptrace_bp_type_to_hbp(state->type, &hbp_type);
+ if (ret)
+ return ret;
+
+ /* The legacy request exposes ctrl as a disabled/enabled boolean. */
+ if (state->ctrl > 1)
+ return -EINVAL;
+
+ other_hbp_type = hbp_type == RISCV_HBP_BREAK ?
+ RISCV_HBP_WATCH : RISCV_HBP_BREAK;
+
+ bp = ptrace_hbp_get_event(hbp_type, child, idx);
+ if (IS_ERR(bp))
+ return PTR_ERR(bp);
+ if (!bp) {
+ bp = ptrace_hbp_create(hbp_type, child, idx);
+ if (IS_ERR(bp))
+ return PTR_ERR(bp);
+ created = true;
+ }
+
+ attr = bp->attr;
+
+ attr.bp_addr = state->addr;
+ attr.bp_len = state->len;
+ attr.bp_type = state->type;
+ /* Enable or disable as requested by ctrl (0 = enabled, 1 = disabled) */
+ attr.disabled = state->ctrl == 1;
+ ret = modify_user_hw_breakpoint(bp, &attr);
+ if (ret) {
+ /* Drop only events allocated by this failed request. */
+ if (created)
+ riscv_ptrace_bp_clear_event(child, hbp_type, idx);
+ return ret;
+ }
+
+ /*
+ * If the same index changed from breakpoint to watchpoint, or back,
+ * clear the old opposite-type slot after the new event is valid so a
+ * failed update does not destroy the previous state.
+ */
+ return riscv_ptrace_bp_clear_event(child, other_hbp_type, idx);
+}
+
+static long riscv_ptrace_gethbpregs(struct task_struct *child, unsigned long idx,
+ unsigned long __user *datap)
+{
+ struct __riscv_hwdebug_state state;
+ long ret;
+
+ ret = riscv_ptrace_bp_get(child, idx, &state);
+ if (ret)
+ return ret;
+ if (copy_to_user(datap, &state, sizeof(state)))
+ return -EFAULT;
+
+ return 0;
+}
+
+static long riscv_ptrace_sethbpregs(struct task_struct *child, unsigned long idx,
+ unsigned long __user *datap)
+{
+ struct __riscv_hwdebug_state state;
+
+ if (copy_from_user(&state, datap, sizeof(state)))
+ return -EFAULT;
+
+ return riscv_ptrace_bp_set(child, idx, &state);
+}
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
+
long arch_ptrace(struct task_struct *child, long request,
unsigned long addr, unsigned long data)
{
long ret = -EIO;
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+ unsigned long __user *datap = (unsigned long __user *)data;
+#endif
switch (request) {
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+ case PTRACE_GETHBPREGS:
+ ret = riscv_ptrace_gethbpregs(child, addr, datap);
+ break;
+ case PTRACE_SETHBPREGS:
+ ret = riscv_ptrace_sethbpregs(child, addr, datap);
+ break;
+#endif
default:
ret = ptrace_request(child, request, addr, data);
break;
@@ -623,8 +932,20 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
compat_ulong_t caddr, compat_ulong_t cdata)
{
long ret = -EIO;
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+ /* cdata is a compat user pointer for the legacy HBP requests. */
+ unsigned long __user *datap = compat_ptr(cdata);
+#endif
switch (request) {
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+ case PTRACE_GETHBPREGS:
+ ret = riscv_ptrace_gethbpregs(child, caddr, datap);
+ break;
+ case PTRACE_SETHBPREGS:
+ ret = riscv_ptrace_sethbpregs(child, caddr, datap);
+ break;
+#endif
default:
ret = compat_ptrace_request(child, request, caddr, cdata);
break;
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v8 3/8] riscv: add ptrace requests for hardware breakpoints
2026-11-07 2:39 ` [PATCH v8 3/8] riscv: add ptrace requests for hardware breakpoints Himanshu Chauhan
@ 2026-11-07 7:04 ` kernel test robot
2026-11-08 2:41 ` sashiko-bot
1 sibling, 0 replies; 17+ messages in thread
From: kernel test robot @ 2026-11-07 7:04 UTC (permalink / raw)
To: Himanshu Chauhan, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-riscv
Cc: oe-kbuild-all, Alexandre Ghiti, Jesse Taube, Oleg Nesterov,
Kees Cook, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-kernel,
linux-mm, linux-perf-users, linux-kselftest, Himanshu Chauhan
Hi Himanshu,
kernel test robot noticed the following build errors:
[auto build test ERROR on shuah-kselftest/next]
[also build test ERROR on shuah-kselftest/fixes perf-tools-next/perf-tools-next linus/master v7.4-rc1 next-20261105]
[cannot apply to kees/for-next/execve tip/perf/core perf-tools/perf-tools acme/perf/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Himanshu-Chauhan/riscv-Introduce-support-for-hardware-break-watchpoints/20261107-080910
base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next
patch link: https://lore.kernel.org/r/20261107023917.40606-4-himanshu.chauhan%40oss.qualcomm.com
patch subject: [PATCH v8 3/8] riscv: add ptrace requests for hardware breakpoints
config: riscv-allnoconfig (https://download.01.org/0day-ci/archive/20261107/202611071408.gyIRvq10-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 16.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20261107/202611071408.gyIRvq10-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202611071408.gyIRvq10-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from kernel/exit.c:57:
>> include/linux/hw_breakpoint.h:137:20: error: redefinition of 'flush_ptrace_hw_breakpoint'
137 | static inline void flush_ptrace_hw_breakpoint(struct task_struct *tsk) { }
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from arch/riscv/include/asm/processor.h:18,
from arch/riscv/include/asm/cmpxchg.h:16,
from arch/riscv/include/asm/barrier.h:14,
from include/asm-generic/bitops/generic-non-atomic.h:7,
from include/linux/bitops.h:28,
from include/linux/thread_info.h:27,
from include/asm-generic/preempt.h:5,
from ./arch/riscv/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:107,
from include/linux/spinlock.h:56,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/mm.h:8,
from kernel/exit.c:8:
arch/riscv/include/asm/hw_breakpoint.h:58:20: note: previous definition of 'flush_ptrace_hw_breakpoint' with type 'void(struct task_struct *)'
58 | static inline void flush_ptrace_hw_breakpoint(struct task_struct *task) { }
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/flush_ptrace_hw_breakpoint +137 include/linux/hw_breakpoint.h
24f1e32c60c45c include/linux/hw_breakpoint.h Frederic Weisbecker 2009-09-09 136
24f1e32c60c45c include/linux/hw_breakpoint.h Frederic Weisbecker 2009-09-09 @137 static inline void flush_ptrace_hw_breakpoint(struct task_struct *tsk) { }
b332828c39326b include/asm-generic/hw_breakpoint.h K.Prasad 2009-06-01 138
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v8 3/8] riscv: add ptrace requests for hardware breakpoints
2026-11-07 2:39 ` [PATCH v8 3/8] riscv: add ptrace requests for hardware breakpoints Himanshu Chauhan
2026-11-07 7:04 ` kernel test robot
@ 2026-11-08 2:41 ` sashiko-bot
1 sibling, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-11-08 2:41 UTC (permalink / raw)
To: Himanshu Chauhan, Paul Walmsley, Palmer Dabbelt, Albert Ou
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-mm,
linux-perf-users
Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] riscv: ptrace: PTRACE_GETHBPREGS/SETHBPREGS collide with
generic SUD requests
- [High] riscv: hw_breakpoint: duplicate flush_ptrace_hw_breakpoint()
stub breaks build
- [Medium] riscv: ptrace: switching slot type in riscv_ptrace_bp_set()
fails with -ENOSPC
- [Medium] riscv: ptrace: sleeping lock in riscv_ptrace_hbptriggered()
with IRQs off
- [Low] riscv: unused clear_ptrace_hw_breakpoint() prototype and
debug_info fields
--
commit 99a21b5687d9bebf111bc51526d0fe5d8bd289d6
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
riscv: add ptrace requests for hardware breakpoints
This commit adds support for the PTRACE_GETHBPREGS and PTRACE_SETHBPREGS
ptrace requests on RISC-V, storing per-task perf hardware breakpoint events
and reporting hits via SIGTRAP/TRAP_HWBKPT.
> diff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h
> index dcce20ba3faab..6e8913d69df23 100644
> --- a/arch/riscv/include/asm/hw_breakpoint.h
> +++ b/arch/riscv/include/asm/hw_breakpoint.h
[ ... ]
> @@ -42,8 +49,14 @@ 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);
> +void clear_ptrace_hw_breakpoint(struct task_struct *tsk);
> +void flush_ptrace_hw_breakpoint(struct task_struct *tsk);
> +void ptrace_hw_copy_thread(struct task_struct *task);
>
> #else
>
> +static inline void flush_ptrace_hw_breakpoint(struct task_struct *task) { }
> +static inline void ptrace_hw_copy_thread(struct task_struct *task) { }
> +
> #endif /* CONFIG_HAVE_HW_BREAKPOINT */
> #endif /* __RISCV_HW_BREAKPOINT_H */
[Severity: High]
Does defining flush_ptrace_hw_breakpoint() here break the build when
CONFIG_HAVE_HW_BREAKPOINT is disabled?
include/linux/hw_breakpoint.h already defines a static inline stub for
flush_ptrace_hw_breakpoint() when CONFIG_HAVE_HW_BREAKPOINT is not set
(and declares the prototype when it is set). Since asm/processor.h now
includes asm/hw_breakpoint.h, any translation unit including both
linux/sched.h and linux/hw_breakpoint.h (such as arch/riscv/kernel/ptrace.c)
fails to compile due to the redefinition.
> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> index e9d339bcb7e9f..a124ea48d8058 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
[ ... ]
> @@ -111,6 +112,19 @@ struct pt_regs;
> #define RISCV_PREEMPT_V_NEED_RESTORE 0x40000000
> #define RISCV_PREEMPT_V_IN_SCHEDULE 0x20000000
>
> +struct debug_info {
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> + /* Have we suspended stepping by a debugger? */
> + int suspended_step;
> + /* Allow breakpoints and watchpoints to be disabled for this thread. */
> + int bp_disabled;
> + int wp_disabled;
> + /* Hardware breakpoints pinned to this task. */
> + struct perf_event *hbp_break[RISCV_MAX_BP];
> + struct perf_event *hbp_watch[RISCV_MAX_BP];
> +#endif
> +};
[Severity: Low]
This isn't a bug, but suspended_step, bp_disabled, and wp_disabled in
struct debug_info, along with the clear_ptrace_hw_breakpoint() prototype in
asm/hw_breakpoint.h, are never used in this patch or later in the series.
> diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
> index 3de2b7124aff3..373a1c14f2073 100644
> --- a/arch/riscv/include/uapi/asm/ptrace.h
> +++ b/arch/riscv/include/uapi/asm/ptrace.h
> @@ -15,6 +15,8 @@
>
> #define PTRACE_GETFDPIC_EXEC 0
> #define PTRACE_GETFDPIC_INTERP 1
> +#define PTRACE_GETHBPREGS 0x4210
> +#define PTRACE_SETHBPREGS 0x4211
[Severity: High]
As reported by Rui Qi on v6 [1], do these values collide with the generic
PTRACE_SET_SYSCALL_USER_DISPATCH_CONFIG (0x4210) and
PTRACE_GET_SYSCALL_USER_DISPATCH_CONFIG (0x4211) requests in
include/uapi/linux/ptrace.h, where 0x4200-0x4300 is reserved for
architecture-independent additions?
Because arch_ptrace() and compat_arch_ptrace() intercept 0x4210 and 0x4211
before calling ptrace_request(), a tracer calling
PTRACE_SET_SYSCALL_USER_DISPATCH_CONFIG when CONFIG_HAVE_HW_BREAKPOINT=y is
routed to riscv_ptrace_gethbpregs(), which overwrites the caller's input
buffer via copy_to_user(), while PTRACE_GET_SYSCALL_USER_DISPATCH_CONFIG is
routed to riscv_ptrace_sethbpregs().
> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> index a0055743d3ba5..be076865d02d0 100644
> --- a/arch/riscv/kernel/ptrace.c
> +++ b/arch/riscv/kernel/ptrace.c
[ ... ]
> @@ -380,6 +382,152 @@ static int riscv_cfi_set(struct task_struct *target,
> }
> #endif
>
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +enum riscv_hbp_type {
> + RISCV_HBP_BREAK,
> + RISCV_HBP_WATCH,
> +};
> +
> +/*
> + * Handle hitting a HW-breakpoint.
> + */
> +static void riscv_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);
> +}
[Severity: Medium]
Can calling force_sig_fault() from riscv_ptrace_hbptriggered() sleep with
interrupts disabled on PREEMPT_RT?
When a breakpoint triggers, hw_breakpoint_handler() -> perf_bp_event() ->
perf_swevent_event() disables hardware interrupts via guard(irqsave)()
before invoking the overflow handler:
perf_swevent_event() [guard(irqsave)()]
-> perf_swevent_overflow()
-> __perf_event_overflow()
-> riscv_ptrace_hbptriggered()
-> force_sig_fault()
-> force_sig_info_to_task()
-> spin_lock_irqsave(&t->sighand->siglock, flags)
On PREEMPT_RT (RISC-V selects ARCH_SUPPORTS_RT), sighand->siglock is a
sleeping rt_spin_lock, so acquiring it with interrupts disabled triggers a
sleeping-in-atomic warning and blocks with IRQs off if contended.
[ ... ]
> @@ -549,12 +697,173 @@ void ptrace_disable(struct task_struct *child)
[ ... ]
> +static int riscv_ptrace_bp_set(struct task_struct *child, unsigned long idx,
> + struct __riscv_hwdebug_state *state)
> +{
> + struct perf_event *bp;
> + struct perf_event_attr attr;
> + enum riscv_hbp_type hbp_type, other_hbp_type;
> + int ret;
> + bool created = false;
> +
> + if (idx >= RISCV_MAX_BP)
> + return -EINVAL;
> +
> + idx = array_index_nospec(idx, RISCV_MAX_BP);
> +
> + ret = riscv_ptrace_bp_type_to_hbp(state->type, &hbp_type);
> + if (ret)
> + return ret;
> +
> + /* The legacy request exposes ctrl as a disabled/enabled boolean. */
> + if (state->ctrl > 1)
> + return -EINVAL;
> +
> + other_hbp_type = hbp_type == RISCV_HBP_BREAK ?
> + RISCV_HBP_WATCH : RISCV_HBP_BREAK;
> +
> + bp = ptrace_hbp_get_event(hbp_type, child, idx);
> + if (IS_ERR(bp))
> + return PTR_ERR(bp);
> + if (!bp) {
> + bp = ptrace_hbp_create(hbp_type, child, idx);
> + if (IS_ERR(bp))
> + return PTR_ERR(bp);
> + created = true;
> + }
> +
> + attr = bp->attr;
> +
> + attr.bp_addr = state->addr;
> + attr.bp_len = state->len;
> + attr.bp_type = state->type;
> + /* Enable or disable as requested by ctrl (0 = enabled, 1 = disabled) */
> + attr.disabled = state->ctrl == 1;
> + ret = modify_user_hw_breakpoint(bp, &attr);
> + if (ret) {
> + /* Drop only events allocated by this failed request. */
> + if (created)
> + riscv_ptrace_bp_clear_event(child, hbp_type, idx);
> + return ret;
> + }
> +
> + /*
> + * If the same index changed from breakpoint to watchpoint, or back,
> + * clear the old opposite-type slot after the new event is valid so a
> + * failed update does not destroy the previous state.
> + */
> + return riscv_ptrace_bp_clear_event(child, other_hbp_type, idx);
> +}
[Severity: Medium]
Can switching an existing slot between a breakpoint and a watchpoint fail
with -ENOSPC when all hardware triggers are already in use?
RISC-V selects CONFIG_HAVE_MIXED_BREAKPOINTS_REGS, so breakpoints and
watchpoints share a single pool of dbtr_total_num slots in
reserve_bp_slot(). Because debug_info splits storage into hbp_break[] and
hbp_watch[], changing the type at index idx causes ptrace_hbp_get_event()
to return NULL and ptrace_hbp_create() to allocate a second perf_event
from the shared pool before riscv_ptrace_bp_clear_event() frees the
existing event at the same index. Modifying the existing perf_event in
place via modify_user_hw_breakpoint() (or storing unified trigger slots in
a single array) would avoid needing a spare hardware slot.
[1] https://lore.kernel.org/r/5a7c08bd-4313-4424-b649-a19ffe5242e4@bytedance.com
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261107023917.40606-1-himanshu.chauhan@oss.qualcomm.com?part=3
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v8 4/8] riscv: add regset access for hardware breakpoints
2026-11-07 2:39 [PATCH v8 0/8] riscv: Introduce support for hardware break/watchpoints Himanshu Chauhan
` (2 preceding siblings ...)
2026-11-07 2:39 ` [PATCH v8 3/8] riscv: add ptrace requests for hardware breakpoints Himanshu Chauhan
@ 2026-11-07 2:39 ` Himanshu Chauhan
2026-11-08 2:41 ` sashiko-bot
2026-11-07 2:39 ` [PATCH v8 5/8] selftests/breakpoints: extend riscv test for ptrace hw break/watchpoints Himanshu Chauhan
` (3 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Himanshu Chauhan @ 2026-11-07 2:39 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-kernel,
linux-mm, linux-perf-users, linux-kselftest, Himanshu Chauhan
Add NT_RISCV_HW_BREAK and NT_RISCV_HW_WATCH. A tracer can use
PTRACE_GETREGSET and PTRACE_SETREGSET to read or program the RISC-V
hardware breakpoint and watchpoint slots.
The regset layout uses fixed-width address and control fields. This
keeps the native and compat layouts the same, because the UAPI fields do
not depend on the task word size. Reuse the ptrace-owned perf event
backend from the previous patch.
Signed-off-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
---
arch/riscv/include/uapi/asm/ptrace.h | 41 ++++
arch/riscv/kernel/ptrace.c | 340 +++++++++++++++++++++++++++
include/uapi/linux/elf.h | 4 +
tools/include/uapi/linux/elf.h | 2 +
4 files changed, 387 insertions(+)
diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h
index 373a1c14f207..c6beb7b287fe 100644
--- a/arch/riscv/include/uapi/asm/ptrace.h
+++ b/arch/riscv/include/uapi/asm/ptrace.h
@@ -10,6 +10,7 @@
#include <linux/types.h>
#include <linux/const.h>
+#include <linux/bits.h>
#define PTRACE_GETFDPIC 33
@@ -166,6 +167,46 @@ struct user_cfi_state {
__u64 shstk_ptr;
};
+/*
+ * bit[3:0] Match
+ * bit[8:4] Size
+ * bit[11:9] When
+ * bit[12] Select
+ * bit[13] Chain
+ * bit[16:14] Type
+ * bit[XLEN-1:17] Reserved
+ */
+#define HWDEBUG_MATCH_MASK __GENMASK(3, 0)
+#define HWDEBUG_SIZE_MASK __GENMASK(8, 4)
+#define HWDEBUG_WHEN_MASK __GENMASK(11, 9)
+#define HWDEBUG_SELECT_MASK _BITUL(12)
+#define HWDEBUG_CHAIN_MASK _BITUL(13)
+#define HWDEBUG_TYPE_MASK __GENMASK(16, 14)
+
+#define HWDEBUG_MATCH(_ctrl) (((_ctrl) & HWDEBUG_MATCH_MASK) >> 0)
+#define HWDEBUG_SIZE(_ctrl) (((_ctrl) & HWDEBUG_SIZE_MASK) >> 4)
+#define HWDEBUG_WHEN(_ctrl) (((_ctrl) & HWDEBUG_WHEN_MASK) >> 9)
+#define HWDEBUG_SELECT(_ctrl) (((_ctrl) & HWDEBUG_SELECT_MASK) >> 12)
+#define HWDEBUG_CHAIN(_ctrl) (((_ctrl) & HWDEBUG_CHAIN_MASK) >> 13)
+#define HWDEBUG_TYPE(_ctrl) (((_ctrl) & HWDEBUG_TYPE_MASK) >> 14)
+
+#define HWDEBUG_MK_MATCH(_match) (((_match) << 0) & HWDEBUG_MATCH_MASK)
+#define HWDEBUG_MK_SIZE(_sz) (((_sz) << 4) & HWDEBUG_SIZE_MASK)
+#define HWDEBUG_MK_WHEN(_when) (((_when) << 9) & HWDEBUG_WHEN_MASK)
+#define HWDEBUG_MK_SELECT(_sel) (((_sel) << 12) & HWDEBUG_SELECT_MASK)
+#define HWDEBUG_MK_CHAIN(_chain) (((_chain) << 13) & HWDEBUG_CHAIN_MASK)
+#define HWDEBUG_MK_TYPE(_type) (((_type) << 14) & HWDEBUG_TYPE_MASK)
+
+struct user_hwdebug_state {
+ __u32 info;
+ __u32 pad;
+ struct {
+ __u64 addr;
+ __u32 control;
+ __u32 pad;
+ } dbg_regs[16];
+};
+
struct __riscv_hwdebug_state {
__u64 addr;
__u64 type;
diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
index be076865d02d..9959a8e4a307 100644
--- a/arch/riscv/kernel/ptrace.c
+++ b/arch/riscv/kernel/ptrace.c
@@ -37,6 +37,10 @@ enum riscv_regset {
#ifdef CONFIG_RISCV_USER_CFI
REGSET_CFI,
#endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+ REGSET_RISCV_HW_BREAK,
+ REGSET_RISCV_HW_WATCH,
+#endif
};
static int riscv_gpr_get(struct task_struct *target,
@@ -526,6 +530,302 @@ static struct perf_event *ptrace_hbp_create(enum riscv_hbp_type hbp_type,
return bp;
}
+static int ptrace_hbp_note_to_type(unsigned int note_type,
+ enum riscv_hbp_type *hbp_type)
+{
+ switch (note_type) {
+ case NT_RISCV_HW_BREAK:
+ *hbp_type = RISCV_HBP_BREAK;
+ return 0;
+ case NT_RISCV_HW_WATCH:
+ *hbp_type = RISCV_HBP_WATCH;
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ptrace_hbp_fill_attr_ctrl(enum riscv_hbp_type hbp_type,
+ struct arch_hw_breakpoint *bpctrl,
+ struct perf_event_attr *attr)
+{
+ int len, type;
+
+ attr->disabled = 0;
+ type = bpctrl->type;
+ len = bpctrl->len;
+
+ switch (hbp_type) {
+ case RISCV_HBP_BREAK:
+ if ((type & HW_BREAKPOINT_X) != type)
+ return -EINVAL;
+ break;
+ case RISCV_HBP_WATCH:
+ if ((type & HW_BREAKPOINT_RW) != type)
+ return -EINVAL;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ attr->bp_len = len;
+ attr->bp_type = type;
+ attr->bp_addr = bpctrl->address;
+
+ return 0;
+}
+
+static int ptrace_hbp_get_resource_info(enum riscv_hbp_type hbp_type, u32 *info)
+{
+ u8 num;
+
+ /*
+ * The NT_RISCV_HW_BREAK/WATCH regset ABI exposes 16 register pairs.
+ * Cap the reported count so GETREGSET/SETREGSET never walk past the
+ * fixed UAPI array even if the platform has more triggers.
+ */
+ switch (hbp_type) {
+ case RISCV_HBP_BREAK:
+ num = min_t(unsigned int, hw_breakpoint_slots(TYPE_INST),
+ RISCV_MAX_BP);
+ break;
+ case RISCV_HBP_WATCH:
+ num = min_t(unsigned int, hw_breakpoint_slots(TYPE_DATA),
+ RISCV_MAX_BP);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ *info = num;
+
+ return 0;
+}
+
+static u32 encode_ctrl_reg(struct perf_event *bp)
+{
+ struct arch_hw_breakpoint *bpctrl = counter_arch_bp(bp);
+ u32 ctrl = 0;
+
+ /* Expose the generic UAPI bp_type values in ptrace control bits. */
+ ctrl |= HWDEBUG_MK_TYPE(bp->attr.bp_type);
+ ctrl |= HWDEBUG_MK_MATCH(bpctrl->match);
+ ctrl |= HWDEBUG_MK_SELECT(bpctrl->select);
+ ctrl |= HWDEBUG_MK_WHEN(bpctrl->time);
+ ctrl |= HWDEBUG_MK_SIZE(bp->attr.bp_len);
+ ctrl |= HWDEBUG_MK_CHAIN(bpctrl->chain);
+
+ return ctrl;
+}
+
+static int ptrace_hbp_get_ctrl(enum riscv_hbp_type hbp_type,
+ struct task_struct *tsk,
+ unsigned long idx,
+ u32 *ctrl)
+{
+ struct perf_event *bp = ptrace_hbp_get_event(hbp_type, tsk, idx);
+
+ if (IS_ERR(bp))
+ return PTR_ERR(bp);
+
+ *ctrl = bp ? encode_ctrl_reg(bp) : 0;
+ return 0;
+}
+
+static int ptrace_hbp_get_addr(enum riscv_hbp_type hbp_type,
+ struct task_struct *tsk,
+ unsigned long idx,
+ u64 *addr)
+{
+ struct perf_event *bp = ptrace_hbp_get_event(hbp_type, tsk, idx);
+
+ if (IS_ERR(bp))
+ return PTR_ERR(bp);
+
+ *addr = bp ? counter_arch_bp(bp)->address : 0;
+ return 0;
+}
+
+static struct perf_event *ptrace_hbp_get_initialised_bp(enum riscv_hbp_type hbp_type,
+ struct task_struct *tsk,
+ unsigned long idx)
+{
+ struct perf_event *bp = ptrace_hbp_get_event(hbp_type, tsk, idx);
+
+ if (!bp)
+ bp = ptrace_hbp_create(hbp_type, tsk, idx);
+
+ return bp;
+}
+
+static void decode_ctrl_reg(u32 uctrl, struct arch_hw_breakpoint *bpctrl)
+{
+ bpctrl->type = HWDEBUG_TYPE(uctrl);
+ bpctrl->match = HWDEBUG_MATCH(uctrl);
+ bpctrl->select = HWDEBUG_SELECT(uctrl);
+ bpctrl->time = HWDEBUG_WHEN(uctrl);
+ bpctrl->len = HWDEBUG_SIZE(uctrl);
+ bpctrl->chain = HWDEBUG_CHAIN(uctrl);
+}
+
+static int ptrace_hbp_set_ctrl(enum riscv_hbp_type hbp_type,
+ struct task_struct *tsk,
+ unsigned long idx,
+ u32 uctrl)
+{
+ int err;
+ struct perf_event *bp;
+ struct perf_event_attr attr;
+ struct arch_hw_breakpoint bpctrl;
+
+ bp = ptrace_hbp_get_initialised_bp(hbp_type, tsk, idx);
+ if (IS_ERR(bp)) {
+ err = PTR_ERR(bp);
+ return err;
+ }
+
+ attr = bp->attr;
+ decode_ctrl_reg(uctrl, &bpctrl);
+ bpctrl.address = attr.bp_addr;
+ err = ptrace_hbp_fill_attr_ctrl(hbp_type, &bpctrl, &attr);
+ if (err)
+ return err;
+
+ return modify_user_hw_breakpoint(bp, &attr);
+}
+
+static int ptrace_hbp_set_addr(enum riscv_hbp_type hbp_type,
+ struct task_struct *tsk,
+ unsigned long idx,
+ u64 addr)
+{
+ int err;
+ struct perf_event *bp;
+ struct perf_event_attr attr;
+
+ bp = ptrace_hbp_get_initialised_bp(hbp_type, tsk, idx);
+ if (IS_ERR(bp)) {
+ err = PTR_ERR(bp);
+ return err;
+ }
+
+ attr = bp->attr;
+ attr.bp_addr = addr;
+ err = modify_user_hw_breakpoint(bp, &attr);
+ return err;
+}
+
+#define PTRACE_HBP_ADDR_SZ sizeof(u64)
+#define PTRACE_HBP_CTRL_SZ sizeof(u32)
+#define PTRACE_HBP_PAD_SZ sizeof(u32)
+
+static int riscv_hw_break_get(struct task_struct *target,
+ const struct user_regset *regset,
+ struct membuf to)
+{
+ unsigned int note_type = regset->core_note_type;
+ enum riscv_hbp_type hbp_type;
+ int ret, idx, num_slots;
+ u32 info, ctrl;
+ u64 addr;
+
+ ret = ptrace_hbp_note_to_type(note_type, &hbp_type);
+ if (ret)
+ return ret;
+
+ /* Resource info: number of available slots */
+ ret = ptrace_hbp_get_resource_info(hbp_type, &info);
+ if (ret)
+ return ret;
+
+ membuf_write(&to, &info, sizeof(info));
+ membuf_zero(&to, sizeof(u32));
+
+ /* Emit one (address, ctrl, pad) entry per available slot */
+ num_slots = (int)info;
+ for (idx = 0; idx < num_slots; idx++) {
+ ret = ptrace_hbp_get_addr(hbp_type, target, idx, &addr);
+ if (ret)
+ return ret;
+ ret = ptrace_hbp_get_ctrl(hbp_type, target, idx, &ctrl);
+ if (ret)
+ return ret;
+ membuf_store(&to, addr);
+ membuf_store(&to, ctrl);
+ membuf_zero(&to, sizeof(u32));
+ }
+ return 0;
+}
+
+static int riscv_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)
+{
+ unsigned int note_type = regset->core_note_type;
+ enum riscv_hbp_type hbp_type;
+ int ret, idx = 0, offset, limit;
+ u32 ctrl;
+ u64 addr;
+
+ ret = ptrace_hbp_note_to_type(note_type, &hbp_type);
+ if (ret)
+ return ret;
+
+ /* Resource info and pad */
+ offset = offsetof(struct user_hwdebug_state, dbg_regs);
+ user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf, 0, offset);
+
+ /* (address, ctrl) registers */
+ limit = regset->n * regset->size;
+ while (count && offset < limit) {
+ if (count < PTRACE_HBP_ADDR_SZ)
+ return -EINVAL;
+
+ /* user_regset_copyin() can do partial copies for odd offsets. */
+ addr = 0;
+ ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &addr,
+ offset, offset + PTRACE_HBP_ADDR_SZ);
+ if (ret)
+ return ret;
+
+ ret = ptrace_hbp_set_addr(hbp_type, target, idx, addr);
+ if (ret)
+ return ret;
+
+ offset += PTRACE_HBP_ADDR_SZ;
+
+ if (!count)
+ break;
+
+ /*
+ * Reject undersized control writes, and keep ctrl initialized in
+ * case user_regset_copyin() still performs a partial copy.
+ */
+ if (count < PTRACE_HBP_CTRL_SZ)
+ return -EINVAL;
+
+ ctrl = 0;
+ ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &ctrl,
+ offset, offset + PTRACE_HBP_CTRL_SZ);
+ if (ret)
+ return ret;
+
+ ret = ptrace_hbp_set_ctrl(hbp_type, target, idx, ctrl);
+ if (ret)
+ return ret;
+
+ offset += PTRACE_HBP_CTRL_SZ;
+
+ user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
+ offset, offset + PTRACE_HBP_PAD_SZ);
+ offset += PTRACE_HBP_PAD_SZ;
+ idx++;
+ }
+
+ return 0;
+}
+
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
static struct user_regset riscv_user_regset[] __ro_after_init = {
@@ -577,6 +877,24 @@ static struct user_regset riscv_user_regset[] __ro_after_init = {
.set = riscv_cfi_set,
},
#endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+ [REGSET_RISCV_HW_BREAK] = {
+ USER_REGSET_NOTE_TYPE(RISCV_HW_BREAK),
+ .n = sizeof(struct user_hwdebug_state) / sizeof(u32),
+ .size = sizeof(u32),
+ .align = sizeof(u32),
+ .regset_get = riscv_hw_break_get,
+ .set = riscv_hw_break_set,
+ },
+ [REGSET_RISCV_HW_WATCH] = {
+ USER_REGSET_NOTE_TYPE(RISCV_HW_WATCH),
+ .n = sizeof(struct user_hwdebug_state) / sizeof(u32),
+ .size = sizeof(u32),
+ .align = sizeof(u32),
+ .regset_get = riscv_hw_break_get,
+ .set = riscv_hw_break_set,
+ },
+#endif
};
static const struct user_regset_view riscv_user_native_view = {
@@ -919,6 +1237,28 @@ static const struct user_regset compat_riscv_user_regset[] = {
.set = riscv_fpr_set,
},
#endif
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+ /*
+ * The hardware-debug regset uses fixed-width UAPI fields, so the
+ * native layout is also the compat layout.
+ */
+ [REGSET_RISCV_HW_BREAK] = {
+ USER_REGSET_NOTE_TYPE(RISCV_HW_BREAK),
+ .n = sizeof(struct user_hwdebug_state) / sizeof(u32),
+ .size = sizeof(u32),
+ .align = sizeof(u32),
+ .regset_get = riscv_hw_break_get,
+ .set = riscv_hw_break_set,
+ },
+ [REGSET_RISCV_HW_WATCH] = {
+ USER_REGSET_NOTE_TYPE(RISCV_HW_WATCH),
+ .n = sizeof(struct user_hwdebug_state) / sizeof(u32),
+ .size = sizeof(u32),
+ .align = sizeof(u32),
+ .regset_get = riscv_hw_break_get,
+ .set = riscv_hw_break_set,
+ },
+#endif
};
static const struct user_regset_view compat_riscv_user_native_view = {
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index ee30dcd80901..1315ac35157c 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -547,6 +547,10 @@ typedef struct elf64_shdr {
#define NT_RISCV_TAGGED_ADDR_CTRL 0x902 /* RISC-V tagged address control (prctl()) */
#define NN_RISCV_USER_CFI "LINUX"
#define NT_RISCV_USER_CFI 0x903 /* RISC-V shadow stack state */
+#define NN_RISCV_HW_BREAK "LINUX"
+#define NT_RISCV_HW_BREAK 0x904 /* RISC-V hardware breakpoint registers */
+#define NN_RISCV_HW_WATCH "LINUX"
+#define NT_RISCV_HW_WATCH 0x905 /* RISCV-V hardware watchpoint registers */
#define NN_LOONGARCH_CPUCFG "LINUX"
#define NT_LOONGARCH_CPUCFG 0xa00 /* LoongArch CPU config registers */
#define NN_LOONGARCH_CSR "LINUX"
diff --git a/tools/include/uapi/linux/elf.h b/tools/include/uapi/linux/elf.h
index 5834b83d7f9a..21f225502051 100644
--- a/tools/include/uapi/linux/elf.h
+++ b/tools/include/uapi/linux/elf.h
@@ -460,6 +460,8 @@ typedef struct elf64_shdr {
#define NT_RISCV_CSR 0x900 /* RISC-V Control and Status Registers */
#define NT_RISCV_VECTOR 0x901 /* RISC-V vector registers */
#define NT_RISCV_TAGGED_ADDR_CTRL 0x902 /* RISC-V tagged address control (prctl()) */
+#define NT_RISCV_HW_BREAK 0x904
+#define NT_RISCV_HW_WATCH 0x905
#define NT_LOONGARCH_CPUCFG 0xa00 /* LoongArch CPU config registers */
#define NT_LOONGARCH_CSR 0xa01 /* LoongArch control and status registers */
#define NT_LOONGARCH_LSX 0xa02 /* LoongArch Loongson SIMD Extension registers */
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v8 4/8] riscv: add regset access for hardware breakpoints
2026-11-07 2:39 ` [PATCH v8 4/8] riscv: add regset access " Himanshu Chauhan
@ 2026-11-08 2:41 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-11-08 2:41 UTC (permalink / raw)
To: Himanshu Chauhan, Paul Walmsley, Palmer Dabbelt, Albert Ou
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-mm,
linux-perf-users
Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] riscv: ptrace: broken breakpoint disable in
ptrace_hbp_fill_attr_ctrl()
- [Medium] riscv: ptrace: zero-filled holes in compat_riscv_user_regset
cause div-by-zero
- [Medium] riscv: ptrace: ignored control fields in decode_ctrl_reg()
- [Medium] riscv: ptrace: wrong return value and slot bound in
riscv_hw_break_get()
- [Medium] riscv: ptrace: leaked perf_event on error in
riscv_hw_break_set()
--
commit 5940d70e24c6c5b6aa9c305717a9b36acc0903b2
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
riscv: add regset access for hardware breakpoints
This commit adds the NT_RISCV_HW_BREAK and NT_RISCV_HW_WATCH regsets to allow
reading and programming RISC-V hardware breakpoint and watchpoint slots via
PTRACE_GETREGSET and PTRACE_SETREGSET.
> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> index be076865d02d0..9959a8e4a3079 100644
> --- a/arch/riscv/kernel/ptrace.c
> +++ b/arch/riscv/kernel/ptrace.c
> @@ -37,6 +37,10 @@ enum riscv_regset {
> #ifdef CONFIG_RISCV_USER_CFI
> REGSET_CFI,
> #endif
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> + REGSET_RISCV_HW_BREAK,
> + REGSET_RISCV_HW_WATCH,
> +#endif
> };
>
> static int riscv_gpr_get(struct task_struct *target,
> @@ -526,6 +530,302 @@ static struct perf_event *ptrace_hbp_create(enum riscv_hbp_type hbp_type,
> return bp;
> }
>
> +static int ptrace_hbp_note_to_type(unsigned int note_type,
> + enum riscv_hbp_type *hbp_type)
> +{
> + switch (note_type) {
> + case NT_RISCV_HW_BREAK:
> + *hbp_type = RISCV_HBP_BREAK;
> + return 0;
> + case NT_RISCV_HW_WATCH:
> + *hbp_type = RISCV_HBP_WATCH;
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ptrace_hbp_fill_attr_ctrl(enum riscv_hbp_type hbp_type,
> + struct arch_hw_breakpoint *bpctrl,
> + struct perf_event_attr *attr)
> +{
> + int len, type;
> +
> + attr->disabled = 0;
> + type = bpctrl->type;
> + len = bpctrl->len;
> +
> + switch (hbp_type) {
> + case RISCV_HBP_BREAK:
> + if ((type & HW_BREAKPOINT_X) != type)
> + return -EINVAL;
> + break;
> + case RISCV_HBP_WATCH:
> + if ((type & HW_BREAKPOINT_RW) != type)
> + return -EINVAL;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + attr->bp_len = len;
> + attr->bp_type = type;
> + attr->bp_addr = bpctrl->address;
> +
> + return 0;
> +}
[ ... ]
> +static u32 encode_ctrl_reg(struct perf_event *bp)
> +{
> + struct arch_hw_breakpoint *bpctrl = counter_arch_bp(bp);
> + u32 ctrl = 0;
> +
> + /* Expose the generic UAPI bp_type values in ptrace control bits. */
> + ctrl |= HWDEBUG_MK_TYPE(bp->attr.bp_type);
> + ctrl |= HWDEBUG_MK_MATCH(bpctrl->match);
> + ctrl |= HWDEBUG_MK_SELECT(bpctrl->select);
> + ctrl |= HWDEBUG_MK_WHEN(bpctrl->time);
> + ctrl |= HWDEBUG_MK_SIZE(bp->attr.bp_len);
> + ctrl |= HWDEBUG_MK_CHAIN(bpctrl->chain);
> +
> + return ctrl;
> +}
[Severity: High]
As reported by Rui Qi on v6 [1], how can a tracer disable a hardware
breakpoint or watchpoint slot, or write a user_hwdebug_state buffer with
unused zeroed slots, when ptrace_hbp_fill_attr_ctrl() unconditionally sets
attr->disabled = 0?
The control word has no separate enable bit, so an unused or disabled slot
has control == 0 (bpctrl->type == HW_BREAKPOINT_EMPTY). Because (0 & mask) !=
0 is false, ptrace_hbp_fill_attr_ctrl() passes attr->disabled = 0 and
attr->bp_type = 0 to modify_user_hw_breakpoint(), which fails with -EINVAL in
hw_breakpoint_arch_parse() -> rv_init_mcontrol_trigger() while re-enabling an
already-active event at the new address set by ptrace_hbp_set_addr().
Conversely, encode_ctrl_reg() encodes bp->attr.bp_type without checking
bp->attr.disabled, so disabled events in a slot are reported as active by
PTRACE_GETREGSET. When uctrl == 0 (or type == HW_BREAKPOINT_EMPTY),
ptrace_hbp_fill_attr_ctrl() should set attr->disabled = 1 and return early,
and encode_ctrl_reg() should return 0 when bp->attr.disabled is set.
[ ... ]
> +static void decode_ctrl_reg(u32 uctrl, struct arch_hw_breakpoint *bpctrl)
> +{
> + bpctrl->type = HWDEBUG_TYPE(uctrl);
> + bpctrl->match = HWDEBUG_MATCH(uctrl);
> + bpctrl->select = HWDEBUG_SELECT(uctrl);
> + bpctrl->time = HWDEBUG_WHEN(uctrl);
> + bpctrl->len = HWDEBUG_SIZE(uctrl);
> + bpctrl->chain = HWDEBUG_CHAIN(uctrl);
> +}
> +
> +static int ptrace_hbp_set_ctrl(enum riscv_hbp_type hbp_type,
> + struct task_struct *tsk,
> + unsigned long idx,
> + u32 uctrl)
> +{
> + int err;
> + struct perf_event *bp;
> + struct perf_event_attr attr;
> + struct arch_hw_breakpoint bpctrl;
> +
> + bp = ptrace_hbp_get_initialised_bp(hbp_type, tsk, idx);
> + if (IS_ERR(bp)) {
> + err = PTR_ERR(bp);
> + return err;
> + }
> +
> + attr = bp->attr;
> + decode_ctrl_reg(uctrl, &bpctrl);
> + bpctrl.address = attr.bp_addr;
> + err = ptrace_hbp_fill_attr_ctrl(hbp_type, &bpctrl, &attr);
> + if (err)
> + return err;
> +
> + return modify_user_hw_breakpoint(bp, &attr);
> +}
[Severity: Medium]
Does ptrace_hbp_set_ctrl() silently discard the match, select, time, and
chain fields decoded by decode_ctrl_reg(), as well as any non-zero reserved
bits in uctrl?
decode_ctrl_reg() unpacks HWDEBUG_MATCH, HWDEBUG_SELECT, HWDEBUG_WHEN, and
HWDEBUG_CHAIN into the stack-local bpctrl, but ptrace_hbp_fill_attr_ctrl()
only copies len, type, and address into attr, and modify_user_hw_breakpoint()
re-initializes counter_arch_bp(bp) from attr alone. Any non-zero values
written to those fields or to reserved bits [31:17] are accepted without
taking effect and read back as 0 in encode_ctrl_reg(), preventing userspace
from detecting unsupported trigger modes.
> +static int ptrace_hbp_set_addr(enum riscv_hbp_type hbp_type,
> + struct task_struct *tsk,
> + unsigned long idx,
> + u64 addr)
> +{
> + int err;
> + struct perf_event *bp;
> + struct perf_event_attr attr;
> +
> + bp = ptrace_hbp_get_initialised_bp(hbp_type, tsk, idx);
> + if (IS_ERR(bp)) {
> + err = PTR_ERR(bp);
> + return err;
> + }
> +
> + attr = bp->attr;
> + attr.bp_addr = addr;
> + err = modify_user_hw_breakpoint(bp, &attr);
> + return err;
> +}
[Severity: Medium]
Does ptrace_hbp_set_addr() leave a newly created perf_event registered in
tsk->thread.debug when modify_user_hw_breakpoint() or the subsequent control
write in riscv_hw_break_set() fails?
When a slot is initially empty, ptrace_hbp_get_initialised_bp() calls
ptrace_hbp_create(), which registers a disabled perf_event and reserves one
of the task's shared hardware trigger slots via reserve_bp_slot(). If
modify_user_hw_breakpoint() fails (for example on an address >= TASK_SIZE) or
riscv_hw_break_set() fails while copying or applying ctrl, the event is not
unregistered, exhausting the task's hardware trigger slots until task exit or
execve.
> +#define PTRACE_HBP_ADDR_SZ sizeof(u64)
> +#define PTRACE_HBP_CTRL_SZ sizeof(u32)
> +#define PTRACE_HBP_PAD_SZ sizeof(u32)
> +
> +static int riscv_hw_break_get(struct task_struct *target,
> + const struct user_regset *regset,
> + struct membuf to)
> +{
> + unsigned int note_type = regset->core_note_type;
> + enum riscv_hbp_type hbp_type;
> + int ret, idx, num_slots;
> + u32 info, ctrl;
> + u64 addr;
> +
> + ret = ptrace_hbp_note_to_type(note_type, &hbp_type);
> + if (ret)
> + return ret;
> +
> + /* Resource info: number of available slots */
> + ret = ptrace_hbp_get_resource_info(hbp_type, &info);
> + if (ret)
> + return ret;
> +
> + membuf_write(&to, &info, sizeof(info));
> + membuf_zero(&to, sizeof(u32));
> +
> + /* Emit one (address, ctrl, pad) entry per available slot */
> + num_slots = (int)info;
> + for (idx = 0; idx < num_slots; idx++) {
> + ret = ptrace_hbp_get_addr(hbp_type, target, idx, &addr);
> + if (ret)
> + return ret;
> + ret = ptrace_hbp_get_ctrl(hbp_type, target, idx, &ctrl);
> + if (ret)
> + return ret;
> + membuf_store(&to, addr);
> + membuf_store(&to, ctrl);
> + membuf_zero(&to, sizeof(u32));
> + }
> + return 0;
> +}
> +
> +static int riscv_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)
> +{
[ ... ]
> + /* (address, ctrl) registers */
> + limit = regset->n * regset->size;
> + while (count && offset < limit) {
[Severity: Medium]
Should riscv_hw_break_get() return to.left instead of 0, and should
riscv_hw_break_set() bound writes by num_slots rather than all 16 entries in
struct user_hwdebug_state?
__regset_get() computes the number of bytes produced as size - res, so
returning 0 after writing only num_slots entries causes PTRACE_GETREGSET and
ELF core dumps to report all 16 slots (264 bytes) as populated even when
hw_breakpoint_slots() is smaller. Meanwhile, riscv_hw_break_set() allows
programming slot indices num_slots <= idx < RISCV_MAX_BP, creating active
breakpoints that riscv_hw_break_get() never reads back.
[ ... ]
> @@ -919,6 +1237,28 @@ static const struct user_regset compat_riscv_user_regset[] = {
> .set = riscv_fpr_set,
> },
> #endif
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> + /*
> + * The hardware-debug regset uses fixed-width UAPI fields, so the
> + * native layout is also the compat layout.
> + */
> + [REGSET_RISCV_HW_BREAK] = {
> + USER_REGSET_NOTE_TYPE(RISCV_HW_BREAK),
> + .n = sizeof(struct user_hwdebug_state) / sizeof(u32),
> + .size = sizeof(u32),
> + .align = sizeof(u32),
> + .regset_get = riscv_hw_break_get,
> + .set = riscv_hw_break_set,
> + },
> + [REGSET_RISCV_HW_WATCH] = {
> + USER_REGSET_NOTE_TYPE(RISCV_HW_WATCH),
> + .n = sizeof(struct user_hwdebug_state) / sizeof(u32),
> + .size = sizeof(u32),
> + .align = sizeof(u32),
> + .regset_get = riscv_hw_break_get,
> + .set = riscv_hw_break_set,
> + },
> +#endif
> };
[Severity: Medium]
Does indexing compat_riscv_user_regset[] with REGSET_RISCV_HW_BREAK and
REGSET_RISCV_HW_WATCH create zero-initialized struct user_regset holes that
cause a division by zero in ptrace_regset()?
In enum riscv_regset, REGSET_RISCV_HW_BREAK and REGSET_RISCV_HW_WATCH follow
REGSET_V, REGSET_TAGGED_ADDR_CTRL, and REGSET_CFI, which are omitted from
compat_riscv_user_regset[]. When any of CONFIG_RISCV_ISA_V,
CONFIG_RISCV_ISA_SUPM, or CONFIG_RISCV_USER_CFI is enabled, those intermediate
slots in compat_riscv_user_regset[] are zero-filled with core_note_type == 0
and size == 0. Calling ptrace(PTRACE_GETREGSET, pid, 0, &iov) on a compat
task matches the hole in find_regset(view, 0) and evaluates kiov->iov_len %
regset->size with regset->size == 0 in kernel/ptrace.c:ptrace_regset().
[1] https://lore.kernel.org/r/5a7c08bd-4313-4424-b649-a19ffe5242e4@bytedance.com
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261107023917.40606-1-himanshu.chauhan@oss.qualcomm.com?part=4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v8 5/8] selftests/breakpoints: extend riscv test for ptrace hw break/watchpoints
2026-11-07 2:39 [PATCH v8 0/8] riscv: Introduce support for hardware break/watchpoints Himanshu Chauhan
` (3 preceding siblings ...)
2026-11-07 2:39 ` [PATCH v8 4/8] riscv: add regset access " Himanshu Chauhan
@ 2026-11-07 2:39 ` Himanshu Chauhan
2026-11-08 2:41 ` sashiko-bot
2026-11-07 2:39 ` [PATCH v8 6/8] riscv: Add shared instruction fetch and decode helpers Himanshu Chauhan
` (2 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Himanshu Chauhan @ 2026-11-07 2:39 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-kernel,
linux-mm, linux-perf-users, linux-kselftest, Himanshu Chauhan
Cover the new ptrace-based hardware breakpoint/watchpoint support
added for riscv: exercise the PTRACE_GETREGSET/SETREGSET regset path
as well as the raw PTRACE_GETHBPREGS/SETHBPREGS interface, alongside
the existing perf_event-based tests.
Also drop a leftover #if 0 block duplicating the HWDEBUG_* control
field macros already defined in uapi/asm/ptrace.h.
Signed-off-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
---
.../breakpoints/breakpoint_test_riscv.c | 698 +++++++++++++++++-
1 file changed, 675 insertions(+), 23 deletions(-)
diff --git a/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
index a8a94a788142..b07f6770c380 100644
--- a/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
+++ b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
@@ -9,6 +9,7 @@
#include <linux/perf_event.h> /* Definition of PERF_* constants */
#include <linux/hw_breakpoint.h> /* Definition of HW_* constants */
#include <sys/syscall.h> /* Definition of SYS_* constants */
+#include <asm/ptrace.h>
#include <unistd.h>
#include <stdbool.h>
#include <stdio.h>
@@ -18,17 +19,512 @@
#include <fcntl.h>
#include <signal.h>
#include <sys/mman.h>
+#include <sys/ptrace.h>
+#include <sys/wait.h>
#include <string.h>
#include <semaphore.h>
#include <errno.h>
+#include <stdint.h>
+#include <stddef.h>
+#include <linux/elf.h>
+#include <ucontext.h>
+
+#include "kselftest.h"
-static int gfd;
-sem_t ib_mtx, wp_mtx;
-static int bp_triggered, wp_triggered;
static int test_func_sink;
static const int wait_timeout_sec = 5;
-int setup_bp(bool is_x, void *addr, int sig)
+static unsigned int hwdebug_control(unsigned int type, unsigned long len)
+{
+ return HWDEBUG_MK_TYPE(type) | HWDEBUG_MK_SIZE(len);
+}
+
+static void cleanup_child(pid_t pid)
+{
+ int status;
+
+ if (pid <= 0)
+ return;
+
+ /* Every error path funnels here so a stopped tracee is never leaked. */
+ kill(pid, SIGKILL);
+ while (waitpid(pid, &status, __WALL) < 0 && errno == EINTR)
+ ;
+}
+
+static pid_t waitpid_timeout(pid_t pid, int *status)
+{
+ struct timespec delay = {
+ .tv_sec = 0,
+ .tv_nsec = 10 * 1000 * 1000,
+ };
+ int tries = wait_timeout_sec * 100;
+ pid_t wpid;
+
+ /* Avoid alarm(): SIGALRM's default action would skip tracee cleanup. */
+ while (tries--) {
+ wpid = waitpid(pid, status, __WALL | WNOHANG);
+ if (wpid < 0 && errno == EINTR)
+ continue;
+ if (wpid != 0)
+ return wpid;
+ nanosleep(&delay, NULL);
+ }
+
+ errno = ETIMEDOUT;
+ return -1;
+}
+
+static bool wait_for_child_stop(pid_t pid, int expected_sig, const char *name)
+{
+ pid_t wpid;
+ int status;
+
+ wpid = waitpid_timeout(pid, &status);
+ if (wpid != pid) {
+ ksft_print_msg("%s: waitpid() failed: %s\n", name,
+ strerror(errno));
+ return false;
+ }
+ if (!WIFSTOPPED(status)) {
+ ksft_print_msg("%s: child did not stop\n", name);
+ return false;
+ }
+ if (WSTOPSIG(status) != expected_sig) {
+ ksft_print_msg("%s: child stopped with signal %d, expected %d\n",
+ name, WSTOPSIG(status), expected_sig);
+ return false;
+ }
+
+ return true;
+}
+
+static bool continue_and_wait_for_trap(pid_t pid, const char *name)
+{
+ siginfo_t siginfo;
+
+ if (ptrace(PTRACE_CONT, pid, NULL, NULL) < 0) {
+ ksft_print_msg("%s: ptrace(PTRACE_CONT) failed: %s\n",
+ name, strerror(errno));
+ return false;
+ }
+
+ if (!wait_for_child_stop(pid, SIGTRAP, name))
+ return false;
+
+ if (ptrace(PTRACE_GETSIGINFO, pid, NULL, &siginfo) != 0) {
+ ksft_print_msg("%s: ptrace(PTRACE_GETSIGINFO): %s\n",
+ name, strerror(errno));
+ return false;
+ }
+ if (siginfo.si_code != TRAP_HWBKPT) {
+ ksft_print_msg("%s: unexpected si_code %d\n", name,
+ siginfo.si_code);
+ return false;
+ }
+
+ return true;
+}
+
+/*
+ * Keep a real instruction address for HW execute breakpoints: prevent inlining
+ * and force a visible side effect so the function can't be optimized away.
+ */
+static __attribute_noinline__ void test_func(void)
+{
+ test_func_sink++;
+ __asm__ __volatile__("" : : "g" (test_func_sink));
+}
+
+/*
+ * BREAKPOINT TEST USING PTRACE
+ */
+static int do_wp_child(void *addr, size_t size)
+{
+ if (ptrace(PTRACE_TRACEME, 0, NULL, NULL) != 0) {
+ ksft_print_msg("ptrace(PTRACE_TRACEME) failed: %s\n",
+ strerror(errno));
+ _exit(1);
+ }
+
+ if (raise(SIGSTOP) != 0) {
+ ksft_print_msg("raise(SIGSTOP) failed: %s\n", strerror(errno));
+ _exit(1);
+ }
+
+ sleep(1);
+ switch (size) {
+ case 1:
+ *(uint8_t *)addr = 47;
+ break;
+ case 2:
+ *(uint16_t *)addr = 47;
+ break;
+ case 4:
+ *(uint32_t *)addr = 47;
+ break;
+ case 8:
+ *(uint64_t *)addr = 47;
+ break;
+ default:
+ ksft_print_msg("Unknown watchpoint access size %zu\n", size);
+ break;
+ }
+
+ _exit(0);
+}
+
+static int do_bp_child(void (*bp_func)(void))
+{
+ if (ptrace(PTRACE_TRACEME, 0, NULL, NULL) != 0) {
+ ksft_print_msg("ptrace(PTRACE_TRACEME) failed: %s\n",
+ strerror(errno));
+ _exit(1);
+ }
+
+ if (raise(SIGSTOP) != 0) {
+ ksft_print_msg("raise(SIGSTOP) failed: %s\n", strerror(errno));
+ _exit(1);
+ }
+
+ bp_func();
+ _exit(0);
+}
+
+unsigned long var;
+
+static bool set_watchpoint(pid_t pid, int size)
+{
+ uint8_t *addr = (uint8_t *)&var;
+ unsigned int control = 0;
+ struct user_hwdebug_state dreg_state;
+ struct iovec iov;
+
+ /* Write watchpoint */
+ switch (size) {
+ case HW_BREAKPOINT_LEN_1:
+ case HW_BREAKPOINT_LEN_2:
+ case HW_BREAKPOINT_LEN_4:
+ case HW_BREAKPOINT_LEN_8:
+ break;
+ default:
+ ksft_print_msg("Unsupported watchpoint size %d\n", size);
+ return false;
+ }
+
+ control = hwdebug_control(HW_BREAKPOINT_W, size);
+ memset(&dreg_state, 0, sizeof(dreg_state));
+ dreg_state.dbg_regs[0].addr = (uintptr_t)(addr);
+ dreg_state.dbg_regs[0].control = control;
+ iov.iov_base = &dreg_state;
+ iov.iov_len = offsetof(struct user_hwdebug_state, dbg_regs) +
+ sizeof(dreg_state.dbg_regs[0]);
+
+ if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_HW_WATCH, &iov) == 0) {
+ memset(&iov, 0, sizeof(iov));
+ memset(&dreg_state, 0, sizeof(dreg_state));
+
+ iov.iov_base = &dreg_state;
+ iov.iov_len = offsetof(struct user_hwdebug_state, dbg_regs) +
+ sizeof(dreg_state.dbg_regs[0]);
+
+ if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_HW_WATCH, &iov) == 0) {
+ ksft_print_msg("ptrace(PTRACE_GETREGSET): Number of watchpoints: %u\n",
+ dreg_state.info);
+ ksft_print_msg("ptrace(PTRACE_GETREGSET): addr: 0x%lx control: 0x%x\n",
+ dreg_state.dbg_regs[0].addr,
+ dreg_state.dbg_regs[0].control);
+ if (!dreg_state.info ||
+ dreg_state.dbg_regs[0].addr != (uintptr_t)addr ||
+ HWDEBUG_TYPE(dreg_state.dbg_regs[0].control) != HW_BREAKPOINT_W ||
+ HWDEBUG_SIZE(dreg_state.dbg_regs[0].control) != size) {
+ ksft_print_msg("Bad wp readback: info=%u addr=0x%lx ctrl=0x%x\n",
+ dreg_state.info,
+ dreg_state.dbg_regs[0].addr,
+ dreg_state.dbg_regs[0].control);
+ return false;
+ }
+ } else {
+ ksft_print_msg("ptrace(PTRACE_GETREGSET): Failed\n");
+ return false;
+ }
+
+ return true;
+ }
+
+ if (errno == EIO)
+ ksft_print_msg("ptrace(PTRACE_SETREGSET, NT_RISCV_HW_WATCH) not supported: %s\n",
+ strerror(errno));
+ else
+ ksft_print_msg("ptrace(PTRACE_SETREGSET, NT_RISCV_HW_WATCH) failed: %s\n",
+ strerror(errno));
+ return false;
+}
+
+static bool set_breakpoint(pid_t pid, void (*bp_func)(void))
+{
+ struct user_hwdebug_state dreg_state;
+ struct iovec iov;
+ unsigned int control = 0;
+
+ control = hwdebug_control(HW_BREAKPOINT_X, HW_BREAKPOINT_LEN_4);
+ memset(&dreg_state, 0, sizeof(dreg_state));
+ dreg_state.dbg_regs[0].addr = (uintptr_t)bp_func;
+ dreg_state.dbg_regs[0].control = control;
+ iov.iov_base = &dreg_state;
+ iov.iov_len = offsetof(struct user_hwdebug_state, dbg_regs) +
+ sizeof(dreg_state.dbg_regs[0]);
+
+ if (ptrace(PTRACE_SETREGSET, pid, NT_RISCV_HW_BREAK, &iov) == 0)
+ return true;
+
+ if (errno == EIO)
+ ksft_print_msg("ptrace(PTRACE_SETREGSET, NT_RISCV_HW_BREAK) not supported: %s\n",
+ strerror(errno));
+ else
+ ksft_print_msg("ptrace(PTRACE_SETREGSET, NT_RISCV_HW_BREAK) failed: %s\n",
+ strerror(errno));
+
+ return false;
+}
+
+static bool run_ptrace_wp_test(void)
+{
+ pid_t pid = fork();
+ bool ret = false;
+
+ if (pid < 0) {
+ ksft_print_msg("fork() failed: %s\n", strerror(errno));
+ return false;
+ }
+
+ if (pid == 0)
+ do_wp_child(&var, 8);
+
+ if (!wait_for_child_stop(pid, SIGSTOP, "ptrace watchpoint"))
+ goto out;
+
+ if (!set_watchpoint(pid, 8))
+ goto out;
+
+ if (!continue_and_wait_for_trap(pid, "ptrace watchpoint"))
+ goto out;
+
+ ksft_print_msg("[ptrace]: Watchpoint test passed!\n");
+ ret = true;
+
+out:
+ cleanup_child(pid);
+ return ret;
+}
+
+static bool run_ptrace_bp_test(void)
+{
+ pid_t pid = fork();
+ bool ret = false;
+
+ if (pid < 0) {
+ ksft_print_msg("fork() failed: %s\n", strerror(errno));
+ return false;
+ }
+
+ if (pid == 0)
+ do_bp_child(test_func);
+
+ if (!wait_for_child_stop(pid, SIGSTOP, "ptrace breakpoint"))
+ goto out;
+
+ if (!set_breakpoint(pid, test_func))
+ goto out;
+
+ if (!continue_and_wait_for_trap(pid, "ptrace breakpoint"))
+ goto out;
+
+ ksft_print_msg("[ptrace]: Breakpoint test passed!\n");
+ ret = true;
+
+out:
+ cleanup_child(pid);
+ return ret;
+}
+
+/*
+ * BREAKPOINT TEST USING PTRACE_SETHBPREGS / PTRACE_GETHBPREGS
+ */
+static bool set_hbpregs_watchpoint(pid_t pid)
+{
+ struct __riscv_hwdebug_state state;
+
+ memset(&state, 0, sizeof(state));
+ state.addr = (unsigned long)&var;
+ state.len = HW_BREAKPOINT_LEN_8;
+ state.type = HW_BREAKPOINT_W;
+ state.ctrl = 0; /* enabled */
+
+ if (ptrace(PTRACE_SETHBPREGS, pid, 0, &state) != 0) {
+ ksft_print_msg("ptrace(PTRACE_SETHBPREGS) failed: %s\n",
+ strerror(errno));
+ return false;
+ }
+
+ /* Read back and verify */
+ memset(&state, 0, sizeof(state));
+ if (ptrace(PTRACE_GETHBPREGS, pid, 0, &state) != 0) {
+ ksft_print_msg("ptrace(PTRACE_GETHBPREGS) failed: %s\n",
+ strerror(errno));
+ return false;
+ }
+
+ ksft_print_msg("[hbpregs] watchpoint readback: addr=0x%lx type=%lu len=%lu ctrl=%lu\n",
+ state.addr, state.type, state.len, state.ctrl);
+ if (state.addr != (unsigned long)&var ||
+ state.type != HW_BREAKPOINT_W ||
+ state.len != HW_BREAKPOINT_LEN_8 ||
+ state.ctrl != 0) {
+ ksft_print_msg("[hbpregs] unexpected watchpoint readback\n");
+ return false;
+ }
+
+ return true;
+}
+
+static bool set_hbpregs_breakpoint(pid_t pid, void (*bp_func)(void))
+{
+ struct __riscv_hwdebug_state state;
+
+ memset(&state, 0, sizeof(state));
+ state.addr = (unsigned long)bp_func;
+ state.len = HW_BREAKPOINT_LEN_4;
+ state.type = HW_BREAKPOINT_X;
+ state.ctrl = 0; /* enabled */
+
+ if (ptrace(PTRACE_SETHBPREGS, pid, 0, &state) != 0) {
+ ksft_print_msg("ptrace(PTRACE_SETHBPREGS) failed: %s\n",
+ strerror(errno));
+ return false;
+ }
+
+ /* Read back and verify */
+ memset(&state, 0, sizeof(state));
+ if (ptrace(PTRACE_GETHBPREGS, pid, 0, &state) != 0) {
+ ksft_print_msg("ptrace(PTRACE_GETHBPREGS) failed: %s\n",
+ strerror(errno));
+ return false;
+ }
+
+ ksft_print_msg("[hbpregs] breakpoint readback: addr=0x%lx type=%lu len=%lu ctrl=%lu\n",
+ state.addr, state.type, state.len, state.ctrl);
+ if (state.addr != (unsigned long)bp_func ||
+ state.type != HW_BREAKPOINT_X ||
+ state.len != HW_BREAKPOINT_LEN_4 ||
+ state.ctrl != 0) {
+ ksft_print_msg("[hbpregs] unexpected breakpoint readback\n");
+ return false;
+ }
+
+ return true;
+}
+
+static bool run_hbpregs_wp_test(void)
+{
+ pid_t pid = fork();
+ bool ret = false;
+
+ if (pid < 0) {
+ ksft_print_msg("fork() failed: %s\n", strerror(errno));
+ return false;
+ }
+
+ if (pid == 0)
+ do_wp_child(&var, 8);
+
+ if (!wait_for_child_stop(pid, SIGSTOP, "hbpregs watchpoint"))
+ goto out;
+
+ if (!set_hbpregs_watchpoint(pid))
+ goto out;
+
+ if (!continue_and_wait_for_trap(pid, "hbpregs watchpoint"))
+ goto out;
+
+ ksft_print_msg("[hbpregs]: Watchpoint test passed!\n");
+ ret = true;
+
+out:
+ cleanup_child(pid);
+ return ret;
+}
+
+static bool run_hbpregs_bp_test(void)
+{
+ pid_t pid = fork();
+ bool ret = false;
+
+ if (pid < 0) {
+ ksft_print_msg("fork() failed: %s\n", strerror(errno));
+ return false;
+ }
+
+ if (pid == 0)
+ do_bp_child(test_func);
+
+ if (!wait_for_child_stop(pid, SIGSTOP, "hbpregs breakpoint"))
+ goto out;
+
+ if (!set_hbpregs_breakpoint(pid, test_func))
+ goto out;
+
+ if (!continue_and_wait_for_trap(pid, "hbpregs breakpoint"))
+ goto out;
+
+ ksft_print_msg("[hbpregs]: Breakpoint test passed!\n");
+ ret = true;
+
+out:
+ cleanup_child(pid);
+ return ret;
+}
+
+static bool run_hbpregs_tests(void)
+{
+ bool ret = true;
+
+ ret &= run_hbpregs_bp_test();
+ ret &= run_hbpregs_wp_test();
+
+ return ret;
+}
+
+/*
+ * BREAKPOINT TEST USING PTRACE_SETHBPREGS / PTRACE_GETHBPREGS - END
+ */
+static bool run_ptrace_tests(void)
+{
+ bool ret = true;
+
+ ret &= run_ptrace_bp_test();
+ ret &= run_ptrace_wp_test();
+
+ return ret;
+}
+
+/*
+ * BREAKPOINT TEST USING PTRACE - END
+ */
+
+/*
+ * BREAKPOINT TEST USING perf events
+ */
+static int gfd;
+sem_t ib_mtx, wp_mtx, icount_branch_mtx;
+static sig_atomic_t bp_triggered, wp_triggered;
+static sig_atomic_t icount_branch_triggered;
+static int icount_branch_result;
+static unsigned long icount_branch_pc;
+
+static int wait_event(sem_t *sem, const char *name);
+
+int setup_bp_len(bool is_x, void *addr, int sig, unsigned long len)
{
struct perf_event_attr pe;
int fd;
@@ -40,7 +536,7 @@ int setup_bp(bool is_x, void *addr, int sig)
pe.config = 0;
pe.bp_type = is_x ? HW_BREAKPOINT_X : HW_BREAKPOINT_W;
pe.bp_addr = (unsigned long)addr;
- pe.bp_len = sizeof(long);
+ pe.bp_len = len;
pe.sample_period = 1;
pe.sample_type = PERF_SAMPLE_IP;
@@ -52,7 +548,7 @@ int setup_bp(bool is_x, void *addr, int sig)
fd = syscall(SYS_perf_event_open, &pe, 0, -1, -1, 0);
if (fd < 0) {
- printf("Failed to open event: %llx\n", pe.config);
+ ksft_print_msg("Failed to open event: %llx\n", pe.config);
return -1;
}
@@ -65,17 +561,21 @@ int setup_bp(bool is_x, void *addr, int sig)
return fd;
}
+int setup_bp(bool is_x, void *addr, int sig)
+{
+ return setup_bp_len(is_x, addr, sig, sizeof(long));
+}
+
static void sig_handler_bp(int signum, siginfo_t *oh, void *uc)
{
int ret;
bp_triggered++;
- printf("Breakpoint triggered!\n");
ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
ret = sem_post(&ib_mtx);
if (ret) {
- printf("Failed to report BP success\n");
+ ksft_print_msg("Failed to report BP success\n");
return;
}
}
@@ -84,26 +584,167 @@ static void sig_handler_wp(int signum, siginfo_t *oh, void *uc)
{
int ret;
- printf("Watchpoint triggered!\n");
ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
wp_triggered++;
ret = sem_post(&wp_mtx);
if (ret) {
- printf("Failed to report WP success\n");
+ ksft_print_msg("Failed to report WP success\n");
return;
}
}
-/*
- * Keep a real instruction address for HW execute breakpoints: prevent inlining
- * and force a visible side effect so the function can't be optimized away.
- */
-static __attribute_noinline__ void test_func(void)
+static void *icount_branch_insn_addr(void)
{
- test_func_sink++;
- __asm__ __volatile__("" : : "g" (test_func_sink));
+ void *addr;
+
+ asm("la %0, icount_branch_insn" : "=r"(addr));
+ return addr;
+}
+
+static void *icount_branch_fallthrough_addr(void)
+{
+ void *addr;
+
+ asm("la %0, icount_branch_fallthrough" : "=r"(addr));
+ return addr;
+}
+
+static void *icount_branch_target_addr(void)
+{
+ void *addr;
+
+ asm("la %0, icount_branch_target" : "=r"(addr));
+ return addr;
+}
+
+static __attribute_noinline__ void run_icount_taken_branch(void)
+{
+ asm volatile(".option push\n"
+ ".option norvc\n"
+ ".globl icount_branch_insn\n"
+ ".globl icount_branch_fallthrough\n"
+ ".globl icount_branch_target\n"
+ "li t0, 1\n"
+ "li t1, 1\n"
+ ".balign 4\n"
+ "icount_branch_insn:\n"
+ "beq t0, t1, icount_branch_target\n"
+ "icount_branch_fallthrough:\n"
+ "la t2, icount_branch_result\n"
+ "li t3, 2\n"
+ "sw t3, 0(t2)\n"
+ "j icount_branch_done\n"
+ ".balign 4\n"
+ "icount_branch_target:\n"
+ "la t2, icount_branch_result\n"
+ "li t3, 1\n"
+ "sw t3, 0(t2)\n"
+ "icount_branch_done:\n"
+ ".option pop\n"
+ :
+ :
+ : "t0", "t1", "t2", "t3", "memory");
+}
+
+static void sig_handler_icount_branch(int signum, siginfo_t *oh, void *uc)
+{
+ ucontext_t *ctx = uc;
+ int ret;
+
+ icount_branch_triggered++;
+ icount_branch_pc = ctx->uc_mcontext.__gregs[REG_PC];
+
+ ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
+ ret = sem_post(&icount_branch_mtx);
+ if (ret) {
+ ksft_print_msg("Failed to report icount branch success\n");
+ return;
+ }
+}
+
+static int trigger_icount_branch(void)
+{
+ struct sigaction sa;
+ void *insn_addr;
+ void *fallthrough_addr;
+ void *target_addr;
+ uint64_t count;
+ ssize_t nread;
+
+ memset(&sa, 0, sizeof(struct sigaction));
+ sa.sa_sigaction = sig_handler_icount_branch;
+ sa.sa_flags = SA_SIGINFO;
+
+ if (sigaction(SIGIO, &sa, NULL) < 0) {
+ ksft_print_msg("Failed to setup icount branch signal handler\n");
+ return -1;
+ }
+
+ icount_branch_triggered = 0;
+ icount_branch_result = 0;
+ icount_branch_pc = 0;
+
+ insn_addr = icount_branch_insn_addr();
+ fallthrough_addr = icount_branch_fallthrough_addr();
+ target_addr = icount_branch_target_addr();
+
+ gfd = setup_bp_len(1, insn_addr, SIGIO, HW_BREAKPOINT_LEN_4);
+ if (gfd < 0) {
+ ksft_print_msg("Failed to setup icount branch breakpoint.\n");
+ return -1;
+ }
+
+ ioctl(gfd, PERF_EVENT_IOC_ENABLE, 0);
+ run_icount_taken_branch();
+
+ if (wait_event(&icount_branch_mtx, "Icount branch") < 0) {
+ ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
+ close(gfd);
+ return -1;
+ }
+
+ nread = read(gfd, &count, sizeof(count));
+ ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
+ close(gfd);
+
+ if (nread != sizeof(count)) {
+ ksft_print_msg("Failed to read icount branch count: %s\n",
+ strerror(errno));
+ return -1;
+ }
+
+ ksft_print_msg("icount branch: insn=%p fall=%p target=%p pc=0x%lx count=%llu res=%d\n",
+ insn_addr, fallthrough_addr, target_addr, icount_branch_pc,
+ (unsigned long long)count, icount_branch_result);
+
+ if (icount_branch_triggered != 1) {
+ ksft_print_msg("Unexpected icount branch signal count %d\n",
+ icount_branch_triggered);
+ return -1;
+ }
+
+ if (count != 1) {
+ ksft_print_msg("Unexpected icount branch event count %llu\n",
+ (unsigned long long)count);
+ return -1;
+ }
+
+ if (icount_branch_result != 1) {
+ ksft_print_msg("Icount branch did not take the branch\n");
+ return -1;
+ }
+
+ if (icount_branch_pc != (unsigned long)target_addr) {
+ ksft_print_msg("Icount branch PC 0x%lx does not match target %p\n",
+ icount_branch_pc, target_addr);
+ return -1;
+ }
+
+ ksft_print_msg("[perf_event]: Icount taken branch PC test passed!\n");
+
+ return 0;
}
static int trigger_bp(void)
@@ -115,14 +756,14 @@ static int trigger_bp(void)
sa.sa_flags = SA_SIGINFO;
if (sigaction(SIGIO, &sa, NULL) < 0) {
- printf("Failed to setup signal handler\n");
+ ksft_print_msg("Failed to setup signal handler\n");
return -1;
}
gfd = setup_bp(1, test_func, SIGIO);
if (gfd < 0) {
- printf("Failed to setup breakpoint.\n");
+ ksft_print_msg("Failed to setup breakpoint.\n");
return -1;
}
@@ -147,14 +788,14 @@ static int trigger_wp(void)
sa.sa_flags = SA_SIGINFO;
if (sigaction(SIGUSR1, &sa, NULL) < 0) {
- printf("Failed to setup signal handler\n");
+ ksft_print_msg("Failed to setup signal handler\n");
return -1;
}
gfd = setup_bp(0, &test_data, SIGUSR1);
if (gfd < 0) {
- printf("Failed to setup watchpoint\n");
+ ksft_print_msg("Failed to setup watchpoint\n");
return -1;
}
@@ -162,6 +803,8 @@ static int trigger_wp(void)
test_data = 0xdeadbeef;
ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
+ close(gfd);
+
return 0;
}
@@ -200,7 +843,7 @@ int main(int argc, char *argv[])
return -1;
if (bp_triggered)
- printf("Breakpoint test passed!\n");
+ ksft_print_msg("[perf_event]: Breakpoint test passed!\n");
sem_init(&wp_mtx, 0, 0);
if (trigger_wp() < 0)
@@ -209,7 +852,16 @@ int main(int argc, char *argv[])
return -1;
if (wp_triggered)
- printf("Watchpoint test passed!\n");
+ ksft_print_msg("[perf_event]: Watchpoint test passed!\n");
+
+ sem_init(&icount_branch_mtx, 0, 0);
+ if (trigger_icount_branch() < 0)
+ return -1;
+
+ if (!run_ptrace_tests())
+ return -1;
+ if (!run_hbpregs_tests())
+ return -1;
return 0;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v8 5/8] selftests/breakpoints: extend riscv test for ptrace hw break/watchpoints
2026-11-07 2:39 ` [PATCH v8 5/8] selftests/breakpoints: extend riscv test for ptrace hw break/watchpoints Himanshu Chauhan
@ 2026-11-08 2:41 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-11-08 2:41 UTC (permalink / raw)
To: Himanshu Chauhan, Paul Walmsley, Palmer Dabbelt, Albert Ou
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-mm,
linux-perf-users
Thank you for your contribution! Sashiko AI review found 7 potential
issues to consider:
- [Medium] selftests/breakpoints: 8-byte write into unsigned long var on
32-bit riscv
- [Medium] selftests/breakpoints: icount_branch_result modified only via
raw asm symbol
- [Low] selftests/breakpoints: format string mismatch for __u64 in
breakpoint_test_riscv
- [Low] selftests/breakpoints: missing kselftest TAP reporting, skip
handling, and changelog mismatch
- [Low] selftests/breakpoints: commit message out of sync with diff
changes
- [Low] selftests/breakpoints: icount_branch_result invisible to C
optimizer
- [Low] selftests/breakpoints: close(gfd) before wait_event() in
trigger_wp() and trigger_bp()
--
commit 91bb1409ac047eda69672c8b99664375baad647d
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
> selftests/breakpoints: extend riscv test for ptrace hw break/watchpoints
>
> Cover the new ptrace-based hardware breakpoint/watchpoint support
> added for riscv: exercise the PTRACE_GETREGSET/SETREGSET regset path
> as well as the raw PTRACE_GETHBPREGS/SETHBPREGS interface, alongside
> the existing perf_event-based tests.
>
> Also drop a leftover #if 0 block duplicating the HWDEBUG_* control
> field macros already defined in uapi/asm/ptrace.h.
>
> Signed-off-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
[Severity: Low]
Is this commit message out of sync with the changes in the diff? The parent
commit "riscv: Add breakpoint and watchpoint test for riscv" has no #if 0
block with duplicate HWDEBUG_* macros, while the new perf_event taken-branch
PC test (trigger_icount_branch()) and the close(gfd) fix in trigger_wp() are
not mentioned.
> diff --git a/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
> index a8a94a788142d..b07f6770c380e 100644
> --- a/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
> +++ b/tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
[ ... ]
> + if (ptrace(PTRACE_GETREGSET, pid, NT_RISCV_HW_WATCH, &iov) == 0) {
> + ksft_print_msg("ptrace(PTRACE_GETREGSET): Number of watchpoints: %u\n",
> + dreg_state.info);
> + ksft_print_msg("ptrace(PTRACE_GETREGSET): addr: 0x%lx control: 0x%x\n",
> + dreg_state.dbg_regs[0].addr,
> + dreg_state.dbg_regs[0].control);
> + if (!dreg_state.info ||
> + dreg_state.dbg_regs[0].addr != (uintptr_t)addr ||
> + HWDEBUG_TYPE(dreg_state.dbg_regs[0].control) != HW_BREAKPOINT_W ||
> + HWDEBUG_SIZE(dreg_state.dbg_regs[0].control) != size) {
> + ksft_print_msg("Bad wp readback: info=%u addr=0x%lx ctrl=0x%x\n",
> + dreg_state.info,
> + dreg_state.dbg_regs[0].addr,
> + dreg_state.dbg_regs[0].control);
> + return false;
> + }
[Severity: Low]
Should these ksft_print_msg() calls use %llx and %llu instead of %lx and
%lu? In arch/riscv/include/uapi/asm/ptrace.h,
struct user_hwdebug_state::dbg_regs[0].addr and
struct __riscv_hwdebug_state::{addr, type, len, ctrl} (printed in
set_hbpregs_watchpoint() and set_hbpregs_breakpoint()) are __u64, which is
unsigned long long on both 32-bit and 64-bit RISC-V, causing -Wformat
warnings and corrupting variadic argument reads on 32-bit RISC-V.
[ ... ]
> +static bool run_ptrace_wp_test(void)
> +{
> + pid_t pid = fork();
> + bool ret = false;
> +
> + if (pid < 0) {
> + ksft_print_msg("fork() failed: %s\n", strerror(errno));
> + return false;
> + }
> +
> + if (pid == 0)
> + do_wp_child(&var, 8);
> +
> + if (!wait_for_child_stop(pid, SIGSTOP, "ptrace watchpoint"))
> + goto out;
> +
> + if (!set_watchpoint(pid, 8))
> + goto out;
[Severity: Medium]
Can do_wp_child(&var, 8) write past the end of var on 32-bit RISC-V?
var is declared as unsigned long (4 bytes on RV32), and
tools/testing/selftests/breakpoints/Makefile builds breakpoint_test_riscv for
both riscv and riscv64. Hardcoding 8 here and in set_hbpregs_watchpoint() /
run_hbpregs_wp_test() instead of sizeof(var) executes *(uint64_t *)addr = 47
in do_wp_child(), overflowing var in .bss and requesting HW_BREAKPOINT_LEN_8
on 32-bit kernels where only lengths up to 4 bytes are supported.
[ ... ]
> +static int icount_branch_result;
> +static unsigned long icount_branch_pc;
[ ... ]
> +static __attribute_noinline__ void run_icount_taken_branch(void)
> +{
> + asm volatile(".option push\n"
> + ".option norvc\n"
> + ".globl icount_branch_insn\n"
> + ".globl icount_branch_fallthrough\n"
> + ".globl icount_branch_target\n"
> + "li t0, 1\n"
> + "li t1, 1\n"
> + ".balign 4\n"
> + "icount_branch_insn:\n"
> + "beq t0, t1, icount_branch_target\n"
> + "icount_branch_fallthrough:\n"
> + "la t2, icount_branch_result\n"
> + "li t3, 2\n"
> + "sw t3, 0(t2)\n"
> + "j icount_branch_done\n"
> + ".balign 4\n"
> + "icount_branch_target:\n"
> + "la t2, icount_branch_result\n"
> + "li t3, 1\n"
> + "sw t3, 0(t2)\n"
> + "icount_branch_done:\n"
> + ".option pop\n"
> + :
> + :
> + : "t0", "t1", "t2", "t3", "memory");
> +}
[Severity: Low]
Can the compiler optimize away or constant-fold icount_branch_result when
building with optimizations enabled? Because icount_branch_result is a
static variable whose address is never taken in C and that is only written via
a raw symbol name in the inline assembly rather than an asm output operand,
interprocedural optimization can fold reads of icount_branch_result to 0 and
elide the symbol from .bss.
[ ... ]
> + if (icount_branch_result != 1) {
> + ksft_print_msg("Icount branch did not take the branch\n");
> + return -1;
> + }
> +
> + if (icount_branch_pc != (unsigned long)target_addr) {
> + ksft_print_msg("Icount branch PC 0x%lx does not match target %p\n",
> + icount_branch_pc, target_addr);
> + return -1;
> + }
[Severity: Medium]
Does trigger_icount_branch() fail at this point in the series and prevent
run_ptrace_tests() and run_hbpregs_tests() from running?
Before the later commit "riscv: Add software supported single stepping with
mc/mc6 triggers" in this series, hw_breakpoint_handler() delivers the perf
breakpoint event at insn_addr without single-stepping to target_addr, so
icount_branch_pc != (unsigned long)target_addr fails and main() returns -1
before reaching the new ptrace tests (in addition to icount_branch_result
being folded to 0 under -O1/-O2 unless marked volatile/__used or passed as an
asm operand).
[ ... ]
> ioctl(gfd, PERF_EVENT_IOC_ENABLE, 0);
> test_data = 0xdeadbeef;
> ioctl(gfd, PERF_EVENT_IOC_DISABLE, 0);
>
> + close(gfd);
> +
> return 0;
> }
[Severity: Low]
Can closing gfd in trigger_wp() (and trigger_bp()) before main() calls
wait_event() drop the perf fasync signal?
__perf_event_overflow() queues pending_irq via irq_work_queue() without
IRQ_WORK_HARD_IRQ, so on !CONFIG_SMP or CONFIG_PREEMPT_RT kernels kill_fasync()
is deferred until after the breakpoint trap returns to userspace:
trigger_wp()
test_data = 0xdeadbeef
<breakpoint trap>
...
__perf_event_overflow()
irq_work_queue(&event->pending_irq)
close(gfd)
__fput()
perf_fasync(-1, ...) <-- clears event->fasync before kill_fasync()
main()
wait_event(&wp_mtx, "Watchpoint") <-- times out after 5s
Closing gfd after wait_event() returns, as trigger_icount_branch() does,
avoids tearing down fasync while the signal is still pending.
[ ... ]
> if (wp_triggered)
> - printf("Watchpoint test passed!\n");
> + ksft_print_msg("[perf_event]: Watchpoint test passed!\n");
> +
> + sem_init(&icount_branch_mtx, 0, 0);
> + if (trigger_icount_branch() < 0)
> + return -1;
> +
> + if (!run_ptrace_tests())
> + return -1;
> + if (!run_hbpregs_tests())
> + return -1;
>
> return 0;
> }
[Severity: Low]
Should main() report results through the kselftest harness and skip when
hardware breakpoints are unsupported?
Although this patch includes kselftest.h and converts several prints to
ksft_print_msg(), main() never calls ksft_print_header(), ksft_set_plan(),
ksft_test_result_*(), or ksft_exit_*(), and exits with a bare return -1 (255).
In addition, when set_watchpoint() or set_breakpoint() sees errno == EIO ("not
supported") on platforms without hardware debug triggers, the test fails
instead of reporting a kselftest skip.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261107023917.40606-1-himanshu.chauhan@oss.qualcomm.com?part=5
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v8 6/8] riscv: Add shared instruction fetch and decode helpers
2026-11-07 2:39 [PATCH v8 0/8] riscv: Introduce support for hardware break/watchpoints Himanshu Chauhan
` (4 preceding siblings ...)
2026-11-07 2:39 ` [PATCH v8 5/8] selftests/breakpoints: extend riscv test for ptrace hw break/watchpoints Himanshu Chauhan
@ 2026-11-07 2:39 ` Himanshu Chauhan
2026-11-08 2:41 ` sashiko-bot
2026-11-07 2:39 ` [PATCH v8 7/8] riscv: Add software supported single stepping with mc/mc6 triggers Himanshu Chauhan
2026-11-07 2:39 ` [PATCH v8 8/8] perf tests: keep bp_signal memory accesses visible Himanshu Chauhan
7 siblings, 1 reply; 17+ messages in thread
From: Himanshu Chauhan @ 2026-11-07 2:39 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-kernel,
linux-mm, linux-perf-users, linux-kselftest, Himanshu Chauhan
Move get_insn() and __read_insn() out of traps_misaligned.c so
breakpoint single-step code can share the instruction fetch logic.
Add helpers to read GPR values and compute the next RISC-V PC for
control-flow instructions handled by software single stepping. The helpers
cover compressed and standard branches, jumps, JALR, and SRET.
Suggested-by: Jesse Taube <jtaubepe@redhat.com>
Signed-off-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
---
MAINTAINERS | 2 +
arch/riscv/include/asm/insn.h | 16 ++
arch/riscv/kernel/traps_misaligned.c | 52 +----
arch/riscv/lib/Makefile | 1 +
arch/riscv/lib/insn.c | 279 +++++++++++++++++++++++++++
5 files changed, 299 insertions(+), 51 deletions(-)
create mode 100644 arch/riscv/lib/insn.c
diff --git a/MAINTAINERS b/MAINTAINERS
index baf97e8c39f6..db65093d751f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -24167,9 +24167,11 @@ RISC-V DEBUGGING
M: Himanshu Chauhan <himanshu@thechauhan.dev>
L: linux-riscv@lists.infradead.org
F: arch/riscv/include/asm/hw_breakpoint.h
+F: arch/riscv/include/asm/insn.h
F: arch/riscv/include/asm/perf_event.h
F: arch/riscv/kernel/hw_breakpoint.c
F: arch/riscv/kernel/perf_*
+F: arch/riscv/lib/insn.c
F: drivers/perf/
F: tools/testing/selftests/breakpoints/breakpoint_test_riscv.c
diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
index ad348d8b8857..01b5bbce1270 100644
--- a/arch/riscv/include/asm/insn.h
+++ b/arch/riscv/include/asm/insn.h
@@ -227,9 +227,16 @@
#define RVC_MASK_C_EBREAK 0xffff
#define RVG_MASK_EBREAK 0xffffffff
#define RVG_MASK_SRET 0xffffffff
+#define RVC_MASK_INSN GENMASK(15, 0)
#define __INSN_LENGTH_MASK _UL(0x3)
#define __INSN_LENGTH_GE_32 _UL(0x3)
+
+static __always_inline bool riscv_insn_is_compressed(u32 code)
+{
+ return (code & (__INSN_LENGTH_MASK)) != (__INSN_LENGTH_GE_32);
+}
+
#define __INSN_OPCODE_MASK _UL(0x7F)
#define __INSN_BRANCH_OPCODE _UL(RVG_OPCODE_BRANCH)
@@ -600,4 +607,13 @@ static inline void riscv_insn_insert_utype_itype_imm(u32 *utype_insn, u32 *itype
*utype_insn |= (imm & RV_U_IMM_31_12_MASK) + ((imm & BIT(11)) << 1);
*itype_insn |= ((imm & RV_I_IMM_11_0_MASK) << RV_I_IMM_11_0_OPOFF);
}
+
+#ifndef __ASSEMBLY__
+#include <asm/ptrace.h>
+
+int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn);
+int get_insn_safe(struct pt_regs *regs, ulong epc, ulong *r_insn);
+unsigned long get_next_insn_address(struct pt_regs *regs, ulong insn, ulong pc);
+#endif /* __ASSEMBLY__ */
+
#endif /* _ASM_RISCV_INSN_H */
diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
index 6e8ae6c66322..541e6a5b8039 100644
--- a/arch/riscv/kernel/traps_misaligned.c
+++ b/arch/riscv/kernel/traps_misaligned.c
@@ -10,6 +10,7 @@
#include <linux/irq.h>
#include <linux/stringify.h>
+#include <asm/insn.h>
#include <asm/processor.h>
#include <asm/ptrace.h>
#include <asm/csr.h>
@@ -129,57 +130,6 @@ static unsigned long get_f32_rs(unsigned long insn, u8 fp_reg_offset,
#define GET_F32_RS2C(insn, regs) (get_f32_rs(insn, 2, regs))
#define GET_F32_RS2S(insn, regs) (get_f32_rs(RVC_RS2S(insn), 0, regs))
-#define __read_insn(regs, insn, insn_addr, type) \
-({ \
- int __ret; \
- \
- if (user_mode(regs)) { \
- __ret = get_user(insn, (type __user *) insn_addr); \
- } else { \
- insn = *(type *)insn_addr; \
- __ret = 0; \
- } \
- \
- __ret; \
-})
-
-static inline int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn)
-{
- ulong insn = 0;
-
- if (epc & 0x2) {
- ulong tmp = 0;
-
- if (__read_insn(regs, insn, epc, u16))
- return -EFAULT;
- /* __get_user() uses regular "lw" which sign extend the loaded
- * value make sure to clear higher order bits in case we "or" it
- * below with the upper 16 bits half.
- */
- insn &= GENMASK(15, 0);
- if ((insn & __INSN_LENGTH_MASK) != __INSN_LENGTH_32) {
- *r_insn = insn;
- return 0;
- }
- epc += sizeof(u16);
- if (__read_insn(regs, tmp, epc, u16))
- return -EFAULT;
- *r_insn = (tmp << 16) | insn;
-
- return 0;
- } else {
- if (__read_insn(regs, insn, epc, u32))
- return -EFAULT;
- if ((insn & __INSN_LENGTH_MASK) == __INSN_LENGTH_32) {
- *r_insn = insn;
- return 0;
- }
- insn &= GENMASK(15, 0);
- *r_insn = insn;
-
- return 0;
- }
-}
union reg_data {
u8 data_bytes[8];
diff --git a/arch/riscv/lib/Makefile b/arch/riscv/lib/Makefile
index f668b98970bd..84cdd35afa42 100644
--- a/arch/riscv/lib/Makefile
+++ b/arch/riscv/lib/Makefile
@@ -1,5 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
lib-y += delay.o
+lib-y += insn.o
lib-y += memcpy.o
lib-y += memset.o
lib-y += memmove.o
diff --git a/arch/riscv/lib/insn.c b/arch/riscv/lib/insn.c
new file mode 100644
index 000000000000..fc44e2aa6f5f
--- /dev/null
+++ b/arch/riscv/lib/insn.c
@@ -0,0 +1,279 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2026 Qualcomm Technologies, Inc.
+ */
+
+#include <linux/uaccess.h>
+
+#include <asm/insn.h>
+#include <asm/ptrace.h>
+
+/*
+ * fetch_insn_u16() - Fetch a 16-bit RISC-V instruction parcel from memory
+ * @regs: Register state used to determine the access context
+ * @insn: Pointer receiving the instruction value
+ * @insn_addr: Address from which to read the instruction parcel
+ *
+ * Fetches an instruction parcel from user or kernel memory, depending on the
+ * execution context indicated by @regs. RISC-V instruction parcels are
+ * stored in little-endian byte order, so the fetched value is converted from
+ * little-endian to the native CPU representation before being stored in @insn.
+ *
+ * The conversion is a no-op on little-endian targets and performs the
+ * required byte swap on big-endian targets.
+ *
+ * Return: 0 on success, or a negative error code if reading user memory
+ * fails.
+ */
+static int fetch_insn_u16(struct pt_regs *regs, u16 *insn,
+ unsigned long insn_addr)
+{
+ u16 val;
+ int ret;
+
+ if (user_mode(regs))
+ ret = get_user(val, (u16 __user *)insn_addr);
+ else
+ ret = get_kernel_nofault(val, (u16 *)insn_addr);
+
+ if (!ret)
+ *insn = le16_to_cpu((__force __le16)val);
+
+ return ret;
+}
+
+static int fetch_insn_u32(struct pt_regs *regs, u32 *insn,
+ unsigned long insn_addr)
+{
+ u32 val;
+ int ret;
+
+ if (user_mode(regs))
+ ret = get_user(val, (u32 __user *)insn_addr);
+ else
+ ret = get_kernel_nofault(val, (u32 *)insn_addr);
+
+ if (!ret)
+ *insn = le32_to_cpu((__force __le32)val);
+
+ return ret;
+}
+
+/**
+ * get_insn() - Fetch and decode a RISC-V instruction
+ * @regs: Register state used for instruction access
+ * @epc: Address of the instruction to fetch
+ * @r_insn: Pointer to store the fetched instruction encoding
+ *
+ * Fetches the instruction at @epc and stores its encoding in @r_insn.
+ * Both standard 32-bit instructions and compressed 16-bit instructions are
+ * supported. If a 32-bit instruction is split across two 16-bit instruction
+ * accesses, the halfwords are combined into a single instruction encoding.
+ *
+ * Return: 0 on success, or %-EFAULT if instruction access fails.
+ */
+int get_insn(struct pt_regs *regs, ulong epc, ulong *r_insn)
+{
+ u32 insn;
+ u16 half, tmp;
+
+ if (!(epc & 0x2)) {
+ if (fetch_insn_u32(regs, &insn, epc))
+ return -EFAULT;
+
+ if (riscv_insn_is_compressed(insn))
+ insn &= RVC_MASK_INSN;
+
+ *r_insn = insn;
+ return 0;
+ }
+
+ if (fetch_insn_u16(regs, &half, epc))
+ return -EFAULT;
+
+ insn = half & RVC_MASK_INSN;
+ if (riscv_insn_is_compressed(insn)) {
+ *r_insn = insn;
+ return 0;
+ }
+
+ if (fetch_insn_u16(regs, &tmp, epc + sizeof(u16)))
+ return -EFAULT;
+
+ *r_insn = ((u32)tmp << 16) | insn;
+
+ return 0;
+}
+
+int get_insn_safe(struct pt_regs *regs, ulong epc, ulong *r_insn)
+{
+ int ret;
+
+ pagefault_disable();
+ ret = get_insn(regs, epc, r_insn);
+ pagefault_enable();
+
+ return ret;
+}
+
+/**
+ * riscv_get_reg_value() - Get the value stored the given RISC-V register number
+ * @regs: Register state containing the saved register values
+ * @regno: RISC-V register number
+ *
+ * Returns the value of the RISC-V register identified by @regno. Register
+ * x0 always returns zero, as required by the RISC-V ISA.
+ *
+ * Return: The register value, or zero if @regno is zero.
+ */
+static unsigned long riscv_get_reg_value(struct pt_regs *regs, unsigned int regno)
+{
+ return regno ? regs_get_register(regs, regno * sizeof(unsigned long)) : 0;
+}
+
+static unsigned long riscv_jalr_target(unsigned long base, long offset)
+{
+ /* JALR, C.JR, and C.JALR clear bit 0 of the computed target address. */
+ return (base + offset) & ~1UL;
+}
+
+/**
+ * get_next_insn_address_compressed() - Calculate the next address for a compressed
+ * RISC-V instruction
+ * @regs: Register state used to evaluate indirect jumps and conditional
+ * branches
+ * @insn: Compressed RISC-V instruction encoding
+ * @pc: Current program counter
+ *
+ * Determines the address at which execution should continue after processing
+ * the compressed instruction in @insn. Indirect jumps use the value of the
+ * instruction's source register, unconditional jumps use the encoded
+ * immediate, and conditional branches evaluate the relevant register value.
+ *
+ * For a branch that is not taken, or for an unsupported compressed
+ * instruction, the address of the following 16-bit instruction is returned.
+ *
+ * Return: The next instruction address.
+ */
+static unsigned long get_next_insn_address_compressed(struct pt_regs *regs, u32 insn,
+ unsigned long pc)
+{
+ unsigned int rs1_num;
+
+ if (riscv_insn_is_c_jalr(insn) || riscv_insn_is_c_jr(insn)) {
+ rs1_num = RV_X(insn, RVC_C2_RS1_OPOFF, 5);
+ return riscv_jalr_target(riscv_get_reg_value(regs, rs1_num), 0);
+ }
+
+ if (riscv_insn_is_c_j(insn) || riscv_insn_is_c_jal(insn))
+ return RVC_EXTRACT_JTYPE_IMM(insn) + pc;
+
+ if (riscv_insn_is_c_beqz(insn)) {
+ rs1_num = RV_X(insn, RVC_C1_RS1_OPOFF, 3) + 8;
+ if (!rs1_num || riscv_get_reg_value(regs, rs1_num) == 0)
+ return RVC_EXTRACT_BTYPE_IMM(insn) + pc;
+ return pc + 2;
+ }
+
+ if (riscv_insn_is_c_bnez(insn)) {
+ rs1_num = RV_X(insn, RVC_C1_RS1_OPOFF, 3) + 8;
+ if (rs1_num && riscv_get_reg_value(regs, rs1_num) != 0)
+ return RVC_EXTRACT_BTYPE_IMM(insn) + pc;
+ return pc + 2;
+ }
+
+ return pc + 2;
+}
+
+/**
+ * riscv_branch_taken() - Determine whether a RISC-V conditional branch is taken
+ * @regs: Register state used to obtain the source operand values
+ * @insn: RISC-V branch instruction encoding
+ *
+ * Evaluates the branch condition encoded in @insn using the values of its
+ * source registers from @regs. Signed comparisons are used for BLT and BGE,
+ * while unsigned comparisons are used for BLTU and BGEU.
+ *
+ * Return: %true if the branch condition is satisfied, or %false otherwise.
+ * Unsupported branch instructions also return %false.
+ */
+static bool riscv_branch_taken(struct pt_regs *regs, u32 insn)
+{
+ unsigned int rs1_num = RV_X(insn, RVG_RS1_OPOFF, 5);
+ unsigned int rs2_num = RV_X(insn, RVG_RS2_OPOFF, 5);
+ unsigned long rs1_val = riscv_get_reg_value(regs, rs1_num);
+ unsigned long rs2_val = riscv_get_reg_value(regs, rs2_num);
+
+ if (riscv_insn_is_beq(insn))
+ return rs1_val == rs2_val;
+ if (riscv_insn_is_bne(insn))
+ return rs1_val != rs2_val;
+ if (riscv_insn_is_blt(insn))
+ return (long)rs1_val < (long)rs2_val;
+ if (riscv_insn_is_bge(insn))
+ return (long)rs1_val >= (long)rs2_val;
+ if (riscv_insn_is_bltu(insn))
+ return rs1_val < rs2_val;
+ if (riscv_insn_is_bgeu(insn))
+ return rs1_val >= rs2_val;
+
+ return false;
+}
+
+/**
+ * get_next_insn_address_standard() - Compute the next PC for standard RISC-V
+ * control-flow instructions
+ *
+ * @regs: Register state of the current context.
+ * @insn: Instruction located at @pc.
+ * @pc: Address of the current instruction.
+ *
+ * Determine the address of the next instruction to be executed after @insn.
+ * The function evaluates control-flow instructions whose target cannot be
+ * obtained by simply advancing the program counter:
+ *
+ * - Conditional branches: returns either the branch target or @pc + 4
+ * depending on the branch outcome.
+ * - JAL: returns the jump target encoded in the instruction.
+ * - JALR: returns the computed indirect jump target using the instruction
+ * immediate and the value of the source register.
+ * - SRET: returns @pc, as control transfer is handled by the trap return
+ * mechanism.
+ *
+ * For all other instructions, execution is assumed to continue at the next
+ * sequential 32-bit instruction and @pc + 4 is returned.
+ *
+ * Return: Address of the next instruction to be executed.
+ */
+static unsigned long get_next_insn_address_standard(struct pt_regs *regs, u32 insn,
+ unsigned long pc)
+{
+ unsigned int rs1_num;
+
+ if ((insn & __INSN_OPCODE_MASK) == __INSN_BRANCH_OPCODE)
+ return riscv_branch_taken(regs, insn) ?
+ RV_EXTRACT_BTYPE_IMM(insn) + pc : pc + 4;
+
+ if (riscv_insn_is_jal(insn))
+ return RV_EXTRACT_JTYPE_IMM(insn) + pc;
+
+ if (riscv_insn_is_jalr(insn)) {
+ rs1_num = RV_X(insn, RVG_RS1_OPOFF, 5);
+ return riscv_jalr_target(riscv_get_reg_value(regs, rs1_num),
+ RV_EXTRACT_ITYPE_IMM(insn));
+ }
+
+ if (riscv_insn_is_sret(insn))
+ return pc;
+
+ return pc + 4;
+}
+
+/* Calculate the new address for after a step */
+unsigned long get_next_insn_address(struct pt_regs *regs, ulong insn, ulong pc)
+{
+ if (riscv_insn_is_compressed(insn))
+ return get_next_insn_address_compressed(regs, insn, pc);
+
+ return get_next_insn_address_standard(regs, insn, pc);
+}
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v8 6/8] riscv: Add shared instruction fetch and decode helpers
2026-11-07 2:39 ` [PATCH v8 6/8] riscv: Add shared instruction fetch and decode helpers Himanshu Chauhan
@ 2026-11-08 2:41 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-11-08 2:41 UTC (permalink / raw)
To: Himanshu Chauhan, Paul Walmsley, Palmer Dabbelt, Albert Ou
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-mm,
linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] riscv: missing s32 sign extension of offsets in
get_next_insn_address()
- [Low] riscv: duplicate include of asm/insn.h in traps_misaligned.c
--
commit c0540231e3e0c755203ccef775001d61bb9b1fb4
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
riscv: Add shared instruction fetch and decode helpers
This commit moves get_insn() into arch/riscv/lib/insn.c and adds helpers to
fetch instructions and compute the next PC for control-flow instructions
during software single-stepping.
> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
> index 6e8ae6c66322c..541e6a5b80394 100644
> --- a/arch/riscv/kernel/traps_misaligned.c
> +++ b/arch/riscv/kernel/traps_misaligned.c
> @@ -10,6 +10,7 @@
> #include <linux/irq.h>
> #include <linux/stringify.h>
>
> +#include <asm/insn.h>
> #include <asm/processor.h>
> #include <asm/ptrace.h>
> #include <asm/csr.h>
[Severity: Low]
This isn't a bug, but arch/riscv/kernel/traps_misaligned.c already includes
asm/insn.h a few lines below.
[ ... ]
> diff --git a/arch/riscv/lib/insn.c b/arch/riscv/lib/insn.c
> new file mode 100644
> index 0000000000000..fc44e2aa6f5f0
> --- /dev/null
> +++ b/arch/riscv/lib/insn.c
[ ... ]
> +static unsigned long get_next_insn_address_compressed(struct pt_regs *regs, u32 insn,
> + unsigned long pc)
> +{
> + unsigned int rs1_num;
> +
> + if (riscv_insn_is_c_jalr(insn) || riscv_insn_is_c_jr(insn)) {
> + rs1_num = RV_X(insn, RVC_C2_RS1_OPOFF, 5);
> + return riscv_jalr_target(riscv_get_reg_value(regs, rs1_num), 0);
> + }
> +
> + if (riscv_insn_is_c_j(insn) || riscv_insn_is_c_jal(insn))
> + return RVC_EXTRACT_JTYPE_IMM(insn) + pc;
> +
> + if (riscv_insn_is_c_beqz(insn)) {
> + rs1_num = RV_X(insn, RVC_C1_RS1_OPOFF, 3) + 8;
> + if (!rs1_num || riscv_get_reg_value(regs, rs1_num) == 0)
> + return RVC_EXTRACT_BTYPE_IMM(insn) + pc;
> + return pc + 2;
> + }
> +
> + if (riscv_insn_is_c_bnez(insn)) {
> + rs1_num = RV_X(insn, RVC_C1_RS1_OPOFF, 3) + 8;
> + if (rs1_num && riscv_get_reg_value(regs, rs1_num) != 0)
> + return RVC_EXTRACT_BTYPE_IMM(insn) + pc;
> + return pc + 2;
> + }
> +
> + return pc + 2;
> +}
[ ... ]
> +static unsigned long get_next_insn_address_standard(struct pt_regs *regs, u32 insn,
> + unsigned long pc)
> +{
> + unsigned int rs1_num;
> +
> + if ((insn & __INSN_OPCODE_MASK) == __INSN_BRANCH_OPCODE)
> + return riscv_branch_taken(regs, insn) ?
> + RV_EXTRACT_BTYPE_IMM(insn) + pc : pc + 4;
> +
> + if (riscv_insn_is_jal(insn))
> + return RV_EXTRACT_JTYPE_IMM(insn) + pc;
> +
> + if (riscv_insn_is_jalr(insn)) {
> + rs1_num = RV_X(insn, RVG_RS1_OPOFF, 5);
> + return riscv_jalr_target(riscv_get_reg_value(regs, rs1_num),
> + RV_EXTRACT_ITYPE_IMM(insn));
> + }
[Severity: High]
On 64-bit RISC-V, do RVC_EXTRACT_*_IMM(insn) and RV_EXTRACT_*_IMM(insn)
zero-extend negative branch and jump offsets to 64 bits instead of
sign-extending them?
Because insn is u32, RV_IMM_SIGN(x_) and RVC_IMM_SIGN(x_) evaluate in 32-bit
unsigned arithmetic (producing a 32-bit unsigned mask such as 0xfffff000U),
and bitwise-ORing that with the GENMASK()-based unsigned long bitfields
zero-extends the result to 64-bit unsigned long. Any backward branch,
backward jump, or negative-offset JALR in get_next_insn_address_compressed()
and get_next_insn_address_standard() will miscompute the next PC by +4 GiB,
arming software single-step breakpoints at the wrong address.
Casting the extracted immediates to s32 before adding pc or passing the
offset to riscv_jalr_target() (as done in simulate-insn.c) would sign-extend
them properly.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261107023917.40606-1-himanshu.chauhan@oss.qualcomm.com?part=6
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v8 7/8] riscv: Add software supported single stepping with mc/mc6 triggers
2026-11-07 2:39 [PATCH v8 0/8] riscv: Introduce support for hardware break/watchpoints Himanshu Chauhan
` (5 preceding siblings ...)
2026-11-07 2:39 ` [PATCH v8 6/8] riscv: Add shared instruction fetch and decode helpers Himanshu Chauhan
@ 2026-11-07 2:39 ` Himanshu Chauhan
2026-11-08 2:41 ` sashiko-bot
2026-11-07 2:39 ` [PATCH v8 8/8] perf tests: keep bp_signal memory accesses visible Himanshu Chauhan
7 siblings, 1 reply; 17+ messages in thread
From: Himanshu Chauhan @ 2026-11-07 2:39 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-kernel,
linux-mm, linux-perf-users, linux-kselftest, Himanshu Chauhan
With mc/mc6 triggers, after handling breakpoint at current pc, there
is no way to tell trigger to skip same breakpoint after the handler
returns. This can cause loop of breakpoints at the same address until
the trigger is disabled or uninstalled. Debugger may keep the breakpoints
enabled and if kernel disables it, there is a loss of state coherency.
To avoid the loop, software disables the current breakpoint, finds the
next instruction that would be executed after pc and puts a breakpoint
at that address. When this breakpoint hits, old breakpoint is reinstalled.
Signed-off-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
---
arch/riscv/include/asm/hw_breakpoint.h | 16 ++
arch/riscv/kernel/hw_breakpoint.c | 297 ++++++++++++++++++++++++-
2 files changed, 308 insertions(+), 5 deletions(-)
diff --git a/arch/riscv/include/asm/hw_breakpoint.h b/arch/riscv/include/asm/hw_breakpoint.h
index 6e8913d69df2..b3ccebb5185e 100644
--- a/arch/riscv/include/asm/hw_breakpoint.h
+++ b/arch/riscv/include/asm/hw_breakpoint.h
@@ -15,10 +15,26 @@ struct task_struct;
/* Maximum number of hardware breakpoints supported */
#define RISCV_MAX_BP 16
+enum riscv_hw_bp_step_method {
+ RISCV_HW_BP_STEP_DISABLED,
+ RISCV_HW_BP_STEP_ICOUNT,
+ RISCV_HW_BP_STEP_SW,
+};
+
struct arch_hw_breakpoint {
unsigned long address;
unsigned long len;
unsigned int type;
+
+ /*
+ * Single-step callback state. While step_method is enabled, the
+ * hardware slot is temporarily programmed as either an icount trigger
+ * or an execute trigger at next_addr, and the original trigger config
+ * is restored when that temporary trigger fires.
+ */
+ unsigned long next_addr;
+ enum riscv_hw_bp_step_method step_method;
+ bool pending_perf_event;
unsigned int match;
unsigned int chain;
unsigned int select;
diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
index 4231170d4901..72be50943937 100644
--- a/arch/riscv/kernel/hw_breakpoint.c
+++ b/arch/riscv/kernel/hw_breakpoint.c
@@ -15,6 +15,7 @@
#include <linux/cpu.h>
#include <linux/cpuhotplug.h>
+#include <asm/insn.h>
#include <asm/sbi.h>
#define RISCV_HW_BP_NUM_MAX 32
@@ -62,6 +63,9 @@ enum {
#define RISCV_DBTR_MC6_HIT1 BIT(25)
#define RISCV_DBTR_MC6_HIT_MASK (RISCV_DBTR_MC6_HIT | RISCV_DBTR_MC6_HIT1)
+#define RISCV_DBTR_ICOUNT_U BIT(6)
+#define RISCV_DBTR_ICOUNT_COUNT_MASK GENMASK(23, 10)
+
#define RISCV_DBTR_MC6_HIT_FALSE 0
#define RISCV_DBTR_MC6_HIT_BEFORE 1
#define RISCV_DBTR_MC6_HIT_AFTER 2
@@ -77,6 +81,7 @@ static union sbi_dbtr_shmem_entry __percpu *sbi_dbtr_shmem;
/* number of debug triggers on this cpu . */
static int dbtr_total_num __ro_after_init;
static int dbtr_type __ro_after_init;
+static bool dbtr_icount_available __ro_after_init;
static unsigned long dbtr_init __ro_after_init;
#define MEM_HI(_m) \
@@ -194,6 +199,17 @@ static void init_sbi_dbtr(void)
return;
}
+ tdata1 = dbtr_tdata1_type(RISCV_DBTR_TRIG_ICOUNT);
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
+ tdata1, 0, 0, 0, 0, 0);
+ if (ret.error) {
+ pr_warn("Failed to detect icount triggers\n");
+ } else if (ret.value) {
+ dbtr_icount_available = 1;
+ pr_warn("Icount trigger available.\n");
+ }
+
tdata1 = dbtr_tdata1_type(RISCV_DBTR_TRIG_MCONTROL6);
ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
@@ -397,6 +413,9 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
hw->tdata1 = 0;
hw->tdata2 = attr->bp_addr;
hw->tdata3 = 0x0;
+ hw->next_addr = 0x0;
+ hw->step_method = RISCV_HW_BP_STEP_DISABLED;
+ hw->pending_perf_event = false;
switch (dbtr_type) {
case RISCV_DBTR_TRIG_MCONTROL:
@@ -414,6 +433,168 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
return ret;
}
+static ulong get_step_address(struct pt_regs *regs, ulong insn)
+{
+ return get_next_insn_address(regs, insn, regs->epc);
+}
+
+static int dbtr_update_event_tdata(struct perf_event *event,
+ unsigned long tdata1,
+ unsigned long tdata2,
+ unsigned long tdata3, bool wait);
+
+static bool hw_breakpoint_event_active(struct perf_event *event)
+{
+ return READ_ONCE(event->state) == PERF_EVENT_STATE_ACTIVE;
+}
+
+static void clear_singlestep_state(struct arch_hw_breakpoint *bp)
+{
+ bp->step_method = RISCV_HW_BP_STEP_DISABLED;
+ bp->next_addr = 0;
+ bp->pending_perf_event = false;
+}
+
+static void disable_unhandled_singlestep(struct perf_event *event,
+ struct pt_regs *regs, bool deliver_event,
+ int err)
+{
+ struct arch_hw_breakpoint *bp = counter_arch_bp(event);
+
+ /*
+ * If the temporary step trigger cannot be installed or restored, leave
+ * the CPU with no active trigger instead of re-entering the same debug
+ * exception forever.
+ */
+ pr_warn("single-step setup failed for bp_addr=0x%lx epc=0x%lx: %d; disabling event\n",
+ bp->address, regs->epc, err);
+ clear_singlestep_state(bp);
+ if (deliver_event && hw_breakpoint_event_active(event))
+ perf_bp_event(event, regs);
+ if (hw_breakpoint_event_active(event))
+ perf_event_disable_inatomic(event);
+}
+
+static unsigned long icount_step_tdata1(void)
+{
+ return dbtr_tdata1_type(RISCV_DBTR_TRIG_ICOUNT) |
+ FIELD_PREP(RISCV_DBTR_ICOUNT_COUNT_MASK, 1) |
+ RISCV_DBTR_ICOUNT_U;
+}
+
+static int restore_original_breakpoint(struct perf_event *event)
+{
+ struct arch_hw_breakpoint *bp = counter_arch_bp(event);
+ int ret;
+
+ /*
+ * Rebuild the original trigger from the perf attributes instead of
+ * keeping a second copy of the trigger data. Do not update the
+ * temporary trigger in place when changing back from icount/software-step
+ * mode: uninstall/install gives the original breakpoint a fresh hardware
+ * slot and clears any transient hit state.
+ */
+ arch_uninstall_hw_breakpoint(event);
+
+ ret = hw_breakpoint_arch_parse(NULL, &event->attr, bp);
+ if (ret)
+ return ret;
+
+ return arch_install_hw_breakpoint(event);
+}
+
+/*
+ * setup_singlestep - Set the breakpoint to next instruction after current breakpoint.
+ */
+static int setup_singlestep(struct perf_event *event, struct pt_regs *regs)
+{
+ struct arch_hw_breakpoint *bp = counter_arch_bp(event);
+ unsigned long insn, next_addr = 0;
+ int ret;
+ struct arch_hw_breakpoint tmp = {};
+
+ clear_singlestep_state(bp);
+
+ if (dbtr_icount_available && !arch_check_bp_in_kernelspace(bp)) {
+ /*
+ * User breakpoints can use an icount trigger to retire exactly
+ * one instruction. The sdtrig icount path reports EPC after the
+ * counted instruction, so the callback sees the post-step PC
+ * without decoding the original instruction here.
+ */
+ tmp.tdata1 = icount_step_tdata1();
+
+ ret = dbtr_update_event_tdata(event, tmp.tdata1, 0, 0, false);
+ if (ret)
+ return ret;
+
+ bp->step_method = RISCV_HW_BP_STEP_ICOUNT;
+ bp->next_addr = 0;
+ return 0;
+ }
+
+ if (!event->hw.target) {
+ /*
+ * A CPU-wide software-step trigger follows one task's decoded
+ * next PC. If that task migrates, the per-CPU slot may wait
+ * forever for an address the CPU will not execute.
+ */
+ return -EOPNOTSUPP;
+ }
+
+ ret = get_insn_safe(regs, regs->epc, &insn);
+ if (ret < 0)
+ return ret;
+
+ next_addr = get_step_address(regs, insn);
+ if (next_addr == regs->epc) {
+ /*
+ * Some control transfers, notably SRET, cannot be represented
+ * as an execute breakpoint at a different decoded PC. Refuse
+ * the software step rather than programming the same address
+ * and looping on the original breakpoint.
+ */
+ return -EOPNOTSUPP;
+ }
+
+ /*
+ * Software path: update the trigger in-place to an execute
+ * breakpoint at next_addr. Build the tdata directly without
+ * calling hw_breakpoint_arch_parse() so that bp->len and bp->address
+ * remain valid for the handler's matching logic after restore.
+ */
+ tmp.tdata2 = next_addr;
+ tmp.tdata3 = 0;
+ switch (dbtr_type) {
+ case RISCV_DBTR_TRIG_MCONTROL6:
+ tmp.tdata1 = dbtr_tdata1_type(RISCV_DBTR_TRIG_MCONTROL6) |
+ RISCV_DBTR_MC6_EXEC;
+ if (user_mode(regs))
+ tmp.tdata1 |= RISCV_DBTR_MC6_U;
+ else
+ tmp.tdata1 |= RISCV_DBTR_MC6_S;
+ break;
+ case RISCV_DBTR_TRIG_MCONTROL:
+ tmp.tdata1 = dbtr_tdata1_type(RISCV_DBTR_TRIG_MCONTROL) |
+ RISCV_DBTR_MC_EXEC;
+ if (user_mode(regs))
+ tmp.tdata1 |= RISCV_DBTR_MC_U;
+ else
+ tmp.tdata1 |= RISCV_DBTR_MC_S;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ ret = dbtr_update_event_tdata(event, tmp.tdata1, next_addr, 0, false);
+ if (ret)
+ return ret;
+
+ bp->step_method = RISCV_HW_BP_STEP_SW;
+ bp->next_addr = next_addr;
+ return 0;
+}
+
static void dbtr_encode_update(union sbi_dbtr_shmem_entry *shmem,
unsigned long idx, unsigned long tdata1,
unsigned long tdata2, unsigned long tdata3)
@@ -574,12 +755,43 @@ static unsigned int mc6_read_and_clear_hit(int idx)
return hit;
}
+static int handle_triggered_breakpoint(struct perf_event *event,
+ struct pt_regs *regs)
+{
+ struct arch_hw_breakpoint *bp = counter_arch_bp(event);
+ bool defer_perf;
+ int ret;
+
+ /*
+ * Report user breakpoints after the temporary step trigger advances
+ * past the armed instruction. Otherwise signal delivery can re-enter
+ * the same execute trigger before the PC has moved.
+ */
+ defer_perf = !arch_check_bp_in_kernelspace(bp);
+ if (!defer_perf) {
+ perf_bp_event(event, regs);
+ if (!hw_breakpoint_event_active(event))
+ return NOTIFY_STOP;
+ }
+
+ ret = setup_singlestep(event, regs);
+ if (ret < 0) {
+ disable_unhandled_singlestep(event, regs, defer_perf, ret);
+ return NOTIFY_STOP;
+ }
+
+ bp->pending_perf_event = defer_perf;
+ return NOTIFY_STOP;
+}
+
/*
* HW Breakpoint/watchpoint handler
*/
static int hw_breakpoint_handler(struct die_args *args)
{
- int i, bp_ret = NOTIFY_DONE;
+ int i, ret = 0, bp_ret = NOTIFY_DONE;
+ struct perf_event *deferred_step_event = NULL;
+ struct arch_hw_breakpoint *deferred_step_bp = NULL;
struct arch_hw_breakpoint *bp;
struct perf_event *event;
@@ -590,6 +802,74 @@ static int hw_breakpoint_handler(struct die_args *args)
bp = counter_arch_bp(event);
+ if (bp->step_method != RISCV_HW_BP_STEP_DISABLED) {
+ bool pending_perf_event;
+
+ /*
+ * This trap is for the temporary trigger installed by
+ * setup_singlestep(). Software stepping must match the
+ * decoded next_addr; icount already reports the post-step
+ * PC and can restore the original trigger immediately.
+ */
+ if (bp->step_method == RISCV_HW_BP_STEP_SW &&
+ args->regs->epc != bp->next_addr) {
+ /*
+ * A nested breakpoint can fire before this
+ * event reaches its decoded next_addr, for
+ * example when perf delivers a signal handler
+ * with its own breakpoint. Let the later slot
+ * handle that trap and keep this temporary step
+ * armed. If nobody handles the trap, restore
+ * below so a real divergence does not lose the
+ * original breakpoint indefinitely.
+ */
+ deferred_step_event = event;
+ deferred_step_bp = bp;
+ continue;
+ }
+
+ if (bp->step_method == RISCV_HW_BP_STEP_ICOUNT &&
+ args->regs->epc == bp->address) {
+ unsigned long tdata1 = icount_step_tdata1();
+
+ /*
+ * A trap can be observed before the original
+ * user instruction retires, leaving EPC on the
+ * armed breakpoint. Delivering the deferred
+ * perf event here would account the same
+ * breakpoint twice, so rearm icount and wait for
+ * a callback with a post-instruction EPC.
+ */
+ ret = dbtr_update_event_tdata(event, tdata1, 0, 0, false);
+ if (ret) {
+ disable_unhandled_singlestep(event,
+ args->regs,
+ bp->pending_perf_event,
+ ret);
+ bp_ret = NOTIFY_STOP;
+ goto exit;
+ }
+ bp_ret = NOTIFY_STOP;
+ goto exit;
+ }
+
+ pending_perf_event = bp->pending_perf_event;
+
+ ret = restore_original_breakpoint(event);
+ if (ret) {
+ clear_singlestep_state(bp);
+ perf_event_disable_inatomic(event);
+ bp_ret = NOTIFY_STOP;
+ goto exit;
+ }
+
+ clear_singlestep_state(bp);
+ if (pending_perf_event && hw_breakpoint_event_active(event))
+ perf_bp_event(event, args->regs);
+ bp_ret = NOTIFY_STOP;
+ goto exit;
+ }
+
switch (event->attr.bp_type) {
/* Breakpoint */
case HW_BREAKPOINT_X:
@@ -603,8 +883,7 @@ static int hw_breakpoint_handler(struct die_args *args)
RISCV_DBTR_MC6_HIT_FALSE;
if (hit) {
- perf_bp_event(event, args->regs);
- bp_ret = NOTIFY_STOP;
+ bp_ret = handle_triggered_breakpoint(event, args->regs);
goto exit;
}
}
@@ -640,8 +919,7 @@ static int hw_breakpoint_handler(struct die_args *args)
hit |= ranges_overlap(bp_start, bp_len, stval,
sizeof(long));
if (hit) {
- perf_bp_event(event, args->regs);
- bp_ret = NOTIFY_STOP;
+ bp_ret = handle_triggered_breakpoint(event, args->regs);
goto exit;
}
}
@@ -653,6 +931,15 @@ static int hw_breakpoint_handler(struct die_args *args)
}
}
+ if (deferred_step_event) {
+ bp = deferred_step_bp;
+ ret = restore_original_breakpoint(deferred_step_event);
+ if (ret)
+ perf_event_disable_inatomic(deferred_step_event);
+ clear_singlestep_state(bp);
+ bp_ret = NOTIFY_STOP;
+ }
+
exit:
return bp_ret;
}
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v8 7/8] riscv: Add software supported single stepping with mc/mc6 triggers
2026-11-07 2:39 ` [PATCH v8 7/8] riscv: Add software supported single stepping with mc/mc6 triggers Himanshu Chauhan
@ 2026-11-08 2:41 ` sashiko-bot
0 siblings, 0 replies; 17+ messages in thread
From: sashiko-bot @ 2026-11-08 2:41 UTC (permalink / raw)
To: Himanshu Chauhan, Paul Walmsley, Palmer Dabbelt, Albert Ou
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-mm,
linux-perf-users
Thank you for your contribution! Sashiko AI review found 11 potential
issues to consider:
- [High] riscv/hw_breakpoint: single-step trigger lost across context
switches
- [High] riscv/hw_breakpoint: defer_perf retires user insn before
perf_bp_event()
- [High] riscv/hw_breakpoint: broken watchpoint check and livelock in
icount step
- [High] riscv/hw_breakpoint: deferred_step_event swallows ebreak and
drops event
- [High] riscv/hw_breakpoint: deadlock and slot leak in
restore_original_breakpoint()
- [High] riscv/hw_breakpoint: IRQ-enabled race in
restore_original_breakpoint()
- [High] riscv/hw_breakpoint: trigger left armed in
disable_unhandled_singlestep()
- [High] riscv/hw_breakpoint: CPU-wide breakpoints fail in
setup_singlestep()
- [High] riscv/hw_breakpoint: missing sign extension in
get_step_address()
- [Medium] riscv/hw_breakpoint: single-pass loop in
hw_breakpoint_handler() starves step
- [Medium] riscv/hw_breakpoint: no software-step fallback in
setup_singlestep()
--
commit d9e21ab5fc5abf89648b7ca31edbf8494d3f53a7
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
riscv: Add software supported single stepping with mc/mc6 triggers
This commit adds single-stepping support for RISC-V hardware breakpoints and
watchpoints using either Sdtrig icount triggers or software-decoded next-PC
execute triggers so that mcontrol/mcontrol6 triggers do not loop on the same
instruction.
> diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
> index 4231170d49015..72be509439378 100644
> --- a/arch/riscv/kernel/hw_breakpoint.c
> +++ b/arch/riscv/kernel/hw_breakpoint.c
[ ... ]
> @@ -414,6 +433,168 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
> return ret;
> }
>
> +static ulong get_step_address(struct pt_regs *regs, ulong insn)
> +{
> + return get_next_insn_address(regs, insn, regs->epc);
> +}
[Severity: High]
On RV64, does get_step_address() -> get_next_insn_address() fail to
sign-extend negative branch and jump immediates to 64 bits?
arch/riscv/lib/insn.c:get_next_insn_address_standard() {
if ((insn & __INSN_OPCODE_MASK) == __INSN_BRANCH_OPCODE)
return riscv_branch_taken(regs, insn) ?
RV_EXTRACT_BTYPE_IMM(insn) + pc : pc + 4;
if (riscv_insn_is_jal(insn))
return RV_EXTRACT_JTYPE_IMM(insn) + pc;
}
In get_next_insn_address_compressed() and get_next_insn_address_standard(),
insn is a u32. In RV_EXTRACT_*_IMM(insn) and RVC_EXTRACT_*_IMM(insn),
RV_IMM_SIGN(x_) and RVC_IMM_SIGN(x_) produce a 32-bit unsigned value that is
zero-extended when combined with 64-bit GENMASK() operands on 64-bit builds.
Adding that result directly to pc (or passing it to riscv_jalr_target()) puts
next_addr 4 GiB above the actual target on any taken backward branch, backward
jump, or negative-offset jalr, so the software step trigger never fires and
the original breakpoint is never restored.
> +
> +static int dbtr_update_event_tdata(struct perf_event *event,
> + unsigned long tdata1,
> + unsigned long tdata2,
> + unsigned long tdata3, bool wait);
> +
> +static bool hw_breakpoint_event_active(struct perf_event *event)
> +{
> + return READ_ONCE(event->state) == PERF_EVENT_STATE_ACTIVE;
> +}
> +
> +static void clear_singlestep_state(struct arch_hw_breakpoint *bp)
> +{
> + bp->step_method = RISCV_HW_BP_STEP_DISABLED;
> + bp->next_addr = 0;
> + bp->pending_perf_event = false;
> +}
> +
> +static void disable_unhandled_singlestep(struct perf_event *event,
> + struct pt_regs *regs, bool deliver_event,
> + int err)
> +{
> + struct arch_hw_breakpoint *bp = counter_arch_bp(event);
> +
> + /*
> + * If the temporary step trigger cannot be installed or restored, leave
> + * the CPU with no active trigger instead of re-entering the same debug
> + * exception forever.
> + */
> + pr_warn("single-step setup failed for bp_addr=0x%lx epc=0x%lx: %d; disabling event\n",
> + bp->address, regs->epc, err);
> + clear_singlestep_state(bp);
> + if (deliver_event && hw_breakpoint_event_active(event))
> + perf_bp_event(event, regs);
> + if (hw_breakpoint_event_active(event))
> + perf_event_disable_inatomic(event);
> +}
[Severity: High]
Does disable_unhandled_singlestep() leave the hardware trigger armed in DBTR
until asynchronous irq_work runs?
perf_event_disable_inatomic() only sets event->pending_disable and queues
event->pending_disable_irq without calling arch_uninstall_hw_breakpoint() or
clearing the hardware trigger on the current CPU. When setup_singlestep()
fails with interrupts disabled in the trapped context (such as a kernel
breakpoint on sret or in an IRQ-disabled region) or on !CONFIG_SMP where
irq_work waits for the timer tick, returning from do_trap_break() resumes at
regs->epc with the trigger still active in hardware and loops on the same
debug exception forever.
> +
> +static unsigned long icount_step_tdata1(void)
> +{
> + return dbtr_tdata1_type(RISCV_DBTR_TRIG_ICOUNT) |
> + FIELD_PREP(RISCV_DBTR_ICOUNT_COUNT_MASK, 1) |
> + RISCV_DBTR_ICOUNT_U;
> +}
> +
> +static int restore_original_breakpoint(struct perf_event *event)
> +{
> + struct arch_hw_breakpoint *bp = counter_arch_bp(event);
> + int ret;
> +
> + /*
> + * Rebuild the original trigger from the perf attributes instead of
> + * keeping a second copy of the trigger data. Do not update the
> + * temporary trigger in place when changing back from icount/software-step
> + * mode: uninstall/install gives the original breakpoint a fresh hardware
> + * slot and clears any transient hit state.
> + */
> + arch_uninstall_hw_breakpoint(event);
[Severity: High]
Can calling arch_uninstall_hw_breakpoint() and arch_install_hw_breakpoint()
from restore_original_breakpoint() deadlock on ecall_lock or leak a duplicate
slot in pcpu_hw_bp_events[]?
Unlike mc_read_and_clear_hit() and dbtr_update_event_tdata(..., false), which
use raw_spin_trylock_irqsave(&ecall_lock) because DIE_DEBUG can interrupt code
already holding ecall_lock on the same CPU, both arch_uninstall_hw_breakpoint()
and arch_install_hw_breakpoint() spin unconditionally on
raw_spin_lock_irqsave(&ecall_lock).
In addition, arch_uninstall_hw_breakpoint() returns void and leaves
*slot = event if dbtr_uninstall_trigger_locked() fails; calling
arch_install_hw_breakpoint() afterwards can install the same event into a
second pcpu_hw_bp_events[] slot, and later teardown only clears the first
matching slot, leaving a dangling perf_event pointer in the second slot.
> +
> + ret = hw_breakpoint_arch_parse(NULL, &event->attr, bp);
> + if (ret)
> + return ret;
> +
> + return arch_install_hw_breakpoint(event);
> +}
[Severity: High]
Because do_trap_break() enables local interrupts before calling handle_break()
for user-mode traps, can restore_original_breakpoint() race with event disable
or preemption across the unlocked window between arch_uninstall_hw_breakpoint()
and arch_install_hw_breakpoint()?
CPU0 CPU1
do_trap_break() [user_mode]
local_irq_enable()
handle_break()
...
restore_original_breakpoint(event)
arch_uninstall_hw_breakpoint(event)
perf_event_disable(event)
<IPI: __perf_event_disable>
event_sched_out(event)
event->state = PERF_EVENT_STATE_OFF
arch_install_hw_breakpoint(event)
*slot = event <-- installed while OFF; dangling pointer after free
If a disable/close IPI runs between uninstall and install,
arch_install_hw_breakpoint() reinstalls an event whose state is already
PERF_EVENT_STATE_OFF, so perf_event_release_kernel() will free the event
without calling event_sched_out() again. Similarly, if the task is preempted
in that window under CONFIG_PREEMPTION, perf_event_task_sched_in() installs
the event and restore_original_breakpoint() then installs it a second time.
Calling perf_bp_event() from hw_breakpoint_handler() and
disable_unhandled_singlestep() while local interrupts are enabled on user-mode
traps also triggers lockdep_assert_irqs_disabled() in perf_bp_event().
> +
> +/*
> + * setup_singlestep - Set the breakpoint to next instruction after current breakpoint.
> + */
> +static int setup_singlestep(struct perf_event *event, struct pt_regs *regs)
> +{
> + struct arch_hw_breakpoint *bp = counter_arch_bp(event);
> + unsigned long insn, next_addr = 0;
> + int ret;
> + struct arch_hw_breakpoint tmp = {};
> +
> + clear_singlestep_state(bp);
> +
> + if (dbtr_icount_available && !arch_check_bp_in_kernelspace(bp)) {
> + /*
> + * User breakpoints can use an icount trigger to retire exactly
> + * one instruction. The sdtrig icount path reports EPC after the
> + * counted instruction, so the callback sees the post-step PC
> + * without decoding the original instruction here.
> + */
> + tmp.tdata1 = icount_step_tdata1();
> +
> + ret = dbtr_update_event_tdata(event, tmp.tdata1, 0, 0, false);
> + if (ret)
> + return ret;
[Severity: Medium]
If dbtr_update_event_tdata() fails to reprogram the current mcontrol/mcontrol6
slot to RISCV_DBTR_TRIG_ICOUNT, should setup_singlestep() fall back to the
software single-step path instead of returning an error that disables the
breakpoint?
init_sbi_dbtr() sets dbtr_icount_available when at least one trigger on the
hart supports icount, but Sdtrig allows trigger types in tinfo to vary per
slot, so the slot allocated to an mcontrol/mcontrol6 breakpoint may not
support type 3 (icount) in place.
> +
> + bp->step_method = RISCV_HW_BP_STEP_ICOUNT;
> + bp->next_addr = 0;
> + return 0;
> + }
> +
> + if (!event->hw.target) {
> + /*
> + * A CPU-wide software-step trigger follows one task's decoded
> + * next PC. If that task migrates, the per-CPU slot may wait
> + * forever for an address the CPU will not execute.
> + */
> + return -EOPNOTSUPP;
> + }
[Severity: High]
Does rejecting !event->hw.target in setup_singlestep() break all CPU-wide
kernel breakpoints (such as register_wide_hw_breakpoint() and per-CPU perf
events) on their first hit?
hw_breakpoint_arch_parse() accepts CPU-wide events and installs them into
hardware, but when they fire, setup_singlestep() returns -EOPNOTSUPP and
disable_unhandled_singlestep() disables the event (or livelocks if irq_work
cannot run before sret). Moreover, because the !event->hw.target check is
placed after the dbtr_icount_available branch, a CPU-wide user breakpoint on a
system with icount arms a per-CPU icount trigger across a preemptible user
trap without being bound to a task, so if the task migrates before returning
to user mode the icount trap and pending perf event fire on whichever user
task runs next on that CPU.
[ ... ]
> + ret = dbtr_update_event_tdata(event, tmp.tdata1, next_addr, 0, false);
> + if (ret)
> + return ret;
> +
> + bp->step_method = RISCV_HW_BP_STEP_SW;
> + bp->next_addr = next_addr;
> + return 0;
> +}
[Severity: High]
Is the temporary single-step trigger lost if the task is context-switched
before the stepped instruction retires?
setup_singlestep() writes tmp.tdata1 and next_addr directly to the current
CPU's slot via dbtr_update_event_tdata(), leaving bp->tdata1, bp->tdata2, and
bp->tdata3 with the original breakpoint or watchpoint values. If the task is
preempted on return from do_trap_break() or faults on the stepped instruction,
perf_event_task_sched_in() -> arch_install_hw_breakpoint() programs
bp->tdata1..3 back into hardware while bp->step_method and
bp->pending_perf_event remain set.
When the task resumes at regs->epc, the original trigger fires again with
args->regs->epc != bp->next_addr (hitting the deferred_step_event fallback,
which clears bp->pending_perf_event without calling perf_bp_event()), or for a
watchpoint in RISCV_HW_BP_STEP_SW the step trigger at bp->next_addr is never
reinstalled at all.
[ ... ]
> @@ -574,12 +755,43 @@ static unsigned int mc6_read_and_clear_hit(int idx)
> return hit;
> }
>
> +static int handle_triggered_breakpoint(struct perf_event *event,
> + struct pt_regs *regs)
> +{
> + struct arch_hw_breakpoint *bp = counter_arch_bp(event);
> + bool defer_perf;
> + int ret;
> +
> + /*
> + * Report user breakpoints after the temporary step trigger advances
> + * past the armed instruction. Otherwise signal delivery can re-enter
> + * the same execute trigger before the PC has moved.
> + */
> + defer_perf = !arch_check_bp_in_kernelspace(bp);
> + if (!defer_perf) {
> + perf_bp_event(event, regs);
> + if (!hw_breakpoint_event_active(event))
> + return NOTIFY_STOP;
> + }
[Severity: High]
Does deferring perf_bp_event() until after single-stepping cause user-space
execute breakpoints to retire the breakpointed instruction before notifying
ptrace or perf?
When defer_perf is true, the kernel returns to user space, executes and
commits the instruction at bp->address, and only calls perf_bp_event(event,
args->regs) on the post-step trap with the post-step regs->epc. A ptrace
debugger therefore receives SIGTRAP only after the instruction at the
breakpoint address has already executed (or never receives it if that
instruction faults or exits), and perf samples record the post-step PC and
register state instead of the breakpoint hit.
[ ... ]
> @@ -590,6 +802,74 @@ static int hw_breakpoint_handler(struct die_args *args)
>
> bp = counter_arch_bp(event);
>
> + if (bp->step_method != RISCV_HW_BP_STEP_DISABLED) {
> + bool pending_perf_event;
> +
> + /*
> + * This trap is for the temporary trigger installed by
> + * setup_singlestep(). Software stepping must match the
> + * decoded next_addr; icount already reports the post-step
> + * PC and can restore the original trigger immediately.
> + */
> + if (bp->step_method == RISCV_HW_BP_STEP_SW &&
> + args->regs->epc != bp->next_addr) {
> + /*
> + * A nested breakpoint can fire before this
> + * event reaches its decoded next_addr, for
> + * example when perf delivers a signal handler
> + * with its own breakpoint. Let the later slot
> + * handle that trap and keep this temporary step
> + * armed. If nobody handles the trap, restore
> + * below so a real divergence does not lose the
> + * original breakpoint indefinitely.
> + */
> + deferred_step_event = event;
> + deferred_step_bp = bp;
> + continue;
> + }
> +
> + if (bp->step_method == RISCV_HW_BP_STEP_ICOUNT &&
> + args->regs->epc == bp->address) {
> + unsigned long tdata1 = icount_step_tdata1();
> +
> + /*
> + * A trap can be observed before the original
> + * user instruction retires, leaving EPC on the
> + * armed breakpoint. Delivering the deferred
> + * perf event here would account the same
> + * breakpoint twice, so rearm icount and wait for
> + * a callback with a post-instruction EPC.
> + */
> + ret = dbtr_update_event_tdata(event, tdata1, 0, 0, false);
> + if (ret) {
> + disable_unhandled_singlestep(event,
> + args->regs,
> + bp->pending_perf_event,
> + ret);
> + bp_ret = NOTIFY_STOP;
> + goto exit;
> + }
> + bp_ret = NOTIFY_STOP;
> + goto exit;
> + }
[Severity: High]
Can the RISCV_HW_BP_BP_STEP_ICOUNT handling here fail on watchpoints and
livelock on same-PC triggers or self-branches?
First, setup_singlestep() enables RISCV_HW_BP_STEP_ICOUNT for all user-space
events including watchpoints, where bp->address is the watched data address
rather than the instruction PC. Comparing args->regs->epc == bp->address is
therefore false when a watchpoint traps again before the load/store retires,
so the handler restores and rearms the watchpoint on the unretired
instruction.
Second, when args->regs->epc == bp->address is true for slot i, jumping to
exit immediately skips slots i + 1 .. dbtr_total_num - 1. If a higher-indexed
breakpoint or watchpoint triggered on the same instruction, that higher slot
remains armed and fires again on every retry while slot i keeps rearming
icount and exiting, causing an infinite trap loop.
Third, if an execute breakpoint is placed on a self-branch (such as j .) or an
ebreak instruction, args->regs->epc == bp->address remains true after the
step, so this branch rearms icount and loops forever.
Fourth, because user_mode(args->regs) is not checked, a kernel ebreak (such as
WARN_ON()) occurring while a user task has an active RISCV_HW_BP_STEP_ICOUNT
slot has args->regs->epc != bp->address and is consumed as a completed user
step with NOTIFY_STOP, looping on the unadvanced kernel ebreak.
> +
> + pending_perf_event = bp->pending_perf_event;
> +
> + ret = restore_original_breakpoint(event);
> + if (ret) {
> + clear_singlestep_state(bp);
> + perf_event_disable_inatomic(event);
> + bp_ret = NOTIFY_STOP;
> + goto exit;
> + }
> +
> + clear_singlestep_state(bp);
> + if (pending_perf_event && hw_breakpoint_event_active(event))
> + perf_bp_event(event, args->regs);
> + bp_ret = NOTIFY_STOP;
> + goto exit;
> + }
> +
> switch (event->attr.bp_type) {
> /* Breakpoint */
> case HW_BREAKPOINT_X:
> @@ -603,8 +883,7 @@ static int hw_breakpoint_handler(struct die_args *args)
> RISCV_DBTR_MC6_HIT_FALSE;
>
> if (hit) {
> - perf_bp_event(event, args->regs);
> - bp_ret = NOTIFY_STOP;
> + bp_ret = handle_triggered_breakpoint(event, args->regs);
> goto exit;
> }
> }
[Severity: Medium]
Can handling new breakpoint hits and single-step completions in a single pass
starve a pending single-step in a higher-indexed slot?
If slot 1 is single-stepping from instruction A to instruction B while slot 0
has an execute breakpoint armed at B, trapping at B matches slot 0 first and
jumps to exit before inspecting slot 1. Once execution steps from B to C,
slot 0 completes its step at C and exits again, leaving slot 1 stuck in
single-step mode with its pending perf event undelivered and its original
breakpoint at A uninstalled.
[ ... ]
> @@ -653,6 +931,15 @@ static int hw_breakpoint_handler(struct die_args *args)
> }
> }
>
> + if (deferred_step_event) {
> + bp = deferred_step_bp;
> + ret = restore_original_breakpoint(deferred_step_event);
> + if (ret)
> + perf_event_disable_inatomic(deferred_step_event);
> + clear_singlestep_state(bp);
> + bp_ret = NOTIFY_STOP;
> + }
> +
> exit:
> return bp_ret;
> }
[Severity: High]
Does the deferred_step_event fallback swallow unrelated software ebreak traps
and drop pending_perf_event?
When a slot is in RISCV_HW_BP_STEP_SW and any EXC_BREAKPOINT occurs at
args->regs->epc != bp->next_addr without matching another hardware trigger,
the trap was not caused by a hardware breakpoint (for example, a user-space
software ebreak in a signal handler or a kernel WARN_ON() / BUG() ebreak).
Returning NOTIFY_STOP here causes handle_break() to treat the software ebreak
as handled instead of delivering SIGTRAP (TRAP_BRKPT) or running report_bug(),
looping forever on a kernel WARN_ON().
This path (as well as the restore_original_breakpoint() error path above) also
calls clear_singlestep_state(bp) without delivering bp->pending_perf_event,
and if control flow diverges away from bp->next_addr (such as via siglongjmp()
from a signal handler) without hitting another breakpoint,
hw_breakpoint_handler() is never entered to restore the original breakpoint.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261107023917.40606-1-himanshu.chauhan@oss.qualcomm.com?part=7
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v8 8/8] perf tests: keep bp_signal memory accesses visible
2026-11-07 2:39 [PATCH v8 0/8] riscv: Introduce support for hardware break/watchpoints Himanshu Chauhan
` (6 preceding siblings ...)
2026-11-07 2:39 ` [PATCH v8 7/8] riscv: Add software supported single stepping with mc/mc6 triggers Himanshu Chauhan
@ 2026-11-07 2:39 ` Himanshu Chauhan
7 siblings, 0 replies; 17+ messages in thread
From: Himanshu Chauhan @ 2026-11-07 2:39 UTC (permalink / raw)
To: Paul Walmsley, Palmer Dabbelt, Albert Ou, linux-riscv
Cc: Alexandre Ghiti, Jesse Taube, Oleg Nesterov, Kees Cook,
Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Ian Rogers, Adrian Hunter, James Clark, Shuah Khan, linux-kernel,
linux-mm, linux-perf-users, linux-kselftest, Himanshu Chauhan
In the breakpoint signal handler test, a breakpoint is set on
__test_function. Keep that helper as a real call target so the breakpoint
has a stable address.
The test also relies on the watched variable being accessed as distinct
memory operations. Use READ_ONCE() and WRITE_ONCE() for those accesses
instead of making the object volatile, so the source marks the exact
operations that must be preserved.
Signed-off-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
---
tools/perf/tests/bp_signal.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/tools/perf/tests/bp_signal.c b/tools/perf/tests/bp_signal.c
index f580ba7486b1..2e4f7747d32f 100644
--- a/tools/perf/tests/bp_signal.c
+++ b/tools/perf/tests/bp_signal.c
@@ -36,15 +36,14 @@ static int fd3;
static int overflows;
static int overflows_2;
-static volatile long the_var;
-
+static long the_var;
/*
* Use ASM to ensure watchpoint and breakpoint can be triggered
* at one instruction.
*/
#if defined (__x86_64__)
-extern void __test_function(volatile long *ptr);
+extern void __test_function(long *ptr);
asm (
".pushsection .text;"
".globl __test_function\n"
@@ -54,16 +53,19 @@ asm (
"ret\n"
".popsection\n");
#else
-static void __test_function(volatile long *ptr)
+static noinline void __test_function(long *ptr)
{
- *ptr = 0x1234;
+ WRITE_ONCE(*ptr, 0x1234);
}
#endif
static noinline int test_function(void)
{
+ long value;
+
__test_function(&the_var);
- the_var++;
+ value = READ_ONCE(the_var);
+ WRITE_ONCE(the_var, value + 1);
return time(NULL);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 17+ messages in thread