[PATCH v3 00/15] pci hotplug tracking

[PATCH v3 00/15] pci hotplug tracking

[Vladimir Sementsov-Ogievskiy]

Hi all!

That's called v3, as it includes "[PATCH v2 0/3] pcie: cleanup code":
Supersedes: <20230207121116.325456-1-vsementsov@yandex-team.ru>
Supersedes: <20230204174758.234951-1-vsementsov@yandex-team.ru>
Supersedes: <20230207120922.325203-1-vsementsov@yandex-team.ru>

Ok, what's this about?

The main patches are the last three 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.

Vladimir Sementsov-Ogievskiy (15):
  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()
  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: add HOTPLUG_STATE event
  qapi: introduce DEVICE_ON event
  qapi: introduce query-hotplug command

 qapi/qdev.json                  |  97 ++++++++++++++++
 include/hw/hotplug.h            |  12 ++
 include/hw/pci/pci.h            |  16 +++
 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 +
 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                    |  66 +++++++++++
 hw/pci/pcie.c                   | 119 +++++++++++++++++--
 hw/pci/pcie_port.c              |   1 +
 hw/pci/shpc.c                   | 196 ++++++++++++++++++++++++--------
 softmmu/qdev-monitor.c          |  30 +++++
 15 files changed, 509 insertions(+), 84 deletions(-)

-- 
2.34.1

[PATCH v3 01/15] 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>
---
 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 v3 02/15] 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>
---
 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 v3 03/15] 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>
---
 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 v3 04/15] 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>
---
 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 v3 05/15] 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>
---
 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 v3 06/15] 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>
---
 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 v3 07/15] 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>
---
 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 v3 08/15] 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>
---
 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 v3 09/15] 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>
---
 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 v3 10/15] 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>
---
 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..e2da00742c 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 v3 11/15] 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>
---
 hw/pci/pcie.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index e2da00742c..774ce85619 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 v3 12/15] 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>
---
 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

[PATCH v3 13/15] qapi: add HOTPLUG_STATE event

[Vladimir Sementsov-Ogievskiy]

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

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 qapi/qdev.json       | 62 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h | 15 +++++++++++
 hw/pci/pci.c         | 33 +++++++++++++++++++++++
 hw/pci/pcie.c        | 49 ++++++++++++++++++++++++++++++++++
 hw/pci/shpc.c        | 42 ++++++++++++++++++++++++++++++
 5 files changed, 201 insertions(+)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 2708fb4e99..40dc34f091 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -158,3 +158,65 @@
 ##
 { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
   'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @HotplugLedState:
+#
+# Since: 8.0
+##
+{ 'enum': 'HotplugLedState',
+  'data': [ 'on', 'blink', 'off' ] }
+
+##
+# @HotplugPowerState:
+#
+# Since: 8.0
+##
+{ 'enum': 'HotplugPowerState',
+  'data': [ 'on', 'off' ] }
+
+##
+# @HotplugSlotState:
+#
+# Since: 8.0
+##
+{ 'enum': 'HotplugSlotState',
+  'data': [ 'power-only', 'enabled', 'disabled' ] }
+
+##
+# @HotplugState:
+#
+# @hotplug-device: hotplug device id
+# @hotplug-path: hotplug device path
+# @hotplug-slot: hotplug device slot (only for SHPC)
+# @device: device name
+# @path: device path
+# @power-led: Power Indicator
+# @attention-led: Attention Indicator
+# @state: slot state, only for SHPC hotplug controller
+# @power: Power Controller state, only for PCIe hotplug
+#
+# Since: 8.0
+##
+{ 'struct': 'HotplugState',
+  'data': { '*hotplug-device': 'str',
+            'hotplug-path': 'str',
+            '*hotplug-slot': 'int',
+            '*device': 'str',
+            'path': 'str',
+            '*power-led': 'HotplugLedState',
+            '*attention-led': 'HotplugLedState',
+            '*state': 'HotplugSlotState',
+            '*power': 'HotplugPowerState' } }
+
+##
+# @HOTPLUG_STATE:
+#
+# Emitted whenever the state of hotplug controller is changed.
+# Only changed values are included into event.
+# Only SHPC and PCIe-native hotplug are supported.
+#
+# Since: 8.0
+##
+{ 'event': 'HOTPLUG_STATE',
+  'data': 'HotplugState' }
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b6c9c44527..900e22a7d3 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -7,6 +7,9 @@
 /* PCI includes legacy ISA access.  */
 #include "hw/isa/isa.h"
 
+#include "qapi/qapi-types-qdev.h"
+#include "qapi/qapi-events-qdev.h"
+
 extern bool pci_available;
 
 /* PCI bus */
@@ -611,4 +614,16 @@ static inline void pci_irq_pulse(PCIDevice *pci_dev)
 MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
 void pci_set_power(PCIDevice *pci_dev, bool state);
 
+void pci_hotplug_state_event(DeviceState *hotplug_dev,
+                             bool has_slot, int64_t slot,
+                             DeviceState *dev,
+                             HotplugLedState power_led_old,
+                             HotplugLedState power_led_new,
+                             HotplugLedState attention_led_old,
+                             HotplugLedState attention_led_new,
+                             HotplugSlotState state_old,
+                             HotplugSlotState state_new,
+                             HotplugPowerState power_old,
+                             HotplugPowerState power_new);
+
 #endif
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 34fd1fb5b8..b7374d3d66 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2772,6 +2772,39 @@ void pci_set_power(PCIDevice *d, bool state)
     }
 }
 
+void pci_hotplug_state_event(DeviceState *hotplug_dev,
+                             bool has_slot, int64_t slot,
+                             DeviceState *dev,
+                             HotplugLedState power_led_old,
+                             HotplugLedState power_led_new,
+                             HotplugLedState attention_led_old,
+                             HotplugLedState attention_led_new,
+                             HotplugSlotState state_old,
+                             HotplugSlotState state_new,
+                             HotplugPowerState power_old,
+                             HotplugPowerState power_new)
+{
+    bool pwr_led = power_led_new != power_led_old;
+    bool attn_led = attention_led_new != attention_led_old;
+    bool state = state_new != state_old;
+    bool pwr = power_new != power_old;
+
+    if (!(pwr_led || attn_led || state || pwr)) {
+        /* No changes - no event */
+        return;
+    }
+
+    qapi_event_send_hotplug_state(hotplug_dev->id,
+                                  hotplug_dev->canonical_path,
+                                  has_slot, slot,
+                                  dev ? dev->id : NULL,
+                                  dev ? dev->canonical_path : NULL,
+                                  pwr_led, power_led_new,
+                                  attn_led, attention_led_new,
+                                  state, state_new,
+                                  pwr, power_new);
+}
+
 static const TypeInfo pci_device_type_info = {
     .name = TYPE_PCI_DEVICE,
     .parent = TYPE_DEVICE,
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 774ce85619..37e2979b3c 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -45,6 +45,35 @@ static bool pcie_sltctl_powered_off(uint16_t sltctl)
         (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF;
 }
 
+static HotplugLedState 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 HOTPLUG_LED_STATE_ON;
+    case PCI_EXP_SLTCTL_PWR_IND_BLINK:
+    case PCI_EXP_SLTCTL_ATTN_IND_BLINK:
+        return HOTPLUG_LED_STATE_BLINK;
+    case PCI_EXP_SLTCTL_PWR_IND_OFF:
+    case PCI_EXP_SLTCTL_ATTN_IND_OFF:
+        return HOTPLUG_LED_STATE_OFF;
+    default:
+        abort();
+    }
+}
+
+static HotplugPowerState pcie_power_state_to_qapi(uint16_t value)
+{
+    switch (value) {
+    case PCI_EXP_SLTCTL_PWR_ON:
+        return HOTPLUG_POWER_STATE_ON;
+    case PCI_EXP_SLTCTL_PWR_OFF:
+        return HOTPLUG_POWER_STATE_OFF;
+    default:
+        abort();
+    }
+}
+
 /***************************************************************************
  * pci express capability helper functions
  */
@@ -728,9 +757,13 @@ 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));
 
     if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) {
         /*
@@ -768,6 +801,22 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
                         sltsta);
     }
 
+    power_led = val & PCI_EXP_SLTCTL_PIC;
+    attn_led = val & PCI_EXP_SLTCTL_AIC;
+    pcc = val & PCI_EXP_SLTCTL_PCC;
+    old_power_led = old_slt_ctl & PCI_EXP_SLTCTL_PIC;
+    old_attn_led = old_slt_ctl & PCI_EXP_SLTCTL_AIC;
+    old_pcc = old_slt_ctl & PCI_EXP_SLTCTL_PCC;
+
+    pci_hotplug_state_event(DEVICE(dev), false, 0, child_dev,
+                            pcie_led_state_to_qapi(old_power_led),
+                            pcie_led_state_to_qapi(power_led),
+                            pcie_led_state_to_qapi(old_attn_led),
+                            pcie_led_state_to_qapi(attn_led),
+                            0, 0, /* no state */
+                            pcie_power_state_to_qapi(old_pcc),
+                            pcie_power_state_to_qapi(pcc));
+
     /*
      * 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 9f964b1d70..3c62d6b054 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -8,6 +8,8 @@
 #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"
 
 /* TODO: model power only and disabled slot states. */
 /* TODO: handle SERR and wakeups */
@@ -123,6 +125,34 @@
 #define SHPC_PCI_TO_IDX(pci_slot) ((pci_slot) - 1)
 #define SHPC_IDX_TO_PHYSICAL(slot) ((slot) + 1)
 
+static HotplugLedState shpc_led_state_to_qapi(uint8_t value)
+{
+    switch (value) {
+    case SHPC_LED_ON:
+        return HOTPLUG_LED_STATE_ON;
+    case SHPC_LED_BLINK:
+        return HOTPLUG_LED_STATE_BLINK;
+    case SHPC_LED_OFF:
+        return HOTPLUG_LED_STATE_OFF;
+    default:
+        abort();
+    }
+}
+
+static HotplugSlotState shpc_slot_state_to_qapi(uint8_t value)
+{
+    switch (value) {
+    case SHPC_STATE_PWRONLY:
+        return HOTPLUG_SLOT_STATE_POWER_ONLY;
+    case SHPC_STATE_ENABLED:
+        return HOTPLUG_SLOT_STATE_ENABLED;
+    case SHPC_STATE_DISABLED:
+        return HOTPLUG_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 +298,12 @@ 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);
+    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);
@@ -302,6 +335,15 @@ static void shpc_slot_command(PCIDevice *d, uint8_t target,
         shpc_set_status(shpc, slot, state, SHPC_SLOT_STATE_MASK);
     }
 
+    pci_hotplug_state_event(DEVICE(d), true, SHPC_IDX_TO_PCI(slot), child_dev,
+                            shpc_led_state_to_qapi(old_power),
+                            shpc_led_state_to_qapi(power ?: old_power),
+                            shpc_led_state_to_qapi(old_attn),
+                            shpc_led_state_to_qapi(attn ?: old_attn),
+                            shpc_slot_state_to_qapi(old_state),
+                            shpc_slot_state_to_qapi(state ?: old_state),
+                            0, 0 /* no PCC */);
+
     if (!shpc_slot_is_off(old_state, old_power, old_attn) &&
         shpc_slot_is_off(state, power, attn))
     {
-- 
2.34.1

[PATCH v3 14/15] qapi: introduce DEVICE_ON event

[Vladimir Sementsov-Ogievskiy]

We have DEVICE_DELETED event, that signals that device_del command is
actually complited. 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 | 24 ++++++++++++++++++++++++
 hw/pci/pcie.c  | 12 ++++++++++++
 hw/pci/shpc.c  | 12 ++++++++++++
 3 files changed, 48 insertions(+)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 40dc34f091..94da7ca228 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -220,3 +220,27 @@
 ##
 { 'event': 'HOTPLUG_STATE',
   'data': 'HotplugState' }
+
+
+##
+# @DEVICE_ON:
+#
+# Emitted whenever the device insertion completion is acknowledged by the guest.
+# For now only emitted for SHPC and PCIe-native hotplug.
+#
+# @device: device name
+#
+# @path: device path
+#
+# Since: 8.0
+#
+# Example:
+#
+# <- { "event": "DEVICE_ON",
+#      "data": { "device": "virtio-net-pci-0",
+#                "path": "/machine/peripheral/virtio-net-pci-0" },
+#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+#
+##
+{ 'event': 'DEVICE_ON',
+  'data': { '*device': 'str', 'path': 'str' } }
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 37e2979b3c..bc7e60ff9d 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -45,6 +45,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 HotplugLedState pcie_led_state_to_qapi(uint16_t value)
 {
     switch (value) {
@@ -816,6 +823,11 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
                             0, 0, /* no state */
                             pcie_power_state_to_qapi(old_pcc),
                             pcie_power_state_to_qapi(pcc));
+    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
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 3c62d6b054..cce9cf19b5 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -293,6 +293,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)
 {
@@ -355,6 +361,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

[PATCH v3 15/15] qapi: introduce query-hotplug command

[Vladimir Sementsov-Ogievskiy]

Add a command that returns same information like HOTPLUG_STATE event.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 qapi/qdev.json                  | 11 +++++++
 include/hw/hotplug.h            | 12 ++++++++
 include/hw/pci/pci_bridge.h     |  2 ++
 include/hw/pci/pcie.h           |  2 ++
 include/hw/pci/shpc.h           |  2 ++
 hw/core/hotplug.c               | 13 ++++++++
 hw/pci-bridge/pci_bridge_dev.c  | 14 +++++++++
 hw/pci-bridge/pcie_pci_bridge.c |  1 +
 hw/pci/pcie.c                   | 28 +++++++++++++++++
 hw/pci/pcie_port.c              |  1 +
 hw/pci/shpc.c                   | 54 ++++++++++++++++++++++++++-------
 softmmu/qdev-monitor.c          | 30 ++++++++++++++++++
 12 files changed, 159 insertions(+), 11 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 94da7ca228..c3c2dfc388 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -244,3 +244,14 @@
 ##
 { 'event': 'DEVICE_ON',
   'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @query-hotplug:
+#
+# Query the state of hotplug controller.
+#
+# Since: 8.0
+##
+{ 'command': 'query-hotplug',
+  'data': { 'id': 'str' },
+  'returns': 'HotplugState' }
diff --git a/include/hw/hotplug.h b/include/hw/hotplug.h
index e15f59c8b3..a6ed7aa382 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;
+    HotplugState *(*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.
+ */
+HotplugState *hotplug_handler_get_hotplug_state(HotplugHandler *plug_handler,
+                                                DeviceState *plugged_dev,
+                                                Error **errp);
 #endif
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index 63a7521567..566cf8df7b 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);
+HotplugState *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/pcie.h b/include/hw/pci/pcie.h
index 3cc2b15957..2379227a70 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);
+HotplugState *pcie_cap_slot_get_hotplug_state(HotplugHandler *hotplug_dev,
+                                              DeviceState *dev, Error **errp);
 #endif /* QEMU_PCIE_H */
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 89c7a3b7fa..b68ec30a87 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);
+HotplugState *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/hw/core/hotplug.c b/hw/core/hotplug.c
index 17ac986685..c60ab86ce2 100644
--- a/hw/core/hotplug.c
+++ b/hw/core/hotplug.c
@@ -57,6 +57,19 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
     }
 }
 
+HotplugState *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/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 4b2696ea7f..21bb3551f0 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);
 }
 
+HotplugState *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/pcie.c b/hw/pci/pcie.c
index bc7e60ff9d..67427c0cf2 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -1161,3 +1161,31 @@ void pcie_acs_reset(PCIDevice *dev)
         pci_set_word(dev->config + dev->exp.acs_cap + PCI_ACS_CTRL, 0);
     }
 }
+
+HotplugState *pcie_cap_slot_get_hotplug_state(HotplugHandler *hotplug_dev,
+                                              DeviceState *dev, Error **errp)
+{
+    PCIDevice *hotplug_pdev = PCI_DEVICE(hotplug_dev);
+    DeviceState *hotplug_ds = DEVICE(hotplug_pdev);
+    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;
+    HotplugState *res = g_new(HotplugState, 1);
+
+    *res = (HotplugState) {
+        .hotplug_device = g_strdup(hotplug_ds->id),
+        .hotplug_path = g_strdup(hotplug_ds->canonical_path),
+        .device = g_strdup(dev->id),
+        .path = g_strdup(dev->canonical_path),
+        .has_power_led = true,
+        .power_led = pcie_led_state_to_qapi(power_led),
+        .has_attention_led = true,
+        .attention_led = pcie_led_state_to_qapi(attn_led),
+        .has_power = true,
+        .power = pcie_power_state_to_qapi(pcc),
+    };
+
+    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 = {
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index cce9cf19b5..4fb37a2436 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -550,8 +550,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);
@@ -561,21 +562,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;
     }
 
@@ -617,16 +617,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;
     }
 
@@ -770,6 +767,41 @@ void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
     shpc_cap_update_dword(d);
 }
 
+HotplugState *shpc_get_hotplug_state(HotplugHandler *hotplug_dev,
+                                     DeviceState *dev, Error **errp)
+{
+    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
+    DeviceState *hotplug_ds = DEVICE(pci_hotplug_dev);
+    SHPCDevice *shpc = pci_hotplug_dev->shpc;
+    int slot;
+    uint8_t state, power, attn;
+    HotplugState *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(HotplugState, 1);
+    *res = (HotplugState) {
+        .hotplug_device = g_strdup(hotplug_ds->id),
+        .hotplug_path = g_strdup(hotplug_ds->canonical_path),
+        .device = g_strdup(dev->id),
+        .path = g_strdup(dev->canonical_path),
+        .has_power_led = true,
+        .power_led = shpc_led_state_to_qapi(power),
+        .has_attention_led = true,
+        .attention_led = shpc_led_state_to_qapi(attn),
+        .has_state = true,
+        .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 b8d2c4dadd..bd47c117dc 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -956,6 +956,36 @@ void qmp_device_del(const char *id, Error **errp)
     }
 }
 
+HotplugState *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;
-- 
2.34.1

Re: [PATCH v3 10/15] pcie: introduce pcie_sltctl_powered_off() helper

[Philippe Mathieu-Daudé]

On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote:
> 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>
> ---
>   hw/pci/pcie.c | 16 ++++++++++------
>   1 file changed, 10 insertions(+), 6 deletions(-)


> +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;

Matter of taste (it is harder to miss the &&):

    return (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF
            && (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_OFF;


> @@ -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))

Certainly simpler!

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 13/15] qapi: add HOTPLUG_STATE event

[Philippe Mathieu-Daudé]

On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote:
> For PCIe and SHPC hotplug it's important to track led indicators,
> especially the power led. Add an event that helps.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   qapi/qdev.json       | 62 ++++++++++++++++++++++++++++++++++++++++++++
>   include/hw/pci/pci.h | 15 +++++++++++
>   hw/pci/pci.c         | 33 +++++++++++++++++++++++
>   hw/pci/pcie.c        | 49 ++++++++++++++++++++++++++++++++++
>   hw/pci/shpc.c        | 42 ++++++++++++++++++++++++++++++
>   5 files changed, 201 insertions(+)
> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2708fb4e99..40dc34f091 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -158,3 +158,65 @@
>   ##
>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>     'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @HotplugLedState:
> +#
> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugLedState',
> +  'data': [ 'on', 'blink', 'off' ] }

Could this be more helpful as generic state in "hw/misc/led.h"?

> +##
> +# @HotplugPowerState:
> +#
> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugPowerState',
> +  'data': [ 'on', 'off' ] }

Could it be better to have a generic SwitchState in qapi/common.json?

No real clue, just wondering...

Re: [PATCH v3 14/15] qapi: introduce DEVICE_ON event

[Philippe Mathieu-Daudé]

On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote:
> We have DEVICE_DELETED event, that signals that device_del command is
> actually complited. 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 | 24 ++++++++++++++++++++++++
>   hw/pci/pcie.c  | 12 ++++++++++++
>   hw/pci/shpc.c  | 12 ++++++++++++
>   3 files changed, 48 insertions(+)
> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 40dc34f091..94da7ca228 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -220,3 +220,27 @@
>   ##
>   { 'event': 'HOTPLUG_STATE',
>     'data': 'HotplugState' }
> +
> +
> +##
> +# @DEVICE_ON:
> +#
> +# Emitted whenever the device insertion completion is acknowledged by the guest.
> +# For now only emitted for SHPC and PCIe-native hotplug.
> +#
> +# @device: device name
> +#
> +# @path: device path
> +#
> +# Since: 8.0
> +#
> +# Example:
> +#
> +# <- { "event": "DEVICE_ON",
> +#      "data": { "device": "virtio-net-pci-0",
> +#                "path": "/machine/peripheral/virtio-net-pci-0" },

Why provide both device & path? Could the type_name be helpful?

> +#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +#
> +##
> +{ 'event': 'DEVICE_ON',
> +  'data': { '*device': 'str', 'path': 'str' } }
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 37e2979b3c..bc7e60ff9d 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -45,6 +45,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 HotplugLedState pcie_led_state_to_qapi(uint16_t value)
>   {
>       switch (value) {
> @@ -816,6 +823,11 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
>                               0, 0, /* no state */
>                               pcie_power_state_to_qapi(old_pcc),
>                               pcie_power_state_to_qapi(pcc));
> +    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);
> +    }

Beside this series aims, but this probably belong to the QDev layer;
if we ever model power domains & co some day.
Then this would be refactored to something like:

           qdev_set_power_on(DEVICE(child_dev));

which itself emit the event.

Regards,

Phil.

Re: [PATCH v3 15/15] qapi: introduce query-hotplug command

[Markus Armbruster]

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

> Add a command that returns same information like HOTPLUG_STATE event.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Events and queries commonly come paired: management applications want
the event so they don't have to poll, and they want the query so they
can resynchronize after a disconnect.  Adding an event without a query
should make reviewers ask why no query.

You add the event in PATCH 13, and the query now.  I'd add them both in
a single patch.  Matter of taste.  If you keep them separate, please
have the first patch mention the second will follow shortly, to help
reviewers.

Re: [PATCH v3 13/15] qapi: add HOTPLUG_STATE event

[Markus Armbruster]

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

> For PCIe and SHPC hotplug it's important to track led indicators,
> especially the power led. Add an event that helps.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  qapi/qdev.json       | 62 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci/pci.h | 15 +++++++++++
>  hw/pci/pci.c         | 33 +++++++++++++++++++++++
>  hw/pci/pcie.c        | 49 ++++++++++++++++++++++++++++++++++
>  hw/pci/shpc.c        | 42 ++++++++++++++++++++++++++++++
>  5 files changed, 201 insertions(+)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2708fb4e99..40dc34f091 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -158,3 +158,65 @@
>  ##
>  { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @HotplugLedState:
> +#

No documentation?

> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugLedState',
> +  'data': [ 'on', 'blink', 'off' ] }
> +
> +##
> +# @HotplugPowerState:

No documentation?

> +#
> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugPowerState',
> +  'data': [ 'on', 'off' ] }

Why not bool?

> +##
> +# @HotplugSlotState:

No documentation?

> +#
> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugSlotState',
> +  'data': [ 'power-only', 'enabled', 'disabled' ] }
> +
> +##
> +# @HotplugState:
> +#
> +# @hotplug-device: hotplug device id
> +# @hotplug-path: hotplug device path
> +# @hotplug-slot: hotplug device slot (only for SHPC)
> +# @device: device name
> +# @path: device path
> +# @power-led: Power Indicator
> +# @attention-led: Attention Indicator
> +# @state: slot state, only for SHPC hotplug controller
> +# @power: Power Controller state, only for PCIe hotplug



> +#
> +# Since: 8.0
> +##
> +{ 'struct': 'HotplugState',
> +  'data': { '*hotplug-device': 'str',
> +            'hotplug-path': 'str',
> +            '*hotplug-slot': 'int',
> +            '*device': 'str',
> +            'path': 'str',
> +            '*power-led': 'HotplugLedState',
> +            '*attention-led': 'HotplugLedState',
> +            '*state': 'HotplugSlotState',
> +            '*power': 'HotplugPowerState' } }

Too terse.

What do @hotplug-device and @device name?  Are these qdev-id?

What kind of paths are @hotplug-path and @path?  Are these paths to an
object device in the QOM tree?  Which object?

What's a @hotplug-slot?

> +
> +##
> +# @HOTPLUG_STATE:
> +#
> +# Emitted whenever the state of hotplug controller is changed.

Suggest "the state of hotplug controller changes."

Regardless, too terse.  What state changes exactly trigger the event?

> +# Only changed values are included into event.

"in the event"

Which values are included for each event trigger?

> +# Only SHPC and PCIe-native hotplug are supported.

Suggest something like "only ... provide this event."

Are parts of HotplugState specific to "SHPC and PCIe-native"?  Or asked
differently: when we make other kinds of hotplug send the event, what
would we need to change here?

> +#
> +# Since: 8.0
> +##
> +{ 'event': 'HOTPLUG_STATE',
> +  'data': 'HotplugState' }

[...]

Re: [PATCH v3 13/15] qapi: add HOTPLUG_STATE event

[Vladimir Sementsov-Ogievskiy]

On 10.02.23 00:28, Philippe Mathieu-Daudé wrote:
> On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote:
>> For PCIe and SHPC hotplug it's important to track led indicators,
>> especially the power led. Add an event that helps.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   qapi/qdev.json       | 62 ++++++++++++++++++++++++++++++++++++++++++++
>>   include/hw/pci/pci.h | 15 +++++++++++
>>   hw/pci/pci.c         | 33 +++++++++++++++++++++++
>>   hw/pci/pcie.c        | 49 ++++++++++++++++++++++++++++++++++
>>   hw/pci/shpc.c        | 42 ++++++++++++++++++++++++++++++
>>   5 files changed, 201 insertions(+)
>>
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index 2708fb4e99..40dc34f091 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -158,3 +158,65 @@
>>   ##
>>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>     'data': { '*device': 'str', 'path': 'str' } }
>> +
>> +##
>> +# @HotplugLedState:
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'enum': 'HotplugLedState',
>> +  'data': [ 'on', 'blink', 'off' ] }
> 
> Could this be more helpful as generic state in "hw/misc/led.h"?

Hmm. LEDState ? Doesn't look similar..

> 
>> +##
>> +# @HotplugPowerState:
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'enum': 'HotplugPowerState',
>> +  'data': [ 'on', 'off' ] }
> 
> Could it be better to have a generic SwitchState in qapi/common.json?
> 

Hmm not sure. This way I stress that it's part of PCIe spec.. Hmm. But still I didn't call it PCIE_*... Maybe generic OnOff in qapi/common.json would be the best.


-- 
Best regards,
Vladimir

Re: [PATCH v3 13/15] qapi: add HOTPLUG_STATE event

[Philippe Mathieu-Daudé]

On 10/2/23 11:47, Vladimir Sementsov-Ogievskiy wrote:
> On 10.02.23 00:28, Philippe Mathieu-Daudé wrote:
>> On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote:
>>> For PCIe and SHPC hotplug it's important to track led indicators,
>>> especially the power led. Add an event that helps.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
>>>   qapi/qdev.json       | 62 ++++++++++++++++++++++++++++++++++++++++++++
>>>   include/hw/pci/pci.h | 15 +++++++++++
>>>   hw/pci/pci.c         | 33 +++++++++++++++++++++++
>>>   hw/pci/pcie.c        | 49 ++++++++++++++++++++++++++++++++++
>>>   hw/pci/shpc.c        | 42 ++++++++++++++++++++++++++++++
>>>   5 files changed, 201 insertions(+)
>>>
>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>> index 2708fb4e99..40dc34f091 100644
>>> --- a/qapi/qdev.json
>>> +++ b/qapi/qdev.json
>>> @@ -158,3 +158,65 @@
>>>   ##
>>>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>>     'data': { '*device': 'str', 'path': 'str' } }
>>> +
>>> +##
>>> +# @HotplugLedState:
>>> +#
>>> +# Since: 8.0
>>> +##
>>> +{ 'enum': 'HotplugLedState',
>>> +  'data': [ 'on', 'blink', 'off' ] }
>>
>> Could this be more helpful as generic state in "hw/misc/led.h"?
> 
> Hmm. LEDState ? Doesn't look similar..

Name 'LedActivity' so we can reuse in LEDState?

   { 'enum': 'LedActivity',
     'data': [ 'on', 'blink', 'off' ] }

Re: [PATCH v3 13/15] qapi: add HOTPLUG_STATE event

[Vladimir Sementsov-Ogievskiy]

On 10.02.23 13:23, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> For PCIe and SHPC hotplug it's important to track led indicators,
>> especially the power led. Add an event that helps.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>>   qapi/qdev.json       | 62 ++++++++++++++++++++++++++++++++++++++++++++
>>   include/hw/pci/pci.h | 15 +++++++++++
>>   hw/pci/pci.c         | 33 +++++++++++++++++++++++
>>   hw/pci/pcie.c        | 49 ++++++++++++++++++++++++++++++++++
>>   hw/pci/shpc.c        | 42 ++++++++++++++++++++++++++++++
>>   5 files changed, 201 insertions(+)
>>
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index 2708fb4e99..40dc34f091 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -158,3 +158,65 @@
>>   ##
>>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>     'data': { '*device': 'str', 'path': 'str' } }
>> +
>> +##
>> +# @HotplugLedState:
>> +#
> 
> No documentation?

Will do!

> 
>> +# Since: 8.0
>> +##
>> +{ 'enum': 'HotplugLedState',
>> +  'data': [ 'on', 'blink', 'off' ] }
>> +
>> +##
>> +# @HotplugPowerState:
> 
> No documentation?
> 
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'enum': 'HotplugPowerState',
>> +  'data': [ 'on', 'off' ] }
> 
> Why not bool?

I wanted to reflect PCI_EXP_SLTCTL_PWR_ON and PCI_EXP_SLTCTL_PWR_OFF.

On the other hand, it's just a bit in the config. Power Controller Control. An unobvious thing that

  0 = Power On
  1 = Power Off

for that bit. So with proper documentation we can use boolean. But on/off is more obvious.

> 
>> +##
>> +# @HotplugSlotState:
> 
> No documentation?
> 
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'enum': 'HotplugSlotState',
>> +  'data': [ 'power-only', 'enabled', 'disabled' ] }
>> +
>> +##
>> +# @HotplugState:
>> +#
>> +# @hotplug-device: hotplug device id
>> +# @hotplug-path: hotplug device path
>> +# @hotplug-slot: hotplug device slot (only for SHPC)
>> +# @device: device name
>> +# @path: device path
>> +# @power-led: Power Indicator
>> +# @attention-led: Attention Indicator
>> +# @state: slot state, only for SHPC hotplug controller
>> +# @power: Power Controller state, only for PCIe hotplug
> 
> 
> 
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'struct': 'HotplugState',
>> +  'data': { '*hotplug-device': 'str',
>> +            'hotplug-path': 'str',
>> +            '*hotplug-slot': 'int',
>> +            '*device': 'str',
>> +            'path': 'str',
>> +            '*power-led': 'HotplugLedState',
>> +            '*attention-led': 'HotplugLedState',
>> +            '*state': 'HotplugSlotState',
>> +            '*power': 'HotplugPowerState' } }
> 
> Too terse.

Will fix)

> 
> What do @hotplug-device and @device name?  Are these qdev-id?
> 
> What kind of paths are @hotplug-path and @path?  Are these paths to an
> object device in the QOM tree?  Which object?

device / path is same name and path as for DEVICE_DELETED

> 
> What's a @hotplug-slot?

pci slot. Significant for SHPC

> 
>> +
>> +##
>> +# @HOTPLUG_STATE:
>> +#
>> +# Emitted whenever the state of hotplug controller is changed.
> 
> Suggest "the state of hotplug controller changes."
> 
> Regardless, too terse.  What state changes exactly trigger the event?

Any change of power-led / attention-led / state / power.

Will add a description

> 
>> +# Only changed values are included into event.
> 
> "in the event"
> 
> Which values are included for each event trigger?

- device ids and names always included
- power-led / attention-led / state / power  - only those who changed

> 
>> +# Only SHPC and PCIe-native hotplug are supported.
> 
> Suggest something like "only ... provide this event."
> 
> Are parts of HotplugState specific to "SHPC and PCIe-native"?  Or asked
> differently: when we make other kinds of hotplug send the event, what
> would we need to change here?

Hmm. Looks like I'd better use a union with type discriminator. This way we'll be able to add any other hotplug later.

(and even now it's better, as not all 4 state fields are shared for PCIe and SHPC)

> 
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'event': 'HOTPLUG_STATE',
>> +  'data': 'HotplugState' }
> 
> [...]
> 

-- 
Best regards,
Vladimir

Re: [PATCH v3 13/15] qapi: add HOTPLUG_STATE event

[Markus Armbruster]

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

> On 10.02.23 13:23, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>> 
>>> For PCIe and SHPC hotplug it's important to track led indicators,
>>> especially the power led. Add an event that helps.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---

[...]

>>> +##
>>> +# @HotplugState:
>>> +#
>>> +# @hotplug-device: hotplug device id
>>> +# @hotplug-path: hotplug device path
>>> +# @hotplug-slot: hotplug device slot (only for SHPC)
>>> +# @device: device name
>>> +# @path: device path
>>> +# @power-led: Power Indicator
>>> +# @attention-led: Attention Indicator
>>> +# @state: slot state, only for SHPC hotplug controller
>>> +# @power: Power Controller state, only for PCIe hotplug
>> 
>> 
>> 
>>> +#
>>> +# Since: 8.0
>>> +##
>>> +{ 'struct': 'HotplugState',
>>> +  'data': { '*hotplug-device': 'str',
>>> +            'hotplug-path': 'str',
>>> +            '*hotplug-slot': 'int',
>>> +            '*device': 'str',
>>> +            'path': 'str',
>>> +            '*power-led': 'HotplugLedState',
>>> +            '*attention-led': 'HotplugLedState',
>>> +            '*state': 'HotplugSlotState',
>>> +            '*power': 'HotplugPowerState' } }
>> 
>> Too terse.
>
> Will fix)
>
>> 
>> What do @hotplug-device and @device name?  Are these qdev-id?
>> 
>> What kind of paths are @hotplug-path and @path?  Are these paths to an
>> object device in the QOM tree?  Which object?
>
> device / path is same name and path as for DEVICE_DELETED

Got it.  But there we have just one device, and here we have two.  Which
two?

Also, DEVICE_DELETED's doc comment is better:

    # @device: the device's ID if it has one
    #
    # @path: the device's QOM path

Suggest to steal from there.

>> What's a @hotplug-slot?
>
> pci slot. Significant for SHPC
>
>> 
>>> +
>>> +##
>>> +# @HOTPLUG_STATE:
>>> +#
>>> +# Emitted whenever the state of hotplug controller is changed.
>> 
>> Suggest "the state of hotplug controller changes."
>> 
>> Regardless, too terse.  What state changes exactly trigger the event?
>
> Any change of power-led / attention-led / state / power.
>
> Will add a description
>
>> 
>>> +# Only changed values are included into event.
>> 
>> "in the event"
>> 
>> Which values are included for each event trigger?
>
> - device ids and names always included
> - power-led / attention-led / state / power  - only those who changed
>
>> 
>>> +# Only SHPC and PCIe-native hotplug are supported.
>> 
>> Suggest something like "only ... provide this event."
>> 
>> Are parts of HotplugState specific to "SHPC and PCIe-native"?  Or asked
>> differently: when we make other kinds of hotplug send the event, what
>> would we need to change here?
>
> Hmm. Looks like I'd better use a union with type discriminator. This way we'll be able to add any other hotplug later.
>
> (and even now it's better, as not all 4 state fields are shared for PCIe and SHPC)

A union feels like the way to go.

>>> +#
>>> +# Since: 8.0
>>> +##
>>> +{ 'event': 'HOTPLUG_STATE',
>>> +  'data': 'HotplugState' }
>> 
>> [...]

Re: [PATCH v3 13/15] qapi: add HOTPLUG_STATE event

[Vladimir Sementsov-Ogievskiy]

On 10.02.23 15:01, Markus Armbruster wrote:
>>> What do @hotplug-device and @device name?  Are these qdev-id?
>>>
>>> What kind of paths are @hotplug-path and @path?  Are these paths to an
>>> object device in the QOM tree?  Which object?
>> device / path is same name and path as for DEVICE_DELETED
> Got it.  But there we have just one device, and here we have two.  Which
> two?

One is "bus" in which we insert the device. I'm not sure that's it correct to call pcie-root-port a "bus", but on the other hand in device_add command it's a "bus" argument. In some arguments in code it's called hotplug_dev or something like that. Probably "bus" is better.

> 
> Also, DEVICE_DELETED's doc comment is better:
> 
>      # @device: the device's ID if it has one
>      #
>      # @path: the device's QOM path
> 
> Suggest to steal from there.
> 

OK, agree.

-- 
Best regards,
Vladimir

Re: [PATCH v3 14/15] qapi: introduce DEVICE_ON event

[Vladimir Sementsov-Ogievskiy]

On 10.02.23 00:37, Philippe Mathieu-Daudé wrote:
> On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote:
>> We have DEVICE_DELETED event, that signals that device_del command is
>> actually complited. 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 | 24 ++++++++++++++++++++++++
>>   hw/pci/pcie.c  | 12 ++++++++++++
>>   hw/pci/shpc.c  | 12 ++++++++++++
>>   3 files changed, 48 insertions(+)
>>
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index 40dc34f091..94da7ca228 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -220,3 +220,27 @@
>>   ##
>>   { 'event': 'HOTPLUG_STATE',
>>     'data': 'HotplugState' }
>> +
>> +
>> +##
>> +# @DEVICE_ON:
>> +#
>> +# Emitted whenever the device insertion completion is acknowledged by the guest.
>> +# For now only emitted for SHPC and PCIe-native hotplug.
>> +#
>> +# @device: device name
>> +#
>> +# @path: device path
>> +#
>> +# Since: 8.0
>> +#
>> +# Example:
>> +#
>> +# <- { "event": "DEVICE_ON",
>> +#      "data": { "device": "virtio-net-pci-0",
>> +#                "path": "/machine/peripheral/virtio-net-pci-0" },
> 
> Why provide both device & path? Could the type_name be helpful?

I just follow DEVICE_DELETED event. Not sure that it's the best thing to do)

> 
>> +#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>> +#
>> +##
>> +{ 'event': 'DEVICE_ON',
>> +  'data': { '*device': 'str', 'path': 'str' } }
>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>> index 37e2979b3c..bc7e60ff9d 100644
>> --- a/hw/pci/pcie.c
>> +++ b/hw/pci/pcie.c
>> @@ -45,6 +45,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 HotplugLedState pcie_led_state_to_qapi(uint16_t value)
>>   {
>>       switch (value) {
>> @@ -816,6 +823,11 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
>>                               0, 0, /* no state */
>>                               pcie_power_state_to_qapi(old_pcc),
>>                               pcie_power_state_to_qapi(pcc));
>> +    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);
>> +    }
> 
> Beside this series aims, but this probably belong to the QDev layer;
> if we ever model power domains & co some day.
> Then this would be refactored to something like:
> 
>            qdev_set_power_on(DEVICE(child_dev));

