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

[Michael S. Tsirkin]

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