[Qemu-devel] [PATCH v2 0/5] ARM: Machine specific boot blobs

[Qemu-devel] [PATCH v2 0/5] ARM: Machine specific boot blobs

[Peter Crosthwaite]

Hi,

This adds support for machine-specific primary boot blobs. This can be
used to install little bits of firmware or boot code without having
to throw the whole QEMU bootloader out and BYO (with device drivers
and all).

It is then used to fix two boards, Zynq and Highbank, both which have
small but critical expectations of pre-boot software setup.

Regards,
Peter

Changed since v1:
Addressed PMM review.
Added secure_board_setup flag (P4)
Added Zynq patch first, then Highbank
See indiv. patches for detailed change logs.


Peter Crosthwaite (5):
  arm: boot: Adjust indentation of FIXUP comments
  arm: boot: Add board specific setup code API
  arm: xilinx_zynq: Add linux pre-boot
  arm: boot: Add secure_board_setup flag
  arm: highbank: Implement PSCI and dummy monitor

 hw/arm/boot.c        | 39 +++++++++++++++++++++++--------
 hw/arm/highbank.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++--------
 hw/arm/xilinx_zynq.c | 42 +++++++++++++++++++++++++++++++++
 include/hw/arm/arm.h | 16 +++++++++++++
 4 files changed, 143 insertions(+), 20 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/5] arm: boot: Adjust indentation of FIXUP comments

[Peter Crosthwaite]

These comments 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.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
Changed since v1:
s/comment/comments in commit message.

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

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index bef451b..2a151e2 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -28,14 +28,14 @@
 #define KERNEL64_LOAD_ADDR 0x00080000
 
 typedef enum {
-    FIXUP_NONE = 0,   /* do nothing */
-    FIXUP_TERMINATOR, /* end of insns */
-    FIXUP_BOARDID,    /* overwrite with board ID number */
-    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 */
-    FIXUP_BOOTREG,    /* overwrite with boot register address */
-    FIXUP_DSB,        /* overwrite with correct DSB insn for cpu */
+    FIXUP_NONE = 0,     /* do nothing */
+    FIXUP_TERMINATOR,   /* end of insns */
+    FIXUP_BOARDID,      /* overwrite with board ID number */
+    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 */
+    FIXUP_BOOTREG,      /* overwrite with boot register address */
+    FIXUP_DSB,          /* overwrite with correct DSB insn for cpu */
     FIXUP_MAX,
 } FixupType;
 
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/5] arm: boot: Add board specific setup code API

[Peter Crosthwaite]

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

The software then returns to the 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).

Rewrite the comment for the primary boot blob.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
Changed since v1:
Rewrite boot blob comment (PMM review)
s/setup/set up/ in comment (PMM review)
s/sequeunce/sequence in commit msg
Add missing "the" in commit message
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        | 20 +++++++++++++++++++-
 include/hw/arm/arm.h | 10 ++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 2a151e2..b0879a5 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -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 */
@@ -58,8 +59,17 @@ static const ARMInsnFixup bootloader_aarch64[] = {
     { 0, FIXUP_TERMINATOR }
 };
 
-/* The worlds second smallest bootloader.  Set r0-r2, then jump to kernel.  */
+/* 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.
+ */
+
 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 +141,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 +651,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 +759,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 +808,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.
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index 4dcd4f9..9217b70 100644
--- a/include/hw/arm/arm.h
+++ b/include/hw/arm/arm.h
@@ -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,
+     * this code-blob will run before anything else. It must return to the
+     * caller via the link register. There is no stack set up. Enabled by
+     * 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

[Qemu-devel] [PATCH v2 3/5] arm: xilinx_zynq: Add linux pre-boot

[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 <alistair.francis@xilinx.com>
Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
Changed since v1:
Left main loader address unchanged
Added comments for SLCR_WRITE macro (PMM review)
Convert SLCR_WRITE impl. to use movw/movt (PMM review)
Missing "-" for device-specific
Changed since RFC:
Use bootloader callback to load blob.
Change "firmware" to "board-setup" for consistency.

 hw/arm/xilinx_zynq.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 9f89483..82a9db8 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -43,6 +43,45 @@ static const int dma_irqs[8] = {
     46, 47, 48, 49, 72, 73, 74, 75
 };
 
+#define BOARD_SETUP_ADDR        0x100
+
+#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
+
+#define ARMV7_IMM16(x) (extract32((x),  0, 12) | \
+                        extract32((x), 12,  4) << 16)
+
+/* Write immediate val to address r0 + addr. r0 should contain base offset
+ * of the SLCR block. Clobbers r1.
+ */
+
+#define SLCR_WRITE(addr, val) \
+    0xe3001000 + ARMV7_IMM16(extract32((val),  0, 16)), /* movw r1 ... */ \
+    0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16)), /* movt r1 ... */ \
+    0xe5801000 + (addr)
+
+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)
@@ -252,6 +291,9 @@ static void zynq_init(MachineState *machine)
     zynq_binfo.nb_cpus = 1;
     zynq_binfo.board_id = 0xd32;
     zynq_binfo.loader_start = 0;
