Re: [Qemu-devel] [PATCH] hw/arm/virt: Assume EL3 boot rom will handle PSCI if one is provided

[Laszlo Ersek]

Ard, any opinion on this?

Thanks
Laszlo

On 02/19/16 17:21, Peter Maydell wrote:
> If the user passes us an EL3 boot rom, then it is going to want to
> implement the PSCI interface itself. In this case, disable QEMU's
> internal PSCI implementation so it does not get in the way, and
> instead start all CPUs in an SMP configuration at once (the boot
> rom will catch them all and pen up the secondaries until needed).
> The boot rom code is also responsible for editing the device tree
> to include any necessary information about its own PSCI implementation
> before eventually passing it to a NonSecure guest.
> 
> (This "start all CPUs at once" approach is what both ARM Trusted
> Firmware and UEFI expect, since it is what the ARM Foundation Model
> does; the other approach would be to provide some emulated hardware
> for "start the secondaries" but this is simplest.)
> 
> This is a compatibility break, but I don't believe that anybody
> was using a secure boot ROM with an SMP configuration. Such a setup
> would be somewhat broken since there was nothing preventing nonsecure
> guest code from calling the QEMU PSCI function to start up a secondary
> core in a way that completely bypassed the secure world.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> This is slightly RFC-ish because I don't actually have any secure boot
> rom code that will cope with SMP right now. But I think that since this
> is a compat break we're better off putting it into 2.6 than not.
> 
>  hw/arm/virt.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 4499e2c..0f01902 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -73,6 +73,7 @@ typedef struct VirtBoardInfo {
>      uint32_t clock_phandle;
>      uint32_t gic_phandle;
>      uint32_t v2m_phandle;
> +    bool using_psci;
>  } VirtBoardInfo;
>  
>  typedef struct {
> @@ -231,6 +232,10 @@ static void fdt_add_psci_node(const VirtBoardInfo *vbi)
>      void *fdt = vbi->fdt;
>      ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
>  
> +    if (!vbi->using_psci) {
> +        return;
> +    }
> +
>      qemu_fdt_add_subnode(fdt, "/psci");
>      if (armcpu->psci_version == 2) {
>          const char comp[] = "arm,psci-0.2\0arm,psci";
> @@ -342,7 +347,7 @@ static void fdt_add_cpu_nodes(const VirtBoardInfo *vbi)
>          qemu_fdt_setprop_string(vbi->fdt, nodename, "compatible",
>                                      armcpu->dtb_compatible);
>  
> -        if (vbi->smp_cpus > 1) {
> +        if (vbi->using_psci && vbi->smp_cpus > 1) {
>              qemu_fdt_setprop_string(vbi->fdt, nodename,
>                                          "enable-method", "psci");
>          }
> @@ -1081,6 +1086,7 @@ static void machvirt_init(MachineState *machine)
>      VirtGuestInfoState *guest_info_state = g_malloc0(sizeof 
> *guest_info_state);
>      VirtGuestInfo *guest_info = &guest_info_state->info;
>      char **cpustr;
> +    bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>  
>      if (!cpu_model) {
>          cpu_model = "cortex-a15";
> @@ -1146,6 +1152,16 @@ static void machvirt_init(MachineState *machine)
>          memory_region_init(secure_sysmem, OBJECT(machine), "secure-memory",
>                             UINT64_MAX);
>          memory_region_add_subregion_overlap(secure_sysmem, 0, sysmem, -1);
> +
> +        if (firmware_loaded) {
> +            /* If we have an EL3 boot ROM then the assumption is that it will
> +             * implement PSCI itself, so disable QEMU's internal 
> implementation
> +             * so it doesn't get in the way. Instead of starting secondary
> +             * CPUs in PSCI powerdown state we will start them all running 
> and
> +             * let the boot ROM sort them out.
> +             */
> +            vbi->using_psci = false;
> +        }
>      }
>  
>      create_fdt(vbi);
> @@ -1175,12 +1191,15 @@ static void machvirt_init(MachineState *machine)
>              object_property_set_bool(cpuobj, false, "has_el3", NULL);
>          }
>  
> -        object_property_set_int(cpuobj, QEMU_PSCI_CONDUIT_HVC, 
> "psci-conduit",
> -                                NULL);
> +        if (vbi->using_psci) {
> +            object_property_set_int(cpuobj, QEMU_PSCI_CONDUIT_HVC,
> +                                    "psci-conduit", NULL);
>  
> -        /* Secondary CPUs start in PSCI powered-down state */
> -        if (n > 0) {
> -            object_property_set_bool(cpuobj, true, "start-powered-off", 
> NULL);
> +            /* Secondary CPUs start in PSCI powered-down state */
> +            if (n > 0) {
> +                object_property_set_bool(cpuobj, true,
> +                                         "start-powered-off", NULL);
> +            }
>          }
>  
>          if (object_property_find(cpuobj, "reset-cbar", NULL)) {
> @@ -1249,7 +1268,7 @@ static void machvirt_init(MachineState *machine)
>      vbi->bootinfo.board_id = -1;
>      vbi->bootinfo.loader_start = vbi->memmap[VIRT_MEM].base;
>      vbi->bootinfo.get_dtb = machvirt_dtb;
> -    vbi->bootinfo.firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
> +    vbi->bootinfo.firmware_loaded = firmware_loaded;
>      arm_load_kernel(ARM_CPU(first_cpu), &vbi->bootinfo);
>  
>      /*
>