|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |