[PATCH] lib: sbi: Fix DBTR error codes and semantics as per SBI v3.0 spec
Anup Patel
anup at brainfault.org
Fri Sep 4 00:59:20 PDT 2026
On Fri, Aug 14, 2026 at 10:54 PM David E. Garcia Porras
<david.garcia at aheadcomputing.com> wrote:
>
> 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.
Too many changes in a single patch. Please try to split each change
into separate patch.
Regards,
Anup
>
> - Set the trig_state.have_hw_trig bit when a trigger is mapped to a
> HW debug trigger (table 98).
>
> Signed-off-by: David E. Garcia Porras <david.garcia at aheadcomputing.com>
> ---
> include/sbi/sbi_dbtr.h | 5 ++-
> lib/sbi/sbi_dbtr.c | 91 +++++++++++++++++++++++++++++++++-------
> lib/sbi/sbi_ecall_dbtr.c | 4 +-
> 3 files changed, 81 insertions(+), 19 deletions(-)
>
> diff --git a/include/sbi/sbi_dbtr.h b/include/sbi/sbi_dbtr.h
> index 5e0bf84e..e6b76086 100644
> --- a/include/sbi/sbi_dbtr.h
> +++ b/include/sbi/sbi_dbtr.h
> @@ -104,7 +104,8 @@ int sbi_dbtr_init(struct sbi_scratch *scratch, bool coldboot);
> int sbi_dbtr_supported(void);
> int sbi_dbtr_setup_shmem(const struct sbi_domain *dom, unsigned long smode,
> unsigned long shmem_phys_lo,
> - unsigned long shmem_phys_hi);
> + unsigned long shmem_phys_hi,
> + unsigned long flags);
> int sbi_dbtr_num_trig(unsigned long trig_tdata1, unsigned long *out);
> int sbi_dbtr_read_trig(unsigned long smode,
> unsigned long trig_idx_base, unsigned long trig_count);
> @@ -115,7 +116,7 @@ int sbi_dbtr_uninstall_trig(unsigned long trig_idx_base,
> int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
> unsigned long trig_idx_mask);
> int sbi_dbtr_update_trig(unsigned long smode,
> - unsigned long trig_count);
> + unsigned long trig_count, unsigned long *out);
> int sbi_dbtr_disable_trig(unsigned long trig_idx_base,
> unsigned long trig_idx_mask);
>
> diff --git a/lib/sbi/sbi_dbtr.c b/lib/sbi/sbi_dbtr.c
> index 01047969..1d4785c7 100644
> --- a/lib/sbi/sbi_dbtr.c
> +++ b/lib/sbi/sbi_dbtr.c
> @@ -261,8 +261,10 @@ int sbi_dbtr_get_total_triggers(void)
>
> int sbi_dbtr_setup_shmem(const struct sbi_domain *dom, unsigned long smode,
> unsigned long shmem_phys_lo,
> - unsigned long shmem_phys_hi)
> + unsigned long shmem_phys_hi,
> + unsigned long flags)
> {
> + unsigned long shmem_size;
> struct sbi_dbtr_hart_triggers_state *hart_state;
>
> if (dom && !sbi_domain_is_assigned_hart(dom, current_hartindex())) {
> @@ -271,6 +273,9 @@ int sbi_dbtr_setup_shmem(const struct sbi_domain *dom, unsigned long smode,
> return SBI_ERR_DENIED;
> }
>
> + if (flags)
> + return SBI_ERR_INVALID_PARAM;
> +
> hart_state = dbtr_thishart_state_ptr();
> if (!hart_state)
> return SBI_ERR_FAILED;
> @@ -304,9 +309,11 @@ int sbi_dbtr_setup_shmem(const struct sbi_domain *dom, unsigned long smode,
> if (shmem_phys_hi)
> return SBI_EINVALID_ADDR;
>
> - if (dom && !sbi_domain_check_addr(dom,
> - DBTR_SHMEM_MAKE_PHYS(shmem_phys_hi, shmem_phys_lo), smode,
> - SBI_DOMAIN_READ | SBI_DOMAIN_WRITE))
> + /* Must fail if address *range* is not fully accessible - size is assumed to be trig_max * (XLEN /2) bytes */
> + shmem_size = (unsigned long) hart_state->total_trigs * (__riscv_xlen / 2);
> + if (dom && !sbi_domain_check_addr_range(dom,
> + DBTR_SHMEM_MAKE_PHYS(shmem_phys_hi, shmem_phys_lo), shmem_size,
> + smode, SBI_DOMAIN_READ | SBI_DOMAIN_WRITE))
> return SBI_ERR_INVALID_ADDRESS;
>
> hart_state->shmem.phys_lo = shmem_phys_lo;
> @@ -332,6 +339,7 @@ static void dbtr_trigger_setup(struct sbi_dbtr_trigger *trig,
> trig->state = 0;
>
> __set_bit(RV_DBTR_BIT(TS, MAPPED), &trig->state);
> + __set_bit(RV_DBTR_BIT(TS, HAVE_TRIG), &trig->state);
>
> SET_TRIG_HW_INDEX(trig->state, trig->index);
>
> @@ -616,6 +624,9 @@ int sbi_dbtr_install_trig(unsigned long smode,
> if (!hs)
> return SBI_ERR_FAILED;
>
> + if (trig_count >= hs->total_trigs)
> + return SBI_ERR_BAD_RANGE;
> +
> if (sbi_dbtr_shmem_disabled(hs))
> return SBI_ERR_NO_SHMEM;
>
> @@ -641,7 +652,7 @@ int sbi_dbtr_install_trig(unsigned long smode,
> *out = _idx;
> sbi_hart_protection_unmap_range((unsigned long)shmem_base,
> trig_count * sizeof(*entry));
> - return SBI_ERR_FAILED;
> + return SBI_ERR_NOT_SUPPORTED;
> }
>
> if (!dbtr_trigger_valid(TDATA1_GET_TYPE(ctrl), ctrl)) {
> @@ -702,13 +713,17 @@ int sbi_dbtr_uninstall_trig(unsigned long trig_idx_base,
> if (!hs)
> return SBI_ERR_FAILED;
>
> - for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
> + for_each_set_bit_from(idx, &trig_mask, RV_MAX_TRIGGERS) {
> trig = INDEX_TO_TRIGGER(idx);
> - if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)))
> + if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)) || idx >= hs->total_trigs)
> return SBI_ERR_INVALID_PARAM;
> + }
>
> + /* Only uninstall after validating the arguments - maintain atomicity */
> + idx = trig_idx_base;
> + for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
> + trig = INDEX_TO_TRIGGER(idx);
> dbtr_trigger_clear(trig);
> -
> sbi_free_trigger(trig);
> }
>
> @@ -727,6 +742,14 @@ int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
> if (!hs)
> return SBI_ERR_FAILED;
>
> + for_each_set_bit_from(idx, &trig_mask, RV_MAX_TRIGGERS) {
> + trig = INDEX_TO_TRIGGER(idx);
> + if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)) || idx >= hs->total_trigs)
> + return SBI_ERR_INVALID_PARAM;
> + }
> +
> + /* Only enable after validating the arguments - maintain atomicity */
> + idx = trig_idx_base;
> for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
> trig = INDEX_TO_TRIGGER(idx);
> sbi_dprintf("%s: enable trigger %lu\n", __func__, idx);
> @@ -737,8 +760,9 @@ int sbi_dbtr_enable_trig(unsigned long trig_idx_base,
> }
>
> int sbi_dbtr_update_trig(unsigned long smode,
> - unsigned long trig_count)
> + unsigned long trig_count, unsigned long *out)
> {
> + int stat = SBI_SUCCESS;
> unsigned long trig_idx;
> struct sbi_dbtr_trigger *trig;
> union sbi_dbtr_shmem_entry *entry;
> @@ -772,23 +796,52 @@ int sbi_dbtr_update_trig(unsigned long smode,
> trig_idx = entry->id.idx;
>
> if (trig_idx >= hs->total_trigs) {
> - sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
> - return SBI_ERR_INVALID_PARAM;
> + stat = SBI_ERR_INVALID_PARAM;
> + goto endloop;
> }
>
> trig = INDEX_TO_TRIGGER(trig_idx);
>
> if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED))) {
> - sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
> - return SBI_ERR_FAILED;
> + stat = SBI_ERR_FAILED;
> + goto endloop;
> + }
> +
> + if (TDATA1_GET_TYPE(entry->data.tdata1) != TDATA1_GET_TYPE(trig->tdata1)) {
> + stat = SBI_ERR_INVALID_PARAM;
> + goto endloop;
> + }
> +
> + switch(TDATA1_GET_TYPE(entry->data.tdata1)) {
> + case RISCV_DBTR_TRIG_MCONTROL:
> + case RISCV_DBTR_TRIG_MCONTROL6:
> + if ( (entry->data.tdata1 & RV_DBTR_BIT_MASK(MC6, CHAIN)) != (trig->tdata1 & RV_DBTR_BIT_MASK(MC6, CHAIN)) ) {
> + stat = SBI_ERR_INVALID_PARAM;
> + goto endloop;
> + }
> + break;
> + default:
> + break;
> }
>
> if ((entry->data.tdata2 && !tdata2_impl) ||
> (entry->data.tdata3 && !tdata3_impl)) {
> - sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
> - return SBI_ERR_NOT_SUPPORTED;
> + stat = SBI_ERR_NOT_SUPPORTED;
> + goto endloop;
> }
>
> + endloop:
> + sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
> + if (stat != SBI_SUCCESS) {
> + *out = _idx;
> + return stat;
> + }
> + }
> +
> + /* Only update after validating request - maintain atomicity */
> + for_each_trig_entry(shmem_base, trig_count, typeof(*entry), entry) {
> + sbi_hart_protection_map_range((unsigned long)entry, sizeof(*entry));
> + trig = INDEX_TO_TRIGGER(entry->id.idx);
> dbtr_trigger_setup(trig, &entry->data);
> sbi_hart_protection_unmap_range((unsigned long)entry, sizeof(*entry));
> dbtr_trigger_enable(trig);
> @@ -809,6 +862,14 @@ int sbi_dbtr_disable_trig(unsigned long trig_idx_base,
> if (!hs)
> return SBI_ERR_FAILED;
>
> + for_each_set_bit_from(idx, &trig_mask, RV_MAX_TRIGGERS) {
> + trig = INDEX_TO_TRIGGER(idx);
> + if (!(trig->state & RV_DBTR_BIT_MASK(TS, MAPPED)) || idx >= hs->total_trigs)
> + return SBI_ERR_INVALID_PARAM;
> + }
> +
> + /* Only disable after validating the arguments - maintain atomicity */
> + idx = trig_idx_base;
> for_each_set_bit_from(idx, &trig_mask, hs->total_trigs) {
> trig = INDEX_TO_TRIGGER(idx);
> dbtr_trigger_disable(trig);
> diff --git a/lib/sbi/sbi_ecall_dbtr.c b/lib/sbi/sbi_ecall_dbtr.c
> index 40a437ee..b86ce5b3 100644
> --- a/lib/sbi/sbi_ecall_dbtr.c
> +++ b/lib/sbi/sbi_ecall_dbtr.c
> @@ -28,7 +28,7 @@ static int sbi_ecall_dbtr_handler(unsigned long extid, unsigned long funcid,
> break;
> case SBI_EXT_DBTR_SETUP_SHMEM:
> ret = sbi_dbtr_setup_shmem(sbi_domain_thishart_ptr(), smode,
> - regs->a0, regs->a1);
> + regs->a0, regs->a1, regs->a2);
> break;
> case SBI_EXT_DBTR_TRIGGER_READ:
> ret = sbi_dbtr_read_trig(smode, regs->a0, regs->a1);
> @@ -43,7 +43,7 @@ static int sbi_ecall_dbtr_handler(unsigned long extid, unsigned long funcid,
> ret = sbi_dbtr_enable_trig(regs->a0, regs->a1);
> break;
> case SBI_EXT_DBTR_TRIGGER_UPDATE:
> - ret = sbi_dbtr_update_trig(smode, regs->a0);
> + ret = sbi_dbtr_update_trig(smode, regs->a0, &out->value);
> break;
> case SBI_EXT_DBTR_TRIGGER_DISABLE:
> ret = sbi_dbtr_disable_trig(regs->a0, regs->a1);
> --
> 2.43.0
>
>
> --
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
More information about the opensbi
mailing list