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

[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 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 file 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.8.3.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 file changed, 4 insertions(+)

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.8.3.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(+)
 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.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

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

[Igor Mammedov]

Currently it's possible to make PCIDevice not hotpluggable 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 "hotpluggable" readonly property, to make
it possible to get it via QOM interface.

Make DeviceClass hotpluggable by default as it was assumed before.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
* s/hotplugable/hotpluggable/

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

* 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..9418fea 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->hotpluggable) {
+        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->hotpluggable) {
+        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_hotpluggable(Object *obj, Error **err)
+{
+    DeviceClass *dc = DEVICE_GET_CLASS(obj);
+    DeviceState *dev = DEVICE(obj);
+
+    return dc->hotpluggable && 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, "hotpluggable",
+                             device_get_hotpluggable, 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 hotpluggable,
+     * so with intent to check it in generic qdev_unplug() /
+     * device_set_realized() functions make every device
+     * hotpluggable. Devices that shouldn't be hoplugable,
+     * should override it in their class_init()
+     */
+    klass->hotpluggable = true;
 }
 
 static void device_unparent(Object *obj)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 684a5da..04bbef4 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.
+ * @hotpluggable: booleean indicating if #DeviceClass is hotpluggable, available
+ * as readonly "hotpluggable" 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 hotpluggable;
 
     /* callbacks */
     void (*reset)(DeviceState *dev);
-- 
1.8.3.1

[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 file 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.8.3.1

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

[Igor Mammedov]

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

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
* move generic hotplug checks to
  "qdev: add "hotpluggable" property to Device" patch
* s/hotplugable/hotpluggable/
---
 hw/acpi/piix4.c         | 10 +++++-----
 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            | 11 +----------
 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 ---
 15 files changed, 23 insertions(+), 35 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 8084a60..fff2126 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->hotpluggable) {
                 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->hotpluggable) {
             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->hotpluggable = false;
 }
 
 static const TypeInfo piix4_pm_info = {
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index e4c345f..3a8fc0b 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->hotpluggable = false;
 }
 
 static const TypeInfo cirrus_vga_info = {
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index efdefd6..abb45ce 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->hotpluggable = false;
 }
 
 static const TypeInfo qxl_primary_info = {
diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index b3a45c8..f74fc43 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->hotpluggable = false;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
 }
 
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index aba292c..334e718 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->hotpluggable = false;
     set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
 }
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index befc39f..821589d 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->hotpluggable) {
             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..df127dd 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->hotpluggable = 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->hotpluggable = false;
 }
 
 static const TypeInfo piix4_ide_info = {
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index 1a1d451..53b15bc 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->hotpluggable = false;
 }
 
 static const TypeInfo piix4_info = {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index edc974e..4cdaabc 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->hotpluggable   = 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->hotpluggable   = 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->hotpluggable   = false;
 }
 
 static const TypeInfo i440fx_info = {
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 49eca95..8a7a21b 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);
 }
diff --git a/hw/usb/hcd-ehci-pci.c b/hw/usb/hcd-ehci-pci.c
index 0c98594..484a9bd 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->hotpluggable = 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..3d35058 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->hotpluggable = false;
 }
 
 static const TypeInfo ohci_pci_info = {
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 238d1d2..ad814b5 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->hotpluggable = 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..0fa814e 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->hotpluggable   = 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.8.3.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 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index fff2126..857d039 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->hotpluggable = 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.8.3.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.8.3.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.8.3.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 9418fea..7711c76 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -213,7 +213,6 @@ void qdev_unplug(DeviceState *dev, Error **errp)
         error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
         return;
     }
-    assert(dc->unplug != NULL);
 
     if (!dc->hotpluggable) {
         error_set(errp, QERR_DEVICE_NO_HOTPLUG,
@@ -223,9 +222,13 @@ void qdev_unplug(DeviceState *dev, Error **errp)
 
     qdev_hot_removed = true;
 
-    if (dc->unplug(dev) < 0) {
-        error_set(errp, QERR_UNDEFINED_ERROR);
-        return;
+    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
+    } else {
+        assert(dc->unplug != NULL);
+        if (dc->unplug(dev) < 0) { /* legacy handler */
+            error_set(errp, QERR_UNDEFINED_ERROR);
+        }
     }
 }
 
@@ -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 8a7a21b..6642e4c 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.8.3.1

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

[Peter Crosthwaite]

On Fri, Dec 13, 2013 at 10:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> 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 file changed, 4 insertions(+)
>
> 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;

This seems to be a change in terminology i.e. s/type/class. Should it
be enumerating_types or enumerating_type_table?

Also blank line here between global var decl and fn.

Otherwise,

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Regards,
Peter

>  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.8.3.1
>
>

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

[Peter Crosthwaite]

On Fri, Dec 13, 2013 at 10:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> Provide generic hotplug interface for devices.

"Provide a". s/devices/hotplug handlers would be cleaner too, to match
your v2 changes.

> 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(+)
>  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);
> +    }

So does it mean anything to have a hotplug handler device that can't
plug? Im thinking this API definition should be compusorly and
throw-errors/assert rather than silent fail.

> +}
> +
> +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 {

/* <private> */

> +    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.

Not sure if it makes sense to publicly document the QOM parent
first-field. It should not be used in code from this context.

> + * @plug: plug callback.
> + * @unplug: unplug callback.
> + */
> +typedef struct HotplugHandlerClass {

/* <private> */

> +    InterfaceClass parent;
> +

/* <public> */

> +    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.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 05/11] qdev: add "hotpluggable" property to Device

[Peter Crosthwaite]

On Fri, Dec 13, 2013 at 10:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> Currently it's possible to make PCIDevice not hotpluggable 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 "hotpluggable" readonly property, to make
> it possible to get it via QOM interface.
>
> Make DeviceClass hotpluggable by default as it was assumed before.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
> * s/hotplugable/hotpluggable/
>
> v3:
> * make DeviceClass hotpluggable by default
>   Since PCIDevice still uses internal no_hotlpug checks it shouldn't
>   reggress. And follow up patch that converts PCIDevices to use
>   "hotpluggable" property will take care about not hotpluggable PCI
>   devices explicitly setting "hotpluggable" to false in their class_init().
>
> * 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..9418fea 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->hotpluggable) {
> +        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->hotpluggable) {
> +        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_hotpluggable(Object *obj, Error **err)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> +    DeviceState *dev = DEVICE(obj);
> +
> +    return dc->hotpluggable && 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, "hotpluggable",
> +                             device_get_hotpluggable, 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 hotpluggable,
> +     * so with intent to check it in generic qdev_unplug() /
> +     * device_set_realized() functions make every device
> +     * hotpluggable. Devices that shouldn't be hoplugable,
> +     * should override it in their class_init()
> +     */
> +    klass->hotpluggable = true;
>  }
>
>  static void device_unparent(Object *obj)
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 684a5da..04bbef4 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.
> + * @hotpluggable: booleean indicating if #DeviceClass is hotpluggable, available

"boolean". Although the fact that its bool is implicit just by saying:

"@hotpluggable: indicates if #DeviceClass is hotpluggable ...

Regards,
Peter

> + * as readonly "hotpluggable" 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 hotpluggable;
>
>      /* callbacks */
>      void (*reset)(DeviceState *dev);
> --
> 1.8.3.1
>
>

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

[Andreas Färber]

Am 14.12.2013 07:53, schrieb Peter Crosthwaite:
> On Fri, Dec 13, 2013 at 10:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
>> 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 file changed, 4 insertions(+)
>>
>> 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;
> 
> This seems to be a change in terminology i.e. s/type/class. Should it
> be enumerating_types or enumerating_type_table?

Too many cooks... While offline, I picked up the original version,
changing to enumerating_types, which Alexey also happened to choose, and
then adopted his drop of "false" assignment.

> Also blank line here between global var decl and fn.

Same thought here, already did in my version. :)

