[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC PATCH 04/10] ARM: vGIC: add struct pending_irq locking



Hi,

On 04/05/17 17:21, Julien Grall wrote:
> Hi Andre,
> 
> On 04/05/17 16:31, Andre Przywara wrote:
>> Introduce the proper locking sequence for the new pending_irq lock.
>> This takes the lock around multiple accesses to struct members,
>> also makes sure we observe the locking order (VGIC VCPU lock first,
>> then pending_irq lock).
> 
> This locking order should be explained in the code. Likely in vgic.h.
> 
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>>  xen/arch/arm/gic.c  | 26 ++++++++++++++++++++++++++
>>  xen/arch/arm/vgic.c | 12 +++++++++++-
>>  2 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 67375a2..e175e9b 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -351,6 +351,7 @@ void gic_disable_cpu(void)
>>  static inline void gic_set_lr(int lr, struct pending_irq *p,
>>                                unsigned int state)
>>  {
>> +    ASSERT(spin_is_locked(&p->lock));
>>      ASSERT(!local_irq_is_enabled());
>>
>>      gic_hw_ops->update_lr(lr, p, state);
>> @@ -413,6 +414,7 @@ void gic_raise_guest_irq(struct vcpu *v, struct
>> pending_irq *p)
>>      unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
>>
>>      ASSERT(spin_is_locked(&v->arch.vgic.lock));
>> +    ASSERT(spin_is_locked(&p->lock));
>>
>>      if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
>>      {
>> @@ -439,6 +441,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>>      gic_hw_ops->read_lr(i, &lr_val);
>>      irq = lr_val.virq;
>>      p = irq_to_pending(v, irq);
>> +    spin_lock(&p->lock);
>>      if ( lr_val.state & GICH_LR_ACTIVE )
>>      {
>>          set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
>> @@ -495,6 +498,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>>              }
>>          }
>>      }
>> +    spin_unlock(&p->lock);
>>  }
>>
>>  void gic_clear_lrs(struct vcpu *v)
>> @@ -545,14 +549,30 @@ static void gic_restore_pending_irqs(struct vcpu
>> *v)
>>              /* No more free LRs: find a lower priority irq to evict */
>>              list_for_each_entry_reverse( p_r, inflight_r, inflight )
>>              {
>> +                if ( p_r->irq < p->irq )
>> +                {
>> +                    spin_lock(&p_r->lock);
>> +                    spin_lock(&p->lock);
>> +                }
>> +                else
>> +                {
>> +                    spin_lock(&p->lock);
>> +                    spin_lock(&p_r->lock);
>> +                }
> 
> Please explain in the commit message and the code why this locking order.
> 
>>                  if ( p_r->priority == p->priority )
>> +                {
>> +                    spin_unlock(&p->lock);
>> +                    spin_unlock(&p_r->lock);
>>                      goto out;
>> +                }
>>                  if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
>>                       !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
>>                      goto found;
>>              }
>>              /* We didn't find a victim this time, and we won't next
>>               * time, so quit */
>> +            spin_unlock(&p->lock);
>> +            spin_unlock(&p_r->lock);
>>              goto out;
>>
>>  found:
>> @@ -562,12 +582,18 @@ found:
>>              clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
>>              gic_add_to_lr_pending(v, p_r);
>>              inflight_r = &p_r->inflight;
>> +
>> +            spin_unlock(&p_r->lock);
>>          }
>> +        else
>> +            spin_lock(&p->lock);
>>
>>          gic_set_lr(lr, p, GICH_LR_PENDING);
>>          list_del_init(&p->lr_queue);
>>          set_bit(lr, &this_cpu(lr_mask));
>>
>> +        spin_unlock(&p->lock);
>> +
>>          /* We can only evict nr_lrs entries */
>>          lrs--;
>>          if ( lrs == 0 )
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index f4ae454..44363bb 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -356,11 +356,16 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t
>> r, int n)
>>      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>>          irq = i + (32 * n);
>>          v_target = vgic_get_target_vcpu(v, irq);
>> +
>> +        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
>>          p = irq_to_pending(v_target, irq);
>> +        spin_lock(&p->lock);
>> +
>>          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
>> -        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
> 
> IHMO, this should belong to a separate patch as not strictly relate to
> this one.

I don't think it makes much sense to split this, as this change is
motivated by the introduction of the pending_irq lock, and we have to
move the VGIC VCPU lock due to the locking order.

> 
>> +
> 
> Spurious change.

Well, that helps to structure the code.

>>          if ( !list_empty(&p->inflight) &&
>> !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>>              gic_raise_guest_irq(v_target, p);
>> +        spin_unlock(&p->lock);
>>          spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
>>          if ( p->desc != NULL )
>>          {
>> @@ -482,10 +487,12 @@ void vgic_vcpu_inject_irq(struct vcpu *v,
>> unsigned int virq)
>>          return;
>>      }
>>
>> +    spin_lock(&n->lock);
>>      set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
>>
>>      if ( !list_empty(&n->inflight) )
>>      {
>> +        spin_unlock(&n->lock);
>>          gic_raise_inflight_irq(v, n);
> 
> Any reason to not code gic_raise_inflight_irq with the lock? This would
> also simplify quite a lot the function and avoid unlock in pretty much
> all the exit path.

gic_raise_inflight_irq() calls gic_update_one_lr(), which works out the
pending_irq from the LR number and then takes the lock.
Yes, there are other ways of solving this:
- remove gic_raise_inflight_irq() at all
- rework gic_update_one_lr() to take a (locked) pending_irq already

Both approaches have other issues that pop up, I think this solution
here is the least hideous and least intrusive.
Frankly I believe that removing gic_raise_inflight_irq() altogether is
the best solution, but this requires more rework (which Stefano hinted
at not liking too much) and I wanted to keep this series as simple as
possible.

Cheers,
Andre.

>>          goto out;
>>      }
>> @@ -501,10 +508,13 @@ void vgic_vcpu_inject_irq(struct vcpu *v,
>> unsigned int virq)
>>          if ( iter->priority > priority )
>>          {
>>              list_add_tail(&n->inflight, &iter->inflight);
>> +            spin_unlock(&n->lock);
>>              goto out;
>>          }
>>      }
>>      list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
>> +    spin_unlock(&n->lock);
>> +
>>  out:
>>      spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>>      /* we have a new higher priority irq, inject it into the guest */
>>
> 
> Cheers,
> 

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.