[Qemu-devel] [PATCH 0/7] ARM: add PSCI enulation support

[Qemu-devel] [PATCH 0/7] ARM: add PSCI enulation support

[Rob Herring]

From: Rob Herring <rob.herring@linaro.org>

This series adds support for enulating ARM PSCI calls. PSCI or Power 
State Coordination Interface is an ARM standard for controlling cpu 
power states. This series supports both AArch32 and AArch64 using HVC or 
SMC calls.

This series is based on Pranavkumar Sawargaonkar's series for PSCI 0.2 
support in KVM[1].

Rob

[1] http://lists.gnu.org/archive/html/qemu-devel/2014-05/msg00421.html

Rob Herring (7):
  target-arm: use correct do_interrupt handler for AArch64 user mode
  target-arm: add powered off cpu state
  target-arm: add hvc and smc exception emulation handling
    infrastructure
  target-arm: support AArch64 for arm_cpu_set_pc
  target-arm: add emulation of PSCI calls for system emulation
  arm/virt: enable PSCI emulation support for system emulation
  arm/highbank: enable PSCI emulation support

 hw/arm/highbank.c          |   8 +++
 hw/arm/virt.c              |  42 ++++++-------
 target-arm/Makefile.objs   |   1 +
 target-arm/cpu-qom.h       |  10 +++
 target-arm/cpu.c           |  13 ++--
 target-arm/cpu.h           |  14 +++++
 target-arm/cpu64.c         |   4 ++
 target-arm/helper-a64.c    |  14 +++++
 target-arm/helper.c        |  33 ++++++++++
 target-arm/internals.h     |  15 +++++
 target-arm/kvm-consts.h    |   6 ++
 target-arm/psci.c          | 152 +++++++++++++++++++++++++++++++++++++++++++++
 target-arm/translate-a64.c |  13 +++-
 target-arm/translate.c     |  24 ++++---
 14 files changed, 308 insertions(+), 41 deletions(-)
 create mode 100644 target-arm/psci.c

-- 
1.9.1

[Qemu-devel] [PATCH 1/7] target-arm: use correct do_interrupt handler for AArch64 user mode

[Rob Herring]

From: Rob Herring <rob.herring@linaro.org>

User mode emulation should never get interrupts and thus should not
use the system emulation exception handler function.

Signed-off-by: Rob Herring <rob.herring@linaro.org>
---
 target-arm/cpu64.c      | 4 ++++
 target-arm/helper-a64.c | 3 +++
 2 files changed, 7 insertions(+)

diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index 8daa622..98d402f 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -187,7 +187,11 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
 {
     CPUClass *cc = CPU_CLASS(oc);
 
+#if defined(CONFIG_USER_ONLY)
+    cc->do_interrupt = arm_cpu_do_interrupt;
+#else
     cc->do_interrupt = aarch64_cpu_do_interrupt;
+#endif
     cc->set_pc = aarch64_cpu_set_pc;
     cc->gdb_read_register = aarch64_cpu_gdb_read_register;
     cc->gdb_write_register = aarch64_cpu_gdb_write_register;
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index bf921cc..84411b4 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -438,6 +438,8 @@ float32 HELPER(fcvtx_f64_to_f32)(float64 a, CPUARMState *env)
     return r;
 }
 
+#if !defined(CONFIG_USER_ONLY)
+
 /* Handle a CPU exception.  */
 void aarch64_cpu_do_interrupt(CPUState *cs)
 {
@@ -512,3 +514,4 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     env->pc = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
 }
+#endif
-- 
1.9.1

[Qemu-devel] [PATCH 2/7] target-arm: add powered off cpu state

[Rob Herring]

From: Rob Herring <rob.herring@linaro.org>

Add tracking of cpu power state in order to support powering off of
cores in system emuluation. The initial state is determined by the
start-powered-off QOM property.

Signed-off-by: Rob Herring <rob.herring@linaro.org>
---
 target-arm/cpu-qom.h | 1 +
 target-arm/cpu.c     | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index edc7f26..8ccb227 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -96,6 +96,7 @@ typedef struct ARMCPU {
 
     /* Should CPU start in PSCI powered-off state? */
     bool start_powered_off;
+    bool powered_off;
 
     /* [QEMU_]KVM_ARM_TARGET_* constant for this CPU, or
      * QEMU_KVM_ARM_TARGET_NONE if the kernel doesn't support this CPU type.
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index c0ddc3e..03c025b 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -39,7 +39,9 @@ static void arm_cpu_set_pc(CPUState *cs, vaddr value)
 
 static bool arm_cpu_has_work(CPUState *cs)
 {
-    return cs->interrupt_request &
+    ARMCPU *cpu = ARM_CPU(cs);
+
+    return !cpu->powered_off && cs->interrupt_request &
         (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD | CPU_INTERRUPT_EXITTB);
 }
 
@@ -90,6 +92,9 @@ static void arm_cpu_reset(CPUState *s)
     env->vfp.xregs[ARM_VFP_MVFR1] = cpu->mvfr1;
     env->vfp.xregs[ARM_VFP_MVFR2] = cpu->mvfr2;
 
+    cpu->powered_off = cpu->start_powered_off;
+    s->halted = cpu->start_powered_off;
+
     if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
         env->iwmmxt.cregs[ARM_IWMMXT_wCID] = 0x69051000 | 'Q';
     }
-- 
1.9.1

[Qemu-devel] [PATCH 3/7] target-arm: add hvc and smc exception emulation handling infrastructure

[Rob Herring]

From: Rob Herring <rob.herring@linaro.org>

Add the infrastructure to handle and emulate hvc and smc exceptions.
This will enable emulation of things such as PSCI calls. This commit
does not change the behavior and will exit with unknown exception.

Signed-off-by: Rob Herring <rob.herring@linaro.org>
---
 target-arm/cpu-qom.h       |  3 +++
 target-arm/cpu.h           |  2 ++
 target-arm/helper-a64.c    | 11 +++++++++++
 target-arm/helper.c        | 33 +++++++++++++++++++++++++++++++++
 target-arm/internals.h     | 15 +++++++++++++++
 target-arm/translate-a64.c | 13 ++++++++++---
 target-arm/translate.c     | 24 +++++++++++++++++-------
 7 files changed, 91 insertions(+), 10 deletions(-)

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 8ccb227..88aaf6a 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -185,6 +185,9 @@ extern const struct VMStateDescription vmstate_arm_cpu;
 void register_cp_regs_for_features(ARMCPU *cpu);
 void init_cpreg_list(ARMCPU *cpu);
 
+bool arm_cpu_do_hvc(CPUState *cs);
+bool arm_cpu_do_smc(CPUState *cs);
+
 void arm_cpu_do_interrupt(CPUState *cpu);
 void arm_v7m_cpu_do_interrupt(CPUState *cpu);
 
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index c83f249..905ba02 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -51,6 +51,8 @@
 #define EXCP_EXCEPTION_EXIT  8   /* Return from v7M exception.  */
 #define EXCP_KERNEL_TRAP     9   /* Jumped to kernel code page.  */
 #define EXCP_STREX          10
+#define EXCP_HVC            11
+#define EXCP_SMC            12
 
 #define ARMV7M_EXCP_RESET   1
 #define ARMV7M_EXCP_NMI     2
diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 84411b4..d2c1097 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -485,6 +485,17 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     case EXCP_FIQ:
         addr += 0x100;
         break;
+    case EXCP_HVC:
+        if (arm_cpu_do_hvc(cs)) {
+            return;
+        }
+        cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
+        return;
+    case EXCP_SMC:
+        if (arm_cpu_do_smc(cs)) {
+            return;
+        }
+        /* Fall-though */
     default:
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
     }
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3be917c..b5b4a17 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3253,6 +3253,28 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     env->thumb = addr & 1;
 }
 
+bool arm_cpu_do_hvc(CPUState *cs)
+{
+    bool ret;
+
+    ret = arm_handle_psci(cs);
+    if (ret) {
+        return ret;
+    }
+    return false;
+}
+
+bool arm_cpu_do_smc(CPUState *cs)
+{
+    bool ret;
+
+    ret = arm_handle_psci(cs);
+    if (ret) {
+        return ret;
+    }
+    return false;
+}
+
 /* Handle a CPU exception.  */
 void arm_cpu_do_interrupt(CPUState *cs)
 {
@@ -3355,6 +3377,17 @@ void arm_cpu_do_interrupt(CPUState *cs)
         mask = CPSR_A | CPSR_I | CPSR_F;
         offset = 4;
         break;
+    case EXCP_HVC:
+        if (arm_cpu_do_hvc(cs)) {
+            return;
+        }
+        cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
+        return;
+    case EXCP_SMC:
+        if (arm_cpu_do_smc(cs)) {
+            return;
+        }
+        /* Fall-though */
     default:
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
         return; /* Never happens.  Keep compiler happy.  */
diff --git a/target-arm/internals.h b/target-arm/internals.h
index d63a975..c71eabb 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -184,6 +184,21 @@ static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
         | (is_thumb ? 0 : ARM_EL_IL);
 }
 
