ARM: Machine specific boot blobs

These comment start immediately after the current longest name in the list. Tab them out to the next tab stop to give a little breathing room and prepare for FIXUP_BOARD_SETUP which will require more indent.

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

---

 hw/arm/boot.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Add an API for boards to inject their own preboot software (or firmware) seqeuence.

The software then returns to bootloader via the link register. This allows boards to do their own little bits of firmware setup without needed to replace the bootloader completely (which is the requirement for existing firmware support).

The blob is loaded by a callback if and only if doing a linux boot (similar to the existing write_secondary support).

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

---
Changed since RFC:
Load blob via firmware.
Remove un-needed 0-word in bootloader sequence.
Remove "blob", just use "board setup" consistently
Remove boolean for (just use a pointer NULL check on write_board_setup) Adjust comment about functionality of primary bootloader

 hw/arm/boot.c        | 17 ++++++++++++++++-
 include/hw/arm/arm.h | 10 ++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

hw/arm/boot.c +1 -0

index 2a151e2..5640989 100644
@@ -31,6 +31,7 @@ typedef enum {
     FIXUP_NONE = 0,     /* do nothing */
     FIXUP_TERMINATOR,   /* end of insns */
     FIXUP_BOARDID,      /* overwrite with board ID number */
+    FIXUP_BOARD_SETUP,  /* overwrite with board specific setup code address */
     FIXUP_ARGPTR,       /* overwrite with pointer to kernel args */
     FIXUP_ENTRYPOINT,   /* overwrite with kernel entry point */
     FIXUP_GIC_CPU_IF,   /* overwrite with GIC CPU interface address */

hw/arm/boot.c +12 -0

 static const ARMInsnFixup bootloader[] = {
+    { 0xe28fe008 }, /* add     lr, pc, #8 */
+    { 0xe51ff004 }, /* ldr     pc, [pc, #-4] */
+    { 0, FIXUP_BOARD_SETUP },
+#define BOOTLOADER_NO_BOARD_SETUP_OFFSET 3
     { 0xe3a00000 }, /* mov     r0, #0 */
     { 0xe59f1004 }, /* ldr     r1, [pc, #4] */
     { 0xe59f2004 }, /* ldr     r2, [pc, #4] */

@@ -131,6 +138,7 @@ static void write_bootloader(const char *name, hwaddr addr,
         case FIXUP_NONE:
             break;
         case FIXUP_BOARDID:
+        case FIXUP_BOARD_SETUP:
         case FIXUP_ARGPTR:
         case FIXUP_ENTRYPOINT:
         case FIXUP_GIC_CPU_IF:

@@ -640,6 +648,9 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
         elf_machine = EM_AARCH64;
     } else {
         primary_loader = bootloader;
+        if (!info->write_board_setup) {
+            primary_loader += BOOTLOADER_NO_BOARD_SETUP_OFFSET;
+        }
         kernel_load_offset = KERNEL_LOAD_ADDR;
         elf_machine = EM_ARM;
     }

@@ -745,6 +756,7 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
         info->initrd_size = initrd_size;
 
         fixupcontext[FIXUP_BOARDID] = info->board_id;
+        fixupcontext[FIXUP_BOARD_SETUP] = info->board_setup_addr;
 
         /* for device tree boot, we pass the DTB directly in r2. Otherwise
          * we point to the kernel args.

@@ -793,6 +805,9 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
         if (info->nb_cpus > 1) {
             info->write_secondary_boot(cpu, info);
         }
+        if (info->write_board_setup) {
+            info->write_board_setup(cpu, info);
+        }
 
         /* Notify devices which need to fake up firmware initialization
          * that we're doing a direct kernel boot.

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

index 4dcd4f9..128a54e 100644

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

+     * defining write_board_setup, which is responsible for loading the blob
+     * to the specified address.
+     */
+    hwaddr board_setup_addr;
+    void (*write_board_setup)(ARMCPU *cpu,
+                              const struct arm_boot_info *info);
 };
 
 /**
--

1.9.1

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.

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

---
Changed since RFC:
Use bootloader callback to load blob. Change "firmware" to "board-setup" for consistency. Tweak commit message.

 hw/arm/highbank.c | 57 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 46 insertions(+), 11 deletions(-)

hw/arm/highbank.c +0 -0

index be04b27..98daba0 100644

hw/arm/highbank.c +1 -0

+
 #define SMP_BOOT_ADDR           0x100
 #define SMP_BOOT_REG            0x40
 #define MPCORE_PERIPHBASE       0xfff10000

@@ -36,6 +39,38 @@
 
 /* Board init.  */

Add a Linux-specific pre-boot routine that matches the device specific bootloaders behaviour. This is needed for modern Linux that expects the ARM PLL in SLCR to be a more even value (not 26).

Cc Alistair Francis <[email protected]>

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

---
Changed since RFC:
Use bootloader callback to load blob. Change "firmware" to "board-setup" for consistency.

 hw/arm/xilinx_zynq.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

hw/arm/xilinx_zynq.c +6 -0

index 9f89483..8c249e8 100644
@@ -43,6 +43,41 @@ static const int dma_irqs[8] = {
     46, 47, 48, 49, 72, 73, 74, 75
 };
 
+#define BOARD_SETUP_ADDR        0x0
+#define LOAD_ADDR               0x1000
+
+#define SLCR_LOCK_OFFSET        0x004
+#define SLCR_UNLOCK_OFFSET      0x008
+#define SLCR_ARM_PLL_OFFSET     0x100

hw/arm/xilinx_zynq.c +23 -2

+
+static void zynq_write_board_setup(ARMCPU *cpu,
+                                   const struct arm_boot_info *info)
+{
+    int n;
+    uint32_t board_setup_blob[] = {
+        0xe3a004f8, /* mov r0, #0xf8000000 */
+        SLCR_WRITE(SLCR_UNLOCK_OFFSET, SLCR_XILINX_UNLOCK_KEY)
+        SLCR_WRITE(SLCR_ARM_PLL_OFFSET, 0x00014008)
+        SLCR_WRITE(SLCR_LOCK_OFFSET, SLCR_XILINX_LOCK_KEY)
+        0xe12fff1e, /* bx lr */
+    };
+    for (n = 0; n < ARRAY_SIZE(board_setup_blob); n++) {
+        board_setup_blob[n] = tswap32(board_setup_blob[n]);
+    }
+    rom_add_blob_fixed("board-setup", board_setup_blob,
+                       sizeof(board_setup_blob), BOARD_SETUP_ADDR);
+}
+
 static struct arm_boot_info zynq_binfo = {};
 
 static void gem_init(NICInfo *nd, uint32_t base, qemu_irq irq)

@@ -251,7 +286,10 @@ static void zynq_init(MachineState *machine)
     zynq_binfo.initrd_filename = initrd_filename;
     zynq_binfo.nb_cpus = 1;
     zynq_binfo.board_id = 0xd32;
-    zynq_binfo.loader_start = 0;
+    zynq_binfo.loader_start = LOAD_ADDR;
+    zynq_binfo.board_setup_addr = BOARD_SETUP_ADDR;
+    zynq_binfo.write_board_setup = zynq_write_board_setup;
+
     arm_load_kernel(ARM_CPU(first_cpu), &zynq_binfo);
 }
 