> Otherwise,
> 
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Thanks, will update while rebasing.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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 03/11] define hotplug interface

[Igor Mammedov]

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

> On Fri, Dec 13, 2013 at 10:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > Provide generic hotplug interface for devices.
> 
> "Provide a". s/devices/hotplug handlers would be cleaner too, to match
> your v2 changes.
sure

> 
> > 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(+)
> >  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);
> > +    }
> 
> So does it mean anything to have a hotplug handler device that can't
> plug? Im thinking this API definition should be compusorly and
> throw-errors/assert rather than silent fail.
It doesn't have "fail" semantic but rather "handler doesn't need to do
anything for plug".
And although in current usage we always have plug/unplug pair,
not asserting would be more flexible from POV of generic API.

> 
> > +}
> > +
> > +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 {
> 
> /* <private> */
> 
> > +    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.
> 
> Not sure if it makes sense to publicly document the QOM parent
> first-field. It should not be used in code from this context.
I'd be glad to, but I think Andreas asked me once to do so for "parent"
in DimmDevice class. I also no sure that is necessary to document this
field in every child.

> 
> > + * @plug: plug callback.
> > + * @unplug: unplug callback.
> > + */
> > +typedef struct HotplugHandlerClass {
> 
> /* <private> */
> 
> > +    InterfaceClass parent;
> > +
> 
> /* <public> */
> 
> > +    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.8.3.1
> >
> >

I'll ally the rest of changes on next respin, thanks for review!

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

[Igor Mammedov]

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

> On Fri, Dec 13, 2013 at 10:44 PM, Igor Mammedov <imammedo@redhat.com> wrote:
> > Currently it's possible to make PCIDevice not hotpluggable 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 "hotpluggable" readonly property, to make
> > it possible to get it via QOM interface.
> >
> > Make DeviceClass hotpluggable by default as it was assumed before.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v4:
> > * s/hotplugable/hotpluggable/
> >
> > v3:
> > * make DeviceClass hotpluggable by default
> >   Since PCIDevice still uses internal no_hotlpug checks it shouldn't
> >   reggress. And follow up patch that converts PCIDevices to use
> >   "hotpluggable" property will take care about not hotpluggable PCI
> >   devices explicitly setting "hotpluggable" to false in their class_init().
> >
> > * 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..9418fea 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->hotpluggable) {
> > +        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->hotpluggable) {
> > +        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_hotpluggable(Object *obj, Error **err)
> > +{
> > +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> > +    DeviceState *dev = DEVICE(obj);
> > +
> > +    return dc->hotpluggable && 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, "hotpluggable",
> > +                             device_get_hotpluggable, 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 hotpluggable,
> > +     * so with intent to check it in generic qdev_unplug() /
> > +     * device_set_realized() functions make every device
> > +     * hotpluggable. Devices that shouldn't be hoplugable,
> > +     * should override it in their class_init()
> > +     */
> > +    klass->hotpluggable = true;
> >  }
> >
> >  static void device_unparent(Object *obj)
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 684a5da..04bbef4 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.
> > + * @hotpluggable: booleean indicating if #DeviceClass is hotpluggable, available
> 
> "boolean". Although the fact that its bool is implicit just by saying:
> 
> "@hotpluggable: indicates if #DeviceClass is hotpluggable ...
sure

> 
> Regards,
> Peter
> 
> > + * as readonly "hotpluggable" 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 hotpluggable;
> >
> >      /* callbacks */
> >      void (*reset)(DeviceState *dev);
> > --
> > 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.

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

[Anthony Liguori]

On Fri, Dec 13, 2013 at 4:44 AM, Igor Mammedov <imammedo@redhat.com> wrote:
> Currently it's possible to make PCIDevice not hotpluggable 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 "hotpluggable" readonly property, to make
> it possible to get it via QOM interface.
>
> Make DeviceClass hotpluggable by default as it was assumed before.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Pushing stuff to a base class in order to reuse code isn't the right
way to model things.

hotplug is a bus concept.  It doesn't apply widely to all possible devices.

Regards,

Anthony Liguori

> ---
> v4:
> * s/hotplugable/hotpluggable/
>
> v3:
> * make DeviceClass hotpluggable by default
>   Since PCIDevice still uses internal no_hotlpug checks it shouldn't
>   reggress. And follow up patch that converts PCIDevices to use
>   "hotpluggable" property will take care about not hotpluggable PCI
>   devices explicitly setting "hotpluggable" to false in their class_init().
>
> * 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..9418fea 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->hotpluggable) {
> +        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->hotpluggable) {
> +        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_hotpluggable(Object *obj, Error **err)
> +{
> +    DeviceClass *dc = DEVICE_GET_CLASS(obj);
> +    DeviceState *dev = DEVICE(obj);
> +
> +    return dc->hotpluggable && 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, "hotpluggable",
> +                             device_get_hotpluggable, 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 hotpluggable,
> +     * so with intent to check it in generic qdev_unplug() /
> +     * device_set_realized() functions make every device
> +     * hotpluggable. Devices that shouldn't be hoplugable,
> +     * should override it in their class_init()
> +     */
> +    klass->hotpluggable = true;
>  }
>
>  static void device_unparent(Object *obj)
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 684a5da..04bbef4 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.
> + * @hotpluggable: booleean indicating if #DeviceClass is hotpluggable, available
> + * as readonly "hotpluggable" 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 hotpluggable;
>
>      /* callbacks */
>      void (*reset)(DeviceState *dev);
> --
> 1.8.3.1
>

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

[Anthony Liguori]

Igor Mammedov <imammedo@redhat.com> writes:

> 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.

I'm having a hard time parsing this description.

Sharing hot plug code is a good thing.  Making hotplug a qdev-level
concept seems like a bad thing to me.

The series is a net add of code so I don't think we're winning anything
by generalizing here.

Is there a use-case this enables that isn't possible today?

Regards,

Anthony Liguori

>
> 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

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

[Peter Crosthwaite]

On Tue, Dec 17, 2013 at 9:26 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
>
>> 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.
>
> I'm having a hard time parsing this description.
>
> Sharing hot plug code is a good thing.  Making hotplug a qdev-level
> concept seems like a bad thing to me.
>

So hotplug seems sane as a bus level concept to me. The problem is
busses are a collection of general devices so the bus concept is lost
when you move from the container to the contained. Are we missing an
absraction layer here - TYPE_BUS_DEVICE?

> The series is a net add of code so I don't think we're winning anything
> by generalizing here.
>
> Is there a use-case this enables that isn't possible today?
>

Has some potential embedded applications is we want to use it for
power-down/up emulation. Exactly what such a sysbus hotplug handler
would look like is an open question.

Regards,
Peter

> Regards,
>
> Anthony Liguori
>
>>
>> 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
>

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

[Igor Mammedov]

On Mon, 16 Dec 2013 15:26:37 -0800
Anthony Liguori <anthony@codemonkey.ws> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > 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.
> 
> I'm having a hard time parsing this description.
> 
> Sharing hot plug code is a good thing.  Making hotplug a qdev-level
> concept seems like a bad thing to me.
we could add TYPE_BUS_DEVICE as Peter suggests but let me explain why
I've chosen DEVICE for it.
All current hotplug code depends on hotpluggable bus but periodically
there were suggestions to use link<>s for hotplug instead of bus.
Making hotplug the bus device only concept will not allow to move in 
direction of hotplug with links which are/were supposed to replace
buses in future.
As side effect series lays ground work for link based hotplug, it's
not complete but a step towards it. It still doesn't allow link based
hotplug, since purpose was to have unified hotplug interface across
PCI/SCSI/virtio devices/buses. 

Maybe we need TYPE_HOTPLUGGABLE_DEVICE so it could be abstracted from
bus as well and eventually it could be used with bussless devices?

> 
> The series is a net add of code so I don't think we're winning anything
> by generalizing here.
it will allow to remove different implementations of hotplug
mechanism in SCSI/virtio and maybe USB buses in addition to converted
PCI hotplug.

> Is there a use-case this enables that isn't possible today?
It was prompted by memory hotplug, alternative was that it can
re-implement hotplug mechanism in its own way or duplicating code
from scsi or pci buses/devices. Neither alternatives look nice when
there is a common usage pattern.

> Regards,
> 
> Anthony Liguori
> 
> >
> > 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

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

[Paolo Bonzini]

Il 17/12/2013 00:26, Anthony Liguori ha scritto:
> Sharing hot plug code is a good thing.  Making hotplug a qdev-level
> concept seems like a bad thing to me.

Can you explain what you mean?

> The series is a net add of code so I don't think we're winning anything
> by generalizing here.

Any generalization that's used just once will be a net add of code (and
this code will be reused by SCSI and x86 memory hotplug at least;
perhaps x86 CPU hotplug too).

Any generalization that requires some boilerplate code will be a net add
of code, too.  QEMU being written in C, we unfortunately cannot avoid that.

So I don't think that lines of code are a good metric.

Paolo

> Is there a use-case this enables that isn't possible today?
> 
> Regards,
> 
> Anthony Liguori
> 
>>
>> 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

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

[Anthony Liguori]

On Tue, Dec 17, 2013 at 4:38 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 17/12/2013 00:26, Anthony Liguori ha scritto:
>> Sharing hot plug code is a good thing.  Making hotplug a qdev-level
>> concept seems like a bad thing to me.
>
> Can you explain what you mean?

The question is whether "hotpluggable" as a property applies to all
devices or not.

But hotplug is strictly a bus level concept.  It's a sequence of
events that correspond to what happens when you add a new device to a
bus after power on.

>> The series is a net add of code so I don't think we're winning anything
>> by generalizing here.
>
> Any generalization that's used just once will be a net add of code (and
> this code will be reused by SCSI and x86 memory hotplug at least;
> perhaps x86 CPU hotplug too).

The question is whether there can be code sharing without touching the
base class.  You could certainly have a HotpluggableBusState and then
a HotpluggableDeviceState.

Interfaces would be another option too.

> Any generalization that requires some boilerplate code will be a net add
> of code, too.  QEMU being written in C, we unfortunately cannot avoid that.
>
> So I don't think that lines of code are a good metric.

The general concern is about polluting widely used base classes.  It's
better if we can avoid adding things to DeviceState and Object
whenever possible.

Regards,

Anthony Liguori

> Paolo
>
>> Is there a use-case this enables that isn't possible today?
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>
>>> 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
>

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

[Paolo Bonzini]

Il 17/12/2013 20:38, Anthony Liguori ha scritto:
> On Tue, Dec 17, 2013 at 4:38 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 17/12/2013 00:26, Anthony Liguori ha scritto:
>>> Sharing hot plug code is a good thing.  Making hotplug a qdev-level
>>> concept seems like a bad thing to me.
>>
>> Can you explain what you mean?
> 
> The question is whether "hotpluggable" as a property applies to all
> devices or not.
> 
> But hotplug is strictly a bus level concept.  It's a sequence of
> events that correspond to what happens when you add a new device to a
> bus after power on.

Hotplugging a device is a special case of plugging a device.  If a bus
or device only supports cold-plug, that can be done using
"bc->allow_hotplug = false" or "dc->hotpluggable = false".

