Re: [PATCH v1 13/15] xen/riscv: implement reprogram_timer() using SBI

[Jan Beulich <jbeulich@xxxxxxxx>]

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