target-arm: KVM to TCG migration

The method is not behaving in the way it's supposed to. It should return the new value only if it's less than the actual one.

Signed-off-by Alvise Rigo <[email protected]>

---
 vmstate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

vmstate.c +0 -0

index 284b080..038b274 100644

vmstate.c +1 -1

-    if (*old <= new) {
+    if (*new <= old) {
         return 0;
     }
     return -EINVAL;

Signature · 1 line

1.8.3.2

The method is not behaving in the way it's supposed to. It should return the new value only if it's less than the actual one.

Signed-off-by Alvise Rigo <[email protected]>

The method is not behaving in the way it's supposed to. It should return the new value only if it's less than the actual one.

Signed-off-by Alvise Rigo <[email protected]>

---
 vmstate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

vmstate.c +4 -4

index 284b080..038b274 100644
@@ -326,11 +326,11 @@ const VMStateInfo vmstate_info_int32_equal = {
 
 static int get_int32_le(QEMUFile *f, void *pv, size_t size)
 {
-    int32_t *old = pv;
-    int32_t new;
-    qemu_get_sbe32s(f, &new);
+    int32_t old = *(int32_t *)pv;
+    int32_t *new = pv;
+    qemu_get_sbe32s(f, new);
 
-    if (*old <= new) {
+    if (*new <= old) {
         return 0;
     }
     return -EINVAL;

BTW, did you find this bug by testing or by code inspection?

The value of this flag indicates the execution mode of the CPU prior the migration. It is used to handle the KVM <-> TCG migration.

Signed-off-by Alvise Rigo <[email protected]>

---
 target-arm/cpu-qom.h | 3 +++
 1 file changed, 3 insertions(+)

target-arm/cpu-qom.h +0 -0

index afbd422..6819bfc 100644

Quoted 13 lines · Alvise Rigo

The value of this flag indicates the execution mode of the CPU prior the migration. It is used to handle the KVM <-> TCG migration.

Signed-off-by Alvise Rigo <[email protected]>

---
 target-arm/cpu-qom.h | 3 +++
 1 file changed, 3 insertions(+)

target-arm/cpu-qom.h +0 -0

index afbd422..6819bfc 100644

target-arm/cpu-qom.h typedef struct ARMCPU { · line 105

Quoted 4 lines

target-arm/cpu-qom.h +1 -0

@@ -102,6 +102,9 @@ typedef struct ARMCPU {
      */
     uint32_t kvm_target;
 
+    /* true if this cpu is using KVM. Read and set in cpu_pre/post_load

*/

target-arm/cpu-qom.h typedef struct ARMCPU { · line 106

Quoted 1 line

target-arm/cpu-qom.h +1 -0

+    bool running_kvm;

This is definitely wrong. We should not care whether either end of the migration connection is using KVM.

thanks
-- PMM

I see, but how can we know the type of migration (KVM to TCG or TCG to KVM) which is taking place only looking at the incoming data? Of course, I'm supposing that the two cases will require different handling and so that distinguish the migration type (at the destination) has to be part of the process.

Since the irq bit seems to not be updated, exclude it from the check done while copying data during migration.

Signed-off-by Alvise Rigo <[email protected]>

---
 target-arm/cpu.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

target-arm/cpu.c +15 -2

index 6e7ce89..e8db00e 100644
@@ -681,20 +681,34 @@ static void cortex_a9_initfn(Object *obj)
 }
 
 #ifndef CONFIG_USER_ONLY
-static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+static void a15_l2ctlr_reset(CPUARMState *env, const ARMCPRegInfo *opaque)
 {
     /* Linux wants the number of processors from here.
      * Might as well set the interrupt-controller bit too.
      */
-    return ((smp_cpus - 1) << 24) | (1 << 23);
+    env->cp15.c9_l2ctlr = ((smp_cpus - 1) << 24) | (1 << 23);
+}
+
+static void a15_l2ctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                           uint64_t value)
+{
+    int smp_cpus_new = ((value >> 24) & 3);
+    int smp_cpus_old = ((env->cp15.c9_l2ctlr >> 24) & 3);
+
+    if (smp_cpus_new != smp_cpus_old) {
+        return;
+    }
+
+    env->cp15.c9_l2ctlr = value;
 }
 #endif

Quoted 13 lines · Alvise Rigo

Since the irq bit seems to not be updated, exclude it from the check done while copying data during migration.

Signed-off-by Alvise Rigo <[email protected]>

---
 target-arm/cpu.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

target-arm/cpu.c +0 -0

index 6e7ce89..e8db00e 100644

target-arm/cpu.c static void cortex_a9_initfn(Object *obj) · line 684

Quoted 4 lines

target-arm/cpu.c +0 -1

@@ -681,20 +681,34 @@ static void cortex_a9_initfn(Object *obj)
 }
 
 #ifndef CONFIG_USER_ONLY
-static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo

*ri)

target-arm/cpu.c static void cortex_a9_initfn(Object *obj) · line 684

Quoted 1 line

target-arm/cpu.c +1 -0

+static void a15_l2ctlr_reset(CPUARMState *env, const ARMCPRegInfo

*opaque)

Quoted 15 lines

target-arm/cpu.c +10 -1

 {
     /* Linux wants the number of processors from here.
      * Might as well set the interrupt-controller bit too.
      */
-    return ((smp_cpus - 1) << 24) | (1 << 23);
+    env->cp15.c9_l2ctlr = ((smp_cpus - 1) << 24) | (1 << 23);
+}
+
+static void a15_l2ctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                           uint64_t value)
+{
+    int smp_cpus_new = ((value >> 24) & 3);
+    int smp_cpus_old = ((env->cp15.c9_l2ctlr >> 24) & 3);
+
+    if (smp_cpus_new != smp_cpus_old) {

target-arm/cpu.c static void cortex_a9_initfn(Object *obj)

Quoted 10 lines

target-arm/cpu.c +4 -0

+        return;
+    }
+
+    env->cp15.c9_l2ctlr = value;
 }
 #endif
 
 static const ARMCPRegInfo cortexa15_cp_reginfo[] = {
 #ifndef CONFIG_USER_ONLY
     { .name = "L2CTLR", .cp = 15, .crn = 9, .crm = 0, .opc1 = 1, .opc2

= 2,

target-arm/cpu.c static void cortex_a9_initfn(Object *obj) · line 712

Quoted 6 lines

target-arm/cpu.c +3 -2

-      .access = PL1_RW, .resetvalue = 0, .readfn = a15_l2ctlr_read,
-      .writefn = arm_cp_write_ignore, },
+      .access = PL1_RW, .resetvalue = 0,
+      .resetfn = a15_l2ctlr_reset, .writefn = a15_l2ctlr_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_l2ctlr) },
 #endif

Why are you changing the behaviour of this register from read-only/writes-ignored to permitting writes? This looks wrong.

thanks
-- PMM

This was a polite way to alter the value of the register in order to not fail the
migration when KVM migrates a value different from the TCG one. KVM in particular does not set the irq bit while TCG does.

CPUARMState:
* added adfsr cp register.
* added aifsr cp register.
These registers have been added because they are migrated by KVM. This prevents the migration from failing when trying to copy their values.

ARMCPRegInfo:
* added a pointer to the parent that generated the register (if any).
* a flag to inform that we have already copied the value of the register:
this prevents the register from being overwritten by the parent register value, which could be not set.

helper.c:
* added mechanism to track the cp register "parent".
* compare_cpreg_array(): compare the incoming cp registers with the cpreg_list keeping a list of the registers that do not succeeded the match.
* handle_cpreg_kvm2tcg_migration(): try to solve the mismatch of cp registers coming from KVM; without this additional step the migration would fail even if it's feasible.

Signed-off-by Alvise Rigo <[email protected]>

---
 target-arm/cpu.h     |  43 +++++++++++++
 target-arm/helper.c  | 167 +++++++++++++++++++++++++++++++++++++++++++++++++--
 target-arm/machine.c |  46 ++++++++++----
 3 files changed, 238 insertions(+), 18 deletions(-)

target-arm/cpu.h +3 -0

index 3c8a2db..a97246d 100644
@@ -184,6 +184,8 @@ typedef struct CPUARMState {
                         MPU write buffer control.  */
         uint32_t c5_insn; /* Fault status registers.  */
         uint32_t c5_data;
+        uint32_t c5_adfsr;
+        uint32_t c5_aifsr;
         uint32_t c6_region[8]; /* MPU base/size registers.  */
         uint32_t c6_insn; /* Fault address registers.  */
         uint32_t c6_data;

@@ -197,6 +199,7 @@ typedef struct CPUARMState {
         uint32_t c9_pmxevtyper; /* perf monitor event type */
         uint32_t c9_pmuserenr; /* perf monitor user enable */
         uint32_t c9_pminten; /* perf monitor interrupt enables */
+        uint32_t c9_l2ctlr; /* L2 Control Register */
         uint32_t c12_vbar; /* vector base address register */
         uint32_t c13_fcse; /* FCSE PID.  */
         uint32_t c13_context; /* Context ID.  */

@@ -867,6 +870,15 @@ struct ARMCPRegInfo {
     uint8_t opc0;

Quoted 4 lines · Alvise Rigo

CPUARMState:
* added adfsr cp register.
* added aifsr cp register.
These registers have been added because they are migrated by KVM. This

prevents

Quoted 1 line

the migration from failing when trying to copy their values.

This should be done in a separate patch.

Quoted 4 lines

ARMCPRegInfo:
* added a pointer to the parent that generated the register (if any).
* a flag to inform that we have already copied the value of the register:
this prevents the register from being overwritten by the parent

register

Quoted 11 lines

value, which could be not set.

helper.c:
* added mechanism to track the cp register "parent".
* compare_cpreg_array(): compare the incoming cp registers with the cpreg_list keeping a list of the registers that do not succeeded the match.
* handle_cpreg_kvm2tcg_migration(): try to solve the mismatch of cp registers coming from KVM; without this additional step the migration would fail even if it's feasible.

Quoted 4 lines

Signed-off-by Alvise Rigo <[email protected]>

---
 target-arm/cpu.h     |  43 +++++++++++++
 target-arm/helper.c  | 167

+++++++++++++++++++++++++++++++++++++++++++++++++--

Quoted 25 lines

 target-arm/machine.c |  46 ++++++++++----
 3 files changed, 238 insertions(+), 18 deletions(-)

target-arm/cpu.h +3 -0

index 3c8a2db..a97246d 100644
@@ -184,6 +184,8 @@ typedef struct CPUARMState {
                         MPU write buffer control.  */
         uint32_t c5_insn; /* Fault status registers.  */
         uint32_t c5_data;
+        uint32_t c5_adfsr;
+        uint32_t c5_aifsr;
         uint32_t c6_region[8]; /* MPU base/size registers.  */
         uint32_t c6_insn; /* Fault address registers.  */
         uint32_t c6_data;

@@ -197,6 +199,7 @@ typedef struct CPUARMState {
         uint32_t c9_pmxevtyper; /* perf monitor event type */
         uint32_t c9_pmuserenr; /* perf monitor user enable */
         uint32_t c9_pminten; /* perf monitor interrupt enables */
+        uint32_t c9_l2ctlr; /* L2 Control Register */
         uint32_t c12_vbar; /* vector base address register */
         uint32_t c13_fcse; /* FCSE PID.  */
         uint32_t c13_context; /* Context ID.  */

target-arm/cpu.h struct ARMCPRegInfo { · line 873

Quoted 4 lines

target-arm/cpu.h +1 -0

@@ -867,6 +870,15 @@ struct ARMCPRegInfo {
     uint8_t opc0;
     uint8_t opc1;
     uint8_t opc2;
+    /* When migrating this flag tells if the register's value has

already

target-arm/cpu.h struct ARMCPRegInfo { · line 874

Quoted 1 line

target-arm/cpu.h +1 -0

+     * been copied. This is used in some unlikely cases where KVM

migrates

target-arm/cpu.h struct ARMCPRegInfo { · line 880

Quoted 6 lines

target-arm/cpu.h +6 -0

+     * a register that in TCG is generated by a wildcarded or
+     * ARM_CP_STATE_BOTH parent (hence a not migratable register);
+     * in those cases, we will set the field "parent" to point to the
+     * generating register. Most of the time this field can stay false.
+     * */
+    bool skip_cpreglist;

This reads to me like it's papering over a problem rather than fixing it.

thanks
-- PMM

I agree that this is a sort of workaround, but it seems to me that a proper solution is not possible without changing the ideas contemplated now in the migration code.
Are we willing to accept some major changes in the code to embrace this type of migration?

Quoted 5 lines · alvise rigo

I agree that this is a sort of workaround, but it seems to me that a proper solution is not possible without changing the ideas contemplated now in the migration code.
Are we willing to accept some major changes in the code to embrace this type of migration?

Well, the migration code as it stands is in theory supposed to cope with KVM<->TCG migration. I would expect that we'd need to improve a number of places where our TCG cpreg emulation is wrong.

thanks
-- PMM

Improvements of cpreg emulation apart, we will still need to tackle with the great amount of registers not migrated because generated by a wildcarded register; some of these registers are in fact migrated by KVM. I think that the idea of keeping a reference
to the generator register (i.e. the only register which is migrated in a wildcarded case) could be considered to handle the migration.

I agree that this is a sort of workaround, but it seems to me that a proper solution is not possible without changing the ideas contemplated now in the migration code.
Are we willing to accept some major changes in the code to embrace this type of migration?

Quoted 5 lines · alvise rigo

I agree that this is a sort of workaround, but it seems to me that a proper solution is not possible without changing the ideas contemplated now in the migration code.
Are we willing to accept some major changes in the code to embrace this type of migration?

Incidentally it occurred to me recently that the design we've gone for for AArch64 support (canonical copy of system register info is the AArch64 view, AArch32 register encoding is a non-migratable view onto the same underlying data) clashes rather with the idea of being able to migrate AArch32 TCG<->KVM. (Migrating AArch32 TCG to/from a KVM which is configured to run the VM in AArch32 on an AArch64 host doesn't run into the same problems, because in that case what KVM exposes to userspace will also be the AArch64 views of registers.)
That was accidental, not an intentional tradeoff, but I'm not entirely sure how to reconcile things...

thanks
-- PMM

So, if the processor does not start in AArch64 mode, we only add the AArch32 version of the ARM_CP_STATE_BOTH register to the hashtable, otherwise nothing changes and we add the two views of the register.

This approach works only
if all the 64bit CPUs will always start in 64 bit mode.

In my opinion, other solutions would require to revisit the idea of ARM_CP_STATE_BOTH in favour of distinct definitions of the registers with multiple views (like VBAR for AArch32 and VBAR_EL1 for AArch64).