Re: [Qemu-devel] [RFC qom-cpu v2 6/8] target-alpha: Register VMStateDescription for AlphaCPU

[Juan Quintela]

Andreas Färber <address@hidden> wrote:
> Am 22.02.2013 14:22, schrieb Juan Quintela:
>> And now that I review the 3rd patch that does the same, couldn't be
>> easier to do just:
>> 
>> 
>> static const VMStateDescription vmstate_cpu = {
>>     .name = "cpu",
>>     .version_id = 1,
>>     .minimum_version_id = 1,
>>     .minimum_version_id_old = 1,
>>     .fields = vmstate_cpu_fields,
>> };
>> 
>> to
>> 
>> static const VMStateDescription vmstate_cpu = {
>>     .name = "cpu",
>>     .version_id = 1,
>>     .minimum_version_id = 1,
>>     .minimum_version_id_old = 1,
>>     .fields = vmstate_cpu_fields,
>> };
>> 
>> static const VMStateDescription vmstate_alpha_cpu = {
>>     .name = "cpu",
>>     .version_id = 1,
>>     .minimum_version_id = 1,
>>     .minimum_version_id_old = 1,
>>     .fields =  (VMStateField []) {
>>          VMSTATE_STRUCT(env, AlphaCPU, 1, &vmstate_cpu, CPUAlphaState),
>>          VMSTATE_END_OF_LIST()
>>     }
>> };
>> 
>> ????
>> 
>> Only reason that I can think of for all the changes that you have done
>> is if you are expecting to move fields from CPUAlphaState to AlphaCPU.
>> Otherwise the method just exposed is much, much easier.
>
> Thanks for your review of this series!
>
> Did you actually read the cover letter though? :)

Obviously not O:-)

/me puts a beer in the "debt" part to Andreas O:-)


> Short-term I do need to access the halted field outside of CPUX86State
> in the referenced pending series, mid-term I would like to be able to
> move more non-TCG fields into FooCPU and not be limited by VMState only
> being able to access CPUArchState fields.
>
> Question: Is the VMSTATE_STRUCT() usage above version-compatible for
> previously-migratable targets such as lm32? We could leave x86 as is
> (modulo the vmsd registration discussion) and do the struct indirection
> for lm32 for now to keep changes minimal as long as non-CPULM32State
> fields are not yet needed.

It is compatible for all as they are Today.  You stop being able to use
the "trick" as soon as you want to access things that are not in env.

> For alpha and openrisc RFCs my big question was rather whether we want
> to use some VMSTATE_ macro to embed "cpu_common" as first field of "cpu"
> and assign it to DeviceClass::vmsd? Not sure which of the macros would
> allow us to register a VMStateDescription standalone for the "legacy"
> targets and as part of another VMStateDescription without sub-field such
> as 'env' above or whether we would need to duplicate the fields into a
> subsection definition then?
>
> More generally it would be a some-targets-do-it-differently-than-others
> kind of issue. But we already have some targets using VMState and others
> not, so you could say we have the issue today already. ;)
>
> In short: My pressing need is a version-compatible solution for x86
> registering VMState for X86CPU (CPUState) rather than CPUX86State,
> anything else could be postponed pending further discussions.

If you do the VMSTATE_STRUCT for all targets, you get half of the path
done.  And then we can study how to do it better for x86. 

About cpu_common.  I think that it is better that you embbed it for new
architectures, and just do it the other way for x86.

static const VMStateDescription vmstate_alpha_cpu = {
    .name = "cpu",
    .version_id = 1,
    .minimum_version_id = 1,
    .minimum_version_id_old = 1,
    .fields =  (VMStateField []) {
         VMSTATE_STRUCT(common, AlphaCPU, 1, &vmstate_cpu_common, CPUArchState),
         VMSTATE_STRUCT(env, AlphaCPU, 1, &vmstate_cpu, CPUAlphaState),
         VMSTATE_END_OF_LIST()
    }
};

Should work.

Notice that I haven't tested that the types are right, I am assuming
that the cpu_common field is called common, etc.

Later, Juan.