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

[Michael S. Tsirkin]

On Tue, Mar 07, 2023 at 07:05:35PM +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.
> 
> 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.
> 
> As a counter-part add query-hotplug command, that shows "device-on"
> state as well as some addtional information.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

>From commit log it sounds you are just adding one new event.
In fact there's more new stuff in qdev.json

Pls document in commit log too.


> ---
>  qapi/qdev.json         | 144 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/hotplug.h   |  12 ++++
>  include/hw/qdev-core.h |   1 +
>  include/monitor/qdev.h |   2 +
>  hw/core/hotplug.c      |  13 ++++
>  softmmu/qdev-monitor.c |  41 ++++++++++++
>  6 files changed, 213 insertions(+)
> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 135cd81586..e37286da78 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.0
> +##
> +{ 'enum': 'LedActivity',
> +  'data': [ 'on', 'blink', 'off' ] }
> +
> +##
> +# @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.0
> +##
> +{ '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.0
> +##
> +{ '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.0
> +##
> +{ 'struct': 'HotplugSHPCState',
> +  'base': 'HotplugBaseState',
> +  'data': { '*slot-state': 'HotplugSHPCSlotState' } }
> +
> +##
> +# @HotplugPCIeNativeState:
> +#
> +# PCIe Native hotplug slot state.
> +#
> +# @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.0
> +##
> +{ 'struct': 'HotplugPCIeNativeState',
> +  'base': 'HotplugBaseState',
> +  'data': { '*power-on': 'bool' } }
> +
> +##
> +# @HotplugType:
> +#
> +# Type of hotplug controller / provider.
> +#
> +# @shpc: Standard Hot Plug Controller
> +#
> +# @pcie-native: PCIe Native hotplug
> +#
> +# Since: 8.0
> +##
> +{ '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.0
> +##
> +{ '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.0
> +##
> +{ '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/include/hw/hotplug.h b/include/hw/hotplug.h
> index e15f59c8b3..b82261d91e 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"
>  
> @@ -58,6 +59,8 @@ struct HotplugHandlerClass {
>      hotplug_fn plug;
>      hotplug_fn unplug_request;
>      hotplug_fn unplug;
> +    HotplugInfo *(*get_hotplug_state)(HotplugHandler *plug_handler,
> +                                      DeviceState *plugged_dev, Error 
> **errp);
>  };
>  
>  /**
> @@ -94,4 +97,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 f5b3b2f89a..dc2f91a2e2 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 */
>      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/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,
> +                                               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/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index b8d2c4dadd..e4956bbd94 100644
> --- a/softmmu/qdev-monitor.c
> +++ b/softmmu/qdev-monitor.c
> @@ -25,6 +25,7 @@
>  #include "sysemu/arch_init.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-qdev.h"
> +#include "qapi/qapi-events-qdev.h"
>  #include "qapi/qmp/dispatch.h"
>  #include "qapi/qmp/qdict.h"
>  #include "qapi/qmp/qerror.h"
> @@ -956,6 +957,36 @@ void qmp_device_del(const char *id, Error **errp)
>      }
>  }
>  
> +HotplugInfo *qmp_query_hotplug(const char *id, Error **errp)
> +{
> +    DeviceState *dev = find_device_state(id, errp);
> +    HotplugHandler *hotplug_ctrl;
> +
> +    if (!dev) {
> +        return NULL;
> +    }
> +
> +    if (dev->parent_bus && !qbus_is_hotpluggable(dev->parent_bus)) {
> +        error_setg(errp, QERR_BUS_NO_HOTPLUG, dev->parent_bus->name);
> +        return NULL;
> +    }
> +
> +    if (!DEVICE_GET_CLASS(dev)->hotpluggable) {
> +        error_setg(errp, QERR_DEVICE_NO_HOTPLUG,
> +                   object_get_typename(OBJECT(dev)));
> +        return NULL;
> +    }
> +
> +    hotplug_ctrl = qdev_get_hotplug_handler(dev);
> +    /*
> +     * hotpluggable device MUST have HotplugHandler, if it doesn't
> +     * then something is very wrong with it.
> +     */
> +    g_assert(hotplug_ctrl);
> +
> +    return hotplug_handler_get_hotplug_state(hotplug_ctrl, dev, errp);
> +}
> +
>  void hmp_device_add(Monitor *mon, const QDict *qdict)
>  {
>      Error *err = NULL;
> @@ -1146,3 +1177,13 @@ bool qmp_command_available(const QmpCommand *cmd, 
> Error **errp)
>      }
>      return true;
>  }
> +
> +void qdev_hotplug_device_on_event(DeviceState *dev)
> +{
> +    if (dev->device_on_sent) {
> +        return;
> +    }
> +
> +    dev->device_on_sent = true;
> +    qapi_event_send_device_on(dev->id, dev->canonical_path);
> +}
> -- 
> 2.34.1