Re: [PATCH v2 1/4] xen/arm: optimize the size of struct vcpu

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/12/2025 5:28 pm, Oleksii Kurochko wrote:
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 3ebdf9953f..8b17871b86 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -370,29 +370,35 @@ int vcpu_vgic_init(struct vcpu *v)
>  {
>      int i;
>  
> -    v->arch.vgic.private_irqs = xzalloc(struct vgic_irq_rank);
> -    if ( v->arch.vgic.private_irqs == NULL )
> +    v->arch.vgic = xzalloc(struct vgic_cpu);
> +    if ( v->arch.vgic == NULL )
> +        return -ENOMEM;
> +
> +    v->arch.vgic->private_irqs = xzalloc(struct vgic_irq_rank);
> +    if ( v->arch.vgic->private_irqs == NULL )
>        return -ENOMEM;

This error path needs to free v->arch.vgic.  (If we continue down this
route.  See below.)

>  
>      /* SGIs/PPIs are always routed to this VCPU */
> -    vgic_rank_init(v->arch.vgic.private_irqs, 0, v->vcpu_id);
> +    vgic_rank_init(v->arch.vgic->private_irqs, 0, v->vcpu_id);
>  
>      v->domain->arch.vgic.handler->vcpu_init(v);
>  
> -    memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
> +    memset(&v->arch.vgic->pending_irqs, 0, 
> sizeof(v->arch.vgic->pending_irqs));
>      for (i = 0; i < 32; i++)
> -        vgic_init_pending_irq(&v->arch.vgic.pending_irqs[i], i);
> +        vgic_init_pending_irq(&v->arch.vgic->pending_irqs[i], i);
>  
> -    INIT_LIST_HEAD(&v->arch.vgic.inflight_irqs);
> -    INIT_LIST_HEAD(&v->arch.vgic.lr_pending);
> -    spin_lock_init(&v->arch.vgic.lock);
> +    INIT_LIST_HEAD(&v->arch.vgic->inflight_irqs);
> +    INIT_LIST_HEAD(&v->arch.vgic->lr_pending);
> +    spin_lock_init(&v->arch.vgic->lock);
>  
>      return 0;
>  }
>  
>  int vcpu_vgic_free(struct vcpu *v)
>  {
> -    xfree(v->arch.vgic.private_irqs);
> +    xfree(v->arch.vgic->private_irqs);
> +    xfree(v->arch.vgic);
> +
>      return 0;
>  }

Free functions should be idempotent.  This was buggy before, even moreso
now.  It wants to be:

void vcpu_vgic_free(struct vcpu *v)
{
    if ( v->arch.vgic )
    {
        XFREE(v->arch.vgic->private_irqs);
        XFREE(v->arch.vgic);
    }
}

Given the type change, this probably wants splitting out into an earlier
patch.

Given the fact that the single caller doesn't even check the return
value, you're fixing a MISRA violation by making it void.

> diff --git a/xen/arch/arm/vgic/vgic-init.c b/xen/arch/arm/vgic/vgic-init.c
> index f8d7d3a226..67f297797f 100644
> --- a/xen/arch/arm/vgic/vgic-init.c
> +++ b/xen/arch/arm/vgic/vgic-init.c
> @@ -241,10 +245,12 @@ void domain_vgic_free(struct domain *d)
>  
>  int vcpu_vgic_free(struct vcpu *v)
>  {
> -    struct vgic_cpu *vgic_cpu = &v->arch.vgic;
> +    struct vgic_cpu *vgic_cpu = v->arch.vgic;
>  
>      INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
>  
> +    xfree(vgic_cpu);
> +
>      return 0;
>  }

Not in your part of the change, but this is bogus.  It's not remotely
safe to init the list head like that.

The list is either already empty, in which case it's a no-op, or it
corrupts the list.  It appears that the list mixes entries from other
vCPUs, and from the domain.

I think this is further proof that NEW_VGIC should be deleted
wholesale.  It's clearly not in a good state, and I get the impression
that a badly timed evtchn sent to a domain in the middle of being
cleaned up will cause Xen to trip over the corrupted list.

>  
> diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
> index 6a558089c5..e64d681dd2 100644
> --- a/xen/arch/arm/vgic/vgic-v2.c
> +++ b/xen/arch/arm/vgic/vgic-v2.c
> @@ -56,8 +56,8 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t 
> csize,
>   */
>  void vgic_v2_fold_lr_state(struct vcpu *vcpu)
>  {
> -    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
> -    unsigned int used_lrs = vcpu->arch.vgic.used_lrs;
> +    struct vgic_cpu *vgic_cpu = vcpu->arch.vgic;
> +    unsigned int used_lrs = vcpu->arch.vgic->used_lrs;

vgic_cpu->used_lrs.

Taking a step back, I think the patch could be much smaller if you only
made private_irqs in NEW_VGIC be a separate pointer, matching the "old"
VGIC code.  Or does that not save enough space in struct vCPU?

~Andrew