On Wed, Aug 27, 2025 at 4:04 AM Charlie Jenkins wrote: > On Tue, Aug 26, 2025 at 10:08:04AM +0530, Himanshu Chauhan wrote: > > On Fri, Aug 22, 2025 at 11:17 PM 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. > > > > Can you please elaborate on this? I remember noticing the absence of > > the resume flag which was causing loops. > Yes, signal handler based hardware debugging doesn't work, only ptrace based. ARM64 and ARM also don't support signal handler based hardware debugging as seen in a later patch. Thank you for your feedback. Jesse's internship came to an end last > Friday (August 22nd) so I will be picking up these patches, but I have > also added her personal email onto this thread. > I forgot to CC my personal email sorry.... > When a breakpoint is triggered and the kernel gains control, the last > instruction to execute was the instruction before the instruction where > the breakpoint is installed. If execution was to be resumed at this > stage, the same breakpoint would be triggered again. So single stepping > requires a careful dance of enabling and disabling breakpoints. However, > we can avoid this overhead and code complexity can be reduced by using > the icount trigger which allows a direct method for single stepping. > > > > > > > > > Signed-off-by: Jesse Taube > > > --- > > > 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. > > That is a good point. > Agreed > > > > + } > > > + > > > + /* > > > + * 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. > > Agreed. > I wasn't sure the best way to cite this, Version number and section is ok with me. > > > > + * 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? > > I believe this was intentional, but perhaps it is not a useful print. It is for someone to find out that a spurious debug trigger event happened as it is expecting either an icount event or a mcontrol event at next_addr if this did not happen it will send a SIG_DEBUG to the user process. Thanks, Jesse > - Charlie > > > > > Regards > > Himanshu > > > -- > > > 2.43.0 > > > >