Re: [Qemu-devel] [PATCH 23/35] vmstate: port arm cpu

[Juan Quintela]

Peter Maydell <address@hidden> wrote:
> On 4 May 2012 11:54, Juan Quintela <address@hidden> wrote:
>> Use one subsection for each feature.  This means that we don't need to
>> bump the version field each time that a new feature gets introduced.
>>
>> Introduce cpsr_vmstate field, as I am not sure if I can "use"
>> uncached_cpsr for saving state.
>>
>> Signed-off-by: Juan Quintela <address@hidden>
>> ---
>>  target-arm/cpu.h     |    5 +-
>>  target-arm/machine.c |  344 
>> ++++++++++++++++++++++----------------------------
>>  2 files changed, 156 insertions(+), 193 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 9434902..37744c6 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -236,6 +236,9 @@ typedef struct CPUARMState {
>>     } cp[15];
>>     void *nvic;
>>     const struct arm_boot_info *boot_info;
>> +
>> +    /* Fields needed as intermediate for vmstate */
>> +    uint32_t cpsr_vmstate;
>>  } CPUARMState;
>
> I still think this is the wrong approach. We need to support
> "this is how you read/write this field" functions. See also
> target-alpha handling of the fpcr.

It is easy to change to support that (see alpha example), but then, each
time that we change protocol format (and we have a pending change for
1.2), every user that is "like" uint64_t, but with accesor and setter,
needs to get a new change.  This other way (with pre_save/post_load)
instead makes trivial to change protocols.

So, we can have "cleaniness" or "flexibility", take one.
I still think that alpha should be changed to this other approach, for
the same reason.

Notice that we _know_ that what we sent is an uint32_t, it is how we get
that uint32_t what changes, that is why I preffer a new field and
pre_load/post_load functions.

The only other sane approach that I can think is changing:

#define VMSTATE_INT32_V(_f, _s, _v)                                   \

into something like:

VMSTATE_INT32_GENERAL(_f, _s, _v, _getter, _setter)

and then _getter/_setter functions for normal ints the obvious copy this
uint32_t?

Would that work for you?



Later, Juan.