Re: [PATCH v1 08/15] xen/riscv: introduce vtimer_set_timer() and vtimer_expired()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/8/26 11:28 AM, Jan Beulich wrote:
> On 24.12.2025 18:03, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/vtimer.h
> > +++ b/xen/arch/riscv/include/asm/vtimer.h
> > @@ -22,4 +22,6 @@ void vcpu_timer_destroy(struct vcpu *v);
> >   
> >   int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config);
> >   
> > +void vtimer_set_timer(struct vtimer *t, const uint64_t ticks);
> > +
> >   #endif /* ASM__RISCV__VTIMER_H */
> > diff --git a/xen/arch/riscv/vtimer.c b/xen/arch/riscv/vtimer.c
> > index 5ba533690bc2..99a0c5986f1d 100644
> > --- a/xen/arch/riscv/vtimer.c
> > +++ b/xen/arch/riscv/vtimer.c
> > @@ -1,6 +1,8 @@
> >   /* SPDX-License-Identifier: GPL-2.0-only */
> >   
> > +#include <xen/domain.h>
> Is this really needed, when ...
>
> >   #include <xen/sched.h>
> ... this is already there?
With the way how includes look in xen/sched.h - no.

> > +#include <xen/time.h>
> Don't you mean xen/timer.h here?
You are right, it should be xen/timer.h as set_timer(), stop_timer() and 
migrate_timer()
are from xen/timer.h.

> > @@ -15,7 +17,9 @@ int domain_vtimer_init(struct domain *d, struct 
> > xen_arch_domainconfig *config)
> >   
> >   static void vtimer_expired(void *data)
> >   {
> > -    panic("%s: TBD\n", __func__);
> > +    struct vtimer *t = data;
> Pointer-to-const please.
>
> > @@ -37,3 +41,27 @@ void vcpu_timer_destroy(struct vcpu *v)
> >   
> >       kill_timer(&v->arch.vtimer.timer);
> >   }
> > +
> > +void vtimer_set_timer(struct vtimer *t, const uint64_t ticks)
> > +{
> > +    s_time_t expires = ticks_to_ns(ticks - boot_clock_cycles);
> boot_clock_cycles is known to just Xen. If the guest provided input is an
> absolute value, how would that work across migration? Doesn't there need
> to be a guest-specific bias instead?
I think that I don't understand fully your questions, but it sounds like it is 
a job
for htimedelta register.

> > +    vcpu_unset_interrupt(t->v, IRQ_VS_TIMER);
> > +
> > +    /*
> > +     * According to the RISC-V sbi spec:
> > +     *   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.
> > +     */
> And SBI is the only way to set the expiry value? No CSR access? (Question
> also concerns the unconditional vcpu_unset_interrupt() above.)
If we don't have SSTC extension support then I suppose yes, as CSR_MI{E,P} could
be accessed only from M-mode:
 (code from OpenSBI)
void sbi_timer_event_start(u64 next_event)
{
        sbi_pmu_ctr_incr_fw(SBI_PMU_FW_SET_TIMER);

        /**
         * Update the stimecmp directly if available. This allows
         * the older software to leverage sstc extension on newer hardware.
         */
        if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(), 
SBI_HART_EXT_SSTC)) {
#if __riscv_xlen == 32
                csr_write(CSR_STIMECMP, next_event & 0xFFFFFFFF);
                csr_write(CSR_STIMECMPH, next_event >> 32);
#else
                csr_write(CSR_STIMECMP, next_event);
#endif
        } else if (timer_dev && timer_dev->timer_event_start) {
                timer_dev->timer_event_start(next_event);
                csr_clear(CSR_MIP, MIP_STIP);
        }
        csr_set(CSR_MIE, MIP_MTIP);
}

> > +    if ( ticks == ((uint64_t)~0ULL) )
> Nit: With the cast you won't need the ULL suffix.
>
> > +    {
> > +        stop_timer(&t->timer);
> > +
> > +        return;
> > +    }
> > +
> > +    set_timer(&t->timer, expires);
> See the handling of VCPUOP_set_singleshot_timer for what you may want to
> do if the expiry asked for is (perhaps just very slightly) into the past.
I got an idea why we want to check if "expires" already expired, but ...

> There you'll also find a use of migrate_timer(), which you will want to
> at least consider using here as well.
... I don't get why we want to migrate timer before set_timer() here.
Could you please explain that?

Thanks.

~ Oleksii