Re: [PATCH v3 14/15] qapi: introduce DEVICE_ON event

[Vladimir Sementsov-Ogievskiy]

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..

--
Best regards,
Vladimir