[PATCH 0/9] docs: various (style, punctuation and typo fixes)

[PATCH 0/9] docs: various (style, punctuation and typo fixes)

[Alex Bennée]

I accumulated some doc fixes that didn't make it into 8.0 and thought
I might as well see if there was anything worth adding to the coding
style while at it.

Let me know what you think.

Alex Bennée (4):
  docs/system: remove excessive punctuation from guest-loader docs
  docs/devel: make a statement about includes
  docs/devel: mention the spacing requirement for QOM
  docs/style: call out the use of GUARD macros

Juan Quintela (1):
  MAINTAINERS: Add Juan Quintela to developer guides review

Peter Maydell (1):
  docs/devel/kconfig.rst: Fix incorrect markup

Stefan Weil (2):
  docs: Fix typo (wphx => whpx)
  docs/cxl: Fix sentence

Yohei Kojima (1):
  qemu-options.hx: Update descriptions of memory options for NUMA node

 MAINTAINERS                  |  1 +
 docs/devel/kconfig.rst       |  2 +-
 docs/devel/qom.rst           |  2 +
 docs/devel/style.rst         | 79 ++++++++++++++++++++++++++++++++++++
 docs/system/devices/cxl.rst  |  2 +-
 docs/system/guest-loader.rst |  6 +--
 docs/system/introduction.rst |  2 +-
 qemu-options.hx              | 25 ++++++++----
 8 files changed, 104 insertions(+), 15 deletions(-)

-- 
2.39.2

[PATCH 1/9] docs/devel/kconfig.rst: Fix incorrect markup

[Alex Bennée]

From: Peter Maydell <peter.maydell@linaro.org>

In rST markup syntax, the inline markup (*italics*, **bold** and
``monospaced``) must be separated from the surrending text by
non-word characters, otherwise it is not interpreted as markup.
To force interpretation as markup in the middle of a word,
you need to use a backslash-escaped space (which will not
appear as a space in the output).

Fix a missing backslash-space in this file, which meant that the ``
after "select" was output literally and the monospacing was
incorrectly extended all the way to the end of the next monospaced
word.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20230411105424.3994585-1-peter.maydell@linaro.org>
---
 docs/devel/kconfig.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/devel/kconfig.rst b/docs/devel/kconfig.rst
index cc1a456edf..ac9453eba9 100644
--- a/docs/devel/kconfig.rst
+++ b/docs/devel/kconfig.rst
@@ -274,7 +274,7 @@ or commenting out lines in the second group.
 
 It is also possible to run QEMU's configure script with the
 ``--without-default-devices`` option.  When this is done, everything defaults
-to ``n`` unless it is ``select``ed or explicitly switched on in the
+to ``n`` unless it is ``select``\ ed or explicitly switched on in the
 ``.mak`` files.  In other words, ``default`` and ``imply`` directives
 are disabled.  When QEMU is built with this option, the user will probably
 want to change some lines in the first group, for example like this::
-- 
2.39.2

[PATCH 2/9] qemu-options.hx: Update descriptions of memory options for NUMA node

[Alex Bennée]

From: Yohei Kojima <y-koj@outlook.jp>

This commit adds the following description:
1. `memdev` option is recommended over `mem` option (see [1,2])
2. users must specify memory for all NUMA nodes (see [2])

This commit also separates descriptions for `mem` and `memdev` into two
paragraphs. The old doc describes legacy `mem` option first, and it was
a bit confusing.

Related documantations:
[1] https://wiki.qemu.org/ChangeLog/5.1#Incompatible_changes
[2] https://www.qemu.org/docs/master/about/removed-features.html

Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
Message-Id: <TYZPR06MB5418D6B0175A49E8E76988439D8E9@TYZPR06MB5418.apcprd06.prod.outlook.com>
---
 qemu-options.hx | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 59bdf67a2c..174f0d0c2d 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -405,15 +405,22 @@ SRST
         -numa node,nodeid=0 -numa node,nodeid=1 \
         -numa cpu,node-id=0,socket-id=0 -numa cpu,node-id=1,socket-id=1
 
-    Legacy '\ ``mem``\ ' assigns a given RAM amount to a node (not supported
-    for 5.1 and newer machine types). '\ ``memdev``\ ' assigns RAM from
-    a given memory backend device to a node. If '\ ``mem``\ ' and
-    '\ ``memdev``\ ' are omitted in all nodes, RAM is split equally between them.
-
-
-    '\ ``mem``\ ' and '\ ``memdev``\ ' are mutually exclusive.
-    Furthermore, if one node uses '\ ``memdev``\ ', all of them have to
-    use it.
+    '\ ``memdev``\ ' option assigns RAM from a given memory backend
+    device to a node. It is recommended to use '\ ``memdev``\ ' option
+    over legacy '\ ``mem``\ ' option. This is because '\ ``memdev``\ '
+    option provides better performance and more control over the
+    backend's RAM (e.g. '\ ``prealloc``\ ' parameter of
+    '\ ``-memory-backend-ram``\ ' allows memory preallocation).
+
+    For compatibility reasons, legacy '\ ``mem``\ ' option is
+    supported in 5.0 and older machine types. Note that '\ ``mem``\ '
+    and '\ ``memdev``\ ' are mutually exclusive. If one node uses
+    '\ ``memdev``\ ', the rest nodes have to use '\ ``memdev``\ '
+    option, and vice versa.
+
+    Users must specify memory for all NUMA nodes by '\ ``memdev``\ '
+    (or legacy '\ ``mem``\ ' if available). In QEMU 5.2, the support
+    for '\ ``-numa node``\ ' without memory specified was removed.
 
     '\ ``initiator``\ ' is an additional option that points to an
     initiator NUMA node that has best performance (the lowest latency or
-- 
2.39.2

[PATCH 3/9] docs: Fix typo (wphx => whpx)

[Alex Bennée]

From: Stefan Weil via <qemu-devel@nongnu.org>

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1529
Signed-off-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230409201007.1157671-1-sw@weilnetz.de>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/system/introduction.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/system/introduction.rst b/docs/system/introduction.rst
index c8a9fe6c1d..3e256f8326 100644
--- a/docs/system/introduction.rst
+++ b/docs/system/introduction.rst
@@ -27,7 +27,7 @@ Tiny Code Generator (TCG) capable of emulating many CPUs.
   * - Hypervisor Framework (hvf)
     - MacOS
     - x86 (64 bit only), Arm (64 bit only)
-  * - Windows Hypervisor Platform (wphx)
+  * - Windows Hypervisor Platform (whpx)
     - Windows
     - x86
   * - NetBSD Virtual Machine Monitor (nvmm)
-- 
2.39.2

[PATCH 4/9] docs/cxl: Fix sentence

[Alex Bennée]

From: Stefan Weil via <qemu-devel@nongnu.org>

Signed-off-by: Stefan Weil <sw@weilnetz.de>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Message-Id: <20230409201828.1159568-1-sw@weilnetz.de>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/system/devices/cxl.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/system/devices/cxl.rst b/docs/system/devices/cxl.rst
index f25783a4ec..4c38223069 100644
--- a/docs/system/devices/cxl.rst
+++ b/docs/system/devices/cxl.rst
@@ -111,7 +111,7 @@ Interfaces provided include:
 
 CXL Root Ports (CXL RP)
 ~~~~~~~~~~~~~~~~~~~~~~~
-A CXL Root Port servers te same purpose as a PCIe Root Port.
+A CXL Root Port serves the same purpose as a PCIe Root Port.
 There are a number of CXL specific Designated Vendor Specific
 Extended Capabilities (DVSEC) in PCIe Configuration Space
 and associated component register access via PCI bars.
-- 
2.39.2

[PATCH 5/9] MAINTAINERS: Add Juan Quintela to developer guides review

[Alex Bennée]

From: Juan Quintela <quintela@redhat.com>

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230419163457.17175-1-quintela@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2c2068ea5c..24154f5721 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -70,6 +70,7 @@ R: Daniel P. Berrangé <berrange@redhat.com>
 R: Thomas Huth <thuth@redhat.com>
 R: Markus Armbruster <armbru@redhat.com>
 R: Philippe Mathieu-Daudé <philmd@linaro.org>
+R: Juan Quintela <quintela@redhat.com>
 W: https://www.qemu.org/docs/master/devel/index.html
 S: Odd Fixes
 F: docs/devel/style.rst
-- 
2.39.2

[PATCH 6/9] docs/system: remove excessive punctuation from guest-loader docs

[Alex Bennée]

A possessive its needs no ' whereas the contraction of it is does.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/system/guest-loader.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/system/guest-loader.rst b/docs/system/guest-loader.rst
index 9ef9776bf0..304ee5d531 100644
--- a/docs/system/guest-loader.rst
+++ b/docs/system/guest-loader.rst
@@ -14,7 +14,7 @@ The guest loader does two things:
   - load blobs (kernels and initial ram disks) into memory
   - sets platform FDT data so hypervisors can find and boot them
 
-This is what is typically done by a boot-loader like grub using it's
+This is what is typically done by a boot-loader like grub using its
 multi-boot capability. A typical example would look like:
 
 .. parsed-literal::
@@ -25,9 +25,9 @@ multi-boot capability. A typical example would look like:
     -device guest-loader,addr=0x47000000,initrd=rootfs.cpio
 
 In the above example the Xen hypervisor is loaded by the -kernel
-parameter and passed it's boot arguments via -append. The Dom0 guest
+parameter and passed its boot arguments via -append. The Dom0 guest
 is loaded into the areas of memory. Each blob will get
-``/chosen/module@<addr>`` entry in the FDT to indicate it's location and
+``/chosen/module@<addr>`` entry in the FDT to indicate its location and
 size. Additional information can be passed with by using additional
 arguments.
 
-- 
2.39.2

[PATCH 7/9] docs/devel: make a statement about includes

[Alex Bennée]

While not explicitly disallowing header macro abuse (because that
would make us hypocrites) lets at least address some things to think
about.

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

diff --git a/docs/devel/style.rst b/docs/devel/style.rst
index 68aa776930..5bc6f2f095 100644
--- a/docs/devel/style.rst
+++ b/docs/devel/style.rst
@@ -300,6 +300,20 @@ putting those into qemu/typedefs.h instead of including the header.
 
 Cyclic inclusion is forbidden.
 
+Generative Includes
+-------------------
+
+QEMU makes fairly extensive use of the macro pre-processor to
+instantiate multiple similar functions. While such abuse of the macro
+processor isn't discouraged it can make debugging and code navigation
+harder. You should consider carefully if the same effect can be
+achieved by making it easy for the compiler to constant fold or using
+python scripting to generate grep friendly code.
+
+If you do use template header files they should be named with the
+``.c.inc`` or ``.h.inc`` suffix to make it clear they are being
+included for expansion.
+
 C types
 =======
 
-- 
2.39.2

[PATCH 8/9] docs/devel: mention the spacing requirement for QOM

[Alex Bennée]

We have a more complete document on QOM but we should at least mention
the style requirements in the style guide.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 docs/devel/qom.rst   |  2 ++
 docs/devel/style.rst | 29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst
index 3e34b07c98..c9237950d0 100644
--- a/docs/devel/qom.rst
+++ b/docs/devel/qom.rst
@@ -1,3 +1,5 @@
+.. _qom:
+
 ===========================
 The QEMU Object Model (QOM)
 ===========================
diff --git a/docs/devel/style.rst b/docs/devel/style.rst
index 5bc6f2f095..0bd01f3fca 100644
--- a/docs/devel/style.rst
+++ b/docs/devel/style.rst
@@ -628,6 +628,35 @@ are still some caveats to beware of
 QEMU Specific Idioms
 ********************
 
+QEMU Object Model Declarations
+==============================
+
+The QEMU Object Model (QOM) provides a framework for handling objects
+in the base C language. The first declaration of a storage or class
+structure should always be the parent and leave a visual space between
+that declaration and the new code.
+
+.. code-block:: c
+
+    typedef struct MyDeviceState {
+        DeviceState parent_obj;
+
+        /* Properties */
+        int prop_a;
+        char *prob_b;
+        /* Other stuff */
+        int internal_state;
+    } MyDeviceState;
+
+    typedef struct MyDeviceClass {
+        ObjectClass parent_class;
+
+        void (*new_fn1)(void);
+        bool (*new_fn2)(CPUState *);
+   } MyDeviceClass;
+
+See :ref:`qom` for more details.
+
 Error handling and reporting
 ============================
 
-- 
2.39.2

[PATCH 9/9] docs/style: call out the use of GUARD macros

[Alex Bennée]

There use makes our code safer so we should mention them.

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

diff --git a/docs/devel/style.rst b/docs/devel/style.rst
index 0bd01f3fca..b50a981a86 100644
--- a/docs/devel/style.rst
+++ b/docs/devel/style.rst
@@ -657,6 +657,42 @@ that declaration and the new code.
 
 See :ref:`qom` for more details.
 
+QEMU GUARD macros
+=================
+
+QEMU provides a number of ``_GUARD`` macros intended to make the
+handling of multiple exit paths easier. For example using
+``QEMU_LOCK_GUARD`` to take a lock will ensure the lock is released on
+exit from the function.
+
+.. code-block:: c
+
+    static int my_critical_function(SomeState *s, void *data)
+    {
+        QEMU_LOCK_GUARD(&s->lock);
+        do_thing1(data);
+        if (check_state2(data)) {
+            return -1;
+        }
+        do_thing3(data);
+        return 0;
+    }
+
+will ensure s->lock is released however the function is exited. There
+are often ``WITH_`` forms of macros which more easily wrap around a
+block inside a function.
+
+.. code-block:: c
+
+    WITH_RCU_READ_LOCK_GUARD() {
+        QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
+            err = do_the_thing(kid->child);
+            if (err < 0) {
+                return err;
+            }
+        }
+    }
+
 Error handling and reporting
 ============================
 
-- 
2.39.2

Re: [PATCH 0/9] docs: various (style, punctuation and typo fixes)

[Peter Maydell]

On Thu, 20 Apr 2023 at 16:57, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> I accumulated some doc fixes that didn't make it into 8.0 and thought
> I might as well see if there was anything worth adding to the coding
> style while at it.
>
> Let me know what you think.

> Peter Maydell (1):
>   docs/devel/kconfig.rst: Fix incorrect markup

This one is in my target-arm pullreq on the list, so you
can drop it.

thanks
-- PMM

Re: [PATCH 6/9] docs/system: remove excessive punctuation from guest-loader docs

[Thomas Huth]

On 20/04/2023 17.57, Alex Bennée wrote:
> A possessive its needs no ' whereas the contraction of it is does.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   docs/system/guest-loader.rst | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/system/guest-loader.rst b/docs/system/guest-loader.rst
> index 9ef9776bf0..304ee5d531 100644
> --- a/docs/system/guest-loader.rst
> +++ b/docs/system/guest-loader.rst
> @@ -14,7 +14,7 @@ The guest loader does two things:
>     - load blobs (kernels and initial ram disks) into memory
>     - sets platform FDT data so hypervisors can find and boot them
>   
> -This is what is typically done by a boot-loader like grub using it's
> +This is what is typically done by a boot-loader like grub using its
>   multi-boot capability. A typical example would look like:
>   
>   .. parsed-literal::
> @@ -25,9 +25,9 @@ multi-boot capability. A typical example would look like:
>       -device guest-loader,addr=0x47000000,initrd=rootfs.cpio
>   
>   In the above example the Xen hypervisor is loaded by the -kernel
> -parameter and passed it's boot arguments via -append. The Dom0 guest
> +parameter and passed its boot arguments via -append. The Dom0 guest
>   is loaded into the areas of memory. Each blob will get
> -``/chosen/module@<addr>`` entry in the FDT to indicate it's location and
> +``/chosen/module@<addr>`` entry in the FDT to indicate its location and
>   size. Additional information can be passed with by using additional
>   arguments.
>   

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 8/9] docs/devel: mention the spacing requirement for QOM

[Mark Cave-Ayland]

On 20/04/2023 16:57, Alex Bennée wrote:

> We have a more complete document on QOM but we should at least mention
> the style requirements in the style guide.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   docs/devel/qom.rst   |  2 ++
>   docs/devel/style.rst | 29 +++++++++++++++++++++++++++++
>   2 files changed, 31 insertions(+)
> 
> diff --git a/docs/devel/qom.rst b/docs/devel/qom.rst
> index 3e34b07c98..c9237950d0 100644
> --- a/docs/devel/qom.rst
> +++ b/docs/devel/qom.rst
> @@ -1,3 +1,5 @@
> +.. _qom:
> +
>   ===========================
>   The QEMU Object Model (QOM)
>   ===========================
> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
> index 5bc6f2f095..0bd01f3fca 100644
> --- a/docs/devel/style.rst
> +++ b/docs/devel/style.rst
> @@ -628,6 +628,35 @@ are still some caveats to beware of
>   QEMU Specific Idioms
>   ********************
>   
> +QEMU Object Model Declarations
> +==============================
> +
> +The QEMU Object Model (QOM) provides a framework for handling objects
> +in the base C language. The first declaration of a storage or class
> +structure should always be the parent and leave a visual space between
> +that declaration and the new code.
> +
> +.. code-block:: c
> +
> +    typedef struct MyDeviceState {
> +        DeviceState parent_obj;
> +
> +        /* Properties */
> +        int prop_a;
> +        char *prob_b;
> +        /* Other stuff */
> +        int internal_state;
> +    } MyDeviceState;
> +
> +    typedef struct MyDeviceClass {
> +        ObjectClass parent_class;

This one should be DeviceClass in this particular example.

> +        void (*new_fn1)(void);
> +        bool (*new_fn2)(CPUState *);
> +   } MyDeviceClass;
> +
> +See :ref:`qom` for more details.

A couple of points:

1) It is probably worth removing the typedefs given that they are handled by the 
various QOM macros

2) There should be mention of the fixed names "parent_obj" and "parent_class" for
the first declaration.

How about something like this:


QEMU Object Model Declarations
==============================

The QEMU Object Model (QOM) provides a framework for handling objects
in the base C language. The first declaration of a storage or class
structure should always be the parent and leave a visual space between
that declaration and the new code.

For a storage structure the first declaration should always be called
"parent_obj" and for a class structure the first member should always
be called "parent_class" as below:

.. code-block:: c

     struct MyDeviceState {
         DeviceState parent_obj;

         /* Properties */
         int prop_a;
         char *prob_b;
         /* Other stuff */
         int internal_state;
     };

     struct MyDeviceClass {
         DeviceClass parent_class;

         void (*new_fn1)(void);
         bool (*new_fn2)(CPUState *);
     };

Note that there is no need to provide typedefs for QOM structures since these are 
generated automatically by the QOM declaration macros. See :ref:`qom` for more details.


ATB,

Mark.

Re: [PATCH 1/9] docs/devel/kconfig.rst: Fix incorrect markup

[Juan Quintela]

Alex Bennée <alex.bennee@linaro.org> wrote:
> From: Peter Maydell <peter.maydell@linaro.org>
>
> In rST markup syntax, the inline markup (*italics*, **bold** and
> ``monospaced``) must be separated from the surrending text by
> non-word characters, otherwise it is not interpreted as markup.
> To force interpretation as markup in the middle of a word,
> you need to use a backslash-escaped space (which will not
> appear as a space in the output).
>
> Fix a missing backslash-space in this file, which meant that the ``
> after "select" was output literally and the monospacing was
> incorrectly extended all the way to the end of the next monospaced
> word.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Message-Id: <20230411105424.3994585-1-peter.maydell@linaro.org>

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

Re: [PATCH 2/9] qemu-options.hx: Update descriptions of memory options for NUMA node

[Juan Quintela]

