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

[Anthony Liguori]

On 11/09/2011 09:56 AM, Avi Kivity wrote:
> On 11/09/2011 05:49 PM, Anthony Liguori wrote:
> > > > VMSTATE_MEMORY_REGION(integratorcm, flash),
> > >
> > > Therefore this line is 100% redundant.
> >
> >
> > Yes, but the problem is that it's not obvious *why*.  That's what I'm
> > trying to get at here.  If you have a VMSTATE_MEMORY_REGION() that has
> > all of it's fields marked immutable and one field marked derived, now
> > it becomes obvious *why* we don't save these fields.
>
> Every MemoryRegion field in qemu today is either immutable or slaved to
> another register.  We could have a system to annotate every field, but
> it's pointless.

If I'm writing a device and doing save/restore and I happen to use a 
MemoryRegion, how do I determine that every field is either immutable or slaved?

> If we had a device that set the region offset to some value it computes
> at runtime that is not derived from state (say, offset = count of writes
> to some register) then there would be some point in it.  But we don't,
> so there isn't.
>
> > Just not having it in the vmstate description makes it very
> > non-obvious.  Is it a bug?  Is there some field in memory region that
> > I'm responsible for setting in a post load hook?
>
> Missing post-load hook bugs are not destructive.  Of course we should
> try to avoid them, but a markup system that we know ends up doing
> nothing is excessive.
>
> > > > 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.
> > >
> > > 100% of the memory API's fields are either immutable or derived.
> >
> > Ok, let's at least make the code make it obvious that that is the case.
>
> The memory/mutators branch simplifies it by eliminating pseudo state
> like flash_mapped.

They just moved the derived state into the MemoryRegion, no?

> > > > 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?
> > >
> > > name (for the monitor)
> > > size
> > > ptr to storage (in device state)
> > > writeable bits mask
> > > clear-on-read mask
> >
> > Really?  Is that all that common outside of PCI config?
>
> Yes, ISR fields often have it (like virtio).

Yes, but virtio-pci was a very special case to avoid taking an extra exit.

Do you know of any other than virtio-pci?  All the ones I can think of (RTC, 
Serial, etc.) are cleared with a write.

> > > read function (if computed on demand; otherwise satisfied from storage)
> > > write function (if have side effects)
> >
> > I tried something like this in Python at one point and the code ended
> > up very big to write a device model.  It's hard to beat the
> > conciseness of the dispatch functions with a switch() statement.
>
> This style of code really wants lambdas.  Without them, we have 4-5
> lines of boilerplate for each callback.  Even then, it's worthwhile IMO
> (and many callbacks can be avoided, both read and write, or merged into
> a device_update_mapping or device_update_irq read-all-state style
> functions).

Yeah, I looked at this but wasn't happy with the results.  In practice, many 
devices end up implementing non-trivial logic when register values change.

What I was really interested in was coming up with a way to get really high 
quality tracing of device register accesses.

Regards,

Anthony Liguori