I thought about it but was not sure where such helper should be placed. For DEVICE_DELETED we don't have one..

-- 
Best regards,
Vladimir

Re: [PATCH v3 15/15] qapi: introduce query-hotplug command

[Vladimir Sementsov-Ogievskiy]

On 10.02.23 13:09, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> Add a command that returns same information like HOTPLUG_STATE event.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> 
> Events and queries commonly come paired: management applications want
> the event so they don't have to poll, and they want the query so they
> can resynchronize after a disconnect.  Adding an event without a query
> should make reviewers ask why no query.
> 
> You add the event in PATCH 13, and the query now.  I'd add them both in
> a single patch.  Matter of taste.  If you keep them separate, please
> have the first patch mention the second will follow shortly, to help
> reviewers.
> 

OK. Thanks for reviewing!

-- 
Best regards,
Vladimir

Re: [PATCH v3 14/15] qapi: introduce DEVICE_ON event

[Markus Armbruster]

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

> On 10.02.23 00:37, Philippe Mathieu-Daudé wrote:
>> On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote:
>>> We have DEVICE_DELETED event, that signals that device_del command is
>>> actually complited. 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 | 24 ++++++++++++++++++++++++
>>>   hw/pci/pcie.c  | 12 ++++++++++++
>>>   hw/pci/shpc.c  | 12 ++++++++++++
>>>   3 files changed, 48 insertions(+)
>>>
>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>> index 40dc34f091..94da7ca228 100644
>>> --- a/qapi/qdev.json
>>> +++ b/qapi/qdev.json
>>> @@ -220,3 +220,27 @@
>>>   ##
>>>   { 'event': 'HOTPLUG_STATE',
>>>     'data': 'HotplugState' }
>>> +
>>> +
>>> +##
>>> +# @DEVICE_ON:
>>> +#
>>> +# Emitted whenever the device insertion completion is acknowledged by the guest.
>>> +# For now only emitted for SHPC and PCIe-native hotplug.
>>> +#
>>> +# @device: device name

Make that "the device's ID if it has one", and ...

>>> +#
>>> +# @path: device path

... "the device's QOM path", please.

>>> +#
>>> +# Since: 8.0
>>> +#
>>> +# Example:
>>> +#
>>> +# <- { "event": "DEVICE_ON",
>>> +#      "data": { "device": "virtio-net-pci-0",
>>> +#                "path": "/machine/peripheral/virtio-net-pci-0" },
>>
>> Why provide both device & path? Could the type_name be helpful?
>
> I just follow DEVICE_DELETED event. Not sure that it's the best thing to do)

The device ID is redundant, since the QOM path of a device with an ID is
/machine/peripheral/ID.

Fine print: code could conceivably violate this invariant.  But code
should *not* create devices with IDs.  IDs are strictly for the user.

We commonly send both device ID and QOM path, mostly for historical
reasons: the former precede the latter.

There are exceptions, such as query-cpus-fast.  Can't say offhand
whether CPUs can be created with IDs.

It's probably best to remain consistent with DEVICE_DELETED here.

I'd be in favour of deprecating and deleting redundant device IDs in QMP
output.

[...]

Re: [PATCH v3 14/15] qapi: introduce DEVICE_ON event

[Vladimir Sementsov-Ogievskiy]

On 13.02.23 12:30, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 10.02.23 00:37, Philippe Mathieu-Daudé wrote:
>>> On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote:
>>>> We have DEVICE_DELETED event, that signals that device_del command is
>>>> actually complited. 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 | 24 ++++++++++++++++++++++++
>>>>    hw/pci/pcie.c  | 12 ++++++++++++
>>>>    hw/pci/shpc.c  | 12 ++++++++++++
>>>>    3 files changed, 48 insertions(+)
>>>>
>>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>>> index 40dc34f091..94da7ca228 100644
>>>> --- a/qapi/qdev.json
>>>> +++ b/qapi/qdev.json
>>>> @@ -220,3 +220,27 @@
>>>>    ##
>>>>    { 'event': 'HOTPLUG_STATE',
>>>>      'data': 'HotplugState' }
>>>> +
>>>> +
>>>> +##
>>>> +# @DEVICE_ON:
>>>> +#
>>>> +# Emitted whenever the device insertion completion is acknowledged by the guest.
>>>> +# For now only emitted for SHPC and PCIe-native hotplug.
>>>> +#
>>>> +# @device: device name
> 
> Make that "the device's ID if it has one", and ...
> 
>>>> +#
>>>> +# @path: device path
> 
> ... "the device's QOM path", please.
> 
>>>> +#
>>>> +# Since: 8.0
>>>> +#
>>>> +# Example:
>>>> +#
>>>> +# <- { "event": "DEVICE_ON",
>>>> +#      "data": { "device": "virtio-net-pci-0",
>>>> +#                "path": "/machine/peripheral/virtio-net-pci-0" },
>>>
>>> Why provide both device & path? Could the type_name be helpful?
>>
>> I just follow DEVICE_DELETED event. Not sure that it's the best thing to do)
> 
> The device ID is redundant, since the QOM path of a device with an ID is
> /machine/peripheral/ID.
> 
> Fine print: code could conceivably violate this invariant.  But code
> should *not* create devices with IDs.  IDs are strictly for the user.
> 
> We commonly send both device ID and QOM path, mostly for historical
> reasons: the former precede the latter.
> 
> There are exceptions, such as query-cpus-fast.  Can't say offhand
> whether CPUs can be created with IDs.
> 
> It's probably best to remain consistent with DEVICE_DELETED here.
> 
> I'd be in favour of deprecating and deleting redundant device IDs in QMP
> output.
> 

Hmm. Good. I can make a patch with deprecation and add new events with only QOM path fields after it.

-- 
Best regards,
Vladimir