Re: [Qemu-devel] [PATCH 80/97] vmstate: Create VMSTATE_SYNTHETIC

[Dr. David Alan Gilbert]

* Juan Quintela (address@hidden) wrote:
> It is used for fields that don't exist on the State.  They are
> generated on the fly for migration.

While it's nicer than what's there before, I don't think this is the
right fix for these fields, and I'd rather not encourage new uses
like this.

It still hides the type from the VMSTATE mechanism and ends up with
the target-* code calling qemu_get*/qemu_put* on simple integers that VMSTATE
does support.

I was thinking something like:
  a) Set a .flags entry that this is a synthetic
  b) Change the .get/.put to post_load/pre_save
  c) Change the vmstate code to use a temporary if the synthetic flag is
     set, but then still call the pre_save/post_load on it passing the
     address of the temporary and of the real data somehow.

That way the vmstate code still sees integers that it knows the type of,
and we don't use qemu_get/qemu_put any more.

Dave


> Signed-off-by: Juan Quintela <address@hidden>
> ---
>  include/migration/vmstate.h | 14 ++++++++++++++
>  target-alpha/machine.c      | 16 +++-------------
>  target-arm/machine.c        | 18 ++----------------
>  3 files changed, 19 insertions(+), 29 deletions(-)
> 
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index d695244..12020d9 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -201,6 +201,20 @@ extern const VMStateInfo vmstate_info_bitmap;
>      .offset       = vmstate_offset_value(_state, _field, _type),     \
>  }
> 
> +/*
> + * This is used for fields synthetized from the state, but that don't
> + * exist as such.  That is the reaso of offset 0.  They get the whole
> + * struct.
> + */
> +
> +#define VMSTATE_SYNTHETIC(_name, _info, _size) {                     \
> +    .name         = (_name),                                         \
> +    .size         = (_size),                                         \
> +    .info         = &(_info),                                        \
> +    .flags        = VMS_SINGLE,                                      \
> +    .offset       = 0,                                               \
> +}
> +
>  #define VMSTATE_POINTER(_field, _state, _test, _info, _type) {  \
>      .name         = (stringify(_field)),                             \
>      .info         = &(_info),                                        \
> diff --git a/target-alpha/machine.c b/target-alpha/machine.c
> index 5e69b1e..a5db209 100644
> --- a/target-alpha/machine.c
> +++ b/target-alpha/machine.c
> @@ -23,20 +23,10 @@ static const VMStateInfo vmstate_fpcr = {
>  static VMStateField vmstate_env_fields[] = {
>      VMSTATE_UINTTL_ARRAY(ir, CPUAlphaState, 31),
>      VMSTATE_UINTTL_ARRAY(fir, CPUAlphaState, 31),
> +
>      /* Save the architecture value of the fpcr, not the internally
> -       expanded version.  Since this architecture value does not
> -       exist in memory to be stored, this requires a but of hoop
> -       jumping.  We want OFFSET=0 so that we effectively pass ENV
> -       to the helper functions, and we need to fill in the name by
> -       hand since there's no field of that name.  */
> -    {
> -        .name = "fpcr",
> -        .version_id = 0,
> -        .size = sizeof(uint64_t),
> -        .info = &vmstate_fpcr,
> -        .flags = VMS_SINGLE,
> -        .offset = 0
> -    },
> +       expanded version.  */
> +    VMSTATE_SYNTHETIC("fpcr", vmstate_fpcr, sizeof(uint64_t)),
>      VMSTATE_UINTTL(pc, CPUAlphaState),
>      VMSTATE_UINTTL(unique, CPUAlphaState),
>      VMSTATE_UINTTL(lock_addr, CPUAlphaState),
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index 3f2c485..fd01e99 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -47,14 +47,7 @@ static const VMStateDescription vmstate_vfp = {
>           */
>          VMSTATE_UINT32(env.vfp.xregs[0], ARMCPU),
>          VMSTATE_UINT32_SUB_ARRAY(env.vfp.xregs, ARMCPU, 2, 14),
> -        {
> -            .name = "fpscr",
> -            .version_id = 0,
> -            .size = sizeof(uint32_t),
> -            .info = &vmstate_fpscr,
> -            .flags = VMS_SINGLE,
> -            .offset = 0,
> -        },
> +        VMSTATE_SYNTHETIC("fpscr", vmstate_fpscr, sizeof(uint32)),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> @@ -224,14 +217,7 @@ const VMStateDescription vmstate_arm_cpu = {
>      .post_load = cpu_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32_ARRAY(env.regs, ARMCPU, 16),
> -        {
> -            .name = "cpsr",
> -            .version_id = 0,
> -            .size = sizeof(uint32_t),
> -            .info = &vmstate_cpsr,
> -            .flags = VMS_SINGLE,
> -            .offset = 0,
> -        },
> +        VMSTATE_SYNTHETIC("cpsr", vmstate_cpsr, sizeof(uint32_t)),
>          VMSTATE_UINT32(env.spsr, ARMCPU),
>          VMSTATE_UINT32_ARRAY(env.banked_spsr, ARMCPU, 6),
>          VMSTATE_UINT32_ARRAY(env.banked_r13, ARMCPU, 6),
> -- 
> 1.9.0
> 
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK