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

[Anthony Liguori]

On 03/23/2011 10:26 AM, Juan Quintela wrote:
> 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.

This is *only* for testing right now.  We can enhance it down the road 
if we want to but this is an opaque thing that's only there to enable 
test cases to be written.

> 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".

I agree, we ought to try to make this schema more consumable.  But some 
of that is that the schema needs to describe types in a more meaningful 
way as there's a lot of weird types right now.

> > > 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.

Yeah, it will tremendously complicate things because QDict's don't 
preserve order.  It's all fixable but why wait to have something that's 
incredibly useful in tree?

> > > 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.

Once we settle on the unit tests, I can look at writing some scripts to 
do this.

Regards,

Anthony Liguori

> Later, Juan.