[PATCH 0/2] xen: EOI on the correct GICC interface

[PATCH 0/2] xen: EOI on the correct GICC interface

[Stefano Stabellini]

Hi all,
this small patch series fixes a serious issue spotted by Julien reading
the GIC code: if the maintenance interrupt is received on a pcpu
different from the one that received the original irq (for example
because the vcpu has been migrated), Xen executes the irq EOI on the
wrong GICC interface.

Fix the issue issuing an SGI.

The patch series is based on Julien's "xen/arm: implement
smp_call_function".


Stefano Stabellini (2):
      xen: allow on_selected_cpus with interrupts disabled
      xen/gic: EOI irqs on the right pcpu

 xen/arch/arm/gic.c           |   15 ++++++++++++++-
 xen/arch/arm/vgic.c          |    7 ++++++-
 xen/common/smp.c             |    7 +++----
 xen/include/asm-arm/domain.h |    2 ++
 4 files changed, 25 insertions(+), 6 deletions(-)

Cheers,

Stefano

[PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled

[Stefano Stabellini]

Allow on_selected_cpus with interrupts disabled, use it with care.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/common/smp.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/common/smp.c b/xen/common/smp.c
index dcd93ad..7deb97c 100644
--- a/xen/common/smp.c
+++ b/xen/common/smp.c
@@ -33,10 +33,9 @@ void on_selected_cpus(
     int wait)
 {
     unsigned int nr_cpus;
+    unsigned long flags;
 
-    ASSERT(local_irq_is_enabled());
-
-    spin_lock(&call_lock);
+    spin_lock_irqsave(&call_lock, flags);
 
     cpumask_copy(&call_data.selected, selected);
 
@@ -54,7 +53,7 @@ void on_selected_cpus(
         cpu_relax();
 
 out:
-    spin_unlock(&call_lock);
+    spin_unlock_irqrestore(&call_lock, flags);
 }
 
 void smp_call_function_interrupt(void)
-- 
1.7.2.5

Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled

[Ian Campbell]

On Fri, 2013-05-03 at 15:58 +0100, Stefano Stabellini wrote:
> Allow on_selected_cpus with interrupts disabled, use it with care.

This is a deadlock waiting to happen. Can we not find a way to do cross
CPU EOI without it? If we can guarantee that we only need to EOI on one
CPU then does that make a specialised SGI vector more plausible?

Can the IPI call not be moved outside the lock? i.e. remove it from the
list under the lock and then IPI outside?

Or could you queue the IRQ on a per-pcpu list of IRQs to EOI and then
outside the lock send an IPI to the other CPU to check the list.

At the least this should assert that he current cpu isn't in the mask
when wait == 1.

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/common/smp.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/smp.c b/xen/common/smp.c
> index dcd93ad..7deb97c 100644
> --- a/xen/common/smp.c
> +++ b/xen/common/smp.c
> @@ -33,10 +33,9 @@ void on_selected_cpus(
>      int wait)
>  {
>      unsigned int nr_cpus;
> +    unsigned long flags;
>  
> -    ASSERT(local_irq_is_enabled());
> -
> -    spin_lock(&call_lock);
> +    spin_lock_irqsave(&call_lock, flags);
>  
>      cpumask_copy(&call_data.selected, selected);
>  
> @@ -54,7 +53,7 @@ void on_selected_cpus(
>          cpu_relax();
>  
>  out:
> -    spin_unlock(&call_lock);
> +    spin_unlock_irqrestore(&call_lock, flags);
>  }
>  
>  void smp_call_function_interrupt(void)

Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled

[Keir Fraser]

On 03/05/2013 16:21, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:

> On Fri, 2013-05-03 at 15:58 +0100, Stefano Stabellini wrote:
>> Allow on_selected_cpus with interrupts disabled, use it with care.
> 
> This is a deadlock waiting to happen. Can we not find a way to do cross
> CPU EOI without it? If we can guarantee that we only need to EOI on one
> CPU then does that make a specialised SGI vector more plausible?
> 
> Can the IPI call not be moved outside the lock? i.e. remove it from the
> list under the lock and then IPI outside?
> 
> Or could you queue the IRQ on a per-pcpu list of IRQs to EOI and then
> outside the lock send an IPI to the other CPU to check the list.
> 
> At the least this should assert that he current cpu isn't in the mask
> when wait == 1.

There's little chance of me being flexible on changing the
on_selected_cpus() interface. This may be better handled under arch/arm, or
with a new interface, or just as you sugegst rethinking the higher-level
problem so you don't get painted into this corner in the first place.
 
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> ---
>>  xen/common/smp.c |    7 +++----
>>  1 files changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/xen/common/smp.c b/xen/common/smp.c
>> index dcd93ad..7deb97c 100644
>> --- a/xen/common/smp.c
>> +++ b/xen/common/smp.c
>> @@ -33,10 +33,9 @@ void on_selected_cpus(
>>      int wait)
>>  {
>>      unsigned int nr_cpus;
>> +    unsigned long flags;
>>  
>> -    ASSERT(local_irq_is_enabled());
>> -
>> -    spin_lock(&call_lock);
>> +    spin_lock_irqsave(&call_lock, flags);
>>  
>>      cpumask_copy(&call_data.selected, selected);
>>  
>> @@ -54,7 +53,7 @@ void on_selected_cpus(
>>          cpu_relax();
>>  
>>  out:
>> -    spin_unlock(&call_lock);
>> +    spin_unlock_irqrestore(&call_lock, flags);
>>  }
>>  
>>  void smp_call_function_interrupt(void)
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled

[Ian Campbell]

On Fri, 2013-05-03 at 16:57 +0100, Keir Fraser wrote:
> On 03/05/2013 16:21, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:
> 
> > On Fri, 2013-05-03 at 15:58 +0100, Stefano Stabellini wrote:
> >> Allow on_selected_cpus with interrupts disabled, use it with care.
> > 
> > This is a deadlock waiting to happen. Can we not find a way to do cross
> > CPU EOI without it? If we can guarantee that we only need to EOI on one
> > CPU then does that make a specialised SGI vector more plausible?
> > 
> > Can the IPI call not be moved outside the lock? i.e. remove it from the
> > list under the lock and then IPI outside?
> > 
> > Or could you queue the IRQ on a per-pcpu list of IRQs to EOI and then
> > outside the lock send an IPI to the other CPU to check the list.
> > 
> > At the least this should assert that he current cpu isn't in the mask
> > when wait == 1.
> 
> There's little chance of me being flexible on changing the
> on_selected_cpus() interface.

I hadn't noticed this was common code, but in any case I agree with you.

>  This may be better handled under arch/arm, or
> with a new interface, or just as you sugegst rethinking the higher-level
> problem so you don't get painted into this corner in the first place.

ACK.

Ian.

Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled

[Ian Campbell]

On Fri, 2013-05-03 at 17:01 +0100, Ian Campbell wrote:
> On Fri, 2013-05-03 at 16:57 +0100, Keir Fraser wrote:

> > There's little chance of me being flexible on changing the
> > on_selected_cpus() interface.
> 
> I hadn't noticed this was common code, but in any case I agree with you.

Which reminds me  -- any chance you could Ack/Nack Julien's patch which
moves the original code (i.e. without this change) to common code.

"[PATCH V2 2/2] xen/arm: implement smp_call_function"
<1366119508-9227-3-git-send-email-julien.grall@citrix.com>

Ian.

Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled

[Keir Fraser]

On 03/05/2013 17:04, "Ian Campbell" <Ian.Campbell@citrix.com> wrote:

> On Fri, 2013-05-03 at 17:01 +0100, Ian Campbell wrote:
>> On Fri, 2013-05-03 at 16:57 +0100, Keir Fraser wrote:
> 
>>> There's little chance of me being flexible on changing the
>>> on_selected_cpus() interface.
>> 
>> I hadn't noticed this was common code, but in any case I agree with you.
> 
> Which reminds me  -- any chance you could Ack/Nack Julien's patch which
> moves the original code (i.e. without this change) to common code.
> 
> "[PATCH V2 2/2] xen/arm: implement smp_call_function"
> <1366119508-9227-3-git-send-email-julien.grall@citrix.com>

Okay, I will take a look. :)

> Ian.
> 

Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled

[Stefano Stabellini]

On Fri, 3 May 2013, Ian Campbell wrote:
> On Fri, 2013-05-03 at 15:58 +0100, Stefano Stabellini wrote:
> > Allow on_selected_cpus with interrupts disabled, use it with care.
> 
> This is a deadlock waiting to happen. Can we not find a way to do cross
> CPU EOI without it? If we can guarantee that we only need to EOI on one
> CPU then does that make a specialised SGI vector more plausible?
> 
> Can the IPI call not be moved outside the lock? i.e. remove it from the
> list under the lock and then IPI outside?

I think it can: after all if the physical IRQ has not beeing EOIed yet,
we cannot receive a second one at this time with ot without the lock
held. 

Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled

[Keir Fraser]

On 03/05/2013 15:58, "Stefano Stabellini" <stefano.stabellini@eu.citrix.com>
wrote:

> Allow on_selected_cpus with interrupts disabled, use it with care.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

NACK. The potential for deadlock is obvious, unclear what 'with care' might
entail.

 -- Keir

> ---
>  xen/common/smp.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/smp.c b/xen/common/smp.c
> index dcd93ad..7deb97c 100644
> --- a/xen/common/smp.c
> +++ b/xen/common/smp.c
> @@ -33,10 +33,9 @@ void on_selected_cpus(
>      int wait)
>  {
>      unsigned int nr_cpus;
> +    unsigned long flags;
>  
> -    ASSERT(local_irq_is_enabled());
> -
> -    spin_lock(&call_lock);
> +    spin_lock_irqsave(&call_lock, flags);
>  
>      cpumask_copy(&call_data.selected, selected);
>  
> @@ -54,7 +53,7 @@ void on_selected_cpus(
>          cpu_relax();
>  
>  out:
> -    spin_unlock(&call_lock);
> +    spin_unlock_irqrestore(&call_lock, flags);
>  }
>  
>  void smp_call_function_interrupt(void)

[PATCH 2/2] xen/gic: EOI irqs on the right pcpu

[Stefano Stabellini]

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

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 61de230..9514d1c 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;
@@ -753,8 +759,15 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
         spin_lock_irq(&v->arch.vgic.lock);
         p = irq_to_pending(v, virq);
         if ( p->desc != NULL ) {
+            int cpu;
             p->desc->status &= ~IRQ_INPROGRESS;
-            GICC[GICC_DIR] = virq;
+            /* Assume only one pcpu needs an EOI */
+            cpu = cpumask_first(&p->eoimask);
+            cpumask_clear(&p->eoimask);
+            if ( cpu == smp_processor_id() )
+                gic_irq_eoi(&virq);
+            else
+                on_selected_cpus(cpumask_of(cpu), gic_irq_eoi, &virq, 0);
         }
         list_del_init(&p->inflight);
         spin_unlock_irq(&v->arch.vgic.lock);
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

Re: [PATCH 2/2] xen/gic: EOI irqs on the right pcpu

[Julien Grall]

On 05/03/2013 03:58 PM, Stefano Stabellini wrote:

> 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).
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/gic.c           |   15 ++++++++++++++-
>  xen/arch/arm/vgic.c          |    7 ++++++-
>  xen/include/asm-arm/domain.h |    2 ++
>  3 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 61de230..9514d1c 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;
> @@ -753,8 +759,15 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
>          spin_lock_irq(&v->arch.vgic.lock);
>          p = irq_to_pending(v, virq);
>          if ( p->desc != NULL ) {
> +            int cpu;
>              p->desc->status &= ~IRQ_INPROGRESS;
> -            GICC[GICC_DIR] = virq;
> +            /* Assume only one pcpu needs an EOI */
> +            cpu = cpumask_first(&p->eoimask);
> +            cpumask_clear(&p->eoimask);
> +            if ( cpu == smp_processor_id() )
> +                gic_irq_eoi(&virq);
> +            else
> +                on_selected_cpus(cpumask_of(cpu), gic_irq_eoi, &virq, 0);


There is a race condition here. You use a pointer to a local variable
(virq) and you don't wait the completion of the function. So virq could
be updated/removed before EOI.

>          }
>          list_del_init(&p->inflight);
>          spin_unlock_irq(&v->arch.vgic.lock);
> 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 */


-- 
Julien