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

[Michael S. Tsirkin]

On Thu, Oct 05, 2023 at 12:29:24PM +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.
> 
> 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      |  12 ++++
>  include/hw/hotplug.h   |  11 ++++
>  include/hw/qdev-core.h |   7 +++
>  include/monitor/qdev.h |   2 +
>  qapi/qdev.json         | 135 +++++++++++++++++++++++++++++++++++++++++
>  softmmu/qdev-monitor.c |  41 +++++++++++++
>  6 files changed, 208 insertions(+)
> 
> diff --git a/hw/core/hotplug.c b/hw/core/hotplug.c
> index 17ac986685..9651fe7c85 100644
> --- a/hw/core/hotplug.c
> +++ b/hw/core/hotplug.c
> @@ -57,6 +57,18 @@ void hotplug_handler_unplug(HotplugHandler *plug_handler,
>      }
>  }
>  
> +HotplugInfo *hotplug_handler_get_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/include/hw/hotplug.h b/include/hw/hotplug.h
> index a9840ed485..23c57a0967 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,12 @@ void hotplug_handler_unplug_request(HotplugHandler 
> *plug_handler,
>  void hotplug_handler_unplug(HotplugHandler *plug_handler,
>                              DeviceState *plugged_dev,
>                              Error **errp);
> +
> +/**
> + * hotplug_handler_get_state:
> + *
> + * Calls #HotplugHandlerClass.get_hotplug_state callback of @plug_handler.
> + */
> +HotplugInfo *hotplug_handler_get_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 151d968238..a27ef2eb24 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -229,6 +229,13 @@ struct DeviceState {
>       * @realized: has device been realized?
>       */
>      bool realized;
> +
> +    /**
> +     * @device_on_event_sent: When true means that DEVICE_ON event was 
> already
> +     *                        sent.
> +     */
> +    bool device_on_event_sent;
> +
>      /**
>       * @pending_deleted_event: track pending deletion events during unplug
>       */
> 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 2899fac837..fa80694735 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -180,3 +180,138 @@
>  #      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
>  ##
>  { 'event': 'DEVICE_UNPLUG_GUEST_ERROR', 'data': 'DeviceAndPath' }
> +
> +##
> +# @LedStatus:
> +#
> +# @on: LED is on.
> +#
> +# @blinking: LED is blinking.
> +#
> +# @off: LED is off.
> +#
> +# Since: 8.2
> +##
> +{ 'enum': 'LedStatus',
> +  'data': [ 'on', 'blinking', 'off' ] }
> +

Querying LED status is by itself a reasonable thing.
It's under guest control though - how do we avoid
a flood of events?


> +##
> +# @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, and the
> +#     card is fully functional from a hardware standpoint.
> +#
> +# @disabled: Slot is disabled, card is safe to be removed.
> +#

Are these really the only 3 states a slot can be in?
And do we care about power-only?


> +# Since: 8.2
> +##
> +{ 'enum': 'HotplugSHPCSlotState',
> +  'data': [ 'power-only', 'enabled', 'disabled' ] }
> +
> +##
> +# @HotplugSHPCState:
> +#
> +# Standard Hot Plug Controller state.
> +#
> +# @slot-state: The slot state field of SHPC Slot Status structure.
> +#
> +# Since: 8.2
> +##
> +{ 'struct': 'HotplugSHPCState',
> +  '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.2
> +##
> +{ 'struct': 'HotplugPCIeNativeState',
> +  'data': { '*power-on': 'bool' } }

and here we don't care about enable?

> +
> +##
> +# @HotplugType:
> +#
> +# Type of hotplug controller / provider.
> +#
> +# @shpc: PCI Standard Hot-plug Controller
> +#
> +# @pcie-native: PCIe Native hotplug
> +#
> +# TODO: add @acpi type
> +#
> +# Since: 8.2
> +##
> +{ 'enum': 'HotplugType',
> +  'data': ['shpc', 'pcie-native'] }
> +
> +##
> +# @HotplugInfo:
> +#
> +# Hotplug slot state.
> +#
> +# @type: type of the hotplug
> +#
> +# @bus: The QOM path of the parent bus where device is plugged into.
> +#
> +# @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.
> +#
> +# @power-led: Power indicator.  When power LED is on the device is
> +#     ready and accepted by guest.  When it is off, device is safe to
> +#     remove.  It is blinking while hot plug or unplug is in progress.
> +#
> +# @attention-led: The attention LED is normally off.  It is on to
> +#     signal a problem.  Blinking is for helping users to locate the
> +#     slot.
> +#
> +# Single: 8.2
> +##
> +{ 'union': 'HotplugInfo',
> +  'base': { 'type': 'HotplugType',
> +            'bus': 'DeviceAndPath',
> +            '*addr': 'str',
> +            'child': 'DeviceAndPath',
> +            'device-on': 'bool',
> +            '*power-led': 'LedStatus',
> +            '*attention-led': 'LedStatus'},
> +  'discriminator': 'type',
> +  'data': { 'shpc': 'HotplugSHPCState',
> +            'pcie-native': 'HotplugPCIeNativeState' } }
> +

What about ACPI hotplug?



> +##
> +# @x-query-hotplug:
> +#
> +# Query the state of hotplug controller.
> +#
> +# @id: the device's ID or QOM path
> +#
> +# Since: 8.2
> +##
> +{ 'command': 'x-query-hotplug',
> +  'data': { 'id': 'str' },
> +  'returns': 'HotplugInfo' }
> +
> +##
> +# @X_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.2
> +##
> +{ 'event': 'X_DEVICE_ON', 'data': 'DeviceAndPath' }
> diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
> index 74f4e41338..483da40cb9 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_x_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_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_event_sent) {
> +        return;
> +    }
> +
> +    dev->device_on_event_sent = true;
> +    qapi_event_send_x_device_on(dev->id, dev->canonical_path);
> +}
> -- 
> 2.34.1