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

[Avi Kivity]

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.

> VMSTATE_BOOL(integratorcm, flash_mapped),
> VMSTATE_UINT32(integratorcm, cm_osc),
> VMSTATE_UINT32(integratorcm, cm_ctrl),
> VMSTATE_UINT32(integratorcm, cm_lock),
> VMSTATE_UINT32(integratorcm, cm_auxosc),
> VMSTATE_UINT32(integratorcm, cm_sdram),
> VMSTATE_UINT32(integratorcm, cm_init),
> VMSTATE_UINT32(integratorcm, cm_flags),
> VMSTATE_UINT32(integratorcm, cm_nvflags),
> VMSTATE_UINT32(integratorcm, int_level),
> VMSTATE_UINT32(integratorcm, irq_enabled),
> VMSTATE_UINT32(integratorcm, fiq_enabled),
>
> As there's a 1-1 mapping here.
>
> You can agree, that this is functionally correct.  But flash_mapped is
> derived state and the contents of flash are almost entirely immutable
> so we don't strictly need to send it.
>
> Okay, then let's add something to vmstate to suppress fields.  It
> could be as simple as:
>
>  struct VMStateDescription {
> +    const char *derived_fields[];
>      const char *name;
>
> 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.

>
> Since VMSTATE_MEMORY_REGION is probably just going to point to a
> substructure, you could mark all of the fields of the memory region as
> immutable except for enabled and mark that derived.  This would also
> let us to do things like systematically make sure that when we're
> listing derived fields, we validate that we have a post_load function.
>

If a post_load is missing, it's just a bug, not missing state, so it's
not a catastrophic bug.  The issues are save-side.

>> If we had a Register class, that would take care of device registers
>> automatically.  As to in flight transactions or hidden state, I don't
>> think there are any shortcuts.
>
> 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
read function (if computed on demand; otherwise satisfied from storage)
write function (if have side effects)

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