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

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

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

Events often need to be paired with a query to let the management
application resynchronize after a QMP disconnect, say for a restart.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

Do we have libvirt patches?  If not, is anybody working on them?  If
not, we need at least interface review by a libvirt developer familiar
with hot-plug.  And we might want to mark the interface experimental.

[...]

> 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:

Not sure about "Activity".  "Status"?

> +#
> +# Three-state led indicator state.

Scratch the sentence?

> +#
> +# @on: Indicator is on.
> +#
> +# @blink: Indicator is blinking.
> +#
> +# @off: Indicator is off.

Suggest "LED is on" ans so forth.

> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'LedActivity',
> +  'data': [ 'on', 'blink', 'off' ] }

Suggest 'blinking'.

> +
> +##
> +# @HotplugSHPCSlotState:
> +#
> +# Standard Hot-Plug Controller slot state.
> +#
> +# @power-only: Slot is powered on but neither clock nor bus are connected.

Comma after "on"?

> +#
> +# @enabled: Slot is powered on, clock and bus are connected, the card is
> +#           fully functional from a hardware standpoint.

Suggest ", and the card"

Please format like

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

to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).

> +#
> +# @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.

Suggest something like "When the power LED is on, the device is ...
When it is off, the device is ...  It is blinking while hot plug or
unplug is in progress."

> +#
> +# @attention-led: Attention indicator. Off status means normal operation,
> +#                 On signals about operational problem, Blinking is for
> +#                 locating the slot.

Suggest something like "The attention LED is normally off.  It is on to
signal a a problem.  Blinking is for helping users to locate 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.

Color me confused.  What's "Slot Status"?

> +#
> +# Since: 8.1
> +##
> +{ 'struct': 'HotplugSHPCState',
> +  'base': 'HotplugBaseState',
> +  'data': { '*slot-state': 'HotplugSHPCSlotState' } }
> +
> +##
> +# @HotplugPCIeNativeState:
> +#
> +# PCIe Native hotplug slot state.

hot-plug

More below, not flagging it there.

> +#
> +# @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).

Please format like

    # @power-on: PCIe Power Controller Control of Slot Control Register.
    #     True means Power On (Power Controller Control bit is 0), and
    #     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

Suggest "PCI Standard Hot-plug Controller", because that's how the spec
seems to spell it.

> +#
> +# @pcie-native: PCIe Native hotplug

What about non-PCI hot-plug?

> +#
> +# Since: 8.1
> +##
> +{ 'enum': 'HotplugType',
> +  'data': ['shpc', 'pcie-native'] }
> +
> +##
> +# @HotplugInfo:
> +#
> +# Generic hotplug slot state.

Well, the generic part is defined here, but the union type pulls in
controller-specific parts.  Scratch "generic"?

> +#
> +# @type: type of the hotplug (shpc or pcie-native)

Repeating enum values in doc comments risks them going stale there.
Scratch the parenthesis?

> +#
> +# @bus: The QOM path of the parent bus where device is hotplugged.

Is this the bus the device is plugged into?

What about bus-less devices?  Hmm, I think we can make an argument that
the infrastructure required to make hot-plug work will always be part of
some kind of bus.  Does this make sense to you?

> +#
> +# @addr: The bus address for hotplugged device if applicable.

Uh, does this encode structured data in a string?

> +#
> +# @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' } }

You have data common to both types in two places: base of HotplugInfo
and base of the variant types HotplugBaseState.  Why?

> +
> +##
> +# @query-hotplug:
> +#
> +# Query the state of hotplug controller.
> +#

@id is undocumented.

> +# Since: 8.1
> +##
> +{ 'command': 'query-hotplug',
> +  'data': { 'id': 'str' },
> +  'returns': 'HotplugInfo' }

I have more questions, but I'd like to understand the meaning of @id
before I type them up.

> +
> +##
> +# @DEVICE_ON:
> +#
> +# Emitted whenever the device insertion completion is acknowledged by the 
> guest.
> +# For now only emitted for SHPC and PCIe-native hotplug.

Please format like

   # 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' }

[...]