[PATCH v7 0/4] pci hotplug tracking

[PATCH v7 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.

v7: improve commit message of 02, change version to 8.1 in QAPI.

v6:
- preparation part merged to master
- drop HOTPLUG_STATE event for now. I'm not sure we need it, and anyway,
  it should be throttled as it may be triggered by guest at any rate.
  And if it is throttled:
  - we can't track real state changes, so probably reporting only
  changed fields is not enough.. Then the format of the event is under
  question.
  - we should implement throttling separately for different devices
  - we should improve throttling to support several events in a short
  time, still keeping small rate in larger periods, as guest tends to
  change some states sequentially in a short time.
  So, that's not easy:) Let's just drop it for now.
- force DEVICE_ON event to be triggered only once per device
- flatten query-hotplug output and add device-on status (corresponding
  to DEVICE_ON event)

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               |  13 +++
 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            |  12 +++
 include/hw/pci/pci_bridge.h     |   2 +
 include/hw/pci/pcie.h           |   2 +
 include/hw/pci/shpc.h           |   2 +
 include/hw/qdev-core.h          |   1 +
 include/monitor/qdev.h          |   6 ++
 qapi/qdev.json                  | 183 +++++++++++++++++++++++++++++---
 softmmu/qdev-monitor.c          |  58 ++++++++++
 14 files changed, 452 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH v7 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 | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 2708fb4e99..135cd81586 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -114,16 +114,37 @@
 { 'command': 'device_del', 'data': {'id': 'str'} }
 
 ##
-# @DEVICE_DELETED:
+# @DeviceAndPath:
 #
-# Emitted whenever the device removal completion is acknowledged by the guest.
-# At this point, it's safe to reuse the specified device ID. Device removal can
-# be initiated by the guest or by HMP/QMP commands.
+# In events we designate devices by both their ID (if the device has one)
+# and QOM path.
+#
+# 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.
 #
 # @device: the device's ID if it has one
 #
 # @path: the device's QOM path
 #
+# Since: 8.0
+##
+{ 'struct': 'DeviceAndPath',
+  'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @DEVICE_DELETED:
+#
+# Emitted whenever the device removal completion is acknowledged by the guest.
+# At this point, it's safe to reuse the specified device ID. Device removal can
+# be initiated by the guest or by HMP/QMP commands.
+#
 # Since: 1.5
 #
 # Example:
@@ -134,18 +155,13 @@
 #      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
 #
 ##
-{ 'event': 'DEVICE_DELETED',
-  'data': { '*device': 'str', 'path': 'str' } }
+{ 'event': 'DEVICE_DELETED', 'data': 'DeviceAndPath' }
 
 ##
 # @DEVICE_UNPLUG_GUEST_ERROR:
 #
 # 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:
@@ -156,5 +172,4 @@
 #      "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 v7 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      |  13 ++++
 include/hw/hotplug.h   |  12 ++++
 include/hw/qdev-core.h |   1 +
 include/monitor/qdev.h |   2 +
 qapi/qdev.json         | 144 +++++++++++++++++++++++++++++++++++++++++
 softmmu/qdev-monitor.c |  41 ++++++++++++
 6 files changed, 213 insertions(+)

diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
index 17ac986685..08d6d03760 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -57,6 +57,19 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
     }
 }
 
+HotplugInfo *hotplug_handler_get_hotplug_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..9c43e1263b 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,13 @@ void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
 void hotplug_handler_unplug(HotplugHandler *plug_handler,
                             DeviceState *plugged_dev,
                             Error **errp);
+
+/**
+ * hotplug_handler_get_hotplug_state:
+ *
+ * Calls #HotplugHandlerClass.get_hotplug_state callback of @plug_handler.
+ */
+HotplugInfo *hotplug_handler_get_hotplug_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 bd50ad5ee1..63889e41c0 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -180,6 +180,7 @@ struct DeviceState {
     char *id;
     char *canonical_path;
     bool realized;
+    bool device_on_sent; /* set once by SHPC or PCIE-hotplug */
     bool pending_deleted_event;
     int64_t pending_deleted_expires_ms;
     QDict *opts;
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 135cd81586..ffd20c43e0 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -173,3 +173,147 @@
 #
 ##
 { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
+
+##
+# @LedActivity:
+#
+# Three-state led indicator state.
+#
+# @on: Indicator is on.
+#
+# @blink: Indicator is blinking.
+#
+# @off: Indicator is off.
+#
+# Since: 8.1
+##
+{ 'enum': 'LedActivity',
+  'data': [ 'on', 'blink', '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, the card is
+#           fully functional from a hardware standpoint.
+#
+# @disabled: Slot is disabled, card is safe to be removed.
+#
+# Since: 8.1
+##
+{ 'enum': 'HotplugSHPCSlotState',
+  'data': [ 'power-only', 'enabled', 'disabled' ] }
+
+##
+# @HotplugBaseState:
+#
+# Base structure for SHPC and PCIe-native hotplug.
+#
+# @power-led: Power indicator. When power indicator is on the device is
+#             ready and accepted by guest. Off status means that device
+#             is safe to remove and blinking is an intermediate state of
+#             hot-plug or hot-unplug.
+#
+# @attention-led: Attention indicator. Off status means normal operation,
+#                 On signals about operational problem, Blinking is for
+#                 locating the slot.
+#
+# Since: 8.1
+##
+{ 'struct': 'HotplugBaseState',
+  'data': { '*power-led': 'LedActivity',
+            '*attention-led': 'LedActivity' } }
+
+##
+# @HotplugSHPCState:
+#
+# Standard Hot Plug Controller state.
+#
+# @slot-state: The slot state field of Slot Status.
+#
+# Since: 8.1
+##
+{ 'struct': 'HotplugSHPCState',
+  'base': 'HotplugBaseState',
+  '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.1
+##
+{ 'struct': 'HotplugPCIeNativeState',
+  'base': 'HotplugBaseState',
+  'data': { '*power-on': 'bool' } }
+
+##
+# @HotplugType:
+#
+# Type of hotplug controller / provider.
+#
+# @shpc: Standard Hot Plug Controller
+#
+# @pcie-native: PCIe Native hotplug
+#
+# Since: 8.1
+##
+{ 'enum': 'HotplugType',
+  'data': ['shpc', 'pcie-native'] }
+
+##
+# @HotplugInfo:
+#
+# Generic hotplug slot state.
+#
+# @type: type of the hotplug (shpc or pcie-native)
+#
+# @bus: The QOM path of the parent bus where device is hotplugged.
+#
+# @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.
+#
+# Single: 8.1
+##
+{ 'union': 'HotplugInfo',
+  'base': { 'type': 'HotplugType',
+            'bus': 'DeviceAndPath',
+            '*addr': 'str',
+            'child': 'DeviceAndPath',
+            'device-on': 'bool' },
+  'discriminator': 'type',
+  'data': { 'shpc': 'HotplugSHPCState',
+            'pcie-native': 'HotplugPCIeNativeState' } }
+
+##
+# @query-hotplug:
+#
+# Query the state of hotplug controller.
+#
+# Since: 8.1
+##
+{ 'command': 'query-hotplug',
+  'data': { 'id': 'str' },
+  'returns': 'HotplugInfo' }
+
+##
+# @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.1
+##
+{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index b8d2c4dadd..e4956bbd94 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_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_hotplug_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_sent) {
+        return;
+    }
+
+    dev->device_on_sent = true;
+    qapi_event_send_device_on(dev->id, dev->canonical_path);
+}
-- 
2.34.1

[PATCH v7 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 e7bc7192f1..af2c49ee44 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 LedActivity shpc_led_state_to_qapi(uint8_t value)
+{
+    switch (value) {
+    case SHPC_LED_ON:
+        return LED_ACTIVITY_ON;
+    case SHPC_LED_BLINK:
+        return LED_ACTIVITY_BLINK;
+    case SHPC_LED_OFF:
+        return LED_ACTIVITY_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_sent,
+        .u.shpc.has_power_led = true,
+        .u.shpc.power_led = shpc_led_state_to_qapi(power),
+        .u.shpc.has_attention_led = true,
+        .u.shpc.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 1677176b2a..f5ba4dd776 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -127,6 +127,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 e4956bbd94..814d8260d7 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_sent) {
-- 
2.34.1

[PATCH v7 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 b8c24cf45f..a47c95e4b2 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 LedActivity 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_ACTIVITY_ON;
+    case PCI_EXP_SLTCTL_PWR_IND_BLINK:
+    case PCI_EXP_SLTCTL_ATTN_IND_BLINK:
+        return LED_ACTIVITY_BLINK;
+    case PCI_EXP_SLTCTL_PWR_IND_OFF:
+    case PCI_EXP_SLTCTL_ATTN_IND_OFF:
+        return LED_ACTIVITY_OFF;
+    default:
+        abort();
+    }
+}
+
 /***************************************************************************
  * pci express capability helper functions
  */
@@ -724,6 +751,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)
@@ -731,6 +780,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)) {
         /*
@@ -768,6 +818,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.
@@ -1100,3 +1156,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_sent,
+        .u.pcie_native.has_power_led = true,
+        .u.pcie_native.power_led = pcie_led_state_to_qapi(power_led),
+        .u.pcie_native.has_attention_led = true,
+        .u.pcie_native.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 3cc2b15957..f755a7cacb 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -146,4 +146,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 v7 1/4] qapi/qdev.json: unite DEVICE_* event data into single structure

[Michael S. Tsirkin]

On Fri, Apr 21, 2023 at 01:32:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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>

Can QAPI maintainers please review this patchset?
It's been a month.

> ---
>  qapi/qdev.json | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2708fb4e99..135cd81586 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -114,16 +114,37 @@
>  { 'command': 'device_del', 'data': {'id': 'str'} }
>  
>  ##
> -# @DEVICE_DELETED:
> +# @DeviceAndPath:
>  #
> -# Emitted whenever the device removal completion is acknowledged by the guest.
> -# At this point, it's safe to reuse the specified device ID. Device removal can
> -# be initiated by the guest or by HMP/QMP commands.
> +# In events we designate devices by both their ID (if the device has one)
> +# and QOM path.
> +#
> +# 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.
>  #
>  # @device: the device's ID if it has one
>  #
>  # @path: the device's QOM path
>  #
> +# Since: 8.0
> +##
> +{ 'struct': 'DeviceAndPath',
> +  'data': { '*device': 'str', 'path': 'str' } }
> +

Should be Since: 8.1 no?


> +##
> +# @DEVICE_DELETED:
> +#
> +# Emitted whenever the device removal completion is acknowledged by the guest.
> +# At this point, it's safe to reuse the specified device ID. Device removal can
> +# be initiated by the guest or by HMP/QMP commands.
> +#
>  # Since: 1.5
>  #
>  # Example:
> @@ -134,18 +155,13 @@
>  #      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>  #
>  ##
> -{ 'event': 'DEVICE_DELETED',
> -  'data': { '*device': 'str', 'path': 'str' } }
> +{ 'event': 'DEVICE_DELETED', 'data': 'DeviceAndPath' }
>  
>  ##
>  # @DEVICE_UNPLUG_GUEST_ERROR:
>  #
>  # 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:
> @@ -156,5 +172,4 @@
>  #      "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

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

[Philippe Mathieu-Daudé]

Hi Vladimir,

On 21/4/23 12:32, 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      |  13 ++++
>   include/hw/hotplug.h   |  12 ++++
>   include/hw/qdev-core.h |   1 +
>   include/monitor/qdev.h |   2 +
>   qapi/qdev.json         | 144 +++++++++++++++++++++++++++++++++++++++++
>   softmmu/qdev-monitor.c |  41 ++++++++++++
>   6 files changed, 213 insertions(+)
> 
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 17ac986685..08d6d03760 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -57,6 +57,19 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
>       }
>   }
>   
> +HotplugInfo *hotplug_handler_get_hotplug_state(HotplugHandler *plug_handler,

What about hotplug_handler_get_state()?

> +                                               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..9c43e1263b 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,13 @@ void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
>   void hotplug_handler_unplug(HotplugHandler *plug_handler,
>                               DeviceState *plugged_dev,
>                               Error **errp);
> +
> +/**
> + * hotplug_handler_get_hotplug_state:
> + *
> + * Calls #HotplugHandlerClass.get_hotplug_state callback of @plug_handler.
> + */
> +HotplugInfo *hotplug_handler_get_hotplug_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 bd50ad5ee1..63889e41c0 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -180,6 +180,7 @@ struct DeviceState {
>       char *id;
>       char *canonical_path;
>       bool realized;
> +    bool device_on_sent; /* set once by SHPC or PCIE-hotplug */

This seems to belong to the next patch (not used here).
Anyhow (besides the field misses its description) from the name
I can't figure out what this is about. Probably too generic name
IMHO.

>       bool pending_deleted_event;
>       int64_t pending_deleted_expires_ms;
>       QDict *opts;
> 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 135cd81586..ffd20c43e0 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -173,3 +173,147 @@
>   #
>   ##
>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
> +
> +##
> +# @LedActivity:
> +#
> +# Three-state led indicator state.
> +#
> +# @on: Indicator is on.
> +#
> +# @blink: Indicator is blinking.
> +#
> +# @off: Indicator is off.
> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'LedActivity',
> +  'data': [ 'on', 'blink', 'off' ] }

Possibly useful enough to add in a new qapi/led.json.

> +##
> +# @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, the card is
> +#           fully functional from a hardware standpoint.
> +#
> +# @disabled: Slot is disabled, card is safe to be removed.
> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'HotplugSHPCSlotState',
> +  'data': [ 'power-only', 'enabled', 'disabled' ] }
> +
> +##
> +# @HotplugBaseState:
> +#
> +# Base structure for SHPC and PCIe-native hotplug.
> +#
> +# @power-led: Power indicator. When power indicator is on the device is
> +#             ready and accepted by guest. Off status means that device
> +#             is safe to remove and blinking is an intermediate state of
> +#             hot-plug or hot-unplug.
> +#
> +# @attention-led: Attention indicator. Off status means normal operation,
> +#                 On signals about operational problem, Blinking is for
> +#                 locating the slot.
> +#
> +# Since: 8.1
> +##
> +{ 'struct': 'HotplugBaseState',
> +  'data': { '*power-led': 'LedActivity',
> +            '*attention-led': 'LedActivity' } }
> +
> +##
> +# @HotplugSHPCState:
> +#
> +# Standard Hot Plug Controller state.
> +#
> +# @slot-state: The slot state field of Slot Status.
> +#
> +# Since: 8.1
> +##
> +{ 'struct': 'HotplugSHPCState',
> +  'base': 'HotplugBaseState',
> +  'data': { '*slot-state': 'HotplugSHPCSlotState' } }
> +
> +##
> +# @HotplugPCIeNativeState:
> +#
> +# PCIe Native hotplug slot state.

Doesn't this belong to qapi/pci.json?

> +#
> +# @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.1
> +##
> +{ 'struct': 'HotplugPCIeNativeState',
> +  'base': 'HotplugBaseState',
> +  'data': { '*power-on': 'bool' } }
> +
> +##
> +# @HotplugType:
> +#
> +# Type of hotplug controller / provider.
> +#
> +# @shpc: Standard Hot Plug Controller
> +#
> +# @pcie-native: PCIe Native hotplug

Ditto.

> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'HotplugType',
> +  'data': ['shpc', 'pcie-native'] }
> +
> +##
> +# @HotplugInfo:
> +#
> +# Generic hotplug slot state.
> +#
> +# @type: type of the hotplug (shpc or pcie-native)
> +#
> +# @bus: The QOM path of the parent bus where device is hotplugged.
> +#
> +# @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.
> +#
> +# Single: 8.1
> +##
> +{ 'union': 'HotplugInfo',
> +  'base': { 'type': 'HotplugType',
> +            'bus': 'DeviceAndPath',
> +            '*addr': 'str',
> +            'child': 'DeviceAndPath',
> +            'device-on': 'bool' },
> +  'discriminator': 'type',
> +  'data': { 'shpc': 'HotplugSHPCState',
> +            'pcie-native': 'HotplugPCIeNativeState' } }
> +
> +##
> +# @query-hotplug:
> +#
> +# Query the state of hotplug controller.
> +#
> +# Since: 8.1
> +##
> +{ 'command': 'query-hotplug',
> +  'data': { 'id': 'str' },
> +  'returns': 'HotplugInfo' }
> +
> +##
> +# @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.1
> +##
> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }

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

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Fri, Apr 21, 2023 at 01:32:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 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>
>
> Can QAPI maintainers please review this patchset?
> It's been a month.

It's been a busy month; sorry for the delay.

>> ---
>>  qapi/qdev.json | 39 +++++++++++++++++++++++++++------------
>>  1 file changed, 27 insertions(+), 12 deletions(-)
>> 
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index 2708fb4e99..135cd81586 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -114,16 +114,37 @@
>>  { 'command': 'device_del', 'data': {'id': 'str'} }
>>  
>>  ##
>> -# @DEVICE_DELETED:
>> +# @DeviceAndPath:
>>  #
>> -# Emitted whenever the device removal completion is acknowledged by the guest.
>> -# At this point, it's safe to reuse the specified device ID. Device removal can
>> -# be initiated by the guest or by HMP/QMP commands.
>> +# In events we designate devices by both their ID (if the device has one)
>> +# and QOM path.
>> +#
>> +# 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.

Hmm.  I appreciate rationale, but I'm not sure it fits here.  Would
readers be worse off if we dropped it?

>>  #
>>  # @device: the device's ID if it has one
>>  #
>>  # @path: the device's QOM path
>>  #
>> +# Since: 8.0
>> +##
>> +{ 'struct': 'DeviceAndPath',
>> +  'data': { '*device': 'str', 'path': 'str' } }
>> +
>
> Should be Since: 8.1 no?

Yes.

Please format like

   ##
   # @DeviceAndPath:
   #
   # In events we designate devices by both their ID (if the device has
   # one) and QOM path.
   #
   # 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.
   #
   # @device: the device's ID if it has one
   #
   # @path: the device's QOM path
   #
   # Since: 8.0
   ##

to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).

>> +##
>> +# @DEVICE_DELETED:
>> +#
>> +# Emitted whenever the device removal completion is acknowledged by the guest.
>> +# At this point, it's safe to reuse the specified device ID. Device removal can
>> +# be initiated by the guest or by HMP/QMP commands.
>> +#

Conflict resolution:

    # Emitted whenever the device removal completion is acknowledged by
    # the guest.  At this point, it's safe to reuse the specified device
    # ID. Device removal can be initiated by the guest or by HMP/QMP
    # commands.

>>  # Since: 1.5
>>  #
>>  # Example:
>> @@ -134,18 +155,13 @@
>>  #      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>>  #
>>  ##
>> -{ 'event': 'DEVICE_DELETED',
>> -  'data': { '*device': 'str', 'path': 'str' } }
>> +{ 'event': 'DEVICE_DELETED', 'data': 'DeviceAndPath' }
>>  
>>  ##
>>  # @DEVICE_UNPLUG_GUEST_ERROR:
>>  #
>>  # 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:
>> @@ -156,5 +172,4 @@
>>  #      "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

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

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> 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.

Events often need to be paired with a query to let the management
application resynchronize after a QMP disconnect, say for a restart.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Do we have libvirt patches?  If not, is anybody working on them?  If
not, we need at least interface review by a libvirt developer familiar
with hot-plug.  And we might want to mark the interface experimental.

[...]

> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 135cd81586..ffd20c43e0 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -173,3 +173,147 @@
>  #
>  ##
>  { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
> +
> +##
> +# @LedActivity:

Not sure about "Activity".  "Status"?

> +#
> +# Three-state led indicator state.

Scratch the sentence?

> +#
> +# @on: Indicator is on.
> +#
> +# @blink: Indicator is blinking.
> +#
> +# @off: Indicator is off.

Suggest "LED is on" ans so forth.

> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'LedActivity',
> +  'data': [ 'on', 'blink', 'off' ] }

Suggest 'blinking'.

> +
> +##
> +# @HotplugSHPCSlotState:
> +#
> +# Standard Hot-Plug Controller slot state.
> +#
> +# @power-only: Slot is powered on but neither clock nor bus are connected.

Comma after "on"?

> +#
> +# @enabled: Slot is powered on, clock and bus are connected, the card is
> +#           fully functional from a hardware standpoint.

Suggest ", and the card"

Please format like

   # @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.

to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).

> +#
> +# @disabled: Slot is disabled, card is safe to be removed.
> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'HotplugSHPCSlotState',
> +  'data': [ 'power-only', 'enabled', 'disabled' ] }
> +
> +##
> +# @HotplugBaseState:
> +#
> +# Base structure for SHPC and PCIe-native hotplug.
> +#
> +# @power-led: Power indicator. When power indicator is on the device is
> +#             ready and accepted by guest. Off status means that device
> +#             is safe to remove and blinking is an intermediate state of
> +#             hot-plug or hot-unplug.

Suggest something like "When the power LED is on, the device is ...
When it is off, the device is ...  It is blinking while hot plug or
unplug is in progress."

> +#
> +# @attention-led: Attention indicator. Off status means normal operation,
> +#                 On signals about operational problem, Blinking is for
> +#                 locating the slot.

Suggest something like "The attention LED is normally off.  It is on to
signal a a problem.  Blinking is for helping users to locate the slot."

> +#
> +# Since: 8.1
> +##
> +{ 'struct': 'HotplugBaseState',
> +  'data': { '*power-led': 'LedActivity',
> +            '*attention-led': 'LedActivity' } }
> +
> +##
> +# @HotplugSHPCState:
> +#
> +# Standard Hot Plug Controller state.
> +#
> +# @slot-state: The slot state field of Slot Status.

Color me confused.  What's "Slot Status"?

> +#
> +# Since: 8.1
> +##
> +{ 'struct': 'HotplugSHPCState',
> +  'base': 'HotplugBaseState',
> +  'data': { '*slot-state': 'HotplugSHPCSlotState' } }
> +
> +##
> +# @HotplugPCIeNativeState:
> +#
> +# PCIe Native hotplug slot state.

hot-plug

More below, not flagging it there.

> +#
> +# @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).

Please format like

    # @power-on: PCIe Power Controller Control of Slot Control Register.
    #     True means Power On (Power Controller Control bit is 0), and
    #     False means Power Off (Power Controller Control bit is 1).

> +#
> +# Since: 8.1
> +##
> +{ 'struct': 'HotplugPCIeNativeState',
> +  'base': 'HotplugBaseState',
> +  'data': { '*power-on': 'bool' } }
> +
> +##
> +# @HotplugType:
> +#
> +# Type of hotplug controller / provider.
> +#
> +# @shpc: Standard Hot Plug Controller

Suggest "PCI Standard Hot-plug Controller", because that's how the spec
seems to spell it.

> +#
> +# @pcie-native: PCIe Native hotplug

What about non-PCI hot-plug?

> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'HotplugType',
> +  'data': ['shpc', 'pcie-native'] }
> +
> +##
> +# @HotplugInfo:
> +#
> +# Generic hotplug slot state.

Well, the generic part is defined here, but the union type pulls in
controller-specific parts.  Scratch "generic"?

> +#
> +# @type: type of the hotplug (shpc or pcie-native)

Repeating enum values in doc comments risks them going stale there.
Scratch the parenthesis?

> +#
> +# @bus: The QOM path of the parent bus where device is hotplugged.

Is this the bus the device is plugged into?

What about bus-less devices?  Hmm, I think we can make an argument that
the infrastructure required to make hot-plug work will always be part of
some kind of bus.  Does this make sense to you?

> +#
> +# @addr: The bus address for hotplugged device if applicable.

Uh, does this encode structured data in a string?

> +#
> +# @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.
> +#
> +# Single: 8.1
> +##
> +{ 'union': 'HotplugInfo',
> +  'base': { 'type': 'HotplugType',
> +            'bus': 'DeviceAndPath',
> +            '*addr': 'str',
> +            'child': 'DeviceAndPath',
> +            'device-on': 'bool' },
> +  'discriminator': 'type',
> +  'data': { 'shpc': 'HotplugSHPCState',
> +            'pcie-native': 'HotplugPCIeNativeState' } }

You have data common to both types in two places: base of HotplugInfo
and base of the variant types HotplugBaseState.  Why?

> +
> +##
> +# @query-hotplug:
> +#
> +# Query the state of hotplug controller.
> +#

@id is undocumented.

> +# Since: 8.1
> +##
> +{ 'command': 'query-hotplug',
> +  'data': { 'id': 'str' },
> +  'returns': 'HotplugInfo' }

I have more questions, but I'd like to understand the meaning of @id
before I type them up.

> +
> +##
> +# @DEVICE_ON:
> +#
> +# Emitted whenever the device insertion completion is acknowledged by the guest.
> +# For now only emitted for SHPC and PCIe-native hotplug.

Please format like

   # Emitted whenever the device insertion completion is acknowledged by
   # the guest.  For now only emitted for SHPC and PCIe-native hotplug.

> +#
> +# Since: 8.1
> +##
> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }

[...]

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

[Vladimir Sementsov-Ogievskiy]

On 22.05.23 12:27, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
>> On Fri, Apr 21, 2023 at 01:32:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> 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>
>>
>> Can QAPI maintainers please review this patchset?
>> It's been a month.
> 
> It's been a busy month; sorry for the delay.
> 
>>> ---
>>>   qapi/qdev.json | 39 +++++++++++++++++++++++++++------------
>>>   1 file changed, 27 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>> index 2708fb4e99..135cd81586 100644
>>> --- a/qapi/qdev.json
>>> +++ b/qapi/qdev.json
>>> @@ -114,16 +114,37 @@
>>>   { 'command': 'device_del', 'data': {'id': 'str'} }
>>>   
>>>   ##
>>> -# @DEVICE_DELETED:
>>> +# @DeviceAndPath:
>>>   #
>>> -# Emitted whenever the device removal completion is acknowledged by the guest.
>>> -# At this point, it's safe to reuse the specified device ID. Device removal can
>>> -# be initiated by the guest or by HMP/QMP commands.
>>> +# In events we designate devices by both their ID (if the device has one)
>>> +# and QOM path.
>>> +#
>>> +# 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.
> 
> Hmm.  I appreciate rationale, but I'm not sure it fits here.  Would
> readers be worse off if we dropped it?

Is there a syntax to add comment to the QAPI structure, which doesn't go into compiled public documentation?

I agree that we don't need this in compiled documentation, but this place in the code really good for the rationale, to avoid starting the discussion from the beginning again.

> 
>>>   #
>>>   # @device: the device's ID if it has one
>>>   #
>>>   # @path: the device's QOM path
>>>   #
>>> +# Since: 8.0
>>> +##
>>> +{ 'struct': 'DeviceAndPath',
>>> +  'data': { '*device': 'str', 'path': 'str' } }
>>> +
>>
>> Should be Since: 8.1 no?
> 
> Yes.
> 
> Please format like
> 
>     ##
>     # @DeviceAndPath:
>     #
>     # In events we designate devices by both their ID (if the device has
>     # one) and QOM path.
>     #
>     # 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.
>     #
>     # @device: the device's ID if it has one
>     #
>     # @path: the device's QOM path
>     #
>     # Since: 8.0
>     ##
> 
> to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
> to conform to current conventions).
> 
>>> +##
>>> +# @DEVICE_DELETED:
>>> +#
>>> +# Emitted whenever the device removal completion is acknowledged by the guest.
>>> +# At this point, it's safe to reuse the specified device ID. Device removal can
>>> +# be initiated by the guest or by HMP/QMP commands.
>>> +#
> 
> Conflict resolution:
> 
>      # Emitted whenever the device removal completion is acknowledged by
>      # the guest.  At this point, it's safe to reuse the specified device
>      # ID. Device removal can be initiated by the guest or by HMP/QMP
>      # commands.
> 
>>>   # Since: 1.5
>>>   #
>>>   # Example:
>>> @@ -134,18 +155,13 @@
>>>   #      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>>>   #
>>>   ##
>>> -{ 'event': 'DEVICE_DELETED',
>>> -  'data': { '*device': 'str', 'path': 'str' } }
>>> +{ 'event': 'DEVICE_DELETED', 'data': 'DeviceAndPath' }
>>>   
>>>   ##
>>>   # @DEVICE_UNPLUG_GUEST_ERROR:
>>>   #
>>>   # 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:
>>> @@ -156,5 +172,4 @@
>>>   #      "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
> 

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

On 19.05.23 18:20, Philippe Mathieu-Daudé wrote:
> Hi Vladimir,
> 
> On 21/4/23 12:32, 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      |  13 ++++
>>   include/hw/hotplug.h   |  12 ++++
>>   include/hw/qdev-core.h |   1 +
>>   include/monitor/qdev.h |   2 +
>>   qapi/qdev.json         | 144 +++++++++++++++++++++++++++++++++++++++++
>>   softmmu/qdev-monitor.c |  41 ++++++++++++
>>   6 files changed, 213 insertions(+)
>>
>> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
>> index 17ac986685..08d6d03760 100644
>> --- a/hw/core/hotplug.c
>> +++ b/hw/core/hotplug.c
>> @@ -57,6 +57,19 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
>>       }
>>   }
>> +HotplugInfo *hotplug_handler_get_hotplug_state(HotplugHandler *plug_handler,
> 
> What about hotplug_handler_get_state()?

OK for me

> 
>> +                                               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..9c43e1263b 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,13 @@ void hotplug_handler_unplug_request(HotplugHandler *plug_handler,
>>   void hotplug_handler_unplug(HotplugHandler *plug_handler,
>>                               DeviceState *plugged_dev,
>>                               Error **errp);
>> +
>> +/**
>> + * hotplug_handler_get_hotplug_state:
>> + *
>> + * Calls #HotplugHandlerClass.get_hotplug_state callback of @plug_handler.
>> + */
>> +HotplugInfo *hotplug_handler_get_hotplug_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 bd50ad5ee1..63889e41c0 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -180,6 +180,7 @@ struct DeviceState {
>>       char *id;
>>       char *canonical_path;
>>       bool realized;
>> +    bool device_on_sent; /* set once by SHPC or PCIE-hotplug */
> 
> This seems to belong to the next patch (not used here).
> Anyhow (besides the field misses its description) from the name
> I can't figure out what this is about. Probably too generic name
> IMHO.

Means "DEVICE_ON event is already sent".

> 
>>       bool pending_deleted_event;
>>       int64_t pending_deleted_expires_ms;
>>       QDict *opts;
>> 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 135cd81586..ffd20c43e0 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -173,3 +173,147 @@
>>   #
>>   ##
>>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
>> +
>> +##
>> +# @LedActivity:
>> +#
>> +# Three-state led indicator state.
>> +#
>> +# @on: Indicator is on.
>> +#
>> +# @blink: Indicator is blinking.
>> +#
>> +# @off: Indicator is off.
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'enum': 'LedActivity',
>> +  'data': [ 'on', 'blink', 'off' ] }
> 
> Possibly useful enough to add in a new qapi/led.json.
> 
>> +##
>> +# @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, the card is
>> +#           fully functional from a hardware standpoint.
>> +#
>> +# @disabled: Slot is disabled, card is safe to be removed.
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'enum': 'HotplugSHPCSlotState',
>> +  'data': [ 'power-only', 'enabled', 'disabled' ] }
>> +
>> +##
>> +# @HotplugBaseState:
>> +#
>> +# Base structure for SHPC and PCIe-native hotplug.
>> +#
>> +# @power-led: Power indicator. When power indicator is on the device is
>> +#             ready and accepted by guest. Off status means that device
>> +#             is safe to remove and blinking is an intermediate state of
>> +#             hot-plug or hot-unplug.
>> +#
>> +# @attention-led: Attention indicator. Off status means normal operation,
>> +#                 On signals about operational problem, Blinking is for
>> +#                 locating the slot.
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'struct': 'HotplugBaseState',
>> +  'data': { '*power-led': 'LedActivity',
>> +            '*attention-led': 'LedActivity' } }
>> +
>> +##
>> +# @HotplugSHPCState:
>> +#
>> +# Standard Hot Plug Controller state.
>> +#
>> +# @slot-state: The slot state field of Slot Status.
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'struct': 'HotplugSHPCState',
>> +  'base': 'HotplugBaseState',
>> +  'data': { '*slot-state': 'HotplugSHPCSlotState' } }
>> +
>> +##
>> +# @HotplugPCIeNativeState:
>> +#
>> +# PCIe Native hotplug slot state.
> 
> Doesn't this belong to qapi/pci.json?

OK, will move.

> 
>> +#
>> +# @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.1
>> +##
>> +{ 'struct': 'HotplugPCIeNativeState',
>> +  'base': 'HotplugBaseState',
>> +  'data': { '*power-on': 'bool' } }
>> +
>> +##
>> +# @HotplugType:
>> +#
>> +# Type of hotplug controller / provider.
>> +#
>> +# @shpc: Standard Hot Plug Controller
>> +#
>> +# @pcie-native: PCIe Native hotplug
> 
> Ditto.
> 
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'enum': 'HotplugType',
>> +  'data': ['shpc', 'pcie-native'] }
>> +
>> +##
>> +# @HotplugInfo:
>> +#
>> +# Generic hotplug slot state.
>> +#
>> +# @type: type of the hotplug (shpc or pcie-native)
>> +#
>> +# @bus: The QOM path of the parent bus where device is hotplugged.
>> +#
>> +# @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.
>> +#
>> +# Single: 8.1
>> +##
>> +{ 'union': 'HotplugInfo',
>> +  'base': { 'type': 'HotplugType',
>> +            'bus': 'DeviceAndPath',
>> +            '*addr': 'str',
>> +            'child': 'DeviceAndPath',
>> +            'device-on': 'bool' },
>> +  'discriminator': 'type',
>> +  'data': { 'shpc': 'HotplugSHPCState',
>> +            'pcie-native': 'HotplugPCIeNativeState' } }
>> +
>> +##
>> +# @query-hotplug:
>> +#
>> +# Query the state of hotplug controller.
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'command': 'query-hotplug',
>> +  'data': { 'id': 'str' },
>> +  'returns': 'HotplugInfo' }
>> +
>> +##
>> +# @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.1
>> +##
>> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }

-- 
Best regards,
Vladimir

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

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 22.05.23 12:27, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>>> On Fri, Apr 21, 2023 at 01:32:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> 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>
>>>
>>> Can QAPI maintainers please review this patchset?
>>> It's been a month.
>> 
>> It's been a busy month; sorry for the delay.
>> 
>>>> ---
>>>>   qapi/qdev.json | 39 +++++++++++++++++++++++++++------------
>>>>   1 file changed, 27 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>>> index 2708fb4e99..135cd81586 100644
>>>> --- a/qapi/qdev.json
>>>> +++ b/qapi/qdev.json
>>>> @@ -114,16 +114,37 @@
>>>>   { 'command': 'device_del', 'data': {'id': 'str'} }
>>>>   
>>>>   ##
>>>> -# @DEVICE_DELETED:
>>>> +# @DeviceAndPath:
>>>>   #
>>>> -# Emitted whenever the device removal completion is acknowledged by the guest.
>>>> -# At this point, it's safe to reuse the specified device ID. Device removal can
>>>> -# be initiated by the guest or by HMP/QMP commands.
>>>> +# In events we designate devices by both their ID (if the device has one)
>>>> +# and QOM path.
>>>> +#
>>>> +# 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.
>> 
>> Hmm.  I appreciate rationale, but I'm not sure it fits here.  Would
>> readers be worse off if we dropped it?
>
> Is there a syntax to add comment to the QAPI structure, which doesn't go into compiled public documentation?

Yes!  qapi-code-gen.rst: "A multi-line comment that starts and ends with
a ``##`` line is a documentation comment."  All other comments are not,
and won't be included in generated documentation.

Example: qapi/qapi-schema.json has

    { 'include': 'pragma.json' }

    # Documentation generated with qapi-gen.py is in source order, with
    # included sub-schemas inserted at the first include directive
    # (subsequent include directives have no effect).  To get a sane and
    # stable order, it's best to include each sub-schema just once, or
    # include it first right here.

    { 'include': 'error.json' }

Not a documentation comment, thus not included in generated
documentation.

Additionally, TODO sections in documentation comments are omitted from
generated documentation.  qapi-code-gen.rst again: "TODO" sections are
not rendered at all (they are for developers, not users of QMP).

> I agree that we don't need this in compiled documentation, but this place in the code really good for the rationale, to avoid starting the discussion from the beginning again.

Saving rationale so we can refer to it later is good.  We tend to use
commit messages for that.  I'd say use comments when the rationale needs
to be more visible.

[...]

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

[Vladimir Sementsov-Ogievskiy]

On 22.05.23 13:47, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> 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.
> 
> Events often need to be paired with a query to let the management
> application resynchronize after a QMP disconnect, say for a restart.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> Do we have libvirt patches?  If not, is anybody working on them?

I'm afraid not [I'm making this for non-libvirt management software]

> If
> not, we need at least interface review by a libvirt developer familiar
> with hot-plug.  And we might want to mark the interface experimental.

experimental sounds fair, will do.

> 
> [...]
> 
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index 135cd81586..ffd20c43e0 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -173,3 +173,147 @@
>>   #
>>   ##
>>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
>> +
>> +##
>> +# @LedActivity:
> 
> Not sure about "Activity".  "Status"?

Status sound better, I agree

> 
>> +#
>> +# Three-state led indicator state.
> 
> Scratch the sentence?
> 
>> +#
>> +# @on: Indicator is on.
>> +#
>> +# @blink: Indicator is blinking.
>> +#
>> +# @off: Indicator is off.
> 
> Suggest "LED is on" ans so forth.
> 
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'enum': 'LedActivity',
>> +  'data': [ 'on', 'blink', 'off' ] }
> 
> Suggest 'blinking'.
> 
>> +
>> +##
>> +# @HotplugSHPCSlotState:
>> +#
>> +# Standard Hot-Plug Controller slot state.
>> +#
>> +# @power-only: Slot is powered on but neither clock nor bus are connected.
> 
> Comma after "on"?
> 
>> +#
>> +# @enabled: Slot is powered on, clock and bus are connected, the card is
>> +#           fully functional from a hardware standpoint.
> 
> Suggest ", and the card"
> 
> Please format like
> 
>     # @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.
> 
> to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
> to conform to current conventions).
> 
>> +#
>> +# @disabled: Slot is disabled, card is safe to be removed.
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'enum': 'HotplugSHPCSlotState',
>> +  'data': [ 'power-only', 'enabled', 'disabled' ] }
>> +
>> +##
>> +# @HotplugBaseState:
>> +#
>> +# Base structure for SHPC and PCIe-native hotplug.
>> +#
>> +# @power-led: Power indicator. When power indicator is on the device is
>> +#             ready and accepted by guest. Off status means that device
>> +#             is safe to remove and blinking is an intermediate state of
>> +#             hot-plug or hot-unplug.
> 
> Suggest something like "When the power LED is on, the device is ...
> When it is off, the device is ...  It is blinking while hot plug or
> unplug is in progress."
> 
>> +#
>> +# @attention-led: Attention indicator. Off status means normal operation,
>> +#                 On signals about operational problem, Blinking is for
>> +#                 locating the slot.
> 
> Suggest something like "The attention LED is normally off.  It is on to
> signal a a problem.  Blinking is for helping users to locate the slot."
> 
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'struct': 'HotplugBaseState',
>> +  'data': { '*power-led': 'LedActivity',
>> +            '*attention-led': 'LedActivity' } }
>> +
>> +##
>> +# @HotplugSHPCState:
>> +#
>> +# Standard Hot Plug Controller state.
>> +#
>> +# @slot-state: The slot state field of Slot Status.
> 
> Color me confused.  What's "Slot Status"?

It's part of SHPC specification. "Slot Status" is a property of slot. It includes "Slot state" ("off" / "on"), which is important for us, and some other things, like "Slot frequency".

> 
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'struct': 'HotplugSHPCState',
>> +  'base': 'HotplugBaseState',
>> +  'data': { '*slot-state': 'HotplugSHPCSlotState' } }
>> +
>> +##
>> +# @HotplugPCIeNativeState:
>> +#
>> +# PCIe Native hotplug slot state.
> 
> hot-plug
> 
> More below, not flagging it there.
> 
>> +#
>> +# @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).
> 
> Please format like
> 
>      # @power-on: PCIe Power Controller Control of Slot Control Register.
>      #     True means Power On (Power Controller Control bit is 0), and
>      #     False means Power Off (Power Controller Control bit is 1).
> 
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'struct': 'HotplugPCIeNativeState',
>> +  'base': 'HotplugBaseState',
>> +  'data': { '*power-on': 'bool' } }
>> +
>> +##
>> +# @HotplugType:
>> +#
>> +# Type of hotplug controller / provider.
>> +#
>> +# @shpc: Standard Hot Plug Controller
> 
> Suggest "PCI Standard Hot-plug Controller", because that's how the spec
> seems to spell it.
> 
>> +#
>> +# @pcie-native: PCIe Native hotplug
> 
> What about non-PCI hot-plug?

Nothing for now, not covered by these patches. (one more point for marking this experimental)

> 
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'enum': 'HotplugType',
>> +  'data': ['shpc', 'pcie-native'] }
>> +
>> +##
>> +# @HotplugInfo:
>> +#
>> +# Generic hotplug slot state.
> 
> Well, the generic part is defined here, but the union type pulls in
> controller-specific parts.  Scratch "generic"?

Agree

> 
>> +#
>> +# @type: type of the hotplug (shpc or pcie-native)
> 
> Repeating enum values in doc comments risks them going stale there.
> Scratch the parenthesis?
> 
>> +#
>> +# @bus: The QOM path of the parent bus where device is hotplugged.
> 
> Is this the bus the device is plugged into?

Yes

> 
> What about bus-less devices?  Hmm, I think we can make an argument that
> the infrastructure required to make hot-plug work will always be part of
> some kind of bus.  Does this make sense to you?

Yes I think we always have some parent device to hot-plug our device into.
It may change in future and then we'll just make the field optional?

> 
>> +#
>> +# @addr: The bus address for hotplugged device if applicable.
> 
> Uh, does this encode structured data in a string?

It should match addr specified in corresponding device_add command. So it's structured a bit.

> 
>> +#
>> +# @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.
>> +#
>> +# Single: 8.1
>> +##
>> +{ 'union': 'HotplugInfo',
>> +  'base': { 'type': 'HotplugType',
>> +            'bus': 'DeviceAndPath',
>> +            '*addr': 'str',
>> +            'child': 'DeviceAndPath',
>> +            'device-on': 'bool' },
>> +  'discriminator': 'type',
>> +  'data': { 'shpc': 'HotplugSHPCState',
>> +            'pcie-native': 'HotplugPCIeNativeState' } }
> 
> You have data common to both types in two places: base of HotplugInfo
> and base of the variant types HotplugBaseState.  Why?

Good question. Don't remember now, will try to simplify for v8

> 
>> +
>> +##
>> +# @query-hotplug:
>> +#
>> +# Query the state of hotplug controller.
>> +#
> 
> @id is undocumented.
> 
>> +# Since: 8.1
>> +##
>> +{ 'command': 'query-hotplug',
>> +  'data': { 'id': 'str' },
>> +  'returns': 'HotplugInfo' }
> 
> I have more questions, but I'd like to understand the meaning of @id
> before I type them up.

It's id of hot-plugged device. Should correspond to @id in device_del command:

   # @id: the device's ID or QOM path

> 
>> +
>> +##
>> +# @DEVICE_ON:
>> +#
>> +# Emitted whenever the device insertion completion is acknowledged by the guest.
>> +# For now only emitted for SHPC and PCIe-native hotplug.
> 
> Please format like
> 
>     # Emitted whenever the device insertion completion is acknowledged by
>     # the guest.  For now only emitted for SHPC and PCIe-native hotplug.
> 
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }
> 
> [...]
> 

Thanks a lot for reviewing!

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

On 22.05.23 15:13, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 22.05.23 12:27, Markus Armbruster wrote:
>>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>>
>>>> On Fri, Apr 21, 2023 at 01:32:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 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>
>>>>
>>>> Can QAPI maintainers please review this patchset?
>>>> It's been a month.
>>>
>>> It's been a busy month; sorry for the delay.
>>>
>>>>> ---
>>>>>    qapi/qdev.json | 39 +++++++++++++++++++++++++++------------
>>>>>    1 file changed, 27 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>>>> index 2708fb4e99..135cd81586 100644
>>>>> --- a/qapi/qdev.json
>>>>> +++ b/qapi/qdev.json
>>>>> @@ -114,16 +114,37 @@
>>>>>    { 'command': 'device_del', 'data': {'id': 'str'} }
>>>>>    
>>>>>    ##
>>>>> -# @DEVICE_DELETED:
>>>>> +# @DeviceAndPath:
>>>>>    #
>>>>> -# Emitted whenever the device removal completion is acknowledged by the guest.
>>>>> -# At this point, it's safe to reuse the specified device ID. Device removal can
>>>>> -# be initiated by the guest or by HMP/QMP commands.
>>>>> +# In events we designate devices by both their ID (if the device has one)
>>>>> +# and QOM path.
>>>>> +#
>>>>> +# 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.
>>>
>>> Hmm.  I appreciate rationale, but I'm not sure it fits here.  Would
>>> readers be worse off if we dropped it?
>>
>> Is there a syntax to add comment to the QAPI structure, which doesn't go into compiled public documentation?
> 
> Yes!  qapi-code-gen.rst: "A multi-line comment that starts and ends with
> a ``##`` line is a documentation comment."  All other comments are not,
> and won't be included in generated documentation.

