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

[Qemu-devel] [RFC PATCH v4 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-v3

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-v4

Changes [v3 -> v4]:
 - Changes are described in the individual patches
 - Some re-ordering of patches: the rename of trigger to edge_trigger is
   now the first patch because it makes subsequent patches easier to
   read
 - All the changes related to handling the pending state are now
   combined in patch 3.
 - Generally tried to address review comments from v3

Changes [v2 -> v3]:
 - Changes are described in the individual patches
 - New patch that renames GIC_..._TRIGGER to GIC_..._EDGE_TRIGGER
 - New patch to keep track of software writing to SPENDR and CPENDR
   registers.
 - New patch that fixes not clearing the pending state for an asserted
   level-triggered interrupt on ACK
 - Separate patch for the GICC_APRn state

Changes [v1 -> v2]:
 - Changes are described in the individual patches
 - VMState additions has been split into a separate patch

Christoffer Dall (8):
  arm_gic: Rename GIC_X_TRIGGER to GIC_X_EDGE_TRIGGER
  hw: arm_gic: Introduce gic_set_priority function
  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                | 186 +++++++++++++----
 hw/intc/arm_gic_common.c         |  17 +-
 hw/intc/arm_gic_kvm.c            | 424 ++++++++++++++++++++++++++++++++++++++-
 hw/intc/gic_internal.h           |  10 +-
 include/hw/intc/arm_gic_common.h |  29 ++-
 include/migration/vmstate.h      |   6 +
 6 files changed, 617 insertions(+), 55 deletions(-)

-- 
1.8.5

[Qemu-devel] [RFC PATCH v4 1/8] arm_gic: Rename GIC_X_TRIGGER to GIC_X_EDGE_TRIGGER

[Christoffer Dall]

TRIGGER can really mean mean anything (e.g. was it triggered, is it
level-triggered, is it edge-triggered, etc.).  Rename to EDGE_TRIGGER to
make the code comprehensible without looking up the data structure.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
Changes [v3 -> v4]:
 - Rename struct field on GICState itself

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

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index d431b7a..27c258a 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -128,7 +128,7 @@ static void gic_set_irq(void *opaque, int irq, int level)
 
     if (level) {
         GIC_SET_LEVEL(irq, cm);
-        if (GIC_TEST_TRIGGER(irq) || GIC_TEST_ENABLED(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);
         }
@@ -188,7 +188,7 @@ void gic_complete_irq(GICState *s, int cpu, int irq)
         return; /* No active IRQ.  */
     /* Mark level triggered interrupts as pending if they are still
        raised.  */
-    if (!GIC_TEST_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
+    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);
@@ -311,7 +311,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
         for (i = 0; i < 4; i++) {
             if (GIC_TEST_MODEL(irq + i))
                 res |= (1 << (i * 2));
-            if (GIC_TEST_TRIGGER(irq + i))
+            if (GIC_TEST_EDGE_TRIGGER(irq + i))
                 res |= (2 << (i * 2));
         }
     } else if (offset < 0xfe0) {
@@ -386,7 +386,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
                 /* If a raised level triggered IRQ enabled then mark
                    is as pending.  */
                 if (GIC_TEST_LEVEL(irq + i, mask)
-                        && !GIC_TEST_TRIGGER(irq + i)) {
+                        && !GIC_TEST_EDGE_TRIGGER(irq + i)) {
                     DPRINTF("Set %d pending mask %x\n", irq + i, mask);
                     GIC_SET_PENDING(irq + i, mask);
                 }
@@ -478,9 +478,9 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
                 GIC_CLEAR_MODEL(irq + i);
             }
             if (value & (2 << (i * 2))) {
-                GIC_SET_TRIGGER(irq + i);
+                GIC_SET_EDGE_TRIGGER(irq + i);
             } else {
-                GIC_CLEAR_TRIGGER(irq + i);
+                GIC_CLEAR_EDGE_TRIGGER(irq + i);
             }
         }
     } else {
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index c765850..710607b 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -51,7 +51,7 @@ static const VMStateDescription vmstate_gic_irq_state = {
         VMSTATE_UINT8(active, gic_irq_state),
         VMSTATE_UINT8(level, gic_irq_state),
         VMSTATE_BOOL(model, gic_irq_state),
-        VMSTATE_BOOL(trigger, gic_irq_state),
+        VMSTATE_BOOL(edge_trigger, gic_irq_state),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -126,7 +126,7 @@ static void arm_gic_common_reset(DeviceState *dev)
     }
     for (i = 0; i < 16; i++) {
         GIC_SET_ENABLED(i, ALL_CPU_MASK);
-        GIC_SET_TRIGGER(i);
+        GIC_SET_EDGE_TRIGGER(i);
     }
     if (s->num_cpu == 1) {
         /* For uniprocessor GICs all interrupts always target the sole CPU */
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 3989fd1..efac78d 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -44,9 +44,9 @@
 #define GIC_SET_LEVEL(irq, cm) s->irq_state[irq].level = (cm)
 #define GIC_CLEAR_LEVEL(irq, cm) s->irq_state[irq].level &= ~(cm)
 #define GIC_TEST_LEVEL(irq, cm) ((s->irq_state[irq].level & (cm)) != 0)
-#define GIC_SET_TRIGGER(irq) s->irq_state[irq].trigger = true
-#define GIC_CLEAR_TRIGGER(irq) s->irq_state[irq].trigger = false
-#define GIC_TEST_TRIGGER(irq) s->irq_state[irq].trigger
+#define GIC_SET_EDGE_TRIGGER(irq) s->irq_state[irq].edge_trigger = true
+#define GIC_CLEAR_EDGE_TRIGGER(irq) s->irq_state[irq].edge_trigger = false
+#define GIC_TEST_EDGE_TRIGGER(irq) (s->irq_state[irq].edge_trigger)
 #define GIC_GET_PRIORITY(irq, cpu) (((irq) < GIC_INTERNAL) ?            \
                                     s->priority1[irq][cpu] :            \
                                     s->priority2[(irq) - GIC_INTERNAL])
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index ed18bb8..40cd3d6 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -37,7 +37,7 @@ typedef struct gic_irq_state {
     uint8_t active;
     uint8_t level;
     bool model; /* 0 = N:N, 1 = 1:N */
-    bool trigger; /* nonzero = edge triggered.  */
+    bool edge_trigger; /* true: edge-triggered, false: level-triggered  */
 } gic_irq_state;
 
 typedef struct GICState {
-- 
1.8.5

[Qemu-devel] [RFC PATCH v4 2/8] hw: arm_gic: Introduce gic_set_priority function

[Christoffer Dall]

To make the code slightly cleaner to look at and make the save/restore
code easier to understand, introduce this function to set the priority of
interrupts.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 hw/intc/arm_gic.c      | 15 ++++++++++-----
 hw/intc/gic_internal.h |  1 +
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 27c258a..6c59650 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -168,6 +168,15 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
     return new_irq;
 }
 
+void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
+{
+    if (irq < GIC_INTERNAL) {
+        s->priority1[irq][cpu] = val;
+    } else {
+        s->priority2[(irq) - GIC_INTERNAL] = val;
+    }
+}
+
 void gic_complete_irq(GICState *s, int cpu, int irq)
 {
     int update = 0;
@@ -443,11 +452,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         irq = (offset - 0x400) + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        if (irq < GIC_INTERNAL) {
-            s->priority1[irq][cpu] = value;
-        } else {
-            s->priority2[irq - GIC_INTERNAL] = value;
-        }
+        gic_set_priority(s, cpu, irq, value);
     } else if (offset < 0xc00) {
         /* Interrupt CPU Target. RAZ/WI on uniprocessor GICs, with the
          * annoying exception of the 11MPCore's GIC.
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index efac78d..8c02d58 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -61,5 +61,6 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu);
 void gic_complete_irq(GICState *s, int cpu, int irq);
 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);
 
 #endif /* !QEMU_ARM_GIC_INTERNAL_H */
-- 
1.8.5

[Qemu-devel] [RFC PATCH v4 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 change the behavior to be in
accordance with the GICv2.0 specs for "generic implementations"
(s->revision == 1 || s->revision == 2).

Generic implementations also mandate that level-triggered interrupts
that are pending due to a write to GICD_ISPENDR stay pending until the
interrupt is acknowledged or until there's a write to the GICD_ICPENDR.
Similarly, only clear the pending state on writes to GICD_ICPENDR if the
interrupt signal is not asserted.  This requires an extra state variable
to keep track of writes to the set/clear registers, corresponding to the
D-flip flop in the GICv2.0 specs Figure 4-10.

The following changes are added:

gic_set_irq:
Split out the 11MPCore from the generic behavior.  For the generic
behavior, simply set pending state on positive level and edge triggered
interrupts and clear the pending state only for level triggered
interrupts.

gic_acknowledge_irq:
Keep existing functionality for 11MPCore, but only clear the pending
state if the interrupt is edge-triggered or if the interrupt is
non-asserted and level-triggered.

gic_complete_irq:
Only resample the line for line-triggered interrupts on an 11MPCore.
Generic implementations will latch the pending state when the level
changes, on reads of GICC_IAR, and on wrtites to GICD_I{SC}PENDRn.

gic_dist_writeb:
1. Fix bugs to ensure that writes to GICD_ICPENDR and GICD_ISPENDR are
   ignored for SGIs (common behavior for 11MPCore and generic
   implementations).
2. Do not clear the pending state for level-triggered interrupts on
   writes to GICD_ICPENDR if the interrupt signal remains asserted.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
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                | 106 ++++++++++++++++++++++++++++++---------
 hw/intc/arm_gic_common.c         |   5 +-
 hw/intc/gic_internal.h           |   3 ++
 include/hw/intc/arm_gic_common.h |   2 +
 4 files changed, 91 insertions(+), 25 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 6c59650..dd76e1d 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -97,6 +97,38 @@ void gic_set_pending_private(GICState *s, int cpu, int irq)
     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);
+        GIC_SET_PENDING(irq, target);
+    } else {
+        /* Clear level-triggered interrupts that have not been set to pending
+         * through GICD_SPENDR.
+         */
+        if (!GIC_TEST_EDGE_TRIGGER(irq) && !GIC_TEST_SW_PENDING(irq, cm)) {
+            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,23 @@ 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_SW_PENDING(new_irq, 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(new_irq, cm);
+    } else {
+        /* Clear pending flags for edge-triggered and non-asserted
+         * level-triggered interrupts
+         */
+        if (GIC_TEST_EDGE_TRIGGER(new_irq) || !GIC_TEST_LEVEL(new_irq, cpu)) {
+            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 +238,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];
@@ -423,25 +470,38 @@ 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)
+          value = 0;
 
-        for (i = 0; i < 8; i++) {
+        for (i = 0; i < 8; i++, irq++) {
             if (value & (1 << i)) {
-                GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
+                GIC_SET_PENDING(irq, GIC_TARGET(irq));
+                GIC_SET_SW_PENDING(irq, GIC_TARGET(irq));
             }
         }
     } else if (offset < 0x300) {
+        int cm = (1 << cpu);
         /* Interrupt Clear Pending.  */
         irq = (offset - 0x280) * 8 + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        for (i = 0; i < 8; i++) {
+        if (irq < GIC_NR_SGIS)
+            value = 0;
+
+        for (i = 0; i < 8; i++, irq++) {
             /* ??? 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);
+                GIC_CLEAR_SW_PENDING(irq, cm);
+                if (s->revision == REV_11MPCORE) {
+                    GIC_CLEAR_PENDING(irq, ALL_CPU_MASK);
+                } else {
+                    if (GIC_TEST_EDGE_TRIGGER(irq) ||
+                        !GIC_TEST_LEVEL(irq, cm)) {
+                        GIC_CLEAR_PENDING(irq, ALL_CPU_MASK);
+                    }
+                }
             }
         }
     } else if (offset < 0x400) {
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 710607b..8c7621a 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -43,11 +43,12 @@ static int gic_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_gic_irq_state = {
     .name = "arm_gic_irq_state",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(enabled, gic_irq_state),
         VMSTATE_UINT8(pending, gic_irq_state),
+        VMSTATE_UINT8(sw_pending, gic_irq_state),
         VMSTATE_UINT8(active, gic_irq_state),
         VMSTATE_UINT8(level, gic_irq_state),
         VMSTATE_BOOL(model, gic_irq_state),
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 8c02d58..3ebe4dc 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -35,6 +35,9 @@
 #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_SW_PENDING(irq, cm) (s->irq_state[irq].sw_pending |= (cm))
+#define GIC_CLEAR_SW_PENDING(irq, cm) (s->irq_state[irq].sw_pending &= ~(cm))
+#define GIC_TEST_SW_PENDING(irq, cm) ((s->irq_state[irq].sw_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)
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 40cd3d6..126e122 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
 
@@ -34,6 +35,7 @@ typedef struct gic_irq_state {
     /* The enable bits are only banked for per-cpu interrupts.  */
     uint8_t enabled;
     uint8_t pending;
+    uint8_t sw_pending; /* keep track of GICD_ISPENDR and GICD_ICPENDR writes */
     uint8_t active;
     uint8_t level;
     bool model; /* 0 = N:N, 1 = 1:N */
-- 
1.8.5

[Qemu-devel] [RFC PATCH v4 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.

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 [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                | 56 ++++++++++++++++++++++++++++------------
 hw/intc/arm_gic_common.c         |  5 ++--
 include/hw/intc/arm_gic_common.h |  7 +++++
 3 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index dd76e1d..d0ccb2a 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,35 +182,50 @@ 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_SW_PENDING(new_irq, cm);
+    cm = GIC_TEST_MODEL(irq) ? ALL_CPU_MASK : cm;
+    GIC_CLEAR_SW_PENDING(irq, 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(new_irq, cm);
+        GIC_CLEAR_PENDING(irq, cm);
+        ret = irq;
     } else {
-        /* Clear pending flags for edge-triggered and non-asserted
-         * level-triggered interrupts
-         */
-        if (GIC_TEST_EDGE_TRIGGER(new_irq) || !GIC_TEST_LEVEL(new_irq, cpu)) {
-            GIC_CLEAR_PENDING(new_irq, cm);
+        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 flags for edge-triggered and non-asserted
+             * level-triggered interrupts
+             */
+            if (GIC_TEST_EDGE_TRIGGER(irq) || !GIC_TEST_LEVEL(irq, cpu)) {
+                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)
@@ -574,6 +591,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;
@@ -593,6 +611,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 8c7621a..4647f0d 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -59,8 +59,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[]) {
@@ -72,6 +72,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 126e122..59a005e 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -56,6 +56,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

[Qemu-devel] [RFC PATCH v4 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 d0ccb2a..7ea869a 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -640,14 +640,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);
@@ -666,10 +667,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 4647f0d..ca12f06 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -59,8 +59,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[]) {
@@ -77,6 +77,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 59a005e..f37bf69 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -69,6 +69,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

[Qemu-devel] [RFC PATCH v4 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 9d09e60..c44e3b5 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -646,9 +646,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

[Qemu-devel] [RFC PATCH v4 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.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
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 | 11 +++++++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 7ea869a..5973bdb 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -649,6 +649,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);
@@ -676,6 +678,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 ca12f06..f9a7a0a 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -59,8 +59,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[]) {
@@ -79,6 +79,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 f37bf69..1c8bb2a 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;
@@ -76,6 +79,14 @@ 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.
+     */
+    uint32_t apr[GIC_NR_APRS][GIC_NCPU];
+
     uint32_t num_cpu;
 
     MemoryRegion iomem; /* Distributor */
-- 
1.8.5

[Qemu-devel] [RFC PATCH v4 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>
---
Changelog [v3]:
 - Separate patch for adding the APR register state

Changelog [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 158f047..517c147 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(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

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

[Peter Maydell]

On 21 December 2013 06:09, 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.
>
> The first patches adds missing features and fixes issues with the
> arm-gic implementation in qemu in preparation for the actual
> save/restore logic.

I still need to review this series, but I'm going to take the
first two patches into target-arm.next now since they're already
reviewed and they're straightforward cleanup.

thanks
-- PMM

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

[Christoffer Dall]

On Mon, Jan 06, 2014 at 11:09:28AM +0000, Peter Maydell wrote:
> On 21 December 2013 06:09, 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.
> >
> > The first patches adds missing features and fixes issues with the
> > arm-gic implementation in qemu in preparation for the actual
> > save/restore logic.
> 
> I still need to review this series, but I'm going to take the
> first two patches into target-arm.next now since they're already
> reviewed and they're straightforward cleanup.
> 
ok, no worries about the review.  The kernel series went into Marcelo's
queue over the holiday, so I expect it to appear in kvm.next any time
soon.

-Christoffer

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

[Peter Maydell]

On 21 December 2013 06:09, 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 change the behavior to be in
> accordance with the GICv2.0 specs for "generic implementations"
> (s->revision == 1 || s->revision == 2).
>
> Generic implementations also mandate that level-triggered interrupts
> that are pending due to a write to GICD_ISPENDR stay pending until the
> interrupt is acknowledged or until there's a write to the GICD_ICPENDR.
> Similarly, only clear the pending state on writes to GICD_ICPENDR if the
> interrupt signal is not asserted.  This requires an extra state variable
> to keep track of writes to the set/clear registers, corresponding to the
> D-flip flop in the GICv2.0 specs Figure 4-10.

So this means we now have two lots of pending state, the pending
and sw_pending fields. I think this is not in fact correct, and there
is only one lot of state in a real h/w GIC (it's that flipflop). AIUI the
correct behaviour is:

  * GIC_TEST_PENDING() should read:
    ((s->irq_state[irq].pending & (cm) != 0) ||
       (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm))
     -- this corresponds to the OR gate in fig 4-10; this is the value
    to be used in reads from ICPENDR/ISPENDR and other "is this
    interrupt pending" checks like the one in gic_update()
  * in gic_set_irq(), if level is 1 (rising edge) we GIC_SET_PENDING
     only if the interrupt is edge-triggered (pending status isn't latched
     for level triggered interrupts). We always GIC_SET_LEVEL(irq, 1).
  * in gic_set_irq(), if level is 0 (falling edge) we don't touch the pending
    state, but just GIC_SET_LEVEL(irq, 0)
  * gic_acknowledge_irq() always does a GIC_CLEAR_PENDING()
    (this won't actually make a level-triggered interrupt be not pending
    because of the second clause of our updated GIC_TEST_PENDING)
  * writes to the I[CS]PENDR registers always result in GIC_SET_PENDING
    or GIC_CLEAR_PENDING calls (updating the latch state)

(The above applies to GIC v1/v2 only; 11mpcore wants to keep the
current GIC_TEST_PENDING definition, etc, but I figured it would
be clearer to just describe the v1/v2 behaviour without peppering it
with "not 11mpcore" caveats.)

The v7M NVIC (ie s->rev ==REV_NVIC) should behave like the
11MPcore GIC. (At least, the v7M specified behaviour for the
pending bits is that they are cleared on exception entry and then
the interrupt re-pends if the signal is still asserted on exception
exit. I suspect that there are cases where v7M diverges from
11MPCore GIC behaviour, but that's what we're currently treating
it like, so leaving it that way will not be a regression.)

I haven't thought too hard about how this should interact with the
KVM GIC save/restore interface, but I think that it should be OK
to just do a simple "save/load the value of the I*PENDR registers",
because the only case where this isn't just a simple replication
of the source state to the destination is:
 * the source has a 1 bit in the pending register because of a
   level triggered interrupt whose input is currently asserted,
   but its pending latch is not set
 * in migration we write a 1 bit into the destination pending
   register, which causes the destination's pending latch to be
   set even though it was clear on the source
 * however this is fairly harmless, because when the guest takes
   the interrupt after migration (which it would have done anyway
   because of the asserted input) the interrupt acknowlege process
   will clear the pending latch and we'll be back in sync with
   where we should be
I think it's technically possible to write a guest which can detect
the difference (it would have to disable interrupts, prod the device
to assert its interrupt, clear the interrupt status on the device and
then reenable interrupts again, at which point if we've migrated
in the middle of that we get a single spurious interrupt). But I would
be entirely unsurprised if the guest saw the same behaviour if
it was running on bare metal and there was a power management
CPU & GIC power-down/power-up transition, since the power
management code also has to do GIC state save and restore...
In any case any non-perverse guest should never notice.

Apologies for the "you need to rewrite this" review at this late
stage, but I didn't find the necessary information about the
behaviour here until the very tail end of last year.

> @@ -423,25 +470,38 @@ 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)
> +          value = 0;

This change is correct but it should be its own patch since it's a
standalone bugfix.

Minor style note: coding style demands braces on all if() statements,
even one-liners like this one (and even if you're just making a slight
tweak to code that was already present and not coding-standard
compliant). scripts/checkpatch.pl ought to spot this.

> --- 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

If we're going to introduce this (and it seems like a good idea) we should
use it more consistently in the places that currently have a hardcoded "16".
(You might want to make that a separate patch.)

thanks
-- PMM

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

[Peter Maydell]

On 21 December 2013 06:09, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> 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.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
> 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 | 11 +++++++++++
>  3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 7ea869a..5973bdb 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -649,6 +649,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);
> @@ -676,6 +678,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 ca12f06..f9a7a0a 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -59,8 +59,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[]) {
> @@ -79,6 +79,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 f37bf69..1c8bb2a 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;
> @@ -76,6 +79,14 @@ 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.

If you add a note to this comment:
    * 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.

then you can add
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(Both last_active[][] and running_irq[] are not guest visible, and
the only thing we use them for in the TCG GIC is so we can set
the running_priority (which is guest visible). I think that doing this
via the APRn instead is actually pretty straightforward: as per
the GICv2 spec section on GICC_EOIR, gic_complete_irq just
needs to clear the highest-priority bit in the relevant APRn and
set the running priority to correspond to whatever the next
highest set bit is. We might also need to implement priority groups
and the binary point stuff too to get this to work correctly.
Obviously there are cases where this behaves differently from
the current implementation (especially if the guest messes with
the interrupt priority registers while an interrupt is active) but
I think they fall into the realm of IMPDEF behaviour. In any
case, we don't need to try to do this right now...)

thanks
-- PMM

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

[Peter Maydell]

On 21 December 2013 06:09, 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.
>
> 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.

This new state should be guest-visible for a v2 GIC
(but not v1 or 11mpcore) via GICD_SPENDSGIR and
GICD_CPENDSGIR, right? An implementation of the
read/write would be nice if it's not too hard, or failing
that at least a LOG_UNIMP.

Other than that, looks OK, though I see one set of missing
braces and it'll need to be tweaked a little given the changes
to patch 3.

thanks
-- PMM

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

[Christoffer Dall]

On Sun, Jan 19, 2014 at 07:49:58PM +0000, Peter Maydell wrote:
> On 21 December 2013 06:09, 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 change the behavior to be in
> > accordance with the GICv2.0 specs for "generic implementations"
> > (s->revision == 1 || s->revision == 2).
> >
> > Generic implementations also mandate that level-triggered interrupts
> > that are pending due to a write to GICD_ISPENDR stay pending until the
> > interrupt is acknowledged or until there's a write to the GICD_ICPENDR.
> > Similarly, only clear the pending state on writes to GICD_ICPENDR if the
> > interrupt signal is not asserted.  This requires an extra state variable
> > to keep track of writes to the set/clear registers, corresponding to the
> > D-flip flop in the GICv2.0 specs Figure 4-10.
> 
> So this means we now have two lots of pending state, the pending
> and sw_pending fields. I think this is not in fact correct, and there
> is only one lot of state in a real h/w GIC (it's that flipflop). AIUI the
> correct behaviour is:
> 
>   * GIC_TEST_PENDING() should read:
>     ((s->irq_state[irq].pending & (cm) != 0) ||
>        (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm))
>      -- this corresponds to the OR gate in fig 4-10; this is the value
>     to be used in reads from ICPENDR/ISPENDR and other "is this
>     interrupt pending" checks like the one in gic_update()
>   * in gic_set_irq(), if level is 1 (rising edge) we GIC_SET_PENDING
>      only if the interrupt is edge-triggered (pending status isn't latched
>      for level triggered interrupts). We always GIC_SET_LEVEL(irq, 1).
>   * in gic_set_irq(), if level is 0 (falling edge) we don't touch the pending
>     state, but just GIC_SET_LEVEL(irq, 0)
>   * gic_acknowledge_irq() always does a GIC_CLEAR_PENDING()
>     (this won't actually make a level-triggered interrupt be not pending
>     because of the second clause of our updated GIC_TEST_PENDING)
>   * writes to the I[CS]PENDR registers always result in GIC_SET_PENDING
>     or GIC_CLEAR_PENDING calls (updating the latch state)

ok, it's probably cleaner code-wise, but should produce the same result
as far as I can see.  I guess I see the pending field as the result of
the final or-gate (possibly because I'm too tied to the way the
in-kernel emulation works where we don't have a separate 'level' state).
Anyway, I'll respin the patch according to your directions (my only
concern with that is that it feels a bit unlogical to have a bitfield
called pending where some of the bits are not set despite the
corresponding interrupts being pending, but we'll see if the overall
code looks more clean with this approach).

Thanks.

> 
> (The above applies to GIC v1/v2 only; 11mpcore wants to keep the
> current GIC_TEST_PENDING definition, etc, but I figured it would
> be clearer to just describe the v1/v2 behaviour without peppering it
> with "not 11mpcore" caveats.)
> 
> The v7M NVIC (ie s->rev ==REV_NVIC) should behave like the
> 11MPcore GIC. (At least, the v7M specified behaviour for the
> pending bits is that they are cleared on exception entry and then
> the interrupt re-pends if the signal is still asserted on exception
> exit. I suspect that there are cases where v7M diverges from
> 11MPCore GIC behaviour, but that's what we're currently treating
> it like, so leaving it that way will not be a regression.)
> 
> I haven't thought too hard about how this should interact with the
> KVM GIC save/restore interface, but I think that it should be OK
> to just do a simple "save/load the value of the I*PENDR registers",
> because the only case where this isn't just a simple replication
> of the source state to the destination is:
>  * the source has a 1 bit in the pending register because of a
>    level triggered interrupt whose input is currently asserted,
>    but its pending latch is not set
>  * in migration we write a 1 bit into the destination pending
>    register, which causes the destination's pending latch to be
>    set even though it was clear on the source
>  * however this is fairly harmless, because when the guest takes
>    the interrupt after migration (which it would have done anyway
>    because of the asserted input) the interrupt acknowlege process
>    will clear the pending latch and we'll be back in sync with
>    where we should be

agreed, isn't this analagous to suspend/resume on hardware, or would you
assume that the device state would be restored to assert the input
again?

> I think it's technically possible to write a guest which can detect
> the difference (it would have to disable interrupts, prod the device
> to assert its interrupt, clear the interrupt status on the device and
> then reenable interrupts again, at which point if we've migrated
> in the middle of that we get a single spurious interrupt). But I would
> be entirely unsurprised if the guest saw the same behaviour if
> it was running on bare metal and there was a power management
> CPU & GIC power-down/power-up transition, since the power
> management code also has to do GIC state save and restore...
> In any case any non-perverse guest should never notice.
> 
> Apologies for the "you need to rewrite this" review at this late
> stage, but I didn't find the necessary information about the
> behaviour here until the very tail end of last year.
> 

No worries, you have a lot on your plate.

> > @@ -423,25 +470,38 @@ 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)
> > +          value = 0;
> 
> This change is correct but it should be its own patch since it's a
> standalone bugfix.
> 
> Minor style note: coding style demands braces on all if() statements,
> even one-liners like this one (and even if you're just making a slight
> tweak to code that was already present and not coding-standard
> compliant). scripts/checkpatch.pl ought to spot this.
>
> > --- 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
> 
> If we're going to introduce this (and it seems like a good idea) we should
> use it more consistently in the places that currently have a hardcoded "16".
> (You might want to make that a separate patch.)
> 
Ok, moving these changes into two separate patches and fixing the code
style.

-Christoffer

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

[Peter Maydell]

On 28 January 2014 02:09, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Sun, Jan 19, 2014 at 07:49:58PM +0000, Peter Maydell wrote:
>> So this means we now have two lots of pending state, the pending
>> and sw_pending fields. I think this is not in fact correct, and there
>> is only one lot of state in a real h/w GIC (it's that flipflop). AIUI the
>> correct behaviour is:
>>
>>   * GIC_TEST_PENDING() should read:
>>     ((s->irq_state[irq].pending & (cm) != 0) ||
>>        (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_LEVEL(irq, cm))
>>      -- this corresponds to the OR gate in fig 4-10; this is the value
>>     to be used in reads from ICPENDR/ISPENDR and other "is this
>>     interrupt pending" checks like the one in gic_update()
>>   * in gic_set_irq(), if level is 1 (rising edge) we GIC_SET_PENDING
>>      only if the interrupt is edge-triggered (pending status isn't latched
>>      for level triggered interrupts). We always GIC_SET_LEVEL(irq, 1).
>>   * in gic_set_irq(), if level is 0 (falling edge) we don't touch the pending
>>     state, but just GIC_SET_LEVEL(irq, 0)
>>   * gic_acknowledge_irq() always does a GIC_CLEAR_PENDING()
>>     (this won't actually make a level-triggered interrupt be not pending
>>     because of the second clause of our updated GIC_TEST_PENDING)
>>   * writes to the I[CS]PENDR registers always result in GIC_SET_PENDING
>>     or GIC_CLEAR_PENDING calls (updating the latch state)
>
> ok, it's probably cleaner code-wise, but should produce the same result
> as far as I can see.  I guess I see the pending field as the result of
> the final or-gate (possibly because I'm too tied to the way the
> in-kernel emulation works where we don't have a separate 'level' state).
> Anyway, I'll respin the patch according to your directions (my only
> concern with that is that it feels a bit unlogical to have a bitfield
> called pending where some of the bits are not set despite the
> corresponding interrupts being pending, but we'll see if the overall
> code looks more clean with this approach).

The key thing is that we definitely shouldn't be keeping more
state than the hardware does; that's always a bad sign.

thanks
-- PMM