[Qemu-devel] [PATCH v3 0/6] ARM: add PSCI 0.2 support in TCG mode

[Qemu-devel] [PATCH v3 0/6] ARM: add PSCI 0.2 support in TCG mode

[Ard Biesheuvel]

This series adds PSCI support to ARM and AArch64 system emulation when running
in TCG mode. As PSCI calls can be made using either hypervisor call (HVC) or
secure monitor call (SMC) instructions, support is added for handling those
in patch #3 before patch #5 adds the actual PSCI dispatch logic. Patch #6
enables PSCI for the mach-virt platform.

Currently, booting multiple cores under TCG is unstable, so the restriction
to 1 cpu in TCG mode is retained for now. However, PSCI reset and poweroff are
supported.

Changes since v2:
- added path #4 to introduce QEMU counterparts of the kernel PSCI constants we
  refer to in the PSCI emulation, this is needed so QEMU can be built in
  environments that don't supply the PSCI header file.

Changes since v1:
- processed first round of review, that was already given when this series was
  sent out by Rob himself back in May

Ard Biesheuvel (1):
  target-arm: add missing PSCI constants needed for PSCI emulation

Rob Herring (5):
  target-arm: add powered off cpu state
  target-arm: do not set do_interrupt handler for AArch64 user mode
  target-arm: add hvc and smc exception emulation handling
    infrastructure
  target-arm: add emulation of PSCI calls for system emulation
  arm/virt: enable PSCI emulation support for system emulation

 hw/arm/virt.c              |  70 +++++++++----------
 target-arm/Makefile.objs   |   1 +
 target-arm/cpu-qom.h       |  11 +++
 target-arm/cpu.c           |  17 +++--
 target-arm/cpu.h           |   8 +++
 target-arm/cpu64.c         |   2 +
 target-arm/helper-a64.c    |  19 +++++
 target-arm/helper.c        |  35 ++++++++++
 target-arm/internals.h     |  20 ++++++
 target-arm/kvm-consts.h    |  40 +++++++++++
 target-arm/machine.c       |   5 +-
 target-arm/psci.c          | 171 +++++++++++++++++++++++++++++++++++++++++++++
 target-arm/translate-a64.c |  26 ++++---
 target-arm/translate.c     |  24 +++++--
 14 files changed, 391 insertions(+), 58 deletions(-)
 create mode 100644 target-arm/psci.c

-- 
1.8.3.2

[Qemu-devel] [PATCH v3 1/6] target-arm: add powered off cpu state

[Ard Biesheuvel]

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>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 target-arm/cpu-qom.h | 2 ++
 target-arm/cpu.c     | 7 ++++++-
 target-arm/machine.c | 5 +++--
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 07f3c9e86639..eae0a7b9c908 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -98,6 +98,8 @@ typedef struct ARMCPU {
 
     /* Should CPU start in PSCI powered-off state? */
     bool start_powered_off;
+    /* CPU currently in PSCI powered-off state */
+    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 8199f32e3267..b4c06c17cf87 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -40,7 +40,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);
 }
 
@@ -91,6 +93,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';
     }
