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

[Ilya Mamay]

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@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