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

[Anthony Liguori <anthony@codemonkey.ws>]

On 11/09/2011 09:05 AM, Avi Kivity wrote:
> On 11/09/2011 04:40 PM, Anthony Liguori wrote:
>>
>> typedef struct {
>>      SysBusDevice busdev;
>>      uint32_t memsz;
>>      MemoryRegion flash;
>>      bool flash_mapped;
>
> Both flash.has_parent and flash_mapped are slaved to a bit of one of the
> registers below.
>
>>      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),
>
> 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.

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?

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

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

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

Regards,

Anthony Liguori