Alex Bennée <alex.bennee@linaro.org> wrote:
> From: Yohei Kojima <y-koj@outlook.jp>
>
> This commit adds the following description:
> 1. `memdev` option is recommended over `mem` option (see [1,2])
> 2. users must specify memory for all NUMA nodes (see [2])
>
> This commit also separates descriptions for `mem` and `memdev` into two
> paragraphs. The old doc describes legacy `mem` option first, and it was
> a bit confusing.
>
> Related documantations:
> [1] https://wiki.qemu.org/ChangeLog/5.1#Incompatible_changes
> [2] https://www.qemu.org/docs/master/about/removed-features.html
>
> Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
> Message-Id: <TYZPR06MB5418D6B0175A49E8E76988439D8E9@TYZPR06MB5418.apcprd06.prod.outlook.com>

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

Re: [PATCH 6/9] docs/system: remove excessive punctuation from guest-loader docs

[Juan Quintela]

Alex Bennée <alex.bennee@linaro.org> wrote:
> A possessive its needs no ' whereas the contraction of it is does.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

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

Re: [PATCH 7/9] docs/devel: make a statement about includes

[Juan Quintela]

Alex Bennée <alex.bennee@linaro.org> wrote:
> While not explicitly disallowing header macro abuse (because that
> would make us hypocrites) lets at least address some things to think
> about.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  docs/devel/style.rst | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
> index 68aa776930..5bc6f2f095 100644
> --- a/docs/devel/style.rst
> +++ b/docs/devel/style.rst
> @@ -300,6 +300,20 @@ putting those into qemu/typedefs.h instead of including the header.
>  
>  Cyclic inclusion is forbidden.
>  
> +Generative Includes
> +-------------------
> +
> +QEMU makes fairly extensive use of the macro pre-processor to
> +instantiate multiple similar functions. While such abuse of the macro
> +processor isn't discouraged it can make debugging and code navigation
> +harder. You should consider carefully if the same effect can be
> +achieved by making it easy for the compiler to constant fold or using
> +python scripting to generate grep friendly code.
> +
> +If you do use template header files they should be named with the
> +``.c.inc`` or ``.h.inc`` suffix to make it clear they are being
> +included for expansion.
> +
>  C types
>  =======

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

Fair enough.

Re: [PATCH 8/9] docs/devel: mention the spacing requirement for QOM

[Juan Quintela]

Alex Bennée <alex.bennee@linaro.org> wrote:
> We have a more complete document on QOM but we should at least mention
> the style requirements in the style guide.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

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

Re: [PATCH 9/9] docs/style: call out the use of GUARD macros

[Juan Quintela]

Alex Bennée <alex.bennee@linaro.org> wrote:
> There use makes our code safer so we should mention them.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>


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

Re: [PATCH 8/9] docs/devel: mention the spacing requirement for QOM

[Philippe Mathieu-Daudé]

On 20/4/23 21:32, Mark Cave-Ayland wrote:
> On 20/04/2023 16:57, Alex Bennée wrote:
> 
>> We have a more complete document on QOM but we should at least mention
>> the style requirements in the style guide.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   docs/devel/qom.rst   |  2 ++
>>   docs/devel/style.rst | 29 +++++++++++++++++++++++++++++
>>   2 files changed, 31 insertions(+)


> A couple of points:
> 
> 1) It is probably worth removing the typedefs given that they are 
> handled by the various QOM macros
> 
> 2) There should be mention of the fixed names "parent_obj" and 
> "parent_class" for
> the first declaration.
> 
> How about something like this:
> 
> 
> QEMU Object Model Declarations
> ==============================
> 
> The QEMU Object Model (QOM) provides a framework for handling objects
> in the base C language. The first declaration of a storage or class
> structure should always be the parent and leave a visual space between

s/should/must/

> that declaration and the new code.
> 
> For a storage structure the first declaration should always be called
> "parent_obj" and for a class structure the first member should always
> be called "parent_class" as below:
> 
> .. code-block:: c
> 
>      struct MyDeviceState {
>          DeviceState parent_obj;
> 
>          /* Properties */
>          int prop_a;
>          char *prob_b;

Should we mention "We recommend placing instance/class properties fields
just after the parent field"?

>          /* Other stuff */
>          int internal_state;
>      };
> 
>      struct MyDeviceClass {
>          DeviceClass parent_class;
> 
>          void (*new_fn1)(void);
>          bool (*new_fn2)(CPUState *);
>      };
> 
> Note that there is no need to provide typedefs for QOM structures since 
> these are generated automatically by the QOM declaration macros. See 
> :ref:`qom` for more details.
> 
> 
> ATB,
> 
> Mark.

Re: [PATCH 9/9] docs/style: call out the use of GUARD macros

[Philippe Mathieu-Daudé]

On 20/4/23 17:57, Alex Bennée wrote:
> There use makes our code safer so we should mention them.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

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

> ---
>   docs/devel/style.rst | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
> index 0bd01f3fca..b50a981a86 100644
> --- a/docs/devel/style.rst
> +++ b/docs/devel/style.rst
> @@ -657,6 +657,42 @@ that declaration and the new code.
>   
>   See :ref:`qom` for more details.
>   
> +QEMU GUARD macros
> +=================
> +
> +QEMU provides a number of ``_GUARD`` macros intended to make the
> +handling of multiple exit paths easier. For example using
> +``QEMU_LOCK_GUARD`` to take a lock will ensure the lock is released on
> +exit from the function.
> +
> +.. code-block:: c
> +
> +    static int my_critical_function(SomeState *s, void *data)
> +    {
> +        QEMU_LOCK_GUARD(&s->lock);
> +        do_thing1(data);
> +        if (check_state2(data)) {
> +            return -1;
> +        }
> +        do_thing3(data);
> +        return 0;
> +    }
> +
> +will ensure s->lock is released however the function is exited. There
> +are often ``WITH_`` forms of macros which more easily wrap around a
> +block inside a function.
> +
> +.. code-block:: c
> +
> +    WITH_RCU_READ_LOCK_GUARD() {
> +        QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
> +            err = do_the_thing(kid->child);
> +            if (err < 0) {
> +                return err;
> +            }
> +        }
> +    }
> +
>   Error handling and reporting
>   ============================
>   

Re: [PATCH 2/9] qemu-options.hx: Update descriptions of memory options for NUMA node

[Philippe Mathieu-Daudé]

On 20/4/23 17:57, Alex Bennée wrote:
> From: Yohei Kojima <y-koj@outlook.jp>
> 
> This commit adds the following description:
> 1. `memdev` option is recommended over `mem` option (see [1,2])
> 2. users must specify memory for all NUMA nodes (see [2])
> 
> This commit also separates descriptions for `mem` and `memdev` into two
> paragraphs. The old doc describes legacy `mem` option first, and it was
> a bit confusing.
> 
> Related documantations:

Typo "documentation".

> [1] https://wiki.qemu.org/ChangeLog/5.1#Incompatible_changes
> [2] https://www.qemu.org/docs/master/about/removed-features.html
> 
> Signed-off-by: Yohei Kojima <y-koj@outlook.jp>
> Message-Id: <TYZPR06MB5418D6B0175A49E8E76988439D8E9@TYZPR06MB5418.apcprd06.prod.outlook.com>
> ---
>   qemu-options.hx | 25 ++++++++++++++++---------
>   1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 59bdf67a2c..174f0d0c2d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -405,15 +405,22 @@ SRST
>           -numa node,nodeid=0 -numa node,nodeid=1 \
>           -numa cpu,node-id=0,socket-id=0 -numa cpu,node-id=1,socket-id=1
>   
> -    Legacy '\ ``mem``\ ' assigns a given RAM amount to a node (not supported
> -    for 5.1 and newer machine types). '\ ``memdev``\ ' assigns RAM from
> -    a given memory backend device to a node. If '\ ``mem``\ ' and
> -    '\ ``memdev``\ ' are omitted in all nodes, RAM is split equally between them.
> -
> -
> -    '\ ``mem``\ ' and '\ ``memdev``\ ' are mutually exclusive.
> -    Furthermore, if one node uses '\ ``memdev``\ ', all of them have to
> -    use it.
> +    '\ ``memdev``\ ' option assigns RAM from a given memory backend
> +    device to a node. It is recommended to use '\ ``memdev``\ ' option
> +    over legacy '\ ``mem``\ ' option. This is because '\ ``memdev``\ '
> +    option provides better performance and more control over the
> +    backend's RAM (e.g. '\ ``prealloc``\ ' parameter of
> +    '\ ``-memory-backend-ram``\ ' allows memory preallocation).
> +
> +    For compatibility reasons, legacy '\ ``mem``\ ' option is
> +    supported in 5.0 and older machine types. Note that '\ ``mem``\ '
> +    and '\ ``memdev``\ ' are mutually exclusive. If one node uses
> +    '\ ``memdev``\ ', the rest nodes have to use '\ ``memdev``\ '
> +    option, and vice versa.
> +
> +    Users must specify memory for all NUMA nodes by '\ ``memdev``\ '
> +    (or legacy '\ ``mem``\ ' if available). In QEMU 5.2, the support
> +    for '\ ``-numa node``\ ' without memory specified was removed.
>   
>       '\ ``initiator``\ ' is an additional option that points to an
>       initiator NUMA node that has best performance (the lowest latency or

Re: [PATCH 9/9] docs/style: call out the use of GUARD macros

[Vladimir Sementsov-Ogievskiy]

On 20.04.23 18:57, Alex Bennée wrote:
> There use makes our code safer so we should mention them.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>


> ---
>   docs/devel/style.rst | 36 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
> 
> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
> index 0bd01f3fca..b50a981a86 100644
> --- a/docs/devel/style.rst
> +++ b/docs/devel/style.rst
> @@ -657,6 +657,42 @@ that declaration and the new code.
>   
>   See :ref:`qom` for more details.
>   
> +QEMU GUARD macros
> +=================
> +
> +QEMU provides a number of ``_GUARD`` macros intended to make the
> +handling of multiple exit paths easier. For example using
> +``QEMU_LOCK_GUARD`` to take a lock will ensure the lock is released on
> +exit from the function.
> +
> +.. code-block:: c
> +
> +    static int my_critical_function(SomeState *s, void *data)
> +    {
> +        QEMU_LOCK_GUARD(&s->lock);
> +        do_thing1(data);
> +        if (check_state2(data)) {
> +            return -1;
> +        }
> +        do_thing3(data);
> +        return 0;
> +    }

For more clearness, maybe add an equivalent code with qemu_mutex_lock() / qemu_mutex_unlock(), I mean:

The equivalent code without _GUARD macro makes us to carefully put qemu_mutex_unlock() on all exit points:

.. code-block:: c

     static int my_critical_function(SomeState *s, void *data)
     {
         qemu_mutex_lock(&s->lock);
         do_thing1(data);
         if (check_state2(data)) {
             qemu_mutex_unlock(&s->lock);
             return -1;
         }
         do_thing3(data);
         qemu_mutex_unlock(&s->lock);
         return 0;
     }

> +
> +will ensure s->lock is released however the function is exited. There
> +are often ``WITH_`` forms of macros which more easily wrap around a
> +block inside a function.
> +
> +.. code-block:: c
> +
> +    WITH_RCU_READ_LOCK_GUARD() {
> +        QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
> +            err = do_the_thing(kid->child);
> +            if (err < 0) {
> +                return err;
> +            }
> +        }
> +    }
> +

and maybe similar here.

>   Error handling and reporting
>   ============================
>   

-- 
Best regards,
Vladimir

Re: [PATCH 4/9] docs/cxl: Fix sentence

[Jonathan Cameron via]

On Thu, 20 Apr 2023 16:57:18 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> From: Stefan Weil via <qemu-devel@nongnu.org>
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Message-Id: <20230409201828.1159568-1-sw@weilnetz.de>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Thomas already sent a pull request with this one in it.
https://lore.kernel.org/qemu-devel/20230420101216.786304-4-thuth@redhat.com/

> ---
>  docs/system/devices/cxl.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/docs/system/devices/cxl.rst b/docs/system/devices/cxl.rst
> index f25783a4ec..4c38223069 100644
> --- a/docs/system/devices/cxl.rst
> +++ b/docs/system/devices/cxl.rst
> @@ -111,7 +111,7 @@ Interfaces provided include:
>  
>  CXL Root Ports (CXL RP)
>  ~~~~~~~~~~~~~~~~~~~~~~~
> -A CXL Root Port servers te same purpose as a PCIe Root Port.
> +A CXL Root Port serves the same purpose as a PCIe Root Port.
>  There are a number of CXL specific Designated Vendor Specific
>  Extended Capabilities (DVSEC) in PCIe Configuration Space
>  and associated component register access via PCI bars.

Re: [PATCH 9/9] docs/style: call out the use of GUARD macros

[Alex Bennée]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

> On 20.04.23 18:57, Alex Bennée wrote:
>> There use makes our code safer so we should mention them.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>
>
>> ---
>>   docs/devel/style.rst | 36 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
>> index 0bd01f3fca..b50a981a86 100644
>> --- a/docs/devel/style.rst
>> +++ b/docs/devel/style.rst
>> @@ -657,6 +657,42 @@ that declaration and the new code.
>>     See :ref:`qom` for more details.
>>   +QEMU GUARD macros
>> +=================
>> +
>> +QEMU provides a number of ``_GUARD`` macros intended to make the
>> +handling of multiple exit paths easier. For example using
>> +``QEMU_LOCK_GUARD`` to take a lock will ensure the lock is released on
>> +exit from the function.
>> +
>> +.. code-block:: c
>> +
>> +    static int my_critical_function(SomeState *s, void *data)
>> +    {
>> +        QEMU_LOCK_GUARD(&s->lock);
>> +        do_thing1(data);
>> +        if (check_state2(data)) {
>> +            return -1;
>> +        }
>> +        do_thing3(data);
>> +        return 0;
>> +    }
>
> For more clearness, maybe add an equivalent code with qemu_mutex_lock() / qemu_mutex_unlock(), I mean:
>
> The equivalent code without _GUARD macro makes us to carefully put qemu_mutex_unlock() on all exit points:
>
> .. code-block:: c
>
>     static int my_critical_function(SomeState *s, void *data)
>     {
>         qemu_mutex_lock(&s->lock);
>         do_thing1(data);
>         if (check_state2(data)) {
>             qemu_mutex_unlock(&s->lock);
>             return -1;
>         }
>         do_thing3(data);
>         qemu_mutex_unlock(&s->lock);
>         return 0;
>     }
>
>> +
>> +will ensure s->lock is released however the function is exited. There
>> +are often ``WITH_`` forms of macros which more easily wrap around a
>> +block inside a function.
>> +
>> +.. code-block:: c
>> +
>> +    WITH_RCU_READ_LOCK_GUARD() {
>> +        QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
>> +            err = do_the_thing(kid->child);
>> +            if (err < 0) {
>> +                return err;
>> +            }
>> +        }
>> +    }
>> +
>
> and maybe similar here.

I added the example although I didn't repeat it for the WITH form
because readers should hopefully have understood the idea with the first
example.


>
>>   Error handling and reporting
>>   ============================
>>   


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 9/9] docs/style: call out the use of GUARD macros

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 12:07, Alex Bennée wrote:
> 
> Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:
> 
>> On 20.04.23 18:57, Alex Bennée wrote:
>>> There use makes our code safer so we should mention them.
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>>
>>
>>> ---
>>>    docs/devel/style.rst | 36 ++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 36 insertions(+)
>>> diff --git a/docs/devel/style.rst b/docs/devel/style.rst
>>> index 0bd01f3fca..b50a981a86 100644
>>> --- a/docs/devel/style.rst
>>> +++ b/docs/devel/style.rst
>>> @@ -657,6 +657,42 @@ that declaration and the new code.
>>>      See :ref:`qom` for more details.
>>>    +QEMU GUARD macros
>>> +=================
>>> +
>>> +QEMU provides a number of ``_GUARD`` macros intended to make the
>>> +handling of multiple exit paths easier. For example using
>>> +``QEMU_LOCK_GUARD`` to take a lock will ensure the lock is released on
>>> +exit from the function.
>>> +
>>> +.. code-block:: c
>>> +
>>> +    static int my_critical_function(SomeState *s, void *data)
>>> +    {
>>> +        QEMU_LOCK_GUARD(&s->lock);
>>> +        do_thing1(data);
>>> +        if (check_state2(data)) {
>>> +            return -1;
>>> +        }
>>> +        do_thing3(data);
>>> +        return 0;
>>> +    }
>>
>> For more clearness, maybe add an equivalent code with qemu_mutex_lock() / qemu_mutex_unlock(), I mean:
>>
>> The equivalent code without _GUARD macro makes us to carefully put qemu_mutex_unlock() on all exit points:
>>
>> .. code-block:: c
>>
>>      static int my_critical_function(SomeState *s, void *data)
>>      {
>>          qemu_mutex_lock(&s->lock);
>>          do_thing1(data);
>>          if (check_state2(data)) {
>>              qemu_mutex_unlock(&s->lock);
>>              return -1;
>>          }
>>          do_thing3(data);
>>          qemu_mutex_unlock(&s->lock);
>>          return 0;
>>      }
>>
>>> +
>>> +will ensure s->lock is released however the function is exited. There
>>> +are often ``WITH_`` forms of macros which more easily wrap around a
>>> +block inside a function.
>>> +
>>> +.. code-block:: c
>>> +
>>> +    WITH_RCU_READ_LOCK_GUARD() {
>>> +        QTAILQ_FOREACH_RCU(kid, &bus->children, sibling) {
>>> +            err = do_the_thing(kid->child);
>>> +            if (err < 0) {
>>> +                return err;
>>> +            }
>>> +        }
>>> +    }
>>> +
>>
>> and maybe similar here.
> 
> I added the example although I didn't repeat it for the WITH form
> because readers should hopefully have understood the idea with the first
> example.
> 

Agreed, thanks!

-- 
Best regards,
Vladimir

Re: [PATCH 8/9] docs/devel: mention the spacing requirement for QOM

[Mark Cave-Ayland]

On 21/04/2023 07:15, Philippe Mathieu-Daudé wrote:

> On 20/4/23 21:32, Mark Cave-Ayland wrote:
>> On 20/04/2023 16:57, Alex Bennée wrote:
>>
>>> We have a more complete document on QOM but we should at least mention
>>> the style requirements in the style guide.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   docs/devel/qom.rst   |  2 ++
>>>   docs/devel/style.rst | 29 +++++++++++++++++++++++++++++
>>>   2 files changed, 31 insertions(+)
> 
> 
>> A couple of points:
>>
>> 1) It is probably worth removing the typedefs given that they are handled by the 
>> various QOM macros
>>
>> 2) There should be mention of the fixed names "parent_obj" and "parent_class" for
>> the first declaration.
>>
>> How about something like this:
>>
>>
>> QEMU Object Model Declarations
>> ==============================
>>
>> The QEMU Object Model (QOM) provides a framework for handling objects
>> in the base C language. The first declaration of a storage or class
>> structure should always be the parent and leave a visual space between
> 
> s/should/must/
> 
>> that declaration and the new code.
>>
>> For a storage structure the first declaration should always be called
>> "parent_obj" and for a class structure the first member should always
>> be called "parent_class" as below:
>>
>> .. code-block:: c
>>
>>      struct MyDeviceState {
>>          DeviceState parent_obj;
>>
>>          /* Properties */
>>          int prop_a;
>>          char *prob_b;
> 
> Should we mention "We recommend placing instance/class properties fields
> just after the parent field"?

I don't think I've ever seen any recommendations on placing instance/class property 
fields other than for the parent DeviceState/DeviceClass?

IMO it doesn't seem worth committing to anything else for now, especially as which 
fields get exposed as properties can be quite fluid these days ;)

>>          /* Other stuff */
>>          int internal_state;
>>      };
>>
>>      struct MyDeviceClass {
>>          DeviceClass parent_class;
>>
>>          void (*new_fn1)(void);
>>          bool (*new_fn2)(CPUState *);
>>      };
>>
>> Note that there is no need to provide typedefs for QOM structures since these are 
>> generated automatically by the QOM declaration macros. See :ref:`qom` for more 
>> details.


ATB,

Mark.