| Message ID | 20170504153123.1204-5-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@arm.com> > --- > 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. > + Spurious change. > 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. > 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,
On 04/05/17 16:31, Andre Przywara wrote: > 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); > + > if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) ) > gic_raise_guest_irq(v_target, p); > + spin_unlock(&p->lock); Why does the lock not cover p->desc below? > spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); > if ( p->desc != NULL ) > {
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@arm.com> >> --- >> 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, >
On Thu, 4 May 2017, Julien Grall wrote: > On 04/05/17 16:31, Andre Przywara wrote: > > 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); > > + > > if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, > > &p->status) ) > > gic_raise_guest_irq(v_target, p); > > + spin_unlock(&p->lock); > > Why does the lock not cover p->desc below? Indeed. The main problem with this patch is that it doesn't say what this lock is supposed to cover. It is OK for the lock not to cover everything pending_irq related as long as it is clear. As it stands, it is not clear. For example, why the lock is not added to following functions? - gic_route_irq_to_guest - gic_remove_irq_from_guest - gic_remove_from_queues - gic_raise_inflight_irq - vgic_migrate_irq - arch_move_irqs - vgic_disable_irqs I came up with this list by grepping for irq_to_pending and listing the function where fields of the pending_irq struct are accessed. > > spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); > > if ( p->desc != NULL ) > > {
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@arm.com> > >> --- > >> 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 */ > >>
On Thu, 4 May 2017, Julien Grall wrote: > > @@ -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. Yes, please add a couple of lines of comment to the code too. However, the code looks correct. > > 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 )
Hi Andre, On 05/05/2017 10:02 AM, 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@arm.com> >>> --- >>> 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. My point here is this change does not require to be specifically in this patch. And moving it in a separate patch would allow you to justify this change and simplify the patch. > >> >>> + >> >> Spurious change. > > Well, that helps to structure the code. I call that code clean-up and should be moved in separate patch. A general rule to make the patches as small as possible. This makes easier to review and justify changes. > >>> 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. The problem I can see with this patch series is it is not going far enough. Without the rest of the vgic rework, it is hard to say whether the locking order or placement of vgic_{get,put}_* are valid. For instance, as we discussed yesterday face to face. In this series you are suggesting the locking order: 1) vgic vcpu lock 2) pending_irq lock Because of this locking order, you may need in some places to take the pending_irq lock temporarily, drop it then take the locking in the correct order. I do believe that we can limit the use of vgic vcpu lock (it mostly protect the list in pending_irq) and we could use the locking order: 1) pending_irq lock 2) vgic vcpu lock This would also simplify the locking afterwards. But the correct ordering can only be decided with a full vGIC rework in mind. What I want to avoid is getting merged a lock ordering today, but in a couple of weeks we have a new series with a different locking ordering. Cheers,
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); + } 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); + 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); 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 */
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). Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic.c | 26 ++++++++++++++++++++++++++ xen/arch/arm/vgic.c | 12 +++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-)