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

[Vladimir Sementsov-Ogievskiy]

A bit old thread, but I'm going to resend, so should answer here about things I 
don't want to change. Be free to ignore it and come to review from scratch in 
v8 (coming soon) if you don't remember, what was it :)

On 19.05.23 18:20, Philippe Mathieu-Daudé wrote:
> 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>

[..]

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

Actually it's used in this patch and forces the event to be sent at most once.
The comment is misleading.

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

I'd postpone it until another user of the enum appear.

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

Now I think it shouldn't. I'd keep all hotplug-related together, even when ACPI 
hotplug will be supported by new event.

Probably it makes sense to make qdev-hotplug.json, but then what to keep in 
qdev.json? Only device-list-properties command.. Seems that it not worth the 
split for now.

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

--
Best regards,
Vladimir