New VMState table based load/save infrastructure
cover New VMState table based load/save infrastructure
His this is 2nd version of the series, I changed:
v2:
* Add _V() constructors. We almost never need the version field.
* Add const to VMStateDescription uses (BlueWirl suggestion)
* Add const to VMStateInfo uses
* Remove VMStateInfo parameter from get/put callbacks. Not needed.
* Load of old versions is done with old foo_load function (Kraxel suggestion)
* Add struct support
* Move i8254 to new infrastructure, to test struct support
* I removed the autostart patches, updated version sent to list
i8254 note:
There is only one timer in the 1st channel, in the other two channels, the timer is not created ever, this is the reason why I sent the irq_timer not in the irq channels.
ToDo:
- still not optional test function. Notice that this can be done with a local VMStateInfo struct.
- Start/end functions, not done waiting for user case (next goal is virtio and I think we need them there).
- For structures, Kraxel suggested to use a VMSTATE_INCLUDE() instead of having to declare a new VMStateInfo struct. As I already have the new struct code working, I sent it with the struct. Thinking about how to implement the VMSTATE_INCLUDE() and which one is easier to use.
Comments?
v1:
This series implement a new save/load approach for device state. I want to start a discussion on how is better to design it. This is the reason why only one device has been ported to this new approach.
I increased the version_id of apic state, just to sent the 3 arrays (isr, tmr, irr) as arrays. I didn't want to introduce structure support before there is any discussion about this, not that it is imposible to do it in a compatible way.
Problems of current approach:
- have to maintain both load and save functions on sync
- it is not type safe
- you can't use easily the device save/load information to do more things. For instance, print the state of a device in the monitor. To do this, you basically have to copy foo_apic() function.
- You can't examine the saved images in any meaningful way
New approach:
- you only have to declare the state once, in a table, save/load functions are generic for all devices.
- it is type safe
- it is qdev-like design (i.e. you don't have to learn yet a completely different appearch for a similar problem)
- it is trivial to add new operations to VMStateInfo, and then you can use them anywhere (for instance printing in the monitor, etc).
Problems of new approach:
- it is less flexible, you can't progamatically save in any order that you want. It is not clear to me that we _want_ that flexibility. Look for instance and hw/msix.c and think if you can be sure that save/load are on sync, and as soon as we get new versions, that things could be tested to be wright.
What do I want to discuss:
a- do we want to be able to load state form old versions?
I am not sure that we are going to get this right for complex devices, and I don't completely see how we can have any kind of testing here. There was already a discussion about this on the list.
b- Is this the right approach? What more do we want/need?
For instance, implementing struct save support, and calling other "sub-descriptions" is not difficult, we just have to decide if we want it.
c- In the current approach, we have loops to send arrays, I think that one
got already done better on new approarch. But we don't have support for ifs (see hw/ide.c
if (s->identify_set) {
qemu_get_buffer(f, (uint8_t *)s->identify_data, 512);
} Do we want support for things like that?
d- how aggresive should the new design be? i.e. be able to be compatible with
old design is good, or can we start with a clean sheet and just remove the gotchas of the previous design?
Juan Quintela (3):
New VMstate save/load infrastructure
Port apic to new VMState design
Port i8254 to new VMState design
hw/apic.c | 67 ++++++------- hw/hw.h | 153 ++++++++++++++++++++++++++++ hw/i8254.c | 72 ++++++++------ savevm.c | 325 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 548 insertions(+), 69 deletions(-)
PATCH 1/3 New VMstate save/load infrastructure
This patch introduces VMState infrastructure, to convert the save/load functions of devices to a table approach. This new approach has the advantages:
- it is type-safe
- you can't have load/save functions out of sync
- will allows us to have new interesting commands, like dump <device>, that shows all its internal state.
- Just now, the only added type is arrays, but we can add structures.
Add support for loading old state using old foo_load() functions
--- hw/hw.h | 153 +++++++++++++++++++++++++++++ savevm.c | 325 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 472 insertions(+), 6 deletions(-)
hw/hw.h +153 -0
@@ -269,4 +269,157 @@ typedef int QEMUBootSetHandler(void *opaque, const char *boot_devices);void qemu_register_boot_set (QEMUBootSetHandler *func,void *opaque);int qemu_boot_set (const char *boot_devices); +typedef struct VMStateInfo VMStateInfo ; +typedef struct VMStateDescription VMStateDescription ; + +struct VMStateInfo { +const char *name; +const size_t size; +const VMStateDescription *vmsd; +void (*get)(QEMUFile *f,void *pv); +void (*put)(QEMUFile *f,const void *pv); +}; + +typedef struct { +const char *name; +size_t num; +size_t offset; +VMStateInfo *info; +int version_id; +}VMStateField ; + +struct VMStateDescription { +const char *name; +int version_id; +int minimum_version_id; +int minimum_version_id_old; +LoadStateHandler *load_state_old; +VMStateField *fields; +}; + +extern VMStateInfo vmstate_info_int8; +extern VMStateInfo vmstate_info_int16; +extern VMStateInfo vmstate_info_int32; +extern VMStateInfo vmstate_info_int64; + +extern VMStateInfo vmstate_info_uint8; +extern VMStateInfo vmstate_info_uint16; +extern VMStateInfo vmstate_info_uint32; +extern VMStateInfo vmstate_info_uint64; + +extern VMStateInfo vmstate_info_timer; + +#define typeof_field2 (type, field) typeof(((type *)0)->field) +#define type_check2 (t1,t2) ((t1*)0 - (t2*)0) +#define type_check_array (t1,t2,n) ((t1(*)[n])0 - (t2*)0) + +#define VMSTATE_FIELD (_field, _state, _version, _info, _type) { \ + .name = (stringify(_field)), \ + .num = 1, \ + .version_id = (_version), \ + .info = &(_info), \ + .offset = offsetof(_state, _field) \ + + type_check2(_type,typeof_field2(_state, _field)) \ +} + +#define VMSTATE_FIELD_A (_field, _state, _num, _version, _info, _type) { \ + .name = (stringify(_field)), \ + .num = (_num), \ + .version_id = (_version), \ + .info = &(_info), \ + .offset = offsetof(_state, _field) \ + + type_check_array(_type,typeof_field2(_state, _field),_num) \ +} + +/* _f : field name +_s : struct state name +_n : num of elements +_v : version +*/ + +#define VMSTATE_INT8_ARRAY_V (_f, _s, _n, _v) \ + VMSTATE_FIELD_A(_f, _s, _n, _v, vmstate_info_int8, int8_t) +#define VMSTATE_INT16_ARRAY_V (_f, _s, _n, _v) \ + VMSTATE_FIELD_A(_f, _s, _n, _v, vmstate_info_int16, int16_t) +#define VMSTATE_INT32_ARRAY_V (_f, _s, _n, _v) \ + VMSTATE_FIELD_A(_f, _s, _n, _v, vmstate_info_int32, int32_t) +#define VMSTATE_INT64_ARRAY_V (_f, _s, _n, _v) \ + VMSTATE_FIELD_A(_f, _s, _n, _v, vmstate_info_int64, int64_t) + +#define VMSTATE_UINT8_ARRAY_V (_f, _s, _n, _v) \ + VMSTATE_FIELD_A(_f, _s, _n, _v, vmstate_info_uint8, uint8_t) +#define VMSTATE_UINT16_ARRAY_V (_f, _s, _n, _v) \ + VMSTATE_FIELD_A(_f, _s, _n, _v, vmstate_info_uint16, uint16_t) +#define VMSTATE_UINT32_ARRAY_V (_f, _s, _n, _v) \ + VMSTATE_FIELD_A(_f, _s, _n, _v, vmstate_info_uint32, uint32_t) +#define VMSTATE_UINT64_ARRAY_V (_f, _s, _n, _v) \ + VMSTATE_FIELD_A(_f, _s, _n, _v, vmstate_info_uint64, uint64_t) + +#define VMSTATE_INT8_ARRAY (_f, _s, _n) \ + VMSTATE_INT8_ARRAY_V(_f, _s, _n, 0) +#define VMSTATE_INT16_ARRAY (_f, _s, _n) \ + VMSTATE_INT16_ARRAY_V(_f, _s, _n, 0) +#define VMSTATE_INT32_ARRAY (_f, _s, _n) \ + VMSTATE_INT32_ARRAY_V(_f, _s, _n, 0) +#define VMSTATE_INT64_ARRAY (_f, _s, _n) \ + VMSTATE_INT64_ARRAY_V(_f, _s, _n, 0) + +#define VMSTATE_UINT8_ARRAY (_f, _s, _n) \ + VMSTATE_UINT8_ARRAY_V(_f, _s, _n, 0) +#define VMSTATE_UINT16_ARRAY (_f, _s, _n) \ + VMSTATE_UINT16_ARRAY_V(_f, _s, _n, 0) +#define VMSTATE_UINT32_ARRAY (_f, _s, _n) \ + VMSTATE_UINT32_ARRAY_V(_f, _s, _n, 0) +#define VMSTATE_UINT64_ARRAY (_f, _s, _n) \ + VMSTATE_UINT64_ARRAY_V(_f, _s, _n, 0) + +#define VMSTATE_INT8_V (_f, _s, _v) \ + VMSTATE_FIELD(_f, _s, _v, vmstate_info_int8, int8_t) +#define VMSTATE_INT16_V (_f, _s, _v) \ + VMSTATE_FIELD(_f, _s, _v, vmstate_info_int16, int16_t) +#define VMSTATE_INT32_V (_f, _s, _v) \ + VMSTATE_FIELD(_f, _s, _v, vmstate_info_int32, int32_t) +#define VMSTATE_INT64_V (_f, _s, _v) \ + VMSTATE_FIELD(_f, _s, _v, vmstate_info_int64, int64_t) + +#define VMSTATE_UINT8_V (_f, _s, _v) \ + VMSTATE_FIELD(_f, _s, _v, vmstate_info_uint8, uint8_t) +#define VMSTATE_UINT16_V (_f, _s, _v) \ + VMSTATE_FIELD(_f, _s, _v, vmstate_info_uint16, uint16_t) +#define VMSTATE_UINT32_V (_f, _s, _v) \ + VMSTATE_FIELD(_f, _s, _v, vmstate_info_uint32, uint32_t) +#define VMSTATE_UINT64_V (_f, _s, _v) \ + VMSTATE_FIELD(_f, _s, _v, vmstate_info_uint64, uint64_t) + +#define VMSTATE_INT8 (_f, _s) \ + VMSTATE_INT8_V(_f, _s, 0) +#define VMSTATE_INT16 (_f, _s) \ + VMSTATE_INT16_V(_f, _s, 0) +#define VMSTATE_INT32 (_f, _s) \ + VMSTATE_INT32_V(_f, _s, 0) +#define VMSTATE_INT64 (_f, _s) \ + VMSTATE_INT64_V(_f, _s, 0) + +#define VMSTATE_UINT8 (_f, _s) \ + VMSTATE_UINT8_V(_f, _s, 0) +#define VMSTATE_UINT16 (_f, _s) \ + VMSTATE_UINT16_V(_f, _s, 0) +#define VMSTATE_UINT32 (_f, _s) \ + VMSTATE_UINT32_V(_f, _s, 0) +#define VMSTATE_UINT64 (_f, _s) \ + VMSTATE_UINT64_V(_f, _s, 0) + + +#define VMSTATE_TIMER_V (_f, _s, _v) \ + VMSTATE_FIELD(_f, _s, _v, vmstate_info_timer, QEMUTimer *) + +#define VMSTATE_TIMER (_f, _s) \ + VMSTATE_TIMER_V(_f, _s, 0) + +#define VMSTATE_END_OF_LIST () \ + {} + +extern int vmstate_register (int instance_id,const VMStateDescription *vmsd, +void *base); +extern void vmstate_unregister (const char *idstr,void *opaque);#endif
savevm.c +319 -6
390 lines not on this page · raw message
Signature · 1 line
1.6.2.5
PATCH 2/3 Port apic to new VMState design
In reply to Juan Quintela--- hw/apic.c | 67 ++++++++++++++++++++++++++++++------------------------------- 1 files changed, 33 insertions(+), 34 deletions(-)
hw/apic.c +33 -34
@@ -864,39 +864,8 @@ static void apic_mem_writel(void *opaque, target_phys_addr_t addr, uint32_t val) } } -static void apic_save (QEMUFile *f,void *opaque) -{ -APICState *s = opaque; -int i; - -qemu_put_be32s (f, &s->apicbase); -qemu_put_8s (f, &s->id); -qemu_put_8s (f, &s->arb_id); -qemu_put_8s (f, &s->tpr); -qemu_put_be32s (f, &s->spurious_vec); -qemu_put_8s (f, &s->log_dest); -qemu_put_8s (f, &s->dest_mode); -for (i =0 ; i <8 ; i++) { -qemu_put_be32s (f, &s->isr[i]); -qemu_put_be32s (f, &s->tmr[i]); -qemu_put_be32s (f, &s->irr[i]); - } -for (i =0 ; i <APIC_LVT_NB ; i++) { -qemu_put_be32s (f, &s->lvt[i]); - } -qemu_put_be32s (f, &s->esr); -qemu_put_be32s (f, &s->icr[0 ]); -qemu_put_be32s (f, &s->icr[1 ]); -qemu_put_be32s (f, &s->divide_conf); -qemu_put_be32 (f, s->count_shift); -qemu_put_be32s (f, &s->initial_count); -qemu_put_be64 (f, s->initial_count_load_time); -qemu_put_be64 (f, s->next_time); - -qemu_put_timer (f, s->timer); -} - -static int apic_load (QEMUFile *f,void *opaque,int version_id) +/* This function is only used for old state version 1 and 2 */ +static int apic_load_old (QEMUFile *f,void *opaque,int version_id) {APICState *s = opaque;int i;
@@ -934,6 +903,36 @@ static int apic_load(QEMUFile *f, void *opaque, int version_id)return 0 ; } +static const VMStateDescription vmstate_apic = { + .name ="apic" , + .version_id =3 , + .minimum_version_id =3 , + .minimum_version_id_old =1 , + .load_state_old = apic_load_old, + .fields = (VMStateField []) { +VMSTATE_UINT32 (apicbase, APICState), +VMSTATE_UINT8 (id, APICState), +VMSTATE_UINT8 (arb_id, APICState), +VMSTATE_UINT8 (tpr, APICState), +VMSTATE_UINT32 (spurious_vec, APICState), +VMSTATE_UINT8 (log_dest, APICState), +VMSTATE_UINT8 (dest_mode, APICState), +VMSTATE_UINT32_ARRAY (isr, APICState,8 ), +VMSTATE_UINT32_ARRAY (tmr, APICState,8 ), +VMSTATE_UINT32_ARRAY (irr, APICState,8 ), +VMSTATE_UINT32_ARRAY (lvt, APICState,APIC_LVT_NB ), +VMSTATE_UINT32 (esr, APICState), +VMSTATE_UINT32_ARRAY (icr, APICState,2 ), +VMSTATE_UINT32 (divide_conf, APICState), +VMSTATE_INT32 (count_shift, APICState), +VMSTATE_UINT32 (initial_count, APICState), +VMSTATE_INT64 (initial_count_load_time, APICState), +VMSTATE_INT64 (next_time, APICState), +VMSTATE_TIMER (timer, APICState), +VMSTATE_END_OF_LIST () + } +}; +static void apic_reset (void *opaque) {APICState *s = opaque;
@@ -996,7 +995,7 @@ int apic_init(CPUState *env) } s->timer =qemu_new_timer (vm_clock, apic_timer, s); -register_savevm ("apic" , s->idx,2 , apic_save, apic_load, s); +vmstate_register (s->idx, &vmstate_apic, s);qemu_register_reset (apic_reset, s); local_apics[s->idx] = s;
Signature · 1 line
1.6.2.5
PATCH 3/3 Port i8254 to new VMState design
In reply to Juan Quintela--- hw/i8254.c | 72 +++++++++++++++++++++++++++++++++++------------------------ 1 files changed, 43 insertions(+), 29 deletions(-)
hw/i8254.c +43 -29
@@ -389,35 +389,37 @@ static void pit_irq_timer(void *opaque)pit_irq_timer_update (s, s->next_transition_time); } -static void pit_save (QEMUFile *f,void *opaque) -{ -PITState *pit = opaque; -PITChannelState *s; -int i; - -for (i =0 ; i <3 ; i++) { - s = &pit->channels[i]; -qemu_put_be32 (f, s->count); -qemu_put_be16s (f, &s->latched_count); -qemu_put_8s (f, &s->count_latched); -qemu_put_8s (f, &s->status_latched); -qemu_put_8s (f, &s->status); -qemu_put_8s (f, &s->read_state); -qemu_put_8s (f, &s->write_state); -qemu_put_8s (f, &s->write_latch); -qemu_put_8s (f, &s->rw_mode); -qemu_put_8s (f, &s->mode); -qemu_put_8s (f, &s->bcd); -qemu_put_8s (f, &s->gate); -qemu_put_be64 (f, s->count_load_time); -if (s->irq_timer) { -qemu_put_be64 (f, s->next_transition_time); -qemu_put_timer (f, s->irq_timer); - } +static const VMStateDescription vmstate_pit_channel = { + .name ="pit channel" , + .version_id =2 , + .minimum_version_id =2 , + .minimum_version_id_old =2 , + .fields = (VMStateField []) { +VMSTATE_INT32 (count, PITChannelState), +VMSTATE_UINT16 (latched_count, PITChannelState), +VMSTATE_UINT8 (count_latched, PITChannelState), +VMSTATE_UINT8 (status_latched, PITChannelState), +VMSTATE_UINT8 (status, PITChannelState), +VMSTATE_UINT8 (read_state, PITChannelState), +VMSTATE_UINT8 (write_state, PITChannelState), +VMSTATE_UINT8 (write_latch, PITChannelState), +VMSTATE_UINT8 (rw_mode, PITChannelState), +VMSTATE_UINT8 (mode, PITChannelState), +VMSTATE_UINT8 (bcd, PITChannelState), +VMSTATE_UINT8 (gate, PITChannelState), +VMSTATE_INT64 (count_load_time, PITChannelState), +VMSTATE_INT64 (next_transition_time, PITChannelState), +VMSTATE_END_OF_LIST () } -} +}; + +VMStateInfo vmstate_info_pit_channel = { + .name ="channel" , + .size =sizeof (PITChannelState), + .vmsd = &vmstate_pit_channel, +}; -static int pit_load (QEMUFile *f,void *opaque,int version_id) +static int pit_load_old (QEMUFile *f,void *opaque,int version_id) {PITState *pit = opaque;PITChannelState *s;
@@ -449,6 +451,19 @@ static int pit_load(QEMUFile *f, void *opaque, int version_id)return 0 ; } +static const VMStateDescription vmstate_pit = { + .name ="i8254" , + .version_id =2 , + .minimum_version_id =2 , + .minimum_version_id_old =1 , + .load_state_old = pit_load_old, + .fields = (VMStateField []) { +VMSTATE_FIELD_A (channels, PITState,3 ,2 , vmstate_info_pit_channel, PITChannelState), +VMSTATE_TIMER (channels[0 ].irq_timer, PITState), +VMSTATE_END_OF_LIST () + } +}; +static void pit_reset (void *opaque) {PITState *pit = opaque;
@@ -495,8 +510,7 @@ PITState *pit_init(int base, qemu_irq irq) s->irq_timer =qemu_new_timer (vm_clock, pit_irq_timer, s); s->irq = irq; -register_savevm ("i8254" , base,1 , pit_save, pit_load, pit); - +vmstate_register (base, &vmstate_pit, pit);qemu_register_reset (pit_reset, pit);register_ioport_write (base,4 ,1 , pit_ioport_write, pit);register_ioport_read (base,3 ,1 , pit_ioport_read, pit);
Signature · 1 line
1.6.2.5
Hi,
Quoted 4 lines
- For structures, Kraxel suggested to use a
VMSTATE_INCLUDE()instead of having to declare a new VMStateInfo struct. As I already have the new struct code working, I sent it with the struct. Thinking about how to implement theVMSTATE_INCLUDE()and which one is easier to use.
Looked at your code and figured the two approaches are not that different. I'd just put the pointer to the chained/included VMStateDescription directly into the VMStateField to avoid the (IMHO pointless) indirection via VMStateInfo.
Also: It would probably useful to have two include modes here: One for embedded structs, i.e. this:
struct dev_state {
PCIDevice dev;
};
... and one for referenced structs, i.e. this:
struct dev2_state {
PCIDevice *dev;
};
cheers,
Gerd
Quoted 11 lines · Gerd Hoffmann
Hi,
Quoted 4 lines
- For structures, Kraxel suggested to use a
VMSTATE_INCLUDE()instead of having to declare a new VMStateInfo struct. As I already have the new struct code working, I sent it with the struct. Thinking about how to implement theVMSTATE_INCLUDE()and which one is easier to use.Looked at your code and figured the two approaches are not that different. I'd just put the pointer to the chained/included VMStateDescription directly into the VMStateField to avoid the (IMHO pointless) indirection via VMStateInfo.
Agreed that the appreaches are very similar, basically question of style.
Quoted 7 lines
Also: It would probably useful to have two include modes here: One for embedded structs, i.e. this:
struct
dev_state{
PCIDevice dev;
};
Quoted 5 lines
... and one for referenced structs, i.e. this:
struct
dev2_state{
PCIDevice *dev;
};
Trying to do pcibus_save() at the moment, and arrived at the conclusion that we need the VMSTATE_INCLUDE() like thing. Basically arrays need to be handled the same:
struct dev_state {
int32_t a[5];
}struct dev_state {
int32_t *a;
}I am working on getting something like:
STRUCT(...)STRUCT_P()ARRAY()ARRAY_P()
You get the idea :)
Later, Juan.