Re: [RFC PATCH 01/10] ARM: vGIC: remove rank lock from IRQ routing functions

[Julien Grall <julien.grall@arm.com>]

Hi Andre,

On 04/05/17 16:31, Andre Przywara wrote:
> gic_route_irq_to_guest() and gic_remove_irq_from_guest() take the rank
> lock, however never actually access the rank structure.
> Avoid taking the lock in those two functions and remove some more
> unneeded code on the way.

The rank is here to protect p->desc when checking that the virtual 
interrupt was not yet routed to another physical interrupt.

Without this locking, you can have two concurrent call of 
gic_route_irq_to_guest that will update the same virtual interrupt but 
with different physical interrupts.

You would have to replace the rank lock by the per-pending_irq lock to 
keep the code safe.

Cheers,

>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  xen/arch/arm/gic.c | 28 ++++------------------------
>  1 file changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index da19130..c734f66 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -136,25 +136,19 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned int priority)
>  int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>                             struct irq_desc *desc, unsigned int priority)
>  {
> -    unsigned long flags;
>      /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
>       * route SPIs to guests, it doesn't make any difference. */
> -    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> -    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> -    struct pending_irq *p = irq_to_pending(v_target, virq);
> -    int res = -EBUSY;
> +    struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
>
>      ASSERT(spin_is_locked(&desc->lock));
>      /* Caller has already checked that the IRQ is an SPI */
>      ASSERT(virq >= 32);
>      ASSERT(virq < vgic_num_irqs(d));
>
> -    vgic_lock_rank(v_target, rank, flags);
> -
>      if ( p->desc ||
>           /* The VIRQ should not be already enabled by the guest */
>           test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
> -        goto out;
> +        return -EBUSY;
>
>      desc->handler = gic_hw_ops->gic_guest_irq_type;
>      set_bit(_IRQ_GUEST, &desc->status);
> @@ -164,29 +158,20 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
>      gic_set_irq_priority(desc, priority);
>
>      p->desc = desc;
> -    res = 0;
>
> -out:
> -    vgic_unlock_rank(v_target, rank, flags);
> -
> -    return res;
> +    return 0;
>  }
>
>  /* This function only works with SPIs for now */
>  int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>                                struct irq_desc *desc)
>  {
> -    struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> -    struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> -    struct pending_irq *p = irq_to_pending(v_target, virq);
> -    unsigned long flags;
> +    struct pending_irq *p = irq_to_pending(d->vcpu[0], virq);
>
>      ASSERT(spin_is_locked(&desc->lock));
>      ASSERT(test_bit(_IRQ_GUEST, &desc->status));
>      ASSERT(p->desc == desc);
>
> -    vgic_lock_rank(v_target, rank, flags);
> -
>      if ( d->is_dying )
>      {
>          desc->handler->shutdown(desc);
> @@ -204,10 +189,7 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>           */
>          if ( test_bit(_IRQ_INPROGRESS, &desc->status) ||
>               !test_bit(_IRQ_DISABLED, &desc->status) )
> -        {
> -            vgic_unlock_rank(v_target, rank, flags);
>              return -EBUSY;
> -        }
>      }
>
>      clear_bit(_IRQ_GUEST, &desc->status);
> @@ -215,8 +197,6 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>
>      p->desc = NULL;
>
> -    vgic_unlock_rank(v_target, rank, flags);
> -
>      return 0;
>  }
>
>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel