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

[Himanshu Chauhan <hchauhan@ventanamicro.com>]

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.

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

> +       }
> +
> +       /*
> +        * 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
>