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

[Richard Henderson <richard.henderson@linaro.org>]

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