Re: [PATCH v3 1/2] riscv: Introduce support for hardware break/watchpoints

[Himanshu Chauhan]

Hi Ilya,

On Thu, Mar 19, 2026 at 4:06 PM Ilya Mamay <mmamayka01@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-riscv