On 2 March 2016 at 05:27, Jean-Christophe Dubois <j...@tribudubois.net> wrote: > Split ARM on/off function from PSCI support code. > > This will allow to reuse these functions in other code. > > Signed-off-by: Jean-Christophe Dubois <j...@tribudubois.net> > --- > > Changes since V1: > * Not present on V1 > > Changes since V2: > * Not present on V2 > > target-arm/Makefile.objs | 1 + > target-arm/arm-powerctl.c | 156 > ++++++++++++++++++++++++++++++++++++++++++++++ > target-arm/arm-powerctl.h | 22 +++++++ > target-arm/psci.c | 72 ++------------------- > 4 files changed, 184 insertions(+), 67 deletions(-) > create mode 100644 target-arm/arm-powerctl.c > create mode 100644 target-arm/arm-powerctl.h > > diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs > index a80eb39..60fd1dd 100644 > --- a/target-arm/Makefile.objs > +++ b/target-arm/Makefile.objs > @@ -9,3 +9,4 @@ obj-y += neon_helper.o iwmmxt_helper.o > obj-y += gdbstub.o > obj-$(TARGET_AARCH64) += cpu64.o translate-a64.o helper-a64.o gdbstub64.o > obj-y += crypto_helper.o > +obj-y += arm-powerctl.o > diff --git a/target-arm/arm-powerctl.c b/target-arm/arm-powerctl.c > new file mode 100644 > index 0000000..99c2af2 > --- /dev/null > +++ b/target-arm/arm-powerctl.c > @@ -0,0 +1,156 @@ > +/* > + * QEMU support -- ARM Power Control specific functions. > + * > + * Copyright (c) 2016 Jean-Christophe Dubois > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + * > + */ > + > +#include "qemu/osdep.h" > +#include <cpu.h> > +#include <cpu-qom.h> > +#include "arm-powerctl.h" > + > +#ifndef DEBUG_ARM_POWERCTL > +#define DEBUG_ARM_POWERCTL 0 > +#endif > + > +#define DPRINTF(fmt, args...) \ > + do { \ > + if (DEBUG_ARM_POWERCTL) { \ > + fprintf(stderr, "[ARM]%s: " fmt , __func__, ##args); \ > + } \ > + } while (0) > + > +CPUState *arm_get_cpu_by_id(uint64_t id) > +{ > + CPUState *cpu; > + > + DPRINTF("cpu %" PRId64 "\n", id); > + > + CPU_FOREACH(cpu) { > + ARMCPU *armcpu = ARM_CPU(cpu); > + > + if (armcpu->mp_affinity == id) { > + return cpu; > + } > + } > + > + qemu_log_mask(LOG_GUEST_ERROR, > + "[ARM]%s: Resquesting unknown CPU %" PRId64 "\n", > + __func__, id);
"Requesting" > + > + return NULL; > +} > + > +int arm_set_cpu_on(uint64_t cpuid, uint64_t entry, uint64_t context_id) > +{ > + CPUState *target_cpu_state; > + ARMCPU *target_cpu; > + CPUClass *target_cpu_class; > + CPUARMState *env = &ARM_CPU(current_cpu)->env; > + > + DPRINTF("cpu %" PRId64 " @ 0x%" PRIx64 " with R0 = 0x%" PRIx64 "\n", > + cpuid, entry, context_id); > + > + /* change to the cpu we are powering up */ > + target_cpu_state = arm_get_cpu_by_id(cpuid); > + if (!target_cpu_state) { > + return -1; > + } > + target_cpu = ARM_CPU(target_cpu_state); > + if (!target_cpu->powered_off) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "[ARM]%s: CPU %" PRId64 " is already running\n", > + __func__, cpuid); > + return -1; > + } > + target_cpu_class = CPU_GET_CLASS(target_cpu); > + > + /* Initialize the cpu we are turning on */ > + cpu_reset(target_cpu_state); > + target_cpu->powered_off = false; > + target_cpu_state->halted = 0; > + > + /* > + * 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. This is a generic function so it's a bit out of place for it to talk about PSCI spec details. What are the semantics provided by this helper function? > + * > + * For now, it is sufficient to assert() that CPUs come out of > + * reset in the same mode as the calling CPU, since we only > + * implement EL1, which means that It's no longer true that we only implement EL1, incidentally. > + * (a) there is no EL2 for the calling CPU to trap into to change > + * its state > + * (b) the newly brought up CPU enters EL1 immediately after coming > + * out of reset in the default state > + */ > + assert(is_a64(env) == is_a64(&target_cpu->env)); > + if (is_a64(env)) { > + if (entry & 1) { > + return -1; > + } > + target_cpu->env.xregs[0] = context_id; > + } else { > + target_cpu->env.regs[0] = context_id; > + target_cpu->env.thumb = entry & 1; > + } > + > + target_cpu_class->set_pc(target_cpu_state, entry); > + > + return 0; > +} > + > +void arm_set_cpu_off(uint64_t cpuid) > +{ > + CPUState *target_cpu_state; > + ARMCPU *target_cpu; > + > + DPRINTF("cpu %" PRId64 "\n", cpuid); > + > + /* change to the cpu we are powering up */ > + target_cpu_state = arm_get_cpu_by_id(cpuid); > + if (!target_cpu_state) { > + return; > + } > + target_cpu = ARM_CPU(target_cpu_state); > + if (target_cpu->powered_off) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "[ARM]%s: CPU %" PRId64 " is already off\n", > + __func__, cpuid); > + return; > + } > + > + target_cpu->powered_off = true; > + target_cpu_state->halted = 1; > + target_cpu_state->exception_index = EXCP_HLT; > + cpu_loop_exit(target_cpu_state); > +} > + > +int arm_reset_cpu(uint64_t cpuid) > +{ > + CPUState *target_cpu_state; > + ARMCPU *target_cpu; > + > + DPRINTF("cpu %" PRId64 "\n", cpuid); > + > + /* change to the cpu we are resetting */ > + target_cpu_state = arm_get_cpu_by_id(cpuid); > + if (!target_cpu_state) { > + return -1; > + } > + target_cpu = ARM_CPU(target_cpu_state); > + if (target_cpu->powered_off) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "[ARM]%s: CPU %" PRId64 " is off\n", > + __func__, cpuid); Whether requesting a power-off of a powered off CPU is a guest error depends on the API used to request it, so you can't log this as a guest error in the generic helper function. > + return -1; > + } > + > + /* Reset the cpu */ > + cpu_reset(target_cpu_state); > + > + return 0; > +} > diff --git a/target-arm/arm-powerctl.h b/target-arm/arm-powerctl.h > new file mode 100644 > index 0000000..13fb526 > --- /dev/null > +++ b/target-arm/arm-powerctl.h > @@ -0,0 +1,22 @@ > +/* > + * QEMU support -- ARM Power Control specific functions. > + * > + * Copyright (c) 2016 Jean-Christophe Dubois > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + * > + */ > + > +#ifndef QEMU_ARM_POWERCTL_H > +#define QEMU_ARM_POWERCTL_H > + > +CPUState *arm_get_cpu_by_id(uint64_t id); > + > +int arm_set_cpu_on(uint64_t cpuid, uint64_t entry, uint64_t context_id); > + > +void arm_set_cpu_off(uint64_t cpuid); > + > +int arm_reset_cpu(uint64_t cpuid); Please provide doc comments for new globally-visible functions. > + > +#endif > diff --git a/target-arm/psci.c b/target-arm/psci.c > index c55487f..362105f 100644 > --- a/target-arm/psci.c > +++ b/target-arm/psci.c > @@ -22,6 +22,7 @@ > #include <kvm-consts.h> > #include <sysemu/sysemu.h> > #include "internals.h" > +#include "arm-powerctl.h" > > bool arm_is_psci_call(ARMCPU *cpu, int excp_type) > { > @@ -73,21 +74,6 @@ bool arm_is_psci_call(ARMCPU *cpu, int excp_type) > } > } > > -static CPUState *get_cpu_by_id(uint64_t id) > -{ > - CPUState *cpu; > - > - CPU_FOREACH(cpu) { > - ARMCPU *armcpu = ARM_CPU(cpu); > - > - if (armcpu->mp_affinity == id) { > - return cpu; > - } > - } > - > - return NULL; > -} > - > void arm_handle_psci_call(ARMCPU *cpu) > { > /* > @@ -98,7 +84,6 @@ void arm_handle_psci_call(ARMCPU *cpu) > * Additional information about the calling convention used is available > in > * the document 'SMC Calling Convention' (ARM DEN 0028) > */ > - CPUState *cs = CPU(cpu); > CPUARMState *env = &cpu->env; > uint64_t param[4]; > uint64_t context_id, mpidr; > @@ -123,7 +108,6 @@ void arm_handle_psci_call(ARMCPU *cpu) > 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; > @@ -137,7 +121,7 @@ void arm_handle_psci_call(ARMCPU *cpu) > > switch (param[2]) { > case 0: > - target_cpu_state = get_cpu_by_id(mpidr); > + target_cpu_state = arm_get_cpu_by_id(mpidr); > if (!target_cpu_state) { > ret = QEMU_PSCI_RET_INVALID_PARAMS; > break; > @@ -168,51 +152,8 @@ void arm_handle_psci_call(ARMCPU *cpu) > entry = param[2]; > context_id = param[3]; > > - /* change to the cpu we are powering up */ > - target_cpu_state = get_cpu_by_id(mpidr); > - 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; > - } You've lost the distinction between these two failure modes -- the PSCI error codes are returned to the guest and are mandated by the PSCI spec so you need to distinguish them. > - target_cpu_class = CPU_GET_CLASS(target_cpu); > - > - /* Initialize the cpu we are turning on */ > - cpu_reset(target_cpu_state); > - target_cpu->powered_off = false; > - target_cpu_state->halted = 0; > - > - /* > - * 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. > - * > - * For now, it is sufficient to assert() that CPUs come out of > - * reset in the same mode as the calling CPU, since we only > - * implement EL1, which means that > - * (a) there is no EL2 for the calling CPU to trap into to change > - * its state > - * (b) the newly brought up CPU enters EL1 immediately after coming > - * out of reset in the default state > - */ > - assert(is_a64(env) == is_a64(&target_cpu->env)); > - if (is_a64(env)) { > - if (entry & 1) { > - ret = QEMU_PSCI_RET_INVALID_PARAMS; > - break; > - } > - target_cpu->env.xregs[0] = context_id; > - } else { > - target_cpu->env.regs[0] = context_id; > - target_cpu->env.thumb = entry & 1; > - } > - target_cpu_class->set_pc(target_cpu_state, entry); > - > - ret = 0; > + ret = arm_set_cpu_on(mpidr, entry, context_id) ? > + QEMU_PSCI_RET_INVALID_PARAMS : 0; > break; > case QEMU_PSCI_0_1_FN_CPU_OFF: > case QEMU_PSCI_0_2_FN_CPU_OFF: > @@ -250,9 +191,6 @@ err: > return; > > cpu_off: > - cpu->powered_off = true; > - cs->halted = 1; > - cs->exception_index = EXCP_HLT; > - cpu_loop_exit(cs); > + arm_set_cpu_off(cpu->mp_affinity); > /* notreached */ > } > -- > 2.5.0 > thanks -- PMM