Re: [Qemu-devel] [PATCH v3 2/7] qom/cpu: move register_vmstate to common CPUClass.realizefn

[Chen Fan <chen.fan.fnst@cn.fujitsu.com>]

[-- Attachment #1: Type: text/plain, Size: 4608 bytes --]


On 01/29/2015 10:04 PM, Igor Mammedov wrote:
> On Wed, 14 Jan 2015 15:27:25 +0800
> Zhu Guihua <zhugh.fnst@cn.fujitsu.com> wrote:
>
>> From: Gu Zheng <guz.fnst@cn.fujitsu.com>
>>
>> Move cpu vmstate register from cpu_exec_init into cpu_common_realizefn,
>> and use cc->get_arch_id as the instance id that suggested by Igor to
>> fix the migration issue.
>>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
>> ---
>>   exec.c            | 32 +++++++++++++++++++-------------
>>   include/qom/cpu.h |  2 ++
>>   qom/cpu.c         |  2 ++
>>   3 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 081818e..c9ffda6 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -513,10 +513,28 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
>>   }
>>   #endif
>>   
>> +void cpu_vmstate_register(CPUState *cpu)
>> +{
>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>> +    int cpu_index = cc->get_arch_id(cpu);
> that probable would be source migration problems:
> because cc->get_arch_id(cpu) depending on topology might
> be not sequential, for example: sockets=4,core=3
> that would create sparse APIC numbering.
>
> as result migration from old qemu to one with this change
> would be rejected due to vmsd id mismatch mismatch.
>
> we need a better way to handle cross version migration
> between old/new scheme.
Hi Igor,

     I think to handler cross version migration issue, we only
need to do is converting new 'apic-id' to old 'cpu_index' to match
vmsd id  between old to new scheme.
we can save old cpu_index in alias id. and in order to keep
instance_id differ from alias id. we can use apic_id + maxcpus
as the instance_id. so during migration we can find the corresponding
cpu with instance_id regardless new/old scheme.

I has made a patch and test migrating from old version to new version.
it seems work fine. pls have a look at the attach file. ;)

Thanks,
Chen