--

1.9.1

Quoted 7 lines · Peter Crosthwaite

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.

Quoted 8 lines

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

---
Changed since RFC:
Use bootloader callback to load blob. Change "firmware" to "board-setup" for consistency. Tweak commit message.

 hw/arm/highbank.c | 57

++++++++++++++++++++++++++++++++++++++++++++-----------

Quoted 10 lines

 1 file changed, 46 insertions(+), 11 deletions(-)

hw/arm/highbank.c +0 -0

index be04b27..98daba0 100644
@@ -28,6 +28,9 @@
 #include "exec/address-spaces.h"
 #include "qemu/error-report.h"
 

hw/arm/highbank.c line 32

Quoted 2 lines

hw/arm/highbank.c +2 -0

+#define BOARD_SETUP_ADDR        0x0
+#define LOAD_ADDR               0x1000

Are these obliged to be on different (4K) pages?

Quoted 9 lines

hw/arm/highbank.c +1 -0

+
 #define SMP_BOOT_ADDR           0x100
 #define SMP_BOOT_REG            0x40
 #define MPCORE_PERIPHBASE       0xfff10000

@@ -36,6 +39,38 @@
 
 /* Board init.  */
 

hw/arm/highbank.c line 50

Quoted 9 lines

hw/arm/highbank.c +9 -0

+static void hb_write_board_setup(ARMCPU *cpu,
+                                 const struct arm_boot_info *info)
+{
+    int n;
+    uint32_t board_setup_blob[] = {
+        /* Reset */
+        0xe320f000, /* nop */
+        0xe320f000, /* nop */
+        /* smc */

It would probably be a good idea to actually have a full vector table here, even if the remaining entries just do "jump off to an infinite-loop error location", so that if the guest is buggy and causes an exception in early bootup before it's got its own vector table set up then we do something noticeable

rather than just leaping back into the start of the kernel boot sequence.

hw/arm/highbank.c line 57

Quoted 7 lines

hw/arm/highbank.c +7 -0

+        0xe10f0000, /* mrs r0, CPSR */
+        0xe200001f, /* and r0, r0, #0x1f - mask off mode bits */
+        0xe3500016, /* cmp r0, #0x16 - are we in monitor mode? */
+        /* if (!monitor_mode) { */
+            0x11600070, /* smcne - go to monitor mode */
+            0x112fff1e, /* bxne lr - return to caller */
+        /* } */

This seems a bit weird. At the reset entry point we know we're not in monitor mode (will be secure SVC). At the SMC entry point (and indeed every other entry point for MVBAR) we know we are in monitor mode. So why have a runtime check?

hw/arm/highbank.c line 59

Quoted 2 lines

hw/arm/highbank.c +2 -0

+        /* do setup from monitor mode */
+        0xe3a00000 + BOARD_SETUP_ADDR, /* mov r0, #BOARD_SETUP_ADDR */

This is an unfortunate choice of location for the monitor vector table, because it turns out to be zero, which will also be the initial default for the non-monitor vector table.

hw/arm/highbank.c line 62

Quoted 3 lines

hw/arm/highbank.c +3 -0

+        0xee0c0f30, /* mcr p15, 0, r0, c12, c0, 1 - set mvbar */
+        0xe58fe008, /* save lr */
+        0xe8dfc000, /* exception return */

...doesn't this mean we set the MVBAR for any SMC the guest does? That seems like an odd way to implement "nop the cache operations"...

hw/arm/highbank.c line 73

Quoted 12 lines

hw/arm/highbank.c +11 -0

+        0,
+        0,
+        0, /* exception return link will end up here */
+    };
+    for (n = 0; n < ARRAY_SIZE(board_setup_blob); n++) {
+        board_setup_blob[n] = tswap32(board_setup_blob[n]);
+    }
+    rom_add_blob_fixed("board-setup", board_setup_blob,
+                       sizeof(board_setup_blob), BOARD_SETUP_ADDR);
+}
+

