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

[Avi Kivity <avi@redhat.com>]

On 11/08/2011 05:32 PM, Anthony Liguori wrote:
>
>>>
>>> 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.
>
> Yes, that's a feature, not a bug.  If we send too little state today
> in version X, then discover this while working on version X + 1, we
> have no recourse.  We have to black list version X.
>
> Discovering this is hard because we have to find a symptom of broken
> migration.  This is often subtle like, "if you migrate while a floppy
> request is in flight, the request is lost resulting in a timeout in
> the guest kernel".
>
> If we send too much state (internal implementation that is derived
> from something else) in version X, then discover this while working on
> version X + 1, we can filter the incoming state in X + 1 to just
> ignore the extra state and derive the correct internal state from the
> other stable registers.
>
> Discovering cases like this is easy because migration fails
> directly--not indirectly through a functional regression.  That means
> this is something we can very easily catch in regression testing.
>
> I actually think this is the way to do it too.  Save/restore
> everything by default and then as we develop and discover migration
> breaks, add filtering in the new versions to ignore and not send
> internal state.  I don't think there's a tremendous amount of value is
> proactively filtering internal state.  A lot of internal state never
> changes over a long period of time.

I might agree if a significant fraction of the memory API's state needed
to be saved.  But that's not the case -- indeed I expect it to be zero.

Take this patch for example, the only field that is mutable is the
enabled/disabled state, which mirrors some bit in a register.  PIIX's
PAM, PCI's BARs are the same.  I doubt there is *any* case where the
memory API is the sole source of this information.

The way we do this now is to call device_update_mappings() whenever a
register that contains mapping information changes, whether it is in a
device_write() callback or in device_post_load().  All that you'd save
with automatic memory API state migration is the latter call.

>
>>> 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".
>
> Or filtering it on the receiving end.  That's the fundamental difference.

I might agree if I thought there is anything worthwhile in the memory
API's 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.
>
> It's very hard to do the style of save/restore that we do correctly.

If we had a Register class, that would take care of device registers
automatically.  As to in flight transactions or hidden state, I don't
think there are any shortcuts.

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