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