[PATCH v2 0/6] interrupt handling fixes

[PATCH v2 0/6] interrupt handling fixes

[Stefano Stabellini]

Hi all,
this series is a reworked version of "Fix multiple issues with the
interrupts on ARM":

http://marc.info/?l=xen-devel&m=137211515720144

It fixes a few different issues that affect interrupt handling in Xen on
ARM today:

- the guest looses a vtimer interrupt notification when it sets a
deadline in the past from the guest vtimer interrupt handler, before
EOIing the interrupt;

- Xen adds a guest irq to the LR registers twice if the guest disables
and renables an interrupt before EOIing it;

- Xen enables interrupts corresponding to devices assigned to dom0
before booting dom0, resulting in the possibility of receiving an
interrupt and not knowing what to do with it.


Changes in v2:
- remove eoi variable and check on p->desc != NULL instead;
- use atomic operations to modify the pending_irq status bits, remove
the now unnecessary locks;
- make status unsigned long;
- in maintenance_interrupt only stops injecting interrupts if no new
interrupts have been added to the LRs;
- add a state to keep track whether the guest irq is enabled at the
vgicd level;
- no need to read the current GICD_ISENABLER before writing it;
- protect startup and shutdown with gic and desc locks;
- disable IRQs that were previously disabled.


Julien Grall (2):
      xen/arm: Physical IRQ is not always equal to virtual IRQ
      xen/arm: Only enable physical IRQs when the guest asks

Stefano Stabellini (4):
      xen/arm: track the state of guest IRQs
      xen/arm: do not add a second irq to the LRs if one is already present
      xen/arm: implement gic_irq_enable and gic_irq_disable
      xen/arm: disable a physical IRQ when the guest disables the corresponding IRQ

 xen/arch/arm/gic.c           |  130 +++++++++++++++++++++++++-----------------
 xen/arch/arm/vgic.c          |   41 ++++++++++---
 xen/include/asm-arm/domain.h |   40 +++++++++++++
 3 files changed, 152 insertions(+), 59 deletions(-)

[PATCH v2 1/6] xen/arm: Physical IRQ is not always equal to virtual IRQ

[Stefano Stabellini]

From: Julien Grall <julien.grall@linaro.org>

When Xen needs to EOI a physical IRQ, we should use the IRQ number
in irq_desc instead of the virtual IRQ.

Remove the eoi flag in maintenance_interrupt and replace the check with
a check on p->desc != NULL.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Changes in v2:
- remove eoi variable and check on p->desc != NULL instead.
---
 xen/arch/arm/gic.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 43c11cb..5f7a548 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -880,7 +880,7 @@ static void gic_irq_eoi(void *info)
 
 static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
 {
-    int i = 0, virq;
+    int i = 0, virq, pirq = -1;
     uint32_t lr;
     struct vcpu *v = current;
     uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 32);
@@ -888,10 +888,9 @@ 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;
+        int cpu;
 
         cpu = -1;
-        eoi = 0;
 
         spin_lock_irq(&gic.lock);
         lr = GICH[GICH_LR + i];
@@ -915,19 +914,19 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
             p->desc->status &= ~IRQ_INPROGRESS;
             /* Assume only one pcpu needs to EOI the irq */
             cpu = p->desc->arch.eoi_cpu;
-            eoi = 1;
+            pirq = p->desc->irq;
         }
         list_del_init(&p->inflight);
         spin_unlock_irq(&v->arch.vgic.lock);
 
