[PATCH v8 0/4] pci hotplug tracking

[PATCH v8 0/4] pci hotplug tracking

[Vladimir Sementsov-Ogievskiy]

Hi all!

Main thing this series does is DEVICE_ON event - a counter-part to
DEVICE_DELETED. A guest-driven event that device is powered-on.
Details are in patch 2. The new event is paried with corresponding
command query-hotplug.


v8:
 - improve naming, wording and style
 - make new QMP interface experimental


Vladimir Sementsov-Ogievskiy (4):
  qapi/qdev.json: unite DEVICE_* event data into single structure
  qapi: add DEVICE_ON and query-hotplug infrastructure
  shpc: implement DEVICE_ON event and query-hotplug
  pcie: implement DEVICE_ON event and query-hotplug

 hw/core/hotplug.c               |  12 +++
 hw/pci-bridge/pci_bridge_dev.c  |  14 +++
 hw/pci-bridge/pcie_pci_bridge.c |   1 +
 hw/pci/pcie.c                   |  83 +++++++++++++++
 hw/pci/pcie_port.c              |   1 +
 hw/pci/shpc.c                   |  86 +++++++++++++++
 include/hw/hotplug.h            |  11 ++
 include/hw/pci/pci_bridge.h     |   2 +
 include/hw/pci/pcie.h           |   2 +
 include/hw/pci/shpc.h           |   2 +
 include/hw/qdev-core.h          |   7 ++
 include/monitor/qdev.h          |   6 ++
 qapi/qdev.json                  | 178 +++++++++++++++++++++++++++++---
 softmmu/qdev-monitor.c          |  58 +++++++++++
 14 files changed, 451 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH v8 1/4] qapi/qdev.json: unite DEVICE_* event data into single structure

[Vladimir Sementsov-Ogievskiy]

DEVICE_DELETED and DEVICE_UNPLUG_GUEST_ERROR has equal data, let's
refactor it to one structure. That also helps to add new events
consistently.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 qapi/qdev.json | 43 +++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 6bc5a733b8..2899fac837 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -116,6 +116,35 @@
 ##
 { 'command': 'device_del', 'data': {'id': 'str'} }
 
+##
+# @DeviceAndPath:
+#
+# In events we designate devices by both their ID (if the device has
+# one) and QOM path.
+#
+# @device: the device's ID if it has one
+#
+# @path: the device's QOM path
+#
+# Since: 8.2
+##
+
+# Rationale
+#
+# Why we need ID?  User specify ID in device_add command and in
+# command line and expects same identifier in the event data.
+#
+# Why we need QOM path?  Some devices don't have ID and we still want
+# to emit events for them.
+#
+# So, we have a bit of redundancy, as QOM path for device that has
+# ID is always /machine/peripheral/ID.  But that's hard to change
+# keeping both simple interface for most users and universality for
+# the generic case.
+
+{ 'struct': 'DeviceAndPath',
+  'data': { '*device': 'str', 'path': 'str' } }
+
 ##
 # @DEVICE_DELETED:
 #
@@ -124,10 +153,6 @@
 # ID. Device removal can be initiated by the guest or by HMP/QMP
 # commands.
 #
-# @device: the device's ID if it has one
-#
-# @path: the device's QOM path
-#
 # Since: 1.5
 #
 # Example:
@@ -137,8 +162,7 @@
 #                "path": "/machine/peripheral/virtio-net-pci-0" },
 #      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 ##
-{ 'event': 'DEVICE_DELETED',
-  'data': { '*device': 'str', 'path': 'str' } }
+{ 'event': 'DEVICE_DELETED', 'data': 'DeviceAndPath' }
 
 ##
 # @DEVICE_UNPLUG_GUEST_ERROR:
@@ -146,10 +170,6 @@
 # Emitted when a device hot unplug fails due to a guest reported
 # error.
 #
-# @device: the device's ID if it has one
-#
-# @path: the device's QOM path
-#
 # Since: 6.2
 #
 # Example:
@@ -159,5 +179,4 @@
 #                "path": "/machine/peripheral/core1" },
 #      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
 ##
-{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
-  'data': { '*device': 'str', 'path': 'str' } }
+{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
-- 
2.34.1

[PATCH v8 2/4] qapi: add DEVICE_ON and query-hotplug infrastructure

[Vladimir Sementsov-Ogievskiy]

We have DEVICE_DELETED event, that signals that device_del command is
actually completed. But we don't have a counter-part for device_add.
Still it's sensible for SHPC and PCIe-native hotplug, as there are time
when the device in some intermediate state. Let's add an event that say
that the device is finally powered on, power indicator is on and
everything is OK for next manipulation on that device.

Motivations:
1. To be sure that device is "accepted" by guest. Guest may ignore
hotplugged device for some reason (for example during OS booting).
Management wants to catch this and handle the problem, instead of
silent assume that everything is OK. So, if we don't get the event by
some timeout, we can report an error, try to unplug/plug the disk again
or do some other things to handle the problem.

2. The device can't be removed (by blockdev-del) while power indicator
of hotplug controller is blinking (QEMU reports "guest is busy (power
indicator blinking)"). So, management should avoid removing the device
until it gets the DEVICE_ON event.
(Probably, better solution for this point is to automatically postpone
deletion until power indicator stops blinking)

3. Also, management tool may make a GUI visualization of power
indicator with help of this event.

New query-hotplug command in additon to "device-on" state also provides
SHPC/PCIe-native specific hotplug controller properties (like leds)
that may help to determine real state of hotplug controller. That may
help to get additional information for further debugging when DEVICE_ON
/ DEVICE_DELETED not come in time as expected.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/core/hotplug.c      |  12 ++++
 include/hw/hotplug.h   |  11 ++++
 include/hw/qdev-core.h |   7 +++
 include/monitor/qdev.h |   2 +
 qapi/qdev.json         | 135 +++++++++++++++++++++++++++++++++++++++++
 softmmu/qdev-monitor.c |  41 +++++++++++++
 6 files changed, 208 insertions(+)

diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index 17ac986685..9651fe7c85 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -57,6 +57,18 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
     }
 }
 
