[PATCH v3] lib: sbi: dbtr: do not unconditionally access tdata2/tdata3 CSRs
Himanshu Chauhan
himanshu.chauhan at oss.qualcomm.com
Tue Jun 16 21:08:06 PDT 2026
On Tue, Jun 16, 2026 at 10:31 PM David E. Garcia Porras
<david.garcia at aheadcomputing.com> 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 <npiggin at gmail.com>
> Suggested-by: Himanshu Chauhan <himanshu.chauhan at oss.qualcomm.com>
> Signed-off-by: David E. Garcia Porras <david.garcia at aheadcomputing.com>
> ---
> 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 <himanshu.chauhan at oss.qualcomm.com>
> --
> 2.43.0
More information about the opensbi
mailing list