[Qemu-devel] [PATCH] target-arm: GIC: bug fixes for arm_gic.c

[Daniel Sangorrin]

Hi, I found some bugs in the way the IRQ number is calculated
at certain places in arm_gic.c. Perhaps there are a few more
errors that I didn't notice. These bugs were not noticeable
when running Linux as a guest, but I found them when running
my own porting of a multicore real-time OS (TOPPERS/FMP).

NOTE: the main problem I had was that the target CPUs where not
set correctly. Instead of GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
it should read as GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));

I have tested the patch both with FMP and with a recent Linux, but
please review it since I'm not an expert on QEMU and this is my
first patch.

target-arm: bug fixes for arm_gic.c

The IRQ number was not calculated correctly in the
function gic_dist_writeb for accesses to set/clear
pending and set/clear enable.

Signed-off-by: Daniel Sangorrin <address@hidden>
---
 hw/arm_gic.c |   90 ++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 50 insertions(+), 40 deletions(-)

diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index f9e423f..f3f233c 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -368,71 +368,81 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
     } else if (offset < 0x180) {
         /* Interrupt Set Enable.  */
         irq = (offset - 0x100) * 8 + GIC_BASE_IRQ;
+        for (i = 0; i < 8; i++) {
+                if (value & (1 << i)) {
+                        irq = irq + i;
+                        break;
+                }
+        }
         if (irq >= s->num_irq)
             goto bad_reg;
         if (irq < 16)
-          value = 0xff;
-        for (i = 0; i < 8; i++) {
-            if (value & (1 << i)) {
-                int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
-                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+            value = 0xff;
+        int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
+        int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;

-                if (!GIC_TEST_ENABLED(irq + i, cm)) {
-                    DPRINTF("Enabled IRQ %d\n", irq + i);
-                }
-                GIC_SET_ENABLED(irq + i, cm);
-                /* If a raised level triggered IRQ enabled then mark
-                   is as pending.  */
-                if (GIC_TEST_LEVEL(irq + i, mask)
-                        && !GIC_TEST_TRIGGER(irq + i)) {
-                    DPRINTF("Set %d pending mask %x\n", irq + i, mask);
-                    GIC_SET_PENDING(irq + i, mask);
-                }
-            }
+        if (!GIC_TEST_ENABLED(irq, cm)) {
+            DPRINTF("Enabled IRQ %d\n", irq);
+        }
+        GIC_SET_ENABLED(irq, cm);
+        /* If a raised level triggered IRQ enabled then mark is as pending */
+        if (GIC_TEST_LEVEL(irq, mask) && !GIC_TEST_TRIGGER(irq)) {
+            DPRINTF("Set %d pending mask %x\n", irq, mask);
+            GIC_SET_PENDING(irq, mask);
         }
     } else if (offset < 0x200) {
         /* Interrupt Clear Enable.  */
         irq = (offset - 0x180) * 8 + GIC_BASE_IRQ;
-        if (irq >= s->num_irq)
-            goto bad_reg;
-        if (irq < 16)
-          value = 0;
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
-                int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
-
-                if (GIC_TEST_ENABLED(irq + i, cm)) {
-                    DPRINTF("Disabled IRQ %d\n", irq + i);
-                }
-                GIC_CLEAR_ENABLED(irq + i, cm);
+                irq = irq + i;
+                break;
             }
         }
-    } else if (offset < 0x280) {
-        /* Interrupt Set Pending.  */
-        irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
         if (irq < 16)
-          irq = 0;
+            value = 0;
+
+        int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;

+        if (GIC_TEST_ENABLED(irq, cm)) {
+            DPRINTF("Disabled IRQ %d\n", irq);
+        }
+        GIC_CLEAR_ENABLED(irq, cm);
+    } else if (offset < 0x280) {
+        /* Interrupt Set Pending.  */
+        irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
-                GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
+                irq = irq + i;
+                break;
             }
         }
+
+        if (irq >= s->num_irq) {
+            goto bad_reg;
+        }
+        if (irq < 16) {
+            irq = 0;
+        }
+
+        GIC_SET_PENDING(irq, GIC_TARGET(irq));
     } else if (offset < 0x300) {
         /* Interrupt Clear Pending.  */
         irq = (offset - 0x280) * 8 + GIC_BASE_IRQ;
-        if (irq >= s->num_irq)
-            goto bad_reg;
         for (i = 0; i < 8; i++) {
-            /* ??? This currently clears the pending bit for all CPUs, even
-               for per-CPU interrupts.  It's unclear whether this is the
-               corect behavior.  */
-            if (value & (1 << i)) {
-                GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
-            }
+                if (value & (1 << i)) {
+                        irq = irq + i;
+                        break;
+                }
         }
+
+        if (irq >= s->num_irq) {
+            goto bad_reg;
+        }
+
+        GIC_CLEAR_PENDING(irq, ALL_CPU_MASK);
     } else if (offset < 0x400) {
         /* Interrupt Active.  */
         goto bad_reg;
-- 
1.7.9.5