Patchwork [5/5] e1000: using new interface--unmap to unplug

login
register
mail settings
Submitter pingfan liu
Date July 25, 2012, 3:31 a.m.
Message ID <1343187070-27371-6-git-send-email-qemulist@gmail.com>
Download mbox | patch
Permalink /patch/173100/
State New
Headers show

Comments

pingfan liu - July 25, 2012, 3:31 a.m.
From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/e1000.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)
Stefan Hajnoczi - July 25, 2012, 7:12 a.m.
On Wed, Jul 25, 2012 at 4:31 AM, Liu Ping Fan <qemulist@gmail.com> wrote:
> From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  hw/e1000.c |   15 +++++++++++++--
>  1 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/hw/e1000.c b/hw/e1000.c
> index 4573f13..4c1e141 100644
> --- a/hw/e1000.c
> +++ b/hw/e1000.c
> @@ -1192,6 +1192,18 @@ e1000_cleanup(VLANClientState *nc)
>      s->nic = NULL;
>  }
>
> +static void
> +pci_e1000_unmap(DeviceState *dev)
> +{
> +    PCIDevice *p = PCI_DEVICE(dev);
> +    E1000State *d = DO_UPCAST(E1000State, dev, p);
> +
> +    /* DO NOT FREE anything!until refcnt=0 */
> +    /* isolate from memory view */
> +    memory_region_destroy(&d->mmio);
> +    memory_region_destroy(&d->io);
> +}

It's not obvious to me why a 2-stage cleanup is needed (->unmap(),
->exit()).  Explaining things a bit more in the commit description
would help.  Here's what I'm thinking:

We want to remove the memory regions at the same time as removing the
device from the tree, but ->exit() is only called when the object is
finalized.  Because of the object reference held during dispatch, the
reference might not reach 0 during hotplug and another thread could
still be running this device's code?

This series only applies this change to e1000 and piix pci hotplug.
How/when will all the other devices be converted?  Will it be safe to
leave them unconverted once dispatch really happens in parallel?

Stefan

Patch

diff --git a/hw/e1000.c b/hw/e1000.c
index 4573f13..4c1e141 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1192,6 +1192,18 @@  e1000_cleanup(VLANClientState *nc)
     s->nic = NULL;
 }
 
+static void
+pci_e1000_unmap(DeviceState *dev)
+{
+    PCIDevice *p = PCI_DEVICE(dev);
+    E1000State *d = DO_UPCAST(E1000State, dev, p);
+
+    /* DO NOT FREE anything!until refcnt=0 */
+    /* isolate from memory view */
+    memory_region_destroy(&d->mmio);
+    memory_region_destroy(&d->io);
+}
+
 static int
 pci_e1000_uninit(PCIDevice *dev)
 {
@@ -1199,8 +1211,6 @@  pci_e1000_uninit(PCIDevice *dev)
 
     qemu_del_timer(d->autoneg_timer);
     qemu_free_timer(d->autoneg_timer);
-    memory_region_destroy(&d->mmio);
-    memory_region_destroy(&d->io);
     qemu_del_vlan_client(&d->nic->nc);
     return 0;
 }
@@ -1283,6 +1293,7 @@  static void e1000_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_NETWORK_ETHERNET;
     dc->desc = "Intel Gigabit Ethernet";
     dc->reset = qdev_e1000_reset;
+    dc->unmap = pci_e1000_unmap;
     dc->vmsd = &vmstate_e1000;
     dc->props = e1000_properties;
 }