[PATCH] lib: sbi: Fix DBTR error codes and semantics as per SBI v3.0 spec

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 <[email protected]>
---
 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
[email protected]
http://lists.infradead.org/mailman/listinfo/opensbi