[PATCH for 7.2 v3 00/13] testing and doc updates (pre-PR)

[PATCH for 7.2 v3 00/13] testing and doc updates (pre-PR)

[Alex Bennée]

Hi,

This contains my last set of fixes which didn't make it in the last
pull request I sent. I've dropped the console peek patch which
unearthed a number of additional tests which can't handle even the
smallest delay in avocado draining the console chardev. As a result we
still occasionally miss login prompts but the rest of the tests are
more stable.

New in this iteration:

  - more bumping of timeouts
  - disable the tcg virt tests in CI in favour of a new alpine based one
  - cirrus updates for macos

The following need review:

 - gitlab: integrate coverage report (1 acks, 1 sobs)
 - tests/avocado: skip aarch64 cloud TCG tests in CI
 - tests/avocado: introduce alpine virt test for CI

Alex Bennée (8):
  tests/docker: allow user to override check target
  docs/devel: add a maintainers section to development process
  docs/devel: make language a little less code centric
  docs/devel: simplify the minimal checklist
  docs/devel: try and improve the language around patch review
  tests/avocado: introduce alpine virt test for CI
  tests/avocado: skip aarch64 cloud TCG tests in CI
  gitlab: integrate coverage report

Cédric Le Goater (1):
  tests/avocado/machine_aspeed.py: Reduce noise on the console for SDK
    tests

Daniel P. Berrangé (1):
  ci: replace x86_64 macos-11 with aarch64 macos-12

Peter Maydell (2):
  tests/avocado: Raise timeout for
    boot_linux.py:BootLinuxPPC64.test_pseries_tcg
  tests/avocado/boot_linux.py: Bump aarch64 virt test timeout to 720s

Stefan Weil (1):
  Run docker probe only if docker or podman are available

 docs/devel/code-of-conduct.rst                |   2 +
 docs/devel/index-process.rst                  |   1 +
 docs/devel/maintainers.rst                    | 106 ++++++++++++++++++
 docs/devel/submitting-a-patch.rst             | 101 +++++++++++------
 docs/devel/submitting-a-pull-request.rst      |  12 +-
 configure                                     |   2 +-
 .gitlab-ci.d/buildtest.yml                    |  12 +-
 .gitlab-ci.d/cirrus.yml                       |  12 +-
 .../cirrus/{macos-11.vars => macos-12.vars}   |  12 +-
 tests/avocado/boot_linux.py                   |  13 ++-
 tests/avocado/machine_aarch64_virt.py         |  46 +++++++-
 tests/avocado/machine_aspeed.py               |  17 ++-
 tests/docker/Makefile.include                 |   2 +
 tests/docker/common.rc                        |   6 +-
 tests/lcitool/libvirt-ci                      |   2 +-
 tests/lcitool/refresh                         |   2 +-
 16 files changed, 275 insertions(+), 73 deletions(-)
 create mode 100644 docs/devel/maintainers.rst
 rename .gitlab-ci.d/cirrus/{macos-11.vars => macos-12.vars} (74%)

-- 
2.34.1

[PATCH v3 01/13] Run docker probe only if docker or podman are available

[Alex Bennée]

From: Stefan Weil <sw@weilnetz.de>

The docker probe uses "sudo -n" which can cause an e-mail with a security warning
each time when configure is run. Therefore run docker probe only if either docker
or podman are available.

That avoids the problematic "sudo -n" on build environments which have neither
docker nor podman installed.

Fixes: c4575b59155e2e00 ("configure: store container engine in config-host.mak")
Signed-off-by: Stefan Weil <sw@weilnetz.de>
Message-Id: <20221030083510.310584-1-sw@weilnetz.de>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20221111145529.4020801-2-alex.bennee@linaro.org>
---
 configure | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure b/configure
index 66928692b0..26c7bc5154 100755
--- a/configure
+++ b/configure
@@ -1780,7 +1780,7 @@ fi
 # functions to probe cross compilers
 
 container="no"
-if test $use_containers = "yes"; then
+if test $use_containers = "yes" && (has "docker" || has "podman"); then
     case $($python "$source_path"/tests/docker/docker.py probe) in
         *docker) container=docker ;;
         podman) container=podman ;;
-- 
2.34.1

[PATCH v3 02/13] tests/avocado/machine_aspeed.py: Reduce noise on the console for SDK tests

[Alex Bennée]

From: Cédric Le Goater <clg@kaod.org>

The Aspeed SDK images are based on OpenBMC which starts a lot of
services. The output noise on the console can break from time to time
the test waiting for the logging prompt.

Change the U-Boot bootargs variable to add "quiet" to the kernel
command line and reduce the output volume. This also drops the test on
the CPU id which was nice to have but not essential.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Message-Id: <20221104075347.370503-1-clg@kaod.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20221111145529.4020801-4-alex.bennee@linaro.org>
---
 tests/avocado/machine_aspeed.py | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/tests/avocado/machine_aspeed.py b/tests/avocado/machine_aspeed.py
index fba6527026..1fc385e1c8 100644
--- a/tests/avocado/machine_aspeed.py
+++ b/tests/avocado/machine_aspeed.py
@@ -12,6 +12,7 @@
 from avocado_qemu import wait_for_console_pattern
 from avocado_qemu import exec_command
 from avocado_qemu import exec_command_and_wait_for_pattern
+from avocado_qemu import interrupt_interactive_console_until_pattern
 from avocado.utils import archive
 from avocado import skipIf
 
@@ -182,6 +183,8 @@ def test_arm_ast2600_evb_buildroot(self):
 
 class AST2x00MachineSDK(QemuSystemTest):
 
+    EXTRA_BOOTARGS = ' quiet'
+
     # FIXME: Although these tests boot a whole distro they are still
     # slower than comparable machine models. There may be some
     # optimisations which bring down the runtime. In the meantime they
@@ -194,7 +197,7 @@ def wait_for_console_pattern(self, success_message, vm=None):
                                  failure_message='Kernel panic - not syncing',
                                  vm=vm)
 
-    def do_test_arm_aspeed_sdk_start(self, image, cpu_id):
+    def do_test_arm_aspeed_sdk_start(self, image):
         self.require_netdev('user')
         self.vm.set_console()
         self.vm.add_args('-drive', 'file=' + image + ',if=mtd,format=raw',
@@ -202,9 +205,13 @@ def do_test_arm_aspeed_sdk_start(self, image, cpu_id):
         self.vm.launch()
 
         self.wait_for_console_pattern('U-Boot 2019.04')
-        self.wait_for_console_pattern('## Loading kernel from FIT Image')
+        interrupt_interactive_console_until_pattern(
+            self, 'Hit any key to stop autoboot:', 'ast#')
+        exec_command_and_wait_for_pattern(
+            self, 'setenv bootargs ${bootargs}' + self.EXTRA_BOOTARGS, 'ast#')
+        exec_command_and_wait_for_pattern(
+            self, 'boot', '## Loading kernel from FIT Image')
         self.wait_for_console_pattern('Starting kernel ...')
-        self.wait_for_console_pattern('Booting Linux on physical CPU ' + cpu_id)
 
     @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
     def test_arm_ast2500_evb_sdk(self):
@@ -221,7 +228,7 @@ def test_arm_ast2500_evb_sdk(self):
         archive.extract(image_path, self.workdir)
 
         self.do_test_arm_aspeed_sdk_start(
-            self.workdir + '/ast2500-default/image-bmc', '0x0')
+            self.workdir + '/ast2500-default/image-bmc')
         self.wait_for_console_pattern('ast2500-default login:')
 
     @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
@@ -243,7 +250,7 @@ def test_arm_ast2600_evb_sdk(self):
         self.vm.add_args('-device',
                          'ds1338,bus=aspeed.i2c.bus.5,address=0x32');
         self.do_test_arm_aspeed_sdk_start(
-            self.workdir + '/ast2600-default/image-bmc', '0xf00')
+            self.workdir + '/ast2600-default/image-bmc')
         self.wait_for_console_pattern('ast2600-default login:')
         exec_command_and_wait_for_pattern(self, 'root', 'Password:')
         exec_command_and_wait_for_pattern(self, '0penBmc', 'root@ast2600-default:~#')
-- 
2.34.1

[PATCH v3 03/13] tests/docker: allow user to override check target

[Alex Bennée]

This is useful when trying to bisect a particular failing test behind
a docker run. For example:

  make docker-test-clang@fedora \
    TARGET_LIST=arm-softmmu \
    TEST_COMMAND="meson test qtest-arm/qos-test" \
    J=9 V=1

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20221111145529.4020801-5-alex.bennee@linaro.org>
---
 tests/docker/Makefile.include | 2 ++
 tests/docker/common.rc        | 6 +++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index c87f14477a..fc7a3b7e71 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -184,6 +184,7 @@ docker:
 	@echo '    TARGET_LIST=a,b,c    Override target list in builds.'
 	@echo '    EXTRA_CONFIGURE_OPTS="..."'
 	@echo '                         Extra configure options.'
+	@echo '    TEST_COMMAND="..."   Override the default `make check` target.'
 	@echo '    IMAGES="a b c ..":   Restrict available images to subset.'
 	@echo '    TESTS="x y z .."     Restrict available tests to subset.'
 	@echo '    J=[0..9]*            Overrides the -jN parameter for make commands'
@@ -230,6 +231,7 @@ docker-run: docker-qemu-src
 			$(if $(NETWORK),$(if $(subst $(NETWORK),,1),--net=$(NETWORK)),--net=none) \
 			-e TARGET_LIST=$(subst $(SPACE),$(COMMA),$(TARGET_LIST))	\
 			-e EXTRA_CONFIGURE_OPTS="$(EXTRA_CONFIGURE_OPTS)" \
+			-e TEST_COMMAND="$(TEST_COMMAND)" 		\
 			-e V=$V -e J=$J -e DEBUG=$(DEBUG)		\
 			-e SHOW_ENV=$(SHOW_ENV) 			\
 			$(if $(NOUSER),,				\
diff --git a/tests/docker/common.rc b/tests/docker/common.rc
index e6f8cee0d6..9a33df2832 100755
--- a/tests/docker/common.rc
+++ b/tests/docker/common.rc
@@ -63,12 +63,12 @@ check_qemu()
 {
     # default to make check unless the caller specifies
     if [ $# = 0 ]; then
-        INVOCATION="check"
+        INVOCATION="${TEST_COMMAND:-make $MAKEFLAGS check}"
     else
-        INVOCATION="$@"
+        INVOCATION="make $MAKEFLAGS $@"
     fi
 
-    make $MAKEFLAGS $INVOCATION
+    $INVOCATION
 }
 
 test_fail()
-- 
2.34.1

[PATCH v3 04/13] docs/devel: add a maintainers section to development process

[Alex Bennée]

We don't currently have a clear place in the documentation to describe
the roles and responsibilities of a maintainer. Lets create one so we
can. I've moved a few small bits out of other files to try and keep
everything in one place.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20221111145529.4020801-6-alex.bennee@linaro.org>
---
 docs/devel/code-of-conduct.rst           |   2 +
 docs/devel/index-process.rst             |   1 +
 docs/devel/maintainers.rst               | 106 +++++++++++++++++++++++
 docs/devel/submitting-a-pull-request.rst |  12 +--
 4 files changed, 113 insertions(+), 8 deletions(-)
 create mode 100644 docs/devel/maintainers.rst

diff --git a/docs/devel/code-of-conduct.rst b/docs/devel/code-of-conduct.rst
index 195444d1b4..f734ed0317 100644
--- a/docs/devel/code-of-conduct.rst
+++ b/docs/devel/code-of-conduct.rst
@@ -1,3 +1,5 @@
+.. _code_of_conduct:
+
 Code of Conduct
 ===============
 
diff --git a/docs/devel/index-process.rst b/docs/devel/index-process.rst
index d0d7a200fd..d50dd74c3e 100644
--- a/docs/devel/index-process.rst
+++ b/docs/devel/index-process.rst
@@ -8,6 +8,7 @@ Notes about how to interact with the community and how and where to submit patch
 
    code-of-conduct
    conflict-resolution
+   maintainers
    style
    submitting-a-patch
    trivial-patches
diff --git a/docs/devel/maintainers.rst b/docs/devel/maintainers.rst
new file mode 100644
index 0000000000..05110909d1
--- /dev/null
+++ b/docs/devel/maintainers.rst
@@ -0,0 +1,106 @@
+.. _maintainers:
+
+The Role of Maintainers
+=======================
+
+Maintainers are a critical part of the project's contributor ecosystem.
+They come from a wide range of backgrounds from unpaid hobbyists
+working in their spare time to employees who work on the project as
+part of their job. Maintainer activities include:
+
+  - reviewing patches and suggesting changes
+  - collecting patches and preparing pull requests
+  - tending to the long term health of their area
+  - participating in other project activities
+
+They are also human and subject to the same pressures as everyone else
+including overload and burnout. Like everyone else they are subject
+to project's :ref:`code_of_conduct` and should also be exemplars of
+excellent community collaborators.
+
+The MAINTAINERS file
+--------------------
+
+The `MAINTAINERS
+<https://gitlab.com/qemu-project/qemu/-/blob/master/MAINTAINERS>`__
+file contains the canonical list of who is a maintainer. The file
+is machine readable so an appropriately configured git (see
+:ref:`cc_the_relevant_maintainer`) can automatically Cc them on
+patches that touch their area of code.
+
+The file also describes the status of the area of code to give an idea
+of how actively that section is maintained.
+
+.. list-table:: Meaning of support status in MAINTAINERS
+   :widths: 25 75
+   :header-rows: 1
+
+   * - Status
+     - Meaning
+   * - Supported
+     - Someone is actually paid to look after this.
+   * - Maintained
+     - Someone actually looks after it.
+   * - Odd Fixes
+     - It has a maintainer but they don't have time to do
+       much other than throw the odd patch in.
+   * - Orphan
+     - No current maintainer.
+   * - Obsolete
+     - Old obsolete code, should use something else.
+
+Please bear in mind that even if someone is paid to support something
+it does not mean they are paid to support you. This is open source and
+the code comes with no warranty and the project makes no guarantees
+about dealing with bugs or features requests.
+
+
+
+Becoming a reviewer
+-------------------
+
+Most maintainers start by becoming subsystem reviewers. While anyone
+is welcome to review code on the mailing list getting added to the
+MAINTAINERS file with a line like::
+
+  R: Random Hacker <rhacker@example.com>
+
+will ensure that patches touching a given subsystem will automatically
+be CC'd to you.
+
+Becoming a maintainer
+---------------------
+
+Maintainers are volunteers who put themselves forward or have been
+asked by others to keep an eye on an area of code. They have generally
+demonstrated to the community, usually via contributions and code
+reviews, that they have a good understanding of the subsystem. They
+are also trusted to make a positive contribution to the project and
+work well with the other contributors.
+
+The process is simple - simply send a patch to the list that updates
+the ``MAINTAINERS`` file. Sometimes this is done as part of a larger
+series when a new sub-system is being added to the code base. This can
+also be done by a retiring maintainer who nominates their replacement
+after discussion with other contributors.
+
+Once the patch is reviewed and merged the only other step is to make
+sure your GPG key is signed.
+
+.. _maintainer_keys:
+
+Maintainer GPG Keys
+~~~~~~~~~~~~~~~~~~~
+
+GPG is used to sign pull requests so they can be identified as really
+coming from the maintainer. If your key is not already signed by
+members of the QEMU community, you should make arrangements to attend
+a `KeySigningParty <https://wiki.qemu.org/KeySigningParty>`__ (for
+example at KVM Forum) or make alternative arrangements to have your
+key signed by an attendee. Key signing requires meeting another
+community member **in person** [#]_ so please make appropriate
+arrangements.
+
+.. [#] In recent pandemic times we have had to exercise some
+       flexibility here. Maintainers still need to sign their pull
+       requests though.
diff --git a/docs/devel/submitting-a-pull-request.rst b/docs/devel/submitting-a-pull-request.rst
index c9d1e8afd9..a4cd7ebbb6 100644
--- a/docs/devel/submitting-a-pull-request.rst
+++ b/docs/devel/submitting-a-pull-request.rst
@@ -53,14 +53,10 @@ series) and that "make check" passes before sending out the pull
 request. As a submaintainer you're one of QEMU's lines of defense
 against bad code, so double check the details.
 
-**All pull requests must be signed**. If your key is not already signed
-by members of the QEMU community, you should make arrangements to attend
-a `KeySigningParty <https://wiki.qemu.org/KeySigningParty>`__ (for
-example at KVM Forum) or make alternative arrangements to have your key
-signed by an attendee.  Key signing requires meeting another community
-member \*in person\* so please make appropriate arrangements.  By
-"signed" here we mean that the pullreq email should quote a tag which is
-a GPG-signed tag (as created with 'gpg tag -s ...').
+**All pull requests must be signed**. By "signed" here we mean that
+the pullreq email should quote a tag which is a GPG-signed tag (as
+created with 'gpg tag -s ...'). See :ref:`maintainer_keys` for
+details.
 
 **Pull requests not for master should say "not for master" and have
 "PULL SUBSYSTEM whatever" in the subject tag**. If your pull request is
-- 
2.34.1

Re: [PATCH v3 04/13] docs/devel: add a maintainers section to development process

[Philippe Mathieu-Daudé]

On 17/11/22 18:25, Alex Bennée wrote:
> We don't currently have a clear place in the documentation to describe
> the roles and responsibilities of a maintainer. Lets create one so we
> can. I've moved a few small bits out of other files to try and keep
> everything in one place.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-Id: <20221111145529.4020801-6-alex.bennee@linaro.org>
> ---
>   docs/devel/code-of-conduct.rst           |   2 +
>   docs/devel/index-process.rst             |   1 +
>   docs/devel/maintainers.rst               | 106 +++++++++++++++++++++++
>   docs/devel/submitting-a-pull-request.rst |  12 +--
>   4 files changed, 113 insertions(+), 8 deletions(-)
>   create mode 100644 docs/devel/maintainers.rst

> +The Role of Maintainers
> +=======================
> +
> +Maintainers are a critical part of the project's contributor ecosystem.
> +They come from a wide range of backgrounds from unpaid hobbyists
> +working in their spare time to employees who work on the project as
> +part of their job. Maintainer activities include:
> +
> +  - reviewing patches and suggesting changes
> +  - collecting patches and preparing pull requests
> +  - tending to the long term health of their area
> +  - participating in other project activities
> +
> +They are also human and subject to the same pressures as everyone else
> +including overload and burnout. Like everyone else they are subject
> +to project's :ref:`code_of_conduct` and should also be exemplars of
> +excellent community collaborators.
> +
> +The MAINTAINERS file
> +--------------------
> +
> +The `MAINTAINERS
> +<https://gitlab.com/qemu-project/qemu/-/blob/master/MAINTAINERS>`__
> +file contains the canonical list of who is a maintainer. The file
> +is machine readable so an appropriately configured git (see
> +:ref:`cc_the_relevant_maintainer`) can automatically Cc them on
> +patches that touch their area of code.
> +
> +The file also describes the status of the area of code to give an idea
> +of how actively that section is maintained.
> +
> +.. list-table:: Meaning of support status in MAINTAINERS
> +   :widths: 25 75
> +   :header-rows: 1
> +
> +   * - Status
> +     - Meaning
> +   * - Supported
> +     - Someone is actually paid to look after this.
> +   * - Maintained
> +     - Someone actually looks after it.
> +   * - Odd Fixes
> +     - It has a maintainer but they don't have time to do
> +       much other than throw the odd patch in.
> +   * - Orphan
> +     - No current maintainer.
> +   * - Obsolete
> +     - Old obsolete code, should use something else.

We could add a line in MAINTAINERS 'Descriptions of section entries'
header: "If you modify this section, please keep in sync with the
description in docs/devel/maintainers.rst".

> +Becoming a reviewer
> +-------------------
> +
> +Most maintainers start by becoming subsystem reviewers. While anyone
> +is welcome to review code on the mailing list getting added to the
> +MAINTAINERS file with a line like::
> +
> +  R: Random Hacker <rhacker@example.com>
> +
> +will ensure that patches touching a given subsystem will automatically
> +be CC'd to you.

Although 'R:' tag means 'designated reviewer', such reviewers is
expected to provide regular spontaneous feedback. Otherwise being
subscribed to the list is sufficient.

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

Re: [PATCH v3 04/13] docs/devel: add a maintainers section to development process

[Alex Bennée]

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

> On 17/11/22 18:25, Alex Bennée wrote:
>> We don't currently have a clear place in the documentation to describe
>> the roles and responsibilities of a maintainer. Lets create one so we
>> can. I've moved a few small bits out of other files to try and keep
>> everything in one place.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Message-Id: <20221111145529.4020801-6-alex.bennee@linaro.org>
>> ---
>>   docs/devel/code-of-conduct.rst           |   2 +
>>   docs/devel/index-process.rst             |   1 +
>>   docs/devel/maintainers.rst               | 106 +++++++++++++++++++++++
>>   docs/devel/submitting-a-pull-request.rst |  12 +--
>>   4 files changed, 113 insertions(+), 8 deletions(-)
>>   create mode 100644 docs/devel/maintainers.rst
>
>> +The Role of Maintainers
>> +=======================
>> +
>> +Maintainers are a critical part of the project's contributor ecosystem.
>> +They come from a wide range of backgrounds from unpaid hobbyists
>> +working in their spare time to employees who work on the project as
>> +part of their job. Maintainer activities include:
>> +
>> +  - reviewing patches and suggesting changes
>> +  - collecting patches and preparing pull requests
>> +  - tending to the long term health of their area
>> +  - participating in other project activities
>> +
>> +They are also human and subject to the same pressures as everyone else
>> +including overload and burnout. Like everyone else they are subject
>> +to project's :ref:`code_of_conduct` and should also be exemplars of
>> +excellent community collaborators.
>> +
>> +The MAINTAINERS file
>> +--------------------
>> +
>> +The `MAINTAINERS
>> +<https://gitlab.com/qemu-project/qemu/-/blob/master/MAINTAINERS>`__
>> +file contains the canonical list of who is a maintainer. The file
>> +is machine readable so an appropriately configured git (see
>> +:ref:`cc_the_relevant_maintainer`) can automatically Cc them on
>> +patches that touch their area of code.
>> +
>> +The file also describes the status of the area of code to give an idea
>> +of how actively that section is maintained.
>> +
>> +.. list-table:: Meaning of support status in MAINTAINERS
>> +   :widths: 25 75
>> +   :header-rows: 1
>> +
>> +   * - Status
>> +     - Meaning
>> +   * - Supported
>> +     - Someone is actually paid to look after this.
>> +   * - Maintained
>> +     - Someone actually looks after it.
>> +   * - Odd Fixes
>> +     - It has a maintainer but they don't have time to do
>> +       much other than throw the odd patch in.
>> +   * - Orphan
>> +     - No current maintainer.
>> +   * - Obsolete
>> +     - Old obsolete code, should use something else.
>
> We could add a line in MAINTAINERS 'Descriptions of section entries'
> header: "If you modify this section, please keep in sync with the
> description in docs/devel/maintainers.rst".
>
>> +Becoming a reviewer
>> +-------------------
>> +
>> +Most maintainers start by becoming subsystem reviewers. While anyone
>> +is welcome to review code on the mailing list getting added to the
>> +MAINTAINERS file with a line like::
>> +
>> +  R: Random Hacker <rhacker@example.com>
>> +
>> +will ensure that patches touching a given subsystem will automatically
>> +be CC'd to you.
>
> Although 'R:' tag means 'designated reviewer', such reviewers is
> expected to provide regular spontaneous feedback. Otherwise being
> subscribed to the list is sufficient.

I've used a slightly different form for the flow:

  Becoming a reviewer
  -------------------

  Most maintainers start by becoming subsystem reviewers. While anyone
  is welcome to review code on the mailing list getting added to the
  MAINTAINERS file with a line like::

    R: Random Hacker <rhacker@example.com>

  marks you as a 'designated reviewer' - expected to provide regular
  spontaneous feedback. This will ensure that patches touching a given
  subsystem will automatically be CC'd to you.

we can of course always improve later ;-)

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


-- 
Alex Bennée

[PATCH v3 05/13] docs/devel: make language a little less code centric

[Alex Bennée]

We welcome all sorts of patches.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20221111145529.4020801-7-alex.bennee@linaro.org>
---
 docs/devel/submitting-a-patch.rst | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst
index fec33ce148..9c7c4331f3 100644
--- a/docs/devel/submitting-a-patch.rst
+++ b/docs/devel/submitting-a-patch.rst
@@ -3,11 +3,11 @@
 Submitting a Patch
 ==================
 
-QEMU welcomes contributions of code (either fixing bugs or adding new
-functionality). However, we get a lot of patches, and so we have some
-guidelines about submitting patches. If you follow these, you'll help
-make our task of code review easier and your patch is likely to be
-committed faster.
+QEMU welcomes contributions to fix bugs, add functionality or improve
+the documentation. However, we get a lot of patches, and so we have
+some guidelines about submitting them. If you follow these, you'll
+help make our task of code review easier and your patch is likely to
+be committed faster.
 
 This page seems very long, so if you are only trying to post a quick
 one-shot fix, the bare minimum we ask is that:
-- 
2.34.1

Re: [PATCH v3 05/13] docs/devel: make language a little less code centric

[Philippe Mathieu-Daudé]

On 17/11/22 18:25, Alex Bennée wrote:
> We welcome all sorts of patches.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-Id: <20221111145529.4020801-7-alex.bennee@linaro.org>
> ---
>   docs/devel/submitting-a-patch.rst | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst
> index fec33ce148..9c7c4331f3 100644
> --- a/docs/devel/submitting-a-patch.rst
> +++ b/docs/devel/submitting-a-patch.rst
> @@ -3,11 +3,11 @@
>   Submitting a Patch
>   ==================
>   
> -QEMU welcomes contributions of code (either fixing bugs or adding new
> -functionality). However, we get a lot of patches, and so we have some
> -guidelines about submitting patches. If you follow these, you'll help
> -make our task of code review easier and your patch is likely to be
> -committed faster.
> +QEMU welcomes contributions to fix bugs, add functionality or improve
> +the documentation. However, we get a lot of patches, and so we have
> +some guidelines about submitting them. If you follow these, you'll
> +help make our task of code review easier and your patch is likely to
> +be committed faster.

Less code centric:

"... you'll help make our task of contribution review easier and your 
change is likely to be accepted and committed faster."

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

[PATCH v3 06/13] docs/devel: simplify the minimal checklist

[Alex Bennée]

The bullet points are quite long and contain process tips. Move those
bits of the bullet to the relevant sections and link to them. Use a
table for nicer formatting of the checklist.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20221111145529.4020801-8-alex.bennee@linaro.org>
---
 docs/devel/submitting-a-patch.rst | 75 ++++++++++++++++++++-----------
 1 file changed, 49 insertions(+), 26 deletions(-)

diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst
index 9c7c4331f3..1f2bde0625 100644
--- a/docs/devel/submitting-a-patch.rst
+++ b/docs/devel/submitting-a-patch.rst
@@ -12,25 +12,18 @@ be committed faster.
 This page seems very long, so if you are only trying to post a quick
 one-shot fix, the bare minimum we ask is that:
 
--  You **must** provide a Signed-off-by: line (this is a hard
-   requirement because it's how you say "I'm legally okay to contribute
-   this and happy for it to go into QEMU", modeled after the `Linux kernel
-   <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__
-   policy.) ``git commit -s`` or ``git format-patch -s`` will add one.
--  All contributions to QEMU must be **sent as patches** to the
-   qemu-devel `mailing list <https://wiki.qemu.org/Contribute/MailingLists>`__.
-   Patch contributions should not be posted on the bug tracker, posted on
-   forums, or externally hosted and linked to. (We have other mailing lists too,
-   but all patches must go to qemu-devel, possibly with a Cc: to another
-   list.) ``git send-email`` (`step-by-step setup
-   guide <https://git-send-email.io/>`__ and `hints and
-   tips <https://elixir.bootlin.com/linux/latest/source/Documentation/process/email-clients.rst>`__)
-   works best for delivering the patch without mangling it, but
-   attachments can be used as a last resort on a first-time submission.
--  You must read replies to your message, and be willing to act on them.
-   Note, however, that maintainers are often willing to manually fix up
-   first-time contributions, since there is a learning curve involved in
-   making an ideal patch submission.
+.. list-table:: Minimal Checklist for Patches
+   :widths: 35 65
+   :header-rows: 1
+
+   * - Check
+     - Reason
+   * - Patches contain Signed-off-by: Real Name <author@email>
+     - States you are legally able to contribute the code. See :ref:`patch_emails_must_include_a_signed_off_by_line`
+   * - Sent as patch emails to ``qemu-devel@nongnu.org``
+     - The project uses an email list based workflow. See :ref:`submitting_your_patches`
+   * - Be prepared to respond to review comments
+     - Code that doesn't pass review will not get merged. See :ref:`participating_in_code_review`
 
 You do not have to subscribe to post (list policy is to reply-to-all to
 preserve CCs and keep non-subscribers in the loop on the threads they
@@ -229,6 +222,19 @@ bisection doesn't land on a known-broken state.
 Submitting your Patches
 -----------------------
 
+The QEMU project uses a public email based workflow for reviewing and
+merging patches. As a result all contributions to QEMU must be **sent
+as patches** to the qemu-devel `mailing list
+<https://wiki.qemu.org/Contribute/MailingLists>`__. Patch
+contributions should not be posted on the bug tracker, posted on
+forums, or externally hosted and linked to. (We have other mailing
+lists too, but all patches must go to qemu-devel, possibly with a Cc:
+to another list.) ``git send-email`` (`step-by-step setup guide
+<https://git-send-email.io/>`__ and `hints and tips
+<https://elixir.bootlin.com/linux/latest/source/Documentation/process/email-clients.rst>`__)
+works best for delivering the patch without mangling it, but
+attachments can be used as a last resort on a first-time submission.
+
 .. _if_you_cannot_send_patch_emails:
 
 If you cannot send patch emails
@@ -314,10 +320,12 @@ git repository to fetch the original commit.
 Patch emails must include a ``Signed-off-by:`` line
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-For more information see `SubmittingPatches 1.12
-<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
-This is vital or we will not be able to apply your patch! Please use
-your real name to sign a patch (not an alias or acronym).
+Your patches **must** include a Signed-off-by: line. This is a hard
+requirement because it's how you say "I'm legally okay to contribute
+this and happy for it to go into QEMU". The process is modelled after
+the `Linux kernel
+<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__
+policy.
 
 If you wrote the patch, make sure your "From:" and "Signed-off-by:"
 lines use the same spelling. It's okay if you subscribe or contribute to
@@ -327,6 +335,11 @@ include a "From:" line in the body of the email (different from your
 envelope From:) that will give credit to the correct author; but again,
 that author's Signed-off-by: line is mandatory, with the same spelling.
 
+There are various tooling options for automatically adding these tags
+include using ``git commit -s`` or ``git format-patch -s``. For more
+information see `SubmittingPatches 1.12
+<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
+
 .. _include_a_meaningful_cover_letter:
 
 Include a meaningful cover letter
@@ -397,9 +410,19 @@ Participating in Code Review
 ----------------------------
 
 All patches submitted to the QEMU project go through a code review
-process before they are accepted. Some areas of code that are well
-maintained may review patches quickly, lesser-loved areas of code may
-have a longer delay.
+process before they are accepted. This will often mean a series will
+go through a number of iterations before being picked up by
+:ref:`maintainers<maintainers>`. You therefore should be prepared to
+read replies to your messages and be willing to act on them.
+
+Maintainers are often willing to manually fix up first-time
+contributions, since there is a learning curve involved in making an
+ideal patch submission. However for the best results you should
+proactively respond to suggestions with changes or justifications for
+your current approach.
+
+Some areas of code that are well maintained may review patches
+quickly, lesser-loved areas of code may have a longer delay.
 
 .. _stay_around_to_fix_problems_raised_in_code_review:
 
-- 
2.34.1

Re: [PATCH v3 06/13] docs/devel: simplify the minimal checklist

[Philippe Mathieu-Daudé]

On 17/11/22 18:25, Alex Bennée wrote:
> The bullet points are quite long and contain process tips. Move those
> bits of the bullet to the relevant sections and link to them. Use a
> table for nicer formatting of the checklist.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-Id: <20221111145529.4020801-8-alex.bennee@linaro.org>
> ---
>   docs/devel/submitting-a-patch.rst | 75 ++++++++++++++++++++-----------
>   1 file changed, 49 insertions(+), 26 deletions(-)

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

Re: [PATCH v3 06/13] docs/devel: simplify the minimal checklist

[Philippe Mathieu-Daudé]

Hi Alex,

On 17/11/22 18:25, Alex Bennée wrote:
> The bullet points are quite long and contain process tips. Move those
> bits of the bullet to the relevant sections and link to them. Use a
> table for nicer formatting of the checklist.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-Id: <20221111145529.4020801-8-alex.bennee@linaro.org>
> ---
>   docs/devel/submitting-a-patch.rst | 75 ++++++++++++++++++++-----------
>   1 file changed, 49 insertions(+), 26 deletions(-)


> @@ -314,10 +320,12 @@ git repository to fetch the original commit.
>   Patch emails must include a ``Signed-off-by:`` line
>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   
> -For more information see `SubmittingPatches 1.12
> -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
> -This is vital or we will not be able to apply your patch! Please use
> -your real name to sign a patch (not an alias or acronym).

Revisiting this patch, asking for some real name instead of alias
was at least helpful during patch review, we could address the
contributor by its name. Addressing an acronym is socially weird
(at least in my culture netiquette).

> +Your patches **must** include a Signed-off-by: line. This is a hard
> +requirement because it's how you say "I'm legally okay to contribute
> +this and happy for it to go into QEMU". The process is modelled after
> +the `Linux kernel
> +<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__
> +policy.

Re: [PATCH v3 06/13] docs/devel: simplify the minimal checklist

[Philippe Mathieu-Daudé]

Ping?

On 5/7/23 13:44, Philippe Mathieu-Daudé wrote:
> Hi Alex,
> 
> On 17/11/22 18:25, Alex Bennée wrote:
>> The bullet points are quite long and contain process tips. Move those
>> bits of the bullet to the relevant sections and link to them. Use a
>> table for nicer formatting of the checklist.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Message-Id: <20221111145529.4020801-8-alex.bennee@linaro.org>
>> ---
>>   docs/devel/submitting-a-patch.rst | 75 ++++++++++++++++++++-----------
>>   1 file changed, 49 insertions(+), 26 deletions(-)
> 
> 
>> @@ -314,10 +320,12 @@ git repository to fetch the original commit.
>>   Patch emails must include a ``Signed-off-by:`` line
>>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> -For more information see `SubmittingPatches 1.12
>> -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
>> -This is vital or we will not be able to apply your patch! Please use
>> -your real name to sign a patch (not an alias or acronym).
> 
> Revisiting this patch, asking for some real name instead of alias
> was at least helpful during patch review, we could address the
> contributor by its name. Addressing an acronym is socially weird
> (at least in my culture netiquette).
> 
>> +Your patches **must** include a Signed-off-by: line. This is a hard
>> +requirement because it's how you say "I'm legally okay to contribute
>> +this and happy for it to go into QEMU". The process is modelled after
>> +the `Linux kernel
>> +<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__
>> +policy.

Re: [PATCH v3 06/13] docs/devel: simplify the minimal checklist

[Alex Bennée]

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

> Ping?
>
> On 5/7/23 13:44, Philippe Mathieu-Daudé wrote:
>> Hi Alex,
>> On 17/11/22 18:25, Alex Bennée wrote:
>>> The bullet points are quite long and contain process tips. Move those
>>> bits of the bullet to the relevant sections and link to them. Use a
>>> table for nicer formatting of the checklist.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Message-Id: <20221111145529.4020801-8-alex.bennee@linaro.org>
>>> ---
>>>   docs/devel/submitting-a-patch.rst | 75 ++++++++++++++++++++-----------
>>>   1 file changed, 49 insertions(+), 26 deletions(-)
>> 
>>> @@ -314,10 +320,12 @@ git repository to fetch the original commit.
>>>   Patch emails must include a ``Signed-off-by:`` line
>>>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> -For more information see `SubmittingPatches 1.12
>>> -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
>>> -This is vital or we will not be able to apply your patch! Please use
>>> -your real name to sign a patch (not an alias or acronym).
>> Revisiting this patch, asking for some real name instead of alias
>> was at least helpful during patch review, we could address the
>> contributor by its name. Addressing an acronym is socially weird
>> (at least in my culture netiquette).

Is it that weird? We use nicks all the time on IRC.

The only driver for real names for the signoff is its harder to have
confidence the contribution is valid because you might not be able to
find who is behind an anonymous nick if something comes up later.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 06/13] docs/devel: simplify the minimal checklist

[Daniel P. Berrangé]

On Fri, Sep 01, 2023 at 11:08:15AM +0100, Alex Bennée wrote:
> 
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
> > Ping?
> >
> > On 5/7/23 13:44, Philippe Mathieu-Daudé wrote:
> >> Hi Alex,
> >> On 17/11/22 18:25, Alex Bennée wrote:
> >>> The bullet points are quite long and contain process tips. Move those
> >>> bits of the bullet to the relevant sections and link to them. Use a
> >>> table for nicer formatting of the checklist.
> >>>
> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> >>> Message-Id: <20221111145529.4020801-8-alex.bennee@linaro.org>
> >>> ---
> >>>   docs/devel/submitting-a-patch.rst | 75 ++++++++++++++++++++-----------
> >>>   1 file changed, 49 insertions(+), 26 deletions(-)
> >> 
> >>> @@ -314,10 +320,12 @@ git repository to fetch the original commit.
> >>>   Patch emails must include a ``Signed-off-by:`` line
> >>>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>> -For more information see `SubmittingPatches 1.12
> >>> -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__.
> >>> -This is vital or we will not be able to apply your patch! Please use
> >>> -your real name to sign a patch (not an alias or acronym).
> >> Revisiting this patch, asking for some real name instead of alias
> >> was at least helpful during patch review, we could address the
> >> contributor by its name. Addressing an acronym is socially weird
> >> (at least in my culture netiquette).
> 
> Is it that weird? We use nicks all the time on IRC.
> 
> The only driver for real names for the signoff is its harder to have
> confidence the contribution is valid because you might not be able to
> find who is behind an anonymous nick if something comes up later.

The Signed-off-by is intended as a legal attestation of permission
to contribute. Having the signoff use an obviously anonymous nick
could be said to undermine the legal value of the Signed-off-by.

That's not to say something that /looks/ like a real name is
actually the persons real name - we can't prove that without ID
checks which we're not going to do.

Something that looks like a real name is at least plausible to
accept as a persons' real world identity, where as a nick is
clearly just an online persona where there is an explicit intent
to hide the real identify. This kind of distinction / intent often
matters in the legal arena.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v3 07/13] docs/devel: try and improve the language around patch review

[Alex Bennée]

It is important that contributors take the review process seriously
and we collaborate in a respectful way while avoiding personal
attacks. Try and make this clear in the language.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20221111145529.4020801-9-alex.bennee@linaro.org>
---
 docs/devel/submitting-a-patch.rst | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst
index 1f2bde0625..80e8693bb6 100644
--- a/docs/devel/submitting-a-patch.rst
+++ b/docs/devel/submitting-a-patch.rst
@@ -434,14 +434,20 @@ developers will identify bugs, or suggest a cleaner approach, or even
 just point out code style issues or commit message typos. You'll need to
 respond to these, and then send a second version of your patches with
 the issues fixed. This takes a little time and effort on your part, but
-if you don't do it then your changes will never get into QEMU. It's also
-just polite -- it is quite disheartening for a developer to spend time
-reviewing your code and suggesting improvements, only to find that
-you're not going to do anything further and it was all wasted effort.
+if you don't do it then your changes will never get into QEMU.
+
+Remember that a maintainer is under no obligation to take your
+patches. If someone has spent the time reviewing your code and
+suggesting improvements and you simply re-post without either
+addressing the comment directly or providing additional justification
+for the change then it becomes wasted effort. You cannot demand others
+merge and then fix up your code after the fact.
 
 When replying to comments on your patches **reply to all and not just
 the sender** -- keeping discussion on the mailing list means everybody
-can follow it.
+can follow it. Remember the spirit of the :ref:`code_of_conduct` and
+keep discussions respectful and collaborative and avoid making
+personal comments.
 
 .. _pay_attention_to_review_comments:
 
-- 
2.34.1

Re: [PATCH v3 07/13] docs/devel: try and improve the language around patch review

[Philippe Mathieu-Daudé]

On 17/11/22 18:25, Alex Bennée wrote:
> It is important that contributors take the review process seriously
> and we collaborate in a respectful way while avoiding personal
> attacks. Try and make this clear in the language.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Message-Id: <20221111145529.4020801-9-alex.bennee@linaro.org>
> ---
>   docs/devel/submitting-a-patch.rst | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)

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

[PATCH v3 08/13] tests/avocado: Raise timeout for boot_linux.py:BootLinuxPPC64.test_pseries_tcg

[Alex Bennée]

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

On my machine, a debug build of QEMU takes about 260 seconds to
complete this test, so with the current timeout value of 180 seconds
it always times out.  Double the timeout value to 360 so the test
definitely has enough time to complete.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20221110142901.3832318-1-peter.maydell@linaro.org>
Message-Id: <20221111145529.4020801-11-alex.bennee@linaro.org>
---
 tests/avocado/boot_linux.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/avocado/boot_linux.py b/tests/avocado/boot_linux.py
index 571d33882a..2be4be395d 100644
--- a/tests/avocado/boot_linux.py
+++ b/tests/avocado/boot_linux.py
@@ -116,7 +116,7 @@ class BootLinuxPPC64(LinuxTest):
     :avocado: tags=arch:ppc64
     """
 
-    timeout = 180
+    timeout = 360
 
     def test_pseries_tcg(self):
         """
-- 
2.34.1

[PATCH v3 09/13] tests/avocado: introduce alpine virt test for CI

[Alex Bennée]

The boot_linux tests download and run a full cloud image boot and
start a full distro. While the ability to test the full boot chain is
worthwhile it is perhaps a little too heavy weight and causes issues
in CI. Fix this by introducing a new alpine linux ISO boot in
machine_aarch64_virt.

This boots a fully loaded -cpu max with all the bells and whistles in
31s on my machine. A full debug build takes around 180s on my machine
so we set a more generous timeout to cover that.

We don't add a test for lesser GIC versions although there is some
coverage for that already in the boot_xen.py tests. If we want to
introduce more comprehensive testing we can do it with a custom kernel
and initrd rather than a full distro boot.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v1
  - use "virt" image instead (even faster)
  - don't drop boot_linux (it is now disabled for CI)
  - re-phrase commit message
  - add alpine to the test name
---
 tests/avocado/machine_aarch64_virt.py | 46 ++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/tests/avocado/machine_aarch64_virt.py b/tests/avocado/machine_aarch64_virt.py
index 21848cba70..c2b2ba2cf8 100644
--- a/tests/avocado/machine_aarch64_virt.py
+++ b/tests/avocado/machine_aarch64_virt.py
@@ -1,4 +1,5 @@
-# Functional test that boots a Linux kernel and checks the console
+# Functional test that boots a various Linux systems and checks the
+# console output.
 #
 # Copyright (c) 2022 Linaro Ltd.
 #
@@ -8,19 +9,62 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 
 import time
+import os
 
 from avocado_qemu import QemuSystemTest
 from avocado_qemu import wait_for_console_pattern
 from avocado_qemu import exec_command
+from avocado_qemu import BUILD_DIR
 
 class Aarch64VirtMachine(QemuSystemTest):
     KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
+    timeout = 360
 
     def wait_for_console_pattern(self, success_message, vm=None):
         wait_for_console_pattern(self, success_message,
                                  failure_message='Kernel panic - not syncing',
                                  vm=vm)
 
+    # This tests the whole boot chain from EFI to Userspace
+    # We only boot a whole OS for the current top level CPU and GIC
+    # Other test profiles should use more minimal boots
+    def test_alpine_virt_tcg_gic_max(self):
+        """
+        :avocado: tags=arch:aarch64
+        :avocado: tags=machine:virt
+        :avocado: tags=accel:tcg
+        """
+        iso_url = ('https://dl-cdn.alpinelinux.org/'
+                   'alpine/v3.16/releases/aarch64/'
+                   'alpine-virt-3.16.3-aarch64.iso')
+
+        # Alpine use sha256 so I recalculated this myself
+        iso_sha1 = '0683bc089486d55c91bf6607d5ecb93925769bc0'
+        iso_path = self.fetch_asset(iso_url, asset_hash=iso_sha1)
+
+        self.vm.set_console()
+        kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
+                               'console=ttyAMA0')
+        self.require_accelerator("tcg")
+
+        self.vm.add_args("-accel", "tcg")
+        self.vm.add_args("-cpu", "max,pauth-impdef=on")
+        self.vm.add_args("-machine",
+                         "virt,acpi=on,"
+                         "virtualization=on,"
+                         "mte=on,"
+                         "gic-version=max,iommu=smmuv3")
+        self.vm.add_args("-smp", "2", "-m", "1024")
+        self.vm.add_args('-bios', os.path.join(BUILD_DIR, 'pc-bios',
+                                               'edk2-aarch64-code.fd'))
+        self.vm.add_args("-drive", f"file={iso_path},format=raw")
+        self.vm.add_args('-device', 'virtio-rng-pci,rng=rng0')
+        self.vm.add_args('-object', 'rng-random,id=rng0,filename=/dev/urandom')
+
+        self.vm.launch()
+        self.wait_for_console_pattern('Welcome to Alpine Linux 3.16')
+
+
     def test_aarch64_virt(self):
         """
         :avocado: tags=arch:aarch64
-- 
2.34.1

Re: [PATCH v3 09/13] tests/avocado: introduce alpine virt test for CI

[Philippe Mathieu-Daudé]

On 17/11/22 18:25, Alex Bennée wrote:
> The boot_linux tests download and run a full cloud image boot and
> start a full distro. While the ability to test the full boot chain is
> worthwhile it is perhaps a little too heavy weight and causes issues
> in CI. Fix this by introducing a new alpine linux ISO boot in
> machine_aarch64_virt.
> 
> This boots a fully loaded -cpu max with all the bells and whistles in
> 31s on my machine. A full debug build takes around 180s on my machine
> so we set a more generous timeout to cover that.
> 
> We don't add a test for lesser GIC versions although there is some
> coverage for that already in the boot_xen.py tests. If we want to
> introduce more comprehensive testing we can do it with a custom kernel
> and initrd rather than a full distro boot.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v1
>    - use "virt" image instead (even faster)
>    - don't drop boot_linux (it is now disabled for CI)
>    - re-phrase commit message
>    - add alpine to the test name
> ---
>   tests/avocado/machine_aarch64_virt.py | 46 ++++++++++++++++++++++++++-
>   1 file changed, 45 insertions(+), 1 deletion(-)


> +    # This tests the whole boot chain from EFI to Userspace
> +    # We only boot a whole OS for the current top level CPU and GIC
> +    # Other test profiles should use more minimal boots
> +    def test_alpine_virt_tcg_gic_max(self):
> +        """
> +        :avocado: tags=arch:aarch64
> +        :avocado: tags=machine:virt
> +        :avocado: tags=accel:tcg
> +        """
> +        iso_url = ('https://dl-cdn.alpinelinux.org/'
> +                   'alpine/v3.16/releases/aarch64/'
> +                   'alpine-virt-3.16.3-aarch64.iso')
> +
> +        # Alpine use sha256 so I recalculated this myself
> +        iso_sha1 = '0683bc089486d55c91bf6607d5ecb93925769bc0'
> +        iso_path = self.fetch_asset(iso_url, asset_hash=iso_sha1)
> +
> +        self.vm.set_console()
> +        kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
> +                               'console=ttyAMA0')
> +        self.require_accelerator("tcg")
> +
> +        self.vm.add_args("-accel", "tcg")
> +        self.vm.add_args("-cpu", "max,pauth-impdef=on")
> +        self.vm.add_args("-machine",
> +                         "virt,acpi=on,"
> +                         "virtualization=on,"
> +                         "mte=on,"
> +                         "gic-version=max,iommu=smmuv3")
> +        self.vm.add_args("-smp", "2", "-m", "1024")
> +        self.vm.add_args('-bios', os.path.join(BUILD_DIR, 'pc-bios',
> +                                               'edk2-aarch64-code.fd'))

I am not sure about restricting to BUILD_DIR, I'd rather use an
externally prebuilt image, i.e.:
https://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/4710/QEMU-AARCH64/RELEASE_GCC5/

(I'd like to use these Avocado tests with a distrib provided QEMU
binary).

Anyhow can be fixed later, so:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v3 10/13] tests/avocado: skip aarch64 cloud TCG tests in CI

[Alex Bennée]

We now have a much lighter weight test in machine_aarch64_virt which
tests the full boot chain in less time. Rename the tests while we are
at it to make it clear it is a Fedora cloud image.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/avocado/boot_linux.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/avocado/boot_linux.py b/tests/avocado/boot_linux.py
index 2be4be395d..79810be942 100644
--- a/tests/avocado/boot_linux.py
+++ b/tests/avocado/boot_linux.py
@@ -58,6 +58,9 @@ def test_pc_q35_kvm(self):
         self.launch_and_wait(set_up_ssh_connection=False)
 
 
+# For Aarch64 we only boot KVM tests in CI as the TCG tests are very
+# heavyweight. There are lighter weight distros which we use in the
+# machine_aarch64_virt.py tests.
 class BootLinuxAarch64(LinuxTest):
     """
     :avocado: tags=arch:aarch64
@@ -73,7 +76,8 @@ def add_common_args(self):
         self.vm.add_args('-device', 'virtio-rng-pci,rng=rng0')
         self.vm.add_args('-object', 'rng-random,id=rng0,filename=/dev/urandom')
 
-    def test_virt_tcg_gicv2(self):
+    @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
+    def test_fedora_cloud_tcg_gicv2(self):
         """
         :avocado: tags=accel:tcg
         :avocado: tags=cpu:max
@@ -86,7 +90,8 @@ def test_virt_tcg_gicv2(self):
         self.add_common_args()
         self.launch_and_wait(set_up_ssh_connection=False)
 
-    def test_virt_tcg_gicv3(self):
+    @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
+    def test_fedora_cloud_tcg_gicv3(self):
         """
         :avocado: tags=accel:tcg
         :avocado: tags=cpu:max
-- 
2.34.1

Re: [PATCH v3 10/13] tests/avocado: skip aarch64 cloud TCG tests in CI

[Philippe Mathieu-Daudé]

On 17/11/22 18:25, Alex Bennée wrote:
> We now have a much lighter weight test in machine_aarch64_virt which
> tests the full boot chain in less time. Rename the tests while we are
> at it to make it clear it is a Fedora cloud image.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/avocado/boot_linux.py | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)

Thanks for the update.

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

[PATCH v3 11/13] gitlab: integrate coverage report

[Alex Bennée]

This should hopefully give is nice coverage information about what our
tests (or at least the subset we are running) have hit. Ideally we
would want a way to trigger coverage on tests likely to be affected by
the current commit.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20221111145529.4020801-12-alex.bennee@linaro.org>
---
 .gitlab-ci.d/buildtest.yml | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index 7173749c52..d21b4a1fd4 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -494,7 +494,17 @@ check-gprof-gcov:
     IMAGE: ubuntu2004
     MAKE_CHECK_ARGS: check
   after_script:
-    - ${CI_PROJECT_DIR}/scripts/ci/coverage-summary.sh
+    - cd build
+    - gcovr --xml-pretty --exclude-unreachable-branches --print-summary
+        -o coverage.xml --root ${CI_PROJECT_DIR} . *.p
+  coverage: /^\s*lines:\s*\d+.\d+\%/
+  artifacts:
+    name: ${CI_JOB_NAME}-${CI_COMMIT_REF_NAME}-${CI_COMMIT_SHA}
+    expire_in: 2 days
+    reports:
+      coverage_report:
+        coverage_format: cobertura
+        path: build/coverage.xml
 
 build-oss-fuzz:
   extends: .native_build_job_template
-- 
2.34.1

[PATCH v3 12/13] tests/avocado/boot_linux.py: Bump aarch64 virt test timeout to 720s

[Alex Bennée]

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

The two tests
tests/avocado/boot_linux.py:BootLinuxAarch64.test_virt_tcg_gicv2
tests/avocado/boot_linux.py:BootLinuxAarch64.test_virt_tcg_gicv3

take quite a long time to run, and the current timeout of 240s
is not enough for the tests to complete on slow machines:
we've seen these tests time out in the gitlab CI in the
'avocado-system-alpine' CI job, for instance. The timeout
is also insufficient for running the test with a debug build
of QEMU: on my machine the tests take over 10 minutes to run
in that config.

Push the timeout up to 720s so that the test definitely has
enough time to complete.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20221117111628.911686-1-peter.maydell@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/avocado/boot_linux.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/avocado/boot_linux.py b/tests/avocado/boot_linux.py
index 79810be942..b3e58fa309 100644
--- a/tests/avocado/boot_linux.py
+++ b/tests/avocado/boot_linux.py
@@ -67,7 +67,7 @@ class BootLinuxAarch64(LinuxTest):
     :avocado: tags=machine:virt
     :avocado: tags=machine:gic-version=2
     """
-    timeout = 240
+    timeout = 720
 
     def add_common_args(self):
         self.vm.add_args('-bios',
-- 
2.34.1

Re: [PATCH v3 12/13] tests/avocado/boot_linux.py: Bump aarch64 virt test timeout to 720s

[Philippe Mathieu-Daudé]

On 17/11/22 18:25, Alex Bennée wrote:
> From: Peter Maydell <peter.maydell@linaro.org>
> 
> The two tests
> tests/avocado/boot_linux.py:BootLinuxAarch64.test_virt_tcg_gicv2
> tests/avocado/boot_linux.py:BootLinuxAarch64.test_virt_tcg_gicv3
> 
> take quite a long time to run, and the current timeout of 240s
> is not enough for the tests to complete on slow machines:
> we've seen these tests time out in the gitlab CI in the
> 'avocado-system-alpine' CI job, for instance. The timeout

The previous patches removed these jobs from GitLab CI, so
this shouldn't be a problem there anymore, but the next part
is still relevant:

> is also insufficient for running the test with a debug build
> of QEMU: on my machine the tests take over 10 minutes to run
> in that config.

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

> Push the timeout up to 720s so that the test definitely has
> enough time to complete.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <20221117111628.911686-1-peter.maydell@linaro.org>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/avocado/boot_linux.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Re: [PATCH v3 12/13] tests/avocado/boot_linux.py: Bump aarch64 virt test timeout to 720s

[Peter Maydell]

On Thu, 17 Nov 2022 at 17:25, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> From: Peter Maydell <peter.maydell@linaro.org>
>
> The two tests
> tests/avocado/boot_linux.py:BootLinuxAarch64.test_virt_tcg_gicv2
> tests/avocado/boot_linux.py:BootLinuxAarch64.test_virt_tcg_gicv3
>
> take quite a long time to run, and the current timeout of 240s
> is not enough for the tests to complete on slow machines:
> we've seen these tests time out in the gitlab CI in the
> 'avocado-system-alpine' CI job, for instance. The timeout
> is also insufficient for running the test with a debug build
> of QEMU: on my machine the tests take over 10 minutes to run
> in that config.
>
> Push the timeout up to 720s so that the test definitely has
> enough time to complete.

Just to let you know, this patch is now upstream via target-arm.next,
so you can drop it from your queue.

thanks
-- PMM

[PATCH v3 13/13] ci: replace x86_64 macos-11 with aarch64 macos-12

[Alex Bennée]

From: Daniel P. Berrangé <berrange@redhat.com>

The Cirrus CI service has announced the intent to discontinue
support for x86_64 macOS CI runners. They already have aarch64
runners available and require all projects to switch to these
images before Jan 1st 2023. The different architecture is
merely determined by the image name requested.

For aarch64 they only support macOS 12 onwards. At the same
time our support policy only guarantees the most recent 2
major versions, so macOS 12 is already technically our min
version.

https://cirrus-ci.org/blog/2022/11/08/sunsetting-intel-macos-instances/

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20221116175023.80627-1-berrange@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 .gitlab-ci.d/cirrus.yml                              | 12 ++++++------
 .gitlab-ci.d/cirrus/{macos-11.vars => macos-12.vars} | 12 ++++++------
 tests/lcitool/libvirt-ci                             |  2 +-
 tests/lcitool/refresh                                |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)
 rename .gitlab-ci.d/cirrus/{macos-11.vars => macos-12.vars} (74%)

diff --git a/.gitlab-ci.d/cirrus.yml b/.gitlab-ci.d/cirrus.yml
index d70da61248..634a73a742 100644
--- a/.gitlab-ci.d/cirrus.yml
+++ b/.gitlab-ci.d/cirrus.yml
@@ -70,19 +70,19 @@ x64-freebsd-13-build:
     INSTALL_COMMAND: pkg install -y
     TEST_TARGETS: check
 
-x64-macos-11-base-build:
+aarch64-macos-12-base-build:
   extends: .cirrus_build_job
   variables:
-    NAME: macos-11
-    CIRRUS_VM_INSTANCE_TYPE: osx_instance
+    NAME: macos-12
+    CIRRUS_VM_INSTANCE_TYPE: macos_instance
     CIRRUS_VM_IMAGE_SELECTOR: image
-    CIRRUS_VM_IMAGE_NAME: big-sur-base
+    CIRRUS_VM_IMAGE_NAME: ghcr.io/cirruslabs/macos-monterey-base:latest
     CIRRUS_VM_CPUS: 12
     CIRRUS_VM_RAM: 24G
     UPDATE_COMMAND: brew update
     INSTALL_COMMAND: brew install
-    PATH_EXTRA: /usr/local/opt/ccache/libexec:/usr/local/opt/gettext/bin
-    PKG_CONFIG_PATH: /usr/local/opt/curl/lib/pkgconfig:/usr/local/opt/ncurses/lib/pkgconfig:/usr/local/opt/readline/lib/pkgconfig
+    PATH_EXTRA: /opt/homebrew/ccache/libexec:/opt/homebrew/gettext/bin
+    PKG_CONFIG_PATH: /opt/homebrew/curl/lib/pkgconfig:/opt/homebrew/ncurses/lib/pkgconfig:/opt/homebrew/readline/lib/pkgconfig
     TEST_TARGETS: check-unit check-block check-qapi-schema check-softfloat check-qtest-x86_64
 
 
diff --git a/.gitlab-ci.d/cirrus/macos-11.vars b/.gitlab-ci.d/cirrus/macos-12.vars
similarity index 74%
rename from .gitlab-ci.d/cirrus/macos-11.vars
rename to .gitlab-ci.d/cirrus/macos-12.vars
index aee9f50de6..ef9e14b373 100644
--- a/.gitlab-ci.d/cirrus/macos-11.vars
+++ b/.gitlab-ci.d/cirrus/macos-12.vars
@@ -1,16 +1,16 @@
 # THIS FILE WAS AUTO-GENERATED
 #
-#  $ lcitool variables macos-11 qemu
+#  $ lcitool variables macos-12 qemu
 #
 # https://gitlab.com/libvirt/libvirt-ci
 
-CCACHE='/usr/local/bin/ccache'
+CCACHE='/opt/homebrew/bin/ccache'
 CPAN_PKGS=''
 CROSS_PKGS=''
-MAKE='/usr/local/bin/gmake'
-NINJA='/usr/local/bin/ninja'
+MAKE='/opt/homebrew/bin/gmake'
+NINJA='/opt/homebrew/bin/ninja'
 PACKAGING_COMMAND='brew'
-PIP3='/usr/local/bin/pip3'
+PIP3='/opt/homebrew/bin/pip3'
 PKGS='bash bc bison bzip2 capstone ccache cmocka ctags curl dbus diffutils dtc flex gcovr gettext git glib gnu-sed gnutls gtk+3 jemalloc jpeg-turbo json-c libepoxy libffi libgcrypt libiscsi libnfs libpng libslirp libssh libtasn1 libusb llvm lzo make meson ncurses nettle ninja perl pixman pkg-config python3 rpm2cpio sdl2 sdl2_image snappy sparse spice-protocol tesseract texinfo usbredir vde vte3 zlib zstd'
 PYPI_PKGS='PyYAML numpy pillow sphinx sphinx-rtd-theme'
-PYTHON='/usr/local/bin/python3'
+PYTHON='/opt/homebrew/bin/python3'
diff --git a/tests/lcitool/libvirt-ci b/tests/lcitool/libvirt-ci
index d40e203631..e3eb28cf2e 160000
--- a/tests/lcitool/libvirt-ci
+++ b/tests/lcitool/libvirt-ci
@@ -1 +1 @@
-Subproject commit d40e203631eb3eacee17e8cf8fd20aa5152db62a
+Subproject commit e3eb28cf2e17fbcf7fe7e19505ee432b8ec5bbb5
diff --git a/tests/lcitool/refresh b/tests/lcitool/refresh
index ce0b24c0b1..fa966e4009 100755
--- a/tests/lcitool/refresh
+++ b/tests/lcitool/refresh
@@ -176,7 +176,7 @@ try:
     #
     generate_cirrus("freebsd-12")
     generate_cirrus("freebsd-13")
-    generate_cirrus("macos-11")
+    generate_cirrus("macos-12")
 
     sys.exit(0)
 except Exception as ex:
-- 
2.34.1