Re: [Qemu-devel] [PATCH v3 06/12] ARM: Factor out ARM on/off PSCI control functions

[Peter Maydell]

On 2 March 2016 at 05:27, Jean-Christophe Dubois <address@hidden> 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 <address@hidden>
> ---
>
> 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