Re: [PATCH v5 1/5] arm/irq: Keep track of irq affinities

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Mykyta,

> On 13 Jan 2026, at 09:45, Mykyta Poturai <Mykyta_Poturai@xxxxxxxx> wrote:
> 
> Currently on Arm the desc->affinity mask of an irq is never updated,
> which makes it hard to know the actual affinity of an interrupt.
> 
> Fix this by updating the field in irq_set_affinity.

You are changing the generic irq_set_affinity which now requires
the irq descriptor to be locked.

This is good because it puts arm at an equivalent state than x86
one.

By doing a quick check there might be places where you
forgot to add the lock/unlock:

vgic/vgic.c line 826 (arch_move_irqs)
vgic/vgic-mmio-v2.c line 173 (vgic_mmio_write_target)

both of them take the irq lock but not the desc lock as far as i can see.

Cheers
Bertrand

> 
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> 
> ---
> v4->v5:
> * add locking
> 
> v3->v4:
> * patch introduced
> ---
> xen/arch/arm/gic-vgic.c |  2 ++
> xen/arch/arm/irq.c      |  9 +++++++--
> xen/arch/arm/vgic.c     | 14 ++++++++++++--
> 3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index ea48c5375a..5253caf002 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -232,7 +232,9 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>             if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
>             {
>                 struct vcpu *v_target = vgic_get_target_vcpu(v, irq);
> +                spin_lock(&p->desc->lock);
>                 irq_set_affinity(p->desc, cpumask_of(v_target->processor));
> +                spin_unlock(&p->desc->lock);
>                 clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
>             }
>         }
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 73e58a5108..7204bc2b68 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -216,10 +216,15 @@ static inline struct domain *irq_get_domain(struct 
> irq_desc *desc)
>     return irq_get_guest_info(desc)->d;
> }
> 
> +/* Must be called with desc->lock held */
> void irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
> {
> -    if ( desc != NULL )
> -        desc->handler->set_affinity(desc, mask);
> +    if ( desc == NULL )
> +        return;
> +
> +    ASSERT(spin_is_locked(&desc->lock));
> +    cpumask_copy(desc->affinity, mask);
> +    desc->handler->set_affinity(desc, mask);
> }
> 
> int request_irq(unsigned int irq, unsigned int irqflags,
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 6647071ad4..c59f6873db 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -445,7 +445,9 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, 
> unsigned int irq)
> 
>     if ( list_empty(&p->inflight) )
>     {
> +        spin_lock(&p->desc->lock);
>         irq_set_affinity(p->desc, cpumask_of(new->processor));
> +        spin_unlock(&p->desc->lock);
>         spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>         return true;
>     }
> @@ -453,7 +455,9 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, 
> unsigned int irq)
>     if ( !list_empty(&p->lr_queue) )
>     {
>         vgic_remove_irq_from_queues(old, p);
> +        spin_lock(&p->desc->lock);
>         irq_set_affinity(p->desc, cpumask_of(new->processor));
> +        spin_unlock(&p->desc->lock);
>         spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>         vgic_inject_irq(new->domain, new, irq, true);
>         return true;
> @@ -473,6 +477,7 @@ void arch_move_irqs(struct vcpu *v)
>     struct domain *d = v->domain;
>     struct pending_irq *p;
>     struct vcpu *v_target;
> +    unsigned long flags;
>     int i;
> 
>     /*
> @@ -494,7 +499,13 @@ void arch_move_irqs(struct vcpu *v)
>         p = irq_to_pending(v_target, virq);
> 
>         if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> +        {
> +            if ( !p->desc )
> +                continue;
> +            spin_lock_irqsave(&p->desc->lock, flags);
>             irq_set_affinity(p->desc, cpu_mask);
> +            spin_unlock_irqrestore(&p->desc->lock, flags);
> +        }
>     }
> }
> 
> @@ -574,8 +585,8 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, 
> unsigned int n)
>         spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
>         if ( p->desc != NULL )
>         {
> -            irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>             spin_lock_irqsave(&p->desc->lock, flags);
> +            irq_set_affinity(p->desc, cpumask_of(v_target->processor));
>             /*
>              * The irq cannot be a PPI, we only support delivery of SPIs
>              * to guests.
> @@ -944,4 +955,3 @@ void vgic_check_inflight_irqs_pending(struct vcpu *v, 
> unsigned int rank, uint32_
>  * indent-tabs-mode: nil
>  * End:
>  */
> -
> -- 
> 2.51.2