Re: [PATCH v1 12/15] xen/riscv: introduce sbi_set_timer()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/12/26 4:12 PM, Jan Beulich wrote:
> On 24.12.2025 18:03, Oleksii Kurochko wrote:
> > Introduce pointer to function which points to a specific sbi_set_timer()
> > implementation. It is done in this way as different OpenSBI version can
> > have different Extenion ID and/or funcion ID for TIME extension.
> >
> > sbi_set_time() programs the clock for next event after stime_value
> > time. This function also clears the pending timer interrupt bit.
> >
> > Introduce extension ID and SBI function ID for TIME extension.
> >
> > Implement only sbi_set_timer_v02() as there is not to much sense
> > to support earlier version and, at the moment, Xen supports only v02.
> Besides this somewhat contradicting the use of a function pointer: What
> about the legacy extension's equivalent?
I think this is not really needed, and the same implementation can be used for
both the Legacy and TIME extensions, since the API is identical and the only
difference is that|sbi_set_timer()| was moved into a separate extension.

Since Xen reports to the guest that it supports SBI v0.2, it is up to the guest
implementation to decide why it is still using|sbi_set_timer()| from the
Legacy extension instead of the TIME extension.

I think that I can add Legacy extension equivalent but considering that we are
using OpenSBI v0.2 for which Time extension is available it seems for me it is
enough to define sbi_set_timer to sbi_set_timer_v02() for now.

> > --- a/xen/arch/riscv/include/asm/sbi.h
> > +++ b/xen/arch/riscv/include/asm/sbi.h
> > @@ -33,6 +33,7 @@
> >   
> >   #define SBI_EXT_BASE                    0x10
> >   #define SBI_EXT_RFENCE                  0x52464E43
> > +#define SBI_EXT_TIME                    0x54494D45
> >   
> >   /* SBI function IDs for BASE extension */
> >   #define SBI_EXT_BASE_GET_SPEC_VERSION   0x0
> > @@ -65,6 +66,9 @@
> >   
> >   #define SBI_SPEC_VERSION_DEFAULT 0x1
> >   
> > +/* SBI function IDs for TIME extension */
> > +#define SBI_EXT_TIME_SET_TIMER  0x0
> Move up besides the other SBI_EXT_* and use the same amount of padding?
Sure, I will do that.

> > @@ -138,6 +142,19 @@ int sbi_remote_hfence_gvma(const cpumask_t *cpu_mask, 
> > vaddr_t start,
> >   int sbi_remote_hfence_gvma_vmid(const cpumask_t *cpu_mask, vaddr_t start,
> >                                   size_t size, unsigned long vmid);
> >   
> > +/*
> > + * Programs the clock for next event after stime_value time. This function also
> > + * clears the pending timer interrupt bit.
> > + * If the supervisor wishes to clear the timer interrupt without scheduling the
> > + * next timer event, it can either request a timer interrupt infinitely far
> > + * into the future (i.e., (uint64_t)-1), or it can instead mask the timer
> > + * interrupt by clearing sie.STIE CSR bit.
> > + *
> > + * This SBI call returns 0 upon success or an implementation specific negative
> > + * error code.
> > + */
> > +extern int (*sbi_set_timer)(uint64_t stime_value);
> Despite the pretty extensive comment, the granularity of the value to be passed
> isn't mentioned.
I update the comment with the following then:
  The stime_value parameter represents absolute time measured in ticks.


> > --- a/xen/arch/riscv/sbi.c
> > +++ b/xen/arch/riscv/sbi.c
> > @@ -249,6 +249,26 @@ static int (* __ro_after_init sbi_rfence)(unsigned long 
> > fid,
> >                                             unsigned long arg4,
> >                                             unsigned long arg5);
> >   
> > +static int cf_check sbi_set_timer_v02(uint64_t stime_value)
> > +{
> > +    struct sbiret ret;
> > +
> > +#ifdef CONFIG_RISCV_64
> > +    ret = sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value, 0,
> > +                    0, 0, 0, 0);
> > +#else
> > +    ret = sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value,
> > +                    stime_value >> 32, 0, 0, 0, 0);
> > +#endif
> How about
>
>      ret = sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value,
> #ifdef CONFIG_RISCV_64
>                      0,
> #else
>                      stime_value >> 32,
> #endif
>                      0, 0, 0, 0);
>
> ? Granted some may say this looks a little m ore clumsy, but it's surely
> less redundancy.
>
> Also I'd suggest to use CONFIG_RISCV_32 with the #ifdef, as then the "else"
> covers both RV64 and RV128.
Makes sense, I will update the function in mentioned way.

> > +    if ( ret.error )
> > +        return sbi_err_map_xen_errno(ret.error);
> > +    else
> > +        return 0;
> > +}
> While I understand this is being debated, I continue to think that this
> kind of use of if/else isn't very helpful. Function's main return
> statements imo benefit from being unconditional.
Considering what returns sbi_err_map_xen_errno() we can just drop if/else
and have only:
  return sbi_err_map_xen_errno(ret.error);
as if ret.error == SBI_SUCCESS(0) then sbi_err_map_xen_errno() will
return 0.

Thanks.

~ Oleksii