Message-ID: <b37f6f25-7569-4cae-b33d-61ed14b19611@rivosinc.com>
Date: Mon, 4 Aug 2025 10:15:22 +0200
From: Clément Léger <cleger@...osinc.com>
To: Jesse Taube <jesse@...osinc.com>, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Cc: Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Alexandre Ghiti <alex@...ti.fr>, Oleg Nesterov <oleg@...hat.com>,
 Himanshu Chauhan <hchauhan@...tanamicro.com>,
 Charlie Jenkins <charlie@...osinc.com>,
 Samuel Holland <samuel.holland@...ive.com>, Deepak Gupta
 <debug@...osinc.com>, Andrew Jones <ajones@...tanamicro.com>,
 Atish Patra <atishp@...osinc.com>, Anup Patel <apatel@...tanamicro.com>,
 Mayuresh Chitale <mchitale@...tanamicro.com>,
 Conor Dooley <conor.dooley@...rochip.com>, WangYuli
 <wangyuli@...ontech.com>, Huacai Chen <chenhuacai@...nel.org>,
 Nam Cao <namcao@...utronix.de>, Andrew Morton <akpm@...ux-foundation.org>,
 "Mike Rapoport (Microsoft)" <rppt@...nel.org>,
 Luis Chamberlain <mcgrof@...nel.org>, Yunhui Cui <cuiyunhui@...edance.com>,
 Joel Granados <joel.granados@...nel.org>,
 Celeste Liu <coelacanthushex@...il.com>, Evan Green <evan@...osinc.com>,
 Nylon Chen <nylon.chen@...ive.com>
Subject: Re: [RFC PATCH 5/6] riscv: hw_breakpoint: Use icount for single
 stepping



On 22/07/2025 19:38, Jesse Taube wrote:
> The Sdtrig RISC-V ISA extension does not have a resume flag for
> returning to and executing the instruction at the breakpoint.
> To avoid skipping the instruction or looping, it is necessary to remove
> the hardware breakpoint and single step. Use the icount feature of
> Sdtrig to accomplish this. Use icount as default with an option to allow
> software-based single stepping when hardware or SBI does not have
> icount functionality, as it may cause unwanted side effects when reading
> the instruction from memory.
> 
> Signed-off-by: Jesse Taube <jesse@...osinc.com>
> ---
>  arch/riscv/Kconfig                | 11 +++++
>  arch/riscv/kernel/hw_breakpoint.c | 81 +++++++++++++++++++++++++------
>  2 files changed, 76 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 95d3047cab10..bbde5e118470 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -1105,6 +1105,17 @@ config PARAVIRT_TIME_ACCOUNTING
>  
>  	  If in doubt, say N here.
>  
> +config HW_BREAKPOINT_COMPUTE_STEP
> +	bool "Allow computing hardware breakpoint step address"
> +	default n
> +	depends on HAVE_HW_BREAKPOINT
> +	help
> +	  Select this option if hardware breakpoints are desired, but
> +	  hardware or SBI does not have icount functionality. This may cause
> +	  unwanted side affects when reading the instruction from memory.

Hi Jesse,

Typo: s/affects/effects

Thanks,

Clément

