[Qemu-devel] [PATCH 0/5] arm: enable TZ support for the GIC

[Qemu-devel] [PATCH 0/5] arm: enable TZ support for the GIC

[Peter Maydell]

This patchset enables the TZ support in the GIC for the systems
where we enable TZ support in the CPU. In practice that means
just the "virt" and "vexpress" boards, since all the others
disable the CPU TZ support.

Patch 1 I've already put out on list as a bugfix targeting 2.4.

The main interesting part in this is that we need some special
handling for the case where both the CPU and the GIC have TZ
support but we want to directly boot a kernel in NonSecure
(this only happens with the "virt" board). In this case our
hw/arm/boot.c minimalist bootloader needs to do the work of
configuring the GIC that would normally be done by Secure
firmware before it started the NS kernel. I've opted to do this
by adding a property on the GIC which says "reset yourself as
if you'd been configured by firmware to make all the IRQs NS",
and then set that property from boot.c if appropriate.

The other possible approach would be to have the code that
boot.c runs in the guest do this reconfiguration, but that
would be awkward because we'd need to start both the primary
and secondary CPUs in Secure and have them do the step-down-to-NS
themselves after messing with the GIC. If people strongly prefer
that I can have a go at it, but I'd really rather not :-)

Andreas, I cc'd you in case you wanted to have a look at whether
I got the property-which-can-be-set-after-realize right (patch 2).

thanks
-- PMM

Peter Maydell (5):
  hw/intc/arm_gic_common.c: Reset all registers
  hw/intc/arm_gic_common: Provide property to make IRQs reset as
    NonSecure
  hw/arm/boot: Configure secure GIC to make IRQs NS if booting an NS
    kernel
  hw/cpu/{a15mpcore,a9mpcore}: enable TrustZone in GIC if it is enabled
    in CPUs
  hw/arm/virt: Enable TZ extensions on the GIC if we are using them

 hw/arm/boot.c                    | 39 ++++++++++++++++++++++
 hw/arm/virt.c                    |  7 ++--
 hw/cpu/a15mpcore.c               | 13 ++++++++
 hw/cpu/a9mpcore.c                | 11 ++++++
 hw/intc/arm_gic_common.c         | 72 +++++++++++++++++++++++++++++++++++++---
 include/hw/intc/arm_gic_common.h |  1 +
 6 files changed, 137 insertions(+), 6 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/5] hw/intc/arm_gic_common.c: Reset all registers

[Peter Maydell]

The arm_gic_common reset function was missing reset code for
several of the GIC's state fields:
 * bpr[]
 * abpr[]
 * priority1[]
 * priority2[]
 * sgi_pending[]
 * irq_target[] (SMP configurations only)

These probably went unnoticed because most guests will either
never touch them, or will write to them in the process of
configuring the GIC before enabling interrupts.

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

diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index 044ad66..a64d071 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -123,7 +123,7 @@ static void arm_gic_common_realize(DeviceState *dev, Error **errp)
 static void arm_gic_common_reset(DeviceState *dev)
 {
     GICState *s = ARM_GIC_COMMON(dev);
-    int i;
+    int i, j;
     memset(s->irq_state, 0, GIC_MAXIRQ * sizeof(gic_irq_state));
     for (i = 0 ; i < s->num_cpu; i++) {
         if (s->revision == REV_11MPCORE) {
@@ -135,15 +135,30 @@ static void arm_gic_common_reset(DeviceState *dev)
         s->running_irq[i] = 1023;
         s->running_priority[i] = 0x100;
         s->cpu_ctlr[i] = 0;
+        s->bpr[i] = GIC_MIN_BPR;
+        s->abpr[i] = GIC_MIN_ABPR;
+        for (j = 0; j < GIC_INTERNAL; j++) {
+            s->priority1[j][i] = 0;
+        }
+        for (j = 0; j < GIC_NR_SGIS; j++) {
+            s->sgi_pending[j][i] = 0;
+        }
     }
     for (i = 0; i < GIC_NR_SGIS; i++) {
         GIC_SET_ENABLED(i, ALL_CPU_MASK);
         GIC_SET_EDGE_TRIGGER(i);
     }
-    if (s->num_cpu == 1) {
+
+    for (i = 0; i < ARRAY_SIZE(s->priority2); i++) {
+        s->priority2[i] = 0;
+    }
+
+    for (i = 0; i < GIC_MAXIRQ; i++) {
         /* For uniprocessor GICs all interrupts always target the sole CPU */
-        for (i = 0; i < GIC_MAXIRQ; i++) {
+        if (s->num_cpu == 1) {
             s->irq_target[i] = 1;
+        } else {
+            s->irq_target[i] = 0;
         }
     }
     s->ctlr = 0;
-- 
1.9.1

[Qemu-devel] [PATCH 2/5] hw/intc/arm_gic_common: Provide property to make IRQs reset as NonSecure

[Peter Maydell]

Implement a property which causes the GIC to reset with interrupts
configured into group 1 (NonSecure) rather than the usual group 0,
and with their initial priority set to the highest NonSecure priority
rather than the usual highest Secure priority.

This is necessary for the situation where we directly boot a kernel
in NonSecure on a system where the GIC supports the security extensions:
otherwise the kernel will be unable to access any of its interrupts.

The property is writable after realize, because we won't know whether
we need to set it until after machine creation is complete. If the
setting is changed it will take effect at the next reset.

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

diff --git a/hw/intc/arm_gic_common.c b/hw/intc/arm_gic_common.c
index a64d071..bc8b9c9 100644
--- a/hw/intc/arm_gic_common.c
+++ b/hw/intc/arm_gic_common.c
@@ -84,6 +84,33 @@ static const VMStateDescription vmstate_gic = {
     }
 };
 
+static bool arm_gic_irq_reset_nonsecure_get(Object *obj, Error **errp)
+{
+    GICState *s = ARM_GIC_COMMON(obj);
+
+    return s->irq_reset_nonsecure;
+}
+
+static void arm_gic_irq_reset_nonsecure_set(Object *obj, bool val, Error **errp)
+{
+    GICState *s = ARM_GIC_COMMON(obj);
+
+    s->irq_reset_nonsecure = val;
+}
+
+static void arm_gic_common_init(Object *obj)
+{
+    /* This property should be set true if the GIC should configure
+     * IRQs as group 1 (NS) on reset, rather than the usual group 0.
+     * It is essentially a standard bool property, except that it can be
+     * set after realize (and will take effect at the next reset).
+     */
+    object_property_add_bool(obj, "irqs-reset-nonsecure",
+                             arm_gic_irq_reset_nonsecure_get,
+                             arm_gic_irq_reset_nonsecure_set,
+                             &error_abort);
+}
+
 static void arm_gic_common_realize(DeviceState *dev, Error **errp)
 {
     GICState *s = ARM_GIC_COMMON(dev);
@@ -124,12 +151,27 @@ static void arm_gic_common_reset(DeviceState *dev)
 {
     GICState *s = ARM_GIC_COMMON(dev);
     int i, j;
+    int resetprio;
+
+    /* If we're resetting a TZ-aware GIC as if secure firmware
+     * had set it up ready to start a kernel in non-secure,
+     * we need to set interrupt priorities to a "zero for the
+     * NS view" value. This is particularly critical for the
+     * priority_mask[] values, because if they are zero then NS
+     * code cannot ever rewrite the priority to anything else.
+     */
+    if (s->security_extn && s->irq_reset_nonsecure) {
+        resetprio = 0x80;
+    } else {
+        resetprio = 0;
+    }
+
     memset(s->irq_state, 0, GIC_MAXIRQ * sizeof(gic_irq_state));
     for (i = 0 ; i < s->num_cpu; i++) {
         if (s->revision == REV_11MPCORE) {
             s->priority_mask[i] = 0xf0;
         } else {
-            s->priority_mask[i] = 0;
+            s->priority_mask[i] = resetprio;
         }
         s->current_pending[i] = 1023;
         s->running_irq[i] = 1023;
@@ -138,7 +180,7 @@ static void arm_gic_common_reset(DeviceState *dev)
         s->bpr[i] = GIC_MIN_BPR;
         s->abpr[i] = GIC_MIN_ABPR;
         for (j = 0; j < GIC_INTERNAL; j++) {
-            s->priority1[j][i] = 0;
+            s->priority1[j][i] = resetprio;
         }
         for (j = 0; j < GIC_NR_SGIS; j++) {
             s->sgi_pending[j][i] = 0;
@@ -150,7 +192,7 @@ static void arm_gic_common_reset(DeviceState *dev)
     }
 
     for (i = 0; i < ARRAY_SIZE(s->priority2); i++) {
-        s->priority2[i] = 0;
+        s->priority2[i] = resetprio;
     }
 
     for (i = 0; i < GIC_MAXIRQ; i++) {
@@ -161,6 +203,12 @@ static void arm_gic_common_reset(DeviceState *dev)
             s->irq_target[i] = 0;
         }
     }
+    if (s->security_extn && s->irq_reset_nonsecure) {
+        for (i = 0; i < GIC_MAXIRQ; i++) {
+            GIC_SET_GROUP(i, ALL_CPU_MASK);
+        }
+    }
+
     s->ctlr = 0;
 }
 
@@ -191,6 +239,7 @@ static const TypeInfo arm_gic_common_type = {
     .name = TYPE_ARM_GIC_COMMON,
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(GICState),
+    .instance_init = arm_gic_common_init,
     .class_size = sizeof(ARMGICCommonClass),
     .class_init = arm_gic_common_class_init,
     .abstract = true,
diff --git a/include/hw/intc/arm_gic_common.h b/include/hw/intc/arm_gic_common.h
index 899db3d..cfc1cce 100644
--- a/include/hw/intc/arm_gic_common.h
+++ b/include/hw/intc/arm_gic_common.h
@@ -118,6 +118,7 @@ typedef struct GICState {
     uint32_t num_irq;
     uint32_t revision;
     bool security_extn;
+    bool irq_reset_nonsecure; /* configure IRQs as group 1 (NS) on reset? */
     int dev_fd; /* kvm device fd if backed by kvm vgic support */
 } GICState;
 
-- 
1.9.1

[Qemu-devel] [PATCH 3/5] hw/arm/boot: Configure secure GIC to make IRQs NS if booting an NS kernel

[Peter Maydell]

If our builtin kernel bootloader is directly booting a kernel in
the NonSecure world, then we must configure the GIC to put all
the IRQs into the NonSecure group. (By default all interrupts
are configured to be Secure on reset, which means that a NonSecure
guest kernel cannot use any of them.) This job would usually
be done by the Secure boot firmware, but our builtin bootloader
is doing the job of firmware.

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

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 1e7fd28..3974499 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -13,6 +13,7 @@
 #include "sysemu/sysemu.h"
 #include "hw/boards.h"
 #include "hw/loader.h"
+#include "hw/intc/arm_gic_common.h"
 #include "elf.h"
 #include "sysemu/device_tree.h"
 #include "qemu/config-file.h"
@@ -557,6 +558,33 @@ static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
     fw_cfg_add_bytes(fw_cfg, data_key, data, size);
 }
 
+static int find_gics(Object *obj, void *opaque)
+{
+    GICState *gic = (GICState *)object_dynamic_cast(obj, TYPE_ARM_GIC_COMMON);
+    bool has_sec_extns;
+
+    if (!gic) {
+        /* Might be a container, traverse it for children */
+        return object_child_foreach(obj, find_gics, opaque);
+    }
+
+    has_sec_extns = object_property_get_bool(obj, "has-security-extensions",
+                                             &error_abort);
+    if (has_sec_extns) {
+        object_property_set_bool(obj, true, "irqs-reset-nonsecure",
+                                 &error_abort);
+    }
+    return 0;
+}
+
+static void reconfigure_gics_nonsecure(void)
+{
+    /* Find every GIC in the system and tell it to reconfigure
+     * itself with interrupts as NonSecure.
+     */
+    object_child_foreach(qdev_get_machine(), find_gics, NULL);
+}
+
 static void arm_load_kernel_notify(Notifier *notifier, void *data)
 {
     CPUState *cs;
@@ -767,6 +795,17 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
     }
     info->is_linux = is_linux;
 
+    if (is_linux && arm_feature(&cpu->env, ARM_FEATURE_EL3) &&
+        !info->secure_boot) {
+        /* We're directly booting a kernel into NonSecure. If the system
+         * has a GIC which implements the security extensions then we must
+         * configure it to have all the interrupts be NonSecure (this is
+         * a job that is done by the Secure boot firmware, and boot.c is
+         * a minimalist firmware-and-boot-loader equivalent).
+         */
+        reconfigure_gics_nonsecure();
+    }
+
     for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
         ARM_CPU(cs)->env.boot_info = info;
     }
-- 
1.9.1

[Qemu-devel] [PATCH 4/5] hw/cpu/{a15mpcore, a9mpcore}: enable TrustZone in GIC if it is enabled in CPUs

[Peter Maydell]

If the A9 and A15 CPUs which we're creating the peripherals for have
TrustZone (EL3) enabled, then also enable it in the GIC we create.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/cpu/a15mpcore.c | 13 +++++++++++++
 hw/cpu/a9mpcore.c  | 11 +++++++++++
 2 files changed, 24 insertions(+)

diff --git a/hw/cpu/a15mpcore.c b/hw/cpu/a15mpcore.c
index acc419e..e3814be 100644
--- a/hw/cpu/a15mpcore.c
+++ b/hw/cpu/a15mpcore.c
@@ -56,10 +56,23 @@ static void a15mp_priv_realize(DeviceState *dev, Error **errp)
     SysBusDevice *busdev;
     int i;
     Error *err = NULL;
+    bool has_el3;
+    Object *cpuobj;
 
     gicdev = DEVICE(&s->gic);
     qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
     qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
+
+    if (!kvm_irqchip_in_kernel()) {
+        /* Make the GIC's TZ support match the CPUs. We assume that
+         * either all the CPUs have TZ, or none do.
+         */
+        cpuobj = OBJECT(qemu_get_cpu(0));
+        has_el3 = object_property_find(cpuobj, "has_el3", &error_abort) &&
+            object_property_get_bool(cpuobj, "has_el3", &error_abort);
+        qdev_prop_set_bit(gicdev, "has-security-extensions", has_el3);
+    }
+
     object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
     if (err != NULL) {
         error_propagate(errp, err);
diff --git a/hw/cpu/a9mpcore.c b/hw/cpu/a9mpcore.c
index c09358c..7046246 100644
--- a/hw/cpu/a9mpcore.c
+++ b/hw/cpu/a9mpcore.c
@@ -49,6 +49,8 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
                  *wdtbusdev;
     Error *err = NULL;
     int i;
+    bool has_el3;
+    Object *cpuobj;
 
     scudev = DEVICE(&s->scu);
     qdev_prop_set_uint32(scudev, "num-cpu", s->num_cpu);
@@ -62,6 +64,15 @@ static void a9mp_priv_realize(DeviceState *dev, Error **errp)
     gicdev = DEVICE(&s->gic);
     qdev_prop_set_uint32(gicdev, "num-cpu", s->num_cpu);
     qdev_prop_set_uint32(gicdev, "num-irq", s->num_irq);
+
+    /* Make the GIC's TZ support match the CPUs. We assume that
+     * either all the CPUs have TZ, or none do.
+     */
+    cpuobj = OBJECT(qemu_get_cpu(0));
+    has_el3 = object_property_find(cpuobj, "has_el3", &error_abort) &&
+        object_property_get_bool(cpuobj, "has_el3", &error_abort);
+    qdev_prop_set_bit(gicdev, "has-security-extensions", has_el3);
+
     object_property_set_bool(OBJECT(&s->gic), true, "realized", &err);
     if (err != NULL) {
         error_propagate(errp, err);
-- 
1.9.1

[Qemu-devel] [PATCH 5/5] hw/arm/virt: Enable TZ extensions on the GIC if we are using them

[Peter Maydell]

If we're creating a board with support for TrustZone, then enable
it on the GIC model as well as on the CPUs.

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

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4846892..084a551 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -360,7 +360,7 @@ static void create_v2m(VirtBoardInfo *vbi, qemu_irq *pic)
     fdt_add_v2m_gic_node(vbi);
 }
 
-static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
+static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic, bool secure)
 {
     /* We create a standalone GIC v2 */
     DeviceState *gicdev;
@@ -379,6 +379,9 @@ static void create_gic(VirtBoardInfo *vbi, qemu_irq *pic)
      * interrupts; there are always 32 of the former (mandated by GIC spec).
      */
     qdev_prop_set_uint32(gicdev, "num-irq", NUM_IRQS + 32);
+    if (!kvm_irqchip_in_kernel()) {
+        qdev_prop_set_bit(gicdev, "has-security-extensions", secure);
+    }
     qdev_init_nofail(gicdev);
     gicbusdev = SYS_BUS_DEVICE(gicdev);
     sysbus_mmio_map(gicbusdev, 0, vbi->memmap[VIRT_GIC_DIST].base);
@@ -875,7 +878,7 @@ static void machvirt_init(MachineState *machine)
 
     create_flash(vbi);
 
-    create_gic(vbi, pic);
+    create_gic(vbi, pic, vms->secure);
 
     create_uart(vbi, pic);
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 3/5] hw/arm/boot: Configure secure GIC to make IRQs NS if booting an NS kernel

[Peter Crosthwaite]

On Tue, Jun 30, 2015 at 6:07 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> If our builtin kernel bootloader is directly booting a kernel in
> the NonSecure world, then we must configure the GIC to put all
> the IRQs into the NonSecure group. (By default all interrupts
> are configured to be Secure on reset, which means that a NonSecure
> guest kernel cannot use any of them.) This job would usually
> be done by the Secure boot firmware, but our builtin bootloader
> is doing the job of firmware.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/boot.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 1e7fd28..3974499 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -13,6 +13,7 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/boards.h"
>  #include "hw/loader.h"
> +#include "hw/intc/arm_gic_common.h"
>  #include "elf.h"
>  #include "sysemu/device_tree.h"
>  #include "qemu/config-file.h"
> @@ -557,6 +558,33 @@ static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
>      fw_cfg_add_bytes(fw_cfg, data_key, data, size);
>  }
>
> +static int find_gics(Object *obj, void *opaque)
> +{
> +    GICState *gic = (GICState *)object_dynamic_cast(obj, TYPE_ARM_GIC_COMMON);
> +    bool has_sec_extns;
> +
> +    if (!gic) {
> +        /* Might be a container, traverse it for children */
> +        return object_child_foreach(obj, find_gics, opaque);
> +    }

This need to traverse the whole tree has come up more than once for
me, so I think this should be a core feature of QOM.

> +
> +    has_sec_extns = object_property_get_bool(obj, "has-security-extensions",
> +                                             &error_abort);
> +    if (has_sec_extns) {
> +        object_property_set_bool(obj, true, "irqs-reset-nonsecure",
> +                                 &error_abort);
> +    }
> +    return 0;
> +}
> +
> +static void reconfigure_gics_nonsecure(void)
> +{
> +    /* Find every GIC in the system and tell it to reconfigure
> +     * itself with interrupts as NonSecure.
> +     */
> +    object_child_foreach(qdev_get_machine(), find_gics, NULL);
> +}
> +
>  static void arm_load_kernel_notify(Notifier *notifier, void *data)
>  {
>      CPUState *cs;
> @@ -767,6 +795,17 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>      }
>      info->is_linux = is_linux;
>
> +    if (is_linux && arm_feature(&cpu->env, ARM_FEATURE_EL3) &&

Do we need to conditional on ARM_FEATURE_EL3 or can we make GIC logic
independent of the primary CPU?



> +        !info->secure_boot) {
> +        /* We're directly booting a kernel into NonSecure. If the system
> +         * has a GIC which implements the security extensions then we must
> +         * configure it to have all the interrupts be NonSecure (this is
> +         * a job that is done by the Secure boot firmware, and boot.c is
> +         * a minimalist firmware-and-boot-loader equivalent).
> +         */

So I actually had my own patches for this one that went in a different
direction. The reason is, there are also other devs out there which
need post-firmware state setup. The one I an thinking of mainly is the
Zynq SLCR setup for non-firmware boots, I.E. the Linux kernel expects
firmware to setup devices to some sort of initialized state (mainly
turning clocks on). I think this GIC setup falls in the same category.
The third use case is the GIC_CPU_IF stuff currently managed by
machine code in boot.c that could be outsourced to GIC (in either a
similar way to what is done here, or more fully outsourced using my
new API).

So with these three use cases of initing devices in post-firmware
state, I created a little interface to allow devices to generically
specify their post linux state. Patches on list overnight. I'll write
more in the cover.

Regards,
Peter

> +        reconfigure_gics_nonsecure();
> +    }
> +
>      for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
>          ARM_CPU(cs)->env.boot_info = info;
>      }
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH 3/5] hw/arm/boot: Configure secure GIC to make IRQs NS if booting an NS kernel

