Re: [Qemu-devel] [QEMU PATCH v5 3/6] migration: extend VMStateInfo
* Halil Pasic (pa...@linux.vnet.ibm.com) wrote: > > > On 10/12/2016 04:59 PM, Dr. David Alan Gilbert wrote: > >> Paolo I agree on a theoretical level. It's just I do not see why this > >> > particular change makes the API simpler. In my opinion this complicates > >> > things because now all VMStateInfo's can do funky stuff. Having > >> > additional > >> > state you can poke is rarely a simplification. Same goes for lots > >> > of arguments especially if some of them are barely ever used. These > >> > additional parameters contribute nothing for the large majority > >> > of the cases (except maybe some head scratching when reading > >> > the code). > > I think it might depend how many VMStateInfo's we have. > > My ideal rule would be there are no .get/.put implementations outside > > of migration/ and we can trust that they would never do anything silly > > (right?); > > Your statement about ideally no .get/.put implementations outside > of migration/ is consistent with my initial understanding of VMStateInfo: > It's there to take care of the marshaling between the on wire representation > and the in memory representation of a single and preferably primitive > vmstate field (not consisting of further fields). Complex stuff like > arrays, structs, indirection via pointers and possibly allocation is > preferably handled elsewhere. So VMStateInfo is the basic building stones, > and the only place which should write to/read from the stream (in > ideal vmstate). > > So in a perfect vmstate world complete type of VMStateInfo is not part of the > API (you do not care about how it's done outside vmstate/), but only the > (possibly pointers to) VMStateInfo's supported by the vmstate API. > > Of course this is not realistic, at least at the moment. > > On the other hand if VMStateInfo is meant for complete customization, > as Jianjun has put it, then it obviously has to be a full fledged member > of the API, and I think then your ideal rule makes no sense to me. > > I also do think we will always need something for handling special > cases because we need to maintain compatibility -- see virtio migration > for example. > > > so the extra parameters aren't going to be misused too badly. > > > > What would you consider bad misuse? I do not see this as a big concern > at the moment. I don't know; but the only justification for needing the VMS_LINKED flag was that only those functions that were marked with VMS_LINKED would be passed the new field. Dave > Cheers, > Halil > > > However, we're probably quite a way from pulling all of the weirder > > .get/.put implementations back in. > > > > Dave > > > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [QEMU PATCH v5 3/6] migration: extend VMStateInfo
On 10/12/2016 04:59 PM, Dr. David Alan Gilbert wrote: >> Paolo I agree on a theoretical level. It's just I do not see why this >> > particular change makes the API simpler. In my opinion this complicates >> > things because now all VMStateInfo's can do funky stuff. Having additional >> > state you can poke is rarely a simplification. Same goes for lots >> > of arguments especially if some of them are barely ever used. These >> > additional parameters contribute nothing for the large majority >> > of the cases (except maybe some head scratching when reading >> > the code). > I think it might depend how many VMStateInfo's we have. > My ideal rule would be there are no .get/.put implementations outside > of migration/ and we can trust that they would never do anything silly > (right?); Your statement about ideally no .get/.put implementations outside of migration/ is consistent with my initial understanding of VMStateInfo: It's there to take care of the marshaling between the on wire representation and the in memory representation of a single and preferably primitive vmstate field (not consisting of further fields). Complex stuff like arrays, structs, indirection via pointers and possibly allocation is preferably handled elsewhere. So VMStateInfo is the basic building stones, and the only place which should write to/read from the stream (in ideal vmstate). So in a perfect vmstate world complete type of VMStateInfo is not part of the API (you do not care about how it's done outside vmstate/), but only the (possibly pointers to) VMStateInfo's supported by the vmstate API. Of course this is not realistic, at least at the moment. On the other hand if VMStateInfo is meant for complete customization, as Jianjun has put it, then it obviously has to be a full fledged member of the API, and I think then your ideal rule makes no sense to me. I also do think we will always need something for handling special cases because we need to maintain compatibility -- see virtio migration for example. > so the extra parameters aren't going to be misused too badly. > What would you consider bad misuse? I do not see this as a big concern at the moment. Cheers, Halil > However, we're probably quite a way from pulling all of the weirder > .get/.put implementations back in. > > Dave > signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [QEMU PATCH v5 3/6] migration: extend VMStateInfo
* Halil Pasic (pa...@linux.vnet.ibm.com) wrote: > > > On 10/12/2016 02:07 PM, Paolo Bonzini wrote: > > > > On 12/10/2016 13:59, Halil Pasic wrote: > >> > IMHO this would: > >> > * allow us to keep the good old MVStateInfo objects unmodified and > >> > the semantic of VMStateInfo unchanged > >> > * make clear that VMStateLinked does not care about the calculated size > >> > and provide a new anchor for documentation > >> > * if overloading the semantic of VMStateField.start is not desired we > >> > could put it into VMStateLinked, if needed we could also put more > >> > stuff in there > >> > * have clearer separation between special handling for (linked/certain) > >> > data structures and the usual scenario with the DAG. > > No, I disagree. We shouldn't be worried about making intrusive changes > > to all invocations or declarations, if that leads to a simpler API. > > Paolo I agree on a theoretical level. It's just I do not see why this > particular change makes the API simpler. In my opinion this complicates > things because now all VMStateInfo's can do funky stuff. Having additional > state you can poke is rarely a simplification. Same goes for lots > of arguments especially if some of them are barely ever used. These > additional parameters contribute nothing for the large majority > of the cases (except maybe some head scratching when reading > the code). I think it might depend how many VMStateInfo's we have. My ideal rule would be there are no .get/.put implementations outside of migration/ and we can trust that they would never do anything silly (right?); so the extra parameters aren't going to be misused too badly. However, we're probably quite a way from pulling all of the weirder .get/.put implementations back in. Dave > No strong opinion here, it's just that I don't understand. I think one > trait of a simple API is that it is easy to document. Unfortunately > the documentation is quite sparse and in the patch the signature > change goes undocumented. > > Well as I said, just an idea, motivated by how I understood he role of > VMStateInfo form reading the code. > > Cheers, > Halil > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [Qemu-devel] [QEMU PATCH v5 3/6] migration: extend VMStateInfo
On 10/12/2016 02:07 PM, Paolo Bonzini wrote: > > On 12/10/2016 13:59, Halil Pasic wrote: >> > IMHO this would: >> > * allow us to keep the good old MVStateInfo objects unmodified and >> > the semantic of VMStateInfo unchanged >> > * make clear that VMStateLinked does not care about the calculated size >> > and provide a new anchor for documentation >> > * if overloading the semantic of VMStateField.start is not desired we >> > could put it into VMStateLinked, if needed we could also put more >> > stuff in there >> > * have clearer separation between special handling for (linked/certain) >> > data structures and the usual scenario with the DAG. > No, I disagree. We shouldn't be worried about making intrusive changes > to all invocations or declarations, if that leads to a simpler API. Paolo I agree on a theoretical level. It's just I do not see why this particular change makes the API simpler. In my opinion this complicates things because now all VMStateInfo's can do funky stuff. Having additional state you can poke is rarely a simplification. Same goes for lots of arguments especially if some of them are barely ever used. These additional parameters contribute nothing for the large majority of the cases (except maybe some head scratching when reading the code). No strong opinion here, it's just that I don't understand. I think one trait of a simple API is that it is easy to document. Unfortunately the documentation is quite sparse and in the patch the signature change goes undocumented. Well as I said, just an idea, motivated by how I understood he role of VMStateInfo form reading the code. Cheers, Halil
Re: [Qemu-devel] [QEMU PATCH v5 3/6] migration: extend VMStateInfo
On 12/10/2016 13:59, Halil Pasic wrote: > IMHO this would: > * allow us to keep the good old MVStateInfo objects unmodified and > the semantic of VMStateInfo unchanged > * make clear that VMStateLinked does not care about the calculated size > and provide a new anchor for documentation > * if overloading the semantic of VMStateField.start is not desired we > could put it into VMStateLinked, if needed we could also put more > stuff in there > * have clearer separation between special handling for (linked/certain) > data structures and the usual scenario with the DAG. No, I disagree. We shouldn't be worried about making intrusive changes to all invocations or declarations, if that leads to a simpler API. I agree that overloading .start can be a bit ugly but you can choose to overload .num_offset instead, which is already better. > I would also suggest unit tests in test/test-vmstate.c. Maybe with > that the vmstate migration of QTAILQ could be factored out as a separate > patch series. Yes, definitely. Paolo
Re: [Qemu-devel] [QEMU PATCH v5 3/6] migration: extend VMStateInfo
On 10/03/2016 08:24 PM, Jianjun Duan wrote:
> Current migration code cannot handle some data structures such as
> QTAILQ in qemu/queue.h. Here we extend the signatures of put/get
> in VMStateInfo so that customized handling is supported.
>
> Signed-off-by: Jianjun Duan
> ---
> hw/net/vmxnet3.c| 18 ++---
> hw/nvram/eeprom93xx.c | 6 ++-
> hw/nvram/fw_cfg.c | 6 ++-
> hw/pci/msix.c | 6 ++-
> hw/pci/pci.c| 12 --
> hw/pci/shpc.c | 5 ++-
> hw/scsi/scsi-bus.c | 6 ++-
> hw/timer/twl92230.c | 6 ++-
> hw/usb/redirect.c | 18 ++---
> hw/virtio/virtio-pci.c | 6 ++-
> hw/virtio/virtio.c | 6 ++-
> include/migration/vmstate.h | 10 +++--
> migration/savevm.c | 5 ++-
> migration/vmstate.c | 95
> -
> target-alpha/machine.c | 5 ++-
> target-arm/machine.c| 12 --
> target-i386/machine.c | 21 ++
> target-mips/machine.c | 10 +++--
> target-ppc/machine.c| 10 +++--
> target-sparc/machine.c | 5 ++-
> 20 files changed, 171 insertions(+), 97 deletions(-)
Hi Jianjun!
I'm not happy with the intrusive nature of this patch. We end up
with a bunch of unused parameters.
Have you considered something like:
typedef struct {
[..]
union{
const VMStateInfo *info;
const VMStateLinked *linked;
};
enum VMStateFlags flags;
[..]
} VMStateField;
/**
* Handle linked datastructures. VMStateField.liked s to be used
* in conjunction with VMStateField.vmsd which describes a node of
* the datastucture without the pointers representing the links.
* The links are embedded in the node starting at VMStateField.start.
* The on wire representation of the information contained in links
* and the head element if the responsibility of a particular VMStateField
* instance. VMStateLinked is responsible for saving/loading the whole
* sub-tree whose root is the field in question including the allocation of
* memory for the nodes. The presence of VMStateField.linked is indicated
* by the VMS_LINKED flag in VMStateField.flags.
*/
struct VMStateLinked {
const char *name;
void (*save)(QEMUFile *f, void *pv, VMStateField *field, QJSON *vmdesc);
int (*load)(QEMUFile *f, void *pv, VMStateField *field);
/* Maybe: size_t offset_links; */
};
IMHO this would:
* allow us to keep the good old MVStateInfo objects unmodified and
the semantic of VMStateInfo unchanged
* make clear that VMStateLinked does not care about the calculated size
and provide a new anchor for documentation
* if overloading the semantic of VMStateField.start is not desired we
could put it into VMStateLinked, if needed we could also put more
stuff in there
* have clearer separation between special handling for (linked/certain)
data structures and the usual scenario with the DAG.
I would also suggest unit tests in test/test-vmstate.c. Maybe with
that the vmstate migration of QTAILQ could be factored out as a separate
patch series.
Cheers,
Halil
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [QEMU PATCH v5 3/6] migration: extend VMStateInfo
* Jianjun Duan (du...@linux.vnet.ibm.com) wrote:
> Current migration code cannot handle some data structures such as
> QTAILQ in qemu/queue.h. Here we extend the signatures of put/get
> in VMStateInfo so that customized handling is supported.
>
> Signed-off-by: Jianjun Duan
> ---
> hw/net/vmxnet3.c| 18 ++---
> hw/nvram/eeprom93xx.c | 6 ++-
> hw/nvram/fw_cfg.c | 6 ++-
> hw/pci/msix.c | 6 ++-
> hw/pci/pci.c| 12 --
> hw/pci/shpc.c | 5 ++-
> hw/scsi/scsi-bus.c | 6 ++-
> hw/timer/twl92230.c | 6 ++-
> hw/usb/redirect.c | 18 ++---
> hw/virtio/virtio-pci.c | 6 ++-
> hw/virtio/virtio.c | 6 ++-
> include/migration/vmstate.h | 10 +++--
> migration/savevm.c | 5 ++-
> migration/vmstate.c | 95
> -
> target-alpha/machine.c | 5 ++-
> target-arm/machine.c| 12 --
> target-i386/machine.c | 21 ++
> target-mips/machine.c | 10 +++--
> target-ppc/machine.c| 10 +++--
> target-sparc/machine.c | 5 ++-
> 20 files changed, 171 insertions(+), 97 deletions(-)
>
> diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
> index 444672a..2ca4b46 100644
> --- a/hw/usb/redirect.c
> +++ b/hw/usb/redirect.c
> @@ -2154,7 +2154,8 @@ static int usbredir_post_load(void *priv, int
> version_id)
> }
>
> /* For usbredirparser migration */
> -static void usbredir_put_parser(QEMUFile *f, void *priv, size_t unused)
> +static void usbredir_put_parser(QEMUFile *f, void *priv, size_t unused,
> +void *opaque, QJSON *vmdesc)
> {
> USBRedirDevice *dev = priv;
> uint8_t *data;
> @@ -2174,7 +2175,8 @@ static void usbredir_put_parser(QEMUFile *f, void
> *priv, size_t unused)
> free(data);
> }
>
> -static int usbredir_get_parser(QEMUFile *f, void *priv, size_t unused)
> +static int usbredir_get_parser(QEMUFile *f, void *priv, size_t unused,
> + void *opaque)
Neither of these built for me; I had to change those to VMStateField rather
than void *;
also is this series tested ontop of Halil's patches - because without them
I'm finding I also had to fix up most of the other virtio devices.
Dave
> USBRedirDevice *dev = priv;
> uint8_t *data;
> @@ -2217,7 +2219,8 @@ static const VMStateInfo usbredir_parser_vmstate_info =
> {
>
>
> /* For buffered packets (iso/irq) queue migration */
> -static void usbredir_put_bufpq(QEMUFile *f, void *priv, size_t unused)
> +static void usbredir_put_bufpq(QEMUFile *f, void *priv, size_t unused,
> + VMStateField *field, QJSON *vmdesc)
> {
> struct endp_data *endp = priv;
> USBRedirDevice *dev = endp->dev;
> @@ -2237,7 +2240,8 @@ static void usbredir_put_bufpq(QEMUFile *f, void *priv,
> size_t unused)
> assert(i == endp->bufpq_size);
> }
>
> -static int usbredir_get_bufpq(QEMUFile *f, void *priv, size_t unused)
> +static int usbredir_get_bufpq(QEMUFile *f, void *priv, size_t unused,
> + VMStateField *field)
> {
> struct endp_data *endp = priv;
> USBRedirDevice *dev = endp->dev;
> @@ -2340,7 +2344,8 @@ static const VMStateDescription usbredir_ep_vmstate = {
>
>
> /* For PacketIdQueue migration */
> -static void usbredir_put_packet_id_q(QEMUFile *f, void *priv, size_t unused)
> +static void usbredir_put_packet_id_q(QEMUFile *f, void *priv, size_t unused,
> + VMStateField *field, QJSON *vmdesc)
> {
> struct PacketIdQueue *q = priv;
> USBRedirDevice *dev = q->dev;
> @@ -2356,7 +2361,8 @@ static void usbredir_put_packet_id_q(QEMUFile *f, void
> *priv, size_t unused)
> assert(remain == 0);
> }
>
> -static int usbredir_get_packet_id_q(QEMUFile *f, void *priv, size_t unused)
> +static int usbredir_get_packet_id_q(QEMUFile *f, void *priv, size_t unused,
> +VMStateField *field)
> {
> struct PacketIdQueue *q = priv;
> USBRedirDevice *dev = q->dev;
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 2d60a00..38a7abd 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -108,7 +108,8 @@ static bool virtio_pci_has_extra_state(DeviceState *d)
> return proxy->flags & VIRTIO_PCI_FLAG_MIGRATE_EXTRA;
> }
>
> -static int get_virtio_pci_modern_state(QEMUFile *f, void *pv, size_t size)
> +static int get_virtio_pci_modern_state(QEMUFile *f, void *pv, size_t size,
> + VMStateField *field)
> {
> VirtIOPCIProxy *proxy = pv;
> int i;
> @@ -137,7 +138,8 @@ static void
> virtio_pci_save_modern_queue_state(VirtIOPCIQueue *vq,
> qemu_put_be32(f, vq->used[1]);
> }
>
> -static void put_virtio_pci_modern_state(QEMUFile *f, void *pv, size_t size)
> +static void put_virtio_pci_modern_state(QEMUFile *f, v
[Qemu-devel] [QEMU PATCH v5 3/6] migration: extend VMStateInfo
Current migration code cannot handle some data structures such as
QTAILQ in qemu/queue.h. Here we extend the signatures of put/get
in VMStateInfo so that customized handling is supported.
Signed-off-by: Jianjun Duan
---
hw/net/vmxnet3.c| 18 ++---
hw/nvram/eeprom93xx.c | 6 ++-
hw/nvram/fw_cfg.c | 6 ++-
hw/pci/msix.c | 6 ++-
hw/pci/pci.c| 12 --
hw/pci/shpc.c | 5 ++-
hw/scsi/scsi-bus.c | 6 ++-
hw/timer/twl92230.c | 6 ++-
hw/usb/redirect.c | 18 ++---
hw/virtio/virtio-pci.c | 6 ++-
hw/virtio/virtio.c | 6 ++-
include/migration/vmstate.h | 10 +++--
migration/savevm.c | 5 ++-
migration/vmstate.c | 95 -
target-alpha/machine.c | 5 ++-
target-arm/machine.c| 12 --
target-i386/machine.c | 21 ++
target-mips/machine.c | 10 +++--
target-ppc/machine.c| 10 +++--
target-sparc/machine.c | 5 ++-
20 files changed, 171 insertions(+), 97 deletions(-)
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 90f6943..943a960 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -2450,7 +2450,8 @@ static void vmxnet3_put_tx_stats_to_file(QEMUFile *f,
qemu_put_be64(f, tx_stat->pktsTxDiscard);
}
-static int vmxnet3_get_txq_descr(QEMUFile *f, void *pv, size_t size)
+static int vmxnet3_get_txq_descr(QEMUFile *f, void *pv, size_t size,
+VMStateField *field)
{
Vmxnet3TxqDescr *r = pv;
@@ -2464,7 +2465,8 @@ static int vmxnet3_get_txq_descr(QEMUFile *f, void *pv,
size_t size)
return 0;
}
-static void vmxnet3_put_txq_descr(QEMUFile *f, void *pv, size_t size)
+static void vmxnet3_put_txq_descr(QEMUFile *f, void *pv, size_t size,
+VMStateField *field, QJSON *vmdesc)
{
Vmxnet3TxqDescr *r = pv;
@@ -2511,7 +2513,8 @@ static void vmxnet3_put_rx_stats_to_file(QEMUFile *f,
qemu_put_be64(f, rx_stat->pktsRxError);
}
-static int vmxnet3_get_rxq_descr(QEMUFile *f, void *pv, size_t size)
+static int vmxnet3_get_rxq_descr(QEMUFile *f, void *pv, size_t size,
+VMStateField *field)
{
Vmxnet3RxqDescr *r = pv;
int i;
@@ -2529,7 +2532,8 @@ static int vmxnet3_get_rxq_descr(QEMUFile *f, void *pv,
size_t size)
return 0;
}
-static void vmxnet3_put_rxq_descr(QEMUFile *f, void *pv, size_t size)
+static void vmxnet3_put_rxq_descr(QEMUFile *f, void *pv, size_t size,
+VMStateField *field, QJSON *vmdesc)
{
Vmxnet3RxqDescr *r = pv;
int i;
@@ -2574,7 +2578,8 @@ static const VMStateInfo rxq_descr_info = {
.put = vmxnet3_put_rxq_descr
};
-static int vmxnet3_get_int_state(QEMUFile *f, void *pv, size_t size)
+static int vmxnet3_get_int_state(QEMUFile *f, void *pv, size_t size,
+VMStateField *field)
{
Vmxnet3IntState *r = pv;
@@ -2585,7 +2590,8 @@ static int vmxnet3_get_int_state(QEMUFile *f, void *pv,
size_t size)
return 0;
}
-static void vmxnet3_put_int_state(QEMUFile *f, void *pv, size_t size)
+static void vmxnet3_put_int_state(QEMUFile *f, void *pv, size_t size,
+VMStateField *field, QJSON *vmdesc)
{
Vmxnet3IntState *r = pv;
diff --git a/hw/nvram/eeprom93xx.c b/hw/nvram/eeprom93xx.c
index 2c16fc2..76d5f41 100644
--- a/hw/nvram/eeprom93xx.c
+++ b/hw/nvram/eeprom93xx.c
@@ -94,14 +94,16 @@ struct _eeprom_t {
This is a Big hack, but it is how the old state did it.
*/
-static int get_uint16_from_uint8(QEMUFile *f, void *pv, size_t size)
+static int get_uint16_from_uint8(QEMUFile *f, void *pv, size_t size,
+ VMStateField *field)
{
uint16_t *v = pv;
*v = qemu_get_ubyte(f);
return 0;
}
-static void put_unused(QEMUFile *f, void *pv, size_t size)
+static void put_unused(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
fprintf(stderr, "uint16_from_uint8 is used only for backwards
compatibility.\n");
fprintf(stderr, "Never should be used to write a new state.\n");
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 92aa563..a8a4a7a 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -524,14 +524,16 @@ static void fw_cfg_reset(DeviceState *d)
Or we broke compatibility in the state, or we can't use struct tm
*/
-static int get_uint32_as_uint16(QEMUFile *f, void *pv, size_t size)
+static int get_uint32_as_uint16(QEMUFile *f, void *pv, size_t size,
+VMStateField *field)
{
uint32_t *v = pv;
*v = qemu_get_be16(f);
return 0;
}
-static void put_unused(QEMUFile *f, void *pv, size_t size)
+static void put_unused(QEMUFile *f, void *pv, size_t size, VMStateField *field,
+ QJSON *vmdesc)
{
fprintf(stderr, "uint32_as_uint16 is only used for backward
compatibility.\n");
fprintf(stderr, "This functions shouldn't be called.\n");
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 0ec1cb1..69e7