Good, thanks!

> 
> Example: qapi/qapi-schema.json has
> 
>      { 'include': 'pragma.json' }
> 
>      # Documentation generated with qapi-gen.py is in source order, with
>      # included sub-schemas inserted at the first include directive
>      # (subsequent include directives have no effect).  To get a sane and
>      # stable order, it's best to include each sub-schema just once, or
>      # include it first right here.
> 
>      { 'include': 'error.json' }
> 
> Not a documentation comment, thus not included in generated
> documentation.
> 
> Additionally, TODO sections in documentation comments are omitted from
> generated documentation.  qapi-code-gen.rst again: "TODO" sections are
> not rendered at all (they are for developers, not users of QMP).
> 
>> I agree that we don't need this in compiled documentation, but this place in the code really good for the rationale, to avoid starting the discussion from the beginning again.
> 
> Saving rationale so we can refer to it later is good.  We tend to use
> commit messages for that.  I'd say use comments when the rationale needs
> to be more visible.
> 

Yes I can move this to the commit message. I just wasn't sure that it's an obvious place in our case, as that's not a commit that introduces new structure but just a no-logic-change refactoring.

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

A bit old thread, but I'm going to resend, so should answer here about things I don't want to change. Be free to ignore it and come to review from scratch in v8 (coming soon) if you don't remember, what was it :)

On 19.05.23 18:20, Philippe Mathieu-Daudé wrote:
> Hi Vladimir,
> 
> On 21/4/23 12:32, 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>

[..]

>>   #endif
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index bd50ad5ee1..63889e41c0 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -180,6 +180,7 @@ struct DeviceState {
>>       char *id;
>>       char *canonical_path;
>>       bool realized;
>> +    bool device_on_sent; /* set once by SHPC or PCIE-hotplug */
> 
> This seems to belong to the next patch (not used here).
> Anyhow (besides the field misses its description) from the name
> I can't figure out what this is about. Probably too generic name
> IMHO.

Actually it's used in this patch and forces the event to be sent at most once.
The comment is misleading.

> 
>>       bool pending_deleted_event;
>>       int64_t pending_deleted_expires_ms;
>>       QDict *opts;
>> 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 135cd81586..ffd20c43e0 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -173,3 +173,147 @@
>>   #
>>   ##
>>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
>> +
>> +##
>> +# @LedActivity:
>> +#
>> +# Three-state led indicator state.
>> +#
>> +# @on: Indicator is on.
>> +#
>> +# @blink: Indicator is blinking.
>> +#
>> +# @off: Indicator is off.
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'enum': 'LedActivity',
>> +  'data': [ 'on', 'blink', 'off' ] }
> 
> Possibly useful enough to add in a new qapi/led.json.

I'd postpone it until another user of the enum appear.

> 
>> +##
>> +# @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, the card is
>> +#           fully functional from a hardware standpoint.
>> +#
>> +# @disabled: Slot is disabled, card is safe to be removed.
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'enum': 'HotplugSHPCSlotState',
>> +  'data': [ 'power-only', 'enabled', 'disabled' ] }
>> +
>> +##
>> +# @HotplugBaseState:
>> +#
>> +# Base structure for SHPC and PCIe-native hotplug.
>> +#
>> +# @power-led: Power indicator. When power indicator is on the device is
>> +#             ready and accepted by guest. Off status means that device
>> +#             is safe to remove and blinking is an intermediate state of
>> +#             hot-plug or hot-unplug.
>> +#
>> +# @attention-led: Attention indicator. Off status means normal operation,
>> +#                 On signals about operational problem, Blinking is for
>> +#                 locating the slot.
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'struct': 'HotplugBaseState',
>> +  'data': { '*power-led': 'LedActivity',
>> +            '*attention-led': 'LedActivity' } }
>> +
>> +##
>> +# @HotplugSHPCState:
>> +#
>> +# Standard Hot Plug Controller state.
>> +#
>> +# @slot-state: The slot state field of Slot Status.
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'struct': 'HotplugSHPCState',
>> +  'base': 'HotplugBaseState',
>> +  'data': { '*slot-state': 'HotplugSHPCSlotState' } }
>> +
>> +##
>> +# @HotplugPCIeNativeState:
>> +#
>> +# PCIe Native hotplug slot state.
> 
> Doesn't this belong to qapi/pci.json?

Now I think it shouldn't. I'd keep all hotplug-related together, even when ACPI hotplug will be supported by new event.

Probably it makes sense to make qdev-hotplug.json, but then what to keep in qdev.json? Only device-list-properties command.. Seems that it not worth the split for now.

> 
>> +#
>> +# @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.1
>> +##
>> +{ 'struct': 'HotplugPCIeNativeState',
>> +  'base': 'HotplugBaseState',
>> +  'data': { '*power-on': 'bool' } }
>> +
>> +##
>> +# @HotplugType:
>> +#
>> +# Type of hotplug controller / provider.
>> +#
>> +# @shpc: Standard Hot Plug Controller
>> +#
>> +# @pcie-native: PCIe Native hotplug
> 
> Ditto.
> 
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'enum': 'HotplugType',
>> +  'data': ['shpc', 'pcie-native'] }
>> +
>> +##
>> +# @HotplugInfo:
>> +#
>> +# Generic hotplug slot state.
>> +#
>> +# @type: type of the hotplug (shpc or pcie-native)
>> +#
>> +# @bus: The QOM path of the parent bus where device is hotplugged.
>> +#
>> +# @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.
>> +#
>> +# Single: 8.1
>> +##
>> +{ 'union': 'HotplugInfo',
>> +  'base': { 'type': 'HotplugType',
>> +            'bus': 'DeviceAndPath',
>> +            '*addr': 'str',
>> +            'child': 'DeviceAndPath',
>> +            'device-on': 'bool' },
>> +  'discriminator': 'type',
>> +  'data': { 'shpc': 'HotplugSHPCState',
>> +            'pcie-native': 'HotplugPCIeNativeState' } }
>> +
>> +##
>> +# @query-hotplug:
>> +#
>> +# Query the state of hotplug controller.
>> +#
>> +# Since: 8.1
>> +##
>> +{ 'command': 'query-hotplug',
>> +  'data': { 'id': 'str' },
>> +  'returns': 'HotplugInfo' }
>> +
>> +##
>> +# @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.1
>> +##
>> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }

-- 
Best regards,
Vladimir