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

[Michael S. Tsirkin]

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

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

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


> ---
>  qapi/qdev.json | 10 ++++++++++
>  hw/pci/pcie.c  | 14 ++++++++++++++
>  hw/pci/shpc.c  | 12 ++++++++++++
>  3 files changed, 36 insertions(+)
> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 6f2d8d6647..116a8a7de8 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -348,3 +348,13 @@
>  { 'command': 'query-hotplug',
>    'data': { 'id': 'str' },
>    'returns': 'HotplugInfo' }
> +
> +##
> +# @DEVICE_ON:
> +#
> +# Emitted whenever the device insertion completion is acknowledged by the 
> guest.
> +# For now only emitted for SHPC and PCIe-native hotplug.
> +#
> +# Since: 8.0
> +##
> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 636f962a23..4297e4e8dc 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -22,6 +22,7 @@
>  
>  #include "monitor/qdev.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-events-qdev.h"
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci/pcie.h"
>  #include "hw/pci/msix.h"
> @@ -47,6 +48,13 @@ static bool pcie_sltctl_powered_off(uint16_t sltctl)
>          && (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF;
>  }
>  
> +static bool pcie_sltctl_powered_on(uint16_t sltctl)
> +{
> +    return (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_ON &&
> +        (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_ON &&
> +        (sltctl & PCI_EXP_SLTCTL_AIC) == PCI_EXP_SLTCTL_ATTN_IND_OFF;
> +}
> +
>  static LedActivity pcie_led_state_to_qapi(uint16_t value)
>  {
>      switch (value) {
> @@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
>          qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, 
> &changed_state);
>      }
>  
> +    if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_on(val) &&
> +        !pcie_sltctl_powered_on(old_slt_ctl) && child_dev)
> +    {
> +        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
> +    }
> +
>      /*
>       * If the slot is populated, power indicator is off and power
>       * controller is off, it is safe to detach the devices.
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index 6a4f93949d..380b2b83b3 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -299,6 +299,12 @@ static bool shpc_slot_is_off(uint8_t state, uint8_t 
> power, uint8_t attn)
>      return state == SHPC_STATE_DISABLED && power == SHPC_LED_OFF;
>  }
>  
> +static bool shpc_slot_is_on(uint8_t state, uint8_t power, uint8_t attn)
> +{
> +    return state == SHPC_STATE_ENABLED && power == SHPC_LED_ON &&
> +        attn == SHPC_LED_OFF;
> +}
> +
>  static void shpc_slot_command(PCIDevice *d, uint8_t target,
>                                uint8_t state, uint8_t power, uint8_t attn)
>  {
> @@ -366,6 +372,12 @@ static void shpc_slot_command(PCIDevice *d, uint8_t 
> target,
>              SHPC_SLOT_EVENT_MRL |
>              SHPC_SLOT_EVENT_PRESENCE;
>      }
> +
> +    if (!shpc_slot_is_on(old_state, old_power, old_attn) &&
> +        shpc_slot_is_on(state, power, attn) && child_dev)
> +    {
> +        qapi_event_send_device_on(child_dev->id, child_dev->canonical_path);
> +    }
>  }
>  
>  static void shpc_command(PCIDevice *d)
> -- 
> 2.34.1