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

[Avi Kivity]

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

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

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

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