[PATCH v5 00/18] pci hotplug tracking

[PATCH v5 00/18] pci hotplug tracking

[Vladimir Sementsov-Ogievskiy]

Hi all!

v5: - don't deprecate IDs and return to ID & QOM scheme
    - split complicated HOTPLUG_STATE patch into several ones

----

The main patches are the last four ones:

- introduce HOTPLUG_STATE event, that inform when hotplug controller
change it's state, especially indicator leds

- query-hotplug command, that provides same information as event on
demand

- DEVICE_ON event - a kind of counterpart for DEVICE_DELETED, signals
when device is finally accepted by guest, power indicator is on and so
on.

That's all for smarter handling of SHPC and PCIe-native hotplug.

If you want to test new events, don't forget
  -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
flag, to disable ACPI hotplug default.

Vladimir Sementsov-Ogievskiy (18):
  pci/shpc: set attention led to OFF on reset
  pci/shpc: change shpc_get_status() return type to uint8_t
  pci/shpc: shpc_slot_command(): handle PWRONLY -> ENABLED transition
  pci/shpc: more generic handle hot-unplug in shpc_slot_command()
  pci/shpc: pass PCIDevice pointer to shpc_slot_command()
  pci/shpc: refactor shpc_device_plug_common()
  pcie: pcie_cap_slot_write_config(): use correct macro
  pcie_regs: drop duplicated indicator value macros
  pcie: drop unused PCIExpressIndicator
  pcie: pcie_cap_slot_enable_power() use correct helper
  pcie: introduce pcie_sltctl_powered_off() helper
  pcie: set power indicator to off on reset by default
  pci: introduce pci_find_the_only_child()
  qapi/qdev.json: unite DEVICE_* event data into single structure
  qapi: add HOTPLUG_STATE infrastructure
  shpc: implement HOTPLUG_STATE event and query-hotplug
  pcie: implement HOTPLUG_STATE event and query-hotplug
  qapi: introduce DEVICE_ON event

 qapi/qdev.json                  | 224 ++++++++++++++++++++++++++++++--
 include/hw/hotplug.h            |  12 ++
 include/hw/pci/pci.h            |   1 +
 include/hw/pci/pci_bridge.h     |   2 +
 include/hw/pci/pcie.h           |  10 +-
 include/hw/pci/pcie_regs.h      |  14 --
 include/hw/pci/shpc.h           |   2 +
 include/monitor/qdev.h          |   7 +
 hw/core/hotplug.c               |  13 ++
 hw/pci-bridge/pci_bridge_dev.c  |  14 ++
 hw/pci-bridge/pcie_pci_bridge.c |   1 +
 hw/pci/pci.c                    |  33 +++++
 hw/pci/pcie.c                   | 122 +++++++++++++++--
 hw/pci/pcie_port.c              |   1 +
 hw/pci/shpc.c                   | 214 ++++++++++++++++++++++--------
 softmmu/qdev-monitor.c          |  67 ++++++++++
 16 files changed, 639 insertions(+), 98 deletions(-)

-- 
2.34.1

[PATCH v5 01/18] pci/shpc: set attention led to OFF on reset

[Vladimir Sementsov-Ogievskiy]

0 is not a valid state for the led. Let's start with OFF.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 hw/pci/shpc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index fca7f6691a..1b3f619dc9 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -223,6 +223,7 @@ void shpc_reset(PCIDevice *d)
                             SHPC_SLOT_STATUS_PRSNT_MASK);
             shpc_set_status(shpc, i, SHPC_LED_OFF, SHPC_SLOT_PWR_LED_MASK);
         }
+        shpc_set_status(shpc, i, SHPC_LED_OFF, SHPC_SLOT_ATTN_LED_MASK);
         shpc_set_status(shpc, i, 0, SHPC_SLOT_STATUS_66);
     }
     shpc_set_sec_bus_speed(shpc, SHPC_SEC_BUS_33);
-- 
2.34.1

[PATCH v5 02/18] pci/shpc: change shpc_get_status() return type to uint8_t

[Vladimir Sementsov-Ogievskiy]

The result of the function is always one byte. The result is always
assigned to uint8_t variable. Also, shpc_get_status() should be
symmetric to shpc_set_status() which has uint8_t value argument.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 hw/pci/shpc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 1b3f619dc9..5d71569b13 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -123,10 +123,13 @@
 #define SHPC_PCI_TO_IDX(pci_slot) ((pci_slot) - 1)
 #define SHPC_IDX_TO_PHYSICAL(slot) ((slot) + 1)
 
-static uint16_t shpc_get_status(SHPCDevice *shpc, int slot, uint16_t msk)
+static uint8_t shpc_get_status(SHPCDevice *shpc, int slot, uint16_t msk)
 {
     uint8_t *status = shpc->config + SHPC_SLOT_STATUS(slot);
-    return (pci_get_word(status) & msk) >> ctz32(msk);
+    uint16_t result = (pci_get_word(status) & msk) >> ctz32(msk);
+
+    assert(result <= UINT8_MAX);
+    return result;
 }
 
 static void shpc_set_status(SHPCDevice *shpc,
-- 
2.34.1

[PATCH v5 03/18] pci/shpc: shpc_slot_command(): handle PWRONLY -> ENABLED transition

[Vladimir Sementsov-Ogievskiy]

ENABLED -> PWRONLY transition is not allowed and we handle it by
shpc_invalid_command(). But PWRONLY -> ENABLED transition is silently
ignored, which seems wrong. Let's handle it as correct.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 hw/pci/shpc.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 5d71569b13..25e4172382 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -273,28 +273,22 @@ static void shpc_slot_command(SHPCDevice *shpc, uint8_t target,
         return;
     }
 
-    switch (power) {
-    case SHPC_LED_NO:
-        break;
-    default:
+    if (power != SHPC_LED_NO) {
         /* TODO: send event to monitor */
         shpc_set_status(shpc, slot, power, SHPC_SLOT_PWR_LED_MASK);
     }
-    switch (attn) {
-    case SHPC_LED_NO:
-        break;
-    default:
+    if (attn != SHPC_LED_NO) {
         /* TODO: send event to monitor */
         shpc_set_status(shpc, slot, attn, SHPC_SLOT_ATTN_LED_MASK);
     }
-
-    if ((current_state == SHPC_STATE_DISABLED && state == SHPC_STATE_PWRONLY) ||
-        (current_state == SHPC_STATE_DISABLED && state == SHPC_STATE_ENABLED)) {
-        shpc_set_status(shpc, slot, state, SHPC_SLOT_STATE_MASK);
-    } else if ((current_state == SHPC_STATE_ENABLED ||
-                current_state == SHPC_STATE_PWRONLY) &&
-               state == SHPC_STATE_DISABLED) {
+    if (state != SHPC_STATE_NO) {
         shpc_set_status(shpc, slot, state, SHPC_SLOT_STATE_MASK);
+    }
+
+    if ((current_state == SHPC_STATE_ENABLED ||
+         current_state == SHPC_STATE_PWRONLY) &&
+        state == SHPC_STATE_DISABLED)
+    {
         power = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK);
         /* TODO: track what monitor requested. */
         /* Look at LED to figure out whether it's ok to remove the device. */
-- 
2.34.1

[PATCH v5 04/18] pci/shpc: more generic handle hot-unplug in shpc_slot_command()

[Vladimir Sementsov-Ogievskiy]

Free slot if both conditions (power-led = OFF and state = DISABLED)
becomes true regardless of the sequence. It is similar to how PCIe
hotplug works.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 hw/pci/shpc.c | 52 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 25e4172382..959dc470f3 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -258,49 +258,59 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
     }
 }
 
+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 void shpc_slot_command(SHPCDevice *shpc, uint8_t target,
                               uint8_t state, uint8_t power, uint8_t attn)
 {
-    uint8_t current_state;
     int slot = SHPC_LOGICAL_TO_IDX(target);
+    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);
+
     if (target < SHPC_CMD_TRGT_MIN || slot >= shpc->nslots) {
         shpc_invalid_command(shpc);
         return;
     }
-    current_state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK);
-    if (current_state == SHPC_STATE_ENABLED && state == SHPC_STATE_PWRONLY) {
+
+    if (old_state == SHPC_STATE_ENABLED && state == SHPC_STATE_PWRONLY) {
         shpc_invalid_command(shpc);
         return;
     }
 
-    if (power != SHPC_LED_NO) {
+    if (power == SHPC_LED_NO) {
+        power = old_power;
+    } else {
         /* TODO: send event to monitor */
         shpc_set_status(shpc, slot, power, SHPC_SLOT_PWR_LED_MASK);
     }
-    if (attn != SHPC_LED_NO) {
+
+    if (attn == SHPC_LED_NO) {
+        attn = old_attn;
+    } else {
         /* TODO: send event to monitor */
         shpc_set_status(shpc, slot, attn, SHPC_SLOT_ATTN_LED_MASK);
     }
-    if (state != SHPC_STATE_NO) {
+
+    if (state == SHPC_STATE_NO) {
+        state = old_state;
+    } else {
         shpc_set_status(shpc, slot, state, SHPC_SLOT_STATE_MASK);
     }
 
-    if ((current_state == SHPC_STATE_ENABLED ||
-         current_state == SHPC_STATE_PWRONLY) &&
-        state == SHPC_STATE_DISABLED)
+    if (!shpc_slot_is_off(old_state, old_power, old_attn) &&
+        shpc_slot_is_off(state, power, attn))
     {
-        power = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK);
-        /* TODO: track what monitor requested. */
-        /* Look at LED to figure out whether it's ok to remove the device. */
-        if (power == SHPC_LED_OFF) {
-            shpc_free_devices_in_slot(shpc, slot);
-            shpc_set_status(shpc, slot, 1, SHPC_SLOT_STATUS_MRL_OPEN);
-            shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_EMPTY,
-                            SHPC_SLOT_STATUS_PRSNT_MASK);
-            shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
-                SHPC_SLOT_EVENT_MRL |
-                SHPC_SLOT_EVENT_PRESENCE;
-        }
+        shpc_free_devices_in_slot(shpc, slot);
+        shpc_set_status(shpc, slot, 1, SHPC_SLOT_STATUS_MRL_OPEN);
+        shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_EMPTY,
+                        SHPC_SLOT_STATUS_PRSNT_MASK);
+        shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |=
+            SHPC_SLOT_EVENT_MRL |
+            SHPC_SLOT_EVENT_PRESENCE;
     }
 }
 
-- 
2.34.1

[PATCH v5 05/18] pci/shpc: pass PCIDevice pointer to shpc_slot_command()

[Vladimir Sementsov-Ogievskiy]

We'll need it in further patch to report bridge in QAPI event.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 hw/pci/shpc.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 959dc470f3..9f964b1d70 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -263,9 +263,10 @@ 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 void shpc_slot_command(SHPCDevice *shpc, uint8_t target,
+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);
     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);
@@ -314,8 +315,9 @@ static void shpc_slot_command(SHPCDevice *shpc, uint8_t target,
     }
 }
 
-static void shpc_command(SHPCDevice *shpc)
+static void shpc_command(PCIDevice *d)
 {
+    SHPCDevice *shpc = d->shpc;
     uint8_t code = pci_get_byte(shpc->config + SHPC_CMD_CODE);
     uint8_t speed;
     uint8_t target;
@@ -336,7 +338,7 @@ static void shpc_command(SHPCDevice *shpc)
         state = (code & SHPC_SLOT_STATE_MASK) >> SHPC_SLOT_STATE_SHIFT;
         power = (code & SHPC_SLOT_PWR_LED_MASK) >> SHPC_SLOT_PWR_LED_SHIFT;
         attn = (code & SHPC_SLOT_ATTN_LED_MASK) >> SHPC_SLOT_ATTN_LED_SHIFT;
-        shpc_slot_command(shpc, target, state, power, attn);
+        shpc_slot_command(d, target, state, power, attn);
         break;
     case 0x40 ... 0x47:
         speed = code & SHPC_SEC_BUS_MASK;
@@ -354,10 +356,10 @@ static void shpc_command(SHPCDevice *shpc)
         }
         for (i = 0; i < shpc->nslots; ++i) {
             if (!(shpc_get_status(shpc, i, SHPC_SLOT_STATUS_MRL_OPEN))) {
-                shpc_slot_command(shpc, i + SHPC_CMD_TRGT_MIN,
+                shpc_slot_command(d, i + SHPC_CMD_TRGT_MIN,
                                   SHPC_STATE_PWRONLY, SHPC_LED_ON, SHPC_LED_NO);
             } else {
-                shpc_slot_command(shpc, i + SHPC_CMD_TRGT_MIN,
+                shpc_slot_command(d, i + SHPC_CMD_TRGT_MIN,
                                   SHPC_STATE_NO, SHPC_LED_OFF, SHPC_LED_NO);
             }
         }
@@ -375,10 +377,10 @@ static void shpc_command(SHPCDevice *shpc)
         }
         for (i = 0; i < shpc->nslots; ++i) {
             if (!(shpc_get_status(shpc, i, SHPC_SLOT_STATUS_MRL_OPEN))) {
-                shpc_slot_command(shpc, i + SHPC_CMD_TRGT_MIN,
+                shpc_slot_command(d, i + SHPC_CMD_TRGT_MIN,
                                   SHPC_STATE_ENABLED, SHPC_LED_ON, SHPC_LED_NO);
             } else {
-                shpc_slot_command(shpc, i + SHPC_CMD_TRGT_MIN,
+                shpc_slot_command(d, i + SHPC_CMD_TRGT_MIN,
                                   SHPC_STATE_NO, SHPC_LED_OFF, SHPC_LED_NO);
             }
         }
@@ -410,7 +412,7 @@ static void shpc_write(PCIDevice *d, unsigned addr, uint64_t val, int l)
         shpc->config[a] &= ~(val & w1cmask); /* W1C: Write 1 to Clear */
     }
     if (ranges_overlap(addr, l, SHPC_CMD_CODE, 2)) {
-        shpc_command(shpc);
+        shpc_command(d);
     }
     shpc_interrupt_update(d);
 }
-- 
2.34.1

[PATCH v5 06/18] pci/shpc: refactor shpc_device_plug_common()

[Vladimir Sementsov-Ogievskiy]

Rename it to shpc_device_get_slot(), to mention what it does rather
than how it is used. It also helps to reuse it in further commit.

Also, add a return value and get rid of local_err.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 hw/pci/shpc.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 9f964b1d70..e7bc7192f1 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -496,8 +496,9 @@ static const MemoryRegionOps shpc_mmio_ops = {
         .max_access_size = 4,
     },
 };
-static void shpc_device_plug_common(PCIDevice *affected_dev, int *slot,
-                                    SHPCDevice *shpc, Error **errp)
+
+static bool shpc_device_get_slot(PCIDevice *affected_dev, int *slot,
+                                 SHPCDevice *shpc, Error **errp)
 {
     int pci_slot = PCI_SLOT(affected_dev->devfn);
     *slot = SHPC_PCI_TO_IDX(pci_slot);
@@ -507,21 +508,20 @@ static void shpc_device_plug_common(PCIDevice *affected_dev, int *slot,
                    "controller. Valid slots are between %d and %d.",
                    pci_slot, SHPC_IDX_TO_PCI(0),
                    SHPC_IDX_TO_PCI(shpc->nslots) - 1);
-        return;
+        return false;
     }
+
+    return true;
 }
 
 void shpc_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                             Error **errp)
 {
-    Error *local_err = NULL;
     PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
     SHPCDevice *shpc = pci_hotplug_dev->shpc;
     int slot;
 
-    shpc_device_plug_common(PCI_DEVICE(dev), &slot, shpc, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!shpc_device_get_slot(PCI_DEVICE(dev), &slot, shpc, errp)) {
         return;
     }
 
@@ -563,16 +563,13 @@ void shpc_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
 void shpc_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                                    DeviceState *dev, Error **errp)
 {
-    Error *local_err = NULL;
     PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
     SHPCDevice *shpc = pci_hotplug_dev->shpc;
     uint8_t state;
     uint8_t led;
     int slot;
 
-    shpc_device_plug_common(PCI_DEVICE(dev), &slot, shpc, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!shpc_device_get_slot(PCI_DEVICE(dev), &slot, shpc, errp)) {
         return;
     }
 
-- 
2.34.1

[PATCH v5 07/18] pcie: pcie_cap_slot_write_config(): use correct macro

[Vladimir Sementsov-Ogievskiy]

PCI_EXP_SLTCTL_PIC_OFF is a value, and PCI_EXP_SLTCTL_PIC is a mask.
Happily PCI_EXP_SLTCTL_PIC_OFF is a maximum value for this mask and is
equal to the mask itself. Still the code looks like a bug. Let's make
it more reader-friendly.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 hw/pci/pcie.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 924fdabd15..82ef723983 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -770,9 +770,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
      * control of powered off slots before powering them on.
      */
     if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) &&
-        (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF &&
+        (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PIC_OFF &&
         (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) ||
-        (old_slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) {
+        (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PIC_OFF)) {
         pcie_cap_slot_do_unplug(dev);
     }
     pcie_cap_update_power(dev);
-- 
2.34.1

[PATCH v5 08/18] pcie_regs: drop duplicated indicator value macros

[Vladimir Sementsov-Ogievskiy]

We already have indicator values in
include/standard-headers/linux/pci_regs.h , no reason to reinvent them
in include/hw/pci/pcie_regs.h. (and we already have usage of
PCI_EXP_SLTCTL_PWR_IND_BLINK and PCI_EXP_SLTCTL_PWR_IND_OFF in
hw/pci/pcie.c, so let's be consistent)

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 include/hw/pci/pcie_regs.h |  9 ---------
 hw/pci/pcie.c              | 13 +++++++------
 2 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index 963dc2e170..00b595a82e 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -70,15 +70,6 @@ typedef enum PCIExpLinkWidth {
 #define PCI_EXP_SLTCTL_IND_ON           0x1
 #define PCI_EXP_SLTCTL_IND_BLINK        0x2
 #define PCI_EXP_SLTCTL_IND_OFF          0x3
-#define PCI_EXP_SLTCTL_AIC_SHIFT        ctz32(PCI_EXP_SLTCTL_AIC)
-#define PCI_EXP_SLTCTL_AIC_OFF                          \
-    (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_AIC_SHIFT)
-
-#define PCI_EXP_SLTCTL_PIC_SHIFT        ctz32(PCI_EXP_SLTCTL_PIC)
-#define PCI_EXP_SLTCTL_PIC_OFF                          \
-    (PCI_EXP_SLTCTL_IND_OFF << PCI_EXP_SLTCTL_PIC_SHIFT)
-#define PCI_EXP_SLTCTL_PIC_ON                          \
-    (PCI_EXP_SLTCTL_IND_ON << PCI_EXP_SLTCTL_PIC_SHIFT)
 
 #define PCI_EXP_SLTCTL_SUPPORTED        \
             (PCI_EXP_SLTCTL_ABPE |      \
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 82ef723983..ccdb2377e1 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -634,8 +634,8 @@ void pcie_cap_slot_init(PCIDevice *dev, PCIESlot *s)
                                  PCI_EXP_SLTCTL_PIC |
                                  PCI_EXP_SLTCTL_AIC);
     pci_word_test_and_set_mask(dev->config + pos + PCI_EXP_SLTCTL,
-                               PCI_EXP_SLTCTL_PIC_OFF |
-                               PCI_EXP_SLTCTL_AIC_OFF);
+                               PCI_EXP_SLTCTL_PWR_IND_OFF |
+                               PCI_EXP_SLTCTL_ATTN_IND_OFF);
     pci_word_test_and_set_mask(dev->wmask + pos + PCI_EXP_SLTCTL,
                                PCI_EXP_SLTCTL_PIC |
                                PCI_EXP_SLTCTL_AIC |
@@ -679,7 +679,7 @@ void pcie_cap_slot_reset(PCIDevice *dev)
                                  PCI_EXP_SLTCTL_PDCE |
                                  PCI_EXP_SLTCTL_ABPE);
     pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL,
-                               PCI_EXP_SLTCTL_AIC_OFF);
+                               PCI_EXP_SLTCTL_ATTN_IND_OFF);
 
     if (dev->cap_present & QEMU_PCIE_SLTCAP_PCP) {
         /* Downstream ports enforce device number 0. */
@@ -694,7 +694,8 @@ void pcie_cap_slot_reset(PCIDevice *dev)
                                        PCI_EXP_SLTCTL_PCC);
         }
 
-        pic = populated ? PCI_EXP_SLTCTL_PIC_ON : PCI_EXP_SLTCTL_PIC_OFF;
+        pic = populated ?
+                PCI_EXP_SLTCTL_PWR_IND_ON : PCI_EXP_SLTCTL_PWR_IND_OFF;
         pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL, pic);
     }
 
@@ -770,9 +771,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
      * control of powered off slots before powering them on.
      */
     if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) &&
-        (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PIC_OFF &&
+        (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF &&
         (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) ||
-        (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PIC_OFF)) {
+        (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PWR_IND_OFF)) {
         pcie_cap_slot_do_unplug(dev);
     }
     pcie_cap_update_power(dev);
-- 
2.34.1

[PATCH v5 09/18] pcie: drop unused PCIExpressIndicator

[Vladimir Sementsov-Ogievskiy]

The structure type is unused. Also, it's the only user of corresponding
macros, so drop them too.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 include/hw/pci/pcie.h      | 8 --------
 include/hw/pci/pcie_regs.h | 5 -----
 2 files changed, 13 deletions(-)

diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 798a262a0a..3cc2b15957 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -27,14 +27,6 @@
 #include "hw/pci/pcie_sriov.h"
 #include "hw/hotplug.h"
 
-typedef enum {
-    /* for attention and power indicator */
-    PCI_EXP_HP_IND_RESERVED     = PCI_EXP_SLTCTL_IND_RESERVED,
-    PCI_EXP_HP_IND_ON           = PCI_EXP_SLTCTL_IND_ON,
-    PCI_EXP_HP_IND_BLINK        = PCI_EXP_SLTCTL_IND_BLINK,
-    PCI_EXP_HP_IND_OFF          = PCI_EXP_SLTCTL_IND_OFF,
-} PCIExpressIndicator;
-
 typedef enum {
     /* these bits must match the bits in Slot Control/Status registers.
      * PCI_EXP_HP_EV_xxx = PCI_EXP_SLTCTL_xxxE = PCI_EXP_SLTSTA_xxx
diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index 00b595a82e..1fe0bdd25b 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -66,11 +66,6 @@ typedef enum PCIExpLinkWidth {
 
 #define PCI_EXP_SLTCAP_PSN_SHIFT        ctz32(PCI_EXP_SLTCAP_PSN)
 
-#define PCI_EXP_SLTCTL_IND_RESERVED     0x0
-#define PCI_EXP_SLTCTL_IND_ON           0x1
-#define PCI_EXP_SLTCTL_IND_BLINK        0x2
-#define PCI_EXP_SLTCTL_IND_OFF          0x3
-
 #define PCI_EXP_SLTCTL_SUPPORTED        \
             (PCI_EXP_SLTCTL_ABPE |      \
              PCI_EXP_SLTCTL_PDCE |      \
-- 
2.34.1

[PATCH v5 10/18] pcie: pcie_cap_slot_enable_power() use correct helper

[Vladimir Sementsov-Ogievskiy]

*_by_mask() helpers shouldn't be used here (and that's the only one).
*_by_mask() helpers do shift their value argument, but in pcie.c code
we use values that are already shifted appropriately.
Happily, PCI_EXP_SLTCTL_PWR_ON is zero, so shift doesn't matter. But if
we apply same helper for PCI_EXP_SLTCTL_PWR_OFF constant it will do
wrong thing.

So, let's use instead pci_word_test_and_clear_mask() which is already
used in the file to clear PCI_EXP_SLTCTL_PWR_OFF bit in
pcie_cap_slot_init() and pcie_cap_slot_reset().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 hw/pci/pcie.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index ccdb2377e1..db8360226f 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -373,8 +373,8 @@ void pcie_cap_slot_enable_power(PCIDevice *dev)
     uint32_t sltcap = pci_get_long(exp_cap + PCI_EXP_SLTCAP);
 
     if (sltcap & PCI_EXP_SLTCAP_PCP) {
-        pci_set_word_by_mask(exp_cap + PCI_EXP_SLTCTL,
-                             PCI_EXP_SLTCTL_PCC, PCI_EXP_SLTCTL_PWR_ON);
+        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_SLTCTL,
+                                     PCI_EXP_SLTCTL_PCC);
     }
 }
 
-- 
2.34.1

[PATCH v5 11/18] pcie: introduce pcie_sltctl_powered_off() helper

[Vladimir Sementsov-Ogievskiy]

In pcie_cap_slot_write_config() we check for PCI_EXP_SLTCTL_PWR_OFF in
a bad form. We should distinguish PCI_EXP_SLTCTL_PWR which is a "mask"
and PCI_EXP_SLTCTL_PWR_OFF which is value for that mask.

Better code is in pcie_cap_slot_unplug_request_cb() and in
pcie_cap_update_power(). Let's use same pattern everywhere. To simplify
things add also a helper.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 hw/pci/pcie.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index db8360226f..90faf0710a 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -39,6 +39,11 @@
 #define PCIE_DEV_PRINTF(dev, fmt, ...)                                  \
     PCIE_DPRINTF("%s:%x "fmt, (dev)->name, (dev)->devfn, ## __VA_ARGS__)
 
+static bool pcie_sltctl_powered_off(uint16_t sltctl)
+{
+    return (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF
+        && (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF;
+}
 
 /***************************************************************************
  * pci express capability helper functions
@@ -395,6 +400,7 @@ static void pcie_cap_update_power(PCIDevice *hotplug_dev)
 
     if (sltcap & PCI_EXP_SLTCAP_PCP) {
         power = (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_ON;
+        /* Don't we need to check also (sltctl & PCI_EXP_SLTCTL_PIC) ? */
     }
 
     pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
@@ -579,8 +585,7 @@ void pcie_cap_slot_unplug_request_cb(HotplugHandler *hotplug_dev,
         return;
     }
 
-    if (((sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF) &&
-        ((sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF)) {
+    if (pcie_sltctl_powered_off(sltctl)) {
         /* slot is powered off -> unplug without round-trip to the guest */
         pcie_cap_slot_do_unplug(hotplug_pdev);
         hotplug_event_notify(hotplug_pdev);
@@ -770,10 +775,9 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
      * this is a work around for guests that overwrite
      * control of powered off slots before powering them on.
      */
-    if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) &&
-        (val & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF &&
-        (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) ||
-        (old_slt_ctl & PCI_EXP_SLTCTL_PIC) != PCI_EXP_SLTCTL_PWR_IND_OFF)) {
+    if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_off(val) &&
+        !pcie_sltctl_powered_off(old_slt_ctl))
+    {
         pcie_cap_slot_do_unplug(dev);
     }
     pcie_cap_update_power(dev);
-- 
2.34.1

[PATCH v5 12/18] pcie: set power indicator to off on reset by default

[Vladimir Sementsov-Ogievskiy]

It should be zero, the only valid values are ON, OFF and BLINK.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 hw/pci/pcie.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 90faf0710a..b8c24cf45f 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -684,6 +684,7 @@ void pcie_cap_slot_reset(PCIDevice *dev)
                                  PCI_EXP_SLTCTL_PDCE |
                                  PCI_EXP_SLTCTL_ABPE);
     pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTCTL,
+                               PCI_EXP_SLTCTL_PWR_IND_OFF |
                                PCI_EXP_SLTCTL_ATTN_IND_OFF);
 
     if (dev->cap_present & QEMU_PCIE_SLTCAP_PCP) {
-- 
2.34.1

[PATCH v5 13/18] pci: introduce pci_find_the_only_child()

[Vladimir Sementsov-Ogievskiy]

To be used in further patch to identify the device hot-plugged into
pcie-root-port.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>
---
 include/hw/pci/pci.h |  1 +
 hw/pci/pci.c         | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index d5a40cd058..b6c9c44527 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -341,6 +341,7 @@ void pci_for_each_device_under_bus_reverse(PCIBus *bus,
 void pci_for_each_bus_depth_first(PCIBus *bus, pci_bus_ret_fn begin,
                                   pci_bus_fn end, void *parent_state);
 PCIDevice *pci_get_function_0(PCIDevice *pci_dev);
+PCIDevice *pci_find_the_only_child(PCIBus *bus, int bus_num, Error **errp);
 
 /* Use this wrapper when specific scan order is not required. */
 static inline
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 208c16f450..34fd1fb5b8 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1771,6 +1771,39 @@ void pci_for_each_device(PCIBus *bus, int bus_num,
     }
 }
 
+typedef struct TheOnlyChild {
+    PCIDevice *dev;
+    int count;
+} TheOnlyChild;
+
+static void the_only_child_fn(PCIBus *bus, PCIDevice *dev, void *opaque)
+{
+    TheOnlyChild *s = opaque;
+
+    s->dev = dev;
+    s->count++;
+}
+
+PCIDevice *pci_find_the_only_child(PCIBus *bus, int bus_num, Error **errp)
+{
+    TheOnlyChild res = {0};
+
+    pci_for_each_device(bus, bus_num, the_only_child_fn, &res);
+
+    if (!res.dev) {
+        assert(res.count == 0);
+        error_setg(errp, "No child devices found");
+        return NULL;
+    }
+
+    if (res.count > 1) {
+        error_setg(errp, "Several child devices found");
+        return NULL;
+    }
+
+    return res.dev;
+}
+
 const pci_class_desc *get_class_desc(int class)
 {
     const pci_class_desc *desc;
-- 
2.34.1

Re: [PATCH v5 13/18] pci: introduce pci_find_the_only_child()

[Michael S. Tsirkin]

On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> To be used in further patch to identify the device hot-plugged into
> pcie-root-port.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>

Wait a second does this work for multifunction devices correctly?

> ---
>  include/hw/pci/pci.h |  1 +
>  hw/pci/pci.c         | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index d5a40cd058..b6c9c44527 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -341,6 +341,7 @@ void pci_for_each_device_under_bus_reverse(PCIBus *bus,
>  void pci_for_each_bus_depth_first(PCIBus *bus, pci_bus_ret_fn begin,
>                                    pci_bus_fn end, void *parent_state);
>  PCIDevice *pci_get_function_0(PCIDevice *pci_dev);
> +PCIDevice *pci_find_the_only_child(PCIBus *bus, int bus_num, Error **errp);
>  
>  /* Use this wrapper when specific scan order is not required. */
>  static inline
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 208c16f450..34fd1fb5b8 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1771,6 +1771,39 @@ void pci_for_each_device(PCIBus *bus, int bus_num,
>      }
>  }
>  
> +typedef struct TheOnlyChild {
> +    PCIDevice *dev;
> +    int count;
> +} TheOnlyChild;
> +
> +static void the_only_child_fn(PCIBus *bus, PCIDevice *dev, void *opaque)
> +{
> +    TheOnlyChild *s = opaque;
> +
> +    s->dev = dev;
> +    s->count++;
> +}
> +
> +PCIDevice *pci_find_the_only_child(PCIBus *bus, int bus_num, Error **errp)
> +{
> +    TheOnlyChild res = {0};
> +
> +    pci_for_each_device(bus, bus_num, the_only_child_fn, &res);
> +
> +    if (!res.dev) {
> +        assert(res.count == 0);
> +        error_setg(errp, "No child devices found");
> +        return NULL;
> +    }
> +
> +    if (res.count > 1) {
> +        error_setg(errp, "Several child devices found");
> +        return NULL;
> +    }
> +
> +    return res.dev;
> +}
> +
>  const pci_class_desc *get_class_desc(int class)
>  {
>      const pci_class_desc *desc;
> -- 
> 2.34.1

Re: [PATCH v5 13/18] pci: introduce pci_find_the_only_child()

[Vladimir Sementsov-Ogievskiy]

On 02.03.23 00:09, Michael S. Tsirkin wrote:
> On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> To be used in further patch to identify the device hot-plugged into
>> pcie-root-port.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
>> Reviewed-by: Anton Kuchin<antonkuchin@yandex-team.ru>
> Wait a second does this work for multifunction devices correctly?
> 

I thought about that and I'm just lost:)

Could several (multifunction?) devices be plugged into one pcie-root-port device?

Same question for SHPC slots. For example, shpc_free_devices_in_slot() looks like we can have several devices in one slot..
On the other hand, in shpc_reset() we have construction shpc->sec_bus->devices[PCI_DEVFN(SHPC_IDX_TO_PCI(i), 0)] to access the device in slot. The only one device.

-- 
Best regards,
Vladimir

Re: [PATCH v5 13/18] pci: introduce pci_find_the_only_child()

[Michael S. Tsirkin]

On Thu, Mar 02, 2023 at 11:28:44AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 02.03.23 00:09, Michael S. Tsirkin wrote:
> > On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > To be used in further patch to identify the device hot-plugged into
> > > pcie-root-port.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
> > > Reviewed-by: Anton Kuchin<antonkuchin@yandex-team.ru>
> > Wait a second does this work for multifunction devices correctly?
> > 
> 
> I thought about that and I'm just lost:)
> 
> Could several (multifunction?) devices be plugged into one pcie-root-port device?

One device per port but one multifunction device is represented as multiple PCIDevice structures.

> Same question for SHPC slots. For example, shpc_free_devices_in_slot() looks like we can have several devices in one slot..
> On the other hand, in shpc_reset() we have construction shpc->sec_bus->devices[PCI_DEVFN(SHPC_IDX_TO_PCI(i), 0)] to access the device in slot. The only one device.
> 
> -- 
> Best regards,
> Vladimir

Same thing.

... and let's not get started about sriov and ari ...

-- 
MST

Re: [PATCH v5 13/18] pci: introduce pci_find_the_only_child()

[Vladimir Sementsov-Ogievskiy]

On 02.03.23 11:37, Michael S. Tsirkin wrote:
> On Thu, Mar 02, 2023 at 11:28:44AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 02.03.23 00:09, Michael S. Tsirkin wrote:
>>> On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> To be used in further patch to identify the device hot-plugged into
>>>> pcie-root-port.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
>>>> Reviewed-by: Anton Kuchin<antonkuchin@yandex-team.ru>
>>> Wait a second does this work for multifunction devices correctly?
>>>
>>
>> I thought about that and I'm just lost:)
>>
>> Could several (multifunction?) devices be plugged into one pcie-root-port device?
> 
> One device per port but one multifunction device is represented as multiple PCIDevice structures.

So, it should be OK to send _one_ event for that multifunction device, and the code is shpc_reset() is OK, but reporting "Several child devices found" is bad idea?

So, if I change the logic from pci_find_the_only_child() to pci_find_first_child() is it OK?

> 
>> Same question for SHPC slots. For example, shpc_free_devices_in_slot() looks like we can have several devices in one slot..
>> On the other hand, in shpc_reset() we have construction shpc->sec_bus->devices[PCI_DEVFN(SHPC_IDX_TO_PCI(i), 0)] to access the device in slot. The only one device.
>>
>> -- 
>> Best regards,
>> Vladimir
> 
> Same thing.
> 
> ... and let's not get started about sriov and ari ...
> 

-- 
Best regards,
Vladimir

Re: [PATCH v5 13/18] pci: introduce pci_find_the_only_child()

[Michael S. Tsirkin]

On Thu, Mar 02, 2023 at 11:45:00AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 02.03.23 11:37, Michael S. Tsirkin wrote:
> > On Thu, Mar 02, 2023 at 11:28:44AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > On 02.03.23 00:09, Michael S. Tsirkin wrote:
> > > > On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > > To be used in further patch to identify the device hot-plugged into
> > > > > pcie-root-port.
> > > > > 
> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
> > > > > Reviewed-by: Anton Kuchin<antonkuchin@yandex-team.ru>
> > > > Wait a second does this work for multifunction devices correctly?
> > > > 
> > > 
> > > I thought about that and I'm just lost:)
> > > 
> > > Could several (multifunction?) devices be plugged into one pcie-root-port device?
> > 
> > One device per port but one multifunction device is represented as multiple PCIDevice structures.
> 
> So, it should be OK to send _one_ event for that multifunction device, and the code is shpc_reset() is OK, but reporting "Several child devices found" is bad idea?

I don't know about your new event, we are discussing it separately.
yes all functions are removed together normally on real hardware.

> So, if I change the logic from pci_find_the_only_child() to pci_find_first_child() is it OK?

Yes though I don't like this name either - need to make it clear that
multifunction is ok, multiple unrelated devices aren't.


> > 
> > > Same question for SHPC slots. For example, shpc_free_devices_in_slot() looks like we can have several devices in one slot..
> > > On the other hand, in shpc_reset() we have construction shpc->sec_bus->devices[PCI_DEVFN(SHPC_IDX_TO_PCI(i), 0)] to access the device in slot. The only one device.
> > > 
> > > -- 
> > > Best regards,
> > > Vladimir
> > 
> > Same thing.
> > 
> > ... and let's not get started about sriov and ari ...
> > 
> 
> -- 
> Best regards,
> Vladimir

Re: [PATCH v5 13/18] pci: introduce pci_find_the_only_child()

[Vladimir Sementsov-Ogievskiy]

On 02.03.23 11:53, Michael S. Tsirkin wrote:
> On Thu, Mar 02, 2023 at 11:45:00AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 02.03.23 11:37, Michael S. Tsirkin wrote:
>>> On Thu, Mar 02, 2023 at 11:28:44AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> On 02.03.23 00:09, Michael S. Tsirkin wrote:
>>>>> On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> To be used in further patch to identify the device hot-plugged into
>>>>>> pcie-root-port.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
>>>>>> Reviewed-by: Anton Kuchin<antonkuchin@yandex-team.ru>
>>>>> Wait a second does this work for multifunction devices correctly?
>>>>>
>>>>
>>>> I thought about that and I'm just lost:)
>>>>
>>>> Could several (multifunction?) devices be plugged into one pcie-root-port device?
>>>
>>> One device per port but one multifunction device is represented as multiple PCIDevice structures.
>>
>> So, it should be OK to send _one_ event for that multifunction device, and the code is shpc_reset() is OK, but reporting "Several child devices found" is bad idea?
> 
> I don't know about your new event, we are discussing it separately.
> yes all functions are removed together normally on real hardware.
> 
>> So, if I change the logic from pci_find_the_only_child() to pci_find_first_child() is it OK?
> 
> Yes though I don't like this name either - need to make it clear that
> multifunction is ok, multiple unrelated devices aren't.

Could we check it somehow that all plugged devices represent the one multifunction device?

> 
> 
>>>
>>>> Same question for SHPC slots. For example, shpc_free_devices_in_slot() looks like we can have several devices in one slot..
>>>> On the other hand, in shpc_reset() we have construction shpc->sec_bus->devices[PCI_DEVFN(SHPC_IDX_TO_PCI(i), 0)] to access the device in slot. The only one device.
>>>>
>>>> -- 
>>>> Best regards,
>>>> Vladimir
>>>
>>> Same thing.
>>>
>>> ... and let's not get started about sriov and ari ...
>>>
>>
>> -- 
>> Best regards,
>> Vladimir
> 

-- 
Best regards,
Vladimir

Re: [PATCH v5 13/18] pci: introduce pci_find_the_only_child()

[Michael S. Tsirkin]

On Thu, Mar 02, 2023 at 12:35:54PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 02.03.23 11:53, Michael S. Tsirkin wrote:
> > On Thu, Mar 02, 2023 at 11:45:00AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > On 02.03.23 11:37, Michael S. Tsirkin wrote:
> > > > On Thu, Mar 02, 2023 at 11:28:44AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > > On 02.03.23 00:09, Michael S. Tsirkin wrote:
> > > > > > On Thu, Feb 16, 2023 at 09:03:51PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > > > > To be used in further patch to identify the device hot-plugged into
> > > > > > > pcie-root-port.
> > > > > > > 
> > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
> > > > > > > Reviewed-by: Anton Kuchin<antonkuchin@yandex-team.ru>
> > > > > > Wait a second does this work for multifunction devices correctly?
> > > > > > 
> > > > > 
> > > > > I thought about that and I'm just lost:)
> > > > > 
> > > > > Could several (multifunction?) devices be plugged into one pcie-root-port device?
> > > > 
> > > > One device per port but one multifunction device is represented as multiple PCIDevice structures.
> > > 
> > > So, it should be OK to send _one_ event for that multifunction device, and the code is shpc_reset() is OK, but reporting "Several child devices found" is bad idea?
> > 
> > I don't know about your new event, we are discussing it separately.
> > yes all functions are removed together normally on real hardware.
> > 
> > > So, if I change the logic from pci_find_the_only_child() to pci_find_first_child() is it OK?
> > 
> > Yes though I don't like this name either - need to make it clear that
> > multifunction is ok, multiple unrelated devices aren't.
> 
> Could we check it somehow that all plugged devices represent the one multifunction device?

You can write a function like this, sure.
But really just return a bus+slot pair.

> > 
> > 
> > > > 
> > > > > Same question for SHPC slots. For example, shpc_free_devices_in_slot() looks like we can have several devices in one slot..
> > > > > On the other hand, in shpc_reset() we have construction shpc->sec_bus->devices[PCI_DEVFN(SHPC_IDX_TO_PCI(i), 0)] to access the device in slot. The only one device.
> > > > > 
> > > > > -- 
> > > > > Best regards,
> > > > > Vladimir
> > > > 
> > > > Same thing.
> > > > 
> > > > ... and let's not get started about sriov and ari ...
> > > > 
> > > 
> > > -- 
> > > Best regards,
> > > Vladimir
> > 
> 
> -- 
> Best regards,
> Vladimir

[PATCH v5 14/18] 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

Re: [PATCH v5 14/18] qapi/qdev.json: unite DEVICE_* event data into single structure

[Michael S. Tsirkin]

On Thu, Feb 16, 2023 at 09:03:52PM +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>

Needs ack from QAPI maintainers.

> ---
>  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 v5 15/18] qapi: add HOTPLUG_STATE infrastructure

[Vladimir Sementsov-Ogievskiy]

For PCIe and SHPC hotplug it's important to track led indicators,
especially the power led. Add an event that helps.

This commits adds infrastructure for HOTPLUG_STATE event and
corresponding query command. The implementation for PCIe and SHPC will
follow in further commits.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 qapi/qdev.json         | 175 +++++++++++++++++++++++++++++++++++++++++
 include/hw/hotplug.h   |  12 +++
 include/monitor/qdev.h |   5 ++
 hw/core/hotplug.c      |  13 +++
 softmmu/qdev-monitor.c |  50 ++++++++++++
 5 files changed, 255 insertions(+)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 135cd81586..6f2d8d6647 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -173,3 +173,178 @@
 #
 ##
 { '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.0
+##
+{ '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.0
+##
+{ '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.0
+##
+{ '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.0
+##
+{ '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.0
+##
+{ '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.0
+##
+{ 'enum': 'HotplugType',
+  'data': ['shpc', 'pcie-native'] }
+
+##
+# @HotplugState:
+#
+# Generic hotplug slot state.
+#
+# @type: type of the hotplug (shpc or pcie-native)
+#
+# Single: 8.0
+##
+{ 'union': 'HotplugState',
+  'base': { 'type': 'HotplugType' },
+  'discriminator': 'type',
+  'data': { 'shpc': 'HotplugSHPCState',
+            'pcie-native': 'HotplugPCIeNativeState' } }
+
+##
+# @HotplugBase:
+#
+# @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's QOM path. The field absent if
+#         there is no device at the moment.
+#
+# Since: 8.0
+##
+{ 'struct': 'HotplugBase',
+  'data': { 'bus': 'DeviceAndPath',
+            '*addr': 'str',
+            'child': 'DeviceAndPath' } }
+
+##
+# @HotplugEvent:
+#
+# @changed: The hotplug controller states being changed. The state
+#           fields that are not changed are not included into @changed.
+#
+# Since: 8.0
+##
+{ 'struct': 'HotplugEvent',
+  'base': 'HotplugBase',
+  'data': { 'changed': 'HotplugState' } }
+
+##
+# @HotplugInfo:
+#
+# @state: Current hotplug controller state. All applicable fields are
+#         included into @state.
+#
+# Since: 8.0
+##
+{ 'struct': 'HotplugInfo',
+  'base': 'HotplugBase',
+  'data': { 'state': 'HotplugState' } }
+
+##
+# @HOTPLUG_STATE:
+#
+# Emitted whenever the state of hotplug controller changes.
+# Only changed values are included into event. Any change of any field
+# of the state trigger the event. Several fields are included into one
+# event if they are changed simultaneously.
+#
+# Since: 8.0
+##
+{ 'event': 'HOTPLUG_STATE',
+  'data': 'HotplugEvent' }
+
+##
+# @query-hotplug:
+#
+# Query the state of hotplug controller.
+#
+# Since: 8.0
+##
+{ 'command': 'query-hotplug',
+  'data': { 'id': 'str' },
+  'returns': 'HotplugInfo' }
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
index e15f59c8b3..b82261d91e 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"
 
@@ -58,6 +59,8 @@ struct HotplugHandlerClass {
     hotplug_fn plug;
     hotplug_fn unplug_request;
     hotplug_fn unplug;
+    HotplugInfo *(*get_hotplug_state)(HotplugHandler *plug_handler,
+                                      DeviceState *plugged_dev, Error **errp);
 };
 
 /**
@@ -94,4 +97,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/monitor/qdev.h b/include/monitor/qdev.h
index 1d57bf6577..a61d5f8f1f 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);
@@ -36,4 +38,7 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
  */
 const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
 
+void qdev_hotplug_state_event(DeviceState *bus, const char *addr,
+                              DeviceState *child, HotplugState *changed_state);
+
 #endif
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/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index b8d2c4dadd..d1cc770d11 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,22 @@ bool qmp_command_available(const QmpCommand *cmd, Error **errp)
     }
     return true;
 }
+
+void qdev_hotplug_state_event(DeviceState *bus, const char *addr,
+                              DeviceState *child, HotplugState *changed_state)
+{
+    DeviceAndPath child_desc, bus_desc = {
+        .device = bus->id,
+        .path = bus->canonical_path,
+    };
+
+    if (child) {
+        child_desc = (DeviceAndPath) {
+            .device = child->id,
+            .path = child->canonical_path,
+        };
+    }
+
+    qapi_event_send_hotplug_state(&bus_desc, addr, child ? &child_desc : NULL,
+                                  changed_state);
+}
-- 
2.34.1

[PATCH v5 16/18] shpc: implement HOTPLUG_STATE event and query-hotplug

[Vladimir Sementsov-Ogievskiy]

For PCIe and SHPC hotplug it's important to track led indicators,
especially the power led.

At this step, implement the prepared infrastructure in SHPC.

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

diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index 63a7521567..b8cb86a6f7 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -126,6 +126,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 a61d5f8f1f..780e64bcdc 100644
--- a/include/monitor/qdev.h
+++ b/include/monitor/qdev.h
@@ -41,4 +41,6 @@ const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
 void qdev_hotplug_state_event(DeviceState *bus, const char *addr,
                               DeviceState *child, HotplugState *changed_state);
 
+DeviceAndPath *qdev_new_device_and_path(DeviceState *dev);
+
 #endif
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..6a4f93949d 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);
@@ -268,9 +304,16 @@ static void shpc_slot_command(PCIDevice *d, uint8_t target,
 {
     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);
+    HotplugState changed_state = {
+        .type = HOTPLUG_TYPE_SHPC,
+    };
+    HotplugSHPCState *cs = &changed_state.u.shpc;
+    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);
@@ -284,24 +327,34 @@ static void shpc_slot_command(PCIDevice *d, uint8_t target,
 
     if (power == SHPC_LED_NO) {
         power = old_power;
-    } else {
-        /* TODO: send event to monitor */
+    } else if (power != old_power) {
+        cs->has_power_led = true;
+        cs->power_led = shpc_led_state_to_qapi(power);
         shpc_set_status(shpc, slot, power, SHPC_SLOT_PWR_LED_MASK);
     }
 
     if (attn == SHPC_LED_NO) {
         attn = old_attn;
-    } else {
-        /* TODO: send event to monitor */
+    } else if (attn != old_attn) {
+        cs->has_attention_led = true;
+        cs->attention_led = shpc_led_state_to_qapi(attn);
         shpc_set_status(shpc, slot, attn, SHPC_SLOT_ATTN_LED_MASK);
     }
 
     if (state == SHPC_STATE_NO) {
         state = old_state;
-    } else {
+    } else if (state != old_state) {
+        cs->has_slot_state = true;
+        cs->slot_state = shpc_slot_state_to_qapi(state);
         shpc_set_status(shpc, slot, state, SHPC_SLOT_STATE_MASK);
     }
 
+    if (cs->has_power_led || cs->has_attention_led || cs->has_slot_state) {
+        g_autofree char *addr = shpc_idx_to_pci_addr(slot);
+
+        qdev_hotplug_state_event(DEVICE(d), addr, child_dev, &changed_state);
+    }
+
     if (!shpc_slot_is_off(old_state, old_power, old_attn) &&
         shpc_slot_is_off(state, power, attn))
     {
@@ -713,6 +766,44 @@ 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),
+        .state = g_new(HotplugState, 1),
+    };
+
+    *res->state = (HotplugState) {
+        .type = HOTPLUG_TYPE_SHPC,
+        .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/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index d1cc770d11..adbba2b8d3 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_state_event(DeviceState *bus, const char *addr,
                               DeviceState *child, HotplugState *changed_state)
 {
-- 
2.34.1

[PATCH v5 17/18] pcie: implement HOTPLUG_STATE event and query-hotplug

[Vladimir Sementsov-Ogievskiy]

For PCIe and SHPC hotplug it's important to track led indicators,
especially the power led. At this step implement the prepared
infrastructure in PCIe.

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

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 */
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index b8c24cf45f..636f962a23 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -19,6 +19,8 @@
  */
 
 #include "qemu/osdep.h"
+
+#include "monitor/qdev.h"
 #include "qapi/error.h"
 #include "hw/pci/pci_bridge.h"
 #include "hw/pci/pcie.h"
@@ -45,6 +47,23 @@ static bool pcie_sltctl_powered_off(uint16_t sltctl)
         && (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_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
  */
@@ -728,9 +747,17 @@ 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)
 {
+    PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(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);
+    uint16_t power_led, attn_led, pcc, old_power_led, old_attn_led, old_pcc;
+    DeviceState *child_dev =
+        DEVICE(pci_find_the_only_child(sec_bus, pci_bus_num(sec_bus), NULL));
+    HotplugState changed_state = {
+        .type = HOTPLUG_TYPE_PCIE_NATIVE,
+    };
+    HotplugPCIeNativeState *cs = &changed_state.u.pcie_native;
 
     if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) {
         /*
@@ -768,6 +795,27 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
                         sltsta);
     }
 
+    power_led = val & PCI_EXP_SLTCTL_PIC;
+    old_power_led = old_slt_ctl & PCI_EXP_SLTCTL_PIC;
+    cs->has_power_led = power_led != old_power_led;
+    cs->power_led = cs->has_power_led ? pcie_led_state_to_qapi(power_led) : 0;
+
+    attn_led = val & PCI_EXP_SLTCTL_AIC;
+    old_attn_led = old_slt_ctl & PCI_EXP_SLTCTL_AIC;
+    cs->has_attention_led = attn_led != old_attn_led;
+    cs->attention_led =
+        cs->has_attention_led ? pcie_led_state_to_qapi(attn_led) : 0;
+
+    pcc = val & PCI_EXP_SLTCTL_PCC;
+    old_pcc = old_slt_ctl & PCI_EXP_SLTCTL_PCC;
+    cs->has_power_on = pcc != old_pcc;
+    cs->power_on = cs->has_power_on ? pcc == PCI_EXP_SLTCTL_PWR_ON : false;
+
+
+    if (cs->has_power_led || cs->has_attention_led || cs->has_power_on) {
+        qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state);
+    }
+
     /*
      * If the slot is populated, power indicator is off and power
      * controller is off, it is safe to detach the devices.
@@ -1100,3 +1148,33 @@ 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) {
+        .bus = qdev_new_device_and_path(DEVICE(hotplug_pdev)),
+        .child = qdev_new_device_and_path(dev),
+        .state = g_new(HotplugState, 1),
+    };
+
+    *res->state = (HotplugState) {
+        .type = HOTPLUG_TYPE_PCIE_NATIVE,
+        .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 65a397ad23..8b28efc52d 100644
--- a/hw/pci/pcie_port.c
+++ b/hw/pci/pcie_port.c
@@ -188,6 +188,7 @@ static void pcie_slot_class_init(ObjectClass *oc, void *data)
     hc->plug = pcie_cap_slot_plug_cb;
     hc->unplug = pcie_cap_slot_unplug_cb;
     hc->unplug_request = pcie_cap_slot_unplug_request_cb;
+    hc->get_hotplug_state = pcie_cap_slot_get_hotplug_state;
 }
 
 static const TypeInfo pcie_slot_type_info = {
-- 
2.34.1

[PATCH v5 18/18] qapi: introduce DEVICE_ON event

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

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 qapi/qdev.json | 10 ++++++++++
 hw/pci/pcie.c  | 14 ++++++++++++++
 hw/pci/shpc.c  | 12 ++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 6f2d8d6647..116a8a7de8 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -348,3 +348,13 @@
 { '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.0
+##
+{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 636f962a23..4297e4e8dc 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -22,6 +22,7 @@
 
 #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"
@@ -47,6 +48,13 @@ 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) {
@@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
         qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state);
     }
 
+    if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_on(val) &&
+        !pcie_sltctl_powered_on(old_slt_ctl) && child_dev)
+    {
+        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
+    }
+
     /*
      * If the slot is populated, power indicator is off and power
      * controller is off, it is safe to detach the devices.
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 6a4f93949d..380b2b83b3 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -299,6 +299,12 @@ 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)
 {
@@ -366,6 +372,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)
+    {
+        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
+    }
 }
 
 static void shpc_command(PCIDevice *d)
-- 
2.34.1

Re: [PATCH v5 18/18] qapi: introduce DEVICE_ON event

[Michael S. Tsirkin]

On Thu, Feb 16, 2023 at 09:03:56PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We have DEVICE_DELETED event, that signals that device_del command is
> actually completed. But we don't have a counter-part for device_add.
> Still it's sensible for SHPC and PCIe-native hotplug, as there are time
> when the device in some intermediate state. Let's add an event that say
> that the device is finally powered on, power indicator is on and
> everything is OK for next manipulation on that device.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

I don't much mind though a bit more motivation would be nice.
How is this going to be used? When does management care?

Meanwhile, for the schema - can this one get ACKs from QAPI maintainers please?


> ---
>  qapi/qdev.json | 10 ++++++++++
>  hw/pci/pcie.c  | 14 ++++++++++++++
>  hw/pci/shpc.c  | 12 ++++++++++++
>  3 files changed, 36 insertions(+)
> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 6f2d8d6647..116a8a7de8 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -348,3 +348,13 @@
>  { '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.0
> +##
> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 636f962a23..4297e4e8dc 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -22,6 +22,7 @@
>  
>  #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"
> @@ -47,6 +48,13 @@ 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) {
> @@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
>          qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state);
>      }
>  
> +    if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_on(val) &&
> +        !pcie_sltctl_powered_on(old_slt_ctl) && child_dev)
> +    {
> +        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
> +    }
> +
>      /*
>       * If the slot is populated, power indicator is off and power
>       * controller is off, it is safe to detach the devices.
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index 6a4f93949d..380b2b83b3 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -299,6 +299,12 @@ 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)
>  {
> @@ -366,6 +372,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)
> +    {
> +        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
> +    }
>  }
>  
>  static void shpc_command(PCIDevice *d)
> -- 
> 2.34.1

Re: [PATCH v5 18/18] qapi: introduce DEVICE_ON event

[Vladimir Sementsov-Ogievskiy]

On 02.03.23 00:07, Michael S. Tsirkin wrote:
> On Thu, Feb 16, 2023 at 09:03:56PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> We have DEVICE_DELETED event, that signals that device_del command is
>> actually completed. But we don't have a counter-part for device_add.
>> Still it's sensible for SHPC and PCIe-native hotplug, as there are time
>> when the device in some intermediate state. Let's add an event that say
>> that the device is finally powered on, power indicator is on and
>> everything is OK for next manipulation on that device.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> I don't much mind though a bit more motivation would be nice.
> How is this going to be used? When does management care?

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

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

> 
> Meanwhile, for the schema - can this one get ACKs from QAPI maintainers please?
> 
> 
>> ---
>>   qapi/qdev.json | 10 ++++++++++
>>   hw/pci/pcie.c  | 14 ++++++++++++++
>>   hw/pci/shpc.c  | 12 ++++++++++++
>>   3 files changed, 36 insertions(+)
>>
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index 6f2d8d6647..116a8a7de8 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -348,3 +348,13 @@
>>   { '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.0
>> +##
>> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 636f962a23..4297e4e8dc 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -22,6 +22,7 @@
>>   
>>   #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"
>> @@ -47,6 +48,13 @@ 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) {
>> @@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
>>           qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state);
>>       }
>>   
>> +    if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_on(val) &&
>> +        !pcie_sltctl_powered_on(old_slt_ctl) && child_dev)
>> +    {
>> +        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
>> +    }
>> +
>>       /*
>>        * If the slot is populated, power indicator is off and power
>>        * controller is off, it is safe to detach the devices.
>> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
>> index 6a4f93949d..380b2b83b3 100644
>> --- a/hw/pci/shpc.c
>> +++ b/hw/pci/shpc.c
>> @@ -299,6 +299,12 @@ 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)
>>   {
>> @@ -366,6 +372,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)
>> +    {
>> +        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
>> +    }
>>   }
>>   
>>   static void shpc_command(PCIDevice *d)
>> -- 
>> 2.34.1
> 

-- 
Best regards,
Vladimir

Re: [PATCH v5 18/18] qapi: introduce DEVICE_ON event

[Michael S. Tsirkin]

On Thu, Mar 02, 2023 at 11:39:42AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 02.03.23 00:07, Michael S. Tsirkin wrote:
> > On Thu, Feb 16, 2023 at 09:03:56PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > We have DEVICE_DELETED event, that signals that device_del command is
> > > actually completed. But we don't have a counter-part for device_add.
> > > Still it's sensible for SHPC and PCIe-native hotplug, as there are time
> > > when the device in some intermediate state. Let's add an event that say
> > > that the device is finally powered on, power indicator is on and
> > > everything is OK for next manipulation on that device.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > 
> > I don't much mind though a bit more motivation would be nice.
> > How is this going to be used? When does management care?
> 
> Some 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.

Note this is kind of weak in that we don't know that there's a driver.

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

That always annoyed me. I wanted delete to just stay pending until
it triggers.

But if we can't fix that,  it's a good reason.  However it can always
start blinking again. So DEVICE_ON is not a good name. DEVICE_REMOVABLE?
And not it's not realiable, it can start blinking by the time you try to
remove.
Another problem is that guest can create a flood of these events
by starting/stopping blinking.

Maybe, if blockdev-del fails then we start listening for events
and notify when it can be retried?

DEVICE_DELETED_RETRY ?

> 3. Also, management tool may make a GUI visualization of power indicator with help of this event.
> 
> > 
> > Meanwhile, for the schema - can this one get ACKs from QAPI maintainers please?
> > 
> > 
> > > ---
> > >   qapi/qdev.json | 10 ++++++++++
> > >   hw/pci/pcie.c  | 14 ++++++++++++++
> > >   hw/pci/shpc.c  | 12 ++++++++++++
> > >   3 files changed, 36 insertions(+)
> > > 
> > > diff --git a/qapi/qdev.json b/qapi/qdev.json
> > > index 6f2d8d6647..116a8a7de8 100644
> > > --- a/qapi/qdev.json
> > > +++ b/qapi/qdev.json
> > > @@ -348,3 +348,13 @@
> > >   { '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.0
> > > +##
> > > +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index 636f962a23..4297e4e8dc 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -22,6 +22,7 @@
> > >   #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"
> > > @@ -47,6 +48,13 @@ 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) {
> > > @@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
> > >           qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state);
> > >       }
> > > +    if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_on(val) &&
> > > +        !pcie_sltctl_powered_on(old_slt_ctl) && child_dev)
> > > +    {
> > > +        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
> > > +    }
> > > +
> > >       /*
> > >        * If the slot is populated, power indicator is off and power
> > >        * controller is off, it is safe to detach the devices.
> > > diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> > > index 6a4f93949d..380b2b83b3 100644
> > > --- a/hw/pci/shpc.c
> > > +++ b/hw/pci/shpc.c
> > > @@ -299,6 +299,12 @@ 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)
> > >   {
> > > @@ -366,6 +372,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)
> > > +    {
> > > +        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
> > > +    }
> > >   }
> > >   static void shpc_command(PCIDevice *d)
> > > -- 
> > > 2.34.1
> > 
> 
> -- 
> Best regards,
> Vladimir

Re: [PATCH v5 18/18] qapi: introduce DEVICE_ON event

[Vladimir Sementsov-Ogievskiy]

On 02.03.23 11:50, Michael S. Tsirkin wrote:
> On Thu, Mar 02, 2023 at 11:39:42AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 02.03.23 00:07, Michael S. Tsirkin wrote:
>>> On Thu, Feb 16, 2023 at 09:03:56PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> We have DEVICE_DELETED event, that signals that device_del command is
>>>> actually completed. But we don't have a counter-part for device_add.
>>>> Still it's sensible for SHPC and PCIe-native hotplug, as there are time
>>>> when the device in some intermediate state. Let's add an event that say
>>>> that the device is finally powered on, power indicator is on and
>>>> everything is OK for next manipulation on that device.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>>
>>> I don't much mind though a bit more motivation would be nice.
>>> How is this going to be used? When does management care?
>>
>> Some 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.
> 
> Note this is kind of weak in that we don't know that there's a driver.

Still, in many cases we are sure that guest has the driver, but can't be sure that the hotplug is handled by guest at all

> 
>> 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.
> 
> That always annoyed me. I wanted delete to just stay pending until
> it triggers.
> 
> But if we can't fix that> it's a good reason.  However it can always
> start blinking again. So DEVICE_ON is not a good name. DEVICE_REMOVABLE?
> And not it's not realiable, it can start blinking by the time you try to
> remove.
> Another problem is that guest can create a flood of these events
> by starting/stopping blinking.

Hmm, right. Do we have some generic rate-limitator for events?

> 
> Maybe, if blockdev-del fails then we start listening for events
> and notify when it can be retried?
> 
> DEVICE_DELETED_RETRY ?

Actually HOTPLUG_STATE event from previous patch cover this.

So, DEVICE_ON is a convenient generic wrapper, to track plug process for different hotplug controllers in the same simple way: wait some seconds for DEVICE_ON, if it comes - we are OK, if not - handle error.

> 
>> 3. Also, management tool may make a GUI visualization of power indicator with help of this event.
>>
>>>
>>> Meanwhile, for the schema - can this one get ACKs from QAPI maintainers please?
>>>
>>>
>>>> ---
>>>>    qapi/qdev.json | 10 ++++++++++
>>>>    hw/pci/pcie.c  | 14 ++++++++++++++
>>>>    hw/pci/shpc.c  | 12 ++++++++++++
>>>>    3 files changed, 36 insertions(+)
>>>>
>>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>>> index 6f2d8d6647..116a8a7de8 100644
>>>> --- a/qapi/qdev.json
>>>> +++ b/qapi/qdev.json
>>>> @@ -348,3 +348,13 @@
>>>>    { '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.0
>>>> +##
>>>> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }
>>>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>>>> index 636f962a23..4297e4e8dc 100644
>>>> --- a/hw/pci/pcie.c
>>>> +++ b/hw/pci/pcie.c
>>>> @@ -22,6 +22,7 @@
>>>>    #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"
>>>> @@ -47,6 +48,13 @@ 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) {
>>>> @@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
>>>>            qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state);
>>>>        }
>>>> +    if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_on(val) &&
>>>> +        !pcie_sltctl_powered_on(old_slt_ctl) && child_dev)
>>>> +    {
>>>> +        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
>>>> +    }
>>>> +
>>>>        /*
>>>>         * If the slot is populated, power indicator is off and power
>>>>         * controller is off, it is safe to detach the devices.
>>>> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
>>>> index 6a4f93949d..380b2b83b3 100644
>>>> --- a/hw/pci/shpc.c
>>>> +++ b/hw/pci/shpc.c
>>>> @@ -299,6 +299,12 @@ 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)
>>>>    {
>>>> @@ -366,6 +372,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)
>>>> +    {
>>>> +        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
>>>> +    }
>>>>    }
>>>>    static void shpc_command(PCIDevice *d)
>>>> -- 
>>>> 2.34.1
>>>
>>
>> -- 
>> Best regards,
>> Vladimir
> 

-- 
Best regards,
Vladimir

Re: [PATCH v5 18/18] qapi: introduce DEVICE_ON event

[Michael S. Tsirkin]

On Thu, Mar 02, 2023 at 12:16:23PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 02.03.23 11:50, Michael S. Tsirkin wrote:
> > On Thu, Mar 02, 2023 at 11:39:42AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > On 02.03.23 00:07, Michael S. Tsirkin wrote:
> > > > On Thu, Feb 16, 2023 at 09:03:56PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > > We have DEVICE_DELETED event, that signals that device_del command is
> > > > > actually completed. But we don't have a counter-part for device_add.
> > > > > Still it's sensible for SHPC and PCIe-native hotplug, as there are time
> > > > > when the device in some intermediate state. Let's add an event that say
> > > > > that the device is finally powered on, power indicator is on and
> > > > > everything is OK for next manipulation on that device.
> > > > > 
> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > > 
> > > > I don't much mind though a bit more motivation would be nice.
> > > > How is this going to be used? When does management care?
> > > 
> > > Some 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.
> > 
> > Note this is kind of weak in that we don't know that there's a driver.
> 
> Still, in many cases we are sure that guest has the driver, but can't be sure that the hotplug is handled by guest at all
> 
> > 
> > > 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.
> > 
> > That always annoyed me. I wanted delete to just stay pending until
> > it triggers.
> > 
> > But if we can't fix that> it's a good reason.  However it can always
> > start blinking again. So DEVICE_ON is not a good name. DEVICE_REMOVABLE?
> > And not it's not realiable, it can start blinking by the time you try to
> > remove.
> > Another problem is that guest can create a flood of these events
> > by starting/stopping blinking.
> 
> Hmm, right. Do we have some generic rate-limitator for events?

just design it sanely.

> > 
> > Maybe, if blockdev-del fails then we start listening for events
> > and notify when it can be retried?
> > 
> > DEVICE_DELETED_RETRY ?
> 
> Actually HOTPLUG_STATE event from previous patch cover this.
> 
> So, DEVICE_ON is a convenient generic wrapper, to track plug process for different hotplug controllers in the same simple way: wait some seconds for DEVICE_ON, if it comes - we are OK, if not - handle error.

I'd like someone from e.g. libvirt to chime in with support for this
and explanation how they plan to use it.


> > 
> > > 3. Also, management tool may make a GUI visualization of power indicator with help of this event.
> > > 
> > > > 
> > > > Meanwhile, for the schema - can this one get ACKs from QAPI maintainers please?
> > > > 
> > > > 
> > > > > ---
> > > > >    qapi/qdev.json | 10 ++++++++++
> > > > >    hw/pci/pcie.c  | 14 ++++++++++++++
> > > > >    hw/pci/shpc.c  | 12 ++++++++++++
> > > > >    3 files changed, 36 insertions(+)
> > > > > 
> > > > > diff --git a/qapi/qdev.json b/qapi/qdev.json
> > > > > index 6f2d8d6647..116a8a7de8 100644
> > > > > --- a/qapi/qdev.json
> > > > > +++ b/qapi/qdev.json
> > > > > @@ -348,3 +348,13 @@
> > > > >    { '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.0
> > > > > +##
> > > > > +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }
> > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > > > index 636f962a23..4297e4e8dc 100644
> > > > > --- a/hw/pci/pcie.c
> > > > > +++ b/hw/pci/pcie.c
> > > > > @@ -22,6 +22,7 @@
> > > > >    #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"
> > > > > @@ -47,6 +48,13 @@ 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) {
> > > > > @@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
> > > > >            qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state);
> > > > >        }
> > > > > +    if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_on(val) &&
> > > > > +        !pcie_sltctl_powered_on(old_slt_ctl) && child_dev)
> > > > > +    {
> > > > > +        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
> > > > > +    }
> > > > > +
> > > > >        /*
> > > > >         * If the slot is populated, power indicator is off and power
> > > > >         * controller is off, it is safe to detach the devices.
> > > > > diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> > > > > index 6a4f93949d..380b2b83b3 100644
> > > > > --- a/hw/pci/shpc.c
> > > > > +++ b/hw/pci/shpc.c
> > > > > @@ -299,6 +299,12 @@ 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)
> > > > >    {
> > > > > @@ -366,6 +372,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)
> > > > > +    {
> > > > > +        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
> > > > > +    }
> > > > >    }
> > > > >    static void shpc_command(PCIDevice *d)
> > > > > -- 
> > > > > 2.34.1
> > > > 
> > > 
> > > -- 
> > > Best regards,
> > > Vladimir
> > 
> 
> -- 
> Best regards,
> Vladimir

Re: [PATCH v5 18/18] qapi: introduce DEVICE_ON event

[Michael S. Tsirkin]

On Thu, Feb 16, 2023 at 09:03:56PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> We have DEVICE_DELETED event, that signals that device_del command is
> actually completed. But we don't have a counter-part for device_add.
> Still it's sensible for SHPC and PCIe-native hotplug, as there are time
> when the device in some intermediate state. Let's add an event that say
> that the device is finally powered on, power indicator is on and
> everything is OK for next manipulation on that device.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  qapi/qdev.json | 10 ++++++++++
>  hw/pci/pcie.c  | 14 ++++++++++++++
>  hw/pci/shpc.c  | 12 ++++++++++++
>  3 files changed, 36 insertions(+)
> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 6f2d8d6647..116a8a7de8 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -348,3 +348,13 @@
>  { '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.0
> +##
> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }

Same as any event, this has to be accompanied by a query.
Which query returns the "ON" status?

-- 
MST

Re: [PATCH v5 18/18] qapi: introduce DEVICE_ON event

[Vladimir Sementsov-Ogievskiy]

On 02.03.23 11:44, Michael S. Tsirkin wrote:
> On Thu, Feb 16, 2023 at 09:03:56PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> We have DEVICE_DELETED event, that signals that device_del command is
>> actually completed. But we don't have a counter-part for device_add.
>> Still it's sensible for SHPC and PCIe-native hotplug, as there are time
>> when the device in some intermediate state. Let's add an event that say
>> that the device is finally powered on, power indicator is on and
>> everything is OK for next manipulation on that device.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   qapi/qdev.json | 10 ++++++++++
>>   hw/pci/pcie.c  | 14 ++++++++++++++
>>   hw/pci/shpc.c  | 12 ++++++++++++
>>   3 files changed, 36 insertions(+)
>>
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index 6f2d8d6647..116a8a7de8 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -348,3 +348,13 @@
>>   { '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.0
>> +##
>> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }
> 
> Same as any event, this has to be accompanied by a query.
> Which query returns the "ON" status?
> 

Hmm. Seems correct to add "ON" status into "hostplug state", to be returned by query-hotplug. And then, its change should be reported by HOTPLUG_STATE, and separate DEVICE_ON is not needed.

-- 
Best regards,
Vladimir

Re: [PATCH v5 18/18] qapi: introduce DEVICE_ON event

[Michael S. Tsirkin]

On Thu, Mar 02, 2023 at 01:03:12PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 02.03.23 11:44, Michael S. Tsirkin wrote:
> > On Thu, Feb 16, 2023 at 09:03:56PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > We have DEVICE_DELETED event, that signals that device_del command is
> > > actually completed. But we don't have a counter-part for device_add.
> > > Still it's sensible for SHPC and PCIe-native hotplug, as there are time
> > > when the device in some intermediate state. Let's add an event that say
> > > that the device is finally powered on, power indicator is on and
> > > everything is OK for next manipulation on that device.
> > > 
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > ---
> > >   qapi/qdev.json | 10 ++++++++++
> > >   hw/pci/pcie.c  | 14 ++++++++++++++
> > >   hw/pci/shpc.c  | 12 ++++++++++++
> > >   3 files changed, 36 insertions(+)
> > > 
> > > diff --git a/qapi/qdev.json b/qapi/qdev.json
> > > index 6f2d8d6647..116a8a7de8 100644
> > > --- a/qapi/qdev.json
> > > +++ b/qapi/qdev.json
> > > @@ -348,3 +348,13 @@
> > >   { '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.0
> > > +##
> > > +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }
> > 
> > Same as any event, this has to be accompanied by a query.
> > Which query returns the "ON" status?
> > 
> 
> Hmm. Seems correct to add "ON" status into "hostplug state", to be returned by query-hotplug. And then, its change should be reported by HOTPLUG_STATE, and separate DEVICE_ON is not needed.
> 
> -- 
> Best regards,
> Vladimir

Given it can go on and off at any time ... I'm not that sure this makes
sense as a generic thing. one has to know about specifics of how shpc
behaves to understand and use it correctly.

-- 
MST

Re: [PATCH v5 00/18] pci hotplug tracking

[Michael S. Tsirkin]

On Thu, Feb 16, 2023 at 09:03:38PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> v5: - don't deprecate IDs and return to ID & QOM scheme
>     - split complicated HOTPLUG_STATE patch into several ones


One small point: when you change patchset subject, that is ok,
but pls reply to old patchset with an email explaining that.

> ----
> 
> The main patches are the last four ones:
> 
> - introduce HOTPLUG_STATE event, that inform when hotplug controller
> change it's state, especially indicator leds
> 
> - query-hotplug command, that provides same information as event on
> demand
> 
> - DEVICE_ON event - a kind of counterpart for DEVICE_DELETED, signals
> when device is finally accepted by guest, power indicator is on and so
> on.
> 
> That's all for smarter handling of SHPC and PCIe-native hotplug.
> 
> If you want to test new events, don't forget
>   -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
> flag, to disable ACPI hotplug default.
> 
> Vladimir Sementsov-Ogievskiy (18):
>   pci/shpc: set attention led to OFF on reset
>   pci/shpc: change shpc_get_status() return type to uint8_t
>   pci/shpc: shpc_slot_command(): handle PWRONLY -> ENABLED transition
>   pci/shpc: more generic handle hot-unplug in shpc_slot_command()
>   pci/shpc: pass PCIDevice pointer to shpc_slot_command()
>   pci/shpc: refactor shpc_device_plug_common()
>   pcie: pcie_cap_slot_write_config(): use correct macro
>   pcie_regs: drop duplicated indicator value macros
>   pcie: drop unused PCIExpressIndicator
>   pcie: pcie_cap_slot_enable_power() use correct helper
>   pcie: introduce pcie_sltctl_powered_off() helper
>   pcie: set power indicator to off on reset by default
>   pci: introduce pci_find_the_only_child()
>   qapi/qdev.json: unite DEVICE_* event data into single structure
>   qapi: add HOTPLUG_STATE infrastructure
>   shpc: implement HOTPLUG_STATE event and query-hotplug
>   pcie: implement HOTPLUG_STATE event and query-hotplug
>   qapi: introduce DEVICE_ON event
> 
>  qapi/qdev.json                  | 224 ++++++++++++++++++++++++++++++--
>  include/hw/hotplug.h            |  12 ++
>  include/hw/pci/pci.h            |   1 +
>  include/hw/pci/pci_bridge.h     |   2 +
>  include/hw/pci/pcie.h           |  10 +-
>  include/hw/pci/pcie_regs.h      |  14 --
>  include/hw/pci/shpc.h           |   2 +
>  include/monitor/qdev.h          |   7 +
>  hw/core/hotplug.c               |  13 ++
>  hw/pci-bridge/pci_bridge_dev.c  |  14 ++
>  hw/pci-bridge/pcie_pci_bridge.c |   1 +
>  hw/pci/pci.c                    |  33 +++++
>  hw/pci/pcie.c                   | 122 +++++++++++++++--
>  hw/pci/pcie_port.c              |   1 +
>  hw/pci/shpc.c                   | 214 ++++++++++++++++++++++--------
>  softmmu/qdev-monitor.c          |  67 ++++++++++
>  16 files changed, 639 insertions(+), 98 deletions(-)
> 
> -- 
> 2.34.1

Re: [PATCH v5 00/18] pci hotplug tracking

[Vladimir Sementsov-Ogievskiy]

On 02.03.23 00:16, Michael S. Tsirkin wrote:
> On Thu, Feb 16, 2023 at 09:03:38PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> v5: - don't deprecate IDs and return to ID & QOM scheme
>>      - split complicated HOTPLUG_STATE patch into several ones
> 
> One small point: when you change patchset subject, that is ok,
> but pls reply to old patchset with an email explaining that.
> 

Good point, will do next time. Sorry for inconvenience.

-- 
Best regards,
Vladimir

Re: [PATCH v5 00/18] pci hotplug tracking

[Michael S. Tsirkin]

On Thu, Feb 16, 2023 at 09:03:38PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> v5: - don't deprecate IDs and return to ID & QOM scheme
>     - split complicated HOTPLUG_STATE patch into several ones


picked up 1-12

new events and commands need more review, in particular by qapi
maintainers.

> ----
> 
> The main patches are the last four ones:
> 
> - introduce HOTPLUG_STATE event, that inform when hotplug controller
> change it's state, especially indicator leds
> 
> - query-hotplug command, that provides same information as event on
> demand
> 
> - DEVICE_ON event - a kind of counterpart for DEVICE_DELETED, signals
> when device is finally accepted by guest, power indicator is on and so
> on.
> 
> That's all for smarter handling of SHPC and PCIe-native hotplug.
> 
> If you want to test new events, don't forget
>   -global ICH9-LPC.acpi-pci-hotplug-with-bridge-support=off
> flag, to disable ACPI hotplug default.
> 
> Vladimir Sementsov-Ogievskiy (18):
>   pci/shpc: set attention led to OFF on reset
>   pci/shpc: change shpc_get_status() return type to uint8_t
>   pci/shpc: shpc_slot_command(): handle PWRONLY -> ENABLED transition
>   pci/shpc: more generic handle hot-unplug in shpc_slot_command()
>   pci/shpc: pass PCIDevice pointer to shpc_slot_command()
>   pci/shpc: refactor shpc_device_plug_common()
>   pcie: pcie_cap_slot_write_config(): use correct macro
>   pcie_regs: drop duplicated indicator value macros
>   pcie: drop unused PCIExpressIndicator
>   pcie: pcie_cap_slot_enable_power() use correct helper
>   pcie: introduce pcie_sltctl_powered_off() helper
>   pcie: set power indicator to off on reset by default
>   pci: introduce pci_find_the_only_child()
>   qapi/qdev.json: unite DEVICE_* event data into single structure
>   qapi: add HOTPLUG_STATE infrastructure
>   shpc: implement HOTPLUG_STATE event and query-hotplug
>   pcie: implement HOTPLUG_STATE event and query-hotplug
>   qapi: introduce DEVICE_ON event
> 
>  qapi/qdev.json                  | 224 ++++++++++++++++++++++++++++++--
>  include/hw/hotplug.h            |  12 ++
>  include/hw/pci/pci.h            |   1 +
>  include/hw/pci/pci_bridge.h     |   2 +
>  include/hw/pci/pcie.h           |  10 +-
>  include/hw/pci/pcie_regs.h      |  14 --
>  include/hw/pci/shpc.h           |   2 +
>  include/monitor/qdev.h          |   7 +
>  hw/core/hotplug.c               |  13 ++
>  hw/pci-bridge/pci_bridge_dev.c  |  14 ++
>  hw/pci-bridge/pcie_pci_bridge.c |   1 +
>  hw/pci/pci.c                    |  33 +++++
>  hw/pci/pcie.c                   | 122 +++++++++++++++--
>  hw/pci/pcie_port.c              |   1 +
>  hw/pci/shpc.c                   | 214 ++++++++++++++++++++++--------
>  softmmu/qdev-monitor.c          |  67 ++++++++++
>  16 files changed, 639 insertions(+), 98 deletions(-)
> 
> -- 
> 2.34.1

Re: [PATCH v5 00/18] pci hotplug tracking

[Vladimir Sementsov-Ogievskiy]

On 02.03.23 00:17, Michael S. Tsirkin wrote:
> On Thu, Feb 16, 2023 at 09:03:38PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> v5: - don't deprecate IDs and return to ID & QOM scheme
>>      - split complicated HOTPLUG_STATE patch into several ones
> 
> picked up 1-12

Thanks!

-- 
Best regards,
Vladimir