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

[Himanshu Chauhan]

Hi Ilya,

On Tue, Apr 7, 2026 at 5:04 PM Ilya Mamay <mmamayka01@xxxxxxxxx> wrote:
>
> Hi Himanshu,
>
> Thank you for your reply regarding the tdata1.hit check. However, there is
> a second issue I raised in my previous message that seems to have been
> overlooked. I also used the wrong terminology in my previous message and would
> like to clarify the issue:
>
> Currently, arch_install_hw_breakpoint() calls SBI_EXT_DBTR_TRIG_INSTALL only
> when the perf event is being scheduled on the tracee. If that call fails
> (e.g., because the hardware WARL fields do not accept the requested
> tdata1-tdata3 values), there is no way to notify the tracer or reject the
> breakpoint earlier. The breakpoint will simply never fire, and the tracer only
> discovers this after the fact by reading the perf_event state – by which time
> the tracee may have already passed the point of interest.
>
> There is no way in the current implementation for the tracer to verify that the
> breakpoint cannot be installed because of WARL constraints.
>
> I understand that a full solution might be non-trivial. However, could you
> at least share your thoughts on how this could be addressed?
>

You have a valid point here. But by design the arch_install/uninstalls
are called as and when events are scheduled based on scheduling of the
tracee. I believe this problem should be with all architectures where
setting breaking is not as simple as a register write (I am thinking
x86 here). As of now, I don't have a solution for it. But I will think
about it unless you have something on top of your head.

Regards
Himanshu

> Regards,
> Ilya
>
> On Mon, Apr 06, 2026 at 10:19:30AM +0530, Himanshu Chauhan wrote:
> > Hi Ilya,
> >
> > On Thu, Mar 19, 2026 at 4:06 PM Ilya Mamay <mmamayka01@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