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

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

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                | 497 ++++++++++++++++++++++++++++++++++++---
 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, 526 insertions(+), 42 deletions(-)

--
1.8.3.2

[Qemu-devel] [PATCH v2 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>
---
 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 270ce05..ea05f8f 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -789,6 +789,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 v2 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>
---
 hw/arm/vexpress.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 7cbd13f..7121b8a 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -229,6 +229,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 v2 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 78f618d..e8bc059 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -354,6 +354,8 @@ static void 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 v2 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>

---

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 ea05f8f..0ee7778 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 v2 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>
---
 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 0ee7778..bee71a1 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 v2 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>

---

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 bee71a1..36ac188 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 v2 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>

---

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                | 49 ++++++++++++++++++++++++++++++++++++----
 hw/intc/arm_gic_common.c         |  2 +-
 include/hw/intc/arm_gic_common.h |  7 +++++-
 3 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 36ac188..1db15aa 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -302,8 +302,25 @@ 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_CTRL 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;
+            }
+        }
         if (offset == 4)
             /* Interrupt Controller Type Register */
             return ((s->num_irq / 32) - 1)
@@ -471,8 +488,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 v2 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>

---

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 1db15aa..3c0414f 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;
@@ -762,7 +836,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 */
@@ -788,9 +862,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 5038885..9a6a2bb 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -379,8 +379,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 */
@@ -478,9 +478,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 d0543d4..7f4a55c 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 v2 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>

---

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 3c0414f..3761d12 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -840,7 +840,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 */
@@ -848,7 +853,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:
@@ -867,13 +879,45 @@ 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 */
         return gic_complete_irq(s, cpu, value & 0x3ff);
     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 v2 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>
---
 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 3761d12..9b021d7 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;
@@ -849,7 +864,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 v2 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>
---
 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 9b021d7..15fd660 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;
@@ -866,7 +893,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 v2 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>

---

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 15fd660..2d83225 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 v2 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>

---

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 2d83225..7eb72df 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 v2 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>
---
 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 7eb72df..e01cfdc 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)
@@ -556,7 +611,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) {
@@ -920,7 +975,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. */
@@ -958,8 +1013,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 v2 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>
---
 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 e01cfdc..808aa18 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 v2 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>

---

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 808aa18..e33c470 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 v2 07/16] hw/intc/arm_gic: Make ICDDCR/GICD_CTLR banked

[Daniel Thompson]

On 30/10/14 22:12, 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>
> 
> ---
> 
> 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                | 49 ++++++++++++++++++++++++++++++++++++----
>  hw/intc/arm_gic_common.c         |  2 +-
>  include/hw/intc/arm_gic_common.h |  7 +++++-
>  3 files changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 36ac188..1db15aa 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -302,8 +302,25 @@ 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_CTRL 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; is missing here.


> +        }
>          if (offset == 4)
>              /* Interrupt Controller Type Register */
>              return ((s->num_irq / 32) - 1)
> @@ -471,8 +488,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];
> 

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

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 6503 bytes --]

Thanks Daniel, I'll address this in the next version.

On 4 November 2014 08:46, Daniel Thompson <daniel.thompson@linaro.org>
wrote:

> On 30/10/14 22:12, 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>
> >
> > ---
> >
> > 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                | 49
> ++++++++++++++++++++++++++++++++++++----
> >  hw/intc/arm_gic_common.c         |  2 +-
> >  include/hw/intc/arm_gic_common.h |  7 +++++-
> >  3 files changed, 52 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> > index 36ac188..1db15aa 100644
> > --- a/hw/intc/arm_gic.c
> > +++ b/hw/intc/arm_gic.c
> > @@ -302,8 +302,25 @@ 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_CTRL 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; is missing here.
>
>
> > +        }
> >          if (offset == 4)
> >              /* Interrupt Controller Type Register */
> >              return ((s->num_irq / 32) - 1)
> > @@ -471,8 +488,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];
> >
>
>

