Re: [PATCH v1 09/15] xen/riscv: add vtimer_{save,restore}()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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