Re: [PATCH v1] xen: move alloc/free_vcpu_struct() to common code

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/17/25 4:37 PM, Oleksii Kurochko wrote:
> On 12/17/25 12:46 PM, Andrew Cooper wrote:
> > On 17/12/2025 10:53 am, Oleksii Kurochko wrote:
> > > alloc_vcpu_struct() and free_vcpu_struct() contain little
> > > architecture-specific logic and are suitable for sharing across
> > > architectures. Move both helpers to common code.
> > >
> > > To support the remaining architectural differences, introduce
> > > arch_vcpu_struct_memflags(), allowing architectures to override the
> > > memory flags passed to alloc_xenheap_pages(). This is currently needed
> > > by x86, which may require MEMF_bits(32) for HVM guests using shadow
> > > paging.
> > You lost the comment explaining the restriction.  This needs adding 
> > back.
> > > Move the definition of MAX_PAGES_PER_VCPU to xen/domain.h and default
> > > it to 1. Retain the ARM64 exception (with CONFIG_NEW_VGIC) where two
> > > pages are required due to larger per-IRQ structures.
> > CONFIG_NEW_VGIC is still off by default, unsupported, and has had no
> > work on it since it's introduction in 2018.
> >
> > There are a lot of good reasons to enforce struct vcpu being a single
> > page allocation, not least because an allocation can fail due to
> > fragmentation despite there being enough free RAM.
> >
> > I would far rather that common code enforced it being page size, and
> > NEW_VGIC gets deleted or adjusted to cope, than to make it this easy for
> > architectures to shoot themselves in the foot.
> I am not sure that everyone would agree to simply drop|NEW_VGIC|.
>
> Based on the commit message ...:
>     ARM: new VGIC: Allocate two pages for struct vcpu
>         At the moment we allocate exactly one page for struct vcpu on 
> ARM, also
>     have a check in place to prevent it growing beyond 4KB.
>     As the struct includes the state of all 32 private (per-VCPU) 
> interrupts,
>     we are at 3840 bytes on arm64 at the moment already. Growing the 
> per-IRQ
>     VGIC structure even slightly makes the VCPU quickly exceed the 4K 
> limit.
>     The new VGIC will need more space per virtual IRQ. I spent a few 
> hours
>     trying to trim this down, but couldn't get it below 4KB, even with 
> the
>     nasty hacks piling up to save some bytes here and there.
>     It turns out that beyond efficiency, maybe, there is no real 
> technical
>     reason this struct has to fit in one page, so lifting the limit to 
> two
>     pages seems like the most pragmatic solution.
>     Restrict the compilation error to compiling with the new VGIC and for
>     ARM64 only.
>
> ... Given this, I initially thought it would be difficult to adjust.
> However, it seems there might be enough to do the following ...:
> @@ -238,7 +238,7 @@ struct arch_vcpu
>      union gic_state_data gic;
>      uint64_t lr_mask;
>
> -    struct vgic_cpu vgic;
> +    struct vgic_cpu *vgic;
>
>      /* Timer registers  */
>      register_t cntkctl;
>
> ... with|vgic| allocated in|vcpu_vgic_init()| and freed 
> in|vcpu_vgic_free()|.
> I only checked whether this is sufficient to build with ...:
>    #if defined(CONFIG_NEW_VGIC) && defined(CONFIG_ARM_64)
>    -#define MAX_PAGES_PER_VCPU  2
>    +#define MAX_PAGES_PER_VCPU  1
>    #endif
>     $ grep -nE "^(CONFIG_NEW_VGIC|CONFIG_ARM_64)=y" xen/.config
>   13:CONFIG_ARM_64=y
>   28:CONFIG_NEW_VGIC=y
> ... and it seems to work.
>
> The question to the Arm maintainers is whether you would be fine with 
> such a
> solution.
I've done some measurements with the suggested changes [1] to Arm’s|arch_vcpu|
structure. As a result, the size of|struct vcpu| on Arm is reduced to 2048 
bytes,
compared to 3840 bytes (without the changes in [1] and with|CONFIG_ARM_64=y|) 
and
4736 bytes (without the changes in [1] and with both|CONFIG_ARM_64=y| and
|CONFIG_NEW_VGIC=y|).

The result looks good enough, so I will propose [1] while preparing v2 of this
patch.

Also, all CI tests passed: 
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/2220545118

[1] xen/arm: optimize the size of struct vcpu ( 
https://gitlab.com/xen-project/people/olkur/xen/-/commit/946a1c2cfaf4157074470a653bba5baa8561ebbf
 )

~ Oleksii

> > > The ARM implementation of alloc/free_vcpu_struct() is removed and
> > > replaced by the common version. Stub implementations are also dropped
> > > from PPC and RISC-V.
> > >
> > > Finally, make alloc_vcpu_struct() and free_vcpu_struct() static to
> > > common/domain.c, as they are no longer used outside common code.
> > >
> > > No functional changes.
> > >
> > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>