Re: [PATCH v1 04/15] xen/riscv: introduce vtimer

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/7/26 4:21 PM, Jan Beulich wrote:
> On 24.12.2025 18:03, Oleksii Kurochko wrote:
> > Introduce a virtual timer structure along with functions to initialize
> > and destroy the virtual timer.
> >
> > Add a vtimer_expired() function and implement it as a stub, as the timer
> > and tasklet subsystems are not functional at this stage.
> Shouldn't those pieces of infrastructure be made work then first?
It could be an option; it’s just not really critical until a guest is running.

I actually considered adding this in the current patch series, but decided to
introduce it later to avoid making the series too large. (On the other hand, it
would be only one additional patch, IIRC)

> I also
> don't quite understand why the subsystems not being functional prevents
> the function to be implemented as far as possible. Most if not all
> functions you need from both subsystems should be available, for living
> in common code.
I chose the wrong words here; this is not the main (that some subsystems isn't
fully functional) reason why I’m using a stub here instead of something 
functional.

Basically, implementing this requires vcpu_kick() and vcpu_set_interrupt(),
which are introduced later in this patch series.

As an alternative, I could drop vtimer_expired() and the related code from this
patch and reintroduce them after vcpu_kick() and vcpu_set_interrupt() are
available.

> > --- a/xen/arch/riscv/include/asm/domain.h
> > +++ b/xen/arch/riscv/include/asm/domain.h
> > @@ -8,6 +8,7 @@
> >   #include <public/hvm/params.h>
> >   
> >   #include <asm/p2m.h>
> > +#include <asm/vtimer.h>
> >   
> >   struct vcpu_vmid {
> >       uint64_t generation;
> > @@ -52,6 +53,9 @@ struct arch_vcpu
> >       struct cpu_info *cpu_info;
> >       void *stack;
> >   
> > +    struct vtimer vtimer;
> > +    bool vtimer_initialized;
> Assuming the field is really needed (see remark further down), why is this
> not part of the struct?
Agree, it would be better to have it as a part of struct vtimer if it will
be used in future.

> > --- /dev/null
> > +++ b/xen/arch/riscv/include/asm/vtimer.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * (c) 2023-2024 Vates
> > + */
> > +
> > +#ifndef ASM__RISCV__VTIMER_H
> > +#define ASM__RISCV__VTIMER_H
> > +
> > +#include <xen/timer.h>
> > +
> > +struct domain;
> > +struct vcpu;
> I don't think this one is needed, as long as you have ...
>
> > +struct xen_arch_domainconfig;
> > +
> > +struct vtimer {
> > +    struct vcpu *v;
> ... this. Question is why this is here: You should be able to get hold of the
> struct vcpu containing a struct vtimer using container_of().
Good point, I haven't thought about that. It could really be done using 
container_of().


> > --- /dev/null
> > +++ b/xen/arch/riscv/vtimer.c
> > @@ -0,0 +1,39 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +#include <xen/sched.h>
> > +
> > +#include <public/xen.h>
> > +
> > +#include <asm/vtimer.h>
> > +
> > +int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config)
> > +{
> > +    /* Nothing to do at the moment */
> > +
> > +    return 0;
> > +}
> The function has no caller and does nothing - why do we need it?
It will be called later in arch_domain_create().

It will be needed if SSTC extension will be supported but could be dropped now.

> > +static void vtimer_expired(void *data)
> > +{
> > +    panic("%s: TBD\n", __func__);
> > +}
> > +
> > +int vcpu_vtimer_init(struct vcpu *v)
> > +{
> > +    struct vtimer *t = &v->arch.vtimer;
> > +
> > +    t->v = v;
> > +    init_timer(&t->timer, vtimer_expired, t, v->processor);
> > +
> > +    v->arch.vtimer_initialized = true;
> init_timer() has specific effects (like setting t->function to non-NULL
> and t->status to other than TIMER_STATUS_invalid). Can't you leverage
> that instead of having a separate boolean? (Iirc we do so elsewhere.)
Nice, it could be used instead of having vtimer_initialized in struct vtimer.

Thanks.

~ Oleksii