[Qemu-devel] [PATCH 00/11 v2] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API

[Qemu-devel] [PATCH 00/11 v2] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API

[Igor Mammedov]

Refactor PCI specific hotplug API to a more generic/reusable one.
Model it after SCSI-BUS like hotplug API replacing single hotplug
callback with hotplug/hot_unplug pair of callbacks as suggested by
Paolo.
Difference between SCSI-BUS and this approach is that the former
is BUS centric while the latter is device centred. Which is evolved
from the fact that hotplug callbacks used by ACPI/SHPC/PCIE are
implemented by devices rather than by bus and bus serves only as
a proxy to forward event to hotplug device.
Memory hotplug also exposes tha same usage pattern hence an attempt
to generalize hotplug API.

Refactoring also simplifies wiring of a hotplug device with a bus,
all it needs is to set "hotplug-device" link on bus, which
would potentially allow to do it from configuration file,
there is not need to setup hotplug device callbacks on bus
synce it can get them via HOTPLUG_DEVICE API of "hotplug-device"
target.

In addition device centred hotplug API may be used by bus-less
hotplug implementations as well if it's decided to use
link<foo...> instead of bus.

Patches 8-11 are should be merged as one and are split only for
simplifying review (they compile fine but PCI hotplug is broken
until the last patch is applyed).

git tree for testing:
https://github.com/imammedo/qemu/commits/hotplug_dev_inf_v2

tested only ACPI and PCIE hotplug.

Hervé Poussineau (1):
  qom: detect bad reentrance during object_class_foreach

Igor Mammedov (9):
  define hotplug interface
  qdev: add to BusState "hotplug-handler" link
  qdev: add "hotplugable" property to Device
  hw/acpi: move typeinfo to the file end
  qdev:pci: refactor PCIDevice to use generic "hotplugable" property
  acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API
  pci/shpc: convert SHPC hotplug to use hotplug-handler API
  pci/pcie: convert PCIE hotplug to use hotplug-handler API
  hw/pci: switch to a generic hotplug handling for PCIDevice

Paolo Bonzini (1):
  qom: do not register interface "types" in the type table

 hw/acpi/piix4.c                |  151 +++++++++++++++++++++-------------------
 hw/core/Makefile.objs          |    1 +
 hw/core/hotplug.c              |   48 +++++++++++++
 hw/core/qdev.c                 |   40 ++++++++++-
 hw/display/cirrus_vga.c        |    2 +-
 hw/display/qxl.c               |    2 +-
 hw/display/vga-pci.c           |    2 +-
 hw/display/vmware_vga.c        |    2 +-
 hw/i386/acpi-build.c           |    6 +-
 hw/ide/piix.c                  |    4 +-
 hw/isa/piix4.c                 |    2 +-
 hw/pci-bridge/pci_bridge_dev.c |    9 +++
 hw/pci-host/piix.c             |    6 +-
 hw/pci/pci.c                   |   41 +----------
 hw/pci/pcie.c                  |   73 +++++++++++++-------
 hw/pci/pcie_port.c             |    8 ++
 hw/pci/shpc.c                  |  133 +++++++++++++++++++++++------------
 hw/usb/hcd-ehci-pci.c          |    2 +-
 hw/usb/hcd-ohci.c              |    2 +-
 hw/usb/hcd-uhci.c              |    2 +-
 hw/usb/hcd-xhci.c              |    2 +-
 include/hw/hotplug.h           |   75 ++++++++++++++++++++
 include/hw/pci/pci.h           |   13 ----
 include/hw/pci/pci_bus.h       |    2 -
 include/hw/pci/pcie.h          |    5 ++
 include/hw/pci/shpc.h          |    8 ++
 include/hw/qdev-core.h         |    8 ++
 qom/object.c                   |   17 ++++-
 28 files changed, 446 insertions(+), 220 deletions(-)
 create mode 100644 hw/core/hotplug.c
 create mode 100644 include/hw/hotplug.h

[Qemu-devel] [PATCH 01/11] qom: do not register interface "types" in the type table

[Igor Mammedov]

From: Paolo Bonzini <pbonzini@redhat.com>

There should be no need to look them up nor enumerate the interface
"types", whose "classes" are really just vtables.  Just create the
types and add them to the interface list of the parent type.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 qom/object.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index fc19cf6..3a43186 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -88,7 +88,7 @@ static TypeImpl *type_table_lookup(const char *name)
     return g_hash_table_lookup(type_table_get(), name);
 }
 
-static TypeImpl *type_register_internal(const TypeInfo *info)
+static TypeImpl *type_new(const TypeInfo *info)
 {
     TypeImpl *ti = g_malloc0(sizeof(*ti));
     int i;
@@ -122,8 +122,15 @@ static TypeImpl *type_register_internal(const TypeInfo *info)
     }
     ti->num_interfaces = i;
 
-    type_table_add(ti);
+    return ti;
+}
 
+static TypeImpl *type_register_internal(const TypeInfo *info)
+{
+    TypeImpl *ti;
+    ti = type_new(info);
+
+    type_table_add(ti);
     return ti;
 }
 
@@ -216,7 +223,7 @@ static void type_initialize_interface(TypeImpl *ti, const char *parent)
     info.name = g_strdup_printf("%s::%s", ti->name, info.parent);
     info.abstract = true;
 
-    iface_impl = type_register(&info);
+    iface_impl = type_new(&info);
     type_initialize(iface_impl);
     g_free((char *)info.name);
 
-- 
1.7.1

[Qemu-devel] [PATCH 02/11] qom: detect bad reentrance during object_class_foreach

[Igor Mammedov]

From: Hervé Poussineau <hpoussin@reactos.org>

We should not modify the type hash table while it is being iterated on.
Assert that it does not happen.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * make ver more descriptinve s/enumerating/enumerating_classes/
    [asked-by: Peter Crosthwaite]
---
 qom/object.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 3a43186..4a0fb86 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -78,8 +78,10 @@ static GHashTable *type_table_get(void)
     return type_table;
 }
 
+static bool enumerating_classes = false;
 static void type_table_add(TypeImpl *ti)
 {
+    assert(!enumerating_classes);
     g_hash_table_insert(type_table_get(), (void *)ti->name, ti);
 }
 
@@ -666,7 +668,9 @@ void object_class_foreach(void (*fn)(ObjectClass *klass, void *opaque),
 {
     OCFData data = { fn, implements_type, include_abstract, opaque };
 
+    enumerating_classes = true;
     g_hash_table_foreach(type_table_get(), object_class_foreach_tramp, &data);
+    enumerating_classes = false;
 }
 
 int object_child_foreach(Object *obj, int (*fn)(Object *child, void *opaque),
-- 
1.7.1

[Qemu-devel] [PATCH 03/11] define hotplug interface

[Igor Mammedov]

Provide generic hotplug interface for devices.
Intended for replacing hotplug mechanism used by
PCI/PCIE/SHPC code and will be used for memory hotplug.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
 * s/device/handler/
 * add hotplug_handler_plug/hotplug_handler_unplug API
v1:
it's scsi-bus like interface, but abstracted from bus altogether
since all current users care about in hotplug handlers, it's
hotplug device and hotplugged device and bus only serves
as a means to get access to hotplug device and it's callbacks.
---
 hw/core/Makefile.objs |    1 +
 hw/core/hotplug.c     |   48 +++++++++++++++++++++++++++++++
 include/hw/hotplug.h  |   75 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+), 0 deletions(-)
 create mode 100644 hw/core/hotplug.c
 create mode 100644 include/hw/hotplug.h

diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 950146c..9e324be 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -2,6 +2,7 @@
 common-obj-y += qdev.o qdev-properties.o
 # irq.o needed for qdev GPIO handling:
 common-obj-y += irq.o
+common-obj-y += hotplug.o
 
 common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
 common-obj-$(CONFIG_XILINX_AXI) += stream.o
diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
new file mode 100644
index 0000000..5c3b5c9
--- /dev/null
+++ b/hw/core/hotplug.c
@@ -0,0 +1,48 @@
+/*
+ * Hotplug handler interface.
+ *
+ * Copyright (c) 2013 Red Hat Inc.
+ *
+ * Authors:
+ *  Igor Mammedov <imammedo@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "hw/hotplug.h"
+#include "qemu/module.h"
+
+void hotplug_handler_plug(HotplugHandler *plug_handler,
+                          DeviceState *plugged_dev,
+                          Error **errp)
+{
+    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
+
+    if (hdc->plug) {
+        hdc->plug(plug_handler, plugged_dev, errp);
+    }
+}
+
+void hotplug_handler_unplug(HotplugHandler *plug_handler,
+                            DeviceState *plugged_dev,
+                            Error **errp)
+{
+    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
+
+    if (hdc->unplug) {
+        hdc->unplug(plug_handler, plugged_dev, errp);
+    }
+}
+
+static const TypeInfo hotplug_handler_info = {
+    .name          = TYPE_HOTPLUG_HANDLER,
+    .parent        = TYPE_INTERFACE,
+    .class_size = sizeof(HotplugHandlerClass),
+};
+
+static void hotplug_handler_register_types(void)
+{
+    type_register_static(&hotplug_handler_info);
+}
+
+type_init(hotplug_handler_register_types)
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
new file mode 100644
index 0000000..64ae6f7
--- /dev/null
+++ b/include/hw/hotplug.h
@@ -0,0 +1,75 @@
+/*
+ * Hotplug handler interface.
+ *
+ * Copyright (c) 2013 Red Hat Inc.
+ *
+ * Authors:
+ *  Igor Mammedov <imammedo@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef HOTPLUG_H
+#define HOTPLUG_H
+
+#include "qom/object.h"
+#include "qemu/typedefs.h"
+
+#define TYPE_HOTPLUG_HANDLER "hotplug-handler"
+
+#define HOTPLUG_HANDLER_CLASS(klass) \
+     OBJECT_CLASS_CHECK(HotplugHandlerClass, (klass), TYPE_HOTPLUG_HANDLER)
+#define HOTPLUG_HANDLER_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(HotplugHandlerClass, (obj), TYPE_HOTPLUG_HANDLER)
+#define HOTPLUG_HANDLER(obj) \
+     INTERFACE_CHECK(HotplugHandler, (obj), TYPE_HOTPLUG_HANDLER)
+
+
+typedef struct HotplugHandler {
+    Object Parent;
+} HotplugHandler;
+
+/**
+ * hotplug_fn:
+ * @plug_handler: a device performing plug/uplug action
+ * @plugged_dev: a device that has been (un)plugged
+ * @errp: returns an error if this function fails
+ */
+typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
+                           DeviceState *plugged_dev, Error **errp);
+
+/**
+ * HotplugDeviceClass:
+ *
+ * Interface to be implemented by a device performing
+ * hardware (un)plug functions.
+ *
+ * @parent: Opaque parent interface.
+ * @plug: plug callback.
+ * @unplug: unplug callback.
+ */
+typedef struct HotplugHandlerClass {
+    InterfaceClass parent;
+
+    hotplug_fn plug;
+    hotplug_fn unplug;
+} HotplugHandlerClass;
+
+/**
+ * hotplug_handler_plug:
+ *
+ * Call #HotplugHandlerClass.plug callback of @plug_handler.
+ */
+void hotplug_handler_plug(HotplugHandler *plug_handler,
+                          DeviceState *plugged_dev,
+                          Error **errp);
+
+/**
+ * hotplug_handler_unplug:
+ *
+ * Call #HotplugHandlerClass.unplug callback of @plug_handler.
+ */
+void hotplug_handler_unplug(HotplugHandler *plug_handler,
+                            DeviceState *plugged_dev,
+                            Error **errp);
+#endif
-- 
1.7.1

[Qemu-devel] [PATCH 04/11] qdev: add to BusState "hotplug-handler" link

[Igor Mammedov]

It will allow to reuse field with different BUSes, reducing code duplication.
Field is intended fot replacing 'hotplug_qdev' field in PCIBus and also
will allow to avoid adding equivalent field to DimmBus with possiblitity
to refactor other BUSes to use it instead of custom field.
In addition once all users of allow_hotplug field are converted to new
API, link could replace allow_hotplug in qdev hotplug code.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/qdev.c         |    4 ++++
 include/hw/qdev-core.h |    5 +++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index e374a93..25c2d2c 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -32,6 +32,7 @@
 #include "qapi/visitor.h"
 #include "qapi/qmp/qjson.h"
 #include "monitor/monitor.h"
+#include "hw/hotplug.h"
 
 int qdev_hotplug = 0;
 static bool qdev_hot_added = false;
@@ -868,6 +869,9 @@ static void qbus_initfn(Object *obj)
     BusState *bus = BUS(obj);
 
     QTAILQ_INIT(&bus->children);
+    object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY,
+                             TYPE_HOTPLUG_HANDLER,
+                             (Object **)&bus->hotplug_handler, NULL);
 }
 
 static char *default_bus_get_fw_dev_path(DeviceState *dev)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index f2043a6..684a5da 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -8,6 +8,7 @@
 #include "qom/object.h"
 #include "hw/irq.h"
 #include "qapi/error.h"
+#include "hw/hotplug.h"
 
 enum {
     DEV_NVECTORS_UNSPECIFIED = -1,
@@ -169,14 +170,18 @@ typedef struct BusChild {
     QTAILQ_ENTRY(BusChild) sibling;
 } BusChild;
 
+#define QDEV_HOTPLUG_HANDLER_PROPERTY "hotplug-handler"
+
 /**
  * BusState:
+ * @hotplug_device: link to a hotplug device associated with bus.
  */
 struct BusState {
     Object obj;
     DeviceState *parent;
     const char *name;
     int allow_hotplug;
+    HotplugHandler *hotplug_handler;
     int max_index;
     QTAILQ_HEAD(ChildrenHead, BusChild) children;
     QLIST_ENTRY(BusState) sibling;
-- 
1.7.1

[Qemu-devel] [PATCH 05/11] qdev: add "hotplugable" property to Device

[Igor Mammedov]

Currently it's possible to make PCIDevice not hotplugable by using
no_hotplug field of PCIDeviceClass. However it limits this
only to PCI devices and prevents from generalizing hotplug code.

So add similar field to DeviceClass so it could be reused with other
Devices and would allow to replace PCI specific hotplug callbacks
with generic implementation.

In addition expose field as "hotplugable" readonly property, to make
it possible to get it via QOM interface.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/qdev.c         |    8 ++++++++
 include/hw/qdev-core.h |    3 +++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 25c2d2c..1b1a684 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -719,6 +719,12 @@ static void device_set_realized(Object *obj, bool value, Error **err)
     dev->realized = value;
 }
 
+static bool device_get_hotplugable(Object *obj, Error **err)
+{
+    DeviceClass *dc = DEVICE_GET_CLASS(obj);
+    return dc->hotplugable;
+}
+
 static void device_initfn(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
@@ -736,6 +742,8 @@ static void device_initfn(Object *obj)
 
     object_property_add_bool(obj, "realized",
                              device_get_realized, device_set_realized, NULL);
+    object_property_add_bool(obj, "hotplugable",
+                             device_get_hotplugable, NULL, NULL);
 
     class = object_get_class(OBJECT(dev));
     do {
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 684a5da..5d4a9c8 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -50,6 +50,8 @@ struct VMStateDescription;
  * is changed to %true. Deprecated, new types inheriting directly from
  * TYPE_DEVICE should use @realize instead, new leaf types should consult
  * their respective parent type.
+ * @hotplugable: booleean indicating if #DeviceClass is hotplugable, available
+ * as readonly "hotplugable" property of #DeviceState instance
  *
  * # Realization #
  * Devices are constructed in two stages,
@@ -99,6 +101,7 @@ typedef struct DeviceClass {
     const char *desc;
     Property *props;
     int no_user;
+    bool hotplugable;
 
     /* callbacks */
     void (*reset)(DeviceState *dev);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 05/11] qdev: add "hotplugable" property to Device

[Paolo Bonzini]

Il 11/12/2013 02:01, Igor Mammedov ha scritto:
> Currently it's possible to make PCIDevice not hotplugable by using
> no_hotplug field of PCIDeviceClass. However it limits this
> only to PCI devices and prevents from generalizing hotplug code.
> 
> So add similar field to DeviceClass so it could be reused with other
> Devices and would allow to replace PCI specific hotplug callbacks
> with generic implementation.
> 
> In addition expose field as "hotplugable" readonly property, to make
> it possible to get it via QOM interface.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/core/qdev.c         |    8 ++++++++
>  include/hw/qdev-core.h |    3 +++
>  2 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 25c2d2c..1b1a684 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -719,6 +719,12 @@ static void device_set_realized(Object *obj, bool value, Error **err)
>      dev->realized = value;
>  }
>  
> +static bool device_get_hotplugable(Object *obj, Error **err)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> +    return dc->hotplugable;
> +}
> +
>  static void device_initfn(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -736,6 +742,8 @@ static void device_initfn(Object *obj)
>  
>      object_property_add_bool(obj, "realized",
>                               device_get_realized, device_set_realized, NULL);
> +    object_property_add_bool(obj, "hotplugable",
> +                             device_get_hotplugable, NULL, NULL);
>  
>      class = object_get_class(OBJECT(dev));
>      do {
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 684a5da..5d4a9c8 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -50,6 +50,8 @@ struct VMStateDescription;
>   * is changed to %true. Deprecated, new types inheriting directly from
>   * TYPE_DEVICE should use @realize instead, new leaf types should consult
>   * their respective parent type.
> + * @hotplugable: booleean indicating if #DeviceClass is hotplugable, available
> + * as readonly "hotplugable" property of #DeviceState instance
>   *
>   * # Realization #
>   * Devices are constructed in two stages,
> @@ -99,6 +101,7 @@ typedef struct DeviceClass {
>      const char *desc;
>      Property *props;
>      int no_user;
> +    bool hotplugable;
>  
>      /* callbacks */
>      void (*reset)(DeviceState *dev);
> 

Does this patch break SCSI and virtio-serial hotplug?

Perhaps you could initialize dc->hotpluggable to true, and return
"dc->hotplugable && device->parent_bus->allow_hotplug" from the property
getter?  Or just set the class to true in the common superclasses of all
buses that support hotplug, but that's a bit harder.

Paolo

Re: [Qemu-devel] [PATCH 05/11] qdev: add "hotplugable" property to Device

[Igor Mammedov]

On Wed, 11 Dec 2013 17:29:06 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 11/12/2013 02:01, Igor Mammedov ha scritto:
> > Currently it's possible to make PCIDevice not hotplugable by using
> > no_hotplug field of PCIDeviceClass. However it limits this
> > only to PCI devices and prevents from generalizing hotplug code.
> > 
> > So add similar field to DeviceClass so it could be reused with other
> > Devices and would allow to replace PCI specific hotplug callbacks
> > with generic implementation.
> > 
> > In addition expose field as "hotplugable" readonly property, to make
> > it possible to get it via QOM interface.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/core/qdev.c         |    8 ++++++++
> >  include/hw/qdev-core.h |    3 +++
> >  2 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 25c2d2c..1b1a684 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -719,6 +719,12 @@ static void device_set_realized(Object *obj, bool value, Error **err)
> >      dev->realized = value;
> >  }
> >  
> > +static bool device_get_hotplugable(Object *obj, Error **err)
> > +{
> > +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> > +    return dc->hotplugable;
> > +}
> > +
> >  static void device_initfn(Object *obj)
> >  {
> >      DeviceState *dev = DEVICE(obj);
> > @@ -736,6 +742,8 @@ static void device_initfn(Object *obj)
> >  
> >      object_property_add_bool(obj, "realized",
> >                               device_get_realized, device_set_realized, NULL);
> > +    object_property_add_bool(obj, "hotplugable",
> > +                             device_get_hotplugable, NULL, NULL);
> >  
> >      class = object_get_class(OBJECT(dev));
> >      do {
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 684a5da..5d4a9c8 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -50,6 +50,8 @@ struct VMStateDescription;
> >   * is changed to %true. Deprecated, new types inheriting directly from
> >   * TYPE_DEVICE should use @realize instead, new leaf types should consult
> >   * their respective parent type.
> > + * @hotplugable: booleean indicating if #DeviceClass is hotplugable, available
> > + * as readonly "hotplugable" property of #DeviceState instance
> >   *
> >   * # Realization #
> >   * Devices are constructed in two stages,
> > @@ -99,6 +101,7 @@ typedef struct DeviceClass {
> >      const char *desc;
> >      Property *props;
> >      int no_user;
> > +    bool hotplugable;
> >  
> >      /* callbacks */
> >      void (*reset)(DeviceState *dev);
> > 
> 
> Does this patch break SCSI and virtio-serial hotplug?
Ouch,
this  patch doesn't but 
"[PATCH 07/11] qdev:pci: refactor PCIDevice to use generic "hotplugable"
property" does. See 2 hunks in hw/core/qdev.c

> 
> Perhaps you could initialize dc->hotpluggable to true, and return
that's probably the easiest way, since before series all devices were
considered hotplugable from qdev POV, so it makes sense to make DeviceClass
hotplugable by default to avoid unexpected regressions.

> "dc->hotplugable && device->parent_bus->allow_hotplug" from the property
> getter? 
sure, since there is no bus-less hotlpug it won't hurt.
I'll respin amended patch as reply to this thread if it's ok.

> Or just set the class to true in the common superclasses of all
> buses that support hotplug, but that's a bit harder.
perhaps exercise for the later.

> 
> Paolo


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 05/11] qdev: add "hotplugable" property to Device

[Paolo Bonzini]

Il 11/12/2013 17:57, Igor Mammedov ha scritto:
> On Wed, 11 Dec 2013 17:29:06 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 11/12/2013 02:01, Igor Mammedov ha scritto:
>>> Currently it's possible to make PCIDevice not hotplugable by using
>>> no_hotplug field of PCIDeviceClass. However it limits this
>>> only to PCI devices and prevents from generalizing hotplug code.
>>>
>>> So add similar field to DeviceClass so it could be reused with other
>>> Devices and would allow to replace PCI specific hotplug callbacks
>>> with generic implementation.
>>>
>>> In addition expose field as "hotplugable" readonly property, to make
>>> it possible to get it via QOM interface.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>>  hw/core/qdev.c         |    8 ++++++++
>>>  include/hw/qdev-core.h |    3 +++
>>>  2 files changed, 11 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 25c2d2c..1b1a684 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -719,6 +719,12 @@ static void device_set_realized(Object *obj, bool value, Error **err)
>>>      dev->realized = value;
>>>  }
>>>  
>>> +static bool device_get_hotplugable(Object *obj, Error **err)
>>> +{
>>> +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
>>> +    return dc->hotplugable;
>>> +}
>>> +
>>>  static void device_initfn(Object *obj)
>>>  {
>>>      DeviceState *dev = DEVICE(obj);
>>> @@ -736,6 +742,8 @@ static void device_initfn(Object *obj)
>>>  
>>>      object_property_add_bool(obj, "realized",
>>>                               device_get_realized, device_set_realized, NULL);
>>> +    object_property_add_bool(obj, "hotplugable",
>>> +                             device_get_hotplugable, NULL, NULL);
>>>  
>>>      class = object_get_class(OBJECT(dev));
>>>      do {
>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>>> index 684a5da..5d4a9c8 100644
>>> --- a/include/hw/qdev-core.h
>>> +++ b/include/hw/qdev-core.h
>>> @@ -50,6 +50,8 @@ struct VMStateDescription;
>>>   * is changed to %true. Deprecated, new types inheriting directly from
>>>   * TYPE_DEVICE should use @realize instead, new leaf types should consult
>>>   * their respective parent type.
>>> + * @hotplugable: booleean indicating if #DeviceClass is hotplugable, available
>>> + * as readonly "hotplugable" property of #DeviceState instance
>>>   *
>>>   * # Realization #
>>>   * Devices are constructed in two stages,
>>> @@ -99,6 +101,7 @@ typedef struct DeviceClass {
>>>      const char *desc;
>>>      Property *props;
>>>      int no_user;
>>> +    bool hotplugable;
>>>  
>>>      /* callbacks */
>>>      void (*reset)(DeviceState *dev);
>>>
>>
>> Does this patch break SCSI and virtio-serial hotplug?
> Ouch,
> this  patch doesn't but 
> "[PATCH 07/11] qdev:pci: refactor PCIDevice to use generic "hotplugable"
> property" does. See 2 hunks in hw/core/qdev.c

If you set dc->hotpluggable = true, those two hunks could also be moved
here.

Paolo

>>
>> Perhaps you could initialize dc->hotpluggable to true, and return
> that's probably the easiest way, since before series all devices were
> considered hotplugable from qdev POV, so it makes sense to make DeviceClass
> hotplugable by default to avoid unexpected regressions.
> 
>> "dc->hotplugable && device->parent_bus->allow_hotplug" from the property
>> getter? 
> sure, since there is no bus-less hotlpug it won't hurt.
> I'll respin amended patch as reply to this thread if it's ok.
> 
>> Or just set the class to true in the common superclasses of all
>> buses that support hotplug, but that's a bit harder.
> perhaps exercise for the later.
> 
>>
>> Paolo
> 
> 

Re: [Qemu-devel] [PATCH 05/11] qdev: add "hotplugable" property to Device

[Igor Mammedov]

On Wed, 11 Dec 2013 18:02:11 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 11/12/2013 17:57, Igor Mammedov ha scritto:
> > On Wed, 11 Dec 2013 17:29:06 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> Il 11/12/2013 02:01, Igor Mammedov ha scritto:
> >>> Currently it's possible to make PCIDevice not hotplugable by using
> >>> no_hotplug field of PCIDeviceClass. However it limits this
> >>> only to PCI devices and prevents from generalizing hotplug code.
> >>>
> >>> So add similar field to DeviceClass so it could be reused with other
> >>> Devices and would allow to replace PCI specific hotplug callbacks
> >>> with generic implementation.
> >>>
> >>> In addition expose field as "hotplugable" readonly property, to make
> >>> it possible to get it via QOM interface.
> >>>
> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>> ---
> >>>  hw/core/qdev.c         |    8 ++++++++
> >>>  include/hw/qdev-core.h |    3 +++
> >>>  2 files changed, 11 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >>> index 25c2d2c..1b1a684 100644
> >>> --- a/hw/core/qdev.c
> >>> +++ b/hw/core/qdev.c
> >>> @@ -719,6 +719,12 @@ static void device_set_realized(Object *obj, bool value, Error **err)
> >>>      dev->realized = value;
> >>>  }
> >>>  
> >>> +static bool device_get_hotplugable(Object *obj, Error **err)
> >>> +{
> >>> +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> >>> +    return dc->hotplugable;
> >>> +}
> >>> +
> >>>  static void device_initfn(Object *obj)
> >>>  {
> >>>      DeviceState *dev = DEVICE(obj);
> >>> @@ -736,6 +742,8 @@ static void device_initfn(Object *obj)
> >>>  
> >>>      object_property_add_bool(obj, "realized",
> >>>                               device_get_realized, device_set_realized, NULL);
> >>> +    object_property_add_bool(obj, "hotplugable",
> >>> +                             device_get_hotplugable, NULL, NULL);
> >>>  
> >>>      class = object_get_class(OBJECT(dev));
> >>>      do {
> >>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> >>> index 684a5da..5d4a9c8 100644
> >>> --- a/include/hw/qdev-core.h
> >>> +++ b/include/hw/qdev-core.h
> >>> @@ -50,6 +50,8 @@ struct VMStateDescription;
> >>>   * is changed to %true. Deprecated, new types inheriting directly from
> >>>   * TYPE_DEVICE should use @realize instead, new leaf types should consult
> >>>   * their respective parent type.
> >>> + * @hotplugable: booleean indicating if #DeviceClass is hotplugable, available
> >>> + * as readonly "hotplugable" property of #DeviceState instance
> >>>   *
> >>>   * # Realization #
> >>>   * Devices are constructed in two stages,
> >>> @@ -99,6 +101,7 @@ typedef struct DeviceClass {
> >>>      const char *desc;
> >>>      Property *props;
> >>>      int no_user;
> >>> +    bool hotplugable;
> >>>  
> >>>      /* callbacks */
> >>>      void (*reset)(DeviceState *dev);
> >>>
> >>
> >> Does this patch break SCSI and virtio-serial hotplug?
> > Ouch,
> > this  patch doesn't but 
> > "[PATCH 07/11] qdev:pci: refactor PCIDevice to use generic "hotplugable"
> > property" does. See 2 hunks in hw/core/qdev.c
> 
> If you set dc->hotpluggable = true, those two hunks could also be moved
> here.
sure. posting as reply to this thread so not to spam list with series respin.
tested with CPU hotplug which was broken too.

> 
> Paolo
> 
> >>
> >> Perhaps you could initialize dc->hotpluggable to true, and return
> > that's probably the easiest way, since before series all devices were
> > considered hotplugable from qdev POV, so it makes sense to make DeviceClass
> > hotplugable by default to avoid unexpected regressions.
> > 
> >> "dc->hotplugable && device->parent_bus->allow_hotplug" from the property
> >> getter? 
> > sure, since there is no bus-less hotlpug it won't hurt.
> > I'll respin amended patch as reply to this thread if it's ok.
> > 
> >> Or just set the class to true in the common superclasses of all
> >> buses that support hotplug, but that's a bit harder.
> > perhaps exercise for the later.
> > 
> >>
> >> Paolo
> > 
> > 
> 


-- 
Regards,
  Igor

[Qemu-devel] [PATCH 5/11 v3 FIXED] qdev: add "hotplugable" property to Device

[Igor Mammedov]

Currently it's possible to make PCIDevice not hotplugable by using
no_hotplug field of PCIDeviceClass. However it limits this
only to PCI devices and prevents from generalizing hotplug code.

So add similar field to DeviceClass so it could be reused with other
Devices and would allow to replace PCI specific hotplug callbacks
with generic implementation.

In addition expose field as "hotplugable" readonly property, to make
it possible to get it via QOM interface.

v3:
make DeviceClass hotlugable by default. Since PCIDevice still uses
internal checks it shouldn't reggress. And follow up patch that
converts PCIDevices to use "hotplugable" property will take care
about not hotplugable PCI devices explicitly setting "hotplugable"
to false in their class_init().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
 move generic hotplug checks from
 "7/11 qdev:pci: refactor PCIDevice to use generic "hotplugable" property"
 to this patch
---
 hw/core/qdev.c         | 29 +++++++++++++++++++++++++++++
 include/hw/qdev-core.h |  3 +++
 2 files changed, 32 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 25c2d2c..21c4522 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -215,6 +215,12 @@ void qdev_unplug(DeviceState *dev, Error **errp)
     }
     assert(dc->unplug != NULL);
 
+    if (!dc->hotplugable) {
+        error_set(errp, QERR_DEVICE_NO_HOTPLUG,
+                  object_get_typename(OBJECT(dev)));
+        return;
+    }
+
     qdev_hot_removed = true;
 
     if (dc->unplug(dev) < 0) {
@@ -679,6 +685,11 @@ static void device_set_realized(Object *obj, bool value, Error **err)
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
     Error *local_err = NULL;
 
+    if (dev->hotplugged && !dc->hotplugable) {
+        error_set(err, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
+        return;
+    }
+
     if (value && !dev->realized) {
         if (!obj->parent && local_err == NULL) {
             static int unattached_count;
@@ -719,6 +730,14 @@ static void device_set_realized(Object *obj, bool value, Error **err)
     dev->realized = value;
 }
 
+static bool device_get_hotplugable(Object *obj, Error **err)
+{
+    DeviceClass *dc = DEVICE_GET_CLASS(obj);
+    DeviceState *dev = DEVICE(obj);
+
+    return dc->hotplugable && dev->parent_bus->allow_hotplug;
+}
+
 static void device_initfn(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
@@ -736,6 +755,8 @@ static void device_initfn(Object *obj)
 
     object_property_add_bool(obj, "realized",
                              device_get_realized, device_set_realized, NULL);
+    object_property_add_bool(obj, "hotplugable",
+                             device_get_hotplugable, NULL, NULL);
 
     class = object_get_class(OBJECT(dev));
     do {
@@ -784,6 +805,14 @@ static void device_class_base_init(ObjectClass *class, void *data)
      * so do not propagate them to the subclasses.
      */
     klass->props = NULL;
+
+    /* by default all devices were considered as hotplugable,
+     * so with intent to check it in generic qdev_unplug() /
+     * device_set_realized() functions make every device
+     * hotplugable. Devices that shouldn't be hoplugable,
+     * should override it in their class_init()
+     */
+    klass->hotplugable = true;
 }
 
 static void device_unparent(Object *obj)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 684a5da..5d4a9c8 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -50,6 +50,8 @@ struct VMStateDescription;
  * is changed to %true. Deprecated, new types inheriting directly from
  * TYPE_DEVICE should use @realize instead, new leaf types should consult
  * their respective parent type.
+ * @hotplugable: booleean indicating if #DeviceClass is hotplugable, available
+ * as readonly "hotplugable" property of #DeviceState instance
  *
  * # Realization #
  * Devices are constructed in two stages,
@@ -99,6 +101,7 @@ typedef struct DeviceClass {
     const char *desc;
     Property *props;
     int no_user;
+    bool hotplugable;
 
     /* callbacks */
     void (*reset)(DeviceState *dev);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 5/11 v3 FIXED] qdev: add "hotplugable" property to Device

[Markus Armbruster]

Please spell it "pluggable", both in C identifiers and strings.

Re: [Qemu-devel] [PATCH 5/11 v3 FIXED] qdev: add "hotplugable" property to Device

[Igor Mammedov]

On Wed, 11 Dec 2013 20:57:43 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Please spell it "pluggable", both in C identifiers and strings.

Sure,
I'll respin series since it will touch several patches.

[Qemu-devel] [PATCH 06/11] hw/acpi: move typeinfo to the file end

[Igor Mammedov]

do so to avoid not necessary forward declarations and
place typeinfo registration at the file end where it's
usualy expected.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/piix4.c |   80 +++++++++++++++++++++++++++---------------------------
 1 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 93849c8..8084a60 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -523,46 +523,6 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
     return s->smb.smbus;
 }
 
-static Property piix4_pm_properties[] = {
-    DEFINE_PROP_UINT32("smb_io_base", PIIX4PMState, smb_io_base, 0),
-    DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0),
-    DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 0),
-    DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
-static void piix4_pm_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->no_hotplug = 1;
-    k->init = piix4_pm_initfn;
-    k->config_write = pm_write_config;
-    k->vendor_id = PCI_VENDOR_ID_INTEL;
-    k->device_id = PCI_DEVICE_ID_INTEL_82371AB_3;
-    k->revision = 0x03;
-    k->class_id = PCI_CLASS_BRIDGE_OTHER;
-    dc->desc = "PM";
-    dc->no_user = 1;
-    dc->vmsd = &vmstate_acpi;
-    dc->props = piix4_pm_properties;
-}
-
-static const TypeInfo piix4_pm_info = {
-    .name          = TYPE_PIIX4_PM,
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(PIIX4PMState),
-    .class_init    = piix4_pm_class_init,
-};
-
-static void piix4_pm_register_types(void)
-{
-    type_register_static(&piix4_pm_info);
-}
-
-type_init(piix4_pm_register_types)
-
 static uint64_t gpe_readb(void *opaque, hwaddr addr, unsigned width)
 {
     PIIX4PMState *s = opaque;
@@ -772,3 +732,43 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
 
     return 0;
 }
+
+static Property piix4_pm_properties[] = {
+    DEFINE_PROP_UINT32("smb_io_base", PIIX4PMState, smb_io_base, 0),
+    DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0),
+    DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 0),
+    DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void piix4_pm_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->no_hotplug = 1;
+    k->init = piix4_pm_initfn;
+    k->config_write = pm_write_config;
+    k->vendor_id = PCI_VENDOR_ID_INTEL;
+    k->device_id = PCI_DEVICE_ID_INTEL_82371AB_3;
+    k->revision = 0x03;
+    k->class_id = PCI_CLASS_BRIDGE_OTHER;
+    dc->desc = "PM";
+    dc->no_user = 1;
+    dc->vmsd = &vmstate_acpi;
+    dc->props = piix4_pm_properties;
+}
+
+static const TypeInfo piix4_pm_info = {
+    .name          = TYPE_PIIX4_PM,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PIIX4PMState),
+    .class_init    = piix4_pm_class_init,
+};
+
+static void piix4_pm_register_types(void)
+{
+    type_register_static(&piix4_pm_info);
+}
+
+type_init(piix4_pm_register_types)
-- 
1.7.1

[Qemu-devel] [PATCH 07/11] qdev:pci: refactor PCIDevice to use generic "hotplugable" property

[Igor Mammedov]

Get rid of PCIDevice specific PCIDeviceClass.no_hotplug and use
generic DeviceClass.hotplugable field instead.

Check if hot(un)plug is allowed into common qdev_unplug/device_set_realized,
so that inherited classes won't have to do it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/piix4.c         |   10 +++++-----
 hw/core/qdev.c          |   11 +++++++++++
 hw/display/cirrus_vga.c |    2 +-
 hw/display/qxl.c        |    2 +-
 hw/display/vga-pci.c    |    2 +-
 hw/display/vmware_vga.c |    2 +-
 hw/i386/acpi-build.c    |    6 +++---
 hw/ide/piix.c           |    4 ++--
 hw/isa/piix4.c          |    2 +-
 hw/pci-host/piix.c      |    6 +++---
 hw/pci/pci.c            |   12 ++----------
 hw/usb/hcd-ehci-pci.c   |    2 +-
 hw/usb/hcd-ohci.c       |    2 +-
 hw/usb/hcd-uhci.c       |    2 +-
 hw/usb/hcd-xhci.c       |    2 +-
 include/hw/pci/pci.h    |    3 ---
 16 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 8084a60..8ccc70b 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -323,9 +323,9 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
     QTAILQ_FOREACH_SAFE(kid, &bus->children, sibling, next) {
         DeviceState *qdev = kid->child;
         PCIDevice *dev = PCI_DEVICE(qdev);
-        PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
+        DeviceClass *dc = DEVICE_GET_CLASS(dev);
         if (PCI_SLOT(dev->devfn) == slot) {
-            if (pc->no_hotplug) {
+            if (!dc->hotplugable) {
                 slot_free = false;
             } else {
                 object_unparent(OBJECT(qdev));
@@ -353,10 +353,10 @@ static void piix4_update_hotplug(PIIX4PMState *s)
     QTAILQ_FOREACH_SAFE(kid, &bus->children, sibling, next) {
         DeviceState *qdev = kid->child;
         PCIDevice *pdev = PCI_DEVICE(qdev);
-        PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pdev);
+        DeviceClass *dc = DEVICE_GET_CLASS(qdev);
         int slot = PCI_SLOT(pdev->devfn);
 
-        if (pc->no_hotplug) {
+        if (!dc->hotplugable) {
             s->pci0_hotplug_enable &= ~(1U << slot);
         }
 
@@ -746,7 +746,6 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = piix4_pm_initfn;
     k->config_write = pm_write_config;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
@@ -757,6 +756,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
     dc->no_user = 1;
     dc->vmsd = &vmstate_acpi;
     dc->props = piix4_pm_properties;
+    dc->hotplugable = false;
 }
 
 static const TypeInfo piix4_pm_info = {
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 1b1a684..6af314b 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -215,6 +215,12 @@ void qdev_unplug(DeviceState *dev, Error **errp)
     }
     assert(dc->unplug != NULL);
 
+    if (!dc->hotplugable) {
+        error_set(errp, QERR_DEVICE_NO_HOTPLUG,
+                  object_get_typename(OBJECT(dev)));
+        return;
+    }
+
     qdev_hot_removed = true;
 
     if (dc->unplug(dev) < 0) {
@@ -679,6 +685,11 @@ static void device_set_realized(Object *obj, bool value, Error **err)
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
     Error *local_err = NULL;
 
+    if (dev->hotplugged && !dc->hotplugable) {
+        error_set(err, QERR_DEVICE_NO_HOTPLUG, object_get_typename(obj));
+        return;
+    }
+
     if (value && !dev->realized) {
         if (!obj->parent && local_err == NULL) {
             static int unattached_count;
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index e4c345f..67b1014 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -2996,7 +2996,6 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = pci_cirrus_vga_initfn;
     k->romfile = VGABIOS_CIRRUS_FILENAME;
     k->vendor_id = PCI_VENDOR_ID_CIRRUS;
@@ -3006,6 +3005,7 @@ static void cirrus_vga_class_init(ObjectClass *klass, void *data)
     dc->desc = "Cirrus CLGD 54xx VGA";
     dc->vmsd = &vmstate_pci_cirrus_vga;
     dc->props = pci_vga_cirrus_properties;
+    dc->hotplugable = false;
 }
 
 static const TypeInfo cirrus_vga_info = {
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index efdefd6..947d225 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2289,7 +2289,6 @@ static void qxl_primary_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = qxl_init_primary;
     k->romfile = "vgabios-qxl.bin";
     k->vendor_id = REDHAT_PCI_VENDOR_ID;
@@ -2300,6 +2299,7 @@ static void qxl_primary_class_init(ObjectClass *klass, void *data)
     dc->reset = qxl_reset_handler;
     dc->vmsd = &qxl_vmstate;
     dc->props = qxl_properties;
+    dc->hotplugable = false;
 }
 
 static const TypeInfo qxl_primary_info = {
diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index b3a45c8..e659d10 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -190,7 +190,6 @@ static void vga_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = pci_std_vga_initfn;
     k->romfile = "vgabios-stdvga.bin";
     k->vendor_id = PCI_VENDOR_ID_QEMU;
@@ -198,6 +197,7 @@ static void vga_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_DISPLAY_VGA;
     dc->vmsd = &vmstate_vga_pci;
     dc->props = vga_pci_properties;
+    dc->hotplugable = false;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
 }
 
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index aba292c..390ae3c 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1296,7 +1296,6 @@ static void vmsvga_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = pci_vmsvga_initfn;
     k->romfile = "vgabios-vmware.bin";
     k->vendor_id = PCI_VENDOR_ID_VMWARE;
@@ -1307,6 +1306,7 @@ static void vmsvga_class_init(ObjectClass *klass, void *data)
     dc->reset = vmsvga_reset;
     dc->vmsd = &vmstate_vmware_vga;
     dc->props = vga_vmware_properties;
+    dc->hotplugable = false;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
 }
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index befc39f..9b3af42 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -186,16 +186,16 @@ static void acpi_get_hotplug_info(AcpiMiscInfo *misc)
            DIV_ROUND_UP(PCI_SLOT_MAX, BITS_PER_BYTE));
 
     for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
-        PCIDeviceClass *pc;
+        DeviceClass *dc;
         PCIDevice *pdev = bus->devices[i];
 
         if (!pdev) {
             continue;
         }
 
-        pc = PCI_DEVICE_GET_CLASS(pdev);
+        dc = DEVICE_GET_CLASS(pdev);
 
-        if (pc->no_hotplug) {
+        if (!dc->hotplugable) {
             int slot = PCI_SLOT(i);
 
             clear_bit(slot, misc->slot_hotplug_enable);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ab36749..3023e87 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -241,7 +241,6 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = pci_piix_ide_initfn;
     k->exit = pci_piix_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
@@ -249,6 +248,7 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->no_user = 1;
+    dc->hotplugable = false;
 }
 
 static const TypeInfo piix3_ide_info = {
@@ -282,7 +282,6 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = pci_piix_ide_initfn;
     k->exit = pci_piix_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
@@ -290,6 +289,7 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
     dc->no_user = 1;
+    dc->hotplugable = false;
 }
 
 static const TypeInfo piix4_ide_info = {
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 1a1d451..5380c00 100644
--- a/hw/isa/piix4.c
+++ b/hw/isa/piix4.c
@@ -107,7 +107,6 @@ static void piix4_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = piix4_initfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB_0;
@@ -115,6 +114,7 @@ static void piix4_class_init(ObjectClass *klass, void *data)
     dc->desc = "ISA bridge";
     dc->no_user = 1;
     dc->vmsd = &vmstate_piix4;
+    dc->hotplugable = false;
 }
 
 static const TypeInfo piix4_info = {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index edc974e..5a0c09a 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -654,7 +654,7 @@ static void piix3_class_init(ObjectClass *klass, void *data)
     dc->desc        = "ISA bridge";
     dc->vmsd        = &vmstate_piix3;
     dc->no_user     = 1,
-    k->no_hotplug   = 1;
+    dc->hotplugable   = false;
     k->init         = piix3_initfn;
     k->config_write = piix3_write_config;
     k->vendor_id    = PCI_VENDOR_ID_INTEL;
@@ -678,7 +678,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
     dc->desc        = "ISA bridge";
     dc->vmsd        = &vmstate_piix3;
     dc->no_user     = 1;
-    k->no_hotplug   = 1;
+    dc->hotplugable   = false;
     k->init         = piix3_initfn;
     k->config_write = piix3_write_config_xen;
     k->vendor_id    = PCI_VENDOR_ID_INTEL;
@@ -699,7 +699,6 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->no_hotplug = 1;
     k->init = i440fx_initfn;
     k->config_write = i440fx_write_config;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
@@ -709,6 +708,7 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
     dc->desc = "Host bridge";
     dc->no_user = 1;
     dc->vmsd = &vmstate_i440fx;
+    dc->hotplugable   = false;
 }
 
 static const TypeInfo i440fx_info = {
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 49eca95..d3dbde3 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1735,11 +1735,7 @@ static int pci_qdev_init(DeviceState *qdev)
                                      pci_dev->devfn);
     if (pci_dev == NULL)
         return -1;
-    if (qdev->hotplugged && pc->no_hotplug) {
-        qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(pci_dev)));
-        do_pci_unregister_device(pci_dev);
-        return -1;
-    }
+
     if (pc->init) {
         rc = pc->init(pci_dev);
         if (rc != 0) {
@@ -1774,12 +1770,7 @@ static int pci_qdev_init(DeviceState *qdev)
 static int pci_unplug_device(DeviceState *qdev)
 {
     PCIDevice *dev = PCI_DEVICE(qdev);
-    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
 
-    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);
 }
@@ -2258,6 +2249,7 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
     k->exit = pci_unregister_device;
     k->bus_type = TYPE_PCI_BUS;
     k->props = pci_props;
+    k->hotplugable = true;
 }
 
 AddressSpace *pci_device_iommu_address_space(PCIDevice *dev)
diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
index 0c98594..4c40dab 100644
--- a/hw/usb/hcd-ehci-pci.c
+++ b/hw/usb/hcd-ehci-pci.c
@@ -123,7 +123,7 @@ static void ehci_class_init(ObjectClass *klass, void *data)
     k->init = usb_ehci_pci_initfn;
     k->class_id = PCI_CLASS_SERIAL_USB;
     k->config_write = usb_ehci_pci_write_config;
-    k->no_hotplug = 1;
+    dc->hotplugable = false;
     dc->vmsd = &vmstate_ehci_pci;
     dc->props = ehci_pci_properties;
 }
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index e38cdeb..1800262 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1993,10 +1993,10 @@ static void ohci_pci_class_init(ObjectClass *klass, void *data)
     k->vendor_id = PCI_VENDOR_ID_APPLE;
     k->device_id = PCI_DEVICE_ID_APPLE_IPID_USB;
     k->class_id = PCI_CLASS_SERIAL_USB;
-    k->no_hotplug = 1;
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
     dc->desc = "Apple USB Controller";
     dc->props = ohci_pci_properties;
+    dc->hotplugable = false;
 }
 
 static const TypeInfo ohci_pci_info = {
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 238d1d2..3afb311 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1318,7 +1318,7 @@ static void uhci_class_init(ObjectClass *klass, void *data)
     k->device_id = info->device_id;
     k->revision  = info->revision;
     k->class_id  = PCI_CLASS_SERIAL_USB;
-    k->no_hotplug = 1;
+    dc->hotplugable = false;
     dc->vmsd = &vmstate_uhci;
     dc->props = uhci_properties;
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index bafe085..50b66e9 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3798,6 +3798,7 @@ static void xhci_class_init(ObjectClass *klass, void *data)
     dc->vmsd    = &vmstate_xhci;
     dc->props   = xhci_properties;
     dc->reset   = xhci_reset;
+    dc->hotplugable   = false;
     set_bit(DEVICE_CATEGORY_USB, dc->categories);
     k->init         = usb_xhci_initfn;
     k->vendor_id    = PCI_VENDOR_ID_NEC;
@@ -3805,7 +3806,6 @@ static void xhci_class_init(ObjectClass *klass, void *data)
     k->class_id     = PCI_CLASS_SERIAL_USB;
     k->revision     = 0x03;
     k->is_express   = 1;
-    k->no_hotplug   = 1;
 }
 
 static const TypeInfo xhci_info = {
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b783e68..6b79358 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -201,9 +201,6 @@ typedef struct PCIDeviceClass {
     /* pcie stuff */
     int is_express;   /* is this device pci express? */
 
-    /* device isn't hot-pluggable */
-    int no_hotplug;
-
     /* rom bar */
     const char *romfile;
 } PCIDeviceClass;
-- 
1.7.1

[Qemu-devel] [PATCH 08/11] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API

[Igor Mammedov]

Split piix4_device_hotplug() into hotplug/unplug callbacks
and register them as "hotplug-handler" interface implementation of
PIIX4_PM device.

Replace pci_bus_hotplug() wiring with setting link on
PCI BUS "hotplug-handler" property to PIIX4_PM device.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/piix4.c |   73 +++++++++++++++++++++++++++++++------------------------
 1 files changed, 41 insertions(+), 32 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 8ccc70b..c36eec5 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -30,6 +30,8 @@
 #include "hw/nvram/fw_cfg.h"
 #include "exec/address-spaces.h"
 #include "hw/acpi/piix4.h"
+#include "qapi/qmp/qerror.h"
+#include "hw/hotplug.h"
 
 //#define DEBUG
 
@@ -107,7 +109,7 @@ typedef struct PIIX4PMState {
     OBJECT_CHECK(PIIX4PMState, (obj), TYPE_PIIX4_PM)
 
 static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
-                                           PCIBus *bus, PIIX4PMState *s);
+                                           BusState *bus, PIIX4PMState *s);
 
 #define ACPI_ENABLE 0xf1
 #define ACPI_DISABLE 0xf0
@@ -478,7 +480,7 @@ static int piix4_pm_initfn(PCIDevice *dev)
     qemu_add_machine_init_done_notifier(&s->machine_ready);
     qemu_register_reset(piix4_reset, s);
 
-    piix4_acpi_system_hot_add_init(pci_address_space_io(dev), dev->bus, s);
+    piix4_acpi_system_hot_add_init(pci_address_space_io(dev), BUS(dev->bus), s);
 
     piix4_pm_add_propeties(s);
     return 0;
@@ -664,13 +666,11 @@ static void piix4_cpu_added_req(Notifier *n, void *opaque)
     piix4_cpu_hotplug_req(s, CPU(opaque), PLUG);
 }
 
-static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
-                                PCIHotplugState state);
-
 static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
-                                           PCIBus *bus, PIIX4PMState *s)
+                                           BusState *bus, PIIX4PMState *s)
 {
     CPUState *cpu;
+    Error *local_error = NULL;
 
     memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
                           "acpi-gpe0", GPE_LEN);
@@ -680,7 +680,14 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
                           "acpi-pci-hotplug", PCI_HOTPLUG_SIZE);
     memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR,
                                 &s->io_pci);
-    pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s));
+    object_property_set_link(OBJECT(bus), OBJECT(s),
+                             QDEV_HOTPLUG_HANDLER_PROPERTY, &local_error);
+    if (error_is_set(&local_error)) {
+        qerror_report_err(local_error);
+        error_free(local_error);
+        abort();
+    }
+    bus->allow_hotplug = 1;
 
     CPU_FOREACH(cpu) {
         CPUClass *cc = CPU_GET_CLASS(cpu);
@@ -696,41 +703,36 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
     qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
 }
 
-static void enable_device(PIIX4PMState *s, int slot)
+static void piix4_pci_device_hotplug_cb(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
 {
-    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
-    s->pci0_slot_device_present |= (1U << slot);
-}
-
-static void disable_device(PIIX4PMState *s, int slot)
-{
-    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
-    s->pci0_status.down |= (1U << slot);
-}
-
-static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
-				PCIHotplugState state)
-{
-    int slot = PCI_SLOT(dev->devfn);
-    PIIX4PMState *s = PIIX4_PM(qdev);
+    PCIDevice *pci_dev = PCI_DEVICE(dev);
+    int slot = PCI_SLOT(pci_dev->devfn);
+    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
 
     /* Don't send event when device is enabled during qemu machine creation:
      * it is present on boot, no hotplug event is necessary. We do send an
      * event when the device is disabled later. */
-    if (state == PCI_COLDPLUG_ENABLED) {
+    if (!dev->hotplugged) {
         s->pci0_slot_device_present |= (1U << slot);
-        return 0;
-    }
-
-    if (state == PCI_HOTPLUG_ENABLED) {
-        enable_device(s, slot);
-    } else {
-        disable_device(s, slot);
+        return;
     }
 
+    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
+    s->pci0_slot_device_present |= (1U << slot);
     pm_update_sci(s);
+}
 
-    return 0;
+static void piix4_pci_device_hot_unplug_cb(HotplugHandler *hotplug_dev,
+                                           DeviceState *dev, Error **errp)
+{
+    PCIDevice *pci_dev = PCI_DEVICE(dev);
+    int slot = PCI_SLOT(pci_dev->devfn);
+    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
+
+    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
+    s->pci0_status.down |= (1U << slot);
+    pm_update_sci(s);
 }
 
 static Property piix4_pm_properties[] = {
@@ -745,6 +747,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
 
     k->init = piix4_pm_initfn;
     k->config_write = pm_write_config;
@@ -757,6 +760,8 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_acpi;
     dc->props = piix4_pm_properties;
     dc->hotplugable = false;
+    hc->plug = piix4_pci_device_hotplug_cb;
+    hc->unplug = piix4_pci_device_hot_unplug_cb;
 }
 
 static const TypeInfo piix4_pm_info = {
@@ -764,6 +769,10 @@ static const TypeInfo piix4_pm_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PIIX4PMState),
     .class_init    = piix4_pm_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void piix4_pm_register_types(void)
-- 
1.7.1

[Qemu-devel] [PATCH 09/11] pci/shpc: convert SHPC hotplug to use hotplug-handler API

[Igor Mammedov]

Split shpc_device_hotplug() into hotplug/unplug callbacks
and register them as "hotplug-handler" interface implementation of
PCI_BRIDGE_DEV device.

Replace pci_bus_hotplug() wiring with setting link on PCI BUS
"hotplug-handler" property to PCI_BRIDGE_DEV device.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pci-bridge/pci_bridge_dev.c |    9 +++
 hw/pci/shpc.c                  |  133 ++++++++++++++++++++++++++--------------
 include/hw/pci/shpc.h          |    8 +++
 3 files changed, 103 insertions(+), 47 deletions(-)

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 440e187..e68145c 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -26,6 +26,7 @@
 #include "hw/pci/slotid_cap.h"
 #include "exec/memory.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/hotplug.h"
 
 #define TYPE_PCI_BRIDGE_DEV "pci-bridge"
 #define PCI_BRIDGE_DEV(obj) \
@@ -136,6 +137,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
+
     k->init = pci_bridge_dev_initfn;
     k->exit = pci_bridge_dev_exitfn;
     k->config_write = pci_bridge_dev_write_config;
@@ -148,6 +151,8 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
     dc->props = pci_bridge_dev_properties;
     dc->vmsd = &pci_bridge_dev_vmstate;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+    hc->plug = shpc_device_hotplug_cb;
+    hc->unplug = shpc_device_hot_unplug_cb;
 }
 
 static const TypeInfo pci_bridge_dev_info = {
@@ -155,6 +160,10 @@ static const TypeInfo pci_bridge_dev_info = {
     .parent        = TYPE_PCI_BRIDGE,
     .instance_size = sizeof(PCIBridgeDev),
     .class_init = pci_bridge_dev_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void pci_bridge_dev_register(void)
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 576244b..92dbd95 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -7,6 +7,7 @@
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/msi.h"
+#include "qapi/qmp/qerror.h"
 
 /* TODO: model power only and disabled slot states. */
 /* TODO: handle SERR and wakeups */
@@ -490,73 +491,103 @@ static const MemoryRegionOps shpc_mmio_ops = {
         .max_access_size = 4,
     },
 };
-
-static int shpc_device_hotplug(DeviceState *qdev, PCIDevice *affected_dev,
-                               PCIHotplugState hotplug_state)
+static void shpc_device_hotplug_common(PCIDevice *affected_dev, int *slot,
+                                       SHPCDevice *shpc, Error **errp)
 {
     int pci_slot = PCI_SLOT(affected_dev->devfn);
-    uint8_t state;
-    uint8_t led;
-    PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
-    SHPCDevice *shpc = d->shpc;
-    int slot = SHPC_PCI_TO_IDX(pci_slot);
-    if (pci_slot < SHPC_IDX_TO_PCI(0) || slot >= shpc->nslots) {
-        error_report("Unsupported PCI slot %d for standard hotplug "
-                     "controller. Valid slots are between %d and %d.",
-                     pci_slot, SHPC_IDX_TO_PCI(0),
-                     SHPC_IDX_TO_PCI(shpc->nslots) - 1);
-        return -1;
+    *slot = SHPC_PCI_TO_IDX(pci_slot);
+
+    if (pci_slot < SHPC_IDX_TO_PCI(0) || *slot >= shpc->nslots) {
+        error_setg(errp, "Unsupported PCI slot %d for standard hotplug "
+                   "controller. Valid slots are between %d and %d.",
+                   pci_slot, SHPC_IDX_TO_PCI(0),
+                   SHPC_IDX_TO_PCI(shpc->nslots) - 1);
+        return;
+    }
+}
+
+void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                            Error **errp)
+{
+    Error *local_err = NULL;
+    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
+    SHPCDevice *shpc = pci_hotplug_dev->shpc;
+    int slot;
+
+    shpc_device_hotplug_common(PCI_DEVICE(dev), &slot, shpc, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        return;
     }
+
     /* Don't send event when device is enabled during qemu machine creation:
      * it is present on boot, no hotplug event is necessary. We do send an
      * event when the device is disabled later. */
-    if (hotplug_state == PCI_COLDPLUG_ENABLED) {
+    if (!dev->hotplugged) {
         shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_MRL_OPEN);
         shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_7_5W,
                         SHPC_SLOT_STATUS_PRSNT_MASK);
-        return 0;
+        return;
     }
-    if (hotplug_state == PCI_HOTPLUG_DISABLED) {
-        shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |= SHPC_SLOT_EVENT_BUTTON;
-        state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK);
-        led = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK);
-        if (state == SHPC_STATE_DISABLED && led == SHPC_LED_OFF) {
-            shpc_free_devices_in_slot(shpc, slot);
-            shpc_set_status(shpc, slot, 1, SHPC_SLOT_STATUS_MRL_OPEN);
-            shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_EMPTY,
-                            SHPC_SLOT_STATUS_PRSNT_MASK);
-            shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
-                SHPC_SLOT_EVENT_MRL |
-                SHPC_SLOT_EVENT_PRESENCE;
-        }
+
+    /* This could be a cancellation of the previous removal.
+     * We check MRL state to figure out. */
+    if (shpc_get_status(shpc, slot, SHPC_SLOT_STATUS_MRL_OPEN)) {
+        shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_MRL_OPEN);
+        shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_7_5W,
+                        SHPC_SLOT_STATUS_PRSNT_MASK);
+        shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
+            SHPC_SLOT_EVENT_BUTTON |
+            SHPC_SLOT_EVENT_MRL |
+            SHPC_SLOT_EVENT_PRESENCE;
     } else {
-        /* This could be a cancellation of the previous removal.
-         * We check MRL state to figure out. */
-        if (shpc_get_status(shpc, slot, SHPC_SLOT_STATUS_MRL_OPEN)) {
-            shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_MRL_OPEN);
-            shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_7_5W,
-                            SHPC_SLOT_STATUS_PRSNT_MASK);
-            shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
-                SHPC_SLOT_EVENT_BUTTON |
-                SHPC_SLOT_EVENT_MRL |
-                SHPC_SLOT_EVENT_PRESENCE;
-        } else {
-            /* Press attention button to cancel removal */
-            shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
-                SHPC_SLOT_EVENT_BUTTON;
-        }
+        /* Press attention button to cancel removal */
+        shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
+            SHPC_SLOT_EVENT_BUTTON;
     }
     shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_66);
-    shpc_interrupt_update(d);
-    return 0;
+    shpc_interrupt_update(pci_hotplug_dev);
+}
+
+void shpc_device_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                               Error **errp)
+{
+    Error *local_err = NULL;
+    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
+    SHPCDevice *shpc = pci_hotplug_dev->shpc;
+    uint8_t state;
+    uint8_t led;
+    int slot;
+
+    shpc_device_hotplug_common(PCI_DEVICE(dev), &slot, shpc, errp);
+    if (error_is_set(&local_err)) {
+        return;
+    }
+
+    shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |= SHPC_SLOT_EVENT_BUTTON;
+    state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK);
+    led = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK);
+    if (state == SHPC_STATE_DISABLED && led == SHPC_LED_OFF) {
+        shpc_free_devices_in_slot(shpc, slot);
+        shpc_set_status(shpc, slot, 1, SHPC_SLOT_STATUS_MRL_OPEN);
+        shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_EMPTY,
+                        SHPC_SLOT_STATUS_PRSNT_MASK);
+        shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
+            SHPC_SLOT_EVENT_MRL |
+            SHPC_SLOT_EVENT_PRESENCE;
+    }
+    shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_66);
+    shpc_interrupt_update(pci_hotplug_dev);
 }
 
 /* Initialize the SHPC structure in bridge's BAR. */
 int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar, unsigned offset)
 {
     int i, ret;
+    Error *local_error = NULL;
     int nslots = SHPC_MAX_SLOTS; /* TODO: qdev property? */
     SHPCDevice *shpc = d->shpc = g_malloc0(sizeof(*d->shpc));
+    BusState *bus = BUS(sec_bus);
     shpc->sec_bus = sec_bus;
     ret = shpc_cap_add_config(d);
     if (ret) {
@@ -616,7 +647,15 @@ int shpc_init(PCIDevice *d, PCIBus *sec_bus, MemoryRegion *bar, unsigned offset)
                           d, "shpc-mmio", SHPC_SIZEOF(d));
     shpc_cap_update_dword(d);
     memory_region_add_subregion(bar, offset, &shpc->mmio);
-    pci_bus_hotplug(sec_bus, shpc_device_hotplug, &d->qdev);
+
+    object_property_set_link(OBJECT(sec_bus), OBJECT(d),
+                             QDEV_HOTPLUG_HANDLER_PROPERTY, &local_error);
+    if (error_is_set(&local_error)) {
+        qerror_report_err(local_error);
+        error_free(local_error);
+        return -1;
+    }
+    bus->allow_hotplug = 1;
 
     d->cap_present |= QEMU_PCI_CAP_SHPC;
     return 0;
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 467911a..eef1a1a 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "exec/memory.h"
 #include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "hw/hotplug.h"
 
 struct SHPCDevice {
     /* Capability offset in device's config space */
@@ -41,6 +43,12 @@ int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
 void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
 void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
 
+
+void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                            Error **errp);
+void shpc_device_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                               Error **errp);
+
 extern VMStateInfo shpc_vmstate_info;
 #define SHPC_VMSTATE(_field, _type) \
     VMSTATE_BUFFER_UNSAFE_INFO(_field, _type, 0, shpc_vmstate_info, 0)
-- 
1.7.1

[Qemu-devel] [PATCH 10/11] pci/pcie: convert PCIE hotplug to use hotplug-handler API

[Igor Mammedov]

Split pcie_cap_slot_hotplug() into hotplug/unplug callbacks
and register them as "hotplug-handler" interface implementation of
PCIE_SLOT device.

Replace pci_bus_hotplug() wiring with setting link on PCI BUS
"hotplug-handler" property to PCI_BRIDGE_DEV device.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pci/pcie.c         |   73 ++++++++++++++++++++++++++++++++-----------------
 hw/pci/pcie_port.c    |    8 +++++
 include/hw/pci/pcie.h |    5 +++
 3 files changed, 61 insertions(+), 25 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index ca60cf2..d4e978c 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -26,6 +26,7 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/pcie_regs.h"
 #include "qemu/range.h"
+#include "qapi/qmp/qerror.h"
 
 //#define DEBUG_PCIE
 #ifdef DEBUG_PCIE
@@ -216,28 +217,20 @@ static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event)
     hotplug_event_notify(dev);
 }
 
-static int pcie_cap_slot_hotplug(DeviceState *qdev,
-                                 PCIDevice *pci_dev, PCIHotplugState state)
+static void pcie_cap_slot_hotplug_common(PCIDevice *hotplug_dev,
+                                         DeviceState *dev,
+                                         uint8_t **exp_cap, Error **errp)
 {
-    PCIDevice *d = PCI_DEVICE(qdev);
-    uint8_t *exp_cap = d->config + d->exp.exp_cap;
-    uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
-
-    /* Don't send event when device is enabled during qemu machine creation:
-     * it is present on boot, no hotplug event is necessary. We do send an
-     * event when the device is disabled later. */
-    if (state == PCI_COLDPLUG_ENABLED) {
-        pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
-                                   PCI_EXP_SLTSTA_PDS);
-        return 0;
-    }
+    PCIDevice *pci_dev = PCI_DEVICE(dev);
+    *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
+    uint16_t sltsta = pci_get_word(*exp_cap + PCI_EXP_SLTSTA);
 
     PCIE_DEV_PRINTF(pci_dev, "hotplug state: %d\n", state);
     if (sltsta & PCI_EXP_SLTSTA_EIS) {
         /* the slot is electromechanically locked.
          * This error is propagated up to qdev and then to HMP/QMP.
          */
-        return -EBUSY;
+        error_setg_errno(errp, -EBUSY, "slot is electromechanically locked");
     }
 
     /* TODO: multifunction hot-plug.
@@ -245,18 +238,40 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
      * hot plugged/unplugged.
      */
     assert(PCI_FUNC(pci_dev->devfn) == 0);
+}
 
-    if (state == PCI_HOTPLUG_ENABLED) {
+void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                              Error **errp)
+{
+    uint8_t *exp_cap;
+
+    pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
+
+    /* Don't send event when device is enabled during qemu machine creation:
+     * it is present on boot, no hotplug event is necessary. We do send an
+     * event when the device is disabled later. */
+    if (!dev->hotplugged) {
         pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
                                    PCI_EXP_SLTSTA_PDS);
-        pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
-    } else {
-        object_unparent(OBJECT(pci_dev));
-        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
-                                     PCI_EXP_SLTSTA_PDS);
-        pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
+        return;
     }
-    return 0;
+
+    pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
+                               PCI_EXP_SLTSTA_PDS);
+    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC);
+}
+
+void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                 Error **errp)
+{
+    uint8_t *exp_cap;
+
+    pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
+
+    object_unparent(OBJECT(dev));
+    pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTSTA,
+                                 PCI_EXP_SLTSTA_PDS);
+    pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC);
 }
 
 /* pci express slot for pci express root/downstream port
@@ -264,6 +279,8 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
 void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
 {
     uint32_t pos = dev->exp.exp_cap;
+    BusState *bus = BUS(pci_bridge_get_sec_bus(PCI_BRIDGE(dev)));
+    Error *local_error = NULL;
 
     pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_FLAGS,
                                PCI_EXP_FLAGS_SLOT);
@@ -305,8 +322,14 @@ void pcie_cap_slot_init(PCIDevice *dev, uint16_t slot)
 
     dev->exp.hpev_notified = false;
 
-    pci_bus_hotplug(pci_bridge_get_sec_bus(PCI_BRIDGE(dev)),
-                    pcie_cap_slot_hotplug, &dev->qdev);
+    object_property_set_link(OBJECT(bus), OBJECT(dev),
+                             QDEV_HOTPLUG_HANDLER_PROPERTY, &local_error);
+    if (error_is_set(&local_error)) {
+        qerror_report_err(local_error);
+        error_free(local_error);
+        abort();
+    }
+    bus->allow_hotplug = 1;
 }
 
 void pcie_cap_slot_reset(PCIDevice *dev)
diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
index 2adb030..fa24877 100644
--- a/hw/pci/pcie_port.c
+++ b/hw/pci/pcie_port.c
@@ -19,6 +19,7 @@
  */
 
 #include "hw/pci/pcie_port.h"
+#include "hw/hotplug.h"
 
 void pcie_port_init_reg(PCIDevice *d)
 {
@@ -149,8 +150,11 @@ static Property pcie_slot_props[] = {
 static void pcie_slot_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     dc->props = pcie_slot_props;
+    hc->plug = pcie_cap_slot_hotplug_cb;
+    hc->unplug = pcie_cap_slot_hot_unplug_cb;
 }
 
 static const TypeInfo pcie_slot_type_info = {
@@ -159,6 +163,10 @@ static const TypeInfo pcie_slot_type_info = {
     .instance_size = sizeof(PCIESlot),
     .abstract = true,
     .class_init = pcie_slot_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void pcie_port_register_types(void)
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 1966169..b0bf7e3 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -25,6 +25,7 @@
 #include "hw/pci/pci_regs.h"
 #include "hw/pci/pcie_regs.h"
 #include "hw/pci/pcie_aer.h"
+#include "hw/hotplug.h"
 
 typedef enum {
     /* for attention and power indicator */
@@ -122,4 +123,8 @@ extern const VMStateDescription vmstate_pcie_device;
     .offset     = vmstate_offset_value(_state, _field, PCIDevice),   \
 }
 
+void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                              Error **errp);
+void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                 Error **errp);
 #endif /* QEMU_PCIE_H */
-- 
1.7.1

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

[Igor Mammedov]

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.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/qdev.c           |   17 +++++++++++++----
 hw/pci/pci.c             |   29 +----------------------------
 include/hw/pci/pci.h     |   10 ----------
 include/hw/pci/pci_bus.h |    2 --
 4 files changed, 14 insertions(+), 44 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6af314b..a00529c 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->hotplugable) {
         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);
+        }
     }
 }
 
@@ -705,6 +708,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 d3dbde3..7a04a9f 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,
@@ -1752,29 +1746,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)
 {
@@ -2245,7 +2219,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 6b79358..dd45e04 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;
-- 
1.7.1

[Qemu-devel] [PATCH 00/11 v3] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API

[Igor Mammedov]

changes since v2:
* s/hotplugable/hotpluggable/
* move hotplug check to an earlier patch:
  "qdev: add "hotpluggable" property to Device"
--
Refactor PCI specific hotplug API to a more generic/reusable one.
Model it after SCSI-BUS like hotplug API replacing single hotplug
callback with hotplug/hot_unplug pair of callbacks as suggested by
Paolo.
Difference between SCSI-BUS and this approach is that the former
is BUS centric while the latter is device centred. Which is evolved
from the fact that hotplug callbacks used by ACPI/SHPC/PCIE are
implemented by devices rather than by bus and bus serves only as
a proxy to forward event to hotplug device.
Memory hotplug also exposes tha same usage pattern hence an attempt
to generalize hotplug API.

Refactoring also simplifies wiring of a hotplug device with a bus,
all it needs is to set "hotplug-device" link on bus, which
would potentially allow to do it from configuration file,
there is not need to setup hotplug device callbacks on bus
synce it can get them via HOTPLUG_DEVICE API of "hotplug-device"
target.

In addition device centred hotplug API may be used by bus-less
hotplug implementations as well if it's decided to use
link<foo...> instead of bus.

Patches 8-11 are should be merged as one and are split only for
simplifying review (they compile fine but PCI hotplug is broken
until the last patch is applyed).

git tree for testing:
https://github.com/imammedo/qemu/commits/hotplug_dev_inf_v3

tested only ACPI and PCIE hotplug.

Hervé Poussineau (1):
  qom: detect bad reentrance during object_class_foreach

Igor Mammedov (9):
  define hotplug interface
  qdev: add to BusState "hotplug-handler" link
  qdev: add "hotpluggable" property to Device
  hw/acpi: move typeinfo to the file end
  qdev:pci: refactor PCIDevice to use generic "hotpluggable" property
  acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API
  pci/shpc: convert SHPC hotplug to use hotplug-handler API
  pci/pcie: convert PCIE hotplug to use hotplug-handler API
  hw/pci: switch to a generic hotplug handling for PCIDevice

Paolo Bonzini (1):
  qom: do not register interface "types" in the type table

 hw/acpi/piix4.c                | 151 ++++++++++++++++++++++-------------------
 hw/core/Makefile.objs          |   1 +
 hw/core/hotplug.c              |  48 +++++++++++++
 hw/core/qdev.c                 |  50 ++++++++++++--
 hw/display/cirrus_vga.c        |   2 +-
 hw/display/qxl.c               |   2 +-
 hw/display/vga-pci.c           |   2 +-
 hw/display/vmware_vga.c        |   2 +-
 hw/i386/acpi-build.c           |   6 +-
 hw/ide/piix.c                  |   4 +-
 hw/isa/piix4.c                 |   2 +-
 hw/pci-bridge/pci_bridge_dev.c |   9 +++
 hw/pci-host/piix.c             |   6 +-
 hw/pci/pci.c                   |  40 +----------
 hw/pci/pcie.c                  |  73 +++++++++++++-------
 hw/pci/pcie_port.c             |   8 +++
 hw/pci/shpc.c                  | 133 +++++++++++++++++++++++-------------
 hw/usb/hcd-ehci-pci.c          |   2 +-
 hw/usb/hcd-ohci.c              |   2 +-
 hw/usb/hcd-uhci.c              |   2 +-
 hw/usb/hcd-xhci.c              |   2 +-
 include/hw/hotplug.h           |  75 ++++++++++++++++++++
 include/hw/pci/pci.h           |  13 ----
 include/hw/pci/pci_bus.h       |   2 -
 include/hw/pci/pcie.h          |   5 ++
 include/hw/pci/shpc.h          |   8 +++
 include/hw/qdev-core.h         |   8 +++
 qom/object.c                   |  17 ++++-
 28 files changed, 455 insertions(+), 220 deletions(-)
 create mode 100644 hw/core/hotplug.c
 create mode 100644 include/hw/hotplug.h

-- 
1.8.3.1

[Qemu-devel] [PATCH 04/11] qdev: add to BusState "hotplug-handler" link

[Igor Mammedov]

It will allow to reuse field with different BUSes, reducing code duplication.
Field is intended fot replacing 'hotplug_qdev' field in PCIBus and also
will allow to avoid adding equivalent field to DimmBus with possiblitity
to refactor other BUSes to use it instead of custom field.
In addition once all users of allow_hotplug field are converted to new
API, link could replace allow_hotplug in qdev hotplug code.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/core/qdev.c         | 4 ++++
 include/hw/qdev-core.h | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index e374a93..25c2d2c 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -32,6 +32,7 @@
 #include "qapi/visitor.h"
 #include "qapi/qmp/qjson.h"
 #include "monitor/monitor.h"
+#include "hw/hotplug.h"
 
 int qdev_hotplug = 0;
 static bool qdev_hot_added = false;
@@ -868,6 +869,9 @@ static void qbus_initfn(Object *obj)
     BusState *bus = BUS(obj);
 
     QTAILQ_INIT(&bus->children);
+    object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY,
+                             TYPE_HOTPLUG_HANDLER,
+                             (Object **)&bus->hotplug_handler, NULL);
 }
 
 static char *default_bus_get_fw_dev_path(DeviceState *dev)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index f2043a6..684a5da 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -8,6 +8,7 @@
 #include "qom/object.h"
 #include "hw/irq.h"
 #include "qapi/error.h"
+#include "hw/hotplug.h"
 
 enum {
     DEV_NVECTORS_UNSPECIFIED = -1,
@@ -169,14 +170,18 @@ typedef struct BusChild {
     QTAILQ_ENTRY(BusChild) sibling;
 } BusChild;
 
+#define QDEV_HOTPLUG_HANDLER_PROPERTY "hotplug-handler"
+
 /**
  * BusState:
+ * @hotplug_device: link to a hotplug device associated with bus.
  */
 struct BusState {
     Object obj;
     DeviceState *parent;
     const char *name;
     int allow_hotplug;
+    HotplugHandler *hotplug_handler;
     int max_index;
     QTAILQ_HEAD(ChildrenHead, BusChild) children;
     QLIST_ENTRY(BusState) sibling;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 04/11] qdev: add to BusState "hotplug-handler" link

[Peter Crosthwaite]

On Fri, Dec 13, 2013 at 10:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> It will allow to reuse field with different BUSes, reducing code duplication.
> Field is intended fot replacing 'hotplug_qdev' field in PCIBus and also
> will allow to avoid adding equivalent field to DimmBus with possiblitity
> to refactor other BUSes to use it instead of custom field.
> In addition once all users of allow_hotplug field are converted to new
> API, link could replace allow_hotplug in qdev hotplug code.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/core/qdev.c         | 4 ++++
>  include/hw/qdev-core.h | 5 +++++
>  2 files changed, 9 insertions(+)
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index e374a93..25c2d2c 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -32,6 +32,7 @@
>  #include "qapi/visitor.h"
>  #include "qapi/qmp/qjson.h"
>  #include "monitor/monitor.h"
> +#include "hw/hotplug.h"
>
>  int qdev_hotplug = 0;
>  static bool qdev_hot_added = false;
> @@ -868,6 +869,9 @@ static void qbus_initfn(Object *obj)
>      BusState *bus = BUS(obj);
>
>      QTAILQ_INIT(&bus->children);
> +    object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY,
> +                             TYPE_HOTPLUG_HANDLER,
> +                             (Object **)&bus->hotplug_handler, NULL);

I think failure of this is fatal. When the patches go through,
probably want to &error_abort.

>  }
>
>  static char *default_bus_get_fw_dev_path(DeviceState *dev)
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index f2043a6..684a5da 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -8,6 +8,7 @@
>  #include "qom/object.h"
>  #include "hw/irq.h"
>  #include "qapi/error.h"
> +#include "hw/hotplug.h"
>
>  enum {
>      DEV_NVECTORS_UNSPECIFIED = -1,
> @@ -169,14 +170,18 @@ typedef struct BusChild {
>      QTAILQ_ENTRY(BusChild) sibling;
>  } BusChild;
>
> +#define QDEV_HOTPLUG_HANDLER_PROPERTY "hotplug-handler"
> +
>  /**
>   * BusState:
> + * @hotplug_device: link to a hotplug device associated with bus.
>   */
>  struct BusState {
>      Object obj;
>      DeviceState *parent;
>      const char *name;
>      int allow_hotplug;
> +    HotplugHandler *hotplug_handler;
>      int max_index;
>      QTAILQ_HEAD(ChildrenHead, BusChild) children;
>      QLIST_ENTRY(BusState) sibling;
> --
> 1.8.3.1
>
>

Re: [Qemu-devel] [PATCH 04/11] qdev: add to BusState "hotplug-handler" link

[Igor Mammedov]

On Sat, 14 Dec 2013 17:05:57 +1000
Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> On Fri, Dec 13, 2013 at 10:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > It will allow to reuse field with different BUSes, reducing code duplication.
> > Field is intended fot replacing 'hotplug_qdev' field in PCIBus and also
> > will allow to avoid adding equivalent field to DimmBus with possiblitity
> > to refactor other BUSes to use it instead of custom field.
> > In addition once all users of allow_hotplug field are converted to new
> > API, link could replace allow_hotplug in qdev hotplug code.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/core/qdev.c         | 4 ++++
> >  include/hw/qdev-core.h | 5 +++++
> >  2 files changed, 9 insertions(+)
> >
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index e374a93..25c2d2c 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -32,6 +32,7 @@
> >  #include "qapi/visitor.h"
> >  #include "qapi/qmp/qjson.h"
> >  #include "monitor/monitor.h"
> > +#include "hw/hotplug.h"
> >
> >  int qdev_hotplug = 0;
> >  static bool qdev_hot_added = false;
> > @@ -868,6 +869,9 @@ static void qbus_initfn(Object *obj)
> >      BusState *bus = BUS(obj);
> >
> >      QTAILQ_INIT(&bus->children);
> > +    object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY,
> > +                             TYPE_HOTPLUG_HANDLER,
> > +                             (Object **)&bus->hotplug_handler, NULL);
> 
> I think failure of this is fatal. When the patches go through,
> probably want to &error_abort.
That's just another example of we don't care about error that
gave birth to &error_abort topic. But I think is a drawback if
error-less initfn. We have a lot of such usage without a way
to report error, that's why "we don't care about error" was
invented I guess.

But more to the point, bus could be created during hotplug, and
we do not want to abort guest, instead hotplug operation should fail.
In this case following setting of QDEV_HOTPLUG_HANDLER_PROPERTY
will fail if object_property_add_link() failed before.

> 
> >  }
> >
> >  static char *default_bus_get_fw_dev_path(DeviceState *dev)
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index f2043a6..684a5da 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -8,6 +8,7 @@
> >  #include "qom/object.h"
> >  #include "hw/irq.h"
> >  #include "qapi/error.h"
> > +#include "hw/hotplug.h"
> >
> >  enum {
> >      DEV_NVECTORS_UNSPECIFIED = -1,
> > @@ -169,14 +170,18 @@ typedef struct BusChild {
> >      QTAILQ_ENTRY(BusChild) sibling;
> >  } BusChild;
> >
> > +#define QDEV_HOTPLUG_HANDLER_PROPERTY "hotplug-handler"
> > +
> >  /**
> >   * BusState:
> > + * @hotplug_device: link to a hotplug device associated with bus.
> >   */
> >  struct BusState {
> >      Object obj;
> >      DeviceState *parent;
> >      const char *name;
> >      int allow_hotplug;
> > +    HotplugHandler *hotplug_handler;
> >      int max_index;
> >      QTAILQ_HEAD(ChildrenHead, BusChild) children;
> >      QLIST_ENTRY(BusState) sibling;
> > --
> > 1.8.3.1
> >
> >

Re: [Qemu-devel] [PATCH 04/11] qdev: add to BusState "hotplug-handler" link

[Igor Mammedov]

On Mon, 16 Dec 2013 16:26:25 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Sat, 14 Dec 2013 17:05:57 +1000
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
[...]
> > >
> > >      QTAILQ_INIT(&bus->children);
> > > +    object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY,
> > > +                             TYPE_HOTPLUG_HANDLER,
> > > +                             (Object **)&bus->hotplug_handler, NULL);
> > 
> > I think failure of this is fatal. When the patches go through,
> > probably want to &error_abort.
[...]
> 
> But more to the point, bus could be created during hotplug, and
> we do not want to abort guest, instead hotplug operation should fail.
> In this case following setting of QDEV_HOTPLUG_HANDLER_PROPERTY
> will fail if object_property_add_link() failed before.

In following patches there is a couple places asking for being replaced
with &error_abort, so if your patches get in before respin, I'll gladly
use &error_abort there.