Re: [PATCH v3] lib: sbi: dbtr: do not unconditionally access tdata2/tdata3 CSRs

On Tue, Jun 16, 2026 at 10:31 PM David E. Garcia Porras
<[email protected]> wrote:
>
> The current SBI DBTR extension implementation accesses tdata2 and tdata3
> without first checking whether either register is implemented on the
> underlying hart. This produces an illegal instruction exception on
> otherwise spec-compliant cores that legitimately omit one or both
> registers.
>
> Per the RISC-V Debug Specification, Chapter 5 (Sdtrig ISA Extension)
> and Section 5.7 (Trigger Module Registers):
>
>   Section 5 (Sdtrig introduction):
>     "If Sdtrig is implemented, the Trigger Module must support at least
>      one trigger. Accessing trigger CSRs that are not used by any of the
>      implemented triggers must result in an illegal instruction
>      exception. M-Mode and Debug Mode accesses to trigger CSRs that are
>      used by any of the implemented triggers must succeed, regardless of
>      the current type of the currently selected trigger."
>
>   Section 5.7 (Trigger Module Registers):
>     "Attempts to access an unimplemented Trigger Module Register raise
>      an illegal instruction exception."
>
> Per-register optionality is also explicit:
>
>   Section 5.7.3 (Trigger Data 2, at 0x7a2):
>     "Trigger-specific data. It is optional if no implemented triggers
>      use it."
>
>   Section 5.7.4 (Trigger Data 3, at 0x7a3):
>     "Trigger-specific data. It is optional if no implemented triggers
>      use it."
>
>   Section 5.7.17 (Trigger Extra (RV32), at 0x7a3), which also applies
>   via textra64 on RV64:
>     "All functionality in this register is optional. Any number of
>      upper bits of mhvalue and svalue may be tied to 0. mhselect and
>      sselect may only support 0 (ignore)."
>
> Unconditionally accessing tdata2/tdata3 in the install/update/read/
> uninstall paths causes SBI calls to fail with an illegal instruction
> exception on hardware that does not implement one or both CSRs, even
> if the supervisor-supplied trigger configuration does not require the
> missing CSR(s).
>
> This patch:
>
>   1. Introduces tdata_read_safe() / tdata_write_safe() helpers that
>      wrap csr_read_allowed / csr_write_allowed so that an illegal-
>      instruction trap raised by an unimplemented CSR is caught locally
>      rather than propagated. On the read path, a trapped read yields
>      zero; on the write path, the trap is silently absorbed (writes to
>      an unimplemented CSR are no-ops by definition). Every tdata2/tdata3
>      read and write in the install/update/read/uninstall paths is
>      converted to these helpers.
>
>   2. On the install and update paths, rejects requests that program
>      a non-zero trig_tdata2 or trig_tdata3 into an unimplemented CSR
>      with SBI_ERR_NOT_SUPPORTED, matching the SBI spec
>      wording in sections 19.4 / 19.5:
>
>        "One of the trigger configuration can't be programmed due to
>         unimplemented optional bits in tdata1, tdata2, or tdata3
>         CSRs."
>
>      Implementation status is probed once per call via the
>      tdata_implemented() helper. This only catches the "whole CSR
>      unimplemented" case; tied-off WARL bits inside an otherwise-
>      implemented CSR are not caught here and would require programming
>      the trigger and reading the value back for comparison, which can
>      be addressed separately.
>
>   3. Enable tdata3 configuration in the debug trigger install path.
>
> References:
>   - RISC-V Debug Specification, Chapter 5 (Sdtrig), sections 5, 5.7,
>     5.7.3, 5.7.4, 5.7.17.
>   - RISC-V SBI Specification v3.0, Chapter 19 (Debug Triggers
>     Extension), sections 19.4, 19.5.
>
> Fixes: 97f234f15c96 ("lib: sbi: Introduce the SBI debug triggers extension support")
> Suggested-by: Nicholas Piggin <[email protected]>
> Suggested-by: Himanshu Chauhan <[email protected]>
> Signed-off-by: David E. Garcia Porras <[email protected]>
> ---
> Changes since v2:
>  - Wrapped the repeated csr_read_allowed / csr_write_allowed +
>    sbi_trap_info boilerplate in tdata_read_safe() / tdata_write_safe()
>    statement-expression macros, and the per-call implementation probe
>    in a tdata_implemented() macro returning a boolean, per Himanshu
>    Chauhan's review. All open-coded tdata2/tdata3 reads, writes and
>    probes now go through these helpers.
>  - The install / update SBI_ERR_NOT_SUPPORTED checks now call
>    tdata_implemented() and are collapsed into a single condition per
>    path.
>
> Changes since v1:
>  - Replaced the boot-time probe and the cached per-hart
>    tdata2_supported / tdata3_supported flags with per-access
>    csr_read_allowed / csr_write_allowed at every tdata2/tdata3 site,
>    per Nicholas Piggin's review. This covers both spec-compliant
>    cores and implementations whose trap behavior depends on the
>    currently selected trigger type (e.g. QEMU's tdata_available()).
>  - Dropped the additions to struct sbi_dbtr_hart_triggers_state.
>  - Install / update SBI_ERR_NOT_SUPPORTED checks now probe via
>    csr_read_allowed once per call. Added a comment noting that this
>    only catches the "whole CSR unimplemented" case; tied-off WARL
>    bits inside an implemented CSR are not detected and can be
>    addressed in a follow-up.
>
>  lib/sbi/sbi_dbtr.c | 63 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
> index 8bcb4312..01047969 100644
> --- a/lib/sbi/sbi_dbtr.c
> +++ b/lib/sbi/sbi_dbtr.c
> @@ -34,6 +34,25 @@ static unsigned long hart_state_ptr_offset;
>         sbi_scratch_write_type((__scratch), void *, hart_state_ptr_offset, \
>                                (__hart_state))
>
> +#define tdata_read_safe(__csr)                                         \
> +       ({                                                              \
> +               struct sbi_trap_info __trap = {0};                      \
> +               csr_read_allowed((__csr), &__trap);                     \
> +       })
> +
> +#define tdata_write_safe(__csr, __value)                               \
> +       ({                                                              \
> +               struct sbi_trap_info __trap = {0};                      \
> +               csr_write_allowed((__csr), &__trap, (__value));         \
> +       })
> +
> +#define tdata_implemented(__csr)                                       \
> +       ({                                                              \
> +               struct sbi_trap_info __trap = {0};                      \
> +               csr_read_allowed((__csr), &__trap);                     \
> +               !__trap.cause;                                          \
> +       })
> +
>  #define INDEX_TO_TRIGGER(_index)                                       \
>         ({                                                              \
>                 struct sbi_dbtr_trigger *__trg = NULL;                  \
> @@ -418,7 +437,8 @@ static void dbtr_trigger_enable(struct sbi_dbtr_trigger *trig)
>          */
>         csr_write(CSR_TSELECT, trig->index);
>         csr_write(CSR_TDATA1, 0x0);
> -       csr_write(CSR_TDATA2, trig->tdata2);
> +       tdata_write_safe(CSR_TDATA2, trig->tdata2);
> +       tdata_write_safe(CSR_TDATA3, trig->tdata3);
>         csr_write(CSR_TDATA1, trig->tdata1);
>  }
>
> @@ -463,7 +483,8 @@ static void dbtr_trigger_clear(struct sbi_dbtr_trigger *trig)
>
>         csr_write(CSR_TSELECT, trig->index);
>         csr_write(CSR_TDATA1, 0x0);
> -       csr_write(CSR_TDATA2, 0x0);
> +       tdata_write_safe(CSR_TDATA2, 0x0);
> +       tdata_write_safe(CSR_TDATA3, 0x0);
>  }
>
>  static int dbtr_trigger_supported(unsigned long type)
> @@ -566,8 +587,8 @@ int sbi_dbtr_read_trig(unsigned long smode,
>                 trig = INDEX_TO_TRIGGER((_idx + trig_idx_base));
>                 csr_write(CSR_TSELECT, trig->index);
>                 trig->tdata1 = csr_read(CSR_TDATA1);
> -               trig->tdata2 = csr_read(CSR_TDATA2);
> -               trig->tdata3 = csr_read(CSR_TDATA3);
> +               trig->tdata2 = tdata_read_safe(CSR_TDATA2);
> +               trig->tdata3 = tdata_read_safe(CSR_TDATA3);
>                 xmit->tstate = cpu_to_lle(trig->state);
>                 xmit->tdata1 = cpu_to_lle(trig->tdata1);
>                 xmit->tdata2 = cpu_to_lle(trig->tdata2);
> @@ -589,6 +610,7 @@ int sbi_dbtr_install_trig(unsigned long smode,
>         unsigned long ctrl;
>         struct sbi_dbtr_trigger *trig;
>         struct sbi_dbtr_hart_triggers_state *hs = NULL;
> +       bool tdata2_impl, tdata3_impl;
>
>         hs = dbtr_thishart_state_ptr();
>         if (!hs)
> @@ -601,6 +623,15 @@ int sbi_dbtr_install_trig(unsigned long smode,
>         sbi_hart_protection_map_range((unsigned long)shmem_base,
>                                       trig_count * sizeof(*entry));
>
> +       /*
> +        * SBI v3.0 sec 19.4 requires SBI_ERR_NOT_SUPPORTED when a trigger
> +        * programs a non-zero value into an unimplemented optional CSR. Only
> +        * the "whole CSR unimplemented" case is caught; WARL bits tied off
> +        * inside an otherwise-implemented CSR are not.
> +        */
> +       tdata2_impl = tdata_implemented(CSR_TDATA2);
> +       tdata3_impl = tdata_implemented(CSR_TDATA3);
> +
>         /* Check requested triggers configuration */
>         for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
>                 recv = (struct sbi_dbtr_data_msg *)(&entry->data);
> @@ -619,6 +650,14 @@ int sbi_dbtr_install_trig(unsigned long smode,
>                                                         trig_count * sizeof(*entry));
>                         return SBI_ERR_FAILED;
>                 }
> +
> +               if ((recv->tdata2 && !tdata2_impl) ||
> +                   (recv->tdata3 && !tdata3_impl)) {
> +                       *out = _idx;
> +                       sbi_hart_protection_unmap_range((unsigned long)shmem_base,
> +                                                       trig_count * sizeof(*entry));
> +                       return SBI_ERR_NOT_SUPPORTED;
> +               }
>         }
>
>         if (hs->available_trigs < trig_count) {
> @@ -705,6 +744,7 @@ int sbi_dbtr_update_trig(unsigned long smode,
>         union sbi_dbtr_shmem_entry *entry;
>         void *shmem_base = NULL;
>         struct sbi_dbtr_hart_triggers_state *hs = NULL;
> +       bool tdata2_impl, tdata3_impl;
>
>         hs = dbtr_thishart_state_ptr();
>         if (!hs)
> @@ -718,6 +758,15 @@ int sbi_dbtr_update_trig(unsigned long smode,
>         if (trig_count >= hs->total_trigs)
>                 return SBI_ERR_BAD_RANGE;
>
> +       /*
> +        * SBI v3.0 sec 19.5 requires SBI_ERR_NOT_SUPPORTED when a trigger
> +        * programs a non-zero value into an unimplemented optional CSR. Only
> +        * the "whole CSR unimplemented" case is caught; WARL bits tied off
> +        * inside an otherwise-implemented CSR are not.
> +        */
> +       tdata2_impl = tdata_implemented(CSR_TDATA2);
> +       tdata3_impl = tdata_implemented(CSR_TDATA3);
> +
>         for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
>                 sbi_hart_protection_map_range((unsigned long)entry, sizeof(*entry));
>                 trig_idx = entry->id.idx;
> @@ -734,6 +783,12 @@ int sbi_dbtr_update_trig(unsigned long smode,
>                         return SBI_ERR_FAILED;
>                 }
>
> +               if ((entry->data.tdata2 && !tdata2_impl) ||
> +                   (entry->data.tdata3 && !tdata3_impl)) {
> +                       sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
> +                       return SBI_ERR_NOT_SUPPORTED;
> +               }
> +
>                 dbtr_trigger_setup(trig, &entry->data);
>                 sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
>                 dbtr_trigger_enable(trig);

Thanks David.
Looks good to me.

Reviewed-By: Himanshu Chauhan <[email protected]>

> --
> 2.43.0

-- 
opensbi mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi