diff mbox

hw/arm/virt: Assume EL3 boot rom will handle PSCI if one is provided

Message ID 1455898919-13868-1-git-send-email-peter.maydell@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Maydell Feb. 19, 2016, 4:21 p.m. UTC
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>
---
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(-)

Comments

Laszlo Ersek Feb. 19, 2016, 9:03 p.m. UTC | #1
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 <peter.maydell@linaro.org>
> ---
> 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);
>  
>      /*
>
Ard Biesheuvel Feb. 19, 2016, 9:41 p.m. UTC | #2
On 19 February 2016 at 22:03, Laszlo Ersek <lersek@redhat.com> 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 <peter.maydell@linaro.org>
>> ---
>> 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);
>>
>>      /*
>>
>
Laszlo Ersek Feb. 19, 2016, 9:58 p.m. UTC | #3
On 02/19/16 22:41, Ard Biesheuvel wrote:
> On 19 February 2016 at 22:03, Laszlo Ersek <lersek@redhat.com> 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 <peter.maydell@linaro.org>
>>> ---
>>> 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);
>>>
>>>      /*
>>>
>>
Peter Maydell Feb. 20, 2016, 11:31 a.m. UTC | #4
On 19 February 2016 at 21:58, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/19/16 22:41, Ard Biesheuvel wrote:
>> On 19 February 2016 at 22:03, Laszlo Ersek <lersek@redhat.com> 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.

Derp. I changed at the last minute from having using_psci be a
local bool in the machvirt_init() function to putting it in the
vbi struct, and forgot that when I deleted the "bool using_psci = true;"
I needed to add something to init vbi->using_psci to true.

thanks
-- PMM
Laszlo Ersek Feb. 22, 2016, 10:12 a.m. UTC | #5
On 02/20/16 12:31, Peter Maydell wrote:
> On 19 February 2016 at 21:58, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 02/19/16 22:41, Ard Biesheuvel wrote:
>>> On 19 February 2016 at 22:03, Laszlo Ersek <lersek@redhat.com> 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.
> 
> Derp. I changed at the last minute from having using_psci be a
> local bool in the machvirt_init() function to putting it in the
> vbi struct, and forgot that when I deleted the "bool using_psci = true;"

Independently, I think that may be why some projects (edk2 for example,
and perhaps even some of the BSDs?) forbid initialization of auto
variables in their coding styles. Could be a little heavy-handed, so I'm
certainly not suggesting that QEMU adopt that rule; it's just
interesting to see that it really occurs sometimes.

Thanks!
Laszlo

> I needed to add something to init vbi->using_psci to true.
> 
> thanks
> -- PMM
>
diff mbox

Patch

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