[Qemu-devel] [RFC PATCH v2 0/6] Support arm-gic-kvm save/restore

[Qemu-devel] [RFC PATCH v2 0/6] 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 few 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-v2

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

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

Christoffer Dall (6):
  hw: arm_gic: Fix gic_set_irq handling
  hw: arm_gic: Introduce GIC_SET_PRIORITY macro
  hw: arm_gic: Keep track of SGI sources
  arm_gic: Support setting/getting binary point reg
  vmstate: Add uint32 2D-array support
  hw: arm_gic_kvm: Add KVM VGIC save/restore logic

 hw/intc/arm_gic.c           |   73 ++++++--
 hw/intc/arm_gic_common.c    |    8 +-
 hw/intc/arm_gic_kvm.c       |  424 ++++++++++++++++++++++++++++++++++++++++++-
 hw/intc/gic_internal.h      |   19 ++
 include/migration/vmstate.h |    6 +
 5 files changed, 506 insertions(+), 24 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [RFC PATCH v2 1/6] hw: arm_gic: Fix gic_set_irq handling

[Christoffer Dall]

For some reason only edge-triggered or enabled level-triggered
interrupts would set the pending state of a raised IRQ.  This is not in
compliance with the specs, which indicate that the pending state is
separate from the enabled state, which only controls if a pending
interrupt is actually forwarded to the CPU interface.

Therefore, simply always set the pending state on a rising edge, but
only clear the pending state of falling edge if the interrupt is level
triggered.

Changelog [v2]:
 - Fix bisection issue, by not using gic_clear_pending yet.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 hw/intc/arm_gic.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index d431b7a..c7a24d5 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -128,11 +128,12 @@ 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)) {
-            DPRINTF("Set %d pending mask %x\n", irq, target);
-            GIC_SET_PENDING(irq, target);
-        }
+        DPRINTF("Set %d pending mask %x\n", irq, target);
+        GIC_SET_PENDING(irq, target);
     } else {
+        if (!GIC_TEST_TRIGGER(irq)) {
+            GIC_CLEAR_PENDING(irq, target);
+        }
         GIC_CLEAR_LEVEL(irq, cm);
     }
     gic_update(s);
-- 
1.7.10.4

[Qemu-devel] [RFC PATCH v2 2/6] hw: arm_gic: Introduce GIC_SET_PRIORITY macro

[Christoffer Dall]

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

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 c7a24d5..7eaa55f 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -169,6 +169,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;
@@ -444,11 +453,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 b3788a8..09e7722 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -111,6 +111,7 @@ 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);
 
 #define TYPE_ARM_GIC_COMMON "arm_gic_common"
 #define ARM_GIC_COMMON(obj) \
-- 
1.7.10.4

[Qemu-devel] [RFC PATCH v2 3/6] 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>

---

Changelog [v2]:
 - Fixed endless loop bug
 - Bump version_id and minimum_version_id on vmstate struct
---
 hw/intc/arm_gic.c        |   41 ++++++++++++++++++++++++++++++++---------
 hw/intc/arm_gic_common.c |    5 +++--
 hw/intc/gic_internal.h   |    3 +++
 3 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 7eaa55f..6470d37 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -97,6 +97,20 @@ void gic_set_pending_private(GICState *s, int cpu, int irq)
     gic_update(s);
 }
 
+static void gic_clear_pending(GICState *s, int irq, int cm, uint8_t src)
+{
+    unsigned cpu;
+
+    GIC_CLEAR_PENDING(irq, cm);
+    if (irq < GIC_NR_SGIS) {
+        cpu = (unsigned)ffs(cm) - 1;
+        while (cpu < NCPU) {
+            s->sgi_source[irq][cpu] &= ~(1 << src);
+            cpu = (unsigned)ffs(cm) - 1;
+        }
+    }
+}
+
 /* Process a change in an external IRQ input.  */
 static void gic_set_irq(void *opaque, int irq, int level)
 {
@@ -132,7 +146,7 @@ static void gic_set_irq(void *opaque, int irq, int level)
         GIC_SET_PENDING(irq, target);
     } else {
         if (!GIC_TEST_TRIGGER(irq)) {
-            GIC_CLEAR_PENDING(irq, target);
+            gic_clear_pending(s, irq, target, 0);
         }
         GIC_CLEAR_LEVEL(irq, cm);
     }
@@ -163,7 +177,8 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
     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);
+    gic_clear_pending(s, new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm,
+                                  GIC_SGI_SRC(new_irq, cpu));
     gic_set_running_irq(s, cpu, new_irq);
     DPRINTF("ACK %d\n", new_irq);
     return new_irq;
@@ -437,12 +452,9 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         irq = (offset - 0x280) * 8 + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        for (i = 0; i < 8; i++) {
-            /* ??? This currently clears the pending bit for all CPUs, even
-               for per-CPU interrupts.  It's unclear whether this is the
-               corect behavior.  */
-            if (value & (1 << i)) {
-                GIC_CLEAR_PENDING(irq + i, ALL_CPU_MASK);
+        for (i = 0; i < 8; i++, irq++) {
+            if (irq > GIC_NR_SGIS && value & (1 << i)) {
+                gic_clear_pending(s, irq, 1 << cpu, 0);
             }
         }
     } else if (offset < 0x400) {
@@ -515,6 +527,7 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
         int cpu;
         int irq;
         int mask;
+        unsigned target_cpu;
 
         cpu = gic_get_current_cpu(s);
         irq = value & 0x3ff;
@@ -534,6 +547,12 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
             break;
         }
         GIC_SET_PENDING(irq, mask);
+        target_cpu = (unsigned)ffs(mask) - 1;
+        while (target_cpu < NCPU) {
+            s->sgi_source[irq][target_cpu] |= (1 << cpu);
+            mask &= ~(1 << target_cpu);
+            target_cpu = (unsigned)ffs(mask) - 1;
+        }
         gic_update(s);
         return;
     }
