Stefano Stabellini
2013-May-03 14:58 UTC
[PATCH 0/2] xen: EOI on the correct GICC interface
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
Stefano Stabellini
2013-May-03 14:58 UTC
[PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled
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
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
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
Ian Campbell
2013-May-03 15:21 UTC
Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled
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)
Keir Fraser
2013-May-03 15:53 UTC
Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled
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)
Keir Fraser
2013-May-03 15:57 UTC
Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled
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
Ian Campbell
2013-May-03 16:01 UTC
Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled
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.
Ian Campbell
2013-May-03 16:04 UTC
Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled
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.
Stefano Stabellini
2013-May-03 16:40 UTC
Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled
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.
Keir Fraser
2013-May-03 17:38 UTC
Re: [PATCH 1/2] xen: allow on_selected_cpus with interrupts disabled
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. >