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

[Anthony Liguori]

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