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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 1/6/26 4:56 PM, Jan Beulich wrote:
> (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.
There is really no need for contiguous memory, and|vmalloc()| could be used.
I expect that|vmalloc()| is more expensive and may make hardware prefetching 
less
effective, with more TLB pressure since it allocates 4 KB pages.
However, the latter two points do not really matter in this case, as only a
single 4 KB page is allocated, so we are unlikely to see any performance issues.

> > +    v->arch.cpu_info = (struct cpu_info *)(v->arch.stack
> > +                                           + STACK_SIZE
> > +                                           - sizeof(struct cpu_info));
> Why the cast?
Just for readability, from compiler point of view it could be just dropped.

> > +    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.)
I didn’t consider the case where a large number of vCPUs are created, as
I have only tested with 2 vCPUs. However, if the number of vCPUs is large,
this could indeed get quite noisy.
I will keep these lines of code in downstream for debugging purposes and
drop them from upstream version of this patch.

> > --- 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().
Makes sense, I will add:
  BUILD_BUG_ON(offsetof(struct cpu_info, guest_cpu_user_regs) != 0);
in|arch_vcpu_create()|, somewhere around the initialization of|v->arch.cpu_info 
= ... . |I noticed that there is no|BUILD_BUG_ON()| variant that can be used 
outside
of a function, or does such a variant exist and I’m just missing it? Or there
is no such sense at all for such variant?

> > --- 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?
No, I don't need. I think we can just keep cpu_info and it would be 
enough. Thanks. ~ Oleksii