Re: [Qemu-devel] [PATCH 4/4] migration: drop unused VMStateField.start

[Dr. David Alan Gilbert]

* Halil Pasic (address@hidden) wrote:
> The member VMStateField.start was solely used to implement the partial
> data migration for VBUFFER data (basically provide migration for a
> sub-buffer). However the implementation of this feature seems broken to
> me, but this goes unnoticed since the feature is not used at all.
> Instead of fixing it lets try simplify things by dropping it.
> 
> Signed-off-by: Halil Pasic <address@hidden>
> Reviewed-by: Guenther Hutzl <address@hidden>

Reviewed-by: Dr. David Alan Gilbert <address@hidden>

> ---
>  hw/char/exynos4210_uart.c   |  2 +-
>  hw/display/g364fb.c         |  2 +-
>  hw/dma/pl330.c              |  8 ++++----
>  hw/intc/exynos4210_gic.c    |  2 +-
>  hw/ipmi/isa_ipmi_bt.c       |  4 ++--
>  hw/ipmi/isa_ipmi_kcs.c      |  4 ++--
>  hw/net/vmxnet3.c            |  2 +-
>  hw/nvram/mac_nvram.c        |  2 +-
>  hw/nvram/spapr_nvram.c      |  2 +-
>  hw/sd/sdhci.c               |  2 +-
>  hw/timer/m48t59.c           |  2 +-
>  include/migration/vmstate.h | 20 ++++++--------------
>  migration/savevm.c          |  2 +-
>  migration/vmstate.c         |  4 ++--
>  target-s390x/machine.c      |  2 +-
>  tests/test-vmstate.c        |  4 ++--
>  util/fifo8.c                |  2 +-
>  17 files changed, 29 insertions(+), 37 deletions(-)
> 
> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> index 1107578..7b1b4b1 100644
> --- a/hw/char/exynos4210_uart.c
> +++ b/hw/char/exynos4210_uart.c
> @@ -565,7 +565,7 @@ static const VMStateDescription 
> vmstate_exynos4210_uart_fifo = {
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(sp, Exynos4210UartFIFO),
>          VMSTATE_UINT32(rp, Exynos4210UartFIFO),
> -        VMSTATE_VBUFFER_UINT32(data, Exynos4210UartFIFO, 1, NULL, 0, size),
> +        VMSTATE_VBUFFER_UINT32(data, Exynos4210UartFIFO, 1, NULL, size),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/hw/display/g364fb.c b/hw/display/g364fb.c
> index 70ef2c7..8cdc205 100644
> --- a/hw/display/g364fb.c
> +++ b/hw/display/g364fb.c
> @@ -464,7 +464,7 @@ static const VMStateDescription vmstate_g364fb = {
>      .minimum_version_id = 1,
>      .post_load = g364fb_post_load,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(vram, G364State, 1, NULL, 0, vram_size),
> +        VMSTATE_VBUFFER_UINT32(vram, G364State, 1, NULL, vram_size),
>          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/dma/pl330.c b/hw/dma/pl330.c
> index c0bd9fe..32cf839 100644
> --- a/hw/dma/pl330.c
> +++ b/hw/dma/pl330.c
> @@ -173,8 +173,8 @@ static const VMStateDescription vmstate_pl330_fifo = {
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(buf, PL330Fifo, 1, NULL, 0, buf_size),
> -        VMSTATE_VBUFFER_UINT32(tag, PL330Fifo, 1, NULL, 0, buf_size),
> +        VMSTATE_VBUFFER_UINT32(buf, PL330Fifo, 1, NULL, buf_size),
> +        VMSTATE_VBUFFER_UINT32(tag, PL330Fifo, 1, NULL, buf_size),
>          VMSTATE_UINT32(head, PL330Fifo),
>          VMSTATE_UINT32(num, PL330Fifo),
>          VMSTATE_UINT32(buf_size, PL330Fifo),
> @@ -282,8 +282,8 @@ static const VMStateDescription vmstate_pl330 = {
>          VMSTATE_STRUCT(manager, PL330State, 0, vmstate_pl330_chan, 
> PL330Chan),
>          VMSTATE_STRUCT_VARRAY_UINT32(chan, PL330State, num_chnls, 0,
>                                       vmstate_pl330_chan, PL330Chan),
> -        VMSTATE_VBUFFER_UINT32(lo_seqn, PL330State, 1, NULL, 0, num_chnls),
> -        VMSTATE_VBUFFER_UINT32(hi_seqn, PL330State, 1, NULL, 0, num_chnls),
> +        VMSTATE_VBUFFER_UINT32(lo_seqn, PL330State, 1, NULL, num_chnls),
> +        VMSTATE_VBUFFER_UINT32(hi_seqn, PL330State, 1, NULL, num_chnls),
>          VMSTATE_STRUCT(fifo, PL330State, 0, vmstate_pl330_fifo, PL330Fifo),
>          VMSTATE_STRUCT(read_queue, PL330State, 0, vmstate_pl330_queue,
>                         PL330Queue),
> diff --git a/hw/intc/exynos4210_gic.c b/hw/intc/exynos4210_gic.c
> index fd7a8f3..2a55817 100644
> --- a/hw/intc/exynos4210_gic.c
> +++ b/hw/intc/exynos4210_gic.c
> @@ -393,7 +393,7 @@ static const VMStateDescription 
> vmstate_exynos4210_irq_gate = {
>      .version_id = 2,
>      .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(level, Exynos4210IRQGateState, 1, NULL, 0, 
> n_in),
> +        VMSTATE_VBUFFER_UINT32(level, Exynos4210IRQGateState, 1, NULL, n_in),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index f036617..6c10ef7 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -471,9 +471,9 @@ static const VMStateDescription vmstate_ISAIPMIBTDevice = 
> {
>          VMSTATE_BOOL(bt.use_irq, ISAIPMIBTDevice),
>          VMSTATE_BOOL(bt.irqs_enabled, ISAIPMIBTDevice),
>          VMSTATE_UINT32(bt.outpos, ISAIPMIBTDevice),
> -        VMSTATE_VBUFFER_UINT32(bt.outmsg, ISAIPMIBTDevice, 1, NULL, 0,
> +        VMSTATE_VBUFFER_UINT32(bt.outmsg, ISAIPMIBTDevice, 1, NULL,
>                                 bt.outlen),
> -        VMSTATE_VBUFFER_UINT32(bt.inmsg, ISAIPMIBTDevice, 1, NULL, 0,
> +        VMSTATE_VBUFFER_UINT32(bt.inmsg, ISAIPMIBTDevice, 1, NULL,
>                                 bt.inlen),
>          VMSTATE_UINT8(bt.control_reg, ISAIPMIBTDevice),
>          VMSTATE_UINT8(bt.mask_reg, ISAIPMIBTDevice),
> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
> index 9a38f8a..b592d53 100644
> --- a/hw/ipmi/isa_ipmi_kcs.c
> +++ b/hw/ipmi/isa_ipmi_kcs.c
> @@ -433,9 +433,9 @@ const VMStateDescription vmstate_ISAIPMIKCSDevice = {
>          VMSTATE_BOOL(kcs.use_irq, ISAIPMIKCSDevice),
>          VMSTATE_BOOL(kcs.irqs_enabled, ISAIPMIKCSDevice),
>          VMSTATE_UINT32(kcs.outpos, ISAIPMIKCSDevice),
> -        VMSTATE_VBUFFER_UINT32(kcs.outmsg, ISAIPMIKCSDevice, 1, NULL, 0,
> +        VMSTATE_VBUFFER_UINT32(kcs.outmsg, ISAIPMIKCSDevice, 1, NULL,
>                                 kcs.outlen),
> -        VMSTATE_VBUFFER_UINT32(kcs.inmsg, ISAIPMIKCSDevice, 1, NULL, 0,
> +        VMSTATE_VBUFFER_UINT32(kcs.inmsg, ISAIPMIKCSDevice, 1, NULL,
>                                 kcs.inlen),
>          VMSTATE_BOOL(kcs.write_end, ISAIPMIKCSDevice),
>          VMSTATE_UINT8(kcs.status_reg, ISAIPMIKCSDevice),
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 90f6943..b4b71f3 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2396,7 +2396,7 @@ static const VMStateDescription 
> vmxstate_vmxnet3_mcast_list = {
>      .pre_load = vmxnet3_mcast_list_pre_load,
>      .needed = vmxnet3_mc_list_needed,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(mcast_list, VMXNET3State, 0, NULL, 0,
> +        VMSTATE_VBUFFER_UINT32(mcast_list, VMXNET3State, 0, NULL,
>              mcast_list_buff_size),
>          VMSTATE_END_OF_LIST()
>      }
> diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c
> index 24f6121..09676e2 100644
> --- a/hw/nvram/mac_nvram.c
> +++ b/hw/nvram/mac_nvram.c
> @@ -83,7 +83,7 @@ static const VMStateDescription vmstate_macio_nvram = {
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(data, MacIONVRAMState, 0, NULL, 0, size),
> +        VMSTATE_VBUFFER_UINT32(data, MacIONVRAMState, 0, NULL, size),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
> index 4de5f70..ccef3f9 100644
> --- a/hw/nvram/spapr_nvram.c
> +++ b/hw/nvram/spapr_nvram.c
> @@ -218,7 +218,7 @@ static const VMStateDescription vmstate_spapr_nvram = {
>      .post_load = spapr_nvram_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(size, sPAPRNVRAM),
> -        VMSTATE_VBUFFER_ALLOC_UINT32(buf, sPAPRNVRAM, 1, NULL, 0, size),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(buf, sPAPRNVRAM, 1, NULL, size),
>          VMSTATE_END_OF_LIST()
>      },
>  };
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 01fbf22..c0d7de7 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -1253,7 +1253,7 @@ const VMStateDescription sdhci_vmstate = {
>          VMSTATE_UINT16(data_count, SDHCIState),
>          VMSTATE_UINT64(admasysaddr, SDHCIState),
>          VMSTATE_UINT8(stopped_state, SDHCIState),
> -        VMSTATE_VBUFFER_UINT32(fifo_buffer, SDHCIState, 1, NULL, 0, 
> buf_maxsz),
> +        VMSTATE_VBUFFER_UINT32(fifo_buffer, SDHCIState, 1, NULL, buf_maxsz),
>          VMSTATE_TIMER_PTR(insert_timer, SDHCIState),
>          VMSTATE_TIMER_PTR(transfer_timer, SDHCIState),
>          VMSTATE_END_OF_LIST()
> diff --git a/hw/timer/m48t59.c b/hw/timer/m48t59.c
> index e46ca88..40a9e18 100644
> --- a/hw/timer/m48t59.c
> +++ b/hw/timer/m48t59.c
> @@ -634,7 +634,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_UINT32(buffer, M48t59State, 0, NULL, size),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1638ee5..26527ad 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -190,7 +190,6 @@ typedef struct {
>      const char *name;
>      size_t offset;
>      size_t size;
> -    size_t start;
>      int num;
>      size_t num_offset;
>      size_t size_offset;
> @@ -570,7 +569,7 @@ extern const VMStateInfo vmstate_info_bitmap;
>      .offset       = vmstate_offset_buffer(_state, _field) + _start,  \
>  }
>  
> -#define VMSTATE_VBUFFER_MULTIPLY(_field, _state, _version, _test, _start, 
> _field_size, _multiply) { \
> +#define VMSTATE_VBUFFER_MULTIPLY(_field, _state, _version, _test, 
> _field_size, _multiply) { \
>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
>      .field_exists = (_test),                                         \
> @@ -579,10 +578,9 @@ extern const VMStateInfo vmstate_info_bitmap;
>      .info         = &vmstate_info_buffer,                            \
>      .flags        = VMS_VBUFFER|VMS_POINTER|VMS_MULTIPLY,            \
>      .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
>  }
>  
> -#define VMSTATE_VBUFFER(_field, _state, _version, _test, _start, 
> _field_size) { \
> +#define VMSTATE_VBUFFER(_field, _state, _version, _test, _field_size) { \
>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
>      .field_exists = (_test),                                         \
> @@ -590,10 +588,9 @@ extern const VMStateInfo vmstate_info_bitmap;
>      .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) { \
> +#define VMSTATE_VBUFFER_UINT32(_field, _state, _version, _test, _field_size) 
> { \
>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
>      .field_exists = (_test),                                         \
> @@ -601,10 +598,9 @@ extern const VMStateInfo vmstate_info_bitmap;
>      .info         = &vmstate_info_buffer,                            \
>      .flags        = VMS_VBUFFER|VMS_POINTER,                         \
>      .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
>  }
>  
> -#define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, _test, 
> _start, _field_size) { \
> +#define VMSTATE_VBUFFER_ALLOC_UINT32(_field, _state, _version, _test, 
> _field_size) { \
>      .name         = (stringify(_field)),                             \
>      .version_id   = (_version),                                      \
>      .field_exists = (_test),                                         \
> @@ -612,7 +608,6 @@ extern const VMStateInfo vmstate_info_bitmap;
>      .info         = &vmstate_info_buffer,                            \
>      .flags        = VMS_VBUFFER|VMS_POINTER|VMS_ALLOC,               \
>      .offset       = offsetof(_state, _field),                        \
> -    .start        = (_start),                                        \
>  }
>  
>  #define VMSTATE_BUFFER_UNSAFE_INFO_TEST(_field, _state, _test, _version, 
> _info, _size) { \
> @@ -912,13 +907,10 @@ extern const VMStateInfo vmstate_info_bitmap;
>      VMSTATE_BUFFER_START_MIDDLE_V(_f, _s, _start, 0)
>  
>  #define VMSTATE_PARTIAL_VBUFFER(_f, _s, _size)                        \
> -    VMSTATE_VBUFFER(_f, _s, 0, NULL, 0, _size)
> +    VMSTATE_VBUFFER(_f, _s, 0, NULL, _size)
>  
>  #define VMSTATE_PARTIAL_VBUFFER_UINT32(_f, _s, _size)                        
> \
> -    VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, 0, _size)
> -
> -#define VMSTATE_SUB_VBUFFER(_f, _s, _start, _size)                    \
> -    VMSTATE_VBUFFER(_f, _s, 0, NULL, _start, _size)
> +    VMSTATE_VBUFFER_UINT32(_f, _s, 0, NULL, _size)
>  
>  #define VMSTATE_BUFFER_TEST(_f, _s, _test)                            \
>      VMSTATE_STATIC_BUFFER(_f, _s, 0, _test, 0, sizeof(typeof_field(_s, _f)))
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 33a2911..1df5c76 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -308,7 +308,7 @@ static const VMStateDescription vmstate_configuration = {
>      .pre_save = configuration_pre_save,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT32(len, SaveState),
> -        VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, 0, len),
> +        VMSTATE_VBUFFER_ALLOC_UINT32(name, SaveState, 0, NULL, len),
>          VMSTATE_END_OF_LIST()
>      },
>  };
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index fc29acf..8767e40 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -66,10 +66,10 @@ static void *vmstate_base_addr(void *opaque, VMStateField 
> *field, bool alloc)
>                  }
>              }
>              if (size) {
> -                *((void **)base_addr + field->start) = g_malloc(size);
> +                *(void **)base_addr = g_malloc(size);
>              }
>          }
> -        base_addr = *(void **)base_addr + field->start;
> +        base_addr = *(void **)base_addr;
>      }
>  
>      return base_addr;
> diff --git a/target-s390x/machine.c b/target-s390x/machine.c
> index edc3a47..8503fa1 100644
> --- a/target-s390x/machine.c
> +++ b/target-s390x/machine.c
> @@ -180,7 +180,7 @@ const VMStateDescription vmstate_s390_cpu = {
>          VMSTATE_UINT8(env.cpu_state, S390CPU),
>          VMSTATE_UINT8(env.sigp_order, S390CPU),
>          VMSTATE_UINT32_V(irqstate_saved_size, S390CPU, 4),
> -        VMSTATE_VBUFFER_UINT32(irqstate, S390CPU, 4, NULL, 0,
> +        VMSTATE_VBUFFER_UINT32(irqstate, S390CPU, 4, NULL,
>                                 irqstate_saved_size),
>          VMSTATE_END_OF_LIST()
>      },
> diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
> index 4ea64b7..aaa7143 100644
> --- a/tests/test-vmstate.c
> +++ b/tests/test-vmstate.c
> @@ -506,9 +506,9 @@ static const VMStateDescription vmstate_vbuffer = {
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(u8_1, TestVBuffer),
> -        VMSTATE_VBUFFER(vBuffer_1, TestVBuffer, 1, NULL, 0,
> +        VMSTATE_VBUFFER(vBuffer_1, TestVBuffer, 1, NULL,
>                          buffer_size),
> -        VMSTATE_VBUFFER_ALLOC_UINT32(vBuffer_alloc_1, TestVBuffer, 1, NULL, 
> 0,
> +        VMSTATE_VBUFFER_ALLOC_UINT32(vBuffer_alloc_1, TestVBuffer, 1, NULL,
>                          buffer_alloc_size),
>          VMSTATE_UINT8(u8_2, TestVBuffer),
>          VMSTATE_END_OF_LIST()
> diff --git a/util/fifo8.c b/util/fifo8.c
> index 5c64101..d38b3bd 100644
> --- a/util/fifo8.c
> +++ b/util/fifo8.c
> @@ -118,7 +118,7 @@ const VMStateDescription vmstate_fifo8 = {
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_VBUFFER_UINT32(data, Fifo8, 1, NULL, 0, capacity),
> +        VMSTATE_VBUFFER_UINT32(data, Fifo8, 1, NULL, capacity),
>          VMSTATE_UINT32(head, Fifo8),
>          VMSTATE_UINT32(num, Fifo8),
>          VMSTATE_END_OF_LIST()
> -- 
> 2.8.4
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK