Re: [Xen-devel] [PATCH 7/9] arm: vgic: fix race between evtchn upcall and evtchnop_send

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-03-19 at 16:18 +0000, Stefano Stabellini wrote:
> On Wed, 6 Mar 2013, Ian Campbell wrote:
> > From: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > 
> > 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.

So I did. My Use of "We therefore.." suggests I've managed to drop a
paragraph from the middle of the commit message, which probably would
have mentioned this ;-)

> Aside from this, the patch is fine.

Thanks.

> 
> 
> > 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 <ian.campbell@xxxxxxxxxx>
> > ---
> >  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
> > 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel