diff mbox series

[PULL,38/73] pci: fix 'hotplugglable' property behavior

Message ID ceefa0b74674f32aeedb1e93bcb6ec9cb12842b1.1678237635.git.mst@redhat.com
State New
Headers show
Series [PULL,01/73] cryptodev: Introduce cryptodev.json | expand

Commit Message

Michael S. Tsirkin March 8, 2023, 1:12 a.m. UTC 
From: Igor Mammedov <imammedo@redhat.com>

Currently the property may flip its state
during VM bring up or just doesn't work as
the name implies.

In particular with PCIE root port that has
'hotplug={on|off}' property, and when it's
turned off, one would expect
  'hotpluggable' == false
for any devices attached to it.
Which is not the case since qbus_is_hotpluggable()
used by the property just checks for presence
of any hotplug_handler set on bus.

The problem is that name BusState::hotplug_handler
from its inception is misnomer, as it handles
not only hotplug but also in many cases coldplug
as well (i.e. generic wiring interface), and
it's fine to have hotplug_handler set on bus
while it doesn't support hotplug (ex. pcie-slot
with hotplug=off).

Another case of root port flipping 'hotpluggable'
state when ACPI PCI hotplug is enabled in this
case root port with 'hotplug=off' starts as
hotpluggable and then later on, pcihp
hotplug_handler clears hotplug_handler
explicitly after checking root port's 'hotplug'
property.

So root-port hotpluggablity check sort of works
if pcihp is enabled but is broken if pcihp is
disabled.

One way to deal with the issue is to ask
hotplug_handler if bus it controls is hotpluggable
or not. To do that add is_hotpluggable_bus()
hook to HotplugHandler interface and use it in
'hotpluggable' property + teach pcie-slot to
actually look into 'hotplug' property state
before deciding if bus is hotpluggable.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Message-Id: <20230302161543.286002-13-imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/hw/hotplug.h   |  2 ++
 include/hw/qdev-core.h | 13 ++++++++++++-
 hw/pci/pcie_port.c     |  8 ++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)
diff mbox series

Patch

diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
index e15f59c8b3..a9840ed485 100644
--- a/include/hw/hotplug.h
+++ b/include/hw/hotplug.h
@@ -48,6 +48,7 @@  typedef void (*hotplug_fn)(HotplugHandler *plug_handler,
  * @unplug: unplug callback.
  *          Used for device removal with devices that implement
  *          asynchronous and synchronous (surprise) removal.
+ * @is_hotpluggable_bus: called to check if bus/its parent allow hotplug on bus
  */
 struct HotplugHandlerClass {
     /* <private> */
@@ -58,6 +59,7 @@  struct HotplugHandlerClass {
     hotplug_fn plug;
     hotplug_fn unplug_request;
     hotplug_fn unplug;
+    bool (*is_hotpluggable_bus)(HotplugHandler *plug_handler, BusState *bus);
 };
 
 /**
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index f5b3b2f89a..bd50ad5ee1 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -812,7 +812,18 @@  void qbus_set_bus_hotplug_handler(BusState *bus);
 
 static inline bool qbus_is_hotpluggable(BusState *bus)
 {
-   return bus->hotplug_handler;
+    HotplugHandler *plug_handler = bus->hotplug_handler;
+    bool ret = !!plug_handler;
+
+    if (plug_handler) {
+        HotplugHandlerClass *hdc;
+
+        hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
+        if (hdc->is_hotpluggable_bus) {
+            ret = hdc->is_hotpluggable_bus(plug_handler, bus);
+        }
+    }
+    return ret;
 }
 
 /**
diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
index 65a397ad23..000633fec1 100644
--- a/hw/pci/pcie_port.c
+++ b/hw/pci/pcie_port.c
@@ -161,6 +161,13 @@  PCIDevice *pcie_find_port_by_pn(PCIBus *bus, uint8_t pn)
     return NULL;
 }
 
+static bool pcie_slot_is_hotpluggbale_bus(HotplugHandler *plug_handler,
+                                          BusState *bus)
+{
+    PCIESlot *s = PCIE_SLOT(bus->parent);
+    return s->hotplug;
+}
+
 static const TypeInfo pcie_port_type_info = {
     .name = TYPE_PCIE_PORT,
     .parent = TYPE_PCI_BRIDGE,
@@ -188,6 +195,7 @@  static void pcie_slot_class_init(ObjectClass *oc, void *data)
     hc->plug = pcie_cap_slot_plug_cb;
     hc->unplug = pcie_cap_slot_unplug_cb;
     hc->unplug_request = pcie_cap_slot_unplug_request_cb;
+    hc->is_hotpluggable_bus = pcie_slot_is_hotpluggbale_bus;
 }
 
 static const TypeInfo pcie_slot_type_info = {