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