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

[Anthony Liguori]

On 11/08/2011 08:38 AM, Avi Kivity wrote:
> On 11/08/2011 03:50 PM, Anthony Liguori wrote:
> > > We agree, the only difference is in what "core" refers to.  I don't want
> > > memory.c do become intermingled with everything else.  It should be in a
> > > separate layer.  Devices would then talk to this layer, and not to the
> > > gluing by themselves as they have to now.
> > >
> > > Or maybe memory.c will be layered on top of QOM, and then it can take
> > > care of everything.  I really don't know QOM well enough to say.
> > > Copying Anthony for more insight.
> >
> >
> > QOM fixes all problems, but I don't think this has anything to do with
> > QOM :-)
> >
> > We fundamentally should do save/restore using a rigorous,
> > automatically generated mechanism where every field is save/restored[1].
>
> "fundamentally", "rigrous", "automatically", "generated", "mechanism",
> "every", "field", and even "a" are all words that I associate with QOM.
> Am I misunderstanding anything?

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.

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

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

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.

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.

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

> > Where the device is mapped within the address space is no longer a
> > property of the device, so restoring the mapping really should happen
> > at whatever owns the address space (in this case sysbus).
> >
> > In this case, integratorcp is the one mapping things into its own
> > address space so flash_mapped ought to be saved by integratorcp.
>
> 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.

Regards,

Anthony Liguori

> > [1] Deciding that a field doesn't need to be saved should be an
> > exception.
>
> It should indicate that the field doesn't need to exist.