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

[Qingfang Deng <qingfang.deng@linux.dev>]

Hi,

On 2026/5/18 14:59, Himanshu Chauhan wrote:
> diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
> new file mode 100644
> index 000000000000..34556a8f3c9b
> --- /dev/null
> +++ b/arch/riscv/kernel/hw_breakpoint.c
> @@ -0,0 +1,736 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2026 Qualcomm Technologies, Inc.
> + */
> +

To make your logs more informative, you may want to #define pr_fmt here.

For example: `#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt` will prefix 
all your logs with "hw_breakpoint: ".

> +#include <linux/hw_breakpoint.h>
> +#include <linux/perf_event.h>
> +#include <linux/spinlock.h>
> +#include <linux/percpu.h>
> +#include <linux/kdebug.h>
> +#include <linux/bitops.h>
> +#include <linux/cpu.h>
> +#include <linux/cpuhotplug.h>
> +
> +#include <asm/sbi.h>
> +
> +/* Registered per-cpu bp/wp */
> +static DEFINE_PER_CPU(struct perf_event *, pcpu_hw_bp_events[RISCV_HW_BP_NUM_MAX]);
> +static DEFINE_PER_CPU(unsigned long, ecall_lock_flags);
> +static DEFINE_PER_CPU(raw_spinlock_t, ecall_lock);
> +
> +/* Per-cpu shared memory between S and M mode */
> +static union sbi_dbtr_shmem_entry __percpu *sbi_dbtr_shmem;
> +
> +/* number of debug triggers on this cpu . */
> +static int dbtr_total_num __ro_after_init;
> +static int dbtr_type __ro_after_init;
> +static int dbtr_init __ro_after_init;
> +
> +#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)
> +{
> +	union sbi_dbtr_shmem_entry *dbtr_shmem;
> +	unsigned long shmem_pa;
Nit: the type of a physical address should be "phys_addr_t".
> +	struct sbiret ret;
> +	int rc = 0;
> +
> +	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);

It's not safe to get the physical address of a percpu-allocated pointer 
with __pa(), as it may be a vmalloc()'d pointer. Please use 
per_cpu_ptr_to_phys() instead.

Also, even per_cpu_ptr_to_phys() will break if the allocation size is 
larger than the page size, as it only returns the physical address of 
the very first page.

> +
> +	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_SETUP_SHMEM,
> +			MEM_LO(shmem_pa), MEM_HI(shmem_pa), 0, 0, 0, 0);
> +
> +	if (ret.error) {
> +		switch (ret.error) {
> +		case SBI_ERR_DENIED:
> +			pr_warn("Access denied for shared memory at %lx\n",
> +				shmem_pa);
> +			rc = -EPERM;
> +			break;
> +
> +		case SBI_ERR_INVALID_PARAM:
> +		case SBI_ERR_INVALID_ADDRESS:
> +			pr_warn("Invalid address parameter (%lu)\n",
> +				ret.error);
> +			rc = -EINVAL;
> +			break;
> +
> +		case SBI_ERR_ALREADY_AVAILABLE:
> +			pr_warn("Shared memory is already set\n");
> +			rc = -EADDRINUSE;
> +			break;
> +
> +		case SBI_ERR_FAILURE:
> +			pr_err("Internal sdtrig state error\n");
> +			rc = -ENXIO;
> +			break;
> +
> +		default:
> +			pr_warn("Unknown error %lu\n", ret.error);
> +			rc = -ENXIO;
> +			break;
> +		}
> +	}
> +
> +	pr_info("CPU %d: HW Breakpoint shared memory registered.\n", cpu);
This will be printed even if an error occurs. Please move it into the 
`else` block of `if (ret.error)`.
> +
> +	return rc;
> +}
> +
> +static int arch_smp_teardown_sbi_shmem(unsigned int cpu)
> +{
> +	struct sbiret ret;
> +
> +	/* Disable shared memory */
> +	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_SETUP_SHMEM,
> +			-1UL, -1UL, 0, 0, 0, 0);
> +
> +	if (ret.error) {
> +		switch (ret.error) {
> +		case SBI_ERR_DENIED:
> +			pr_err("Access denied for shared memory.\n");
> +			break;
> +
> +		case SBI_ERR_INVALID_PARAM:
> +		case SBI_ERR_INVALID_ADDRESS:
> +			pr_err("Invalid address parameter (%lu)\n", ret.error);
> +			break;
> +
> +		case SBI_ERR_ALREADY_AVAILABLE:
> +			pr_err("Shared memory is already set\n");
> +			break;
> +		case SBI_ERR_FAILURE:
> +			pr_err("Internal sdtrig state error\n");
> +			break;
> +		default:
> +			pr_err("Unknown error %lu\n", ret.error);
> +			break;
> +		}
> +	}
> +
> +	pr_warn("CPU %d: HW Breakpoint shared memory disabled.\n", cpu);
Ditto.
> +
> +	return 0;
> +}

Best regards,

Qingfang