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

[Laszlo Ersek <lersek@redhat.com>]

On 03/01/16 18:39, 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 <peter.maydell@linaro.org>
> ---
> v1->v2 changes:
>  * rearranged so we initialise vbi->using_psci properly
> 
>  hw/arm/virt.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 4499e2c..9d07b6d 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";
> @@ -1108,6 +1114,15 @@ static void machvirt_init(MachineState *machine)
>          exit(1);
>      }
>  
> +    /* 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.
> +     * The usual case is that we do use QEMU's PSCI implementation.
> +     */
> +    vbi->using_psci = !(vms->secure && firmware_loaded);
> +
>      /* The maximum number of CPUs depends on the GIC version, or on how
>       * many redistributors we can fit into the memory map.
>       */
> @@ -1175,12 +1190,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 +1267,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);
>  
>      /*
> 

I compared this with v1 and the discussion under it:

http://thread.gmane.org/gmane.comp.emulators.qemu/395305

The new, explicit condition looks valid.

Also, the location is right; albeit not visible in the context above,
"vbi->using_psci" is set right after "vbi" is set from find_machine_info().

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo