[Qemu-devel] [PATCH for-2.5 v1 0/4] ARM: Machine specific boot blobs

[Qemu-devel] [PATCH for-2.5 v1 0/4] 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


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

 hw/arm/boot.c        | 33 +++++++++++++++++++++---------
 hw/arm/highbank.c    | 57 ++++++++++++++++++++++++++++++++++++++++++----------
 hw/arm/xilinx_zynq.c | 40 +++++++++++++++++++++++++++++++++++-
 include/hw/arm/arm.h | 10 +++++++++
 4 files changed, 119 insertions(+), 21 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH for-2.5 v1 1/4] arm: boot: Adjust indentation of FIXUP comments

[Peter Crosthwaite]

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 <crosthwaite.peter@gmail.com>
---

 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

Re: [Qemu-devel] [PATCH for-2.5 v1 1/4] arm: boot: Adjust indentation of FIXUP comments

[Peter Maydell]

On 25 October 2015 at 23:13, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> 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 <crosthwaite.peter@gmail.com>
> ---
>
>  hw/arm/boot.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH for-2.5 v1 2/4] arm: boot: Add board specific setup code API

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

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
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(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 2a151e2..5640989 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,14 @@ static const ARMInsnFixup bootloader_aarch64[] = {
     { 0, FIXUP_TERMINATOR }
 };
 
-/* The worlds second smallest bootloader.  Set r0-r2, then jump to kernel.  */
+/* The worlds second smallest bootloader.  Call the board-setup code, Set r0-r2,
+ * then jump to kernel.
+ */
 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.
diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
index 4dcd4f9..128a54e 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 setup. 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

Re: [Qemu-devel] [PATCH for-2.5 v1 2/4] arm: boot: Add board specific setup code API

[Peter Maydell]

On 25 October 2015 at 23:13, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> 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 <crosthwaite.peter@gmail.com>
> ---
> 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 <peter.maydell@linaro.org>

>
>  hw/arm/boot.c        | 17 ++++++++++++++++-
>  include/hw/arm/arm.h | 10 ++++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 2a151e2..5640989 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,14 @@ static const ARMInsnFixup bootloader_aarch64[] = {
>      { 0, FIXUP_TERMINATOR }
>  };
>
> -/* The worlds second smallest bootloader.  Set r0-r2, then jump to kernel.  */
> +/* The worlds second smallest bootloader.  Call the board-setup code, Set r0-r2,
> + * 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.
 */

>  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.
> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> index 4dcd4f9..128a54e 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 setup. Enabled by

"set up".

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

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.5 v1 2/4] arm: boot: Add board specific setup code API

[Peter Crosthwaite]

[-- Attachment #1: Type: text/plain, Size: 5698 bytes --]

On Tue, Oct 27, 2015 at 5:19 AM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 25 October 2015 at 23:13, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
> > 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 <crosthwaite.peter@gmail.com>
> > ---
> > 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 <peter.maydell@linaro.org>
>
>

Thanks.


> >
> >  hw/arm/boot.c        | 17 ++++++++++++++++-
> >  include/hw/arm/arm.h | 10 ++++++++++
> >  2 files changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index 2a151e2..5640989 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,14 @@ static const ARMInsnFixup bootloader_aarch64[] = {
> >      { 0, FIXUP_TERMINATOR }
> >  };
> >
> > -/* The worlds second smallest bootloader.  Set r0-r2, then jump to
> kernel.  */
> > +/* The worlds second smallest bootloader.  Call the board-setup code,
> Set r0-r2,
> > + * 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.
>  */
>
>
Will take verbatim.


> >  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.
> > diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
> > index 4dcd4f9..128a54e 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 setup. Enabled by
>
> "set up".
>
>
Will fix.

Regards,
Peter


> > +     * 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
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 7906 bytes --]

[Qemu-devel] [PATCH for-2.5 v1 3/4] 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.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
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(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index be04b27..98daba0 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -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

Re: [Qemu-devel] [PATCH for-2.5 v1 3/4] arm: highbank: Implement PSCI and dummy monitor

[Peter Maydell]

On 25 October 2015 at 23:13, 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.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> 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(-)
>
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index be04b27..98daba0 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -28,6 +28,9 @@
>  #include "exec/address-spaces.h"
>  #include "qemu/error-report.h"
>
> +#define BOARD_SETUP_ADDR        0x0
> +#define LOAD_ADDR               0x1000

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

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

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.

> +        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?

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

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

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

Re: [Qemu-devel] [PATCH for-2.5 v1 3/4] arm: highbank: Implement PSCI and dummy monitor

[Peter Crosthwaite]

[-- Attachment #1: Type: text/plain, Size: 4686 bytes --]

On Tue, Oct 27, 2015 at 5:26 AM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 25 October 2015 at 23:13, 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.
> >
> > Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> > ---
> > 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(-)
> >
> > diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> > index be04b27..98daba0 100644
> > --- a/hw/arm/highbank.c
> > +++ b/hw/arm/highbank.c
> > @@ -28,6 +28,9 @@
> >  #include "exec/address-spaces.h"
> >  #include "qemu/error-report.h"
> >
> > +#define BOARD_SETUP_ADDR        0x0
> > +#define LOAD_ADDR               0x1000
>
> Are these obliged to be on different (4K) pages?
>

No. 0x1000 was just picked as a nice round number.


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


> rather than just leaping back into the start of the kernel
> boot sequence.
>
> > +        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?
>
>
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.



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



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


Yes.

So to fix I will have two tables (in the same blob), the 0 table will
unconditionally smc from reset, then set MVBAR to the MVBAR table. Then the
0 table needs to corrupt itself to some default before returning to to boot
blob. Open to suggestions on what the contents of that 0 table should be
post monitor setup. The MVBAR table then erets from every vector.

Regards,
Peter


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

[-- Attachment #2: Type: text/html, Size: 6847 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 v1 3/4] arm: highbank: Implement PSCI and dummy monitor

[Peter Maydell]

On 27 October 2015 at 15:11, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
>
>
> On Tue, Oct 27, 2015 at 5:26 AM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> On 25 October 2015 at 23:13, Peter Crosthwaite
>> <crosthwaitepeter@gmail.com> wrote:

>> > +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?

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

>> rather than just leaping back into the start of the kernel
>> boot sequence.
>>
>> > +        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?
>>
>
> 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.

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

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

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

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

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

Re: [Qemu-devel] [PATCH for-2.5 v1 3/4] arm: highbank: Implement PSCI and dummy monitor

[Peter Crosthwaite]

[-- Attachment #1: Type: text/plain, Size: 4196 bytes --]

On Tue, Oct 27, 2015 at 8:32 AM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 October 2015 at 15:11, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
> >
> >
> > On Tue, Oct 27, 2015 at 5:26 AM, Peter Maydell <peter.maydell@linaro.org
> >
> > wrote:
> >>
> >> On 25 October 2015 at 23:13, Peter Crosthwaite
> >> <crosthwaitepeter@gmail.com> wrote:
>
> >> > +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?
>
> I just meant "noticeable when you're debugging", like
> a tight loop at a very low location in memory.
>
> >> rather than just leaping back into the start of the kernel
> >> boot sequence.
> >>
> >> > +        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?
> >>
> >
> > 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.
>
> You already have to deal with both entry points, that's why you
> have a bunch of nops in your code right now.
>
> >>
> >> > +        /* 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
>
> 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 :-)
>
> > 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.
>
> 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) ?
>
>
Ok. I think the bootloader should do it. I'll do some digging.

Regards,
Peter



> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 5874 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 v1 3/4] arm: highbank: Implement PSCI and dummy monitor

[Rob Herring]

On Sun, Oct 25, 2015 at 6:13 PM, 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.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

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 <robh@kernel.org>

> ---
> 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(-)
>
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index be04b27..98daba0 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -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
>

Re: [Qemu-devel] [PATCH for-2.5 v1 3/4] arm: highbank: Implement PSCI and dummy monitor

[Peter Crosthwaite]

[-- Attachment #1: Type: text/plain, Size: 5155 bytes --]

On Tue, Oct 27, 2015 at 1:29 PM, Rob Herring <robh@kernel.org> wrote:

> On Sun, Oct 25, 2015 at 6:13 PM, 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.
> >
> > Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>
> 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 <robh@kernel.org>
>
>
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.

Regards,
Peter



> > ---
> > 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(-)
> >
> > diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> > index be04b27..98daba0 100644
> > --- a/hw/arm/highbank.c
> > +++ b/hw/arm/highbank.c
> > @@ -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
> >
>

[-- Attachment #2: Type: text/html, Size: 7167 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 v1 3/4] arm: highbank: Implement PSCI and dummy monitor

[Rob Herring]

On Tue, Oct 27, 2015 at 4:30 PM, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
>
>
> On Tue, Oct 27, 2015 at 1:29 PM, Rob Herring <robh@kernel.org> wrote:
>>
>> On Sun, Oct 25, 2015 at 6:13 PM, 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.
>> >
>> > Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>
>> 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 <robh@kernel.org>
>>
>
> 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.

The firmware switches out of secure mode. The kernel doesn't allow
switching, so it is true of all platforms that run non-secure
(although some do it in u-boot).

Rob

Re: [Qemu-devel] [PATCH for-2.5 v1 3/4] arm: highbank: Implement PSCI and dummy monitor

[Peter Maydell]

On 27 October 2015 at 20:29, Rob Herring <robh@kernel.org> wrote:
> 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

Re: [Qemu-devel] [PATCH for-2.5 v1 3/4] arm: highbank: Implement PSCI and dummy monitor

[Peter Crosthwaite]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

On Wed, Oct 28, 2015 at 5:10 AM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 October 2015 at 20:29, Rob Herring <robh@kernel.org> wrote:
> > 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.
>
>
Do we want to bite the bullet and add arm-gcc to configure so we can do it
properly?

Regards,
Peter



> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 1269 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 v1 3/4] arm: highbank: Implement PSCI and dummy monitor

[Peter Maydell]

On 28 October 2015 at 16:35, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
>
>
> On Wed, Oct 28, 2015 at 5:10 AM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> On 27 October 2015 at 20:29, Rob Herring <robh@kernel.org> wrote:
>> > 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.
>>
>
> Do we want to bite the bullet and add arm-gcc to configure so we can do it
> properly?

Requiring a cross toolchain is a world of pain I'd rather avoid.

-- PMM

[Qemu-devel] [PATCH for-2.5 v1 4/4] 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 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(-)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 9f89483..8c249e8 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -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
+
+#define SLCR_XILINX_UNLOCK_KEY  0xdf0d
+#define SLCR_XILINX_LOCK_KEY    0x767b
+
+#define SLCR_WRITE(addr, val) \
+        0xe3a01000 + extract32((val), 0, 8), \
+        0xe3811c00 + extract32((val), 8, 8), \
+        0xe3811800 + extract32((val), 16, 8), \
+        0xe3811400 + extract32((val), 24, 8), \
+        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)
@@ -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

Re: [Qemu-devel] [PATCH for-2.5 v1 4/4] arm: xilinx_zynq: Add linux pre-boot

[Peter Maydell]

On 25 October 2015 at 23:13, 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>
> ---
> 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(-)
>
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 9f89483..8c249e8 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -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
> +
> +#define SLCR_XILINX_UNLOCK_KEY  0xdf0d
> +#define SLCR_XILINX_LOCK_KEY    0x767b
> +
> +#define SLCR_WRITE(addr, val) \
> +        0xe3a01000 + extract32((val), 0, 8), \
> +        0xe3811c00 + extract32((val), 8, 8), \
> +        0xe3811800 + extract32((val), 16, 8), \
> +        0xe3811400 + extract32((val), 24, 8), \
> +        0xe5801000 + (addr),

This could use a comment about what these insns are...

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

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

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.5 v1 4/4] arm: xilinx_zynq: Add linux pre-boot

[Peter Crosthwaite]

[-- Attachment #1: Type: text/plain, Size: 3631 bytes --]

On Tue, Oct 27, 2015 at 5:31 AM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 25 October 2015 at 23:13, 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>
> > ---
> > 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(-)
> >
> > diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> > index 9f89483..8c249e8 100644
> > --- a/hw/arm/xilinx_zynq.c
> > +++ b/hw/arm/xilinx_zynq.c
> > @@ -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
> > +
> > +#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 */

This could use a comment about what these insns are...
>
> 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).
>
>
Will fix.


> > +
> > +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
>
> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
>
Thanks.

Regards,
Peter


> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 5589 bytes --]

Re: [Qemu-devel] [PATCH for-2.5 v1 4/4] arm: xilinx_zynq: Add linux pre-boot

[Peter Maydell]

On 27 October 2015 at 15:21, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
>
>
> On Tue, Oct 27, 2015 at 5:31 AM, Peter Maydell <peter.maydell@linaro.org>
> wrote:
>>
>> On 25 October 2015 at 23:13, 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>
>> > ---
>> > 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(-)
>> >
>> > diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
>> > index 9f89483..8c249e8 100644
>> > --- a/hw/arm/xilinx_zynq.c
>> > +++ b/hw/arm/xilinx_zynq.c
>> > @@ -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
>> > +
>> > +#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 */

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

Re: [Qemu-devel] [PATCH for-2.5 v1 4/4] arm: xilinx_zynq: Add linux pre-boot

[Peter Crosthwaite]

[-- Attachment #1: Type: text/plain, Size: 2822 bytes --]

On Tue, Oct 27, 2015 at 8:37 AM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 27 October 2015 at 15:21, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
> >
> >
> > On Tue, Oct 27, 2015 at 5:31 AM, Peter Maydell <peter.maydell@linaro.org
> >
> > wrote:
> >>
> >> On 25 October 2015 at 23:13, 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>
> >> > ---
> >> > 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(-)
> >> >
> >> > diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> >> > index 9f89483..8c249e8 100644
> >> > --- a/hw/arm/xilinx_zynq.c
> >> > +++ b/hw/arm/xilinx_zynq.c
> >> > @@ -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
> >> > +
> >> > +#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 */
>
> This board is Cortex-A9, right? Why not use movw/movt and
> save yourself two instructions?


Ok i'll use the two instruction form.


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

Id like to just be able to recycle this macro for all three writes so I
don't see too much value in hand-optimising the 16-bit writes to save only
two instructions. The full 32b flexibility makes the code easier to patch
if the values have to change (or more-likely, add more writes).

Regards,
Peter


>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 4489 bytes --]