Re: [Qemu-devel] [PATCH 9/9] hw/pci: switch to a generic hotplug handling for PCIDevice

[Michael S. Tsirkin]

On Tue, Jan 14, 2014 at 05:55:54PM +0100, Igor Mammedov wrote:
> make qdev_unplug()/device_set_realized() to call hotplug handler's
> plug/unplug methods if available and remove not needed anymore
> hot(un)plug handling from PCIDevice.
> 
> In case if hotplug handler is not available, revert to the legacy
> hotplug method.

When isn't it available?
For buses other than PCI?

> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> v2:
>  * fix test-qdev-global-props build failure during "make check"
> ---
>  hw/core/qdev.c           |   17 +++++++++++++----
>  hw/pci/pci.c             |   29 +----------------------------
>  include/hw/pci/pci.h     |   10 ----------
>  include/hw/pci/pci_bus.h |    2 --
>  tests/Makefile           |    2 +-
>  5 files changed, 15 insertions(+), 45 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index d8b83f1..3486e5d 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -213,7 +213,6 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>          error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
>          return;
>      }
> -    assert(dc->unplug != NULL);
>  
>      if (!dc->hotpluggable) {
>          error_set(errp, QERR_DEVICE_NO_HOTPLUG,
> @@ -223,9 +222,13 @@ void qdev_unplug(DeviceState *dev, Error **errp)
>  
>      qdev_hot_removed = true;
>  
> -    if (dc->unplug(dev) < 0) {
> -        error_set(errp, QERR_UNDEFINED_ERROR);
> -        return;
> +    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
> +        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
> +    } else {
> +        assert(dc->unplug != NULL);
> +        if (dc->unplug(dev) < 0) { /* legacy handler */
> +            error_set(errp, QERR_UNDEFINED_ERROR);
> +        }
>      }
>  }
>  
> @@ -720,6 +723,12 @@ static void device_set_realized(Object *obj, bool value, 
> Error **err)
>              dc->realize(dev, &local_err);
>          }
>  
> +        if (dev->parent_bus && dev->parent_bus->hotplug_handler &&
> +            local_err == NULL) {
> +            hotplug_handler_plug(dev->parent_bus->hotplug_handler,
> +                                 dev, &local_err);
> +        }
> +
>          if (qdev_get_vmsd(dev) && local_err == NULL) {
>              vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
>                                             dev->instance_id_alias,
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index b8770ef..7e36f29 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
> @@ -346,13 +347,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,
> @@ -1750,29 +1744,9 @@ 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;
> -        }
> -    }
>      return 0;
>  }
>  
> -static int pci_unplug_device(DeviceState *qdev)
> -{
> -    PCIDevice *dev = PCI_DEVICE(qdev);
> -
> -    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
> -                             PCI_HOTPLUG_DISABLED);
> -}
> -
>  PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool 
> multifunction,
>                                      const char *name)
>  {
> @@ -2243,7 +2217,6 @@ static void pci_device_class_init(ObjectClass *klass, 
> void *data)
>  {
>      DeviceClass *k = DEVICE_CLASS(klass);
>      k->init = pci_qdev_init;
> -    k->unplug = pci_unplug_device;
>      k->exit = pci_unregister_device;
>      k->bus_type = TYPE_PCI_BUS;
>      k->props = pci_props;
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 03d4bee..c709d62 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -327,15 +327,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"
> @@ -354,7 +345,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;
> diff --git a/tests/Makefile b/tests/Makefile
> index 0aaf657..0bbdecb 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -163,7 +163,7 @@ tests/test-xbzrle$(EXESUF): tests/test-xbzrle.o xbzrle.o 
> page_cache.o libqemuuti
>  tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
>  tests/test-int128$(EXESUF): tests/test-int128.o
>  tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
> -     hw/core/qdev.o hw/core/qdev-properties.o \
> +     hw/core/qdev.o hw/core/qdev-properties.o hw/core/hotplug.o\
>       hw/core/irq.o \
>       $(qom-core-obj) \
>       $(test-qapi-obj-y) \
> -- 
> 1.7.1