| Message ID | 20170606025135.2685-2-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 6 June 2017 at 03:51, David Gibson <david@gibson.dropbear.id.au> wrote: > From: Laurent Vivier <lvivier@redhat.com> > > We can replace the four remaining calls of register_savevm() by > calls to register_savevm_live(). So we can remove the function and > as we don't allocate anymore the ops pointer with g_new0() > we don't have to free it then. > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > Reviewed-by: Juan Quintela <quintela@redhat.com> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/net/vmxnet3.c | 8 ++++++-- > hw/s390x/s390-skeys.c | 9 +++++++-- > hw/s390x/s390-virtio-ccw.c | 8 ++++++-- > include/migration/vmstate.h | 8 -------- > migration/savevm.c | 16 ---------------- > slirp/slirp.c | 8 ++++++-- > 6 files changed, 25 insertions(+), 32 deletions(-) Great to see register_savevm() finally disappearing. Any chance of an update to docs/migration.txt, which still mentions register_savevm(), but on the other hand doesn't say anything about register_savevm_live() and unregister_savevm(). (Doc comments in the .h file for those functions would be nice too...) Things that would be interesting to explain/document: * what is special about vmxnet3 that makes it the only pci device that needs to use this rather than having a vmstate struct? * why does s390-skeys call the register function with a NULL pointer but the unregister pointer with a device pointer? (Could we replace the uses of these which pass a dev pointer with vmstate structs and then drop the dev parameter?) thanks -- PMM
On 06/06/2017 19:49, Peter Maydell wrote: > Things that would be interesting to explain/document: > * what is special about vmxnet3 that makes it the only pci device > that needs to use this rather than having a vmstate struct? Nothing, it should be replaced by VMSTATE_MSIX (at the cost of breaking migration from and to older QEMU versions). Paolo
Peter Maydell <peter.maydell@linaro.org> wrote: > On 6 June 2017 at 03:51, David Gibson <david@gibson.dropbear.id.au> wrote: >> From: Laurent Vivier <lvivier@redhat.com> >> >> We can replace the four remaining calls of register_savevm() by >> calls to register_savevm_live(). So we can remove the function and >> as we don't allocate anymore the ops pointer with g_new0() >> we don't have to free it then. >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> Reviewed-by: Juan Quintela <quintela@redhat.com> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >> --- >> hw/net/vmxnet3.c | 8 ++++++-- >> hw/s390x/s390-skeys.c | 9 +++++++-- >> hw/s390x/s390-virtio-ccw.c | 8 ++++++-- >> include/migration/vmstate.h | 8 -------- >> migration/savevm.c | 16 ---------------- >> slirp/slirp.c | 8 ++++++-- >> 6 files changed, 25 insertions(+), 32 deletions(-) > > Great to see register_savevm() finally disappearing. > > Any chance of an update to docs/migration.txt, which still > mentions register_savevm(), but on the other hand doesn't > say anything about register_savevm_live() and unregister_savevm(). > (Doc comments in the .h file for those functions would be > nice too...) Ok, will take a look. > Things that would be interesting to explain/document: > * what is special about vmxnet3 that makes it the only pci device > that needs to use this rather than having a vmstate struct? Will take a look. vmxnet3 used to be a mess (in relation to migration). > * why does s390-skeys call the register function with a NULL > pointer but the unregister pointer with a device pointer? No clue, will left that > (Could we replace the uses of these which pass a dev pointer > with vmstate structs and then drop the dev parameter?) Not sure, have to take a look. Thanks, Juan.
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index 8b1fab2..4df3110 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -2262,6 +2262,11 @@ static const MemoryRegionOps b1_ops = { }, }; +static SaveVMHandlers savevm_vmxnet3_msix = { + .save_state = vmxnet3_msix_save, + .load_state = vmxnet3_msix_load, +}; + static uint64_t vmxnet3_device_serial_num(VMXNET3State *s) { uint64_t dsn_payload; @@ -2331,8 +2336,7 @@ static void vmxnet3_pci_realize(PCIDevice *pci_dev, Error **errp) vmxnet3_device_serial_num(s)); } - register_savevm(dev, "vmxnet3-msix", -1, 1, - vmxnet3_msix_save, vmxnet3_msix_load, s); + register_savevm_live(dev, "vmxnet3-msix", -1, 1, &savevm_vmxnet3_msix, s); } static void vmxnet3_instance_init(Object *obj) diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c index 619152c..35e7f63 100644 --- a/hw/s390x/s390-skeys.c +++ b/hw/s390x/s390-skeys.c @@ -362,6 +362,11 @@ static inline bool s390_skeys_get_migration_enabled(Object *obj, Error **errp) return ss->migration_enabled; } +static SaveVMHandlers savevm_s390_storage_keys = { + .save_state = s390_storage_keys_save, + .load_state = s390_storage_keys_load, +}; + static inline void s390_skeys_set_migration_enabled(Object *obj, bool value, Error **errp) { @@ -375,8 +380,8 @@ static inline void s390_skeys_set_migration_enabled(Object *obj, bool value, ss->migration_enabled = value; if (ss->migration_enabled) { - register_savevm(NULL, TYPE_S390_SKEYS, 0, 1, s390_storage_keys_save, - s390_storage_keys_load, ss); + register_savevm_live(NULL, TYPE_S390_SKEYS, 0, 1, + &savevm_s390_storage_keys, ss); } else { unregister_savevm(DEVICE(ss), TYPE_S390_SKEYS, ss); } diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c index c9021f2..a806345 100644 --- a/hw/s390x/s390-virtio-ccw.c +++ b/hw/s390x/s390-virtio-ccw.c @@ -104,6 +104,11 @@ void s390_memory_init(ram_addr_t mem_size) s390_skeys_init(); } +static SaveVMHandlers savevm_gtod = { + .save_state = gtod_save, + .load_state = gtod_load, +}; + static void ccw_init(MachineState *machine) { int ret; @@ -151,8 +156,7 @@ static void ccw_init(MachineState *machine) s390_create_virtio_net(BUS(css_bus), "virtio-net-ccw"); /* Register savevm handler for guest TOD clock */ - register_savevm(NULL, "todclock", 0, 1, - gtod_save, gtod_load, kvm_state); + register_savevm_live(NULL, "todclock", 0, 1, &savevm_gtod, kvm_state); } static void s390_cpu_plug(HotplugHandler *hotplug_dev, diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h index 6689562..8a3e9e6 100644 --- a/include/migration/vmstate.h +++ b/include/migration/vmstate.h @@ -59,14 +59,6 @@ typedef struct SaveVMHandlers { LoadStateHandler *load_state; } SaveVMHandlers; -int register_savevm(DeviceState *dev, - const char *idstr, - int instance_id, - int version_id, - SaveStateHandler *save_state, - LoadStateHandler *load_state, - void *opaque); - int register_savevm_live(DeviceState *dev, const char *idstr, int instance_id, diff --git a/migration/savevm.c b/migration/savevm.c index 9c320f5..035c127 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -645,21 +645,6 @@ int register_savevm_live(DeviceState *dev, return 0; } -int register_savevm(DeviceState *dev, - const char *idstr, - int instance_id, - int version_id, - SaveStateHandler *save_state, - LoadStateHandler *load_state, - void *opaque) -{ - SaveVMHandlers *ops = g_new0(SaveVMHandlers, 1); - ops->save_state = save_state; - ops->load_state = load_state; - return register_savevm_live(dev, idstr, instance_id, version_id, - ops, opaque); -} - void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque) { SaveStateEntry *se, *new_se; @@ -679,7 +664,6 @@ void unregister_savevm(DeviceState *dev, const char *idstr, void *opaque) if (strcmp(se->idstr, id) == 0 && se->opaque == opaque) { QTAILQ_REMOVE(&savevm_state.handlers, se, entry); g_free(se->compat); - g_free(se->ops); g_free(se); } } diff --git a/slirp/slirp.c b/slirp/slirp.c index e79345b..2386493 100644 --- a/slirp/slirp.c +++ b/slirp/slirp.c @@ -272,6 +272,11 @@ static void slirp_init_once(void) static void slirp_state_save(QEMUFile *f, void *opaque); static int slirp_state_load(QEMUFile *f, void *opaque, int version_id); +static SaveVMHandlers savevm_slirp_state = { + .save_state = slirp_state_save, + .load_state = slirp_state_load, +}; + Slirp *slirp_init(int restricted, bool in_enabled, struct in_addr vnetwork, struct in_addr vnetmask, struct in_addr vhost, bool in6_enabled, @@ -321,8 +326,7 @@ Slirp *slirp_init(int restricted, bool in_enabled, struct in_addr vnetwork, slirp->opaque = opaque; - register_savevm(NULL, "slirp", 0, 4, - slirp_state_save, slirp_state_load, slirp); + register_savevm_live(NULL, "slirp", 0, 4, &savevm_slirp_state, slirp); QTAILQ_INSERT_TAIL(&slirp_instances, slirp, entry);