| Message ID | 20160930142003.53232-2-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 30/09/2016 16:19, Halil Pasic wrote: > 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. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > hw/virtio/virtio.c | 15 +++++++++++++++ > include/hw/virtio/virtio.h | 25 +++++++++++++++++++++++++ > 2 files changed, 40 insertions(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 18ce333..ca0a780 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -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); > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 888c8de..01de49b 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -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... > + > +#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), \ } > +#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 > #define VMSTATE_VIRTIO_DEVICE(devname, v, getf, putf) \ > static const VMStateDescription vmstate_virtio_ ## devname = { \ >
On 09/30/2016 04:50 PM, Paolo Bonzini wrote: > > > On 30/09/2016 16:19, Halil Pasic wrote: >> 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. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> hw/virtio/virtio.c | 15 +++++++++++++++ >> include/hw/virtio/virtio.h | 25 +++++++++++++++++++++++++ >> 2 files changed, 40 insertions(+) >> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 18ce333..ca0a780 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -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); >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >> index 888c8de..01de49b 100644 >> --- a/include/hw/virtio/virtio.h >> +++ b/include/hw/virtio/virtio.h >> @@ -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... > >> + >> +#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. > 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. >> +#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. 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 > > Paolo > >> #define VMSTATE_VIRTIO_DEVICE(devname, v, getf, putf) \ >> static const VMStateDescription vmstate_virtio_ ## devname = { \ >> >
On 03/10/2016 12:36, Halil Pasic wrote: >> > #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). 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
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. On 10/03/2016 01:29 PM, Paolo Bonzini wrote: > > > On 03/10/2016 12:36, Halil Pasic wrote: >>>> #define VMSTATE_PCI_DEVICE(_field, _state) { This was probably supposed to be VMSTATE_VIRTIO_DEVICE. \ >>>> .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. >>>> .flags = VMS_SINGLE, \ >>>> .offset = vmstate_offset_value(_state, _field, VirtIODevice), \ This is useless if we keep virtio_save and virtio_load. >>>> } >>>> >>>> >> 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). > > 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. > > 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. > 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. > 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. 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
On 03/10/2016 15:34, Halil Pasic wrote: > 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. > > On 10/03/2016 01:29 PM, Paolo Bonzini wrote: >> >> >> On 03/10/2016 12:36, Halil Pasic wrote: >>>>> #define VMSTATE_PCI_DEVICE(_field, _state) { > > This was probably supposed to be VMSTATE_VIRTIO_DEVICE. Yes. >>>>> .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. >> 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. As above. >> 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. > 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
On 10/03/2016 05:24 PM, Paolo Bonzini wrote: > > > On 03/10/2016 15:34, Halil Pasic wrote: >> 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. >> >> On 10/03/2016 01:29 PM, Paolo Bonzini wrote: >>> >>> >>> On 03/10/2016 12:36, Halil Pasic wrote: >>>>>> #define VMSTATE_PCI_DEVICE(_field, _state) { >> >> This was probably supposed to be VMSTATE_VIRTIO_DEVICE. > > Yes. > >>>>>> .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. > >>> 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. > > As above. > >>> 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. > >> 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 > Hi Paolo, clear now. I will come back with a v2 with the suggestions implemented. Thank you very much for your time. Cheers, Halil
* Paolo Bonzini (pbonzini@redhat.com) wrote: > > > On 03/10/2016 15:34, Halil Pasic wrote: > > 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. > > > > On 10/03/2016 01:29 PM, Paolo Bonzini wrote: > >> > >> > >> On 03/10/2016 12:36, Halil Pasic wrote: > >>>>> #define VMSTATE_PCI_DEVICE(_field, _state) { > > > > This was probably supposed to be VMSTATE_VIRTIO_DEVICE. > > Yes. > > >>>>> .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. > > >> 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. > > As above. > > >> 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. > > > 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. 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 > Paolo -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 05/10/2016 16:29, Dr. David Alan Gilbert wrote: > 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 > 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.
* Paolo Bonzini (pbonzini@redhat.com) wrote: > > > On 05/10/2016 16:29, Dr. David Alan Gilbert wrote: > > 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. > 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) > 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 > > Paolo > > > 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 / dgilbert@redhat.com / Manchester, UK
On 05/10/2016 21:00, Dr. David Alan Gilbert wrote: > * Paolo Bonzini (pbonzini@redhat.com) wrote: >> >> >> On 05/10/2016 16:29, Dr. David Alan Gilbert wrote: >>> 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. > >> 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) I would just expand the macro in the virtio-gpu case. For now what Halil did is fine. Paolo
On 10/05/2016 05:52 PM, Paolo Bonzini wrote: > > On 05/10/2016 16:29, Dr. David Alan Gilbert wrote: >> > 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 > 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
On 10/05/2016 09:00 PM, Dr. David Alan Gilbert wrote: > * Paolo Bonzini (pbonzini@redhat.com) wrote: >> >> >> On 05/10/2016 16:29, Dr. David Alan Gilbert wrote: >>> 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. > >> 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. > > 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. > 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
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 18ce333..ca0a780 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -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); diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 888c8de..01de49b 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -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 = { \
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. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- hw/virtio/virtio.c | 15 +++++++++++++++ include/hw/virtio/virtio.h | 25 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+)