[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <57f163bb-1b4e-4f57-8f93-aee6ce1bd317@ventanamicro.com>
Date: Thu, 8 May 2025 17:24:42 +0530
From: Himanshu Chauhan <hchauhan@...tanamicro.com>
To: Charlie Jenkins <charlie@...osinc.com>
Cc: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu
Subject: Re: [RFC PATCH 2/2] riscv: Introduce support for hardware
break/watchpoints
On 5/8/25 03:02, Charlie Jenkins wrote:
> 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@...tanamicro.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.
I see the initialization happening on all the CPUs. I am using OpenSBI
Top-of-line.
Linux kernel is from https://github.com/ventanamicro/linux.git [branch:
dev-upstream]
Part of boot log:
[ 0.267250] arch_hw_breakpoint_init: total number of type 6 triggers: 2
[ 0.268108] CPU 0: HW Breakpoint shared memory registered.
[ 0.268835] CPU 1: HW Breakpoint shared memory registered.
[ 0.269932] CPU 2: HW Breakpoint shared memory registered.
[ 0.270468] CPU 3: HW Breakpoint shared memory registered.
[ 0.276554] sse: SBI SSE extension detected
[ 0.308172] HugeTLB: allocation took 0ms with
hugepage_allocation_threads=1
[ 0.309682] HugeTLB: registered 2.00 MiB page size, pre-allocated 0 pages
Can you please send me your config and bootlog?
Regards
Himanshu
>> 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@...ts.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
Powered by blists - more mailing lists