Re: [PATCH v3 13/15] qapi: add HOTPLUG_STATE event

[Vladimir Sementsov-Ogievskiy]

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' }
>
> [...]

--
Best regards,
Vladimir