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

[Marcel Apfelbaum <marcel.a@redhat.com>]

On Mon, 2013-12-09 at 14:55 +0100, Igor Mammedov wrote:
> On Mon, 09 Dec 2013 11:09:02 +0200
> Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> 
> > On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/pci/pci.c             |   70 +++++++++++++++++++++++++++++++++------------
> > >  include/hw/pci/pci.h     |   10 ------
> > >  include/hw/pci/pci_bus.h |    2 -
> > >  3 files changed, 51 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 49eca95..26229de 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -35,6 +35,7 @@
> > >  #include "hw/pci/msi.h"
> > >  #include "hw/pci/msix.h"
> > >  #include "exec/address-spaces.h"
> > > +#include "hw/hotplug.h"
> > >  
> > >  //#define DEBUG_PCI
> > >  #ifdef DEBUG_PCI
> > > @@ -348,13 +349,6 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> > >      bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
> > >  }
> > >  
> > > -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
> > > -{
> > > -    bus->qbus.allow_hotplug = 1;
> > > -    bus->hotplug = hotplug;
> > > -    bus->hotplug_qdev = qdev;
> > > -}
> > > -
> > >  PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> > >                           pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> > >                           void *irq_opaque,
> > > @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev)
> > >  {
> > >      PCIDevice *pci_dev = (PCIDevice *)qdev;
> > >      PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> > > +    DeviceState *hotplug_dev;
> > > +    Error *local_err = NULL;
> > >      PCIBus *bus;
> > >      int rc;
> > >      bool is_default_rom;
> > > @@ -1756,32 +1752,68 @@ static int pci_qdev_init(DeviceState *qdev)
> > >      }
> > >      pci_add_option_rom(pci_dev, is_default_rom);
> > >  
> > > -    if (bus->hotplug) {
> > > -        /* Let buses differentiate between hotplug and when device is
> > > -         * enabled during qemu machine creation. */
> > > -        rc = bus->hotplug(bus->hotplug_qdev, pci_dev,
> > > -                          qdev->hotplugged ? PCI_HOTPLUG_ENABLED:
> > > -                          PCI_COLDPLUG_ENABLED);
> > > -        if (rc != 0) {
> > > -            int r = pci_unregister_device(&pci_dev->qdev);
> > > -            assert(!r);
> > > -            return rc;
> > > +    hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
> > > +                                                  &local_err));
> > > +    if (error_is_set(&local_err)) {
> > > +        goto error_exit;
> > > +    }
> > > +    if (hotplug_dev) {
> > It seems that object_property_get_link never returns NULL
> > if no error, meaning that if you go for this link
> > you *must* be sure that the link exists.
> > If the above is the case, I think you can loose the "if" statement.
> > Otherwise, if NULL is an option, you would abort the device init unnecessary. 
> Link property might exists but it might be not set. in that case get_link
> returns NULL without error set. So if nobody bothered to set link, it would mean
> skip hotplug section (it's not fail).
Got it, thanks!
Marcel

> 
> > 
> > > +        HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> > > +
> > > +        /* handler can differentiate between hotplug and when device is
> > > +         * enabled during qemu machine creation by inspecting
> > > +         * dev->hotplugged field. */
> > > +        if (hdc->hotplug) {
> > > +            hdc->hotplug(hotplug_dev, qdev, &local_err);
> > > +            if (error_is_set(&local_err)) {
> > > +                int r = pci_unregister_device(&pci_dev->qdev);
> > > +                assert(!r);
> > > +                goto error_exit;
> > > +            }
> > >          }
> > >      }
> > >      return 0;
> > > +
> > > +error_exit:
> > > +    qerror_report_err(local_err);
> > > +    error_free(local_err);
> > > +    return -1;
> > >  }
> > >  
> > >  static int pci_unplug_device(DeviceState *qdev)
> > >  {
> > >      PCIDevice *dev = PCI_DEVICE(qdev);
> > >      PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> > > +    BusState *bus = qdev_get_parent_bus(qdev);
> > > +    DeviceState *hotplug_dev;
> > > +    Error *local_err = NULL;
> > >  
> > >      if (pc->no_hotplug) {
> > >          qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev)));
> > >          return -1;
> > >      }
> > > -    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
> > > -                             PCI_HOTPLUG_DISABLED);
> > > +
> > > +    hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
> > > +                                                  &local_err));
> > > +    if (error_is_set(&local_err)) {
> > > +        goto error_exit;
> > > +    }
> > > +    if (hotplug_dev) 
> > Same as above,
> > 
> > I hope it helped,
> > Thanks,
> > Marcel 
> > 
> > > +        HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> > > +
> > > +        if (hdc->hot_unplug) {
> > > +            hdc->hot_unplug(hotplug_dev, qdev, &local_err);
> > > +            if (error_is_set(&local_err)) {
> > > +                goto error_exit;
> > > +            }
> > > +        }
> > > +    }
> > > +    return 0;
> > > +
> > > +error_exit:
> > > +    qerror_report_err(local_err);
> > > +    error_free(local_err);
> > > +    return -1;
> > >  }
> > >  
> > >  PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
> > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > index b783e68..33dec40 100644
> > > --- a/include/hw/pci/pci.h
> > > +++ b/include/hw/pci/pci.h
> > > @@ -330,15 +330,6 @@ typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
> > >  typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
> > >  typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
> > >  
> > > -typedef enum {
> > > -    PCI_HOTPLUG_DISABLED,
> > > -    PCI_HOTPLUG_ENABLED,
> > > -    PCI_COLDPLUG_ENABLED,
> > > -} PCIHotplugState;
> > > -
> > > -typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
> > > -                              PCIHotplugState state);
> > > -
> > >  #define TYPE_PCI_BUS "PCI"
> > >  #define PCI_BUS(obj) OBJECT_CHECK(PCIBus, (obj), TYPE_PCI_BUS)
> > >  #define TYPE_PCIE_BUS "PCIE"
> > > @@ -357,7 +348,6 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
> > >  void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> > >                    void *irq_opaque, int nirq);
> > >  int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
> > > -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
> > >  /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
> > >  int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
> > >  PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> > > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > > index 9df1788..fabaeee 100644
> > > --- a/include/hw/pci/pci_bus.h
> > > +++ b/include/hw/pci/pci_bus.h
> > > @@ -16,8 +16,6 @@ struct PCIBus {
> > >      pci_set_irq_fn set_irq;
> > >      pci_map_irq_fn map_irq;
> > >      pci_route_irq_fn route_intx_to_irq;
> > > -    pci_hotplug_fn hotplug;
> > > -    DeviceState *hotplug_qdev;
> > >      void *irq_opaque;
> > >      PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX];
> > >      PCIDevice *parent_dev;
> > 
> > 
> > 
>