+    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

[Qemu-devel] [PATCH v2 4/5] arm: boot: Add secure_board_setup flag

[Peter Crosthwaite]

Add a flag that when set, will cause the primary CPU to start in secure
mode, even if the overall boot in 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>
---

 hw/arm/boot.c        | 3 ++-
 include/hw/arm/arm.h | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index b0879a5..6680d45 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;
                 }
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;
 };
 
 /**
-- 
1.9.1

[Qemu-devel] [PATCH v2 5/5] arm: highbank: Implement PSCI and dummy monitor

[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. All traps to
monitor mode implement the nop.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
Changed since v1:
fallthrough all of trap table to nop implementation
use movw for table address
leave loader at 0
Move MVBAR (and blob to non-zero)
Split nop implementation from MVBAR setup
set secure boot for board
implement NS switch in blob
Changed since RFC:
Use bootloader callback to load blob.
Change "firmware" to "board-setup" for consistency.
Tweak commit message.

 hw/arm/highbank.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 56 insertions(+), 10 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index be04b27..f3578a3 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -32,10 +32,55 @@
 #define SMP_BOOT_REG            0x40
 #define MPCORE_PERIPHBASE       0xfff10000
 
+#define MVBAR_ADDR              0x200
+
 #define NIRQ_GIC                160
 
 /* Board init.  */
 
+/* MVBAR_ADDR is limited by precision of movw */
+
+QEMU_BUILD_BUG_ON(MVBAR_ADDR >= (1 << 16));
+
+#define ARMV7_IMM16(x) (extract32((x),  0, 12) | \
+                        extract32((x), 12,  4) << 16)
+
+static void hb_write_board_setup(ARMCPU *cpu,
+                                 const struct arm_boot_info *info)
+{
+    int n;
+    uint32_t board_setup_blob[] = {
+        /* MVBAR_ADDR */
+        0xe320f000, /* nop */
+        0xe320f000, /* nop */
+        0xe320f000, /* nop */
+        0xe320f000, /* nop */
+        0xe320f000, /* nop */
+        0xe320f000, /* nop */
+        0xe320f000, /* nop */
+        0xe320f000, /* nop */
+#define ERET_ADDR (MVBAR_ADDR + 8 * sizeof(uint32_t))
+        0xe58fe008, /* save lr */
+        0xe8dfc000, /* exception return */
+        0,
+        0,
+        0, /* exception return link will end up here */
+#define BOARD_SETUP_ADDR (ERET_ADDR + 5 * sizeof(uint32_t))
+        0xe3000000 + ARMV7_IMM16(MVBAR_ADDR), /* movw r0, MVBAR_ADDR */
+        0xee0c0f30, /* mcr p15, 0, r0, c12, c0, 1 - set mvbar */
+        0xee110f11, /* mrc p15, 0, r0, c1 , c1, 0 - get scr */
+        0xe3810001, /* orr r0, #1 - set NS */
+        0xee010f11, /* mcr p15, 0, r0, c1 , c1, 0 - set scr */
+        0xe1600070, /* smc - go to monitor mode to flush NS change */
+        0xe12fff1e, /* bx lr - return to caller */
+    };
+    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), MVBAR_ADDR);
+}
+
 static void hb_write_secondary(ARMCPU *cpu, const struct arm_boot_info *info)
 {
     int n;
@@ -248,16 +293,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)) {
@@ -378,6 +420,10 @@ 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;
+    highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
+    highbank_binfo.write_board_setup = hb_write_board_setup;
+    highbank_binfo.secure_board_setup = true;
+
     arm_load_kernel(ARM_CPU(first_cpu), &highbank_binfo);
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2 0/5] ARM: Machine specific boot blobs

[Peter Maydell]

