Re: [PATCH v7 2/4] qapi: add DEVICE_ON and query-hotplug infrastructure

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Vladimir,

On 21/4/23 12:32, 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.
> 
> Motivations:
> 1. To be sure that device is "accepted" by guest. Guest may ignore
> hotplugged device for some reason (for example during OS booting).
> Management wants to catch this and handle the problem, instead of
> silent assume that everything is OK. So, if we don't get the event by
> some timeout, we can report an error, try to unplug/plug the disk again
> or do some other things to handle the problem.
> 
> 2. The device can't be removed (by blockdev-del) while power indicator
> of hotplug controller is blinking (QEMU reports "guest is busy (power
> indicator blinking)"). So, management should avoid removing the device
> until it gets the DEVICE_ON event.
> (Probably, better solution for this point is to automatically postpone
> deletion until power indicator stops blinking)
> 
> 3. Also, management tool may make a GUI visualization of power
> indicator with help of this event.
> 
> New query-hotplug command in additon to "device-on" state also provides
> SHPC/PCIe-native specific hotplug controller properties (like leds)
> that may help to determine real state of hotplug controller. That may
> help to get additional information for further debugging when DEVICE_ON
> / DEVICE_DELETED not come in time as expected.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>   hw/core/hotplug.c      |  13 ++++
>   include/hw/hotplug.h   |  12 ++++
>   include/hw/qdev-core.h |   1 +
>   include/monitor/qdev.h |   2 +
>   qapi/qdev.json         | 144 +++++++++++++++++++++++++++++++++++++++++
>   softmmu/qdev-monitor.c |  41 ++++++++++++
>   6 files changed, 213 insertions(+)
> 
> 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,

What about hotplug_handler_get_state()?

> +                                               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/include/hw/hotplug.h b/include/hw/hotplug.h
> index a9840ed485..9c43e1263b 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"
>   
> @@ -60,6 +61,8 @@ struct HotplugHandlerClass {
>       hotplug_fn unplug_request;
>       hotplug_fn unplug;
>       bool (*is_hotpluggable_bus)(HotplugHandler *plug_handler, BusState *bus);
> +    HotplugInfo *(*get_hotplug_state)(HotplugHandler *plug_handler,
> +                                      DeviceState *plugged_dev, Error **errp);
>   };
>   
>   /**
> @@ -96,4 +99,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/qdev-core.h b/include/hw/qdev-core.h
> index bd50ad5ee1..63889e41c0 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -180,6 +180,7 @@ struct DeviceState {
>       char *id;
>       char *canonical_path;
>       bool realized;
> +    bool device_on_sent; /* set once by SHPC or PCIE-hotplug */

This seems to belong to the next patch (not used here).
Anyhow (besides the field misses its description) from the name
I can't figure out what this is about. Probably too generic name
IMHO.

>       bool pending_deleted_event;
>       int64_t pending_deleted_expires_ms;
>       QDict *opts;
> diff --git a/include/monitor/qdev.h b/include/monitor/qdev.h
> index 1d57bf6577..c1c8798e89 100644
> --- a/include/monitor/qdev.h
> +++ b/include/monitor/qdev.h
> @@ -36,4 +36,6 @@ DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>    */
>   const char *qdev_set_id(DeviceState *dev, char *id, Error **errp);
>   
> +void qdev_hotplug_device_on_event(DeviceState *dev);
> +
>   #endif
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 135cd81586..ffd20c43e0 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -173,3 +173,147 @@
>   #
>   ##
>   { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
> +
> +##
> +# @LedActivity:
> +#
> +# Three-state led indicator state.
> +#
> +# @on: Indicator is on.
> +#
> +# @blink: Indicator is blinking.
> +#
> +# @off: Indicator is off.
> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'LedActivity',
> +  'data': [ 'on', 'blink', 'off' ] }

Possibly useful enough to add in a new qapi/led.json.

> +##
> +# @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 is safe to be removed.
> +#
> +# Since: 8.1
> +##
> +{ '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.1
> +##
> +{ '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.1
> +##
> +{ 'struct': 'HotplugSHPCState',
> +  'base': 'HotplugBaseState',
> +  'data': { '*slot-state': 'HotplugSHPCSlotState' } }
> +
> +##
> +# @HotplugPCIeNativeState:
> +#
> +# PCIe Native hotplug slot state.

Doesn't this belong to qapi/pci.json?

> +#
> +# @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.1
> +##
> +{ 'struct': 'HotplugPCIeNativeState',
> +  'base': 'HotplugBaseState',
> +  'data': { '*power-on': 'bool' } }
> +
> +##
> +# @HotplugType:
> +#
> +# Type of hotplug controller / provider.
> +#
> +# @shpc: Standard Hot Plug Controller
> +#
> +# @pcie-native: PCIe Native hotplug

Ditto.

> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'HotplugType',
> +  'data': ['shpc', 'pcie-native'] }
> +
> +##
> +# @HotplugInfo:
> +#
> +# Generic hotplug slot state.
> +#
> +# @type: type of the hotplug (shpc or pcie-native)
> +#
> +# @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
> +#
> +# @device-on: Device is powered-on by guest. This state changes at most
> +#             once for the device and corresponds to DEVICE_ON event.
> +#
> +# Single: 8.1
> +##
> +{ 'union': 'HotplugInfo',
> +  'base': { 'type': 'HotplugType',
> +            'bus': 'DeviceAndPath',
> +            '*addr': 'str',
> +            'child': 'DeviceAndPath',
> +            'device-on': 'bool' },
> +  'discriminator': 'type',
> +  'data': { 'shpc': 'HotplugSHPCState',
> +            'pcie-native': 'HotplugPCIeNativeState' } }
> +
> +##
> +# @query-hotplug:
> +#
> +# Query the state of hotplug controller.
> +#
> +# Since: 8.1
> +##
> +{ '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.1
> +##
> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' }