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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 11 Oct 2018 10:50:13 +0200
David Hildenbrand <david@redhat.com> wrote:

> 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.

*1

> >>>>> 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.    
> >>>

*2

> >>> 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).

The correct order should be opposite to one that created a devices,
i.e. unplug -> unrealize -> delete.
Doing unplug stuff after device was unrealized looks outright wrong
(essentially device doesn't exists anymore except memory where it's
been located).

> 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.

There is nothing special in unplug handlers wrt plug ones, they all are
external to the being created device. Theoretically we can move pre_plug
/plug from device_set_realize() to outer caller qdev_device_add() and
nothing would change.

The problem here is the lack of unplug handler for pci device so
unplugging boils down to object_unparent() which will unrealize
device (and in process unplug it) and then delete it.
What we really need is to factor out unplug code from pci device
unrealizefn(). Then ideally unplug controller could look like:
 static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
 {
+    hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    ... do some port specific unplug ...
+    hotplug_handler_do_unplug(hotplug_ctrl, dev); // default pci device unplug or pmem specific
     object_unparent(OBJECT(dev));
 }

where tear down and unrealize/delete parts are separated from each other.

> 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.
there is no device at this point, only memory where it's been located.

> 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).
making virtio-pmem-pci the memory device is orthogonal to the (un)plug
topic.

> If we want to do something before unplug, we can use the current unplug
> handler (via hotplug handler chain).
>
> Do you have other ideas?
I'd proceed with suggestions made earlier [1][2] on this thread.
That should solve the issue at hand with out factoring out PCI unplug
from old pci::unrealize(). One would have to introduce shim unplug
handlers for pci/bridge/pcie that would call object_unparent(), but
that's the extent of another shallow PCI re-factoring.
Of cause that's assuming that sequence
 1.  memory_device_unplug()
 2.  pci_unplug()
is correct in virtio-pmem-pci case.