Patchwork [v2,1/2] hw/arm_sysctl.c: Add the Versatile Express system registers

login
register
mail settings
Submitter Peter Maydell
Date March 4, 2011, 8:34 p.m.
Message ID <1299270860-8927-2-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/85438/
State New
Headers show

Comments

Peter Maydell - March 4, 2011, 8:34 p.m.
Add support for the Versatile Express SYS_CFG registers, which provide
a generic means of reading or writing configuration information from
various parts of the board. We only implement shutdown and reset.

Also make the RESETCTL register RAZ/WI on Versatile Express rather
than reset the board. Other system registers are generally the same
as Versatile and Realview.

This includes a VMState version number bump for arm_sysctl,
since we have new register state to preserve.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm_sysctl.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 63 insertions(+), 2 deletions(-)
Paolo Bonzini - March 5, 2011, 12:11 p.m.
On 03/04/2011 09:34 PM, Peter Maydell wrote:
>       .name = "realview_sysctl",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>       .fields = (VMStateField[]) {
>           VMSTATE_UINT32(leds, arm_sysctl_state),
>           VMSTATE_UINT16(lockval, arm_sysctl_state),
> @@ -41,6 +44,9 @@ static const VMStateDescription vmstate_arm_sysctl = {
>           VMSTATE_UINT32(flags, arm_sysctl_state),
>           VMSTATE_UINT32(nvflags, arm_sysctl_state),
>           VMSTATE_UINT32(resetlevel, arm_sysctl_state),
> +        VMSTATE_UINT32(sys_cfgdata, arm_sysctl_state),
> +        VMSTATE_UINT32(sys_cfgctrl, arm_sysctl_state),
> +        VMSTATE_UINT32(sys_cfgstat, arm_sysctl_state),
>           VMSTATE_END_OF_LIST()
>       }

You need to present the fields as version 2-only.

Paolo
Peter Maydell - March 5, 2011, 12:34 p.m.
On 5 March 2011 12:11, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 03/04/2011 09:34 PM, Peter Maydell wrote:
>>
>>      .name = "realview_sysctl",
>> -    .version_id = 1,
>> -    .minimum_version_id = 1,
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_UINT32(leds, arm_sysctl_state),
>>          VMSTATE_UINT16(lockval, arm_sysctl_state),
>> @@ -41,6 +44,9 @@ static const VMStateDescription vmstate_arm_sysctl = {
>>          VMSTATE_UINT32(flags, arm_sysctl_state),
>>          VMSTATE_UINT32(nvflags, arm_sysctl_state),
>>          VMSTATE_UINT32(resetlevel, arm_sysctl_state),
>> +        VMSTATE_UINT32(sys_cfgdata, arm_sysctl_state),
>> +        VMSTATE_UINT32(sys_cfgctrl, arm_sysctl_state),
>> +        VMSTATE_UINT32(sys_cfgstat, arm_sysctl_state),
>>          VMSTATE_END_OF_LIST()
>>      }
>
> You need to present the fields as version 2-only.

Can you give an example/explanation? docs/migration.txt doesn't
seem to cover this...

thanks
-- PMM
Paolo Bonzini - March 5, 2011, 2:59 p.m.
On 03/05/2011 01:34 PM, Peter Maydell wrote:
>>> >>  +        VMSTATE_UINT32(sys_cfgdata, arm_sysctl_state),
>>> >>  +        VMSTATE_UINT32(sys_cfgctrl, arm_sysctl_state),
>>> >>  +        VMSTATE_UINT32(sys_cfgstat, arm_sysctl_state),
>>> >>            VMSTATE_END_OF_LIST()
>>> >>        }
>> >
>> >  You need to present the fields as version 2-only.
> Can you give an example/explanation? docs/migration.txt doesn't
> seem to cover this...

Sure, sorry for being terse. It simply needs to be:

         VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2),
         VMSTATE_UINT32_V(sys_cfgctrl, arm_sysctl_state, 2),
         VMSTATE_UINT32_V(sys_cfgstat, arm_sysctl_state, 2),

Also, minimum_version_id needs to remain 1 since you do support loading 
version 1 saved virtual machines.

Paolo
Peter Maydell - March 5, 2011, 4:50 p.m.
On 5 March 2011 14:59, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 03/05/2011 01:34 PM, Peter Maydell wrote:
>> Can you give an example/explanation? docs/migration.txt doesn't
>> seem to cover this...
>
> Sure, sorry for being terse. It simply needs to be:
>
>        VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2),
>        VMSTATE_UINT32_V(sys_cfgctrl, arm_sysctl_state, 2),
>        VMSTATE_UINT32_V(sys_cfgstat, arm_sysctl_state, 2),
>
> Also, minimum_version_id needs to remain 1 since you do support loading
> version 1 saved virtual machines.

Ah. I was just following Juan's suggestion:
> - if you don't care about backward compatibility, just add +1 to all the
> version fields and you are done.

but I guess if it's that trivial we might as well support it.
(My guess is that basically nobody is doing any kind of
migration of ARM TCG VMs, so I think it's all a bit moot.)

What do the new fields get set to when you do a load from
a v1 snapshot?

Other random snapshot/vmstate questions while I'm here:

(1) Is there supposed to be any kind of guard on trying to
do a vmsave on a system with devices that don't implement
save/load? IME it just produces a snapshot which doesn't
work when you reload it...
(2) How do you track incompatible changes at the machine
level? For instance, suppose we accidentally forgot to
model a NOT gate in an IRQ line, so we add a qemu_irq_invert()
to the machine init function. None of the devices have
changed, but you can't load the state of an old version
of the machine into a new version, because then the
devices on either end of the inverter would be inconsistent
about the state of the line. But there's no version number
for a machine as far as I can see.


I've appended a draft of a suggested extra section for
docs/migration.txt so you can tell me if I've misunderstood
it all :-)

---begin---
=== Adding state fields to a device ===

If you make a bugfix or enhancement to a device which requires the
addition of extra state, you need to add these new state fields
to the VMStateDescription so that:
 (1) they are saved and loaded correctly
 (2) migration between the new and old versions either works
     or fails cleanly.

If the change is such that migration between versions would not
work anyway, you can just add the new fields using the usual
VMSTATE_* macros, increment the version_id and set the
minimum_version_id to be the same as the version_id.

Migration from the old version to the new version can be supported
if it is OK for the new fields to remain in their default state
[XXX is this right? are they zeroed, or do they get the value
the device's reset function sets them to, or something else?]
when the state of an old-version snapshot is loaded. To implement
this you need to use the VMSTATE_*_V macros which let you specify
the version in which a field was introduced, for instance:

 VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2)

for a field introduced in version 2. You should also increment
the version_id, but leave the minimum_version_id unchanged.
Newly added VMSTATE_*_V fields should go at the end of the
VMState description.

---endit---

thanks
-- PMM
Paolo Bonzini - March 5, 2011, 5:04 p.m.
On 03/05/2011 05:50 PM, Peter Maydell wrote:
> (1) Is there supposed to be any kind of guard on trying to
> do a vmsave on a system with devices that don't implement
> save/load? IME it just produces a snapshot which doesn't
> work when you reload it...

I think you're right, devices currently have to call 
register_device_unmigratable manually.  I guess you could add support to 
qdev, so that qdev-ified devices could specify a special "forbid 
migration" value for the vmsd field.

Alternatively, you could have NULL mean "forbid migration" and a special 
value for "do not save anything, it will just work".

> (2) How do you track incompatible changes at the machine
> level? For instance, suppose we accidentally forgot to
> model a NOT gate in an IRQ line, so we add a qemu_irq_invert()
> to the machine init function. None of the devices have
> changed, but you can't load the state of an old version
> of the machine into a new version, because then the
> devices on either end of the inverter would be inconsistent
> about the state of the line. But there's no version number
> for a machine as far as I can see.

You can change the machine model and keep the incompatible machine as a 
separate model.  A machine can specify compatibility properties that are 
meant exactly for this kind of bug-compatible behavior.  Reloading the 
VM must be done with the correct -M switch for the old model.

Examples of how to do this are in hw/pc_piix.c (which will provide good 
ideas for further grepping, too :)).

Paolo
Peter Maydell - March 7, 2011, 11:45 a.m.
On 5 March 2011 17:04, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 03/05/2011 05:50 PM, Peter Maydell wrote:
>>
>> (1) Is there supposed to be any kind of guard on trying to
>> do a vmsave on a system with devices that don't implement
>> save/load? IME it just produces a snapshot which doesn't
>> work when you reload it...
>
> I think you're right, devices currently have to call
> register_device_unmigratable manually.

That's a shame, since there are still plenty of devices in
the tree which just don't implement save/restore. It would
be nice if trying to vmsave one of those boards produced
an error listing all the devices that would need support
added for it to work.

> I guess you could add support to
> qdev, so that qdev-ified devices could specify a special "forbid migration"
> value for the vmsd field.

> Alternatively, you could have NULL mean "forbid migration" and a special
> value for "do not save anything, it will just work".

You definitely want the default to be "save/load support status
unknown, forbid migration" (whether the device is qdev or not),
and then you can whitelist devices where somebody's actually
checked the code and confirmed that saving nothing is OK.

-- PMM
Jan Kiszka - March 7, 2011, 12:09 p.m.
On 2011-03-07 12:45, Peter Maydell wrote:
> On 5 March 2011 17:04, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 03/05/2011 05:50 PM, Peter Maydell wrote:
>>>
>>> (1) Is there supposed to be any kind of guard on trying to
>>> do a vmsave on a system with devices that don't implement
>>> save/load? IME it just produces a snapshot which doesn't
>>> work when you reload it...
>>
>> I think you're right, devices currently have to call
>> register_device_unmigratable manually.
> 
> That's a shame, since there are still plenty of devices in
> the tree which just don't implement save/restore. It would
> be nice if trying to vmsave one of those boards produced
> an error listing all the devices that would need support
> added for it to work.

register_device_unmigratable is for devices that are conceptually
unmigratable (like assigned physical devices where we can't
extract/restore the state). It's not for devices that simply lack
vmstate support because no one bothered yet. Once we have device state
visualization (it's making progress again), I hope the motivation to
convert the remaining devices will increase further.

But I agree (and pointed this out when register_device_unmigratable was
introduced) that it should become a qdev flag. ivshmem could simple
register two devices, master as migratable, peer as not.

We could provide a void vmsd that qdev devices could use to declare "I'm
migratable", either because they still do legacy vmstate registration
(fortunately, only few are left) or because they have no state. Then the
absence of a vmsd would imply "unmigratable".

> 
>> I guess you could add support to
>> qdev, so that qdev-ified devices could specify a special "forbid migration"
>> value for the vmsd field.
> 
>> Alternatively, you could have NULL mean "forbid migration" and a special
>> value for "do not save anything, it will just work".
> 
> You definitely want the default to be "save/load support status
> unknown, forbid migration" (whether the device is qdev or not),
> and then you can whitelist devices where somebody's actually
> checked the code and confirmed that saving nothing is OK.

Part of the problem is that there are still non-qdev devices that are
basically outside any radar (except the one that scans code...).

Jan
Peter Maydell - March 22, 2011, 7:05 p.m.
On 5 March 2011 16:50, Peter Maydell <peter.maydell@linaro.org> wrote:
> I've appended a draft of a suggested extra section for
> docs/migration.txt so you can tell me if I've misunderstood
> it all :-)

Ping? In particular if somebody can fill in the [XXX] bit for
me (and correct any other mistakes!) I'll submit this as a proper
patch to the docs.

> ---begin---
> === Adding state fields to a device ===
>
> If you make a bugfix or enhancement to a device which requires the
> addition of extra state, you need to add these new state fields
> to the VMStateDescription so that:
>  (1) they are saved and loaded correctly
>  (2) migration between the new and old versions either works
>     or fails cleanly.
>
> If the change is such that migration between versions would not
> work anyway, you can just add the new fields using the usual
> VMSTATE_* macros, increment the version_id and set the
> minimum_version_id to be the same as the version_id.
>
> Migration from the old version to the new version can be supported
> if it is OK for the new fields to remain in their default state
> [XXX is this right? are they zeroed, or do they get the value
> the device's reset function sets them to, or something else?]
> when the state of an old-version snapshot is loaded. To implement
> this you need to use the VMSTATE_*_V macros which let you specify
> the version in which a field was introduced, for instance:
>
>  VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2)
>
> for a field introduced in version 2. You should also increment
> the version_id, but leave the minimum_version_id unchanged.
> Newly added VMSTATE_*_V fields should go at the end of the
> VMState description.
>
> ---endit---

thanks
-- PMM
Juan Quintela - March 22, 2011, 7:53 p.m.
Peter Maydell <peter.maydell@linaro.org> wrote:
> On 5 March 2011 14:59, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 03/05/2011 01:34 PM, Peter Maydell wrote:

sorry, I miss this email before.

>
> Ah. I was just following Juan's suggestion:
>> - if you don't care about backward compatibility, just add +1 to all the
>> version fields and you are done.
>
> but I guess if it's that trivial we might as well support it.
> (My guess is that basically nobody is doing any kind of
> migration of ARM TCG VMs, so I think it's all a bit moot.)
>
> What do the new fields get set to when you do a load from
> a v1 snapshot?
>
> Other random snapshot/vmstate questions while I'm here:
>
> (1) Is there supposed to be any kind of guard on trying to
> do a vmsave on a system with devices that don't implement
> save/load? IME it just produces a snapshot which doesn't
> work when you reload it...

only real way to do it is to set a device that is not migratable.  Only
having devices that don't have a savevm section would not work, because
for instance usb devices don't need a savevm section.

> (2) How do you track incompatible changes at the machine
> level? For instance, suppose we accidentally forgot to
> model a NOT gate in an IRQ line, so we add a qemu_irq_invert()
> to the machine init function. None of the devices have
> changed, but you can't load the state of an old version
> of the machine into a new version, because then the
> devices on either end of the inverter would be inconsistent
> about the state of the line. But there's no version number
> for a machine as far as I can see.

Dunno.  I have only worked to x86*, no clue about integrated boards that
have "interesting" conections.  At some point we would have to "define"
the machine board better, just now it is not there.

> I've appended a draft of a suggested extra section for
> docs/migration.txt so you can tell me if I've misunderstood
> it all :-)
>
> ---begin---
> === Adding state fields to a device ===
>
> If you make a bugfix or enhancement to a device which requires the
> addition of extra state, you need to add these new state fields
> to the VMStateDescription so that:
>  (1) they are saved and loaded correctly
>  (2) migration between the new and old versions either works
>      or fails cleanly.
>
> If the change is such that migration between versions would not
> work anyway, you can just add the new fields using the usual
> VMSTATE_* macros, increment the version_id and set the
> minimum_version_id to be the same as the version_id.
>
> Migration from the old version to the new version can be supported
> if it is OK for the new fields to remain in their default state
> [XXX is this right? are they zeroed, or do they get the value
> the device's reset function sets them to, or something else?]

You can initialize in your init function at the value that you want, or
use foo_post_load() function (that receives the version as a parameter)
to assign to any correct values that you need.

> when the state of an old-version snapshot is loaded. To implement
> this you need to use the VMSTATE_*_V macros which let you specify
> the version in which a field was introduced, for instance:
>
>  VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2)
>
> for a field introduced in version 2. You should also increment
> the version_id, but leave the minimum_version_id unchanged.
> Newly added VMSTATE_*_V fields should go at the end of the
> VMState description.

Just to make things more complicated, this has been "deprecated" O:-)

Ok, I am going to try to explain it myself.

We have one vmstate section.

const VMStateDescription vmstate_foo = {
    .name = "foo",
    .version_id = 0,
    .minimum_version_id = 0,
    .minimum_version_id_old = 0,
    .fields      = (VMStateField []) {
        VMSTATE_INT32(bar, FOOState),
        VMSTATE_END_OF_LIST()
    }
};

And we then notice that we need another int (bar2) on the state.
Posibilities:

- We know that old device was wrong, and that there is no way we can
  load (reliabely) from version 0.  Then we just increase the version:

const VMStateDescription vmstate_foo = {
    .name = "foo",
    .version_id = 1,
    .minimum_version_id = 1,
    .minimum_version_id_old = 1,
    .fields      = (VMStateField []) {
        VMSTATE_INT32(bar, FOOState),
        VMSTATE_INT32(bar2, FOOState),
        VMSTATE_END_OF_LIST()
    }
};

  And we are done.

- We know that we can load from v1.  But that we want to always sent
  bar2 for migration, then we just increase versions to:


const VMStateDescription vmstate_foo = {
    .name = "foo",
    .version_id = 2,
    .minimum_version_id = 1,
    .minimum_version_id_old = 1,
    .fields      = (VMStateField []) {
        VMSTATE_INT32(bar, FOOState),
        VMSTATE_INT32_V(bar2, FOOState, 1),
        VMSTATE_END_OF_LIST()
    }
};

And we are done.  We are able to receive state 0 and 1, and we would
always sent version 1.

- We know that bar2 is only need sometimes, and we know what sometimes
  mean.  Then we use a new subsection.

/* this function returns true if bar2 is needed */
static bool foo_bar2_needed(void *opaque)
{
    FOOState *s = opaque;

    return ......;
}

const VMStateDescription vmstate_bar2 = {
    .name = "foo/bar2",
    .version_id = 0,
    .minimum_version_id = 0,
    .minimum_version_id_old = 0,
    .fields      = (VMStateField []) {
        VMSTATE_INT32(bar2, FOOState),
        VMSTATE_END_OF_LIST()
    }
};

const VMStateDescription vmstate_foo = {
    .name = "foo",
    .version_id = 2,
    .minimum_version_id = 1,
    .minimum_version_id_old = 1,
    .fields      = (VMStateField []) {
        VMSTATE_INT32(bar, FOOState),
        VMSTATE_INT32_V(bar2, FOOState, 1),
        VMSTATE_END_OF_LIST()
    }
,
    .subsections = (VMStateSubsection []) {
        {
            .vmsd = &vmstate_bar2,
            .needed = foo_bar2_needed,
        }, {
            /* empty */
        }
    }
};

Why do we want to go to all this trouble? To be able to do a migration
from a new version to one old version.  If bar2 is not needed, we know
that we can _not_ send bar2 subsection. If bar2 is needed, we just sent
it and we fail the migration to the old version.

Notice that this only makes sense if foo_bar2_needed() normally returns
false, if it returns always true, it is just as good to just increase
the version.   An example on when this is useful is with ide.  When we
defined vmstate_ide_drive, we forgot the pio state.  So, it always
worked well execpt if we were in the middle of a pio operation.  PIO
operations are only used (normally) during boot.   Adding a new
subsection means that we normally don't sent the pio state, except if it
is really needed.

Have I manage to explain myself a little bit?

Later, Juan.
Peter Maydell - March 22, 2011, 8:32 p.m.
On 22 March 2011 19:53, Juan Quintela <quintela@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> Migration from the old version to the new version can be supported
>> if it is OK for the new fields to remain in their default state
>> [XXX is this right? are they zeroed, or do they get the value
>> the device's reset function sets them to, or something else?]
>
> You can initialize in your init function at the value that you want, or
> use foo_post_load() function (that receives the version as a parameter)
> to assign to any correct values that you need.

To check I understand this, this means an incoming migration is
always done to a fresh, never-been-used-before device that has had
its init called but not its reset?

>> when the state of an old-version snapshot is loaded. To implement
>> this you need to use the VMSTATE_*_V macros which let you specify
>> the version in which a field was introduced, for instance:
>>
>>  VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2)
>>
>> for a field introduced in version 2. You should also increment
>> the version_id, but leave the minimum_version_id unchanged.
>> Newly added VMSTATE_*_V fields should go at the end of the
>> VMState description.
>
> Just to make things more complicated, this has been "deprecated" O:-)

It has? Your examples below still use it...

> - We know that old device was wrong, and that there is no way we can
>  load (reliabely) from version 0.  Then we just increase the version:

If you're increasing the version can you also clean up by
converting any old VMSTATE_*_V() into plain VMSTATE_*() at this
point, since we can't migrate from those old versions any more?

> - We know that we can load from v1.  But that we want to always sent
>  bar2 for migration, then we just increase versions to:
>
>
> const VMStateDescription vmstate_foo = {
>    .name = "foo",
>    .version_id = 2,
>    .minimum_version_id = 1,
>    .minimum_version_id_old = 1,
>    .fields      = (VMStateField []) {
>        VMSTATE_INT32(bar, FOOState),
>        VMSTATE_INT32_V(bar2, FOOState, 1),
>        VMSTATE_END_OF_LIST()
>    }
> };
>
> And we are done.  We are able to receive state 0 and 1, and we would
> always sent version 1.

Your numbers in the struct and the text don't seem to match?
My guess is you meant to write version_id = 1, minimum_version* = 0 ?

> Have I manage to explain myself a little bit?

Yes, thanks, that's very helpful.

-- PMM
Paolo Bonzini - March 23, 2011, 8:54 a.m.
On 03/22/2011 09:32 PM, Peter Maydell wrote:
>> >  Just to make things more complicated, this has been "deprecated"O:-)
>
> It has? Your examples below still use it...

The case in which the "subsection needed" function returns true should 
be rare, so the version number should rarely need to be bumped.  In this 
sense, using _V is discouraged/deprecated.

In fact, some people would prefer the version number not to be bumped 
anymore, and subsections to be always used instead.  So far, every time 
the above argument was brought up in the list, people always found a way 
to define the "subsection needed" function so that it didn't return 
true, and the decision on deprecation of _V was postponed.

Subsections make it easier for downstream versions to backport features 
arbitrarily.  Suppose you release QEMU with a device at version 9.  The 
next version adds feature A as version 10 and feature B as version 11. 
For a downstream vendor, backporting just feature B is difficult because 
they would have three choices:

- the good, but also the hardest: bump to version 11, and save some 
dummy (but valid) value for fields related to feature A.  This 
introduces undesired differences from upstream, and may be difficult.

- the bad: bump to version 10, and have a migration format that is 
incompatible with upstream version 10.

- the ugly: keep version 9, and convert the migration data for feature B 
to a subsection.  This introduces differences from upstream and makes 
the migration format incompatible with upstream version, but avoids that 
the same version number means different things in different distributions.

So, those people say that subsections are a bit more friendly to 
downstream vendors.  So they suggest that upstream should use the third 
option to begin with, and even use subsections even if the "subsection 
needed" function returns true.  This makes the backport easier and more 
straightforward.  The argument is good but, as I said, so far there has 
never been an actual need to apply it.

So, Juan's mail documents what QEMU is doing right now accurately, but 
there isn't 100% agreement that it should be that way in the future. 
Just note that you are encouraged to use subsections (and thus devise a 
way to make the subsection optional) whenever possible and whenever it 
makes sense to help such downstream distributors.

Paolo
Juan Quintela - March 23, 2011, 9:44 a.m.
Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 March 2011 19:53, Juan Quintela <quintela@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>> Migration from the old version to the new version can be supported
>>> if it is OK for the new fields to remain in their default state
>>> [XXX is this right? are they zeroed, or do they get the value
>>> the device's reset function sets them to, or something else?]
>>
>> You can initialize in your init function at the value that you want, or
>> use foo_post_load() function (that receives the version as a parameter)
>> to assign to any correct values that you need.
>
> To check I understand this, this means an incoming migration is
> always done to a fresh, never-been-used-before device that has had
> its init called but not its reset?
>
>>> when the state of an old-version snapshot is loaded. To implement
>>> this you need to use the VMSTATE_*_V macros which let you specify
>>> the version in which a field was introduced, for instance:
>>>
>>>  VMSTATE_UINT32_V(sys_cfgdata, arm_sysctl_state, 2)
>>>
>>> for a field introduced in version 2. You should also increment
>>> the version_id, but leave the minimum_version_id unchanged.
>>> Newly added VMSTATE_*_V fields should go at the end of the
>>> VMState description.
>>
>> Just to make things more complicated, this has been "deprecated" O:-)
>
> It has? Your examples below still use it...

as Paolo says, it should be rare that you need it.


>> - We know that old device was wrong, and that there is no way we can
>>  load (reliabely) from version 0.  Then we just increase the version:
>
> If you're increasing the version can you also clean up by
> converting any old VMSTATE_*_V() into plain VMSTATE_*() at this
> point, since we can't migrate from those old versions any more?

From vl.c

    qemu_system_reset();
    if (loadvm) {
        if (load_vmstate(loadvm) < 0) {
            autostart = 0;
        }
    }

    if (incoming) {
        int ret = qemu_start_incoming_migration(incoming);
        if (ret < 0) {
            fprintf(stderr, "Migration failed. Exit code %s(%d), exiting.\n",
                    incoming, ret);
            exit(ret);
        }
    } else if (autostart) {
        vm_start();
    }


reset is always called after init, before both incoming migration and
normal start.

>> - We know that we can load from v1.  But that we want to always sent
>>  bar2 for migration, then we just increase versions to:
>>
>>
>> const VMStateDescription vmstate_foo = {
>>    .name = "foo",
>>    .version_id = 2,
>>    .minimum_version_id = 1,
>>    .minimum_version_id_old = 1,
>>    .fields      = (VMStateField []) {
>>        VMSTATE_INT32(bar, FOOState),
>>        VMSTATE_INT32_V(bar2, FOOState, 1),
>>        VMSTATE_END_OF_LIST()
>>    }
>> };
>>
>> And we are done.  We are able to receive state 0 and 1, and we would
>> always sent version 1.
>
> Your numbers in the struct and the text don't seem to match?
> My guess is you meant to write version_id = 1, minimum_version* = 0 ?

