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