-        if ( eoi ) {
+        if ( p->desc != NULL ) {
             /* 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*)(uintptr_t)virq);
+                gic_irq_eoi((void*)(uintptr_t)pirq);
             else
                 on_selected_cpus(cpumask_of(cpu),
-                                 gic_irq_eoi, (void*)(uintptr_t)virq, 0);
+                                 gic_irq_eoi, (void*)(uintptr_t)pirq, 0);
         }
 
         i++;
-- 
1.7.10.4

[PATCH v2 2/6] xen/arm: track the state of guest IRQs

[Stefano Stabellini]

Introduce a status field in struct pending_irq. Valid states are
GUEST_PENDING, GUEST_VISIBLE and GUEST_ENABLED and they are not mutually
exclusive.  See the in-code comment for an explanation of the states and
how they are used.
Use atomic operations to set and clear the status bits. Note that
setting GIC_IRQ_GUEST_VISIBLE and clearing GIC_IRQ_GUEST_PENDING can be
done in two separate operations as the underlying pending status is
actually only cleared on the LR after the guest ACKs the interrupts.
Until that happens it's not possible to receive another interrupt.

The main effect of this patch is that an IRQ can be set to GUEST_PENDING
while it is being serviced by the guest. In maintenance_interrupt we
check whether GUEST_PENDING is set and if it is we reinject the IRQ one
more time, if the interrupt is still enabled at the vgicd level.
If it is not, it is going to be injected as soon as the guest renables
the interrupt.

In maintenance_interrupt only stops injecting interrupts if no new
interrupts have been added to the LRs.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Changes in v2:
- use atomic operations to modify the pending_irq status bits, remove
the now unnecessary locks;
- make status unsigned long;
- in maintenance_interrupt only stops injecting interrupts if no new
interrupts have been added to the LRs;
- add a state to keep track whether the guest irq is enabled at the
vgicd level.
---
 xen/arch/arm/gic.c           |   52 +++++++++++++++++++++++++++++++-----------
 xen/arch/arm/vgic.c          |   15 +++++++++---
 xen/include/asm-arm/domain.h |   40 ++++++++++++++++++++++++++++++++
 3 files changed, 91 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 5f7a548..d736a30 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -635,17 +635,20 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
 
     spin_lock_irqsave(&gic.lock, flags);
 
+    n = irq_to_pending(v, virtual_irq);
+
     if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
     {
         i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
         if (i < nr_lrs) {
             set_bit(i, &this_cpu(lr_mask));
             gic_set_lr(i, virtual_irq, state, priority);
+            set_bit(_GIC_IRQ_GUEST_VISIBLE, &n->status);
+            clear_bit(_GIC_IRQ_GUEST_PENDING, &n->status);
             goto out;
         }
     }
 
-    n = irq_to_pending(v, virtual_irq);
     if ( !list_empty(&n->lr_queue) )
         goto out;
 
@@ -680,6 +683,8 @@ static void gic_restore_pending_irqs(struct vcpu *v)
         list_del_init(&p->lr_queue);
         set_bit(i, &this_cpu(lr_mask));
         spin_unlock_irqrestore(&gic.lock, flags);
+        set_bit(_GIC_IRQ_GUEST_VISIBLE, &p->status);
+        clear_bit(_GIC_IRQ_GUEST_PENDING, &p->status);
     }
 
 }
@@ -884,10 +889,11 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
     uint32_t lr;
     struct vcpu *v = current;
     uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 32);
+    unsigned int set_int = 0;
 
     while ((i = find_next_bit((const long unsigned int *) &eisr,
                               64, i)) < 64) {
-        struct pending_irq *p;
+        struct pending_irq *p, *p2;
         int cpu;
 
         cpu = -1;
@@ -898,17 +904,6 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
         GICH[GICH_LR + i] = 0;
         clear_bit(i, &this_cpu(lr_mask));
 
-        if ( !list_empty(&v->arch.vgic.lr_pending) ) {
-            p = list_entry(v->arch.vgic.lr_pending.next, typeof(*p), lr_queue);
-            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
-            list_del_init(&p->lr_queue);
-            set_bit(i, &this_cpu(lr_mask));
-        } else {
-            gic_inject_irq_stop();
-        }
-        spin_unlock_irq(&gic.lock);
-
-        spin_lock_irq(&v->arch.vgic.lock);
         p = irq_to_pending(v, virq);
         if ( p->desc != NULL ) {
             p->desc->status &= ~IRQ_INPROGRESS;
@@ -916,6 +911,31 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
             cpu = p->desc->arch.eoi_cpu;
             pirq = p->desc->irq;
         }
+        if ( test_bit(_GIC_IRQ_GUEST_PENDING, &p->status) &&
+             test_bit(_GIC_IRQ_GUEST_ENABLED, &p->status))
+        {
+            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
+            clear_bit(_GIC_IRQ_GUEST_PENDING, &p->status);
+            i++;
+            spin_unlock_irq(&gic.lock);
+            set_int++;
+            continue;
+        }
+
+        clear_bit(_GIC_IRQ_GUEST_VISIBLE, &p->status);
+
+        if ( !list_empty(&v->arch.vgic.lr_pending) ) {
+            p2 = list_entry(v->arch.vgic.lr_pending.next, typeof(*p2), lr_queue);
+            gic_set_lr(i, p2->irq, GICH_LR_PENDING, p2->priority);
+            set_bit(_GIC_IRQ_GUEST_VISIBLE, &p2->status);
+            clear_bit(_GIC_IRQ_GUEST_PENDING, &p2->status);
+            list_del_init(&p2->lr_queue);
+            set_bit(i, &this_cpu(lr_mask));
+            set_int++;
+        }
+        spin_unlock_irq(&gic.lock);
+
+        spin_lock_irq(&v->arch.vgic.lock);
         list_del_init(&p->inflight);
         spin_unlock_irq(&v->arch.vgic.lock);
 
@@ -931,6 +951,12 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
 
         i++;
     }
+    if ( !set_int )
+    {
+        spin_lock_irq(&gic.lock);
+        gic_inject_irq_stop();
+        spin_unlock_irq(&gic.lock);
+    }
 }
 
 void gic_dump_info(struct vcpu *v)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 2e4b11f..2f7acca 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -369,6 +369,7 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
     while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
         irq = i + (32 * n);
         p = irq_to_pending(v, irq);
+        set_bit(_GIC_IRQ_GUEST_ENABLED, &p->status);
         if ( !list_empty(&p->inflight) )
             gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
         i++;
@@ -672,8 +673,15 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
 
-    /* vcpu offline or irq already pending */
-    if (test_bit(_VPF_down, &v->pause_flags) || !list_empty(&n->inflight))
+    if ( !list_empty(&n->inflight) )
+    {
+        set_bit(_GIC_IRQ_GUEST_PENDING, &n->status);
+        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+        return;
+    }
+
+    /* vcpu offline */
+    if ( test_bit(_VPF_down, &v->pause_flags) )
     {
         spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
         return;
@@ -682,6 +690,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
     priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte);
 
     n->irq = irq;
+    set_bit(_GIC_IRQ_GUEST_PENDING, &n->status);
     n->priority = priority;
     if (!virtual)
         n->desc = irq_to_desc(irq);
@@ -689,7 +698,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
         n->desc = NULL;
 
     /* the irq is enabled */
-    if ( rank->ienable & (1 << (irq % 32)) )
+    if ( test_bit(_GIC_IRQ_GUEST_ENABLED, &n->status) )
         gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
 
     list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index d5cae2e..9c3f09f 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -22,6 +22,46 @@ struct vgic_irq_rank {
 struct pending_irq
 {
     int irq;
+    /*
+     * The following two states track the lifecycle of the guest irq.
+     * However because we are not sure and we don't want to track
+     * whether an irq added to an LR register is PENDING or ACTIVE, the
+     * following states are just an approximation.
+     *
+     * GIC_IRQ_GUEST_PENDING: the irq is asserted
+     *
+     * GIC_IRQ_GUEST_VISIBLE: the irq has been added to an LR register,
+     * therefore the guest is aware of it. From the guest point of view
+     * the irq can be pending (if the guest has not acked the irq yet)
+     * or active (after acking the irq).
+     *
+     * In order for the state machine to be fully accurate, for level
+     * interrupts, we should keep the GIC_IRQ_GUEST_PENDING state until
+     * the guest deactivates the irq. However because we are not sure
+     * when that happens, we simply remove the GIC_IRQ_GUEST_PENDING
+     * state when we add the irq to an LR register. We add it back when
+     * we receive another interrupt notification.
+     * Therefore it is possible to set GIC_IRQ_GUEST_PENDING while the
+     * irq is GIC_IRQ_GUEST_VISIBLE. We could also change the state of
+     * the guest irq in the LR register from active to active and
+     * pending, but for simplicity we simply inject a second irq after
+     * the guest EOIs the first one.
+     *
+     *
+     * An additional state is used to keep track of whether the guest
+     * irq is enabled at the vgicd level:
+     *
+     * GIC_IRQ_GUEST_ENABLED: the guest IRQ is enabled at the VGICD
+     * level (GICD_ICENABLER/GICD_ISENABLER).
+     *
+     */
+#define _GIC_IRQ_GUEST_PENDING  0
+#define GIC_IRQ_GUEST_PENDING   (1UL<<0)
+#define _GIC_IRQ_GUEST_VISIBLE  1
+#define GIC_IRQ_GUEST_VISIBLE   (1UL<<1)
+#define _GIC_IRQ_GUEST_ENABLED  2
+#define GIC_IRQ_GUEST_ENABLED   (1UL<<2)
+    unsigned long status;
     struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
     uint8_t priority;
     /* inflight is used to append instances of pending_irq to
-- 
1.7.10.4

[PATCH v2 3/6] xen/arm: do not add a second irq to the LRs if one is already present

[Stefano Stabellini]

When the guest re-enable IRQs, do not add guest IRQs to LRs twice.

Suggested-by: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/vgic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 2f7acca..75df571 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -370,7 +370,7 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         irq = i + (32 * n);
         p = irq_to_pending(v, irq);
         set_bit(_GIC_IRQ_GUEST_ENABLED, &p->status);
-        if ( !list_empty(&p->inflight) )
+        if ( !list_empty(&p->inflight) && !test_bit(_GIC_IRQ_GUEST_VISIBLE, &p->status) )
             gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
         i++;
     }
-- 
1.7.10.4

Re: [PATCH v2 3/6] xen/arm: do not add a second irq to the LRs if one is already present

[Ian Campbell]

On Tue, 2013-12-10 at 13:06 +0000, Stefano Stabellini wrote:
> When the guest re-enable IRQs, do not add guest IRQs to LRs twice.
> 
> Suggested-by: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v2 4/6] xen/arm: implement gic_irq_enable and gic_irq_disable

[Stefano Stabellini]

Rename gic_irq_startup to gic_irq_enable.
Rename gic_irq_shutdown to gic_irq_disable.

Implement gic_irq_startup and gic_irq_shutdown calling gic_irq_enable
and gic_irq_disable.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Changes in v2:
- no need to read the current GICD_ISENABLER before writing it.
---
 xen/arch/arm/gic.c |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index d736a30..5e60c5a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -129,19 +129,15 @@ void gic_restore_state(struct vcpu *v)
     gic_restore_pending_irqs(v);
 }
 
-static unsigned int gic_irq_startup(struct irq_desc *desc)
+static void gic_irq_enable(struct irq_desc *desc)
 {
-    uint32_t enabler;
     int irq = desc->irq;
 
     /* Enable routing */
-    enabler = GICD[GICD_ISENABLER + irq / 32];
-    GICD[GICD_ISENABLER + irq / 32] = enabler | (1u << (irq % 32));
-
-    return 0;
+    GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
 }
 
-static void gic_irq_shutdown(struct irq_desc *desc)
+static void gic_irq_disable(struct irq_desc *desc)
 {
     int irq = desc->irq;
 
@@ -149,14 +145,15 @@ static void gic_irq_shutdown(struct irq_desc *desc)
     GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
 }
 
-static void gic_irq_enable(struct irq_desc *desc)
+static unsigned int gic_irq_startup(struct irq_desc *desc)
 {
-
+    gic_irq_enable(desc);
+    return 0;
 }
 
-static void gic_irq_disable(struct irq_desc *desc)
+static void gic_irq_shutdown(struct irq_desc *desc)
 {
-
+    gic_irq_disable(desc);
 }
 
 static void gic_irq_ack(struct irq_desc *desc)
-- 
1.7.10.4

[PATCH v2 5/6] xen/arm: Only enable physical IRQs when the guest asks

[Stefano Stabellini]

From: Julien Grall <julien.grall@linaro.org>

Set/Unset IRQ_DISABLED from gic_irq_enable and gic_irq_disable.
Enable IRQs when the guest requests it, not unconditionally at boot time.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Julien Grall <julien.grall@citrix.com>

Changes in v2:
- protect startup and shutdown with gic and desc locks.
---
 xen/arch/arm/gic.c  |   46 ++++++++++++++++++++++++++--------------------
 xen/arch/arm/vgic.c |    6 ++----
 2 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 5e60c5a..62330dd 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -133,16 +133,26 @@ static void gic_irq_enable(struct irq_desc *desc)
 {
     int irq = desc->irq;
 
+    spin_lock(&desc->lock);
+    spin_lock(&gic.lock);
     /* Enable routing */
     GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
+    desc->status &= ~IRQ_DISABLED;
+    spin_unlock(&gic.lock);
+    spin_unlock(&desc->lock);
 }
 
 static void gic_irq_disable(struct irq_desc *desc)
 {
     int irq = desc->irq;
 
+    spin_lock(&desc->lock);
+    spin_lock(&gic.lock);
     /* Disable routing */
     GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
+    desc->status |= IRQ_DISABLED;
+    spin_unlock(&gic.lock);
+    spin_unlock(&desc->lock);
 }
 
 static unsigned int gic_irq_startup(struct irq_desc *desc)
@@ -247,24 +257,20 @@ static int gic_route_irq(unsigned int irq, bool_t level,
     ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
     ASSERT(irq < gic.lines);      /* Can't route interrupts that don't exist */
 
-    spin_lock_irqsave(&desc->lock, flags);
-    spin_lock(&gic.lock);
-
     if ( desc->action != NULL )
-    {
-        spin_unlock(&gic.lock);
-        spin_unlock(&desc->lock);
         return -EBUSY;
-    }
-
-    desc->handler = &gic_host_irq_type;
 
     /* Disable interrupt */
     desc->handler->shutdown(desc);
 
-    gic_set_irq_properties(irq, level, cpu_mask, priority);
+    spin_lock_irqsave(&desc->lock, flags);
 
+    desc->handler = &gic_host_irq_type;
+
+    spin_lock(&gic.lock);
+    gic_set_irq_properties(irq, level, cpu_mask, priority);
     spin_unlock(&gic.lock);
+
     spin_unlock_irqrestore(&desc->lock, flags);
     return 0;
 }
@@ -557,16 +563,13 @@ void __init release_irq(unsigned int irq)
 
     desc = irq_to_desc(irq);
 
+    desc->handler->shutdown(desc);
+
     spin_lock_irqsave(&desc->lock,flags);
     action = desc->action;
     desc->action  = NULL;
-    desc->status |= IRQ_DISABLED;
     desc->status &= ~IRQ_GUEST;
 
-    spin_lock(&gic.lock);
-    desc->handler->shutdown(desc);
-    spin_unlock(&gic.lock);
-
     spin_unlock_irqrestore(&desc->lock,flags);
 
     /* Wait to make sure it's not being used on another CPU */
@@ -583,11 +586,8 @@ static int __setup_irq(struct irq_desc *desc, unsigned int irq,
         return -EBUSY;
 
     desc->action  = new;
-    desc->status &= ~IRQ_DISABLED;
     dsb();
 
-    desc->handler->startup(desc);
-
     return 0;
 }
 
@@ -600,11 +600,12 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
     desc = irq_to_desc(irq->irq);
 
     spin_lock_irqsave(&desc->lock, flags);
-
     rc = __setup_irq(desc, irq->irq, new);
-
     spin_unlock_irqrestore(&desc->lock, flags);
 
+    desc->handler->startup(desc);
+
+
     return rc;
 }
 
@@ -740,6 +741,7 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
     unsigned long flags;
     int retval;
     bool_t level;
+    struct pending_irq *p;
 
     action = xmalloc(struct irqaction);
     if (!action)
@@ -766,6 +768,10 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
         goto out;
     }
 
+    /* TODO: do not assume delivery to vcpu0 */
+    p = irq_to_pending(d->vcpu[0], irq->irq);
+    p->desc = desc;
+
 out:
     spin_unlock(&gic.lock);
     spin_unlock_irqrestore(&desc->lock, flags);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 75df571..5886696 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -372,6 +372,8 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         set_bit(_GIC_IRQ_GUEST_ENABLED, &p->status);
         if ( !list_empty(&p->inflight) && !test_bit(_GIC_IRQ_GUEST_VISIBLE, &p->status) )
             gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
+        if ( p->desc != NULL )
+            p->desc->handler->enable(p->desc);
         i++;
     }
 }
@@ -692,10 +694,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
     n->irq = irq;
     set_bit(_GIC_IRQ_GUEST_PENDING, &n->status);
     n->priority = priority;
-    if (!virtual)
-        n->desc = irq_to_desc(irq);
-    else
-        n->desc = NULL;
 
     /* the irq is enabled */
     if ( test_bit(_GIC_IRQ_GUEST_ENABLED, &n->status) )
-- 
1.7.10.4

Re: [PATCH v2 5/6] xen/arm: Only enable physical IRQs when the guest asks

[Ian Campbell]

On Tue, 2013-12-10 at 13:06 +0000, Stefano Stabellini wrote:
> From: Julien Grall <julien.grall@linaro.org>
> 
> Set/Unset IRQ_DISABLED from gic_irq_enable and gic_irq_disable.
> Enable IRQs when the guest requests it, not unconditionally at boot time.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH v2 6/6] xen/arm: disable a physical IRQ when the guest disables the corresponding IRQ

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Changes in v2:
- disable IRQs that were previously disabled.
---
 xen/arch/arm/vgic.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5886696..ad4f80c 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -360,6 +360,22 @@ read_as_zero:
     return 1;
 }
 
+static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
+{
+    struct pending_irq *p;
+    unsigned int irq;
+    int i = 0;
+
+    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
+        irq = i + (32 * n);
+        p = irq_to_pending(v, irq);
+        clear_bit(_GIC_IRQ_GUEST_ENABLED, &p->status);
+        if ( p->desc != NULL )
+            p->desc->handler->disable(p->desc);
+        i++;
+    }
+}
+
 static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
 {
     struct pending_irq *p;
@@ -490,8 +506,10 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICENABLER);
         if ( rank == NULL) goto write_ignore;
         vgic_lock_rank(v, rank);
+        tr = rank->ienable;
         rank->ienable &= ~*r;
         vgic_unlock_rank(v, rank);
+        vgic_disable_irqs(v, (*r) & tr, gicd_reg - GICD_ICENABLER);
         return 1;
 
     case GICD_ISPENDR ... GICD_ISPENDRN:
-- 
1.7.10.4

Re: [PATCH v2 6/6] xen/arm: disable a physical IRQ when the guest disables the corresponding IRQ

[Ian Campbell]

On Tue, 2013-12-10 at 13:06 +0000, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> Changes in v2:
> - disable IRQs that were previously disabled.

These should be after a "---" marker, you've done this wrong
consistently in this series.

Re: [PATCH v2 6/6] xen/arm: disable a physical IRQ when the guest disables the corresponding IRQ

[Stefano Stabellini]

On Tue, 10 Dec 2013, Ian Campbell wrote:
> On Tue, 2013-12-10 at 13:06 +0000, Stefano Stabellini wrote:
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> > Changes in v2:
> > - disable IRQs that were previously disabled.
> 
> These should be after a "---" marker, you've done this wrong
> consistently in this series.
 
Unfortunately guilt doesn't support it. The content I write after "---"
would get lost.
I have the same problem with patch series I send to QEMU and Linux.
I can either do what I have been doing or avoid writing the log on each
patch and limiting myself to writing the log to patch #0.
So far I have been keeping the log as part of the commit messages.

