[PATCH 00/18] dbtr: Fixes and heterogeneous trigger types

[PATCH 00/18] dbtr: Fixes and heterogeneous trigger types

[Nicholas Piggin]

In the course of implementing and testing heterogeneous trigger
types as found in the Tenstorrent Ascalon CPU, I found some issues
to fix as well.

Patches 1-11 contain fixes and improvements for existing function,
and make most of the new kvm-unit-tests tests pass. 12-17 implement
"heterogenous" triggers where different HW triggers support
different functions so we have to relax the 1:1 mapping from DBTR trig
to HW trigger index and allocate HW triggers independently from what
can satisfy the requested type. Patch 18 is another possible fix but
it relies on update to SBI spec so I put it last. I can move it ahead
into the fixes block if the issue is confirmed.

Thanks,
Nick

Nicholas Piggin (18):
  dbtr: Add consistent range checks to trigger ecalls
  dbtr: Trigger update should set sbiret.value on failure
  dbtr: Fix endian conversion in trigger install handler
  dbtr: Return correct error on install not supported
  dbtr: Do not support chain bit
  dbtr: Improve trigger update error checking
  dbtr: Check for invalid and unsupported triggers in update
  dbtr: Improve error handling for trigger enable, disable, uninstall
  dbtr: Read triggers should not read HW trigger if not mapped
  dbtr: Avoid crash in sbi_debug_read_triggers
  dbtr: Succeed operations with no triggers in mask
  dbtr: Move hardware trigger probing to a function
  dbtr: Rework install and update error handling
  dbtr: Decouple dbtr trigger index from hardware trigger number
  dbtr: Move trigger feature support test into a function
  dbtr: Heterogeneous trigger type support
  dbtr: Heterogeneous access type matching for mcontrol triggers
  dbtr: Work around specification bug in range checks

 include/sbi/sbi_dbtr.h   |  17 +-
 lib/sbi/sbi_dbtr.c       | 499 ++++++++++++++++++++++++++++++---------
 lib/sbi/sbi_ecall_dbtr.c |   2 +-
 3 files changed, 407 insertions(+), 111 deletions(-)

-- 
2.51.0


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

[PATCH 01/18] dbtr: Add consistent range checks to trigger ecalls

[Nicholas Piggin]

Be more consistent with the sequence of trigger range checks in dbtr
ecall handlers. This includes adding a missing range check in install
triggers that can result in SBI past the end of the shm region.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 8bcb4312..b0160163 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -550,6 +550,9 @@ int sbi_dbtr_read_trig(unsigned long smode,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
+	if (trig_count >= hs->total_trigs)
+		return SBI_ERR_BAD_RANGE;
+
 	if (trig_idx_base >= hs->total_trigs ||
 	    trig_idx_base + trig_count >= hs->total_trigs)
 		return SBI_ERR_INVALID_PARAM;
@@ -594,6 +597,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;
 
@@ -663,6 +669,11 @@ int sbi_dbtr_uninstall_trig(unsigned long trig_idx_base,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
+	if (trig_idx_base >= hs->total_trigs ||
+		find_last_bit(&trig_idx_mask, hs->total_trigs) +
+				trig_idx_base >= hs->total_trigs)
+		return SBI_ERR_INVALID_PARAM;
+
 	for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
 		trig = INDEX_TO_TRIGGER(idx);
 		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)))
@@ -688,6 +699,11 @@ int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
+	if (trig_idx_base >= hs->total_trigs ||
+		find_last_bit(&trig_idx_mask, hs->total_trigs) +
+				trig_idx_base >= hs->total_trigs)
+		return SBI_ERR_INVALID_PARAM;
+
 	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);
@@ -710,14 +726,14 @@ int sbi_dbtr_update_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;
 
 	shmem_base = hart_shmem_base(hs);
 
-	if (trig_count >= hs->total_trigs)
-		return SBI_ERR_BAD_RANGE;
-
 	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;
@@ -754,6 +770,11 @@ int sbi_dbtr_disable_trig(unsigned long trig_idx_base,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
+	if (trig_idx_base >= hs->total_trigs ||
+		find_last_bit(&trig_idx_mask, hs->total_trigs) +
+				trig_idx_base >= hs->total_trigs)
+		return SBI_ERR_INVALID_PARAM;
+
 	for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
 		trig = INDEX_TO_TRIGGER(idx);
 		dbtr_trigger_disable(trig);
-- 
2.51.0


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

[PATCH 02/18] dbtr: Trigger update should set sbiret.value on failure

[Nicholas Piggin]

According to RISC-V SBI specification, trigger updates should return the
value of the problem trigger index on failure.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/sbi/sbi_dbtr.h   | 2 +-
 lib/sbi/sbi_dbtr.c       | 4 +++-
 lib/sbi/sbi_ecall_dbtr.c | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/sbi/sbi_dbtr.h b/include/sbi/sbi_dbtr.h
index 5e0bf84e..b8b5c9ab 100644
--- a/include/sbi/sbi_dbtr.h
+++ b/include/sbi/sbi_dbtr.h
@@ -115,7 +115,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 b0160163..8f59e407 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -714,7 +714,7 @@ 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)
 {
 	unsigned long trig_idx;
 	struct sbi_dbtr_trigger *trig;
@@ -740,6 +740,7 @@ int sbi_dbtr_update_trig(unsigned long smode,
 
 		if (trig_idx >= hs->total_trigs) {
 			sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
+			*out = _idx;
 			return SBI_ERR_INVALID_PARAM;
 		}
 
@@ -747,6 +748,7 @@ int sbi_dbtr_update_trig(unsigned long smode,
 
 		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED))) {
 			sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
+			*out = _idx;
 			return SBI_ERR_FAILED;
 		}
 
diff --git a/lib/sbi/sbi_ecall_dbtr.c b/lib/sbi/sbi_ecall_dbtr.c
index 40a437ee..f3196feb 100644
--- a/lib/sbi/sbi_ecall_dbtr.c
+++ b/lib/sbi/sbi_ecall_dbtr.c
@@ -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.51.0


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

[PATCH 03/18] dbtr: Fix endian conversion in trigger install handler

[Nicholas Piggin]

An endian conversion was missed loading tdata1 from shm. Add the
conversion and change the variable name from ctrl to tdata1 while here.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 8f59e407..10ed6b3e 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -589,7 +589,6 @@ int sbi_dbtr_install_trig(unsigned long smode,
 	union sbi_dbtr_shmem_entry *entry;
 	struct sbi_dbtr_data_msg *recv;
 	struct sbi_dbtr_id_msg *xmit;
-	unsigned long ctrl;
 	struct sbi_dbtr_trigger *trig;
 	struct sbi_dbtr_hart_triggers_state *hs = NULL;
 
@@ -609,17 +608,19 @@ int sbi_dbtr_install_trig(unsigned long smode,
 
 	/* Check requested triggers configuration */
 	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
+		unsigned long tdata1;
+
 		recv = (struct sbi_dbtr_data_msg *)(&entry->data);
-		ctrl = recv->tdata1;
+		tdata1 = lle_to_cpu(recv->tdata1);
 
-		if (!dbtr_trigger_supported(TDATA1_GET_TYPE(ctrl))) {
+		if (!dbtr_trigger_supported(TDATA1_GET_TYPE(tdata1))) {
 			*out = _idx;
 			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
 							trig_count * sizeof(*entry));
 			return SBI_ERR_FAILED;
 		}
 
-		if (!dbtr_trigger_valid(TDATA1_GET_TYPE(ctrl), ctrl)) {
+		if (!dbtr_trigger_valid(TDATA1_GET_TYPE(tdata1), tdata1)) {
 			*out = _idx;
 			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
 							trig_count * sizeof(*entry));
-- 
2.51.0


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

[PATCH 04/18] dbtr: Return correct error on install not supported

[Nicholas Piggin]

Return SBI_ERR_NOT_SUPPORTED if a trigger is valid but not
supported.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 10ed6b3e..65c3b2df 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -617,7 +617,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(tdata1), tdata1)) {
-- 
2.51.0


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

[PATCH 05/18] dbtr: Do not support chain bit

[Nicholas Piggin]

There is no chain bit validation in in SBI, so do not support it.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 65c3b2df..224f2350 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -466,11 +466,21 @@ static void dbtr_trigger_clear(struct sbi_dbtr_trigger *trig)
 	csr_write(CSR_TDATA2, 0x0);
 }
 
-static int dbtr_trigger_supported(unsigned long type)
+static int dbtr_trigger_supported(unsigned long type, unsigned long tdata)
 {
 	switch (type) {
 	case RISCV_DBTR_TRIG_MCONTROL:
+		/*
+		 * SBI currently does not validate chain bit in install/update
+		 * so we do not support it.
+		 */
+		if (tdata & RV_DBTR_BIT_MASK(MC, CHAIN))
+			return 0;
+		return 1;
 	case RISCV_DBTR_TRIG_MCONTROL6:
+		if (tdata & RV_DBTR_BIT_MASK(MC6, CHAIN))
+			return 0;
+		return 1;
 	case RISCV_DBTR_TRIG_ICOUNT:
 		return 1;
 	default:
@@ -613,7 +623,7 @@ int sbi_dbtr_install_trig(unsigned long smode,
 		recv = (struct sbi_dbtr_data_msg *)(&entry->data);
 		tdata1 = lle_to_cpu(recv->tdata1);
 
-		if (!dbtr_trigger_supported(TDATA1_GET_TYPE(tdata1))) {
+		if (!dbtr_trigger_supported(TDATA1_GET_TYPE(tdata1), tdata1)) {
 			*out = _idx;
 			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
 							trig_count * sizeof(*entry));
-- 
2.51.0


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

[PATCH 06/18] dbtr: Improve trigger update error checking

[Nicholas Piggin]

Trigger updates should ensure all triggers can be upated without failure
before making any changes. Updates that change the trigger type must
also be disallowed according to SBI specification.

Change the style of shmem access and checking to match the trigger
install code and perform all checks first. Add the missing check to
prevent type change.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 224f2350..d2845fec 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -727,9 +727,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 *out)
 {
-	unsigned long trig_idx;
 	struct sbi_dbtr_trigger *trig;
 	union sbi_dbtr_shmem_entry *entry;
+	struct sbi_dbtr_data_msg *recv;
 	void *shmem_base = NULL;
 	struct sbi_dbtr_hart_triggers_state *hs = NULL;
 
@@ -744,30 +744,49 @@ int sbi_dbtr_update_trig(unsigned long smode,
 		return SBI_ERR_NO_SHMEM;
 
 	shmem_base = hart_shmem_base(hs);
+	sbi_hart_protection_map_range((unsigned long)shmem_base,
+				      trig_count * sizeof(*entry));
 
+	/* Check requested triggers configuration */
 	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;
+		unsigned long trig_idx, tdata1;
 
+		trig_idx = entry->id.idx;
 		if (trig_idx >= hs->total_trigs) {
-			sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
 			*out = _idx;
+			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
+							trig_count * sizeof(*entry));
 			return SBI_ERR_INVALID_PARAM;
 		}
 
 		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));
 			*out = _idx;
+			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
+							trig_count * sizeof(*entry));
 			return SBI_ERR_FAILED;
 		}
 
+		recv = (struct sbi_dbtr_data_msg *)(&entry->data);
+		tdata1 = lle_to_cpu(recv->tdata1);
+		if (TDATA1_GET_TYPE(tdata1) != TDATA1_GET_TYPE(trig->tdata1)) {
+			*out = _idx;
+			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
+							trig_count * sizeof(*entry));
+			return SBI_ERR_INVALID_PARAM;
+		}
+	}
+
+	/* Update triggers */
+	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), 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);
 	}
 
+	sbi_hart_protection_unmap_range((unsigned long)shmem_base,
+					trig_count * sizeof(*entry));
+
 	return SBI_SUCCESS;
 }
 
-- 
2.51.0


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

[PATCH 07/18] dbtr: Check for invalid and unsupported triggers in update

[Nicholas Piggin]

Update triggers does not check supported or valid, so it permits tdata1.m
and tdata1.dmode bits being set.

Fix update to test that triggers are valid and supported.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index d2845fec..3925170f 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -749,7 +749,7 @@ int sbi_dbtr_update_trig(unsigned long smode,
 
 	/* Check requested triggers configuration */
 	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
-		unsigned long trig_idx, tdata1;
+		unsigned long trig_idx, tdata1, type;
 
 		trig_idx = entry->id.idx;
 		if (trig_idx >= hs->total_trigs) {
@@ -769,7 +769,23 @@ int sbi_dbtr_update_trig(unsigned long smode,
 
 		recv = (struct sbi_dbtr_data_msg *)(&entry->data);
 		tdata1 = lle_to_cpu(recv->tdata1);
-		if (TDATA1_GET_TYPE(tdata1) != TDATA1_GET_TYPE(trig->tdata1)) {
+		type = TDATA1_GET_TYPE(tdata1);
+
+		if (!dbtr_trigger_supported(type, tdata1)) {
+			*out = _idx;
+			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
+							trig_count * sizeof(*entry));
+			return SBI_ERR_NOT_SUPPORTED;
+		}
+
+		if (!dbtr_trigger_valid(type, tdata1)) {
+			*out = _idx;
+			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
+							trig_count * sizeof(*entry));
+			return SBI_ERR_INVALID_PARAM;
+		}
+
+		if (type != TDATA1_GET_TYPE(trig->tdata1)) {
 			*out = _idx;
 			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
 							trig_count * sizeof(*entry));
-- 
2.51.0


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

[PATCH 08/18] dbtr: Improve error handling for trigger enable, disable, uninstall

[Nicholas Piggin]

When updating multiple triggers, error checking should be performed on
all triggers before changing any. Check all triggers for mapping to HW
in these handlers.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 3925170f..a2c45061 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -672,7 +672,7 @@ int sbi_dbtr_uninstall_trig(unsigned long trig_idx_base,
 			    unsigned long trig_idx_mask)
 {
 	unsigned long trig_mask = trig_idx_mask << trig_idx_base;
-	unsigned long idx = trig_idx_base;
+	unsigned long idx;
 	struct sbi_dbtr_trigger *trig;
 	struct sbi_dbtr_hart_triggers_state *hs;
 
@@ -685,13 +685,17 @@ int sbi_dbtr_uninstall_trig(unsigned long trig_idx_base,
 				trig_idx_base >= hs->total_trigs)
 		return SBI_ERR_INVALID_PARAM;
 
+	idx = trig_idx_base;
 	for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
 		trig = INDEX_TO_TRIGGER(idx);
 		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)))
 			return SBI_ERR_INVALID_PARAM;
+	}
 
+	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);
 	}
 
@@ -702,7 +706,7 @@ int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
 			 unsigned long trig_idx_mask)
 {
 	unsigned long trig_mask = trig_idx_mask << trig_idx_base;
-	unsigned long idx = trig_idx_base;
+	unsigned long idx;
 	struct sbi_dbtr_trigger *trig;
 	struct sbi_dbtr_hart_triggers_state *hs;
 
@@ -715,6 +719,14 @@ int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
 				trig_idx_base >= hs->total_trigs)
 		return SBI_ERR_INVALID_PARAM;
 
