OpenSBI Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: "David E. Garcia Porras" <david.garcia@aheadcomputing.com>
To: opensbi@lists.infradead.org
Cc: "David E. Garcia Porras" <david.garcia@aheadcomputing.com>
Subject: [PATCH] lib: sbi: Fix DBTR error codes and semantics as per SBI v3.0 spec
Date: Fri, 14 Aug 2026 11:23:29 -0600	[thread overview]
Message-ID: <20260814172329.52258-1-david.garcia@aheadcomputing.com> (raw)

Align the Debug Triggers (DBTR) extension implementation with the
error codes and behavior required by the SBI v3.0 specification,
chapter 19:

 - sbi_debug_set_shmem (sec 19.2): accept the flags parameter and
   return SBI_ERR_INVALID_PARAM when it is not zero. Also validate
   the entire shared memory range of trig_max * (XLEN / 2) bytes
   instead of only the base address, returning SBI_ERR_INVALID_ADDRESS
   when the range is not accessible.

 - sbi_debug_install_triggers (sec 19.4, table 101): return
   SBI_ERR_BAD_RANGE when trig_count >= trig_max, and return
   SBI_ERR_NOT_SUPPORTED instead of SBI_ERR_FAILED when the trigger
   type is not supported.

 - sbi_debug_update_triggers (sec 19.5, table 102): return the array
   index of the failing trigger configuration in sbiret.value, and
   return SBI_ERR_INVALID_PARAM when trig_tdata1.type or
   trig_tdata1.chain do not match the originally installed debug
   trigger. Validate the complete request before updating any trigger.

 - sbi_debug_uninstall_triggers, sbi_debug_enable_triggers and
   sbi_debug_disable_triggers (secs 19.6-19.8, tables 103-105): return
   SBI_ERR_INVALID_PARAM when any trigger in the set has
   trig_idx >= trig_max, and validate the complete set before acting
   on any trigger so a partially processed batch is not left behind.

 - Set the trig_state.have_hw_trig bit when a trigger is mapped to a
   HW debug trigger (table 98).

Signed-off-by: David E. Garcia Porras <david.garcia@aheadcomputing.com>
---
 include/sbi/sbi_dbtr.h   |  5 ++-
 lib/sbi/sbi_dbtr.c       | 91 +++++++++++++++++++++++++++++++++-------
 lib/sbi/sbi_ecall_dbtr.c |  4 +-
 3 files changed, 81 insertions(+), 19 deletions(-)

diff --git a/include/sbi/sbi_dbtr.h b/include/sbi/sbi_dbtr.h
index 5e0bf84e..e6b76086 100644
--- a/include/sbi/sbi_dbtr.h
+++ b/include/sbi/sbi_dbtr.h
@@ -104,7 +104,8 @@ int sbi_dbtr_init(struct sbi_scratch *scratch, bool coldboot);
 int sbi_dbtr_supported(void);
 int sbi_dbtr_setup_shmem(const struct sbi_domain *dom, unsigned long smode,
 			 unsigned long shmem_phys_lo,
-			 unsigned long shmem_phys_hi);
+			 unsigned long shmem_phys_hi,
+			 unsigned long flags);
 int sbi_dbtr_num_trig(unsigned long trig_tdata1, unsigned long *out);
 int sbi_dbtr_read_trig(unsigned long smode,
 		       unsigned long trig_idx_base, unsigned long trig_count);
@@ -115,7 +116,7 @@ int sbi_dbtr_uninstall_trig(unsigned long trig_idx_base,
 int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
 			 unsigned long trig_idx_mask);
 int sbi_dbtr_update_trig(unsigned long smode,
-			 unsigned long trig_count);
+			 unsigned long trig_count, unsigned long *out);
 int sbi_dbtr_disable_trig(unsigned long trig_idx_base,
 			  unsigned long trig_idx_mask);
 
diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 01047969..1d4785c7 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -261,8 +261,10 @@ int sbi_dbtr_get_total_triggers(void)
 
 int sbi_dbtr_setup_shmem(const struct sbi_domain *dom, unsigned long smode,
 			 unsigned long shmem_phys_lo,
-			 unsigned long shmem_phys_hi)
+			 unsigned long shmem_phys_hi,
+			 unsigned long flags)
 {
+	unsigned long shmem_size;
 	struct sbi_dbtr_hart_triggers_state *hart_state;
 
 	if (dom && !sbi_domain_is_assigned_hart(dom, current_hartindex())) {
@@ -271,6 +273,9 @@ int sbi_dbtr_setup_shmem(const struct sbi_domain *dom, unsigned long smode,
 		return SBI_ERR_DENIED;
 	}
 
+	if (flags)
+		return SBI_ERR_INVALID_PARAM;
+
 	hart_state = dbtr_thishart_state_ptr();
 	if (!hart_state)
 		return SBI_ERR_FAILED;
@@ -304,9 +309,11 @@ int sbi_dbtr_setup_shmem(const struct sbi_domain *dom, unsigned long smode,
 	if (shmem_phys_hi)
 		return SBI_EINVALID_ADDR;
 
-	if (dom && !sbi_domain_check_addr(dom,
-		  DBTR_SHMEM_MAKE_PHYS(shmem_phys_hi, shmem_phys_lo), smode,
-		  SBI_DOMAIN_READ | SBI_DOMAIN_WRITE))
+	/* Must fail if address *range* is not fully accessible - size is assumed to be trig_max * (XLEN /2) bytes */
+	shmem_size = (unsigned long) hart_state->total_trigs * (__riscv_xlen / 2);
+	if (dom && !sbi_domain_check_addr_range(dom,
+		  DBTR_SHMEM_MAKE_PHYS(shmem_phys_hi, shmem_phys_lo), shmem_size,
+		  smode, SBI_DOMAIN_READ | SBI_DOMAIN_WRITE))
 		return SBI_ERR_INVALID_ADDRESS;
 
 	hart_state->shmem.phys_lo = shmem_phys_lo;
@@ -332,6 +339,7 @@ static void dbtr_trigger_setup(struct sbi_dbtr_trigger *trig,
 	trig->state = 0;
 
 	__set_bit(RV_DBTR_BIT(TS, MAPPED), &trig->state);
+	__set_bit(RV_DBTR_BIT(TS, HAVE_TRIG), &trig->state);
 
 	SET_TRIG_HW_INDEX(trig->state, trig->index);
 
@@ -616,6 +624,9 @@ int sbi_dbtr_install_trig(unsigned long smode,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
+	if (trig_count >= hs->total_trigs)
+		return SBI_ERR_BAD_RANGE;
+
 	if (sbi_dbtr_shmem_disabled(hs))
 		return SBI_ERR_NO_SHMEM;
 
@@ -641,7 +652,7 @@ int sbi_dbtr_install_trig(unsigned long smode,
 			*out = _idx;
 			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
 							trig_count * sizeof(*entry));
-			return SBI_ERR_FAILED;
+			return SBI_ERR_NOT_SUPPORTED;
 		}
 
 		if (!dbtr_trigger_valid(TDATA1_GET_TYPE(ctrl), ctrl)) {
@@ -702,13 +713,17 @@ int sbi_dbtr_uninstall_trig(unsigned long trig_idx_base,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
-	for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
+	for_each_set_bit_from(idx, &trig_mask, RV_MAX_TRIGGERS) {
 		trig = INDEX_TO_TRIGGER(idx);
-		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)))
+		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)) || idx >= hs->total_trigs)
 			return SBI_ERR_INVALID_PARAM;
+	}
 
+	/* Only uninstall after validating the arguments - maintain atomicity */
+	idx = trig_idx_base;
+	for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
+		trig = INDEX_TO_TRIGGER(idx);
 		dbtr_trigger_clear(trig);
-
 		sbi_free_trigger(trig);
 	}
 
@@ -727,6 +742,14 @@ int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
+	for_each_set_bit_from(idx, &trig_mask, RV_MAX_TRIGGERS) {
+		trig = INDEX_TO_TRIGGER(idx);
+		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)) || idx >= hs->total_trigs)
+			return SBI_ERR_INVALID_PARAM;
+	}
+
+	/* Only enable after validating the arguments - maintain atomicity */
+	idx = trig_idx_base;
 	for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
 		trig = INDEX_TO_TRIGGER(idx);
 		sbi_dprintf("%s: enable trigger %lu\n", __func__, idx);
@@ -737,8 +760,9 @@ int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
 }
 
 int sbi_dbtr_update_trig(unsigned long smode,
-			 unsigned long trig_count)
+			 unsigned long trig_count, unsigned long *out)
 {
+	int stat = SBI_SUCCESS;
 	unsigned long trig_idx;
 	struct sbi_dbtr_trigger *trig;
 	union sbi_dbtr_shmem_entry *entry;
@@ -772,23 +796,52 @@ int sbi_dbtr_update_trig(unsigned long smode,
 		trig_idx = entry->id.idx;
 
 		if (trig_idx >= hs->total_trigs) {
-			sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
-			return SBI_ERR_INVALID_PARAM;
+			stat = SBI_ERR_INVALID_PARAM;
+			goto endloop;
 		}
 
 		trig = INDEX_TO_TRIGGER(trig_idx);
 
 		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED))) {
-			sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
-			return SBI_ERR_FAILED;
+			stat = SBI_ERR_FAILED;
+			goto endloop;
+		}
+
+		if (TDATA1_GET_TYPE(entry->data.tdata1) != TDATA1_GET_TYPE(trig->tdata1)) {
+			stat = SBI_ERR_INVALID_PARAM;
+			goto endloop;
+		}
+
+		switch(TDATA1_GET_TYPE(entry->data.tdata1)) {
+			case RISCV_DBTR_TRIG_MCONTROL:
+			case RISCV_DBTR_TRIG_MCONTROL6:
+				if ( (entry->data.tdata1 & RV_DBTR_BIT_MASK(MC6, CHAIN)) != (trig->tdata1 & RV_DBTR_BIT_MASK(MC6, CHAIN)) ) {
+					stat = SBI_ERR_INVALID_PARAM;
+					goto endloop;
+				}
+				break;
+			default:
+				break;
 		}
 
 		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;
+			stat = SBI_ERR_NOT_SUPPORTED;
+			goto endloop;
 		}
 
+		endloop:
+			sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
+			if (stat != SBI_SUCCESS) {
+				*out = _idx;
+				return stat;
+			}
+	}
+
+	/* Only update after validating request - maintain atomicity */
+	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
+		sbi_hart_protection_map_range((unsigned long)entry, sizeof(*entry));
+		trig = INDEX_TO_TRIGGER(entry->id.idx);
 		dbtr_trigger_setup(trig, &entry->data);
 		sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
 		dbtr_trigger_enable(trig);
@@ -809,6 +862,14 @@ int sbi_dbtr_disable_trig(unsigned long trig_idx_base,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
+	for_each_set_bit_from(idx, &trig_mask, RV_MAX_TRIGGERS) {
+		trig = INDEX_TO_TRIGGER(idx);
+		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)) || idx >= hs->total_trigs)
+			return SBI_ERR_INVALID_PARAM;
+	}
+
+	/* Only disable after validating the arguments - maintain atomicity */
+	idx = trig_idx_base;
 	for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
 		trig = INDEX_TO_TRIGGER(idx);
 		dbtr_trigger_disable(trig);
diff --git a/lib/sbi/sbi_ecall_dbtr.c b/lib/sbi/sbi_ecall_dbtr.c
index 40a437ee..b86ce5b3 100644
--- a/lib/sbi/sbi_ecall_dbtr.c
+++ b/lib/sbi/sbi_ecall_dbtr.c
@@ -28,7 +28,7 @@ static int sbi_ecall_dbtr_handler(unsigned long extid, unsigned long funcid,
 		break;
 	case SBI_EXT_DBTR_SETUP_SHMEM:
 		ret = sbi_dbtr_setup_shmem(sbi_domain_thishart_ptr(), smode,
-					   regs->a0, regs->a1);
+					   regs->a0, regs->a1, regs->a2);
 		break;
 	case SBI_EXT_DBTR_TRIGGER_READ:
 		ret = sbi_dbtr_read_trig(smode, regs->a0, regs->a1);
@@ -43,7 +43,7 @@ static int sbi_ecall_dbtr_handler(unsigned long extid, unsigned long funcid,
 		ret = sbi_dbtr_enable_trig(regs->a0, regs->a1);
 		break;
 	case SBI_EXT_DBTR_TRIGGER_UPDATE:
-		ret = sbi_dbtr_update_trig(smode, regs->a0);
+		ret = sbi_dbtr_update_trig(smode, regs->a0, &out->value);
 		break;
 	case SBI_EXT_DBTR_TRIGGER_DISABLE:
 		ret = sbi_dbtr_disable_trig(regs->a0, regs->a1);
-- 
2.43.0


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

                 reply	other threads:[~2026-08-14 17:23 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260814172329.52258-1-david.garcia@aheadcomputing.com \
    --to=david.garcia@aheadcomputing.com \
    --cc=opensbi@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox