[Qemu-devel] [PATCH v5 0/8] Support arm-gic-kvm save/restore

[Qemu-devel] [PATCH v5 0/8] Support arm-gic-kvm save/restore

[Christoffer Dall]

Implement support to save/restore the ARM KVM VGIC state from the
kernel.  The basic appraoch is to transfer state from the in-kernel VGIC
to the emulated arm-gic state representation and let the standard QEMU
vmstate save/restore handle saving the arm-gic state.  Restore works by
reversing the process.

The first patches adds missing features and fixes issues with the
arm-gic implementation in qemu in preparation for the actual
save/restore logic.

The patches depend on the device control patch series sent out earlier,
which can also be found here:
git://git.linaro.org/people/cdall/qemu-arm.git migration/device-ctrl-v4

The whole patch series based on top of the above can be found here:
git://git.linaro.org/people/cdall/qemu-arm.git migration/vgic-v5

Changes are described in the individual patches.

Christoffer Dall (8):
  arm_gic: Introduce define for GIC_NR_SGIS
  arm_gic: Fix GICD_ICPENDR and GICD_ISPENDR writes
  arm_gic: Fix GIC pending behavior
  hw: arm_gic: Keep track of SGI sources
  arm_gic: Support setting/getting binary point reg
  vmstate: Add uint32 2D-array support
  arm_gic: Add GICC_APRn state to the GICState
  hw: arm_gic_kvm: Add KVM VGIC save/restore logic

 hw/intc/arm_gic.c                | 202 +++++++++++++++----
 hw/intc/arm_gic_common.c         |   8 +-
 hw/intc/arm_gic_kvm.c            | 424 ++++++++++++++++++++++++++++++++++++++-
 hw/intc/gic_internal.h           |  16 +-
 include/hw/intc/arm_gic_common.h |  34 ++++
 include/migration/vmstate.h      |   6 +
 6 files changed, 647 insertions(+), 43 deletions(-)

-- 
1.8.5.2

[Qemu-devel] [PATCH v5 1/8] arm_gic: Introduce define for GIC_NR_SGIS

[Christoffer Dall]

Instead of hardcoding 16 various places in the code, use a define to
make it more clear what is going on.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes [v1 -> v5]:
 - New patch in series

 hw/intc/arm_gic.c                | 17 +++++++++++------
 include/hw/intc/arm_gic_common.h |  1 +
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 9409684..98c6ff5 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -380,8 +380,10 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         irq = (offset - 0x100) * 8 + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        if (irq < 16)
-          value = 0xff;
+        if (irq < GIC_NR_SGIS) {
+            value = 0xff;
+        }
+
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
                 int mask =
@@ -406,8 +408,10 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         irq = (offset - 0x180) * 8 + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        if (irq < 16)
-          value = 0;
+        if (irq < GIC_NR_SGIS) {
+            value = 0;
+        }
+
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
                 int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
@@ -423,8 +427,9 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        if (irq < 16)
-          irq = 0;
+        if (irq < GIC_NR_SGIS) {
+            irq = 0;
+        }
 
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 40cd3d6..dbf8787 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -27,6 +27,7 @@
 #define GIC_MAXIRQ 1020
 /* First 32 are private to each CPU (SGIs and PPIs). */
 #define GIC_INTERNAL 32
+#define GIC_NR_SGIS 16
 /* Maximum number of possible CPU interfaces, determined by GIC architecture */
 #define GIC_NCPU 8
 
-- 
1.8.5.2

[Qemu-devel] [PATCH v5 2/8] arm_gic: Fix GICD_ICPENDR and GICD_ISPENDR writes

[Christoffer Dall]

Fix two bugs that would allow changing the state of SGIs through the
ICPENDR and ISPENDRs.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes [v1 -> v5]:
 - New patch in series

 hw/intc/arm_gic.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 98c6ff5..1c4a114 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -428,7 +428,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         if (irq >= s->num_irq)
             goto bad_reg;
         if (irq < GIC_NR_SGIS) {
-            irq = 0;
+            value = 0;
         }
 
         for (i = 0; i < 8; i++) {
@@ -441,6 +441,10 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         irq = (offset - 0x280) * 8 + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
+        if (irq < GIC_NR_SGIS) {
+            value = 0;
+        }
+
         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
-- 
1.8.5.2

[Qemu-devel] [PATCH v5 3/8] arm_gic: Fix GIC pending behavior

[Christoffer Dall]

The existing implementation of the pending behavior in gic_set_irq,
gic_acknowledge_irq, gic_complete_irq, and the distributor pending
set/clear registers does not follow the semantics of the GICv2.0 specs,
but may implement the 11MPCore support.  Therefore, maintain the
existing semantics for 11MPCore and v7M NVIC and change the behavior to
be in accordance with the GICv2.0 specs for "generic implementations"
(s->revision == 1 || s->revision == 2).

Generic implementations distinguish between setting a level-triggered
interrupt pending through writes to the GICD_ISPENDR and when hardware
raises the interrupt line.  Writing to the GICD_ICPENDR will not cause
the interrupt to become non-pending if the line is still active, and
conversely, if the line is deactivated but the interrupt is marked as
pending through a write to GICD_ISPENDR, the interrupt remains pending.
Handle this situation in the GIC_TEST_PENDING (which now becomes a
static inline named gic_test_pending) and let the 'pending' field
correspond only to the latched state of the D-flip flop in the GICv2.0
specs Figure 4-10.

The following changes are added:

gic_test_pending:
Make this a static inline and split out the 11MPCore from the generic
behavior.  For the generic behavior, consider interrupts pending if:
    ((s->irq_state[irq].pending & (cm) != 0) ||
       (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm))

gic_set_irq:
Split out the 11MPCore from the generic behavior.  For the generic
behavior, always GIC_SET_LEVEL(irq, 1) on positive level, but only
GIC_SET_PENDING for edge-triggered interrupts and vice versa on a
negative level.

gic_complete_irq:
Only resample the line for line-triggered interrupts on an 11MPCore.
Generic implementations will sample the line directly in
gic_test_pending().

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes [v4 -> v5]:
 - Factor out GIC_NR_SGIS and gic_dist_writeb bugfixes
 - Change meaning of pending field for level-triggered interrupts for
   GIC v1/v2, to only capture manually written state to pending
   registers.  Add or-clause to gic_test_pending to sample the line
   state, as per Peter's suggestions:
   https://lists.cs.columbia.edu/pipermail/kvmarm/2014-January/008798.html

Changes [v3 -> v4]:
 - Maintain 11MPCore semantics
 - Combine all pending interrupts fixing patches into this patch.  See
   the detailed description above.

Changes [v1 -> v2]:
 - Fix bisection issue, by not using gic_clear_pending yet.

 hw/intc/arm_gic.c      | 74 ++++++++++++++++++++++++++++++++++++--------------
 hw/intc/gic_internal.h | 16 ++++++++++-
 2 files changed, 69 insertions(+), 21 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 1c4a114..5e2cf14 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -66,7 +66,7 @@ void gic_update(GICState *s)
         best_prio = 0x100;
         best_irq = 1023;
         for (irq = 0; irq < s->num_irq; irq++) {
-            if (GIC_TEST_ENABLED(irq, cm) && GIC_TEST_PENDING(irq, cm)) {
+            if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, cm)) {
                 if (GIC_GET_PRIORITY(irq, cpu) < best_prio) {
                     best_prio = GIC_GET_PRIORITY(irq, cpu);
                     best_irq = irq;
@@ -89,14 +89,46 @@ void gic_set_pending_private(GICState *s, int cpu, int irq)
 {
     int cm = 1 << cpu;
 
-    if (GIC_TEST_PENDING(irq, cm))
+    if (gic_test_pending(s, irq, cm)) {
         return;
+    }
 
     DPRINTF("Set %d pending cpu %d\n", irq, cpu);
     GIC_SET_PENDING(irq, cm);
     gic_update(s);
 }
 
+static void gic_set_irq_11mpcore(GICState *s, int irq, int level,
+                                 int cm, int target)
+{
+    if (level) {
+        GIC_SET_LEVEL(irq, cm);
+        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
+            DPRINTF("Set %d pending mask %x\n", irq, target);
+            GIC_SET_PENDING(irq, target);
+        }
+    } else {
+        GIC_CLEAR_LEVEL(irq, cm);
+    }
+}
+
+static void gic_set_irq_generic(GICState *s, int irq, int level,
+                                int cm, int target)
+{
+    if (level) {
+        GIC_SET_LEVEL(irq, cm);
+        DPRINTF("Set %d pending mask %x\n", irq, target);
+        if (GIC_TEST_EDGE_TRIGGER(irq)) {
+            GIC_SET_PENDING(irq, target);
+        }
+    } else {
+        if (GIC_TEST_EDGE_TRIGGER(irq)) {
+            GIC_CLEAR_PENDING(irq, target);
+        }
+        GIC_CLEAR_LEVEL(irq, cm);
+    }
+}
+
 /* Process a change in an external IRQ input.  */
 static void gic_set_irq(void *opaque, int irq, int level)
 {
@@ -126,15 +158,12 @@ static void gic_set_irq(void *opaque, int irq, int level)
         return;
     }
 
-    if (level) {
-        GIC_SET_LEVEL(irq, cm);
-        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
-            DPRINTF("Set %d pending mask %x\n", irq, target);
-            GIC_SET_PENDING(irq, target);
-        }
+    if (s->revision == REV_11MPCORE) {
+        gic_set_irq_11mpcore(s, irq, level, cm, target);
     } else {
-        GIC_CLEAR_LEVEL(irq, cm);
+        gic_set_irq_generic(s, irq, level, cm, target);
     }
+
     gic_update(s);
 }
 
@@ -160,9 +189,10 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
         return 1023;
     }
     s->last_active[new_irq][cpu] = s->running_irq[cpu];
-    /* Clear pending flags for both level and edge triggered interrupts.
-       Level triggered IRQs will be reasserted once they become inactive.  */
-    GIC_CLEAR_PENDING(new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm);
+
+    cm = GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm;
+    GIC_CLEAR_PENDING(new_irq, cm);
+
     gic_set_running_irq(s, cpu, new_irq);
     DPRINTF("ACK %d\n", new_irq);
     return new_irq;
@@ -195,14 +225,18 @@ void gic_complete_irq(GICState *s, int cpu, int irq)
     }
     if (s->running_irq[cpu] == 1023)
         return; /* No active IRQ.  */
-    /* Mark level triggered interrupts as pending if they are still
-       raised.  */
-    if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
-        && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) {
-        DPRINTF("Set %d pending mask %x\n", irq, cm);
-        GIC_SET_PENDING(irq, cm);
-        update = 1;
+
+    if (s->revision == REV_11MPCORE) {
+        /* Mark level triggered interrupts as pending if they are still
+           raised.  */
+        if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
+            && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) {
+            DPRINTF("Set %d pending mask %x\n", irq, cm);
+            GIC_SET_PENDING(irq, cm);
+            update = 1;
+        }
     }
+
     if (irq != s->running_irq[cpu]) {
         /* Complete an IRQ that is not currently running.  */
         int tmp = s->running_irq[cpu];
@@ -273,7 +307,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
         res = 0;
         mask = (irq < GIC_INTERNAL) ?  cm : ALL_CPU_MASK;
         for (i = 0; i < 8; i++) {
-            if (GIC_TEST_PENDING(irq + i, mask)) {
+            if (gic_test_pending(s, irq + i, mask)) {
                 res |= (1 << i);
             }
         }
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 8c02d58..92a6f7a 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -34,7 +34,6 @@
 #define GIC_TEST_ENABLED(irq, cm) ((s->irq_state[irq].enabled & (cm)) != 0)
 #define GIC_SET_PENDING(irq, cm) s->irq_state[irq].pending |= (cm)
 #define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm)
-#define GIC_TEST_PENDING(irq, cm) ((s->irq_state[irq].pending & (cm)) != 0)
 #define GIC_SET_ACTIVE(irq, cm) s->irq_state[irq].active |= (cm)
 #define GIC_CLEAR_ACTIVE(irq, cm) s->irq_state[irq].active &= ~(cm)
 #define GIC_TEST_ACTIVE(irq, cm) ((s->irq_state[irq].active & (cm)) != 0)
@@ -63,4 +62,19 @@ void gic_update(GICState *s);
 void gic_init_irqs_and_distributor(GICState *s, int num_irq);
 void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
 
+static inline bool gic_test_pending(GICState *s, int irq, int cm)
+{
+    if (s->revision == REV_NVIC || s->revision == REV_11MPCORE) {
+        return s->irq_state[irq].pending & cm;
+    } else {
+        /* Edge-triggered interrupts are marked pending on a rising edge, but
+         * level-triggered interrupts are either considered pending when the
+         * level is active or if software has explicitly written to
+         * GICD_ISPENDR to set the state pending.
+         */
+        return (s->irq_state[irq].pending & cm) ||
+            (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm));
+    }
+}
+
 #endif /* !QEMU_ARM_GIC_INTERNAL_H */
-- 
1.8.5.2

[Qemu-devel] [PATCH v5 4/8] hw: arm_gic: Keep track of SGI sources

[Christoffer Dall]

Right now the arm gic emulation doesn't keep track of the source of an
SGI (which apparently Linux guests don't use, or they're fine with
assuming CPU 0 always).

Add the necessary matrix on the GICState structure and maintain the data
when setting and clearing the pending state of an IRQ and make the state
visible to the guest.

Note that we always choose to present the source as the lowest-numbered
CPU in case multiple cores have signalled the same SGI number to a core
on the system.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes [v4 -> v5]:
 - Add braces to if-statement
 - Make GICD_CPENDSGIR and GICD_SPENDSGIR guest visible for v1/v2 GIC

Changes [v3 -> v4]:
 - Assert that we are not messing with SGI state in gic_set_irq
 - Move bugfix of GICD_SPENDR to pending fixes patch
 - Get rid of the idea of git_clear_pending and handle clearing of
   source bits directly in gic_acknowledge_irq
 - Don't loop through CPUs to clear SGI sources
 - Return source CPU directly from gic_acknowledge_irq
 - Rename sgi_source to sgi_pending
 - Add comment (courtesey of Peter) to sgi_pending struct member.

Changes [v2 -> v3]:
 - Changed ffs(x) - 1 to ctz32
 - Changed cpu type to int in gic_clear_pending to avoid cast
 - Really try to fix the endless loop bug
 - Change gic_clear_pending to only clear the pending bit of SGIs if all
   CPUs do not have that IRQ pending from any CPUs.
 - Wrap long line in gic_internal.h
 - Fix bug allowing setting SGIs through the GICD_SPENDR

Changes [v1 -> v2]:
 - Fixed endless loop bug
 - Bump version_id and minimum_version_id on vmstate struct

 hw/intc/arm_gic.c                | 94 +++++++++++++++++++++++++++++++++++-----
 hw/intc/arm_gic_common.c         |  5 ++-
 include/hw/intc/arm_gic_common.h |  7 +++
 3 files changed, 93 insertions(+), 13 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 5e2cf14..4054fb6 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -154,6 +154,8 @@ static void gic_set_irq(void *opaque, int irq, int level)
         target = cm;
     }
 
+    assert(irq >= GIC_NR_SGIS);
+
     if (level == GIC_TEST_LEVEL(irq, cm)) {
         return;
     }
@@ -180,22 +182,49 @@ static void gic_set_running_irq(GICState *s, int cpu, int irq)
 
 uint32_t gic_acknowledge_irq(GICState *s, int cpu)
 {
-    int new_irq;
+    int ret, irq, src;
     int cm = 1 << cpu;
-    new_irq = s->current_pending[cpu];
-    if (new_irq == 1023
-            || GIC_GET_PRIORITY(new_irq, cpu) >= s->running_priority[cpu]) {
+    irq = s->current_pending[cpu];
+    if (irq == 1023
+            || GIC_GET_PRIORITY(irq, cpu) >= s->running_priority[cpu]) {
         DPRINTF("ACK no pending IRQ\n");
         return 1023;
     }
-    s->last_active[new_irq][cpu] = s->running_irq[cpu];
+    s->last_active[irq][cpu] = s->running_irq[cpu];
 
-    cm = GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm;
-    GIC_CLEAR_PENDING(new_irq, cm);
+    cm = GIC_TEST_MODEL(irq) ? ALL_CPU_MASK : cm;
+    if (s->revision == REV_11MPCORE) {
+        /* Clear pending flags for both level and edge triggered interrupts.
+         * Level triggered IRQs will be reasserted once they become inactive.
+         */
+        GIC_CLEAR_PENDING(irq, cm);
+        ret = irq;
+    } else {
+        if (irq < GIC_NR_SGIS) {
+            /* Lookup the source CPU for the SGI and clear this in the
+             * sgi_pending map.  Return the src and clear the overall pending
+             * state on this CPU if the SGI is not pending from any CPUs.
+             */
+            assert(s->sgi_pending[irq][cpu] != 0);
+            src = ctz32(s->sgi_pending[irq][cpu]);
+            s->sgi_pending[irq][cpu] &= ~(1 << src);
+            if (s->sgi_pending[irq][cpu] == 0) {
+                GIC_CLEAR_PENDING(irq, cm);
+            }
+            ret = irq | ((src & 0x7) << 10);
+        } else {
+            /* Clear pending state for both level and edge triggered
+             * interrupts. (level triggered interrupts with an active line
+             * remain pending, see gic_test_pending)
+             */
+            GIC_CLEAR_PENDING(irq, cm);
+            ret = irq;
+        }
+    }
 
-    gic_set_running_irq(s, cpu, new_irq);
-    DPRINTF("ACK %d\n", new_irq);
-    return new_irq;
+    gic_set_running_irq(s, cpu, irq);
+    DPRINTF("ACK %d\n", irq);
+    return ret;
 }
 
 void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
@@ -357,6 +386,22 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
             if (GIC_TEST_EDGE_TRIGGER(irq + i))
                 res |= (2 << (i * 2));
         }
+    } else if (offset < 0xf10) {
+        goto bad_reg;
+    } else if (offset < 0xf30) {
+        if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
+            goto bad_reg;
+        }
+
+        if (offset < 0xf20) {
+            /* GICD_CPENDSGIRn */
+            irq = (offset - 0xf10);
+        } else {
+            irq = (offset - 0xf20);
+            /* GICD_SPENDSGIRn */
+        }
+
+        res = s->sgi_pending[irq][cpu];
     } else if (offset < 0xfe0) {
         goto bad_reg;
     } else /* offset >= 0xfe0 */ {
@@ -531,9 +576,29 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
                 GIC_CLEAR_EDGE_TRIGGER(irq + i);
             }
         }
-    } else {
+    } else if (offset < 0xf10) {
         /* 0xf00 is only handled for 32-bit writes.  */
         goto bad_reg;
+    } else if (offset < 0xf20) {
+        /* GICD_CPENDSGIRn */
+        if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
+            goto bad_reg;
+        }
+        irq = (offset - 0xf10);
+
+        GIC_CLEAR_PENDING(irq, 1 << cpu);
+        s->sgi_pending[irq][cpu] &= ~value;
+    } else if (offset < 0xf30) {
+        /* GICD_SPENDSGIRn */
+        if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
+            goto bad_reg;
+        }
+        irq = (offset - 0xf20);
+
+        GIC_SET_PENDING(irq, 1 << cpu);
+        s->sgi_pending[irq][cpu] |= value;
+    } else {
+        goto bad_reg;
     }
     gic_update(s);
     return;
@@ -557,6 +622,7 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
         int cpu;
         int irq;
         int mask;
+        int target_cpu;
 
         cpu = gic_get_current_cpu(s);
         irq = value & 0x3ff;
@@ -576,6 +642,12 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
             break;
         }
         GIC_SET_PENDING(irq, mask);
+        target_cpu = ctz32(mask);
+        while (target_cpu < GIC_NCPU) {
+            s->sgi_pending[irq][target_cpu] |= (1 << cpu);
+            mask &= ~(1 << target_cpu);
+            target_cpu = ctz32(mask);
+        }
         gic_update(s);
         return;
     }
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index e4fc650..92de7f8 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -58,8 +58,8 @@ static const VMStateDescription vmstate_gic_irq_state = {
 
 static const VMStateDescription vmstate_gic = {
     .name = "arm_gic",
-    .version_id = 4,
-    .minimum_version_id = 4,
+    .version_id = 5,
+    .minimum_version_id = 5,
     .pre_save = gic_pre_save,
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
@@ -71,6 +71,7 @@ static const VMStateDescription vmstate_gic = {
         VMSTATE_UINT8_2DARRAY(priority1, GICState, GIC_INTERNAL, GIC_NCPU),
         VMSTATE_UINT8_ARRAY(priority2, GICState, GIC_MAXIRQ - GIC_INTERNAL),
         VMSTATE_UINT16_2DARRAY(last_active, GICState, GIC_MAXIRQ, GIC_NCPU),
+        VMSTATE_UINT8_2DARRAY(sgi_pending, GICState, GIC_NR_SGIS, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(priority_mask, GICState, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(running_irq, GICState, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(running_priority, GICState, GIC_NCPU),
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index dbf8787..5dd826d 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -55,6 +55,13 @@ typedef struct GICState {
     uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
     uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
     uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
+    /* For each SGI on the target CPU, we store 8 bits
+     * indicating which source CPUs have made this SGI
+     * pending on the target CPU. These correspond to
+     * the bytes in the GIC_SPENDSGIR* registers as
+     * read by the target CPU.
+     */
+    uint8_t sgi_pending[GIC_NR_SGIS][GIC_NCPU];
 
     uint16_t priority_mask[GIC_NCPU];
     uint16_t running_irq[GIC_NCPU];
-- 
1.8.5.2

[Qemu-devel] [PATCH v5 5/8] arm_gic: Support setting/getting binary point reg

[Christoffer Dall]

Add a binary_point field to the gic emulation structure and support
setting/getting this register now when we have it.  We don't actually
support interrupt grouping yet, oh well.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes [v2 -> v3]:
 - Treat writes for GIC prior to v2 as RAZ/WI.

Changes [v1 -> v2]:
 - Renamed binary_point to bpr and abpr
 - Added GICC_ABPR read-as-write logic for TCG

 hw/intc/arm_gic.c                | 12 +++++++++---
 hw/intc/arm_gic_common.c         |  6 ++++--
 include/hw/intc/arm_gic_common.h |  7 +++++++
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 4054fb6..36473a0 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -671,14 +671,15 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
     case 0x04: /* Priority mask */
         return s->priority_mask[cpu];
     case 0x08: /* Binary Point */
-        /* ??? Not implemented.  */
-        return 0;
+        return s->bpr[cpu];
     case 0x0c: /* Acknowledge */
         return gic_acknowledge_irq(s, cpu);
     case 0x14: /* Running Priority */
         return s->running_priority[cpu];
     case 0x18: /* Highest Pending Interrupt */
         return s->current_pending[cpu];
+    case 0x1c: /* Aliased Binary Point */
+        return s->abpr[cpu];
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "gic_cpu_read: Bad offset %x\n", (int)offset);
@@ -697,10 +698,15 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
         s->priority_mask[cpu] = (value & 0xff);
         break;
     case 0x08: /* Binary Point */
-        /* ??? Not implemented.  */
+        s->bpr[cpu] = (value & 0x7);
         break;
     case 0x10: /* End Of Interrupt */
         return gic_complete_irq(s, cpu, value & 0x3ff);
+    case 0x1c: /* Aliased Binary Point */
+        if (s->revision >= 2) {
+            s->abpr[cpu] = (value & 0x7);
+        }
+        break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "gic_cpu_write: Bad offset %x\n", (int)offset);
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 92de7f8..d2d8ce1 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -58,8 +58,8 @@ static const VMStateDescription vmstate_gic_irq_state = {
 
 static const VMStateDescription vmstate_gic = {
     .name = "arm_gic",
-    .version_id = 5,
-    .minimum_version_id = 5,
+    .version_id = 6,
+    .minimum_version_id = 6,
     .pre_save = gic_pre_save,
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
@@ -76,6 +76,8 @@ static const VMStateDescription vmstate_gic = {
         VMSTATE_UINT16_ARRAY(running_irq, GICState, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(running_priority, GICState, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(current_pending, GICState, GIC_NCPU),
+        VMSTATE_UINT8_ARRAY(bpr, GICState, GIC_NCPU),
+        VMSTATE_UINT8_ARRAY(abpr, GICState, GIC_NCPU),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 5dd826d..0f0644b 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -68,6 +68,13 @@ typedef struct GICState {
     uint16_t running_priority[GIC_NCPU];
     uint16_t current_pending[GIC_NCPU];
 
+    /* We present the GICv2 without security extensions to a guest and
+     * therefore the guest can configure the GICC_CTLR to configure group 1
+     * binary point in the abpr.
+     */
+    uint8_t  bpr[GIC_NCPU];
+    uint8_t  abpr[GIC_NCPU];
+
     uint32_t num_cpu;
 
     MemoryRegion iomem; /* Distributor */
-- 
1.8.5.2

[Qemu-devel] [PATCH v5 6/8] vmstate: Add uint32 2D-array support

[Christoffer Dall]

Add support for saving VMState of 2D arrays of uint32 values.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 include/migration/vmstate.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index be193ba..a106d51 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -656,9 +656,15 @@ extern const VMStateInfo vmstate_info_bitmap;
 #define VMSTATE_UINT32_ARRAY_V(_f, _s, _n, _v)                        \
     VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_uint32, uint32_t)
 
+#define VMSTATE_UINT32_2DARRAY_V(_f, _s, _n1, _n2, _v)                \
+    VMSTATE_2DARRAY(_f, _s, _n1, _n2, _v, vmstate_info_uint32, uint32_t)
+
 #define VMSTATE_UINT32_ARRAY(_f, _s, _n)                              \
     VMSTATE_UINT32_ARRAY_V(_f, _s, _n, 0)
 
+#define VMSTATE_UINT32_2DARRAY(_f, _s, _n1, _n2)                      \
+    VMSTATE_UINT32_2DARRAY_V(_f, _s, _n1, _n2, 0)
+
 #define VMSTATE_UINT64_ARRAY_V(_f, _s, _n, _v)                        \
     VMSTATE_ARRAY(_f, _s, _n, _v, vmstate_info_uint64, uint64_t)
 
-- 
1.8.5.2

[Qemu-devel] [PATCH v5 7/8] arm_gic: Add GICC_APRn state to the GICState

[Christoffer Dall]

The GICC_APRn registers are not currently supported by the ARM GIC v2.0
emulation.  This patch adds the missing state.

Note that we also change the number of APRs to use a define GIC_NR_APRS
based on the maximum number of preemption levels.  This patch also adds
RAZ/WI accessors for the four registers on the emulated CPU interface.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes [v4 -> v5]:
 - Add TODO comment about reworking last_active and running_irq to use
   te new field instead.

Changes [v3 -> v4]:
 - Fixed grammatical error and use qemu_log_mask for print.

 hw/intc/arm_gic.c                |  5 +++++
 hw/intc/arm_gic_common.c         |  5 +++--
 include/hw/intc/arm_gic_common.h | 19 +++++++++++++++++++
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 36473a0..e4f7eba 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -680,6 +680,8 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
         return s->current_pending[cpu];
     case 0x1c: /* Aliased Binary Point */
         return s->abpr[cpu];
+    case 0xd0: case 0xd4: case 0xd8: case 0xdc:
+        return s->apr[(offset - 0xd0) / 4][cpu];
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "gic_cpu_read: Bad offset %x\n", (int)offset);
@@ -707,6 +709,9 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
             s->abpr[cpu] = (value & 0x7);
         }
         break;
+    case 0xd0: case 0xd4: case 0xd8: case 0xdc:
+        qemu_log_mask(LOG_UNIMP, "Writing APR not implemented\n");
+        break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "gic_cpu_write: Bad offset %x\n", (int)offset);
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index d2d8ce1..6d884ec 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -58,8 +58,8 @@ static const VMStateDescription vmstate_gic_irq_state = {
 
 static const VMStateDescription vmstate_gic = {
     .name = "arm_gic",
-    .version_id = 6,
-    .minimum_version_id = 6,
+    .version_id = 7,
+    .minimum_version_id = 7,
     .pre_save = gic_pre_save,
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
@@ -78,6 +78,7 @@ static const VMStateDescription vmstate_gic = {
         VMSTATE_UINT16_ARRAY(current_pending, GICState, GIC_NCPU),
         VMSTATE_UINT8_ARRAY(bpr, GICState, GIC_NCPU),
         VMSTATE_UINT8_ARRAY(abpr, GICState, GIC_NCPU),
+        VMSTATE_UINT32_2DARRAY(apr, GICState, GIC_NR_APRS, GIC_NCPU),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 0f0644b..f6887ed 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -31,6 +31,9 @@
 /* Maximum number of possible CPU interfaces, determined by GIC architecture */
 #define GIC_NCPU 8
 
+#define MAX_NR_GROUP_PRIO 128
+#define GIC_NR_APRS (MAX_NR_GROUP_PRIO / 32)
+
 typedef struct gic_irq_state {
     /* The enable bits are only banked for per-cpu interrupts.  */
     uint8_t enabled;
@@ -75,6 +78,22 @@ typedef struct GICState {
     uint8_t  bpr[GIC_NCPU];
     uint8_t  abpr[GIC_NCPU];
 
+    /* The APR is implementation defined, so we choose a layout identical to
+     * the KVM ABI layout for QEMU's implementation of the gic:
+     * If an interrupt for preemption level X is active, then
+     *   APRn[X mod 32] == 0b1,  where n = X / 32
+     * otherwise the bit is clear.
+     *
+     * TODO: rewrite the interrupt acknowlege/complete routines to use
+     * the APR registers to track the necessary information to update
+     * s->running_priority[] on interrupt completion (ie completely remove
+     * last_active[][] and running_irq[]). This will be necessary if we ever
+     * want to support TCG<->KVM migration, or TCG guests which can
+     * do power management involving powering down and restarting
+     * the GIC.
+     */
+    uint32_t apr[GIC_NR_APRS][GIC_NCPU];
+
     uint32_t num_cpu;
 
     MemoryRegion iomem; /* Distributor */
-- 
1.8.5.2

[Qemu-devel] [PATCH v5 8/8] hw: arm_gic_kvm: Add KVM VGIC save/restore logic

[Christoffer Dall]

Save and restore the ARM KVM VGIC state from the kernel.  We rely on
QEMU to marshal the GICState data structure and therefore simply
synchronize the kernel state with the QEMU emulated state in both
directions.

We take some care on the restore path to check the VGIC has been
configured with enough IRQs and CPU interfaces that we can properly
restore the state, and for separate set/clear registers we first fully
clear the registers and then set the required bits.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes [v2 -> v3]:
 - Separate patch for adding the APR register state

Changes [v1 -> v2]:
 - Remove num_irq from GIC VMstate structure
 - Increment GIC VMstate version number
 - Use extract32/deposit32 for bit-field modifications
 - Address other smaller review comments
 - Renames kvm_arm_gic_dist_[readr/writer] functions to
   kvm_dist_[get/put] and shortened other function names
 - Use concrete format for APRn

 hw/intc/arm_gic_kvm.c | 424 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 422 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 964d4d7..100b6bf 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -3,6 +3,7 @@
  *
  * Copyright (c) 2012 Linaro Limited
  * Written by Peter Maydell
+ * Save/Restore logic added by Christoffer Dall.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -23,6 +24,20 @@
 #include "kvm_arm.h"
 #include "gic_internal.h"
 
+//#define DEBUG_GIC_KVM
+
+#ifdef DEBUG_GIC_KVM
+static const int debug_gic_kvm = 1;
+#else
+static const int debug_gic_kvm = 0;
+#endif
+
+#define DPRINTF(fmt, ...) do { \
+        if (debug_gic_kvm) { \
+            printf("arm_gic: " fmt , ## __VA_ARGS__); \
+        } \
+    } while (0)
+
 #define TYPE_KVM_ARM_GIC "kvm-arm-gic"
 #define KVM_ARM_GIC(obj) \
      OBJECT_CHECK(GICState, (obj), TYPE_KVM_ARM_GIC)
@@ -72,14 +87,419 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
     kvm_set_irq(kvm_state, kvm_irq, !!level);
 }
 
+static bool kvm_arm_gic_can_save_restore(GICState *s)
+{
+    return s->dev_fd >= 0;
+}
+
+static void kvm_gic_access(GICState *s, int group, int offset,
+                                   int cpu, uint32_t *val, bool write)
+{
+    struct kvm_device_attr attr;
+    int type;
+    int err;
+
+    cpu = cpu & 0xff;
+
+    attr.flags = 0;
+    attr.group = group;
+    attr.attr = (((uint64_t)cpu << KVM_DEV_ARM_VGIC_CPUID_SHIFT) &
+                 KVM_DEV_ARM_VGIC_CPUID_MASK) |
+                (((uint64_t)offset << KVM_DEV_ARM_VGIC_OFFSET_SHIFT) &
+                 KVM_DEV_ARM_VGIC_OFFSET_MASK);
+    attr.addr = (uintptr_t)val;
+
+    if (write) {
+        type = KVM_SET_DEVICE_ATTR;
+    } else {
+        type = KVM_GET_DEVICE_ATTR;
+    }
+
+    err = kvm_device_ioctl(s->dev_fd, type, &attr);
+    if (err < 0) {
+        fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
+                strerror(-err));
+        abort();
+    }
+}
+
+static void kvm_gicd_access(GICState *s, int offset, int cpu,
+                            uint32_t *val, bool write)
+{
+    kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
+                   offset, cpu, val, write);
+}
+
+static void kvm_gicc_access(GICState *s, int offset, int cpu,
+                            uint32_t *val, bool write)
+{
+    kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CPU_REGS,
+                   offset, cpu, val, write);
+}
+
+#define for_each_irq_reg(_ctr, _max_irq, _field_width) \
+    for (_ctr = 0; _ctr < ((_max_irq) / (32 / (_field_width))); _ctr++)
+
+/*
+ * Translate from the in-kernel field for an IRQ value to/from the qemu
+ * representation.
+ */
+typedef void (*vgic_translate_fn)(GICState *s, int irq, int cpu,
+                                  uint32_t *field, bool to_kernel);
+
+/* synthetic translate function used for clear/set registers to completely
+ * clear a setting using a clear-register before setting the remaing bits
+ * using a set-register */
+static void translate_clear(GICState *s, int irq, int cpu,
+                            uint32_t *field, bool to_kernel)
+{
+    if (to_kernel) {
+        *field = ~0;
+    } else {
+        /* does not make sense: qemu model doesn't use set/clear regs */
+        abort();
+    }
+}
+
+static void translate_enabled(GICState *s, int irq, int cpu,
+                              uint32_t *field, bool to_kernel)
+{
+    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+
+    if (to_kernel) {
+        *field = GIC_TEST_ENABLED(irq, cm);
+    } else {
+        if (*field & 1) {
+            GIC_SET_ENABLED(irq, cm);
+        }
+    }
+}
+
+static void translate_pending(GICState *s, int irq, int cpu,
+                              uint32_t *field, bool to_kernel)
+{
+    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+
+    if (to_kernel) {
+        *field = gic_test_pending(s, irq, cm);
+    } else {
+        if (*field & 1) {
+            GIC_SET_PENDING(irq, cm);
+            /* TODO: Capture is level-line is held high in the kernel */
+        }
+    }
+}
+
+static void translate_active(GICState *s, int irq, int cpu,
+                             uint32_t *field, bool to_kernel)
+{
+    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+
+    if (to_kernel) {
+        *field = GIC_TEST_ACTIVE(irq, cm);
+    } else {
+        if (*field & 1) {
+            GIC_SET_ACTIVE(irq, cm);
+        }
+    }
+}
+
+static void translate_trigger(GICState *s, int irq, int cpu,
+                              uint32_t *field, bool to_kernel)
+{
+    if (to_kernel) {
+        *field = (GIC_TEST_EDGE_TRIGGER(irq)) ? 0x2 : 0x0;
+    } else {
+        if (*field & 0x2) {
+            GIC_SET_EDGE_TRIGGER(irq);
+        }
+    }
+}
+
+static void translate_priority(GICState *s, int irq, int cpu,
+                               uint32_t *field, bool to_kernel)
+{
+    if (to_kernel) {
+        *field = GIC_GET_PRIORITY(irq, cpu) & 0xff;
+    } else {
+        gic_set_priority(s, cpu, irq, *field & 0xff);
+    }
+}
+
+static void translate_targets(GICState *s, int irq, int cpu,
+                              uint32_t *field, bool to_kernel)
+{
+    if (to_kernel) {
+        *field = s->irq_target[irq] & 0xff;
+    } else {
+        s->irq_target[irq] = *field & 0xff;
+    }
+}
+
+static void translate_sgisource(GICState *s, int irq, int cpu,
+                                uint32_t *field, bool to_kernel)
+{
+    if (to_kernel) {
+        *field = s->sgi_pending[irq][cpu] & 0xff;
+    } else {
+        s->sgi_pending[irq][cpu] = *field & 0xff;
+    }
+}
+
+/* Read a register group from the kernel VGIC */
+static void kvm_dist_get(GICState *s, uint32_t offset, int width,
+                         int maxirq, vgic_translate_fn translate_fn)
+{
+    uint32_t reg;
+    int i;
+    int j;
+    int irq;
+    int cpu;
+    int regsz = 32 / width; /* irqs per kernel register */
+    uint32_t field;
+
+    for_each_irq_reg(i, maxirq, width) {
+        irq = i * regsz;
+        cpu = 0;
+        while ((cpu < s->num_cpu && irq < GIC_INTERNAL) || cpu == 0) {
+            kvm_gicd_access(s, offset, cpu, &reg, false);
+            for (j = 0; j < regsz; j++) {
+                field = extract32(reg, j * width, width);
+                translate_fn(s, irq + j, cpu, &field, false);
+            }
+
+            cpu++;
+        }
+        offset += 4;
+    }
+}
+
+/* Write a register group to the kernel VGIC */
+static void kvm_dist_put(GICState *s, uint32_t offset, int width,
+                         int maxirq, vgic_translate_fn translate_fn)
+{
+    uint32_t reg;
+    int i;
+    int j;
+    int irq;
+    int cpu;
+    int regsz = 32 / width; /* irqs per kernel register */
+    uint32_t field;
+
+    for_each_irq_reg(i, maxirq, width) {
+        irq = i * regsz;
+        cpu = 0;
+        while ((cpu < s->num_cpu && irq < GIC_INTERNAL) || cpu == 0) {
+            reg = 0;
+            for (j = 0; j < regsz; j++) {
+                translate_fn(s, irq + j, cpu, &field, true);
+                reg = deposit32(reg, j * width, width, field);
+            }
+            kvm_gicd_access(s, offset, cpu, &reg, true);
+
+            cpu++;
+        }
+        offset += 4;
+    }
+}
+
 static void kvm_arm_gic_put(GICState *s)
 {
-    /* TODO: there isn't currently a kernel interface to set the GIC state */
+    uint32_t reg;
+    int i;
+    int cpu;
+    int num_cpu;
+    int num_irq;
+
+    if (!kvm_arm_gic_can_save_restore(s)) {
+            DPRINTF("Cannot put kernel gic state, no kernel interface");
+            return;
+    }
+
+    /* Note: We do the restore in a slightly different order than the save
+     * (where the order doesn't matter and is simply ordered according to the
+     * register offset values */
+
+    /*****************************************************************
+     * Distributor State
+     */
+
+    /* s->enabled -> GICD_CTLR */
+    reg = s->enabled;
+    kvm_gicd_access(s, 0x0, 0, &reg, true);
+
+    /* Sanity checking on GICD_TYPER and s->num_irq, s->num_cpu */
+    kvm_gicd_access(s, 0x4, 0, &reg, false);
+    num_irq = ((reg & 0x1f) + 1) * 32;
+    num_cpu = ((reg & 0xe0) >> 5) + 1;
+
+    if (num_irq < s->num_irq) {
+            fprintf(stderr, "Restoring %u IRQs, but kernel supports max %d\n",
+                    s->num_irq, num_irq);
+            abort();
+    } else if (num_cpu != s->num_cpu) {
+            fprintf(stderr, "Restoring %u CPU interfaces, kernel only has %d\n",
+                    s->num_cpu, num_cpu);
+            /* Did we not create the VCPUs in the kernel yet? */
+            abort();
+    }
+
+    /* TODO: Consider checking compatibility with the IIDR ? */
+
+    /* irq_state[n].enabled -> GICD_ISENABLERn */
+    kvm_dist_put(s, 0x180, 1, s->num_irq, translate_clear);
+    kvm_dist_put(s, 0x100, 1, s->num_irq, translate_enabled);
+
+    /* s->irq_target[irq] -> GICD_ITARGETSRn
+     * (restore targets before pending to ensure the pending state is set on
+     * the appropriate CPU interfaces in the kernel) */
+    kvm_dist_put(s, 0x800, 8, s->num_irq, translate_targets);
+
+    /* irq_state[n].pending + irq_state[n].level -> GICD_ISPENDRn */
+    kvm_dist_put(s, 0x280, 1, s->num_irq, translate_clear);
+    kvm_dist_put(s, 0x200, 1, s->num_irq, translate_pending);
+
+    /* irq_state[n].active -> GICD_ISACTIVERn */
+    kvm_dist_put(s, 0x380, 1, s->num_irq, translate_clear);
+    kvm_dist_put(s, 0x300, 1, s->num_irq, translate_active);
+
+    /* irq_state[n].trigger -> GICD_ICFRn */
+    kvm_dist_put(s, 0xc00, 2, s->num_irq, translate_trigger);
+
+    /* s->priorityX[irq] -> ICD_IPRIORITYRn */
+    kvm_dist_put(s, 0x400, 8, s->num_irq, translate_priority);
+
+    /* s->sgi_pending -> ICD_CPENDSGIRn */
+    kvm_dist_put(s, 0xf10, 8, GIC_NR_SGIS, translate_clear);
+    kvm_dist_put(s, 0xf20, 8, GIC_NR_SGIS, translate_sgisource);
+
+
+    /*****************************************************************
+     * CPU Interface(s) State
+     */
+
+    for (cpu = 0; cpu < s->num_cpu; cpu++) {
+        /* s->cpu_enabled[cpu] -> GICC_CTLR */
+        reg = s->cpu_enabled[cpu];
+        kvm_gicc_access(s, 0x00, cpu, &reg, true);
+
+        /* s->priority_mask[cpu] -> GICC_PMR */
+        reg = (s->priority_mask[cpu] & 0xff);
+        kvm_gicc_access(s, 0x04, cpu, &reg, true);
+
+        /* s->bpr[cpu] -> GICC_BPR */
+        reg = (s->bpr[cpu] & 0x7);
+        kvm_gicc_access(s, 0x08, cpu, &reg, true);
+
+        /* s->abpr[cpu] -> GICC_ABPR */
+        reg = (s->abpr[cpu] & 0x7);
+        kvm_gicc_access(s, 0x1c, cpu, &reg, true);
+
+        /* s->apr[n][cpu] -> GICC_APRn */
+        for (i = 0; i < 4; i++) {
+            reg = s->apr[i][cpu];
+            kvm_gicc_access(s, 0xd0 + i * 4, cpu, &reg, true);
+        }
+    }
 }
 
 static void kvm_arm_gic_get(GICState *s)
 {
-    /* TODO: there isn't currently a kernel interface to get the GIC state */
+    uint32_t reg;
+    int i;
+    int cpu;
+
+    if (!kvm_arm_gic_can_save_restore(s)) {
+            DPRINTF("Cannot get kernel gic state, no kernel interface");
+            return;
+    }
+
+    /*****************************************************************
+     * Distributor State
+     */
+
+    /* GICD_CTLR -> s->enabled */
+    kvm_gicd_access(s, 0x0, 0, &reg, false);
+    s->enabled = reg & 1;
+
+    /* Sanity checking on GICD_TYPER -> s->num_irq, s->num_cpu */
+    kvm_gicd_access(s, 0x4, 0, &reg, false);
+    s->num_irq = ((reg & 0x1f) + 1) * 32;
+    s->num_cpu = ((reg & 0xe0) >> 5) + 1;
+
+    if (s->num_irq > GIC_MAXIRQ) {
+            fprintf(stderr, "Too many IRQs reported from the kernel: %d\n",
+                    s->num_irq);
+            abort();
+    }
+
+    /* GICD_IIDR -> ? */
+    kvm_gicd_access(s, 0x8, 0, &reg, false);
+
+    /* Verify no GROUP 1 interrupts configured in the kernel */
+    for_each_irq_reg(i, s->num_irq, 1) {
+        kvm_gicd_access(s, 0x80 + (i * 4), 0, &reg, false);
+        if (reg != 0) {
+            fprintf(stderr, "Unsupported GICD_IGROUPRn value: %08x\n",
+                    reg);
+            abort();
+        }
+    }
+
+    /* Clear all the IRQ settings */
+    for (i = 0; i < s->num_irq; i++) {
+        memset(&s->irq_state[i], 0, sizeof(s->irq_state[0]));
+    }
+
+    /* GICD_ISENABLERn -> irq_state[n].enabled */
+    kvm_dist_get(s, 0x100, 1, s->num_irq, translate_enabled);
+
+    /* GICD_ISPENDRn -> irq_state[n].pending + irq_state[n].level */
+    kvm_dist_get(s, 0x200, 1, s->num_irq, translate_pending);
+
+    /* GICD_ISACTIVERn -> irq_state[n].active */
+    kvm_dist_get(s, 0x300, 1, s->num_irq, translate_active);
+
+    /* GICD_ICFRn -> irq_state[n].trigger */
+    kvm_dist_get(s, 0xc00, 2, s->num_irq, translate_trigger);
+
+    /* GICD_IPRIORITYRn -> s->priorityX[irq] */
+    kvm_dist_get(s, 0x400, 8, s->num_irq, translate_priority);
+
+    /* GICD_ITARGETSRn -> s->irq_target[irq] */
+    kvm_dist_get(s, 0x800, 8, s->num_irq, translate_targets);
+
+    /* GICD_CPENDSGIRn -> s->sgi_pending */
+    kvm_dist_get(s, 0xf10, 8, GIC_NR_SGIS, translate_sgisource);
+
+
+    /*****************************************************************
+     * CPU Interface(s) State
+     */
+
+    for (cpu = 0; cpu < s->num_cpu; cpu++) {
+        /* GICC_CTLR -> s->cpu_enabled[cpu] */
+        kvm_gicc_access(s, 0x00, cpu, &reg, false);
+        s->cpu_enabled[cpu] = (reg & 1);
+
+        /* GICC_PMR -> s->priority_mask[cpu] */
+        kvm_gicc_access(s, 0x04, cpu, &reg, false);
+        s->priority_mask[cpu] = (reg & 0xff);
+
+        /* GICC_BPR -> s->bpr[cpu] */
+        kvm_gicc_access(s, 0x08, cpu, &reg, false);
+        s->bpr[cpu] = (reg & 0x7);
+
+        /* GICC_ABPR -> s->abpr[cpu] */
+        kvm_gicc_access(s, 0x1c, cpu, &reg, false);
+        s->abpr[cpu] = (reg & 0x7);
+
+        /* GICC_APRn -> s->apr[n][cpu] */
+        for (i = 0; i < 4; i++) {
+            kvm_gicc_access(s, 0xd0 + i * 4, cpu, &reg, false);
+            s->apr[i][cpu] = reg;
+        }
+    }
 }
 
 static void kvm_arm_gic_reset(DeviceState *dev)
-- 
1.8.5.2

Re: [Qemu-devel] [PATCH v5 1/8] arm_gic: Introduce define for GIC_NR_SGIS

[Peter Maydell]

On 28 January 2014 20:32, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Instead of hardcoding 16 various places in the code, use a define to
> make it more clear what is going on.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 2/8] arm_gic: Fix GICD_ICPENDR and GICD_ISPENDR writes

[Peter Maydell]

On 28 January 2014 20:32, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Fix two bugs that would allow changing the state of SGIs through the
> ICPENDR and ISPENDRs.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 0/8] Support arm-gic-kvm save/restore

[Peter Maydell]

On 28 January 2014 20:32, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Implement support to save/restore the ARM KVM VGIC state from the
> kernel.  The basic appraoch is to transfer state from the in-kernel VGIC
> to the emulated arm-gic state representation and let the standard QEMU
> vmstate save/restore handle saving the arm-gic state.  Restore works by
> reversing the process.

> Christoffer Dall (8):
>   arm_gic: Introduce define for GIC_NR_SGIS
>   arm_gic: Fix GICD_ICPENDR and GICD_ISPENDR writes
>   arm_gic: Fix GIC pending behavior
>   hw: arm_gic: Keep track of SGI sources
>   arm_gic: Support setting/getting binary point reg
>   vmstate: Add uint32 2D-array support
>   arm_gic: Add GICC_APRn state to the GICState
>   hw: arm_gic_kvm: Add KVM VGIC save/restore logic

I've applied the first two of these to target-arm.next since they're
so simple, and I might as well put them in to the pull request
I'm going to send out shortly. The rest I need a little more time
to review.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 3/8] arm_gic: Fix GIC pending behavior

[Peter Maydell]

On 28 January 2014 20:32, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> The existing implementation of the pending behavior in gic_set_irq,
> gic_acknowledge_irq, gic_complete_irq, and the distributor pending
> set/clear registers does not follow the semantics of the GICv2.0 specs,
> but may implement the 11MPCore support.  Therefore, maintain the
> existing semantics for 11MPCore and v7M NVIC and change the behavior to
> be in accordance with the GICv2.0 specs for "generic implementations"
> (s->revision == 1 || s->revision == 2).
>
> Generic implementations distinguish between setting a level-triggered
> interrupt pending through writes to the GICD_ISPENDR and when hardware
> raises the interrupt line.  Writing to the GICD_ICPENDR will not cause
> the interrupt to become non-pending if the line is still active, and
> conversely, if the line is deactivated but the interrupt is marked as
> pending through a write to GICD_ISPENDR, the interrupt remains pending.
> Handle this situation in the GIC_TEST_PENDING (which now becomes a
> static inline named gic_test_pending) and let the 'pending' field
> correspond only to the latched state of the D-flip flop in the GICv2.0
> specs Figure 4-10.
>
> The following changes are added:
>
> gic_test_pending:
> Make this a static inline and split out the 11MPCore from the generic
> behavior.  For the generic behavior, consider interrupts pending if:
>     ((s->irq_state[irq].pending & (cm) != 0) ||
>        (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm))
>
> gic_set_irq:
> Split out the 11MPCore from the generic behavior.  For the generic
> behavior, always GIC_SET_LEVEL(irq, 1) on positive level, but only
> GIC_SET_PENDING for edge-triggered interrupts and vice versa on a
> negative level.
>
> gic_complete_irq:
> Only resample the line for line-triggered interrupts on an 11MPCore.
> Generic implementations will sample the line directly in
> gic_test_pending().
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

This looks broadly right; a couple of comments below.

> ---
> Changes [v4 -> v5]:
>  - Factor out GIC_NR_SGIS and gic_dist_writeb bugfixes
>  - Change meaning of pending field for level-triggered interrupts for
>    GIC v1/v2, to only capture manually written state to pending
>    registers.  Add or-clause to gic_test_pending to sample the line
>    state, as per Peter's suggestions:
>    https://lists.cs.columbia.edu/pipermail/kvmarm/2014-January/008798.html
>
> Changes [v3 -> v4]:
>  - Maintain 11MPCore semantics
>  - Combine all pending interrupts fixing patches into this patch.  See
>    the detailed description above.
>
> Changes [v1 -> v2]:
>  - Fix bisection issue, by not using gic_clear_pending yet.
>
>  hw/intc/arm_gic.c      | 74 ++++++++++++++++++++++++++++++++++++--------------
>  hw/intc/gic_internal.h | 16 ++++++++++-
>  2 files changed, 69 insertions(+), 21 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 1c4a114..5e2cf14 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -66,7 +66,7 @@ void gic_update(GICState *s)
>          best_prio = 0x100;
>          best_irq = 1023;
>          for (irq = 0; irq < s->num_irq; irq++) {
> -            if (GIC_TEST_ENABLED(irq, cm) && GIC_TEST_PENDING(irq, cm)) {
> +            if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, cm)) {
>                  if (GIC_GET_PRIORITY(irq, cpu) < best_prio) {
>                      best_prio = GIC_GET_PRIORITY(irq, cpu);
>                      best_irq = irq;
> @@ -89,14 +89,46 @@ void gic_set_pending_private(GICState *s, int cpu, int irq)
>  {
>      int cm = 1 << cpu;
>
> -    if (GIC_TEST_PENDING(irq, cm))
> +    if (gic_test_pending(s, irq, cm)) {
>          return;
> +    }
>
>      DPRINTF("Set %d pending cpu %d\n", irq, cpu);
>      GIC_SET_PENDING(irq, cm);
>      gic_update(s);
>  }
>
> +static void gic_set_irq_11mpcore(GICState *s, int irq, int level,
> +                                 int cm, int target)
> +{
> +    if (level) {
> +        GIC_SET_LEVEL(irq, cm);
> +        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
> +            DPRINTF("Set %d pending mask %x\n", irq, target);
> +            GIC_SET_PENDING(irq, target);
> +        }
> +    } else {
> +        GIC_CLEAR_LEVEL(irq, cm);
> +    }
> +}
> +
> +static void gic_set_irq_generic(GICState *s, int irq, int level,
> +                                int cm, int target)
> +{
> +    if (level) {
> +        GIC_SET_LEVEL(irq, cm);
> +        DPRINTF("Set %d pending mask %x\n", irq, target);
> +        if (GIC_TEST_EDGE_TRIGGER(irq)) {
> +            GIC_SET_PENDING(irq, target);
> +        }
> +    } else {
> +        if (GIC_TEST_EDGE_TRIGGER(irq)) {
> +            GIC_CLEAR_PENDING(irq, target);
> +        }

This doesn't look right. A falling edge for an edge triggered
interrupt should just CLEAR_LEVEL and leave the state of the
pending latch alone.

> +        GIC_CLEAR_LEVEL(irq, cm);
> +    }
> +}
> +
>  /* Process a change in an external IRQ input.  */
>  static void gic_set_irq(void *opaque, int irq, int level)
>  {
> @@ -126,15 +158,12 @@ static void gic_set_irq(void *opaque, int irq, int level)
>          return;
>      }
>
> -    if (level) {
> -        GIC_SET_LEVEL(irq, cm);
> -        if (GIC_TEST_EDGE_TRIGGER(irq) || GIC_TEST_ENABLED(irq, cm)) {
> -            DPRINTF("Set %d pending mask %x\n", irq, target);
> -            GIC_SET_PENDING(irq, target);
> -        }
> +    if (s->revision == REV_11MPCORE) {

Or NVIC.

> +        gic_set_irq_11mpcore(s, irq, level, cm, target);
>      } else {
> -        GIC_CLEAR_LEVEL(irq, cm);
> +        gic_set_irq_generic(s, irq, level, cm, target);
>      }
> +
>      gic_update(s);
>  }
>
> @@ -160,9 +189,10 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
>          return 1023;
>      }
>      s->last_active[new_irq][cpu] = s->running_irq[cpu];
> -    /* Clear pending flags for both level and edge triggered interrupts.
> -       Level triggered IRQs will be reasserted once they become inactive.  */
> -    GIC_CLEAR_PENDING(new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm);
> +
> +    cm = GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm;
> +    GIC_CLEAR_PENDING(new_irq, cm);
> +

This assignment to cm ends up changing behaviour of following
code for 11mpcore/NVIC, I think. It looks to me like you can
just drop this hunk.

>      gic_set_running_irq(s, cpu, new_irq);
>      DPRINTF("ACK %d\n", new_irq);
>      return new_irq;
> @@ -195,14 +225,18 @@ void gic_complete_irq(GICState *s, int cpu, int irq)
>      }
>      if (s->running_irq[cpu] == 1023)
>          return; /* No active IRQ.  */
> -    /* Mark level triggered interrupts as pending if they are still
> -       raised.  */
> -    if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
> -        && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) {
> -        DPRINTF("Set %d pending mask %x\n", irq, cm);
> -        GIC_SET_PENDING(irq, cm);
> -        update = 1;
> +
> +    if (s->revision == REV_11MPCORE) {

Or NVIC.

> +        /* Mark level triggered interrupts as pending if they are still
> +           raised.  */
> +        if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
> +            && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) {
> +            DPRINTF("Set %d pending mask %x\n", irq, cm);
> +            GIC_SET_PENDING(irq, cm);
> +            update = 1;
> +        }
>      }
> +
>      if (irq != s->running_irq[cpu]) {
>          /* Complete an IRQ that is not currently running.  */
>          int tmp = s->running_irq[cpu];
> @@ -273,7 +307,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
>          res = 0;
>          mask = (irq < GIC_INTERNAL) ?  cm : ALL_CPU_MASK;
>          for (i = 0; i < 8; i++) {
> -            if (GIC_TEST_PENDING(irq + i, mask)) {
> +            if (gic_test_pending(s, irq + i, mask)) {
>                  res |= (1 << i);
>              }
>          }
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index 8c02d58..92a6f7a 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -34,7 +34,6 @@
>  #define GIC_TEST_ENABLED(irq, cm) ((s->irq_state[irq].enabled & (cm)) != 0)
>  #define GIC_SET_PENDING(irq, cm) s->irq_state[irq].pending |= (cm)
>  #define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm)
> -#define GIC_TEST_PENDING(irq, cm) ((s->irq_state[irq].pending & (cm)) != 0)
>  #define GIC_SET_ACTIVE(irq, cm) s->irq_state[irq].active |= (cm)
>  #define GIC_CLEAR_ACTIVE(irq, cm) s->irq_state[irq].active &= ~(cm)
>  #define GIC_TEST_ACTIVE(irq, cm) ((s->irq_state[irq].active & (cm)) != 0)
> @@ -63,4 +62,19 @@ void gic_update(GICState *s);
>  void gic_init_irqs_and_distributor(GICState *s, int num_irq);
>  void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
>
> +static inline bool gic_test_pending(GICState *s, int irq, int cm)
> +{
> +    if (s->revision == REV_NVIC || s->revision == REV_11MPCORE) {
> +        return s->irq_state[irq].pending & cm;
> +    } else {
> +        /* Edge-triggered interrupts are marked pending on a rising edge, but
> +         * level-triggered interrupts are either considered pending when the
> +         * level is active or if software has explicitly written to
> +         * GICD_ISPENDR to set the state pending.
> +         */
> +        return (s->irq_state[irq].pending & cm) ||
> +            (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm));
> +    }
> +}
> +
>  #endif /* !QEMU_ARM_GIC_INTERNAL_H */

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 4/8] hw: arm_gic: Keep track of SGI sources

[Peter Maydell]

On 28 January 2014 20:32, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> Right now the arm gic emulation doesn't keep track of the source of an
> SGI (which apparently Linux guests don't use, or they're fine with
> assuming CPU 0 always).
>
> Add the necessary matrix on the GICState structure and maintain the data
> when setting and clearing the pending state of an IRQ and make the state
> visible to the guest.
>
> Note that we always choose to present the source as the lowest-numbered
> CPU in case multiple cores have signalled the same SGI number to a core
> on the system.

> @@ -531,9 +576,29 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>                  GIC_CLEAR_EDGE_TRIGGER(irq + i);
>              }
>          }
> -    } else {
> +    } else if (offset < 0xf10) {
>          /* 0xf00 is only handled for 32-bit writes.  */
>          goto bad_reg;
> +    } else if (offset < 0xf20) {
> +        /* GICD_CPENDSGIRn */
> +        if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
> +            goto bad_reg;
> +        }
> +        irq = (offset - 0xf10);
> +
> +        GIC_CLEAR_PENDING(irq, 1 << cpu);
> +        s->sgi_pending[irq][cpu] &= ~value;

