[PATCH v5 1/3] riscv: Introduce support for hardware break/watchpoints
Jesse Taube
jtaubepe at redhat.com
Fri Jul 24 08:55:20 PDT 2026
On Fri, Jul 24, 2026 at 11:40 AM Jesse Taube <jtaubepe at redhat.com> wrote:
>
> On Thu, Jul 23, 2026 at 7:18 AM Himanshu Chauhan
> <himanshu.chauhan at oss.qualcomm.com> wrote:
> >
> > 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 at oss.qualcomm.com>
> > ---
> > arch/riscv/Kconfig | 1 +
> > arch/riscv/include/asm/hw_breakpoint.h | 337 +++++++++++
> > arch/riscv/include/asm/kdebug.h | 3 +-
> > arch/riscv/kernel/Makefile | 1 +
> > arch/riscv/kernel/hw_breakpoint.c | 738 +++++++++++++++++++++++++
> > arch/riscv/kernel/traps.c | 6 +
> > 6 files changed, 1085 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 f7028caaeae0..a624dacdaf12 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -172,6 +172,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 !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..66a2df679f30
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/hw_breakpoint.h
> > @@ -0,0 +1,337 @@
> > +/* 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>
> > +
> > +/* Maximum number of hardware breakpoints supported */
> > +#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
> > +
> > +#define RISCV_DBTR_BIT(_prefix, _name) \
> > + RISCV_DBTR_##_prefix##_##_name##_BIT
> > +
> > +#define RISCV_DBTR_BIT_MASK(_prefix, _name) \
> > + RISCV_DBTR_##_prefix##_name##_BIT_MASK
> > +
> > +#define RISCV_DBTR_BIT_MASK_VAL(_prefix, _name, _width) \
> > + (((1UL << (_width)) - 1) << RISCV_DBTR_BIT(_prefix, _name))
>
> Can we use GENMASK here
>
> > +
> > +#define CLEAR_DBTR_BIT(_target, _prefix, _bit_name) \
> > + __clear_bit(RISCV_DBTR_BIT(_prefix, _bit_name), &(_target))
> > +
> > +#define SET_DBTR_BIT(_target, _prefix, _bit_name) \
> > + __set_bit(RISCV_DBTR_BIT(_prefix, _bit_name), &(_target))
> > +
> > +#define RISCV_DBTR_EXEC (0x1UL << 0)
> > +#define RISCV_DBTR_LOAD (0x1UL << 1)
> > +#define RISCV_DBTR_STORE (0x1UL << 2)
>
> Please use `BIT_UL()` for these
>
> > +#define RISCV_DBTR_LDST (RISCV_DBTR_LOAD | RISCV_DBTR_STORE)
> > +
> > +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,
> > +};
> > +
> > +/* Trigger Data 1 */
> > +enum {
> > + RISCV_DBTR_BIT(TDATA1, DATA) = 0,
> > +#if __riscv_xlen == 64
> > + RISCV_DBTR_BIT(TDATA1, DMODE) = 59,
> > + RISCV_DBTR_BIT(TDATA1, TYPE) = 60,
> > +#elif __riscv_xlen == 32
> > + RISCV_DBTR_BIT(TDATA1, DMODE) = 27,
> > + RISCV_DBTR_BIT(TDATA1, TYPE) = 28,
> > +#else
> > + #error "Unknown __riscv_xlen"
> > +#endif
> > +};
> > +
> > +enum {
> > +#if __riscv_xlen == 64
> > + RISCV_DBTR_BIT_MASK(TDATA1, DATA) = RISCV_DBTR_BIT_MASK_VAL(TDATA1, DATA, 59),
> > +#elif __riscv_xlen == 32
> > + RISCV_DBTR_BIT_MASK(TDATA1, DATA) = RISCV_DBTR_BIT_MASK_VAL(TDATA1, DATA, 27),
> > +#else
> > + #error "Unknown __riscv_xlen"
> > +#endif
> > + RISCV_DBTR_BIT_MASK(TDAT1, DMODE) = RISCV_DBTR_BIT_MASK_VAL(TDATA1, DMODE, 1),
> > + RISCV_DBTR_BIT_MASK(TDATA1, TYPE) = RISCV_DBTR_BIT_MASK_VAL(TDATA1, TYPE, 4),
> > +};
> > +
> > +/* MC - Match Control Type Register */
> > +enum {
> > + RISCV_DBTR_BIT(MC, LOAD) = 0,
> > + RISCV_DBTR_BIT(MC, STORE) = 1,
> > + RISCV_DBTR_BIT(MC, EXEC) = 2,
> > + RISCV_DBTR_BIT(MC, U) = 3,
> > + RISCV_DBTR_BIT(MC, S) = 4,
> > + RISCV_DBTR_BIT(MC, RES2) = 5,
> > + RISCV_DBTR_BIT(MC, M) = 6,
> > + RISCV_DBTR_BIT(MC, MATCH) = 7,
> > + RISCV_DBTR_BIT(MC, CHAIN) = 11,
> > + RISCV_DBTR_BIT(MC, ACTION) = 12,
> > + RISCV_DBTR_BIT(MC, SIZELO) = 16,
> > + RISCV_DBTR_BIT(MC, TIMING) = 18,
> > + RISCV_DBTR_BIT(MC, SELECT) = 19,
> > + RISCV_DBTR_BIT(MC, HIT) = 20,
> > +#if __riscv_xlen >= 64
> > + RISCV_DBTR_BIT(MC, SIZEHI) = 21,
> > +#endif
> > +#if __riscv_xlen == 64
> > + RISCV_DBTR_BIT(MC, MASKMAX) = 53,
> > + RISCV_DBTR_BIT(MC, DMODE) = 59,
> > + RISCV_DBTR_BIT(MC, TYPE) = 60,
> > +#elif __riscv_xlen == 32
> > + RISCV_DBTR_BIT(MC, MASKMAX) = 21,
> > + RISCV_DBTR_BIT(MC, DMODE) = 27,
> > + RISCV_DBTR_BIT(MC, TYPE) = 28,
> > +#else
> > + #error "Unknown riscv xlen"
> > +#endif
> > +};
> > +
> > +enum {
> > + RISCV_DBTR_BIT_MASK(MC, LOAD) = RISCV_DBTR_BIT_MASK_VAL(MC, LOAD, 1),
> > + RISCV_DBTR_BIT_MASK(MC, STORE) = RISCV_DBTR_BIT_MASK_VAL(MC, STORE, 1),
> > + RISCV_DBTR_BIT_MASK(MC, EXEC) = RISCV_DBTR_BIT_MASK_VAL(MC, EXEC, 1),
> > + RISCV_DBTR_BIT_MASK(MC, U) = RISCV_DBTR_BIT_MASK_VAL(MC, U, 1),
> > + RISCV_DBTR_BIT_MASK(MC, S) = RISCV_DBTR_BIT_MASK_VAL(MC, S, 1),
> > + RISCV_DBTR_BIT_MASK(MC, RES2) = RISCV_DBTR_BIT_MASK_VAL(MC, RES2, 1),
> > + RISCV_DBTR_BIT_MASK(MC, M) = RISCV_DBTR_BIT_MASK_VAL(MC, M, 1),
> > + RISCV_DBTR_BIT_MASK(MC, MATCH) = RISCV_DBTR_BIT_MASK_VAL(MC, MATCH, 4),
> > + RISCV_DBTR_BIT_MASK(MC, CHAIN) = RISCV_DBTR_BIT_MASK_VAL(MC, CHAIN, 1),
> > + RISCV_DBTR_BIT_MASK(MC, ACTION) = RISCV_DBTR_BIT_MASK_VAL(MC, ACTION, 4),
> > + RISCV_DBTR_BIT_MASK(MC, SIZELO) = RISCV_DBTR_BIT_MASK_VAL(MC, SIZELO, 2),
> > + RISCV_DBTR_BIT_MASK(MC, TIMING) = RISCV_DBTR_BIT_MASK_VAL(MC, TIMING, 1),
> > + RISCV_DBTR_BIT_MASK(MC, SELECT) = RISCV_DBTR_BIT_MASK_VAL(MC, SELECT, 1),
> > + RISCV_DBTR_BIT_MASK(MC, HIT) = RISCV_DBTR_BIT_MASK_VAL(MC, HIT, 1),
> > +#if __riscv_xlen >= 64
> > + RISCV_DBTR_BIT_MASK(MC, SIZEHI) = RISCV_DBTR_BIT_MASK_VAL(MC, SIZEHI, 2),
> > +#endif
> > + RISCV_DBTR_BIT_MASK(MC, MASKMAX) = RISCV_DBTR_BIT_MASK_VAL(MC, MASKMAX, 6),
> > + RISCV_DBTR_BIT_MASK(MC, DMODE) = RISCV_DBTR_BIT_MASK_VAL(MC, DMODE, 1),
> > + RISCV_DBTR_BIT_MASK(MC, TYPE) = RISCV_DBTR_BIT_MASK_VAL(MC, TYPE, 4),
> > +};
> > +
> > +/* MC6 - Match Control 6 Type Register */
> > +enum {
> > + RISCV_DBTR_BIT(MC6, LOAD) = 0,
> > + RISCV_DBTR_BIT(MC6, STORE) = 1,
> > + RISCV_DBTR_BIT(MC6, EXEC) = 2,
> > + RISCV_DBTR_BIT(MC6, U) = 3,
> > + RISCV_DBTR_BIT(MC6, S) = 4,
> > + RISCV_DBTR_BIT(MC6, RES2) = 5,
> > + RISCV_DBTR_BIT(MC6, M) = 6,
> > + RISCV_DBTR_BIT(MC6, MATCH) = 7,
> > + RISCV_DBTR_BIT(MC6, CHAIN) = 11,
> > + RISCV_DBTR_BIT(MC6, ACTION) = 12,
> > + RISCV_DBTR_BIT(MC6, SIZE) = 16,
> > + RISCV_DBTR_BIT(MC6, TIMING) = 20,
> > + RISCV_DBTR_BIT(MC6, SELECT) = 21,
> > + RISCV_DBTR_BIT(MC6, HIT) = 22,
> > + RISCV_DBTR_BIT(MC6, VU) = 23,
> > + RISCV_DBTR_BIT(MC6, VS) = 24,
> > +#if __riscv_xlen == 64
> > + RISCV_DBTR_BIT(MC6, DMODE) = 59,
> > + RISCV_DBTR_BIT(MC6, TYPE) = 60,
> > +#elif __riscv_xlen == 32
> > + RISCV_DBTR_BIT(MC6, DMODE) = 27,
> > + RISCV_DBTR_BIT(MC6, TYPE) = 28,
> > +#else
> > + #error "Unknown riscv xlen"
> > +#endif
> > +};
> > +
> > +enum {
> > + RISCV_DBTR_BIT_MASK(MC6, LOAD) = RISCV_DBTR_BIT_MASK_VAL(MC6, LOAD, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, STORE) = RISCV_DBTR_BIT_MASK_VAL(MC6, STORE, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, EXEC) = RISCV_DBTR_BIT_MASK_VAL(MC6, EXEC, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, U) = RISCV_DBTR_BIT_MASK_VAL(MC6, U, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, S) = RISCV_DBTR_BIT_MASK_VAL(MC6, S, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, RES2) = RISCV_DBTR_BIT_MASK_VAL(MC6, RES2, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, M) = RISCV_DBTR_BIT_MASK_VAL(MC6, M, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, MATCH) = RISCV_DBTR_BIT_MASK_VAL(MC6, MATCH, 4),
> > + RISCV_DBTR_BIT_MASK(MC6, CHAIN) = RISCV_DBTR_BIT_MASK_VAL(MC6, CHAIN, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, ACTION) = RISCV_DBTR_BIT_MASK_VAL(MC6, ACTION, 4),
> > + RISCV_DBTR_BIT_MASK(MC6, SIZE) = RISCV_DBTR_BIT_MASK_VAL(MC6, SIZE, 4),
> > + RISCV_DBTR_BIT_MASK(MC6, TIMING) = RISCV_DBTR_BIT_MASK_VAL(MC6, TIMING, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, SELECT) = RISCV_DBTR_BIT_MASK_VAL(MC6, SELECT, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, HIT) = RISCV_DBTR_BIT_MASK_VAL(MC6, HIT, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, VU) = RISCV_DBTR_BIT_MASK_VAL(MC6, VU, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, VS) = RISCV_DBTR_BIT_MASK_VAL(MC6, VS, 1),
> > +#if __riscv_xlen == 64
> > + RISCV_DBTR_BIT_MASK(MC6, DMODE) = RISCV_DBTR_BIT_MASK_VAL(MC6, DMODE, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, TYPE) = RISCV_DBTR_BIT_MASK_VAL(MC6, TYPE, 4),
> > +#elif __riscv_xlen == 32
> > + RISCV_DBTR_BIT_MASK(MC6, DMODE) = RISCV_DBTR_BIT_MASK_VAL(MC6, DMODE, 1),
> > + RISCV_DBTR_BIT_MASK(MC6, TYPE) = RISCV_DBTR_BIT_MASK_VAL(MC6, TYPE, 4),
> > +#else
> > + #error "Unknown riscv xlen"
> > +#endif
> > +};
> > +
> > +#define RISCV_DBTR_SET_TDATA1_TYPE(_t1, _type) \
> > + ({ \
> > + typeof(_t1) (td1t1) = (_t1); \
> > + (td1t1) &= ~RISCV_DBTR_BIT_MASK(TDATA1, TYPE); \
> > + (td1t1) |= (((unsigned long)(_type) \
> > + << RISCV_DBTR_BIT(TDATA1, TYPE)) \
> > + & RISCV_DBTR_BIT_MASK(TDATA1, TYPE)); \
> > + (td1t1); \
> > + })
> > +
> > +#define RISCV_DBTR_SET_MC_TYPE(_t1, _type) \
> > + ({ \
> > + typeof(_t1) (mct1) = (_t1); \
> > + (mct1) &= ~RISCV_DBTR_BIT_MASK(MC, TYPE); \
> > + (mct1) |= (((unsigned long)(_type) \
> > + << RISCV_DBTR_BIT(MC, TYPE)) \
> > + & RISCV_DBTR_BIT_MASK(MC, TYPE)); \
> > + (mct1); \
> > + })
> > +
> > +#define RISCV_DBTR_SET_MC6_TYPE(_t1, _type) \
> > + ({ \
> > + typeof(_t1) (mc6t1) = (_t1); \
> > + (mc6t1) &= ~RISCV_DBTR_BIT_MASK(MC6, TYPE); \
> > + (mc6t1) |= (((unsigned long)(_type) \
> > + << RISCV_DBTR_BIT(MC6, TYPE)) \
> > + & RISCV_DBTR_BIT_MASK(MC6, TYPE)); \
> > + (mc6t1); \
> > + })
> > +
> > +#define RISCV_DBTR_SET_MC_EXEC_BIT(_t1) \
> > + SET_DBTR_BIT(_t1, MC, EXEC)
> > +
> > +#define RISCV_DBTR_SET_MC_LOAD_BIT(_t1) \
> > + SET_DBTR_BIT(_t1, MC, LOAD)
> > +
> > +#define RISCV_DBTR_SET_MC_STORE_BIT(_t1) \
> > + SET_DBTR_BIT(_t1, MC, STORE)
> > +
> > +#define RISCV_DBTR_SET_MC_SIZELO(_t1, _val) \
> > + ({ \
> > + typeof(_t1) (mcslt1) = (_t1); \
> > + mcslt1 &= ~RISCV_DBTR_BIT_MASK(MC, SIZELO); \
> > + mcslt1 |= (((_val) << RISCV_DBTR_BIT(MC, SIZELO)) \
> > + & RISCV_DBTR_BIT_MASK(MC, SIZELO)); \
> > + (mcslt1); \
> > + })
> > +
> > +#if __riscv_xlen >= 64
> > +#define RISCV_DBTR_SET_MC_SIZEHI(_t1, _val) \
> > + ({ \
> > + typeof(_t1) (mcsht1) = (_t1); \
> > + mcsht1 &= ~RISCV_DBTR_BIT_MASK(MC, SIZEHI); \
> > + mcsht1 |= (((_val) << RISCV_DBTR_BIT(MC, SIZEHI)) \
> > + & RISCV_DBTR_BIT_MASK(MC, SIZEHI)); \
> > + (mcsht1); \
> > + })
> > +#else
> > +/* SIZEHI does not exist in the rv32 mcontrol layout; nothing to set. */
> > +#define RISCV_DBTR_SET_MC_SIZEHI(_t1, _val) ((void)(_val), (_t1))
> > +#endif
> > +
> > +#define RISCV_DBTR_SET_MC6_EXEC_BIT(_t1) \
> > + SET_DBTR_BIT(_t1, MC6, EXEC)
> > +
> > +#define RISCV_DBTR_SET_MC6_LOAD_BIT(_t1) \
> > + SET_DBTR_BIT(_t1, MC6, LOAD)
> > +
> > +#define RISCV_DBTR_SET_MC6_STORE_BIT(_t1) \
> > + SET_DBTR_BIT(_t1, MC6, STORE)
> > +
> > +#define RISCV_DBTR_SET_MC6_SIZE(_t1, _val) \
> > + ({ \
> > + typeof(_t1) (mc6szt1) = (_t1); \
> > + (mc6szt1) &= ~RISCV_DBTR_BIT_MASK(MC6, SIZE); \
> > + (mc6szt1) |= (((_val) << RISCV_DBTR_BIT(MC6, SIZE)) \
> > + & RISCV_DBTR_BIT_MASK(MC6, SIZE)); \
> > + (mc6szt1); \
> > + })
> > +
>
> To be honest with you im not a huge fan of these macros Im not sure if
> you saw my version of this patch, but it cleans
> up these macros and adds all the fixes i said here.
>
> > +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);
> > +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)
>
> This is macro guarded everywhere, no need to define it.
>
> > +{
> > + return 0;
> > +}
> > +
> > +static inline void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
> > +{
> > +}
> > +
> > +static inline void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
>
> This is macro guarded everywhere, no need to define it.
>
> > +{
> > +}
> > +
> > +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..2d7914f50c5f
> > --- /dev/null
> > +++ b/arch/riscv/kernel/hw_breakpoint.c
> > @@ -0,0 +1,738 @@
> > +// 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/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[RISCV_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;
>
> Why not also use DEFINE_PER_CPU for this?
>
> > +
> > +/* 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
>
> There are already macros for this:
> +#define SBI_SHMEM_HI(pa) ((unsigned long)upper_32_bits(pa))
> +#define SBI_SHMEM_LO(pa) ((unsigned long)lower_32_bits(pa))
> Other SBI extentions also use the same SHMEM format so it
> might be nice to add these to include/asm/sbi.h
i forgot that Anup said to me that it should be
#ifdef CONFIG_32BIT
#define SBI_SHMEM_LO(pa) ((unsigned long)lower_32_bits(pa))
#define SBI_SHMEM_HI(pa) ((unsigned long)upper_32_bits(pa))
#else
#define SBI_SHMEM_LO(pa) ((unsigned long)pa)
#define SBI_SHMEM_HI(pa) 0UL
#endif
Also `__riscv_xlen` shouldnt be used as it is 64 when using a 64bit
compiler for a 32bit config.
Thanks,
Jesse Taube
> > +
> > +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;
> > + 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 = 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) {
> > + switch (ret.error) {
> > + case SBI_ERR_DENIED:
> > + pr_warn("Access denied for shared memory at %pa\n",
> > + &shmem_pa);
> > + rc = -EPERM;
> > + break;
> > +
> > + case SBI_ERR_INVALID_PARAM:
> > + case SBI_ERR_INVALID_ADDRESS:
> > + pr_warn("Invalid address parameter (%lu)\n",
> > + ret.error);
> > + rc = -EINVAL;
> > + break;
> > +
> > + case SBI_ERR_ALREADY_AVAILABLE:
> > + pr_warn("Shared memory is already set\n");
> > + rc = -EADDRINUSE;
> > + break;
> > +
> > + case SBI_ERR_FAILURE:
> > + pr_err("Internal sdtrig state error\n");
> > + rc = -ENXIO;
> > + break;
> > +
> > + default:
> > + pr_warn("Unknown error %lu\n", ret.error);
> > + rc = -ENXIO;
> > + break;
> > + }
> > + } else {
> > + pr_info("CPU %d: HW Breakpoint shared memory registered.\n", cpu);
> > + }
>
> Instead of having a switch case for errors please use:
> + 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);
> + }
> There are a few place like this too.
>
> > +
> > + 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);
>
> In `include/asm/sbi.h` there is:
> -#define SBI_SHMEM_DISABLE -1
> +#define SBI_SHMEM_DISABLE (-1UL)
> Can we use it here
>
> > +
> > + if (ret.error) {
> > + switch (ret.error) {
> > + case SBI_ERR_DENIED:
> > + pr_err("Access denied for shared memory.\n");
> > + break;
> > +
> > + case SBI_ERR_INVALID_PARAM:
> > + case SBI_ERR_INVALID_ADDRESS:
> > + pr_err("Invalid address parameter (%lu)\n", ret.error);
> > + break;
> > +
> > + case SBI_ERR_ALREADY_AVAILABLE:
> > + pr_err("Shared memory is already set\n");
> > + break;
> > + case SBI_ERR_FAILURE:
> > + pr_err("Internal sdtrig state error\n");
> > + break;
> > + default:
> > + pr_err("Unknown error %lu\n", ret.error);
> > + break;
> > + }
> > + } else {
> > + 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("SBI_EXT_DBTR is not supported\n");
> > + 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("Failed to detect triggers\n");
> > + dbtr_total_num = 0;
> > + goto done;
> > + }
> > +
> > + tdata1 = 0;
> > + tdata1 = RISCV_DBTR_SET_TDATA1_TYPE(tdata1, 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 = ret.value;
> > + dbtr_type = RISCV_DBTR_TRIG_MCONTROL6;
> > + pr_warn("Mcontrol6 trigger available.\n");
> > + goto done;
> > + }
> > +
> > + /* fallback to type 2 triggers if type 6 is not available */
> > +
> > + tdata1 = 0;
> > + tdata1 = RISCV_DBTR_SET_TDATA1_TYPE(tdata1, 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 = ret.value;
> > + dbtr_type = RISCV_DBTR_TRIG_MCONTROL;
> > + goto done;
> > + }
> > +
> > +done:
>
> it is possible that multiple cores will run this function and set
> `dbtr_init` and friends even though they are marked as `__ro_after_init`
> It might be better
>
> > + 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)
>
> We can move this check into `init_sbi_dbtr` sence both callers do this check.
>
> > + 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 = RISCV_DBTR_EXEC;
> > + RISCV_DBTR_SET_MC_EXEC_BIT(hw->tdata1);
> > + break;
> > + case HW_BREAKPOINT_R:
> > + hw->type = RISCV_DBTR_LOAD;
> > + RISCV_DBTR_SET_MC_LOAD_BIT(hw->tdata1);
> > + break;
> > + case HW_BREAKPOINT_W:
> > + hw->type = RISCV_DBTR_STORE;
> > + RISCV_DBTR_SET_MC_STORE_BIT(hw->tdata1);
> > + break;
> > + case HW_BREAKPOINT_RW:
> > + hw->type = RISCV_DBTR_LDST;
> > + RISCV_DBTR_SET_MC_LOAD_BIT(hw->tdata1);
> > + RISCV_DBTR_SET_MC_STORE_BIT(hw->tdata1);
> > + 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;
> > + hw->tdata1 = RISCV_DBTR_SET_MC_SIZELO(hw->tdata1, 0);
> > + hw->tdata1 = RISCV_DBTR_SET_MC_SIZEHI(hw->tdata1, 0);
> > + } else {
> > + switch (attr->bp_len) {
> > + case HW_BREAKPOINT_LEN_1:
> > + hw->len = 1;
> > + hw->tdata1 = RISCV_DBTR_SET_MC_SIZELO(hw->tdata1, 1);
> > + break;
> > + case HW_BREAKPOINT_LEN_2:
> > + hw->len = 2;
> > + hw->tdata1 = RISCV_DBTR_SET_MC_SIZELO(hw->tdata1, 2);
> > + break;
> > + case HW_BREAKPOINT_LEN_4:
> > + hw->len = 4;
> > + hw->tdata1 = RISCV_DBTR_SET_MC_SIZELO(hw->tdata1, 3);
> > + break;
> > +#if __riscv_xlen >= 64
> > + case HW_BREAKPOINT_LEN_8:
> > + hw->len = 8;
> > + hw->tdata1 = RISCV_DBTR_SET_MC_SIZELO(hw->tdata1, 1);
> > + hw->tdata1 = RISCV_DBTR_SET_MC_SIZEHI(hw->tdata1, 1);
> > + break;
> > +#endif
> > + /* Set to match any size */
> > + default:
> > + hw->len = 0;
> > + hw->tdata1 = RISCV_DBTR_SET_MC_SIZELO(hw->tdata1, 0);
> > + hw->tdata1 = RISCV_DBTR_SET_MC_SIZEHI(hw->tdata1, 0);
> > + break;
> > + }
> > + }
> > +
> > + hw->tdata1 = RISCV_DBTR_SET_MC_TYPE(hw->tdata1, RISCV_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 = RISCV_DBTR_EXEC;
> > + RISCV_DBTR_SET_MC6_EXEC_BIT(hw->tdata1);
> > + break;
> > + case HW_BREAKPOINT_R:
> > + hw->type = RISCV_DBTR_LOAD;
> > + RISCV_DBTR_SET_MC6_LOAD_BIT(hw->tdata1);
> > + break;
> > + case HW_BREAKPOINT_W:
> > + hw->type = RISCV_DBTR_STORE;
> > + RISCV_DBTR_SET_MC6_STORE_BIT(hw->tdata1);
> > + break;
> > + case HW_BREAKPOINT_RW:
> > + hw->type = RISCV_DBTR_LDST;
> > + RISCV_DBTR_SET_MC6_STORE_BIT(hw->tdata1);
> > + RISCV_DBTR_SET_MC6_LOAD_BIT(hw->tdata1);
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + if (attr->bp_type == HW_BREAKPOINT_X) {
> > + /* See rv_init_mcontrol_trigger() for rationale. */
> > + hw->len = 0;
> > + hw->tdata1 = RISCV_DBTR_SET_MC6_SIZE(hw->tdata1, 0);
> > + } else {
> > + switch (attr->bp_len) {
> > + case HW_BREAKPOINT_LEN_1:
> > + hw->len = 1;
> > + hw->tdata1 = RISCV_DBTR_SET_MC6_SIZE(hw->tdata1, 1);
> > + break;
> > + case HW_BREAKPOINT_LEN_2:
> > + hw->len = 2;
> > + hw->tdata1 = RISCV_DBTR_SET_MC6_SIZE(hw->tdata1, 2);
> > + break;
> > + case HW_BREAKPOINT_LEN_4:
> > + hw->len = 4;
> > + hw->tdata1 = RISCV_DBTR_SET_MC6_SIZE(hw->tdata1, 3);
> > + break;
> > +#if __riscv_xlen >= 64
> > + case HW_BREAKPOINT_LEN_8:
> > + hw->len = 8;
> > + hw->tdata1 = RISCV_DBTR_SET_MC6_SIZE(hw->tdata1, 5);
> > + break;
> > +#endif
> > + /* Set to match any size */
> > + default:
> > + hw->len = 0;
> > + hw->tdata1 = RISCV_DBTR_SET_MC6_SIZE(hw->tdata1, 0);
> > + }
> > + }
> > +
> > + hw->tdata1 = RISCV_DBTR_SET_MC6_TYPE(hw->tdata1, RISCV_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 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;
> > +}
> > +
> > +/*
> > + * 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) {
> There is no need to save another copy of `type` we can just use
> `event->attr.bp_type` here directly same idea with `bp->address`
>
> > + /* Breakpoint */
> > + case RISCV_DBTR_EXEC:
> > + if (bp->address == args->regs->epc) {
> > + perf_bp_event(event, args->regs);
> > + ret = NOTIFY_STOP;
> > + }
>
> If `bp->address != args->regs->epc` this will silently fail.
>
> > + break;
> > +
> > + /* Watchpoint */
> > + case RISCV_DBTR_LOAD:
> > + case RISCV_DBTR_STORE:
> > + case RISCV_DBTR_LDST:
> > + {
> > + unsigned long stval = csr_read(CSR_STVAL);
>
> This value is saved in `regs->badaddr`
>
> > + unsigned long bp_start = bp->address;
> > + unsigned long bp_len = bp->len ?: 1;
> + unsigned long bp_len = bp->len ?bp->len: 1;
>
> > + unsigned long bp_end = bp_start + bp_len - 1;
> > + unsigned long stval_end = stval + sizeof(long) - 1;
> > + unsigned long tdata1;
> > + bool hit = false;
> > + struct sbiret sret;
> > + union sbi_dbtr_shmem_entry *shmem;
> > +
> > + if (bp_end < bp_start)
> > + bp_end = ~0UL;
> > + if (stval_end < stval)
> > + stval_end = ~0UL;
> > +
> > + /*
> > + * Prefer tdata1.hit from SBI trigger readout whenever
> > + * possible. Fall back to address-based matching if HIT
> > + * isn't observed/supported.
> > + */
>
> Can we break this out into another function?
>
> > + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> > + *this_cpu_ptr(&ecall_lock_flags));
> > + shmem = this_cpu_ptr(sbi_dbtr_shmem);
> > + sret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_READ,
> > + i, 1, 0, 0, 0, 0);
> > + if (!sret.error) {
> > + tdata1 = le_to_cpu(shmem->data.tdata1);
> > +
> > + if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL)
> > + hit = !!(tdata1 & RISCV_DBTR_BIT_MASK(MC, HIT));
> > + else if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL6)
> > + hit = !!(tdata1 & RISCV_DBTR_BIT_MASK(MC6, HIT));
> > + }
> > + raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock),
> > + *this_cpu_ptr(&ecall_lock_flags));
> > +
> > + /*
> > + * Sdtrig may report STVAL as the lowest accessed
> > + * address while the watchpoint can match a higher byte
> > + * in the same access.
> > + */
> > + if (hit ||
> > + (stval >= bp_start && stval <= bp_end) ||
> > + (bp_start >= stval && bp_start <= stval_end)) {
>
> (abs_diff(stval, bp_start) < bp_len)
> this is the same functionality and cleaner imo
>
> > + perf_bp_event(event, args->regs);
> > + ret = NOTIFY_STOP;
> > + }
>
> this can also silently fail.
>
> > + break;
> > + }
> > +
> > + default:
> > + pr_warn("Unknown type: %u\n", 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("Failed to install trigger\n");
> > + err = -EIO;
>
> + 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);
> > + err = -EINVAL;
> > + goto done;
> > + }
> > +
> > + slot = this_cpu_ptr(&pcpu_hw_bp_events[idx]);
> > + if (*slot) {
> > + pr_warn("Slot %lu is in use\n", idx);
> > + 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),
> > + *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;
> > +
> > + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> > + *this_cpu_ptr(&ecall_lock_flags));
> > +
> > + 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("Breakpoint not installed.\n");
> > + goto out;
> > + }
> > +
> > + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_UNINSTALL,
> > + i, 1, 0, 0, 0, 0);
> > +
> > + if (ret.error) {
> > + pr_warn("Failed to uninstall trigger %d.\n", i);
> > + goto out;
> > + }
> > +
> > + out:
> > + raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock),
> > + *this_cpu_ptr(&ecall_lock_flags));
> > +}
> > +
> > +void arch_enable_hw_breakpoint(struct perf_event *event)
> > +{
> > + struct sbiret ret;
> > + int i;
> > + struct perf_event **slot;
> > +
> > + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> > + *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),
> > + *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);
> > + 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("Breakpoint not installed.\n");
> > + 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("Failed to update trigger %d.\n", 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]);
>
> Can we put the `struct perf_event **slot` at the top of the function.
>
> > +
> > + 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)
> > +{
> > +}
>
> AFAIK empty functions are on the same line like void foo() { }
>
> > +void clear_ptrace_hw_breakpoint(struct task_struct *tsk)
> > +{
> > +}
>
> This function is unused and never called, please remove it.
> Jesse Taube
>
> > +
> > +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));
> > +
> > + if (!dbtr_init)
> > + 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);
> > + 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 8c62c771a656..029fd66a285e 100644
> > --- a/arch/riscv/kernel/traps.c
> > +++ b/arch/riscv/kernel/traps.c
> > @@ -286,6 +286,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
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
More information about the linux-riscv
mailing list