[Qemu-devel] [PATCH v4 00/17] arm_gic: Add security and grouping support

[Qemu-devel] [PATCH v4 00/17] arm_gic: Add security and grouping support

[Peter Maydell]

This patch series adds support for GICv1 and GICv2 security
extensions, as well as support for GIC interrupt grouping on GICv2.

This is based on the work originally by Fabian and then by Greg.
I've gone through and dealt with all the issues I raised in code
review, and a few others I noticed as I was working on it.
The general structure of the changes is still the same, though
I've reordered one or two of the patches; I've touched most of
the lines of code in the series, though, as well as deleting
quite a few (the patches now add ~375 lines of code rather than
over 475).

I think these patches are in a suitable state to apply (and
they have no dependencies that aren't in master), assuming no
further issues found in review.

With this patchset the security extensions are still disabled
on all boards, so the actual functional change is that GICv2
now correctly implements interrupt grouping. This is enabled
always for GICv2, because the programming model is fully backwards
compatible with treating the GIC like one which doesn't support
groups (which is what Linux does).

The next part of this work is going to be actually enabling
the security extensions. Here's a sketch of my plan for that:
 * the a15mpcore and a9mpcore wrapper objects will default to
   enabling the security extensions in the GIC they create
   (unless the GIC is the KVM one). They also provide a
   QOM property to override this
 * for the set of legacy boards which are currently disabling
   has_el3 on their CPUs, we also have them disable TZ in the GIC
   (a non-TZ CPU and a TZ GIC is a bad combo because the CPU
   has no way to put the interrupts into Group1 where it can
   use them, so the whole system is busted)
 * the virt board creates its GIC directly, so it should also
   set the has-security-extensions property as needed
 * if boot.c is starting the CPUs directly in NonSecure
   mode (because we're booting a kernel directly rather than
   starting firmware, and arm_boot_info::secure_boot is false)
   then it must also manually configure the GIC distributor
   to put all interrupts into Group1. This is boot.c having
   to do a firmware configuration job since it's effectively
   acting as lightweight builtin firmware.

I think we could reasonably review and commit this patchseries
without waiting for that bit of board-wiring work; let me know
if you disagree.


Major changes since v3:
 * renamed property to 'has-security-extensions', to be a bit
   more in line with the CPU's 'has_el3'. I'm not wedded to this
   name so if anybody wants to suggest something better (or
   tell me our convention for prop names is underscores!) feel free
 * error on realize if security extensions turned on for a GIC
   which doesn't support them
 * new patch: switch to read/write_with_attrs MMIO callbacks so
   we can get at the Secure/NonSecure tx attribute
 * make the GIC_*_GROUP macros work like the others, with a simple
   SET/CLEAR/TEST semantic
 * new patch: save and restore GICD_IGROUPRn state when using KVM
   now we have the state struct fields to keep it in [the kernel
   doesn't implement grouping, but if it ever does we will be ready]
 * rather than having a 2-element array for storing the S and NS
   banked versions of GICD_CTLR and GICC_CTLR, just store the S
   version, since in both cases the NS view is just an alias of
   a subset of bits from the S register. This allows us to nicely
   simplify a lot of the logic that deals with these registers.
 * fixed bug in handling of GICC_BPR for GICv2-without-TZ
 * added missing masks in gic_set_priority_mask() and gic_set_priority()
 * make AckCtl operate on GICv2-without-TZ
 * handle an UNPREDICTABLE case (Secure EOI of a Group1 irq
   with AckCtl == 0) in a way more convenient for the implementation
 * reuse gic_get_current_pending_irq() in implementation of IAR writes,
   rather than reimplementing equivalent logic
 * new patch: support grouping in a single gic_update function (rather
   than having split update functions for the two cases)
 * new patch: wire FIQ up on highbank/midway; this means we're now
   consistent in having FIQ wired up on all our boards with GICv2
 * lots of minor formatting tweaks, etc; see individual commit messages


Fabian Aggeler (12):
  hw/intc/arm_gic: Create outbound FIQ lines
  hw/intc/arm_gic: Add Security Extensions property
  hw/intc/arm_gic: Add Interrupt Group Registers
  hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked
  hw/intc/arm_gic: Make ICCBPR/GICC_BPR banked
  hw/intc/arm_gic: Make ICCICR/GICC_CTLR banked
  hw/intc/arm_gic: Implement Non-secure view of RPR
  hw/intc/arm_gic: Restrict priority view
  hw/intc/arm_gic: Handle grouping for GICC_HPPIR
  hw/intc/arm_gic: Change behavior of EOIR writes
  hw/intc/arm_gic: Change behavior of IAR writes
  hw/arm/vexpress.c: Wire FIQ between CPU <> GIC

Greg Bellows (1):
  hw/arm/virt.c: Wire FIQ between CPU <> GIC

Peter Maydell (4):
  hw/intc/arm_gic: Switch to read/write callbacks with tx attributes
  hw/intc/arm_gic_kvm.c: Save and restore GICD_IGROUPRn state
  hw/intc/arm_gic: Add grouping support to gic_update()
  hw/arm/highbank.c: Wire FIQ between CPU <> GIC

 hw/arm/highbank.c                |   3 +
 hw/arm/vexpress.c                |   2 +
 hw/arm/virt.c                    |   2 +
 hw/intc/arm_gic.c                | 469 ++++++++++++++++++++++++++++++++-------
 hw/intc/arm_gic_common.c         |  22 +-
 hw/intc/arm_gic_kvm.c            |  51 +++--
 hw/intc/armv7m_nvic.c            |   8 +-
 hw/intc/gic_internal.h           |  29 ++-
 include/hw/intc/arm_gic_common.h |  24 +-
 9 files changed, 492 insertions(+), 118 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v4 01/17] hw/intc/arm_gic: Create outbound FIQ lines

[Peter Maydell]

From: Fabian Aggeler <aggelerf@ethz.ch>

Create the outbound FIQ lines from the GIC to the CPUs; these are
used if the GIC has security extensions or grouping support.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-2-git-send-email-greg.bellows@linaro.org
[PMM: added FIQ lines to kvm-arm-gic so its interface is the same;
tweaked commit message]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c                | 3 +++
 hw/intc/arm_gic_kvm.c            | 5 ++++-
 include/hw/intc/arm_gic_common.h | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index a04c822..e9fb8b9 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -790,6 +790,9 @@ void gic_init_irqs_and_distributor(GICState *s)
     for (i = 0; i < NUM_CPU(s); i++) {
         sysbus_init_irq(sbd, &s->parent_irq[i]);
     }
+    for (i = 0; i < NUM_CPU(s); i++) {
+        sysbus_init_irq(sbd, &s->parent_fiq[i]);
+    }
     memory_region_init_io(&s->iomem, OBJECT(s), &gic_dist_ops, s,
                           "gic_dist", 0x1000);
 }
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index e1952ad..5aedae1 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -554,12 +554,15 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
      */
     i += (GIC_INTERNAL * s->num_cpu);
     qdev_init_gpio_in(dev, kvm_arm_gic_set_irq, i);
-    /* We never use our outbound IRQ lines but provide them so that
+    /* We never use our outbound IRQ/FIQ lines but provide them so that
      * we maintain the same interface as the non-KVM GIC.
      */
     for (i = 0; i < s->num_cpu; i++) {
         sysbus_init_irq(sbd, &s->parent_irq[i]);
     }
+    for (i = 0; i < s->num_cpu; i++) {
+        sysbus_init_irq(sbd, &s->parent_fiq[i]);
+    }
 
     /* Try to create the device via the device control API */
     s->dev_fd = -1;
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index f6887ed..01c6f24 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -50,6 +50,7 @@ typedef struct GICState {
     /*< public >*/
 
     qemu_irq parent_irq[GIC_NCPU];
+    qemu_irq parent_fiq[GIC_NCPU];
     bool enabled;
     bool cpu_enabled[GIC_NCPU];
 
-- 
1.9.1

[Qemu-devel] [PATCH v4 02/17] hw/intc/arm_gic: Add Security Extensions property

[Peter Maydell]

From: Fabian Aggeler <aggelerf@ethz.ch>

Add a QOM property which allows the GIC Security Extensions to be
enabled. These are an optional part of the GICv1 and GICv2 architecture.
This commit just adds the property and some sanity checks that it
is only enabled on GIC revisions that support it.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-5-git-send-email-greg.bellows@linaro.org
[PMM: changed property name, added checks that it isn't set for
 older GIC revisions or if using the KVM VGIC; reworded commit message]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c                | 5 ++++-
 hw/intc/arm_gic_common.c         | 9 +++++++++
 hw/intc/arm_gic_kvm.c            | 6 ++++++
 include/hw/intc/arm_gic_common.h | 1 +
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index e9fb8b9..cdf7408 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -298,7 +298,10 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
         if (offset == 0)
             return s->enabled;
         if (offset == 4)
-            return ((s->num_irq / 32) - 1) | ((NUM_CPU(s) - 1) << 5);
+            /* Interrupt Controller Type Register */
+            return ((s->num_irq / 32) - 1)
+                    | ((NUM_CPU(s) - 1) << 5)
+                    | (s->security_extn << 10);
         if (offset < 0x08)
             return 0;
         if (offset >= 0x80) {
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 18b01ba..5ed21f1 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -110,6 +110,13 @@ static void arm_gic_common_realize(DeviceState *dev, Error **errp)
                    num_irq);
         return;
     }
+
+    if (s->security_extn &&
+        (s->revision == REV_11MPCORE || s->revision == REV_NVIC)) {
+        error_setg(errp, "this GIC revision does not implement "
+                   "the security extensions");
+        return;
+    }
 }
 
 static void arm_gic_common_reset(DeviceState *dev)
@@ -149,6 +156,8 @@ static Property arm_gic_common_properties[] = {
      * (Internally, 0xffffffff also indicates "not a GIC but an NVIC".)
      */
     DEFINE_PROP_UINT32("revision", GICState, revision, 1),
+    /* True if the GIC should implement the security extensions */
+    DEFINE_PROP_BOOL("has-security-extensions", GICState, security_extn, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 5aedae1..cb47b12 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -544,6 +544,12 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (s->security_extn) {
+        error_setg(errp, "the in-kernel VGIC does not implement the "
+                   "security extensions");
+        return;
+    }
+
     i = s->num_irq - GIC_INTERNAL;
     /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU.
      * GPIO array layout is thus:
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 01c6f24..7825134 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -105,6 +105,7 @@ typedef struct GICState {
     MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
     uint32_t num_irq;
     uint32_t revision;
+    bool security_extn;
     int dev_fd; /* kvm device fd if backed by kvm vgic support */
 } GICState;
 
-- 
1.9.1

[Qemu-devel] [PATCH v4 03/17] hw/intc/arm_gic: Switch to read/write callbacks with tx attributes

[Peter Maydell]

Switch the GIC's MMIO callback functions to the read_with_attrs
and write_with_attrs functions which provide MemTxAttrs. This will
allow the GIC to correctly handle secure and nonsecure register
accesses.

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

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index cdf7408..29015c2 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -282,7 +282,7 @@ void gic_complete_irq(GICState *s, int cpu, int irq)
     }
 }
 
-static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
+static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
 {
     GICState *s = (GICState *)opaque;
     uint32_t res;
@@ -418,24 +418,30 @@ bad_reg:
     return 0;
 }
 
-static uint32_t gic_dist_readw(void *opaque, hwaddr offset)
+static MemTxResult gic_dist_read(void *opaque, hwaddr offset, uint64_t *data,
+                                 unsigned size, MemTxAttrs attrs)
 {
-    uint32_t val;
-    val = gic_dist_readb(opaque, offset);
-    val |= gic_dist_readb(opaque, offset + 1) << 8;
-    return val;
-}
-
-static uint32_t gic_dist_readl(void *opaque, hwaddr offset)
-{
-    uint32_t val;
-    val = gic_dist_readw(opaque, offset);
-    val |= gic_dist_readw(opaque, offset + 2) << 16;
-    return val;
+    switch (size) {
+    case 1:
+        *data = gic_dist_readb(opaque, offset, attrs);
+        return MEMTX_OK;
+    case 2:
+        *data = gic_dist_readb(opaque, offset, attrs);
+        *data |= gic_dist_readb(opaque, offset + 1, attrs) << 8;
+        return MEMTX_OK;
+    case 4:
+        *data = gic_dist_readb(opaque, offset, attrs);
+        *data |= gic_dist_readb(opaque, offset + 1, attrs) << 8;
+        *data |= gic_dist_readb(opaque, offset + 2, attrs) << 16;
+        *data |= gic_dist_readb(opaque, offset + 3, attrs) << 24;
+        return MEMTX_OK;
+    default:
+        return MEMTX_ERROR;
+    }
 }
 
 static void gic_dist_writeb(void *opaque, hwaddr offset,
-                            uint32_t value)
+                            uint32_t value, MemTxAttrs attrs)
 {
     GICState *s = (GICState *)opaque;
     int irq;
@@ -612,14 +618,14 @@ bad_reg:
 }
 
 static void gic_dist_writew(void *opaque, hwaddr offset,
-                            uint32_t value)
+                            uint32_t value, MemTxAttrs attrs)
 {
-    gic_dist_writeb(opaque, offset, value & 0xff);
-    gic_dist_writeb(opaque, offset + 1, value >> 8);
+    gic_dist_writeb(opaque, offset, value & 0xff, attrs);
+    gic_dist_writeb(opaque, offset + 1, value >> 8, attrs);
 }
 
 static void gic_dist_writel(void *opaque, hwaddr offset,
-                            uint32_t value)
+                            uint32_t value, MemTxAttrs attrs)
 {
     GICState *s = (GICState *)opaque;
     if (offset == 0xf00) {
@@ -655,45 +661,72 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
         gic_update(s);
         return;
     }
-    gic_dist_writew(opaque, offset, value & 0xffff);
-    gic_dist_writew(opaque, offset + 2, value >> 16);
+    gic_dist_writew(opaque, offset, value & 0xffff, attrs);
+    gic_dist_writew(opaque, offset + 2, value >> 16, attrs);
+}
+
+static MemTxResult gic_dist_write(void *opaque, hwaddr offset, uint64_t data,
+                                  unsigned size, MemTxAttrs attrs)
+{
+    switch (size) {
+    case 1:
+        gic_dist_writeb(opaque, offset, data, attrs);
+        return MEMTX_OK;
+    case 2:
+        gic_dist_writew(opaque, offset, data, attrs);
+        return MEMTX_OK;
+    case 4:
+        gic_dist_writel(opaque, offset, data, attrs);
+        return MEMTX_OK;
+    default:
+        return MEMTX_ERROR;
+    }
 }
 
 static const MemoryRegionOps gic_dist_ops = {
-    .old_mmio = {
-        .read = { gic_dist_readb, gic_dist_readw, gic_dist_readl, },
-        .write = { gic_dist_writeb, gic_dist_writew, gic_dist_writel, },
-    },
+    .read_with_attrs = gic_dist_read,
+    .write_with_attrs = gic_dist_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
+static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
+                                uint64_t *data, MemTxAttrs attrs)
 {
     switch (offset) {
     case 0x00: /* Control */
-        return s->cpu_enabled[cpu];
+        *data = s->cpu_enabled[cpu];
+        break;
     case 0x04: /* Priority mask */
-        return s->priority_mask[cpu];
+        *data = s->priority_mask[cpu];
+        break;
     case 0x08: /* Binary Point */
-        return s->bpr[cpu];
+        *data = s->bpr[cpu];
+        break;
     case 0x0c: /* Acknowledge */
-        return gic_acknowledge_irq(s, cpu);
+        *data = gic_acknowledge_irq(s, cpu);
+        break;
     case 0x14: /* Running Priority */
-        return s->running_priority[cpu];
+        *data = s->running_priority[cpu];
+        break;
     case 0x18: /* Highest Pending Interrupt */
-        return s->current_pending[cpu];
+        *data = s->current_pending[cpu];
+        break;
     case 0x1c: /* Aliased Binary Point */
-        return s->abpr[cpu];
+        *data = s->abpr[cpu];
+        break;
     case 0xd0: case 0xd4: case 0xd8: case 0xdc:
-        return s->apr[(offset - 0xd0) / 4][cpu];
+        *data = s->apr[(offset - 0xd0) / 4][cpu];
+        break;
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "gic_cpu_read: Bad offset %x\n", (int)offset);
-        return 0;
+        return MEMTX_ERROR;
     }
+    return MEMTX_OK;
 }
 
-static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
+static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
+                                 uint32_t value, MemTxAttrs attrs)
 {
     switch (offset) {
     case 0x00: /* Control */
@@ -708,7 +741,7 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
         break;
     case 0x10: /* End Of Interrupt */
         gic_complete_irq(s, cpu, value & 0x3ff);
-        return;
+        return MEMTX_OK;
     case 0x1c: /* Aliased Binary Point */
         if (s->revision >= 2) {
             s->abpr[cpu] = (value & 0x7);
@@ -720,56 +753,59 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
     default:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "gic_cpu_write: Bad offset %x\n", (int)offset);
-        return;
+        return MEMTX_ERROR;
     }
     gic_update(s);
+    return MEMTX_OK;
 }
 
 /* Wrappers to read/write the GIC CPU interface for the current CPU */
-static uint64_t gic_thiscpu_read(void *opaque, hwaddr addr,
-                                 unsigned size)
+static MemTxResult gic_thiscpu_read(void *opaque, hwaddr addr, uint64_t *data,
+                                    unsigned size, MemTxAttrs attrs)
 {
     GICState *s = (GICState *)opaque;
-    return gic_cpu_read(s, gic_get_current_cpu(s), addr);
+    return gic_cpu_read(s, gic_get_current_cpu(s), addr, data, attrs);
 }
 
-static void gic_thiscpu_write(void *opaque, hwaddr addr,
-                              uint64_t value, unsigned size)
+static MemTxResult gic_thiscpu_write(void *opaque, hwaddr addr,
+                                     uint64_t value, unsigned size,
+                                     MemTxAttrs attrs)
 {
     GICState *s = (GICState *)opaque;
-    gic_cpu_write(s, gic_get_current_cpu(s), addr, value);
+    return gic_cpu_write(s, gic_get_current_cpu(s), addr, value, attrs);
 }
 
 /* Wrappers to read/write the GIC CPU interface for a specific CPU.
  * These just decode the opaque pointer into GICState* + cpu id.
  */
-static uint64_t gic_do_cpu_read(void *opaque, hwaddr addr,
-                                unsigned size)
+static MemTxResult gic_do_cpu_read(void *opaque, hwaddr addr, uint64_t *data,
+                                   unsigned size, MemTxAttrs attrs)
 {
     GICState **backref = (GICState **)opaque;
     GICState *s = *backref;
     int id = (backref - s->backref);
-    return gic_cpu_read(s, id, addr);
+    return gic_cpu_read(s, id, addr, data, attrs);
 }
 
-static void gic_do_cpu_write(void *opaque, hwaddr addr,
-                             uint64_t value, unsigned size)
+static MemTxResult gic_do_cpu_write(void *opaque, hwaddr addr,
+                                    uint64_t value, unsigned size,
+                                    MemTxAttrs attrs)
 {
     GICState **backref = (GICState **)opaque;
     GICState *s = *backref;
     int id = (backref - s->backref);
-    gic_cpu_write(s, id, addr, value);
+    return gic_cpu_write(s, id, addr, value, attrs);
 }
 
 static const MemoryRegionOps gic_thiscpu_ops = {
-    .read = gic_thiscpu_read,
-    .write = gic_thiscpu_write,
+    .read_with_attrs = gic_thiscpu_read,
+    .write_with_attrs = gic_thiscpu_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
 static const MemoryRegionOps gic_cpu_ops = {
-    .read = gic_do_cpu_read,
-    .write = gic_do_cpu_write,
+    .read_with_attrs = gic_do_cpu_read,
+    .write_with_attrs = gic_do_cpu_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-- 
1.9.1

[Qemu-devel] [PATCH v4 04/17] hw/intc/arm_gic: Add Interrupt Group Registers

[Peter Maydell]

From: Fabian Aggeler <aggelerf@ethz.ch>

The Interrupt Group Registers allow the guest to configure interrupts
into one of two groups, where Group0 are higher priority and may
be routed to IRQ or FIQ, and Group1 are lower priority and always
routed to IRQ. (In a GIC with the security extensions Group0 is
Secure interrupts and Group 1 is NonSecure.)
The GICv2 always supports interrupt grouping; the GICv1 does only
if it implements the security extensions.

This patch implements the ability to read and write the registers;
the actual functionality the bits control will be added in a
subsequent patch.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-7-git-send-email-greg.bellows@linaro.org
[PMM: bring GIC_*_GROUP macros into line with the others, ie a
 simple SET/CLEAR/TEST rather than GROUP0/GROUP1;
 utility gic_has_groups() function;
 minor style fixes;
 bump vmstate version]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c                | 50 +++++++++++++++++++++++++++++++++++++---
 hw/intc/arm_gic_common.c         |  5 ++--
 hw/intc/gic_internal.h           |  4 ++++
 include/hw/intc/arm_gic_common.h |  1 +
 4 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 29015c2..1aa4520 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -45,6 +45,14 @@ static inline int gic_get_current_cpu(GICState *s)
     return 0;
 }
 
+/* Return true if this GIC config has interrupt groups, which is
+ * true if we're a GICv2, or a GICv1 with the security extensions.
+ */
+static inline bool gic_has_groups(GICState *s)
+{
+    return s->revision == 2 || s->security_extn;
+}
+
 /* TODO: Many places that call this routine could be optimized.  */
 /* Update interrupt status after enabled or pending bits have been changed.  */
 void gic_update(GICState *s)
@@ -305,8 +313,24 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
         if (offset < 0x08)
             return 0;
         if (offset >= 0x80) {
-            /* Interrupt Security , RAZ/WI */
-            return 0;
+            /* Interrupt Group Registers: these RAZ/WI if this is an NS
+             * access to a GIC with the security extensions, or if the GIC
+             * doesn't have groups at all.
+             */
+            res = 0;
+            if (!(s->security_extn && !attrs.secure) && gic_has_groups(s)) {
+                /* Every byte offset holds 8 group status bits */
+                irq = (offset - 0x080) * 8 + GIC_BASE_IRQ;
+                if (irq >= s->num_irq) {
+                    goto bad_reg;
+                }
+                for (i = 0; i < 8; i++) {
+                    if (GIC_TEST_GROUP(irq + i, cm)) {
+                        res |= (1 << i);
+                    }
+                }
+            }
+            return res;
         }
         goto bad_reg;
     } else if (offset < 0x200) {
@@ -456,7 +480,27 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         } else if (offset < 4) {
             /* ignored.  */
         } else if (offset >= 0x80) {
-            /* Interrupt Security Registers, RAZ/WI */
+            /* Interrupt Group Registers: RAZ/WI for NS access to secure
+             * GIC, or for GICs without groups.
+             */
+            if (!(s->security_extn && !attrs.secure) && gic_has_groups(s)) {
+                /* Every byte offset holds 8 group status bits */
+                irq = (offset - 0x80) * 8 + GIC_BASE_IRQ;
+                if (irq >= s->num_irq) {
+                    goto bad_reg;
+                }
+                for (i = 0; i < 8; i++) {
+                    /* Group bits are banked for private interrupts */
+                    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+                    if (value & (1 << i)) {
+                        /* Group1 (Non-secure) */
+                        GIC_SET_GROUP(irq + i, cm);
+                    } else {
+                        /* Group0 (Secure) */
+                        GIC_CLEAR_GROUP(irq + i, cm);
+                    }
+                }
+            }
         } else {
             goto bad_reg;
         }
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 5ed21f1..b5a85e5 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -52,14 +52,15 @@ static const VMStateDescription vmstate_gic_irq_state = {
         VMSTATE_UINT8(level, gic_irq_state),
         VMSTATE_BOOL(model, gic_irq_state),
         VMSTATE_BOOL(edge_trigger, gic_irq_state),
+        VMSTATE_UINT8(group, gic_irq_state),
         VMSTATE_END_OF_LIST()
     }
 };
 
 static const VMStateDescription vmstate_gic = {
     .name = "arm_gic",
-    .version_id = 7,
-    .minimum_version_id = 7,
+    .version_id = 8,
+    .minimum_version_id = 8,
     .pre_save = gic_pre_save,
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index e87ef36..e8cf773 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -50,6 +50,10 @@
                                     s->priority1[irq][cpu] :            \
                                     s->priority2[(irq) - GIC_INTERNAL])
 #define GIC_TARGET(irq) s->irq_target[irq]
+#define GIC_CLEAR_GROUP(irq, cm) (s->irq_state[irq].group &= ~(cm))
+#define GIC_SET_GROUP(irq, cm) (s->irq_state[irq].group |= (cm))
+#define GIC_TEST_GROUP(irq, cm) ((s->irq_state[irq].group & (cm)) != 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 7825134..b78981e 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -42,6 +42,7 @@ typedef struct gic_irq_state {
     uint8_t level;
     bool model; /* 0 = N:N, 1 = 1:N */
     bool edge_trigger; /* true: edge-triggered, false: level-triggered  */
+    uint8_t group;
 } gic_irq_state;
 
 typedef struct GICState {
-- 
1.9.1

[Qemu-devel] [PATCH v4 05/17] hw/intc/arm_gic_kvm.c: Save and restore GICD_IGROUPRn state

[Peter Maydell]

Now that the GIC base class has state fields for the GICD_IGROUPRn
registers, make kvm_arm_gic_get() and kvm_arm_gic_put() write and
read them. This allows us to remove the check that made us
fail migration if the guest had set any of the group register bits.

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

diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index cb47b12..3591ca7 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -176,6 +176,20 @@ static void translate_clear(GICState *s, int irq, int cpu,
     }
 }
 
+static void translate_group(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_GROUP(irq, cm);
+    } else {
+        if (*field & 1) {
+            GIC_SET_GROUP(irq, cm);
+        }
+    }
+}
+
 static void translate_enabled(GICState *s, int irq, int cpu,
                               uint32_t *field, bool to_kernel)
 {
@@ -365,6 +379,9 @@ static void kvm_arm_gic_put(GICState *s)
     kvm_dist_put(s, 0x180, 1, s->num_irq, translate_clear);
     kvm_dist_put(s, 0x100, 1, s->num_irq, translate_enabled);
 
+    /* irq_state[n].group -> GICD_IGROUPRn */
+    kvm_dist_put(s, 0x80, 1, s->num_irq, translate_group);
+
     /* 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) */
@@ -454,21 +471,14 @@ static void kvm_arm_gic_get(GICState *s)
     /* 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_IGROUPRn -> irq_state[n].group */
+    kvm_dist_get(s, 0x80, 1, s->num_irq, translate_group);
+
     /* GICD_ISENABLERn -> irq_state[n].enabled */
     kvm_dist_get(s, 0x100, 1, s->num_irq, translate_enabled);
 
-- 
1.9.1

[Qemu-devel] [PATCH v4 06/17] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked

[Peter Maydell]

From: Fabian Aggeler <aggelerf@ethz.ch>

ICDDCR/GICD_CTLR is banked if the GIC has the security extensions,
and the S (or only) copy has separate enable bits for Group0 and
Group1 enable if the GIC implements interrupt groups.

EnableGroup0 (Bit [1]) in GICv1 is architecturally IMPDEF. Since this
bit (Enable Non-secure) is present in the integrated GIC of the Cortex-A9
MPCore, we support this bit in our GICv1 implementation too.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-8-git-send-email-greg.bellows@linaro.org
[PMM: rewritten to store the state in a single s->ctlr uint32,
 with the NS register handled as an alias of bit 1 in that value;
 added vmstate version bump]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c                | 28 +++++++++++++++++++++++-----
 hw/intc/arm_gic_common.c         |  8 ++++----
 hw/intc/armv7m_nvic.c            |  2 +-
 hw/intc/gic_internal.h           |  2 ++
 include/hw/intc/arm_gic_common.h |  5 ++++-
 5 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 1aa4520..4f13ff2 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -67,7 +67,8 @@ void gic_update(GICState *s)
     for (cpu = 0; cpu < NUM_CPU(s); cpu++) {
         cm = 1 << cpu;
         s->current_pending[cpu] = 1023;
-        if (!s->enabled || !s->cpu_enabled[cpu]) {
+        if (!(s->ctlr & (GICD_CTLR_EN_GRP0 | GICD_CTLR_EN_GRP1))
+            || !s->cpu_enabled[cpu]) {
             qemu_irq_lower(s->parent_irq[cpu]);
             return;
         }
@@ -303,8 +304,16 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
     cpu = gic_get_current_cpu(s);
     cm = 1 << cpu;
     if (offset < 0x100) {
-        if (offset == 0)
-            return s->enabled;
+        if (offset == 0) {      /* GICD_CTLR */
+            if (s->security_extn && !attrs.secure) {
+                /* The NS bank of this register is just an alias of the
+                 * EnableGrp1 bit in the S bank version.
+                 */
+                return extract32(s->ctlr, 1, 1);
+            } else {
+                return s->ctlr;
+            }
+        }
         if (offset == 4)
             /* Interrupt Controller Type Register */
             return ((s->num_irq / 32) - 1)
@@ -475,8 +484,17 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
     cpu = gic_get_current_cpu(s);
     if (offset < 0x100) {
         if (offset == 0) {
-            s->enabled = (value & 1);
-            DPRINTF("Distribution %sabled\n", s->enabled ? "En" : "Dis");
+            if (s->security_extn && !attrs.secure) {
+                /* NS version is just an alias of the S version's bit 1 */
+                s->ctlr = deposit32(s->ctlr, 1, 1, value);
+            } else if (gic_has_groups(s)) {
+                s->ctlr = value & (GICD_CTLR_EN_GRP0 | GICD_CTLR_EN_GRP1);
+            } else {
+                s->ctlr = value & GICD_CTLR_EN_GRP0;
+            }
+            DPRINTF("Distributor: Group0 %sabled; Group 1 %sabled\n",
+                    s->ctlr & GICD_CTLR_EN_GRP0 ? "En" : "Dis",
+                    s->ctlr & GICD_CTLR_EN_GRP1 ? "En" : "Dis");
         } else if (offset < 4) {
             /* ignored.  */
         } else if (offset >= 0x80) {
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index b5a85e5..bef76fc 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -59,12 +59,12 @@ static const VMStateDescription vmstate_gic_irq_state = {
 
 static const VMStateDescription vmstate_gic = {
     .name = "arm_gic",
-    .version_id = 8,
-    .minimum_version_id = 8,
+    .version_id = 9,
+    .minimum_version_id = 9,
     .pre_save = gic_pre_save,
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_BOOL(enabled, GICState),
+        VMSTATE_UINT32(ctlr, GICState),
         VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
         VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
                              vmstate_gic_irq_state, gic_irq_state),
@@ -146,7 +146,7 @@ static void arm_gic_common_reset(DeviceState *dev)
             s->irq_target[i] = 1;
         }
     }
-    s->enabled = false;
+    s->ctlr = 0;
 }
 
 static Property arm_gic_common_properties[] = {
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 6ff6c7f..4e6456e 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -468,7 +468,7 @@ static void armv7m_nvic_reset(DeviceState *dev)
     s->gic.cpu_enabled[0] = true;
     s->gic.priority_mask[0] = 0x100;
     /* The NVIC as a whole is always enabled. */
-    s->gic.enabled = true;
+    s->gic.ctlr = 1;
     systick_reset(s);
 }
 
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index e8cf773..3b4b3fb 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -54,6 +54,8 @@
 #define GIC_SET_GROUP(irq, cm) (s->irq_state[irq].group |= (cm))
 #define GIC_TEST_GROUP(irq, cm) ((s->irq_state[irq].group & (cm)) != 0)
 
+#define GICD_CTLR_EN_GRP0 (1U << 0)
+#define GICD_CTLR_EN_GRP1 (1U << 1)
 
 /* 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 b78981e..d5d3877 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -52,7 +52,10 @@ typedef struct GICState {
 
     qemu_irq parent_irq[GIC_NCPU];
     qemu_irq parent_fiq[GIC_NCPU];
-    bool enabled;
+    /* GICD_CTLR; for a GIC with the security extensions the NS banked version
+     * of this register is just an alias of bit 1 of the S banked version.
+     */
+    uint32_t ctlr;
     bool cpu_enabled[GIC_NCPU];
 
     gic_irq_state irq_state[GIC_MAXIRQ];
-- 
1.9.1

[Qemu-devel] [PATCH v4 07/17] hw/intc/arm_gic: Make ICCBPR/GICC_BPR banked

[Peter Maydell]

From: Fabian Aggeler <aggelerf@ethz.ch>

This register is banked in GICs with Security Extensions. Storing the
non-secure copy of BPR in the abpr, which is an alias to the non-secure
copy for secure access. ABPR itself is only accessible from secure state
if the GIC implements Security Extensions.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-10-git-send-email-greg.bellows@linaro.org
[PMM: rewrote to fix style issues and correct handling of GICv2
 without security extensions]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c                | 31 ++++++++++++++++++++++++++-----
 include/hw/intc/arm_gic_common.h | 11 ++++++++---
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 4f13ff2..e6ad8de 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -762,7 +762,12 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
         *data = s->priority_mask[cpu];
         break;
     case 0x08: /* Binary Point */
-        *data = s->bpr[cpu];
+        if (s->security_extn && !attrs.secure) {
+            /* BPR is banked. Non-secure copy stored in ABPR. */
+            *data = s->abpr[cpu];
+        } else {
+            *data = s->bpr[cpu];
+        }
         break;
     case 0x0c: /* Acknowledge */
         *data = gic_acknowledge_irq(s, cpu);
@@ -774,7 +779,16 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
         *data = s->current_pending[cpu];
         break;
     case 0x1c: /* Aliased Binary Point */
-        *data = s->abpr[cpu];
+        /* GIC v2, no security: ABPR
+         * GIC v1, no security: not implemented (RAZ/WI)
+         * With security extensions, secure access: ABPR (alias of NS BPR)
+         * With security extensions, nonsecure access: RAZ/WI
+         */
+        if (!gic_has_groups(s) || (s->security_extn && !attrs.secure)) {
+            *data = 0;
+        } else {
+            *data = s->abpr[cpu];
+        }
         break;
     case 0xd0: case 0xd4: case 0xd8: case 0xdc:
         *data = s->apr[(offset - 0xd0) / 4][cpu];
@@ -799,14 +813,21 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
         s->priority_mask[cpu] = (value & 0xff);
         break;
     case 0x08: /* Binary Point */
-        s->bpr[cpu] = (value & 0x7);
+        if (s->security_extn && !attrs.secure) {
+            s->abpr[cpu] = MAX(value & 0x7, GIC_MIN_ABPR);
+        } else {
+            s->bpr[cpu] = MAX(value & 0x7, GIC_MIN_BPR);
+        }
         break;
     case 0x10: /* End Of Interrupt */
         gic_complete_irq(s, cpu, value & 0x3ff);
         return MEMTX_OK;
     case 0x1c: /* Aliased Binary Point */
-        if (s->revision >= 2) {
-            s->abpr[cpu] = (value & 0x7);
+        if (!gic_has_groups(s) || (s->security_extn && !attrs.secure)) {
+            /* unimplemented, or NS access: RAZ/WI */
+            return MEMTX_OK;
+        } else {
+            s->abpr[cpu] = MAX(value & 0x7, GIC_MIN_ABPR);
         }
         break;
     case 0xd0: case 0xd4: case 0xd8: case 0xdc:
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index d5d3877..261402f 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -34,6 +34,9 @@
 #define MAX_NR_GROUP_PRIO 128
 #define GIC_NR_APRS (MAX_NR_GROUP_PRIO / 32)
 
+#define GIC_MIN_BPR 0
+#define GIC_MIN_ABPR (GIC_MIN_BPR + 1)
+
 typedef struct gic_irq_state {
     /* The enable bits are only banked for per-cpu interrupts.  */
     uint8_t enabled;
@@ -76,9 +79,11 @@ 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.
+    /* If we present the GICv2 without security extensions to a guest,
+     * the guest can configure the GICC_CTLR to configure group 1 binary point
+     * in the abpr.
+     * For a GIC with Security Extensions we use use bpr for the
+     * secure copy and abpr as storage for the non-secure copy of the register.
      */
     uint8_t  bpr[GIC_NCPU];
     uint8_t  abpr[GIC_NCPU];
-- 
1.9.1

[Qemu-devel] [PATCH v4 08/17] hw/intc/arm_gic: Make ICCICR/GICC_CTLR banked

[Peter Maydell]

From: Fabian Aggeler <aggelerf@ethz.ch>

ICCICR/GICC_CTLR is banked in GICv1 implementations with Security
Extensions or in GICv2 in independent from Security Extensions.
This makes it possible to enable forwarding of interrupts from
the CPU interfaces to the connected processors for Group0 and Group1.

We also allow to set additional bits like AckCtl and FIQEn by changing
the type from bool to uint32. Since the field does not only store the
enable bit anymore and since we are touching the vmstate, we use the
opportunity to rename the field to cpu_ctlr.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-9-git-send-email-greg.bellows@linaro.org
[PMM: rewrote to store state in a single uint32_t rather than
 keeping the NS and S banked variants separate; this considerably
 simplifies the get/set functions]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c                | 51 ++++++++++++++++++++++++++++++++++++----
 hw/intc/arm_gic_common.c         |  8 +++----
 hw/intc/arm_gic_kvm.c            |  8 +++----
 hw/intc/armv7m_nvic.c            |  2 +-
 hw/intc/gic_internal.h           | 16 +++++++++++++
 include/hw/intc/arm_gic_common.h |  5 +++-
 6 files changed, 76 insertions(+), 14 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index e6ad8de..4aaaac2 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -68,7 +68,7 @@ void gic_update(GICState *s)
         cm = 1 << cpu;
         s->current_pending[cpu] = 1023;
         if (!(s->ctlr & (GICD_CTLR_EN_GRP0 | GICD_CTLR_EN_GRP1))
-            || !s->cpu_enabled[cpu]) {
+            || !(s->cpu_ctlr[cpu] & (GICC_CTLR_EN_GRP0 | GICC_CTLR_EN_GRP1))) {
             qemu_irq_lower(s->parent_irq[cpu]);
             return;
         }
@@ -242,6 +242,50 @@ void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
     }
 }
 
+static uint32_t gic_get_cpu_control(GICState *s, int cpu, MemTxAttrs attrs)
+{
+    uint32_t ret = s->cpu_ctlr[cpu];
+
+    if (s->security_extn && !attrs.secure) {
+        /* Construct the NS banked view of GICC_CTLR from the correct
+         * bits of the S banked view. We don't need to move the bypass
+         * control bits because we don't implement that (IMPDEF) part
+         * of the GIC architecture.
+         */
+        ret = (ret & (GICC_CTLR_EN_GRP1 | GICC_CTLR_EOIMODE_NS)) >> 1;
+    }
+    return ret;
+}
+
+static void gic_set_cpu_control(GICState *s, int cpu, uint32_t value,
+                                MemTxAttrs attrs)
+{
+    uint32_t mask;
+
+    if (s->security_extn && !attrs.secure) {
+        /* The NS view can only write certain bits in the register;
+         * the rest are unchanged
+         */
+        mask = GICC_CTLR_EN_GRP1;
+        if (s->revision == 2) {
+            mask |= GICC_CTLR_EOIMODE_NS;
+        }
+        s->cpu_ctlr[cpu] &= ~mask;
+        s->cpu_ctlr[cpu] |= (value << 1) & mask;
+    } else {
+        if (s->revision == 2) {
+            mask = s->security_extn ? GICC_CTLR_V2_S_MASK : GICC_CTLR_V2_MASK;
+        } else {
+            mask = s->security_extn ? GICC_CTLR_V1_S_MASK : GICC_CTLR_V1_MASK;
+        }
+        s->cpu_ctlr[cpu] = value & mask;
+    }
+    DPRINTF("CPU Interface %d: Group0 Interrupts %sabled, "
+            "Group1 Interrupts %sabled\n", cpu,
+            (s->cpu_ctlr[cpu] & GICC_CTLR_EN_GRP0) ? "En" : "Dis",
+            (s->cpu_ctlr[cpu] & GICC_CTLR_EN_GRP1) ? "En" : "Dis");
+}
+
 void gic_complete_irq(GICState *s, int cpu, int irq)
 {
     int update = 0;
@@ -756,7 +800,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
 {
     switch (offset) {
     case 0x00: /* Control */
-        *data = s->cpu_enabled[cpu];
+        *data = gic_get_cpu_control(s, cpu, attrs);
         break;
     case 0x04: /* Priority mask */
         *data = s->priority_mask[cpu];
@@ -806,8 +850,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
 {
     switch (offset) {
     case 0x00: /* Control */
-        s->cpu_enabled[cpu] = (value & 1);
-        DPRINTF("CPU %d %sabled\n", cpu, s->cpu_enabled[cpu] ? "En" : "Dis");
+        gic_set_cpu_control(s, cpu, value, attrs);
         break;
     case 0x04: /* Priority mask */
         s->priority_mask[cpu] = (value & 0xff);
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index bef76fc..044ad66 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -59,13 +59,13 @@ static const VMStateDescription vmstate_gic_irq_state = {
 
 static const VMStateDescription vmstate_gic = {
     .name = "arm_gic",
-    .version_id = 9,
-    .minimum_version_id = 9,
+    .version_id = 10,
+    .minimum_version_id = 10,
     .pre_save = gic_pre_save,
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(ctlr, GICState),
-        VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
+        VMSTATE_UINT32_ARRAY(cpu_ctlr, GICState, GIC_NCPU),
         VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
                              vmstate_gic_irq_state, gic_irq_state),
         VMSTATE_UINT8_ARRAY(irq_target, GICState, GIC_MAXIRQ),
@@ -134,7 +134,7 @@ static void arm_gic_common_reset(DeviceState *dev)
         s->current_pending[i] = 1023;
         s->running_irq[i] = 1023;
         s->running_priority[i] = 0x100;
-        s->cpu_enabled[i] = false;
+        s->cpu_ctlr[i] = 0;
     }
     for (i = 0; i < GIC_NR_SGIS; i++) {
         GIC_SET_ENABLED(i, ALL_CPU_MASK);
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 3591ca7..c5a2f81 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -414,8 +414,8 @@ static void kvm_arm_gic_put(GICState *s)
      */
 
     for (cpu = 0; cpu < s->num_cpu; cpu++) {
-        /* s->cpu_enabled[cpu] -> GICC_CTLR */
-        reg = s->cpu_enabled[cpu];
+        /* s->cpu_ctlr[cpu] -> GICC_CTLR */
+        reg = s->cpu_ctlr[cpu];
         kvm_gicc_access(s, 0x00, cpu, &reg, true);
 
         /* s->priority_mask[cpu] -> GICC_PMR */
@@ -506,9 +506,9 @@ static void kvm_arm_gic_get(GICState *s)
      */
 
     for (cpu = 0; cpu < s->num_cpu; cpu++) {
-        /* GICC_CTLR -> s->cpu_enabled[cpu] */
+        /* GICC_CTLR -> s->cpu_ctlr[cpu] */
         kvm_gicc_access(s, 0x00, cpu, &reg, false);
-        s->cpu_enabled[cpu] = (reg & 1);
+        s->cpu_ctlr[cpu] = reg;
 
         /* GICC_PMR -> s->priority_mask[cpu] */
         kvm_gicc_access(s, 0x04, cpu, &reg, false);
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index 4e6456e..c226daf 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -465,7 +465,7 @@ static void armv7m_nvic_reset(DeviceState *dev)
      * as enabled by default, and with a priority mask which allows
      * all interrupts through.
      */
-    s->gic.cpu_enabled[0] = true;
+    s->gic.cpu_ctlr[0] = GICC_CTLR_EN_GRP0;
     s->gic.priority_mask[0] = 0x100;
     /* The NVIC as a whole is always enabled. */
     s->gic.ctlr = 1;
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 3b4b3fb..81c764c 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -57,6 +57,22 @@
 #define GICD_CTLR_EN_GRP0 (1U << 0)
 #define GICD_CTLR_EN_GRP1 (1U << 1)
 
+#define GICC_CTLR_EN_GRP0    (1U << 0)
+#define GICC_CTLR_EN_GRP1    (1U << 1)
+#define GICC_CTLR_ACK_CTL    (1U << 2)
+#define GICC_CTLR_FIQ_EN     (1U << 3)
+#define GICC_CTLR_CBPR       (1U << 4) /* GICv1: SBPR */
+#define GICC_CTLR_EOIMODE    (1U << 9)
+#define GICC_CTLR_EOIMODE_NS (1U << 10)
+
+/* Valid bits for GICC_CTLR for GICv1, v1 with security extensions,
+ * GICv2 and GICv2 with security extensions:
+ */
+#define GICC_CTLR_V1_MASK    0x1
+#define GICC_CTLR_V1_S_MASK  0x1f
+#define GICC_CTLR_V2_MASK    0x21f
+#define GICC_CTLR_V2_S_MASK  0x61f
+
 /* The special cases for the revision property: */
 #define REV_11MPCORE 0
 #define REV_NVIC 0xffffffff
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 261402f..899db3d 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -59,7 +59,10 @@ typedef struct GICState {
      * of this register is just an alias of bit 1 of the S banked version.
      */
     uint32_t ctlr;
-    bool cpu_enabled[GIC_NCPU];
+    /* GICC_CTLR; again, the NS banked version is just aliases of bits of
+     * the S banked register, so our state only needs to store the S version.
+     */
+    uint32_t cpu_ctlr[GIC_NCPU];
 
     gic_irq_state irq_state[GIC_MAXIRQ];
     uint8_t irq_target[GIC_MAXIRQ];
-- 
1.9.1

[Qemu-devel] [PATCH v4 09/17] hw/intc/arm_gic: Implement Non-secure view of RPR

[Peter Maydell]

From: Fabian Aggeler <aggelerf@ethz.ch>

For GICs with Security Extensions Non-secure reads have a restricted
view on the current running priority.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-11-git-send-email-greg.bellows@linaro.org
[PMM: make function static, minor comment tweak]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 4aaaac2..e3bbe9e 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -286,6 +286,23 @@ static void gic_set_cpu_control(GICState *s, int cpu, uint32_t value,
             (s->cpu_ctlr[cpu] & GICC_CTLR_EN_GRP1) ? "En" : "Dis");
 }
 
+static uint8_t gic_get_running_priority(GICState *s, int cpu, MemTxAttrs attrs)
+{
+    if (s->security_extn && !attrs.secure) {
+        if (s->running_priority[cpu] & 0x80) {
+            /* Running priority in upper half of range: return the Non-secure
+             * view of the priority.
+             */
+            return s->running_priority[cpu] << 1;
+        } else {
+            /* Running priority in lower half of range: RAZ */
+            return 0;
+        }
+    } else {
+        return s->running_priority[cpu];
+    }
+}
+
 void gic_complete_irq(GICState *s, int cpu, int irq)
 {
     int update = 0;
@@ -817,7 +834,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
         *data = gic_acknowledge_irq(s, cpu);
         break;
     case 0x14: /* Running Priority */
-        *data = s->running_priority[cpu];
+        *data = gic_get_running_priority(s, cpu, attrs);
         break;
     case 0x18: /* Highest Pending Interrupt */
         *data = s->current_pending[cpu];
-- 
1.9.1

[Qemu-devel] [PATCH v4 10/17] hw/intc/arm_gic: Restrict priority view

[Peter Maydell]

From: Fabian Aggeler <aggelerf@ethz.ch>

GICs with Security Extensions restrict the non-secure view of the
interrupt priority and priority mask registers.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-15-git-send-email-greg.bellows@linaro.org
[PMM: minor code tweaks; fixed missing masking in gic_set_priority_mask
and gic_set_priority]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c      | 63 ++++++++++++++++++++++++++++++++++++++++++++++----
 hw/intc/arm_gic_kvm.c  |  2 +-
 hw/intc/gic_internal.h |  3 ++-
 3 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index e3bbe9e..7c0ddc8 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -233,8 +233,16 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
     return ret;
 }
 
-void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
+void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val,
+                      MemTxAttrs attrs)
 {
+    if (s->security_extn && !attrs.secure) {
+        if (!GIC_TEST_GROUP(irq, (1 << cpu))) {
+            return; /* Ignore Non-secure access of Group0 IRQ */
+        }
+        val = 0x80 | (val >> 1); /* Non-secure view */
+    }
+
     if (irq < GIC_INTERNAL) {
         s->priority1[irq][cpu] = val;
     } else {
@@ -242,6 +250,51 @@ void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
     }
 }
 
+static uint32_t gic_get_priority(GICState *s, int cpu, int irq,
+                                 MemTxAttrs attrs)
+{
+    uint32_t prio = GIC_GET_PRIORITY(irq, cpu);
+
+    if (s->security_extn && !attrs.secure) {
+        if (!GIC_TEST_GROUP(irq, (1 << cpu))) {
+            return 0; /* Non-secure access cannot read priority of Group0 IRQ */
+        }
+        prio = (prio << 1) & 0xff; /* Non-secure view */
+    }
+    return prio;
+}
+
+static void gic_set_priority_mask(GICState *s, int cpu, uint8_t pmask,
+                                  MemTxAttrs attrs)
+{
+    if (s->security_extn && !attrs.secure) {
+        if (s->priority_mask[cpu] & 0x80) {
+            /* Priority Mask in upper half */
+            pmask = 0x80 | (pmask >> 1);
+        } else {
+            /* Non-secure write ignored if priority mask is in lower half */
+            return;
+        }
+    }
+    s->priority_mask[cpu] = pmask;
+}
+
+static uint32_t gic_get_priority_mask(GICState *s, int cpu, MemTxAttrs attrs)
+{
+    uint32_t pmask = s->priority_mask[cpu];
+
+    if (s->security_extn && !attrs.secure) {
+        if (pmask & 0x80) {
+            /* Priority Mask in upper half, return Non-secure view */
+            pmask = (pmask << 1) & 0xff;
+        } else {
+            /* Priority Mask in lower half, RAZ */
+            pmask = 0;
+        }
+    }
+    return pmask;
+}
+
 static uint32_t gic_get_cpu_control(GICState *s, int cpu, MemTxAttrs attrs)
 {
     uint32_t ret = s->cpu_ctlr[cpu];
@@ -451,7 +504,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
         irq = (offset - 0x400) + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        res = GIC_GET_PRIORITY(irq, cpu);
+        res = gic_get_priority(s, cpu, irq, attrs);
     } else if (offset < 0xc00) {
         /* Interrupt CPU Target.  */
         if (s->num_cpu == 1 && s->revision != REV_11MPCORE) {
@@ -669,7 +722,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
         irq = (offset - 0x400) + GIC_BASE_IRQ;
         if (irq >= s->num_irq)
             goto bad_reg;
-        gic_set_priority(s, cpu, irq, value);
+        gic_set_priority(s, cpu, irq, value, attrs);
     } else if (offset < 0xc00) {
         /* Interrupt CPU Target. RAZ/WI on uniprocessor GICs, with the
          * annoying exception of the 11MPCore's GIC.
@@ -820,7 +873,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
         *data = gic_get_cpu_control(s, cpu, attrs);
         break;
     case 0x04: /* Priority mask */
-        *data = s->priority_mask[cpu];
+        *data = gic_get_priority_mask(s, cpu, attrs);
         break;
     case 0x08: /* Binary Point */
         if (s->security_extn && !attrs.secure) {
@@ -870,7 +923,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
         gic_set_cpu_control(s, cpu, value, attrs);
         break;
     case 0x04: /* Priority mask */
-        s->priority_mask[cpu] = (value & 0xff);
+        gic_set_priority_mask(s, cpu, value, attrs);
         break;
     case 0x08: /* Binary Point */
         if (s->security_extn && !attrs.secure) {
diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index c5a2f81..54f18df 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -251,7 +251,7 @@ static void translate_priority(GICState *s, int irq, int cpu,
     if (to_kernel) {
         *field = GIC_GET_PRIORITY(irq, cpu) & 0xff;
     } else {
-        gic_set_priority(s, cpu, irq, *field & 0xff);
+        gic_set_priority(s, cpu, irq, *field & 0xff, MEMTXATTRS_UNSPECIFIED);
     }
 }
 
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 81c764c..119fb81 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -82,7 +82,8 @@ 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);
-void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
+void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val,
+                      MemTxAttrs attrs);
 
 static inline bool gic_test_pending(GICState *s, int irq, int cm)
 {
-- 
1.9.1

[Qemu-devel] [PATCH v4 11/17] hw/intc/arm_gic: Handle grouping for GICC_HPPIR

[Peter Maydell]

From: Fabian Aggeler <aggelerf@ethz.ch>

Grouping (GICv2) and Security Extensions change the behaviour of reads
of the highest priority pending interrupt register (ICCHPIR/GICC_HPPIR).

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-12-git-send-email-greg.bellows@linaro.org
[PMM: make utility fn static; coding style fixes; AckCtl has an effect
 for GICv2 without security extensions as well; removed checks on enable
 bits because these are done when we set current_pending[cpu]]
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 7c0ddc8..75c69b3 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -176,6 +176,32 @@ static void gic_set_irq(void *opaque, int irq, int level)
     gic_update(s);
 }
 
+static uint16_t gic_get_current_pending_irq(GICState *s, int cpu,
+                                            MemTxAttrs attrs)
+{
+    uint16_t pending_irq = s->current_pending[cpu];
+
+    if (pending_irq < GIC_MAXIRQ && gic_has_groups(s)) {
+        int group = GIC_TEST_GROUP(pending_irq, (1 << cpu));
+        /* On a GIC without the security extensions, reading this register
+         * behaves in the same way as a secure access to a GIC with them.
+         */
+        bool secure = !s->security_extn || attrs.secure;
+
+        if (group == 0 && !secure) {
+            /* Group0 interrupts hidden from Non-secure access */
+            return 1023;
+        }
+        if (group == 1 && secure && !(s->cpu_ctlr[cpu] & GICC_CTLR_ACK_CTL)) {
+            /* Group1 interrupts only seen by Secure access if
+             * AckCtl bit set.
+             */
+            return 1022;
+        }
+    }
+    return pending_irq;
+}
+
 static void gic_set_running_irq(GICState *s, int cpu, int irq)
 {
     s->running_irq[cpu] = irq;
@@ -890,7 +916,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
         *data = gic_get_running_priority(s, cpu, attrs);
         break;
     case 0x18: /* Highest Pending Interrupt */
-        *data = s->current_pending[cpu];
+        *data = gic_get_current_pending_irq(s, cpu, attrs);
         break;
     case 0x1c: /* Aliased Binary Point */
         /* GIC v2, no security: ABPR
-- 
1.9.1

[Qemu-devel] [PATCH v4 12/17] hw/intc/arm_gic: Change behavior of EOIR writes

[Peter Maydell]

From: Fabian Aggeler <aggelerf@ethz.ch>

Grouping (GICv2) and Security Extensions change the behavior of EOIR
writes. Completing Group0 interrupts is only allowed from Secure state.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-13-git-send-email-greg.bellows@linaro.org
[PMM: Rather than go to great lengths to ignore the UNPREDICTABLE case
 of a Secure EOI of a Group1 (NS) irq with AckCtl == 0, we just let
 it fall through; add a comment about it.]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c      | 14 ++++++++++++--
 hw/intc/armv7m_nvic.c  |  2 +-
 hw/intc/gic_internal.h |  2 +-
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 75c69b3..4ad80e7 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -382,7 +382,7 @@ static uint8_t gic_get_running_priority(GICState *s, int cpu, MemTxAttrs attrs)
     }
 }
 
-void gic_complete_irq(GICState *s, int cpu, int irq)
+void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
 {
     int update = 0;
     int cm = 1 << cpu;
@@ -412,6 +412,16 @@ void gic_complete_irq(GICState *s, int cpu, int irq)
         }
     }
 
+    if (s->security_extn && !attrs.secure && !GIC_TEST_GROUP(irq, cm)) {
+        DPRINTF("Non-secure EOI for Group0 interrupt %d ignored\n", irq);
+        return;
+    }
+
+    /* Secure EOI with GICC_CTLR.AckCtl == 0 when the IRQ is a Group 1
+     * interrupt is UNPREDICTABLE. We choose to handle it as if AckCtl == 1,
+     * 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];
@@ -959,7 +969,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
         }
         break;
     case 0x10: /* End Of Interrupt */
-        gic_complete_irq(s, cpu, value & 0x3ff);
+        gic_complete_irq(s, cpu, value & 0x3ff, attrs);
         return MEMTX_OK;
     case 0x1c: /* Aliased Binary Point */
         if (!gic_has_groups(s) || (s->security_extn && !attrs.secure)) {
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index c226daf..dd06ceb 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -135,7 +135,7 @@ void armv7m_nvic_complete_irq(void *opaque, int irq)
     nvic_state *s = (nvic_state *)opaque;
     if (irq >= 16)
         irq += 16;
-    gic_complete_irq(&s->gic, 0, irq);
+    gic_complete_irq(&s->gic, 0, irq, MEMTXATTRS_UNSPECIFIED);
 }
 
 static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 119fb81..d3cebef 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -79,7 +79,7 @@
 
 void gic_set_pending_private(GICState *s, int cpu, int irq);
 uint32_t gic_acknowledge_irq(GICState *s, int cpu);
-void gic_complete_irq(GICState *s, int cpu, int irq);
+void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs);
 void gic_update(GICState *s);
 void gic_init_irqs_and_distributor(GICState *s);
 void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val,
-- 
1.9.1

[Qemu-devel] [PATCH v4 13/17] hw/intc/arm_gic: Change behavior of IAR writes

[Peter Maydell]

From: Fabian Aggeler <aggelerf@ethz.ch>

Grouping (GICv2) and Security Extensions change the behavior of IAR
reads. Acknowledging Group0 interrupts is only allowed from Secure
state and acknowledging Group1 interrupts from Secure state is only
allowed if AckCtl bit is set.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-14-git-send-email-greg.bellows@linaro.org
[PMM: simplify significantly by reusing the existing
 gic_get_current_pending_irq() rather than reimplementing the
 same logic here]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/arm_gic.c      | 22 ++++++++++++++++------
 hw/intc/armv7m_nvic.c  |  2 +-
 hw/intc/gic_internal.h |  2 +-
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 4ad80e7..6abdb14 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -213,14 +213,24 @@ static void gic_set_running_irq(GICState *s, int cpu, int irq)
     gic_update(s);
 }
 
-uint32_t gic_acknowledge_irq(GICState *s, int cpu)
+uint32_t gic_acknowledge_irq(GICState *s, int cpu, MemTxAttrs attrs)
 {
     int ret, irq, src;
     int cm = 1 << cpu;
-    irq = s->current_pending[cpu];
-    if (irq == 1023
-            || GIC_GET_PRIORITY(irq, cpu) >= s->running_priority[cpu]) {
-        DPRINTF("ACK no pending IRQ\n");
+
+    /* gic_get_current_pending_irq() will return 1022 or 1023 appropriately
+     * for the case where this GIC supports grouping and the pending interrupt
+     * is in the wrong group.
+     */
+    irq = gic_get_current_pending_irq(s, cpu, attrs);;
+
+    if (irq >= GIC_MAXIRQ) {
+        DPRINTF("ACK, no pending interrupt or it is hidden: %d\n", irq);
+        return irq;
+    }
+
+    if (GIC_GET_PRIORITY(irq, cpu) >= s->running_priority[cpu]) {
+        DPRINTF("ACK, pending interrupt (%d) has insufficient priority\n", irq);
         return 1023;
     }
     s->last_active[irq][cpu] = s->running_irq[cpu];
@@ -920,7 +930,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
         }
         break;
     case 0x0c: /* Acknowledge */
-        *data = gic_acknowledge_irq(s, cpu);
+        *data = gic_acknowledge_irq(s, cpu, attrs);
         break;
     case 0x14: /* Running Priority */
         *data = gic_get_running_priority(s, cpu, attrs);
diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
index dd06ceb..49368ca 100644
--- a/hw/intc/armv7m_nvic.c
+++ b/hw/intc/armv7m_nvic.c
@@ -122,7 +122,7 @@ int armv7m_nvic_acknowledge_irq(void *opaque)
     nvic_state *s = (nvic_state *)opaque;
     uint32_t irq;
 
-    irq = gic_acknowledge_irq(&s->gic, 0);
+    irq = gic_acknowledge_irq(&s->gic, 0, MEMTXATTRS_UNSPECIFIED);
     if (irq == 1023)
         hw_error("Interrupt but no vector\n");
     if (irq >= 32)
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index d3cebef..20c1e8a 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -78,7 +78,7 @@
 #define REV_NVIC 0xffffffff
 
 void gic_set_pending_private(GICState *s, int cpu, int irq);
-uint32_t gic_acknowledge_irq(GICState *s, int cpu);
+uint32_t gic_acknowledge_irq(GICState *s, int cpu, MemTxAttrs attrs);
 void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs);
 void gic_update(GICState *s);
 void gic_init_irqs_and_distributor(GICState *s);
-- 
1.9.1

[Qemu-devel] [PATCH v4 14/17] hw/intc/arm_gic: Add grouping support to gic_update()

[Peter Maydell]

Add support to gic_update() for determining the current IRQ
and FIQ status when interrupt grouping is supported. This
simply requires that instead of always raising IRQ we
check the group of the highest priority pending interrupt
and the GICC_CTLR.FIQEn bit to see whether we should raise
IRQ or FIQ.

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

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 6abdb14..c1d2e70 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -60,7 +60,7 @@ void gic_update(GICState *s)
     int best_irq;
     int best_prio;
     int irq;
-    int level;
+    int irq_level, fiq_level;
     int cpu;
     int cm;
 
@@ -70,6 +70,7 @@ void gic_update(GICState *s)
         if (!(s->ctlr & (GICD_CTLR_EN_GRP0 | GICD_CTLR_EN_GRP1))
             || !(s->cpu_ctlr[cpu] & (GICC_CTLR_EN_GRP0 | GICC_CTLR_EN_GRP1))) {
             qemu_irq_lower(s->parent_irq[cpu]);
+            qemu_irq_lower(s->parent_fiq[cpu]);
             return;
         }
         best_prio = 0x100;
@@ -83,15 +84,31 @@ void gic_update(GICState *s)
                 }
             }
         }
-        level = 0;
+
+        irq_level = fiq_level = 0;
+
         if (best_prio < s->priority_mask[cpu]) {
             s->current_pending[cpu] = best_irq;
             if (best_prio < s->running_priority[cpu]) {
-                DPRINTF("Raised pending IRQ %d (cpu %d)\n", best_irq, cpu);
-                level = 1;
+                int group = GIC_TEST_GROUP(best_irq, cm);
+
+                if (extract32(s->ctlr, group, 1) &&
+                    extract32(s->cpu_ctlr[cpu], group, 1)) {
+                    if (group == 0 && s->cpu_ctlr[cpu] & GICC_CTLR_FIQ_EN) {
+                        DPRINTF("Raised pending FIQ %d (cpu %d)\n",
+                                best_irq, cpu);
+                        fiq_level = 1;
+                    } else {
+                        DPRINTF("Raised pending IRQ %d (cpu %d)\n",
+                                best_irq, cpu);
+                        irq_level = 1;
+                    }
+                }
             }
         }
-        qemu_set_irq(s->parent_irq[cpu], level);
+
+        qemu_set_irq(s->parent_irq[cpu], irq_level);
+        qemu_set_irq(s->parent_fiq[cpu], fiq_level);
     }
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH v4 15/17] hw/arm/virt.c: Wire FIQ between CPU <> GIC

[Peter Maydell]

From: Greg Bellows <greg.bellows@linaro.org>

Connect FIQ output of the GIC CPU interfaces to the CPUs.

Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-4-git-send-email-greg.bellows@linaro.org
[PMM: minor format tweak]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 565f573..a7f9a10 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -386,6 +386,8 @@ static uint32_t create_gic(const VirtBoardInfo *vbi, qemu_irq *pic)
                               qdev_get_gpio_in(gicdev, ppibase + 27));
 
         sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
+        sysbus_connect_irq(gicbusdev, i + smp_cpus,
+                           qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
     }
 
     for (i = 0; i < NUM_IRQS; i++) {
-- 
1.9.1

[Qemu-devel] [PATCH v4 16/17] hw/arm/vexpress.c: Wire FIQ between CPU <> GIC

[Peter Maydell]

From: Fabian Aggeler <aggelerf@ethz.ch>

Connect FIQ output of the GIC CPU interfaces to the CPUs.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
Message-id: 1429113742-8371-3-git-send-email-greg.bellows@linaro.org
[PMM: minor format tweak]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/vexpress.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 3989bc5..8f1a5ea 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -253,6 +253,8 @@ static void init_cpus(const char *cpu_model, const char *privdev,
         DeviceState *cpudev = DEVICE(qemu_get_cpu(n));
 
         sysbus_connect_irq(busdev, n, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
+        sysbus_connect_irq(busdev, n + smp_cpus,
+                           qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
     }
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH v4 17/17] hw/arm/highbank.c: Wire FIQ between CPU <> GIC

[Peter Maydell]

Connect FIQ output of the GIC CPU interfaces to the CPUs.

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

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index b2d048b..f8353a7 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -217,6 +217,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
     qemu_irq pic[128];
     int n;
     qemu_irq cpu_irq[4];
+    qemu_irq cpu_fiq[4];
     MemoryRegion *sysram;
     MemoryRegion *dram;
     MemoryRegion *sysmem;
@@ -269,6 +270,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
             exit(1);
         }
         cpu_irq[n] = qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ);
+        cpu_fiq[n] = qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_FIQ);
     }
 
     sysmem = get_system_memory();
@@ -313,6 +315,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
     sysbus_mmio_map(busdev, 0, MPCORE_PERIPHBASE);
     for (n = 0; n < smp_cpus; n++) {
         sysbus_connect_irq(busdev, n, cpu_irq[n]);
+        sysbus_connect_irq(busdev, n + smp_cpus, cpu_fiq[n]);
     }
 
     for (n = 0; n < 128; n++) {
-- 
1.9.1

Re: [Qemu-devel] [PATCH v4 01/17] hw/intc/arm_gic: Create outbound FIQ lines

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:27PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> Create the outbound FIQ lines from the GIC to the CPUs; these are
> used if the GIC has security extensions or grouping support.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-2-git-send-email-greg.bellows@linaro.org
> [PMM: added FIQ lines to kvm-arm-gic so its interface is the same;
> tweaked commit message]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> ---
>  hw/intc/arm_gic.c                | 3 +++
>  hw/intc/arm_gic_kvm.c            | 5 ++++-
>  include/hw/intc/arm_gic_common.h | 1 +
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index a04c822..e9fb8b9 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -790,6 +790,9 @@ void gic_init_irqs_and_distributor(GICState *s)
>      for (i = 0; i < NUM_CPU(s); i++) {
>          sysbus_init_irq(sbd, &s->parent_irq[i]);
>      }
> +    for (i = 0; i < NUM_CPU(s); i++) {
> +        sysbus_init_irq(sbd, &s->parent_fiq[i]);
> +    }
>      memory_region_init_io(&s->iomem, OBJECT(s), &gic_dist_ops, s,
>                            "gic_dist", 0x1000);
>  }
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index e1952ad..5aedae1 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -554,12 +554,15 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>       */
>      i += (GIC_INTERNAL * s->num_cpu);
>      qdev_init_gpio_in(dev, kvm_arm_gic_set_irq, i);
> -    /* We never use our outbound IRQ lines but provide them so that
> +    /* We never use our outbound IRQ/FIQ lines but provide them so that
>       * we maintain the same interface as the non-KVM GIC.
>       */
>      for (i = 0; i < s->num_cpu; i++) {
>          sysbus_init_irq(sbd, &s->parent_irq[i]);
>      }
> +    for (i = 0; i < s->num_cpu; i++) {
> +        sysbus_init_irq(sbd, &s->parent_fiq[i]);
> +    }
>  
>      /* Try to create the device via the device control API */
>      s->dev_fd = -1;
> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
> index f6887ed..01c6f24 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -50,6 +50,7 @@ typedef struct GICState {
>      /*< public >*/
>  
>      qemu_irq parent_irq[GIC_NCPU];
> +    qemu_irq parent_fiq[GIC_NCPU];
>      bool enabled;
>      bool cpu_enabled[GIC_NCPU];
>  
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 02/17] hw/intc/arm_gic: Add Security Extensions property

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:28PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> Add a QOM property which allows the GIC Security Extensions to be
> enabled. These are an optional part of the GICv1 and GICv2 architecture.
> This commit just adds the property and some sanity checks that it
> is only enabled on GIC revisions that support it.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-5-git-send-email-greg.bellows@linaro.org
> [PMM: changed property name, added checks that it isn't set for
>  older GIC revisions or if using the KVM VGIC; reworded commit message]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gic.c                | 5 ++++-
>  hw/intc/arm_gic_common.c         | 9 +++++++++
>  hw/intc/arm_gic_kvm.c            | 6 ++++++
>  include/hw/intc/arm_gic_common.h | 1 +
>  4 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index e9fb8b9..cdf7408 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -298,7 +298,10 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
>          if (offset == 0)
>              return s->enabled;
>          if (offset == 4)
> -            return ((s->num_irq / 32) - 1) | ((NUM_CPU(s) - 1) << 5);
> +            /* Interrupt Controller Type Register */
> +            return ((s->num_irq / 32) - 1)
> +                    | ((NUM_CPU(s) - 1) << 5)
> +                    | (s->security_extn << 10);
>          if (offset < 0x08)
>              return 0;
>          if (offset >= 0x80) {
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index 18b01ba..5ed21f1 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -110,6 +110,13 @@ static void arm_gic_common_realize(DeviceState *dev, Error **errp)
>                     num_irq);
>          return;
>      }
> +
> +    if (s->security_extn &&
> +        (s->revision == REV_11MPCORE || s->revision == REV_NVIC)) {
> +        error_setg(errp, "this GIC revision does not implement "
> +                   "the security extensions");
> +        return;
> +    }
>  }
>  
>  static void arm_gic_common_reset(DeviceState *dev)
> @@ -149,6 +156,8 @@ static Property arm_gic_common_properties[] = {
>       * (Internally, 0xffffffff also indicates "not a GIC but an NVIC".)
>       */
>      DEFINE_PROP_UINT32("revision", GICState, revision, 1),
> +    /* True if the GIC should implement the security extensions */
> +    DEFINE_PROP_BOOL("has-security-extensions", GICState, security_extn, 0),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index 5aedae1..cb47b12 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -544,6 +544,12 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    if (s->security_extn) {
> +        error_setg(errp, "the in-kernel VGIC does not implement the "
> +                   "security extensions");
> +        return;
> +    }
> +
>      i = s->num_irq - GIC_INTERNAL;
>      /* For the GIC, also expose incoming GPIO lines for PPIs for each CPU.
>       * GPIO array layout is thus:
> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
> index 01c6f24..7825134 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -105,6 +105,7 @@ typedef struct GICState {
>      MemoryRegion cpuiomem[GIC_NCPU + 1]; /* CPU interfaces */
>      uint32_t num_irq;
>      uint32_t revision;
> +    bool security_extn;
>      int dev_fd; /* kvm device fd if backed by kvm vgic support */
>  } GICState;
>  
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 03/17] hw/intc/arm_gic: Switch to read/write callbacks with tx attributes

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:29PM +0100, Peter Maydell wrote:
> Switch the GIC's MMIO callback functions to the read_with_attrs
> and write_with_attrs functions which provide MemTxAttrs. This will
> allow the GIC to correctly handle secure and nonsecure register
> accesses.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gic.c | 144 ++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 90 insertions(+), 54 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index cdf7408..29015c2 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -282,7 +282,7 @@ void gic_complete_irq(GICState *s, int cpu, int irq)
>      }
>  }
>  
> -static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
> +static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
>  {
>      GICState *s = (GICState *)opaque;
>      uint32_t res;
> @@ -418,24 +418,30 @@ bad_reg:
>      return 0;
>  }
>  
> -static uint32_t gic_dist_readw(void *opaque, hwaddr offset)
> +static MemTxResult gic_dist_read(void *opaque, hwaddr offset, uint64_t *data,
> +                                 unsigned size, MemTxAttrs attrs)
>  {
> -    uint32_t val;
> -    val = gic_dist_readb(opaque, offset);
> -    val |= gic_dist_readb(opaque, offset + 1) << 8;
> -    return val;
> -}
> -
> -static uint32_t gic_dist_readl(void *opaque, hwaddr offset)
> -{
> -    uint32_t val;
> -    val = gic_dist_readw(opaque, offset);
> -    val |= gic_dist_readw(opaque, offset + 2) << 16;
> -    return val;
> +    switch (size) {
> +    case 1:
> +        *data = gic_dist_readb(opaque, offset, attrs);
> +        return MEMTX_OK;
> +    case 2:
> +        *data = gic_dist_readb(opaque, offset, attrs);
> +        *data |= gic_dist_readb(opaque, offset + 1, attrs) << 8;
> +        return MEMTX_OK;
> +    case 4:
> +        *data = gic_dist_readb(opaque, offset, attrs);
> +        *data |= gic_dist_readb(opaque, offset + 1, attrs) << 8;
> +        *data |= gic_dist_readb(opaque, offset + 2, attrs) << 16;
> +        *data |= gic_dist_readb(opaque, offset + 3, attrs) << 24;
> +        return MEMTX_OK;
> +    default:
> +        return MEMTX_ERROR;
> +    }
>  }
>  
>  static void gic_dist_writeb(void *opaque, hwaddr offset,
> -                            uint32_t value)
> +                            uint32_t value, MemTxAttrs attrs)
>  {
>      GICState *s = (GICState *)opaque;
>      int irq;
> @@ -612,14 +618,14 @@ bad_reg:
>  }
>  
>  static void gic_dist_writew(void *opaque, hwaddr offset,
> -                            uint32_t value)
> +                            uint32_t value, MemTxAttrs attrs)
>  {
> -    gic_dist_writeb(opaque, offset, value & 0xff);
> -    gic_dist_writeb(opaque, offset + 1, value >> 8);
> +    gic_dist_writeb(opaque, offset, value & 0xff, attrs);
> +    gic_dist_writeb(opaque, offset + 1, value >> 8, attrs);
>  }
>  
>  static void gic_dist_writel(void *opaque, hwaddr offset,
> -                            uint32_t value)
> +                            uint32_t value, MemTxAttrs attrs)
>  {
>      GICState *s = (GICState *)opaque;
>      if (offset == 0xf00) {
> @@ -655,45 +661,72 @@ static void gic_dist_writel(void *opaque, hwaddr offset,
>          gic_update(s);
>          return;
>      }
> -    gic_dist_writew(opaque, offset, value & 0xffff);
> -    gic_dist_writew(opaque, offset + 2, value >> 16);
> +    gic_dist_writew(opaque, offset, value & 0xffff, attrs);
> +    gic_dist_writew(opaque, offset + 2, value >> 16, attrs);
> +}
> +
> +static MemTxResult gic_dist_write(void *opaque, hwaddr offset, uint64_t data,
> +                                  unsigned size, MemTxAttrs attrs)
> +{
> +    switch (size) {
> +    case 1:
> +        gic_dist_writeb(opaque, offset, data, attrs);
> +        return MEMTX_OK;
> +    case 2:
> +        gic_dist_writew(opaque, offset, data, attrs);
> +        return MEMTX_OK;
> +    case 4:
> +        gic_dist_writel(opaque, offset, data, attrs);
> +        return MEMTX_OK;
> +    default:
> +        return MEMTX_ERROR;
> +    }
>  }
>  
>  static const MemoryRegionOps gic_dist_ops = {
> -    .old_mmio = {
> -        .read = { gic_dist_readb, gic_dist_readw, gic_dist_readl, },
> -        .write = { gic_dist_writeb, gic_dist_writew, gic_dist_writel, },
> -    },
> +    .read_with_attrs = gic_dist_read,
> +    .write_with_attrs = gic_dist_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
> +static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
> +                                uint64_t *data, MemTxAttrs attrs)
>  {
>      switch (offset) {
>      case 0x00: /* Control */
> -        return s->cpu_enabled[cpu];
> +        *data = s->cpu_enabled[cpu];
> +        break;
>      case 0x04: /* Priority mask */
> -        return s->priority_mask[cpu];
> +        *data = s->priority_mask[cpu];
> +        break;
>      case 0x08: /* Binary Point */
> -        return s->bpr[cpu];
> +        *data = s->bpr[cpu];
> +        break;
>      case 0x0c: /* Acknowledge */
> -        return gic_acknowledge_irq(s, cpu);
> +        *data = gic_acknowledge_irq(s, cpu);
> +        break;
>      case 0x14: /* Running Priority */
> -        return s->running_priority[cpu];
> +        *data = s->running_priority[cpu];
> +        break;
>      case 0x18: /* Highest Pending Interrupt */
> -        return s->current_pending[cpu];
> +        *data = s->current_pending[cpu];
> +        break;
>      case 0x1c: /* Aliased Binary Point */
> -        return s->abpr[cpu];
> +        *data = s->abpr[cpu];
> +        break;
>      case 0xd0: case 0xd4: case 0xd8: case 0xdc:
> -        return s->apr[(offset - 0xd0) / 4][cpu];
> +        *data = s->apr[(offset - 0xd0) / 4][cpu];
> +        break;
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "gic_cpu_read: Bad offset %x\n", (int)offset);
> -        return 0;
> +        return MEMTX_ERROR;
>      }
> +    return MEMTX_OK;
>  }
>  
> -static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
> +static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
> +                                 uint32_t value, MemTxAttrs attrs)
>  {
>      switch (offset) {
>      case 0x00: /* Control */
> @@ -708,7 +741,7 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
>          break;
>      case 0x10: /* End Of Interrupt */
>          gic_complete_irq(s, cpu, value & 0x3ff);
> -        return;
> +        return MEMTX_OK;
>      case 0x1c: /* Aliased Binary Point */
>          if (s->revision >= 2) {
>              s->abpr[cpu] = (value & 0x7);
> @@ -720,56 +753,59 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
>      default:
>          qemu_log_mask(LOG_GUEST_ERROR,
>                        "gic_cpu_write: Bad offset %x\n", (int)offset);
> -        return;
> +        return MEMTX_ERROR;
>      }
>      gic_update(s);
> +    return MEMTX_OK;
>  }
>  
>  /* Wrappers to read/write the GIC CPU interface for the current CPU */
> -static uint64_t gic_thiscpu_read(void *opaque, hwaddr addr,
> -                                 unsigned size)
> +static MemTxResult gic_thiscpu_read(void *opaque, hwaddr addr, uint64_t *data,
> +                                    unsigned size, MemTxAttrs attrs)
>  {
>      GICState *s = (GICState *)opaque;
> -    return gic_cpu_read(s, gic_get_current_cpu(s), addr);
> +    return gic_cpu_read(s, gic_get_current_cpu(s), addr, data, attrs);
>  }
>  
> -static void gic_thiscpu_write(void *opaque, hwaddr addr,
> -                              uint64_t value, unsigned size)
> +static MemTxResult gic_thiscpu_write(void *opaque, hwaddr addr,
> +                                     uint64_t value, unsigned size,
> +                                     MemTxAttrs attrs)
>  {
>      GICState *s = (GICState *)opaque;
> -    gic_cpu_write(s, gic_get_current_cpu(s), addr, value);
> +    return gic_cpu_write(s, gic_get_current_cpu(s), addr, value, attrs);
>  }
>  
>  /* Wrappers to read/write the GIC CPU interface for a specific CPU.
>   * These just decode the opaque pointer into GICState* + cpu id.
>   */
> -static uint64_t gic_do_cpu_read(void *opaque, hwaddr addr,
> -                                unsigned size)
> +static MemTxResult gic_do_cpu_read(void *opaque, hwaddr addr, uint64_t *data,
> +                                   unsigned size, MemTxAttrs attrs)
>  {
>      GICState **backref = (GICState **)opaque;
>      GICState *s = *backref;
>      int id = (backref - s->backref);
> -    return gic_cpu_read(s, id, addr);
> +    return gic_cpu_read(s, id, addr, data, attrs);
>  }
>  
> -static void gic_do_cpu_write(void *opaque, hwaddr addr,
> -                             uint64_t value, unsigned size)
> +static MemTxResult gic_do_cpu_write(void *opaque, hwaddr addr,
> +                                    uint64_t value, unsigned size,
> +                                    MemTxAttrs attrs)
>  {
>      GICState **backref = (GICState **)opaque;
>      GICState *s = *backref;
>      int id = (backref - s->backref);
> -    gic_cpu_write(s, id, addr, value);
> +    return gic_cpu_write(s, id, addr, value, attrs);
>  }
>  
>  static const MemoryRegionOps gic_thiscpu_ops = {
> -    .read = gic_thiscpu_read,
> -    .write = gic_thiscpu_write,
> +    .read_with_attrs = gic_thiscpu_read,
> +    .write_with_attrs = gic_thiscpu_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
>  static const MemoryRegionOps gic_cpu_ops = {
> -    .read = gic_do_cpu_read,
> -    .write = gic_do_cpu_write,
> +    .read_with_attrs = gic_do_cpu_read,
> +    .write_with_attrs = gic_do_cpu_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 04/17] hw/intc/arm_gic: Add Interrupt Group Registers

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:30PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> The Interrupt Group Registers allow the guest to configure interrupts
> into one of two groups, where Group0 are higher priority and may
> be routed to IRQ or FIQ, and Group1 are lower priority and always
> routed to IRQ. (In a GIC with the security extensions Group0 is
> Secure interrupts and Group 1 is NonSecure.)
> The GICv2 always supports interrupt grouping; the GICv1 does only
> if it implements the security extensions.
> 
> This patch implements the ability to read and write the registers;
> the actual functionality the bits control will be added in a
> subsequent patch.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-7-git-send-email-greg.bellows@linaro.org
> [PMM: bring GIC_*_GROUP macros into line with the others, ie a
>  simple SET/CLEAR/TEST rather than GROUP0/GROUP1;
>  utility gic_has_groups() function;
>  minor style fixes;
>  bump vmstate version]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

I see now why we are doing the mask thing on the group bits to support the
banking of private interrupts...
The comment,
/* Group bits are banked for private interrupts */
would have been more useful to me close to the definition of the GIC_SET_GROUP
macros but that may just be me...

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gic.c                | 50 +++++++++++++++++++++++++++++++++++++---
>  hw/intc/arm_gic_common.c         |  5 ++--
>  hw/intc/gic_internal.h           |  4 ++++
>  include/hw/intc/arm_gic_common.h |  1 +
>  4 files changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 29015c2..1aa4520 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -45,6 +45,14 @@ static inline int gic_get_current_cpu(GICState *s)
>      return 0;
>  }
>  
> +/* Return true if this GIC config has interrupt groups, which is
> + * true if we're a GICv2, or a GICv1 with the security extensions.
> + */
> +static inline bool gic_has_groups(GICState *s)
> +{
> +    return s->revision == 2 || s->security_extn;
> +}
> +
>  /* TODO: Many places that call this routine could be optimized.  */
>  /* Update interrupt status after enabled or pending bits have been changed.  */
>  void gic_update(GICState *s)
> @@ -305,8 +313,24 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
>          if (offset < 0x08)
>              return 0;
>          if (offset >= 0x80) {
> -            /* Interrupt Security , RAZ/WI */
> -            return 0;
> +            /* Interrupt Group Registers: these RAZ/WI if this is an NS
> +             * access to a GIC with the security extensions, or if the GIC
> +             * doesn't have groups at all.
> +             */
> +            res = 0;
> +            if (!(s->security_extn && !attrs.secure) && gic_has_groups(s)) {
> +                /* Every byte offset holds 8 group status bits */
> +                irq = (offset - 0x080) * 8 + GIC_BASE_IRQ;
> +                if (irq >= s->num_irq) {
> +                    goto bad_reg;
> +                }
> +                for (i = 0; i < 8; i++) {
> +                    if (GIC_TEST_GROUP(irq + i, cm)) {
> +                        res |= (1 << i);
> +                    }
> +                }
> +            }
> +            return res;
>          }
>          goto bad_reg;
>      } else if (offset < 0x200) {
> @@ -456,7 +480,27 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>          } else if (offset < 4) {
>              /* ignored.  */
>          } else if (offset >= 0x80) {
> -            /* Interrupt Security Registers, RAZ/WI */
> +            /* Interrupt Group Registers: RAZ/WI for NS access to secure
> +             * GIC, or for GICs without groups.
> +             */
> +            if (!(s->security_extn && !attrs.secure) && gic_has_groups(s)) {
> +                /* Every byte offset holds 8 group status bits */
> +                irq = (offset - 0x80) * 8 + GIC_BASE_IRQ;
> +                if (irq >= s->num_irq) {
> +                    goto bad_reg;
> +                }
> +                for (i = 0; i < 8; i++) {
> +                    /* Group bits are banked for private interrupts */
> +                    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +                    if (value & (1 << i)) {
> +                        /* Group1 (Non-secure) */
> +                        GIC_SET_GROUP(irq + i, cm);
> +                    } else {
> +                        /* Group0 (Secure) */
> +                        GIC_CLEAR_GROUP(irq + i, cm);
> +                    }
> +                }
> +            }
>          } else {
>              goto bad_reg;
>          }
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index 5ed21f1..b5a85e5 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -52,14 +52,15 @@ static const VMStateDescription vmstate_gic_irq_state = {
>          VMSTATE_UINT8(level, gic_irq_state),
>          VMSTATE_BOOL(model, gic_irq_state),
>          VMSTATE_BOOL(edge_trigger, gic_irq_state),
> +        VMSTATE_UINT8(group, gic_irq_state),
>          VMSTATE_END_OF_LIST()
>      }
>  };
>  
>  static const VMStateDescription vmstate_gic = {
>      .name = "arm_gic",
> -    .version_id = 7,
> -    .minimum_version_id = 7,
> +    .version_id = 8,
> +    .minimum_version_id = 8,
>      .pre_save = gic_pre_save,
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index e87ef36..e8cf773 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -50,6 +50,10 @@
>                                      s->priority1[irq][cpu] :            \
>                                      s->priority2[(irq) - GIC_INTERNAL])
>  #define GIC_TARGET(irq) s->irq_target[irq]
> +#define GIC_CLEAR_GROUP(irq, cm) (s->irq_state[irq].group &= ~(cm))
> +#define GIC_SET_GROUP(irq, cm) (s->irq_state[irq].group |= (cm))
> +#define GIC_TEST_GROUP(irq, cm) ((s->irq_state[irq].group & (cm)) != 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 7825134..b78981e 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -42,6 +42,7 @@ typedef struct gic_irq_state {
>      uint8_t level;
>      bool model; /* 0 = N:N, 1 = 1:N */
>      bool edge_trigger; /* true: edge-triggered, false: level-triggered  */
> +    uint8_t group;
>  } gic_irq_state;
>  
>  typedef struct GICState {
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 06/17] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:32PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> ICDDCR/GICD_CTLR is banked if the GIC has the security extensions,
> and the S (or only) copy has separate enable bits for Group0 and
> Group1 enable if the GIC implements interrupt groups.
> 
> EnableGroup0 (Bit [1]) in GICv1 is architecturally IMPDEF. Since this
> bit (Enable Non-secure) is present in the integrated GIC of the Cortex-A9
> MPCore, we support this bit in our GICv1 implementation too.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-8-git-send-email-greg.bellows@linaro.org
> [PMM: rewritten to store the state in a single s->ctlr uint32,
>  with the NS register handled as an alias of bit 1 in that value;
>  added vmstate version bump]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gic.c                | 28 +++++++++++++++++++++++-----
>  hw/intc/arm_gic_common.c         |  8 ++++----
>  hw/intc/armv7m_nvic.c            |  2 +-
>  hw/intc/gic_internal.h           |  2 ++
>  include/hw/intc/arm_gic_common.h |  5 ++++-
>  5 files changed, 34 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 1aa4520..4f13ff2 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -67,7 +67,8 @@ void gic_update(GICState *s)
>      for (cpu = 0; cpu < NUM_CPU(s); cpu++) {
>          cm = 1 << cpu;
>          s->current_pending[cpu] = 1023;
> -        if (!s->enabled || !s->cpu_enabled[cpu]) {
> +        if (!(s->ctlr & (GICD_CTLR_EN_GRP0 | GICD_CTLR_EN_GRP1))
> +            || !s->cpu_enabled[cpu]) {
>              qemu_irq_lower(s->parent_irq[cpu]);
>              return;
>          }
> @@ -303,8 +304,16 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
>      cpu = gic_get_current_cpu(s);
>      cm = 1 << cpu;
>      if (offset < 0x100) {
> -        if (offset == 0)
> -            return s->enabled;
> +        if (offset == 0) {      /* GICD_CTLR */
> +            if (s->security_extn && !attrs.secure) {
> +                /* The NS bank of this register is just an alias of the
> +                 * EnableGrp1 bit in the S bank version.
> +                 */
> +                return extract32(s->ctlr, 1, 1);
> +            } else {
> +                return s->ctlr;
> +            }
> +        }
>          if (offset == 4)
>              /* Interrupt Controller Type Register */
>              return ((s->num_irq / 32) - 1)
> @@ -475,8 +484,17 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>      cpu = gic_get_current_cpu(s);
>      if (offset < 0x100) {
>          if (offset == 0) {
> -            s->enabled = (value & 1);
> -            DPRINTF("Distribution %sabled\n", s->enabled ? "En" : "Dis");
> +            if (s->security_extn && !attrs.secure) {
> +                /* NS version is just an alias of the S version's bit 1 */
> +                s->ctlr = deposit32(s->ctlr, 1, 1, value);
> +            } else if (gic_has_groups(s)) {
> +                s->ctlr = value & (GICD_CTLR_EN_GRP0 | GICD_CTLR_EN_GRP1);
> +            } else {
> +                s->ctlr = value & GICD_CTLR_EN_GRP0;
> +            }
> +            DPRINTF("Distributor: Group0 %sabled; Group 1 %sabled\n",
> +                    s->ctlr & GICD_CTLR_EN_GRP0 ? "En" : "Dis",
> +                    s->ctlr & GICD_CTLR_EN_GRP1 ? "En" : "Dis");
>          } else if (offset < 4) {
>              /* ignored.  */
>          } else if (offset >= 0x80) {
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index b5a85e5..bef76fc 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -59,12 +59,12 @@ static const VMStateDescription vmstate_gic_irq_state = {
>  
>  static const VMStateDescription vmstate_gic = {
>      .name = "arm_gic",
> -    .version_id = 8,
> -    .minimum_version_id = 8,
> +    .version_id = 9,
> +    .minimum_version_id = 9,
>      .pre_save = gic_pre_save,
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {
> -        VMSTATE_BOOL(enabled, GICState),
> +        VMSTATE_UINT32(ctlr, GICState),
>          VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
>          VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
>                               vmstate_gic_irq_state, gic_irq_state),
> @@ -146,7 +146,7 @@ static void arm_gic_common_reset(DeviceState *dev)
>              s->irq_target[i] = 1;
>          }
>      }
> -    s->enabled = false;
> +    s->ctlr = 0;
>  }
>  
>  static Property arm_gic_common_properties[] = {
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 6ff6c7f..4e6456e 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -468,7 +468,7 @@ static void armv7m_nvic_reset(DeviceState *dev)
>      s->gic.cpu_enabled[0] = true;
>      s->gic.priority_mask[0] = 0x100;
>      /* The NVIC as a whole is always enabled. */
> -    s->gic.enabled = true;
> +    s->gic.ctlr = 1;
>      systick_reset(s);
>  }
>  
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index e8cf773..3b4b3fb 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -54,6 +54,8 @@
>  #define GIC_SET_GROUP(irq, cm) (s->irq_state[irq].group |= (cm))
>  #define GIC_TEST_GROUP(irq, cm) ((s->irq_state[irq].group & (cm)) != 0)
>  
> +#define GICD_CTLR_EN_GRP0 (1U << 0)
> +#define GICD_CTLR_EN_GRP1 (1U << 1)
>  
>  /* 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 b78981e..d5d3877 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -52,7 +52,10 @@ typedef struct GICState {
>  
>      qemu_irq parent_irq[GIC_NCPU];
>      qemu_irq parent_fiq[GIC_NCPU];
> -    bool enabled;
> +    /* GICD_CTLR; for a GIC with the security extensions the NS banked version
> +     * of this register is just an alias of bit 1 of the S banked version.
> +     */
> +    uint32_t ctlr;
>      bool cpu_enabled[GIC_NCPU];
>  
>      gic_irq_state irq_state[GIC_MAXIRQ];
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 07/17] hw/intc/arm_gic: Make ICCBPR/GICC_BPR banked

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:33PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> This register is banked in GICs with Security Extensions. Storing the
> non-secure copy of BPR in the abpr, which is an alias to the non-secure
> copy for secure access. ABPR itself is only accessible from secure state
> if the GIC implements Security Extensions.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-10-git-send-email-greg.bellows@linaro.org
> [PMM: rewrote to fix style issues and correct handling of GICv2
>  without security extensions]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gic.c                | 31 ++++++++++++++++++++++++++-----
>  include/hw/intc/arm_gic_common.h | 11 ++++++++---
>  2 files changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 4f13ff2..e6ad8de 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -762,7 +762,12 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>          *data = s->priority_mask[cpu];
>          break;
>      case 0x08: /* Binary Point */
> -        *data = s->bpr[cpu];
> +        if (s->security_extn && !attrs.secure) {
> +            /* BPR is banked. Non-secure copy stored in ABPR. */
> +            *data = s->abpr[cpu];
> +        } else {
> +            *data = s->bpr[cpu];
> +        }
>          break;
>      case 0x0c: /* Acknowledge */
>          *data = gic_acknowledge_irq(s, cpu);
> @@ -774,7 +779,16 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>          *data = s->current_pending[cpu];
>          break;
>      case 0x1c: /* Aliased Binary Point */
> -        *data = s->abpr[cpu];
> +        /* GIC v2, no security: ABPR
> +         * GIC v1, no security: not implemented (RAZ/WI)
> +         * With security extensions, secure access: ABPR (alias of NS BPR)
> +         * With security extensions, nonsecure access: RAZ/WI
> +         */
> +        if (!gic_has_groups(s) || (s->security_extn && !attrs.secure)) {
> +            *data = 0;
> +        } else {
> +            *data = s->abpr[cpu];
> +        }
>          break;
>      case 0xd0: case 0xd4: case 0xd8: case 0xdc:
>          *data = s->apr[(offset - 0xd0) / 4][cpu];
> @@ -799,14 +813,21 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
>          s->priority_mask[cpu] = (value & 0xff);
>          break;
>      case 0x08: /* Binary Point */
> -        s->bpr[cpu] = (value & 0x7);
> +        if (s->security_extn && !attrs.secure) {
> +            s->abpr[cpu] = MAX(value & 0x7, GIC_MIN_ABPR);
> +        } else {
> +            s->bpr[cpu] = MAX(value & 0x7, GIC_MIN_BPR);
> +        }
>          break;
>      case 0x10: /* End Of Interrupt */
>          gic_complete_irq(s, cpu, value & 0x3ff);
>          return MEMTX_OK;
>      case 0x1c: /* Aliased Binary Point */
> -        if (s->revision >= 2) {
> -            s->abpr[cpu] = (value & 0x7);
> +        if (!gic_has_groups(s) || (s->security_extn && !attrs.secure)) {
> +            /* unimplemented, or NS access: RAZ/WI */
> +            return MEMTX_OK;
> +        } else {
> +            s->abpr[cpu] = MAX(value & 0x7, GIC_MIN_ABPR);
>          }
>          break;
>      case 0xd0: case 0xd4: case 0xd8: case 0xdc:
> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
> index d5d3877..261402f 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -34,6 +34,9 @@
>  #define MAX_NR_GROUP_PRIO 128
>  #define GIC_NR_APRS (MAX_NR_GROUP_PRIO / 32)
>  
> +#define GIC_MIN_BPR 0
> +#define GIC_MIN_ABPR (GIC_MIN_BPR + 1)
> +
>  typedef struct gic_irq_state {
>      /* The enable bits are only banked for per-cpu interrupts.  */
>      uint8_t enabled;
> @@ -76,9 +79,11 @@ 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.
> +    /* If we present the GICv2 without security extensions to a guest,
> +     * the guest can configure the GICC_CTLR to configure group 1 binary point
> +     * in the abpr.
> +     * For a GIC with Security Extensions we use use bpr for the
> +     * secure copy and abpr as storage for the non-secure copy of the register.
>       */
>      uint8_t  bpr[GIC_NCPU];
>      uint8_t  abpr[GIC_NCPU];
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 08/17] hw/intc/arm_gic: Make ICCICR/GICC_CTLR banked

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:34PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> ICCICR/GICC_CTLR is banked in GICv1 implementations with Security
> Extensions or in GICv2 in independent from Security Extensions.
> This makes it possible to enable forwarding of interrupts from
> the CPU interfaces to the connected processors for Group0 and Group1.
> 
> We also allow to set additional bits like AckCtl and FIQEn by changing
> the type from bool to uint32. Since the field does not only store the
> enable bit anymore and since we are touching the vmstate, we use the
> opportunity to rename the field to cpu_ctlr.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-9-git-send-email-greg.bellows@linaro.org
> [PMM: rewrote to store state in a single uint32_t rather than
>  keeping the NS and S banked variants separate; this considerably
>  simplifies the get/set functions]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gic.c                | 51 ++++++++++++++++++++++++++++++++++++----
>  hw/intc/arm_gic_common.c         |  8 +++----
>  hw/intc/arm_gic_kvm.c            |  8 +++----
>  hw/intc/armv7m_nvic.c            |  2 +-
>  hw/intc/gic_internal.h           | 16 +++++++++++++
>  include/hw/intc/arm_gic_common.h |  5 +++-
>  6 files changed, 76 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index e6ad8de..4aaaac2 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -68,7 +68,7 @@ void gic_update(GICState *s)
>          cm = 1 << cpu;
>          s->current_pending[cpu] = 1023;
>          if (!(s->ctlr & (GICD_CTLR_EN_GRP0 | GICD_CTLR_EN_GRP1))
> -            || !s->cpu_enabled[cpu]) {
> +            || !(s->cpu_ctlr[cpu] & (GICC_CTLR_EN_GRP0 | GICC_CTLR_EN_GRP1))) {
>              qemu_irq_lower(s->parent_irq[cpu]);
>              return;
>          }
> @@ -242,6 +242,50 @@ void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
>      }
>  }
>  
> +static uint32_t gic_get_cpu_control(GICState *s, int cpu, MemTxAttrs attrs)
> +{
> +    uint32_t ret = s->cpu_ctlr[cpu];
> +
> +    if (s->security_extn && !attrs.secure) {
> +        /* Construct the NS banked view of GICC_CTLR from the correct
> +         * bits of the S banked view. We don't need to move the bypass
> +         * control bits because we don't implement that (IMPDEF) part
> +         * of the GIC architecture.
> +         */
> +        ret = (ret & (GICC_CTLR_EN_GRP1 | GICC_CTLR_EOIMODE_NS)) >> 1;
> +    }
> +    return ret;
> +}
> +
> +static void gic_set_cpu_control(GICState *s, int cpu, uint32_t value,
> +                                MemTxAttrs attrs)
> +{
> +    uint32_t mask;
> +
> +    if (s->security_extn && !attrs.secure) {
> +        /* The NS view can only write certain bits in the register;
> +         * the rest are unchanged
> +         */
> +        mask = GICC_CTLR_EN_GRP1;
> +        if (s->revision == 2) {
> +            mask |= GICC_CTLR_EOIMODE_NS;
> +        }
> +        s->cpu_ctlr[cpu] &= ~mask;
> +        s->cpu_ctlr[cpu] |= (value << 1) & mask;
> +    } else {
> +        if (s->revision == 2) {
> +            mask = s->security_extn ? GICC_CTLR_V2_S_MASK : GICC_CTLR_V2_MASK;
> +        } else {
> +            mask = s->security_extn ? GICC_CTLR_V1_S_MASK : GICC_CTLR_V1_MASK;
> +        }
> +        s->cpu_ctlr[cpu] = value & mask;
> +    }
> +    DPRINTF("CPU Interface %d: Group0 Interrupts %sabled, "
> +            "Group1 Interrupts %sabled\n", cpu,
> +            (s->cpu_ctlr[cpu] & GICC_CTLR_EN_GRP0) ? "En" : "Dis",
> +            (s->cpu_ctlr[cpu] & GICC_CTLR_EN_GRP1) ? "En" : "Dis");
> +}
> +
>  void gic_complete_irq(GICState *s, int cpu, int irq)
>  {
>      int update = 0;
> @@ -756,7 +800,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>  {
>      switch (offset) {
>      case 0x00: /* Control */
> -        *data = s->cpu_enabled[cpu];
> +        *data = gic_get_cpu_control(s, cpu, attrs);
>          break;
>      case 0x04: /* Priority mask */
>          *data = s->priority_mask[cpu];
> @@ -806,8 +850,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
>  {
>      switch (offset) {
>      case 0x00: /* Control */
> -        s->cpu_enabled[cpu] = (value & 1);
> -        DPRINTF("CPU %d %sabled\n", cpu, s->cpu_enabled[cpu] ? "En" : "Dis");
> +        gic_set_cpu_control(s, cpu, value, attrs);
>          break;
>      case 0x04: /* Priority mask */
>          s->priority_mask[cpu] = (value & 0xff);
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index bef76fc..044ad66 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -59,13 +59,13 @@ static const VMStateDescription vmstate_gic_irq_state = {
>  
>  static const VMStateDescription vmstate_gic = {
>      .name = "arm_gic",
> -    .version_id = 9,
> -    .minimum_version_id = 9,
> +    .version_id = 10,
> +    .minimum_version_id = 10,
>      .pre_save = gic_pre_save,
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(ctlr, GICState),
> -        VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
> +        VMSTATE_UINT32_ARRAY(cpu_ctlr, GICState, GIC_NCPU),
>          VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
>                               vmstate_gic_irq_state, gic_irq_state),
>          VMSTATE_UINT8_ARRAY(irq_target, GICState, GIC_MAXIRQ),
> @@ -134,7 +134,7 @@ static void arm_gic_common_reset(DeviceState *dev)
>          s->current_pending[i] = 1023;
>          s->running_irq[i] = 1023;
>          s->running_priority[i] = 0x100;
> -        s->cpu_enabled[i] = false;
> +        s->cpu_ctlr[i] = 0;
>      }
>      for (i = 0; i < GIC_NR_SGIS; i++) {
>          GIC_SET_ENABLED(i, ALL_CPU_MASK);
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index 3591ca7..c5a2f81 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -414,8 +414,8 @@ static void kvm_arm_gic_put(GICState *s)
>       */
>  
>      for (cpu = 0; cpu < s->num_cpu; cpu++) {
> -        /* s->cpu_enabled[cpu] -> GICC_CTLR */
> -        reg = s->cpu_enabled[cpu];
> +        /* s->cpu_ctlr[cpu] -> GICC_CTLR */
> +        reg = s->cpu_ctlr[cpu];
>          kvm_gicc_access(s, 0x00, cpu, &reg, true);
>  
>          /* s->priority_mask[cpu] -> GICC_PMR */
> @@ -506,9 +506,9 @@ static void kvm_arm_gic_get(GICState *s)
>       */
>  
>      for (cpu = 0; cpu < s->num_cpu; cpu++) {
> -        /* GICC_CTLR -> s->cpu_enabled[cpu] */
> +        /* GICC_CTLR -> s->cpu_ctlr[cpu] */
>          kvm_gicc_access(s, 0x00, cpu, &reg, false);
> -        s->cpu_enabled[cpu] = (reg & 1);
> +        s->cpu_ctlr[cpu] = reg;
>  
>          /* GICC_PMR -> s->priority_mask[cpu] */
>          kvm_gicc_access(s, 0x04, cpu, &reg, false);
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index 4e6456e..c226daf 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -465,7 +465,7 @@ static void armv7m_nvic_reset(DeviceState *dev)
>       * as enabled by default, and with a priority mask which allows
>       * all interrupts through.
>       */
> -    s->gic.cpu_enabled[0] = true;
> +    s->gic.cpu_ctlr[0] = GICC_CTLR_EN_GRP0;
>      s->gic.priority_mask[0] = 0x100;
>      /* The NVIC as a whole is always enabled. */
>      s->gic.ctlr = 1;
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index 3b4b3fb..81c764c 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -57,6 +57,22 @@
>  #define GICD_CTLR_EN_GRP0 (1U << 0)
>  #define GICD_CTLR_EN_GRP1 (1U << 1)
>  
> +#define GICC_CTLR_EN_GRP0    (1U << 0)
> +#define GICC_CTLR_EN_GRP1    (1U << 1)
> +#define GICC_CTLR_ACK_CTL    (1U << 2)
> +#define GICC_CTLR_FIQ_EN     (1U << 3)
> +#define GICC_CTLR_CBPR       (1U << 4) /* GICv1: SBPR */
> +#define GICC_CTLR_EOIMODE    (1U << 9)
> +#define GICC_CTLR_EOIMODE_NS (1U << 10)
> +
> +/* Valid bits for GICC_CTLR for GICv1, v1 with security extensions,
> + * GICv2 and GICv2 with security extensions:
> + */
> +#define GICC_CTLR_V1_MASK    0x1
> +#define GICC_CTLR_V1_S_MASK  0x1f
> +#define GICC_CTLR_V2_MASK    0x21f
> +#define GICC_CTLR_V2_S_MASK  0x61f
> +
>  /* The special cases for the revision property: */
>  #define REV_11MPCORE 0
>  #define REV_NVIC 0xffffffff
> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
> index 261402f..899db3d 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -59,7 +59,10 @@ typedef struct GICState {
>       * of this register is just an alias of bit 1 of the S banked version.
>       */
>      uint32_t ctlr;
> -    bool cpu_enabled[GIC_NCPU];
> +    /* GICC_CTLR; again, the NS banked version is just aliases of bits of
> +     * the S banked register, so our state only needs to store the S version.
> +     */
> +    uint32_t cpu_ctlr[GIC_NCPU];
>  
>      gic_irq_state irq_state[GIC_MAXIRQ];
>      uint8_t irq_target[GIC_MAXIRQ];
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 10/17] hw/intc/arm_gic: Restrict priority view

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:36PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> GICs with Security Extensions restrict the non-secure view of the
> interrupt priority and priority mask registers.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-15-git-send-email-greg.bellows@linaro.org
> [PMM: minor code tweaks; fixed missing masking in gic_set_priority_mask
> and gic_set_priority]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gic.c      | 63 ++++++++++++++++++++++++++++++++++++++++++++++----
>  hw/intc/arm_gic_kvm.c  |  2 +-
>  hw/intc/gic_internal.h |  3 ++-
>  3 files changed, 61 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index e3bbe9e..7c0ddc8 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -233,8 +233,16 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
>      return ret;
>  }
>  
> -void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
> +void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val,
> +                      MemTxAttrs attrs)
>  {
> +    if (s->security_extn && !attrs.secure) {
> +        if (!GIC_TEST_GROUP(irq, (1 << cpu))) {
> +            return; /* Ignore Non-secure access of Group0 IRQ */
> +        }
> +        val = 0x80 | (val >> 1); /* Non-secure view */
> +    }
> +
>      if (irq < GIC_INTERNAL) {
>          s->priority1[irq][cpu] = val;
>      } else {
> @@ -242,6 +250,51 @@ void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
>      }
>  }
>  
> +static uint32_t gic_get_priority(GICState *s, int cpu, int irq,
> +                                 MemTxAttrs attrs)
> +{
> +    uint32_t prio = GIC_GET_PRIORITY(irq, cpu);
> +
> +    if (s->security_extn && !attrs.secure) {
> +        if (!GIC_TEST_GROUP(irq, (1 << cpu))) {
> +            return 0; /* Non-secure access cannot read priority of Group0 IRQ */
> +        }
> +        prio = (prio << 1) & 0xff; /* Non-secure view */
> +    }
> +    return prio;
> +}
> +
> +static void gic_set_priority_mask(GICState *s, int cpu, uint8_t pmask,
> +                                  MemTxAttrs attrs)
> +{
> +    if (s->security_extn && !attrs.secure) {
> +        if (s->priority_mask[cpu] & 0x80) {
> +            /* Priority Mask in upper half */
> +            pmask = 0x80 | (pmask >> 1);
> +        } else {
> +            /* Non-secure write ignored if priority mask is in lower half */
> +            return;
> +        }
> +    }
> +    s->priority_mask[cpu] = pmask;
> +}
> +
> +static uint32_t gic_get_priority_mask(GICState *s, int cpu, MemTxAttrs attrs)
> +{
> +    uint32_t pmask = s->priority_mask[cpu];
> +
> +    if (s->security_extn && !attrs.secure) {
> +        if (pmask & 0x80) {
> +            /* Priority Mask in upper half, return Non-secure view */
> +            pmask = (pmask << 1) & 0xff;
> +        } else {
> +            /* Priority Mask in lower half, RAZ */
> +            pmask = 0;
> +        }
> +    }
> +    return pmask;
> +}
> +
>  static uint32_t gic_get_cpu_control(GICState *s, int cpu, MemTxAttrs attrs)
>  {
>      uint32_t ret = s->cpu_ctlr[cpu];
> @@ -451,7 +504,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
>          irq = (offset - 0x400) + GIC_BASE_IRQ;
>          if (irq >= s->num_irq)
>              goto bad_reg;
> -        res = GIC_GET_PRIORITY(irq, cpu);
> +        res = gic_get_priority(s, cpu, irq, attrs);
>      } else if (offset < 0xc00) {
>          /* Interrupt CPU Target.  */
>          if (s->num_cpu == 1 && s->revision != REV_11MPCORE) {
> @@ -669,7 +722,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>          irq = (offset - 0x400) + GIC_BASE_IRQ;
>          if (irq >= s->num_irq)
>              goto bad_reg;
> -        gic_set_priority(s, cpu, irq, value);
> +        gic_set_priority(s, cpu, irq, value, attrs);
>      } else if (offset < 0xc00) {
>          /* Interrupt CPU Target. RAZ/WI on uniprocessor GICs, with the
>           * annoying exception of the 11MPCore's GIC.
> @@ -820,7 +873,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>          *data = gic_get_cpu_control(s, cpu, attrs);
>          break;
>      case 0x04: /* Priority mask */
> -        *data = s->priority_mask[cpu];
> +        *data = gic_get_priority_mask(s, cpu, attrs);
>          break;
>      case 0x08: /* Binary Point */
>          if (s->security_extn && !attrs.secure) {
> @@ -870,7 +923,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
>          gic_set_cpu_control(s, cpu, value, attrs);
>          break;
>      case 0x04: /* Priority mask */
> -        s->priority_mask[cpu] = (value & 0xff);
> +        gic_set_priority_mask(s, cpu, value, attrs);
>          break;
>      case 0x08: /* Binary Point */
>          if (s->security_extn && !attrs.secure) {
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index c5a2f81..54f18df 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -251,7 +251,7 @@ static void translate_priority(GICState *s, int irq, int cpu,
>      if (to_kernel) {
>          *field = GIC_GET_PRIORITY(irq, cpu) & 0xff;
>      } else {
> -        gic_set_priority(s, cpu, irq, *field & 0xff);
> +        gic_set_priority(s, cpu, irq, *field & 0xff, MEMTXATTRS_UNSPECIFIED);
>      }
>  }
>  
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index 81c764c..119fb81 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -82,7 +82,8 @@ 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);
> -void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
> +void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val,
> +                      MemTxAttrs attrs);
>  
>  static inline bool gic_test_pending(GICState *s, int irq, int cm)
>  {
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 09/17] hw/intc/arm_gic: Implement Non-secure view of RPR

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:35PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> For GICs with Security Extensions Non-secure reads have a restricted
> view on the current running priority.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-11-git-send-email-greg.bellows@linaro.org
> [PMM: make function static, minor comment tweak]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gic.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 4aaaac2..e3bbe9e 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -286,6 +286,23 @@ static void gic_set_cpu_control(GICState *s, int cpu, uint32_t value,
>              (s->cpu_ctlr[cpu] & GICC_CTLR_EN_GRP1) ? "En" : "Dis");
>  }
>  
> +static uint8_t gic_get_running_priority(GICState *s, int cpu, MemTxAttrs attrs)
> +{
> +    if (s->security_extn && !attrs.secure) {
> +        if (s->running_priority[cpu] & 0x80) {
> +            /* Running priority in upper half of range: return the Non-secure
> +             * view of the priority.
> +             */
> +            return s->running_priority[cpu] << 1;
> +        } else {
> +            /* Running priority in lower half of range: RAZ */
> +            return 0;
> +        }
> +    } else {
> +        return s->running_priority[cpu];
> +    }
> +}
> +
>  void gic_complete_irq(GICState *s, int cpu, int irq)
>  {
>      int update = 0;
> @@ -817,7 +834,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>          *data = gic_acknowledge_irq(s, cpu);
>          break;
>      case 0x14: /* Running Priority */
> -        *data = s->running_priority[cpu];
> +        *data = gic_get_running_priority(s, cpu, attrs);
>          break;
>      case 0x18: /* Highest Pending Interrupt */
>          *data = s->current_pending[cpu];
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 11/17] hw/intc/arm_gic: Handle grouping for GICC_HPPIR

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:37PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> Grouping (GICv2) and Security Extensions change the behaviour of reads
> of the highest priority pending interrupt register (ICCHPIR/GICC_HPPIR).
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-12-git-send-email-greg.bellows@linaro.org
> [PMM: make utility fn static; coding style fixes; AckCtl has an effect
>  for GICv2 without security extensions as well; removed checks on enable
>  bits because these are done when we set current_pending[cpu]]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  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 7c0ddc8..75c69b3 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -176,6 +176,32 @@ static void gic_set_irq(void *opaque, int irq, int level)
>      gic_update(s);
>  }
>  
> +static uint16_t gic_get_current_pending_irq(GICState *s, int cpu,
> +                                            MemTxAttrs attrs)
> +{
> +    uint16_t pending_irq = s->current_pending[cpu];
> +
> +    if (pending_irq < GIC_MAXIRQ && gic_has_groups(s)) {
> +        int group = GIC_TEST_GROUP(pending_irq, (1 << cpu));
> +        /* On a GIC without the security extensions, reading this register
> +         * behaves in the same way as a secure access to a GIC with them.
> +         */
> +        bool secure = !s->security_extn || attrs.secure;
> +
> +        if (group == 0 && !secure) {
> +            /* Group0 interrupts hidden from Non-secure access */
> +            return 1023;
> +        }
> +        if (group == 1 && secure && !(s->cpu_ctlr[cpu] & GICC_CTLR_ACK_CTL)) {
> +            /* Group1 interrupts only seen by Secure access if
> +             * AckCtl bit set.
> +             */
> +            return 1022;
> +        }
> +    }
> +    return pending_irq;
> +}
> +
>  static void gic_set_running_irq(GICState *s, int cpu, int irq)
>  {
>      s->running_irq[cpu] = irq;
> @@ -890,7 +916,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>          *data = gic_get_running_priority(s, cpu, attrs);
>          break;
>      case 0x18: /* Highest Pending Interrupt */
> -        *data = s->current_pending[cpu];
> +        *data = gic_get_current_pending_irq(s, cpu, attrs);
>          break;
>      case 0x1c: /* Aliased Binary Point */
>          /* GIC v2, no security: ABPR
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 12/17] hw/intc/arm_gic: Change behavior of EOIR writes

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:38PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> Grouping (GICv2) and Security Extensions change the behavior of EOIR
> writes. Completing Group0 interrupts is only allowed from Secure state.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-13-git-send-email-greg.bellows@linaro.org
> [PMM: Rather than go to great lengths to ignore the UNPREDICTABLE case
>  of a Secure EOI of a Group1 (NS) irq with AckCtl == 0, we just let
>  it fall through; add a comment about it.]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gic.c      | 14 ++++++++++++--
>  hw/intc/armv7m_nvic.c  |  2 +-
>  hw/intc/gic_internal.h |  2 +-
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 75c69b3..4ad80e7 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -382,7 +382,7 @@ static uint8_t gic_get_running_priority(GICState *s, int cpu, MemTxAttrs attrs)
>      }
>  }
>  
> -void gic_complete_irq(GICState *s, int cpu, int irq)
> +void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
>  {
>      int update = 0;
>      int cm = 1 << cpu;
> @@ -412,6 +412,16 @@ void gic_complete_irq(GICState *s, int cpu, int irq)
>          }
>      }
>  
> +    if (s->security_extn && !attrs.secure && !GIC_TEST_GROUP(irq, cm)) {
> +        DPRINTF("Non-secure EOI for Group0 interrupt %d ignored\n", irq);
> +        return;
> +    }
> +
> +    /* Secure EOI with GICC_CTLR.AckCtl == 0 when the IRQ is a Group 1
> +     * interrupt is UNPREDICTABLE. We choose to handle it as if AckCtl == 1,
> +     * 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];
> @@ -959,7 +969,7 @@ static MemTxResult gic_cpu_write(GICState *s, int cpu, int offset,
>          }
>          break;
>      case 0x10: /* End Of Interrupt */
> -        gic_complete_irq(s, cpu, value & 0x3ff);
> +        gic_complete_irq(s, cpu, value & 0x3ff, attrs);
>          return MEMTX_OK;
>      case 0x1c: /* Aliased Binary Point */
>          if (!gic_has_groups(s) || (s->security_extn && !attrs.secure)) {
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index c226daf..dd06ceb 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -135,7 +135,7 @@ void armv7m_nvic_complete_irq(void *opaque, int irq)
>      nvic_state *s = (nvic_state *)opaque;
>      if (irq >= 16)
>          irq += 16;
> -    gic_complete_irq(&s->gic, 0, irq);
> +    gic_complete_irq(&s->gic, 0, irq, MEMTXATTRS_UNSPECIFIED);
>  }
>  
>  static uint32_t nvic_readl(nvic_state *s, uint32_t offset)
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index 119fb81..d3cebef 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -79,7 +79,7 @@
>  
>  void gic_set_pending_private(GICState *s, int cpu, int irq);
>  uint32_t gic_acknowledge_irq(GICState *s, int cpu);
> -void gic_complete_irq(GICState *s, int cpu, int irq);
> +void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs);
>  void gic_update(GICState *s);
>  void gic_init_irqs_and_distributor(GICState *s);
>  void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val,
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 13/17] hw/intc/arm_gic: Change behavior of IAR writes

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:39PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> Grouping (GICv2) and Security Extensions change the behavior of IAR
> reads. Acknowledging Group0 interrupts is only allowed from Secure
> state and acknowledging Group1 interrupts from Secure state is only
> allowed if AckCtl bit is set.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-14-git-send-email-greg.bellows@linaro.org
> [PMM: simplify significantly by reusing the existing
>  gic_get_current_pending_irq() rather than reimplementing the
>  same logic here]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gic.c      | 22 ++++++++++++++++------
>  hw/intc/armv7m_nvic.c  |  2 +-
>  hw/intc/gic_internal.h |  2 +-
>  3 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 4ad80e7..6abdb14 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -213,14 +213,24 @@ static void gic_set_running_irq(GICState *s, int cpu, int irq)
>      gic_update(s);
>  }
>  
> -uint32_t gic_acknowledge_irq(GICState *s, int cpu)
> +uint32_t gic_acknowledge_irq(GICState *s, int cpu, MemTxAttrs attrs)
>  {
>      int ret, irq, src;
>      int cm = 1 << cpu;
> -    irq = s->current_pending[cpu];
> -    if (irq == 1023
> -            || GIC_GET_PRIORITY(irq, cpu) >= s->running_priority[cpu]) {
> -        DPRINTF("ACK no pending IRQ\n");
> +
> +    /* gic_get_current_pending_irq() will return 1022 or 1023 appropriately
> +     * for the case where this GIC supports grouping and the pending interrupt
> +     * is in the wrong group.
> +     */
> +    irq = gic_get_current_pending_irq(s, cpu, attrs);;
> +
> +    if (irq >= GIC_MAXIRQ) {
> +        DPRINTF("ACK, no pending interrupt or it is hidden: %d\n", irq);
> +        return irq;
> +    }
> +
> +    if (GIC_GET_PRIORITY(irq, cpu) >= s->running_priority[cpu]) {
> +        DPRINTF("ACK, pending interrupt (%d) has insufficient priority\n", irq);
>          return 1023;
>      }
>      s->last_active[irq][cpu] = s->running_irq[cpu];
> @@ -920,7 +930,7 @@ static MemTxResult gic_cpu_read(GICState *s, int cpu, int offset,
>          }
>          break;
>      case 0x0c: /* Acknowledge */
> -        *data = gic_acknowledge_irq(s, cpu);
> +        *data = gic_acknowledge_irq(s, cpu, attrs);
>          break;
>      case 0x14: /* Running Priority */
>          *data = gic_get_running_priority(s, cpu, attrs);
> diff --git a/hw/intc/armv7m_nvic.c b/hw/intc/armv7m_nvic.c
> index dd06ceb..49368ca 100644
> --- a/hw/intc/armv7m_nvic.c
> +++ b/hw/intc/armv7m_nvic.c
> @@ -122,7 +122,7 @@ int armv7m_nvic_acknowledge_irq(void *opaque)
>      nvic_state *s = (nvic_state *)opaque;
>      uint32_t irq;
>  
> -    irq = gic_acknowledge_irq(&s->gic, 0);
> +    irq = gic_acknowledge_irq(&s->gic, 0, MEMTXATTRS_UNSPECIFIED);
>      if (irq == 1023)
>          hw_error("Interrupt but no vector\n");
>      if (irq >= 32)
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index d3cebef..20c1e8a 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -78,7 +78,7 @@
>  #define REV_NVIC 0xffffffff
>  
>  void gic_set_pending_private(GICState *s, int cpu, int irq);
> -uint32_t gic_acknowledge_irq(GICState *s, int cpu);
> +uint32_t gic_acknowledge_irq(GICState *s, int cpu, MemTxAttrs attrs);
>  void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs);
>  void gic_update(GICState *s);
>  void gic_init_irqs_and_distributor(GICState *s);
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 14/17] hw/intc/arm_gic: Add grouping support to gic_update()

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:40PM +0100, Peter Maydell wrote:
> Add support to gic_update() for determining the current IRQ
> and FIQ status when interrupt grouping is supported. This
> simply requires that instead of always raising IRQ we
> check the group of the highest priority pending interrupt
> and the GICC_CTLR.FIQEn bit to see whether we should raise
> IRQ or FIQ.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Nice one avoiding clone of gic_update(), thanks.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/intc/arm_gic.c | 27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 6abdb14..c1d2e70 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -60,7 +60,7 @@ void gic_update(GICState *s)
>      int best_irq;
>      int best_prio;
>      int irq;
> -    int level;
> +    int irq_level, fiq_level;
>      int cpu;
>      int cm;
>  
> @@ -70,6 +70,7 @@ void gic_update(GICState *s)
>          if (!(s->ctlr & (GICD_CTLR_EN_GRP0 | GICD_CTLR_EN_GRP1))
>              || !(s->cpu_ctlr[cpu] & (GICC_CTLR_EN_GRP0 | GICC_CTLR_EN_GRP1))) {
>              qemu_irq_lower(s->parent_irq[cpu]);
> +            qemu_irq_lower(s->parent_fiq[cpu]);
>              return;
>          }
>          best_prio = 0x100;
> @@ -83,15 +84,31 @@ void gic_update(GICState *s)
>                  }
>              }
>          }
> -        level = 0;
> +
> +        irq_level = fiq_level = 0;
> +
>          if (best_prio < s->priority_mask[cpu]) {
>              s->current_pending[cpu] = best_irq;
>              if (best_prio < s->running_priority[cpu]) {
> -                DPRINTF("Raised pending IRQ %d (cpu %d)\n", best_irq, cpu);
> -                level = 1;
> +                int group = GIC_TEST_GROUP(best_irq, cm);
> +
> +                if (extract32(s->ctlr, group, 1) &&
> +                    extract32(s->cpu_ctlr[cpu], group, 1)) {
> +                    if (group == 0 && s->cpu_ctlr[cpu] & GICC_CTLR_FIQ_EN) {
> +                        DPRINTF("Raised pending FIQ %d (cpu %d)\n",
> +                                best_irq, cpu);
> +                        fiq_level = 1;
> +                    } else {
> +                        DPRINTF("Raised pending IRQ %d (cpu %d)\n",
> +                                best_irq, cpu);
> +                        irq_level = 1;
> +                    }
> +                }
>              }
>          }
> -        qemu_set_irq(s->parent_irq[cpu], level);
> +
> +        qemu_set_irq(s->parent_irq[cpu], irq_level);
> +        qemu_set_irq(s->parent_fiq[cpu], fiq_level);
>      }
>  }
>  
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 15/17] hw/arm/virt.c: Wire FIQ between CPU <> GIC

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:41PM +0100, Peter Maydell wrote:
> From: Greg Bellows <greg.bellows@linaro.org>
> 
> Connect FIQ output of the GIC CPU interfaces to the CPUs.
> 
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-4-git-send-email-greg.bellows@linaro.org
> [PMM: minor format tweak]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/arm/virt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 565f573..a7f9a10 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -386,6 +386,8 @@ static uint32_t create_gic(const VirtBoardInfo *vbi, qemu_irq *pic)
>                                qdev_get_gpio_in(gicdev, ppibase + 27));
>  
>          sysbus_connect_irq(gicbusdev, i, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
> +        sysbus_connect_irq(gicbusdev, i + smp_cpus,
> +                           qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
>      }
>  
>      for (i = 0; i < NUM_IRQS; i++) {
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 16/17] hw/arm/vexpress.c: Wire FIQ between CPU <> GIC

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:42PM +0100, Peter Maydell wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> Connect FIQ output of the GIC CPU interfaces to the CPUs.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> Message-id: 1429113742-8371-3-git-send-email-greg.bellows@linaro.org
> [PMM: minor format tweak]
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> ---
>  hw/arm/vexpress.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 3989bc5..8f1a5ea 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -253,6 +253,8 @@ static void init_cpus(const char *cpu_model, const char *privdev,
>          DeviceState *cpudev = DEVICE(qemu_get_cpu(n));
>  
>          sysbus_connect_irq(busdev, n, qdev_get_gpio_in(cpudev, ARM_CPU_IRQ));
> +        sysbus_connect_irq(busdev, n + smp_cpus,
> +                           qdev_get_gpio_in(cpudev, ARM_CPU_FIQ));
>      }
>  }
>  
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 00/17] arm_gic: Add security and grouping support

[Edgar E. Iglesias]

On Fri, May 01, 2015 at 06:50:26PM +0100, Peter Maydell wrote:
> This patch series adds support for GICv1 and GICv2 security
> extensions, as well as support for GIC interrupt grouping on GICv2.

A question. Once we enable the the security extensions on the GICs,
do you have any suggestions on howto best handle direct boots into
NS EL2/1 (Linux)?

The GIC resets to all interrupts configured for Group0 and Linux running
in NS mode cannot change that so we need some kind of boot-loader
code or magic to do what firmware would have been expected to do
at boot time (switch some irqs to NS).

Cheers,
Edgar


> 
> This is based on the work originally by Fabian and then by Greg.
> I've gone through and dealt with all the issues I raised in code
> review, and a few others I noticed as I was working on it.
> The general structure of the changes is still the same, though
> I've reordered one or two of the patches; I've touched most of
> the lines of code in the series, though, as well as deleting
> quite a few (the patches now add ~375 lines of code rather than
> over 475).
> 
> I think these patches are in a suitable state to apply (and
> they have no dependencies that aren't in master), assuming no
> further issues found in review.
> 
> With this patchset the security extensions are still disabled
> on all boards, so the actual functional change is that GICv2
> now correctly implements interrupt grouping. This is enabled
> always for GICv2, because the programming model is fully backwards
> compatible with treating the GIC like one which doesn't support
> groups (which is what Linux does).
> 
> The next part of this work is going to be actually enabling
> the security extensions. Here's a sketch of my plan for that:
>  * the a15mpcore and a9mpcore wrapper objects will default to
>    enabling the security extensions in the GIC they create
>    (unless the GIC is the KVM one). They also provide a
>    QOM property to override this
>  * for the set of legacy boards which are currently disabling
>    has_el3 on their CPUs, we also have them disable TZ in the GIC
>    (a non-TZ CPU and a TZ GIC is a bad combo because the CPU
>    has no way to put the interrupts into Group1 where it can
>    use them, so the whole system is busted)
>  * the virt board creates its GIC directly, so it should also
>    set the has-security-extensions property as needed
>  * if boot.c is starting the CPUs directly in NonSecure
>    mode (because we're booting a kernel directly rather than
>    starting firmware, and arm_boot_info::secure_boot is false)
>    then it must also manually configure the GIC distributor
>    to put all interrupts into Group1. This is boot.c having
>    to do a firmware configuration job since it's effectively
>    acting as lightweight builtin firmware.
> 
> I think we could reasonably review and commit this patchseries
> without waiting for that bit of board-wiring work; let me know
> if you disagree.
> 
> 
> Major changes since v3:
>  * renamed property to 'has-security-extensions', to be a bit
>    more in line with the CPU's 'has_el3'. I'm not wedded to this
>    name so if anybody wants to suggest something better (or
>    tell me our convention for prop names is underscores!) feel free
>  * error on realize if security extensions turned on for a GIC
>    which doesn't support them
>  * new patch: switch to read/write_with_attrs MMIO callbacks so
>    we can get at the Secure/NonSecure tx attribute
>  * make the GIC_*_GROUP macros work like the others, with a simple
>    SET/CLEAR/TEST semantic
>  * new patch: save and restore GICD_IGROUPRn state when using KVM
>    now we have the state struct fields to keep it in [the kernel
>    doesn't implement grouping, but if it ever does we will be ready]
>  * rather than having a 2-element array for storing the S and NS
>    banked versions of GICD_CTLR and GICC_CTLR, just store the S
>    version, since in both cases the NS view is just an alias of
>    a subset of bits from the S register. This allows us to nicely
>    simplify a lot of the logic that deals with these registers.
>  * fixed bug in handling of GICC_BPR for GICv2-without-TZ
>  * added missing masks in gic_set_priority_mask() and gic_set_priority()
>  * make AckCtl operate on GICv2-without-TZ
>  * handle an UNPREDICTABLE case (Secure EOI of a Group1 irq
>    with AckCtl == 0) in a way more convenient for the implementation
>  * reuse gic_get_current_pending_irq() in implementation of IAR writes,
>    rather than reimplementing equivalent logic
>  * new patch: support grouping in a single gic_update function (rather
>    than having split update functions for the two cases)
>  * new patch: wire FIQ up on highbank/midway; this means we're now
>    consistent in having FIQ wired up on all our boards with GICv2
>  * lots of minor formatting tweaks, etc; see individual commit messages
> 
> 
> Fabian Aggeler (12):
>   hw/intc/arm_gic: Create outbound FIQ lines
>   hw/intc/arm_gic: Add Security Extensions property
>   hw/intc/arm_gic: Add Interrupt Group Registers
>   hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked
>   hw/intc/arm_gic: Make ICCBPR/GICC_BPR banked
>   hw/intc/arm_gic: Make ICCICR/GICC_CTLR banked
>   hw/intc/arm_gic: Implement Non-secure view of RPR
>   hw/intc/arm_gic: Restrict priority view
>   hw/intc/arm_gic: Handle grouping for GICC_HPPIR
>   hw/intc/arm_gic: Change behavior of EOIR writes
>   hw/intc/arm_gic: Change behavior of IAR writes
>   hw/arm/vexpress.c: Wire FIQ between CPU <> GIC
> 
> Greg Bellows (1):
>   hw/arm/virt.c: Wire FIQ between CPU <> GIC
> 
> Peter Maydell (4):
>   hw/intc/arm_gic: Switch to read/write callbacks with tx attributes
>   hw/intc/arm_gic_kvm.c: Save and restore GICD_IGROUPRn state
>   hw/intc/arm_gic: Add grouping support to gic_update()
>   hw/arm/highbank.c: Wire FIQ between CPU <> GIC
> 
>  hw/arm/highbank.c                |   3 +
>  hw/arm/vexpress.c                |   2 +
>  hw/arm/virt.c                    |   2 +
>  hw/intc/arm_gic.c                | 469 ++++++++++++++++++++++++++++++++-------
>  hw/intc/arm_gic_common.c         |  22 +-
>  hw/intc/arm_gic_kvm.c            |  51 +++--
>  hw/intc/armv7m_nvic.c            |   8 +-
>  hw/intc/gic_internal.h           |  29 ++-
>  include/hw/intc/arm_gic_common.h |  24 +-
>  9 files changed, 492 insertions(+), 118 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH v4 00/17] arm_gic: Add security and grouping support

[Peter Maydell]

On 5 May 2015 at 03:08, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> On Fri, May 01, 2015 at 06:50:26PM +0100, Peter Maydell wrote:
>> This patch series adds support for GICv1 and GICv2 security
>> extensions, as well as support for GIC interrupt grouping on GICv2.
>
> A question. Once we enable the the security extensions on the GICs,
> do you have any suggestions on howto best handle direct boots into
> NS EL2/1 (Linux)?
>
> The GIC resets to all interrupts configured for Group0 and Linux running
> in NS mode cannot change that so we need some kind of boot-loader
> code or magic to do what firmware would have been expected to do
> at boot time (switch some irqs to NS).

This is what I had in mind with the bit about:

>>  * if boot.c is starting the CPUs directly in NonSecure
>>    mode (because we're booting a kernel directly rather than
>>    starting firmware, and arm_boot_info::secure_boot is false)
>>    then it must also manually configure the GIC distributor
>>    to put all interrupts into Group1. This is boot.c having
>>    to do a firmware configuration job since it's effectively
>>    acting as lightweight builtin firmware.

I hadn't made up my mind whether this was easier to do via
boot.c writing a bunch of values to GICD registers or by
having the GIC provide a function/method to call to do the job.

-- PMM