|
[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
On Fri, 5 May 2017, Andre Przywara wrote:
> 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.
Just to be clear: I like any rework/refactoring you can come up with,
*after* ITS :-)
> >> 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 */
> >>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |