| Message ID | 20230209200808.869275-15-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | pci hotplug tracking | expand |
On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote: > We have DEVICE_DELETED event, that signals that device_del command is > actually complited. 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. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > --- > qapi/qdev.json | 24 ++++++++++++++++++++++++ > hw/pci/pcie.c | 12 ++++++++++++ > hw/pci/shpc.c | 12 ++++++++++++ > 3 files changed, 48 insertions(+) > > diff --git a/qapi/qdev.json b/qapi/qdev.json > index 40dc34f091..94da7ca228 100644 > --- a/qapi/qdev.json > +++ b/qapi/qdev.json > @@ -220,3 +220,27 @@ > ## > { 'event': 'HOTPLUG_STATE', > 'data': 'HotplugState' } > + > + > +## > +# @DEVICE_ON: > +# > +# Emitted whenever the device insertion completion is acknowledged by the guest. > +# For now only emitted for SHPC and PCIe-native hotplug. > +# > +# @device: device name > +# > +# @path: device path > +# > +# Since: 8.0 > +# > +# Example: > +# > +# <- { "event": "DEVICE_ON", > +# "data": { "device": "virtio-net-pci-0", > +# "path": "/machine/peripheral/virtio-net-pci-0" }, Why provide both device & path? Could the type_name be helpful? > +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > +# > +## > +{ 'event': 'DEVICE_ON', > + 'data': { '*device': 'str', 'path': 'str' } } > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 37e2979b3c..bc7e60ff9d 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -45,6 +45,13 @@ static bool pcie_sltctl_powered_off(uint16_t sltctl) > (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF; > } > > +static bool pcie_sltctl_powered_on(uint16_t sltctl) > +{ > + return (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_ON && > + (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_ON && > + (sltctl & PCI_EXP_SLTCTL_AIC) == PCI_EXP_SLTCTL_ATTN_IND_OFF; > +} > + > static HotplugLedState pcie_led_state_to_qapi(uint16_t value) > { > switch (value) { > @@ -816,6 +823,11 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > 0, 0, /* no state */ > pcie_power_state_to_qapi(old_pcc), > pcie_power_state_to_qapi(pcc)); > + if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_on(val) && > + !pcie_sltctl_powered_on(old_slt_ctl) && child_dev) > + { > + qapi_event_send_device_on(child_dev->id, child_dev->canonical_path); > + } Beside this series aims, but this probably belong to the QDev layer; if we ever model power domains & co some day. Then this would be refactored to something like: qdev_set_power_on(DEVICE(child_dev)); which itself emit the event. Regards, Phil.
On 10.02.23 00:37, Philippe Mathieu-Daudé wrote: > On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote: >> We have DEVICE_DELETED event, that signals that device_del command is >> actually complited. 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. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> --- >> qapi/qdev.json | 24 ++++++++++++++++++++++++ >> hw/pci/pcie.c | 12 ++++++++++++ >> hw/pci/shpc.c | 12 ++++++++++++ >> 3 files changed, 48 insertions(+) >> >> diff --git a/qapi/qdev.json b/qapi/qdev.json >> index 40dc34f091..94da7ca228 100644 >> --- a/qapi/qdev.json >> +++ b/qapi/qdev.json >> @@ -220,3 +220,27 @@ >> ## >> { 'event': 'HOTPLUG_STATE', >> 'data': 'HotplugState' } >> + >> + >> +## >> +# @DEVICE_ON: >> +# >> +# Emitted whenever the device insertion completion is acknowledged by the guest. >> +# For now only emitted for SHPC and PCIe-native hotplug. >> +# >> +# @device: device name >> +# >> +# @path: device path >> +# >> +# Since: 8.0 >> +# >> +# Example: >> +# >> +# <- { "event": "DEVICE_ON", >> +# "data": { "device": "virtio-net-pci-0", >> +# "path": "/machine/peripheral/virtio-net-pci-0" }, > > Why provide both device & path? Could the type_name be helpful? I just follow DEVICE_DELETED event. Not sure that it's the best thing to do) > >> +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } >> +# >> +## >> +{ 'event': 'DEVICE_ON', >> + 'data': { '*device': 'str', 'path': 'str' } } >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >> index 37e2979b3c..bc7e60ff9d 100644 >> --- a/hw/pci/pcie.c >> +++ b/hw/pci/pcie.c >> @@ -45,6 +45,13 @@ static bool pcie_sltctl_powered_off(uint16_t sltctl) >> (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF; >> } >> +static bool pcie_sltctl_powered_on(uint16_t sltctl) >> +{ >> + return (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_ON && >> + (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_ON && >> + (sltctl & PCI_EXP_SLTCTL_AIC) == PCI_EXP_SLTCTL_ATTN_IND_OFF; >> +} >> + >> static HotplugLedState pcie_led_state_to_qapi(uint16_t value) >> { >> switch (value) { >> @@ -816,6 +823,11 @@ void pcie_cap_slot_write_config(PCIDevice *dev, >> 0, 0, /* no state */ >> pcie_power_state_to_qapi(old_pcc), >> pcie_power_state_to_qapi(pcc)); >> + if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_on(val) && >> + !pcie_sltctl_powered_on(old_slt_ctl) && child_dev) >> + { >> + qapi_event_send_device_on(child_dev->id, child_dev->canonical_path); >> + } > > Beside this series aims, but this probably belong to the QDev layer; > if we ever model power domains & co some day. > Then this would be refactored to something like: > > qdev_set_power_on(DEVICE(child_dev)); I thought about it but was not sure where such helper should be placed. For DEVICE_DELETED we don't have one..
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > On 10.02.23 00:37, Philippe Mathieu-Daudé wrote: >> On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote: >>> We have DEVICE_DELETED event, that signals that device_del command is >>> actually complited. 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. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >>> --- >>> qapi/qdev.json | 24 ++++++++++++++++++++++++ >>> hw/pci/pcie.c | 12 ++++++++++++ >>> hw/pci/shpc.c | 12 ++++++++++++ >>> 3 files changed, 48 insertions(+) >>> >>> diff --git a/qapi/qdev.json b/qapi/qdev.json >>> index 40dc34f091..94da7ca228 100644 >>> --- a/qapi/qdev.json >>> +++ b/qapi/qdev.json >>> @@ -220,3 +220,27 @@ >>> ## >>> { 'event': 'HOTPLUG_STATE', >>> 'data': 'HotplugState' } >>> + >>> + >>> +## >>> +# @DEVICE_ON: >>> +# >>> +# Emitted whenever the device insertion completion is acknowledged by the guest. >>> +# For now only emitted for SHPC and PCIe-native hotplug. >>> +# >>> +# @device: device name Make that "the device's ID if it has one", and ... >>> +# >>> +# @path: device path ... "the device's QOM path", please. >>> +# >>> +# Since: 8.0 >>> +# >>> +# Example: >>> +# >>> +# <- { "event": "DEVICE_ON", >>> +# "data": { "device": "virtio-net-pci-0", >>> +# "path": "/machine/peripheral/virtio-net-pci-0" }, >> >> Why provide both device & path? Could the type_name be helpful? > > I just follow DEVICE_DELETED event. Not sure that it's the best thing to do) The device ID is redundant, since the QOM path of a device with an ID is /machine/peripheral/ID. Fine print: code could conceivably violate this invariant. But code should *not* create devices with IDs. IDs are strictly for the user. We commonly send both device ID and QOM path, mostly for historical reasons: the former precede the latter. There are exceptions, such as query-cpus-fast. Can't say offhand whether CPUs can be created with IDs. It's probably best to remain consistent with DEVICE_DELETED here. I'd be in favour of deprecating and deleting redundant device IDs in QMP output. [...]
On 13.02.23 12:30, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes: > >> On 10.02.23 00:37, Philippe Mathieu-Daudé wrote: >>> On 9/2/23 21:08, Vladimir Sementsov-Ogievskiy wrote: >>>> We have DEVICE_DELETED event, that signals that device_del command is >>>> actually complited. 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. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >>>> --- >>>> qapi/qdev.json | 24 ++++++++++++++++++++++++ >>>> hw/pci/pcie.c | 12 ++++++++++++ >>>> hw/pci/shpc.c | 12 ++++++++++++ >>>> 3 files changed, 48 insertions(+) >>>> >>>> diff --git a/qapi/qdev.json b/qapi/qdev.json >>>> index 40dc34f091..94da7ca228 100644 >>>> --- a/qapi/qdev.json >>>> +++ b/qapi/qdev.json >>>> @@ -220,3 +220,27 @@ >>>> ## >>>> { 'event': 'HOTPLUG_STATE', >>>> 'data': 'HotplugState' } >>>> + >>>> + >>>> +## >>>> +# @DEVICE_ON: >>>> +# >>>> +# Emitted whenever the device insertion completion is acknowledged by the guest. >>>> +# For now only emitted for SHPC and PCIe-native hotplug. >>>> +# >>>> +# @device: device name > > Make that "the device's ID if it has one", and ... > >>>> +# >>>> +# @path: device path > > ... "the device's QOM path", please. > >>>> +# >>>> +# Since: 8.0 >>>> +# >>>> +# Example: >>>> +# >>>> +# <- { "event": "DEVICE_ON", >>>> +# "data": { "device": "virtio-net-pci-0", >>>> +# "path": "/machine/peripheral/virtio-net-pci-0" }, >>> >>> Why provide both device & path? Could the type_name be helpful? >> >> I just follow DEVICE_DELETED event. Not sure that it's the best thing to do) > > The device ID is redundant, since the QOM path of a device with an ID is > /machine/peripheral/ID. > > Fine print: code could conceivably violate this invariant. But code > should *not* create devices with IDs. IDs are strictly for the user. > > We commonly send both device ID and QOM path, mostly for historical > reasons: the former precede the latter. > > There are exceptions, such as query-cpus-fast. Can't say offhand > whether CPUs can be created with IDs. > > It's probably best to remain consistent with DEVICE_DELETED here. > > I'd be in favour of deprecating and deleting redundant device IDs in QMP > output. > Hmm. Good. I can make a patch with deprecation and add new events with only QOM path fields after it.
diff --git a/qapi/qdev.json b/qapi/qdev.json index 40dc34f091..94da7ca228 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -220,3 +220,27 @@ ## { 'event': 'HOTPLUG_STATE', 'data': 'HotplugState' } + + +## +# @DEVICE_ON: +# +# Emitted whenever the device insertion completion is acknowledged by the guest. +# For now only emitted for SHPC and PCIe-native hotplug. +# +# @device: device name +# +# @path: device path +# +# Since: 8.0 +# +# Example: +# +# <- { "event": "DEVICE_ON", +# "data": { "device": "virtio-net-pci-0", +# "path": "/machine/peripheral/virtio-net-pci-0" }, +# "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } +# +## +{ 'event': 'DEVICE_ON', + 'data': { '*device': 'str', 'path': 'str' } } diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 37e2979b3c..bc7e60ff9d 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -45,6 +45,13 @@ static bool pcie_sltctl_powered_off(uint16_t sltctl) (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_OFF; } +static bool pcie_sltctl_powered_on(uint16_t sltctl) +{ + return (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_ON && + (sltctl & PCI_EXP_SLTCTL_PIC) == PCI_EXP_SLTCTL_PWR_IND_ON && + (sltctl & PCI_EXP_SLTCTL_AIC) == PCI_EXP_SLTCTL_ATTN_IND_OFF; +} + static HotplugLedState pcie_led_state_to_qapi(uint16_t value) { switch (value) { @@ -816,6 +823,11 @@ void pcie_cap_slot_write_config(PCIDevice *dev, 0, 0, /* no state */ pcie_power_state_to_qapi(old_pcc), pcie_power_state_to_qapi(pcc)); + if ((sltsta & PCI_EXP_SLTSTA_PDS) && pcie_sltctl_powered_on(val) && + !pcie_sltctl_powered_on(old_slt_ctl) && child_dev) + { + qapi_event_send_device_on(child_dev->id, child_dev->canonical_path); + } /* * If the slot is populated, power indicator is off and power diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c index 3c62d6b054..cce9cf19b5 100644 --- a/hw/pci/shpc.c +++ b/hw/pci/shpc.c @@ -293,6 +293,12 @@ static bool shpc_slot_is_off(uint8_t state, uint8_t power, uint8_t attn) return state == SHPC_STATE_DISABLED && power == SHPC_LED_OFF; } +static bool shpc_slot_is_on(uint8_t state, uint8_t power, uint8_t attn) +{ + return state == SHPC_STATE_ENABLED && power == SHPC_LED_ON && + attn == SHPC_LED_OFF; +} + static void shpc_slot_command(PCIDevice *d, uint8_t target, uint8_t state, uint8_t power, uint8_t attn) { @@ -355,6 +361,12 @@ static void shpc_slot_command(PCIDevice *d, uint8_t target, SHPC_SLOT_EVENT_MRL | SHPC_SLOT_EVENT_PRESENCE; } + + if (!shpc_slot_is_on(old_state, old_power, old_attn) && + shpc_slot_is_on(state, power, attn) && child_dev) + { + qapi_event_send_device_on(child_dev->id, child_dev->canonical_path); + } } static void shpc_command(PCIDevice *d)
We have DEVICE_DELETED event, that signals that device_del command is actually complited. 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. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- qapi/qdev.json | 24 ++++++++++++++++++++++++ hw/pci/pcie.c | 12 ++++++++++++ hw/pci/shpc.c | 12 ++++++++++++ 3 files changed, 48 insertions(+)