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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 26/09/2014 11:10, Peter Maydell ha scritto:
> On 10 September 2014 08:02, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> From: Rob Herring <rob.herring@linaro.org>
>>
>> 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 <rob.herring@linaro.org>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
>> +    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):

FWIW, looks good.

Thanks,

Paolo

> 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
> 
>