Message-ID: <aB0HXDweWAagCofs@ghost>
Date: Thu, 8 May 2025 12:34:52 -0700
From: Charlie Jenkins <charlie@...osinc.com>
To: Himanshu Chauhan <hchauhan@...tanamicro.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 Thu, May 08, 2025 at 05:24:42PM +0530, Himanshu Chauhan wrote:
> 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]

The patch in that branch is different! In this series there is:

#if __riscv_xlen == 64
#define MEM_HI(_m)	((u64)_m >> 32)
#define MEM_LO(_m)	((u64)_m & 0xFFFFFFFFUL)
#elif __riscv_xlen == 32
#define MEM_HI(_m)	(((u64)_m >> 32) & 0x3)
#define MEM_LO(_m)	((u64)_m & 0xFFFFFFFFUL)
#else
#error "Unknown __riscv_xlen"
#endif

static int arch_smp_setup_sbi_shmem(unsigned int cpu)
{
	struct sbi_dbtr_shmem_entry *dbtr_shmem;
	unsigned long shmem_pa;
	struct sbiret ret;
	int rc;

	dbtr_shmem = per_cpu_ptr(sbi_dbtr_shmem, cpu);
	if (!dbtr_shmem) {
		pr_err("Invalid per-cpu shared memory for debug triggers\n");
		return -ENODEV;
	}

	shmem_pa = __pa(dbtr_shmem);

	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_SETUP_SHMEM,
			(!MEM_LO(shmem_pa) ? 0xFFFFFFFFUL : MEM_LO(shmem_pa)),
			(!MEM_HI(shmem_pa) ? 0xFFFFFFFFUL : MEM_HI(shmem_pa)),
			 0, 0, 0, 0);

But in the Ventana branch there is:


#if __riscv_xlen == 64
#define MEM_HI(_m)	0
#define MEM_LO(_m)	((u64)_m)
#elif __riscv_xlen == 32
#define MEM_HI(_m)	((u64)_m >> 32)
#define MEM_LO(_m)	((u64)_m & 0xFFFFFFFFUL)
#else
#error "Unknown __riscv_xlen"
#endif

static int arch_smp_setup_sbi_shmem(unsigned int cpu)
{
	struct sbi_dbtr_shmem_entry *dbtr_shmem;
	unsigned long shmem_pa;
	struct sbiret ret;
	int rc;

	dbtr_shmem = per_cpu_ptr(sbi_dbtr_shmem, cpu);
	if (!dbtr_shmem) {
		pr_err("Invalid per-cpu shared memory for debug triggers\n");
		return -ENODEV;
	}

	shmem_pa = __pa(dbtr_shmem);

	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_SETUP_SHMEM,
			MEM_LO(shmem_pa), MEM_HI(shmem_pa), 0, 0, 0, 0);


I suppose this was causing the shmem setup to fail and preventing the
kernel from bringing up the other harts.

Can you send a version 2 that reflects the version in the Ventana tree?

- Charlie

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