[PATCH v2] lib: sbi: dbtr: do not unconditionally access tdata2/tdata3 CSRs
Himanshu Chauhan
himanshu.chauhan at oss.qualcomm.com
Sun Jun 14 22:33:37 PDT 2026
Hi David,
Thank you for your patch. Few comments inline.
On Tue, May 26, 2026 at 10:57 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. 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 <npiggin at gmail.com>
> Signed-off-by: David E. Garcia Porras <david.garcia at aheadcomputing.com>
> ---
> 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
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
More information about the opensbi
mailing list