>> +
>> +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
>> +        vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
>> +    }
>> +#if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
> that ifdef block affects only sparc/mips/cris and builds target specific code
> while you are trying to call it from target independent qom/cpu.c
>
> I'd suggest leave it where it was or better move into respective
> targets realize_fns
>
>> +    register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
>> +                    cpu_save, cpu_load, cpu->env_ptr);
>> +    assert(cc->vmsd == NULL);
>> +    assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
>> +#endif
>> +    if (cc->vmsd != NULL) {
>> +        vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
>> +    }
>> +}
>> +
>>   void cpu_exec_init(CPUArchState *env)
>>   {
>>       CPUState *cpu = ENV_GET_CPU(env);
>> -    CPUClass *cc = CPU_GET_CLASS(cpu);
>>       CPUState *some_cpu;
>>       int cpu_index;
>>   
>> @@ -539,18 +557,6 @@ void cpu_exec_init(CPUArchState *env)
>>   #if defined(CONFIG_USER_ONLY)
>>       cpu_list_unlock();
>>   #endif
>> -    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
>> -        vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
>> -    }
>> -#if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
>> -    register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
>> -                    cpu_save, cpu_load, env);
>> -    assert(cc->vmsd == NULL);
>> -    assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
>> -#endif
>> -    if (cc->vmsd != NULL) {
>> -        vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
>> -    }
> And in general do CONFIG_USER_ONLY targets actually need/use
> migration code?
>
>>   }
>>   
>>   #if defined(TARGET_HAS_ICE)
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 2098f1c..936afcd 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -562,6 +562,8 @@ void cpu_interrupt(CPUState *cpu, int mask);
>>   
>>   #endif /* USER_ONLY */
>>   
>> +void cpu_vmstate_register(CPUState *cpu);
>> +
>>   #ifdef CONFIG_SOFTMMU
>>   static inline void cpu_unassigned_access(CPUState *cpu, hwaddr addr,
>>                                            bool is_write, bool is_exec,
>> diff --git a/qom/cpu.c b/qom/cpu.c
>> index 9c68fa4..a639822 100644
>> --- a/qom/cpu.c
>> +++ b/qom/cpu.c
>> @@ -302,6 +302,8 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>>   {
>>       CPUState *cpu = CPU(dev);
>>   
>> +    cpu_vmstate_register(cpu);
>> +
>>       if (dev->hotplugged) {
>>           cpu_synchronize_post_init(cpu);
>>           cpu_resume(cpu);
> .
>


[-- Attachment #2: fix-cross-version-impact.diff --]
[-- Type: text/x-patch, Size: 5032 bytes --]

diff --git a/exec.c b/exec.c
index bda96c6..6f3a90d 100644
--- a/exec.c
+++ b/exec.c
@@ -516,10 +516,12 @@ void tcg_cpu_address_space_init(CPUState *cpu, AddressSpace *as)
 void cpu_vmstate_register(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
-    int cpu_index = cc->get_arch_id(cpu);
+    int cpu_index = cc->get_arch_id(cpu) + max_cpus;
+    int compat_index = cc->get_compat_id(cpu);
 
     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
-        vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
+        vmstate_register_with_alias_id(NULL, cpu_index, &vmstate_cpu_common,
+                                       cpu, compat_index, 3);
     }
 #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
     register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
@@ -528,7 +530,8 @@ void cpu_vmstate_register(CPUState *cpu)
     assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
 #endif
     if (cc->vmsd != NULL) {
-        vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
+        vmstate_register_with_alias_id(NULL, cpu_index, cc->vmsd,
+                                       cpu, compat_index, 3);
     }
 }
 
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 0592b4d..3d0bcbf 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -130,6 +130,7 @@ typedef struct CPUClass {
     void (*dump_statistics)(CPUState *cpu, FILE *f,
                             fprintf_function cpu_fprintf, int flags);
     int64_t (*get_arch_id)(CPUState *cpu);
+    int64_t (*get_compat_id)(CPUState *cpu);
     bool (*get_paging_enabled)(const CPUState *cpu);
     void (*get_memory_mapping)(CPUState *cpu, MemoryMappingList *list,
                                Error **errp);
diff --git a/qom/cpu.c b/qom/cpu.c
index a639822..48dee41 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -323,6 +323,11 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
     return cpu->cpu_index;
 }
 
+static int64_t cpu_common_get_compat_id(CPUState *cpu)
+{
+    return cpu->cpu_index;
+}
+
 static void cpu_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -332,6 +337,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->parse_features = cpu_common_parse_features;
     k->reset = cpu_common_reset;
     k->get_arch_id = cpu_common_get_arch_id;
+    k->get_compat_id = cpu_common_get_compat_id;
     k->has_work = cpu_common_has_work;
     k->get_paging_enabled = cpu_common_get_paging_enabled;
     k->get_memory_mapping = cpu_common_get_memory_mapping;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e25789a..8c024b9 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2794,13 +2794,15 @@ static void x86_cpu_apic_realize(X86CPU *cpu, Error **errp)
 {
     DeviceState *apic_state = cpu->apic_state;
     CPUClass *cc = CPU_GET_CLASS(CPU(cpu));
+    int cpu_index = cc->get_arch_id(CPU(cpu)) + max_cpus;
+    int compat_index = cc->get_compat_id(CPU(cpu));
 
     if (apic_state == NULL) {
         return;
     }
 
-    vmstate_register(0, cc->get_arch_id(CPU(cpu)),
-                     &vmstate_apic_common, apic_state);
+    vmstate_register_with_alias_id(NULL, cpu_index, &vmstate_apic_common,
+                                   apic_state, compat_index, 3);
 
     if (qdev_init(cpu->apic_state)) {
         error_setg(errp, "APIC device '%s' could not be initialized",
@@ -3042,6 +3044,15 @@ static int64_t x86_cpu_get_arch_id(CPUState *cs)
     return env->cpuid_apic_id;
 }
 
+static int64_t x86_cpu_get_compat_id(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
+
+    return x86_compat_index_from_apic_id(smp_cores, smp_threads,
+                                         env->cpuid_apic_id);
+}
+
 static bool x86_cpu_get_paging_enabled(const CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
@@ -3122,6 +3133,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     cc->gdb_read_register = x86_cpu_gdb_read_register;
     cc->gdb_write_register = x86_cpu_gdb_write_register;
     cc->get_arch_id = x86_cpu_get_arch_id;
+    cc->get_compat_id = x86_cpu_get_compat_id;
     cc->get_paging_enabled = x86_cpu_get_paging_enabled;
 #ifdef CONFIG_USER_ONLY
     cc->handle_mmu_fault = x86_cpu_handle_mmu_fault;
diff --git a/target-i386/topology.h b/target-i386/topology.h
index dcb4988..42d1d07 100644
--- a/target-i386/topology.h
+++ b/target-i386/topology.h
@@ -150,4 +150,20 @@ static inline void x86_topo_ids_from_apic_id(unsigned nr_cores,
     topo->smt_id = apic_id & offset_mask;
 }
 
+static inline unsigned x86_compat_index_from_apic_id(unsigned nr_cores,
+                                                     unsigned nr_threads,
+                                                     apic_id_t apic_id)
+{
+    X86CPUTopoInfo topo;
+
+    x86_topo_ids_from_apic_id(nr_cores, nr_threads, apic_id, &topo);
+
+
+    return topo.pkg_id * nr_cores * nr_threads +
+           topo.core_id * nr_threads +
+           topo.smt_id;
+}
+
+
+
 #endif /* TARGET_I386_TOPOLOGY_H */