[PATCH] lib: sbi: dbtr: do not unconditionally access tdata2/tdata3 CSRs
David E. Garcia Porras
david.garcia at aheadcomputing.com
Mon May 25 11:50:33 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)."
Not probing for tdata2/tdata3 support at boot and unconditionally accessing
them 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. Probes tdata2 and tdata3 implementation at boot, per hart, by
attempting a guarded read with the M-mode illegal-instruction
trap handler temporarily set to record-and-skip. The presence of
each register is cached in the per-hart DBTR state.
2. Gates every read/write of tdata2 and tdata3 in the install, update,
read, and uninstall paths on the corresponding presence flag.
3. When a trigger configuration in shared memory carries a non-zero
trig_tdata2 or trig_tdata3 that the hart cannot honor because the
CSR is not implemented, the offending entry is rejected with
SBI_ERR_NOT_SUPPORTED, matching the spec wording in §19.4/§19.5
of the SBI v3.0 specification:
"One of the trigger configuration can't be programmed due to
unimplemented optional bits in tdata1, tdata2, or tdata3 CSRs."
4. When the supervisor-supplied trig_tdata2 / trig_tdata3 are zero
and the CSR is unimplemented, the corresponding CSR write is
skipped (it would be a no-op anyway) and the operation proceeds.
On the uninstall path, clearing of an unimplemented tdata2/tdata3
is likewise skipped.
5. sbi_debug_read_triggers fills the corresponding shared-memory
word with zero for any unimplemented register, which is the
correct "would-read-as" value for a tied-off WARL register.
6. Enable tdata3 configuration in the debug trigger install path.
References:
- RISC-V Debug Specification, Chapter 5 (Sdtrig), §5, §5.7, §5.7.2,
§5.7.3, §5.7.4, §5.7.17.
- RISC-V SBI Specification v3.0, Chapter 19 (Debug Triggers
Extension), §19.4, §19.5.
Fixes: 97f234f15c96 ("lib: sbi: Introduce the SBI debug triggers extension support")
Signed-off-by: David E. Garcia Porras <david.garcia at aheadcomputing.com>
---
include/sbi/sbi_dbtr.h | 2 ++
lib/sbi/sbi_dbtr.c | 60 ++++++++++++++++++++++++++++++++++++++----
2 files changed, 58 insertions(+), 4 deletions(-)
diff --git a/include/sbi/sbi_dbtr.h b/include/sbi/sbi_dbtr.h
index 5e0bf84e..723d30c8 100644
--- a/include/sbi/sbi_dbtr.h
+++ b/include/sbi/sbi_dbtr.h
@@ -75,6 +75,8 @@ struct sbi_dbtr_hart_triggers_state {
u32 available_trigs;
u32 hartid;
u32 probed;
+ bool tdata2_supported;
+ bool tdata3_supported;
};
#define TDATA1_GET_TYPE(_t1) \
diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 8bcb4312..e17f697e 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -211,6 +211,17 @@ int sbi_dbtr_init(struct sbi_scratch *scratch, bool coldboot)
}
}
+ /*
+ * Probe tdata2/tdata3 support. These CSRs are optional in the
+ * RISC-V Sdtrig extension: accessing them on hardware that does
+ * not implement them raises an illegal instruction exception.
+ */
+ csr_read_allowed(CSR_TDATA2, &trap);
+ hart_state->tdata2_supported = !trap.cause;
+
+ csr_read_allowed(CSR_TDATA3, &trap);
+ hart_state->tdata3_supported = !trap.cause;
+
hart_state->probed = 1;
_probed:
@@ -367,12 +378,17 @@ 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_dbtr_hart_triggers_state *hs;
unsigned long state;
unsigned long tdata1;
if (!trig || !(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)))
return;
+ hs = dbtr_thishart_state_ptr();
+ if (!hs)
+ return;
+
state = trig->state;
tdata1 = trig->tdata1;
@@ -418,7 +434,10 @@ 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);
+ if (hs->tdata2_supported)
+ csr_write(CSR_TDATA2, trig->tdata2);
+ if (hs->tdata3_supported)
+ csr_write(CSR_TDATA3, trig->tdata3);
csr_write(CSR_TDATA1, trig->tdata1);
}
@@ -458,12 +477,21 @@ static void dbtr_trigger_disable(struct sbi_dbtr_trigger *trig)
static void dbtr_trigger_clear(struct sbi_dbtr_trigger *trig)
{
+ struct sbi_dbtr_hart_triggers_state *hs;
+
if (!trig || !(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)))
return;
+ hs = dbtr_thishart_state_ptr();
+ if (!hs)
+ return;
+
csr_write(CSR_TSELECT, trig->index);
csr_write(CSR_TDATA1, 0x0);
- csr_write(CSR_TDATA2, 0x0);
+ if (hs->tdata2_supported)
+ csr_write(CSR_TDATA2, 0x0);
+ if (hs->tdata3_supported)
+ csr_write(CSR_TDATA3, 0x0);
}
static int dbtr_trigger_supported(unsigned long type)
@@ -566,8 +594,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 = hs->tdata2_supported ? csr_read(CSR_TDATA2) : 0;
+ trig->tdata3 = hs->tdata3_supported ? csr_read(CSR_TDATA3) : 0;
xmit->tstate = cpu_to_lle(trig->state);
xmit->tdata1 = cpu_to_lle(trig->tdata1);
xmit->tdata2 = cpu_to_lle(trig->tdata2);
@@ -619,6 +647,20 @@ int sbi_dbtr_install_trig(unsigned long smode,
trig_count * sizeof(*entry));
return SBI_ERR_FAILED;
}
+
+ if (recv->tdata2 && !hs->tdata2_supported) {
+ *out = _idx;
+ sbi_hart_protection_unmap_range((unsigned long)shmem_base,
+ trig_count * sizeof(*entry));
+ return SBI_ERR_NOT_SUPPORTED;
+ }
+
+ if (recv->tdata3 && !hs->tdata3_supported) {
+ *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) {
@@ -734,6 +776,16 @@ int sbi_dbtr_update_trig(unsigned long smode,
return SBI_ERR_FAILED;
}
+ if (entry->data.tdata2 && !hs->tdata2_supported) {
+ sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
+ return SBI_ERR_NOT_SUPPORTED;
+ }
+
+ if (entry->data.tdata3 && !hs->tdata3_supported) {
+ 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