[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH v2 11/22] ARM: vGIC: protect gic_events_need_delivery() with pending_irq lock
Hi Andre, On 21/07/17 20:59, Andre Przywara wrote: gic_events_need_delivery() reads the cur_priority field twice, also This does not seem inline with what we discussed. I.e cur_priority can be read without the pending_irq lock, assuming proper barrier are around. If the problem is reading it twice, then an ACCESS_ONCE(...) should fix it. relies on the consistency of status bits. status has been designed to be used without lock. If this is not the case anymore, then we should document it. In this particular case, I need a bit more context to see why this lock is necessary. IHMO, there are other way to avoid it, such as reading both the priority and the enabled bit before hand. So it should take pending_irq lock. Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx> --- xen/arch/arm/gic.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index df89530..9637682 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -666,7 +666,7 @@ int gic_events_need_delivery(void) { struct vcpu *v = current; struct pending_irq *p; - unsigned long flags; + unsigned long flags, vcpu_flags; const unsigned long apr = gic_hw_ops->read_apr(0); int mask_priority; int active_priority; @@ -675,7 +675,7 @@ int gic_events_need_delivery(void) mask_priority = gic_hw_ops->read_vmcr_priority(); active_priority = find_next_bit(&apr, 32, 0); - spin_lock_irqsave(&v->arch.vgic.lock, flags); + spin_lock_irqsave(&v->arch.vgic.lock, vcpu_flags); /* TODO: We order the guest irqs by priority, but we don't change * the priority of host irqs. */ @@ -684,19 +684,21 @@ int gic_events_need_delivery(void) * ordered by priority */ list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight ) { - if ( GIC_PRI_TO_GUEST(p->cur_priority) >= mask_priority ) - goto out; - if ( GIC_PRI_TO_GUEST(p->cur_priority) >= active_priority ) - goto out; - if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) + vgic_irq_lock(p, flags); + if ( GIC_PRI_TO_GUEST(p->cur_priority) < mask_priority && + GIC_PRI_TO_GUEST(p->cur_priority) < active_priority && + !test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) { - rc = 1; - goto out; + vgic_irq_unlock(p, flags); + continue; } + + rc = test_bit(GIC_IRQ_GUEST_ENABLED, &p->status); + vgic_irq_unlock(p, flags); + break; } -out: - spin_unlock_irqrestore(&v->arch.vgic.lock, flags); + spin_unlock_irqrestore(&v->arch.vgic.lock, vcpu_flags); return rc; } Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |