Xen devel - Jun 2013 - [PATCH 0/5] Fix multiple issues with the interrupts on ARM

Hello,

This patch series aims to fix different issues on Xen on ARM with the
arndale and the versatile express:
  - Handle correctly one shot IRQ (fixed by patch 3)
  - Make timers work with heavy load (fixed by patch 2)
  - Make ethernet card works on the TC2 (fixed by patch 5)

Some of these patches (2 and 5) are proof of concept. I would be happy if
someone find a better solution :).

Cheers,

Julien Grall (4):
  xen/arm: Physical IRQ is not always equal to virtual IRQ
  xen/arm: Don''t reinject the IRQ if it''s already in LRs
  xen/arm: Rename gic_irq_{startup,shutdown} to gic_irq_{mask,unmask}
  xen/arm: Only enable physical IRQs when the guest asks

Stefano Stabellini (1):
  xen/arm: Keep count of inflight interrupts

 xen/arch/arm/domain_build.c  |   14 +++++
 xen/arch/arm/gic.c           |  119 ++++++++++++++++++++++++++++++------------
 xen/arch/arm/vgic.c          |   11 ++--
 xen/include/asm-arm/domain.h |    2 +
 xen/include/asm-arm/gic.h    |    7 +++
 xen/include/asm-arm/irq.h    |    6 +++
 6 files changed, 122 insertions(+), 37 deletions(-)

-- 
1.7.10.4

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

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

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/gic.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 177560e..0fee3f2 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -810,7 +810,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;
     uint32_t lr;
     struct vcpu *v = current;
     uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) <<
32);
@@ -846,6 +846,7 @@ static void maintenance_interrupt(int irq, void *dev_id,
struct cpu_user_regs *r
             /* 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);
@@ -854,10 +855,10 @@ static void maintenance_interrupt(int irq, void *dev_id,
struct cpu_user_regs *r
             /* 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

From: Stefano Stabellini <stefano.stabellini@citrix.com>

For guest''s timers (both virtual and physical), Xen will inject virtual
interrupt. Linux handles timer interrupt as:
  1) Receive the interrupt and ack it
  2) Handle the current event timer
  3) Set the next event timer
  4) EOI the interrupt

It''s unlikely possible to reinject another interrupt before
the previous one is EOIed because the next deadline is shorter than the time
to execute code until EOI it.

Signed-off-by: Stefano Stabellini <stefano.stabellini@citrix.com>
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/gic.c           |   35 +++++++++++++++++++++++------------
 xen/arch/arm/vgic.c          |    1 +
 xen/include/asm-arm/domain.h |    2 ++
 3 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 0fee3f2..21575df 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -817,7 +817,7 @@ 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;
+        struct pending_irq *p, *n;
         int cpu, eoi;
 
         cpu = -1;
@@ -826,13 +826,32 @@ static void maintenance_interrupt(int irq, void *dev_id,
struct cpu_user_regs *r
         spin_lock_irq(&gic.lock);
         lr = GICH[GICH_LR + i];
         virq = lr & GICH_LR_VIRTUAL_MASK;
+
+        p = irq_to_pending(v, virq);
+        if ( p->desc != NULL ) {
+            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;
+        }
+        if ( !atomic_dec_and_test(&p->inflight_cnt) )
+        {
+            /* Physical IRQ can''t be reinject */
+            WARN_ON(p->desc != NULL);
+            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
+            spin_unlock_irq(&gic.lock);
+            i++;
+            continue;
+        }
+
         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);
+            n = list_entry(v->arch.vgic.lr_pending.next, typeof(*n),
lr_queue);
+            gic_set_lr(i, n->irq, GICH_LR_PENDING, n->priority);
+            list_del_init(&n->lr_queue);
             set_bit(i, &this_cpu(lr_mask));
         } else {
             gic_inject_irq_stop();
@@ -840,14 +859,6 @@ static void maintenance_interrupt(int irq, void *dev_id,
struct cpu_user_regs *r
         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;
-            /* 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);
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 7eaccb7..2d91dce 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -672,6 +672,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq,
int virtual)
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
 
+    atomic_inc(&n->inflight_cnt);
     /* vcpu offline or irq already pending */
     if (test_bit(_VPF_down, &v->pause_flags) ||
!list_empty(&n->inflight))
     {
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 339b6e6..fa0b776 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -8,6 +8,7 @@
 #include <asm/p2m.h>
 #include <asm/vfp.h>
 #include <public/hvm/params.h>
+#include <asm/atomic.h>
 
 /* Represents state corresponding to a block of 32 interrupts */
 struct vgic_irq_rank {
@@ -21,6 +22,7 @@ struct vgic_irq_rank {
 struct pending_irq
 {
     int irq;
+    atomic_t inflight_cnt;
     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

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

When an IRQ, marked as IRQS_ONESHOT, is injected Linux will:
  - Disable the IRQ
  - Call the interrupt handler
  - Conditionnally enable the IRQ
  - EOI the IRQ

When Linux will re-enable the IRQ, Xen will inject again the IRQ because
it''s
still inflight. Therefore, LRs will contains duplicated IRQs and Xen will
EOI it twice if it''s a physical IRQ.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/gic.c        |   27 +++++++++++++++++++++++++++
 xen/arch/arm/vgic.c       |    3 ++-
 xen/include/asm-arm/gic.h |    3 +++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 21575df..bf05716 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct
irqaction *new)
     return rc;
 }
 
+/* Check if an IRQ was already injected to the current VCPU */
+bool_t gic_irq_injected(unsigned int irq)
+{
+    bool_t found = 0;
+    int i = 0;
+    unsigned int virq;
+
+    spin_lock_irq(&gic.lock);
+
+    while ( (i = find_next_bit((unsigned long *)&this_cpu(lr_mask),
+                               nr_lrs, i)) < nr_lrs )
+    {
+        virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK;
+
+        if ( virq == irq )
+        {
+            found = 1;
+            break;
+        }
+        i++;
+    }
+
+    spin_unlock_irq(&gic.lock);
+
+    return found;
+}
+
 static inline void gic_set_lr(int lr, unsigned int virtual_irq,
         unsigned int state, unsigned int priority)
 {
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 2d91dce..cea9233 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -369,8 +369,9 @@ 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);
-        if ( !list_empty(&p->inflight) )
+        if ( !list_empty(&p->inflight) && !gic_irq_injected(irq)
)
             gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
+
         i++;
     }
 }
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 513c1fc..f9e9ef1 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -197,6 +197,9 @@ extern unsigned int gic_number_lines(void);
 int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
                   unsigned int *out_hwirq, unsigned int *out_type);
 
+/* Check if an IRQ was already injected to the current VCPU */
+bool_t gic_irq_injected(unsigned int irq);
+
 #endif /* __ASSEMBLY__ */
 #endif
 
-- 
1.7.10.4

Also implement enable/disable as mask/unmask irq.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/gic.c |   27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index bf05716..b16ba8c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -103,7 +103,7 @@ 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_unmask(struct irq_desc *desc)
 {
     uint32_t enabler;
     int irq = desc->irq;
@@ -111,11 +111,9 @@ static unsigned int gic_irq_startup(struct irq_desc *desc)
     /* Enable routing */
     enabler = GICD[GICD_ISENABLER + irq / 32];
     GICD[GICD_ISENABLER + irq / 32] = enabler | (1u << (irq % 32));
-
-    return 0;
 }
 
-static void gic_irq_shutdown(struct irq_desc *desc)
+static void gic_irq_mask(struct irq_desc *desc)
 {
     uint32_t enabler;
     int irq = desc->irq;
@@ -125,14 +123,11 @@ static void gic_irq_shutdown(struct irq_desc *desc)
     GICD[GICD_ICENABLER + irq / 32] = enabler | (1u << (irq % 32));
 }
 
-static void gic_irq_enable(struct irq_desc *desc)
-{
-
-}
-
-static void gic_irq_disable(struct irq_desc *desc)
+static unsigned int gic_irq_startup(struct irq_desc *desc)
 {
+    gic_irq_unmask(desc);
 
+    return 0;
 }
 
 static void gic_irq_ack(struct irq_desc *desc)
@@ -166,9 +161,9 @@ static void gic_irq_set_affinity(struct irq_desc *desc,
const cpumask_t *mask)
 static hw_irq_controller gic_host_irq_type = {
     .typename = "gic",
     .startup = gic_irq_startup,
-    .shutdown = gic_irq_shutdown,
-    .enable = gic_irq_enable,
-    .disable = gic_irq_disable,
+    .shutdown = gic_irq_mask,
+    .enable = gic_irq_unmask,
+    .disable = gic_irq_mask,
     .ack = gic_irq_ack,
     .end = gic_host_irq_end,
     .set_affinity = gic_irq_set_affinity,
@@ -176,9 +171,9 @@ static hw_irq_controller gic_host_irq_type = {
 static hw_irq_controller gic_guest_irq_type = {
     .typename = "gic",
     .startup = gic_irq_startup,
-    .shutdown = gic_irq_shutdown,
-    .enable = gic_irq_enable,
-    .disable = gic_irq_disable,
+    .shutdown = gic_irq_mask,
+    .enable = gic_irq_unmask,
+    .disable = gic_irq_mask,
     .ack = gic_irq_ack,
     .end = gic_guest_irq_end,
     .set_affinity = gic_irq_set_affinity,
-- 
1.7.10.4

If the guest VCPU receives an interrupts when it''s disabled, it will
throw
away the IRQ with EOIed it. This is result to lost IRQ forever.
Directly EOIed the interrupt doesn''t help because the IRQ could be
fired
again and result to an infinited loop.

It happens during dom0 boot on the versatile express TC2 with the ethernet
card.

Let the interrupt disabled when Xen setups the route and enable it when Linux
asks to enable it.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/domain_build.c |   14 ++++++++++++++
 xen/arch/arm/gic.c          |   25 +++++++++++++++++++++++--
 xen/arch/arm/vgic.c         |    7 +++----
 xen/include/asm-arm/gic.h   |    4 ++++
 xen/include/asm-arm/irq.h   |    6 ++++++
 5 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f5befbd..0470a2d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -408,6 +408,20 @@ static int map_device(struct domain *d, const struct
dt_device_node *dev)
         }
 
         DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq, irq.type);
+
+        /*
+         * Only map SGI interrupt in the guest as XEN won''t handle
+         * it correctly.
+         * TODO: Fix it
+         */
+        if ( !irq_is_sgi(irq.irq) )
+        {
+            printk(XENLOG_WARNING "WARNING: Don''t map irq %u for
%s has "
+                   "XEN doesn''t handle properly non-SGI
interrupt\n",
+                   i, dt_node_full_name(dev));
+            continue;
+        }
+
         /* Don''t check return because the IRQ can be use by multiple
device */
         gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
     }
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index b16ba8c..e7d082a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type = {
     .set_affinity = gic_irq_set_affinity,
 };
 
+void gic_irq_enable(struct irq_desc *desc)
+{
+    spin_lock_irq(&desc->lock);
+    spin_lock(&gic.lock);
+
+    desc->handler->enable(desc);
+
+    spin_unlock(&gic.lock);
+    spin_unlock_irq(&desc->lock);
+}
+
 /* needs to be called with gic.lock held */
 static void gic_set_irq_properties(unsigned int irq, bool_t level,
         unsigned int cpu_mask, unsigned int priority)
@@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc, unsigned int
irq,
     desc->status &= ~IRQ_DISABLED;
     dsb();
 
-    desc->handler->startup(desc);
-
     return 0;
 }
 
@@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct
irqaction *new)
 
     rc = __setup_irq(desc, irq->irq, new);
 
+    desc->handler->startup(desc);
+
     spin_unlock_irqrestore(&desc->lock, flags);
 
     return rc;
@@ -711,6 +722,7 @@ void gic_inject(void)
         gic_inject_irq_start();
 }
 
+/* TODO: Handle properly non SGI-interrupt */
 int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
                            const char * devname)
 {
@@ -719,11 +731,18 @@ 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;
+    /* XXX: handler other VCPU than 0 */
+    struct vcpu *v = d->vcpu[0];
 
     action = xmalloc(struct irqaction);
     if (!action)
         return -ENOMEM;
 
+    /* XXX: Here we assume a 1:1 interrupt mapping between the host and dom0 */
+    BUG_ON(irq->irq >= (d->arch.vgic.nr_lines + NR_LOCAL_IRQS));
+    p = irq_to_pending(v, irq->irq);
+
     action->dev_id = d;
     action->name = devname;
     action->free_on_release = 1;
@@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const struct
dt_irq *irq,
         goto out;
     }
 
+    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 cea9233..4f3d816 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -372,6 +372,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int
n)
         if ( !list_empty(&p->inflight) && !gic_irq_injected(irq)
)
             gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
 
+        if ( p->desc != NULL )
+            gic_irq_enable(p->desc);
+
         i++;
     }
 }
@@ -685,10 +688,6 @@ 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
-        n->desc = NULL;
 
     /* the irq is enabled */
     if ( rank->ienable & (1 << (irq % 32)) )
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index f9e9ef1..f7f3c1e 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -134,6 +134,7 @@
 
 #ifndef __ASSEMBLY__
 #include <xen/device_tree.h>
+#include <xen/irq.h>
 
 extern int domain_vgic_init(struct domain *d);
 extern void domain_vgic_free(struct domain *d);
@@ -154,6 +155,9 @@ extern void gic_inject(void);
 extern void gic_clear_pending_irqs(struct vcpu *v);
 extern int gic_events_need_delivery(void);
 
+/* Helper to enable an IRQ and take all the needed locks */
+extern void gic_irq_enable(struct irq_desc *desc);
+
 extern void __cpuinit init_maintenance_interrupt(void);
 extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
         unsigned int state, unsigned int priority);
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 80ff68d..346dc1d 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -46,6 +46,12 @@ int __init request_dt_irq(const struct dt_irq *irq,
                           void *dev_id);
 int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
 
+#define FIRST_SGI_IRQ   32
+static inline bool_t irq_is_sgi(unsigned int irq)
+{
+    return (irq >= FIRST_SGI_IRQ);
+}
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:
-- 
1.7.10.4

On Tue, 25 Jun 2013, Julien Grall wrote:
> From: Julien Grall <julien.grall@linaro.org>
> 
> When Xen needs to EOI a physical IRQ, we must use the IRQ number
> in irq_desc instead of the virtual IRQ.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/gic.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 177560e..0fee3f2 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -810,7 +810,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;
>      uint32_t lr;
>      struct vcpu *v = current;
>      uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1])
<< 32);
> @@ -846,6 +846,7 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>              /* 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);
> @@ -854,10 +855,10 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>              /* 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 think that virq and pirq are guaranteed to always be the same, at
least at the moment. Look at vgic_vcpu_inject_irq: it takes just one irq
parameter, that is both the physical and the virtual irq number.
Unless we change the vgic_vcpu_inject_irq interface to allow virq !pirq, I
don''t think this patch makes much sense.

On Tue, 25 Jun 2013, Julien Grall wrote:
> From: Julien Grall <julien.grall@linaro.org>
> 
> When an IRQ, marked as IRQS_ONESHOT, is injected Linux will:
>   - Disable the IRQ
>   - Call the interrupt handler
>   - Conditionnally enable the IRQ
>   - EOI the IRQ
> 
> When Linux will re-enable the IRQ, Xen will inject again the IRQ because
it''s
> still inflight. Therefore, LRs will contains duplicated IRQs and Xen will
> EOI it twice if it''s a physical IRQ.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/gic.c        |   27 +++++++++++++++++++++++++++
>  xen/arch/arm/vgic.c       |    3 ++-
>  xen/include/asm-arm/gic.h |    3 +++
>  3 files changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 21575df..bf05716 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq *irq,
struct irqaction *new)
>      return rc;
>  }
>  
> +/* Check if an IRQ was already injected to the current VCPU */
> +bool_t gic_irq_injected(unsigned int irq)
Can you rename it to something more specific, like gic_irq_inlr?
> +{
> +    bool_t found = 0;
> +    int i = 0;
> +    unsigned int virq;
> +
> +    spin_lock_irq(&gic.lock);
> +
> +    while ( (i = find_next_bit((unsigned long *)&this_cpu(lr_mask),
> +                               nr_lrs, i)) < nr_lrs )
> +    {
> +        virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK;
> +
> +        if ( virq == irq )
> +        {
> +            found = 1;
> +            break;
> +        }
> +        i++;
> +    }
Instead of reading back all the GICH_LR registers, can''t just just read
the ones that have a corresponding bit set in lr_mask?

Also you should be able to avoid having to read the GICH_LR registers by
simply checking if the irq is in the lr_queue list: if an irq is in
inflight but not in lr_queue, it means that it is in one of the LRs.


> +    spin_unlock_irq(&gic.lock);
> +
> +    return found;
> +}
> +
>  static inline void gic_set_lr(int lr, unsigned int virtual_irq,
>          unsigned int state, unsigned int priority)
>  {
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 2d91dce..cea9233 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -369,8 +369,9 @@ 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);
> -        if ( !list_empty(&p->inflight) )
> +        if ( !list_empty(&p->inflight) &&
!gic_irq_injected(irq) )
>              gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
> +
>          i++;
>      }
>  }
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 513c1fc..f9e9ef1 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -197,6 +197,9 @@ extern unsigned int gic_number_lines(void);
>  int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
>                    unsigned int *out_hwirq, unsigned int *out_type);
>  
> +/* Check if an IRQ was already injected to the current VCPU */
> +bool_t gic_irq_injected(unsigned int irq);
> +
>  #endif /* __ASSEMBLY__ */
>  #endif
>  
> -- 
> 1.7.10.4
>

On 06/25/2013 02:24 PM, Stefano Stabellini wrote:
> On Tue, 25 Jun 2013, Julien Grall wrote:
>> From: Julien Grall <julien.grall@linaro.org>
>>
>> When an IRQ, marked as IRQS_ONESHOT, is injected Linux will:
>>   - Disable the IRQ
>>   - Call the interrupt handler
>>   - Conditionnally enable the IRQ
>>   - EOI the IRQ
>>
>> When Linux will re-enable the IRQ, Xen will inject again the IRQ
because it''s
>> still inflight. Therefore, LRs will contains duplicated IRQs and Xen
will
>> EOI it twice if it''s a physical IRQ.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/gic.c        |   27 +++++++++++++++++++++++++++
>>  xen/arch/arm/vgic.c       |    3 ++-
>>  xen/include/asm-arm/gic.h |    3 +++
>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 21575df..bf05716 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq *irq,
struct irqaction *new)
>>      return rc;
>>  }
>>  
>> +/* Check if an IRQ was already injected to the current VCPU */
>> +bool_t gic_irq_injected(unsigned int irq)
> 
> Can you rename it to something more specific, like gic_irq_inlr?
> 
>> +{
>> +    bool_t found = 0;
>> +    int i = 0;
>> +    unsigned int virq;
>> +
>> +    spin_lock_irq(&gic.lock);
>> +
>> +    while ( (i = find_next_bit((unsigned long
*)&this_cpu(lr_mask),
>> +                               nr_lrs, i)) < nr_lrs )
>> +    {
>> +        virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK;
>> +
>> +        if ( virq == irq )
>> +        {
>> +            found = 1;
>> +            break;
>> +        }
>> +        i++;
>> +    }
> 
> Instead of reading back all the GICH_LR registers, can''t just just
read
> the ones that have a corresponding bit set in lr_mask?
It''s already the case, I use find_next_bit to find the next used LRs.
> Also you should be able to avoid having to read the GICH_LR registers by
> simply checking if the irq is in the lr_queue list: if an irq is in
> inflight but not in lr_queue, it means that it is in one of the LRs.

No. When a disabled IRQ is injected to the VCPU, Xen puts it in inflight
but not in lr_queue. We don''t have a way to know whether the IRQ is
really in LRs or not.

-- 
Julien

On 06/25/2013 02:16 PM, Stefano Stabellini wrote:
> On Tue, 25 Jun 2013, Julien Grall wrote:
>> From: Julien Grall <julien.grall@linaro.org>
>>
>> When Xen needs to EOI a physical IRQ, we must use the IRQ number
>> in irq_desc instead of the virtual IRQ.
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/gic.c |    7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 177560e..0fee3f2 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -810,7 +810,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;
>>      uint32_t lr;
>>      struct vcpu *v = current;
>>      uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1])
<< 32);
>> @@ -846,6 +846,7 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>>              /* 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);
>> @@ -854,10 +855,10 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>>              /* 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 think that virq and pirq are guaranteed to always be the same, at
> least at the moment. Look at vgic_vcpu_inject_irq: it takes just one irq
> parameter, that is both the physical and the virtual irq number.
> Unless we change the vgic_vcpu_inject_irq interface to allow virq !>
pirq, I don''t think this patch makes much sense.

Right. I wrote this patch because it easier to forget to modify some
part when non-1:1 IRQ mappings will be created :).

-- 
Julien

On Tue, 2013-06-25 at 16:21 +0100, Julien Grall wrote:
> On 06/25/2013 02:16 PM, Stefano Stabellini wrote:
> 
> > On Tue, 25 Jun 2013, Julien Grall wrote:
> >> From: Julien Grall <julien.grall@linaro.org>
> >>
> >> When Xen needs to EOI a physical IRQ, we must use the IRQ number
> >> in irq_desc instead of the virtual IRQ.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> ---
> >>  xen/arch/arm/gic.c |    7 ++++---
> >>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >> index 177560e..0fee3f2 100644
> >> --- a/xen/arch/arm/gic.c
> >> +++ b/xen/arch/arm/gic.c
> >> @@ -810,7 +810,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;
> >>      uint32_t lr;
> >>      struct vcpu *v = current;
> >>      uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t)
GICH[GICH_EISR1]) << 32);
> >> @@ -846,6 +846,7 @@ static void maintenance_interrupt(int irq,
void *dev_id, struct cpu_user_regs *r
> >>              /* 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);
> >> @@ -854,10 +855,10 @@ static void maintenance_interrupt(int irq,
void *dev_id, struct cpu_user_regs *r
> >>              /* 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 think that virq and pirq are guaranteed to always be the same, at
> > least at the moment. Look at vgic_vcpu_inject_irq: it takes just one
irq
> > parameter, that is both the physical and the virtual irq number.
> 
> > Unless we change the vgic_vcpu_inject_irq interface to allow virq
!> > pirq, I don''t think this patch makes much sense.
But what is the downside?
> Right. I wrote this patch because it easier to forget to modify some
> part when non-1:1 IRQ mappings will be created :).
I''d be tempted to make this change on that basis, it is correct both
before and after any change to vgic_vcpu_inject_irq and doesn''t appear
to be expensive or anything. Not to mention that it is semantically
correct.


Ian.

On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
> From: Stefano Stabellini <stefano.stabellini@citrix.com>
> 
> For guest''s timers (both virtual and physical), Xen will inject
virtual
> interrupt. Linux handles timer interrupt as:
We should be wary of developing things based on the way which Linux
happens to do things. On x86 we have several time modes, which can be
selected based upon guest behaviour (described in
docs/man/xl.cfg.pod.5). Do we need to do something similar here?
>   1) Receive the interrupt and ack it
>   2) Handle the current event timer
>   3) Set the next event timer
>   4) EOI the interrupt
> 
> It''s unlikely possible to reinject another interrupt before
I can''t parse this sentence. "unlikely to be possible"
perhaps? but I''m
not sure if that is what you meant.
> the previous one is EOIed because the next deadline is shorter than the
time
> to execute code until EOI it.
If we get into this situation once is there any danger that we will get
into it repeatedly and overflow the count?

Overall I''m not convinced this is the right approach to get the
behaviour we want. Isn''t this interrupt level triggered, with the level
being determined by a comparison of two registers? IOW can''t we
determine whether to retrigger the interrupt or not by examining the
state of our emulated versions of those registers? A generic mechanism
to callback into the appropriate emulator on EOI plus a little bit of
logic in the vtimer code is all it ought to take.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@citrix.com>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/gic.c           |   35 +++++++++++++++++++++++------------
>  xen/arch/arm/vgic.c          |    1 +
>  xen/include/asm-arm/domain.h |    2 ++
>  3 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 0fee3f2..21575df 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -817,7 +817,7 @@ 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;
> +        struct pending_irq *p, *n;
>          int cpu, eoi;
>  
>          cpu = -1;
> @@ -826,13 +826,32 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>          spin_lock_irq(&gic.lock);
>          lr = GICH[GICH_LR + i];
>          virq = lr & GICH_LR_VIRTUAL_MASK;
> +
> +        p = irq_to_pending(v, virq);
> +        if ( p->desc != NULL ) {
> +            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;
> +        }
> +        if ( !atomic_dec_and_test(&p->inflight_cnt) )
> +        {
> +            /* Physical IRQ can''t be reinject */
> +            WARN_ON(p->desc != NULL);
> +            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> +            spin_unlock_irq(&gic.lock);
> +            i++;
> +            continue;
> +        }
> +
>          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);
> +            n = list_entry(v->arch.vgic.lr_pending.next, typeof(*n),
lr_queue);
> +            gic_set_lr(i, n->irq, GICH_LR_PENDING, n->priority);
> +            list_del_init(&n->lr_queue);
>              set_bit(i, &this_cpu(lr_mask));
>          } else {
>              gic_inject_irq_stop();
> @@ -840,14 +859,6 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>          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;
> -            /* 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);
>  
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 7eaccb7..2d91dce 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -672,6 +672,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int
irq, int virtual)
>  
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>  
> +    atomic_inc(&n->inflight_cnt);
>      /* vcpu offline or irq already pending */
>      if (test_bit(_VPF_down, &v->pause_flags) ||
!list_empty(&n->inflight))
>      {
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 339b6e6..fa0b776 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -8,6 +8,7 @@
>  #include <asm/p2m.h>
>  #include <asm/vfp.h>
>  #include <public/hvm/params.h>
> +#include <asm/atomic.h>
>  
>  /* Represents state corresponding to a block of 32 interrupts */
>  struct vgic_irq_rank {
> @@ -21,6 +22,7 @@ struct vgic_irq_rank {
>  struct pending_irq
>  {
>      int irq;
> +    atomic_t inflight_cnt;
>      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

On Tue, 2013-06-25 at 14:24 +0100, Stefano Stabellini
wrote:
> On Tue, 25 Jun 2013, Julien Grall wrote:
> > From: Julien Grall <julien.grall@linaro.org>
> > 
> > When an IRQ, marked as IRQS_ONESHOT, is injected Linux will:
> >   - Disable the IRQ
> >   - Call the interrupt handler
> >   - Conditionnally enable the IRQ
> >   - EOI the IRQ
> > 
> > When Linux will re-enable the IRQ, Xen will inject again the IRQ
because it''s
> > still inflight. Therefore, LRs will contains duplicated IRQs and Xen
will
> > EOI it twice if it''s a physical IRQ.
> > 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > ---
> >  xen/arch/arm/gic.c        |   27 +++++++++++++++++++++++++++
> >  xen/arch/arm/vgic.c       |    3 ++-
> >  xen/include/asm-arm/gic.h |    3 +++
> >  3 files changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 21575df..bf05716 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq *irq,
struct irqaction *new)
> >      return rc;
> >  }
> >  
> > +/* Check if an IRQ was already injected to the current VCPU */
> > +bool_t gic_irq_injected(unsigned int irq)
> 
> Can you rename it to something more specific, like gic_irq_inlr?
> 
> > +{
> > +    bool_t found = 0;
> > +    int i = 0;
> > +    unsigned int virq;
> > +
> > +    spin_lock_irq(&gic.lock);
> > +
> > +    while ( (i = find_next_bit((unsigned long
*)&this_cpu(lr_mask),
> > +                               nr_lrs, i)) < nr_lrs )
> > +    {
> > +        virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK;
> > +
> > +        if ( virq == irq )
> > +        {
> > +            found = 1;
> > +            break;
> > +        }
> > +        i++;
> > +    }
> 
> Instead of reading back all the GICH_LR registers, can''t just just
read
> the ones that have a corresponding bit set in lr_mask?
> 
> Also you should be able to avoid having to read the GICH_LR registers by
> simply checking if the irq is in the lr_queue list: if an irq is in
> inflight but not in lr_queue, it means that it is in one of the LRs.
This sounds roughly equivalent to what I was about to suggest which was
a bool in the irq descriptor.

In any case we should certainly try and avoid walking all the LRs via
some means.

Ian.

On Tue, 25 Jun 2013, Julien Grall wrote:
> If the guest VCPU receives an interrupts when it''s disabled, it
will throw
                                   ^interrupt
> away the IRQ with EOIed it.
What does this mean? I cannot parse the sentence.

> This is result to lost IRQ forever.
> Directly EOIed the interrupt doesn''t help because the IRQ could be
fired
> again and result to an infinited loop.
> 
> It happens during dom0 boot on the versatile express TC2 with the ethernet
> card.
> 
> Let the interrupt disabled when Xen setups the route and enable it when
Linux
> asks to enable it.
Is the problem that Xen keeps the interrupt enabled even when Linux
disables it at the gic level? Is this what this patch is trying to
address?

> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> ---
>  xen/arch/arm/domain_build.c |   14 ++++++++++++++
>  xen/arch/arm/gic.c          |   25 +++++++++++++++++++++++--
>  xen/arch/arm/vgic.c         |    7 +++----
>  xen/include/asm-arm/gic.h   |    4 ++++
>  xen/include/asm-arm/irq.h   |    6 ++++++
>  5 files changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f5befbd..0470a2d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -408,6 +408,20 @@ static int map_device(struct domain *d, const struct
dt_device_node *dev)
>          }
>  
>          DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq,
irq.type);
> +
> +        /*
> +         * Only map SGI interrupt in the guest as XEN won''t
handle
> +         * it correctly.
> +         * TODO: Fix it
> +         */
> +        if ( !irq_is_sgi(irq.irq) )
> +        {
> +            printk(XENLOG_WARNING "WARNING: Don''t map irq %u
for %s has "
> +                   "XEN doesn''t handle properly non-SGI
interrupt\n",
> +                   i, dt_node_full_name(dev));
do you mean SPI?

> +            continue;
> +        }
> +
>          /* Don''t check return because the IRQ can be use by
multiple device */
>          gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
>      }
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index b16ba8c..e7d082a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type = {
>      .set_affinity = gic_irq_set_affinity,
>  };
>  
> +void gic_irq_enable(struct irq_desc *desc)
> +{
> +    spin_lock_irq(&desc->lock);
> +    spin_lock(&gic.lock);
> +
> +    desc->handler->enable(desc);
> +
> +    spin_unlock(&gic.lock);
> +    spin_unlock_irq(&desc->lock);
> +}
This function looks a bit too similar to gic_irq_mask and friends,
except that it takes two locks.

To make that obvious it''s probably better to call it
gic_irq_enable_safe
or gic_irq_enable_locked.

>  /* needs to be called with gic.lock held */
>  static void gic_set_irq_properties(unsigned int irq, bool_t level,
>          unsigned int cpu_mask, unsigned int priority)
> @@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc, unsigned
int irq,
>      desc->status &= ~IRQ_DISABLED;
>      dsb();
>  
> -    desc->handler->startup(desc);
> -
>      return 0;
>  }
>  
> @@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq *irq,
struct irqaction *new)
>  
>      rc = __setup_irq(desc, irq->irq, new);
>  
> +    desc->handler->startup(desc);
> +
>      spin_unlock_irqrestore(&desc->lock, flags);
>  
>      return rc;
This two changes make it so guest irqs are not enabled by default, good.

> @@ -711,6 +722,7 @@ void gic_inject(void)
>          gic_inject_irq_start();
>  }
>  
> +/* TODO: Handle properly non SGI-interrupt */
>  int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>                             const char * devname)
>  {
> @@ -719,11 +731,18 @@ 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;
> +    /* XXX: handler other VCPU than 0 */
> +    struct vcpu *v = d->vcpu[0];
>  
>      action = xmalloc(struct irqaction);
>      if (!action)
>          return -ENOMEM;
>  
> +    /* XXX: Here we assume a 1:1 interrupt mapping between the host and
dom0 */
> +    BUG_ON(irq->irq >= (d->arch.vgic.nr_lines + NR_LOCAL_IRQS));
> +    p = irq_to_pending(v, irq->irq);
> +
>      action->dev_id = d;
>      action->name = devname;
>      action->free_on_release = 1;
> @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const
struct dt_irq *irq,
>          goto out;
>      }
>  
> +    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 cea9233..4f3d816 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -372,6 +372,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t
r, int n)
>          if ( !list_empty(&p->inflight) &&
!gic_irq_injected(irq) )
>              gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
>  
> +        if ( p->desc != NULL )
> +            gic_irq_enable(p->desc);
> +
>          i++;
>      }
>  }
Should we add a gic_irq_disable call when the guest disables irqs?

> @@ -685,10 +688,6 @@ 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
> -        n->desc = NULL;
>  
>      /* the irq is enabled */
>      if ( rank->ienable & (1 << (irq % 32)) )
I don''t quite understand why are you changing where the
"desc"
assignement is done.
If it is a cleanup you shouldn''t mix it with a patch like this that is
supposed to fix a specific issue. Otherwise please explain why you need
this change.

> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index f9e9ef1..f7f3c1e 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -134,6 +134,7 @@
>  
>  #ifndef __ASSEMBLY__
>  #include <xen/device_tree.h>
> +#include <xen/irq.h>
>  
>  extern int domain_vgic_init(struct domain *d);
>  extern void domain_vgic_free(struct domain *d);
> @@ -154,6 +155,9 @@ extern void gic_inject(void);
>  extern void gic_clear_pending_irqs(struct vcpu *v);
>  extern int gic_events_need_delivery(void);
>  
> +/* Helper to enable an IRQ and take all the needed locks */
> +extern void gic_irq_enable(struct irq_desc *desc);
> +
>  extern void __cpuinit init_maintenance_interrupt(void);
>  extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
>          unsigned int state, unsigned int priority);
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 80ff68d..346dc1d 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -46,6 +46,12 @@ int __init request_dt_irq(const struct dt_irq *irq,
>                            void *dev_id);
>  int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
>  
> +#define FIRST_SGI_IRQ   32
> +static inline bool_t irq_is_sgi(unsigned int irq)
> +{
> +    return (irq >= FIRST_SGI_IRQ);
> +}
Aren''t SGIs supposed to be between 0 and 15? Aren''t you
talking about
SPIs here?

On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
> If the guest VCPU receives an interrupts when it''s disabled, it
will throw
> away the IRQ with EOIed it.
Did you mean "without EOIing it" or perhaps "having EOIed
it"?
>  This is result to lost IRQ forever.
"This results in losing the IRQ forever".
> Directly EOIed the interrupt doesn''t help because the IRQ could be
fired
EOIing in this context.
> again and result to an infinited loop.
                        infinite
> It happens during dom0 boot on the versatile express TC2 with the ethernet
> card.
> 
> Let the interrupt disabled when Xen setups the route and enable it when
Linux
  "Lets ... when Xen sets up the route..."
> asks to enable it.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> ---
>  xen/arch/arm/domain_build.c |   14 ++++++++++++++
>  xen/arch/arm/gic.c          |   25 +++++++++++++++++++++++--
>  xen/arch/arm/vgic.c         |    7 +++----
>  xen/include/asm-arm/gic.h   |    4 ++++
>  xen/include/asm-arm/irq.h   |    6 ++++++
>  5 files changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f5befbd..0470a2d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -408,6 +408,20 @@ static int map_device(struct domain *d, const struct
dt_device_node *dev)
>          }
>  
>          DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq,
irq.type);
> +
> +        /*
> +         * Only map SGI interrupt in the guest as XEN won''t
handle
> +         * it correctly.
> +         * TODO: Fix it
> +         */
> +        if ( !irq_is_sgi(irq.irq) )
> +        {
> +            printk(XENLOG_WARNING "WARNING: Don''t map irq %u
for %s has "
s/has/as/ I think.

Did you really mean SGI here? I''d have thought from the context that
you
would mean SPIs. SGIs aren''t anything to do with any real devices
almost
by definition -- if you saw on in the device tree I''d be very
surprised!
> +                   "XEN doesn''t handle properly non-SGI
interrupt\n",
> +                   i, dt_node_full_name(dev));
> +            continue;
> +        }
> +
>          /* Don''t check return because the IRQ can be use by
multiple device */
>          gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
>      }
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index b16ba8c..e7d082a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type = {
>      .set_affinity = gic_irq_set_affinity,
>  };
>  
> +void gic_irq_enable(struct irq_desc *desc)
> +{
> +    spin_lock_irq(&desc->lock);
> +    spin_lock(&gic.lock);
> +
> +    desc->handler->enable(desc);
> +
> +    spin_unlock(&gic.lock);
> +    spin_unlock_irq(&desc->lock);
> +}
> +
>  /* needs to be called with gic.lock held */
>  static void gic_set_irq_properties(unsigned int irq, bool_t level,
>          unsigned int cpu_mask, unsigned int priority)
> @@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc, unsigned
int irq,
>      desc->status &= ~IRQ_DISABLED;
>      dsb();
>  
> -    desc->handler->startup(desc);
> -
>      return 0;
>  }
>  
> @@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq *irq,
struct irqaction *new)
>  
>      rc = __setup_irq(desc, irq->irq, new);
>  
> +    desc->handler->startup(desc);
> +
>      spin_unlock_irqrestore(&desc->lock, flags);
>  
>      return rc;
> @@ -711,6 +722,7 @@ void gic_inject(void)
>          gic_inject_irq_start();
>  }
>  
> +/* TODO: Handle properly non SGI-interrupt */
>  int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>                             const char * devname)
>  {
> @@ -719,11 +731,18 @@ 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;
> +    /* XXX: handler other VCPU than 0 */
That should be something like "XXX: handle VCPUs other than 0".

This only matters if we can route SGIs or PPIs to the guest though I
think, since they are the only banked interrupts? For SPIs we actually
want to actively avoid doing this multiple times, don''t we?

For the banked interrupts I think we just need a loop here, or for
p->desc to not be part of the pending_irq struct but actually part of
some separate per-domain datastructure, since it would be very weird to
have a domain where the PPIs differed between CPUs. (I''m not sure if
that is allowed by the hardware, I bet it is, but it would be a
pathological case IMHO...).

I think a perdomain irq_desc * array is probably the right answer,
unless someone can convincingly argue that PPI routing differing between
VCPUs in a guest is a useful thing...
> +    struct vcpu *v = d->vcpu[0];
>  
>      action = xmalloc(struct irqaction);
>      if (!action)
>          return -ENOMEM;
>  
> +    /* XXX: Here we assume a 1:1 interrupt mapping between the host and
dom0 */
> +    BUG_ON(irq->irq >= (d->arch.vgic.nr_lines + NR_LOCAL_IRQS));
> +    p = irq_to_pending(v, irq->irq);
> +
>      action->dev_id = d;
>      action->name = devname;
>      action->free_on_release = 1;
> @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const
struct dt_irq *irq,
>          goto out;
>      }
>  
> +    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 cea9233..4f3d816 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -372,6 +372,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t
r, int n)
>          if ( !list_empty(&p->inflight) &&
!gic_irq_injected(irq) )
>              gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
>  
> +        if ( p->desc != NULL )
> +            gic_irq_enable(p->desc);
> +
>          i++;
>      }
>  }
> @@ -685,10 +688,6 @@ 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
> -        n->desc = NULL;
>  
>      /* the irq is enabled */
>      if ( rank->ienable & (1 << (irq % 32)) )
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index f9e9ef1..f7f3c1e 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -134,6 +134,7 @@
>  
>  #ifndef __ASSEMBLY__
>  #include <xen/device_tree.h>
> +#include <xen/irq.h>
>  
>  extern int domain_vgic_init(struct domain *d);
>  extern void domain_vgic_free(struct domain *d);
> @@ -154,6 +155,9 @@ extern void gic_inject(void);
>  extern void gic_clear_pending_irqs(struct vcpu *v);
>  extern int gic_events_need_delivery(void);
>  
> +/* Helper to enable an IRQ and take all the needed locks */
> +extern void gic_irq_enable(struct irq_desc *desc);
> +
>  extern void __cpuinit init_maintenance_interrupt(void);
>  extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
>          unsigned int state, unsigned int priority);
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index 80ff68d..346dc1d 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -46,6 +46,12 @@ int __init request_dt_irq(const struct dt_irq *irq,
>                            void *dev_id);
>  int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new);
>  
> +#define FIRST_SGI_IRQ   32
> +static inline bool_t irq_is_sgi(unsigned int irq)
> +{
> +    return (irq >= FIRST_SGI_IRQ);
> +}
> +
>  #endif /* _ASM_HW_IRQ_H */
>  /*
>   * Local variables:

On Tue, 25 Jun 2013, Julien Grall wrote:
> On 06/25/2013 02:24 PM, Stefano Stabellini wrote:
> 
> > On Tue, 25 Jun 2013, Julien Grall wrote:
> >> From: Julien Grall <julien.grall@linaro.org>
> >>
> >> When an IRQ, marked as IRQS_ONESHOT, is injected Linux will:
> >>   - Disable the IRQ
> >>   - Call the interrupt handler
> >>   - Conditionnally enable the IRQ
> >>   - EOI the IRQ
> >>
> >> When Linux will re-enable the IRQ, Xen will inject again the IRQ
because it''s
> >> still inflight. Therefore, LRs will contains duplicated IRQs and
Xen will
> >> EOI it twice if it''s a physical IRQ.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >> ---
> >>  xen/arch/arm/gic.c        |   27 +++++++++++++++++++++++++++
> >>  xen/arch/arm/vgic.c       |    3 ++-
> >>  xen/include/asm-arm/gic.h |    3 +++
> >>  3 files changed, 32 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >> index 21575df..bf05716 100644
> >> --- a/xen/arch/arm/gic.c
> >> +++ b/xen/arch/arm/gic.c
> >> @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct dt_irq
*irq, struct irqaction *new)
> >>      return rc;
> >>  }
> >>  
> >> +/* Check if an IRQ was already injected to the current VCPU */
> >> +bool_t gic_irq_injected(unsigned int irq)
> > 
> > Can you rename it to something more specific, like gic_irq_inlr?
> > 
> >> +{
> >> +    bool_t found = 0;
> >> +    int i = 0;
> >> +    unsigned int virq;
> >> +
> >> +    spin_lock_irq(&gic.lock);
> >> +
> >> +    while ( (i = find_next_bit((unsigned long
*)&this_cpu(lr_mask),
> >> +                               nr_lrs, i)) < nr_lrs )
> >> +    {
> >> +        virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK;
> >> +
> >> +        if ( virq == irq )
> >> +        {
> >> +            found = 1;
> >> +            break;
> >> +        }
> >> +        i++;
> >> +    }
> > 
> > Instead of reading back all the GICH_LR registers, can''t just
just read
> > the ones that have a corresponding bit set in lr_mask?
> 
> It''s already the case, I use find_next_bit to find the next used
LRs.
> 
> > Also you should be able to avoid having to read the GICH_LR registers
by
> > simply checking if the irq is in the lr_queue list: if an irq is in
> > inflight but not in lr_queue, it means that it is in one of the LRs.
> 
> 
> No. When a disabled IRQ is injected to the VCPU, Xen puts it in inflight
> but not in lr_queue. We don''t have a way to know whether the IRQ
is
> really in LRs or not.
I think it''s time we introduce a "status" member in struct
irq_desc, so
that we are not dependent on the information in the GICH_LR registers or
the queue a pending_irq has been added to.
I would implement it as a bitfield:

int status;
#define GIC_IRQ_ENABLED  (1<<0)
#define GIC_IRQ_INFLIGHT (1<<1)
#define GIC_IRQ_INLR     (1<<2)

This way you should just go through the inflight queue and check whether
status & GIC_IRQ_INLR.

At the moment we just want to represent this basic state machine:

irq guest asserted -> inflight -> injected (inlr) -> unasserted
(maintenance interrupt)

On Tue, 2013-06-25 at 17:36 +0100, Stefano Stabellini
wrote:
> I think it''s time we introduce a "status" member in
struct irq_desc, so
> that we are not dependent on the information in the GICH_LR registers or
> the queue a pending_irq has been added to.
Yes please, I find this one of the hardest things to keep straight in my
head (not helped by my inability to remember which of pending and
inflight is which...)
> I would implement it as a bitfield:
> 
> int status;
> #define GIC_IRQ_ENABLED  (1<<0)
> #define GIC_IRQ_INFLIGHT (1<<1)
> #define GIC_IRQ_INLR     (1<<2)
> 
> This way you should just go through the inflight queue and check whether
> status & GIC_IRQ_INLR.
Since some of this stuff happens in interrupt context you probably want
test_bit/set_bit et al rather than regular boolean logic, don''t you?
> At the moment we just want to represent this basic state machine:
> 
> irq guest asserted -> inflight -> injected (inlr) -> unasserted
(maintenance interrupt)
Can we model the states after the active/pending states which the gic
has? It might make a bunch of stuff clearer?

Ian.

On 06/25/2013 05:36 PM, Stefano Stabellini wrote:
> On Tue, 25 Jun 2013, Julien Grall wrote:
>> On 06/25/2013 02:24 PM, Stefano Stabellini wrote:
>>
>>> On Tue, 25 Jun 2013, Julien Grall wrote:
>>>> From: Julien Grall <julien.grall@linaro.org>
>>>>
>>>> When an IRQ, marked as IRQS_ONESHOT, is injected Linux will:
>>>>   - Disable the IRQ
>>>>   - Call the interrupt handler
>>>>   - Conditionnally enable the IRQ
>>>>   - EOI the IRQ
>>>>
>>>> When Linux will re-enable the IRQ, Xen will inject again the
IRQ because it''s
>>>> still inflight. Therefore, LRs will contains duplicated IRQs
and Xen will
>>>> EOI it twice if it''s a physical IRQ.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>> ---
>>>>  xen/arch/arm/gic.c        |   27 +++++++++++++++++++++++++++
>>>>  xen/arch/arm/vgic.c       |    3 ++-
>>>>  xen/include/asm-arm/gic.h |    3 +++
>>>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>>> index 21575df..bf05716 100644
>>>> --- a/xen/arch/arm/gic.c
>>>> +++ b/xen/arch/arm/gic.c
>>>> @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct
dt_irq *irq, struct irqaction *new)
>>>>      return rc;
>>>>  }
>>>>  
>>>> +/* Check if an IRQ was already injected to the current VCPU */
>>>> +bool_t gic_irq_injected(unsigned int irq)
>>>
>>> Can you rename it to something more specific, like gic_irq_inlr?
>>>
>>>> +{
>>>> +    bool_t found = 0;
>>>> +    int i = 0;
>>>> +    unsigned int virq;
>>>> +
>>>> +    spin_lock_irq(&gic.lock);
>>>> +
>>>> +    while ( (i = find_next_bit((unsigned long
*)&this_cpu(lr_mask),
>>>> +                               nr_lrs, i)) < nr_lrs )
>>>> +    {
>>>> +        virq = GICH[GICH_LR + i] & GICH_LR_VIRTUAL_MASK;
>>>> +
>>>> +        if ( virq == irq )
>>>> +        {
>>>> +            found = 1;
>>>> +            break;
>>>> +        }
>>>> +        i++;
>>>> +    }
>>>
>>> Instead of reading back all the GICH_LR registers, can''t
just just read
>>> the ones that have a corresponding bit set in lr_mask?
>>
>> It''s already the case, I use find_next_bit to find the next
used LRs.
>>
>>> Also you should be able to avoid having to read the GICH_LR
registers by
>>> simply checking if the irq is in the lr_queue list: if an irq is in
>>> inflight but not in lr_queue, it means that it is in one of the
LRs.
>>
>>
>> No. When a disabled IRQ is injected to the VCPU, Xen puts it in
inflight
>> but not in lr_queue. We don''t have a way to know whether the
IRQ is
>> really in LRs or not.
> 
> I think it''s time we introduce a "status" member in
struct irq_desc, so
Do you mean struct irq_pending?
> that we are not dependent on the information in the GICH_LR registers or
> the queue a pending_irq has been added to.
> I would implement it as a bitfield:
> 
> int status;
> #define GIC_IRQ_ENABLED  (1<<0)
> #define GIC_IRQ_INFLIGHT (1<<1)
> #define GIC_IRQ_INLR     (1<<2)
> 
> This way you should just go through the inflight queue and check whether
> status & GIC_IRQ_INLR.
> 
> At the moment we just want to represent this basic state machine:
> 
> irq guest asserted -> inflight -> injected (inlr) -> unasserted
(maintenance interrupt)

Sounds good to me. I will try to implement this approach.

Moreover, it will fix another issue with this patch. I have just noticed
that it''s possible to reinject an IRQ on different vCPU even if
it''s
injected on another vCPU.

-- 
Julien

On 06/25/2013 05:19 PM, Stefano Stabellini wrote:
> On Tue, 25 Jun 2013, Julien Grall wrote:
>> If the guest VCPU receives an interrupts when it''s disabled,
it will throw
>                                    ^interrupt
> 
>> away the IRQ with EOIed it.
> 
> What does this mean? I cannot parse the sentence.
"it will throw away the IRQ without EOIing".
> 
>> This is result to lost IRQ forever.
>> Directly EOIed the interrupt doesn''t help because the IRQ
could be fired
>> again and result to an infinited loop.
>>
>> It happens during dom0 boot on the versatile express TC2 with the
ethernet
>> card.
>>
>> Let the interrupt disabled when Xen setups the route and enable it when
Linux
>> asks to enable it.
> 
> Is the problem that Xen keeps the interrupt enabled even when Linux
> disables it at the gic level? Is this what this patch is trying to
> address?
This patch only delays the physical IRQ activation until Linux will
enable it. This patch avoids to lose IRQ when a VCPU is disabled.

I tried to also disable the IRQ when Linux asks to disable it but the
versatile express platform hang.
> 
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>> ---
>>  xen/arch/arm/domain_build.c |   14 ++++++++++++++
>>  xen/arch/arm/gic.c          |   25 +++++++++++++++++++++++--
>>  xen/arch/arm/vgic.c         |    7 +++----
>>  xen/include/asm-arm/gic.h   |    4 ++++
>>  xen/include/asm-arm/irq.h   |    6 ++++++
>>  5 files changed, 50 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index f5befbd..0470a2d 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -408,6 +408,20 @@ static int map_device(struct domain *d, const
struct dt_device_node *dev)
>>          }
>>  
>>          DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq,
irq.type);
>> +
>> +        /*
>> +         * Only map SGI interrupt in the guest as XEN won''t
handle
>> +         * it correctly.
>> +         * TODO: Fix it
>> +         */
>> +        if ( !irq_is_sgi(irq.irq) )
>> +        {
>> +            printk(XENLOG_WARNING "WARNING: Don''t map
irq %u for %s has "
>> +                   "XEN doesn''t handle properly non-SGI
interrupt\n",
>> +                   i, dt_node_full_name(dev));
> 
> do you mean SPI?
> 
> 
>> +            continue;
>> +        }
>> +
>>          /* Don''t check return because the IRQ can be use by
multiple device */
>>          gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
>>      }
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index b16ba8c..e7d082a 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type = {
>>      .set_affinity = gic_irq_set_affinity,
>>  };
>>  
>> +void gic_irq_enable(struct irq_desc *desc)
>> +{
>> +    spin_lock_irq(&desc->lock);
>> +    spin_lock(&gic.lock);
>> +
>> +    desc->handler->enable(desc);
>> +
>> +    spin_unlock(&gic.lock);
>> +    spin_unlock_irq(&desc->lock);
>> +}
> 
> This function looks a bit too similar to gic_irq_mask and friends,
> except that it takes two locks.
> 
> To make that obvious it''s probably better to call it
gic_irq_enable_safe
> or gic_irq_enable_locked.
> 
> 
>>  /* needs to be called with gic.lock held */
>>  static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>          unsigned int cpu_mask, unsigned int priority)
>> @@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc,
unsigned int irq,
>>      desc->status &= ~IRQ_DISABLED;
>>      dsb();
>>  
>> -    desc->handler->startup(desc);
>> -
>>      return 0;
>>  }
>>  
>> @@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq *irq,
struct irqaction *new)
>>  
>>      rc = __setup_irq(desc, irq->irq, new);
>>  
>> +    desc->handler->startup(desc);
>> +
>>      spin_unlock_irqrestore(&desc->lock, flags);
>>  
>>      return rc;
> 
> This two changes make it so guest irqs are not enabled by default, good.
> 
> 
>> @@ -711,6 +722,7 @@ void gic_inject(void)
>>          gic_inject_irq_start();
>>  }
>>  
>> +/* TODO: Handle properly non SGI-interrupt */
>>  int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>>                             const char * devname)
>>  {
>> @@ -719,11 +731,18 @@ 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;
>> +    /* XXX: handler other VCPU than 0 */
>> +    struct vcpu *v = d->vcpu[0];
>>  
>>      action = xmalloc(struct irqaction);
>>      if (!action)
>>          return -ENOMEM;
>>  
>> +    /* XXX: Here we assume a 1:1 interrupt mapping between the host
and dom0 */
>> +    BUG_ON(irq->irq >= (d->arch.vgic.nr_lines +
NR_LOCAL_IRQS));
>> +    p = irq_to_pending(v, irq->irq);
>> +
>>      action->dev_id = d;
>>      action->name = devname;
>>      action->free_on_release = 1;
>> @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const
struct dt_irq *irq,
>>          goto out;
>>      }
>>  
>> +    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 cea9233..4f3d816 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -372,6 +372,9 @@ static void vgic_enable_irqs(struct vcpu *v,
uint32_t r, int n)
>>          if ( !list_empty(&p->inflight) &&
!gic_irq_injected(irq) )
>>              gic_set_guest_irq(v, irq, GICH_LR_PENDING,
p->priority);
>>  
>> +        if ( p->desc != NULL )
>> +            gic_irq_enable(p->desc);
>> +
>>          i++;
>>      }
>>  }
> 
> Should we add a gic_irq_disable call when the guest disables irqs?
> 
> 
>> @@ -685,10 +688,6 @@ 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
>> -        n->desc = NULL;
>>  
>>      /* the irq is enabled */
>>      if ( rank->ienable & (1 << (irq % 32)) )
> 
> I don''t quite understand why are you changing where the
"desc"
> assignement is done.
> If it is a cleanup you shouldn''t mix it with a patch like this
that is
> supposed to fix a specific issue. Otherwise please explain why you need
> this change.
> 
> 
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index f9e9ef1..f7f3c1e 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -134,6 +134,7 @@
>>  
>>  #ifndef __ASSEMBLY__
>>  #include <xen/device_tree.h>
>> +#include <xen/irq.h>
>>  
>>  extern int domain_vgic_init(struct domain *d);
>>  extern void domain_vgic_free(struct domain *d);
>> @@ -154,6 +155,9 @@ extern void gic_inject(void);
>>  extern void gic_clear_pending_irqs(struct vcpu *v);
>>  extern int gic_events_need_delivery(void);
>>  
>> +/* Helper to enable an IRQ and take all the needed locks */
>> +extern void gic_irq_enable(struct irq_desc *desc);
>> +
>>  extern void __cpuinit init_maintenance_interrupt(void);
>>  extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
>>          unsigned int state, unsigned int priority);
>> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
>> index 80ff68d..346dc1d 100644
>> --- a/xen/include/asm-arm/irq.h
>> +++ b/xen/include/asm-arm/irq.h
>> @@ -46,6 +46,12 @@ int __init request_dt_irq(const struct dt_irq *irq,
>>                            void *dev_id);
>>  int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction
*new);
>>  
>> +#define FIRST_SGI_IRQ   32
>> +static inline bool_t irq_is_sgi(unsigned int irq)
>> +{
>> +    return (irq >= FIRST_SGI_IRQ);
>> +}
> 
> Aren''t SGIs supposed to be between 0 and 15? Aren''t you
talking about
> SPIs here?

On Tue, 25 Jun 2013, Ian Campbell wrote:
> On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
> > From: Stefano Stabellini <stefano.stabellini@citrix.com>
> > 
> > For guest''s timers (both virtual and physical), Xen will
inject virtual
> > interrupt. Linux handles timer interrupt as:
> 
> We should be wary of developing things based on the way which Linux
> happens to do things. On x86 we have several time modes, which can be
> selected based upon guest behaviour (described in
> docs/man/xl.cfg.pod.5). Do we need to do something similar here?
> 
> >   1) Receive the interrupt and ack it
> >   2) Handle the current event timer
> >   3) Set the next event timer
> >   4) EOI the interrupt
> > 
> > It''s unlikely possible to reinject another interrupt before
> 
> I can''t parse this sentence. "unlikely to be possible"
perhaps? but I''m
> not sure if that is what you meant.
> 
> > the previous one is EOIed because the next deadline is shorter than
the time
> > to execute code until EOI it.
> 
> If we get into this situation once is there any danger that we will get
> into it repeatedly and overflow the count?
> 
> Overall I''m not convinced this is the right approach to get the
> behaviour we want. Isn''t this interrupt level triggered, with the
level
> being determined by a comparison of two registers? IOW can''t we
> determine whether to retrigger the interrupt or not by examining the
> state of our emulated versions of those registers? A generic mechanism
> to callback into the appropriate emulator on EOI plus a little bit of
> logic in the vtimer code is all it ought to take.
AFAICT this what could happen:

- vtimer fires
- xen mask the vtimer
- xen adds the vtimer to the LR for the guest
- xen EOIs the vtimer
- the guest receive the vtimer interrupt
- the guest set the next deadline in the past
- the guest enables the vtimer
## an unexpected vtimer interrupt is received by Xen but the current
## one is still being serviced 
- the guest eoi the vtimer

as a result the guest looses an interrupt.
Julien, is that correct?

If that is the case, this can only happen once, right?  In that case
rather than an atomic_t we could just have a bit in the status field I
proposed before. It should be enough for us to keep track of the case
when the irq is supposed to stay high even after the guest EOIs it. (Of
course that means that we need to re-inject it into the guest).

> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@citrix.com>
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > ---
> >  xen/arch/arm/gic.c           |   35
+++++++++++++++++++++++------------
> >  xen/arch/arm/vgic.c          |    1 +
> >  xen/include/asm-arm/domain.h |    2 ++
> >  3 files changed, 26 insertions(+), 12 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 0fee3f2..21575df 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -817,7 +817,7 @@ 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;
> > +        struct pending_irq *p, *n;
> >          int cpu, eoi;
> >  
> >          cpu = -1;
> > @@ -826,13 +826,32 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
> >          spin_lock_irq(&gic.lock);
> >          lr = GICH[GICH_LR + i];
> >          virq = lr & GICH_LR_VIRTUAL_MASK;
> > +
> > +        p = irq_to_pending(v, virq);
> > +        if ( p->desc != NULL ) {
> > +            p->desc->status &= ~IRQ_INPROGRESS;
Now that I am thinking about this, shouldn''t this be protected by
taking
the desc->lock?
> > +            /* Assume only one pcpu needs to EOI the irq */
> > +            cpu = p->desc->arch.eoi_cpu;
> > +            eoi = 1;
> > +            pirq = p->desc->irq;
> > +        }
> > +        if ( !atomic_dec_and_test(&p->inflight_cnt) )
> > +        {
> > +            /* Physical IRQ can''t be reinject */
> > +            WARN_ON(p->desc != NULL);
> > +            gic_set_lr(i, p->irq, GICH_LR_PENDING,
p->priority);
> > +            spin_unlock_irq(&gic.lock);
> > +            i++;
> > +            continue;
> > +        }
> > +
> >          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);
> > +            n = list_entry(v->arch.vgic.lr_pending.next,
typeof(*n), lr_queue);
> > +            gic_set_lr(i, n->irq, GICH_LR_PENDING,
n->priority);
> > +            list_del_init(&n->lr_queue);
> >              set_bit(i, &this_cpu(lr_mask));
> >          } else {
> >              gic_inject_irq_stop();
> > @@ -840,14 +859,6 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
> >          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;
> > -            /* 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);
> >

On Tue, 25 Jun 2013, Julien Grall wrote:
> On 06/25/2013 05:36 PM, Stefano Stabellini wrote:
> 
> > On Tue, 25 Jun 2013, Julien Grall wrote:
> >> On 06/25/2013 02:24 PM, Stefano Stabellini wrote:
> >>
> >>> On Tue, 25 Jun 2013, Julien Grall wrote:
> >>>> From: Julien Grall <julien.grall@linaro.org>
> >>>>
> >>>> When an IRQ, marked as IRQS_ONESHOT, is injected Linux
will:
> >>>>   - Disable the IRQ
> >>>>   - Call the interrupt handler
> >>>>   - Conditionnally enable the IRQ
> >>>>   - EOI the IRQ
> >>>>
> >>>> When Linux will re-enable the IRQ, Xen will inject again
the IRQ because it''s
> >>>> still inflight. Therefore, LRs will contains duplicated
IRQs and Xen will
> >>>> EOI it twice if it''s a physical IRQ.
> >>>>
> >>>> Signed-off-by: Julien Grall
<julien.grall@linaro.org>
> >>>> ---
> >>>>  xen/arch/arm/gic.c        |   27
+++++++++++++++++++++++++++
> >>>>  xen/arch/arm/vgic.c       |    3 ++-
> >>>>  xen/include/asm-arm/gic.h |    3 +++
> >>>>  3 files changed, 32 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >>>> index 21575df..bf05716 100644
> >>>> --- a/xen/arch/arm/gic.c
> >>>> +++ b/xen/arch/arm/gic.c
> >>>> @@ -570,6 +570,33 @@ int __init setup_dt_irq(const struct
dt_irq *irq, struct irqaction *new)
> >>>>      return rc;
> >>>>  }
> >>>>  
> >>>> +/* Check if an IRQ was already injected to the current
VCPU */
> >>>> +bool_t gic_irq_injected(unsigned int irq)
> >>>
> >>> Can you rename it to something more specific, like
gic_irq_inlr?
> >>>
> >>>> +{
> >>>> +    bool_t found = 0;
> >>>> +    int i = 0;
> >>>> +    unsigned int virq;
> >>>> +
> >>>> +    spin_lock_irq(&gic.lock);
> >>>> +
> >>>> +    while ( (i = find_next_bit((unsigned long
*)&this_cpu(lr_mask),
> >>>> +                               nr_lrs, i)) < nr_lrs )
> >>>> +    {
> >>>> +        virq = GICH[GICH_LR + i] &
GICH_LR_VIRTUAL_MASK;
> >>>> +
> >>>> +        if ( virq == irq )
> >>>> +        {
> >>>> +            found = 1;
> >>>> +            break;
> >>>> +        }
> >>>> +        i++;
> >>>> +    }
> >>>
> >>> Instead of reading back all the GICH_LR registers,
can''t just just read
> >>> the ones that have a corresponding bit set in lr_mask?
> >>
> >> It''s already the case, I use find_next_bit to find the
next used LRs.
> >>
> >>> Also you should be able to avoid having to read the GICH_LR
registers by
> >>> simply checking if the irq is in the lr_queue list: if an irq
is in
> >>> inflight but not in lr_queue, it means that it is in one of
the LRs.
> >>
> >>
> >> No. When a disabled IRQ is injected to the VCPU, Xen puts it in
inflight
> >> but not in lr_queue. We don''t have a way to know whether
the IRQ is
> >> really in LRs or not.
> > 
> > I think it''s time we introduce a "status" member in
struct irq_desc, so
> 
> Do you mean struct irq_pending?
Yes, sorry I meant struct irq_pending

On Tue, 25 Jun 2013, Ian Campbell wrote:
> On Tue, 2013-06-25 at 17:36 +0100, Stefano Stabellini wrote:
> > I think it''s time we introduce a "status" member in
struct irq_desc, so
> > that we are not dependent on the information in the GICH_LR registers
or
> > the queue a pending_irq has been added to.
> 
> Yes please, I find this one of the hardest things to keep straight in my
> head (not helped by my inability to remember which of pending and
> inflight is which...)
> 
> > I would implement it as a bitfield:
> > 
> > int status;
> > #define GIC_IRQ_ENABLED  (1<<0)
> > #define GIC_IRQ_INFLIGHT (1<<1)
> > #define GIC_IRQ_INLR     (1<<2)
> > 
> > This way you should just go through the inflight queue and check
whether
> > status & GIC_IRQ_INLR.
> 
> Since some of this stuff happens in interrupt context you probably want
> test_bit/set_bit et al rather than regular boolean logic, don''t
you?
> 
> > At the moment we just want to represent this basic state machine:
> > 
> > irq guest asserted -> inflight -> injected (inlr) ->
unasserted (maintenance interrupt)
> 
> Can we model the states after the active/pending states which the gic
> has? It might make a bunch of stuff clearer?
It might be worth storing those too.
So maybe:

#define GIC_IRQ_ENABLED           (1<<0)
#define GIC_IRQ_PENDING           (1<<1)
#define GIC_IRQ_ACTIVE            (1<<2)
#define GIC_IRQ_GUEST_INFLIGHT    (1<<3)
#define GIC_IRQ_GUEST_INLR        (1<<4)

however if we do store the physical gic states (GIC_IRQ_PENDING and
GIC_IRQ_ACTIVE) then maybe the right place for them is actually irq_desc
rather than irq_pending that is used just for guest irqs.

On Tue, 25 Jun 2013, Julien Grall wrote:
> On 06/25/2013 05:19 PM, Stefano Stabellini wrote:
> 
> > On Tue, 25 Jun 2013, Julien Grall wrote:
> >> If the guest VCPU receives an interrupts when it''s
disabled, it will throw
> >                                    ^interrupt
> > 
> >> away the IRQ with EOIed it.
> > 
> > What does this mean? I cannot parse the sentence.
> 
> "it will throw away the IRQ without EOIing".
> 
> > 
> >> This is result to lost IRQ forever.
> >> Directly EOIed the interrupt doesn''t help because the IRQ
could be fired
> >> again and result to an infinited loop.
> >>
> >> It happens during dom0 boot on the versatile express TC2 with the
ethernet
> >> card.
> >>
> >> Let the interrupt disabled when Xen setups the route and enable it
when Linux
> >> asks to enable it.
> > 
> > Is the problem that Xen keeps the interrupt enabled even when Linux
> > disables it at the gic level? Is this what this patch is trying to
> > address?
> 
> This patch only delays the physical IRQ activation until Linux will
> enable it. This patch avoids to lose IRQ when a VCPU is disabled.
> 
> I tried to also disable the IRQ when Linux asks to disable it but the
> versatile express platform hang.
do you know why? and the arndale?

On 06/25/2013 05:28 PM, Ian Campbell wrote:
> On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
>> If the guest VCPU receives an interrupts when it''s disabled,
it will throw
>> away the IRQ with EOIed it.
> 
> Did you mean "without EOIing it" or perhaps "having EOIed
it"?
> 
>>  This is result to lost IRQ forever.
> 
> "This results in losing the IRQ forever".
> 
>> Directly EOIed the interrupt doesn''t help because the IRQ
could be fired
> 
> EOIing in this context.
> 
>> again and result to an infinited loop.
> 
>                         infinite
> 
>> It happens during dom0 boot on the versatile express TC2 with the
ethernet
>> card.
>>
>> Let the interrupt disabled when Xen setups the route and enable it when
Linux
> 
>   "Lets ... when Xen sets up the route..."
> 
>> asks to enable it.
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>> ---
>>  xen/arch/arm/domain_build.c |   14 ++++++++++++++
>>  xen/arch/arm/gic.c          |   25 +++++++++++++++++++++++--
>>  xen/arch/arm/vgic.c         |    7 +++----
>>  xen/include/asm-arm/gic.h   |    4 ++++
>>  xen/include/asm-arm/irq.h   |    6 ++++++
>>  5 files changed, 50 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index f5befbd..0470a2d 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -408,6 +408,20 @@ static int map_device(struct domain *d, const
struct dt_device_node *dev)
>>          }
>>  
>>          DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq,
irq.type);
>> +
>> +        /*
>> +         * Only map SGI interrupt in the guest as XEN won''t
handle
>> +         * it correctly.
>> +         * TODO: Fix it
>> +         */
>> +        if ( !irq_is_sgi(irq.irq) )
>> +        {
>> +            printk(XENLOG_WARNING "WARNING: Don''t map
irq %u for %s has "
> 
> s/has/as/ I think.
> 
> Did you really mean SGI here? I''d have thought from the context
that you
> would mean SPIs. SGIs aren''t anything to do with any real devices
almost
> by definition -- if you saw on in the device tree I''d be very
surprised!

It was SPIs. I will fix it in the next patch series.
> 
>> +                   "XEN doesn''t handle properly non-SGI
interrupt\n",
>> +                   i, dt_node_full_name(dev));
>> +            continue;
>> +        }
>> +
>>          /* Don''t check return because the IRQ can be use by
multiple device */
>>          gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
>>      }
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index b16ba8c..e7d082a 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type = {
>>      .set_affinity = gic_irq_set_affinity,
>>  };
>>  
>> +void gic_irq_enable(struct irq_desc *desc)
>> +{
>> +    spin_lock_irq(&desc->lock);
>> +    spin_lock(&gic.lock);
>> +
>> +    desc->handler->enable(desc);
>> +
>> +    spin_unlock(&gic.lock);
>> +    spin_unlock_irq(&desc->lock);
>> +}
>> +
>>  /* needs to be called with gic.lock held */
>>  static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>          unsigned int cpu_mask, unsigned int priority)
>> @@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc,
unsigned int irq,
>>      desc->status &= ~IRQ_DISABLED;
>>      dsb();
>>  
>> -    desc->handler->startup(desc);
>> -
>>      return 0;
>>  }
>>  
>> @@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq *irq,
struct irqaction *new)
>>  
>>      rc = __setup_irq(desc, irq->irq, new);
>>  
>> +    desc->handler->startup(desc);
>> +
>>      spin_unlock_irqrestore(&desc->lock, flags);
>>  
>>      return rc;
>> @@ -711,6 +722,7 @@ void gic_inject(void)
>>          gic_inject_irq_start();
>>  }
>>  
>> +/* TODO: Handle properly non SGI-interrupt */
>>  int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
>>                             const char * devname)
>>  {
>> @@ -719,11 +731,18 @@ 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;
>> +    /* XXX: handler other VCPU than 0 */
> 
> That should be something like "XXX: handle VCPUs other than 0".
> 
> This only matters if we can route SGIs or PPIs to the guest though I
> think, since they are the only banked interrupts? For SPIs we actually
> want to actively avoid doing this multiple times, don''t we?

Yes. Here the VCPU is only used to retrieved the struct pending_irq.
> 
> For the banked interrupts I think we just need a loop here, or for
> p->desc to not be part of the pending_irq struct but actually part of
> some separate per-domain datastructure, since it would be very weird to
> have a domain where the PPIs differed between CPUs. (I''m not sure
if
> that is allowed by the hardware, I bet it is, but it would be a
> pathological case IMHO...).
> I think a perdomain irq_desc * array is probably the right answer,
> unless someone can convincingly argue that PPI routing differing between
> VCPUs in a guest is a useful thing...

Until now, I didn''t see PPIs on other devices than the arch timers and
the GIC. I don''t know if it''s possible, but pending_irq are
also banked
for PPIs, so it''s not an issue.

The issue is how do we link the physical PPI to the virtual PPI? Is a
1:1 mapping. How does Xen handle PPI when a it is coming on VCPUs which
doesn''t handle it (for instance a domU)?
>> +    struct vcpu *v = d->vcpu[0];
>>  
>>      action = xmalloc(struct irqaction);
>>      if (!action)
>>          return -ENOMEM;
>>  
>> +    /* XXX: Here we assume a 1:1 interrupt mapping between the host
and dom0 */
>> +    BUG_ON(irq->irq >= (d->arch.vgic.nr_lines +
NR_LOCAL_IRQS));
>> +    p = irq_to_pending(v, irq->irq);
>> +
>>      action->dev_id = d;
>>      action->name = devname;
>>      action->free_on_release = 1;
>> @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const
struct dt_irq *irq,
>>          goto out;
>>      }
>>  
>> +    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 cea9233..4f3d816 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -372,6 +372,9 @@ static void vgic_enable_irqs(struct vcpu *v,
uint32_t r, int n)
>>          if ( !list_empty(&p->inflight) &&
!gic_irq_injected(irq) )
>>              gic_set_guest_irq(v, irq, GICH_LR_PENDING,
p->priority);
>>  
>> +        if ( p->desc != NULL )
>> +            gic_irq_enable(p->desc);
>> +
>>          i++;
>>      }
>>  }
>> @@ -685,10 +688,6 @@ 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
>> -        n->desc = NULL;
>>  
>>      /* the irq is enabled */
>>      if ( rank->ienable & (1 << (irq % 32)) )
>> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
>> index f9e9ef1..f7f3c1e 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -134,6 +134,7 @@
>>  
>>  #ifndef __ASSEMBLY__
>>  #include <xen/device_tree.h>
>> +#include <xen/irq.h>
>>  
>>  extern int domain_vgic_init(struct domain *d);
>>  extern void domain_vgic_free(struct domain *d);
>> @@ -154,6 +155,9 @@ extern void gic_inject(void);
>>  extern void gic_clear_pending_irqs(struct vcpu *v);
>>  extern int gic_events_need_delivery(void);
>>  
>> +/* Helper to enable an IRQ and take all the needed locks */
>> +extern void gic_irq_enable(struct irq_desc *desc);
>> +
>>  extern void __cpuinit init_maintenance_interrupt(void);
>>  extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
>>          unsigned int state, unsigned int priority);
>> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
>> index 80ff68d..346dc1d 100644
>> --- a/xen/include/asm-arm/irq.h
>> +++ b/xen/include/asm-arm/irq.h
>> @@ -46,6 +46,12 @@ int __init request_dt_irq(const struct dt_irq *irq,
>>                            void *dev_id);
>>  int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction
*new);
>>  
>> +#define FIRST_SGI_IRQ   32
>> +static inline bool_t irq_is_sgi(unsigned int irq)
>> +{
>> +    return (irq >= FIRST_SGI_IRQ);
>> +}
>> +
>>  #endif /* _ASM_HW_IRQ_H */
>>  /*
>>   * Local variables:
> 
>

On 06/25/2013 05:58 PM, Stefano Stabellini wrote:
> On Tue, 25 Jun 2013, Ian Campbell wrote:
>> On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
>>> From: Stefano Stabellini <stefano.stabellini@citrix.com>
>>>
>>> For guest''s timers (both virtual and physical), Xen will
inject virtual
>>> interrupt. Linux handles timer interrupt as:
>>
>> We should be wary of developing things based on the way which Linux
>> happens to do things. On x86 we have several time modes, which can be
>> selected based upon guest behaviour (described in
>> docs/man/xl.cfg.pod.5). Do we need to do something similar here?
>>
>>>   1) Receive the interrupt and ack it
>>>   2) Handle the current event timer
>>>   3) Set the next event timer
>>>   4) EOI the interrupt
>>>
>>> It''s unlikely possible to reinject another interrupt
before
>>
>> I can''t parse this sentence. "unlikely to be
possible" perhaps? but I''m
>> not sure if that is what you meant.
>>
>>> the previous one is EOIed because the next deadline is shorter than
the time
>>> to execute code until EOI it.
>>
>> If we get into this situation once is there any danger that we will get
>> into it repeatedly and overflow the count?
>>
>> Overall I''m not convinced this is the right approach to get
the
>> behaviour we want. Isn''t this interrupt level triggered, with
the level
>> being determined by a comparison of two registers? IOW can''t
we
>> determine whether to retrigger the interrupt or not by examining the
>> state of our emulated versions of those registers? A generic mechanism
>> to callback into the appropriate emulator on EOI plus a little bit of
>> logic in the vtimer code is all it ought to take.
> 
> AFAICT this what could happen:
> 
> - vtimer fires
> - xen mask the vtimer
> - xen adds the vtimer to the LR for the guest
> - xen EOIs the vtimer
> - the guest receive the vtimer interrupt
> - the guest set the next deadline in the past
> - the guest enables the vtimer
> ## an unexpected vtimer interrupt is received by Xen but the current
> ## one is still being serviced 
> - the guest eoi the vtimer
> 
> as a result the guest looses an interrupt.
> Julien, is that correct?
Yes.
> If that is the case, this can only happen once, right?  In that case
> rather than an atomic_t we could just have a bit in the status field I
> proposed before. It should be enough for us to keep track of the case
> when the irq is supposed to stay high even after the guest EOIs it. (Of
> course that means that we need to re-inject it into the guest).

For the timer yes. I wonder, what happen if Xen receive an SGI (for
instance SCHEDULE ipi) twice, does Xen must re-inject it multiple times?
>>> Signed-off-by: Stefano Stabellini
<stefano.stabellini@citrix.com>
>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>> ---
>>>  xen/arch/arm/gic.c           |   35
+++++++++++++++++++++++------------
>>>  xen/arch/arm/vgic.c          |    1 +
>>>  xen/include/asm-arm/domain.h |    2 ++
>>>  3 files changed, 26 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index 0fee3f2..21575df 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -817,7 +817,7 @@ 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;
>>> +        struct pending_irq *p, *n;
>>>          int cpu, eoi;
>>>  
>>>          cpu = -1;
>>> @@ -826,13 +826,32 @@ static void maintenance_interrupt(int irq,
void *dev_id, struct cpu_user_regs *r
>>>          spin_lock_irq(&gic.lock);
>>>          lr = GICH[GICH_LR + i];
>>>          virq = lr & GICH_LR_VIRTUAL_MASK;
>>> +
>>> +        p = irq_to_pending(v, virq);
>>> +        if ( p->desc != NULL ) {
>>> +            p->desc->status &= ~IRQ_INPROGRESS;
> 
> Now that I am thinking about this, shouldn''t this be protected by
taking
> the desc->lock?
Right. I don''t think desc->lock is enough if we want to protect from
release_irq.
If this function is called, it will wait until IRQ_INPROGRESS is
removed. How about moving ~IRQ_INPROGRESS at then end of the block and
add a dmb() before?
>>> +            /* Assume only one pcpu needs to EOI the irq */
>>> +            cpu = p->desc->arch.eoi_cpu;
>>> +            eoi = 1;
>>> +            pirq = p->desc->irq;
>>> +        }
>>> +        if ( !atomic_dec_and_test(&p->inflight_cnt) )
>>> +        {
>>> +            /* Physical IRQ can''t be reinject */
>>> +            WARN_ON(p->desc != NULL);
>>> +            gic_set_lr(i, p->irq, GICH_LR_PENDING,
p->priority);
>>> +            spin_unlock_irq(&gic.lock);
>>> +            i++;
>>> +            continue;
>>> +        }
>>> +
>>>          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);
>>> +            n = list_entry(v->arch.vgic.lr_pending.next,
typeof(*n), lr_queue);
>>> +            gic_set_lr(i, n->irq, GICH_LR_PENDING,
n->priority);
>>> +            list_del_init(&n->lr_queue);
>>>              set_bit(i, &this_cpu(lr_mask));
>>>          } else {
>>>              gic_inject_irq_stop();
>>> @@ -840,14 +859,6 @@ static void maintenance_interrupt(int irq,
void *dev_id, struct cpu_user_regs *r
>>>          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;
>>> -            /* 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);
>>>  
>

On Tue, 25 Jun 2013, Julien Grall wrote:
> On 06/25/2013 05:28 PM, Ian Campbell wrote:
> 
> > On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
> >> If the guest VCPU receives an interrupts when it''s
disabled, it will throw
> >> away the IRQ with EOIed it.
> > 
> > Did you mean "without EOIing it" or perhaps "having
EOIed it"?
> > 
> >>  This is result to lost IRQ forever.
> > 
> > "This results in losing the IRQ forever".
> > 
> >> Directly EOIed the interrupt doesn''t help because the IRQ
could be fired
> > 
> > EOIing in this context.
> > 
> >> again and result to an infinited loop.
> > 
> >                         infinite
> > 
> >> It happens during dom0 boot on the versatile express TC2 with the
ethernet
> >> card.
> >>
> >> Let the interrupt disabled when Xen setups the route and enable it
when Linux
> > 
> >   "Lets ... when Xen sets up the route..."
> > 
> >> asks to enable it.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> >> ---
> >>  xen/arch/arm/domain_build.c |   14 ++++++++++++++
> >>  xen/arch/arm/gic.c          |   25 +++++++++++++++++++++++--
> >>  xen/arch/arm/vgic.c         |    7 +++----
> >>  xen/include/asm-arm/gic.h   |    4 ++++
> >>  xen/include/asm-arm/irq.h   |    6 ++++++
> >>  5 files changed, 50 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/domain_build.c
b/xen/arch/arm/domain_build.c
> >> index f5befbd..0470a2d 100644
> >> --- a/xen/arch/arm/domain_build.c
> >> +++ b/xen/arch/arm/domain_build.c
> >> @@ -408,6 +408,20 @@ static int map_device(struct domain *d, const
struct dt_device_node *dev)
> >>          }
> >>  
> >>          DPRINT("irq %u = %u type = 0x%x\n", i, irq.irq,
irq.type);
> >> +
> >> +        /*
> >> +         * Only map SGI interrupt in the guest as XEN
won''t handle
> >> +         * it correctly.
> >> +         * TODO: Fix it
> >> +         */
> >> +        if ( !irq_is_sgi(irq.irq) )
> >> +        {
> >> +            printk(XENLOG_WARNING "WARNING: Don''t
map irq %u for %s has "
> > 
> > s/has/as/ I think.
> > 
> > Did you really mean SGI here? I''d have thought from the
context that you
> > would mean SPIs. SGIs aren''t anything to do with any real
devices almost
> > by definition -- if you saw on in the device tree I''d be very
surprised!
> 
> 
> It was SPIs. I will fix it in the next patch series.
> 
> > 
> >> +                   "XEN doesn''t handle properly
non-SGI interrupt\n",
> >> +                   i, dt_node_full_name(dev));
> >> +            continue;
> >> +        }
> >> +
> >>          /* Don''t check return because the IRQ can be use
by multiple device */
> >>          gic_route_irq_to_guest(d, &irq, dt_node_name(dev));
> >>      }
> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >> index b16ba8c..e7d082a 100644
> >> --- a/xen/arch/arm/gic.c
> >> +++ b/xen/arch/arm/gic.c
> >> @@ -179,6 +179,17 @@ static hw_irq_controller gic_guest_irq_type =
{
> >>      .set_affinity = gic_irq_set_affinity,
> >>  };
> >>  
> >> +void gic_irq_enable(struct irq_desc *desc)
> >> +{
> >> +    spin_lock_irq(&desc->lock);
> >> +    spin_lock(&gic.lock);
> >> +
> >> +    desc->handler->enable(desc);
> >> +
> >> +    spin_unlock(&gic.lock);
> >> +    spin_unlock_irq(&desc->lock);
> >> +}
> >> +
> >>  /* needs to be called with gic.lock held */
> >>  static void gic_set_irq_properties(unsigned int irq, bool_t
level,
> >>          unsigned int cpu_mask, unsigned int priority)
> >> @@ -543,8 +554,6 @@ static int __setup_irq(struct irq_desc *desc,
unsigned int irq,
> >>      desc->status &= ~IRQ_DISABLED;
> >>      dsb();
> >>  
> >> -    desc->handler->startup(desc);
> >> -
> >>      return 0;
> >>  }
> >>  
> >> @@ -560,6 +569,8 @@ int __init setup_dt_irq(const struct dt_irq
*irq, struct irqaction *new)
> >>  
> >>      rc = __setup_irq(desc, irq->irq, new);
> >>  
> >> +    desc->handler->startup(desc);
> >> +
> >>      spin_unlock_irqrestore(&desc->lock, flags);
> >>  
> >>      return rc;
> >> @@ -711,6 +722,7 @@ void gic_inject(void)
> >>          gic_inject_irq_start();
> >>  }
> >>  
> >> +/* TODO: Handle properly non SGI-interrupt */
> >>  int gic_route_irq_to_guest(struct domain *d, const struct dt_irq
*irq,
> >>                             const char * devname)
> >>  {
> >> @@ -719,11 +731,18 @@ 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;
> >> +    /* XXX: handler other VCPU than 0 */
> > 
> > That should be something like "XXX: handle VCPUs other than
0".
> > 
> > This only matters if we can route SGIs or PPIs to the guest though I
> > think, since they are the only banked interrupts? For SPIs we actually
> > want to actively avoid doing this multiple times, don''t we?
> 
> 
> Yes. Here the VCPU is only used to retrieved the struct pending_irq.
> 
> > 
> > For the banked interrupts I think we just need a loop here, or for
> > p->desc to not be part of the pending_irq struct but actually part
of
> > some separate per-domain datastructure, since it would be very weird
to
> > have a domain where the PPIs differed between CPUs. (I''m not
sure if
> > that is allowed by the hardware, I bet it is, but it would be a
> > pathological case IMHO...).
> 
> > I think a perdomain irq_desc * array is probably the right answer,
> > unless someone can convincingly argue that PPI routing differing
between
> > VCPUs in a guest is a useful thing...
> 
> 
> Until now, I didn''t see PPIs on other devices than the arch timers
and
> the GIC. I don''t know if it''s possible, but pending_irq
are also banked
> for PPIs, so it''s not an issue.
> 
> The issue is how do we link the physical PPI to the virtual PPI? Is a
> 1:1 mapping. How does Xen handle PPI when a it is coming on VCPUs which
> doesn''t handle it (for instance a domU)?
We should be already able to handle this case: vgic_vcpu_inject_irq uses
smp_send_event_check_mask to make sure the other physical processor
receives the notification and injects the virq.

On Tue, 25 Jun 2013, Julien Grall wrote:
> On 06/25/2013 05:58 PM, Stefano Stabellini wrote:
> 
> > On Tue, 25 Jun 2013, Ian Campbell wrote:
> >> On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
> >>> From: Stefano Stabellini <stefano.stabellini@citrix.com>
> >>>
> >>> For guest''s timers (both virtual and physical), Xen
will inject virtual
> >>> interrupt. Linux handles timer interrupt as:
> >>
> >> We should be wary of developing things based on the way which
Linux
> >> happens to do things. On x86 we have several time modes, which can
be
> >> selected based upon guest behaviour (described in
> >> docs/man/xl.cfg.pod.5). Do we need to do something similar here?
> >>
> >>>   1) Receive the interrupt and ack it
> >>>   2) Handle the current event timer
> >>>   3) Set the next event timer
> >>>   4) EOI the interrupt
> >>>
> >>> It''s unlikely possible to reinject another interrupt
before
> >>
> >> I can''t parse this sentence. "unlikely to be
possible" perhaps? but I''m
> >> not sure if that is what you meant.
> >>
> >>> the previous one is EOIed because the next deadline is shorter
than the time
> >>> to execute code until EOI it.
> >>
> >> If we get into this situation once is there any danger that we
will get
> >> into it repeatedly and overflow the count?
> >>
> >> Overall I''m not convinced this is the right approach to
get the
> >> behaviour we want. Isn''t this interrupt level triggered,
with the level
> >> being determined by a comparison of two registers? IOW
can''t we
> >> determine whether to retrigger the interrupt or not by examining
the
> >> state of our emulated versions of those registers? A generic
mechanism
> >> to callback into the appropriate emulator on EOI plus a little bit
of
> >> logic in the vtimer code is all it ought to take.
> > 
> > AFAICT this what could happen:
> > 
> > - vtimer fires
> > - xen mask the vtimer
> > - xen adds the vtimer to the LR for the guest
> > - xen EOIs the vtimer
> > - the guest receive the vtimer interrupt
> > - the guest set the next deadline in the past
> > - the guest enables the vtimer
> > ## an unexpected vtimer interrupt is received by Xen but the current
> > ## one is still being serviced 
> > - the guest eoi the vtimer
> > 
> > as a result the guest looses an interrupt.
> > Julien, is that correct?
> 
> Yes.
Could you please add something like that to the commit message?

> > If that is the case, this can only happen once, right?  In that case
> > rather than an atomic_t we could just have a bit in the status field I
> > proposed before. It should be enough for us to keep track of the case
> > when the irq is supposed to stay high even after the guest EOIs it.
(Of
> > course that means that we need to re-inject it into the guest).
> 
> 
> For the timer yes. I wonder, what happen if Xen receive an SGI (for
> instance SCHEDULE ipi) twice, does Xen must re-inject it multiple times?
I don''t think it can happen with anything but the vtimer: in order for
the scenario above to happen it takes an irq that is EOId in the
hardware before the guest EOIs it.

SGIs are completely "emulated", there is not an hardware irq
corresponding to them. From the Xen point of view the SGI is inflight
exactly and only from the moment the first vcpu sends it, to the point
when the receiving vcpu EOIs it.

> >>> Signed-off-by: Stefano Stabellini
<stefano.stabellini@citrix.com>
> >>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> >>> ---
> >>>  xen/arch/arm/gic.c           |   35
+++++++++++++++++++++++------------
> >>>  xen/arch/arm/vgic.c          |    1 +
> >>>  xen/include/asm-arm/domain.h |    2 ++
> >>>  3 files changed, 26 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >>> index 0fee3f2..21575df 100644
> >>> --- a/xen/arch/arm/gic.c
> >>> +++ b/xen/arch/arm/gic.c
> >>> @@ -817,7 +817,7 @@ 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;
> >>> +        struct pending_irq *p, *n;
> >>>          int cpu, eoi;
> >>>  
> >>>          cpu = -1;
> >>> @@ -826,13 +826,32 @@ static void maintenance_interrupt(int
irq, void *dev_id, struct cpu_user_regs *r
> >>>          spin_lock_irq(&gic.lock);
> >>>          lr = GICH[GICH_LR + i];
> >>>          virq = lr & GICH_LR_VIRTUAL_MASK;
> >>> +
> >>> +        p = irq_to_pending(v, virq);
> >>> +        if ( p->desc != NULL ) {
> >>> +            p->desc->status &= ~IRQ_INPROGRESS;
> > 
> > Now that I am thinking about this, shouldn''t this be
protected by taking
> > the desc->lock?
> 
> Right. I don''t think desc->lock is enough if we want to protect
from
> release_irq.
> If this function is called, it will wait until IRQ_INPROGRESS is
> removed. How about moving ~IRQ_INPROGRESS at then end of the block and
> add a dmb() before?
that should work

On Tue, 2013-06-25 at 18:05 +0100, Stefano Stabellini
wrote:
> On Tue, 25 Jun 2013, Ian Campbell wrote:
> > On Tue, 2013-06-25 at 17:36 +0100, Stefano Stabellini wrote:
> > > I think it''s time we introduce a "status"
member in struct irq_desc, so
> > > that we are not dependent on the information in the GICH_LR
registers or
> > > the queue a pending_irq has been added to.
> > 
> > Yes please, I find this one of the hardest things to keep straight in
my
> > head (not helped by my inability to remember which of pending and
> > inflight is which...)
> > 
> > > I would implement it as a bitfield:
> > > 
> > > int status;
> > > #define GIC_IRQ_ENABLED  (1<<0)
> > > #define GIC_IRQ_INFLIGHT (1<<1)
> > > #define GIC_IRQ_INLR     (1<<2)
> > > 
> > > This way you should just go through the inflight queue and check
whether
> > > status & GIC_IRQ_INLR.
> > 
> > Since some of this stuff happens in interrupt context you probably
want
> > test_bit/set_bit et al rather than regular boolean logic,
don''t you?
> > 
> > > At the moment we just want to represent this basic state machine:
> > > 
> > > irq guest asserted -> inflight -> injected (inlr) ->
unasserted (maintenance interrupt)
> > 
> > Can we model the states after the active/pending states which the gic
> > has? It might make a bunch of stuff clearer?
> 
> It might be worth storing those too.
> So maybe:
> 
> #define GIC_IRQ_ENABLED           (1<<0)
> #define GIC_IRQ_PENDING           (1<<1)
> #define GIC_IRQ_ACTIVE            (1<<2)
> #define GIC_IRQ_GUEST_INFLIGHT    (1<<3)
> #define GIC_IRQ_GUEST_INLR        (1<<4)
> 
> however if we do store the physical gic states (GIC_IRQ_PENDING and
> GIC_IRQ_ACTIVE) then maybe the right place for them is actually irq_desc
> rather than irq_pending that is used just for guest irqs.
I was thinking of these as states of the emulated interrupts, rather
than any underlying physical interrupts, so I think irq_pending is
correct?

It occurs to me that at least some of these bits are also fields in the
LR. I think it is good to save them separately (reducing the
intertwining of our interrupt handling from GIC internals is a good
thing) but it means you will need to take care of syncing the state
between the LR and our internal state at various points, since the LR
can change based on the guest EOI etc.

Ian.

On Tue, 2013-06-25 at 18:38 +0100, Julien Grall wrote:
> >> @@ -719,11 +731,18 @@ 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;
> >> +    /* XXX: handler other VCPU than 0 */
> > 
> > That should be something like "XXX: handle VCPUs other than
0".
> > 
> > This only matters if we can route SGIs or PPIs to the guest though I
> > think, since they are the only banked interrupts? For SPIs we actually
> > want to actively avoid doing this multiple times, don''t we?
> 
> 
> Yes. Here the VCPU is only used to retrieved the struct pending_irq.
Which is per-CPU for PPIs and SGIs. Do we not care about PPIs here?
> > 
> > For the banked interrupts I think we just need a loop here, or for
> > p->desc to not be part of the pending_irq struct but actually part
of
> > some separate per-domain datastructure, since it would be very weird
to
> > have a domain where the PPIs differed between CPUs. (I''m not
sure if
> > that is allowed by the hardware, I bet it is, but it would be a
> > pathological case IMHO...).
> 
> > I think a perdomain irq_desc * array is probably the right answer,
> > unless someone can convincingly argue that PPI routing differing
between
> > VCPUs in a guest is a useful thing...
> 
> 
> Until now, I didn''t see PPIs on other devices than the arch timers
and
> the GIC. I don''t know if it''s possible, but pending_irq
are also banked
> for PPIs, so it''s not an issue.
> 
> The issue is how do we link the physical PPI to the virtual PPI? Is a
> 1:1 mapping. How does Xen handle PPI when a it is coming on VCPUs which
> doesn''t handle it (for instance a domU)?
How do you mean?

Ian.

On Tue, 2013-06-25 at 17:58 +0100, Stefano Stabellini
wrote:
> On Tue, 25 Jun 2013, Ian Campbell wrote:
> > On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
> > > From: Stefano Stabellini <stefano.stabellini@citrix.com>
> > > 
> > > For guest''s timers (both virtual and physical), Xen will
inject virtual
> > > interrupt. Linux handles timer interrupt as:
> > 
> > We should be wary of developing things based on the way which Linux
> > happens to do things. On x86 we have several time modes, which can be
> > selected based upon guest behaviour (described in
> > docs/man/xl.cfg.pod.5). Do we need to do something similar here?
> > 
> > >   1) Receive the interrupt and ack it
> > >   2) Handle the current event timer
> > >   3) Set the next event timer
> > >   4) EOI the interrupt
> > > 
> > > It''s unlikely possible to reinject another interrupt
before
> > 
> > I can''t parse this sentence. "unlikely to be
possible" perhaps? but I''m
> > not sure if that is what you meant.
> > 
> > > the previous one is EOIed because the next deadline is shorter
than the time
> > > to execute code until EOI it.
> > 
> > If we get into this situation once is there any danger that we will
get
> > into it repeatedly and overflow the count?
> > 
> > Overall I''m not convinced this is the right approach to get
the
> > behaviour we want. Isn''t this interrupt level triggered, with
the level
> > being determined by a comparison of two registers? IOW can''t
we
> > determine whether to retrigger the interrupt or not by examining the
> > state of our emulated versions of those registers? A generic mechanism
> > to callback into the appropriate emulator on EOI plus a little bit of
> > logic in the vtimer code is all it ought to take.
> 
> AFAICT this what could happen:
> 
> - vtimer fires
> - xen mask the vtimer
> - xen adds the vtimer to the LR for the guest
> - xen EOIs the vtimer
This step is Xen deprioritises the interrupt (by writing to the GICD_DIR
register)...
> - the guest receive the vtimer interrupt
> - the guest set the next deadline in the past
> - the guest enables the vtimer
> ## an unexpected vtimer interrupt is received by Xen but the current
> ## one is still being serviced 
> - the guest eoi the vtimer
... and the actual Xen EOI only happens here in response to the
maintenance interrupt resulting from the guests EOI.

(or maybe I''ve misunderstood what you mean by "EOI the
vtimer")

Ian.

On Tue, 2013-06-25 at 19:38 +0100, Stefano Stabellini
wrote:
> > > If that is the case, this can only happen once, right?  In that
case
> > > rather than an atomic_t we could just have a bit in the status
field I
> > > proposed before. It should be enough for us to keep track of the
case
> > > when the irq is supposed to stay high even after the guest EOIs
it. (Of
> > > course that means that we need to re-inject it into the guest).
> > 
> > 
> > For the timer yes. I wonder, what happen if Xen receive an SGI (for
> > instance SCHEDULE ipi) twice, does Xen must re-inject it multiple
times?
> 
> I don''t think it can happen with anything but the vtimer: in order
for
> the scenario above to happen it takes an irq that is EOId in the
> hardware before the guest EOIs it.
> 
> SGIs are completely "emulated", there is not an hardware irq
> corresponding to them. From the Xen point of view the SGI is inflight
> exactly and only from the moment the first vcpu sends it, to the point
> when the receiving vcpu EOIs it.
Are we talking about real SGIs (passed by Xen between physical
processors) or fake SGIs (emulated between virtual processors)?

Ian.

On Wed, 26 Jun 2013, Ian Campbell wrote:
> On Tue, 2013-06-25 at 17:58 +0100, Stefano Stabellini wrote:
> > On Tue, 25 Jun 2013, Ian Campbell wrote:
> > > On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
> > > > From: Stefano Stabellini
<stefano.stabellini@citrix.com>
> > > > 
> > > > For guest''s timers (both virtual and physical), Xen
will inject virtual
> > > > interrupt. Linux handles timer interrupt as:
> > > 
> > > We should be wary of developing things based on the way which
Linux
> > > happens to do things. On x86 we have several time modes, which
can be
> > > selected based upon guest behaviour (described in
> > > docs/man/xl.cfg.pod.5). Do we need to do something similar here?
> > > 
> > > >   1) Receive the interrupt and ack it
> > > >   2) Handle the current event timer
> > > >   3) Set the next event timer
> > > >   4) EOI the interrupt
> > > > 
> > > > It''s unlikely possible to reinject another
interrupt before
> > > 
> > > I can''t parse this sentence. "unlikely to be
possible" perhaps? but I''m
> > > not sure if that is what you meant.
> > > 
> > > > the previous one is EOIed because the next deadline is
shorter than the time
> > > > to execute code until EOI it.
> > > 
> > > If we get into this situation once is there any danger that we
will get
> > > into it repeatedly and overflow the count?
> > > 
> > > Overall I''m not convinced this is the right approach to
get the
> > > behaviour we want. Isn''t this interrupt level triggered,
with the level
> > > being determined by a comparison of two registers? IOW
can''t we
> > > determine whether to retrigger the interrupt or not by examining
the
> > > state of our emulated versions of those registers? A generic
mechanism
> > > to callback into the appropriate emulator on EOI plus a little
bit of
> > > logic in the vtimer code is all it ought to take.
> > 
> > AFAICT this what could happen:
> > 
> > - vtimer fires
> > - xen mask the vtimer
> > - xen adds the vtimer to the LR for the guest
> > - xen EOIs the vtimer
> 
> This step is Xen deprioritises the interrupt (by writing to the GICD_DIR
> register)...
> 
> > - the guest receive the vtimer interrupt
> > - the guest set the next deadline in the past
> > - the guest enables the vtimer
> > ## an unexpected vtimer interrupt is received by Xen but the current
> > ## one is still being serviced 
> > - the guest eoi the vtimer
> 
> ... and the actual Xen EOI only happens here in response to the
> maintenance interrupt resulting from the guests EOI.
> 
> (or maybe I''ve misunderstood what you mean by "EOI the
vtimer")
The vtimer is a Xen irq that injects an irq into the guest from the Xen
interrupt handler. It''s not a guest irq.
See xen/arch/arm/time.c:vtimer_interrupt.

On Wed, 26 Jun 2013, Ian Campbell wrote:
> On Tue, 2013-06-25 at 19:38 +0100, Stefano Stabellini wrote:
> > > > If that is the case, this can only happen once, right?  In
that case
> > > > rather than an atomic_t we could just have a bit in the
status field I
> > > > proposed before. It should be enough for us to keep track of
the case
> > > > when the irq is supposed to stay high even after the guest
EOIs it. (Of
> > > > course that means that we need to re-inject it into the
guest).
> > > 
> > > 
> > > For the timer yes. I wonder, what happen if Xen receive an SGI
(for
> > > instance SCHEDULE ipi) twice, does Xen must re-inject it multiple
times?
> > 
> > I don''t think it can happen with anything but the vtimer: in
order for
> > the scenario above to happen it takes an irq that is EOId in the
> > hardware before the guest EOIs it.
> > 
> > SGIs are completely "emulated", there is not an hardware irq
> > corresponding to them. From the Xen point of view the SGI is inflight
> > exactly and only from the moment the first vcpu sends it, to the point
> > when the receiving vcpu EOIs it.
> 
> Are we talking about real SGIs (passed by Xen between physical
> processors) or fake SGIs (emulated between virtual processors)?
I was talking about fake SGIs

On Wed, 2013-06-26 at 12:08 +0100, Stefano Stabellini
wrote:
> On Wed, 26 Jun 2013, Ian Campbell wrote:
> > On Tue, 2013-06-25 at 17:58 +0100, Stefano Stabellini wrote:
> > > On Tue, 25 Jun 2013, Ian Campbell wrote:
> > > > On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
> > > > > From: Stefano Stabellini
<stefano.stabellini@citrix.com>
> > > > > 
> > > > > For guest''s timers (both virtual and
physical), Xen will inject virtual
> > > > > interrupt. Linux handles timer interrupt as:
> > > > 
> > > > We should be wary of developing things based on the way
which Linux
> > > > happens to do things. On x86 we have several time modes,
which can be
> > > > selected based upon guest behaviour (described in
> > > > docs/man/xl.cfg.pod.5). Do we need to do something similar
here?
> > > > 
> > > > >   1) Receive the interrupt and ack it
> > > > >   2) Handle the current event timer
> > > > >   3) Set the next event timer
> > > > >   4) EOI the interrupt
> > > > > 
> > > > > It''s unlikely possible to reinject another
interrupt before
> > > > 
> > > > I can''t parse this sentence. "unlikely to be
possible" perhaps? but I''m
> > > > not sure if that is what you meant.
> > > > 
> > > > > the previous one is EOIed because the next deadline is
shorter than the time
> > > > > to execute code until EOI it.
> > > > 
> > > > If we get into this situation once is there any danger that
we will get
> > > > into it repeatedly and overflow the count?
> > > > 
> > > > Overall I''m not convinced this is the right
approach to get the
> > > > behaviour we want. Isn''t this interrupt level
triggered, with the level
> > > > being determined by a comparison of two registers? IOW
can''t we
> > > > determine whether to retrigger the interrupt or not by
examining the
> > > > state of our emulated versions of those registers? A generic
mechanism
> > > > to callback into the appropriate emulator on EOI plus a
little bit of
> > > > logic in the vtimer code is all it ought to take.
> > > 
> > > AFAICT this what could happen:
> > > 
> > > - vtimer fires
> > > - xen mask the vtimer
> > > - xen adds the vtimer to the LR for the guest
> > > - xen EOIs the vtimer
> > 
> > This step is Xen deprioritises the interrupt (by writing to the
GICD_DIR
> > register)...
> > 
> > > - the guest receive the vtimer interrupt
> > > - the guest set the next deadline in the past
> > > - the guest enables the vtimer
> > > ## an unexpected vtimer interrupt is received by Xen but the
current
> > > ## one is still being serviced 
> > > - the guest eoi the vtimer
> > 
> > ... and the actual Xen EOI only happens here in response to the
> > maintenance interrupt resulting from the guests EOI.
> > 
> > (or maybe I''ve misunderstood what you mean by "EOI the
vtimer")
> 
> The vtimer is a Xen irq that injects an irq into the guest from the Xen
> interrupt handler. It''s not a guest irq.
> See xen/arch/arm/time.c:vtimer_interrupt.
OK, so what does it mean to "EOI" that timer?

Ian.

On Wed, 2013-06-26 at 12:10 +0100, Stefano Stabellini
wrote:
> On Wed, 26 Jun 2013, Ian Campbell wrote:
> > On Tue, 2013-06-25 at 19:38 +0100, Stefano Stabellini wrote:
> > > > > If that is the case, this can only happen once, right? 
In that case
> > > > > rather than an atomic_t we could just have a bit in the
status field I
> > > > > proposed before. It should be enough for us to keep
track of the case
> > > > > when the irq is supposed to stay high even after the
guest EOIs it. (Of
> > > > > course that means that we need to re-inject it into the
guest).
> > > > 
> > > > 
> > > > For the timer yes. I wonder, what happen if Xen receive an
SGI (for
> > > > instance SCHEDULE ipi) twice, does Xen must re-inject it
multiple times?
> > > 
> > > I don''t think it can happen with anything but the
vtimer: in order for
> > > the scenario above to happen it takes an irq that is EOId in the
> > > hardware before the guest EOIs it.
> > > 
> > > SGIs are completely "emulated", there is not an
hardware irq
> > > corresponding to them. From the Xen point of view the SGI is
inflight
> > > exactly and only from the moment the first vcpu sends it, to the
point
> > > when the receiving vcpu EOIs it.
> > 
> > Are we talking about real SGIs (passed by Xen between physical
> > processors) or fake SGIs (emulated between virtual processors)?
> 
> I was talking about fake SGIs
I think Julien was talking about real ones (the Xen schedule IPI).

Ian.

On Wed, 26 Jun 2013, Ian Campbell wrote:
> On Tue, 2013-06-25 at 18:05 +0100, Stefano Stabellini wrote:
> > On Tue, 25 Jun 2013, Ian Campbell wrote:
> > > On Tue, 2013-06-25 at 17:36 +0100, Stefano Stabellini wrote:
> > > > I think it''s time we introduce a "status"
member in struct irq_desc, so
> > > > that we are not dependent on the information in the GICH_LR
registers or
> > > > the queue a pending_irq has been added to.
> > > 
> > > Yes please, I find this one of the hardest things to keep
straight in my
> > > head (not helped by my inability to remember which of pending and
> > > inflight is which...)
> > > 
> > > > I would implement it as a bitfield:
> > > > 
> > > > int status;
> > > > #define GIC_IRQ_ENABLED  (1<<0)
> > > > #define GIC_IRQ_INFLIGHT (1<<1)
> > > > #define GIC_IRQ_INLR     (1<<2)
> > > > 
> > > > This way you should just go through the inflight queue and
check whether
> > > > status & GIC_IRQ_INLR.
> > > 
> > > Since some of this stuff happens in interrupt context you
probably want
> > > test_bit/set_bit et al rather than regular boolean logic,
don''t you?
> > > 
> > > > At the moment we just want to represent this basic state
machine:
> > > > 
> > > > irq guest asserted -> inflight -> injected (inlr)
-> unasserted (maintenance interrupt)
> > > 
> > > Can we model the states after the active/pending states which the
gic
> > > has? It might make a bunch of stuff clearer?
> > 
> > It might be worth storing those too.
> > So maybe:
> > 
> > #define GIC_IRQ_ENABLED           (1<<0)
> > #define GIC_IRQ_PENDING           (1<<1)
> > #define GIC_IRQ_ACTIVE            (1<<2)
> > #define GIC_IRQ_GUEST_INFLIGHT    (1<<3)
> > #define GIC_IRQ_GUEST_INLR        (1<<4)
> > 
> > however if we do store the physical gic states (GIC_IRQ_PENDING and
> > GIC_IRQ_ACTIVE) then maybe the right place for them is actually
irq_desc
> > rather than irq_pending that is used just for guest irqs.
> 
> I was thinking of these as states of the emulated interrupts, rather
> than any underlying physical interrupts, so I think irq_pending is
> correct?
In that case yes

> It occurs to me that at least some of these bits are also fields in the
> LR. I think it is good to save them separately (reducing the
> intertwining of our interrupt handling from GIC internals is a good
> thing) but it means you will need to take care of syncing the state
> between the LR and our internal state at various points, since the LR
> can change based on the guest EOI etc.
I don''t think we can accurately distinguish between pending and active
states for guest irqs, because the transition between the two happens
transparently from Xen''s point of view.
The only thing we can do is update the state in irq_pending when we save
and restore the GICH_LR registers.
That might still be useful because it would allow us to know which ones
of the irqs currently in the LRs registers can be temporarily set aside
(for example to implement priorities).

On Wed, 26 Jun 2013, Ian Campbell wrote:
> On Wed, 2013-06-26 at 12:08 +0100, Stefano Stabellini wrote:
> > On Wed, 26 Jun 2013, Ian Campbell wrote:
> > > On Tue, 2013-06-25 at 17:58 +0100, Stefano Stabellini wrote:
> > > > On Tue, 25 Jun 2013, Ian Campbell wrote:
> > > > > On Tue, 2013-06-25 at 00:04 +0100, Julien Grall wrote:
> > > > > > From: Stefano Stabellini
<stefano.stabellini@citrix.com>
> > > > > > 
> > > > > > For guest''s timers (both virtual and
physical), Xen will inject virtual
> > > > > > interrupt. Linux handles timer interrupt as:
> > > > > 
> > > > > We should be wary of developing things based on the way
which Linux
> > > > > happens to do things. On x86 we have several time
modes, which can be
> > > > > selected based upon guest behaviour (described in
> > > > > docs/man/xl.cfg.pod.5). Do we need to do something
similar here?
> > > > > 
> > > > > >   1) Receive the interrupt and ack it
> > > > > >   2) Handle the current event timer
> > > > > >   3) Set the next event timer
> > > > > >   4) EOI the interrupt
> > > > > > 
> > > > > > It''s unlikely possible to reinject
another interrupt before
> > > > > 
> > > > > I can''t parse this sentence. "unlikely to
be possible" perhaps? but I''m
> > > > > not sure if that is what you meant.
> > > > > 
> > > > > > the previous one is EOIed because the next
deadline is shorter than the time
> > > > > > to execute code until EOI it.
> > > > > 
> > > > > If we get into this situation once is there any danger
that we will get
> > > > > into it repeatedly and overflow the count?
> > > > > 
> > > > > Overall I''m not convinced this is the right
approach to get the
> > > > > behaviour we want. Isn''t this interrupt level
triggered, with the level
> > > > > being determined by a comparison of two registers? IOW
can''t we
> > > > > determine whether to retrigger the interrupt or not by
examining the
> > > > > state of our emulated versions of those registers? A
generic mechanism
> > > > > to callback into the appropriate emulator on EOI plus a
little bit of
> > > > > logic in the vtimer code is all it ought to take.
> > > > 
> > > > AFAICT this what could happen:
> > > > 
> > > > - vtimer fires
> > > > - xen mask the vtimer
> > > > - xen adds the vtimer to the LR for the guest
> > > > - xen EOIs the vtimer
> > > 
> > > This step is Xen deprioritises the interrupt (by writing to the
GICD_DIR
> > > register)...
> > > 
> > > > - the guest receive the vtimer interrupt
> > > > - the guest set the next deadline in the past
> > > > - the guest enables the vtimer
> > > > ## an unexpected vtimer interrupt is received by Xen but the
current
> > > > ## one is still being serviced 
> > > > - the guest eoi the vtimer
> > > 
> > > ... and the actual Xen EOI only happens here in response to the
> > > maintenance interrupt resulting from the guests EOI.
> > > 
> > > (or maybe I''ve misunderstood what you mean by "EOI
the vtimer")
> > 
> > The vtimer is a Xen irq that injects an irq into the guest from the
Xen
> > interrupt handler. It''s not a guest irq.
> > See xen/arch/arm/time.c:vtimer_interrupt.
> 
> OK, so what does it mean to "EOI" that timer?
 
By "xen EOIs the vtimer" I meant Xen EOIs the vtimer interrupt in the
real hardware.
By "the guest eoi the vtimer" I meant the guest EOIs the virtual
interrupt that we injected into the guest.

On Wed, 2013-06-26 at 12:23 +0100, Stefano Stabellini
wrote:
> On Wed, 26 Jun 2013, Ian Campbell wrote:
> > On Wed, 2013-06-26 at 12:08 +0100, Stefano Stabellini wrote:
> > > On Wed, 26 Jun 2013, Ian Campbell wrote:
> > > > On Tue, 2013-06-25 at 17:58 +0100, Stefano Stabellini wrote:
> > > > > On Tue, 25 Jun 2013, Ian Campbell wrote:
> > > > > > On Tue, 2013-06-25 at 00:04 +0100, Julien Grall
wrote:
> > > > > > > From: Stefano Stabellini
<stefano.stabellini@citrix.com>
> > > > > > > 
> > > > > > > For guest''s timers (both virtual and
physical), Xen will inject virtual
> > > > > > > interrupt. Linux handles timer interrupt as:
> > > > > > 
> > > > > > We should be wary of developing things based on
the way which Linux
> > > > > > happens to do things. On x86 we have several time
modes, which can be
> > > > > > selected based upon guest behaviour (described in
> > > > > > docs/man/xl.cfg.pod.5). Do we need to do something
similar here?
> > > > > > 
> > > > > > >   1) Receive the interrupt and ack it
> > > > > > >   2) Handle the current event timer
> > > > > > >   3) Set the next event timer
> > > > > > >   4) EOI the interrupt
> > > > > > > 
> > > > > > > It''s unlikely possible to reinject
another interrupt before
> > > > > > 
> > > > > > I can''t parse this sentence.
"unlikely to be possible" perhaps? but I''m
> > > > > > not sure if that is what you meant.
> > > > > > 
> > > > > > > the previous one is EOIed because the next
deadline is shorter than the time
> > > > > > > to execute code until EOI it.
> > > > > > 
> > > > > > If we get into this situation once is there any
danger that we will get
> > > > > > into it repeatedly and overflow the count?
> > > > > > 
> > > > > > Overall I''m not convinced this is the
right approach to get the
> > > > > > behaviour we want. Isn''t this interrupt
level triggered, with the level
> > > > > > being determined by a comparison of two registers?
IOW can''t we
> > > > > > determine whether to retrigger the interrupt or
not by examining the
> > > > > > state of our emulated versions of those registers?
A generic mechanism
> > > > > > to callback into the appropriate emulator on EOI
plus a little bit of
> > > > > > logic in the vtimer code is all it ought to take.
> > > > > 
> > > > > AFAICT this what could happen:
> > > > > 
> > > > > - vtimer fires
> > > > > - xen mask the vtimer
> > > > > - xen adds the vtimer to the LR for the guest
> > > > > - xen EOIs the vtimer
> > > > 
> > > > This step is Xen deprioritises the interrupt (by writing to
the GICD_DIR
> > > > register)...
> > > > 
> > > > > - the guest receive the vtimer interrupt
> > > > > - the guest set the next deadline in the past
> > > > > - the guest enables the vtimer
> > > > > ## an unexpected vtimer interrupt is received by Xen
but the current
> > > > > ## one is still being serviced 
> > > > > - the guest eoi the vtimer
> > > > 
> > > > ... and the actual Xen EOI only happens here in response to
the
> > > > maintenance interrupt resulting from the guests EOI.
> > > > 
> > > > (or maybe I''ve misunderstood what you mean by
"EOI the vtimer")
> > > 
> > > The vtimer is a Xen irq that injects an irq into the guest from
the Xen
> > > interrupt handler. It''s not a guest irq.
> > > See xen/arch/arm/time.c:vtimer_interrupt.
> > 
> > OK, so what does it mean to "EOI" that timer?
>  
> By "xen EOIs the vtimer" I meant Xen EOIs the vtimer interrupt in
the
> real hardware.
Oh right, for some reason I thought this was driven off Xen timer
infrastructure, rather than being an actual interrupt.

In that case my comments about deprioritisation and actual EOI happening
later are correct, aren''t they?
> By "the guest eoi the vtimer" I meant the guest EOIs the virtual
> interrupt that we injected into the guest.

On Wed, 26 Jun 2013, Ian Campbell wrote:
> On Wed, 2013-06-26 at 12:23 +0100, Stefano Stabellini wrote:
> > On Wed, 26 Jun 2013, Ian Campbell wrote:
> > > On Wed, 2013-06-26 at 12:08 +0100, Stefano Stabellini wrote:
> > > > On Wed, 26 Jun 2013, Ian Campbell wrote:
> > > > > On Tue, 2013-06-25 at 17:58 +0100, Stefano Stabellini
wrote:
> > > > > > On Tue, 25 Jun 2013, Ian Campbell wrote:
> > > > > > > On Tue, 2013-06-25 at 00:04 +0100, Julien
Grall wrote:
> > > > > > > > From: Stefano Stabellini
<stefano.stabellini@citrix.com>
> > > > > > > > 
> > > > > > > > For guest''s timers (both
virtual and physical), Xen will inject virtual
> > > > > > > > interrupt. Linux handles timer interrupt
as:
> > > > > > > 
> > > > > > > We should be wary of developing things based
on the way which Linux
> > > > > > > happens to do things. On x86 we have several
time modes, which can be
> > > > > > > selected based upon guest behaviour
(described in
> > > > > > > docs/man/xl.cfg.pod.5). Do we need to do
something similar here?
> > > > > > > 
> > > > > > > >   1) Receive the interrupt and ack it
> > > > > > > >   2) Handle the current event timer
> > > > > > > >   3) Set the next event timer
> > > > > > > >   4) EOI the interrupt
> > > > > > > > 
> > > > > > > > It''s unlikely possible to
reinject another interrupt before
> > > > > > > 
> > > > > > > I can''t parse this sentence.
"unlikely to be possible" perhaps? but I''m
> > > > > > > not sure if that is what you meant.
> > > > > > > 
> > > > > > > > the previous one is EOIed because the
next deadline is shorter than the time
> > > > > > > > to execute code until EOI it.
> > > > > > > 
> > > > > > > If we get into this situation once is there
any danger that we will get
> > > > > > > into it repeatedly and overflow the count?
> > > > > > > 
> > > > > > > Overall I''m not convinced this is
the right approach to get the
> > > > > > > behaviour we want. Isn''t this
interrupt level triggered, with the level
> > > > > > > being determined by a comparison of two
registers? IOW can''t we
> > > > > > > determine whether to retrigger the interrupt
or not by examining the
> > > > > > > state of our emulated versions of those
registers? A generic mechanism
> > > > > > > to callback into the appropriate emulator on
EOI plus a little bit of
> > > > > > > logic in the vtimer code is all it ought to
take.
> > > > > > 
> > > > > > AFAICT this what could happen:
> > > > > > 
> > > > > > - vtimer fires
> > > > > > - xen mask the vtimer
> > > > > > - xen adds the vtimer to the LR for the guest
> > > > > > - xen EOIs the vtimer
> > > > > 
> > > > > This step is Xen deprioritises the interrupt (by
writing to the GICD_DIR
> > > > > register)...
> > > > > 
> > > > > > - the guest receive the vtimer interrupt
> > > > > > - the guest set the next deadline in the past
> > > > > > - the guest enables the vtimer
> > > > > > ## an unexpected vtimer interrupt is received by
Xen but the current
> > > > > > ## one is still being serviced 
> > > > > > - the guest eoi the vtimer
> > > > > 
> > > > > ... and the actual Xen EOI only happens here in
response to the
> > > > > maintenance interrupt resulting from the guests EOI.
> > > > > 
> > > > > (or maybe I''ve misunderstood what you mean by
"EOI the vtimer")
> > > > 
> > > > The vtimer is a Xen irq that injects an irq into the guest
from the Xen
> > > > interrupt handler. It''s not a guest irq.
> > > > See xen/arch/arm/time.c:vtimer_interrupt.
> > > 
> > > OK, so what does it mean to "EOI" that timer?
> >  
> > By "xen EOIs the vtimer" I meant Xen EOIs the vtimer
interrupt in the
> > real hardware.
> 
> Oh right, for some reason I thought this was driven off Xen timer
> infrastructure, rather than being an actual interrupt.
> 
> In that case my comments about deprioritisation and actual EOI happening
> later are correct, aren''t they?
No, they are not. This is the full sequence of events, including
deprioritization and EOI:

- the vtimer interrupt fires
- xen deprioritizes the vtimer interrupt
- xen masks the vtimer interrupt
- xen adds the vtimer interrupt to the LR for the guest
- xen EOIs the vtimer interrupt
- the guest receives the vtimer interrupt
- the guest deprioritizes the vtimer interrupt
- the guest set the next deadline in the past
- the guest enables the vtimer
## an unexpected vtimer interrupt is received by Xen but the current
## one is still being serviced 
- the guest EOIs the vtimer interrupt

On Wed, 2013-06-26 at 12:50 +0100, Stefano Stabellini
wrote:
> On Wed, 26 Jun 2013, Ian Campbell wrote:
> > In that case my comments about deprioritisation and actual EOI
happening
> > later are correct, aren''t they?
> 
> No, they are not. This is the full sequence of events, including
> deprioritization and EOI:
> 
> - the vtimer interrupt fires
> - xen deprioritizes the vtimer interrupt
> - xen masks the vtimer interrupt
> - xen adds the vtimer interrupt to the LR for the guest
> - xen EOIs the vtimer interrupt
> - the guest receives the vtimer interrupt
> - the guest deprioritizes the vtimer interrupt
> - the guest set the next deadline in the past
> - the guest enables the vtimer
> ## an unexpected vtimer interrupt is received by Xen but the current
> ## one is still being serviced 
> - the guest EOIs the vtimer interrupt
Is this particular to the handling of the vtimer interrupt? In the
normal case an interrupt which is injected into the guest isn''t EOIed
by
Xen until the maintenance interrupt, isn''t it? But the vtimer is
special
because it isn''t routed to the guest even though we do so via an
orthogonal mechanism?

Perhaps the solution here is a third type of interrupt alongside
"for-xen" and "for-a-specific-guest" which is
"for-current-guest"?

Ian.

On 06/26/2013 11:55 AM, Ian Campbell wrote:
> On Tue, 2013-06-25 at 18:38 +0100, Julien Grall wrote:
>>>> @@ -719,11 +731,18 @@ 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;
>>>> +    /* XXX: handler other VCPU than 0 */
>>>
>>> That should be something like "XXX: handle VCPUs other than
0".
>>>
>>> This only matters if we can route SGIs or PPIs to the guest though
I
>>> think, since they are the only banked interrupts? For SPIs we
actually
>>> want to actively avoid doing this multiple times, don''t
we?
>>
>>
>> Yes. Here the VCPU is only used to retrieved the struct pending_irq.
> 
> Which is per-CPU for PPIs and SGIs. Do we not care about PPIs here?
I don''t see reason to route physical PPIs. Is it possible to have a
device (other than the timer and the GIC) with PPIs?
>>>
>>> For the banked interrupts I think we just need a loop here, or for
>>> p->desc to not be part of the pending_irq struct but actually
part of
>>> some separate per-domain datastructure, since it would be very
weird to
>>> have a domain where the PPIs differed between CPUs. (I''m
not sure if
>>> that is allowed by the hardware, I bet it is, but it would be a
>>> pathological case IMHO...).
>>
>>> I think a perdomain irq_desc * array is probably the right answer,
>>> unless someone can convincingly argue that PPI routing differing
between
>>> VCPUs in a guest is a useful thing...
>>
>>
>> Until now, I didn''t see PPIs on other devices than the arch
timers and
>> the GIC. I don''t know if it''s possible, but
pending_irq are also banked
>> for PPIs, so it''s not an issue.
>>
>> The issue is how do we link the physical PPI to the virtual PPI? Is a
>> 1:1 mapping. How does Xen handle PPI when a it is coming on VCPUs which
>> doesn''t handle it (for instance a domU)?
> 
> How do you mean?

My sentence wasn''t clear.

As for the arch timer, which is using PPIs, routing theses interrupts
require some code to support the device in Xen, mainly to save/restore
the context when the VCPU is moved.

Can we assume that Xen will never route PPIs to a guest?

-- 
Julien

On Wed, 26 Jun 2013, Ian Campbell wrote:
> On Wed, 2013-06-26 at 12:50 +0100, Stefano Stabellini wrote:
> > On Wed, 26 Jun 2013, Ian Campbell wrote:
> 
> > > In that case my comments about deprioritisation and actual EOI
happening
> > > later are correct, aren''t they?
> > 
> > No, they are not. This is the full sequence of events, including
> > deprioritization and EOI:
> > 
> > - the vtimer interrupt fires
> > - xen deprioritizes the vtimer interrupt
> > - xen masks the vtimer interrupt
> > - xen adds the vtimer interrupt to the LR for the guest
> > - xen EOIs the vtimer interrupt
> > - the guest receives the vtimer interrupt
> > - the guest deprioritizes the vtimer interrupt
> > - the guest set the next deadline in the past
> > - the guest enables the vtimer
> > ## an unexpected vtimer interrupt is received by Xen but the current
> > ## one is still being serviced 
> > - the guest EOIs the vtimer interrupt
> 
> Is this particular to the handling of the vtimer interrupt? 
Yes

> In the
> normal case an interrupt which is injected into the guest isn''t
EOIed by
> Xen until the maintenance interrupt, isn''t it?
Right

> But the vtimer is special
> because it isn''t routed to the guest even though we do so via an
> orthogonal mechanism?
Yes

> Perhaps the solution here is a third type of interrupt alongside
> "for-xen" and "for-a-specific-guest" which is
"for-current-guest"?
We could try but I did implement this strategy first and I ran into some
serious problems. It could have been because the vtimer emulation in the
FastModel had issues.

Hello,

I wonder if this patchset going to be revised and applied?
It appeared pretty useful for us.
We are experimenting with an Android as an only DomU. For sure it was done
with giving to Android direct access to iomem and delivering some hw
irq''s
to DomU.

Eventually we had showstopper issue with hw irqs delivery to DomU kernel.
At some moment some IRQs was not delivered to DomU. The issue disappeared
with applying this patchset.

Sincerely,
Andrii Anisov.


On Tue, Jun 25, 2013 at 2:04 AM, Julien Grall
<julien.grall@linaro.org>wrote:
> Hello,
>
> This patch series aims to fix different issues on Xen on ARM with the
> arndale and the versatile express:
>   - Handle correctly one shot IRQ (fixed by patch 3)
>   - Make timers work with heavy load (fixed by patch 2)
>   - Make ethernet card works on the TC2 (fixed by patch 5)
>
> Some of these patches (2 and 5) are proof of concept. I would be happy if
> someone find a better solution :).
>
> Cheers,
>
> Julien Grall (4):
>   xen/arm: Physical IRQ is not always equal to virtual IRQ
>   xen/arm: Don''t reinject the IRQ if it''s already in LRs
>   xen/arm: Rename gic_irq_{startup,shutdown} to gic_irq_{mask,unmask}
>   xen/arm: Only enable physical IRQs when the guest asks
>
> Stefano Stabellini (1):
>   xen/arm: Keep count of inflight interrupts
>
>  xen/arch/arm/domain_build.c  |   14 +++++
>  xen/arch/arm/gic.c           |  119
> ++++++++++++++++++++++++++++++------------
>  xen/arch/arm/vgic.c          |   11 ++--
>  xen/include/asm-arm/domain.h |    2 +
>  xen/include/asm-arm/gic.h    |    7 +++
>  xen/include/asm-arm/irq.h    |    6 +++
>  6 files changed, 122 insertions(+), 37 deletions(-)
>
> --
> 1.7.10.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

On 07/31/2013 02:08 PM, Andrii Anisov wrote:
> Hello,
Hi,
> I wonder if this patchset going to be revised and applied?
I will try to send as soon as possible a new version.

-- 
Julien

On Tue, 2013-06-25 at 17:19 +0100, Stefano Stabellini wrote:

We really need to get something along these lines into 4.4, as a bug fix
I think. It seems from the below that the issue which this patch tries
to address was actually observed on vexpress, although it doesn''t seem
likely to be specific to any platform.

> > @@ -744,6 +763,8 @@ int gic_route_irq_to_guest(struct domain *d, const
struct dt_irq *irq,
> >          goto out;
> >      }
> >  
> > +    p->desc = desc;
> > +
> >  out:
> >      spin_unlock(&gic.lock);
> >      spin_unlock_irqrestore(&desc->lock, flags);
> > @@ -685,10 +688,6 @@ 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
> > -        n->desc = NULL;
> >  
> >      /* the irq is enabled */
> >      if ( rank->ienable & (1 << (irq % 32)) )
> 
> I don''t quite understand why are you changing where the
"desc"
> assignement is done.
> If it is a cleanup you shouldn''t mix it with a patch like this
that is
> supposed to fix a specific issue. Otherwise please explain why you need
> this change.
Was this because gic_router_irq_to_guest has set p->desc as shown above
and therefore the feeling was that it wasn''t required here?

I don''t think this can be right though, gic_route_irq_to_guest is only
called for SPIs routed via the DTB or the platform specific mapping
hook. In particular it is never called for the various PPIs, such as the
timer PPI and the evtchn PPI.

As it happens the existing PPIs which we pass to the guest are a
virtual, and perhaps we can already rely on n->desc==NULL already in
that case.

TBH, the existing code here is very weird, n->desc should have been set
(possibly on all vcpus) at the time the interrupt was configured/routed,
it certainly shouldn''t be updated on every injection. In practice I
think it only happened on the first and was static thereafter so this is
just an odd form of lazy initialisation. If it is in fact changing on
each injection then I''ve no idea what this code is doing ;-)

Ian.

On Mon, 2 Dec 2013, Ian Campbell wrote:
> On Tue, 2013-06-25 at 17:19 +0100, Stefano Stabellini wrote:
> 
> We really need to get something along these lines into 4.4, as a bug fix
> I think. It seems from the below that the issue which this patch tries
> to address was actually observed on vexpress, although it doesn''t
seem
> likely to be specific to any platform.
FYI I am reworking this series using a different approach