Re: [PATCH v5 1/3] riscv: Introduce support for hardware break/watchpoints

[Jesse Taube]

On Fri, Jul 24, 2026 at 11:40 AM Jesse Taube <jtaubepe@xxxxxxxxxx> wrote:
>
> On Thu, Jul 23, 2026 at 7:18 AM Himanshu Chauhan
> <himanshu.chauhan@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >