Enable Xen to handle timer reprogramming on RISC-V using
standard SBI calls.
Add a RISC-V implementation of reprogram_timer() to replace the stub:
- Re-enable the function previously stubbed in stubs.c.
- Use sbi_set_timer() to program the timer for the given timeout.
- Disable the timer when timeout == 0 by clearing the SIE.STIE bit.
- Calculate the deadline based on the current boot clock cycle count
and timer ticks.
- Ensure correct behavior when the deadline is already passed.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
xen/arch/riscv/stubs.c | 5 -----
xen/arch/riscv/time.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index 68ee859ca1a8..d120274af2fe 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -21,11 +21,6 @@ nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
/* time.c */
-int reprogram_timer(s_time_t timeout)
-{
- BUG_ON("unimplemented");
-}
-
void send_timer_event(struct vcpu *v)
{
BUG_ON("unimplemented");
diff --git a/xen/arch/riscv/time.c b/xen/arch/riscv/time.c
index e962f8518d78..53ba1cfb4a99 100644
--- a/xen/arch/riscv/time.c
+++ b/xen/arch/riscv/time.c
@@ -4,8 +4,12 @@
#include <xen/init.h>
#include <xen/lib.h>
#include <xen/sections.h>
+#include <xen/time.h>
#include <xen/types.h>
+#include <asm/csr.h>
+#include <asm/sbi.h>
+
unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */
uint64_t __ro_after_init boot_clock_cycles;
@@ -39,6 +43,33 @@ static void __init preinit_dt_xen_time(void)
cpu_khz = rate / 1000;
}
+int reprogram_timer(s_time_t timeout)
+{
+ uint64_t deadline, now;
+ int rc;
+
+ if ( timeout == 0 )
+ {
+ /* Disable timers */
+ csr_clear(CSR_SIE, BIT(IRQ_S_TIMER, UL));
+
+ return 1;
+ }
+
+ deadline = ns_to_ticks(timeout) + boot_clock_cycles;
+ now = get_cycles();
+ if ( deadline <= now )
+ return 0;
+
+ /* Enable timer */
+ csr_set(CSR_SIE, BIT(IRQ_S_TIMER, UL));
+
+ if ( (rc = sbi_set_timer(deadline)) )
+ panic("%s: timer wasn't set because: %d\n", __func__, rc);
+
+ return 1;
+}
+
void __init preinit_xen_time(void)
{
if ( acpi_disabled )
--
2.52.0On 24.12.2025 18:03, Oleksii Kurochko wrote:
> @@ -39,6 +43,33 @@ static void __init preinit_dt_xen_time(void)
> cpu_khz = rate / 1000;
> }
>
> +int reprogram_timer(s_time_t timeout)
> +{
> + uint64_t deadline, now;
> + int rc;
> +
> + if ( timeout == 0 )
> + {
> + /* Disable timers */
> + csr_clear(CSR_SIE, BIT(IRQ_S_TIMER, UL));
> +
> + return 1;
> + }
> +
> + deadline = ns_to_ticks(timeout) + boot_clock_cycles;
> + now = get_cycles();
> + if ( deadline <= now )
> + return 0;
> +
> + /* Enable timer */
> + csr_set(CSR_SIE, BIT(IRQ_S_TIMER, UL));
Still learning RISC-V, so question for my understanding: Even if the timeout
is short enough to expire before the one SIE bit will be set, the interrupt
will still occur (effectively immediately)? (Else the bit may need setting
first.)
> + if ( (rc = sbi_set_timer(deadline)) )
> + panic("%s: timer wasn't set because: %d\n", __func__, rc);
Hmm, if this function ends up being used from any guest accessible path (e.g.
a hypercall), such panic()-ing better shouldn't be there.
> + return 1;
> +}
Finally, before we get yet another instance of this de-fact boolean function:
Wouldn't we better globally switch it to be properly "bool" first?
Jan
On 1/12/26 4:24 PM, Jan Beulich wrote:
> On 24.12.2025 18:03, Oleksii Kurochko wrote:
>> @@ -39,6 +43,33 @@ static void __init preinit_dt_xen_time(void)
>> cpu_khz = rate / 1000;
>> }
>>
>> +int reprogram_timer(s_time_t timeout)
>> +{
>> + uint64_t deadline, now;
>> + int rc;
>> +
>> + if ( timeout == 0 )
>> + {
>> + /* Disable timers */
>> + csr_clear(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>> +
>> + return 1;
>> + }
>> +
>> + deadline = ns_to_ticks(timeout) + boot_clock_cycles;
>> + now = get_cycles();
>> + if ( deadline <= now )
>> + return 0;
>> +
>> + /* Enable timer */
>> + csr_set(CSR_SIE, BIT(IRQ_S_TIMER, UL));
> Still learning RISC-V, so question for my understanding: Even if the timeout
> is short enough to expire before the one SIE bit will be set, the interrupt
> will still occur (effectively immediately)? (Else the bit may need setting
> first.)
The interrupt will become pending first (when mtime >= mtimecmp or
mtime >= CSR_STIMECMP in case of SSTC) and then fire immediately once
|SIE.STIE |(and global|SIE|) are enabled.
>
>> + if ( (rc = sbi_set_timer(deadline)) )
>> + panic("%s: timer wasn't set because: %d\n", __func__, rc);
> Hmm, if this function ends up being used from any guest accessible path (e.g.
> a hypercall), such panic()-ing better shouldn't be there.
I don't have such use cases now and I don't expect that guest should use
this function.
>
>> + return 1;
>> +}
> Finally, before we get yet another instance of this de-fact boolean function:
> Wouldn't we better globally switch it to be properly "bool" first?
We could do that, I will prepare a separate patch in the next version of
this patch series.
Thanks.
~ Oleksii
On 13.01.2026 17:50, Oleksii Kurochko wrote:
> On 1/12/26 4:24 PM, Jan Beulich wrote:
>> On 24.12.2025 18:03, Oleksii Kurochko wrote:
>>> @@ -39,6 +43,33 @@ static void __init preinit_dt_xen_time(void)
>>> cpu_khz = rate / 1000;
>>> }
>>>
>>> +int reprogram_timer(s_time_t timeout)
>>> +{
>>> + uint64_t deadline, now;
>>> + int rc;
>>> +
>>> + if ( timeout == 0 )
>>> + {
>>> + /* Disable timers */
>>> + csr_clear(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>>> +
>>> + return 1;
>>> + }
>>> +
>>> + deadline = ns_to_ticks(timeout) + boot_clock_cycles;
>>> + now = get_cycles();
>>> + if ( deadline <= now )
>>> + return 0;
>>> +
>>> + /* Enable timer */
>>> + csr_set(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>> Still learning RISC-V, so question for my understanding: Even if the timeout
>> is short enough to expire before the one SIE bit will be set, the interrupt
>> will still occur (effectively immediately)? (Else the bit may need setting
>> first.)
>
> The interrupt will become pending first (when mtime >= mtimecmp or
> mtime >= CSR_STIMECMP in case of SSTC) and then fire immediately once
> |SIE.STIE |(and global|SIE|) are enabled.
>
>>
>>> + if ( (rc = sbi_set_timer(deadline)) )
>>> + panic("%s: timer wasn't set because: %d\n", __func__, rc);
>> Hmm, if this function ends up being used from any guest accessible path (e.g.
>> a hypercall), such panic()-ing better shouldn't be there.
>
> I don't have such use cases now and I don't expect that guest should use
> this function.
How do you envision supporting e.g. VCPUOP_set_singleshot_timer without
involving this function?
Jan
On 1/14/26 10:13 AM, Jan Beulich wrote:
> On 13.01.2026 17:50, Oleksii Kurochko wrote:
>> On 1/12/26 4:24 PM, Jan Beulich wrote:
>>> On 24.12.2025 18:03, Oleksii Kurochko wrote:
>>>> @@ -39,6 +43,33 @@ static void __init preinit_dt_xen_time(void)
>>>> cpu_khz = rate / 1000;
>>>> }
>>>>
>>>> +int reprogram_timer(s_time_t timeout)
>>>> +{
>>>> + uint64_t deadline, now;
>>>> + int rc;
>>>> +
>>>> + if ( timeout == 0 )
>>>> + {
>>>> + /* Disable timers */
>>>> + csr_clear(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>>>> +
>>>> + return 1;
>>>> + }
>>>> +
>>>> + deadline = ns_to_ticks(timeout) + boot_clock_cycles;
>>>> + now = get_cycles();
>>>> + if ( deadline <= now )
>>>> + return 0;
>>>> +
>>>> + /* Enable timer */
>>>> + csr_set(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>>> Still learning RISC-V, so question for my understanding: Even if the timeout
>>> is short enough to expire before the one SIE bit will be set, the interrupt
>>> will still occur (effectively immediately)? (Else the bit may need setting
>>> first.)
>> The interrupt will become pending first (when mtime >= mtimecmp or
>> mtime >= CSR_STIMECMP in case of SSTC) and then fire immediately once
>> |SIE.STIE |(and global|SIE|) are enabled.
>>
>>>> + if ( (rc = sbi_set_timer(deadline)) )
>>>> + panic("%s: timer wasn't set because: %d\n", __func__, rc);
>>> Hmm, if this function ends up being used from any guest accessible path (e.g.
>>> a hypercall), such panic()-ing better shouldn't be there.
>> I don't have such use cases now and I don't expect that guest should use
>> this function.
> How do you envision supporting e.g. VCPUOP_set_singleshot_timer without
> involving this function?
Looking at what is in common code for VCPUOP_set_singleshot_timer, it doesn't
use reprogram_timer(), it is just activate/deactivate timer.
~ Oleksii
On 14.01.2026 10:41, Oleksii Kurochko wrote:
>
> On 1/14/26 10:13 AM, Jan Beulich wrote:
>> On 13.01.2026 17:50, Oleksii Kurochko wrote:
>>> On 1/12/26 4:24 PM, Jan Beulich wrote:
>>>> On 24.12.2025 18:03, Oleksii Kurochko wrote:
>>>>> @@ -39,6 +43,33 @@ static void __init preinit_dt_xen_time(void)
>>>>> cpu_khz = rate / 1000;
>>>>> }
>>>>>
>>>>> +int reprogram_timer(s_time_t timeout)
>>>>> +{
>>>>> + uint64_t deadline, now;
>>>>> + int rc;
>>>>> +
>>>>> + if ( timeout == 0 )
>>>>> + {
>>>>> + /* Disable timers */
>>>>> + csr_clear(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>>>>> +
>>>>> + return 1;
>>>>> + }
>>>>> +
>>>>> + deadline = ns_to_ticks(timeout) + boot_clock_cycles;
>>>>> + now = get_cycles();
>>>>> + if ( deadline <= now )
>>>>> + return 0;
>>>>> +
>>>>> + /* Enable timer */
>>>>> + csr_set(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>>>> Still learning RISC-V, so question for my understanding: Even if the timeout
>>>> is short enough to expire before the one SIE bit will be set, the interrupt
>>>> will still occur (effectively immediately)? (Else the bit may need setting
>>>> first.)
>>> The interrupt will become pending first (when mtime >= mtimecmp or
>>> mtime >= CSR_STIMECMP in case of SSTC) and then fire immediately once
>>> |SIE.STIE |(and global|SIE|) are enabled.
>>>
>>>>> + if ( (rc = sbi_set_timer(deadline)) )
>>>>> + panic("%s: timer wasn't set because: %d\n", __func__, rc);
>>>> Hmm, if this function ends up being used from any guest accessible path (e.g.
>>>> a hypercall), such panic()-ing better shouldn't be there.
>>> I don't have such use cases now and I don't expect that guest should use
>>> this function.
>> How do you envision supporting e.g. VCPUOP_set_singleshot_timer without
>> involving this function?
>
> Looking at what is in common code for VCPUOP_set_singleshot_timer, it doesn't
> use reprogram_timer(), it is just activate/deactivate timer.
And how would that work without, eventually, using reprogram_timer()? While not
directly on a hypercall path, the use can still be guest-induced.
Jan
On 1/14/26 10:53 AM, Jan Beulich wrote:
> On 14.01.2026 10:41, Oleksii Kurochko wrote:
>> On 1/14/26 10:13 AM, Jan Beulich wrote:
>>> On 13.01.2026 17:50, Oleksii Kurochko wrote:
>>>> On 1/12/26 4:24 PM, Jan Beulich wrote:
>>>>> On 24.12.2025 18:03, Oleksii Kurochko wrote:
>>>>>> @@ -39,6 +43,33 @@ static void __init preinit_dt_xen_time(void)
>>>>>> cpu_khz = rate / 1000;
>>>>>> }
>>>>>>
>>>>>> +int reprogram_timer(s_time_t timeout)
>>>>>> +{
>>>>>> + uint64_t deadline, now;
>>>>>> + int rc;
>>>>>> +
>>>>>> + if ( timeout == 0 )
>>>>>> + {
>>>>>> + /* Disable timers */
>>>>>> + csr_clear(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>>>>>> +
>>>>>> + return 1;
>>>>>> + }
>>>>>> +
>>>>>> + deadline = ns_to_ticks(timeout) + boot_clock_cycles;
>>>>>> + now = get_cycles();
>>>>>> + if ( deadline <= now )
>>>>>> + return 0;
>>>>>> +
>>>>>> + /* Enable timer */
>>>>>> + csr_set(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>>>>> Still learning RISC-V, so question for my understanding: Even if the timeout
>>>>> is short enough to expire before the one SIE bit will be set, the interrupt
>>>>> will still occur (effectively immediately)? (Else the bit may need setting
>>>>> first.)
>>>> The interrupt will become pending first (when mtime >= mtimecmp or
>>>> mtime >= CSR_STIMECMP in case of SSTC) and then fire immediately once
>>>> |SIE.STIE |(and global|SIE|) are enabled.
>>>>
>>>>>> + if ( (rc = sbi_set_timer(deadline)) )
>>>>>> + panic("%s: timer wasn't set because: %d\n", __func__, rc);
>>>>> Hmm, if this function ends up being used from any guest accessible path (e.g.
>>>>> a hypercall), such panic()-ing better shouldn't be there.
>>>> I don't have such use cases now and I don't expect that guest should use
>>>> this function.
>>> How do you envision supporting e.g. VCPUOP_set_singleshot_timer without
>>> involving this function?
>> Looking at what is in common code for VCPUOP_set_singleshot_timer, it doesn't
>> use reprogram_timer(), it is just activate/deactivate timer.
> And how would that work without, eventually, using reprogram_timer()? While not
> directly on a hypercall path, the use can still be guest-induced.
Of course, eventually|reprogram_timer()| will be used. I incorrectly thought
that we were talking about its direct use on the hypercall path.
I am not really sure what we should do in the case when rc != 0. Looking at the
OpenSBI call, it always returns 0, except when sbi_set_timer() is not supported,
in which case it returns -SBI_ENOTSUPP. With such a return value, I think it would
be acceptable to call domain_crash(current->domain). On the other hand, if some
other negative error code is returned, it might be better to return 0 and simply
allow the timer programming to be retried later.
However, if we look at the comments for other architectures, the meaning of a
return value of 0 from this function is:
Returns 1 on success; 0 if the timeout is too soon or is in the past.
In that case, it becomes difficult to distinguish whether 0 was returned due to
an error or because the timeout was too soon or already in the past.
It seems like at the moment it is better to call domain_crash() and change it
if it will be necessity in the future as I expect that the only negative code
which will be returned by sbi_set_timer() will -SBI_ENOTSUPP and if this SBI
call isn't supported then we anyway need a different way to set a timer.
~ Oleksii
On 14.01.2026 11:33, Oleksii Kurochko wrote:
>
> On 1/14/26 10:53 AM, Jan Beulich wrote:
>> On 14.01.2026 10:41, Oleksii Kurochko wrote:
>>> On 1/14/26 10:13 AM, Jan Beulich wrote:
>>>> On 13.01.2026 17:50, Oleksii Kurochko wrote:
>>>>> On 1/12/26 4:24 PM, Jan Beulich wrote:
>>>>>> On 24.12.2025 18:03, Oleksii Kurochko wrote:
>>>>>>> @@ -39,6 +43,33 @@ static void __init preinit_dt_xen_time(void)
>>>>>>> cpu_khz = rate / 1000;
>>>>>>> }
>>>>>>>
>>>>>>> +int reprogram_timer(s_time_t timeout)
>>>>>>> +{
>>>>>>> + uint64_t deadline, now;
>>>>>>> + int rc;
>>>>>>> +
>>>>>>> + if ( timeout == 0 )
>>>>>>> + {
>>>>>>> + /* Disable timers */
>>>>>>> + csr_clear(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>>>>>>> +
>>>>>>> + return 1;
>>>>>>> + }
>>>>>>> +
>>>>>>> + deadline = ns_to_ticks(timeout) + boot_clock_cycles;
>>>>>>> + now = get_cycles();
>>>>>>> + if ( deadline <= now )
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> + /* Enable timer */
>>>>>>> + csr_set(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>>>>>> Still learning RISC-V, so question for my understanding: Even if the timeout
>>>>>> is short enough to expire before the one SIE bit will be set, the interrupt
>>>>>> will still occur (effectively immediately)? (Else the bit may need setting
>>>>>> first.)
>>>>> The interrupt will become pending first (when mtime >= mtimecmp or
>>>>> mtime >= CSR_STIMECMP in case of SSTC) and then fire immediately once
>>>>> |SIE.STIE |(and global|SIE|) are enabled.
>>>>>
>>>>>>> + if ( (rc = sbi_set_timer(deadline)) )
>>>>>>> + panic("%s: timer wasn't set because: %d\n", __func__, rc);
>>>>>> Hmm, if this function ends up being used from any guest accessible path (e.g.
>>>>>> a hypercall), such panic()-ing better shouldn't be there.
>>>>> I don't have such use cases now and I don't expect that guest should use
>>>>> this function.
>>>> How do you envision supporting e.g. VCPUOP_set_singleshot_timer without
>>>> involving this function?
>>> Looking at what is in common code for VCPUOP_set_singleshot_timer, it doesn't
>>> use reprogram_timer(), it is just activate/deactivate timer.
>> And how would that work without, eventually, using reprogram_timer()? While not
>> directly on a hypercall path, the use can still be guest-induced.
>
> Of course, eventually|reprogram_timer()| will be used. I incorrectly thought
> that we were talking about its direct use on the hypercall path.
>
> I am not really sure what we should do in the case when rc != 0. Looking at the
> OpenSBI call, it always returns 0, except when sbi_set_timer() is not supported,
> in which case it returns -SBI_ENOTSUPP. With such a return value, I think it would
> be acceptable to call domain_crash(current->domain).
How is current->domain related to a failure in reprogram_timer()?
> On the other hand, if some
> other negative error code is returned, it might be better to return 0 and simply
> allow the timer programming to be retried later.
> However, if we look at the comments for other architectures, the meaning of a
> return value of 0 from this function is:
> Returns 1 on success; 0 if the timeout is too soon or is in the past.
> In that case, it becomes difficult to distinguish whether 0 was returned due to
> an error or because the timeout was too soon or already in the past.
Well, your problem is that neither Arm nor x86 can actually fail. Hence
calling code isn't presently prepared for that. With panic() (and hence
also BUG()) and domain_crash() ruled out, maybe generic infrastructure
needs touching first (in a different way than making the function's return
type "bool")?
Jan
On 1/14/26 12:17 PM, Jan Beulich wrote:
> On 14.01.2026 11:33, Oleksii Kurochko wrote:
>> On 1/14/26 10:53 AM, Jan Beulich wrote:
>>> On 14.01.2026 10:41, Oleksii Kurochko wrote:
>>>> On 1/14/26 10:13 AM, Jan Beulich wrote:
>>>>> On 13.01.2026 17:50, Oleksii Kurochko wrote:
>>>>>> On 1/12/26 4:24 PM, Jan Beulich wrote:
>>>>>>> On 24.12.2025 18:03, Oleksii Kurochko wrote:
>>>>>>>> @@ -39,6 +43,33 @@ static void __init preinit_dt_xen_time(void)
>>>>>>>> cpu_khz = rate / 1000;
>>>>>>>> }
>>>>>>>>
>>>>>>>> +int reprogram_timer(s_time_t timeout)
>>>>>>>> +{
>>>>>>>> + uint64_t deadline, now;
>>>>>>>> + int rc;
>>>>>>>> +
>>>>>>>> + if ( timeout == 0 )
>>>>>>>> + {
>>>>>>>> + /* Disable timers */
>>>>>>>> + csr_clear(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>>>>>>>> +
>>>>>>>> + return 1;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + deadline = ns_to_ticks(timeout) + boot_clock_cycles;
>>>>>>>> + now = get_cycles();
>>>>>>>> + if ( deadline <= now )
>>>>>>>> + return 0;
>>>>>>>> +
>>>>>>>> + /* Enable timer */
>>>>>>>> + csr_set(CSR_SIE, BIT(IRQ_S_TIMER, UL));
>>>>>>> Still learning RISC-V, so question for my understanding: Even if the timeout
>>>>>>> is short enough to expire before the one SIE bit will be set, the interrupt
>>>>>>> will still occur (effectively immediately)? (Else the bit may need setting
>>>>>>> first.)
>>>>>> The interrupt will become pending first (when mtime >= mtimecmp or
>>>>>> mtime >= CSR_STIMECMP in case of SSTC) and then fire immediately once
>>>>>> |SIE.STIE |(and global|SIE|) are enabled.
>>>>>>
>>>>>>>> + if ( (rc = sbi_set_timer(deadline)) )
>>>>>>>> + panic("%s: timer wasn't set because: %d\n", __func__, rc);
>>>>>>> Hmm, if this function ends up being used from any guest accessible path (e.g.
>>>>>>> a hypercall), such panic()-ing better shouldn't be there.
>>>>>> I don't have such use cases now and I don't expect that guest should use
>>>>>> this function.
>>>>> How do you envision supporting e.g. VCPUOP_set_singleshot_timer without
>>>>> involving this function?
>>>> Looking at what is in common code for VCPUOP_set_singleshot_timer, it doesn't
>>>> use reprogram_timer(), it is just activate/deactivate timer.
>>> And how would that work without, eventually, using reprogram_timer()? While not
>>> directly on a hypercall path, the use can still be guest-induced.
>> Of course, eventually|reprogram_timer()| will be used. I incorrectly thought
>> that we were talking about its direct use on the hypercall path.
>>
>> I am not really sure what we should do in the case when rc != 0. Looking at the
>> OpenSBI call, it always returns 0, except when sbi_set_timer() is not supported,
>> in which case it returns -SBI_ENOTSUPP. With such a return value, I think it would
>> be acceptable to call domain_crash(current->domain).
> How is current->domain related to a failure in reprogram_timer()?
Agree, it isn't, a failure could happen during a ran of any domain.
>
>> On the other hand, if some
>> other negative error code is returned, it might be better to return 0 and simply
>> allow the timer programming to be retried later.
>> However, if we look at the comments for other architectures, the meaning of a
>> return value of 0 from this function is:
>> Returns 1 on success; 0 if the timeout is too soon or is in the past.
>> In that case, it becomes difficult to distinguish whether 0 was returned due to
>> an error or because the timeout was too soon or already in the past.
> Well, your problem is that neither Arm nor x86 can actually fail. Hence
> calling code isn't presently prepared for that. With panic() (and hence
> also BUG()) and domain_crash() ruled out, maybe generic infrastructure
> needs touching first (in a different way than making the function's return
> type "bool")?
I think making the function's return still is fine and it is only question to
arch-specific reprogram_timer() what to do when an error happens.
Still doesn't clear to me what should be a reaction on failure of
reprogram_timer().
Considering that SBI spec doesn't specify a list of possible errors and now
the only possible error is -ENOSUPP it seems to me it is fine
to have panic() as we don't have any other mechanism to set a timer
except SBI call (except the case SSTC is supported then we can use just
supervisor timer register directly without SBI call).
~ Oleksii
On 14.01.2026 13:41, Oleksii Kurochko wrote: > On 1/14/26 12:17 PM, Jan Beulich wrote: >> On 14.01.2026 11:33, Oleksii Kurochko wrote: >>> On the other hand, if some >>> other negative error code is returned, it might be better to return 0 and simply >>> allow the timer programming to be retried later. >>> However, if we look at the comments for other architectures, the meaning of a >>> return value of 0 from this function is: >>> Returns 1 on success; 0 if the timeout is too soon or is in the past. >>> In that case, it becomes difficult to distinguish whether 0 was returned due to >>> an error or because the timeout was too soon or already in the past. >> Well, your problem is that neither Arm nor x86 can actually fail. Hence >> calling code isn't presently prepared for that. With panic() (and hence >> also BUG()) and domain_crash() ruled out, maybe generic infrastructure >> needs touching first (in a different way than making the function's return >> type "bool")? > > I think making the function's return still is fine and it is only question to > arch-specific reprogram_timer() what to do when an error happens. > > Still doesn't clear to me what should be a reaction on failure of > reprogram_timer(). > Considering that SBI spec doesn't specify a list of possible errors and now > the only possible error is -ENOSUPP it seems to me it is fine > to have panic() as we don't have any other mechanism to set a timer > except SBI call panic() (or BUG_ON()) is pretty drastic a measure when possibly the system could be kept alive. If is pretty certain that future SBI timer calls also aren't going to work, then I'd agree that panic()ing might be appropriate. If otoh a subsequent call might work, a less heavyweight action would seem preferable. (Welcome to the funs of relying on lower-level software.) > (except the case SSTC is supported then we can use just > supervisor timer register directly without SBI call). So maybe a good first step would be to use that extension if available? Might even think about requiring it for the time being ... Jan
On 1/14/26 4:04 PM, Jan Beulich wrote: > On 14.01.2026 13:41, Oleksii Kurochko wrote: >> On 1/14/26 12:17 PM, Jan Beulich wrote: >>> On 14.01.2026 11:33, Oleksii Kurochko wrote: >>>> On the other hand, if some >>>> other negative error code is returned, it might be better to return 0 and simply >>>> allow the timer programming to be retried later. >>>> However, if we look at the comments for other architectures, the meaning of a >>>> return value of 0 from this function is: >>>> Returns 1 on success; 0 if the timeout is too soon or is in the past. >>>> In that case, it becomes difficult to distinguish whether 0 was returned due to >>>> an error or because the timeout was too soon or already in the past. >>> Well, your problem is that neither Arm nor x86 can actually fail. Hence >>> calling code isn't presently prepared for that. With panic() (and hence >>> also BUG()) and domain_crash() ruled out, maybe generic infrastructure >>> needs touching first (in a different way than making the function's return >>> type "bool")? >> I think making the function's return still is fine and it is only question to >> arch-specific reprogram_timer() what to do when an error happens. >> >> Still doesn't clear to me what should be a reaction on failure of >> reprogram_timer(). >> Considering that SBI spec doesn't specify a list of possible errors and now >> the only possible error is -ENOSUPP it seems to me it is fine >> to have panic() as we don't have any other mechanism to set a timer >> except SBI call > panic() (or BUG_ON()) is pretty drastic a measure when possibly the system > could be kept alive. If is pretty certain that future SBI timer calls also > aren't going to work, then I'd agree that panic()ing might be appropriate. > If otoh a subsequent call might work, a less heavyweight action would seem > preferable. (Welcome to the funs of relying on lower-level software.) I don’t know how OpenSBI will be updated in the future, but looking at the current situation, since SBI timer calls have existed from very early OpenSBI versions up to the latest ones, repeatedly issuing the SBI timer call will always return the same negative error code indicating that the timer call is not supported. I can add a comment above panic(), or include an explanation in the commit message. > >> (except the case SSTC is supported then we can use just >> supervisor timer register directly without SBI call). > So maybe a good first step would be to use that extension if available? > Might even think about requiring it for the time being ... I initially started working on SSTC support, but then stopped because an almost production-ready board to which I do have indirect access does not support this extension. As a result, I decided to proceed with SBI approach as it covers both cases when SSTC is available and when no. ~ Oleksii
© 2016 - 2026 Red Hat, Inc.