Re: [Qemu-devel] [PATCH 4/5] integratorcp: convert integratorcm to VMState

[Avi Kivity <avi@redhat.com>]

On 11/08/2011 05:04 PM, Anthony Liguori wrote:
>
> There's no code generation in QOM :-)
>
> This just comes down to how we do save/restore.  We white list things
> we care about.  We should move to a model where we save/restore
> everything (preferably via code generation), and then black
> list/transform state before it goes over the wire.
>
> Mike Roth's migration Visitor series is a first step in this
> direction.  The reason I bring this up in this context though is that
> using that mind set makes the answer about what to do here obvious. 
> If it's a member of a device's state, it should be save/restored.

Ok.

>
> MemoryRegion is a member of the device's state, so it should be
> save/restored with the device.

Not all MemoryRegion fields are state.  In some instantiations, none of
them are.

>
>>> That means we should have a VMSTATE_MEMORY_REGION().
>>>
>>> VMSTATE_MEMORY_REGION should save off the state of the memory region,
>>> and restore it appropriately.  VMSTATE_MEMORY_REGION's implementation
>>> does not need to live in memory.c.  It can certainly live in savevm.c
>>> or somewhere else more appropriate.
>>
>> What state is that?  Some devices have fixed size, offset, parent, and
>> enable/disable state (is there a word for that?), so there is no state
>> that needs to be transferred.  For other devices this is all dynamic.
>
> Any mutable state should be save/restored.  Immutable state doesn't
> need to be saved as it's created as part of the device model.

The memory API doesn't know which fields are mutable and which are not.

>
> If the question is, how do we restore the immutable state, that should
> be happening as part of device creation, no?
>
>> The way I see it, we create a link between some device state (a
>> register) and a memory API field (like the offset).  This way, when one
>> changes, so does the other.  In complicated devices we'll have to write
>> a callback.
>
> In devices where we dynamically change the offset (it's mutable), we
> should save the offset and restore it.  Since offset is sometimes
> mutable and sometimes immutable, we should always save/restore it.  In
> the cases where it's really immutable, since the value isn't changing,
> there's no harm in doing save/restore.

There is, you're taking an implementation detail and making it into an
ABI.  Change the implementation and migration breaks.

You can have a real region modeled as a set of nested regions, or as one
big region (with a more complicated switch () statement in the
callback).  This shouldn't be reflected in the save/restore ABI.

>
> Yes, we could save just the device register, and use a callback to
> regenerate the offset.  But that adds complexity and leads to more
> save/restore bugs.
>
> We shouldn't be reluctant to save/restore derived state.  Whether we
> send it over the wire is a different story.  We should start by saving
> as much state as we need to, and then sit down and start removing
> state and adding callbacks as we need to.

"saving state without sending it over the wire" is another way of saying
"not saving state".

> That way, we start with a strong statement of correctness as opposed
> to starting from a position of weak correctness.

We also start from a position of fragility wrt. implementation details.

>> flash_mapped always reflects a bit in a real register.  We shouldn't
>> duplicate state.
>
>
> Why?  The only thing that removing it does is create additional
> complexity for save/restore.  You may argue that sending minimal state
> improves migration compatibility but I think the current state of
> save/restore is an existence proof that this line of reasoning is
> incorrect.

It doesn't create additional complexity for save restore, and I don't
think that the current state of save/restore proves anything except that
it needs a lot more work.

-- 
error compiling committee.c: too many arguments to function