From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH 7/9] arm: vgic: fix race between evtchn upcall and evtchnop_send Date: Tue, 19 Mar 2013 16:18:11 +0000 Message-ID: References: <1362559920.8941.98.camel@hastur.hellion.org.uk> <1362560076-25897-7-git-send-email-ijc@hellion.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1362560076-25897-7-git-send-email-ijc@hellion.org.uk> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Stefano Stabellini , "Tim (Xen.org)" , Ian Campbell , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On Wed, 6 Mar 2013, Ian Campbell wrote: > From: Ian Campbell > > On ARM the evtchn upcall is done by using a local PPI interrupt. However the > guest will clear the evtchn_upcall_pending bit before it EOIs that PPI (which > happens late). This means vgic_vcpu_inject_irq (called via > vcpu_mark_events_pending) sees the PPI as in flight and ends up not reinjecting > it, if this happens after the guest has finished its event channel processing > loop but before the EOI then we have lost the upcall. > > We therefore also need to call gic_restore_pending_irqs on the exit to guest > path in order to pickup any newly inject IRQ and propagate it into a free LR. You forgot to mention that on exit to guest, before calling gic_restore_pending_irqs, we check whether we need to reinject the PPI. Aside from this, the patch is fine. > This doesn't currently support bumping a lower priority interrupt out of the > LRs in order to inject a new higher priority interrupt. We don't yet implement > interrupt prioritisation (and guests don't use it either) so this will do for > now. > > Since gic_restore_pending_irqs is now called in the return to guest path it is > called with interrupts disabled and accordinly must use the irqsave/irqrestore > spinlock primitives. > > Signed-off-by: Ian Campbell > --- > xen/arch/arm/gic.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 6592562..59e007a 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -556,17 +556,18 @@ static void gic_restore_pending_irqs(struct vcpu *v) > { > int i; > struct pending_irq *p, *t; > + unsigned long flags; > > list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue ) > { > i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs); > if ( i >= nr_lrs ) return; > > - spin_lock_irq(&gic.lock); > + spin_lock_irqsave(&gic.lock, flags); > gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority); > list_del_init(&p->lr_queue); > set_bit(i, &this_cpu(lr_mask)); > - spin_unlock_irq(&gic.lock); > + spin_unlock_irqrestore(&gic.lock, flags); > } > > } > @@ -589,6 +590,10 @@ static void gic_inject_irq_stop(void) > > void gic_inject(void) > { > + if ( vcpu_info(current, evtchn_upcall_pending) ) > + vgic_vcpu_inject_irq(current, VGIC_IRQ_EVTCHN_CALLBACK, 1); > + > + gic_restore_pending_irqs(current); > if (!this_cpu(lr_mask)) > gic_inject_irq_stop(); > else > -- > 1.7.10.4 >