[2/2] qapi: introduce DEVICE_POWER_ON for SHPC hotplug

Hi all! That's an RFC patch.

The problem is that SHPC protocol says that power-led is blinking for 5
seconds before actual turning-on the device. If we call device-del
during this time the attention button press is ignored and we never get
DEVICE_DELETED event, which is unexpected for the user.

I suggest add a pair for DEVICE_DELETED: DEVICE_POWER_ON. So user
should wait for DEVICE_POWER_ON after device-add before making any
other operations with the device (incluing device-del).

What I'm unsure is what about other types of hotplug - PCIE and
ACPI.. Do they suffer from similar problems? Seems not.. Should we sent
for them this event at some moment of should the user be aware of which
kind of hotplug is in use to determine to wait for the DEVICE_POWER_ON
or not to wait.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/pci/shpc.c  | 16 ++++++++++++++++
 qapi/qdev.json | 23 +++++++++++++++++++++++
 2 files changed, 39 insertions(+)

On Wed, Nov 16, 2022 at 07:12:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all! That's an RFC patch.
> 
> The problem is that SHPC protocol says that power-led is blinking for 5
> seconds before actual turning-on the device. If we call device-del
> during this time the attention button press is ignored and we never get
> DEVICE_DELETED event, which is unexpected for the user.
> 
> I suggest add a pair for DEVICE_DELETED: DEVICE_POWER_ON. So user
> should wait for DEVICE_POWER_ON after device-add before making any
> other operations with the device (incluing device-del).
> 
> What I'm unsure is what about other types of hotplug - PCIE and
> ACPI.. Do they suffer from similar problems?

I didn't yet look at this patchset deeply (we are in freeze anyway)
but PCIE is substancially same as SHPC.

Take a look at Gerd's "improve native hotplug for pcie root ports"
same kind of approach probably works for SHPC.

> Seems not.. Should we sent
> for them this event at some moment of should the user be aware of which
> kind of hotplug is in use to determine to wait for the DEVICE_POWER_ON
> or not to wait.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  hw/pci/shpc.c  | 16 ++++++++++++++++
>  qapi/qdev.json | 23 +++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
> index ba241e2818..7c53971c1c 100644
> --- a/hw/pci/shpc.c
> +++ b/hw/pci/shpc.c
> @@ -1,5 +1,6 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-events-qdev.h"
>  #include "qemu/host-utils.h"
>  #include "qemu/range.h"
>  #include "qemu/error-report.h"
> @@ -273,6 +274,18 @@ static void shpc_free_devices_in_slot(SHPCDevice *shpc, int slot)
>      }
>  }
>  
> +static void shpc_devices_power_on_in_slot(SHPCDevice *shpc, int slot)
> +{
> +    int devfn;
> +    PCIDevice *dev;
> +
> +    FOR_EACH_DEVICE_IN_SLOT(shpc, slot, dev, devfn) {
> +        DeviceState *ds = DEVICE(dev);
> +
> +        qapi_event_send_device_power_on(!!ds->id, ds->id, ds->canonical_path);
> +    }
> +}
> +
>  static void shpc_slot_command(SHPCDevice *shpc, uint8_t target,
>                                uint8_t state, uint8_t power, uint8_t attn)
>  {
> @@ -291,6 +304,9 @@ static void shpc_slot_command(SHPCDevice *shpc, uint8_t target,
>      switch (power) {
>      case SHPC_LED_NO:
>          break;
> +    case SHPC_LED_ON:
> +        shpc_devices_power_on_in_slot(shpc, slot);
> +        __attribute__ ((fallthrough));
>      default:
>          /* TODO: send event to monitor */
>          shpc_set_status(shpc, slot, power, SHPC_SLOT_PWR_LED_MASK);
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2708fb4e99..360dcf8ba6 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -158,3 +158,26 @@
>  ##
>  { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @DEVICE_POWER_ON:
> +#
> +# Emitted whenever power is on for the devices plugged into pci slot.
> +# At this point it's safe to remove the device.
> +#
> +# @device: the device's ID if it has one
> +#
> +# @path: the device's QOM path
> +#
> +# Since: 7.2
> +#
> +# Example:
> +#
> +# <- { "event": "DEVICE_POWER_ON",
> +#      "data": { "device": "virtio-disk-0",
> +#                "path": "/machine/peripheral/virtio-disk-0" },
> +#      "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +#
> +##
> +{ 'event': 'DEVICE_POWER_ON',
> +  'data': { '*device': 'str', 'path': 'str' } }
> -- 
> 2.34.1

On 11/16/22 19:26, Michael S. Tsirkin wrote:
> On Wed, Nov 16, 2022 at 07:12:34PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all! That's an RFC patch.
>>
>> The problem is that SHPC protocol says that power-led is blinking for 5
>> seconds before actual turning-on the device. If we call device-del
>> during this time the attention button press is ignored and we never get
>> DEVICE_DELETED event, which is unexpected for the user.
>>
>> I suggest add a pair for DEVICE_DELETED: DEVICE_POWER_ON. So user
>> should wait for DEVICE_POWER_ON after device-add before making any
>> other operations with the device (incluing device-del).
>>
>> What I'm unsure is what about other types of hotplug - PCIE and
>> ACPI.. Do they suffer from similar problems?
> I didn't yet look at this patchset deeply (we are in freeze anyway)
> but PCIE is substancially same as SHPC.
> 
> Take a look at Gerd's "improve native hotplug for pcie root ports"
> same kind of approach probably works for SHPC.

Looking at it. Yes, I think this approach is OK, thanks for the link.

I doubt now that we really need new event. Instead I can update SHPC to return an error when trying to unplug with blinking power indicator (like 81124b3c7a5dae "pcie: add power indicator blink check").