[-- Attachment #2: Type: text/html, Size: 8661 bytes --]

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

[Daniel Thompson]

On 30/10/14 22:12, 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>
> 
> ---
> 
> 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 808aa18..e33c470 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;

Shouldn't that be continue? Otherwise the state of CPU[N-1] influences
whether interrupts can be delivered to CPU[N].


> +        }
> +
> +        /* 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_gr && (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);
> 

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

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 5960 bytes --]

Thanks Daniel.

I see what you are saying, but historically the code looks like it has
always returned so I'd have to investigate it more as I am still learning
the code myself.  If this is a regression, it would be one inherited from
the previous gic_update() function.

I'll look further into it to see if this is an issue.

If anyone else familiar with the code has any input it would be appreciated.

Regards,

Greg

On 7 November 2014 06:44, Daniel Thompson <daniel.thompson@linaro.org>
wrote:

> On 30/10/14 22:12, 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>
> >
> > ---
> >
> > 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 808aa18..e33c470 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;
>
> Shouldn't that be continue? Otherwise the state of CPU[N-1] influences
> whether interrupts can be delivered to CPU[N].
>
>
> > +        }
> > +
> > +        /* 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_gr && (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);
> >
>
>

[-- Attachment #2: Type: text/html, Size: 8060 bytes --]

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

[Christoffer Dall]

On Mon, Nov 10, 2014 at 3:43 PM, Greg Bellows <greg.bellows@linaro.org> wrote:
[...]

> On 7 November 2014 06:44, Daniel Thompson <daniel.thompson@linaro.org>
> wrote:
>>
>> On 30/10/14 22:12, 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>
>> >
>> > ---
>> >
>> > 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 808aa18..e33c470 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;
>>
>> Shouldn't that be continue? Otherwise the state of CPU[N-1] influences
>> whether interrupts can be delivered to CPU[N].
>>
>>

> I see what you are saying, but historically the code looks like it has
> always returned so I'd have to investigate it more as I am still learning
> the code myself.  If this is a regression, it would be one inherited from
> the previous gic_update() function.

This does look like an existing bug, and I agree it should be continue
(you could optimize the disable IRQ state by using return if the
distributor disable all IRQs, but the code complexity cost is probably
not worth the optimization for the unusual situation.

Thanks,
-Christoffer

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

[Peter Maydell]

On 30 October 2014 at 22:11, Greg Bellows <greg.bellows@linaro.org> wrote:
> 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>

(Yes, this is review on a six month old patchset. My
punishment for taking so long to get to this is that
I'm the one that's going to have to pick up this work
and fix the review issues :-))

> ---
>  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 270ce05..ea05f8f 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -789,6 +789,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];

This is OK, but we need to init the new irq lines in
arm_gic_kvm.c too, to keep them with the same interface
to the rest of QEMU.

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:11, Greg Bellows <greg.bellows@linaro.org> 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>
> ---
>  hw/arm/vexpress.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
> index 7cbd13f..7121b8a 100644
> --- a/hw/arm/vexpress.c
> +++ b/hw/arm/vexpress.c
> @@ -229,6 +229,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));
>      }
>  }

This and patch 3 aren't wrong, but there's probably other board
level wiring up to do (eg setting the "enable security extns"
property on the GIC object). We should do all the board level
changes last, after the GIC changes.

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> wrote:
> 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>
>
> ---
>
> 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 ea05f8f..0ee7778 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),

Could use a comment describing the property. Also, we should
make the name of the property be in line with what we picked
for board or CPU level TZ properties. I think that's "secure".

Trying to set this property on something that's not a GICv1
or GICv2 should cause an error at realize.

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

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> 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.
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> ---
>  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 0ee7778..bee71a1 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;
> +}

We can do this with the transaction attributes patchset now.
However this function and its callsites will need adjusting
because we need the MemTxAttrs value to answer the question.
(Given that the question is just "attrs.secure" we probably
don't need the wrapper unless we want to include in this
"accesses are always secure if the GIC doesn't implement
the security extensions" logic.)

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> 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.

Worth mentioning in the commit that this patch only
implements the register accessors, not the functionality
that the bits control.

> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>
> ---
>
> 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 bee71a1..36ac188 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)) {

It would probably be clearer to write this as
   if (whatever) {
       return 0;
   }
   if (whatever) {
       return 0;
   }
   logic for registers;

rather than inverting the conditions from their more
natural and readable sense.

Also I suspect we will want some utility functions. One
that springs to mind here would be a gic_has_groups()
which returns (gic is v2 || (gic is v1 && has security extns)).

> +                /* Every byte offset holds 8 group status bits */
> +                irq = (offset - 0x080) * 8 + GIC_BASE_IRQ;

Better written as 0x80 I think.

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

Missing trailing space before */.

> +                    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),

We want to bump the vmstate version at some point in this
series, but if we're making several changes to the structure
I guess we can do that last.

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

This is confusing (especially the way the TEST macro
is doing an == 0 comparison). I think we should just have
GIC_SET_GROUP/GIC_CLEAR_GROUP/GIC_TEST_GROUP macros,
and not try to include the idea of "bit set means
group 1" in the macro names.

> +
>
>  /* 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
>

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> 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>
>
> ---
>
> 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                | 49 ++++++++++++++++++++++++++++++++++++----
>  hw/intc/arm_gic_common.c         |  2 +-
>  include/hw/intc/arm_gic_common.h |  7 +++++-
>  3 files changed, 52 insertions(+), 6 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 36ac188..1db15aa 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -302,8 +302,25 @@ 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_CTRL contains both a group0 and group1

Typo: should be _CTLR.

> +                 * 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;
> +            }
> +        }
>          if (offset == 4)
>              /* Interrupt Controller Type Register */
>              return ((s->num_irq / 32) - 1)
> @@ -471,8 +488,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..

Typo: repeated '.'.

> +                 */
> +                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 */
> +    };

Rather than a union, we should just make the code
use enabled_grp[0] when there's no grouping enabled.

