[PATCH 0/5] docs/devel: improve API documentation for QOM

[PATCH 0/5] docs/devel: improve API documentation for QOM

[Alex Bennée]

Hi,

At the recent QEMU maintainers summit we lamented the fact it was hard
to push forward with our modernising plans as legacy approaches still
get introduced into the code. A lot of knowledge about the "modern"
way of doing things is locked up in senior developers heads. Lets make
a push to improve the developer documentation and ensure best
practice is easy to find.

Some sort of parable about distribution of fishes and knowledge seems
appropriate here.

Alex Bennée (5):
  docs/devel: add some front matter to the devel index
  include/migration: mark vmstate_register() as a legacy function
  include/hw/qdev-core: fixup kerneldoc annotations (!COMPLETE)
  docs/devel: split qom-api reference into new file
  docs/devel: introduce some key concepts for QOM development

 docs/devel/index-api.rst     |   2 +
 docs/devel/index-process.rst |   2 +
 docs/devel/index-tcg.rst     |   2 +
 docs/devel/index.rst         |  24 ++++++-
 docs/devel/qdev-api.rst      |  12 ++++
 docs/devel/qom-api.rst       |   9 +++
 docs/devel/qom.rst           |  50 +++++++++++++-
 docs/devel/tcg.rst           |   2 +
 include/hw/qdev-core.h       | 123 +++++++++++++++++++++++++++++------
 include/migration/vmstate.h  |   9 ++-
 10 files changed, 210 insertions(+), 25 deletions(-)
 create mode 100644 docs/devel/qdev-api.rst
 create mode 100644 docs/devel/qom-api.rst

-- 
2.39.2

[PATCH 1/5] docs/devel: add some front matter to the devel index

[Alex Bennée]

Give an overview of the most useful bits of the devel documentation to
read depending on what the developer wants to do.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/index-process.rst |  2 ++
 docs/devel/index-tcg.rst     |  2 ++
 docs/devel/index.rst         | 24 ++++++++++++++++++++++--
 docs/devel/tcg.rst           |  2 ++
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/docs/devel/index-process.rst b/docs/devel/index-process.rst
index d50dd74c3e..362f97ee30 100644
--- a/docs/devel/index-process.rst
+++ b/docs/devel/index-process.rst
@@ -1,3 +1,5 @@
+.. _development_process:
+
 QEMU Community Processes
 ------------------------
 
diff --git a/docs/devel/index-tcg.rst b/docs/devel/index-tcg.rst
index b44ff8b5a4..a992844e5c 100644
--- a/docs/devel/index-tcg.rst
+++ b/docs/devel/index-tcg.rst
@@ -1,3 +1,5 @@
+.. _tcg:
+
 TCG Emulation
 -------------
 
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 09cfb322be..8f7e3dd80f 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -2,10 +2,30 @@
 Developer Information
 ---------------------
 
-This section of the manual documents various parts of the internals of QEMU.
-You only need to read it if you are interested in reading or
+This section of the manual documents various parts of the internals of
+QEMU. You only need to read it if you are interested in reading or
 modifying QEMU's source code.
 
+QEMU is a large and mature project with a number of complex subsystems
+that can be overwhelming to understand. The development documentation
+is not comprehensive but hopefully presents enough of a starting point
+to get you started. If there are areas that are unclear please reach
+out either via the IRC channel or mailing list and hopefully we can
+improve the documentation for future developers.
+
+All developers will want to familiarise themselves with
+:ref:`development_process` and how the community interacts. Please pay
+particular attention to the :ref:`coding-style` and
+:ref:`submitting-a-patch` sections to avoid common pitfalls.
+
+If you wish to implement a new hardware model you will want to read
+through the :ref:`qom` documentation to understand how QEMU's object
+model works.
+
+Those wishing to enhance or add new CPU emulation capabilities will
+want to read our :ref:`tcg` documentation, especially the overview of
+the :ref:`tcg_internals`.
+
 .. toctree::
    :maxdepth: 1
 
diff --git a/docs/devel/tcg.rst b/docs/devel/tcg.rst
index b4096a17df..2786f2f679 100644
--- a/docs/devel/tcg.rst
+++ b/docs/devel/tcg.rst
@@ -1,3 +1,5 @@
+.. _tcg_internals:
+
 ====================
 Translator Internals
 ====================
-- 
2.39.2

Re: [PATCH 1/5] docs/devel: add some front matter to the devel index

[Richard Henderson]

On 6/19/23 19:14, Alex Bennée wrote:
> +QEMU is a large and mature project with a number of complex subsystems
> +that can be overwhelming to understand. The development documentation
> +is not comprehensive but hopefully presents enough of a starting point
> +to get you started. If there are areas that are unclear please reach

"start" used twice -- "presents enough to get you started"?

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 1/5] docs/devel: add some front matter to the devel index

[Peter Maydell]

On Mon, 19 Jun 2023 at 18:14, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Give an overview of the most useful bits of the devel documentation to
> read depending on what the developer wants to do.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 2/5] include/migration: mark vmstate_register() as a legacy function

[Alex Bennée]

Mention that QOM-ified devices already have support for registering
the description.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/migration/vmstate.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 084f5e784a..35579b2c1f 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1209,7 +1209,14 @@ int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
                                    int required_for_version,
                                    Error **errp);
 
-/* Returns: 0 on success, -1 on failure */
+/**
+ * vmstate_register() - legacy function to register state serialisation description
+ *
+ * New code shouldn't be using this function as QOM-ified devices have
+ * dc->vmsd to store the serialisation description.
+ *
+ * Returns: 0 on success, -1 on failure
+ */
 static inline int vmstate_register(VMStateIf *obj, int instance_id,
                                    const VMStateDescription *vmsd,
                                    void *opaque)
-- 
2.39.2

Re: [PATCH 2/5] include/migration: mark vmstate_register() as a legacy function

[Philippe Mathieu-Daudé]

On 19/6/23 19:14, Alex Bennée wrote:
> Mention that QOM-ified devices already have support for registering
> the description.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/migration/vmstate.h | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 2/5] include/migration: mark vmstate_register() as a legacy function

[Richard Henderson]

On 6/19/23 19:14, Alex Bennée wrote:
> Mention that QOM-ified devices already have support for registering
> the description.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   include/migration/vmstate.h | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 2/5] include/migration: mark vmstate_register() as a legacy function

[Peter Xu]

On Mon, Jun 19, 2023 at 06:14:34PM +0100, Alex Bennée wrote:
> Mention that QOM-ified devices already have support for registering
> the description.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Acked-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 2/5] include/migration: mark vmstate_register() as a legacy function

[Juan Quintela]

Alex Bennée <alex.bennee@linaro.org> wrote:
> Mention that QOM-ified devices already have support for registering
> the description.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

I really remove that function in a future series (well, I substitute it
with vmstate_register_id() and vmstate_register_any(), but the comment
applies to the new versions also).

Later, Juan.

[PATCH 3/5] include/hw/qdev-core: fixup kerneldoc annotations (!COMPLETE)

[Alex Bennée]

Fix up the kerneldoc markup and start documenting the various fields
in QDEV related structures. Unfortunately this is not enough include
the documentation because kerneldoc currently chokes on some of our
macros such as:

    /**
     * @gpios: list of named GPIOs the device provides.
     */
    QLIST_HEAD(, NamedGPIOList) gpios;

where it demands we document QLIST_HEAD and NamedGPIOList despite them
not technically being fields in the structure.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/qdev-core.h | 123 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 102 insertions(+), 21 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index f1070d6dc7..74b4971d7e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -38,7 +38,10 @@ typedef void (*BusRealize)(BusState *bus, Error **errp);
 typedef void (*BusUnrealize)(BusState *bus);
 
 /**
- * DeviceClass:
+ * struct DeviceClass:
+ *
+ * The base class for all devices.
+ *
  * @props: Properties accessing state fields.
  * @realize: Callback function invoked when the #DeviceState:realized
  * property is changed to %true.
@@ -97,22 +100,34 @@ typedef void (*BusUnrealize)(BusState *bus);
  *
  */
 struct DeviceClass {
-    /*< private >*/
+    /* private: */
     ObjectClass parent_class;
-    /*< public >*/
+    /* public: */
 
+    /**
+     * @categories: device categories device belongs to
+     */
     DECLARE_BITMAP(categories, DEVICE_CATEGORY_MAX);
+    /**
+     * @fw_name: name used to identify device to firmware interfaces
+     */
     const char *fw_name;
+    /**
+     * @desc: human readable description of device
+     */
     const char *desc;
 
-    /*
-     * The underscore at the end ensures a compile-time error if someone
-     * assigns to dc->props instead of using device_class_set_props.
+    /**
+     * @props_: properties associated with device, should only be
+     * assigned by using device_class_set_props(). The underscore
+     * ensures a compile-time error if someone attempts to assign
+     * dc->props directly.
      */
     Property *props_;
 
-    /*
-     * Can this device be instantiated with -device / device_add?
+    /**
+     * @user_creatable: Can this device be instantiated with -device / device_add?
+     *
      * All devices should support instantiation with device_add, and
      * this flag should not exist.  But we're not there, yet.  Some
      * devices fail to instantiate with cryptic error messages.
@@ -126,19 +141,28 @@ struct DeviceClass {
     bool hotpluggable;
 
     /* callbacks */
-    /*
-     * Reset method here is deprecated and replaced by methods in the
-     * resettable class interface to implement a multi-phase reset.
+    /**
+     * @reset: deprecated device reset method pointer
+     *
+     * Modern code should use the ResettableClass interface to
+     * implement a multi-phase reset.
+     *
      * TODO: remove once every reset callback is unused
      */
     DeviceReset reset;
     DeviceRealize realize;
     DeviceUnrealize unrealize;
 
-    /* device state */
+    /**
+     * @vmsd: device state serialisation description for
+     * migration/save/restore
+     */
     const VMStateDescription *vmsd;
 
-    /* Private to qdev / bus.  */
+    /**
+     * @bus_type: bus type
+     * private: to qdev / bus.
+     */
     const char *bus_type;
 };
 
@@ -168,36 +192,91 @@ typedef struct {
 } MemReentrancyGuard;
 
 /**
- * DeviceState:
- * @reset: ResettableState for the device; handled by Resettable interface.
+ * struct DeviceState - common device state, accessed with qdev helpers
  *
  * This structure should not be accessed directly.  We declare it here
  * so that it can be embedded in individual device state structures.
  */
 struct DeviceState {
-    /*< private >*/
+    /* private: */
     Object parent_obj;
-    /*< public >*/
+    /* public: */
 
+    /**
+     * @id: global device id
+     */
     char *id;
+    /**
+     * @canonical_path: canonical path of realized device in the QOM tree
+     */
     char *canonical_path;
+    /**
+     * @realized: has device been realized?
+     */
     bool realized;
+    /**
+     * @pending_deleted_event: track pending deletion events during unplug
+     */
     bool pending_deleted_event;
+    /**
+     * @pending_deleted_expires_ms: optional timeout for deletion events
+     */
     int64_t pending_deleted_expires_ms;
+    /**
+     * @opts: QDict of options for the device
+     */
     QDict *opts;
+    /**
+     * @hotplugged: was device added after PHASE_MACHINE_READY?
+     */
     int hotplugged;
+    /**
+     * @allow_unplug_during_migration: can device be unplugged during migration
+     */
     bool allow_unplug_during_migration;
+    /**
+     * @parent_bus: bus this device belongs to
+     */
     BusState *parent_bus;
+    /**
+     * @gpios: list of named GPIOs the device provides.
+     */
     QLIST_HEAD(, NamedGPIOList) gpios;
+    /**
+     * @clocks: list of named clocks the device provides.
+     */
     QLIST_HEAD(, NamedClockList) clocks;
+    /**
+     * @child_bus: list of child buses
+     */
     QLIST_HEAD(, BusState) child_bus;
+    /**
+     * @num_child_bus: number of @child_bus entries
+     */
     int num_child_bus;
+    /**
+     * @instance_id_alias: device alias for handling legacy migration setups
+     */
     int instance_id_alias;
+    /**
+     * @alias_required_for_version: indicates @instance_id_alias is
+     * needed for migration
+     */
     int alias_required_for_version;
+    /**
+     * @reset: ResettableState for the device; handled by Resettable interface.
+     */
     ResettableState reset;
+    /**
+     * @unplug_blockers: list of reasons to block unplugging of device
+     */
     GSList *unplug_blockers;
 
-    /* Is the device currently in mmio/pio/dma? Used to prevent re-entrancy */
+    /**
+     * @mem_reentrancy_guard: Is the device currently in mmio/pio/dma?
+     *
+     * Used to prevent re-entrancy confusing things.
+     */
     MemReentrancyGuard mem_reentrancy_guard;
 };
 
@@ -265,7 +344,7 @@ typedef struct BusChild {
 #define QDEV_HOTPLUG_HANDLER_PROPERTY "hotplug-handler"
 
 /**
- * BusState:
+ * struct BusState:
  * @hotplug_handler: link to a hotplug handler associated with bus.
  * @reset: ResettableState for the bus; handled by Resettable interface.
  */
@@ -290,7 +369,8 @@ struct BusState {
 };
 
 /**
- * GlobalProperty:
+ * typedef GlobalProperty - a global property type
+ *
  * @used: Set to true if property was used when initializing a device.
  * @optional: If set to true, GlobalProperty will be skipped without errors
  *            if the property doesn't exist.
@@ -871,7 +951,8 @@ void device_listener_register(DeviceListener *listener);
 void device_listener_unregister(DeviceListener *listener);
 
 /**
- * @qdev_should_hide_device:
+ * qdev_should_hide_device() - check if device should be hidden
+ *
  * @opts: options QDict
  * @from_json: true if @opts entries are typed, false for all strings
  * @errp: pointer to error object
-- 
2.39.2

Re: [PATCH 3/5] include/hw/qdev-core: fixup kerneldoc annotations (!COMPLETE)

[Richard Henderson]

On 6/19/23 19:14, Alex Bennée wrote:
> Fix up the kerneldoc markup and start documenting the various fields
> in QDEV related structures. Unfortunately this is not enough include
> the documentation because kerneldoc currently chokes on some of our
> macros such as:
> 
>      /**
>       * @gpios: list of named GPIOs the device provides.
>       */
>      QLIST_HEAD(, NamedGPIOList) gpios;
> 
> where it demands we document QLIST_HEAD and NamedGPIOList despite them
> not technically being fields in the structure.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   include/hw/qdev-core.h | 123 ++++++++++++++++++++++++++++++++++-------
>   1 file changed, 102 insertions(+), 21 deletions(-)

I wonder if e.g.

   typedef QLIST_HEAD(, NamedGPIOList) NamedGPIOListHead;

outside of struct DeviceClass would help with those.

Anyway, for this patch,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 4/5] docs/devel: split qom-api reference into new file

[Alex Bennée]

Lets try and keep the overview of the sub-system digestible by
splitting the core API stuff into a separate file. As QOM and QDEV
work together we should also try and enumerate the qdev_ functions.
Currently this is a little broken as kerneldoc doesn't understand our
macros.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/index-api.rst |  2 ++
 docs/devel/qdev-api.rst  | 12 ++++++++++++
 docs/devel/qom-api.rst   |  9 +++++++++
 docs/devel/qom.rst       |  3 ++-
 4 files changed, 25 insertions(+), 1 deletion(-)
 create mode 100644 docs/devel/qdev-api.rst
 create mode 100644 docs/devel/qom-api.rst

diff --git a/docs/devel/index-api.rst b/docs/devel/index-api.rst
index 7108821746..539ad29c21 100644
--- a/docs/devel/index-api.rst
+++ b/docs/devel/index-api.rst
@@ -11,5 +11,7 @@ generated from in-code annotations to function prototypes.
    loads-stores
    memory
    modules
+   qom-api
+   qdev-api
    ui
    zoned-storage
diff --git a/docs/devel/qdev-api.rst b/docs/devel/qdev-api.rst
new file mode 100644
index 0000000000..d47c4d7493
--- /dev/null
+++ b/docs/devel/qdev-api.rst
@@ -0,0 +1,12 @@
+.. _qdev-api:
+
+================================
+QEMU Device (qdev) API Reference
+================================
+
+We don't currently generate the API documentation for QDEV due to QEMU
+macros confusing the kerneldoc tool. For now see the headers in
+``include/hw/qdev-core.h``
+
+..
+  kernel-doc:: include/hw/qdev-core.h
diff --git a/docs/devel/qom-api.rst b/docs/devel/qom-api.rst
new file mode 100644
index 0000000000..ed1f17e797
--- /dev/null
+++ b/docs/devel/qom-api.rst
@@ -0,0 +1,9 @@
+.. _qom-api:
+
+=====================================
+QEMU Object Model (QOM) API Reference
+=====================================
+
+This is the complete API documentation for :ref:`qom`.
+
+.. kernel-doc:: include/qom/object.h
diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst
index c9237950d0..98a4f178d5 100644
--- a/docs/devel/qom.rst
+++ b/docs/devel/qom.rst
@@ -387,4 +387,5 @@ OBJECT_DEFINE_ABSTRACT_TYPE() macro can be used instead:
 API Reference
 -------------
 
-.. kernel-doc:: include/qom/object.h
+See the :ref:`QOM API<qom-api>` and :ref:`QDEV API<qdev-api>`
+documents for the complete API description.
-- 
2.39.2

Re: [PATCH 4/5] docs/devel: split qom-api reference into new file

[Philippe Mathieu-Daudé]

On 19/6/23 19:14, Alex Bennée wrote:
> Lets try and keep the overview of the sub-system digestible by
> splitting the core API stuff into a separate file. As QOM and QDEV
> work together we should also try and enumerate the qdev_ functions.
> Currently this is a little broken as kerneldoc doesn't understand our
> macros.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   docs/devel/index-api.rst |  2 ++
>   docs/devel/qdev-api.rst  | 12 ++++++++++++
>   docs/devel/qom-api.rst   |  9 +++++++++
>   docs/devel/qom.rst       |  3 ++-
>   4 files changed, 25 insertions(+), 1 deletion(-)
>   create mode 100644 docs/devel/qdev-api.rst
>   create mode 100644 docs/devel/qom-api.rst

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 4/5] docs/devel: split qom-api reference into new file

[Richard Henderson]

On 6/19/23 19:14, Alex Bennée wrote:
> +We don't currently generate the API documentation for QDEV due to QEMU
> +macros confusing the kerneldoc tool. For now see the headers in
> +``include/hw/qdev-core.h``
> +
> +..
> +  kernel-doc:: include/hw/qdev-core.h

I'm confused.  Isn't that exactly what you're doing here?


r~

Re: [PATCH 4/5] docs/devel: split qom-api reference into new file

[Philippe Mathieu-Daudé]

On 20/6/23 12:03, Richard Henderson wrote:
> On 6/19/23 19:14, Alex Bennée wrote:
>> +We don't currently generate the API documentation for QDEV due to QEMU
>> +macros confusing the kerneldoc tool. For now see the headers in
>> +``include/hw/qdev-core.h``
>> +
>> +..
>> +  kernel-doc:: include/hw/qdev-core.h
> 
> I'm confused.  Isn't that exactly what you're doing here?

IIUC the kernel-doc style comments from "hw/qdev-core.h"
are embedded as rST doc, see:
https://kernel.readthedocs.io/en/sphinx-samples/kernel-documentation.html#including-kernel-doc-comments

[PATCH 5/5] docs/devel: introduce some key concepts for QOM development

[Alex Bennée]

Using QOM correctly is increasingly important to maintaining a modern
code base. However the current documentation skips some important
concepts before launching into a simple example. Lets:

  - at least mention properties
  - mention TYPE_OBJECT and TYPE_DEVICE
  - talk about why we have realize/unrealize
  - mention the QOM tree

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/qom.rst | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst
index 98a4f178d5..53633fbd35 100644
--- a/docs/devel/qom.rst
+++ b/docs/devel/qom.rst
@@ -13,6 +13,53 @@ features:
 - System for dynamically registering types
 - Support for single-inheritance of types
 - Multiple inheritance of stateless interfaces
+- Mapping internal members to publicly exposed properties
+
+The root object class is TYPE_OBJECT which provides for the basic
+object methods.
+
+The Device Class
+================
+
+The TYPE_DEVICE class is the parent class for all modern devices
+implemented in QEMU and adds some specific methods to handle QEMU
+device model. This includes managing the lifetime of devices from
+creation through to when they become visible to the guest and
+eventually unrealized.
+
+Device Life-cycle
+-----------------
+
+As class initialisation cannot fail devices have an two additional
+methods to handle the creation of dynamic devices. The ``realize``
+function is called with ``Error **`` pointer which should be set if
+the device cannot complete its setup. Otherwise on successful
+completion of the ``realize`` method the device object is added to the
+QOM tree and made visible to the guest.
+
+The reverse function is ``unrealize`` and should be were clean-up
+code lives to tidy up after the system is done with the device.
+
+All devices can be instantiated by C code, however only some can
+created dynamically via the command line or monitor. Likewise only
+some can be unplugged after creation and need an explicit
+``unrealize`` implementation. This is determined by the
+``user_creatable`` and ``hotpluggable`` variables in the root
+``DeviceClass`` structure.
+
+The QOM tree
+------------
+
+The QOM tree is a composition tree which represents all of the objects
+that make up a QEMU "machine". You can view this tree by running
+``info qom-tree`` in the :ref:`QEMU monitor`. It will contain both
+objects created by the machine itself as well those created due to
+user configuration.
+
+Creating a minimal device
+=========================
+
+A simple minimal device implementation may look something like bellow:
 
 .. code-block:: c
    :caption: Creating a minimal type
-- 
2.39.2

Re: [PATCH 5/5] docs/devel: introduce some key concepts for QOM development

[Richard Henderson]

On 6/19/23 19:14, Alex Bennée wrote:
> +As class initialisation cannot fail devices have an two additional
> +methods to handle the creation of dynamic devices. The ``realize``

Beginning with "as" feels like a continuation from something that has been omitted. 
You've skipped over describing ``init`` entirely, and then assumed it.

> +The reverse function is ``unrealize`` and should be were clean-up

"and is where clean-up"


r~

Re: [PATCH 5/5] docs/devel: introduce some key concepts for QOM development

[Paolo Bonzini]

On Mon, Jun 19, 2023 at 7:15 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Using QOM correctly is increasingly important to maintaining a modern
> code base. However the current documentation skips some important
> concepts before launching into a simple example. Lets:
>
>   - at least mention properties
>   - mention TYPE_OBJECT and TYPE_DEVICE
>   - talk about why we have realize/unrealize
>   - mention the QOM tree
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  docs/devel/qom.rst | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>
> diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst
> index 98a4f178d5..53633fbd35 100644
> --- a/docs/devel/qom.rst
> +++ b/docs/devel/qom.rst
> @@ -13,6 +13,53 @@ features:
>  - System for dynamically registering types
>  - Support for single-inheritance of types
>  - Multiple inheritance of stateless interfaces
> +- Mapping internal members to publicly exposed properties
> +
> +The root object class is TYPE_OBJECT which provides for the basic
> +object methods.

Very nice, but I would suggest some changes here.

> +The Device Class
> +================
> +
> +The TYPE_DEVICE class is the parent class for all modern devices
> +implemented in QEMU and adds some specific methods to handle QEMU
> +device model. This includes managing the lifetime of devices from
> +creation through to when they become visible to the guest and
> +eventually unrealized.

Place this paragraph before "Alternatively several static types".

> +Device Life-cycle
> +-----------------


Make this "=====" level and move it at the very end of the document.

Replace these two lines:

The first example of such a QOM method was #CPUClass.reset,
another example is #DeviceClass.realize.

with

One example of such methods is ``DeviceClass.reset``. More examples
can be found at [link to Device Life-Cycle].

> +As class initialisation cannot fail devices have an two additional
> +methods to handle the creation of dynamic devices. The ``realize``
> +function is called with ``Error **`` pointer which should be set if
> +the device cannot complete its setup. Otherwise on successful
> +completion of the ``realize`` method the device object is added to the
> +QOM tree and made visible to the guest.
> +
> +The reverse function is ``unrealize`` and should be were clean-up
> +code lives to tidy up after the system is done with the device.
> +
> +All devices can be instantiated by C code, however only some can
> +created dynamically via the command line or monitor. Likewise only
> +some can be unplugged after creation and need an explicit
> +``unrealize`` implementation. This is determined by the
> +``user_creatable`` and ``hotpluggable`` variables in the root
> +``DeviceClass`` structure.

Move the second sentence (the one starting with "Likewise") to a
separate paragraph. Unpluggability is determined by the HotplugHandler
rather than by "user_creatable" and "hotpluggable".

> +The QOM tree
> +------------
> +
> +The QOM tree is a composition tree which represents all of the objects
> +that make up a QEMU "machine". You can view this tree by running
> +``info qom-tree`` in the :ref:`QEMU monitor`. It will contain both
> +objects created by the machine itself as well those created due to
> +user configuration.

Also make this "===========".

> +Creating a minimal device
> +=========================

Name this "Creating a QOM class", and change "Class Initialization",
"Interfaces" and "Methods" to "-------------".

> +A simple minimal device implementation may look something like bellow:

"below".

Paolo