[PATCH 00/16] arm: Fix handling of unrecognized functions in PSCI emulation

[PATCH 00/16] arm: Fix handling of unrecognized functions in PSCI emulation

[Peter Maydell]

This series fixes our handling of PSCI calls where the function ID is
not recognized. These are supposed to return an error value, but
currently we instead emulate the SMC or HVC instruction to trap to the
guest at EL3 or EL2. Particularly of note for code review:
 * patches 4-9 include some "is this the right behaviour for
   this hardware" questions for the maintainers of those boards
 * patch 15 has a DTB API question, as well as being a change in
   what we edit in a DTB we are passed by the user
 * testing of the affected machines would be welcome

We tried to fix that bug in commit 9fcd15b9193e819b, but ran into
regressions and so reverted it in commit 4825eaae4fdd56fba0f for
release 7.0.  This series fixes the underlying problems causing the
regressions and reverts the revert.  It also fixes some other bugs
that were preventing booting of guests on the midway board and that
meant that the Linux kernel I tested couldn't bring up the secondary
CPUs when running with more than one guest CPU.

The regressions happened on boards which enabled PSCI emulation while
still running guest code at EL3. This used to work (as long as the
guest code itself wasn't trying to implement PSCI!)  because of the
fall-through-to-emulate-the-insn behaviour, but once the PSCI
emulation handles all SMC calls, most EL3 guest code will stop working
correctly. The solution this patchset adopts is to avoid enabling
QEMU's PSCI emulation when running guest code at EL3.

The affected boards are:
 * orangepi-pc, mcimx6ul-evk, mcimx7d-sabre, highbank, midway,
   xlnx-zcu102 (for any EL3 guest code)
 * xlnx-versal-virt (only for EL3 code run via -kernel)
 * virt (only for EL3 code run via -kernel or generic-loader)
For all these cases we will no longer enable PSCI emulation.
(This might in theory break guest code that used to work because
it was relying on running under QEMU and having the PSCI emulation
despite being at EL3 itself, but hopefully such code is rare.)

In order to only enable PSCI emulation when the guest is running at an
exception level lower than the level that our PSCI emulation
"firmware" would be running at, we make the arm_load_kernel() code in
boot.c responsible for setting the CPU properties psci-conduit and
start-powered-off. This is because only that code knows what EL it is
going to start the guest at (which depends on things like whether it
has decided that the guest is a Linux kernel or not).

The complicated case in all of this is the highbank and midway board
models, because of all the boards which enable QEMU's PSCI emulation
only these also use the boot.c secure_board_setup flag to say "run a
fragment of QEMU-provided boot code in the guest at EL3 to set
something up before running the guest at EL2 or EL1". That fragment of
code includes use of the SMC instruction, so when PSCI emulation
starts making that a NOP rather than a trap-to-guest-EL3 the setup
code will change behaviour. Fortunately, for this specific board's use
case the NOP is fine. The purpose of the setup code is to arrange that
unknown SMCs act as NOPs, because guest code may use a
highbank/midway-specific SMC ABI to enable the L2x0 cache
controller. So when the PSCI emulation starts to NOP the unknown SMCs
the setup code won't actively break, and the guest behaviour will
still be OK. (See patch 11's commit message for fuller details.)

Patches 1 and 2 make the relevant CPU properties settable after the
CPU object has been realized. This is necessary because
arm_load_kernel() runs very late, after the whole machine (including
the CPU objects) has been fully initialized.  (It was the restriction
on setting these properties before realize that previously led us to
set them in the SoC emulation code and thus to do it unconditionally.)

Patch 3 provides the "set up psci conduit" functionality in the boot.c
code; this is opt-in per board by setting a field in the arm_boot_info
struct.

Patches 4 to 9 move the individual boards across to using the new
approach. In each case I had to make a decision about the behaviour of
secondary CPUs when running guest firmware at EL3 -- should the
secondaries start off powered-down (waiting for the guest to power
them up via some kind of emulated power-control device), or powered-up
(so they all start running the firmware at once)? In a few cases I was
able to find the answer to this; in the rest I have erred on the side
of retaining the current "start powered down" behaviour, and added a
TODO comment to that effect. If you know the actual hardware
behaviour, let me know.

Patch 10 is the revert-the-revert patch.

Patch 11 removes the highbank/midway board use of the secure_board_setup
functionality; the commit message has the details about why this is safe.

Patches 12 to 14 are more minor cleanups that allow, and follow on from,
dropping the highbank-specific secondary CPU boot stub code.

Patch 15 is a change that is somewhat unrelated, but is necessary to
get the highbank board to successfully boot in SMP and to get the
midway board to start a Linux guest at all.

I have tested this with make check/check-acceptance and also with some
test images I have locally (including highbank and midway), but I
don't have test images for most of these boards, and in particular I
don't really have anything that exercises "run guest EL3 code" for
most of them. Testing would be welcome.

thanks
-- PMM

Peter Maydell (16):
  target/arm: make psci-conduit settable after realize
  cpu.c: Make start-powered-off settable after realize
  hw/arm/boot: Support setting psci-conduit based on guest EL
  hw/arm: imx: Don't enable PSCI conduit when booting guest in EL3
  hw/arm: allwinner: Don't enable PSCI conduit when booting guest in EL3
  hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in
    EL3
  hw/arm/versal: Let boot.c handle PSCI enablement
  hw/arm/virt: Let boot.c handle PSCI enablement
  hw/arm: highbank: For EL3 guests, don't enable PSCI, start all cores
  Revert "Revert "arm: tcg: Adhere to SMCCC 1.3 section 5.2""
  hw/arm/highbank: Drop use of secure_board_setup
  hw/arm/boot: Prevent setting both psci_conduit and secure_board_setup
  hw/arm/boot: Don't write secondary boot stub if using PSCI
  hw/arm/highbank: Drop unused secondary boot stub code
  hw/arm/boot: Drop nb_cpus field from arm_boot_info
  hw/arm/boot: Drop existing dtb /psci node rather than retaining it

 include/hw/arm/boot.h        |  14 ++++-
 include/hw/arm/xlnx-versal.h |   1 -
 cpu.c                        |  22 ++++++-
 hw/arm/allwinner-h3.c        |   9 ++-
 hw/arm/aspeed.c              |   1 -
 hw/arm/boot.c                | 107 +++++++++++++++++++++++++++++------
 hw/arm/exynos4_boards.c      |   1 -
 hw/arm/fsl-imx6ul.c          |   2 -
 hw/arm/fsl-imx7.c            |   8 +--
 hw/arm/highbank.c            |  72 +----------------------
 hw/arm/imx25_pdk.c           |   3 +-
 hw/arm/kzm.c                 |   1 -
 hw/arm/mcimx6ul-evk.c        |   2 +-
 hw/arm/mcimx7d-sabre.c       |   2 +-
 hw/arm/npcm7xx.c             |   3 -
 hw/arm/orangepi.c            |   5 +-
 hw/arm/raspi.c               |   1 -
 hw/arm/realview.c            |   1 -
 hw/arm/sabrelite.c           |   1 -
 hw/arm/sbsa-ref.c            |   1 -
 hw/arm/vexpress.c            |   1 -
 hw/arm/virt.c                |  13 +----
 hw/arm/xilinx_zynq.c         |   1 -
 hw/arm/xlnx-versal-virt.c    |   6 +-
 hw/arm/xlnx-versal.c         |   5 +-
 hw/arm/xlnx-zcu102.c         |   1 +
 hw/arm/xlnx-zynqmp.c         |  13 +++--
 target/arm/cpu.c             |   6 +-
 target/arm/psci.c            |  35 ++----------
 29 files changed, 164 insertions(+), 174 deletions(-)

-- 
2.25.1

[PATCH 01/16] target/arm: make psci-conduit settable after realize

[Peter Maydell]

We want to allow the psci-conduit property to be set after realize,
because the parts of the code which are best placed to decide if it's
OK to enable QEMU's builtin PSCI emulation (the board code and the
arm_load_kernel() function are distant from the code which creates
and realizes CPUs (typically inside an SoC object's init and realize
method) and run afterwards.

Since the DEFINE_PROP_* macros don't have support for creating
properties which can be changed after realize, change the property to
be created with object_property_add_uint32_ptr(), which is what we
already use in this function for creating settable-after-realize
properties like init-svtor and init-nsvtor.

Note that it doesn't conceptually make sense to change the setting of
the property after the machine has been completely initialized,
beacuse this would mean that the behaviour of the machine when first
started would differ from its behaviour when the system is
subsequently reset.  (It would also require the underlying state to
be migrated, which we don't do.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index cdbc4cdd012..5a9c02a2561 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1317,6 +1317,11 @@ void arm_cpu_post_init(Object *obj)
                                        OBJ_PROP_FLAG_READWRITE);
     }
 
+    /* Not DEFINE_PROP_UINT32: we want this to be settable after realize */
+    object_property_add_uint32_ptr(obj, "psci-conduit",
+                                   &cpu->psci_conduit,
+                                   OBJ_PROP_FLAG_READWRITE);
+
     qdev_property_add_static(DEVICE(obj), &arm_cpu_cfgend_property);
 
     if (arm_feature(&cpu->env, ARM_FEATURE_GENERIC_TIMER)) {
@@ -1987,7 +1992,6 @@ static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
 }
 
 static Property arm_cpu_properties[] = {
-    DEFINE_PROP_UINT32("psci-conduit", ARMCPU, psci_conduit, 0),
     DEFINE_PROP_UINT64("midr", ARMCPU, midr, 0),
     DEFINE_PROP_UINT64("mp-affinity", ARMCPU,
                         mp_affinity, ARM64_AFFINITY_INVALID),
-- 
2.25.1

[PATCH 02/16] cpu.c: Make start-powered-off settable after realize

[Peter Maydell]

The CPU object's start-powered-off property is currently only
settable before the CPU object is realized.  For arm machines this is
awkward, because we would like to decide whether the CPU should be
powered-off based on how we are booting the guest code, which is
something done in the machine model code and in common code called by
the machine model, which runs much later and in completely different
parts of the codebase from the SoC object code that is responsible
for creating and realizing the CPU objects.

Allow start-powered-off to be set after realize.  Since this isn't
something that's supported by the DEFINE_PROP_* macros, we have to
switch the property definition to use the
object_class_property_add_bool() function.

Note that it doesn't conceptually make sense to change the setting of
the property after the machine has been completely initialized,
beacuse this would mean that the behaviour of the machine when first
started would differ from its behaviour when the system is
subsequently reset.  (It would also require the underlying state to
be migrated, which we don't do.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 cpu.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/cpu.c b/cpu.c
index 016bf06a1ae..3ea38aea707 100644
--- a/cpu.c
+++ b/cpu.c
@@ -196,13 +196,33 @@ static Property cpu_common_props[] = {
     DEFINE_PROP_LINK("memory", CPUState, memory, TYPE_MEMORY_REGION,
                      MemoryRegion *),
 #endif
-    DEFINE_PROP_BOOL("start-powered-off", CPUState, start_powered_off, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static bool cpu_get_start_powered_off(Object *obj, Error **errp)
+{
+    CPUState *cpu = CPU(obj);
+    return cpu->start_powered_off;
+}
+
+static void cpu_set_start_powered_off(Object *obj, bool value, Error **errp)
+{
+    CPUState *cpu = CPU(obj);
+    cpu->start_powered_off = value;
+}
+
 void cpu_class_init_props(DeviceClass *dc)
 {
+    ObjectClass *oc = OBJECT_CLASS(dc);
+
     device_class_set_props(dc, cpu_common_props);
+    /*
+     * We can't use DEFINE_PROP_BOOL in the Property array for this
+     * property, because we want this to be settable after realize.
+     */
+    object_class_property_add_bool(oc, "start-powered-off",
+                                   cpu_get_start_powered_off,
+                                   cpu_set_start_powered_off);
 }
 
 void cpu_exec_initfn(CPUState *cpu)
-- 
2.25.1

[PATCH 03/16] hw/arm/boot: Support setting psci-conduit based on guest EL

[Peter Maydell]

Currently we expect board code to set the psci-conduit property on
CPUs and ensure that secondary CPUs are created with the
start-powered-off property set to false, if the board wishes to use
QEMU's builtin PSCI emulation.  This worked OK for the virt board
where we first wanted to use it, because the virt board directly
creates its CPUs and is in a reasonable position to set those
properties.  For other boards which model real hardware and use a
separate SoC object, however, it is more awkward.  Most PSCI-using
boards just set the psci-conduit board unconditionally.

This was never strictly speaking correct (because you would not be
able to run EL3 guest firmware that itself provided the PSCI
interface, as the QEMU implementation would overrule it), but mostly
worked in practice because for non-PSCI SMC calls QEMU would emulate
the SMC instruction as normal (by trapping to guest EL3).  However,
we would like to make our PSCI emulation follow the part of the SMCC
specification that mandates that SMC calls with unknown function
identifiers return a failure code, which means that all SMC calls
will be handled by the PSCI code and the "emulate as normal" path
will no longer be taken.

We tried to implement that in commit 9fcd15b9193e81
("arm: tcg: Adhere to SMCCC 1.3 section 5.2"), but this
regressed attempts to run EL3 guest code on the affected boards:
 * mcimx6ul-evk, mcimx7d-sabre, orangepi, xlnx-zcu102
 * for the case only of EL3 code loaded via -kernel (and
   not via -bios or -pflash), virt and xlnx-versal-virt
so for the 7.0 release we reverted it (in commit 4825eaae4fdd56f).

This commit provides a mechanism that boards can use to arrange that
psci-conduit is set if running guest code at a low enough EL but not
if it would be running at the same EL that the conduit implies that
the QEMU PSCI implementation is using.  (Later commits will convert
individual board models to use this mechanism.)

We do this by moving the setting of the psci-conduit and
start-powered-off properties to arm_load_kernel().  Boards which want
to potentially use emulated PSCI must set a psci_conduit field in the
arm_boot_info struct to the type of conduit they want to use (SMC or
HVC); arm_load_kernel() will then set the CPUs up accordingly if it
is not going to start the guest code at the same or higher EL as the
fake QEMU firmware would be at.

Board/SoC code which uses this mechanism should no longer set the CPU
psci-conduit property directly.  It should only set the
start-powered-off property for secondaries if EL3 guest firmware
running bare metal expects that rather than the alternative "all CPUs
start executing the firmware at once".

Note that when calculating whether we are going to run guest
code at EL3, we ignore the setting of arm_boot_info::secure_board_setup,
which might cause us to run a stub bit of guest code at EL3 which
does some board-specific setup before dropping to EL2 or EL1 to
run the guest kernel. This is OK because only one board that
enables PSCI sets secure_board_setup (the highbank board), and
the stub code it writes will behave the same way whether the
one SMC call it makes is handled by "emulate the SMC" or by
"PSCI default returns an error code". So we can leave that stub
code in place until after we've changed the PSCI default behaviour;
at that point we will remove it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/boot.h | 10 +++++++++
 hw/arm/boot.c         | 50 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/include/hw/arm/boot.h b/include/hw/arm/boot.h
index ce2b48b88bc..0bcb58babba 100644
--- a/include/hw/arm/boot.h
+++ b/include/hw/arm/boot.h
@@ -86,6 +86,16 @@ struct arm_boot_info {
      * the user it should implement this hook.
      */
     void (*modify_dtb)(const struct arm_boot_info *info, void *fdt);
+    /*
+     * If a board wants to use the QEMU emulated-firmware PSCI support,
+     * it should set this to QEMU_PSCI_CONDUIT_HVC or QEMU_PSCI_CONDUIT_SMC
+     * as appropriate. arm_load_kernel() will set the psci-conduit and
+     * start-powered-off properties on the CPUs accordingly.
+     * Note that if the guest image is started at the same exception level
+     * as the conduit specifies calls should go to (eg guest firmware booted
+     * to EL3) then PSCI will not be enabled.
+     */
+    int psci_conduit;
     /* Used internally by arm_boot.c */
     int is_linux;
     hwaddr initrd_start;
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 399f8e837ce..327e449f831 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1299,6 +1299,8 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
 {
     CPUState *cs;
     AddressSpace *as = arm_boot_address_space(cpu, info);
+    int boot_el;
+    CPUARMState *env = &cpu->env;
 
     /*
      * CPU objects (unlike devices) are not automatically reset on system
@@ -1329,6 +1331,54 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
         arm_setup_direct_kernel_boot(cpu, info);
     }
 
+    /*
+     * Disable the PSCI conduit if it is set up to target the same
+     * or a lower EL than the one we're going to start the guest code in.
+     * This logic needs to agree with the code in do_cpu_reset() which
+     * decides whether we're going to boot the guest in the highest
+     * supported exception level or in a lower one.
+     */
+
+    /* Boot into highest supported EL ... */
+    if (arm_feature(env, ARM_FEATURE_EL3)) {
+        boot_el = 3;
+    } else if (arm_feature(env, ARM_FEATURE_EL2)) {
+        boot_el = 2;
+    } else {
+        boot_el = 1;
+    }
+    /* ...except that if we're booting Linux we adjust the EL we boot into */
+    if (info->is_linux && !info->secure_boot) {
+        boot_el = arm_feature(env, ARM_FEATURE_EL2) ? 2 : 1;
+    }
+
+    if ((info->psci_conduit == QEMU_PSCI_CONDUIT_HVC && boot_el >= 2) ||
+        (info->psci_conduit == QEMU_PSCI_CONDUIT_SMC && boot_el == 3)) {
+        info->psci_conduit = QEMU_PSCI_CONDUIT_DISABLED;
+    }
+
+    if (info->psci_conduit != QEMU_PSCI_CONDUIT_DISABLED) {
+        for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
+            Object *cpuobj = OBJECT(cs);
+
+            object_property_set_int(cpuobj, "psci-conduit", info->psci_conduit,
+                                    &error_abort);
+            /*
+             * Secondary CPUs start in PSCI powered-down state. Like the
+             * code in do_cpu_reset(), we assume first_cpu is the primary
+             * CPU.
+             */
+            if (cs != first_cpu) {
+                object_property_set_bool(cpuobj, "start-powered-off", true,
+                                         &error_abort);
+            }
+        }
+    }
+
+    /*
+     * arm_load_dtb() may add a PSCI node so it must be called after we have
+     * decided whether to enable PSCI and set the psci-conduit CPU properties.
+     */
     if (!info->skip_dtb_autoload && have_dtb(info)) {
         if (arm_load_dtb(info->dtb_start, info, info->dtb_limit, as, ms) < 0) {
             exit(1);
-- 
2.25.1

[PATCH 04/16] hw/arm: imx: Don't enable PSCI conduit when booting guest in EL3

[Peter Maydell]

Change the iMX-SoC based boards to use the new boot.c functionality
to allow us to enable psci-conduit only if the guest is being booted
in EL1 or EL2, so that if the user runs guest EL3 firmware code our
PSCI emulation doesn't get in its way.

To do this we stop setting the psci-conduit property on the CPU
objects in the SoC code, and instead set the psci_conduit field in
the arm_boot_info struct to tell the common boot loader code that
we'd like PSCI if the guest is starting at an EL that it makes
sense with.

This affects the mcimx6ul-evk and mcimx7d-sabre boards.

Note that for the mcimx7d board, this means that when running guest
code at EL3 there is currently no way to power on the secondary CPUs,
because we do not currently have a model of the system reset
controller module which should be used to do that for the imx7 SoC,
only for the imx6 SoC.  (Previously EL3 code which knew it was
running on QEMU could use a PSCI call to do this.) This doesn't
affect the imx6ul-evk board because it is uniprocessor.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I don't have the i.mx7 manual to hand, so I'm partly making
assumptions based on the i.mx6 behaviour. If somebody with the
manual could double-check that it does indeed start up with the
secondary CPUs powered down via the SRC that would be great.
---
 hw/arm/fsl-imx6ul.c    | 2 --
 hw/arm/fsl-imx7.c      | 8 ++++----
 hw/arm/mcimx6ul-evk.c  | 1 +
 hw/arm/mcimx7d-sabre.c | 1 +
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
index 1d1a708dd97..f1897123294 100644
--- a/hw/arm/fsl-imx6ul.c
+++ b/hw/arm/fsl-imx6ul.c
@@ -166,8 +166,6 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    object_property_set_int(OBJECT(&s->cpu), "psci-conduit",
-                            QEMU_PSCI_CONDUIT_SMC, &error_abort);
     qdev_realize(DEVICE(&s->cpu), NULL, &error_abort);
 
     /*
diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
index 149885f2b80..cc6fdb9373f 100644
--- a/hw/arm/fsl-imx7.c
+++ b/hw/arm/fsl-imx7.c
@@ -159,9 +159,6 @@ static void fsl_imx7_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < smp_cpus; i++) {
         o = OBJECT(&s->cpu[i]);
 
-        object_property_set_int(o, "psci-conduit", QEMU_PSCI_CONDUIT_SMC,
-                                &error_abort);
-
         /* On uniprocessor, the CBAR is set to 0 */
         if (smp_cpus > 1) {
             object_property_set_int(o, "reset-cbar", FSL_IMX7_A7MPCORE_ADDR,
@@ -169,7 +166,10 @@ static void fsl_imx7_realize(DeviceState *dev, Error **errp)
         }
 
         if (i) {
-            /* Secondary CPUs start in PSCI powered-down state */
+            /*
+             * Secondary CPUs start in powered-down state (and can be
+             * powered up via the SRC system reset controller)
+             */
             object_property_set_bool(o, "start-powered-off", true,
                                      &error_abort);
         }
diff --git a/hw/arm/mcimx6ul-evk.c b/hw/arm/mcimx6ul-evk.c
index 28b4886f48b..8131518426a 100644
--- a/hw/arm/mcimx6ul-evk.c
+++ b/hw/arm/mcimx6ul-evk.c
@@ -35,6 +35,7 @@ static void mcimx6ul_evk_init(MachineState *machine)
         .board_id = -1,
         .ram_size = machine->ram_size,
         .nb_cpus = machine->smp.cpus,
+        .psci_conduit = QEMU_PSCI_CONDUIT_SMC,
     };
 
     s = FSL_IMX6UL(object_new(TYPE_FSL_IMX6UL));
diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
index 50a5ecde31a..ba84fc21920 100644
--- a/hw/arm/mcimx7d-sabre.c
+++ b/hw/arm/mcimx7d-sabre.c
@@ -37,6 +37,7 @@ static void mcimx7d_sabre_init(MachineState *machine)
         .board_id = -1,
         .ram_size = machine->ram_size,
         .nb_cpus = machine->smp.cpus,
+        .psci_conduit = QEMU_PSCI_CONDUIT_SMC,
     };
 
     s = FSL_IMX7(object_new(TYPE_FSL_IMX7));
-- 
2.25.1

[PATCH 05/16] hw/arm: allwinner: Don't enable PSCI conduit when booting guest in EL3

[Peter Maydell]

Change the allwinner-h3 based board to use the new boot.c
functionality to allow us to enable psci-conduit only if the guest is
being booted in EL1 or EL2, so that if the user runs guest EL3
firmware code our PSCI emulation doesn't get in its way.

To do this we stop setting the psci-conduit property on the CPU
objects in the SoC code, and instead set the psci_conduit field in
the arm_boot_info struct to tell the common boot loader code that
we'd like PSCI if the guest is starting at an EL that it makes sense
with.

This affects the orangepi-pc board.

This commit leaves the secondary CPUs in the powered-down state if
the guest is booting at EL3, which is the same behaviour as before
this commit.  The secondaries can no longer be started by that EL3
code making a PSCI call but can still be started via the CPU
Configuration Module registers (which we model in
hw/misc/allwinner-cpucfg.c).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
If anybody knows for definite that the secondaries should be
powered-down at startup for guest firmware, we can delete the TODO.
The allwinner-cpucfg.c code makes the reset value for the
REG_CPU*_RST_CTRL registers "CPUX_RESET_RELEASED", which might
suggest otherwise, but that could easily just be a QEMU error.
---
 hw/arm/allwinner-h3.c | 9 ++++-----
 hw/arm/orangepi.c     | 1 +
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index f9b7ed18711..f54aff6d2d2 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -235,11 +235,10 @@ static void allwinner_h3_realize(DeviceState *dev, Error **errp)
     /* CPUs */
     for (i = 0; i < AW_H3_NUM_CPUS; i++) {
 
-        /* Provide Power State Coordination Interface */
-        qdev_prop_set_int32(DEVICE(&s->cpus[i]), "psci-conduit",
-                            QEMU_PSCI_CONDUIT_SMC);
-
-        /* Disable secondary CPUs */
+        /*
+         * Disable secondary CPUs. TODO: check whether this is what
+         * guest EL3 firmware would really expect.
+         */
         qdev_prop_set_bit(DEVICE(&s->cpus[i]), "start-powered-off",
                           i > 0);
 
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
index e7963822367..68fe9182414 100644
--- a/hw/arm/orangepi.c
+++ b/hw/arm/orangepi.c
@@ -105,6 +105,7 @@ static void orangepi_init(MachineState *machine)
     }
     orangepi_binfo.loader_start = h3->memmap[AW_H3_DEV_SDRAM];
     orangepi_binfo.ram_size = machine->ram_size;
+    orangepi_binfo.psci_conduit = QEMU_PSCI_CONDUIT_SMC;
     arm_load_kernel(ARM_CPU(first_cpu), machine, &orangepi_binfo);
 }
 
-- 
2.25.1

[PATCH 06/16] hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in EL3

[Peter Maydell]

Change the Xilinx ZynqMP-based board xlnx-zcu102 to use the new
boot.c functionality to allow us to enable psci-conduit only if
the guest is being booted in EL1 or EL2, so that if the user runs
guest EL3 firmware code our PSCI emulation doesn't get in its
way.

To do this we stop setting the psci-conduit property on the CPU
objects in the SoC code, and instead set the psci_conduit field in
the arm_boot_info struct to tell the common boot loader code that
we'd like PSCI if the guest is starting at an EL that it makes
sense with.

Note that this means that EL3 guest code will have no way
to power on secondary cores, because we don't model any
kind of power controller that does that on this SoC.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Again, if anybody knows the real-hardware EL3 behaviour for
CPUs that would be great.
---
 hw/arm/xlnx-zcu102.c |  1 +
 hw/arm/xlnx-zynqmp.c | 13 ++++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
index 45eb19ab3b7..4c84bb932aa 100644
--- a/hw/arm/xlnx-zcu102.c
+++ b/hw/arm/xlnx-zcu102.c
@@ -236,6 +236,7 @@ static void xlnx_zcu102_init(MachineState *machine)
     s->binfo.ram_size = ram_size;
     s->binfo.loader_start = 0;
     s->binfo.modify_dtb = zcu102_modify_dtb;
+    s->binfo.psci_conduit = QEMU_PSCI_CONDUIT_SMC;
     arm_load_kernel(s->soc.boot_cpu_ptr, machine, &s->binfo);
 }
 
diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
index 1c52a575aad..17305fe7b76 100644
--- a/hw/arm/xlnx-zynqmp.c
+++ b/hw/arm/xlnx-zynqmp.c
@@ -215,7 +215,10 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s,
 
         name = object_get_canonical_path_component(OBJECT(&s->rpu_cpu[i]));
         if (strcmp(name, boot_cpu)) {
-            /* Secondary CPUs start in PSCI powered-down state */
+            /*
+             * Secondary CPUs start in powered-down state.
+             * TODO: check this is what EL3 firmware expects.
+             */
             object_property_set_bool(OBJECT(&s->rpu_cpu[i]),
                                      "start-powered-off", true, &error_abort);
         } else {
@@ -435,12 +438,12 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < num_apus; i++) {
         const char *name;
 
-        object_property_set_int(OBJECT(&s->apu_cpu[i]), "psci-conduit",
-                                QEMU_PSCI_CONDUIT_SMC, &error_abort);
-
         name = object_get_canonical_path_component(OBJECT(&s->apu_cpu[i]));
         if (strcmp(name, boot_cpu)) {
-            /* Secondary CPUs start in PSCI powered-down state */
+            /*
+             * Secondary CPUs start in powered-down state.
+             * TODO: check this is what EL3 firmware expects.
+             */
             object_property_set_bool(OBJECT(&s->apu_cpu[i]),
                                      "start-powered-off", true, &error_abort);
         } else {
-- 
2.25.1

[PATCH 07/16] hw/arm/versal: Let boot.c handle PSCI enablement

[Peter Maydell]

Instead of setting the CPU psci-conduit and start-powered-off
properties in the xlnx-versal-virt board code, set the arm_boot_info
psci_conduit field so that the boot.c code can do it.

This will fix a corner case where we were incorrectly enabling PSCI
emulation when booting guest code into EL3 because it was an ELF file
passed to -kernel.  (EL3 guest code started via -bios, -pflash, or
the generic loader was already being run with PSCI emulation
disabled.)

Note that EL3 guest code has no way to turn on the secondary CPUs
because there's no emulated power controller, but this was already
true for EL3 guest code run via -bios, -pflash, or the generic
loader.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/xlnx-versal.h | 1 -
 hw/arm/xlnx-versal-virt.c    | 6 ++++--
 hw/arm/xlnx-versal.c         | 5 +----
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
index 895ba12c61e..2de487564e4 100644
--- a/include/hw/arm/xlnx-versal.h
+++ b/include/hw/arm/xlnx-versal.h
@@ -89,7 +89,6 @@ struct Versal {
 
     struct {
         MemoryRegion *mr_ddr;
-        uint32_t psci_conduit;
     } cfg;
 };
 
diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
index 0c5edc898e1..1b25342501b 100644
--- a/hw/arm/xlnx-versal-virt.c
+++ b/hw/arm/xlnx-versal-virt.c
@@ -626,6 +626,9 @@ static void versal_virt_init(MachineState *machine)
      * When loading an OS, we turn on QEMU's PSCI implementation with SMC
      * as the PSCI conduit. When there's no -kernel, we assume the user
      * provides EL3 firmware to handle PSCI.
+     *
+     * Even if the user provides a kernel filename, arm_load_kernel()
+     * may suppress PSCI if it's going to boot that guest code at EL3.
      */
     if (machine->kernel_filename) {
         psci_conduit = QEMU_PSCI_CONDUIT_SMC;
@@ -635,8 +638,6 @@ static void versal_virt_init(MachineState *machine)
                             TYPE_XLNX_VERSAL);
     object_property_set_link(OBJECT(&s->soc), "ddr", OBJECT(machine->ram),
                              &error_abort);
-    object_property_set_int(OBJECT(&s->soc), "psci-conduit", psci_conduit,
-                            &error_abort);
     sysbus_realize(SYS_BUS_DEVICE(&s->soc), &error_fatal);
 
     fdt_create(s);
@@ -677,6 +678,7 @@ static void versal_virt_init(MachineState *machine)
     s->binfo.loader_start = 0x0;
     s->binfo.get_dtb = versal_virt_get_dtb;
     s->binfo.modify_dtb = versal_virt_modify_dtb;
+    s->binfo.psci_conduit = psci_conduit;
     if (machine->kernel_filename) {
         arm_load_kernel(&s->soc.fpd.apu.cpu[0], machine, &s->binfo);
     } else {
diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
index b2705b6925e..458ba33815f 100644
--- a/hw/arm/xlnx-versal.c
+++ b/hw/arm/xlnx-versal.c
@@ -35,10 +35,8 @@ static void versal_create_apu_cpus(Versal *s)
         object_initialize_child(OBJECT(s), "apu-cpu[*]", &s->fpd.apu.cpu[i],
                                 XLNX_VERSAL_ACPU_TYPE);
         obj = OBJECT(&s->fpd.apu.cpu[i]);
-        object_property_set_int(obj, "psci-conduit", s->cfg.psci_conduit,
-                                &error_abort);
         if (i) {
-            /* Secondary CPUs start in PSCI powered-down state */
+            /* Secondary CPUs start in powered-down state */
             object_property_set_bool(obj, "start-powered-off", true,
                                      &error_abort);
         }
@@ -481,7 +479,6 @@ static void versal_init(Object *obj)
 static Property versal_properties[] = {
     DEFINE_PROP_LINK("ddr", Versal, cfg.mr_ddr, TYPE_MEMORY_REGION,
                      MemoryRegion *),
-    DEFINE_PROP_UINT32("psci-conduit", Versal, cfg.psci_conduit, 0),
     DEFINE_PROP_END_OF_LIST()
 };
 
-- 
2.25.1

[PATCH 08/16] hw/arm/virt: Let boot.c handle PSCI enablement

[Peter Maydell]

Instead of setting the CPU psci-conduit and start-powered-off
properties in the virt board code, set the arm_boot_info psci_conduit
field so that the boot.c code can do it.

This will fix a corner case where we were incorrectly enabling PSCI
emulation when booting guest code into EL3 because it was an ELF file
passed to -kernel or to the generic loader.  (EL3 guest code started
via -bios or -pflash was already being run with PSCI emulation
disabled.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 141350bf215..398145a7180 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2088,17 +2088,6 @@ static void machvirt_init(MachineState *machine)
             object_property_set_bool(cpuobj, "has_el2", false, NULL);
         }
 
-        if (vms->psci_conduit != QEMU_PSCI_CONDUIT_DISABLED) {
-            object_property_set_int(cpuobj, "psci-conduit", vms->psci_conduit,
-                                    NULL);
-
-            /* Secondary CPUs start in PSCI powered-down state */
-            if (n > 0) {
-                object_property_set_bool(cpuobj, "start-powered-off", true,
-                                         NULL);
-            }
-        }
-
         if (vmc->kvm_no_adjvtime &&
             object_property_find(cpuobj, "kvm-no-adjvtime")) {
             object_property_set_bool(cpuobj, "kvm-no-adjvtime", true, NULL);
@@ -2246,6 +2235,7 @@ static void machvirt_init(MachineState *machine)
     vms->bootinfo.get_dtb = machvirt_dtb;
     vms->bootinfo.skip_dtb_autoload = true;
     vms->bootinfo.firmware_loaded = firmware_loaded;
+    vms->bootinfo.psci_conduit = vms->psci_conduit;
     arm_load_kernel(ARM_CPU(first_cpu), machine, &vms->bootinfo);
 
     vms->machine_done.notify = virt_machine_done;
-- 
2.25.1

[PATCH 09/16] hw/arm: highbank: For EL3 guests, don't enable PSCI, start all cores

[Peter Maydell]

Change the highbank/midway boards to use the new boot.c functionality
to allow us to enable psci-conduit only if the guest is being booted
in EL1 or EL2, so that if the user runs guest EL3 firmware code our
PSCI emulation doesn't get in its way.

To do this we stop setting the psci-conduit and start-powered-off
properties on the CPU objects in the board code, and instead set the
psci_conduit field in the arm_boot_info struct to tell the common
boot loader code that we'd like PSCI if the guest is starting at an
EL that it makes sense with (in which case it will set these
properties).

This means that when running guest code at EL3, all the cores
will start execution at once on poweron. This matches the
real hardware behaviour. (A brief description of the hardware
boot process is in the u-boot documentation for these boards:
https://u-boot.readthedocs.io/en/latest/board/highbank/highbank.html#boot-process
 -- in theory one might run the 'a9boot'/'a15boot' secure monitor
code in QEMU, though we probably don't emulate enough for that.)

This affects the highbank and midway boards.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/highbank.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index 4210894d814..048f8550cb9 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -271,12 +271,6 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
         object_property_set_int(cpuobj, "psci-conduit", QEMU_PSCI_CONDUIT_SMC,
                                 &error_abort);
 
-        if (n) {
-            /* Secondary CPUs start in PSCI powered-down state */
-            object_property_set_bool(cpuobj, "start-powered-off", true,
-                                     &error_abort);
-        }
-
         if (object_property_find(cpuobj, "reset-cbar")) {
             object_property_set_int(cpuobj, "reset-cbar", MPCORE_PERIPHBASE,
                                     &error_abort);
@@ -397,6 +391,7 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
     highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
     highbank_binfo.write_board_setup = hb_write_board_setup;
     highbank_binfo.secure_board_setup = true;
+    highbank_binfo.psci_conduit = QEMU_PSCI_CONDUIT_SMC;
 
     arm_load_kernel(ARM_CPU(first_cpu), machine, &highbank_binfo);
 }
-- 
2.25.1

[PATCH 10/16] Revert "Revert "arm: tcg: Adhere to SMCCC 1.3 section 5.2""

[Peter Maydell]

Now that we have arranged for all the affected board models
to not enable the PSCI emulation if they are running guest code
at EL3, we can revert commit 4825eaae4fdd56fba0f, thus
reinstating commit 9fcd15b9193e819b, without bringing back the
regressions that caused us to revert it.

For clarity, here is the original commit message of 9fcd15b9193e819b:

The SMCCC 1.3 spec section 5.2 says

  The Unknown SMC Function Identifier is a sign-extended value of (-1)
  that is returned in the R0, W0 or X0 registers. An implementation must
  return this error code when it receives:

    * An SMC or HVC call with an unknown Function Identifier
    * An SMC or HVC call for a removed Function Identifier
    * An SMC64/HVC64 call from AArch32 state

To comply with these statements, let's always return -1 when we encounter
an unknown HVC or SMC call.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/psci.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/target/arm/psci.c b/target/arm/psci.c
index 6709e280133..b279c0b9a45 100644
--- a/target/arm/psci.c
+++ b/target/arm/psci.c
@@ -27,15 +27,13 @@
 
 bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
 {
-    /* Return true if the r0/x0 value indicates a PSCI call and
-     * the exception type matches the configured PSCI conduit. This is
-     * called before the SMC/HVC instruction is executed, to decide whether
-     * we should treat it as a PSCI call or with the architecturally
+    /*
+     * Return true if the exception type matches the configured PSCI conduit.
+     * This is called before the SMC/HVC instruction is executed, to decide
+     * whether we should treat it as a PSCI call or with the architecturally
      * defined behaviour for an SMC or HVC (which might be UNDEF or trap
      * to EL2 or to EL3).
      */
-    CPUARMState *env = &cpu->env;
-    uint64_t param = is_a64(env) ? env->xregs[0] : env->regs[0];
 
     switch (excp_type) {
     case EXCP_HVC:
@@ -52,27 +50,7 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type)
         return false;
     }
 
-    switch (param) {
-    case QEMU_PSCI_0_2_FN_PSCI_VERSION:
-    case QEMU_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
-    case QEMU_PSCI_0_2_FN_AFFINITY_INFO:
-    case QEMU_PSCI_0_2_FN64_AFFINITY_INFO:
-    case QEMU_PSCI_0_2_FN_SYSTEM_RESET:
-    case QEMU_PSCI_0_2_FN_SYSTEM_OFF:
-    case QEMU_PSCI_0_1_FN_CPU_ON:
-    case QEMU_PSCI_0_2_FN_CPU_ON:
-    case QEMU_PSCI_0_2_FN64_CPU_ON:
-    case QEMU_PSCI_0_1_FN_CPU_OFF:
-    case QEMU_PSCI_0_2_FN_CPU_OFF:
-    case QEMU_PSCI_0_1_FN_CPU_SUSPEND:
-    case QEMU_PSCI_0_2_FN_CPU_SUSPEND:
-    case QEMU_PSCI_0_2_FN64_CPU_SUSPEND:
-    case QEMU_PSCI_0_1_FN_MIGRATE:
-    case QEMU_PSCI_0_2_FN_MIGRATE:
-        return true;
-    default:
-        return false;
-    }
+    return true;
 }
 
 void arm_handle_psci_call(ARMCPU *cpu)
@@ -194,10 +172,9 @@ void arm_handle_psci_call(ARMCPU *cpu)
         break;
     case QEMU_PSCI_0_1_FN_MIGRATE:
     case QEMU_PSCI_0_2_FN_MIGRATE:
+    default:
         ret = QEMU_PSCI_RET_NOT_SUPPORTED;
         break;
-    default:
-        g_assert_not_reached();
     }
 
 err:
-- 
2.25.1

[PATCH 11/16] hw/arm/highbank: Drop use of secure_board_setup

[Peter Maydell]

Guest code on highbank may make non-PSCI SMC calls in order to
enable/disable the L2x0 cache controller (see the Linux kernel's
arch/arm/mach-highbank/highbank.c highbank_l2c310_write_sec()
function).  The ABI for this is documented in kernel commit
8e56130dcb as being borrowed from the OMAP44xx ROM.  The OMAP44xx TRM
documents this function ID as having no return value and potentially
trashing all guest registers except SP and PC. For QEMU's purposes
(where our L2x0 model is a stub and enabling or disabling it doesn't
affect the guest behaviour) a simple "do nothing" SMC is fine.

We currently implement this NOP behaviour using a little bit of
Secure code we run before jumping to the guest kernel, which is
written by arm_write_secure_board_setup_dummy_smc().  The code sets
up a set of Secure vectors where the SMC entry point returns without
doing anything.

Now that the PSCI SMC emulation handles all SMC calls (setting r0 to
an error code if the input r0 function identifier is not recognized),
we can use that default behaviour as sufficient for the highbank
cache controller call.  (Because the guest code assumes r0 has no
interesting value on exit it doesn't matter that we set it to the
error code).  We can therefore delete the highbank board code that
sets secure_board_setup to true and writes the secure-code bootstub.

(Note that because the OMAP44xx ABI puts function-identifiers in
r12 and PSCI uses r0, we only avoid a clash because Linux's code
happens to put the function-identifier in both registers. But this
is true also when the kernel is running on real firmware that
implements both ABIs as far as I can see.)

This change fixes in passing booting on the 'midway' board model,
which has been completely broken since we added support for Hyp
mode to the Cortex-A15 CPU. When we did that boot.c was made to
start running the guest code in Hyp mode; this includes the
board_setup hook, which instantly UNDEFs because the NSACR is
not accessible from Hyp. (Put another way, we never made the
secure_board_setup hook support cope with Hyp mode.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/highbank.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index 048f8550cb9..a21afd178d1 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -48,12 +48,6 @@
 
 /* Board init.  */
 
-static void hb_write_board_setup(ARMCPU *cpu,
-                                 const struct arm_boot_info *info)
-{
-    arm_write_secure_board_setup_dummy_smc(cpu, info, MVBAR_ADDR);
-}
-
 static void hb_write_secondary(ARMCPU *cpu, const struct arm_boot_info *info)
 {
     int n;
@@ -389,8 +383,6 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
     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;
     highbank_binfo.psci_conduit = QEMU_PSCI_CONDUIT_SMC;
 
     arm_load_kernel(ARM_CPU(first_cpu), machine, &highbank_binfo);
-- 
2.25.1

[PATCH 12/16] hw/arm/boot: Prevent setting both psci_conduit and secure_board_setup

[Peter Maydell]

Now that we have dealt with the one special case (highbank) that needed
to set both psci_conduit and secure_board_setup, we don't need to
allow that combination any more. It doesn't make sense in general,
so use an assertion to ensure we don't add new boards that do it
by accident without thinking through the consequences.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 327e449f831..0424c178305 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1339,6 +1339,16 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
      * supported exception level or in a lower one.
      */
 
+    /*
+     * If PSCI is enabled, then SMC calls all go to the PSCI handler and
+     * are never emulated to trap into guest code. It therefore does not
+     * make sense for the board to have a setup code fragment that runs
+     * in Secure, because this will probably need to itself issue an SMC of some
+     * kind as part of its operation.
+     */
+    assert(info->psci_conduit == QEMU_PSCI_CONDUIT_DISABLED ||
+           !info->secure_board_setup);
+
     /* Boot into highest supported EL ... */
     if (arm_feature(env, ARM_FEATURE_EL3)) {
         boot_el = 3;
-- 
2.25.1

[PATCH 13/16] hw/arm/boot: Don't write secondary boot stub if using PSCI

[Peter Maydell]

If we're using PSCI emulation to start secondary CPUs, there is no
point in writing the "secondary boot" stub code, because it will
never be used -- secondary CPUs start powered-off, and when powered
on are set to begin execution at the address specified by the guest's
power-on PSCI call, not at the stub.

Move the call to the hook that writes the secondary boot stub code so
that we can do it only if we're starting a Linux kernel and not using
PSCI.

(None of the users of the hook care about the ordering of its call
relative to anything else: they only use it to write a rom blob to
guest memory.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/boot.h |  3 +++
 hw/arm/boot.c         | 35 ++++++++++++++++++++++++-----------
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/include/hw/arm/boot.h b/include/hw/arm/boot.h
index 0bcb58babba..0cfc1c95c4e 100644
--- a/include/hw/arm/boot.h
+++ b/include/hw/arm/boot.h
@@ -70,6 +70,9 @@ struct arm_boot_info {
      * boot loader/boot ROM code, and secondary_cpu_reset_hook() should
      * perform any necessary CPU reset handling and set the PC for the
      * secondary CPUs to point at this boot blob.
+     *
+     * These hooks won't be called if secondary CPUs are booting via
+     * emulated PSCI (see psci_conduit below).
      */
     void (*write_secondary_boot)(ARMCPU *cpu,
                                  const struct arm_boot_info *info);
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 0424c178305..184628ce564 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -804,7 +804,7 @@ static void do_cpu_reset(void *opaque)
                         set_kernel_args(info, as);
                     }
                 }
-            } else {
+            } else if (info->secondary_cpu_reset_hook) {
                 info->secondary_cpu_reset_hook(cpu, info);
             }
         }
@@ -1030,13 +1030,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
         elf_machine = EM_ARM;
     }
 
-    if (!info->secondary_cpu_reset_hook) {
-        info->secondary_cpu_reset_hook = default_reset_secondary;
-    }
-    if (!info->write_secondary_boot) {
-        info->write_secondary_boot = default_write_secondary;
-    }
-
     if (info->nb_cpus == 0)
         info->nb_cpus = 1;
 
@@ -1216,9 +1209,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
         write_bootloader("bootloader", info->loader_start,
                          primary_loader, fixupcontext, as);
 
-        if (info->nb_cpus > 1) {
-            info->write_secondary_boot(cpu, info);
-        }
         if (info->write_board_setup) {
             info->write_board_setup(cpu, info);
         }
@@ -1385,6 +1375,29 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
         }
     }
 
+    if (info->psci_conduit == QEMU_PSCI_CONDUIT_DISABLED &&
+        info->is_linux && info->nb_cpus > 1) {
+        /*
+         * We're booting Linux but not using PSCI, so for SMP we need
+         * to write a custom secondary CPU boot loader stub, and arrange
+         * for the secondary CPU reset to make the accompanying initialization.
+         */
+        if (!info->secondary_cpu_reset_hook) {
+            info->secondary_cpu_reset_hook = default_reset_secondary;
+        }
+        if (!info->write_secondary_boot) {
+            info->write_secondary_boot = default_write_secondary;
+        }
+        info->write_secondary_boot(cpu, info);
+    } else {
+        /*
+         * No secondary boot stub; don't use the reset hook that would
+         * have set the CPU up to call it
+         */
+        info->write_secondary_boot = NULL;
+        info->secondary_cpu_reset_hook = NULL;
+    }
+
     /*
      * arm_load_dtb() may add a PSCI node so it must be called after we have
      * decided whether to enable PSCI and set the psci-conduit CPU properties.
-- 
2.25.1

[PATCH 14/16] hw/arm/highbank: Drop unused secondary boot stub code

[Peter Maydell]

The highbank and midway board code includes boot-stub code for
handling secondary CPU boot which keeps the secondaries in a pen
until the primary writes to a known location with the address they
should jump to.

This code is never used, because the boards enable QEMU's PSCI
emulation, so secondary CPUs are kept powered off until the PSCI call
which turns them on, and then start execution from the address given
by the guest in that PSCI call.  Delete the unreachable code.

(The code was wrong for midway in any case -- on the Cortex-A15 the
GIC CPU interface registers are at a different offset from PERIPHBASE
compared to the Cortex-A9, and the code baked-in the offsets for
highbank's A9.)

Note that this commit implicitly depends on the preceding "Don't
write secondary boot stub if using PSCI" commit -- the default
secondary-boot stub code overlaps with one of the highbank-specific
bootcode rom blobs, so we must suppress the secondary-boot
stub code entirely, not merely replace the highbank-specific
version with the default.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/highbank.c | 56 -----------------------------------------------
 1 file changed, 56 deletions(-)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index a21afd178d1..da681b15708 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -48,60 +48,6 @@
 
 /* Board init.  */
 
-static void hb_write_secondary(ARMCPU *cpu, const struct arm_boot_info *info)
-{
-    int n;
-    uint32_t smpboot[] = {
-        0xee100fb0, /* mrc p15, 0, r0, c0, c0, 5 - read current core id */
-        0xe210000f, /* ands r0, r0, #0x0f */
-        0xe3a03040, /* mov r3, #0x40 - jump address is 0x40 + 0x10 * core id */
-        0xe0830200, /* add r0, r3, r0, lsl #4 */
-        0xe59f2024, /* ldr r2, privbase */
-        0xe3a01001, /* mov r1, #1 */
-        0xe5821100, /* str r1, [r2, #256] - set GICC_CTLR.Enable */
-        0xe3a010ff, /* mov r1, #0xff */
-        0xe5821104, /* str r1, [r2, #260] - set GICC_PMR.Priority to 0xff */
-        0xf57ff04f, /* dsb */
-        0xe320f003, /* wfi */
-        0xe5901000, /* ldr     r1, [r0] */
-        0xe1110001, /* tst     r1, r1 */
-        0x0afffffb, /* beq     <wfi> */
-        0xe12fff11, /* bx      r1 */
-        MPCORE_PERIPHBASE   /* privbase: MPCore peripheral base address.  */
-    };
-    for (n = 0; n < ARRAY_SIZE(smpboot); n++) {
-        smpboot[n] = tswap32(smpboot[n]);
-    }
-    rom_add_blob_fixed_as("smpboot", smpboot, sizeof(smpboot), SMP_BOOT_ADDR,
-                          arm_boot_address_space(cpu, info));
-}
-
-static void hb_reset_secondary(ARMCPU *cpu, const struct arm_boot_info *info)
-{
-    CPUARMState *env = &cpu->env;
-
-    switch (info->nb_cpus) {
-    case 4:
-        address_space_stl_notdirty(&address_space_memory,
-                                   SMP_BOOT_REG + 0x30, 0,
-                                   MEMTXATTRS_UNSPECIFIED, NULL);
-        /* fallthrough */
-    case 3:
-        address_space_stl_notdirty(&address_space_memory,
-                                   SMP_BOOT_REG + 0x20, 0,
-                                   MEMTXATTRS_UNSPECIFIED, NULL);
-        /* fallthrough */
-    case 2:
-        address_space_stl_notdirty(&address_space_memory,
-                                   SMP_BOOT_REG + 0x10, 0,
-                                   MEMTXATTRS_UNSPECIFIED, NULL);
-        env->regs[15] = SMP_BOOT_ADDR;
-        break;
-    default:
-        break;
-    }
-}
-
 #define NUM_REGS      0x200
 static void hb_regs_write(void *opaque, hwaddr offset,
                           uint64_t value, unsigned size)
@@ -380,8 +326,6 @@ 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.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.psci_conduit = QEMU_PSCI_CONDUIT_SMC;
 
-- 
2.25.1

[PATCH 15/16] hw/arm/boot: Drop nb_cpus field from arm_boot_info

[Peter Maydell]

We use the arm_boot_info::nb_cpus field in only one place, and that
place can easily get the number of CPUs locally rather than relying
on the board code to have set the field correctly.  (At least one
board, xlnx-versal-virt, does not set the field despite having more
than one CPU.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/boot.h   | 1 -
 hw/arm/aspeed.c         | 1 -
 hw/arm/boot.c           | 7 +++----
 hw/arm/exynos4_boards.c | 1 -
 hw/arm/highbank.c       | 1 -
 hw/arm/imx25_pdk.c      | 3 +--
 hw/arm/kzm.c            | 1 -
 hw/arm/mcimx6ul-evk.c   | 1 -
 hw/arm/mcimx7d-sabre.c  | 1 -
 hw/arm/npcm7xx.c        | 3 ---
 hw/arm/orangepi.c       | 4 +---
 hw/arm/raspi.c          | 1 -
 hw/arm/realview.c       | 1 -
 hw/arm/sabrelite.c      | 1 -
 hw/arm/sbsa-ref.c       | 1 -
 hw/arm/vexpress.c       | 1 -
 hw/arm/virt.c           | 1 -
 hw/arm/xilinx_zynq.c    | 1 -
 18 files changed, 5 insertions(+), 26 deletions(-)

diff --git a/include/hw/arm/boot.h b/include/hw/arm/boot.h
index 0cfc1c95c4e..c7ebae156ec 100644
--- a/include/hw/arm/boot.h
+++ b/include/hw/arm/boot.h
@@ -56,7 +56,6 @@ struct arm_boot_info {
     hwaddr smp_loader_start;
     hwaddr smp_bootreg_addr;
     hwaddr gic_cpu_if_addr;
-    int nb_cpus;
     int board_id;
     /* ARM machines that support the ARM Security Extensions use this field to
      * control whether Linux is booted as secure(true) or non-secure(false).
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index cf20ae0db5e..d911dc904fb 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -431,7 +431,6 @@ static void aspeed_machine_init(MachineState *machine)
 
     aspeed_board_binfo.ram_size = machine->ram_size;
     aspeed_board_binfo.loader_start = sc->memmap[ASPEED_DEV_SDRAM];
-    aspeed_board_binfo.nb_cpus = sc->num_cpus;
 
     if (amc->i2c_init) {
         amc->i2c_init(bmc);
diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 184628ce564..b46f1fe889e 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -1030,9 +1030,6 @@ static void arm_setup_direct_kernel_boot(ARMCPU *cpu,
         elf_machine = EM_ARM;
     }
 
-    if (info->nb_cpus == 0)
-        info->nb_cpus = 1;
-
     /* Assume that raw images are linux kernels, and ELF images are not.  */
     kernel_size = arm_load_elf(info, &elf_entry, &image_low_addr,
                                &image_high_addr, elf_machine, as);
@@ -1291,6 +1288,7 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
     AddressSpace *as = arm_boot_address_space(cpu, info);
     int boot_el;
     CPUARMState *env = &cpu->env;
+    int nb_cpus = 0;
 
     /*
      * CPU objects (unlike devices) are not automatically reset on system
@@ -1300,6 +1298,7 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
      */
     for (cs = first_cpu; cs; cs = CPU_NEXT(cs)) {
         qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
+        nb_cpus++;
     }
 
     /*
@@ -1376,7 +1375,7 @@ void arm_load_kernel(ARMCPU *cpu, MachineState *ms, struct arm_boot_info *info)
     }
 
     if (info->psci_conduit == QEMU_PSCI_CONDUIT_DISABLED &&
-        info->is_linux && info->nb_cpus > 1) {
+        info->is_linux && nb_cpus > 1) {
         /*
          * We're booting Linux but not using PSCI, so for SMP we need
          * to write a custom secondary CPU boot loader stub, and arrange
diff --git a/hw/arm/exynos4_boards.c b/hw/arm/exynos4_boards.c
index 35dd9875da1..ef5bcbc212c 100644
--- a/hw/arm/exynos4_boards.c
+++ b/hw/arm/exynos4_boards.c
@@ -67,7 +67,6 @@ static unsigned long exynos4_board_ram_size[EXYNOS4_NUM_OF_BOARDS] = {
 static struct arm_boot_info exynos4_board_binfo = {
     .loader_start     = EXYNOS4210_BASE_BOOT_ADDR,
     .smp_loader_start = EXYNOS4210_SMP_BOOT_ADDR,
-    .nb_cpus          = EXYNOS4210_NCPUS,
     .write_secondary_boot = exynos4210_write_secondary,
 };
 
diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index da681b15708..f12aacea6b8 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -324,7 +324,6 @@ static void calxeda_init(MachineState *machine, enum cxmachines machine_id)
      * clear that the value is meaningless.
      */
     highbank_binfo.board_id = -1;
-    highbank_binfo.nb_cpus = smp_cpus;
     highbank_binfo.loader_start = 0;
     highbank_binfo.board_setup_addr = BOARD_SETUP_ADDR;
     highbank_binfo.psci_conduit = QEMU_PSCI_CONDUIT_SMC;
diff --git a/hw/arm/imx25_pdk.c b/hw/arm/imx25_pdk.c
index 6dff0001633..b4f7f4e8a7f 100644
--- a/hw/arm/imx25_pdk.c
+++ b/hw/arm/imx25_pdk.c
@@ -114,8 +114,7 @@ static void imx25_pdk_init(MachineState *machine)
 
     imx25_pdk_binfo.ram_size = machine->ram_size;
     imx25_pdk_binfo.loader_start = FSL_IMX25_SDRAM0_ADDR;
-    imx25_pdk_binfo.board_id = 1771,
-    imx25_pdk_binfo.nb_cpus = 1;
+    imx25_pdk_binfo.board_id = 1771;
 
     for (i = 0; i < FSL_IMX25_NUM_ESDHCS; i++) {
         BusState *bus;
diff --git a/hw/arm/kzm.c b/hw/arm/kzm.c
index 39559c44c29..b1b281c9acb 100644
--- a/hw/arm/kzm.c
+++ b/hw/arm/kzm.c
@@ -124,7 +124,6 @@ static void kzm_init(MachineState *machine)
     }
 
     kzm_binfo.ram_size = machine->ram_size;
-    kzm_binfo.nb_cpus = 1;
 
     if (!qtest_enabled()) {
         arm_load_kernel(&s->soc.cpu, machine, &kzm_binfo);
diff --git a/hw/arm/mcimx6ul-evk.c b/hw/arm/mcimx6ul-evk.c
index 8131518426a..d83c3c380e8 100644
--- a/hw/arm/mcimx6ul-evk.c
+++ b/hw/arm/mcimx6ul-evk.c
@@ -34,7 +34,6 @@ static void mcimx6ul_evk_init(MachineState *machine)
         .loader_start = FSL_IMX6UL_MMDC_ADDR,
         .board_id = -1,
         .ram_size = machine->ram_size,
-        .nb_cpus = machine->smp.cpus,
         .psci_conduit = QEMU_PSCI_CONDUIT_SMC,
     };
 
diff --git a/hw/arm/mcimx7d-sabre.c b/hw/arm/mcimx7d-sabre.c
index ba84fc21920..6182b15f190 100644
--- a/hw/arm/mcimx7d-sabre.c
+++ b/hw/arm/mcimx7d-sabre.c
@@ -36,7 +36,6 @@ static void mcimx7d_sabre_init(MachineState *machine)
         .loader_start = FSL_IMX7_MMDC_ADDR,
         .board_id = -1,
         .ram_size = machine->ram_size,
-        .nb_cpus = machine->smp.cpus,
         .psci_conduit = QEMU_PSCI_CONDUIT_SMC,
     };
 
diff --git a/hw/arm/npcm7xx.c b/hw/arm/npcm7xx.c
index 878c2208e07..d85cc027651 100644
--- a/hw/arm/npcm7xx.c
+++ b/hw/arm/npcm7xx.c
@@ -355,10 +355,7 @@ static struct arm_boot_info npcm7xx_binfo = {
 
 void npcm7xx_load_kernel(MachineState *machine, NPCM7xxState *soc)
 {
-    NPCM7xxClass *sc = NPCM7XX_GET_CLASS(soc);
-
     npcm7xx_binfo.ram_size = machine->ram_size;
-    npcm7xx_binfo.nb_cpus = sc->num_cpus;
 
     arm_load_kernel(&soc->cpu[0], machine, &npcm7xx_binfo);
 }
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
index 68fe9182414..3ace4748704 100644
--- a/hw/arm/orangepi.c
+++ b/hw/arm/orangepi.c
@@ -25,9 +25,7 @@
 #include "hw/qdev-properties.h"
 #include "hw/arm/allwinner-h3.h"
 
-static struct arm_boot_info orangepi_binfo = {
-    .nb_cpus = AW_H3_NUM_CPUS,
-};
+static struct arm_boot_info orangepi_binfo;
 
 static void orangepi_init(MachineState *machine)
 {
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index b4dd6c1e99a..92d068d1f9d 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -204,7 +204,6 @@ static void setup_boot(MachineState *machine, RaspiProcessorId processor_id,
 
     s->binfo.board_id = MACH_TYPE_BCM2708;
     s->binfo.ram_size = ram_size;
-    s->binfo.nb_cpus = machine->smp.cpus;
 
     if (processor_id <= PROCESSOR_ID_BCM2836) {
         /*
diff --git a/hw/arm/realview.c b/hw/arm/realview.c
index ddc70b54a56..7b424e94a5f 100644
--- a/hw/arm/realview.c
+++ b/hw/arm/realview.c
@@ -363,7 +363,6 @@ static void realview_init(MachineState *machine,
     memory_region_add_subregion(sysmem, SMP_BOOT_ADDR, ram_hack);
 
     realview_binfo.ram_size = ram_size;
-    realview_binfo.nb_cpus = smp_cpus;
     realview_binfo.board_id = realview_board_id[board_type];
     realview_binfo.loader_start = (board_type == BOARD_PB_A8 ? 0x70000000 : 0);
     arm_load_kernel(ARM_CPU(first_cpu), machine, &realview_binfo);
diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
index cce49aa25cf..41191245b81 100644
--- a/hw/arm/sabrelite.c
+++ b/hw/arm/sabrelite.c
@@ -93,7 +93,6 @@ static void sabrelite_init(MachineState *machine)
     }
 
     sabrelite_binfo.ram_size = machine->ram_size;
-    sabrelite_binfo.nb_cpus = machine->smp.cpus;
     sabrelite_binfo.secure_boot = true;
     sabrelite_binfo.write_secondary_boot = sabrelite_write_secondary;
     sabrelite_binfo.secondary_cpu_reset_hook = sabrelite_reset_secondary;
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index dd944553f78..23874019639 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -776,7 +776,6 @@ static void sbsa_ref_init(MachineState *machine)
     create_secure_ec(secure_sysmem);
 
     sms->bootinfo.ram_size = machine->ram_size;
-    sms->bootinfo.nb_cpus = smp_cpus;
     sms->bootinfo.board_id = -1;
     sms->bootinfo.loader_start = sbsa_ref_memmap[SBSA_MEM].base;
     sms->bootinfo.get_dtb = sbsa_ref_dtb;
diff --git a/hw/arm/vexpress.c b/hw/arm/vexpress.c
index 3e99b7918ab..e1d1983ae65 100644
--- a/hw/arm/vexpress.c
+++ b/hw/arm/vexpress.c
@@ -708,7 +708,6 @@ static void vexpress_common_init(MachineState *machine)
     }
 
     daughterboard->bootinfo.ram_size = machine->ram_size;
-    daughterboard->bootinfo.nb_cpus = machine->smp.cpus;
     daughterboard->bootinfo.board_id = VEXPRESS_BOARD_ID;
     daughterboard->bootinfo.loader_start = daughterboard->loader_start;
     daughterboard->bootinfo.smp_loader_start = map[VE_SRAM];
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 398145a7180..46bf7ceddf3 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2229,7 +2229,6 @@ static void machvirt_init(MachineState *machine)
     }
 
     vms->bootinfo.ram_size = machine->ram_size;
-    vms->bootinfo.nb_cpus = smp_cpus;
     vms->bootinfo.board_id = -1;
     vms->bootinfo.loader_start = vms->memmap[VIRT_MEM].base;
     vms->bootinfo.get_dtb = machvirt_dtb;
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 50e7268396c..3190cc0b8db 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -343,7 +343,6 @@ static void zynq_init(MachineState *machine)
     sysbus_mmio_map(busdev, 0, 0xF8007000);
 
     zynq_binfo.ram_size = machine->ram_size;
-    zynq_binfo.nb_cpus = 1;
     zynq_binfo.board_id = 0xd32;
     zynq_binfo.loader_start = 0;
     zynq_binfo.board_setup_addr = BOARD_SETUP_ADDR;
-- 
2.25.1

[PATCH 16/16] hw/arm/boot: Drop existing dtb /psci node rather than retaining it

[Peter Maydell]

If we're using PSCI emulation, we add a /psci node to the device tree
we pass to the guest.  At the moment, if the dtb already has a /psci
node in it, we retain it, rather than replacing it. (This behaviour
was added in commit c39770cd637765 in 2018.)

This is a problem if the existing node doesn't match our PSCI
emulation.  In particular, it might specify the wrong method (HVC vs
SMC), or wrong function IDs for cpu_suspend/cpu_off/etc, in which
case the guest will not get the behaviour it wants when it makes PSCI
calls.

An example of this is trying to boot the highbank or midway board
models using the device tree supplied in the kernel sources: this
device tree includes a /psci node that specifies function IDs that
don't match the (PSCI 0.2 compliant) IDs that QEMU uses.  The dtb
cpu_suspend function ID happens to match the PSCI 0.2 cpu_off ID, so
the guest hangs after booting when the kernel tries to idle the CPU
and instead it gets turned off.

Instead of retaining an existing /psci node, delete it entirely
and replace it with a node whose properties match QEMU's PSCI
emulation behaviour. This matches the way we handle /memory nodes,
where we also delete any existing nodes and write in ones that
match the way QEMU is going to behave.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I'm not confident about the FDT API to use to remove an
existing node -- I used qemu_fdt_nop_node() as that matches the
code in boot.c that's removing the memory nodes. There is
also an fdt_del_node(), though...
---
 hw/arm/boot.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index b46f1fe889e..b1e95978f26 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -478,12 +478,13 @@ static void fdt_add_psci_node(void *fdt)
     }
 
     /*
-     * If /psci node is present in provided DTB, assume that no fixup
-     * is necessary and all PSCI configuration should be taken as-is
+     * A pre-existing /psci node might specify function ID values
+     * that don't match QEMU's PSCI implementation. Delete the whole
+     * node and put our own in instead.
      */
     rc = fdt_path_offset(fdt, "/psci");
     if (rc >= 0) {
-        return;
+        qemu_fdt_nop_node(fdt, "/psci");
     }
 
     qemu_fdt_add_subnode(fdt, "/psci");
-- 
2.25.1

Re: [PATCH 00/16] arm: Fix handling of unrecognized functions in PSCI emulation

[Edgar E. Iglesias]

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

On Thu, Jan 27, 2022 at 4:46 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> This series fixes our handling of PSCI calls where the function ID is
> not recognized. These are supposed to return an error value, but
> currently we instead emulate the SMC or HVC instruction to trap to the
> guest at EL3 or EL2. Particularly of note for code review:
>  * patches 4-9 include some "is this the right behaviour for
>    this hardware" questions for the maintainers of those boards
>  * patch 15 has a DTB API question, as well as being a change in
>    what we edit in a DTB we are passed by the user
>  * testing of the affected machines would be welcome
>
> We tried to fix that bug in commit 9fcd15b9193e819b, but ran into
> regressions and so reverted it in commit 4825eaae4fdd56fba0f for
> release 7.0.  This series fixes the underlying problems causing the
> regressions and reverts the revert.  It also fixes some other bugs
> that were preventing booting of guests on the midway board and that
> meant that the Linux kernel I tested couldn't bring up the secondary
> CPUs when running with more than one guest CPU.
>
> The regressions happened on boards which enabled PSCI emulation while
> still running guest code at EL3. This used to work (as long as the
> guest code itself wasn't trying to implement PSCI!)  because of the
> fall-through-to-emulate-the-insn behaviour, but once the PSCI
> emulation handles all SMC calls, most EL3 guest code will stop working
> correctly. The solution this patchset adopts is to avoid enabling
> QEMU's PSCI emulation when running guest code at EL3.
>
> The affected boards are:
>  * orangepi-pc, mcimx6ul-evk, mcimx7d-sabre, highbank, midway,
>    xlnx-zcu102 (for any EL3 guest code)
>  * xlnx-versal-virt (only for EL3 code run via -kernel)
>  * virt (only for EL3 code run via -kernel or generic-loader)
> For all these cases we will no longer enable PSCI emulation.
> (This might in theory break guest code that used to work because
> it was relying on running under QEMU and having the PSCI emulation
> despite being at EL3 itself, but hopefully such code is rare.)
>
> In order to only enable PSCI emulation when the guest is running at an
> exception level lower than the level that our PSCI emulation
> "firmware" would be running at, we make the arm_load_kernel() code in
> boot.c responsible for setting the CPU properties psci-conduit and
> start-powered-off. This is because only that code knows what EL it is
> going to start the guest at (which depends on things like whether it
> has decided that the guest is a Linux kernel or not).
>
> The complicated case in all of this is the highbank and midway board
> models, because of all the boards which enable QEMU's PSCI emulation
> only these also use the boot.c secure_board_setup flag to say "run a
> fragment of QEMU-provided boot code in the guest at EL3 to set
> something up before running the guest at EL2 or EL1". That fragment of
> code includes use of the SMC instruction, so when PSCI emulation
> starts making that a NOP rather than a trap-to-guest-EL3 the setup
> code will change behaviour. Fortunately, for this specific board's use
> case the NOP is fine. The purpose of the setup code is to arrange that
> unknown SMCs act as NOPs, because guest code may use a
> highbank/midway-specific SMC ABI to enable the L2x0 cache
> controller. So when the PSCI emulation starts to NOP the unknown SMCs
> the setup code won't actively break, and the guest behaviour will
> still be OK. (See patch 11's commit message for fuller details.)
>
> Patches 1 and 2 make the relevant CPU properties settable after the
> CPU object has been realized. This is necessary because
> arm_load_kernel() runs very late, after the whole machine (including
> the CPU objects) has been fully initialized.  (It was the restriction
> on setting these properties before realize that previously led us to
> set them in the SoC emulation code and thus to do it unconditionally.)
>
> Patch 3 provides the "set up psci conduit" functionality in the boot.c
> code; this is opt-in per board by setting a field in the arm_boot_info
> struct.
>
> Patches 4 to 9 move the individual boards across to using the new
> approach. In each case I had to make a decision about the behaviour of
> secondary CPUs when running guest firmware at EL3 -- should the
> secondaries start off powered-down (waiting for the guest to power
> them up via some kind of emulated power-control device), or powered-up
> (so they all start running the firmware at once)? In a few cases I was
> able to find the answer to this; in the rest I have erred on the side
> of retaining the current "start powered down" behaviour, and added a
> TODO comment to that effect. If you know the actual hardware
> behaviour, let me know.
>

Hi Peter,

For ZynqMP and Versal, they should start up powered-off.



>
> Patch 10 is the revert-the-revert patch.
>
> Patch 11 removes the highbank/midway board use of the secure_board_setup
> functionality; the commit message has the details about why this is safe.
>
> Patches 12 to 14 are more minor cleanups that allow, and follow on from,
> dropping the highbank-specific secondary CPU boot stub code.
>
> Patch 15 is a change that is somewhat unrelated, but is necessary to
> get the highbank board to successfully boot in SMP and to get the
> midway board to start a Linux guest at all.
>
> I have tested this with make check/check-acceptance and also with some
> test images I have locally (including highbank and midway), but I
> don't have test images for most of these boards, and in particular I
> don't really have anything that exercises "run guest EL3 code" for
> most of them. Testing would be welcome.
>

For the Xilinx parts:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

In one of our test-cases I saw an issue where the test-case was relying on
QEMU's PSCI even though we're running EL3 firmware. The test firmware (TBM)
has PSCI support but relies on models that don't yet exist in upstream,
I'll
have to fix that on my side.

Best regards,
Edgar



>
> thanks
> -- PMM
>
> Peter Maydell (16):
>   target/arm: make psci-conduit settable after realize
>   cpu.c: Make start-powered-off settable after realize
>   hw/arm/boot: Support setting psci-conduit based on guest EL
>   hw/arm: imx: Don't enable PSCI conduit when booting guest in EL3
>   hw/arm: allwinner: Don't enable PSCI conduit when booting guest in EL3
>   hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in
>     EL3
>   hw/arm/versal: Let boot.c handle PSCI enablement
>   hw/arm/virt: Let boot.c handle PSCI enablement
>   hw/arm: highbank: For EL3 guests, don't enable PSCI, start all cores
>   Revert "Revert "arm: tcg: Adhere to SMCCC 1.3 section 5.2""
>   hw/arm/highbank: Drop use of secure_board_setup
>   hw/arm/boot: Prevent setting both psci_conduit and secure_board_setup
>   hw/arm/boot: Don't write secondary boot stub if using PSCI
>   hw/arm/highbank: Drop unused secondary boot stub code
>   hw/arm/boot: Drop nb_cpus field from arm_boot_info
>   hw/arm/boot: Drop existing dtb /psci node rather than retaining it
>
>  include/hw/arm/boot.h        |  14 ++++-
>  include/hw/arm/xlnx-versal.h |   1 -
>  cpu.c                        |  22 ++++++-
>  hw/arm/allwinner-h3.c        |   9 ++-
>  hw/arm/aspeed.c              |   1 -
>  hw/arm/boot.c                | 107 +++++++++++++++++++++++++++++------
>  hw/arm/exynos4_boards.c      |   1 -
>  hw/arm/fsl-imx6ul.c          |   2 -
>  hw/arm/fsl-imx7.c            |   8 +--
>  hw/arm/highbank.c            |  72 +----------------------
>  hw/arm/imx25_pdk.c           |   3 +-
>  hw/arm/kzm.c                 |   1 -
>  hw/arm/mcimx6ul-evk.c        |   2 +-
>  hw/arm/mcimx7d-sabre.c       |   2 +-
>  hw/arm/npcm7xx.c             |   3 -
>  hw/arm/orangepi.c            |   5 +-
>  hw/arm/raspi.c               |   1 -
>  hw/arm/realview.c            |   1 -
>  hw/arm/sabrelite.c           |   1 -
>  hw/arm/sbsa-ref.c            |   1 -
>  hw/arm/vexpress.c            |   1 -
>  hw/arm/virt.c                |  13 +----
>  hw/arm/xilinx_zynq.c         |   1 -
>  hw/arm/xlnx-versal-virt.c    |   6 +-
>  hw/arm/xlnx-versal.c         |   5 +-
>  hw/arm/xlnx-zcu102.c         |   1 +
>  hw/arm/xlnx-zynqmp.c         |  13 +++--
>  target/arm/cpu.c             |   6 +-
>  target/arm/psci.c            |  35 ++----------
>  29 files changed, 164 insertions(+), 174 deletions(-)
>
> --
> 2.25.1
>
>

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

Re: [PATCH 01/16] target/arm: make psci-conduit settable after realize

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> We want to allow the psci-conduit property to be set after realize,
> because the parts of the code which are best placed to decide if it's
> OK to enable QEMU's builtin PSCI emulation (the board code and the
> arm_load_kernel() function are distant from the code which creates
> and realizes CPUs (typically inside an SoC object's init and realize
> method) and run afterwards.
> 
> Since the DEFINE_PROP_* macros don't have support for creating
> properties which can be changed after realize, change the property to
> be created with object_property_add_uint32_ptr(), which is what we
> already use in this function for creating settable-after-realize
> properties like init-svtor and init-nsvtor.
> 
> Note that it doesn't conceptually make sense to change the setting of
> the property after the machine has been completely initialized,
> beacuse this would mean that the behaviour of the machine when first
> started would differ from its behaviour when the system is
> subsequently reset.  (It would also require the underlying state to
> be migrated, which we don't do.)
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/cpu.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 02/16] cpu.c: Make start-powered-off settable after realize

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> The CPU object's start-powered-off property is currently only
> settable before the CPU object is realized.  For arm machines this is
> awkward, because we would like to decide whether the CPU should be
> powered-off based on how we are booting the guest code, which is
> something done in the machine model code and in common code called by
> the machine model, which runs much later and in completely different
> parts of the codebase from the SoC object code that is responsible
> for creating and realizing the CPU objects.
> 
> Allow start-powered-off to be set after realize.  Since this isn't
> something that's supported by the DEFINE_PROP_* macros, we have to
> switch the property definition to use the
> object_class_property_add_bool() function.
> 
> Note that it doesn't conceptually make sense to change the setting of
> the property after the machine has been completely initialized,
> beacuse this would mean that the behaviour of the machine when first
> started would differ from its behaviour when the system is
> subsequently reset.  (It would also require the underlying state to
> be migrated, which we don't do.)
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   cpu.c | 22 +++++++++++++++++++++-
>   1 file changed, 21 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 03/16] hw/arm/boot: Support setting psci-conduit based on guest EL

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> Currently we expect board code to set the psci-conduit property on
> CPUs and ensure that secondary CPUs are created with the
> start-powered-off property set to false, if the board wishes to use
> QEMU's builtin PSCI emulation.  This worked OK for the virt board
> where we first wanted to use it, because the virt board directly
> creates its CPUs and is in a reasonable position to set those
> properties.  For other boards which model real hardware and use a
> separate SoC object, however, it is more awkward.  Most PSCI-using
> boards just set the psci-conduit board unconditionally.
> 
> This was never strictly speaking correct (because you would not be
> able to run EL3 guest firmware that itself provided the PSCI
> interface, as the QEMU implementation would overrule it), but mostly
> worked in practice because for non-PSCI SMC calls QEMU would emulate
> the SMC instruction as normal (by trapping to guest EL3).  However,
> we would like to make our PSCI emulation follow the part of the SMCC
> specification that mandates that SMC calls with unknown function
> identifiers return a failure code, which means that all SMC calls
> will be handled by the PSCI code and the "emulate as normal" path
> will no longer be taken.
> 
> We tried to implement that in commit 9fcd15b9193e81
> ("arm: tcg: Adhere to SMCCC 1.3 section 5.2"), but this
> regressed attempts to run EL3 guest code on the affected boards:
>   * mcimx6ul-evk, mcimx7d-sabre, orangepi, xlnx-zcu102
>   * for the case only of EL3 code loaded via -kernel (and
>     not via -bios or -pflash), virt and xlnx-versal-virt
> so for the 7.0 release we reverted it (in commit 4825eaae4fdd56f).
> 
> This commit provides a mechanism that boards can use to arrange that
> psci-conduit is set if running guest code at a low enough EL but not
> if it would be running at the same EL that the conduit implies that
> the QEMU PSCI implementation is using.  (Later commits will convert
> individual board models to use this mechanism.)
> 
> We do this by moving the setting of the psci-conduit and
> start-powered-off properties to arm_load_kernel().  Boards which want
> to potentially use emulated PSCI must set a psci_conduit field in the
> arm_boot_info struct to the type of conduit they want to use (SMC or
> HVC); arm_load_kernel() will then set the CPUs up accordingly if it
> is not going to start the guest code at the same or higher EL as the
> fake QEMU firmware would be at.
> 
> Board/SoC code which uses this mechanism should no longer set the CPU
> psci-conduit property directly.  It should only set the
> start-powered-off property for secondaries if EL3 guest firmware
> running bare metal expects that rather than the alternative "all CPUs
> start executing the firmware at once".
> 
> Note that when calculating whether we are going to run guest
> code at EL3, we ignore the setting of arm_boot_info::secure_board_setup,
> which might cause us to run a stub bit of guest code at EL3 which
> does some board-specific setup before dropping to EL2 or EL1 to
> run the guest kernel. This is OK because only one board that
> enables PSCI sets secure_board_setup (the highbank board), and
> the stub code it writes will behave the same way whether the
> one SMC call it makes is handled by "emulate the SMC" or by
> "PSCI default returns an error code". So we can leave that stub
> code in place until after we've changed the PSCI default behaviour;
> at that point we will remove it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/arm/boot.h | 10 +++++++++
>   hw/arm/boot.c         | 50 +++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 60 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 05/16] hw/arm: allwinner: Don't enable PSCI conduit when booting guest in EL3

[Niek Linnenbank]

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

Hi Peter,



On Thu, Jan 27, 2022 at 4:46 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> Change the allwinner-h3 based board to use the new boot.c
> functionality to allow us to enable psci-conduit only if the guest is
> being booted in EL1 or EL2, so that if the user runs guest EL3
> firmware code our PSCI emulation doesn't get in its way.
>
> To do this we stop setting the psci-conduit property on the CPU
> objects in the SoC code, and instead set the psci_conduit field in
> the arm_boot_info struct to tell the common boot loader code that
> we'd like PSCI if the guest is starting at an EL that it makes sense
> with.
>
> This affects the orangepi-pc board.
>
> This commit leaves the secondary CPUs in the powered-down state if
> the guest is booting at EL3, which is the same behaviour as before
> this commit.  The secondaries can no longer be started by that EL3
> code making a PSCI call but can still be started via the CPU
> Configuration Module registers (which we model in
> hw/misc/allwinner-cpucfg.c).
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>

While testing your patches on the orangepi-pc machine, I've found that two
acceptance tests BootLinuxConsole.test_arm_orangepi_bionic_20_08 and
BootLinuxConsole.test_arm_orangepi_uboot_netbsd9 of the orangepi-pc board
are no longer passing on current master:

  ARMBIAN_ARTIFACTS_CACHED=yes AVOCADO_ALLOW_LARGE_STORAGE=yes avocado
--show=app,console run -t machine:orangepi-pc
tests/avocado/boot_linux_console.py
  ...
 (4/5)
tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08:
-console: U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200)
\console: DRAM:
INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout
reached\nOriginal status: ERROR\n{'name':
'4-tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08',
'logdi>
 (5/5)
tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9:
/console: U-Boot SPL 2020.01+dfsg-1 (Jan 08 2020 - 08:19:44 +0000)
console: DRAM:
INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout
reached\nOriginal status: ERROR\n{'name':
'5-tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9',
'logd>
RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 2 |
CANCEL 0
JOB TIME   : 221.25 s

Bisecting the error I was able to trace it back to commit 5ead62185d
("memory: Make memory_region_is_mapped() succeed when mapped via an alias").
I'll try to find the original thread and respond to that with this
information.

However, with commit 5ead62185d reverted, all tested passed fine:

ARMBIAN_ARTIFACTS_CACHED=yes AVOCADO_ALLOW_LARGE_STORAGE=yes avocado
--show=app,console run -t machine:orangepi-pc
tests/avocado/boot_linux_console.py
...
PASS (16.48 s)
RESULTS    : PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 |
CANCEL 0
JOB TIME   : 116.63 s

So for the orangepi-pc and allwinner-h3:

Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>
Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com>


> ---
> If anybody knows for definite that the secondaries should be
> powered-down at startup for guest firmware, we can delete the TODO.
>

Looking at the Allwinner H3 datasheet in 4.4.3.7 page 146, it says that
for the CPU1 Status Register the default value indicates at least that its
not in a
wait for interrupt standby mode. And if I look in U-Boot's
arm/arm/cpu/armv7/sunxi/psci.c code
in the psci_cpu_on implementation, there is an explicit 'power on' part
there, suggesting the secondary CPUs
are by default off. So while I don't have any hard proof, these findings
suggest we are modeling the correct behavior
with secondary CPUs by default off.



> The allwinner-cpucfg.c code makes the reset value for the
> REG_CPU*_RST_CTRL registers "CPUX_RESET_RELEASED", which might
> suggest otherwise, but that could easily just be a QEMU error.
> ---
>  hw/arm/allwinner-h3.c | 9 ++++-----
>  hw/arm/orangepi.c     | 1 +
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> index f9b7ed18711..f54aff6d2d2 100644
> --- a/hw/arm/allwinner-h3.c
> +++ b/hw/arm/allwinner-h3.c
> @@ -235,11 +235,10 @@ static void allwinner_h3_realize(DeviceState *dev,
> Error **errp)
>      /* CPUs */
>      for (i = 0; i < AW_H3_NUM_CPUS; i++) {
>
> -        /* Provide Power State Coordination Interface */
> -        qdev_prop_set_int32(DEVICE(&s->cpus[i]), "psci-conduit",
> -                            QEMU_PSCI_CONDUIT_SMC);
> -
> -        /* Disable secondary CPUs */
> +        /*
> +         * Disable secondary CPUs. TODO: check whether this is what
> +         * guest EL3 firmware would really expect.
> +         */
>          qdev_prop_set_bit(DEVICE(&s->cpus[i]), "start-powered-off",
>                            i > 0);
>
> diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> index e7963822367..68fe9182414 100644
> --- a/hw/arm/orangepi.c
> +++ b/hw/arm/orangepi.c
> @@ -105,6 +105,7 @@ static void orangepi_init(MachineState *machine)
>      }
>      orangepi_binfo.loader_start = h3->memmap[AW_H3_DEV_SDRAM];
>      orangepi_binfo.ram_size = machine->ram_size;
> +    orangepi_binfo.psci_conduit = QEMU_PSCI_CONDUIT_SMC;
>      arm_load_kernel(ARM_CPU(first_cpu), machine, &orangepi_binfo);
>  }
>
> --
> 2.25.1
>
>

-- 
Niek Linnenbank

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

Re: [PATCH 04/16] hw/arm: imx: Don't enable PSCI conduit when booting guest in EL3

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> Change the iMX-SoC based boards to use the new boot.c functionality
> to allow us to enable psci-conduit only if the guest is being booted
> in EL1 or EL2, so that if the user runs guest EL3 firmware code our
> PSCI emulation doesn't get in its way.
> 
> To do this we stop setting the psci-conduit property on the CPU
> objects in the SoC code, and instead set the psci_conduit field in
> the arm_boot_info struct to tell the common boot loader code that
> we'd like PSCI if the guest is starting at an EL that it makes
> sense with.
> 
> This affects the mcimx6ul-evk and mcimx7d-sabre boards.
> 
> Note that for the mcimx7d board, this means that when running guest
> code at EL3 there is currently no way to power on the secondary CPUs,
> because we do not currently have a model of the system reset
> controller module which should be used to do that for the imx7 SoC,
> only for the imx6 SoC.  (Previously EL3 code which knew it was
> running on QEMU could use a PSCI call to do this.) This doesn't
> affect the imx6ul-evk board because it is uniprocessor.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> I don't have the i.mx7 manual to hand, so I'm partly making
> assumptions based on the i.mx6 behaviour. If somebody with the
> manual could double-check that it does indeed start up with the
> secondary CPUs powered down via the SRC that would be great.
> ---
>   hw/arm/fsl-imx6ul.c    | 2 --
>   hw/arm/fsl-imx7.c      | 8 ++++----
>   hw/arm/mcimx6ul-evk.c  | 1 +
>   hw/arm/mcimx7d-sabre.c | 1 +
>   4 files changed, 6 insertions(+), 6 deletions(-)

Acked-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 06/16] hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in EL3

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> Change the Xilinx ZynqMP-based board xlnx-zcu102 to use the new
> boot.c functionality to allow us to enable psci-conduit only if
> the guest is being booted in EL1 or EL2, so that if the user runs
> guest EL3 firmware code our PSCI emulation doesn't get in its
> way.
> 
> To do this we stop setting the psci-conduit property on the CPU
> objects in the SoC code, and instead set the psci_conduit field in
> the arm_boot_info struct to tell the common boot loader code that
> we'd like PSCI if the guest is starting at an EL that it makes
> sense with.
> 
> Note that this means that EL3 guest code will have no way
> to power on secondary cores, because we don't model any
> kind of power controller that does that on this SoC.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
> Again, if anybody knows the real-hardware EL3 behaviour for
> CPUs that would be great.
> ---
>   hw/arm/xlnx-zcu102.c |  1 +
>   hw/arm/xlnx-zynqmp.c | 13 ++++++++-----
>   2 files changed, 9 insertions(+), 5 deletions(-)

Acked-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 07/16] hw/arm/versal: Let boot.c handle PSCI enablement

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> Instead of setting the CPU psci-conduit and start-powered-off
> properties in the xlnx-versal-virt board code, set the arm_boot_info
> psci_conduit field so that the boot.c code can do it.
> 
> This will fix a corner case where we were incorrectly enabling PSCI
> emulation when booting guest code into EL3 because it was an ELF file
> passed to -kernel.  (EL3 guest code started via -bios, -pflash, or
> the generic loader was already being run with PSCI emulation
> disabled.)
> 
> Note that EL3 guest code has no way to turn on the secondary CPUs
> because there's no emulated power controller, but this was already
> true for EL3 guest code run via -bios, -pflash, or the generic
> loader.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/arm/xlnx-versal.h | 1 -
>   hw/arm/xlnx-versal-virt.c    | 6 ++++--
>   hw/arm/xlnx-versal.c         | 5 +----
>   3 files changed, 5 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 08/16] hw/arm/virt: Let boot.c handle PSCI enablement

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> Instead of setting the CPU psci-conduit and start-powered-off
> properties in the virt board code, set the arm_boot_info psci_conduit
> field so that the boot.c code can do it.
> 
> This will fix a corner case where we were incorrectly enabling PSCI
> emulation when booting guest code into EL3 because it was an ELF file
> passed to -kernel or to the generic loader.  (EL3 guest code started
> via -bios or -pflash was already being run with PSCI emulation
> disabled.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/arm/virt.c | 12 +-----------
>   1 file changed, 1 insertion(+), 11 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 09/16] hw/arm: highbank: For EL3 guests, don't enable PSCI, start all cores

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> Change the highbank/midway boards to use the new boot.c functionality
> to allow us to enable psci-conduit only if the guest is being booted
> in EL1 or EL2, so that if the user runs guest EL3 firmware code our
> PSCI emulation doesn't get in its way.
> 
> To do this we stop setting the psci-conduit and start-powered-off
> properties on the CPU objects in the board code, and instead set the
> psci_conduit field in the arm_boot_info struct to tell the common
> boot loader code that we'd like PSCI if the guest is starting at an
> EL that it makes sense with (in which case it will set these
> properties).
> 
> This means that when running guest code at EL3, all the cores
> will start execution at once on poweron. This matches the
> real hardware behaviour. (A brief description of the hardware
> boot process is in the u-boot documentation for these boards:
> https://u-boot.readthedocs.io/en/latest/board/highbank/highbank.html#boot-process
>   -- in theory one might run the 'a9boot'/'a15boot' secure monitor
> code in QEMU, though we probably don't emulate enough for that.)
> 
> This affects the highbank and midway boards.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/arm/highbank.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 10/16] Revert "Revert "arm: tcg: Adhere to SMCCC 1.3 section 5.2""

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> Now that we have arranged for all the affected board models
> to not enable the PSCI emulation if they are running guest code
> at EL3, we can revert commit 4825eaae4fdd56fba0f, thus
> reinstating commit 9fcd15b9193e819b, without bringing back the
> regressions that caused us to revert it.
> 
> For clarity, here is the original commit message of 9fcd15b9193e819b:
> 
> The SMCCC 1.3 spec section 5.2 says
> 
>    The Unknown SMC Function Identifier is a sign-extended value of (-1)
>    that is returned in the R0, W0 or X0 registers. An implementation must
>    return this error code when it receives:
> 
>      * An SMC or HVC call with an unknown Function Identifier
>      * An SMC or HVC call for a removed Function Identifier
>      * An SMC64/HVC64 call from AArch32 state
> 
> To comply with these statements, let's always return -1 when we encounter
> an unknown HVC or SMC call.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   target/arm/psci.c | 35 ++++++-----------------------------
>   1 file changed, 6 insertions(+), 29 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 11/16] hw/arm/highbank: Drop use of secure_board_setup

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> Guest code on highbank may make non-PSCI SMC calls in order to
> enable/disable the L2x0 cache controller (see the Linux kernel's
> arch/arm/mach-highbank/highbank.c highbank_l2c310_write_sec()
> function).  The ABI for this is documented in kernel commit
> 8e56130dcb as being borrowed from the OMAP44xx ROM.  The OMAP44xx TRM
> documents this function ID as having no return value and potentially
> trashing all guest registers except SP and PC. For QEMU's purposes
> (where our L2x0 model is a stub and enabling or disabling it doesn't
> affect the guest behaviour) a simple "do nothing" SMC is fine.
> 
> We currently implement this NOP behaviour using a little bit of
> Secure code we run before jumping to the guest kernel, which is
> written by arm_write_secure_board_setup_dummy_smc().  The code sets
> up a set of Secure vectors where the SMC entry point returns without
> doing anything.
> 
> Now that the PSCI SMC emulation handles all SMC calls (setting r0 to
> an error code if the input r0 function identifier is not recognized),
> we can use that default behaviour as sufficient for the highbank
> cache controller call.  (Because the guest code assumes r0 has no
> interesting value on exit it doesn't matter that we set it to the
> error code).  We can therefore delete the highbank board code that
> sets secure_board_setup to true and writes the secure-code bootstub.
> 
> (Note that because the OMAP44xx ABI puts function-identifiers in
> r12 and PSCI uses r0, we only avoid a clash because Linux's code
> happens to put the function-identifier in both registers. But this
> is true also when the kernel is running on real firmware that
> implements both ABIs as far as I can see.)
> 
> This change fixes in passing booting on the 'midway' board model,
> which has been completely broken since we added support for Hyp
> mode to the Cortex-A15 CPU. When we did that boot.c was made to
> start running the guest code in Hyp mode; this includes the
> board_setup hook, which instantly UNDEFs because the NSACR is
> not accessible from Hyp. (Put another way, we never made the
> secure_board_setup hook support cope with Hyp mode.)
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/arm/highbank.c | 8 --------
>   1 file changed, 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 12/16] hw/arm/boot: Prevent setting both psci_conduit and secure_board_setup

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> Now that we have dealt with the one special case (highbank) that needed
> to set both psci_conduit and secure_board_setup, we don't need to
> allow that combination any more. It doesn't make sense in general,
> so use an assertion to ensure we don't add new boards that do it
> by accident without thinking through the consequences.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/arm/boot.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 13/16] hw/arm/boot: Don't write secondary boot stub if using PSCI

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> If we're using PSCI emulation to start secondary CPUs, there is no
> point in writing the "secondary boot" stub code, because it will
> never be used -- secondary CPUs start powered-off, and when powered
> on are set to begin execution at the address specified by the guest's
> power-on PSCI call, not at the stub.
> 
> Move the call to the hook that writes the secondary boot stub code so
> that we can do it only if we're starting a Linux kernel and not using
> PSCI.
> 
> (None of the users of the hook care about the ordering of its call
> relative to anything else: they only use it to write a rom blob to
> guest memory.)
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/arm/boot.h |  3 +++
>   hw/arm/boot.c         | 35 ++++++++++++++++++++++++-----------
>   2 files changed, 27 insertions(+), 11 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 14/16] hw/arm/highbank: Drop unused secondary boot stub code

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> The highbank and midway board code includes boot-stub code for
> handling secondary CPU boot which keeps the secondaries in a pen
> until the primary writes to a known location with the address they
> should jump to.
> 
> This code is never used, because the boards enable QEMU's PSCI
> emulation, so secondary CPUs are kept powered off until the PSCI call
> which turns them on, and then start execution from the address given
> by the guest in that PSCI call.  Delete the unreachable code.
> 
> (The code was wrong for midway in any case -- on the Cortex-A15 the
> GIC CPU interface registers are at a different offset from PERIPHBASE
> compared to the Cortex-A9, and the code baked-in the offsets for
> highbank's A9.)
> 
> Note that this commit implicitly depends on the preceding "Don't
> write secondary boot stub if using PSCI" commit -- the default
> secondary-boot stub code overlaps with one of the highbank-specific
> bootcode rom blobs, so we must suppress the secondary-boot
> stub code entirely, not merely replace the highbank-specific
> version with the default.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/arm/highbank.c | 56 -----------------------------------------------
>   1 file changed, 56 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 15/16] hw/arm/boot: Drop nb_cpus field from arm_boot_info

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> We use the arm_boot_info::nb_cpus  field in only one place, and that
> place can easily get the number of CPUs locally rather than relying
> on the board code to have set the field correctly.  (At least one
> board, xlnx-versal-virt, does not set the field despite having more
> than one CPU.)
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   include/hw/arm/boot.h   | 1 -
>   hw/arm/aspeed.c         | 1 -
>   hw/arm/boot.c           | 7 +++----
>   hw/arm/exynos4_boards.c | 1 -
>   hw/arm/highbank.c       | 1 -
>   hw/arm/imx25_pdk.c      | 3 +--
>   hw/arm/kzm.c            | 1 -
>   hw/arm/mcimx6ul-evk.c   | 1 -
>   hw/arm/mcimx7d-sabre.c  | 1 -
>   hw/arm/npcm7xx.c        | 3 ---
>   hw/arm/orangepi.c       | 4 +---
>   hw/arm/raspi.c          | 1 -
>   hw/arm/realview.c       | 1 -
>   hw/arm/sabrelite.c      | 1 -
>   hw/arm/sbsa-ref.c       | 1 -
>   hw/arm/vexpress.c       | 1 -
>   hw/arm/virt.c           | 1 -
>   hw/arm/xilinx_zynq.c    | 1 -
>   18 files changed, 5 insertions(+), 26 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 16/16] hw/arm/boot: Drop existing dtb /psci node rather than retaining it

[Richard Henderson]

On 1/28/22 02:46, Peter Maydell wrote:
> If we're using PSCI emulation, we add a /psci node to the device tree
> we pass to the guest.  At the moment, if the dtb already has a /psci
> node in it, we retain it, rather than replacing it. (This behaviour
> was added in commit c39770cd637765 in 2018.)
> 
> This is a problem if the existing node doesn't match our PSCI
> emulation.  In particular, it might specify the wrong method (HVC vs
> SMC), or wrong function IDs for cpu_suspend/cpu_off/etc, in which
> case the guest will not get the behaviour it wants when it makes PSCI
> calls.
> 
> An example of this is trying to boot the highbank or midway board
> models using the device tree supplied in the kernel sources: this
> device tree includes a /psci node that specifies function IDs that
> don't match the (PSCI 0.2 compliant) IDs that QEMU uses.  The dtb
> cpu_suspend function ID happens to match the PSCI 0.2 cpu_off ID, so
> the guest hangs after booting when the kernel tries to idle the CPU
> and instead it gets turned off.
> 
> Instead of retaining an existing /psci node, delete it entirely
> and replace it with a node whose properties match QEMU's PSCI
> emulation behaviour. This matches the way we handle /memory nodes,
> where we also delete any existing nodes and write in ones that
> match the way QEMU is going to behave.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I'm not confident about the FDT API to use to remove an
> existing node -- I used qemu_fdt_nop_node() as that matches the
> code in boot.c that's removing the memory nodes. There is
> also an fdt_del_node(), though...

It all depends on whether we've got saved offsets for nodes in the DTB, I guess. 
fdt_del_node says that it changes node offsets, and fdt_nop_node says that it doesn't.

Anyway,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 05/16] hw/arm: allwinner: Don't enable PSCI conduit when booting guest in EL3

[Andre Przywara]

On Sun, 30 Jan 2022 23:35:37 +0100
Niek Linnenbank <nieklinnenbank@gmail.com> wrote:

Hi,

(CC:ing Samuel for his intimate Allwinner BootROM knowledge)

> Hi Peter,
> 
> 
> 
> On Thu, Jan 27, 2022 at 4:46 PM Peter Maydell <peter.maydell@linaro.org>
> wrote:
> 
> > Change the allwinner-h3 based board to use the new boot.c
> > functionality to allow us to enable psci-conduit only if the guest is
> > being booted in EL1 or EL2, so that if the user runs guest EL3
> > firmware code our PSCI emulation doesn't get in its way.
> >
> > To do this we stop setting the psci-conduit property on the CPU
> > objects in the SoC code, and instead set the psci_conduit field in
> > the arm_boot_info struct to tell the common boot loader code that
> > we'd like PSCI if the guest is starting at an EL that it makes sense
> > with.
> >
> > This affects the orangepi-pc board.
> >
> > This commit leaves the secondary CPUs in the powered-down state if
> > the guest is booting at EL3, which is the same behaviour as before
> > this commit.  The secondaries can no longer be started by that EL3
> > code making a PSCI call but can still be started via the CPU
> > Configuration Module registers (which we model in
> > hw/misc/allwinner-cpucfg.c).
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >  
> 
> While testing your patches on the orangepi-pc machine, I've found that two
> acceptance tests BootLinuxConsole.test_arm_orangepi_bionic_20_08 and
> BootLinuxConsole.test_arm_orangepi_uboot_netbsd9 of the orangepi-pc board
> are no longer passing on current master:
> 
>   ARMBIAN_ARTIFACTS_CACHED=yes AVOCADO_ALLOW_LARGE_STORAGE=yes avocado
> --show=app,console run -t machine:orangepi-pc
> tests/avocado/boot_linux_console.py
>   ...
>  (4/5)
> tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08:
> -console: U-Boot SPL 2020.04-armbian (Sep 02 2020 - 10:16:13 +0200)
> \console: DRAM:
> INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout
> reached\nOriginal status: ERROR\n{'name':
> '4-tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic_20_08',
> 'logdi>  
>  (5/5)
> tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9:
> /console: U-Boot SPL 2020.01+dfsg-1 (Jan 08 2020 - 08:19:44 +0000)
> console: DRAM:
> INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout
> reached\nOriginal status: ERROR\n{'name':
> '5-tests/avocado/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9',
> 'logd>  
> RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 2 |
> CANCEL 0
> JOB TIME   : 221.25 s
> 
> Bisecting the error I was able to trace it back to commit 5ead62185d
> ("memory: Make memory_region_is_mapped() succeed when mapped via an alias").
> I'll try to find the original thread and respond to that with this
> information.
> 
> However, with commit 5ead62185d reverted, all tested passed fine:
> 
> ARMBIAN_ARTIFACTS_CACHED=yes AVOCADO_ALLOW_LARGE_STORAGE=yes avocado
> --show=app,console run -t machine:orangepi-pc
> tests/avocado/boot_linux_console.py
> ...
> PASS (16.48 s)
> RESULTS    : PASS 5 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 |
> CANCEL 0
> JOB TIME   : 116.63 s
> 
> So for the orangepi-pc and allwinner-h3:
> 
> Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>
> Tested-by: Niek Linnenbank <nieklinnenbank@gmail.com>
> 
> 
> > ---
> > If anybody knows for definite that the secondaries should be
> > powered-down at startup for guest firmware, we can delete the TODO.
> >  
> 
> Looking at the Allwinner H3 datasheet in 4.4.3.7 page 146, it says that
> for the CPU1 Status Register the default value indicates at least that its
> not in a
> wait for interrupt standby mode. And if I look in U-Boot's
> arm/arm/cpu/armv7/sunxi/psci.c code
> in the psci_cpu_on implementation, there is an explicit 'power on' part
> there, suggesting the secondary CPUs
> are by default off. So while I don't have any hard proof, these findings
> suggest we are modeling the correct behavior
> with secondary CPUs by default off.

So when it comes to firmware, indeed the secondaries seem to be off when
the first user provided code (boot0/SPL) is entered. However there is an
MPIDR read in the BROM, with the corresponding "branch if not primary
core". I think this is because the BROM is mapped at the reset vector, so
upon SMP firmware releasing the reset line it always starts in ROM, then
gets diverted to the actual entry point.
Maybe Samuel can confirm that the secondary cores are power gated when the
SoCs comes out of reset?

Cheers,
Andre

> > The allwinner-cpucfg.c code makes the reset value for the
> > REG_CPU*_RST_CTRL registers "CPUX_RESET_RELEASED", which might
> > suggest otherwise, but that could easily just be a QEMU error.
> > ---
> >  hw/arm/allwinner-h3.c | 9 ++++-----
> >  hw/arm/orangepi.c     | 1 +
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
> > index f9b7ed18711..f54aff6d2d2 100644
> > --- a/hw/arm/allwinner-h3.c
> > +++ b/hw/arm/allwinner-h3.c
> > @@ -235,11 +235,10 @@ static void allwinner_h3_realize(DeviceState *dev,
> > Error **errp)
> >      /* CPUs */
> >      for (i = 0; i < AW_H3_NUM_CPUS; i++) {
> >
> > -        /* Provide Power State Coordination Interface */
> > -        qdev_prop_set_int32(DEVICE(&s->cpus[i]), "psci-conduit",
> > -                            QEMU_PSCI_CONDUIT_SMC);
> > -
> > -        /* Disable secondary CPUs */
> > +        /*
> > +         * Disable secondary CPUs. TODO: check whether this is what
> > +         * guest EL3 firmware would really expect.
> > +         */
> >          qdev_prop_set_bit(DEVICE(&s->cpus[i]), "start-powered-off",
> >                            i > 0);
> >
> > diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> > index e7963822367..68fe9182414 100644
> > --- a/hw/arm/orangepi.c
> > +++ b/hw/arm/orangepi.c
> > @@ -105,6 +105,7 @@ static void orangepi_init(MachineState *machine)
> >      }
> >      orangepi_binfo.loader_start = h3->memmap[AW_H3_DEV_SDRAM];
> >      orangepi_binfo.ram_size = machine->ram_size;
> > +    orangepi_binfo.psci_conduit = QEMU_PSCI_CONDUIT_SMC;
> >      arm_load_kernel(ARM_CPU(first_cpu), machine, &orangepi_binfo);
> >  }
> >
> > --
> > 2.25.1
> >
> >  
> 

Re: [PATCH 00/16] arm: Fix handling of unrecognized functions in PSCI emulation

[Cédric Le Goater]

On 1/27/22 16:46, Peter Maydell wrote:
> This series fixes our handling of PSCI calls where the function ID is
> not recognized. These are supposed to return an error value, but
> currently we instead emulate the SMC or HVC instruction to trap to the
> guest at EL3 or EL2. Particularly of note for code review:
>   * patches 4-9 include some "is this the right behaviour for
>     this hardware" questions for the maintainers of those boards
>   * patch 15 has a DTB API question, as well as being a change in
>     what we edit in a DTB we are passed by the user
>   * testing of the affected machines would be welcome
> 
> We tried to fix that bug in commit 9fcd15b9193e819b, but ran into
> regressions and so reverted it in commit 4825eaae4fdd56fba0f for
> release 7.0.  This series fixes the underlying problems causing the
> regressions and reverts the revert.  It also fixes some other bugs
> that were preventing booting of guests on the midway board and that
> meant that the Linux kernel I tested couldn't bring up the secondary
> CPUs when running with more than one guest CPU.
> 
> The regressions happened on boards which enabled PSCI emulation while
> still running guest code at EL3. This used to work (as long as the
> guest code itself wasn't trying to implement PSCI!)  because of the
> fall-through-to-emulate-the-insn behaviour, but once the PSCI
> emulation handles all SMC calls, most EL3 guest code will stop working
> correctly. The solution this patchset adopts is to avoid enabling
> QEMU's PSCI emulation when running guest code at EL3.
> 
> The affected boards are:
>   * orangepi-pc, mcimx6ul-evk, mcimx7d-sabre, highbank, midway,
>     xlnx-zcu102 (for any EL3 guest code)
>   * xlnx-versal-virt (only for EL3 code run via -kernel)
>   * virt (only for EL3 code run via -kernel or generic-loader)
> For all these cases we will no longer enable PSCI emulation.
> (This might in theory break guest code that used to work because
> it was relying on running under QEMU and having the PSCI emulation
> despite being at EL3 itself, but hopefully such code is rare.)
> 
> In order to only enable PSCI emulation when the guest is running at an
> exception level lower than the level that our PSCI emulation
> "firmware" would be running at, we make the arm_load_kernel() code in
> boot.c responsible for setting the CPU properties psci-conduit and
> start-powered-off. This is because only that code knows what EL it is
> going to start the guest at (which depends on things like whether it
> has decided that the guest is a Linux kernel or not).
> 
> The complicated case in all of this is the highbank and midway board
> models, because of all the boards which enable QEMU's PSCI emulation
> only these also use the boot.c secure_board_setup flag to say "run a
> fragment of QEMU-provided boot code in the guest at EL3 to set
> something up before running the guest at EL2 or EL1". That fragment of
> code includes use of the SMC instruction, so when PSCI emulation
> starts making that a NOP rather than a trap-to-guest-EL3 the setup
> code will change behaviour. Fortunately, for this specific board's use
> case the NOP is fine. The purpose of the setup code is to arrange that
> unknown SMCs act as NOPs, because guest code may use a
> highbank/midway-specific SMC ABI to enable the L2x0 cache
> controller. So when the PSCI emulation starts to NOP the unknown SMCs
> the setup code won't actively break, and the guest behaviour will
> still be OK. (See patch 11's commit message for fuller details.)
> 
> Patches 1 and 2 make the relevant CPU properties settable after the
> CPU object has been realized. This is necessary because
> arm_load_kernel() runs very late, after the whole machine (including
> the CPU objects) has been fully initialized.  (It was the restriction
> on setting these properties before realize that previously led us to
> set them in the SoC emulation code and thus to do it unconditionally.)
> 
> Patch 3 provides the "set up psci conduit" functionality in the boot.c
> code; this is opt-in per board by setting a field in the arm_boot_info
> struct.
> 
> Patches 4 to 9 move the individual boards across to using the new
> approach. In each case I had to make a decision about the behaviour of
> secondary CPUs when running guest firmware at EL3 -- should the
> secondaries start off powered-down (waiting for the guest to power
> them up via some kind of emulated power-control device), or powered-up
> (so they all start running the firmware at once)? In a few cases I was
> able to find the answer to this; in the rest I have erred on the side
> of retaining the current "start powered down" behaviour, and added a
> TODO comment to that effect. If you know the actual hardware
> behaviour, let me know.
> 
> Patch 10 is the revert-the-revert patch.
> 
> Patch 11 removes the highbank/midway board use of the secure_board_setup
> functionality; the commit message has the details about why this is safe.
> 
> Patches 12 to 14 are more minor cleanups that allow, and follow on from,
> dropping the highbank-specific secondary CPU boot stub code.
> 
> Patch 15 is a change that is somewhat unrelated, but is necessary to
> get the highbank board to successfully boot in SMP and to get the
> midway board to start a Linux guest at all.
> 
> I have tested this with make check/check-acceptance and also with some
> test images I have locally (including highbank and midway), but I
> don't have test images for most of these boards, and in particular I
> don't really have anything that exercises "run guest EL3 code" for
> most of them. Testing would be welcome.

for the Aspeed machines,

Tested-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> 
> thanks
> -- PMM
> 
> Peter Maydell (16):
>    target/arm: make psci-conduit settable after realize
>    cpu.c: Make start-powered-off settable after realize
>    hw/arm/boot: Support setting psci-conduit based on guest EL
>    hw/arm: imx: Don't enable PSCI conduit when booting guest in EL3
>    hw/arm: allwinner: Don't enable PSCI conduit when booting guest in EL3
>    hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in
>      EL3
>    hw/arm/versal: Let boot.c handle PSCI enablement
>    hw/arm/virt: Let boot.c handle PSCI enablement
>    hw/arm: highbank: For EL3 guests, don't enable PSCI, start all cores
>    Revert "Revert "arm: tcg: Adhere to SMCCC 1.3 section 5.2""
>    hw/arm/highbank: Drop use of secure_board_setup
>    hw/arm/boot: Prevent setting both psci_conduit and secure_board_setup
>    hw/arm/boot: Don't write secondary boot stub if using PSCI
>    hw/arm/highbank: Drop unused secondary boot stub code
>    hw/arm/boot: Drop nb_cpus field from arm_boot_info
>    hw/arm/boot: Drop existing dtb /psci node rather than retaining it
> 
>   include/hw/arm/boot.h        |  14 ++++-
>   include/hw/arm/xlnx-versal.h |   1 -
>   cpu.c                        |  22 ++++++-
>   hw/arm/allwinner-h3.c        |   9 ++-
>   hw/arm/aspeed.c              |   1 -
>   hw/arm/boot.c                | 107 +++++++++++++++++++++++++++++------
>   hw/arm/exynos4_boards.c      |   1 -
>   hw/arm/fsl-imx6ul.c          |   2 -
>   hw/arm/fsl-imx7.c            |   8 +--
>   hw/arm/highbank.c            |  72 +----------------------
>   hw/arm/imx25_pdk.c           |   3 +-
>   hw/arm/kzm.c                 |   1 -
>   hw/arm/mcimx6ul-evk.c        |   2 +-
>   hw/arm/mcimx7d-sabre.c       |   2 +-
>   hw/arm/npcm7xx.c             |   3 -
>   hw/arm/orangepi.c            |   5 +-
>   hw/arm/raspi.c               |   1 -
>   hw/arm/realview.c            |   1 -
>   hw/arm/sabrelite.c           |   1 -
>   hw/arm/sbsa-ref.c            |   1 -
>   hw/arm/vexpress.c            |   1 -
>   hw/arm/virt.c                |  13 +----
>   hw/arm/xilinx_zynq.c         |   1 -
>   hw/arm/xlnx-versal-virt.c    |   6 +-
>   hw/arm/xlnx-versal.c         |   5 +-
>   hw/arm/xlnx-zcu102.c         |   1 +
>   hw/arm/xlnx-zynqmp.c         |  13 +++--
>   target/arm/cpu.c             |   6 +-
>   target/arm/psci.c            |  35 ++----------
>   29 files changed, 164 insertions(+), 174 deletions(-)
> 

Re: [PATCH 05/16] hw/arm: allwinner: Don't enable PSCI conduit when booting guest in EL3

[Samuel Holland]

On 1/31/22 4:52 AM, Andre Przywara wrote:
> On Sun, 30 Jan 2022 23:35:37 +0100
> Niek Linnenbank <nieklinnenbank@gmail.com> wrote:
>> On Thu, Jan 27, 2022 at 4:46 PM Peter Maydell <peter.maydell@linaro.org>
>> wrote:
>>> If anybody knows for definite that the secondaries should be
>>> powered-down at startup for guest firmware, we can delete the TODO.
>>
>> Looking at the Allwinner H3 datasheet in 4.4.3.7 page 146, it says that
>> for the CPU1 Status Register the default value indicates at least that its
>> not in a
>> wait for interrupt standby mode. And if I look in U-Boot's
>> arm/arm/cpu/armv7/sunxi/psci.c code
>> in the psci_cpu_on implementation, there is an explicit 'power on' part
>> there, suggesting the secondary CPUs
>> are by default off. So while I don't have any hard proof, these findings
>> suggest we are modeling the correct behavior
>> with secondary CPUs by default off.
> 
> So when it comes to firmware, indeed the secondaries seem to be off when
> the first user provided code (boot0/SPL) is entered. However there is an
> MPIDR read in the BROM, with the corresponding "branch if not primary
> core". I think this is because the BROM is mapped at the reset vector, so
> upon SMP firmware releasing the reset line it always starts in ROM, then
> gets diverted to the actual entry point.

Yes, I believe this is an accurate explanation.

> Maybe Samuel can confirm that the secondary cores are power gated when the
> SoCs comes out of reset?

The secondary cores are powered up but held in reset, as described in the
datasheet (sections 4.4.3.5/8/11 "CPU1/2/3 Reset Register"). The boot ROM does
not touch any of the CPU Configuration registers, so they hold their default
values into U-Boot execution. Looking at the registers from the U-Boot shell
confirms the datasheet:

=> md.l 1f01500 20
01f01500: 00000000 00000000 00000000 00000000  ................
                 ^ output gating[0] is disabled for all CPUs
01f01510: 00000000 00000000 00000000 00000000  ................
01f01520: 00000001 00000000 00000000 00000000  ................
01f01530: 00000000 00000000 00000000 00000000  ................
01f01540: 00000000 00000000 00000000 00000000  ................
                ^^       ^^       ^^       ^^
                power switches[1] are enabled for all CPUs
01f01550: 00000000 00000000 00000000 00000000  ................
01f01560: 00000000 00000000 00000000 00000000  ................
01f01570: 00000000 00000000 00000000 00000000  ................

=> md.l 1f01c40 40
01f01c40: 00000003 00000000 00000001 00000000  ................
                 ^ nCPUPORESET and nCORERESET deasserted
01f01c50: 00000000 00000000 00000000 00000000  ................
01f01c60: 00000000 00000000 00000000 00000000  ................
01f01c70: 00000000 00000000 00000000 00000000  ................
01f01c80: 00000001 00000000 00000000 00000000  ................
                 ^ nCPUPORESET deasserted; nCORERESET asserted
01f01c90: 00000000 00000000 00000000 00000000  ................
01f01ca0: 00000000 00000000 00000000 00000000  ................
01f01cb0: 00000000 00000000 00000000 00000000  ................
01f01cc0: 00000001 00000000 00000000 00000000  ................
                 ^ nCPUPORESET deasserted; nCORERESET asserted
01f01cd0: 00000000 00000000 00000000 00000000  ................
01f01ce0: 00000000 00000000 00000000 00000000  ................
01f01cf0: 00000000 00000000 00000000 00000000  ................
01f01d00: 00000001 00000000 00000000 00000000  ................
                 ^ nCPUPORESET deasserted; nCORERESET asserted
01f01d10: 00000000 00000000 00000000 00000000  ................
01f01d20: 00000000 00000000 00000000 00000000  ................
01f01d30: 00000000 00000000 00000000 00000000  ................

Since these CPU configuration registers are in the "CPUS" (ARISC) power domain,
it is not possible to reset them to their defaults using software. However, on
64-bit SoCs the CPUCFG registers are in the "CPU_SUBSYS" (ARM) power domain, and
they can be fully reset by toggling CPU_SYS_RESET[2][3]. Using A64 as an
example, the default value for the Cluster Reset Control Register 0x11101101,
which also matches the manual (section 3.4.5.8). Again, this has core 0 reset
deasserted and the remaining core resets asserted. So far, this behavior has
been consistent across every Allwinner SoC I have worked with.

Regards,
Samuel

[0]:
https://github.com/crust-firmware/crust/blob/master/platform/h3/include/platform/prcm.h#L55
[1]:
https://github.com/crust-firmware/crust/blob/master/platform/h3/include/platform/prcm.h#L69
[2]:
https://github.com/crust-firmware/crust/blob/master/platform/a64/include/platform/cpucfg.h#L98
[3]:
https://github.com/crust-firmware/crust/blob/master/drivers/css/sun50i-a64-css.c#L24

Re: [PATCH 00/16] arm: Fix handling of unrecognized functions in PSCI emulation

[Peter Maydell]

On Thu, 27 Jan 2022 at 15:46, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This series fixes our handling of PSCI calls where the function ID is
> not recognized. These are supposed to return an error value, but
> currently we instead emulate the SMC or HVC instruction to trap to the
> guest at EL3 or EL2. Particularly of note for code review:
>  * patches 4-9 include some "is this the right behaviour for
>    this hardware" questions for the maintainers of those boards
>  * patch 15 has a DTB API question, as well as being a change in
>    what we edit in a DTB we are passed by the user
>  * testing of the affected machines would be welcome

Thanks to everybody for their review, testing, and confirmation
of the behaviour of particular bits of hardware.

I'm going to apply this to target-arm.next, with patches 5 (allwinner)
and 6 (xilinx) updated to remove the TODO comments, since we've
confirmed that the don't-start-secondaries behaviour is right for
that hardware.

-- PMM

Re: [PATCH 06/16] hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in EL3

[Alexander Graf]

On 27.01.22 16:46, Peter Maydell wrote:
> Change the Xilinx ZynqMP-based board xlnx-zcu102 to use the new
> boot.c functionality to allow us to enable psci-conduit only if
> the guest is being booted in EL1 or EL2, so that if the user runs
> guest EL3 firmware code our PSCI emulation doesn't get in its
> way.
>
> To do this we stop setting the psci-conduit property on the CPU
> objects in the SoC code, and instead set the psci_conduit field in
> the arm_boot_info struct to tell the common boot loader code that
> we'd like PSCI if the guest is starting at an EL that it makes
> sense with.
>
> Note that this means that EL3 guest code will have no way
> to power on secondary cores, because we don't model any
> kind of power controller that does that on this SoC.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


It's been a while since I worked with ZynqMP, but typically your ATF in 
EL3 will want to talk to a microblaze firmware blob on the PMU.

I only see a stand alone PMU machine for microblaze and a PMU IRQ 
handling I/O block in QEMU, but nothing that would listen to the events. 
So I'm fairly sure it will be broken after this patch - and really only 
worked by accident before.

I've added Michal Simek and Stefano Stabellini (both Xilinx) to CC to 
clarify and determine the best path forward here - either disallow EL3 
in the model or build proper PMU emulation in QEMU which then handles 
those PSCI triggered IPI events.


Alex

[1] 
https://github.com/Xilinx/arm-trusted-firmware/blob/master/plat/xilinx/zynqmp/plat_psci.c


> ---
> Again, if anybody knows the real-hardware EL3 behaviour for
> CPUs that would be great.
> ---
>   hw/arm/xlnx-zcu102.c |  1 +
>   hw/arm/xlnx-zynqmp.c | 13 ++++++++-----
>   2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
> index 45eb19ab3b7..4c84bb932aa 100644
> --- a/hw/arm/xlnx-zcu102.c
> +++ b/hw/arm/xlnx-zcu102.c
> @@ -236,6 +236,7 @@ static void xlnx_zcu102_init(MachineState *machine)
>       s->binfo.ram_size = ram_size;
>       s->binfo.loader_start = 0;
>       s->binfo.modify_dtb = zcu102_modify_dtb;
> +    s->binfo.psci_conduit = QEMU_PSCI_CONDUIT_SMC;
>       arm_load_kernel(s->soc.boot_cpu_ptr, machine, &s->binfo);
>   }
>   
> diff --git a/hw/arm/xlnx-zynqmp.c b/hw/arm/xlnx-zynqmp.c
> index 1c52a575aad..17305fe7b76 100644
> --- a/hw/arm/xlnx-zynqmp.c
> +++ b/hw/arm/xlnx-zynqmp.c
> @@ -215,7 +215,10 @@ static void xlnx_zynqmp_create_rpu(MachineState *ms, XlnxZynqMPState *s,
>   
>           name = object_get_canonical_path_component(OBJECT(&s->rpu_cpu[i]));
>           if (strcmp(name, boot_cpu)) {
> -            /* Secondary CPUs start in PSCI powered-down state */
> +            /*
> +             * Secondary CPUs start in powered-down state.
> +             * TODO: check this is what EL3 firmware expects.
> +             */
>               object_property_set_bool(OBJECT(&s->rpu_cpu[i]),
>                                        "start-powered-off", true, &error_abort);
>           } else {
> @@ -435,12 +438,12 @@ static void xlnx_zynqmp_realize(DeviceState *dev, Error **errp)
>       for (i = 0; i < num_apus; i++) {
>           const char *name;
>   
> -        object_property_set_int(OBJECT(&s->apu_cpu[i]), "psci-conduit",
> -                                QEMU_PSCI_CONDUIT_SMC, &error_abort);
> -
>           name = object_get_canonical_path_component(OBJECT(&s->apu_cpu[i]));
>           if (strcmp(name, boot_cpu)) {
> -            /* Secondary CPUs start in PSCI powered-down state */
> +            /*
> +             * Secondary CPUs start in powered-down state.
> +             * TODO: check this is what EL3 firmware expects.
> +             */
>               object_property_set_bool(OBJECT(&s->apu_cpu[i]),
>                                        "start-powered-off", true, &error_abort);
>           } else {

Re: [PATCH 07/16] hw/arm/versal: Let boot.c handle PSCI enablement

[Alexander Graf]

On 27.01.22 16:46, Peter Maydell wrote:
> Instead of setting the CPU psci-conduit and start-powered-off
> properties in the xlnx-versal-virt board code, set the arm_boot_info
> psci_conduit field so that the boot.c code can do it.
>
> This will fix a corner case where we were incorrectly enabling PSCI
> emulation when booting guest code into EL3 because it was an ELF file
> passed to -kernel.  (EL3 guest code started via -bios, -pflash, or
> the generic loader was already being run with PSCI emulation
> disabled.)
>
> Note that EL3 guest code has no way to turn on the secondary CPUs
> because there's no emulated power controller, but this was already
> true for EL3 guest code run via -bios, -pflash, or the generic
> loader.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Let's try with the same CCs here too - Versal uses the same mechanism as 
ZynqMP :)


Alex


> ---
>   include/hw/arm/xlnx-versal.h | 1 -
>   hw/arm/xlnx-versal-virt.c    | 6 ++++--
>   hw/arm/xlnx-versal.c         | 5 +----
>   3 files changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/arm/xlnx-versal.h b/include/hw/arm/xlnx-versal.h
> index 895ba12c61e..2de487564e4 100644
> --- a/include/hw/arm/xlnx-versal.h
> +++ b/include/hw/arm/xlnx-versal.h
> @@ -89,7 +89,6 @@ struct Versal {
>   
>       struct {
>           MemoryRegion *mr_ddr;
> -        uint32_t psci_conduit;
>       } cfg;
>   };
>   
> diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> index 0c5edc898e1..1b25342501b 100644
> --- a/hw/arm/xlnx-versal-virt.c
> +++ b/hw/arm/xlnx-versal-virt.c
> @@ -626,6 +626,9 @@ static void versal_virt_init(MachineState *machine)
>        * When loading an OS, we turn on QEMU's PSCI implementation with SMC
>        * as the PSCI conduit. When there's no -kernel, we assume the user
>        * provides EL3 firmware to handle PSCI.
> +     *
> +     * Even if the user provides a kernel filename, arm_load_kernel()
> +     * may suppress PSCI if it's going to boot that guest code at EL3.
>        */
>       if (machine->kernel_filename) {
>           psci_conduit = QEMU_PSCI_CONDUIT_SMC;
> @@ -635,8 +638,6 @@ static void versal_virt_init(MachineState *machine)
>                               TYPE_XLNX_VERSAL);
>       object_property_set_link(OBJECT(&s->soc), "ddr", OBJECT(machine->ram),
>                                &error_abort);
> -    object_property_set_int(OBJECT(&s->soc), "psci-conduit", psci_conduit,
> -                            &error_abort);
>       sysbus_realize(SYS_BUS_DEVICE(&s->soc), &error_fatal);
>   
>       fdt_create(s);
> @@ -677,6 +678,7 @@ static void versal_virt_init(MachineState *machine)
>       s->binfo.loader_start = 0x0;
>       s->binfo.get_dtb = versal_virt_get_dtb;
>       s->binfo.modify_dtb = versal_virt_modify_dtb;
> +    s->binfo.psci_conduit = psci_conduit;
>       if (machine->kernel_filename) {
>           arm_load_kernel(&s->soc.fpd.apu.cpu[0], machine, &s->binfo);
>       } else {
> diff --git a/hw/arm/xlnx-versal.c b/hw/arm/xlnx-versal.c
> index b2705b6925e..458ba33815f 100644
> --- a/hw/arm/xlnx-versal.c
> +++ b/hw/arm/xlnx-versal.c
> @@ -35,10 +35,8 @@ static void versal_create_apu_cpus(Versal *s)
>           object_initialize_child(OBJECT(s), "apu-cpu[*]", &s->fpd.apu.cpu[i],
>                                   XLNX_VERSAL_ACPU_TYPE);
>           obj = OBJECT(&s->fpd.apu.cpu[i]);
> -        object_property_set_int(obj, "psci-conduit", s->cfg.psci_conduit,
> -                                &error_abort);
>           if (i) {
> -            /* Secondary CPUs start in PSCI powered-down state */
> +            /* Secondary CPUs start in powered-down state */
>               object_property_set_bool(obj, "start-powered-off", true,
>                                        &error_abort);
>           }
> @@ -481,7 +479,6 @@ static void versal_init(Object *obj)
>   static Property versal_properties[] = {
>       DEFINE_PROP_LINK("ddr", Versal, cfg.mr_ddr, TYPE_MEMORY_REGION,
>                        MemoryRegion *),
> -    DEFINE_PROP_UINT32("psci-conduit", Versal, cfg.psci_conduit, 0),
>       DEFINE_PROP_END_OF_LIST()
>   };
>   

Re: [PATCH 10/16] Revert "Revert "arm: tcg: Adhere to SMCCC 1.3 section 5.2""

[Alexander Graf]

On 27.01.22 16:46, Peter Maydell wrote:
> Now that we have arranged for all the affected board models
> to not enable the PSCI emulation if they are running guest code
> at EL3, we can revert commit 4825eaae4fdd56fba0f, thus
> reinstating commit 9fcd15b9193e819b, without bringing back the
> regressions that caused us to revert it.
>
> For clarity, here is the original commit message of 9fcd15b9193e819b:
>
> The SMCCC 1.3 spec section 5.2 says
>
>    The Unknown SMC Function Identifier is a sign-extended value of (-1)
>    that is returned in the R0, W0 or X0 registers. An implementation must
>    return this error code when it receives:
>
>      * An SMC or HVC call with an unknown Function Identifier
>      * An SMC or HVC call for a removed Function Identifier
>      * An SMC64/HVC64 call from AArch32 state
>
> To comply with these statements, let's always return -1 when we encounter
> an unknown HVC or SMC call.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Wouldn't it make more sense to apply the original patch again so that 
git blame points to the correct offender? ;)


Alex

Re: [PATCH 06/16] hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in EL3

[Peter Maydell]

On Mon, 7 Feb 2022 at 14:21, Alexander Graf <agraf@csgraf.de> wrote:
>
>
> On 27.01.22 16:46, Peter Maydell wrote:
> > Change the Xilinx ZynqMP-based board xlnx-zcu102 to use the new
> > boot.c functionality to allow us to enable psci-conduit only if
> > the guest is being booted in EL1 or EL2, so that if the user runs
> > guest EL3 firmware code our PSCI emulation doesn't get in its
> > way.
> >
> > To do this we stop setting the psci-conduit property on the CPU
> > objects in the SoC code, and instead set the psci_conduit field in
> > the arm_boot_info struct to tell the common boot loader code that
> > we'd like PSCI if the guest is starting at an EL that it makes
> > sense with.
> >
> > Note that this means that EL3 guest code will have no way
> > to power on secondary cores, because we don't model any
> > kind of power controller that does that on this SoC.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
>
> It's been a while since I worked with ZynqMP, but typically your ATF in
> EL3 will want to talk to a microblaze firmware blob on the PMU.
>
> I only see a stand alone PMU machine for microblaze and a PMU IRQ
> handling I/O block in QEMU, but nothing that would listen to the events.
> So I'm fairly sure it will be broken after this patch - and really only
> worked by accident before.

Edgar submitted a power-control model patchset:
https://patchew.org/QEMU/20220203140141.310870-1-edgar.iglesias@gmail.com/

thanks
-- PMM

Re: [PATCH 06/16] hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in EL3

[Alexander Graf]

On 07.02.22 16:22, Peter Maydell wrote:
> On Mon, 7 Feb 2022 at 14:21, Alexander Graf <agraf@csgraf.de> wrote:
>>
>> On 27.01.22 16:46, Peter Maydell wrote:
>>> Change the Xilinx ZynqMP-based board xlnx-zcu102 to use the new
>>> boot.c functionality to allow us to enable psci-conduit only if
>>> the guest is being booted in EL1 or EL2, so that if the user runs
>>> guest EL3 firmware code our PSCI emulation doesn't get in its
>>> way.
>>>
>>> To do this we stop setting the psci-conduit property on the CPU
>>> objects in the SoC code, and instead set the psci_conduit field in
>>> the arm_boot_info struct to tell the common boot loader code that
>>> we'd like PSCI if the guest is starting at an EL that it makes
>>> sense with.
>>>
>>> Note that this means that EL3 guest code will have no way
>>> to power on secondary cores, because we don't model any
>>> kind of power controller that does that on this SoC.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> It's been a while since I worked with ZynqMP, but typically your ATF in
>> EL3 will want to talk to a microblaze firmware blob on the PMU.
>>
>> I only see a stand alone PMU machine for microblaze and a PMU IRQ
>> handling I/O block in QEMU, but nothing that would listen to the events.
>> So I'm fairly sure it will be broken after this patch - and really only
>> worked by accident before.
> Edgar submitted a power-control model patchset:
> https://patchew.org/QEMU/20220203140141.310870-1-edgar.iglesias@gmail.com/


Ah, nice. Would this also work for Versal?


Thanks,

Alex

Re: [PATCH 06/16] hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in EL3

[Edgar E. Iglesias]

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

On Mon, Feb 7, 2022 at 4:33 PM Alexander Graf <agraf@csgraf.de> wrote:

>
> On 07.02.22 16:22, Peter Maydell wrote:
> > On Mon, 7 Feb 2022 at 14:21, Alexander Graf <agraf@csgraf.de> wrote:
> >>
> >> On 27.01.22 16:46, Peter Maydell wrote:
> >>> Change the Xilinx ZynqMP-based board xlnx-zcu102 to use the new
> >>> boot.c functionality to allow us to enable psci-conduit only if
> >>> the guest is being booted in EL1 or EL2, so that if the user runs
> >>> guest EL3 firmware code our PSCI emulation doesn't get in its
> >>> way.
> >>>
> >>> To do this we stop setting the psci-conduit property on the CPU
> >>> objects in the SoC code, and instead set the psci_conduit field in
> >>> the arm_boot_info struct to tell the common boot loader code that
> >>> we'd like PSCI if the guest is starting at an EL that it makes
> >>> sense with.
> >>>
> >>> Note that this means that EL3 guest code will have no way
> >>> to power on secondary cores, because we don't model any
> >>> kind of power controller that does that on this SoC.
> >>>
> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >>
> >> It's been a while since I worked with ZynqMP, but typically your ATF in
> >> EL3 will want to talk to a microblaze firmware blob on the PMU.
> >>
> >> I only see a stand alone PMU machine for microblaze and a PMU IRQ
> >> handling I/O block in QEMU, but nothing that would listen to the events.
> >> So I'm fairly sure it will be broken after this patch - and really only
> >> worked by accident before.
> > Edgar submitted a power-control model patchset:
> >
> https://patchew.org/QEMU/20220203140141.310870-1-edgar.iglesias@gmail.com/
>
>
> Ah, nice. Would this also work for Versal?
>
>
> Thanks,
>
> Alex
>

Hi,

Both Versal and ZynqMP require MicroBlaze firmware to run the reference
implementations of Trusted Firmware. We never supported this in upstream
QEMU but we do support it with our fork (by running multiple QEMU instances
co-simulating).

Having said that, we do have tons of EL3 test-cases that we use to validate
QEMU that run with EL3 enabled in upstream.

So there's two user flows:
1. Direct boots using QEMUs builtin PSCI (Most users use this to run Linux,
Xen, U-boot, etc)
2. Firmware boot at EL3 without QEMUs builtin PSCI (Mostly used by
test-code)

Number #2 is the one affected here and that by accident used to have the
builtin PSCI support enabled but now requires more power control modelling
to keep working.
Unless I'm missing something, the -kernel boots will continue to use the
builtin PSCI implementation.

Cheers,
Edgar

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

Re: [PATCH 06/16] hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in EL3

[Alexander Graf]

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


On 07.02.22 16:52, Edgar E. Iglesias wrote:
>
>
> On Mon, Feb 7, 2022 at 4:33 PM Alexander Graf <agraf@csgraf.de> wrote:
>
>
>     On 07.02.22 16:22, Peter Maydell wrote:
>     > On Mon, 7 Feb 2022 at 14:21, Alexander Graf <agraf@csgraf.de> wrote:
>     >>
>     >> On 27.01.22 16:46, Peter Maydell wrote:
>     >>> Change the Xilinx ZynqMP-based board xlnx-zcu102 to use the new
>     >>> boot.c functionality to allow us to enable psci-conduit only if
>     >>> the guest is being booted in EL1 or EL2, so that if the user runs
>     >>> guest EL3 firmware code our PSCI emulation doesn't get in its
>     >>> way.
>     >>>
>     >>> To do this we stop setting the psci-conduit property on the CPU
>     >>> objects in the SoC code, and instead set the psci_conduit field in
>     >>> the arm_boot_info struct to tell the common boot loader code that
>     >>> we'd like PSCI if the guest is starting at an EL that it makes
>     >>> sense with.
>     >>>
>     >>> Note that this means that EL3 guest code will have no way
>     >>> to power on secondary cores, because we don't model any
>     >>> kind of power controller that does that on this SoC.
>     >>>
>     >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>     >>
>     >> It's been a while since I worked with ZynqMP, but typically
>     your ATF in
>     >> EL3 will want to talk to a microblaze firmware blob on the PMU.
>     >>
>     >> I only see a stand alone PMU machine for microblaze and a PMU IRQ
>     >> handling I/O block in QEMU, but nothing that would listen to
>     the events.
>     >> So I'm fairly sure it will be broken after this patch - and
>     really only
>     >> worked by accident before.
>     > Edgar submitted a power-control model patchset:
>     >
>     https://patchew.org/QEMU/20220203140141.310870-1-edgar.iglesias@gmail.com/
>
>
>     Ah, nice. Would this also work for Versal?
>
>
>     Thanks,
>
>     Alex
>
>
> Hi,
>
> Both Versal and ZynqMP require MicroBlaze firmware to run the 
> reference implementations of Trusted Firmware. We never supported this 
> in upstream QEMU but we do support it with our fork (by running 
> multiple QEMU instances co-simulating).
>
> Having said that, we do have tons of EL3 test-cases that we use to 
> validate QEMU that run with EL3 enabled in upstream.
>
> So there's two user flows:
> 1. Direct boots using QEMUs builtin PSCI (Most users use this to run 
> Linux, Xen, U-boot, etc)
> 2. Firmware boot at EL3 without QEMUs builtin PSCI (Mostly used by 
> test-code)
>
> Number #2 is the one affected here and that by accident used to have 
> the builtin PSCI support enabled but now requires more power control 
> modelling to keep working.
> Unless I'm missing something, the -kernel boots will continue to use 
> the builtin PSCI implementation.


So nobody is using upstream QEMU to validate and prototype ATF/EL1s/EL0s 
code? That's a shame :). I suppose there is little value without the 
bitstream emulation and R cluster. Do you have plans to bring multi 
process emulation upstream some day to enable these there?


Alex

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

Re: [PATCH 06/16] hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in EL3

[Philippe Mathieu-Daudé via]

On 7/2/22 16:59, Alexander Graf wrote:
> 
> On 07.02.22 16:52, Edgar E. Iglesias wrote:

>> Both Versal and ZynqMP require MicroBlaze firmware to run the 
>> reference implementations of Trusted Firmware. We never supported this 
>> in upstream QEMU but we do support it with our fork (by running 
>> multiple QEMU instances co-simulating).
>>
>> Having said that, we do have tons of EL3 test-cases that we use to 
>> validate QEMU that run with EL3 enabled in upstream.
>>
>> So there's two user flows:
>> 1. Direct boots using QEMUs builtin PSCI (Most users use this to run 
>> Linux, Xen, U-boot, etc)
>> 2. Firmware boot at EL3 without QEMUs builtin PSCI (Mostly used by 
>> test-code)
>>
>> Number #2 is the one affected here and that by accident used to have 
>> the builtin PSCI support enabled but now requires more power control 
>> modelling to keep working.
>> Unless I'm missing something, the -kernel boots will continue to use 
>> the builtin PSCI implementation.
> 
> 
> So nobody is using upstream QEMU to validate and prototype ATF/EL1s/EL0s 
> code? That's a shame :). I suppose there is little value without the 
> bitstream emulation and R cluster. Do you have plans to bring multi 
> process emulation upstream some day to enable these there?

The R cluster is already in mainstream, isn't it?

Re: [PATCH 06/16] hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in EL3

[Alexander Graf]

On 07.02.22 17:06, Philippe Mathieu-Daudé wrote:
> On 7/2/22 16:59, Alexander Graf wrote:
>>
>> On 07.02.22 16:52, Edgar E. Iglesias wrote:
>
>>> Both Versal and ZynqMP require MicroBlaze firmware to run the 
>>> reference implementations of Trusted Firmware. We never supported 
>>> this in upstream QEMU but we do support it with our fork (by running 
>>> multiple QEMU instances co-simulating).
>>>
>>> Having said that, we do have tons of EL3 test-cases that we use to 
>>> validate QEMU that run with EL3 enabled in upstream.
>>>
>>> So there's two user flows:
>>> 1. Direct boots using QEMUs builtin PSCI (Most users use this to run 
>>> Linux, Xen, U-boot, etc)
>>> 2. Firmware boot at EL3 without QEMUs builtin PSCI (Mostly used by 
>>> test-code)
>>>
>>> Number #2 is the one affected here and that by accident used to have 
>>> the builtin PSCI support enabled but now requires more power control 
>>> modelling to keep working.
>>> Unless I'm missing something, the -kernel boots will continue to use 
>>> the builtin PSCI implementation.
>>
>>
>> So nobody is using upstream QEMU to validate and prototype 
>> ATF/EL1s/EL0s code? That's a shame :). I suppose there is little 
>> value without the bitstream emulation and R cluster. Do you have 
>> plans to bring multi process emulation upstream some day to enable 
>> these there?
>
> The R cluster is already in mainstream, isn't it?


In that case, wouldn't it make sense to build an emulation model of the 
PMU behavior so that normal ATF works out of the box?


Thanks,

Alex

Re: [PATCH 06/16] hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in EL3

[Edgar E. Iglesias]

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

On Mon, Feb 7, 2022 at 5:24 PM Alexander Graf <agraf@csgraf.de> wrote:

>
> On 07.02.22 17:06, Philippe Mathieu-Daudé wrote:
> > On 7/2/22 16:59, Alexander Graf wrote:
> >>
> >> On 07.02.22 16:52, Edgar E. Iglesias wrote:
> >
> >>> Both Versal and ZynqMP require MicroBlaze firmware to run the
> >>> reference implementations of Trusted Firmware. We never supported
> >>> this in upstream QEMU but we do support it with our fork (by running
> >>> multiple QEMU instances co-simulating).
> >>>
> >>> Having said that, we do have tons of EL3 test-cases that we use to
> >>> validate QEMU that run with EL3 enabled in upstream.
> >>>
> >>> So there's two user flows:
> >>> 1. Direct boots using QEMUs builtin PSCI (Most users use this to run
> >>> Linux, Xen, U-boot, etc)
> >>> 2. Firmware boot at EL3 without QEMUs builtin PSCI (Mostly used by
> >>> test-code)
> >>>
> >>> Number #2 is the one affected here and that by accident used to have
> >>> the builtin PSCI support enabled but now requires more power control
> >>> modelling to keep working.
> >>> Unless I'm missing something, the -kernel boots will continue to use
> >>> the builtin PSCI implementation.
> >>
> >>
> >> So nobody is using upstream QEMU to validate and prototype
> >> ATF/EL1s/EL0s code? That's a shame :). I suppose there is little
> >> value without the bitstream emulation and R cluster. Do you have
> >> plans to bring multi process emulation upstream some day to enable
> >> these there?
> >
> > The R cluster is already in mainstream, isn't it?
>
>
> In that case, wouldn't it make sense to build an emulation model of the
> PMU behavior so that normal ATF works out of the box?
>
>
> Thanks,
>
> Alex
>

Yes, that makes sense and there are several ways to implement it. To fully
support the programmability of the PMU we'd need to model the MicroBlazes
together with the ARM cores.

But PMU support does not really conflict with this patch series, or is
there something I'm missing?

Cheers,
Edgar

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

Re: [PATCH 06/16] hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in EL3

[Philippe Mathieu-Daudé via]

On 7/2/22 19:13, Edgar E. Iglesias wrote:
> 
> On Mon, Feb 7, 2022 at 5:24 PM Alexander Graf <agraf@csgraf.de 
> <mailto:agraf@csgraf.de>> wrote:
> 
> 
>     On 07.02.22 17:06, Philippe Mathieu-Daudé wrote:
>      > On 7/2/22 16:59, Alexander Graf wrote:
>      >>
>      >> On 07.02.22 16:52, Edgar E. Iglesias wrote:
>      >
>      >>> Both Versal and ZynqMP require MicroBlaze firmware to run the
>      >>> reference implementations of Trusted Firmware. We never supported
>      >>> this in upstream QEMU but we do support it with our fork (by
>     running
>      >>> multiple QEMU instances co-simulating).
>      >>>
>      >>> Having said that, we do have tons of EL3 test-cases that we use to
>      >>> validate QEMU that run with EL3 enabled in upstream.
>      >>>
>      >>> So there's two user flows:
>      >>> 1. Direct boots using QEMUs builtin PSCI (Most users use this
>     to run
>      >>> Linux, Xen, U-boot, etc)
>      >>> 2. Firmware boot at EL3 without QEMUs builtin PSCI (Mostly used by
>      >>> test-code)
>      >>>
>      >>> Number #2 is the one affected here and that by accident used to
>     have
>      >>> the builtin PSCI support enabled but now requires more power
>     control
>      >>> modelling to keep working.
>      >>> Unless I'm missing something, the -kernel boots will continue
>     to use
>      >>> the builtin PSCI implementation.
>      >>
>      >>
>      >> So nobody is using upstream QEMU to validate and prototype
>      >> ATF/EL1s/EL0s code? That's a shame :). I suppose there is little
>      >> value without the bitstream emulation and R cluster. Do you have
>      >> plans to bring multi process emulation upstream some day to enable
>      >> these there?
>      >
>      > The R cluster is already in mainstream, isn't it?
> 
> 
>     In that case, wouldn't it make sense to build an emulation model of the
>     PMU behavior so that normal ATF works out of the box?
> 
> 
>     Thanks,
> 
>     Alex
> 
> 
> Yes, that makes sense and there are several ways to implement it. To 
> fully support the programmability of the PMU we'd need to model the 
> MicroBlazes together with the ARM cores.
> 
> But PMU support does not really conflict with this patch series, or is 
> there something I'm missing?

My understanding is Alex generically wonders about code coverage, not
about the ZynqMP in particular :)

Re: [PATCH 06/16] hw/arm/xlnx-zcu102: Don't enable PSCI conduit when booting guest in EL3

[Alexander Graf]

On 07.02.22 19:59, Philippe Mathieu-Daudé wrote:
> On 7/2/22 19:13, Edgar E. Iglesias wrote:
>>
>> On Mon, Feb 7, 2022 at 5:24 PM Alexander Graf <agraf@csgraf.de 
>> <mailto:agraf@csgraf.de>> wrote:
>>
>>
>>     On 07.02.22 17:06, Philippe Mathieu-Daudé wrote:
>>      > On 7/2/22 16:59, Alexander Graf wrote:
>>      >>
>>      >> On 07.02.22 16:52, Edgar E. Iglesias wrote:
>>      >
>>      >>> Both Versal and ZynqMP require MicroBlaze firmware to run the
>>      >>> reference implementations of Trusted Firmware. We never 
>> supported
>>      >>> this in upstream QEMU but we do support it with our fork (by
>>     running
>>      >>> multiple QEMU instances co-simulating).
>>      >>>
>>      >>> Having said that, we do have tons of EL3 test-cases that we 
>> use to
>>      >>> validate QEMU that run with EL3 enabled in upstream.
>>      >>>
>>      >>> So there's two user flows:
>>      >>> 1. Direct boots using QEMUs builtin PSCI (Most users use this
>>     to run
>>      >>> Linux, Xen, U-boot, etc)
>>      >>> 2. Firmware boot at EL3 without QEMUs builtin PSCI (Mostly 
>> used by
>>      >>> test-code)
>>      >>>
>>      >>> Number #2 is the one affected here and that by accident used to
>>     have
>>      >>> the builtin PSCI support enabled but now requires more power
>>     control
>>      >>> modelling to keep working.
>>      >>> Unless I'm missing something, the -kernel boots will continue
>>     to use
>>      >>> the builtin PSCI implementation.
>>      >>
>>      >>
>>      >> So nobody is using upstream QEMU to validate and prototype
>>      >> ATF/EL1s/EL0s code? That's a shame :). I suppose there is little
>>      >> value without the bitstream emulation and R cluster. Do you have
>>      >> plans to bring multi process emulation upstream some day to 
>> enable
>>      >> these there?
>>      >
>>      > The R cluster is already in mainstream, isn't it?
>>
>>
>>     In that case, wouldn't it make sense to build an emulation model 
>> of the
>>     PMU behavior so that normal ATF works out of the box?
>>
>>
>>     Thanks,
>>
>>     Alex
>>
>>
>> Yes, that makes sense and there are several ways to implement it. To 
>> fully support the programmability of the PMU we'd need to model the 
>> MicroBlazes together with the ARM cores.
>>
>> But PMU support does not really conflict with this patch series, or 
>> is there something I'm missing?
>
> My understanding is Alex generically wonders about code coverage, not
> about the ZynqMP in particular :)


I'm more curious what the purpose of zynqmp / versal simulation in QEMU 
is. What we're saying here is that we only care about "Linux at EL2 and 
below" plus a Xilinx validation test suite. I understand how multi-QEMU 
emulation may be difficult, but EL3 simulation with Cortex-A plus 
Cortex-R clusters and a simulated PMU sounds like it would get you a 
very long way on simulation coverage.

That said, Xilinx probably knows their user base the best, so if they 
decide that the ability to run TrustZone code is not something they 
believe their users need in QEMU, I'm definitely happy with that stance.


Alex