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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.

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 <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