On 12/10/2018 16:21, Igor Mammedov wrote:
> On Fri, 12 Oct 2018 10:45:41 +0200
> David Hildenbrand <da...@redhat.com> wrote:
>
>>>
>>> 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).
>>
>> pre_plug -> realize -> plug
>>
>> unplug -> unrealize -> post_unplug
>>
>> doesn't look that wrong to me. But the problem seems to be that unplug
>> basically spans the whole unrealize phase (including the post_unplug
>> phase). So unplug should usually already contains the post_unplug part
>> as you noted below (when moving the object_unparent() part out).
> that just shortcut to move forward somewhere, personally I prefer having
> as less callbacks as possible, to me current unplug is pretty flexible
> we can do practically anything from it pre_unplug and post_unplug if
> necessary. Hence I don't see a reason for adding extra callbacks on top
> and for already mentioned reasons tight integration (hiding) of hotplug
> infrastructure within device_set_realized().
Yes, I agree if object_unparent() is moved out.
>
>
>>>> 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.
>>
>> I guess at some point we should definitely move them out, this only
>> leads to confusion. (e.g. hotplug handlers getting called on device
>> within device hierarchies although we don't want this to be possible)
> For that to happen we probably would need to make qdev_device_add()
> provide a friendly C API for adding a device coming not from CLI
> with its options. Right now we would need to build QemuOpts
> before manually before creating device with qdev_device_add(),
> it might be fine but I haven't really looked into it.
Yes, this might require more thought.
>
>>> 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.
>>
>> That makes sense, but we would then handle it for all PCI devices via
>> the hotplug chain I guess. (otherwise a object_unparent would be missing)
> I have an additional idea on top this, which will do a little more, see
> example:
>
> 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
> + => pci_unplug_handler():
> + object_property_set_bool(OBJECT(dev), FALSE, "realized", &err);
> object_unparent(OBJECT(dev));
> }
>
> i.e. simulate tear down by doing explicit unrealize() from unplug handler
> but don't delete device from handler. Just leave deleting it to point of
> origin of unplug event. (concrete hw endpoints that trigger it)
>
> It's still not how it should be (unrealize and tear down are still done
> as a single step), but at least we isolate it from deleting part.
> If isolating pci.unrealize() won't be sufficient, we might try to factor out
> from there minimal parts that's necessary for composite virtio device to
> work.
> (I don't insist on complete PCI unplug refactoring, so it won't block
> this series).
>
Yes, I had a similar idea in mind. First of all we need to get the
hotplug handler calls right and then think about how/where to move out
the actual PCI realization stuff. (hotplug handlers getting overwritten,
see below)
>> [...]
>>>>
>>>> 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.
>>
>> That is indeed possible as long as the memory device part has to come
>> first. I'll give it a try.
>>
>> I already started prototyping and found some other PCI hotplug handler
>> issues I have to solve first ....
> I've been recently auditing plug/unplug parts across tree so if you have
> any question regarding it feel free to ping me.
>
I guess its best to talk at KVM forum. There are plenty of things to
sort out before this can be considered clean :)
(most importantly the ACPI hotplug handler overwriting other hotplug
handlers and only registering after all devices have been coldplugged -
grml.). I have a basic prototype running, but that hotplug handler part
needs some more love.
Thanks!
--
Thanks,
David / dhildenb
