| Message ID | 20230209200808.869275-14-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | pci hotplug tracking | expand |
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...
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? > +# 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 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.
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' ] }
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' } > > [...] >
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.
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)) {
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(+)