Fix hotplug/hotunplug issue about virtio 1.0 devices

RFC PATCH In review Lin Ma 2 participants 3 messages Jul 20, 2015 – Jul 21, 2015

lore

Share & tools

lore

Lin Ma

author

2015-07-20 15:16 UTC #0 lore raw

RFC PATCH 1/1 Fix hotplug/hotunplug issue about virtio 1.0 devices

Hi Michael and Gerd,

I found an hotplug/hotunplug issue about virtio 1.0 devices and trying to fix it. The bug description is:
(qemu) device_add virtio-gpu-pci,id=gpu0
(qemu) device_del gpu0
(qemu) device_add virtio-gpu-pci,id=gpu0
Duplicate ID 'gpu0' for device
Try "help device_add" for more information

My fix looks like this:

hw/virtio/virtio-pci.c +16 -0

index 283401a..098fc83 100644
@@ -1413,6 +1413,12 @@ static void virtio_pci_modern_region_map(VirtIOPCIProxy *proxy,
     virtio_pci_add_mem_cap(proxy, cap);
 }
 
+static void virtio_pci_modern_region_unmap(VirtIOPCIProxy *proxy,
+                                           VirtIOPCIRegion *region)
+{
+    memory_region_del_subregion(&proxy->modern_bar, &region->mr);
+}
+
 /* This is called by virtio-bus just after the device is plugged. */
 static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
 {

@@ -1584,6 +1590,16 @@ static void virtio_pci_exit(PCIDevice *pci_dev)
 
     msix_uninit_exclusive_bar(pci_dev);
     address_space_destroy(&proxy->modern_as);
+    object_unparent(OBJECT(&proxy->modern_cfg));
+
+    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
+    if (modern) {
+        virtio_pci_modern_region_unmap(proxy, &proxy->common);
+        virtio_pci_modern_region_unmap(proxy, &proxy->isr);
+        virtio_pci_modern_region_unmap(proxy, &proxy->device);
+        virtio_pci_modern_region_unmap(proxy, &proxy->notify);
+    }
+    memory_region_unref(&proxy->modern_bar);
 }
 
 static void virtio_pci_reset(DeviceState *qdev)

But after applying the fix, I got the following errors:
(qemu) device_add virtio-gpu-pci,id=gpu0
(qemu) device_del gpu0
(qemu) **
ERROR:qom/object.c:825:object_unref: assertion failed: (obj->ref > 0)
......

I think because of the grace period of rcu, the function memory_region_unref(as->root) in do_address_space_destroy isn't performed immediately, The do_address_space_destroy is always performed after virtio_pci_exit, That caused 'assertion failed: (obj->ref > 0)'. I have no idea whether my guess is correct or in correct, and don't know how to avoid this assertion failure in this situation. Does my fix make sense ? May I have your ideas or suggestions?

Thanks!
Lin

Michael S. Tsirkin

2015-07-20 15:37 UTC #1 lore raw

Quoted 7 lines · Lin Ma

Hi Michael and Gerd,
I found an hotplug/hotunplug issue about virtio 1.0 devices and trying to fix it.
The bug description is:
(qemu) device_add virtio-gpu-pci,id=gpu0
(qemu) device_del gpu0

At this point, you must wait for guest to ack device removal.

Quoted 62 lines

(qemu) device_add virtio-gpu-pci,id=gpu0
Duplicate ID 'gpu0' for device
Try "help device_add" for more information
My fix looks like this:
hw/virtio/virtio-pci.c +6 -0
index 283401a..098fc83 100644
@@ -1413,6 +1413,12 @@ static void
 virtio_pci_modern_region_map(VirtIOPCIProxy *proxy,
     virtio_pci_add_mem_cap(proxy, cap);
 }
 
+static void virtio_pci_modern_region_unmap(VirtIOPCIProxy *proxy,
+                                           VirtIOPCIRegion *region)
+{
+    memory_region_del_subregion(&proxy->modern_bar, &region->mr);
+}
+
 /* This is called by virtio-bus just after the device is plugged. */
 static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
{
hw/virtio/virtio-pci.c +10 -0
@@ -1584,6 +1590,16 @@ static void virtio_pci_exit(PCIDevice *pci_dev)
 
     msix_uninit_exclusive_bar(pci_dev);
     address_space_destroy(&proxy->modern_as);
+    object_unparent(OBJECT(&proxy->modern_cfg));
+
+    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
+    if (modern) {
+        virtio_pci_modern_region_unmap(proxy, &proxy->common);
+        virtio_pci_modern_region_unmap(proxy, &proxy->isr);
+        virtio_pci_modern_region_unmap(proxy, &proxy->device);
+        virtio_pci_modern_region_unmap(proxy, &proxy->notify);
+    }
+    memory_region_unref(&proxy->modern_bar);
 }
 
 static void virtio_pci_reset(DeviceState *qdev)
