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

[Anthony Liguori <anthony@codemonkey.ws>]

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