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
<address@hidden> wrote:
> On 30 June 2015 at 20:01, Peter Crosthwaite
> <address@hidden> wrote:
>> 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.
>
> 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
>