Patchwork [08/15] hw/arm_gic.c: lower IRQ only on changing of enable bit.

login
register
mail settings
Submitter Evgeny Voevodin
Date Dec. 9, 2011, 1:34 p.m.
Message ID <1323437682-28792-9-git-send-email-e.voevodin@samsung.com>
Download mbox | patch
Permalink /patch/130375/
State New
Headers show

Comments

Evgeny Voevodin - Dec. 9, 2011, 1:34 p.m.
In previous version IRQ was lowered every time if enable bits were
not set. If platform has splitted IRQ source to pass IRQ to two
identical GICs simultaneously in first of which IRQ passing is
enabled but in second is disabled, handling IRQ by second GIC would
lower IRQ previously raised by first GIC.
Linux kernel v3.0 faces this problem.
The problem is avoided if IRQ is only lowered as result of
transitioning enable bits to zeroes.

Signed-off-by: Evgeny Voevodin <e.voevodin@samsung.com>
---
 hw/arm_gic.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)
Paul Brook - Dec. 9, 2011, 3:55 p.m.
> In previous version IRQ was lowered every time if enable bits were
> not set. If platform has splitted IRQ source to pass IRQ to two
> identical GICs simultaneously in first of which IRQ passing is
> enabled but in second is disabled, handling IRQ by second GIC would
> lower IRQ previously raised by first GIC.
> Linux kernel v3.0 faces this problem.
> The problem is avoided if IRQ is only lowered as result of
> transitioning enable bits to zeroes.

This is almost certainly wrong.  IRQ lines are state based, not event based. 
Re-asserting an existing level should always be a no-op.

My guess is your real bug it that your IRQ wiring is broken, and you have 
multiple outputs connected to a single input. Don't do that. Ever.  If you 
need to connent multiple output pins to a single input pin then you need an 
explicit device (e.g. an or gate) in between.

Paul

Patch

diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index 527c9ce..7e3db4f 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -84,7 +84,9 @@  typedef struct gic_state
     SysBusDevice busdev;
     qemu_irq parent_irq[NCPU];
     int enabled;
+    int enabled_prev;
     int cpu_enabled[NCPU];
+    int cpu_enabled_prev[NCPU];
 
     gic_irq_state irq_state[GIC_NIRQ];
 #ifndef NVIC
@@ -116,12 +118,22 @@  static void gic_update(gic_state *s)
     int level;
     int cpu;
     int cm;
+    int enabled_prev;
+    int cpu_enabled_prev;
 
+    enabled_prev = s->enabled_prev;
+    s->enabled_prev = s->enabled;
     for (cpu = 0; cpu < NUM_CPU(s); cpu++) {
+        cpu_enabled_prev = s->cpu_enabled_prev[cpu];
+        s->cpu_enabled_prev[cpu] = s->cpu_enabled[cpu];
         cm = 1 << cpu;
         s->current_pending[cpu] = 1023;
         if (!s->enabled || !s->cpu_enabled[cpu]) {
-	    qemu_irq_lower(s->parent_irq[cpu]);
+            /* lower IRQ only if enable bit was changed */
+            if (enabled_prev != s->enabled
+                    || cpu_enabled_prev != s->cpu_enabled[cpu]) {
+                qemu_irq_lower(s->parent_irq[cpu]);
+            }
             return;
         }
         best_prio = 0x100;
@@ -650,6 +662,7 @@  static void gic_reset(gic_state *s)
 #else
         s->cpu_enabled[i] = 0;
 #endif
+        s->cpu_enabled_prev[i] = s->cpu_enabled[i];
     }
     for (i = 0; i < 16; i++) {
         GIC_SET_ENABLED(i, ALL_CPU_MASK);
@@ -661,6 +674,7 @@  static void gic_reset(gic_state *s)
 #else
     s->enabled = 0;
 #endif
+    s->enabled_prev = s->enabled;
 }
 
 static void gic_save(QEMUFile *f, void *opaque)
@@ -669,8 +683,10 @@  static void gic_save(QEMUFile *f, void *opaque)
     int i;
     int j;
 
+    qemu_put_be32(f, s->enabled_prev);
     qemu_put_be32(f, s->enabled);
     for (i = 0; i < NUM_CPU(s); i++) {
+        qemu_put_be32(f, s->cpu_enabled_prev[i]);
         qemu_put_be32(f, s->cpu_enabled[i]);
         for (j = 0; j < 32; j++)
             qemu_put_be32(f, s->priority1[j][i]);
@@ -706,8 +722,10 @@  static int gic_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 2)
         return -EINVAL;
 
+    s->enabled_prev = qemu_get_be32(f);
     s->enabled = qemu_get_be32(f);
     for (i = 0; i < NUM_CPU(s); i++) {
+        s->cpu_enabled_prev[i] = qemu_get_be32(f);
         s->cpu_enabled[i] = qemu_get_be32(f);
         for (j = 0; j < 32; j++)
             s->priority1[j][i] = qemu_get_be32(f);