Re: [Qemu-devel] [PACTH v4 5/6] target-arm: add emulation of PSCI calls for system emulation

[Peter Maydell]

On 10 September 2014 08:02, Ard Biesheuvel <address@hidden> wrote:
> From: Rob Herring <address@hidden>
>
> Add support for handling PSCI calls in system emulation. Both version
> 0.1 and 0.2 of the PSCI spec are supported. Platforms can enable support
> by setting the "psci-conduit" QOM property on the cpus to SMC or HVC
> emulation and having a PSCI binding in their dtb.
>
> Signed-off-by: Rob Herring <address@hidden>
> Signed-off-by: Ard Biesheuvel <address@hidden>

> +    case QEMU_PSCI_0_2_FN_SYSTEM_RESET:
> +        qemu_system_reset_request();
> +        break;
> +    case QEMU_PSCI_0_2_FN_SYSTEM_OFF:
> +        qemu_system_shutdown_request();
> +        break;

I just realised that this isn't quite right: PSCI
mandates that the SYSTEM_RESET and SYSTEM_OFF
functions never return to the caller, but the QEMU
qemu_system_*_request() functions are just requests
which the main loop will later handle asynchronously.
So we should put the calling CPU into power off
(and rely on CPU reset to power it up again if it's
CPU 0; we don't care if we're shutting down, obviously).

I propose to apply the following fixup patch to
deal with this (since this patchset is very nearly
ready and I know Ard's not going to be back to
deal with it for a few weeks):

diff --git a/target-arm/psci.c b/target-arm/psci.c
index 7347cbd..1cda7d3 100644
--- a/target-arm/psci.c
+++ b/target-arm/psci.c
@@ -85,10 +85,15 @@ bool arm_handle_psci(CPUState *cs)
         break;
     case QEMU_PSCI_0_2_FN_SYSTEM_RESET:
         qemu_system_reset_request();
-        break;
+        /* QEMU reset and shutdown are async requests, but PSCI
+         * mandates that we never return from the reset/shutdown
+         * call, so power the CPU off now so it doesn't execute
+         * anything further.
+         */
+        goto cpu_off;
     case QEMU_PSCI_0_2_FN_SYSTEM_OFF:
         qemu_system_shutdown_request();
-        break;
+        goto cpu_off;
     case QEMU_PSCI_0_1_FN_CPU_ON:
     case QEMU_PSCI_0_2_FN_CPU_ON:
     case QEMU_PSCI_0_2_FN64_CPU_ON:
@@ -144,11 +149,7 @@ bool arm_handle_psci(CPUState *cs)
         break;
     case QEMU_PSCI_0_1_FN_CPU_OFF:
     case QEMU_PSCI_0_2_FN_CPU_OFF:
-        cpu->powered_off = true;
-        cs->halted = 1;
-        cs->exception_index = EXCP_HLT;
-        cpu_loop_exit(cs);
-        /* notreached */
+        goto cpu_off;
     case QEMU_PSCI_0_1_FN_CPU_SUSPEND:
     case QEMU_PSCI_0_2_FN_CPU_SUSPEND:
     case QEMU_PSCI_0_2_FN64_CPU_SUSPEND:
@@ -180,4 +181,11 @@ err:
         env->regs[0] = ret;
     }
     return true;
+
+cpu_off:
+    cpu->powered_off = true;
+    cs->halted = 1;
+    cs->exception_index = EXCP_HLT;
+    cpu_loop_exit(cs);
+    /* notreached */
 }

thanks
-- PMM