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

Hi David,

Thank you for your patch. Few comments inline.

On Tue, May 26, 2026 at 10:57 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. Wraps every tdata2/tdata3 read and write in 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).
>
>   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
>      csr_read_allowed. 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]>
> Signed-off-by: David E. Garcia Porras <[email protected]>
> ---
> 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 | 76 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 72 insertions(+), 4 deletions(-)
>
> diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
> index 8bcb4312..aeb92f58 100644
> --- a/lib/sbi/sbi_dbtr.c
> +++ b/lib/sbi/sbi_dbtr.c
> @@ -367,6 +367,7 @@ static inline void update_bit(unsigned long new, int nr, volatile unsigned long
>
>  static void dbtr_trigger_enable(struct sbi_dbtr_trigger *trig)
>  {
> +       struct sbi_trap_info trap = {0};
>         unsigned long state;
>         unsigned long tdata1;
>
> @@ -418,7 +419,15 @@ 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);
> +       /*
> +        * tdata2 and tdata3 are optional in the RISC-V Sdtrig extension.
> +        * Use csr_write_allowed so that writing to an unimplemented CSR
> +        * traps locally and is silently absorbed; install/update have
> +        * already rejected non-zero requested values for unimplemented
> +        * CSRs.
> +        */
> +       csr_write_allowed(CSR_TDATA2, &trap, trig->tdata2);
> +       csr_write_allowed(CSR_TDATA3, &trap, trig->tdata3);
>         csr_write(CSR_TDATA1, trig->tdata1);
>  }
>
> @@ -458,12 +467,16 @@ static void dbtr_trigger_disable(struct sbi_dbtr_trigger *trig)
>
>  static void dbtr_trigger_clear(struct sbi_dbtr_trigger *trig)
>  {
> +       struct sbi_trap_info trap = {0};
> +
>         if (!trig || !(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)))
>                 return;
>
>         csr_write(CSR_TSELECT, trig->index);
>         csr_write(CSR_TDATA1, 0x0);
> -       csr_write(CSR_TDATA2, 0x0);
> +       /* Clearing an unimplemented tdata2/tdata3 is a no-op; absorb the trap. */
> +       csr_write_allowed(CSR_TDATA2, &trap, 0x0);
> +       csr_write_allowed(CSR_TDATA3, &trap, 0x0);
>  }
>
>  static int dbtr_trigger_supported(unsigned long type)
> @@ -562,12 +575,14 @@ int sbi_dbtr_read_trig(unsigned long smode,
>         sbi_hart_protection_map_range((unsigned long)shmem_base,
>                                       trig_count * sizeof(*entry));
>         for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
> +               struct sbi_trap_info trap = {0};
> +
>                 xmit = &entry->data;
>                 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 = csr_read_allowed(CSR_TDATA2, &trap);
> +               trig->tdata3 = csr_read_allowed(CSR_TDATA3, &trap);

I would suggest that you wrap sbi_trap_info and csr_read_allowed() is
a macro or inline function with a "safe" name suffix. Something like
"read_tadata2_safe()". It will return the value or zero as
csr_read_allowed does today. Define same semantics for writes. Replace
read/write with these safe functions.

>                 xmit->tstate = cpu_to_lle(trig->state);
>                 xmit->tdata1 = cpu_to_lle(trig->tdata1);
>                 xmit->tdata2 = cpu_to_lle(trig->tdata2);
> @@ -589,6 +604,8 @@ int sbi_dbtr_install_trig(unsigned long smode,
>         unsigned long ctrl;
>         struct sbi_dbtr_trigger *trig;
>         struct sbi_dbtr_hart_triggers_state *hs = NULL;
> +       struct sbi_trap_info trap = {0};
> +       bool tdata2_impl, tdata3_impl;
>
>         hs = dbtr_thishart_state_ptr();
>         if (!hs)
> @@ -601,6 +618,19 @@ int sbi_dbtr_install_trig(unsigned long smode,
>         sbi_hart_protection_map_range((unsigned long)shmem_base,
>                                       trig_count * sizeof(*entry));
>
> +       /*
> +        * Probe tdata2/tdata3 implementation status for the
> +        * SBI v3.0 sec 19.4/19.5 mandatory SBI_ERR_NOT_SUPPORTED check
> +        * below. This only catches the "whole CSR unimplemented" case;
> +        * WARL bits tied off inside an otherwise-implemented CSR are
> +        * not caught here.
> +        */
> +       csr_read_allowed(CSR_TDATA2, &trap);
> +       tdata2_impl = !trap.cause;
> +       trap.cause = 0;
> +       csr_read_allowed(CSR_TDATA3, &trap);
> +       tdata3_impl = !trap.cause;

Similarly, wrap this in an inline function or statement-expression
macro returning boolean value.

> +
>         /* 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 +649,20 @@ int sbi_dbtr_install_trig(unsigned long smode,
>                                                         trig_count * sizeof(*entry));
>                         return SBI_ERR_FAILED;
>                 }
> +
> +               if (recv->tdata2 && !tdata2_impl) {

Call the macro above here and other similar places.

> +                       *out = _idx;
> +                       sbi_hart_protection_unmap_range((unsigned long)shmem_base,
> +                                                       trig_count * sizeof(*entry));
> +                       return SBI_ERR_NOT_SUPPORTED;
> +               }
> +
> +               if (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 +749,8 @@ 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;
> +       struct sbi_trap_info trap = {0};
> +       bool tdata2_impl, tdata3_impl;
>
>         hs = dbtr_thishart_state_ptr();
>         if (!hs)
> @@ -718,6 +764,18 @@ int sbi_dbtr_update_trig(unsigned long smode,
>         if (trig_count >= hs->total_trigs)
>                 return SBI_ERR_BAD_RANGE;
>
> +       /*
> +        * Probe tdata2/tdata3 implementation status for the
> +        * SBI v3.0 sec 19.4/19.5 mandatory SBI_ERR_NOT_SUPPORTED check
> +        * below. Only the "whole CSR unimplemented" case is caught
> +        * here; tied-off WARL bits inside an implemented CSR are not.
> +        */
> +       csr_read_allowed(CSR_TDATA2, &trap);
> +       tdata2_impl = !trap.cause;
> +       trap.cause = 0;
> +       csr_read_allowed(CSR_TDATA3, &trap);
> +       tdata3_impl = !trap.cause;
> +
>         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 +792,16 @@ int sbi_dbtr_update_trig(unsigned long smode,
>                         return SBI_ERR_FAILED;
>                 }
>
> +               if (entry->data.tdata2 && !tdata2_impl) {
> +                       sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
> +                       return SBI_ERR_NOT_SUPPORTED;
> +               }
> +
> +               if (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
Regards
Himanshu

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

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