+	idx = trig_idx_base;
+	for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
+		trig = INDEX_TO_TRIGGER(idx);
+		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)))
+			return SBI_ERR_INVALID_PARAM;
+	}
+
+	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);
@@ -810,7 +822,7 @@ int sbi_dbtr_disable_trig(unsigned long trig_idx_base,
 			  unsigned long trig_idx_mask)
 {
 	unsigned long trig_mask = trig_idx_mask << trig_idx_base;
-	unsigned long idx = trig_idx_base;
+	unsigned long idx;
 	struct sbi_dbtr_trigger *trig;
 	struct sbi_dbtr_hart_triggers_state *hs;
 
@@ -823,6 +835,14 @@ int sbi_dbtr_disable_trig(unsigned long trig_idx_base,
 				trig_idx_base >= hs->total_trigs)
 		return SBI_ERR_INVALID_PARAM;
 
+	idx = trig_idx_base;
+	for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
+		trig = INDEX_TO_TRIGGER(idx);
+		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)))
+			return SBI_ERR_INVALID_PARAM;
+	}
+
+	idx = trig_idx_base;
 	for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
 		trig = INDEX_TO_TRIGGER(idx);
 		dbtr_trigger_disable(trig);
-- 
2.51.0


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

[PATCH 09/18] dbtr: Read triggers should not read HW trigger if not mapped

[Nicholas Piggin]

When no hardware trigger is mapped to a dbtr trig idx there is logically
no hardware trigger to be read. The mapped status can be tested in the
tstate word, so callers understand this.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index a2c45061..6efd5435 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -577,14 +577,26 @@ int sbi_dbtr_read_trig(unsigned long smode,
 	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
 		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);
+
 		xmit->tstate = cpu_to_lle(trig->state);
-		xmit->tdata1 = cpu_to_lle(trig->tdata1);
-		xmit->tdata2 = cpu_to_lle(trig->tdata2);
-		xmit->tdata3 = cpu_to_lle(trig->tdata3);
+		if (trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)) {
+			csr_write(CSR_TSELECT, trig->index);
+			trig->tdata1 = csr_read(CSR_TDATA1);
+			trig->tdata2 = csr_read(CSR_TDATA2);
+			trig->tdata3 = csr_read(CSR_TDATA3);
+			xmit->tdata1 = cpu_to_lle(trig->tdata1);
+			xmit->tdata2 = cpu_to_lle(trig->tdata2);
+			xmit->tdata3 = cpu_to_lle(trig->tdata3);
+		} else {
+			/*
+			 * If the trig idx does not have a HW trigger
+			 * assigned then just return zeroes in tdata
+			 * words.
+			 */
+			xmit->tdata1 = 0;
+			xmit->tdata2 = 0;
+			xmit->tdata3 = 0;
+		}
 	}
 	sbi_hart_protection_unmap_range((unsigned long)shmem_base,
 					trig_count * sizeof(*entry));
-- 
2.51.0


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

[PATCH 10/18] dbtr: Avoid crash in sbi_debug_read_triggers

[Nicholas Piggin]

Not all trigger types make tdata2 or tdata3 csrs available to read.
This can result in crash in the SBI_EXT_DBTR_TRIGGER_READ ecall.

Fix this by using csr_read_allowed() to read those csrs. In the case of
a trap nothing needs to be done since the caller will know those values
are unavailable, based on the trigger type returned in tdata1.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 6efd5435..f72c76f2 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -575,6 +575,8 @@ 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));
 
@@ -582,8 +584,8 @@ int sbi_dbtr_read_trig(unsigned long smode,
 		if (trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)) {
 			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->tdata1 = cpu_to_lle(trig->tdata1);
 			xmit->tdata2 = cpu_to_lle(trig->tdata2);
 			xmit->tdata3 = cpu_to_lle(trig->tdata3);
-- 
2.51.0


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

[PATCH 11/18] dbtr: Succeed operations with no triggers in mask

[Nicholas Piggin]

Trigger operations that use bitmasks should succeed if there are no
triggers in the mask.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index f72c76f2..5735bdc6 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -694,6 +694,9 @@ int sbi_dbtr_uninstall_trig(unsigned long trig_idx_base,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
+	if (!trig_idx_mask)
+		return SBI_SUCCESS;
+
 	if (trig_idx_base >= hs->total_trigs ||
 		find_last_bit(&trig_idx_mask, hs->total_trigs) +
 				trig_idx_base >= hs->total_trigs)
@@ -728,6 +731,9 @@ int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
+	if (!trig_idx_mask)
+		return SBI_SUCCESS;
+
 	if (trig_idx_base >= hs->total_trigs ||
 		find_last_bit(&trig_idx_mask, hs->total_trigs) +
 				trig_idx_base >= hs->total_trigs)
@@ -844,6 +850,9 @@ int sbi_dbtr_disable_trig(unsigned long trig_idx_base,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
+	if (!trig_idx_mask)
+		return SBI_SUCCESS;
+
 	if (trig_idx_base >= hs->total_trigs ||
 		find_last_bit(&trig_idx_mask, hs->total_trigs) +
 				trig_idx_base >= hs->total_trigs)
-- 
2.51.0


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

[PATCH 12/18] dbtr: Move hardware trigger probing to a function

[Nicholas Piggin]

This makes the code a bit neater. HW trigger probing will be expanded
with subsequent changes.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 89 +++++++++++++++++++++++++---------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 5735bdc6..cfcd728f 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -134,11 +134,57 @@ static inline void sbi_free_trigger(struct sbi_dbtr_trigger *trig)
 	hart_state->available_trigs++;
 }
 
-int sbi_dbtr_init(struct sbi_scratch *scratch, bool coldboot)
+static bool sbi_hw_trigger_probe(int i)
 {
 	struct sbi_trap_info trap = {0};
 	unsigned long tdata1;
 	unsigned long val;
+
+	csr_write_allowed(CSR_TSELECT, &trap, i);
+	if (trap.cause)
+		return false;
+
+	val = csr_read_allowed(CSR_TSELECT, &trap);
+	if (trap.cause)
+		return false;
+
+	/*
+	 * Read back tselect and check that it contains the
+	 * written value
+	 */
+	if (val != i)
+		return false;
+
+	val = csr_read_allowed(CSR_TINFO, &trap);
+	if (trap.cause) {
+		unsigned long type;
+
+		/*
+		 * If reading tinfo caused an exception, the
+		 * debugger must read tdata1 to discover the
+		 * type.
+		 */
+		tdata1 = csr_read_allowed(CSR_TDATA1, &trap);
+		if (trap.cause)
+			return false;
+
+		type = TDATA1_GET_TYPE(tdata1);
+		if (type == 0)
+			return false;
+
+		sbi_trigger_init(INDEX_TO_TRIGGER(i), BIT(type), i);
+	} else {
+		if (val == 1)
+			return false;
+
+		sbi_trigger_init(INDEX_TO_TRIGGER(i), val, i);
+	}
+
+	return true;
+}
+
+int sbi_dbtr_init(struct sbi_scratch *scratch, bool coldboot)
+{
 	int i;
 	struct sbi_dbtr_hart_triggers_state *hart_state = NULL;
 
@@ -168,47 +214,8 @@ int sbi_dbtr_init(struct sbi_scratch *scratch, bool coldboot)
 		goto _probed;
 
 	for (i = 0; i < RV_MAX_TRIGGERS; i++) {
-		csr_write_allowed(CSR_TSELECT, &trap, i);
-		if (trap.cause)
-			break;
-
-		val = csr_read_allowed(CSR_TSELECT, &trap);
-		if (trap.cause)
-			break;
-
-		/*
-		 * Read back tselect and check that it contains the
-		 * written value
-		 */
-		if (val != i)
-			break;
-
-		val = csr_read_allowed(CSR_TINFO, &trap);
-		if (trap.cause) {
-			/*
-			 * If reading tinfo caused an exception, the
-			 * debugger must read tdata1 to discover the
-			 * type.
-			 */
-			tdata1 = csr_read_allowed(CSR_TDATA1,
-						  &trap);
-			if (trap.cause)
-				break;
-
-			if (TDATA1_GET_TYPE(tdata1) == 0)
-				break;
-
-			sbi_trigger_init(INDEX_TO_TRIGGER(i),
-					 BIT(TDATA1_GET_TYPE(tdata1)),
-					 i);
+		if (sbi_hw_trigger_probe(i))
 			hart_state->total_trigs++;
-		} else {
-			if (val == 1)
-				break;
-
-			sbi_trigger_init(INDEX_TO_TRIGGER(i), val, i);
-			hart_state->total_trigs++;
-		}
 	}
 
 	hart_state->probed = 1;
-- 
2.51.0


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

[PATCH 13/18] dbtr: Rework install and update error handling

[Nicholas Piggin]

Move the unmap error path to an error handling label rather
than duplicate it several times.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 54 +++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 24 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index cfcd728f..3aea486e 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -622,6 +622,7 @@ int sbi_dbtr_install_trig(unsigned long smode,
 	struct sbi_dbtr_id_msg *xmit;
 	struct sbi_dbtr_trigger *trig;
 	struct sbi_dbtr_hart_triggers_state *hs = NULL;
+	int err;
 
 	hs = dbtr_thishart_state_ptr();
 	if (!hs)
@@ -646,24 +647,21 @@ int sbi_dbtr_install_trig(unsigned long smode,
 
 		if (!dbtr_trigger_supported(TDATA1_GET_TYPE(tdata1), tdata1)) {
 			*out = _idx;
-			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-							trig_count * sizeof(*entry));
-			return SBI_ERR_NOT_SUPPORTED;
+			err = SBI_ERR_NOT_SUPPORTED;
+			goto out_unmap;
 		}
 
 		if (!dbtr_trigger_valid(TDATA1_GET_TYPE(tdata1), tdata1)) {
 			*out = _idx;
-			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-							trig_count * sizeof(*entry));
-			return SBI_ERR_FAILED;
+			err = SBI_ERR_FAILED;
+			goto out_unmap;
 		}
 	}
 
 	if (hs->available_trigs < trig_count) {
 		*out = hs->available_trigs;
-		sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-					       trig_count * sizeof(*entry));
-		return SBI_ERR_FAILED;
+		err = SBI_ERR_FAILED;
+		goto out_unmap;
 	}
 
 	/* Install triggers */
@@ -687,6 +685,12 @@ int sbi_dbtr_install_trig(unsigned long smode,
 					trig_count * sizeof(*entry));
 
 	return SBI_SUCCESS;
+
+out_unmap:
+	sbi_hart_protection_unmap_range((unsigned long)shmem_base,
+					trig_count * sizeof(*entry));
+
+	return err;
 }
 
 int sbi_dbtr_uninstall_trig(unsigned long trig_idx_base,
@@ -771,6 +775,7 @@ int sbi_dbtr_update_trig(unsigned long smode,
 	struct sbi_dbtr_data_msg *recv;
 	void *shmem_base = NULL;
 	struct sbi_dbtr_hart_triggers_state *hs = NULL;
+	int err;
 
 	hs = dbtr_thishart_state_ptr();
 	if (!hs)
@@ -793,17 +798,15 @@ int sbi_dbtr_update_trig(unsigned long smode,
 		trig_idx = entry->id.idx;
 		if (trig_idx >= hs->total_trigs) {
 			*out = _idx;
-			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-							trig_count * sizeof(*entry));
-			return SBI_ERR_INVALID_PARAM;
+			err = SBI_ERR_INVALID_PARAM;
+			goto out_unmap;
 		}
 
 		trig = INDEX_TO_TRIGGER(trig_idx);
 		if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED))) {
 			*out = _idx;
-			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-							trig_count * sizeof(*entry));
-			return SBI_ERR_FAILED;
+			err = SBI_ERR_FAILED;
+			goto out_unmap;
 		}
 
 		recv = (struct sbi_dbtr_data_msg *)(&entry->data);
