| Message ID | 1491957874-31600-3-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 12/04/17 01:44, Andre Przywara wrote: > So far irq_to_pending() is just a convenience function to lookup > in statically allocated arrays. This will change with LPIs, which are > more dynamic. > So move the irq_to_pending() call under the VGIC VCPU lock, so we > only use this pointer while holding the lock. That's a call for an ASSERT in irq_to_pending. And you would have notice that not all irq_to_pending will then be protected by the vGIC lock (see vgic_migrate_irq for instance). Also, please explain why the vgic lock as technically irq_to_pending is lock agnostic... And LPI structure will be per domain. So how do you expect the locking to work? > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic.c | 5 ++++- > xen/arch/arm/vgic.c | 8 +++++--- > 2 files changed, 9 insertions(+), 4 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index da19130..dcb1783 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -402,10 +402,13 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n) > > void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) > { > - struct pending_irq *p = irq_to_pending(v, virtual_irq); > + struct pending_irq *p; > unsigned long flags; > > spin_lock_irqsave(&v->arch.vgic.lock, flags); > + > + p = irq_to_pending(v, virtual_irq); > + > if ( !list_empty(&p->lr_queue) ) > list_del_init(&p->lr_queue); > spin_unlock_irqrestore(&v->arch.vgic.lock, flags); > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 83569b0..c953f13 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -466,14 +466,16 @@ void vgic_clear_pending_irqs(struct vcpu *v) > void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) > { > uint8_t priority; > - struct pending_irq *iter, *n = irq_to_pending(v, virq); > + struct pending_irq *iter, *n; > unsigned long flags; > bool running; > > - priority = vgic_get_virq_priority(v, virq);; > - > spin_lock_irqsave(&v->arch.vgic.lock, flags); > > + n = irq_to_pending(v, virq); > + > + priority = vgic_get_virq_priority(v, virq); The commit message only speak about moving "irq_to_pending. So why the priority has been moved to? This will introduce a lock ordering issue (rank lock vs vgic lock) between vgic_vcpu_inject_irq and ITARGET/IROUTER emulation. The former is taking vgic lock first and then rank whilst the latter is doing the invert (see vgic_migrate_irq). > + > /* vcpu offline */ > if ( test_bit(_VPF_down, &v->pause_flags) ) > { > Cheers,
On 12/04/17 11:13, Julien Grall wrote: > Hi Andre, > > On 12/04/17 01:44, Andre Przywara wrote: >> So far irq_to_pending() is just a convenience function to lookup >> in statically allocated arrays. This will change with LPIs, which are >> more dynamic. >> So move the irq_to_pending() call under the VGIC VCPU lock, so we >> only use this pointer while holding the lock. > > That's a call for an ASSERT in irq_to_pending. And you would have notice > that not all irq_to_pending will then be protected by the vGIC lock (see > vgic_migrate_irq for instance). > > Also, please explain why the vgic lock as technically irq_to_pending is > lock agnostic... And LPI structure will be per domain. So how do you > expect the locking to work? I thought a bit more about this and I think vgic_vcpu_inject_irq will not be protected correctly for LPI. Unlike SPIs, LPIs don't have active state in the GIC so theoretically a new interrupt can come up right after handling the first one. Although, it might have been possible that the vLPI was moved from vCPU A to vCPU B and still in the LRs (not yet handled by the guest or not yet cleaned). So there is a race between gic_update_one_lr and vgic_vcpu_inject, both will take a different lock (resp. old and new) which may lead to a list corruption. I am not sure how to protect this case as this could happen because of a "DISCARD -> MAPTI", "MOVI", "MOVALL" > >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/gic.c | 5 ++++- >> xen/arch/arm/vgic.c | 8 +++++--- >> 2 files changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index da19130..dcb1783 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -402,10 +402,13 @@ static inline void gic_add_to_lr_pending(struct >> vcpu *v, struct pending_irq *n) >> >> void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) >> { >> - struct pending_irq *p = irq_to_pending(v, virtual_irq); >> + struct pending_irq *p; >> unsigned long flags; >> >> spin_lock_irqsave(&v->arch.vgic.lock, flags); >> + >> + p = irq_to_pending(v, virtual_irq); >> + >> if ( !list_empty(&p->lr_queue) ) >> list_del_init(&p->lr_queue); >> spin_unlock_irqrestore(&v->arch.vgic.lock, flags); >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index 83569b0..c953f13 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -466,14 +466,16 @@ void vgic_clear_pending_irqs(struct vcpu *v) >> void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) >> { >> uint8_t priority; >> - struct pending_irq *iter, *n = irq_to_pending(v, virq); >> + struct pending_irq *iter, *n; >> unsigned long flags; >> bool running; >> >> - priority = vgic_get_virq_priority(v, virq);; > >> - >> spin_lock_irqsave(&v->arch.vgic.lock, flags); >> >> + n = irq_to_pending(v, virq); >> + >> + priority = vgic_get_virq_priority(v, virq); > > The commit message only speak about moving "irq_to_pending. So why the > priority has been moved to? > > This will introduce a lock ordering issue (rank lock vs vgic lock) > between vgic_vcpu_inject_irq and ITARGET/IROUTER emulation. > > The former is taking vgic lock first and then rank whilst the latter is > doing the invert (see vgic_migrate_irq). > >> + >> /* vcpu offline */ >> if ( test_bit(_VPF_down, &v->pause_flags) ) >> { >> > > Cheers, >
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index da19130..dcb1783 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -402,10 +402,13 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n) void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq) { - struct pending_irq *p = irq_to_pending(v, virtual_irq); + struct pending_irq *p; unsigned long flags; spin_lock_irqsave(&v->arch.vgic.lock, flags); + + p = irq_to_pending(v, virtual_irq); + if ( !list_empty(&p->lr_queue) ) list_del_init(&p->lr_queue); spin_unlock_irqrestore(&v->arch.vgic.lock, flags); diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 83569b0..c953f13 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -466,14 +466,16 @@ void vgic_clear_pending_irqs(struct vcpu *v) void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) { uint8_t priority; - struct pending_irq *iter, *n = irq_to_pending(v, virq); + struct pending_irq *iter, *n; unsigned long flags; bool running; - priority = vgic_get_virq_priority(v, virq); - spin_lock_irqsave(&v->arch.vgic.lock, flags); + n = irq_to_pending(v, virq); + + priority = vgic_get_virq_priority(v, virq); + /* vcpu offline */ if ( test_bit(_VPF_down, &v->pause_flags) ) {
So far irq_to_pending() is just a convenience function to lookup in statically allocated arrays. This will change with LPIs, which are more dynamic. So move the irq_to_pending() call under the VGIC VCPU lock, so we only use this pointer while holding the lock. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic.c | 5 ++++- xen/arch/arm/vgic.c | 8 +++++--- 2 files changed, 9 insertions(+), 4 deletions(-)