Add support for 64bit ARM system registers

PATCH Superseded Alexey Starikovskiy 3 participants 8 messages Mar 14, 2012

lore

Share & tools

lore

Alexey Starikovskiy

author

2012-03-14 13:00 UTC #0 lore raw

PATCH v2 1/3 Add support for 64bit ARM system registers

Signed-off-by Alexey Starikovskiy<[email protected]>

---
 target-arm/cpu.h     |   10 ++++------
 target-arm/helper.c  |   14 +++++++-------
 target-arm/machine.c |   16 ++++++----------
 3 files changed, 17 insertions(+), 23 deletions(-)

target-arm/cpu.h +4 -6

index 0d9b39c..0298a98 100644
@@ -117,11 +117,9 @@ typedef struct CPUARMState {
         uint32_t c1_coproc; /* Coprocessor access register.  */
         uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.  */
         uint32_t c1_scr; /* secure config register.  */
-        uint32_t c2_base0; /* MMU translation table base 0.  */
-        uint32_t c2_base1; /* MMU translation table base 1.  */
+        uint64_t c2_base0; /* MMU translation table base 0.  */
+        uint64_t c2_base1; /* MMU translation table base 1.  */
         uint32_t c2_control; /* MMU translation table base control.  */
-        uint32_t c2_mask; /* MMU translation table base selection mask.  */
-        uint32_t c2_base_mask; /* MMU translation table base 0 mask. */
         uint32_t c2_data; /* MPU data cachable bits.  */
         uint32_t c2_insn; /* MPU instruction cachable bits.  */
         uint32_t c3; /* MMU domain access control register

@@ -131,7 +129,7 @@ typedef struct CPUARMState {
         uint32_t c6_region[8]; /* MPU base/size registers.  */
         uint32_t c6_insn; /* Fault address registers.  */
         uint32_t c6_data;
-        uint32_t c7_par;  /* Translation result. */
+        uint64_t c7_par;  /* Translation result. */
         uint32_t c9_insn; /* Cache lockdown registers.  */
         uint32_t c9_data;
         uint32_t c9_pmcr; /* performance monitor control register */

@@ -455,7 +453,7 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
 #define cpu_signal_handler cpu_arm_signal_handler
 #define cpu_list arm_cpu_list
 
-#define CPU_SAVE_VERSION 6
+#define CPU_SAVE_VERSION 7
 
 /* MMU modes definitions */
 #define MMU_MODE0_SUFFIX _kernel

target-arm/helper.c +7 -7

index abe1c30..d190104 100644
@@ -325,7 +325,6 @@ void cpu_reset(CPUARMState *env)
         }
     }
     env->vfp.xregs[ARM_VFP_FPEXC] = 0;
-    env->cp15.c2_base_mask = 0xffffc000u;
     /* v7 performance monitor control register: same implementor
      * field as main ID register, and we implement no event counters.
      */

@@ -1050,12 +1049,15 @@ static inline int check_ap(CPUState *env, int ap, int domain_prot,
 static uint32_t get_level1_table_address(CPUState *env, uint32_t address)
 {
     uint32_t table;
+    int t0size = env->cp15.c2_control&  0x7;
+    uint32_t mask = ~(((uint32_t)0xffffffffu)>>  t0size);
 
-    if (address&  env->cp15.c2_mask)
+    if (address&  mask) {
         table = env->cp15.c2_base1&  0xffffc000;
-    else
-        table = env->cp15.c2_base0&  env->cp15.c2_base_mask;
-
+    } else {
+        mask = ~((uint32_t)0x3fffu>>  t0size);
+        table = env->cp15.c2_base0&  mask;
+    }
     table |= (address>>  18)&  0x3ffc;
     return table;
 }

@@ -1531,8 +1533,6 @@ void HELPER(set_cp15)(CPUState *env, uint32_t insn, uint32_t val)
 	    case 2:
                 val&= 7;
                 env->cp15.c2_control = val;
-		env->cp15.c2_mask = ~(((uint32_t)0xffffffffu)>>  val);
-                env->cp15.c2_base_mask = ~((uint32_t)0x3fffu>>  val);
 		break;
 	    default:
 		goto bad_reg;

target-arm/machine.c +6 ~~-10~~

index f66b8df..8fa738e 100644
@@ -27,11 +27,9 @@ void cpu_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, env->cp15.c1_coproc);
     qemu_put_be32(f, env->cp15.c1_xscaleauxcr);
     qemu_put_be32(f, env->cp15.c1_scr);
-    qemu_put_be32(f, env->cp15.c2_base0);
-    qemu_put_be32(f, env->cp15.c2_base1);
+    qemu_put_be64(f, env->cp15.c2_base0);
+    qemu_put_be64(f, env->cp15.c2_base1);
     qemu_put_be32(f, env->cp15.c2_control);
-    qemu_put_be32(f, env->cp15.c2_mask);
-    qemu_put_be32(f, env->cp15.c2_base_mask);
     qemu_put_be32(f, env->cp15.c2_data);
     qemu_put_be32(f, env->cp15.c2_insn);
     qemu_put_be32(f, env->cp15.c3);

@@ -42,7 +40,7 @@ void cpu_save(QEMUFile *f, void *opaque)
     }
     qemu_put_be32(f, env->cp15.c6_insn);
     qemu_put_be32(f, env->cp15.c6_data);
-    qemu_put_be32(f, env->cp15.c7_par);
+    qemu_put_be64(f, env->cp15.c7_par);
     qemu_put_be32(f, env->cp15.c9_insn);
     qemu_put_be32(f, env->cp15.c9_data);
     qemu_put_be32(f, env->cp15.c9_pmcr);

@@ -145,11 +143,9 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
     env->cp15.c1_coproc = qemu_get_be32(f);
     env->cp15.c1_xscaleauxcr = qemu_get_be32(f);
     env->cp15.c1_scr = qemu_get_be32(f);
-    env->cp15.c2_base0 = qemu_get_be32(f);
-    env->cp15.c2_base1 = qemu_get_be32(f);
+    env->cp15.c2_base0 = qemu_get_be64(f);
+    env->cp15.c2_base1 = qemu_get_be64(f);
     env->cp15.c2_control = qemu_get_be32(f);
-    env->cp15.c2_mask = qemu_get_be32(f);
-    env->cp15.c2_base_mask = qemu_get_be32(f);
     env->cp15.c2_data = qemu_get_be32(f);
     env->cp15.c2_insn = qemu_get_be32(f);
     env->cp15.c3 = qemu_get_be32(f);

@@ -160,7 +156,7 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
     }
     env->cp15.c6_insn = qemu_get_be32(f);
     env->cp15.c6_data = qemu_get_be32(f);
-    env->cp15.c7_par = qemu_get_be32(f);
+    env->cp15.c7_par = qemu_get_be64(f);
     env->cp15.c9_insn = qemu_get_be32(f);
     env->cp15.c9_data = qemu_get_be32(f);
     env->cp15.c9_pmcr = qemu_get_be32(f);

Alexey Starikovskiy

author

2012-03-14 13:01 UTC #1 lore raw

PATCH v2 2/3 Support for MRCC and MCRR instructions

Signed-off-by Alexey Starikovskiy<[email protected]>

---
 target-arm/helper.c    |   28 ++++++++++++++++++++++++++++
 target-arm/helper.h    |    2 ++
 target-arm/translate.c |   47 +++++++++++++++++++++++++++++------------------
 3 files changed, 59 insertions(+), 18 deletions(-)

target-arm/helper.c +28 -0

index d190104..3c4c0e4 100644
@@ -670,6 +670,16 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t insn)
     cpu_abort(env, "cp15 insn %08x\n", insn);
 }
 
+void HELPER(set_cp15_64)(CPUState * env, uint32_t insn, uint64_t val)
+{
+    cpu_abort(env, "cp15 insn %08x\n", insn);
+}
+
+uint64_t HELPER(get_cp15_64)(CPUState * env, uint32_t insn)
+{
+    cpu_abort(env, "cp15 insn %08x\n", insn);
+}
+
 /* These should probably raise undefined insn exceptions.  */
 void HELPER(v7m_msr)(CPUState *env, uint32_t reg, uint32_t val)
 {

@@ -2261,6 +2271,24 @@ bad_reg:
     return 0;
 }
 
+void HELPER(set_cp15_64)(CPUState *env, uint32_t insn, uint64_t val)
+{
+    int crm = insn&  0xf;
+    int opc1 = (insn>>  4)&  0xf;
+    cpu_abort(env, "Unimplemented cp15 register 64bit write (c%d[%d])\n",
+              crm, opc1);
+}
+
+uint64_t HELPER(get_cp15_64)(CPUState *env, uint32_t insn)
+{
+    /* Used for block cache operations, so just return 0 */
+#if 0
+    cpu_abort(env, "Unimplemented cp15 register 64bit read (c%d[%d])\n",
+              crm, opc1);
+#endif
+    return 0;
+}
+
 void HELPER(set_r13_banked)(CPUState *env, uint32_t mode, uint32_t val)
 {
     if ((env->uncached_cpsr&  CPSR_M) == mode) {

target-arm/helper.h +2 -0

index 16dd5fc..bc8151c 100644
@@ -60,7 +60,9 @@ DEF_HELPER_3(v7m_msr, void, env, i32, i32)
 DEF_HELPER_2(v7m_mrs, i32, env, i32)
 
 DEF_HELPER_3(set_cp15, void, env, i32, i32)
+DEF_HELPER_3(set_cp15_64, void, env, i32, i64)
 DEF_HELPER_2(get_cp15, i32, env, i32)
+DEF_HELPER_2(get_cp15_64, i64, env, i32)
 
 DEF_HELPER_3(set_cp, void, env, i32, i32)
 DEF_HELPER_2(get_cp, i32, env, i32)

target-arm/translate.c +29 ~~-18~~

62 lines not on this page · raw message

Alexey Starikovskiy

author

2012-03-14 13:02 UTC #2 lore raw

PATCH v2 3/3 Minimal ARM LPAE support

In reply to Alexey Starikovskiy

Sufficient to boot Linux kernel on vexpress-a15

Missing:
* Extends the DBGDRAR and DBGDSAR to 64 bits, to hold PAs of up to 40 bits.
* Defines two Memory Attribute Indirection Registers, MAIRn, to replace PRRR and NMRR when using the Long-descriptor translation table format.
* Provides two IMPLEMENTATION DEFINED Auxiliary Memory Attribute Indirection Registers 0 AMAIRn.

Does not change behaviour of Short-descriptor format (AKA _v6)

Signed-off-by Alexey Starikovskiy<[email protected]>

---
 target-arm/cpu.h    |    1
 target-arm/helper.c |  231 +++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 221 insertions(+), 11 deletions(-)

target-arm/cpu.h +1 -0

index 0298a98..1d87c7e 100644
@@ -381,6 +381,7 @@ enum arm_features {
     ARM_FEATURE_ARM_DIV, /* divide supported in ARM encoding */
     ARM_FEATURE_VFP4, /* VFPv4 (implies that NEON is v2) */
     ARM_FEATURE_GENERIC_TIMER,
+    ARM_FEATURE_LPAE, /* Large Physical Address Extension */
 };
 
 static inline int arm_feature(CPUARMState *env, int feature)

target-arm/helper.c +220 ~~-11~~

311 lines not on this page · raw message

Michael Roth

2012-03-14 18:57 UTC #3 lore raw

In reply to Alexey Starikovskiy

Quoted 112 lines · Alexey Starikovskiy

Signed-off-by Alexey Starikovskiy<[email protected]>

---
 target-arm/cpu.h     |   10 ++++------
 target-arm/helper.c  |   14 +++++++-------
 target-arm/machine.c |   16 ++++++----------
 3 files changed, 17 insertions(+), 23 deletions(-)

target-arm/cpu.h +4 -6

index 0d9b39c..0298a98 100644
@@ -117,11 +117,9 @@ typedef struct CPUARMState {
         uint32_t c1_coproc; /* Coprocessor access register.  */
         uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.  */
         uint32_t c1_scr; /* secure config register.  */
-        uint32_t c2_base0; /* MMU translation table base 0.  */
-        uint32_t c2_base1; /* MMU translation table base 1.  */
+        uint64_t c2_base0; /* MMU translation table base 0.  */
+        uint64_t c2_base1; /* MMU translation table base 1.  */
         uint32_t c2_control; /* MMU translation table base control.  */
-        uint32_t c2_mask; /* MMU translation table base selection mask.  */
-        uint32_t c2_base_mask; /* MMU translation table base 0 mask. */
         uint32_t c2_data; /* MPU data cachable bits.  */
         uint32_t c2_insn; /* MPU instruction cachable bits.  */
         uint32_t c3; /* MMU domain access control register

@@ -131,7 +129,7 @@ typedef struct CPUARMState {
         uint32_t c6_region[8]; /* MPU base/size registers.  */
         uint32_t c6_insn; /* Fault address registers.  */
         uint32_t c6_data;
-        uint32_t c7_par;  /* Translation result. */
+        uint64_t c7_par;  /* Translation result. */
         uint32_t c9_insn; /* Cache lockdown registers.  */
         uint32_t c9_data;
         uint32_t c9_pmcr; /* performance monitor control register */

@@ -455,7 +453,7 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
 #define cpu_signal_handler cpu_arm_signal_handler
 #define cpu_list arm_cpu_list
 
-#define CPU_SAVE_VERSION 6
+#define CPU_SAVE_VERSION 7
 
 /* MMU modes definitions */
 #define MMU_MODE0_SUFFIX _kernel

target-arm/helper.c +7 -7

index abe1c30..d190104 100644
@@ -325,7 +325,6 @@ void cpu_reset(CPUARMState *env)
         }
     }
     env->vfp.xregs[ARM_VFP_FPEXC] = 0;
-    env->cp15.c2_base_mask = 0xffffc000u;
     /* v7 performance monitor control register: same implementor
      * field as main ID register, and we implement no event counters.
      */

@@ -1050,12 +1049,15 @@ static inline int check_ap(CPUState *env, int ap, int domain_prot,
 static uint32_t get_level1_table_address(CPUState *env, uint32_t address)
 {
     uint32_t table;
+    int t0size = env->cp15.c2_control&  0x7;
+    uint32_t mask = ~(((uint32_t)0xffffffffu)>>  t0size);
 
-    if (address&  env->cp15.c2_mask)
+    if (address&  mask) {
         table = env->cp15.c2_base1&  0xffffc000;
-    else
-        table = env->cp15.c2_base0&  env->cp15.c2_base_mask;
-
+    } else {
+        mask = ~((uint32_t)0x3fffu>>  t0size);
+        table = env->cp15.c2_base0&  mask;
+    }
     table |= (address>>  18)&  0x3ffc;
     return table;
 }

@@ -1531,8 +1533,6 @@ void HELPER(set_cp15)(CPUState *env, uint32_t insn, uint32_t val)
 	    case 2:
                 val&= 7;
                 env->cp15.c2_control = val;
-		env->cp15.c2_mask = ~(((uint32_t)0xffffffffu)>>  val);
-                env->cp15.c2_base_mask = ~((uint32_t)0x3fffu>>  val);
 		break;
 	    default:
 		goto bad_reg;

target-arm/machine.c +3 -5

index f66b8df..8fa738e 100644
@@ -27,11 +27,9 @@ void cpu_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, env->cp15.c1_coproc);
     qemu_put_be32(f, env->cp15.c1_xscaleauxcr);
     qemu_put_be32(f, env->cp15.c1_scr);
-    qemu_put_be32(f, env->cp15.c2_base0);
-    qemu_put_be32(f, env->cp15.c2_base1);
+    qemu_put_be64(f, env->cp15.c2_base0);
+    qemu_put_be64(f, env->cp15.c2_base1);
     qemu_put_be32(f, env->cp15.c2_control);
-    qemu_put_be32(f, env->cp15.c2_mask);
-    qemu_put_be32(f, env->cp15.c2_base_mask);
     qemu_put_be32(f, env->cp15.c2_data);
     qemu_put_be32(f, env->cp15.c2_insn);
     qemu_put_be32(f, env->cp15.c3);

@@ -42,7 +40,7 @@ void cpu_save(QEMUFile *f, void *opaque)
     }
     qemu_put_be32(f, env->cp15.c6_insn);
     qemu_put_be32(f, env->cp15.c6_data);
-    qemu_put_be32(f, env->cp15.c7_par);
+    qemu_put_be64(f, env->cp15.c7_par);
     qemu_put_be32(f, env->cp15.c9_insn);
     qemu_put_be32(f, env->cp15.c9_data);
     qemu_put_be32(f, env->cp15.c9_pmcr);

target-arm/machine.c int cpu_load(QEMUFile *f, void *opaque, int version_id) · line 147

Quoted 7 lines

target-arm/machine.c +2 -2

@@ -145,11 +143,9 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
     env->cp15.c1_coproc = qemu_get_be32(f);
     env->cp15.c1_xscaleauxcr = qemu_get_be32(f);
     env->cp15.c1_scr = qemu_get_be32(f);
-    env->cp15.c2_base0 = qemu_get_be32(f);
-    env->cp15.c2_base1 = qemu_get_be32(f);
+    env->cp15.c2_base0 = qemu_get_be64(f);
+    env->cp15.c2_base1 = qemu_get_be64(f);

This breaks old -> new migration. Should CPU_SAVE_VERSION a do this conditionally on version_id >= CPU_SAVE_VERSION

target-arm/machine.c int cpu_load(QEMUFile *f, void *opaque, int version_id) · line 159

Quoted 12 lines

target-arm/machine.c +1 -3

     env->cp15.c2_control = qemu_get_be32(f);
-    env->cp15.c2_mask = qemu_get_be32(f);
-    env->cp15.c2_base_mask = qemu_get_be32(f);
     env->cp15.c2_data = qemu_get_be32(f);
     env->cp15.c2_insn = qemu_get_be32(f);
     env->cp15.c3 = qemu_get_be32(f);

@@ -160,7 +156,7 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
     }
     env->cp15.c6_insn = qemu_get_be32(f);
     env->cp15.c6_data = qemu_get_be32(f);
-    env->cp15.c7_par = qemu_get_be32(f);
+    env->cp15.c7_par = qemu_get_be64(f);

Here too.

Quoted 6 lines

target-arm/machine.c +0 -0

     env->cp15.c9_insn = qemu_get_be32(f);
     env->cp15.c9_data = qemu_get_be32(f);
     env->cp15.c9_pmcr = qemu_get_be32(f);

Michael Roth

2012-03-14 19:06 UTC #4 lore raw

Quoted 115 lines · Michael Roth

Quoted 114 lines · Alexey Starikovskiy

Signed-off-by Alexey Starikovskiy<[email protected]>

---
 target-arm/cpu.h     |   10 ++++------
 target-arm/helper.c  |   14 +++++++-------
 target-arm/machine.c |   16 ++++++----------
 3 files changed, 17 insertions(+), 23 deletions(-)

target-arm/cpu.h +4 -6

index 0d9b39c..0298a98 100644
@@ -117,11 +117,9 @@ typedef struct CPUARMState {
         uint32_t c1_coproc; /* Coprocessor access register.  */
         uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.  */
         uint32_t c1_scr; /* secure config register.  */
-        uint32_t c2_base0; /* MMU translation table base 0.  */
-        uint32_t c2_base1; /* MMU translation table base 1.  */
+        uint64_t c2_base0; /* MMU translation table base 0.  */
+        uint64_t c2_base1; /* MMU translation table base 1.  */
         uint32_t c2_control; /* MMU translation table base control.  */
-        uint32_t c2_mask; /* MMU translation table base selection mask.  */
-        uint32_t c2_base_mask; /* MMU translation table base 0 mask. */
         uint32_t c2_data; /* MPU data cachable bits.  */
         uint32_t c2_insn; /* MPU instruction cachable bits.  */
         uint32_t c3; /* MMU domain access control register

@@ -131,7 +129,7 @@ typedef struct CPUARMState {
         uint32_t c6_region[8]; /* MPU base/size registers.  */
         uint32_t c6_insn; /* Fault address registers.  */
         uint32_t c6_data;
-        uint32_t c7_par;  /* Translation result. */
+        uint64_t c7_par;  /* Translation result. */
         uint32_t c9_insn; /* Cache lockdown registers.  */
         uint32_t c9_data;
         uint32_t c9_pmcr; /* performance monitor control register */

@@ -455,7 +453,7 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
 #define cpu_signal_handler cpu_arm_signal_handler
 #define cpu_list arm_cpu_list
 
-#define CPU_SAVE_VERSION 6
+#define CPU_SAVE_VERSION 7
 
 /* MMU modes definitions */
 #define MMU_MODE0_SUFFIX _kernel

target-arm/helper.c +7 -7

index abe1c30..d190104 100644
@@ -325,7 +325,6 @@ void cpu_reset(CPUARMState *env)
         }
     }
     env->vfp.xregs[ARM_VFP_FPEXC] = 0;
-    env->cp15.c2_base_mask = 0xffffc000u;
     /* v7 performance monitor control register: same implementor
      * field as main ID register, and we implement no event counters.
      */

@@ -1050,12 +1049,15 @@ static inline int check_ap(CPUState *env, int ap, int domain_prot,
 static uint32_t get_level1_table_address(CPUState *env, uint32_t address)
 {
     uint32_t table;
+    int t0size = env->cp15.c2_control&  0x7;
+    uint32_t mask = ~(((uint32_t)0xffffffffu)>>  t0size);
 
-    if (address&  env->cp15.c2_mask)
+    if (address&  mask) {
         table = env->cp15.c2_base1&  0xffffc000;
-    else
-        table = env->cp15.c2_base0&  env->cp15.c2_base_mask;
-
+    } else {
+        mask = ~((uint32_t)0x3fffu>>  t0size);
+        table = env->cp15.c2_base0&  mask;
+    }
     table |= (address>>  18)&  0x3ffc;
     return table;
 }

@@ -1531,8 +1533,6 @@ void HELPER(set_cp15)(CPUState *env, uint32_t insn, uint32_t val)
 	    case 2:
                 val&= 7;
                 env->cp15.c2_control = val;
-		env->cp15.c2_mask = ~(((uint32_t)0xffffffffu)>>  val);
-                env->cp15.c2_base_mask = ~((uint32_t)0x3fffu>>  val);
 		break;
 	    default:
 		goto bad_reg;

target-arm/machine.c +3 -5

index f66b8df..8fa738e 100644
@@ -27,11 +27,9 @@ void cpu_save(QEMUFile *f, void *opaque)
     qemu_put_be32(f, env->cp15.c1_coproc);
     qemu_put_be32(f, env->cp15.c1_xscaleauxcr);
     qemu_put_be32(f, env->cp15.c1_scr);
-    qemu_put_be32(f, env->cp15.c2_base0);
-    qemu_put_be32(f, env->cp15.c2_base1);
+    qemu_put_be64(f, env->cp15.c2_base0);
+    qemu_put_be64(f, env->cp15.c2_base1);
     qemu_put_be32(f, env->cp15.c2_control);
-    qemu_put_be32(f, env->cp15.c2_mask);
-    qemu_put_be32(f, env->cp15.c2_base_mask);
     qemu_put_be32(f, env->cp15.c2_data);
     qemu_put_be32(f, env->cp15.c2_insn);
     qemu_put_be32(f, env->cp15.c3);

@@ -42,7 +40,7 @@ void cpu_save(QEMUFile *f, void *opaque)
     }
     qemu_put_be32(f, env->cp15.c6_insn);
     qemu_put_be32(f, env->cp15.c6_data);
-    qemu_put_be32(f, env->cp15.c7_par);
+    qemu_put_be64(f, env->cp15.c7_par);
     qemu_put_be32(f, env->cp15.c9_insn);
     qemu_put_be32(f, env->cp15.c9_data);
     qemu_put_be32(f, env->cp15.c9_pmcr);

@@ -145,11 +143,9 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
     env->cp15.c1_coproc = qemu_get_be32(f);
     env->cp15.c1_xscaleauxcr = qemu_get_be32(f);

Quoted 8 lines

target-arm/machine.c int cpu_load(QEMUFile *f, void *opaque, int version_id) · line 147
Quoted 5 lines
target-arm/machine.c +2 -2
     env->cp15.c1_scr = qemu_get_be32(f);
-    env->cp15.c2_base0 = qemu_get_be32(f);
-    env->cp15.c2_base1 = qemu_get_be32(f);
+    env->cp15.c2_base0 = qemu_get_be64(f);
+    env->cp15.c2_base1 = qemu_get_be64(f);
This breaks old -> new migration. Should CPU_SAVE_VERSION a do this conditionally on version_id >= CPU_SAVE_VERSION

Heh, let's try that again. Should *increment* CPU_SAVE_VERSION *and*

Also, historically I guess arm just bails on old->new but that seems wrong. Either we need to increment though.

Quoted 22 lines

target-arm/machine.c int cpu_load(QEMUFile *f, void *opaque, int version_id) · line 159

Quoted 13 lines

target-arm/machine.c +1 -3

     env->cp15.c2_control = qemu_get_be32(f);
-    env->cp15.c2_mask = qemu_get_be32(f);
-    env->cp15.c2_base_mask = qemu_get_be32(f);
     env->cp15.c2_data = qemu_get_be32(f);
     env->cp15.c2_insn = qemu_get_be32(f);
     env->cp15.c3 = qemu_get_be32(f);

@@ -160,7 +156,7 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
     }
     env->cp15.c6_insn = qemu_get_be32(f);
     env->cp15.c6_data = qemu_get_be32(f);
-    env->cp15.c7_par = qemu_get_be32(f);
+    env->cp15.c7_par = qemu_get_be64(f);

Here too.

Quoted 6 lines

target-arm/machine.c +0 -0

     env->cp15.c9_insn = qemu_get_be32(f);
     env->cp15.c9_data = qemu_get_be32(f);
     env->cp15.c9_pmcr = qemu_get_be32(f);

Alexey Starikovskiy

author

2012-03-14 19:09 UTC #5 lore raw

Do I need to do anything beside following or not?

Thanks,
Alex.

Quoted 2 lines · Michael Roth

Quoted 2 lines
Quoted 2 lines
target-arm/cpu.h +1 -1
-#define CPU_SAVE_VERSION 6
+#define CPU_SAVE_VERSION 7

Michael Roth

2012-03-14 19:39 UTC #6 lore raw

Quoted 1 line · Alexey Starikovskiy

Do I need to do anything beside following or not?

Not sure, ARM folks?

My suggestion would be to lose the the catch-all -EINVAL error we throw in machine.c:cpu_load() when version_id != CPU_SAVE_VERSION and only conditionally load 64-bit registers if the source's version_id >= CPU_SAVE_VERSION. But we've never done that in the past, and we broke old->new as recently as 2 months ago so I'm not sure it's worth it since there hasn't been a release since then.

Quoted 9 lines

Thanks,
Alex.
Quoted 2 lines · Michael Roth
Quoted 2 lines
Quoted 2 lines
target-arm/cpu.h +1 -1
-#define CPU_SAVE_VERSION 6
+#define CPU_SAVE_VERSION 7

Peter Maydell

2012-03-14 20:38 UTC #7 lore raw

Quoted 11 lines · Michael Roth

Quoted 1 line · Alexey Starikovskiy
Do I need to do anything beside following or not?
Not sure, ARM folks?
My suggestion would be to lose the the catch-all -EINVAL error we throw in machine.c:cpu_load() when version_id != CPU_SAVE_VERSION and only conditionally load 64-bit registers if the source's version_id >= CPU_SAVE_VERSION. But we've never done that in the past, and we broke old->new as recently as 2 months ago so I'm not sure it's worth it since there hasn't been a release since then.

My position at the moment is that old->new migration on ARM is not supported and further that we make no attempt to avoid version bumps. At some point as KVM-on-ARM gets towards being complete we'll have to start worrying about compatibility, but at the moment I really don't think that either (a) all the devices have tested and working migration state or (b) we have a CPUState that's actually in good enough shape[*] that we can reasonably start to insist on not breaking cross-version migration.

So migration-wise I have no problem with the patch as it stands. (I have other issues with it I suspect but haven't got to reviewing it yet.)

[*] as an example of the kind of problem I'd like to see a solution for, there's no reason that adding support for a new feature like LPAE should require the migration state for non-LPAE CPUs to change, but as things stand it does.

-- PMM