Re: [RFC PATCH 4/9] hw/qdev: Introduce DeviceClass::[un]wire() handlers

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 28 Jan 2025 12:52:39 -0800
Richard Henderson <richard.henderson@linaro.org> wrote:

> On 1/28/25 06:21, Philippe Mathieu-Daudé wrote:
> > We are trying to understand what means "a qdev is realized".
> > One explanation was "the device is guest visible"; however
> > many devices are realized before being mapped, thus are not
> > "guest visible". Some devices map / wire their IRQs before
> > being realized (such ISA devices). There is a need for devices
> > to be "automatically" mapped/wired (see [2]) such CLI-created
> > devices, but this apply generically to dynamic machines.
> > 
> > Currently the device creation steps are expected to roughly be:
> > 
> >    (external use)                (QDev core)                   (Device Impl)
> >     ~~~~~~~~~~~~                  ~~~~~~~~~                     ~~~~~~~~~~~
> > 
> >                                 INIT enter  
> >                     ----->  
> >                           +----------------------+
> >                           |    Allocate state    |
> >                           +----------------------+  
> >                                                   ----->  
> >                                                          +---------------------+
> >                                                          | INIT children       |
> >                                                          |                     |
> >                                                          | Alias children properties
> >                                                          |                     |
> >                                                          | Expose properties   |
> >                                  INIT exit               +---------------------+
> >                     <-----------------------------------
> >   +----------------+
> >   | set properties |
> >   |                |
> >   | set ClkIn      |
> >   +----------------+          REALIZE enter  
> >                     ---------------------------------->  
> >                                                         +----------------------+
> >                                                         | Use config properties|
> >                                                         |                      |
> >                                                         | Realize children     |
> >                                                         |                      |
> >                                                         | Init GPIOs/IRQs      |
> >                                                         |                      |
> >                                                         | Init MemoryRegions   |
> >                                                         +----------------------+
> >                                 REALIZE exit
> >                     <-----------------------------------                        ----  "realized" / "guest visible"
> > +-----------------+
> > | Explicit wiring:|
> > |   IRQs          |
> > |   I/O / Mem     |
> > |   ClkOut        |
> > +-----------------+             RESET enter  
> >                      --------------------------------->  
> >                                                         +----------------------+
> >                                                         | Reset default values |
> >                                                         +----------------------+
> > 
> > But as mentioned, various devices "wire" parts before they exit
> > the "realize" step.
> > In order to clarify, I'm trying to enforce what can be done
> > *before* and *after* realization.
> > 
> > *after* a device is expected to be stable (no more configurable)
> > and fully usable.
> > 
> > To be able to use internal/auto wiring (such ISA devices) and
> > keep the current external/explicit wiring, I propose to add an
> > extra "internal wiring" step, happening after the REALIZE step

we partially have such wiring in place
  hotplug_handler_plug
'hotplug_' prefix is legacy remnant to confuse every, it really
should be renamed to just plug_handler or 'wire_foo'.

For bus-less CPUs it's used both on hot- and cold-plug paths.

It does take care of wiring after realize part is completed
(basically exposing device to the external users).

And I it also handles 'bus' based plug workflow
see qdev_get_hotplug_handler() assuming bus owner has provided a callback.
It's likely no-one had cared about ISA, as it was out of
hotplug scope when the interface was introduced.

Unplug part is however is not wired directly into unrealize() stage,
but rather to an external caller (mgmt or guest), which
essentially unwinds into the same hotplug_handler chain as plug,
to unwire device and then the same external caller does call unrealize
on device. (see:hotplug_handler_unplug() )

As for more direct hack, there is also DEVICE_LISTENER_CALL() callback,
which is more close to this approach (modulo users are hidden as
there is not explicit wiring).
Unrealize part is called too late (which probably is not right),
and realize part is called before subtree is realized (might work for
current users, but likely should be placed at the same site as wire() call.

Hence we should think if already existing methods could be reused/adapted to,
before adding similar wire/unwire mechanism.


> > as:
> > 
> >    (external use)                (QDev core)                   (Device Impl)
> >     ~~~~~~~~~~~~                  ~~~~~~~~~                     ~~~~~~~~~~~
> > 
> >                                 INIT enter  
> >                     ----->  
> >                           +----------------------+
> >                           |    Allocate state    |
> >                           +----------------------+  
> >                                                   ----->  
> >                                                          +---------------------+
> >                                                          | INIT children       |
> >                                                          |                     |
> >                                                          | Alias children properties
> >                                                          |                     |
> >                                                          | Expose properties   |
> >                                  INIT exit               +---------------------+
> >                     <-----------------------------------
> >   +----------------+
> >   | set properties |
> >   |                |
> >   | set ClkIn      |
> >   +----------------+          REALIZE enter  
> >                     ---------------------------------->  
> >                                                         +----------------------+
> >                                                         | Use config properties|
> >                                                         |                      |
> >                                                         | Realize children     |
> >                                                         |                      |
> >                                                         | Init GPIOs/IRQs      |
> >                                                         |                      |
> >                                                         | Init MemoryRegions   |
> >                                                         +----------------------+
> >                                 REALIZE exit       <---
> >                           +----------------------+
> >                           | Internal auto wiring |
> >                           |   IRQs               |  (i.e. ISA bus)
> >                           |   I/O / Mem          |
> >                           |   ClkOut             |
> >                           +----------------------+
> >                      <---                                                       ----  "realized"
> > +-----------------+
> > | External wiring:|
> > |   IRQs          |
> > |   I/O / Mem     |
> > |   ClkOut        |
> > +-----------------+             RESET enter                                    ----  "guest visible"  
> >                      --------------------------------->  
> >                                                         +----------------------+
> >                                                         | Reset default values |
> >                                                         +----------------------+
> > 
> > The "realized" point is not changed. "guest visible" concept only
> > occurs *after* wiring, just before the reset phase.
> > 
> > This change introduces the DeviceClass::wire handler within qdev
> > core realization code.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> >   include/hw/qdev-core.h |  7 +++++++
> >   hw/core/qdev.c         | 20 +++++++++++++++++++-
> >   2 files changed, 26 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> > index 530f3da7021..021bb7afdc0 100644
> > --- a/include/hw/qdev-core.h
> > +++ b/include/hw/qdev-core.h
> > @@ -102,7 +102,12 @@ typedef int (*DeviceSyncConfig)(DeviceState *dev, Error **errp);
> >    * @props: Properties accessing state fields.
> >    * @realize: Callback function invoked when the #DeviceState:realized
> >    * property is changed to %true.
> > + * @wire: Callback function called after @realize to connect IRQs,
> > + * clocks and map memories. Can not fail.
> > + * @unwire: Callback function to undo @wire. Called before @unrealize.
> > + * Can not fail.
> >    * @unrealize: Callback function invoked when the #DeviceState:realized
> > + * property is changed to %false. Can not fail.
> >    * property is changed to %false.
> >    * @sync_config: Callback function invoked when QMP command device-sync-config
> >    * is called. Should synchronize device configuration from host to guest part
> > @@ -171,6 +176,8 @@ struct DeviceClass {
> >        */
> >       DeviceReset legacy_reset;
> >       DeviceRealize realize;
> > +    void (*wire)(DeviceState *dev);
> > +    void (*unwire)(DeviceState *dev);
> >       DeviceUnrealize unrealize;
> >       DeviceSyncConfig sync_config;
> >   
> > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > index 82bbdcb654e..38449255365 100644
> > --- a/hw/core/qdev.c
> > +++ b/hw/core/qdev.c
> > @@ -554,6 +554,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> >               }
> >          }
> >   
> > +        if (dc->wire) {
> > +            if (!dc->unwire) {
> > +                warn_report_once("wire() without unwire() for type '%s'",
> > +                                 object_get_typename(OBJECT(dev)));
> > +            }
> > +            dc->wire(dev);
> > +        }
> > +
> > +        /* At this point the device is "guest visible". */
> >          qatomic_store_release(&dev->realized, value);
> >   
> >       } else if (!value && dev->realized) {
> > @@ -573,6 +582,15 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> >            */
> >           smp_wmb();
> >   
> > +        if (dc->unwire) {
> > +            if (!dc->wire) {
> > +                error_report("disconnect() without connect() for type '%s'",
> > +                             object_get_typename(OBJECT(dev)));
> > +                abort();
> > +            }
> > +            dc->unwire(dev);
> > +        }  
> 
> Mismatched error messages (wire vs connect).
> But, really, just check both directions properly at startup.
> There's probably lots of places where devices are never unrealized.
> 
> Otherwise, this seems sane, having a kind of post_init on the realize path, running after 
> all superclass realization is done.
> 
> 
> r~
>