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

[Jan Beulich <jbeulich@xxxxxxxx>]

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