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

[Charlie Jenkins <charlie@rivosinc.com>]

On Wed, May 07, 2025 at 04:58:56PM +0530, Himanshu Chauhan wrote:
> Hi Charlie,
> 
> On 5/6/25 07:30, Charlie Jenkins wrote:
> > On Thu, Feb 22, 2024 at 06:20:59PM +0530, Himanshu Chauhan 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 <hchauhan@ventanamicro.com>
> > > ---
> > >   arch/riscv/Kconfig                     |   1 +
> > >   arch/riscv/include/asm/hw_breakpoint.h | 327 ++++++++++++
> > >   arch/riscv/include/asm/kdebug.h        |   3 +-
> > >   arch/riscv/kernel/Makefile             |   1 +
> > >   arch/riscv/kernel/hw_breakpoint.c      | 659 +++++++++++++++++++++++++
> > >   arch/riscv/kernel/traps.c              |   6 +
> > >   6 files changed, 996 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/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
> > > new file mode 100644
> > > index 000000000000..7787123c7180
> > > --- /dev/null
> > > +++ b/arch/riscv/kernel/hw_breakpoint.c
> > > +
> > > +void clear_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("%s: total number of type %d triggers: %u\n",
> > > +			__func__, dbtr_type, dbtr_total_num);
> > > +	} else {
> > > +		pr_info("%s: No hardware triggers available\n", __func__);
> > > +		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("%s: Failed to allocate shared memory.\n", __func__);
> > > +		rc = -ENOMEM;
> > > +		goto out;
> > > +	}
> > > +
> > > +	/* Hotplug handler to register/unregister shared memory with SBI */
> > > +	rc = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> > When using this, only hart 0 is getting setup. I think instead we want
> > the following to have all harts get setup:
> > 
> > 	for_each_online_cpu(cpu)
> > 		arch_smp_setup_sbi_shmem(cpu);
> > 
> > 	/* Hotplug handler to register/unregister shared memory with SBI */
> > 	rc = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> 
> cpuhp_setup_state() install the callbacks and invoke the @startup callback
> (if not NULL) for all online CPUs. So there is no need to call
> "arch_smp_setup_sbi_shmem" for each CPU and then install the hotplug
> handler.

That's what I thought as well, but when testing that is not what was
happening.

> 
> If you are running this on QEMU, could you please share the qemu command you
> are invoking? I will test at my end and update you.

This is my qemu command:

qemu-system-riscv64 -nographic -m 1G -machine virt -smp 4 \
    -kernel arch/riscv/boot/Image -bios /home/charlie/opensbi/build/platform/generic/firmware/fw_dynamic.bin \
    -append "root=/dev/vda rw earlycon console=ttyS0" \
    -drive file=/home/charlie/buildroot/output/images/rootfs.ext2,format=raw,id=hd0,if=none \
    -cpu rv64,zicond=true \
    -device virtio-blk-device,drive=hd0 -gdb tcp::1234

- Charlie

> 
> Regards
> 
> Himanshu
> 
> > 
> > 
> > However, I am testing against tip-of-tree opensbi and am hitting an
> > issue during the setup on all harts:
> > 
> > [    0.202332] arch_smp_setup_sbi_shmem: Invalid address parameter (18446744073709551611)
> > [    0.202794] CPU 0: HW Breakpoint shared memory registered.
> > 
> > Additionally, this seems like it should be a fatal error, but it
> > continues on to print that the shared memory is registered because there
> > is no check before printing this seemingly successful message.
> > 
> > I know I am reviving an old thread, but do you have any insight into
> > what might be happening?
> > 
> > - Charlie
> > 
> > > +			       "riscv/hw_breakpoint:prepare",
> > > +			       arch_smp_setup_sbi_shmem,
> > > +			       arch_smp_teardown_sbi_shmem);
> > > +
> > > +	if (rc < 0) {
> > > +		pr_warn("%s: Failed to setup CPU hotplug state\n", __func__);
> > > +		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 a1b9be3c4332..53e1dfe5746b 100644
> > > --- a/arch/riscv/kernel/traps.c
> > > +++ b/arch/riscv/kernel/traps.c
> > > @@ -277,6 +277,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.34.1
> > > 
> > > 
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv