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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.12.2025 13:42, Oleksii Kurochko wrote:
> 
> On 12/18/25 7:15 PM, Andrew Cooper wrote:
>> 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.
> 
> Btw, IIUC, it could be also be something like:
>    (void) vcpu_vgic_free(...)
> and then we won't break any MISRA rule, right?

That would address one Misra concern, but there would then still be dead code.

Jan