>> Inside object_unparent(), the call flow of unrealize steps is defined. >> By moving the "real unplug" part into "do_unplug" and therefor >> essentially calling it when unrealizing, we could generalize this for >> all unplug handlers. >> I think, order of realization and therefore the order of hotplug handler >> calls is strictly defined already. Same applies to unrealization if we >> would factor the essential parts out into e.g. "do_unplug". That order >> is strictly encoded in device_set_realized() and bus_set_realized(). > > I don't see any benefits in adding do_unplug() in this case, > it would essentially replace hotplug_handler_unplug() in event origin > point with object_unparent() and renaming unplug() to do_unplug(). > > As result object_unparent() will start do more than unparenting and > destroying the device and a point where unplug originates becomes > poorly documented/lost for a reader among other object_unparent() calls. > > hotplug handlers are controller businesses and not the device one so > hiding (generalizing) it inside of device.realize() doesn't look > the right this to do, I'd rather keep these things separate as much > as possible.
As long as we find another way to make this work I am happy. What I propose here works (and in my view does not violate any concepts, it just extends hotunplug handlers to device hierarchies getting unplugged). But I also understand the potential problems you think we could have in the future. Let's see if we can make your suggestion work. > >>> If I remember right, the suggested and partially implemented idea >>> in one of your previous series was to override default hotplug >>> handler with a machine one for plugged in device [1][2]. >>> However impl. wasn't exactly what I've suggested since it matches >>> all memory-devices. >>> >>> 1) qdev: let machine hotplug handler to override bus hotplug handler >>> 2) pc: route all memory devices through the machine hotplug handler >>> >>> So lets reiterate, we have TYPE_VIRTIO_PMEM and TYPE_VIRTIO_PMEM_PCI >>> the former implements TYPE_MEMORY_DEVICE interface and the later is >>> a wrapper PCI/whatnot device shim. >>> So when you plug that composite device you'd get 2 independent >>> plug hooks called, which makes it unrelable/broken design. >> >> Can you elaborate why this is broken? I don't consider the >> realize/unrealize order broken, and that is where we plug into. But yes, >> we logically plug a device hierarchy and therefore get a separate >> hotplug handler calls. > > 1st: > > consider we have a composite device A that contains B explicitly > manged by A (un)realizefn(). Now if we use your model of independent > callbacks we have only following fixed plug flow possible: > a>realize() > ::device_set_realized() > a:realizefn() > b->realize() > ::device_set_realized() > b:realizefn() > hotplug_handler_plug(b) => b_hotplug_callback > ... > hotplug_handler_plug(a) => a_hotplug_callback > > that limits composite device wiring to external components to > the only possible order That why we have pre_plug, plug and post_plug handlers for I guess ... > 1st: b_hotplug_callback() + 2nd: a_hotplug_callback > and other way around for unplug() > > That however might not be order we need to do wiring/teardown though, > depending on composite device and controllers it might be necessary to > call callbacks in opposite order or mix both into one to do the wiring > correctly. And to do that we would need drop (move) b_hotplug_callback() > into a_hotplug_callback(), i.e. make it the way I've originally suggested. > > hotplug callback call flow might be different in child_bus case > (well, it's different in current code) and it might change in future > (ex: I'm looking into dropping hotplug_handler_post_plug() and > moving hotplug_handler_plug() to a later point to address the same > issue as commits 25e89788/8449bcf94 but without post_plug callback). .. however that sounds like a good idea, I was wondering the same why we actually need the post_plug handler. > > It's more robust for devices do not depend heavily on specific order > and define its plug sequence explicitly outside of device core. > This way it won't break apart when device core code is amended. > > 2nd: > from user point of view (and API) when composite device is created > we are dealing with 1 device (even if it's composed of many others internally, > it's not an external user business). The same should apply to hotplug > handlers. i.e. wire that device using whatever controllers are necessary > but do not jump through layers inside of device from external code > which hotplug handlers are. It somehow looks strange to have plug handler scattered all over device_set_realize(), while unplug handlers are at a completely different place and not involved in unrealize(). This makes one think that hotplugging a device hierarchy works, while unplugging does currently not work. > >>> My next question would be why TYPE_VIRTIO_PMEM_PCI can't implement >>> TYPE_MEMORY_DEVICE interface and TYPE_VIRTIO_PMEM be a simple VIRTIO >>> device without any hotplug hooks (so shim device would proxy all >>> memory-device logic to its child)? >>> /huh, then you don't need get_device_id() as well/ >> >> I had the same idea while going through different options. Then we would >> have to forward all calls directly to the child. We cannot reuse >> TYPE_MEMORY_DEVICE, so we would either need a new interface or define >> the functions we want manually for each such device. > for all I care parent device could alias out its memory-device api to a child > or just outright access/call child internal APIs/members to do the job > (that's what virtio devices often do), but that's the price to pay for > ability to change type of the end device. > >>> 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() >>> >>> Similar logic applies to device_add/device_del paths, with a difference that >>> origin point would be monitor/qmp. >> >> Let's see. User calls device_del(). That triggers an unplug_request. For >> virtio-pmem, there is nothing to do. > virtio-pmem is not a concrete device though, it's just internal thingy > inside of concrete device, the actual virtio-pmem-pci device is associated > with a pci controller that notifies guest about unplug request using > whatever hotplug method was configured for the controller (shpc/native > pci-e/acpi). > >> eject hook is called by the guest. For now we do an object_unparent. >> This would now be wrong. We would have to call a proper hotplug handler >> chain (I guess that's the refactoring you mentioned above). > it works for typical pci devices but it won't work for composite > ones that need access to controllers/resources not provided by > it's direct parent. At minimum it should be object_unparent() wrapped > into unplug() callback callback set for the pci bus if we'd look for shallow > conversion. But I haven't looked in details... I will do that during the next weeks. I'll resend the unrelated memory device changes for now. > >>> Point is to have a single explicit callback chain that applies to a concrete >>> device type. That way if ever change an ordering of calling plug callbacks >>> in qdev core, the expected for a device callback sequence would still >>> remain in place ensuring that device (un)plugged as expected. >> >> I haven't tested yet if this will work, but I can give it a try. I >> learned that in QEMU things often seem easier than they actually are :) >> >>> >>> Also it should be easier to trace for a reader, than 2 disjoint callbacks of >>> composite device (which only know to author of that device and then only >>> till he/she remembers how that thing works). >> >> In my view it makes things slightly more complicated, because you have >> to follow a hotplug handler chain that plugs devices via proxy devices. >> (e.g. passing through TYPE_MEMORY_DEVICE calls to a child, and therefore >> essentially hotplugging the child), instead of only watching out for >> which device get's hotplugged and finding exactly one hotplug handler. >> Of course, for a device hierarchy, multiple devices get logically >> hotplugged. > it's child only from internal point of view (in virtio design it's > virtio interface to share logic between different transports), > users don't really care about it. > It's job of proxy to forward data between external/internal world, > unfortunately adds more boilerplate but that's how virtio has been > designed. > As for following explicitly defined hotplug handlers chain, > it's explict and relevant parts are close to each other so it's easier > to understand what's going on, than trying to figure out implicit > callbacks sequence and how they are related. > So to summarize, you clearly favor a hotplug chain on a single device over multiple hotplug handlers for separate devices in a device hierarchy. -- Thanks, David / dhildenb
