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

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