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

[Peter Crosthwaite]

On Fri, Nov 6, 2015 at 6:45 AM, Peter Maydell <address@hidden> wrote:
>
> On 3 November 2015 at 04:30, 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 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 <address@hidden>
> > ---
> > 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.
>      */
>

Taken verbatim. Thanks.

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

Fixed. Changed to De Morgan && equivalent with the two !s for easier reading:

assert(!(info->secure_board_setup && kvm_enabled()));

Regards,
Peter

> > +
> >      /* 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