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

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

[David E. Garcia Porras]

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@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


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

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

[Nicholas Piggin]

On Mon, May 25, 2026 at 12:50:33PM -0600, David E. Garcia Porras 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.

Hey,

I attempted to fix this to work around a crash I saw with QEMU.

https://lists.infradead.org/pipermail/opensbi/2026-March/009600.html

I didn't actually dig far enough into the spec or see the CSR write
side of the issue because it was following the recipe from the spec, and
QEMU doesn't crash in that case.

AFAIKS you're right though, tdata2/3 must always be readable if they
are implemented for any trigger. so QEMU's implementation seems contrary
to the Section 5 introduction (I have a QEMU Sdtrig fix series, I'll add
a fix to it).

Unfortunately your fix here I think won't help the existing QEMU bug
because trap depends on the selected trigger type (see
tdata_available()). Arguably we could just ignore QEMU... but using
csr_read_allowed/csr_write_allowed would solve both cases. What do you
think?

[snip]

> @@ -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;
> +		}
>  	}

These checks seem consistent with the SBI spec, but even with them we
miss classes of these errors AFAIKS. To be comprehensive we might need
to program the trigger then read it back and compare. Which might
require some complicated unwinding.

In any case I don't dislike adding the checks, but perhaps they can be
addressed separately and we could decide whether to cover other cases
too.

Thanks,
Nick

> 
>  	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);
> --

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

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

[David E. Garcia Porras]

Hi Nick,

Thanks for the review and for the QEMU context, I missed that work from your patch series.
If QEMU behaves as stated (tying trap directly to specific triggers), then yes my checks won't suffice.
I'll update my patch and submit a V2, lmk if we should maybe merge it into your patch series.

> Unfortunately your fix here I think won't help the existing QEMU bug
> because trap depends on the selected trigger type (see
> tdata_available()). Arguably we could just ignore QEMU... but using
> csr_read_allowed/csr_write_allowed would solve both cases. What do
> you think?

Agreed. v2 will drop the boot-time probe and instead wrap every
tdata2/tdata3 read and write with csr_read_allowed/csr_write_allowed,
checking trap.cause locally, this should cover all cases.

For read path, I used your exact same code.

> These checks seem consistent with the SBI spec, but even with them we
> miss classes of these errors AFAIKS. To be comprehensive we might
> need to program the trigger then read it back and compare. Which
> might require some complicated unwinding.
>
> In any case I don't dislike adding the checks, but perhaps they can
> be addressed separately and we could decide whether to cover other
> cases too.

I'd like to keep the SBI_ERR_NOT_SUPPORTED checks in v2 to satisfy
the SBI spec wording in section 19.4 / 19.5:

  "One of the trigger configuration can't be programmed due to
   unimplemented optional bits in tdata1, tdata2, or tdata3 CSRs."

I'll implement them via csr_read_allowed so there's a single
consistent code path, and I'll add an explicit comment noting that
this only catches the "whole CSR unimplemented" case -- WARL
tied-off bits within a CSR are not caught and would require the
program-then-read-back approach you described. We can address that
as a separate follow-up patch.

v2 incoming.

Thanks,
David

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

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

[David E. Garcia Porras]

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@gmail.com>
Signed-off-by: David E. Garcia Porras <david.garcia@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


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

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

[Himanshu Chauhan]

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@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@gmail.com>
> Signed-off-by: David E. Garcia Porras <david.garcia@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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

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

[David E. Garcia Porras]

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@gmail.com>
Suggested-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
Signed-off-by: David E. Garcia Porras <david.garcia@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);
-- 
2.43.0

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

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

[Himanshu Chauhan]

On Tue, Jun 16, 2026 at 10:31 PM David E. Garcia Porras
<david.garcia@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@gmail.com>
> Suggested-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
> Signed-off-by: David E. Garcia Porras <david.garcia@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@oss.qualcomm.com>

> --
> 2.43.0

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

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

[Anup Patel]

On Tue, Jun 16, 2026 at 10:31 PM David E. Garcia Porras
<david.garcia@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@gmail.com>
> Suggested-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
> Signed-off-by: David E. Garcia Porras <david.garcia@aheadcomputing.com>

Applied this patch to the riscv/opensbi repo.

Thanks,
Anup

> ---
> 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);
> --
> 2.43.0
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi