[PATCH v2] lib: sbi: dbtr: do not unconditionally access tdata2/tdata3 CSRs
David E. Garcia Porras
david.garcia at aheadcomputing.com
Tue May 26 10:26:51 PDT 2026
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);
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;
+
/* 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) {
+ *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);
--
2.43.0
More information about the opensbi
mailing list