[PATCH v4 00/16] pci hotplug tracking

[PATCH v4 00/16] pci hotplug tracking

[Vladimir Sementsov-Ogievskiy]

Hi all!

v4: rework the API after discussion in the mailing list

----

The main patches are the last two 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 (16):
  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: deprecate "device" field of DEVICE_* events
  qapi: add HOTPLUG_STATE event
  qapi: introduce DEVICE_ON event

 docs/about/deprecated.rst       |   9 ++
 qapi/qdev.json                  | 200 ++++++++++++++++++++++++++++-
 include/hw/hotplug.h            |  12 ++
 include/hw/pci/pci.h            |   4 +
 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          |   5 +
 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                   | 215 ++++++++++++++++++++++++--------
 softmmu/qdev-monitor.c          |  39 ++++++
 17 files changed, 608 insertions(+), 88 deletions(-)

-- 
2.34.1

[PATCH v4 01/16] 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

Re: [PATCH v4 01/16] pci/shpc: set attention led to OFF on reset

[Philippe Mathieu-Daudé]

On 13/2/23 15:00, Vladimir Sementsov-Ogievskiy wrote:
> 0 is not a valid state for the led. Let's start with OFF.

"0 is not valid" so we should abort(value != 0) in shpc_set_status(),

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

... however value=0 is used:

hw/pci/shpc.c:215:            shpc_set_status(shpc, i, 0, 
SHPC_SLOT_STATUS_MRL_OPEN);
hw/pci/shpc.c:226:        shpc_set_status(shpc, i, 0, SHPC_SLOT_STATUS_66);
hw/pci/shpc.c:522:        shpc_set_status(shpc, slot, 0, 
SHPC_SLOT_STATUS_MRL_OPEN);
hw/pci/shpc.c:531:        shpc_set_status(shpc, slot, 0, 
SHPC_SLOT_STATUS_MRL_OPEN);
hw/pci/shpc.c:543:    shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_66);
hw/pci/shpc.c:589:    shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_66);

Is this fixable?

Re: [PATCH v4 01/16] pci/shpc: set attention led to OFF on reset

[Vladimir Sementsov-Ogievskiy]

On 13.02.23 17:16, Philippe Mathieu-Daudé wrote:
> On 13/2/23 15:00, Vladimir Sementsov-Ogievskiy wrote:
>> 0 is not a valid state for the led. Let's start with OFF.
> 
> "0 is not valid" so we should abort(value != 0) in shpc_set_status(),
> 
>> 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);
> 
> ... however value=0 is used:
> 
> hw/pci/shpc.c:215:            shpc_set_status(shpc, i, 0, SHPC_SLOT_STATUS_MRL_OPEN);
> hw/pci/shpc.c:226:        shpc_set_status(shpc, i, 0, SHPC_SLOT_STATUS_66);
> hw/pci/shpc.c:522:        shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_MRL_OPEN);
> hw/pci/shpc.c:531:        shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_MRL_OPEN);
> hw/pci/shpc.c:543:    shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_66);
> hw/pci/shpc.c:589:    shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_66);
> 
> Is this fixable?

0 is not valid only for _STATE and _LED_ masks. It's OK for other fields

-- 
Best regards,
Vladimir

Re: [PATCH v4 01/16] pci/shpc: set attention led to OFF on reset

[Anton Kuchin]

On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote:
> 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);
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>

[PATCH v4 02/16] 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

Re: [PATCH v4 02/16] pci/shpc: change shpc_get_status() return type to uint8_t

[Anton Kuchin]

On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote:
> 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,
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>

[PATCH v4 03/16] 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

Re: [PATCH v4 03/16] pci/shpc: shpc_slot_command(): handle PWRONLY -> ENABLED transition

[Anton Kuchin]

On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote:
> 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. */
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>

[PATCH v4 04/16] 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

Re: [PATCH v4 04/16] pci/shpc: more generic handle hot-unplug in shpc_slot_command()

[Anton Kuchin]

On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote:
> 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;
>       }
>   }
>   
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>

[PATCH v4 05/16] 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

Re: [PATCH v4 05/16] pci/shpc: pass PCIDevice pointer to shpc_slot_command()

[Anton Kuchin]

On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote:
> 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);
>   }
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>

[PATCH v4 06/16] 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>
---
 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

Re: [PATCH v4 06/16] pci/shpc: refactor shpc_device_plug_common()

[Anton Kuchin]

On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote:
> 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>
> ---
>   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;
>       }
>   
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>

[PATCH v4 07/16] 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

Re: [PATCH v4 07/16] pcie: pcie_cap_slot_write_config(): use correct macro

[Anton Kuchin]

On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote:
> 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);
Reviewed-by: Anton Kuchin <antonkuchin@yandex-team.ru>

[PATCH v4 08/16] 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

Re: [PATCH v4 08/16] pcie_regs: drop duplicated indicator value macros

[Anton Kuchin]

On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote:
> 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);

[PATCH v4 09/16] 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

Re: [PATCH v4 09/16] pcie: drop unused PCIExpressIndicator

[Anton Kuchin]

On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote:
> 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 |      \

[PATCH v4 10/16] 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

Re: [PATCH v4 10/16] pcie: pcie_cap_slot_enable_power() use correct helper

[Anton Kuchin]

On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote:
> *_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);
>       }
>   }
>   

[PATCH v4 11/16] 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>
---
 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

Re: [PATCH v4 11/16] pcie: introduce pcie_sltctl_powered_off() helper

[Anton Kuchin]

On 13/02/2023 16:00, 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>
> 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);

[PATCH v4 12/16] 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 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

Re: [PATCH v4 12/16] pcie: set power indicator to off on reset by default

[Anton Kuchin]

On 13/02/2023 16:00, Vladimir Sementsov-Ogievskiy wrote:
> 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) {

[PATCH v4 13/16] 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

Re: [PATCH v4 13/16] pci: introduce pci_find_the_only_child()

[Anton Kuchin]

On 13/02/2023 16:01, 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>
> ---
>   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;

[PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Vladimir Sementsov-Ogievskiy]

The device field is redundant, because QOM path always include device
ID when this ID exist.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 docs/about/deprecated.rst |  9 +++++++++
 qapi/qdev.json            | 12 ++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index da2e6fe63d..b389934691 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -171,6 +171,15 @@ accepted incorrect commands will return an error. Users should make sure that
 all arguments passed to ``device_add`` are consistent with the documented
 property types.
 
+QEMU Machine Protocol (QMP) events
+----------------------------------
+
+``DEVICE_DELETED`` & ``DEVICE_UNPLUG_GUEST_ERROR`` field ``device`` (since 8.0)
+'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Devices that has ``ID`` always has QOM path `/machine/peripheral/ID`. So, the
+``device`` field is redundant and deprecated. Use the ``path`` field instead.
+
 Host Architectures
 ------------------
 
diff --git a/qapi/qdev.json b/qapi/qdev.json
index 2708fb4e99..325ef554f9 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -124,6 +124,9 @@
 #
 # @path: the device's QOM path
 #
+# Features:
+# @deprecated: Member @device is deprecated as redundant. Use @path instead.
+#
 # Since: 1.5
 #
 # Example:
@@ -135,7 +138,8 @@
 #
 ##
 { 'event': 'DEVICE_DELETED',
-  'data': { '*device': 'str', 'path': 'str' } }
+  'data': { '*device': { 'type': 'str', 'features': [ 'deprecated' ] },
+            'path': 'str' } }
 
 ##
 # @DEVICE_UNPLUG_GUEST_ERROR:
@@ -146,6 +150,9 @@
 #
 # @path: the device's QOM path
 #
+# Features:
+# @deprecated: Member @device is deprecated as redundant. Use @path instead.
+#
 # Since: 6.2
 #
 # Example:
@@ -157,4 +164,5 @@
 #
 ##
 { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
-  'data': { '*device': 'str', 'path': 'str' } }
+  'data': { '*device': { 'type': 'str', 'features': [ 'deprecated' ] },
+            'path': 'str' } }
-- 
2.34.1

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Daniel P. Berrangé]

On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> The device field is redundant, because QOM path always include device
> ID when this ID exist.

The flipside to that view is that applications configuring QEMU are
specifying the device ID for -device (CLI) / device_add (QMP) and
not the QOM path. IOW, the device ID is the more interesting field
than QOM path, so feels like the wrong one to be dropping.

Is there any real benefit to dropping this ? 

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  docs/about/deprecated.rst |  9 +++++++++
>  qapi/qdev.json            | 12 ++++++++++--
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index da2e6fe63d..b389934691 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -171,6 +171,15 @@ accepted incorrect commands will return an error. Users should make sure that
>  all arguments passed to ``device_add`` are consistent with the documented
>  property types.
>  
> +QEMU Machine Protocol (QMP) events
> +----------------------------------
> +
> +``DEVICE_DELETED`` & ``DEVICE_UNPLUG_GUEST_ERROR`` field ``device`` (since 8.0)
> +'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Devices that has ``ID`` always has QOM path `/machine/peripheral/ID`. So, the
> +``device`` field is redundant and deprecated. Use the ``path`` field instead.
> +
>  Host Architectures
>  ------------------
>  
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2708fb4e99..325ef554f9 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -124,6 +124,9 @@
>  #
>  # @path: the device's QOM path
>  #
> +# Features:
> +# @deprecated: Member @device is deprecated as redundant. Use @path instead.
> +#
>  # Since: 1.5
>  #
>  # Example:
> @@ -135,7 +138,8 @@
>  #
>  ##
>  { 'event': 'DEVICE_DELETED',
> -  'data': { '*device': 'str', 'path': 'str' } }
> +  'data': { '*device': { 'type': 'str', 'features': [ 'deprecated' ] },
> +            'path': 'str' } }
>  
>  ##
>  # @DEVICE_UNPLUG_GUEST_ERROR:
> @@ -146,6 +150,9 @@
>  #
>  # @path: the device's QOM path
>  #
> +# Features:
> +# @deprecated: Member @device is deprecated as redundant. Use @path instead.
> +#
>  # Since: 6.2
>  #
>  # Example:
> @@ -157,4 +164,5 @@
>  #
>  ##
>  { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
> -  'data': { '*device': 'str', 'path': 'str' } }
> +  'data': { '*device': { 'type': 'str', 'features': [ 'deprecated' ] },
> +            'path': 'str' } }
> -- 
> 2.34.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> The device field is redundant, because QOM path always include device
>> ID when this ID exist.
>
> The flipside to that view is that applications configuring QEMU are
> specifying the device ID for -device (CLI) / device_add (QMP) and
> not the QOM path. IOW, the device ID is the more interesting field
> than QOM path, so feels like the wrong one to be dropping.

QOM path is a reliable way to identify a device.  Device ID isn't:
devices need not have one.  Therefore, dropping the QOM path would be
wrong.

> Is there any real benefit to dropping this ? 

The device ID is a trap for the unwary: relying on it is fine until you
run into a scenario where you have to deal with devices lacking IDs.

I suggested to deprecate it in review of "[PATCH v3 14/15] qapi:
introduce DEVICE_ON event" (Message-ID: <873579x67l.fsf@pond.sub.org>).
Quote:

    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.

    [...]

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

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Peter Krempa]

On Tue, Feb 14, 2023 at 09:54:22 +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >> The device field is redundant, because QOM path always include device
> >> ID when this ID exist.
> >
> > The flipside to that view is that applications configuring QEMU are
> > specifying the device ID for -device (CLI) / device_add (QMP) and
> > not the QOM path. IOW, the device ID is the more interesting field
> > than QOM path, so feels like the wrong one to be dropping.
> 
> QOM path is a reliable way to identify a device.  Device ID isn't:
> devices need not have one.  Therefore, dropping the QOM path would be
> wrong.
> 
> > Is there any real benefit to dropping this ? 
> 
> The device ID is a trap for the unwary: relying on it is fine until you
> run into a scenario where you have to deal with devices lacking IDs.

Note that libvirt's code is still using the 'device' bit rather than QOM
path and the fix might not be entirely trivial although should not be
too hard.

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Daniel P. Berrangé]

On Tue, Feb 14, 2023 at 10:25:22AM +0100, Peter Krempa wrote:
> On Tue, Feb 14, 2023 at 09:54:22 +0100, Markus Armbruster wrote:
> > Daniel P. Berrangé <berrange@redhat.com> writes:
> > 
> > > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > >> The device field is redundant, because QOM path always include device
> > >> ID when this ID exist.
> > >
> > > The flipside to that view is that applications configuring QEMU are
> > > specifying the device ID for -device (CLI) / device_add (QMP) and
> > > not the QOM path. IOW, the device ID is the more interesting field
> > > than QOM path, so feels like the wrong one to be dropping.
> > 
> > QOM path is a reliable way to identify a device.  Device ID isn't:
> > devices need not have one.  Therefore, dropping the QOM path would be
> > wrong.
> > 
> > > Is there any real benefit to dropping this ? 
> > 
> > The device ID is a trap for the unwary: relying on it is fine until you
> > run into a scenario where you have to deal with devices lacking IDs.
> 
> Note that libvirt's code is still using the 'device' bit rather than QOM
> path and the fix might not be entirely trivial although should not be
> too hard.

What's the documented way to construct a QOM path, given only an ID  as
input ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Feb 14, 2023 at 10:25:22AM +0100, Peter Krempa wrote:
>> On Tue, Feb 14, 2023 at 09:54:22 +0100, Markus Armbruster wrote:
>> > Daniel P. Berrangé <berrange@redhat.com> writes:
>> > 
>> > > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> > >> The device field is redundant, because QOM path always include device
>> > >> ID when this ID exist.
>> > >
>> > > The flipside to that view is that applications configuring QEMU are
>> > > specifying the device ID for -device (CLI) / device_add (QMP) and
>> > > not the QOM path. IOW, the device ID is the more interesting field
>> > > than QOM path, so feels like the wrong one to be dropping.
>> > 
>> > QOM path is a reliable way to identify a device.  Device ID isn't:
>> > devices need not have one.  Therefore, dropping the QOM path would be
>> > wrong.
>> > 
>> > > Is there any real benefit to dropping this ? 
>> > 
>> > The device ID is a trap for the unwary: relying on it is fine until you
>> > run into a scenario where you have to deal with devices lacking IDs.
>> 
>> Note that libvirt's code is still using the 'device' bit rather than QOM
>> path and the fix might not be entirely trivial although should not be
>> too hard.
>
> What's the documented way to construct a QOM path, given only an ID  as
> input ?

QOM paths a gap in our documentation, even though the composition tree
structure has been stable since day one, and is de facto ABI.

Short answer: "/machine/peripheral/ID".

Long answer follows.

We have three "containers" under /machine that serve as parents for
devices:

* /machine/peripheral/

  Parent of user-created devices with ID.  Children are named "ID".

  Put there by qdev_set_id(), called from qdev_device_add_from_qdict().

  On "user-created": Nothing stops board code to abuse qdev_set_id() for
  onboard devices, directly or indirectly, but it really, really
  shouldn't.

* /machine/peripheral-anon/

  Parent of user-created devices without ID.  Children are named
  "device[N]", where N counts up from zero.

  Put there by qdev_set_id(), called from qdev_device_add_from_qdict().

  Again, abuse by board code is possible, but would be wrong.

  Beware: a particular device's N changes when the set of devices
  created before it grows or shrinks.  Messing with the machine type can
  change it (different onboard devices).

* /machine/unattached/

  Surrogate parent of onboard devices created without a parent.

  Put there by device_set_realized() (general case),
  qdev_connect_gpio_out_named() (input pins) , memory_region_do_init()
  (memory regions), qemu_create_machine() (the main sysbus).

  I believe this container was created as a convenience, so we don't
  have to retrofit parents to existing code.  Probably abused ever
  since.

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Philippe Mathieu-Daudé]

On 14/2/23 12:49, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Tue, Feb 14, 2023 at 10:25:22AM +0100, Peter Krempa wrote:
>>> On Tue, Feb 14, 2023 at 09:54:22 +0100, Markus Armbruster wrote:
>>>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>>>
>>>>> On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> The device field is redundant, because QOM path always include device
>>>>>> ID when this ID exist.
>>>>>
>>>>> The flipside to that view is that applications configuring QEMU are
>>>>> specifying the device ID for -device (CLI) / device_add (QMP) and
>>>>> not the QOM path. IOW, the device ID is the more interesting field
>>>>> than QOM path, so feels like the wrong one to be dropping.
>>>>
>>>> QOM path is a reliable way to identify a device.  Device ID isn't:
>>>> devices need not have one.  Therefore, dropping the QOM path would be
>>>> wrong.
>>>>
>>>>> Is there any real benefit to dropping this ?
>>>>
>>>> The device ID is a trap for the unwary: relying on it is fine until you
>>>> run into a scenario where you have to deal with devices lacking IDs.
>>>
>>> Note that libvirt's code is still using the 'device' bit rather than QOM
>>> path and the fix might not be entirely trivial although should not be
>>> too hard.
>>
>> What's the documented way to construct a QOM path, given only an ID  as
>> input ?
> 
> QOM paths a gap in our documentation, even though the composition tree
> structure has been stable since day one, and is de facto ABI.
> 
> Short answer: "/machine/peripheral/ID".
> 
> Long answer follows.
> 
> We have three "containers" under /machine that serve as parents for
> devices:
> 
> * /machine/peripheral/
> 
>    Parent of user-created devices with ID.  Children are named "ID".
> 
>    Put there by qdev_set_id(), called from qdev_device_add_from_qdict().
> 
>    On "user-created": Nothing stops board code to abuse qdev_set_id() for
>    onboard devices, directly or indirectly, but it really, really
>    shouldn't.
> 
> * /machine/peripheral-anon/
> 
>    Parent of user-created devices without ID.  Children are named
>    "device[N]", where N counts up from zero.
> 
>    Put there by qdev_set_id(), called from qdev_device_add_from_qdict().
> 
>    Again, abuse by board code is possible, but would be wrong.
> 
>    Beware: a particular device's N changes when the set of devices
>    created before it grows or shrinks.  Messing with the machine type can
>    change it (different onboard devices).
> 
> * /machine/unattached/
> 
>    Surrogate parent of onboard devices created without a parent.
> 
>    Put there by device_set_realized() (general case),
>    qdev_connect_gpio_out_named() (input pins) , memory_region_do_init()
>    (memory regions), qemu_create_machine() (the main sysbus).
> 
>    I believe this container was created as a convenience, so we don't
>    have to retrofit parents to existing code.  Probably abused ever
>    since.

Are you suggesting this is a stable interface and we can not move
devices (like from /machine/unattached/ to /machine/peripheral/)
without going thru the deprecation process?

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Markus Armbruster]

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

> On 14/2/23 12:49, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:

[...]

