Re: [PATCH v3 01/16] xen/riscv: implement arch_vcpu_{create,destroy}()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.02.2026 17:52, Oleksii Kurochko wrote:
> Introduce architecture-specific functions to create and destroy VCPUs.
> Note that arch_vcpu_create() currently returns -EOPNOTSUPP, as the virtual
> timer and interrupt controller are not yet implemented.
> 
> Add calle-saved registers used to preserve Xen’s own execution context
> when switching between vCPU stacks.

"Add" is lacking context here: You don't add those to arch_vcpu_create(),
which is the context left from the earlier paragraph.

> It is going to be used in the following way (pseudocode):
>   context_switch(prev_vcpu, next_vcpu):
>     ...
> 
>     /* Switch from previous stack to the next stack. */
>     __context_switch(prev_vcpu, next_vcpu);
> 
>     ...
>     schedule_tail(prev_vcpu):
>         Save and restore vCPU's CSRs.
> The Xen-saved context allows __context_switch() to switch execution
> from the previous vCPU’s stack to the next vCPU’s stack and later resume
> execution on the original stack when switching back.
> 
> During vCPU creation, the Xen-saved context is going to be initialized
> with:
>   - SP pointing to the newly allocated vCPU stack
>   - RA pointing to a helper that performs final vCPU setup before
>     transferring control to the guest
> After the first execution of __context_switch(), RA naturally points to
> the instruction following the call site, and the remaining callee-saved
> registers contain the Xen register state at the time of the switch.

RA doesn't "naturally" point anywhere until you actually implement more
pieces. Please, again, can descriptions be written such that they make
sense at the point where the patch being described applies?

> --- /dev/null
> +++ b/xen/arch/riscv/domain.c
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/init.h>
> +#include <xen/mm.h>
> +#include <xen/sched.h>
> +#include <xen/vmap.h>
> +
> +static void continue_new_vcpu(struct vcpu *prev)
> +{
> +    BUG_ON("unimplemented\n");
> +}
> +
> +static void __init __maybe_unused build_assertions(void)
> +{
> +    /*
> +     * Enforce the requirement documented in struct cpu_info that
> +     * guest_cpu_user_regs must be the first field.
> +     */
> +    BUILD_BUG_ON(offsetof(struct cpu_info, guest_cpu_user_regs) != 0);
> +}
> +
> +int arch_vcpu_create(struct vcpu *v)
> +{
> +    int rc = 0;
> +    void *stack = vzalloc(STACK_SIZE);

Much like you use void * here, ...

> +    if ( !stack )
> +        return -ENOMEM;
> +
> +    v->arch.cpu_info = stack + STACK_SIZE - sizeof(struct cpu_info);
> +
> +    v->arch.xen_saved_context.sp = (register_t)v->arch.cpu_info;
> +    v->arch.xen_saved_context.ra = (register_t)continue_new_vcpu;
> +
> +    /* Idle VCPUs don't need the rest of this setup */
> +    if ( is_idle_vcpu(v) )
> +        return rc;
> +
> +    /*
> +     * As the vtimer and interrupt controller (IC) are not yet implemented,
> +     * return an error.
> +     *
> +     * TODO: Drop this once the vtimer and IC are implemented.
> +     */
> +    rc = -EOPNOTSUPP;
> +    goto fail;
> +
> +    return rc;
> +
> + fail:
> +    arch_vcpu_destroy(v);
> +    return rc;
> +}
> +
> +void arch_vcpu_destroy(struct vcpu *v)
> +{
> +    vfree((char *)v->arch.cpu_info + sizeof(struct cpu_info) - STACK_SIZE);

... you probably want to do so here as well. And btw, this can be shortened:

    vfree((void *)&v->arch.cpu_info[1] - STACK_SIZE);

Jan