[Qemu-devel] [PATCH v3 00/16] target-arm: Add GICv1/SecExt and GICv2/Grouping

[Qemu-devel] [PATCH v3 00/16] target-arm: Add GICv1/SecExt and GICv2/Grouping

[Greg Bellows]

This patch series adds ARM GICv1 and GICv2 security extension support.  As a
result GIC interrupt grouping and FIQ enablement have also been added.  FIQ
enablement is limited to ARM the ARM vexpress and virt machines.

At the current moment, the security extension capability is not enabled as it
depends on ARM secure address space support for proper operation.  Instead,
secure checks are hardwired as non-secure.

v2 -> v3
- Add missing return in gic_dist_readb()
- Fix typos in patch 7
- Add sign off to patches
- Rebased to current upstream

v1 -> v2
- Fixed GIC_SET macro logic for group 0 and 1
- Fixed gic_update to use correct GIC_CTLR bit for group 1
- Reworked gic_set/get_cpu_control to better handle non-security extension case
  for GICv1.
- Fixed various BPR read/write issues.
- Fixed EOIR ackctl issue.
- Fixed issue with gic_acknowledge not properly checking secure state.
- Fixed gic_update use of incorrect bit to check cpu_control group 1
  enablement.
- Added clarifying comments
- Various fixes based on initial version review comments (see individual
  patches for details).


Fabian Aggeler (15):
  hw/intc/arm_gic: Request FIQ sources
  hw/arm/vexpress.c: Wire FIQ between CPU <> GIC
  hw/intc/arm_gic: Add Security Extensions property
  hw/intc/arm_gic: Add ns_access() function
  hw/intc/arm_gic: Add Interrupt Group Registers
  hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked
  hw/intc/arm_gic: Make ICCICR/GICC_CTLR banked
  hw/intc/arm_gic: Make ICCBPR/GICC_BPR banked
  hw/intc/arm_gic: Implement Non-secure view of RPR
  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/intc/arm_gic: Restrict priority view
  hw/intc/arm_gic: Break out gic_update() function
  hw/intc/arm_gic: add gic_update() for grouping

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

 hw/arm/vexpress.c                |   2 +
 hw/arm/virt.c                    |   2 +
 hw/intc/arm_gic.c                | 498 ++++++++++++++++++++++++++++++++++++---
 hw/intc/arm_gic_common.c         |   9 +-
 hw/intc/arm_gic_kvm.c            |   8 +-
 hw/intc/armv7m_nvic.c            |   2 +-
 hw/intc/gic_internal.h           |  25 ++
 include/hw/intc/arm_gic_common.h |  23 +-
 8 files changed, 527 insertions(+), 42 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH v3 01/16] hw/intc/arm_gic: Request FIQ sources

[Greg Bellows]

From: Fabian Aggeler <aggelerf@ethz.ch>

Preparing for FIQ lines from GIC to CPUs, which is needed for GIC
Security Extensions.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 hw/intc/arm_gic.c                | 3 +++
 include/hw/intc/arm_gic_common.h | 1 +
 2 files changed, 4 insertions(+)

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/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.8.3.2

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

[Greg Bellows]

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>
---
 hw/arm/vexpress.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 3989bc5..c2602a2 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.8.3.2

[Qemu-devel] [PATCH v3 03/16] hw/arm/virt.c: Wire FIQ between CPU <> GIC

[Greg Bellows]

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

Signed-off-by: Greg Bellows <greg.bellows@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..f3326cf 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.8.3.2

[Qemu-devel] [PATCH v3 04/16] hw/intc/arm_gic: Add Security Extensions property

[Greg Bellows]

From: Fabian Aggeler <aggelerf@ethz.ch>

The existing implementation does not support Security Extensions mentioned
in the GICv1 and GICv2 architecture specification. Security Extensions are
not available on all GICs. This property makes it possible to enable Security Extensions.

It also makes GICD_TYPER/ICDICTR.SecurityExtn RAO for GICs which implement
Security Extensions.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>

---

v1 -> v2
- Change GICState security extension property from a uint8 type to bool
---
 hw/intc/arm_gic.c                | 5 ++++-
 hw/intc/arm_gic_common.c         | 1 +
 include/hw/intc/arm_gic_common.h | 1 +
 3 files changed, 6 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..e35049d 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -149,6 +149,7 @@ static Property arm_gic_common_properties[] = {
      * (Internally, 0xffffffff also indicates "not a GIC but an NVIC".)
      */
     DEFINE_PROP_UINT32("revision", GICState, revision, 1),
+    DEFINE_PROP_BOOL("security-extn", GICState, security_extn, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
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.8.3.2

[Qemu-devel] [PATCH v3 05/16] hw/intc/arm_gic: Add ns_access() function

[Greg Bellows]

From: Fabian Aggeler <aggelerf@ethz.ch>

Security Extensions for GICv1 and GICv2 use register banking
to provide transparent access to seperate Secure and Non-secure
copies of GIC configuration registers. This function will later
be replaced by code determining the security state of a read/write
access to a register.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 hw/intc/arm_gic.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index cdf7408..e0bce6e 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -45,6 +45,13 @@ static inline int gic_get_current_cpu(GICState *s)
     return 0;
 }
 
+/* Security state of a read / write access */
+static inline bool ns_access(void)
+{
+    /* TODO: use actual security state */
+    return true;
+}
+
 /* 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)
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 06/16] hw/intc/arm_gic: Add Interrupt Group Registers

[Greg Bellows]

From: Fabian Aggeler <aggelerf@ethz.ch>

Interrupt Group Registers (previously called Interrupt Security
Registers) as defined in GICv1 with Security Extensions or GICv2 allow
to configure interrupts as Secure (Group0) or Non-secure (Group1).
In GICv2 these registers are implemented independent of the existence of
Security Extensions.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>

---

v1 -> v2
- Add clarifying comments to gic_dist_readb/writeb on interrupt group register
  update
- Swap GIC_SET_GROUP0/1 macro logic.  Setting the irq_state.group field for
  group 0 should clear the bit not set it.  Similarly, setting the field for
  group 1 should set the bit not clear it.
---
 hw/intc/arm_gic.c                | 49 +++++++++++++++++++++++++++++++++++++---
 hw/intc/arm_gic_common.c         |  1 +
 hw/intc/gic_internal.h           |  4 ++++
 include/hw/intc/arm_gic_common.h |  1 +
 4 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index e0bce6e..aa4402e 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -312,8 +312,27 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
         if (offset < 0x08)
             return 0;
         if (offset >= 0x80) {
-            /* Interrupt Security , RAZ/WI */
-            return 0;
+            /* Interrupt Group Registers
+             *
+             * For GIC with Security Extn and Non-secure access RAZ/WI
+             * For GICv1 without Security Extn RAZ/WI
+             */
+            res = 0;
+            if (!(s->security_extn && ns_access()) &&
+                    ((s->revision == 1 && s->security_extn)
+                            || s->revision == 2)) {
+                /* 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_GROUP0(irq + i, cm)) {
+                        res |= (1 << i);
+                    }
+                }
+            }
+            return res;
         }
         goto bad_reg;
     } else if (offset < 0x200) {
@@ -457,7 +476,31 @@ 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
+             *
+             * For GIC with Security Extn and Non-secure access RAZ/WI
+             * For GICv1 without Security Extn RAZ/WI
+             */
+            if (!(s->security_extn && ns_access()) &&
+                    ((s->revision == 1 && s->security_extn)
+                            || s->revision == 2)) {
+                /* 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++) {
+                    /* Group bits are banked for private interrupts (internal)*/
+                    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
+                    if (value & (1 << i)) {
+                        /* Group1 (Non-secure) */
+                        GIC_SET_GROUP1(irq + i, cm);
+                    } else {
+                        /* Group0 (Secure) */
+                        GIC_SET_GROUP0(irq + i, cm);
+                    }
+                }
+            }
         } else {
             goto bad_reg;
         }
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index e35049d..28f3b2a 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -52,6 +52,7 @@ 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()
     }
 };
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index e87ef36..f01955a 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_SET_GROUP0(irq, cm) (s->irq_state[irq].group &= ~(cm))
+#define GIC_SET_GROUP1(irq, cm) (s->irq_state[irq].group |= (cm))
+#define GIC_TEST_GROUP0(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.8.3.2

[Qemu-devel] [PATCH v3 07/16] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked

[Greg Bellows]

From: Fabian Aggeler <aggelerf@ethz.ch>

ICDDCR/GICD_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
Distributor to the CPU interfaces for Group0 and Group1.

EnableGroup0 (Bit [1]) in GICv1 is IMPDEF. Since this bit (Enable
Non-secure) is present in the integrated IC of the Cortex-A9 MPCore,
which implements the GICv1 profile, we support this bit in GICv1 too.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>

---
v2 -> v3
- Added missing return in gic_dist_readb()
- Fixed typos

v1 -> v2
- Fix gic_dist_writeb() update of GICD_CTRL to only use bit[0] of the
  EnableGrp1 field not bit[1].
- Add clarifying comments
---
 hw/intc/arm_gic.c                | 50 ++++++++++++++++++++++++++++++++++++----
 hw/intc/arm_gic_common.c         |  2 +-
 include/hw/intc/arm_gic_common.h |  7 +++++-
 3 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index aa4402e..b9dfde3 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -302,8 +302,26 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
     cpu = gic_get_current_cpu(s);
     cm = 1 << cpu;
     if (offset < 0x100) {
-        if (offset == 0)
-            return s->enabled;
+        if (offset == 0) {      /* GICD_CTLR */
+            res = 0;
+            if ((s->revision == 2 && !s->security_extn)
+                    || (s->security_extn && !ns_access())) {
+                /* In this case the GICD_CTLR contains both a group0 and group1
+                 * enable bit, so we create the resuling value by aggregating
+                 * the bits from the two enable values.
+                 * The group0 enable bit is only visible to secure accesses.
+                 * The group1 enable bit (bit[1]) is an alias of bit[0] in
+                 * the non-secure copy (enabled_grp[1]).
+                 */
+                res = (s->enabled_grp[1] << 1) | s->enabled_grp[0];
+            } else if (s->security_extn && ns_access()) {
+                res = s->enabled_grp[1];
+            } else {
+                /* Neither GICv2 nor Security Extensions present */
+                res = s->enabled;
+            }
+            return res;
+        }
         if (offset == 4)
             /* Interrupt Controller Type Register */
             return ((s->num_irq / 32) - 1)
@@ -471,8 +489,32 @@ 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->revision == 2 && !s->security_extn)
+                    || (s->security_extn && !ns_access())) {
+                s->enabled_grp[0] = value & (1U << 0); /* EnableGrp0 */
+                /* For a GICv1 with Security Extn "EnableGrp1" is IMPDEF. */
+                /* We only use the first bit of the enabled_grp vars to
+                 * indicate enabled or disabled.  In this case we have to shift
+                 * the incoming value down to the low bit because the group1
+                 * enabled bit is bit[1] in the secure/GICv2 GICD_CTLR.
+                 */
+                s->enabled_grp[1] = (value >> 1) & 0x1; /* EnableGrp1 */
+                DPRINTF("Group0 distribution %sabled\n"
+                        "Group1 distribution %sabled\n",
+                                        s->enabled_grp[0] ? "En" : "Dis",
+                                        s->enabled_grp[1] ? "En" : "Dis");
+            } else if (s->security_extn && ns_access()) {
+                /* If we are non-secure only the group1 enable bit is visible
+                 * as bit[0] in the GICD_CTLR.
+                 */
+                s->enabled_grp[1] = (value & 0x1);
+                DPRINTF("Group1 distribution %sabled\n",
+                        s->enabled_grp[1] ? "En" : "Dis");
+            } else {
+                /* Neither GICv2 nor Security Extensions present */
+                s->enabled = (value & 0x1);
+                DPRINTF("Distribution %sabled\n", s->enabled ? "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 28f3b2a..c44050d 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -64,7 +64,7 @@ static const VMStateDescription vmstate_gic = {
     .pre_save = gic_pre_save,
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_BOOL(enabled, GICState),
+        VMSTATE_UINT8_ARRAY(enabled_grp, GICState, GIC_NR_GROUP),
         VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
         VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
                              vmstate_gic_irq_state, gic_irq_state),
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index b78981e..16e193d 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -30,6 +30,8 @@
 #define GIC_NR_SGIS 16
 /* Maximum number of possible CPU interfaces, determined by GIC architecture */
 #define GIC_NCPU 8
+/* Number of Groups (Group0 [Secure], Group1 [Non-secure]) */
+#define GIC_NR_GROUP 2
 
 #define MAX_NR_GROUP_PRIO 128
 #define GIC_NR_APRS (MAX_NR_GROUP_PRIO / 32)
@@ -52,7 +54,10 @@ typedef struct GICState {
 
     qemu_irq parent_irq[GIC_NCPU];
     qemu_irq parent_fiq[GIC_NCPU];
-    bool enabled;
+    union {
+        uint8_t enabled;
+        uint8_t enabled_grp[GIC_NR_GROUP]; /* EnableGrp0 and EnableGrp1 */
+    };
     bool cpu_enabled[GIC_NCPU];
 
     gic_irq_state irq_state[GIC_MAXIRQ];
-- 
1.8.3.2

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

[Greg Bellows]

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

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>

---

v1 -> v2
- Rework gic_set_cpu_control() and gic_get_cpu_control() to close gap on
  handling GICv1 wihtout security extensions.
- Fix use of incorrect control index in update.
---
 hw/intc/arm_gic.c                | 82 +++++++++++++++++++++++++++++++++++++---
 hw/intc/arm_gic_common.c         |  5 ++-
 hw/intc/arm_gic_kvm.c            |  8 ++--
 hw/intc/armv7m_nvic.c            |  2 +-
 hw/intc/gic_internal.h           | 14 +++++++
 include/hw/intc/arm_gic_common.h |  2 +-
 6 files changed, 100 insertions(+), 13 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index b9dfde3..b402e00 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -66,7 +66,7 @@ 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->enabled || !(s->cpu_control[cpu][1] & 1)) {
             qemu_irq_lower(s->parent_irq[cpu]);
             return;
         }
@@ -240,6 +240,80 @@ void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
     }
 }
 
+uint32_t gic_get_cpu_control(GICState *s, int cpu)
+{
+    uint32_t ret;
+
+    if (!s->security_extn) {
+        if (s->revision == 1) {
+            ret = s->cpu_control[cpu][1];
+            ret &= 0x1;     /* Mask of reserved bits */
+        } else {
+            ret = s->cpu_control[cpu][0];
+            ret &= GICC_CTLR_S_MASK;   /* Mask of reserved bits */
+        }
+    } else {
+        if (ns_access()) {
+            ret = s->cpu_control[cpu][1];
+            ret &= GICC_CTLR_NS_MASK;   /* Mask of reserved bits */
+            if (s->revision == 1) {
+                ret &= 0x1; /* Mask of reserved bits */
+            }
+        } else {
+            ret = s->cpu_control[cpu][0];
+            ret &= GICC_CTLR_S_MASK;   /* Mask of reserved bits */
+        }
+    }
+
+    return ret;
+}
+
+void gic_set_cpu_control(GICState *s, int cpu, uint32_t value)
+{
+    if (!s->security_extn) {
+        if (s->revision == 1) {
+            s->cpu_control[cpu][1] = value & 0x1;
+            DPRINTF("CPU Interface %d %sabled\n", cpu,
+                s->cpu_control[cpu][1] ? "En" : "Dis");
+        } else {
+            /* Write to Secure instance of the register */
+            s->cpu_control[cpu][0] = value & GICC_CTLR_S_MASK;
+            /* Synchronize EnableGrp1 alias of Non-secure copy */
+            s->cpu_control[cpu][1] &= ~GICC_CTLR_NS_EN_GRP1;
+            s->cpu_control[cpu][1] |=
+                (value & GICC_CTLR_S_EN_GRP1) ? GICC_CTLR_NS_EN_GRP1 : 0;
+            DPRINTF("CPU Interface %d: Group0 Interrupts %sabled, "
+                "Group1 Interrupts %sabled\n", cpu,
+                (s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0) ? "En" : "Dis",
+                (s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP1) ? "En" : "Dis");
+        }
+    } else {
+        if (ns_access()) {
+            if (s->revision == 1) {
+                s->cpu_control[cpu][1] = value & 0x1;
+                DPRINTF("CPU Interface %d %sabled\n", cpu,
+                    s->cpu_control[cpu][1] ? "En" : "Dis");
+            } else {
+                /* Write to Non-secure instance of the register */
+                s->cpu_control[cpu][1] = value & GICC_CTLR_NS_MASK;
+                /* Synchronize EnableGrp1 alias of Secure copy */
+                s->cpu_control[cpu][0] &= ~GICC_CTLR_S_EN_GRP1;
+                s->cpu_control[cpu][0] |=
+                    (value & GICC_CTLR_NS_EN_GRP1) ? GICC_CTLR_S_EN_GRP1 : 0;
+            }
+            DPRINTF("CPU Interface %d: Group1 Interrupts %sabled\n", cpu,
+                (s->cpu_control[cpu][1] & GICC_CTLR_NS_EN_GRP1) ? "En" : "Dis");
+        } else {
+            /* Write to Secure instance of the register */
+            s->cpu_control[cpu][0] = value & GICC_CTLR_S_MASK;
+            /* Synchronize EnableGrp1 alias of Non-secure copy */
+            s->cpu_control[cpu][1] &= ~GICC_CTLR_NS_EN_GRP1;
+            s->cpu_control[cpu][1] |=
+                (value & GICC_CTLR_S_EN_GRP1) ? GICC_CTLR_NS_EN_GRP1 : 0;
+        }
+    }
+}
+
 void gic_complete_irq(GICState *s, int cpu, int irq)
 {
     int update = 0;
@@ -763,7 +837,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
 {
     switch (offset) {
     case 0x00: /* Control */
-        return s->cpu_enabled[cpu];
+        return gic_get_cpu_control(s, cpu);
     case 0x04: /* Priority mask */
         return s->priority_mask[cpu];
     case 0x08: /* Binary Point */
@@ -789,9 +863,7 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
 {
     switch (offset) {
     case 0x00: /* Control */
-        s->cpu_enabled[cpu] = (value & 1);
-        DPRINTF("CPU %d %sabled\n", cpu, s->cpu_enabled[cpu] ? "En" : "Dis");
-        break;
+        return gic_set_cpu_control(s, cpu, value);
     case 0x04: /* Priority mask */
         s->priority_mask[cpu] = (value & 0xff);
         break;
diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index c44050d..e225f2b 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -65,7 +65,7 @@ static const VMStateDescription vmstate_gic = {
     .post_load = gic_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8_ARRAY(enabled_grp, GICState, GIC_NR_GROUP),
-        VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
+        VMSTATE_UINT32_2DARRAY(cpu_control, GICState, GIC_NCPU, GIC_NR_GROUP),
         VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
                              vmstate_gic_irq_state, gic_irq_state),
         VMSTATE_UINT8_ARRAY(irq_target, GICState, GIC_MAXIRQ),
@@ -127,7 +127,8 @@ 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_control[i][0] = false;
+        s->cpu_control[i][1] = false;
     }
     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 e1952ad..e0a9c01 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -397,8 +397,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_enabled[cpu][0] -> GICC_CTLR */
+        reg = s->cpu_control[cpu];
         kvm_gicc_access(s, 0x00, cpu, &reg, true);
 
         /* s->priority_mask[cpu] -> GICC_PMR */
@@ -496,9 +496,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_control[cpu][0] */
         kvm_gicc_access(s, 0x00, cpu, &reg, false);
-        s->cpu_enabled[cpu] = (reg & 1);
+        s->cpu_control[cpu][0] = 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 6ff6c7f..97fb6be 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_control[0][0] = true;
     s->gic.priority_mask[0] = 0x100;
     /* The NVIC as a whole is always enabled. */
     s->gic.enabled = true;
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index f01955a..e360de6 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -54,6 +54,17 @@
 #define GIC_SET_GROUP1(irq, cm) (s->irq_state[irq].group |= (cm))
 #define GIC_TEST_GROUP0(irq, cm) ((s->irq_state[irq].group & (cm)) == 0)
 
+#define GICC_CTLR_S_EN_GRP0   (1U << 0)
+#define GICC_CTLR_S_EN_GRP1   (1U << 1)
+#define GICC_CTLR_S_ACK_CTL       (1U << 2)
+#define GICC_CTLR_S_FIQ_EN        (1U << 3)
+#define GICC_CTLR_S_CBPR          (1U << 4) /* GICv1: SBPR */
+
+#define GICC_CTLR_S_MASK          0x7ff
+
+#define GICC_CTLR_NS_EN_GRP1  (1U << 0)
+#define GICC_CTLR_NS_MASK         (1 | 3 << 5 | 1 << 9)
+
 
 /* The special cases for the revision property: */
 #define REV_11MPCORE 0
@@ -65,6 +76,9 @@ 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);
+uint32_t gic_get_cpu_control(GICState *s, int cpu);
+void gic_set_cpu_control(GICState *s, int cpu, uint32_t value);
+
 
 static inline bool gic_test_pending(GICState *s, int irq, int cm)
 {
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 16e193d..1daa672 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -58,7 +58,7 @@ typedef struct GICState {
         uint8_t enabled;
         uint8_t enabled_grp[GIC_NR_GROUP]; /* EnableGrp0 and EnableGrp1 */
     };
-    bool cpu_enabled[GIC_NCPU];
+    uint32_t cpu_control[GIC_NCPU][GIC_NR_GROUP];
 
     gic_irq_state irq_state[GIC_MAXIRQ];
     uint8_t irq_target[GIC_MAXIRQ];
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 09/16] hw/intc/arm_gic: Make ICCBPR/GICC_BPR banked

[Greg Bellows]

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>

---

v1 -> v2
- Fix ABPR read handling when security extensions are not present
- Fix BPR write to take into consideration the minimum value written to ABPR
  and restrict BPR->ABPR mirroring to GICv2 and up.
- Fix ABPR write to take into consideration the minumum value written
- Fix ABPR write condition break-down to include mirroring of ABPR writes to
  BPR.
---
 hw/intc/arm_gic.c                | 54 ++++++++++++++++++++++++++++++++++++----
 include/hw/intc/arm_gic_common.h | 11 +++++---
 2 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index b402e00..b62cde2 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -841,7 +841,12 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
     case 0x04: /* Priority mask */
         return s->priority_mask[cpu];
     case 0x08: /* Binary Point */
-        return s->bpr[cpu];
+        if (s->security_extn && ns_access()) {
+            /* BPR is banked. Non-secure copy stored in ABPR. */
+            return s->abpr[cpu];
+        } else {
+            return s->bpr[cpu];
+        }
     case 0x0c: /* Acknowledge */
         return gic_acknowledge_irq(s, cpu);
     case 0x14: /* Running Priority */
@@ -849,7 +854,14 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
     case 0x18: /* Highest Pending Interrupt */
         return s->current_pending[cpu];
     case 0x1c: /* Aliased Binary Point */
-        return s->abpr[cpu];
+        if (!s->security_extn || (s->security_extn && ns_access())) {
+            /* If Security Extensions are present ABPR is a secure register,
+             * only accessible from secure state.
+             */
+            return 0;
+        } else {
+            return s->abpr[cpu];
+        }
     case 0xd0: case 0xd4: case 0xd8: case 0xdc:
         return s->apr[(offset - 0xd0) / 4][cpu];
     default:
@@ -868,14 +880,46 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
         s->priority_mask[cpu] = (value & 0xff);
         break;
     case 0x08: /* Binary Point */
-        s->bpr[cpu] = (value & 0x7);
+        if (s->security_extn && ns_access()) {
+            /* BPR is banked. Non-secure copy stored in ABPR. */
+            /* The non-secure (ABPR) must not be below an implementation
+             * defined minimum value between 1-4.
+             * NOTE: BPR_MIN is currently set to 0, which is always true given
+             *       the value is unsigned, so no check is necessary.
+             */
+            s->abpr[cpu] = (GIC_MIN_ABPR <= (value & 0x7))
+                                ? (value & 0x7) : GIC_MIN_ABPR;
+        } else {
+            s->bpr[cpu] = (value & 0x7);
+            if (s->revision >= 2) {
+                /* On GICv2 without sec ext, GICC_ABPR is an alias of GICC_BPR
+                 * so mirror the write.
+                 */
+                 s->abpr[cpu] = s->bpr[cpu];
+            }
+        }
         break;
     case 0x10: /* End Of Interrupt */
         gic_complete_irq(s, cpu, value & 0x3ff);
         return;
     case 0x1c: /* Aliased Binary Point */
-        if (s->revision >= 2) {
-            s->abpr[cpu] = (value & 0x7);
+        /* This register only exists on GICv2 or GICv1 w/security.  Writes when
+         * the register is not implemented (no sec ext) are ignored.
+         */
+        if (s->security_extn) {
+            if (!ns_access()) {
+                s->abpr[cpu] = (GIC_MIN_ABPR <= (value & 0x7))
+                                ? (value & 0x7) : GIC_MIN_ABPR;
+            }
+        } else {
+            if (s->revision >= 2) {
+                /* In a GICv2 impl without the security extension, the
+                 * GICC_ABPR is an alias to GICC_BPR, so mirror the write.
+                 */
+                s->abpr[cpu] = (GIC_MIN_ABPR <= (value & 0x7))
+                                ? (value & 0x7) : GIC_MIN_ABPR;
+                s->bpr[cpu] = s->abpr[cpu];
+            }
         }
         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 1daa672..3b0459a 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -36,6 +36,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;
@@ -78,9 +81,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.8.3.2

[Qemu-devel] [PATCH v3 10/16] hw/intc/arm_gic: Implement Non-secure view of RPR

[Greg Bellows]

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>
---
 hw/intc/arm_gic.c      | 17 ++++++++++++++++-
 hw/intc/gic_internal.h |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index b62cde2..e65a271 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -314,6 +314,21 @@ void gic_set_cpu_control(GICState *s, int cpu, uint32_t value)
     }
 }
 
+uint8_t gic_get_running_priority(GICState *s, int cpu)
+{
+    if (s->security_extn && ns_access()) {
+        if (s->running_priority[cpu] & 0x80) {
+            /* Running priority in upper half, return Non-secure view */
+            return s->running_priority[cpu] << 1;
+        } else {
+            /* Running priority in lower half, RAZ */
+            return 0;
+        }
+    } else {
+        return s->running_priority[cpu];
+    }
+}
+
 void gic_complete_irq(GICState *s, int cpu, int irq)
 {
     int update = 0;
@@ -850,7 +865,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
     case 0x0c: /* Acknowledge */
         return gic_acknowledge_irq(s, cpu);
     case 0x14: /* Running Priority */
-        return s->running_priority[cpu];
+        return gic_get_running_priority(s, cpu);
     case 0x18: /* Highest Pending Interrupt */
         return s->current_pending[cpu];
     case 0x1c: /* Aliased Binary Point */
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index e360de6..821ce16 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -78,6 +78,7 @@ void gic_init_irqs_and_distributor(GICState *s);
 void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
 uint32_t gic_get_cpu_control(GICState *s, int cpu);
 void gic_set_cpu_control(GICState *s, int cpu, uint32_t value);
+uint8_t gic_get_running_priority(GICState *s, int cpu);
 
 
 static inline bool gic_test_pending(GICState *s, int irq, int cm)
-- 
1.8.3.2

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

[Greg Bellows]

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>
---
 hw/intc/arm_gic.c      | 29 ++++++++++++++++++++++++++++-
 hw/intc/gic_internal.h |  1 +
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index e65a271..d6f8dae 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -329,6 +329,33 @@ uint8_t gic_get_running_priority(GICState *s, int cpu)
     }
 }
 
+uint16_t gic_get_current_pending_irq(GICState *s, int cpu)
+{
+    bool isGrp0;
+    uint16_t pendingId = s->current_pending[cpu];
+
+    if (pendingId < GIC_MAXIRQ && (s->revision >= 2 || s->security_extn)) {
+        isGrp0 = GIC_TEST_GROUP0(pendingId, (1 << cpu));
+        if ((isGrp0 && !s->enabled_grp[0])
+                || (!isGrp0 && !s->enabled_grp[1])) {
+            return 1023;
+        }
+        if (s->security_extn) {
+            if (isGrp0 && ns_access()) {
+                /* Group0 interrupts hidden from Non-secure access */
+                return 1023;
+            }
+            if (!isGrp0 && !ns_access()
+                    && !(s->cpu_control[cpu][0] & GICC_CTLR_S_ACK_CTL)) {
+                /* Group1 interrupts only seen by Secure access if
+                 * AckCtl bit set. */
+                return 1022;
+            }
+        }
+    }
+    return pendingId;
+}
+
 void gic_complete_irq(GICState *s, int cpu, int irq)
 {
     int update = 0;
@@ -867,7 +894,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
     case 0x14: /* Running Priority */
         return gic_get_running_priority(s, cpu);
     case 0x18: /* Highest Pending Interrupt */
-        return s->current_pending[cpu];
+        return gic_get_current_pending_irq(s, cpu);
     case 0x1c: /* Aliased Binary Point */
         if (!s->security_extn || (s->security_extn && ns_access())) {
             /* If Security Extensions are present ABPR is a secure register,
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 821ce16..fbb1f66 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -79,6 +79,7 @@ void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val);
 uint32_t gic_get_cpu_control(GICState *s, int cpu);
 void gic_set_cpu_control(GICState *s, int cpu, uint32_t value);
 uint8_t gic_get_running_priority(GICState *s, int cpu);
+uint16_t gic_get_current_pending_irq(GICState *s, int cpu);
 
 
 static inline bool gic_test_pending(GICState *s, int irq, int cm)
-- 
1.8.3.2

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

[Greg Bellows]

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

---

v1 -> v2
- Fix issue with EOIR writes involving AckCtl.  AckCtl is ignored on EOIR
  group 1 interrupts when non-secure.  Group 1 interrupts are only ignored when
  secure and AckCTl is clear.
---
 hw/intc/arm_gic.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index d6f8dae..1ba4dfd 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -384,6 +384,21 @@ void gic_complete_irq(GICState *s, int cpu, int irq)
             GIC_SET_PENDING(irq, cm);
             update = 1;
         }
+    } else if ((s->revision >= 2 && !s->security_extn)
+                 || (s->security_extn && !ns_access())) {
+        /* Handle GICv2 without Security Extensions or GIC with Security
+         * Extensions and a secure write.
+         */
+        if (!GIC_TEST_GROUP0(irq, cm) && !ns_access()
+                && !(s->cpu_control[cpu][0] & GICC_CTLR_S_ACK_CTL)) {
+            /* Unpredictable. We choose to ignore. */
+            DPRINTF("EOI for Group1 interrupt %d ignored "
+                    "(AckCtl disabled)\n", irq);
+            return;
+        }
+    } else if (s->security_extn && ns_access() && GIC_TEST_GROUP0(irq, cm)) {
+        DPRINTF("Non-secure EOI for Group0 interrupt %d ignored\n", irq);
+        return;
     }
 
     if (irq != s->running_irq[cpu]) {
-- 
1.8.3.2

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

[Greg Bellows]

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>

---

v1 -> v2
- Fix issue in gic_acknowledge_irq() where the GICC_CTLR_S_ACK_CTL flag is
  applied without first checking whether the read is secure or non-secure.
  Secure reads of IAR when AckCtl is 0 return a spurious ID of 1022, but
  non-secure ignores the flag.
---
 hw/intc/arm_gic.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 1ba4dfd..3959693 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -190,11 +190,36 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
     int ret, irq, src;
     int cm = 1 << cpu;
     irq = s->current_pending[cpu];
+    bool isGrp0;
     if (irq == 1023
             || GIC_GET_PRIORITY(irq, cpu) >= s->running_priority[cpu]) {
         DPRINTF("ACK no pending IRQ\n");
         return 1023;
     }
+
+    if (s->revision >= 2 || s->security_extn) {
+        isGrp0 = GIC_TEST_GROUP0(irq, (1 << cpu));
+        if ((isGrp0 && (!s->enabled_grp[0]
+                    || !(s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0)))
+           || (!isGrp0 && (!s->enabled_grp[1]
+                    || !(s->cpu_control[cpu][1] & GICC_CTLR_NS_EN_GRP1)))) {
+            return 1023;
+        }
+
+        if ((s->revision >= 2 && !s->security_extn)
+                || (s->security_extn && !ns_access())) {
+            if (!isGrp0 && !ns_access() &&
+                !(s->cpu_control[cpu][0] & GICC_CTLR_S_ACK_CTL)) {
+                DPRINTF("Read of IAR ignored for Group1 interrupt %d "
+                        "(AckCtl disabled)\n", irq);
+                return 1022;
+            }
+        } else if (s->security_extn && ns_access() && isGrp0) {
+            DPRINTF("Non-secure read of IAR ignored for Group0 interrupt %d\n",
+                    irq);
+            return 1023;
+        }
+    }
     s->last_active[irq][cpu] = s->running_irq[cpu];
 
     if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 14/16] hw/intc/arm_gic: Restrict priority view

[Greg Bellows]

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>
---
 hw/intc/arm_gic.c      | 66 +++++++++++++++++++++++++++++++++++++++++++++-----
 hw/intc/gic_internal.h |  3 +++
 2 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 3959693..781cca9 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -258,11 +258,66 @@ uint32_t gic_acknowledge_irq(GICState *s, int cpu)
 
 void gic_set_priority(GICState *s, int cpu, int irq, uint8_t val)
 {
+    uint8_t prio = val;
+
+    if (s->security_extn && ns_access()) {
+        if (GIC_TEST_GROUP0(irq, (1 << cpu))) {
+            return; /* Ignore Non-secure access of Group0 IRQ */
+        }
+        prio = 0x80 | (prio >> 1); /* Non-secure view */
+    }
+
     if (irq < GIC_INTERNAL) {
-        s->priority1[irq][cpu] = val;
+        s->priority1[irq][cpu] = prio;
     } else {
-        s->priority2[(irq) - GIC_INTERNAL] = val;
+        s->priority2[(irq) - GIC_INTERNAL] = prio;
+    }
+}
+
+uint32_t gic_get_priority(GICState *s, int cpu, int irq)
+{
+    uint32_t prio = GIC_GET_PRIORITY(irq, cpu);
+
+    if (s->security_extn && ns_access()) {
+        if (GIC_TEST_GROUP0(irq, (1 << cpu))) {
+            return 0; /* Non-secure access cannot read priority of Group0 IRQ */
+        }
+        prio = (prio << 1); /* Non-secure view */
     }
+    return prio;
+}
+
+void gic_set_priority_mask(GICState *s, int cpu, uint8_t val)
+{
+    uint8_t pmask = (val & 0xff);
+
+    if (s->security_extn && ns_access()) {
+        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;
+}
+
+uint32_t gic_get_priority_mask(GICState *s, int cpu)
+{
+    uint32_t pmask = s->priority_mask[cpu];
+
+    if (s->security_extn && ns_access()) {
+        if (pmask & 0x80) {
+            /* Priority Mask in upper half, return Non-secure view */
+            pmask = (pmask << 1);
+        } else {
+            /* Priority Mask in lower half, RAZ */
+            pmask = 0;
+        }
+    }
+    return pmask;
+
 }
 
 uint32_t gic_get_cpu_control(GICState *s, int cpu)
@@ -557,7 +612,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
         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);
     } else if (offset < 0xc00) {
         /* Interrupt CPU Target.  */
         if (s->num_cpu == 1 && s->revision != REV_11MPCORE) {
@@ -921,7 +976,7 @@ static uint32_t gic_cpu_read(GICState *s, int cpu, int offset)
     case 0x00: /* Control */
         return gic_get_cpu_control(s, cpu);
     case 0x04: /* Priority mask */
-        return s->priority_mask[cpu];
+        return gic_get_priority_mask(s, cpu);
     case 0x08: /* Binary Point */
         if (s->security_extn && ns_access()) {
             /* BPR is banked. Non-secure copy stored in ABPR. */
@@ -959,8 +1014,7 @@ static void gic_cpu_write(GICState *s, int cpu, int offset, uint32_t value)
     case 0x00: /* Control */
         return gic_set_cpu_control(s, cpu, value);
     case 0x04: /* Priority mask */
-        s->priority_mask[cpu] = (value & 0xff);
-        break;
+        return gic_set_priority_mask(s, cpu, value);
     case 0x08: /* Binary Point */
         if (s->security_extn && ns_access()) {
             /* BPR is banked. Non-secure copy stored in ABPR. */
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index fbb1f66..13fe5a6 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -76,6 +76,9 @@ 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);
+uint32_t gic_get_priority(GICState *s, int cpu, int irq);
+void gic_set_priority_mask(GICState *s, int cpu, uint8_t val);
+uint32_t gic_get_priority_mask(GICState *s, int cpu);
 uint32_t gic_get_cpu_control(GICState *s, int cpu);
 void gic_set_cpu_control(GICState *s, int cpu, uint32_t value);
 uint8_t gic_get_running_priority(GICState *s, int cpu);
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 15/16] hw/intc/arm_gic: Break out gic_update() function

[Greg Bellows]

From: Fabian Aggeler <aggelerf@ethz.ch>

Prepare to split gic_update() in two functions, one for GICs with
interrupt grouping and one without grouping (existing).

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
---
 hw/intc/arm_gic.c      | 11 ++++++++---
 hw/intc/gic_internal.h |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 781cca9..c03b3dd 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -52,9 +52,7 @@ static inline bool ns_access(void)
     return true;
 }
 
-/* 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)
+inline void gic_update_no_grouping(GICState *s)
 {
     int best_irq;
     int best_prio;
@@ -93,6 +91,13 @@ void gic_update(GICState *s)
     }
 }
 
+/* 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)
+{
+    gic_update_no_grouping(s);
+}
+
 void gic_set_pending_private(GICState *s, int cpu, int irq)
 {
     int cm = 1 << cpu;
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index 13fe5a6..e16a7e5 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -73,6 +73,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);
+inline void gic_update_no_grouping(GICState *s);
 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.8.3.2

[Qemu-devel] [PATCH v3 16/16] hw/intc/arm_gic: add gic_update() for grouping

[Greg Bellows]

From: Fabian Aggeler <aggelerf@ethz.ch>

GICs with grouping (GICv2 or GICv1 with Security Extensions) have a
different exception generation model which is more complicated than
without interrupt grouping. We add a new function to handle this model.

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>

---

v1 -> v2
- Fix issue in gic_update_with_grouping() using the wrong combination of
  flag and CPU control bank for checking if group 1 interrupts are enabled.
---
 hw/intc/arm_gic.c      | 87 +++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/intc/gic_internal.h |  1 +
 2 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index c03b3dd..570bd4f 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -52,6 +52,87 @@ static inline bool ns_access(void)
     return true;
 }
 
+inline void gic_update_with_grouping(GICState *s)
+{
+    int best_irq;
+    int best_prio;
+    int irq;
+    int irq_level;
+    int fiq_level;
+    int cpu;
+    int cm;
+    bool next_int;
+    bool next_grp0;
+    bool gicc_grp0_enabled;
+    bool gicc_grp1_enabled;
+
+    for (cpu = 0; cpu < NUM_CPU(s); cpu++) {
+        cm = 1 << cpu;
+        gicc_grp0_enabled = s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0;
+        gicc_grp1_enabled = s->cpu_control[cpu][1] & GICC_CTLR_NS_EN_GRP1;
+        next_int = 0;
+        next_grp0 = 0;
+
+        s->current_pending[cpu] = 1023;
+        if ((!s->enabled_grp[0] && !s->enabled_grp[1])
+                || (!gicc_grp0_enabled && !gicc_grp1_enabled)) {
+            qemu_irq_lower(s->parent_irq[cpu]);
+            qemu_irq_lower(s->parent_fiq[cpu]);
+            return;
+        }
+
+        /* Determine highest priority pending interrupt */
+        best_prio = 0x100;
+        best_irq = 1023;
+        for (irq = 0; irq < s->num_irq; irq++) {
+            if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, cm)) {
+                if (GIC_GET_PRIORITY(irq, cpu) < best_prio) {
+                    best_prio = GIC_GET_PRIORITY(irq, cpu);
+                    best_irq = irq;
+                }
+            }
+        }
+
+        /* Priority of IRQ higher than priority mask? */
+        if (best_prio < s->priority_mask[cpu]) {
+            s->current_pending[cpu] = best_irq;
+            if (GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[0]) {
+                /* TODO: Add subpriority handling (binary point register) */
+                if (best_prio < s->running_priority[cpu]) {
+                    next_int = true;
+                    next_grp0 = true;
+                }
+            } else if (!GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[1]) {
+                /* TODO: Add subpriority handling (binary point register) */
+                if (best_prio < s->running_priority[cpu]) {
+                    next_int = true;
+                    next_grp0 = false;
+                }
+            }
+        }
+
+        fiq_level = 0;
+        irq_level = 0;
+        if (next_int) {
+            if (next_grp0 && (s->cpu_control[cpu][0] & GICC_CTLR_S_FIQ_EN)) {
+                if (gicc_grp0_enabled) {
+                    fiq_level = 1;
+                    DPRINTF("Raised pending FIQ %d (cpu %d)\n", best_irq, cpu);
+                }
+            } else {
+                if ((next_grp0 && gicc_grp0_enabled)
+                     || (!next_grp0 && gicc_grp1_enabled)) {
+                    irq_level = 1;
+                    DPRINTF("Raised pending IRQ %d (cpu %d)\n", best_irq, cpu);
+                }
+            }
+        }
+        /* Set IRQ/FIQ signal */
+        qemu_set_irq(s->parent_irq[cpu], irq_level);
+        qemu_set_irq(s->parent_fiq[cpu], fiq_level);
+    }
+}
+
 inline void gic_update_no_grouping(GICState *s)
 {
     int best_irq;
@@ -95,7 +176,11 @@ inline void gic_update_no_grouping(GICState *s)
 /* Update interrupt status after enabled or pending bits have been changed.  */
 void gic_update(GICState *s)
 {
-    gic_update_no_grouping(s);
+    if (s->revision >= 2 || s->security_extn) {
+        gic_update_with_grouping(s);
+    } else {
+        gic_update_no_grouping(s);
+    }
 }
 
 void gic_set_pending_private(GICState *s, int cpu, int irq)
diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
index e16a7e5..01859ed 100644
--- a/hw/intc/gic_internal.h
+++ b/hw/intc/gic_internal.h
@@ -73,6 +73,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);
+inline void gic_update_with_grouping(GICState *s);
 inline void gic_update_no_grouping(GICState *s);
 void gic_update(GICState *s);
 void gic_init_irqs_and_distributor(GICState *s);
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v3 05/16] hw/intc/arm_gic: Add ns_access() function

[Edgar E. Iglesias]

On Wed, Apr 15, 2015 at 11:02:11AM -0500, Greg Bellows wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> Security Extensions for GICv1 and GICv2 use register banking
> to provide transparent access to seperate Secure and Non-secure
> copies of GIC configuration registers. This function will later
> be replaced by code determining the security state of a read/write
> access to a register.


Hi,

Can we rebase this on top of Peters memory attribute series to get the actual secure attribute?

Cheers,
Edgar



> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> ---
>  hw/intc/arm_gic.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index cdf7408..e0bce6e 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -45,6 +45,13 @@ static inline int gic_get_current_cpu(GICState *s)
>      return 0;
>  }
>  
> +/* Security state of a read / write access */
> +static inline bool ns_access(void)
> +{
> +    /* TODO: use actual security state */
> +    return true;
> +}
> +
>  /* 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)
> -- 
> 1.8.3.2
> 
> 

Re: [Qemu-devel] [PATCH v3 06/16] hw/intc/arm_gic: Add Interrupt Group Registers

[Edgar E. Iglesias]

On Wed, Apr 15, 2015 at 11:02:12AM -0500, Greg Bellows wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> Interrupt Group Registers (previously called Interrupt Security
> Registers) as defined in GICv1 with Security Extensions or GICv2 allow
> to configure interrupts as Secure (Group0) or Non-secure (Group1).
> In GICv2 these registers are implemented independent of the existence of
> Security Extensions.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> 
> ---
> 
> v1 -> v2
> - Add clarifying comments to gic_dist_readb/writeb on interrupt group register
>   update
> - Swap GIC_SET_GROUP0/1 macro logic.  Setting the irq_state.group field for
>   group 0 should clear the bit not set it.  Similarly, setting the field for
>   group 1 should set the bit not clear it.
> ---
>  hw/intc/arm_gic.c                | 49 +++++++++++++++++++++++++++++++++++++---
>  hw/intc/arm_gic_common.c         |  1 +
>  hw/intc/gic_internal.h           |  4 ++++
>  include/hw/intc/arm_gic_common.h |  1 +
>  4 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index e0bce6e..aa4402e 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -312,8 +312,27 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
>          if (offset < 0x08)
>              return 0;
>          if (offset >= 0x80) {
> -            /* Interrupt Security , RAZ/WI */
> -            return 0;
> +            /* Interrupt Group Registers
> +             *
> +             * For GIC with Security Extn and Non-secure access RAZ/WI
> +             * For GICv1 without Security Extn RAZ/WI
> +             */
> +            res = 0;
> +            if (!(s->security_extn && ns_access()) &&
> +                    ((s->revision == 1 && s->security_extn)
> +                            || s->revision == 2)) {
> +                /* 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_GROUP0(irq + i, cm)) {
> +                        res |= (1 << i);
> +                    }
> +                }
> +            }
> +            return res;
>          }
>          goto bad_reg;
>      } else if (offset < 0x200) {
> @@ -457,7 +476,31 @@ 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
> +             *
> +             * For GIC with Security Extn and Non-secure access RAZ/WI
> +             * For GICv1 without Security Extn RAZ/WI
> +             */
> +            if (!(s->security_extn && ns_access()) &&
> +                    ((s->revision == 1 && s->security_extn)
> +                            || s->revision == 2)) {
> +                /* 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++) {
> +                    /* Group bits are banked for private interrupts (internal)*/
> +                    int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
> +                    if (value & (1 << i)) {
> +                        /* Group1 (Non-secure) */
> +                        GIC_SET_GROUP1(irq + i, cm);
> +                    } else {
> +                        /* Group0 (Secure) */
> +                        GIC_SET_GROUP0(irq + i, cm);
> +                    }

This looks weird to me...
Shouldn't this simply be s->irq_state[irq].group = value & (1 << i) ?
(possibly behind macros allthough I must say these macros are confusing)

> +                }
> +            }
>          } else {
>              goto bad_reg;
>          }
> diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
> index e35049d..28f3b2a 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -52,6 +52,7 @@ 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()
>      }
>  };
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index e87ef36..f01955a 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_SET_GROUP0(irq, cm) (s->irq_state[irq].group &= ~(cm))
> +#define GIC_SET_GROUP1(irq, cm) (s->irq_state[irq].group |= (cm))


Can we make the group a bool or some integer?
An interrupt can only belong to one group at a time AFAIK, so we don't really need bit masking...
Or am I missing something?

Cheers,
Edgar


> +#define GIC_TEST_GROUP0(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.8.3.2
> 
> 

Re: [Qemu-devel] [PATCH v3 07/16] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked

[Edgar E. Iglesias]

On Wed, Apr 15, 2015 at 11:02:13AM -0500, Greg Bellows wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> ICDDCR/GICD_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
> Distributor to the CPU interfaces for Group0 and Group1.
> 
> EnableGroup0 (Bit [1]) in GICv1 is IMPDEF. Since this bit (Enable
> Non-secure) is present in the integrated IC of the Cortex-A9 MPCore,
> which implements the GICv1 profile, we support this bit in GICv1 too.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> 
> ---
> v2 -> v3
> - Added missing return in gic_dist_readb()
> - Fixed typos
> 
> v1 -> v2
> - Fix gic_dist_writeb() update of GICD_CTRL to only use bit[0] of the
>   EnableGrp1 field not bit[1].
> - Add clarifying comments
> ---
>  hw/intc/arm_gic.c                | 50 ++++++++++++++++++++++++++++++++++++----
>  hw/intc/arm_gic_common.c         |  2 +-
>  include/hw/intc/arm_gic_common.h |  7 +++++-
>  3 files changed, 53 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index aa4402e..b9dfde3 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -302,8 +302,26 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset)
>      cpu = gic_get_current_cpu(s);
>      cm = 1 << cpu;
>      if (offset < 0x100) {
> -        if (offset == 0)
> -            return s->enabled;
> +        if (offset == 0) {      /* GICD_CTLR */
> +            res = 0;
> +            if ((s->revision == 2 && !s->security_extn)
> +                    || (s->security_extn && !ns_access())) {
> +                /* In this case the GICD_CTLR contains both a group0 and group1
> +                 * enable bit, so we create the resuling value by aggregating
> +                 * the bits from the two enable values.
> +                 * The group0 enable bit is only visible to secure accesses.
> +                 * The group1 enable bit (bit[1]) is an alias of bit[0] in
> +                 * the non-secure copy (enabled_grp[1]).
> +                 */
> +                res = (s->enabled_grp[1] << 1) | s->enabled_grp[0];
> +            } else if (s->security_extn && ns_access()) {
> +                res = s->enabled_grp[1];
> +            } else {
> +                /* Neither GICv2 nor Security Extensions present */
> +                res = s->enabled;
> +            }
> +            return res;
> +        }
>          if (offset == 4)
>              /* Interrupt Controller Type Register */
>              return ((s->num_irq / 32) - 1)
> @@ -471,8 +489,32 @@ 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->revision == 2 && !s->security_extn)
> +                    || (s->security_extn && !ns_access())) {
> +                s->enabled_grp[0] = value & (1U << 0); /* EnableGrp0 */
> +                /* For a GICv1 with Security Extn "EnableGrp1" is IMPDEF. */
> +                /* We only use the first bit of the enabled_grp vars to
> +                 * indicate enabled or disabled.  In this case we have to shift
> +                 * the incoming value down to the low bit because the group1
> +                 * enabled bit is bit[1] in the secure/GICv2 GICD_CTLR.
> +                 */
> +                s->enabled_grp[1] = (value >> 1) & 0x1; /* EnableGrp1 */

I would personally prefer just:

s->enabled_grp[0] = value & 1;
s->enabled_grp[1] = value & 2;


> +                DPRINTF("Group0 distribution %sabled\n"
> +                        "Group1 distribution %sabled\n",
> +                                        s->enabled_grp[0] ? "En" : "Dis",
> +                                        s->enabled_grp[1] ? "En" : "Dis");
> +            } else if (s->security_extn && ns_access()) {
> +                /* If we are non-secure only the group1 enable bit is visible
> +                 * as bit[0] in the GICD_CTLR.
> +                 */
> +                s->enabled_grp[1] = (value & 0x1);

Can we remove the parenthesis?


> +                DPRINTF("Group1 distribution %sabled\n",
> +                        s->enabled_grp[1] ? "En" : "Dis");
> +            } else {
> +                /* Neither GICv2 nor Security Extensions present */
> +                s->enabled = (value & 0x1);
> +                DPRINTF("Distribution %sabled\n", s->enabled ? "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 28f3b2a..c44050d 100644
> --- a/hw/intc/arm_gic_common.c
> +++ b/hw/intc/arm_gic_common.c
> @@ -64,7 +64,7 @@ static const VMStateDescription vmstate_gic = {
>      .pre_save = gic_pre_save,
>      .post_load = gic_post_load,
>      .fields = (VMStateField[]) {
> -        VMSTATE_BOOL(enabled, GICState),
> +        VMSTATE_UINT8_ARRAY(enabled_grp, GICState, GIC_NR_GROUP),
>          VMSTATE_BOOL_ARRAY(cpu_enabled, GICState, GIC_NCPU),
>          VMSTATE_STRUCT_ARRAY(irq_state, GICState, GIC_MAXIRQ, 1,
>                               vmstate_gic_irq_state, gic_irq_state),
> diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
> index b78981e..16e193d 100644
> --- a/include/hw/intc/arm_gic_common.h
> +++ b/include/hw/intc/arm_gic_common.h
> @@ -30,6 +30,8 @@
>  #define GIC_NR_SGIS 16
>  /* Maximum number of possible CPU interfaces, determined by GIC architecture */
>  #define GIC_NCPU 8
> +/* Number of Groups (Group0 [Secure], Group1 [Non-secure]) */
> +#define GIC_NR_GROUP 2
>  
>  #define MAX_NR_GROUP_PRIO 128
>  #define GIC_NR_APRS (MAX_NR_GROUP_PRIO / 32)
> @@ -52,7 +54,10 @@ typedef struct GICState {
>  
>      qemu_irq parent_irq[GIC_NCPU];
>      qemu_irq parent_fiq[GIC_NCPU];
> -    bool enabled;
> +    union {
> +        uint8_t enabled;
> +        uint8_t enabled_grp[GIC_NR_GROUP]; /* EnableGrp0 and EnableGrp1 */
> +    };
>      bool cpu_enabled[GIC_NCPU];
>  
>      gic_irq_state irq_state[GIC_MAXIRQ];
> -- 
> 1.8.3.2
> 
> 

Re: [Qemu-devel] [PATCH v3 16/16] hw/intc/arm_gic: add gic_update() for grouping

[Edgar E. Iglesias]

On Wed, Apr 15, 2015 at 11:02:22AM -0500, Greg Bellows wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
> 
> GICs with grouping (GICv2 or GICv1 with Security Extensions) have a
> different exception generation model which is more complicated than
> without interrupt grouping. We add a new function to handle this model.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> 
> ---
> 
> v1 -> v2
> - Fix issue in gic_update_with_grouping() using the wrong combination of
>   flag and CPU control bank for checking if group 1 interrupts are enabled.
> ---
>  hw/intc/arm_gic.c      | 87 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  hw/intc/gic_internal.h |  1 +
>  2 files changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index c03b3dd..570bd4f 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -52,6 +52,87 @@ static inline bool ns_access(void)
>      return true;
>  }
>  
> +inline void gic_update_with_grouping(GICState *s)
> +{
> +    int best_irq;
> +    int best_prio;
> +    int irq;
> +    int irq_level;
> +    int fiq_level;
> +    int cpu;
> +    int cm;
> +    bool next_int;
> +    bool next_grp0;
> +    bool gicc_grp0_enabled;
> +    bool gicc_grp1_enabled;
> +
> +    for (cpu = 0; cpu < NUM_CPU(s); cpu++) {
> +        cm = 1 << cpu;
> +        gicc_grp0_enabled = s->cpu_control[cpu][0] & GICC_CTLR_S_EN_GRP0;
> +        gicc_grp1_enabled = s->cpu_control[cpu][1] & GICC_CTLR_NS_EN_GRP1;
> +        next_int = 0;
> +        next_grp0 = 0;
> +
> +        s->current_pending[cpu] = 1023;
> +        if ((!s->enabled_grp[0] && !s->enabled_grp[1])
> +                || (!gicc_grp0_enabled && !gicc_grp1_enabled)) {
> +            qemu_irq_lower(s->parent_irq[cpu]);
> +            qemu_irq_lower(s->parent_fiq[cpu]);
> +            return;
> +        }
> +
> +        /* Determine highest priority pending interrupt */
> +        best_prio = 0x100;
> +        best_irq = 1023;
> +        for (irq = 0; irq < s->num_irq; irq++) {
> +            if (GIC_TEST_ENABLED(irq, cm) && gic_test_pending(s, irq, cm)) {
> +                if (GIC_GET_PRIORITY(irq, cpu) < best_prio) {
> +                    best_prio = GIC_GET_PRIORITY(irq, cpu);
> +                    best_irq = irq;
> +                }
> +            }
> +        }
> +
> +        /* Priority of IRQ higher than priority mask? */
> +        if (best_prio < s->priority_mask[cpu]) {
> +            s->current_pending[cpu] = best_irq;
> +            if (GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[0]) {
> +                /* TODO: Add subpriority handling (binary point register) */
> +                if (best_prio < s->running_priority[cpu]) {
> +                    next_int = true;
> +                    next_grp0 = true;
> +                }
> +            } else if (!GIC_TEST_GROUP0(best_irq, cm) && s->enabled_grp[1]) {
> +                /* TODO: Add subpriority handling (binary point register) */
> +                if (best_prio < s->running_priority[cpu]) {
> +                    next_int = true;
> +                    next_grp0 = false;
> +                }
> +            }
> +        }
> +
> +        fiq_level = 0;
> +        irq_level = 0;
> +        if (next_int) {
> +            if (next_grp0 && (s->cpu_control[cpu][0] & GICC_CTLR_S_FIQ_EN)) {
> +                if (gicc_grp0_enabled) {
> +                    fiq_level = 1;
> +                    DPRINTF("Raised pending FIQ %d (cpu %d)\n", best_irq, cpu);
> +                }
> +            } else {
> +                if ((next_grp0 && gicc_grp0_enabled)
> +                     || (!next_grp0 && gicc_grp1_enabled)) {
> +                    irq_level = 1;
> +                    DPRINTF("Raised pending IRQ %d (cpu %d)\n", best_irq, cpu);
> +                }
> +            }
> +        }
> +        /* Set IRQ/FIQ signal */
> +        qemu_set_irq(s->parent_irq[cpu], irq_level);
> +        qemu_set_irq(s->parent_fiq[cpu], fiq_level);
> +    }
> +}

Hi,

Isn't the grouping function a superset of gic_update_no_grouping?
Can we reuse it for the non-grouping case?

Cheers,
Edgar


> +
>  inline void gic_update_no_grouping(GICState *s)
>  {
>      int best_irq;
> @@ -95,7 +176,11 @@ inline void gic_update_no_grouping(GICState *s)
>  /* Update interrupt status after enabled or pending bits have been changed.  */
>  void gic_update(GICState *s)
>  {
> -    gic_update_no_grouping(s);
> +    if (s->revision >= 2 || s->security_extn) {
> +        gic_update_with_grouping(s);
> +    } else {
> +        gic_update_no_grouping(s);
> +    }
>  }
>  
>  void gic_set_pending_private(GICState *s, int cpu, int irq)
> diff --git a/hw/intc/gic_internal.h b/hw/intc/gic_internal.h
> index e16a7e5..01859ed 100644
> --- a/hw/intc/gic_internal.h
> +++ b/hw/intc/gic_internal.h
> @@ -73,6 +73,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);
> +inline void gic_update_with_grouping(GICState *s);
>  inline void gic_update_no_grouping(GICState *s);
>  void gic_update(GICState *s);
>  void gic_init_irqs_and_distributor(GICState *s);
> -- 
> 1.8.3.2
> 
> 

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

[Peter Maydell]

On 15 April 2015 at 17:02, Greg Bellows <greg.bellows@linaro.org> 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_control.
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>
> ---
>
> v1 -> v2
> - Rework gic_set_cpu_control() and gic_get_cpu_control() to close gap on
>   handling GICv1 wihtout security extensions.
> - Fix use of incorrect control index in update.
> ---
>  hw/intc/arm_gic.c                | 82 +++++++++++++++++++++++++++++++++++++---
>  hw/intc/arm_gic_common.c         |  5 ++-
>  hw/intc/arm_gic_kvm.c            |  8 ++--
>  hw/intc/armv7m_nvic.c            |  2 +-
>  hw/intc/gic_internal.h           | 14 +++++++
>  include/hw/intc/arm_gic_common.h |  2 +-
>  6 files changed, 100 insertions(+), 13 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index b9dfde3..b402e00 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -66,7 +66,7 @@ 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->enabled || !(s->cpu_control[cpu][1] & 1)) {

This breaks no-security-extensions configs at this point
in the patchseries, because here we always test
cpu_control[cpu][1] bit 0...

> +void gic_set_cpu_control(GICState *s, int cpu, uint32_t value)
> +{
> +    if (!s->security_extn) {
> +        if (s->revision == 1) {
> +            s->cpu_control[cpu][1] = value & 0x1;
> +            DPRINTF("CPU Interface %d %sabled\n", cpu,
> +                s->cpu_control[cpu][1] ? "En" : "Dis");
> +        } else {
> +            /* Write to Secure instance of the register */
> +            s->cpu_control[cpu][0] = value & GICC_CTLR_S_MASK;
> +            /* Synchronize EnableGrp1 alias of Non-secure copy */
> +            s->cpu_control[cpu][1] &= ~GICC_CTLR_NS_EN_GRP1;
> +            s->cpu_control[cpu][1] |=
> +                (value & GICC_CTLR_S_EN_GRP1) ? GICC_CTLR_NS_EN_GRP1 : 0;

...but here we only set bit 0 of [cpu][1] if bit 1 of the value is
set. So a guest which only sets bit 0 stops working (my testcase
is an aarch64 linux guest on the virt board).

This does seem to start working again later in the series, so
clearly something needs to be rearranged so we don't break
bisection.

-- PMM

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

[Peter Maydell]

On 27 April 2015 at 16:00, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 15 April 2015 at 17:02, Greg Bellows <greg.bellows@linaro.org> 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_control.
>>
>> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>>
>> ---
>>
>> v1 -> v2
>> - Rework gic_set_cpu_control() and gic_get_cpu_control() to close gap on
>>   handling GICv1 wihtout security extensions.
>> - Fix use of incorrect control index in update.
>> ---
>>  hw/intc/arm_gic.c                | 82 +++++++++++++++++++++++++++++++++++++---
>>  hw/intc/arm_gic_common.c         |  5 ++-
>>  hw/intc/arm_gic_kvm.c            |  8 ++--
>>  hw/intc/armv7m_nvic.c            |  2 +-
>>  hw/intc/gic_internal.h           | 14 +++++++
>>  include/hw/intc/arm_gic_common.h |  2 +-
>>  6 files changed, 100 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index b9dfde3..b402e00 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -66,7 +66,7 @@ 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->enabled || !(s->cpu_control[cpu][1] & 1)) {
>
> This breaks no-security-extensions configs at this point
> in the patchseries, because here we always test
> cpu_control[cpu][1] bit 0...
>
>> +void gic_set_cpu_control(GICState *s, int cpu, uint32_t value)
>> +{
>> +    if (!s->security_extn) {
>> +        if (s->revision == 1) {
>> +            s->cpu_control[cpu][1] = value & 0x1;
>> +            DPRINTF("CPU Interface %d %sabled\n", cpu,
>> +                s->cpu_control[cpu][1] ? "En" : "Dis");
>> +        } else {
>> +            /* Write to Secure instance of the register */
>> +            s->cpu_control[cpu][0] = value & GICC_CTLR_S_MASK;
>> +            /* Synchronize EnableGrp1 alias of Non-secure copy */
>> +            s->cpu_control[cpu][1] &= ~GICC_CTLR_NS_EN_GRP1;
>> +            s->cpu_control[cpu][1] |=
>> +                (value & GICC_CTLR_S_EN_GRP1) ? GICC_CTLR_NS_EN_GRP1 : 0;
>
> ...but here we only set bit 0 of [cpu][1] if bit 1 of the value is
> set. So a guest which only sets bit 0 stops working (my testcase
> is an aarch64 linux guest on the virt board).

I think actually both parts of this code are wrong in some
way. Firstly, until we've actually fully implemented support
for grouping we want to be using the cpu_control[cpu][0] bit 0
for testing whether the CPU interface has disabled interrupts.
(When we later add the grouping support we end up not taking
this fast exit unless both group0 and group1 are disabled,
which is why the later patch unbreaks us.) Secondly, for
rev1 no-security-extns GICs we want to read/write cpu_control[cpu][0],
not [1], for the register access functions...

-- PMM