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

[Himanshu Chauhan <hchauhan@ventanamicro.com>]

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.

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.

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv