On Thu, Feb 07, 2013 at 06:20:52PM +0100, Andreas Färber wrote: > Am 07.02.2013 17:40, schrieb Eduardo Habkost: > > On Sat, Feb 02, 2013 at 04:04:11PM +0100, Andreas Färber wrote: > >> In comparison to DeviceClass::vmsd, CPU VMState is split in two, > >> "cpu_common" and "cpu", and uses cpu_index as instance_id instead of -1. > >> Therefore add a CPU-specific CPUClass::vmsd field. > >> > >> Unlike the legacy CPUArchState registration, rather register CPUState. > >> > >> Signed-off-by: Juan Quintela <quint...@redhat.com> > >> Signed-off-by: Andreas Färber <afaer...@suse.de> > >> --- > >> exec.c | 13 +++++++++++-- > >> include/qom/cpu.h | 3 +++ > >> 2 Dateien geändert, 14 Zeilen hinzugefügt(+), 2 Zeilen entfernt(-) > >> > >> diff --git a/exec.c b/exec.c > >> index b85508b..5eee174 100644 > >> --- a/exec.c > >> +++ b/exec.c > >> @@ -219,7 +219,7 @@ void cpu_exec_init_all(void) > >> #endif > >> } > >> > >> -#if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY) > >> +#if !defined(CONFIG_USER_ONLY) > >> > >> static int cpu_common_post_load(void *opaque, int version_id) > >> { > >> @@ -266,6 +266,9 @@ CPUState *qemu_get_cpu(int index) > >> void cpu_exec_init(CPUArchState *env) > >> { > >> CPUState *cpu = ENV_GET_CPU(env); > >> +#if !defined(CONFIG_USER_ONLY) && !defined(CPU_SAVE_VERSION) > >> + CPUClass *cc = CPU_GET_CLASS(cpu); > >> +#endif > >> CPUArchState **penv; > >> int cpu_index; > >> > >> @@ -290,10 +293,16 @@ void cpu_exec_init(CPUArchState *env) > >> #if defined(CONFIG_USER_ONLY) > >> cpu_list_unlock(); > >> #endif > >> -#if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY) > >> +#if !defined(CONFIG_USER_ONLY) > >> vmstate_register(NULL, cpu_index, &vmstate_cpu_common, env); > >> +#if defined(CPU_SAVE_VERSION) > >> register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION, > >> cpu_save, cpu_load, env); > > > > What about introducing stub register_savevm() function in libqemustub.a, > > so we can eliminate the CONFIG_USER_ONLY ifdefs? > > > > (we already have vmstate_register()/vmstate_unregister() stubs). > > register_savevm() itself is not the issue, it's the VMStateDescription > itself with all its external references that would be quite some (IMO > useless) work to make available to *-user...
Are you talking about the VMStateDescription struct declaration, or about actually setting the vmsd field? The struct declaration is available even if CONFIG_USER_ONLY is set. See qdev.c. It doesn't have any #ifdefs around vmstate_register()/vmstate_unregister() calls. I don't suggest we set the field to non-NULL if CONFIG_USER_ONLY is set (that would be difficult and unnecessary). > > >> +#else > > > > Do we have any architecture that sets CPU_SAVE_VERSION _and_ sets > > cc->vmsd at the same time? If not, we could put the "if (cc->vmsd) > > vmstate_register()" part outside any #ifdef/#else block. > > They shouldn't. When this series and any follow-up by Juan himself were > applied, there would be no more CPU_SAVE_VERSION and all CPUs would > register a vmsd one way or another. Targets to be converted include ppc, > arm and sparc. > > Together with the final RFC patch in this series, doing it inside an > #else block avoids repeating the lax checks that have led alpha, > openrisc and a few others to not register things correctly. What exactly were the lax checks that have led them to not register things correctly? Would my suggestion below cause the same problems? > This is the > part of the patch that I adopted from Juan. I don't insist though. I am OK with "#ifdef CPU_SAVE_VERSION" #ifdef because it is for legacy code (and should be temporary, right?), but I think we can easily drop the #ifdefs around all the other lines. I mean, we can easily make the code look like this: void cpu_exec_init(CPUArchState *env) { CPUState *cpu = ENV_GET_CPU(env); CPUClass *cc = CPU_GET_CLASS(cpu); [...] vmstate_register(NULL, cpu_index, &vmstate_cpu_common, env); #if defined(CPU_SAVE_VERSION) register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION, cpu_save, cpu_load, env); #endif if (cc->vmsd) { vmstate_register(NULL, cpu_index, cc->vmsd, cpu); } } If we want to catch architectures that don't set CPU_SAVE_VERSION but also don't register any vmsd (is this the bug you are trying to catch?), we could do this: #if defined(CPU_SAVE_VERSION) register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION, cpu_save, cpu_load, env); #elif !defined(CONFIG_USER_ONLY) assert(cc->vmsd); #endif if (cc->vmsd) { vmstate_register(NULL, cpu_index, cc->vmsd, cpu); } > > Regards, > Andreas > > >> + if (cc->vmsd != NULL) { > >> + vmstate_register(NULL, cpu_index, cc->vmsd, cpu); > >> + } > >> +#endif > >> #endif > >> } > >> > >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h > >> index 46f2247..b870752 100644 > >> --- a/include/qom/cpu.h > >> +++ b/include/qom/cpu.h > >> @@ -43,6 +43,7 @@ typedef struct CPUState CPUState; > >> * @class_by_name: Callback to map -cpu command line model name to an > >> * instantiatable CPU type. > >> * @reset: Callback to reset the #CPUState to its initial state. > >> + * @vmsd: State description for migration. > >> * > >> * Represents a CPU family or model. > >> */ > >> @@ -54,6 +55,8 @@ typedef struct CPUClass { > >> ObjectClass *(*class_by_name)(const char *cpu_model); > >> > >> void (*reset)(CPUState *cpu); > >> + > >> + const struct VMStateDescription *vmsd; > >> } CPUClass; > >> > >> struct KVMState; > >> -- > >> 1.7.10.4 > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -- Eduardo