[PATCH 3/3] lib: sbi_timer: Add support for timer events
Anup Patel
anup at brainfault.org
Thu Apr 23 01:25:59 PDT 2026
On Thu, Apr 23, 2026 at 12:24 PM Nicholas Piggin <npiggin at gmail.com> wrote:
>
> On Wed, Apr 15, 2026 at 04:30:02PM +0530, Anup Patel wrote:
> > Currently, the sbi_timer only supports timer events configured via
> > SBI calls. Introduce struct sbi_timer_event and related functions
> > to allow configuring timer events from any part of OpenSBI.
> >
> > Signed-off-by: Anup Patel <anup.patel at oss.qualcomm.com>
> > ---
> > include/sbi/sbi_timer.h | 43 ++++++++-
> > lib/sbi/sbi_ecall_legacy.c | 4 +-
> > lib/sbi/sbi_ecall_time.c | 4 +-
> > lib/sbi/sbi_timer.c | 177 +++++++++++++++++++++++++++++++++----
> > 4 files changed, 205 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/sbi/sbi_timer.h b/include/sbi/sbi_timer.h
> > index 2e1a7879..f23d72db 100644
> > --- a/include/sbi/sbi_timer.h
> > +++ b/include/sbi/sbi_timer.h
> > @@ -10,7 +10,38 @@
> > #ifndef __SBI_TIMER_H__
> > #define __SBI_TIMER_H__
> >
> > -#include <sbi/sbi_types.h>
> > +#include <sbi/sbi_list.h>
> > +
> > +/** Timer event abstraction */
> > +struct sbi_timer_event {
> > + /** List head for per-HART event list (Internal) */
> > + struct sbi_dlist head;
> > +
> > + /** Hart on which the event is started / running (Internal) */
> > + int hart_index;
> > +
> > + /** Time stamp when the event expires (Internal) */
> > + u64 time_stamp;
> > +
> > + /** Event callback to be called upon expiry */
> > + void (*callback)(struct sbi_timer_event *ev);
> > +
> > + /** Event cleanup to be called upon sbi_hart_exit() */
> ^ sbi_timer_exit()
>
> Minor typo.
Yes, it's a typo. I will update.
>
> > + void (*cleanup)(struct sbi_timer_event *ev);
> > +
> > + /** Event specific private data */
> > + void *priv;
> > +};
> > +
> > +#define SBI_INIT_TIMER_EVENT(__ptr, __callback, __cleanup, __priv) \
> > +do { \
> > + SBI_INIT_LIST_HEAD(&(__ptr)->head); \
> > + (__ptr)->hart_index = -1; \
> > + (__ptr)->time_stamp = 0; \
> > + (__ptr)->callback = (__callback); \
> > + (__ptr)->cleanup = (__cleanup); \
> > + (__ptr)->priv = (__priv); \
> > +} while (0)
> >
> > /** Timer hardware device */
> > struct sbi_timer_device {
> > @@ -86,8 +117,14 @@ void sbi_timer_set_delta(ulong delta);
> > void sbi_timer_set_delta_upper(ulong delta_upper);
> > #endif
> >
> > -/** Start timer event for current HART */
> > -void sbi_timer_event_start(u64 next_event);
> > +/** Start timer event on current HART */
> > +void sbi_timer_event_start(struct sbi_timer_event *ev, u64 next_event);
> > +
> > +/** Stop timer event on current HART */
> > +void sbi_timer_event_stop(struct sbi_timer_event *ev);
> > +
> > +/** Start supervisor timer event on current HART */
> > +void sbi_timer_smode_event_start(u64 next_event);
> >
> > /** Process timer event for current HART */
> > void sbi_timer_process(void);
> > diff --git a/lib/sbi/sbi_ecall_legacy.c b/lib/sbi/sbi_ecall_legacy.c
> > index 50a7660d..4501c4ad 100644
> > --- a/lib/sbi/sbi_ecall_legacy.c
> > +++ b/lib/sbi/sbi_ecall_legacy.c
> > @@ -54,9 +54,9 @@ static int sbi_ecall_legacy_handler(unsigned long extid, unsigned long funcid,
> > switch (extid) {
> > case SBI_EXT_0_1_SET_TIMER:
> > #if __riscv_xlen == 32
> > - sbi_timer_event_start((((u64)regs->a1 << 32) | (u64)regs->a0));
> > + sbi_timer_smode_event_start((((u64)regs->a1 << 32) | (u64)regs->a0));
> > #else
> > - sbi_timer_event_start((u64)regs->a0);
> > + sbi_timer_smode_event_start((u64)regs->a0);
> > #endif
> > break;
> > case SBI_EXT_0_1_CONSOLE_PUTCHAR:
> > diff --git a/lib/sbi/sbi_ecall_time.c b/lib/sbi/sbi_ecall_time.c
> > index 6ea6f054..a0aa580d 100644
> > --- a/lib/sbi/sbi_ecall_time.c
> > +++ b/lib/sbi/sbi_ecall_time.c
> > @@ -22,9 +22,9 @@ static int sbi_ecall_time_handler(unsigned long extid, unsigned long funcid,
> >
> > if (funcid == SBI_EXT_TIME_SET_TIMER) {
> > #if __riscv_xlen == 32
> > - sbi_timer_event_start((((u64)regs->a1 << 32) | (u64)regs->a0));
> > + sbi_timer_smode_event_start((((u64)regs->a1 << 32) | (u64)regs->a0));
> > #else
> > - sbi_timer_event_start((u64)regs->a0);
> > + sbi_timer_smode_event_start((u64)regs->a0);
> > #endif
> > } else
> > ret = SBI_ENOTSUPP;
> > diff --git a/lib/sbi/sbi_timer.c b/lib/sbi/sbi_timer.c
> > index 9806c033..539157fa 100644
> > --- a/lib/sbi/sbi_timer.c
> > +++ b/lib/sbi/sbi_timer.c
> > @@ -10,9 +10,11 @@
> > #include <sbi/riscv_asm.h>
> > #include <sbi/riscv_barrier.h>
> > #include <sbi/riscv_encoding.h>
> > +#include <sbi/riscv_locks.h>
> > #include <sbi/sbi_console.h>
> > #include <sbi/sbi_error.h>
> > #include <sbi/sbi_hart.h>
> > +#include <sbi/sbi_hartmask.h>
> > #include <sbi/sbi_platform.h>
> > #include <sbi/sbi_pmu.h>
> > #include <sbi/sbi_scratch.h>
> > @@ -20,6 +22,9 @@
> >
> > struct timer_state {
> > u64 time_delta;
> > + spinlock_t event_list_lock;
> > + struct sbi_dlist event_list;
> > + struct sbi_timer_event smode_ev;
> > };
> >
> > static unsigned long timer_state_off;
> > @@ -136,8 +141,119 @@ void sbi_timer_set_delta_upper(ulong delta_upper)
> > }
> > #endif
> >
> > -void sbi_timer_event_start(u64 next_event)
> > +static void __sbi_timer_update_device(struct timer_state *tstate)
> > {
> > + struct sbi_timer_event *ev;
> > +
> > + if (!timer_dev)
> > + return;
> > +
> > + if (sbi_list_empty(&tstate->event_list)) {
> > + if (timer_dev->timer_event_stop)
> > + timer_dev->timer_event_stop();
> > + csr_clear(CSR_MIE, MIP_MTIP);
> > + } else {
> > + ev = sbi_list_first_entry(&tstate->event_list, struct sbi_timer_event, head);
> > + if (timer_dev->timer_event_start)
> > + timer_dev->timer_event_start(ev->time_stamp);
> > + csr_set(CSR_MIE, MIP_MTIP);
> > + }
> > +}
> > +
> > +static void __sbi_timer_event_stop(struct sbi_timer_event *ev)
> > +{
> > + if (ev->hart_index > -1) {
> > + sbi_list_del(&ev->head);
> > + ev->hart_index = -1;
> > + }
> > +}
> > +
> > +void sbi_timer_event_start(struct sbi_timer_event *ev, u64 next_event)
> > +{
> > + struct sbi_timer_event *tev, *next_ev = NULL;
> > + struct timer_state *tstate;
> > +
> > + if (!ev)
> > + return;
> > +
> > + /* Ensure that event is not on the per-HART event list */
> > + if (ev->hart_index > -1) {
>
> It is a bit worrying to permit sbi_timer_event_start() if the timer is
> already on a list. Is that necessary, or could you return an error in
> this case?
>
> I'm thinking problems like this -
>
> HART0 HART1
> sbi_timer_event_start() sbi_timer_process()
>
> if (hart_index > -1) {
> __sbi_timer_event_stop(ev)
> if (ev->callback) {
> spin_unlock();
> spin_lock();
> __sbi_timer_event_stop(ev)
> spin_unlock();
> callback_fn()
> sbi_timer_event_start(); /* re-add periodic timer */
It is generally good to have ability to stop a running timer
event from any CPU but I agree the problem you are highlighting.
I think the real problem in this patch is the spin_unlock()/spin_lock()
dance around the callback function called by sbi_timer_process().
To avoid the spin_unlock()/spin_lock() dance in sbi_timer_process(),
I think it is better to call the callback function with lock held and callback
function can optionally return next_time_stamp if it wants to re-add
periodic timer. Suggestions ?
>
> > + tstate = sbi_scratch_offset_ptr(sbi_hartindex_to_scratch(ev->hart_index),
> > + timer_state_off);
> > + spin_lock(&tstate->event_list_lock);
> > + __sbi_timer_event_stop(ev);
> > + spin_unlock(&tstate->event_list_lock);
> > + }
> > +
> > + tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
> > + spin_lock(&tstate->event_list_lock);
> > +
> > + /* Find where to insert the event in per-HART event list */
> > + sbi_list_for_each_entry(tev, &tstate->event_list, head) {
> > + if (next_event < tev->time_stamp) {
> > + next_ev = tev;
> > + break;
> > + }
> > + }
> > +
> > + /* Insert the event in per-HART event list */
> > + ev->hart_index = current_hartindex();
> > + ev->time_stamp = next_event;
> > + if (next_ev)
> > + sbi_list_add(&ev->head, &next_ev->head);
> > + else
> > + sbi_list_add_tail(&ev->head, &tstate->event_list);
> > +
> > + __sbi_timer_update_device(tstate);
> > +
> > + spin_unlock(&tstate->event_list_lock);
> > +}
> > +
> > +void sbi_timer_event_stop(struct sbi_timer_event *ev)
> > +{
> > + struct timer_state *tstate;
> > +
> > + if (!ev)
> > + return;
> > +
> > + /* Ensure that event is not on the per-HART event list */
> > + if (ev->hart_index > -1) {
> > + tstate = sbi_scratch_offset_ptr(sbi_hartindex_to_scratch(ev->hart_index),
> > + timer_state_off);
> > + spin_lock(&tstate->event_list_lock);
> > + __sbi_timer_event_stop(ev);
> > + spin_unlock(&tstate->event_list_lock);
>
> Similar concern here, timer could be in the middle of running. Maybe it
> is benign. I think it would be nice to make this a "sync" stop though,
> so it can wait for potential running timers.
>
> timer event could have a 'running' state which is updated inside the
> lock, then sbi_timer_event_stop() (and sbi_timer_exit()) could wait for
> it to clear.
We might have to waste a lot of CPU cycles in the busy-loop.
>
> > + }
> > +
> > + /* Re-program timer device on the current HART */
> > + tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
> > + spin_lock(&tstate->event_list_lock);
> > + __sbi_timer_update_device(tstate);
> > + spin_unlock(&tstate->event_list_lock);
>
> This could be skipped if ev->hart_index != current_hartid()
Yes, good suggestion. I will update.
>
> > +}
> > +
> > +static void sbi_timer_smode_event_callback(struct sbi_timer_event *ev)
> > +{
> > + /*
> > + * If sstc extension is available, supervisor can receive the timer
> > + * directly without M-mode come in between. This function should
> > + * only invoked if M-mode programs the timer for its own purpose.
> > + */
> > + if (!sbi_hart_has_extension(sbi_scratch_thishart_ptr(), SBI_HART_EXT_SSTC))
> > + csr_set(CSR_MIP, MIP_STIP);
> > +}
> > +
> > +static void sbi_timer_smode_event_cleanup(struct sbi_timer_event *ev)
> > +{
> > + if (!sbi_hart_has_extension(sbi_scratch_thishart_ptr(), SBI_HART_EXT_SSTC))
> > + csr_clear(CSR_MIP, MIP_STIP);
> > +}
> > +
> > +void sbi_timer_smode_event_start(u64 next_event)
> > +{
> > + struct timer_state *tstate = sbi_scratch_offset_ptr(sbi_scratch_thishart_ptr(),
> > + timer_state_off);
> > +
> > sbi_pmu_ctr_incr_fw(SBI_PMU_FW_SET_TIMER);
> >
> > /**
> > @@ -146,23 +262,34 @@ void sbi_timer_event_start(u64 next_event)
> > */
> > if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(), SBI_HART_EXT_SSTC)) {
> > csr_write64(CSR_STIMECMP, next_event);
> > - } else if (timer_dev && timer_dev->timer_event_start) {
> > - timer_dev->timer_event_start(next_event);
> > + } else {
> > csr_clear(CSR_MIP, MIP_STIP);
> > + sbi_timer_event_start(&tstate->smode_ev, next_event);
> > }
> > - csr_set(CSR_MIE, MIP_MTIP);
> > }
> >
> > void sbi_timer_process(void)
> > {
> > - csr_clear(CSR_MIE, MIP_MTIP);
> > - /*
> > - * If sstc extension is available, supervisor can receive the timer
> > - * directly without M-mode come in between. This function should
> > - * only invoked if M-mode programs the timer for its own purpose.
> > - */
> > - if (!sbi_hart_has_extension(sbi_scratch_thishart_ptr(), SBI_HART_EXT_SSTC))
> > - csr_set(CSR_MIP, MIP_STIP);
> > + struct timer_state *tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
> > + struct sbi_timer_event *ev;
> > +
> > + spin_lock(&tstate->event_list_lock);
> > +
> > + while (!sbi_list_empty(&tstate->event_list)) {
> > + ev = sbi_list_first_entry(&tstate->event_list, struct sbi_timer_event, head);
> > + if (ev->time_stamp <= sbi_timer_value()) {
>
> Since the list is sorted this could break if time_stamp >
> sbi_timer_value()?
Yes, good suggestion. I will update.
Regards,
Anup
More information about the opensbi
mailing list