Igor's interface applies just as well to the case of plugging a device
at startup; I think separating the two makes little sense.  And once you
have cold-plug and hot-plug in qdev core, it makes sense to add unplug
as well.  Also because we already have surprise removal in qdev core
(that's unparent) and we have some kind of unplug request support
(device_del/dc->unplug).

One possibility that remains is to put cold/hot-plug in a "BusDevice"
class rather than in the core qdev:

    Device
      BusDevice    <-- can be cold/hot-plugged

but this adds more complication.  For example, the same CPU can be
hotpluggable or not depending on the board model, should the superclass
be Device or BusDevice.  And if we ever have multi-CPU targets, with the
"core" CPU not hotpluggable and additional hotpluggable ones (e.g. for
GPUs) what would be the superclass of the common CPU superclass?

> The question is whether there can be code sharing without touching the
> base class.  You could certainly have a HotpluggableBusState and then
> a HotpluggableDeviceState.
> 
> Interfaces would be another option too.

Interfaces are fine, but the question is who finds them and calls them.
 In this case, the discovery mechanism is a link property, and the
calling mechanism is an explicit hook in the "realized" property.

If we had aspect-oriented programming, we would be marking join points
instead of writing "if (dev->foo) bar(dev->foo)" conditionals.  But the
idea is the same.

> The general concern is about polluting widely used base classes.  It's
> better if we can avoid adding things to DeviceState and Object
> whenever possible.

I agree.  At the same time we should make base classes as small as
possible, but not smaller than that.

Paolo

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

[Igor Mammedov]

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

> Il 17/12/2013 20:38, Anthony Liguori ha scritto:
> > On Tue, Dec 17, 2013 at 4:38 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> Il 17/12/2013 00:26, Anthony Liguori ha scritto:
> >>> Sharing hot plug code is a good thing.  Making hotplug a qdev-level
> >>> concept seems like a bad thing to me.
> >>
> >> Can you explain what you mean?
> > 
> > The question is whether "hotpluggable" as a property applies to all
> > devices or not.
I think Andreas asked me to provide "hotpluggable" property to
distinguish hotpluggable vs not hotpluggable DimmDevice via qom interface.

> > 
> > But hotplug is strictly a bus level concept.  It's a sequence of
> > events that correspond to what happens when you add a new device to a
> > bus after power on.
> 
> Hotplugging a device is a special case of plugging a device.  If a bus
> or device only supports cold-plug, that can be done using
> "bc->allow_hotplug = false" or "dc->hotpluggable = false".
Do we need per instance ability to set "hotpluggable" property?
For example board might want to mark some CPUs as not hotpluggable.

> 
> Igor's interface applies just as well to the case of plugging a device
> at startup; I think separating the two makes little sense.  And once you
> have cold-plug and hot-plug in qdev core, it makes sense to add unplug
> as well.  Also because we already have surprise removal in qdev core
> (that's unparent) and we have some kind of unplug request support
> (device_del/dc->unplug).
> 
> One possibility that remains is to put cold/hot-plug in a "BusDevice"
> class rather than in the core qdev:
> 
>     Device
>       BusDevice    <-- can be cold/hot-plugged
> 
> but this adds more complication.  For example, the same CPU can be
> hotpluggable or not depending on the board model, should the superclass
> be Device or BusDevice.  And if we ever have multi-CPU targets, with the
> "core" CPU not hotpluggable and additional hotpluggable ones (e.g. for
> GPUs) what would be the superclass of the common CPU superclass?
> 
> > The question is whether there can be code sharing without touching the
> > base class.  You could certainly have a HotpluggableBusState and then
> > a HotpluggableDeviceState.
> > 
> > Interfaces would be another option too.
> 
> Interfaces are fine, but the question is who finds them and calls them.
>  In this case, the discovery mechanism is a link property, and the
> calling mechanism is an explicit hook in the "realized" property.
If we don't need per instance "hotpluggable" state and we can call
interfaces from generic qdev/device code, then we would need at first
only TYPE_HOTPLUGGABLE_BUS_DEVICE_IF and later for link<> based hotplug
we could add just TYPE_HOTPLUGGABLE_DEVICE_IF. Difference would be in
the way they get access to hotplug device link, former one will use bus
for it and second some other way.

> 
> If we had aspect-oriented programming, we would be marking join points
> instead of writing "if (dev->foo) bar(dev->foo)" conditionals.  But the
> idea is the same.
> 
> > The general concern is about polluting widely used base classes.  It's
> > better if we can avoid adding things to DeviceState and Object
> > whenever possible.
> 
> I agree.  At the same time we should make base classes as small as
> possible, but not smaller than that.
> 
> Paolo
> 

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

[Paolo Bonzini]

Il 18/12/2013 16:48, Igor Mammedov ha scritto:
>> Hotplugging a device is a special case of plugging a device.  If a bus
>> or device only supports cold-plug, that can be done using
>> "bc->allow_hotplug = false" or "dc->hotpluggable = false".
> Do we need per instance ability to set "hotpluggable" property?
> For example board might want to mark some CPUs as not hotpluggable.

I think such fine-grained control could be handled from dc->unplug.
Let's not do anything more than we need, until we need it.

Right now, all we need to model is the fact that a device X can act as
hotplug controller for multiple buses, X is not the parent of those
buses, and we need to tell those buses about X.  This is hotplug-handler
in BusState.

We also like to distinguish devices that only support -device (or are
even only board-instantiatable) from devices that support device_add.
This is dc->hotpluggable.  It is not absolutely necessary, but it makes
sense if "plugging" gets a more central place in qdev.  This is the case
after you add hotplug-handler.

>>> Interfaces would be another option too.
>>
>> Interfaces are fine, but the question is who finds them and calls them.
>> In this case, the discovery mechanism is a link property, and the
>> calling mechanism is an explicit hook in the "realized" property.
>
> If we don't need per instance "hotpluggable" state and we can call
> interfaces from generic qdev/device code, then we would need at first
> only TYPE_HOTPLUGGABLE_BUS_DEVICE_IF and later for link<> based hotplug
> we could add just TYPE_HOTPLUGGABLE_DEVICE_IF. Difference would be in
> the way they get access to hotplug device link, former one will use bus
> for it and second some other way.

I think this is overengineered.  What you have is flexible and decently
clean.  I don't think there's any need to go from the device to the bus
and from there optionally to the handler.  Most of the time you couldn't
do anything really in the bus, you would have to call some sort of
bus-specific callback (like SCSIBusInfo).  It is then equivalent to set
the bus's parent device as a (hot)plug handler and go straight from the
device to the handler, as in your patches.

Paolo

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

[Michael S. Tsirkin]

On Wed, Dec 18, 2013 at 04:48:09PM +0100, Igor Mammedov wrote:
> On Wed, 18 Dec 2013 11:36:52 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > Il 17/12/2013 20:38, Anthony Liguori ha scritto:
> > > On Tue, Dec 17, 2013 at 4:38 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >> Il 17/12/2013 00:26, Anthony Liguori ha scritto:
> > >>> Sharing hot plug code is a good thing.  Making hotplug a qdev-level
> > >>> concept seems like a bad thing to me.
> > >>
> > >> Can you explain what you mean?
> > > 
> > > The question is whether "hotpluggable" as a property applies to all
> > > devices or not.
> I think Andreas asked me to provide "hotpluggable" property to
> distinguish hotpluggable vs not hotpluggable DimmDevice via qom interface.
> 
> > > 
> > > But hotplug is strictly a bus level concept.  It's a sequence of
> > > events that correspond to what happens when you add a new device to a
> > > bus after power on.
> > 
> > Hotplugging a device is a special case of plugging a device.  If a bus
> > or device only supports cold-plug, that can be done using
> > "bc->allow_hotplug = false" or "dc->hotpluggable = false".
> Do we need per instance ability to set "hotpluggable" property?
> For example board might want to mark some CPUs as not hotpluggable.

It could be useful.
In real life same device can be on-board or on a plugin card.
But it's not a must, we survived without this so far.

So maybe start not supporting it, add later?

> > 
> > Igor's interface applies just as well to the case of plugging a device
> > at startup; I think separating the two makes little sense.  And once you
> > have cold-plug and hot-plug in qdev core, it makes sense to add unplug
> > as well.  Also because we already have surprise removal in qdev core
> > (that's unparent) and we have some kind of unplug request support
> > (device_del/dc->unplug).
> > 
> > One possibility that remains is to put cold/hot-plug in a "BusDevice"
> > class rather than in the core qdev:
> > 
> >     Device
> >       BusDevice    <-- can be cold/hot-plugged
> > 
> > but this adds more complication.  For example, the same CPU can be
> > hotpluggable or not depending on the board model, should the superclass
> > be Device or BusDevice.  And if we ever have multi-CPU targets, with the
> > "core" CPU not hotpluggable and additional hotpluggable ones (e.g. for
> > GPUs) what would be the superclass of the common CPU superclass?
> > 
> > > The question is whether there can be code sharing without touching the
> > > base class.  You could certainly have a HotpluggableBusState and then
> > > a HotpluggableDeviceState.
> > > 
> > > Interfaces would be another option too.
> > 
> > Interfaces are fine, but the question is who finds them and calls them.
> >  In this case, the discovery mechanism is a link property, and the
> > calling mechanism is an explicit hook in the "realized" property.
> If we don't need per instance "hotpluggable" state and we can call
> interfaces from generic qdev/device code, then we would need at first
> only TYPE_HOTPLUGGABLE_BUS_DEVICE_IF and later for link<> based hotplug
> we could add just TYPE_HOTPLUGGABLE_DEVICE_IF. Difference would be in
> the way they get access to hotplug device link, former one will use bus
> for it and second some other way.
> 
> > 
> > If we had aspect-oriented programming, we would be marking join points
> > instead of writing "if (dev->foo) bar(dev->foo)" conditionals.  But the
> > idea is the same.
> > 
> > > The general concern is about polluting widely used base classes.  It's
> > > better if we can avoid adding things to DeviceState and Object
> > > whenever possible.
> > 
> > I agree.  At the same time we should make base classes as small as
> > possible, but not smaller than that.
> > 
> > Paolo
> > 

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

[Igor Mammedov]

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

> Il 18/12/2013 16:48, Igor Mammedov ha scritto:
> >> Hotplugging a device is a special case of plugging a device.  If a bus
> >> or device only supports cold-plug, that can be done using
> >> "bc->allow_hotplug = false" or "dc->hotpluggable = false".
> > Do we need per instance ability to set "hotpluggable" property?
> > For example board might want to mark some CPUs as not hotpluggable.
> 
> I think such fine-grained control could be handled from dc->unplug.
> Let's not do anything more than we need, until we need it.
I had in mind a lightweight conversion of initial memory to DimmDevices
for i386 target and disabling its unplug until initial memory unplug
would be properly handled by board and SeaBIOS.

> 
> Paolo

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

[Igor Mammedov]

On Wed, 18 Dec 2013 18:26:07 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Dec 18, 2013 at 04:48:09PM +0100, Igor Mammedov wrote:
> > On Wed, 18 Dec 2013 11:36:52 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> > > Il 17/12/2013 20:38, Anthony Liguori ha scritto:
> > > > On Tue, Dec 17, 2013 at 4:38 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > >> Il 17/12/2013 00:26, Anthony Liguori ha scritto:
> > > >>> Sharing hot plug code is a good thing.  Making hotplug a qdev-level
> > > >>> concept seems like a bad thing to me.
> > > >>
> > > >> Can you explain what you mean?
> > > > 
> > > > The question is whether "hotpluggable" as a property applies to all
> > > > devices or not.
> > I think Andreas asked me to provide "hotpluggable" property to
> > distinguish hotpluggable vs not hotpluggable DimmDevice via qom interface.
> > 
> > > > 
> > > > But hotplug is strictly a bus level concept.  It's a sequence of
> > > > events that correspond to what happens when you add a new device to a
> > > > bus after power on.
> > > 
> > > Hotplugging a device is a special case of plugging a device.  If a bus
> > > or device only supports cold-plug, that can be done using
> > > "bc->allow_hotplug = false" or "dc->hotpluggable = false".
> > Do we need per instance ability to set "hotpluggable" property?
> > For example board might want to mark some CPUs as not hotpluggable.
> 
> It could be useful.
> In real life same device can be on-board or on a plugin card.
> But it's not a must, we survived without this so far.
> 
> So maybe start not supporting it, add later?
> 
Yes, that's surely could be done later

> > > 
> > > Paolo
> > >