From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: Halil Pasic <pasic@linux.vnet.ibm.com>
Cc: qemu-devel@nongnu.org, Amit Shah <amit.shah@redhat.com>,
"quintela@redhat.com Guenther Hutzl" <hutzl@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH 4/4] migration: drop unused VMStateField.start
Date: Thu, 20 Oct 2016 13:00:00 +0100 [thread overview]
Message-ID: <20161020115959.GG2039@work-vm> (raw)
In-Reply-To: <20161018105724.26520-5-pasic@linux.vnet.ibm.com>
* Halil Pasic (pasic@linux.vnet.ibm.com) 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 <pasic@linux.vnet.ibm.com>
> Reviewed-by: Guenther Hutzl <hutzl@linux.vnet.ibm.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> 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 / dgilbert@redhat.com / Manchester, UK
next prev parent reply other threads:[~2016-10-20 12:00 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-18 10:57 [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start Halil Pasic
2016-10-18 10:57 ` [Qemu-devel] [PATCH 1/4] tests/test-vmstate.c: Add vBuffer test Halil Pasic
2016-10-20 11:52 ` Dr. David Alan Gilbert
2016-10-18 10:57 ` [Qemu-devel] [PATCH 2/4] tests/test-vmstate.c: prove VMStateField.start broken Halil Pasic
2016-10-18 13:27 ` Dr. David Alan Gilbert
2016-10-18 13:43 ` Halil Pasic
2016-10-18 13:54 ` Dr. David Alan Gilbert
2016-10-18 15:33 ` Halil Pasic
2016-10-18 18:32 ` Dr. David Alan Gilbert
2016-10-19 11:04 ` Halil Pasic
2016-10-20 12:00 ` Dr. David Alan Gilbert
2016-10-20 13:05 ` Halil Pasic
2016-10-18 10:57 ` [Qemu-devel] [PATCH 3/4] Revert "tests/test-vmstate.c: prove VMStateField.start broken" Halil Pasic
2016-10-18 10:57 ` [Qemu-devel] [PATCH 4/4] migration: drop unused VMStateField.start Halil Pasic
2016-10-20 12:00 ` Dr. David Alan Gilbert [this message]
2016-10-18 11:24 ` [Qemu-devel] [PATCH 0/4] remove unused VMSTateField.start no-reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161020115959.GG2039@work-vm \
--to=dgilbert@redhat.com \
--cc=amit.shah@redhat.com \
--cc=hutzl@linux.vnet.ibm.com \
--cc=pasic@linux.vnet.ibm.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).