thanks
-- PMM

Quoted 22 lines · Peter Crosthwaite

Add a Linux-specific pre-boot routine that matches the device specific bootloaders behaviour. This is needed for modern Linux that expects the ARM PLL in SLCR to be a more even value (not 26).

Cc Alistair Francis <[email protected]>

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

---
Changed since RFC:
Use bootloader callback to load blob. Change "firmware" to "board-setup" for consistency.

 hw/arm/xilinx_zynq.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

hw/arm/xilinx_zynq.c +0 -0

index 9f89483..8c249e8 100644
@@ -43,6 +43,41 @@ static const int dma_irqs[8] = {
     46, 47, 48, 49, 72, 73, 74, 75
 };
 

hw/arm/xilinx_zynq.c static const int dma_irqs[8] = { · line 55

Quoted 10 lines

hw/arm/xilinx_zynq.c +10 -0

+#define BOARD_SETUP_ADDR        0x0
+#define LOAD_ADDR               0x1000
+
+#define SLCR_LOCK_OFFSET        0x004
+#define SLCR_UNLOCK_OFFSET      0x008
+#define SLCR_ARM_PLL_OFFSET     0x100
+
+#define SLCR_XILINX_UNLOCK_KEY  0xdf0d
+#define SLCR_XILINX_LOCK_KEY    0x767b
+

Not ending the macro with a trailing comma would make the uses of it below look a bit more natural (since they then have the comma sort of how you'd expect).

Quoted 34 lines

hw/arm/xilinx_zynq.c +23 -1

+
+static void zynq_write_board_setup(ARMCPU *cpu,
+                                   const struct arm_boot_info *info)
+{
+    int n;
+    uint32_t board_setup_blob[] = {
+        0xe3a004f8, /* mov r0, #0xf8000000 */
+        SLCR_WRITE(SLCR_UNLOCK_OFFSET, SLCR_XILINX_UNLOCK_KEY)
+        SLCR_WRITE(SLCR_ARM_PLL_OFFSET, 0x00014008)
+        SLCR_WRITE(SLCR_LOCK_OFFSET, SLCR_XILINX_LOCK_KEY)
+        0xe12fff1e, /* bx lr */
+    };
+    for (n = 0; n < ARRAY_SIZE(board_setup_blob); n++) {
+        board_setup_blob[n] = tswap32(board_setup_blob[n]);
+    }
+    rom_add_blob_fixed("board-setup", board_setup_blob,
+                       sizeof(board_setup_blob), BOARD_SETUP_ADDR);
+}
+
 static struct arm_boot_info zynq_binfo = {};
 
 static void gem_init(NICInfo *nd, uint32_t base, qemu_irq irq)

@@ -251,7 +286,10 @@ static void zynq_init(MachineState *machine)
     zynq_binfo.initrd_filename = initrd_filename;
     zynq_binfo.nb_cpus = 1;
     zynq_binfo.board_id = 0xd32;
-    zynq_binfo.loader_start = 0;
+    zynq_binfo.loader_start = LOAD_ADDR;
+    zynq_binfo.board_setup_addr = BOARD_SETUP_ADDR;
+    zynq_binfo.write_board_setup = zynq_write_board_setup;
+
     arm_load_kernel(ARM_CPU(first_cpu), &zynq_binfo);
 }
 

Quoted 2 lines

--
1.9.1

Otherwise

Reviewed-by Peter Maydell <[email protected]>

thanks
-- PMM

Quoted 11 lines · Peter Crosthwaite

Add an API for boards to inject their own preboot software (or firmware) seqeuence.

The software then returns to bootloader via the link register. This allows boards to do their own little bits of firmware setup without needed to replace the bootloader completely (which is the requirement for existing firmware support).

The blob is loaded by a callback if and only if doing a linux boot (similar to the existing write_secondary support).

Quoted 8 lines

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

---
Changed since RFC:
Load blob via firmware.
Remove un-needed 0-word in bootloader sequence.
Remove "blob", just use "board setup" consistently
Remove boolean for (just use a pointer NULL check on write_board_setup) Adjust comment about functionality of primary bootloader

Couple of comment tweaks, but otherwise

Reviewed-by Peter Maydell <[email protected]>

Quoted 10 lines

 hw/arm/boot.c        | 17 ++++++++++++++++-
 include/hw/arm/arm.h | 10 ++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

hw/arm/boot.c +0 -0

index 2a151e2..5640989 100644

hw/arm/boot.c typedef enum { · line 34

Quoted 4 lines

hw/arm/boot.c +1 -0

@@ -31,6 +31,7 @@ typedef enum {
     FIXUP_NONE = 0,     /* do nothing */
     FIXUP_TERMINATOR,   /* end of insns */
     FIXUP_BOARDID,      /* overwrite with board ID number */
+    FIXUP_BOARD_SETUP,  /* overwrite with board specific setup code

address */

hw/arm/boot.c static const ARMInsnFixup bootloader_aarch64[] = { · line 61

Quoted 8 lines

hw/arm/boot.c +0 -1

     FIXUP_ARGPTR,       /* overwrite with pointer to kernel args */
     FIXUP_ENTRYPOINT,   /* overwrite with kernel entry point */
     FIXUP_GIC_CPU_IF,   /* overwrite with GIC CPU interface address */

@@ -58,8 +59,14 @@ static const ARMInsnFixup bootloader_aarch64[] = {
     { 0, FIXUP_TERMINATOR }
 };
 
-/* The worlds second smallest bootloader.  Set r0-r2, then jump to

kernel. */

hw/arm/boot.c static const ARMInsnFixup bootloader_aarch64[] = { · line 62

Quoted 1 line

hw/arm/boot.c +1 -0

+/* The worlds second smallest bootloader.  Call the board-setup code,

Set r0-r2,

hw/arm/boot.c static const ARMInsnFixup bootloader_aarch64[] = { · line 64

Quoted 2 lines

hw/arm/boot.c +2 -0

+ * then jump to kernel.
+ */

While we're editing this comment we might as well fix the grammar error and remove the claim about size (since we're gradually increasing it); a note about the fact the first part is not always used is probably helpful too:

/* A very small bootloader: call the board-setup code (if needed),
* set r0-r2, then jump to the kernel.
* If we're not calling boot setup code then we don't copy across
* the first BOOTLOADER_NO_BOARD_SETUP_OFFSET insns in this array. */

hw/arm/boot.c static void write_bootloader(const char *name,

Quoted 9 lines

hw/arm/boot.c +4 -0

 static const ARMInsnFixup bootloader[] = {
+    { 0xe28fe008 }, /* add     lr, pc, #8 */
+    { 0xe51ff004 }, /* ldr     pc, [pc, #-4] */
+    { 0, FIXUP_BOARD_SETUP },
+#define BOOTLOADER_NO_BOARD_SETUP_OFFSET 3
     { 0xe3a00000 }, /* mov     r0, #0 */
     { 0xe59f1004 }, /* ldr     r1, [pc, #4] */
     { 0xe59f2004 }, /* ldr     r2, [pc, #4] */

@@ -131,6 +138,7 @@ static void write_bootloader(const char *name,

hwaddr addr,

hw/arm/boot.c static void arm_load_kernel_notify(Notifier

Quoted 8 lines

hw/arm/boot.c +1 -0

         case FIXUP_NONE:
             break;
         case FIXUP_BOARDID:
+        case FIXUP_BOARD_SETUP:
         case FIXUP_ARGPTR:
         case FIXUP_ENTRYPOINT:
         case FIXUP_GIC_CPU_IF:

@@ -640,6 +648,9 @@ static void arm_load_kernel_notify(Notifier

*notifier, void *data)

hw/arm/boot.c static void arm_load_kernel_notify(Notifier

Quoted 10 lines

hw/arm/boot.c +3 -0

         elf_machine = EM_AARCH64;
     } else {
         primary_loader = bootloader;
+        if (!info->write_board_setup) {
+            primary_loader += BOOTLOADER_NO_BOARD_SETUP_OFFSET;
+        }
         kernel_load_offset = KERNEL_LOAD_ADDR;
         elf_machine = EM_ARM;
     }

@@ -745,6 +756,7 @@ static void arm_load_kernel_notify(Notifier

*notifier, void *data)

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

Quoted 6 lines

hw/arm/boot.c +1 -0

         info->initrd_size = initrd_size;
 
         fixupcontext[FIXUP_BOARDID] = info->board_id;
+        fixupcontext[FIXUP_BOARD_SETUP] = info->board_setup_addr;
 
         /* for device tree boot, we pass the DTB directly in r2.

Otherwise

hw/arm/boot.c static void arm_load_kernel_notify(Notifier

Quoted 2 lines

hw/arm/boot.c +0 -0

          * we point to the kernel args.

@@ -793,6 +805,9 @@ static void arm_load_kernel_notify(Notifier

*notifier, void *data)

Quoted 14 lines

hw/arm/boot.c +3 -0

         if (info->nb_cpus > 1) {
             info->write_secondary_boot(cpu, info);
         }
+        if (info->write_board_setup) {
+            info->write_board_setup(cpu, info);
+        }
 
         /* Notify devices which need to fake up firmware initialization
          * that we're doing a direct kernel boot.

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

index 4dcd4f9..128a54e 100644

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

Quoted 5 lines

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

@@ -87,6 +87,16 @@ struct arm_boot_info {
      * -pflash. It also implies that fw_cfg_find() will succeed.
      */
     bool firmware_loaded;
+
+    /* Address at which board specific loader/setup code exists. If

enabled,

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

Quoted 1 line

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

+     * this code-blob will run before anything else. It must return to

the

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

Quoted 1 line

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

+     * caller via the link register. There is no stack setup. Enabled by

"set up".

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

Quoted 1 line

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

+     * defining write_board_setup, which is responsible for loading the

blob

Quoted 10 lines

include/hw/arm/arm.h +5 -1

+     * to the specified address.
+     */
+    hwaddr board_setup_addr;
+    void (*write_board_setup)(ARMCPU *cpu,
+                              const struct arm_boot_info *info);
 };
 
 /**
--

1.9.1

thanks
-- PMM

Quoted 4 lines · Peter Maydell

On 25 October 2015 at 23:13, Peter Crosthwaite
<[email protected]> wrote:

Quoted 15 lines

hw/arm/highbank.c line 50

Quoted 9 lines

hw/arm/highbank.c +9 -0

+static void hb_write_board_setup(ARMCPU *cpu,
+                                 const struct arm_boot_info *info)
+{
+    int n;
+    uint32_t board_setup_blob[] = {
+        /* Reset */
+        0xe320f000, /* nop */
+        0xe320f000, /* nop */
+        /* smc */

It would probably be a good idea to actually have a full vector table here, even if the remaining entries just do "jump off to an infinite-loop error location", so that if the guest is buggy and causes an exception in early bootup before it's got its own vector table set up then we do something noticeable

What could we to that is noticeable to the guest?

Quoted 11 lines

rather than just leaping back into the start of the kernel boot sequence.

hw/arm/highbank.c line 57

Quoted 7 lines

hw/arm/highbank.c +7 -0

+        0xe10f0000, /* mrs r0, CPSR */
+        0xe200001f, /* and r0, r0, #0x1f - mask off mode bits */
+        0xe3500016, /* cmp r0, #0x16 - are we in monitor mode? */
+        /* if (!monitor_mode) { */
+            0x11600070, /* smcne - go to monitor mode */
+            0x112fff1e, /* bxne lr - return to caller */
+        /* } */

Quoted 5 lines

This seems a bit weird. At the reset entry point we know we're not in monitor mode (will be secure SVC). At the SMC entry point (and indeed every other entry point for MVBAR) we know we are in monitor mode. So why have a runtime check?

It is handling both the initial setup and nopping at the same time. It could be done split but this allowed a solutions without a jump table, which is hard to maintain in machine code.

Quoted 8 lines

hw/arm/highbank.c line 59

Quoted 3 lines

hw/arm/highbank.c +2 -0

+        /* do setup from monitor mode */
+        0xe3a00000 + BOARD_SETUP_ADDR, /* mov r0, #BOARD_SETUP_ADDR */

This is an unfortunate choice of location for the monitor vector table, because it turns out to be zero, which will also be the initial default for the non-monitor vector table.

Linux can manage this

so I went this was for the simplicity of an all-in-one
table. Otherwise I need to have two tables. One to trap the first SMC to 0x8 which then sets up MVBAR, then another table for the runtime nop.

Quoted 25 lines · Peter Maydell

Quoted 22 lines · Peter Crosthwaite

Add a Linux-specific pre-boot routine that matches the device specific bootloaders behaviour. This is needed for modern Linux that expects the ARM PLL in SLCR to be a more even value (not 26).

Cc Alistair Francis <[email protected]>

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

---
Changed since RFC:
Use bootloader callback to load blob. Change "firmware" to "board-setup" for consistency.

 hw/arm/xilinx_zynq.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

hw/arm/xilinx_zynq.c +0 -0

index 9f89483..8c249e8 100644
@@ -43,6 +43,41 @@ static const int dma_irqs[8] = {
     46, 47, 48, 49, 72, 73, 74, 75
 };
 

Quoted 10 lines

hw/arm/xilinx_zynq.c static const int dma_irqs[8] = { · line 55

Quoted 10 lines

hw/arm/xilinx_zynq.c +10 -0

+#define BOARD_SETUP_ADDR        0x0
+#define LOAD_ADDR               0x1000
+
+#define SLCR_LOCK_OFFSET        0x004
+#define SLCR_UNLOCK_OFFSET      0x008
+#define SLCR_ARM_PLL_OFFSET     0x100
+
+#define SLCR_XILINX_UNLOCK_KEY  0xdf0d
+#define SLCR_XILINX_LOCK_KEY    0x767b
+

Something like:

/* Write immediate val to address r0 + addr. r0 should contain base offset of SLCR block.
* Clobbers r1.
*/

#define SLCR_WRITE(addr, val) \
        0xe3a01000 + extract32((val), 0, 8), /* mov r1, val & (FF << 0) */ \
        0xe3811c00 + extract32((val), 8, 8), /* orr r1, r1, val & (FF << 8)
*/ \
        0xe3811800 + extract32((val), 16, 8), /* orr r1, r1, val & (FF <<
16) */ \
        0xe3811400 + extract32((val), 24, 8), /* orr r1, r1, val & (FF <<
24) */ \
        0xe5801000 + (addr) /* str r0, r1, #addr */

Quoted 26 lines · Peter Crosthwaite

On Tue, Oct 27, 2015 at 5:26 AM, Peter Maydell <[email protected]

Quoted 4 lines · wrote:

On 25 October 2015 at 23:13, Peter Crosthwaite
<[email protected]> wrote:

Quoted 15 lines

hw/arm/highbank.c line 50

Quoted 9 lines

hw/arm/highbank.c +9 -0

+static void hb_write_board_setup(ARMCPU *cpu,
+                                 const struct arm_boot_info *info)
+{
+    int n;
+    uint32_t board_setup_blob[] = {
+        /* Reset */
+        0xe320f000, /* nop */
+        0xe320f000, /* nop */
+        /* smc */

It would probably be a good idea to actually have a full vector table here, even if the remaining entries just do "jump off to an infinite-loop error location", so that if the guest is buggy and causes an exception in early bootup before it's got its own vector table set up then we do something noticeable

Quoted 1 line

What could we to that is noticeable to the guest?

I just meant "noticeable when you're debugging", like a tight loop at a very low location in memory.

Quoted 11 lines

Quoted 11 lines

rather than just leaping back into the start of the kernel boot sequence.

Quoted 7 lines

hw/arm/highbank.c +7 -0

+        0xe10f0000, /* mrs r0, CPSR */
+        0xe200001f, /* and r0, r0, #0x1f - mask off mode bits */
+        0xe3500016, /* cmp r0, #0x16 - are we in monitor mode? */
+        /* if (!monitor_mode) { */
+            0x11600070, /* smcne - go to monitor mode */
+            0x112fff1e, /* bxne lr - return to caller */
+        /* } */

Quoted 7 lines

Quoted 5 lines

This seems a bit weird. At the reset entry point we know we're not in monitor mode (will be secure SVC). At the SMC entry point (and indeed every other entry point for MVBAR) we know we are in monitor mode. So why have a runtime check?

It is handling both the initial setup and nopping at the same time. It

could

Quoted 2 lines

be done split but this allowed a solutions without a jump table, which is hard to maintain in machine code.

You already have to deal with both entry points, that's why you have a bunch of nops in your code right now.

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+        /* do setup from monitor mode */
+        0xe3a00000 + BOARD_SETUP_ADDR, /* mov r0, #BOARD_SETUP_ADDR

*/

Quoted 7 lines

Quoted 5 lines

This is an unfortunate choice of location for the monitor vector table, because it turns out to be zero, which will also be the initial default for the non-monitor vector table.

Linux can manage this

Yeah, it can, because in general Linux (unless buggy) won't take any kind of exception until after it's got its own vector table set up somewhere (usually after MMU init). But it's confusing for people who are fiddling with Linux's early boot code, and then they tend to come to us to debug it for them :-)

Quoted 2 lines

so I went this was for the simplicity of an all-in-one table. Otherwise I need to have two tables. One to trap the first SMC to

0x8

Quoted 1 line

which then sets up MVBAR, then another table for the runtime nop.

This confuses me. Surely we should set up MVBAR on initial reset entry (ie at the start of this little blob of boot specific code), and SMC is always a runtime nop?

...oh, I see, this is because we start our boot in NS SVC, because we think we're booting a kernel, which is rather at cross-purposes with a firmware-ish blob. I'd thought that we would be entering from reset in Secure SVC, in which case you don't need to do anything to set up MVBAR.

That's actually a problem for this general scheme of doing things, because if you don't happen to have spare space at address 0 to put a vector table for your board-blob then you have no way to set MVBAR to wherever you do happen to have free space. Maybe we need to have the board-blob run in Secure mode (and then drop down to NS either in the board blob or in the generic bootloader after it returns from the board blob) ?

thanks
-- PMM

Quoted 44 lines · Peter Crosthwaite

On Tue, Oct 27, 2015 at 5:31 AM, Peter Maydell <[email protected]

Quoted 25 lines · wrote:

Quoted 22 lines · Peter Crosthwaite

Add a Linux-specific pre-boot routine that matches the device specific bootloaders behaviour. This is needed for modern Linux that expects the ARM PLL in SLCR to be a more even value (not 26).

Cc Alistair Francis <[email protected]>

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

---
Changed since RFC:
Use bootloader callback to load blob. Change "firmware" to "board-setup" for consistency.

 hw/arm/xilinx_zynq.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

hw/arm/xilinx_zynq.c +0 -0

index 9f89483..8c249e8 100644
@@ -43,6 +43,41 @@ static const int dma_irqs[8] = {
     46, 47, 48, 49, 72, 73, 74, 75
 };
 

Quoted 10 lines

hw/arm/xilinx_zynq.c static const int dma_irqs[8] = { · line 55

Quoted 10 lines

hw/arm/xilinx_zynq.c +10 -0

+#define BOARD_SETUP_ADDR        0x0
+#define LOAD_ADDR               0x1000
+
+#define SLCR_LOCK_OFFSET        0x004
+#define SLCR_UNLOCK_OFFSET      0x008
+#define SLCR_ARM_PLL_OFFSET     0x100
+
+#define SLCR_XILINX_UNLOCK_KEY  0xdf0d
+#define SLCR_XILINX_LOCK_KEY    0x767b
+

Something like:

Quoted 1 line

/* Write immediate val to address r0 + addr. r0 should contain base

offset

Quoted 6 lines

of SLCR block.
* Clobbers r1.
*/

#define SLCR_WRITE(addr, val) \
        0xe3a01000 + extract32((val), 0, 8), /* mov r1, val & (FF << 0)

*/ \

Quoted 1 line

        0xe3811c00 + extract32((val), 8, 8), /* orr r1, r1, val & (FF <<

8)

Quoted 6 lines

*/ \
        0xe3811800 + extract32((val), 16, 8), /* orr r1, r1, val & (FF <<
16) */ \
        0xe3811400 + extract32((val), 24, 8), /* orr r1, r1, val & (FF <<
24) */ \
        0xe5801000 + (addr) /* str r0, r1, #addr */

This board is Cortex-A9, right? Why not use movw/movt and save yourself two instructions?

(in fact two out of three
of your constants are 16-bit and can be done with a single movw insn.)

thanks
-- PMM

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.

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

---
Changed since RFC:
Use bootloader callback to load blob. Change "firmware" to "board-setup" for consistency. Tweak commit message.

 hw/arm/highbank.c | 57 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 46 insertions(+), 11 deletions(-)

hw/arm/highbank.c +46 -12

index be04b27..98daba0 100644
@@ -28,6 +28,9 @@
 #include "exec/address-spaces.h"
 #include "qemu/error-report.h"
 
+#define BOARD_SETUP_ADDR        0x0
+#define LOAD_ADDR               0x1000
+
 #define SMP_BOOT_ADDR           0x100
 #define SMP_BOOT_REG            0x40
 #define MPCORE_PERIPHBASE       0xfff10000

@@ -36,6 +39,38 @@
 
 /* Board init.  */
 
+static void hb_write_board_setup(ARMCPU *cpu,
+                                 const struct arm_boot_info *info)
+{
+    int n;
+    uint32_t board_setup_blob[] = {
+        /* Reset */
+        0xe320f000, /* nop */
+        0xe320f000, /* nop */
+        /* smc */
+        0xe10f0000, /* mrs r0, CPSR */
+        0xe200001f, /* and r0, r0, #0x1f - mask off mode bits */
+        0xe3500016, /* cmp r0, #0x16 - are we in monitor mode? */
+        /* if (!monitor_mode) { */
+            0x11600070, /* smcne - go to monitor mode */
+            0x112fff1e, /* bxne lr - return to caller */
+        /* } */
+        /* do setup from monitor mode */
+        0xe3a00000 + BOARD_SETUP_ADDR, /* mov r0, #BOARD_SETUP_ADDR */
+        0xee0c0f30, /* mcr p15, 0, r0, c12, c0, 1 - set mvbar */
+        0xe58fe008, /* save lr */
+        0xe8dfc000, /* exception return */
+        0,
+        0,
+        0, /* exception return link will end up here */
+    };
+    for (n = 0; n < ARRAY_SIZE(board_setup_blob); n++) {
+        board_setup_blob[n] = tswap32(board_setup_blob[n]);
+    }
+    rom_add_blob_fixed("board-setup", board_setup_blob,
+                       sizeof(board_setup_blob), BOARD_SETUP_ADDR);
+}
+
 static void hb_write_secondary(ARMCPU *cpu, const struct arm_boot_info *info)
 {
     int n;

@@ -248,16 +283,13 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
         cpuobj = object_new(object_class_get_name(oc));
         cpu = ARM_CPU(cpuobj);
 
-        /* By default A9 and A15 CPUs have EL3 enabled.  This board does not
-         * currently support EL3 so the CPU EL3 property is disabled before
-         * realization.
-         */
-        if (object_property_find(cpuobj, "has_el3", NULL)) {
-            object_property_set_bool(cpuobj, false, "has_el3", &err);
-            if (err) {
-                error_report_err(err);
-                exit(1);
-            }
+        object_property_set_int(cpuobj, QEMU_PSCI_CONDUIT_SMC,
+                                "psci-conduit", &error_abort);
+
+        if (n) {
+            /* Secondary CPUs start in PSCI powered-down state */
+            object_property_set_bool(cpuobj, true,
+                                     "start-powered-off", &error_abort);
         }
 
         if (object_property_find(cpuobj, "reset-cbar", NULL)) {

@@ -375,9 +407,12 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
      */
     highbank_binfo.board_id = -1;
     highbank_binfo.nb_cpus = smp_cpus;
-    highbank_binfo.loader_start = 0;
+    highbank_binfo.loader_start = LOAD_ADDR;
     highbank_binfo.write_secondary_boot = hb_write_secondary;
     highbank_binfo.secondary_cpu_reset_hook = hb_reset_secondary;
+    highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
+    highbank_binfo.write_board_setup = hb_write_board_setup;
+
     arm_load_kernel(ARM_CPU(first_cpu), &highbank_binfo);
 }
 
--

1.9.1

Quoted 3 lines · Peter Crosthwaite

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

Quoted 5 lines

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.

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

Thanks for doing this. I'm not a big fan of how the machine code for boot code is embedded into C in qemu, but that's a separate issue.

Quoted 7 lines

---
Changed since RFC:
Use bootloader callback to load blob. Change "firmware" to "board-setup" for consistency. Tweak commit message.

 hw/arm/highbank.c | 57

++++++++++++++++++++++++++++++++++++++++++++-----------

Quoted 43 lines

 1 file changed, 46 insertions(+), 11 deletions(-)

hw/arm/highbank.c +26 -0

index be04b27..98daba0 100644
@@ -28,6 +28,9 @@
 #include "exec/address-spaces.h"
 #include "qemu/error-report.h"
 
+#define BOARD_SETUP_ADDR        0x0
+#define LOAD_ADDR               0x1000
+
 #define SMP_BOOT_ADDR           0x100
 #define SMP_BOOT_REG            0x40
 #define MPCORE_PERIPHBASE       0xfff10000

@@ -36,6 +39,38 @@
 
 /* Board init.  */
 
+static void hb_write_board_setup(ARMCPU *cpu,
+                                 const struct arm_boot_info *info)
+{
+    int n;
+    uint32_t board_setup_blob[] = {
+        /* Reset */
+        0xe320f000, /* nop */
+        0xe320f000, /* nop */
+        /* smc */
+        0xe10f0000, /* mrs r0, CPSR */
+        0xe200001f, /* and r0, r0, #0x1f - mask off mode bits */
+        0xe3500016, /* cmp r0, #0x16 - are we in monitor mode? */
+        /* if (!monitor_mode) { */
+            0x11600070, /* smcne - go to monitor mode */
+            0x112fff1e, /* bxne lr - return to caller */
+        /* } */
+        /* do setup from monitor mode */
+        0xe3a00000 + BOARD_SETUP_ADDR, /* mov r0, #BOARD_SETUP_ADDR */
+        0xee0c0f30, /* mcr p15, 0, r0, c12, c0, 1 - set mvbar */
+        0xe58fe008, /* save lr */
+        0xe8dfc000, /* exception return */
+        0,
+        0,

hw/arm/highbank.c line 74

Quoted 10 lines

hw/arm/highbank.c +9 -0

+        0, /* exception return link will end up here */
+    };
+    for (n = 0; n < ARRAY_SIZE(board_setup_blob); n++) {
+        board_setup_blob[n] = tswap32(board_setup_blob[n]);
+    }
+    rom_add_blob_fixed("board-setup", board_setup_blob,
+                       sizeof(board_setup_blob), BOARD_SETUP_ADDR);
+}
+
 static void hb_write_secondary(ARMCPU *cpu, const struct arm_boot_info

*info)

hw/arm/highbank.c static void calxeda_init(MachineState *machine,

Quoted 3 lines

hw/arm/highbank.c +0 -0

 {
     int n;

@@ -248,16 +283,13 @@ static void calxeda_init(MachineState *machine,

enum cxmachines machine_id)

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

Quoted 4 lines

hw/arm/highbank.c +0 -1

         cpuobj = object_new(object_class_get_name(oc));
         cpu = ARM_CPU(cpuobj);
 
-        /* By default A9 and A15 CPUs have EL3 enabled.  This board

does not

hw/arm/highbank.c static void calxeda_init(MachineState *machine, · line 252

Quoted 1 line

hw/arm/highbank.c +0 -1

-         * currently support EL3 so the CPU EL3 property is disabled

before

hw/arm/highbank.c static void calxeda_init(MachineState *machine,

Quoted 19 lines

hw/arm/highbank.c +7 -8

-         * realization.
-         */
-        if (object_property_find(cpuobj, "has_el3", NULL)) {
-            object_property_set_bool(cpuobj, false, "has_el3", &err);
-            if (err) {
-                error_report_err(err);
-                exit(1);
-            }
+        object_property_set_int(cpuobj, QEMU_PSCI_CONDUIT_SMC,
+                                "psci-conduit", &error_abort);
+
+        if (n) {
+            /* Secondary CPUs start in PSCI powered-down state */
+            object_property_set_bool(cpuobj, true,
+                                     "start-powered-off", &error_abort);
         }
 
         if (object_property_find(cpuobj, "reset-cbar", NULL)) {

@@ -375,9 +407,12 @@ static void calxeda_init(MachineState *machine,

enum cxmachines machine_id)

Quoted 16 lines

hw/arm/highbank.c +4 -2

      */
     highbank_binfo.board_id = -1;
     highbank_binfo.nb_cpus = smp_cpus;
-    highbank_binfo.loader_start = 0;
+    highbank_binfo.loader_start = LOAD_ADDR;
     highbank_binfo.write_secondary_boot = hb_write_secondary;
     highbank_binfo.secondary_cpu_reset_hook = hb_reset_secondary;
+    highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
+    highbank_binfo.write_board_setup = hb_write_board_setup;
+
     arm_load_kernel(ARM_CPU(first_cpu), &highbank_binfo);
 }
 
--

1.9.1

Quoted 10 lines · Rob Herring

Quoted 7 lines · Peter Crosthwaite

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.

Quoted 7 lines

Quoted 1 line

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

Thanks for doing this. I'm not a big fan of how the machine code for boot code is embedded into C in qemu, but that's a separate issue.

Acked-by Rob Herring <[email protected]>

Thanks,

One question while you are here if you have the answer off-hand. Does the highbank firmware switch out of secure mode before booting the kernel or stay in secure mode? I know of platforms that regularly boot Linux into secure mode, so if it is one of those we might as well get it right per-board when doing Peters proposed idea.

Thanks for doing this. I'm not a big fan of how the machine code for boot code is embedded into C in qemu, but that's a separate issue.

Quoted 2 lines · Rob Herring

Thanks for doing this. I'm not a big fan of how the machine code for boot code is embedded into C in qemu, but that's a separate issue.

FWIW, I'm becoming increasingly unhappy with the approach too... it was straightforward when it was four asm instructions that worked for any board, but it's been gradually accreting extra complexity as we've gone along.

thanks
-- PMM

Quoted 5 lines · Peter Maydell

Quoted 2 lines · Rob Herring

Thanks for doing this. I'm not a big fan of how the machine code for boot code is embedded into C in qemu, but that's a separate issue.

Quoted 5 lines

FWIW, I'm becoming increasingly unhappy with the approach too... it was straightforward when it was four asm instructions that worked for any board, but it's been gradually accreting extra complexity as we've gone along.

Do we want to bite the bullet and add arm-gcc to configure so we can do it properly?