On 30 October 2015 at 05:34, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> Hi,
>
> This adds support for machine-specific primary boot blobs. This can be
> used to install little bits of firmware or boot code without having
> to throw the whole QEMU bootloader out and BYO (with device drivers
> and all).
>
> It is then used to fix two boards, Zynq and Highbank, both which have
> small but critical expectations of pre-boot software setup.
>
> Regards,
> Peter
>
> Changed since v1:
> Addressed PMM review.
> Added secure_board_setup flag (P4)
> Added Zynq patch first, then Highbank
> See indiv. patches for detailed change logs.
>
>
> Peter Crosthwaite (5):
>   arm: boot: Adjust indentation of FIXUP comments
>   arm: boot: Add board specific setup code API
>   arm: xilinx_zynq: Add linux pre-boot
>   arm: boot: Add secure_board_setup flag
>   arm: highbank: Implement PSCI and dummy monitor

Patches 1..3 are OK and I've put them in target-arm.next.
Patches 4 and 5 I have comments on.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/5] arm: boot: Add secure_board_setup flag

[Peter Maydell]

On 30 October 2015 at 05:34, 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 in non-secure. This is useful for when

"is non-secure".

> 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>
> ---
>
>  hw/arm/boot.c        | 3 ++-
>  include/hw/arm/arm.h | 6 ++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index b0879a5..6680d45 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;
>                  }
> 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;
>  };

I thought you were planning to have the generic code do the
S->NS transition; but I guess it works better in the board
code (we have to go up into Monitor and back down again, right?)

Is it an error for the board to set secure_board_setup if
the CPU doesn't have EL3? (if so, worth mentioning in this
comment; maybe assert?)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/5] arm: boot: Add secure_board_setup flag

[Peter Crosthwaite]

On Fri, Oct 30, 2015 at 1:49 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 October 2015 at 05:34, 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 in non-secure. This is useful for when
>
> "is non-secure".
>
>> 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>
>> ---
>>
>>  hw/arm/boot.c        | 3 ++-
>>  include/hw/arm/arm.h | 6 ++++++
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index b0879a5..6680d45 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;
>>                  }
>> 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;
>>  };
>
> I thought you were planning to have the generic code do the
> S->NS transition; but I guess it works better in the board
> code (we have to go up into Monitor and back down again, right?)
>

Yes I had to change my mind on this one. The issue was that ARM arch
doesn't guarantee a NS switch by simply modding SCR.NS inline and I
wanted to follow this convention. The recommended way is via eret
(presumably from monitor mode?). So to implement this for highbank I
do a dummy SMC after the SCR.NS switch (from secure EL1). This can't
be done generically as board-setup may or may-not install a functional
monitor.

> Is it an error for the board to set secure_board_setup if
> the CPU doesn't have EL3? (if so, worth mentioning in this
> comment; maybe assert?)
>

I don't like assert, as has_el3 is in theory is user modifiable (via
either -cpu transplants or directly via -global). I think it is an
error_exit().

Regards,
Peter

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v2 5/5] arm: highbank: Implement PSCI and dummy monitor

[Peter Maydell]

On 30 October 2015 at 05:35, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> 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.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> Changed since v1:
> fallthrough all of trap table to nop implementation
> use movw for table address
> leave loader at 0
> Move MVBAR (and blob to non-zero)
> Split nop implementation from MVBAR setup
> set secure boot for board
> implement NS switch in blob
> Changed since RFC:
> Use bootloader callback to load blob.
> Change "firmware" to "board-setup" for consistency.
> Tweak commit message.
>
>  hw/arm/highbank.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 56 insertions(+), 10 deletions(-)
>
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index be04b27..f3578a3 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -32,10 +32,55 @@
>  #define SMP_BOOT_REG            0x40
>  #define MPCORE_PERIPHBASE       0xfff10000
>
> +#define MVBAR_ADDR              0x200
> +
>  #define NIRQ_GIC                160
>
>  /* Board init.  */
>
> +/* MVBAR_ADDR is limited by precision of movw */
> +
> +QEMU_BUILD_BUG_ON(MVBAR_ADDR >= (1 << 16));
> +
> +#define ARMV7_IMM16(x) (extract32((x),  0, 12) | \
> +                        extract32((x), 12,  4) << 16)
> +
> +static void hb_write_board_setup(ARMCPU *cpu,
> +                                 const struct arm_boot_info *info)
> +{
> +    int n;
> +    uint32_t board_setup_blob[] = {
> +        /* MVBAR_ADDR */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +        0xe320f000, /* nop */
> +#define ERET_ADDR (MVBAR_ADDR + 8 * sizeof(uint32_t))
> +        0xe58fe008, /* save lr */
> +        0xe8dfc000, /* exception return */
> +        0,
> +        0,
> +        0, /* exception return link will end up here */
> +#define BOARD_SETUP_ADDR (ERET_ADDR + 5 * sizeof(uint32_t))
> +        0xe3000000 + ARMV7_IMM16(MVBAR_ADDR), /* movw r0, MVBAR_ADDR */
> +        0xee0c0f30, /* mcr p15, 0, r0, c12, c0, 1 - set mvbar */
> +        0xee110f11, /* mrc p15, 0, r0, c1 , c1, 0 - get scr */
> +        0xe3810001, /* orr r0, #1 - set NS */
> +        0xee010f11, /* mcr p15, 0, r0, c1 , c1, 0 - set scr */
> +        0xe1600070, /* smc - go to monitor mode to flush NS change */
> +        0xe12fff1e, /* bx lr - return to caller */
> +    };

This still confuses me. What I was expecting to see was something like:

    /* Monitor mode vector table; entry points which will only be reached
     * if the guest kernel is buggy are tight loops to avoid executing
     * random code by accident. The SMC vector entrypoint just returns,
     * making all SMC calls operate as NOPs.
     */
    0xwhatever, /* notused1: b notused1 */
    0xwhatever, /* notused2: b notused2 */
    0xwhatever, /* smc: movs pc, lr */
    0xwhatever, /* prefetch_abort: b prefetch_abort */
    0xwhatever, /* data_abort: b data_abort */
    0xwhatever, /* notused3: b notused3 */
    0xwhatever, /* irq: b irq */
    0xwhatever, /* fiq: b fiq */
    /* Entry point for reset (set via board_setup_addr) */
    [code here to set mvbar, etc]

Note that MOVS PC, LR is much simpler for exception return than RFE,
because you don't need to mess about saving the LR and SPSR to
memory in order to just read them back again. I'd have used ERET,
which has a more explanatory mnemonic, but that only exists in
CPUs with the virtualization extensions.

> +    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), MVBAR_ADDR);
> +}
> +
>  static void hb_write_secondary(ARMCPU *cpu, const struct arm_boot_info *info)
>  {
>      int n;
> @@ -248,16 +293,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)) {
> @@ -378,6 +420,10 @@ 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;
> +    highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
> +    highbank_binfo.write_board_setup = hb_write_board_setup;
> +    highbank_binfo.secure_board_setup = true;
> +
>      arm_load_kernel(ARM_CPU(first_cpu), &highbank_binfo);

If the user hands us "-cpu something-without-el3" I think we'll
now go blithely ahead with the secure_board_setup with hilarious
consquences, where previously we'd probably at least manage to
try to boot the kernel. (You can argue that the user shouldn't
be handing us silly arguments if you like.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/5] arm: boot: Add secure_board_setup flag

[Peter Maydell]

On 30 October 2015 at 20:59, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Fri, Oct 30, 2015 at 1:49 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> I thought you were planning to have the generic code do the
>> S->NS transition; but I guess it works better in the board
>> code (we have to go up into Monitor and back down again, right?)
>>
>
> Yes I had to change my mind on this one. The issue was that ARM arch
> doesn't guarantee a NS switch by simply modding SCR.NS inline and I
> wanted to follow this convention. The recommended way is via eret
> (presumably from monitor mode?). So to implement this for highbank I
> do a dummy SMC after the SCR.NS switch (from secure EL1). This can't
> be done generically as board-setup may or may-not install a functional
> monitor.
>
>> Is it an error for the board to set secure_board_setup if
>> the CPU doesn't have EL3? (if so, worth mentioning in this
>> comment; maybe assert?)
>>
>
> I don't like assert, as has_el3 is in theory is user modifiable (via
> either -cpu transplants or directly via -global). I think it is an
> error_exit().

The other question is what happens on a board like this if
the user says -enable-kvm -cpu cortex-a15 ? Does that get us
a CPU without the EL3 property? (I forget...) In any case it
shouldn't be an error unless the board genuinely can't work
with KVM at all, and if we're using KVM then the board
blob definitely won't be running in Secure (and can't flip
to Monitor mode either).

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/5] arm: boot: Add secure_board_setup flag

[Peter Crosthwaite]

On Fri, Oct 30, 2015 at 2:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 October 2015 at 20:59, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> On Fri, Oct 30, 2015 at 1:49 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> I thought you were planning to have the generic code do the
>>> S->NS transition; but I guess it works better in the board
>>> code (we have to go up into Monitor and back down again, right?)
>>>
>>
>> Yes I had to change my mind on this one. The issue was that ARM arch
>> doesn't guarantee a NS switch by simply modding SCR.NS inline and I
>> wanted to follow this convention. The recommended way is via eret
>> (presumably from monitor mode?). So to implement this for highbank I
>> do a dummy SMC after the SCR.NS switch (from secure EL1). This can't
>> be done generically as board-setup may or may-not install a functional
>> monitor.
>>
>>> Is it an error for the board to set secure_board_setup if
>>> the CPU doesn't have EL3? (if so, worth mentioning in this
>>> comment; maybe assert?)
>>>
>>
>> I don't like assert, as has_el3 is in theory is user modifiable (via
>> either -cpu transplants or directly via -global). I think it is an
>> error_exit().
>
> The other question is what happens on a board like this if
> the user says -enable-kvm -cpu cortex-a15 ? Does that get us
> a CPU without the EL3 property? (I forget...) In any case it
> shouldn't be an error unless the board genuinely can't work
> with KVM at all, and if we're using KVM then the board
> blob definitely won't be running in Secure (and can't flip
> to Monitor mode either).

I think all we can do is exit on !kvm and have the board if() the
firmware blob for the same. What is supposed to actually happen when a
virtualized guest running under KVM calls SMC? Does the VM have any
say on what that SMC does or is that the property of the host OS? The
latter suggests that Highbank Linux simply cannot be run under KVM.

Regards,
Peter

>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v2 5/5] arm: highbank: Implement PSCI and dummy monitor

[Peter Crosthwaite]

On Fri, Oct 30, 2015 at 2:10 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 October 2015 at 05:35, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> 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.
>>
>> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>> ---
>> Changed since v1:
>> fallthrough all of trap table to nop implementation
>> use movw for table address
>> leave loader at 0
>> Move MVBAR (and blob to non-zero)
>> Split nop implementation from MVBAR setup
>> set secure boot for board
>> implement NS switch in blob
>> Changed since RFC:
>> Use bootloader callback to load blob.
>> Change "firmware" to "board-setup" for consistency.
>> Tweak commit message.
>>
>>  hw/arm/highbank.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 56 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
>> index be04b27..f3578a3 100644
>> --- a/hw/arm/highbank.c
>> +++ b/hw/arm/highbank.c
>> @@ -32,10 +32,55 @@
>>  #define SMP_BOOT_REG            0x40
>>  #define MPCORE_PERIPHBASE       0xfff10000
>>
>> +#define MVBAR_ADDR              0x200
>> +
>>  #define NIRQ_GIC                160
>>
>>  /* Board init.  */
>>
>> +/* MVBAR_ADDR is limited by precision of movw */
>> +
>> +QEMU_BUILD_BUG_ON(MVBAR_ADDR >= (1 << 16));
>> +
>> +#define ARMV7_IMM16(x) (extract32((x),  0, 12) | \
>> +                        extract32((x), 12,  4) << 16)
>> +
>> +static void hb_write_board_setup(ARMCPU *cpu,
>> +                                 const struct arm_boot_info *info)
>> +{
>> +    int n;
>> +    uint32_t board_setup_blob[] = {
>> +        /* MVBAR_ADDR */
>> +        0xe320f000, /* nop */
>> +        0xe320f000, /* nop */
>> +        0xe320f000, /* nop */
>> +        0xe320f000, /* nop */
>> +        0xe320f000, /* nop */
>> +        0xe320f000, /* nop */
>> +        0xe320f000, /* nop */
>> +        0xe320f000, /* nop */
>> +#define ERET_ADDR (MVBAR_ADDR + 8 * sizeof(uint32_t))
>> +        0xe58fe008, /* save lr */
>> +        0xe8dfc000, /* exception return */
>> +        0,
>> +        0,
>> +        0, /* exception return link will end up here */
>> +#define BOARD_SETUP_ADDR (ERET_ADDR + 5 * sizeof(uint32_t))
>> +        0xe3000000 + ARMV7_IMM16(MVBAR_ADDR), /* movw r0, MVBAR_ADDR */
>> +        0xee0c0f30, /* mcr p15, 0, r0, c12, c0, 1 - set mvbar */
>> +        0xee110f11, /* mrc p15, 0, r0, c1 , c1, 0 - get scr */
>> +        0xe3810001, /* orr r0, #1 - set NS */
>> +        0xee010f11, /* mcr p15, 0, r0, c1 , c1, 0 - set scr */
>> +        0xe1600070, /* smc - go to monitor mode to flush NS change */
>> +        0xe12fff1e, /* bx lr - return to caller */
>> +    };
>
> This still confuses me. What I was expecting to see was something like:
>
>     /* Monitor mode vector table; entry points which will only be reached
>      * if the guest kernel is buggy are tight loops to avoid executing
>      * random code by accident. The SMC vector entrypoint just returns,
>      * making all SMC calls operate as NOPs.
>      */
>     0xwhatever, /* notused1: b notused1 */
>     0xwhatever, /* notused2: b notused2 */
>     0xwhatever, /* smc: movs pc, lr */
>     0xwhatever, /* prefetch_abort: b prefetch_abort */
>     0xwhatever, /* data_abort: b data_abort */
>     0xwhatever, /* notused3: b notused3 */
>     0xwhatever, /* irq: b irq */
>     0xwhatever, /* fiq: b fiq */

I don't get the difference between notused and prefetch_abort etc. The
only one that is used is SMC. Which ones should tight loop and which
ones should nop? Do we actually have to branch somewhere or can we get
away with one-inst tight loop / nops as appropriate?

>     /* Entry point for reset (set via board_setup_addr) */
>     [code here to set mvbar, etc]
>
> Note that MOVS PC, LR is much simpler for exception return than RFE,
> because you don't need to mess about saving the LR and SPSR to
> memory in order to just read them back again. I'd have used ERET,
> which has a more explanatory mnemonic, but that only exists in
> CPUs with the virtualization extensions.
>
>> +    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), MVBAR_ADDR);
>> +}
>> +
>>  static void hb_write_secondary(ARMCPU *cpu, const struct arm_boot_info *info)
>>  {
>>      int n;
>> @@ -248,16 +293,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)) {
>> @@ -378,6 +420,10 @@ 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;
>> +    highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
>> +    highbank_binfo.write_board_setup = hb_write_board_setup;
>> +    highbank_binfo.secure_board_setup = true;
>> +
>>      arm_load_kernel(ARM_CPU(first_cpu), &highbank_binfo);
>
> If the user hands us "-cpu something-without-el3" I think we'll
> now go blithely ahead with the secure_board_setup with hilarious
> consquences, where previously we'd probably at least manage to
> try to boot the kernel. (You can argue that the user shouldn't
> be handing us silly arguments if you like.)
>

I already have a patch that knocks out -cpu for highbank (for other
reasons) so might as well prepend it to series to make this go away.

Regards,
Peter

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v2 4/5] arm: boot: Add secure_board_setup flag

[Peter Maydell]

On 30 October 2015 at 21:24, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Fri, Oct 30, 2015 at 2:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The other question is what happens on a board like this if
>> the user says -enable-kvm -cpu cortex-a15 ? Does that get us
>> a CPU without the EL3 property? (I forget...) In any case it
>> shouldn't be an error unless the board genuinely can't work
>> with KVM at all, and if we're using KVM then the board
>> blob definitely won't be running in Secure (and can't flip
>> to Monitor mode either).
>
> I think all we can do is exit on !kvm and have the board if() the
> firmware blob for the same. What is supposed to actually happen when a
> virtualized guest running under KVM calls SMC? Does the VM have any
> say on what that SMC does or is that the property of the host OS? The
> latter suggests that Highbank Linux simply cannot be run under KVM.

The VM can intercept SMC, yes (HCR.TSC causes SMC to trap to Hyp,
regardless of what the SCR settings are). I don't know whether KVM
actually does trap it. Typically the need for SMC calls seems to
have only been for A9 and earlier CPUs, though -- mostly the A15
managed to avoid the requirement, so it hasn't been necessary for
KVM to deal with random board specific SMC handling. (Do we need
the SMC handling for the A15 variant of the highbank model?)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/5] arm: boot: Add secure_board_setup flag

[Peter Crosthwaite]

On Fri, Oct 30, 2015 at 3:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 October 2015 at 21:24, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> On Fri, Oct 30, 2015 at 2:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> The other question is what happens on a board like this if
>>> the user says -enable-kvm -cpu cortex-a15 ? Does that get us
>>> a CPU without the EL3 property? (I forget...) In any case it
>>> shouldn't be an error unless the board genuinely can't work
>>> with KVM at all, and if we're using KVM then the board
>>> blob definitely won't be running in Secure (and can't flip
>>> to Monitor mode either).
>>
>> I think all we can do is exit on !kvm and have the board if() the
>> firmware blob for the same. What is supposed to actually happen when a
>> virtualized guest running under KVM calls SMC? Does the VM have any
>> say on what that SMC does or is that the property of the host OS? The
>> latter suggests that Highbank Linux simply cannot be run under KVM.
>
> The VM can intercept SMC, yes (HCR.TSC causes SMC to trap to Hyp,
> regardless of what the SCR settings are). I don't know whether KVM
> actually does trap it. Typically the need for SMC calls seems to
> have only been for A9 and earlier CPUs, though -- mostly the A15
> managed to avoid the requirement, so it hasn't been necessary for
> KVM to deal with random board specific SMC handling. (Do we need
> the SMC handling for the A15 variant of the highbank model?)
>

I think so. This patch series repaired both Higbank and Midway,
although I am use a very generic Kernel so maybe a Kernel config gets
Midway in better shape. My gut feeling is yes, we need SMC for the a15
too.

Regards,
Peter

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v2 5/5] arm: highbank: Implement PSCI and dummy monitor

[Peter Maydell]

On 30 October 2015 at 21:32, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Fri, Oct 30, 2015 at 2:10 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This still confuses me. What I was expecting to see was something like:
>>
>>     /* Monitor mode vector table; entry points which will only be reached
>>      * if the guest kernel is buggy are tight loops to avoid executing
>>      * random code by accident. The SMC vector entrypoint just returns,
>>      * making all SMC calls operate as NOPs.
>>      */
>>     0xwhatever, /* notused1: b notused1 */
>>     0xwhatever, /* notused2: b notused2 */
>>     0xwhatever, /* smc: movs pc, lr */
>>     0xwhatever, /* prefetch_abort: b prefetch_abort */
>>     0xwhatever, /* data_abort: b data_abort */
>>     0xwhatever, /* notused3: b notused3 */
>>     0xwhatever, /* irq: b irq */
>>     0xwhatever, /* fiq: b fiq */
>
> I don't get the difference between notused and prefetch_abort etc. The
> only one that is used is SMC. Which ones should tight loop and which
> ones should nop? Do we actually have to branch somewhere or can we get
> away with one-inst tight loop / nops as appropriate?

The only one which we need to have do something is the SMC entry
point. None of the others will get called unless the guest manages
to do something screwy. They're all just separate tight loops
so if you look in the debugger and it says you're looping at
0x210 you know it was an unexpected prefetch abort rather than
an unexpected irq or whatever. (The only way to get to the notused
entries is literally to jump to them, incidentally, since the
CPU itself won't use them as exception entrypoints.)

We don't need to branch anywhere, since helpfully the ret insn
for smc only takes one insn. The rest are all just branch-to-self.
(If we did ever need more complicated smc handling then we could
just have "smc: b smchandler" and put the smchandler after the
vector table somewhere.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 4/5] arm: boot: Add secure_board_setup flag

[Peter Crosthwaite]

On Fri, Oct 30, 2015 at 2:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 October 2015 at 20:59, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> On Fri, Oct 30, 2015 at 1:49 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> I thought you were planning to have the generic code do the
>>> S->NS transition; but I guess it works better in the board
>>> code (we have to go up into Monitor and back down again, right?)
>>>
>>
>> Yes I had to change my mind on this one. The issue was that ARM arch
>> doesn't guarantee a NS switch by simply modding SCR.NS inline and I
>> wanted to follow this convention. The recommended way is via eret
>> (presumably from monitor mode?). So to implement this for highbank I
>> do a dummy SMC after the SCR.NS switch (from secure EL1). This can't
>> be done generically as board-setup may or may-not install a functional
>> monitor.
>>
>>> Is it an error for the board to set secure_board_setup if
>>> the CPU doesn't have EL3? (if so, worth mentioning in this
>>> comment; maybe assert?)
>>>

Does the same apply to secure_boot flag as well? I can't see any
guarding or erroring out there. secure_boot is simply ignored when
there is no EL3, and the same if on EL3 will apply to this new flag as
well. If we need to fix this, we should fix them both together.

>>
>> I don't like assert, as has_el3 is in theory is user modifiable (via
>> either -cpu transplants or directly via -global). I think it is an
>> error_exit().
>
> The other question is what happens on a board like this if
> the user says -enable-kvm -cpu cortex-a15 ? Does that get us
> a CPU without the EL3 property? (I forget...) In any case it
> shouldn't be an error unless the board genuinely can't work
> with KVM at all, and if we're using KVM then the board
> blob definitely won't be running in Secure (and can't flip
> to Monitor mode either).
>

And a similar conflict must happen in KVM if secure_boot is set right?
So whatever check/fix logic we apply to secure_board_setup needs to
apply to secure_boot too.

Regards,
Peter

> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v2 4/5] arm: boot: Add secure_board_setup flag

[Peter Maydell]

On 31 October 2015 at 03:40, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Fri, Oct 30, 2015 at 2:14 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 30 October 2015 at 20:59, Peter Crosthwaite
>> <crosthwaitepeter@gmail.com> wrote:
>>> On Fri, Oct 30, 2015 at 1:49 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>>> Is it an error for the board to set secure_board_setup if
>>>> the CPU doesn't have EL3? (if so, worth mentioning in this
>>>> comment; maybe assert?)
>>>>
>
> Does the same apply to secure_boot flag as well? I can't see any
> guarding or erroring out there. secure_boot is simply ignored when
> there is no EL3, and the same if on EL3 will apply to this new flag as
> well. If we need to fix this, we should fix them both together.

I started off this reply agreeing with you that we should fix both
of them. But then I realised that the two cases are actually different.

Our only current board that sets secure_boot is the vexpress
board, and the A15 version of that can happily boot in
non-secure or secure, because it doesn't need any SMC entry
points. So it works with KVM. (The A9 flavour wouldn't, but
KVM doesn't support A9 guests anyway.) That means that whether
secure_boot is in conflict with KVM or no-EL3 is a board
level decision (and it sounds like on highbank it should
perhaps be an error, though I have a feeling that suitably
configured kernels should be able to run in KVM, so perhaps not).

On the other hand, the secure_board_setup flag means the
board-blob must be relying on EL3 actually existing and
working (otherwise it would have provided a blob that worked
even in NS and not bothered setting secure_board_setup).
So "secure_board_setup + (KVM or no EL3) == error" is always
true for any board, and can be checked in boot.c.

So I think:
 * secure_boot + KVM is OK (or if it's not, it's the board
   code's job to error out)
 * secure_board_setup + KVM is not OK

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 3/5] arm: xilinx_zynq: Add linux pre-boot

[Alistair Francis]

On Thu, Oct 29, 2015 at 10:34 PM, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> 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 <alistair.francis@xilinx.com>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

This looks fine to me. I haven't looked much at the other patches though.

I think a comment in the zynq_write_board_setup() function saying what
it is doing wouldn't hurt either. Basically just copying the commit
message.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

> ---
> Changed since v1:
> Left main loader address unchanged
> Added comments for SLCR_WRITE macro (PMM review)
> Convert SLCR_WRITE impl. to use movw/movt (PMM review)
> Missing "-" for device-specific
> Changed since RFC:
> Use bootloader callback to load blob.
> Change "firmware" to "board-setup" for consistency.
>
>  hw/arm/xilinx_zynq.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 9f89483..82a9db8 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -43,6 +43,45 @@ static const int dma_irqs[8] = {
>      46, 47, 48, 49, 72, 73, 74, 75
>  };
>
> +#define BOARD_SETUP_ADDR        0x100
> +
> +#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
> +
> +#define ARMV7_IMM16(x) (extract32((x),  0, 12) | \
> +                        extract32((x), 12,  4) << 16)
> +
> +/* Write immediate val to address r0 + addr. r0 should contain base offset
> + * of the SLCR block. Clobbers r1.
> + */
> +
> +#define SLCR_WRITE(addr, val) \
> +    0xe3001000 + ARMV7_IMM16(extract32((val),  0, 16)), /* movw r1 ... */ \
> +    0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16)), /* movt r1 ... */ \
> +    0xe5801000 + (addr)
> +
> +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)
> @@ -252,6 +291,9 @@ static void zynq_init(MachineState *machine)
>      zynq_binfo.nb_cpus = 1;
>      zynq_binfo.board_id = 0xd32;
>      zynq_binfo.loader_start = 0;
> +    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
>
>