[RFC] 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:
  {
@@ -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

On Mon, Jul 20, 2015 at 11:14:58PM +0800, Lin Ma wrote:
> 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.


> (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:
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 283401a..098fc83 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -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

在 2015年07月20日 23:36, Michael S. Tsirkin 写道:
> On Mon, Jul 20, 2015 at 11:14:58PM +0800, Lin Ma wrote:
>> 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.
>
>
>> (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:
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 283401a..098fc83 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -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
>