Re: [PATCH] hw/arm/boot: Set NSACR.{CP11, CP10} in dummy SMC setup routine

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

On Mon, 4 Nov 2019 at 15:12, Clement Deschamps
<clement.deschamps@greensocs.com> wrote:
>
> Set the NSACR CP11 and CP10 bits, to allow FPU access in Non-Secure state
> when using dummy SMC setup routine. Otherwise an AArch32 kernel will UNDEF as
> soon as it tries to use the FPU.
>
> This fixes kernel panic when booting raspbian on raspi2.
>
> Successfully tested with:
>   2017-01-11-raspbian-jessie-lite.img
>   2018-11-13-raspbian-stretch-lite.img
>   2019-07-10-raspbian-buster-lite.img
>
> See also commit ece628fcf6 that fixes the issue when *not* using the
> dummy SMC setup routine.
>
> Fixes: fc1120a7f5
> Signed-off-by: Clement Deschamps <clement.deschamps@greensocs.com>
> ---
>  hw/arm/boot.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index ef6724960c..8fb4a63606 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -240,6 +240,9 @@ void arm_write_secure_board_setup_dummy_smc(ARMCPU *cpu,
>      };
>      uint32_t board_setup_blob[] = {
>          /* board setup addr */
> +        0xee110f51, /* mrc     p15, 0, r0, c1, c1, 2  ;read NSACR */
> +        0xe3800b03, /* orr     r0, #0xc00             ;set CP11, CP10 */
> +        0xee010f51, /* mcr     p15, 0, r0, c1, c1, 2  ;write NSACR */
>          0xe3a00e00 + (mvbar_addr >> 4), /* mov r0, #mvbar_addr */
>          0xee0c0f30, /* mcr     p15, 0, r0, c12, c0, 1 ;set MVBAR */
>          0xee110f11, /* mrc     p15, 0, r0, c1 , c1, 0 ;read SCR */
> --
> 2.23.0

Yep. The boot.c logic sets NSACR.{CP11,CP10} for booting a
Linux kernel unless (a) the board says we should boot the
kernel in Secure mode or (b) this is the primary CPU and
the board requires the board_setup code to run in Secure mode.
In that case the board_setup code must set NSACR in the same
way that it is responsible for dropping down to NS.

This affects highbank and raspi, which are the only boards
that set info->secure_board_setup; both of those use the
arm_write_secure_board_setup_dummy_smc(), so this fix is
all we should need.

I plan to fold in this comment update:

diff --git a/include/hw/arm/boot.h b/include/hw/arm/boot.h
index 7f4d0ca7cdf..ce2b48b88bc 100644
--- a/include/hw/arm/boot.h
+++ b/include/hw/arm/boot.h
@@ -107,9 +107,12 @@ struct arm_boot_info {
     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
+    /*
+     * 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
+     * changing to non-secure state if implementing a non-secure boot,
+     * including setting up EL3/Secure registers such as the NSACR as
+     * required by the Linux booting ABI before the switch to non-secure.
      */
     bool secure_board_setup;

and tweak the commit message to mention highbank, when I
apply it to target-arm.next.

thanks
-- PMM