[Qemu-devel] [PATCH v5 0/6] QEMU ARM64 Migration Fixes

[Qemu-devel] [PATCH v5 0/6] QEMU ARM64 Migration Fixes

[Alex Bennée]

Hi,

Following some review comments (and a patch) from Peter I've re-spun
this series:

v5
  - Added Peter's SPSR_EL1 state fix for architectural mapping
  - As a result SPSR save/restore no longer does munge
  - FP register save/restore re-done to deal float128 mapping
  - Some minor [ spaces ] added

I submitted the kernel side of this on Friday

Branch: https://github.com/stsquad/qemu/tree/migration/fixes-v5
Kernel: https://git.linaro.org/people/alex.bennee/linux.git/shortlog/refs/heads/migration/kvmarm-fixes-for-4.0-v3

Alex Bennée (5):
  target-arm: kvm: save/restore mp state
  hw/intc: arm_gic_kvm.c restore config first
  target-arm: kvm64 sync FP register state
  target-arm: kvm64 fix save/restore of SPSR regs
  target-arm: cpu.h document why env->spsr exists

Peter Maydell (1):
  target-arm: Store SPSR_EL1 state in banked_spsr[1] (SPSR_svc)

 hw/intc/arm_gic_kvm.c   |   7 ++-
 target-arm/cpu.h        |   5 +++
 target-arm/helper-a64.c |   2 +-
 target-arm/helper.c     |   2 +-
 target-arm/internals.h  |   5 ++-
 target-arm/kvm.c        |  40 +++++++++++++++++
 target-arm/kvm32.c      |   4 ++
 target-arm/kvm64.c      | 111 +++++++++++++++++++++++++++++++++++++++++++++---
 target-arm/kvm_arm.h    |  18 ++++++++
 9 files changed, 184 insertions(+), 10 deletions(-)

-- 
2.3.2

[Qemu-devel] [PATCH v5 1/6] target-arm: Store SPSR_EL1 state in banked_spsr[1] (SPSR_svc)

[Alex Bennée]

From: Peter Maydell <peter.maydell@linaro.org>

The AArch64 SPSR_EL1 register is architecturally mandated to
be mapped to the AArch32 SPSR_svc register. This means its
state should live in QEMU's env->banked_spsr[1] field.
Correct the various places in the code that incorrectly
put it in banked_spsr[0].

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

diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 7e0d038..861f6fa 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -523,7 +523,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         aarch64_save_sp(env, arm_current_el(env));
         env->elr_el[new_el] = env->pc;
     } else {
-        env->banked_spsr[0] = cpsr_read(env);
+        env->banked_spsr[aarch64_banked_spsr_index(new_el)] = cpsr_read(env);
         if (!env->thumb) {
             env->cp15.esr_el[new_el] |= 1 << 25;
         }
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 10886c5..d77c6de 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -2438,7 +2438,7 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
     { .name = "SPSR_EL1", .state = ARM_CP_STATE_AA64,
       .type = ARM_CP_ALIAS,
       .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 0, .opc2 = 0,
-      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, banked_spsr[0]) },
+      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, banked_spsr[1]) },
     /* We rely on the access checks not allowing the guest to write to the
      * state field when SPSel indicates that it's being used as the stack
      * pointer.
diff --git a/target-arm/internals.h b/target-arm/internals.h
index bb171a7..2cc3017 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -82,11 +82,14 @@ static inline void arm_log_exception(int idx)
 
 /*
  * For AArch64, map a given EL to an index in the banked_spsr array.
+ * Note that this mapping and the AArch32 mapping defined in bank_number()
+ * must agree such that the AArch64<->AArch32 SPSRs have the architecturally
+ * mandated mapping between each other.
  */
 static inline unsigned int aarch64_banked_spsr_index(unsigned int el)
 {
     static const unsigned int map[4] = {
-        [1] = 0, /* EL1.  */
+        [1] = 1, /* EL1.  */
         [2] = 6, /* EL2.  */
         [3] = 7, /* EL3.  */
     };
-- 
2.3.2

[Qemu-devel] [PATCH v5 2/6] target-arm: kvm: save/restore mp state

[Alex Bennée]

This adds the saving and restore of the current Multi-Processing state
of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of
potential states for x86 we only use two for ARM. Either the process is
running or not. We then save this state into the cpu_powered TCG state
to avoid changing the serialisation format.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - make mpstate field runtime dependant (kvm_enabled())
  - drop initial KVM_CAP_MP_STATE requirement
  - re-use cpu_powered instead of new field

v4
  - s/HALTED/STOPPED/
  - move code from machine.c to kvm.

diff --git a/target-arm/kvm.c b/target-arm/kvm.c
index 72c1fa1..a74832c 100644
--- a/target-arm/kvm.c
+++ b/target-arm/kvm.c
@@ -458,6 +458,46 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu)
     }
 }
 
+/*
+ * Update KVM's MP_STATE based on what QEMU thinks it is
+ */
+int kvm_arm_sync_mpstate_to_kvm(ARMCPU *cpu)
+{
+    if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) {
+        struct kvm_mp_state mp_state = {
+            .mp_state =
+            cpu->powered_off ? KVM_MP_STATE_STOPPED : KVM_MP_STATE_RUNNABLE
+        };
+        int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, &mp_state);
+        if (ret) {
+            fprintf(stderr, "%s: failed to set MP_STATE %d/%s\n",
+                    __func__, ret, strerror(ret));
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+/*
+ * Sync the KVM MP_STATE into QEMU
+ */
+int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
+{
+    if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) {
+        struct kvm_mp_state mp_state;
+        int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state);
+        if (ret) {
+            fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n",
+                    __func__, ret, strerror(ret));
+            abort();
+        }
+        cpu->powered_off = (mp_state.mp_state == KVM_MP_STATE_STOPPED);
+    }
+
+    return 0;
+}
+
 void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
 {
 }
diff --git a/target-arm/kvm32.c b/target-arm/kvm32.c
index 94030d1..49b6bab 100644
--- a/target-arm/kvm32.c
+++ b/target-arm/kvm32.c
@@ -356,6 +356,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         return EINVAL;
     }
 
+    kvm_arm_sync_mpstate_to_kvm(cpu);
+
     return ret;
 }
 
@@ -427,5 +429,7 @@ int kvm_arch_get_registers(CPUState *cs)
      */
     write_list_to_cpustate(cpu);
 
+    kvm_arm_sync_mpstate_to_qemu(cpu);
+
     return 0;
 }
diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index 8cf3a62..fed03f2 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -211,6 +211,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         return EINVAL;
     }
 
+    kvm_arm_sync_mpstate_to_kvm(cpu);
+
     /* TODO:
      * FP state
      */
@@ -310,6 +312,8 @@ int kvm_arch_get_registers(CPUState *cs)
      */
     write_list_to_cpustate(cpu);
 
+    kvm_arm_sync_mpstate_to_qemu(cpu);
+
     /* TODO: other registers */
     return ret;
 }
diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
index 455dea3..7b75758 100644
--- a/target-arm/kvm_arm.h
+++ b/target-arm/kvm_arm.h
@@ -162,6 +162,24 @@ typedef struct ARMHostCPUClass {
  */
 bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc);
 
+
+/**
+ * kvm_arm_sync_mpstate_to_kvm
+ * @cpu: ARMCPU
+ *
+ * If supported set the KVM MP_STATE based on QEMUs migration data.
+ */
+int kvm_arm_sync_mpstate_to_kvm(ARMCPU *cpu);
+
+/**
+ * kvm_arm_sync_mpstate_to_qemu
+ * @cpu: ARMCPU
+ *
+ * If supported get the MP_STATE from KVM and store in QEMUs migration
+ * data.
+ */
+int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu);
+
 #endif
 
 #endif
-- 
2.3.2

[Qemu-devel] [PATCH v5 3/6] hw/intc: arm_gic_kvm.c restore config first

[Alex Bennée]

As there is logic to deal with the difference between edge and level
triggered interrupts in the kernel we must ensure it knows the
configuration of the IRQs before we restore the pending state.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index 0d20750..e2512f1 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -370,6 +370,11 @@ static void kvm_arm_gic_put(GICState *s)
      * the appropriate CPU interfaces in the kernel) */
     kvm_dist_put(s, 0x800, 8, s->num_irq, translate_targets);
 
+    /* irq_state[n].trigger -> GICD_ICFGRn
+     * (restore targets before pending IRQs so we treat level/edge
+     * correctly */
+    kvm_dist_put(s, 0xc00, 2, s->num_irq, translate_trigger);
+
     /* irq_state[n].pending + irq_state[n].level -> GICD_ISPENDRn */
     kvm_dist_put(s, 0x280, 1, s->num_irq, translate_clear);
     kvm_dist_put(s, 0x200, 1, s->num_irq, translate_pending);
@@ -378,8 +383,6 @@ static void kvm_arm_gic_put(GICState *s)
     kvm_dist_put(s, 0x380, 1, s->num_irq, translate_clear);
     kvm_dist_put(s, 0x300, 1, s->num_irq, translate_active);
 
-    /* irq_state[n].trigger -> GICD_ICFRn */
-    kvm_dist_put(s, 0xc00, 2, s->num_irq, translate_trigger);
 
     /* s->priorityX[irq] -> ICD_IPRIORITYRn */
     kvm_dist_put(s, 0x400, 8, s->num_irq, translate_priority);
-- 
2.3.2

[Qemu-devel] [PATCH v5 4/6] target-arm: kvm64 sync FP register state

[Alex Bennée]

For migration to work we need to sync all of the register state. This is
especially noticeable when GCC starts using FP registers as spill
registers even with integer programs.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---

v4:
  - fixed merge conflicts
  - rm superfluous reg.id++
v5:
  - use interim float128 to deal with endianess
  - correctly map into vfp.regs[]
  - fix spacing around []s

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index fed03f2..857e970 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -126,9 +126,16 @@ bool kvm_arm_reg_syncs_via_cpreg_list(uint64_t regidx)
 #define AARCH64_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
                  KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
 
+#define AARCH64_SIMD_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U128 | \
+                 KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
+
+#define AARCH64_SIMD_CTRL_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U32 | \
+                 KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
+
 int kvm_arch_put_registers(CPUState *cs, int level)
 {
     struct kvm_one_reg reg;
+    uint32_t fpr;
     uint64_t val;
     int i;
     int ret;
@@ -207,15 +214,42 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         }
     }
 
+    /* Advanced SIMD and FP registers
+     * We map Qn = regs[2n+1]:regs[2n]
+     */
+    for (i = 0; i < 32; i++) {
+        int rd = i << 1;
+        float128 fp_val = make_float128(env->vfp.regs[rd + 1],
+                                        env->vfp.regs[rd]);
+        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
+        reg.addr = (uintptr_t)(&fp_val);
+        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    reg.addr = (uintptr_t)(&fpr);
+    fpr = vfp_get_fpsr(env);
+    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
+    fpr = vfp_get_fpcr(env);
+    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+
     if (!write_list_to_kvmstate(cpu)) {
         return EINVAL;
     }
 
     kvm_arm_sync_mpstate_to_kvm(cpu);
 
-    /* TODO:
-     * FP state
-     */
     return ret;
 }
 
@@ -223,6 +257,7 @@ int kvm_arch_get_registers(CPUState *cs)
 {
     struct kvm_one_reg reg;
     uint64_t val;
+    uint32_t fpr;
     int i;
     int ret;
 
@@ -304,6 +339,38 @@ int kvm_arch_get_registers(CPUState *cs)
         }
     }
 
+    /* Advanced SIMD and FP registers
+     * We map Qn = regs[2n+1]:regs[2n]
+     */
+    for (i = 0; i < 32; i++) {
+        float128 fp_val;
+        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
+        reg.addr = (uintptr_t)(&fp_val);
+        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        } else {
+            int rd = i << 1;
+            env->vfp.regs[rd + 1] = fp_val.high;
+            env->vfp.regs[rd] = fp_val.low;
+        }
+    }
+
+    reg.addr = (uintptr_t)(&fpr);
+    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpsr);
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+    vfp_set_fpsr(env, fpr);
+
+    reg.id = AARCH64_SIMD_CTRL_REG(fp_regs.fpcr);
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret) {
+        return ret;
+    }
+    vfp_set_fpcr(env, fpr);
+
     if (!write_kvmstate_to_list(cpu)) {
         return EINVAL;
     }
-- 
2.3.2

[Qemu-devel] [PATCH v5 5/6] target-arm: kvm64 fix save/restore of SPSR regs

[Alex Bennée]

The current code was negatively indexing the cpu state array and not
synchronizing banked spsr register state with the current mode's spsr
state, causing occasional failures with migration.