Re: [PATCH v2 6/6] xen/arm: disable a physical IRQ when the guest disables the corresponding IRQ

[Ian Campbell]

On Tue, 2013-12-10 at 18:36 +0000, Stefano Stabellini wrote:
> On Tue, 10 Dec 2013, Ian Campbell wrote:
> > On Tue, 2013-12-10 at 13:06 +0000, Stefano Stabellini wrote:
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > > Changes in v2:
> > > - disable IRQs that were previously disabled.
> > 
> > These should be after a "---" marker, you've done this wrong
> > consistently in this series.
>  
> Unfortunately guilt doesn't support it. The content I write after "---"
> would get lost.

I suppose guilt uses the same tool as git am to actual apply the patches
on push, so it trims the comments too :-(

> I have the same problem with patch series I send to QEMU and Linux.
> I can either do what I have been doing or avoid writing the log on each
> patch and limiting myself to writing the log to patch #0.
> So far I have been keeping the log as part of the commit messages.

Sounds like a pretty major shortcoming of the tool TBH, perhaps time to
investigate alternatives?

Perhaps git smudge/clean filters could be used such that some other
string is ("--- CUT ME"?) stored in .git/guilt/patches/blah and is
converted on push/refresh etc to ---?

Ian.

Re: [PATCH v2 6/6] xen/arm: disable a physical IRQ when the guest disables the corresponding IRQ

[Stefano Stabellini]

On Wed, 11 Dec 2013, Ian Campbell wrote:
> On Tue, 2013-12-10 at 18:36 +0000, Stefano Stabellini wrote:
> > On Tue, 10 Dec 2013, Ian Campbell wrote:
> > > On Tue, 2013-12-10 at 13:06 +0000, Stefano Stabellini wrote:
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > 
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > > Changes in v2:
> > > > - disable IRQs that were previously disabled.
> > > 
> > > These should be after a "---" marker, you've done this wrong
> > > consistently in this series.
> >  
> > Unfortunately guilt doesn't support it. The content I write after "---"
> > would get lost.
> 
> I suppose guilt uses the same tool as git am to actual apply the patches
> on push, so it trims the comments too :-(
> 
> > I have the same problem with patch series I send to QEMU and Linux.
> > I can either do what I have been doing or avoid writing the log on each
> > patch and limiting myself to writing the log to patch #0.
> > So far I have been keeping the log as part of the commit messages.
> 
> Sounds like a pretty major shortcoming of the tool TBH, perhaps time to
> investigate alternatives?
> 
> Perhaps git smudge/clean filters could be used such that some other
> string is ("--- CUT ME"?) stored in .git/guilt/patches/blah and is
> converted on push/refresh etc to ---?

I manually edited guilt to avoid cutting off things after --- and before
the diff.

Re: [PATCH v2 6/6] xen/arm: disable a physical IRQ when the guest disables the corresponding IRQ

[Ian Campbell]

On Wed, 2013-12-11 at 19:01 +0000, Stefano Stabellini wrote:
> On Wed, 11 Dec 2013, Ian Campbell wrote:
> > On Tue, 2013-12-10 at 18:36 +0000, Stefano Stabellini wrote:
> > > On Tue, 10 Dec 2013, Ian Campbell wrote:
> > > > On Tue, 2013-12-10 at 13:06 +0000, Stefano Stabellini wrote:
> > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > 
> > > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > > 
> > > > > Changes in v2:
> > > > > - disable IRQs that were previously disabled.
> > > > 
> > > > These should be after a "---" marker, you've done this wrong
> > > > consistently in this series.
> > >  
> > > Unfortunately guilt doesn't support it. The content I write after "---"
> > > would get lost.
> > 
> > I suppose guilt uses the same tool as git am to actual apply the patches
> > on push, so it trims the comments too :-(
> > 
> > > I have the same problem with patch series I send to QEMU and Linux.
> > > I can either do what I have been doing or avoid writing the log on each
> > > patch and limiting myself to writing the log to patch #0.
> > > So far I have been keeping the log as part of the commit messages.
> > 
> > Sounds like a pretty major shortcoming of the tool TBH, perhaps time to
> > investigate alternatives?
> > 
> > Perhaps git smudge/clean filters could be used such that some other
> > string is ("--- CUT ME"?) stored in .git/guilt/patches/blah and is
> > converted on push/refresh etc to ---?
> 
> I manually edited guilt to avoid cutting off things after --- and before
> the diff.

Nice, thanks!

Ian.

Re: [PATCH v2 0/6] interrupt handling fixes

[Ian Campbell]

On Tue, 2013-12-10 at 13:05 +0000, Stefano Stabellini wrote:
> Hi all,
> this series is a reworked version of "Fix multiple issues with the
> interrupts on ARM":

#1 and #4-#6 are acked they seem comparatively straight forward does it
make sense to apply those now without the other two? Is it safe to do
so?

Ian.

Re: [PATCH v2 0/6] interrupt handling fixes

[Julien Grall]

On 12/10/2013 03:22 PM, Ian Campbell wrote:
> On Tue, 2013-12-10 at 13:05 +0000, Stefano Stabellini wrote:
>> Hi all,
>> this series is a reworked version of "Fix multiple issues with the
>> interrupts on ARM":
> 
> #1 and #4-#6 are acked they seem comparatively straight forward does it
> make sense to apply those now without the other two? Is it safe to do
> so?

Before applying these patches, I want to give a try on different
platforms If I'm not mistaken the patch series were only tried on midway.

-- 
Julien Grall

Re: [PATCH v2 0/6] interrupt handling fixes

[Stefano Stabellini]

On Tue, 10 Dec 2013, Julien Grall wrote:
> On 12/10/2013 03:22 PM, Ian Campbell wrote:
> > On Tue, 2013-12-10 at 13:05 +0000, Stefano Stabellini wrote:
> >> Hi all,
> >> this series is a reworked version of "Fix multiple issues with the
> >> interrupts on ARM":
> > 
> > #1 and #4-#6 are acked they seem comparatively straight forward does it
> > make sense to apply those now without the other two? Is it safe to do
> > so?
> 
> Before applying these patches, I want to give a try on different
> platforms If I'm not mistaken the patch series were only tried on midway.

I think I found some bugs. I can wait before testing.

Re: [PATCH v2 0/6] interrupt handling fixes

[Ian Campbell]

On Tue, 2013-12-10 at 15:36 +0000, Stefano Stabellini wrote:
> On Tue, 10 Dec 2013, Julien Grall wrote:
> > On 12/10/2013 03:22 PM, Ian Campbell wrote:
> > > On Tue, 2013-12-10 at 13:05 +0000, Stefano Stabellini wrote:
> > >> Hi all,
> > >> this series is a reworked version of "Fix multiple issues with the
> > >> interrupts on ARM":
> > > 
> > > #1 and #4-#6 are acked they seem comparatively straight forward does it
> > > make sense to apply those now without the other two? Is it safe to do
> > > so?
> > 
> > Before applying these patches, I want to give a try on different
> > platforms If I'm not mistaken the patch series were only tried on midway.
> 
> I think I found some bugs. I can wait before testing.

You mean he should wait, right?

I was going to try them as well, but I'll put that on hold for a moment.

Ian.

Re: [PATCH v2 0/6] interrupt handling fixes

[Stefano Stabellini]

On Tue, 10 Dec 2013, Ian Campbell wrote:
> On Tue, 2013-12-10 at 15:36 +0000, Stefano Stabellini wrote:
> > On Tue, 10 Dec 2013, Julien Grall wrote:
> > > On 12/10/2013 03:22 PM, Ian Campbell wrote:
> > > > On Tue, 2013-12-10 at 13:05 +0000, Stefano Stabellini wrote:
> > > >> Hi all,
> > > >> this series is a reworked version of "Fix multiple issues with the
> > > >> interrupts on ARM":
> > > > 
> > > > #1 and #4-#6 are acked they seem comparatively straight forward does it
> > > > make sense to apply those now without the other two? Is it safe to do
> > > > so?
> > > 
> > > Before applying these patches, I want to give a try on different
> > > platforms If I'm not mistaken the patch series were only tried on midway.
> > 
> > I think I found some bugs. I can wait before testing.
> 
> You mean he should wait, right?
> 
> I was going to try them as well, but I'll put that on hold for a moment.

It works well for dom0, but maybe not for domUs. I would wait a bit more
for tests or apply patches.

Re: [PATCH v2 0/6] interrupt handling fixes

[Stefano Stabellini]

On Tue, 10 Dec 2013, Stefano Stabellini wrote:
> On Tue, 10 Dec 2013, Ian Campbell wrote:
> > On Tue, 2013-12-10 at 15:36 +0000, Stefano Stabellini wrote:
> > > On Tue, 10 Dec 2013, Julien Grall wrote:
> > > > On 12/10/2013 03:22 PM, Ian Campbell wrote:
> > > > > On Tue, 2013-12-10 at 13:05 +0000, Stefano Stabellini wrote:
> > > > >> Hi all,
> > > > >> this series is a reworked version of "Fix multiple issues with the
> > > > >> interrupts on ARM":
> > > > > 
> > > > > #1 and #4-#6 are acked they seem comparatively straight forward does it
> > > > > make sense to apply those now without the other two? Is it safe to do
> > > > > so?
> > > > 
> > > > Before applying these patches, I want to give a try on different
> > > > platforms If I'm not mistaken the patch series were only tried on midway.
> > > 
> > > I think I found some bugs. I can wait before testing.
> > 
> > You mean he should wait, right?
> > 
> > I was going to try them as well, but I'll put that on hold for a moment.
> 
> It works well for dom0, but maybe not for domUs. I would wait a bit more
> for tests or apply patches.

The third version of the series works well for me for dom0 and domUs. I
would encourage you to test it.

Re: [PATCH v2 0/6] interrupt handling fixes

[Stefano Stabellini]

On Tue, 10 Dec 2013, Ian Campbell wrote:
> On Tue, 2013-12-10 at 13:05 +0000, Stefano Stabellini wrote:
> > Hi all,
> > this series is a reworked version of "Fix multiple issues with the
> > interrupts on ARM":
> 
> #1 and #4-#6 are acked they seem comparatively straight forward does it
> make sense to apply those now without the other two? Is it safe to do
> so?

#1 and #4 yes, not #6.
In any case I am about to resend another iteration of the series.