> +
> +	  If unsure, say N.
> +
>  config RELOCATABLE
>  	bool "Build a relocatable kernel"
>  	depends on !XIP_KERNEL
> diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
> index 9e3a3b82d300..437fd82b9590 100644
> --- a/arch/riscv/kernel/hw_breakpoint.c
> +++ b/arch/riscv/kernel/hw_breakpoint.c
> @@ -20,6 +20,7 @@
>  #define DBTR_TDATA1_DMODE		BIT_UL(__riscv_xlen - 5)
>  
>  #define DBTR_TDATA1_TYPE_MCONTROL	(2UL << DBTR_TDATA1_TYPE_SHIFT)
> +#define DBTR_TDATA1_TYPE_ICOUNT		(3UL << DBTR_TDATA1_TYPE_SHIFT)
>  #define DBTR_TDATA1_TYPE_MCONTROL6	(6UL << DBTR_TDATA1_TYPE_SHIFT)
>  
>  #define DBTR_TDATA1_MCONTROL6_LOAD		BIT(0)
> @@ -55,6 +56,14 @@
>  #define DBTR_TDATA1_MCONTROL_SIZELO_64BIT	1
>  #define DBTR_TDATA1_MCONTROL_SIZEHI_64BIT	1
>  
> +#define DBTR_TDATA1_ICOUNT_U			BIT(6)
> +#define DBTR_TDATA1_ICOUNT_S			BIT(7)
> +#define DBTR_TDATA1_ICOUNT_PENDING		BIT(8)
> +#define DBTR_TDATA1_ICOUNT_M			BIT(9)
> +#define DBTR_TDATA1_ICOUNT_COUNT_FIELD		GENMASK(23, 10)
> +#define DBTR_TDATA1_ICOUNT_VU			BIT(25)
> +#define DBTR_TDATA1_ICOUNT_VS			BIT(26)
> +
>  /* Registered per-cpu bp/wp */
>  static DEFINE_PER_CPU(struct perf_event *, pcpu_hw_bp_events[RV_MAX_TRIGGERS]);
>  static DEFINE_PER_CPU(unsigned long, ecall_lock_flags);
> @@ -65,6 +74,7 @@ static DEFINE_PER_CPU(union sbi_dbtr_shmem_entry, sbi_dbtr_shmem);
>  
>  /* number of debug triggers on this cpu . */
>  static int dbtr_total_num __ro_after_init;
> +static bool have_icount __ro_after_init;
>  static unsigned long dbtr_type __ro_after_init;
>  static unsigned long dbtr_init __ro_after_init;
>  
> @@ -168,6 +178,7 @@ static int arch_smp_teardown_sbi_shmem(unsigned int cpu)
>  static void init_sbi_dbtr(void)
>  {
>  	struct sbiret ret;
> +	unsigned long dbtr_count = 0;
>  
>  	/*
>  	 * Called by hw_breakpoint_slots and arch_hw_breakpoint_init.
> @@ -182,6 +193,25 @@ static void init_sbi_dbtr(void)
>  		return;
>  	}
>  
> +	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
> +		DBTR_TDATA1_TYPE_ICOUNT, 0, 0, 0, 0, 0);
> +	if (ret.error) {
> +		pr_warn("%s: failed to detect icount triggers. error: %ld.\n",
> +			__func__, ret.error);
> +	} else if (!ret.value) {
> +		if (IS_ENABLED(CONFIG_HW_BREAKPOINT_COMPUTE_STEP)) {
> +			pr_warn("%s: No icount triggers available. "
> +				"Falling-back to computing single step address.\n", __func__);
> +		} else {
> +			pr_err("%s: No icount triggers available.\n", __func__);
> +			dbtr_total_num = 0;
> +			return;
> +		}
> +	} else {
> +		dbtr_count = ret.value;
> +		have_icount = true;
> +	}
> +
>  	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_NUM_TRIGGERS,
>  			DBTR_TDATA1_TYPE_MCONTROL6, 0, 0, 0, 0, 0);
>  	if (ret.error) {
> @@ -190,7 +220,7 @@ static void init_sbi_dbtr(void)
>  	} else if (!ret.value) {
>  		pr_warn("%s: No mcontrol6 triggers available.\n", __func__);
>  	} else {
> -		dbtr_total_num = ret.value;
> +		dbtr_total_num = min_not_zero((unsigned long)ret.value, dbtr_count);
>  		dbtr_type = DBTR_TDATA1_TYPE_MCONTROL6;
>  		return;
>  	}
> @@ -205,7 +235,7 @@ static void init_sbi_dbtr(void)
>  		pr_err("%s: No mcontrol triggers available.\n", __func__);
>  		dbtr_total_num = 0;
>  	} else {
> -		dbtr_total_num = ret.value;
> +		dbtr_total_num = min_not_zero((unsigned long)ret.value, dbtr_count);
>  		dbtr_type = DBTR_TDATA1_TYPE_MCONTROL;
>  	}
>  }
> @@ -344,6 +374,21 @@ static int rv_init_mcontrol6_trigger(const struct perf_event_attr *attr,
>  	return 0;
>  }
>  
> +static int rv_init_icount_trigger(struct arch_hw_breakpoint *hw)
> +{
> +	unsigned long tdata1 = DBTR_TDATA1_TYPE_ICOUNT;
> +
> +	/* Step one instruction */
> +	tdata1 |= FIELD_PREP(DBTR_TDATA1_ICOUNT_COUNT_FIELD, 1);
> +
> +	tdata1 |= DBTR_TDATA1_ICOUNT_U;
> +
> +	hw->tdata1 = tdata1;
> +	hw->tdata2 = 0;
> +
> +	return 0;
> +}
> +
>  int hw_breakpoint_arch_parse(struct perf_event *bp,
>  			     const struct perf_event_attr *attr,
>  			     struct arch_hw_breakpoint *hw)
> @@ -389,24 +434,28 @@ static int setup_singlestep(struct perf_event *event, struct pt_regs *regs)
>  	/* Remove breakpoint even if return error as not to loop */
>  	arch_uninstall_hw_breakpoint(event);
>  
> -	ret = get_insn(regs, regs->epc, &insn);
> -	if (ret < 0)
> -		return ret;
> +	if (have_icount) {
> +		rv_init_icount_trigger(bp);
> +	} else {
> +		ret = get_insn(regs, regs->epc, &insn);
> +		if (ret < 0)
> +			return ret;
>  
> -	next_addr = get_step_address(regs, insn);
> +		next_addr = get_step_address(regs, insn);
>  
> -	ret = get_insn(regs, next_addr, &insn);
> -	if (ret < 0)
> -		return ret;
> +		ret = get_insn(regs, next_addr, &insn);
> +		if (ret < 0)
> +			return ret;
>  
> -	bp_insn.bp_type = HW_BREAKPOINT_X;
> -	bp_insn.bp_addr = next_addr;
> -	/* Get the size of the intruction */
> -	bp_insn.bp_len = GET_INSN_LENGTH(insn);
> +		bp_insn.bp_type = HW_BREAKPOINT_X;
> +		bp_insn.bp_addr = next_addr;
> +		/* Get the size of the intruction */
> +		bp_insn.bp_len = GET_INSN_LENGTH(insn);
>  
> -	ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp);
> -	if (ret)
> -		return ret;
> +		ret = hw_breakpoint_arch_parse(NULL, &bp_insn, bp);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = arch_install_hw_breakpoint(event);
>  	if (ret)

Powered by blists - more mailing lists