Some munging is done to take care of the aarch64 mapping and also to
ensure the most current value of the spsr is updated to the banked
registers (relevant for KVM<->TCG migration).

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2 (ajb)
  - minor tweaks and clarifications
v3
  - Use the correct bank index function for setting/getting env->spsr
  - only deal with spsrs in elevated exception levels
v4
 - try and make commentary clearer
 - ensure env->banked_spsr[0] = env->spsr before we sync
v5
 - fix banking index now banking fixed
 - keep wide spacing on [ ] forms
 - claimed authorship

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index 857e970..5270fa7 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -139,6 +139,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     uint64_t val;
     int i;
     int ret;
+    unsigned int el;
 
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
@@ -205,9 +206,24 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         return ret;
     }
 
+    /* Saved Program State Registers
+     *
+     * Before we restore from the banked_spsr[] array we need to
+     * ensure that any modifications to env->spsr are correctly
+     * reflected in the banks.
+     */
+    el = arm_current_el(env);
+    if (el > 0) {
+        i = is_a64(env) ?
+            aarch64_banked_spsr_index(el) :
+            bank_number(env->uncached_cpsr & CPSR_M);
+        env->banked_spsr[i] = env->spsr;
+    }
+
+    /* KVM 0-4 map to QEMU banks 1-5 */
     for (i = 0; i < KVM_NR_SPSR; i++) {
         reg.id = AARCH64_CORE_REG(spsr[i]);
-        reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
+        reg.addr = (uintptr_t) &env->banked_spsr[i + 1];
         ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
         if (ret) {
             return ret;
@@ -253,11 +269,13 @@ int kvm_arch_put_registers(CPUState *cs, int level)
     return ret;
 }
 
+
 int kvm_arch_get_registers(CPUState *cs)
 {
     struct kvm_one_reg reg;
     uint64_t val;
     uint32_t fpr;
+    unsigned int el;
     int i;
     int ret;
 
@@ -330,15 +348,27 @@ int kvm_arch_get_registers(CPUState *cs)
         return ret;
     }
 
+    /* Fetch the SPSR registers
+     *
+     * KVM SPSRs 0-4 map to QEMU banks 1-5
+     */
     for (i = 0; i < KVM_NR_SPSR; i++) {
         reg.id = AARCH64_CORE_REG(spsr[i]);
-        reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
+        reg.addr = (uintptr_t) &env->banked_spsr[i + 1];
         ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
         if (ret) {
             return ret;
         }
     }
 
+    el = arm_current_el(env);
+    if (el > 0) {
+        i = is_a64(env) ?
+            aarch64_banked_spsr_index(el) :
+            bank_number(env->uncached_cpsr & CPSR_M);
+        env->spsr = env->banked_spsr[i];
+    }
+
     /* Advanced SIMD and FP registers
      * We map Qn = regs[2n+1]:regs[2n]
      */
-- 
2.3.2

[Qemu-devel] [PATCH v5 6/6] target-arm: cpu.h document why env->spsr exists

[Alex Bennée]

I was getting very confused about the duplication of state so wanted to
make it explicit.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 083211c..6dc1799 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -155,6 +155,11 @@ typedef struct CPUARMState {
        This contains all the other bits.  Use cpsr_{read,write} to access
        the whole CPSR.  */
     uint32_t uncached_cpsr;
+    /* The spsr is a alias for spsr_elN where N is the current
+     * exception level. It is provided for here so the TCG msr/mrs
+     * implementation can access one register. Care needs to be taken
+     * to ensure the banked_spsr[] is also updated.
+     */
     uint32_t spsr;
 
     /* Banked registers.  */
-- 
2.3.2

Re: [Qemu-devel] [PATCH v5 1/6] target-arm: Store SPSR_EL1 state in banked_spsr[1] (SPSR_svc)

[Greg Bellows]

On Mon, Mar 23, 2015 at 12:05 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
> From: Peter Maydell <peter.maydell@linaro.org>
>
> The AArch64 SPSR_EL1 register is architecturally mandated to
> be mapped to the AArch32 SPSR_svc register. This means its
> state should live in QEMU's env->banked_spsr[1] field.
> Correct the various places in the code that incorrectly
> put it in banked_spsr[0].
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index 7e0d038..861f6fa 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -523,7 +523,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>          aarch64_save_sp(env, arm_current_el(env));
>          env->elr_el[new_el] = env->pc;
>      } else {
> -        env->banked_spsr[0] = cpsr_read(env);
> +        env->banked_spsr[aarch64_banked_spsr_index(new_el)] = cpsr_read(env);

Are the other banks (2-5) only used for KVM?  It seems we go out of
our way to manage this larger SPSR array then not use all of the slots
in QEMU itself.

>          if (!env->thumb) {
>              env->cp15.esr_el[new_el] |= 1 << 25;
>          }
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 10886c5..d77c6de 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2438,7 +2438,7 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
>      { .name = "SPSR_EL1", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_ALIAS,
>        .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 0, .opc2 = 0,
> -      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, banked_spsr[0]) },
> +      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, banked_spsr[1]) },
>      /* We rely on the access checks not allowing the guest to write to the
>       * state field when SPSel indicates that it's being used as the stack
>       * pointer.
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index bb171a7..2cc3017 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -82,11 +82,14 @@ static inline void arm_log_exception(int idx)
>
>  /*
>   * For AArch64, map a given EL to an index in the banked_spsr array.
> + * Note that this mapping and the AArch32 mapping defined in bank_number()
> + * must agree such that the AArch64<->AArch32 SPSRs have the architecturally
> + * mandated mapping between each other.
>   */
>  static inline unsigned int aarch64_banked_spsr_index(unsigned int el)
>  {
>      static const unsigned int map[4] = {
> -        [1] = 0, /* EL1.  */
> +        [1] = 1, /* EL1.  */
>          [2] = 6, /* EL2.  */
>          [3] = 7, /* EL3.  */
>      };
> --
> 2.3.2
>
>

Re: [Qemu-devel] [PATCH v5 1/6] target-arm: Store SPSR_EL1 state in banked_spsr[1] (SPSR_svc)

[Peter Maydell]

On 24 March 2015 at 14:32, Greg Bellows <greg.bellows@linaro.org> wrote:
> On Mon, Mar 23, 2015 at 12:05 PM, Alex Bennée <alex.bennee@linaro.org> wrote:
>> From: Peter Maydell <peter.maydell@linaro.org>

>> @@ -523,7 +523,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>>          aarch64_save_sp(env, arm_current_el(env));
>>          env->elr_el[new_el] = env->pc;
>>      } else {
>> -        env->banked_spsr[0] = cpsr_read(env);
>> +        env->banked_spsr[aarch64_banked_spsr_index(new_el)] = cpsr_read(env);
>
> Are the other banks (2-5) only used for KVM?  It seems we go out of
> our way to manage this larger SPSR array then not use all of the slots
> in QEMU itself.

They're used in AArch32 (where they are the SPSR for various
32 bit modes). In AArch64 you can access those registers via
MSR/MRS (we probably haven't implemented those yet because they
are only accessible at EL2 and above) so hypervisors can do
worldswitches. But for exception entry and return (which is
what this code is) we only use SPSR_EL0/SPSR_EL1/SPSR_EL2/SPSR_EL3
which is a subset of the AArch32 SPSRs.

-- PMM

Re: [Qemu-devel] [PATCH v5 2/6] target-arm: kvm: save/restore mp state

[Peter Maydell]

On 23 March 2015 at 17:05, Alex Bennée <alex.bennee@linaro.org> wrote:
> This adds the saving and restore of the current Multi-Processing state
> of the machine. While the KVM_GET/SET_MP_STATE API exposes a number of
> potential states for x86 we only use two for ARM. Either the process is
> running or not. We then save this state into the cpu_powered TCG state
> to avoid changing the serialisation format.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - make mpstate field runtime dependant (kvm_enabled())
>   - drop initial KVM_CAP_MP_STATE requirement
>   - re-use cpu_powered instead of new field
>
> v4
>   - s/HALTED/STOPPED/
>   - move code from machine.c to kvm.
>
> diff --git a/target-arm/kvm.c b/target-arm/kvm.c
> index 72c1fa1..a74832c 100644
> --- a/target-arm/kvm.c
> +++ b/target-arm/kvm.c
> @@ -458,6 +458,46 @@ void kvm_arm_reset_vcpu(ARMCPU *cpu)
>      }
>  }
>
> +/*
> + * Update KVM's MP_STATE based on what QEMU thinks it is
> + */
> +int kvm_arm_sync_mpstate_to_kvm(ARMCPU *cpu)
> +{
> +    if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) {

Doing the ioctl to check the extension every time isn't great.
Look at the way ppc and s390 do a cap check at init time and
cache the answer.

> +        struct kvm_mp_state mp_state = {
> +            .mp_state =
> +            cpu->powered_off ? KVM_MP_STATE_STOPPED : KVM_MP_STATE_RUNNABLE
> +        };
> +        int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MP_STATE, &mp_state);
> +        if (ret) {
> +            fprintf(stderr, "%s: failed to set MP_STATE %d/%s\n",
> +                    __func__, ret, strerror(ret));
> +            return -1;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +/*
> + * Sync the KVM MP_STATE into QEMU
> + */
> +int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
> +{
> +    if (kvm_check_extension(CPU(cpu)->kvm_state, KVM_CAP_MP_STATE)) {
> +        struct kvm_mp_state mp_state;
> +        int ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MP_STATE, &mp_state);
> +        if (ret) {
> +            fprintf(stderr, "%s: failed to get MP_STATE %d/%s\n",
> +                    __func__, ret, strerror(ret));
> +            abort();
> +        }
> +        cpu->powered_off = (mp_state.mp_state == KVM_MP_STATE_STOPPED);
> +    }
> +
> +    return 0;
> +}
> +
>  void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
>  {
>  }
> diff --git a/target-arm/kvm32.c b/target-arm/kvm32.c
> index 94030d1..49b6bab 100644
> --- a/target-arm/kvm32.c
> +++ b/target-arm/kvm32.c
> @@ -356,6 +356,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>          return EINVAL;
>      }
>
> +    kvm_arm_sync_mpstate_to_kvm(cpu);
> +
>      return ret;
>  }
>
> @@ -427,5 +429,7 @@ int kvm_arch_get_registers(CPUState *cs)
>       */
>      write_list_to_cpustate(cpu);
>
> +    kvm_arm_sync_mpstate_to_qemu(cpu);
> +
>      return 0;
>  }
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 8cf3a62..fed03f2 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -211,6 +211,8 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>          return EINVAL;
>      }
>
> +    kvm_arm_sync_mpstate_to_kvm(cpu);
> +
>      /* TODO:
>       * FP state
>       */
> @@ -310,6 +312,8 @@ int kvm_arch_get_registers(CPUState *cs)
>       */
>      write_list_to_cpustate(cpu);
>
> +    kvm_arm_sync_mpstate_to_qemu(cpu);
> +
>      /* TODO: other registers */
>      return ret;
>  }
> diff --git a/target-arm/kvm_arm.h b/target-arm/kvm_arm.h
> index 455dea3..7b75758 100644
> --- a/target-arm/kvm_arm.h
> +++ b/target-arm/kvm_arm.h
> @@ -162,6 +162,24 @@ typedef struct ARMHostCPUClass {
>   */
>  bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc);
>
> +
> +/**
> + * kvm_arm_sync_mpstate_to_kvm
> + * @cpu: ARMCPU
> + *
> + * If supported set the KVM MP_STATE based on QEMUs migration data.

Apostrophes (again!). And the reference to migration data
here is spurious.


> + */
> +int kvm_arm_sync_mpstate_to_kvm(ARMCPU *cpu);
> +
> +/**
> + * kvm_arm_sync_mpstate_to_qemu
> + * @cpu: ARMCPU
> + *
> + * If supported get the MP_STATE from KVM and store in QEMUs migration
> + * data.
> + */
> +int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu);
> +
>  #endif
>
>  #endif
> --
> 2.3.2

-- PMM

Re: [Qemu-devel] [PATCH v5 3/6] hw/intc: arm_gic_kvm.c restore config first

[Peter Maydell]

On 23 March 2015 at 17:05, Alex Bennée <alex.bennee@linaro.org> wrote:
> As there is logic to deal with the difference between edge and level
> triggered interrupts in the kernel we must ensure it knows the
> configuration of the IRQs before we restore the pending state.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
>
> diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
> index 0d20750..e2512f1 100644
> --- a/hw/intc/arm_gic_kvm.c
> +++ b/hw/intc/arm_gic_kvm.c
> @@ -370,6 +370,11 @@ static void kvm_arm_gic_put(GICState *s)
>       * the appropriate CPU interfaces in the kernel) */
>      kvm_dist_put(s, 0x800, 8, s->num_irq, translate_targets);
>
> +    /* irq_state[n].trigger -> GICD_ICFGRn
> +     * (restore targets before pending IRQs so we treat level/edge
> +     * correctly */
> +    kvm_dist_put(s, 0xc00, 2, s->num_irq, translate_trigger);

You don't seem to have acted on Christoffer's query in v4
about this comment...

-- PMM

Re: [Qemu-devel] [PATCH v5 4/6] target-arm: kvm64 sync FP register state

[Peter Maydell]

