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

[Stefan Hajnoczi]

On Wed, Jul 25, 2012 at 4:31 AM, Liu Ping Fan qemul...@gmail.com wrote:
 From: Liu Ping Fan pingf...@linux.vnet.ibm.com

 Signed-off-by: Liu Ping Fan pingf...@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
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Liu Ping Fan]

From: Liu Ping Fan pingf...@linux.vnet.ibm.com

Signed-off-by: Liu Ping Fan pingf...@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);
+}
+
 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;
 }
-- 
1.7.4.4

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html