[Qemu-devel] [PATCH for-2.5 v3 0/3] ARM: Highbank: Add monitor support

[Qemu-devel] [PATCH for-2.5 v3 0/3] ARM: Highbank: Add monitor support

[Peter Crosthwaite]

Hi,

This adds dummy monitor support to the Highbank board. It is needed by
the Highbank kernel which expects a monitor to be present.

A feature is added to arm/boot's board_setup feature, that allows the
board_setup entry point to be entered in secure mode (which is needed
to configure a monitor).

This feature does not play nice with -cpu override, but cpu override
is not valid for well-defined ARM SoCs. So defeature -cpu for Highbank.

Regards,
Peter

See indiv. patches for detailed change logs.

Changed since v2:
Defeature -cpu for Highbank (new patch)
Rework board_setup blob implementation (PMM review)
Conditionalise feature on TCG
Dropped initial board_setup and Zynq patches (Merged)
Changed since v1:
Addressed PMM review.
Added secure_board_setup flag (P4)
Added Zynq patch first, then Highbank


Peter Crosthwaite (3):
  arm: boot: Add secure_board_setup flag
  arm: highbank: Defeature CPU override
  arm: highbank: Implement PSCI and dummy monitor

 hw/arm/boot.c        |  8 ++++-
 hw/arm/highbank.c    | 90 ++++++++++++++++++++++++++++++++++++++--------------
 include/hw/arm/arm.h |  6 ++++
 3 files changed, 79 insertions(+), 25 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH for-2.5 v3 1/3] 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 is non-secure. This is useful for when
there is a board-setup blob that needs to run from secure mode, but
device and secondary CPU init should still be done as-normal for a non-
secure boot.

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
changed since v2:
Assert if running KVM and board_setup_secure is set

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

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index b0879a5..f671454 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;
                 }
@@ -598,6 +599,11 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
     struct arm_boot_info *info =
         container_of(n, struct arm_boot_info, load_kernel_notifier);
 
+    /* It is the boards job to make sure secure_board_setup is actually
+     * possible
+     */
+    assert(!info->secure_board_setup || tcg_enabled());
+
     /* Load the kernel.  */
     if (!info->kernel_filename || info->firmware_loaded) {
 
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

Re: [Qemu-devel] [PATCH for-2.5 v3 1/3] arm: boot: Add secure_board_setup flag

[Peter Maydell]

On 3 November 2015 at 04:30, 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 is non-secure. This is useful for when
> there is a board-setup blob that needs to run from secure mode, but
> device and secondary CPU init should still be done as-normal for a non-
> secure boot.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---
> changed since v2:
> Assert if running KVM and board_setup_secure is set
>
>  hw/arm/boot.c        | 8 +++++++-
>  include/hw/arm/arm.h | 6 ++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index b0879a5..f671454 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;
>                  }
> @@ -598,6 +599,11 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>      struct arm_boot_info *info =
>          container_of(n, struct arm_boot_info, load_kernel_notifier);
>
> +    /* It is the boards job to make sure secure_board_setup is actually
> +     * possible
> +     */

I think we could improve this comment a bit:
    /* The board code is not supposed to set secure_board_setup unless
     * running its code in secure mode is actually possible, and KVM
     * doesn't support secure.
     */

> +    assert(!info->secure_board_setup || tcg_enabled());

This assertion causes us to fail "make check", because there's
a test target which starts every board with the "qtest" accelerator,
which is not TCG. You need !kvm_enabled() instead (and
to include sysemu/kvm.h).

> +
>      /* Load the kernel.  */
>      if (!info->kernel_filename || info->firmware_loaded) {
>
> 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;
>  };

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.5 v3 1/3] arm: boot: Add secure_board_setup flag

[Peter Crosthwaite]

On Fri, Nov 6, 2015 at 6:45 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On 3 November 2015 at 04:30, 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 is non-secure. This is useful for when
> > there is a board-setup blob that needs to run from secure mode, but
> > device and secondary CPU init should still be done as-normal for a non-
> > secure boot.
> >
> > Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> > ---
> > changed since v2:
> > Assert if running KVM and board_setup_secure is set
> >
> >  hw/arm/boot.c        | 8 +++++++-
> >  include/hw/arm/arm.h | 6 ++++++
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index b0879a5..f671454 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;
> >                  }
> > @@ -598,6 +599,11 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
> >      struct arm_boot_info *info =
> >          container_of(n, struct arm_boot_info, load_kernel_notifier);
> >
> > +    /* It is the boards job to make sure secure_board_setup is actually
> > +     * possible
> > +     */
>
> I think we could improve this comment a bit:
>     /* The board code is not supposed to set secure_board_setup unless
>      * running its code in secure mode is actually possible, and KVM
>      * doesn't support secure.
>      */
>

Taken verbatim. Thanks.

> > +    assert(!info->secure_board_setup || tcg_enabled());
>
> This assertion causes us to fail "make check", because there's
> a test target which starts every board with the "qtest" accelerator,
> which is not TCG. You need !kvm_enabled() instead (and
> to include sysemu/kvm.h).
>

Fixed. Changed to De Morgan && equivalent with the two !s for easier reading:

assert(!(info->secure_board_setup && kvm_enabled()));

Regards,
Peter

> > +
> >      /* Load the kernel.  */
> >      if (!info->kernel_filename || info->firmware_loaded) {
> >
> > 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;
> >  };
>
> thanks
> -- PMM

[Qemu-devel] [PATCH for-2.5 v3 2/3] arm: highbank: Defeature CPU override

[Peter Crosthwaite]

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

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---

 hw/arm/highbank.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index be04b27..f2e248b 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -223,15 +223,13 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
     MemoryRegion *sysmem;
     char *sysboot_filename;
 
-    if (!cpu_model) {
-        switch (machine_id) {
-        case CALXEDA_HIGHBANK:
-            cpu_model = "cortex-a9";
-            break;
-        case CALXEDA_MIDWAY:
-            cpu_model = "cortex-a15";
-            break;
-        }
+    switch (machine_id) {
+    case CALXEDA_HIGHBANK:
+        cpu_model = "cortex-a9";
+        break;
+    case CALXEDA_MIDWAY:
+        cpu_model = "cortex-a15";
+        break;
     }
 
     for (n = 0; n < smp_cpus; n++) {
@@ -240,11 +238,6 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
         ARMCPU *cpu;
         Error *err = NULL;
 
-        if (!oc) {
-            error_report("Unable to find CPU definition");
-            exit(1);
-        }
-
         cpuobj = object_new(object_class_get_name(oc));
         cpu = ARM_CPU(cpuobj);
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH for-2.5 v3 2/3] arm: highbank: Defeature CPU override

[Peter Maydell]

On 3 November 2015 at 04:30, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> This board should not support CPU model override. This allows for
> easier patching of the board with being able to rely on the CPU
> type being correct.
>
> Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
> ---

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

thanks
-- PMM

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

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

Signed-off-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
---
Changed since v2:
Change to spinlock/movs trap table and drop fallthrough (PMM review)
Do not do board-setup if KVM is enabled. Issue a warning.
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 | 69 +++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 59 insertions(+), 10 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index f2e248b..30e8054 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -32,10 +32,52 @@
 #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 */
+        /* Default unimplemented and unused vectors to spin. Makes it
+         * easier to debug (as opposed to the CPU running away).
+         */
+        0xeafffffe, /* notused1: b notused */
+        0xeafffffe, /* notused2: b notused */
+        0xe1b0f00e, /* smc: movs pc, lr - exception return */
+        0xeafffffe, /* prefetch_abort: b prefetch_abort */
+        0xeafffffe, /* data_abort: b data_abort */
+        0xeafffffe, /* notused3: b notused3 */
+        0xeafffffe, /* irq: b irq */
+        0xeafffffe, /* fiq: b fiq */
+#define BOARD_SETUP_ADDR (MVBAR_ADDR + 8 * 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;
@@ -241,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)) {
@@ -371,6 +410,16 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
     highbank_binfo.loader_start = 0;
     highbank_binfo.write_secondary_boot = hb_write_secondary;
     highbank_binfo.secondary_cpu_reset_hook = hb_reset_secondary;
+    if (tcg_enabled()) {
+        highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
+        highbank_binfo.write_board_setup = hb_write_board_setup;
+        highbank_binfo.secure_board_setup = true;
+    } else {
+        error_report("WARNING: TCG unavailable - "
+                     "unable to load built-in Monitor support.\n"
+                     "Some guests (such as Linux) may not boot\n");
+    }
+
     arm_load_kernel(ARM_CPU(first_cpu), &highbank_binfo);
 }
 
-- 
1.9.1

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

[Peter Maydell]

On 3 November 2015 at 04:30, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> Firstly, enable monitor mode and PSCI, both are which are features of

"both of which"

> this board.
>
> In addition to PSCI, this board also uses SMC for cache maintainence
> ops. This means we need a secure monitor to catch these and nop them.
> Use the ARM boot board-setup feature to implement this. All traps to
> monitor mode implement the nop.
>
> As a KVM CPU cannot run in secure mode, do not do the board-setup if
> not running TCG. Report a warning explaining the limitation is this
> case.

"in this case".

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

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

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

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

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

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

thanks
-- PMM

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

[Peter Crosthwaite]

On Fri, Nov 6, 2015 at 6:47 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 November 2015 at 04:30, Peter Crosthwaite
> <crosthwaitepeter@gmail.com> wrote:
>> Firstly, enable monitor mode and PSCI, both are which are features of
>
> "both of which"
>

Fixed.

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

Clarified this a little better as since v3 the non-smc traps are
spinning rather than nopping.

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

Fixed.

>
>> @@ -371,6 +410,16 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
>>      highbank_binfo.loader_start = 0;
>>      highbank_binfo.write_secondary_boot = hb_write_secondary;
>>      highbank_binfo.secondary_cpu_reset_hook = hb_reset_secondary;
>> +    if (tcg_enabled()) {
>
> This also needs to be !kvm_enabled(), so 'make check' works.
>

Fixed.

>> +        highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
>> +        highbank_binfo.write_board_setup = hb_write_board_setup;
>> +        highbank_binfo.secure_board_setup = true;
>> +    } else {
>> +        error_report("WARNING: TCG unavailable - "
>> +                     "unable to load built-in Monitor support.\n"
>> +                     "Some guests (such as Linux) may not boot\n");
>
> You can't have newlines in an error_report() string. I suggest
>
>         error_report("WARNING: cannot load built-in Monitor support if KVM "
>                      "is enabled. Some guests (such as Linux) may not boot.");
>

Taken verbatim, although I line wrapped earlier due to new guideline
to stay a little back from 80 chars if possible.

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

Thanks.

Regards,
Peter

> thanks
> -- PMM