Re: [PATCH v8 0/4] pci hotplug tracking

[Vladimir Sementsov-Ogievskiy]

On 02.11.23 14:31, Michael S. Tsirkin wrote:
> On Thu, Oct 05, 2023 at 12:29:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > Hi all!
> >
> > Main thing this series does is DEVICE_ON event - a counter-part to
> > DEVICE_DELETED. A guest-driven event that device is powered-on.
> > Details are in patch 2. The new event is paried with corresponding
> > command query-hotplug.
>
> Several things questionable here:
> 1. depending on guest activity you can get as many
>     DEVICE_ON events as you like

No, I've made it so it may be sent only once per device

> 2. it's just for shpc and native pcie - things are
>     confusing enough for management, we should make sure
>     it can work for all devices

Agree, I'm thinking about it

> 3. what about non hotpluggable devices? do we want the event for them?

I think, yes, especially if we make async=true|false flag for device_add, so 
that successful device_add must be always followed by DEVICE_ON - like 
device_del is followed by DEVICE_DELETED.

Maybe, to generalize, it should be called not DEVICE_ON (which mostly relate to 
hotplug controller statuses) but DEVICE_ADDED - a full counterpart for 
DEVICE_DELETED.

> I feel this needs actual motivation so we can judge what's the
> right way to do it.

My first motivation for this series was the fact that successful device_add doesn't 
guarantee that hard disk successfully hotplugged to the guest. It relates to some 
problems with shpc/pcie hotplug we had in the past, and they are mostly fixed. But still, 
for management tool it's good to understand that all actions related to hotplug 
controller are done and we have "green light".

Recently new motivation come, as I described in my "ping" letter 
<6bd19a07-5224-464d-b54d-1d738f5ba8f7@yandex-team.ru>, that we have a performance 
degradation because of 7bed89958bfbf40df, which introduces drain_call_rcu() in device_add, to 
make it more synchronous. So, my suggestion is make it instead more asynchronous (probably with 
special flag) and rely on DEVICE_ON event.

> > v8:
> >   - improve naming, wording and style
> >   - make new QMP interface experimental
> >
> >
> > Vladimir Sementsov-Ogievskiy (4):
> >    qapi/qdev.json: unite DEVICE_* event data into single structure
> >    qapi: add DEVICE_ON and query-hotplug infrastructure
> >    shpc: implement DEVICE_ON event and query-hotplug
> >    pcie: implement DEVICE_ON event and query-hotplug
> >
> >   hw/core/hotplug.c               |  12 +++
> >   hw/pci-bridge/pci_bridge_dev.c  |  14 +++
> >   hw/pci-bridge/pcie_pci_bridge.c |   1 +
> >   hw/pci/pcie.c                   |  83 +++++++++++++++
> >   hw/pci/pcie_port.c              |   1 +
> >   hw/pci/shpc.c                   |  86 +++++++++++++++
> >   include/hw/hotplug.h            |  11 ++
> >   include/hw/pci/pci_bridge.h     |   2 +
> >   include/hw/pci/pcie.h           |   2 +
> >   include/hw/pci/shpc.h           |   2 +
> >   include/hw/qdev-core.h          |   7 ++
> >   include/monitor/qdev.h          |   6 ++
> >   qapi/qdev.json                  | 178 +++++++++++++++++++++++++++++---
> >   softmmu/qdev-monitor.c          |  58 +++++++++++
> >   14 files changed, 451 insertions(+), 12 deletions(-)
> >
> > --
> > 2.34.1

--
Best regards,
Vladimir