From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59267) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YKkDS-0005WL-CX for qemu-devel@nongnu.org; Mon, 09 Feb 2015 04:01:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YKkDN-000598-NZ for qemu-devel@nongnu.org; Mon, 09 Feb 2015 04:01:14 -0500 Received: from [59.151.112.132] (port=63974 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YKkDL-00058D-32 for qemu-devel@nongnu.org; Mon, 09 Feb 2015 04:01:09 -0500 Message-ID: <54D8701A.7030608@cn.fujitsu.com> Date: Mon, 9 Feb 2015 16:30:18 +0800 From: Chen Fan MIME-Version: 1.0 References: <20150129150444.6653ea36@nial.brq.redhat.com> In-Reply-To: <20150129150444.6653ea36@nial.brq.redhat.com> Content-Type: multipart/mixed; boundary="------------060109010401010401080503" Subject: Re: [Qemu-devel] [PATCH v3 2/7] qom/cpu: move register_vmstate to common CPUClass.realizefn List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov , Zhu Guihua Cc: qemu-devel@nongnu.org, tangchen@cn.fujitsu.com, isimatu.yasuaki@jp.fujitsu.com, guz.fnst@cn.fujitsu.com, anshul.makkar@profitbricks.com, afaerber@suse.de --------------060109010401010401080503 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit On 01/29/2015 10:04 PM, Igor Mammedov wrote: > On Wed, 14 Jan 2015 15:27:25 +0800 > Zhu Guihua wrote: > >> From: Gu Zheng >> >> 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 >> Signed-off-by: Zhu Guihua >> --- >> 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); > . > --------------060109010401010401080503 Content-Type: text/x-patch; name="fix-cross-version-impact.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="fix-cross-version-impact.diff" 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 */ --------------060109010401010401080503--