Thread (14 messages) 14 messages, 3 authors, 2015-03-26
STALE4125d
Revisions (11)
  1. v1 [diff vs current]
  2. v1 [diff vs current]
  3. v1 [diff vs current]
  4. v1 [diff vs current]
  5. v2 [diff vs current]
  6. v1 [diff vs current]
  7. v2 [diff vs current]
  8. v4 [diff vs current]
  9. v4 [diff vs current]
  10. v5 [diff vs current]
  11. v5 current

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

From: Peter Maydell <hidden>
Date: 2015-03-26 17:25:28
Also in: kvm, kvmarm, qemu-devel

On 23 March 2015 at 17:05, Alex Benn?e [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted>

---
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.
quoted hunk ↗ jump to hunk
+    }
+
+    /* 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help