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

[Charlie Jenkins <charlie@rivosinc.com>]

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,


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