[PATCH for 7.2-rc1 v2 00/12] testing, docs, plugins, arm pre-PR

[PATCH for 7.2-rc1 v2 00/12] testing, docs, plugins, arm pre-PR

[Alex Bennée]

Hi,

This is my pre-PR series for the pull request I'm going to send on
Monday in time for Tuesday tagging of rc1. Anything not reviewed will
get dropped from the PR (which probably includes the GICD_IIDR which
was just an annoyance I noticed while debugging Xen for another
series). The following still need review:

 - hw/intc: add implementation of GICD_IIDR to Arm GIC
 - gitlab: integrate coverage report (1 acks, 1 sobs)
 - tests/plugins: add a new vcpu state tracking plugin
 - tests/docker: allow user to override check target

Alex Bennée (9):
  tests/avocado: improve behaviour waiting for login prompts
  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/plugins: add a new vcpu state tracking plugin
  gitlab: integrate coverage report
  hw/intc: add implementation of GICD_IIDR to Arm GIC

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

Peter Maydell (1):
  tests/avocado: Raise timeout for
    boot_linux.py:BootLinuxPPC64.test_pseries_tcg

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 +-
 hw/intc/arm_gic.c                        |  44 ++++---
 tests/plugin/vcpu.c                      | 153 +++++++++++++++++++++++
 .gitlab-ci.d/buildtest.yml               |  12 +-
 tests/avocado/avocado_qemu/__init__.py   |  89 ++++++++++++-
 tests/avocado/boot_linux.py              |   2 +-
 tests/avocado/machine_aspeed.py          |  17 ++-
 tests/docker/Makefile.include            |   2 +
 tests/docker/common.rc                   |   6 +-
 tests/plugin/meson.build                 |   2 +-
 15 files changed, 480 insertions(+), 71 deletions(-)
 create mode 100644 docs/devel/maintainers.rst
 create mode 100644 tests/plugin/vcpu.c

-- 
2.34.1

[PATCH v2 01/12] 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>
---
 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 v2 02/12] tests/avocado: improve behaviour waiting for login prompts

[Alex Bennée]

This attempts to deal with the problem of login prompts not being
guaranteed to be terminated with a newline. The solution to this is to
peek at the incoming data looking to see if we see an up-coming match
before we fall back to the old readline() logic. The reason to mostly
rely on readline is because I am occasionally seeing the peek stalling
despite data being there.

This seems kinda hacky and gross so I'm open to alternative approaches
and cleaner python code.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

---
v2
  - remove superfluous /r/n
---
 tests/avocado/avocado_qemu/__init__.py | 89 +++++++++++++++++++++++++-
 1 file changed, 88 insertions(+), 1 deletion(-)

diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
index 910f3ba1ea..20cba57161 100644
--- a/tests/avocado/avocado_qemu/__init__.py
+++ b/tests/avocado/avocado_qemu/__init__.py
@@ -131,6 +131,58 @@ def pick_default_qemu_bin(bin_prefix='qemu-system-', arch=None):
             return path
     return None
 
+def _peek_ahead(console, min_match, success_message):
+    """
+    peek ahead in the console stream keeping an eye out for the
+    success message.
+
+    Returns some message to process or None, indicating the normal
+    readline should occur.
+    """
+    console_logger = logging.getLogger('console')
+    peek_len = 0
+    retries = 0
+
+    while True:
+        try:
+            old_peek_len = peek_len
+            peek_ahead = console.peek(min_match).decode()
+            peek_len = len(peek_ahead)
+
+            # if we get stuck too long lets just fallback to readline
+            if peek_len <= old_peek_len:
+                retries += 1
+                if retries > 10:
+                    return None
+
+            # if we see a newline in the peek we can let safely bail
+            # and let the normal readline() deal with it
+            if peek_ahead.endswith(('\n', '\r')):
+                return None
+
+            # if we haven't seen enough for the whole message but the
+            # first part matches lets just loop again
+            if len(peek_ahead) < min_match and \
+               success_message[:peek_len] in peek_ahead:
+                continue
+
+            # if we see the whole success_message we are done, consume
+            # it and pass back so we can exit to the user
+            if success_message in peek_ahead:
+                return console.read(peek_len).decode()
+
+            # of course if we've seen enough then this line probably
+            # doesn't contain what we are looking for, fallback
+            if peek_len > min_match:
+                return None
+
+        except UnicodeDecodeError:
+            console_logger.log("error in decode of peek")
+            return None
+
+    # we should never get here
+    return None
+
 
 def _console_interaction(test, success_message, failure_message,
                          send_string, keep_sending=False, vm=None):
@@ -139,17 +191,52 @@ def _console_interaction(test, success_message, failure_message,
         vm = test.vm
     console = vm.console_socket.makefile(mode='rb', encoding='utf-8')
     console_logger = logging.getLogger('console')
+
+    # Establish the minimum number of bytes we would need to
+    # potentially match against success_message
+    if success_message is not None:
+        min_match = len(success_message)
+    else:
+        min_match = 0
+
+    console_logger.debug("looking for %d:(%s), sending %s (always=%s)",
+                         min_match, success_message, send_string, keep_sending)
+
     while True:
+        msg = None
+
+        # First send our string, optionally repeating the send next
+        # cycle.
         if send_string:
             vm.console_socket.sendall(send_string.encode())
             if not keep_sending:
                 send_string = None # send only once
+
+        # If the console has no data to read we briefly
+        # sleep before continuing.
+        if not console.readable():
+            time.sleep(0.1)
+            continue
+
         try:
-            msg = console.readline().decode().strip()
+
+            # First we shall peek ahead for a potential match to cover waiting
+            # for lines without any newlines.
+            if min_match > 0:
+                msg = _peek_ahead(console, min_match, success_message)
+
+            # otherwise we block here for a full line
+            if not msg:
+                msg = console.readline().decode().strip()
+
         except UnicodeDecodeError:
+            console_logger.debug("skipped unicode error")
             msg = None
+
+        # if nothing came out we continue and try again
         if not msg:
             continue
+
         console_logger.debug(msg)
         if success_message is None or success_message in msg:
             break
-- 
2.34.1

[PATCH v2 03/12] 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>
---
 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 v2 04/12] 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>

---
v1
 - fix s/target /target./
 - CHECK_TARGET -> TEST_COMMAND
---
 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 v2 05/12] 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: <20221012121152.1179051-2-alex.bennee@linaro.org>

---
v2
  - s/roll/role
  - s/projects/project's
  - mention looking after the long term health of area
  - add a section on becoming a reviewer
  - expand becoming section
  - add footnote about key signing
---
 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

[PATCH v2 06/12] 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: <20221012121152.1179051-3-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

[PATCH v2 07/12] 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: <20221012121152.1179051-4-alex.bennee@linaro.org>

---
v2
  - emphasise a Real Name should be used
  - s/therefor/therefore/
---
 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

[PATCH v2 08/12] 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: <20221012121152.1179051-5-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

[PATCH v2 09/12] tests/plugins: add a new vcpu state tracking plugin

[Alex Bennée]

Although we call qemu_plugin_register_vcpu_idle_cb() in the bb test we
don't really exercise the rest of the state change callbacks. Add a
new test that tests the whole API.

[AJB: I wrote this in an attempt to flush out a reproducer for #1195
although so far no joy.]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Idan Horowitz <idan.horowitz@gmail.com>

----
v2
  - and min max tracking
  - fix { style on get_timestamp
---
 tests/plugin/vcpu.c      | 153 +++++++++++++++++++++++++++++++++++++++
 tests/plugin/meson.build |   2 +-
 2 files changed, 154 insertions(+), 1 deletion(-)
 create mode 100644 tests/plugin/vcpu.c

diff --git a/tests/plugin/vcpu.c b/tests/plugin/vcpu.c
new file mode 100644
index 0000000000..f4fa518420
--- /dev/null
+++ b/tests/plugin/vcpu.c
@@ -0,0 +1,153 @@
+/*
+ * Test plugin for exercising the vcpu event callbacks. These exist
+ * for when vcpus are created and destroyed (especially in linux-user
+ * where vcpu ~= thread) and when they pause and restart (generally
+ * for wfi and the like in system emulation).
+ *
+ * Copyright (c) 2022 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <inttypes.h>
+#include <assert.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <glib.h>
+
+#include <qemu-plugin.h>
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+typedef struct {
+    uint64_t start_time_ns;
+    uint64_t idle_count;
+    uint64_t last_idle_ts;
+    uint64_t min_idle_ns;
+    uint64_t max_idle_ns;
+    uint64_t total_idle_ns;
+    uint64_t exit_time_ns;
+} VCPUData;
+
+static GMutex expand_counts_lock;
+static GArray *counts; /* array of VCPUData */
+static bool sys_emu;
+
+/*
+ * Fetch VCPU data for a given index, allocate if required.
+ */
+static VCPUData *get_vcpu_data(int cpu_index)
+{
+    if (cpu_index >= counts->len) {
+        g_mutex_lock(&expand_counts_lock);
+        counts = g_array_set_size(counts, cpu_index + 1);
+        g_mutex_unlock(&expand_counts_lock);
+    }
+    /* race if set size re-allocs? */
+    return &g_array_index(counts, VCPUData, cpu_index);
+}
+
+static uint64_t get_timestamp(void)
+{
+    struct timespec ts;
+    clock_gettime(CLOCK_MONOTONIC, &ts);
+    return ts.tv_sec * 1000000000LL + ts.tv_nsec;
+}
+
+static void vcpu_init(qemu_plugin_id_t id, unsigned int cpu_index)
+{
+    VCPUData *d = get_vcpu_data(cpu_index);
+    d->start_time_ns = get_timestamp();
+    d->min_idle_ns = UINT64_MAX;
+}
+
+static void vcpu_idle(qemu_plugin_id_t id, unsigned int cpu_index)
+{
+    VCPUData *d = get_vcpu_data(cpu_index);
+    d->last_idle_ts = get_timestamp();
+    d->idle_count++;
+}
+
+static void vcpu_resume(qemu_plugin_id_t id, unsigned int cpu_index)
+{
+    VCPUData *d = get_vcpu_data(cpu_index);
+    uint64_t now = get_timestamp();
+    uint64_t delta = now - d->last_idle_ts;
+    d->total_idle_ns += delta;
+    if (delta > d->max_idle_ns) {
+        d->max_idle_ns = delta;
+    } else if (delta < d->min_idle_ns) {
+        d->min_idle_ns = delta;
+    }
+}
+
+static void vcpu_exit(qemu_plugin_id_t id, unsigned int cpu_index)
+{
+    VCPUData *d = get_vcpu_data(cpu_index);
+    d->exit_time_ns = get_timestamp();
+}
+
+/*
+ * Report our final stats
+ */
+static void plugin_exit(qemu_plugin_id_t id, void *p)
+{
+    g_autoptr(GString) report = g_string_new("");
+    const char *vcpu_or_thread = sys_emu ? "vcpu" : "thread";
+    int i;
+
+    g_string_printf(report, "Exit: we had a total of %d %ss\n",
+                    counts->len, vcpu_or_thread);
+
+    for (i = 0; i < counts->len; i++) {
+        VCPUData *d = &g_array_index(counts, VCPUData, i);
+
+        /* FIXME: we never see vcpu_exit for the main thread */
+        if (!d->exit_time_ns) {
+            d->exit_time_ns = get_timestamp();
+        }
+
+        g_string_append_printf(report, "%s %d: %"PRId64" µs lifetime",
+                               vcpu_or_thread, i,
+                               (d->exit_time_ns - d->start_time_ns) / 1000);
+        if (d->idle_count) {
+            uint64_t idle_us = d->total_idle_ns / 1000;
+            uint64_t idle_avg = d->total_idle_ns / d->idle_count;
+            g_string_append_printf(report, ", %"PRId64" idles, %"
+                                   PRId64 " µs total idle time, %"
+                                   PRId64 " ns min, %"
+                                   PRId64 " ns max, %"
+                                   PRId64 " ns per idle",
+                                   d->idle_count, idle_us,
+                                   d->min_idle_ns, d->max_idle_ns, idle_avg);
+        }
+        g_string_append_printf(report, "\n");
+    }
+    qemu_plugin_outs(report->str);
+}
+
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
+                                           const qemu_info_t *info,
+                                           int argc, char **argv)
+{
+    int entries = 2;
+
+    if (info->system_emulation) {
+        entries = info->system.max_vcpus;
+        sys_emu = true;
+    }
+
+    counts = g_array_sized_new(true, true, sizeof(VCPUData), entries);
+    g_mutex_init(&expand_counts_lock);
+
+    qemu_plugin_register_vcpu_init_cb(id, vcpu_init);
+    qemu_plugin_register_vcpu_idle_cb(id, vcpu_idle);
+    qemu_plugin_register_vcpu_resume_cb(id, vcpu_resume);
+    qemu_plugin_register_vcpu_exit_cb(id, vcpu_exit);
+
+    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
+    return 0;
+}
diff --git a/tests/plugin/meson.build b/tests/plugin/meson.build
index 2bbfc4b19e..8c6b232183 100644
--- a/tests/plugin/meson.build
+++ b/tests/plugin/meson.build
@@ -1,5 +1,5 @@
 t = []
-foreach i : ['bb', 'empty', 'insn', 'mem', 'syscall']
+foreach i : ['bb', 'empty', 'insn', 'mem', 'syscall', 'vcpu']
   t += shared_module(i, files(i + '.c'),
                      include_directories: '../../include/qemu',
                      dependencies: glib)
-- 
2.34.1

[PATCH v2 10/12] 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>
---
 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 v2 11/12] 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>
---
 .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 v2 12/12] hw/intc: add implementation of GICD_IIDR to Arm GIC

[Alex Bennée]

a66a24585f (hw/intc/arm_gic: Implement read of GICC_IIDR) implemented
this for the CPU interface register. The fact we don't implement it
shows up when running Xen with -d guest_error which is definitely
wrong because the guest is perfectly entitled to read it.

Lightly re-factor this region of registers and also add a comment to
the function in case anyway was under the illusion we only return
bytes from a function called readb.

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

---
v2
  - checkpatch fixes.
---
 hw/intc/arm_gic.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 492b2421ab..65b1ef7151 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -941,6 +941,10 @@ static void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
     gic_update(s);
 }
 
+/*
+ * Although this is named a byte read we don't always return bytes and
+ * rely on the calling function oring bits together.
+ */
 static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
 {
     GICState *s = (GICState *)opaque;
@@ -954,23 +958,35 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
     cpu = gic_get_current_cpu(s);
     cm = 1 << cpu;
     if (offset < 0x100) {
-        if (offset == 0) {      /* GICD_CTLR */
-            if (s->security_extn && !attrs.secure) {
-                /* The NS bank of this register is just an alias of the
-                 * EnableGrp1 bit in the S bank version.
-                 */
-                return extract32(s->ctlr, 1, 1);
-            } else {
-                return s->ctlr;
+        if (offset < 0xc) {
+            switch (offset) {
+            case 0: /* GICD_CTLR[7:0] */
+            {
+                if (s->security_extn && !attrs.secure) {
+                    /*
+                     * The NS bank of this register is just an alias of the
+                     * EnableGrp1 bit in the S bank version.
+                     */
+                    return extract32(s->ctlr, 1, 1);
+                } else {
+                    return s->ctlr;
+                }
             }
-        }
-        if (offset == 4)
-            /* Interrupt Controller Type Register */
-            return ((s->num_irq / 32) - 1)
+            case 4: /* GIC_TYPER - Interrupt Controller Type Register */
+            {
+                return ((s->num_irq / 32) - 1)
                     | ((s->num_cpu - 1) << 5)
                     | (s->security_extn << 10);
-        if (offset < 0x08)
-            return 0;
+            }
+            case 8: /* GICD_IIDR - Implementer ID Register */
+            {
+                return 0x43b; /* Arm JEP106 identity */
+            }
+            default:
+                /* return 0 for high bits of above */
+                return 0;
+            }
+        }
         if (offset >= 0x80) {
             /* Interrupt Group Registers: these RAZ/WI if this is an NS
              * access to a GIC with the security extensions, or if the GIC
-- 
2.34.1

Re: [PATCH v2 01/12] Run docker probe only if docker or podman are available

[Thomas Huth]

On 11/11/2022 15.55, Alex Bennée wrote:
> 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>
> ---
>   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 ;;

Maybe the probe should better be done in the docker.py script instead? ... 
but doing it here likely does not hurt either, so:

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

Re: [PATCH v2 04/12] tests/docker: allow user to override check target

[Philippe Mathieu-Daudé]

On 11/11/22 15:55, Alex Bennée wrote:
> 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>
> 
> ---
> v1
>   - fix s/target /target./
>   - CHECK_TARGET -> TEST_COMMAND
> ---
>   tests/docker/Makefile.include | 2 ++
>   tests/docker/common.rc        | 6 +++---
>   2 files changed, 5 insertions(+), 3 deletions(-)

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

Re: [PATCH v2 11/12] gitlab: integrate coverage report

[Philippe Mathieu-Daudé]

On 11/11/22 15:55, Alex Bennée wrote:
> 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.

IIUC per [*] this will not appear in the pipeline but in 
https://gitlab.com/qemu-project/qemu/-/graphs/master/charts under 'Code 
coverage statistics', right?

If so, can you document this in this description? Also maybe this can
be linked in some docs/devel/ci*rst file.

[*] 
https://docs.gitlab.com/ee/ci/pipelines/settings.html#view-code-coverage-history

> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   .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

Hmm do we need the 'name' and 'expire_in' keys? That would be to keep 
the coverage.xml file?

> +    reports:
> +      coverage_report:
> +        coverage_format: cobertura
> +        path: build/coverage.xml
>   
>   build-oss-fuzz:
>     extends: .native_build_job_template

Re: [PATCH for 7.2-rc1 v2 00/12] testing, docs, plugins, arm pre-PR

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> Hi,
>
> This is my pre-PR series for the pull request I'm going to send on
> Monday in time for Tuesday tagging of rc1. Anything not reviewed will
> get dropped from the PR (which probably includes the GICD_IIDR which
> was just an annoyance I noticed while debugging Xen for another
> series). The following still need review:
>
>  - hw/intc: add implementation of GICD_IIDR to Arm GIC
>  - gitlab: integrate coverage report (1 acks, 1 sobs)
>  - tests/plugins: add a new vcpu state tracking plugin
>  - tests/docker: allow user to override check target

Any more before I roll the PR?

-- 
Alex Bennée

Re: [PATCH v2 12/12] hw/intc: add implementation of GICD_IIDR to Arm GIC

[Peter Maydell]

On Fri, 11 Nov 2022 at 14:55, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> a66a24585f (hw/intc/arm_gic: Implement read of GICC_IIDR) implemented
> this for the CPU interface register. The fact we don't implement it
> shows up when running Xen with -d guest_error which is definitely
> wrong because the guest is perfectly entitled to read it.
>
> Lightly re-factor this region of registers and also add a comment to
> the function in case anyway was under the illusion we only return
> bytes from a function called readb.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
> v2
>   - checkpatch fixes.
> ---
>  hw/intc/arm_gic.c | 44 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 492b2421ab..65b1ef7151 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -941,6 +941,10 @@ static void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs)
>      gic_update(s);
>  }
>
> +/*
> + * Although this is named a byte read we don't always return bytes and
> + * rely on the calling function oring bits together.
> + */

Rather than documenting this, maybe it would be better to
fix the weirdness? We only do this for exactly one register,
the GICD_TYPER. Everything else is naturally byte-based.
(The GICD_CTLR looks like it is also doing this, but the
only non-zero bits are in the low byte, so it isn't really.)

The GICD_TYPER returning bigger than a byte's worth of
data I think is a bug we introduced in commit 5543d1abb6e2
when we added the security extensions support -- before that
all the bits we needed to return were in the low byte. So
I think we can fix this with just (untested):

--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -955,6 +955,7 @@ static uint32_t gic_dist_readb(void *opaque,
hwaddr offset, MemTxAttrs attrs)
     cm = 1 << cpu;
     if (offset < 0x100) {
         if (offset == 0) {      /* GICD_CTLR */
+            /* We rely here on the only non-zero bits being in byte 0 */
             if (s->security_extn && !attrs.secure) {
                 /* The NS bank of this register is just an alias of the
                  * EnableGrp1 bit in the S bank version.
@@ -964,11 +965,14 @@ static uint32_t gic_dist_readb(void *opaque,
hwaddr offset, MemTxAttrs attrs)
                 return s->ctlr;
             }
         }
-        if (offset == 4)
-            /* Interrupt Controller Type Register */
-            return ((s->num_irq / 32) - 1)
-                    | ((s->num_cpu - 1) << 5)
-                    | (s->security_extn << 10);
+        if (offset == 4) {
+            /* GICD_TYPER byte 0 */
+            return ((s->num_irq / 32) - 1) | ((s->num_cpu - 1) << 5);
+        }
+        if (offset == 5) {
+            /* GICD_TYPER byte 1 */
+            return (s->security_extn << 2);
+        }
         if (offset < 0x08)
             return 0;
         if (offset >= 0x80) {


(you can add my Signed-off-by: if you want to turn that into a proper patch.)

thanks
-- PMM

Re: [PATCH v2 02/12] tests/avocado: improve behaviour waiting for login prompts

[Peter Maydell]

On Fri, 11 Nov 2022 at 14:58, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> This attempts to deal with the problem of login prompts not being
> guaranteed to be terminated with a newline. The solution to this is to
> peek at the incoming data looking to see if we see an up-coming match
> before we fall back to the old readline() logic. The reason to mostly
> rely on readline is because I am occasionally seeing the peek stalling
> despite data being there.
>
> This seems kinda hacky and gross so I'm open to alternative approaches
> and cleaner python code.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

With this patch, the evb_sdk test fails:

Fetching asset from
./build/arm-clang/tests/avocado/machine_aspeed.py:AST2x00MachineSDK.test_arm_ast2500_evb_sdk
JOB ID     : 542e050c4f7e1ddd6d5cdd350e4c26e1bdfcdee4
JOB LOG    : /home/petmay01/avocado/job-results/job-2022-11-14T16.21-542e050/job.log
 (1/1) ./build/arm-clang/tests/avocado/machine_aspeed.py:AST2x00MachineSDK.test_arm_ast2500_evb_sdk:
ERROR: log() missing 1 required positional argument: 'msg' (82.57 s)
RESULTS    : PASS 0 | ERROR 1 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0
| CANCEL 0
JOB TIME   : 84.09 s

The avocado log reports a traceback where Python has thrown a
UnicodeDecodeError, and then subsequently an attempted debug
message in the error-handling path has a syntax error
("log() missing 1 required positional argument"):

2022-11-14 16:22:47,952 __init__         L0240 DEBUG| Stopping Host
logger for ttyVUART0...
2022-11-14 16:22:48,240 __init__         L0240 DEBUG| Starting Wait
for /xyz/ope…control/host0/boot/one_time...
2022-11-14 16:22:48,570 stacktrace       L0039 ERROR|
2022-11-14 16:22:48,570 stacktrace       L0041 ERROR| Reproduced
traceback from: /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm
-clang/tests/venv/lib/python3.8/site-packages/avocado/core/test.py:770
2022-11-14 16:22:48,572 stacktrace       L0045 ERROR| Traceback (most
recent call last):
2022-11-14 16:22:48,572 stacktrace       L0045 ERROR|   File
"/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/avocad
o/avocado_qemu/__init__.py", line 149, in _peek_ahead
2022-11-14 16:22:48,572 stacktrace       L0045 ERROR|     peek_ahead =
console.peek(min_match).decode()
2022-11-14 16:22:48,572 stacktrace       L0045 ERROR|
UnicodeDecodeError: 'utf-8' codec can't decode bytes in position
44-45: unexpec
ted end of data
2022-11-14 16:22:48,572 stacktrace       L0045 ERROR|
2022-11-14 16:22:48,572 stacktrace       L0045 ERROR| During handling
of the above exception, another exception occurred:
2022-11-14 16:22:48,572 stacktrace       L0045 ERROR|
2022-11-14 16:22:48,572 stacktrace       L0045 ERROR| Traceback (most
recent call last):
2022-11-14 16:22:48,572 stacktrace       L0045 ERROR|   File
"/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/venv/l
ib/python3.8/site-packages/avocado/core/decorators.py", line 90, in wrapper
2022-11-14 16:22:48,572 stacktrace       L0045 ERROR|     return
function(obj, *args, **kwargs)
2022-11-14 16:22:48,572 stacktrace       L0045 ERROR|   File
"/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/avocad
o/machine_aspeed.py", line 225, in test_arm_ast2500_evb_sdk
2022-11-14 16:22:48,572 stacktrace       L0045 ERROR|
self.wait_for_console_pattern('ast2500-default login:')
2022-11-14 16:22:48,573 stacktrace       L0045 ERROR|   File
"/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/avocad
o/machine_aspeed.py", line 193, in wait_for_console_pattern
2022-11-14 16:22:48,573 stacktrace       L0045 ERROR|
wait_for_console_pattern(self, success_message,
2022-11-14 16:22:48,573 stacktrace       L0045 ERROR|   File
"/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/avocad
o/avocado_qemu/__init__.py", line 286, in wait_for_console_pattern
2022-11-14 16:22:48,573 stacktrace       L0045 ERROR|
_console_interaction(test, success_message, failure_message, None,
vm=vm)
2022-11-14 16:22:48,573 stacktrace       L0045 ERROR|   File
"/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/avocad
o/avocado_qemu/__init__.py", line 226, in _console_interaction
2022-11-14 16:22:48,573 stacktrace       L0045 ERROR|     msg =
_peek_ahead(console, min_match, success_message)
2022-11-14 16:22:48,573 stacktrace       L0045 ERROR|   File
"/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/avocad
o/avocado_qemu/__init__.py", line 180, in _peek_ahead
2022-11-14 16:22:48,573 stacktrace       L0045 ERROR|
console_logger.log("error in decode of peek")
2022-11-14 16:22:48,573 stacktrace       L0045 ERROR| TypeError: log()
missing 1 required positional argument: 'msg'
2022-11-14 16:22:48,573 stacktrace       L0046 ERROR|
2022-11-14 16:22:48,573 test             L0775 DEBUG| Local variables:
2022-11-14 16:22:48,605 test             L0778 DEBUG|  -> obj <class
'machine_aspeed.AST2x00MachineSDK'>:
1-./build/arm-clang/tests/avocado/machine_aspeed.py:AST2x00MachineSDK.test_arm_ast2500_evb_sdk
2022-11-14 16:22:48,605 test             L0778 DEBUG|  -> args <class
'tuple'>: ()
2022-11-14 16:22:48,605 test             L0778 DEBUG|  -> kwargs
<class 'dict'>: {}
2022-11-14 16:22:48,605 test             L0778 DEBUG|  -> condition
<class 'NoneType'>: None
2022-11-14 16:22:48,605 test             L0778 DEBUG|  -> function
<class 'function'>: <function
AST2x00MachineSDK.test_arm_ast2500_evb_sdk at 0x7f05ee0678b0>
2022-11-14 16:22:48,606 test             L0778 DEBUG|  -> message
<class 'str'>: Running on GitLab
2022-11-14 16:22:48,606 test             L0778 DEBUG|  -> negate
<class 'bool'>: False

thanks
-- PMM

Re: [PATCH v2 02/12] tests/avocado: improve behaviour waiting for login prompts

[Philippe Mathieu-Daudé]

On 14/11/22 17:28, Peter Maydell wrote:
> On Fri, 11 Nov 2022 at 14:58, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> This attempts to deal with the problem of login prompts not being
>> guaranteed to be terminated with a newline. The solution to this is to
>> peek at the incoming data looking to see if we see an up-coming match
>> before we fall back to the old readline() logic. The reason to mostly
>> rely on readline is because I am occasionally seeing the peek stalling
>> despite data being there.
>>
>> This seems kinda hacky and gross so I'm open to alternative approaches
>> and cleaner python code.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> With this patch, the evb_sdk test fails:
> 
> Fetching asset from
> ./build/arm-clang/tests/avocado/machine_aspeed.py:AST2x00MachineSDK.test_arm_ast2500_evb_sdk
> JOB ID     : 542e050c4f7e1ddd6d5cdd350e4c26e1bdfcdee4
> JOB LOG    : /home/petmay01/avocado/job-results/job-2022-11-14T16.21-542e050/job.log
>   (1/1) ./build/arm-clang/tests/avocado/machine_aspeed.py:AST2x00MachineSDK.test_arm_ast2500_evb_sdk:
> ERROR: log() missing 1 required positional argument: 'msg' (82.57 s)
> RESULTS    : PASS 0 | ERROR 1 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0
> | CANCEL 0
> JOB TIME   : 84.09 s
> 
> The avocado log reports a traceback where Python has thrown a
> UnicodeDecodeError, and then subsequently an attempted debug
> message in the error-handling path has a syntax error
> ("log() missing 1 required positional argument"):

> _console_interaction(test, success_message, failure_message, None,
> vm=vm)
> 2022-11-14 16:22:48,573 stacktrace       L0045 ERROR|   File
> "/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/avocad
> o/avocado_qemu/__init__.py", line 226, in _console_interaction
> 2022-11-14 16:22:48,573 stacktrace       L0045 ERROR|     msg =
> _peek_ahead(console, min_match, success_message)
> 2022-11-14 16:22:48,573 stacktrace       L0045 ERROR|   File
> "/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/avocad
> o/avocado_qemu/__init__.py", line 180, in _peek_ahead
> 2022-11-14 16:22:48,573 stacktrace       L0045 ERROR|
> console_logger.log("error in decode of peek")
> 2022-11-14 16:22:48,573 stacktrace       L0045 ERROR| TypeError: log()
> missing 1 required positional argument: 'msg'

Indeed, log() expects a Level as first argument. Here we simply want to
report the exception as a warning and continue:

-- >8 --
          except UnicodeDecodeError:
-            console_logger.log("error in decode of peek")
+            console_logger.warning("error in decode of peek")
              return None
---

Re: [PATCH v2 02/12] tests/avocado: improve behaviour waiting for login prompts

[Cédric Le Goater]

Hello Alex,

On 11/11/22 15:55, Alex Bennée wrote:
> This attempts to deal with the problem of login prompts not being
> guaranteed to be terminated with a newline. The solution to this is to
> peek at the incoming data looking to see if we see an up-coming match
> before we fall back to the old readline() logic. The reason to mostly
> rely on readline is because I am occasionally seeing the peek stalling
> despite data being there.
> 
> This seems kinda hacky and gross so I'm open to alternative approaches
> and cleaner python code.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

I have pulled this patch in the aspeed tree hoping it would improve tests:

   AST2x00MachineSDK.test_arm_ast2500_evb_sdk
   AST2x00MachineSDK.test_arm_ast2600_evb_sdk

but the failure rate has increased :/ I have seen failures in these also :

   AST2x00Machine.test_arm_ast2500_evb_buildroot
   AST2x00Machine.test_arm_ast2600_evb_buildroot

which used to be quite stable.

Sorry, this is not of much help. the issue might be elsewhere.

Thanks for the time you spend on this.

C.


> ---
> v2
>    - remove superfluous /r/n
> ---
>   tests/avocado/avocado_qemu/__init__.py | 89 +++++++++++++++++++++++++-
>   1 file changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
> index 910f3ba1ea..20cba57161 100644
> --- a/tests/avocado/avocado_qemu/__init__.py
> +++ b/tests/avocado/avocado_qemu/__init__.py
> @@ -131,6 +131,58 @@ def pick_default_qemu_bin(bin_prefix='qemu-system-', arch=None):
>               return path
>       return None
>   
> +def _peek_ahead(console, min_match, success_message):
> +    """
> +    peek ahead in the console stream keeping an eye out for the
> +    success message.
> +
> +    Returns some message to process or None, indicating the normal
> +    readline should occur.
> +    """
> +    console_logger = logging.getLogger('console')
> +    peek_len = 0
> +    retries = 0
> +
> +    while True:
> +        try:
> +            old_peek_len = peek_len
> +            peek_ahead = console.peek(min_match).decode()
> +            peek_len = len(peek_ahead)
> +
> +            # if we get stuck too long lets just fallback to readline
> +            if peek_len <= old_peek_len:
> +                retries += 1
> +                if retries > 10:
> +                    return None
> +
> +            # if we see a newline in the peek we can let safely bail
> +            # and let the normal readline() deal with it
> +            if peek_ahead.endswith(('\n', '\r')):
> +                return None
> +
> +            # if we haven't seen enough for the whole message but the
> +            # first part matches lets just loop again
> +            if len(peek_ahead) < min_match and \
> +               success_message[:peek_len] in peek_ahead:
> +                continue
> +
> +            # if we see the whole success_message we are done, consume
> +            # it and pass back so we can exit to the user
> +            if success_message in peek_ahead:
> +                return console.read(peek_len).decode()
> +
> +            # of course if we've seen enough then this line probably
> +            # doesn't contain what we are looking for, fallback
> +            if peek_len > min_match:
> +                return None
> +
> +        except UnicodeDecodeError:
> +            console_logger.log("error in decode of peek")
> +            return None
> +
> +    # we should never get here
> +    return None
> +
>   
>   def _console_interaction(test, success_message, failure_message,
>                            send_string, keep_sending=False, vm=None):
> @@ -139,17 +191,52 @@ def _console_interaction(test, success_message, failure_message,
>           vm = test.vm
>       console = vm.console_socket.makefile(mode='rb', encoding='utf-8')
>       console_logger = logging.getLogger('console')
> +
> +    # Establish the minimum number of bytes we would need to
> +    # potentially match against success_message
> +    if success_message is not None:
> +        min_match = len(success_message)
> +    else:
> +        min_match = 0
> +
> +    console_logger.debug("looking for %d:(%s), sending %s (always=%s)",
> +                         min_match, success_message, send_string, keep_sending)
> +
>       while True:
> +        msg = None
> +
> +        # First send our string, optionally repeating the send next
> +        # cycle.
>           if send_string:
>               vm.console_socket.sendall(send_string.encode())
>               if not keep_sending:
>                   send_string = None # send only once
> +
> +        # If the console has no data to read we briefly
> +        # sleep before continuing.
> +        if not console.readable():
> +            time.sleep(0.1)
> +            continue
> +
>           try:
> -            msg = console.readline().decode().strip()
> +
> +            # First we shall peek ahead for a potential match to cover waiting
> +            # for lines without any newlines.
> +            if min_match > 0:
> +                msg = _peek_ahead(console, min_match, success_message)
> +
> +            # otherwise we block here for a full line
> +            if not msg:
> +                msg = console.readline().decode().strip()
> +
>           except UnicodeDecodeError:
> +            console_logger.debug("skipped unicode error")
>               msg = None
> +
> +        # if nothing came out we continue and try again
>           if not msg:
>               continue
> +
>           console_logger.debug(msg)
>           if success_message is None or success_message in msg:
>               break

Re: [PATCH v2 02/12] tests/avocado: improve behaviour waiting for login prompts

[Peter Maydell]

On Thu, 17 Nov 2022 at 13:39, Cédric Le Goater <clg@kaod.org> wrote:
>
> Hello Alex,
>
> On 11/11/22 15:55, Alex Bennée wrote:
> > This attempts to deal with the problem of login prompts not being
> > guaranteed to be terminated with a newline. The solution to this is to
> > peek at the incoming data looking to see if we see an up-coming match
> > before we fall back to the old readline() logic. The reason to mostly
> > rely on readline is because I am occasionally seeing the peek stalling
> > despite data being there.
> >
> > This seems kinda hacky and gross so I'm open to alternative approaches
> > and cleaner python code.
> >
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> I have pulled this patch in the aspeed tree hoping it would improve tests:
>
>    AST2x00MachineSDK.test_arm_ast2500_evb_sdk
>    AST2x00MachineSDK.test_arm_ast2600_evb_sdk
>
> but the failure rate has increased :/ I have seen failures in these also :
>
>    AST2x00Machine.test_arm_ast2500_evb_buildroot
>    AST2x00Machine.test_arm_ast2600_evb_buildroot
>
> which used to be quite stable.

This version of this patch is buggy, so you should drop it from
your tree...

-- PMM

Re: [PATCH v2 02/12] tests/avocado: improve behaviour waiting for login prompts

[Alex Bennée]

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

> Hello Alex,
>
> On 11/11/22 15:55, Alex Bennée wrote:
>> This attempts to deal with the problem of login prompts not being
>> guaranteed to be terminated with a newline. The solution to this is to
>> peek at the incoming data looking to see if we see an up-coming match
>> before we fall back to the old readline() logic. The reason to mostly
>> rely on readline is because I am occasionally seeing the peek stalling
>> despite data being there.
>> This seems kinda hacky and gross so I'm open to alternative
>> approaches
>> and cleaner python code.
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> I have pulled this patch in the aspeed tree hoping it would improve tests:
>
>   AST2x00MachineSDK.test_arm_ast2500_evb_sdk
>   AST2x00MachineSDK.test_arm_ast2600_evb_sdk
>
> but the failure rate has increased :/ I have seen failures in these also :
>
>   AST2x00Machine.test_arm_ast2500_evb_buildroot
>   AST2x00Machine.test_arm_ast2600_evb_buildroot
>
> which used to be quite stable.
>
> Sorry, this is not of much help. the issue might be elsewhere.

Do you see what is happening in the logs? I've made a couple of tweaks
since and it gets through the negotiation but then timesout:

  console: looking for 22:(ast2600-default login:), sending None (always=False)
  /console: [    0.939039] aspeed-pcie 1e7700c0.pcie: [1] : tx idle timeout [0]
  -console: [    1.385144] spi-nor spi0.1: unrecognized JEDEC id bytes: 00 00 00 00 00 00
  console: [    1.413028] spi-nor spi2.0: unrecognized JEDEC id bytes: 00 00 00 00 00 00                                                                                       
  console: [    1.617539] aspeed-rtc 1e781000.rtc: hctosys: unable to read the hardware clock
  -console: rofs = mtd4 squashfs rwfs = mtd5 jffs2
  /console: [   14.563495] systemd[1]: Failed to find module 'autofs4'                  
  -console: [   18.034371] systemd[176]: /lib/systemd/system-generators/systemd-gpt-auto-generator failed with exit status 1.
  |console: [FAILED] Failed to start Intel Power Control for the Host 0.
  /console: [FAILED] Failed to start Phosphor C…istening on device /dev/ttyS2.
  console: [DEPEND] Dependency failed for Host logger for ttyS2.
  -console: Phosphor OpenBMC (Phosphor OpenBMC Project Reference Distro) nodistro.0 ast2600-default ttyS4
  console: ast2600-default login:
   (always=False)g for 9:(Password:), sending root
  console: root
  |console: Password:
   (always=False)g for 23:(root@ast2600-default:~#), sending 0penBmc
  console: Login timed out after 60 seconds.
  |console: Phosphor OpenBMC (Phosphor OpenBMC Project Reference Distro) nodistro.0 ast2600-default ttyS4
  /avocado.test:

>
> Thanks for the time you spend on this.
>
> C.
>
>
>> ---
>> v2
>>    - remove superfluous /r/n
>> ---
>>   tests/avocado/avocado_qemu/__init__.py | 89 +++++++++++++++++++++++++-
>>   1 file changed, 88 insertions(+), 1 deletion(-)
>> diff --git a/tests/avocado/avocado_qemu/__init__.py
>> b/tests/avocado/avocado_qemu/__init__.py
>> index 910f3ba1ea..20cba57161 100644
>> --- a/tests/avocado/avocado_qemu/__init__.py
>> +++ b/tests/avocado/avocado_qemu/__init__.py
>> @@ -131,6 +131,58 @@ def pick_default_qemu_bin(bin_prefix='qemu-system-', arch=None):
>>               return path
>>       return None
>>   +def _peek_ahead(console, min_match, success_message):
>> +    """
>> +    peek ahead in the console stream keeping an eye out for the
>> +    success message.
>> +
>> +    Returns some message to process or None, indicating the normal
>> +    readline should occur.
>> +    """
>> +    console_logger = logging.getLogger('console')
>> +    peek_len = 0
>> +    retries = 0
>> +
>> +    while True:
>> +        try:
>> +            old_peek_len = peek_len
>> +            peek_ahead = console.peek(min_match).decode()
>> +            peek_len = len(peek_ahead)
>> +
>> +            # if we get stuck too long lets just fallback to readline
>> +            if peek_len <= old_peek_len:
>> +                retries += 1
>> +                if retries > 10:
>> +                    return None
>> +
>> +            # if we see a newline in the peek we can let safely bail
>> +            # and let the normal readline() deal with it
>> +            if peek_ahead.endswith(('\n', '\r')):
>> +                return None
>> +
>> +            # if we haven't seen enough for the whole message but the
>> +            # first part matches lets just loop again
>> +            if len(peek_ahead) < min_match and \
>> +               success_message[:peek_len] in peek_ahead:
>> +                continue
>> +
>> +            # if we see the whole success_message we are done, consume
>> +            # it and pass back so we can exit to the user
>> +            if success_message in peek_ahead:
>> +                return console.read(peek_len).decode()
>> +
>> +            # of course if we've seen enough then this line probably
>> +            # doesn't contain what we are looking for, fallback
>> +            if peek_len > min_match:
>> +                return None
>> +
>> +        except UnicodeDecodeError:
>> +            console_logger.log("error in decode of peek")
>> +            return None
>> +
>> +    # we should never get here
>> +    return None
>> +
>>     def _console_interaction(test, success_message, failure_message,
>>                            send_string, keep_sending=False, vm=None):
>> @@ -139,17 +191,52 @@ def _console_interaction(test, success_message, failure_message,
>>           vm = test.vm
>>       console = vm.console_socket.makefile(mode='rb', encoding='utf-8')
>>       console_logger = logging.getLogger('console')
>> +
>> +    # Establish the minimum number of bytes we would need to
>> +    # potentially match against success_message
>> +    if success_message is not None:
>> +        min_match = len(success_message)
>> +    else:
>> +        min_match = 0
>> +
>> +    console_logger.debug("looking for %d:(%s), sending %s (always=%s)",
>> +                         min_match, success_message, send_string, keep_sending)
>> +
>>       while True:
>> +        msg = None
>> +
>> +        # First send our string, optionally repeating the send next
>> +        # cycle.
>>           if send_string:
>>               vm.console_socket.sendall(send_string.encode())
>>               if not keep_sending:
>>                   send_string = None # send only once
>> +
>> +        # If the console has no data to read we briefly
>> +        # sleep before continuing.
>> +        if not console.readable():
>> +            time.sleep(0.1)
>> +            continue
>> +
>>           try:
>> -            msg = console.readline().decode().strip()
>> +
>> +            # First we shall peek ahead for a potential match to cover waiting
>> +            # for lines without any newlines.
>> +            if min_match > 0:
>> +                msg = _peek_ahead(console, min_match, success_message)
>> +
>> +            # otherwise we block here for a full line
>> +            if not msg:
>> +                msg = console.readline().decode().strip()
>> +
>>           except UnicodeDecodeError:
>> +            console_logger.debug("skipped unicode error")
>>               msg = None
>> +
>> +        # if nothing came out we continue and try again
>>           if not msg:
>>               continue
>> +
>>           console_logger.debug(msg)
>>           if success_message is None or success_message in msg:
>>               break


-- 
Alex Bennée

Re: [PATCH v2 02/12] tests/avocado: improve behaviour waiting for login prompts

[Cédric Le Goater]

On 11/17/22 15:04, Alex Bennée wrote:
> 
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> Hello Alex,
>>
>> On 11/11/22 15:55, Alex Bennée wrote:
>>> This attempts to deal with the problem of login prompts not being
>>> guaranteed to be terminated with a newline. The solution to this is to
>>> peek at the incoming data looking to see if we see an up-coming match
>>> before we fall back to the old readline() logic. The reason to mostly
>>> rely on readline is because I am occasionally seeing the peek stalling
>>> despite data being there.
>>> This seems kinda hacky and gross so I'm open to alternative
>>> approaches
>>> and cleaner python code.
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>
>> I have pulled this patch in the aspeed tree hoping it would improve tests:
>>
>>    AST2x00MachineSDK.test_arm_ast2500_evb_sdk
>>    AST2x00MachineSDK.test_arm_ast2600_evb_sdk
>>
>> but the failure rate has increased :/ I have seen failures in these also :
>>
>>    AST2x00Machine.test_arm_ast2500_evb_buildroot
>>    AST2x00Machine.test_arm_ast2600_evb_buildroot
>>
>> which used to be quite stable.
>>
>> Sorry, this is not of much help. the issue might be elsewhere.
> 
> Do you see what is happening in the logs? I've made a couple of tweaks
> since and it gets through the negotiation but then timesout:
> 
>    console: looking for 22:(ast2600-default login:), sending None (always=False)
>    /console: [    0.939039] aspeed-pcie 1e7700c0.pcie: [1] : tx idle timeout [0]
>    -console: [    1.385144] spi-nor spi0.1: unrecognized JEDEC id bytes: 00 00 00 00 00 00
>    console: [    1.413028] spi-nor spi2.0: unrecognized JEDEC id bytes: 00 00 00 00 00 00
>    console: [    1.617539] aspeed-rtc 1e781000.rtc: hctosys: unable to read the hardware clock
>    -console: rofs = mtd4 squashfs rwfs = mtd5 jffs2
>    /console: [   14.563495] systemd[1]: Failed to find module 'autofs4'
>    -console: [   18.034371] systemd[176]: /lib/systemd/system-generators/systemd-gpt-auto-generator failed with exit status 1.
>    |console: [FAILED] Failed to start Intel Power Control for the Host 0.
>    /console: [FAILED] Failed to start Phosphor C…istening on device /dev/ttyS2.
>    console: [DEPEND] Dependency failed for Host logger for ttyS2.
>    -console: Phosphor OpenBMC (Phosphor OpenBMC Project Reference Distro) nodistro.0 ast2600-default ttyS4
>    console: ast2600-default login:
>     (always=False)g for 9:(Password:), sending root
>    console: root
>    |console: Password:
>     (always=False)g for 23:(root@ast2600-default:~#), sending 0penBmc
>    console: Login timed out after 60 seconds.
>    |console: Phosphor OpenBMC (Phosphor OpenBMC Project Reference Distro) nodistro.0 ast2600-default ttyS4
>    /avocado.test:

Same for me :

console: Starting kernel ...
console: looking for 22:(ast2600-default login:), sending None (always=False)
\console: [    0.845678] aspeed-pcie 1e7700c0.pcie: [1] : tx idle timeout [0]
|console: [    1.388468] spi-nor spi0.1: unrecognized JEDEC id bytes: 00 00 00 00 00 00
console: [    1.409448] spi-nor spi2.0: unrecognized JEDEC id bytes: 00 00 00 00 00 00
console: [    1.564132] aspeed-rtc 1e781000.rtc: hctosys: unable to read the hardware clock
\console: rofs = mtd4 squashfs rwfs = mtd5 jffs2
/console: [   12.708097] systemd[1]: Failed to find module 'autofs4'
|console: [   14.757409] systemd[177]: /lib/systemd/system-generators/systemd-gpt-auto-generator failed with exit status 1.
|console: [DEPEND] Dependency failed for Host logger for ttyS2.
\console: Phosphor OpenBMC (Phosphor OpenBMC Project Reference Distro) nodistro.0 ast2600-default ttyS4
console:
ast2600-default login:
  (always=False)g for 9:(Password:), sending root
console: root
-console: Password:
  (always=False)g for 23:(root@ast2600-default:~#), sending 0penBmc
INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout reached\nOriginal status: ERROR\n{'name': '1-build/tests/avocado/machine_aspeed.py:AST2x00MachineSDK.test_arm_ast2600_evb_sdk', 'logdir': '/home/legoater/avocado/job-results/job-2022-11-17T18.03-780... (240.28 s)