On Mon, 09 Dec 2013 18:18:42 +0100 Paolo Bonzini <pbonz...@redhat.com> wrote:
> Il 09/12/2013 17:48, Igor Mammedov ha scritto: > >> > > >> > We could have separate check/plug methods. Only check can fail, it must > >> > be idempotent, and it can be invoked while the device is still > >> > unrealized. > > Reasons I've stated before apply to 'check' as well, for only specific > > device knows > > when it's fine to call it. That would just replace "plug" with "check" in > > realize() > > for not much of benefit. > > Check is idempotent, and can be called before realize makes any change > (it could also be called after the device is added to > /machine/unattached, it's not a big difference). > > Plug is called after realize. PCIE case: "check" before realize will work since code that does check depends only on hotplug device (i.e. PCIE slot) and do not access not yet realized device at all. however SHPC case: check code access pci_slot that is derived from PCIDevice.devfn, which in turn could be initialized in realize() (see pci_qdev_init() devfn auto allocation). So it's not possible to call check before realize() it should be called from realize(). Perhaps other hotplug buses/devices have similar limitations, where it's not fine to access device state from outside before calling it's realize(), so it should be some post_realize() hook then to make it generic which leads to the following: if ->plug() called after realize() fails, all we need to do is to fail "realize" property setter. That should cause qdev_device_add() -> object_unparent() -> device_unparent() -> unrealize() doing all necessary cleanup. > > >> > > >> > The reason I liked the interface, is because it removes the need for > >> > each bus to add its own plug/unplug handling. > > ^^^ > > Have you meant device instead of bus? > > I meant each bus-specific abstract class (PCIDevice, SCSIDevice, etc.). > > > It's still improvement over current PCI hotplug code and allows to simplify > > other buses as well by removing callbacks from them. > > Exactly. But so far you don't get any benefit: no removal of PCI > hotplug code, no removal of allow_hotunplug. What I'm proposing, I > believe, has a small cost and already starts the transition (which I > believe we can complete for 2.0). > > > The only way to call callbacks from DEVICE.realize()/unplug(), I see, is if > > we make > > them all nofail, then it would be safe to call them in "realize" property > > setter. > > But we have at least 2 callbacks that can fail: > > pcie_cap_slot_hotplug() and shpc_device_hotplug() > > Both of them can be handled by a "check" step in the handler. > > > Goal of this series was to add and demonstrate reusable hotplug interface as > > opposed to PCI specific or SCSI-bus specific ones, so it could be used for > > memory > > hotplug as well. It might not do everything we would like but it looks like > > a move > > the right direction. > > If it's wrong direction, I could drop the idea and fallback to an original > > less intrusive approach, taking in account your comment to move type > > definitions > > into separate header. > > No, absolutely. I think it's the right direction, I just think more > aspects of it should be made generic. > > Paolo > -- Regards, Igor
