[Qemu-devel] [RFC PATCH v3 00/10] Support arm-gic-kvm save/restore

[Qemu-devel] [RFC PATCH v3 00/10] Support arm-gic-kvm save/restore

[Christoffer Dall]

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

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

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

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

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

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

Christoffer Dall (10):
  hw: arm_gic: Fix gic_set_irq handling
  hw: arm_gic: Introduce gic_set_priority function
  hw: arm_gic: Keep track of SGI sources
  arm_gic: Support setting/getting binary point reg
  arm_gic: Rename GIC_X_TRIGGER to GIC_X_EDGE_TRIGGER
  arm_gic: Keep track of GICD_CPENDR and GICD_SPENDR
  arm_gic: Fix gic_acknowledge_irq pending bit clear
  vmstate: Add uint32 2D-array support
  arm_gic: Add GICC_APRn state to the GICState
  hw: arm_gic_kvm: Add KVM VGIC save/restore logic

 hw/intc/arm_gic.c                | 133 +++++++++---
 hw/intc/arm_gic_common.c         |  15 +-
 hw/intc/arm_gic_kvm.c            | 424 ++++++++++++++++++++++++++++++++++++++-
 hw/intc/gic_internal.h           |  13 +-
 include/hw/intc/arm_gic_common.h |  21 ++
 include/migration/vmstate.h      |   6 +
 6 files changed, 569 insertions(+), 43 deletions(-)

-- 
1.8.4.3

[Qemu-devel] [RFC PATCH v3 01/10] 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.8.4.3

[Qemu-devel] [RFC PATCH v3 02/10] hw: arm_gic: Introduce gic_set_priority function

[Christoffer Dall]

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

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

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 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 3989fd1..f916725 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -61,5 +61,6 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu);
 void gic_complete_irq(GICState *s, int cpu, int irq);
 void gic_update(GICState *s);
 void gic_init_irqs_and_distributor(GICState *s, int num_irq);
+void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
 
 #endif /* !QEMU_ARM_GIC_INTERNAL_H */
-- 
1.8.4.3

[Qemu-devel] [RFC PATCH v3 03/10] 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.

Also fixes a subtle ancient bug in handling of writes to the GICD_SPENDr
where the irq variable was set to 0 if the IRQ was an SGI (irq < 16),
but the intention was to set the written value, the "value" variable, to
0.  Make the check explicit instead and ignore any such writes.

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

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

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

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 7eaa55f..2ed9a1a 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -97,6 +97,32 @@ 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)
+{
+    int cpu;
+
+    DPRINTF("%s: irq %d, cm 0x%x, src %d\n", __func__, irq, cm, src);
+    if (irq < GIC_NR_SGIS) {
+        bool pend = false;
+
+        for (cpu = 0; cpu < s->num_cpu; cpu++) {
+            if (cm & (1 << cpu)) {
+                s->sgi_source[irq][cpu] &= ~(1 << src);
+            } else {
+                if (s->sgi_source[irq][cpu]) {
+                    pend = true;
+                }
+            }
+        }
+
+        if (!pend) {
+            GIC_CLEAR_PENDING(irq, cm);
+        }
+    } else {
+        GIC_CLEAR_PENDING(irq, cm);
+    }
+}
+
 /* Process a change in an external IRQ input.  */
 static void gic_set_irq(void *opaque, int irq, int level)
 {
@@ -132,7 +158,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 +189,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;
@@ -424,12 +451,9 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        if (irq < 16)
-          irq = 0;
-
-        for (i = 0; i < 8; i++) {
-            if (value & (1 << i)) {
-                GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
+        for (i = 0; i < 8; i++, irq++) {
+            if (irq >= GIC_NR_SGIS && value & (1 << i)) {
+                GIC_SET_PENDING(irq, GIC_TARGET(irq));
             }
         }
     } else if (offset < 0x300) {
@@ -437,12 +461,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 +536,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 +556,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 < GIC_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 +579,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 +590,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 c765850..d1a1b0f 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -58,8 +58,8 @@ static const VMStateDescription vmstate_gic_irq_state = {
 
 static const VMStateDescription vmstate_gic = {
     .name = "arm_gic",
-    .version_id = 4,
-    .minimum_version_id = 4,
+    .version_id = 5,
+    .minimum_version_id = 5,
     .pre_save = gic_pre_save,
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
@@ -71,6 +71,7 @@ static const VMStateDescription vmstate_gic = {
         VMSTATE_UINT8_2DARRAY(priority1, GICState, GIC_INTERNAL, GIC_NCPU),
         VMSTATE_UINT8_ARRAY(priority2, GICState, GIC_MAXIRQ - GIC_INTERNAL),
         VMSTATE_UINT16_2DARRAY(last_active, GICState, GIC_MAXIRQ, GIC_NCPU),
+        VMSTATE_UINT8_2DARRAY(sgi_source, GICState, GIC_NR_SGIS, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(priority_mask, GICState, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(running_irq, GICState, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(running_priority, GICState, GIC_NCPU),
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index f916725..3d36653 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -51,6 +51,9 @@
                                     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)
 
 /* The special cases for the revision property: */
 #define REV_11MPCORE 0
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index ed18bb8..e19481b 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -27,6 +27,7 @@
 #define GIC_MAXIRQ 1020
 /* First 32 are private to each CPU (SGIs and PPIs). */
 #define GIC_INTERNAL 32
+#define GIC_NR_SGIS  16
 /* Maximum number of possible CPU interfaces, determined by GIC architecture */
 #define GIC_NCPU 8
 
@@ -54,6 +55,7 @@ typedef struct GICState {
     uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
     uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
     uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
+    uint8_t sgi_source[GIC_NR_SGIS][GIC_NCPU];
 
     uint16_t priority_mask[GIC_NCPU];
     uint16_t running_irq[GIC_NCPU];
-- 
1.8.4.3

[Qemu-devel] [RFC PATCH v3 04/10] 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 [v3]:
 - Treat writes for GIC prior to v2 as RAZ/WI.

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

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 2ed9a1a..73acf62 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -587,8 +587,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;
@@ -597,6 +596,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);
@@ -615,10 +616,15 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
         s->priority_mask[cpu] = (value & 0xff);
         break;
     case 0x08: /* Binary Point */
-        /* ??? Not implemented.  */
+        s->bpr[cpu] = (value & 0x7);
         break;
     case 0x10: /* End Of Interrupt */
         return gic_complete_irq(s, cpu, value & 0x3ff);
+    case 0x1c: /* Aliased Binary Point */
+        if (s->revision >= 2) {
+            s->abpr[cpu] = (value & 0x7);
+        }
+        break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "gic_cpu_write: Bad offset %x\n", (int)offset);
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index d1a1b0f..6fbdafc 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -58,8 +58,8 @@ static const VMStateDescription vmstate_gic_irq_state = {
 
 static const VMStateDescription vmstate_gic = {
     .name = "arm_gic",
-    .version_id = 5,
-    .minimum_version_id = 5,
+    .version_id = 6,
+    .minimum_version_id = 6,
     .pre_save = gic_pre_save,
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
@@ -76,6 +76,8 @@ static const VMStateDescription vmstate_gic = {
         VMSTATE_UINT16_ARRAY(running_irq, GICState, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(running_priority, GICState, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(current_pending, GICState, GIC_NCPU),
+        VMSTATE_UINT8_ARRAY(bpr, GICState, GIC_NCPU),
+        VMSTATE_UINT8_ARRAY(abpr, GICState, GIC_NCPU),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index e19481b..120d6b2 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -62,6 +62,13 @@ typedef struct GICState {
     uint16_t running_priority[GIC_NCPU];
     uint16_t current_pending[GIC_NCPU];
 
+    /* We present the GICv2 without security extensions to a guest and
+     * therefore the guest can configure the GICC_CTLR to configure group 1
+     * binary point in the abpr.
+     */
+    uint8_t  bpr[GIC_NCPU];
+    uint8_t  abpr[GIC_NCPU];
+
     uint32_t num_cpu;
 
     MemoryRegion iomem; /* Distributor */
-- 
1.8.4.3

[Qemu-devel] [RFC PATCH v3 05/10] arm_gic: Rename GIC_X_TRIGGER to GIC_X_EDGE_TRIGGER

[Christoffer Dall]

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

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

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 73acf62..5736b95 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -157,7 +157,7 @@ static void gic_set_irq(void *opaque, int irq, int level)
         DPRINTF("Set %d pending mask %x\n", irq, target);
         GIC_SET_PENDING(irq, target);
     } else {
-        if (!GIC_TEST_TRIGGER(irq)) {
+        if (!GIC_TEST_EDGE_TRIGGER(irq)) {
             gic_clear_pending(s, irq, target, 0);
         }
         GIC_CLEAR_LEVEL(irq, cm);
@@ -225,7 +225,7 @@ void gic_complete_irq(GICState *s, int cpu, int irq)
         return; /* No active IRQ.  */
     /* Mark level triggered interrupts as pending if they are still
        raised.  */
-    if (!GIC_TEST_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
+    if (!GIC_TEST_EDGE_TRIGGER(irq) && GIC_TEST_ENABLED(irq, cm)
         && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) {
         DPRINTF("Set %d pending mask %x\n", irq, cm);
         GIC_SET_PENDING(irq, cm);
@@ -348,7 +348,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
         for (i = 0; i < 4; i++) {
             if (GIC_TEST_MODEL(irq + i))
                 res |= (1 << (i * 2));
-            if (GIC_TEST_TRIGGER(irq + i))
+            if (GIC_TEST_EDGE_TRIGGER(irq + i))
                 res |= (2 << (i * 2));
         }
     } else if (offset < 0xfe0) {
@@ -423,7 +423,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
                 /* If a raised level triggered IRQ enabled then mark
                    is as pending.  */
                 if (GIC_TEST_LEVEL(irq + i, mask)
-                        && !GIC_TEST_TRIGGER(irq + i)) {
+                        && !GIC_TEST_EDGE_TRIGGER(irq + i)) {
                     DPRINTF("Set %d pending mask %x\n", irq + i, mask);
                     GIC_SET_PENDING(irq + i, mask);
                 }
@@ -505,9 +505,9 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
                 GIC_CLEAR_MODEL(irq + i);
             }
             if (value & (2 << (i * 2))) {
-                GIC_SET_TRIGGER(irq + i);
+                GIC_SET_EDGE_TRIGGER(irq + i);
             } else {
-                GIC_CLEAR_TRIGGER(irq + i);
+                GIC_CLEAR_EDGE_TRIGGER(irq + i);
             }
         }
     } else {
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 6fbdafc..41ddc9b 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -129,7 +129,7 @@ static void arm_gic_common_reset(DeviceState *dev)
     }
     for (i = 0; i < 16; i++) {
         GIC_SET_ENABLED(i, ALL_CPU_MASK);
-        GIC_SET_TRIGGER(i);
+        GIC_SET_EDGE_TRIGGER(i);
     }
     if (s->num_cpu == 1) {
         /* For uniprocessor GICs all interrupts always target the sole CPU */
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 3d36653..5471749 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -44,9 +44,9 @@
 #define GIC_SET_LEVEL(irq, cm) s->irq_state[irq].level = (cm)
 #define GIC_CLEAR_LEVEL(irq, cm) s->irq_state[irq].level &= ~(cm)
 #define GIC_TEST_LEVEL(irq, cm) ((s->irq_state[irq].level & (cm)) != 0)
-#define GIC_SET_TRIGGER(irq) s->irq_state[irq].trigger = true
-#define GIC_CLEAR_TRIGGER(irq) s->irq_state[irq].trigger = false
-#define GIC_TEST_TRIGGER(irq) s->irq_state[irq].trigger
+#define GIC_SET_EDGE_TRIGGER(irq) s->irq_state[irq].trigger = true
+#define GIC_CLEAR_EDGE_TRIGGER(irq) s->irq_state[irq].trigger = false
+#define GIC_TEST_EDGE_TRIGGER(irq) (s->irq_state[irq].trigger)
 #define GIC_GET_PRIORITY(irq, cpu) (((irq) < GIC_INTERNAL) ?            \
                                     s->priority1[irq][cpu] :            \
                                     s->priority2[(irq) - GIC_INTERNAL])
-- 
1.8.4.3

[Qemu-devel] [RFC PATCH v3 06/10] 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           |  3 +++
 include/hw/intc/arm_gic_common.h |  1 +
 4 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 5736b95..9811161 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -119,6 +119,12 @@ static void gic_clear_pending(GICState *s, int irq, int cm, uint8_t src)
             GIC_CLEAR_PENDING(irq, cm);
         }
     } else {
+        /* 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);
     }
 }
@@ -189,8 +195,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;
@@ -454,16 +461,23 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         for (i = 0; i < 8; i++, irq++) {
             if (irq >= GIC_NR_SGIS && value & (1 << i)) {
                 GIC_SET_PENDING(irq, GIC_TARGET(irq));
+                if (!GIC_TEST_EDGE_TRIGGER(irq)) {
+                    GIC_SET_SW_PENDING(irq, GIC_TARGET(irq));
+                }
             }
         }
     } else if (offset < 0x300) {
+        int cm = (1 << cpu);
         /* Interrupt Clear Pending.  */
         irq = (offset - 0x280) * 8 + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
         for (i = 0; i < 8; i++, 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_EDGE_TRIGGER(irq) || !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 41ddc9b..65e3dc8 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 5471749..b04cb50 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -35,6 +35,9 @@
 #define GIC_SET_PENDING(irq, cm) s->irq_state[irq].pending |= (cm)
 #define GIC_CLEAR_PENDING(irq, cm) s->irq_state[irq].pending &= ~(cm)
 #define GIC_TEST_PENDING(irq, cm) ((s->irq_state[irq].pending & (cm)) != 0)
+#define GIC_SET_SW_PENDING(irq, cm) (s->irq_state[irq].sw_pending |= (cm))
+#define GIC_CLEAR_SW_PENDING(irq, cm) (s->irq_state[irq].sw_pending &= ~(cm))
+#define GIC_TEST_SW_PENDING(irq, cm) ((s->irq_state[irq].sw_pending & (cm)) != 0)
 #define GIC_SET_ACTIVE(irq, cm) s->irq_state[irq].active |= (cm)
 #define GIC_CLEAR_ACTIVE(irq, cm) s->irq_state[irq].active &= ~(cm)
 #define GIC_TEST_ACTIVE(irq, cm) ((s->irq_state[irq].active & (cm)) != 0)
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 120d6b2..2d6c23f 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -35,6 +35,7 @@ typedef struct gic_irq_state {
     /* The enable bits are only banked for per-cpu interrupts.  */
     uint8_t enabled;
     uint8_t pending;
+    uint8_t sw_pending; /* keep track of GICD_ISPENDR and GICD_ICPENDR writes */
     uint8_t active;
     uint8_t level;
     bool model; /* 0 = N:N, 1 = 1:N */
-- 
1.8.4.3

[Qemu-devel] [RFC PATCH v3 07/10] arm_gic: Fix gic_acknowledge_irq pending bit clear

[Christoffer Dall]

The pending flags for level-triggered interrupts should not be cleared
if the interrupt input signal remains asserted.

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

Changelog[3]:
 - New patch in the series
---
 hw/intc/arm_gic.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 9811161..20e926c 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -193,11 +193,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.  */
+    /* Clear pending flags for edge-triggered and non-asserted level-triggered
+     * interrupts.
+     */
     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));
+    if (GIC_TEST_EDGE_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;
-- 
1.8.4.3

[Qemu-devel] [RFC PATCH v3 08/10] vmstate: Add uint32 2D-array support

[Christoffer Dall]

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

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

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

[Qemu-devel] [RFC PATCH v3 09/10] arm_gic: Add GICC_APRn state to the GICState

[Christoffer Dall]

The GICC_APRn registers are not currently supported by the ARM GIC v2.0
emulation.  This patch adds the missing state.  The state was previously
added in the main vgic save/restore patch, but moved to a separate patch
as suggested under review.

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

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

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 20e926c..09f03e6 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -616,6 +616,8 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
         return s->current_pending[cpu];
     case 0x1c: /* Aliased Binary Point */
         return s->abpr[cpu];
+    case 0xd0: case 0xd4: case 0xd8: case 0xdc:
+        return s->apr[(offset - 0xd0) / 4][cpu];
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "gic_cpu_read: Bad offset %x\n", (int)offset);
@@ -643,6 +645,9 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
             s->abpr[cpu] = (value & 0x7);
         }
         break;
+    case 0xd0: case 0xd4: case 0xd8: case 0xdc:
+        DPRINTF("Writing APR not yet supported\n");
+        break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "gic_cpu_write: Bad offset %x\n", (int)offset);
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 65e3dc8..8dcb4fb 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -59,8 +59,8 @@ static const VMStateDescription vmstate_gic_irq_state = {
 
 static const VMStateDescription vmstate_gic = {
     .name = "arm_gic",
-    .version_id = 6,
-    .minimum_version_id = 6,
+    .version_id = 7,
+    .minimum_version_id = 7,
     .pre_save = gic_pre_save,
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
@@ -79,6 +79,7 @@ static const VMStateDescription vmstate_gic = {
         VMSTATE_UINT16_ARRAY(current_pending, GICState, GIC_NCPU),
         VMSTATE_UINT8_ARRAY(bpr, GICState, GIC_NCPU),
         VMSTATE_UINT8_ARRAY(abpr, GICState, GIC_NCPU),
+        VMSTATE_UINT32_2DARRAY(apr, GICState, GIC_NR_APRS, GIC_NCPU),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 2d6c23f..e815593 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -31,6 +31,9 @@
 /* Maximum number of possible CPU interfaces, determined by GIC architecture */
 #define GIC_NCPU 8
 
+#define MAX_NR_GROUP_PRIO 128
+#define GIC_NR_APRS (MAX_NR_GROUP_PRIO / 32)
+
 typedef struct gic_irq_state {
     /* The enable bits are only banked for per-cpu interrupts.  */
     uint8_t enabled;
@@ -70,6 +73,14 @@ typedef struct GICState {
     uint8_t  bpr[GIC_NCPU];
     uint8_t  abpr[GIC_NCPU];
 
+    /* The APR is implementation defined, so we choose a layout identical to
+     * the KVM ABI layout for QEMU's implementation of the gic:
+     * If an interrupt for preemption level X are active, then
+     *   APRn[X mod 32] == 0b1,  where n = X / 32
+     * otherwise the bit is clear.
+     */
+    uint32_t apr[GIC_NR_APRS][GIC_NCPU];
+
     uint32_t num_cpu;
 
     MemoryRegion iomem; /* Distributor */
-- 
1.8.4.3

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

Changelog [v2]:
 - Remove num_irq from GIC VMstate structure
 - Increment GIC VMstate version number
 - Use extract32/deposit32 for bit-field modifications
 - Address other smaller review comments
 - Renames kvm_arm_gic_dist_[readr/writer] functions to
   kvm_dist_[get/put] and shortened other function names
 - Use concrete format for APRn
---
 hw/intc/arm_gic_kvm.c | 424 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 422 insertions(+), 2 deletions(-)

diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 158f047..b98433e 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -3,6 +3,7 @@
  *
  * Copyright (c) 2012 Linaro Limited
  * Written by Peter Maydell
+ * Save/Restore logic added by Christoffer Dall.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -23,6 +24,20 @@
 #include "kvm_arm.h"
 #include "gic_internal.h"
 
+//#define DEBUG_GIC_KVM
+
+#ifdef DEBUG_GIC_KVM
+static const int debug_gic_kvm = 1;
+#else
+static const int debug_gic_kvm = 0;
+#endif
+
+#define DPRINTF(fmt, ...) do { \
+        if (debug_gic_kvm) { \
+            printf("arm_gic: " fmt , ## __VA_ARGS__); \
+        } \
+    } while (0)
+
 #define TYPE_KVM_ARM_GIC "kvm-arm-gic"
 #define KVM_ARM_GIC(obj) \
      OBJECT_CHECK(GICState, (obj), TYPE_KVM_ARM_GIC)
@@ -72,14 +87,419 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
     kvm_set_irq(kvm_state, kvm_irq, !!level);
 }
 
+static bool kvm_arm_gic_can_save_restore(GICState *s)
+{
+    return s->dev_fd >= 0;
+}
+
+static void kvm_gic_access(GICState *s, int group, int offset,
+                                   int cpu, uint32_t *val, bool write)
+{
+    struct kvm_device_attr attr;
+    int type;
+    int err;
+
+    cpu = cpu & 0xff;
+
+    attr.flags = 0;
+    attr.group = group;
+    attr.attr = (((uint64_t)cpu << KVM_DEV_ARM_VGIC_CPUID_SHIFT) &
+                 KVM_DEV_ARM_VGIC_CPUID_MASK) |
+                (((uint64_t)offset << KVM_DEV_ARM_VGIC_OFFSET_SHIFT) &
+                 KVM_DEV_ARM_VGIC_OFFSET_MASK);
+    attr.addr = (uintptr_t)val;
+
+    if (write) {
+        type = KVM_SET_DEVICE_ATTR;
+    } else {
+        type = KVM_GET_DEVICE_ATTR;
+    }
+
+    err = kvm_device_ioctl(s->dev_fd, type, &attr);
+    if (err < 0) {
+        fprintf(stderr, "KVM_{SET/GET}_DEVICE_ATTR failed: %s\n",
+                strerror(-err));
+        abort();
+    }
+}
+
+static void kvm_gicd_access(GICState *s, int offset, int cpu,
+                            uint32_t *val, bool write)
+{
+    kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_DIST_REGS,
+                   offset, cpu, val, write);
+}
+
+static void kvm_gicc_access(GICState *s, int offset, int cpu,
+                            uint32_t *val, bool write)
+{
+    kvm_gic_access(s, KVM_DEV_ARM_VGIC_GRP_CPU_REGS,
+                   offset, cpu, val, write);
+}
+
+#define for_each_irq_reg(_ctr, _max_irq, _field_width) \
+    for (_ctr = 0; _ctr < ((_max_irq) / (32 / (_field_width))); _ctr++)
+
+/*
+ * Translate from the in-kernel field for an IRQ value to/from the qemu
+ * representation.
+ */
+typedef void (*vgic_translate_fn)(GICState *s, int irq, int cpu,
+                                  uint32_t *field, bool to_kernel);
+
+/* synthetic translate function used for clear/set registers to completely
+ * clear a setting using a clear-register before setting the remaing bits
+ * using a set-register */
+static void translate_clear(GICState *s, int irq, int cpu,
+                            uint32_t *field, bool to_kernel)
+{
+    if (to_kernel) {
+        *field = ~0;
+    } else {
+        /* does not make sense: qemu model doesn't use set/clear regs */
+        abort();
+    }
+}
+
+static void translate_enabled(GICState *s, int irq, int cpu,
+                              uint32_t *field, bool to_kernel)
+{
+    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+
+    if (to_kernel) {
+        *field = GIC_TEST_ENABLED(irq, cm);
+    } else {
+        if (*field & 1) {
+            GIC_SET_ENABLED(irq, cm);
+        }
+    }
+}
+
+static void translate_pending(GICState *s, int irq, int cpu,
+                              uint32_t *field, bool to_kernel)
+{
+    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+
+    if (to_kernel) {
+        *field = GIC_TEST_PENDING(irq, cm);
+    } else {
+        if (*field & 1) {
+            GIC_SET_PENDING(irq, cm);
+            /* TODO: Capture is level-line is held high in the kernel */
+        }
+    }
+}
+
+static void translate_active(GICState *s, int irq, int cpu,
+                             uint32_t *field, bool to_kernel)
+{
+    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+
+    if (to_kernel) {
+        *field = GIC_TEST_ACTIVE(irq, cm);
+    } else {
+        if (*field & 1) {
+            GIC_SET_ACTIVE(irq, cm);
+        }
+    }
+}
+
+static void translate_trigger(GICState *s, int irq, int cpu,
+                              uint32_t *field, bool to_kernel)
+{
+    if (to_kernel) {
+        *field = (GIC_TEST_EDGE_TRIGGER(irq)) ? 0x2 : 0x0;
+    } else {
+        if (*field & 0x2) {
+            GIC_SET_EDGE_TRIGGER(irq);
+        }
+    }
+}
+
+static void translate_priority(GICState *s, int irq, int cpu,
+                               uint32_t *field, bool to_kernel)
+{
+    if (to_kernel) {
+        *field = GIC_GET_PRIORITY(irq, cpu) & 0xff;
+    } else {
+        gic_set_priority(s, cpu, irq, *field & 0xff);
+    }
+}
+
+static void translate_targets(GICState *s, int irq, int cpu,
+                              uint32_t *field, bool to_kernel)
+{
+    if (to_kernel) {
+        *field = s->irq_target[irq] & 0xff;
+    } else {
+        s->irq_target[irq] = *field & 0xff;
+    }
+}
+
+static void translate_sgisource(GICState *s, int irq, int cpu,
+                                uint32_t *field, bool to_kernel)
+{
+    if (to_kernel) {
+        *field = s->sgi_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)
-- 
1.8.4.3

Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling

[Peter Maydell]

On 19 November 2013 06:18, 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.
>
> 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);

So I think this is a correct change in the sense that
it's fixing the behaviour of this function. However
we seem to get our pending behaviour for level triggered
interrupts wrong in several places:
 * here
 * gic_acknowledge_irq (which you fix in a later patch)
 * gic_complete_irq, which currently says "enabled
   level triggered still-raised interrupts should be
   remarked as pending"

This feels to me like a cluster of errors which have come
from somebody's misreading of the spec and which probably
combine to produce a coherent not-too-far-from-correct
result, and which we should therefore fix all at once rather
than only partially.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v3 03/10] hw: arm_gic: Keep track of SGI sources

[Peter Maydell]

On 19 November 2013 06:18, 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.
>
> Also fixes a subtle ancient bug in handling of writes to the GICD_SPENDr
> where the irq variable was set to 0 if the IRQ was an SGI (irq < 16),
> but the intention was to set the written value, the "value" variable, to
> 0.  Make the check explicit instead and ignore any such writes.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
>
> Changelog [v3]:
>  - Changed ffs(x) - 1 to ctz32
>  - Changed cpu type to int in gic_clear_pending to avoid cast
>  - Really try to fix the endless loop bug
>  - Change gic_clear_pending to only clear the pending bit of SGIs if all
>    CPUs do not have that IRQ pending from any CPUs.
>  - Wrap long line in gic_internal.h
>  - Fix bug allowing setting SGIs through the GICD_SPENDR
>
> Changelog [v2]:
>  - Fixed endless loop bug
>  - Bump version_id and minimum_version_id on vmstate struct
> ---
>  hw/intc/arm_gic.c                | 62 ++++++++++++++++++++++++++++++----------
>  hw/intc/arm_gic_common.c         |  5 ++--
>  hw/intc/gic_internal.h           |  3 ++
>  include/hw/intc/arm_gic_common.h |  2 ++
>  4 files changed, 55 insertions(+), 17 deletions(-)

I realised that the way we store the state in the GICState struct
isn't actually very far from the way the hardware
does (ie like the banked GICD_SPENDSGIR*), but I had
to go look at the translate functions in the last patch
in this series to figure this out -- the GICD_SPENDSGIR*
are effectively the concatenation of the CPU's
sgi_source[irq][cpu] bits. I think we could make this a lot
more obvious by choice of struct member name and with a
comment in the GICState struct:

    /* For each SGI on the target CPU, we store 8 bits
     * indicating which source CPUs have made this SGI
     * pending on the target CPU. These correspond to
     * the bytes in the GIC_SPENDSGIR* registers as
     * read by the target CPU.
     */
    uint8_t sgi_pending[GIC_NR_SGIS][GIC_NCPU];

>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 7eaa55f..2ed9a1a 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -97,6 +97,32 @@ 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)
> +{
> +    int cpu;
> +
> +    DPRINTF("%s: irq %d, cm 0x%x, src %d\n", __func__, irq, cm, src);
> +    if (irq < GIC_NR_SGIS) {
> +        bool pend = false;
> +
> +        for (cpu = 0; cpu < s->num_cpu; cpu++) {
> +            if (cm & (1 << cpu)) {
> +                s->sgi_source[irq][cpu] &= ~(1 << src);
> +            } else {
> +                if (s->sgi_source[irq][cpu]) {
> +                    pend = true;
> +                }
> +            }
> +        }

This loop looks very odd. In general we should only be
clearing sgi_source[][] bits in two cases:
 (1) CPU X has just read its GICC_IAR for an interrupt ID,
     which happens to be an SGI with source CPU Y
 (2) CPU X wrote to its GICD_CPENDSGIR register

In the current code there is also a code path where we
might think we want to clear a pending SGI which
comes from gic_set_irq(). However this should I think
never actually happen, because it would represent
hardware setting a software-only interrupt. We should
probably assert in gic_set_irq() that the irq number
doesn't represent an SGI [or change everything using
GIC interrupt numbers to a different convention which
doesn't have irq line numbers for the SGIs, but urgh.].

So in either case we should know exactly both the
source CPU and the destination CPU number, so don't
need to loop around doing "for each cpu maybe
clear a bit" at all.

The destination CPU's overall pending status for
the SGI, which is the other thing we need to know,
is just the logical OR of the per-source pending
bits, so conveniently is just
  (s->sgi_source[irq][destcpu] != 0)

> +
> +        if (!pend) {
> +            GIC_CLEAR_PENDING(irq, cm);
> +        }
> +    } else {
> +        GIC_CLEAR_PENDING(irq, cm);
> +    }



> +}
> +
>  /* Process a change in an external IRQ input.  */
>  static void gic_set_irq(void *opaque, int irq, int level)
>  {
> @@ -132,7 +158,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 +189,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;
> @@ -424,12 +451,9 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>          irq = (offset - 0x200) * 8 + GIC_BASE_IRQ;
>          if (irq >= s->num_irq)
>              goto bad_reg;
> -        if (irq < 16)
> -          irq = 0;
> -
> -        for (i = 0; i < 8; i++) {
> -            if (value & (1 << i)) {
> -                GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
> +        for (i = 0; i < 8; i++, irq++) {
> +            if (irq >= GIC_NR_SGIS && value & (1 << i)) {

You might as well haul this test out of the loop, we know we
won't cross the SGI-to-not-SGI boundary in the middle of a byte.
Ditto below.

> +                GIC_SET_PENDING(irq, GIC_TARGET(irq));
>              }
>          }
>      } else if (offset < 0x300) {
> @@ -437,12 +461,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 +536,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 +556,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 < GIC_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 +579,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 +590,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;

We should just have gic_acknowledge_irq() return the
value with the source CPU bits set correctly -- after
all that function already needs to know which source
CPU it's selected in order to clear the right SGI
pending bit.

> +        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 c765850..d1a1b0f 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -58,8 +58,8 @@ static const VMStateDescription vmstate_gic_irq_state = {
>
>  static const VMStateDescription vmstate_gic = {
>      .name = "arm_gic",
> -    .version_id = 4,
> -    .minimum_version_id = 4,
> +    .version_id = 5,
> +    .minimum_version_id = 5,
>      .pre_save = gic_pre_save,
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {
> @@ -71,6 +71,7 @@ static const VMStateDescription vmstate_gic = {
>          VMSTATE_UINT8_2DARRAY(priority1, GICState, GIC_INTERNAL, GIC_NCPU),
>          VMSTATE_UINT8_ARRAY(priority2, GICState, GIC_MAXIRQ - GIC_INTERNAL),
>          VMSTATE_UINT16_2DARRAY(last_active, GICState, GIC_MAXIRQ, GIC_NCPU),
> +        VMSTATE_UINT8_2DARRAY(sgi_source, GICState, GIC_NR_SGIS, GIC_NCPU),
>          VMSTATE_UINT16_ARRAY(priority_mask, GICState, GIC_NCPU),
>          VMSTATE_UINT16_ARRAY(running_irq, GICState, GIC_NCPU),
>          VMSTATE_UINT16_ARRAY(running_priority, GICState, GIC_NCPU),
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index f916725..3d36653 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -51,6 +51,9 @@
>                                      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)

You could add a comment that it is IMPDEF which CPU we pick
when multiple source CPUs have asserted an SGI simultaneously.

Also using ctz32() would avoid that "- 1".

I assume we never call this macro unless there's really
guaranteed to be a pending bit in the sgi_source[][]
entry, but it's not totally obvious this is true :-)

>  /* The special cases for the revision property: */
>  #define REV_11MPCORE 0
> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
> index ed18bb8..e19481b 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -27,6 +27,7 @@
>  #define GIC_MAXIRQ 1020
>  /* First 32 are private to each CPU (SGIs and PPIs). */
>  #define GIC_INTERNAL 32
> +#define GIC_NR_SGIS  16
>  /* Maximum number of possible CPU interfaces, determined by GIC architecture */
>  #define GIC_NCPU 8
>
> @@ -54,6 +55,7 @@ typedef struct GICState {
>      uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
>      uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
>      uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
> +    uint8_t sgi_source[GIC_NR_SGIS][GIC_NCPU];
>
>      uint16_t priority_mask[GIC_NCPU];
>      uint16_t running_irq[GIC_NCPU];
> --
> 1.8.4.3
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm


thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v3 04/10] arm_gic: Support setting/getting binary point reg

[Peter Maydell]

On 19 November 2013 06:18, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> 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 [v3]:
>  - Treat writes for GIC prior to v2 as RAZ/WI.
>
> Changelog [v2]:
>  - Renamed binary_point to bpr and abpr
>  - Added GICC_ABPR read-as-write logic for TCG

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

-- PMM

Re: [Qemu-devel] [RFC PATCH v3 05/10] arm_gic: Rename GIC_X_TRIGGER to GIC_X_EDGE_TRIGGER

[Peter Maydell]

On 19 November 2013 06:18, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> TRIGGER can really mean mean anything (e.g. was it triggered, is it
> level-triggered, is it edge-triggered, etc.).  Rename to EDGE_TRIGGER to
> make the code comprehensible without looking up the data structure.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>

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

You might as well rename the 'trigger' struct field too...

-- PMM

Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling

[Peter Maydell]

On 28 November 2013 16:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 November 2013 06:18, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> So I think this is a correct change in the sense that
> it's fixing the behaviour of this function. However
> we seem to get our pending behaviour for level triggered
> interrupts wrong in several places:
>  * here
>  * gic_acknowledge_irq (which you fix in a later patch)
>  * gic_complete_irq, which currently says "enabled
>    level triggered still-raised interrupts should be
>    remarked as pending"
>
> This feels to me like a cluster of errors which have come
> from somebody's misreading of the spec and which probably
> combine to produce a coherent not-too-far-from-correct
> result, and which we should therefore fix all at once rather
> than only partially.

The other possibility is that it's a correct implementation
of 11MPCore GIC semantics -- the documentation of the
11MPCore definitely says that level triggered interrupts
go from Pending to Active and can't be Active+Pending
unless software messes with the GIC state. So either
the docs are actively wrong for 11MPCore or it behaves
differently from GICv1 and v2 here (my guess would be
the latter, in which case we need to support both flavours).

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v3 09/10] arm_gic: Add GICC_APRn state to the GICState

[Peter Maydell]

On 19 November 2013 06:18, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> The GICC_APRn registers are not currently supported by the ARM GIC v2.0
> emulation.  This patch adds the missing state.  The state was previously
> added in the main vgic save/restore patch, but moved to a separate patch
> as suggested under review.

Remarks about changes under code review go under the '---';
the commit message should ideally be a freestanding description
of the change.

> Note that we also change the number of APRs to use a define GIC_NR_APRS
> based on the maximum number of preemption levels.  This patch also adds
> RAZ/WI accessors for the four registers on the emulated CPU interface.
>
> Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> ---
>  hw/intc/arm_gic.c                |  5 +++++
>  hw/intc/arm_gic_common.c         |  5 +++--
>  include/hw/intc/arm_gic_common.h | 11 +++++++++++
>  3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 20e926c..09f03e6 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -616,6 +616,8 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
>          return s->current_pending[cpu];
>      case 0x1c: /* Aliased Binary Point */
>          return s->abpr[cpu];
> +    case 0xd0: case 0xd4: case 0xd8: case 0xdc:
> +        return s->apr[(offset - 0xd0) / 4][cpu];
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "gic_cpu_read: Bad offset %x\n", (int)offset);
> @@ -643,6 +645,9 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
>              s->abpr[cpu] = (value & 0x7);
>          }
>          break;
> +    case 0xd0: case 0xd4: case 0xd8: case 0xdc:
> +        DPRINTF("Writing APR not yet supported\n");

qemu_log_mask(LOG_UNIMP, ....);

> +        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 65e3dc8..8dcb4fb 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -59,8 +59,8 @@ static const VMStateDescription vmstate_gic_irq_state = {
>
>  static const VMStateDescription vmstate_gic = {
>      .name = "arm_gic",
> -    .version_id = 6,
> -    .minimum_version_id = 6,
> +    .version_id = 7,
> +    .minimum_version_id = 7,
>      .pre_save = gic_pre_save,
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {
> @@ -79,6 +79,7 @@ static const VMStateDescription vmstate_gic = {
>          VMSTATE_UINT16_ARRAY(current_pending, GICState, GIC_NCPU),
>          VMSTATE_UINT8_ARRAY(bpr, GICState, GIC_NCPU),
>          VMSTATE_UINT8_ARRAY(abpr, GICState, GIC_NCPU),
> +        VMSTATE_UINT32_2DARRAY(apr, GICState, GIC_NR_APRS, GIC_NCPU),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
> index 2d6c23f..e815593 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -31,6 +31,9 @@
>  /* Maximum number of possible CPU interfaces, determined by GIC architecture */
>  #define GIC_NCPU 8
>
> +#define MAX_NR_GROUP_PRIO 128
> +#define GIC_NR_APRS (MAX_NR_GROUP_PRIO / 32)
> +
>  typedef struct gic_irq_state {
>      /* The enable bits are only banked for per-cpu interrupts.  */
>      uint8_t enabled;
> @@ -70,6 +73,14 @@ typedef struct GICState {
>      uint8_t  bpr[GIC_NCPU];
>      uint8_t  abpr[GIC_NCPU];
>
> +    /* The APR is implementation defined, so we choose a layout identical to
> +     * the KVM ABI layout for QEMU's implementation of the gic:
> +     * If an interrupt for preemption level X are active, then

"is active"

> +     *   APRn[X mod 32] == 0b1,  where n = X / 32
> +     * otherwise the bit is clear.
> +     */
> +    uint32_t apr[GIC_NR_APRS][GIC_NCPU];
> +
>      uint32_t num_cpu;
>
>      MemoryRegion iomem; /* Distributor */
> --
> 1.8.4.3

thanks
-- PMM

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

[Peter Maydell]

On 19 November 2013 06:18, 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 [v3]:
>  - Separate patch for adding the APR register state
>
> Changelog [v2]:
>  - Remove num_irq from GIC VMstate structure
>  - Increment GIC VMstate version number
>  - Use extract32/deposit32 for bit-field modifications
>  - Address other smaller review comments
>  - Renames kvm_arm_gic_dist_[readr/writer] functions to
>    kvm_dist_[get/put] and shortened other function names
>  - Use concrete format for APRn

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

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling

[Christoffer Dall]

On Thu, Nov 28, 2013 at 04:17:43PM +0000, Peter Maydell wrote:
> On 19 November 2013 06:18, 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.
> >
> > 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);
> 
> So I think this is a correct change in the sense that
> it's fixing the behaviour of this function. However
> we seem to get our pending behaviour for level triggered
> interrupts wrong in several places:
>  * here
>  * gic_acknowledge_irq (which you fix in a later patch)
>  * gic_complete_irq, which currently says "enabled
>    level triggered still-raised interrupts should be
>    remarked as pending"
> 
> This feels to me like a cluster of errors which have come
> from somebody's misreading of the spec and which probably
> combine to produce a coherent not-too-far-from-correct
> result, and which we should therefore fix all at once rather
> than only partially.
> 
Fair enough, I'll try and combine these.

-Christoffer

Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling

[Christoffer Dall]

On Thu, Nov 28, 2013 at 05:43:54PM +0000, Peter Maydell wrote:
> On 28 November 2013 16:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 19 November 2013 06:18, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> > So I think this is a correct change in the sense that
> > it's fixing the behaviour of this function. However
> > we seem to get our pending behaviour for level triggered
> > interrupts wrong in several places:
> >  * here
> >  * gic_acknowledge_irq (which you fix in a later patch)
> >  * gic_complete_irq, which currently says "enabled
> >    level triggered still-raised interrupts should be
> >    remarked as pending"
> >
> > This feels to me like a cluster of errors which have come
> > from somebody's misreading of the spec and which probably
> > combine to produce a coherent not-too-far-from-correct
> > result, and which we should therefore fix all at once rather
> > than only partially.
> 
> The other possibility is that it's a correct implementation
> of 11MPCore GIC semantics -- the documentation of the
> 11MPCore definitely says that level triggered interrupts
> go from Pending to Active and can't be Active+Pending
> unless software messes with the GIC state. So either
> the docs are actively wrong for 11MPCore or it behaves
> differently from GICv1 and v2 here (my guess would be
> the latter, in which case we need to support both flavours).
> 
A correct implementation?  I don't see how, unless the pending/level
fields are used in some obscure different way for the 11MPCore than for
GICv2.0.  For the 11MPCore, shouldn't it be:

if (level) {
	GIC_SET_LEVEL(irq, cm);
	if (GIC_TEST_TRIGGER || !GIC_TEST_ACTIVE(irq, cm)) {
		GIC_SET_PENDING(irq, target);
	}
} else {
	GIC_CLEAR_LEVEL(irq, cm)
}

and then the acknowledge could should check if the level is high and we
are acking an active interrupt, make it pending instead of inactive?


That being said, we are absolutely sure that support the 11MPCore is
still needed?

I would probably go the route of creating some structs with function
pointers in them for things like the ack or raise etc. which have
different semantics for the two versions and have separate functions to
reduce the branching in each funciton.  What do you think?

-Christoffer

Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling

[Peter Maydell]

On 19 December 2013 05:49, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Thu, Nov 28, 2013 at 05:43:54PM +0000, Peter Maydell wrote:
>> The other possibility is that it's a correct implementation
>> of 11MPCore GIC semantics -- the documentation of the
>> 11MPCore definitely says that level triggered interrupts
>> go from Pending to Active and can't be Active+Pending
>> unless software messes with the GIC state. So either
>> the docs are actively wrong for 11MPCore or it behaves
>> differently from GICv1 and v2 here (my guess would be
>> the latter, in which case we need to support both flavours).
>>
> A correct implementation?  I don't see how, unless the pending/level
> fields are used in some obscure different way for the 11MPCore than for
> GICv2.0.

That is my point -- the 11MPCore docs say that pending works
differently.

> That being said, we are absolutely sure that support the 11MPCore is
> still needed?

I can't just rip it all out of QEMU, really, though it does count
as one of our less used CPUs I suspect.

> I would probably go the route of creating some structs with function
> pointers in them for things like the ack or raise etc. which have
> different semantics for the two versions and have separate functions to
> reduce the branching in each funciton.  What do you think?

My initial thought would be either to have if statements at the
relevant points (which is how we've handled 11mpcore
differences so far), or to bite the bullet and reflect the
difference in the QOM class structure so we can use
QOM methods [ie function pointers in the class struct].

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling

[Peter Crosthwaite]

On Thu, Dec 19, 2013 at 7:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 19 December 2013 05:49, Christoffer Dall <christoffer.dall@linaro.org> wrote:
>> On Thu, Nov 28, 2013 at 05:43:54PM +0000, Peter Maydell wrote:
>>> The other possibility is that it's a correct implementation
>>> of 11MPCore GIC semantics -- the documentation of the
>>> 11MPCore definitely says that level triggered interrupts
>>> go from Pending to Active and can't be Active+Pending
>>> unless software messes with the GIC state. So either
>>> the docs are actively wrong for 11MPCore or it behaves
>>> differently from GICv1 and v2 here (my guess would be
>>> the latter, in which case we need to support both flavours).
>>>
>> A correct implementation?  I don't see how, unless the pending/level
>> fields are used in some obscure different way for the 11MPCore than for
>> GICv2.0.
>
> That is my point -- the 11MPCore docs say that pending works
> differently.
>
>> That being said, we are absolutely sure that support the 11MPCore is
>> still needed?
>
> I can't just rip it all out of QEMU, really, though it does count
> as one of our less used CPUs I suspect.
>
>> I would probably go the route of creating some structs with function
>> pointers in them for things like the ack or raise etc. which have
>> different semantics for the two versions and have separate functions to
>> reduce the branching in each funciton.  What do you think?
>
> My initial thought would be either to have if statements at the
> relevant points (which is how we've handled 11mpcore
> differences so far), or to bite the bullet and reflect the
> difference in the QOM class structure so we can use
> QOM methods [ie function pointers in the class struct].
>

Even in the "if" approach its probably best to put the "is-11mpcore"
or "version" property in the class structure. So I think you want the
QOM class structure both ways.

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling

[Peter Maydell]

On 19 December 2013 13:49, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Dec 19, 2013 at 7:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> My initial thought would be either to have if statements at the
>> relevant points (which is how we've handled 11mpcore
>> differences so far), or to bite the bullet and reflect the
>> difference in the QOM class structure so we can use
>> QOM methods [ie function pointers in the class struct].
>>
>
> Even in the "if" approach its probably best to put the "is-11mpcore"
> or "version" property in the class structure. So I think you want the
> QOM class structure both ways.

We have precedent elsewhere for having a "revision" property
in the object struct rather than having subclasses per class, don't
we? (arm_gic already has such a property, it's the 'revision' field.)

Properties can't go in the class struct because by definition
they can be set per-instance by the creator of the object.

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling

[Peter Crosthwaite]

On Thu, Dec 19, 2013 at 11:59 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 19 December 2013 13:49, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Thu, Dec 19, 2013 at 7:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> My initial thought would be either to have if statements at the
>>> relevant points (which is how we've handled 11mpcore
>>> differences so far), or to bite the bullet and reflect the
>>> difference in the QOM class structure so we can use
>>> QOM methods [ie function pointers in the class struct].
>>>
>>
>> Even in the "if" approach its probably best to put the "is-11mpcore"
>> or "version" property in the class structure. So I think you want the
>> QOM class structure both ways.
>
> We have precedent elsewhere for having a "revision" property
> in the object struct rather than having subclasses per class, don't
> we? (arm_gic already has such a property, it's the 'revision' field.)

So given its already there, can you just cheat and get the revs right
and if on them?

Back to longer term though, I'm thinking of the sysbus EHCI as the
modern precedent. The small diffs between the incompatible
implementations are determined at class init time via which concrete
subclass you instantiated.

>
> Properties can't go in the class struct because by definition
> they can be set per-instance by the creator of the object.
>

You have multiple inherited classes. TYPE_ARM_GIC defines the class
property (maybe im looking for a better word than property here), and
TYPE_11MPCORE_GIC and friends set it in their class_init. Same as for
QOM abstract functions, just instead it's an abstract constant.

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [RFC PATCH v3 01/10] hw: arm_gic: Fix gic_set_irq handling

[Peter Maydell]

On 19 December 2013 14:26, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Dec 19, 2013 at 11:59 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> On 19 December 2013 13:49, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> On Thu, Dec 19, 2013 at 7:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> My initial thought would be either to have if statements at the
>>>> relevant points (which is how we've handled 11mpcore
>>>> differences so far), or to bite the bullet and reflect the
>>>> difference in the QOM class structure so we can use
>>>> QOM methods [ie function pointers in the class struct].
>>>>
>>>
>>> Even in the "if" approach its probably best to put the "is-11mpcore"
>>> or "version" property in the class structure. So I think you want the
>>> QOM class structure both ways.
>>
>> We have precedent elsewhere for having a "revision" property
>> in the object struct rather than having subclasses per class, don't
>> we? (arm_gic already has such a property, it's the 'revision' field.)
>
> So given its already there, can you just cheat and get the revs right
> and if on them?

That was the proposal, yes :-)

> Back to longer term though, I'm thinking of the sysbus EHCI as the
> modern precedent. The small diffs between the incompatible
> implementations are determined at class init time via which concrete
> subclass you instantiated.

Yeah, we could certainly do that. I was just hoping to avoid
doing a rework that created a pile of subclasses just for
another 11mpcore weirdness :-)

-- PMM