@@ -551,6 +570,8 @@ static const MemoryRegionOps gic_dist_ops = {
 
 static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
 {
+    int value;
+
     switch (offset) {
     case 0x00: /* Control */
         return s->cpu_enabled[cpu];
@@ -560,7 +581,9 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
         /* ??? Not implemented.  */
         return 0;
     case 0x0c: /* Acknowledge */
-        return gic_acknowledge_irq(s, cpu);
+        value = gic_acknowledge_irq(s, cpu);
+        value |= (GIC_SGI_SRC(value, cpu) & 0x7) << 10;
+        return value;
     case 0x14: /* Running Priority */
         return s->running_priority[cpu];
     case 0x18: /* Highest Pending Interrupt */
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 709b5c2..0657e8b 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, NCPU),
         VMSTATE_UINT8_ARRAY(priority2, GICState, GIC_MAXIRQ - GIC_INTERNAL),
         VMSTATE_UINT16_2DARRAY(last_active, GICState, GIC_MAXIRQ, NCPU),
+        VMSTATE_UINT8_2DARRAY(sgi_source, GICState, GIC_NR_SGIS, NCPU),
         VMSTATE_UINT16_ARRAY(priority_mask, GICState, NCPU),
         VMSTATE_UINT16_ARRAY(running_irq, GICState, NCPU),
         VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU),
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 09e7722..5b53242 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.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 NCPU 8
 
@@ -58,6 +59,7 @@
                                     s->priority1[irq][cpu] :            \
                                     s->priority2[(irq) - GIC_INTERNAL])
 #define GIC_TARGET(irq) s->irq_target[irq]
+#define GIC_SGI_SRC(irq, cpu) (((irq) < GIC_NR_SGIS) ? ffs(s->sgi_source[irq][cpu]) - 1 : 0)
 
 typedef struct gic_irq_state {
     /* The enable bits are only banked for per-cpu interrupts.  */
@@ -83,6 +85,7 @@ typedef struct GICState {
     uint8_t priority1[GIC_INTERNAL][NCPU];
     uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
     uint16_t last_active[GIC_MAXIRQ][NCPU];
+    uint8_t sgi_source[GIC_NR_SGIS][NCPU];
 
     uint16_t priority_mask[NCPU];
     uint16_t running_irq[NCPU];
-- 
1.7.10.4

[Qemu-devel] [RFC PATCH v2 4/6] 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.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

Changelog [v2]:
 - Renamed binary_point to bpr and abpr
 - Added GICC_ABPR read-as-write logic for TCG
---
 hw/intc/arm_gic.c        |   10 +++++++---
 hw/intc/arm_gic_common.c |    6 ++++--
 hw/intc/gic_internal.h   |    7 +++++++
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 6470d37..d1ddac1 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -578,8 +578,7 @@ 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 */
         value = gic_acknowledge_irq(s, cpu);
         value |= (GIC_SGI_SRC(value, cpu) & 0x7) << 10;
@@ -588,6 +587,8 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
         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);
@@ -606,10 +607,13 @@ 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 */
+        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 0657e8b..5449d77 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, NCPU),
         VMSTATE_UINT16_ARRAY(running_priority, GICState, NCPU),
         VMSTATE_UINT16_ARRAY(current_pending, GICState, NCPU),
+        VMSTATE_UINT8_ARRAY(bpr, GICState, NCPU),
+        VMSTATE_UINT8_ARRAY(abpr, GICState, NCPU),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 5b53242..758b85a 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -92,6 +92,13 @@ typedef struct GICState {
     uint16_t running_priority[NCPU];
     uint16_t current_pending[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[NCPU];
+    uint8_t  abpr[NCPU];
+
     uint32_t num_cpu;
 
     MemoryRegion iomem; /* Distributor */
-- 
1.7.10.4

[Qemu-devel] [RFC PATCH v2 5/6] vmstate: Add uint32 2D-array support

[Christoffer Dall]

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

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 1c31b5d..e5538c7 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -633,9 +633,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.7.10.4

[Qemu-devel] [RFC PATCH v2 6/6] 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.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

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_common.c |    5 +-
 hw/intc/arm_gic_kvm.c    |  424 +++++++++++++++++++++++++++++++++++++++++++++-
 hw/intc/gic_internal.h   |    8 +
 3 files changed, 433 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 5449d77..1d3b738 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, NCPU),
         VMSTATE_UINT8_ARRAY(bpr, GICState, NCPU),
         VMSTATE_UINT8_ARRAY(abpr, GICState, NCPU),
+        VMSTATE_UINT32_2DARRAY(apr, GICState, 4, NCPU),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 158f047..1510c4d 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_TRIGGER(irq)) ? 0x2 : 0x0;
+    } else {
+        if (*field & 0x2) {
+            GIC_SET_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_source[irq][cpu] & 0xff;
+    } else {
+        s->sgi_source[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_source -> 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_source */
+    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)
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 758b85a..f9133b9 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -99,6 +99,14 @@ typedef struct GICState {
     uint8_t  bpr[NCPU];
     uint8_t  abpr[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 one or more interrupts for preemption level X are active, then
+     *   APRn[X mod 32] == 0b1,  where n = X / 32
+     * otherwise the bit is clear.
+     */
+    uint32_t apr[4][NCPU];
+
     uint32_t num_cpu;
 
     MemoryRegion iomem; /* Distributor */
-- 
1.7.10.4

Re: [Qemu-devel] [RFC PATCH v2 6/6] hw: arm_gic_kvm: Add KVM VGIC save/restore logic

[Alex Bennée]

christoffer.dall@linaro.org writes:

> Save and restore the ARM KVM VGIC state from the kernel.  We rely on
<snip>
>  
>  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[]) {

Does this mean QEMU and Kernel need to be kept in lock-step for
compatibility?

>  
> +//#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)
> +

Shouldn't we be using QEMU logging framework for this? Also for the
fprintfs later on.

>  #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) {

Should 1, 0x2 etc be #define'd constants?

<snip>
>  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_source -> ICD_CPENDSGIRn */
> +    kvm_dist_put(s, 0xf10, 8, GIC_NR_SGIS, translate_clear);
> +    kvm_dist_put(s, 0xf20, 8, GIC_NR_SGIS, translate_sgisource);

Again what are these magic numbers? I assume the comments refer to the
actual reg names in the manual? Perhaps putting a reference to the
manual if these values aren't to be used else where?

<snip>

-- 
Alex Bennée

Re: [Qemu-devel] [RFC PATCH v2 1/6] hw: arm_gic: Fix gic_set_irq handling

[Peter Maydell]

On 26 September 2013 22:03, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> For some reason only edge-triggered or enabled level-triggered
> interrupts would set the pending state of a raised IRQ.  This is not in
> compliance with the specs, which indicate that the pending state is
> separate from the enabled state, which only controls if a pending
> interrupt is actually forwarded to the CPU interface.
>
> Therefore, simply always set the pending state on a rising edge, but
> only clear the pending state of falling edge if the interrupt is level
> triggered.

> @@ -128,11 +128,12 @@ 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)) {
> -            DPRINTF("Set %d pending mask %x\n", irq, target);
> -            GIC_SET_PENDING(irq, target);
> -        }
> +        DPRINTF("Set %d pending mask %x\n", irq, target);
> +        GIC_SET_PENDING(irq, target);
>      } else {
> +        if (!GIC_TEST_TRIGGER(irq)) {
> +            GIC_CLEAR_PENDING(irq, target);
> +        }
>          GIC_CLEAR_LEVEL(irq, cm);
>      }
>      gic_update(s);

The old logic is definitely wrong here, but this still isn't
quite right. See the GIC v2 spec, section 4.3.8 GICD_ICPENDRn
and specifically the circuit diagram in Figure 4.10.
For a level triggered interrupt we mustn't clear the pending
state on a 1->0 transition of the input if it was latched
by the guest writing to GICD_ISPENDR.

In other words, the internal state of the GIC (ie the state
of the latch) and the value in the ICPENDR/ISPENDR register
on read aren't the same thing, and the bug in our current
GIC model is that we're trying to use the .pending field
for both purposes at the same time.

-- PMM

Re: [Qemu-devel] [RFC PATCH v2 2/6] hw: arm_gic: Introduce GIC_SET_PRIORITY macro

[Peter Maydell]

On 26 September 2013 22:03, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> To make the code slightly cleaner to look at and make the save/restore
> code easier to understand, introduce this macro to set the priority of
> interrupts.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

Commit message and subject need updating since it's not
a macro and not GIC_SET_PRIORITY any more, but otherwise

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

-- PMM

Re: [Qemu-devel] [RFC PATCH v2 3/6] hw: arm_gic: Keep track of SGI sources

[Peter Maydell]

On 26 September 2013 22:03, 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.

Shouldn't the state you have in sgi_source[][] be surfaced as the
GICD_CPENDSGIR/GICD_SPENDSGIR registers [for a v2 GIC; they don't
exist in v1]? It might then be better to represent the state in
our data structures in the same layout as the registers.

>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>
> ---
>
> Changelog [v2]:
>  - Fixed endless loop bug
>  - Bump version_id and minimum_version_id on vmstate struct
> ---
>  hw/intc/arm_gic.c        |   41 ++++++++++++++++++++++++++++++++---------
>  hw/intc/arm_gic_common.c |    5 +++--
>  hw/intc/gic_internal.h   |    3 +++
>  3 files changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 7eaa55f..6470d37 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -97,6 +97,20 @@ void gic_set_pending_private(GICState *s, int cpu, int irq)
>      gic_update(s);
>  }
>
> +static void gic_clear_pending(GICState *s, int irq, int cm, uint8_t src)
> +{

I think that in the cases where we pass in 0 for src that the
irq can't be < GIC_NR_SGIS.

> +    unsigned cpu;
> +
> +    GIC_CLEAR_PENDING(irq, cm);
> +    if (irq < GIC_NR_SGIS) {
> +        cpu = (unsigned)ffs(cm) - 1;

If you used ctz32() rather than ffs() it would save you having to
subtract one all the time. Also, those unsigned casts are pretty
ugly: better to just make 'cpu' an int...

> +        while (cpu < NCPU) {
> +            s->sgi_source[irq][cpu] &= ~(1 << src);
> +            cpu = (unsigned)ffs(cm) - 1;
> +        }

...this still seems to be an infinite loop: cm isn't modified
inside the loop so cpu will always have the same value each time.

     610:       eb fe                   jmp    610 <gic_clear_pending+0x50>

> +    }

Are you sure the logic in this function is right? (ie that we
should only clear the sgi_source[][] bit for this source, and
not completely? If nothing else, I think the interrupt should
stay pending if some other source cpu still wants it to be
pending. That is, I think we need to track the pending status
separately for each (irq,target-cpu,source-cpu) separately for
SGIs. (I'm not totally sure I have this right though, the spec
is quite confusing.)

> +}
> +

>  /* Maximum number of possible CPU interfaces, determined by GIC architecture */
>  #define NCPU 8
>
> @@ -58,6 +59,7 @@
>                                      s->priority1[irq][cpu] :            \
>                                      s->priority2[(irq) - GIC_INTERNAL])
>  #define GIC_TARGET(irq) s->irq_target[irq]
> +#define GIC_SGI_SRC(irq, cpu) (((irq) < GIC_NR_SGIS) ? ffs(s->sgi_source[irq][cpu]) - 1 : 0)

WARNING: line over 80 characters
#161: FILE: hw/intc/gic_internal.h:62:
+#define GIC_SGI_SRC(irq, cpu) (((irq) < GIC_NR_SGIS) ?
ffs(s->sgi_source[irq][cpu]) - 1 : 0)

-- PMM

Re: [Qemu-devel] [RFC PATCH v2 4/6] arm_gic: Support setting/getting binary point reg

[Peter Maydell]

On 26 September 2013 22:03, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> @@ -606,10 +607,13 @@ 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 */
> +        s->abpr[cpu] = (value & 0x7);
> +        break;

The ABPR should RAZ/WI for GICs prior to v2, so this needs
to be guarded with "if (s->revision >= 2) { ...".

Otherwise looks good.

-- PMM

Re: [Qemu-devel] [RFC PATCH v2 5/6] vmstate: Add uint32 2D-array support

[Peter Maydell]

On 26 September 2013 22:03, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> Add support for saving VMtate of 2D arrays of uint32 values.

"VMState". Otherwise:

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

-- PMM

Re: [Qemu-devel] [RFC PATCH v2 3/6] hw: arm_gic: Keep track of SGI sources

