Re: [PATCH 5/8] riscv: hw_breakpoint: Use icount for single stepping

[Jesse T <mr.bossman075@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 15268 bytes --]

On Wed, Aug 27, 2025 at 4:04 AM Charlie Jenkins <charlie@rivosinc.com>
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 <jesse@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.
>

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 <jesse@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.
>
> 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
> > >
>

[-- Attachment #2: Type: text/html, Size: 20744 bytes --]