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.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
xen/arch/riscv/include/asm/sbi.h | 17 +++++++++++++++++
xen/arch/riscv/sbi.c | 26 ++++++++++++++++++++++++++
2 files changed, 43 insertions(+)
diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
index a88d3d57127a..c54dc7642ff1 100644
--- 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
+
struct sbiret {
long error;
long value;
@@ -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);
+
/*
* Initialize SBI library
*
diff --git a/xen/arch/riscv/sbi.c b/xen/arch/riscv/sbi.c
index 425dce44c679..206ea3462c50 100644
--- 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
+
+ if ( ret.error )
+ return sbi_err_map_xen_errno(ret.error);
+ else
+ return 0;
+}
+
+int (* __ro_after_init sbi_set_timer)(uint64_t stime_value);
+
int sbi_remote_sfence_vma(const cpumask_t *cpu_mask, vaddr_t start,
size_t size)
{
@@ -326,6 +346,12 @@ int __init sbi_init(void)
sbi_rfence = sbi_rfence_v02;
printk("SBI v0.2 RFENCE extension detected\n");
}
+
+ if ( sbi_probe_extension(SBI_EXT_TIME) > 0 )
+ {
+ sbi_set_timer = sbi_set_timer_v02;
+ printk("SBI v0.2 TIME extension detected\n");
+ }
}
else
panic("Ooops. SBI spec version 0.1 detected. Need to add support");
--
2.52.0On 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?
> --- 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?
> @@ -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.
> --- 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.
> + 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.
Jan
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
On 13.01.2026 17:33, Oleksii Kurochko wrote: > 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. Feels like here you're negating what just before you wrote in reply to 10/15. IOW - I'm now sufficiently confused. (Just consider if you ran Xen itself as a guest of the very same Xen. From what you said for 10/15, it would end up not seeing the TIME extension as available, hence would need a fallback to the Legacy one.) Jan
On 1/14/26 10:07 AM, Jan Beulich wrote: > On 13.01.2026 17:33, Oleksii Kurochko wrote: >> 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. > Feels like here you're negating what just before you wrote in reply to 10/15. > IOW - I'm now sufficiently confused. (Just consider if you ran Xen itself as > a guest of the very same Xen. From what you said for 10/15, it would end up > not seeing the TIME extension as available, hence would need a fallback to > the Legacy one. You are right, a fallback to the Legacy one should be provided for such cases. Actually, it is why Linux could be ran now as it has its fallback too. ~ Oleksii
© 2016 - 2026 Red Hat, Inc.