virtio migration: simplify vmstate helper
cover virtio migration: simplify vmstate helper
As a part of the long term effort to convert migration to vmstate the migration of virtio devices was recently partially switched to vmstate starting with the outer layer (commit 5943124cc "virtio: Migration helper function and macro" and the subsequent "virtio-*: Wrap in vmstate" commits). This was done by introducing a vmstate based wrapper driving the well know virtio migration process. As this transition is still in progress, some synergies were left unexploited, and some things can be expressed in a more vmstatish way.
Let us simplify a couple of things and get rid of some code duplication.
Did only a couple of smoke tests. Comprehensive testing is still to be done.
Halil Pasic (11):
virtio: add VIRTIO_DEF_DEVICE_VMSD macro
virtio-blk: convert to VIRTIO_DEF_DEVICE_VMSD
virtio-net: convert to VIRTIO_DEF_DEVICE_VMSD
virtio-9p: convert to VIRTIO_DEF_DEVICE_VMSD
virtio-serial: convert to VIRTIO_DEF_DEVICE_VMSD
virtio-gpu: do not use VMSTATE_VIRTIO_DEVICE
virtio-input: convert to VIRTIO_DEF_DEVICE_VMSD
virtio-scsi: convert to VIRTIO_DEF_DEVICE_VMSD
virtio-balloon: convert to VIRTIO_DEF_DEVICE_VMSD
virtio-rng: convert to VIRTIO_DEF_DEVICE_VMSD
virtio: remove unused VMSTATE_VIRTIO_DEVICE
hw/9pfs/virtio-9p-device.c | 7 +------ hw/block/virtio-blk.c | 17 +---------------- hw/char/virtio-serial-bus.c | 8 +------- hw/display/virtio-gpu.c | 39 +++++++++++++++++++++++++++------------ hw/input/virtio-input.c | 13 +++---------- hw/net/virtio-net.c | 32 +++++++++++--------------------- hw/scsi/virtio-scsi.c | 18 +----------------- hw/virtio/virtio-balloon.c | 7 +------ hw/virtio/virtio-rng.c | 10 ++-------- hw/virtio/virtio.c | 11 ++++++++++- include/hw/virtio/virtio.h | 31 ++++++++++++++++++------------- 11 files changed, 76 insertions(+), 117 deletions(-)
Signature · 1 line
2.8.4
PATCH 1/12 virtio: add VIRTIO_DEF_DEVICE_VMSD macro
In most cases the functions passed to VMSTATE_VIRTIO_DEVICE only call the virtio_load and virtio_save wrappers. Some include some pre- and post- massaging too. The massaging is better expressed as such in the VMStateDescription.
Let us introduce a new macro called VIRTIO_DEF_DEVICE_VMSD and replace VMSTATE_VIRTIO_DEVICE with it gradually.
--- hw/virtio/virtio.c | 15 +++++++++++++++ include/hw/virtio/virtio.h | 25 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+)
hw/virtio/virtio.c +15 -0
@@ -1622,6 +1622,21 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)virtio_save (VIRTIO_DEVICE (opaque), f); } +/* A wrapper for use as a VMState .put function */ +void virtio_save_as_vmsi_put (QEMUFile *f,void *opaque,size_t size) +{ +virtio_save (VIRTIO_DEVICE (opaque), f); +} + +/* A wrapper for use as a VMState .get function */ +int virtio_load_as_vmsi_get (QEMUFile *f,void *opaque,size_t size) +{ +VirtIODevice *vdev =VIRTIO_DEVICE (opaque); +DeviceClass *dc =DEVICE_CLASS (VIRTIO_DEVICE_GET_CLASS (vdev)); + +return virtio_load (vdev, f, dc->vmsd->version_id); +} +static int virtio_set_features_nocheck (VirtIODevice *vdev,uint64_t val) {VirtioDeviceClass *k =VIRTIO_DEVICE_GET_CLASS (vdev);
include/hw/virtio/virtio.h +25 -0
@@ -176,6 +176,31 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);void virtio_save (VirtIODevice *vdev,QEMUFile *f);void virtio_vmstate_save (QEMUFile *f,void *opaque,size_t size); +void virtio_save_as_vmsi_put (QEMUFile *f,void *opaque,size_t size); +int virtio_load_as_vmsi_get (QEMUFile *f,void *opaque,size_t size); + +#define VMSTATE_VIRTIO_FIELD \ + { \ + .name = "virtio", \ + .info = &(const VMStateInfo) { \ + .name = "virtio", \ + .get = virtio_load_as_vmsi_get, \ + .put = virtio_save_as_vmsi_put, \ + }, \ + .flags = VMS_SINGLE, \ + } + +#define VIRTIO_DEF_DEVICE_VMSD (devname, v, ...) \ + static const VMStateDescription vmstate_virtio_ ## devname = { \ + .name = "virtio-" #devname , \ + .minimum_version_id = v, \ + .version_id = v, \ + .fields = (VMStateField[]) { \ + VMSTATE_VIRTIO_FIELD, \ + VMSTATE_END_OF_LIST() \ + }, \ + __VA_ARGS__ \ + };#define VMSTATE_VIRTIO_DEVICE (devname, v, getf, putf) \ static const VMStateDescription vmstate_virtio_ ## devname = { \
Signature · 1 line
2.8.4
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
Quoted 45 lines · Halil Pasic
In most cases the functions passed to
VMSTATE_VIRTIO_DEVICEonly call thevirtio_loadandvirtio_savewrappers. Some include some pre- and post- massaging too. The massaging is better expressed as such in the VMStateDescription.Let us introduce a new macro called
VIRTIO_DEF_DEVICE_VMSDand replaceVMSTATE_VIRTIO_DEVICEwith it gradually.Signed-off-by Halil Pasic <[email protected]>--- hw/virtio/virtio.c | 15 +++++++++++++++ include/hw/virtio/virtio.h | 25 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+)hw/virtio/virtio.c +15
-0@@ -1622,6 +1622,21 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)virtio_save (VIRTIO_DEVICE (opaque), f); } +/* A wrapper for use as a VMState .put function */ +void virtio_save_as_vmsi_put (QEMUFile *f,void *opaque,size_t size) +{ +virtio_save (VIRTIO_DEVICE (opaque), f); +} + +/* A wrapper for use as a VMState .get function */ +int virtio_load_as_vmsi_get (QEMUFile *f,void *opaque,size_t size) +{ +VirtIODevice *vdev =VIRTIO_DEVICE (opaque); +DeviceClass *dc =DEVICE_CLASS (VIRTIO_DEVICE_GET_CLASS (vdev)); + +return virtio_load (vdev, f, dc->vmsd->version_id); +} +static int virtio_set_features_nocheck (VirtIODevice *vdev,uint64_t val) {VirtioDeviceClass *k =VIRTIO_DEVICE_GET_CLASS (vdev);include/hw/virtio/virtio.h +0
-0
include/hw/virtio/virtio.h void virtio_notify(VirtIODevice *vdev, VirtQueue *vq); · line 180
Quoted 5 lines
include/hw/virtio/virtio.h +2
-0@@ -176,6 +176,31 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);void virtio_save (VirtIODevice *vdev,QEMUFile *f);void virtio_vmstate_save (QEMUFile *f,void *opaque,size_t size); +void virtio_save_as_vmsi_put (QEMUFile *f,void *opaque,size_t size); +int virtio_load_as_vmsi_get (QEMUFile *f,void *opaque,size_t size);
These can be called virtio_device_get and virtio_device_put, and can be static if...
include/hw/virtio/virtio.h void virtio_notify(VirtIODevice *vdev, VirtQueue *vq); · line 191
Quoted 11 lines
include/hw/virtio/virtio.h +11
-0+ +#define VMSTATE_VIRTIO_FIELD \ + { \ + .name = "virtio", \ + .info = &(const VMStateInfo) { \ + .name = "virtio", \ + .get = virtio_load_as_vmsi_get, \ + .put = virtio_save_as_vmsi_put, \ + }, \ + .flags = VMS_SINGLE, \ + }
... you only export the VMStateInfo.
Also please define the macro like the rest of the VMSTATE macros, i.e.
#define VMSTATE_PCI_DEVICE(_field, _state) { \
.name = (stringify(_field)), \
.size = sizeof(VirtIODevice), \
.vmsd = &vmstate_virtio_device, \
.flags = VMS_SINGLE, \
.offset = vmstate_offset_value(_state, _field, VirtIODevice), \}
include/hw/virtio/virtio.h void virtio_notify(VirtIODevice *vdev, VirtQueue *vq); · line 203
Quoted 11 lines
include/hw/virtio/virtio.h +11
-0+#define VIRTIO_DEF_DEVICE_VMSD (devname, v, ...) \ + static const VMStateDescription vmstate_virtio_ ## devname = { \ + .name = "virtio-" #devname , \ + .minimum_version_id = v, \ + .version_id = v, \ + .fields = (VMStateField[]) { \ + VMSTATE_VIRTIO_FIELD, \ + VMSTATE_END_OF_LIST() \ + }, \ + __VA_ARGS__ \ + };
and here, don't use the macro, using its expansion everywhere instead.
Paolo
Quoted 3 lines
include/hw/virtio/virtio.h +0
-0#define VMSTATE_VIRTIO_DEVICE (devname, v, getf, putf) \ static const VMStateDescription vmstate_virtio_ ## devname = { \
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
Quoted 57 lines · Paolo Bonzini
Quoted 45 lines · Halil Pasic
In most cases the functions passed to
VMSTATE_VIRTIO_DEVICEonly call thevirtio_loadandvirtio_savewrappers. Some include some pre- and post- massaging too. The massaging is better expressed as such in the VMStateDescription.Let us introduce a new macro called
VIRTIO_DEF_DEVICE_VMSDand replaceVMSTATE_VIRTIO_DEVICEwith it gradually.Signed-off-by Halil Pasic <[email protected]>--- hw/virtio/virtio.c | 15 +++++++++++++++ include/hw/virtio/virtio.h | 25 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+)hw/virtio/virtio.c +15
-0@@ -1622,6 +1622,21 @@ void virtio_vmstate_save(QEMUFile *f, void *opaque, size_t size)virtio_save (VIRTIO_DEVICE (opaque), f); } +/* A wrapper for use as a VMState .put function */ +void virtio_save_as_vmsi_put (QEMUFile *f,void *opaque,size_t size) +{ +virtio_save (VIRTIO_DEVICE (opaque), f); +} + +/* A wrapper for use as a VMState .get function */ +int virtio_load_as_vmsi_get (QEMUFile *f,void *opaque,size_t size) +{ +VirtIODevice *vdev =VIRTIO_DEVICE (opaque); +DeviceClass *dc =DEVICE_CLASS (VIRTIO_DEVICE_GET_CLASS (vdev)); + +return virtio_load (vdev, f, dc->vmsd->version_id); +} +static int virtio_set_features_nocheck (VirtIODevice *vdev,uint64_t val) {VirtioDeviceClass *k =VIRTIO_DEVICE_GET_CLASS (vdev);include/hw/virtio/virtio.h +0
-0include/hw/virtio/virtio.h void virtio_notify(VirtIODevice *vdev, VirtQueue *vq); · line 180
Quoted 5 lines
include/hw/virtio/virtio.h +2
-0@@ -176,6 +176,31 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);void virtio_save (VirtIODevice *vdev,QEMUFile *f);void virtio_vmstate_save (QEMUFile *f,void *opaque,size_t size); +void virtio_save_as_vmsi_put (QEMUFile *f,void *opaque,size_t size); +int virtio_load_as_vmsi_get (QEMUFile *f,void *opaque,size_t size);These can be called
virtio_device_getandvirtio_device_put, and can be static if...
Quoted 14 lines
include/hw/virtio/virtio.h void virtio_notify(VirtIODevice *vdev, VirtQueue *vq); · line 191
Quoted 11 lines
include/hw/virtio/virtio.h +11
-0+ +#define VMSTATE_VIRTIO_FIELD \ + { \ + .name = "virtio", \ + .info = &(const VMStateInfo) { \ + .name = "virtio", \ + .get = virtio_load_as_vmsi_get, \ + .put = virtio_save_as_vmsi_put, \ + }, \ + .flags = VMS_SINGLE, \ + }... you only export the VMStateInfo.
Hi Paolo,
thanks I missed that, you are totally right.
Quoted 11 lines
Also please define the macro like the rest of the VMSTATE macros, i.e.
#define VMSTATE_PCI_DEVICE(_field, _state) { \ .name = (stringify(_field)), \ .size = sizeof(VirtIODevice), \ .vmsd = &vmstate_virtio_device, \ .flags = VMS_SINGLE, \ .offset = vmstate_offset_value(_state, _field, VirtIODevice), \}
I'm not sure if I understand where are you coming from, but if I do, I think I have to disagree here. I think you are coming from the normal device inheritance direction, where you have the parent storage object (that is its instance data( embedded into the child, and the corresponding parent state is supposed to get migrated as a (vmstate) field of the child.
Now if you look at the documentation of virtio migration (or the code) it is pretty obvious that the situation is very different for virtio. Virtio migration is kind of a template method approach (with virtio_load and virtio_save being the template methods), and the parent (core, transport) and the child (device specific) data are intermingled (the device data is in the middle). We can not change this for compatibility reasons (sadly).
Thus I would find using the usual conventions rather misleading here. But as I said, my understanding may be wrong, so if it is, please correct me.
Quoted 12 lines
include/hw/virtio/virtio.h void virtio_notify(VirtIODevice *vdev, VirtQueue *vq); · line 203
Quoted 11 lines
include/hw/virtio/virtio.h +11
-0+#define VIRTIO_DEF_DEVICE_VMSD (devname, v, ...) \ + static const VMStateDescription vmstate_virtio_ ## devname = { \ + .name = "virtio-" #devname , \ + .minimum_version_id = v, \ + .version_id = v, \ + .fields = (VMStateField[]) { \ + VMSTATE_VIRTIO_FIELD, \ + VMSTATE_END_OF_LIST() \ + }, \ + __VA_ARGS__ \ + };
Quoted 3 lines
and here, don't use the macro, using its expansion everywhere instead.
From the above I think it is clear, that I would like to keep this macro.
This macro is direct substitute for VMSTATE_VIRTIO_DEVICE, so I really do not get what is the difference, and why should I use an expansion everywhere instead, when the previous macro was fine and received no criticism during the reviews. I think the only thing we gain with the expansion is code duplication. If you insist on this, could you provide some background, so I'm also able to understand the why?
Cheers,
Halil
Quoted 7 lines
Paolo
Quoted 3 lines
include/hw/virtio/virtio.h +0
-0#define VMSTATE_VIRTIO_DEVICE (devname, v, getf, putf) \ static const VMStateDescription vmstate_virtio_ ## devname = { \
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
Quoted 15 lines · Halil Pasic
Quoted 9 lines
Quoted 9 lines
#define VMSTATE_PCI_DEVICE(_field, _state) { \ .name = (stringify(_field)), \ .size = sizeof(VirtIODevice), \ .vmsd = &vmstate_virtio_device, \ .flags = VMS_SINGLE, \ .offset = vmstate_offset_value(_state, _field, VirtIODevice), \}
I'm not sure if I understand where are you coming from, but if I do, I think I have to disagree here. I think you are coming from the normal device inheritance direction, where you have the parent storage object (that is its instance data( embedded into the child, and the corresponding parent state is supposed to get migrated as a (vmstate) field of the child.
Quoted 6 lines
Now if you look at the documentation of virtio migration (or the code) it is pretty obvious that the situation is very different for virtio. Virtio migration is kind of a template method approach (with
virtio_loadandvirtio_savebeing the template methods), and the parent (core, transport) and the child (device specific) data are intermingled (the device data is in the middle). We can not change this for compatibility reasons (sadly).
Regarding VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things differently for no particular reason. Your macro is already doing exactly the same as other VMSTATE_* macros, just with different conventions for the arguments. I don't see any advantage in changing that.
Regarding VIRTIO_DEF_DEVICE_VMSD, it's true that virtio migration is currently done differently and we will definitely have to do things somewhat different due to compatibility, but we can at least evolve towards having a normal VMStateDescription (virtio-gpu is already more similar to this). Having everything hidden behind a magic macro makes things harder to follow than other vmstate definitions. It's okay when you have a very small veneer as in commit 5943124cc, but in my opinion having field definitions inside macro arguments is already too much.
Paolo
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
Hi Paolo,
I'm sorry, but I do not get it quite yet, or more exactly I have the feeling I did not manage to bring my point over. So I will try with more details.
Quoted 4 lines · Paolo Bonzini
Quoted 1 line · Halil Pasic
Quoted 1 line
Quoted 1 line
#define
VMSTATE_PCI_DEVICE(_field, _state){
This was probably supposed to be VMSTATE_VIRTIO_DEVICE.
\
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
.name = (stringify(_field)), \ .size = sizeof(VirtIODevice), \ .vmsd = &vmstate_virtio_device, \
This one does not exist at least very tricky to write because of the peculiarities of virtio migration. This one would need to migrate the transport stuff too. And the specialized device to, which is not normal.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
.flags = VMS_SINGLE, \ .offset = vmstate_offset_value(_state, _field, VirtIODevice), \
This is useless if we keep virtio_save and virtio_load.
Quoted 16 lines
Quoted 9 lines
Quoted 3 lines
Quoted 3 lines
}
I'm not sure if I understand where are you coming from, but if I do, I think I have to disagree here. I think you are coming from the normal device inheritance direction, where you have the parent storage object (that is its instance data( embedded into the child, and the corresponding parent state is supposed to get migrated as a (vmstate) field of the child.
Quoted 6 lines
Now if you look at the documentation of virtio migration (or the code) it is pretty obvious that the situation is very different for virtio. Virtio migration is kind of a template method approach (with
virtio_loadandvirtio_savebeing the template methods), and the parent (core, transport) and the child (device specific) data are intermingled (the device data is in the middle). We can not change this for compatibility reasons (sadly).
Quoted 4 lines
Regarding
VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things differently for no particular reason. Your macro is already doing exactly the same as other VMSTATE_* macros, just with different conventions for the arguments. I don't see any advantage in changing that.
In my opinion it is not the same. In the case of VMSTATE_PCI_DEVICE we have (a self contained) parent (in sense of inheritance) device, which is embedded as _field in the specialized device and is migrated by the vmstatedescription of the parent. The rest of the specialized devices state is represented by the other fields.
VMSTATE_VIRTIO_FIELD is however just there to make sure virtio_load and virtio_save are called at the right time with the right arguments. The specialized device is then migrated by the save/load callbacks of the device class, or the vmsd residing in the device class. VMSTATE_VIRTIO_FIELD is supposed to be the only field, if the virtio device adheres to the virtio-migration document. VMSTATE_VIRTIO_FIELD has no arguments because it is
a virtual field and does not depend on the offset stuff.
To summarize currently I have no idea how to write up the vmstate field definition macro VMSTATE_VIRTIO_DEVICE so that it meets your expectations.
Quoted 6 lines
Regarding
VIRTIO_DEF_DEVICE_VMSD, it's true that virtio migration is currently done differently and we will definitely have to do things somewhat different due to compatibility, but we can at least evolve towards having a normal VMStateDescription (virtio-gpu is already more similar to this).
Virtio-gpu does not adhere to the virtio-migration document because it saves the specialized device state after the virtio_save is done (that is after the common virtio subsections). This is however more like the normal approach -- first you save the parent, then the child.
Quoted 2 lines
Having everything hidden behind a magic macro makes things harder to follow than other vmstate definitions.
Again in my opinion the virtio migration is different that the rest of the vmstate migration stuff, and it's ugly to some extent. So the idea was to make it look different and hide the ugliness behind the macro. If you insist i will create a version where the macros are expanded so it's easier to say if this improves or worsens the readability.
Quoted 3 lines
It's okay when
you have a very small veneer as in commit5943124cc, but in my opinion having field definitions inside macro arguments is already too much.
I think this is matter of taste, and your taste matters more ;). I do agree that the variadic for the massaging functions is more complicated that the two function pointers taken by VMSTATE_VIRTIO_DEVICE. My idea was that we end up with more readable code on the caller-side, but if you prefer function pointers and NULLs if no callback is needed needed (most cases), I can live with that.
Sorry again for my thick head.
Cheers,
Halil
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
Quoted 6 lines · Halil Pasic
Hi Paolo,
I'm sorry, but I do not get it quite yet, or more exactly I have the feeling I did not manage to bring my point over. So I will try with more details.
Quoted 7 lines
Quoted 4 lines · Paolo Bonzini
Quoted 1 line · Halil Pasic
Quoted 1 line
Quoted 1 line
#define
VMSTATE_PCI_DEVICE(_field, _state){This was probably supposed to be
VMSTATE_VIRTIO_DEVICE.
Yes.
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
.name = (stringify(_field)), \ .size = sizeof(VirtIODevice), \ .vmsd = &vmstate_virtio_device, \This one does not exist at least very tricky to write because of the peculiarities of virtio migration. This one would need to migrate the transport stuff too. And the specialized device to, which is not normal.
This is my own typo - this should be .info. Sorry.
Quoted 19 lines
Quoted 4 lines
Regarding
VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things differently for no particular reason. Your macro is already doing exactly the same as other VMSTATE_* macros, just with different conventions for the arguments. I don't see any advantage in changing that.In my opinion it is not the same. In the case of
VMSTATE_PCI_DEVICEwe have (a self contained) parent (in sense of inheritance) device, which is embedded as_fieldin the specialized device and is migrated by the vmstatedescription of the parent. The rest of the specialized devices state is represented by the other fields.
VMSTATE_VIRTIO_FIELDis however just there to make surevirtio_loadandvirtio_saveare called at the right time with the right arguments. The specialized device is then migrated by the save/load callbacks of the device class, or the vmsd residing in the device class.VMSTATE_VIRTIO_FIELDis supposed to be the only field, if the virtio device adheres to the virtio-migration document.VMSTATE_VIRTIO_FIELDhas no arguments because it is
a virtual field and does not depend on the offset stuff.
Quoted 3 lines
To summarize currently I have no idea how to write up the vmstate field definition macro
VMSTATE_VIRTIO_DEVICEso that it meets your expectations.
As above.
Quoted 8 lines
Quoted 2 lines
Having everything hidden behind a magic macro makes things harder to follow than other vmstate definitions.
Again in my opinion the virtio migration is different that the rest of the vmstate migration stuff, and it's ugly to some extent. So the idea was to make it look different and hide the ugliness behind the macro. If you insist i will create a version where the macros are expanded so it's easier to say if this improves or worsens the readability.
I agree it is not exactly the same as the other devices. But in my opinion it's not different-enough to do everything completely more, and in the future we should aim at making it less different.
Quoted 6 lines
I think this is matter of taste, and your taste matters more ;). I do agree that the variadic for the massaging functions is more complicated that the two function pointers taken by
VMSTATE_VIRTIO_DEVICE. My idea was that we end up with more readable code on the caller-side, but if you prefer function pointers and NULLs if no callback is needed needed (most cases), I can live with that.
Well, the third possibility would be expanding the VMStateDescription definition, :) where .post_load becomes just yet another initializer.
Paolo
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
Quoted 74 lines · Paolo Bonzini
Quoted 6 lines · Halil Pasic
Hi Paolo,
I'm sorry, but I do not get it quite yet, or more exactly I have the feeling I did not manage to bring my point over. So I will try with more details.
Quoted 7 lines
Quoted 4 lines · Paolo Bonzini
Quoted 1 line · Halil Pasic
Quoted 1 line
Quoted 1 line
#define
VMSTATE_PCI_DEVICE(_field, _state){This was probably supposed to be
VMSTATE_VIRTIO_DEVICE.Yes.
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
.name = (stringify(_field)), \ .size = sizeof(VirtIODevice), \ .vmsd = &vmstate_virtio_device, \This one does not exist at least very tricky to write because of the peculiarities of virtio migration. This one would need to migrate the transport stuff too. And the specialized device to, which is not normal.
This is my own typo - this should be .info. Sorry.
Quoted 19 lines
Quoted 4 lines
Regarding
VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things differently for no particular reason. Your macro is already doing exactly the same as other VMSTATE_* macros, just with different conventions for the arguments. I don't see any advantage in changing that.In my opinion it is not the same. In the case of
VMSTATE_PCI_DEVICEwe have (a self contained) parent (in sense of inheritance) device, which is embedded as_fieldin the specialized device and is migrated by the vmstatedescription of the parent. The rest of the specialized devices state is represented by the other fields.
VMSTATE_VIRTIO_FIELDis however just there to make surevirtio_loadandvirtio_saveare called at the right time with the right arguments. The specialized device is then migrated by the save/load callbacks of the device class, or the vmsd residing in the device class.VMSTATE_VIRTIO_FIELDis supposed to be the only field, if the virtio device adheres to the virtio-migration document.VMSTATE_VIRTIO_FIELDhas no arguments because it is
a virtual field and does not depend on the offset stuff.Quoted 3 lines
To summarize currently I have no idea how to write up the vmstate field definition macro
VMSTATE_VIRTIO_DEVICEso that it meets your expectations.As above.
Quoted 8 lines
Quoted 2 lines
Having everything hidden behind a magic macro makes things harder to follow than other vmstate definitions.
Again in my opinion the virtio migration is different that the rest of the vmstate migration stuff, and it's ugly to some extent. So the idea was to make it look different and hide the ugliness behind the macro. If you insist i will create a version where the macros are expanded so it's easier to say if this improves or worsens the readability.
I agree it is not exactly the same as the other devices. But in my opinion it's not different-enough to do everything completely more, and in the future we should aim at making it less different.
Quoted 6 lines
I think this is matter of taste, and your taste matters more ;). I do agree that the variadic for the massaging functions is more complicated that the two function pointers taken by
VMSTATE_VIRTIO_DEVICE. My idea was that we end up with more readable code on the caller-side, but if you prefer function pointers and NULLs if no callback is needed needed (most cases), I can live with that.
Quoted 5 lines
Well, the third possibility would be expanding the VMStateDescription definition, :) where .post_load becomes just yet another initializer.
Paolo
Hi Paolo,
clear now. I will come back with a v2 with the suggestions implemented. Thank you very much for your time.
Cheers,
Halil
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
In reply to Paolo BonziniQuoted 74 lines · * Paolo Bonzini ([email protected]) wrote:
Quoted 6 lines · Halil Pasic
Hi Paolo,
I'm sorry, but I do not get it quite yet, or more exactly I have the feeling I did not manage to bring my point over. So I will try with more details.
Quoted 7 lines
Quoted 4 lines · Paolo Bonzini
Quoted 1 line · Halil Pasic
Quoted 1 line
Quoted 1 line
#define
VMSTATE_PCI_DEVICE(_field, _state){This was probably supposed to be
VMSTATE_VIRTIO_DEVICE.Yes.
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
.name = (stringify(_field)), \ .size = sizeof(VirtIODevice), \ .vmsd = &vmstate_virtio_device, \This one does not exist at least very tricky to write because of the peculiarities of virtio migration. This one would need to migrate the transport stuff too. And the specialized device to, which is not normal.
This is my own typo - this should be .info. Sorry.
Quoted 19 lines
Quoted 4 lines
Regarding
VMSTATE_VIRTIO_FIELD, it's just a matter of not doing things differently for no particular reason. Your macro is already doing exactly the same as other VMSTATE_* macros, just with different conventions for the arguments. I don't see any advantage in changing that.In my opinion it is not the same. In the case of
VMSTATE_PCI_DEVICEwe have (a self contained) parent (in sense of inheritance) device, which is embedded as_fieldin the specialized device and is migrated by the vmstatedescription of the parent. The rest of the specialized devices state is represented by the other fields.
VMSTATE_VIRTIO_FIELDis however just there to make surevirtio_loadandvirtio_saveare called at the right time with the right arguments. The specialized device is then migrated by the save/load callbacks of the device class, or the vmsd residing in the device class.VMSTATE_VIRTIO_FIELDis supposed to be the only field, if the virtio device adheres to the virtio-migration document.VMSTATE_VIRTIO_FIELDhas no arguments because it is
a virtual field and does not depend on the offset stuff.Quoted 3 lines
To summarize currently I have no idea how to write up the vmstate field definition macro
VMSTATE_VIRTIO_DEVICEso that it meets your expectations.As above.
Quoted 8 lines
Quoted 2 lines
Having everything hidden behind a magic macro makes things harder to follow than other vmstate definitions.
Again in my opinion the virtio migration is different that the rest of the vmstate migration stuff, and it's ugly to some extent. So the idea was to make it look different and hide the ugliness behind the macro. If you insist i will create a version where the macros are expanded so it's easier to say if this improves or worsens the readability.
I agree it is not exactly the same as the other devices. But in my opinion it's not different-enough to do everything completely more, and in the future we should aim at making it less different.
Quoted 6 lines
I think this is matter of taste, and your taste matters more ;). I do agree that the variadic for the massaging functions is more complicated that the two function pointers taken by
VMSTATE_VIRTIO_DEVICE. My idea was that we end up with more readable code on the caller-side, but if you prefer function pointers and NULLs if no callback is needed needed (most cases), I can live with that.
Quoted 2 lines
Well, the third possibility would be expanding the VMStateDescription definition, :) where .post_load becomes just yet another initializer.
Hmm, I actually disagree here; I like the macros that Halil has got; to me the important thing is that in most cases they're trivially short and in the slightly weirder cases they're not much bigger. The virtio-input conversion especially is nice and simple. The only weird case is virtio-gpu, and well virtio-gpu is the one that's odd here (compared to the rest of virtio).
I'd say we take these macros (apart from anything minor) - I'd anticipate they'd evolve slowly as we move more and more chunks to VMState.
Dave
Quoted 1 line
Paolo
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
Quoted 3 lines · Dr. David Alan Gilbert
The virtio-input conversion especially is nice and simple. The only weird case is virtio-gpu, and well virtio-gpu is the one that's odd here (compared to the rest of virtio).
Though virtio-gpu would be the one that could become nicer without the macros. :)
What I don't like about the macros is that they don't allow you to extend the VMStateDescription. However, if you agree with Halil your opinion counts more.
Paolo
Quoted 2 lines
I'd say we take these macros (apart from anything minor) - I'd anticipate they'd evolve slowly as we move more and more chunks to VMState.
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
Quoted 9 lines · * Paolo Bonzini ([email protected]) wrote:
Quoted 3 lines · Dr. David Alan Gilbert
The virtio-input conversion especially is nice and simple. The only weird case is virtio-gpu, and well virtio-gpu is the one that's odd here (compared to the rest of virtio).
Though virtio-gpu would be the one that could become nicer without the macros. :)
Yes, it would.
Quoted 2 lines
What I don't like about the macros is that they don't allow you to extend the VMStateDescription.
Hmm so would this work:
add an 'extra_field' that we normally leave empty:
#define VIRTIO_DEF_DEVICE_VMSD(devname, v, extra_field, ...) \
static const VMStateDescription vmstate_virtio_ ## devname = { \
.name = "virtio-" #devname , \
.minimum_version_id = v, \
.version_id = v, \
.fields = (VMStateField[]) { \
VMSTATE_VIRTIO_FIELD, \
extra_field \
VMSTATE_END_OF_LIST() \
}, \
__VA_ARGS__ \
};so the normal case would gain messy commas:
VIRTIO_DEF_DEVICE_VMSD(console,3, /* empty */)
the case with pre/post would be:
VIRTIO_DEF_DEVICE_VMSD(vhost_vsock, VHOST_VSOCK_SAVEVM_VERSION, /* empty */, .pre_save = vhost_vsock_pre_save, .post_load = vhost_vsock_post_load)
.....
Define a macro for this field so it's passed as one entry:
#define VMSTATE_VIRTIO_GPU_FIELD \
{ \
.name = "virtio-gpu", \
.info = &(const VMStateInfo) { \
.name = "virtio-gpu", \
.get = virtio_gpu_load, \
.put = virtio_gpu_save, \
}, \
.flags = VMS_SINGLE, \
} /* device */,VIRTIO_DEF_DEVICE_VMSD(virtio-gpu, VIRTIO_GPU_VM_VERSION, VMSTATE_VIRTIO_GPU_FIELD)
Quoted 2 lines
However, if you agree with Halil your
opinion counts more.
Well I think Halil's stuff moves it in the right direction;
with everything in VIRTIO_DEF_DEVICE_VMSD it means we can move things out of virtio_load/save and into corresponding members in VIRTIO_DEF_DEVICE_VMSD's
.fields array (before or after VMSTATE_VIRTIO_FIELD) without having
to change any of the devices. Eventually virtio_load/save disappear.
Dave
Quoted 5 lines
Paolo
Quoted 2 lines
I'd say we take these macros (apart from anything minor) - I'd anticipate they'd evolve slowly as we move more and more chunks to VMState.
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
Quoted 55 lines · Dr. David Alan Gilbert
Quoted 9 lines · * Paolo Bonzini ([email protected]) wrote:
Quoted 3 lines · Dr. David Alan Gilbert
The virtio-input conversion especially is nice and simple. The only weird case is virtio-gpu, and well virtio-gpu is the one that's odd here (compared to the rest of virtio).
Though virtio-gpu would be the one that could become nicer without the macros. :)
Yes, it would.
Quoted 2 lines
What I don't like about the macros is that they don't allow you to extend the VMStateDescription.
Hmm so would this work:
add an '
extra_field' that we normally leave empty:#define VIRTIO_DEF_DEVICE_VMSD(devname, v, extra_field, ...) \ static const VMStateDescription vmstate_virtio_ ## devname = { \ .name = "virtio-" #devname , \ .minimum_version_id = v, \ .version_id = v, \ .fields = (VMStateField[]) { \ VMSTATE_VIRTIO_FIELD, \ extra_field \ VMSTATE_END_OF_LIST() \ }, \ __VA_ARGS__ \ };so the normal case would gain messy commas:
VIRTIO_DEF_DEVICE_VMSD(console,3, /* empty */)the case with pre/post would be:
VIRTIO_DEF_DEVICE_VMSD(vhost_vsock,VHOST_VSOCK_SAVEVM_VERSION, /* empty */, .pre_save =vhost_vsock_pre_save, .post_load =vhost_vsock_post_load).....
Define a macro for this field so it's passed as one entry:
#define VMSTATE_VIRTIO_GPU_FIELD \ { \ .name = "virtio-gpu", \ .info = &(const VMStateInfo) { \ .name = "virtio-gpu", \ .get = virtio_gpu_load, \ .put = virtio_gpu_save, \ }, \ .flags = VMS_SINGLE, \ } /* device */,
Quoted 1 line
VIRTIO_DEF_DEVICE_VMSD(virtio-gpu, VIRTIO_GPU_VM_VERSION, VMSTATE_VIRTIO_GPU_FIELD)
I would just expand the macro in the virtio-gpu case. For now what Halil did is fine.
Paolo
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
In reply to Dr. David Alan GilbertQuoted 15 lines · Dr. David Alan Gilbert
Quoted 9 lines · * Paolo Bonzini ([email protected]) wrote:
Quoted 3 lines · Dr. David Alan Gilbert
The virtio-input conversion especially is nice and simple. The only weird case is virtio-gpu, and well virtio-gpu is the one that's odd here (compared to the rest of virtio).
Though virtio-gpu would be the one that could become nicer without the macros. :)
Yes, it would.
Quoted 2 lines
What I don't like about the macros is that they don't allow you to extend the VMStateDescription.
[..]
Quoted 9 lines
Quoted 2 lines
However, if you agree with Halil your
opinion counts more.Well I think Halil's stuff moves it in the right direction;
with everything inVIRTIO_DEF_DEVICE_VMSDit means we can move things out of virtio_load/save and into corresponding members in VIRTIO_DEF_DEVICE_VMSD's
.fields array (before or afterVMSTATE_VIRTIO_FIELD) without having
to change any of the devices. Eventually virtio_load/save disappear.
I think this would end up messy if we want to preserve the current behavior of virtio_load/save, and more importantly compatibility. AFAIU we would have a 'synthetic' field for the configuration/transport migration first, then a field dealing with the state in VirtIODevice excluding config_vector and subsections (the state of virtio-core), then we would have a sequence of fields dealing with the device specific state of the transport agnostic virtio device (e.g. virtio-net, virtio-gpu) that is emulate load/save from VirtIODeviceClass, then we would have to take care of the virtio core subsections and possibly device specific subsections (or have yet another synthetic field for device specific subsections). This is actually the first thing I wanted to do in response to "[RFC 0/6] converting some of virtio to VMState" but after some thinking, concluded for myself that it will end up too messy and way less readable that what Dave did there, without any real gain to make the increased complexity justified.
Or am I mistaken here?
Halil
Re: [Qemu-devel] [PATCH 01/12] virtio: add VIRTIO_DEF_DEVICE_VMSD macro
In reply to Paolo BonziniQuoted 5 lines · Paolo Bonzini
Quoted 3 lines · Dr. David Alan Gilbert
Quoted 3 lines
The virtio-input conversion especially is nice and simple. The only weird case is virtio-gpu, and well virtio-gpu is the one that's odd here (compared to the rest of virtio).
Quoted 9 lines
Though virtio-gpu would be the one that could become nicer without the macros. :)
What I don't like about the macros is that they don't allow you to extend the VMStateDescription. However, if you agree with Halil your opinion counts more.
Paolo
Hi guys!
Paolo you probably mean the fields part of the VMStateDescription. In my opinion it is an important question if we should want the ability to extend there. I was basing my assumptions on the series "[RFC 0/6] converting some of virtio to VMState" by Dave. This series designates VirtioDeviceClass.vmsd as the place for the state description of a specialized device which would normally belong to where Paolo wants it. The trouble in my opinion comes from the need to maintain compatibility which basically means put the data exactly the way it was.
I do agree with Paolo that for new devices where this constrain is non-existent we could and probably should get rid of the virtio migration is very special stuff and do it normally. But for that in my opinion we first need to figure out what is the new way of virtio migration, document it, and provide an internal API for it. If we are going to do that, I think it will actually turn out beneficial, that if we name the weird (legacy) stuff differently because we can then name the new not-so-weird stuff according to the normal conventions.
IMHO couple of questions to be answered before drafting the new approach are:
* Is the proxy device (basically the transport) migrating itself or is it migrated by the transport agnostic device (e.g. virtio-net)? In my opinion these are two separate QOM/QDEV devices so they migrate separately. But this would arise some problems.
* How is inheritance and VMState is supposed to work together, including versioning?
In the meanwhile I have re-implemented the series following Paolo's suggestions (at least I hope) so I will send it out as a v2 shortly so we can make a direct comparison.
Cheers,
Halil
PATCH 2/12 virtio-blk: convert to VIRTIO_DEF_DEVICE_VMSD
In reply to Halil PasicRefactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD replaces VMSTATE_VIRTIO_DEVICE.
--- hw/block/virtio-blk.c | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-)
hw/block/virtio-blk.c +1 -16
@@ -803,13 +803,6 @@ static void virtio_blk_set_status(VirtIODevice *vdev, uint8_t status) } } -static void virtio_blk_save (QEMUFile *f,void *opaque,size_t size) -{ -VirtIODevice *vdev =VIRTIO_DEVICE (opaque); - -virtio_save (vdev, f); -} -static void virtio_blk_save_device (VirtIODevice *vdev,QEMUFile *f) {VirtIOBlock *s =VIRTIO_BLK (vdev);
@@ -828,14 +821,6 @@ static void virtio_blk_save_device(VirtIODevice *vdev, QEMUFile *f)qemu_put_sbyte (f,0 ); } -static int virtio_blk_load (QEMUFile *f,void *opaque,size_t size) -{ -VirtIOBlock *s = opaque; -VirtIODevice *vdev =VIRTIO_DEVICE (s); - -return virtio_load (vdev, f,2 ); -} -static int virtio_blk_load_device (VirtIODevice *vdev,QEMUFile *f,int version_id) {
@@ -956,7 +941,7 @@ static void virtio_blk_instance_init(Object *obj) DEVICE(obj), NULL); } -VMSTATE_VIRTIO_DEVICE (blk,2 , virtio_blk_load, virtio_blk_save); +VIRTIO_DEF_DEVICE_VMSD(blk, 2)static Property virtio_blk_properties[] = {DEFINE_BLOCK_PROPERTIES (VirtIOBlock, conf.conf),
Signature · 1 line
2.8.4
PATCH 3/12 virtio-net: convert to VIRTIO_DEF_DEVICE_VMSD
In reply to Halil PasicRefactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD replaces VMSTATE_VIRTIO_DEVICE.
--- hw/net/virtio-net.c | 32 +++++++++++--------------------- 1 file changed, 11 insertions(+), 21 deletions(-)
hw/net/virtio-net.c +11 -21
@@ -1498,17 +1498,6 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue)virtio_net_set_queues (n); } -static void virtio_net_save (QEMUFile *f,void *opaque,size_t size) -{ -VirtIONet *n = opaque; -VirtIODevice *vdev =VIRTIO_DEVICE (n); - -/* At this point, backend must be stopped, otherwise -* it might keep writing to memory. */ -assert (!n->vhost_started); -virtio_save (vdev, f); -} -static void virtio_net_save_device (VirtIODevice *vdev,QEMUFile *f) {VirtIONet *n =VIRTIO_NET (vdev);
@@ -1544,14 +1533,6 @@ static void virtio_net_save_device(VirtIODevice *vdev, QEMUFile *f) } } -static int virtio_net_load (QEMUFile *f,void *opaque,size_t size) -{ -VirtIONet *n = opaque; -VirtIODevice *vdev =VIRTIO_DEVICE (n); - -return virtio_load (vdev, f,VIRTIO_NET_VM_VERSION ); -} -static int virtio_net_load_device (VirtIODevice *vdev,QEMUFile *f,int version_id) {
@@ -1854,8 +1835,17 @@ static void virtio_net_instance_init(Object *obj) DEVICE(n), NULL); } -VMSTATE_VIRTIO_DEVICE (net,VIRTIO_NET_VM_VERSION , virtio_net_load, - virtio_net_save); +static void virtio_net_pre_save (void *opaque) +{ +VirtIONet *n = opaque; + +/* At this point, backend must be stopped, otherwise +* it might keep writing to memory. */ +assert (!n->vhost_started); +} + +VIRTIO_DEF_DEVICE_VMSD (net,VIRTIO_NET_VM_VERSION , + .pre_save = virtio_net_pre_save)static Property virtio_net_properties[] = {DEFINE_PROP_BIT ("csum" , VirtIONet, host_features,VIRTIO_NET_F_CSUM , true),
Signature · 1 line
2.8.4
PATCH 4/12 virtio-9p: convert to VIRTIO_DEF_DEVICE_VMSD
In reply to Halil PasicRefactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD replaces VMSTATE_VIRTIO_DEVICE.
--- hw/9pfs/virtio-9p-device.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
hw/9pfs/virtio-9p-device.c +1 -6
@@ -97,11 +97,6 @@ static void virtio_9p_get_config(VirtIODevice *vdev, uint8_t *config)g_free (cfg); } -static int virtio_9p_load (QEMUFile *f,void *opaque,size_t size) -{ -return virtio_load (VIRTIO_DEVICE (opaque), f,1 ); -} -static void virtio_9p_device_realize (DeviceState *dev,Error **errp) {VirtIODevice *vdev =VIRTIO_DEVICE (dev);
@@ -168,7 +163,7 @@ void virtio_init_iov_from_pdu(V9fsPDU *pdu, struct iovec **piov,/* virtio-9p device */ -VMSTATE_VIRTIO_DEVICE (9p ,1 , virtio_9p_load, virtio_vmstate_save); +VIRTIO_DEF_DEVICE_VMSD(9p, 1)static Property virtio_9p_properties[] = {DEFINE_PROP_STRING ("mount_tag" , V9fsVirtioState, state.fsconf.tag),
Signature · 1 line
2.8.4
PATCH 5/12 virtio-serial: convert to VIRTIO_DEF_DEVICE_VMSD
In reply to Halil PasicRefactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD replaces VMSTATE_VIRTIO_DEVICE.
--- hw/char/virtio-serial-bus.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
hw/char/virtio-serial-bus.c +1 -7
@@ -728,12 +728,6 @@ static int fetch_active_ports_list(QEMUFile *f,return 0 ; } -static int virtio_serial_load (QEMUFile *f,void *opaque,size_t size) -{ -/* The virtio device */ -return virtio_load (VIRTIO_DEVICE (opaque), f,3 ); -} -static int virtio_serial_load_device (VirtIODevice *vdev,QEMUFile *f,int version_id) {
@@ -1075,7 +1069,7 @@ static void virtio_serial_device_unrealize(DeviceState *dev, Error **errp) }/* Note: 'console' is used for backwards compatibility */ -VMSTATE_VIRTIO_DEVICE (console,3 , virtio_serial_load, virtio_vmstate_save); +VIRTIO_DEF_DEVICE_VMSD(console, 3)static Property virtio_serial_properties[] = { DEFINE_PROP_UINT32("max_ports", VirtIOSerial, serial.max_virtserial_ports,
Signature · 1 line
2.8.4
PATCH 6/12 virtio-gpu: do not use VMSTATE_VIRTIO_DEVICE
In reply to Halil PasicRefactor so that VMSTATE_VIRTIO_DEVICE macro which we want to remove in the future is not used any more. The device virtio-gpu is special because it actually does not adhere to the virtio migration schema, because device state is last.
--- hw/display/virtio-gpu.c | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-)
hw/display/virtio-gpu.c +27 -12
@@ -990,12 +990,9 @@ static const VMStateDescription vmstate_virtio_gpu_scanouts = {static void virtio_gpu_save (QEMUFile *f,void *opaque,size_t size) {VirtIOGPU *g = opaque; -VirtIODevice *vdev =VIRTIO_DEVICE (g);struct virtio_gpu_simple_resource *res;int i; -virtio_save (vdev, f); -/* in 2d mode we should never find unprocessed commands here */ assert (QTAILQ_EMPTY (&g->cmdq));
@@ -1020,16 +1017,10 @@ static void virtio_gpu_save(QEMUFile *f, void *opaque, size_t size)static int virtio_gpu_load (QEMUFile *f,void *opaque,size_t size) {VirtIOGPU *g = opaque; -VirtIODevice *vdev =VIRTIO_DEVICE (g);struct virtio_gpu_simple_resource *res;struct virtio_gpu_scanout *scanout;uint32_t resource_id, pformat; -int i, ret; - - ret =virtio_load (vdev, f,VIRTIO_GPU_VM_VERSION ); -if (ret) { -return ret; - } +int i; resource_id =qemu_get_be32 (f);while (resource_id !=0 ) {
@@ -1219,8 +1210,32 @@ static void virtio_gpu_reset(VirtIODevice *vdev)#endif } -VMSTATE_VIRTIO_DEVICE (gpu,VIRTIO_GPU_VM_VERSION , virtio_gpu_load, - virtio_gpu_save); +/* +* For historical reasons virtio_gpu does not adhere to virtio migration +* scheme as described in doc/virtio-migration.txt, in a sense that no +* save/load callback are provided to the core. Instead the device data +* is saved/loaded after the core data. +* +* Because of this we need a special vmsd. +*/ +static const VMStateDescription vmstate_virtio_gpu = { + .name ="virtio-gpu" , + .minimum_version_id =VIRTIO_GPU_VM_VERSION , + .version_id =VIRTIO_GPU_VM_VERSION , + .fields = (VMStateField []) { +VMSTATE_VIRTIO_FIELD /* core */ , + { + .name ="virtio-gpu" , + .info = &(const VMStateInfo ) { + .name ="virtio-gpu" , + .get = virtio_gpu_load, + .put = virtio_gpu_save, + }, + .flags =VMS_SINGLE , + }/* device */ , +VMSTATE_END_OF_LIST () + }, +};static Property virtio_gpu_properties[] = {DEFINE_PROP_UINT32 ("max_outputs" , VirtIOGPU, conf.max_outputs,1 ),
Signature · 1 line
2.8.4
PATCH 7/12 virtio-input: convert to VIRTIO_DEF_DEVICE_VMSD
In reply to Halil PasicRefactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD replaces VMSTATE_VIRTIO_DEVICE.
--- hw/input/virtio-input.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
hw/input/virtio-input.c +3 -10
@@ -217,19 +217,12 @@ static void virtio_input_reset(VirtIODevice *vdev) } } -static int virtio_input_load (QEMUFile *f,void *opaque,size_t size) +static int virtio_input_post_load (void *opaque,int version_id) {VirtIOInput *vinput = opaque;VirtIOInputClass *vic =VIRTIO_INPUT_GET_CLASS (vinput);VirtIODevice *vdev =VIRTIO_DEVICE (vinput); -int ret; - ret =virtio_load (vdev, f,VIRTIO_INPUT_VM_VERSION ); -if (ret) { -return ret; - } - -/* post_load() */ vinput->active = vdev->status &VIRTIO_CONFIG_S_DRIVER_OK ;if (vic->change_active) { vic->change_active (vinput);
@@ -296,8 +289,8 @@ static void virtio_input_device_unrealize(DeviceState *dev, Error **errp)virtio_cleanup (vdev); } -VMSTATE_VIRTIO_DEVICE (input,VIRTIO_INPUT_VM_VERSION , virtio_input_load, - virtio_vmstate_save); +VIRTIO_DEF_DEVICE_VMSD (input,VIRTIO_INPUT_VM_VERSION , + .post_load = virtio_input_post_load)static Property virtio_input_properties[] = {DEFINE_PROP_STRING ("serial" , VirtIOInput, serial),
Signature · 1 line
2.8.4
PATCH 8/12 virtio-scsi: convert to VIRTIO_DEF_DEVICE_VMSD
In reply to Halil PasicRefactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD replaces VMSTATE_VIRTIO_DEVICE.
--- hw/scsi/virtio-scsi.c | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-)
hw/scsi/virtio-scsi.c +1 -17
@@ -663,22 +663,6 @@ static void virtio_scsi_reset(VirtIODevice *vdev) s->events_dropped = false; } -/* The device does not have anything to save beyond the virtio data. -* Request data is saved with callbacks from SCSI devices. -*/ -static void virtio_scsi_save (QEMUFile *f,void *opaque,size_t size) -{ -VirtIODevice *vdev =VIRTIO_DEVICE (opaque); -virtio_save (vdev, f); -} - -static int virtio_scsi_load (QEMUFile *f,void *opaque,size_t size) -{ -VirtIODevice *vdev =VIRTIO_DEVICE (opaque); - -return virtio_load (vdev, f,1 ); -} -void virtio_scsi_push_event (VirtIOSCSI *s,SCSIDevice *dev,uint32_t event,uint32_t reason) {
@@ -921,7 +905,7 @@ static Property virtio_scsi_properties[] = {DEFINE_PROP_END_OF_LIST (), }; -VMSTATE_VIRTIO_DEVICE (scsi,1 , virtio_scsi_load, virtio_scsi_save); +VIRTIO_DEF_DEVICE_VMSD(scsi, 1)static void virtio_scsi_common_class_init (ObjectClass *klass,void *data) {
Signature · 1 line
2.8.4
PATCH 9/12 virtio-balloon: convert to VIRTIO_DEF_DEVICE_VMSD
In reply to Halil PasicRefactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD replaces VMSTATE_VIRTIO_DEVICE.
--- hw/virtio/virtio-balloon.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
hw/virtio/virtio-balloon.c +1 -6
@@ -404,11 +404,6 @@ static void virtio_balloon_save_device(VirtIODevice *vdev, QEMUFile *f)qemu_put_be32 (f, s->actual); } -static int virtio_balloon_load (QEMUFile *f,void *opaque,size_t size) -{ -return virtio_load (VIRTIO_DEVICE (opaque), f,1 ); -} -static int virtio_balloon_load_device (VirtIODevice *vdev,QEMUFile *f,int version_id) {
@@ -494,7 +489,7 @@ static void virtio_balloon_instance_init(Object *obj) NULL, s, NULL); } -VMSTATE_VIRTIO_DEVICE (balloon,1 , virtio_balloon_load, virtio_vmstate_save); +VIRTIO_DEF_DEVICE_VMSD(balloon, 1)static Property virtio_balloon_properties[] = { DEFINE_PROP_BIT("deflate-on-oom", VirtIOBalloon, host_features,
Signature · 1 line
2.8.4
PATCH 10/12 virtio-rng: convert to VIRTIO_DEF_DEVICE_VMSD
In reply to Halil PasicRefactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD replaces VMSTATE_VIRTIO_DEVICE.
--- hw/virtio/virtio-rng.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
hw/virtio/virtio-rng.c +2 -8
@@ -120,15 +120,9 @@ static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp)return f; } -static int virtio_rng_load (QEMUFile *f,void *opaque,size_t size) +static int virtio_rng_post_load (void *opaque,int version_id) {VirtIORNG *vrng = opaque; -int ret; - - ret =virtio_load (VIRTIO_DEVICE (vrng), f,1 ); -if (ret !=0 ) { -return ret; - }/* We may have an element ready but couldn't process it due to a quota * limit. Make sure to try again after live migration when the quota may
@@ -216,7 +210,7 @@ static void virtio_rng_device_unrealize(DeviceState *dev, Error **errp)virtio_cleanup (vdev); } -VMSTATE_VIRTIO_DEVICE (rng,1 , virtio_rng_load, virtio_vmstate_save); +VIRTIO_DEF_DEVICE_VMSD(rng, 1, .post_load = virtio_rng_post_load)static Property virtio_rng_properties[] = {/* Set a default rate limit of 2^47 bytes per minute or roughly 2TB/s. If
Signature · 1 line
2.8.4
PATCH 11/12 vhost-vsock: convert to VIRTIO_DEF_DEVICE_VMSD
In reply to Halil PasicRefactor so the previously introduced VIRTIO_DEF_DEVICE_VMSD replaces VMSTATE_VIRTIO_DEVICE.
--- hw/virtio/vhost-vsock.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-)
hw/virtio/vhost-vsock.c +12 -21
@@ -236,17 +236,6 @@ out:g_free (elem); } -static void vhost_vsock_save (QEMUFile *f,void *opaque,size_t size) -{ -VHostVSock *vsock = opaque; -VirtIODevice *vdev =VIRTIO_DEVICE (vsock); - -/* At this point, backend must be stopped, otherwise -* it might keep writing to memory. */ -assert (!vsock->vhost_dev.started); -virtio_save (vdev, f); -} -static void vhost_vsock_post_load_timer_cleanup (VHostVSock *vsock) {if (!vsock->post_load_timer) {
@@ -266,16 +255,19 @@ static void vhost_vsock_post_load_timer_cb(void *opaque)vhost_vsock_send_transport_reset (vsock); } -static int vhost_vsock_load (QEMUFile *f,void *opaque,size_t size) +static void vhost_vsock_pre_save (void *opaque) +{ +VHostVSock *vsock = opaque; + +/* At this point, backend must be stopped, otherwise +* it might keep writing to memory. */ +assert (!vsock->vhost_dev.started); +} + +static int vhost_vsock_post_load (void *opaque,int version_id) {VHostVSock *vsock = opaque;VirtIODevice *vdev =VIRTIO_DEVICE (vsock); -int ret; - - ret =virtio_load (vdev, f,VHOST_VSOCK_SAVEVM_VERSION ); -if (ret) { -return ret; - }if (virtio_queue_get_addr (vdev,2 )) {/* Defer transport reset event to a vm clock timer so that virtqueue
@@ -288,12 +280,11 @@ static int vhost_vsock_load(QEMUFile *f, void *opaque, size_t size) vsock);timer_mod (vsock->post_load_timer,1 ); } -return 0 ; } -VMSTATE_VIRTIO_DEVICE (vhost_vsock,VHOST_VSOCK_SAVEVM_VERSION , - vhost_vsock_load, vhost_vsock_save); +VIRTIO_DEF_DEVICE_VMSD (vhost_vsock,VHOST_VSOCK_SAVEVM_VERSION , + .pre_save = vhost_vsock_pre_save, .post_load = vhost_vsock_post_load)static void vhost_vsock_device_realize (DeviceState *dev,Error **errp) {
Signature · 1 line
2.8.4
PATCH 12/12 virtio: remove unused VMSTATE_VIRTIO_DEVICE
In reply to Halil PasicPreviously we mande sure VMSTATE_VIRTIO_DEVICE is not used any more. Let us remove it along with the associated wrapper function.
--- hw/virtio/virtio.c | 6 ------ include/hw/virtio/virtio.h | 20 -------------------- 2 files changed, 26 deletions(-)
hw/virtio/virtio.c +0 -6
@@ -1617,12 +1617,6 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) }/* A wrapper for use as a VMState .put function */ -void virtio_vmstate_save (QEMUFile *f,void *opaque,size_t size) -{ -virtio_save (VIRTIO_DEVICE (opaque), f); -} - -/* A wrapper for use as a VMState .put function */ void virtio_save_as_vmsi_put (QEMUFile *f,void *opaque,size_t size) {virtio_save (VIRTIO_DEVICE (opaque), f);
include/hw/virtio/virtio.h +0 -20
@@ -175,7 +175,6 @@ bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq);void virtio_notify (VirtIODevice *vdev,VirtQueue *vq);void virtio_save (VirtIODevice *vdev,QEMUFile *f); -void virtio_vmstate_save (QEMUFile *f,void *opaque,size_t size);void virtio_save_as_vmsi_put (QEMUFile *f,void *opaque,size_t size);int virtio_load_as_vmsi_get (QEMUFile *f,void *opaque,size_t size);
@@ -202,25 +201,6 @@ int virtio_load_as_vmsi_get(QEMUFile *f, void *opaque, size_t size); __VA_ARGS__ \ }; -#define VMSTATE_VIRTIO_DEVICE (devname, v, getf, putf) \ - static const VMStateDescription vmstate_virtio_ ## devname = { \ - .name = "virtio-" #devname , \ - .minimum_version_id = v, \ - .version_id = v, \ - .fields = (VMStateField[]) { \ - { \ - .name = "virtio", \ - .info = &(const VMStateInfo) {\ - .name = "virtio", \ - .get = getf, \ - .put = putf, \ - }, \ - .flags = VMS_SINGLE, \ - }, \ - VMSTATE_END_OF_LIST() \ - } \ - } -int virtio_load (VirtIODevice *vdev,QEMUFile *f,int version_id);void virtio_notify_config (VirtIODevice *vdev);
Signature · 1 line
2.8.4
Quoted 8 lines · Halil Pasic
As a part of the long term effort to convert migration to vmstate the migration of virtio devices was recently partially switched to vmstate starting with the outer layer (commit
5943124cc"virtio: Migration helper function and macro" and the subsequent "virtio-*: Wrap in vmstate" commits). This was done by introducing a vmstate based wrapper driving the well know virtio migration process. As this transition is still in progress, some synergies were left unexploited, and some things can be expressed in a more vmstatish way.
Another useful thing to do is to move code out of virtio_load and into a post_load callback of vmstate_virtio.
Paolo
Quoted 30 lines
Let us simplify a couple of things and get rid of some code duplication.
Did only a couple of smoke tests. Comprehensive testing is still to be done.
Halil Pasic (11):
virtio: addVIRTIO_DEF_DEVICE_VMSDmacro
virtio-blk: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-net: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-9p: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-serial: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-gpu: do not useVMSTATE_VIRTIO_DEVICE
virtio-input: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-scsi: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-balloon: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-rng: convert toVIRTIO_DEF_DEVICE_VMSD
virtio: remove unusedVMSTATE_VIRTIO_DEVICEhw/9pfs/virtio-9p-device.c | 7 +------ hw/block/virtio-blk.c | 17 +---------------- hw/char/virtio-serial-bus.c | 8 +------- hw/display/virtio-gpu.c | 39 +++++++++++++++++++++++++++------------ hw/input/virtio-input.c | 13 +++---------- hw/net/virtio-net.c | 32 +++++++++++--------------------- hw/scsi/virtio-scsi.c | 18 +----------------- hw/virtio/virtio-balloon.c | 7 +------ hw/virtio/virtio-rng.c | 10 ++-------- hw/virtio/virtio.c | 11 ++++++++++- include/hw/virtio/virtio.h | 31 ++++++++++++++++++------------- 11 files changed, 76 insertions(+), 117 deletions(-)
Dr. David Alan Gilbert
Reviewed-byQuoted 12 lines · * Paolo Bonzini ([email protected]) wrote:
Quoted 8 lines · Halil Pasic
As a part of the long term effort to convert migration to vmstate the migration of virtio devices was recently partially switched to vmstate starting with the outer layer (commit
5943124cc"virtio: Migration helper function and macro" and the subsequent "virtio-*: Wrap in vmstate" commits). This was done by introducing a vmstate based wrapper driving the well know virtio migration process. As this transition is still in progress, some synergies were left unexploited, and some things can be expressed in a more vmstatish way.
Quoted 2 lines
Another useful thing to do is to move code out of
virtio_loadand into apost_loadcallback ofvmstate_virtio.
Yes, and I'm hoping that we'll be able to get more of virtio_load/virtio_save into normal macros within the vmstate_virtio eventually.
So other than the minor issues Paolo suggested on the macro:
Dave
Quoted 34 lines
Paolo
Quoted 30 lines
Let us simplify a couple of things and get rid of some code duplication.
Did only a couple of smoke tests. Comprehensive testing is still to be done.
Halil Pasic (11):
virtio: addVIRTIO_DEF_DEVICE_VMSDmacro
virtio-blk: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-net: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-9p: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-serial: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-gpu: do not useVMSTATE_VIRTIO_DEVICE
virtio-input: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-scsi: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-balloon: convert toVIRTIO_DEF_DEVICE_VMSD
virtio-rng: convert toVIRTIO_DEF_DEVICE_VMSD
virtio: remove unusedVMSTATE_VIRTIO_DEVICEhw/9pfs/virtio-9p-device.c | 7 +------ hw/block/virtio-blk.c | 17 +---------------- hw/char/virtio-serial-bus.c | 8 +------- hw/display/virtio-gpu.c | 39 +++++++++++++++++++++++++++------------ hw/input/virtio-input.c | 13 +++---------- hw/net/virtio-net.c | 32 +++++++++++--------------------- hw/scsi/virtio-scsi.c | 18 +----------------- hw/virtio/virtio-balloon.c | 7 +------ hw/virtio/virtio-rng.c | 10 ++-------- hw/virtio/virtio.c | 11 ++++++++++- include/hw/virtio/virtio.h | 31 ++++++++++++++++++------------- 11 files changed, 76 insertions(+), 117 deletions(-)
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Quoted 10 lines · Paolo Bonzini
Quoted 8 lines · Halil Pasic
Quoted 8 lines
As a part of the long term effort to convert migration to vmstate the migration of virtio devices was recently partially switched to vmstate starting with the outer layer (commit
5943124cc"virtio: Migration helper function and macro" and the subsequent "virtio-*: Wrap in vmstate" commits). This was done by introducing a vmstate based wrapper driving the well know virtio migration process. As this transition is still in progress, some synergies were left unexploited, and some things can be expressed in a more vmstatish way.
Quoted 5 lines
Another useful thing to do is to move code out of
virtio_loadand into apost_loadcallback ofvmstate_virtio.Paolo
Hi Paolo,
thanks for the review. I agree, but I see this out of scope for this patch series. If you like I could do something along the lines as a separate patch.
Cheers,
Halil
Quoted 5 lines · Halil Pasic
Quoted 2 lines
Another useful thing to do is to move code out of
virtio_loadand into apost_loadcallback ofvmstate_virtio.thanks for the review. I agree, but I see this out of scope for this patch series. If you like I could do something along the lines as a separate patch.
Yes, I agree.
Paolo