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

[Ard Biesheuvel]

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



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