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 <[email protected]> >>>>>> Signed-off-by: Andreas Färber <[email protected]> >>>>>> --- >>>>>> 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 -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
