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

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

[Juan Quintela]

Hi

Hi

on this v2:
- rebased on top of master (no conflicts)
- handled reviewed by
- improved documentation
- Changed the ppc hack to maintain backwards compatibility.

Please review.

[v1]
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
  migration: Hack to maintain backwards compatibility for ppc
  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 | 34 ++++++++++++++++++++++++++++++++++
 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/intc/xics.c              | 17 +++++++++++++++--
 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              | 25 +++++++++++++++++++++++--
 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          | 32 ++++++++++++++++++++++++++++++++
 net/slirp.c                 |  5 +++--
 27 files changed, 139 insertions(+), 37 deletions(-)

-- 
2.41.0

[PATCH v2 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.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
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 v2 02/13] migration: Use vmstate_register_any()

[Juan Quintela]

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

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
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 v2 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)
$

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
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 v2 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)

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
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 v2 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)

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
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 v2 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)

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
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 v2 07/13] migration: Hack to maintain backwards compatibility for ppc

[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

Created vmstate_replace_hack_for_ppc() with warnings left and right
that it is a hack.

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>
---
 include/migration/vmstate.h | 11 +++++++++++
 hw/intc/xics.c              | 17 +++++++++++++++--
 hw/ppc/spapr.c              | 25 +++++++++++++++++++++++--
 migration/savevm.c          | 18 ++++++++++++++++++
 4 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 9ca7e9cc48..65deaecc92 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1230,6 +1230,17 @@ static inline int vmstate_register(VMStateIf *obj, int instance_id,
                                           opaque, -1, 0, NULL);
 }
 
+/**
+ * vmstate_replace_hack_for_ppc() - ppc used to abuse vmstate_register
+ *
+ * Don't even think about using this function in new code.
+ *
+ * Returns: 0 on success, -1 on failure
+ */
+int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id,
+                                 const VMStateDescription *vmsd,
+                                 void *opaque);
+
 /**
  * vmstate_register_any() - legacy function to register state
  * serialisation description and let the function choose the id
diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index c7f8abd71e..a03979e72a 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -335,8 +335,21 @@ static void icp_realize(DeviceState *dev, Error **errp)
             return;
         }
     }
-
-    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
+    /*
+     * The way that pre_2_10_icp is handling is really, really hacky.
+     * We used to have here this call:
+     *
+     * vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
+     *
+     * But we were doing:
+     *     pre_2_10_vmstate_register_dummy_icp()
+     *     this vmstate_register()
+     *     pre_2_10_vmstate_unregister_dummy_icp()
+     *
+     * So for a short amount of time we had to vmstate entries with
+     * the same name.  This fixes it.
+     */
+    vmstate_replace_hack_for_ppc(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
 }
 
 static void icp_unrealize(DeviceState *dev)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cb840676d3..a75cf475ad 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -143,6 +143,11 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
 }
 
 static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
+    /*
+     * 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 = "icp/server",
     .version_id = 1,
     .minimum_version_id = 1,
@@ -155,16 +160,32 @@ static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
     },
 };
 
+/*
+ * See comment in hw/intc/xics.c:icp_realize()
+ *
+ * You have to remove vmstate_replace_hack_for_ppc() when you remove
+ * the machine types that need the following function.
+ */
 static void pre_2_10_vmstate_register_dummy_icp(int i)
 {
     vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp,
                      (void *)(uintptr_t) i);
 }
 
+/*
+ * See comment in hw/intc/xics.c:icp_realize()
+ *
+ * You have to remove vmstate_replace_hack_for_ppc() when you remove
+ * the machine types that need the following function.
+ */
 static void pre_2_10_vmstate_unregister_dummy_icp(int i)
 {
-    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
-                       (void *)(uintptr_t) i);
+    /*
+     * This used to be:
+     *
+     *    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
+     * 	                    (void *)(uintptr_t) i);
+     */
 }
 
 int spapr_max_server_number(SpaprMachineState *spapr)
diff --git a/migration/savevm.c b/migration/savevm.c
index 8622f229e5..d3a30686d4 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -846,6 +846,24 @@ static void vmstate_check(const VMStateDescription *vmsd)
     }
 }
 
+/*
+ * See comment in hw/intc/xics.c:icp_realize()
+ *
+ * This function can be removed when
+ * pre_2_10_vmstate_register_dummy_icp() is removed.
+ */
+int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id,
+                                 const VMStateDescription *vmsd,
+                                 void *opaque)
+{
+    SaveStateEntry *se = find_se(vmsd->name, instance_id);
+
+    if (se) {
+        savevm_state_handler_remove(se);
+    }
+    return vmstate_register(obj, instance_id, vmsd, opaque);
+}
+
 int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque, int alias_id,
-- 
2.41.0

[PATCH v2 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 65deaecc92..896c3f69d2 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 v2 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 d3a30686d4..3e0ece84e8 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 v2 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..bfd8710c95 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 there are 4 uint8_t fields in the KBDState structure.  We
+registered this ``VMSTATEDescription`` with one of the following
+functions.  The first one will generate a device ``instance_id``
+different for each registration.  Use the second one if you already
+have an id that is 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 v2 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);
    }
}

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
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 v2 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()

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
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 v2 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.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
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 v2 06/13] migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices

[Christian Borntraeger]

Am 20.10.23 um 11:07 schrieb 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)
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 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(-)

Actually both devices should be theŕe only once - I think.

> 
> 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 v2 06/13] migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices

[Juan Quintela]

Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
> Am 20.10.23 um 11:07 schrieb 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)
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>> 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(-)
>
> Actually both devices should be theŕe only once - I think.

Reverting the patch (but with the check that we don't add duplicated
entries):

# Testing device 's390-skeys-qemu'
Broken pipe
../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:194: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)
$ 

This is device-intraspect-test.

Somehow this function decides that you can hotplug this two s390
devices, if that is not the case, they need to be marked somehow not
hot-plugabble.

static void test_one_device(QTestState *qts, const char *type)
{
    QDict *resp;
    char *help, *escaped;
    GRegex *comma;

    g_test_message("Testing device '%s'", type);

    resp = qtest_qmp(qts, "{'execute': 'device-list-properties',"
                          " 'arguments': {'typename': %s}}",
               type);
    qobject_unref(resp);

    comma = g_regex_new(",", 0, 0, NULL);
    escaped = g_regex_replace_literal(comma, type, -1, 0, ",,", 0, NULL);
    g_regex_unref(comma);

    help = qtest_hmp(qts, "device_add \"%s,help\"", escaped);
    g_free(help);
    g_free(escaped);
}

Thanks, Juan.



>
>> 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 v2 06/13] migration: Use VMSTATE_INSTANCE_ID_ANY for s390 devices

[Thomas Huth]

On 20/10/2023 11.54, Juan Quintela wrote:
> Christian Borntraeger <borntraeger@linux.ibm.com> wrote:
>> Am 20.10.23 um 11:07 schrieb 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)
>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>>> 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(-)
>>
>> Actually both devices should be theŕe only once - I think.
> 
> Reverting the patch (but with the check that we don't add duplicated
> entries):
> 
> # Testing device 's390-skeys-qemu'
> Broken pipe
> ../../../../../mnt/code/qemu/full/tests/qtest/libqtest.c:194: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
> Aborted (core dumped)
> $
> 
> This is device-intraspect-test.
> 
> Somehow this function decides that you can hotplug this two s390
> devices, if that is not the case, they need to be marked somehow not
> hot-plugabble.

Sorry, no, it's not hot-plugging what is happening here, it's device 
introspection. That means it should always be possible to create a second 
instance of a device for introspection - it must just not be realized if 
there can be only one instance.

Looking at the code here:

tatic inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
                                             Error **errp)
{
     S390SKeysState *ss = S390_SKEYS(obj);

     /* Prevent double registration of savevm handler */
     if (ss->migration_enabled == value) {
         return;
     }

     ss->migration_enabled = value;

     if (ss->migration_enabled) {
         register_savevm_live(TYPE_S390_SKEYS, 0, 1,
                              &savevm_s390_storage_keys, ss);
     } else {
         unregister_savevm(VMSTATE_IF(ss), TYPE_S390_SKEYS, ss);
     }
}

static void s390_skeys_instance_init(Object *obj)
{
     object_property_add_bool(obj, "migration-enabled",
                              s390_skeys_get_migration_enabled,
                              s390_skeys_set_migration_enabled);
     object_property_set_bool(obj, "migration-enabled", true, NULL);
}

I think the problem is the object_property_set_bool() in the 
_instance_init() function. The setting of the property should maybe rather 
happen during the realization instead?

  Thomas

Re: [PATCH v2 07/13] migration: Hack to maintain backwards compatibility for ppc

[Nicholas Piggin]

On Fri Oct 20, 2023 at 7:07 PM AEST, Juan Quintela 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
>
> Created vmstate_replace_hack_for_ppc() with warnings left and right
> that it is a hack.

Thanks. We'll look at deprecating 2.9 soon so this can all be removed.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> 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>
> ---
>  include/migration/vmstate.h | 11 +++++++++++
>  hw/intc/xics.c              | 17 +++++++++++++++--
>  hw/ppc/spapr.c              | 25 +++++++++++++++++++++++--
>  migration/savevm.c          | 18 ++++++++++++++++++
>  4 files changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 9ca7e9cc48..65deaecc92 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1230,6 +1230,17 @@ static inline int vmstate_register(VMStateIf *obj, int instance_id,
>                                            opaque, -1, 0, NULL);
>  }
>  
> +/**
> + * vmstate_replace_hack_for_ppc() - ppc used to abuse vmstate_register
> + *
> + * Don't even think about using this function in new code.
> + *
> + * Returns: 0 on success, -1 on failure
> + */
> +int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id,
> +                                 const VMStateDescription *vmsd,
> +                                 void *opaque);
> +
>  /**
>   * vmstate_register_any() - legacy function to register state
>   * serialisation description and let the function choose the id
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index c7f8abd71e..a03979e72a 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -335,8 +335,21 @@ static void icp_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>      }
> -
> -    vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> +    /*
> +     * The way that pre_2_10_icp is handling is really, really hacky.
> +     * We used to have here this call:
> +     *
> +     * vmstate_register(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
> +     *
> +     * But we were doing:
> +     *     pre_2_10_vmstate_register_dummy_icp()
> +     *     this vmstate_register()
> +     *     pre_2_10_vmstate_unregister_dummy_icp()
> +     *
> +     * So for a short amount of time we had to vmstate entries with
> +     * the same name.  This fixes it.
> +     */
> +    vmstate_replace_hack_for_ppc(NULL, icp->cs->cpu_index, &vmstate_icp_server, icp);
>  }
>  
>  static void icp_unrealize(DeviceState *dev)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index cb840676d3..a75cf475ad 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -143,6 +143,11 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void *opaque)
>  }
>  
>  static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> +    /*
> +     * 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 = "icp/server",
>      .version_id = 1,
>      .minimum_version_id = 1,
> @@ -155,16 +160,32 @@ static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
>      },
>  };
>  
> +/*
> + * See comment in hw/intc/xics.c:icp_realize()
> + *
> + * You have to remove vmstate_replace_hack_for_ppc() when you remove
> + * the machine types that need the following function.
> + */
>  static void pre_2_10_vmstate_register_dummy_icp(int i)
>  {
>      vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp,
>                       (void *)(uintptr_t) i);
>  }
>  
> +/*
> + * See comment in hw/intc/xics.c:icp_realize()
> + *
> + * You have to remove vmstate_replace_hack_for_ppc() when you remove
> + * the machine types that need the following function.
> + */
>  static void pre_2_10_vmstate_unregister_dummy_icp(int i)
>  {
> -    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
> -                       (void *)(uintptr_t) i);
> +    /*
> +     * This used to be:
> +     *
> +     *    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
> +     * 	                    (void *)(uintptr_t) i);
> +     */
>  }
>  
>  int spapr_max_server_number(SpaprMachineState *spapr)
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 8622f229e5..d3a30686d4 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -846,6 +846,24 @@ static void vmstate_check(const VMStateDescription *vmsd)
>      }
>  }
>  
> +/*
> + * See comment in hw/intc/xics.c:icp_realize()
> + *
> + * This function can be removed when
> + * pre_2_10_vmstate_register_dummy_icp() is removed.
> + */
> +int vmstate_replace_hack_for_ppc(VMStateIf *obj, int instance_id,
> +                                 const VMStateDescription *vmsd,
> +                                 void *opaque)
> +{
> +    SaveStateEntry *se = find_se(vmsd->name, instance_id);
> +
> +    if (se) {
> +        savevm_state_handler_remove(se);
> +    }
> +    return vmstate_register(obj, instance_id, vmsd, opaque);
> +}
> +
>  int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
>                                     const VMStateDescription *vmsd,
>                                     void *opaque, int alias_id,

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

[Stefan Berger]

On 10/20/23 05:07, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>


Reviewed-by: Stefan Berger <stefanb@linux.ibm.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..bfd8710c95 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 there are 4 uint8_t fields in the KBDState structure.  We
> +registered this ``VMSTATEDescription`` with one of the following
> +functions.  The first one will generate a device ``instance_id``
> +different for each registration.  Use the second one if you already
> +have an id that is 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:

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

[Thomas Huth]

On 20/10/2023 11.07, 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)
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 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);
>   }

This is an instance_init() function. We shouldn't register vmstate (and 
timer) here, but do it in the realize() function instead.

>   
>   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);
>   }

Now here it's been done in the realize() function already ... did this 
really cause trouble for you?
Anyway, I wonder why the first parameter is NULL here ... shouldn't this 
point to dev instead?

>   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);
>   }

It's an instance_init() function again. This should be done in the realize() 
instead.

>   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);
>   }

Dito, this should be moved to realize().

  Thomas

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

[Thomas Huth]

On 20/10/2023 11.07, 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)
> $
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 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);
>   }

Would it make sense to use another unique ID of the device instead? E.g.:

diff a/hw/ide/isa.c b/hw/ide/isa.c
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -73,7 +73,9 @@ 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(VMSTATE_IF(dev),
+                     object_property_get_int(OBJECT(dev), "irq", &error_abort),
+                     &vmstate_ide_isa, s);
      ide_bus_register_restart_cb(&s->bus);
  }
  
  Thomas

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

[Thomas Huth]

On 20/10/2023 11.07, Juan Quintela wrote:
> I have no idea if we can have more than one vmware_vga device, so play
> it safe.

FWIW, it doesn't look like it's possible:

$ ./qemu-system-x86_64 -device vmware-svga  -device vmware-svga
RAMBlock "vmsvga.fifo" already registered, abort!
Aborted (core dumped)

(NB: Aborting is very user-unfriendly here, but that's something for another 
patch...)

> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 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;
>   }

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Thomas Huth]

On 20/10/2023 11.07, 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)
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 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(-)

Please check whether you could replace this by this patch instead:

  https://lore.kernel.org/qemu-devel/20231020145554.662751-1-thuth@redhat.com/

  Thanks,
   Thomas

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

[Thomas Huth]

On 20/10/2023 11.07, 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)
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 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(-)

Please use this patch series instead:

  https://lore.kernel.org/qemu-devel/20231020150554.664422-1-thuth@redhat.com/

  Thanks,
   Thomas

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

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 20/10/2023 11.07, 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)
>> $
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>> 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);
>>   }
>
> Would it make sense to use another unique ID of the device instead? E.g.:
>
> diff a/hw/ide/isa.c b/hw/ide/isa.c
> --- a/hw/ide/isa.c
> +++ b/hw/ide/isa.c
> @@ -73,7 +73,9 @@ 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(VMSTATE_IF(dev),
> +                     object_property_get_int(OBJECT(dev), "irq", &error_abort),
> +                     &vmstate_ide_isa, s);
>      ide_bus_register_restart_cb(&s->bus);
>  }
>    Thomas

Ide is not my part of expertise.
But anything that is different for each instantance is going to be good
for me.

The whole point of this series is to be able to test that there are no
duplicates.  Duplicates are one error when we do real migration.  How we
reach the goal of no duplicates doesn't matter to me.

Later, Juan.

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

[Volker Rümelin]

Am 20.10.23 um 11:07 schrieb Juan Quintela:
> We can have more than one audio card.

Hi Juan,

I wouldn't use the term "audio card" here. In QEMU speak, Audiodev is an
"audio backend".

With best regards,
Volker

>
> void audio_init_audiodevs(void)
> {
>     AudiodevListEntry *e;
>
>     QSIMPLEQ_FOREACH(e, &audiodevs, next) {
>         audio_init(e->dev, &error_fatal);
>     }
> }
>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 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:

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

[Thomas Huth]

On 20/10/2023 21.42, Juan Quintela wrote:
> Thomas Huth <thuth@redhat.com> wrote:
>> On 20/10/2023 11.07, 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)
>>> $
>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>>> 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);
>>>    }
>>
>> Would it make sense to use another unique ID of the device instead? E.g.:
>>
>> diff a/hw/ide/isa.c b/hw/ide/isa.c
>> --- a/hw/ide/isa.c
>> +++ b/hw/ide/isa.c
>> @@ -73,7 +73,9 @@ 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(VMSTATE_IF(dev),
>> +                     object_property_get_int(OBJECT(dev), "irq", &error_abort),
>> +                     &vmstate_ide_isa, s);
>>       ide_bus_register_restart_cb(&s->bus);
>>   }
>>     Thomas
> 
> Ide is not my part of expertise.
> But anything that is different for each instantance is going to be good
> for me.

It's not really my turf either ... ok, so unless the IDE maintainer speaks 
up, I think it's maybe best if you continue with your "_any" patch.

  Thomas

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

[Juan Quintela]

Volker Rümelin <vr_qemu@t-online.de> wrote:
> Am 20.10.23 um 11:07 schrieb Juan Quintela:
>> We can have more than one audio card.
>
> Hi Juan,
>
> I wouldn't use the term "audio card" here. In QEMU speak, Audiodev is an
> "audio backend".

Thanks.  Changed that.

> With best regards,
> Volker
>
>>
>> void audio_init_audiodevs(void)
>> {
>>     AudiodevListEntry *e;
>>
>>     QSIMPLEQ_FOREACH(e, &audiodevs, next) {
>>         audio_init(e->dev, &error_fatal);
>>     }
>> }
>>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>> 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:

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

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 20/10/2023 21.42, Juan Quintela wrote:
>> Thomas Huth <thuth@redhat.com> wrote:
>>> On 20/10/2023 11.07, 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)
>>>> $
>>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>>>> 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);
>>>>    }
>>>
>>> Would it make sense to use another unique ID of the device instead? E.g.:
>>>
>>> diff a/hw/ide/isa.c b/hw/ide/isa.c
>>> --- a/hw/ide/isa.c
>>> +++ b/hw/ide/isa.c
>>> @@ -73,7 +73,9 @@ 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(VMSTATE_IF(dev),
>>> +                     object_property_get_int(OBJECT(dev), "irq", &error_abort),
>>> +                     &vmstate_ide_isa, s);
>>>       ide_bus_register_restart_cb(&s->bus);
>>>   }
>>>     Thomas
>> Ide is not my part of expertise.
>> But anything that is different for each instantance is going to be good
>> for me.
>
> It's not really my turf either ... ok, so unless the IDE maintainer
> speaks up, I think it's maybe best if you continue with your "_any"
> patch.

Ide maintainer can do your change if he likes it.  It is outside of my
understanding to accept it or not (and furthermore, it breaks migration
if you have only one device, with more than one it is already broken).

Later, Juan.