[PATCH 00/13] migration: Check for duplicates on vmstate_register()

[PATCH 00/13] migration: Check for duplicates on vmstate_register()

[Juan Quintela]

Hi

This series are based in a patch from Peter than check if a we try to
register the same device with the same instance_id more than once.  It
was not merged when he sent it because it broke "make check".  So I
fixed all devices to be able to merge it.

- I create vmstate_register_any(), its the same that
  vmstate_register(VMSTATE_INSTANCE_ID_ANY)
- Later I check in vmstate_register() that they are not calling it
  with VMSTATE_INSTANCE_ID_ANY
- After that I change vmstate_register() to make sure that we don't
  include a duplicate.

And we get all the errors that I change in patches 3, 4, 5, 6, 7.
After those patches: make check works again.
And then I reviewed all the rest of vmstate_register() callers.

There are the cases where they pass a device_id that is generated
somehow, that ones are ok.

Then we have the ones that pass always 0.  This ones are only valid
when there is a maximum of one device instantiated for a given
machine.

- audio: you can choose more than one audio output.
- eeprom93xx: you can have more than one e100 card.

- vmware_vga: I am not completely sure here, it appears that you could
  have more than one.  Notice that VMSTATE_INSTANCE_ID_ANY will give
  us the value 0 if there is only one instance, so we are in no
  trouble.  We can drop it if people think that we can't have more
  than one vmware_vga.

- for the rest of the devices, I can't see any that can be
  instantiated more than once (testing it is easy, just starting the
  machine will make it fail).  Notice that again, for the same
  reasoning, we could change all the calls to _any().  And only left
  the vmstate_register(... 0 ...) calls for devices that we know that
  we only ever want one.

What needs to be done:

- icp/server: We need to rename the old icp server name.  Notice that
  I doubt that anyone is migrating this, but I need help from PPC
  experts.  As said in the commit message, it is "abusing" the interface:
  - it register a new device
  - it realizes that it is instantiting an old beard
  - it unregister the new device
  - it registers the old device

- rest of devices:

  * pxa2xx devices: I can't see how you can create more than one
    device in a machine
  * acpi_build: I can't see how to create more than once.
  * replay: neither
  * cpu timers: created in vl.c
  * global_state: only once
  * s390 css: not a way that I can think
  * spapr: looks only one
  * or1ktimer: I can only see one
  * tsc*: I see only use in pxa2xx and one by board

- And now, another abuser:

vmstate_register(VMSTATE_IF(tcet), tcet->liobn, &vmstate_spapr_tce_table,

tcet->liobn is an uint32_t, and instance_id is an int.  And it just happens that is value is < VMSTATE_INSTANCE_ID_ANY.

Please, review.

Juan Quintela (12):
  migration: Create vmstate_register_any()
  migration: Use vmstate_register_any()
  migration: Use vmstate_register_any() for isa-ide
  migration: Use vmstate_register_any() for ipmi-bt*
  migration: Use VMSTATE_INSTANCE_ID_ANY for slirp
  migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices
  RFC migration: icp/server is a mess
  migration: vmstate_register() check that instance_id is valid
  migration: Improve example and documentation of vmstate_register()
  migration: Use vmstate_register_any() for audio
  migration: Use vmstate_register_any() for eeprom93xx
  migration: Use vmstate_register_any() for vmware_vga

Peter Xu (1):
  migration: Check in savevm_state_handler_insert for dups

 docs/devel/migration.rst    | 12 ++++++++----
 include/migration/vmstate.h | 23 +++++++++++++++++++++++
 audio/audio.c               |  2 +-
 backends/dbus-vmstate.c     |  3 +--
 backends/tpm/tpm_emulator.c |  3 +--
 hw/display/vmware_vga.c     |  2 +-
 hw/i2c/core.c               |  2 +-
 hw/ide/isa.c                |  2 +-
 hw/input/adb.c              |  2 +-
 hw/input/ads7846.c          |  2 +-
 hw/input/stellaris_input.c  |  3 +--
 hw/ipmi/ipmi_bmc_extern.c   |  2 +-
 hw/ipmi/ipmi_bmc_sim.c      |  2 +-
 hw/ipmi/isa_ipmi_bt.c       |  2 +-
 hw/ipmi/isa_ipmi_kcs.c      |  2 +-
 hw/net/eepro100.c           |  3 +--
 hw/nvram/eeprom93xx.c       |  2 +-
 hw/pci/pci.c                |  2 +-
 hw/ppc/spapr.c              |  7 ++++++-
 hw/ppc/spapr_nvdimm.c       |  3 +--
 hw/s390x/s390-skeys.c       |  3 ++-
 hw/s390x/s390-stattrib.c    |  3 ++-
 hw/timer/arm_timer.c        |  2 +-
 hw/virtio/virtio-mem.c      |  4 ++--
 migration/savevm.c          | 14 ++++++++++++++
 net/slirp.c                 |  5 +++--
 26 files changed, 78 insertions(+), 34 deletions(-)

-- 
2.41.0

[PATCH 01/13] migration: Create vmstate_register_any()

[Juan Quintela]

We have lots of cases where we are using an instance_id==0 when we
should be using VMSTATE_INSTANCE_ID_ANY (-1).  Basically everything
that can have more than one needs to have a proper instance_id or -1
and the system will take one for it.

vmstate_register_any(): We register with -1.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/vmstate.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1a31fb7293..9ca7e9cc48 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1230,6 +1230,23 @@ static inline int vmstate_register(VMStateIf *obj, int instance_id,
                                           opaque, -1, 0, NULL);
 }
 
+/**
+ * vmstate_register_any() - legacy function to register state
+ * serialisation description and let the function choose the id
+ *
+ * New code shouldn't be using this function as QOM-ified devices have
+ * dc->vmsd to store the serialisation description.
+ *
+ * Returns: 0 on success, -1 on failure
+ */
+static inline int vmstate_register_any(VMStateIf *obj,
+                                       const VMStateDescription *vmsd,
+                                       void *opaque)
+{
+    return vmstate_register_with_alias_id(obj, VMSTATE_INSTANCE_ID_ANY, vmsd,
+                                          opaque, -1, 0, NULL);
+}
+
 void vmstate_unregister(VMStateIf *obj, const VMStateDescription *vmsd,
                         void *opaque);
 
-- 
2.41.0

[PATCH 02/13] migration: Use vmstate_register_any()

[Juan Quintela]

This are the easiest cases, where we were already using
VMSTATE_INSTANCE_ID_ANY.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 backends/dbus-vmstate.c     | 3 +--
 backends/tpm/tpm_emulator.c | 3 +--
 hw/i2c/core.c               | 2 +-
 hw/input/adb.c              | 2 +-
 hw/input/ads7846.c          | 2 +-
 hw/input/stellaris_input.c  | 3 +--
 hw/net/eepro100.c           | 3 +--
 hw/pci/pci.c                | 2 +-
 hw/ppc/spapr_nvdimm.c       | 3 +--
 hw/timer/arm_timer.c        | 2 +-
 hw/virtio/virtio-mem.c      | 4 ++--
 11 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
index 57369ec0f2..a9d8cb0acd 100644
--- a/backends/dbus-vmstate.c
+++ b/backends/dbus-vmstate.c
@@ -426,8 +426,7 @@ dbus_vmstate_complete(UserCreatable *uc, Error **errp)
         return;
     }
 
-    if (vmstate_register(VMSTATE_IF(self), VMSTATE_INSTANCE_ID_ANY,
-                         &dbus_vmstate, self) < 0) {
+    if (vmstate_register_any(VMSTATE_IF(self), &dbus_vmstate, self) < 0) {
         error_setg(errp, "Failed to register vmstate");
     }
 }
diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index 402a2d6312..8920b75251 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -978,8 +978,7 @@ static void tpm_emulator_inst_init(Object *obj)
         qemu_add_vm_change_state_handler(tpm_emulator_vm_state_change,
                                          tpm_emu);
 
-    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
-                     &vmstate_tpm_emulator, obj);
+    vmstate_register_any(NULL, &vmstate_tpm_emulator, obj);
 }
 
 /*
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index bed594fe59..879a1d45cb 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -64,7 +64,7 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
     bus = I2C_BUS(qbus_new(TYPE_I2C_BUS, parent, name));
     QLIST_INIT(&bus->current_devs);
     QSIMPLEQ_INIT(&bus->pending_masters);
-    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_i2c_bus, bus);
+    vmstate_register_any(NULL, &vmstate_i2c_bus, bus);
     return bus;
 }
 
diff --git a/hw/input/adb.c b/hw/input/adb.c
index 214ae6f42b..8aed0da2cd 100644
--- a/hw/input/adb.c
+++ b/hw/input/adb.c
@@ -247,7 +247,7 @@ static void adb_bus_realize(BusState *qbus, Error **errp)
     adb_bus->autopoll_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, adb_autopoll,
                                            adb_bus);
 
-    vmstate_register(NULL, -1, &vmstate_adb_bus, adb_bus);
+    vmstate_register_any(NULL, &vmstate_adb_bus, adb_bus);
 }
 
 static void adb_bus_unrealize(BusState *qbus)
diff --git a/hw/input/ads7846.c b/hw/input/ads7846.c
index dc0998ac79..91116c6bdb 100644
--- a/hw/input/ads7846.c
+++ b/hw/input/ads7846.c
@@ -158,7 +158,7 @@ static void ads7846_realize(SSIPeripheral *d, Error **errp)
 
     ads7846_int_update(s);
 
-    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_ads7846, s);
+    vmstate_register_any(NULL, &vmstate_ads7846, s);
 }
 
 static void ads7846_class_init(ObjectClass *klass, void *data)
diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c
index e6ee5e11f1..a58721c8cd 100644
--- a/hw/input/stellaris_input.c
+++ b/hw/input/stellaris_input.c
@@ -88,6 +88,5 @@ void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode)
     }
     s->num_buttons = n;
     qemu_add_kbd_event_handler(stellaris_gamepad_put_key, s);
-    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
-                     &vmstate_stellaris_gamepad, s);
+    vmstate_register_any(NULL, &vmstate_stellaris_gamepad, s);
 }
diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index dc07984ae9..94ce9e18ff 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -1883,8 +1883,7 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
 
     s->vmstate = g_memdup(&vmstate_eepro100, sizeof(vmstate_eepro100));
     s->vmstate->name = qemu_get_queue(s->nic)->model;
-    vmstate_register(VMSTATE_IF(&pci_dev->qdev), VMSTATE_INSTANCE_ID_ANY,
-                     s->vmstate, s);
+    vmstate_register_any(VMSTATE_IF(&pci_dev->qdev), s->vmstate, s);
 }
 
 static void eepro100_instance_init(Object *obj)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index b0d21bf43a..294c3c38ea 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -147,7 +147,7 @@ static void pci_bus_realize(BusState *qbus, Error **errp)
     bus->machine_done.notify = pcibus_machine_done;
     qemu_add_machine_init_done_notifier(&bus->machine_done);
 
-    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_pcibus, bus);
+    vmstate_register_any(NULL, &vmstate_pcibus, bus);
 }
 
 static void pcie_bus_realize(BusState *qbus, Error **errp)
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index b2f009c816..ad7afe7544 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -876,8 +876,7 @@ static void spapr_nvdimm_realize(NVDIMMDevice *dimm, Error **errp)
         s_nvdimm->hcall_flush_required = true;
     }
 
-    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
-                     &vmstate_spapr_nvdimm_states, dimm);
+    vmstate_register_any(NULL, &vmstate_spapr_nvdimm_states, dimm);
 }
 
 static void spapr_nvdimm_unrealize(NVDIMMDevice *dimm)
diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
index 69c8863472..9afe8da831 100644
--- a/hw/timer/arm_timer.c
+++ b/hw/timer/arm_timer.c
@@ -181,7 +181,7 @@ static arm_timer_state *arm_timer_init(uint32_t freq)
     s->control = TIMER_CTRL_IE;
 
     s->timer = ptimer_init(arm_timer_tick, s, PTIMER_POLICY_LEGACY);
-    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_arm_timer, s);
+    vmstate_register_any(NULL, &vmstate_arm_timer, s);
     return s;
 }
 
diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 9dc3c61b5a..a5ea3be414 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -1119,8 +1119,8 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
     host_memory_backend_set_mapped(vmem->memdev, true);
     vmstate_register_ram(&vmem->memdev->mr, DEVICE(vmem));
     if (vmem->early_migration) {
-        vmstate_register(VMSTATE_IF(vmem), VMSTATE_INSTANCE_ID_ANY,
-                         &vmstate_virtio_mem_device_early, vmem);
+        vmstate_register_any(VMSTATE_IF(vmem),
+                             &vmstate_virtio_mem_device_early, vmem);
     }
     qemu_register_reset(virtio_mem_system_reset, vmem);
 
-- 
2.41.0

[PATCH 03/13] migration: Use vmstate_register_any() for isa-ide

[Juan Quintela]

Otherwise qom-test fails.

ok 4 /i386/qom/x-remote
qemu-system-i386: savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=isa-ide, instance_id=0x0
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)
$

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ide/isa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 95053e026f..ea60c08116 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -73,7 +73,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
     ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2);
     ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
     ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum));
-    vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
+    vmstate_register_any(VMSTATE_IF(dev), &vmstate_ide_isa, s);
     ide_bus_register_restart_cb(&s->bus);
 }
 
-- 
2.41.0

[PATCH 04/13] migration: Use vmstate_register_any() for ipmi-bt*

[Juan Quintela]

Otherwise device-introspection-test fails.

$ ./tests/qtest/device-introspect-test
...
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ipmi/ipmi_bmc_extern.c | 2 +-
 hw/ipmi/ipmi_bmc_sim.c    | 2 +-
 hw/ipmi/isa_ipmi_bt.c     | 2 +-
 hw/ipmi/isa_ipmi_kcs.c    | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
index e232d35ba2..324a2c8835 100644
--- a/hw/ipmi/ipmi_bmc_extern.c
+++ b/hw/ipmi/ipmi_bmc_extern.c
@@ -504,7 +504,7 @@ static void ipmi_bmc_extern_init(Object *obj)
     IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
 
     ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
-    vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe);
+    vmstate_register_any(NULL, &vmstate_ipmi_bmc_extern, ibe);
 }
 
 static void ipmi_bmc_extern_finalize(Object *obj)
diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
index 905e091094..404db5d5bc 100644
--- a/hw/ipmi/ipmi_bmc_sim.c
+++ b/hw/ipmi/ipmi_bmc_sim.c
@@ -2188,7 +2188,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
 
     ibs->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ipmi_timeout, ibs);
 
-    vmstate_register(NULL, 0, &vmstate_ipmi_sim, ibs);
+    vmstate_register_any(NULL, &vmstate_ipmi_sim, ibs);
 }
 
 static Property ipmi_sim_properties[] = {
diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index a83e7243d6..afb76b548a 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -125,7 +125,7 @@ static void isa_ipmi_bt_init(Object *obj)
 
     ipmi_bmc_find_and_link(obj, (Object **) &iib->bt.bmc);
 
-    vmstate_register(NULL, 0, &vmstate_ISAIPMIBTDevice, iib);
+    vmstate_register_any(NULL, &vmstate_ISAIPMIBTDevice, iib);
 }
 
 static void *isa_ipmi_bt_get_backend_data(IPMIInterface *ii)
diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index b2ed70b9da..5ab63b2fcf 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -132,7 +132,7 @@ static void isa_ipmi_kcs_init(Object *obj)
      * IPMI device, so receive it, but transmit a different
      * version.
      */
-    vmstate_register(NULL, 0, &vmstate_ISAIPMIKCSDevice, iik);
+    vmstate_register_any(NULL, &vmstate_ISAIPMIKCSDevice, iik);
 }
 
 static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)
-- 
2.41.0

[PATCH 05/13] migration: Use VMSTATE_INSTANCE_ID_ANY for slirp

[Juan Quintela]

Each user network conection create a new slirp instance.  We register
more than one slirp instance for number 0.

qemu-system-x86_64: -netdev user,id=hs1: savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=slirp, instance_id=0x0
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 net/slirp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/slirp.c b/net/slirp.c
index c33b3e02e7..25b49c4526 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -46,6 +46,7 @@
 #include "qapi/qmp/qdict.h"
 #include "util.h"
 #include "migration/register.h"
+#include "migration/vmstate.h"
 #include "migration/qemu-file-types.h"
 
 static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
@@ -659,8 +660,8 @@ static int net_slirp_init(NetClientState *peer, const char *model,
      * specific version?
      */
     g_assert(slirp_state_version() == 4);
-    register_savevm_live("slirp", 0, slirp_state_version(),
-                         &savevm_slirp_state, s->slirp);
+    register_savevm_live("slirp", VMSTATE_INSTANCE_ID_ANY,
+                         slirp_state_version(), &savevm_slirp_state, s->slirp);
 
     s->poll_notifier.notify = net_slirp_poll_notify;
     main_loop_poll_add_notifier(&s->poll_notifier);
-- 
2.41.0

[PATCH 06/13] migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices

[Juan Quintela]

Just with make check I can see that we can have more than one of this
devices, so use ANY.

ok 5 /s390x/device/introspect/abstract-interfaces
...
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/s390x/s390-skeys.c    | 3 ++-
 hw/s390x/s390-stattrib.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 5024faf411..ef089e1967 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -22,6 +22,7 @@
 #include "sysemu/kvm.h"
 #include "migration/qemu-file-types.h"
 #include "migration/register.h"
+#include "migration/vmstate.h"
 
 #define S390_SKEYS_BUFFER_SIZE (128 * KiB)  /* Room for 128k storage keys */
 #define S390_SKEYS_SAVE_FLAG_EOS 0x01
@@ -457,7 +458,7 @@ static inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
     ss->migration_enabled = value;
 
     if (ss->migration_enabled) {
-        register_savevm_live(TYPE_S390_SKEYS, 0, 1,
+        register_savevm_live(TYPE_S390_SKEYS, VMSTATE_INSTANCE_ID_ANY, 1,
                              &savevm_s390_storage_keys, ss);
     } else {
         unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 220e845d12..055d382c3c 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -13,6 +13,7 @@
 #include "qemu/units.h"
 #include "migration/qemu-file.h"
 #include "migration/register.h"
+#include "migration/vmstate.h"
 #include "hw/s390x/storage-attributes.h"
 #include "qemu/error-report.h"
 #include "exec/ram_addr.h"
@@ -380,7 +381,7 @@ static void s390_stattrib_instance_init(Object *obj)
 {
     S390StAttribState *sas = S390_STATTRIB(obj);
 
-    register_savevm_live(TYPE_S390_STATTRIB, 0, 0,
+    register_savevm_live(TYPE_S390_STATTRIB, VMSTATE_INSTANCE_ID_ANY, 0,
                          &savevm_s390_stattrib_handlers, sas);
 
     object_property_add_bool(obj, "migration-enabled",
-- 
2.41.0

[PATCH 07/13] RFC migration: icp/server is a mess

[Juan Quintela]

Current code does:
- register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
  dependinfg on cpu number
- for newer machines, it register vmstate_icp with "icp/server" name
  and instance 0
- now it unregisters "icp/server" for the 1st instance.

This is wrong at many levels:
- we shouldn't have two VMSTATEDescriptions with the same name
- In case this is the only solution that we can came with, it needs to
  be:
  * register pre_2_10_vmstate_dummy_icp
  * unregister pre_2_10_vmstate_dummy_icp
  * register real vmstate_icp

As the initialization of this machine is already complex enough, I
need help from PPC maintainers to fix this.

Volunteers?

CC: Cedric Le Goater <clg@kaod.org>
CC: Daniel Henrique Barboza <danielhb413@gmail.com>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Greg Kurz <groug@kaod.org>

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/ppc/spapr.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cb840676d3..8531d13492 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
 }
 
 static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
-    .name = "icp/server",
+    /*
+     * Hack ahead.  We can't have two devices with the same name and
+     * instance id.  So I rename this to pass make check.
+     * Real help from people who knows the hardware is needed.
+     */
+    .name = "pre-2.10-icp/server",
     .version_id = 1,
     .minimum_version_id = 1,
     .needed = pre_2_10_vmstate_dummy_icp_needed,
-- 
2.41.0

[PATCH 08/13] migration: vmstate_register() check that instance_id is valid

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 include/migration/vmstate.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9ca7e9cc48..d1282a78ef 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -28,6 +28,7 @@
 #define QEMU_VMSTATE_H
 
 #include "hw/vmstate-if.h"
+#include "qemu/error-report.h"
 
 typedef struct VMStateInfo VMStateInfo;
 typedef struct VMStateField VMStateField;
@@ -1226,6 +1227,11 @@ static inline int vmstate_register(VMStateIf *obj, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque)
 {
+    if (instance_id == VMSTATE_INSTANCE_ID_ANY) {
+        error_report("vmstate_register: Invalid device: %s instance_id: %d",
+                     vmsd->name, instance_id);
+        return -1;
+    }
     return vmstate_register_with_alias_id(obj, instance_id, vmsd,
                                           opaque, -1, 0, NULL);
 }
-- 
2.41.0

[PATCH 09/13] migration: Check in savevm_state_handler_insert for dups

[Juan Quintela]

From: Peter Xu <peterx@redhat.com>

Before finally register one SaveStateEntry, we detect for duplicated
entries.  This could be helpful to notify us asap instead of get
silent migration failures which could be hard to diagnose.

For example, this patch will generate a message like this (if without
previous fixes on x2apic) as long as we wants to boot a VM instance
with "-smp 200,maxcpus=288,sockets=2,cores=72,threads=2" and QEMU will
bail out even before VM starts:

savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=apic, instance_id=0x0

Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/savevm.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/migration/savevm.c b/migration/savevm.c
index 8622f229e5..e26833eaae 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -237,6 +237,8 @@ static SaveState savevm_state = {
     .global_section_id = 0,
 };
 
+static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id);
+
 static bool should_validate_capability(int capability)
 {
     assert(capability >= 0 && capability < MIGRATION_CAPABILITY__MAX);
@@ -716,6 +718,18 @@ static void savevm_state_handler_insert(SaveStateEntry *nse)
 
     assert(priority <= MIG_PRI_MAX);
 
+    /*
+     * This should never happen otherwise migration will probably fail
+     * silently somewhere because we can be wrongly applying one
+     * object properties upon another one.  Bail out ASAP.
+     */
+    if (find_se(nse->idstr, nse->instance_id)) {
+        error_report("%s: Detected duplicate SaveStateEntry: "
+                     "id=%s, instance_id=0x%"PRIx32, __func__,
+                     nse->idstr, nse->instance_id);
+        exit(EXIT_FAILURE);
+    }
+
     for (i = priority - 1; i >= 0; i--) {
         se = savevm_state.handler_pri_head[i];
         if (se != NULL) {
-- 
2.41.0

[PATCH 10/13] migration: Improve example and documentation of vmstate_register()

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 docs/devel/migration.rst | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
index c3e1400c0c..a9fde75862 100644
--- a/docs/devel/migration.rst
+++ b/docs/devel/migration.rst
@@ -165,13 +165,17 @@ An example (from hw/input/pckbd.c)
       }
   };
 
-We are declaring the state with name "pckbd".
-The ``version_id`` is 3, and the fields are 4 uint8_t in a KBDState structure.
-We registered this with:
+We are declaring the state with name "pckbd".  The ``version_id`` is
+3, and the fields are 4 uint8_t in a KBDState structure.  We
+registered this with one of those.  The first one will generate a
+device ``instance_id`` different for each registration.  Use the
+second one if you already have an id different for each instance of
+the device:
 
 .. code:: c
 
-    vmstate_register(NULL, 0, &vmstate_kbd, s);
+    vmstate_register_any(NULL, &vmstate_kbd, s);
+    vmstate_register(NULL, instance_id, &vmstate_kbd, s);
 
 For devices that are ``qdev`` based, we can register the device in the class
 init function:
-- 
2.41.0

[PATCH 11/13] migration: Use vmstate_register_any() for audio

[Juan Quintela]

We can have more than one audio card.

void audio_init_audiodevs(void)
{
    AudiodevListEntry *e;

    QSIMPLEQ_FOREACH(e, &audiodevs, next) {
        audio_init(e->dev, &error_fatal);
    }
}

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 audio/audio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/audio/audio.c b/audio/audio.c
index e9815d6812..f91e05b72c 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1781,7 +1781,7 @@ static AudioState *audio_init(Audiodev *dev, Error **errp)
 
     QTAILQ_INSERT_TAIL(&audio_states, s, list);
     QLIST_INIT (&s->card_head);
-    vmstate_register (NULL, 0, &vmstate_audio, s);
+    vmstate_register_any(NULL, &vmstate_audio, s);
     return s;
 
 out:
-- 
2.41.0

[PATCH 12/13] migration: Use vmstate_register_any() for eeprom93xx

[Juan Quintela]

We can have more than one eeprom93xx.
For instance:

e100_nic_realize() -> eeprom93xx_new()

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/nvram/eeprom93xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvram/eeprom93xx.c b/hw/nvram/eeprom93xx.c
index 1081e2cc0d..57d63638d7 100644
--- a/hw/nvram/eeprom93xx.c
+++ b/hw/nvram/eeprom93xx.c
@@ -321,7 +321,7 @@ eeprom_t *eeprom93xx_new(DeviceState *dev, uint16_t nwords)
     /* Output DO is tristate, read results in 1. */
     eeprom->eedo = 1;
     logout("eeprom = 0x%p, nwords = %u\n", eeprom, nwords);
-    vmstate_register(VMSTATE_IF(dev), 0, &vmstate_eeprom, eeprom);
+    vmstate_register_any(VMSTATE_IF(dev), &vmstate_eeprom, eeprom);
     return eeprom;
 }
 
-- 
2.41.0

[PATCH 13/13] migration: Use vmstate_register_any() for vmware_vga

[Juan Quintela]

I have no idea if we can have more than one vmware_vga device, so play
it safe.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/display/vmware_vga.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 09591fbd39..7490d43881 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1264,7 +1264,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
 
     vga_common_init(&s->vga, OBJECT(dev), &error_fatal);
     vga_init(&s->vga, OBJECT(dev), address_space, io, true);
-    vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga);
+    vmstate_register_any(NULL, &vmstate_vga_common, &s->vga);
     s->new_depth = 32;
 }
 
-- 
2.41.0

Re: [PATCH 01/13] migration: Create vmstate_register_any()

[Stefan Berger]

On 10/19/23 15:08, Juan Quintela wrote:
> We have lots of cases where we are using an instance_id==0 when we
> should be using VMSTATE_INSTANCE_ID_ANY (-1).  Basically everything
> that can have more than one needs to have a proper instance_id or -1
> and the system will take one for it.
>
> vmstate_register_any(): We register with -1.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>


Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   include/migration/vmstate.h | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1a31fb7293..9ca7e9cc48 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1230,6 +1230,23 @@ static inline int vmstate_register(VMStateIf *obj, int instance_id,
>                                             opaque, -1, 0, NULL);
>   }
>
> +/**
> + * vmstate_register_any() - legacy function to register state
> + * serialisation description and let the function choose the id
> + *
> + * New code shouldn't be using this function as QOM-ified devices have
> + * dc->vmsd to store the serialisation description.
> + *
> + * Returns: 0 on success, -1 on failure
> + */
> +static inline int vmstate_register_any(VMStateIf *obj,
> +                                       const VMStateDescription *vmsd,
> +                                       void *opaque)
> +{
> +    return vmstate_register_with_alias_id(obj, VMSTATE_INSTANCE_ID_ANY, vmsd,
> +                                          opaque, -1, 0, NULL);
> +}
> +
>   void vmstate_unregister(VMStateIf *obj, const VMStateDescription *vmsd,
>                           void *opaque);
>

Re: [PATCH 02/13] migration: Use vmstate_register_any()

[Stefan Berger]

On 10/19/23 15:08, Juan Quintela wrote:
> This are the easiest cases, where we were already using
> VMSTATE_INSTANCE_ID_ANY.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>


Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   backends/dbus-vmstate.c     | 3 +--
>   backends/tpm/tpm_emulator.c | 3 +--
>   hw/i2c/core.c               | 2 +-
>   hw/input/adb.c              | 2 +-
>   hw/input/ads7846.c          | 2 +-
>   hw/input/stellaris_input.c  | 3 +--
>   hw/net/eepro100.c           | 3 +--
>   hw/pci/pci.c                | 2 +-
>   hw/ppc/spapr_nvdimm.c       | 3 +--
>   hw/timer/arm_timer.c        | 2 +-
>   hw/virtio/virtio-mem.c      | 4 ++--
>   11 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/backends/dbus-vmstate.c b/backends/dbus-vmstate.c
> index 57369ec0f2..a9d8cb0acd 100644
> --- a/backends/dbus-vmstate.c
> +++ b/backends/dbus-vmstate.c
> @@ -426,8 +426,7 @@ dbus_vmstate_complete(UserCreatable *uc, Error **errp)
>           return;
>       }
>
> -    if (vmstate_register(VMSTATE_IF(self), VMSTATE_INSTANCE_ID_ANY,
> -                         &dbus_vmstate, self) < 0) {
> +    if (vmstate_register_any(VMSTATE_IF(self), &dbus_vmstate, self) < 0) {
>           error_setg(errp, "Failed to register vmstate");
>       }
>   }
> diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
> index 402a2d6312..8920b75251 100644
> --- a/backends/tpm/tpm_emulator.c
> +++ b/backends/tpm/tpm_emulator.c
> @@ -978,8 +978,7 @@ static void tpm_emulator_inst_init(Object *obj)
>           qemu_add_vm_change_state_handler(tpm_emulator_vm_state_change,
>                                            tpm_emu);
>
> -    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
> -                     &vmstate_tpm_emulator, obj);
> +    vmstate_register_any(NULL, &vmstate_tpm_emulator, obj);
>   }
>
>   /*
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index bed594fe59..879a1d45cb 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -64,7 +64,7 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
>       bus = I2C_BUS(qbus_new(TYPE_I2C_BUS, parent, name));
>       QLIST_INIT(&bus->current_devs);
>       QSIMPLEQ_INIT(&bus->pending_masters);
> -    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_i2c_bus, bus);
> +    vmstate_register_any(NULL, &vmstate_i2c_bus, bus);
>       return bus;
>   }
>
> diff --git a/hw/input/adb.c b/hw/input/adb.c
> index 214ae6f42b..8aed0da2cd 100644
> --- a/hw/input/adb.c
> +++ b/hw/input/adb.c
> @@ -247,7 +247,7 @@ static void adb_bus_realize(BusState *qbus, Error **errp)
>       adb_bus->autopoll_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, adb_autopoll,
>                                              adb_bus);
>
> -    vmstate_register(NULL, -1, &vmstate_adb_bus, adb_bus);
> +    vmstate_register_any(NULL, &vmstate_adb_bus, adb_bus);
>   }
>
>   static void adb_bus_unrealize(BusState *qbus)
> diff --git a/hw/input/ads7846.c b/hw/input/ads7846.c
> index dc0998ac79..91116c6bdb 100644
> --- a/hw/input/ads7846.c
> +++ b/hw/input/ads7846.c
> @@ -158,7 +158,7 @@ static void ads7846_realize(SSIPeripheral *d, Error **errp)
>
>       ads7846_int_update(s);
>
> -    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_ads7846, s);
> +    vmstate_register_any(NULL, &vmstate_ads7846, s);
>   }
>
>   static void ads7846_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/input/stellaris_input.c b/hw/input/stellaris_input.c
> index e6ee5e11f1..a58721c8cd 100644
> --- a/hw/input/stellaris_input.c
> +++ b/hw/input/stellaris_input.c
> @@ -88,6 +88,5 @@ void stellaris_gamepad_init(int n, qemu_irq *irq, const int *keycode)
>       }
>       s->num_buttons = n;
>       qemu_add_kbd_event_handler(stellaris_gamepad_put_key, s);
> -    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
> -                     &vmstate_stellaris_gamepad, s);
> +    vmstate_register_any(NULL, &vmstate_stellaris_gamepad, s);
>   }
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index dc07984ae9..94ce9e18ff 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -1883,8 +1883,7 @@ static void e100_nic_realize(PCIDevice *pci_dev, Error **errp)
>
>       s->vmstate = g_memdup(&vmstate_eepro100, sizeof(vmstate_eepro100));
>       s->vmstate->name = qemu_get_queue(s->nic)->model;
> -    vmstate_register(VMSTATE_IF(&pci_dev->qdev), VMSTATE_INSTANCE_ID_ANY,
> -                     s->vmstate, s);
> +    vmstate_register_any(VMSTATE_IF(&pci_dev->qdev), s->vmstate, s);
>   }
>
>   static void eepro100_instance_init(Object *obj)
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index b0d21bf43a..294c3c38ea 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -147,7 +147,7 @@ static void pci_bus_realize(BusState *qbus, Error **errp)
>       bus->machine_done.notify = pcibus_machine_done;
>       qemu_add_machine_init_done_notifier(&bus->machine_done);
>
> -    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_pcibus, bus);
> +    vmstate_register_any(NULL, &vmstate_pcibus, bus);
>   }
>
>   static void pcie_bus_realize(BusState *qbus, Error **errp)
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index b2f009c816..ad7afe7544 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -876,8 +876,7 @@ static void spapr_nvdimm_realize(NVDIMMDevice *dimm, Error **errp)
>           s_nvdimm->hcall_flush_required = true;
>       }
>
> -    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY,
> -                     &vmstate_spapr_nvdimm_states, dimm);
> +    vmstate_register_any(NULL, &vmstate_spapr_nvdimm_states, dimm);
>   }
>
>   static void spapr_nvdimm_unrealize(NVDIMMDevice *dimm)
> diff --git a/hw/timer/arm_timer.c b/hw/timer/arm_timer.c
> index 69c8863472..9afe8da831 100644
> --- a/hw/timer/arm_timer.c
> +++ b/hw/timer/arm_timer.c
> @@ -181,7 +181,7 @@ static arm_timer_state *arm_timer_init(uint32_t freq)
>       s->control = TIMER_CTRL_IE;
>
>       s->timer = ptimer_init(arm_timer_tick, s, PTIMER_POLICY_LEGACY);
> -    vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_arm_timer, s);
> +    vmstate_register_any(NULL, &vmstate_arm_timer, s);
>       return s;
>   }
>
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index 9dc3c61b5a..a5ea3be414 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -1119,8 +1119,8 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
>       host_memory_backend_set_mapped(vmem->memdev, true);
>       vmstate_register_ram(&vmem->memdev->mr, DEVICE(vmem));
>       if (vmem->early_migration) {
> -        vmstate_register(VMSTATE_IF(vmem), VMSTATE_INSTANCE_ID_ANY,
> -                         &vmstate_virtio_mem_device_early, vmem);
> +        vmstate_register_any(VMSTATE_IF(vmem),
> +                             &vmstate_virtio_mem_device_early, vmem);
>       }
>       qemu_register_reset(virtio_mem_system_reset, vmem);
>

Re: [PATCH 03/13] migration: Use vmstate_register_any() for isa-ide

[Stefan Berger]

On 10/19/23 15:08, Juan Quintela wrote:
> Otherwise qom-test fails.
>
> ok 4 /i386/qom/x-remote
> qemu-system-i386: savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=isa-ide, instance_id=0x0
> Broken pipe
> ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
> Aborted (core dumped)
> $
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   hw/ide/isa.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ide/isa.c b/hw/ide/isa.c
> index 95053e026f..ea60c08116 100644
> --- a/hw/ide/isa.c
> +++ b/hw/ide/isa.c
> @@ -73,7 +73,7 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
>       ide_bus_init(&s->bus, sizeof(s->bus), dev, 0, 2);
>       ide_init_ioport(&s->bus, isadev, s->iobase, s->iobase2);
>       ide_bus_init_output_irq(&s->bus, isa_get_irq(isadev, s->irqnum));
> -    vmstate_register(VMSTATE_IF(dev), 0, &vmstate_ide_isa, s);
> +    vmstate_register_any(VMSTATE_IF(dev), &vmstate_ide_isa, s);
>       ide_bus_register_restart_cb(&s->bus);
>   }
>

Re: [PATCH 04/13] migration: Use vmstate_register_any() for ipmi-bt*

[Stefan Berger]

On 10/19/23 15:08, Juan Quintela wrote:
> Otherwise device-introspection-test fails.
>
> $ ./tests/qtest/device-introspect-test
> ...
> Broken pipe
> ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
> Aborted (core dumped)
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   hw/ipmi/ipmi_bmc_extern.c | 2 +-
>   hw/ipmi/ipmi_bmc_sim.c    | 2 +-
>   hw/ipmi/isa_ipmi_bt.c     | 2 +-
>   hw/ipmi/isa_ipmi_kcs.c    | 2 +-
>   4 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/ipmi/ipmi_bmc_extern.c b/hw/ipmi/ipmi_bmc_extern.c
> index e232d35ba2..324a2c8835 100644
> --- a/hw/ipmi/ipmi_bmc_extern.c
> +++ b/hw/ipmi/ipmi_bmc_extern.c
> @@ -504,7 +504,7 @@ static void ipmi_bmc_extern_init(Object *obj)
>       IPMIBmcExtern *ibe = IPMI_BMC_EXTERN(obj);
>
>       ibe->extern_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, extern_timeout, ibe);
> -    vmstate_register(NULL, 0, &vmstate_ipmi_bmc_extern, ibe);
> +    vmstate_register_any(NULL, &vmstate_ipmi_bmc_extern, ibe);
>   }
>
>   static void ipmi_bmc_extern_finalize(Object *obj)
> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c
> index 905e091094..404db5d5bc 100644
> --- a/hw/ipmi/ipmi_bmc_sim.c
> +++ b/hw/ipmi/ipmi_bmc_sim.c
> @@ -2188,7 +2188,7 @@ static void ipmi_sim_realize(DeviceState *dev, Error **errp)
>
>       ibs->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, ipmi_timeout, ibs);
>
> -    vmstate_register(NULL, 0, &vmstate_ipmi_sim, ibs);
> +    vmstate_register_any(NULL, &vmstate_ipmi_sim, ibs);
>   }
>
>   static Property ipmi_sim_properties[] = {
> diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
> index a83e7243d6..afb76b548a 100644
> --- a/hw/ipmi/isa_ipmi_bt.c
> +++ b/hw/ipmi/isa_ipmi_bt.c
> @@ -125,7 +125,7 @@ static void isa_ipmi_bt_init(Object *obj)
>
>       ipmi_bmc_find_and_link(obj, (Object **) &iib->bt.bmc);
>
> -    vmstate_register(NULL, 0, &vmstate_ISAIPMIBTDevice, iib);
> +    vmstate_register_any(NULL, &vmstate_ISAIPMIBTDevice, iib);
>   }
>
>   static void *isa_ipmi_bt_get_backend_data(IPMIInterface *ii)
> diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
> index b2ed70b9da..5ab63b2fcf 100644
> --- a/hw/ipmi/isa_ipmi_kcs.c
> +++ b/hw/ipmi/isa_ipmi_kcs.c
> @@ -132,7 +132,7 @@ static void isa_ipmi_kcs_init(Object *obj)
>        * IPMI device, so receive it, but transmit a different
>        * version.
>        */
> -    vmstate_register(NULL, 0, &vmstate_ISAIPMIKCSDevice, iik);
> +    vmstate_register_any(NULL, &vmstate_ISAIPMIKCSDevice, iik);
>   }
>
>   static void *isa_ipmi_kcs_get_backend_data(IPMIInterface *ii)

Re: [PATCH 05/13] migration: Use VMSTATE_INSTANCE_ID_ANY for slirp

[Stefan Berger]

On 10/19/23 15:08, Juan Quintela wrote:
> Each user network conection create a new slirp instance.  We register
> more than one slirp instance for number 0.
>
> qemu-system-x86_64: -netdev user,id=hs1: savevm_state_handler_insert: Detected duplicate SaveStateEntry: id=slirp, instance_id=0x0
> Broken pipe
> ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
> Aborted (core dumped)
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   net/slirp.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/slirp.c b/net/slirp.c
> index c33b3e02e7..25b49c4526 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -46,6 +46,7 @@
>   #include "qapi/qmp/qdict.h"
>   #include "util.h"
>   #include "migration/register.h"
> +#include "migration/vmstate.h"
>   #include "migration/qemu-file-types.h"
>
>   static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
> @@ -659,8 +660,8 @@ static int net_slirp_init(NetClientState *peer, const char *model,
>        * specific version?
>        */
>       g_assert(slirp_state_version() == 4);
> -    register_savevm_live("slirp", 0, slirp_state_version(),
> -                         &savevm_slirp_state, s->slirp);
> +    register_savevm_live("slirp", VMSTATE_INSTANCE_ID_ANY,
> +                         slirp_state_version(), &savevm_slirp_state, s->slirp);
>
>       s->poll_notifier.notify = net_slirp_poll_notify;
>       main_loop_poll_add_notifier(&s->poll_notifier);

Re: [PATCH 06/13] migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices

[Stefan Berger]

On 10/19/23 15:08, Juan Quintela wrote:
> Just with make check I can see that we can have more than one of this
> devices, so use ANY.
>
> ok 5 /s390x/device/introspect/abstract-interfaces
> ...
> Broken pipe
> ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:195: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
> Aborted (core dumped)
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   hw/s390x/s390-skeys.c    | 3 ++-
>   hw/s390x/s390-stattrib.c | 3 ++-
>   2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index 5024faf411..ef089e1967 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -22,6 +22,7 @@
>   #include "sysemu/kvm.h"
>   #include "migration/qemu-file-types.h"
>   #include "migration/register.h"
> +#include "migration/vmstate.h"
>
>   #define S390_SKEYS_BUFFER_SIZE (128 * KiB)  /* Room for 128k storage keys */
>   #define S390_SKEYS_SAVE_FLAG_EOS 0x01
> @@ -457,7 +458,7 @@ static inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
>       ss->migration_enabled = value;
>
>       if (ss->migration_enabled) {
> -        register_savevm_live(TYPE_S390_SKEYS, 0, 1,
> +        register_savevm_live(TYPE_S390_SKEYS, VMSTATE_INSTANCE_ID_ANY, 1,
>                                &savevm_s390_storage_keys, ss);
>       } else {
>           unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
> index 220e845d12..055d382c3c 100644
> --- a/hw/s390x/s390-stattrib.c
> +++ b/hw/s390x/s390-stattrib.c
> @@ -13,6 +13,7 @@
>   #include "qemu/units.h"
>   #include "migration/qemu-file.h"
>   #include "migration/register.h"
> +#include "migration/vmstate.h"
>   #include "hw/s390x/storage-attributes.h"
>   #include "qemu/error-report.h"
>   #include "exec/ram_addr.h"
> @@ -380,7 +381,7 @@ static void s390_stattrib_instance_init(Object *obj)
>   {
>       S390StAttribState *sas = S390_STATTRIB(obj);
>
> -    register_savevm_live(TYPE_S390_STATTRIB, 0, 0,
> +    register_savevm_live(TYPE_S390_STATTRIB, VMSTATE_INSTANCE_ID_ANY, 0,
>                            &savevm_s390_stattrib_handlers, sas);
>
>       object_property_add_bool(obj, "migration-enabled",

Re: [PATCH 10/13] migration: Improve example and documentation of vmstate_register()

[Stefan Berger]

On 10/19/23 15:08, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>   docs/devel/migration.rst | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
> index c3e1400c0c..a9fde75862 100644
> --- a/docs/devel/migration.rst
> +++ b/docs/devel/migration.rst
> @@ -165,13 +165,17 @@ An example (from hw/input/pckbd.c)
>         }
>     };
>
> -We are declaring the state with name "pckbd".
> -The ``version_id`` is 3, and the fields are 4 uint8_t in a KBDState structure.
> -We registered this with:
> +We are declaring the state with name "pckbd".  The ``version_id`` is
> +3, and the fields are 4 uint8_t in a KBDState structure.  We

and there are 4  uint8_t fields in the KBDState structure.


> +registered this with one of those.  The first one will generate a

I am not sure what this means 'We registered this with one of those'. 
What is 'one of those'?

Maybe you mean: We register the KBDState with one of the following 
functions.

> +device ``instance_id`` different for each registration.  Use the
> +second one if you already have an id different for each instance of
> +the device:
... have an id that is is different for each ...
>
>   .. code:: c
>
> -    vmstate_register(NULL, 0, &vmstate_kbd, s);
> +    vmstate_register_any(NULL, &vmstate_kbd, s);
> +    vmstate_register(NULL, instance_id, &vmstate_kbd, s);
>
>   For devices that are ``qdev`` based, we can register the device in the class
>   init function:

Re: [PATCH 11/13] migration: Use vmstate_register_any() for audio

[Stefan Berger]

On 10/19/23 15:08, Juan Quintela wrote:
> We can have more than one audio card.
>
> void audio_init_audiodevs(void)
> {
>      AudiodevListEntry *e;
>
>      QSIMPLEQ_FOREACH(e, &audiodevs, next) {
>          audio_init(e->dev, &error_fatal);
>      }
> }
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>


> ---
>   audio/audio.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index e9815d6812..f91e05b72c 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1781,7 +1781,7 @@ static AudioState *audio_init(Audiodev *dev, Error **errp)
>
>       QTAILQ_INSERT_TAIL(&audio_states, s, list);
>       QLIST_INIT (&s->card_head);
> -    vmstate_register (NULL, 0, &vmstate_audio, s);
> +    vmstate_register_any(NULL, &vmstate_audio, s);
>       return s;
>
>   out:

Re: [PATCH 12/13] migration: Use vmstate_register_any() for eeprom93xx

[Stefan Berger]

On 10/19/23 15:08, Juan Quintela wrote:
> We can have more than one eeprom93xx.
> For instance:
>
> e100_nic_realize() -> eeprom93xx_new()
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   hw/nvram/eeprom93xx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/nvram/eeprom93xx.c b/hw/nvram/eeprom93xx.c
> index 1081e2cc0d..57d63638d7 100644
> --- a/hw/nvram/eeprom93xx.c
> +++ b/hw/nvram/eeprom93xx.c
> @@ -321,7 +321,7 @@ eeprom_t *eeprom93xx_new(DeviceState *dev, uint16_t nwords)
>       /* Output DO is tristate, read results in 1. */
>       eeprom->eedo = 1;
>       logout("eeprom = 0x%p, nwords = %u\n", eeprom, nwords);
> -    vmstate_register(VMSTATE_IF(dev), 0, &vmstate_eeprom, eeprom);
> +    vmstate_register_any(VMSTATE_IF(dev), &vmstate_eeprom, eeprom);
>       return eeprom;
>   }
>

Re: [PATCH 13/13] migration: Use vmstate_register_any() for vmware_vga

[Stefan Berger]

On 10/19/23 15:08, Juan Quintela wrote:
> I have no idea if we can have more than one vmware_vga device, so play
> it safe.
>
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   hw/display/vmware_vga.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index 09591fbd39..7490d43881 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -1264,7 +1264,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
>
>       vga_common_init(&s->vga, OBJECT(dev), &error_fatal);
>       vga_init(&s->vga, OBJECT(dev), address_space, io, true);
> -    vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga);
> +    vmstate_register_any(NULL, &vmstate_vga_common, &s->vga);

And the first one registered with 'any' will again have instance_id = 0 
assigned. So there's no side effect to be expected with any of these 
device, I suppose.


>       s->new_depth = 32;
>   }
>

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Greg Kurz]

Hi Juan,

On Thu, 19 Oct 2023 21:08:25 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Current code does:
> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
>   dependinfg on cpu number
> - for newer machines, it register vmstate_icp with "icp/server" name
>   and instance 0
> - now it unregisters "icp/server" for the 1st instance.
> 

Heh I remember about this hack... it was caused by some rework in
the interrupt controller that broke migration.

> This is wrong at many levels:
> - we shouldn't have two VMSTATEDescriptions with the same name

I don't know how bad it is. The idea here is to send extra
state in the stream because older QEMU expect it (but won't use
it), so it made sense to keep the same name.

> - In case this is the only solution that we can came with, it needs to
>   be:
>   * register pre_2_10_vmstate_dummy_icp
>   * unregister pre_2_10_vmstate_dummy_icp
>   * register real vmstate_icp
> 
> As the initialization of this machine is already complex enough, I
> need help from PPC maintainers to fix this.
> 

What about dropping all this code, i.e. basically reverting 46f7afa37096 ("spapr:
fix migration of ICPState objects from/to older QEMU") ?

Unless we still care to migrate pseries machine types from 2017 of
course...

> Volunteers?
> 

Not working on PPC anymore since almost two years, I certainly don't have time,
nor motivation to fix this. I might be able to answer some questions or to
review someone else's patch that gets rid of the offending code, at best.

Cheers,

--
Greg


> CC: Cedric Le Goater <clg@kaod.org>
> CC: Daniel Henrique Barboza <danielhb413@gmail.com>
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: Greg Kurz <groug@kaod.org>
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/ppc/spapr.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cb840676d3..8531d13492 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
>  }
>  
>  static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> -    .name = "icp/server",
> +    /*
> +     * Hack ahead.  We can't have two devices with the same name and
> +     * instance id.  So I rename this to pass make check.
> +     * Real help from people who knows the hardware is needed.
> +     */
> +    .name = "pre-2.10-icp/server",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .needed = pre_2_10_vmstate_dummy_icp_needed,



-- 
Greg

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Cédric Le Goater]

On 10/19/23 22:49, Greg Kurz wrote:
> Hi Juan,
> 
> On Thu, 19 Oct 2023 21:08:25 +0200
> Juan Quintela <quintela@redhat.com> wrote:
> 
>> Current code does:
>> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
>>    dependinfg on cpu number
>> - for newer machines, it register vmstate_icp with "icp/server" name
>>    and instance 0
>> - now it unregisters "icp/server" for the 1st instance.
>>
> 
> Heh I remember about this hack... it was caused by some rework in
> the interrupt controller that broke migration.
> 
>> This is wrong at many levels:
>> - we shouldn't have two VMSTATEDescriptions with the same name
> 
> I don't know how bad it is. The idea here is to send extra
> state in the stream because older QEMU expect it (but won't use
> it), so it made sense to keep the same name.
> 
>> - In case this is the only solution that we can came with, it needs to
>>    be:
>>    * register pre_2_10_vmstate_dummy_icp
>>    * unregister pre_2_10_vmstate_dummy_icp
>>    * register real vmstate_icp
>>
>> As the initialization of this machine is already complex enough, I
>> need help from PPC maintainers to fix this.
>>
> 
> What about dropping all this code, i.e. basically reverting 46f7afa37096 ("spapr:
> fix migration of ICPState objects from/to older QEMU") ?

I'd vote for removing the dummy ICP states for pre-2.10 pseries machines.
Migration compatibility would be broken for these old versions but, with
a clear error report, it should be more than enough. I doubt anyone will
need such a feature now days.

C.


> Unless we still care to migrate pseries machine types from 2017 of
> course...
> 
>> Volunteers?
>>
> 
> Not working on PPC anymore since almost two years, I certainly don't have time,
> nor motivation to fix this. I might be able to answer some questions or to
> review someone else's patch that gets rid of the offending code, at best.
> 
> Cheers,
> 
> --
> Greg
> 
> 
>> CC: Cedric Le Goater <clg@kaod.org>
>> CC: Daniel Henrique Barboza <danielhb413@gmail.com>
>> CC: David Gibson <david@gibson.dropbear.id.au>
>> CC: Greg Kurz <groug@kaod.org>
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>   hw/ppc/spapr.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index cb840676d3..8531d13492 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
>>   }
>>   
>>   static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
>> -    .name = "icp/server",
>> +    /*
>> +     * Hack ahead.  We can't have two devices with the same name and
>> +     * instance id.  So I rename this to pass make check.
>> +     * Real help from people who knows the hardware is needed.
>> +     */
>> +    .name = "pre-2.10-icp/server",
>>       .version_id = 1,
>>       .minimum_version_id = 1,
>>       .needed = pre_2_10_vmstate_dummy_icp_needed,
> 
> 
> 

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Greg Kurz]

On Thu, 19 Oct 2023 21:08:25 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Current code does:
> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
>   dependinfg on cpu number
> - for newer machines, it register vmstate_icp with "icp/server" name
>   and instance 0
> - now it unregisters "icp/server" for the 1st instance.
> 
> This is wrong at many levels:
> - we shouldn't have two VMSTATEDescriptions with the same name
> - In case this is the only solution that we can came with, it needs to
>   be:
>   * register pre_2_10_vmstate_dummy_icp
>   * unregister pre_2_10_vmstate_dummy_icp
>   * register real vmstate_icp
> 
> As the initialization of this machine is already complex enough, I
> need help from PPC maintainers to fix this.
> 
> Volunteers?
> 
> CC: Cedric Le Goater <clg@kaod.org>
> CC: Daniel Henrique Barboza <danielhb413@gmail.com>
> CC: David Gibson <david@gibson.dropbear.id.au>
> CC: Greg Kurz <groug@kaod.org>
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/ppc/spapr.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cb840676d3..8531d13492 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
>  }
>  
>  static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> -    .name = "icp/server",
> +    /*
> +     * Hack ahead.  We can't have two devices with the same name and
> +     * instance id.  So I rename this to pass make check.
> +     * Real help from people who knows the hardware is needed.
> +     */
> +    .name = "pre-2.10-icp/server",
>      .version_id = 1,
>      .minimum_version_id = 1,
>      .needed = pre_2_10_vmstate_dummy_icp_needed,

I guess this fix is acceptable as well and a lot simpler than
reverting the hack actually. Outcome is the same : drop
compat with pseries-2.9 and older.

Reviewed-by: Greg Kurz <groug@kaod.org>

-- 
Greg

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Thomas Huth]

On 19/10/2023 23.15, Cédric Le Goater wrote:
> On 10/19/23 22:49, Greg Kurz wrote:
>> Hi Juan,
>>
>> On Thu, 19 Oct 2023 21:08:25 +0200
>> Juan Quintela <quintela@redhat.com> wrote:
>>
>>> Current code does:
>>> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
>>>    dependinfg on cpu number
>>> - for newer machines, it register vmstate_icp with "icp/server" name
>>>    and instance 0
>>> - now it unregisters "icp/server" for the 1st instance.
>>>
>>
>> Heh I remember about this hack... it was caused by some rework in
>> the interrupt controller that broke migration.
>>
>>> This is wrong at many levels:
>>> - we shouldn't have two VMSTATEDescriptions with the same name
>>
>> I don't know how bad it is. The idea here is to send extra
>> state in the stream because older QEMU expect it (but won't use
>> it), so it made sense to keep the same name.
>>
>>> - In case this is the only solution that we can came with, it needs to
>>>    be:
>>>    * register pre_2_10_vmstate_dummy_icp
>>>    * unregister pre_2_10_vmstate_dummy_icp
>>>    * register real vmstate_icp
>>>
>>> As the initialization of this machine is already complex enough, I
>>> need help from PPC maintainers to fix this.
>>>
>>
>> What about dropping all this code, i.e. basically reverting 46f7afa37096 
>> ("spapr:
>> fix migration of ICPState objects from/to older QEMU") ?
> 
> I'd vote for removing the dummy ICP states for pre-2.10 pseries machines.
> Migration compatibility would be broken for these old versions but, with
> a clear error report, it should be more than enough. I doubt anyone will
> need such a feature now days.

In that case: Please also put the pseries-2.1 machine up to pseries-2.9 onto 
the deprecation list, so that they can finally get removed after two 
releases. It does not make sense to keep compat machines around if the 
compatibility is not available anymore.

  Thomas

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Juan Quintela]

Greg Kurz <groug@kaod.org> wrote:
> On Thu, 19 Oct 2023 21:08:25 +0200
> Juan Quintela <quintela@redhat.com> wrote:
>
>> Current code does:
>> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
>>   dependinfg on cpu number
>> - for newer machines, it register vmstate_icp with "icp/server" name
>>   and instance 0
>> - now it unregisters "icp/server" for the 1st instance.
>> 
>> This is wrong at many levels:
>> - we shouldn't have two VMSTATEDescriptions with the same name
>> - In case this is the only solution that we can came with, it needs to
>>   be:
>>   * register pre_2_10_vmstate_dummy_icp
>>   * unregister pre_2_10_vmstate_dummy_icp
>>   * register real vmstate_icp
>> 
>> As the initialization of this machine is already complex enough, I
>> need help from PPC maintainers to fix this.
>> 
>> Volunteers?
>> 
>> CC: Cedric Le Goater <clg@kaod.org>
>> CC: Daniel Henrique Barboza <danielhb413@gmail.com>
>> CC: David Gibson <david@gibson.dropbear.id.au>
>> CC: Greg Kurz <groug@kaod.org>
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/ppc/spapr.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index cb840676d3..8531d13492 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
>>  }
>>  
>>  static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
>> -    .name = "icp/server",
>> +    /*
>> +     * Hack ahead.  We can't have two devices with the same name and
>> +     * instance id.  So I rename this to pass make check.
>> +     * Real help from people who knows the hardware is needed.
>> +     */
>> +    .name = "pre-2.10-icp/server",
>>      .version_id = 1,
>>      .minimum_version_id = 1,
>>      .needed = pre_2_10_vmstate_dummy_icp_needed,
>
> I guess this fix is acceptable as well and a lot simpler than
> reverting the hack actually. Outcome is the same : drop
> compat with pseries-2.9 and older.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>

I fully agree with you here.
The other options given on this thread is deprecate that machines, but I
would like to have this series sooner than 2 releases.  And what ppc is
doing here is (and has always been) a hack and an abuse about how
vmstate registrations is supposed to work.

Thanks, Juan.

Re: [PATCH 13/13] migration: Use vmstate_register_any() for vmware_vga

[Juan Quintela]

Stefan Berger <stefanb@linux.ibm.com> wrote:
> On 10/19/23 15:08, Juan Quintela wrote:
>> I have no idea if we can have more than one vmware_vga device, so play
>> it safe.
>>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>
>> ---
>>   hw/display/vmware_vga.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
>> index 09591fbd39..7490d43881 100644
>> --- a/hw/display/vmware_vga.c
>> +++ b/hw/display/vmware_vga.c
>> @@ -1264,7 +1264,7 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
>>
>>       vga_common_init(&s->vga, OBJECT(dev), &error_fatal);
>>       vga_init(&s->vga, OBJECT(dev), address_space, io, true);
>> -    vmstate_register(NULL, 0, &vmstate_vga_common, &s->vga);
>> +    vmstate_register_any(NULL, &vmstate_vga_common, &s->vga);
>
> And the first one registered with 'any' will again have instance_id =
> 0 assigned. So there's no side effect to be expected with any of these
> device, I suppose.

I will really change all the remaining registrations with 0 to any.

* If there is only a registration for that device: nothing changes
* If there is more than one registration for that device: It *could*
  work (from the migration point of view).

But then there are devices that *clearly* will not be able to have more
than one instance, so 0 is the best option there.  On top of my head:

* cpu-timers
* replay
* migration global_state

And the rest that I put on the cover letter are basically devices that
are used only once on the board initilization routine, so I feel safe
leaving them with zero.

Thanks for the review, Juan.

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Cédric Le Goater]

On 10/20/23 07:10, Thomas Huth wrote:
> On 19/10/2023 23.15, Cédric Le Goater wrote:
>> On 10/19/23 22:49, Greg Kurz wrote:
>>> Hi Juan,
>>>
>>> On Thu, 19 Oct 2023 21:08:25 +0200
>>> Juan Quintela <quintela@redhat.com> wrote:
>>>
>>>> Current code does:
>>>> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
>>>>    dependinfg on cpu number
>>>> - for newer machines, it register vmstate_icp with "icp/server" name
>>>>    and instance 0
>>>> - now it unregisters "icp/server" for the 1st instance.
>>>>
>>>
>>> Heh I remember about this hack... it was caused by some rework in
>>> the interrupt controller that broke migration.
>>>
>>>> This is wrong at many levels:
>>>> - we shouldn't have two VMSTATEDescriptions with the same name
>>>
>>> I don't know how bad it is. The idea here is to send extra
>>> state in the stream because older QEMU expect it (but won't use
>>> it), so it made sense to keep the same name.
>>>
>>>> - In case this is the only solution that we can came with, it needs to
>>>>    be:
>>>>    * register pre_2_10_vmstate_dummy_icp
>>>>    * unregister pre_2_10_vmstate_dummy_icp
>>>>    * register real vmstate_icp
>>>>
>>>> As the initialization of this machine is already complex enough, I
>>>> need help from PPC maintainers to fix this.
>>>>
>>>
>>> What about dropping all this code, i.e. basically reverting 46f7afa37096 ("spapr:
>>> fix migration of ICPState objects from/to older QEMU") ?
>>
>> I'd vote for removing the dummy ICP states for pre-2.10 pseries machines.
>> Migration compatibility would be broken for these old versions but, with
>> a clear error report, it should be more than enough. I doubt anyone will
>> need such a feature now days.
> 
> In that case: Please also put the pseries-2.1 machine up to pseries-2.9 onto the deprecation list, so that they can finally get removed after two releases. It does not make sense to keep compat machines around if the compatibility is not available anymore.

This would be a really good cleanup for PPC to deprecate pseries-2.1/2.9.
We did a few workarounds in that time frame which wouldn't be necessary
anymore.

Thanks,

C.

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Nicholas Piggin]

On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz wrote:
> On Thu, 19 Oct 2023 21:08:25 +0200
> Juan Quintela <quintela@redhat.com> wrote:
>
> > Current code does:
> > - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> >   dependinfg on cpu number
> > - for newer machines, it register vmstate_icp with "icp/server" name
> >   and instance 0
> > - now it unregisters "icp/server" for the 1st instance.
> > 
> > This is wrong at many levels:
> > - we shouldn't have two VMSTATEDescriptions with the same name
> > - In case this is the only solution that we can came with, it needs to
> >   be:
> >   * register pre_2_10_vmstate_dummy_icp
> >   * unregister pre_2_10_vmstate_dummy_icp
> >   * register real vmstate_icp
> > 
> > As the initialization of this machine is already complex enough, I
> > need help from PPC maintainers to fix this.
> > 
> > Volunteers?
> > 
> > CC: Cedric Le Goater <clg@kaod.org>
> > CC: Daniel Henrique Barboza <danielhb413@gmail.com>
> > CC: David Gibson <david@gibson.dropbear.id.au>
> > CC: Greg Kurz <groug@kaod.org>
> > 
> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> > ---
> >  hw/ppc/spapr.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index cb840676d3..8531d13492 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
> >  }
> >  
> >  static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > -    .name = "icp/server",
> > +    /*
> > +     * Hack ahead.  We can't have two devices with the same name and
> > +     * instance id.  So I rename this to pass make check.
> > +     * Real help from people who knows the hardware is needed.
> > +     */
> > +    .name = "pre-2.10-icp/server",
> >      .version_id = 1,
> >      .minimum_version_id = 1,
> >      .needed = pre_2_10_vmstate_dummy_icp_needed,
>
> I guess this fix is acceptable as well and a lot simpler than
> reverting the hack actually. Outcome is the same : drop
> compat with pseries-2.9 and older.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>

So the reason we can't have duplicate names registered, aside from it
surely going bad if we actually send or receive a stream at the point
they are registered, is the duplcate check introduced in patch 9? But
before that, this hack does seem to actually work because the duplicate
is unregistered right away.

If I understand the workaround, there is an asymmetry in the migration
sequence in that receiving an unexpected object would cause a failure,
but going from newer to older would just skip some "expected" objects
and that didn't cause a problem. So you only have to deal with ignoring
the unexpected ones going form older to newer.

Side question, is it possible to flag the problem of *not* receiving
an object that you did expect? That might be a source of bugs too.

Anyway, I wonder if we could fix this spapr problem by adding a special
case wild card instance matcher to ignore it? It's still a bit hacky
but maybe a bit nicer. I don't mind deprecating the machine soon if
you want to clear the wildcard hack away soon, but it would be nice to
separate the deprecation and removal from the fix, if possible.

This patch is not tested but hopefully helps illustrate the idea.

Thanks,
Nick

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1a31fb7293..8ce03edefa 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1205,6 +1205,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
 bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
 
 #define  VMSTATE_INSTANCE_ID_ANY  -1
+#define  VMSTATE_INSTANCE_ID_WILD -2
 
 /* Returns: 0 on success, -1 on failure */
 int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cb840676d3..2418899dd4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -155,16 +155,10 @@ static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
     },
 };
 
-static void pre_2_10_vmstate_register_dummy_icp(int i)
+static void pre_2_10_vmstate_register_dummy_icp(void)
 {
-    vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp,
-                     (void *)(uintptr_t) i);
-}
-
-static void pre_2_10_vmstate_unregister_dummy_icp(int i)
-{
-    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
-                       (void *)(uintptr_t) i);
+    vmstate_register(NULL, VMSTATE_INSTANCE_ID_WILD,
+                     &pre_2_10_vmstate_dummy_icp, NULL);
 }
 
 int spapr_max_server_number(SpaprMachineState *spapr)
@@ -2665,12 +2659,10 @@ static void spapr_init_cpus(SpaprMachineState *spapr)
     }
 
     if (smc->pre_2_10_has_unused_icps) {
-        for (i = 0; i < spapr_max_server_number(spapr); i++) {
-            /* Dummy entries get deregistered when real ICPState objects
-             * are registered during CPU core hotplug.
-             */
-            pre_2_10_vmstate_register_dummy_icp(i);
-        }
+        /* Dummy entries get deregistered when real ICPState objects
+         * are registered during CPU core hotplug.
+         */
+        pre_2_10_vmstate_register_dummy_icp();
     }
 
     for (i = 0; i < possible_cpus->len; i++) {
@@ -3873,21 +3865,9 @@ void spapr_core_release(DeviceState *dev)
 static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
 {
     MachineState *ms = MACHINE(hotplug_dev);
-    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     CPUCore *cc = CPU_CORE(dev);
     CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
 
-    if (smc->pre_2_10_has_unused_icps) {
-        SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
-        int i;
-
-        for (i = 0; i < cc->nr_threads; i++) {
-            CPUState *cs = CPU(sc->threads[i]);
-
-            pre_2_10_vmstate_register_dummy_icp(cs->cpu_index);
-        }
-    }
-
     assert(core_slot);
     core_slot->cpu = NULL;
     qdev_unrealize(dev);
@@ -3968,10 +3948,8 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
     MachineClass *mc = MACHINE_GET_CLASS(spapr);
-    SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
     SpaprCpuCore *core = SPAPR_CPU_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(dev);
-    CPUState *cs;
     SpaprDrc *drc;
     CPUArchId *core_slot;
     int index;
@@ -4018,13 +3996,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
                            &error_abort);
         }
     }
-
-    if (smc->pre_2_10_has_unused_icps) {
-        for (i = 0; i < cc->nr_threads; i++) {
-            cs = CPU(core->threads[i]);
-            pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
-        }
-    }
 }
 
 static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
diff --git a/migration/savevm.c b/migration/savevm.c
index 497ce02bd7..f33449e208 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -989,6 +989,10 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc)
         trace_savevm_section_skip(se->idstr, se->section_id);
         return 0;
     }
+    if (se->instance_id == VMSTATE_INSTANCE_ID_WILD) {
+        warn_report("Wildcard vmstate entry must set needed=false");
+        return 0;
+    }
 
     trace_savevm_section_start(se->idstr, se->section_id);
     save_section_header(f, se, QEMU_VM_SECTION_FULL);
@@ -1731,13 +1735,16 @@ int qemu_save_device_state(QEMUFile *f)
 
 static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id)
 {
+    SaveStateEntry *se_wild = NULL;
     SaveStateEntry *se;
 
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
-        if (!strcmp(se->idstr, idstr) &&
-            (instance_id == se->instance_id ||
-             instance_id == se->alias_id))
-            return se;
+        if (!strcmp(se->idstr, idstr)) {
+            if (instance_id == se->instance_id || instance_id == se->alias_id)
+                return se;
+            if (se->instance_id == VMSTATE_INSTANCE_ID_WILD)
+                se_wild = se;
+        }
         /* Migrating from an older version? */
         if (strstr(se->idstr, idstr) && se->compat) {
             if (!strcmp(se->compat->idstr, idstr) &&
@@ -1746,7 +1753,7 @@ static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id)
                 return se;
         }
     }
-    return NULL;
+    return se_wild;
 }
 
 enum LoadVMExitCodes {

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Greg Kurz]

On Fri, 20 Oct 2023 09:30:44 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Greg Kurz <groug@kaod.org> wrote:
> > On Thu, 19 Oct 2023 21:08:25 +0200
> > Juan Quintela <quintela@redhat.com> wrote:
> >
> >> Current code does:
> >> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> >>   dependinfg on cpu number
> >> - for newer machines, it register vmstate_icp with "icp/server" name
> >>   and instance 0
> >> - now it unregisters "icp/server" for the 1st instance.
> >> 
> >> This is wrong at many levels:
> >> - we shouldn't have two VMSTATEDescriptions with the same name
> >> - In case this is the only solution that we can came with, it needs to
> >>   be:
> >>   * register pre_2_10_vmstate_dummy_icp
> >>   * unregister pre_2_10_vmstate_dummy_icp
> >>   * register real vmstate_icp
> >> 
> >> As the initialization of this machine is already complex enough, I
> >> need help from PPC maintainers to fix this.
> >> 
> >> Volunteers?
> >> 
> >> CC: Cedric Le Goater <clg@kaod.org>
> >> CC: Daniel Henrique Barboza <danielhb413@gmail.com>
> >> CC: David Gibson <david@gibson.dropbear.id.au>
> >> CC: Greg Kurz <groug@kaod.org>
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  hw/ppc/spapr.c | 7 ++++++-
> >>  1 file changed, 6 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index cb840676d3..8531d13492 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
> >>  }
> >>  
> >>  static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> >> -    .name = "icp/server",
> >> +    /*
> >> +     * Hack ahead.  We can't have two devices with the same name and
> >> +     * instance id.  So I rename this to pass make check.
> >> +     * Real help from people who knows the hardware is needed.
> >> +     */
> >> +    .name = "pre-2.10-icp/server",
> >>      .version_id = 1,
> >>      .minimum_version_id = 1,
> >>      .needed = pre_2_10_vmstate_dummy_icp_needed,
> >
> > I guess this fix is acceptable as well and a lot simpler than
> > reverting the hack actually. Outcome is the same : drop
> > compat with pseries-2.9 and older.
> >
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> I fully agree with you here.
> The other options given on this thread is deprecate that machines, but I
> would like to have this series sooner than 2 releases.

Yeah and, especially, the deprecation of all these machine types is
itself a massive chunk of work as it will call to identify and
remove other related workarounds as well. Given that pretty much
everyone working in PPC/PAPR moved away, can the community handle
such a big change ?

>  And what ppc is
> doing here is (and has always been) a hack and an abuse about how
> vmstate registrations is supposed to work.
> 

Sorry again... We should have involved migration experts at the time. :-)

> Thanks, Juan.
> 

Cheers,

-- 
Greg

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Thomas Huth]

On 20/10/2023 10.06, Greg Kurz wrote:
> On Fri, 20 Oct 2023 09:30:44 +0200
> Juan Quintela <quintela@redhat.com> wrote:
> 
>> Greg Kurz <groug@kaod.org> wrote:
>>> On Thu, 19 Oct 2023 21:08:25 +0200
>>> Juan Quintela <quintela@redhat.com> wrote:
>>>
>>>> Current code does:
>>>> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
>>>>    dependinfg on cpu number
>>>> - for newer machines, it register vmstate_icp with "icp/server" name
>>>>    and instance 0
>>>> - now it unregisters "icp/server" for the 1st instance.
>>>>
>>>> This is wrong at many levels:
>>>> - we shouldn't have two VMSTATEDescriptions with the same name
>>>> - In case this is the only solution that we can came with, it needs to
>>>>    be:
>>>>    * register pre_2_10_vmstate_dummy_icp
>>>>    * unregister pre_2_10_vmstate_dummy_icp
>>>>    * register real vmstate_icp
>>>>
>>>> As the initialization of this machine is already complex enough, I
>>>> need help from PPC maintainers to fix this.
>>>>
>>>> Volunteers?
>>>>
>>>> CC: Cedric Le Goater <clg@kaod.org>
>>>> CC: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> CC: David Gibson <david@gibson.dropbear.id.au>
>>>> CC: Greg Kurz <groug@kaod.org>
>>>>
>>>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>>> ---
>>>>   hw/ppc/spapr.c | 7 ++++++-
>>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index cb840676d3..8531d13492 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
>>>>   }
>>>>   
>>>>   static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
>>>> -    .name = "icp/server",
>>>> +    /*
>>>> +     * Hack ahead.  We can't have two devices with the same name and
>>>> +     * instance id.  So I rename this to pass make check.
>>>> +     * Real help from people who knows the hardware is needed.
>>>> +     */
>>>> +    .name = "pre-2.10-icp/server",
>>>>       .version_id = 1,
>>>>       .minimum_version_id = 1,
>>>>       .needed = pre_2_10_vmstate_dummy_icp_needed,
>>>
>>> I guess this fix is acceptable as well and a lot simpler than
>>> reverting the hack actually. Outcome is the same : drop
>>> compat with pseries-2.9 and older.
>>>
>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>
>> I fully agree with you here.
>> The other options given on this thread is deprecate that machines, but I
>> would like to have this series sooner than 2 releases.
> 
> Yeah and, especially, the deprecation of all these machine types is
> itself a massive chunk of work as it will call to identify and
> remove other related workarounds as well. Given that pretty much
> everyone working in PPC/PAPR moved away, can the community handle
> such a big change ?

I think you could treat that as two work items. First the deprecation and 
removal of old machine types. Second the (optional) cleanups. If we don't 
immediately manage to find and remove each and every possible spot that 
could be cleaned up after the removal of the machine types, so be it. But 
better at least *start* to remove the old cruft, beginning with the machine 
type, than dragging this stuff along forever.

  Thomas

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Juan Quintela]

"Nicholas Piggin" <npiggin@gmail.com> wrote:
> On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz wrote:
>> On Thu, 19 Oct 2023 21:08:25 +0200
>> Juan Quintela <quintela@redhat.com> wrote:


> So the reason we can't have duplicate names registered, aside from it
> surely going bad if we actually send or receive a stream at the point
> they are registered, is the duplcate check introduced in patch 9? But
> before that, this hack does seem to actually work because the duplicate
> is unregistered right away.

You are creating a new general case that has only a single use that you
agree it is "hacky" O:-)

The problem here is that you haven't made your mind what "ipc/server"
means.  You want sometimes to mean pre_2_10, sometimes to mean other
thing.  That is not how this is supposed to work.  See my proposed
change, it is one line change, and just do the right thing.

I know, it breaks backwards compatibility.  But for one machine type
that people are proposing to deprecate/remove.

> If I understand the workaround, there is an asymmetry in the migration
> sequence in that receiving an unexpected object would cause a failure,
> but going from newer to older would just skip some "expected" objects
> and that didn't cause a problem. So you only have to deal with ignoring
> the unexpected ones going form older to newer.


Ok, found a different workaround.
Sending a new version of the series with a different hack that maintains
backwards compatibility.

Later, Juan.

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Greg Kurz]

On Fri, 20 Oct 2023 17:49:38 +1000
"Nicholas Piggin" <npiggin@gmail.com> wrote:

> On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz wrote:
> > On Thu, 19 Oct 2023 21:08:25 +0200
> > Juan Quintela <quintela@redhat.com> wrote:
> >
> > > Current code does:
> > > - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> > >   dependinfg on cpu number
> > > - for newer machines, it register vmstate_icp with "icp/server" name
> > >   and instance 0
> > > - now it unregisters "icp/server" for the 1st instance.
> > > 
> > > This is wrong at many levels:
> > > - we shouldn't have two VMSTATEDescriptions with the same name
> > > - In case this is the only solution that we can came with, it needs to
> > >   be:
> > >   * register pre_2_10_vmstate_dummy_icp
> > >   * unregister pre_2_10_vmstate_dummy_icp
> > >   * register real vmstate_icp
> > > 
> > > As the initialization of this machine is already complex enough, I
> > > need help from PPC maintainers to fix this.
> > > 
> > > Volunteers?
> > > 
> > > CC: Cedric Le Goater <clg@kaod.org>
> > > CC: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > CC: David Gibson <david@gibson.dropbear.id.au>
> > > CC: Greg Kurz <groug@kaod.org>
> > > 
> > > Signed-off-by: Juan Quintela <quintela@redhat.com>
> > > ---
> > >  hw/ppc/spapr.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index cb840676d3..8531d13492 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
> > >  }
> > >  
> > >  static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > > -    .name = "icp/server",
> > > +    /*
> > > +     * Hack ahead.  We can't have two devices with the same name and
> > > +     * instance id.  So I rename this to pass make check.
> > > +     * Real help from people who knows the hardware is needed.
> > > +     */
> > > +    .name = "pre-2.10-icp/server",
> > >      .version_id = 1,
> > >      .minimum_version_id = 1,
> > >      .needed = pre_2_10_vmstate_dummy_icp_needed,
> >
> > I guess this fix is acceptable as well and a lot simpler than
> > reverting the hack actually. Outcome is the same : drop
> > compat with pseries-2.9 and older.
> >
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> So the reason we can't have duplicate names registered, aside from it
> surely going bad if we actually send or receive a stream at the point
> they are registered, is the duplcate check introduced in patch 9? But
> before that, this hack does seem to actually work because the duplicate
> is unregistered right away.
> 

Correct.

> If I understand the workaround, there is an asymmetry in the migration
> sequence in that receiving an unexpected object would cause a failure,
> but going from newer to older would just skip some "expected" objects
> and that didn't cause a problem. So you only have to deal with ignoring
> the unexpected ones going form older to newer.
> 

Correct.

> Side question, is it possible to flag the problem of *not* receiving
> an object that you did expect? That might be a source of bugs too.
> 

AFAICR we try to only migrate state that differs from reset : the
destination cannot really assume it will receive anything for a
given device.

> Anyway, I wonder if we could fix this spapr problem by adding a special
> case wild card instance matcher to ignore it? It's still a bit hacky
> but maybe a bit nicer. I don't mind deprecating the machine soon if
> you want to clear the wildcard hack away soon, but it would be nice to
> separate the deprecation and removal from the fix, if possible.
> 
> This patch is not tested but hopefully helps illustrate the idea.
> 

I'm not sure this will fly with older QEMUs that don't know about
VMSTATE_INSTANCE_ID_WILD... but I'll let Juan comment on that.

> Thanks,
> Nick
> 

Cheers,

--
Greg

> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1a31fb7293..8ce03edefa 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1205,6 +1205,7 @@ int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>  bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
>  
>  #define  VMSTATE_INSTANCE_ID_ANY  -1
> +#define  VMSTATE_INSTANCE_ID_WILD -2
>  
>  /* Returns: 0 on success, -1 on failure */
>  int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cb840676d3..2418899dd4 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -155,16 +155,10 @@ static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
>      },
>  };
>  
> -static void pre_2_10_vmstate_register_dummy_icp(int i)
> +static void pre_2_10_vmstate_register_dummy_icp(void)
>  {
> -    vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp,
> -                     (void *)(uintptr_t) i);
> -}
> -
> -static void pre_2_10_vmstate_unregister_dummy_icp(int i)
> -{
> -    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
> -                       (void *)(uintptr_t) i);
> +    vmstate_register(NULL, VMSTATE_INSTANCE_ID_WILD,
> +                     &pre_2_10_vmstate_dummy_icp, NULL);
>  }
>  
>  int spapr_max_server_number(SpaprMachineState *spapr)
> @@ -2665,12 +2659,10 @@ static void spapr_init_cpus(SpaprMachineState *spapr)
>      }
>  
>      if (smc->pre_2_10_has_unused_icps) {
> -        for (i = 0; i < spapr_max_server_number(spapr); i++) {
> -            /* Dummy entries get deregistered when real ICPState objects
> -             * are registered during CPU core hotplug.
> -             */
> -            pre_2_10_vmstate_register_dummy_icp(i);
> -        }
> +        /* Dummy entries get deregistered when real ICPState objects
> +         * are registered during CPU core hotplug.
> +         */
> +        pre_2_10_vmstate_register_dummy_icp();
>      }
>  
>      for (i = 0; i < possible_cpus->len; i++) {
> @@ -3873,21 +3865,9 @@ void spapr_core_release(DeviceState *dev)
>  static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
>  {
>      MachineState *ms = MACHINE(hotplug_dev);
> -    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
>      CPUCore *cc = CPU_CORE(dev);
>      CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
>  
> -    if (smc->pre_2_10_has_unused_icps) {
> -        SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> -        int i;
> -
> -        for (i = 0; i < cc->nr_threads; i++) {
> -            CPUState *cs = CPU(sc->threads[i]);
> -
> -            pre_2_10_vmstate_register_dummy_icp(cs->cpu_index);
> -        }
> -    }
> -
>      assert(core_slot);
>      core_slot->cpu = NULL;
>      qdev_unrealize(dev);
> @@ -3968,10 +3948,8 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
>      MachineClass *mc = MACHINE_GET_CLASS(spapr);
> -    SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
>      SpaprCpuCore *core = SPAPR_CPU_CORE(OBJECT(dev));
>      CPUCore *cc = CPU_CORE(dev);
> -    CPUState *cs;
>      SpaprDrc *drc;
>      CPUArchId *core_slot;
>      int index;
> @@ -4018,13 +3996,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev)
>                             &error_abort);
>          }
>      }
> -
> -    if (smc->pre_2_10_has_unused_icps) {
> -        for (i = 0; i < cc->nr_threads; i++) {
> -            cs = CPU(core->threads[i]);
> -            pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
> -        }
> -    }
>  }
>  
>  static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 497ce02bd7..f33449e208 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -989,6 +989,10 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, JSONWriter *vmdesc)
>          trace_savevm_section_skip(se->idstr, se->section_id);
>          return 0;
>      }
> +    if (se->instance_id == VMSTATE_INSTANCE_ID_WILD) {
> +        warn_report("Wildcard vmstate entry must set needed=false");
> +        return 0;
> +    }
>  
>      trace_savevm_section_start(se->idstr, se->section_id);
>      save_section_header(f, se, QEMU_VM_SECTION_FULL);
> @@ -1731,13 +1735,16 @@ int qemu_save_device_state(QEMUFile *f)
>  
>  static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id)
>  {
> +    SaveStateEntry *se_wild = NULL;
>      SaveStateEntry *se;
>  
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> -        if (!strcmp(se->idstr, idstr) &&
> -            (instance_id == se->instance_id ||
> -             instance_id == se->alias_id))
> -            return se;
> +        if (!strcmp(se->idstr, idstr)) {
> +            if (instance_id == se->instance_id || instance_id == se->alias_id)
> +                return se;
> +            if (se->instance_id == VMSTATE_INSTANCE_ID_WILD)
> +                se_wild = se;
> +        }
>          /* Migrating from an older version? */
>          if (strstr(se->idstr, idstr) && se->compat) {
>              if (!strcmp(se->compat->idstr, idstr) &&
> @@ -1746,7 +1753,7 @@ static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id)
>                  return se;
>          }
>      }
> -    return NULL;
> +    return se_wild;
>  }
>  
>  enum LoadVMExitCodes {



-- 
Greg

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Juan Quintela]

Greg Kurz <groug@kaod.org> wrote:
> On Fri, 20 Oct 2023 09:30:44 +0200
> Juan Quintela <quintela@redhat.com> wrote:
>
>> Greg Kurz <groug@kaod.org> wrote:
>> > On Thu, 19 Oct 2023 21:08:25 +0200
>> > Juan Quintela <quintela@redhat.com> wrote:
>> >
>> >> Current code does:
>> >> - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
>> >>   dependinfg on cpu number
>> >> - for newer machines, it register vmstate_icp with "icp/server" name
>> >>   and instance 0
>> >> - now it unregisters "icp/server" for the 1st instance.
>> >> 
>> >> This is wrong at many levels:
>> >> - we shouldn't have two VMSTATEDescriptions with the same name
>> >> - In case this is the only solution that we can came with, it needs to
>> >>   be:
>> >>   * register pre_2_10_vmstate_dummy_icp
>> >>   * unregister pre_2_10_vmstate_dummy_icp
>> >>   * register real vmstate_icp
>> >> 
>> >> As the initialization of this machine is already complex enough, I
>> >> need help from PPC maintainers to fix this.
>> >> 
>> >> Volunteers?
>> >> 
>> >> CC: Cedric Le Goater <clg@kaod.org>
>> >> CC: Daniel Henrique Barboza <danielhb413@gmail.com>
>> >> CC: David Gibson <david@gibson.dropbear.id.au>
>> >> CC: Greg Kurz <groug@kaod.org>
>> >> 
>> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> >> ---
>> >>  hw/ppc/spapr.c | 7 ++++++-
>> >>  1 file changed, 6 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> >> index cb840676d3..8531d13492 100644
>> >> --- a/hw/ppc/spapr.c
>> >> +++ b/hw/ppc/spapr.c
>> >> @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
>> >>  }
>> >>  
>> >>  static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
>> >> -    .name = "icp/server",
>> >> +    /*
>> >> +     * Hack ahead.  We can't have two devices with the same name and
>> >> +     * instance id.  So I rename this to pass make check.
>> >> +     * Real help from people who knows the hardware is needed.
>> >> +     */
>> >> +    .name = "pre-2.10-icp/server",
>> >>      .version_id = 1,
>> >>      .minimum_version_id = 1,
>> >>      .needed = pre_2_10_vmstate_dummy_icp_needed,
>> >
>> > I guess this fix is acceptable as well and a lot simpler than
>> > reverting the hack actually. Outcome is the same : drop
>> > compat with pseries-2.9 and older.
>> >
>> > Reviewed-by: Greg Kurz <groug@kaod.org>
>> 
>> I fully agree with you here.
>> The other options given on this thread is deprecate that machines, but I
>> would like to have this series sooner than 2 releases.
>
> Yeah and, especially, the deprecation of all these machine types is
> itself a massive chunk of work as it will call to identify and
> remove other related workarounds as well. Given that pretty much
> everyone working in PPC/PAPR moved away, can the community handle
> such a big change ?
>
>>  And what ppc is
>> doing here is (and has always been) a hack and an abuse about how
>> vmstate registrations is supposed to work.
>> 
>
> Sorry again... We should have involved migration experts at the time. :-)

I would have told you that this can't be done O:-)

Sent another version with a vmstate hack to accomodate this.  You don't
have to deprecate the machines due to migration O:-)

And now that I have ppc gurus attention, could you comment in the other
question:

./hw/ppc/spapr_iommu.c

static void spapr_tce_table_realize(DeviceState *dev, Error **errp)
{
    ...
    vmstate_register(VMSTATE_IF(tcet), tcet->liobn, &vmstate_spapr_tce_table,
                     tcet);
}

./include/hw/ppc/spapr.h

struct SpaprTceTable {
    ...
    uint32_t liobn;
    ....
};

./include/migration.h

static inline int vmstate_register(VMStateIf *obj, int instance_id,
                                   const VMStateDescription *vmsd,
                                   void *opaque);


liobn is an uint32_t and insntance_id is an int.

For this series, I started with this:

static inline int vmstate_register(VMStateIf *obj, int instance_id,
                                   const VMStateDescription *vmsd,
                                   void *opaque)
{
    if (instance_id < 0) {
        error_report("vmstate_register: Invalid device: %s instance_id: %d",
                     vmsd->name, instance_id);
        return -1;
    }
    return vmstate_register_with_alias_id(obj, instance_id, vmsd,
                                          opaque, -1, 0, NULL);
}

And it failed on this.  So I change the test to

    if (instance_id == VM_INSTANCE_ID_ANY) {
       ....
    }

But we are still having troubles with signs here.

Posible actions:
- Look the other side and hope that liobn is never -1.
  (if it is -1, it would become 0, so not really a big trouble).
- change vmstate type to uint32_t and make VM_INSTANCE_ID_ANY to UINT32_MAX
  (exact same problem if liobn happens to be UINT32_MAX)

I have no clue what are the valid values of liobn.  So I am leaning to
just look the other way and do nothing.

Advise?

Thanks, Juan.

Re: [PATCH 10/13] migration: Improve example and documentation of vmstate_register()

[Juan Quintela]

Stefan Berger <stefanb@linux.ibm.com> wrote:
> On 10/19/23 15:08, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>   docs/devel/migration.rst | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/docs/devel/migration.rst b/docs/devel/migration.rst
>> index c3e1400c0c..a9fde75862 100644
>> --- a/docs/devel/migration.rst
>> +++ b/docs/devel/migration.rst
>> @@ -165,13 +165,17 @@ An example (from hw/input/pckbd.c)
>>         }
>>     };
>>
>> -We are declaring the state with name "pckbd".
>> -The ``version_id`` is 3, and the fields are 4 uint8_t in a KBDState structure.
>> -We registered this with:
>> +We are declaring the state with name "pckbd".  The ``version_id`` is
>> +3, and the fields are 4 uint8_t in a KBDState structure.  We
>
> and there are 4  uint8_t fields in the KBDState structure.

Done thanks.

>
>> +registered this with one of those.  The first one will generate a
>
> I am not sure what this means 'We registered this with one of
> those'. What is 'one of those'?

Changed to:

We
registered this ``VMSTATEDescription`` with one of the following
functions.

> Maybe you mean: We register the KBDState with one of the following
> functions.
>
>> +device ``instance_id`` different for each registration.  Use the
>> +second one if you already have an id different for each instance of
>> +the device:
> ... have an id that is is different for each ...

Done

>>
>>   .. code:: c
>>
>> -    vmstate_register(NULL, 0, &vmstate_kbd, s);
>> +    vmstate_register_any(NULL, &vmstate_kbd, s);
>> +    vmstate_register(NULL, instance_id, &vmstate_kbd, s);
>>
>>   For devices that are ``qdev`` based, we can register the device in the class
>>   init function:

Thanks

Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Nicholas Piggin]

On Fri Oct 20, 2023 at 6:33 PM AEST, Greg Kurz wrote:
> On Fri, 20 Oct 2023 17:49:38 +1000
> "Nicholas Piggin" <npiggin@gmail.com> wrote:
>
> > On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz wrote:
> > > On Thu, 19 Oct 2023 21:08:25 +0200
> > > Juan Quintela <quintela@redhat.com> wrote:
> > >
> > > > Current code does:
> > > > - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> > > >   dependinfg on cpu number
> > > > - for newer machines, it register vmstate_icp with "icp/server" name
> > > >   and instance 0
> > > > - now it unregisters "icp/server" for the 1st instance.
> > > > 
> > > > This is wrong at many levels:
> > > > - we shouldn't have two VMSTATEDescriptions with the same name
> > > > - In case this is the only solution that we can came with, it needs to
> > > >   be:
> > > >   * register pre_2_10_vmstate_dummy_icp
> > > >   * unregister pre_2_10_vmstate_dummy_icp
> > > >   * register real vmstate_icp
> > > > 
> > > > As the initialization of this machine is already complex enough, I
> > > > need help from PPC maintainers to fix this.
> > > > 
> > > > Volunteers?
> > > > 
> > > > CC: Cedric Le Goater <clg@kaod.org>
> > > > CC: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > > CC: David Gibson <david@gibson.dropbear.id.au>
> > > > CC: Greg Kurz <groug@kaod.org>
> > > > 
> > > > Signed-off-by: Juan Quintela <quintela@redhat.com>
> > > > ---
> > > >  hw/ppc/spapr.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index cb840676d3..8531d13492 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
> > > >  }
> > > >  
> > > >  static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > > > -    .name = "icp/server",
> > > > +    /*
> > > > +     * Hack ahead.  We can't have two devices with the same name and
> > > > +     * instance id.  So I rename this to pass make check.
> > > > +     * Real help from people who knows the hardware is needed.
> > > > +     */
> > > > +    .name = "pre-2.10-icp/server",
> > > >      .version_id = 1,
> > > >      .minimum_version_id = 1,
> > > >      .needed = pre_2_10_vmstate_dummy_icp_needed,
> > >
> > > I guess this fix is acceptable as well and a lot simpler than
> > > reverting the hack actually. Outcome is the same : drop
> > > compat with pseries-2.9 and older.
> > >
> > > Reviewed-by: Greg Kurz <groug@kaod.org>
> > 
> > So the reason we can't have duplicate names registered, aside from it
> > surely going bad if we actually send or receive a stream at the point
> > they are registered, is the duplcate check introduced in patch 9? But
> > before that, this hack does seem to actually work because the duplicate
> > is unregistered right away.
> > 
>
> Correct.
>
> > If I understand the workaround, there is an asymmetry in the migration
> > sequence in that receiving an unexpected object would cause a failure,
> > but going from newer to older would just skip some "expected" objects
> > and that didn't cause a problem. So you only have to deal with ignoring
> > the unexpected ones going form older to newer.
> > 
>
> Correct.
>
> > Side question, is it possible to flag the problem of *not* receiving
> > an object that you did expect? That might be a source of bugs too.
> > 
>
> AFAICR we try to only migrate state that differs from reset : the
> destination cannot really assume it will receive anything for a
> given device.

That's true, I guess you could always add some flag yourself if
you certainly need something.

>
> > Anyway, I wonder if we could fix this spapr problem by adding a special
> > case wild card instance matcher to ignore it? It's still a bit hacky
> > but maybe a bit nicer. I don't mind deprecating the machine soon if
> > you want to clear the wildcard hack away soon, but it would be nice to
> > separate the deprecation and removal from the fix, if possible.
> > 
> > This patch is not tested but hopefully helps illustrate the idea.
> > 
>
> I'm not sure this will fly with older QEMUs that don't know about
> VMSTATE_INSTANCE_ID_WILD... but I'll let Juan comment on that.

You could be right about that. He gave a simpler solution now
anyway.

Thanks,
Nick