Re: [Qemu-devel] [PATCH v4 18/24] qdev: hotplug: provide do_unplug handler

[David Hildenbrand <david@redhat.com>]

On 08/10/2018 16:12, Igor Mammedov wrote:
> On Mon, 8 Oct 2018 14:41:50 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 08/10/2018 14:19, Igor Mammedov wrote:
>>> On Mon, 8 Oct 2018 13:47:53 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>>> That way using [2] and [1 - modulo it should match only concrete type]
>>>>> machine would be able to override hotplug handlers for TYPE_VIRTIO_PMEM_PCI
>>>>> and explicitly call machine + pci hotplug handlers in necessary order.
>>>>>
>>>>> flow would look like:
>>>>>   [acpi|shcp|native pci-e eject]->  
>>>>>        hotplug_ctrl = qdev_get_hotplug_handler(dev);
>>>>>        hotplug_handler_unplug(hotplug_ctrl, dev, &local_err); ->
>>>>>             machine_unplug()
>>>>>                machine_virtio_pci_pmem_cb(): 
>>>>>                   // we now that's device has 2 stage hotplug handlers,
>>>>>                   // so we can arrange hotplug sequence in necessary order
>>>>>                   hotplug_ctrl2 = qdev_get_bus_hotplug_handler(dev);
>>>>>
>>>>>                   //then do unplug in whatever order that's correct,
>>>>>                   // I'd assume tear down/stop PCI device first, flushing
>>>>>                   // command virtio command queues and that unplug memory itself.
>>>>>                   hotplug_handler_unplug(hotplug_ctrl2, dev, &local_err);
>>>>>                   memory_device_unplug()
>>>>>     
>>>>
>>>> Looking into the details, this order is not possible. The unplug will
>>>> essentially do a device_unparent() leading to the whole hierarchy
>>>> getting destroyed. The memory_device part always has to come first.  
>>>
>>> Question here is if there are anything that should be handled first on
>>> virtio level before memory_device/pmem part is called?
>>> If there isn't it might be fine to swap the order of unplug sequence.
>>>   
>>
>> Was asking myself the same thing, but as we are effectively holding the
>> iothread lock and the guest triggered the unplug, I guess it is fine to
>> unregister the memory region at this point.
> It looks the same to me but I'm not familiar with virtio or PCI.
> I'd ask Michael if it's safe thing to do.

It is certainly cleaner to do it after the device was unrealized.

The only way I see is to provide a post_unplug handler that will be run
after unrealize(false), but before deleting the object(s).

As I already said that, the unplug/unplug_request handlers are very
different to the other handlers, as they actively delete(request to
delete) an object. In contrast to pre_plug/plug that don't create an
object but only wire it up while realizing the device.

That is the reason why we can't do stuff after calling the bus hotunplug
handler but only before it. We cannot really modify the calling order.

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 046d8f1f76..777a9486bf 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -885,6 +885,12 @@ static void device_set_realized(Object *obj, bool
value, Error **errp)
             local_errp = local_err ? NULL : &local_err;
             dc->unrealize(dev, local_errp);
         }
+
+        hotplug_ctrl = qdev_get_hotplug_handler(dev);
+        if (hotplug_ctrl) {
+            hotplug_handler_post_unplug(hotplug_ctrl, dev);
+        }
+
         dev->pending_deleted_event = true;
         DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
     }

At that point all devices are unrealized but still alive.

We can do what you imagined from there - make virtio-pmem-pci the memory
device and overwrite its hotplug handler. Call a handler chain (in case
we would have a pci post_unplug handler someday).

If we want to do something before unplug, we can use the current unplug
handler (via hotplug handler chain).

Do you have other ideas?

-- 

Thanks,

David / dhildenb