qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 1/6] ARM: Factor out ARM on/off PSCI control


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH v4 1/6] ARM: Factor out ARM on/off PSCI control functions
Date: Wed, 23 Mar 2016 15:24:48 +0000

On 19 March 2016 at 20:59, 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
>
> Changes since V3:
>  * Move to a more generic/usefull API
>  * Manage CPU level/mode change at startup
>  * Allow PSCI to cope with EL2/HYP level.
>  * Keep distinct errors for different causes.
>
>  target-arm/Makefile.objs  |   1 +
>  target-arm/arm-powerctl.c | 224 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  target-arm/arm-powerctl.h |  44 +++++++++
>  target-arm/psci.c         |  69 ++------------
>  4 files changed, 275 insertions(+), 63 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..8ebb3d8
> --- /dev/null
> +++ b/target-arm/arm-powerctl.c
> @@ -0,0 +1,224 @@
> +/*
> + * 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 "internals.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: Requesting unknown CPU %" PRId64 "\n",
> +                  __func__, id);
> +
> +    return NULL;
> +}
> +
> +int arm_set_cpu_on(uint64_t cpuid, uint64_t entry, uint64_t context_id,
> +                   int target_el, bool target_aa64)
> +{
> +    CPUState *target_cpu_state;
> +    ARMCPU *target_cpu;
> +    CPUClass *target_cpu_class;
> +
> +    DPRINTF("cpu %" PRId64 " (EL %d, %s) @ 0x%" PRIx64 " with R0 = 0x%" 
> PRIx64 "\n",
> +            cpuid, target_el, target_aa64 ? "aarch64" : "aarch32", entry,
> +            context_id);
> +
> +    /* requested EL level need to be above 0 */
> +    assert(target_el >= 1 && target_el <= 3);
> +
> +    /* change to the cpu we are powering up */
> +    target_cpu_state = arm_get_cpu_by_id(cpuid);
> +    if (!target_cpu_state) {
> +        /* The cpu was not found */
> +        return QEMU_ARM_POWERCTL_INVALID_PARAM;
> +    }
> +    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 QEMU_ARM_POWERCTL_ALREADY_ON;
> +    }
> +    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 newly brought CPU is requested to enter the exception level
> +     * "target_el" and be in the requested mode (aarch64 ou aarch32).
> +     */
> +
> +    /*
> +     * Check that the CPU is supporting the requested level
> +     */
> +    switch (target_el) {
> +    case 3:
> +        if (arm_feature(&target_cpu->env, ARM_FEATURE_EL3)) {
> +            if (is_a64(&target_cpu->env)) {

Why are we looking at the is_a64() state of the CPU as it comes
out of reset? What we care about is target_aa64 here (if we
are putting the CPU into a 64-bit state at a lower exception
level then we must ensure the state is consistent with all
the higher levels being 64-bit).

> +                /* Lower EL3 exception is aarch64 */
> +                target_cpu->env.cp15.scr_el3 |= SCR_RW;
> +                if (arm_feature(&target_cpu->env, ARM_FEATURE_EL2)) {
> +                    /* Lower EL2 exception is aarch64 */
> +                    target_cpu->env.cp15.hcr_el2 |= HCR_RW;

If you're starting in EL3 you don't need to mess with any of
these registers, because they control register width for
EL2 and EL1. You can let the guest do that itself.

> +                }
> +                target_cpu->env.pstate = PSTATE_MODE_EL3h;

> +            } else {
> +                switch_mode(&target_cpu->env, ARM_CPU_MODE_MON);

switch_mode() is an internal function (and does some register
bank copying we don't need); use
 cpsr_write(&target_cpu->env, ARM_CPU_MODE_MON, CPSR_M, CPSRWriteRaw);


> +            }
> +            /* Processor is in secure mode */
> +            target_cpu->env.cp15.scr_el3 &= ~SCR_NS;
> +        } else {
> +            return QEMU_ARM_POWERCTL_INVALID_PARAM;
> +        }
> +        break;
> +    case 2:
> +        if (arm_feature(&target_cpu->env, ARM_FEATURE_EL2)) {
> +            if (is_a64(&target_cpu->env)) {
> +                if (arm_feature(&target_cpu->env, ARM_FEATURE_EL3)) {
> +                    /* Lower EL3 exception is aarch64 */
> +                    target_cpu->env.cp15.scr_el3 |= SCR_RW;
> +                }
> +                /* Lower EL2 exception is aarch64 */
> +                target_cpu->env.cp15.hcr_el2 |= HCR_RW;

Again, to go into EL2 you don't need to worry about setting HCR_EL2.RW.

> +                target_cpu->env.pstate = PSTATE_MODE_EL2h;
> +            } else {
> +                switch_mode(&target_cpu->env, ARM_CPU_MODE_HYP);
> +            }
> +            /* Processor is not in secure mode */
> +            target_cpu->env.cp15.scr_el3 |= SCR_NS;
> +        } else {
> +            return QEMU_ARM_POWERCTL_INVALID_PARAM;
> +        }
> +        break;
> +    case 1:
> +    default:
> +        if (is_a64(&target_cpu->env)) {
> +            if (arm_feature(&target_cpu->env, ARM_FEATURE_EL3)) {
> +                /* Lower EL3 exception is aarch64 */
> +                target_cpu->env.cp15.scr_el3 |= SCR_RW;
> +            }
> +            if (arm_feature(&target_cpu->env, ARM_FEATURE_EL2)) {
> +                /* Lower EL2 exception is aarch64 */
> +                target_cpu->env.cp15.hcr_el2 |= HCR_RW;
> +            }
> +            target_cpu->env.pstate = PSTATE_MODE_EL1h;
> +        } else {
> +            switch_mode(&target_cpu->env, ARM_CPU_MODE_SVC);
> +        }
> +        /* Processor is not in secure mode */
> +        target_cpu->env.cp15.scr_el3 |= SCR_NS;
> +        break;
> +    }
> +
> +    /* We assert if the request cannot be fulfilled */
> +    assert(target_aa64 == arm_el_is_aa64(&target_cpu->env, target_el));

So, when can this happen, or is it genuinely "can't happen" ?

> +
> +    /* We check if the started CPU is now in the correct level */
> +    assert(target_el == arm_current_el(&target_cpu->env));
> +
> +    if (target_aa64) {
> +        /* Thumb32 is not supported on AARCH64 */
> +        if (entry & 1) {
> +            return QEMU_ARM_POWERCTL_INVALID_PARAM;
> +        }

If you're going to check the PC alignment for 64-bit then you might
as well check that it's 4-aligned.

> +        target_cpu->env.xregs[0] = context_id;
> +    } else {
> +        target_cpu->env.regs[0] = context_id;
> +        target_cpu->env.thumb = entry & 1;
> +    }
> +
> +    /* Start the new CPU at the requested address */
> +    target_cpu_class->set_pc(target_cpu_state, entry);
> +
> +    /* We are good to go */
> +    return QEMU_ARM_POWERCTL_RET_SUCCESS;
> +}
> +
> +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 QEMU_ARM_POWERCTL_INVALID_PARAM;
> +    }
> +    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);
> +        return -1;
> +    }
> +
> +    /* Reset the cpu */
> +    cpu_reset(target_cpu_state);
> +
> +    return QEMU_ARM_POWERCTL_RET_SUCCESS;
> +}
> diff --git a/target-arm/arm-powerctl.h b/target-arm/arm-powerctl.h
> new file mode 100644
> index 0000000..2195483
> --- /dev/null
> +++ b/target-arm/arm-powerctl.h
> @@ -0,0 +1,44 @@
> +/*
> + * 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
> +
> +#include "kvm-consts.h"
> +
> +#define QEMU_ARM_POWERCTL_RET_SUCCESS QEMU_PSCI_RET_SUCCESS
> +#define QEMU_ARM_POWERCTL_INVALID_PARAM QEMU_PSCI_RET_INVALID_PARAMS
> +#define QEMU_ARM_POWERCTL_ALREADY_ON QEMU_PSCI_RET_ALREADY_ON
> +
> +/*
> + * Retreive a CPUState object from its CPU ID
> + */
> +CPUState *arm_get_cpu_by_id(uint64_t id);

New global functions should all have properly formatted doc comments.

> +
> +/*
> + * Start a give CPU. The CPU will start running at address "entry" with
> + * "context_id" in r0/x0. The CPU should start at exception level "target_el"
> + * and in mode aa64 or aa32 depending on "target_aa64"
> + */
> +int arm_set_cpu_on(uint64_t cpuid, uint64_t entry, uint64_t context_id,
> +                   int target_el, bool target_aa64);

What's the return value? Do we start the CPU secure or nonsecure?
In AArch32, in which mode do we start? We need to either document
these things or let the caller specify.

(For PSCI we want to always start in non-secure, and we want to start
in the same execution state as the calling cpu, which I take to include
the mode. We won't ever try to start a cpu in EL3.)

> +
> +/*
> + * Stop/Power off a given CPU
> + */
> +
> +void arm_set_cpu_off(uint64_t cpuid);
> +
> +/*
> + * Reset a given CPU
> + */
> +int arm_reset_cpu(uint64_t cpuid);
> +
> +#endif
> diff --git a/target-arm/psci.c b/target-arm/psci.c
> index c55487f..f1c8eb2 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;
> @@ -167,52 +151,14 @@ void arm_handle_psci_call(ARMCPU *cpu)
>          mpidr = param[1];
>          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;
> -        }
> -        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,
> +                             arm_current_el(&ARM_CPU(current_cpu)->env),
> +                             is_a64(&ARM_CPU(current_cpu)->env));

You already have the current CPU's env in the 'env' variable;
you don't need to go via current_cpu for this.

>          break;
>      case QEMU_PSCI_0_1_FN_CPU_OFF:
>      case QEMU_PSCI_0_2_FN_CPU_OFF:
> @@ -250,9 +196,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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]