On 23 March 2015 at 17:05, Alex Bennée <alex.bennee@linaro.org> wrote:
> +    /* Advanced SIMD and FP registers
> +     * We map Qn = regs[2n+1]:regs[2n]
> +     */
> +    for (i = 0; i < 32; i++) {
> +        int rd = i << 1;
> +        float128 fp_val = make_float128(env->vfp.regs[rd + 1],
> +                                        env->vfp.regs[rd]);

float128 is part of softfloat; it doesn't really belong here...

-- PMM

Re: [Qemu-devel] [PATCH v5 5/6] target-arm: kvm64 fix save/restore of SPSR regs

[Peter Maydell]

On 23 March 2015 at 17:05, Alex Bennée <alex.bennee@linaro.org> wrote:
> The current code was negatively indexing the cpu state array and not
> synchronizing banked spsr register state with the current mode's spsr
> state, causing occasional failures with migration.
>
> Some munging is done to take care of the aarch64 mapping and also to
> ensure the most current value of the spsr is updated to the banked
> registers (relevant for KVM<->TCG migration).
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2 (ajb)
>   - minor tweaks and clarifications
> v3
>   - Use the correct bank index function for setting/getting env->spsr
>   - only deal with spsrs in elevated exception levels
> v4
>  - try and make commentary clearer
>  - ensure env->banked_spsr[0] = env->spsr before we sync
> v5
>  - fix banking index now banking fixed
>  - keep wide spacing on [ ] forms
>  - claimed authorship
>
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 857e970..5270fa7 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -139,6 +139,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      uint64_t val;
>      int i;
>      int ret;
> +    unsigned int el;
>
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> @@ -205,9 +206,24 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>          return ret;
>      }
>
> +    /* Saved Program State Registers
> +     *
> +     * Before we restore from the banked_spsr[] array we need to
> +     * ensure that any modifications to env->spsr are correctly
> +     * reflected in the banks.
> +     */
> +    el = arm_current_el(env);
> +    if (el > 0) {
> +        i = is_a64(env) ?
> +            aarch64_banked_spsr_index(el) :
> +            bank_number(env->uncached_cpsr & CPSR_M);
> +        env->banked_spsr[i] = env->spsr;

This doesn't look right. If we're currently A64 then
env->spsr is unused and this code will trash one of
the banked_spsr[] fields.

> +    }
> +
> +    /* KVM 0-4 map to QEMU banks 1-5 */
>      for (i = 0; i < KVM_NR_SPSR; i++) {
>          reg.id = AARCH64_CORE_REG(spsr[i]);
> -        reg.addr = (uintptr_t) &env->banked_spsr[i - 1];
> +        reg.addr = (uintptr_t) &env->banked_spsr[i + 1];
>          ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
>          if (ret) {
>              return ret;
> @@ -253,11 +269,13 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      return ret;
>  }
>
> +

Stray extra space.

-- PMM

Re: [Qemu-devel] [PATCH v5 6/6] target-arm: cpu.h document why env->spsr exists

[Peter Maydell]

On 23 March 2015 at 17:05, Alex Bennée <alex.bennee@linaro.org> wrote:
> I was getting very confused about the duplication of state so wanted to
> make it explicit.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 083211c..6dc1799 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -155,6 +155,11 @@ typedef struct CPUARMState {
>         This contains all the other bits.  Use cpsr_{read,write} to access
>         the whole CPSR.  */
>      uint32_t uncached_cpsr;
> +    /* The spsr is a alias for spsr_elN where N is the current
> +     * exception level.

This comment is still wrong.

> It is provided for here so the TCG msr/mrs
> +     * implementation can access one register.

...and this is wrong too.

> Care needs to be taken
> +     * to ensure the banked_spsr[] is also updated.
> +     */
>      uint32_t spsr;
>
>      /* Banked registers.  */

I think it would be better to just drop this patch, really...

-- PMM