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

Oleksii Kurochko posted 15 patches 2 weeks ago
[PATCH v1 04/15] xen/riscv: introduce vtimer
Posted by Oleksii Kurochko 2 weeks ago
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.

Call vcpu_vtimer_init() in arch_vcpu_create().

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/Makefile             |  1 +
 xen/arch/riscv/domain.c             |  8 ++++--
 xen/arch/riscv/include/asm/domain.h |  4 +++
 xen/arch/riscv/include/asm/vtimer.h | 25 ++++++++++++++++++
 xen/arch/riscv/vtimer.c             | 39 +++++++++++++++++++++++++++++
 5 files changed, 75 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/riscv/include/asm/vtimer.h
 create mode 100644 xen/arch/riscv/vtimer.c

diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile
index 8863d4b15605..5bd180130165 100644
--- a/xen/arch/riscv/Makefile
+++ b/xen/arch/riscv/Makefile
@@ -22,6 +22,7 @@ obj-y += traps.o
 obj-y += vmid.o
 obj-y += vm_event.o
 obj-y += vsbi/
+obj-y += vtimer.o
 
 $(TARGET): $(TARGET)-syms
 	$(OBJCOPY) -O binary -S $< $@
diff --git a/xen/arch/riscv/domain.c b/xen/arch/riscv/domain.c
index 44387d056546..dd3c237d163d 100644
--- a/xen/arch/riscv/domain.c
+++ b/xen/arch/riscv/domain.c
@@ -6,6 +6,7 @@
 #include <asm/cpufeature.h>
 #include <asm/csr.h>
 #include <asm/riscv_encoding.h>
+#include <asm/vtimer.h>
 
 static void vcpu_csr_init(struct vcpu *v)
 {
@@ -97,11 +98,14 @@ int arch_vcpu_create(struct vcpu *v)
     if ( is_idle_vcpu(v) )
         return rc;
 
+    if ( (rc = vcpu_vtimer_init(v)) )
+        goto fail;
+
     /*
-     * As the vtimer and interrupt controller (IC) are not yet implemented,
+     * As interrupt controller (IC) is not yet implemented,
      * return an error.
      *
-     * TODO: Drop this once the vtimer and IC are implemented.
+     * TODO: Drop this once IC is implemented.
      */
     rc = -EOPNOTSUPP;
     goto fail;
diff --git a/xen/arch/riscv/include/asm/domain.h b/xen/arch/riscv/include/asm/domain.h
index a0ffbbc09c6f..be7ddaff30e7 100644
--- 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;
+
     /* CSRs */
     register_t hstatus;
     register_t hedeleg;
diff --git a/xen/arch/riscv/include/asm/vtimer.h b/xen/arch/riscv/include/asm/vtimer.h
new file mode 100644
index 000000000000..a2ca704cf0cc
--- /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;
+struct xen_arch_domainconfig;
+
+struct vtimer {
+    struct vcpu *v;
+    struct timer timer;
+};
+
+int vcpu_vtimer_init(struct vcpu *v);
+void vcpu_timer_destroy(struct vcpu *v);
+
+int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config);
+
+#endif /* ASM__RISCV__VTIMER_H */
diff --git a/xen/arch/riscv/vtimer.c b/xen/arch/riscv/vtimer.c
new file mode 100644
index 000000000000..5ba533690bc2
--- /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;
+}
+
+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;
+
+    return 0;
+}
+
+void vcpu_timer_destroy(struct vcpu *v)
+{
+    if ( !v->arch.vtimer_initialized )
+        return;
+
+    kill_timer(&v->arch.vtimer.timer);
+}
-- 
2.52.0
Re: [PATCH v1 04/15] xen/riscv: introduce vtimer
Posted by Jan Beulich 21 hours ago
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