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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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