(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
