All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
@ 2014-02-05 15:36 Igor Mammedov
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 1/9] define hotplug interface Igor Mammedov
                   ` (9 more replies)
  0 siblings, 10 replies; 15+ messages in thread
From: Igor Mammedov @ 2014-02-05 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, mst,
	jan.kiszka, marcel.a, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini, afaerber

changes since v5:
 - fixed typos Eric pointed to
 - fix conflict of [5/9] with new patch "acpi-build: append description for non-hotplug"
 - rebased on top of todays PCI tree

changes since v4:
 - rebased on top of PCI tree
 - added wrapper to set hotplug-device property

changes since v3:
 - fixup/add comments as reqused by  Peter Crosthwaite
 - use error_abort to reduce error handling verbosity
 - fix tests/test-qdev-global-props build failure on make check
 - rebase on top of current master:133fe7743 (with interface fixes)

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_v6

tested only ACPI and PCIE hotplug, since SHPC is not functional, tested
it under debugger that hotplhug event reaches handler and corresponding interrupt is emited.


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

 hw/acpi/pcihp.c                |   46 ++++++++++----
 hw/acpi/piix4.c                |  128 +++++++++++++++++++++-------------------
 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           |    4 +-
 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                  |   65 ++++++++++++--------
 hw/pci/pcie_port.c             |    8 +++
 hw/pci/shpc.c                  |  124 ++++++++++++++++++++++++---------------
 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/acpi/acpi.h         |    1 +
 include/hw/acpi/pcihp.h        |   10 ++-
 include/hw/hotplug.h           |   78 ++++++++++++++++++++++++
 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         |   15 +++++
 tests/Makefile                 |    2 +-
 31 files changed, 463 insertions(+), 222 deletions(-)
 create mode 100644 hw/core/hotplug.c
 create mode 100644 include/hw/hotplug.h

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v6 1/9] define hotplug interface
  2014-02-05 15:36 [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
@ 2014-02-05 15:36 ` Igor Mammedov
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 2/9] qdev: add to BusState "hotplug-handler" link Igor Mammedov
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2014-02-05 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, mst,
	jan.kiszka, marcel.a, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini, afaerber

Provide a generic hotplug interface for hotplug handlers.
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>
---
v4:
* s/2013/2014/ in copyright headers
v3:
* amend commit description as requested by Peter Crosthwaite
* add <private|public> doc comments to type definitions
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  |   78 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 0 deletions(-)
 create mode 100644 hw/core/hotplug.c
 create mode 100644 include/hw/hotplug.h

diff --git a/hw/core/Makefile.objs b/hw/core/Makefile.objs
index 950146c..9e324be 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -2,6 +2,7 @@
 common-obj-y += qdev.o qdev-properties.o
 # irq.o needed for qdev GPIO handling:
 common-obj-y += irq.o
+common-obj-y += hotplug.o
 
 common-obj-$(CONFIG_EMPTY_SLOT) += empty_slot.o
 common-obj-$(CONFIG_XILINX_AXI) += stream.o
diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
new file mode 100644
index 0000000..5573d9d
--- /dev/null
+++ b/hw/core/hotplug.c
@@ -0,0 +1,48 @@
+/*
+ * Hotplug handler interface.
+ *
+ * Copyright (c) 2014 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..a6533cb
--- /dev/null
+++ b/include/hw/hotplug.h
@@ -0,0 +1,78 @@
+/*
+ * Hotplug handler interface.
+ *
+ * Copyright (c) 2014 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.
+ * @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.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v6 2/9] qdev: add to BusState "hotplug-handler" link
  2014-02-05 15:36 [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 1/9] define hotplug interface Igor Mammedov
@ 2014-02-05 15:36 ` Igor Mammedov
  2014-02-05 15:52   ` Andreas Färber
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 3/9] qdev: add "hotpluggable" property to Device Igor Mammedov
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 15+ messages in thread
From: Igor Mammedov @ 2014-02-05 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, mst,
	jan.kiszka, marcel.a, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini, afaerber

It will allow to reuse field with different BUSes,
reducing code duplication. Field is intended for
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 field in qdev hotplug code.

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

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 82a9123..c9f0c33 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;
@@ -870,6 +871,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 2c4f140..41ec533 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,
@@ -180,14 +181,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;
@@ -321,4 +326,11 @@ extern int qdev_hotplug;
 
 char *qdev_get_dev_path(DeviceState *dev);
 
+static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
+                                            Error **errp)
+{
+    object_property_set_link(OBJECT(bus), OBJECT(handler),
+                             QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
+    bus->allow_hotplug = 1;
+}
 #endif
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v6 3/9] qdev: add "hotpluggable" property to Device
  2014-02-05 15:36 [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 1/9] define hotplug interface Igor Mammedov
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 2/9] qdev: add to BusState "hotplug-handler" link Igor Mammedov
@ 2014-02-05 15:36 ` Igor Mammedov
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 4/9] hw/acpi: move typeinfo to the file end Igor Mammedov
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2014-02-05 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, mst,
	jan.kiszka, marcel.a, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini, afaerber

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. Following
patches will replace PCIDeviceClass.no_hotplug with this
new property.

In addition expose field as "hotpluggable" readonly property,
to make it possible to read its value via QOM interface.

Make DeviceClass hotpluggable by default as it was assumed
before.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v6:
* s/hoplugable/hotpluggable/
v5:
* drop 'boolean' from doc comment of hotpluggable field
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(+), 0 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index c9f0c33..5c864db 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) {
@@ -694,6 +700,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;
@@ -734,6 +745,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);
@@ -750,6 +769,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 {
@@ -786,6 +807,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 hotpluggable,
+     * 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 41ec533..08d329d 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: indicates if #DeviceClass is hotpluggable, available
+ * as readonly "hotpluggable" property of #DeviceState instance
  *
  * # Realization #
  * Devices are constructed in two stages,
@@ -110,6 +112,7 @@ typedef struct DeviceClass {
      * TODO remove once we're there
      */
     bool cannot_instantiate_with_device_add_yet;
+    bool hotpluggable;
 
     /* callbacks */
     void (*reset)(DeviceState *dev);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v6 4/9] hw/acpi: move typeinfo to the file end
  2014-02-05 15:36 [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
                   ` (2 preceding siblings ...)
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 3/9] qdev: add "hotpluggable" property to Device Igor Mammedov
@ 2014-02-05 15:36 ` Igor Mammedov
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 5/9] qdev:pci: refactor PCIDevice to use generic "hotpluggable" property Igor Mammedov
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2014-02-05 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, mst,
	jan.kiszka, marcel.a, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini, afaerber

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

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
* fix typo in commit message s/usualy/usually/
---
 hw/acpi/piix4.c |   92 +++++++++++++++++++++++++++---------------------------
 1 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 7a0efcb..0f45e11 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -466,52 +466,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_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
-                     use_acpi_pci_hotplug, true),
-    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->vmsd = &vmstate_acpi;
-    dc->props = piix4_pm_properties;
-    /*
-     * Reason: part of PIIX4 southbridge, needs to be wired up,
-     * e.g. by mips_malta_init()
-     */
-    dc->cannot_instantiate_with_device_add_yet = true;
-}
-
-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;
@@ -566,3 +520,49 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
     s->cpu_added_notifier.notify = piix4_cpu_added_req;
     qemu_register_cpu_added_notifier(&s->cpu_added_notifier);
 }
+
+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_BOOL("acpi-pci-hotplug-with-bridge-support", PIIX4PMState,
+                     use_acpi_pci_hotplug, true),
+    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->vmsd = &vmstate_acpi;
+    dc->props = piix4_pm_properties;
+    /*
+     * Reason: part of PIIX4 southbridge, needs to be wired up,
+     * e.g. by mips_malta_init()
+     */
+    dc->cannot_instantiate_with_device_add_yet = true;
+}
+
+static const TypeInfo piix4_pm_info = {
+    .name          = TYPE_PIIX4_PM,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(PIIX4PMState),
+    .class_init    = piix4_pm_class_init,
+};
+
+static void piix4_pm_register_types(void)
+{
+    type_register_static(&piix4_pm_info);
+}
+
+type_init(piix4_pm_register_types)
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v6 5/9] qdev:pci: refactor PCIDevice to use generic "hotpluggable" property
  2014-02-05 15:36 [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
                   ` (3 preceding siblings ...)
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 4/9] hw/acpi: move typeinfo to the file end Igor Mammedov
@ 2014-02-05 15:36 ` Igor Mammedov
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 6/9] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API Igor Mammedov
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2014-02-05 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, mst,
	jan.kiszka, marcel.a, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini, afaerber

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/pcihp.c         |    3 ++-
 hw/acpi/piix4.c         |    2 +-
 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    |    4 +++-
 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 ---
 16 files changed, 21 insertions(+), 30 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 1ce6fc2..3bd5a06 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -105,12 +105,13 @@ static PCIBus *acpi_pcihp_find_hotplug_bus(AcpiPciHpState *s, int bsel)
 static bool acpi_pcihp_pc_no_hotplug(AcpiPciHpState *s, PCIDevice *dev)
 {
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
+    DeviceClass *dc = DEVICE_GET_CLASS(dev);
     /*
      * ACPI doesn't allow hotplug of bridge devices.  Don't allow
      * hot-unplug of bridge devices unless they were added by hotplug
      * (and so, not described by acpi).
      */
-    return (pc->is_bridge && !dev->qdev.hotplugged) || pc->no_hotplug;
+    return (pc->is_bridge && !dev->qdev.hotplugged) || !dc->hotpluggable;
 }
 
 static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slots)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 0f45e11..077776a 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -536,7 +536,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;
@@ -551,6 +550,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
      * e.g. by mips_malta_init()
      */
     dc->cannot_instantiate_with_device_add_yet = true;
+    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 e4f172e..ec82e00 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2299,7 +2299,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;
@@ -2310,6 +2309,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 6a43a7d..b388b21 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -788,6 +788,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
     memset(slot_device_present, 0x00, sizeof slot_device_present);
 
     for (i = 0; i < ARRAY_SIZE(bus->devices); ++i) {
+        DeviceClass *dc;
         PCIDeviceClass *pc;
         PCIDevice *pdev = bus->devices[i];
         int slot = PCI_SLOT(i);
@@ -798,8 +799,9 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
 
         set_bit(slot, slot_device_present);
         pc = PCI_DEVICE_GET_CLASS(pdev);
+        dc = DEVICE_GET_CLASS(pdev);
 
-        if (pc->no_hotplug || pc->is_bridge) {
+        if (!dc->hotpluggable || pc->is_bridge) {
             int slot = PCI_SLOT(i);
 
             clear_bit(slot, slot_hotplug_enable);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 9b5960b..0eda301 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -241,13 +241,13 @@ 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;
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+    dc->hotpluggable = false;
 }
 
 static const TypeInfo piix3_ide_info = {
@@ -280,13 +280,13 @@ 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;
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
     k->class_id = PCI_CLASS_STORAGE_IDE;
     set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+    dc->hotpluggable = false;
 }
 
 static const TypeInfo piix4_ide_info = {
diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
index def6fe3..492cd22 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;
@@ -119,6 +118,7 @@ static void piix4_class_init(ObjectClass *klass, void *data)
      * e.g. by mips_malta_init()
      */
     dc->cannot_instantiate_with_device_add_yet = true;
+    dc->hotpluggable = false;
 }
 
 static const TypeInfo piix4_info = {
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index e89d5c1..ffdc853 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -628,7 +628,7 @@ static void piix3_class_init(ObjectClass *klass, void *data)
 
     dc->desc        = "ISA bridge";
     dc->vmsd        = &vmstate_piix3;
-    k->no_hotplug   = 1;
+    dc->hotpluggable   = false;
     k->init         = piix3_initfn;
     k->config_write = piix3_write_config;
     k->vendor_id    = PCI_VENDOR_ID_INTEL;
@@ -656,7 +656,7 @@ static void piix3_xen_class_init(ObjectClass *klass, void *data)
 
     dc->desc        = "ISA bridge";
     dc->vmsd        = &vmstate_piix3;
-    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;
@@ -682,7 +682,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;
@@ -696,6 +695,7 @@ static void i440fx_class_init(ObjectClass *klass, void *data)
      * host-facing part, which can't be device_add'ed, yet.
      */
     dc->cannot_instantiate_with_device_add_yet = true;
+    dc->hotpluggable   = false;
 }
 
 static const TypeInfo i440fx_info = {
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 1221f32..d69961f 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1761,11 +1761,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) {
@@ -1800,12 +1796,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 5252346..c173b6a 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -201,9 +201,6 @@ typedef struct PCIDeviceClass {
     /* pcie stuff */
     int is_express;   /* is this device pci express? */
 
-    /* device isn't hot-pluggable */
-    int no_hotplug;
-
     /* rom bar */
     const char *romfile;
 } PCIDeviceClass;
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v6 6/9] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API
  2014-02-05 15:36 [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
                   ` (4 preceding siblings ...)
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 5/9] qdev:pci: refactor PCIDevice to use generic "hotpluggable" property Igor Mammedov
@ 2014-02-05 15:36 ` Igor Mammedov
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 7/9] pci/shpc: convert SHPC " Igor Mammedov
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2014-02-05 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, mst,
	jan.kiszka, marcel.a, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini, afaerber

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>
---
v3:
* s/Not supported/Unsupported/
* s/propery/property/
* drop trailing dot in the error message
v2:
* use error_abort to make error handling less verbose
---
 hw/acpi/pcihp.c         |   43 +++++++++++++++++++++++++++++++------------
 hw/acpi/piix4.c         |   36 ++++++++++++++++++++++--------------
 include/hw/acpi/acpi.h  |    1 +
 include/hw/acpi/pcihp.h |   10 ++++++----
 4 files changed, 60 insertions(+), 30 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 3bd5a06..f80c480 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -46,6 +46,7 @@
 # define ACPI_PCIHP_DPRINTF(format, ...)     do { } while (0)
 #endif
 
+#define ACPI_PCI_HOTPLUG_STATUS 2
 #define ACPI_PCIHP_ADDR 0xae00
 #define ACPI_PCIHP_SIZE 0x0014
 #define ACPI_PCIHP_LEGACY_SIZE 0x000f
@@ -179,29 +180,47 @@ void acpi_pcihp_reset(AcpiPciHpState *s)
     acpi_pcihp_update(s);
 }
 
-int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
-                              PCIHotplugState state)
+void acpi_pcihp_device_plug_cb(ACPIREGS *ar, qemu_irq irq, AcpiPciHpState *s,
+                               DeviceState *dev, Error **errp)
 {
-    int slot = PCI_SLOT(dev->devfn);
-    int bsel = acpi_pcihp_get_bsel(dev->bus);
+    PCIDevice *pdev = PCI_DEVICE(dev);
+    int slot = PCI_SLOT(pdev->devfn);
+    int bsel = acpi_pcihp_get_bsel(pdev->bus);
     if (bsel < 0) {
-        return -1;
+        error_setg(errp, "Unsupported bus. Bus doesn't have property '"
+                   ACPI_PCIHP_PROP_BSEL "' set");
+        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 (state == PCI_COLDPLUG_ENABLED) {
-        return 0;
+    if (!dev->hotplugged) {
+        return;
     }
 
-    if (state == PCI_HOTPLUG_ENABLED) {
-        s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
-    } else {
-        s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
+    s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
+
+    ar->gpe.sts[0] |= ACPI_PCI_HOTPLUG_STATUS;
+    acpi_update_sci(ar, irq);
+}
+
+void acpi_pcihp_device_unplug_cb(ACPIREGS *ar, qemu_irq irq, AcpiPciHpState *s,
+                                 DeviceState *dev, Error **errp)
+{
+    PCIDevice *pdev = PCI_DEVICE(dev);
+    int slot = PCI_SLOT(pdev->devfn);
+    int bsel = acpi_pcihp_get_bsel(pdev->bus);
+    if (bsel < 0) {
+        error_setg(errp, "Unsupported bus. Bus doesn't have property '"
+                   ACPI_PCIHP_PROP_BSEL "' set");
+        return;
     }
 
-    return 0;
+    s->acpi_pcihp_pci_status[bsel].down |= (1U << slot);
+
+    ar->gpe.sts[0] |= ACPI_PCI_HOTPLUG_STATUS;
+    acpi_update_sci(ar, irq);
 }
 
 static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 077776a..9f21653 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -32,6 +32,7 @@
 #include "hw/acpi/piix4.h"
 #include "hw/acpi/pcihp.h"
 #include "hw/acpi/cpu_hotplug.h"
+#include "hw/hotplug.h"
 
 //#define DEBUG
 
@@ -44,8 +45,6 @@
 #define GPE_BASE 0xafe0
 #define GPE_LEN 4
 
-#define PIIX4_PCI_HOTPLUG_STATUS 2
-
 struct pci_status {
     uint32_t up; /* deprecated, maintained for migration compatibility */
     uint32_t down;
@@ -311,24 +310,26 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
     acpi_pm1_evt_power_down(&s->ar);
 }
 
-static int piix4_acpi_pci_hotplug(DeviceState *qdev, PCIDevice *dev,
-                                  PCIHotplugState state)
+static void piix4_pci_device_plug_cb(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
 {
-    PIIX4PMState *s = PIIX4_PM(qdev);
-    int ret = acpi_pcihp_device_hotplug(&s->acpi_pci_hotplug, dev, state);
-    if (ret < 0) {
-        return ret;
-    }
-    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
+    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
+    acpi_pcihp_device_plug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev, errp);
+}
 
-    acpi_update_sci(&s->ar, s->irq);
-    return 0;
+static void piix4_pci_device_unplug_cb(HotplugHandler *hotplug_dev,
+                                       DeviceState *dev, Error **errp)
+{
+    PIIX4PMState *s = PIIX4_PM(hotplug_dev);
+    acpi_pcihp_device_unplug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev,
+                                errp);
 }
 
-static void piix4_update_bus_hotplug(PCIBus *bus, void *opaque)
+static void piix4_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
 {
     PIIX4PMState *s = opaque;
-    pci_bus_hotplug(bus, piix4_acpi_pci_hotplug, DEVICE(s));
+
+    qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s), &error_abort);
 }
 
 static void piix4_pm_machine_ready(Notifier *n, void *opaque)
@@ -535,6 +536,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;
@@ -551,6 +553,8 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
      */
     dc->cannot_instantiate_with_device_add_yet = true;
     dc->hotpluggable = false;
+    hc->plug = piix4_pci_device_plug_cb;
+    hc->unplug = piix4_pci_device_unplug_cb;
 }
 
 static const TypeInfo piix4_pm_info = {
@@ -558,6 +562,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)
diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index 3e53297..a9fae9d 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -24,6 +24,7 @@
 #include "qemu/notify.h"
 #include "qemu/option.h"
 #include "exec/memory.h"
+#include "hw/irq.h"
 
 /* from linux include/acpi/actype.h */
 /* Default ACPI register widths */
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 0a90e4a..9323838 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -29,7 +29,8 @@
 
 #include <inttypes.h>
 #include <qemu/typedefs.h>
-#include "hw/pci/pci.h" /* for PCIHotplugState */
+#include "hw/acpi/acpi.h"
+#include "migration/vmstate.h"
 
 typedef struct AcpiPciHpPciStatus {
     uint32_t up;
@@ -52,9 +53,10 @@ typedef struct AcpiPciHpState {
 void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
                      MemoryRegion *address_space_io, bool bridges_enabled);
 
-/* Invoke on device hotplug */
-int acpi_pcihp_device_hotplug(AcpiPciHpState *, PCIDevice *,
-                              PCIHotplugState state);
+void acpi_pcihp_device_plug_cb(ACPIREGS *ar, qemu_irq irq, AcpiPciHpState *s,
+                               DeviceState *dev, Error **errp);
+void acpi_pcihp_device_unplug_cb(ACPIREGS *ar, qemu_irq irq, AcpiPciHpState *s,
+                                 DeviceState *dev, Error **errp);
 
 /* Called on reset */
 void acpi_pcihp_reset(AcpiPciHpState *s);
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v6 7/9] pci/shpc: convert SHPC hotplug to use hotplug-handler API
  2014-02-05 15:36 [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
                   ` (5 preceding siblings ...)
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 6/9] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API Igor Mammedov
@ 2014-02-05 15:36 ` Igor Mammedov
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 8/9] pci/pcie: convert PCIE " Igor Mammedov
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2014-02-05 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, mst,
	jan.kiszka, marcel.a, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini, afaerber

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>
---
v2:
* drop error_is_set() and check local_error pointer directly
* keep original logic, i.e. do not abort in bridge init.
---
 hw/pci-bridge/pci_bridge_dev.c |    9 +++
 hw/pci/shpc.c                  |  124 +++++++++++++++++++++++++---------------
 include/hw/pci/shpc.h          |    8 +++
 3 files changed, 94 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..180faa7 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,65 +491,93 @@ 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 (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 (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. */
@@ -616,7 +645,8 @@ 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);
+
+    qbus_set_hotplug_handler(BUS(sec_bus), DEVICE(d), NULL);
 
     d->cap_present |= QEMU_PCI_CAP_SHPC;
     return 0;
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 467911a..eef1a1a 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -4,6 +4,8 @@
 #include "qemu-common.h"
 #include "exec/memory.h"
 #include "migration/vmstate.h"
+#include "qapi/error.h"
+#include "hw/hotplug.h"
 
 struct SHPCDevice {
     /* Capability offset in device's config space */
@@ -41,6 +43,12 @@ int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
 void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
 void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
 
+
+void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                            Error **errp);
+void shpc_device_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                               Error **errp);
+
 extern VMStateInfo shpc_vmstate_info;
 #define SHPC_VMSTATE(_field, _type) \
     VMSTATE_BUFFER_UNSAFE_INFO(_field, _type, 0, shpc_vmstate_info, 0)
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v6 8/9] pci/pcie: convert PCIE hotplug to use hotplug-handler API
  2014-02-05 15:36 [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
                   ` (6 preceding siblings ...)
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 7/9] pci/shpc: convert SHPC " Igor Mammedov
@ 2014-02-05 15:36 ` Igor Mammedov
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 9/9] hw/pci: switch to a generic hotplug handling for PCIDevice Igor Mammedov
  2014-02-10  9:09 ` [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Michael S. Tsirkin
  9 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2014-02-05 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, mst,
	jan.kiszka, marcel.a, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini, afaerber

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>
---
v2:
* keep original non abort behavior of pcie_cap_slot_init()
---
 hw/pci/pcie.c         |   65 ++++++++++++++++++++++++++++++-------------------
 hw/pci/pcie_port.c    |    8 ++++++
 include/hw/pci/pcie.h |    5 ++++
 3 files changed, 53 insertions(+), 25 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index ca60cf2..8ecd11e 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
@@ -305,8 +320,8 @@ 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);
+    qbus_set_hotplug_handler(BUS(pci_bridge_get_sec_bus(PCI_BRIDGE(dev))),
+                             DEVICE(dev), NULL);
 }
 
 void pcie_cap_slot_reset(PCIDevice *dev)
diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
index 2adb030..fa24877 100644
--- a/hw/pci/pcie_port.c
+++ b/hw/pci/pcie_port.c
@@ -19,6 +19,7 @@
  */
 
 #include "hw/pci/pcie_port.h"
+#include "hw/hotplug.h"
 
 void pcie_port_init_reg(PCIDevice *d)
 {
@@ -149,8 +150,11 @@ static Property pcie_slot_props[] = {
 static void pcie_slot_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
+    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
 
     dc->props = pcie_slot_props;
+    hc->plug = pcie_cap_slot_hotplug_cb;
+    hc->unplug = pcie_cap_slot_hot_unplug_cb;
 }
 
 static const TypeInfo pcie_slot_type_info = {
@@ -159,6 +163,10 @@ static const TypeInfo pcie_slot_type_info = {
     .instance_size = sizeof(PCIESlot),
     .abstract = true,
     .class_init = pcie_slot_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { }
+    }
 };
 
 static void pcie_port_register_types(void)
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 1966169..b0bf7e3 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -25,6 +25,7 @@
 #include "hw/pci/pci_regs.h"
 #include "hw/pci/pcie_regs.h"
 #include "hw/pci/pcie_aer.h"
+#include "hw/hotplug.h"
 
 typedef enum {
     /* for attention and power indicator */
@@ -122,4 +123,8 @@ extern const VMStateDescription vmstate_pcie_device;
     .offset     = vmstate_offset_value(_state, _field, PCIDevice),   \
 }
 
+void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                              Error **errp);
+void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                 Error **errp);
 #endif /* QEMU_PCIE_H */
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v6 9/9] hw/pci: switch to a generic hotplug handling for PCIDevice
  2014-02-05 15:36 [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
                   ` (7 preceding siblings ...)
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 8/9] pci/pcie: convert PCIE " Igor Mammedov
@ 2014-02-05 15:36 ` Igor Mammedov
  2014-02-10  9:09 ` [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Michael S. Tsirkin
  9 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2014-02-05 15:36 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, mst,
	jan.kiszka, marcel.a, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini, afaerber

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 for compatibility with not yet converted buses.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
 * fix test-qdev-global-props build failure during "make check"
---
 hw/core/qdev.c           |   17 +++++++++++++----
 hw/pci/pci.c             |   29 +----------------------------
 include/hw/pci/pci.h     |   10 ----------
 include/hw/pci/pci_bus.h |    2 --
 tests/Makefile           |    2 +-
 5 files changed, 15 insertions(+), 45 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 5c864db..64b66e0 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -213,7 +213,6 @@ void qdev_unplug(DeviceState *dev, Error **errp)
         error_set(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
         return;
     }
-    assert(dc->unplug != NULL);
 
     if (!dc->hotpluggable) {
         error_set(errp, QERR_DEVICE_NO_HOTPLUG,
@@ -223,9 +222,13 @@ void qdev_unplug(DeviceState *dev, Error **errp)
 
     qdev_hot_removed = true;
 
-    if (dc->unplug(dev) < 0) {
-        error_set(errp, QERR_UNDEFINED_ERROR);
-        return;
+    if (dev->parent_bus && dev->parent_bus->hotplug_handler) {
+        hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, errp);
+    } else {
+        assert(dc->unplug != NULL);
+        if (dc->unplug(dev) < 0) { /* legacy handler */
+            error_set(errp, QERR_UNDEFINED_ERROR);
+        }
     }
 }
 
@@ -720,6 +723,12 @@ static void device_set_realized(Object *obj, bool value, Error **err)
             dc->realize(dev, &local_err);
         }
 
+        if (dev->parent_bus && dev->parent_bus->hotplug_handler &&
+            local_err == NULL) {
+            hotplug_handler_plug(dev->parent_bus->hotplug_handler,
+                                 dev, &local_err);
+        }
+
         if (qdev_get_vmsd(dev) && local_err == NULL) {
             vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev,
                                            dev->instance_id_alias,
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index d69961f..4e0701d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -35,6 +35,7 @@
 #include "hw/pci/msi.h"
 #include "hw/pci/msix.h"
 #include "exec/address-spaces.h"
+#include "hw/hotplug.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI
@@ -346,13 +347,6 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
     bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
 }
 
-void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
-{
-    bus->qbus.allow_hotplug = 1;
-    bus->hotplug = hotplug;
-    bus->hotplug_qdev = qdev;
-}
-
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                          void *irq_opaque,
@@ -1778,29 +1772,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)
 {
@@ -2271,7 +2245,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 c173b6a..693dd6b 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -327,15 +327,6 @@ typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
 typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
 typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
 
-typedef enum {
-    PCI_HOTPLUG_DISABLED,
-    PCI_HOTPLUG_ENABLED,
-    PCI_COLDPLUG_ENABLED,
-} PCIHotplugState;
-
-typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
-                              PCIHotplugState state);
-
 #define TYPE_PCI_BUS "PCI"
 #define PCI_BUS(obj) OBJECT_CHECK(PCIBus, (obj), TYPE_PCI_BUS)
 #define TYPE_PCIE_BUS "PCIE"
@@ -354,7 +345,6 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                   void *irq_opaque, int nirq);
 int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
-void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
 /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
 int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 9df1788..fabaeee 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -16,8 +16,6 @@ struct PCIBus {
     pci_set_irq_fn set_irq;
     pci_map_irq_fn map_irq;
     pci_route_irq_fn route_intx_to_irq;
-    pci_hotplug_fn hotplug;
-    DeviceState *hotplug_qdev;
     void *irq_opaque;
     PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX];
     PCIDevice *parent_dev;
diff --git a/tests/Makefile b/tests/Makefile
index fd36eee..9a7d2f1 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -163,7 +163,7 @@ tests/test-xbzrle$(EXESUF): tests/test-xbzrle.o xbzrle.o page_cache.o libqemuuti
 tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
 tests/test-int128$(EXESUF): tests/test-int128.o
 tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
-	hw/core/qdev.o hw/core/qdev-properties.o \
+	hw/core/qdev.o hw/core/qdev-properties.o hw/core/hotplug.o\
 	hw/core/irq.o \
 	$(qom-core-obj) \
 	$(test-qapi-obj-y) \
-- 
1.7.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v6 2/9] qdev: add to BusState "hotplug-handler" link
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 2/9] qdev: add to BusState "hotplug-handler" link Igor Mammedov
@ 2014-02-05 15:52   ` Andreas Färber
  2014-02-05 16:31     ` Igor Mammedov
  2014-02-05 16:44     ` Paolo Bonzini
  0 siblings, 2 replies; 15+ messages in thread
From: Andreas Färber @ 2014-02-05 15:52 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, mst,
	jan.kiszka, marcel.a, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini

Am 05.02.2014 16:36, schrieb Igor Mammedov:
> It will allow to reuse field with different BUSes,
> reducing code duplication. Field is intended for
> 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 field in qdev hotplug code.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/core/qdev.c         |    4 ++++
>  include/hw/qdev-core.h |   12 ++++++++++++
>  2 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 82a9123..c9f0c33 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;
> @@ -870,6 +871,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);

Will/should the user ever change that property? If not, we could drop
this hunk and change the inline link-setting below to just do it the C
way. Otherwise it should probably be using &error_abort instead of NULL.

Regards,
Andreas

>  }
>  
>  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 2c4f140..41ec533 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,
> @@ -180,14 +181,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;
> @@ -321,4 +326,11 @@ extern int qdev_hotplug;
>  
>  char *qdev_get_dev_path(DeviceState *dev);
>  
> +static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
> +                                            Error **errp)
> +{
> +    object_property_set_link(OBJECT(bus), OBJECT(handler),
> +                             QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
> +    bus->allow_hotplug = 1;
> +}
>  #endif
> 


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v6 2/9] qdev: add to BusState "hotplug-handler" link
  2014-02-05 15:52   ` Andreas Färber
@ 2014-02-05 16:31     ` Igor Mammedov
  2014-02-05 16:44     ` Paolo Bonzini
  1 sibling, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2014-02-05 16:31 UTC (permalink / raw)
  To: Andreas Färber
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, mst,
	jan.kiszka, marcel.a, qemu-devel, armbru, blauwirbel,
	alex.williamson, kraxel, aliguori, pbonzini

On Wed, 05 Feb 2014 16:52:27 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 05.02.2014 16:36, schrieb Igor Mammedov:
> > It will allow to reuse field with different BUSes,
> > reducing code duplication. Field is intended for
> > 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 field in qdev hotplug code.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/core/qdev.c         |    4 ++++
> >  include/hw/qdev-core.h |   12 ++++++++++++
> >  2 files changed, 16 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 82a9123..c9f0c33 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;
> > @@ -870,6 +871,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);
> 
> Will/should the user ever change that property? If not, we could drop
> this hunk and change the inline link-setting below to just do it the C
> way. Otherwise it should probably be using &error_abort instead of NULL.
So far user is expected to set property only one time.
But why deviate from QOM an allow user to poke directly into arbitrary bus
internals even with help of inline helper below? 
Link also will allow to keep pointer safe, i.e. hotplug_handler won't
disappear suddenly.

As for using &error_abort in initfn(), this function might be called during
hotplug and crash running guest.
One way to handle such errors could be passing errp to initfn(),
another is shown is this patch: i.e. ignore error in initfn() as it's done
in other initfn()-s and handle error at set time in qbus_set_hotplug_handler(),
then caller can decide whether do abort or report error up the call stack, i.e.
to libvirt via QMP.

> 
> Regards,
> Andreas
> 
> >  }
> >  
> >  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 2c4f140..41ec533 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,
> > @@ -180,14 +181,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;
> > @@ -321,4 +326,11 @@ extern int qdev_hotplug;
> >  
> >  char *qdev_get_dev_path(DeviceState *dev);
> >  
> > +static inline void qbus_set_hotplug_handler(BusState *bus, DeviceState *handler,
> > +                                            Error **errp)
> > +{
> > +    object_property_set_link(OBJECT(bus), OBJECT(handler),
> > +                             QDEV_HOTPLUG_HANDLER_PROPERTY, errp);
> > +    bus->allow_hotplug = 1;
> > +}
> >  #endif
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


-- 
Regards,
  Igor

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v6 2/9] qdev: add to BusState "hotplug-handler" link
  2014-02-05 15:52   ` Andreas Färber
  2014-02-05 16:31     ` Igor Mammedov
@ 2014-02-05 16:44     ` Paolo Bonzini
  1 sibling, 0 replies; 15+ messages in thread
From: Paolo Bonzini @ 2014-02-05 16:44 UTC (permalink / raw)
  To: Andreas Färber, Igor Mammedov, qemu-devel
  Cc: kwolf, peter.maydell, peter.crosthwaite, aliguori, mst,
	jan.kiszka, marcel.a, armbru, blauwirbel, alex.williamson,
	kraxel, stefanha

Il 05/02/2014 16:52, Andreas Färber ha scritto:
>> > +    object_property_add_link(obj, QDEV_HOTPLUG_HANDLER_PROPERTY,
>> > +                             TYPE_HOTPLUG_HANDLER,
>> > +                             (Object **)&bus->hotplug_handler, NULL);
> Will/should the user ever change that property? If not, we could drop
> this hunk and change the inline link-setting below to just do it the C
> way. Otherwise it should probably be using &error_abort instead of NULL.

No, they shouldn't.  But OTOH it's probably useful in general to _read_ 
the property, and links handle reference counting nicely too.  It's 
similar to the parent_bus link.

I think we should add something like getter/setter for links like we 
have for object_property_add_str, because in most of the current cases 
the link should be read-only.

It should not hold this series, though.

Paolo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
  2014-02-05 15:36 [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
                   ` (8 preceding siblings ...)
  2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 9/9] hw/pci: switch to a generic hotplug handling for PCIDevice Igor Mammedov
@ 2014-02-10  9:09 ` Michael S. Tsirkin
  2014-02-10  9:53   ` Igor Mammedov
  9 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2014-02-10  9:09 UTC (permalink / raw)
  To: Igor Mammedov
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, marcel.a,
	jan.kiszka, qemu-devel, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini, afaerber

On Wed, Feb 05, 2014 at 04:36:43PM +0100, Igor Mammedov wrote:
> changes since v5:
>  - fixed typos Eric pointed to
>  - fix conflict of [5/9] with new patch "acpi-build: append description for non-hotplug"
>  - rebased on top of todays PCI tree
> 
> changes since v4:
>  - rebased on top of PCI tree
>  - added wrapper to set hotplug-device property
> 
> changes since v3:
>  - fixup/add comments as reqused by  Peter Crosthwaite
>  - use error_abort to reduce error handling verbosity
>  - fix tests/test-qdev-global-props build failure on make check
>  - rebase on top of current master:133fe7743 (with interface fixes)
> 
> changes since v2:
> * s/hotplugable/hotpluggable/
> * move hotplug check to an earlier patch:
>   "qdev: add "hotpluggable" property to Device"

This was on top my
    acpi-build: append description for non-hotplug
which I'm not merging yet, so I had to rebase.

Applied and pushed, please verify and confirm.

> --
> 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_v6
> 
> tested only ACPI and PCIE hotplug, since SHPC is not functional, tested
> it under debugger that hotplhug event reaches handler and corresponding interrupt is emited.
> 
> 
> 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
> 
>  hw/acpi/pcihp.c                |   46 ++++++++++----
>  hw/acpi/piix4.c                |  128 +++++++++++++++++++++-------------------
>  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           |    4 +-
>  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                  |   65 ++++++++++++--------
>  hw/pci/pcie_port.c             |    8 +++
>  hw/pci/shpc.c                  |  124 ++++++++++++++++++++++++---------------
>  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/acpi/acpi.h         |    1 +
>  include/hw/acpi/pcihp.h        |   10 ++-
>  include/hw/hotplug.h           |   78 ++++++++++++++++++++++++
>  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         |   15 +++++
>  tests/Makefile                 |    2 +-
>  31 files changed, 463 insertions(+), 222 deletions(-)
>  create mode 100644 hw/core/hotplug.c
>  create mode 100644 include/hw/hotplug.h

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
  2014-02-10  9:09 ` [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Michael S. Tsirkin
@ 2014-02-10  9:53   ` Igor Mammedov
  0 siblings, 0 replies; 15+ messages in thread
From: Igor Mammedov @ 2014-02-10  9:53 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: kwolf, peter.maydell, peter.crosthwaite, stefanha, marcel.a,
	jan.kiszka, qemu-devel, armbru, blauwirbel, alex.williamson,
	kraxel, aliguori, pbonzini, afaerber

On Mon, 10 Feb 2014 11:09:16 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Feb 05, 2014 at 04:36:43PM +0100, Igor Mammedov wrote:
> > changes since v5:
> >  - fixed typos Eric pointed to
> >  - fix conflict of [5/9] with new patch "acpi-build: append description for non-hotplug"
> >  - rebased on top of todays PCI tree
> > 
> > changes since v4:
> >  - rebased on top of PCI tree
> >  - added wrapper to set hotplug-device property
> > 
> > changes since v3:
> >  - fixup/add comments as reqused by  Peter Crosthwaite
> >  - use error_abort to reduce error handling verbosity
> >  - fix tests/test-qdev-global-props build failure on make check
> >  - rebase on top of current master:133fe7743 (with interface fixes)
> > 
> > changes since v2:
> > * s/hotplugable/hotpluggable/
> > * move hotplug check to an earlier patch:
> >   "qdev: add "hotpluggable" property to Device"
> 
> This was on top my
>     acpi-build: append description for non-hotplug
> which I'm not merging yet, so I had to rebase.
> 
> Applied and pushed, please verify and confirm.
Rebased patch looks good.

Thanks!

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2014-02-10  9:54 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-05 15:36 [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 1/9] define hotplug interface Igor Mammedov
2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 2/9] qdev: add to BusState "hotplug-handler" link Igor Mammedov
2014-02-05 15:52   ` Andreas Färber
2014-02-05 16:31     ` Igor Mammedov
2014-02-05 16:44     ` Paolo Bonzini
2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 3/9] qdev: add "hotpluggable" property to Device Igor Mammedov
2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 4/9] hw/acpi: move typeinfo to the file end Igor Mammedov
2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 5/9] qdev:pci: refactor PCIDevice to use generic "hotpluggable" property Igor Mammedov
2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 6/9] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-handler API Igor Mammedov
2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 7/9] pci/shpc: convert SHPC " Igor Mammedov
2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 8/9] pci/pcie: convert PCIE " Igor Mammedov
2014-02-05 15:36 ` [Qemu-devel] [PATCH v6 9/9] hw/pci: switch to a generic hotplug handling for PCIDevice Igor Mammedov
2014-02-10  9:09 ` [Qemu-devel] [PATCH v6 0/9] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Michael S. Tsirkin
2014-02-10  9:53   ` Igor Mammedov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.