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

Philippe Mathieu-Daudé posted 7 patches 2 weeks, 1 day ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240209123226.32576-1-philmd@linaro.org
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, John Snow <jsnow@redhat.com>, BALATON Zoltan <balaton@eik.bme.hu>, "Michael S. Tsirkin" <mst@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Laurent Vivier <laurent@vivier.eu>
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(-)
[RFC PATCH 0/7] hw/qdev: Split 'wiring' phase from 'realize'
Posted by Philippe Mathieu-Daudé 2 weeks, 1 day ago
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?

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(-)

-- 
2.41.0


Re: [RFC PATCH 0/7] hw/qdev: Split 'wiring' phase from 'realize'
Posted by BALATON Zoltan 2 weeks, 1 day ago
Hello,

Instead of adding a new method to devices, why not move wiring to the code 
that created/realized the device? Either it's the job of the code that 
created the device and wants to use it or if it's an internal object the 
device itself is created then it should be OK to wire it in the device 
realize method. I probably don't understand the problem fully, what I want 
to say is that device creation is complex enough now so trying to avoid 
adding more complexity to it would be a good thing.

Regards,
BALATON Zoltan
Re: [RFC PATCH 0/7] hw/qdev: Split 'wiring' phase from 'realize'
Posted by Bernhard Beschow 2 weeks, 1 day ago

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(-)
>