| Message ID | 20230216180356.156832-19-vsementsov@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | pci hotplug tracking | expand |
On Thu, Feb 16, 2023 at 09:03:56PM +0300, 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. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> I don't much mind though a bit more motivation would be nice. How is this going to be used? When does management care? Meanwhile, for the schema - can this one get ACKs from QAPI maintainers please? > --- > qapi/qdev.json | 10 ++++++++++ > hw/pci/pcie.c | 14 ++++++++++++++ > hw/pci/shpc.c | 12 ++++++++++++ > 3 files changed, 36 insertions(+) > > diff --git a/qapi/qdev.json b/qapi/qdev.json > index 6f2d8d6647..116a8a7de8 100644 > --- a/qapi/qdev.json > +++ b/qapi/qdev.json > @@ -348,3 +348,13 @@ > { '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.0 > +## > +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' } > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > index 636f962a23..4297e4e8dc 100644 > --- a/hw/pci/pcie.c > +++ b/hw/pci/pcie.c > @@ -22,6 +22,7 @@ > > #include "monitor/qdev.h" > #include "qapi/error.h" > +#include "qapi/qapi-events-qdev.h" > #include "hw/pci/pci_bridge.h" > #include "hw/pci/pcie.h" > #include "hw/pci/msix.h" > @@ -47,6 +48,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 LedActivity pcie_led_state_to_qapi(uint16_t value) > { > switch (value) { > @@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state); > } > > + 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 > * controller is off, it is safe to detach the devices. > diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c > index 6a4f93949d..380b2b83b3 100644 > --- a/hw/pci/shpc.c > +++ b/hw/pci/shpc.c > @@ -299,6 +299,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) > { > @@ -366,6 +372,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) > -- > 2.34.1
On 02.03.23 00:07, Michael S. Tsirkin wrote: > On Thu, Feb 16, 2023 at 09:03:56PM +0300, 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. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > I don't much mind though a bit more motivation would be nice. > How is this going to be used? When does management care? Some 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. 3. Also, management tool may make a GUI visualization of power indicator with help of this event. > > Meanwhile, for the schema - can this one get ACKs from QAPI maintainers please? > > >> --- >> qapi/qdev.json | 10 ++++++++++ >> hw/pci/pcie.c | 14 ++++++++++++++ >> hw/pci/shpc.c | 12 ++++++++++++ >> 3 files changed, 36 insertions(+) >> >> diff --git a/qapi/qdev.json b/qapi/qdev.json >> index 6f2d8d6647..116a8a7de8 100644 >> --- a/qapi/qdev.json >> +++ b/qapi/qdev.json >> @@ -348,3 +348,13 @@ >> { '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.0 >> +## >> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' } >> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >> index 636f962a23..4297e4e8dc 100644 >> --- a/hw/pci/pcie.c >> +++ b/hw/pci/pcie.c >> @@ -22,6 +22,7 @@ >> >> #include "monitor/qdev.h" >> #include "qapi/error.h" >> +#include "qapi/qapi-events-qdev.h" >> #include "hw/pci/pci_bridge.h" >> #include "hw/pci/pcie.h" >> #include "hw/pci/msix.h" >> @@ -47,6 +48,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 LedActivity pcie_led_state_to_qapi(uint16_t value) >> { >> switch (value) { >> @@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev, >> qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state); >> } >> >> + 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 >> * controller is off, it is safe to detach the devices. >> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c >> index 6a4f93949d..380b2b83b3 100644 >> --- a/hw/pci/shpc.c >> +++ b/hw/pci/shpc.c >> @@ -299,6 +299,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) >> { >> @@ -366,6 +372,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) >> -- >> 2.34.1 >
On Thu, Feb 16, 2023 at 09:03:56PM +0300, 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. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > --- > qapi/qdev.json | 10 ++++++++++ > hw/pci/pcie.c | 14 ++++++++++++++ > hw/pci/shpc.c | 12 ++++++++++++ > 3 files changed, 36 insertions(+) > > diff --git a/qapi/qdev.json b/qapi/qdev.json > index 6f2d8d6647..116a8a7de8 100644 > --- a/qapi/qdev.json > +++ b/qapi/qdev.json > @@ -348,3 +348,13 @@ > { '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.0 > +## > +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' } Same as any event, this has to be accompanied by a query. Which query returns the "ON" status?
On Thu, Mar 02, 2023 at 11:39:42AM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 02.03.23 00:07, Michael S. Tsirkin wrote: > > On Thu, Feb 16, 2023 at 09:03:56PM +0300, 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. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > > > I don't much mind though a bit more motivation would be nice. > > How is this going to be used? When does management care? > > Some 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. Note this is kind of weak in that we don't know that there's a driver. > 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. That always annoyed me. I wanted delete to just stay pending until it triggers. But if we can't fix that, it's a good reason. However it can always start blinking again. So DEVICE_ON is not a good name. DEVICE_REMOVABLE? And not it's not realiable, it can start blinking by the time you try to remove. Another problem is that guest can create a flood of these events by starting/stopping blinking. Maybe, if blockdev-del fails then we start listening for events and notify when it can be retried? DEVICE_DELETED_RETRY ? > 3. Also, management tool may make a GUI visualization of power indicator with help of this event. > > > > > Meanwhile, for the schema - can this one get ACKs from QAPI maintainers please? > > > > > > > --- > > > qapi/qdev.json | 10 ++++++++++ > > > hw/pci/pcie.c | 14 ++++++++++++++ > > > hw/pci/shpc.c | 12 ++++++++++++ > > > 3 files changed, 36 insertions(+) > > > > > > diff --git a/qapi/qdev.json b/qapi/qdev.json > > > index 6f2d8d6647..116a8a7de8 100644 > > > --- a/qapi/qdev.json > > > +++ b/qapi/qdev.json > > > @@ -348,3 +348,13 @@ > > > { '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.0 > > > +## > > > +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' } > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > index 636f962a23..4297e4e8dc 100644 > > > --- a/hw/pci/pcie.c > > > +++ b/hw/pci/pcie.c > > > @@ -22,6 +22,7 @@ > > > #include "monitor/qdev.h" > > > #include "qapi/error.h" > > > +#include "qapi/qapi-events-qdev.h" > > > #include "hw/pci/pci_bridge.h" > > > #include "hw/pci/pcie.h" > > > #include "hw/pci/msix.h" > > > @@ -47,6 +48,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 LedActivity pcie_led_state_to_qapi(uint16_t value) > > > { > > > switch (value) { > > > @@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > > > qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state); > > > } > > > + 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 > > > * controller is off, it is safe to detach the devices. > > > diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c > > > index 6a4f93949d..380b2b83b3 100644 > > > --- a/hw/pci/shpc.c > > > +++ b/hw/pci/shpc.c > > > @@ -299,6 +299,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) > > > { > > > @@ -366,6 +372,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) > > > -- > > > 2.34.1 > > > > -- > Best regards, > Vladimir
On 02.03.23 11:50, Michael S. Tsirkin wrote: > On Thu, Mar 02, 2023 at 11:39:42AM +0300, Vladimir Sementsov-Ogievskiy wrote: >> On 02.03.23 00:07, Michael S. Tsirkin wrote: >>> On Thu, Feb 16, 2023 at 09:03:56PM +0300, 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. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >>> >>> I don't much mind though a bit more motivation would be nice. >>> How is this going to be used? When does management care? >> >> Some 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. > > Note this is kind of weak in that we don't know that there's a driver. Still, in many cases we are sure that guest has the driver, but can't be sure that the hotplug is handled by guest at all > >> 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. > > That always annoyed me. I wanted delete to just stay pending until > it triggers. > > But if we can't fix that> it's a good reason. However it can always > start blinking again. So DEVICE_ON is not a good name. DEVICE_REMOVABLE? > And not it's not realiable, it can start blinking by the time you try to > remove. > Another problem is that guest can create a flood of these events > by starting/stopping blinking. Hmm, right. Do we have some generic rate-limitator for events? > > Maybe, if blockdev-del fails then we start listening for events > and notify when it can be retried? > > DEVICE_DELETED_RETRY ? Actually HOTPLUG_STATE event from previous patch cover this. So, DEVICE_ON is a convenient generic wrapper, to track plug process for different hotplug controllers in the same simple way: wait some seconds for DEVICE_ON, if it comes - we are OK, if not - handle error. > >> 3. Also, management tool may make a GUI visualization of power indicator with help of this event. >> >>> >>> Meanwhile, for the schema - can this one get ACKs from QAPI maintainers please? >>> >>> >>>> --- >>>> qapi/qdev.json | 10 ++++++++++ >>>> hw/pci/pcie.c | 14 ++++++++++++++ >>>> hw/pci/shpc.c | 12 ++++++++++++ >>>> 3 files changed, 36 insertions(+) >>>> >>>> diff --git a/qapi/qdev.json b/qapi/qdev.json >>>> index 6f2d8d6647..116a8a7de8 100644 >>>> --- a/qapi/qdev.json >>>> +++ b/qapi/qdev.json >>>> @@ -348,3 +348,13 @@ >>>> { '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.0 >>>> +## >>>> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' } >>>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c >>>> index 636f962a23..4297e4e8dc 100644 >>>> --- a/hw/pci/pcie.c >>>> +++ b/hw/pci/pcie.c >>>> @@ -22,6 +22,7 @@ >>>> #include "monitor/qdev.h" >>>> #include "qapi/error.h" >>>> +#include "qapi/qapi-events-qdev.h" >>>> #include "hw/pci/pci_bridge.h" >>>> #include "hw/pci/pcie.h" >>>> #include "hw/pci/msix.h" >>>> @@ -47,6 +48,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 LedActivity pcie_led_state_to_qapi(uint16_t value) >>>> { >>>> switch (value) { >>>> @@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev, >>>> qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state); >>>> } >>>> + 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 >>>> * controller is off, it is safe to detach the devices. >>>> diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c >>>> index 6a4f93949d..380b2b83b3 100644 >>>> --- a/hw/pci/shpc.c >>>> +++ b/hw/pci/shpc.c >>>> @@ -299,6 +299,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) >>>> { >>>> @@ -366,6 +372,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) >>>> -- >>>> 2.34.1 >>> >> >> -- >> Best regards, >> Vladimir >
On Thu, Mar 02, 2023 at 12:16:23PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 02.03.23 11:50, Michael S. Tsirkin wrote: > > On Thu, Mar 02, 2023 at 11:39:42AM +0300, Vladimir Sementsov-Ogievskiy wrote: > > > On 02.03.23 00:07, Michael S. Tsirkin wrote: > > > > On Thu, Feb 16, 2023 at 09:03:56PM +0300, 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. > > > > > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > > > > > > > I don't much mind though a bit more motivation would be nice. > > > > How is this going to be used? When does management care? > > > > > > Some 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. > > > > Note this is kind of weak in that we don't know that there's a driver. > > Still, in many cases we are sure that guest has the driver, but can't be sure that the hotplug is handled by guest at all > > > > > > 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. > > > > That always annoyed me. I wanted delete to just stay pending until > > it triggers. > > > > But if we can't fix that> it's a good reason. However it can always > > start blinking again. So DEVICE_ON is not a good name. DEVICE_REMOVABLE? > > And not it's not realiable, it can start blinking by the time you try to > > remove. > > Another problem is that guest can create a flood of these events > > by starting/stopping blinking. > > Hmm, right. Do we have some generic rate-limitator for events? just design it sanely. > > > > Maybe, if blockdev-del fails then we start listening for events > > and notify when it can be retried? > > > > DEVICE_DELETED_RETRY ? > > Actually HOTPLUG_STATE event from previous patch cover this. > > So, DEVICE_ON is a convenient generic wrapper, to track plug process for different hotplug controllers in the same simple way: wait some seconds for DEVICE_ON, if it comes - we are OK, if not - handle error. I'd like someone from e.g. libvirt to chime in with support for this and explanation how they plan to use it. > > > > > 3. Also, management tool may make a GUI visualization of power indicator with help of this event. > > > > > > > > > > > Meanwhile, for the schema - can this one get ACKs from QAPI maintainers please? > > > > > > > > > > > > > --- > > > > > qapi/qdev.json | 10 ++++++++++ > > > > > hw/pci/pcie.c | 14 ++++++++++++++ > > > > > hw/pci/shpc.c | 12 ++++++++++++ > > > > > 3 files changed, 36 insertions(+) > > > > > > > > > > diff --git a/qapi/qdev.json b/qapi/qdev.json > > > > > index 6f2d8d6647..116a8a7de8 100644 > > > > > --- a/qapi/qdev.json > > > > > +++ b/qapi/qdev.json > > > > > @@ -348,3 +348,13 @@ > > > > > { '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.0 > > > > > +## > > > > > +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' } > > > > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c > > > > > index 636f962a23..4297e4e8dc 100644 > > > > > --- a/hw/pci/pcie.c > > > > > +++ b/hw/pci/pcie.c > > > > > @@ -22,6 +22,7 @@ > > > > > #include "monitor/qdev.h" > > > > > #include "qapi/error.h" > > > > > +#include "qapi/qapi-events-qdev.h" > > > > > #include "hw/pci/pci_bridge.h" > > > > > #include "hw/pci/pcie.h" > > > > > #include "hw/pci/msix.h" > > > > > @@ -47,6 +48,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 LedActivity pcie_led_state_to_qapi(uint16_t value) > > > > > { > > > > > switch (value) { > > > > > @@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev, > > > > > qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state); > > > > > } > > > > > + 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 > > > > > * controller is off, it is safe to detach the devices. > > > > > diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c > > > > > index 6a4f93949d..380b2b83b3 100644 > > > > > --- a/hw/pci/shpc.c > > > > > +++ b/hw/pci/shpc.c > > > > > @@ -299,6 +299,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) > > > > > { > > > > > @@ -366,6 +372,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) > > > > > -- > > > > > 2.34.1 > > > > > > > > > > -- > > > Best regards, > > > Vladimir > > > > -- > Best regards, > Vladimir
On 02.03.23 11:44, Michael S. Tsirkin wrote: > On Thu, Feb 16, 2023 at 09:03:56PM +0300, 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. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> --- >> qapi/qdev.json | 10 ++++++++++ >> hw/pci/pcie.c | 14 ++++++++++++++ >> hw/pci/shpc.c | 12 ++++++++++++ >> 3 files changed, 36 insertions(+) >> >> diff --git a/qapi/qdev.json b/qapi/qdev.json >> index 6f2d8d6647..116a8a7de8 100644 >> --- a/qapi/qdev.json >> +++ b/qapi/qdev.json >> @@ -348,3 +348,13 @@ >> { '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.0 >> +## >> +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' } > > Same as any event, this has to be accompanied by a query. > Which query returns the "ON" status? > Hmm. Seems correct to add "ON" status into "hostplug state", to be returned by query-hotplug. And then, its change should be reported by HOTPLUG_STATE, and separate DEVICE_ON is not needed.
On Thu, Mar 02, 2023 at 01:03:12PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On 02.03.23 11:44, Michael S. Tsirkin wrote: > > On Thu, Feb 16, 2023 at 09:03:56PM +0300, 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. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > > --- > > > qapi/qdev.json | 10 ++++++++++ > > > hw/pci/pcie.c | 14 ++++++++++++++ > > > hw/pci/shpc.c | 12 ++++++++++++ > > > 3 files changed, 36 insertions(+) > > > > > > diff --git a/qapi/qdev.json b/qapi/qdev.json > > > index 6f2d8d6647..116a8a7de8 100644 > > > --- a/qapi/qdev.json > > > +++ b/qapi/qdev.json > > > @@ -348,3 +348,13 @@ > > > { '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.0 > > > +## > > > +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' } > > > > Same as any event, this has to be accompanied by a query. > > Which query returns the "ON" status? > > > > Hmm. Seems correct to add "ON" status into "hostplug state", to be returned by query-hotplug. And then, its change should be reported by HOTPLUG_STATE, and separate DEVICE_ON is not needed. > > -- > Best regards, > Vladimir Given it can go on and off at any time ... I'm not that sure this makes sense as a generic thing. one has to know about specifics of how shpc behaves to understand and use it correctly.
diff --git a/qapi/qdev.json b/qapi/qdev.json index 6f2d8d6647..116a8a7de8 100644 --- a/qapi/qdev.json +++ b/qapi/qdev.json @@ -348,3 +348,13 @@ { '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.0 +## +{ 'event': 'DEVICE_ON', 'data': 'DeviceAndPath' } diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c index 636f962a23..4297e4e8dc 100644 --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -22,6 +22,7 @@ #include "monitor/qdev.h" #include "qapi/error.h" +#include "qapi/qapi-events-qdev.h" #include "hw/pci/pci_bridge.h" #include "hw/pci/pcie.h" #include "hw/pci/msix.h" @@ -47,6 +48,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 LedActivity pcie_led_state_to_qapi(uint16_t value) { switch (value) { @@ -816,6 +824,12 @@ void pcie_cap_slot_write_config(PCIDevice *dev, qdev_hotplug_state_event(DEVICE(dev), NULL, child_dev, &changed_state); } + 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 * controller is off, it is safe to detach the devices. diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c index 6a4f93949d..380b2b83b3 100644 --- a/hw/pci/shpc.c +++ b/hw/pci/shpc.c @@ -299,6 +299,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) { @@ -366,6 +372,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 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. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> --- qapi/qdev.json | 10 ++++++++++ hw/pci/pcie.c | 14 ++++++++++++++ hw/pci/shpc.c | 12 ++++++++++++ 3 files changed, 36 insertions(+)