[Qemu-devel] Patch: NE2000 savevm problem solved!

[Qemu-devel] Patch: NE2000 savevm problem solved!

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1221 bytes --]

Hi,

With the attached patch, Win98 guest resumes -user-net properly after
loadvm'ming.

It adds savevm functions for NE2000State and PCIDevice, which I created by
"perl qemu-struct2savevm.pl NE2000State ne2000 < hw/ne2000.c" and
"perl qemu-struct2savevm.pl PCIDevice generic_pci < hw/ne2000.c".

Two issues remain:
- *Every* PCIDevice is now savevm'ed, because the savevm handler is
  registered in pci_register_device.
- the savevm handler is therefore registered with the name passed to
  pci_register_device. It would be probably better to prefix that with
  "pci_" (note that it only works at the moment because vga registers
  a "vga" savevm handler, while the PCI device is registered as "VGA").

I am willing to fix those issues when you tell me what solution you
prefer.

BTW, it would be sufficient to add savevm handlers for ne2000, its
PCIDevice, and piix3_state. Leave one out and you break user networking.
Knoppix only resumes properly, because it does not modify the irq of the
ne2000 card, and is much more clever about the internal state of that
card being possibly randomly reset.

Ciao,
Dscho

P.S.: If somebody cares for a technical description of savevm handlers, I
will write what I learnt so far.

[-- Attachment #2: Type: TEXT/PLAIN, Size: 3501 bytes --]

diff -Nurb qemu_cvs/hw/ne2000.c qemu_ne2000_savevm/hw/ne2000.c
--- qemu_cvs/hw/ne2000.c	2004-09-24 05:28:43.000000000 +0200
+++ qemu_ne2000_savevm/hw/ne2000.c	2004-09-24 05:28:43.000000000 +0200
@@ -538,6 +538,59 @@
     return 0;
 }
 
+static void ne2000_save(QEMUFile* f,void* opaque)
+{
+	NE2000State* s=(NE2000State*)opaque;
+
+	qemu_put_8s(f, &s->cmd);
+	qemu_put_be32s(f, &s->start);
+	qemu_put_be32s(f, &s->stop);
+	qemu_put_8s(f, &s->boundary);
+	qemu_put_8s(f, &s->tsr);
+	qemu_put_8s(f, &s->tpsr);
+	qemu_put_be16s(f, &s->tcnt);
+	qemu_put_be16s(f, &s->rcnt);
+	qemu_put_be32s(f, &s->rsar);
+	qemu_put_8s(f, &s->rsr);
+	qemu_put_8s(f, &s->isr);
+	qemu_put_8s(f, &s->dcfg);
+	qemu_put_8s(f, &s->imr);
+	qemu_put_buffer(f, s->phys, 6);
+	qemu_put_8s(f, &s->curpag);
+	qemu_put_buffer(f, s->mult, 8);
+	qemu_put_be32s(f, &s->irq);
+	qemu_put_buffer(f, s->mem, NE2000_MEM_SIZE);
+}
+
+static int ne2000_load(QEMUFile* f,void* opaque,int version_id)
+{
+	NE2000State* s=(NE2000State*)opaque;
+
+	if (version_id != 1)
+            return -EINVAL;
+
+	qemu_get_8s(f, &s->cmd);
+	qemu_get_be32s(f, &s->start);
+	qemu_get_be32s(f, &s->stop);
+	qemu_get_8s(f, &s->boundary);
+	qemu_get_8s(f, &s->tsr);
+	qemu_get_8s(f, &s->tpsr);
+	qemu_get_be16s(f, &s->tcnt);
+	qemu_get_be16s(f, &s->rcnt);
+	qemu_get_be32s(f, &s->rsar);
+	qemu_get_8s(f, &s->rsr);
+	qemu_get_8s(f, &s->isr);
+	qemu_get_8s(f, &s->dcfg);
+	qemu_get_8s(f, &s->imr);
+	qemu_get_buffer(f, s->phys, 6);
+	qemu_get_8s(f, &s->curpag);
+	qemu_get_buffer(f, s->mult, 8);
+	qemu_get_be32s(f, &s->irq);
+	qemu_get_buffer(f, s->mem, NE2000_MEM_SIZE);
+
+	return 0;
+}
+
 void isa_ne2000_init(int base, int irq, NetDriverState *nd)
 {
     NE2000State *s;
@@ -546,6 +599,8 @@
     if (!s)
         return;
     
+    register_savevm("ne2000", 0, 1, ne2000_save, ne2000_load, s);
+
     register_ioport_write(base, 16, 1, ne2000_ioport_write, s);
     register_ioport_read(base, 16, 1, ne2000_ioport_read, s);
 
@@ -620,4 +675,8 @@
     s->nd = nd;
     ne2000_reset(s);
     qemu_add_read_packet(nd, ne2000_can_receive, ne2000_receive, s);
+
+    register_savevm("ne2000", 0, 1, ne2000_save, ne2000_load, s);
+
 }
+
diff -Nurb qemu_cvs/hw/pci.c qemu_ne2000_savevm/hw/pci.c
--- qemu_cvs/hw/pci.c	2004-09-24 05:28:43.000000000 +0200
+++ qemu_ne2000_savevm/hw/pci.c	2004-09-24 05:28:43.000000000 +0200
@@ -62,6 +62,31 @@
     return bus;
 }
 
+static void generic_pci_save(QEMUFile* f,void* opaque)
+{
+	PCIDevice* s=(PCIDevice*)opaque;
+
+	qemu_put_buffer(f, s->config, 256);
+	qemu_put_be32s(f, &s->devfn);
+	qemu_put_buffer(f, s->name, 64);
+	qemu_put_be32s(f, &s->irq_index);
+}
+
+static int generic_pci_load(QEMUFile* f,void* opaque,int version_id)
+{
+	PCIDevice* s=(PCIDevice*)opaque;
+
+	if (version_id != 1)
+            return -EINVAL;
+
+	qemu_get_buffer(f, s->config, 256);
+	qemu_get_be32s(f, &s->devfn);
+	qemu_get_buffer(f, s->name, 64);
+	qemu_get_be32s(f, &s->irq_index);
+
+	return 0;
+}
+
 /* -1 for devfn means auto assign */
 PCIDevice *pci_register_device(PCIBus *bus, const char *name, 
                                int instance_size, int devfn,
@@ -96,6 +121,9 @@
     pci_dev->config_write = config_write;
     pci_dev->irq_index = pci_irq_index++;
     bus->devices[devfn] = pci_dev;
+
+    register_savevm(name, 0, 1, generic_pci_save, generic_pci_load, pci_dev);
+
     return pci_dev;
 }
 

[-- Attachment #3: Type: APPLICATION/x-perl, Size: 1657 bytes --]

Re: [Qemu-devel] Patch: NE2000 savevm problem solved!

[Gianni Tedesco]

On Fri, 2004-09-24 at 12:07 +0200, Johannes Schindelin wrote:
> With the attached patch, Win98 guest resumes -user-net properly after
> loadvm'ming.
> 
> It adds savevm functions for NE2000State and PCIDevice, which I created by
> "perl qemu-struct2savevm.pl NE2000State ne2000 < hw/ne2000.c" and
> "perl qemu-struct2savevm.pl PCIDevice generic_pci < hw/ne2000.c".
> 
> Two issues remain:
> - *Every* PCIDevice is now savevm'ed, because the savevm handler is
>   registered in pci_register_device.
> - the savevm handler is therefore registered with the name passed to
>   pci_register_device. It would be probably better to prefix that with
>   "pci_" (note that it only works at the moment because vga registers
>   a "vga" savevm handler, while the PCI device is registered as "VGA").
> 
> I am willing to fix those issues when you tell me what solution you
> prefer.

I'm not sure how well your pci_generic_{load,save} will work. What
devices did you test it on? Perhaps it would be a better idea to not
make it default, but allow devices to use them if they know they will
work.

As an aside. I think it's generally a good idea to make the hardware
more object oriented in nature. ie. all devices have common methods
(ctor, dtor, load, save) and all bussess with (ctor, dtor, add, remove).

That would pave the way for having a nice GUI that allows user to
configure a machine, and in the future to configure it at run-time with
hot-swap. That would also be useful for OS developers to test their hot-
swap subsystems.

-- 
// Gianni Tedesco (gianni at scaramanga dot co dot uk)
lynx --source www.scaramanga.co.uk/scaramanga.asc | gpg --import
8646BE7D: 6D9F 2287 870E A2C9 8F60 3A3C 91B5 7669 8646 BE7D

Re: [Qemu-devel] Patch: NE2000 savevm problem solved!

[Johannes Schindelin]

Hi,

On Mon, 27 Sep 2004, Gianni Tedesco wrote:

> On Fri, 2004-09-24 at 12:07 +0200, Johannes Schindelin wrote:
> > I am willing to fix those issues when you tell me what solution you
> > prefer.
>
> I'm not sure how well your pci_generic_{load,save} will work. What
> devices did you test it on? Perhaps it would be a better idea to not
> make it default, but allow devices to use them if they know they will
> work.

Yes, it is better not to make it a default, I agree. I tested with a
standard configuration using Win98 guest, i.e. Cirrus VGA, NE2000, etc. As
nothing broke, I decided to use it as a default for a quick fix, and find
a better solution later.

> As an aside. I think it's generally a good idea to make the hardware
> more object oriented in nature. ie. all devices have common methods
> (ctor, dtor, load, save) and all bussess with (ctor, dtor, add, remove).

I find that a lot of that is there already. There may not be a base
prototype for the constructor, but the savevm mechanism works quite well
(You register support for it in the "ctor" by register_savevm, where you
supply a load and a save function which comply with a prototype), and what
I saw from PCI looked fine by me. And there exist sort of constructors for
every device, i.e. pci_ne2000_init() for the network card.

> That would pave the way for having a nice GUI that allows user to
> configure a machine, and in the future to configure it at run-time with
> hot-swap. That would also be useful for OS developers to test their hot-
> swap subsystems.

Well, as for the GUI thing... A normal user would probably need just a way
to change the CD or the floppy with a nice File Chooser.

Anyway, what I wanted to achieve was to find out why network was a
netunwork after loadvm'ing, and maybe fix it. That I did. Would it be okay
for you if I keep the generic_pci_load/save(), but add a prototype and let
piix and ne2000 handle the register_savevm() themselves?

Ciao,
Dscho

Re: [Qemu-devel] Patch: NE2000 savevm problem solved!

[Gianni Tedesco]

On Tue, 2004-09-28 at 00:23 +0200, Johannes Schindelin wrote:
> > As an aside. I think it's generally a good idea to make the hardware
> > more object oriented in nature. ie. all devices have common methods
> > (ctor, dtor, load, save) and all bussess with (ctor, dtor, add, remove).
> 
> I find that a lot of that is there already. There may not be a base
> prototype for the constructor, but the savevm mechanism works quite well
> (You register support for it in the "ctor" by register_savevm, where you
> supply a load and a save function which comply with a prototype), and what
> I saw from PCI looked fine by me. And there exist sort of constructors for
> every device, i.e. pci_ne2000_init() for the network card.

Yeah, the PCI subsystem is really nice, but if you look at each system
(pc, ppc, etc..), there has to be some code there to hook up PIC's,
timers, etc...

As more hardware is added (lets say we get SMP support in future), the
possible number of configurations become greater, and the more twisted
that code there will get.

> > That would pave the way for having a nice GUI that allows user to
> > configure a machine, and in the future to configure it at run-time with
> > hot-swap. That would also be useful for OS developers to test their hot-
> > swap subsystems.
> 
> Well, as for the GUI thing... A normal user would probably need just a way
> to change the CD or the floppy with a nice File Chooser.

Yeah, I'm more talking about adding and removing NIC's for example.. Or
when I get around to finishing USB support, allowing a digital camera on
the host to be added to the guest system at runtime :)

> Anyway, what I wanted to achieve was to find out why network was a
> netunwork after loadvm'ing, and maybe fix it. That I did. Would it be okay
> for you if I keep the generic_pci_load/save(), but add a prototype and let
> piix and ne2000 handle the register_savevm() themselves?

Yeah, that's perfect from my POV. But it's Fabrices opinion that really
matters ;)

-- 
// Gianni Tedesco (gianni at scaramanga dot co dot uk)
lynx --source www.scaramanga.co.uk/scaramanga.asc | gpg --import
8646BE7D: 6D9F 2287 870E A2C9 8F60 3A3C 91B5 7669 8646 BE7D