Re: [Qemu-arm] [PATCH v2 4/5] arm: boot: Add secure_board_setup flag

[Peter Crosthwaite]

On Fri, Oct 30, 2015 at 1:49 PM, Peter Maydell <address@hidden> wrote:
> On 30 October 2015 at 05:34, Peter Crosthwaite
> <address@hidden> wrote:
>> Add a flag that when set, will cause the primary CPU to start in secure
>> mode, even if the overall boot in non-secure. This is useful for when
>
> "is non-secure".
>
>> there is a board-setup blob that needs to run from secure mode, but
>> device and secondary CPU init should still be done as-normal for a non-
>> secure boot.
>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> ---
>>
>>  hw/arm/boot.c        | 3 ++-
>>  include/hw/arm/arm.h | 6 ++++++
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index b0879a5..6680d45 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -495,7 +495,8 @@ static void do_cpu_reset(void *opaque)
>>                  }
>>
>>                  /* Set to non-secure if not a secure boot */
>> -                if (!info->secure_boot) {
>> +                if (!info->secure_boot &&
>> +                    (cs != first_cpu || !info->secure_board_setup)) {
>>                      /* Linux expects non-secure state */
>>                      env->cp15.scr_el3 |= SCR_NS;
>>                  }
>> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
>> index 9217b70..60dc919 100644
>> --- a/include/hw/arm/arm.h
>> +++ b/include/hw/arm/arm.h
>> @@ -97,6 +97,12 @@ struct arm_boot_info {
>>      hwaddr board_setup_addr;
>>      void (*write_board_setup)(ARMCPU *cpu,
>>                                const struct arm_boot_info *info);
>> +
>> +    /* If set, the board specific loader/setup blob will be run from secure
>> +     * mode, regardless of secure_boot. The blob becomes responsible for
>> +     * changing to non-secure state if implementing a non-secure boot
>> +     */
>> +    bool secure_board_setup;
>>  };
>
> I thought you were planning to have the generic code do the
> S->NS transition; but I guess it works better in the board
> code (we have to go up into Monitor and back down again, right?)
>

Yes I had to change my mind on this one. The issue was that ARM arch
doesn't guarantee a NS switch by simply modding SCR.NS inline and I
wanted to follow this convention. The recommended way is via eret
(presumably from monitor mode?). So to implement this for highbank I
do a dummy SMC after the SCR.NS switch (from secure EL1). This can't
be done generically as board-setup may or may-not install a functional
monitor.

> Is it an error for the board to set secure_board_setup if
> the CPU doesn't have EL3? (if so, worth mentioning in this
> comment; maybe assert?)
>

I don't like assert, as has_el3 is in theory is user modifiable (via
either -cpu transplants or directly via -global). I think it is an
error_exit().

Regards,
Peter

> thanks
> -- PMM