+HotplugInfo *hotplug_handler_get_state(HotplugHandler *plug_handler,
+                                       DeviceState *plugged_dev, Error **errp)
+{
+    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
+
+    if (hdc->get_hotplug_state) {
+        return hdc->get_hotplug_state(plug_handler, plugged_dev, errp);
+    }
+
+    return NULL;
+}
+
 static const TypeInfo hotplug_handler_info = {
     .name          = TYPE_HOTPLUG_HANDLER,
     .parent        = TYPE_INTERFACE,
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
index a9840ed485..23c57a0967 100644
--- a/include/hw/hotplug.h
+++ b/include/hw/hotplug.h
@@ -13,6 +13,7 @@
 #define HOTPLUG_H
 
 #include "qom/object.h"
+#include "qapi/qapi-types-qdev.h"
 
 #define TYPE_HOTPLUG_HANDLER "hotplug-handler"
 
@@ -60,6 +61,8 @@ struct HotplugHandlerClass {
     hotplug_fn unplug_request;
     hotplug_fn unplug;
     bool (*is_hotpluggable_bus)(HotplugHandler *plug_handler, BusState *bus);
+    HotplugInfo *(*get_hotplug_state)(HotplugHandler *plug_handler,
+                                      DeviceState *plugged_dev, Error **errp);
 };
 
 /**
@@ -96,4 +99,12 @@ void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
 void hotplug_handler_unplug(HotplugHandler *plug_handler,
                             DeviceState *plugged_dev,
                             Error **errp);
+
+/**
+ * hotplug_handler_get_state:
+ *
+ * Calls #HotplugHandlerClass.get_hotplug_state callback of @plug_handler.
+ */
+HotplugInfo *hotplug_handler_get_state(HotplugHandler *plug_handler,
+                                       DeviceState *plugged_dev, Error **errp);
 #endif
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 151d968238..a27ef2eb24 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -229,6 +229,13 @@ struct DeviceState {
      * @realized: has device been realized?
      */
     bool realized;
+
+    /**
+     * @device_on_event_sent: When true means that DEVICE_ON event was already
+     *                        sent.
+     */
+    bool device_on_event_sent;
+
     /**
      * @pending_deleted_event: track pending deletion events during unplug
      */
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index 1d57bf6577..c1c8798e89 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -36,4 +36,6 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
  */
 const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
 
+void qdev_hotplug_device_on_event(DeviceState *dev);
+
 #endif
diff --git a/qapi/qdev.json b/qapi/qdev.json
index 2899fac837..fa80694735 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -180,3 +180,138 @@
 #      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
 ##
 { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
+
+##
+# @LedStatus:
+#
+# @on: LED is on.
+#
+# @blinking: LED is blinking.
+#
+# @off: LED is off.
+#
+# Since: 8.2
+##
+{ 'enum': 'LedStatus',
+  'data': [ 'on', 'blinking', 'off' ] }
+
+##
+# @HotplugSHPCSlotState:
+#
+# Standard Hot-Plug Controller slot state.
+#
+# @power-only: Slot is powered on, but neither clock nor bus are
+#     connected.
+#
+# @enabled: Slot is powered on, clock and bus are connected, and the
+#     card is fully functional from a hardware standpoint.
+#
+# @disabled: Slot is disabled, card is safe to be removed.
+#
+# Since: 8.2
+##
+{ 'enum': 'HotplugSHPCSlotState',
+  'data': [ 'power-only', 'enabled', 'disabled' ] }
+
+##
+# @HotplugSHPCState:
+#
+# Standard Hot Plug Controller state.
+#
+# @slot-state: The slot state field of SHPC Slot Status structure.
+#
+# Since: 8.2
+##
+{ 'struct': 'HotplugSHPCState',
+  'data': { '*slot-state': 'HotplugSHPCSlotState' } }
+
+##
+# @HotplugPCIeNativeState:
+#
+# PCIe Native hotplug slot state.
+#
+# @power-on: PCIe Power Controller Control of Slot Control Register.
+#     True means Power On (Power Controller Control bit is 0), False
+#     means Power Off (Power Controller Control bit is 1).
+#
+# Since: 8.2
+##
+{ 'struct': 'HotplugPCIeNativeState',
+  'data': { '*power-on': 'bool' } }
+
+##
+# @HotplugType:
+#
+# Type of hotplug controller / provider.
+#
+# @shpc: PCI Standard Hot-plug Controller
+#
+# @pcie-native: PCIe Native hotplug
+#
+# TODO: add @acpi type
+#
+# Since: 8.2
+##
+{ 'enum': 'HotplugType',
+  'data': ['shpc', 'pcie-native'] }
+
+##
+# @HotplugInfo:
+#
+# Hotplug slot state.
+#
+# @type: type of the hotplug
+#
+# @bus: The QOM path of the parent bus where device is plugged into.
+#
+# @addr: The bus address for hotplugged device if applicable.
+#
+# @child: the hotplugged device
+#
+# @device-on: Device is powered-on by guest.  This state changes at
+#     most once for the device and corresponds to DEVICE_ON event.
+#
+# @power-led: Power indicator.  When power LED is on the device is
+#     ready and accepted by guest.  When it is off, device is safe to
+#     remove.  It is blinking while hot plug or unplug is in progress.
+#
+# @attention-led: The attention LED is normally off.  It is on to
+#     signal a problem.  Blinking is for helping users to locate the
+#     slot.
+#
+# Single: 8.2
+##
+{ 'union': 'HotplugInfo',
+  'base': { 'type': 'HotplugType',
+            'bus': 'DeviceAndPath',
+            '*addr': 'str',
+            'child': 'DeviceAndPath',
+            'device-on': 'bool',
+            '*power-led': 'LedStatus',
+            '*attention-led': 'LedStatus'},
+  'discriminator': 'type',
+  'data': { 'shpc': 'HotplugSHPCState',
+            'pcie-native': 'HotplugPCIeNativeState' } }
+
+##
+# @x-query-hotplug:
+#
+# Query the state of hotplug controller.
+#
+# @id: the device's ID or QOM path
+#
+# Since: 8.2
+##
+{ 'command': 'x-query-hotplug',
+  'data': { 'id': 'str' },
+  'returns': 'HotplugInfo' }
+
+##
+# @X_DEVICE_ON:
+#
+# Emitted whenever the device insertion completion is acknowledged by
+# the guest.  For now only emitted for SHPC and PCIe-native hotplug.
+#
+# Since: 8.2
+##
+{ 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' }
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 74f4e41338..483da40cb9 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -25,6 +25,7 @@
 #include "sysemu/arch_init.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-qdev.h"
+#include "qapi/qapi-events-qdev.h"
 #include "qapi/qmp/dispatch.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
@@ -956,6 +957,36 @@ void qmp_device_del(const char *id, Error **errp)
     }
 }
 
+HotplugInfo *qmp_x_query_hotplug(const char *id, Error **errp)
+{
+    DeviceState *dev = find_device_state(id, errp);
+    HotplugHandler *hotplug_ctrl;
+
+    if (!dev) {
+        return NULL;
+    }
+
+    if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
+        error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
+        return NULL;
+    }
+
+    if (!DEVICE_GET_CLASS(dev)->hotpluggable) {
+        error_setg(errp, QERR_DEVICE_NO_HOTPLUG,
+                   object_get_typename(OBJECT(dev)));
+        return NULL;
+    }
+
+    hotplug_ctrl = qdev_get_hotplug_handler(dev);
+    /*
+     * hotpluggable device MUST have HotplugHandler, if it doesn't
+     * then something is very wrong with it.
+     */
+    g_assert(hotplug_ctrl);
+
+    return hotplug_handler_get_state(hotplug_ctrl, dev, errp);
+}
+
 void hmp_device_add(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
@@ -1146,3 +1177,13 @@ bool qmp_command_available(const QmpCommand *cmd, Error **errp)
     }
     return true;
 }
+
+void qdev_hotplug_device_on_event(DeviceState *dev)
+{
+    if (dev->device_on_event_sent) {
+        return;
+    }
+
+    dev->device_on_event_sent = true;
+    qapi_event_send_x_device_on(dev->id, dev->canonical_path);
+}
-- 
2.34.1

[PATCH v8 3/4] shpc: implement DEVICE_ON event and query-hotplug

[Vladimir Sementsov-Ogievskiy]

For PCIe and SHPC hotplug it's important to track led indicators and
"device-on" status.

At this step, implement the prepared infrastructure in SHPC.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/pci-bridge/pci_bridge_dev.c  | 14 ++++++
 hw/pci-bridge/pcie_pci_bridge.c |  1 +
 hw/pci/shpc.c                   | 86 +++++++++++++++++++++++++++++++++
 include/hw/pci/pci_bridge.h     |  2 +
 include/hw/pci/shpc.h           |  2 +
 include/monitor/qdev.h          |  4 ++
 softmmu/qdev-monitor.c          | 17 +++++++
 7 files changed, 126 insertions(+)

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 4b2696ea7f..69ffe93e2a 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -241,6 +241,19 @@ void pci_bridge_dev_unplug_request_cb(HotplugHandler *hotplug_dev,
     shpc_device_unplug_request_cb(hotplug_dev, dev, errp);
 }
 
+HotplugInfo *pci_bridge_dev_get_hotplug_state(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", object_get_typename(OBJECT(hotplug_dev)));
+        return NULL;
+    }
+    return shpc_get_hotplug_state(hotplug_dev, dev, errp);
+}
+
 static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -261,6 +274,7 @@ static void pci_bridge_dev_class_init(ObjectClass *klass, void *data)
     hc->plug = pci_bridge_dev_plug_cb;
     hc->unplug = pci_bridge_dev_unplug_cb;
     hc->unplug_request = pci_bridge_dev_unplug_request_cb;
+    hc->get_hotplug_state = pci_bridge_dev_get_hotplug_state;
 }
 
 static const TypeInfo pci_bridge_dev_info = {
diff --git a/hw/pci-bridge/pcie_pci_bridge.c b/hw/pci-bridge/pcie_pci_bridge.c
index 2301b2ca0b..959b536303 100644
--- a/hw/pci-bridge/pcie_pci_bridge.c
+++ b/hw/pci-bridge/pcie_pci_bridge.c
@@ -157,6 +157,7 @@ static void pcie_pci_bridge_class_init(ObjectClass *klass, void *data)
     hc->plug = pci_bridge_dev_plug_cb;
     hc->unplug = pci_bridge_dev_unplug_cb;
     hc->unplug_request = pci_bridge_dev_unplug_request_cb;
+    hc->get_hotplug_state = pci_bridge_dev_get_hotplug_state;
 }
 
 static const TypeInfo pcie_pci_bridge_info = {
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index df7f370111..0cd44150e5 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -8,6 +8,9 @@
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_bus.h"
 #include "hw/pci/msi.h"
+#include "qapi/qapi-types-qdev.h"
+#include "qapi/qapi-events-qdev.h"
+#include "monitor/qdev.h"
 
 /* TODO: model power only and disabled slot states. */
 /* TODO: handle SERR and wakeups */
@@ -123,6 +126,39 @@
 #define SHPC_PCI_TO_IDX(pci_slot) ((pci_slot) - 1)
 #define SHPC_IDX_TO_PHYSICAL(slot) ((slot) + 1)
 
+static char *shpc_idx_to_pci_addr(int slot)
+{
+    return g_strdup_printf("%d", SHPC_IDX_TO_PCI(slot));
+}
+
+static LedStatus shpc_led_state_to_qapi(uint8_t value)
+{
+    switch (value) {
+    case SHPC_LED_ON:
+        return LED_STATUS_ON;
+    case SHPC_LED_BLINK:
+        return LED_STATUS_BLINKING;
+    case SHPC_LED_OFF:
+        return LED_STATUS_OFF;
+    default:
+        abort();
+    }
+}
+
+static HotplugSHPCSlotState shpc_slot_state_to_qapi(uint8_t value)
+{
+    switch (value) {
+    case SHPC_STATE_PWRONLY:
+        return HOTPLUGSHPC_SLOT_STATE_POWER_ONLY;
+    case SHPC_STATE_ENABLED:
+        return HOTPLUGSHPC_SLOT_STATE_ENABLED;
+    case SHPC_STATE_DISABLED:
+        return HOTPLUGSHPC_SLOT_STATE_DISABLED;
+    default:
+        abort();
+    }
+}
+
 static uint8_t shpc_get_status(SHPCDevice *shpc, int slot, uint16_t msk)
 {
     uint8_t *status = shpc->config + SHPC_SLOT_STATUS(slot);
@@ -263,14 +299,23 @@ static bool shpc_slot_is_off(uint8_t state, uint8_t power, uint8_t attn)
     return state == SHPC_STATE_DISABLED && power == SHPC_LED_OFF;
 }
 
+static bool shpc_slot_is_on(uint8_t state, uint8_t power, uint8_t attn)
+{
+    return state == SHPC_STATE_ENABLED && power == SHPC_LED_ON &&
+        attn == SHPC_LED_OFF;
+}
+
 static void shpc_slot_command(PCIDevice *d, uint8_t target,
                               uint8_t state, uint8_t power, uint8_t attn)
 {
     SHPCDevice *shpc = d->shpc;
     int slot = SHPC_LOGICAL_TO_IDX(target);
+    int pci_slot = SHPC_IDX_TO_PCI(slot);
     uint8_t old_state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK);
     uint8_t old_power = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK);
     uint8_t old_attn = shpc_get_status(shpc, slot, SHPC_SLOT_ATTN_LED_MASK);
