Re: [PATCH v5 18/18] qapi: introduce DEVICE_ON event

[Michael S. Tsirkin]

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