[Peter Maydell]

On 14 October 2013 16:36, Peter Maydell <peter.maydell@linaro.org> wrote:
> Are you sure the logic in this function is right? (ie that we
> should only clear the sgi_source[][] bit for this source, and
> not completely? If nothing else, I think the interrupt should
> stay pending if some other source cpu still wants it to be
> pending. That is, I think we need to track the pending status
> separately for each (irq,target-cpu,source-cpu) separately for
> SGIs. (I'm not totally sure I have this right though, the spec
> is quite confusing.)

Having spoken to Marc I'm now a bit less confused. For
SGIs:
 * there is effectively a pending bit for each
   (irq-number, source-cpu, target-cpu) tuple
 * for a v2 GIC these are guest-visible in GICD_CPENDSGIRn
   and GICD_SPENDSGIRn; for a pre-v2 GIC the state still
   exists, it's just not guest-visible
 * the overall "pending state" bit in GICD_ISPENDRn and
   GICD_ICPENDRn is effectively the logical OR of the
   pending bits for each source-cpu; writes to this bit
   are ignored
 * when the guest reads from GICC_IAR, if the SGI is
   pending for multiple source-cpus for this target-cpu
   then we clear the pending bit for the (int, src, tgt)
   tuple in question, but it will still be set for the
   other (int,src,tgt) tuples for the other source-cpus
   (and so the GICD_ISPENDRn bit will still be set)

Tangentially, I notice that we don't correctly handle
the PENDING bit for level triggered interrupts, since
we do:

    /* Clear pending flags for both level and edge triggered interrupts.
       Level triggered IRQs will be reasserted once they become inactive.  */
    gic_clear_pending(s, new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm,
                                  GIC_SGI_SRC(new_irq, cpu));

in gic_acknowledge_irq(). This is wrong, because section
3.2.4 is clear for a level triggered interrupt that if the
interrupt signal remains asserted (which it usually will be)
then we go from Pending to Active+Pending (whereas our
current implementation goes from Pending to Active and
then resets Pending later in gic_complete_irq()).

-- PMM

Re: [Qemu-devel] [RFC PATCH v2 6/6] hw: arm_gic_kvm: Add KVM VGIC save/restore logic

[Peter Maydell]

On 27 September 2013 09:11, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> christoffer.dall@linaro.org writes:
>
>> Save and restore the ARM KVM VGIC state from the kernel.  We rely on
> <snip>
>>
>>  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[]) {
>
> Does this mean QEMU and Kernel need to be kept in lock-step for
> compatibility?

No. This patch is a little confusing because it's both adding
the new fields and also adding the save/restore support, but
once we have the data structures and vmstate in QEMU holding
all the state the kernel needs, there shouldn't be any need
to update the vmstate in a backwards-incompatible way.

>>
>> +//#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)
>> +
>
> Shouldn't we be using QEMU logging framework for this? Also for the
> fprintfs later on.

No, these are debug printfs, not things which would be interesting
to the average user/person trying to debug a guest. We don't
have a particularly clean framework for compile time
enabled debug printfs, so 'some random macro in each individual
file' is the current approach.

-- PMM

Re: [Qemu-devel] [RFC PATCH v2 6/6] hw: arm_gic_kvm: Add KVM VGIC save/restore logic

[Peter Maydell]

On 26 September 2013 22:03, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> 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.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>
> 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_common.c |    5 +-
>  hw/intc/arm_gic_kvm.c    |  424 +++++++++++++++++++++++++++++++++++++++++++++-
>  hw/intc/gic_internal.h   |    8 +
>  3 files changed, 433 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index 5449d77..1d3b738 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, NCPU),
>          VMSTATE_UINT8_ARRAY(bpr, GICState, NCPU),
>          VMSTATE_UINT8_ARRAY(abpr, GICState, NCPU),
> +        VMSTATE_UINT32_2DARRAY(apr, GICState, 4, NCPU),

I feel like we should add this new apr state (plus some
documentation and at least the TCG read/write interface
to the state) in one patch and then put the save/load
in its own patch.

> +    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();

Bad indent.

> +    }
> +}
>  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_source -> 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);
> +        }
> +    }
>  }

So an interesting question here is "is there state we currently
save/restore for the TCG GIC which we're not passing to the
kernel, and if so why?". I think the fields we don't do anything
with are:
 gic_irq_state::model
 GICState::last_active
 GICState::running_irq
 GICState::running_priority
 GICState::current_pending

I'm guessing these are a mix of "kernel doesn't implement
something" and "TCG model is wrong and this state should
show up somewhere else"; but which is which?
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index 758b85a..f9133b9 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -99,6 +99,14 @@ typedef struct GICState {
>      uint8_t  bpr[NCPU];
>      uint8_t  abpr[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 one or more interrupts for preemption level X are active, then
> +     *   APRn[X mod 32] == 0b1,  where n = X / 32
> +     * otherwise the bit is clear.
> +     */
> +    uint32_t apr[4][NCPU];

You can delete the "or more" -- there can onyl be one
interrupt active at each group priority (GICv2 spec 3.3.3).

Also, where does the hardcoded 4 come from? Answer: it's
the worst-case number of GICC_APRn registers, which happens
if you have 7 bits of group priority, which means you have
128 preemption levels, which at one bit per level fits into
4 32 bit APR regs. I think a GIC_NR_APRS #define might be nice.

> +
>      uint32_t num_cpu;
>
>      MemoryRegion iomem; /* Distributor */
> --
> 1.7.10.4

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v2 1/6] hw: arm_gic: Fix gic_set_irq handling

[Christoffer Dall]

On Mon, Oct 14, 2013 at 03:24:43PM +0100, Peter Maydell wrote:
> On 26 September 2013 22:03, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > For some reason only edge-triggered or enabled level-triggered
> > interrupts would set the pending state of a raised IRQ.  This is not in
> > compliance with the specs, which indicate that the pending state is
> > separate from the enabled state, which only controls if a pending
> > interrupt is actually forwarded to the CPU interface.
> >
> > Therefore, simply always set the pending state on a rising edge, but
> > only clear the pending state of falling edge if the interrupt is level
> > triggered.
> 
> > @@ -128,11 +128,12 @@ 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)) {
> > -            DPRINTF("Set %d pending mask %x\n", irq, target);
> > -            GIC_SET_PENDING(irq, target);
> > -        }
> > +        DPRINTF("Set %d pending mask %x\n", irq, target);
> > +        GIC_SET_PENDING(irq, target);
> >      } else {
> > +        if (!GIC_TEST_TRIGGER(irq)) {
> > +            GIC_CLEAR_PENDING(irq, target);
> > +        }
> >          GIC_CLEAR_LEVEL(irq, cm);
> >      }
> >      gic_update(s);
> 
> The old logic is definitely wrong here, but this still isn't
> quite right. See the GIC v2 spec, section 4.3.8 GICD_ICPENDRn
> and specifically the circuit diagram in Figure 4.10.
> For a level triggered interrupt we mustn't clear the pending
> state on a 1->0 transition of the input if it was latched
> by the guest writing to GICD_ISPENDR.
> 
> In other words, the internal state of the GIC (ie the state
> of the latch) and the value in the ICPENDR/ISPENDR register
> on read aren't the same thing, and the bug in our current
> GIC model is that we're trying to use the .pending field
> for both purposes at the same time.
> 
So I think that's a comment that belongs more in the category of all the
things that are broken with the GICv2 emulation and should be separate
fixes.  I don't believe Linux uses this and the in-kernel GIC emulation
also doesn't keep track of this state, but the following patch should
address the issue.  Do you want me to fold such two patches into one?

-Christoffer

[Qemu-devel] [PATCH] arm_gic: Keep track of GICD_CPENDR and GICD_SPENDR

[Christoffer Dall]

If software writes to the ISPENDR and sets the pending state of a
level-triggered interrupt, the falling edge of the hardware input must
not clear the pending state.  Conversely, if software writes to the
ICPENDR, the pending state of a level-triggered interrupt should only be
cleared if the hardware input is not asserted.

This requires an extra state variable to keep track of software writes.

Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 hw/intc/arm_gic.c        | 20 +++++++++++++++++---
 hw/intc/arm_gic_common.c |  5 +++--
 hw/intc/gic_internal.h   |  4 ++++
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index d1ddac1..db54061 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -101,6 +101,12 @@ static void gic_clear_pending(GICState *s, int irq, int cm, uint8_t src)
 {
     unsigned cpu;
 
+    /* If a level-triggered interrupt has been set to pending through the
+     * GICD_SPENDR, then a falling edge does not clear the pending state.
+     */
+    if (GIC_TEST_SW_PENDING(irq, cm))
+        return;
+
     GIC_CLEAR_PENDING(irq, cm);
     if (irq < GIC_NR_SGIS) {
         cpu = (unsigned)ffs(cm) - 1;
@@ -177,8 +183,9 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
     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(s, new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm,
-                                  GIC_SGI_SRC(new_irq, cpu));
+    cm = GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm;
+    GIC_CLEAR_SW_PENDING(new_irq, cm);
+    gic_clear_pending(s, new_irq, cm, GIC_SGI_SRC(new_irq, cpu));
     gic_set_running_irq(s, cpu, new_irq);
     DPRINTF("ACK %d\n", new_irq);
     return new_irq;
@@ -445,16 +452,23 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         for (i = 0; i < 8; i++) {
             if (value & (1 << i)) {
                 GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
+                if (!GIC_TEST_TRIGGER(irq + i)) {
+                    GIC_SET_SW_PENDING(irq + i, GIC_TARGET(irq + i));
+                }
             }
         }
     } 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++, irq++) {
             if (irq > GIC_NR_SGIS && value & (1 << i)) {
-                gic_clear_pending(s, irq, 1 << cpu, 0);
+                GIC_CLEAR_SW_PENDING(irq, cm);
+                if (GIC_TEST_TRIGGER(irq + i) || !GIC_TEST_LEVEL(irq, cm)) {
+                    GIC_CLEAR_PENDING(irq, cm);
+                }
             }
         }
     } else if (offset < 0x400) {
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 1d3b738..7f0615f 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 f9133b9..173c607 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -43,6 +43,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)
@@ -65,6 +68,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.1.2

Re: [Qemu-devel] [RFC PATCH v2 3/6] hw: arm_gic: Keep track of SGI sources

[Christoffer Dall]

On Mon, Oct 14, 2013 at 05:33:38PM +0100, Peter Maydell wrote:
> On 14 October 2013 16:36, Peter Maydell <peter.maydell@linaro.org> wrote:

[...]

> 
> Tangentially, I notice that we don't correctly handle
> the PENDING bit for level triggered interrupts, since
> we do:
> 
>     /* Clear pending flags for both level and edge triggered interrupts.
>        Level triggered IRQs will be reasserted once they become inactive.  */
>     gic_clear_pending(s, new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm,
>                                   GIC_SGI_SRC(new_irq, cpu));
> 
> in gic_acknowledge_irq(). This is wrong, because section
> 3.2.4 is clear for a level triggered interrupt that if the
> interrupt signal remains asserted (which it usually will be)
> then we go from Pending to Active+Pending (whereas our
> current implementation goes from Pending to Active and
> then resets Pending later in gic_complete_irq()).
> 

Yes, I will send this patch to address this as part of the revised series:


diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index bf1eb02..fce66c6 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -175,10 +180,15 @@ 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(s, new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm,
-                                  GIC_SGI_SRC(new_irq, cpu));
+    /* Clear pending flags for edge-triggered and non-asserted level-triggered
+     * interrupts.
+     */
+    cm = GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm;
+    if (GIC_TEST_TRIGGER(new_irq) || !GIC_TEST_LEVEL(new_irq, cm)) {
+        gic_clear_pending(s, new_irq, cm, GIC_SGI_SRC(new_irq, cpu));
+    }
+
     gic_set_running_irq(s, cpu, new_irq);
     DPRINTF("ACK %d\n", new_irq);
     return new_irq;

-- 
Christoffer

Re: [Qemu-devel] [PATCH] arm_gic: Keep track of GICD_CPENDR and GICD_SPENDR

[Bhushan Bharat-R65777]

Hi Christoffer,

Not related to the patch, for edge type of interrupt, will setting bit in ICD_SPENDR generate interrupt?

Thanks
-Bharat

> -----Original Message-----
> From: Christoffer Dall [mailto:christoffer.dall@linaro.org]
> Sent: Wednesday, October 23, 2013 8:57 PM
> To: peter.maydell@linaro.org
> Cc: patches@linaro.org; qemu-devel@nongnu.org; kvmarm@lists.cs.columbia.edu
> Subject: [PATCH] arm_gic: Keep track of GICD_CPENDR and GICD_SPENDR
> 
> If software writes to the ISPENDR and sets the pending state of a level-
> triggered interrupt, the falling edge of the hardware input must not clear the
> pending state.  Conversely, if software writes to the ICPENDR, the pending state
> of a level-triggered interrupt should only be cleared if the hardware input is
> not asserted.
> 
> This requires an extra state variable to keep track of software writes.
> 
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  hw/intc/arm_gic.c        | 20 +++++++++++++++++---
>  hw/intc/arm_gic_common.c |  5 +++--
>  hw/intc/gic_internal.h   |  4 ++++
>  3 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index d1ddac1..db54061 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -101,6 +101,12 @@ static void gic_clear_pending(GICState *s, int irq, int cm,
> uint8_t src)  {
>      unsigned cpu;
> 
> +    /* If a level-triggered interrupt has been set to pending through the
> +     * GICD_SPENDR, then a falling edge does not clear the pending state.
> +     */
> +    if (GIC_TEST_SW_PENDING(irq, cm))
> +        return;
> +
>      GIC_CLEAR_PENDING(irq, cm);
>      if (irq < GIC_NR_SGIS) {
>          cpu = (unsigned)ffs(cm) - 1;
> @@ -177,8 +183,9 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
>      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(s, new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm,
> -                                  GIC_SGI_SRC(new_irq, cpu));
> +    cm = GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm;
> +    GIC_CLEAR_SW_PENDING(new_irq, cm);
> +    gic_clear_pending(s, new_irq, cm, GIC_SGI_SRC(new_irq, cpu));
>      gic_set_running_irq(s, cpu, new_irq);
>      DPRINTF("ACK %d\n", new_irq);
>      return new_irq;
> @@ -445,16 +452,23 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>          for (i = 0; i < 8; i++) {
>              if (value & (1 << i)) {
>                  GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
> +                if (!GIC_TEST_TRIGGER(irq + i)) {
> +                    GIC_SET_SW_PENDING(irq + i, GIC_TARGET(irq + i));
> +                }
>              }
>          }
>      } 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++, irq++) {
>              if (irq > GIC_NR_SGIS && value & (1 << i)) {
> -                gic_clear_pending(s, irq, 1 << cpu, 0);
> +                GIC_CLEAR_SW_PENDING(irq, cm);
> +                if (GIC_TEST_TRIGGER(irq + i) || !GIC_TEST_LEVEL(irq, cm)) {
> +                    GIC_CLEAR_PENDING(irq, cm);
> +                }
>              }
>          }
>      } else if (offset < 0x400) {
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c index
> 1d3b738..7f0615f 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 f9133b9..173c607 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -43,6 +43,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) @@ -65,6 +68,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.1.2
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm

Re: [Qemu-devel] [PATCH] arm_gic: Keep track of GICD_CPENDR and GICD_SPENDR

[Christoffer Dall]

On Tue, Oct 29, 2013 at 04:10:59PM +0000, Bhushan Bharat-R65777 wrote:
> Hi Christoffer,
> 
> Not related to the patch, for edge type of interrupt, will setting bit in ICD_SPENDR generate interrupt?
> 
Hi Bharat,

That depends, if the interrupt was not previously pending, setting the
bit for the corresponding interrupt will cause it to be pending, if the
interrupt is also enabled and has high enough priority, then the
interrupt gets forwarded to the CPU interface and if the corresponding
CPU is not masking interrupts, it will cause an interrupt on the CPU.

The same is actually valid for level-triggered interrupts too, however,
the difference is that if the interrupt was already pending, it has no
effect whatsoever on edge-triggered interrupts, but level-triggered
interrupts will stay pending even if the external line is deasserted.

-Christoffer

> 
> > -----Original Message-----
> > From: Christoffer Dall [mailto:christoffer.dall@linaro.org]
> > Sent: Wednesday, October 23, 2013 8:57 PM
> > To: peter.maydell@linaro.org
> > Cc: patches@linaro.org; qemu-devel@nongnu.org; kvmarm@lists.cs.columbia.edu
> > Subject: [PATCH] arm_gic: Keep track of GICD_CPENDR and GICD_SPENDR
> > 
> > If software writes to the ISPENDR and sets the pending state of a level-
> > triggered interrupt, the falling edge of the hardware input must not clear the
> > pending state.  Conversely, if software writes to the ICPENDR, the pending state
> > of a level-triggered interrupt should only be cleared if the hardware input is
> > not asserted.
> > 
> > This requires an extra state variable to keep track of software writes.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> >  hw/intc/arm_gic.c        | 20 +++++++++++++++++---
> >  hw/intc/arm_gic_common.c |  5 +++--
> >  hw/intc/gic_internal.h   |  4 ++++
> >  3 files changed, 24 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index d1ddac1..db54061 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -101,6 +101,12 @@ static void gic_clear_pending(GICState *s, int irq, int cm,
> > uint8_t src)  {
> >      unsigned cpu;
> > 
> > +    /* If a level-triggered interrupt has been set to pending through the
> > +     * GICD_SPENDR, then a falling edge does not clear the pending state.
> > +     */
> > +    if (GIC_TEST_SW_PENDING(irq, cm))
> > +        return;
> > +
> >      GIC_CLEAR_PENDING(irq, cm);
> >      if (irq < GIC_NR_SGIS) {
> >          cpu = (unsigned)ffs(cm) - 1;
> > @@ -177,8 +183,9 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
> >      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(s, new_irq, GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm,
> > -                                  GIC_SGI_SRC(new_irq, cpu));
> > +    cm = GIC_TEST_MODEL(new_irq) ? ALL_CPU_MASK : cm;
> > +    GIC_CLEAR_SW_PENDING(new_irq, cm);
> > +    gic_clear_pending(s, new_irq, cm, GIC_SGI_SRC(new_irq, cpu));
> >      gic_set_running_irq(s, cpu, new_irq);
> >      DPRINTF("ACK %d\n", new_irq);
> >      return new_irq;
> > @@ -445,16 +452,23 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
> >          for (i = 0; i < 8; i++) {
> >              if (value & (1 << i)) {
> >                  GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
> > +                if (!GIC_TEST_TRIGGER(irq + i)) {
> > +                    GIC_SET_SW_PENDING(irq + i, GIC_TARGET(irq + i));
> > +                }
> >              }
> >          }
> >      } 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++, irq++) {
> >              if (irq > GIC_NR_SGIS && value & (1 << i)) {
> > -                gic_clear_pending(s, irq, 1 << cpu, 0);
> > +                GIC_CLEAR_SW_PENDING(irq, cm);
> > +                if (GIC_TEST_TRIGGER(irq + i) || !GIC_TEST_LEVEL(irq, cm)) {
> > +                    GIC_CLEAR_PENDING(irq, cm);
> > +                }
> >              }
> >          }
> >      } else if (offset < 0x400) {
> > diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c index
> > 1d3b738..7f0615f 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 f9133b9..173c607 100644
> > --- a/hw/intc/gic_internal.h
> > +++ b/hw/intc/gic_internal.h
> > @@ -43,6 +43,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) @@ -65,6 +68,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.1.2
> > 
> > _______________________________________________
> > kvmarm mailing list
> > kvmarm@lists.cs.columbia.edu
> > https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
> 
> 

-- 
Christoffer

Re: [Qemu-devel] [RFC PATCH v2 3/6] hw: arm_gic: Keep track of SGI sources

[Christoffer Dall]

On Mon, Oct 14, 2013 at 04:36:24PM +0100, Peter Maydell wrote:
> On 26 September 2013 22:03, 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.
> 
> Shouldn't the state you have in sgi_source[][] be surfaced as the
> GICD_CPENDSGIR/GICD_SPENDSGIR registers [for a v2 GIC; they don't
> exist in v1]? It might then be better to represent the state in
> our data structures in the same layout as the registers.
> 

Hmm, those registers actually don't represent which CPU is the *source*
of a given SGI.  I think the array I propose is quite reasonable...

> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >
> > ---
> >
> > Changelog [v2]:
> >  - Fixed endless loop bug
> >  - Bump version_id and minimum_version_id on vmstate struct
> > ---
> >  hw/intc/arm_gic.c        |   41 ++++++++++++++++++++++++++++++++---------
> >  hw/intc/arm_gic_common.c |    5 +++--
> >  hw/intc/gic_internal.h   |    3 +++
> >  3 files changed, 38 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> > index 7eaa55f..6470d37 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -97,6 +97,20 @@ void gic_set_pending_private(GICState *s, int cpu, int irq)
> >      gic_update(s);
> >  }
> >
> > +static void gic_clear_pending(GICState *s, int irq, int cm, uint8_t src)
> > +{
> 
> I think that in the cases where we pass in 0 for src that the
> irq can't be < GIC_NR_SGIS.
> 

not quite sure what you mean by this comment, we pass in 0 for src in
two cases: 1) when we are dealing with something else than an SGI
(irq >= GIC_NR_SGIS), but we could pass whatever, the value doesn't
make sense here 2) when we are dealing with an SGI, the source can be 0
up to s->num_cpu - 1.

> > +    unsigned cpu;
> > +
> > +    GIC_CLEAR_PENDING(irq, cm);
> > +    if (irq < GIC_NR_SGIS) {
> > +        cpu = (unsigned)ffs(cm) - 1;
> 
> If you used ctz32() rather than ffs() it would save you having to
> subtract one all the time. Also, those unsigned casts are pretty
> ugly: better to just make 'cpu' an int...
> 

yeah, that was quite ridiculous.

> > +        while (cpu < NCPU) {
> > +            s->sgi_source[irq][cpu] &= ~(1 << src);
> > +            cpu = (unsigned)ffs(cm) - 1;
> > +        }
> 
> ...this still seems to be an infinite loop: cm isn't modified
> inside the loop so cpu will always have the same value each time.
> 
>      610:       eb fe                   jmp    610 <gic_clear_pending+0x50>
> 

if only I had for_each_set_bit - does QEMU have something sane for this?
Anyway, I tried again to fix this.  Thanks for spotting my broken fix
for my broken code.

> > +    }
> 
> Are you sure the logic in this function is right? (ie that we
> should only clear the sgi_source[][] bit for this source, and
> not completely? If nothing else, I think the interrupt should
> stay pending if some other source cpu still wants it to be
> pending. That is, I think we need to track the pending status
> separately for each (irq,target-cpu,source-cpu) separately for
> SGIs. (I'm not totally sure I have this right though, the spec
> is quite confusing.)
> 

no, you're right, for SGIs we need to loop through the sgi_source array
and make sure the irq is not pending on any CPUs from any CPUs.

> > +}
> > +
> 
> >  /* Maximum number of possible CPU interfaces, determined by GIC architecture */
> >  #define NCPU 8
> >
> > @@ -58,6 +59,7 @@
> >                                      s->priority1[irq][cpu] :            \
> >                                      s->priority2[(irq) - GIC_INTERNAL])
> >  #define GIC_TARGET(irq) s->irq_target[irq]
> > +#define GIC_SGI_SRC(irq, cpu) (((irq) < GIC_NR_SGIS) ? ffs(s->sgi_source[irq][cpu]) - 1 : 0)
> 
> WARNING: line over 80 characters
> #161: FILE: hw/intc/gic_internal.h:62:
> +#define GIC_SGI_SRC(irq, cpu) (((irq) < GIC_NR_SGIS) ?
> ffs(s->sgi_source[irq][cpu]) - 1 : 0)
> 

Thans,
-Christoffer

Re: [Qemu-devel] [RFC PATCH v2 6/6] hw: arm_gic_kvm: Add KVM VGIC save/restore logic

[Christoffer Dall]

On Fri, Sep 27, 2013 at 09:11:18AM +0100, Alex Bennée wrote:
> 
> christoffer.dall@linaro.org writes:
> 

[...]

> > +
> > +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) {
> 
> Should 1, 0x2 etc be #define'd constants?
> 
> <snip>

It would be a little tedious.  The whole bunch of emulation files for
the GIC here are sort of predicated on the fact that you know how to
find the GIC manual and can see the register encodings and determine the
offsets.

There's also very little reuse for these, as this is the only place
where we translate the qemu-specific data structure format to the KVM
API format.

That being said, I wouldn't mind cleaning up a lot of this code and
adding defines for the register offsets would at least be very helpful,
but for now I'm sticking with the current style, trying to get this code
finished, and then I'll follow up with a cleanup patch set, which takes
care of the whole lot - not just the KVM interface specific bits.

> >  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_source -> ICD_CPENDSGIRn */
> > +    kvm_dist_put(s, 0xf10, 8, GIC_NR_SGIS, translate_clear);
> > +    kvm_dist_put(s, 0xf20, 8, GIC_NR_SGIS, translate_sgisource);
> 
> Again what are these magic numbers? I assume the comments refer to the
> actual reg names in the manual? Perhaps putting a reference to the
> manual if these values aren't to be used else where?
> 
These are offsets (as indicated by the parameter name in kvm_dist_put)
for the corresponding registers, and yes, the comments are the reg names
used in the manual.  As I said above, you're lost here if you're not
looking at the manual while looking at the code, but I really do want to
clean up this part and share the offsets with arm_gic.c later, add nice
references to the manual and a whole bunch of other stuff that could be
made much nicer, but I'd really like to get this functional piece of
code in first.

Thanks,
-Christoffer

Re: [Qemu-devel] [RFC PATCH v2 6/6] hw: arm_gic_kvm: Add KVM VGIC save/restore logic

[Christoffer Dall]

On Tue, Oct 15, 2013 at 12:15:03PM +0100, Peter Maydell wrote:
> On 26 September 2013 22:03, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > 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.
> >
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> >
> > 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_common.c |    5 +-
> >  hw/intc/arm_gic_kvm.c    |  424 +++++++++++++++++++++++++++++++++++++++++++++-
> >  hw/intc/gic_internal.h   |    8 +
> >  3 files changed, 433 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> > index 5449d77..1d3b738 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, NCPU),
> >          VMSTATE_UINT8_ARRAY(bpr, GICState, NCPU),
> >          VMSTATE_UINT8_ARRAY(abpr, GICState, NCPU),
> > +        VMSTATE_UINT32_2DARRAY(apr, GICState, 4, NCPU),
> 
> I feel like we should add this new apr state (plus some
> documentation and at least the TCG read/write interface
> to the state) in one patch and then put the save/load
> in its own patch.
> 

Sounds reasonable, I've added a separate patch for the next series.

> > +    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();
> 
> Bad indent.
> 

indeed, sorry about that.

> > +    }
> > +}
> >  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_source -> 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);
> > +        }
> > +    }
> >  }
> 
> So an interesting question here is "is there state we currently
> save/restore for the TCG GIC which we're not passing to the
> kernel, and if so why?". I think the fields we don't do anything
> with are:
>  gic_irq_state::model

the model field specifies if the interrup uses the 1:N or the N:N model.
In reasonably recent versions of the GIC all SGIs are N:N and all
peripheral interrupts are 1:N and it is therefore a static property for
a GIC v2.0 that we don't need to save and restore.  QEMU's model does
seem to emulate an older version and a newer version at the same time,
and this logic is slightly broken for the ICFGRn handling, but this
would require a discussion of exactly what QEMU should be supporting and
what we may break if we decide to support one thing vs. another.

>  GICState::last_active
>  GICState::running_irq
>  GICState::running_priority

As far as I understand the code, these three are used to implement the
running priority support on the QEMU emulated CPU interface side.  This
state is implemented in hardware for the virtual CPU interface for the
VGIC based on the information in the list registers and the
GICD_ISACTIVER and registers.  I think the QEMU code code be changed to
look at the active state of the registers and with a coherent CPU
affinity model should be able to reproduce this state on the fly without
keeping these extra state variables around, but I'm not 100% confident.

>  GICState::current_pending

Again, the virtual CPU register generates this state based on the LRs
present on the interface.  The in-kernel distributor emulation needs to
choose the right interrupts to queue based on priority if prorities were
to be supported in the kernel.  The necessary state, however, is
preserved through the registers that we do save an restore.

> 
> I'm guessing these are a mix of "kernel doesn't implement
> something" and "TCG model is wrong and this state should
> show up somewhere else"; but which is which?

So I think the answer to all of the above is that this is implementation
state that QEMU carries because it makes it more efficient/easier or
just because that was the way it was written, and the kernel does
maintain this state via the current registers being saved and restored.

This of course implies that the QEMU state is somewhat redundant, but
this may be on purpose.

Do you feel it is necessary for this sort of documentation to go into
the source somewhere, or would you just like to clear it up for the
review?


> > diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> > index 758b85a..f9133b9 100644
> > --- a/hw/intc/gic_internal.h
> > +++ b/hw/intc/gic_internal.h
> > @@ -99,6 +99,14 @@ typedef struct GICState {
> >      uint8_t  bpr[NCPU];
> >      uint8_t  abpr[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 one or more interrupts for preemption level X are active, then
> > +     *   APRn[X mod 32] == 0b1,  where n = X / 32
> > +     * otherwise the bit is clear.
> > +     */
> > +    uint32_t apr[4][NCPU];
> 
> You can delete the "or more" -- there can onyl be one
> interrupt active at each group priority (GICv2 spec 3.3.3).
> 
> Also, where does the hardcoded 4 come from? Answer: it's
> the worst-case number of GICC_APRn registers, which happens
> if you have 7 bits of group priority, which means you have
> 128 preemption levels, which at one bit per level fits into
> 4 32 bit APR regs. I think a GIC_NR_APRS #define might be nice.
> 

Fair enough, I added one in the separate patch based on a new define for
max number of preemption levels.

> > +
> >      uint32_t num_cpu;
> >
> >      MemoryRegion iomem; /* Distributor */
> > --
> > 1.7.10.4

Thanks for the review!
-Christoffer