+static inline uint32_t syn_aa64_hvc(uint32_t imm16)
+{
+    return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
+}
+
+static inline uint32_t syn_aa32_hvc(uint32_t imm16)
+{
+    return (EC_AA32_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
+}
+
+static inline uint32_t syn_aa64_smc(uint32_t imm16)
+{
+    return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
+}
+
 static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
 {
     return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index b62db4d..fa49ed8 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1449,11 +1449,18 @@ static void disas_exc(DisasContext *s, uint32_t insn)
         /* SVC, HVC, SMC; since we don't support the Virtualization
          * or TrustZone extensions these all UNDEF except SVC.
          */
-        if (op2_ll != 1) {
-            unallocated_encoding(s);
+        switch (op2_ll) {
+        case 1:
+            gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
+            break;
+        case 2:
+            gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_smc(imm16));
+            break;
+        case 3:
+            gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_hvc(imm16));
             break;
         }
-        gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
+        unallocated_encoding(s);
         break;
     case 1:
         if (op2_ll != 0) {
diff --git a/target-arm/translate.c b/target-arm/translate.c
index a4d920b..13ece7f 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -7727,9 +7727,14 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
         case 7:
         {
             int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4);
-            /* SMC instruction (op1 == 3)
-               and undefined instructions (op1 == 0 || op1 == 2)
-               will trap */
+            /* HVC and SMC instructions */
+            if (op1 == 2) {
+                gen_exception_insn(s, 0, EXCP_HVC, imm16);
+                break;
+            } else if (op1 == 3) {
+                gen_exception_insn(s, 0, EXCP_SMC, 0);
+                break;
+            }
             if (op1 != 1) {
                 goto illegal_op;
             }
@@ -9555,10 +9560,15 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                     goto illegal_op;
 
                 if (insn & (1 << 26)) {
-                    /* Secure monitor call (v6Z) */
-                    qemu_log_mask(LOG_UNIMP,
-                                  "arm: unimplemented secure monitor call\n");
-                    goto illegal_op; /* not implemented.  */
+                    if (!(insn & (1 << 20))) {
+                        /* Hypervisor call (v7) */
+                        uint32_t imm16 = extract32(insn, 16, 4);
+                        imm16 |= extract32(insn, 0, 12) << 4;
+                        gen_exception_insn(s, 0, EXCP_HVC, imm16);
+                    } else {
+                        /* Secure monitor call (v6+) */
+                        gen_exception_insn(s, 0, EXCP_SMC, 0);
+                    }
                 } else {
                     op = (insn >> 20) & 7;
                     switch (op) {
-- 
1.9.1

[Qemu-devel] [PATCH 4/7] target-arm: support AArch64 for arm_cpu_set_pc

[Rob Herring]

From: Rob Herring <rob.herring@linaro.org>

Add AArch64 support to arm_cpu_set_pc and make it available to other files.

Signed-off-by: Rob Herring <rob.herring@linaro.org>
---
 target-arm/cpu.c |  7 -------
 target-arm/cpu.h | 12 ++++++++++++
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 03c025b..2d18a20 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -30,13 +30,6 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 
-static void arm_cpu_set_pc(CPUState *cs, vaddr value)
-{
-    ARMCPU *cpu = ARM_CPU(cs);
-
-    cpu->env.regs[15] = value;
-}
-
 static bool arm_cpu_has_work(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 905ba02..efe3cd2 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1197,6 +1197,18 @@ static inline void cpu_pc_from_tb(CPUARMState *env, TranslationBlock *tb)
     }
 }
 
+static inline void arm_cpu_set_pc(CPUState *cs, vaddr value)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+
+    if (is_a64(&cpu->env)) {
+        cpu->env.pc = value;
+    } else {
+        cpu->env.regs[15] = value;
+    }
+
+}
+
 /* Load an instruction and return it in the standard little-endian order */
 static inline uint32_t arm_ldl_code(CPUARMState *env, target_ulong addr,
                                     bool do_swap)
-- 
1.9.1

[Qemu-devel] [PATCH 5/7] target-arm: add emulation of PSCI calls for system emulation

[Rob Herring]

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 "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.herring@linaro.org>
---
 target-arm/Makefile.objs |   1 +
 target-arm/cpu-qom.h     |   6 ++
 target-arm/cpu.c         |   1 +
 target-arm/helper.c      |  16 ++---
 target-arm/kvm-consts.h  |   6 ++
 target-arm/psci.c        | 152 +++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 174 insertions(+), 8 deletions(-)
 create mode 100644 target-arm/psci.c

diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
index dcd167e..deda9f4 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-y += 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 88aaf6a..2905525 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -98,6 +98,11 @@ typedef struct ARMCPU {
     bool start_powered_off;
     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.
      */
@@ -185,6 +190,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 2d18a20..eb21a52 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -1009,6 +1009,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/helper.c b/target-arm/helper.c
index b5b4a17..637c46a 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3255,23 +3255,23 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
 
 bool arm_cpu_do_hvc(CPUState *cs)
 {
-    bool ret;
+    ARMCPU *cpu = ARM_CPU(cs);
 
-    ret = arm_handle_psci(cs);
-    if (ret) {
-        return ret;
+    if (cpu->psci_method == QEMU_PSCI_METHOD_HVC) {
+        return arm_handle_psci(cs);
     }
+
     return false;
 }
 
 bool arm_cpu_do_smc(CPUState *cs)
 {
-    bool ret;
+    ARMCPU *cpu = ARM_CPU(cs);
 
-    ret = arm_handle_psci(cs);
-    if (ret) {
-        return ret;
+    if (cpu->psci_method == QEMU_PSCI_METHOD_SMC) {
+        return arm_handle_psci(cs);
     }
+
     return false;
 }
 
diff --git a/target-arm/kvm-consts.h b/target-arm/kvm-consts.h
index 5cf93ab..d0a89c7 100644
--- a/target-arm/kvm-consts.h
+++ b/target-arm/kvm-consts.h
@@ -91,6 +91,12 @@ MISMATCH_CHECK(QEMU_PSCI_0_2_FN64_MIGRATE, PSCI_0_2_FN64_MIGRATE)
 MISMATCH_CHECK(QEMU_PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU, \
                PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU)
 
+enum {
+    QEMU_PSCI_METHOD_DISABLED = 0,
+    QEMU_PSCI_METHOD_SMC = 1,
+    QEMU_PSCI_METHOD_HVC = 2,
+};
+
 /* Note that KVM uses overlapping values for AArch32 and AArch64
  * target CPU numbers. AArch32 targets:
  */
diff --git a/target-arm/psci.c b/target-arm/psci.c
new file mode 100644
index 0000000..5c66236
--- /dev/null
+++ b/target-arm/psci.c
@@ -0,0 +1,152 @@
+/*
+ * Copyright (C) 2014 - Linaro
+ * Author: Rob Herring <rob.herring@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>
+#include <linux/psci.h>
+
+#if !defined(CONFIG_USER_ONLY)
+
+bool arm_handle_psci(CPUState *cs)
+{
+    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++) {
+        param[i] = is_a64(env) ? env->xregs[i] : env->regs[i];
+    }
+
+    if ((param[0] & PSCI_0_2_64BIT) && !is_a64(env)) {
+        ret = PSCI_RET_INVALID_PARAMS;
+        goto err;
+    }
+
+    switch (param[0]) {
+    case PSCI_0_2_FN_PSCI_VERSION:
+        ret = PSCI_VERSION_MAJOR(0) | PSCI_VERSION_MINOR(2);
+        break;
+    case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
+        ret = PSCI_0_2_TOS_MP;    /* No trusted OS */
+        break;
+    case PSCI_0_2_FN_AFFINITY_INFO:
+    case PSCI_0_2_FN64_AFFINITY_INFO:
+        mpidr = param[1];
+
+        switch (param[2]) {
+        case 0:
+            cs = qemu_get_cpu(mpidr & 0xff);
+            if (!cs) {
+                ret = PSCI_RET_INVALID_PARAMS;
+                break;
+            }
+            cpu = ARM_CPU(cs);
+            ret = cpu->powered_off ? 1 : 0;
+            break;
+        default:
+            /* Everything above affinity level 0 is always on. */
+            ret = 0;
+        }
+        break;
+    case PSCI_0_2_FN_SYSTEM_RESET:
+        qemu_system_reset_request();
+        break;
+    case PSCI_0_2_FN_SYSTEM_OFF:
+        qemu_system_powerdown_request();
+        break;
+    case QEMU_PSCI_FN_CPU_ON:
+    case PSCI_0_2_FN_CPU_ON:
+    case PSCI_0_2_FN64_CPU_ON:
+        mpidr = param[1];
+        entry = param[2];
+        context_id = param[3];
+
+        /* change to the cpu we are powering up */
+        cs = qemu_get_cpu(mpidr & 0xff);
+        if (!cs) {
+            ret = PSCI_RET_INVALID_PARAMS;
+            break;
+        }
+        cpu = ARM_CPU(cs);
+
+        if (!cpu->powered_off) {
+            ret = PSCI_RET_ALREADY_ON;
+            break;
+        }
+
+        /* Initialize the cpu we are turning on */
+        cpu_reset(cs);
+        arm_cpu_set_pc(cs, entry);
+        cpu->powered_off = false;
+        cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+        /* Set the context_id in r0/x0 */
+        if (is_a64(env)) {
+            cpu->env.xregs[0] = context_id;
+        } else {
+            cpu->env.regs[0] = context_id;
+        }
+
+        ret = 0;
+        break;
+    case QEMU_PSCI_FN_CPU_OFF:
+    case 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 = PSCI_RET_DENIED;
+        break;
+    case QEMU_PSCI_FN_CPU_SUSPEND:
+    case PSCI_0_2_FN_CPU_SUSPEND:
+    case PSCI_0_2_FN64_CPU_SUSPEND:
+        /* Affinity levels are not supported in QEMU */
+        if (param[1] & 0xfffe0000) {
+            ret = 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;
+        break;
+    case QEMU_PSCI_FN_MIGRATE:
+    case PSCI_0_2_FN_MIGRATE:
+        ret = PSCI_RET_NOT_SUPPORTED;
+        break;
+    default:
+        return false;
+    }
+
+err:
+    if (is_a64(env)) {
+        env->xregs[0] = ret;
+    } else {
+        env->regs[0] = ret;
+    }
+    return true;
+}
+#endif
-- 
1.9.1

[Qemu-devel] [PATCH 6/7] arm/virt: enable PSCI emulation support for system emulation

[Rob Herring]

From: Rob Herring <rob.herring@linaro.org>

Now that we have PSCI emulation, enable it for the virt platform.
This simplifies the virt machine a bit now that PSCI and SMP no longer
need to be KVM only features.

Signed-off-by: Rob Herring <rob.herring@linaro.org>
---

Note: This will need to be rebased as comments on KVM PSCI 0.2 support 
are addressed. This patch effectively includes my comments.

 hw/arm/virt.c | 42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9f28c12..d8ab88d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -181,21 +181,22 @@ static void create_fdt(VirtBoardInfo *vbi)
                                 "clk24mhz");
     qemu_fdt_setprop_cell(fdt, "/apb-pclk", "phandle", vbi->clock_phandle);
 
-    /* No PSCI for TCG yet */
-    if (kvm_enabled()) {
-        qemu_fdt_add_subnode(fdt, "/psci");
-        if (kvm_check_extension(kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
-            qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci-0.2");
-        } else {
-            qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci");
-            qemu_fdt_setprop_cell(fdt, "/psci", "cpu_suspend",
-                                  PSCI_FN_CPU_SUSPEND);
-            qemu_fdt_setprop_cell(fdt, "/psci", "cpu_off", PSCI_FN_CPU_OFF);
-            qemu_fdt_setprop_cell(fdt, "/psci", "cpu_on", PSCI_FN_CPU_ON);
-            qemu_fdt_setprop_cell(fdt, "/psci", "migrate", PSCI_FN_MIGRATE);
-        }
-        qemu_fdt_setprop_string(fdt, "/psci", "method", "hvc");
+    qemu_fdt_add_subnode(fdt, "/psci");
+    qemu_fdt_setprop_string(fdt, "/psci", "method", "hvc");
+    if (kvm_enabled() && !kvm_check_extension(kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
+        qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci");
+    } else {
+        const char compat[] = "arm,psci-0.2\0arm,psci";
+        qemu_fdt_setprop(fdt, "/psci", "compatible", compat, sizeof(compat));
     }
+    qemu_fdt_setprop_cell(fdt, "/psci", "cpu_suspend",
+                              QEMU_PSCI_FN_CPU_SUSPEND);
+    qemu_fdt_setprop_cell(fdt, "/psci", "cpu_off",
+                              QEMU_PSCI_FN_CPU_OFF);
+    qemu_fdt_setprop_cell(fdt, "/psci", "cpu_on",
+                              QEMU_PSCI_FN_CPU_ON);
+    qemu_fdt_setprop_cell(fdt, "/psci", "migrate",
+                              QEMU_PSCI_FN_MIGRATE);
 }
 
 static void fdt_add_timer_nodes(const VirtBoardInfo *vbi)
@@ -410,16 +411,6 @@ static void machvirt_init(QEMUMachineInitArgs *args)
 
     vbi->smp_cpus = smp_cpus;
 
-    /*
-     * Only supported method of starting secondary CPUs is PSCI and
-     * PSCI is not yet supported with TCG, so limit smp_cpus to 1
-     * if we're not using KVM.
-     */
-    if (!kvm_enabled() && smp_cpus > 1) {
-        error_report("mach-virt: must enable KVM to use multiple CPUs");
-        exit(1);
-    }
-
     if (args->ram_size > vbi->memmap[VIRT_MEM].size) {
         error_report("mach-virt: cannot model more than 30GB RAM");
         exit(1);
@@ -438,6 +429,9 @@ static void machvirt_init(QEMUMachineInitArgs *args)
         }
         cpuobj = object_new(object_class_get_name(oc));
 
+        object_property_set_int(cpuobj, QEMU_PSCI_METHOD_HVC, "psci-method",
+                                NULL);
+
         /* Secondary CPUs start in PSCI powered-down state */
         if (n > 0) {
             object_property_set_bool(cpuobj, true, "start-powered-off", NULL);
-- 
1.9.1

[Qemu-devel] [PATCH 7/7] arm/highbank: enable PSCI emulation support

[Rob Herring]

From: Rob Herring <rob.herring@linaro.org>

Enable PSCI enulation on highbank and midway platforms.

Note that this requires fixing the PSCI function IDs in the DTB to match
what QEMU is using. This should get fixed.

Signed-off-by: Rob Herring <rob.herring@linaro.org>
---
 hw/arm/highbank.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
index 46b9f1e..092df1f 100644
--- a/hw/arm/highbank.c
+++ b/hw/arm/highbank.c
@@ -242,6 +242,14 @@ static void calxeda_init(QEMUMachineInitArgs *args, enum cxmachines machine)
         cpuobj = object_new(object_class_get_name(oc));
         cpu = ARM_CPU(cpuobj);
 
+        object_property_set_int(cpuobj, QEMU_PSCI_METHOD_SMC, "psci-method",
+                                NULL);
+
+        /* Secondary CPUs start in PSCI powered-down state */
+        if (n > 0) {
+            object_property_set_bool(cpuobj, true, "start-powered-off", NULL);
+        }
+
         if (object_property_find(cpuobj, "reset-cbar", NULL)) {
             object_property_set_int(cpuobj, MPCORE_PERIPHBASE,
                                     "reset-cbar", &error_abort);
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/7] target-arm: use correct do_interrupt handler for AArch64 user mode

[Peter Maydell]

On 5 May 2014 17:00, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@linaro.org>
>
> User mode emulation should never get interrupts and thus should not
> use the system emulation exception handler function.

This is true, but arm_cpu_do_interrupt() is also a system
emulation exception handler function, so it's no better.

I assume we're doing this because we're about to add
code to aarch64_cpu_do_interrupt() which doesn't compile
in user mode, though you don't mention this in the commit
message.

> @@ -187,7 +187,11 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      CPUClass *cc = CPU_CLASS(oc);
>
> +#if defined(CONFIG_USER_ONLY)
> +    cc->do_interrupt = arm_cpu_do_interrupt;
> +#else
>      cc->do_interrupt = aarch64_cpu_do_interrupt;
> +#endif

I think you can simply only do the assignment ifndef
CONFIG_USER_ONLY (which will leave the pointer NULL
for user-mode) -- it will never be called (and if it does
it'll be easier to find the bug if it's a segfault than if it
tries to execute the 32 bit system mode interrupt code...)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/7] target-arm: support AArch64 for arm_cpu_set_pc

[Peter Maydell]

On 5 May 2014 17:00, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@linaro.org>
>
> Add AArch64 support to arm_cpu_set_pc and make it available to other files.
>
> Signed-off-by: Rob Herring <rob.herring@linaro.org>
> ---
>  target-arm/cpu.c |  7 -------
>  target-arm/cpu.h | 12 ++++++++++++
>  2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 03c025b..2d18a20 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -30,13 +30,6 @@
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
>
> -static void arm_cpu_set_pc(CPUState *cs, vaddr value)
> -{
> -    ARMCPU *cpu = ARM_CPU(cs);
> -
> -    cpu->env.regs[15] = value;
> -}
> -
>  static bool arm_cpu_has_work(CPUState *cs)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 905ba02..efe3cd2 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1197,6 +1197,18 @@ static inline void cpu_pc_from_tb(CPUARMState *env, TranslationBlock *tb)
>      }
>  }
>
> +static inline void arm_cpu_set_pc(CPUState *cs, vaddr value)
> +{
> +    ARMCPU *cpu = ARM_CPU(cs);
> +
> +    if (is_a64(&cpu->env)) {
> +        cpu->env.pc = value;
> +    } else {
> +        cpu->env.regs[15] = value;
> +    }
> +
> +}

The set_pc() functions are supposed to be private implementations
of methods on the CPU object. This one is fine where it is;
the AArch64 support is in aarch64_cpu_set_pc(). In either case
you should only be calling it via the CPUClass->set_pc pointer.
Assuming you have a CPUState *cpu, then:

    CPUClass *cc = CPU_GET_CLASS(cpu);
    cc->set_pc(cpu, pc);

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/7] target-arm: add powered off cpu state

[Peter Maydell]

On 5 May 2014 17:00, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@linaro.org>
>
> Add tracking of cpu power state in order to support powering off of
> cores in system emuluation. The initial state is determined by the
> start-powered-off QOM property.
>
> Signed-off-by: Rob Herring <rob.herring@linaro.org>
> ---
>  target-arm/cpu-qom.h | 1 +
>  target-arm/cpu.c     | 7 ++++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index edc7f26..8ccb227 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -96,6 +96,7 @@ typedef struct ARMCPU {
>
>      /* Should CPU start in PSCI powered-off state? */
>      bool start_powered_off;

This could use a comment:
      /* CPU currently in PSCI powered-off state */

Also this is variable CPU state, so it will need to
be migrated, which means you need to add a
VMSTATE_BOOL(powered_off, ARMCPU),
to vmstate_arm_cpu in machine.c and bump the
version numbers.

> +    bool powered_off;
>
>      /* [QEMU_]KVM_ARM_TARGET_* constant for this CPU, or
>       * QEMU_KVM_ARM_TARGET_NONE if the kernel doesn't support this CPU type.
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index c0ddc3e..03c025b 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -39,7 +39,9 @@ static void arm_cpu_set_pc(CPUState *cs, vaddr value)
>
>  static bool arm_cpu_has_work(CPUState *cs)
>  {
> -    return cs->interrupt_request &
> +    ARMCPU *cpu = ARM_CPU(cs);
> +
> +    return !cpu->powered_off && cs->interrupt_request &
>          (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD | CPU_INTERRUPT_EXITTB);
>  }
>
> @@ -90,6 +92,9 @@ static void arm_cpu_reset(CPUState *s)
>      env->vfp.xregs[ARM_VFP_MVFR1] = cpu->mvfr1;
>      env->vfp.xregs[ARM_VFP_MVFR2] = cpu->mvfr2;
>
> +    cpu->powered_off = cpu->start_powered_off;
> +    s->halted = cpu->start_powered_off;
> +
>      if (arm_feature(env, ARM_FEATURE_IWMMXT)) {
>          env->iwmmxt.cregs[ARM_IWMMXT_wCID] = 0x69051000 | 'Q';
>      }
> --
> 1.9.1

The rest of this looks correct.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/7] target-arm: add hvc and smc exception emulation handling infrastructure

[Peter Maydell]

On 5 May 2014 17:00, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@linaro.org>
>
> Add the infrastructure to handle and emulate hvc and smc exceptions.
> This will enable emulation of things such as PSCI calls. This commit
> does not change the behavior and will exit with unknown exception.
>
> Signed-off-by: Rob Herring <rob.herring@linaro.org>
> ---
>  target-arm/cpu-qom.h       |  3 +++
>  target-arm/cpu.h           |  2 ++
>  target-arm/helper-a64.c    | 11 +++++++++++
>  target-arm/helper.c        | 33 +++++++++++++++++++++++++++++++++
>  target-arm/internals.h     | 15 +++++++++++++++
>  target-arm/translate-a64.c | 13 ++++++++++---
>  target-arm/translate.c     | 24 +++++++++++++++++-------
>  7 files changed, 91 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index 8ccb227..88aaf6a 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -185,6 +185,9 @@ extern const struct VMStateDescription vmstate_arm_cpu;
>  void register_cp_regs_for_features(ARMCPU *cpu);
>  void init_cpreg_list(ARMCPU *cpu);
>
> +bool arm_cpu_do_hvc(CPUState *cs);
> +bool arm_cpu_do_smc(CPUState *cs);
> +
>  void arm_cpu_do_interrupt(CPUState *cpu);
>  void arm_v7m_cpu_do_interrupt(CPUState *cpu);
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index c83f249..905ba02 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -51,6 +51,8 @@
>  #define EXCP_EXCEPTION_EXIT  8   /* Return from v7M exception.  */
>  #define EXCP_KERNEL_TRAP     9   /* Jumped to kernel code page.  */
>  #define EXCP_STREX          10
> +#define EXCP_HVC            11
> +#define EXCP_SMC            12
>
>  #define ARMV7M_EXCP_RESET   1
>  #define ARMV7M_EXCP_NMI     2
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index 84411b4..d2c1097 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -485,6 +485,17 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>      case EXCP_FIQ:
>          addr += 0x100;
>          break;
> +    case EXCP_HVC:
> +        if (arm_cpu_do_hvc(cs)) {
> +            return;
> +        }
> +        cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
> +        return;
> +    case EXCP_SMC:
> +        if (arm_cpu_do_smc(cs)) {
> +            return;
> +        }
> +        /* Fall-though */

We can't cpu_abort() just because the guest hands us an HVC
or SMC that we don't happen to handle in QEMU. We should
just qemu_log_mask(LOG_GUEST_ERROR, ...) and then do the
architecturally mandated behaviour for SMC or HVC instructions
on a CPU which doesn't implement EL2/EL3, ie treat as
UnallocatedEncoding(). (Don't forget to fix up exception.syndrome
in this case, since it should be syn_uncategorized(), not
the SMC/HVC value.)

>      default:
>          cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);

(this cpu_abort() is OK because it's really a "can never
reach here" assertion.)

>      }
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3be917c..b5b4a17 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3253,6 +3253,28 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>      env->thumb = addr & 1;
>  }
>
> +bool arm_cpu_do_hvc(CPUState *cs)
> +{
> +    bool ret;
> +
> +    ret = arm_handle_psci(cs);
> +    if (ret) {
> +        return ret;
> +    }
> +    return false;
> +}
> +
> +bool arm_cpu_do_smc(CPUState *cs)
> +{
> +    bool ret;
> +
> +    ret = arm_handle_psci(cs);
> +    if (ret) {
> +        return ret;
> +    }
> +    return false;
> +}

This hunk means the series doesn't compile after this
patch is applied. I think in this patch these two functions
should both just "return false;" indicating that no HVC
or SMC calls have any special handling by QEMU. Then the
patch which adds psci.c can also add the calls.

> +
>  /* Handle a CPU exception.  */
>  void arm_cpu_do_interrupt(CPUState *cs)
>  {
> @@ -3355,6 +3377,17 @@ void arm_cpu_do_interrupt(CPUState *cs)
>          mask = CPSR_A | CPSR_I | CPSR_F;
>          offset = 4;
>          break;
> +    case EXCP_HVC:
> +        if (arm_cpu_do_hvc(cs)) {
> +            return;
> +        }
> +        cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
> +        return;
> +    case EXCP_SMC:
> +        if (arm_cpu_do_smc(cs)) {
> +            return;
> +        }
> +        /* Fall-though */

Same remarks about cpu_abort() apply here.

>      default:
>          cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
>          return; /* Never happens.  Keep compiler happy.  */
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index d63a975..c71eabb 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -184,6 +184,21 @@ static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
>          | (is_thumb ? 0 : ARM_EL_IL);
>  }
>
> +static inline uint32_t syn_aa64_hvc(uint32_t imm16)
> +{
> +    return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> +}
> +
> +static inline uint32_t syn_aa32_hvc(uint32_t imm16)
> +{
> +    return (EC_AA32_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> +}
> +
> +static inline uint32_t syn_aa64_smc(uint32_t imm16)
> +{
> +    return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> +}
> +

You want a syn_aa32_smc() as well [note that it doesn't
take an immediate].

>  static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
>  {
>      return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index b62db4d..fa49ed8 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1449,11 +1449,18 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>          /* SVC, HVC, SMC; since we don't support the Virtualization
>           * or TrustZone extensions these all UNDEF except SVC.
>           */
> -        if (op2_ll != 1) {
> -            unallocated_encoding(s);
> +        switch (op2_ll) {
> +        case 1:
> +            gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
> +            break;
> +        case 2:
> +            gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_smc(imm16));

This should be syn_aa64_hvc()...

> +            break;
> +        case 3:
> +            gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_hvc(imm16));

...and this should be syn_aa64_smc().

>              break;
>          }
> -        gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
> +        unallocated_encoding(s);
>          break;
>      case 1:
>          if (op2_ll != 0) {
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index a4d920b..13ece7f 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7727,9 +7727,14 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
>          case 7:
>          {
>              int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4);
> -            /* SMC instruction (op1 == 3)
> -               and undefined instructions (op1 == 0 || op1 == 2)
> -               will trap */
> +            /* HVC and SMC instructions */
> +            if (op1 == 2) {
> +                gen_exception_insn(s, 0, EXCP_HVC, imm16);
> +                break;
> +            } else if (op1 == 3) {
> +                gen_exception_insn(s, 0, EXCP_SMC, 0);
> +                break;

The fourth argument to gen_exception_insn() should be the
syndrome register value, so there should be calls to
syn_aa32_hvc()/syn_aa32_smc() here.

> +            }
>              if (op1 != 1) {
>                  goto illegal_op;
>              }
> @@ -9555,10 +9560,15 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>                      goto illegal_op;
>
>                  if (insn & (1 << 26)) {
> -                    /* Secure monitor call (v6Z) */
> -                    qemu_log_mask(LOG_UNIMP,
> -                                  "arm: unimplemented secure monitor call\n");
> -                    goto illegal_op; /* not implemented.  */
> +                    if (!(insn & (1 << 20))) {
> +                        /* Hypervisor call (v7) */
> +                        uint32_t imm16 = extract32(insn, 16, 4);
> +                        imm16 |= extract32(insn, 0, 12) << 4;

This is the wrong way round, I think: imm16 is imm4:imm12, not
imm12:imm4.

> +                        gen_exception_insn(s, 0, EXCP_HVC, imm16);
> +                    } else {
> +                        /* Secure monitor call (v6+) */
> +                        gen_exception_insn(s, 0, EXCP_SMC, 0);

Again, missing calls to syn_aa32_hvc()/syn_aa32_smc().

> +                    }
>                  } else {
>                      op = (insn >> 20) & 7;
>                      switch (op) {
> --
> 1.9.1
>


thanks
-- PMM

Re: [Qemu-devel] [PATCH 6/7] arm/virt: enable PSCI emulation support for system emulation

[Peter Maydell]

On 5 May 2014 17:00, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@linaro.org>
>
> Now that we have PSCI emulation, enable it for the virt platform.
> This simplifies the virt machine a bit now that PSCI and SMP no longer
> need to be KVM only features.
>
> Signed-off-by: Rob Herring <rob.herring@linaro.org>
> ---
>
> Note: This will need to be rebased as comments on KVM PSCI 0.2 support
> are addressed. This patch effectively includes my comments.
>
>  hw/arm/virt.c | 42 ++++++++++++++++++------------------------
>  1 file changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 9f28c12..d8ab88d 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -181,21 +181,22 @@ static void create_fdt(VirtBoardInfo *vbi)
>                                  "clk24mhz");
>      qemu_fdt_setprop_cell(fdt, "/apb-pclk", "phandle", vbi->clock_phandle);
>
> -    /* No PSCI for TCG yet */
> -    if (kvm_enabled()) {
> -        qemu_fdt_add_subnode(fdt, "/psci");
> -        if (kvm_check_extension(kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
> -            qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci-0.2");
> -        } else {
> -            qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci");
> -            qemu_fdt_setprop_cell(fdt, "/psci", "cpu_suspend",
> -                                  PSCI_FN_CPU_SUSPEND);
> -            qemu_fdt_setprop_cell(fdt, "/psci", "cpu_off", PSCI_FN_CPU_OFF);
> -            qemu_fdt_setprop_cell(fdt, "/psci", "cpu_on", PSCI_FN_CPU_ON);
> -            qemu_fdt_setprop_cell(fdt, "/psci", "migrate", PSCI_FN_MIGRATE);
> -        }
> -        qemu_fdt_setprop_string(fdt, "/psci", "method", "hvc");
> +    qemu_fdt_add_subnode(fdt, "/psci");
> +    qemu_fdt_setprop_string(fdt, "/psci", "method", "hvc");
> +    if (kvm_enabled() && !kvm_check_extension(kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
> +        qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci");
> +    } else {
> +        const char compat[] = "arm,psci-0.2\0arm,psci";
> +        qemu_fdt_setprop(fdt, "/psci", "compatible", compat, sizeof(compat));
>      }

My suggestion to Pranav was that we abstract away the "which PSCI
version?" decision into a field in ARMCPU, in which case we can
just have TCG always set it to 0.2. So some of this logic
will get a little simpler on rebase.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 5/7] target-arm: add emulation of PSCI calls for system emulation

[Peter Maydell]

On 5 May 2014 17:00, Rob Herring <robherring2@gmail.com> 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 "psci-method" QOM property on the cpus to SMC or HVC
> emulation and having PSCI binding in their dtb.
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -98,6 +98,11 @@ typedef struct ARMCPU {
>      bool start_powered_off;
>      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.
>       */
> @@ -185,6 +190,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 2d18a20..eb21a52 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -1009,6 +1009,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()
>  };

Andreas, does this seem reasonable, or is there a nicer way
to do an "enumeration" like QOM property I'm unaware of?
(in this case the values are "none"/"smc"/"hvc").

> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b5b4a17..637c46a 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3255,23 +3255,23 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>
>  bool arm_cpu_do_hvc(CPUState *cs)
>  {
> -    bool ret;
> +    ARMCPU *cpu = ARM_CPU(cs);
>
> -    ret = arm_handle_psci(cs);
> -    if (ret) {
> -        return ret;
> +    if (cpu->psci_method == QEMU_PSCI_METHOD_HVC) {
> +        return arm_handle_psci(cs);
>      }
> +
>      return false;
>  }
>
>  bool arm_cpu_do_smc(CPUState *cs)
>  {
> -    bool ret;
> +    ARMCPU *cpu = ARM_CPU(cs);
>
> -    ret = arm_handle_psci(cs);
> -    if (ret) {
> -        return ret;
> +    if (cpu->psci_method == QEMU_PSCI_METHOD_SMC) {
> +        return arm_handle_psci(cs);
>      }
> +
>      return false;
>  }
>
> diff --git a/target-arm/kvm-consts.h b/target-arm/kvm-consts.h
> index 5cf93ab..d0a89c7 100644
> --- a/target-arm/kvm-consts.h
> +++ b/target-arm/kvm-consts.h
> @@ -91,6 +91,12 @@ MISMATCH_CHECK(QEMU_PSCI_0_2_FN64_MIGRATE, PSCI_0_2_FN64_MIGRATE)
>  MISMATCH_CHECK(QEMU_PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU, \
>                 PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU)
>
> +enum {
> +    QEMU_PSCI_METHOD_DISABLED = 0,
> +    QEMU_PSCI_METHOD_SMC = 1,
> +    QEMU_PSCI_METHOD_HVC = 2,
> +};
> +
>  /* Note that KVM uses overlapping values for AArch32 and AArch64
>   * target CPU numbers. AArch32 targets:
>   */
> diff --git a/target-arm/psci.c b/target-arm/psci.c
> new file mode 100644
> index 0000000..5c66236
> --- /dev/null
> +++ b/target-arm/psci.c
> @@ -0,0 +1,152 @@
> +/*
> + * Copyright (C) 2014 - Linaro
> + * Author: Rob Herring <rob.herring@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>
> +#include <linux/psci.h>
> +
> +#if !defined(CONFIG_USER_ONLY)

Rather than #ifdeffing the whole file out, I suggest
putting psci.o into an "obj-$(CONFIG_SOFTMMU)" line in
target-arm/Makefile.objs (the callsites are already inside
!defined(CONFIG_USER_ONLY) I think.)

> +
> +bool arm_handle_psci(CPUState *cs)
> +{

A brief comment here referencing the PSCI standard and
the "SMC calling conventions" doc would be nice, so readers
have an idea of where to look to find out what we're
implementing.

> +    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++) {
> +        param[i] = is_a64(env) ? env->xregs[i] : env->regs[i];

This is implicitly assuming that it's always right to
zero-extend arguments in the case of a call from AArch32;
as it happens that's correct for all the current PSCI
functions, but I think it could use a comment.

> +    }
> +
> +    if ((param[0] & PSCI_0_2_64BIT) && !is_a64(env)) {
> +        ret = PSCI_RET_INVALID_PARAMS;
> +        goto err;
> +    }
> +
> +    switch (param[0]) {
> +    case PSCI_0_2_FN_PSCI_VERSION:
> +        ret = PSCI_VERSION_MAJOR(0) | PSCI_VERSION_MINOR(2);
> +        break;
> +    case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> +        ret = PSCI_0_2_TOS_MP;    /* No trusted OS */
> +        break;
> +    case PSCI_0_2_FN_AFFINITY_INFO:
> +    case PSCI_0_2_FN64_AFFINITY_INFO:
> +        mpidr = param[1];
> +
> +        switch (param[2]) {
> +        case 0:
> +            cs = qemu_get_cpu(mpidr & 0xff);
> +            if (!cs) {
> +                ret = PSCI_RET_INVALID_PARAMS;
> +                break;
> +            }
> +            cpu = ARM_CPU(cs);
> +            ret = cpu->powered_off ? 1 : 0;
> +            break;
> +        default:
> +            /* Everything above affinity level 0 is always on. */
> +            ret = 0;
> +        }
> +        break;
> +    case PSCI_0_2_FN_SYSTEM_RESET:
> +        qemu_system_reset_request();
> +        break;
> +    case PSCI_0_2_FN_SYSTEM_OFF:
> +        qemu_system_powerdown_request();
> +        break;
> +    case QEMU_PSCI_FN_CPU_ON:
> +    case PSCI_0_2_FN_CPU_ON:
> +    case PSCI_0_2_FN64_CPU_ON:
> +        mpidr = param[1];
> +        entry = param[2];
> +        context_id = param[3];
> +
> +        /* change to the cpu we are powering up */
> +        cs = qemu_get_cpu(mpidr & 0xff);

I think it would be clearer to use a different variable
for the other CPU, rather than changing cs.

> +        if (!cs) {
> +            ret = PSCI_RET_INVALID_PARAMS;
> +            break;
> +        }
> +        cpu = ARM_CPU(cs);
> +
> +        if (!cpu->powered_off) {
> +            ret = PSCI_RET_ALREADY_ON;
> +            break;
> +        }
> +
> +        /* Initialize the cpu we are turning on */
> +        cpu_reset(cs);
> +        arm_cpu_set_pc(cs, entry);
> +        cpu->powered_off = false;
> +        cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> +
> +        /* Set the context_id in r0/x0 */
> +        if (is_a64(env)) {
> +            cpu->env.xregs[0] = context_id;
> +        } else {
> +            cpu->env.regs[0] = context_id;
> +        }

If the calling CPU is in AArch32 then we're going
to restart the target CPU in AArch64 but with R0
set rather than X0. That doesn't seem right...

> +
> +        ret = 0;
> +        break;
> +    case QEMU_PSCI_FN_CPU_OFF:
> +    case PSCI_0_2_FN_CPU_OFF:
> +        cpu->powered_off = true;
> +        cs->exit_request = 1;

I need to check up on whether setting exit_request here
is right, but I don't have time just this instant. More
later :-)

> +        cs->halted = 1;
> +
> +        /* CPU_OFF should never return, but if it does return an error */
> +        ret = PSCI_RET_DENIED;
> +        break;
> +    case QEMU_PSCI_FN_CPU_SUSPEND:
> +    case PSCI_0_2_FN_CPU_SUSPEND:
> +    case PSCI_0_2_FN64_CPU_SUSPEND:
> +        /* Affinity levels are not supported in QEMU */
> +        if (param[1] & 0xfffe0000) {
> +            ret = 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;
> +        break;
> +    case QEMU_PSCI_FN_MIGRATE:
> +    case PSCI_0_2_FN_MIGRATE:
> +        ret = PSCI_RET_NOT_SUPPORTED;
> +        break;
> +    default:
> +        return false;
> +    }
> +
> +err:
> +    if (is_a64(env)) {
> +        env->xregs[0] = ret;
> +    } else {
> +        env->regs[0] = ret;
> +    }
> +    return true;
> +}
> +#endif
> --
> 1.9.1

thanks
-- PMM

Re: [Qemu-devel] [PATCH 6/7] arm/virt: enable PSCI emulation support for system emulation

[Rob Herring]

On Wed, May 14, 2014 at 12:51 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 5 May 2014 17:00, Rob Herring <robherring2@gmail.com> wrote:
>> From: Rob Herring <rob.herring@linaro.org>
>>
>> Now that we have PSCI emulation, enable it for the virt platform.
>> This simplifies the virt machine a bit now that PSCI and SMP no longer
>> need to be KVM only features.

[...]

>> +    qemu_fdt_add_subnode(fdt, "/psci");
>> +    qemu_fdt_setprop_string(fdt, "/psci", "method", "hvc");
>> +    if (kvm_enabled() && !kvm_check_extension(kvm_state, KVM_CAP_ARM_PSCI_0_2)) {
>> +        qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci");
>> +    } else {
>> +        const char compat[] = "arm,psci-0.2\0arm,psci";
>> +        qemu_fdt_setprop(fdt, "/psci", "compatible", compat, sizeof(compat));
>>      }
>
> My suggestion to Pranav was that we abstract away the "which PSCI
> version?" decision into a field in ARMCPU, in which case we can
> just have TCG always set it to 0.2. So some of this logic
> will get a little simpler on rebase.

You can't. You have to support both because you don't know what the
kernel supports. An old kernel will only support arm,psci.

Rob

Re: [Qemu-devel] [PATCH 3/7] target-arm: add hvc and smc exception emulation handling infrastructure

[Rob Herring]

On Wed, May 14, 2014 at 12:44 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 5 May 2014 17:00, Rob Herring <robherring2@gmail.com> wrote:
>> From: Rob Herring <rob.herring@linaro.org>
>>
>> Add the infrastructure to handle and emulate hvc and smc exceptions.
>> This will enable emulation of things such as PSCI calls. This commit
>> does not change the behavior and will exit with unknown exception.

[...]

>> +    case EXCP_HVC:
>> +        if (arm_cpu_do_hvc(cs)) {
>> +            return;
>> +        }
>> +        cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
>> +        return;
>> +    case EXCP_SMC:
>> +        if (arm_cpu_do_smc(cs)) {
>> +            return;
>> +        }
>> +        /* Fall-though */
>
> We can't cpu_abort() just because the guest hands us an HVC
> or SMC that we don't happen to handle in QEMU. We should
> just qemu_log_mask(LOG_GUEST_ERROR, ...) and then do the
> architecturally mandated behaviour for SMC or HVC instructions
> on a CPU which doesn't implement EL2/EL3, ie treat as
> UnallocatedEncoding(). (Don't forget to fix up exception.syndrome
> in this case, since it should be syn_uncategorized(), not
> the SMC/HVC value.)

So I think this and shifting setting ESR/FAR after the switch should
be sufficient:

    case EXCP_SMC:
        if (arm_cpu_do_smc(cs)) {
            return;
        }
        env->exception.syndrome = syn_uncategorized();
        if (is_a64(env)) {
            env->pc -= 4;
        } else {
            env->regs[15] -= env->thumb ? 2 : 4;
        }
        break;

I'm not completely sure the PC adjustment is correct.

>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 3be917c..b5b4a17 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3253,6 +3253,28 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>>      env->thumb = addr & 1;
>>  }
>>
>> +bool arm_cpu_do_hvc(CPUState *cs)
>> +{
>> +    bool ret;
>> +
>> +    ret = arm_handle_psci(cs);
>> +    if (ret) {
>> +        return ret;
>> +    }
>> +    return false;
>> +}
>> +
>> +bool arm_cpu_do_smc(CPUState *cs)
>> +{
>> +    bool ret;
>> +
>> +    ret = arm_handle_psci(cs);
>> +    if (ret) {
>> +        return ret;
>> +    }
>> +    return false;
>> +}
>
> This hunk means the series doesn't compile after this
> patch is applied. I think in this patch these two functions
> should both just "return false;" indicating that no HVC
> or SMC calls have any special handling by QEMU. Then the
> patch which adds psci.c can also add the calls.

Ah yes, I had it like that and forgot to go back and split it up in
the process of rebasing.


>> +static inline uint32_t syn_aa64_hvc(uint32_t imm16)
>> +{
>> +    return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
>> +}
>> +
>> +static inline uint32_t syn_aa32_hvc(uint32_t imm16)
>> +{
>> +    return (EC_AA32_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
>> +}
>> +
>> +static inline uint32_t syn_aa64_smc(uint32_t imm16)
>> +{
>> +    return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
>> +}
>> +
>
> You want a syn_aa32_smc() as well [note that it doesn't
> take an immediate].

I also noticed I need to set ARM_EL_IL based on thumb or not on the
aa32 variants.


>> +                    if (!(insn & (1 << 20))) {
>> +                        /* Hypervisor call (v7) */
>> +                        uint32_t imm16 = extract32(insn, 16, 4);
>> +                        imm16 |= extract32(insn, 0, 12) << 4;
>
> This is the wrong way round, I think: imm16 is imm4:imm12, not
> imm12:imm4.

You're right. ARM and Thumb are reversed.

Rob

Re: [Qemu-devel] [PATCH 6/7] arm/virt: enable PSCI emulation support for system emulation

[Peter Maydell]

On 14 May 2014 20:15, Rob Herring <rob.herring@linaro.org> wrote:
> On Wed, May 14, 2014 at 12:51 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> My suggestion to Pranav was that we abstract away the "which PSCI
>> version?" decision into a field in ARMCPU, in which case we can
>> just have TCG always set it to 0.2. So some of this logic
>> will get a little simpler on rebase.
>
> You can't. You have to support both because you don't know what the
> kernel supports. An old kernel will only support arm,psci.

An old host kernel, or an old guest kernel? The former is fine,
because the KVM CPU init code will just ask for the KVM
capability and fill in the ARMCPU field appropriately.
For the latter, how are you supposed to determine what the
guest kernel can support?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 6/7] arm/virt: enable PSCI emulation support for system emulation

[Rob Herring]

On Wed, May 14, 2014 at 4:25 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 14 May 2014 20:15, Rob Herring <rob.herring@linaro.org> wrote:
>> On Wed, May 14, 2014 at 12:51 PM, Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>> My suggestion to Pranav was that we abstract away the "which PSCI
>>> version?" decision into a field in ARMCPU, in which case we can
>>> just have TCG always set it to 0.2. So some of this logic
>>> will get a little simpler on rebase.
>>
>> You can't. You have to support both because you don't know what the
>> kernel supports. An old kernel will only support arm,psci.
>
> An old host kernel, or an old guest kernel? The former is fine,
> because the KVM CPU init code will just ask for the KVM
> capability and fill in the ARMCPU field appropriately.
> For the latter, how are you supposed to determine what the
> guest kernel can support?

Guest kernels and this was exactly my point that you can't determine
it. The virt dtb is for the guest kernel and must be either 0.1 PSCI
only or both 0.1 and 0.2. I think I misread what you meant. Reading
the other thread, as long as you just mean changing the if statement
like this, then we are in agreement:

    if (psci version is 0.1) {
        qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci");
    } else {
        const char compat[] = "arm,psci-0.2\0arm,psci";
        qemu_fdt_setprop(fdt, "/psci", "compatible", compat, sizeof(compat));
    }

Rob

Re: [Qemu-devel] [PATCH 5/7] target-arm: add emulation of PSCI calls for system emulation

[Rob Herring]

On Wed, May 14, 2014 at 1:12 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 May 2014 17:00, Rob Herring <robherring2@gmail.com> 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 "psci-method" QOM property on the cpus to SMC or HVC
>> emulation and having PSCI binding in their dtb.

[...]

>> +        /* Initialize the cpu we are turning on */
>> +        cpu_reset(cs);
>> +        arm_cpu_set_pc(cs, entry);
>> +        cpu->powered_off = false;
>> +        cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>> +
>> +        /* Set the context_id in r0/x0 */
>> +        if (is_a64(env)) {
>> +            cpu->env.xregs[0] = context_id;
>> +        } else {
>> +            cpu->env.regs[0] = context_id;
>> +        }
>
> If the calling CPU is in AArch32 then we're going
> to restart the target CPU in AArch64 but with R0
> set rather than X0. That doesn't seem right...

Probably just one on many issues to support A32 EL1... Since the core
is being reset and register state is undefined, I can just do:

cpu->env.xregs[0] = cpu->env.regs[0] = context_id;

EL2/3 support will then further complicate things (or just remove all
this code).

>> +
>> +        ret = 0;
>> +        break;
>> +    case QEMU_PSCI_FN_CPU_OFF:
>> +    case PSCI_0_2_FN_CPU_OFF:
>> +        cpu->powered_off = true;
>> +        cs->exit_request = 1;
>
> I need to check up on whether setting exit_request here
> is right, but I don't have time just this instant. More
> later :-)

IIRC, things did not work without it.

Rob

Re: [Qemu-devel] [PATCH 6/7] arm/virt: enable PSCI emulation support for system emulation

[Peter Maydell]

On 14 May 2014 23:58, Rob Herring <rob.herring@linaro.org> wrote:
> On Wed, May 14, 2014 at 4:25 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> An old host kernel, or an old guest kernel? The former is fine,
>> because the KVM CPU init code will just ask for the KVM
>> capability and fill in the ARMCPU field appropriately.
>> For the latter, how are you supposed to determine what the
>> guest kernel can support?
>
> Guest kernels and this was exactly my point that you can't determine
> it. The virt dtb is for the guest kernel and must be either 0.1 PSCI
> only or both 0.1 and 0.2. I think I misread what you meant. Reading
> the other thread, as long as you just mean changing the if statement
> like this, then we are in agreement:
>
>     if (psci version is 0.1) {
>         qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci");
>     } else {
>         const char compat[] = "arm,psci-0.2\0arm,psci";
>         qemu_fdt_setprop(fdt, "/psci", "compatible", compat, sizeof(compat));
>     }

Yes, that's all I meant; sorry for the confusion.

I hadn't noticed that we announce and support both the 0.1
and 0.2 interfaces in the else {} branch, which is probably
why my phrasing was confusing.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/7] target-arm: add hvc and smc exception emulation handling infrastructure

[Peter Maydell]

On 14 May 2014 21:55, Rob Herring <rob.herring@linaro.org> wrote:
> On Wed, May 14, 2014 at 12:44 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> We can't cpu_abort() just because the guest hands us an HVC
>> or SMC that we don't happen to handle in QEMU. We should
>> just qemu_log_mask(LOG_GUEST_ERROR, ...) and then do the
>> architecturally mandated behaviour for SMC or HVC instructions
>> on a CPU which doesn't implement EL2/EL3, ie treat as
>> UnallocatedEncoding(). (Don't forget to fix up exception.syndrome
>> in this case, since it should be syn_uncategorized(), not
>> the SMC/HVC value.)
>
> So I think this and shifting setting ESR/FAR after the switch should
> be sufficient:
>
>     case EXCP_SMC:
>         if (arm_cpu_do_smc(cs)) {
>             return;
>         }
>         env->exception.syndrome = syn_uncategorized();
>         if (is_a64(env)) {
>             env->pc -= 4;
>         } else {
>             env->regs[15] -= env->thumb ? 2 : 4;
>         }
>         break;
>
> I'm not completely sure the PC adjustment is correct.

I don't think you want that env->thumb conditional:
the SMC instruction is 4 bytes in both the A32 and
T32 encodings, so we always need to rewind by 4 bytes.

>>> +static inline uint32_t syn_aa64_hvc(uint32_t imm16)
>>> +{
>>> +    return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
>>> +}
>>> +
>>> +static inline uint32_t syn_aa32_hvc(uint32_t imm16)
>>> +{
>>> +    return (EC_AA32_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
>>> +}
>>> +
>>> +static inline uint32_t syn_aa64_smc(uint32_t imm16)
>>> +{
>>> +    return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
>>> +}
>>> +
>>
>> You want a syn_aa32_smc() as well [note that it doesn't
>> take an immediate].
>
> I also noticed I need to set ARM_EL_IL based on thumb or not on the
> aa32 variants.

No, for HVC and SMC the Thumb instructions are 32 bit so
ARM_EL_IL should be set. (Breakpoint and SVC are different because
the T32 BKPT and SVC insn encodings are only 16 bits long.
In retrospect calling the syn_aa32_* parameter "is_thumb" rather
than "is_16bit" was perhaps misleading.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH 7/7] arm/highbank: enable PSCI emulation support

[Peter Crosthwaite]

On Mon, May 5, 2014 at 4:00 PM, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@linaro.org>
>
> Enable PSCI enulation on highbank and midway platforms.

"emulation"

>
> Note that this requires fixing the PSCI function IDs in the DTB to match
> what QEMU is using. This should get fixed.
>
> Signed-off-by: Rob Herring <rob.herring@linaro.org>
> ---
>  hw/arm/highbank.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 46b9f1e..092df1f 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -242,6 +242,14 @@ static void calxeda_init(QEMUMachineInitArgs *args, enum cxmachines machine)
>          cpuobj = object_new(object_class_get_name(oc));
>          cpu = ARM_CPU(cpuobj);
>
> +        object_property_set_int(cpuobj, QEMU_PSCI_METHOD_SMC, "psci-method",
> +                                NULL);
> +
> +        /* Secondary CPUs start in PSCI powered-down state */
> +        if (n > 0) {
> +            object_property_set_bool(cpuobj, true, "start-powered-off", NULL);

Use &error_abort in cases where things should never possibly fail (and
i think this is the case here). The semantics of a NULL errp is "try
the function and if it fails I don't care".

Regards,
Peter

> +        }
> +
>          if (object_property_find(cpuobj, "reset-cbar", NULL)) {
>              object_property_set_int(cpuobj, MPCORE_PERIPHBASE,
>                                      "reset-cbar", &error_abort);
> --
> 1.9.1
>
>

Re: [Qemu-devel] [PATCH 2/7] target-arm: add powered off cpu state

[Peter Crosthwaite]

On Mon, May 5, 2014 at 4:00 PM, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@linaro.org>
>
> Add tracking of cpu power state in order to support powering off of
> cores in system emuluation. The initial state is determined by the

"emulation"

Regards,
Peter