On Thu, Feb 07, 2013 at 07:02:32PM +0100, Andreas Färber wrote: > Am 07.02.2013 18:46, schrieb Eduardo Habkost: > > 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? > > I'm talking about, e.g., #include "machine.c", i.e. VMStateDescription > vmstate_something = { ... }; something_else = &vmstate_something;. > > This broke horribly. > > > 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). > > That's what I was saying, yes. > > >>>> +#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? > > In short: Lack of patch 6. > > !defined(CPU_SAVE_VERSION) but implementing cpu_save/load when they > should've #define'd CPU_SAVE_VERSION. > > In turn I want to assure that when !defined(CPU_SAVE_VERSION) > implementing cpu_save/load leads to build error. > > > 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); > > &vmstate_cpu_common will break for linux-user.
Oops. Then what about: #if !defined(CONFIG_USER_ONLY) static const VMStateDescription vmstate_cpu_common { ... }; #define cpu_common_vmsd (&vmstate_cpu_common) #else #define cpu_common_vmsd NULL #endif [...] vmstate_register(NULL, cpu_index, cpu_common_vmsd, env); [...] This pattern is similar to what I suggested for the code that initializes cc->vmsd on the target-specific class_init functions. I don't really love the way it looks, but I prefer #ifdefs on declarative parts of the code than inside functions. I wonder if we could simplify it further. > > > #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); > > We would need to leave out the #elif and assert cc->vmsd == NULL, but > then it would address my concern, yes. > > !defined(CPU_SAVE_VERSION) => cc->vmsd != NULL is not guaranteed: OK. I just thought that was the bug you were trying to catch, but it was a different problem, as you explained above. > For 1.4 some unmigratable CPUs register via dc->vmsd instead. > > Regards, > Andreas > > > #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 > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -- Eduardo