Re: [RFC PATCH 0/7] hw/qdev: Split 'wiring' phase from 'realize'

[Bernhard Beschow]

Am 9. Februar 2024 12:32:18 UTC schrieb "Philippe Mathieu-Daudé" 
<philmd@linaro.org>:
>Hi,
>
>Various issues related to implementing dynamic machines have
>been documented in [1].
>
>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 series introduces the DeviceClass::wire handler within qdev
>core realization code, and convert devices using the implicit
>wiring to using that explicit handler.
>
>QDev API assertions patches will be posted later as another series.
>
>Thoughts?

Hi Phil,

What exactly are you trying to achieve? I assume that the goal is to make 
devices user-creatable which currently can't due to current limitations. For 
this I'm presenting an idea:

I'd probably not force the new discipline onto devices which aren't 
user-creatable -- that seems like too much gymnastics to me for no gain. 
Instead, I'd act on the suggestion of having an interface TYPE_USER_CREATABLE 
and put the wire method there.

This wire method would be responsible for performing *all* duties between the 
states "realized" and "guest-visible". Based on the parameters it is given by 
the user, it would find all resources it needs to wire up the device. It finds 
these resources by e.g. scanning the QOM tree. A good practice would be to pass 
the root node for the scanning as parameter, e.g. the current machine.

It could happen that a user-crrated device depends on another user-created 
device. To make this work, the part of QEMU that deals with user-created 
devices would perform a fixed point iteration over all not yet finished 
user-created devices. When there is no progress within one iteration, QEMU 
aborts with an error message. This error message could come from the `wire` 
invocation of any -- or even better -- all not yet finished devices.

To demonstrate the approach, one could probably pick just one device, add the 
interface and hack up something in the core handling of user-created devices.

Does this idea sound reasonable? Is this what you're ultimately after?

Best regards,
Bernhatd

>
>Regards,
>
>Phil.
>
>[1] https://lore.kernel.org/all/87o7d1i7ky.fsf@pond.sub.org/
>[2] 
>https://lore.kernel.org/qemu-devel/20231127052024.435743-1-gustavo.romero@linaro.org/
>
>Cc: Eduardo Habkost <eduardo@habkost.net>
>Cc: Markus Armbruster <armbru@redhat.com>' --cc '
>Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>'
>Cc: Richard Henderson <richard.henderson@linaro.org>
>Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>Cc: Thomas Huth <thuth@redhat.com>
>Cc: Paolo Bonzini <pbonzini@redhat.com>
>Cc: Alexander Graf <agraf@csgraf.de>
>Cc: Bernhard Beschow <shentey@gmail.com>
>Cc: Stefan Hajnoczi <stefanha@redhat.com>
>Cc: Peter Maydell <peter.maydell@linaro.org>
>Cc: Cédric Le Goater <clg@kaod.org>
>Cc: Luc Michel <luc.michel@amd.com>
>Cc: Zhao Liu <zhao1.liu@linux.intel.com>
>Cc: Gustavo Romero <gustavo.romero@linaro.org>
>
>Philippe Mathieu-Daudé (7):
>  hw/qdev: Introduce DeviceClass::[un]wire() handlers
>  hw/input/pckbd: Connect i8042 GPIOs once mouse/keyboard are realized
>  hw/ide/cmd646: Configure IDE bus IRQs after realization
>  hw/ide/sii3112: Configure IDE bus IRQs after realization
>  hw/ide/via: Configure IDE bus IRQs after realization
>  hw/intc/mips_gic: Initialize IRQ array once device is realized
>  hw/misc/mac_via: Have VIA1 child access parent IRQ once realized
>
> include/hw/qdev-core.h |  8 +++++++-
> hw/core/qdev.c         | 21 ++++++++++++++++++++-
> hw/ide/cmd646.c        | 12 +++++++++++-
> hw/ide/sii3112.c       | 10 ++++++++++
> hw/ide/via.c           | 10 ++++++++++
> hw/input/pckbd.c       | 38 +++++++++++++++++++++++++++-----------
> hw/intc/mips_gic.c     | 11 +++++++++--
> hw/misc/mac_via.c      |  9 ++++++++-
> 8 files changed, 102 insertions(+), 17 deletions(-)
>