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 <address@hidden> 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 <address@hidden>
> ---
>  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
>
>