For PCIe and SHPC hotplug it's important to track led indicators,
especially the power led. Add an event that helps.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
qapi/qdev.json | 62 ++++++++++++++++++++++++++++++++++++++++++++
include/hw/pci/pci.h | 15 +++++++++++
hw/pci/pci.c | 33 +++++++++++++++++++++++
hw/pci/pcie.c | 49 ++++++++++++++++++++++++++++++++++
hw/pci/shpc.c | 42 ++++++++++++++++++++++++++++++
5 files changed, 201 insertions(+)
diff --git a/qapi/qdev.json b/qapi/qdev.json
index 2708fb4e99..40dc34f091 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -158,3 +158,65 @@
##
{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @HotplugLedState:
+#
+# Since: 8.0
+##
+{ 'enum': 'HotplugLedState',
+ 'data': [ 'on', 'blink', 'off' ] }
+
+##
+# @HotplugPowerState:
+#
+# Since: 8.0
+##
+{ 'enum': 'HotplugPowerState',
+ 'data': [ 'on', 'off' ] }
+
+##
+# @HotplugSlotState:
+#
+# Since: 8.0
+##
+{ 'enum': 'HotplugSlotState',
+ 'data': [ 'power-only', 'enabled', 'disabled' ] }
+
+##
+# @HotplugState:
+#
+# @hotplug-device: hotplug device id
+# @hotplug-path: hotplug device path
+# @hotplug-slot: hotplug device slot (only for SHPC)
+# @device: device name
+# @path: device path
+# @power-led: Power Indicator
+# @attention-led: Attention Indicator
+# @state: slot state, only for SHPC hotplug controller
+# @power: Power Controller state, only for PCIe hotplug
+#
+# Since: 8.0
+##
+{ 'struct': 'HotplugState',
+ 'data': { '*hotplug-device': 'str',
+ 'hotplug-path': 'str',
+ '*hotplug-slot': 'int',
+ '*device': 'str',
+ 'path': 'str',
+ '*power-led': 'HotplugLedState',
+ '*attention-led': 'HotplugLedState',
+ '*state': 'HotplugSlotState',
+ '*power': 'HotplugPowerState' } }
+
+##
+# @HOTPLUG_STATE:
+#
+# Emitted whenever the state of hotplug controller is changed.
+# Only changed values are included into event.
+# Only SHPC and PCIe-native hotplug are supported.
+#
+# Since: 8.0
+##
+{ 'event': 'HOTPLUG_STATE',
+ 'data': 'HotplugState' }
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b6c9c44527..900e22a7d3 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -7,6 +7,9 @@
/* PCI includes legacy ISA access. */
#include "hw/isa/isa.h"
+#include "qapi/qapi-types-qdev.h"
+#include "qapi/qapi-events-qdev.h"
+
extern bool pci_available;
/* PCI bus */
@@ -611,4 +614,16 @@ static inline void pci_irq_pulse(PCIDevice *pci_dev)
MSIMessage pci_get_msi_message(PCIDevice *dev, int vector);
void pci_set_power(PCIDevice *pci_dev, bool state);
+void pci_hotplug_state_event(DeviceState *hotplug_dev,
+ bool has_slot, int64_t slot,
+ DeviceState *dev,
+ HotplugLedState power_led_old,
+ HotplugLedState power_led_new,
+ HotplugLedState attention_led_old,
+ HotplugLedState attention_led_new,
+ HotplugSlotState state_old,
+ HotplugSlotState state_new,
+ HotplugPowerState power_old,
+ HotplugPowerState power_new);
+
#endif
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 34fd1fb5b8..b7374d3d66 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2772,6 +2772,39 @@ void pci_set_power(PCIDevice *d, bool state)
}
}
+void pci_hotplug_state_event(DeviceState *hotplug_dev,
+ bool has_slot, int64_t slot,
+ DeviceState *dev,
+ HotplugLedState power_led_old,
+ HotplugLedState power_led_new,
+ HotplugLedState attention_led_old,
+ HotplugLedState attention_led_new,
+ HotplugSlotState state_old,
+ HotplugSlotState state_new,
+ HotplugPowerState power_old,
+ HotplugPowerState power_new)
+{
+ bool pwr_led = power_led_new != power_led_old;
+ bool attn_led = attention_led_new != attention_led_old;
+ bool state = state_new != state_old;
+ bool pwr = power_new != power_old;
+
+ if (!(pwr_led || attn_led || state || pwr)) {
+ /* No changes - no event */
+ return;
+ }
+
+ qapi_event_send_hotplug_state(hotplug_dev->id,
+ hotplug_dev->canonical_path,
+ has_slot, slot,
+ dev ? dev->id : NULL,
+ dev ? dev->canonical_path : NULL,
+ pwr_led, power_led_new,
+ attn_led, attention_led_new,
+ state, state_new,
+ pwr, power_new);
+}
+
static const TypeInfo pci_device_type_info = {
.name = TYPE_PCI_DEVICE,
.parent = TYPE_DEVICE,
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 774ce85619..37e2979b3c 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -45,6 +45,35 @@ static bool pcie_sltctl_powered_off(uint16_t sltctl)
(sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF;
}
+static HotplugLedState pcie_led_state_to_qapi(uint16_t value)
+{
+ switch (value) {
+ case PCI_EXP_SLTCTL_PWR_IND_ON:
+ case PCI_EXP_SLTCTL_ATTN_IND_ON:
+ return HOTPLUG_LED_STATE_ON;
+ case PCI_EXP_SLTCTL_PWR_IND_BLINK:
+ case PCI_EXP_SLTCTL_ATTN_IND_BLINK:
+ return HOTPLUG_LED_STATE_BLINK;
+ case PCI_EXP_SLTCTL_PWR_IND_OFF:
+ case PCI_EXP_SLTCTL_ATTN_IND_OFF:
+ return HOTPLUG_LED_STATE_OFF;
+ default:
+ abort();
+ }
+}
+
+static HotplugPowerState pcie_power_state_to_qapi(uint16_t value)
+{
+ switch (value) {
+ case PCI_EXP_SLTCTL_PWR_ON:
+ return HOTPLUG_POWER_STATE_ON;
+ case PCI_EXP_SLTCTL_PWR_OFF:
+ return HOTPLUG_POWER_STATE_OFF;
+ default:
+ abort();
+ }
+}
+
/***************************************************************************
* pci express capability helper functions
*/
@@ -728,9 +757,13 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
uint16_t old_slt_ctl, uint16_t old_slt_sta,
uint32_t addr, uint32_t val, int len)
{
+ PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
uint32_t pos = dev->exp.exp_cap;
uint8_t *exp_cap = dev->config + pos;
uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
+ uint16_t power_led, attn_led, pcc, old_power_led, old_attn_led, old_pcc;
+ DeviceState *child_dev =
+ DEVICE(pci_find_the_only_child(sec_bus, pci_bus_num(sec_bus), NULL));
if (ranges_overlap(addr, len, pos + PCI_EXP_SLTSTA, 2)) {
/*
@@ -768,6 +801,22 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
sltsta);
}
+ power_led = val & PCI_EXP_SLTCTL_PIC;
+ attn_led = val & PCI_EXP_SLTCTL_AIC;
+ pcc = val & PCI_EXP_SLTCTL_PCC;
+ old_power_led = old_slt_ctl & PCI_EXP_SLTCTL_PIC;
+ old_attn_led = old_slt_ctl & PCI_EXP_SLTCTL_AIC;
+ old_pcc = old_slt_ctl & PCI_EXP_SLTCTL_PCC;
+
+ pci_hotplug_state_event(DEVICE(dev), false, 0, child_dev,
+ pcie_led_state_to_qapi(old_power_led),
+ pcie_led_state_to_qapi(power_led),
+ pcie_led_state_to_qapi(old_attn_led),
+ pcie_led_state_to_qapi(attn_led),
+ 0, 0, /* no state */
+ pcie_power_state_to_qapi(old_pcc),
+ pcie_power_state_to_qapi(pcc));
+
/*
* If the slot is populated, power indicator is off and power
* controller is off, it is safe to detach the devices.
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 9f964b1d70..3c62d6b054 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -8,6 +8,8 @@
#include "hw/pci/pci.h"
#include "hw/pci/pci_bus.h"
#include "hw/pci/msi.h"
+#include "qapi/qapi-types-qdev.h"
+#include "qapi/qapi-events-qdev.h"
/* TODO: model power only and disabled slot states. */
/* TODO: handle SERR and wakeups */
@@ -123,6 +125,34 @@
#define SHPC_PCI_TO_IDX(pci_slot) ((pci_slot) - 1)
#define SHPC_IDX_TO_PHYSICAL(slot) ((slot) + 1)
+static HotplugLedState shpc_led_state_to_qapi(uint8_t value)
+{
+ switch (value) {
+ case SHPC_LED_ON:
+ return HOTPLUG_LED_STATE_ON;
+ case SHPC_LED_BLINK:
+ return HOTPLUG_LED_STATE_BLINK;
+ case SHPC_LED_OFF:
+ return HOTPLUG_LED_STATE_OFF;
+ default:
+ abort();
+ }
+}
+
+static HotplugSlotState shpc_slot_state_to_qapi(uint8_t value)
+{
+ switch (value) {
+ case SHPC_STATE_PWRONLY:
+ return HOTPLUG_SLOT_STATE_POWER_ONLY;
+ case SHPC_STATE_ENABLED:
+ return HOTPLUG_SLOT_STATE_ENABLED;
+ case SHPC_STATE_DISABLED:
+ return HOTPLUG_SLOT_STATE_DISABLED;
+ default:
+ abort();
+ }
+}
+
static uint8_t shpc_get_status(SHPCDevice *shpc, int slot, uint16_t msk)
{
uint8_t *status = shpc->config + SHPC_SLOT_STATUS(slot);
@@ -268,9 +298,12 @@ static void shpc_slot_command(PCIDevice *d, uint8_t target,
{
SHPCDevice *shpc = d->shpc;
int slot = SHPC_LOGICAL_TO_IDX(target);
+ int pci_slot = SHPC_IDX_TO_PCI(slot);
uint8_t old_state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK);
uint8_t old_power = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK);
uint8_t old_attn = shpc_get_status(shpc, slot, SHPC_SLOT_ATTN_LED_MASK);
+ DeviceState *child_dev =
+ DEVICE(shpc->sec_bus->devices[PCI_DEVFN(pci_slot, 0)]);
if (target < SHPC_CMD_TRGT_MIN || slot >= shpc->nslots) {
shpc_invalid_command(shpc);
@@ -302,6 +335,15 @@ static void shpc_slot_command(PCIDevice *d, uint8_t target,
shpc_set_status(shpc, slot, state, SHPC_SLOT_STATE_MASK);
}
+ pci_hotplug_state_event(DEVICE(d), true, SHPC_IDX_TO_PCI(slot), child_dev,
+ shpc_led_state_to_qapi(old_power),
+ shpc_led_state_to_qapi(power ?: old_power),
+ shpc_led_state_to_qapi(old_attn),
+ shpc_led_state_to_qapi(attn ?: old_attn),
+ shpc_slot_state_to_qapi(old_state),
+ shpc_slot_state_to_qapi(state ?: old_state),
+ 0, 0 /* no PCC */);
+
if (!shpc_slot_is_off(old_state, old_power, old_attn) &&
shpc_slot_is_off(state, power, attn))
{
--
2.34.1Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> For PCIe and SHPC hotplug it's important to track led indicators,
> especially the power led. Add an event that helps.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> qapi/qdev.json | 62 ++++++++++++++++++++++++++++++++++++++++++++
> include/hw/pci/pci.h | 15 +++++++++++
> hw/pci/pci.c | 33 +++++++++++++++++++++++
> hw/pci/pcie.c | 49 ++++++++++++++++++++++++++++++++++
> hw/pci/shpc.c | 42 ++++++++++++++++++++++++++++++
> 5 files changed, 201 insertions(+)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2708fb4e99..40dc34f091 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -158,3 +158,65 @@
> ##
> { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
> 'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @HotplugLedState:
> +#
No documentation?
> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugLedState',
> + 'data': [ 'on', 'blink', 'off' ] }
> +
> +##
> +# @HotplugPowerState:
No documentation?
> +#
> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugPowerState',
> + 'data': [ 'on', 'off' ] }
Why not bool?
> +##
> +# @HotplugSlotState:
No documentation?
> +#
> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugSlotState',
> + 'data': [ 'power-only', 'enabled', 'disabled' ] }
> +
> +##
> +# @HotplugState:
> +#
> +# @hotplug-device: hotplug device id
> +# @hotplug-path: hotplug device path
> +# @hotplug-slot: hotplug device slot (only for SHPC)
> +# @device: device name
> +# @path: device path
> +# @power-led: Power Indicator
> +# @attention-led: Attention Indicator
> +# @state: slot state, only for SHPC hotplug controller
> +# @power: Power Controller state, only for PCIe hotplug
> +#
> +# Since: 8.0
> +##
> +{ 'struct': 'HotplugState',
> + 'data': { '*hotplug-device': 'str',
> + 'hotplug-path': 'str',
> + '*hotplug-slot': 'int',
> + '*device': 'str',
> + 'path': 'str',
> + '*power-led': 'HotplugLedState',
> + '*attention-led': 'HotplugLedState',
> + '*state': 'HotplugSlotState',
> + '*power': 'HotplugPowerState' } }
Too terse.
What do @hotplug-device and @device name? Are these qdev-id?
What kind of paths are @hotplug-path and @path? Are these paths to an
object device in the QOM tree? Which object?
What's a @hotplug-slot?
> +
> +##
> +# @HOTPLUG_STATE:
> +#
> +# Emitted whenever the state of hotplug controller is changed.
Suggest "the state of hotplug controller changes."
Regardless, too terse. What state changes exactly trigger the event?
> +# Only changed values are included into event.
"in the event"
Which values are included for each event trigger?
> +# Only SHPC and PCIe-native hotplug are supported.
Suggest something like "only ... provide this event."
Are parts of HotplugState specific to "SHPC and PCIe-native"? Or asked
differently: when we make other kinds of hotplug send the event, what
would we need to change here?
> +#
> +# Since: 8.0
> +##
> +{ 'event': 'HOTPLUG_STATE',
> + 'data': 'HotplugState' }
[...]
On 10.02.23 13:23, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>
>> For PCIe and SHPC hotplug it's important to track led indicators,
>> especially the power led. Add an event that helps.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> qapi/qdev.json | 62 ++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/pci/pci.h | 15 +++++++++++
>> hw/pci/pci.c | 33 +++++++++++++++++++++++
>> hw/pci/pcie.c | 49 ++++++++++++++++++++++++++++++++++
>> hw/pci/shpc.c | 42 ++++++++++++++++++++++++++++++
>> 5 files changed, 201 insertions(+)
>>
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index 2708fb4e99..40dc34f091 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -158,3 +158,65 @@
>> ##
>> { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>> 'data': { '*device': 'str', 'path': 'str' } }
>> +
>> +##
>> +# @HotplugLedState:
>> +#
>
> No documentation?
Will do!
>
>> +# Since: 8.0
>> +##
>> +{ 'enum': 'HotplugLedState',
>> + 'data': [ 'on', 'blink', 'off' ] }
>> +
>> +##
>> +# @HotplugPowerState:
>
> No documentation?
>
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'enum': 'HotplugPowerState',
>> + 'data': [ 'on', 'off' ] }
>
> Why not bool?
I wanted to reflect PCI_EXP_SLTCTL_PWR_ON and PCI_EXP_SLTCTL_PWR_OFF.
On the other hand, it's just a bit in the config. Power Controller Control. An unobvious thing that
0 = Power On
1 = Power Off
for that bit. So with proper documentation we can use boolean. But on/off is more obvious.
>
>> +##
>> +# @HotplugSlotState:
>
> No documentation?
>
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'enum': 'HotplugSlotState',
>> + 'data': [ 'power-only', 'enabled', 'disabled' ] }
>> +
>> +##
>> +# @HotplugState:
>> +#
>> +# @hotplug-device: hotplug device id
>> +# @hotplug-path: hotplug device path
>> +# @hotplug-slot: hotplug device slot (only for SHPC)
>> +# @device: device name
>> +# @path: device path
>> +# @power-led: Power Indicator
>> +# @attention-led: Attention Indicator
>> +# @state: slot state, only for SHPC hotplug controller
>> +# @power: Power Controller state, only for PCIe hotplug
>
>
>
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'struct': 'HotplugState',
>> + 'data': { '*hotplug-device': 'str',
>> + 'hotplug-path': 'str',
>> + '*hotplug-slot': 'int',
>> + '*device': 'str',
>> + 'path': 'str',
>> + '*power-led': 'HotplugLedState',
>> + '*attention-led': 'HotplugLedState',
>> + '*state': 'HotplugSlotState',
>> + '*power': 'HotplugPowerState' } }
>
> Too terse.
Will fix)
>
> What do @hotplug-device and @device name? Are these qdev-id?
>
> What kind of paths are @hotplug-path and @path? Are these paths to an
> object device in the QOM tree? Which object?
device / path is same name and path as for DEVICE_DELETED
>
> What's a @hotplug-slot?
pci slot. Significant for SHPC
>
>> +
>> +##
>> +# @HOTPLUG_STATE:
>> +#
>> +# Emitted whenever the state of hotplug controller is changed.
>
> Suggest "the state of hotplug controller changes."
>
> Regardless, too terse. What state changes exactly trigger the event?
Any change of power-led / attention-led / state / power.
Will add a description
>
>> +# Only changed values are included into event.
>
> "in the event"
>
> Which values are included for each event trigger?
- device ids and names always included
- power-led / attention-led / state / power - only those who changed
>
>> +# Only SHPC and PCIe-native hotplug are supported.
>
> Suggest something like "only ... provide this event."
>
> Are parts of HotplugState specific to "SHPC and PCIe-native"? Or asked
> differently: when we make other kinds of hotplug send the event, what
> would we need to change here?
Hmm. Looks like I'd better use a union with type discriminator. This way we'll be able to add any other hotplug later.
(and even now it's better, as not all 4 state fields are shared for PCIe and SHPC)
>
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'event': 'HOTPLUG_STATE',
>> + 'data': 'HotplugState' }
>
> [...]
>
--
Best regards,
Vladimir
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> On 10.02.23 13:23, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
>>
>>> For PCIe and SHPC hotplug it's important to track led indicators,
>>> especially the power led. Add an event that helps.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
[...]
>>> +##
>>> +# @HotplugState:
>>> +#
>>> +# @hotplug-device: hotplug device id
>>> +# @hotplug-path: hotplug device path
>>> +# @hotplug-slot: hotplug device slot (only for SHPC)
>>> +# @device: device name
>>> +# @path: device path
>>> +# @power-led: Power Indicator
>>> +# @attention-led: Attention Indicator
>>> +# @state: slot state, only for SHPC hotplug controller
>>> +# @power: Power Controller state, only for PCIe hotplug
>>
>>
>>
>>> +#
>>> +# Since: 8.0
>>> +##
>>> +{ 'struct': 'HotplugState',
>>> + 'data': { '*hotplug-device': 'str',
>>> + 'hotplug-path': 'str',
>>> + '*hotplug-slot': 'int',
>>> + '*device': 'str',
>>> + 'path': 'str',
>>> + '*power-led': 'HotplugLedState',
>>> + '*attention-led': 'HotplugLedState',
>>> + '*state': 'HotplugSlotState',
>>> + '*power': 'HotplugPowerState' } }
>>
>> Too terse.
>
> Will fix)
>
>>
>> What do @hotplug-device and @device name? Are these qdev-id?
>>
>> What kind of paths are @hotplug-path and @path? Are these paths to an
>> object device in the QOM tree? Which object?
>
> device / path is same name and path as for DEVICE_DELETED
Got it. But there we have just one device, and here we have two. Which
two?
Also, DEVICE_DELETED's doc comment is better:
# @device: the device's ID if it has one
#
# @path: the device's QOM path
Suggest to steal from there.
>> What's a @hotplug-slot?
>
> pci slot. Significant for SHPC
>
>>
>>> +
>>> +##
>>> +# @HOTPLUG_STATE:
>>> +#
>>> +# Emitted whenever the state of hotplug controller is changed.
>>
>> Suggest "the state of hotplug controller changes."
>>
>> Regardless, too terse. What state changes exactly trigger the event?
>
> Any change of power-led / attention-led / state / power.
>
> Will add a description
>
>>
>>> +# Only changed values are included into event.
>>
>> "in the event"
>>
>> Which values are included for each event trigger?
>
> - device ids and names always included
> - power-led / attention-led / state / power - only those who changed
>
>>
>>> +# Only SHPC and PCIe-native hotplug are supported.
>>
>> Suggest something like "only ... provide this event."
>>
>> Are parts of HotplugState specific to "SHPC and PCIe-native"? Or asked
>> differently: when we make other kinds of hotplug send the event, what
>> would we need to change here?
>
> Hmm. Looks like I'd better use a union with type discriminator. This way we'll be able to add any other hotplug later.
>
> (and even now it's better, as not all 4 state fields are shared for PCIe and SHPC)
A union feels like the way to go.
>>> +#
>>> +# Since: 8.0
>>> +##
>>> +{ 'event': 'HOTPLUG_STATE',
>>> + 'data': 'HotplugState' }
>>
>> [...]
On 10.02.23 15:01, Markus Armbruster wrote: >>> What do @hotplug-device and @device name? Are these qdev-id? >>> >>> What kind of paths are @hotplug-path and @path? Are these paths to an >>> object device in the QOM tree? Which object? >> device / path is same name and path as for DEVICE_DELETED > Got it. But there we have just one device, and here we have two. Which > two? One is "bus" in which we insert the device. I'm not sure that's it correct to call pcie-root-port a "bus", but on the other hand in device_add command it's a "bus" argument. In some arguments in code it's called hotplug_dev or something like that. Probably "bus" is better. > > Also, DEVICE_DELETED's doc comment is better: > > # @device: the device's ID if it has one > # > # @path: the device's QOM path > > Suggest to steal from there. > OK, agree. -- Best regards, Vladimir
On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote:
> For PCIe and SHPC hotplug it's important to track led indicators,
> especially the power led. Add an event that helps.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
> qapi/qdev.json | 62 ++++++++++++++++++++++++++++++++++++++++++++
> include/hw/pci/pci.h | 15 +++++++++++
> hw/pci/pci.c | 33 +++++++++++++++++++++++
> hw/pci/pcie.c | 49 ++++++++++++++++++++++++++++++++++
> hw/pci/shpc.c | 42 ++++++++++++++++++++++++++++++
> 5 files changed, 201 insertions(+)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 2708fb4e99..40dc34f091 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -158,3 +158,65 @@
> ##
> { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
> 'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @HotplugLedState:
> +#
> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugLedState',
> + 'data': [ 'on', 'blink', 'off' ] }
Could this be more helpful as generic state in "hw/misc/led.h"?
> +##
> +# @HotplugPowerState:
> +#
> +# Since: 8.0
> +##
> +{ 'enum': 'HotplugPowerState',
> + 'data': [ 'on', 'off' ] }
Could it be better to have a generic SwitchState in qapi/common.json?
No real clue, just wondering...
On 10.02.23 00:28, Philippe Mathieu-Daudé wrote:
> On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote:
>> For PCIe and SHPC hotplug it's important to track led indicators,
>> especially the power led. Add an event that helps.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> ---
>> qapi/qdev.json | 62 ++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/pci/pci.h | 15 +++++++++++
>> hw/pci/pci.c | 33 +++++++++++++++++++++++
>> hw/pci/pcie.c | 49 ++++++++++++++++++++++++++++++++++
>> hw/pci/shpc.c | 42 ++++++++++++++++++++++++++++++
>> 5 files changed, 201 insertions(+)
>>
>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>> index 2708fb4e99..40dc34f091 100644
>> --- a/qapi/qdev.json
>> +++ b/qapi/qdev.json
>> @@ -158,3 +158,65 @@
>> ##
>> { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>> 'data': { '*device': 'str', 'path': 'str' } }
>> +
>> +##
>> +# @HotplugLedState:
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'enum': 'HotplugLedState',
>> + 'data': [ 'on', 'blink', 'off' ] }
>
> Could this be more helpful as generic state in "hw/misc/led.h"?
Hmm. LEDState ? Doesn't look similar..
>
>> +##
>> +# @HotplugPowerState:
>> +#
>> +# Since: 8.0
>> +##
>> +{ 'enum': 'HotplugPowerState',
>> + 'data': [ 'on', 'off' ] }
>
> Could it be better to have a generic SwitchState in qapi/common.json?
>
Hmm not sure. This way I stress that it's part of PCIe spec.. Hmm. But still I didn't call it PCIE_*... Maybe generic OnOff in qapi/common.json would be the best.
--
Best regards,
Vladimir
On 10/2/23 11:47, Vladimir Sementsov-Ogievskiy wrote:
> On 10.02.23 00:28, Philippe Mathieu-Daudé wrote:
>> On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote:
>>> For PCIe and SHPC hotplug it's important to track led indicators,
>>> especially the power led. Add an event that helps.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>> ---
>>> qapi/qdev.json | 62 ++++++++++++++++++++++++++++++++++++++++++++
>>> include/hw/pci/pci.h | 15 +++++++++++
>>> hw/pci/pci.c | 33 +++++++++++++++++++++++
>>> hw/pci/pcie.c | 49 ++++++++++++++++++++++++++++++++++
>>> hw/pci/shpc.c | 42 ++++++++++++++++++++++++++++++
>>> 5 files changed, 201 insertions(+)
>>>
>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>> index 2708fb4e99..40dc34f091 100644
>>> --- a/qapi/qdev.json
>>> +++ b/qapi/qdev.json
>>> @@ -158,3 +158,65 @@
>>> ##
>>> { 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
>>> 'data': { '*device': 'str', 'path': 'str' } }
>>> +
>>> +##
>>> +# @HotplugLedState:
>>> +#
>>> +# Since: 8.0
>>> +##
>>> +{ 'enum': 'HotplugLedState',
>>> + 'data': [ 'on', 'blink', 'off' ] }
>>
>> Could this be more helpful as generic state in "hw/misc/led.h"?
>
> Hmm. LEDState ? Doesn't look similar..
Name 'LedActivity' so we can reuse in LEDState?
{ 'enum': 'LedActivity',
'data': [ 'on', 'blink', 'off' ] }
© 2016 - 2026 Red Hat, Inc.