Re: [Qemu-devel] [PATCH for-2.5 v3 1/3] arm: boot: Add secure_board_setup flag

[Peter Maydell <peter.maydell@linaro.org>]

On 3 November 2015 at 04:30, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> Add a flag that when set, will cause the primary CPU to start in secure
> mode, even if the overall boot is non-secure. This is useful for when
> 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 <crosthwaite.peter@gmail.com>
> ---
> changed since v2:
> Assert if running KVM and board_setup_secure is set
>
>  hw/arm/boot.c        | 8 +++++++-
>  include/hw/arm/arm.h | 6 ++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index b0879a5..f671454 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;
>                  }
> @@ -598,6 +599,11 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>      struct arm_boot_info *info =
>          container_of(n, struct arm_boot_info, load_kernel_notifier);
>
> +    /* It is the boards job to make sure secure_board_setup is actually
> +     * possible
> +     */

I think we could improve this comment a bit:
    /* The board code is not supposed to set secure_board_setup unless
     * running its code in secure mode is actually possible, and KVM
     * doesn't support secure.
     */

> +    assert(!info->secure_board_setup || tcg_enabled());

This assertion causes us to fail "make check", because there's
a test target which starts every board with the "qtest" accelerator,
which is not TCG. You need !kvm_enabled() instead (and
to include sysemu/kvm.h).

> +
>      /* Load the kernel.  */
>      if (!info->kernel_filename || info->firmware_loaded) {
>
> 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;
>  };

thanks
-- PMM