Re: [Qemu-devel] [PATCH V3 1/5] vmstate: introduce get_bufsize entry in VMStateField

["Andreas Färber" <afaerber@suse.de>]

Am 28.12.2011 16:32, schrieb Mitsyanko Igor:
> New get_bufsize field in VMStateField is supposed to help us easily add save/restore
> support of dynamically allocated buffers in device's states.
> There are some cases when information about size of dynamically allocated buffer is
> already presented in specific device's state structure, but in such a form that
> can not be used with existing VMStateField interface. Currently, we either can get size from
> another variable in device's state as it is with VMSTATE_VBUFFER_* macros, or we can
> also multiply value kept in a variable by a constant with VMSTATE_BUFFER_MULTIPLY
> macro. If we need to perform any other action, we're forced to add additional
> variable with size information to device state structure with the only intention
> to use it in VMStateDescription. This approach is not very pretty. Adding extra
> flags to VMStateFlags enum for every other possible operation with size field
> seems redundant, and still it would't cover cases when we need to perform a set of
> operations to get size value.
> With get_bufsize callback we can calculate size of dynamic array in whichever
> way we need. We don't need .size_offset field anymore, so we can remove it from
> VMState Field structure to compensate for extra memory consuption because of
> get_bufsize addition. Macros VMSTATE_VBUFFER* are modified to use new callback
> instead of .size_offset. Macro VMSTATE_BUFFER_MULTIPLY and VMFlag VMS_MULTIPLY
> are removed completely as they are now redundant.
> 
> Signed-off-by: Mitsyanko Igor <i.mitsyanko@samsung.com>

Ping! Juan, what do you think?

Andreas

> ---
>  hw/g364fb.c    |    7 ++++++-
>  hw/hw.h        |   41 +++++++----------------------------------
>  hw/m48t59.c    |    7 ++++++-
>  hw/mac_nvram.c |    8 +++++++-
>  hw/onenand.c   |    7 ++++++-
>  savevm.c       |   10 ++--------
>  6 files changed, 34 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/g364fb.c b/hw/g364fb.c
> index 34fb08c..1ab36c2 100644
> --- a/hw/g364fb.c
> +++ b/hw/g364fb.c
> @@ -495,6 +495,11 @@ static int g364fb_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static int g364fb_get_vramsize(void *opaque, int version_id)
> +{
> +    return ((G364State *)opaque)->vram_size;
> +}
> +
>  static const VMStateDescription vmstate_g364fb = {
>      .name = "g364fb",
>      .version_id = 1,
> @@ -502,7 +507,7 @@ static const VMStateDescription vmstate_g364fb = {
>      .minimum_version_id_old = 1,
>      .post_load = g364fb_post_load,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(vram, G364State, 1, NULL, 0, vram_size),
> +        VMSTATE_VBUFFER(vram, G364State, 1, NULL, 0, g364fb_get_vramsize),
>          VMSTATE_BUFFER_UNSAFE(color_palette, G364State, 0, 256 * 3),
>          VMSTATE_BUFFER_UNSAFE(cursor_palette, G364State, 0, 9),
>          VMSTATE_UINT16_ARRAY(cursor, G364State, 512),
> diff --git a/hw/hw.h b/hw/hw.h
> index efa04d1..a2a43b6 100644
> --- a/hw/hw.h
> +++ b/hw/hw.h
> @@ -303,7 +303,6 @@ enum VMStateFlags {
>      VMS_ARRAY_OF_POINTER = 0x040,
>      VMS_VARRAY_UINT16    = 0x080,  /* Array with size in uint16_t field */
>      VMS_VBUFFER          = 0x100,  /* Buffer with size in int32_t field */
> -    VMS_MULTIPLY         = 0x200,  /* multiply "size" field by field_size */
>      VMS_VARRAY_UINT8     = 0x400,  /* Array with size in uint8_t field*/
>      VMS_VARRAY_UINT32    = 0x800,  /* Array with size in uint32_t field*/
>  };
> @@ -315,12 +314,12 @@ typedef struct {
>      size_t start;
>      int num;
>      size_t num_offset;
> -    size_t size_offset;
>      const VMStateInfo *info;
>      enum VMStateFlags flags;
>      const VMStateDescription *vmsd;
>      int version_id;
>      bool (*field_exists)(void *opaque, int version_id);
> +    int (*get_bufsize)(void *opaque, int version_id);
>  } VMStateField;
>  
>  typedef struct VMStateSubsection {
> @@ -584,34 +583,11 @@ extern const VMStateInfo vmstate_info_unused_buffer;
>      .offset       = vmstate_offset_buffer(_state, _field) + _start,  \
>  }
>  
> -#define VMSTATE_BUFFER_MULTIPLY(_field, _state, _version, _test, _start, _field_size, _multiply) { \
> +#define VMSTATE_VBUFFER(_field, _state, _version, _test, _start, _get_bufsize) { \
>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
>      .field_exists = (_test),                                         \
> -    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
> -    .size         = (_multiply),                                      \
> -    .info         = &vmstate_info_buffer,                            \
> -    .flags        = VMS_VBUFFER|VMS_MULTIPLY,                        \
> -    .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
> -}
> -
> -#define VMSTATE_VBUFFER(_field, _state, _version, _test, _start, _field_size) { \
> -    .name         = (stringify(_field)),                             \
> -    .version_id   = (_version),                                      \
> -    .field_exists = (_test),                                         \
> -    .size_offset  = vmstate_offset_value(_state, _field_size, int32_t),\
> -    .info         = &vmstate_info_buffer,                            \
> -    .flags        = VMS_VBUFFER|VMS_POINTER,                         \
> -    .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
> -}
> -
> -#define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, _start, _field_size) { \
> -    .name         = (stringify(_field)),                             \
> -    .version_id   = (_version),                                      \
> -    .field_exists = (_test),                                         \
> -    .size_offset  = vmstate_offset_value(_state, _field_size, uint32_t),\
> +    .get_bufsize  = (_get_bufsize),                                  \
>      .info         = &vmstate_info_buffer,                            \
>      .flags        = VMS_VBUFFER|VMS_POINTER,                         \
>      .offset       = offsetof(_state, _field),                        \
> @@ -891,14 +867,11 @@ extern const VMStateDescription vmstate_hid_ptr_device;
>  #define VMSTATE_BUFFER_START_MIDDLE(_f, _s, _start) \
>      VMSTATE_STATIC_BUFFER(_f, _s, 0, NULL, _start, sizeof(typeof_field(_s, _f)))
>  
> -#define VMSTATE_PARTIAL_VBUFFER(_f, _s, _size)                        \
> -    VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _size)
> -
> -#define VMSTATE_PARTIAL_VBUFFER_UINT32(_f, _s, _size)                        \
> -    VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, 0, _size)
> +#define VMSTATE_PARTIAL_VBUFFER(_f, _s, _get_bufsize)                 \
> +    VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _get_bufsize)
>  
> -#define VMSTATE_SUB_VBUFFER(_f, _s, _start, _size)                    \
> -    VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _size)
> +#define VMSTATE_SUB_VBUFFER(_f, _s, _start, _get_bufsize)             \
> +    VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _get_bufsize)
>  
>  #define VMSTATE_BUFFER_TEST(_f, _s, _test)                            \
>      VMSTATE_STATIC_BUFFER(_f, _s, 0, _test, 0, sizeof(typeof_field(_s, _f)))
> diff --git a/hw/m48t59.c b/hw/m48t59.c
> index c043996..4e4c9f3 100644
> --- a/hw/m48t59.c
> +++ b/hw/m48t59.c
> @@ -582,6 +582,11 @@ static const MemoryRegionOps nvram_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> +static int m48t59_get_bufsize(void *opaque, int version_id)
> +{
> +    return ((M48t59State *)opaque)->size;
> +}
> +
>  static const VMStateDescription vmstate_m48t59 = {
>      .name = "m48t59",
>      .version_id = 1,
> @@ -590,7 +595,7 @@ static const VMStateDescription vmstate_m48t59 = {
>      .fields      = (VMStateField[]) {
>          VMSTATE_UINT8(lock, M48t59State),
>          VMSTATE_UINT16(addr, M48t59State),
> -        VMSTATE_VBUFFER_UINT32(buffer, M48t59State, 0, NULL, 0, size),
> +        VMSTATE_VBUFFER(buffer, M48t59State, 0, NULL, 0, m48t59_get_bufsize),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/hw/mac_nvram.c b/hw/mac_nvram.c
> index ed0a2b7..f4367f8 100644
> --- a/hw/mac_nvram.c
> +++ b/hw/mac_nvram.c
> @@ -100,13 +100,19 @@ static const MemoryRegionOps macio_nvram_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
> +static int macio_nvram_get_datasize(void *opaque, int version_id)
> +{
> +    return ((MacIONVRAMState *)opaque)->size;
> +}
> +
>  static const VMStateDescription vmstate_macio_nvram = {
>      .name = "macio_nvram",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .minimum_version_id_old = 1,
>      .fields      = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(data, MacIONVRAMState, 0, NULL, 0, size),
> +        VMSTATE_VBUFFER(data, MacIONVRAMState, 0, NULL, 0,
> +                macio_nvram_get_datasize),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/hw/onenand.c b/hw/onenand.c
> index a9d8d67..3e2016b 100644
> --- a/hw/onenand.c
> +++ b/hw/onenand.c
> @@ -160,6 +160,11 @@ static int onenand_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static int onenand_get_blockwpsize(void *opaque, int version_id)
> +{
> +    return ((OneNANDState *)opaque)->blocks;
> +}
> +
>  static const VMStateDescription vmstate_onenand = {
>      .name = "onenand",
>      .version_id = 1,
> @@ -181,7 +186,7 @@ static const VMStateDescription vmstate_onenand = {
>          VMSTATE_UINT16(intstatus, OneNANDState),
>          VMSTATE_UINT16(wpstatus, OneNANDState),
>          VMSTATE_INT32(secs_cur, OneNANDState),
> -        VMSTATE_PARTIAL_VBUFFER(blockwp, OneNANDState, blocks),
> +        VMSTATE_PARTIAL_VBUFFER(blockwp, OneNANDState, onenand_get_blockwpsize),
>          VMSTATE_UINT8(ecc.cp, OneNANDState),
>          VMSTATE_UINT16_ARRAY(ecc.lp, OneNANDState, 2),
>          VMSTATE_UINT16(ecc.count, OneNANDState),
> diff --git a/savevm.c b/savevm.c
> index f153c25..831c50a 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1412,10 +1412,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>              int size = field->size;
>  
>              if (field->flags & VMS_VBUFFER) {
> -                size = *(int32_t *)(opaque+field->size_offset);
> -                if (field->flags & VMS_MULTIPLY) {
> -                    size *= field->size;
> -                }
> +                size = field->get_bufsize(opaque, version_id);
>              }
>              if (field->flags & VMS_ARRAY) {
>                  n_elems = field->num;
> @@ -1476,10 +1473,7 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>              int size = field->size;
>  
>              if (field->flags & VMS_VBUFFER) {
> -                size = *(int32_t *)(opaque+field->size_offset);
> -                if (field->flags & VMS_MULTIPLY) {
> -                    size *= field->size;
> -                }
> +                size = field->get_bufsize(opaque, vmsd->version_id);
>              }
>              if (field->flags & VMS_ARRAY) {
>                  n_elems = field->num;

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg