Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 09 Dec 2013 18:18:42 +0100
Paolo Bonzini <pbonzini@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