On Fri, Aug 22, 2025 at 11:17 PM Jesse Taube <je...@rivosinc.com> 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.
Can you please elaborate on this? I remember noticing the absence of the resume flag which was causing loops. > > Signed-off-by: Jesse Taube <je...@rivosinc.com> > --- > OpenSBI implementation of sbi_debug_read_triggers does not return the > updated CSR values. There needs to be a check for working > sbi_debug_read_triggers before this works. > > https://lists.riscv.org/g/tech-prs/message/1476 > > RFC -> V1: > - Add dbtr_mode to rv_init_icount_trigger > - Add icount_triggered to check which breakpoint was triggered > - Fix typo: s/affects/effects > - Move HW_BREAKPOINT_COMPUTE_STEP to Platform type > V1 -> V2: > - Remove HW_BREAKPOINT_COMPUTE_STEP kconfig option > --- > arch/riscv/kernel/hw_breakpoint.c | 173 ++++++++++++++++++++++++++---- > 1 file changed, 155 insertions(+), 18 deletions(-) > > diff --git a/arch/riscv/kernel/hw_breakpoint.c > b/arch/riscv/kernel/hw_breakpoint.c > index 3f96e744a711..f12306247436 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) > @@ -62,6 +63,14 @@ > (FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZELO_FIELD, lo) | \ > FIELD_PREP(DBTR_TDATA1_MCONTROL_SIZEHI_FIELD, hi)) > > +#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) > + > enum dbtr_mode { > DBTR_MODE_U = 0, > DBTR_MODE_S, > @@ -79,6 +88,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; > > @@ -129,6 +139,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. > @@ -143,6 +154,19 @@ 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) { > + pr_warn("%s: No icount triggers available. " > + "Falling-back to computing single step address.\n", > __func__); > + } 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) { > @@ -151,7 +175,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; > } > @@ -166,7 +190,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; > } > } > @@ -320,6 +344,36 @@ 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, enum > dbtr_mode mode) > +{ > + unsigned long tdata1 = DBTR_TDATA1_TYPE_ICOUNT; > + > + /* Step one instruction */ > + tdata1 |= FIELD_PREP(DBTR_TDATA1_ICOUNT_COUNT_FIELD, 1); > + > + switch (mode) { > + case DBTR_MODE_U: > + tdata1 |= DBTR_TDATA1_ICOUNT_U; > + break; > + case DBTR_MODE_S: > + tdata1 |= DBTR_TDATA1_ICOUNT_S; > + break; > + case DBTR_MODE_VS: > + tdata1 |= DBTR_TDATA1_ICOUNT_VS; > + break; > + case DBTR_MODE_VU: > + tdata1 |= DBTR_TDATA1_ICOUNT_VU; > + break; > + default: > + return -EINVAL; > + } > + > + 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) > @@ -372,24 +426,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_nofault(regs, regs->epc, &insn); > - if (ret < 0) > - return ret; > + if (have_icount) { > + rv_init_icount_trigger(bp, DBTR_MODE_U); > + } else { > + ret = get_insn_nofault(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_nofault(regs, next_addr, &insn); > - if (ret < 0) > - return ret; > + ret = get_insn_nofault(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) > @@ -400,6 +458,79 @@ static int setup_singlestep(struct perf_event *event, > struct pt_regs *regs) > return 0; > } > > +/** > + * icount_triggered - Check if event's icount was triggered. > + * @event: Perf event to check > + * > + * Check the given perf event's icount breakpoint was triggered. > + * > + * Returns: 1 if icount was triggered. > + * 0 if icount was not triggered. > + * negative on failure. > + */ > +static int icount_triggered(struct perf_event *event) > +{ > + union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(&sbi_dbtr_shmem); > + struct sbiret ret; > + struct perf_event **slot; > + unsigned long tdata1; > + int i; > + > + for (i = 0; i < dbtr_total_num; i++) { > + slot = this_cpu_ptr(&pcpu_hw_bp_events[i]); > + > + if (*slot == event) > + break; > + } > + > + if (i == dbtr_total_num) { > + pr_warn("%s: Breakpoint not installed.\n", __func__); > + return -ENOENT; > + } > + > + raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock), > + *this_cpu_ptr(&ecall_lock_flags)); > + > + ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_READ, > + i, 1, 0, 0, 0, 0); > + tdata1 = shmem->data.tdata1; > + > + raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock), > + *this_cpu_ptr(&ecall_lock_flags)); > + if (ret.error) { > + pr_warn("%s: failed to read trigger. error: %ld\n", __func__, > ret.error); > + return sbi_err_map_linux_errno(ret.error); To avoid a flurry of events or messages, it would probably be good to disable the trigger. > + } > + > + /* > + * The RISC-V Debug Specification > + * Tim Newsome, Paul Donahue (Ventana Micro Systems) > + * Version 1.0, Revised 2025-02-21: Ratified I think mentioning the version number and section would be enough. > + * 5.7.13. Instruction Count (icount, at 0x7a1) > + * When count is 1 and the trigger matches, then pending becomes set. > + * In addition count will become 0 unless it is hard-wired to 1. > + * When pending is set, the trigger fires just before any further > + * instructions are executed in a mode where the trigger is enabled. > + * As the trigger fires, pending is cleared. In addition, if count is > + * hard-wired to 1 then m, s, u, vs, and vu are all cleared. > + */ > + if (FIELD_GET(DBTR_TDATA1_ICOUNT_COUNT_FIELD, tdata1) == 0) > + return 1; > + > + if (FIELD_GET(DBTR_TDATA1_ICOUNT_COUNT_FIELD, tdata1) != 1) > + return 0; > + > + if (tdata1 & DBTR_TDATA1_ICOUNT_U) > + return 0; > + if (tdata1 & DBTR_TDATA1_ICOUNT_S) > + return 0; > + if (tdata1 & DBTR_TDATA1_ICOUNT_VU) > + return 0; > + if (tdata1 & DBTR_TDATA1_ICOUNT_VU) > + return 0; > + return 1; > +} > + > /* > * HW Breakpoint/watchpoint handler > */ > @@ -460,7 +591,10 @@ static int hw_breakpoint_handler(struct pt_regs *regs) > > if (bp->in_callback) { > expecting_callback = true; > - if (regs->epc != bp->next_addr) { > + if (have_icount) { > + if (icount_triggered(event) != 1) > + continue; > + } else if (regs->epc != bp->next_addr) { > continue; > } > > @@ -477,7 +611,10 @@ static int hw_breakpoint_handler(struct pt_regs *regs) > > } > > - if (expecting_callback) { > + if (expecting_callback && have_icount) { > + pr_err("%s: in_callback was set, but icount was not > triggered, epc (%lx).\n", > + __func__, regs->epc); > + } else if (expecting_callback) { > pr_err("%s: in_callback was set, but epc (%lx) was not at > next address(%lx).\n", > __func__, regs->epc, bp->next_addr); > } Is this just for debugging or do you want to commit it? Regards Himanshu > -- > 2.43.0 >
