Introduce a function pointer for sbi_set_timer(), since different OpenSBI
versions may implement the TIME extension with different extension IDs
and/or function IDs.
If the TIME extension is not available, fall back to the legacy timer
mechanism. This is useful when Xen runs as a guest under another Xen,
because the TIME extension is not currently virtualised and therefore
will not appear as available.
The sbi_set_timer() pointer will be used by reprogram_timer() to program
Xen’s physical timer as without SSTC extension there is no any other
option except SBI call to do that as only M-timer is available for us.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
- Move up defintion of SBI_EXT_TIME_SET_TIMER and use the same padding as
defintions around it.
- Add an extra comment about stime_value granuality above declaration of
sbi_set_timer function pointer.
- Refactor implemetation of sbi_set_timer_v02().
- Provide fallback for sbi_set_timer_v01().
- Update the commit message.
---
xen/arch/riscv/include/asm/sbi.h | 18 ++++++++++++++
xen/arch/riscv/sbi.c | 40 ++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+)
diff --git a/xen/arch/riscv/include/asm/sbi.h b/xen/arch/riscv/include/asm/sbi.h
index 79f7ff5c5501..e0e31d7afa20 100644
--- a/xen/arch/riscv/include/asm/sbi.h
+++ b/xen/arch/riscv/include/asm/sbi.h
@@ -29,6 +29,10 @@
#define SBI_EXT_BASE 0x10
#define SBI_EXT_RFENCE 0x52464E43
+#define SBI_EXT_TIME 0x54494D45
+
+/* SBI function IDs for TIME extension */
+#define SBI_EXT_TIME_SET_TIMER 0x0
/* SBI function IDs for BASE extension */
#define SBI_EXT_BASE_GET_SPEC_VERSION 0x0
@@ -134,6 +138,20 @@ 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.
+ * The stime_value parameter represents absolute time measured in ticks.
+ *
+ * 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..2c7757c8839f 100644
--- a/xen/arch/riscv/sbi.c
+++ b/xen/arch/riscv/sbi.c
@@ -249,6 +249,38 @@ 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;
+
+ ret = sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value,
+#ifdef CONFIG_RISCV_32
+ stime_value >> 32,
+#else
+ 0,
+#endif
+ 0, 0, 0, 0);
+
+ return sbi_err_map_xen_errno(ret.error);
+}
+
+static int cf_check sbi_set_timer_v01(uint64_t stime_value)
+{
+ struct sbiret ret;
+
+ ret = sbi_ecall(SBI_EXT_0_1_SET_TIMER, 0, stime_value,
+#ifdef CONFIG_RISCV_32
+ stime_value >> 32,
+#else
+ 0,
+#endif
+ 0, 0, 0, 0);
+
+ return sbi_err_map_xen_errno(ret.error);
+}
+
+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 +358,14 @@ 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
+ sbi_set_timer = sbi_set_timer_v01;
}
else
panic("Ooops. SBI spec version 0.1 detected. Need to add support");
--
2.52.0
On 22.01.2026 17:47, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/sbi.h
> +++ b/xen/arch/riscv/include/asm/sbi.h
> @@ -29,6 +29,10 @@
>
> #define SBI_EXT_BASE 0x10
> #define SBI_EXT_RFENCE 0x52464E43
> +#define SBI_EXT_TIME 0x54494D45
> +
> +/* SBI function IDs for TIME extension */
> +#define SBI_EXT_TIME_SET_TIMER 0x0
Nit: Do you really mean to have the time extension IDs above ...
> /* SBI function IDs for BASE extension */
> #define SBI_EXT_BASE_GET_SPEC_VERSION 0x0
... the base extension ones?
> @@ -134,6 +138,20 @@ 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.
> + * The stime_value parameter represents absolute time measured in ticks.
> + *
> + * This SBI call returns 0 upon success or an implementation specific negative
> + * error code.
> + */
> +extern int (*sbi_set_timer)(uint64_t stime_value);
__read_mostly or even __ro_after_init?
> @@ -326,6 +358,14 @@ 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");
Is this really relevant to log especially in release builds? IOW can this at
least be downgraded to dprintk()?
Jan
On 2/4/26 7:50 AM, Jan Beulich wrote:
> On 22.01.2026 17:47, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/include/asm/sbi.h
>> +++ b/xen/arch/riscv/include/asm/sbi.h
>> @@ -29,6 +29,10 @@
>>
>> #define SBI_EXT_BASE 0x10
>> #define SBI_EXT_RFENCE 0x52464E43
>> +#define SBI_EXT_TIME 0x54494D45
>> +
>> +/* SBI function IDs for TIME extension */
>> +#define SBI_EXT_TIME_SET_TIMER 0x0
> Nit: Do you really mean to have the time extension IDs above ...
>
>> /* SBI function IDs for BASE extension */
>> #define SBI_EXT_BASE_GET_SPEC_VERSION 0x0
> ... the base extension ones?
I will move it after SBI_EXT_RFENCE_REMOTE_HFENCE_VVMA_ASID to be aligned with how
extensions are declared.
>
>> @@ -134,6 +138,20 @@ 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.
>> + * The stime_value parameter represents absolute time measured in ticks.
>> + *
>> + * This SBI call returns 0 upon success or an implementation specific negative
>> + * error code.
>> + */
>> +extern int (*sbi_set_timer)(uint64_t stime_value);
> __read_mostly or even __ro_after_init?
I will add __ro_after_init to be in sync with sbi_rfence.
>
>> @@ -326,6 +358,14 @@ 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");
> Is this really relevant to log especially in release builds? IOW can this at
> least be downgraded to dprintk()?
Probably not, it could be useful for debugging to understand what kind and version
of extension is used. I am okay with using of dprintk().
Thanks.
~ Oleksii
On 22.01.2026 17:47, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/sbi.c
> +++ b/xen/arch/riscv/sbi.c
> @@ -249,6 +249,38 @@ 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;
> +
> + ret = sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value,
> +#ifdef CONFIG_RISCV_32
> + stime_value >> 32,
> +#else
> + 0,
> +#endif
> + 0, 0, 0, 0);
> +
> + return sbi_err_map_xen_errno(ret.error);
> +}
> +
> +static int cf_check sbi_set_timer_v01(uint64_t stime_value)
> +{
> + struct sbiret ret;
> +
> + ret = sbi_ecall(SBI_EXT_0_1_SET_TIMER, 0, stime_value,
From this name I'm judging version 0.1 is meant. How does this fit with ...
> @@ -326,6 +358,14 @@ 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
> + sbi_set_timer = sbi_set_timer_v01;
> }
> else
> panic("Ooops. SBI spec version 0.1 detected. Need to add support");
... the panic() here?
Jan
On 2/3/26 6:02 PM, Jan Beulich wrote:
> On 22.01.2026 17:47, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/sbi.c
>> +++ b/xen/arch/riscv/sbi.c
>> @@ -249,6 +249,38 @@ 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;
>> +
>> + ret = sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value,
>> +#ifdef CONFIG_RISCV_32
>> + stime_value >> 32,
>> +#else
>> + 0,
>> +#endif
>> + 0, 0, 0, 0);
>> +
>> + return sbi_err_map_xen_errno(ret.error);
>> +}
>> +
>> +static int cf_check sbi_set_timer_v01(uint64_t stime_value)
>> +{
>> + struct sbiret ret;
>> +
>> + ret = sbi_ecall(SBI_EXT_0_1_SET_TIMER, 0, stime_value,
> From this name I'm judging version 0.1 is meant. How does this fit with ...
>
>> @@ -326,6 +358,14 @@ 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
>> + sbi_set_timer = sbi_set_timer_v01;
>> }
>> else
>> panic("Ooops. SBI spec version 0.1 detected. Need to add support");
> ... the panic() here?
panic() is still here as then we will want to add support for rfence v01 SBI call
too what hasn't been done yet.
Could it be an option to change panic to:
sbi_set_timer = sbi_set_timer_v01;
dprintk("SBI v0.1 isn't fully supported\n");
and then add sbi_rfence = sbi_rfence_v01 when i will be first used?
~ Oleksii
On 04.02.2026 10:29, Oleksii Kurochko wrote:
> On 2/3/26 6:02 PM, Jan Beulich wrote:
>> On 22.01.2026 17:47, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/sbi.c
>>> +++ b/xen/arch/riscv/sbi.c
>>> @@ -249,6 +249,38 @@ 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;
>>> +
>>> + ret = sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value,
>>> +#ifdef CONFIG_RISCV_32
>>> + stime_value >> 32,
>>> +#else
>>> + 0,
>>> +#endif
>>> + 0, 0, 0, 0);
>>> +
>>> + return sbi_err_map_xen_errno(ret.error);
>>> +}
>>> +
>>> +static int cf_check sbi_set_timer_v01(uint64_t stime_value)
>>> +{
>>> + struct sbiret ret;
>>> +
>>> + ret = sbi_ecall(SBI_EXT_0_1_SET_TIMER, 0, stime_value,
>> From this name I'm judging version 0.1 is meant. How does this fit with ...
>>
>>> @@ -326,6 +358,14 @@ 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
>>> + sbi_set_timer = sbi_set_timer_v01;
>>> }
>>> else
>>> panic("Ooops. SBI spec version 0.1 detected. Need to add support");
>> ... the panic() here?
>
> panic() is still here as then we will want to add support for rfence v01 SBI call
> too what hasn't been done yet.
>
> Could it be an option to change panic to:
> sbi_set_timer = sbi_set_timer_v01;
> dprintk("SBI v0.1 isn't fully supported\n");
> and then add sbi_rfence = sbi_rfence_v01 when i will be first used?
I don't mind keeping the panic(), but what you add here wants to be structured
such that things won't need moving around once the panic() goes away. I.e. you
want to avoid dealing with v0.1 in both the if() and the else. To accommodate
that, perhaps sbi_set_timer_v01 could simply be the initializer of the new
function pointer variable?
Beyond that, once again you want to clarify things in the commit message.
Adding support for a case which elsewhere you panic() on is, well, confusing
without some explanation.
Jan
© 2016 - 2026 Red Hat, Inc.