ARM: Highbank: Add monitor support

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 <[email protected]>

---
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(-)

hw/arm/boot.c +2 -1

index b0879a5..f671454 100644
@@ -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;
                 }

hw/arm/boot.c +1 -0

+
     /* Load the kernel.  */
     if (!info->kernel_filename || info->firmware_loaded) {
 

include/hw/arm/arm.h +0 -0

index 9217b70..60dc919 100644

Quoted 33 lines · Peter Crosthwaite

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 <[email protected]>

---
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(-)

hw/arm/boot.c +2 -1

index b0879a5..f671454 100644
@@ -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);
 

hw/arm/boot.c static void arm_load_kernel_notify(Notifier *notifier, void *data) · line 604

Quoted 3 lines

hw/arm/boot.c +3 -0

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

hw/arm/boot.c static void arm_load_kernel_notify(Notifier *notifier, void *data) · line 605

Quoted 1 line

hw/arm/boot.c +1 -0

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

Quoted 9 lines

hw/arm/boot.c +1 -0

+
     /* Load the kernel.  */
     if (!info->kernel_filename || info->firmware_loaded) {
 

include/hw/arm/arm.h +0 -0

index 9217b70..60dc919 100644

include/hw/arm/arm.h struct arm_boot_info { · line 106

Quoted 10 lines

include/hw/arm/arm.h +6 -0

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

This board should not support CPU model override. This allows for easier patching of the board with being able to rely on the CPU type being correct.

Signed-off-by Peter Crosthwaite <[email protected]>

---

Firstly, enable monitor mode and PSCI, both are which are features of

this board.

In addition to PSCI, this board also uses SMC for cache maintainence ops. This means we need a secure monitor to catch these and nop them. Use the ARM boot board-setup feature to implement this. All traps to monitor mode implement the nop.

As a KVM CPU cannot run in secure mode, do not do the board-setup if not running TCG. Report a warning explaining the limitation is this case.

Quoted 1 line · Peter Crosthwaite

Firstly, enable monitor mode and PSCI, both are which are features of

"both of which"

Quoted 7 lines

this board.

In addition to PSCI, this board also uses SMC for cache maintainence ops. This means we need a secure monitor to catch these and nop them. Use the ARM boot board-setup feature to implement this. All traps to monitor mode implement the nop.

Quoted 3 lines

As a KVM CPU cannot run in secure mode, do not do the board-setup if not running TCG. Report a warning explaining the limitation is this case.

"in this case".

hw/arm/highbank.c static void calxeda_init(MachineState *machine, enum cxmachines machine_id) · line 413

Quoted 6 lines

hw/arm/highbank.c +1 -0

@@ -371,6 +410,16 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
     highbank_binfo.loader_start = 0;
     highbank_binfo.write_secondary_boot = hb_write_secondary;
     highbank_binfo.secondary_cpu_reset_hook = hb_reset_secondary;
+    if (tcg_enabled()) {

This also needs to be !kvm_enabled(), so 'make check' works.

hw/arm/highbank.c static void calxeda_init(MachineState *machine, enum cxmachines machine_id) · line 420

Quoted 7 lines

hw/arm/highbank.c +7 -0

+        highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
+        highbank_binfo.write_board_setup = hb_write_board_setup;
+        highbank_binfo.secure_board_setup = true;
+    } else {
+        error_report("WARNING: TCG unavailable - "
+                     "unable to load built-in Monitor support.\n"
+                     "Some guests (such as Linux) may not boot\n");

You can't have newlines in an error_report() string. I suggest

error_report("WARNING: cannot load built-in Monitor support if KVM "
"is enabled. Some guests (such as Linux) may not boot.");

Otherwise

Reviewed-by Peter Maydell <[email protected]>

thanks
-- PMM