diff --git a/target-arm/machine.c b/target-arm/machine.c
index 3bcc7cc833e0..63329ebd551f 100644
--- a/target-arm/machine.c
+++ b/target-arm/machine.c
@@ -218,8 +218,8 @@ static int cpu_post_load(void *opaque, int version_id)
 
 const VMStateDescription vmstate_arm_cpu = {
     .name = "cpu",
-    .version_id = 20,
-    .minimum_version_id = 20,
+    .version_id = 21,
+    .minimum_version_id = 21,
     .pre_save = cpu_pre_save,
     .post_load = cpu_post_load,
     .fields = (VMStateField[]) {
@@ -259,6 +259,7 @@ const VMStateDescription vmstate_arm_cpu = {
         VMSTATE_UINT64(env.exception.vaddress, ARMCPU),
         VMSTATE_TIMER(gt_timer[GTIMER_PHYS], ARMCPU),
         VMSTATE_TIMER(gt_timer[GTIMER_VIRT], ARMCPU),
+        VMSTATE_BOOL(powered_off, ARMCPU),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (VMStateSubsection[]) {
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v3 1/6] target-arm: add powered off cpu state

[Peter Maydell]

On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v3 2/6] target-arm: do not set do_interrupt handler for AArch64 user mode

[Ard Biesheuvel]

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. Remove the reference,
and '#ifndef USER_MODE_ONLY' the function itself as well, so that we can add
system mode only functionality to it.

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

diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index aa42803959be..9f88b9f4eea0 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -196,7 +196,9 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
 {
     CPUClass *cc = CPU_CLASS(oc);
 
+#if !defined(CONFIG_USER_ONLY)
     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 2e9ef64786ae..89b913ee9396 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -438,6 +438,8 @@ uint64_t HELPER(crc32c_64)(uint64_t acc, uint64_t val, uint32_t bytes)
     return crc32c(acc, buf, bytes) ^ 0xffffffff;
 }
 
+#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.8.3.2

Re: [Qemu-devel] [PATCH v3 2/6] target-arm: do not set do_interrupt handler for AArch64 user mode

[Peter Maydell]

On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. Remove the reference,
> and '#ifndef USER_MODE_ONLY' the function itself as well, so that we can add
> system mode only functionality to it.
>
> Signed-off-by: Rob Herring <rob.herring@linaro.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  target-arm/cpu64.c      | 2 ++
>  target-arm/helper-a64.c | 3 +++
>  2 files changed, 5 insertions(+)
>
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index aa42803959be..9f88b9f4eea0 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -196,7 +196,9 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
>  {
>      CPUClass *cc = CPU_CLASS(oc);
>
> +#if !defined(CONFIG_USER_ONLY)
>      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 2e9ef64786ae..89b913ee9396 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -438,6 +438,8 @@ uint64_t HELPER(crc32c_64)(uint64_t acc, uint64_t val, uint32_t bytes)
>      return crc32c(acc, buf, bytes) ^ 0xffffffff;
>  }
>
> +#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

For consistency we need to not set do_interrupt in
the arm_cpu_class_init() function too. (Then we'll
have it be NULL in all cases, since v7m already
guards its assignment with an ifdef.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 2/6] target-arm: do not set do_interrupt handler for AArch64 user mode

[Ard Biesheuvel]

On 9 September 2014 17:25, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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. Remove the reference,
>> and '#ifndef USER_MODE_ONLY' the function itself as well, so that we can add
>> system mode only functionality to it.
>>
>> Signed-off-by: Rob Herring <rob.herring@linaro.org>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  target-arm/cpu64.c      | 2 ++
>>  target-arm/helper-a64.c | 3 +++
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
>> index aa42803959be..9f88b9f4eea0 100644
>> --- a/target-arm/cpu64.c
>> +++ b/target-arm/cpu64.c
>> @@ -196,7 +196,9 @@ static void aarch64_cpu_class_init(ObjectClass *oc, void *data)
>>  {
>>      CPUClass *cc = CPU_CLASS(oc);
>>
>> +#if !defined(CONFIG_USER_ONLY)
>>      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 2e9ef64786ae..89b913ee9396 100644
>> --- a/target-arm/helper-a64.c
>> +++ b/target-arm/helper-a64.c
>> @@ -438,6 +438,8 @@ uint64_t HELPER(crc32c_64)(uint64_t acc, uint64_t val, uint32_t bytes)
>>      return crc32c(acc, buf, bytes) ^ 0xffffffff;
>>  }
>>
>> +#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
>
> For consistency we need to not set do_interrupt in
> the arm_cpu_class_init() function too. (Then we'll
> have it be NULL in all cases, since v7m already
> guards its assignment with an ifdef.)
>

Would you prefer a single patch that does both?
Or a followup patch?

Re: [Qemu-devel] [PATCH v3 2/6] target-arm: do not set do_interrupt handler for AArch64 user mode

[Peter Maydell]

On 9 September 2014 16:27, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 9 September 2014 17:25, Peter Maydell <peter.maydell@linaro.org> wrote:
>> For consistency we need to not set do_interrupt in
>> the arm_cpu_class_init() function too. (Then we'll
>> have it be NULL in all cases, since v7m already
>> guards its assignment with an ifdef.)
>>
>
> Would you prefer a single patch that does both?
> Or a followup patch?

I think a single patch doing both will be OK.
I don't care strongly either way.

-- PMM

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

[Ard Biesheuvel]

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>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 target-arm/cpu-qom.h       |  3 +++
 target-arm/cpu.h           |  2 ++
 target-arm/helper-a64.c    | 16 ++++++++++++++++
 target-arm/helper.c        | 23 +++++++++++++++++++++++
 target-arm/internals.h     | 20 ++++++++++++++++++++
 target-arm/translate-a64.c | 26 +++++++++++++++++---------
 target-arm/translate.c     | 24 +++++++++++++++++-------
 7 files changed, 98 insertions(+), 16 deletions(-)

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index eae0a7b9c908..104cc67e82d2 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -192,6 +192,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 51bedc826299..d235929f4c12 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 89b913ee9396..1f8072ab141b 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -485,6 +485,22 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     case EXCP_FIQ:
         addr += 0x100;
         break;
+    case EXCP_HVC:
+        if (arm_cpu_do_hvc(cs)) {
+            return;
+        }
+        /* Treat as unallocated encoding */
+        qemu_log_mask(LOG_GUEST_ERROR, "HVC not implemented on this CPU\n");
+        env->exception.syndrome = syn_uncategorized();
+        break;
+    case EXCP_SMC:
+        if (arm_cpu_do_smc(cs)) {
+            return;
+        }
+        /* Treat as unallocated encoding */
+        qemu_log_mask(LOG_GUEST_ERROR, "SMC not implemented on this CPU\n");
+        env->exception.syndrome = syn_uncategorized();
+        break;
     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 2b95f33872cb..440ee07a2ff8 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3497,6 +3497,16 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
     env->thumb = addr & 1;
 }
 
+bool arm_cpu_do_hvc(CPUState *cs)
+{
+    return false;
+}
+
+bool arm_cpu_do_smc(CPUState *cs)
+{
+    return false;
+}
+
 /* Handle a CPU exception.  */
 void arm_cpu_do_interrupt(CPUState *cs)
 {
@@ -3513,6 +3523,19 @@ void arm_cpu_do_interrupt(CPUState *cs)
 
     /* TODO: Vectored interrupt controller.  */
     switch (cs->exception_index) {
+    case EXCP_HVC:
+        if (arm_cpu_do_hvc(cs)) {
+            return;
+        }
+        qemu_log_mask(LOG_GUEST_ERROR, "HVC not implemented on this CPU\n");
+        goto hvc_unallocated;
+    case EXCP_SMC:
+        if (arm_cpu_do_smc(cs)) {
+            return;
+        }
+        qemu_log_mask(LOG_GUEST_ERROR, "SMC not implemented on this CPU\n");
+    hvc_unallocated:
+        /* Fall through -- treat as unallocated encoding */
     case EXCP_UDEF:
         new_mode = ARM_CPU_MODE_UND;
         addr = 0x04;
diff --git a/target-arm/internals.h b/target-arm/internals.h
index 53c2e3cf3e7e..caab98e6b508 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -210,6 +210,26 @@ 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_aa32_smc(void)
+{
+    return (EC_AA32_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL;
+}
+
 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 8e66b6c97282..65e35e3aaec0 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1473,20 +1473,28 @@ static void disas_exc(DisasContext *s, uint32_t insn)
 
     switch (opc) {
     case 0:
-        /* 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);
-            break;
-        }
         /* For SVC, HVC and SMC we advance the single-step state
          * machine before taking the exception. This is architecturally
          * mandated, to ensure that single-stepping a system call
          * instruction works properly.
          */
-        gen_ss_advance(s);
-        gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
+        switch (op2_ll) {
+        case 1:
+            gen_ss_advance(s);
+            gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16));
+            break;
+        case 2:
+            gen_ss_advance(s);
+            gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
+            break;
+        case 3:
+            gen_ss_advance(s);
+            gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_smc(imm16));
+            break;
+        default:
+            unallocated_encoding(s);
+            break;
+        }
         break;
     case 1:
         if (op2_ll != 0) {
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 2c0b1deaea81..a4545ed2bc40 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -7871,9 +7871,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, syn_aa32_hvc(imm16));
+                break;
+            } else if (op1 == 3) {
+                gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
+                break;
+            }
             if (op1 != 1) {
                 goto illegal_op;
             }
@@ -9709,10 +9714,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) << 12;
+                        imm16 |= extract32(insn, 0, 12);
+                        gen_exception_insn(s, 0, EXCP_HVC, syn_aa32_hvc(imm16));
+                    } else {
+                        /* Secure monitor call (v6+) */
+                        gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
+                    }
                 } else {
                     op = (insn >> 20) & 7;
                     switch (op) {
-- 
1.8.3.2

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

[Peter Maydell]

On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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.

This generally looks ok except that it has nasty interactions
with singlestep debug. For SVC, HVC and SMC instructions that
do something, we want to advance the singlestep state machine
using gen_ss_advance(), so that singlestep of one of these
insns works correctly. For UNDEF instruction patterns we don't
want to advance the state machine, so that we don't treat the
insn we didn't execute like we single-stepped it. Unfortunately
this patch means that SMC and HVC are now "sometimes this
does something but sometimes we act like it UNDEFed"...

This is my suggestion for the best compromise between
"theoretical perfect fidelity to the architecture" and
"not too painful to implement":
at translate time, do:

  if (psci enabled via HVC || EL2 implemented) {
      gen_ss_advance(s);
      gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
  } else {
      unallocated_encoding();
  }
and ditto for SMC.

Then in the exception handler have the same code as
now (with fall through to UNDEF if PSCI doesn't
recognise the op). That corresponds to "EL3 firmware
implements PSCI and handles unrecognised ops by
feeding the guest an UNDEF", which nobody would
expect to singlestep cleanly either.

> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7871,9 +7871,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, syn_aa32_hvc(imm16));
> +                break;
> +            } else if (op1 == 3) {
> +                gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
> +                break;
> +            }
>              if (op1 != 1) {
>                  goto illegal_op;
>              }
> @@ -9709,10 +9714,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) << 12;
> +                        imm16 |= extract32(insn, 0, 12);
> +                        gen_exception_insn(s, 0, EXCP_HVC, syn_aa32_hvc(imm16));
> +                    } else {
> +                        /* Secure monitor call (v6+) */
> +                        gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
> +                    }
>                  } else {
>                      op = (insn >> 20) & 7;
>                      switch (op) {

I'm pretty sure you can't do the A32/T32 HVC/SMC
like this, because of oddball cases like SMC in
an IT block. Look at the way we handle SWI by
setting s->is_jmp and then doing the actual
gen_exception() work in the tail end of the
main loop. (It might be possible to do HVC
the way you have it since HVC in an IT block
is UNPREDICTABLE, but let's do it all the same
way...)

thanks
-- PMM

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

[Ard Biesheuvel]

On 9 September 2014 19:45, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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.
>
> This generally looks ok except that it has nasty interactions
> with singlestep debug. For SVC, HVC and SMC instructions that
> do something, we want to advance the singlestep state machine
> using gen_ss_advance(), so that singlestep of one of these
> insns works correctly. For UNDEF instruction patterns we don't
> want to advance the state machine, so that we don't treat the
> insn we didn't execute like we single-stepped it. Unfortunately
> this patch means that SMC and HVC are now "sometimes this
> does something but sometimes we act like it UNDEFed"...
>
> This is my suggestion for the best compromise between
> "theoretical perfect fidelity to the architecture" and
> "not too painful to implement":
> at translate time, do:
>
>   if (psci enabled via HVC || EL2 implemented) {
>       gen_ss_advance(s);
>       gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
>   } else {
>       unallocated_encoding();
>   }
> and ditto for SMC.
>

OK, so does that mean I need to add fields to DisasContext for these
functions to inspect at the translation stage, and copy the
PSCI_METHOD_[SVC|HVC|NONE] values in it?
Or is there another way to get at that information at that stage?

> Then in the exception handler have the same code as
> now (with fall through to UNDEF if PSCI doesn't
> recognise the op). That corresponds to "EL3 firmware
> implements PSCI and handles unrecognised ops by
> feeding the guest an UNDEF", which nobody would
> expect to singlestep cleanly either.
>

ok

>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -7871,9 +7871,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, syn_aa32_hvc(imm16));
>> +                break;
>> +            } else if (op1 == 3) {
>> +                gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
>> +                break;
>> +            }
>>              if (op1 != 1) {
>>                  goto illegal_op;
>>              }
>> @@ -9709,10 +9714,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) << 12;
>> +                        imm16 |= extract32(insn, 0, 12);
>> +                        gen_exception_insn(s, 0, EXCP_HVC, syn_aa32_hvc(imm16));
>> +                    } else {
>> +                        /* Secure monitor call (v6+) */
>> +                        gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
>> +                    }
>>                  } else {
>>                      op = (insn >> 20) & 7;
>>                      switch (op) {
>
> I'm pretty sure you can't do the A32/T32 HVC/SMC
> like this, because of oddball cases like SMC in
> an IT block. Look at the way we handle SWI by
> setting s->is_jmp and then doing the actual
> gen_exception() work in the tail end of the
> main loop. (It might be possible to do HVC
> the way you have it since HVC in an IT block
> is UNPREDICTABLE, but let's do it all the same
> way...)
>

Yeah, makes sense. I will also add ARCH(6K) and ARCH(7) checks, for
SMC and HVC respectively.
(I don't suppose there is any point in adding TZ and VIRT feature bits
for this atm)

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

[Peter Maydell]

On 9 September 2014 22:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 9 September 2014 19:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This is my suggestion for the best compromise between
>> "theoretical perfect fidelity to the architecture" and
>> "not too painful to implement":
>> at translate time, do:
>>
>>   if (psci enabled via HVC || EL2 implemented) {
>>       gen_ss_advance(s);
>>       gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
>>   } else {
>>       unallocated_encoding();
>>   }
>> and ditto for SMC.
>>
>
> OK, so does that mean I need to add fields to DisasContext for these
> functions to inspect at the translation stage, and copy the
> PSCI_METHOD_[SVC|HVC|NONE] values in it?

You only need one field in DisasContext, but yes.
(The idea of DisasContext and not giving most of translate-a64.c
access to the CPU object is that it makes it hard to accidentally
access stuff in the CPU object that's not valid to depend on at
translate time, because it's an easy to spot and easy to review
change if something new gets added. PSCI method type is
OK because it's constant for the life of the simulation.)

> Yeah, makes sense. I will also add ARCH(6K) and ARCH(7) checks, for
> SMC and HVC respectively.
> (I don't suppose there is any point in adding TZ and VIRT feature bits
> for this atm)

We already have ARM_FEATURE_EL2 and ARM_FEATURE_EL3,
actually. You should probably look at Edgar's patchset on list which
adds proper SMC/HVC support -- that has failed review on a
few of the early patches but the middle of the set includes
some which also change this area:
http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02865.html
http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02866.html
I don't want this patchset to depend on that one but you
might find the shape of the code useful. (It doesn't do anything
in the 32 bit code, though.)

thanks
-- PMM

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

[Ard Biesheuvel]

On 9 September 2014 23:59, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 September 2014 22:51, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 9 September 2014 19:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> This is my suggestion for the best compromise between
>>> "theoretical perfect fidelity to the architecture" and
>>> "not too painful to implement":
>>> at translate time, do:
>>>
>>>   if (psci enabled via HVC || EL2 implemented) {
>>>       gen_ss_advance(s);
>>>       gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
>>>   } else {
>>>       unallocated_encoding();
>>>   }
>>> and ditto for SMC.
>>>
>>
>> OK, so does that mean I need to add fields to DisasContext for these
>> functions to inspect at the translation stage, and copy the
>> PSCI_METHOD_[SVC|HVC|NONE] values in it?
>
> You only need one field in DisasContext, but yes.
> (The idea of DisasContext and not giving most of translate-a64.c
> access to the CPU object is that it makes it hard to accidentally
> access stuff in the CPU object that's not valid to depend on at
> translate time, because it's an easy to spot and easy to review
> change if something new gets added. PSCI method type is
> OK because it's constant for the life of the simulation.)
>

OK

>> Yeah, makes sense. I will also add ARCH(6K) and ARCH(7) checks, for
>> SMC and HVC respectively.
>> (I don't suppose there is any point in adding TZ and VIRT feature bits
>> for this atm)
>
> We already have ARM_FEATURE_EL2 and ARM_FEATURE_EL3,
> actually. You should probably look at Edgar's patchset on list which
> adds proper SMC/HVC support -- that has failed review on a
> few of the early patches but the middle of the set includes
> some which also change this area:
> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02865.html
> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02866.html
> I don't want this patchset to depend on that one but you
> might find the shape of the code useful. (It doesn't do anything
> in the 32 bit code, though.)
>

I had a peek, and there is indeed substantial overlap, but not in an
incompatible way afaict
Setting ARM_FEATURE_EL[2|3] and then only emulate some HVC/SMC calls
under some circumstances is probably not the right way to go.
In fact, I think they are mutually exclusive, i.e., it probably makes
little sense to fully emulate EL2 /and/ use the HVC conduit for PSCI
at the same time.

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

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 2295 bytes --]

We also have v4 of the TZ patches which do provide 32-bit EL3 support, but
not EL2.  Maybe good to align with this code as well.

http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg07347.html


On 9 September 2014 16:59, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 9 September 2014 22:51, Ard Biesheuvel <ard.biesheuvel@linaro.org>
> wrote:
> > On 9 September 2014 19:45, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >> This is my suggestion for the best compromise between
> >> "theoretical perfect fidelity to the architecture" and
> >> "not too painful to implement":
> >> at translate time, do:
> >>
> >>   if (psci enabled via HVC || EL2 implemented) {
> >>       gen_ss_advance(s);
> >>       gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
> >>   } else {
> >>       unallocated_encoding();
> >>   }
> >> and ditto for SMC.
> >>
> >
> > OK, so does that mean I need to add fields to DisasContext for these
> > functions to inspect at the translation stage, and copy the
> > PSCI_METHOD_[SVC|HVC|NONE] values in it?
>
> You only need one field in DisasContext, but yes.
> (The idea of DisasContext and not giving most of translate-a64.c
> access to the CPU object is that it makes it hard to accidentally
> access stuff in the CPU object that's not valid to depend on at
> translate time, because it's an easy to spot and easy to review
> change if something new gets added. PSCI method type is
> OK because it's constant for the life of the simulation.)
>
> > Yeah, makes sense. I will also add ARCH(6K) and ARCH(7) checks, for
> > SMC and HVC respectively.
> > (I don't suppose there is any point in adding TZ and VIRT feature bits
> > for this atm)
>
> We already have ARM_FEATURE_EL2 and ARM_FEATURE_EL3,
> actually. You should probably look at Edgar's patchset on list which
> adds proper SMC/HVC support -- that has failed review on a
> few of the early patches but the middle of the set includes
> some which also change this area:
> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02865.html
> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02866.html
> I don't want this patchset to depend on that one but you
> might find the shape of the code useful. (It doesn't do anything
> in the 32 bit code, though.)
>
> thanks
> -- PMM
>
>

[-- Attachment #2: Type: text/html, Size: 3410 bytes --]

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

[Ard Biesheuvel]

On 10 September 2014 17:42, Greg Bellows <greg.bellows@linaro.org> wrote:
> We also have v4 of the TZ patches which do provide 32-bit EL3 support, but
> not EL2.  Maybe good to align with this code as well.
>
> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg07347.html
>

As far as I can tell, there is very little overlap, only patch 8/33
contains some stuff that we added as well.
I expect PSCI handling and emulation of hvc and/or smc to be mutually
exclusive, nothing we won't be able to handle with a couple of
conditionals.

My apologies for not aligning with you beforehand, I just adopted some
patches from Rob that I needed for reset and poweroff under UEFI, and
I had no idea there was so much in flight already.

-- 
Ard.

>
> On 9 September 2014 16:59, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On 9 September 2014 22:51, Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> wrote:
>> > On 9 September 2014 19:45, Peter Maydell <peter.maydell@linaro.org>
>> > wrote:
>> >> This is my suggestion for the best compromise between
>> >> "theoretical perfect fidelity to the architecture" and
>> >> "not too painful to implement":
>> >> at translate time, do:
>> >>
>> >>   if (psci enabled via HVC || EL2 implemented) {
>> >>       gen_ss_advance(s);
>> >>       gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
>> >>   } else {
>> >>       unallocated_encoding();
>> >>   }
>> >> and ditto for SMC.
>> >>
>> >
>> > OK, so does that mean I need to add fields to DisasContext for these
>> > functions to inspect at the translation stage, and copy the
>> > PSCI_METHOD_[SVC|HVC|NONE] values in it?
>>
>> You only need one field in DisasContext, but yes.
>> (The idea of DisasContext and not giving most of translate-a64.c
>> access to the CPU object is that it makes it hard to accidentally
>> access stuff in the CPU object that's not valid to depend on at
>> translate time, because it's an easy to spot and easy to review
>> change if something new gets added. PSCI method type is
>> OK because it's constant for the life of the simulation.)
>>
>> > Yeah, makes sense. I will also add ARCH(6K) and ARCH(7) checks, for
>> > SMC and HVC respectively.
>> > (I don't suppose there is any point in adding TZ and VIRT feature bits
>> > for this atm)
>>
>> We already have ARM_FEATURE_EL2 and ARM_FEATURE_EL3,
>> actually. You should probably look at Edgar's patchset on list which
>> adds proper SMC/HVC support -- that has failed review on a
>> few of the early patches but the middle of the set includes
>> some which also change this area:
>> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02865.html
>> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02866.html
>> I don't want this patchset to depend on that one but you
>> might find the shape of the code useful. (It doesn't do anything
>> in the 32 bit code, though.)
>>
>> thanks
>> -- PMM
>>
>

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

[Peter Maydell]

On 10 September 2014 17:13, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> As far as I can tell, there is very little overlap, only patch 8/33
> contains some stuff that we added as well.
> I expect PSCI handling and emulation of hvc and/or smc to be mutually
> exclusive, nothing we won't be able to handle with a couple of
> conditionals.

Agreed.

> My apologies for not aligning with you beforehand, I just adopted some
> patches from Rob that I needed for reset and poweroff under UEFI, and
> I had no idea there was so much in flight already.

The other stuff needs to come together into something coherent
anyway; I expect your changes to land first.

-- PMM

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

[Greg Bellows]

[-- Attachment #1: Type: text/plain, Size: 3187 bytes --]

No problem, just wanted to point it out in case it helps.

Greg

On 10 September 2014 11:13, Ard Biesheuvel <ard.biesheuvel@linaro.org>
wrote:

> On 10 September 2014 17:42, Greg Bellows <greg.bellows@linaro.org> wrote:
> > We also have v4 of the TZ patches which do provide 32-bit EL3 support,
> but
> > not EL2.  Maybe good to align with this code as well.
> >
> > http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg07347.html
> >
>
> As far as I can tell, there is very little overlap, only patch 8/33
> contains some stuff that we added as well.
> I expect PSCI handling and emulation of hvc and/or smc to be mutually
> exclusive, nothing we won't be able to handle with a couple of
> conditionals.
>
> My apologies for not aligning with you beforehand, I just adopted some
> patches from Rob that I needed for reset and poweroff under UEFI, and
> I had no idea there was so much in flight already.
>
> --
> Ard.
>
> >
> > On 9 September 2014 16:59, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >>
> >> On 9 September 2014 22:51, Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> wrote:
> >> > On 9 September 2014 19:45, Peter Maydell <peter.maydell@linaro.org>
> >> > wrote:
> >> >> This is my suggestion for the best compromise between
> >> >> "theoretical perfect fidelity to the architecture" and
> >> >> "not too painful to implement":
> >> >> at translate time, do:
> >> >>
> >> >>   if (psci enabled via HVC || EL2 implemented) {
> >> >>       gen_ss_advance(s);
> >> >>       gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
> >> >>   } else {
> >> >>       unallocated_encoding();
> >> >>   }
> >> >> and ditto for SMC.
> >> >>
> >> >
> >> > OK, so does that mean I need to add fields to DisasContext for these
> >> > functions to inspect at the translation stage, and copy the
> >> > PSCI_METHOD_[SVC|HVC|NONE] values in it?
> >>
> >> You only need one field in DisasContext, but yes.
> >> (The idea of DisasContext and not giving most of translate-a64.c
> >> access to the CPU object is that it makes it hard to accidentally
> >> access stuff in the CPU object that's not valid to depend on at
> >> translate time, because it's an easy to spot and easy to review
> >> change if something new gets added. PSCI method type is
> >> OK because it's constant for the life of the simulation.)
> >>
> >> > Yeah, makes sense. I will also add ARCH(6K) and ARCH(7) checks, for
> >> > SMC and HVC respectively.
> >> > (I don't suppose there is any point in adding TZ and VIRT feature bits
> >> > for this atm)
> >>
> >> We already have ARM_FEATURE_EL2 and ARM_FEATURE_EL3,
> >> actually. You should probably look at Edgar's patchset on list which
> >> adds proper SMC/HVC support -- that has failed review on a
> >> few of the early patches but the middle of the set includes
> >> some which also change this area:
> >> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02865.html
> >> http://lists.gnu.org/archive/html/qemu-devel/2014-08/msg02866.html
> >> I don't want this patchset to depend on that one but you
> >> might find the shape of the code useful. (It doesn't do anything
> >> in the 32 bit code, though.)
> >>
> >> thanks
> >> -- PMM
> >>
> >
>

[-- Attachment #2: Type: text/html, Size: 4857 bytes --]

[Qemu-devel] [PATCH v3 4/6] target-arm: add missing PSCI constants needed for PSCI emulation

[Ard Biesheuvel]

This adds some PSCI function IDs and symbolic return codes that are needed
to implement PSCI emulation in TCG mode.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 target-arm/kvm-consts.h | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/target-arm/kvm-consts.h b/target-arm/kvm-consts.h
index 091c1267d659..ba5a7c7a894d 100644
--- a/target-arm/kvm-consts.h
+++ b/target-arm/kvm-consts.h
@@ -59,14 +59,21 @@ MISMATCH_CHECK(QEMU_PSCI_0_1_FN_MIGRATE, KVM_PSCI_FN_MIGRATE)
         (QEMU_PSCI_0_2_FN_BASE + QEMU_PSCI_0_2_64BIT)
 #define QEMU_PSCI_0_2_FN64(n) (QEMU_PSCI_0_2_FN64_BASE + (n))
 
+#define QEMU_PSCI_0_2_FN_PSCI_VERSION QEMU_PSCI_0_2_FN(0)
 #define QEMU_PSCI_0_2_FN_CPU_SUSPEND QEMU_PSCI_0_2_FN(1)
 #define QEMU_PSCI_0_2_FN_CPU_OFF QEMU_PSCI_0_2_FN(2)
 #define QEMU_PSCI_0_2_FN_CPU_ON QEMU_PSCI_0_2_FN(3)
+#define QEMU_PSCI_0_2_FN_AFFINITY_INFO QEMU_PSCI_0_2_FN(4)
 #define QEMU_PSCI_0_2_FN_MIGRATE QEMU_PSCI_0_2_FN(5)
+#define QEMU_PSCI_0_2_FN_MIGRATE_INFO_TYPE QEMU_PSCI_0_2_FN(6)
+#define QEMU_PSCI_0_2_FN_MIGRATE_INFO_UP_CPU QEMU_PSCI_0_2_FN(7)
+#define QEMU_PSCI_0_2_FN_SYSTEM_OFF QEMU_PSCI_0_2_FN(8)
+#define QEMU_PSCI_0_2_FN_SYSTEM_RESET QEMU_PSCI_0_2_FN(9)
 
 #define QEMU_PSCI_0_2_FN64_CPU_SUSPEND QEMU_PSCI_0_2_FN64(1)
 #define QEMU_PSCI_0_2_FN64_CPU_OFF QEMU_PSCI_0_2_FN64(2)
 #define QEMU_PSCI_0_2_FN64_CPU_ON QEMU_PSCI_0_2_FN64(3)
+#define QEMU_PSCI_0_2_FN64_AFFINITY_INFO QEMU_PSCI_0_2_FN64(4)
 #define QEMU_PSCI_0_2_FN64_MIGRATE QEMU_PSCI_0_2_FN64(5)
 
 MISMATCH_CHECK(QEMU_PSCI_0_2_FN_CPU_SUSPEND, PSCI_0_2_FN_CPU_SUSPEND)
@@ -77,6 +84,39 @@ MISMATCH_CHECK(QEMU_PSCI_0_2_FN64_CPU_SUSPEND, PSCI_0_2_FN64_CPU_SUSPEND)
 MISMATCH_CHECK(QEMU_PSCI_0_2_FN64_CPU_ON, PSCI_0_2_FN64_CPU_ON)
 MISMATCH_CHECK(QEMU_PSCI_0_2_FN64_MIGRATE, PSCI_0_2_FN64_MIGRATE)
 
+/* PSCI v0.2 return values used by TCG emulation of PSCI */
+
+/* No Trusted OS migration to worry about when offlining CPUs */
+#define QEMU_PSCI_0_2_RET_TOS_MIGRATION_NOT_REQUIRED        2
+
+/* We implement version 0.2 only */
+#define QEMU_PSCI_0_2_RET_VERSION_0_2                       2
+
+MISMATCH_CHECK(QEMU_PSCI_0_2_RET_TOS_MIGRATION_NOT_REQUIRED, PSCI_0_2_TOS_MP)
+MISMATCH_CHECK(QEMU_PSCI_0_2_RET_VERSION_0_2,
+               PSCI_VERSION_MAJOR(0) | PSCI_VERSION_MINOR(2))
+
+/* PSCI return values (inclusive of all PSCI versions) */
+#define QEMU_PSCI_RET_SUCCESS                     0
+#define QEMU_PSCI_RET_NOT_SUPPORTED               -1
+#define QEMU_PSCI_RET_INVALID_PARAMS              -2
+#define QEMU_PSCI_RET_DENIED                      -3
+#define QEMU_PSCI_RET_ALREADY_ON                  -4
+#define QEMU_PSCI_RET_ON_PENDING                  -5
+#define QEMU_PSCI_RET_INTERNAL_FAILURE            -6
+#define QEMU_PSCI_RET_NOT_PRESENT                 -7
+#define QEMU_PSCI_RET_DISABLED                    -8
+
+MISMATCH_CHECK(QEMU_PSCI_RET_SUCCESS, PSCI_RET_SUCCESS)
+MISMATCH_CHECK(QEMU_PSCI_RET_NOT_SUPPORTED, PSCI_RET_NOT_SUPPORTED)
+MISMATCH_CHECK(QEMU_PSCI_RET_INVALID_PARAMS, PSCI_RET_INVALID_PARAMS)
+MISMATCH_CHECK(QEMU_PSCI_RET_DENIED, PSCI_RET_DENIED)
+MISMATCH_CHECK(QEMU_PSCI_RET_ALREADY_ON, PSCI_RET_ALREADY_ON)
+MISMATCH_CHECK(QEMU_PSCI_RET_ON_PENDING, PSCI_RET_ON_PENDING)
+MISMATCH_CHECK(QEMU_PSCI_RET_INTERNAL_FAILURE, PSCI_RET_INTERNAL_FAILURE)
+MISMATCH_CHECK(QEMU_PSCI_RET_NOT_PRESENT, PSCI_RET_NOT_PRESENT)
+MISMATCH_CHECK(QEMU_PSCI_RET_DISABLED, PSCI_RET_DISABLED)
+
 /* Note that KVM uses overlapping values for AArch32 and AArch64
  * target CPU numbers. AArch32 targets:
  */
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v3 4/6] target-arm: add missing PSCI constants needed for PSCI emulation

[Peter Maydell]

On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> This adds some PSCI function IDs and symbolic return codes that are needed
> to implement PSCI emulation in TCG mode.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  target-arm/kvm-consts.h | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 4/6] target-arm: add missing PSCI constants needed for PSCI emulation

[Ard Biesheuvel]

On 9 September 2014 17:28, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> This adds some PSCI function IDs and symbolic return codes that are needed
>> to implement PSCI emulation in TCG mode.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  target-arm/kvm-consts.h | 40 ++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>

Could you hold off merging this?
Or squash this:

 MISMATCH_CHECK(QEMU_PSCI_0_2_RET_VERSION_0_2,
-               PSCI_VERSION_MAJOR(0) | PSCI_VERSION_MINOR(2))
+              (PSCI_VERSION_MAJOR(0) | PSCI_VERSION_MINOR(2)))

Re: [Qemu-devel] [PATCH v3 4/6] target-arm: add missing PSCI constants needed for PSCI emulation

[Peter Maydell]

On 9 September 2014 16:34, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 9 September 2014 17:28, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> This adds some PSCI function IDs and symbolic return codes that are needed
>>> to implement PSCI emulation in TCG mode.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  target-arm/kvm-consts.h | 40 ++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 40 insertions(+)
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>
> Could you hold off merging this?

I have a bunch of review commentary on it anyway so you'll
need to respin. (If you wait an hour or so I'll get through
writing up the review on patches 3/5/6.)

-- PMM

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

[Ard Biesheuvel]

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>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 target-arm/Makefile.objs |   1 +
 target-arm/cpu-qom.h     |   6 ++
 target-arm/cpu.c         |  10 ++-
 target-arm/cpu.h         |   6 ++
 target-arm/helper.c      |  12 ++++
 target-arm/psci.c        | 171 +++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 203 insertions(+), 3 deletions(-)
 create mode 100644 target-arm/psci.c

diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
index dcd167e0d880..9460b409a5a1 100644
--- a/target-arm/Makefile.objs
+++ b/target-arm/Makefile.objs
@@ -7,5 +7,6 @@ obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
 obj-y += translate.o op_helper.o helper.o cpu.o
 obj-y += neon_helper.o iwmmxt_helper.o
 obj-y += gdbstub.o
+obj-$(CONFIG_SOFTMMU) += psci.o
 obj-$(TARGET_AARCH64) += cpu64.o translate-a64.o helper-a64.o gdbstub64.o
 obj-y += crypto_helper.o
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 104cc67e82d2..469fefb3fec8 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -101,6 +101,11 @@ typedef struct ARMCPU {
     /* CPU currently in PSCI powered-off state */
     bool powered_off;
 
+    /* PSCI emulation state
+     * 0 - disabled, 1 - smc, 2 - hvc
+     */
+    uint32_t psci_method;
+
     /* [QEMU_]KVM_ARM_TARGET_* constant for this CPU, or
      * QEMU_KVM_ARM_TARGET_NONE if the kernel doesn't support this CPU type.
      */
@@ -192,6 +197,7 @@ extern const struct VMStateDescription vmstate_arm_cpu;
 void register_cp_regs_for_features(ARMCPU *cpu);
 void init_cpreg_list(ARMCPU *cpu);
 
+bool arm_handle_psci(CPUState *cs);
 bool arm_cpu_do_hvc(CPUState *cs);
 bool arm_cpu_do_smc(CPUState *cs);
 
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index b4c06c17cf87..df094fdf5359 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -268,9 +268,12 @@ static void arm_cpu_initfn(Object *obj)
     cpu->psci_version = 1; /* By default assume PSCI v0.1 */
     cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
 
-    if (tcg_enabled() && !inited) {
-        inited = true;
-        arm_translate_init();
+    if (tcg_enabled()) {
+        cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
+        if (!inited) {
+            inited = true;
+            arm_translate_init();
+        }
     }
 }
 
@@ -1024,6 +1027,7 @@ static const ARMCPUInfo arm_cpus[] = {
 
 static Property arm_cpu_properties[] = {
     DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
+    DEFINE_PROP_UINT32("psci-method", ARMCPU, psci_method, 0),
     DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
     DEFINE_PROP_END_OF_LIST()
 };
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index d235929f4c12..2624117e58d3 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1350,4 +1350,10 @@ static inline void cpu_pc_from_tb(CPUARMState *env, TranslationBlock *tb)
     }
 }
 
+enum {
+    QEMU_PSCI_METHOD_DISABLED = 0,
+    QEMU_PSCI_METHOD_SMC = 1,
+    QEMU_PSCI_METHOD_HVC = 2,
+};
+
 #endif
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 440ee07a2ff8..021f8ed2c45f 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3499,11 +3499,23 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
 
 bool arm_cpu_do_hvc(CPUState *cs)
 {
+    ARMCPU *cpu = ARM_CPU(cs);
+
+    if (cpu->psci_method == QEMU_PSCI_METHOD_HVC) {
+        return arm_handle_psci(cs);
+    }
+
     return false;
 }
 
 bool arm_cpu_do_smc(CPUState *cs)
 {
+    ARMCPU *cpu = ARM_CPU(cs);
+
+    if (cpu->psci_method == QEMU_PSCI_METHOD_SMC) {
+        return arm_handle_psci(cs);
+    }
+
     return false;
 }
 
diff --git a/target-arm/psci.c b/target-arm/psci.c
new file mode 100644
index 000000000000..78efa06b1b00
--- /dev/null
+++ b/target-arm/psci.c
@@ -0,0 +1,171 @@
+/*
+ * Copyright (C) 2014 - Linaro
+ * Author: Rob Herring <rob.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>
+
+bool arm_handle_psci(CPUState *cs)
+{
+    /*
+     * This function partially implements the logic for dispatching Power State
+     * Coordination Interface (PSCI) calls (as described in ARM DEN 0022B.b),
+     * to the extent required for bringing up and taking down secondary cores,
+     * and for handling reset and poweroff requests.
+     * Additional information about the calling convention used is available in
+     * the document 'SMC Calling Convention' (ARM DEN 0028)
+     */
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+    uint64_t param[4];
+    uint64_t context_id, mpidr;
+    target_ulong entry;
+    int32_t ret = 0;
+    int i;
+
+    for (i = 0; i < 4; i++) {
+        /*
+         * All PSCI functions take explicit 32-bit or native int sized
+         * arguments so we can simply zero-extend all arguments regardless
+         * of which exact function we are about to call.
+         */
+        param[i] = is_a64(env) ? env->xregs[i] : env->regs[i];
+    }
+
+    if ((param[0] & QEMU_PSCI_0_2_64BIT) && !is_a64(env)) {
+        ret = QEMU_PSCI_RET_INVALID_PARAMS;
+        goto err;
+    }
+
+    switch (param[0]) {
+        CPUState *target_cpu_state;
+        ARMCPU *target_cpu;
+        CPUClass *target_cpu_class;
+
+    case QEMU_PSCI_0_2_FN_PSCI_VERSION:
+        ret = QEMU_PSCI_0_2_RET_VERSION_0_2;
+        break;
+    case QEMU_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
+        ret = QEMU_PSCI_0_2_RET_TOS_MIGRATION_NOT_REQUIRED; /* No trusted OS */
+        break;
+    case QEMU_PSCI_0_2_FN_AFFINITY_INFO:
+    case QEMU_PSCI_0_2_FN64_AFFINITY_INFO:
+        mpidr = param[1];
+
+        switch (param[2]) {
+        case 0:
+            target_cpu_state = qemu_get_cpu(mpidr & 0xff);
+            if (!target_cpu_state) {
+                ret = QEMU_PSCI_RET_INVALID_PARAMS;
+                break;
+            }
+            target_cpu = ARM_CPU(target_cpu_state);
+            ret = target_cpu->powered_off ? 1 : 0;
+            break;
+        default:
+            /* Everything above affinity level 0 is always on. */
+            ret = 0;
+        }
+        break;
+    case QEMU_PSCI_0_2_FN_SYSTEM_RESET:
+        qemu_system_reset_request();
+        break;
+    case QEMU_PSCI_0_2_FN_SYSTEM_OFF:
+        qemu_system_shutdown_request();
+        break;
+    case QEMU_PSCI_0_1_FN_CPU_ON:
+    case QEMU_PSCI_0_2_FN_CPU_ON:
+    case QEMU_PSCI_0_2_FN64_CPU_ON:
+        mpidr = param[1];
+        entry = param[2];
+        context_id = param[3];
+
+        /* change to the cpu we are powering up */
+        target_cpu_state = qemu_get_cpu(mpidr & 0xff);
+        if (!target_cpu_state) {
+            ret = QEMU_PSCI_RET_INVALID_PARAMS;
+            break;
+        }
+        target_cpu = ARM_CPU(target_cpu_state);
+        if (!target_cpu->powered_off) {
+            ret = QEMU_PSCI_RET_ALREADY_ON;
+            break;
+        }
+        target_cpu_class = CPU_GET_CLASS(target_cpu);
+
+        /* Initialize the cpu we are turning on */
+        cpu_reset(target_cpu_state);
+        target_cpu_class->set_pc(target_cpu_state, entry);
+        target_cpu->powered_off = false;
+        target_cpu_state->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+        /*
+         * The PSCI spec mandates that newly brought up CPUs enter the
+         * exception level of the caller in the same execution mode as
+         * the caller, with context_id in x0/r0, respectively.
+         */
+        if (is_a64(env)) {
+            target_cpu->env.aarch64 = 1;
+            target_cpu->env.xregs[0] = context_id;
+        } else {
+            target_cpu->env.aarch64 = 0;
+            target_cpu->env.regs[0] = context_id;
+        }
+
+        ret = 0;
+        break;
+    case QEMU_PSCI_0_1_FN_CPU_OFF:
+    case QEMU_PSCI_0_2_FN_CPU_OFF:
+        cpu->powered_off = true;
+        cs->exit_request = 1;
+        cs->halted = 1;
+
+        /* CPU_OFF should never return, but if it does return an error */
+        ret = QEMU_PSCI_RET_DENIED;
+        break;
+    case QEMU_PSCI_0_1_FN_CPU_SUSPEND:
+    case QEMU_PSCI_0_2_FN_CPU_SUSPEND:
+    case QEMU_PSCI_0_2_FN64_CPU_SUSPEND:
+        /* Affinity levels are not supported in QEMU */
+        if (param[1] & 0xfffe0000) {
+            ret = QEMU_PSCI_RET_INVALID_PARAMS;
+            break;
+        }
+        /* Powerdown is not supported, we always go into WFI */
+        cs->halted = 1;
+        cs->exit_request = 1;
+
+        /* Return success when we wakeup */
+        ret = 0;
+        break;
+    case QEMU_PSCI_0_1_FN_MIGRATE:
+    case QEMU_PSCI_0_2_FN_MIGRATE:
+        ret = QEMU_PSCI_RET_NOT_SUPPORTED;
+        break;
+    default:
+        return false;
+    }
+
+err:
+    if (is_a64(env)) {
+        env->xregs[0] = ret;
+    } else {
+        env->regs[0] = ret;
+    }
+    return true;
+}
-- 
1.8.3.2

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

[Peter Maydell]

On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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.
>
> Signed-off-by: Rob Herring <rob.herring@linaro.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  target-arm/Makefile.objs |   1 +
>  target-arm/cpu-qom.h     |   6 ++
>  target-arm/cpu.c         |  10 ++-
>  target-arm/cpu.h         |   6 ++
>  target-arm/helper.c      |  12 ++++
>  target-arm/psci.c        | 171 +++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 203 insertions(+), 3 deletions(-)
>  create mode 100644 target-arm/psci.c
>
> diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
> index dcd167e0d880..9460b409a5a1 100644
> --- a/target-arm/Makefile.objs
> +++ b/target-arm/Makefile.objs
> @@ -7,5 +7,6 @@ obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
>  obj-y += translate.o op_helper.o helper.o cpu.o
>  obj-y += neon_helper.o iwmmxt_helper.o
>  obj-y += gdbstub.o
> +obj-$(CONFIG_SOFTMMU) += psci.o
>  obj-$(TARGET_AARCH64) += cpu64.o translate-a64.o helper-a64.o gdbstub64.o
>  obj-y += crypto_helper.o
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index 104cc67e82d2..469fefb3fec8 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -101,6 +101,11 @@ typedef struct ARMCPU {
>      /* CPU currently in PSCI powered-off state */
>      bool powered_off;
>
> +    /* PSCI emulation state
> +     * 0 - disabled, 1 - smc, 2 - hvc
> +     */
> +    uint32_t psci_method;
> +
>      /* [QEMU_]KVM_ARM_TARGET_* constant for this CPU, or
>       * QEMU_KVM_ARM_TARGET_NONE if the kernel doesn't support this CPU type.
>       */
> @@ -192,6 +197,7 @@ extern const struct VMStateDescription vmstate_arm_cpu;
>  void register_cp_regs_for_features(ARMCPU *cpu);
>  void init_cpreg_list(ARMCPU *cpu);
>
> +bool arm_handle_psci(CPUState *cs);
>  bool arm_cpu_do_hvc(CPUState *cs);
>  bool arm_cpu_do_smc(CPUState *cs);
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index b4c06c17cf87..df094fdf5359 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -268,9 +268,12 @@ static void arm_cpu_initfn(Object *obj)
>      cpu->psci_version = 1; /* By default assume PSCI v0.1 */
>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
>
> -    if (tcg_enabled() && !inited) {
> -        inited = true;
> -        arm_translate_init();
> +    if (tcg_enabled()) {
> +        cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
> +        if (!inited) {
> +            inited = true;
> +            arm_translate_init();
> +        }
>      }
>  }
>
> @@ -1024,6 +1027,7 @@ static const ARMCPUInfo arm_cpus[] = {
>
>  static Property arm_cpu_properties[] = {
>      DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
> +    DEFINE_PROP_UINT32("psci-method", ARMCPU, psci_method, 0),
>      DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
>      DEFINE_PROP_END_OF_LIST()
>  };
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index d235929f4c12..2624117e58d3 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1350,4 +1350,10 @@ static inline void cpu_pc_from_tb(CPUARMState *env, TranslationBlock *tb)
>      }
>  }
>
> +enum {
> +    QEMU_PSCI_METHOD_DISABLED = 0,
> +    QEMU_PSCI_METHOD_SMC = 1,
> +    QEMU_PSCI_METHOD_HVC = 2,
> +};
> +
>  #endif
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 440ee07a2ff8..021f8ed2c45f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3499,11 +3499,23 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>
>  bool arm_cpu_do_hvc(CPUState *cs)
>  {
> +    ARMCPU *cpu = ARM_CPU(cs);
> +
> +    if (cpu->psci_method == QEMU_PSCI_METHOD_HVC) {
> +        return arm_handle_psci(cs);
> +    }
> +
>      return false;
>  }
>
>  bool arm_cpu_do_smc(CPUState *cs)
>  {
> +    ARMCPU *cpu = ARM_CPU(cs);
> +
> +    if (cpu->psci_method == QEMU_PSCI_METHOD_SMC) {
> +        return arm_handle_psci(cs);
> +    }
> +
>      return false;
>  }
>
> diff --git a/target-arm/psci.c b/target-arm/psci.c
> new file mode 100644
> index 000000000000..78efa06b1b00
> --- /dev/null
> +++ b/target-arm/psci.c
> @@ -0,0 +1,171 @@
> +/*
> + * Copyright (C) 2014 - Linaro
> + * Author: Rob Herring <rob.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>
> +
> +bool arm_handle_psci(CPUState *cs)
> +{
> +    /*
> +     * This function partially implements the logic for dispatching Power State
> +     * Coordination Interface (PSCI) calls (as described in ARM DEN 0022B.b),
> +     * to the extent required for bringing up and taking down secondary cores,
> +     * and for handling reset and poweroff requests.
> +     * Additional information about the calling convention used is available in
> +     * the document 'SMC Calling Convention' (ARM DEN 0028)
> +     */
> +    ARMCPU *cpu = ARM_CPU(cs);
> +    CPUARMState *env = &cpu->env;
> +    uint64_t param[4];
> +    uint64_t context_id, mpidr;
> +    target_ulong entry;
> +    int32_t ret = 0;
> +    int i;
> +
> +    for (i = 0; i < 4; i++) {
> +        /*
> +         * All PSCI functions take explicit 32-bit or native int sized
> +         * arguments so we can simply zero-extend all arguments regardless
> +         * of which exact function we are about to call.
> +         */
> +        param[i] = is_a64(env) ? env->xregs[i] : env->regs[i];
> +    }
> +
> +    if ((param[0] & QEMU_PSCI_0_2_64BIT) && !is_a64(env)) {
> +        ret = QEMU_PSCI_RET_INVALID_PARAMS;
> +        goto err;
> +    }
> +
> +    switch (param[0]) {
> +        CPUState *target_cpu_state;
> +        ARMCPU *target_cpu;
> +        CPUClass *target_cpu_class;
> +
> +    case QEMU_PSCI_0_2_FN_PSCI_VERSION:
> +        ret = QEMU_PSCI_0_2_RET_VERSION_0_2;
> +        break;
> +    case QEMU_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> +        ret = QEMU_PSCI_0_2_RET_TOS_MIGRATION_NOT_REQUIRED; /* No trusted OS */
> +        break;
> +    case QEMU_PSCI_0_2_FN_AFFINITY_INFO:
> +    case QEMU_PSCI_0_2_FN64_AFFINITY_INFO:
> +        mpidr = param[1];
> +
> +        switch (param[2]) {
> +        case 0:
> +            target_cpu_state = qemu_get_cpu(mpidr & 0xff);
> +            if (!target_cpu_state) {
> +                ret = QEMU_PSCI_RET_INVALID_PARAMS;
> +                break;
> +            }
> +            target_cpu = ARM_CPU(target_cpu_state);
> +            ret = target_cpu->powered_off ? 1 : 0;
> +            break;
> +        default:
> +            /* Everything above affinity level 0 is always on. */
> +            ret = 0;
> +        }
> +        break;
> +    case QEMU_PSCI_0_2_FN_SYSTEM_RESET:
> +        qemu_system_reset_request();
> +        break;
> +    case QEMU_PSCI_0_2_FN_SYSTEM_OFF:
> +        qemu_system_shutdown_request();
> +        break;
> +    case QEMU_PSCI_0_1_FN_CPU_ON:
> +    case QEMU_PSCI_0_2_FN_CPU_ON:
> +    case QEMU_PSCI_0_2_FN64_CPU_ON:
> +        mpidr = param[1];
> +        entry = param[2];
> +        context_id = param[3];
> +
> +        /* change to the cpu we are powering up */
> +        target_cpu_state = qemu_get_cpu(mpidr & 0xff);
> +        if (!target_cpu_state) {
> +            ret = QEMU_PSCI_RET_INVALID_PARAMS;
> +            break;
> +        }
> +        target_cpu = ARM_CPU(target_cpu_state);
> +        if (!target_cpu->powered_off) {
> +            ret = QEMU_PSCI_RET_ALREADY_ON;
> +            break;
> +        }
> +        target_cpu_class = CPU_GET_CLASS(target_cpu);
> +
> +        /* Initialize the cpu we are turning on */
> +        cpu_reset(target_cpu_state);
> +        target_cpu_class->set_pc(target_cpu_state, entry);

You need to do this set_pc after setting the
aarch64/aarch32 flag below, otherwise it won't set the
correct internal PC state. (But see remarks below.)

For AArch32 you need to honour bit 0 of the entry point
address which means we need to switch into Thumb mode.
(For AArch64 the spec requires us to fail the call
with INVALID_PARAMS if bit 0 is set.)

> +        target_cpu->powered_off = false;
> +        target_cpu_state->interrupt_request |= CPU_INTERRUPT_EXITTB;

I don't see why this line is needed -- the target
CPU can't possibly be midway through executing
a TB, because it was powered off.

> +
> +        /*
> +         * The PSCI spec mandates that newly brought up CPUs enter the
> +         * exception level of the caller in the same execution mode as
> +         * the caller, with context_id in x0/r0, respectively.
> +         */
> +        if (is_a64(env)) {
> +            target_cpu->env.aarch64 = 1;
> +            target_cpu->env.xregs[0] = context_id;
> +        } else {
> +            target_cpu->env.aarch64 = 0;
> +            target_cpu->env.regs[0] = context_id;
> +        }

I don't think you can safely just set the aarch64/aarch32
flag like this. You'd need to also at least write a valid
CPSR and generally do more of the things an exception return
would do.

I think in fact since for us EL1 is currently the highest
possible exception level, it's never possible for the
CPU reset state to be at a different register width from
the register width of the caller. So we should just
assert that. (When EL2/EL3 support get added this code
will need to be enhanced to cope with that anyway,
since at that point the EL that we start the guest
CPU in isn't the same as the EL the CPU resets in.)

> +
> +        ret = 0;
> +        break;
> +    case QEMU_PSCI_0_1_FN_CPU_OFF:
> +    case QEMU_PSCI_0_2_FN_CPU_OFF:
> +        cpu->powered_off = true;
> +        cs->exit_request = 1;
> +        cs->halted = 1;
> +
> +        /* CPU_OFF should never return, but if it does return an error */
> +        ret = QEMU_PSCI_RET_DENIED;

I think it would be better to do:
    cpu->powered_off = true;
    cs->halted = 1;
    cs->exception_index = EXCP_HLT;
    cpu_loop_exit(cs);
    /* notreached */

> +        break;
> +    case QEMU_PSCI_0_1_FN_CPU_SUSPEND:
> +    case QEMU_PSCI_0_2_FN_CPU_SUSPEND:
> +    case QEMU_PSCI_0_2_FN64_CPU_SUSPEND:
> +        /* Affinity levels are not supported in QEMU */
> +        if (param[1] & 0xfffe0000) {
> +            ret = QEMU_PSCI_RET_INVALID_PARAMS;
> +            break;
> +        }
> +        /* Powerdown is not supported, we always go into WFI */
> +        cs->halted = 1;
> +        cs->exit_request = 1;
> +
> +        /* Return success when we wakeup */
> +        ret = 0;

I think you could just call helper_wfi() here
instead of doing things manually (which will
longjump you out of here, as opposed to your
returning with flags set such that the main cpu
loop does a longjmp on your behalf). You would
need to set the xregs[0] return value before
that though...

> +        break;
> +    case QEMU_PSCI_0_1_FN_MIGRATE:
> +    case QEMU_PSCI_0_2_FN_MIGRATE:
> +        ret = QEMU_PSCI_RET_NOT_SUPPORTED;
> +        break;
> +    default:
> +        return false;
> +    }
> +
> +err:
> +    if (is_a64(env)) {
> +        env->xregs[0] = ret;
> +    } else {
> +        env->regs[0] = ret;
> +    }
> +    return true;
> +}
> --
> 1.8.3.2

thanks
-- PMM

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

[Ard Biesheuvel]

On 9 September 2014 19:17, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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.
>>
>> Signed-off-by: Rob Herring <rob.herring@linaro.org>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  target-arm/Makefile.objs |   1 +
>>  target-arm/cpu-qom.h     |   6 ++
>>  target-arm/cpu.c         |  10 ++-
>>  target-arm/cpu.h         |   6 ++
>>  target-arm/helper.c      |  12 ++++
>>  target-arm/psci.c        | 171 +++++++++++++++++++++++++++++++++++++++++++++++
>>  6 files changed, 203 insertions(+), 3 deletions(-)
>>  create mode 100644 target-arm/psci.c
>>
>> diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
>> index dcd167e0d880..9460b409a5a1 100644
>> --- a/target-arm/Makefile.objs
>> +++ b/target-arm/Makefile.objs
>> @@ -7,5 +7,6 @@ obj-$(call lnot,$(CONFIG_KVM)) += kvm-stub.o
>>  obj-y += translate.o op_helper.o helper.o cpu.o
>>  obj-y += neon_helper.o iwmmxt_helper.o
>>  obj-y += gdbstub.o
>> +obj-$(CONFIG_SOFTMMU) += psci.o
>>  obj-$(TARGET_AARCH64) += cpu64.o translate-a64.o helper-a64.o gdbstub64.o
>>  obj-y += crypto_helper.o
>> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
>> index 104cc67e82d2..469fefb3fec8 100644
>> --- a/target-arm/cpu-qom.h
>> +++ b/target-arm/cpu-qom.h
>> @@ -101,6 +101,11 @@ typedef struct ARMCPU {
>>      /* CPU currently in PSCI powered-off state */
>>      bool powered_off;
>>
>> +    /* PSCI emulation state
>> +     * 0 - disabled, 1 - smc, 2 - hvc
>> +     */
>> +    uint32_t psci_method;
>> +
>>      /* [QEMU_]KVM_ARM_TARGET_* constant for this CPU, or
>>       * QEMU_KVM_ARM_TARGET_NONE if the kernel doesn't support this CPU type.
>>       */
>> @@ -192,6 +197,7 @@ extern const struct VMStateDescription vmstate_arm_cpu;
>>  void register_cp_regs_for_features(ARMCPU *cpu);
>>  void init_cpreg_list(ARMCPU *cpu);
>>
>> +bool arm_handle_psci(CPUState *cs);
>>  bool arm_cpu_do_hvc(CPUState *cs);
>>  bool arm_cpu_do_smc(CPUState *cs);
>>
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index b4c06c17cf87..df094fdf5359 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -268,9 +268,12 @@ static void arm_cpu_initfn(Object *obj)
>>      cpu->psci_version = 1; /* By default assume PSCI v0.1 */
>>      cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
>>
>> -    if (tcg_enabled() && !inited) {
>> -        inited = true;
>> -        arm_translate_init();
>> +    if (tcg_enabled()) {
>> +        cpu->psci_version = 2; /* TCG implements PSCI 0.2 */
>> +        if (!inited) {
>> +            inited = true;
>> +            arm_translate_init();
>> +        }
>>      }
>>  }
>>
>> @@ -1024,6 +1027,7 @@ static const ARMCPUInfo arm_cpus[] = {
>>
>>  static Property arm_cpu_properties[] = {
>>      DEFINE_PROP_BOOL("start-powered-off", ARMCPU, start_powered_off, false),
>> +    DEFINE_PROP_UINT32("psci-method", ARMCPU, psci_method, 0),
>>      DEFINE_PROP_UINT32("midr", ARMCPU, midr, 0),
>>      DEFINE_PROP_END_OF_LIST()
>>  };
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index d235929f4c12..2624117e58d3 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -1350,4 +1350,10 @@ static inline void cpu_pc_from_tb(CPUARMState *env, TranslationBlock *tb)
>>      }
>>  }
>>
>> +enum {
>> +    QEMU_PSCI_METHOD_DISABLED = 0,
>> +    QEMU_PSCI_METHOD_SMC = 1,
>> +    QEMU_PSCI_METHOD_HVC = 2,
>> +};
>> +
>>  #endif
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 440ee07a2ff8..021f8ed2c45f 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3499,11 +3499,23 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs)
>>
>>  bool arm_cpu_do_hvc(CPUState *cs)
>>  {
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +
>> +    if (cpu->psci_method == QEMU_PSCI_METHOD_HVC) {
>> +        return arm_handle_psci(cs);
>> +    }
>> +
>>      return false;
>>  }
>>
>>  bool arm_cpu_do_smc(CPUState *cs)
>>  {
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +
>> +    if (cpu->psci_method == QEMU_PSCI_METHOD_SMC) {
>> +        return arm_handle_psci(cs);
>> +    }
>> +
>>      return false;
>>  }
>>
>> diff --git a/target-arm/psci.c b/target-arm/psci.c
>> new file mode 100644
>> index 000000000000..78efa06b1b00
>> --- /dev/null
>> +++ b/target-arm/psci.c
>> @@ -0,0 +1,171 @@
>> +/*
>> + * Copyright (C) 2014 - Linaro
>> + * Author: Rob Herring <rob.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>
>> +
>> +bool arm_handle_psci(CPUState *cs)
>> +{
>> +    /*
>> +     * This function partially implements the logic for dispatching Power State
>> +     * Coordination Interface (PSCI) calls (as described in ARM DEN 0022B.b),
>> +     * to the extent required for bringing up and taking down secondary cores,
>> +     * and for handling reset and poweroff requests.
>> +     * Additional information about the calling convention used is available in
>> +     * the document 'SMC Calling Convention' (ARM DEN 0028)
>> +     */
>> +    ARMCPU *cpu = ARM_CPU(cs);
>> +    CPUARMState *env = &cpu->env;
>> +    uint64_t param[4];
>> +    uint64_t context_id, mpidr;
>> +    target_ulong entry;
>> +    int32_t ret = 0;
>> +    int i;
>> +
>> +    for (i = 0; i < 4; i++) {
>> +        /*
>> +         * All PSCI functions take explicit 32-bit or native int sized
>> +         * arguments so we can simply zero-extend all arguments regardless
>> +         * of which exact function we are about to call.
>> +         */
>> +        param[i] = is_a64(env) ? env->xregs[i] : env->regs[i];
>> +    }
>> +
>> +    if ((param[0] & QEMU_PSCI_0_2_64BIT) && !is_a64(env)) {
>> +        ret = QEMU_PSCI_RET_INVALID_PARAMS;
>> +        goto err;
>> +    }
>> +
>> +    switch (param[0]) {
>> +        CPUState *target_cpu_state;
>> +        ARMCPU *target_cpu;
>> +        CPUClass *target_cpu_class;
>> +
>> +    case QEMU_PSCI_0_2_FN_PSCI_VERSION:
>> +        ret = QEMU_PSCI_0_2_RET_VERSION_0_2;
>> +        break;
>> +    case QEMU_PSCI_0_2_FN_MIGRATE_INFO_TYPE:
>> +        ret = QEMU_PSCI_0_2_RET_TOS_MIGRATION_NOT_REQUIRED; /* No trusted OS */
>> +        break;
>> +    case QEMU_PSCI_0_2_FN_AFFINITY_INFO:
>> +    case QEMU_PSCI_0_2_FN64_AFFINITY_INFO:
>> +        mpidr = param[1];
>> +
>> +        switch (param[2]) {
>> +        case 0:
>> +            target_cpu_state = qemu_get_cpu(mpidr & 0xff);
>> +            if (!target_cpu_state) {
>> +                ret = QEMU_PSCI_RET_INVALID_PARAMS;
>> +                break;
>> +            }
>> +            target_cpu = ARM_CPU(target_cpu_state);
>> +            ret = target_cpu->powered_off ? 1 : 0;
>> +            break;
>> +        default:
>> +            /* Everything above affinity level 0 is always on. */
>> +            ret = 0;
>> +        }
>> +        break;
>> +    case QEMU_PSCI_0_2_FN_SYSTEM_RESET:
>> +        qemu_system_reset_request();
>> +        break;
>> +    case QEMU_PSCI_0_2_FN_SYSTEM_OFF:
>> +        qemu_system_shutdown_request();
>> +        break;
>> +    case QEMU_PSCI_0_1_FN_CPU_ON:
>> +    case QEMU_PSCI_0_2_FN_CPU_ON:
>> +    case QEMU_PSCI_0_2_FN64_CPU_ON:
>> +        mpidr = param[1];
>> +        entry = param[2];
>> +        context_id = param[3];
>> +
>> +        /* change to the cpu we are powering up */
>> +        target_cpu_state = qemu_get_cpu(mpidr & 0xff);
>> +        if (!target_cpu_state) {
>> +            ret = QEMU_PSCI_RET_INVALID_PARAMS;
>> +            break;
>> +        }
>> +        target_cpu = ARM_CPU(target_cpu_state);
>> +        if (!target_cpu->powered_off) {
>> +            ret = QEMU_PSCI_RET_ALREADY_ON;
>> +            break;
>> +        }
>> +        target_cpu_class = CPU_GET_CLASS(target_cpu);
>> +
>> +        /* Initialize the cpu we are turning on */
>> +        cpu_reset(target_cpu_state);
>> +        target_cpu_class->set_pc(target_cpu_state, entry);
>
> You need to do this set_pc after setting the
> aarch64/aarch32 flag below, otherwise it won't set the
> correct internal PC state. (But see remarks below.)
>

OK

> For AArch32 you need to honour bit 0 of the entry point
> address which means we need to switch into Thumb mode.
> (For AArch64 the spec requires us to fail the call
> with INVALID_PARAMS if bit 0 is set.)
>

OK

>> +        target_cpu->powered_off = false;
>> +        target_cpu_state->interrupt_request |= CPU_INTERRUPT_EXITTB;
>
> I don't see why this line is needed -- the target
> CPU can't possibly be midway through executing
> a TB, because it was powered off.
>

I suppose Rob may have had his reasons for putting this here -- Rob?

>> +
>> +        /*
>> +         * The PSCI spec mandates that newly brought up CPUs enter the
>> +         * exception level of the caller in the same execution mode as
>> +         * the caller, with context_id in x0/r0, respectively.
>> +         */
>> +        if (is_a64(env)) {
>> +            target_cpu->env.aarch64 = 1;
>> +            target_cpu->env.xregs[0] = context_id;
>> +        } else {
>> +            target_cpu->env.aarch64 = 0;
>> +            target_cpu->env.regs[0] = context_id;
>> +        }
>
> I don't think you can safely just set the aarch64/aarch32
> flag like this. You'd need to also at least write a valid
> CPSR and generally do more of the things an exception return
> would do.
>
Well, setting the flag should be redundant in fact, so I agree that
asserting they are equal should be sufficient.

> I think in fact since for us EL1 is currently the highest
> possible exception level, it's never possible for the
> CPU reset state to be at a different register width from
> the register width of the caller. So we should just
> assert that. (When EL2/EL3 support get added this code
> will need to be enhanced to cope with that anyway,
> since at that point the EL that we start the guest
> CPU in isn't the same as the EL the CPU resets in.)
>

indeed.

>> +
>> +        ret = 0;
>> +        break;
>> +    case QEMU_PSCI_0_1_FN_CPU_OFF:
>> +    case QEMU_PSCI_0_2_FN_CPU_OFF:
>> +        cpu->powered_off = true;
>> +        cs->exit_request = 1;
>> +        cs->halted = 1;
>> +
>> +        /* CPU_OFF should never return, but if it does return an error */
>> +        ret = QEMU_PSCI_RET_DENIED;
>
> I think it would be better to do:
>     cpu->powered_off = true;
>     cs->halted = 1;
>     cs->exception_index = EXCP_HLT;
>     cpu_loop_exit(cs);
>     /* notreached */
>

OK let me try that

>> +        break;
>> +    case QEMU_PSCI_0_1_FN_CPU_SUSPEND:
>> +    case QEMU_PSCI_0_2_FN_CPU_SUSPEND:
>> +    case QEMU_PSCI_0_2_FN64_CPU_SUSPEND:
>> +        /* Affinity levels are not supported in QEMU */
>> +        if (param[1] & 0xfffe0000) {
>> +            ret = QEMU_PSCI_RET_INVALID_PARAMS;
>> +            break;
>> +        }
>> +        /* Powerdown is not supported, we always go into WFI */
>> +        cs->halted = 1;
>> +        cs->exit_request = 1;
>> +
>> +        /* Return success when we wakeup */
>> +        ret = 0;
>
> I think you could just call helper_wfi() here
> instead of doing things manually (which will
> longjump you out of here, as opposed to your
> returning with flags set such that the main cpu
> loop does a longjmp on your behalf). You would
> need to set the xregs[0] return value before
> that though...
>

OK

>> +        break;
>> +    case QEMU_PSCI_0_1_FN_MIGRATE:
>> +    case QEMU_PSCI_0_2_FN_MIGRATE:
>> +        ret = QEMU_PSCI_RET_NOT_SUPPORTED;
>> +        break;
>> +    default:
>> +        return false;
>> +    }
>> +
>> +err:
>> +    if (is_a64(env)) {
>> +        env->xregs[0] = ret;
>> +    } else {
>> +        env->regs[0] = ret;
>> +    }
>> +    return true;
>> +}
>> --
>> 1.8.3.2
>
> thanks
> -- PMM

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

[Ard Biesheuvel]

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 no longer
needs to be a KVM only feature.

Signed-off-by: Rob Herring <rob.herring@linaro.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/virt.c | 70 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 34 insertions(+), 36 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d6fffc75bda0..fefe80219d2f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -189,47 +189,43 @@ static void create_fdt(VirtBoardInfo *vbi)
 
 static void fdt_add_psci_node(const VirtBoardInfo *vbi)
 {
+    uint32_t cpu_suspend_fn;
+    uint32_t cpu_off_fn;
+    uint32_t cpu_on_fn;
+    uint32_t migrate_fn;
     void *fdt = vbi->fdt;
     ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
 
-    /* No PSCI for TCG yet */
-    if (kvm_enabled()) {
-        uint32_t cpu_suspend_fn;
-        uint32_t cpu_off_fn;
-        uint32_t cpu_on_fn;
-        uint32_t migrate_fn;
-
-        qemu_fdt_add_subnode(fdt, "/psci");
-        if (armcpu->psci_version == 2) {
-            const char comp[] = "arm,psci-0.2\0arm,psci";
-            qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
-
-            cpu_off_fn = QEMU_PSCI_0_2_FN_CPU_OFF;
-            if (arm_feature(&armcpu->env, ARM_FEATURE_AARCH64)) {
-                cpu_suspend_fn = QEMU_PSCI_0_2_FN64_CPU_SUSPEND;
-                cpu_on_fn = QEMU_PSCI_0_2_FN64_CPU_ON;
-                migrate_fn = QEMU_PSCI_0_2_FN64_MIGRATE;
-            } else {
-                cpu_suspend_fn = QEMU_PSCI_0_2_FN_CPU_SUSPEND;
-                cpu_on_fn = QEMU_PSCI_0_2_FN_CPU_ON;
-                migrate_fn = QEMU_PSCI_0_2_FN_MIGRATE;
-            }
-        } else {
-            qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci");
+    qemu_fdt_add_subnode(fdt, "/psci");
+    if (armcpu->psci_version == 2) {
+        const char comp[] = "arm,psci-0.2\0arm,psci";
+        qemu_fdt_setprop(fdt, "/psci", "compatible", comp, sizeof(comp));
 
-            cpu_suspend_fn = QEMU_PSCI_0_1_FN_CPU_SUSPEND;
-            cpu_off_fn = QEMU_PSCI_0_1_FN_CPU_OFF;
-            cpu_on_fn = QEMU_PSCI_0_1_FN_CPU_ON;
-            migrate_fn = QEMU_PSCI_0_1_FN_MIGRATE;
+        cpu_off_fn = QEMU_PSCI_0_2_FN_CPU_OFF;
+        if (arm_feature(&armcpu->env, ARM_FEATURE_AARCH64)) {
+            cpu_suspend_fn = QEMU_PSCI_0_2_FN64_CPU_SUSPEND;
+            cpu_on_fn = QEMU_PSCI_0_2_FN64_CPU_ON;
+            migrate_fn = QEMU_PSCI_0_2_FN64_MIGRATE;
+        } else {
+            cpu_suspend_fn = QEMU_PSCI_0_2_FN_CPU_SUSPEND;
+            cpu_on_fn = QEMU_PSCI_0_2_FN_CPU_ON;
+            migrate_fn = QEMU_PSCI_0_2_FN_MIGRATE;
         }
+    } else {
+        qemu_fdt_setprop_string(fdt, "/psci", "compatible", "arm,psci");
 
-        qemu_fdt_setprop_string(fdt, "/psci", "method", "hvc");
-
-        qemu_fdt_setprop_cell(fdt, "/psci", "cpu_suspend", cpu_suspend_fn);
-        qemu_fdt_setprop_cell(fdt, "/psci", "cpu_off", cpu_off_fn);
-        qemu_fdt_setprop_cell(fdt, "/psci", "cpu_on", cpu_on_fn);
-        qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
+        cpu_suspend_fn = QEMU_PSCI_0_1_FN_CPU_SUSPEND;
+        cpu_off_fn = QEMU_PSCI_0_1_FN_CPU_OFF;
+        cpu_on_fn = QEMU_PSCI_0_1_FN_CPU_ON;
+        migrate_fn = QEMU_PSCI_0_1_FN_MIGRATE;
     }
+
+    qemu_fdt_setprop_string(fdt, "/psci", "method", "hvc");
+
+    qemu_fdt_setprop_cell(fdt, "/psci", "cpu_suspend", cpu_suspend_fn);
+    qemu_fdt_setprop_cell(fdt, "/psci", "cpu_off", cpu_off_fn);
+    qemu_fdt_setprop_cell(fdt, "/psci", "cpu_on", cpu_on_fn);
+    qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
 }
 
 static void fdt_add_timer_nodes(const VirtBoardInfo *vbi)
@@ -468,8 +464,7 @@ static void machvirt_init(MachineState *machine)
     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
+     * SMP is not yet supported with TCG, so limit smp_cpus to 1
      * if we're not using KVM.
      */
     if (!kvm_enabled() && smp_cpus > 1) {
@@ -495,6 +490,9 @@ static void machvirt_init(MachineState *machine)
         }
         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.8.3.2

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

[Peter Maydell]

On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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 no longer
> needs to be a KVM only feature.

> @@ -468,8 +464,7 @@ static void machvirt_init(MachineState *machine)
>      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
> +     * SMP is not yet supported with TCG, so limit smp_cpus to 1
>       * if we're not using KVM.
>       */
>      if (!kvm_enabled() && smp_cpus > 1) {

This patch looks OK, but we should just enable the PSCI
code rather than leaving it unusable. Otherwise nobody
will report any SMP related bugs. I've posted some
patches to fix bugs today, which seems to be at least
good enough to boot an SMP kernel; we should flush out
the remaining bugs and fix those too.

-- PMM