On 02/19/16 22:41, Ard Biesheuvel wrote:
> On 19 February 2016 at 22:03, Laszlo Ersek <ler...@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.mayd...@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);
>>>
>>> /*
>>>
>>
