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

[Ilya Mamay]

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).

> +/* 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