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

[Laszlo Ersek]

On 02/19/16 22:41, Ard Biesheuvel wrote:
> On 19 February 2016 at 22:03, Laszlo Ersek <address@hidden> wrote:
>> Ard, any opinion on this?
>>
> 
> I agree with Peter. Note that this is strictly about emulation, under
> KVM we always run at EL1 or below and PSCI calls are handled by the
> host kernel, not QEMU

Great, that's all I wanted to hear -- out of scope for me. :)

Actually, I have now read the patch even, and I have the following comments:

- As long as "using_psci" is true, the behavior doesn't change. Great.

- The only place where using_psci *changes* to false is reachable only
with (vms->secure && firmware_loaded). That's what wasn't immediately
obvious from the patch -- when vms->secure is true (-machine secure=on),
it's out of scope for me. :)

- However, I think I might have noticed a bug -- or rather missed
something trivial --, namely, where is "using_psci" *initially* set to
true? The "machines" static array is not touched.

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);
>>>
>>>      /*
>>>
>>