New VMState table based load/save 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.

Quoted 8 lines · Juan Quintela

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.

I think all the structures should be 'const'.

Also VMStateField and
VMStateDescription should be private to savevm.c.

hw/hw.h +20 -0

+enum VMStateType {
+    VMSTATE_TYPE_UNSPEC = 0,
+    VMSTATE_TYPE_INT8,
+    VMSTATE_TYPE_INT16,
+    VMSTATE_TYPE_INT32,
+    VMSTATE_TYPE_INT64,
+    VMSTATE_TYPE_UINT8,
+    VMSTATE_TYPE_UINT16,
+    VMSTATE_TYPE_UINT32,
+    VMSTATE_TYPE_UINT64,
+    VMSTATE_TYPE_TIMER,
+};
+
+typedef struct VMStateInfo VMStateInfo;
+
+struct VMStateInfo {
+    const char *name;
+    size_t size;
+    enum VMStateType type;
+    void (*get)(QEMUFile *f, VMStateInfo *info, void *pv);

Quoted 26 lines

hw/hw.h +26 -0

+enum VMStateType {
+    VMSTATE_TYPE_UNSPEC = 0,
+    VMSTATE_TYPE_INT8,
+    VMSTATE_TYPE_INT16,
+    VMSTATE_TYPE_INT32,
+    VMSTATE_TYPE_INT64,
+    VMSTATE_TYPE_UINT8,
+    VMSTATE_TYPE_UINT16,
+    VMSTATE_TYPE_UINT32,
+    VMSTATE_TYPE_UINT64,
+    VMSTATE_TYPE_TIMER,
+};
+
+typedef struct VMStateInfo VMStateInfo;
+
+struct VMStateInfo {
+    const char *name;
+    size_t size;
+    enum VMStateType type;
+    void (*get)(QEMUFile *f, VMStateInfo *info, void *pv);
+    void (*put)(QEMUFile *f, VMStateInfo *info, const void *pv);
+};
+
+typedef struct {
+    const char *name;
+    size_t num;

hw/hw.h typedef int QEMUBootSetHandler(void *opaque, const char *boot_devices); · line 301

Quoted 4 lines

hw/hw.h +4 -0

+    size_t offset;
+    VMStateInfo *info;
+    int version_id;
+} VMStateField;

Hmm, I'm not sure VMStateInfo is that useful here. All the get/put callbacks are simple two-liners, and it is unlikely that the number of types ever grows.

I think I would stick "enum VMStateType" directly into VMStateField and kill one level of indirection.

hw/hw.h typedef int QEMUBootSetHandler(void *opaque, const char *boot_devices); · line 308

Quoted 7 lines

hw/hw.h +7 -0

+
+typedef struct {
+    const char *name;
+    int version_id;
+    int minimum_version_id;
+    VMStateField *fields;
+} VMStateDescription;

Add compat_load() callback for version_id < minimum_version_id here?

Quoted 3 lines

Hmm, I'm not sure VMStateInfo is that useful here. All the get/put callbacks are simple two-liners, and it is unlikely that the number of types ever grows.

Thinking about this. VMStateType is not needed at all (see that it is not used in the patch). But Ilike the VMStateInfo struct approach. This is the easier way of dealing with structs. For instance pci_device_save(). My idea was to just create a new VMStateInfo for it, and then everything that needs to call pci_device_save() just add a field like:

VMSTATE_FILED(dev, ThisPCIDevice, verion_id, pci_vmstate_info, PCIDevice)

Quoted 7 lines · Reimar Döffinger

Hello,
sorry for replying in the middle of the thread, I was to fast and deleted the other mails already.
And just in case I mention I am new around here, so feel free to ignore me if you feel I am completely wrong.
One thing I don't like too much about it is that you can't really handle "calculated" fields.

Calculated fields are not by definition part of the state :) The state are the other fields that are used to save the state. I haven't yet seen calculade fields (but I haven't looked throughfully yet). With the current design, basically you can only save things that are in one struct (the way it is stored is an offset against a base address).

Quoted 6 lines

Saving both seems a bit like a bad design: the value of one implies the exact value of the other, so it is at least pointless.
More importantly (though I do not know if qemu intends to care about this) it might be able to hand-craft a saved vm that after loading then violates some assumptions of the emulation code, possibly being exploitable.

It is already that way. This design don't change anything. And I am not sure how to fix it. We don't have a "is this value safe for this field", around yet. It is possible to add some support for it, but I would like to 1st have an use case.

Quoted 4 lines

If nothing else, I'd at least add support for a "verify" function that gets a "const state *" and can abort loading the VM in case someone tries something evil (or can print some useful hint instead of having qemu crash silently on the user, possibly at some later time).

This is as different problem that is not solved in qemu either. I agree that it would be nice to have such a function, but I am not sure that I know how to do it. and what is worse. if you can modify the image, you can always change anything in the middle of the RAM. I don't really see too much point trying to get a verify function for devices, when we can't have it for RAM.

Quoted 9 lines · Juan Quintela

Quoted 7 lines · Reimar Döffinger

Hello,
sorry for replying in the middle of the thread, I was to fast and deleted the other mails already.
And just in case I mention I am new around here, so feel free to ignore me if you feel I am completely wrong.
One thing I don't like too much about it is that you can't really handle "calculated" fields.

Quoted 6 lines

Calculated fields are not by definition part of the state :) The state are the other fields that are used to save the state. I haven't yet seen calculade fields (but I haven't looked throughfully yet). With the current design, basically you can only save things that are in one struct (the way it is stored is an offset against a base address).

I am not sure we 100% understand each other, so I maybe tell the specific example.
I made a change to the eepro100 driver to fix dumping the network statistics.
The main problem is, depending on which device you emulate, the size of the statistics struct changes.
Since it looked ugly etc. I decided not to calculate the size of these statistics each time but instead save it in the device state. But instead of adding it to save_vm and load_vm (which also would change the version) I just set that statistics size again according to which device is emulated. This also assures that the emulated device and the statistics size always fit together, even if someone fiddles with

the saved state.
The "problem" with your approach if I understand it right is that I couldn't do that since the device never knows when it would have to re-fill these fields.
Basically what I am asking is if you couldn't just add an optional callback so some additional stuff can be done after the "basic" state has been loaded - or if that isn't desired at least a callback that allows verifying the loaded values and aborting execution.

Quoted 4 lines

It is already that way. This design don't change anything. And I am not sure how to fix it. We don't have a "is this value safe for this field", around yet. It is possible to add some support for it, but I would like to 1st have an use case.

Well, I meant nowadays it is just possible to add a check in load_vm and fix any values that are off. While it is quite a bit of work there is nothing in the API stopping you from doing it, you even can return -EINVAL and hopefully the core will print some somewhat useful message.

Quoted 11 lines

Quoted 4 lines

If nothing else, I'd at least add support for a "verify" function that gets a "const state *" and can abort loading the VM in case someone tries something evil (or can print some useful hint instead of having qemu crash silently on the user, possibly at some later time).

This is as different problem that is not solved in qemu either. I agree that it would be nice to have such a function, but I am not sure that I know how to do it. and what is worse. if you can modify the image, you can always change anything in the middle of the RAM. I don't really see too much point trying to get a verify function for devices, when we can't have it for RAM.

That is completely different from what I meant.
Changing the RAM compromises the VM and only the VM, an exploit in a device emulation might allow to compromise the _host_. Is it now clearer what I meant?

Are your changes on upstream hw/eepro100.c? I can't see anything there that can't be done in a table approach.

Quoted 5 lines

Quoted 4 lines

It is already that way. This design don't change anything. And I am not sure how to fix it. We don't have a "is this value safe for this field", around yet. It is possible to add some support for it, but I would like to 1st have an use case.

Quoted 4 lines

Well, I meant nowadays it is just possible to add a check in load_vm and fix any values that are off. While it is quite a bit of work there is nothing in the API stopping you from doing it, you even can return -EINVAL and hopefully the core will print some somewhat useful message.

I guess we are going to have an optional callback to be called before/after loading the state. You should be able to put your verify there.

Quoted 4 lines

That is completely different from what I meant.
Changing the RAM compromises the VM and only the VM, an exploit in a device emulation might allow to compromise the _host_. Is it now clearer what I meant?

yes, I see where you are meaning now. But I guess that one is needed to be solved, not only for migration. Not sure what to do about this.

Quoted 2 lines · Juan Quintela

Are your changes on upstream hw/eepro100.c? I can't see anything there that can't be done in a table approach.

No, so far noone got around to taking my patches apart (and that is actually one I have not yet properly submitted, it is mangled into that
patch: http://article.gmane.org/gmane.comp.emulators.qemu/49853

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

It is already that way. This design don't change anything. And I am not sure how to fix it. We don't have a "is this value safe for this field", around yet. It is possible to add some support for it, but I would like to 1st have an use case.

Well, I meant nowadays it is just possible to add a check in load_vm and fix any values that are off. While it is quite a bit of work there is nothing in the API stopping you from doing it, you even can return -EINVAL and hopefully the core will print some somewhat useful message.

Quoted 3 lines

I guess we are going to have an optional callback to be called before/after loading the state. You should be able to put your verify there.

Maybe I'm silly, but what would the callback for before loading state be good for?

Quoted 7 lines

Quoted 4 lines

That is completely different from what I meant.
Changing the RAM compromises the VM and only the VM, an exploit in a device emulation might allow to compromise the _host_. Is it now clearer what I meant?

yes, I see where you are meaning now. But I guess that one is needed to be solved, not only for migration. Not sure what to do about this.

I think it is mostly leg-work of finding the assumptions the emulations do. That really should be left to maintainers where available IMO. I'm just suggesting that it's better to design the API in a way that doesn't further discourage fixing this :-).
If the patch is close to being accepted maybe I can help out by writing such verification code for vmware_vga, there e.g. depth, bypp, wred, wgreen and wblue must fit together as well as width/height/new_width/new_height and fb_size (I think)
and width/height/bypp must be limit to ensure no integer overflows...

Basically what I am asking is if you couldn't just add an optional callback so some additional stuff can be done after the "basic" state has been loaded - or if that isn't desired at least a callback that allows verifying the loaded values and aborting execution.

That is completely different from what I meant.
Changing the RAM compromises the VM and only the VM, an exploit in a device emulation might allow to compromise the _host_. Is it now clearer what I meant?

Quoted 4 lines

Basically what I am asking is if you couldn't just add an optional callback so some additional stuff can be done after the "basic" state has been loaded - or if that isn't desired at least a callback that allows verifying the loaded values and aborting execution.

I think we are going to need post-processing callbacks anyway. Several drivers have to do some actions after loading the state information. There you'll be able to set the stats size and also perform sanity checks on the loaded state.

Quoted 4 lines

That is completely different from what I meant.
Changing the RAM compromises the VM and only the VM, an exploit in a device emulation might allow to compromise the _host_. Is it now clearer what I meant?

When you are able modify the savevm state you already have access to the host ...

Quoted 5 lines

Quoted 2 lines

When you are able modify the savevm state you already have access to the host ...

Huh? Being able to modify the savevm state is not the same as being able to run arbitrary code on the host.

Yes, in theory. And in practice? What is the point in allowing remote write access to savevm state?

Quoted 3 lines

Currently there is no way you could even consider running a savevm from an untrusted source, but I think that is just because of qemu's current implementation, not because it has to be.

Getting that right is a pretty big job though ...

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?

Quoted 5 lines

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.

Yes, we want. And I think the right approach to do that is to simply use the old, existing code and switch based on version_id. To stick with the apic example: When loadvm finds a v2 (or older) apic section, go call apic_load. When loadvm finds a v3 (or newer) apic section, call the new generic load code with the apic vmstatefield list.

Quoted 4 lines

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.

Yes, we want. PCI devices call a generic function to save pci state. We want a common pci vmstatefield list too and have some way to refer to them from the device tables.

Quoted 7 lines

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?

No. We want fixed-sized sections, bonus points for a 'size' field in the header. Just save everything unconditionally. The current save/load functions do way too much stuff conditionally. Saving a few bytes simply isn't worth the complexity of getting that right.

From msix.c

Quoted 3 lines

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?

Handle compatibility by keeping the old load functions and start over with a clean sheet.

void msix_save(PCIDevice *dev, QEMUFile *f)

{
     unsigned n = dev->msix_entries_nr;

     qemu_put_buffer(f, dev->msix_table_page, n * MSIX_ENTRY_SIZE);
     qemu_put_buffer(f, dev->msix_table_page + MSIX_PAGE_PENDING, (n + 7) / 8);
}

msix_table_page is not NULL only if QEMU_PCI_CAP_MSIX is present. I think that optional fields are needed, or a better way of dealing with things like this.