| Message ID | 20230421103207.845847-3-vsementsov@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pci hotplug tracking | expand |
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' }
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' } [...]
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> >> --- >> 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()? OK for me > >> + 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. Means "DEVICE_ON event is already sent". > >> 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? OK, will move. > >> +# >> +# @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' }
On 22.05.23 13:47, Markus Armbruster wrote: > 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? I'm afraid not [I'm making this for non-libvirt management software] > 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. experimental sounds fair, will do. > > [...] > >> 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"? Status sound better, I agree > >> +# >> +# 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"? It's part of SHPC specification. "Slot Status" is a property of slot. It includes "Slot state" ("off" / "on"), which is important for us, and some other things, like "Slot frequency". > >> +# >> +# 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? Nothing for now, not covered by these patches. (one more point for marking this experimental) > >> +# >> +# 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"? Agree > >> +# >> +# @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? Yes > > 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? Yes I think we always have some parent device to hot-plug our device into. It may change in future and then we'll just make the field optional? > >> +# >> +# @addr: The bus address for hotplugged device if applicable. > > Uh, does this encode structured data in a string? It should match addr specified in corresponding device_add command. So it's structured a bit. > >> +# >> +# @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? Good question. Don't remember now, will try to simplify for v8 > >> + >> +## >> +# @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. It's id of hot-plugged device. Should correspond to @id in device_del command: # @id: the device's ID or QOM path > >> + >> +## >> +# @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' } > > [...] > Thanks a lot for reviewing!
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' }
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/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 */ 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' ] } + +## +# @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. +# +# @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 +# +# 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' } 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); +}
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(+)