@@ -812,23 +815,20 @@ int sbi_dbtr_update_trig(unsigned long smode,
 
 		if (!dbtr_trigger_supported(type, tdata1)) {
 			*out = _idx;
-			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-							trig_count * sizeof(*entry));
-			return SBI_ERR_NOT_SUPPORTED;
+			err = SBI_ERR_NOT_SUPPORTED;
+			goto out_unmap;
 		}
 
 		if (!dbtr_trigger_valid(type, tdata1)) {
 			*out = _idx;
-			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-							trig_count * sizeof(*entry));
-			return SBI_ERR_INVALID_PARAM;
+			err = SBI_ERR_INVALID_PARAM;
+			goto out_unmap;
 		}
 
 		if (type != TDATA1_GET_TYPE(trig->tdata1)) {
 			*out = _idx;
-			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-							trig_count * sizeof(*entry));
-			return SBI_ERR_INVALID_PARAM;
+			err = SBI_ERR_INVALID_PARAM;
+			goto out_unmap;
 		}
 	}
 
@@ -843,6 +843,12 @@ int sbi_dbtr_update_trig(unsigned long smode,
 					trig_count * sizeof(*entry));
 
 	return SBI_SUCCESS;
+
+out_unmap:
+	sbi_hart_protection_unmap_range((unsigned long)shmem_base,
+					trig_count * sizeof(*entry));
+
+	return err;
 }
 
 int sbi_dbtr_disable_trig(unsigned long trig_idx_base,
-- 
2.51.0


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

[PATCH 14/18] dbtr: Decouple dbtr trigger index from hardware trigger number

[Nicholas Piggin]

Some Sdtrig implementations have heterogeneous triggers, which require
the dbtr trigger index be decoupled from the hardware trigger allocated
to it.

The SBI interface supports this, prepare the implementation by creating
a new sbi_hw_trigger data structure that is mapped by the
sbi_dbtr_trigger with its HW_INDEX field.

The dbtr and hw triggers are still fixed to the same number.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/sbi/sbi_dbtr.h | 14 ++++++-
 lib/sbi/sbi_dbtr.c     | 87 ++++++++++++++++++++++++++++++------------
 2 files changed, 76 insertions(+), 25 deletions(-)

diff --git a/include/sbi/sbi_dbtr.h b/include/sbi/sbi_dbtr.h
index b8b5c9ab..c0b9369e 100644
--- a/include/sbi/sbi_dbtr.h
+++ b/include/sbi/sbi_dbtr.h
@@ -48,9 +48,14 @@ struct sbi_dbtr_shmem {
 	unsigned long phys_hi;
 };
 
+struct sbi_hw_trigger {
+	unsigned long index;
+	bool inuse; /* Allocated for a DBTR trigger */
+	unsigned long type_mask; /* Types supported by this trigger */
+};
+
 struct sbi_dbtr_trigger {
 	unsigned long index;
-	unsigned long type_mask;
 	unsigned long state;
 	unsigned long tdata1;
 	unsigned long tdata2;
@@ -69,6 +74,7 @@ struct sbi_dbtr_id_msg {
 };
 
 struct sbi_dbtr_hart_triggers_state {
+	struct sbi_hw_trigger hw_triggers[RV_MAX_TRIGGERS];
 	struct sbi_dbtr_trigger triggers[RV_MAX_TRIGGERS];
 	struct sbi_dbtr_shmem shmem;
 	u32 total_trigs;
@@ -89,6 +95,12 @@ struct sbi_dbtr_hart_triggers_state {
 			   & RV_DBTR_BIT_MASK(TS, HW_IDX));	\
 	}while (0);
 
+#define GET_TRIG_HW_INDEX(_state)				\
+	({							\
+		(_state & RV_DBTR_BIT_MASK(TS, HW_IDX))		\
+			    >> RV_DBTR_BIT(TS, HW_IDX);		\
+	})
+
 /** SBI shared mem messages layout */
 union sbi_dbtr_shmem_entry {
 	struct sbi_dbtr_data_msg data;
diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 3aea486e..74b114b8 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -11,6 +11,7 @@
 #include <sbi/sbi_csr_detect.h>
 #include <sbi/sbi_platform.h>
 #include <sbi/sbi_byteorder.h>
+#include <sbi/sbi_string.h>
 #include <sbi/sbi_console.h>
 #include <sbi/sbi_domain.h>
 #include <sbi/sbi_trap.h>
@@ -43,6 +44,15 @@ static unsigned long hart_state_ptr_offset;
 		(__trg);						\
 	})
 
+#define INDEX_TO_HW_TRIGGER(_index)					\
+	({								\
+		struct sbi_hw_trigger *__trg = NULL;			\
+		struct sbi_dbtr_hart_triggers_state *__hs = NULL;	\
+		__hs = dbtr_get_hart_state_ptr(sbi_scratch_thishart_ptr()); \
+		__trg = &__hs->hw_triggers[_index];			\
+		(__trg);						\
+	})
+
 #define for_each_trig_entry(_base, _max, _etype, _entry)		\
 	for (int _idx = 0; _entry = ((_etype *)_base + _idx),		\
 	     _idx < _max;						\
@@ -75,17 +85,6 @@ static inline void *hart_shmem_base(
 			hs->shmem.phys_hi, hs->shmem.phys_lo));
 }
 
