Add implementations of vtimer_save() and vtimer_restore().

At the moment, vrtimer_save() does nothing as SSTC, which provided
virtualization-aware timer,  isn't supported yet, so emulated (SBI-based)
timer is used.

vtimer uses internal Xen timer: initialize it on the pcpu the vcpu is
running on, rather than the processor that it's creating the vcpu.

On vcpu restore migrate (when vtimer_restore() is going to be called)
the vtimer to the pcpu the vcpu is running on.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 xen/arch/riscv/include/asm/vtimer.h |  3 +++
 xen/arch/riscv/vtimer.c             | 15 +++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/xen/arch/riscv/include/asm/vtimer.h b/xen/arch/riscv/include/asm/vtimer.h
index 2cacaf74b83b..e0f94f7c31c7 100644
--- a/xen/arch/riscv/include/asm/vtimer.h
+++ b/xen/arch/riscv/include/asm/vtimer.h
@@ -24,4 +24,7 @@ int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config);
 
 void vtimer_set_timer(struct vtimer *t, const uint64_t ticks);
 
+void vtimer_save(struct vcpu *v);
+void vtimer_restore(struct vcpu *v);
+
 #endif /* ASM__RISCV__VTIMER_H */
diff --git a/xen/arch/riscv/vtimer.c b/xen/arch/riscv/vtimer.c
index 99a0c5986f1d..4256fe9a2bb0 100644
--- a/xen/arch/riscv/vtimer.c
+++ b/xen/arch/riscv/vtimer.c
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 
+#include <xen/bug.h>
 #include <xen/domain.h>
 #include <xen/sched.h>
 #include <xen/time.h>
@@ -65,3 +66,17 @@ void vtimer_set_timer(struct vtimer *t, const uint64_t ticks)
 
     set_timer(&t->timer, expires);
 }
+
+void vtimer_save(struct vcpu *p)
+{
+    ASSERT(!is_idle_vcpu(p));
+
+    /* Nothing to do at the moment as SSTC isn't supported now. */
+}
+
+void vtimer_restore(struct vcpu *n)
+{
+    ASSERT(!is_idle_vcpu(n));
+
+    migrate_timer(&n->arch.vtimer.timer, n->processor);
+}
-- 
2.52.0

On 24.12.2025 18:03, Oleksii Kurochko wrote:
> Add implementations of vtimer_save() and vtimer_restore().

And these are going to serve what purpose? Are they for context switch, or
for migration / save / restore? In the former case (supported by the naming
of the function parameters), I think they want naming differently (to avoid
confusion). See how x86 has e.g. ..._ctxt_switch_{from,to}() and then
..._switch_{from,to}() helpers.

> At the moment, vrtimer_save() does nothing as SSTC, which provided
> virtualization-aware timer,  isn't supported yet, so emulated (SBI-based)
> timer is used.

Is "emulated" really the correct term here? You don't intercept any guest
insns, but rather provide a virtual SBI.

> vtimer uses internal Xen timer: initialize it on the pcpu the vcpu is
> running on, rather than the processor that it's creating the vcpu.

This doesn't look to describe anything this patch does.

> On vcpu restore migrate (when vtimer_restore() is going to be called)
> the vtimer to the pcpu the vcpu is running on.

Why "going to be" when you describe what the function does?

> --- a/xen/arch/riscv/include/asm/vtimer.h
> +++ b/xen/arch/riscv/include/asm/vtimer.h
> @@ -24,4 +24,7 @@ int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config);
>  
>  void vtimer_set_timer(struct vtimer *t, const uint64_t ticks);
>  
> +void vtimer_save(struct vcpu *v);
> +void vtimer_restore(struct vcpu *v);

Misra demands that parameter names in declarations match ...

> @@ -65,3 +66,17 @@ void vtimer_set_timer(struct vtimer *t, const uint64_t ticks)
>  
>      set_timer(&t->timer, expires);
>  }
> +
> +void vtimer_save(struct vcpu *p)
> +{
> +    ASSERT(!is_idle_vcpu(p));
> +
> +    /* Nothing to do at the moment as SSTC isn't supported now. */
> +}
> +
> +void vtimer_restore(struct vcpu *n)
> +{
> +    ASSERT(!is_idle_vcpu(n));
> +
> +    migrate_timer(&n->arch.vtimer.timer, n->processor);
> +}

... the ones in the definitions. No matter that RISC-V isn't scanned by Eclair,
yet, I think you want to avoid the need to later fix things up.

Jan

