Re: [Qemu-devel] [PATCH] Various NICs: Fix suspend/resume of multiple instances

[Jan Kiszka <jan.kiszka@siemens.com>]

Paul Brook wrote:
> On Wednesday 16 July 2008, Jan Kiszka wrote:
>> We ran into the issue that restoring two (or more) instances of the same
>> NIC does not work. The reason is that the same instance number is passed
>> to register_savevm by many NIC emulations. Patch below addresses this
>> for eepro100, ne2000, pcnet and rtl8139
> 
> This is wrong. You should pass -1.

Well... OK. Here is a version that
 - documents this preferred usage pattern,
 - fixes those four NICs accordingly,
 - cleans up the e1000 as well so that no one will ever use this as an
   incorrect reference again.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/e1000.c    |   10 +++-------
 hw/eepro100.c |    3 +--
 hw/ne2000.c   |    5 ++---
 hw/pcnet.c    |    2 +-
 hw/rtl8139.c  |    3 +--
 vl.c          |    4 +++-
 6 files changed, 11 insertions(+), 16 deletions(-)

Index: b/hw/pcnet.c
===================================================================
--- a/hw/pcnet.c
+++ b/hw/pcnet.c
@@ -1916,7 +1916,7 @@ static void pcnet_common_init(PCNetState
         d->vc = NULL;
     }
     pcnet_h_reset(d);
-    register_savevm("pcnet", 0, 2, pcnet_save, pcnet_load, d);
+    register_savevm("pcnet", -1, 2, pcnet_save, pcnet_load, d);
 }
 
 /* PCI interface */
Index: b/hw/eepro100.c
===================================================================
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1792,8 +1792,7 @@ static void nic_init(PCIBus * bus, NICIn
 
     qemu_register_reset(nic_reset, s);
 
-    /* XXX: instance number ? */
-    register_savevm(name, 0, 3, nic_save, nic_load, s);
+    register_savevm(name, -1, 3, nic_save, nic_load, s);
 }
 
 void pci_i82551_init(PCIBus * bus, NICInfo * nd, int devfn)
Index: b/hw/ne2000.c
===================================================================
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -753,7 +753,7 @@ void isa_ne2000_init(int base, qemu_irq
              s->macaddr[4],
              s->macaddr[5]);
 
-    register_savevm("ne2000", 0, 2, ne2000_save, ne2000_load, s);
+    register_savevm("ne2000", -1, 2, ne2000_save, ne2000_load, s);
 }
 
 /***********************************************************/
@@ -823,6 +823,5 @@ void pci_ne2000_init(PCIBus *bus, NICInf
              s->macaddr[4],
              s->macaddr[5]);
 
-    /* XXX: instance number ? */
-    register_savevm("ne2000", 0, 3, ne2000_save, ne2000_load, s);
+    register_savevm("ne2000", -1, 3, ne2000_save, ne2000_load, s);
 }
Index: b/hw/rtl8139.c
===================================================================
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -3454,8 +3454,7 @@ void pci_rtl8139_init(PCIBus *bus, NICIn
     s->cplus_txbuffer_len = 0;
     s->cplus_txbuffer_offset = 0;
 
-    /* XXX: instance number ? */
-    register_savevm("rtl8139", 0, 3, rtl8139_save, rtl8139_load, s);
+    register_savevm("rtl8139", -1, 3, rtl8139_save, rtl8139_load, s);
 
 #if RTL8139_ONBOARD_TIMER
     s->timer = qemu_new_timer(vm_clock, rtl8139_timer, s);
Index: b/hw/e1000.c
===================================================================
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -76,7 +76,6 @@ typedef struct E1000State_st {
     PCIDevice dev;
     VLANClientState *vc;
     NICInfo *nd;
-    uint32_t instance;
     uint32_t mmio_base;
     int mmio_index;
 
@@ -814,7 +813,6 @@ nic_save(QEMUFile *f, void *opaque)
     int i, j;
 
     pci_device_save(&s->dev, f);
-    qemu_put_be32s(f, &s->instance);
     qemu_put_be32s(f, &s->mmio_base);
     qemu_put_be32s(f, &s->rxbuf_size);
     qemu_put_be32s(f, &s->rxbuf_min_shift);
@@ -859,7 +857,8 @@ nic_load(QEMUFile *f, void *opaque, int
 
     if ((ret = pci_device_load(&s->dev, f)) < 0)
         return ret;
-    qemu_get_be32s(f, &s->instance);
+    if (version_id == 1)
+        qemu_get_be32s(f, &i); /* once some unused instance id */
     qemu_get_be32s(f, &s->mmio_base);
     qemu_get_be32s(f, &s->rxbuf_size);
     qemu_get_be32s(f, &s->rxbuf_min_shift);
@@ -958,7 +957,6 @@ pci_e1000_init(PCIBus *bus, NICInfo *nd,
 {
     E1000State *d;
     uint8_t *pci_conf;
-    static int instance;
     uint16_t checksum = 0;
     char *info_str = "e1000";
     int i;
@@ -989,8 +987,6 @@ pci_e1000_init(PCIBus *bus, NICInfo *nd,
     pci_register_io_region((PCIDevice *)d, 1, IOPORT_SIZE,
                            PCI_ADDRESS_SPACE_IO, ioport_map);
 
-    d->instance = instance++;
-
     d->nd = nd;
     memmove(d->eeprom_data, e1000_eeprom_template,
         sizeof e1000_eeprom_template);
@@ -1016,5 +1012,5 @@ pci_e1000_init(PCIBus *bus, NICInfo *nd,
              d->nd->macaddr[0], d->nd->macaddr[1], d->nd->macaddr[2],
              d->nd->macaddr[3], d->nd->macaddr[4], d->nd->macaddr[5]);
 
-    register_savevm(info_str, d->instance, 1, nic_save, nic_load, d);
+    register_savevm(info_str, -1, 2, nic_save, nic_load, d);
 }
Index: b/vl.c
===================================================================
--- a/vl.c
+++ b/vl.c
@@ -6063,7 +6063,9 @@ typedef struct SaveStateEntry {
 static SaveStateEntry *first_se;
 
 /* TODO: Individual devices generally have very little idea about the rest
-   of the system, so instance_id should be removed/replaced.  */
+   of the system, so instance_id should be removed/replaced.
+   Meanwhile pass -1 as instance_id if you do not already have a clearly
+   distinguishing id for all instances of your device class. */
 int register_savevm(const char *idstr,
                     int instance_id,
                     int version_id,