RISC-V hardware breakpoint framework is built on top of perf subsystem
and uses SBI debug trigger extension to
install/uninstall/update/enable/disable hardware triggers as specified
in Sdtrig ISA extension.
Signed-off-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
---
arch/riscv/Kconfig | 1 +
arch/riscv/include/asm/hw_breakpoint.h | 332 +++++++++++++
arch/riscv/include/asm/kdebug.h | 3 +-
arch/riscv/kernel/Makefile | 1 +
arch/riscv/kernel/hw_breakpoint.c | 657 +++++++++++++++++++++++++
arch/riscv/kernel/traps.c | 6 +
6 files changed, 999 insertions(+), 1 deletion(-)
create mode 100644 arch/riscv/include/asm/hw_breakpoint.h
create mode 100644 arch/riscv/kernel/hw_breakpoint.c
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 90c531e6abf5..ac4b4175763e 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -170,6 +170,7 @@ config RISCV
select HAVE_FUNCTION_ERROR_INJECTION
select HAVE_GCC_PLUGINS
select HAVE_GENERIC_VDSO if MMU
+ select HAVE_HW_BREAKPOINT if PERF_EVENTS
select HAVE_IRQ_TIME_ACCOUNTING
select HAVE_KERNEL_BZIP2 if !XIP_KERNEL && !EFI_ZBOOT
select HAVE_KERNEL_GZIP if !XIP_KERNEL && !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..a5ef70fa14a1
--- /dev/null
+++ b/arch/riscv/include/asm/hw_breakpoint.h
@@ -0,0 +1,332 @@
+/* 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>
+
+#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
+
+#define RV_DBTR_BIT(_prefix, _name) \
+ RV_DBTR_##_prefix##_##_name##_BIT
+
+#define RV_DBTR_BIT_MASK(_prefix, _name) \
+ RV_DBTR_##_prefix##_name##_BIT_MASK
+
+#define RV_DBTR_BIT_MASK_VAL(_prefix, _name, _width) \
+ (((1UL << (_width)) - 1) << RV_DBTR_BIT(_prefix, _name))
+
+#define CLEAR_DBTR_BIT(_target, _prefix, _bit_name) \
+ __clear_bit(RV_DBTR_BIT(_prefix, _bit_name), &(_target))
+
+#define SET_DBTR_BIT(_target, _prefix, _bit_name) \
+ __set_bit(RV_DBTR_BIT(_prefix, _bit_name), &(_target))
+
+enum {
+ RV_DBTR_BP = 0,
+ RV_DBTR_WP = 1,
+};
+
+enum {
+ RV_DBTR_TRIG_NONE = 0,
+ RV_DBTR_TRIG_LEGACY,
+ RV_DBTR_TRIG_MCONTROL,
+ RV_DBTR_TRIG_ICOUNT,
+ RV_DBTR_TRIG_ITRIGGER,
+ RV_DBTR_TRIG_ETRIGGER,
+ RV_DBTR_TRIG_MCONTROL6,
+};
+
+/* Trigger Data 1 */
+enum {
+ RV_DBTR_BIT(TDATA1, DATA) = 0,
+#if __riscv_xlen == 64
+ RV_DBTR_BIT(TDATA1, DMODE) = 59,
+ RV_DBTR_BIT(TDATA1, TYPE) = 60,
+#elif __riscv_xlen == 32
+ RV_DBTR_BIT(TDATA1, DMODE) = 27,
+ RV_DBTR_BIT(TDATA1, TYPE) = 28,
+#else
+ #error "Unknown __riscv_xlen"
+#endif
+};
+
+enum {
+#if __riscv_xlen == 64
+ RV_DBTR_BIT_MASK(TDATA1, DATA) = RV_DBTR_BIT_MASK_VAL(TDATA1, DATA, 59),
+#elif __riscv_xlen == 32
+ RV_DBTR_BIT_MASK(TDATA1, DATA) = RV_DBTR_BIT_MASK_VAL(TDATA1, DATA, 27),
+#else
+ #error "Unknown __riscv_xlen"
+#endif
+ RV_DBTR_BIT_MASK(TDAT1, DMODE) = RV_DBTR_BIT_MASK_VAL(TDATA1, DMODE, 1),
+ RV_DBTR_BIT_MASK(TDATA1, TYPE) = RV_DBTR_BIT_MASK_VAL(TDATA1, TYPE, 4),
+};
+
+/* MC - Match Control Type Register */
+enum {
+ RV_DBTR_BIT(MC, LOAD) = 0,
+ RV_DBTR_BIT(MC, STORE) = 1,
+ RV_DBTR_BIT(MC, EXEC) = 2,
+ RV_DBTR_BIT(MC, U) = 3,
+ RV_DBTR_BIT(MC, S) = 4,
+ RV_DBTR_BIT(MC, RES2) = 5,
+ RV_DBTR_BIT(MC, M) = 6,
+ RV_DBTR_BIT(MC, MATCH) = 7,
+ RV_DBTR_BIT(MC, CHAIN) = 11,
+ RV_DBTR_BIT(MC, ACTION) = 12,
+ RV_DBTR_BIT(MC, SIZELO) = 16,
+ RV_DBTR_BIT(MC, TIMING) = 18,
+ RV_DBTR_BIT(MC, SELECT) = 19,
+ RV_DBTR_BIT(MC, HIT) = 20,
+#if __riscv_xlen >= 64
+ RV_DBTR_BIT(MC, SIZEHI) = 21,
+#endif
+#if __riscv_xlen == 64
+ RV_DBTR_BIT(MC, MASKMAX) = 53,
+ RV_DBTR_BIT(MC, DMODE) = 59,
+ RV_DBTR_BIT(MC, TYPE) = 60,
+#elif __riscv_xlen == 32
+ RV_DBTR_BIT(MC, MASKMAX) = 21,
+ RV_DBTR_BIT(MC, DMODE) = 27,
+ RV_DBTR_BIT(MC, TYPE) = 28,
+#else
+ #error "Unknown riscv xlen"
+#endif
+};
+
+enum {
+ RV_DBTR_BIT_MASK(MC, LOAD) = RV_DBTR_BIT_MASK_VAL(MC, LOAD, 1),
+ RV_DBTR_BIT_MASK(MC, STORE) = RV_DBTR_BIT_MASK_VAL(MC, STORE, 1),
+ RV_DBTR_BIT_MASK(MC, EXEC) = RV_DBTR_BIT_MASK_VAL(MC, EXEC, 1),
+ RV_DBTR_BIT_MASK(MC, U) = RV_DBTR_BIT_MASK_VAL(MC, U, 1),
+ RV_DBTR_BIT_MASK(MC, S) = RV_DBTR_BIT_MASK_VAL(MC, S, 1),
+ RV_DBTR_BIT_MASK(MC, RES2) = RV_DBTR_BIT_MASK_VAL(MC, RES2, 1),
+ RV_DBTR_BIT_MASK(MC, M) = RV_DBTR_BIT_MASK_VAL(MC, M, 1),
+ RV_DBTR_BIT_MASK(MC, MATCH) = RV_DBTR_BIT_MASK_VAL(MC, MATCH, 4),
+ RV_DBTR_BIT_MASK(MC, CHAIN) = RV_DBTR_BIT_MASK_VAL(MC, CHAIN, 1),
+ RV_DBTR_BIT_MASK(MC, ACTION) = RV_DBTR_BIT_MASK_VAL(MC, ACTION, 4),
+ RV_DBTR_BIT_MASK(MC, SIZELO) = RV_DBTR_BIT_MASK_VAL(MC, SIZELO, 2),
+ RV_DBTR_BIT_MASK(MC, TIMING) = RV_DBTR_BIT_MASK_VAL(MC, TIMING, 1),
+ RV_DBTR_BIT_MASK(MC, SELECT) = RV_DBTR_BIT_MASK_VAL(MC, SELECT, 1),
+ RV_DBTR_BIT_MASK(MC, HIT) = RV_DBTR_BIT_MASK_VAL(MC, HIT, 1),
+#if __riscv_xlen >= 64
+ RV_DBTR_BIT_MASK(MC, SIZEHI) = RV_DBTR_BIT_MASK_VAL(MC, SIZEHI, 2),
+#endif
+ RV_DBTR_BIT_MASK(MC, MASKMAX) = RV_DBTR_BIT_MASK_VAL(MC, MASKMAX, 6),
+ RV_DBTR_BIT_MASK(MC, DMODE) = RV_DBTR_BIT_MASK_VAL(MC, DMODE, 1),
+ RV_DBTR_BIT_MASK(MC, TYPE) = RV_DBTR_BIT_MASK_VAL(MC, TYPE, 4),
+};
+
+/* MC6 - Match Control 6 Type Register */
+enum {
+ RV_DBTR_BIT(MC6, LOAD) = 0,
+ RV_DBTR_BIT(MC6, STORE) = 1,
+ RV_DBTR_BIT(MC6, EXEC) = 2,
+ RV_DBTR_BIT(MC6, U) = 3,
+ RV_DBTR_BIT(MC6, S) = 4,
+ RV_DBTR_BIT(MC6, RES2) = 5,
+ RV_DBTR_BIT(MC6, M) = 6,
+ RV_DBTR_BIT(MC6, MATCH) = 7,
+ RV_DBTR_BIT(MC6, CHAIN) = 11,
+ RV_DBTR_BIT(MC6, ACTION) = 12,
+ RV_DBTR_BIT(MC6, SIZE) = 16,
+ RV_DBTR_BIT(MC6, TIMING) = 20,
+ RV_DBTR_BIT(MC6, SELECT) = 21,
+ RV_DBTR_BIT(MC6, HIT) = 22,
+ RV_DBTR_BIT(MC6, VU) = 23,
+ RV_DBTR_BIT(MC6, VS) = 24,
+#if __riscv_xlen == 64
+ RV_DBTR_BIT(MC6, DMODE) = 59,
+ RV_DBTR_BIT(MC6, TYPE) = 60,
+#elif __riscv_xlen == 32
+ RV_DBTR_BIT(MC6, DMODE) = 27,
+ RV_DBTR_BIT(MC6, TYPE) = 28,
+#else
+ #error "Unknown riscv xlen"
+#endif
+};
+
+enum {
+ RV_DBTR_BIT_MASK(MC6, LOAD) = RV_DBTR_BIT_MASK_VAL(MC6, LOAD, 1),
+ RV_DBTR_BIT_MASK(MC6, STORE) = RV_DBTR_BIT_MASK_VAL(MC6, STORE, 1),
+ RV_DBTR_BIT_MASK(MC6, EXEC) = RV_DBTR_BIT_MASK_VAL(MC6, EXEC, 1),
+ RV_DBTR_BIT_MASK(MC6, U) = RV_DBTR_BIT_MASK_VAL(MC6, U, 1),
+ RV_DBTR_BIT_MASK(MC6, S) = RV_DBTR_BIT_MASK_VAL(MC6, S, 1),
+ RV_DBTR_BIT_MASK(MC6, RES2) = RV_DBTR_BIT_MASK_VAL(MC6, RES2, 1),
+ RV_DBTR_BIT_MASK(MC6, M) = RV_DBTR_BIT_MASK_VAL(MC6, M, 1),
+ RV_DBTR_BIT_MASK(MC6, MATCH) = RV_DBTR_BIT_MASK_VAL(MC6, MATCH, 4),
+ RV_DBTR_BIT_MASK(MC6, CHAIN) = RV_DBTR_BIT_MASK_VAL(MC6, CHAIN, 1),
+ RV_DBTR_BIT_MASK(MC6, ACTION) = RV_DBTR_BIT_MASK_VAL(MC6, ACTION, 4),
+ RV_DBTR_BIT_MASK(MC6, SIZE) = RV_DBTR_BIT_MASK_VAL(MC6, SIZE, 4),
+ RV_DBTR_BIT_MASK(MC6, TIMING) = RV_DBTR_BIT_MASK_VAL(MC6, TIMING, 1),
+ RV_DBTR_BIT_MASK(MC6, SELECT) = RV_DBTR_BIT_MASK_VAL(MC6, SELECT, 1),
+ RV_DBTR_BIT_MASK(MC6, HIT) = RV_DBTR_BIT_MASK_VAL(MC6, HIT, 1),
+ RV_DBTR_BIT_MASK(MC6, VU) = RV_DBTR_BIT_MASK_VAL(MC6, VU, 1),
+ RV_DBTR_BIT_MASK(MC6, VS) = RV_DBTR_BIT_MASK_VAL(MC6, VS, 1),
+#if __riscv_xlen == 64
+ RV_DBTR_BIT_MASK(MC6, DMODE) = RV_DBTR_BIT_MASK_VAL(MC6, DMODE, 1),
+ RV_DBTR_BIT_MASK(MC6, TYPE) = RV_DBTR_BIT_MASK_VAL(MC6, TYPE, 4),
+#elif __riscv_xlen == 32
+ RV_DBTR_BIT_MASK(MC6, DMODE) = RV_DBTR_BIT_MASK_VAL(MC6, DMODE, 1),
+ RV_DBTR_BIT_MASK(MC6, TYPE) = RV_DBTR_BIT_MASK_VAL(MC6, TYPE, 4),
+#else
+ #error "Unknown riscv xlen"
+#endif
+};
+
+#define RV_DBTR_SET_TDATA1_TYPE(_t1, _type) \
+ ({ \
+ typeof(_t1) (td1t1) = (_t1); \
+ (td1t1) &= ~RV_DBTR_BIT_MASK(TDATA1, TYPE); \
+ (td1t1) |= (((unsigned long)(_type) \
+ << RV_DBTR_BIT(TDATA1, TYPE)) \
+ & RV_DBTR_BIT_MASK(TDATA1, TYPE)); \
+ (td1t1); \
+ })
+
+#define RV_DBTR_SET_MC_TYPE(_t1, _type) \
+ ({ \
+ typeof(_t1) (mct1) = (_t1); \
+ (mct1) &= ~RV_DBTR_BIT_MASK(MC, TYPE); \
+ (mct1) |= (((unsigned long)(_type) \
+ << RV_DBTR_BIT(MC, TYPE)) \
+ & RV_DBTR_BIT_MASK(MC, TYPE)); \
+ (mct1); \
+ })
+
+#define RV_DBTR_SET_MC6_TYPE(_t1, _type) \
+ ({ \
+ typeof(_t1) (mc6t1) = (_t1); \
+ (mc6t1) &= ~RV_DBTR_BIT_MASK(MC6, TYPE); \
+ (mc6t1) |= (((unsigned long)(_type) \
+ << RV_DBTR_BIT(MC6, TYPE)) \
+ & RV_DBTR_BIT_MASK(MC6, TYPE)); \
+ (mc6t1); \
+ })
+
+#define RV_DBTR_SET_MC_EXEC_BIT(_t1) \
+ SET_DBTR_BIT(_t1, MC, EXEC)
+
+#define RV_DBTR_SET_MC_LOAD_BIT(_t1) \
+ SET_DBTR_BIT(_t1, MC, LOAD)
+
+#define RV_DBTR_SET_MC_STORE_BIT(_t1) \
+ SET_DBTR_BIT(_t1, MC, STORE)
+
+#define RV_DBTR_SET_MC_SIZELO(_t1, _val) \
+ ({ \
+ typeof(_t1) (mcslt1) = (_t1); \
+ mcslt1 &= ~RV_DBTR_BIT_MASK(MC, SIZELO); \
+ mcslt1 |= (((_val) << RV_DBTR_BIT(MC, SIZELO)) \
+ & RV_DBTR_BIT_MASK(MC, SIZELO)); \
+ (mcslt1); \
+ })
+
+#define RV_DBTR_SET_MC_SIZEHI(_t1, _val) \
+ ({ \
+ typeof(_t1) (mcsht1) = (_t1); \
+ mcsht1 &= ~RV_DBTR_BIT_MASK(MC, SIZEHI); \
+ mcsht1 |= (((_val) << RV_DBTR_BIT(MC, SIZEHI)) \
+ & RV_DBTR_BIT_MASK(MC, SIZEHI)); \
+ (mcsht1); \
+ })
+
+#define RV_DBTR_SET_MC6_EXEC_BIT(_t1) \
+ SET_DBTR_BIT(_t1, MC6, EXEC)
+
+#define RV_DBTR_SET_MC6_LOAD_BIT(_t1) \
+ SET_DBTR_BIT(_t1, MC6, LOAD)
+
+#define RV_DBTR_SET_MC6_STORE_BIT(_t1) \
+ SET_DBTR_BIT(_t1, MC6, STORE)
+
+#define RV_DBTR_SET_MC6_SIZE(_t1, _val) \
+ ({ \
+ typeof(_t1) (mc6szt1) = (_t1); \
+ (mc6szt1) &= ~RV_DBTR_BIT_MASK(MC6, SIZE); \
+ (mc6szt1) |= (((_val) << RV_DBTR_BIT(MC6, SIZE)) \
+ & RV_DBTR_BIT_MASK(MC6, SIZE)); \
+ (mc6szt1); \
+ })
+
+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;
+};
+
+/* Maximum number of hardware breakpoints supported */
+#define HW_BP_NUM_MAX 32
+
+struct perf_event_attr;
+struct notifier_block;
+struct perf_event;
+struct pt_regs;
+
+int hw_breakpoint_slots(int type);
+int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw);
+int hw_breakpoint_arch_parse(struct perf_event *bp,
+ const struct perf_event_attr *attr,
+ struct arch_hw_breakpoint *hw);
+int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
+ unsigned long val, void *data);
+
+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);
+void clear_ptrace_hw_breakpoint(struct task_struct *tsk);
+void flush_ptrace_hw_breakpoint(struct task_struct *tsk);
+
+#else
+
+int hw_breakpoint_slots(int type)
+{
+ return 0;
+}
+
+static inline void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
+{
+}
+
+static inline void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
+{
+}
+
+void arch_enable_hw_breakpoint(struct perf_event *bp)
+{
+}
+
+void arch_update_hw_breakpoint(struct perf_event *bp)
+{
+}
+
+void arch_disable_hw_breakpoint(struct perf_event *bp)
+{
+}
+
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
+#endif /* __RISCV_HW_BREAKPOINT_H */
diff --git a/arch/riscv/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 cabb99cadfb6..590a280762c9 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..8076829d23d3
--- /dev/null
+++ b/arch/riscv/kernel/hw_breakpoint.c
@@ -0,0 +1,657 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2026 Qualcomm Technologies, Inc.
+ */
+
+#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/cpu.h>
+#include <linux/cpuhotplug.h>
+
+#include <asm/sbi.h>
+
+/* Registered per-cpu bp/wp */
+static DEFINE_PER_CPU(struct perf_event *, pcpu_hw_bp_events[HW_BP_NUM_MAX]);
+static DEFINE_PER_CPU(unsigned long, ecall_lock_flags);
+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 int dbtr_init __ro_after_init;
+
+#if __riscv_xlen == 64
+#define MEM_HI(_m) 0
+#define MEM_LO(_m) ((u64)(_m))
+#elif __riscv_xlen == 32
+#define MEM_HI(_m) ((u64)(_m) >> 32)
+#define MEM_LO(_m) ((u64)(_m) & 0xFFFFFFFFUL)
+#else
+#error "Unknown __riscv_xlen"
+#endif
+
+static int arch_smp_setup_sbi_shmem(unsigned int cpu)
+{
+ union sbi_dbtr_shmem_entry *dbtr_shmem;
+ unsigned long shmem_pa;
+ struct sbiret ret;
+ int rc = 0;
+
+ dbtr_shmem = per_cpu_ptr(sbi_dbtr_shmem, cpu);
+ if (!dbtr_shmem) {
+ pr_err("Invalid per-cpu shared memory for debug triggers\n");
+ return -ENODEV;
+ }
+
+ shmem_pa = __pa(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) {
+ switch (ret.error) {
+ case SBI_ERR_DENIED:
+ pr_warn("%s: Access denied for shared memory at %lx\n",
+ __func__, shmem_pa);
+ rc = -EPERM;
+ break;
+
+ case SBI_ERR_INVALID_PARAM:
+ case SBI_ERR_INVALID_ADDRESS:
+ pr_warn("%s: Invalid address parameter (%lu)\n",
+ __func__, ret.error);
+ rc = -EINVAL;
+ break;
+
+ case SBI_ERR_ALREADY_AVAILABLE:
+ pr_warn("%s: Shared memory is already set\n",
+ __func__);
+ rc = -EADDRINUSE;
+ break;
+
+ case SBI_ERR_FAILURE:
+ pr_err("%s: Internal sdtrig state error\n",
+ __func__);
+ rc = -ENXIO;
+ break;
+
+ default:
+ pr_warn("%s: Unknown error %lu\n", __func__, ret.error);
+ rc = -ENXIO;
+ break;
+ }
+ }
+
+ pr_info("CPU %d: HW Breakpoint shared memory registered.\n", cpu);
+
+ return rc;
+}
+
+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,
+ -1UL, -1UL, 0, 0, 0, 0);
+
+ if (ret.error) {
+ switch (ret.error) {
+ case SBI_ERR_DENIED:
+ pr_err("%s: Access denied for shared memory.\n",
+ __func__);
+ break;
+
+ case SBI_ERR_INVALID_PARAM:
+ case SBI_ERR_INVALID_ADDRESS:
+ pr_err("%s: Invalid address parameter (%lu)\n",
+ __func__, ret.error);
+ break;
+
+ case SBI_ERR_ALREADY_AVAILABLE:
+ pr_err("%s: Shared memory is already set\n",
+ __func__);
+ break;
+ case SBI_ERR_FAILURE:
+ pr_err("%s: Internal sdtrig state error\n",
+ __func__);
+ break;
+ default:
+ pr_err("%s: Unknown error %lu\n", __func__, ret.error);
+ break;
+ }
+ }
+
+ pr_warn("CPU %d: HW Breakpoint shared memory disabled.\n", cpu);
+
+ return 0;
+}
+
+static void init_sbi_dbtr(void)
+{
+ unsigned long tdata1;
+ struct sbiret ret;
+
+ if (sbi_probe_extension(SBI_EXT_DBTR) <= 0) {
+ pr_warn("%s: SBI_EXT_DBTR is not supported\n", __func__);
+ dbtr_total_num = 0;
+ goto done;
+ }
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
+ 0, 0, 0, 0, 0, 0);
+ if (ret.error) {
+ pr_warn("%s: Failed to detect triggers\n", __func__);
+ dbtr_total_num = 0;
+ goto done;
+ }
+
+ tdata1 = 0;
+ tdata1 = RV_DBTR_SET_TDATA1_TYPE(tdata1, RV_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("%s: failed to detect mcontrol6 triggers\n", __func__);
+ } else if (!ret.value) {
+ pr_warn("%s: type 6 triggers not available\n", __func__);
+ } else {
+ dbtr_total_num = ret.value;
+ dbtr_type = RV_DBTR_TRIG_MCONTROL6;
+ pr_warn("%s: mcontrol6 trigger available.\n", __func__);
+ goto done;
+ }
+
+ /* fallback to type 2 triggers if type 6 is not available */
+
+ tdata1 = 0;
+ tdata1 = RV_DBTR_SET_TDATA1_TYPE(tdata1, RV_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("%s: failed to detect mcontrol triggers\n", __func__);
+ } else if (!ret.value) {
+ pr_warn("%s: type 2 triggers not available\n", __func__);
+ } else {
+ dbtr_total_num = ret.value;
+ dbtr_type = RV_DBTR_TRIG_MCONTROL;
+ goto done;
+ }
+
+done:
+ dbtr_init = 1;
+}
+
+int hw_breakpoint_slots(int type)
+{
+ /*
+ * We can be called early, so don't rely on
+ * static variables being initialised.
+ */
+
+ if (!dbtr_init)
+ init_sbi_dbtr();
+
+ return dbtr_total_num;
+}
+
+int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw)
+{
+ unsigned int len;
+ unsigned long va;
+
+ va = hw->address;
+ len = hw->len;
+
+ return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
+}
+
+static int rv_init_mcontrol_trigger(const struct perf_event_attr *attr,
+ struct arch_hw_breakpoint *hw)
+{
+ switch (attr->bp_type) {
+ case HW_BREAKPOINT_X:
+ hw->type = RV_DBTR_BP;
+ RV_DBTR_SET_MC_EXEC_BIT(hw->tdata1);
+ break;
+ case HW_BREAKPOINT_R:
+ hw->type = RV_DBTR_WP;
+ RV_DBTR_SET_MC_LOAD_BIT(hw->tdata1);
+ break;
+ case HW_BREAKPOINT_W:
+ hw->type = RV_DBTR_WP;
+ RV_DBTR_SET_MC_STORE_BIT(hw->tdata1);
+ break;
+ case HW_BREAKPOINT_RW:
+ hw->type = RV_DBTR_WP;
+ RV_DBTR_SET_MC_LOAD_BIT(hw->tdata1);
+ RV_DBTR_SET_MC_STORE_BIT(hw->tdata1);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ switch (attr->bp_len) {
+ case HW_BREAKPOINT_LEN_1:
+ hw->len = 1;
+ hw->tdata1 = RV_DBTR_SET_MC_SIZELO(hw->tdata1, 1);
+ break;
+ case HW_BREAKPOINT_LEN_2:
+ hw->len = 2;
+ hw->tdata1 = RV_DBTR_SET_MC_SIZELO(hw->tdata1, 2);
+ break;
+ case HW_BREAKPOINT_LEN_4:
+ hw->len = 4;
+ hw->tdata1 = RV_DBTR_SET_MC_SIZELO(hw->tdata1, 3);
+ break;
+#if __riscv_xlen >= 64
+ case HW_BREAKPOINT_LEN_8:
+ hw->len = 8;
+ hw->tdata1 = RV_DBTR_SET_MC_SIZELO(hw->tdata1, 1);
+ hw->tdata1 = RV_DBTR_SET_MC_SIZEHI(hw->tdata1, 1);
+ break;
+#endif
+ default:
+ return -EINVAL;
+ }
+
+ hw->tdata1 = RV_DBTR_SET_MC_TYPE(hw->tdata1, RV_DBTR_TRIG_MCONTROL);
+
+ CLEAR_DBTR_BIT(hw->tdata1, MC, DMODE);
+ CLEAR_DBTR_BIT(hw->tdata1, MC, TIMING);
+ CLEAR_DBTR_BIT(hw->tdata1, MC, SELECT);
+ CLEAR_DBTR_BIT(hw->tdata1, MC, ACTION);
+ CLEAR_DBTR_BIT(hw->tdata1, MC, CHAIN);
+ CLEAR_DBTR_BIT(hw->tdata1, MC, MATCH);
+ CLEAR_DBTR_BIT(hw->tdata1, MC, M);
+
+ SET_DBTR_BIT(hw->tdata1, MC, S);
+ SET_DBTR_BIT(hw->tdata1, MC, U);
+
+ return 0;
+}
+
+static int rv_init_mcontrol6_trigger(const struct perf_event_attr *attr,
+ struct arch_hw_breakpoint *hw)
+{
+ switch (attr->bp_type) {
+ case HW_BREAKPOINT_X:
+ hw->type = RV_DBTR_BP;
+ RV_DBTR_SET_MC6_EXEC_BIT(hw->tdata1);
+ break;
+ case HW_BREAKPOINT_R:
+ hw->type = RV_DBTR_WP;
+ RV_DBTR_SET_MC6_LOAD_BIT(hw->tdata1);
+ break;
+ case HW_BREAKPOINT_W:
+ hw->type = RV_DBTR_WP;
+ RV_DBTR_SET_MC6_STORE_BIT(hw->tdata1);
+ break;
+ case HW_BREAKPOINT_RW:
+ hw->type = RV_DBTR_WP;
+ RV_DBTR_SET_MC6_STORE_BIT(hw->tdata1);
+ RV_DBTR_SET_MC6_LOAD_BIT(hw->tdata1);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ switch (attr->bp_len) {
+ case HW_BREAKPOINT_LEN_1:
+ hw->len = 1;
+ hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 1);
+ break;
+ case HW_BREAKPOINT_LEN_2:
+ hw->len = 2;
+ hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 2);
+ break;
+ case HW_BREAKPOINT_LEN_4:
+ hw->len = 4;
+ hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 3);
+ break;
+ case HW_BREAKPOINT_LEN_8:
+ hw->len = 8;
+ hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 5);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ hw->tdata1 = RV_DBTR_SET_MC6_TYPE(hw->tdata1, RV_DBTR_TRIG_MCONTROL6);
+
+ CLEAR_DBTR_BIT(hw->tdata1, MC6, DMODE);
+ CLEAR_DBTR_BIT(hw->tdata1, MC6, TIMING);
+ CLEAR_DBTR_BIT(hw->tdata1, MC6, SELECT);
+ CLEAR_DBTR_BIT(hw->tdata1, MC6, ACTION);
+ CLEAR_DBTR_BIT(hw->tdata1, MC6, CHAIN);
+ CLEAR_DBTR_BIT(hw->tdata1, MC6, MATCH);
+ CLEAR_DBTR_BIT(hw->tdata1, MC6, M);
+ CLEAR_DBTR_BIT(hw->tdata1, MC6, VS);
+ CLEAR_DBTR_BIT(hw->tdata1, MC6, VU);
+
+ SET_DBTR_BIT(hw->tdata1, MC6, S);
+ SET_DBTR_BIT(hw->tdata1, MC6, U);
+
+ 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->tdata2 = attr->bp_addr;
+ hw->tdata3 = 0x0;
+
+ switch (dbtr_type) {
+ case RV_DBTR_TRIG_MCONTROL:
+ ret = rv_init_mcontrol_trigger(attr, hw);
+ break;
+ case RV_DBTR_TRIG_MCONTROL6:
+ ret = rv_init_mcontrol6_trigger(attr, hw);
+ break;
+ default:
+ pr_warn("unsupported trigger type\n");
+ ret = -EOPNOTSUPP;
+ break;
+ }
+
+ return ret;
+}
+
+/*
+ * HW Breakpoint/watchpoint handler
+ */
+static int hw_breakpoint_handler(struct die_args *args)
+{
+ int ret = NOTIFY_DONE;
+ struct arch_hw_breakpoint *bp;
+ struct perf_event *event;
+ int i;
+
+ 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 (bp->type) {
+ /* Breakpoint */
+ case RV_DBTR_BP:
+ if (bp->address == args->regs->epc) {
+ perf_bp_event(event, args->regs);
+ ret = NOTIFY_STOP;
+ }
+ break;
+
+ /* Watchpoint */
+ case RV_DBTR_WP:
+ if (bp->address == csr_read(CSR_STVAL)) {
+ perf_bp_event(event, args->regs);
+ ret = NOTIFY_STOP;
+ }
+ break;
+
+ default:
+ pr_warn("%s: Unknown type: %u\n", __func__, bp->type);
+ break;
+ }
+ }
+
+ return ret;
+}
+
+int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
+ unsigned long val, void *data)
+{
+ if (val != DIE_DEBUG)
+ return NOTIFY_DONE;
+
+ return hw_breakpoint_handler(data);
+}
+
+/* atomic: counter->ctx->lock is held */
+int arch_install_hw_breakpoint(struct perf_event *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;
+ struct sbiret ret;
+ int err = 0;
+
+ raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
+ *this_cpu_ptr(&ecall_lock_flags));
+
+ xmit = &shmem->data;
+ recv = &shmem->id;
+ 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("%s: failed to install trigger\n", __func__);
+ err = -EIO;
+ goto done;
+ }
+
+ idx = le_to_cpu(recv->idx);
+ if (idx >= dbtr_total_num) {
+ pr_warn("%s: invalid trigger index %lu\n", __func__, idx);
+ err = -EINVAL;
+ goto done;
+ }
+
+ slot = this_cpu_ptr(&pcpu_hw_bp_events[idx]);
+ if (*slot) {
+ pr_warn("%s: slot %lu is in use\n", __func__, idx);
+ err = -EBUSY;
+ goto done;
+ }
+
+ pr_debug("Trigger %lu installed at index 0x%lx\n", bp->tdata2, idx);
+
+ /* Save the event - to be looked up in handler */
+ *slot = event;
+
+done:
+ raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock),
+ *this_cpu_ptr(&ecall_lock_flags));
+ return err;
+}
+
+/* atomic: counter->ctx->lock is held */
+void arch_uninstall_hw_breakpoint(struct perf_event *event)
+{
+ struct sbiret ret;
+ int i;
+
+ for (i = 0; i < dbtr_total_num; i++) {
+ struct perf_event **slot = this_cpu_ptr(&pcpu_hw_bp_events[i]);
+
+ if (*slot == event) {
+ *slot = NULL;
+ break;
+ }
+ }
+
+ if (i == dbtr_total_num) {
+ pr_warn("%s: Breakpoint not installed.\n", __func__);
+ return;
+ }
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_UNINSTALL,
+ i, 1, 0, 0, 0, 0);
+ if (ret.error)
+ pr_warn("%s: Failed to uninstall trigger %d.\n", __func__, i);
+}
+
+void arch_enable_hw_breakpoint(struct perf_event *event)
+{
+ struct sbiret ret;
+ int i;
+ struct perf_event **slot;
+
+ 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("%s: Breakpoint not installed.\n", __func__);
+ return;
+ }
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_ENABLE,
+ i, 1, 0, 0, 0, 0);
+
+ if (ret.error) {
+ pr_warn("%s: Failed to install trigger %d\n", __func__, i);
+ return;
+ }
+}
+EXPORT_SYMBOL_GPL(arch_enable_hw_breakpoint);
+
+void arch_update_hw_breakpoint(struct perf_event *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 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("%s: Breakpoint not installed.\n", __func__);
+ return;
+ }
+
+ raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
+ *this_cpu_ptr(&ecall_lock_flags));
+
+ xmit = &shmem->data;
+ 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_UPDATE,
+ i, 1, 0, 0, 0, 0);
+ if (ret.error)
+ pr_warn("%s: Failed to update trigger %d.\n", __func__, i);
+
+ raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock),
+ *this_cpu_ptr(&ecall_lock_flags));
+}
+EXPORT_SYMBOL_GPL(arch_update_hw_breakpoint);
+
+void arch_disable_hw_breakpoint(struct perf_event *event)
+{
+ struct sbiret ret;
+ int i;
+
+ for (i = 0; i < dbtr_total_num; i++) {
+ struct perf_event **slot = this_cpu_ptr(&pcpu_hw_bp_events[i]);
+
+ if (*slot == event)
+ break;
+ }
+
+ if (i == dbtr_total_num) {
+ pr_warn("%s: Breakpoint not installed.\n", __func__);
+ return;
+ }
+
+ ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_DISABLE,
+ i, 1, 0, 0, 0, 0);
+
+ if (ret.error) {
+ pr_warn("%s: Failed to uninstall trigger %d.\n", __func__, i);
+ return;
+ }
+}
+EXPORT_SYMBOL_GPL(arch_disable_hw_breakpoint);
+
+void hw_breakpoint_pmu_read(struct perf_event *bp)
+{
+ /* TODO */
+}
+
+void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
+{
+ /* TODO */
+}
+
+void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
+{
+ /* TODO */
+}
+
+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));
+
+ if (!dbtr_init)
+ init_sbi_dbtr();
+
+ if (dbtr_total_num) {
+ pr_info("%s: total number of type %d triggers: %u\n",
+ __func__, dbtr_type, dbtr_total_num);
+ } else {
+ pr_info("%s: No hardware triggers available\n", __func__);
+ 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("%s: Failed to allocate shared memory.\n", __func__);
+ 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("%s: Failed to setup CPU hotplug state\n", __func__);
+ free_percpu(sbi_dbtr_shmem);
+ return rc;
+ }
+ out:
+ return rc;
+}
+arch_initcall(arch_hw_breakpoint_init);
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 5fb57fad188a..3024abd26952 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -288,6 +288,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.0Hi,
Thank you for this patch, I have one question regarding the configuration of the size field:
On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
>+ case HW_BREAKPOINT_LEN_1:
>+ hw->len = 1;
>+ hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 1);
>+ break;
>+ case HW_BREAKPOINT_LEN_2:
>+ hw->len = 2;
>+ hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 2);
>+ break;
>+ case HW_BREAKPOINT_LEN_4:
>+ hw->len = 4;
>+ hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 3);
>+ break;
>+ case HW_BREAKPOINT_LEN_8:
>+ hw->len = 8;
>+ hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 5);
>+ break;
GDB's gdbarch_breakpoint_from_pc method returns len=2 for non-aligned addresses, causing size mismatch with hardware triggers.
A simple test is as follows:
root@k3:~# cat test.c
#include <stdio.h>
int a = 0;
int main()
{
printf("start test\n");
a = 1;
printf("a = %d\n", a);
printf("end test\n");
return 0;
}
root@k3:~# gcc -march=rv64gc -g test.c -o test
root@k3:~# gdb test
...
start
...
Temporary breakpoint 1, main () at test.c:6
6 printf("start test\n");
(gdb) x/8i $pc
=> 0x2aaaaaa6ea <main+8>: auipc a0,0x0
0x2aaaaaa6ee <main+12>: addi a0,a0,86
0x2aaaaaa6f2 <main+16>: jal 0x2aaaaaa5d0 <puts@plt>
0x2aaaaaa6f6 <main+20>: auipc a5,0x2
0x2aaaaaa6fa <main+24>: addi a5,a5,-1770
0x2aaaaaa6fe <main+28>: li a4,1
0x2aaaaaa700 <main+30>: sw a4,0(a5)
0x2aaaaaa702 <main+32>: auipc a5,0x2
(gdb) hbreak *0x2aaaaaa6f2
Hardware assisted breakpoint 2 at 0x2aaaaaa6f2: file test.c, line 6.
(gdb) c
Continuing.
start test
a = 1
end test
[Inferior 1 (process 1784) exited normally]
(gdb)
root@k3:~# gcc -march=rv64g -g test.c -o test
root@k3:~# gdb test
...
start
...
Temporary breakpoint 1, main () at test.c:6
6 printf("start test\n");
(gdb) x/8i $pc
=> 0x2aaaaaa6f4 <main+16>: auipc a0,0x0
0x2aaaaaa6f8 <main+20>: addi a0,a0,100
0x2aaaaaa6fc <main+24>: jal 0x2aaaaaa5d0 <puts@plt>
0x2aaaaaa700 <main+28>: auipc a5,0x2
0x2aaaaaa704 <main+32>: addi a5,a5,-1780
0x2aaaaaa708 <main+36>: li a4,1
0x2aaaaaa70c <main+40>: sw a4,0(a5)
0x2aaaaaa710 <main+44>: auipc a5,0x2
(gdb) hbreak *0x2aaaaaa6fc
Hardware assisted breakpoint 2 at 0x2aaaaaa6fc: file test.c, line 6.
(gdb) c
Continuing.
Breakpoint 2, 0x0000002aaaaaa6fc in main () at test.c:6
6 printf("start test\n");
(gdb)
As a result, hardware breakpoints set on 16-bit instruction addresses may fail to trigger due to this size mismatch. So can we consider setting the SIZE field to 0 (match any size), hardware triggers match memory accesses of any size.
Hi Liangzhen,
Sorry I was busy with ptrace implementation on this patch series.
Which is done now and I will send it after the next revision of this
patch.
On Fri, Apr 3, 2026 at 1:09 PM liangzhen <liangzhen@linux.spacemit.com> wrote:
>
> Hi,
>
> Thank you for this patch, I have one question regarding the configuration of the size field:
>
> On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
>
> >+ case HW_BREAKPOINT_LEN_1:
> >+ hw->len = 1;
> >+ hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 1);
> >+ break;
> >+ case HW_BREAKPOINT_LEN_2:
> >+ hw->len = 2;
> >+ hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 2);
> >+ break;
> >+ case HW_BREAKPOINT_LEN_4:
> >+ hw->len = 4;
> >+ hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 3);
> >+ break;
> >+ case HW_BREAKPOINT_LEN_8:
> >+ hw->len = 8;
> >+ hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 5);
> >+ break;
>
> GDB's gdbarch_breakpoint_from_pc method returns len=2 for non-aligned addresses, causing size mismatch with hardware triggers.
>
> A simple test is as follows:
>
> root@k3:~# cat test.c
> #include <stdio.h>
>
> int a = 0;
> int main()
> {
> printf("start test\n");
> a = 1;
> printf("a = %d\n", a);
> printf("end test\n");
> return 0;
> }
>
>
> root@k3:~# gcc -march=rv64gc -g test.c -o test
> root@k3:~# gdb test
> ...
> start
> ...
> Temporary breakpoint 1, main () at test.c:6
> 6 printf("start test\n");
> (gdb) x/8i $pc
> => 0x2aaaaaa6ea <main+8>: auipc a0,0x0
> 0x2aaaaaa6ee <main+12>: addi a0,a0,86
> 0x2aaaaaa6f2 <main+16>: jal 0x2aaaaaa5d0 <puts@plt>
> 0x2aaaaaa6f6 <main+20>: auipc a5,0x2
> 0x2aaaaaa6fa <main+24>: addi a5,a5,-1770
> 0x2aaaaaa6fe <main+28>: li a4,1
> 0x2aaaaaa700 <main+30>: sw a4,0(a5)
> 0x2aaaaaa702 <main+32>: auipc a5,0x2
> (gdb) hbreak *0x2aaaaaa6f2
> Hardware assisted breakpoint 2 at 0x2aaaaaa6f2: file test.c, line 6.
> (gdb) c
> Continuing.
> start test
> a = 1
> end test
> [Inferior 1 (process 1784) exited normally]
> (gdb)
>
> root@k3:~# gcc -march=rv64g -g test.c -o test
> root@k3:~# gdb test
> ...
> start
> ...
> Temporary breakpoint 1, main () at test.c:6
> 6 printf("start test\n");
> (gdb) x/8i $pc
> => 0x2aaaaaa6f4 <main+16>: auipc a0,0x0
> 0x2aaaaaa6f8 <main+20>: addi a0,a0,100
> 0x2aaaaaa6fc <main+24>: jal 0x2aaaaaa5d0 <puts@plt>
> 0x2aaaaaa700 <main+28>: auipc a5,0x2
> 0x2aaaaaa704 <main+32>: addi a5,a5,-1780
> 0x2aaaaaa708 <main+36>: li a4,1
> 0x2aaaaaa70c <main+40>: sw a4,0(a5)
> 0x2aaaaaa710 <main+44>: auipc a5,0x2
> (gdb) hbreak *0x2aaaaaa6fc
> Hardware assisted breakpoint 2 at 0x2aaaaaa6fc: file test.c, line 6.
> (gdb) c
> Continuing.
>
> Breakpoint 2, 0x0000002aaaaaa6fc in main () at test.c:6
> 6 printf("start test\n");
> (gdb)
>
> As a result, hardware breakpoints set on 16-bit instruction addresses may fail to trigger due to this size mismatch. So can we consider setting the SIZE field to 0 (match any size), hardware triggers match memory accesses of any size.
>
I think you have a good test case here. We can set size to 0 if
specifically asked or in the default case.
Do you have a patch set against GDB to test this out?
Thanks
Regards
Himanshu
I implemented the ptrace and GDB breakpoint support based on this
patch series. When setting the SIZE field to 0, the test case works
correctly.
On 4/6/2026 12:48 PM, Himanshu Chauhan wrote:
> Hi Liangzhen,
>
> Sorry I was busy with ptrace implementation on this patch series.
> Which is done now and I will send it after the next revision of this
> patch.
>
> On Fri, Apr 3, 2026 at 1:09 PM liangzhen <liangzhen@linux.spacemit.com> wrote:
>> Hi,
>>
>> Thank you for this patch, I have one question regarding the configuration of the size field:
>>
>> On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
>>
>>> + case HW_BREAKPOINT_LEN_1:
>>> + hw->len = 1;
>>> + hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 1);
>>> + break;
>>> + case HW_BREAKPOINT_LEN_2:
>>> + hw->len = 2;
>>> + hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 2);
>>> + break;
>>> + case HW_BREAKPOINT_LEN_4:
>>> + hw->len = 4;
>>> + hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 3);
>>> + break;
>>> + case HW_BREAKPOINT_LEN_8:
>>> + hw->len = 8;
>>> + hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 5);
>>> + break;
>> GDB's gdbarch_breakpoint_from_pc method returns len=2 for non-aligned addresses, causing size mismatch with hardware triggers.
>>
>> A simple test is as follows:
>>
>> root@k3:~# cat test.c
>> #include <stdio.h>
>>
>> int a = 0;
>> int main()
>> {
>> printf("start test\n");
>> a = 1;
>> printf("a = %d\n", a);
>> printf("end test\n"); return 0; } root@k3:~# gcc -march=rv64gc -g test.c -o test root@k3:~# gdb test ... start ... Temporary breakpoint 1, main () at test.c:6 6 printf("start test\n");
>> (gdb) x/8i $pc
>> => 0x2aaaaaa6ea <main+8>: auipc a0,0x0
>> 0x2aaaaaa6ee <main+12>: addi a0,a0,86
>> 0x2aaaaaa6f2 <main+16>: jal 0x2aaaaaa5d0 <puts@plt>
>> 0x2aaaaaa6f6 <main+20>: auipc a5,0x2
>> 0x2aaaaaa6fa <main+24>: addi a5,a5,-1770
>> 0x2aaaaaa6fe <main+28>: li a4,1
>> 0x2aaaaaa700 <main+30>: sw a4,0(a5)
>> 0x2aaaaaa702 <main+32>: auipc a5,0x2
>> (gdb) hbreak *0x2aaaaaa6f2
>> Hardware assisted breakpoint 2 at 0x2aaaaaa6f2: file test.c, line 6.
>> (gdb) c
>> Continuing.
>> start test
>> a = 1
>> end test
>> [Inferior 1 (process 1784) exited normally]
>> (gdb)
>>
>> root@k3:~# gcc -march=rv64g -g test.c -o test
>> root@k3:~# gdb test
>> ...
>> start
>> ...
>> Temporary breakpoint 1, main () at test.c:6
>> 6 printf("start test\n");
>> (gdb) x/8i $pc
>> => 0x2aaaaaa6f4 <main+16>: auipc a0,0x0
>> 0x2aaaaaa6f8 <main+20>: addi a0,a0,100
>> 0x2aaaaaa6fc <main+24>: jal 0x2aaaaaa5d0 <puts@plt>
>> 0x2aaaaaa700 <main+28>: auipc a5,0x2
>> 0x2aaaaaa704 <main+32>: addi a5,a5,-1780
>> 0x2aaaaaa708 <main+36>: li a4,1
>> 0x2aaaaaa70c <main+40>: sw a4,0(a5)
>> 0x2aaaaaa710 <main+44>: auipc a5,0x2
>> (gdb) hbreak *0x2aaaaaa6fc
>> Hardware assisted breakpoint 2 at 0x2aaaaaa6fc: file test.c, line 6.
>> (gdb) c
>> Continuing.
>>
>> Breakpoint 2, 0x0000002aaaaaa6fc in main () at test.c:6
>> 6 printf("start test\n");
>> (gdb)
>>
>> As a result, hardware breakpoints set on 16-bit instruction addresses may fail to trigger due to this size mismatch. So can we consider setting the SIZE field to 0 (match any size), hardware triggers match memory accesses of any size.
>>
> I think you have a good test case here. We can set size to 0 if
> specifically asked or in the default case.
> Do you have a patch set against GDB to test this out?
>
> Thanks
> Regards
> Himanshu
>
Hi,
On Tue, Apr 7, 2026 at 7:18 AM liangzhen <liangzhen@linux.spacemit.com> wrote:
>
> I implemented the ptrace and GDB breakpoint support based on this
> patch series. When setting the SIZE field to 0, the test case works
> correctly.
>
Noted. I will be floating the ptrace support immediately after the
revised version of this series. I would request you to test the ptrace
with your GDB breakpoint support. Please let me know when you are
sending patch series for GDB.
Thank you
Regards
Himanshu
>
> On 4/6/2026 12:48 PM, Himanshu Chauhan wrote:
> > Hi Liangzhen,
> >
> > Sorry I was busy with ptrace implementation on this patch series.
> > Which is done now and I will send it after the next revision of this
> > patch.
> >
> > On Fri, Apr 3, 2026 at 1:09 PM liangzhen <liangzhen@linux.spacemit.com> wrote:
> >> Hi,
> >>
> >> Thank you for this patch, I have one question regarding the configuration of the size field:
> >>
> >> On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> >>
> >>> + case HW_BREAKPOINT_LEN_1:
> >>> + hw->len = 1;
> >>> + hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 1);
> >>> + break;
> >>> + case HW_BREAKPOINT_LEN_2:
> >>> + hw->len = 2;
> >>> + hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 2);
> >>> + break;
> >>> + case HW_BREAKPOINT_LEN_4:
> >>> + hw->len = 4;
> >>> + hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 3);
> >>> + break;
> >>> + case HW_BREAKPOINT_LEN_8:
> >>> + hw->len = 8;
> >>> + hw->tdata1 = RV_DBTR_SET_MC6_SIZE(hw->tdata1, 5);
> >>> + break;
> >> GDB's gdbarch_breakpoint_from_pc method returns len=2 for non-aligned addresses, causing size mismatch with hardware triggers.
> >>
> >> A simple test is as follows:
> >>
> >> root@k3:~# cat test.c
> >> #include <stdio.h>
> >>
> >> int a = 0;
> >> int main()
> >> {
> >> printf("start test\n");
> >> a = 1;
> >> printf("a = %d\n", a);
> >> printf("end test\n"); return 0; } root@k3:~# gcc -march=rv64gc -g test.c -o test root@k3:~# gdb test ... start ... Temporary breakpoint 1, main () at test.c:6 6 printf("start test\n");
> >> (gdb) x/8i $pc
> >> => 0x2aaaaaa6ea <main+8>: auipc a0,0x0
> >> 0x2aaaaaa6ee <main+12>: addi a0,a0,86
> >> 0x2aaaaaa6f2 <main+16>: jal 0x2aaaaaa5d0 <puts@plt>
> >> 0x2aaaaaa6f6 <main+20>: auipc a5,0x2
> >> 0x2aaaaaa6fa <main+24>: addi a5,a5,-1770
> >> 0x2aaaaaa6fe <main+28>: li a4,1
> >> 0x2aaaaaa700 <main+30>: sw a4,0(a5)
> >> 0x2aaaaaa702 <main+32>: auipc a5,0x2
> >> (gdb) hbreak *0x2aaaaaa6f2
> >> Hardware assisted breakpoint 2 at 0x2aaaaaa6f2: file test.c, line 6.
> >> (gdb) c
> >> Continuing.
> >> start test
> >> a = 1
> >> end test
> >> [Inferior 1 (process 1784) exited normally]
> >> (gdb)
> >>
> >> root@k3:~# gcc -march=rv64g -g test.c -o test
> >> root@k3:~# gdb test
> >> ...
> >> start
> >> ...
> >> Temporary breakpoint 1, main () at test.c:6
> >> 6 printf("start test\n");
> >> (gdb) x/8i $pc
> >> => 0x2aaaaaa6f4 <main+16>: auipc a0,0x0
> >> 0x2aaaaaa6f8 <main+20>: addi a0,a0,100
> >> 0x2aaaaaa6fc <main+24>: jal 0x2aaaaaa5d0 <puts@plt>
> >> 0x2aaaaaa700 <main+28>: auipc a5,0x2
> >> 0x2aaaaaa704 <main+32>: addi a5,a5,-1780
> >> 0x2aaaaaa708 <main+36>: li a4,1
> >> 0x2aaaaaa70c <main+40>: sw a4,0(a5)
> >> 0x2aaaaaa710 <main+44>: auipc a5,0x2
> >> (gdb) hbreak *0x2aaaaaa6fc
> >> Hardware assisted breakpoint 2 at 0x2aaaaaa6fc: file test.c, line 6.
> >> (gdb) c
> >> Continuing.
> >>
> >> Breakpoint 2, 0x0000002aaaaaa6fc in main () at test.c:6
> >> 6 printf("start test\n");
> >> (gdb)
> >>
> >> As a result, hardware breakpoints set on 16-bit instruction addresses may fail to trigger due to this size mismatch. So can we consider setting the SIZE field to 0 (match any size), hardware triggers match memory accesses of any size.
> >>
> > I think you have a good test case here. We can set size to 0 if
> > specifically asked or in the default case.
> > Do you have a patch set against GDB to test this out?
> >
> > Thanks
> > Regards
> > Himanshu
> >
Hi, On 4/7/2026 12:53 PM, Himanshu Chauhan wrote: > Noted. I will be floating the ptrace support immediately after the > revised version of this series. I would request you to test the ptrace > with your GDB breakpoint support. Please let me know when you are > sending patch series for GDB. Sure, I will do that. And the GDB changes has also been posted: - https://patchwork.sourceware.org/project/gdb/patch/20260407101214.2744-1-zhuangqiubin@linux.spacemit.com/
Hi, Himanshu
I have been using your previous HBP implementation for some time and have
encountered some of its shortcomings, which this implementation has also
inherited.
On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> +/*
> + * HW Breakpoint/watchpoint handler
> + */
> +static int hw_breakpoint_handler(struct die_args *args)
> +{
> + int ret = NOTIFY_DONE;
> + struct arch_hw_breakpoint *bp;
> + struct perf_event *event;
> + int i;
> +
> + 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 (bp->type) {
> + /* Breakpoint */
> + case RV_DBTR_BP:
> + if (bp->address == args->regs->epc) {
> + perf_bp_event(event, args->regs);
> + ret = NOTIFY_STOP;
> + }
> + break;
> +
> + /* Watchpoint */
> + case RV_DBTR_WP:
> + if (bp->address == csr_read(CSR_STVAL)) {
Sdtrig spec (5.7.11 and 5.7.12 in the tdata1.select field description) states that:
"There is at least (!) one compare value and it contains the lowest virtual
address of the access. In addition, it is recommended that there are additional
compare values for the other accessed virtual addresses match. (E.g. on a 32-bit
read from 0x4000, the lowest address is 0x4000 and the other addresses are
0x4001, 0x4002, and 0x4003".
A 32-bit read from 0x4000 will definitely have compare value 0x4000 (the lowest
virtual address), but may also have compare values at 0x4001, 0x4002, and 0x4003.
This means that a 16-bit watchpoint at 0x4002 will fire on this 32-bit read, yet
stval will be 0x4000, and the current hw_breakpoint_handler implementation will
not detect it.
Therefore, it is important to check tdata1.hit bit (which indicates that the
trigger condition was definitely met) and only use the comparison between
bp->address and CSR_STVAL as a fallback (if tdata1.hit is not supported by the
hardware).
> +/* 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;
> + struct sbiret ret;
> + int err = 0;
> +
> + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> + *this_cpu_ptr(&ecall_lock_flags));
> +
> + xmit = &shmem->data;
> + recv = &shmem->id;
> + 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);
perf installs breakpoints (via the call chain pmu.add->arch_install_hw_breakpoint)
during context rotation after a perf_event_open or ptrace call finishes.
So, if the hardware does not support the given tdata1, tdata2 and tdata3
configuration and OpenSBI fails to set up the trigger, the kernel will still
report that the breakpoint was successfully installed.
A probing stage is therefore needed before or during perf event
registration to ensure that all breakpoints can be installed
successfully during context rotation.
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Ilya,
On Thu, Mar 19, 2026 at 4:06 PM Ilya Mamay <mmamayka01@gmail.com> wrote:
>
> Hi, Himanshu
>
> I have been using your previous HBP implementation for some time and have
> encountered some of its shortcomings, which this implementation has also
> inherited.
>
> On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> > +/*
> > + * HW Breakpoint/watchpoint handler
> > + */
> > +static int hw_breakpoint_handler(struct die_args *args)
> > +{
> > + int ret = NOTIFY_DONE;
> > + struct arch_hw_breakpoint *bp;
> > + struct perf_event *event;
> > + int i;
> > +
> > + 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 (bp->type) {
> > + /* Breakpoint */
> > + case RV_DBTR_BP:
> > + if (bp->address == args->regs->epc) {
>
> > + perf_bp_event(event, args->regs);
> > + ret = NOTIFY_STOP;
> > + }
> > + break;
> > +
> > + /* Watchpoint */
> > + case RV_DBTR_WP:
> > + if (bp->address == csr_read(CSR_STVAL)) {
>
> Sdtrig spec (5.7.11 and 5.7.12 in the tdata1.select field description) states that:
> "There is at least (!) one compare value and it contains the lowest virtual
> address of the access. In addition, it is recommended that there are additional
> compare values for the other accessed virtual addresses match. (E.g. on a 32-bit
> read from 0x4000, the lowest address is 0x4000 and the other addresses are
> 0x4001, 0x4002, and 0x4003".
>
> A 32-bit read from 0x4000 will definitely have compare value 0x4000 (the lowest
> virtual address), but may also have compare values at 0x4001, 0x4002, and 0x4003.
> This means that a 16-bit watchpoint at 0x4002 will fire on this 32-bit read, yet
> stval will be 0x4000, and the current hw_breakpoint_handler implementation will
> not detect it.
>
> Therefore, it is important to check tdata1.hit bit (which indicates that the
> trigger condition was definitely met) and only use the comparison between
> bp->address and CSR_STVAL as a fallback (if tdata1.hit is not supported by the
> hardware).
>
You are right. I will add this in my next revision.
Thanks & Regards
Himanshu
> > +/* 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;
> > + struct sbiret ret;
> > + int err = 0;
> > +
> > + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> > + *this_cpu_ptr(&ecall_lock_flags));
> > +
> > + xmit = &shmem->data;
> > + recv = &shmem->id;
> > + 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);
>
> perf installs breakpoints (via the call chain pmu.add->arch_install_hw_breakpoint)
> during context rotation after a perf_event_open or ptrace call finishes.
> So, if the hardware does not support the given tdata1, tdata2 and tdata3
> configuration and OpenSBI fails to set up the trigger, the kernel will still
> report that the breakpoint was successfully installed.
>
> A probing stage is therefore needed before or during perf event
> registration to ensure that all breakpoints can be installed
> successfully during context rotation.
>
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Himanshu,
Thank you for your reply regarding the tdata1.hit check. However, there is
a second issue I raised in my previous message that seems to have been
overlooked. I also used the wrong terminology in my previous message and would
like to clarify the issue:
Currently, arch_install_hw_breakpoint() calls SBI_EXT_DBTR_TRIG_INSTALL only
when the perf event is being scheduled on the tracee. If that call fails
(e.g., because the hardware WARL fields do not accept the requested
tdata1-tdata3 values), there is no way to notify the tracer or reject the
breakpoint earlier. The breakpoint will simply never fire, and the tracer only
discovers this after the fact by reading the perf_event state – by which time
the tracee may have already passed the point of interest.
There is no way in the current implementation for the tracer to verify that the
breakpoint cannot be installed because of WARL constraints.
I understand that a full solution might be non-trivial. However, could you
at least share your thoughts on how this could be addressed?
Regards,
Ilya
On Mon, Apr 06, 2026 at 10:19:30AM +0530, Himanshu Chauhan wrote:
> Hi Ilya,
>
> On Thu, Mar 19, 2026 at 4:06 PM Ilya Mamay <mmamayka01@gmail.com> wrote:
> >
> > Hi, Himanshu
> >
> > I have been using your previous HBP implementation for some time and have
> > encountered some of its shortcomings, which this implementation has also
> > inherited.
> >
> > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> > > +/*
> > > + * HW Breakpoint/watchpoint handler
> > > + */
> > > +static int hw_breakpoint_handler(struct die_args *args)
> > > +{
> > > + int ret = NOTIFY_DONE;
> > > + struct arch_hw_breakpoint *bp;
> > > + struct perf_event *event;
> > > + int i;
> > > +
> > > + 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 (bp->type) {
> > > + /* Breakpoint */
> > > + case RV_DBTR_BP:
> > > + if (bp->address == args->regs->epc) {
> >
> > > + perf_bp_event(event, args->regs);
> > > + ret = NOTIFY_STOP;
> > > + }
> > > + break;
> > > +
> > > + /* Watchpoint */
> > > + case RV_DBTR_WP:
> > > + if (bp->address == csr_read(CSR_STVAL)) {
> >
> > Sdtrig spec (5.7.11 and 5.7.12 in the tdata1.select field description) states that:
> > "There is at least (!) one compare value and it contains the lowest virtual
> > address of the access. In addition, it is recommended that there are additional
> > compare values for the other accessed virtual addresses match. (E.g. on a 32-bit
> > read from 0x4000, the lowest address is 0x4000 and the other addresses are
> > 0x4001, 0x4002, and 0x4003".
> >
> > A 32-bit read from 0x4000 will definitely have compare value 0x4000 (the lowest
> > virtual address), but may also have compare values at 0x4001, 0x4002, and 0x4003.
> > This means that a 16-bit watchpoint at 0x4002 will fire on this 32-bit read, yet
> > stval will be 0x4000, and the current hw_breakpoint_handler implementation will
> > not detect it.
> >
> > Therefore, it is important to check tdata1.hit bit (which indicates that the
> > trigger condition was definitely met) and only use the comparison between
> > bp->address and CSR_STVAL as a fallback (if tdata1.hit is not supported by the
> > hardware).
> >
> You are right. I will add this in my next revision.
>
> Thanks & Regards
> Himanshu
>
> > > +/* 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;
> > > + struct sbiret ret;
> > > + int err = 0;
> > > +
> > > + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> > > + *this_cpu_ptr(&ecall_lock_flags));
> > > +
> > > + xmit = &shmem->data;
> > > + recv = &shmem->id;
> > > + 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);
> >
> > perf installs breakpoints (via the call chain pmu.add->arch_install_hw_breakpoint)
> > during context rotation after a perf_event_open or ptrace call finishes.
> > So, if the hardware does not support the given tdata1, tdata2 and tdata3
> > configuration and OpenSBI fails to set up the trigger, the kernel will still
> > report that the breakpoint was successfully installed.
> >
> > A probing stage is therefore needed before or during perf event
> > registration to ensure that all breakpoints can be installed
> > successfully during context rotation.
> >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Ilya,
On Tue, Apr 7, 2026 at 5:04 PM Ilya Mamay <mmamayka01@gmail.com> wrote:
>
> Hi Himanshu,
>
> Thank you for your reply regarding the tdata1.hit check. However, there is
> a second issue I raised in my previous message that seems to have been
> overlooked. I also used the wrong terminology in my previous message and would
> like to clarify the issue:
>
> Currently, arch_install_hw_breakpoint() calls SBI_EXT_DBTR_TRIG_INSTALL only
> when the perf event is being scheduled on the tracee. If that call fails
> (e.g., because the hardware WARL fields do not accept the requested
> tdata1-tdata3 values), there is no way to notify the tracer or reject the
> breakpoint earlier. The breakpoint will simply never fire, and the tracer only
> discovers this after the fact by reading the perf_event state – by which time
> the tracee may have already passed the point of interest.
>
> There is no way in the current implementation for the tracer to verify that the
> breakpoint cannot be installed because of WARL constraints.
>
> I understand that a full solution might be non-trivial. However, could you
> at least share your thoughts on how this could be addressed?
>
You have a valid point here. But by design the arch_install/uninstalls
are called as and when events are scheduled based on scheduling of the
tracee. I believe this problem should be with all architectures where
setting breaking is not as simple as a register write (I am thinking
x86 here). As of now, I don't have a solution for it. But I will think
about it unless you have something on top of your head.
Regards
Himanshu
> Regards,
> Ilya
>
> On Mon, Apr 06, 2026 at 10:19:30AM +0530, Himanshu Chauhan wrote:
> > Hi Ilya,
> >
> > On Thu, Mar 19, 2026 at 4:06 PM Ilya Mamay <mmamayka01@gmail.com> wrote:
> > >
> > > Hi, Himanshu
> > >
> > > I have been using your previous HBP implementation for some time and have
> > > encountered some of its shortcomings, which this implementation has also
> > > inherited.
> > >
> > > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> > > > +/*
> > > > + * HW Breakpoint/watchpoint handler
> > > > + */
> > > > +static int hw_breakpoint_handler(struct die_args *args)
> > > > +{
> > > > + int ret = NOTIFY_DONE;
> > > > + struct arch_hw_breakpoint *bp;
> > > > + struct perf_event *event;
> > > > + int i;
> > > > +
> > > > + 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 (bp->type) {
> > > > + /* Breakpoint */
> > > > + case RV_DBTR_BP:
> > > > + if (bp->address == args->regs->epc) {
> > >
> > > > + perf_bp_event(event, args->regs);
> > > > + ret = NOTIFY_STOP;
> > > > + }
> > > > + break;
> > > > +
> > > > + /* Watchpoint */
> > > > + case RV_DBTR_WP:
> > > > + if (bp->address == csr_read(CSR_STVAL)) {
> > >
> > > Sdtrig spec (5.7.11 and 5.7.12 in the tdata1.select field description) states that:
> > > "There is at least (!) one compare value and it contains the lowest virtual
> > > address of the access. In addition, it is recommended that there are additional
> > > compare values for the other accessed virtual addresses match. (E.g. on a 32-bit
> > > read from 0x4000, the lowest address is 0x4000 and the other addresses are
> > > 0x4001, 0x4002, and 0x4003".
> > >
> > > A 32-bit read from 0x4000 will definitely have compare value 0x4000 (the lowest
> > > virtual address), but may also have compare values at 0x4001, 0x4002, and 0x4003.
> > > This means that a 16-bit watchpoint at 0x4002 will fire on this 32-bit read, yet
> > > stval will be 0x4000, and the current hw_breakpoint_handler implementation will
> > > not detect it.
> > >
> > > Therefore, it is important to check tdata1.hit bit (which indicates that the
> > > trigger condition was definitely met) and only use the comparison between
> > > bp->address and CSR_STVAL as a fallback (if tdata1.hit is not supported by the
> > > hardware).
> > >
> > You are right. I will add this in my next revision.
> >
> > Thanks & Regards
> > Himanshu
> >
> > > > +/* 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;
> > > > + struct sbiret ret;
> > > > + int err = 0;
> > > > +
> > > > + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> > > > + *this_cpu_ptr(&ecall_lock_flags));
> > > > +
> > > > + xmit = &shmem->data;
> > > > + recv = &shmem->id;
> > > > + 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);
> > >
> > > perf installs breakpoints (via the call chain pmu.add->arch_install_hw_breakpoint)
> > > during context rotation after a perf_event_open or ptrace call finishes.
> > > So, if the hardware does not support the given tdata1, tdata2 and tdata3
> > > configuration and OpenSBI fails to set up the trigger, the kernel will still
> > > report that the breakpoint was successfully installed.
> > >
> > > A probing stage is therefore needed before or during perf event
> > > registration to ensure that all breakpoints can be installed
> > > successfully during context rotation.
> > >
> > > > _______________________________________________
> > > > linux-riscv mailing list
> > > > linux-riscv@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi,
Thank you for this patch - it;s a valuable addition to the RISC-V HBP support.
I have one question regarding the locking:
On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> +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;
> + struct sbiret ret;
> + int err = 0;
> +
> + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> + *this_cpu_ptr(&ecall_lock_flags));
Could you clarify the purpose of the ecall_lock in arch_install_hw_breakpoint and
arch_update_hw_breakpoint? Why is it used only in two functions mentioned above,
but not in other PMU callbacks like arch_uninstall_hw_breakpoint?
Regards
Ilya
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi,
On Wed, Mar 18, 2026 at 5:30 PM Ilya Mamay <mmamayka01@gmail.com> wrote:
>
> Hi,
>
> Thank you for this patch - it;s a valuable addition to the RISC-V HBP support.
> I have one question regarding the locking:
>
> On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> > +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;
> > + struct sbiret ret;
> > + int err = 0;
> > +
> > + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> > + *this_cpu_ptr(&ecall_lock_flags));
>
> Could you clarify the purpose of the ecall_lock in arch_install_hw_breakpoint and
> arch_update_hw_breakpoint? Why is it used only in two functions mentioned above,
> but not in other PMU callbacks like arch_uninstall_hw_breakpoint?
>
I probably missed it in arch_uninstall_hw_breakpoint. I will take care of it.
Thanks & Regards
Himanshu
> Regards
> Ilya
>
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi, Himanshu
Thanks for your reply. However, could you please explain the actual purpose of
this ecall_lock? I still don't understand why it is needed.
On Mon, Apr 06, 2026 at 10:21:06AM +0530, Himanshu Chauhan wrote:
> Hi,
>
> On Wed, Mar 18, 2026 at 5:30 PM Ilya Mamay <mmamayka01@gmail.com> wrote:
> >
> > Hi,
> >
> > Thank you for this patch - it;s a valuable addition to the RISC-V HBP support.
> > I have one question regarding the locking:
> >
> > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> > > +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;
> > > + struct sbiret ret;
> > > + int err = 0;
> > > +
> > > + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> > > + *this_cpu_ptr(&ecall_lock_flags));
> >
> > Could you clarify the purpose of the ecall_lock in arch_install_hw_breakpoint and
> > arch_update_hw_breakpoint? Why is it used only in two functions mentioned above,
> > but not in other PMU callbacks like arch_uninstall_hw_breakpoint?
> >
> I probably missed it in arch_uninstall_hw_breakpoint. I will take care of it.
>
> Thanks & Regards
> Himanshu
>
> > Regards
> > Ilya
> >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi,
On Tue, Apr 7, 2026 at 3:59 PM Ilya Mamay <mmamayka01@gmail.com> wrote:
>
> Hi, Himanshu
>
> Thanks for your reply. However, could you please explain the actual purpose of
> this ecall_lock? I still don't understand why it is needed.
The arch_install/uinstall calls can be called as part of ptrace system
calls. This path can be preempted and leave the in-kernel state of the
hardware breakpoints inconsistent. I am using these for this purpose.
Regards
Himanshu
>
> On Mon, Apr 06, 2026 at 10:21:06AM +0530, Himanshu Chauhan wrote:
> > Hi,
> >
> > On Wed, Mar 18, 2026 at 5:30 PM Ilya Mamay <mmamayka01@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > Thank you for this patch - it;s a valuable addition to the RISC-V HBP support.
> > > I have one question regarding the locking:
> > >
> > > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> > > > +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;
> > > > + struct sbiret ret;
> > > > + int err = 0;
> > > > +
> > > > + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> > > > + *this_cpu_ptr(&ecall_lock_flags));
> > >
> > > Could you clarify the purpose of the ecall_lock in arch_install_hw_breakpoint and
> > > arch_update_hw_breakpoint? Why is it used only in two functions mentioned above,
> > > but not in other PMU callbacks like arch_uninstall_hw_breakpoint?
> > >
> > I probably missed it in arch_uninstall_hw_breakpoint. I will take care of it.
> >
> > Thanks & Regards
> > Himanshu
> >
> > > Regards
> > > Ilya
> > >
> > > > _______________________________________________
> > > > linux-riscv mailing list
> > > > linux-riscv@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote: > +static int rv_init_mcontrol_trigger(const struct perf_event_attr *attr, > +static int rv_init_mcontrol6_trigger(const struct perf_event_attr *attr, Another minor thing in passing, can you do s/rv/riscv/ here? rv is actual subsystem and riscv arch code is almost exclusively using riscv as a prefix. Cheers, Conor.
On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> +static void init_sbi_dbtr(void)
> +{
> + unsigned long tdata1;
> + struct sbiret ret;
> +
> + if (sbi_probe_extension(SBI_EXT_DBTR) <= 0) {
> + pr_warn("%s: SBI_EXT_DBTR is not supported\n", __func__);
This is going to run an all systems, right? A pr_warn() seems
inappropriate, given that not supporting this extension isn't a problem.
If you want to produce warnings, only do it for < 0 return values and
maybe print the actual error code too?
> + dbtr_total_num = 0;
> + goto done;
> + }
> +
> + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
> + 0, 0, 0, 0, 0, 0);
> + if (ret.error) {
> + pr_warn("%s: Failed to detect triggers\n", __func__);
> + dbtr_total_num = 0;
> + goto done;
> + }
> +
> + tdata1 = 0;
> + tdata1 = RV_DBTR_SET_TDATA1_TYPE(tdata1, RV_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("%s: failed to detect mcontrol6 triggers\n", __func__);
> + } else if (!ret.value) {
> + pr_warn("%s: type 6 triggers not available\n", __func__);
> + } else {
> + dbtr_total_num = ret.value;
> + dbtr_type = RV_DBTR_TRIG_MCONTROL6;
> + pr_warn("%s: mcontrol6 trigger available.\n", __func__);
> + goto done;
> + }
> +
> + /* fallback to type 2 triggers if type 6 is not available */
> +
> + tdata1 = 0;
> + tdata1 = RV_DBTR_SET_TDATA1_TYPE(tdata1, RV_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("%s: failed to detect mcontrol triggers\n", __func__);
> + } else if (!ret.value) {
> + pr_warn("%s: type 2 triggers not available\n", __func__);
> + } else {
> + dbtr_total_num = ret.value;
> + dbtr_type = RV_DBTR_TRIG_MCONTROL;
> + goto done;
> + }
> +
> +done:
> + dbtr_init = 1;
> +}
> +
> +void hw_breakpoint_pmu_read(struct perf_event *bp)
> +{
> + /* TODO */
> +}
> +
> +void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
> +{
> + /* TODO */
> +}
> +
> +void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
> +{
> + /* TODO */
> +}
This isn't an RFC, why does it have TODOs?
Hi,
On Mon, Feb 23, 2026 at 3:55 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> > +static void init_sbi_dbtr(void)
> > +{
> > + unsigned long tdata1;
> > + struct sbiret ret;
> > +
> > + if (sbi_probe_extension(SBI_EXT_DBTR) <= 0) {
> > + pr_warn("%s: SBI_EXT_DBTR is not supported\n", __func__);
>
> This is going to run an all systems, right? A pr_warn() seems
> inappropriate, given that not supporting this extension isn't a problem.
> If you want to produce warnings, only do it for < 0 return values and
> maybe print the actual error code too?
It's correct that the absence of the extension is not a problem but an
informational message can be helpful.
So, I can use pr_info or pr_info_once for this with proper error code. Ok?
Regards
Himanshu
>
> > + dbtr_total_num = 0;
> > + goto done;
> > + }
> > +
> > + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
> > + 0, 0, 0, 0, 0, 0);
> > + if (ret.error) {
> > + pr_warn("%s: Failed to detect triggers\n", __func__);
> > + dbtr_total_num = 0;
> > + goto done;
> > + }
> > +
> > + tdata1 = 0;
> > + tdata1 = RV_DBTR_SET_TDATA1_TYPE(tdata1, RV_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("%s: failed to detect mcontrol6 triggers\n", __func__);
> > + } else if (!ret.value) {
> > + pr_warn("%s: type 6 triggers not available\n", __func__);
> > + } else {
> > + dbtr_total_num = ret.value;
> > + dbtr_type = RV_DBTR_TRIG_MCONTROL6;
> > + pr_warn("%s: mcontrol6 trigger available.\n", __func__);
> > + goto done;
> > + }
> > +
> > + /* fallback to type 2 triggers if type 6 is not available */
> > +
> > + tdata1 = 0;
> > + tdata1 = RV_DBTR_SET_TDATA1_TYPE(tdata1, RV_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("%s: failed to detect mcontrol triggers\n", __func__);
> > + } else if (!ret.value) {
> > + pr_warn("%s: type 2 triggers not available\n", __func__);
> > + } else {
> > + dbtr_total_num = ret.value;
> > + dbtr_type = RV_DBTR_TRIG_MCONTROL;
> > + goto done;
> > + }
> > +
> > +done:
> > + dbtr_init = 1;
> > +}
>
> > +
> > +void hw_breakpoint_pmu_read(struct perf_event *bp)
> > +{
> > + /* TODO */
> > +}
> > +
> > +void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
> > +{
> > + /* TODO */
> > +}
> > +
> > +void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
> > +{
> > + /* TODO */
> > +}
>
> This isn't an RFC, why does it have TODOs?
On Tue, Feb 24, 2026 at 09:30:24AM +0530, Himanshu Chauhan wrote:
> Hi,
>
> On Mon, Feb 23, 2026 at 3:55 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> > > +static void init_sbi_dbtr(void)
> > > +{
> > > + unsigned long tdata1;
> > > + struct sbiret ret;
> > > +
> > > + if (sbi_probe_extension(SBI_EXT_DBTR) <= 0) {
> > > + pr_warn("%s: SBI_EXT_DBTR is not supported\n", __func__);
> >
> > This is going to run an all systems, right? A pr_warn() seems
> > inappropriate, given that not supporting this extension isn't a problem.
> > If you want to produce warnings, only do it for < 0 return values and
> > maybe print the actual error code too?
>
> It's correct that the absence of the extension is not a problem but an
> informational message can be helpful.
> So, I can use pr_info or pr_info_once for this with proper error code. Ok?
IMO, you shouldn't be printing unless there's an actual error. If every
extension we had support for in the kernel printed a message when it was
absent, our logs would be saturated.
>
> Regards
> Himanshu
Did you miss the comment at the end about the remaining TODOs?
Cheers,
Conor.
>
> >
> > > + dbtr_total_num = 0;
> > > + goto done;
> > > + }
> > > +
> > > + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
> > > + 0, 0, 0, 0, 0, 0);
> > > + if (ret.error) {
> > > + pr_warn("%s: Failed to detect triggers\n", __func__);
> > > + dbtr_total_num = 0;
> > > + goto done;
> > > + }
> > > +
> > > + tdata1 = 0;
> > > + tdata1 = RV_DBTR_SET_TDATA1_TYPE(tdata1, RV_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("%s: failed to detect mcontrol6 triggers\n", __func__);
> > > + } else if (!ret.value) {
> > > + pr_warn("%s: type 6 triggers not available\n", __func__);
> > > + } else {
> > > + dbtr_total_num = ret.value;
> > > + dbtr_type = RV_DBTR_TRIG_MCONTROL6;
> > > + pr_warn("%s: mcontrol6 trigger available.\n", __func__);
> > > + goto done;
> > > + }
> > > +
> > > + /* fallback to type 2 triggers if type 6 is not available */
> > > +
> > > + tdata1 = 0;
> > > + tdata1 = RV_DBTR_SET_TDATA1_TYPE(tdata1, RV_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("%s: failed to detect mcontrol triggers\n", __func__);
> > > + } else if (!ret.value) {
> > > + pr_warn("%s: type 2 triggers not available\n", __func__);
> > > + } else {
> > > + dbtr_total_num = ret.value;
> > > + dbtr_type = RV_DBTR_TRIG_MCONTROL;
> > > + goto done;
> > > + }
> > > +
> > > +done:
> > > + dbtr_init = 1;
> > > +}
> >
> > > +
> > > +void hw_breakpoint_pmu_read(struct perf_event *bp)
> > > +{
> > > + /* TODO */
> > > +}
> > > +
> > > +void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
> > > +{
> > > + /* TODO */
> > > +}
> > > +
> > > +void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
> > > +{
> > > + /* TODO */
> > > +}
> >
> > This isn't an RFC, why does it have TODOs?
On Tue, Feb 24, 2026 at 1:33 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Tue, Feb 24, 2026 at 09:30:24AM +0530, Himanshu Chauhan wrote:
> > Hi,
> >
> > On Mon, Feb 23, 2026 at 3:55 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> > >
> > > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote:
> > > > +static void init_sbi_dbtr(void)
> > > > +{
> > > > + unsigned long tdata1;
> > > > + struct sbiret ret;
> > > > +
> > > > + if (sbi_probe_extension(SBI_EXT_DBTR) <= 0) {
> > > > + pr_warn("%s: SBI_EXT_DBTR is not supported\n", __func__);
> > >
> > > This is going to run an all systems, right? A pr_warn() seems
> > > inappropriate, given that not supporting this extension isn't a problem.
> > > If you want to produce warnings, only do it for < 0 return values and
> > > maybe print the actual error code too?
> >
> > It's correct that the absence of the extension is not a problem but an
> > informational message can be helpful.
> > So, I can use pr_info or pr_info_once for this with proper error code. Ok?
>
> IMO, you shouldn't be printing unless there's an actual error. If every
> extension we had support for in the kernel printed a message when it was
> absent, our logs would be saturated.
Accepted.
>
> >
> > Regards
> > Himanshu
>
> Did you miss the comment at the end about the remaining TODOs?
No. As I mentioned in the cover letter, the ptrace support is not
implemented here. I am actively working on it and these are
implemented in ptrace work.
The test is done using the perf events directly. The second patch in
this patch set has the test application.
Regards
Himanshu
>
> Cheers,
> Conor.
>
> >
> > >
> > > > + dbtr_total_num = 0;
> > > > + goto done;
> > > > + }
> > > > +
> > > > + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
> > > > + 0, 0, 0, 0, 0, 0);
> > > > + if (ret.error) {
> > > > + pr_warn("%s: Failed to detect triggers\n", __func__);
> > > > + dbtr_total_num = 0;
> > > > + goto done;
> > > > + }
> > > > +
> > > > + tdata1 = 0;
> > > > + tdata1 = RV_DBTR_SET_TDATA1_TYPE(tdata1, RV_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("%s: failed to detect mcontrol6 triggers\n", __func__);
> > > > + } else if (!ret.value) {
> > > > + pr_warn("%s: type 6 triggers not available\n", __func__);
> > > > + } else {
> > > > + dbtr_total_num = ret.value;
> > > > + dbtr_type = RV_DBTR_TRIG_MCONTROL6;
> > > > + pr_warn("%s: mcontrol6 trigger available.\n", __func__);
> > > > + goto done;
> > > > + }
> > > > +
> > > > + /* fallback to type 2 triggers if type 6 is not available */
> > > > +
> > > > + tdata1 = 0;
> > > > + tdata1 = RV_DBTR_SET_TDATA1_TYPE(tdata1, RV_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("%s: failed to detect mcontrol triggers\n", __func__);
> > > > + } else if (!ret.value) {
> > > > + pr_warn("%s: type 2 triggers not available\n", __func__);
> > > > + } else {
> > > > + dbtr_total_num = ret.value;
> > > > + dbtr_type = RV_DBTR_TRIG_MCONTROL;
> > > > + goto done;
> > > > + }
> > > > +
> > > > +done:
> > > > + dbtr_init = 1;
> > > > +}
> > >
> > > > +
> > > > +void hw_breakpoint_pmu_read(struct perf_event *bp)
> > > > +{
> > > > + /* TODO */
> > > > +}
> > > > +
> > > > +void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
> > > > +{
> > > > + /* TODO */
> > > > +}
> > > > +
> > > > +void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
> > > > +{
> > > > + /* TODO */
> > > > +}
> > >
> > > This isn't an RFC, why does it have TODOs?
On Wed, Feb 25, 2026 at 10:33:27AM +0530, Himanshu Chauhan wrote: > On Tue, Feb 24, 2026 at 1:33 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > > On Tue, Feb 24, 2026 at 09:30:24AM +0530, Himanshu Chauhan wrote: > > > Hi, > > > > > > On Mon, Feb 23, 2026 at 3:55 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > > > > > > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote: > > > > Did you miss the comment at the end about the remaining TODOs? > > No. As I mentioned in the cover letter, the ptrace support is not > implemented here. I am actively working on it and these are > implemented in ptrace work. > The test is done using the perf events directly. The second patch in > this patch set has the test application. Then the patchset should still be marked RFC, since it is not finished.
On Wed, Feb 25, 2026 at 2:31 PM Conor Dooley <conor@kernel.org> wrote: > > On Wed, Feb 25, 2026 at 10:33:27AM +0530, Himanshu Chauhan wrote: > > On Tue, Feb 24, 2026 at 1:33 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > > > > On Tue, Feb 24, 2026 at 09:30:24AM +0530, Himanshu Chauhan wrote: > > > > Hi, > > > > > > > > On Mon, Feb 23, 2026 at 3:55 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > > > > > > > > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote: > > > > > > > Did you miss the comment at the end about the remaining TODOs? > > > > No. As I mentioned in the cover letter, the ptrace support is not > > implemented here. I am actively working on it and these are > > implemented in ptrace work. > > The test is done using the perf events directly. The second patch in > > this patch set has the test application. > > Then the patchset should still be marked RFC, since it is not finished. Wow! let's all of us post only large series (covering multiple features) which are difficult to review instead of making life easier for reviewers through incremental series which make gradual progress over-time. FYI, the v1 of this series was posted as RFC almost 2 years back. (Refer, https://lwn.net/Articles/963234/) Regards, Anup
On Thu, Feb 26, 2026 at 11:05:14AM +0530, Anup Patel wrote: > On Wed, Feb 25, 2026 at 2:31 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Wed, Feb 25, 2026 at 10:33:27AM +0530, Himanshu Chauhan wrote: > > > On Tue, Feb 24, 2026 at 1:33 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > > > > > > On Tue, Feb 24, 2026 at 09:30:24AM +0530, Himanshu Chauhan wrote: > > > > > Hi, > > > > > > > > > > On Mon, Feb 23, 2026 at 3:55 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > > > > > > > > > > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote: > > > > > > > > > > Did you miss the comment at the end about the remaining TODOs? > > > > > > No. As I mentioned in the cover letter, the ptrace support is not > > > implemented here. I am actively working on it and these are > > > implemented in ptrace work. > > > The test is done using the perf events directly. The second patch in > > > this patch set has the test application. > > > > Then the patchset should still be marked RFC, since it is not finished. > > Wow! let's all of us post only large series (covering multiple features) > which are difficult to review instead of making life easier for reviewers > through incremental series which make gradual progress over-time. Where did I say don't post it? In fact, marking it RFC *requires* posting it. Don't put words in my mouth.
On Thu, Feb 26, 2026 at 2:14 PM Conor Dooley <conor@kernel.org> wrote: > > On Thu, Feb 26, 2026 at 11:05:14AM +0530, Anup Patel wrote: > > On Wed, Feb 25, 2026 at 2:31 PM Conor Dooley <conor@kernel.org> wrote: > > > > > > On Wed, Feb 25, 2026 at 10:33:27AM +0530, Himanshu Chauhan wrote: > > > > On Tue, Feb 24, 2026 at 1:33 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > > > > > > > > On Tue, Feb 24, 2026 at 09:30:24AM +0530, Himanshu Chauhan wrote: > > > > > > Hi, > > > > > > > > > > > > On Mon, Feb 23, 2026 at 3:55 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > > > > > > > > > > > > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote: > > > > > > > > > > > > > Did you miss the comment at the end about the remaining TODOs? > > > > > > > > No. As I mentioned in the cover letter, the ptrace support is not > > > > implemented here. I am actively working on it and these are > > > > implemented in ptrace work. > > > > The test is done using the perf events directly. The second patch in > > > > this patch set has the test application. > > > > > > Then the patchset should still be marked RFC, since it is not finished. > > > > Wow! let's all of us post only large series (covering multiple features) > > which are difficult to review instead of making life easier for reviewers > > through incremental series which make gradual progress over-time. > > Where did I say don't post it? In fact, marking it RFC *requires* > posting it. Don't put words in my mouth. > Well, I disagree with you suggestion of marking this series as RFC. This series has well defined scope and is based on ratified spec. A series need not cover all possible features to be a regular non-RFC patches. Don't throw your RFC related opinions pointlessly. Regards, Anup
On 26 February 2026 11:40:51 GMT, Anup Patel <apatel@ventanamicro.com> wrote: >On Thu, Feb 26, 2026 at 2:14 PM Conor Dooley <conor@kernel.org> wrote: >> >> On Thu, Feb 26, 2026 at 11:05:14AM +0530, Anup Patel wrote: >> > On Wed, Feb 25, 2026 at 2:31 PM Conor Dooley <conor@kernel.org> wrote: >> > > >> > > On Wed, Feb 25, 2026 at 10:33:27AM +0530, Himanshu Chauhan wrote: >> > > > On Tue, Feb 24, 2026 at 1:33 PM Conor Dooley <conor.dooley@microchip.com> wrote: >> > > > > >> > > > > On Tue, Feb 24, 2026 at 09:30:24AM +0530, Himanshu Chauhan wrote: >> > > > > > Hi, >> > > > > > >> > > > > > On Mon, Feb 23, 2026 at 3:55 PM Conor Dooley <conor.dooley@microchip.com> wrote: >> > > > > > > >> > > > > > > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote: >> > > >> > > > > >> > > > > Did you miss the comment at the end about the remaining TODOs? >> > > > >> > > > No. As I mentioned in the cover letter, the ptrace support is not >> > > > implemented here. I am actively working on it and these are >> > > > implemented in ptrace work. >> > > > The test is done using the perf events directly. The second patch in >> > > > this patch set has the test application. >> > > >> > > Then the patchset should still be marked RFC, since it is not finished. >> > >> > Wow! let's all of us post only large series (covering multiple features) >> > which are difficult to review instead of making life easier for reviewers >> > through incremental series which make gradual progress over-time. >> >> Where did I say don't post it? In fact, marking it RFC *requires* >> posting it. Don't put words in my mouth. >> > >Well, I disagree with you suggestion of marking this series as RFC. >This series has well defined scope and is based on ratified spec. >A series need not cover all possible features to be a regular >non-RFC patches. > >Don't throw your RFC related opinions pointlessly. It's clearly not pointless when there are unexplained TODOs. Seeing to-do items is absolutely sufficient grounds to question them, and when the response doesn't clearly explain why they're okay, my thoughts about RFC are reasonable - even if ultimately they might be incorrect. Instead of being passive aggressive with me, you could have just explained why it was okay.
On Thu, Feb 26, 2026 at 6:05 PM Conor Dooley <conor@kernel.org> wrote: > > > > On 26 February 2026 11:40:51 GMT, Anup Patel <apatel@ventanamicro.com> wrote: > >On Thu, Feb 26, 2026 at 2:14 PM Conor Dooley <conor@kernel.org> wrote: > >> > >> On Thu, Feb 26, 2026 at 11:05:14AM +0530, Anup Patel wrote: > >> > On Wed, Feb 25, 2026 at 2:31 PM Conor Dooley <conor@kernel.org> wrote: > >> > > > >> > > On Wed, Feb 25, 2026 at 10:33:27AM +0530, Himanshu Chauhan wrote: > >> > > > On Tue, Feb 24, 2026 at 1:33 PM Conor Dooley <conor.dooley@microchip.com> wrote: > >> > > > > > >> > > > > On Tue, Feb 24, 2026 at 09:30:24AM +0530, Himanshu Chauhan wrote: > >> > > > > > Hi, > >> > > > > > > >> > > > > > On Mon, Feb 23, 2026 at 3:55 PM Conor Dooley <conor.dooley@microchip.com> wrote: > >> > > > > > > > >> > > > > > > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote: > >> > > > >> > > > > > >> > > > > Did you miss the comment at the end about the remaining TODOs? > >> > > > > >> > > > No. As I mentioned in the cover letter, the ptrace support is not > >> > > > implemented here. I am actively working on it and these are > >> > > > implemented in ptrace work. > >> > > > The test is done using the perf events directly. The second patch in > >> > > > this patch set has the test application. > >> > > > >> > > Then the patchset should still be marked RFC, since it is not finished. > >> > > >> > Wow! let's all of us post only large series (covering multiple features) > >> > which are difficult to review instead of making life easier for reviewers > >> > through incremental series which make gradual progress over-time. > >> > >> Where did I say don't post it? In fact, marking it RFC *requires* > >> posting it. Don't put words in my mouth. > >> > > > >Well, I disagree with you suggestion of marking this series as RFC. > >This series has well defined scope and is based on ratified spec. > >A series need not cover all possible features to be a regular > >non-RFC patches. > > > >Don't throw your RFC related opinions pointlessly. > > It's clearly not pointless when there are unexplained TODOs. > Seeing to-do items is absolutely sufficient grounds to question them, and when the response doesn't clearly explain why they're okay, my thoughts about RFC are reasonable - even if ultimately they might be incorrect. Treating series RFC just because there is some future work to be done is clearly a pointless opinion of yours. People across kernel subsystem develop features incrementally as multiple series. > Instead of being passive aggressive with me, you could have just explained why it was okay. You started the agression by asking the series to be RFC just based on your opinion.
On Wed, Feb 25, 2026 at 2:30 PM Conor Dooley <conor@kernel.org> wrote: > > On Wed, Feb 25, 2026 at 10:33:27AM +0530, Himanshu Chauhan wrote: > > On Tue, Feb 24, 2026 at 1:33 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > > > > On Tue, Feb 24, 2026 at 09:30:24AM +0530, Himanshu Chauhan wrote: > > > > Hi, > > > > > > > > On Mon, Feb 23, 2026 at 3:55 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > > > > > > > > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote: > > > > > > > Did you miss the comment at the end about the remaining TODOs? > > > > No. As I mentioned in the cover letter, the ptrace support is not > > implemented here. I am actively working on it and these are > > implemented in ptrace work. > > The test is done using the perf events directly. The second patch in > > this patch set has the test application. > > Then the patchset should still be marked RFC, since it is not finished. Why? I disagree. It enables the debug triggers support with perf. Ptrace will build upon it. Regards Himanshu
On Thu, Feb 26, 2026 at 08:17:47AM +0530, Himanshu Chauhan wrote: > On Wed, Feb 25, 2026 at 2:30 PM Conor Dooley <conor@kernel.org> wrote: > > > > On Wed, Feb 25, 2026 at 10:33:27AM +0530, Himanshu Chauhan wrote: > > > On Tue, Feb 24, 2026 at 1:33 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > > > > > > On Tue, Feb 24, 2026 at 09:30:24AM +0530, Himanshu Chauhan wrote: > > > > > Hi, > > > > > > > > > > On Mon, Feb 23, 2026 at 3:55 PM Conor Dooley <conor.dooley@microchip.com> wrote: > > > > > > > > > > > > On Mon, Feb 23, 2026 at 10:19:17AM +0530, Himanshu Chauhan wrote: > > > > > > > > > > Did you miss the comment at the end about the remaining TODOs? > > > > > > No. As I mentioned in the cover letter, the ptrace support is not > > > implemented here. I am actively working on it and these are > > > implemented in ptrace work. > > > The test is done using the perf events directly. The second patch in > > > this patch set has the test application. > > > > Then the patchset should still be marked RFC, since it is not finished. > > Why? I disagree. > It enables the debug triggers support with perf. Ptrace will build upon it. If the functions marked TODO are are not required for the patchset to be merged, your commit message should clearly explain why these particular functions do not need to be implemented at this stage. Cheers, Conor.
© 2016 - 2026 Red Hat, Inc.