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
