On 9 September 2014 19:17, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote:
>> From: Rob Herring <rob.herr...@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 "psci-method" QOM property on the cpus to SMC or HVC
>> emulation and having PSCI binding in their dtb.
>>
>> Signed-off-by: Rob Herring <rob.herr...@linaro.org>
>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>> ---
>> target-arm/Makefile.objs | 1 +
>> target-arm/cpu-qom.h | 6 ++
>> target-arm/cpu.c | 10 ++-
>> target-arm/cpu.h | 6 ++
>> target-arm/helper.c | 12 ++++
>> target-arm/psci.c | 171
>> +++++++++++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 203 insertions(+), 3 deletions(-)
>> create mode 100644 target-arm/psci.c
>>
>> diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
>> index dcd167e0d880..9460b409a5a1 100644
>> --- a/target-arm/Makefile.objs
>> +++ b/target-arm/Makefile.objs
>> @@ -7,5 +7,6 @@ obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
>> obj-y += translate.o op_helper.o helper.o cpu.o
>> obj-y += neon_helper.o iwmmxt_helper.o
>> obj-y += gdbstub.o
>> +obj-$(CONFIG_SOFTMMU) += psci.o
>> obj-$(TARGET_AARCH64) += cpu64.o translate-a64.o helper-a64.o gdbstub64.o
>> obj-y += crypto_helper.o
>> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
>> index 104cc67e82d2..469fefb3fec8 100644
>> --- a/target-arm/cpu-qom.h
>> +++ b/target-arm/cpu-qom.h
>> @@ -101,6 +101,11 @@ typedef struct ARMCPU {
>> /* CPU currently in PSCI powered-off state */
>> bool powered_off;
>>
>> + /* PSCI emulation state
>> + * 0 - disabled, 1 - smc, 2 - hvc
>> + */
>> + uint32_t psci_method;
>> +
>> /* [QEMU_]KVM_ARM_TARGET_* constant for this CPU, or
>> * QEMU_KVM_ARM_TARGET_NONE if the kernel doesn't support this CPU type.
>> */
>> @@ -192,6 +197,7 @@ extern const struct VMStateDescription vmstate_arm_cpu;
>> void register_cp_regs_for_features(ARMCPU *cpu);
>> void init_cpreg_list(ARMCPU *cpu);
>>
>> +bool arm_handle_psci(CPUState *cs);
>> bool arm_cpu_do_hvc(CPUState *cs);
>> bool arm_cpu_do_smc(CPUState *cs);
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index b4c06c17cf87..df094fdf5359 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -268,9 +268,12 @@ static void arm_cpu_initfn(Object *obj)
>> cpu->psci_version = 1; /* By default assume PSCI v0.1 */
>> cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
>>
>> - if (tcg_enabled() && !inited) {
>> - inited = true;
>> - arm_translate_init();
>> + if (tcg_enabled()) {
>> + cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
>> + if (!inited) {
>> + inited = true;
>> + arm_translate_init();
>> + }
>> }
>> }
>>
>> @@ -1024,6 +1027,7 @@ static const ARMCPUInfo arm_cpus[] = {
>>
>> static Property arm_cpu_properties[] = {
>> DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
>> + DEFINE_PROP_UINT32("psci-method", ARMCPU, psci_method, 0),
>> DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
>> DEFINE_PROP_END_OF_LIST()
>> };
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index d235929f4c12..2624117e58d3 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -1350,4 +1350,10 @@ static inline void cpu_pc_from_tb(CPUARMState *env,
>> TranslationBlock *tb)
>> }
>> }
>>
>> +enum {
>> + QEMU_PSCI_METHOD_DISABLED = 0,
>> + QEMU_PSCI_METHOD_SMC = 1,
>> + QEMU_PSCI_METHOD_HVC = 2,
>> +};
>> +
>> #endif
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 440ee07a2ff8..021f8ed2c45f 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3499,11 +3499,23 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>>
>> bool arm_cpu_do_hvc(CPUState *cs)
>> {
>> + ARMCPU *cpu = ARM_CPU(cs);
>> +
>> + if (cpu->psci_method == QEMU_PSCI_METHOD_HVC) {
>> + return arm_handle_psci(cs);
>> + }
>> +
>> return false;
>> }
>>
>> bool arm_cpu_do_smc(CPUState *cs)
>> {
>> + ARMCPU *cpu = ARM_CPU(cs);
>> +
>> + if (cpu->psci_method == QEMU_PSCI_METHOD_SMC) {
>> + return arm_handle_psci(cs);
>> + }
>> +
>> return false;
>> }
>>
>> diff --git a/target-arm/psci.c b/target-arm/psci.c
>> new file mode 100644
>> index 000000000000..78efa06b1b00
>> --- /dev/null
>> +++ b/target-arm/psci.c
>> @@ -0,0 +1,171 @@
>> +/*
>> + * Copyright (C) 2014 - Linaro
>> + * Author: Rob Herring <rob.herr...@linaro.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +#include <cpu.h>
>> +#include <cpu-qom.h>
>> +#include <kvm-consts.h>
>> +#include <sysemu/sysemu.h>
>> +
>> +bool arm_handle_psci(CPUState *cs)
>> +{
>> + /*
>> + * This function partially implements the logic for dispatching Power
>> State
>> + * Coordination Interface (PSCI) calls (as described in ARM DEN
>> 0022B.b),
>> + * to the extent required for bringing up and taking down secondary
>> cores,
>> + * and for handling reset and poweroff requests.
>> + * Additional information about the calling convention used is
>> available in
>> + * the document 'SMC Calling Convention' (ARM DEN 0028)
>> + */
>> + ARMCPU *cpu = ARM_CPU(cs);
>> + CPUARMState *env = &cpu->env;
>> + uint64_t param[4];
>> + uint64_t context_id, mpidr;
>> + target_ulong entry;
>> + int32_t ret = 0;
>> + int i;
>> +
>> + for (i = 0; i < 4; i++) {
>> + /*
>> + * All PSCI functions take explicit 32-bit or native int sized
>> + * arguments so we can simply zero-extend all arguments regardless
>> + * of which exact function we are about to call.
>> + */
>> + param[i] = is_a64(env) ? env->xregs[i] : env->regs[i];
>> + }
>> +
>> + if ((param[0] & QEMU_PSCI_0_2_64BIT) && !is_a64(env)) {
>> + ret = QEMU_PSCI_RET_INVALID_PARAMS;
>> + goto err;
>> + }
>> +
>> + switch (param[0]) {
>> + CPUState *target_cpu_state;
>> + ARMCPU *target_cpu;
>> + CPUClass *target_cpu_class;
>> +
>> + case QEMU_PSCI_0_2_FN_PSCI_VERSION:
>> + ret = QEMU_PSCI_0_2_RET_VERSION_0_2;
>> + break;
>> + case QEMU_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
>> + ret = QEMU_PSCI_0_2_RET_TOS_MIGRATION_NOT_REQUIRED; /* No trusted
>> OS */
>> + break;
>> + case QEMU_PSCI_0_2_FN_AFFINITY_INFO:
>> + case QEMU_PSCI_0_2_FN64_AFFINITY_INFO:
>> + mpidr = param[1];
>> +
>> + switch (param[2]) {
>> + case 0:
>> + target_cpu_state = qemu_get_cpu(mpidr & 0xff);
>> + if (!target_cpu_state) {
>> + ret = QEMU_PSCI_RET_INVALID_PARAMS;
>> + break;
>> + }
>> + target_cpu = ARM_CPU(target_cpu_state);
>> + ret = target_cpu->powered_off ? 1 : 0;
>> + break;
>> + default:
>> + /* Everything above affinity level 0 is always on. */
>> + ret = 0;
>> + }
>> + break;
>> + 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;
>> + case QEMU_PSCI_0_1_FN_CPU_ON:
>> + case QEMU_PSCI_0_2_FN_CPU_ON:
>> + case QEMU_PSCI_0_2_FN64_CPU_ON:
>> + mpidr = param[1];
>> + entry = param[2];
>> + context_id = param[3];
>> +
>> + /* change to the cpu we are powering up */
>> + target_cpu_state = qemu_get_cpu(mpidr & 0xff);
>> + if (!target_cpu_state) {
>> + ret = QEMU_PSCI_RET_INVALID_PARAMS;
>> + break;
>> + }
>> + target_cpu = ARM_CPU(target_cpu_state);
>> + if (!target_cpu->powered_off) {
>> + ret = QEMU_PSCI_RET_ALREADY_ON;
>> + break;
>> + }
>> + target_cpu_class = CPU_GET_CLASS(target_cpu);
>> +
>> + /* Initialize the cpu we are turning on */
>> + cpu_reset(target_cpu_state);
>> + target_cpu_class->set_pc(target_cpu_state, entry);
>
> You need to do this set_pc after setting the
> aarch64/aarch32 flag below, otherwise it won't set the
> correct internal PC state. (But see remarks below.)
>
OK
> For AArch32 you need to honour bit 0 of the entry point
> address which means we need to switch into Thumb mode.
> (For AArch64 the spec requires us to fail the call
> with INVALID_PARAMS if bit 0 is set.)
>
OK
>> + target_cpu->powered_off = false;
>> + target_cpu_state->interrupt_request |= CPU_INTERRUPT_EXITTB;
>
> I don't see why this line is needed -- the target
> CPU can't possibly be midway through executing
> a TB, because it was powered off.
>
I suppose Rob may have had his reasons for putting this here -- Rob?
>> +
>> + /*
>> + * The PSCI spec mandates that newly brought up CPUs enter the
>> + * exception level of the caller in the same execution mode as
>> + * the caller, with context_id in x0/r0, respectively.
>> + */
>> + if (is_a64(env)) {
>> + target_cpu->env.aarch64 = 1;
>> + target_cpu->env.xregs[0] = context_id;
>> + } else {
>> + target_cpu->env.aarch64 = 0;
>> + target_cpu->env.regs[0] = context_id;
>> + }
>
> I don't think you can safely just set the aarch64/aarch32
> flag like this. You'd need to also at least write a valid
> CPSR and generally do more of the things an exception return
> would do.
>
Well, setting the flag should be redundant in fact, so I agree that
asserting they are equal should be sufficient.
> I think in fact since for us EL1 is currently the highest
> possible exception level, it's never possible for the
> CPU reset state to be at a different register width from
> the register width of the caller. So we should just
> assert that. (When EL2/EL3 support get added this code
> will need to be enhanced to cope with that anyway,
> since at that point the EL that we start the guest
> CPU in isn't the same as the EL the CPU resets in.)
>
indeed.
>> +
>> + ret = 0;
>> + break;
>> + case QEMU_PSCI_0_1_FN_CPU_OFF:
>> + case QEMU_PSCI_0_2_FN_CPU_OFF:
>> + cpu->powered_off = true;
>> + cs->exit_request = 1;
>> + cs->halted = 1;
>> +
>> + /* CPU_OFF should never return, but if it does return an error */
>> + ret = QEMU_PSCI_RET_DENIED;
>
> I think it would be better to do:
> cpu->powered_off = true;
> cs->halted = 1;
> cs->exception_index = EXCP_HLT;
> cpu_loop_exit(cs);
> /* notreached */
>
OK let me try that
>> + break;
>> + case QEMU_PSCI_0_1_FN_CPU_SUSPEND:
>> + case QEMU_PSCI_0_2_FN_CPU_SUSPEND:
>> + case QEMU_PSCI_0_2_FN64_CPU_SUSPEND:
>> + /* Affinity levels are not supported in QEMU */
>> + if (param[1] & 0xfffe0000) {
>> + ret = QEMU_PSCI_RET_INVALID_PARAMS;
>> + break;
>> + }
>> + /* Powerdown is not supported, we always go into WFI */
>> + cs->halted = 1;
>> + cs->exit_request = 1;
>> +
>> + /* Return success when we wakeup */
>> + ret = 0;
>
> I think you could just call helper_wfi() here
> instead of doing things manually (which will
> longjump you out of here, as opposed to your
> returning with flags set such that the main cpu
> loop does a longjmp on your behalf). You would
> need to set the xregs[0] return value before
> that though...
>
OK
>> + break;
>> + case QEMU_PSCI_0_1_FN_MIGRATE:
>> + case QEMU_PSCI_0_2_FN_MIGRATE:
>> + ret = QEMU_PSCI_RET_NOT_SUPPORTED;
>> + break;
>> + default:
>> + return false;
>> + }
>> +
>> +err:
>> + if (is_a64(env)) {
>> + env->xregs[0] = ret;
>> + } else {
>> + env->regs[0] = ret;
>> + }
>> + return true;
>> +}
>> --
>> 1.8.3.2
>
> thanks
> -- PMM
