Re: [PATCH v1 02/15] xen/riscv: implement arch_vcpu_{create,destroy}()

[Jan Beulich <jbeulich@xxxxxxxx>]

(some or even all of the comments may also apply to present Arm code)

On 24.12.2025 18:03, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/domain.c
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/mm.h>
> +#include <xen/sched.h>
> +
> +static void continue_new_vcpu(struct vcpu *prev)
> +{
> +    BUG_ON("unimplemented\n");
> +}
> +
> +int arch_vcpu_create(struct vcpu *v)
> +{
> +    int rc = 0;
> +
> +    BUILD_BUG_ON(sizeof(struct cpu_info) > STACK_SIZE);

I fear you're in trouble also when == or when only a few bytes are left on
the stack. IOW I'm unconvinced that this is a useful check to have.

> +    v->arch.stack = alloc_xenheap_pages(STACK_ORDER, 
> MEMF_node(vcpu_to_node(v)));
> +    if ( !v->arch.stack )
> +        return -ENOMEM;

You don't really need contiguous memory, do you? In which case why not
vmalloc()? This would then also use the larger domheap.

> +    v->arch.cpu_info = (struct cpu_info *)(v->arch.stack
> +                                           + STACK_SIZE
> +                                           - sizeof(struct cpu_info));

Why the cast?

> +    memset(v->arch.cpu_info, 0, sizeof(*v->arch.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;
> +
> +    printk("Create vCPU with sp=%#lx, pc=%#lx, cpu_info(%#lx)\n",
> +           v->arch.xen_saved_context.sp, v->arch.xen_saved_context.ra,
> +           (unsigned long)v->arch.cpu_info);

Please don't, as this is going to get pretty noisy. (And if this wanted
keeping, use %p for pointers rather than casting to unsigned long.)

> +    /* 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)
> +{
> +    free_xenheap_pages(v->arch.stack, STACK_ORDER);
> +}

Better to use FREE_XENHEAP_PAGES() here, I think, to make the function
idempotent.

> --- a/xen/arch/riscv/include/asm/current.h
> +++ b/xen/arch/riscv/include/asm/current.h
> @@ -21,6 +21,12 @@ struct pcpu_info {
>  /* tp points to one of these */
>  extern struct pcpu_info pcpu_info[NR_CPUS];
>  
> +/* Per-VCPU state that lives at the top of the stack */
> +struct cpu_info {
> +    /* This should be the first member. */
> +    struct cpu_user_regs guest_cpu_user_regs;
> +};

You may want to enforce what the comment says by way of a BUILD_BUG_ON().

> --- a/xen/arch/riscv/include/asm/domain.h
> +++ b/xen/arch/riscv/include/asm/domain.h
> @@ -49,6 +49,9 @@ struct arch_vcpu
>          register_t ra;
>      } xen_saved_context;
>  
> +    struct cpu_info *cpu_info;
> +    void *stack;

Do you really need both fields, when one is derived from the other?

Jan