This doesn't look quite right. If the SGI is pending
from multiple source CPUs and we use CPENDSGIRn to
clear the bits corresponding to only some of those
source CPUs, then the interrupt as a whole should stay
pending on this (target) CPU. I think this is:

    s->sgi_pending[irq][cpu] &= ~value;
    if (s->sgi_pending[irq][cpu] == 0) {
        GIC_CLEAR_PENDING(irq, 1 << cpu);
    }

(compare the code in gic_acknowledge_irq())

If you fix that, then
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v5 4/8] hw: arm_gic: Keep track of SGI sources

[Christoffer Dall]

On Fri, Jan 31, 2014 at 06:33:25PM +0000, Peter Maydell wrote:
> On 28 January 2014 20:32, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > Right now the arm gic emulation doesn't keep track of the source of an
> > SGI (which apparently Linux guests don't use, or they're fine with
> > assuming CPU 0 always).
> >
> > Add the necessary matrix on the GICState structure and maintain the data
> > when setting and clearing the pending state of an IRQ and make the state
> > visible to the guest.
> >
> > Note that we always choose to present the source as the lowest-numbered
> > CPU in case multiple cores have signalled the same SGI number to a core
> > on the system.
> 
> > @@ -531,9 +576,29 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
> >                  GIC_CLEAR_EDGE_TRIGGER(irq + i);
> >              }
> >          }
> > -    } else {
> > +    } else if (offset < 0xf10) {
> >          /* 0xf00 is only handled for 32-bit writes.  */
> >          goto bad_reg;
> > +    } else if (offset < 0xf20) {
> > +        /* GICD_CPENDSGIRn */
> > +        if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
> > +            goto bad_reg;
> > +        }
> > +        irq = (offset - 0xf10);
> > +
> > +        GIC_CLEAR_PENDING(irq, 1 << cpu);
> > +        s->sgi_pending[irq][cpu] &= ~value;
> 
> This doesn't look quite right. If the SGI is pending
> from multiple source CPUs and we use CPENDSGIRn to
> clear the bits corresponding to only some of those
> source CPUs, then the interrupt as a whole should stay
> pending on this (target) CPU. I think this is:
> 
>     s->sgi_pending[irq][cpu] &= ~value;
>     if (s->sgi_pending[irq][cpu] == 0) {
>         GIC_CLEAR_PENDING(irq, 1 << cpu);
>     }

