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

[Anthony Liguori <anthony@codemonkey.ws>]

On 11/08/2011 11:19 AM, Avi Kivity wrote:
> 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.

So we have:

typedef struct {
     SysBusDevice busdev;
     uint32_t memsz;
     MemoryRegion flash;
     bool flash_mapped;
     uint32_t cm_osc;
     uint32_t cm_ctrl;
     uint32_t cm_lock;
     uint32_t cm_auxosc;
     uint32_t cm_sdram;
     uint32_t cm_init;
     uint32_t cm_flags;
     uint32_t cm_nvflags;
     uint32_t int_level;
     uint32_t irq_enabled;
     uint32_t fiq_enabled;
} integratorcm_state;

What I'm saying is let's do:

VMSTATE_SYSBUS(integratorcm_state, busdev),
VMSTATE_UINT32(integratorcm, memsz),
VMSTATE_MEMORY_REGION(integratorcm, flash),
VMSTATE_BOOL(integratorcm, flash_mapped),
VMSTATE_UINT32(integratorcm, cm_osc),
VMSTATE_UINT32(integratorcm, cm_ctrl),
VMSTATE_UINT32(integratorcm, cm_lock),
VMSTATE_UINT32(integratorcm, cm_auxosc),
VMSTATE_UINT32(integratorcm, cm_sdram),
VMSTATE_UINT32(integratorcm, cm_init),
VMSTATE_UINT32(integratorcm, cm_flags),
VMSTATE_UINT32(integratorcm, cm_nvflags),
VMSTATE_UINT32(integratorcm, int_level),
VMSTATE_UINT32(integratorcm, irq_enabled),
VMSTATE_UINT32(integratorcm, fiq_enabled),

As there's a 1-1 mapping here.

You can agree, that this is functionally correct.  But flash_mapped is derived 
state and the contents of flash are almost entirely immutable so we don't 
strictly need to send it.

Okay, then let's add something to vmstate to suppress fields.  It could be as 
simple as:

  struct VMStateDescription {
+    const char *derived_fields[];
      const char *name;

This gives us a few things.  First, it means we're describing how to marshal 
everything which I really believe is the direction we need to go.  Second, it 
makes writing VMState descriptions easier to review.  Every field should be in 
the VMState description.  Any field that is in the derived_fields array should 
have its value set in the post_load function.  You could also have an 
immutable_fields array to indicate which fields are immutable.

Since VMSTATE_MEMORY_REGION is probably just going to point to a substructure, 
you could mark all of the fields of the memory region as immutable except for 
enabled and mark that derived.  This would also let us to do things like 
systematically make sure that when we're listing derived fields, we validate 
that we have a post_load function.

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

BTW, I've thought about this in the past but never came up with anything that 
really made sense.  Have you thought about what what a Register class would do?

Regards,

Anthony Liguori