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

[Jan Beulich <jbeulich@xxxxxxxx>]

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? 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.

> --- 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?

> --- /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().

> --- /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?

> +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.)

Jan