-static void sbi_trigger_init(struct sbi_dbtr_trigger *trig,
-			     unsigned long type_mask, unsigned long idx)
-{
-	trig->type_mask = type_mask;
-	trig->state = 0;
-	trig->tdata1 = 0;
-	trig->tdata2 = 0;
-	trig->tdata3 = 0;
-	trig->index = idx;
-}
-
 static inline struct sbi_dbtr_trigger *sbi_alloc_trigger(void)
 {
 	int i;
@@ -126,6 +125,16 @@ static inline void sbi_free_trigger(struct sbi_dbtr_trigger *trig)
 	if (!hart_state)
 		return;
 
+	if (trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)) {
+		unsigned long hw_index = GET_TRIG_HW_INDEX(trig->state);
+		struct sbi_hw_trigger *hw_trig = INDEX_TO_HW_TRIGGER(hw_index);
+		if (!hw_trig->inuse)
+			sbi_printf("DBTR error: Hardware trigger was not marked inuse\n");
+		if (hw_index != hw_trig->index)
+			sbi_printf("DBTR error: HW index mismatch\n");
+		hw_trig->inuse = false;
+	}
+
 	trig->state = 0;
 	trig->tdata1 = 0;
 	trig->tdata2 = 0;
@@ -136,10 +145,15 @@ static inline void sbi_free_trigger(struct sbi_dbtr_trigger *trig)
 
 static bool sbi_hw_trigger_probe(int i)
 {
+	struct sbi_hw_trigger *hw_trig = INDEX_TO_HW_TRIGGER(i);
 	struct sbi_trap_info trap = {0};
 	unsigned long tdata1;
 	unsigned long val;
 
+	sbi_memset(hw_trig, 0, sizeof(*hw_trig));
+
+	hw_trig->index = i;
+
 	csr_write_allowed(CSR_TSELECT, &trap, i);
 	if (trap.cause)
 		return false;
@@ -172,12 +186,12 @@ static bool sbi_hw_trigger_probe(int i)
 		if (type == 0)
 			return false;
 
-		sbi_trigger_init(INDEX_TO_TRIGGER(i), BIT(type), i);
+		hw_trig->type_mask = BIT(type);
 	} else {
 		if (val == 1)
 			return false;
 
-		sbi_trigger_init(INDEX_TO_TRIGGER(i), val, i);
+		hw_trig->type_mask = val;
 	}
 
 	return true;
@@ -213,6 +227,17 @@ int sbi_dbtr_init(struct sbi_scratch *scratch, bool coldboot)
 	if (hart_state->probed)
 		goto _probed;
 
+	/*
+	 * dbtr triggers are what is exposed to the supervisor. hw triggers
+	 * are what is selected by tselect. For now these are tied to the
+	 * same index.
+	 */
+	for (i = 0; i < RV_MAX_TRIGGERS; i++) {
+		struct sbi_dbtr_trigger *trig = INDEX_TO_TRIGGER(i);
+		sbi_memset(trig, 0, sizeof(*trig));
+		trig->index = i;
+	}
+
 	for (i = 0; i < RV_MAX_TRIGGERS; i++) {
 		if (sbi_hw_trigger_probe(i))
 			hart_state->total_trigs++;
@@ -304,6 +329,7 @@ int sbi_dbtr_setup_shmem(const struct sbi_domain *dom, unsigned long smode,
 }
 
 static void dbtr_trigger_setup(struct sbi_dbtr_trigger *trig,
+			       struct sbi_hw_trigger *hw_trig,
 			       struct sbi_dbtr_data_msg *recv)
 {
 	unsigned long tdata1;
@@ -321,7 +347,9 @@ static void dbtr_trigger_setup(struct sbi_dbtr_trigger *trig,
 
 	__set_bit(RV_DBTR_BIT(TS, MAPPED), &trig->state);
 
-	SET_TRIG_HW_INDEX(trig->state, trig->index);
+	SET_TRIG_HW_INDEX(trig->state, hw_trig->index);
+
+	hw_trig->inuse = true;
 
 	switch (TDATA1_GET_TYPE(tdata1)) {
 	case RISCV_DBTR_TRIG_MCONTROL:
@@ -423,7 +451,7 @@ static void dbtr_trigger_enable(struct sbi_dbtr_trigger *trig)
 	 * where tdata2 and tdata3 can be written with any value that makes
 	 * sense for any trigger type supported by this trigger.
 	 */
-	csr_write(CSR_TSELECT, trig->index);
+	csr_write(CSR_TSELECT, GET_TRIG_HW_INDEX(state));
 	csr_write(CSR_TDATA1, 0x0);
 	csr_write(CSR_TDATA2, trig->tdata2);
 	csr_write(CSR_TDATA1, trig->tdata1);
@@ -459,7 +487,7 @@ static void dbtr_trigger_disable(struct sbi_dbtr_trigger *trig)
 		break;
 	}
 
-	csr_write(CSR_TSELECT, trig->index);
+	csr_write(CSR_TSELECT, GET_TRIG_HW_INDEX(trig->state));
 	csr_write(CSR_TDATA1, trig->tdata1);
 }
 
@@ -468,7 +496,7 @@ static void dbtr_trigger_clear(struct sbi_dbtr_trigger *trig)
 	if (!trig || !(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)))
 		return;
 
-	csr_write(CSR_TSELECT, trig->index);
+	csr_write(CSR_TSELECT, GET_TRIG_HW_INDEX(trig->state));
 	csr_write(CSR_TDATA1, 0x0);
 	csr_write(CSR_TDATA2, 0x0);
 }
@@ -527,7 +555,7 @@ int sbi_dbtr_num_trig(unsigned long data, unsigned long *out)
 	unsigned long type = TDATA1_GET_TYPE(data);
 	u32 hartid = current_hartid();
 	unsigned long total = 0;
-	struct sbi_dbtr_trigger *trig;
+	struct sbi_hw_trigger *hw_trig;
 	int i;
 	struct sbi_dbtr_hart_triggers_state *hs;
 
