[PATCH 0/3] lib: sbi: dbtr: per-slot trigger capability detection

[PATCH 0/3] lib: sbi: dbtr: per-slot trigger capability detection

[David E. Garcia Porras]

Patch 1 adds an optional platform device (struct sbi_dbtr_device)
reporting whether a trigger slot supports a given configuration --
WARL fields tied off inside an implemented trigger CSR are not
discoverable via tinfo -- and makes trigger allocation
configuration-aware instead of picking the first free slot.

Patches 2 and 3 fix error return codes: sbi_dbtr_read_trig() returns
SBI_ERR_BAD_RANGE per SBI v3.0 section 19.3, and
sbi_dbtr_install_trig() returns SBI_ERR_INVALID_PARAM for an invalid
trigger configuration per section 19.4.

David E. Garcia Porras (3):
  lib: sbi: dbtr: add platform device for per-slot trigger capabilities
  lib: sbi: dbtr: return SBI_ERR_BAD_RANGE for invalid trigger index
    range
  lib: sbi: dbtr: return SBI_ERR_INVALID_PARAM for invalid trigger
    configuration

 include/sbi/sbi_dbtr.h |  10 ++++
 lib/sbi/sbi_dbtr.c     | 166 ++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 154 insertions(+), 22 deletions(-)

--
2.43.0


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

[PATCH 1/3] lib: sbi: dbtr: add platform device for per-slot trigger capabilities

[David E. Garcia Porras]

The tinfo probe only discovers trigger types; WARL fields tied off
inside an implemented trigger CSR (e.g. an mcontrol6 trigger without
load/store address match) are not discoverable. sbi_alloc_trigger()
also selects the first free trigger slot irrespective of the requested
trigger type or configuration. Unsupported configurations are thus
silently dropped by the hardware while SBI reports success, where SBI
v3.0 sections 19.4 / 19.5 require SBI_ERR_NOT_SUPPORTED.

Add an optional platform device, struct sbi_dbtr_device, with a
trigger_supported(idx, tdata1, tdata2, tdata3) callback reporting
whether the trigger slot selected by idx (the tselect value) supports
a given configuration. Use it, together with the probed per-slot
type_mask, to:

  - allocate only trigger slots supporting the requested configuration
    (new dbtr_find_free_slot() helper)
  - reject unsupportable install/update requests with
    SBI_ERR_NOT_SUPPORTED
  - count only supporting slots in sbi_dbtr_num_trig() (SBI v3.0
    section 19.1)

The install path now dry-runs the allocation of the whole batch before
programming any trigger, so a partially-installed batch is never left
behind. Without a registered device, only the type matching applies (unchanged behavior).

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 |  10 ++++
 lib/sbi/sbi_dbtr.c     | 162 +++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 152 insertions(+), 20 deletions(-)

diff --git a/include/sbi/sbi_dbtr.h b/include/sbi/sbi_dbtr.h
index 5e0bf84e..90871f0d 100644
--- a/include/sbi/sbi_dbtr.h
+++ b/include/sbi/sbi_dbtr.h
@@ -77,6 +77,16 @@ struct sbi_dbtr_hart_triggers_state {
 	u32 probed;
 };
 
+/** Platform specific debug trigger operations */
+struct sbi_dbtr_device {
+	char name[32];
+	bool (*trigger_supported)(unsigned long idx, unsigned long tdata1,
+				  unsigned long tdata2, unsigned long tdata3);
+};
+
+const struct sbi_dbtr_device *sbi_dbtr_get_device(void);
+void sbi_dbtr_set_device(const struct sbi_dbtr_device *dev);
+
 #define TDATA1_GET_TYPE(_t1)					\
 	EXTRACT_FIELD(_t1, RV_DBTR_BIT_MASK(TDATA1, TYPE))
 
diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
index 01047969..a04f367d 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -24,6 +24,22 @@
 /** Offset of pointer to HART's debug triggers info in scratch space */
 static unsigned long hart_state_ptr_offset;
 
+/** Device specific debug trigger operations */
+static const struct sbi_dbtr_device *dbtr_dev = NULL;
+
+const struct sbi_dbtr_device *sbi_dbtr_get_device(void)
+{
+	return dbtr_dev;
+}
+
+void sbi_dbtr_set_device(const struct sbi_dbtr_device *dev)
+{
+	if (!dev || dbtr_dev)
+		return;
+
+	dbtr_dev = dev;
+}
+
 #define dbtr_get_hart_state_ptr(__scratch)				\
 	sbi_scratch_read_type((__scratch), void *, hart_state_ptr_offset)
 
@@ -105,10 +121,75 @@ static void sbi_trigger_init(struct sbi_dbtr_trigger *trig,
 	trig->index = idx;
 }
 
-static inline struct sbi_dbtr_trigger *sbi_alloc_trigger(void)
+static bool dbtr_trigger_hw_supported(unsigned long idx, unsigned long tdata1,
+				      unsigned long tdata2,
+				      unsigned long tdata3)
+{
+	if (dbtr_dev && dbtr_dev->trigger_supported)
+		return dbtr_dev->trigger_supported(idx, tdata1, tdata2,
+						   tdata3);
+
+	return true;
+}
+
+static bool dbtr_trigger_any_hw_supported(
+			struct sbi_dbtr_hart_triggers_state *hs,
+			unsigned long tdata1, unsigned long tdata2,
+			unsigned long tdata3)
+{
+	unsigned long type = TDATA1_GET_TYPE(tdata1);
+	struct sbi_dbtr_trigger *trig;
+	int i;
+
+	for (i = 0; i < hs->total_trigs; i++) {
+		trig = INDEX_TO_TRIGGER(i);
+		if (__test_bit(type, &trig->type_mask) &&
+		    dbtr_trigger_hw_supported(trig->index, tdata1, tdata2,
+					      tdata3))
+			return true;
+	}
+
+	return false;
+}
+
+/*
+ * Find the first free hardware trigger slot supporting the configuration.
+ * Slots set in claimed_mask are treated as taken, which allows the caller
+ * to track slot availability. A 32-bit mask covers RV_MAX_TRIGGERS (32);
+ * for a larger number of triggers, this function needs to be updated.
+ */
+static int dbtr_find_free_slot(struct sbi_dbtr_hart_triggers_state *hs,
+			       u32 claimed_mask,
+			       unsigned long tdata1, unsigned long tdata2,
+			       unsigned long tdata3)
+{
+	unsigned long type = TDATA1_GET_TYPE(tdata1);
+	struct sbi_dbtr_trigger *trig;
+	int i;
+
+	for (i = 0; i < hs->total_trigs; i++) {
+		trig = INDEX_TO_TRIGGER(i);
+		if (trig->state & RV_DBTR_BIT_MASK(TS, MAPPED))
+			continue;
+		if (claimed_mask & BIT(i))
+			continue;
+		if (!__test_bit(type, &trig->type_mask))
+			continue;
+		if (!dbtr_trigger_hw_supported(trig->index, tdata1,
+					       tdata2, tdata3))
+			continue;
+		return i;
+	}
+
+	return SBI_ENOENT;
+}
+
+static inline struct sbi_dbtr_trigger *sbi_alloc_trigger(unsigned long tdata1,
+							 unsigned long tdata2,
+							 unsigned long tdata3)
 {
 	int i;
-	struct sbi_dbtr_trigger *f_trig = NULL;
+	struct sbi_dbtr_trigger *f_trig;
 	struct sbi_dbtr_hart_triggers_state *hart_state;
 
 	hart_state = dbtr_thishart_state_ptr();
@@ -118,17 +199,12 @@ static inline struct sbi_dbtr_trigger *sbi_alloc_trigger(void)
 	if (hart_state->available_trigs <= 0)
 		return NULL;
 
-	for (i = 0; i < hart_state->total_trigs; i++) {
-		f_trig = INDEX_TO_TRIGGER(i);
-		if (f_trig->state & RV_DBTR_BIT_MASK(TS, MAPPED))
-			continue;
-		hart_state->available_trigs--;
-		break;
-	}
-
-	if (i == hart_state->total_trigs)
+	i = dbtr_find_free_slot(hart_state, 0, tdata1, tdata2, tdata3);
+	if (i < 0)
 		return NULL;
 
+	f_trig = INDEX_TO_TRIGGER(i);
+	hart_state->available_trigs--;
 	__set_bit(RV_DBTR_BIT(TS, MAPPED), &f_trig->state);
 
 	return f_trig;
@@ -547,7 +623,8 @@ int sbi_dbtr_num_trig(unsigned long data, unsigned long *out)
 	for (i = 0; i < hs->total_trigs; i++) {
 		trig = INDEX_TO_TRIGGER(i);
 
-		if (__test_bit(type, &trig->type_mask))
+		if (__test_bit(type, &trig->type_mask) &&
+		    dbtr_trigger_hw_supported(trig->index, data, 0, 0))
 			total++;
 	}
 
@@ -608,6 +685,8 @@ int sbi_dbtr_install_trig(unsigned long smode,
 	struct sbi_dbtr_data_msg *recv;
 	struct sbi_dbtr_id_msg *xmit;
 	unsigned long ctrl;
+	u32 claimed = 0;
+	int slot;
 	struct sbi_dbtr_trigger *trig;
 	struct sbi_dbtr_hart_triggers_state *hs = NULL;
 	bool tdata2_impl, tdata3_impl;
@@ -626,8 +705,10 @@ int sbi_dbtr_install_trig(unsigned long smode,
 	/*
 	 * 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.
+	 * the "whole CSR unimplemented" case is caught here; WARL bits tied
+	 * off inside an otherwise-implemented CSR are delegated to the
+	 * device-specific trigger_supported() callback via
+	 * dbtr_trigger_any_hw_supported().
 	 */
 	tdata2_impl = tdata_implemented(CSR_TDATA2);
 	tdata3_impl = tdata_implemented(CSR_TDATA3);
@@ -658,6 +739,16 @@ int sbi_dbtr_install_trig(unsigned long smode,
 							trig_count * sizeof(*entry));
 			return SBI_ERR_NOT_SUPPORTED;
 		}
+
+		if (!dbtr_trigger_any_hw_supported(hs,
+						   lle_to_cpu(recv->tdata1),
+						   lle_to_cpu(recv->tdata2),
+						   lle_to_cpu(recv->tdata3))) {
+			*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) {
@@ -667,17 +758,40 @@ int sbi_dbtr_install_trig(unsigned long smode,
 		return SBI_ERR_FAILED;
 	}
 
-	/* Install triggers */
+	/*
+	 * Dry-run the allocation of the whole batch so that no trigger
+	 * is installed if any of the requested configurations cannot be
+	 * matched to a free hardware trigger slot.
+	 */
 	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
-		/*
-		 * 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);
+		slot = dbtr_find_free_slot(hs, claimed,
+					   lle_to_cpu(recv->tdata1),
+					   lle_to_cpu(recv->tdata2),
+					   lle_to_cpu(recv->tdata3));
+		if (slot < 0) {
+			*out = _idx;
+			sbi_hart_protection_unmap_range((unsigned long)shmem_base,
+							trig_count * sizeof(*entry));
+			return SBI_ERR_FAILED;
+		}
+		claimed |= BIT(slot);
+	}
 
+	/* Install triggers */
+	for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
 		recv = (struct sbi_dbtr_data_msg *)(&entry->data);
 		xmit = (struct sbi_dbtr_id_msg *)(&entry->id);
 
+		/*
+		 * The dry-run above matched every requested configuration
+		 * to a free hardware trigger slot, so allocation must
+		 * succeed.
+		 */
+		trig = sbi_alloc_trigger(lle_to_cpu(recv->tdata1),
+					 lle_to_cpu(recv->tdata2),
+					 lle_to_cpu(recv->tdata3));
+
 		dbtr_trigger_setup(trig,  recv);
 		dbtr_trigger_enable(trig);
 		xmit->idx = cpu_to_lle(trig->index);
@@ -789,6 +903,14 @@ int sbi_dbtr_update_trig(unsigned long smode,
 			return SBI_ERR_NOT_SUPPORTED;
 		}
 
+		if (!dbtr_trigger_hw_supported(trig->index,
+					       lle_to_cpu(entry->data.tdata1),
+					       lle_to_cpu(entry->data.tdata2),
+					       lle_to_cpu(entry->data.tdata3))) {
+			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

[PATCH 2/3] lib: sbi: dbtr: return SBI_ERR_BAD_RANGE for invalid trigger index range

[David E. Garcia Porras]

sbi_dbtr_read_trig() returns SBI_ERR_INVALID_PARAM for an out-of-range
trigger index range, where SBI v3.0 section 19.3 defines
SBI_ERR_BAD_RANGE. Return SBI_ERR_BAD_RANGE, matching the equivalent
range check in sbi_dbtr_update_trig().

Fixes: 324021423d06 ("lib: sbi: dbtr: Fix update_triggers to match SBI")
Signed-off-by: David E. Garcia Porras <david.garcia@aheadcomputing.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 a04f367d..d06fcaeb 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -650,7 +650,7 @@ int sbi_dbtr_read_trig(unsigned long smode,
 
 	if (trig_idx_base >= hs->total_trigs ||
 	    trig_idx_base + trig_count >= hs->total_trigs)
-		return SBI_ERR_INVALID_PARAM;
+		return SBI_ERR_BAD_RANGE;
 
 	if (sbi_dbtr_shmem_disabled(hs))
 		return SBI_ERR_NO_SHMEM;
-- 
2.43.0


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

[PATCH 3/3] lib: sbi: dbtr: return SBI_ERR_INVALID_PARAM for invalid trigger configuration

[David E. Garcia Porras]

sbi_dbtr_install_trig() returns the generic SBI_ERR_FAILED when
dbtr_trigger_valid() rejects a trigger configuration (dmode or M-mode
bits set), where SBI v3.0 section 19.4 defines SBI_ERR_INVALID_PARAM
for an invalid trigger configuration entry. Return
SBI_ERR_INVALID_PARAM instead.

Fixes: 97f234f15c96 ("lib: sbi: Introduce the SBI debug triggers extension support")
Signed-off-by: David E. Garcia Porras <david.garcia@aheadcomputing.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 d06fcaeb..4fd7fb5e 100644
--- a/lib/sbi/sbi_dbtr.c
+++ b/lib/sbi/sbi_dbtr.c
@@ -729,7 +729,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_INVALID_PARAM;
 		}
 
 		if ((recv->tdata2 && !tdata2_impl) ||
-- 
2.43.0


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