[Peter Maydell]

On 30 June 2015 at 20:01, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Jun 30, 2015 at 6:07 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> If our builtin kernel bootloader is directly booting a kernel in
>> the NonSecure world, then we must configure the GIC to put all
>> the IRQs into the NonSecure group. (By default all interrupts
>> are configured to be Secure on reset, which means that a NonSecure
>> guest kernel cannot use any of them.) This job would usually
>> be done by the Secure boot firmware, but our builtin bootloader
>> is doing the job of firmware.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/arm/boot.c | 39 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 1e7fd28..3974499 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -13,6 +13,7 @@
>>  #include "sysemu/sysemu.h"
>>  #include "hw/boards.h"
>>  #include "hw/loader.h"
>> +#include "hw/intc/arm_gic_common.h"
>>  #include "elf.h"
>>  #include "sysemu/device_tree.h"
>>  #include "qemu/config-file.h"
>> @@ -557,6 +558,33 @@ static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
>>      fw_cfg_add_bytes(fw_cfg, data_key, data, size);
>>  }
>>
>> +static int find_gics(Object *obj, void *opaque)
>> +{
>> +    GICState *gic = (GICState *)object_dynamic_cast(obj, TYPE_ARM_GIC_COMMON);
>> +    bool has_sec_extns;
>> +
>> +    if (!gic) {
>> +        /* Might be a container, traverse it for children */
>> +        return object_child_foreach(obj, find_gics, opaque);
>> +    }
>
> This need to traverse the whole tree has come up more than once for
> me, so I think this should be a core feature of QOM.

I did think about that, yeah. I guess something like
object_descendants_foreach(), same signature as
object_child_foreach(), same semantics except it also
iterates through the whole tree (and calls the callback
for the nodes-with-children as well as the leaves) ?

>> +
>> +    has_sec_extns = object_property_get_bool(obj, "has-security-extensions",
>> +                                             &error_abort);
>> +    if (has_sec_extns) {
>> +        object_property_set_bool(obj, true, "irqs-reset-nonsecure",
>> +                                 &error_abort);
>> +    }
>> +    return 0;
>> +}
>> +
>> +static void reconfigure_gics_nonsecure(void)
>> +{
>> +    /* Find every GIC in the system and tell it to reconfigure
>> +     * itself with interrupts as NonSecure.
>> +     */
>> +    object_child_foreach(qdev_get_machine(), find_gics, NULL);
>> +}
>> +
>>  static void arm_load_kernel_notify(Notifier *notifier, void *data)
>>  {
>>      CPUState *cs;
>> @@ -767,6 +795,17 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>>      }
>>      info->is_linux = is_linux;
>>
>> +    if (is_linux && arm_feature(&cpu->env, ARM_FEATURE_EL3) &&
>
> Do we need to conditional on ARM_FEATURE_EL3 or can we make GIC logic
> independent of the primary CPU?

The point here is that "do we need to do this" is exactly
dependent on what we're doing with the CPU. Only if we
want to put the guest into NS do we do this, and the
condition for "are we going to put the guest into NS"
is "is this a Linux boot on a CPU with EL3 but where
the board says don't boot in S". It matches what the
existing logic does for when it sets the SCR_NS bit in
do_cpu_reset() in this file.

>> +        !info->secure_boot) {
>> +        /* We're directly booting a kernel into NonSecure. If the system
>> +         * has a GIC which implements the security extensions then we must
>> +         * configure it to have all the interrupts be NonSecure (this is
>> +         * a job that is done by the Secure boot firmware, and boot.c is
>> +         * a minimalist firmware-and-boot-loader equivalent).
>> +         */
>
> So I actually had my own patches for this one that went in a different
> direction. The reason is, there are also other devs out there which
> need post-firmware state setup. The one I an thinking of mainly is the
> Zynq SLCR setup for non-firmware boots, I.E. the Linux kernel expects
> firmware to setup devices to some sort of initialized state (mainly
> turning clocks on). I think this GIC setup falls in the same category.
> The third use case is the GIC_CPU_IF stuff currently managed by
> machine code in boot.c that could be outsourced to GIC (in either a
> similar way to what is done here, or more fully outsourced using my
> new API).

I'm a bit torn here -- I don't want to make it *too* easy for
people to add linux-booting specific code to random devices,
as this will lead to the bootloader code having its tentacles
everywhere within the tree...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/arm/boot: Configure secure GIC to make IRQs NS if booting an NS kernel

[Peter Crosthwaite]

On Tue, Jun 30, 2015 at 12:42 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 30 June 2015 at 20:01, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Tue, Jun 30, 2015 at 6:07 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> If our builtin kernel bootloader is directly booting a kernel in
>>> the NonSecure world, then we must configure the GIC to put all
>>> the IRQs into the NonSecure group. (By default all interrupts
>>> are configured to be Secure on reset, which means that a NonSecure
>>> guest kernel cannot use any of them.) This job would usually
>>> be done by the Secure boot firmware, but our builtin bootloader
>>> is doing the job of firmware.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>  hw/arm/boot.c | 39 +++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 39 insertions(+)
>>>
>>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>>> index 1e7fd28..3974499 100644
>>> --- a/hw/arm/boot.c
>>> +++ b/hw/arm/boot.c
>>> @@ -13,6 +13,7 @@
>>>  #include "sysemu/sysemu.h"
>>>  #include "hw/boards.h"
>>>  #include "hw/loader.h"
>>> +#include "hw/intc/arm_gic_common.h"
>>>  #include "elf.h"
>>>  #include "sysemu/device_tree.h"
>>>  #include "qemu/config-file.h"
>>> @@ -557,6 +558,33 @@ static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
>>>      fw_cfg_add_bytes(fw_cfg, data_key, data, size);
>>>  }
>>>
>>> +static int find_gics(Object *obj, void *opaque)
>>> +{
>>> +    GICState *gic = (GICState *)object_dynamic_cast(obj, TYPE_ARM_GIC_COMMON);
>>> +    bool has_sec_extns;
>>> +
>>> +    if (!gic) {
>>> +        /* Might be a container, traverse it for children */
>>> +        return object_child_foreach(obj, find_gics, opaque);
>>> +    }
>>
>> This need to traverse the whole tree has come up more than once for
>> me, so I think this should be a core feature of QOM.
>
> I did think about that, yeah. I guess something like
> object_descendants_foreach(), same signature as
> object_child_foreach(), same semantics except it also
> iterates through the whole tree (and calls the callback
> for the nodes-with-children as well as the leaves) ?
>
>>> +
>>> +    has_sec_extns = object_property_get_bool(obj, "has-security-extensions",
>>> +                                             &error_abort);
>>> +    if (has_sec_extns) {
>>> +        object_property_set_bool(obj, true, "irqs-reset-nonsecure",
>>> +                                 &error_abort);
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +static void reconfigure_gics_nonsecure(void)
>>> +{
>>> +    /* Find every GIC in the system and tell it to reconfigure
>>> +     * itself with interrupts as NonSecure.
>>> +     */
>>> +    object_child_foreach(qdev_get_machine(), find_gics, NULL);
>>> +}
>>> +
>>>  static void arm_load_kernel_notify(Notifier *notifier, void *data)
>>>  {
>>>      CPUState *cs;
>>> @@ -767,6 +795,17 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>>>      }
>>>      info->is_linux = is_linux;
>>>
>>> +    if (is_linux && arm_feature(&cpu->env, ARM_FEATURE_EL3) &&
>>
>> Do we need to conditional on ARM_FEATURE_EL3 or can we make GIC logic
>> independent of the primary CPU?
>
> The point here is that "do we need to do this" is exactly
> dependent on what we're doing with the CPU. Only if we
> want to put the guest into NS do we do this, and the
> condition for "are we going to put the guest into NS"
> is "is this a Linux boot on a CPU with EL3 but where
> the board says don't boot in S". It matches what the
> existing logic does for when it sets the SCR_NS bit in
> do_cpu_reset() in this file.
>

Then maybe this belongs on the lowest common denominator for GIC and
CPU - the SoC level. SoCs can register a linux_init function that
checks the CPU and GIC NS support and does the switchup. Can make it a
common function that multiple socs/machines can call (virt, vexpress
and zynq-mp so far I think). For cases where the linux_init in
genuinely self contained (like my zynq SLCR case), the device can
register it.

>>> +        !info->secure_boot) {
>>> +        /* We're directly booting a kernel into NonSecure. If the system
>>> +         * has a GIC which implements the security extensions then we must
>>> +         * configure it to have all the interrupts be NonSecure (this is
>>> +         * a job that is done by the Secure boot firmware, and boot.c is
>>> +         * a minimalist firmware-and-boot-loader equivalent).
>>> +         */
>>
>> So I actually had my own patches for this one that went in a different
>> direction. The reason is, there are also other devs out there which
>> need post-firmware state setup. The one I an thinking of mainly is the
>> Zynq SLCR setup for non-firmware boots, I.E. the Linux kernel expects
>> firmware to setup devices to some sort of initialized state (mainly
>> turning clocks on). I think this GIC setup falls in the same category.
>> The third use case is the GIC_CPU_IF stuff currently managed by
>> machine code in boot.c that could be outsourced to GIC (in either a
>> similar way to what is done here, or more fully outsourced using my
>> new API).
>
> I'm a bit torn here -- I don't want to make it *too* easy for
> people to add linux-booting specific code to random devices,
> as this will lead to the bootloader code having its tentacles
> everywhere within the tree...
>

Maybe the compromise to to restrict this API to SoCs and machines and
I can handle my SLCR case with a single "post-firmware" boolean
property?

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH 3/5] hw/arm/boot: Configure secure GIC to make IRQs NS if booting an NS kernel

[Peter Maydell]

On 30 June 2015 at 21:10, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Jun 30, 2015 at 12:42 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> The point here is that "do we need to do this" is exactly
>> dependent on what we're doing with the CPU. Only if we
>> want to put the guest into NS do we do this, and the
>> condition for "are we going to put the guest into NS"
>> is "is this a Linux boot on a CPU with EL3 but where
>> the board says don't boot in S". It matches what the
>> existing logic does for when it sets the SCR_NS bit in
>> do_cpu_reset() in this file.
>>
>
> Then maybe this belongs on the lowest common denominator for GIC and
> CPU - the SoC level. SoCs can register a linux_init function that
> checks the CPU and GIC NS support and does the switchup.

The one board we have so far that needs this code doesn't
have an SoC level object -- virt just creates the CPU and
GIC itself...

-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/arm/boot: Configure secure GIC to make IRQs NS if booting an NS kernel

[Peter Crosthwaite]

On Tue, Jun 30, 2015 at 1:16 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 June 2015 at 21:10, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Tue, Jun 30, 2015 at 12:42 PM, Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>> The point here is that "do we need to do this" is exactly
>>> dependent on what we're doing with the CPU. Only if we
>>> want to put the guest into NS do we do this, and the
>>> condition for "are we going to put the guest into NS"
>>> is "is this a Linux boot on a CPU with EL3 but where
>>> the board says don't boot in S". It matches what the
>>> existing logic does for when it sets the SCR_NS bit in
>>> do_cpu_reset() in this file.
>>>
>>
>> Then maybe this belongs on the lowest common denominator for GIC and
>> CPU - the SoC level. SoCs can register a linux_init function that
>> checks the CPU and GIC NS support and does the switchup.
>
> The one board we have so far that needs this code doesn't
> have an SoC level object -- virt just creates the CPU and
> GIC itself...

Right, but machines are QOMified objects now, so they should be able
to implement this interface.

zynqmp SoC needs this feature for when we switch on EL3.

Regards,
Peter

>
> -- PMM
>

Re: [Qemu-devel] [PATCH 3/5] hw/arm/boot: Configure secure GIC to make IRQs NS if booting an NS kernel

[Peter Maydell]

On 30 June 2015 at 21:10, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Jun 30, 2015 at 12:42 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> On 30 June 2015 at 20:01, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> So I actually had my own patches for this one that went in a different
>>> direction. The reason is, there are also other devs out there which
>>> need post-firmware state setup. The one I an thinking of mainly is the
>>> Zynq SLCR setup for non-firmware boots, I.E. the Linux kernel expects
>>> firmware to setup devices to some sort of initialized state (mainly
>>> turning clocks on). I think this GIC setup falls in the same category.
>>> The third use case is the GIC_CPU_IF stuff currently managed by
>>> machine code in boot.c that could be outsourced to GIC (in either a
>>> similar way to what is done here, or more fully outsourced using my
>>> new API).
>>
>> I'm a bit torn here -- I don't want to make it *too* easy for
>> people to add linux-booting specific code to random devices,
>> as this will lead to the bootloader code having its tentacles
>> everywhere within the tree...

So I thought about this a bit, and I guess that having a general
interface is probably better than specifically setting a GIC
property. It does need to be called once at arm_kernel_load_notify
time, not as a reset hook. I also think it should pass in the
arm_boot_info* as a parameter. This would let the GIC do the
right thing based on whether we're booting S or NS.

Are you planning to respin this patchset, or should I just pull
the relevant bits into my series?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/5] hw/arm/boot: Configure secure GIC to make IRQs NS if booting an NS kernel

[Peter Crosthwaite]

On Thu, Jul 2, 2015 at 5:41 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 June 2015 at 21:10, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Tue, Jun 30, 2015 at 12:42 PM, Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>> On 30 June 2015 at 20:01, Peter Crosthwaite
>>> <peter.crosthwaite@xilinx.com> wrote:
>>>> So I actually had my own patches for this one that went in a different
>>>> direction. The reason is, there are also other devs out there which
>>>> need post-firmware state setup. The one I an thinking of mainly is the
>>>> Zynq SLCR setup for non-firmware boots, I.E. the Linux kernel expects
>>>> firmware to setup devices to some sort of initialized state (mainly
>>>> turning clocks on). I think this GIC setup falls in the same category.
>>>> The third use case is the GIC_CPU_IF stuff currently managed by
>>>> machine code in boot.c that could be outsourced to GIC (in either a
>>>> similar way to what is done here, or more fully outsourced using my
>>>> new API).
>>>
>>> I'm a bit torn here -- I don't want to make it *too* easy for
>>> people to add linux-booting specific code to random devices,
>>> as this will lead to the bootloader code having its tentacles
>>> everywhere within the tree...
>
> So I thought about this a bit, and I guess that having a general
> interface is probably better than specifically setting a GIC
> property. It does need to be called once at arm_kernel_load_notify
> time, not as a reset hook. I also think it should pass in the
> arm_boot_info* as a parameter. This would let the GIC do the
> right thing based on whether we're booting S or NS.
>
> Are you planning to respin this patchset, or should I just pull
> the relevant bits into my series?
>

Just pull relevants I think. Should be just patches 1 and 2, the
boot.c one needs rewrite and the gic one is a write-off as yours is
much more functionally correct.

Regards,
Peter

> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH 0/5] arm: enable TZ support for the GIC

[Peter Maydell]

On 30 June 2015 at 14:07, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset enables the TZ support in the GIC for the systems
> where we enable TZ support in the CPU. In practice that means
> just the "virt" and "vexpress" boards, since all the others
> disable the CPU TZ support.

Further playing about with this has raised a couple of
interesting issues:
 (1) we forgot to actually implement the secure physical timer
(and we need to fix up the wiring bodge we do in the boards
where we assume the physical timer is the NS phys timer)
 (2) the UEFI firmware for the virt board assumes it is
not running in a Secure environment, so is likely to
go wrong if we present it with a TZ-aware CPU and GIC.
We probably need to default to no-TZ.

(This also sort of links in with the wider question of
how we tell firmware running on 'virt' about system
configurations where some devices are S only.)

-- PMM