@@ -541,9 +569,9 @@ int sbi_dbtr_num_trig(unsigned long data, unsigned long *out)
 	}
 
 	for (i = 0; i < hs->total_trigs; i++) {
-		trig = INDEX_TO_TRIGGER(i);
+		hw_trig = INDEX_TO_HW_TRIGGER(i);
 
-		if (__test_bit(type, &trig->type_mask))
+		if (__test_bit(type, &hw_trig->type_mask))
 			total++;
 	}
 
@@ -589,7 +617,7 @@ int sbi_dbtr_read_trig(unsigned long smode,
 
 		xmit->tstate = cpu_to_lle(trig->state);
 		if (trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)) {
-			csr_write(CSR_TSELECT, trig->index);
+			csr_write(CSR_TSELECT, GET_TRIG_HW_INDEX(trig->state));
 			trig->tdata1 = csr_read(CSR_TDATA1);
 			trig->tdata2 = csr_read_allowed(CSR_TDATA2, &trap);
 			trig->tdata3 = csr_read_allowed(CSR_TDATA3, &trap);
@@ -666,6 +694,7 @@ int sbi_dbtr_install_trig(unsigned long smode,
 
 	/* Install triggers */
 	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
+		struct sbi_hw_trigger *hw_trig;
 		/*
 		 * Since we have already checked if enough triggers are
 		 * available, trigger allocation must succeed.
@@ -675,10 +704,18 @@ int sbi_dbtr_install_trig(unsigned long smode,
 		recv = (struct sbi_dbtr_data_msg *)(&entry->data);
 		xmit = (struct sbi_dbtr_id_msg *)(&entry->id);
 
-		dbtr_trigger_setup(trig,  recv);
+		hw_trig = INDEX_TO_HW_TRIGGER(trig->index); /* 1:1 trig / HW idx */
+		if (hw_trig->inuse) {
+			*out = hs->available_trigs;
+			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
+					trig_count * sizeof(*entry));
+			sbi_printf("DBTR error: Hardware trigger already inuse\n");
+			return SBI_ERR_FAILED;
+		}
+
+		dbtr_trigger_setup(trig, hw_trig, recv);
 		dbtr_trigger_enable(trig);
 		xmit->idx = cpu_to_lle(trig->index);
-
 	}
 
 	sbi_hart_protection_unmap_range((unsigned long)shmem_base,
@@ -771,6 +808,7 @@ int sbi_dbtr_update_trig(unsigned long smode,
 			 unsigned long trig_count, unsigned long *out)
 {
 	struct sbi_dbtr_trigger *trig;
+	struct sbi_hw_trigger *hw_trig;
 	union sbi_dbtr_shmem_entry *entry;
 	struct sbi_dbtr_data_msg *recv;
 	void *shmem_base = NULL;
@@ -835,7 +873,8 @@ int sbi_dbtr_update_trig(unsigned long smode,
 	/* Update triggers */
 	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
 		trig = INDEX_TO_TRIGGER(entry->id.idx);
-		dbtr_trigger_setup(trig, &entry->data);
+		hw_trig = INDEX_TO_HW_TRIGGER(GET_TRIG_HW_INDEX(trig->state));
+		dbtr_trigger_setup(trig, hw_trig, &entry->data);
 		dbtr_trigger_enable(trig);
 	}
 
-- 
2.51.0


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

[PATCH 15/18] dbtr: Move trigger feature support test into a function

[Nicholas Piggin]

With heterogeneous triggers this test becomes more complicated and is
neeed in more places, so move it into a function.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 74b114b8..77162efe 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -85,6 +85,14 @@ static inline void *hart_shmem_base(
 			hs->shmem.phys_hi, hs->shmem.phys_lo));
 }
 
+static bool sbi_hw_trigger_supports(struct sbi_hw_trigger *hw_trig,
+				unsigned long type, unsigned long tdata1)
+{
+	if (!(BIT(type) & hw_trig->type_mask))
+		return false;
+	return true;
+}
+
 static inline struct sbi_dbtr_trigger *sbi_alloc_trigger(void)
 {
 	int i;
@@ -555,7 +563,6 @@ int sbi_dbtr_num_trig(unsigned long data, unsigned long *out)
 	unsigned long type = TDATA1_GET_TYPE(data);
 	u32 hartid = current_hartid();
 	unsigned long total = 0;
-	struct sbi_hw_trigger *hw_trig;
 	int i;
 	struct sbi_dbtr_hart_triggers_state *hs;
 
@@ -569,9 +576,7 @@ int sbi_dbtr_num_trig(unsigned long data, unsigned long *out)
 	}
 
 	for (i = 0; i < hs->total_trigs; i++) {
-		hw_trig = INDEX_TO_HW_TRIGGER(i);
-
-		if (__test_bit(type, &hw_trig->type_mask))
+		if (sbi_hw_trigger_supports(INDEX_TO_HW_TRIGGER(i), type, data))
 			total++;
 	}
 
-- 
2.51.0


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

[PATCH 16/18] dbtr: Heterogeneous trigger type support

[Nicholas Piggin]

The Tenstorrent Ascalon core has 8 triggers that support the mcontrol6
type and another that supports the icount type.

When installing SBI triggers, allocate HW triggers to them that satisfy
their type requirement. A first pass is made over the requested SBI
triggers which checks that all can be satisfied with available hardware
triggers. If yes, then a second pass allocates each HW trigger and maps
the corresponding SBI trigger to it.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 104 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 74 insertions(+), 30 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 77162efe..f91a2b0d 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -93,6 +93,31 @@ static bool sbi_hw_trigger_supports(struct sbi_hw_trigger *hw_trig,
 	return true;
 }
 
