[Qemu-devel] Re: [PATCH 11/11] test-vmstate: add test case to verify we don't change VMState

[Juan Quintela]

Anthony Liguori <address@hidden> wrote:
> On 03/23/2011 09:17 AM, Juan Quintela wrote:
>> Anthony Liguori<address@hidden>  wrote:
>> We need to fix the ordering problem.
>
> Dunno what you mean by ordering.

vmstate:

static const VMStateDescription vmstate_cpu = {
    .name = "cpu",
    .version_id = CPU_SAVE_VERSION,
    .minimum_version_id = 3,
    .minimum_version_id_old = 3,
    .pre_save = cpu_pre_save,
    .post_load = cpu_post_load,
    .fields      = (VMStateField []) {
        VMSTATE_UINTTL_ARRAY(regs, CPUState, CPU_NB_REGS),
        VMSTATE_UINTTL(eip, CPUState),
        VMSTATE_UINTTL(eflags, CPUState),
        VMSTATE_UINT32(hflags, CPUState),
        /* FPU */


vs

dump

    "cpu": {
        "mcg_cap": "uint64", 
        "a20_mask": "int32", 
        "tsc_offset": "uint64", 
        "idt": {
            "flags": "uint32", 
            "limit": "uint32", 
            "selector": "uint32", 
            "base": "uint32", 
            "__version__": 1
        }, 
        "intercept_cr_write": "uint16", 
        "nmi_injected": "uint8", 


You see that they are not in same order, then I can't use the schema to
read an arbitrary savevm image.  I think that ordering should be
preserved, makes schema much, much more useful.


Once told that, I think that doing a big schema is just wrong, we should
do an schema for device (or at least for architecture).  And no
hardcoded names (as they are today).  It is just trivial to run it for
x86_64-softmmu/i386-softmmu (the things that should work nowadays).

That way, downstreams can use it for its own "minimal machines".


>> Whatever schema we have should be good enough to allow:
>> - describe me this blob that contains the state for this device.
>
> Schema for VMState is different than what's used for this test case
> here.  I agree, it's a harder problem than just what's being spit out
> here :-)

It should be the same IMHO, it will not complicate anything here, and
just make it more useful.

>> eepro100 at least is missing.  Althought I would vote to just change the
>> eepro100 "naming" to always use eepro100 or similar, and remove the
>> current hack of having to change the vmstate->name for each different
>> device.
>
> I just ran into eepro100 and my head nearly exploded.

Being there, know the feeling.

> I set the name to be eepro100-base and then just added that once.  A
> better solution would be to separate out the fields such that we can
> have a bunch of VMStateDescriptions that all use the same fields.
>
> I think we ought to merge VMStateDescription into DeviceInfo.  For
> compatibility, we probably need a vmstate_alias name since the device
> names don't always map 1-1 with the qdev names.  But this should
> eliminate the problem of reusing VMStateDescriptions for multiple
> devices.

Agreed with that.

Later, Juan.