| Message ID | 20170526173540.10066-4-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 26/05/17 18:35, Andre Przywara wrote: > So far irq_to_pending() is just a convenience function to lookup > statically allocated arrays. This will change with LPIs, which are > more dynamic, so the memory for their struct pending_irq might go away. > The proper answer to the issue of preventing stale pointers is > ref-counting, which requires more rework and will be introduced with > a later rework. > For now move the irq_to_pending() calls that are used with LPIs under the > VGIC VCPU lock, and only use the returned pointer while holding the lock. > This prevents the memory from being freed while we use it. > For the sake of completeness we take care about all irq_to_pending() > users, even those which later will never deal with LPIs. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic.c | 5 ++++- > xen/arch/arm/vgic.c | 39 ++++++++++++++++++++++++++++++--------- > 2 files changed, 34 insertions(+), 10 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 54b2aad..69d732b 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -234,23 +234,29 @@ static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq) > bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) > { > unsigned long flags; > - struct pending_irq *p = irq_to_pending(old, irq); > + struct pending_irq *p; > + > + spin_lock_irqsave(&old->arch.vgic.lock, flags); > + > + p = irq_to_pending(old, irq); > > /* nothing to do for virtual interrupts */ > if ( p->desc == NULL ) > + { > + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); > return true; > + } > > /* migration already in progress, no need to do anything */ > if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) ) > { > gprintk(XENLOG_WARNING, "irq %u migration failed: requested while in progress\n", irq); > + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); > return false; > } > > perfc_incr(vgic_irq_migrates); > > - spin_lock_irqsave(&old->arch.vgic.lock, flags); > - > if ( list_empty(&p->inflight) ) > { > irq_set_affinity(p->desc, cpumask_of(new->processor)); > @@ -285,6 +291,13 @@ void arch_move_irqs(struct vcpu *v) > struct vcpu *v_target; > int i; > > + /* > + * We don't migrate LPIs at the moment. > + * If we ever do, we must make sure that the struct pending_irq does > + * not go away, as there is no lock preventing this here. > + */ > + ASSERT(!is_lpi(vgic_num_irqs(d) - 1)); Hmmm? This raise a question of why vgic_num_irqs does not include the LPIs today... > + > for ( i = 32; i < vgic_num_irqs(d); i++ ) > { > v_target = vgic_get_target_vcpu(v, i); > @@ -299,6 +312,7 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n) > { > const unsigned long mask = r; > struct pending_irq *p; > + struct irq_desc *desc; > unsigned int irq; > unsigned long flags; > int i = 0; > @@ -307,14 +321,19 @@ void vgic_disable_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); > clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > gic_remove_from_queues(v_target, irq); gic_remove_from_queues is taking v_target vGIC lock. So you just introduced a deadlock. You remove it in the next patch but still, we should not introduce regression even temporarily. This would make to difficult to bisect the series. TBH, I am not a big fan of spreading the mess of vGIC locking when we are going to rework the vGIC and know that this code will not be called for LPIs. BTW, this series is not bisectable because the host ITS is only enabled from patch #12. > - if ( p->desc != NULL ) > + desc = p->desc; > + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); > + > + if ( desc != NULL ) > { > - spin_lock_irqsave(&p->desc->lock, flags); > - p->desc->handler->disable(p->desc); > - spin_unlock_irqrestore(&p->desc->lock, flags); > + spin_lock_irqsave(&desc->lock, flags); > + desc->handler->disable(desc); > + spin_unlock_irqrestore(&desc->lock, flags); > } > i++; > } > @@ -349,9 +368,9 @@ 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); > 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, irq, p->priority); > spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); > @@ -460,7 +479,7 @@ 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; > > @@ -468,6 +487,8 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) > > spin_lock_irqsave(&v->arch.vgic.lock, flags); > > + n = irq_to_pending(v, virq); > + > /* vcpu offline */ > if ( test_bit(_VPF_down, &v->pause_flags) ) > { > Cheers,
On Tue, 30 May 2017, Julien Grall wrote: > Hi Andre, > > On 26/05/17 18:35, Andre Przywara wrote: > > So far irq_to_pending() is just a convenience function to lookup > > statically allocated arrays. This will change with LPIs, which are > > more dynamic, so the memory for their struct pending_irq might go away. > > The proper answer to the issue of preventing stale pointers is > > ref-counting, which requires more rework and will be introduced with > > a later rework. > > For now move the irq_to_pending() calls that are used with LPIs under the > > VGIC VCPU lock, and only use the returned pointer while holding the lock. > > This prevents the memory from being freed while we use it. > > For the sake of completeness we take care about all irq_to_pending() > > users, even those which later will never deal with LPIs. > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > xen/arch/arm/gic.c | 5 ++++- > > xen/arch/arm/vgic.c | 39 ++++++++++++++++++++++++++++++--------- > > 2 files changed, 34 insertions(+), 10 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 54b2aad..69d732b 100644 > > --- a/xen/arch/arm/vgic.c > > +++ b/xen/arch/arm/vgic.c > > @@ -234,23 +234,29 @@ static int vgic_get_virq_priority(struct vcpu *v, > > unsigned int virq) > > bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) > > { > > unsigned long flags; > > - struct pending_irq *p = irq_to_pending(old, irq); > > + struct pending_irq *p; > > + > > + spin_lock_irqsave(&old->arch.vgic.lock, flags); > > + > > + p = irq_to_pending(old, irq); > > > > /* nothing to do for virtual interrupts */ > > if ( p->desc == NULL ) > > + { > > + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); > > return true; > > + } > > > > /* migration already in progress, no need to do anything */ > > if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) ) > > { > > gprintk(XENLOG_WARNING, "irq %u migration failed: requested while > > in progress\n", irq); > > + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); > > return false; > > } > > > > perfc_incr(vgic_irq_migrates); > > > > - spin_lock_irqsave(&old->arch.vgic.lock, flags); > > - > > if ( list_empty(&p->inflight) ) > > { > > irq_set_affinity(p->desc, cpumask_of(new->processor)); > > @@ -285,6 +291,13 @@ void arch_move_irqs(struct vcpu *v) > > struct vcpu *v_target; > > int i; > > > > + /* > > + * We don't migrate LPIs at the moment. > > + * If we ever do, we must make sure that the struct pending_irq does > > + * not go away, as there is no lock preventing this here. > > + */ > > + ASSERT(!is_lpi(vgic_num_irqs(d) - 1)); > > Hmmm? This raise a question of why vgic_num_irqs does not include the LPIs > today... > > > + > > for ( i = 32; i < vgic_num_irqs(d); i++ ) > > { > > v_target = vgic_get_target_vcpu(v, i); > > @@ -299,6 +312,7 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int > > n) > > { > > const unsigned long mask = r; > > struct pending_irq *p; > > + struct irq_desc *desc; > > unsigned int irq; > > unsigned long flags; > > int i = 0; > > @@ -307,14 +321,19 @@ void vgic_disable_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); > > clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); > > gic_remove_from_queues(v_target, irq); > > gic_remove_from_queues is taking v_target vGIC lock. So you just introduced a > deadlock. You remove it in the next patch but still, we should not introduce > regression even temporarily. This would make to difficult to bisect the > series. Yeah, we cannot introduce regressions like this one. > TBH, I am not a big fan of spreading the mess of vGIC locking when we are > going to rework the vGIC and know that this code will not be called for LPIs. I asked for this in alpine.DEB.2.10.1705191729560.18759@sstabellini-ThinkPad-X260, this way we covered all basis. The double lock is bad, but the rest of the changes look OK to me. > BTW, this series is not bisectable because the host ITS is only enabled from > patch #12. > > > - if ( p->desc != NULL ) > > + desc = p->desc; > > + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); > > + > > + if ( desc != NULL ) > > { > > - spin_lock_irqsave(&p->desc->lock, flags); > > - p->desc->handler->disable(p->desc); > > - spin_unlock_irqrestore(&p->desc->lock, flags); > > + spin_lock_irqsave(&desc->lock, flags); > > + desc->handler->disable(desc); > > + spin_unlock_irqrestore(&desc->lock, flags); > > } > > i++; > > } > > @@ -349,9 +368,9 @@ 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); > > 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, irq, p->priority); > > spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); > > @@ -460,7 +479,7 @@ 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; > > > > @@ -468,6 +487,8 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int > > virq) > > > > spin_lock_irqsave(&v->arch.vgic.lock, flags); > > > > + n = irq_to_pending(v, virq); > > + > > /* vcpu offline */ > > if ( test_bit(_VPF_down, &v->pause_flags) ) > > { > > > > Cheers, > > -- > Julien Grall >
Hi Stefano, On 30/05/17 22:46, Stefano Stabellini wrote: > On Tue, 30 May 2017, Julien Grall wrote >> On 26/05/17 18:35, Andre Przywara wrote: >> TBH, I am not a big fan of spreading the mess of vGIC locking when we are >> going to rework the vGIC and know that this code will not be called for LPIs. > > I asked for this in > alpine.DEB.2.10.1705191729560.18759@sstabellini-ThinkPad-X260, this way > we covered all basis. The double lock is bad, but the rest of the > changes look OK to me. It is just adding more churn that will have to be reworked when fixing the vGIC. Anyway, I am not going to argue on that. Cheers,
Hi, On 30/05/17 12:08, Julien Grall wrote: > Hi Andre, > > On 26/05/17 18:35, Andre Przywara wrote: >> So far irq_to_pending() is just a convenience function to lookup >> statically allocated arrays. This will change with LPIs, which are >> more dynamic, so the memory for their struct pending_irq might go away. >> The proper answer to the issue of preventing stale pointers is >> ref-counting, which requires more rework and will be introduced with >> a later rework. >> For now move the irq_to_pending() calls that are used with LPIs under the >> VGIC VCPU lock, and only use the returned pointer while holding the lock. >> This prevents the memory from being freed while we use it. >> For the sake of completeness we take care about all irq_to_pending() >> users, even those which later will never deal with LPIs. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/gic.c | 5 ++++- >> xen/arch/arm/vgic.c | 39 ++++++++++++++++++++++++++++++--------- >> 2 files changed, 34 insertions(+), 10 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 54b2aad..69d732b 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -234,23 +234,29 @@ static int vgic_get_virq_priority(struct vcpu >> *v, unsigned int virq) >> bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned >> int irq) >> { >> unsigned long flags; >> - struct pending_irq *p = irq_to_pending(old, irq); >> + struct pending_irq *p; >> + >> + spin_lock_irqsave(&old->arch.vgic.lock, flags); >> + >> + p = irq_to_pending(old, irq); >> >> /* nothing to do for virtual interrupts */ >> if ( p->desc == NULL ) >> + { >> + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); >> return true; >> + } >> >> /* migration already in progress, no need to do anything */ >> if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) ) >> { >> gprintk(XENLOG_WARNING, "irq %u migration failed: requested >> while in progress\n", irq); >> + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); >> return false; >> } >> >> perfc_incr(vgic_irq_migrates); >> >> - spin_lock_irqsave(&old->arch.vgic.lock, flags); >> - >> if ( list_empty(&p->inflight) ) >> { >> irq_set_affinity(p->desc, cpumask_of(new->processor)); >> @@ -285,6 +291,13 @@ void arch_move_irqs(struct vcpu *v) >> struct vcpu *v_target; >> int i; >> >> + /* >> + * We don't migrate LPIs at the moment. >> + * If we ever do, we must make sure that the struct pending_irq does >> + * not go away, as there is no lock preventing this here. >> + */ >> + ASSERT(!is_lpi(vgic_num_irqs(d) - 1)); > > Hmmm? This raise a question of why vgic_num_irqs does not include the > LPIs today... I admit that this line is a bit puzzling. I guess vgic_num_irqs() should be renamed to something like vgic_max_spis(), but this wouldn't be exactly the right name either. I will extend the comment to note that the current code assumes vgic_num_irqs() only covers SPIs. Is that OK? >> + >> for ( i = 32; i < vgic_num_irqs(d); i++ ) >> { >> v_target = vgic_get_target_vcpu(v, i); >> @@ -299,6 +312,7 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, >> int n) >> { >> const unsigned long mask = r; >> struct pending_irq *p; >> + struct irq_desc *desc; >> unsigned int irq; >> unsigned long flags; >> int i = 0; >> @@ -307,14 +321,19 @@ void vgic_disable_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); >> clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); >> gic_remove_from_queues(v_target, irq); > > gic_remove_from_queues is taking v_target vGIC lock. So you just > introduced a deadlock. You remove it in the next patch but still, we > should not introduce regression even temporarily. This would make to > difficult to bisect the series. Good point, thanks for spotting this. I will try to address this. I compile-tested every single patch, but didn't try to boot every one. > TBH, I am not a big fan of spreading the mess of vGIC locking when we > are going to rework the vGIC and know that this code will not be called > for LPIs. > > BTW, this series is not bisectable because the host ITS is only enabled > from patch #12. I moved that to the front now. TBH I spotted this issue before, and had a simpler version of patch #12 to plug this. Maybe we should consider to merge this one for 4.9 still, as currently enabling the ITS in .config and running it on an ITS machine will fail to boot Dom0. Cheers, Andre. > >> - if ( p->desc != NULL ) >> + desc = p->desc; >> + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); >> + >> + if ( desc != NULL ) >> { >> - spin_lock_irqsave(&p->desc->lock, flags); >> - p->desc->handler->disable(p->desc); >> - spin_unlock_irqrestore(&p->desc->lock, flags); >> + spin_lock_irqsave(&desc->lock, flags); >> + desc->handler->disable(desc); >> + spin_unlock_irqrestore(&desc->lock, flags); >> } >> i++; >> } >> @@ -349,9 +368,9 @@ 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); >> 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, irq, p->priority); >> spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); >> @@ -460,7 +479,7 @@ 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; >> >> @@ -468,6 +487,8 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned >> int virq) >> >> spin_lock_irqsave(&v->arch.vgic.lock, flags); >> >> + n = irq_to_pending(v, virq); >> + >> /* vcpu offline */ >> if ( test_bit(_VPF_down, &v->pause_flags) ) >> { >> > > Cheers, >
On Tue, 6 Jun 2017, Andre Przywara wrote: > Maybe we should consider to merge this one for 4.9 still, > as currently enabling the ITS in .config and running it on an ITS > machine will fail to boot Dom0. Here, you are talking about this patch, patch #3, right? Although it should be "safe", it touches a lot of common code. I think it is too risky to commit it now to fix an experimental feature. I would rather wait until it is committed to staging in the 4.10 dev window, then backport it. > >> - if ( p->desc != NULL ) > >> + desc = p->desc; > >> + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); > >> + > >> + if ( desc != NULL ) > >> { > >> - spin_lock_irqsave(&p->desc->lock, flags); > >> - p->desc->handler->disable(p->desc); > >> - spin_unlock_irqrestore(&p->desc->lock, flags); > >> + spin_lock_irqsave(&desc->lock, flags); > >> + desc->handler->disable(desc); > >> + spin_unlock_irqrestore(&desc->lock, flags); > >> } > >> i++; > >> } > >> @@ -349,9 +368,9 @@ 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); > >> 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, irq, p->priority); > >> spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); > >> @@ -460,7 +479,7 @@ 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; > >> > >> @@ -468,6 +487,8 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned > >> int virq) > >> > >> spin_lock_irqsave(&v->arch.vgic.lock, flags); > >> > >> + n = irq_to_pending(v, virq); > >> + > >> /* vcpu offline */ > >> if ( test_bit(_VPF_down, &v->pause_flags) ) > >> { > >>
Hi, On 06/06/17 19:46, Stefano Stabellini wrote: > On Tue, 6 Jun 2017, Andre Przywara wrote: >> Maybe we should consider to merge this one for 4.9 still, >> as currently enabling the ITS in .config and running it on an ITS >> machine will fail to boot Dom0. > > Here, you are talking about this patch, patch #3, right? No, sorry, I meant patch 12. > Although it should be "safe", it touches a lot of common code. I think > it is too risky to commit it now to fix an experimental feature. I would > rather wait until it is committed to staging in the 4.10 dev window, > then backport it. Well, if you compile origin/staging with the ITS configured in and run this on a machine with an ITS in the DT, it will not boot (regardless of the ITS not being used anyway). This is because we try to map collections on the host (using MAPC commands), but don't enable the host ITS, so those commands are not executed and we time out. I discovered this shortly after you merged the first ITS patches and made a simple patch, but for some reasons this didn't end up on the public list. Let me send this out and we can decide whether we need this for 4.9 still. Cheers, Andre. >>>> - if ( p->desc != NULL ) >>>> + desc = p->desc; >>>> + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); >>>> + >>>> + if ( desc != NULL ) >>>> { >>>> - spin_lock_irqsave(&p->desc->lock, flags); >>>> - p->desc->handler->disable(p->desc); >>>> - spin_unlock_irqrestore(&p->desc->lock, flags); >>>> + spin_lock_irqsave(&desc->lock, flags); >>>> + desc->handler->disable(desc); >>>> + spin_unlock_irqrestore(&desc->lock, flags); >>>> } >>>> i++; >>>> } >>>> @@ -349,9 +368,9 @@ 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); >>>> 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, irq, p->priority); >>>> spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); >>>> @@ -460,7 +479,7 @@ 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; >>>> >>>> @@ -468,6 +487,8 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned >>>> int virq) >>>> >>>> spin_lock_irqsave(&v->arch.vgic.lock, flags); >>>> >>>> + n = irq_to_pending(v, virq); >>>> + >>>> /* vcpu offline */ >>>> if ( test_bit(_VPF_down, &v->pause_flags) ) >>>> { >>>>
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 54b2aad..69d732b 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -234,23 +234,29 @@ static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq) bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) { unsigned long flags; - struct pending_irq *p = irq_to_pending(old, irq); + struct pending_irq *p; + + spin_lock_irqsave(&old->arch.vgic.lock, flags); + + p = irq_to_pending(old, irq); /* nothing to do for virtual interrupts */ if ( p->desc == NULL ) + { + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); return true; + } /* migration already in progress, no need to do anything */ if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) ) { gprintk(XENLOG_WARNING, "irq %u migration failed: requested while in progress\n", irq); + spin_unlock_irqrestore(&old->arch.vgic.lock, flags); return false; } perfc_incr(vgic_irq_migrates); - spin_lock_irqsave(&old->arch.vgic.lock, flags); - if ( list_empty(&p->inflight) ) { irq_set_affinity(p->desc, cpumask_of(new->processor)); @@ -285,6 +291,13 @@ void arch_move_irqs(struct vcpu *v) struct vcpu *v_target; int i; + /* + * We don't migrate LPIs at the moment. + * If we ever do, we must make sure that the struct pending_irq does + * not go away, as there is no lock preventing this here. + */ + ASSERT(!is_lpi(vgic_num_irqs(d) - 1)); + for ( i = 32; i < vgic_num_irqs(d); i++ ) { v_target = vgic_get_target_vcpu(v, i); @@ -299,6 +312,7 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n) { const unsigned long mask = r; struct pending_irq *p; + struct irq_desc *desc; unsigned int irq; unsigned long flags; int i = 0; @@ -307,14 +321,19 @@ void vgic_disable_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); clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status); gic_remove_from_queues(v_target, irq); - if ( p->desc != NULL ) + desc = p->desc; + spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); + + if ( desc != NULL ) { - spin_lock_irqsave(&p->desc->lock, flags); - p->desc->handler->disable(p->desc); - spin_unlock_irqrestore(&p->desc->lock, flags); + spin_lock_irqsave(&desc->lock, flags); + desc->handler->disable(desc); + spin_unlock_irqrestore(&desc->lock, flags); } i++; } @@ -349,9 +368,9 @@ 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); 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, irq, p->priority); spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags); @@ -460,7 +479,7 @@ 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; @@ -468,6 +487,8 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq) spin_lock_irqsave(&v->arch.vgic.lock, flags); + n = irq_to_pending(v, virq); + /* vcpu offline */ if ( test_bit(_VPF_down, &v->pause_flags) ) {
So far irq_to_pending() is just a convenience function to lookup statically allocated arrays. This will change with LPIs, which are more dynamic, so the memory for their struct pending_irq might go away. The proper answer to the issue of preventing stale pointers is ref-counting, which requires more rework and will be introduced with a later rework. For now move the irq_to_pending() calls that are used with LPIs under the VGIC VCPU lock, and only use the returned pointer while holding the lock. This prevents the memory from being freed while we use it. For the sake of completeness we take care about all irq_to_pending() users, even those which later will never deal with LPIs. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic.c | 5 ++++- xen/arch/arm/vgic.c | 39 ++++++++++++++++++++++++++++++--------- 2 files changed, 34 insertions(+), 10 deletions(-)