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 <vsement...@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
