Re: [Qemu-devel] [PATCH RFC qom-cpu-next 1/6] cpu: Register VMStateDescription through CPUState

[Andreas Färber]

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 <address@hidden>
>> Signed-off-by: Andreas Färber <address@hidden>
>> ---
>>  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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg