[Qemu-devel] [PATCH v1 0/7] pci: hotplug handler reworks

[Qemu-devel] [PATCH v1 0/7] pci: hotplug handler reworks

[David Hildenbrand]

This series reworks some pci hotplug handlers (except for s390, that will
require more work but is not required for now).

1. Route all unplug calls via the hotplug handler when called from the
   unplug_request handler. This will be required to get multi-stage
   hotplug handlers running, but also makes sense on its own (just like we
   already did for some CPU/memory hotplug handlers).

2. Introduce some pre_plug handlers where it makes sense already.

3. Call the plug/pre_plug handler also for coldplugged devices. Especially
   pcihp is special as it overwrites hotplug handlers.

This series will not yet factor out pre_plug/plug/unplug from pci device
realize/unrealize functions, this will require more work but this
series is also required first to get it running.

David Hildenbrand (7):
  pcihp: perform check for bus capability in pre_plug handler
  pcihp: overwrite hotplug handler recursively from the start
  pcihp: route unplug via the hotplug handler
  pci/pcie: route unplug via the hotplug handler
  pci/shpc: move hotplug checks to preplug handler
  pci/shpc: route unplug via the hotplug handler
  spapr_pci: route unplug via the hotplug handler

 hw/acpi/pcihp.c                 | 40 +++++++++++++++++++++++-----
 hw/acpi/piix4.c                 | 39 ++++++++++++++-------------
 hw/pci-bridge/pci_bridge_dev.c  | 23 +++++++++++++++-
 hw/pci-bridge/pcie_pci_bridge.c | 23 +++++++++++++++-
 hw/pci/pcie.c                   | 10 ++++++-
 hw/pci/pcie_port.c              |  1 +
 hw/pci/shpc.c                   | 47 ++++++++++++++++++---------------
 hw/ppc/spapr_pci.c              | 33 ++++++++++++++---------
 include/hw/acpi/pcihp.h         |  5 ++++
 include/hw/pci/pcie.h           |  2 ++
 include/hw/pci/shpc.h           |  4 +++
 11 files changed, 165 insertions(+), 62 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v1 1/7] pcihp: perform check for bus capability in pre_plug handler

[David Hildenbrand]

Perform the check in the pre_plug handler. In addition, we need the
capability only if the device is actually hotplugged (and not created
during machine initialization). This is a preparation for coldplugging
devices via that hotplug handler.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/acpi/pcihp.c         | 21 +++++++++++++++------
 hw/acpi/piix4.c         | 16 ++++++++++++++--
 include/hw/acpi/pcihp.h |  2 ++
 3 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 80d42e12ff..5e7cef173c 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -217,17 +217,24 @@ void acpi_pcihp_reset(AcpiPciHpState *s)
     acpi_pcihp_update(s);
 }
 
-void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
-                               DeviceState *dev, Error **errp)
+void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
+                                   DeviceState *dev, Error **errp)
 {
-    PCIDevice *pdev = PCI_DEVICE(dev);
-    int slot = PCI_SLOT(pdev->devfn);
-    int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev));
-    if (bsel < 0) {
+    /* Only hotplugged devices need the hotplug capability. */
+    if (dev->hotplugged &&
+        acpi_pcihp_get_bsel(pci_get_bus(PCI_DEVICE(dev))) < 0) {
         error_setg(errp, "Unsupported bus. Bus doesn't have property '"
                    ACPI_PCIHP_PROP_BSEL "' set");
         return;
     }
+}
+
+void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
+                               DeviceState *dev, Error **errp)
+{
+    PCIDevice *pdev = PCI_DEVICE(dev);
+    int slot = PCI_SLOT(pdev->devfn);
+    int bsel;
 
     /* 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
@@ -236,6 +243,8 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
         return;
     }
 
+    bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev));
+    g_assert(bsel >= 0);
     s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
     acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS);
 }
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index e330f24c71..167afe2cea 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -370,6 +370,18 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
     acpi_pm1_evt_power_down(&s->ar);
 }
 
+static void piix4_device_pre_plug_cb(HotplugHandler *hotplug_dev,
+                                    DeviceState *dev, Error **errp)
+{
+    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
+    } else if (!object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
+               !object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        error_setg(errp, "acpi: device plug request for not supported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
+    }
+}
+
 static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
                                  DeviceState *dev, Error **errp)
 {
@@ -392,8 +404,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
             acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
         }
     } else {
-        error_setg(errp, "acpi: device plug request for not supported device"
-                   " type: %s", object_get_typename(OBJECT(dev)));
+        g_assert_not_reached();
     }
 }
 
@@ -702,6 +713,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
      */
     dc->user_creatable = false;
     dc->hotpluggable = false;
+    hc->pre_plug = piix4_device_pre_plug_cb;
     hc->plug = piix4_device_plug_cb;
     hc->unplug_request = piix4_device_unplug_request_cb;
     hc->unplug = piix4_device_unplug_cb;
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 8a65f99fc8..ce31625850 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -56,6 +56,8 @@ typedef struct AcpiPciHpState {
 void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root,
                      MemoryRegion *address_space_io, bool bridges_enabled);
 
+void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
+                                   DeviceState *dev, Error **errp);
 void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
                                DeviceState *dev, Error **errp);
 void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
-- 
2.17.2

[Qemu-devel] [PATCH v1 2/7] pcihp: overwrite hotplug handler recursively from the start

[David Hildenbrand]

For now, the hotplug handler is not called for devices that are
being cold plugged. The hotplug handler is setup when the machine
initialization is fully done. Only bridges that were cold plugged are
considered.

Set the hotplug handler for the root piix bus directly when realizing.
Overwrite the hotplug handler of bridges when hotplugging/coldplugging
them.

This will now make sure that the ACPI PCI hotplug handler is also called
for cold-plugged devices (also on bridges) and for bridges that were
hotplugged.

When trying to hotplug a device to a hotplugged bridge, we now correctly
get the error message
 "Unsupported bus. Bus doesn't have property 'acpi-pcihp-bsel' set"
Insted of going via the standard PCI hotplug handler.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/acpi/pcihp.c | 10 ++++++++++
 hw/acpi/piix4.c | 16 +---------------
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 5e7cef173c..647b5e8d82 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -30,6 +30,7 @@
 #include "hw/hw.h"
 #include "hw/i386/pc.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bridge.h"
 #include "hw/acpi/acpi.h"
 #include "sysemu/sysemu.h"
 #include "exec/address-spaces.h"
@@ -236,6 +237,15 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
     int slot = PCI_SLOT(pdev->devfn);
     int bsel;
 
+    if (!s->legacy_piix && object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
+        PCIBus *sec = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
+
+        /* Overwrite the default hotplug handler with the ACPI PCI one. */
+        qbus_set_hotplug_handler(BUS(sec), DEVICE(hotplug_dev), &error_abort);
+        /* No need to do it recursively */
+        assert(QLIST_EMPTY(&sec->child));
+    }
+
     /* 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. */
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 167afe2cea..e611778daf 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -446,15 +446,6 @@ static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
     }
 }
 
-static void piix4_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
-{
-    PIIX4PMState *s = opaque;
-
-    /* pci_bus cannot outlive PIIX4PMState, because /machine keeps it alive
-     * and it's not hot-unpluggable */
-    qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s), &error_abort);
-}
-
 static void piix4_pm_machine_ready(Notifier *n, void *opaque)
 {
     PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready);
@@ -468,12 +459,6 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
     pci_conf[0x63] = 0x60;
     pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) |
         (memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
-
-    if (s->use_acpi_pci_hotplug) {
-        pci_for_each_bus(pci_get_bus(d), piix4_update_bus_hotplug, s);
-    } else {
-        piix4_update_bus_hotplug(pci_get_bus(d), s);
-    }
 }
 
 static void piix4_pm_add_propeties(PIIX4PMState *s)
@@ -547,6 +532,7 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
 
     piix4_acpi_system_hot_add_init(pci_address_space_io(dev),
                                    pci_get_bus(dev), s);
+    qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), DEVICE(s), &error_abort);
 
     piix4_pm_add_propeties(s);
 }
-- 
2.17.2

[Qemu-devel] [PATCH v1 3/7] pcihp: route unplug via the hotplug handler

[David Hildenbrand]

Preparation for multi-stage hotplug handlers.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/acpi/pcihp.c         | 11 ++++++++++-
 hw/acpi/piix4.c         |  7 +++++--
 include/hw/acpi/pcihp.h |  3 +++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 647b5e8d82..062f3c9091 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -154,6 +154,7 @@ static bool acpi_pcihp_pc_no_hotplug(AcpiPciHpState *s, PCIDevice *dev)
 
 static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slots)
 {
+    HotplugHandler *hotplug_ctrl;
     BusChild *kid, *next;
     int slot = ctz32(slots);
     PCIBus *bus = acpi_pcihp_find_hotplug_bus(s, bsel);
@@ -171,7 +172,8 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
         PCIDevice *dev = PCI_DEVICE(qdev);
         if (PCI_SLOT(dev->devfn) == slot) {
             if (!acpi_pcihp_pc_no_hotplug(s, dev)) {
-                object_unparent(OBJECT(qdev));
+                hotplug_ctrl = qdev_get_hotplug_handler(qdev);
+                hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort);
             }
         }
     }
@@ -261,6 +263,13 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
 
 void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
                                  DeviceState *dev, Error **errp)
+{
+    object_unparent(OBJECT(dev));
+}
+
+void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
+                                         AcpiPciHpState *s, DeviceState *dev,
+                                         Error **errp)
 {
     PCIDevice *pdev = PCI_DEVICE(dev);
     int slot = PCI_SLOT(pdev->devfn);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index e611778daf..ba5632becc 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -418,8 +418,8 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
         acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
                                       dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
-        acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
-                                    errp);
+        acpi_pcihp_device_unplug_request_cb(hotplug_dev, &s->acpi_pci_hotplug,
+                                            dev, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
                !s->cpu_hotplug_legacy) {
         acpi_cpu_unplug_request_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
@@ -437,6 +437,9 @@ static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
     if (s->acpi_memory_hotplug.is_enabled &&
         object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         acpi_memory_unplug_cb(&s->acpi_memory_hotplug, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
+                                    errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
                !s->cpu_hotplug_legacy) {
         acpi_cpu_unplug_cb(&s->cpuhp_state, dev, errp);
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index ce31625850..8bc4a4c01d 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -62,6 +62,9 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
                                DeviceState *dev, Error **errp);
 void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
                                  DeviceState *dev, Error **errp);
+void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
+                                         AcpiPciHpState *s, DeviceState *dev,
+                                         Error **errp);
 
 /* Called on reset */
 void acpi_pcihp_reset(AcpiPciHpState *s);
-- 
2.17.2

[Qemu-devel] [PATCH v1 4/7] pci/pcie: route unplug via the hotplug handler

[David Hildenbrand]

Preparation for multi-stage hotplug handlers.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/pci/pcie.c         | 10 +++++++++-
 hw/pci/pcie_port.c    |  1 +
 include/hw/pci/pcie.h |  2 ++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 02a7bf3af5..744727dc0b 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -364,11 +364,19 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
     }
 }
 
-static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
+void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                 Error **errp)
 {
     object_unparent(OBJECT(dev));
 }
 
+static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(dev));
+
+    hotplug_handler_unplug(hotplug_ctrl, DEVICE(dev), &error_abort);
+}
+
 void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                          DeviceState *dev, Error **errp)
 {
diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
index 13fe6c6945..6e993da603 100644
--- a/hw/pci/pcie_port.c
+++ b/hw/pci/pcie_port.c
@@ -156,6 +156,7 @@ static void pcie_slot_class_init(ObjectClass *oc, void *data)
     dc->props = pcie_slot_props;
     hc->pre_plug = pcie_cap_slot_pre_plug_cb;
     hc->plug = pcie_cap_slot_hotplug_cb;
+    hc->unplug = pcie_cap_slot_hot_unplug_cb;
     hc->unplug_request = pcie_cap_slot_hot_unplug_request_cb;
 }
 
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 001d230d7d..b6f05a0e43 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -135,6 +135,8 @@ void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                                Error **errp);
 void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp);
+void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                 Error **errp);
 void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                          DeviceState *dev, Error **errp);
 #endif /* QEMU_PCIE_H */
-- 
2.17.2

[Qemu-devel] [PATCH v1 5/7] pci/shpc: move hotplug checks to preplug handler

[David Hildenbrand]

Move the checks to the pre_plug handler. I don't see a reason to check
for the PCI slot when unplugging.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/pci-bridge/pci_bridge_dev.c  | 11 ++++++++--
 hw/pci-bridge/pcie_pci_bridge.c | 11 ++++++++--
 hw/pci/shpc.c                   | 36 ++++++++++++++-------------------
 include/hw/pci/shpc.h           |  2 ++
 4 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 97a8e8b6a4..2362dcbc64 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -206,8 +206,8 @@ static const VMStateDescription pci_bridge_dev_vmstate = {
     }
 };
 
-static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
-                                      DeviceState *dev, Error **errp)
+static void pci_bridge_dev_pre_plug_cb(HotplugHandler *hotplug_dev,
+                                       DeviceState *dev, Error **errp)
 {
     PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
 
@@ -216,6 +216,12 @@ static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
                    "this %s", TYPE_PCI_BRIDGE_DEV);
         return;
     }
+    shpc_device_pre_plug_cb(hotplug_dev, dev, errp);
+}
+
+static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
+                                      DeviceState *dev, Error **errp)
+{
     shpc_device_hotplug_cb(hotplug_dev, dev, errp);
 }
 
@@ -251,6 +257,7 @@ 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->pre_plug = pci_bridge_dev_pre_plug_cb;
     hc->plug = pci_bridge_dev_hotplug_cb;
     hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
 }
diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
index 04cf5a6a92..cde8af0a4e 100644
--- a/hw/pci-bridge/pcie_pci_bridge.c
+++ b/hw/pci-bridge/pcie_pci_bridge.c
@@ -137,8 +137,8 @@ static const VMStateDescription pcie_pci_bridge_dev_vmstate = {
         }
 };
 
-static void pcie_pci_bridge_hotplug_cb(HotplugHandler *hotplug_dev,
-                                      DeviceState *dev, Error **errp)
+static void pcie_pci_bridge_pre_plug_cb(HotplugHandler *hotplug_dev,
+                                        DeviceState *dev, Error **errp)
 {
     PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
 
@@ -147,6 +147,12 @@ static void pcie_pci_bridge_hotplug_cb(HotplugHandler *hotplug_dev,
                    "this %s", TYPE_PCIE_PCI_BRIDGE_DEV);
         return;
     }
+    shpc_device_pre_plug_cb(hotplug_dev, dev, errp);
+}
+
+static void pcie_pci_bridge_hotplug_cb(HotplugHandler *hotplug_dev,
+                                      DeviceState *dev, Error **errp)
+{
     shpc_device_hotplug_cb(hotplug_dev, dev, errp);
 }
 
@@ -180,6 +186,7 @@ static void pcie_pci_bridge_class_init(ObjectClass *klass, void *data)
     dc->props = pcie_pci_bridge_dev_properties;
     dc->reset = &pcie_pci_bridge_reset;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
+    hc->pre_plug = pcie_pci_bridge_pre_plug_cb;
     hc->plug = pcie_pci_bridge_hotplug_cb;
     hc->unplug_request = pcie_pci_bridge_hot_unplug_request_cb;
 }
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index a8462d48bb..1caf4a7ee9 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -482,13 +482,21 @@ static const MemoryRegionOps shpc_mmio_ops = {
         .max_access_size = 4,
     },
 };
-static void shpc_device_hotplug_common(PCIDevice *affected_dev, int *slot,
-                                       SHPCDevice *shpc, Error **errp)
+
+static inline int shpc_device_get_slot(PCIDevice *dev)
+{
+    return SHPC_PCI_TO_IDX(dev->devfn);
+}
+
+void shpc_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                             Error **errp)
 {
-    int pci_slot = PCI_SLOT(affected_dev->devfn);
-    *slot = SHPC_PCI_TO_IDX(pci_slot);
+    SHPCDevice *shpc = PCI_DEVICE(hotplug_dev)->shpc;
+    PCIDevice *pdev = PCI_DEVICE(dev);
+    int pci_slot = PCI_SLOT(pdev->devfn);
+    int slot = shpc_device_get_slot(pdev);
 
-    if (pci_slot < SHPC_IDX_TO_PCI(0) || *slot >= shpc->nslots) {
+    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),
@@ -500,16 +508,9 @@ static void shpc_device_hotplug_common(PCIDevice *affected_dev, int *slot,
 void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                             Error **errp)
 {
-    Error *local_err = NULL;
     PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
     SHPCDevice *shpc = pci_hotplug_dev->shpc;
-    int slot;
-
-    shpc_device_hotplug_common(PCI_DEVICE(dev), &slot, shpc, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
+    int slot = shpc_device_get_slot(PCI_DEVICE(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
@@ -543,18 +544,11 @@ void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
 void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                        DeviceState *dev, Error **errp)
 {
-    Error *local_err = NULL;
     PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
     SHPCDevice *shpc = pci_hotplug_dev->shpc;
+    int slot = shpc_device_get_slot(PCI_DEVICE(dev));
     uint8_t state;
     uint8_t led;
-    int slot;
-
-    shpc_device_hotplug_common(PCI_DEVICE(dev), &slot, shpc, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
 
     shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |= SHPC_SLOT_EVENT_BUTTON;
     state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK);
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index ee19fecf61..82eacf8e96 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -45,6 +45,8 @@ void shpc_free(PCIDevice *dev);
 void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
 
 
+void shpc_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                             Error **errp);
 void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                             Error **errp);
 void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
-- 
2.17.2

[Qemu-devel] [PATCH v1 6/7] pci/shpc: route unplug via the hotplug handler

[David Hildenbrand]

Preparation for multi-stage hotplug handlers.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/pci-bridge/pci_bridge_dev.c  | 14 ++++++++++++++
 hw/pci-bridge/pcie_pci_bridge.c | 14 ++++++++++++++
 hw/pci/shpc.c                   | 11 ++++++++++-
 include/hw/pci/shpc.h           |  2 ++
 4 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 2362dcbc64..978ab2a896 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -225,6 +225,19 @@ static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
     shpc_device_hotplug_cb(hotplug_dev, dev, errp);
 }
 
+static void pci_bridge_dev_hot_unplug_cb(HotplugHandler *hotplug_dev,
+                                         DeviceState *dev, Error **errp)
+{
+    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
+
+    if (!shpc_present(pci_hotplug_dev)) {
+        error_setg(errp, "standard hotplug controller has been disabled for "
+                   "this %s", TYPE_PCI_BRIDGE_DEV);
+        return;
+    }
+    shpc_device_hot_unplug_cb(hotplug_dev, dev, errp);
+}
+
 static void pci_bridge_dev_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                                  DeviceState *dev,
                                                  Error **errp)
@@ -259,6 +272,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     hc->pre_plug = pci_bridge_dev_pre_plug_cb;
     hc->plug = pci_bridge_dev_hotplug_cb;
+    hc->unplug = pci_bridge_dev_hot_unplug_cb;
     hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
 }
 
diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
index cde8af0a4e..6ca711210e 100644
--- a/hw/pci-bridge/pcie_pci_bridge.c
+++ b/hw/pci-bridge/pcie_pci_bridge.c
@@ -156,6 +156,19 @@ static void pcie_pci_bridge_hotplug_cb(HotplugHandler *hotplug_dev,
     shpc_device_hotplug_cb(hotplug_dev, dev, errp);
 }
 
+static void pcie_pci_bridge_hot_unplug_cb(HotplugHandler *hotplug_dev,
+                                          DeviceState *dev, Error **errp)
+{
+    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
+
+    if (!shpc_present(pci_hotplug_dev)) {
+        error_setg(errp, "standard hotplug controller has been disabled for "
+                   "this %s", TYPE_PCIE_PCI_BRIDGE_DEV);
+        return;
+    }
+    shpc_device_hot_unplug_cb(hotplug_dev, dev, errp);
+}
+
 static void pcie_pci_bridge_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                                  DeviceState *dev,
                                                  Error **errp)
@@ -188,6 +201,7 @@ static void pcie_pci_bridge_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     hc->pre_plug = pcie_pci_bridge_pre_plug_cb;
     hc->plug = pcie_pci_bridge_hotplug_cb;
+    hc->unplug = pcie_pci_bridge_hot_unplug_cb;
     hc->unplug_request = pcie_pci_bridge_hot_unplug_request_cb;
 }
 
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 1caf4a7ee9..591f5189c5 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -238,6 +238,7 @@ static void shpc_invalid_command(SHPCDevice *shpc)
 
 static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
 {
+    HotplugHandler *hotplug_ctrl;
     int devfn;
     int pci_slot = SHPC_IDX_TO_PCI(slot);
     for (devfn = PCI_DEVFN(pci_slot, 0);
@@ -245,7 +246,9 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
          ++devfn) {
         PCIDevice *affected_dev = shpc->sec_bus->devices[devfn];
         if (affected_dev) {
-            object_unparent(OBJECT(affected_dev));
+            hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(affected_dev));
+            hotplug_handler_unplug(hotplug_ctrl, DEVICE(affected_dev),
+                                   &error_abort);
         }
     }
 }
@@ -541,6 +544,12 @@ void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
     shpc_interrupt_update(pci_hotplug_dev);
 }
 
+void shpc_device_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                               Error **errp)
+{
+    object_unparent(OBJECT(dev));
+}
+
 void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                        DeviceState *dev, Error **errp)
 {
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 82eacf8e96..40c5ac9bfe 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -49,6 +49,8 @@ void shpc_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                              Error **errp);
 void shpc_device_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                             Error **errp);
+void shpc_device_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
+                               Error **errp);
 void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                        DeviceState *dev, Error **errp);
 
-- 
2.17.2

[Qemu-devel] [PATCH v1 7/7] spapr_pci: route unplug via the hotplug handler

[David Hildenbrand]

Preparation for multi-stage hotplug handlers.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/ppc/spapr_pci.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 58afa46204..64b8591023 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1370,18 +1370,9 @@ static int spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev,
 /* Callback to be called during DRC release. */
 void spapr_phb_remove_pci_device_cb(DeviceState *dev)
 {
-    /* some version guests do not wait for completion of a device
-     * cleanup (generally done asynchronously by the kernel) before
-     * signaling to QEMU that the device is safe, but instead sleep
-     * for some 'safe' period of time. unfortunately on a busy host
-     * this sleep isn't guaranteed to be long enough, resulting in
-     * bad things like IRQ lines being left asserted during final
-     * device removal. to deal with this we call reset just prior
-     * to finalizing the device, which will put the device back into
-     * an 'idle' state, as the device cleanup code expects.
-     */
-    pci_device_reset(PCI_DEVICE(dev));
-    object_unparent(OBJECT(dev));
+    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
+
+    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
 }
 
 static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb,
@@ -1490,6 +1481,23 @@ out:
     }
 }
 
+static void spapr_pci_unplug(HotplugHandler *plug_handler,
+                             DeviceState *plugged_dev, Error **errp)
+{
+    /* some version guests do not wait for completion of a device
+     * cleanup (generally done asynchronously by the kernel) before
+     * signaling to QEMU that the device is safe, but instead sleep
+     * for some 'safe' period of time. unfortunately on a busy host
+     * this sleep isn't guaranteed to be long enough, resulting in
+     * bad things like IRQ lines being left asserted during final
+     * device removal. to deal with this we call reset just prior
+     * to finalizing the device, which will put the device back into
+     * an 'idle' state, as the device cleanup code expects.
+     */
+    pci_device_reset(PCI_DEVICE(plugged_dev));
+    object_unparent(OBJECT(plugged_dev));
+}
+
 static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
                                      DeviceState *plugged_dev, Error **errp)
 {
@@ -1965,6 +1973,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
     dc->user_creatable = true;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     hp->plug = spapr_pci_plug;
+    hp->unplug = spapr_pci_unplug;
     hp->unplug_request = spapr_pci_unplug_request;
 }
 
-- 
2.17.2

Re: [Qemu-devel] [Qemu-ppc] [PATCH v1 7/7] spapr_pci: route unplug via the hotplug handler

[Greg Kurz]

On Wed, 24 Oct 2018 12:19:30 +0200
David Hildenbrand <david@redhat.com> wrote:

> Preparation for multi-stage hotplug handlers.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_pci.c | 33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 58afa46204..64b8591023 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1370,18 +1370,9 @@ static int spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev,
>  /* Callback to be called during DRC release. */
>  void spapr_phb_remove_pci_device_cb(DeviceState *dev)
>  {
> -    /* some version guests do not wait for completion of a device
> -     * cleanup (generally done asynchronously by the kernel) before
> -     * signaling to QEMU that the device is safe, but instead sleep
> -     * for some 'safe' period of time. unfortunately on a busy host
> -     * this sleep isn't guaranteed to be long enough, resulting in
> -     * bad things like IRQ lines being left asserted during final
> -     * device removal. to deal with this we call reset just prior
> -     * to finalizing the device, which will put the device back into
> -     * an 'idle' state, as the device cleanup code expects.
> -     */
> -    pci_device_reset(PCI_DEVICE(dev));
> -    object_unparent(OBJECT(dev));
> +    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +
> +    hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
>  }
>  
>  static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb,
> @@ -1490,6 +1481,23 @@ out:
>      }
>  }
>  
> +static void spapr_pci_unplug(HotplugHandler *plug_handler,
> +                             DeviceState *plugged_dev, Error **errp)
> +{
> +    /* some version guests do not wait for completion of a device
> +     * cleanup (generally done asynchronously by the kernel) before
> +     * signaling to QEMU that the device is safe, but instead sleep
> +     * for some 'safe' period of time. unfortunately on a busy host
> +     * this sleep isn't guaranteed to be long enough, resulting in
> +     * bad things like IRQ lines being left asserted during final
> +     * device removal. to deal with this we call reset just prior
> +     * to finalizing the device, which will put the device back into
> +     * an 'idle' state, as the device cleanup code expects.
> +     */
> +    pci_device_reset(PCI_DEVICE(plugged_dev));
> +    object_unparent(OBJECT(plugged_dev));
> +}
> +
>  static void spapr_pci_unplug_request(HotplugHandler *plug_handler,
>                                       DeviceState *plugged_dev, Error **errp)
>  {
> @@ -1965,6 +1973,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>      dc->user_creatable = true;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      hp->plug = spapr_pci_plug;
> +    hp->unplug = spapr_pci_unplug;
>      hp->unplug_request = spapr_pci_unplug_request;
>  }
>  

Re: [Qemu-devel] [PATCH v1 0/7] pci: hotplug handler reworks

[David Hildenbrand]

On 24.10.18 12:19, David Hildenbrand wrote:
> This series reworks some pci hotplug handlers (except for s390, that will
> require more work but is not required for now).
> 
> 1. Route all unplug calls via the hotplug handler when called from the
>    unplug_request handler. This will be required to get multi-stage
>    hotplug handlers running, but also makes sense on its own (just like we
>    already did for some CPU/memory hotplug handlers).
> 
> 2. Introduce some pre_plug handlers where it makes sense already.
> 
> 3. Call the plug/pre_plug handler also for coldplugged devices. Especially
>    pcihp is special as it overwrites hotplug handlers.
> 
> This series will not yet factor out pre_plug/plug/unplug from pci device
> realize/unrealize functions, this will require more work but this
> series is also required first to get it running.
> 
> David Hildenbrand (7):
>   pcihp: perform check for bus capability in pre_plug handler
>   pcihp: overwrite hotplug handler recursively from the start
>   pcihp: route unplug via the hotplug handler
>   pci/pcie: route unplug via the hotplug handler
>   pci/shpc: move hotplug checks to preplug handler
>   pci/shpc: route unplug via the hotplug handler
>   spapr_pci: route unplug via the hotplug handler
> 
>  hw/acpi/pcihp.c                 | 40 +++++++++++++++++++++++-----
>  hw/acpi/piix4.c                 | 39 ++++++++++++++-------------
>  hw/pci-bridge/pci_bridge_dev.c  | 23 +++++++++++++++-
>  hw/pci-bridge/pcie_pci_bridge.c | 23 +++++++++++++++-
>  hw/pci/pcie.c                   | 10 ++++++-
>  hw/pci/pcie_port.c              |  1 +
>  hw/pci/shpc.c                   | 47 ++++++++++++++++++---------------
>  hw/ppc/spapr_pci.c              | 33 ++++++++++++++---------
>  include/hw/acpi/pcihp.h         |  5 ++++
>  include/hw/pci/pcie.h           |  2 ++
>  include/hw/pci/shpc.h           |  4 +++
>  11 files changed, 165 insertions(+), 62 deletions(-)
> 

Did some more testing. Can somebody have a look/pick up? Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 2/7] pcihp: overwrite hotplug handler recursively from the start

[Igor Mammedov]

On Wed, 24 Oct 2018 12:19:25 +0200
David Hildenbrand <david@redhat.com> wrote:

> For now, the hotplug handler is not called for devices that are
> being cold plugged. The hotplug handler is setup when the machine
> initialization is fully done. Only bridges that were cold plugged are
> considered.
> 
> Set the hotplug handler for the root piix bus directly when realizing.
> Overwrite the hotplug handler of bridges when hotplugging/coldplugging
> them.
> 
> This will now make sure that the ACPI PCI hotplug handler is also called
> for cold-plugged devices (also on bridges) and for bridges that were
> hotplugged.
> 
> When trying to hotplug a device to a hotplugged bridge, we now correctly
> get the error message
>  "Unsupported bus. Bus doesn't have property 'acpi-pcihp-bsel' set"
> Insted of going via the standard PCI hotplug handler.
Erroring out is probably not ok, since it can break existing setups
where SHPC hotplugging to hotplugged bridge was working just fine before.

Marcel/Michael what's your take on this change in behaviour?
CCing libvirt in case they are doing this stuff


> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/acpi/pcihp.c | 10 ++++++++++
>  hw/acpi/piix4.c | 16 +---------------
>  2 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 5e7cef173c..647b5e8d82 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -30,6 +30,7 @@
>  #include "hw/hw.h"
>  #include "hw/i386/pc.h"
>  #include "hw/pci/pci.h"
> +#include "hw/pci/pci_bridge.h"
>  #include "hw/acpi/acpi.h"
>  #include "sysemu/sysemu.h"
>  #include "exec/address-spaces.h"
> @@ -236,6 +237,15 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>      int slot = PCI_SLOT(pdev->devfn);
>      int bsel;
>  
> +    if (!s->legacy_piix && object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
> +        PCIBus *sec = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
> +
> +        /* Overwrite the default hotplug handler with the ACPI PCI one. */
> +        qbus_set_hotplug_handler(BUS(sec), DEVICE(hotplug_dev), &error_abort);
> +        /* No need to do it recursively */
> +        assert(QLIST_EMPTY(&sec->child));
> +    }
> +
>      /* 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. */
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 167afe2cea..e611778daf 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -446,15 +446,6 @@ static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
>      }
>  }
>  
> -static void piix4_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
> -{
> -    PIIX4PMState *s = opaque;
> -
> -    /* pci_bus cannot outlive PIIX4PMState, because /machine keeps it alive
> -     * and it's not hot-unpluggable */
> -    qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s), &error_abort);
> -}
> -
>  static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>  {
>      PIIX4PMState *s = container_of(n, PIIX4PMState, machine_ready);
> @@ -468,12 +459,6 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>      pci_conf[0x63] = 0x60;
>      pci_conf[0x67] = (memory_region_present(io_as, 0x3f8) ? 0x08 : 0) |
>          (memory_region_present(io_as, 0x2f8) ? 0x90 : 0);
> -
> -    if (s->use_acpi_pci_hotplug) {
> -        pci_for_each_bus(pci_get_bus(d), piix4_update_bus_hotplug, s);
> -    } else {
> -        piix4_update_bus_hotplug(pci_get_bus(d), s);
> -    }
>  }
>  
>  static void piix4_pm_add_propeties(PIIX4PMState *s)
> @@ -547,6 +532,7 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>  
>      piix4_acpi_system_hot_add_init(pci_address_space_io(dev),
>                                     pci_get_bus(dev), s);
> +    qbus_set_hotplug_handler(BUS(pci_get_bus(dev)), DEVICE(s), &error_abort);
>  
>      piix4_pm_add_propeties(s);
>  }

Re: [Qemu-devel] [PATCH v1 1/7] pcihp: perform check for bus capability in pre_plug handler

[Igor Mammedov]

On Wed, 24 Oct 2018 12:19:24 +0200
David Hildenbrand <david@redhat.com> wrote:

> Perform the check in the pre_plug handler. In addition, we need the
> capability only if the device is actually hotplugged (and not created
> during machine initialization). This is a preparation for coldplugging
> devices via that hotplug handler.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/pcihp.c         | 21 +++++++++++++++------
>  hw/acpi/piix4.c         | 16 ++++++++++++++--
>  include/hw/acpi/pcihp.h |  2 ++
>  3 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 80d42e12ff..5e7cef173c 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -217,17 +217,24 @@ void acpi_pcihp_reset(AcpiPciHpState *s)
>      acpi_pcihp_update(s);
>  }
>  
> -void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
> -                               DeviceState *dev, Error **errp)
> +void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> +                                   DeviceState *dev, Error **errp)
>  {
> -    PCIDevice *pdev = PCI_DEVICE(dev);
> -    int slot = PCI_SLOT(pdev->devfn);
> -    int bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev));
> -    if (bsel < 0) {
> +    /* Only hotplugged devices need the hotplug capability. */
> +    if (dev->hotplugged &&
> +        acpi_pcihp_get_bsel(pci_get_bus(PCI_DEVICE(dev))) < 0) {
>          error_setg(errp, "Unsupported bus. Bus doesn't have property '"
>                     ACPI_PCIHP_PROP_BSEL "' set");
>          return;
>      }
> +}
> +
> +void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
> +                               DeviceState *dev, Error **errp)
> +{
> +    PCIDevice *pdev = PCI_DEVICE(dev);
> +    int slot = PCI_SLOT(pdev->devfn);
> +    int bsel;
>  
>      /* 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
> @@ -236,6 +243,8 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>          return;
>      }
>  
> +    bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev));
> +    g_assert(bsel >= 0);
>      s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
>      acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS);
>  }
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index e330f24c71..167afe2cea 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -370,6 +370,18 @@ static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
>      acpi_pm1_evt_power_down(&s->ar);
>  }
>  
> +static void piix4_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> +                                    DeviceState *dev, Error **errp)
> +{
> +    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> +        acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
> +    } else if (!object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> +               !object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        error_setg(errp, "acpi: device plug request for not supported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));
> +    }
> +}
> +
>  static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>                                   DeviceState *dev, Error **errp)
>  {
> @@ -392,8 +404,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>              acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
>          }
>      } else {
> -        error_setg(errp, "acpi: device plug request for not supported device"
> -                   " type: %s", object_get_typename(OBJECT(dev)));
> +        g_assert_not_reached();
>      }
>  }
>  
> @@ -702,6 +713,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
>       */
>      dc->user_creatable = false;
>      dc->hotpluggable = false;
> +    hc->pre_plug = piix4_device_pre_plug_cb;
>      hc->plug = piix4_device_plug_cb;
>      hc->unplug_request = piix4_device_unplug_request_cb;
>      hc->unplug = piix4_device_unplug_cb;
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index 8a65f99fc8..ce31625850 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -56,6 +56,8 @@ typedef struct AcpiPciHpState {
>  void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root,
>                       MemoryRegion *address_space_io, bool bridges_enabled);
>  
> +void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> +                                   DeviceState *dev, Error **errp);
>  void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>                                 DeviceState *dev, Error **errp);
>  void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,

Re: [Qemu-devel] [PATCH v1 3/7] pcihp: route unplug via the hotplug handler

[Igor Mammedov]

On Wed, 24 Oct 2018 12:19:26 +0200
David Hildenbrand <david@redhat.com> wrote:

> Preparation for multi-stage hotplug handlers.
Commit message should describe what and why the patch does
for the sake of unaware. (I know 'why' right now but will for
sure forget all the reasons later)

beside of rerouting it also makes likes of
acpi_pcihp_device_unplug_cb()
to be called from unplug chain vs current request chain which
I bet were confusing.

Otherwise patch looks good

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/acpi/pcihp.c         | 11 ++++++++++-
>  hw/acpi/piix4.c         |  7 +++++--
>  include/hw/acpi/pcihp.h |  3 +++
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 647b5e8d82..062f3c9091 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -154,6 +154,7 @@ static bool acpi_pcihp_pc_no_hotplug(AcpiPciHpState *s, PCIDevice *dev)
>  
>  static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slots)
>  {
> +    HotplugHandler *hotplug_ctrl;
>      BusChild *kid, *next;
>      int slot = ctz32(slots);
>      PCIBus *bus = acpi_pcihp_find_hotplug_bus(s, bsel);
> @@ -171,7 +172,8 @@ static void acpi_pcihp_eject_slot(AcpiPciHpState *s, unsigned bsel, unsigned slo
>          PCIDevice *dev = PCI_DEVICE(qdev);
>          if (PCI_SLOT(dev->devfn) == slot) {
>              if (!acpi_pcihp_pc_no_hotplug(s, dev)) {
> -                object_unparent(OBJECT(qdev));
> +                hotplug_ctrl = qdev_get_hotplug_handler(qdev);
> +                hotplug_handler_unplug(hotplug_ctrl, qdev, &error_abort);
>              }
>          }
>      }
> @@ -261,6 +263,13 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>  
>  void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>                                   DeviceState *dev, Error **errp)
> +{
> +    object_unparent(OBJECT(dev));
> +}
> +
> +void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> +                                         AcpiPciHpState *s, DeviceState *dev,
> +                                         Error **errp)
>  {
>      PCIDevice *pdev = PCI_DEVICE(dev);
>      int slot = PCI_SLOT(pdev->devfn);
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index e611778daf..ba5632becc 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -418,8 +418,8 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>          acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug,
>                                        dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> -        acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> -                                    errp);
> +        acpi_pcihp_device_unplug_request_cb(hotplug_dev, &s->acpi_pci_hotplug,
> +                                            dev, errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
>                 !s->cpu_hotplug_legacy) {
>          acpi_cpu_unplug_request_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
> @@ -437,6 +437,9 @@ static void piix4_device_unplug_cb(HotplugHandler *hotplug_dev,
>      if (s->acpi_memory_hotplug.is_enabled &&
>          object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          acpi_memory_unplug_cb(&s->acpi_memory_hotplug, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> +        acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> +                                    errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
>                 !s->cpu_hotplug_legacy) {
>          acpi_cpu_unplug_cb(&s->cpuhp_state, dev, errp);
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index ce31625850..8bc4a4c01d 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -62,6 +62,9 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>                                 DeviceState *dev, Error **errp);
>  void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>                                   DeviceState *dev, Error **errp);
> +void acpi_pcihp_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> +                                         AcpiPciHpState *s, DeviceState *dev,
> +                                         Error **errp);
>  
>  /* Called on reset */
>  void acpi_pcihp_reset(AcpiPciHpState *s);

Re: [Qemu-devel] [PATCH v1 4/7] pci/pcie: route unplug via the hotplug handler

[Igor Mammedov]

On Wed, 24 Oct 2018 12:19:27 +0200
David Hildenbrand <david@redhat.com> wrote:

> Preparation for multi-stage hotplug handlers.
commit message needs a rationale shat/why is done here, especially
if it's preliminary refactoring for some future work
(which I'd shortly describe here as well).

The same applies to patches 5-7.

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/pci/pcie.c         | 10 +++++++++-
>  hw/pci/pcie_port.c    |  1 +
>  include/hw/pci/pcie.h |  2 ++
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 02a7bf3af5..744727dc0b 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -364,11 +364,19 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>      }
>  }
>  
> -static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> +void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                                 Error **errp)
>  {
>      object_unparent(OBJECT(dev));
>  }
>  
> +static void pcie_unplug_device(PCIBus *bus, PCIDevice *dev, void *opaque)
> +{
> +    HotplugHandler *hotplug_ctrl = qdev_get_hotplug_handler(DEVICE(dev));
> +
> +    hotplug_handler_unplug(hotplug_ctrl, DEVICE(dev), &error_abort);
> +}
> +
>  void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                           DeviceState *dev, Error **errp)
>  {
> diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
> index 13fe6c6945..6e993da603 100644
> --- a/hw/pci/pcie_port.c
> +++ b/hw/pci/pcie_port.c
> @@ -156,6 +156,7 @@ static void pcie_slot_class_init(ObjectClass *oc, void *data)
>      dc->props = pcie_slot_props;
>      hc->pre_plug = pcie_cap_slot_pre_plug_cb;
>      hc->plug = pcie_cap_slot_hotplug_cb;
> +    hc->unplug = pcie_cap_slot_hot_unplug_cb;
>      hc->unplug_request = pcie_cap_slot_hot_unplug_request_cb;
>  }
>  
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index 001d230d7d..b6f05a0e43 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -135,6 +135,8 @@ void pcie_cap_slot_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                 Error **errp);
>  void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                Error **errp);
> +void pcie_cap_slot_hot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                                 Error **errp);
>  void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                           DeviceState *dev, Error **errp);
>  #endif /* QEMU_PCIE_H */

Re: [Qemu-devel] [PATCH v1 0/7] pci: hotplug handler reworks

[Igor Mammedov]

On Wed, 31 Oct 2018 18:31:30 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 24.10.18 12:19, David Hildenbrand wrote:
> > This series reworks some pci hotplug handlers (except for s390, that will
> > require more work but is not required for now).
> > 
> > 1. Route all unplug calls via the hotplug handler when called from the
> >    unplug_request handler. This will be required to get multi-stage
> >    hotplug handlers running, but also makes sense on its own (just like we
> >    already did for some CPU/memory hotplug handlers).
> > 
> > 2. Introduce some pre_plug handlers where it makes sense already.
> > 
> > 3. Call the plug/pre_plug handler also for coldplugged devices. Especially
> >    pcihp is special as it overwrites hotplug handlers.
> > 
> > This series will not yet factor out pre_plug/plug/unplug from pci device
> > realize/unrealize functions, this will require more work but this
> > series is also required first to get it running.
> > 
> > David Hildenbrand (7):
> >   pcihp: perform check for bus capability in pre_plug handler
> >   pcihp: overwrite hotplug handler recursively from the start
> >   pcihp: route unplug via the hotplug handler
> >   pci/pcie: route unplug via the hotplug handler
> >   pci/shpc: move hotplug checks to preplug handler
> >   pci/shpc: route unplug via the hotplug handler
> >   spapr_pci: route unplug via the hotplug handler
> > 
> >  hw/acpi/pcihp.c                 | 40 +++++++++++++++++++++++-----
> >  hw/acpi/piix4.c                 | 39 ++++++++++++++-------------
> >  hw/pci-bridge/pci_bridge_dev.c  | 23 +++++++++++++++-
> >  hw/pci-bridge/pcie_pci_bridge.c | 23 +++++++++++++++-
> >  hw/pci/pcie.c                   | 10 ++++++-
> >  hw/pci/pcie_port.c              |  1 +
> >  hw/pci/shpc.c                   | 47 ++++++++++++++++++---------------
> >  hw/ppc/spapr_pci.c              | 33 ++++++++++++++---------
> >  include/hw/acpi/pcihp.h         |  5 ++++
> >  include/hw/pci/pcie.h           |  2 ++
> >  include/hw/pci/shpc.h           |  4 +++
> >  11 files changed, 165 insertions(+), 62 deletions(-)
> >   
> 
> Did some more testing. Can somebody have a look/pick up? Thanks!
Did a quick pass over series, patches overall looks good here is some other nits
that apply to series:
 * make more descriptive commit messages (important for history and for whoever comes later to read it)
 * I don't really like a mix of style in callbacks naming
    for ex: there is hotplug and then for unplug you add hot_unplug instead of hotunplug
    I'd prefer consistent approach in naming.
    (either use underscores or drop them or maybe drop 'hot' part as it's not only for hotplug anymore)

Re: [Qemu-devel] [PATCH v1 0/7] pci: hotplug handler reworks

[David Hildenbrand]

On 01.11.18 15:55, Igor Mammedov wrote:
> On Wed, 31 Oct 2018 18:31:30 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 24.10.18 12:19, David Hildenbrand wrote:
>>> This series reworks some pci hotplug handlers (except for s390, that will
>>> require more work but is not required for now).
>>>
>>> 1. Route all unplug calls via the hotplug handler when called from the
>>>    unplug_request handler. This will be required to get multi-stage
>>>    hotplug handlers running, but also makes sense on its own (just like we
>>>    already did for some CPU/memory hotplug handlers).
>>>
>>> 2. Introduce some pre_plug handlers where it makes sense already.
>>>
>>> 3. Call the plug/pre_plug handler also for coldplugged devices. Especially
>>>    pcihp is special as it overwrites hotplug handlers.
>>>
>>> This series will not yet factor out pre_plug/plug/unplug from pci device
>>> realize/unrealize functions, this will require more work but this
>>> series is also required first to get it running.
>>>
>>> David Hildenbrand (7):
>>>   pcihp: perform check for bus capability in pre_plug handler
>>>   pcihp: overwrite hotplug handler recursively from the start
>>>   pcihp: route unplug via the hotplug handler
>>>   pci/pcie: route unplug via the hotplug handler
>>>   pci/shpc: move hotplug checks to preplug handler
>>>   pci/shpc: route unplug via the hotplug handler
>>>   spapr_pci: route unplug via the hotplug handler
>>>
>>>  hw/acpi/pcihp.c                 | 40 +++++++++++++++++++++++-----
>>>  hw/acpi/piix4.c                 | 39 ++++++++++++++-------------
>>>  hw/pci-bridge/pci_bridge_dev.c  | 23 +++++++++++++++-
>>>  hw/pci-bridge/pcie_pci_bridge.c | 23 +++++++++++++++-
>>>  hw/pci/pcie.c                   | 10 ++++++-
>>>  hw/pci/pcie_port.c              |  1 +
>>>  hw/pci/shpc.c                   | 47 ++++++++++++++++++---------------
>>>  hw/ppc/spapr_pci.c              | 33 ++++++++++++++---------
>>>  include/hw/acpi/pcihp.h         |  5 ++++
>>>  include/hw/pci/pcie.h           |  2 ++
>>>  include/hw/pci/shpc.h           |  4 +++
>>>  11 files changed, 165 insertions(+), 62 deletions(-)
>>>   
>>
>> Did some more testing. Can somebody have a look/pick up? Thanks!
> Did a quick pass over series, patches overall looks good here is some other nits
> that apply to series:

Thanks for the review, will address the comments tomorrow!

>  * make more descriptive commit messages (important for history and for whoever comes later to read it)

Yes, I'll add more details. I agree that people without context might
have a harder time figuring out why this is done.

>  * I don't really like a mix of style in callbacks naming
>     for ex: there is hotplug and then for unplug you add hot_unplug instead of hotunplug
>     I'd prefer consistent approach in naming.
>     (either use underscores or drop them or maybe drop 'hot' part as it's not only for hotplug anymore)

Yes, me too. And I guess I will add some cleanup patches first, that
will introduce a common naming scheme, (especially dropping the "hot")


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 2/7] pcihp: overwrite hotplug handler recursively from the start

[David Hildenbrand]

On 01.11.18 15:10, Igor Mammedov wrote:
> On Wed, 24 Oct 2018 12:19:25 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> For now, the hotplug handler is not called for devices that are
>> being cold plugged. The hotplug handler is setup when the machine
>> initialization is fully done. Only bridges that were cold plugged are
>> considered.
>>
>> Set the hotplug handler for the root piix bus directly when realizing.
>> Overwrite the hotplug handler of bridges when hotplugging/coldplugging
>> them.
>>
>> This will now make sure that the ACPI PCI hotplug handler is also called
>> for cold-plugged devices (also on bridges) and for bridges that were
>> hotplugged.
>>
>> When trying to hotplug a device to a hotplugged bridge, we now correctly
>> get the error message
>>  "Unsupported bus. Bus doesn't have property 'acpi-pcihp-bsel' set"
>> Insted of going via the standard PCI hotplug handler.
> Erroring out is probably not ok, since it can break existing setups
> where SHPC hotplugging to hotplugged bridge was working just fine before.

The question is if it actually was supposed (and eventually did) work.

If this was the expected behavior (mixing hotplug types), then the
necessary change to this patch would boil down to checking if the bridge
it hot or coldplugged.

> 
> Marcel/Michael what's your take on this change in behaviour?
> CCing libvirt in case they are doing this stuff
> 

Indeed, it would be nice to know if this was actually supposed to work
like this (coldplugged bridges using ACPI hotplug and hotplugged bridges
using SHPC hotplug).


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 2/7] pcihp: overwrite hotplug handler recursively from the start

[Igor Mammedov]

On Fri, 2 Nov 2018 12:43:10 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 01.11.18 15:10, Igor Mammedov wrote:
> > On Wed, 24 Oct 2018 12:19:25 +0200
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> For now, the hotplug handler is not called for devices that are
> >> being cold plugged. The hotplug handler is setup when the machine
> >> initialization is fully done. Only bridges that were cold plugged are
> >> considered.
> >>
> >> Set the hotplug handler for the root piix bus directly when realizing.
> >> Overwrite the hotplug handler of bridges when hotplugging/coldplugging
> >> them.
> >>
> >> This will now make sure that the ACPI PCI hotplug handler is also called
> >> for cold-plugged devices (also on bridges) and for bridges that were
> >> hotplugged.
> >>
> >> When trying to hotplug a device to a hotplugged bridge, we now correctly
> >> get the error message
> >>  "Unsupported bus. Bus doesn't have property 'acpi-pcihp-bsel' set"
> >> Insted of going via the standard PCI hotplug handler.  
> > Erroring out is probably not ok, since it can break existing setups
> > where SHPC hotplugging to hotplugged bridge was working just fine before.  
> 
> The question is if it actually was supposed (and eventually did) work.
I think it works now, it's QEMU 'ACPI hotplug hack' (which exists for
the sake of Windows) limitation. We weren't able to dynamically add
ACPI description for hotplugged bridge, so it was using native hotplug.
Now theoretically we can load tables dynamically but that, would add
maintenance nightmare (versioned tables) and would be harder to debug.
I'd rather not go that direction and keep current limited version,
suggesting users to use native hotplug if guest is capable.

> If this was the expected behavior (mixing hotplug types), then the
> necessary change to this patch would boil down to checking if the bridge
> it hot or coldplugged.
> 
> > 
> > Marcel/Michael what's your take on this change in behaviour?
> > CCing libvirt in case they are doing this stuff
> >   
> 
> Indeed, it would be nice to know if this was actually supposed to work
> like this (coldplugged bridges using ACPI hotplug and hotplugged bridges
> using SHPC hotplug).
> 
> 

Re: [Qemu-devel] [PATCH v1 2/7] pcihp: overwrite hotplug handler recursively from the start

[David Hildenbrand]

On 02.11.18 14:00, Igor Mammedov wrote:
> On Fri, 2 Nov 2018 12:43:10 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 01.11.18 15:10, Igor Mammedov wrote:
>>> On Wed, 24 Oct 2018 12:19:25 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> For now, the hotplug handler is not called for devices that are
>>>> being cold plugged. The hotplug handler is setup when the machine
>>>> initialization is fully done. Only bridges that were cold plugged are
>>>> considered.
>>>>
>>>> Set the hotplug handler for the root piix bus directly when realizing.
>>>> Overwrite the hotplug handler of bridges when hotplugging/coldplugging
>>>> them.
>>>>
>>>> This will now make sure that the ACPI PCI hotplug handler is also called
>>>> for cold-plugged devices (also on bridges) and for bridges that were
>>>> hotplugged.
>>>>
>>>> When trying to hotplug a device to a hotplugged bridge, we now correctly
>>>> get the error message
>>>>  "Unsupported bus. Bus doesn't have property 'acpi-pcihp-bsel' set"
>>>> Insted of going via the standard PCI hotplug handler.  
>>> Erroring out is probably not ok, since it can break existing setups
>>> where SHPC hotplugging to hotplugged bridge was working just fine before.  
>>
>> The question is if it actually was supposed (and eventually did) work.
> I think it works now, it's QEMU 'ACPI hotplug hack' (which exists for
> the sake of Windows) limitation. We weren't able to dynamically add
> ACPI description for hotplugged bridge, so it was using native hotplug.
> Now theoretically we can load tables dynamically but that, would add
> maintenance nightmare (versioned tables) and would be harder to debug.
> I'd rather not go that direction and keep current limited version,
> suggesting users to use native hotplug if guest is capable.

Alright I'll keep current behavior (checking if the bridge is hotplugged
or coldplugged). Thanks!


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 2/7] pcihp: overwrite hotplug handler recursively from the start

[Michael S. Tsirkin]

On Fri, Nov 02, 2018 at 02:00:32PM +0100, Igor Mammedov wrote:
> On Fri, 2 Nov 2018 12:43:10 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
> > On 01.11.18 15:10, Igor Mammedov wrote:
> > > On Wed, 24 Oct 2018 12:19:25 +0200
> > > David Hildenbrand <david@redhat.com> wrote:
> > >   
> > >> For now, the hotplug handler is not called for devices that are
> > >> being cold plugged. The hotplug handler is setup when the machine
> > >> initialization is fully done. Only bridges that were cold plugged are
> > >> considered.
> > >>
> > >> Set the hotplug handler for the root piix bus directly when realizing.
> > >> Overwrite the hotplug handler of bridges when hotplugging/coldplugging
> > >> them.
> > >>
> > >> This will now make sure that the ACPI PCI hotplug handler is also called
> > >> for cold-plugged devices (also on bridges) and for bridges that were
> > >> hotplugged.
> > >>
> > >> When trying to hotplug a device to a hotplugged bridge, we now correctly
> > >> get the error message
> > >>  "Unsupported bus. Bus doesn't have property 'acpi-pcihp-bsel' set"
> > >> Insted of going via the standard PCI hotplug handler.  
> > > Erroring out is probably not ok, since it can break existing setups
> > > where SHPC hotplugging to hotplugged bridge was working just fine before.  
> > 
> > The question is if it actually was supposed (and eventually did) work.
> I think it works now, it's QEMU 'ACPI hotplug hack' (which exists for
> the sake of Windows) limitation. We weren't able to dynamically add
> ACPI description for hotplugged bridge, so it was using native hotplug.
> Now theoretically we can load tables dynamically but that, would add
> maintenance nightmare (versioned tables) and would be harder to debug.
> I'd rather not go that direction and keep current limited version,
> suggesting users to use native hotplug if guest is capable.

Well a bunch of tables need to be dynamic, and generating them from ACPI
isn't a significant step up from generating them in the BIOS which did
create huge headaches, for many reasons but in particular because we
need to add custom interfaces for every little thing we are adding.
By comparison dynamic loading is a single interface and we can
ship any AML code we want across it.

So I'm working on a limited form of dynamic loading with versioning and
I don't necessarily agree it has to be a nightmare, but yes it does need
to be limited very carefully. Implementing bridge hotplug there
isn't in scope for me at this point.

> > If this was the expected behavior (mixing hotplug types), then the
> > necessary change to this patch would boil down to checking if the bridge
> > it hot or coldplugged.
> > 
> > > 
> > > Marcel/Michael what's your take on this change in behaviour?
> > > CCing libvirt in case they are doing this stuff
> > >   
> > 
> > Indeed, it would be nice to know if this was actually supposed to work
> > like this (coldplugged bridges using ACPI hotplug and hotplugged bridges
> > using SHPC hotplug).
> > 
> > 

Re: [Qemu-devel] [PATCH v1 5/7] pci/shpc: move hotplug checks to preplug handler

[David Hildenbrand]

On 24.10.18 12:19, David Hildenbrand wrote:
> Move the checks to the pre_plug handler. I don't see a reason to check
> for the PCI slot when unplugging.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  hw/pci-bridge/pci_bridge_dev.c  | 11 ++++++++--
>  hw/pci-bridge/pcie_pci_bridge.c | 11 ++++++++--
>  hw/pci/shpc.c                   | 36 ++++++++++++++-------------------
>  include/hw/pci/shpc.h           |  2 ++
>  4 files changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 97a8e8b6a4..2362dcbc64 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -206,8 +206,8 @@ static const VMStateDescription pci_bridge_dev_vmstate = {
>      }
>  };
>  
> -static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
> -                                      DeviceState *dev, Error **errp)
> +static void pci_bridge_dev_pre_plug_cb(HotplugHandler *hotplug_dev,
> +                                       DeviceState *dev, Error **errp)
>  {
>      PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
>  
> @@ -216,6 +216,12 @@ static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
>                     "this %s", TYPE_PCI_BRIDGE_DEV);
>          return;
>      }
> +    shpc_device_pre_plug_cb(hotplug_dev, dev, errp);
> +}
> +
> +static void pci_bridge_dev_hotplug_cb(HotplugHandler *hotplug_dev,
> +                                      DeviceState *dev, Error **errp)
> +{
>      shpc_device_hotplug_cb(hotplug_dev, dev, errp);
>  }
>  
> @@ -251,6 +257,7 @@ 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->pre_plug = pci_bridge_dev_pre_plug_cb;
>      hc->plug = pci_bridge_dev_hotplug_cb;
>      hc->unplug_request = pci_bridge_dev_hot_unplug_request_cb;
>  }
> diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
> index 04cf5a6a92..cde8af0a4e 100644
> --- a/hw/pci-bridge/pcie_pci_bridge.c
> +++ b/hw/pci-bridge/pcie_pci_bridge.c
> @@ -137,8 +137,8 @@ static const VMStateDescription pcie_pci_bridge_dev_vmstate = {
>          }
>  };
>  
> -static void pcie_pci_bridge_hotplug_cb(HotplugHandler *hotplug_dev,
> -                                      DeviceState *dev, Error **errp)
> +static void pcie_pci_bridge_pre_plug_cb(HotplugHandler *hotplug_dev,
> +                                        DeviceState *dev, Error **errp)
>  {
>      PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
>  
> @@ -147,6 +147,12 @@ static void pcie_pci_bridge_hotplug_cb(HotplugHandler *hotplug_dev,
>                     "this %s", TYPE_PCIE_PCI_BRIDGE_DEV);
>          return;
>      }
> +    shpc_device_pre_plug_cb(hotplug_dev, dev, errp);
> +}
> +
> +static void pcie_pci_bridge_hotplug_cb(HotplugHandler *hotplug_dev,
> +                                      DeviceState *dev, Error **errp)
> +{
>      shpc_device_hotplug_cb(hotplug_dev, dev, errp);
>  }
>  
> @@ -180,6 +186,7 @@ static void pcie_pci_bridge_class_init(ObjectClass *klass, void *data)
>      dc->props = pcie_pci_bridge_dev_properties;
>      dc->reset = &pcie_pci_bridge_reset;
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> +    hc->pre_plug = pcie_pci_bridge_pre_plug_cb;
>      hc->plug = pcie_pci_bridge_hotplug_cb;
>      hc->unplug_request = pcie_pci_bridge_hot_unplug_request_cb;
>  }
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index a8462d48bb..1caf4a7ee9 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -482,13 +482,21 @@ static const MemoryRegionOps shpc_mmio_ops = {
>          .max_access_size = 4,
>      },
>  };
> -static void shpc_device_hotplug_common(PCIDevice *affected_dev, int *slot,
> -                                       SHPCDevice *shpc, Error **errp)
> +
> +static inline int shpc_device_get_slot(PCIDevice *dev)
> +{
> +    return SHPC_PCI_TO_IDX(dev->devfn);
> +}
> +
> +void shpc_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                             Error **errp)
>  {
> -    int pci_slot = PCI_SLOT(affected_dev->devfn);
> -    *slot = SHPC_PCI_TO_IDX(pci_slot);
> +    SHPCDevice *shpc = PCI_DEVICE(hotplug_dev)->shpc;
> +    PCIDevice *pdev = PCI_DEVICE(dev);
> +    int pci_slot = PCI_SLOT(pdev->devfn);
> +    int slot = shpc_device_get_slot(pdev);
>  
> -    if (pci_slot < SHPC_IDX_TO_PCI(0) || *slot >= shpc->nslots) {
> +    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),


Looking at the details, pdev->devfn might not be properly assigned yet
in pre_plug, as it will (currently) be done during realize(). So moving
this to pre_plug can only be done after the PCI realization has been
refactored.


-- 

Thanks,

David / dhildenb