Re: [PATCH 3/3] lib: sbi_timer: Add support for timer events

[Nicholas Piggin <npiggin@gmail.com>]

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

> +	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 */

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

> +	}
> +
> +	/* 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()

> +}
> +
> +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()?

> +			__sbi_timer_event_stop(ev);
> +			if (ev->callback) {
> +				spin_unlock(&tstate->event_list_lock);
> +				ev->callback(ev);
> +				spin_lock(&tstate->event_list_lock);
> +			}
> +		}
> +	}
> +
> +	__sbi_timer_update_device(tstate);
> +
> +	spin_unlock(&tstate->event_list_lock);
>  }
>  
>  const struct sbi_timer_device *sbi_timer_get_device(void)

Thanks,
Nick

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