Message ID | 1359817456-25561-2-git-send-email-afaerber@suse.de |
---|---|
State | New |
Headers | show |
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 <quintela@redhat.com> > Signed-off-by: Andreas Färber <afaerber@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). > +#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. > + 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 > >
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 <quintela@redhat.com> >> Signed-off-by: Andreas Färber <afaerber@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... >> +#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. This is the part of the patch that I adopted from Juan. I don't insist though. 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
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 <quintela@redhat.com> > >> Signed-off-by: Andreas Färber <afaerber@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
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 <quintela@redhat.com> >>>> Signed-off-by: Andreas Färber <afaerber@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. > #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: 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
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 <quintela@redhat.com> > >>>> Signed-off-by: Andreas Färber <afaerber@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
Am 07.02.2013 19:22, schrieb Eduardo Habkost: > 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 <quintela@redhat.com> >>>>>> Signed-off-by: Andreas Färber <afaerber@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. As commented on the x86 part I'm not quite happy with that pattern... Is there a way we could keep the referencing local to the code using it, i.e. have a single vmstate_dummy in *-user code that we can #define vmstate_x86_cpu etc. to for CONFIG_USER_ONLY? I don't quite see where and how, might require to define a file-local struct VMStateDescription without fields or so. Andreas
Am 07.02.2013 19:27, schrieb Andreas Färber: > Am 07.02.2013 19:22, schrieb Eduardo Habkost: >> 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 <quintela@redhat.com> >>>>>>> Signed-off-by: Andreas Färber <afaerber@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. > > As commented on the x86 part I'm not quite happy with that pattern... > > Is there a way we could keep the referencing local to the code using it, > i.e. have a single vmstate_dummy in *-user code that we can #define > vmstate_x86_cpu etc. to for CONFIG_USER_ONLY? I don't quite see where > and how, might require to define a file-local struct VMStateDescription > without fields or so. Found a solution to both your suggestions, v2 coming up. Andreas
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); +#else + 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;