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

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 04/05/17 16:53, Julien Grall wrote:
> 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.

Really? To me that sounds quite surprising.
My understanding is that the VGIC VCPU lock protected the pending_irq
(and thus the desc pointer?) so far, and the desc itself has its own lock.
According to the comment in the struct irq_rank declaration the lock
protects the members of this struct only.

Looking briefly at users of pending_irq->desc (for instance
gicv[23]_update_lr() or gic_update_one_lr()) I can't see any hint that
they care about the lock.

So should that be fixed or at least documented?

> 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.

That indeed sounds reasonable.

Cheers,
Andre.

>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>>  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;
>>  }
>>
>>
> 

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