[PATCH 1/3] lib: sbi: dbtr: add platform device for per-slot trigger capabilities
Anup Patel
anup at brainfault.org
Mon Aug 31 22:05:54 PDT 2026
On Tue, Jul 28, 2026 at 12:00 AM David E. Garcia Porras
<david.garcia at aheadcomputing.com> wrote:
>
> 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 at aheadcomputing.com>
LGTM.
Reviewed-by: Anup Patel <anup at brainfault.org>
Applied this patch to the riscv/opensbi repo.
Thanks,
Anup
> ---
> 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
>
More information about the opensbi
mailing list