On 1/8/26 11:43 AM, Jan Beulich wrote:
> On 24.12.2025 18:03, Oleksii Kurochko wrote:
>> Add implementations of vtimer_save() and vtimer_restore().
> And these are going to serve what purpose? Are they for context switch, or
> for migration / save / restore? In the former case (supported by the naming
> of the function parameters), I think they want naming differently (to avoid
> confusion). See how x86 has e.g. ..._ctxt_switch_{from,to}() and then
> ..._switch_{from,to}() helpers.

Based only on the name it is clear for what ..._ctxt_switch_{from,to}() will
be used, ..._switch_{from,to}() isn't clear just based on the name how it will
be used.

Anyway, I am okay to change vtimer_{save,restore}() to vtimer_ctx_switch_{from,to}()
and then follow for other stuff to follow the same approach (as I used for everything
*_save() *_store()).

>> At the moment, vrtimer_save() does nothing as SSTC, which provided
>> virtualization-aware timer,  isn't supported yet, so emulated (SBI-based)
>> timer is used.
> Is "emulated" really the correct term here? You don't intercept any guest
> insns, but rather provide a virtual SBI.

I wasn't sure that it is the best one term.
Probably then just "virtual (SBI-based) timer" is better to use.

>
>> vtimer uses internal Xen timer: initialize it on the pcpu the vcpu is
>> running on, rather than the processor that it's creating the vcpu.
> This doesn't look to describe anything this patch does.

Hm, and why not?

In vcpu_vtimer_init() we're initializing timer (it was incorrect to use
"internal Xen timer" though) on CPU is stored in vcpu->processor by calling
init_timier().

I will update this part then to:
  Initialize the timer contained in|struct vtimer| by calling|init_timer()|.

>
>> On vcpu restore migrate (when vtimer_restore() is going to be called)
>> the vtimer to the pcpu the vcpu is running on.
> Why "going to be" when you describe what the function does?

Because it isn't called now. The part inside (...) could be dropped.

>
>> --- a/xen/arch/riscv/include/asm/vtimer.h
>> +++ b/xen/arch/riscv/include/asm/vtimer.h
>> @@ -24,4 +24,7 @@ int domain_vtimer_init(struct domain *d, struct xen_arch_domainconfig *config);
>>   
>>   void vtimer_set_timer(struct vtimer *t, const uint64_t ticks);
>>   
>> +void vtimer_save(struct vcpu *v);
>> +void vtimer_restore(struct vcpu *v);
> Misra demands that parameter names in declarations match ...
>
>> @@ -65,3 +66,17 @@ void vtimer_set_timer(struct vtimer *t, const uint64_t ticks)
>>   
>>       set_timer(&t->timer, expires);
>>   }
>> +
>> +void vtimer_save(struct vcpu *p)
>> +{
>> +    ASSERT(!is_idle_vcpu(p));
>> +
>> +    /* Nothing to do at the moment as SSTC isn't supported now. */
>> +}
>> +
>> +void vtimer_restore(struct vcpu *n)
>> +{
>> +    ASSERT(!is_idle_vcpu(n));
>> +
>> +    migrate_timer(&n->arch.vtimer.timer, n->processor);
>> +}
> ... the ones in the definitions. No matter that RISC-V isn't scanned by Eclair,
> yet, I think you want to avoid the need to later fix things up.

Sure, I'll fix that.

Thanks.

~ Oleksii

On 13.01.2026 16:32, Oleksii Kurochko wrote:
> On 1/8/26 11:43 AM, Jan Beulich wrote:
>> On 24.12.2025 18:03, Oleksii Kurochko wrote:
>>> vtimer uses internal Xen timer: initialize it on the pcpu the vcpu is
>>> running on, rather than the processor that it's creating the vcpu.
>> This doesn't look to describe anything this patch does.
> 
> Hm, and why not?

Because this patch doesn't initialize any timer. The only timer-related
call I see is one to migrate_timer().

> In vcpu_vtimer_init() we're initializing timer (it was incorrect to use
> "internal Xen timer" though) on CPU is stored in vcpu->processor by calling
> init_timier().
> 
> I will update this part then to:
>   Initialize the timer contained in|struct vtimer| by calling|init_timer()|.

But you don't call that function. (Nor is this, btw, a useful sentence
to have in a patch description. May I suggest that you read a fair number
of in particular Andrew's or Roger's patch descriptions, to get a feel
for what wants saying and what doesn't need to be said? In the case above:
How else could you plausibly initialize that timer? Hence the latter part
of the sentence is largely meaningless. Plus - is leaving the field
uninitialized a plausible option? IOW you're merely stating the obvious
anyway. Sadly, and I'm sorry to have to say that, this carries through
many of your patch descriptions: You mechanically state what is being
done, when really the thinking behind what you're doing and, often,
further plans would be relevant to call out.)

Jan