I had this vague feeling that it was too easy when I wrote the code,
nice catch!

> 
> (compare the code in gic_acknowledge_irq())
> 
> If you fix that, then
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 

Thanks!
-Christoffer

Re: [Qemu-devel] [PATCH v5 3/8] arm_gic: Fix GIC pending behavior

[Christoffer Dall]

On Fri, Jan 31, 2014 at 06:09:20PM +0000, Peter Maydell wrote:
> On 28 January 2014 20:32, Christoffer Dall <christoffer.dall@linaro.org> wrote:

[...]

> 
> This looks broadly right; a couple of comments below.
> 

[...]

> > +
> > +static void gic_set_irq_generic(GICState *s, int irq, int level,
> > +                                int cm, int target)
> > +{
> > +    if (level) {
> > +        GIC_SET_LEVEL(irq, cm);
> > +        DPRINTF("Set %d pending mask %x\n", irq, target);
> > +        if (GIC_TEST_EDGE_TRIGGER(irq)) {
> > +            GIC_SET_PENDING(irq, target);
> > +        }
> > +    } else {
> > +        if (GIC_TEST_EDGE_TRIGGER(irq)) {
> > +            GIC_CLEAR_PENDING(irq, target);
> > +        }
> 
> This doesn't look right. A falling edge for an edge triggered
> interrupt should just CLEAR_LEVEL and leave the state of the
> pending latch alone.
> 

duh, yeah, thanks for spotting.

[...]

Thanks,
-Christoffer