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 {