My fault. copy paste :-(

>> Have I manage to explain myself a little bit?
>
> Yes, thanks, that's very helpful.

You are welcome.

Later, Juan.

Patch

diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c
index 799b007..e4a17f5 100644
--- a/hw/arm_sysctl.c
+++ b/hw/arm_sysctl.c
@@ -27,12 +27,15 @@  typedef struct {
     uint32_t resetlevel;
     uint32_t proc_id;
     uint32_t sys_mci;
+    uint32_t sys_cfgdata;
+    uint32_t sys_cfgctrl;
+    uint32_t sys_cfgstat;
 } arm_sysctl_state;
 
 static const VMStateDescription vmstate_arm_sysctl = {
     .name = "realview_sysctl",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(leds, arm_sysctl_state),
         VMSTATE_UINT16(lockval, arm_sysctl_state),
@@ -41,6 +44,9 @@  static const VMStateDescription vmstate_arm_sysctl = {
         VMSTATE_UINT32(flags, arm_sysctl_state),
         VMSTATE_UINT32(nvflags, arm_sysctl_state),
         VMSTATE_UINT32(resetlevel, arm_sysctl_state),
+        VMSTATE_UINT32(sys_cfgdata, arm_sysctl_state),
+        VMSTATE_UINT32(sys_cfgctrl, arm_sysctl_state),
+        VMSTATE_UINT32(sys_cfgstat, arm_sysctl_state),
         VMSTATE_END_OF_LIST()
     }
 };
@@ -53,6 +59,7 @@  static const VMStateDescription vmstate_arm_sysctl = {
 #define BOARD_ID_EB 0x140
 #define BOARD_ID_PBA8 0x178
 #define BOARD_ID_PBX 0x182
+#define BOARD_ID_VEXPRESS 0x190
 
 static int board_id(arm_sysctl_state *s)
 {
@@ -104,6 +111,10 @@  static uint32_t arm_sysctl_read(void *opaque, target_phys_addr_t offset)
     case 0x38: /* NVFLAGS */
         return s->nvflags;
     case 0x40: /* RESETCTL */
+        if (board_id(s) == BOARD_ID_VEXPRESS) {
+            /* reserved: RAZ/WI */
+            return 0;
+        }
         return s->resetlevel;
     case 0x44: /* PCICTL */
         return 1;
@@ -142,7 +153,23 @@  static uint32_t arm_sysctl_read(void *opaque, target_phys_addr_t offset)
     case 0xcc: /* SYS_TEST_OSC3 */
     case 0xd0: /* SYS_TEST_OSC4 */
         return 0;
+    case 0xa0: /* SYS_CFGDATA */
+        if (board_id(s) != BOARD_ID_VEXPRESS) {
+            goto bad_reg;
+        }
+        return s->sys_cfgdata;
+    case 0xa4: /* SYS_CFGCTRL */
+        if (board_id(s) != BOARD_ID_VEXPRESS) {
+            goto bad_reg;
+        }
+        return s->sys_cfgctrl;
+    case 0xa8: /* SYS_CFGSTAT */
+        if (board_id(s) != BOARD_ID_VEXPRESS) {
+            goto bad_reg;
+        }
+        return s->sys_cfgstat;
     default:
+    bad_reg:
         printf ("arm_sysctl_read: Bad register offset 0x%x\n", (int)offset);
         return 0;
     }
@@ -190,6 +217,10 @@  static void arm_sysctl_write(void *opaque, target_phys_addr_t offset,
         s->nvflags &= ~val;
         break;
     case 0x40: /* RESETCTL */
+        if (board_id(s) == BOARD_ID_VEXPRESS) {
+            /* reserved: RAZ/WI */
+            break;
+        }
         if (s->lockval == LOCK_VALUE) {
             s->resetlevel = val;
             if (val & 0x100)
@@ -216,7 +247,37 @@  static void arm_sysctl_write(void *opaque, target_phys_addr_t offset,
     case 0x98: /* OSCRESET3 */
     case 0x9c: /* OSCRESET4 */
         break;
+    case 0xa0: /* SYS_CFGDATA */
+        if (board_id(s) != BOARD_ID_VEXPRESS) {
+            goto bad_reg;
+        }
+        s->sys_cfgdata = val;
+        return;
+    case 0xa4: /* SYS_CFGCTRL */
+        if (board_id(s) != BOARD_ID_VEXPRESS) {
+            goto bad_reg;
+        }
+        s->sys_cfgctrl = val & ~(3 << 18);
+        s->sys_cfgstat = 1;            /* complete */
+        switch (s->sys_cfgctrl) {
+        case 0xc0800000:            /* SYS_CFG_SHUTDOWN to motherboard */
+            qemu_system_shutdown_request();
+            break;
+        case 0xc0900000:            /* SYS_CFG_REBOOT to motherboard */
+            qemu_system_reset_request();
+            break;
+        default:
+            s->sys_cfgstat |= 2;        /* error */
+        }
+        return;
+    case 0xa8: /* SYS_CFGSTAT */
+        if (board_id(s) != BOARD_ID_VEXPRESS) {
+            goto bad_reg;
+        }
+        s->sys_cfgstat = val & 3;
+        return;
     default:
+    bad_reg:
         printf ("arm_sysctl_write: Bad register offset 0x%x\n", (int)offset);
         return;
     }