But after applying the fix, I got the following errors:
(qemu) device_add virtio-gpu-pci,id=gpu0
(qemu) device_del gpu0
(qemu) **
ERROR:qom/object.c:825:object_unref: assertion failed: (obj->ref > 0)
......
I think because of the grace period of rcu, the function memory_region_unref(as->root) in do_address_space_destroy isn't performed immediately,
The do_address_space_destroy is always performed after virtio_pci_exit, That caused 'assertion failed: (obj->ref > 0)'.
I have no idea whether my guess is correct or in correct, and don't know how to avoid this assertion failure in this situation.
Does my fix make sense ? May I have your ideas or suggestions?
Thanks!
Lin

Lin Ma

author

2015-07-21 05:38 UTC #2 lore raw

Quoted 9 lines · Michael S. Tsirkin

Quoted 7 lines · Lin Ma
Hi Michael and Gerd,
I found an hotplug/hotunplug issue about virtio 1.0 devices and trying to fix it.
The bug description is:
(qemu) device_add virtio-gpu-pci,id=gpu0
(qemu) device_del gpu0
At this point, you must wait for guest to ack device removal.

After removing the device from qmp, The DEVICE_DELETED async event was shown.
So the guest already acked, Then add a device with same id, It always reports 'Duplicate ID ...'. The device was completely removed from guest, But still in object list of qemu.

Quoted 65 lines

Quoted 64 lines
(qemu) device_add virtio-gpu-pci,id=gpu0
Duplicate ID 'gpu0' for device
Try "help device_add" for more information
My fix looks like this:
hw/virtio/virtio-pci.c +6 -0
index 283401a..098fc83 100644
@@ -1413,6 +1413,12 @@ static void
 virtio_pci_modern_region_map(VirtIOPCIProxy *proxy,
      virtio_pci_add_mem_cap(proxy, cap);
  }
 
+static void virtio_pci_modern_region_unmap(VirtIOPCIProxy *proxy,
+                                           VirtIOPCIRegion *region)
+{
+    memory_region_del_subregion(&proxy->modern_bar, &region->mr);
+}
+
  /* This is called by virtio-bus just after the device is plugged. */
  static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
  {
hw/virtio/virtio-pci.c +10 -0
@@ -1584,6 +1590,16 @@ static void virtio_pci_exit(PCIDevice *pci_dev)
 
      msix_uninit_exclusive_bar(pci_dev);
      address_space_destroy(&proxy->modern_as);
+    object_unparent(OBJECT(&proxy->modern_cfg));
+
+    bool modern = !(proxy->flags & VIRTIO_PCI_FLAG_DISABLE_MODERN);
+    if (modern) {
+        virtio_pci_modern_region_unmap(proxy, &proxy->common);
+        virtio_pci_modern_region_unmap(proxy, &proxy->isr);
+        virtio_pci_modern_region_unmap(proxy, &proxy->device);
+        virtio_pci_modern_region_unmap(proxy, &proxy->notify);
+    }
+    memory_region_unref(&proxy->modern_bar);
  }
 
  static void virtio_pci_reset(DeviceState *qdev)
But after applying the fix, I got the following errors:
(qemu) device_add virtio-gpu-pci,id=gpu0
(qemu) device_del gpu0
(qemu) **
ERROR:qom/object.c:825:object_unref: assertion failed: (obj->ref > 0)
......
I think because of the grace period of rcu, the function memory_region_unref(as->root) in do_address_space_destroy isn't performed immediately,
The do_address_space_destroy is always performed after virtio_pci_exit, That caused 'assertion failed: (obj->ref > 0)'.
I have no idea whether my guess is correct or in correct, and don't know how to avoid this assertion failure in this situation.
Does my fix make sense ? May I have your ideas or suggestions?
Thanks!
Lin