[Qemu-devel] [PATCH v4 0/5] QEMU ARM64 Migration Fixes

[Qemu-devel] [PATCH v4 0/5] QEMU ARM64 Migration Fixes

[Alex Bennée]

This is hopefully the final update to the series. I've skipped v3 for
the purposes of having a sane relationship to the branch name ;-)

v4
  - Dropped the pl011 IRQ fiddling patch
  - Save/Restore MP STATE
    - moved into kvm.c
    - changed MP_STATE to STOPPED
  - Sync FP State
    - Removed superfluous reg.id++
  - Save/Restore SPSR
    - try and make commentary clearer
    - ensure env->banked_spsr[0] = env->spsr before we sync
  - document env->spsr
    - briefer commit message, leaving questions for the list ;-)

I submitted the kernel side of this on Friday

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

Alex Bennée (4):
  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: cpu.h document why env->spsr exists

Christoffer Dall (1):
  target-arm: kvm64 fix save/restore of SPSR regs

 hw/intc/arm_gic_kvm.c |   7 +++-
 target-arm/cpu.h      |   5 +++
 target-arm/kvm.c      |  40 ++++++++++++++++++
 target-arm/kvm32.c    |   4 ++
 target-arm/kvm64.c    | 111 +++++++++++++++++++++++++++++++++++++++++++++++---
 target-arm/kvm_arm.h  |  18 ++++++++
 6 files changed, 178 insertions(+), 7 deletions(-)

-- 
2.3.2

[Qemu-devel] [PATCH v4 1/5] 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

Re: [Qemu-devel] [PATCH v4 1/5] target-arm: kvm: save/restore mp state

[Peter Maydell]

On 16 March 2015 at 11:01, 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.

This (and the comments and function names below) still seem to
be looking at this in terms of some ambiguous "multiprocessing
state". What we are actually dealing with here is "is the
vCPU powered on or not", and I'd rather we said so explicitly.

> 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)) {

Doesn't seem like a great idea to do the extension check
every time we sync state, when it's always going to be the
same for any particular run of QEMU.

> +        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));

kvm_vcpu_ioctl() returns a negative errno, but strerror wants
a positive one.

> +            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.

"QEMU's". Also, not migration data, it's just our state.

> + */
> +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.

Apostrophe again.

> + */
> +int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu);

thanks
-- PMM

[Qemu-devel] [PATCH v4 2/5] 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 1ad3eb0..2f21ae7 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

Re: [Qemu-devel] [PATCH v4 2/5] hw/intc: arm_gic_kvm.c restore config first

[Christoffer Dall]

On Mon, Mar 16, 2015 at 11:01:53AM +0000, Alex Bennée 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 1ad3eb0..2f21ae7 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

targets? trigger? configurations?

> +     * 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 v4 3/5] 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++

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index fed03f2..8fd0c8d 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -126,9 +126,17 @@ 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))
 
+/* The linux headers don't define a 128 bit wide SIMD macro for us */
+#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 +215,37 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         }
     }
 
+    /* Advanced SIMD and FP registers */
+    for (i = 0; i < 32; i++) {
+        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
+        reg.addr = (uintptr_t)(&env->vfp.regs[i]);
+        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 +253,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 +335,31 @@ int kvm_arch_get_registers(CPUState *cs)
         }
     }
 
+    /* Advanced SIMD and FP registers */
+    for (i = 0; i < 32; i++) {
+        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
+        reg.addr = (uintptr_t)(&env->vfp.regs[i]);
+        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+        if (ret) {
+            return ret;
+        }
+    }
+
+    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

Re: [Qemu-devel] [PATCH v4 3/5] target-arm: kvm64 sync FP register state

[Peter Maydell]

On 16 March 2015 at 11:01, Alex Bennée <alex.bennee@linaro.org> wrote:
> 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++
>
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index fed03f2..8fd0c8d 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -126,9 +126,17 @@ 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))
>
> +/* The linux headers don't define a 128 bit wide SIMD macro for us */

I'm not clear what this comment is trying to tell me...

> +#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 +215,37 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>          }
>      }
>
> +    /* Advanced SIMD and FP registers */
> +    for (i = 0; i < 32; i++) {
> +        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
> +        reg.addr = (uintptr_t)(&env->vfp.regs[i]);

env->vfp.regs[] is a 64 entry array, but here we
are only dealing with the first 32 entries (and
furthermore trying to write 128 bits into a 64 bit
element...)

> +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);

Does this work right for bigendian? (ie do the kernel and
QEMU agree on which order the two halves of the 128 bit
value go in? I suspect not...)

> +        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 +253,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 +335,31 @@ int kvm_arch_get_registers(CPUState *cs)
>          }
>      }
>
> +    /* Advanced SIMD and FP registers */
> +    for (i = 0; i < 32; i++) {
> +        reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
> +        reg.addr = (uintptr_t)(&env->vfp.regs[i]);
> +        ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);

Same remarks apply here as for the set loop.

> +        if (ret) {
> +            return ret;
> +        }
> +    }
> +
> +    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
>

-- PMM

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

[Alex Bennée]

From: Christoffer Dall <christoffer.dall@linaro.org>

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: Christoffer Dall <christoffer.dall@linaro.org>
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

diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
index 8fd0c8d..7ddb1b1 100644
--- a/target-arm/kvm64.c
+++ b/target-arm/kvm64.c
@@ -140,6 +140,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;
@@ -206,9 +207,29 @@ 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 and map aarch64 exception levels if required.
+     */
+    el = arm_current_el(env);
+    if (el > 0) {
+        if (is_a64(env)) {
+            g_assert(el == 1);
+            env->banked_spsr[0] = env->spsr;
+            /* QEMUs AARCH64 EL1 SPSR is in bank 0, so map it to
+             * KVM_SPSR_SVC for syncing to KVM */
+            env->banked_spsr[1] = env->banked_spsr[0];
+        } else {
+            i = bank_number(env->uncached_cpsr & CPSR_M);
+            env->banked_spsr[i] = env->spsr;
+        }
+    }
+
     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;
@@ -254,6 +275,7 @@ 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;
 
@@ -326,15 +348,34 @@ int kvm_arch_get_registers(CPUState *cs)
         return ret;
     }
 
+    /* Fetch the SPSR registers
+     *
+     * KVM has an array of state indexed for all the possible aarch32
+     * privilege levels. These map onto QEMUs aarch32 banks 1 - 4.
+     */
     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) {
+        if (is_a64(env)) {
+            g_assert(el == 1);
+            /* KVM_SPSR_SVC holds the AARCH64 EL1 SPSR which QEMU
+             * keeps in bank 0 so copy it across. */
+            env->banked_spsr[0] = env->banked_spsr[1];
+            i = aarch64_banked_spsr_index(el);
+        } else {
+            i = bank_number(env->uncached_cpsr & CPSR_M);
+        }
+        env->spsr = env->banked_spsr[i];
+    }
+
     /* Advanced SIMD and FP registers */
     for (i = 0; i < 32; i++) {
         reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
-- 
2.3.2

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

[Christoffer Dall]

On Mon, Mar 16, 2015 at 11:01:55AM +0000, Alex Bennée wrote:
> From: Christoffer Dall <christoffer.dall@linaro.org>
> 
> 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: Christoffer Dall <christoffer.dall@linaro.org>
> 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
> 
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 8fd0c8d..7ddb1b1 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -140,6 +140,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;
> @@ -206,9 +207,29 @@ 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 and map aarch64 exception levels if required.
> +     */
> +    el = arm_current_el(env);
> +    if (el > 0) {
> +        if (is_a64(env)) {
> +            g_assert(el == 1);
> +            env->banked_spsr[0] = env->spsr;
> +            /* QEMUs AARCH64 EL1 SPSR is in bank 0, so map it to
> +             * KVM_SPSR_SVC for syncing to KVM */
> +            env->banked_spsr[1] = env->banked_spsr[0];
> +        } else {
> +            i = bank_number(env->uncached_cpsr & CPSR_M);
> +            env->banked_spsr[i] = env->spsr;
> +        }
> +    }
> +
>      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;
> @@ -254,6 +275,7 @@ 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;
>  
> @@ -326,15 +348,34 @@ int kvm_arch_get_registers(CPUState *cs)
>          return ret;
>      }
>  
> +    /* Fetch the SPSR registers
> +     *
> +     * KVM has an array of state indexed for all the possible aarch32
> +     * privilege levels. These map onto QEMUs aarch32 banks 1 - 4.
> +     */
>      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) {
> +        if (is_a64(env)) {
> +            g_assert(el == 1);
> +            /* KVM_SPSR_SVC holds the AARCH64 EL1 SPSR which QEMU
> +             * keeps in bank 0 so copy it across. */
> +            env->banked_spsr[0] = env->banked_spsr[1];
> +            i = aarch64_banked_spsr_index(el);
> +        } else {
> +            i = bank_number(env->uncached_cpsr & CPSR_M);
> +        }
> +        env->spsr = env->banked_spsr[i];
> +    }
> +
>      /* Advanced SIMD and FP registers */
>      for (i = 0; i < 32; i++) {
>          reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
> -- 
> 2.3.2
> 

looks good!
-Christoffer

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

[Peter Maydell]

On 16 March 2015 at 11:01, Alex Bennée <alex.bennee@linaro.org> wrote:
> From: Christoffer Dall <christoffer.dall@linaro.org>
>
> 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: Christoffer Dall <christoffer.dall@linaro.org>
> 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
>
> diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> index 8fd0c8d..7ddb1b1 100644
> --- a/target-arm/kvm64.c
> +++ b/target-arm/kvm64.c
> @@ -140,6 +140,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;
> @@ -206,9 +207,29 @@ 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 and map aarch64 exception levels if required.

"AArch64". Not entirely sure what the "map exception levels"
is saying.

> +     */
> +    el = arm_current_el(env);
> +    if (el > 0) {
> +        if (is_a64(env)) {
> +            g_assert(el == 1);
> +            env->banked_spsr[0] = env->spsr;

If we're in AArch64 mode then I don't believe
env->spsr has valid content at this point. The live
copy is in env->banked_sprsr[] for all cases.

> +            /* QEMUs AARCH64 EL1 SPSR is in bank 0, so map it to
> +             * KVM_SPSR_SVC for syncing to KVM */

"QEMU's" and "AArch64", but this is just a bug in our
implementation -- the mapping between AArch64 SPSR_EL1
and AArch32 SPSR_svc is architecturally mandated.
We should be using banked_spsr[1] for our regdef for
SPSR_EL1 in helper.c.

Also the "*/" should be on its own line.

> +            env->banked_spsr[1] = env->banked_spsr[0];

What is going on here? Architecturally, AArch64 SPSR_EL1
is mapped to AArch32 SPSR_svc, which is env->banked_spsr[1].
env->banked_spsr[0] would be the SPSR for USR and SYS, except
they don't have an SPSR (you cannot take exceptions into them).
I think QEMU just has a banked_spsr[0] for convenience of
implementation and it should never actually be used.
Is this code just working around the QEMU SPSR_EL1 bug?

> +            i = bank_number(env->uncached_cpsr & CPSR_M);
> +            env->banked_spsr[i] = env->spsr;
> +        }
> +    }
> +
>      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];

Coding style demands spaces around the "+" operator.

Note that this code is implicitly relying on the
ordering of register banks defined by the bank_number()
function, which is a bit icky.

>          ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
>          if (ret) {
>              return ret;
> @@ -254,6 +275,7 @@ 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;
>
> @@ -326,15 +348,34 @@ int kvm_arch_get_registers(CPUState *cs)
>          return ret;
>      }
>
> +    /* Fetch the SPSR registers
> +     *
> +     * KVM has an array of state indexed for all the possible aarch32
> +     * privilege levels. These map onto QEMUs aarch32 banks 1 - 4.

QEMU's. AArch32. Also, you mean "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];

Spaces again.

>          ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
>          if (ret) {
>              return ret;
>          }
>      }
>
> +    el = arm_current_el(env);
> +    if (el > 0) {
> +        if (is_a64(env)) {
> +            g_assert(el == 1);
> +            /* KVM_SPSR_SVC holds the AARCH64 EL1 SPSR which QEMU
> +             * keeps in bank 0 so copy it across. */
> +            env->banked_spsr[0] = env->banked_spsr[1];
> +            i = aarch64_banked_spsr_index(el);

More workarounds for a bug we should just fix, I think.

> +        } else {
> +            i = bank_number(env->uncached_cpsr & CPSR_M);
> +        }
> +        env->spsr = env->banked_spsr[i];
> +    }
> +
>      /* Advanced SIMD and FP registers */
>      for (i = 0; i < 32; i++) {
>          reg.id = AARCH64_SIMD_CORE_REG(fp_regs.vregs[i]);
> --
> 2.3.2

thanks
-- PMM

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

[Christoffer Dall]

On Tue, Mar 17, 2015 at 04:18:16PM +0000, Peter Maydell wrote:
> On 16 March 2015 at 11:01, Alex Bennée <alex.bennee@linaro.org> wrote:
> > From: Christoffer Dall <christoffer.dall@linaro.org>
> >
> > 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: Christoffer Dall <christoffer.dall@linaro.org>
> > 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
> >
> > diff --git a/target-arm/kvm64.c b/target-arm/kvm64.c
> > index 8fd0c8d..7ddb1b1 100644
> > --- a/target-arm/kvm64.c
> > +++ b/target-arm/kvm64.c
> > @@ -140,6 +140,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;
> > @@ -206,9 +207,29 @@ 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 and map aarch64 exception levels if required.
> 
> "AArch64". Not entirely sure what the "map exception levels"
> is saying.
> 
> > +     */
> > +    el = arm_current_el(env);
> > +    if (el > 0) {
> > +        if (is_a64(env)) {
> > +            g_assert(el == 1);
> > +            env->banked_spsr[0] = env->spsr;
> 
> If we're in AArch64 mode then I don't believe
> env->spsr has valid content at this point. The live
> copy is in env->banked_sprsr[] for all cases.
> 

ah, ok, so we can just get rid of that one.

> > +            /* QEMUs AARCH64 EL1 SPSR is in bank 0, so map it to
> > +             * KVM_SPSR_SVC for syncing to KVM */
> 
> "QEMU's" and "AArch64", but this is just a bug in our
> implementation -- the mapping between AArch64 SPSR_EL1
> and AArch32 SPSR_svc is architecturally mandated.
> We should be using banked_spsr[1] for our regdef for
> SPSR_EL1 in helper.c.
> 
> Also the "*/" should be on its own line.
> 
> > +            env->banked_spsr[1] = env->banked_spsr[0];
> 
> What is going on here? Architecturally, AArch64 SPSR_EL1
> is mapped to AArch32 SPSR_svc, which is env->banked_spsr[1].
> env->banked_spsr[0] would be the SPSR for USR and SYS, except
> they don't have an SPSR (you cannot take exceptions into them).
> I think QEMU just has a banked_spsr[0] for convenience of
> implementation and it should never actually be used.
> Is this code just working around the QEMU SPSR_EL1 bug?
> 

no, the idea was just to let the loop below just work.  We accomplish
this by taking the AArch64 spsr (which is stored in banked_spsr[0]) and
copying it into banked_spsr[1], which is not used in AArch64 and we can
do the QEMU's spsr are indexed at kvm-index + 1.



> > +            i = bank_number(env->uncached_cpsr & CPSR_M);
> > +            env->banked_spsr[i] = env->spsr;
> > +        }
> > +    }
> > +
> >      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];
> 
> Coding style demands spaces around the "+" operator.
> 
> Note that this code is implicitly relying on the
> ordering of register banks defined by the bank_number()
> function, which is a bit icky.

right, I thought you wrote this code with some deeper intention of doing
it this way so I tried to stick with the general idea - but now I
actually looked at git blame and realized that you didn't even write it.

Given all this churn around this, probably it's much cleaner to get rid
of the loop and have an explicit sync for each architecturally
implemented register, i.e. the SPSR_EL1 and the various mode-specific
AArch32 SPSR registers?

> 
> >          ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> >          if (ret) {
> >              return ret;
> > @@ -254,6 +275,7 @@ 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;
> >
> > @@ -326,15 +348,34 @@ int kvm_arch_get_registers(CPUState *cs)
> >          return ret;
> >      }
> >
> > +    /* Fetch the SPSR registers
> > +     *
> > +     * KVM has an array of state indexed for all the possible aarch32
> > +     * privilege levels. These map onto QEMUs aarch32 banks 1 - 4.
> 
> QEMU's. AArch32. Also, you mean "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];
> 
> Spaces again.
> 
> >          ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> >          if (ret) {
> >              return ret;
> >          }
> >      }
> >
> > +    el = arm_current_el(env);
> > +    if (el > 0) {
> > +        if (is_a64(env)) {
> > +            g_assert(el == 1);
> > +            /* KVM_SPSR_SVC holds the AARCH64 EL1 SPSR which QEMU
> > +             * keeps in bank 0 so copy it across. */
> > +            env->banked_spsr[0] = env->banked_spsr[1];
> > +            i = aarch64_banked_spsr_index(el);
> 
> More workarounds for a bug we should just fix, I think.
> 

again, this is just for the loop above to be generic, and then fix
things up afterwards so that things work both for is_a64() and
!is_a64().

Thanks,
-Christoffer

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

[Peter Maydell]

On 17 March 2015 at 19:04, Christoffer Dall <christoffer.dall@linaro.org> wrote:
> On Tue, Mar 17, 2015 at 04:18:16PM +0000, Peter Maydell wrote:
>> Note that this code is implicitly relying on the
>> ordering of register banks defined by the bank_number()
>> function, which is a bit icky.
>
> right, I thought you wrote this code with some deeper intention of doing
> it this way so I tried to stick with the general idea - but now I
> actually looked at git blame and realized that you didn't even write it.
>
> Given all this churn around this, probably it's much cleaner to get rid
> of the loop and have an explicit sync for each architecturally
> implemented register, i.e. the SPSR_EL1 and the various mode-specific
> AArch32 SPSR registers?

Yes, this seems like a good idea. I almost suggested it when
I was writing out my review comments, in fact...

>> >      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];
>>
>> Spaces again.
>>
>> >          ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
>> >          if (ret) {
>> >              return ret;
>> >          }
>> >      }
>> >
>> > +    el = arm_current_el(env);
>> > +    if (el > 0) {
>> > +        if (is_a64(env)) {
>> > +            g_assert(el == 1);
>> > +            /* KVM_SPSR_SVC holds the AARCH64 EL1 SPSR which QEMU
>> > +             * keeps in bank 0 so copy it across. */
>> > +            env->banked_spsr[0] = env->banked_spsr[1];
>> > +            i = aarch64_banked_spsr_index(el);
>>
>> More workarounds for a bug we should just fix, I think.
>>
>
> again, this is just for the loop above to be generic, and then fix
> things up afterwards so that things work both for is_a64() and
> !is_a64().

But the only reason we're fixing anything up is that we're
working around the bug. If we didn't have that bug and the
QEMU definition of where SPSR_EL1's state lived correctly
pointed at banked_spsr[1], then the only thing you'd need
to do for syncing is copy the KVM SPSRs into banked_spsr[1..5].

-- PMM

[Qemu-devel] [PATCH v4 5/5] 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 v4 5/5] target-arm: cpu.h document why env->spsr exists

[Peter Maydell]

On 16 March 2015 at 11:01, 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.

I could have sworn I'd commented about this in an earlier
version. It's not an alias for spsr_elN, because on AArch32 there are
multiple SPSRs at EL1; it is the current SPSR, and
the SPSRs for other modes are stored in banked_spsr[].

    /* SPSR for current mode. */

would do IMHO (matching the comment for env->regs[].)

-- PMM