>>> What's the documented way to construct a QOM path, given only an ID  as
>>> input ?
>> 
>> QOM paths a gap in our documentation, even though the composition tree
>> structure has been stable since day one, and is de facto ABI.
>> 
>> Short answer: "/machine/peripheral/ID".
>> 
>> Long answer follows.
>> 
>> We have three "containers" under /machine that serve as parents for
>> devices:
>> 
>> * /machine/peripheral/
>> 
>>   Parent of user-created devices with ID.  Children are named "ID".
>> 
>>   Put there by qdev_set_id(), called from qdev_device_add_from_qdict().
>> 
>>   On "user-created": Nothing stops board code to abuse qdev_set_id() for
>>   onboard devices, directly or indirectly, but it really, really
>>   shouldn't.
>> 
>> * /machine/peripheral-anon/
>> 
>>   Parent of user-created devices without ID.  Children are named
>>   "device[N]", where N counts up from zero.
>> 
>>   Put there by qdev_set_id(), called from qdev_device_add_from_qdict().
>> 
>>   Again, abuse by board code is possible, but would be wrong.
>> 
>>   Beware: a particular device's N changes when the set of devices
>>   created before it grows or shrinks.  Messing with the machine type can
>>   change it (different onboard devices).
>> 
>> * /machine/unattached/
>> 
>>   Surrogate parent of onboard devices created without a parent.
>> 
>>   Put there by device_set_realized() (general case),
>>   qdev_connect_gpio_out_named() (input pins) , memory_region_do_init()
>>   (memory regions), qemu_create_machine() (the main sysbus).
>> 
>>   I believe this container was created as a convenience, so we don't
>>   have to retrofit parents to existing code.  Probably abused ever
>>   since.
>
> Are you suggesting this is a stable interface and we can not move
> devices (like from /machine/unattached/ to /machine/peripheral/)
> without going thru the deprecation process?

Difficult question!

The point of not changing interfaces incompatibly without a grace period
/ deprecation process is not breaking users of the interface.

When an interface has always worked a certain way, its users may well
depend on it, whether it's documented or not.

The question to ask is always "will this break users?"

For documented aspects, we generally assume it will.  Doesn't mean we
can simply assume "won't" for undocumented aspects.

Does this make sense?

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Philippe Mathieu-Daudé]

On 14/2/23 13:17, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> On 14/2/23 12:49, Markus Armbruster wrote:
>>> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> [...]
> 
>>>> What's the documented way to construct a QOM path, given only an ID  as
>>>> input ?
>>>
>>> QOM paths a gap in our documentation, even though the composition tree
>>> structure has been stable since day one, and is de facto ABI.
>>>
>>> Short answer: "/machine/peripheral/ID".
>>>
>>> Long answer follows.
>>>
>>> We have three "containers" under /machine that serve as parents for
>>> devices:
>>>
>>> * /machine/peripheral/
>>>
>>>    Parent of user-created devices with ID.  Children are named "ID".
>>>
>>>    Put there by qdev_set_id(), called from qdev_device_add_from_qdict().
>>>
>>>    On "user-created": Nothing stops board code to abuse qdev_set_id() for
>>>    onboard devices, directly or indirectly, but it really, really
>>>    shouldn't.
>>>
>>> * /machine/peripheral-anon/
>>>
>>>    Parent of user-created devices without ID.  Children are named
>>>    "device[N]", where N counts up from zero.
>>>
>>>    Put there by qdev_set_id(), called from qdev_device_add_from_qdict().
>>>
>>>    Again, abuse by board code is possible, but would be wrong.
>>>
>>>    Beware: a particular device's N changes when the set of devices
>>>    created before it grows or shrinks.  Messing with the machine type can
>>>    change it (different onboard devices).
>>>
>>> * /machine/unattached/
>>>
>>>    Surrogate parent of onboard devices created without a parent.
>>>
>>>    Put there by device_set_realized() (general case),
>>>    qdev_connect_gpio_out_named() (input pins) , memory_region_do_init()
>>>    (memory regions), qemu_create_machine() (the main sysbus).
>>>
>>>    I believe this container was created as a convenience, so we don't
>>>    have to retrofit parents to existing code.  Probably abused ever
>>>    since.
>>
>> Are you suggesting this is a stable interface and we can not move
>> devices (like from /machine/unattached/ to /machine/peripheral/)
>> without going thru the deprecation process?
> 
> Difficult question!
> 
> The point of not changing interfaces incompatibly without a grace period
> / deprecation process is not breaking users of the interface.
> 
> When an interface has always worked a certain way, its users may well
> depend on it, whether it's documented or not.
> 
> The question to ask is always "will this break users?"
> 
> For documented aspects, we generally assume it will.  Doesn't mean we
> can simply assume "won't" for undocumented aspects.
> 
> Does this make sense?

Yes, but I never considered the QOM paths as a stable interface...
I'm very surprised.

"Automatically assigned to /machine/unattached/" doesn't seem
quite stable...

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Markus Armbruster]

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

> On 14/2/23 13:17, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>>> On 14/2/23 12:49, Markus Armbruster wrote:
>>>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> [...]
>> 
>>>>> What's the documented way to construct a QOM path, given only an ID  as
>>>>> input ?
>>>>
>>>> QOM paths a gap in our documentation, even though the composition tree
>>>> structure has been stable since day one, and is de facto ABI.
>>>>
>>>> Short answer: "/machine/peripheral/ID".
>>>>
>>>> Long answer follows.
>>>>
>>>> We have three "containers" under /machine that serve as parents for
>>>> devices:
>>>>
>>>> * /machine/peripheral/
>>>>
>>>>    Parent of user-created devices with ID.  Children are named "ID".
>>>>
>>>>    Put there by qdev_set_id(), called from qdev_device_add_from_qdict().
>>>>
>>>>    On "user-created": Nothing stops board code to abuse qdev_set_id() for
>>>>    onboard devices, directly or indirectly, but it really, really
>>>>    shouldn't.
>>>>
>>>> * /machine/peripheral-anon/
>>>>
>>>>    Parent of user-created devices without ID.  Children are named
>>>>    "device[N]", where N counts up from zero.
>>>>
>>>>    Put there by qdev_set_id(), called from qdev_device_add_from_qdict().
>>>>
>>>>    Again, abuse by board code is possible, but would be wrong.
>>>>
>>>>    Beware: a particular device's N changes when the set of devices
>>>>    created before it grows or shrinks.  Messing with the machine type can
>>>>    change it (different onboard devices).
>>>>
>>>> * /machine/unattached/
>>>>
>>>>    Surrogate parent of onboard devices created without a parent.
>>>>
>>>>    Put there by device_set_realized() (general case),
>>>>    qdev_connect_gpio_out_named() (input pins) , memory_region_do_init()
>>>>    (memory regions), qemu_create_machine() (the main sysbus).
>>>>
>>>>    I believe this container was created as a convenience, so we don't
>>>>    have to retrofit parents to existing code.  Probably abused ever
>>>>    since.
>>>
>>> Are you suggesting this is a stable interface and we can not move
>>> devices (like from /machine/unattached/ to /machine/peripheral/)
>>> without going thru the deprecation process?
>> 
>> Difficult question!
>> 
>> The point of not changing interfaces incompatibly without a grace period
>> / deprecation process is not breaking users of the interface.
>> 
>> When an interface has always worked a certain way, its users may well
>> depend on it, whether it's documented or not.
>> 
>> The question to ask is always "will this break users?"
>> 
>> For documented aspects, we generally assume it will.  Doesn't mean we
>> can simply assume "won't" for undocumented aspects.
>> 
>> Does this make sense?
>
> Yes, but I never considered the QOM paths as a stable interface...
> I'm very surprised.

I think it's a gray area.

For a good part of the QMP interface, we make an effort to review and
document, and to spell out what is stable and what isn't.  Sadly, QOM
and qdev are exceptions.

Properties are an essential part of the QMP interface, yet they are
virtually undocumented: closest we have is output of "-device
TYPENAME,help", which is utterly inadequate.  There is no systematic
review.  We've never been quite clear on which properties are part of
the stable interface.

QOM paths are a much less prominent part of the QMP interface, but they
are a part.  The structure of the QOM composition tree is undocumented.
Are they part of the stable interface?  Anybody's guess.  I figure they
weren't intended to be stable interace.  But then a QOM path is the only
way to device_del a device without ID.  Gray area.

Moreover, Hyrum's law[*] can catch up with us any time.

> "Automatically assigned to /machine/unattached/" doesn't seem
> quite stable...

The practical difficulties in (ab)using these push them towards the
unstable end of the gray area.


[*] "With a sufficient number of users of an API, it does not matter
what you promise in the contract: all observable behaviors of your
system will be depended on by somebody."

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Daniel P. Berrangé <berrange@redhat.com> writes:
>
>> On Tue, Feb 14, 2023 at 10:25:22AM +0100, Peter Krempa wrote:
>>> On Tue, Feb 14, 2023 at 09:54:22 +0100, Markus Armbruster wrote:
>>> > Daniel P. Berrangé <berrange@redhat.com> writes:
>>> > 
>>> > > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> > >> The device field is redundant, because QOM path always include device
>>> > >> ID when this ID exist.
>>> > >
>>> > > The flipside to that view is that applications configuring QEMU are
>>> > > specifying the device ID for -device (CLI) / device_add (QMP) and
>>> > > not the QOM path. IOW, the device ID is the more interesting field
>>> > > than QOM path, so feels like the wrong one to be dropping.
>>> > 
>>> > QOM path is a reliable way to identify a device.  Device ID isn't:
>>> > devices need not have one.  Therefore, dropping the QOM path would be
>>> > wrong.
>>> > 
>>> > > Is there any real benefit to dropping this ? 
>>> > 
>>> > The device ID is a trap for the unwary: relying on it is fine until you
>>> > run into a scenario where you have to deal with devices lacking IDs.
>>> 
>>> Note that libvirt's code is still using the 'device' bit rather than QOM
>>> path and the fix might not be entirely trivial although should not be
>>> too hard.
>>
>> What's the documented way to construct a QOM path, given only an ID  as
>> input ?
>
> QOM paths a gap in our documentation, even though the composition tree
> structure has been stable since day one, and is de facto ABI.
>
> Short answer: "/machine/peripheral/ID".
>
> Long answer follows.
>
> We have three "containers" under /machine that serve as parents for
> devices:
>
> * /machine/peripheral/
>
>   Parent of user-created devices with ID.  Children are named "ID".
>
>   Put there by qdev_set_id(), called from qdev_device_add_from_qdict().
>
>   On "user-created": Nothing stops board code to abuse qdev_set_id() for
>   onboard devices, directly or indirectly, but it really, really
>   shouldn't.
>
> * /machine/peripheral-anon/
>
>   Parent of user-created devices without ID.  Children are named
>   "device[N]", where N counts up from zero.
>
>   Put there by qdev_set_id(), called from qdev_device_add_from_qdict().
>
>   Again, abuse by board code is possible, but would be wrong.
>
>   Beware: a particular device's N changes when the set of devices
>   created before it grows or shrinks.  Messing with the machine type can
>   change it (different onboard devices).

Correction: that should affect only /machine/unattached/.  Messing with
-device and such affects /machine/peripheral-anon/.

> * /machine/unattached/
>
>   Surrogate parent of onboard devices created without a parent.

Forgot to mention: Children are named "device[N]", where N counts up
from zero.

>   Put there by device_set_realized() (general case),
>   qdev_connect_gpio_out_named() (input pins) , memory_region_do_init()
>   (memory regions), qemu_create_machine() (the main sysbus).
>
>   I believe this container was created as a convenience, so we don't
>   have to retrofit parents to existing code.  Probably abused ever
>   since.

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Daniel P. Berrangé]

On Tue, Feb 14, 2023 at 09:54:22AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >> The device field is redundant, because QOM path always include device
> >> ID when this ID exist.
> >
> > The flipside to that view is that applications configuring QEMU are
> > specifying the device ID for -device (CLI) / device_add (QMP) and
> > not the QOM path. IOW, the device ID is the more interesting field
> > than QOM path, so feels like the wrong one to be dropping.
> 
> QOM path is a reliable way to identify a device.  Device ID isn't:
> devices need not have one.  Therefore, dropping the QOM path would be
> wrong.
> 
> > Is there any real benefit to dropping this ? 
> 
> The device ID is a trap for the unwary: relying on it is fine until you
> run into a scenario where you have to deal with devices lacking IDs.

When a mgmt app is configuring QEMU though, it does it exclusively
with device ID values. If I add a device "-device foo,id=dev0",
and then later hot-unplug it "device_del dev0", it is pretty
reasonable to then expect that the DEVICE_DELETED even will then
include the ID value the app has been using elsewhere.

If the mgmt app is using IDs everywhere when dealing with a device,
then trap effectively doesn't exist for their usage scenario.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Feb 14, 2023 at 09:54:22AM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> >> The device field is redundant, because QOM path always include device
>> >> ID when this ID exist.
>> >
>> > The flipside to that view is that applications configuring QEMU are
>> > specifying the device ID for -device (CLI) / device_add (QMP) and
>> > not the QOM path. IOW, the device ID is the more interesting field
>> > than QOM path, so feels like the wrong one to be dropping.
>> 
>> QOM path is a reliable way to identify a device.  Device ID isn't:
>> devices need not have one.  Therefore, dropping the QOM path would be
>> wrong.
>> 
>> > Is there any real benefit to dropping this ? 
>> 
>> The device ID is a trap for the unwary: relying on it is fine until you
>> run into a scenario where you have to deal with devices lacking IDs.
>
> When a mgmt app is configuring QEMU though, it does it exclusively
> with device ID values. If I add a device "-device foo,id=dev0",
> and then later hot-unplug it "device_del dev0", it is pretty
> reasonable to then expect that the DEVICE_DELETED even will then
> include the ID value the app has been using elsewhere.

The management application would be well advised to use QOM paths with
device_del, because only that works even for devices created by default
(which have no ID), and devices the user created behind the management
application's back.

> If the mgmt app is using IDs everywhere when dealing with a device,
> then trap effectively doesn't exist for their usage scenario.

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Vladimir Sementsov-Ogievskiy]

On 14.02.23 14:57, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Tue, Feb 14, 2023 at 09:54:22AM +0100, Markus Armbruster wrote:
>>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>>
>>>> On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>> The device field is redundant, because QOM path always include device
>>>>> ID when this ID exist.
>>>>
>>>> The flipside to that view is that applications configuring QEMU are
>>>> specifying the device ID for -device (CLI) / device_add (QMP) and
>>>> not the QOM path. IOW, the device ID is the more interesting field
>>>> than QOM path, so feels like the wrong one to be dropping.
>>>
>>> QOM path is a reliable way to identify a device.  Device ID isn't:
>>> devices need not have one.  Therefore, dropping the QOM path would be
>>> wrong.
>>>
>>>> Is there any real benefit to dropping this ?
>>>
>>> The device ID is a trap for the unwary: relying on it is fine until you
>>> run into a scenario where you have to deal with devices lacking IDs.
>>
>> When a mgmt app is configuring QEMU though, it does it exclusively
>> with device ID values. If I add a device "-device foo,id=dev0",
>> and then later hot-unplug it "device_del dev0", it is pretty
>> reasonable to then expect that the DEVICE_DELETED even will then
>> include the ID value the app has been using elsewhere.
> 
> The management application would be well advised to use QOM paths with
> device_del, because only that works even for devices created by default
> (which have no ID), and devices the user created behind the management
> application's back.
> 
>> If the mgmt app is using IDs everywhere when dealing with a device,
>> then trap effectively doesn't exist for their usage scenario.
> 

What if we go one step further and deprecate "id" in device_add / device_del as well?

So that user will have to use qom path also in device_add. We may return an error if user don't specify "machine/peripheral/" prefix.. Or allow to create device with any QOM path?

-- 
Best regards,
Vladimir

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Daniel P. Berrangé]

On Tue, Feb 14, 2023 at 12:57:28PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Feb 14, 2023 at 09:54:22AM +0100, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> >> >> The device field is redundant, because QOM path always include device
> >> >> ID when this ID exist.
> >> >
> >> > The flipside to that view is that applications configuring QEMU are
> >> > specifying the device ID for -device (CLI) / device_add (QMP) and
> >> > not the QOM path. IOW, the device ID is the more interesting field
> >> > than QOM path, so feels like the wrong one to be dropping.
> >> 
> >> QOM path is a reliable way to identify a device.  Device ID isn't:
> >> devices need not have one.  Therefore, dropping the QOM path would be
> >> wrong.
> >> 
> >> > Is there any real benefit to dropping this ? 
> >> 
> >> The device ID is a trap for the unwary: relying on it is fine until you
> >> run into a scenario where you have to deal with devices lacking IDs.
> >
> > When a mgmt app is configuring QEMU though, it does it exclusively
> > with device ID values. If I add a device "-device foo,id=dev0",
> > and then later hot-unplug it "device_del dev0", it is pretty
> > reasonable to then expect that the DEVICE_DELETED even will then
> > include the ID value the app has been using elsewhere.
> 
> The management application would be well advised to use QOM paths with
> device_del, because only that works even for devices created by default
> (which have no ID), and devices the user created behind the management
> application's back.

If an application is using -nodefaults, then the only devices which
exist will be those which are hardwired into the machine, and they
can't be used with device_del anyway as they're hardwired.

So the only reason is to cope with devices created secretly by
the users, and that's a hairy enough problem that most apps won't
even try to cope with it.

At least in terms of the device hotplug area, it feels like we're
adding an extra hurdle for apps to solve a problem that they don't
actually face in practice.

QOM paths are needed in some other QMP commands though, where
there is definite need to refer to devices that are hardwired,
most obviously qom-set/qom-get.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Feb 14, 2023 at 12:57:28PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Tue, Feb 14, 2023 at 09:54:22AM +0100, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >> 
>> >> > On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> >> >> The device field is redundant, because QOM path always include device
>> >> >> ID when this ID exist.
>> >> >
>> >> > The flipside to that view is that applications configuring QEMU are
>> >> > specifying the device ID for -device (CLI) / device_add (QMP) and
>> >> > not the QOM path. IOW, the device ID is the more interesting field
>> >> > than QOM path, so feels like the wrong one to be dropping.
>> >> 
>> >> QOM path is a reliable way to identify a device.  Device ID isn't:
>> >> devices need not have one.  Therefore, dropping the QOM path would be
>> >> wrong.
>> >> 
>> >> > Is there any real benefit to dropping this ? 
>> >> 
>> >> The device ID is a trap for the unwary: relying on it is fine until you
>> >> run into a scenario where you have to deal with devices lacking IDs.
>> >
>> > When a mgmt app is configuring QEMU though, it does it exclusively
>> > with device ID values. If I add a device "-device foo,id=dev0",
>> > and then later hot-unplug it "device_del dev0", it is pretty
>> > reasonable to then expect that the DEVICE_DELETED even will then
>> > include the ID value the app has been using elsewhere.
>> 
>> The management application would be well advised to use QOM paths with
>> device_del, because only that works even for devices created by default
>> (which have no ID), and devices the user created behind the management
>> application's back.
>
> If an application is using -nodefaults, then the only devices which
> exist will be those which are hardwired into the machine, and they
> can't be used with device_del anyway as they're hardwired.

Your trust in the sanity of our board code is touching ;)

> So the only reason is to cope with devices created secretly by
> the users, and that's a hairy enough problem that most apps won't
> even try to cope with it.

Fair enough.

> At least in terms of the device hotplug area, it feels like we're
> adding an extra hurdle for apps to solve a problem that they don't
> actually face in practice.
>
> QOM paths are needed in some other QMP commands though, where
> there is definite need to refer to devices that are hardwired,
> most obviously qom-set/qom-get.

Also query-cpus-fast, query-hotpluggable-cpus, and possibly more I
missed.

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Vladimir Sementsov-Ogievskiy]

On 14.02.23 19:28, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> On Tue, Feb 14, 2023 at 12:57:28PM +0100, Markus Armbruster wrote:
>>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>>
>>>> On Tue, Feb 14, 2023 at 09:54:22AM +0100, Markus Armbruster wrote:
>>>>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>>>>
>>>>>> On Mon, Feb 13, 2023 at 05:01:01PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> The device field is redundant, because QOM path always include device
>>>>>>> ID when this ID exist.
>>>>>>
>>>>>> The flipside to that view is that applications configuring QEMU are
>>>>>> specifying the device ID for -device (CLI) / device_add (QMP) and
>>>>>> not the QOM path. IOW, the device ID is the more interesting field
>>>>>> than QOM path, so feels like the wrong one to be dropping.
>>>>>
>>>>> QOM path is a reliable way to identify a device.  Device ID isn't:
>>>>> devices need not have one.  Therefore, dropping the QOM path would be
>>>>> wrong.
>>>>>
>>>>>> Is there any real benefit to dropping this ?
>>>>>
>>>>> The device ID is a trap for the unwary: relying on it is fine until you
>>>>> run into a scenario where you have to deal with devices lacking IDs.
>>>>
>>>> When a mgmt app is configuring QEMU though, it does it exclusively
>>>> with device ID values. If I add a device "-device foo,id=dev0",
>>>> and then later hot-unplug it "device_del dev0", it is pretty
>>>> reasonable to then expect that the DEVICE_DELETED even will then
>>>> include the ID value the app has been using elsewhere.
>>>
>>> The management application would be well advised to use QOM paths with
>>> device_del, because only that works even for devices created by default
>>> (which have no ID), and devices the user created behind the management
>>> application's back.
>>
>> If an application is using -nodefaults, then the only devices which
>> exist will be those which are hardwired into the machine, and they
>> can't be used with device_del anyway as they're hardwired.
> 
> Your trust in the sanity of our board code is touching ;)
> 
>> So the only reason is to cope with devices created secretly by
>> the users, and that's a hairy enough problem that most apps won't
>> even try to cope with it.
> 
> Fair enough.
> 
>> At least in terms of the device hotplug area, it feels like we're
>> adding an extra hurdle for apps to solve a problem that they don't
>> actually face in practice.
>>
>> QOM paths are needed in some other QMP commands though, where
>> there is definite need to refer to devices that are hardwired,
>> most obviously qom-set/qom-get.
> 
> Also query-cpus-fast, query-hotpluggable-cpus, and possibly more I
> missed.
> 


So, finally, we don't have consensus on deprecating ids?

For me the most strong argument is that if user specify id in device_add, user should get exactly that id in DEVICE_DELETED and other events.

So if deprecate something, we'd better deprecate ids altogether, making users specify full QOM path even in device_add. But that seems quite painful for existing users with no visible benefit.

So, if no objections, I plan to resend with old "optional id & qom_path" designation for devices. We still can do a deprecation in future.

-- 
Best regards,
Vladimir

Re: [PATCH v4 14/16] qapi: deprecate "device" field of DEVICE_* events

[Markus Armbruster]

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


[...]

> So, if no objections, I plan to resend with old "optional id & qom_path" designation for devices. We still can do a deprecation in future.

Yes, please.

[PATCH v4 15/16] 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                  | 175 ++++++++++++++++++++++++++++++++
 include/hw/hotplug.h            |  12 +++
 include/hw/pci/pci.h            |   3 +
 include/hw/pci/pci_bridge.h     |   2 +
 include/hw/pci/pcie.h           |   2 +
 include/hw/pci/shpc.h           |   2 +
 include/monitor/qdev.h          |   5 +
 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                   |  79 ++++++++++++++
 hw/pci/pcie_port.c              |   1 +
 hw/pci/shpc.c                   | 102 ++++++++++++++++++-
 softmmu/qdev-monitor.c          |  39 +++++++
 14 files changed, 445 insertions(+), 5 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index 325ef554f9..b6ad311dd4 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -166,3 +166,178 @@
 { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
   'data': { '*device': { 'type': 'str', 'features': [ 'deprecated' ] },
             'path': 'str' } }
+
+##
+# @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 us 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': 'str',
+            '*addr': 'str',
+            '*child': 'str' } }
+
+##
+# @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/hw/pci/pci.h b/include/hw/pci/pci.h
index b6c9c44527..8ebd0ad51e 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 */
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/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/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 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/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/pcie.c b/hw/pci/pcie.c
index b8c24cf45f..08ac37581e 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,34 @@ 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);
+    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;
+    HotplugInfo *res = g_new(HotplugInfo, 1);
+
+    *res = (HotplugInfo) {
+        .bus = g_strdup(hotplug_ds->canonical_path),
+        .child = g_strdup(dev->canonical_path),
+        .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 = {
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index e7bc7192f1..70447bba08 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,45 @@ 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);
+    DeviceState *hotplug_ds = DEVICE(pci_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 = g_strdup(hotplug_ds->canonical_path),
+        .addr = shpc_idx_to_pci_addr(slot),
+        .child = g_strdup(dev->canonical_path),
+        .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 b8d2c4dadd..95b7d34700 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,11 @@ 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)
+{
+    qapi_event_send_hotplug_state(bus->canonical_path,
+                                  addr, child ? child->canonical_path : NULL,
+                                  changed_state);
+}
-- 
2.34.1

Re: [PATCH v4 15/16] qapi: add HOTPLUG_STATE event

[Philippe Mathieu-Daudé]

On 13/2/23 15:01, 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                  | 175 ++++++++++++++++++++++++++++++++
>   include/hw/hotplug.h            |  12 +++
>   include/hw/pci/pci.h            |   3 +
>   include/hw/pci/pci_bridge.h     |   2 +
>   include/hw/pci/pcie.h           |   2 +
>   include/hw/pci/shpc.h           |   2 +
>   include/monitor/qdev.h          |   5 +
>   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                   |  79 ++++++++++++++
>   hw/pci/pcie_port.c              |   1 +
>   hw/pci/shpc.c                   | 102 ++++++++++++++++++-
>   softmmu/qdev-monitor.c          |  39 +++++++
>   14 files changed, 445 insertions(+), 5 deletions(-)

-ETOOBIG

> +##
> +# @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 us safe to be removed.

s/us/is/

> +#
> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugSHPCSlotState',
> +  'data': [ 'power-only', 'enabled', 'disabled' ] }

Re: [PATCH v4 15/16] qapi: add HOTPLUG_STATE event

[Markus Armbruster]

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

> On 13/2/23 15:01, 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                  | 175 ++++++++++++++++++++++++++++++++
>>   include/hw/hotplug.h            |  12 +++
>>   include/hw/pci/pci.h            |   3 +
>>   include/hw/pci/pci_bridge.h     |   2 +
>>   include/hw/pci/pcie.h           |   2 +
>>   include/hw/pci/shpc.h           |   2 +
>>   include/monitor/qdev.h          |   5 +
>>   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                   |  79 ++++++++++++++
>>   hw/pci/pcie_port.c              |   1 +
>>   hw/pci/shpc.c                   | 102 ++++++++++++++++++-
>>   softmmu/qdev-monitor.c          |  39 +++++++
>>   14 files changed, 445 insertions(+), 5 deletions(-)
>
> -ETOOBIG

Can't see how this could be split sensibly: it's a single query/event
pair.  If you can, let us know.

Re: [PATCH v4 15/16] qapi: add HOTPLUG_STATE event

[Vladimir Sementsov-Ogievskiy]

On 14.02.23 11:56, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> On 13/2/23 15:01, 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                  | 175 ++++++++++++++++++++++++++++++++
>>>    include/hw/hotplug.h            |  12 +++
>>>    include/hw/pci/pci.h            |   3 +
>>>    include/hw/pci/pci_bridge.h     |   2 +
>>>    include/hw/pci/pcie.h           |   2 +
>>>    include/hw/pci/shpc.h           |   2 +
>>>    include/monitor/qdev.h          |   5 +
>>>    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                   |  79 ++++++++++++++
>>>    hw/pci/pcie_port.c              |   1 +
>>>    hw/pci/shpc.c                   | 102 ++++++++++++++++++-
>>>    softmmu/qdev-monitor.c          |  39 +++++++
>>>    14 files changed, 445 insertions(+), 5 deletions(-)
>>
>> -ETOOBIG
> 
> Can't see how this could be split sensibly: it's a single query/event
> pair.  If you can, let us know.
> 

I can split necessary handlers in device code to a separate preparation patch.

-- 
Best regards,
Vladimir

[PATCH v4 16/16] 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 | 13 +++++++++++++
 hw/pci/pcie.c  | 13 +++++++++++++
 hw/pci/shpc.c  | 12 ++++++++++++
 3 files changed, 38 insertions(+)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index b6ad311dd4..2143bb2792 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -341,3 +341,16 @@
 { '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.
+#
+# @path: the hotplugged device's QOM path
+#
+# Since: 8.0
+##
+{ 'event': 'DEVICE_ON',
+  'data': { 'path': 'str' } }
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 08ac37581e..efa90e9e6e 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -47,6 +47,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 +823,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->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 70447bba08..105be8f1c1 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->canonical_path);
+    }
 }
 
 static void shpc_command(PCIDevice *d)
-- 
2.34.1

Re: [PATCH v4 16/16] qapi: introduce DEVICE_ON event

[Philippe Mathieu-Daudé]

On 13/2/23 15:01, 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 | 13 +++++++++++++
>   hw/pci/pcie.c  | 13 +++++++++++++
>   hw/pci/shpc.c  | 12 ++++++++++++
>   3 files changed, 38 insertions(+)
> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index b6ad311dd4..2143bb2792 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -341,3 +341,16 @@
>   { '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.
> +#
> +# @path: the hotplugged device's QOM path
> +#
> +# Since: 8.0
> +##
> +{ 'event': 'DEVICE_ON',
> +  'data': { 'path': 'str' } }

Could 'qom-path' or 'canonical-path' be more meaningful here?

> @@ -816,6 +823,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->canonical_path);
> +    }

Re: [PATCH v4 16/16] qapi: introduce DEVICE_ON event

[Markus Armbruster]

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

> On 13/2/23 15:01, 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 | 13 +++++++++++++
>>   hw/pci/pcie.c  | 13 +++++++++++++
>>   hw/pci/shpc.c  | 12 ++++++++++++
>>   3 files changed, 38 insertions(+)
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index b6ad311dd4..2143bb2792 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -341,3 +341,16 @@
>>   { '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.
>> +#
>> +# @path: the hotplugged device's QOM path
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'event': 'DEVICE_ON',
>> +  'data': { 'path': 'str' } }
>
> Could 'qom-path' or 'canonical-path' be more meaningful here?

@qom-path would be clearer, no doubt.  But @path is consistent with the
closely related DEVICE_DELETED event.

>> @@ -816,6 +823,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->canonical_path);
>> +    }

Re: [PATCH v4 16/16] qapi: introduce DEVICE_ON event

[Vladimir Sementsov-Ogievskiy]

On 14.02.23 11:58, Markus Armbruster wrote:
> Philippe Mathieu-Daudé<philmd@linaro.org>  writes:
> 
>> On 13/2/23 15:01, 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 | 13 +++++++++++++
>>>    hw/pci/pcie.c  | 13 +++++++++++++
>>>    hw/pci/shpc.c  | 12 ++++++++++++
>>>    3 files changed, 38 insertions(+)
>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>> index b6ad311dd4..2143bb2792 100644
>>> --- a/qapi/qdev.json
>>> +++ b/qapi/qdev.json
>>> @@ -341,3 +341,16 @@
>>>    { '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.
>>> +#
>>> +# @path: the hotplugged device's QOM path
>>> +#
>>> +# Since: 8.0
>>> +##
>>> +{ 'event': 'DEVICE_ON',
>>> +  'data': { 'path': 'str' } }
>> Could 'qom-path' or 'canonical-path' be more meaningful here?
> @qom-path would be clearer, no doubt.  But @path is consistent with the
> closely related DEVICE_DELETED event.
> 

If we agree to depreacte "device", we probably can deprecate "path" too, and add duplicating "qom-path" to other events. So, we'll have a consistent and clear API.

-- 
Best regards,
Vladimir