From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-178.mta1.migadu.com (out-178.mta1.migadu.com [95.215.58.178]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 24F123DC4CE for ; Fri, 17 Jul 2026 07:19:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.178 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784272770; cv=none; b=VV7hDzJytdjLU1pnCS1WCeGCuwbinLfv4IWcr2hNol3wZpEwCT6JmaauqmniN9oALWWufnYhg5MYQo205+qdrFLJuBmmwFY0BFOEdnirVH8u3JGbZv8uv4qruzz8UDVyVeUUz4D1Knu8XchkMu4Vjvvch384MFaGaiZqxyaOTEw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1784272770; c=relaxed/simple; bh=0ttvLAWcj4rf2IECu1wW2kpBabOWd6E2Ul8HyRStlx0=; h=Message-ID:Date:MIME-Version:Subject:To:References:From: In-Reply-To:Content-Type; b=sr/6VlhmlXlzqv/lCc9iZnJ3q/mnkGmtY12Dkf9HLwbewNO28XB4xDLx/jv2EtWZZsaX6umVu31T+Y8tIpstekPiF2ntRqSwhEVl+swJxUVwkjNUc6P8Cd999YZA8ooTvsIbHdDyXx8RH1gqs/BHi/sAJxR4ByNSUHLifrp1q/o= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=w2Rw97yr; arc=none smtp.client-ip=95.215.58.178 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="w2Rw97yr" Message-ID: <765ac2fe-a0ff-4980-915c-b2546a85b86a@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1784272766; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=oiyh43muGYb5yG7m7nFyoOnCjLjuRG2YSiAzdEsauHY=; b=w2Rw97yrn+trEb11MlepQMLUkeql4jIzLT9wpa6NXqVcPhtTNmtfsohe2mh1s4SCxOBFVd 0hRXvGoo2j0WBEZUXkoSOzVfGnY/IiEn9BhuPTS5no3cNsfxL3c3CRc8mKAFUOeGlxLYet wR65RRXYd6PXhJCmTLY8ltkv9g/0gXE= Date: Fri, 17 Jul 2026 15:19:00 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH v4 1/2] riscv: Introduce support for hardware break/watchpoints To: Himanshu Chauhan , linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, pjw@kernel.org, palmer@dabbelt.com, aou@eecs.berkeley.edu, alex@ghiti.fr, shuah@kernel.org References: <20260518065920.872131-1-himanshu.chauhan@oss.qualcomm.com> <20260518065920.872131-2-himanshu.chauhan@oss.qualcomm.com> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Qingfang Deng In-Reply-To: <20260518065920.872131-2-himanshu.chauhan@oss.qualcomm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +/* 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