* [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
@ 2013-12-06 17:03 Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 1/7] define hotplug interface Igor Mammedov
` (7 more replies)
0 siblings, 8 replies; 34+ messages in thread
From: Igor Mammedov @ 2013-12-06 17:03 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, ehabkost, mst, marcel.a, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
Series is replacement for a more simple attempt to generalize hotplug API
http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02529.html
promted by Paolo in this patch review.
--
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 4-7 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 applyed).
git tree for testing:
https://github.com/imammedo/qemu/commits/hotplug_dev_inf_v1
tested only ACPI hotplug with following HMP commands:
==
netdev_add user,id=n1
device_add e1000,id=e1,netdev=n1
device_del e1
==
Igor Mammedov (7):
define hotplug interface
qdev: add to BusState "hotplug-device" link
hw/acpi: move typeinfo to the file end
acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-device
interface
pci/shpc: convert SHPC hotplug to use hotplug-device interface
pci/pcie: convert PCIE hotplug to use hotplug-device interface
hw/pci: convert PCI bus to use "hotplug-device" interface.
hw/acpi/piix4.c | 143 +++++++++++++++++++++-------------------
hw/core/Makefile.objs | 1 +
hw/core/hotplug.c | 25 +++++++
hw/core/qdev.c | 4 +
hw/pci-bridge/pci_bridge_dev.c | 9 +++
hw/pci/pci.c | 70 ++++++++++++++-----
hw/pci/pcie.c | 73 +++++++++++++-------
hw/pci/pcie_port.c | 8 ++
hw/pci/shpc.c | 132 ++++++++++++++++++++++++-------------
include/hw/hotplug.h | 50 ++++++++++++++
include/hw/pci/pci.h | 10 ---
include/hw/pci/pci_bus.h | 2 -
include/hw/pci/pcie.h | 4 +
include/hw/pci/shpc.h | 7 ++
include/hw/qdev-core.h | 4 +
15 files changed, 372 insertions(+), 170 deletions(-)
create mode 100644 hw/core/hotplug.c
create mode 100644 include/hw/hotplug.h
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 1/7] define hotplug interface
2013-12-06 17:03 [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
@ 2013-12-06 17:03 ` Igor Mammedov
2013-12-06 17:26 ` Paolo Bonzini
` (2 more replies)
2013-12-06 17:03 ` [Qemu-devel] [PATCH 2/7] qdev: add to BusState "hotplug-device" link Igor Mammedov
` (6 subsequent siblings)
7 siblings, 3 replies; 34+ messages in thread
From: Igor Mammedov @ 2013-12-06 17:03 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, ehabkost, mst, marcel.a, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
Provide generic hotplug interface for devices.
Intended for replacing hotplug mechanism used by
PCI/PCIE/SHPC code.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
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 | 25 ++++++++++++++++++++++++
include/hw/hotplug.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 76 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..47f6555 100644
--- a/hw/core/Makefile.objs
+++ b/hw/core/Makefile.objs
@@ -10,4 +10,5 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
common-obj-$(CONFIG_SOFTMMU) += null-machine.o
common-obj-$(CONFIG_SOFTMMU) += loader.o
common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
+common-obj-$(CONFIG_SOFTMMU) += hotplug.o
diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
new file mode 100644
index 0000000..3e84d9c
--- /dev/null
+++ b/hw/core/hotplug.c
@@ -0,0 +1,25 @@
+/*
+ * Hotplug device 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"
+
+static const TypeInfo hotplug_device_info = {
+ .name = TYPE_HOTPLUG_DEVICE,
+ .parent = TYPE_INTERFACE,
+ .class_size = sizeof(HotplugDeviceClass),
+};
+
+static void hotplug_device_register_types(void)
+{
+ type_register_static(&hotplug_device_info);
+}
+
+type_init(hotplug_device_register_types)
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
new file mode 100644
index 0000000..cfa79bb
--- /dev/null
+++ b/include/hw/hotplug.h
@@ -0,0 +1,50 @@
+/*
+ * Hotplug device 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 "hw/qdev-core.h"
+
+#define TYPE_HOTPLUG_DEVICE "hotplug-device"
+
+#define HOTPLUG_DEVICE_CLASS(klass) \
+ OBJECT_CLASS_CHECK(HotplugDeviceClass, (klass), TYPE_HOTPLUG_DEVICE)
+#define HOTPLUG_DEVICE_GET_CLASS(obj) \
+ OBJECT_GET_CLASS(HotplugDeviceClass, (obj), TYPE_HOTPLUG_DEVICE)
+
+/**
+ * hotplug_fn:
+ * @hotplug_dev: a device performing hotplug/uplug action
+ * @hotplugged_dev: a device that has been hotplugged
+ * @errp: returns an error if this function fails
+ */
+typedef void (*hotplug_fn)(DeviceState *hotplug_dev,
+ DeviceState *hotplugged_dev, Error **errp);
+
+/**
+ * HotplugDeviceClass:
+ *
+ * Interface to be implemented by a device performing
+ * hardware hotplug/unplug functions.
+ *
+ * @parent: Opaque parent interface.
+ * @hotplug: hotplug callback.
+ * @hot_unplug: hot unplug callback.
+ */
+typedef struct HotplugDeviceClass {
+ InterfaceClass parent;
+
+ hotplug_fn hotplug;
+ hotplug_fn hot_unplug;
+} HotplugDeviceClass;
+
+#endif
--
1.7.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 2/7] qdev: add to BusState "hotplug-device" link
2013-12-06 17:03 [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 1/7] define hotplug interface Igor Mammedov
@ 2013-12-06 17:03 ` Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 3/7] hw/acpi: move typeinfo to the file end Igor Mammedov
` (5 subsequent siblings)
7 siblings, 0 replies; 34+ messages in thread
From: Igor Mammedov @ 2013-12-06 17:03 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, ehabkost, mst, marcel.a, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
It will allow to reuse field in different BUSes, reducing code duplication.
Field is intended fot replacing 'hotplug_qdev' field in PCIBus and also
will allow not to add equivalent field to DimmBus with possiblitity
to refactor other BUSes to use it instead of custom field.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/core/qdev.c | 4 ++++
include/hw/qdev-core.h | 4 ++++
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index e374a93..43cf160 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_DEVICE_PROPERTY,
+ TYPE_HOTPLUG_DEVICE,
+ (Object **)&bus->hotplug_device, 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..9dc0fd1 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -169,14 +169,18 @@ typedef struct BusChild {
QTAILQ_ENTRY(BusChild) sibling;
} BusChild;
+#define QDEV_HOTPLUG_DEVICE_PROPERTY "hotplug-device"
+
/**
* BusState:
+ * @hotplug_device: link to a hotplug device associated with bus.
*/
struct BusState {
Object obj;
DeviceState *parent;
const char *name;
int allow_hotplug;
+ DeviceState *hotplug_device;
int max_index;
QTAILQ_HEAD(ChildrenHead, BusChild) children;
QLIST_ENTRY(BusState) sibling;
--
1.7.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 3/7] hw/acpi: move typeinfo to the file end
2013-12-06 17:03 [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 1/7] define hotplug interface Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 2/7] qdev: add to BusState "hotplug-device" link Igor Mammedov
@ 2013-12-06 17:03 ` Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 4/7] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-device interface Igor Mammedov
` (4 subsequent siblings)
7 siblings, 0 replies; 34+ messages in thread
From: Igor Mammedov @ 2013-12-06 17:03 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, ehabkost, mst, marcel.a, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
do so to avoid not necessary forward declarations and
place typeinfo registration at the file end where it's
usualy expected.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/piix4.c | 80 +++++++++++++++++++++++++++---------------------------
1 files changed, 40 insertions(+), 40 deletions(-)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 93849c8..8084a60 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -523,46 +523,6 @@ i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
return s->smb.smbus;
}
-static Property piix4_pm_properties[] = {
- DEFINE_PROP_UINT32("smb_io_base", PIIX4PMState, smb_io_base, 0),
- DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0),
- DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 0),
- DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
- DEFINE_PROP_END_OF_LIST(),
-};
-
-static void piix4_pm_class_init(ObjectClass *klass, void *data)
-{
- DeviceClass *dc = DEVICE_CLASS(klass);
- PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
- k->no_hotplug = 1;
- k->init = piix4_pm_initfn;
- k->config_write = pm_write_config;
- k->vendor_id = PCI_VENDOR_ID_INTEL;
- k->device_id = PCI_DEVICE_ID_INTEL_82371AB_3;
- k->revision = 0x03;
- k->class_id = PCI_CLASS_BRIDGE_OTHER;
- dc->desc = "PM";
- dc->no_user = 1;
- dc->vmsd = &vmstate_acpi;
- dc->props = piix4_pm_properties;
-}
-
-static const TypeInfo piix4_pm_info = {
- .name = TYPE_PIIX4_PM,
- .parent = TYPE_PCI_DEVICE,
- .instance_size = sizeof(PIIX4PMState),
- .class_init = piix4_pm_class_init,
-};
-
-static void piix4_pm_register_types(void)
-{
- type_register_static(&piix4_pm_info);
-}
-
-type_init(piix4_pm_register_types)
-
static uint64_t gpe_readb(void *opaque, hwaddr addr, unsigned width)
{
PIIX4PMState *s = opaque;
@@ -772,3 +732,43 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
return 0;
}
+
+static Property piix4_pm_properties[] = {
+ DEFINE_PROP_UINT32("smb_io_base", PIIX4PMState, smb_io_base, 0),
+ DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0),
+ DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_DISABLED, PIIX4PMState, disable_s4, 0),
+ DEFINE_PROP_UINT8(ACPI_PM_PROP_S4_VAL, PIIX4PMState, s4_val, 2),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void piix4_pm_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+ k->no_hotplug = 1;
+ k->init = piix4_pm_initfn;
+ k->config_write = pm_write_config;
+ k->vendor_id = PCI_VENDOR_ID_INTEL;
+ k->device_id = PCI_DEVICE_ID_INTEL_82371AB_3;
+ k->revision = 0x03;
+ k->class_id = PCI_CLASS_BRIDGE_OTHER;
+ dc->desc = "PM";
+ dc->no_user = 1;
+ dc->vmsd = &vmstate_acpi;
+ dc->props = piix4_pm_properties;
+}
+
+static const TypeInfo piix4_pm_info = {
+ .name = TYPE_PIIX4_PM,
+ .parent = TYPE_PCI_DEVICE,
+ .instance_size = sizeof(PIIX4PMState),
+ .class_init = piix4_pm_class_init,
+};
+
+static void piix4_pm_register_types(void)
+{
+ type_register_static(&piix4_pm_info);
+}
+
+type_init(piix4_pm_register_types)
--
1.7.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 4/7] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-device interface
2013-12-06 17:03 [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
` (2 preceding siblings ...)
2013-12-06 17:03 ` [Qemu-devel] [PATCH 3/7] hw/acpi: move typeinfo to the file end Igor Mammedov
@ 2013-12-06 17:03 ` Igor Mammedov
2013-12-09 9:02 ` Marcel Apfelbaum
2013-12-06 17:03 ` [Qemu-devel] [PATCH 5/7] pci/shpc: convert SHPC " Igor Mammedov
` (3 subsequent siblings)
7 siblings, 1 reply; 34+ messages in thread
From: Igor Mammedov @ 2013-12-06 17:03 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, ehabkost, mst, marcel.a, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
Split piix4_device_hotplug() into hotplug/unplug callbacks
and register them as "hotplug-device" interface implementation of
PIIX4_PM device.
Replace pci_bus_hotplug() wiring with setting link on
PCI BUS "hotplug-device" property to PIIX4_PM device.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/piix4.c | 73 +++++++++++++++++++++++++++++++------------------------
1 files changed, 41 insertions(+), 32 deletions(-)
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 8084a60..8327d80 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_DEVICE_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(DeviceState *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(DeviceState *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);
+ HotplugDeviceClass *hc = HOTPLUG_DEVICE_CLASS(klass);
k->no_hotplug = 1;
k->init = piix4_pm_initfn;
@@ -757,6 +760,8 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
dc->no_user = 1;
dc->vmsd = &vmstate_acpi;
dc->props = piix4_pm_properties;
+ hc->hotplug = piix4_pci_device_hotplug_cb;
+ hc->hot_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_DEVICE },
+ { }
+ }
};
static void piix4_pm_register_types(void)
--
1.7.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 5/7] pci/shpc: convert SHPC hotplug to use hotplug-device interface
2013-12-06 17:03 [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
` (3 preceding siblings ...)
2013-12-06 17:03 ` [Qemu-devel] [PATCH 4/7] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-device interface Igor Mammedov
@ 2013-12-06 17:03 ` Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 6/7] pci/pcie: convert PCIE " Igor Mammedov
` (2 subsequent siblings)
7 siblings, 0 replies; 34+ messages in thread
From: Igor Mammedov @ 2013-12-06 17:03 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, ehabkost, mst, marcel.a, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
Split shpc_device_hotplug() into hotplug/unplug callbacks
and register them as "hotplug-device" interface implementation of
PCI_BRIDGE_DEV device.
Replace pci_bus_hotplug() wiring with setting link on PCI BUS
"hotplug-device" 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 | 132 ++++++++++++++++++++++++++--------------
include/hw/pci/shpc.h | 7 ++
3 files changed, 101 insertions(+), 47 deletions(-)
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 440e187..8a1813e 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);
+ HotplugDeviceClass *hc = HOTPLUG_DEVICE_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->hotplug = shpc_device_hotplug_cb;
+ hc->hot_unplug = shpc_device_hotplug_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_DEVICE },
+ { }
+ }
};
static void pci_bridge_dev_register(void)
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 576244b..e1ef84d 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,102 @@ 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(DeviceState *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, errp);
+ if (error_is_set(&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(DeviceState *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 +646,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_DEVICE_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..ea1efde 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -4,6 +4,7 @@
#include "qemu-common.h"
#include "exec/memory.h"
#include "migration/vmstate.h"
+#include "qapi/error.h"
struct SHPCDevice {
/* Capability offset in device's config space */
@@ -41,6 +42,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(DeviceState *hotplug_dev, DeviceState *dev,
+ Error **errp);
+void shpc_device_hot_unplug_cb(DeviceState *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] 34+ messages in thread
* [Qemu-devel] [PATCH 6/7] pci/pcie: convert PCIE hotplug to use hotplug-device interface
2013-12-06 17:03 [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
` (4 preceding siblings ...)
2013-12-06 17:03 ` [Qemu-devel] [PATCH 5/7] pci/shpc: convert SHPC " Igor Mammedov
@ 2013-12-06 17:03 ` Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface Igor Mammedov
2013-12-06 17:31 ` [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Paolo Bonzini
7 siblings, 0 replies; 34+ messages in thread
From: Igor Mammedov @ 2013-12-06 17:03 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, ehabkost, mst, marcel.a, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
Split pcie_cap_slot_hotplug() into hotplug/unplug callbacks
and register them as "hotplug-device" interface implementation of
PCIE_SLOT device.
Replace pci_bus_hotplug() wiring with setting link on PCI BUS
"hotplug-device" 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 | 4 +++
3 files changed, 60 insertions(+), 25 deletions(-)
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index ca60cf2..0288717 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(DeviceState *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(DeviceState *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_DEVICE_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..43f28c5 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);
+ HotplugDeviceClass *hc = HOTPLUG_DEVICE_CLASS(oc);
dc->props = pcie_slot_props;
+ hc->hotplug = pcie_cap_slot_hotplug_cb;
+ hc->hot_unplug = pcie_cap_slot_hotplug_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_DEVICE },
+ { }
+ }
};
static void pcie_port_register_types(void)
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 1966169..889c82a 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -122,4 +122,8 @@ extern const VMStateDescription vmstate_pcie_device;
.offset = vmstate_offset_value(_state, _field, PCIDevice), \
}
+void pcie_cap_slot_hotplug_cb(DeviceState *hotplug_dev, DeviceState *dev,
+ Error **errp);
+void pcie_cap_slot_hot_unplug_cb(DeviceState *hotplug_dev, DeviceState *dev,
+ Error **errp);
#endif /* QEMU_PCIE_H */
--
1.7.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-06 17:03 [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
` (5 preceding siblings ...)
2013-12-06 17:03 ` [Qemu-devel] [PATCH 6/7] pci/pcie: convert PCIE " Igor Mammedov
@ 2013-12-06 17:03 ` Igor Mammedov
2013-12-06 17:29 ` Paolo Bonzini
2013-12-09 9:09 ` Marcel Apfelbaum
2013-12-06 17:31 ` [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Paolo Bonzini
7 siblings, 2 replies; 34+ messages in thread
From: Igor Mammedov @ 2013-12-06 17:03 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, ehabkost, mst, marcel.a, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/pci/pci.c | 70 +++++++++++++++++++++++++++++++++------------
include/hw/pci/pci.h | 10 ------
include/hw/pci/pci_bus.h | 2 -
3 files changed, 51 insertions(+), 31 deletions(-)
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 49eca95..26229de 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -35,6 +35,7 @@
#include "hw/pci/msi.h"
#include "hw/pci/msix.h"
#include "exec/address-spaces.h"
+#include "hw/hotplug.h"
//#define DEBUG_PCI
#ifdef DEBUG_PCI
@@ -348,13 +349,6 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
}
-void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
-{
- bus->qbus.allow_hotplug = 1;
- bus->hotplug = hotplug;
- bus->hotplug_qdev = qdev;
-}
-
PCIBus *pci_register_bus(DeviceState *parent, const char *name,
pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
void *irq_opaque,
@@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev)
{
PCIDevice *pci_dev = (PCIDevice *)qdev;
PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
+ DeviceState *hotplug_dev;
+ Error *local_err = NULL;
PCIBus *bus;
int rc;
bool is_default_rom;
@@ -1756,32 +1752,68 @@ static int pci_qdev_init(DeviceState *qdev)
}
pci_add_option_rom(pci_dev, is_default_rom);
- if (bus->hotplug) {
- /* Let buses differentiate between hotplug and when device is
- * enabled during qemu machine creation. */
- rc = bus->hotplug(bus->hotplug_qdev, pci_dev,
- qdev->hotplugged ? PCI_HOTPLUG_ENABLED:
- PCI_COLDPLUG_ENABLED);
- if (rc != 0) {
- int r = pci_unregister_device(&pci_dev->qdev);
- assert(!r);
- return rc;
+ hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
+ &local_err));
+ if (error_is_set(&local_err)) {
+ goto error_exit;
+ }
+ if (hotplug_dev) {
+ HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
+
+ /* handler can differentiate between hotplug and when device is
+ * enabled during qemu machine creation by inspecting
+ * dev->hotplugged field. */
+ if (hdc->hotplug) {
+ hdc->hotplug(hotplug_dev, qdev, &local_err);
+ if (error_is_set(&local_err)) {
+ int r = pci_unregister_device(&pci_dev->qdev);
+ assert(!r);
+ goto error_exit;
+ }
}
}
return 0;
+
+error_exit:
+ qerror_report_err(local_err);
+ error_free(local_err);
+ return -1;
}
static int pci_unplug_device(DeviceState *qdev)
{
PCIDevice *dev = PCI_DEVICE(qdev);
PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
+ BusState *bus = qdev_get_parent_bus(qdev);
+ DeviceState *hotplug_dev;
+ Error *local_err = NULL;
if (pc->no_hotplug) {
qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev)));
return -1;
}
- return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
- PCI_HOTPLUG_DISABLED);
+
+ hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
+ &local_err));
+ if (error_is_set(&local_err)) {
+ goto error_exit;
+ }
+ if (hotplug_dev) {
+ HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
+
+ if (hdc->hot_unplug) {
+ hdc->hot_unplug(hotplug_dev, qdev, &local_err);
+ if (error_is_set(&local_err)) {
+ goto error_exit;
+ }
+ }
+ }
+ return 0;
+
+error_exit:
+ qerror_report_err(local_err);
+ error_free(local_err);
+ return -1;
}
PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b783e68..33dec40 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -330,15 +330,6 @@ typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
-typedef enum {
- PCI_HOTPLUG_DISABLED,
- PCI_HOTPLUG_ENABLED,
- PCI_COLDPLUG_ENABLED,
-} PCIHotplugState;
-
-typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
- PCIHotplugState state);
-
#define TYPE_PCI_BUS "PCI"
#define PCI_BUS(obj) OBJECT_CHECK(PCIBus, (obj), TYPE_PCI_BUS)
#define TYPE_PCIE_BUS "PCIE"
@@ -357,7 +348,6 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
void *irq_opaque, int nirq);
int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
-void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
/* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
PCIBus *pci_register_bus(DeviceState *parent, const char *name,
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 9df1788..fabaeee 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -16,8 +16,6 @@ struct PCIBus {
pci_set_irq_fn set_irq;
pci_map_irq_fn map_irq;
pci_route_irq_fn route_intx_to_irq;
- pci_hotplug_fn hotplug;
- DeviceState *hotplug_qdev;
void *irq_opaque;
PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX];
PCIDevice *parent_dev;
--
1.7.1
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] define hotplug interface
2013-12-06 17:03 ` [Qemu-devel] [PATCH 1/7] define hotplug interface Igor Mammedov
@ 2013-12-06 17:26 ` Paolo Bonzini
2013-12-09 12:42 ` Igor Mammedov
2013-12-09 5:40 ` Li Guang
2013-12-09 8:58 ` Marcel Apfelbaum
2 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2013-12-06 17:26 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, mst, marcel.a, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
Il 06/12/2013 18:03, Igor Mammedov ha scritto:
> Provide generic hotplug interface for devices.
> Intended for replacing hotplug mechanism used by
> PCI/PCIE/SHPC code.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> 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 | 25 ++++++++++++++++++++++++
> include/hw/hotplug.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 76 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..47f6555 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -10,4 +10,5 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
> common-obj-$(CONFIG_SOFTMMU) += null-machine.o
> common-obj-$(CONFIG_SOFTMMU) += loader.o
> common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> +common-obj-$(CONFIG_SOFTMMU) += hotplug.o
>
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> new file mode 100644
> index 0000000..3e84d9c
> --- /dev/null
> +++ b/hw/core/hotplug.c
> @@ -0,0 +1,25 @@
> +/*
> + * Hotplug device 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"
> +
> +static const TypeInfo hotplug_device_info = {
> + .name = TYPE_HOTPLUG_DEVICE,
Perhaps replace "device" with "handler" throughout?
> + .parent = TYPE_INTERFACE,
> + .class_size = sizeof(HotplugDeviceClass),
> +};
> +
> +static void hotplug_device_register_types(void)
> +{
> + type_register_static(&hotplug_device_info);
> +}
> +
> +type_init(hotplug_device_register_types)
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> new file mode 100644
> index 0000000..cfa79bb
> --- /dev/null
> +++ b/include/hw/hotplug.h
> @@ -0,0 +1,50 @@
> +/*
> + * Hotplug device 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 "hw/qdev-core.h"
> +
> +#define TYPE_HOTPLUG_DEVICE "hotplug-device"
> +
> +#define HOTPLUG_DEVICE_CLASS(klass) \
> + OBJECT_CLASS_CHECK(HotplugDeviceClass, (klass), TYPE_HOTPLUG_DEVICE)
> +#define HOTPLUG_DEVICE_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(HotplugDeviceClass, (obj), TYPE_HOTPLUG_DEVICE)
Missing HOTPLUG_DEVICE macro, see include/hw/stream.h.
> +/**
> + * hotplug_fn:
> + * @hotplug_dev: a device performing hotplug/uplug action
> + * @hotplugged_dev: a device that has been hotplugged
> + * @errp: returns an error if this function fails
> + */
> +typedef void (*hotplug_fn)(DeviceState *hotplug_dev,
First argument should be HotplugDevice (which is a struct containing a
single Object field; again see include/hw/stream.h).
> + DeviceState *hotplugged_dev, Error **errp);
> +
> +/**
> + * HotplugDeviceClass:
> + *
> + * Interface to be implemented by a device performing
> + * hardware hotplug/unplug functions.
> + *
> + * @parent: Opaque parent interface.
> + * @hotplug: hotplug callback.
> + * @hot_unplug: hot unplug callback.
Just plug/unplug. IIRC it is used for cold-plug too.
Otherwise looks great.
Paolo
> + */
> +typedef struct HotplugDeviceClass {
> + InterfaceClass parent;
> +
> + hotplug_fn hotplug;
> + hotplug_fn hot_unplug;
> +} HotplugDeviceClass;
> +
> +#endif
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-06 17:03 ` [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface Igor Mammedov
@ 2013-12-06 17:29 ` Paolo Bonzini
2013-12-09 13:41 ` Igor Mammedov
2013-12-09 9:09 ` Marcel Apfelbaum
1 sibling, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2013-12-06 17:29 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, mst, marcel.a, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
Il 06/12/2013 18:03, Igor Mammedov ha scritto:
> + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
> + &local_err));
> + if (error_is_set(&local_err)) {
> + goto error_exit;
> + }
> + if (hotplug_dev) {
> + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> +
> + /* handler can differentiate between hotplug and when device is
> + * enabled during qemu machine creation by inspecting
> + * dev->hotplugged field. */
> + if (hdc->hotplug) {
> + hdc->hotplug(hotplug_dev, qdev, &local_err);
> + if (error_is_set(&local_err)) {
> + int r = pci_unregister_device(&pci_dev->qdev);
> + assert(!r);
> + goto error_exit;
> + }
> }
> }
> +
> + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
> + &local_err));
> + if (error_is_set(&local_err)) {
> + goto error_exit;
> + }
> + if (hotplug_dev) {
> + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> +
> + if (hdc->hot_unplug) {
> + hdc->hot_unplug(hotplug_dev, qdev, &local_err);
> + if (error_is_set(&local_err)) {
> + goto error_exit;
> + }
> + }
> + }
Please move the parts under the "if" to hotplug.c (something like
hotplug_handler_plug and hotplug_handler_unplug). Also, should this be
moved up to generic code (e.g. bus_add_child/bus_remove_child)?
A NULL link can be a no-op for coldplugged devices, an error for
hotplugged devices, and an assertion failure for hot-unplug.
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
2013-12-06 17:03 [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
` (6 preceding siblings ...)
2013-12-06 17:03 ` [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface Igor Mammedov
@ 2013-12-06 17:31 ` Paolo Bonzini
2013-12-09 14:15 ` Igor Mammedov
7 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2013-12-06 17:31 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, mst, marcel.a, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
Il 06/12/2013 18:03, Igor Mammedov ha scritto:
> Series is replacement for a more simple attempt to generalize hotplug API
> http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02529.html
> promted by Paolo in this patch review.
> --
>
> 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 4-7 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 applyed).
>
> git tree for testing:
> https://github.com/imammedo/qemu/commits/hotplug_dev_inf_v1
>
> tested only ACPI hotplug with following HMP commands:
> ==
> netdev_add user,id=n1
> device_add e1000,id=e1,netdev=n1
> device_del e1
> ==
>
> Igor Mammedov (7):
> define hotplug interface
> qdev: add to BusState "hotplug-device" link
> hw/acpi: move typeinfo to the file end
> acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-device
> interface
> pci/shpc: convert SHPC hotplug to use hotplug-device interface
> pci/pcie: convert PCIE hotplug to use hotplug-device interface
> hw/pci: convert PCI bus to use "hotplug-device" interface.
>
> hw/acpi/piix4.c | 143 +++++++++++++++++++++-------------------
> hw/core/Makefile.objs | 1 +
> hw/core/hotplug.c | 25 +++++++
> hw/core/qdev.c | 4 +
> hw/pci-bridge/pci_bridge_dev.c | 9 +++
> hw/pci/pci.c | 70 ++++++++++++++-----
> hw/pci/pcie.c | 73 +++++++++++++-------
> hw/pci/pcie_port.c | 8 ++
> hw/pci/shpc.c | 132 ++++++++++++++++++++++++-------------
> include/hw/hotplug.h | 50 ++++++++++++++
> include/hw/pci/pci.h | 10 ---
> include/hw/pci/pci_bus.h | 2 -
> include/hw/pci/pcie.h | 4 +
> include/hw/pci/shpc.h | 7 ++
> include/hw/qdev-core.h | 4 +
> 15 files changed, 372 insertions(+), 170 deletions(-)
> create mode 100644 hw/core/hotplug.c
> create mode 100644 include/hw/hotplug.h
>
I didn't look closely at patches 4-6, but it looks very nice and
extensible. I will convert SCSI to use this too when it goes in.
Can you pick up my qom patches for interfaces too ("qom: fix
registration of QOM interfaces")?
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] define hotplug interface
2013-12-06 17:03 ` [Qemu-devel] [PATCH 1/7] define hotplug interface Igor Mammedov
2013-12-06 17:26 ` Paolo Bonzini
@ 2013-12-09 5:40 ` Li Guang
2013-12-09 9:11 ` Marcel Apfelbaum
2013-12-09 12:44 ` Igor Mammedov
2013-12-09 8:58 ` Marcel Apfelbaum
2 siblings, 2 replies; 34+ messages in thread
From: Li Guang @ 2013-12-09 5:40 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, marcel.a, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
Hi, Igor
Igor Mammedov wrote:
> Provide generic hotplug interface for devices.
> Intended for replacing hotplug mechanism used by
> PCI/PCIE/SHPC code.
>
> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> ---
> 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 | 25 ++++++++++++++++++++++++
> include/hw/hotplug.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 76 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..47f6555 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -10,4 +10,5 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
> common-obj-$(CONFIG_SOFTMMU) += null-machine.o
> common-obj-$(CONFIG_SOFTMMU) += loader.o
> common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> +common-obj-$(CONFIG_SOFTMMU) += hotplug.o
>
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> new file mode 100644
> index 0000000..3e84d9c
> --- /dev/null
> +++ b/hw/core/hotplug.c
> @@ -0,0 +1,25 @@
> +/*
> + * Hotplug device 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"
> +
> +static const TypeInfo hotplug_device_info = {
> + .name = TYPE_HOTPLUG_DEVICE,
> + .parent = TYPE_INTERFACE,
> + .class_size = sizeof(HotplugDeviceClass),
> +};
> +
> +static void hotplug_device_register_types(void)
> +{
> + type_register_static(&hotplug_device_info);
> +}
> +
> +type_init(hotplug_device_register_types)
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> new file mode 100644
> index 0000000..cfa79bb
> --- /dev/null
> +++ b/include/hw/hotplug.h
> @@ -0,0 +1,50 @@
> +/*
> + * Hotplug device 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 "hw/qdev-core.h"
> +
> +#define TYPE_HOTPLUG_DEVICE "hotplug-device"
> +
> +#define HOTPLUG_DEVICE_CLASS(klass) \
> + OBJECT_CLASS_CHECK(HotplugDeviceClass, (klass), TYPE_HOTPLUG_DEVICE)
> +#define HOTPLUG_DEVICE_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(HotplugDeviceClass, (obj), TYPE_HOTPLUG_DEVICE)
> +
>
Hmm..., this is interface, but device, a bold opinion,
can have something like TYPE_HOTPLUG_INTERFACE ... ?
> +/**
> + * hotplug_fn:
> + * @hotplug_dev: a device performing hotplug/uplug action
>
s/uplug/unplug
Thanks!
Li Guang
> + * @hotplugged_dev: a device that has been hotplugged
> + * @errp: returns an error if this function fails
> + */
> +typedef void (*hotplug_fn)(DeviceState *hotplug_dev,
> + DeviceState *hotplugged_dev, Error **errp);
> +
> +/**
> + * HotplugDeviceClass:
> + *
> + * Interface to be implemented by a device performing
> + * hardware hotplug/unplug functions.
> + *
> + * @parent: Opaque parent interface.
> + * @hotplug: hotplug callback.
> + * @hot_unplug: hot unplug callback.
> + */
> +typedef struct HotplugDeviceClass {
> + InterfaceClass parent;
> +
> + hotplug_fn hotplug;
> + hotplug_fn hot_unplug;
> +} HotplugDeviceClass;
> +
> +#endif
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] define hotplug interface
2013-12-06 17:03 ` [Qemu-devel] [PATCH 1/7] define hotplug interface Igor Mammedov
2013-12-06 17:26 ` Paolo Bonzini
2013-12-09 5:40 ` Li Guang
@ 2013-12-09 8:58 ` Marcel Apfelbaum
2013-12-09 12:52 ` Igor Mammedov
2 siblings, 1 reply; 34+ messages in thread
From: Marcel Apfelbaum @ 2013-12-09 8:58 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote:
> Provide generic hotplug interface for devices.
> Intended for replacing hotplug mechanism used by
> PCI/PCIE/SHPC code.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> 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 | 25 ++++++++++++++++++++++++
> include/hw/hotplug.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 76 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..47f6555 100644
> --- a/hw/core/Makefile.objs
> +++ b/hw/core/Makefile.objs
> @@ -10,4 +10,5 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
> common-obj-$(CONFIG_SOFTMMU) += null-machine.o
> common-obj-$(CONFIG_SOFTMMU) += loader.o
> common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> +common-obj-$(CONFIG_SOFTMMU) += hotplug.o
>
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> new file mode 100644
> index 0000000..3e84d9c
> --- /dev/null
> +++ b/hw/core/hotplug.c
> @@ -0,0 +1,25 @@
> +/*
> + * Hotplug device 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"
> +
> +static const TypeInfo hotplug_device_info = {
> + .name = TYPE_HOTPLUG_DEVICE,
> + .parent = TYPE_INTERFACE,
> + .class_size = sizeof(HotplugDeviceClass),
> +};
> +
> +static void hotplug_device_register_types(void)
> +{
> + type_register_static(&hotplug_device_info);
> +}
> +
> +type_init(hotplug_device_register_types)
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> new file mode 100644
> index 0000000..cfa79bb
> --- /dev/null
> +++ b/include/hw/hotplug.h
> @@ -0,0 +1,50 @@
> +/*
> + * Hotplug device 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 "hw/qdev-core.h"
> +
> +#define TYPE_HOTPLUG_DEVICE "hotplug-device"
> +
> +#define HOTPLUG_DEVICE_CLASS(klass) \
> + OBJECT_CLASS_CHECK(HotplugDeviceClass, (klass), TYPE_HOTPLUG_DEVICE)
> +#define HOTPLUG_DEVICE_GET_CLASS(obj) \
> + OBJECT_GET_CLASS(HotplugDeviceClass, (obj), TYPE_HOTPLUG_DEVICE)
> +
> +/**
> + * hotplug_fn:
> + * @hotplug_dev: a device performing hotplug/uplug action
/s/uplug/unplug :)
> + * @hotplugged_dev: a device that has been hotplugged
> + * @errp: returns an error if this function fails
> + */
> +typedef void (*hotplug_fn)(DeviceState *hotplug_dev,
> + DeviceState *hotplugged_dev, Error **errp);
> +
> +/**
> + * HotplugDeviceClass:
> + *
> + * Interface to be implemented by a device performing
> + * hardware hotplug/unplug functions.
> + *
> + * @parent: Opaque parent interface.
> + * @hotplug: hotplug callback.
> + * @hot_unplug: hot unplug callback.
> + */
> +typedef struct HotplugDeviceClass {
May I ask why do we call it a class if it is an interface?
I would call it HotplugDeviceInterface, so I will not think
that it is part of the inheritance chain every time I see it in code.
Did I miss something?
Thanks,
Marcel
> + InterfaceClass parent;
> +
> + hotplug_fn hotplug;
> + hotplug_fn hot_unplug;
> +} HotplugDeviceClass;
> +
> +#endif
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-device interface
2013-12-06 17:03 ` [Qemu-devel] [PATCH 4/7] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-device interface Igor Mammedov
@ 2013-12-09 9:02 ` Marcel Apfelbaum
2013-12-09 13:24 ` Igor Mammedov
0 siblings, 1 reply; 34+ messages in thread
From: Marcel Apfelbaum @ 2013-12-09 9:02 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote:
> Split piix4_device_hotplug() into hotplug/unplug callbacks
> and register them as "hotplug-device" interface implementation of
> PIIX4_PM device.
>
> Replace pci_bus_hotplug() wiring with setting link on
> PCI BUS "hotplug-device" property to PIIX4_PM device.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/acpi/piix4.c | 73 +++++++++++++++++++++++++++++++------------------------
> 1 files changed, 41 insertions(+), 32 deletions(-)
>
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 8084a60..8327d80 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_DEVICE_PROPERTY, &local_error);
> + if (error_is_set(&local_error)) {
> + qerror_report_err(local_error);
> + error_free(local_error);
> + abort();
> + }
> + bus->allow_hotplug = 1;
I saw the exact same lines in this patch and the next two,
I would "recycle" the pci_bus_hotplug (changing the signature
and implementation) in order to avoid code duplication.
What do you think?
Thanks,
Marcel
>
> 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(DeviceState *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(DeviceState *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);
> + HotplugDeviceClass *hc = HOTPLUG_DEVICE_CLASS(klass);
>
> k->no_hotplug = 1;
> k->init = piix4_pm_initfn;
> @@ -757,6 +760,8 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
> dc->no_user = 1;
> dc->vmsd = &vmstate_acpi;
> dc->props = piix4_pm_properties;
> + hc->hotplug = piix4_pci_device_hotplug_cb;
> + hc->hot_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_DEVICE },
> + { }
> + }
> };
>
> static void piix4_pm_register_types(void)
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-06 17:03 ` [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface Igor Mammedov
2013-12-06 17:29 ` Paolo Bonzini
@ 2013-12-09 9:09 ` Marcel Apfelbaum
2013-12-09 13:55 ` Igor Mammedov
1 sibling, 1 reply; 34+ messages in thread
From: Marcel Apfelbaum @ 2013-12-09 9:09 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/pci/pci.c | 70 +++++++++++++++++++++++++++++++++------------
> include/hw/pci/pci.h | 10 ------
> include/hw/pci/pci_bus.h | 2 -
> 3 files changed, 51 insertions(+), 31 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 49eca95..26229de 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -35,6 +35,7 @@
> #include "hw/pci/msi.h"
> #include "hw/pci/msix.h"
> #include "exec/address-spaces.h"
> +#include "hw/hotplug.h"
>
> //#define DEBUG_PCI
> #ifdef DEBUG_PCI
> @@ -348,13 +349,6 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
> }
>
> -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
> -{
> - bus->qbus.allow_hotplug = 1;
> - bus->hotplug = hotplug;
> - bus->hotplug_qdev = qdev;
> -}
> -
> PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> void *irq_opaque,
> @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev)
> {
> PCIDevice *pci_dev = (PCIDevice *)qdev;
> PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> + DeviceState *hotplug_dev;
> + Error *local_err = NULL;
> PCIBus *bus;
> int rc;
> bool is_default_rom;
> @@ -1756,32 +1752,68 @@ static int pci_qdev_init(DeviceState *qdev)
> }
> pci_add_option_rom(pci_dev, is_default_rom);
>
> - if (bus->hotplug) {
> - /* Let buses differentiate between hotplug and when device is
> - * enabled during qemu machine creation. */
> - rc = bus->hotplug(bus->hotplug_qdev, pci_dev,
> - qdev->hotplugged ? PCI_HOTPLUG_ENABLED:
> - PCI_COLDPLUG_ENABLED);
> - if (rc != 0) {
> - int r = pci_unregister_device(&pci_dev->qdev);
> - assert(!r);
> - return rc;
> + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
> + &local_err));
> + if (error_is_set(&local_err)) {
> + goto error_exit;
> + }
> + if (hotplug_dev) {
It seems that object_property_get_link never returns NULL
if no error, meaning that if you go for this link
you *must* be sure that the link exists.
If the above is the case, I think you can loose the "if" statement.
Otherwise, if NULL is an option, you would abort the device init unnecessary.
> + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> +
> + /* handler can differentiate between hotplug and when device is
> + * enabled during qemu machine creation by inspecting
> + * dev->hotplugged field. */
> + if (hdc->hotplug) {
> + hdc->hotplug(hotplug_dev, qdev, &local_err);
> + if (error_is_set(&local_err)) {
> + int r = pci_unregister_device(&pci_dev->qdev);
> + assert(!r);
> + goto error_exit;
> + }
> }
> }
> return 0;
> +
> +error_exit:
> + qerror_report_err(local_err);
> + error_free(local_err);
> + return -1;
> }
>
> static int pci_unplug_device(DeviceState *qdev)
> {
> PCIDevice *dev = PCI_DEVICE(qdev);
> PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> + BusState *bus = qdev_get_parent_bus(qdev);
> + DeviceState *hotplug_dev;
> + Error *local_err = NULL;
>
> if (pc->no_hotplug) {
> qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev)));
> return -1;
> }
> - return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
> - PCI_HOTPLUG_DISABLED);
> +
> + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
> + &local_err));
> + if (error_is_set(&local_err)) {
> + goto error_exit;
> + }
> + if (hotplug_dev)
Same as above,
I hope it helped,
Thanks,
Marcel
> + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> +
> + if (hdc->hot_unplug) {
> + hdc->hot_unplug(hotplug_dev, qdev, &local_err);
> + if (error_is_set(&local_err)) {
> + goto error_exit;
> + }
> + }
> + }
> + return 0;
> +
> +error_exit:
> + qerror_report_err(local_err);
> + error_free(local_err);
> + return -1;
> }
>
> PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index b783e68..33dec40 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -330,15 +330,6 @@ typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
> typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
> typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
>
> -typedef enum {
> - PCI_HOTPLUG_DISABLED,
> - PCI_HOTPLUG_ENABLED,
> - PCI_COLDPLUG_ENABLED,
> -} PCIHotplugState;
> -
> -typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
> - PCIHotplugState state);
> -
> #define TYPE_PCI_BUS "PCI"
> #define PCI_BUS(obj) OBJECT_CHECK(PCIBus, (obj), TYPE_PCI_BUS)
> #define TYPE_PCIE_BUS "PCIE"
> @@ -357,7 +348,6 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
> void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> void *irq_opaque, int nirq);
> int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
> -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
> /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
> int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
> PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index 9df1788..fabaeee 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -16,8 +16,6 @@ struct PCIBus {
> pci_set_irq_fn set_irq;
> pci_map_irq_fn map_irq;
> pci_route_irq_fn route_intx_to_irq;
> - pci_hotplug_fn hotplug;
> - DeviceState *hotplug_qdev;
> void *irq_opaque;
> PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX];
> PCIDevice *parent_dev;
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] define hotplug interface
2013-12-09 5:40 ` Li Guang
@ 2013-12-09 9:11 ` Marcel Apfelbaum
2013-12-09 12:44 ` Igor Mammedov
1 sibling, 0 replies; 34+ messages in thread
From: Marcel Apfelbaum @ 2013-12-09 9:11 UTC (permalink / raw)
To: Li Guang
Cc: peter.maydell, ehabkost, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, pbonzini, Igor Mammedov, afaerber
On Mon, 2013-12-09 at 13:40 +0800, Li Guang wrote:
> Hi, Igor
>
> Igor Mammedov wrote:
> > Provide generic hotplug interface for devices.
> > Intended for replacing hotplug mechanism used by
> > PCI/PCIE/SHPC code.
> >
> > Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> > ---
> > 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 | 25 ++++++++++++++++++++++++
> > include/hw/hotplug.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 76 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..47f6555 100644
> > --- a/hw/core/Makefile.objs
> > +++ b/hw/core/Makefile.objs
> > @@ -10,4 +10,5 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
> > common-obj-$(CONFIG_SOFTMMU) += null-machine.o
> > common-obj-$(CONFIG_SOFTMMU) += loader.o
> > common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> > +common-obj-$(CONFIG_SOFTMMU) += hotplug.o
> >
> > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> > new file mode 100644
> > index 0000000..3e84d9c
> > --- /dev/null
> > +++ b/hw/core/hotplug.c
> > @@ -0,0 +1,25 @@
> > +/*
> > + * Hotplug device 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"
> > +
> > +static const TypeInfo hotplug_device_info = {
> > + .name = TYPE_HOTPLUG_DEVICE,
> > + .parent = TYPE_INTERFACE,
> > + .class_size = sizeof(HotplugDeviceClass),
> > +};
> > +
> > +static void hotplug_device_register_types(void)
> > +{
> > + type_register_static(&hotplug_device_info);
> > +}
> > +
> > +type_init(hotplug_device_register_types)
> > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> > new file mode 100644
> > index 0000000..cfa79bb
> > --- /dev/null
> > +++ b/include/hw/hotplug.h
> > @@ -0,0 +1,50 @@
> > +/*
> > + * Hotplug device 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 "hw/qdev-core.h"
> > +
> > +#define TYPE_HOTPLUG_DEVICE "hotplug-device"
> > +
> > +#define HOTPLUG_DEVICE_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(HotplugDeviceClass, (klass), TYPE_HOTPLUG_DEVICE)
> > +#define HOTPLUG_DEVICE_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(HotplugDeviceClass, (obj), TYPE_HOTPLUG_DEVICE)
> > +
> >
>
> Hmm..., this is interface, but device, a bold opinion,
> can have something like TYPE_HOTPLUG_INTERFACE ... ?
Sorry, I did not see this before sending my comments,
I am completely agree with you on this.
Thanks,
Marcel
>
> > +/**
> > + * hotplug_fn:
> > + * @hotplug_dev: a device performing hotplug/uplug action
> >
>
> s/uplug/unplug
>
>
> Thanks!
> Li Guang
>
> > + * @hotplugged_dev: a device that has been hotplugged
> > + * @errp: returns an error if this function fails
> > + */
> > +typedef void (*hotplug_fn)(DeviceState *hotplug_dev,
> > + DeviceState *hotplugged_dev, Error **errp);
> > +
> > +/**
> > + * HotplugDeviceClass:
> > + *
> > + * Interface to be implemented by a device performing
> > + * hardware hotplug/unplug functions.
> > + *
> > + * @parent: Opaque parent interface.
> > + * @hotplug: hotplug callback.
> > + * @hot_unplug: hot unplug callback.
> > + */
> > +typedef struct HotplugDeviceClass {
> > + InterfaceClass parent;
> > +
> > + hotplug_fn hotplug;
> > + hotplug_fn hot_unplug;
> > +} HotplugDeviceClass;
> > +
> > +#endif
> >
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] define hotplug interface
2013-12-06 17:26 ` Paolo Bonzini
@ 2013-12-09 12:42 ` Igor Mammedov
2013-12-09 13:15 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Igor Mammedov @ 2013-12-09 12:42 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, ehabkost, mst, marcel.a, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
On Fri, 06 Dec 2013 18:26:37 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 06/12/2013 18:03, Igor Mammedov ha scritto:
> > Provide generic hotplug interface for devices.
> > Intended for replacing hotplug mechanism used by
> > PCI/PCIE/SHPC code.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > 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 | 25 ++++++++++++++++++++++++
> > include/hw/hotplug.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 76 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..47f6555 100644
> > --- a/hw/core/Makefile.objs
> > +++ b/hw/core/Makefile.objs
> > @@ -10,4 +10,5 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
> > common-obj-$(CONFIG_SOFTMMU) += null-machine.o
> > common-obj-$(CONFIG_SOFTMMU) += loader.o
> > common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> > +common-obj-$(CONFIG_SOFTMMU) += hotplug.o
> >
> > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> > new file mode 100644
> > index 0000000..3e84d9c
> > --- /dev/null
> > +++ b/hw/core/hotplug.c
> > @@ -0,0 +1,25 @@
> > +/*
> > + * Hotplug device 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"
> > +
> > +static const TypeInfo hotplug_device_info = {
> > + .name = TYPE_HOTPLUG_DEVICE,
>
> Perhaps replace "device" with "handler" throughout?
Marcel and Li suggested s/device/interface/ what do you think about it?
>
> > + .parent = TYPE_INTERFACE,
> > + .class_size = sizeof(HotplugDeviceClass),
> > +};
> > +
> > +static void hotplug_device_register_types(void)
> > +{
> > + type_register_static(&hotplug_device_info);
> > +}
> > +
> > +type_init(hotplug_device_register_types)
> > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> > new file mode 100644
> > index 0000000..cfa79bb
> > --- /dev/null
> > +++ b/include/hw/hotplug.h
> > @@ -0,0 +1,50 @@
> > +/*
> > + * Hotplug device 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 "hw/qdev-core.h"
> > +
> > +#define TYPE_HOTPLUG_DEVICE "hotplug-device"
> > +
> > +#define HOTPLUG_DEVICE_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(HotplugDeviceClass, (klass), TYPE_HOTPLUG_DEVICE)
> > +#define HOTPLUG_DEVICE_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(HotplugDeviceClass, (obj), TYPE_HOTPLUG_DEVICE)
>
> Missing HOTPLUG_DEVICE macro, see include/hw/stream.h.
Since there were no users for it, I've dropped it. But I can sure add it.
>
> > +/**
> > + * hotplug_fn:
> > + * @hotplug_dev: a device performing hotplug/uplug action
> > + * @hotplugged_dev: a device that has been hotplugged
> > + * @errp: returns an error if this function fails
> > + */
> > +typedef void (*hotplug_fn)(DeviceState *hotplug_dev,
>
> First argument should be HotplugDevice (which is a struct containing a
> single Object field; again see include/hw/stream.h).
>
> > + DeviceState *hotplugged_dev, Error **errp);
> > +
> > +/**
> > + * HotplugDeviceClass:
> > + *
> > + * Interface to be implemented by a device performing
> > + * hardware hotplug/unplug functions.
> > + *
> > + * @parent: Opaque parent interface.
> > + * @hotplug: hotplug callback.
> > + * @hot_unplug: hot unplug callback.
>
> Just plug/unplug. IIRC it is used for cold-plug too.
>
> Otherwise looks great.
>
> Paolo
>
> > + */
> > +typedef struct HotplugDeviceClass {
> > + InterfaceClass parent;
> > +
> > + hotplug_fn hotplug;
> > + hotplug_fn hot_unplug;
> > +} HotplugDeviceClass;
> > +
> > +#endif
> >
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] define hotplug interface
2013-12-09 5:40 ` Li Guang
2013-12-09 9:11 ` Marcel Apfelbaum
@ 2013-12-09 12:44 ` Igor Mammedov
1 sibling, 0 replies; 34+ messages in thread
From: Igor Mammedov @ 2013-12-09 12:44 UTC (permalink / raw)
To: Li Guang
Cc: peter.maydell, ehabkost, mst, marcel.a, qemu-devel, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
On Mon, 09 Dec 2013 13:40:04 +0800
Li Guang <lig.fnst@cn.fujitsu.com> wrote:
> Hi, Igor
>
> Igor Mammedov wrote:
> > Provide generic hotplug interface for devices.
> > Intended for replacing hotplug mechanism used by
> > PCI/PCIE/SHPC code.
> >
> > Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> > ---
> > 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 | 25 ++++++++++++++++++++++++
> > include/hw/hotplug.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 76 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..47f6555 100644
> > --- a/hw/core/Makefile.objs
> > +++ b/hw/core/Makefile.objs
> > @@ -10,4 +10,5 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
> > common-obj-$(CONFIG_SOFTMMU) += null-machine.o
> > common-obj-$(CONFIG_SOFTMMU) += loader.o
> > common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> > +common-obj-$(CONFIG_SOFTMMU) += hotplug.o
> >
> > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> > new file mode 100644
> > index 0000000..3e84d9c
> > --- /dev/null
> > +++ b/hw/core/hotplug.c
> > @@ -0,0 +1,25 @@
> > +/*
> > + * Hotplug device 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"
> > +
> > +static const TypeInfo hotplug_device_info = {
> > + .name = TYPE_HOTPLUG_DEVICE,
> > + .parent = TYPE_INTERFACE,
> > + .class_size = sizeof(HotplugDeviceClass),
> > +};
> > +
> > +static void hotplug_device_register_types(void)
> > +{
> > + type_register_static(&hotplug_device_info);
> > +}
> > +
> > +type_init(hotplug_device_register_types)
> > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> > new file mode 100644
> > index 0000000..cfa79bb
> > --- /dev/null
> > +++ b/include/hw/hotplug.h
> > @@ -0,0 +1,50 @@
> > +/*
> > + * Hotplug device 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 "hw/qdev-core.h"
> > +
> > +#define TYPE_HOTPLUG_DEVICE "hotplug-device"
> > +
> > +#define HOTPLUG_DEVICE_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(HotplugDeviceClass, (klass), TYPE_HOTPLUG_DEVICE)
> > +#define HOTPLUG_DEVICE_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(HotplugDeviceClass, (obj), TYPE_HOTPLUG_DEVICE)
> > +
> >
>
> Hmm..., this is interface, but device, a bold opinion,
> can have something like TYPE_HOTPLUG_INTERFACE ... ?
Looks good to me, I'll change it.
>
> > +/**
> > + * hotplug_fn:
> > + * @hotplug_dev: a device performing hotplug/uplug action
> >
>
> s/uplug/unplug
>
>
> Thanks!
> Li Guang
>
> > + * @hotplugged_dev: a device that has been hotplugged
> > + * @errp: returns an error if this function fails
> > + */
> > +typedef void (*hotplug_fn)(DeviceState *hotplug_dev,
> > + DeviceState *hotplugged_dev, Error **errp);
> > +
> > +/**
> > + * HotplugDeviceClass:
> > + *
> > + * Interface to be implemented by a device performing
> > + * hardware hotplug/unplug functions.
> > + *
> > + * @parent: Opaque parent interface.
> > + * @hotplug: hotplug callback.
> > + * @hot_unplug: hot unplug callback.
> > + */
> > +typedef struct HotplugDeviceClass {
> > + InterfaceClass parent;
> > +
> > + hotplug_fn hotplug;
> > + hotplug_fn hot_unplug;
> > +} HotplugDeviceClass;
> > +
> > +#endif
> >
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] define hotplug interface
2013-12-09 8:58 ` Marcel Apfelbaum
@ 2013-12-09 12:52 ` Igor Mammedov
0 siblings, 0 replies; 34+ messages in thread
From: Igor Mammedov @ 2013-12-09 12:52 UTC (permalink / raw)
To: Marcel Apfelbaum
Cc: peter.maydell, ehabkost, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
On Mon, 09 Dec 2013 10:58:04 +0200
Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote:
> > Provide generic hotplug interface for devices.
> > Intended for replacing hotplug mechanism used by
> > PCI/PCIE/SHPC code.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > 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 | 25 ++++++++++++++++++++++++
> > include/hw/hotplug.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 76 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..47f6555 100644
> > --- a/hw/core/Makefile.objs
> > +++ b/hw/core/Makefile.objs
> > @@ -10,4 +10,5 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
> > common-obj-$(CONFIG_SOFTMMU) += null-machine.o
> > common-obj-$(CONFIG_SOFTMMU) += loader.o
> > common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
> > +common-obj-$(CONFIG_SOFTMMU) += hotplug.o
> >
> > diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> > new file mode 100644
> > index 0000000..3e84d9c
> > --- /dev/null
> > +++ b/hw/core/hotplug.c
> > @@ -0,0 +1,25 @@
> > +/*
> > + * Hotplug device 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"
> > +
> > +static const TypeInfo hotplug_device_info = {
> > + .name = TYPE_HOTPLUG_DEVICE,
> > + .parent = TYPE_INTERFACE,
> > + .class_size = sizeof(HotplugDeviceClass),
> > +};
> > +
> > +static void hotplug_device_register_types(void)
> > +{
> > + type_register_static(&hotplug_device_info);
> > +}
> > +
> > +type_init(hotplug_device_register_types)
> > diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> > new file mode 100644
> > index 0000000..cfa79bb
> > --- /dev/null
> > +++ b/include/hw/hotplug.h
> > @@ -0,0 +1,50 @@
> > +/*
> > + * Hotplug device 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 "hw/qdev-core.h"
> > +
> > +#define TYPE_HOTPLUG_DEVICE "hotplug-device"
> > +
> > +#define HOTPLUG_DEVICE_CLASS(klass) \
> > + OBJECT_CLASS_CHECK(HotplugDeviceClass, (klass), TYPE_HOTPLUG_DEVICE)
> > +#define HOTPLUG_DEVICE_GET_CLASS(obj) \
> > + OBJECT_GET_CLASS(HotplugDeviceClass, (obj), TYPE_HOTPLUG_DEVICE)
> > +
> > +/**
> > + * hotplug_fn:
> > + * @hotplug_dev: a device performing hotplug/uplug action
> /s/uplug/unplug :)
>
> > + * @hotplugged_dev: a device that has been hotplugged
> > + * @errp: returns an error if this function fails
> > + */
> > +typedef void (*hotplug_fn)(DeviceState *hotplug_dev,
> > + DeviceState *hotplugged_dev, Error **errp);
> > +
> > +/**
> > + * HotplugDeviceClass:
> > + *
> > + * Interface to be implemented by a device performing
> > + * hardware hotplug/unplug functions.
> > + *
> > + * @parent: Opaque parent interface.
> > + * @hotplug: hotplug callback.
> > + * @hot_unplug: hot unplug callback.
> > + */
> > +typedef struct HotplugDeviceClass {
> May I ask why do we call it a class if it is an interface?
> I would call it HotplugDeviceInterface, so I will not think
> that it is part of the inheritance chain every time I see it in code.
> Did I miss something?
We have InterfaceClass as a parent, so it's just consistent to follow
the same pattern.
And naming is in consistence with what I've seen in object.h
and include/hw/stream.h after which I've modeled it.
>
> Thanks,
> Marcel
>
>
> > + InterfaceClass parent;
> > +
> > + hotplug_fn hotplug;
> > + hotplug_fn hot_unplug;
> > +} HotplugDeviceClass;
> > +
> > +#endif
>
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] define hotplug interface
2013-12-09 12:42 ` Igor Mammedov
@ 2013-12-09 13:15 ` Paolo Bonzini
0 siblings, 0 replies; 34+ messages in thread
From: Paolo Bonzini @ 2013-12-09 13:15 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, marcel.a, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
Il 09/12/2013 13:42, Igor Mammedov ha scritto:
> On Fri, 06 Dec 2013 18:26:37 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> Il 06/12/2013 18:03, Igor Mammedov ha scritto:
>>> Provide generic hotplug interface for devices.
>>> Intended for replacing hotplug mechanism used by
>>> PCI/PCIE/SHPC code.
>>>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> 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 | 25 ++++++++++++++++++++++++
>>> include/hw/hotplug.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 76 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..47f6555 100644
>>> --- a/hw/core/Makefile.objs
>>> +++ b/hw/core/Makefile.objs
>>> @@ -10,4 +10,5 @@ common-obj-$(CONFIG_SOFTMMU) += sysbus.o
>>> common-obj-$(CONFIG_SOFTMMU) += null-machine.o
>>> common-obj-$(CONFIG_SOFTMMU) += loader.o
>>> common-obj-$(CONFIG_SOFTMMU) += qdev-properties-system.o
>>> +common-obj-$(CONFIG_SOFTMMU) += hotplug.o
>>>
>>> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
>>> new file mode 100644
>>> index 0000000..3e84d9c
>>> --- /dev/null
>>> +++ b/hw/core/hotplug.c
>>> @@ -0,0 +1,25 @@
>>> +/*
>>> + * Hotplug device 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"
>>> +
>>> +static const TypeInfo hotplug_device_info = {
>>> + .name = TYPE_HOTPLUG_DEVICE,
>>
>> Perhaps replace "device" with "handler" throughout?
> Marcel and Li suggested s/device/interface/ what do you think about it?
I think "handler" and "interface" are two different things.
This is the interface that makes a device able to handle hotplug.
So, "Interface" would be something to use instead of Class in the struct
name if you wanted (e.g. HotplugDeviceInterface or
HotplugHandlerInterface). As you pointed out, however, the convention
seems to be to stick with "Class". If you want to change
StreamSlaveClass to StreamSlaveInterface, and use HotplugXYZInterface
here, that would be okay. But I don't think this is necessary; I just
made it as an example to explain why my request was a bit different from
Marcel and Li's.
>>> +#define TYPE_HOTPLUG_DEVICE "hotplug-device"
>>> +
>>> +#define HOTPLUG_DEVICE_CLASS(klass) \
>>> + OBJECT_CLASS_CHECK(HotplugDeviceClass, (klass), TYPE_HOTPLUG_DEVICE)
>>> +#define HOTPLUG_DEVICE_GET_CLASS(obj) \
>>> + OBJECT_GET_CLASS(HotplugDeviceClass, (obj), TYPE_HOTPLUG_DEVICE)
>>
>> Missing HOTPLUG_DEVICE macro, see include/hw/stream.h.
>
> Since there were no users for it, I've dropped it. But I can sure add it.
I think the other request below will add a user.
Paolo
>>> +/**
>>> + * hotplug_fn:
>>> + * @hotplug_dev: a device performing hotplug/uplug action
>>> + * @hotplugged_dev: a device that has been hotplugged
>>> + * @errp: returns an error if this function fails
>>> + */
>>> +typedef void (*hotplug_fn)(DeviceState *hotplug_dev,
>>
>> First argument should be HotplugDevice (which is a struct containing a
>> single Object field; again see include/hw/stream.h).
>>
>>> + DeviceState *hotplugged_dev, Error **errp);
>>> +
>>> +/**
>>> + * HotplugDeviceClass:
>>> + *
>>> + * Interface to be implemented by a device performing
>>> + * hardware hotplug/unplug functions.
>>> + *
>>> + * @parent: Opaque parent interface.
>>> + * @hotplug: hotplug callback.
>>> + * @hot_unplug: hot unplug callback.
>>
>> Just plug/unplug. IIRC it is used for cold-plug too.
>>
>> Otherwise looks great.
>>
>> Paolo
>>
>>> + */
>>> +typedef struct HotplugDeviceClass {
>>> + InterfaceClass parent;
>>> +
>>> + hotplug_fn hotplug;
>>> + hotplug_fn hot_unplug;
>>> +} HotplugDeviceClass;
>>> +
>>> +#endif
>>>
>>
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-device interface
2013-12-09 9:02 ` Marcel Apfelbaum
@ 2013-12-09 13:24 ` Igor Mammedov
0 siblings, 0 replies; 34+ messages in thread
From: Igor Mammedov @ 2013-12-09 13:24 UTC (permalink / raw)
To: Marcel Apfelbaum
Cc: peter.maydell, ehabkost, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
On Mon, 09 Dec 2013 11:02:29 +0200
Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote:
> > Split piix4_device_hotplug() into hotplug/unplug callbacks
> > and register them as "hotplug-device" interface implementation of
> > PIIX4_PM device.
> >
> > Replace pci_bus_hotplug() wiring with setting link on
> > PCI BUS "hotplug-device" property to PIIX4_PM device.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > hw/acpi/piix4.c | 73 +++++++++++++++++++++++++++++++------------------------
> > 1 files changed, 41 insertions(+), 32 deletions(-)
> >
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index 8084a60..8327d80 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_DEVICE_PROPERTY, &local_error);
> > + if (error_is_set(&local_error)) {
> > + qerror_report_err(local_error);
> > + error_free(local_error);
> > + abort();
> > + }
> > + bus->allow_hotplug = 1;
> I saw the exact same lines in this patch and the next two,
> I would "recycle" the pci_bus_hotplug (changing the signature
> and implementation) in order to avoid code duplication.
> What do you think?
it's not exactly the same, in shpc case we do not abort.
I've left it with a bit of code duplication for reason that in abort case
error check code disappears one Peter's abort_error patch is merged
(it's on my todo list to switch to abort_error once available).
And as for "bus->allow_hotplug = 1" it should also be removed once all
current users are switched to common hotplug interface, then empty link could
serve the same purpose as allow_hotplug.
I'm not sure that creating function to replace essentially 2-liner in 2 places
justifies trouble considering that it should be obsoleted anyway.
> Thanks,
> Marcel
>
> >
> > 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(DeviceState *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(DeviceState *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);
> > + HotplugDeviceClass *hc = HOTPLUG_DEVICE_CLASS(klass);
> >
> > k->no_hotplug = 1;
> > k->init = piix4_pm_initfn;
> > @@ -757,6 +760,8 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
> > dc->no_user = 1;
> > dc->vmsd = &vmstate_acpi;
> > dc->props = piix4_pm_properties;
> > + hc->hotplug = piix4_pci_device_hotplug_cb;
> > + hc->hot_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_DEVICE },
> > + { }
> > + }
> > };
> >
> > static void piix4_pm_register_types(void)
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-06 17:29 ` Paolo Bonzini
@ 2013-12-09 13:41 ` Igor Mammedov
2013-12-09 13:47 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Igor Mammedov @ 2013-12-09 13:41 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, ehabkost, marcel.a, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
On Fri, 06 Dec 2013 18:29:58 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 06/12/2013 18:03, Igor Mammedov ha scritto:
> > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
> > + &local_err));
> > + if (error_is_set(&local_err)) {
> > + goto error_exit;
> > + }
> > + if (hotplug_dev) {
> > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> > +
> > + /* handler can differentiate between hotplug and when device is
> > + * enabled during qemu machine creation by inspecting
> > + * dev->hotplugged field. */
> > + if (hdc->hotplug) {
> > + hdc->hotplug(hotplug_dev, qdev, &local_err);
> > + if (error_is_set(&local_err)) {
> > + int r = pci_unregister_device(&pci_dev->qdev);
> > + assert(!r);
> > + goto error_exit;
> > + }
> > }
> > }
>
> > +
> > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
> > + &local_err));
> > + if (error_is_set(&local_err)) {
> > + goto error_exit;
> > + }
> > + if (hotplug_dev) {
> > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> > +
> > + if (hdc->hot_unplug) {
> > + hdc->hot_unplug(hotplug_dev, qdev, &local_err);
> > + if (error_is_set(&local_err)) {
> > + goto error_exit;
> > + }
> > + }
> > + }
>
> Please move the parts under the "if" to hotplug.c (something like
> hotplug_handler_plug and hotplug_handler_unplug). Also, should this be
sure.
> moved up to generic code (e.g. bus_add_child/bus_remove_child)?
Current usage hints that it's more realize() related part. If handler
fails device might want to do same device specific failure handling
before returning from realize() with failure.
It will work in bus_add_child/bus_remove_child as well but it would
basically put handlers to nofail category.
>
> A NULL link can be a no-op for coldplugged devices, an error for
> hotplugged devices, and an assertion failure for hot-unplug.
>
> Paolo
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-09 13:41 ` Igor Mammedov
@ 2013-12-09 13:47 ` Paolo Bonzini
2013-12-09 14:14 ` Igor Mammedov
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2013-12-09 13:47 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, marcel.a, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
Il 09/12/2013 14:41, Igor Mammedov ha scritto:
>> > Please move the parts under the "if" to hotplug.c (something like
>> > hotplug_handler_plug and hotplug_handler_unplug). Also, should this be
> sure.
>
>> > moved up to generic code (e.g. bus_add_child/bus_remove_child)?
> Current usage hints that it's more realize() related part. If handler
> fails device might want to do same device specific failure handling
> before returning from realize() with failure.
> It will work in bus_add_child/bus_remove_child as well but it would
> basically put handlers to nofail category.
Makes sense. realize() for the "plug" handler, and qdev_unplug for the
unplug handler, I guess.
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-09 9:09 ` Marcel Apfelbaum
@ 2013-12-09 13:55 ` Igor Mammedov
2013-12-09 14:01 ` Marcel Apfelbaum
0 siblings, 1 reply; 34+ messages in thread
From: Igor Mammedov @ 2013-12-09 13:55 UTC (permalink / raw)
To: Marcel Apfelbaum
Cc: peter.maydell, ehabkost, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
On Mon, 09 Dec 2013 11:09:02 +0200
Marcel Apfelbaum <marcel.a@redhat.com> wrote:
> On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > hw/pci/pci.c | 70 +++++++++++++++++++++++++++++++++------------
> > include/hw/pci/pci.h | 10 ------
> > include/hw/pci/pci_bus.h | 2 -
> > 3 files changed, 51 insertions(+), 31 deletions(-)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 49eca95..26229de 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -35,6 +35,7 @@
> > #include "hw/pci/msi.h"
> > #include "hw/pci/msix.h"
> > #include "exec/address-spaces.h"
> > +#include "hw/hotplug.h"
> >
> > //#define DEBUG_PCI
> > #ifdef DEBUG_PCI
> > @@ -348,13 +349,6 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> > bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
> > }
> >
> > -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
> > -{
> > - bus->qbus.allow_hotplug = 1;
> > - bus->hotplug = hotplug;
> > - bus->hotplug_qdev = qdev;
> > -}
> > -
> > PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> > pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> > void *irq_opaque,
> > @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev)
> > {
> > PCIDevice *pci_dev = (PCIDevice *)qdev;
> > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> > + DeviceState *hotplug_dev;
> > + Error *local_err = NULL;
> > PCIBus *bus;
> > int rc;
> > bool is_default_rom;
> > @@ -1756,32 +1752,68 @@ static int pci_qdev_init(DeviceState *qdev)
> > }
> > pci_add_option_rom(pci_dev, is_default_rom);
> >
> > - if (bus->hotplug) {
> > - /* Let buses differentiate between hotplug and when device is
> > - * enabled during qemu machine creation. */
> > - rc = bus->hotplug(bus->hotplug_qdev, pci_dev,
> > - qdev->hotplugged ? PCI_HOTPLUG_ENABLED:
> > - PCI_COLDPLUG_ENABLED);
> > - if (rc != 0) {
> > - int r = pci_unregister_device(&pci_dev->qdev);
> > - assert(!r);
> > - return rc;
> > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
> > + &local_err));
> > + if (error_is_set(&local_err)) {
> > + goto error_exit;
> > + }
> > + if (hotplug_dev) {
> It seems that object_property_get_link never returns NULL
> if no error, meaning that if you go for this link
> you *must* be sure that the link exists.
> If the above is the case, I think you can loose the "if" statement.
> Otherwise, if NULL is an option, you would abort the device init unnecessary.
Link property might exists but it might be not set. in that case get_link
returns NULL without error set. So if nobody bothered to set link, it would mean
skip hotplug section (it's not fail).
>
> > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> > +
> > + /* handler can differentiate between hotplug and when device is
> > + * enabled during qemu machine creation by inspecting
> > + * dev->hotplugged field. */
> > + if (hdc->hotplug) {
> > + hdc->hotplug(hotplug_dev, qdev, &local_err);
> > + if (error_is_set(&local_err)) {
> > + int r = pci_unregister_device(&pci_dev->qdev);
> > + assert(!r);
> > + goto error_exit;
> > + }
> > }
> > }
> > return 0;
> > +
> > +error_exit:
> > + qerror_report_err(local_err);
> > + error_free(local_err);
> > + return -1;
> > }
> >
> > static int pci_unplug_device(DeviceState *qdev)
> > {
> > PCIDevice *dev = PCI_DEVICE(qdev);
> > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> > + BusState *bus = qdev_get_parent_bus(qdev);
> > + DeviceState *hotplug_dev;
> > + Error *local_err = NULL;
> >
> > if (pc->no_hotplug) {
> > qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev)));
> > return -1;
> > }
> > - return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
> > - PCI_HOTPLUG_DISABLED);
> > +
> > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
> > + &local_err));
> > + if (error_is_set(&local_err)) {
> > + goto error_exit;
> > + }
> > + if (hotplug_dev)
> Same as above,
>
> I hope it helped,
> Thanks,
> Marcel
>
> > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> > +
> > + if (hdc->hot_unplug) {
> > + hdc->hot_unplug(hotplug_dev, qdev, &local_err);
> > + if (error_is_set(&local_err)) {
> > + goto error_exit;
> > + }
> > + }
> > + }
> > + return 0;
> > +
> > +error_exit:
> > + qerror_report_err(local_err);
> > + error_free(local_err);
> > + return -1;
> > }
> >
> > PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
> > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > index b783e68..33dec40 100644
> > --- a/include/hw/pci/pci.h
> > +++ b/include/hw/pci/pci.h
> > @@ -330,15 +330,6 @@ typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
> > typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
> > typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
> >
> > -typedef enum {
> > - PCI_HOTPLUG_DISABLED,
> > - PCI_HOTPLUG_ENABLED,
> > - PCI_COLDPLUG_ENABLED,
> > -} PCIHotplugState;
> > -
> > -typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
> > - PCIHotplugState state);
> > -
> > #define TYPE_PCI_BUS "PCI"
> > #define PCI_BUS(obj) OBJECT_CHECK(PCIBus, (obj), TYPE_PCI_BUS)
> > #define TYPE_PCIE_BUS "PCIE"
> > @@ -357,7 +348,6 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
> > void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> > void *irq_opaque, int nirq);
> > int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
> > -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
> > /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
> > int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
> > PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > index 9df1788..fabaeee 100644
> > --- a/include/hw/pci/pci_bus.h
> > +++ b/include/hw/pci/pci_bus.h
> > @@ -16,8 +16,6 @@ struct PCIBus {
> > pci_set_irq_fn set_irq;
> > pci_map_irq_fn map_irq;
> > pci_route_irq_fn route_intx_to_irq;
> > - pci_hotplug_fn hotplug;
> > - DeviceState *hotplug_qdev;
> > void *irq_opaque;
> > PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX];
> > PCIDevice *parent_dev;
>
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-09 13:55 ` Igor Mammedov
@ 2013-12-09 14:01 ` Marcel Apfelbaum
0 siblings, 0 replies; 34+ messages in thread
From: Marcel Apfelbaum @ 2013-12-09 14:01 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, pbonzini, afaerber
On Mon, 2013-12-09 at 14:55 +0100, Igor Mammedov wrote:
> On Mon, 09 Dec 2013 11:09:02 +0200
> Marcel Apfelbaum <marcel.a@redhat.com> wrote:
>
> > On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > hw/pci/pci.c | 70 +++++++++++++++++++++++++++++++++------------
> > > include/hw/pci/pci.h | 10 ------
> > > include/hw/pci/pci_bus.h | 2 -
> > > 3 files changed, 51 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 49eca95..26229de 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -35,6 +35,7 @@
> > > #include "hw/pci/msi.h"
> > > #include "hw/pci/msix.h"
> > > #include "exec/address-spaces.h"
> > > +#include "hw/hotplug.h"
> > >
> > > //#define DEBUG_PCI
> > > #ifdef DEBUG_PCI
> > > @@ -348,13 +349,6 @@ void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> > > bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
> > > }
> > >
> > > -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
> > > -{
> > > - bus->qbus.allow_hotplug = 1;
> > > - bus->hotplug = hotplug;
> > > - bus->hotplug_qdev = qdev;
> > > -}
> > > -
> > > PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> > > pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> > > void *irq_opaque,
> > > @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev)
> > > {
> > > PCIDevice *pci_dev = (PCIDevice *)qdev;
> > > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
> > > + DeviceState *hotplug_dev;
> > > + Error *local_err = NULL;
> > > PCIBus *bus;
> > > int rc;
> > > bool is_default_rom;
> > > @@ -1756,32 +1752,68 @@ static int pci_qdev_init(DeviceState *qdev)
> > > }
> > > pci_add_option_rom(pci_dev, is_default_rom);
> > >
> > > - if (bus->hotplug) {
> > > - /* Let buses differentiate between hotplug and when device is
> > > - * enabled during qemu machine creation. */
> > > - rc = bus->hotplug(bus->hotplug_qdev, pci_dev,
> > > - qdev->hotplugged ? PCI_HOTPLUG_ENABLED:
> > > - PCI_COLDPLUG_ENABLED);
> > > - if (rc != 0) {
> > > - int r = pci_unregister_device(&pci_dev->qdev);
> > > - assert(!r);
> > > - return rc;
> > > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
> > > + &local_err));
> > > + if (error_is_set(&local_err)) {
> > > + goto error_exit;
> > > + }
> > > + if (hotplug_dev) {
> > It seems that object_property_get_link never returns NULL
> > if no error, meaning that if you go for this link
> > you *must* be sure that the link exists.
> > If the above is the case, I think you can loose the "if" statement.
> > Otherwise, if NULL is an option, you would abort the device init unnecessary.
> Link property might exists but it might be not set. in that case get_link
> returns NULL without error set. So if nobody bothered to set link, it would mean
> skip hotplug section (it's not fail).
Got it, thanks!
Marcel
>
> >
> > > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> > > +
> > > + /* handler can differentiate between hotplug and when device is
> > > + * enabled during qemu machine creation by inspecting
> > > + * dev->hotplugged field. */
> > > + if (hdc->hotplug) {
> > > + hdc->hotplug(hotplug_dev, qdev, &local_err);
> > > + if (error_is_set(&local_err)) {
> > > + int r = pci_unregister_device(&pci_dev->qdev);
> > > + assert(!r);
> > > + goto error_exit;
> > > + }
> > > }
> > > }
> > > return 0;
> > > +
> > > +error_exit:
> > > + qerror_report_err(local_err);
> > > + error_free(local_err);
> > > + return -1;
> > > }
> > >
> > > static int pci_unplug_device(DeviceState *qdev)
> > > {
> > > PCIDevice *dev = PCI_DEVICE(qdev);
> > > PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> > > + BusState *bus = qdev_get_parent_bus(qdev);
> > > + DeviceState *hotplug_dev;
> > > + Error *local_err = NULL;
> > >
> > > if (pc->no_hotplug) {
> > > qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev)));
> > > return -1;
> > > }
> > > - return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
> > > - PCI_HOTPLUG_DISABLED);
> > > +
> > > + hotplug_dev = DEVICE(object_property_get_link(OBJECT(bus), "hotplug-device",
> > > + &local_err));
> > > + if (error_is_set(&local_err)) {
> > > + goto error_exit;
> > > + }
> > > + if (hotplug_dev)
> > Same as above,
> >
> > I hope it helped,
> > Thanks,
> > Marcel
> >
> > > + HotplugDeviceClass *hdc = HOTPLUG_DEVICE_GET_CLASS(hotplug_dev);
> > > +
> > > + if (hdc->hot_unplug) {
> > > + hdc->hot_unplug(hotplug_dev, qdev, &local_err);
> > > + if (error_is_set(&local_err)) {
> > > + goto error_exit;
> > > + }
> > > + }
> > > + }
> > > + return 0;
> > > +
> > > +error_exit:
> > > + qerror_report_err(local_err);
> > > + error_free(local_err);
> > > + return -1;
> > > }
> > >
> > > PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
> > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > index b783e68..33dec40 100644
> > > --- a/include/hw/pci/pci.h
> > > +++ b/include/hw/pci/pci.h
> > > @@ -330,15 +330,6 @@ typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
> > > typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
> > > typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
> > >
> > > -typedef enum {
> > > - PCI_HOTPLUG_DISABLED,
> > > - PCI_HOTPLUG_ENABLED,
> > > - PCI_COLDPLUG_ENABLED,
> > > -} PCIHotplugState;
> > > -
> > > -typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
> > > - PCIHotplugState state);
> > > -
> > > #define TYPE_PCI_BUS "PCI"
> > > #define PCI_BUS(obj) OBJECT_CHECK(PCIBus, (obj), TYPE_PCI_BUS)
> > > #define TYPE_PCIE_BUS "PCIE"
> > > @@ -357,7 +348,6 @@ PCIBus *pci_bus_new(DeviceState *parent, const char *name,
> > > void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
> > > void *irq_opaque, int nirq);
> > > int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
> > > -void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
> > > /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
> > > int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
> > > PCIBus *pci_register_bus(DeviceState *parent, const char *name,
> > > diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > > index 9df1788..fabaeee 100644
> > > --- a/include/hw/pci/pci_bus.h
> > > +++ b/include/hw/pci/pci_bus.h
> > > @@ -16,8 +16,6 @@ struct PCIBus {
> > > pci_set_irq_fn set_irq;
> > > pci_map_irq_fn map_irq;
> > > pci_route_irq_fn route_intx_to_irq;
> > > - pci_hotplug_fn hotplug;
> > > - DeviceState *hotplug_qdev;
> > > void *irq_opaque;
> > > PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX];
> > > PCIDevice *parent_dev;
> >
> >
> >
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-09 13:47 ` Paolo Bonzini
@ 2013-12-09 14:14 ` Igor Mammedov
2013-12-09 14:36 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Igor Mammedov @ 2013-12-09 14:14 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, ehabkost, marcel.a, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
On Mon, 09 Dec 2013 14:47:00 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/12/2013 14:41, Igor Mammedov ha scritto:
> >> > Please move the parts under the "if" to hotplug.c (something like
> >> > hotplug_handler_plug and hotplug_handler_unplug). Also, should this be
> > sure.
> >
> >> > moved up to generic code (e.g. bus_add_child/bus_remove_child)?
> > Current usage hints that it's more realize() related part. If handler
> > fails device might want to do same device specific failure handling
> > before returning from realize() with failure.
> > It will work in bus_add_child/bus_remove_child as well but it would
> > basically put handlers to nofail category.
>
> Makes sense. realize() for the "plug" handler, and qdev_unplug for the
> unplug handler, I guess.
Just to be sure, I've meant not DEVICE.realize() but each device specific
one.
qdev_unplug() might work for now, but I haven't checked all devices that
might use interface and if it would break anything. Ideally it should be
in device's unrealize() complementing realize() part.
I'd wait till all buses converted to new interface before attempting to
generalize current plug/unplug call pathes though.
>
> Paolo
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API
2013-12-06 17:31 ` [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Paolo Bonzini
@ 2013-12-09 14:15 ` Igor Mammedov
0 siblings, 0 replies; 34+ messages in thread
From: Igor Mammedov @ 2013-12-09 14:15 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, ehabkost, mst, marcel.a, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
On Fri, 06 Dec 2013 18:31:55 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 06/12/2013 18:03, Igor Mammedov ha scritto:
> > Series is replacement for a more simple attempt to generalize hotplug API
> > http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02529.html
> > promted by Paolo in this patch review.
> > --
> >
> > 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 4-7 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 applyed).
> >
> > git tree for testing:
> > https://github.com/imammedo/qemu/commits/hotplug_dev_inf_v1
> >
> > tested only ACPI hotplug with following HMP commands:
> > ==
> > netdev_add user,id=n1
> > device_add e1000,id=e1,netdev=n1
> > device_del e1
> > ==
> >
> > Igor Mammedov (7):
> > define hotplug interface
> > qdev: add to BusState "hotplug-device" link
> > hw/acpi: move typeinfo to the file end
> > acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-device
> > interface
> > pci/shpc: convert SHPC hotplug to use hotplug-device interface
> > pci/pcie: convert PCIE hotplug to use hotplug-device interface
> > hw/pci: convert PCI bus to use "hotplug-device" interface.
> >
> > hw/acpi/piix4.c | 143 +++++++++++++++++++++-------------------
> > hw/core/Makefile.objs | 1 +
> > hw/core/hotplug.c | 25 +++++++
> > hw/core/qdev.c | 4 +
> > hw/pci-bridge/pci_bridge_dev.c | 9 +++
> > hw/pci/pci.c | 70 ++++++++++++++-----
> > hw/pci/pcie.c | 73 +++++++++++++-------
> > hw/pci/pcie_port.c | 8 ++
> > hw/pci/shpc.c | 132 ++++++++++++++++++++++++-------------
> > include/hw/hotplug.h | 50 ++++++++++++++
> > include/hw/pci/pci.h | 10 ---
> > include/hw/pci/pci_bus.h | 2 -
> > include/hw/pci/pcie.h | 4 +
> > include/hw/pci/shpc.h | 7 ++
> > include/hw/qdev-core.h | 4 +
> > 15 files changed, 372 insertions(+), 170 deletions(-)
> > create mode 100644 hw/core/hotplug.c
> > create mode 100644 include/hw/hotplug.h
> >
>
> I didn't look closely at patches 4-6, but it looks very nice and
> extensible. I will convert SCSI to use this too when it goes in.
>
> Can you pick up my qom patches for interfaces too ("qom: fix
> registration of QOM interfaces")?
sure.
>
> Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-09 14:14 ` Igor Mammedov
@ 2013-12-09 14:36 ` Paolo Bonzini
2013-12-09 15:08 ` Igor Mammedov
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2013-12-09 14:36 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, marcel.a, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
Il 09/12/2013 15:14, Igor Mammedov ha scritto:
>> >
>> > Makes sense. realize() for the "plug" handler, and qdev_unplug for the
>> > unplug handler, I guess.
> Just to be sure, I've meant not DEVICE.realize() but each device specific
> one.
If it's each specific device, then why should the hotplug handler link
be in DeviceState?
I think it should be in device_set_realized.
> qdev_unplug() might work for now, but I haven't checked all devices that
> might use interface and if it would break anything. Ideally it should be
> in device's unrealize() complementing realize() part.
>
> I'd wait till all buses converted to new interface before attempting to
> generalize current plug/unplug call pathes though.
I agree that adding a default behavior for no link probably requires
conversion of all buses. However, looking for the link in the generic
code can be done now.
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-09 14:36 ` Paolo Bonzini
@ 2013-12-09 15:08 ` Igor Mammedov
2013-12-09 15:16 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Igor Mammedov @ 2013-12-09 15:08 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, ehabkost, mst, marcel.a, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
On Mon, 09 Dec 2013 15:36:35 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/12/2013 15:14, Igor Mammedov ha scritto:
> >> >
> >> > Makes sense. realize() for the "plug" handler, and qdev_unplug for the
> >> > unplug handler, I guess.
> > Just to be sure, I've meant not DEVICE.realize() but each device specific
> > one.
>
> If it's each specific device, then why should the hotplug handler link
> be in DeviceState?
The reason I've put it there is to eventually replace allow_hotplug field with it,
it also reduces code duplication (i.e. we wont' have to add it in PCIDevice,
DimmDevice ...) and potentially allows to use NULL for error in
property lookup since each bus will have it.
>
> I think it should be in device_set_realized.
if we dub it nofail then it's fine, otherwise failure path becomes more complicated.
Calling handler in specific device realize() allows to gracefully abort
realize() since that device knows what needs to be done to do so:
For example:
@@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev)
...
+ hdc->hotplug(hotplug_dev, qdev, &local_err);
+ if (error_is_set(&local_err)) {
+ int r = pci_unregister_device(&pci_dev->qdev);
Calling handler in realize will not allow to do it.
It's just much more flexible to call handler from specific device since it knows
when it's the best to call handler and how to deal with failure.
>
> > qdev_unplug() might work for now, but I haven't checked all devices that
> > might use interface and if it would break anything. Ideally it should be
> > in device's unrealize() complementing realize() part.
> >
> > I'd wait till all buses converted to new interface before attempting to
> > generalize current plug/unplug call pathes though.
>
> I agree that adding a default behavior for no link probably requires
> conversion of all buses. However, looking for the link in the generic
> code can be done now.
>
> Paolo
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-09 15:08 ` Igor Mammedov
@ 2013-12-09 15:16 ` Paolo Bonzini
2013-12-09 16:48 ` Igor Mammedov
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2013-12-09 15:16 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, mst, marcel.a, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
Il 09/12/2013 16:08, Igor Mammedov ha scritto:
> On Mon, 09 Dec 2013 15:36:35 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> Il 09/12/2013 15:14, Igor Mammedov ha scritto:
>>>>>
>>>>> Makes sense. realize() for the "plug" handler, and qdev_unplug for the
>>>>> unplug handler, I guess.
>>> Just to be sure, I've meant not DEVICE.realize() but each device specific
>>> one.
>>
>> If it's each specific device, then why should the hotplug handler link
>> be in DeviceState?
> The reason I've put it there is to eventually replace allow_hotplug field with it,
> it also reduces code duplication (i.e. we wont' have to add it in PCIDevice,
> DimmDevice ...) and potentially allows to use NULL for error in
> property lookup since each bus will have it.
I agree that's the right thing to do.
>> I think it should be in device_set_realized.
> if we dub it nofail then it's fine, otherwise failure path becomes more complicated.
>
> Calling handler in specific device realize() allows to gracefully abort
> realize() since that device knows what needs to be done to do so:
> For example:
> @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev)
> ...
> + hdc->hotplug(hotplug_dev, qdev, &local_err);
> + if (error_is_set(&local_err)) {
> + int r = pci_unregister_device(&pci_dev->qdev);
>
> Calling handler in realize will not allow to do it.
> It's just much more flexible to call handler from specific device since it knows
> when it's the best to call handler and how to deal with failure.
We could have separate check/plug methods. Only check can fail, it must
be idempotent, and it can be invoked while the device is still unrealized.
The reason I liked the interface, is because it removes the need for
each bus to add its own plug/unplug handling.
Paolo
>>> qdev_unplug() might work for now, but I haven't checked all devices that
>>> might use interface and if it would break anything. Ideally it should be
>>> in device's unrealize() complementing realize() part.
>>>
>>> I'd wait till all buses converted to new interface before attempting to
>>> generalize current plug/unplug call pathes though.
>>
>> I agree that adding a default behavior for no link probably requires
>> conversion of all buses. However, looking for the link in the generic
>> code can be done now.
>>
>> Paolo
>>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-09 15:16 ` Paolo Bonzini
@ 2013-12-09 16:48 ` Igor Mammedov
2013-12-09 17:18 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Igor Mammedov @ 2013-12-09 16:48 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, ehabkost, mst, marcel.a, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
On Mon, 09 Dec 2013 16:16:57 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/12/2013 16:08, Igor Mammedov ha scritto:
> > On Mon, 09 Dec 2013 15:36:35 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >> Il 09/12/2013 15:14, Igor Mammedov ha scritto:
> >>>>>
> >>>>> Makes sense. realize() for the "plug" handler, and qdev_unplug for the
> >>>>> unplug handler, I guess.
> >>> Just to be sure, I've meant not DEVICE.realize() but each device specific
> >>> one.
> >>
> >> If it's each specific device, then why should the hotplug handler link
> >> be in DeviceState?
> > The reason I've put it there is to eventually replace allow_hotplug field with it,
> > it also reduces code duplication (i.e. we wont' have to add it in PCIDevice,
> > DimmDevice ...) and potentially allows to use NULL for error in
> > property lookup since each bus will have it.
>
> I agree that's the right thing to do.
>
> >> I think it should be in device_set_realized.
> > if we dub it nofail then it's fine, otherwise failure path becomes more complicated.
> >
> > Calling handler in specific device realize() allows to gracefully abort
> > realize() since that device knows what needs to be done to do so:
> > For example:
> > @@ -1720,6 +1714,8 @@ static int pci_qdev_init(DeviceState *qdev)
> > ...
> > + hdc->hotplug(hotplug_dev, qdev, &local_err);
> > + if (error_is_set(&local_err)) {
> > + int r = pci_unregister_device(&pci_dev->qdev);
> >
> > Calling handler in realize will not allow to do it.
> > It's just much more flexible to call handler from specific device since it knows
> > when it's the best to call handler and how to deal with failure.
>
> We could have separate check/plug methods. Only check can fail, it must
> be idempotent, and it can be invoked while the device is still unrealized.
Reasons I've stated before apply to 'check' as well, for only specific device knows
when it's fine to call it. That would just replace "plug" with "check" in realize()
for not much of benefit.
>
> The reason I liked the interface, is because it removes the need for
> each bus to add its own plug/unplug handling.
^^^
Have you meant device instead of bus?
In PCI code it's a PCIDevice who calls hotplug handling, PCI BUS serves only as
a source for plug/upnlug handlers and hotplug_qdev.
This series moves hotplug_qdev to BusState so it could be reused by other buses
and removes from bus not belonging to it plug/unplug callbacks.
It's still improvement over current PCI hotplug code and allows to simplify
other buses as well by removing callbacks from them.
The only way to call callbacks from DEVICE.realize()/unplug(), I see, is if we make
them all nofail, then it would be safe to call them in "realize" property setter.
But we have at least 2 callbacks that can fail:
pcie_cap_slot_hotplug() and shpc_device_hotplug()
maybe more. I'm not an expert on both of them to tell for sure if they could
be made nofail. We can try do it, but it could perfectly apply on top
of this series and could be made later.
Goal of this series was to add and demonstrate reusable hotplug interface as
opposed to PCI specific or SCSI-bus specific ones, so it could be used for memory
hotplug as well. It might not do everything we would like but it looks like a move
the right direction.
If it's wrong direction, I could drop the idea and fallback to an original
less intrusive approach, taking in account your comment to move type definitions
into separate header.
>
> Paolo
>
> >>> qdev_unplug() might work for now, but I haven't checked all devices that
> >>> might use interface and if it would break anything. Ideally it should be
> >>> in device's unrealize() complementing realize() part.
> >>>
> >>> I'd wait till all buses converted to new interface before attempting to
> >>> generalize current plug/unplug call pathes though.
> >>
> >> I agree that adding a default behavior for no link probably requires
> >> conversion of all buses. However, looking for the link in the generic
> >> code can be done now.
> >>
> >> Paolo
> >>
> >
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-09 16:48 ` Igor Mammedov
@ 2013-12-09 17:18 ` Paolo Bonzini
2013-12-09 21:15 ` Igor Mammedov
0 siblings, 1 reply; 34+ messages in thread
From: Paolo Bonzini @ 2013-12-09 17:18 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, mst, marcel.a, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
Il 09/12/2013 17:48, Igor Mammedov ha scritto:
>> >
>> > We could have separate check/plug methods. Only check can fail, it must
>> > be idempotent, and it can be invoked while the device is still unrealized.
> Reasons I've stated before apply to 'check' as well, for only specific device knows
> when it's fine to call it. That would just replace "plug" with "check" in realize()
> for not much of benefit.
Check is idempotent, and can be called before realize makes any change
(it could also be called after the device is added to
/machine/unattached, it's not a big difference).
Plug is called after realize.
>> >
>> > The reason I liked the interface, is because it removes the need for
>> > each bus to add its own plug/unplug handling.
> ^^^
> Have you meant device instead of bus?
I meant each bus-specific abstract class (PCIDevice, SCSIDevice, etc.).
> It's still improvement over current PCI hotplug code and allows to simplify
> other buses as well by removing callbacks from them.
Exactly. But so far you don't get any benefit: no removal of PCI
hotplug code, no removal of allow_hotunplug. What I'm proposing, I
believe, has a small cost and already starts the transition (which I
believe we can complete for 2.0).
> The only way to call callbacks from DEVICE.realize()/unplug(), I see, is if we make
> them all nofail, then it would be safe to call them in "realize" property setter.
> But we have at least 2 callbacks that can fail:
> pcie_cap_slot_hotplug() and shpc_device_hotplug()
Both of them can be handled by a "check" step in the handler.
> Goal of this series was to add and demonstrate reusable hotplug interface as
> opposed to PCI specific or SCSI-bus specific ones, so it could be used for memory
> hotplug as well. It might not do everything we would like but it looks like a move
> the right direction.
> If it's wrong direction, I could drop the idea and fallback to an original
> less intrusive approach, taking in account your comment to move type definitions
> into separate header.
No, absolutely. I think it's the right direction, I just think more
aspects of it should be made generic.
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-09 17:18 ` Paolo Bonzini
@ 2013-12-09 21:15 ` Igor Mammedov
2013-12-09 22:28 ` Paolo Bonzini
0 siblings, 1 reply; 34+ messages in thread
From: Igor Mammedov @ 2013-12-09 21:15 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, ehabkost, mst, marcel.a, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
On Mon, 09 Dec 2013 18:18:42 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/12/2013 17:48, Igor Mammedov ha scritto:
> >> >
> >> > We could have separate check/plug methods. Only check can fail, it must
> >> > be idempotent, and it can be invoked while the device is still unrealized.
> > Reasons I've stated before apply to 'check' as well, for only specific device knows
> > when it's fine to call it. That would just replace "plug" with "check" in realize()
> > for not much of benefit.
>
> Check is idempotent, and can be called before realize makes any change
> (it could also be called after the device is added to
> /machine/unattached, it's not a big difference).
>
> Plug is called after realize.
PCIE case: "check" before realize will work since code that does check depends
only on hotplug device (i.e. PCIE slot) and do not access not yet realized
device at all.
however
SHPC case: check code access pci_slot that is derived from PCIDevice.devfn,
which in turn could be initialized in realize() (see pci_qdev_init() devfn
auto allocation). So it's not possible to call check before realize() it
should be called from realize().
Perhaps other hotplug buses/devices have similar limitations, where it's not
fine to access device state from outside before calling it's realize(), so it
should be some post_realize() hook then to make it generic which leads to the
following:
if ->plug() called after realize() fails, all we need to do is to
fail "realize" property setter. That should cause
qdev_device_add() -> object_unparent() -> device_unparent() -> unrealize()
doing all necessary cleanup.
>
> >> >
> >> > The reason I liked the interface, is because it removes the need for
> >> > each bus to add its own plug/unplug handling.
> > ^^^
> > Have you meant device instead of bus?
>
> I meant each bus-specific abstract class (PCIDevice, SCSIDevice, etc.).
>
> > It's still improvement over current PCI hotplug code and allows to simplify
> > other buses as well by removing callbacks from them.
>
> Exactly. But so far you don't get any benefit: no removal of PCI
> hotplug code, no removal of allow_hotunplug. What I'm proposing, I
> believe, has a small cost and already starts the transition (which I
> believe we can complete for 2.0).
>
> > The only way to call callbacks from DEVICE.realize()/unplug(), I see, is if we make
> > them all nofail, then it would be safe to call them in "realize" property setter.
> > But we have at least 2 callbacks that can fail:
> > pcie_cap_slot_hotplug() and shpc_device_hotplug()
>
> Both of them can be handled by a "check" step in the handler.
>
> > Goal of this series was to add and demonstrate reusable hotplug interface as
> > opposed to PCI specific or SCSI-bus specific ones, so it could be used for memory
> > hotplug as well. It might not do everything we would like but it looks like a move
> > the right direction.
> > If it's wrong direction, I could drop the idea and fallback to an original
> > less intrusive approach, taking in account your comment to move type definitions
> > into separate header.
>
> No, absolutely. I think it's the right direction, I just think more
> aspects of it should be made generic.
>
> Paolo
>
--
Regards,
Igor
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface.
2013-12-09 21:15 ` Igor Mammedov
@ 2013-12-09 22:28 ` Paolo Bonzini
0 siblings, 0 replies; 34+ messages in thread
From: Paolo Bonzini @ 2013-12-09 22:28 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, ehabkost, marcel.a, mst, qemu-devel, blauwirbel,
alex.williamson, anthony, afaerber
Il 09/12/2013 22:15, Igor Mammedov ha scritto:
>> > Check is idempotent, and can be called before realize makes any change
>> > (it could also be called after the device is added to
>> > /machine/unattached, it's not a big difference).
>> >
>> > Plug is called after realize.
> PCIE case: "check" before realize will work since code that does check depends
> only on hotplug device (i.e. PCIE slot) and do not access not yet realized
> device at all.
>
> however
> SHPC case: check code access pci_slot that is derived from PCIDevice.devfn,
> which in turn could be initialized in realize() (see pci_qdev_init() devfn
> auto allocation). So it's not possible to call check before realize() it
> should be called from realize().
>
> Perhaps other hotplug buses/devices have similar limitations, where it's not
> fine to access device state from outside before calling it's realize(), so it
> should be some post_realize() hook then to make it generic which leads to the
> following:
> if ->plug() called after realize() fails, all we need to do is to
> fail "realize" property setter. That should cause
> qdev_device_add() -> object_unparent() -> device_unparent() -> unrealize()
> doing all necessary cleanup.
If you can make it work, that'd be great.
Otherwise, let's do it later, but please make a wiki page with a todo
list. We already have too many items on the same critical path
(hotplug, memdev, NUMA,...).
Paolo
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2013-12-09 22:28 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-06 17:03 [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 1/7] define hotplug interface Igor Mammedov
2013-12-06 17:26 ` Paolo Bonzini
2013-12-09 12:42 ` Igor Mammedov
2013-12-09 13:15 ` Paolo Bonzini
2013-12-09 5:40 ` Li Guang
2013-12-09 9:11 ` Marcel Apfelbaum
2013-12-09 12:44 ` Igor Mammedov
2013-12-09 8:58 ` Marcel Apfelbaum
2013-12-09 12:52 ` Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 2/7] qdev: add to BusState "hotplug-device" link Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 3/7] hw/acpi: move typeinfo to the file end Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 4/7] acpi/piix4pm: convert ACPI PCI hotplug to use hotplug-device interface Igor Mammedov
2013-12-09 9:02 ` Marcel Apfelbaum
2013-12-09 13:24 ` Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 5/7] pci/shpc: convert SHPC " Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 6/7] pci/pcie: convert PCIE " Igor Mammedov
2013-12-06 17:03 ` [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface Igor Mammedov
2013-12-06 17:29 ` Paolo Bonzini
2013-12-09 13:41 ` Igor Mammedov
2013-12-09 13:47 ` Paolo Bonzini
2013-12-09 14:14 ` Igor Mammedov
2013-12-09 14:36 ` Paolo Bonzini
2013-12-09 15:08 ` Igor Mammedov
2013-12-09 15:16 ` Paolo Bonzini
2013-12-09 16:48 ` Igor Mammedov
2013-12-09 17:18 ` Paolo Bonzini
2013-12-09 21:15 ` Igor Mammedov
2013-12-09 22:28 ` Paolo Bonzini
2013-12-09 9:09 ` Marcel Apfelbaum
2013-12-09 13:55 ` Igor Mammedov
2013-12-09 14:01 ` Marcel Apfelbaum
2013-12-06 17:31 ` [Qemu-devel] [PATCH 0/7] Refactor PCI/SHPC/PCIE hotplug to use a more generic hotplug API Paolo Bonzini
2013-12-09 14:15 ` 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.