+static struct sbi_hw_trigger *sbi_alloc_hw_trigger(
+				unsigned long type, unsigned long tdata1)
+{
+	int i;
+	struct sbi_dbtr_hart_triggers_state *hart_state;
+
+	hart_state = dbtr_thishart_state_ptr();
+	if (!hart_state)
+		return NULL;
+
+	for (i = 0; i < hart_state->total_trigs; i++) {
+		struct sbi_hw_trigger *hw_trig = INDEX_TO_HW_TRIGGER(i);
+
+		if (hw_trig->inuse)
+			continue;
+		if (!sbi_hw_trigger_supports(hw_trig, type, tdata1))
+			continue;
+		hw_trig->inuse = true;
+
+		return hw_trig;
+	}
+
+	return NULL;
+}
+
 static inline struct sbi_dbtr_trigger *sbi_alloc_trigger(void)
 {
 	int i;
@@ -357,8 +382,6 @@ static void dbtr_trigger_setup(struct sbi_dbtr_trigger *trig,
 
 	SET_TRIG_HW_INDEX(trig->state, hw_trig->index);
 
-	hw_trig->inuse = true;
-
 	switch (TDATA1_GET_TYPE(tdata1)) {
 	case RISCV_DBTR_TRIG_MCONTROL:
 		if (__test_bit(RV_DBTR_BIT(MC, U), &tdata1))
@@ -654,8 +677,10 @@ int sbi_dbtr_install_trig(unsigned long smode,
 	struct sbi_dbtr_data_msg *recv;
 	struct sbi_dbtr_id_msg *xmit;
 	struct sbi_dbtr_trigger *trig;
+	struct sbi_hw_trigger *hw_trig;
+	struct sbi_hw_trigger **hw_trigs;
 	struct sbi_dbtr_hart_triggers_state *hs = NULL;
-	int err;
+	int err, i;
 
 	hs = dbtr_thishart_state_ptr();
 	if (!hs)
@@ -671,64 +696,83 @@ int sbi_dbtr_install_trig(unsigned long smode,
 	sbi_hart_protection_map_range((unsigned long)shmem_base,
 				      trig_count * sizeof(*entry));
 
-	/* Check requested triggers configuration */
+	/*
+	 * The first pass checks requested triggers configuration and allocates
+	 * HW triggers, putting them into an array indexed by the recv'ed
+	 * triggers. The second pass allocates DBTR triggers and maps them to
+	 * the HW triggers.
+	 */
+	hw_trigs = sbi_zalloc(sizeof(hw_trigs[0]) * trig_count);
+	i = 0;
 	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
-		unsigned long tdata1;
+		unsigned long tdata1, type;
 
 		recv = (struct sbi_dbtr_data_msg *)(&entry->data);
 		tdata1 = lle_to_cpu(recv->tdata1);
+		type = TDATA1_GET_TYPE(tdata1);
 
-		if (!dbtr_trigger_supported(TDATA1_GET_TYPE(tdata1), tdata1)) {
+		if (!dbtr_trigger_supported(type, tdata1)) {
 			*out = _idx;
 			err = SBI_ERR_NOT_SUPPORTED;
-			goto out_unmap;
+			goto out_free_hw;
+		}
+
+		if (!dbtr_trigger_valid(type, tdata1)) {
+			*out = _idx;
+			err = SBI_ERR_INVALID_PARAM;
+			goto out_free_hw;
 		}
 
-		if (!dbtr_trigger_valid(TDATA1_GET_TYPE(tdata1), tdata1)) {
+		hw_trig = sbi_alloc_hw_trigger(type, tdata1);
+		if (!hw_trig) {
 			*out = _idx;
 			err = SBI_ERR_FAILED;
-			goto out_unmap;
+			goto out_free_hw;
 		}
-	}
 
-	if (hs->available_trigs < trig_count) {
-		*out = hs->available_trigs;
-		err = SBI_ERR_FAILED;
-		goto out_unmap;
+		hw_trigs[i] = hw_trig;
+		i++;
 	}
 
 	/* Install triggers */
+	i = 0;
 	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
-		struct sbi_hw_trigger *hw_trig;
-		/*
-		 * Since we have already checked if enough triggers are
-		 * available, trigger allocation must succeed.
-		 */
-		trig = sbi_alloc_trigger();
-
 		recv = (struct sbi_dbtr_data_msg *)(&entry->data);
 		xmit = (struct sbi_dbtr_id_msg *)(&entry->id);
 
-		hw_trig = INDEX_TO_HW_TRIGGER(trig->index); /* 1:1 trig / HW idx */
-		if (hw_trig->inuse) {
-			*out = hs->available_trigs;
-			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
-					trig_count * sizeof(*entry));
-			sbi_printf("DBTR error: Hardware trigger already inuse\n");
-			return SBI_ERR_FAILED;
-		}
+		/*
+		 * HW triggers were all allocated in the previous pass. Now
+		 * allocate and map dbtr triggers to them.
+		 */
+		hw_trig = hw_trigs[i++];
+
+		/*
+		 * Since we have already checked if enough HW triggers are
+		 * available, dbtr trig allocation must succeed here.
+		 */
+		trig = sbi_alloc_trigger();
 
 		dbtr_trigger_setup(trig, hw_trig, recv);
 		dbtr_trigger_enable(trig);
 		xmit->idx = cpu_to_lle(trig->index);
 	}
 
+	sbi_free(hw_trigs);
 	sbi_hart_protection_unmap_range((unsigned long)shmem_base,
 					trig_count * sizeof(*entry));
 
 	return SBI_SUCCESS;
 
-out_unmap:
+out_free_hw:
+	/* Free unused HW allocations */
+	for (i = 0; i < trig_count; i++) {
+		hw_trig = hw_trigs[i];
+		if (!hw_trig)
+			break;
+		hw_trig->inuse = false;
+	}
+
+	sbi_free(hw_trigs);
 	sbi_hart_protection_unmap_range((unsigned long)shmem_base,
 					trig_count * sizeof(*entry));
 
-- 
2.51.0


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

[PATCH 17/18] dbtr: Heterogeneous access type matching for mcontrol triggers

[Nicholas Piggin]

The Tenstorrent Ascalon core has a set of mcontrol6 triggers which can
match load/store, and a set which can match execute.

Detect access types supported in mc/mc6 triggers by writing 1 to
load/store/access fields and reading back the result. Unsupported bits
read back 0.

Account for this capability when allocating HW triggers.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/sbi/sbi_dbtr.h |  1 +
 lib/sbi/sbi_dbtr.c     | 77 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/include/sbi/sbi_dbtr.h b/include/sbi/sbi_dbtr.h
index c0b9369e..16407f34 100644
--- a/include/sbi/sbi_dbtr.h
+++ b/include/sbi/sbi_dbtr.h
@@ -52,6 +52,7 @@ struct sbi_hw_trigger {
 	unsigned long index;
 	bool inuse; /* Allocated for a DBTR trigger */
 	unsigned long type_mask; /* Types supported by this trigger */
+	unsigned long mcontrol_rwx_mask; /* rwx match bits supported */
 };
 
 struct sbi_dbtr_trigger {
diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index f91a2b0d..f3186993 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -60,6 +60,15 @@ static unsigned long hart_state_ptr_offset;
 
 #define DBTR_SHMEM_MAKE_PHYS(_p_hi, _p_lo) (_p_lo)
 
+/*
+ * MC and MC6 type and access fields are the same, no
+ * point duplicating this with MC vs MC6 accessors.
+ */
+#define RV_DBTR_MC_RWX_MASK						\
+		(RV_DBTR_BIT_MASK(MC, LOAD) |				\
+		 RV_DBTR_BIT_MASK(MC, STORE) |				\
+		 RV_DBTR_BIT_MASK(MC, EXEC))
+
 /* must call with hs != NULL */
 static inline bool sbi_dbtr_shmem_disabled(
 	struct sbi_dbtr_hart_triggers_state *hs)
@@ -90,6 +99,19 @@ static bool sbi_hw_trigger_supports(struct sbi_hw_trigger *hw_trig,
 {
 	if (!(BIT(type) & hw_trig->type_mask))
 		return false;
+
+	switch (type) {
+	case RISCV_DBTR_TRIG_MCONTROL:
+	case RISCV_DBTR_TRIG_MCONTROL6:
+		if ((hw_trig->mcontrol_rwx_mask & tdata1) != (tdata1 & RV_DBTR_MC_RWX_MASK))
+			return false;
+		break;
+	case RISCV_DBTR_TRIG_ICOUNT:
+		break;
+	default:
+		break;
+	}
+
 	return true;
 }
 
@@ -180,6 +202,7 @@ static bool sbi_hw_trigger_probe(int i)
 {
 	struct sbi_hw_trigger *hw_trig = INDEX_TO_HW_TRIGGER(i);
 	struct sbi_trap_info trap = {0};
+	unsigned long type;
 	unsigned long tdata1;
 	unsigned long val;
 
@@ -204,8 +227,6 @@ static bool sbi_hw_trigger_probe(int i)
 
 	val = csr_read_allowed(CSR_TINFO, &trap);
 	if (trap.cause) {
-		unsigned long type;
-
 		/*
 		 * If reading tinfo caused an exception, the
 		 * debugger must read tdata1 to discover the
@@ -227,6 +248,51 @@ static bool sbi_hw_trigger_probe(int i)
 		hw_trig->type_mask = val;
 	}
 
+	/* Probe supported features for each supported type */
+	for (type = 0; type < 32; type++) {
+		if (!(hw_trig->type_mask & (1UL << type)))
+			continue;
+		csr_write(CSR_TDATA1, 0x0);
+		csr_write(CSR_TDATA2, 0x0);
+
+		switch (type) {
+		case RISCV_DBTR_TRIG_MCONTROL:
+		case RISCV_DBTR_TRIG_MCONTROL6:
+			/*
+			 * Set an MC/MC6 type trigger with load/store/exec bits
+			 * set, then read which remain set. The implementation
+			 * should read back zeroes for unsupported bits.
+			 */
+			tdata1 = RV_DBTR_MC_RWX_MASK;
+			RV_DBTR_SET_MC_TYPE(tdata1, type);
+			csr_write(CSR_TDATA1, tdata1);
+			tdata1 = csr_read(CSR_TDATA1);
+			if (TDATA1_GET_TYPE(tdata1) != type) {
+				sbi_printf("DBTR error: Hardware trigger type could not be set\n");
+				/* Disable the trigger type */
+				hw_trig->type_mask &= ~(1UL << type);
+			}
+			hw_trig->mcontrol_rwx_mask = tdata1 & RV_DBTR_MC_RWX_MASK;
+			if (hw_trig->mcontrol_rwx_mask == 0) {
+				sbi_printf("DBTR error: Hardware trigger type has no access bits\n");
+				/* Disable the trigger type */
+				hw_trig->type_mask &= ~(1UL << type);
+			}
+
+			csr_write(CSR_TDATA1, 0x0); /* Reset tdata1 */
+			break;
+		case RISCV_DBTR_TRIG_ICOUNT:
+			break;
+		default:
+			break;
+		}
+	}
+
+	if (hw_trig->type_mask == 0) {
+		/* May get here if the above "impossible" conditions are hit */
+		return false;
+	}
+
 	return true;
 }
 
@@ -917,6 +983,13 @@ int sbi_dbtr_update_trig(unsigned long smode,
 			err = SBI_ERR_INVALID_PARAM;
 			goto out_unmap;
 		}
+
+		hw_trig = INDEX_TO_HW_TRIGGER(GET_TRIG_HW_INDEX(trig->state));
+		if (!sbi_hw_trigger_supports(hw_trig, type, tdata1)) {
+			*out = _idx;
+			err = SBI_ERR_NOT_SUPPORTED;
+			goto out_unmap;
+		}
 	}
 
 	/* Update triggers */
-- 
2.51.0


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

[PATCH 18/18] dbtr: Work around specification bug in range checks

[Nicholas Piggin]

The SBI spec appears to have an off-by-one / inequality error in range
testing, reported here:

https://github.com/riscv-non-isa/riscv-sbi-doc/issues/257

This change updates range checking accordingly.

Note: may want to hold off this change until upstream spec confirms.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 lib/sbi/sbi_dbtr.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index f3186993..2edec1f5 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -689,12 +689,9 @@ int sbi_dbtr_read_trig(unsigned long smode,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
-	if (trig_count >= hs->total_trigs)
-		return SBI_ERR_BAD_RANGE;
-
 	if (trig_idx_base >= hs->total_trigs ||
-	    trig_idx_base + trig_count >= hs->total_trigs)
-		return SBI_ERR_INVALID_PARAM;
+	    trig_idx_base + trig_count > hs->total_trigs)
+		return SBI_ERR_BAD_RANGE;
 
 	if (sbi_dbtr_shmem_disabled(hs))
 		return SBI_ERR_NO_SHMEM;
@@ -752,7 +749,7 @@ int sbi_dbtr_install_trig(unsigned long smode,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
-	if (trig_count >= hs->total_trigs)
+	if (trig_count > hs->total_trigs)
 		return SBI_ERR_BAD_RANGE;
 
 	if (sbi_dbtr_shmem_disabled(hs))
@@ -934,7 +931,7 @@ int sbi_dbtr_update_trig(unsigned long smode,
 	if (!hs)
 		return SBI_ERR_FAILED;
 
-	if (trig_count >= hs->total_trigs)
+	if (trig_count > hs->total_trigs)
 		return SBI_ERR_BAD_RANGE;
 
 	if (sbi_dbtr_shmem_disabled(hs))
-- 
2.51.0


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

Re: [PATCH 01/18] dbtr: Add consistent range checks to trigger ecalls

[Ilya Mamay]

Hi, Nicholas
This is a significant improvement over the previous version!

I have a few comments below, though they may be off the mark since they're
based on my own reading of the SBI and Sdtrig specifications.

On Fri, Mar 13, 2026 at 03:19:30PM +1000, Nicholas Piggin wrote:
> @@ -594,6 +597,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;

According to the SBI specification (section 19.4):
- "The sbiret.value is set to zero upon success or if shared memory is
  disabled". I interpret this to mean that sbi_dbtr_install_trig must return a
  trigger index along with an SBI_ERR_BAD_RANGE error.

- "sbiret.value is set to the array index i of the failing
  trigger configuration". In this case, sbiret.value should be hs->total_trigs,
  because that is the index of the first failing trigger configuration.

Same comment applies to sbi_dbtr_update_trig.

> @@ -710,14 +726,14 @@ int sbi_dbtr_update_trig(unsigned long smode,
>  	if (!hs)
>  		return SBI_ERR_FAILED;
>  
> +	if (trig_count >= hs->total_trigs)
> +		return SBI_ERR_BAD_RANGE;
> +

Regards
Ilya

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

Re: [PATCH 01/18] dbtr: Add consistent range checks to trigger ecalls

[Himanshu Chauhan]

On Fri, Mar 13, 2026 at 10:50 AM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Be more consistent with the sequence of trigger range checks in dbtr
> ecall handlers. This includes adding a missing range check in install
> triggers that can result in SBI past the end of the shm region.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  lib/sbi/sbi_dbtr.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
> index 8bcb4312..b0160163 100644
> --- a/lib/sbi/sbi_dbtr.c
> +++ b/lib/sbi/sbi_dbtr.c
> @@ -550,6 +550,9 @@ int sbi_dbtr_read_trig(unsigned long smode,
>         if (!hs)
>                 return SBI_ERR_FAILED;
>
> +       if (trig_count >= hs->total_trigs)
> +               return SBI_ERR_BAD_RANGE;
> +
>         if (trig_idx_base >= hs->total_trigs ||
>             trig_idx_base + trig_count >= hs->total_trigs)
>                 return SBI_ERR_INVALID_PARAM;
> @@ -594,6 +597,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;
>
> @@ -663,6 +669,11 @@ int sbi_dbtr_uninstall_trig(unsigned long trig_idx_base,
>         if (!hs)
>                 return SBI_ERR_FAILED;
>
> +       if (trig_idx_base >= hs->total_trigs ||
> +               find_last_bit(&trig_idx_mask, hs->total_trigs) +
> +                               trig_idx_base >= hs->total_trigs)
> +               return SBI_ERR_INVALID_PARAM;
> +
>         for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
>                 trig = INDEX_TO_TRIGGER(idx);
>                 if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)))
> @@ -688,6 +699,11 @@ int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
>         if (!hs)
>                 return SBI_ERR_FAILED;
>
> +       if (trig_idx_base >= hs->total_trigs ||
> +               find_last_bit(&trig_idx_mask, hs->total_trigs) +
> +                               trig_idx_base >= hs->total_trigs)
> +               return SBI_ERR_INVALID_PARAM;
> +
>         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);
> @@ -710,14 +726,14 @@ int sbi_dbtr_update_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;
>
>         shmem_base = hart_shmem_base(hs);
>
> -       if (trig_count >= hs->total_trigs)
> -               return SBI_ERR_BAD_RANGE;
> -
>         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;
> @@ -754,6 +770,11 @@ int sbi_dbtr_disable_trig(unsigned long trig_idx_base,
>         if (!hs)
>                 return SBI_ERR_FAILED;
>
> +       if (trig_idx_base >= hs->total_trigs ||
> +               find_last_bit(&trig_idx_mask, hs->total_trigs) +
> +                               trig_idx_base >= hs->total_trigs)
> +               return SBI_ERR_INVALID_PARAM;
> +
>         for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
>                 trig = INDEX_TO_TRIGGER(idx);
>                 dbtr_trigger_disable(trig);

Looks good to me.

Reviewed-by: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>

> --
> 2.51.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