+    DeviceState *child_dev =
+        DEVICE(shpc->sec_bus->devices[PCI_DEVFN(pci_slot, 0)]);
 
     if (target < SHPC_CMD_TRGT_MIN || slot >= shpc->nslots) {
         shpc_invalid_command(shpc);
@@ -313,6 +358,12 @@ static void shpc_slot_command(PCIDevice *d, uint8_t target,
             SHPC_SLOT_EVENT_MRL |
             SHPC_SLOT_EVENT_PRESENCE;
     }
+
+    if (!shpc_slot_is_on(old_state, old_power, old_attn) &&
+        shpc_slot_is_on(state, power, attn) && child_dev)
+    {
+        qdev_hotplug_device_on_event(child_dev);
+    }
 }
 
 static void shpc_command(PCIDevice *d)
@@ -713,6 +764,41 @@ void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
     shpc_cap_update_dword(d);
 }
 
+HotplugInfo *shpc_get_hotplug_state(HotplugHandler *hotplug_dev,
+                                    DeviceState *dev, Error **errp)
+{
+    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
+    SHPCDevice *shpc = pci_hotplug_dev->shpc;
+    int slot;
+    uint8_t state, power, attn;
+    HotplugInfo *res;
+
+    if (!shpc_device_get_slot(PCI_DEVICE(dev), &slot, shpc, errp)) {
+        return NULL;
+    }
+
+    state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK);
+    power = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK);
+    attn = shpc_get_status(shpc, slot, SHPC_SLOT_ATTN_LED_MASK);
+
+    res = g_new(HotplugInfo, 1);
+    *res = (HotplugInfo) {
+        .bus = qdev_new_device_and_path(DEVICE(pci_hotplug_dev)),
+        .addr = shpc_idx_to_pci_addr(slot),
+        .child = qdev_new_device_and_path(dev),
+        .type = HOTPLUG_TYPE_SHPC,
+        .device_on = dev->device_on_event_sent,
+        .has_power_led = true,
+        .power_led = shpc_led_state_to_qapi(power),
+        .has_attention_led = true,
+        .attention_led = shpc_led_state_to_qapi(attn),
+        .u.shpc.has_slot_state = true,
+        .u.shpc.slot_state = shpc_slot_state_to_qapi(state),
+    };
+
+    return res;
+}
+
 static int shpc_save(QEMUFile *f, void *pv, size_t size,
                      const VMStateField *field, JSONWriter *vmdesc)
 {
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index ea54a81a15..2756c64953 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -139,6 +139,8 @@ void pci_bridge_dev_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp);
 void pci_bridge_dev_unplug_request_cb(HotplugHandler *hotplug_dev,
                                       DeviceState *dev, Error **errp);
+HotplugInfo *pci_bridge_dev_get_hotplug_state(HotplugHandler *hotplug_dev,
+                                              DeviceState *dev, Error **errp);
 
 /*
  * before qdev initialization(qdev_init()), this function sets bus_name and
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 89c7a3b7fa..bf722ce65d 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -51,6 +51,8 @@ void shpc_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                            Error **errp);
 void shpc_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                                    DeviceState *dev, Error **errp);
+HotplugInfo *shpc_get_hotplug_state(HotplugHandler *hotplug_dev,
+                                    DeviceState *dev, Error **errp);
 
 extern VMStateInfo shpc_vmstate_info;
 #define SHPC_VMSTATE(_field, _type,  _test) \
diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
index c1c8798e89..949a3672cb 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -1,6 +1,8 @@
 #ifndef MONITOR_QDEV_H
 #define MONITOR_QDEV_H
 
+#include "qapi/qapi-types-qdev.h"
+
 /*** monitor commands ***/
 
 void hmp_info_qtree(Monitor *mon, const QDict *qdict);
@@ -38,4 +40,6 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
 
 void qdev_hotplug_device_on_event(DeviceState *dev);
 
+DeviceAndPath *qdev_new_device_and_path(DeviceState *dev);
+
 #endif
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 483da40cb9..19c31446d8 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -1178,6 +1178,23 @@ bool qmp_command_available(const QmpCommand *cmd, Error **errp)
     return true;
 }
 
+DeviceAndPath *qdev_new_device_and_path(DeviceState *dev)
+{
+    DeviceAndPath *res;
+
+    if (!dev) {
+        return NULL;
+    }
+
+    res = g_new(DeviceAndPath, 1);
+    *res = (DeviceAndPath) {
+        .device = g_strdup(dev->id),
+        .path = g_strdup(dev->canonical_path),
+    };
+
+    return res;
+}
+
 void qdev_hotplug_device_on_event(DeviceState *dev)
 {
     if (dev->device_on_event_sent) {
-- 
2.34.1

[PATCH v8 4/4] pcie: implement DEVICE_ON event and query-hotplug

[Vladimir Sementsov-Ogievskiy]

For PCIe and SHPC hotplug it's important to track led indicators and
"device-on" status.

At this step implement the prepared infrastructure in PCIe.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/pci/pcie.c         | 83 +++++++++++++++++++++++++++++++++++++++++++
 hw/pci/pcie_port.c    |  1 +
 include/hw/pci/pcie.h |  2 ++
 3 files changed, 86 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 6db0cf69cd..8b5c163c25 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -19,7 +19,10 @@
  */
 
 #include "qemu/osdep.h"
+
+#include "monitor/qdev.h"
 #include "qapi/error.h"
+#include "qapi/qapi-events-qdev.h"
 #include "hw/pci/pci_bridge.h"
 #include "hw/pci/pcie.h"
 #include "hw/pci/msix.h"
@@ -45,6 +48,30 @@ static bool pcie_sltctl_powered_off(uint16_t sltctl)
         && (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF;
 }
 
+static bool pcie_sltctl_powered_on(uint16_t sltctl)
+{
+    return (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_ON &&
+        (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_ON &&
+        (sltctl & PCI_EXP_SLTCTL_AIC) == PCI_EXP_SLTCTL_ATTN_IND_OFF;
+}
+
+static LedStatus pcie_led_state_to_qapi(uint16_t value)
+{
+    switch (value) {
+    case PCI_EXP_SLTCTL_PWR_IND_ON:
+    case PCI_EXP_SLTCTL_ATTN_IND_ON:
+        return LED_STATUS_ON;
+    case PCI_EXP_SLTCTL_PWR_IND_BLINK:
+    case PCI_EXP_SLTCTL_ATTN_IND_BLINK:
+        return LED_STATUS_BLINKING;
+    case PCI_EXP_SLTCTL_PWR_IND_OFF:
+    case PCI_EXP_SLTCTL_ATTN_IND_OFF:
+        return LED_STATUS_OFF;
+    default:
+        abort();
+    }
+}
+
 /***************************************************************************
  * pci express capability helper functions
  */
@@ -735,6 +762,28 @@ void pcie_cap_slot_get(PCIDevice *dev, uint16_t *slt_ctl, uint16_t *slt_sta)
     *slt_sta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
 }
 
+static void find_child_fn(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+    PCIDevice **child = opaque;
+
+    if (!*child) {
+        *child = dev;
+    }
+}
+
+/*
+ * Returns the plugged device or first function of multifunction plugged device
+ */
+static PCIDevice *pcie_cap_slot_find_child(PCIDevice *dev)
+{
+    PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
+    PCIDevice *child = NULL;
+
+    pci_for_each_device(sec_bus, pci_bus_num(sec_bus), find_child_fn, &child);
+
+    return child;
+}
+
 void pcie_cap_slot_write_config(PCIDevice *dev,
                                 uint16_t old_slt_ctl, uint16_t old_slt_sta,
                                 uint32_t addr, uint32_t val, int len)
@@ -742,6 +791,7 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
     uint32_t pos = dev->exp.exp_cap;
     uint8_t *exp_cap = dev->config + pos;
     uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
+    DeviceState *child_dev = DEVICE(pcie_cap_slot_find_child(dev));
 
     if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) {
         /*
@@ -779,6 +829,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
                         sltsta);
     }
 
+    if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_on(val) &&
+        !pcie_sltctl_powered_on(old_slt_ctl) && child_dev)
+    {
+        qdev_hotplug_device_on_event(child_dev);
+    }
+
     /*
      * If the slot is populated, power indicator is off and power
      * controller is off, it is safe to detach the devices.
@@ -1113,3 +1169,30 @@ void pcie_acs_reset(PCIDevice *dev)
         pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0);
     }
 }
+
+HotplugInfo *pcie_cap_slot_get_hotplug_state(HotplugHandler *hotplug_dev,
+                                             DeviceState *dev, Error **errp)
+{
+    PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
+    uint8_t *exp_cap = hotplug_pdev->config + hotplug_pdev->exp.exp_cap;
+    uint16_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
+    uint16_t power_led = sltctl & PCI_EXP_SLTCTL_PIC;
+    uint16_t attn_led = sltctl & PCI_EXP_SLTCTL_AIC;
+    uint16_t pcc = sltctl & PCI_EXP_SLTCTL_PCC;
+    HotplugInfo *res = g_new(HotplugInfo, 1);
+
+    *res = (HotplugInfo) {
+        .type = HOTPLUG_TYPE_PCIE_NATIVE,
+        .bus = qdev_new_device_and_path(DEVICE(hotplug_pdev)),
+        .child = qdev_new_device_and_path(dev),
+        .device_on = dev->device_on_event_sent,
+        .has_power_led = true,
+        .power_led = pcie_led_state_to_qapi(power_led),
+        .has_attention_led = true,
+        .attention_led = pcie_led_state_to_qapi(attn_led),
+        .u.pcie_native.has_power_on = true,
+        .u.pcie_native.power_on = pcc == PCI_EXP_SLTCTL_PWR_ON,
+    };
+
+    return res;
+}
diff --git a/hw/pci/pcie_port.c b/hw/pci/pcie_port.c
index 20ff2b39e8..91e53c269c 100644
--- a/hw/pci/pcie_port.c
+++ b/hw/pci/pcie_port.c
@@ -234,6 +234,7 @@ static void pcie_slot_class_init(ObjectClass *oc, void *data)
     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;
+    hc->get_hotplug_state = pcie_cap_slot_get_hotplug_state;
 }
 
 static const TypeInfo pcie_slot_type_info = {
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 11f5a91bbb..01fb9e163c 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -147,4 +147,6 @@ void pcie_cap_slot_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                              Error **errp);
 void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
                                      DeviceState *dev, Error **errp);
+HotplugInfo *pcie_cap_slot_get_hotplug_state(HotplugHandler *hotplug_dev,
+                                             DeviceState *dev, Error **errp);
 #endif /* QEMU_PCIE_H */
-- 
2.34.1

Re: [PATCH v8 0/4] pci hotplug tracking

[Vladimir Sementsov-Ogievskiy]

Ping.

And some addition. We have the case, when the commit

commit 7bed89958bfbf40df9ca681cefbdca63abdde39d
Author: Maxim Levitsky <mlevitsk@redhat.com>
Date:   Tue Oct 6 14:38:58 2020 +0200

     device_core: use drain_call_rcu in in qmp_device_add
     
     Soon, a device removal might only happen on RCU callback execution.
     This is okay for device-del which provides a DEVICE_DELETED event,
     but not for the failure case of device-add.  To avoid changing
     monitor semantics, just drain all pending RCU callbacks on error.

sensibly slows down vm initialization (several calls to device_add of pc-dimm).

And looking at commit message, I see that what I do in the series is exactly a suggestion to change monitor semantics.

What do you think?

Maybe we need a boolean "async" parameter for device_add, which will turn off drain_call_rcu() call and rely on user to handle DEVICE_ON?

On 05.10.23 12:29, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Main thing this series does is DEVICE_ON event - a counter-part to
> DEVICE_DELETED. A guest-driven event that device is powered-on.
> Details are in patch 2. The new event is paried with corresponding
> command query-hotplug.
> 
> 
> v8:
>   - improve naming, wording and style
>   - make new QMP interface experimental
> 
> 
> Vladimir Sementsov-Ogievskiy (4):
>    qapi/qdev.json: unite DEVICE_* event data into single structure
>    qapi: add DEVICE_ON and query-hotplug infrastructure
>    shpc: implement DEVICE_ON event and query-hotplug
>    pcie: implement DEVICE_ON event and query-hotplug
> 
>   hw/core/hotplug.c               |  12 +++
>   hw/pci-bridge/pci_bridge_dev.c  |  14 +++
>   hw/pci-bridge/pcie_pci_bridge.c |   1 +
>   hw/pci/pcie.c                   |  83 +++++++++++++++
>   hw/pci/pcie_port.c              |   1 +
>   hw/pci/shpc.c                   |  86 +++++++++++++++
>   include/hw/hotplug.h            |  11 ++
>   include/hw/pci/pci_bridge.h     |   2 +
>   include/hw/pci/pcie.h           |   2 +
>   include/hw/pci/shpc.h           |   2 +
>   include/hw/qdev-core.h          |   7 ++
>   include/monitor/qdev.h          |   6 ++
>   qapi/qdev.json                  | 178 +++++++++++++++++++++++++++++---
>   softmmu/qdev-monitor.c          |  58 +++++++++++
>   14 files changed, 451 insertions(+), 12 deletions(-)
> 

-- 
Best regards,
Vladimir

Re: [PATCH v8 0/4] pci hotplug tracking

[Vladimir Sementsov-Ogievskiy]

[cc Peter, Nikolay and libvirt list]

On 02.11.23 11:06, Vladimir Sementsov-Ogievskiy wrote:
> Ping.
> 
> And some addition. We have the case, when the commit
> 
> commit 7bed89958bfbf40df9ca681cefbdca63abdde39d
> Author: Maxim Levitsky <mlevitsk@redhat.com>
> Date:   Tue Oct 6 14:38:58 2020 +0200
> 
>      device_core: use drain_call_rcu in in qmp_device_add
>      Soon, a device removal might only happen on RCU callback execution.
>      This is okay for device-del which provides a DEVICE_DELETED event,
>      but not for the failure case of device-add.  To avoid changing
>      monitor semantics, just drain all pending RCU callbacks on error.
> 
> sensibly slows down vm initialization (several calls to device_add of pc-dimm).
> 
> And looking at commit message, I see that what I do in the series is exactly a suggestion to change monitor semantics.
> 
> What do you think?
> 
> Maybe we need a boolean "async" parameter for device_add, which will turn off drain_call_rcu() call and rely on user to handle DEVICE_ON?
> 
> On 05.10.23 12:29, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Main thing this series does is DEVICE_ON event - a counter-part to
>> DEVICE_DELETED. A guest-driven event that device is powered-on.
>> Details are in patch 2. The new event is paried with corresponding
>> command query-hotplug.
>>
>>
>> v8:
>>   - improve naming, wording and style
>>   - make new QMP interface experimental
>>
>>
>> Vladimir Sementsov-Ogievskiy (4):
>>    qapi/qdev.json: unite DEVICE_* event data into single structure
>>    qapi: add DEVICE_ON and query-hotplug infrastructure
>>    shpc: implement DEVICE_ON event and query-hotplug
>>    pcie: implement DEVICE_ON event and query-hotplug
>>
>>   hw/core/hotplug.c               |  12 +++
>>   hw/pci-bridge/pci_bridge_dev.c  |  14 +++
>>   hw/pci-bridge/pcie_pci_bridge.c |   1 +
>>   hw/pci/pcie.c                   |  83 +++++++++++++++
>>   hw/pci/pcie_port.c              |   1 +
>>   hw/pci/shpc.c                   |  86 +++++++++++++++
>>   include/hw/hotplug.h            |  11 ++
>>   include/hw/pci/pci_bridge.h     |   2 +
>>   include/hw/pci/pcie.h           |   2 +
>>   include/hw/pci/shpc.h           |   2 +
>>   include/hw/qdev-core.h          |   7 ++
>>   include/monitor/qdev.h          |   6 ++
>>   qapi/qdev.json                  | 178 +++++++++++++++++++++++++++++---
>>   softmmu/qdev-monitor.c          |  58 +++++++++++
>>   14 files changed, 451 insertions(+), 12 deletions(-)
>>
> 

-- 
Best regards,
Vladimir

Re: [PATCH v8 0/4] pci hotplug tracking

[Michael S. Tsirkin]

On Thu, Oct 05, 2023 at 12:29:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Main thing this series does is DEVICE_ON event - a counter-part to
> DEVICE_DELETED. A guest-driven event that device is powered-on.
> Details are in patch 2. The new event is paried with corresponding
> command query-hotplug.

Several things questionable here:
1. depending on guest activity you can get as many
   DEVICE_ON events as you like
2. it's just for shpc and native pcie - things are
   confusing enough for management, we should make sure
   it can work for all devices
3. what about non hotpluggable devices? do we want the event for them?


I feel this needs actual motivation so we can judge what's the
right way to do it.


> 
> v8:
>  - improve naming, wording and style
>  - make new QMP interface experimental
> 
> 
> Vladimir Sementsov-Ogievskiy (4):
>   qapi/qdev.json: unite DEVICE_* event data into single structure
>   qapi: add DEVICE_ON and query-hotplug infrastructure
>   shpc: implement DEVICE_ON event and query-hotplug
>   pcie: implement DEVICE_ON event and query-hotplug
> 
>  hw/core/hotplug.c               |  12 +++
>  hw/pci-bridge/pci_bridge_dev.c  |  14 +++
>  hw/pci-bridge/pcie_pci_bridge.c |   1 +
>  hw/pci/pcie.c                   |  83 +++++++++++++++
>  hw/pci/pcie_port.c              |   1 +
>  hw/pci/shpc.c                   |  86 +++++++++++++++
>  include/hw/hotplug.h            |  11 ++
>  include/hw/pci/pci_bridge.h     |   2 +
>  include/hw/pci/pcie.h           |   2 +
>  include/hw/pci/shpc.h           |   2 +
>  include/hw/qdev-core.h          |   7 ++
>  include/monitor/qdev.h          |   6 ++
>  qapi/qdev.json                  | 178 +++++++++++++++++++++++++++++---
>  softmmu/qdev-monitor.c          |  58 +++++++++++
>  14 files changed, 451 insertions(+), 12 deletions(-)
> 
> -- 
> 2.34.1

Re: [PATCH v8 2/4] qapi: add DEVICE_ON and query-hotplug infrastructure

[Michael S. Tsirkin]

On Thu, Oct 05, 2023 at 12:29:24PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We have DEVICE_DELETED event, that signals that device_del command is
> actually completed. But we don't have a counter-part for device_add.
> Still it's sensible for SHPC and PCIe-native hotplug, as there are time
> when the device in some intermediate state. Let's add an event that say
> that the device is finally powered on, power indicator is on and
> everything is OK for next manipulation on that device.
> 
> Motivations:
> 1. To be sure that device is "accepted" by guest. Guest may ignore
> hotplugged device for some reason (for example during OS booting).
> Management wants to catch this and handle the problem, instead of
> silent assume that everything is OK. So, if we don't get the event by
> some timeout, we can report an error, try to unplug/plug the disk again
> or do some other things to handle the problem.
> 
> 2. The device can't be removed (by blockdev-del) while power indicator
> of hotplug controller is blinking (QEMU reports "guest is busy (power
> indicator blinking)"). So, management should avoid removing the device
> until it gets the DEVICE_ON event.
> (Probably, better solution for this point is to automatically postpone
> deletion until power indicator stops blinking)
> 
> 3. Also, management tool may make a GUI visualization of power
> indicator with help of this event.
> 
> New query-hotplug command in additon to "device-on" state also provides
> SHPC/PCIe-native specific hotplug controller properties (like leds)
> that may help to determine real state of hotplug controller. That may
> help to get additional information for further debugging when DEVICE_ON
> / DEVICE_DELETED not come in time as expected.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/core/hotplug.c      |  12 ++++
>  include/hw/hotplug.h   |  11 ++++
>  include/hw/qdev-core.h |   7 +++
>  include/monitor/qdev.h |   2 +
>  qapi/qdev.json         | 135 +++++++++++++++++++++++++++++++++++++++++
>  softmmu/qdev-monitor.c |  41 +++++++++++++
>  6 files changed, 208 insertions(+)
> 
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 17ac986685..9651fe7c85 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -57,6 +57,18 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
>      }
>  }
>  
> +HotplugInfo *hotplug_handler_get_state(HotplugHandler *plug_handler,
> +                                       DeviceState *plugged_dev, Error **errp)
> +{
> +    HotplugHandlerClass *hdc = HOTPLUG_HANDLER_GET_CLASS(plug_handler);
> +
> +    if (hdc->get_hotplug_state) {
> +        return hdc->get_hotplug_state(plug_handler, plugged_dev, errp);
> +    }
> +
> +    return NULL;
> +}
> +
>  static const TypeInfo hotplug_handler_info = {
>      .name          = TYPE_HOTPLUG_HANDLER,
>      .parent        = TYPE_INTERFACE,
> diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
> index a9840ed485..23c57a0967 100644
> --- a/include/hw/hotplug.h
> +++ b/include/hw/hotplug.h
> @@ -13,6 +13,7 @@
>  #define HOTPLUG_H
>  
>  #include "qom/object.h"
> +#include "qapi/qapi-types-qdev.h"
>  
>  #define TYPE_HOTPLUG_HANDLER "hotplug-handler"
>  
> @@ -60,6 +61,8 @@ struct HotplugHandlerClass {
>      hotplug_fn unplug_request;
>      hotplug_fn unplug;
>      bool (*is_hotpluggable_bus)(HotplugHandler *plug_handler, BusState *bus);
> +    HotplugInfo *(*get_hotplug_state)(HotplugHandler *plug_handler,
> +                                      DeviceState *plugged_dev, Error **errp);
>  };
>  
>  /**
> @@ -96,4 +99,12 @@ void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
>  void hotplug_handler_unplug(HotplugHandler *plug_handler,
>                              DeviceState *plugged_dev,
>                              Error **errp);
> +
> +/**
> + * hotplug_handler_get_state:
> + *
> + * Calls #HotplugHandlerClass.get_hotplug_state callback of @plug_handler.
> + */
> +HotplugInfo *hotplug_handler_get_state(HotplugHandler *plug_handler,
> +                                       DeviceState *plugged_dev, Error **errp);
>  #endif
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 151d968238..a27ef2eb24 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -229,6 +229,13 @@ struct DeviceState {
>       * @realized: has device been realized?
>       */
>      bool realized;
> +
> +    /**
> +     * @device_on_event_sent: When true means that DEVICE_ON event was already
> +     *                        sent.
> +     */
> +    bool device_on_event_sent;
> +
>      /**
>       * @pending_deleted_event: track pending deletion events during unplug
>       */
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index 1d57bf6577..c1c8798e89 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -36,4 +36,6 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>   */
>  const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
>  
> +void qdev_hotplug_device_on_event(DeviceState *dev);
> +
>  #endif
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2899fac837..fa80694735 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -180,3 +180,138 @@
>  #      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
>  ##
>  { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
> +
> +##
> +# @LedStatus:
> +#
> +# @on: LED is on.
> +#
> +# @blinking: LED is blinking.
> +#
> +# @off: LED is off.
> +#
> +# Since: 8.2
> +##
> +{ 'enum': 'LedStatus',
> +  'data': [ 'on', 'blinking', 'off' ] }
> +

Querying LED status is by itself a reasonable thing.
It's under guest control though - how do we avoid
a flood of events?


> +##
> +# @HotplugSHPCSlotState:
> +#
> +# Standard Hot-Plug Controller slot state.
> +#
> +# @power-only: Slot is powered on, but neither clock nor bus are
> +#     connected.
> +#
> +# @enabled: Slot is powered on, clock and bus are connected, and the
> +#     card is fully functional from a hardware standpoint.
> +#
> +# @disabled: Slot is disabled, card is safe to be removed.
> +#

Are these really the only 3 states a slot can be in?
And do we care about power-only?


> +# Since: 8.2
> +##
> +{ 'enum': 'HotplugSHPCSlotState',
> +  'data': [ 'power-only', 'enabled', 'disabled' ] }
> +
> +##
> +# @HotplugSHPCState:
> +#
> +# Standard Hot Plug Controller state.
> +#
> +# @slot-state: The slot state field of SHPC Slot Status structure.
> +#
> +# Since: 8.2
> +##
> +{ 'struct': 'HotplugSHPCState',
> +  'data': { '*slot-state': 'HotplugSHPCSlotState' } }
> +
> +##
> +# @HotplugPCIeNativeState:
> +#
> +# PCIe Native hotplug slot state.
> +#
> +# @power-on: PCIe Power Controller Control of Slot Control Register.
> +#     True means Power On (Power Controller Control bit is 0), False
> +#     means Power Off (Power Controller Control bit is 1).
> +#
> +# Since: 8.2
> +##
> +{ 'struct': 'HotplugPCIeNativeState',
> +  'data': { '*power-on': 'bool' } }

and here we don't care about enable?

> +
> +##
> +# @HotplugType:
> +#
> +# Type of hotplug controller / provider.
> +#
> +# @shpc: PCI Standard Hot-plug Controller
> +#
> +# @pcie-native: PCIe Native hotplug
> +#
> +# TODO: add @acpi type
> +#
> +# Since: 8.2
> +##
> +{ 'enum': 'HotplugType',
> +  'data': ['shpc', 'pcie-native'] }
> +
> +##
> +# @HotplugInfo:
> +#
> +# Hotplug slot state.
> +#
> +# @type: type of the hotplug
> +#
> +# @bus: The QOM path of the parent bus where device is plugged into.
> +#
> +# @addr: The bus address for hotplugged device if applicable.
> +#
> +# @child: the hotplugged device
> +#
> +# @device-on: Device is powered-on by guest.  This state changes at
> +#     most once for the device and corresponds to DEVICE_ON event.
> +#
> +# @power-led: Power indicator.  When power LED is on the device is
> +#     ready and accepted by guest.  When it is off, device is safe to
> +#     remove.  It is blinking while hot plug or unplug is in progress.
> +#
> +# @attention-led: The attention LED is normally off.  It is on to
> +#     signal a problem.  Blinking is for helping users to locate the
> +#     slot.
> +#
> +# Single: 8.2
> +##
> +{ 'union': 'HotplugInfo',
> +  'base': { 'type': 'HotplugType',
> +            'bus': 'DeviceAndPath',
> +            '*addr': 'str',
> +            'child': 'DeviceAndPath',
> +            'device-on': 'bool',
> +            '*power-led': 'LedStatus',
> +            '*attention-led': 'LedStatus'},
> +  'discriminator': 'type',
> +  'data': { 'shpc': 'HotplugSHPCState',
> +            'pcie-native': 'HotplugPCIeNativeState' } }
> +

What about ACPI hotplug?



> +##
> +# @x-query-hotplug:
> +#
> +# Query the state of hotplug controller.
> +#
> +# @id: the device's ID or QOM path
> +#
> +# Since: 8.2
> +##
> +{ 'command': 'x-query-hotplug',
> +  'data': { 'id': 'str' },
> +  'returns': 'HotplugInfo' }
> +
> +##
> +# @X_DEVICE_ON:
> +#
> +# Emitted whenever the device insertion completion is acknowledged by
> +# the guest.  For now only emitted for SHPC and PCIe-native hotplug.
> +#
> +# Since: 8.2
> +##
> +{ 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' }
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 74f4e41338..483da40cb9 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -25,6 +25,7 @@
>  #include "sysemu/arch_init.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-qdev.h"
> +#include "qapi/qapi-events-qdev.h"
>  #include "qapi/qmp/dispatch.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qerror.h"
> @@ -956,6 +957,36 @@ void qmp_device_del(const char *id, Error **errp)
>      }
>  }
>  
> +HotplugInfo *qmp_x_query_hotplug(const char *id, Error **errp)
> +{
> +    DeviceState *dev = find_device_state(id, errp);
> +    HotplugHandler *hotplug_ctrl;
> +
> +    if (!dev) {
> +        return NULL;
> +    }
> +
> +    if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
> +        error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
> +        return NULL;
> +    }
> +
> +    if (!DEVICE_GET_CLASS(dev)->hotpluggable) {
> +        error_setg(errp, QERR_DEVICE_NO_HOTPLUG,
> +                   object_get_typename(OBJECT(dev)));
> +        return NULL;
> +    }
> +
> +    hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +    /*
> +     * hotpluggable device MUST have HotplugHandler, if it doesn't
> +     * then something is very wrong with it.
> +     */
> +    g_assert(hotplug_ctrl);
> +
> +    return hotplug_handler_get_state(hotplug_ctrl, dev, errp);
> +}
> +
>  void hmp_device_add(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> @@ -1146,3 +1177,13 @@ bool qmp_command_available(const QmpCommand *cmd, Error **errp)
>      }
>      return true;
>  }
> +
> +void qdev_hotplug_device_on_event(DeviceState *dev)
> +{
> +    if (dev->device_on_event_sent) {
> +        return;
> +    }
> +
> +    dev->device_on_event_sent = true;
> +    qapi_event_send_x_device_on(dev->id, dev->canonical_path);
> +}
> -- 
> 2.34.1

Re: [PATCH v8 0/4] pci hotplug tracking

[Vladimir Sementsov-Ogievskiy]

On 02.11.23 14:31, Michael S. Tsirkin wrote:
> On Thu, Oct 05, 2023 at 12:29:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Main thing this series does is DEVICE_ON event - a counter-part to
>> DEVICE_DELETED. A guest-driven event that device is powered-on.
>> Details are in patch 2. The new event is paried with corresponding
>> command query-hotplug.
> 
> Several things questionable here:
> 1. depending on guest activity you can get as many
>     DEVICE_ON events as you like

No, I've made it so it may be sent only once per device

> 2. it's just for shpc and native pcie - things are
>     confusing enough for management, we should make sure
>     it can work for all devices

Agree, I'm thinking about it

> 3. what about non hotpluggable devices? do we want the event for them?
> 

I think, yes, especially if we make async=true|false flag for device_add, so that successful device_add must be always followed by DEVICE_ON - like device_del is followed by DEVICE_DELETED.

Maybe, to generalize, it should be called not DEVICE_ON (which mostly relate to hotplug controller statuses) but DEVICE_ADDED - a full counterpart for DEVICE_DELETED.

> 
> I feel this needs actual motivation so we can judge what's the
> right way to do it.

My first motivation for this series was the fact that successful device_add doesn't guarantee that hard disk successfully hotplugged to the guest. It relates to some problems with shpc/pcie hotplug we had in the past, and they are mostly fixed. But still, for management tool it's good to understand that all actions related to hotplug controller are done and we have "green light".

Recently new motivation come, as I described in my "ping" letter <6bd19a07-5224-464d-b54d-1d738f5ba8f7@yandex-team.ru>, that we have a performance degradation because of 7bed89958bfbf40df, which introduces drain_call_rcu() in device_add, to make it more synchronous. So, my suggestion is make it instead more asynchronous (probably with special flag) and rely on DEVICE_ON event.

> 
> 
>>
>> v8:
>>   - improve naming, wording and style
>>   - make new QMP interface experimental
>>
>>
>> Vladimir Sementsov-Ogievskiy (4):
>>    qapi/qdev.json: unite DEVICE_* event data into single structure
>>    qapi: add DEVICE_ON and query-hotplug infrastructure
>>    shpc: implement DEVICE_ON event and query-hotplug
>>    pcie: implement DEVICE_ON event and query-hotplug
>>
>>   hw/core/hotplug.c               |  12 +++
>>   hw/pci-bridge/pci_bridge_dev.c  |  14 +++
>>   hw/pci-bridge/pcie_pci_bridge.c |   1 +
>>   hw/pci/pcie.c                   |  83 +++++++++++++++
>>   hw/pci/pcie_port.c              |   1 +
>>   hw/pci/shpc.c                   |  86 +++++++++++++++
>>   include/hw/hotplug.h            |  11 ++
>>   include/hw/pci/pci_bridge.h     |   2 +
>>   include/hw/pci/pcie.h           |   2 +
>>   include/hw/pci/shpc.h           |   2 +
>>   include/hw/qdev-core.h          |   7 ++
>>   include/monitor/qdev.h          |   6 ++
>>   qapi/qdev.json                  | 178 +++++++++++++++++++++++++++++---
>>   softmmu/qdev-monitor.c          |  58 +++++++++++
>>   14 files changed, 451 insertions(+), 12 deletions(-)
>>
>> -- 
>> 2.34.1
> 

-- 
Best regards,
Vladimir

Re: [PATCH v8 0/4] pci hotplug tracking

[Michael S. Tsirkin]

On Thu, Nov 02, 2023 at 03:00:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 02.11.23 14:31, Michael S. Tsirkin wrote:
> > On Thu, Oct 05, 2023 at 12:29:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > Hi all!
> > > 
> > > Main thing this series does is DEVICE_ON event - a counter-part to
> > > DEVICE_DELETED. A guest-driven event that device is powered-on.
> > > Details are in patch 2. The new event is paried with corresponding
> > > command query-hotplug.
> > 
> > Several things questionable here:
> > 1. depending on guest activity you can get as many
> >     DEVICE_ON events as you like
> 
> No, I've made it so it may be sent only once per device

Maybe document that?

> > 2. it's just for shpc and native pcie - things are
> >     confusing enough for management, we should make sure
> >     it can work for all devices
> 
> Agree, I'm thinking about it
> 
> > 3. what about non hotpluggable devices? do we want the event for them?
> > 
> 
> I think, yes, especially if we make async=true|false flag for device_add, so that successful device_add must be always followed by DEVICE_ON - like device_del is followed by DEVICE_DELETED.
> 
> Maybe, to generalize, it should be called not DEVICE_ON (which mostly relate to hotplug controller statuses) but DEVICE_ADDED - a full counterpart for DEVICE_DELETED.
> 
> > 
> > I feel this needs actual motivation so we can judge what's the
> > right way to do it.
> 
> My first motivation for this series was the fact that successful device_add doesn't guarantee that hard disk successfully hotplugged to the guest. It relates to some problems with shpc/pcie hotplug we had in the past, and they are mostly fixed. But still, for management tool it's good to understand that all actions related to hotplug controller are done and we have "green light".

what does "successfully" mean though? E.g. a bunch of guests will not
properly show you the device if the disk is not formatted properly.

> 
> Recently new motivation come, as I described in my "ping" letter <6bd19a07-5224-464d-b54d-1d738f5ba8f7@yandex-team.ru>, that we have a performance degradation because of 7bed89958bfbf40df, which introduces drain_call_rcu() in device_add, to make it more synchronous. So, my suggestion is make it instead more asynchronous (probably with special flag) and rely on DEVICE_ON event.

This one?

commit 7bed89958bfbf40df9ca681cefbdca63abdde39d
Author: Maxim Levitsky <mlevitsk@redhat.com>
Date:   Tue Oct 6 14:38:58 2020 +0200

    device_core: use drain_call_rcu in in qmp_device_add
    
    Soon, a device removal might only happen on RCU callback execution.
    This is okay for device-del which provides a DEVICE_DELETED event,
    but not for the failure case of device-add.  To avoid changing
    monitor semantics, just drain all pending RCU callbacks on error.
    
    Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
    Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
    Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
    Message-Id: <20200913160259.32145-4-mlevitsk@redhat.com>
    [Don't use it in qmp_device_del. - Paolo]
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index e9b7228480..bcfb90a08f 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -803,6 +803,18 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
         return;
     }
     dev = qdev_device_add(opts, errp);
+
+    /*
+     * Drain all pending RCU callbacks. This is done because
+     * some bus related operations can delay a device removal
+     * (in this case this can happen if device is added and then
+     * removed due to a configuration error)
+     * to a RCU callback, but user might expect that this interface
+     * will finish its job completely once qmp command returns result
+     * to the user
+     */
+    drain_call_rcu();
+
     if (!dev) {
         qemu_opts_del(opts);
         return;



So maybe just move drain_call_rcu under if (!dev) then and be done with
it?

-- 
MST

Re: [PATCH v8 0/4] pci hotplug tracking

[Vladimir Sementsov-Ogievskiy]

On 02.11.23 15:12, Michael S. Tsirkin wrote:
> On Thu, Nov 02, 2023 at 03:00:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 02.11.23 14:31, Michael S. Tsirkin wrote:
>>> On Thu, Oct 05, 2023 at 12:29:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> Hi all!
>>>>
>>>> Main thing this series does is DEVICE_ON event - a counter-part to
>>>> DEVICE_DELETED. A guest-driven event that device is powered-on.
>>>> Details are in patch 2. The new event is paried with corresponding
>>>> command query-hotplug.
>>>
>>> Several things questionable here:
>>> 1. depending on guest activity you can get as many
>>>      DEVICE_ON events as you like
>>
>> No, I've made it so it may be sent only once per device
> 
> Maybe document that?

Right, my fault

> 
>>> 2. it's just for shpc and native pcie - things are
>>>      confusing enough for management, we should make sure
>>>      it can work for all devices
>>
>> Agree, I'm thinking about it
>>
>>> 3. what about non hotpluggable devices? do we want the event for them?
>>>
>>
>> I think, yes, especially if we make async=true|false flag for device_add, so that successful device_add must be always followed by DEVICE_ON - like device_del is followed by DEVICE_DELETED.
>>
>> Maybe, to generalize, it should be called not DEVICE_ON (which mostly relate to hotplug controller statuses) but DEVICE_ADDED - a full counterpart for DEVICE_DELETED.
>>
>>>
>>> I feel this needs actual motivation so we can judge what's the
>>> right way to do it.
>>
>> My first motivation for this series was the fact that successful device_add doesn't guarantee that hard disk successfully hotplugged to the guest. It relates to some problems with shpc/pcie hotplug we had in the past, and they are mostly fixed. But still, for management tool it's good to understand that all actions related to hotplug controller are done and we have "green light".
> 
> what does "successfully" mean though? E.g. a bunch of guests will not
> properly show you the device if the disk is not formatted properly.

Yes, I understand, that we may say only about "some degree of success".

But here is some physical sense still: DEVICE_ON indicates, that it's now safe to call device_del. And calling device_del before DEVICE_ON is a kind of unexpected behavior.


> 
>>
>> Recently new motivation come, as I described in my "ping" letter <6bd19a07-5224-464d-b54d-1d738f5ba8f7@yandex-team.ru>, that we have a performance degradation because of 7bed89958bfbf40df, which introduces drain_call_rcu() in device_add, to make it more synchronous. So, my suggestion is make it instead more asynchronous (probably with special flag) and rely on DEVICE_ON event.
> 
> This one?
> 
> commit 7bed89958bfbf40df9ca681cefbdca63abdde39d
> Author: Maxim Levitsky <mlevitsk@redhat.com>
> Date:   Tue Oct 6 14:38:58 2020 +0200
> 
>      device_core: use drain_call_rcu in in qmp_device_add
>      
>      Soon, a device removal might only happen on RCU callback execution.
>      This is okay for device-del which provides a DEVICE_DELETED event,
>      but not for the failure case of device-add.  To avoid changing
>      monitor semantics, just drain all pending RCU callbacks on error.
>      
>      Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>      Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
>      Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>      Message-Id: <20200913160259.32145-4-mlevitsk@redhat.com>
>      [Don't use it in qmp_device_del. - Paolo]
>      Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index e9b7228480..bcfb90a08f 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -803,6 +803,18 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
>           return;
>       }
>       dev = qdev_device_add(opts, errp);
> +
> +    /*
> +     * Drain all pending RCU callbacks. This is done because
> +     * some bus related operations can delay a device removal
> +     * (in this case this can happen if device is added and then
> +     * removed due to a configuration error)
> +     * to a RCU callback, but user might expect that this interface
> +     * will finish its job completely once qmp command returns result
> +     * to the user
> +     */
> +    drain_call_rcu();
> +
>       if (!dev) {
>           qemu_opts_del(opts);
>           return;
> 
> 
> 
> So maybe just move drain_call_rcu under if (!dev) then and be done with
> it?
> 

Hmm, I read the commit message thinking that it saying about device removal by mistake and actually want to say both about device_add and device_del.. But I was wrong?

Hmm, it directly say "just drain all pending RCU callbacks on error", but does that on success path as well.

Yes, moving drain_call_rcu makes sense for me, and will close the second "motivation". I can make a patch.

-- 
Best regards,
Vladimir

Re: [PATCH v8 0/4] pci hotplug tracking

[Michael S. Tsirkin]

On Thu, Nov 02, 2023 at 04:28:43PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 02.11.23 15:12, Michael S. Tsirkin wrote:
> > On Thu, Nov 02, 2023 at 03:00:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > On 02.11.23 14:31, Michael S. Tsirkin wrote:
> > > > On Thu, Oct 05, 2023 at 12:29:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > > Hi all!
> > > > > 
> > > > > Main thing this series does is DEVICE_ON event - a counter-part to
> > > > > DEVICE_DELETED. A guest-driven event that device is powered-on.
> > > > > Details are in patch 2. The new event is paried with corresponding
> > > > > command query-hotplug.
> > > > 
> > > > Several things questionable here:
> > > > 1. depending on guest activity you can get as many
> > > >      DEVICE_ON events as you like
> > > 
> > > No, I've made it so it may be sent only once per device
> > 
> > Maybe document that?
> 
> Right, my fault
> 
> > 
> > > > 2. it's just for shpc and native pcie - things are
> > > >      confusing enough for management, we should make sure
> > > >      it can work for all devices
> > > 
> > > Agree, I'm thinking about it
> > > 
> > > > 3. what about non hotpluggable devices? do we want the event for them?
> > > > 
> > > 
> > > I think, yes, especially if we make async=true|false flag for device_add, so that successful device_add must be always followed by DEVICE_ON - like device_del is followed by DEVICE_DELETED.
> > > 
> > > Maybe, to generalize, it should be called not DEVICE_ON (which mostly relate to hotplug controller statuses) but DEVICE_ADDED - a full counterpart for DEVICE_DELETED.
> > > 
> > > > 
> > > > I feel this needs actual motivation so we can judge what's the
> > > > right way to do it.
> > > 
> > > My first motivation for this series was the fact that successful device_add doesn't guarantee that hard disk successfully hotplugged to the guest. It relates to some problems with shpc/pcie hotplug we had in the past, and they are mostly fixed. But still, for management tool it's good to understand that all actions related to hotplug controller are done and we have "green light".
> > 
> > what does "successfully" mean though? E.g. a bunch of guests will not
> > properly show you the device if the disk is not formatted properly.
> 
> Yes, I understand, that we may say only about "some degree of success".
> 
> But here is some physical sense still: DEVICE_ON indicates, that it's now safe to call device_del. And calling device_del before DEVICE_ON is a kind of unexpected behavior.
> 

Is that really true? I really don't think we should introduce new types
of undefined behavior.


> > 
> > > 
> > > Recently new motivation come, as I described in my "ping" letter <6bd19a07-5224-464d-b54d-1d738f5ba8f7@yandex-team.ru>, that we have a performance degradation because of 7bed89958bfbf40df, which introduces drain_call_rcu() in device_add, to make it more synchronous. So, my suggestion is make it instead more asynchronous (probably with special flag) and rely on DEVICE_ON event.
> > 
> > This one?
> > 
> > commit 7bed89958bfbf40df9ca681cefbdca63abdde39d
> > Author: Maxim Levitsky <mlevitsk@redhat.com>
> > Date:   Tue Oct 6 14:38:58 2020 +0200
> > 
> >      device_core: use drain_call_rcu in in qmp_device_add
> >      Soon, a device removal might only happen on RCU callback execution.
> >      This is okay for device-del which provides a DEVICE_DELETED event,
> >      but not for the failure case of device-add.  To avoid changing
> >      monitor semantics, just drain all pending RCU callbacks on error.
> >      Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> >      Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
> >      Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >      Message-Id: <20200913160259.32145-4-mlevitsk@redhat.com>
> >      [Don't use it in qmp_device_del. - Paolo]
> >      Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> > index e9b7228480..bcfb90a08f 100644
> > --- a/softmmu/qdev-monitor.c
> > +++ b/softmmu/qdev-monitor.c
> > @@ -803,6 +803,18 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
> >           return;
> >       }
> >       dev = qdev_device_add(opts, errp);
> > +
> > +    /*
> > +     * Drain all pending RCU callbacks. This is done because
> > +     * some bus related operations can delay a device removal
> > +     * (in this case this can happen if device is added and then
> > +     * removed due to a configuration error)
> > +     * to a RCU callback, but user might expect that this interface
> > +     * will finish its job completely once qmp command returns result
> > +     * to the user
> > +     */
> > +    drain_call_rcu();
> > +
> >       if (!dev) {
> >           qemu_opts_del(opts);
> >           return;
> > 
> > 
> > 
> > So maybe just move drain_call_rcu under if (!dev) then and be done with
> > it?
> > 
> 
> Hmm, I read the commit message thinking that it saying about device removal by mistake and actually want to say both about device_add and device_del.. But I was wrong?
> 
> Hmm, it directly say "just drain all pending RCU callbacks on error", but does that on success path as well.
> 
> Yes, moving drain_call_rcu makes sense for me, and will close the second "motivation". I can make a patch.
> 
> -- 
> Best regards,
> Vladimir

Re: [PATCH v8 0/4] pci hotplug tracking

[Vladimir Sementsov-Ogievskiy]

On 02.11.23 16:32, Michael S. Tsirkin wrote:
> On Thu, Nov 02, 2023 at 04:28:43PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 02.11.23 15:12, Michael S. Tsirkin wrote:
>>> On Thu, Nov 02, 2023 at 03:00:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> On 02.11.23 14:31, Michael S. Tsirkin wrote:
>>>>> On Thu, Oct 05, 2023 at 12:29:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Hi all!
>>>>>>
>>>>>> Main thing this series does is DEVICE_ON event - a counter-part to
>>>>>> DEVICE_DELETED. A guest-driven event that device is powered-on.
>>>>>> Details are in patch 2. The new event is paried with corresponding
>>>>>> command query-hotplug.
>>>>>
>>>>> Several things questionable here:
>>>>> 1. depending on guest activity you can get as many
>>>>>       DEVICE_ON events as you like
>>>>
>>>> No, I've made it so it may be sent only once per device
>>>
>>> Maybe document that?
>>
>> Right, my fault
>>
>>>
>>>>> 2. it's just for shpc and native pcie - things are
>>>>>       confusing enough for management, we should make sure
>>>>>       it can work for all devices
>>>>
>>>> Agree, I'm thinking about it
>>>>
>>>>> 3. what about non hotpluggable devices? do we want the event for them?
>>>>>
>>>>
>>>> I think, yes, especially if we make async=true|false flag for device_add, so that successful device_add must be always followed by DEVICE_ON - like device_del is followed by DEVICE_DELETED.
>>>>
>>>> Maybe, to generalize, it should be called not DEVICE_ON (which mostly relate to hotplug controller statuses) but DEVICE_ADDED - a full counterpart for DEVICE_DELETED.
>>>>
>>>>>
>>>>> I feel this needs actual motivation so we can judge what's the
>>>>> right way to do it.
>>>>
>>>> My first motivation for this series was the fact that successful device_add doesn't guarantee that hard disk successfully hotplugged to the guest. It relates to some problems with shpc/pcie hotplug we had in the past, and they are mostly fixed. But still, for management tool it's good to understand that all actions related to hotplug controller are done and we have "green light".
>>>
>>> what does "successfully" mean though? E.g. a bunch of guests will not
>>> properly show you the device if the disk is not formatted properly.
>>
>> Yes, I understand, that we may say only about "some degree of success".
>>
>> But here is some physical sense still: DEVICE_ON indicates, that it's now safe to call device_del. And calling device_del before DEVICE_ON is a kind of unexpected behavior.
>>
> 
> Is that really true? I really don't think we should introduce new types
> of undefined behavior.

Good question. At least, it was true before some recent fixes to SHPC..

And even if it is so now, we could instead fix it by some "internal DEVICE_ON", and postpone device deletion until we have it...

OK, I need a round to rethink this all. Thanks!

> 
> 
>>>
>>>>
>>>> Recently new motivation come, as I described in my "ping" letter <6bd19a07-5224-464d-b54d-1d738f5ba8f7@yandex-team.ru>, that we have a performance degradation because of 7bed89958bfbf40df, which introduces drain_call_rcu() in device_add, to make it more synchronous. So, my suggestion is make it instead more asynchronous (probably with special flag) and rely on DEVICE_ON event.
>>>
>>> This one?
>>>
>>> commit 7bed89958bfbf40df9ca681cefbdca63abdde39d
>>> Author: Maxim Levitsky <mlevitsk@redhat.com>
>>> Date:   Tue Oct 6 14:38:58 2020 +0200
>>>
>>>       device_core: use drain_call_rcu in in qmp_device_add
>>>       Soon, a device removal might only happen on RCU callback execution.
>>>       This is okay for device-del which provides a DEVICE_DELETED event,
>>>       but not for the failure case of device-add.  To avoid changing
>>>       monitor semantics, just drain all pending RCU callbacks on error.
>>>       Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>>>       Suggested-by: Stefan Hajnoczi <stefanha@gmail.com>
>>>       Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>       Message-Id: <20200913160259.32145-4-mlevitsk@redhat.com>
>>>       [Don't use it in qmp_device_del. - Paolo]
>>>       Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>
>>> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
>>> index e9b7228480..bcfb90a08f 100644
>>> --- a/softmmu/qdev-monitor.c
>>> +++ b/softmmu/qdev-monitor.c
>>> @@ -803,6 +803,18 @@ void qmp_device_add(QDict *qdict, QObject **ret_data, Error **errp)
>>>            return;
>>>        }
>>>        dev = qdev_device_add(opts, errp);
>>> +
>>> +    /*
>>> +     * Drain all pending RCU callbacks. This is done because
>>> +     * some bus related operations can delay a device removal
>>> +     * (in this case this can happen if device is added and then
>>> +     * removed due to a configuration error)
>>> +     * to a RCU callback, but user might expect that this interface
>>> +     * will finish its job completely once qmp command returns result
>>> +     * to the user
>>> +     */
>>> +    drain_call_rcu();
>>> +
>>>        if (!dev) {
>>>            qemu_opts_del(opts);
>>>            return;
>>>
>>>
>>>
>>> So maybe just move drain_call_rcu under if (!dev) then and be done with
>>> it?
>>>
>>
>> Hmm, I read the commit message thinking that it saying about device removal by mistake and actually want to say both about device_add and device_del.. But I was wrong?
>>
>> Hmm, it directly say "just drain all pending RCU callbacks on error", but does that on success path as well.
>>
>> Yes, moving drain_call_rcu makes sense for me, and will close the second "motivation". I can make a patch.
>>
>> -- 
>> Best regards,
>> Vladimir
> 

-- 
Best regards,
Vladimir