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

["Cédric Le Goater" <clg@kaod.org>]

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