>      bool cpu_enabled[GIC_NCPU];
>
>      gic_irq_state irq_state[GIC_MAXIRQ];
> --
> 1.8.3.2
>

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:11, Greg Bellows <greg.bellows@linaro.org> wrote:
> 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.

Hi Greg -- just noticed you forgot to add your own signed-off-by:
tag to these patches (needed as well as Fabian's since they passed
through your hands). Could you reply to this cover letter giving it,
please?

thanks
-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, 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>
>
> ---
>
> 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 1db15aa..3c0414f 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;
> +        }
> +    }
> +}

These feel a bit verbose -- I shall look to see if there's any
simplification possible here.

> +
>  void gic_complete_irq(GICState *s, int cpu, int irq)
>  {
>      int update = 0;
> @@ -762,7 +836,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 */
> @@ -788,9 +862,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);

gic_set_cpu_control() has a 'void' return type, so you
can't use it like this.

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

The type has changed from bool to uint32_t, so we should
be initializing these to 0, not 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 5038885..9a6a2bb 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -379,8 +379,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];

This is missing the second array index.  (Comment should have
the same var name as the code, too.)

>          kvm_gicc_access(s, 0x00, cpu, &reg, true);
>
>          /* s->priority_mask[cpu] -> GICC_PMR */
> @@ -478,9 +478,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 d0543d4..7f4a55c 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 */

Odd indent here.

> +
> +#define GICC_CTLR_S_MASK          0x7ff
> +
> +#define GICC_CTLR_NS_EN_GRP1  (1U << 0)
> +#define GICC_CTLR_NS_MASK         (1 | 3 << 5 | 1 << 9)

More brackets in this expression would help clarity.

> +
>
>  /* 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

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> 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>
>
> ---
>
> 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 3c0414f..3761d12 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -840,7 +840,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 */
> @@ -848,7 +853,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:
> @@ -867,13 +879,45 @@ 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];

My reading of the spec says that GICv2 without Security extensions
should not alias these two registers.

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> 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>
> ---
>  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 3761d12..9b021d7 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;
> @@ -849,7 +864,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);

I think this patch should be combined with patch 14 (which
deals with the other half of the priority register changes.)

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> 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>
> ---
>  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 9b021d7..15fd660 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;
> +}

Some coding style nits about var name capitalisation and
multiline comment style, but otherwise OK.

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> 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
> and completing Group1 interrupts from Secure state is only allowed if
> AckCtl bit is set.
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>
> ---
>
> 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 15fd660..2d83225 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;
> +        }

For GICv2 without the security extns, EOIR accesses should behave
as if they were secure, so the call to ns_access() inside this
conditional is wrong. We probably need to disentangle the v1-vs-v2
differences here.

> +    } else if (s->security_extn && ns_access() && GIC_TEST_GROUP0(irq, cm)) {
> +        DPRINTF("Non-secure EOI for Group0 interrupt %d ignored\n", irq);
> +        return;
>      }

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> 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.

Subject says "IAR writes" but it means "IAR reads".

>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>
> ---
>
> 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 2d83225..7eb72df 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;
> +        }
> +    }

This doesn't quite line up with the pseudocode in the GIC spec.
It's probably going to be easier to read with some utility functions
for 'grouping enabled' etc.

>      s->last_active[irq][cpu] = s->running_irq[cpu];
>
>      if (s->revision == REV_11MPCORE || s->revision == REV_NVIC) {
> --
> 1.8.3.2
>

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> 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>
> ---
>  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 7eb72df..e01cfdc 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)
> @@ -556,7 +611,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) {
> @@ -920,7 +975,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. */
> @@ -958,8 +1013,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);

'return some_function_returning_void()' again.

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

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> wrote:
> 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>
> ---
>  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 e01cfdc..808aa18 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);

This should probably be 'static inline' and doesn't need
a prototype in the header file.

-- PMM

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

[Peter Maydell]

On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> 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>
>
> ---
>
> 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 808aa18..e33c470 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);
> +    }
> +}

I'm not 100% convinced of the benefit of splitting out the
"no grouping" and "grouping" code paths (for instance it means
this function doesn't have the bugfix from commit b52b81e44f7
to honour the cpu-target-mask). I'll see how I feel when I
get to this patch in rework :-)

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

static inline, no proto.

-- PMM

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

[Peter Maydell]

On 14 April 2015 at 20:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 October 2014 at 22:12, Greg Bellows <greg.bellows@linaro.org> 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.

I've just noticed that these are actually already implemented
in KVM's in-kernel GICv2 model and exposed to userspace, so
we should have another patch which makes them be properly
handled in kvm_arm_gic_get/put.

Other than that I think the only KVM-related change we need
is going to be to give the arm_gic_kvm a 'secure' property
(which gives a realize error if it's set to 'true'), purely
to maintain consistency of interface between it and the
fully-emulated GICv2. (The view you get from a KVM vGIC
is basically a v2 GIC with grouping but no TZ support.)

-- PMM