[Qemu-devel] [PATCH 0/5] arm_gic: Drop running_irq and last_active arrays

[Qemu-devel] [PATCH 0/5] arm_gic: Drop running_irq and last_active arrays

[Peter Maydell]

This patchset is a bit of cleanup to our GIC implementation that
I've wanted to do for ages.

Our current GIC code uses a couple of arrays (running_irq and
last_active) to track currently active interrupts so that
it can correctly determine the running priority as potentially
nested interrupts are taken and then dismissed. This does
work, but:
 * the effectively-a-linked-list is not very hardware-ish,
   which is usually a bit of a red flag when doing modelling
 * the GICv2 spec adds the Active Priority Registers which are
   for use for saving and restoring this sort of state, and
   implementing these properly effectively constrains you to
   an implementation that doesn't look like what we have now
 * it doesn't really fit with the GIC grouping and security
   extensions, where a guest can say "dismiss the last group 1
   interrupt" rather than just "dismiss the last interrupt".

This series gets rid of those arrays and instead uses the
Active Priority Registers to do the job. The APRs have one
bit per "preemption level" (ie per distinct group priority).
When we take an interrupt we set the appropriate bit to 1
(and this will always be the lowest set bit in the register,
because low-numbered priorities are higher and we wouldn't
have taken the interrupt unless it was higher than our current
priority). Similarly, when we dismiss an interrupt this just
clears the lowest set bit in the register, which must be the
current active interrupt. (It's important not to try to look
at the current configured priority of the interrupt number,
because the guest might have reconfigured it while it was
active.) The new running priority is then calculable by
looking at the new lowest set bit.

The new code also takes a step in the direction of
separating the idea of "priority drop" from "deactivate interrupt",
which we will need to implement the GICv2 feature which allows
guests to do these two things as separate operations. There's
more work to do in this area though.

Patch series structure:
 * patch 1 disentangles the v7M NVIC from some of the internal
   state we're about to rewrite
 * patch 2 fixes a bug that would have meant we could have multiple
   active interrupts at the same group priority, which (a) isn't
   permitted and (b) would break the redesign we're about to do
 * patch 3 is fixing the guest accessors for the APR registers
 * patch 4 is the meat of the change
 * patch 5 is a bonus bugfix


Peter Maydell (5):
  armv7m_nvic: Implement ICSR without using internal GIC state
  hw/intc/arm_gic: Running priority is group priority, not full priority
  hw/intc/arm_gic: Fix handling of GICC_APR<n>, GICC_NSAPR<n> registers
  hw/intc/arm_gic: Drop running_irq and last_active arrays
  hw/intc/arm_gic: Actually set the active bits for active interrupts

 hw/intc/arm_gic.c                | 245 ++++++++++++++++++++++++++++++++++-----
 hw/intc/arm_gic_common.c         |   8 +-
 hw/intc/armv7m_nvic.c            |  13 +--
 include/hw/intc/arm_gic_common.h |  11 +-
 4 files changed, 224 insertions(+), 53 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/5] armv7m_nvic: Implement ICSR without using internal GIC state

[Peter Maydell]

Change the implementation of the Interrupt Control and State Register
in the v7M NVIC to not use the running_irq and last_active internal
state fields in the GIC. These fields don't correspond to state in
a real GIC and will be removed soon.
The changes to the ICSR are:
 * the VECTACTIVE field is documented as identical to the IPSR[8:0]
   field, so implement it that way
 * implement RETTOBASE via looking at the active state bits

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/armv7m_nvic.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index e13b729..3ec8408 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -185,26 +185,25 @@ static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
         return cpu->midr;
     case 0xd04: /* Interrupt Control State.  */
         /* VECTACTIVE */
-        val = s->gic.running_irq[0];
+        cpu = ARM_CPU(current_cpu);
+        val = cpu->env.v7m.exception;
         if (val == 1023) {
             val = 0;
         } else if (val >= 32) {
             val -= 16;
         }
-        /* RETTOBASE */
-        if (s->gic.running_irq[0] == 1023
-                || s->gic.last_active[s->gic.running_irq[0]][0] == 1023) {
-            val |= (1 << 11);
-        }
         /* VECTPENDING */
         if (s->gic.current_pending[0] != 1023)
             val |= (s->gic.current_pending[0] << 12);
-        /* ISRPENDING */
+        /* ISRPENDING and RETTOBASE */
         for (irq = 32; irq < s->num_irq; irq++) {
             if (s->gic.irq_state[irq].pending) {
                 val |= (1 << 22);
                 break;
             }
+            if (irq != cpu->env.v7m.exception && s->gic.irq_state[irq].active) {
+                val |= (1 << 11);
+            }
         }
         /* PENDSTSET */
         if (s->gic.irq_state[ARMV7M_EXCP_SYSTICK].pending)
-- 
1.9.1

[Qemu-devel] [PATCH 2/5] hw/intc/arm_gic: Running priority is group priority, not full priority

[Peter Maydell]

Priority values for the GIC are divided into a "group priority"
and a "subpriority" (with the division being determined by the
binary point register). The running priority is only determined
by the group priority of the active interrupts, not the
subpriority. In particular, this means that there can't be more
than one active interrupt at any particular group priority.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 454bfd7..9814bb9 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -219,13 +219,39 @@ static uint16_t gic_get_current_pending_irq(GICState *s, int cpu,
     return pending_irq;
 }
 
+static int gic_get_group_priority(GICState *s, int cpu, int irq)
+{
+    /* Return the group priority of the specified interrupt
+     * (which is the top bits of its priority, with the number
+     * of bits masked determined by the applicable binary point register).
+     */
+    int bpr;
+    uint32_t mask;
+
+    if (gic_has_groups(s) &&
+        !(s->cpu_ctlr[cpu] & GICC_CTLR_CBPR) &&
+        GIC_TEST_GROUP(irq, (1 << cpu))) {
+        bpr = s->abpr[cpu];
+    } else {
+        bpr = s->bpr[cpu];
+    }
+
+    /* a BPR of 0 means the group priority bits are [7:1];
+     * a BPR of 1 means they are [7:2], and so on down to
+     * a BPR of 7 meaning no group priority bits at all.
+     */
+    mask = ~0U << ((bpr & 7) + 1);
+
+    return GIC_GET_PRIORITY(irq, cpu) & mask;
+}
+
 static void gic_set_running_irq(GICState *s, int cpu, int irq)
 {
     s->running_irq[cpu] = irq;
     if (irq == 1023) {
         s->running_priority[cpu] = 0x100;
     } else {
-        s->running_priority[cpu] = GIC_GET_PRIORITY(irq, cpu);
+        s->running_priority[cpu] = gic_get_group_priority(s, cpu, irq);
     }
     gic_update(s);
 }
-- 
1.9.1

[Qemu-devel] [PATCH 3/5] hw/intc/arm_gic: Fix handling of GICC_APR<n>, GICC_NSAPR<n> registers

[Peter Maydell]

A GICv2 has both GICC_APR<n> and GICC_NSAPR<n> registers, with
the latter holding the active priority bits for Group 1 interrupts
(usually Nonsecure interrupts), and the Nonsecure view of the
GICC_APR<n> is the second half of the GICC_NSAPR<n> registers.
Turn our half-hearted implementation of APR<n> into a proper
implementation of both APR<n> and NSAPR<n>:

 * Add the underlying state for NSAPR<n>
 * Make sure APR<n> aren't visible for pre-GICv2
 * Implement reading of NSAPR<n>
 * Make non-secure reads of APR<n> behave correctly
 * Implement writing to APR<n> and NSAPR<n>

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c                | 114 ++++++++++++++++++++++++++++++++++++++-
 hw/intc/arm_gic_common.c         |   5 +-
 include/hw/intc/arm_gic_common.h |   1 +
 3 files changed, 116 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 9814bb9..ed7faf5 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -954,6 +954,68 @@ static const MemoryRegionOps gic_dist_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
+static inline uint32_t gic_apr_ns_view(GICState *s, int cpu, int regno)
+{
+    /* Return the Nonsecure view of GICC_APR<regno>. This is the
+     * second half of GICC_NSAPR.
+     */
+    switch (GIC_MIN_BPR) {
+    case 0:
+        if (regno < 2) {
+            return s->nsapr[regno + 2][cpu];
+        }
+        break;
+    case 1:
+        if (regno == 0) {
+            return s->nsapr[regno + 1][cpu];
+        }
+        break;
+    case 2:
+        if (regno == 0) {
+            return extract32(s->nsapr[0][cpu], 16, 16);
+        }
+        break;
+    case 3:
+        if (regno == 0) {
+            return extract32(s->nsapr[0][cpu], 8, 8);
+        }
+        break;
+    default:
+        g_assert_not_reached();
+    }
+    return 0;
+}
+
+static inline void gic_apr_write_ns_view(GICState *s, int cpu, int regno,
+                                         uint32_t value)
+{
+    /* Write the Nonsecure view of GICC_APR<regno>. */
+    switch (GIC_MIN_BPR) {
+    case 0:
+        if (regno < 2) {
+            s->nsapr[regno + 2][cpu] = value;
+        }
+        break;
+    case 1:
+        if (regno == 0) {
+            s->nsapr[regno + 1][cpu] = value;
+        }
+        break;
+    case 2:
+        if (regno == 0) {
+            s->nsapr[0][cpu] = deposit32(s->nsapr[0][cpu], 16, 16, value);
+        }
+        break;
+    case 3:
+        if (regno == 0) {
+            s->nsapr[0][cpu] = deposit32(s->nsapr[0][cpu], 8, 8, value);
+        }
+        break;
+    default:
+        g_assert_not_reached();
+    }
+}
+
 static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
                                 uint64_t *data, MemTxAttrs attrs)
 {
@@ -994,8 +1056,31 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
         }
         break;
     case 0xd0: case 0xd4: case 0xd8: case 0xdc:
-        *data = s->apr[(offset - 0xd0) / 4][cpu];
+    {
+        int regno = (offset - 0xd0) / 4;
+
+        if (regno >= GIC_NR_APRS || s->revision != 2) {
+            *data = 0;
+        } else if (s->security_extn && !attrs.secure) {
+            /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
+            *data = gic_apr_ns_view(s, regno, cpu);
+        } else {
+            *data = s->apr[regno][cpu];
+        }
         break;
+    }
+    case 0xe0: case 0xe4: case 0xe8: case 0xec:
+    {
+        int regno = (offset - 0xe0) / 4;
+
+        if (regno >= GIC_NR_APRS || s->revision != 2 || !gic_has_groups(s) ||
+            (s->security_extn && !attrs.secure)) {
+            *data = 0;
+        } else {
+            *data = s->nsapr[regno][cpu];
+        }
+        break;
+    }
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "gic_cpu_read: Bad offset %x\n", (int)offset);
@@ -1033,8 +1118,33 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
         }
         break;
     case 0xd0: case 0xd4: case 0xd8: case 0xdc:
-        qemu_log_mask(LOG_UNIMP, "Writing APR not implemented\n");
+    {
+        int regno = (offset - 0xd0) / 4;
+
+        if (regno >= GIC_NR_APRS || s->revision != 2) {
+            return MEMTX_OK;
+        }
+        if (s->security_extn && !attrs.secure) {
+            /* NS view of GICC_APR<n> is the top half of GIC_NSAPR<n> */
+            gic_apr_write_ns_view(s, regno, cpu, value);
+        } else {
+            s->apr[regno][cpu] = value;
+        }
+        break;
+    }
+    case 0xe0: case 0xe4: case 0xe8: case 0xec:
+    {
+        int regno = (offset - 0xe0) / 4;
+
+        if (regno >= GIC_NR_APRS || s->revision != 2) {
+            return MEMTX_OK;
+        }
+        if (!gic_has_groups(s) || (s->security_extn && !attrs.secure)) {
+            return MEMTX_OK;
+        }
+        s->nsapr[regno][cpu] = value;
         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 a64d071..ea412ae 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 = 10,
-    .minimum_version_id = 10,
+    .version_id = 11,
+    .minimum_version_id = 11,
     .pre_save = gic_pre_save,
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
@@ -80,6 +80,7 @@ static const VMStateDescription vmstate_gic = {
         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_UINT32_2DARRAY(nsapr, 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 899db3d..f7d33a5 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -106,6 +106,7 @@ typedef struct GICState {
      * the GIC.
      */
     uint32_t apr[GIC_NR_APRS][GIC_NCPU];
+    uint32_t nsapr[GIC_NR_APRS][GIC_NCPU];
 
     uint32_t num_cpu;
 
-- 
1.9.1

[Qemu-devel] [PATCH 4/5] hw/intc/arm_gic: Drop running_irq and last_active arrays

[Peter Maydell]

The running_irq and last_active arrays represent state which
doesn't exist in a real hardware GIC. The only thing we use
them for is updating the running priority when an interrupt
is completed, but in fact we can use the active-priority
registers to do this. The running priority is always the
priority corresponding to the lowest set bit in the active
priority registers, because only one interrupt at any
particular priority can be active at once.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c                | 103 ++++++++++++++++++++++++++++-----------
 hw/intc/arm_gic_common.c         |   7 +--
 include/hw/intc/arm_gic_common.h |  10 ----
 3 files changed, 76 insertions(+), 44 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index ed7faf5..427c221 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -245,15 +245,72 @@ static int gic_get_group_priority(GICState *s, int cpu, int irq)
     return GIC_GET_PRIORITY(irq, cpu) & mask;
 }
 
-static void gic_set_running_irq(GICState *s, int cpu, int irq)
+static void gic_activate_irq(GICState *s, int cpu, int irq)
 {
-    s->running_irq[cpu] = irq;
-    if (irq == 1023) {
-        s->running_priority[cpu] = 0x100;
+    /* Set the appropriate Active Priority Register bit for this IRQ,
+     * and update the running priority.
+     */
+    int prio = gic_get_group_priority(s, cpu, irq);
+    int preemption_level = prio >> (GIC_MIN_BPR + 1);
+    int regno = preemption_level / 32;
+    int bitno = preemption_level % 32;
+
+    if (gic_has_groups(s) && GIC_TEST_GROUP(irq, (1 << cpu))) {
+        s->nsapr[regno][cpu] &= (1 << bitno);
     } else {
-        s->running_priority[cpu] = gic_get_group_priority(s, cpu, irq);
+        s->apr[regno][cpu] &= (1 << bitno);
     }
-    gic_update(s);
+
+    s->running_priority[cpu] = prio;
+}
+
+static int gic_get_prio_from_apr_bits(GICState *s, int cpu)
+{
+    /* Recalculate the current running priority for this CPU based
+     * on the set bits in the Active Priority Registers.
+     */
+    int i;
+    for (i = 0; i < GIC_NR_APRS; i++) {
+        uint32_t apr = s->apr[i][cpu] | s->nsapr[i][cpu];
+        if (!apr) {
+            continue;
+        }
+        return (i * 32 + ctz32(apr)) << (GIC_MIN_BPR + 1);
+    }
+    return 0x100;
+}
+
+static void gic_drop_prio(GICState *s, int cpu, int group)
+{
+    /* Drop the priority of the currently active interrupt in the
+     * specified group.
+     *
+     * Note that we can guarantee (because of the requirement to nest
+     * GICC_IAR reads [which activate an interrupt and raise priority]
+     * with GICC_EOIR writes [which drop the priority for the interrupt])
+     * that the interrupt we're being called for is the highest priority
+     * active interrupt, meaning that it has the lowest set bit in the
+     * APR registers.
+     *
+     * If the guest does not honour the ordering constraints then the
+     * behaviour of the GIC is UNPREDICTABLE, which for us means that
+     * the values of the APR registers might become incorrect and the
+     * running priority will be wrong, so interrupts that should preempt
+     * might not do so, and interrupts that should not preempt might do so.
+     */
+    int i;
+
+    for (i = 0; i < GIC_NR_APRS; i++) {
+        uint32_t *papr = group ? &s->nsapr[i][cpu] : &s->apr[i][cpu];
+        if (!*papr) {
+            continue;
+        }
+        /* Clear lowest set bit */
+        *papr &= *papr - 1;
+        break;
+    }
+
+    s->running_priority[cpu] = gic_get_prio_from_apr_bits(s, cpu);
 }
 
 uint32_t gic_acknowledge_irq(GICState *s, int cpu, MemTxAttrs attrs)
@@ -276,7 +333,6 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu, MemTxAttrs attrs)
         DPRINTF("ACK, pending interrupt (%d) has insufficient priority\n", irq);
         return 1023;
     }
-    s->last_active[irq][cpu] = s->running_irq[cpu];
 
     if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
         /* Clear pending flags for both level and edge triggered interrupts.
@@ -307,7 +363,8 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu, MemTxAttrs attrs)
         }
     }
 
-    gic_set_running_irq(s, cpu, irq);
+    gic_activate_irq(s, cpu, irq);
+    gic_update(s);
     DPRINTF("ACK %d\n", irq);
     return ret;
 }
@@ -437,8 +494,9 @@ static uint8_t gic_get_running_priority(GICState *s, int cpu, MemTxAttrs attrs)
 
 void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
 {
-    int update = 0;
     int cm = 1 << cpu;
+    int group;
+
     DPRINTF("EOI %d\n", irq);
     if (irq >= s->num_irq) {
         /* This handles two cases:
@@ -451,8 +509,9 @@ void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
          */
         return;
     }
-    if (s->running_irq[cpu] == 1023)
+    if (s->running_priority[cpu] == 0x100) {
         return; /* No active IRQ.  */
+    }
 
     if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
         /* Mark level triggered interrupts as pending if they are still
@@ -461,11 +520,12 @@ void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
             && GIC_TEST_LEVEL(irq, cm) && (GIC_TARGET(irq) & cm) != 0) {
             DPRINTF("Set %d pending mask %x\n", irq, cm);
             GIC_SET_PENDING(irq, cm);
-            update = 1;
         }
     }
 
-    if (s->security_extn && !attrs.secure && !GIC_TEST_GROUP(irq, cm)) {
+    group = gic_has_groups(s) && GIC_TEST_GROUP(irq, cm);
+
+    if (s->security_extn && !attrs.secure && !group) {
         DPRINTF("Non-secure EOI for Group0 interrupt %d ignored\n", irq);
         return;
     }
@@ -475,23 +535,8 @@ void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
      * i.e. go ahead and complete the irq anyway.
      */
 
-    if (irq != s->running_irq[cpu]) {
-        /* Complete an IRQ that is not currently running.  */
-        int tmp = s->running_irq[cpu];
-        while (s->last_active[tmp][cpu] != 1023) {
-            if (s->last_active[tmp][cpu] == irq) {
-                s->last_active[tmp][cpu] = s->last_active[irq][cpu];
-                break;
-            }
-            tmp = s->last_active[tmp][cpu];
-        }
-        if (update) {
-            gic_update(s);
-        }
-    } else {
-        /* Complete the current running IRQ.  */
-        gic_set_running_irq(s, cpu, s->last_active[s->running_irq[cpu]][cpu]);
-    }
+    gic_drop_prio(s, cpu, group);
+    gic_update(s);
 }
 
 static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index ea412ae..d19efac 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 = 11,
-    .minimum_version_id = 11,
+    .version_id = 12,
+    .minimum_version_id = 12,
     .pre_save = gic_pre_save,
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
@@ -71,10 +71,8 @@ static const VMStateDescription vmstate_gic = {
         VMSTATE_UINT8_ARRAY(irq_target, GICState, GIC_MAXIRQ),
         VMSTATE_UINT8_2DARRAY(priority1, GICState, GIC_INTERNAL, GIC_NCPU),
         VMSTATE_UINT8_ARRAY(priority2, GICState, GIC_MAXIRQ - GIC_INTERNAL),
-        VMSTATE_UINT16_2DARRAY(last_active, GICState, GIC_MAXIRQ, GIC_NCPU),
         VMSTATE_UINT8_2DARRAY(sgi_pending, GICState, GIC_NR_SGIS, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(priority_mask, GICState, GIC_NCPU),
-        VMSTATE_UINT16_ARRAY(running_irq, GICState, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(running_priority, GICState, GIC_NCPU),
         VMSTATE_UINT16_ARRAY(current_pending, GICState, GIC_NCPU),
         VMSTATE_UINT8_ARRAY(bpr, GICState, GIC_NCPU),
@@ -133,7 +131,6 @@ static void arm_gic_common_reset(DeviceState *dev)
             s->priority_mask[i] = 0;
         }
         s->current_pending[i] = 1023;
-        s->running_irq[i] = 1023;
         s->running_priority[i] = 0x100;
         s->cpu_ctlr[i] = 0;
         s->bpr[i] = GIC_MIN_BPR;
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index f7d33a5..bbc4c4d 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -68,7 +68,6 @@ typedef struct GICState {
     uint8_t irq_target[GIC_MAXIRQ];
     uint8_t priority1[GIC_INTERNAL][GIC_NCPU];
     uint8_t priority2[GIC_MAXIRQ - GIC_INTERNAL];
-    uint16_t last_active[GIC_MAXIRQ][GIC_NCPU];
     /* For each SGI on the target CPU, we store 8 bits
      * indicating which source CPUs have made this SGI
      * pending on the target CPU. These correspond to
@@ -78,7 +77,6 @@ typedef struct GICState {
     uint8_t sgi_pending[GIC_NR_SGIS][GIC_NCPU];
 
     uint16_t priority_mask[GIC_NCPU];
-    uint16_t running_irq[GIC_NCPU];
     uint16_t running_priority[GIC_NCPU];
     uint16_t current_pending[GIC_NCPU];
 
@@ -96,14 +94,6 @@ typedef struct GICState {
      * If an interrupt for preemption level X is active, then
      *   APRn[X mod 32] == 0b1,  where n = X / 32
      * otherwise the bit is clear.
-     *
-     * TODO: rewrite the interrupt acknowlege/complete routines to use
-     * the APR registers to track the necessary information to update
-     * s->running_priority[] on interrupt completion (ie completely remove
-     * last_active[][] and running_irq[]). This will be necessary if we ever
-     * want to support TCG<->KVM migration, or TCG guests which can
-     * do power management involving powering down and restarting
-     * the GIC.
      */
     uint32_t apr[GIC_NR_APRS][GIC_NCPU];
     uint32_t nsapr[GIC_NR_APRS][GIC_NCPU];
-- 
1.9.1

[Qemu-devel] [PATCH 5/5] hw/intc/arm_gic: Actually set the active bits for active interrupts

[Peter Maydell]

Although we were correctly handling interrupts becoming active
and then inactive, we weren't actually exposing this to the guest
by setting the 'active' flag for the interrupt, so reads
of GICD_ICACTIVERn and GICD_ISACTIVERn would generally incorrectly
return zeroes. Correct this oversight.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 427c221..dc5a44b 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -262,6 +262,7 @@ static void gic_activate_irq(GICState *s, int cpu, int irq)
     }
 
     s->running_priority[cpu] = prio;
+    GIC_SET_ACTIVE(irq, 1 << cpu);
 }
 
 static int gic_get_prio_from_apr_bits(GICState *s, int cpu)
@@ -536,6 +537,7 @@ void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
      */
 
     gic_drop_prio(s, cpu, group);
+    GIC_CLEAR_ACTIVE(irq, cm);
     gic_update(s);
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/5] arm_gic: Drop running_irq and last_active arrays

[Peter Maydell]

Ping?

thanks
-- PMM

On 28 July 2015 at 14:22, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset is a bit of cleanup to our GIC implementation that
> I've wanted to do for ages.
>
> Our current GIC code uses a couple of arrays (running_irq and
> last_active) to track currently active interrupts so that
> it can correctly determine the running priority as potentially
> nested interrupts are taken and then dismissed. This does
> work, but:
>  * the effectively-a-linked-list is not very hardware-ish,
>    which is usually a bit of a red flag when doing modelling
>  * the GICv2 spec adds the Active Priority Registers which are
>    for use for saving and restoring this sort of state, and
>    implementing these properly effectively constrains you to
>    an implementation that doesn't look like what we have now
>  * it doesn't really fit with the GIC grouping and security
>    extensions, where a guest can say "dismiss the last group 1
>    interrupt" rather than just "dismiss the last interrupt".
>
> This series gets rid of those arrays and instead uses the
> Active Priority Registers to do the job. The APRs have one
> bit per "preemption level" (ie per distinct group priority).
> When we take an interrupt we set the appropriate bit to 1
> (and this will always be the lowest set bit in the register,
> because low-numbered priorities are higher and we wouldn't
> have taken the interrupt unless it was higher than our current
> priority). Similarly, when we dismiss an interrupt this just
> clears the lowest set bit in the register, which must be the
> current active interrupt. (It's important not to try to look
> at the current configured priority of the interrupt number,
> because the guest might have reconfigured it while it was
> active.) The new running priority is then calculable by
> looking at the new lowest set bit.
>
> The new code also takes a step in the direction of
> separating the idea of "priority drop" from "deactivate interrupt",
> which we will need to implement the GICv2 feature which allows
> guests to do these two things as separate operations. There's
> more work to do in this area though.
>
> Patch series structure:
>  * patch 1 disentangles the v7M NVIC from some of the internal
>    state we're about to rewrite
>  * patch 2 fixes a bug that would have meant we could have multiple
>    active interrupts at the same group priority, which (a) isn't
>    permitted and (b) would break the redesign we're about to do
>  * patch 3 is fixing the guest accessors for the APR registers
>  * patch 4 is the meat of the change
>  * patch 5 is a bonus bugfix
>
>
> Peter Maydell (5):
>   armv7m_nvic: Implement ICSR without using internal GIC state
>   hw/intc/arm_gic: Running priority is group priority, not full priority
>   hw/intc/arm_gic: Fix handling of GICC_APR<n>, GICC_NSAPR<n> registers
>   hw/intc/arm_gic: Drop running_irq and last_active arrays
>   hw/intc/arm_gic: Actually set the active bits for active interrupts
>
>  hw/intc/arm_gic.c                | 245 ++++++++++++++++++++++++++++++++++-----
>  hw/intc/arm_gic_common.c         |   8 +-
>  hw/intc/armv7m_nvic.c            |  13 +--
>  include/hw/intc/arm_gic_common.h |  11 +-
>  4 files changed, 224 insertions(+), 53 deletions(-)

Re: [Qemu-devel] [PATCH 0/5] arm_gic: Drop running_irq and last_active arrays

[Peter Maydell]

Looks like nobody cared to review this one, so applying
to target-arm.next.

thanks
-- PMM

On 14 August 2015 at 11:11, Peter Maydell <peter.maydell@linaro.org> wrote:
> Ping?
>
> thanks
> -- PMM
>
> On 28 July 2015 at 14:22, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This patchset is a bit of cleanup to our GIC implementation that
>> I've wanted to do for ages.
>>
>> Our current GIC code uses a couple of arrays (running_irq and
>> last_active) to track currently active interrupts so that
>> it can correctly determine the running priority as potentially
>> nested interrupts are taken and then dismissed. This does
>> work, but:
>>  * the effectively-a-linked-list is not very hardware-ish,
>>    which is usually a bit of a red flag when doing modelling
>>  * the GICv2 spec adds the Active Priority Registers which are
>>    for use for saving and restoring this sort of state, and
>>    implementing these properly effectively constrains you to
>>    an implementation that doesn't look like what we have now
>>  * it doesn't really fit with the GIC grouping and security
>>    extensions, where a guest can say "dismiss the last group 1
>>    interrupt" rather than just "dismiss the last interrupt".
>>
>> This series gets rid of those arrays and instead uses the
>> Active Priority Registers to do the job. The APRs have one
>> bit per "preemption level" (ie per distinct group priority).
>> When we take an interrupt we set the appropriate bit to 1
>> (and this will always be the lowest set bit in the register,
>> because low-numbered priorities are higher and we wouldn't
>> have taken the interrupt unless it was higher than our current
>> priority). Similarly, when we dismiss an interrupt this just
>> clears the lowest set bit in the register, which must be the
>> current active interrupt. (It's important not to try to look
>> at the current configured priority of the interrupt number,
>> because the guest might have reconfigured it while it was
>> active.) The new running priority is then calculable by
>> looking at the new lowest set bit.
>>
>> The new code also takes a step in the direction of
>> separating the idea of "priority drop" from "deactivate interrupt",
>> which we will need to implement the GICv2 feature which allows
>> guests to do these two things as separate operations. There's
>> more work to do in this area though.
>>
>> Patch series structure:
>>  * patch 1 disentangles the v7M NVIC from some of the internal
>>    state we're about to rewrite
>>  * patch 2 fixes a bug that would have meant we could have multiple
>>    active interrupts at the same group priority, which (a) isn't
>>    permitted and (b) would break the redesign we're about to do
>>  * patch 3 is fixing the guest accessors for the APR registers
>>  * patch 4 is the meat of the change
>>  * patch 5 is a bonus bugfix
>>
>>
>> Peter Maydell (5):
>>   armv7m_nvic: Implement ICSR without using internal GIC state
>>   hw/intc/arm_gic: Running priority is group priority, not full priority
>>   hw/intc/arm_gic: Fix handling of GICC_APR<n>, GICC_NSAPR<n> registers
>>   hw/intc/arm_gic: Drop running_irq and last_active arrays
>>   hw/intc/arm_gic: Actually set the active bits for active interrupts
>>
>>  hw/intc/arm_gic.c                | 245 ++++++++++++++++++++++++++++++++++-----
>>  hw/intc/arm_gic_common.c         |   8 +-
>>  hw/intc/armv7m_nvic.c            |  13 +--
>>  include/hw/intc/arm_gic_common.h |  11 +-
>>  4 files changed, 224 insertions(+), 53 deletions(-)