Xen devel - May 2013 - [PATCH v2] xen/gic: EOI irqs on the right pcpu

We need to write the irq number to GICC_DIR on the physical cpu that
previously received the interrupt, but currently we are doing it on the
pcpu that received the maintenance interrupt. As a consequence if a
vcpu is migrated to a different pcpu, the irq is going to be EOI''ed on
the wrong pcpu.

This covers the case where dom0 vcpu0 is running on pcpu1 for example
(you can test this scenario by using xl vcpu-pin).


Changes in v2:
- pass virq by value to gic_irq_eoi;
- EOI the interrupt without any spin locks held and with interrupt
enabled.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: keir@xen.org
---
 xen/arch/arm/gic.c           |   24 +++++++++++++++++++++++-
 xen/arch/arm/vgic.c          |    7 ++++++-
 xen/include/asm-arm/domain.h |    2 ++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 61de230..2674f84 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -723,6 +723,12 @@ int gicv_setup(struct domain *d)
                         gic.vbase);
 }
 
+static void gic_irq_eoi(void *info)
+{
+    int virq = (int) info;
+    GICC[GICC_DIR] = virq;
+}
+
 static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs
*regs)
 {
     int i = 0, virq;
@@ -733,6 +739,10 @@ static void maintenance_interrupt(int irq, void *dev_id,
struct cpu_user_regs *r
     while ((i = find_next_bit((const long unsigned int *) &eisr,
                               64, i)) < 64) {
         struct pending_irq *p;
+        int cpu, eoi;
+
+        cpu = -1;
+        eoi = 0;
 
         spin_lock_irq(&gic.lock);
         lr = GICH[GICH_LR + i];
@@ -754,11 +764,23 @@ static void maintenance_interrupt(int irq, void *dev_id,
struct cpu_user_regs *r
         p = irq_to_pending(v, virq);
         if ( p->desc != NULL ) {
             p->desc->status &= ~IRQ_INPROGRESS;
-            GICC[GICC_DIR] = virq;
+            /* Assume only one pcpu needs to EOI the irq */
+            cpu = cpumask_first(&p->eoimask);
+            cpumask_clear(&p->eoimask);
+            eoi = 1;
         }
         list_del_init(&p->inflight);
         spin_unlock_irq(&v->arch.vgic.lock);
 
+        if ( eoi ) {
+            /* this is not racy because we can''t receive another irq
of the
+             * same type until we EOI it.  */
+            if ( cpu == smp_processor_id() )
+                gic_irq_eoi((void*)virq);
+            else
+                on_selected_cpus(cpumask_of(cpu), gic_irq_eoi, (void*)virq, 0);
+        }
+
         i++;
     }
 }
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index f9c1a6b..c5370d5 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -676,9 +676,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq,
int virtual)
     n->irq = irq;
     n->priority = priority;
     if (!virtual)
+    {
         n->desc = irq_to_desc(irq);
-    else
+        cpumask_clear(&n->eoimask);
+        /* Assume we received the IRQ on the current pcpu */
+        cpumask_set_cpu(smp_processor_id(), &n->eoimask);
+    } else {
         n->desc = NULL;
+    }
 
     /* the irq is enabled */
     if ( rank->ienable & (1 << (irq % 32)) )
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index cca7416..5561531 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -3,6 +3,7 @@
 
 #include <xen/config.h>
 #include <xen/cache.h>
+#include <xen/cpumask.h>
 #include <xen/sched.h>
 #include <asm/page.h>
 #include <asm/p2m.h>
@@ -21,6 +22,7 @@ struct pending_irq
 {
     int irq;
     struct irq_desc *desc; /* only set it the irq corresponds to a physical irq
*/
+    cpumask_t eoimask;
     uint8_t priority;
     /* inflight is used to append instances of pending_irq to
      * vgic.inflight_irqs */
-- 
1.7.2.5

> @@ -733,6 +739,10 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>      while ((i = find_next_bit((const long unsigned int *) &eisr,
>                                64, i)) < 64) {
>          struct pending_irq *p;
> +        int cpu, eoi;
> +
> +        cpu = -1;
> +        eoi = 0;
>  
>          spin_lock_irq(&gic.lock);
>          lr = GICH[GICH_LR + i];
> @@ -754,11 +764,23 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>          p = irq_to_pending(v, virq);
>          if ( p->desc != NULL ) {
>              p->desc->status &= ~IRQ_INPROGRESS;
> -            GICC[GICC_DIR] = virq;
> +            /* Assume only one pcpu needs to EOI the irq */
> +            cpu = cpumask_first(&p->eoimask);
> +            cpumask_clear(&p->eoimask);
> +            eoi = 1;
Either this assumption is true, in which case you can use an int for
p->eoimask and avoid frobbing around with cpumasks, or it isn''t in
which
case you can trivially adjust the call to on_select_cpus to DTRT.

I think the assumption is true and you can just use an int.
>          }
>          list_del_init(&p->inflight);
>          spin_unlock_irq(&v->arch.vgic.lock);
>  
> +        if ( eoi ) {
> +            /* this is not racy because we can''t receive another
irq of the
> +             * same type until we EOI it.  */
> +            if ( cpu == smp_processor_id() )
> +                gic_irq_eoi((void*)virq);
> +            else
> +                on_selected_cpus(cpumask_of(cpu), gic_irq_eoi,
(void*)virq, 0);
> +        }
> +
>          i++;
>      }
>  }
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index f9c1a6b..c5370d5 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -676,9 +676,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int
irq, int virtual)
>      n->irq = irq;
>      n->priority = priority;
>      if (!virtual)
> +    {
>          n->desc = irq_to_desc(irq);
> -    else
> +        cpumask_clear(&n->eoimask);
> +        /* Assume we received the IRQ on the current pcpu */
> +        cpumask_set_cpu(smp_processor_id(), &n->eoimask);
Is there any way for this to be false?

Perhaps eoimask should be part of arch_irq_desc so it can be saved at
the actual point we handle the interrupt?

Ian.

On Tue, 7 May 2013, Ian Campbell wrote:
> > @@ -733,6 +739,10 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
> >      while ((i = find_next_bit((const long unsigned int *) &eisr,
> >                                64, i)) < 64) {
> >          struct pending_irq *p;
> > +        int cpu, eoi;
> > +
> > +        cpu = -1;
> > +        eoi = 0;
> >  
> >          spin_lock_irq(&gic.lock);
> >          lr = GICH[GICH_LR + i];
> > @@ -754,11 +764,23 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
> >          p = irq_to_pending(v, virq);
> >          if ( p->desc != NULL ) {
> >              p->desc->status &= ~IRQ_INPROGRESS;
> > -            GICC[GICC_DIR] = virq;
> > +            /* Assume only one pcpu needs to EOI the irq */
> > +            cpu = cpumask_first(&p->eoimask);
> > +            cpumask_clear(&p->eoimask);
> > +            eoi = 1;
> 
> Either this assumption is true, in which case you can use an int for
> p->eoimask and avoid frobbing around with cpumasks, or it isn''t
in which
> case you can trivially adjust the call to on_select_cpus to DTRT.
> 
> I think the assumption is true and you can just use an int.
OK, int it is

> >          }
> >          list_del_init(&p->inflight);
> >          spin_unlock_irq(&v->arch.vgic.lock);
> >  
> > +        if ( eoi ) {
> > +            /* this is not racy because we can''t receive
another irq of the
> > +             * same type until we EOI it.  */
> > +            if ( cpu == smp_processor_id() )
> > +                gic_irq_eoi((void*)virq);
> > +            else
> > +                on_selected_cpus(cpumask_of(cpu), gic_irq_eoi,
(void*)virq, 0);
> > +        }
> > +
> >          i++;
> >      }
> >  }
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index f9c1a6b..c5370d5 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -676,9 +676,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v,
unsigned int irq, int virtual)
> >      n->irq = irq;
> >      n->priority = priority;
> >      if (!virtual)
> > +    {
> >          n->desc = irq_to_desc(irq);
> > -    else
> > +        cpumask_clear(&n->eoimask);
> > +        /* Assume we received the IRQ on the current pcpu */
> > +        cpumask_set_cpu(smp_processor_id(), &n->eoimask);
> 
> Is there any way for this to be false?
> 
> Perhaps eoimask should be part of arch_irq_desc so it can be saved at
> the actual point we handle the interrupt?
Even if I don''t think the assumption could be false, having eoimask in
arch_irq_desc makes more sense, I''ll do that