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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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.

~ 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>