| Message ID | 1330936245-2570-6-git-send-email-i.mitsyanko@samsung.com |
|---|---|
| State | New |
| Headers | show |
Am 05.03.2012 09:30, schrieb Igor Mitsyanko: > 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: Igor Mitsyanko <i.mitsyanko@samsung.com> Apart from this commit message being an overwhelmingly huge block of text ;) (maybe insert an empty line or two?) this had touched on the overall discussion of whether to pursue a declarative or imperative approach for VMState. So, what about adding this as a new, optional mechanism and leaving VBUFFER / BUFFER_MULTIPLY users untouched? Andreas [...] > diff --git a/vmstate.h b/vmstate.h > index 9d3c49c..a210e08 100644 > --- a/vmstate.h > +++ b/vmstate.h > @@ -73,8 +73,7 @@ enum VMStateFlags { > VMS_BUFFER = 0x020, /* static sized buffer */ > 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_VBUFFER = 0x100, /* Buffer with dynamically calculated size */ > VMS_VARRAY_UINT8 = 0x400, /* Array with size in uint8_t field*/ > VMS_VARRAY_UINT32 = 0x800, /* Array with size in uint32_t field*/ > }; > @@ -86,12 +85,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); > + size_t (*get_bufsize)(void *opaque, int version_id); > } VMStateField; > > typedef struct VMStateSubsection { > @@ -354,34 +353,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), \ > @@ -570,14 +546,11 @@ extern const VMStateInfo vmstate_info_unused_buffer; > #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)))
On 03/14/2012 04:55 PM, Andreas Färber wrote: > Am 05.03.2012 09:30, schrieb Igor Mitsyanko: >> 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: Igor Mitsyanko<i.mitsyanko@samsung.com> > > Apart from this commit message being an overwhelmingly huge block of > text ;) (maybe insert an empty line or two?) OK :) this had touched on the > overall discussion of whether to pursue a declarative or imperative > approach for VMState. > > So, what about adding this as a new, optional mechanism and leaving > VBUFFER / BUFFER_MULTIPLY users untouched? I don't mind. Now that we decided (in another thread) against live saving of wp_groups in SDState, the question with this patch arises again: do we need to introduce get_bufsize or we should just use additional member in SDState to hold wp_groups buffer size for use with current VMSTATE_VBUFFER_UINT32 macro?
diff --git a/hw/exynos4210_uart.c b/hw/exynos4210_uart.c index 73a9c18..cbd12e8 100644 --- a/hw/exynos4210_uart.c +++ b/hw/exynos4210_uart.c @@ -554,6 +554,13 @@ static void exynos4210_uart_reset(DeviceState *dev) PRINT_DEBUG("UART%d: Rx FIFO size: %d\n", s->channel, s->rx.size); } +static size_t exynos4210_uart_get_datasize(void *opaque, int id) +{ + Exynos4210UartFIFO *s = (Exynos4210UartFIFO *)opaque; + + return s->size; +} + static const VMStateDescription vmstate_exynos4210_uart_fifo = { .name = "exynos4210.uart.fifo", .version_id = 1, @@ -562,7 +569,8 @@ 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(data, Exynos4210UartFIFO, 1, NULL, 0, + exynos4210_uart_get_datasize), VMSTATE_END_OF_LIST() } }; diff --git a/hw/g364fb.c b/hw/g364fb.c index 3a0b68f..ccd4b76 100644 --- a/hw/g364fb.c +++ b/hw/g364fb.c @@ -491,6 +491,11 @@ static int g364fb_post_load(void *opaque, int version_id) return 0; } +static size_t g364fb_get_vramsize(void *opaque, int version_id) +{ + return ((G364State *)opaque)->vram_size; +} + static const VMStateDescription vmstate_g364fb = { .name = "g364fb", .version_id = 1, @@ -498,7 +503,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/m48t59.c b/hw/m48t59.c index 60bbb00..34355c3 100644 --- a/hw/m48t59.c +++ b/hw/m48t59.c @@ -582,6 +582,11 @@ static const MemoryRegionOps nvram_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; +static size_t 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..220e745 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 size_t 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 db6af68..0da8a0a 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 size_t 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 80be1ff..333e67a 100644 --- a/savevm.c +++ b/savevm.c @@ -1413,9 +1413,12 @@ 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; + if (field->get_bufsize) { + size = field->get_bufsize(opaque, version_id); + size > field->start ? (size -= field->start) : (size = 0); + } else { + error_report("%s vmstate load error: couldn't get " + "buffer %s size", vmsd->name, field->name); } } if (field->flags & VMS_ARRAY) { @@ -1477,9 +1480,12 @@ 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; + if (field->get_bufsize) { + size = field->get_bufsize(opaque, vmsd->version_id); + size > field->start ? (size -= field->start) : (size = 0); + } else { + error_report("%s vmstate save error: couldn't get " + "buffer %s size", vmsd->name, field->name); } } if (field->flags & VMS_ARRAY) { diff --git a/vmstate.h b/vmstate.h index 9d3c49c..a210e08 100644 --- a/vmstate.h +++ b/vmstate.h @@ -73,8 +73,7 @@ enum VMStateFlags { VMS_BUFFER = 0x020, /* static sized buffer */ 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_VBUFFER = 0x100, /* Buffer with dynamically calculated size */ VMS_VARRAY_UINT8 = 0x400, /* Array with size in uint8_t field*/ VMS_VARRAY_UINT32 = 0x800, /* Array with size in uint32_t field*/ }; @@ -86,12 +85,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); + size_t (*get_bufsize)(void *opaque, int version_id); } VMStateField; typedef struct VMStateSubsection { @@ -354,34 +353,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), \ @@ -570,14 +546,11 @@ extern const VMStateInfo vmstate_info_unused_buffer; #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)))
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: Igor Mitsyanko <i.mitsyanko@samsung.com> --- hw/exynos4210_uart.c | 10 +++++++++- hw/g364fb.c | 7 ++++++- hw/m48t59.c | 7 ++++++- hw/mac_nvram.c | 8 +++++++- hw/onenand.c | 7 ++++++- savevm.c | 18 ++++++++++++------ vmstate.h | 43 ++++++++----------------------------------- 7 files changed, 54 insertions(+), 46 deletions(-)