[PATCH v2 00/11] more misc fixes for 8.0 (tests, gdbstub, meta, docs)

[PATCH v2 00/11] more misc fixes for 8.0 (tests, gdbstub, meta, docs)

[Alex Bennée]

Testing of kaniko as an alternative builder ran into the weeds so that
patch has been dropped. It looks like the gitlab registry doesn't
support layer caching. However the build also seems to be very
unstable leading to a bunch of container build failures, e.g.:

  https://gitlab.com/stsquad/qemu/-/pipelines/823381159/failures

I've also dropped the avocado version bump. We do gain two new patches
- one minor gdbstub fix for BSD and including the Xen KVM test.

It would be nice to get some review of the documentation update from
the block maintainers but if no one objects I still intend to merge it
in lieu of anything better.

I'll roll the PR tomorrow morning.

Alex.

Alex Bennée (6):
  scripts/coverage: initial coverage comparison script
  gdbstub: don't report auxv feature unless on Linux
  MAINTAINERS: add a section for policy documents
  qemu-options: finesse the recommendations around -blockdev
  metadata: add .git-blame-ignore-revs
  gitlab: fix typo

Daniel P. Berrangé (2):
  tests/qemu-iotests: explicitly invoke 'check' via 'python'
  tests/vm: use the default system python for NetBSD

David Woodhouse (1):
  tests/avocado: Test Xen guest support under KVM

Marco Liebel (1):
  Use hexagon toolchain version 16.0.0

Philippe Mathieu-Daudé (1):
  gdbstub: Only build libgdb_user.fa / libgdb_softmmu.fa if necessary

 MAINTAINERS                                   |  19 ++
 gdbstub/gdbstub.c                             |   2 +-
 .git-blame-ignore-revs                        |  21 +++
 .gitlab-ci.d/base.yml                         |   2 +-
 gdbstub/meson.build                           |   6 +-
 qemu-options.hx                               |   8 +-
 scripts/coverage/compare_gcov_json.py         | 119 ++++++++++++
 tests/avocado/kvm_xen_guest.py                | 170 ++++++++++++++++++
 .../dockerfiles/debian-hexagon-cross.docker   |   2 +-
 tests/qemu-iotests/meson.build                |   7 +-
 tests/vm/netbsd                               |   3 +-
 11 files changed, 347 insertions(+), 12 deletions(-)
 create mode 100644 .git-blame-ignore-revs
 create mode 100755 scripts/coverage/compare_gcov_json.py
 create mode 100644 tests/avocado/kvm_xen_guest.py

-- 
2.39.2

[PATCH v2 01/11] scripts/coverage: initial coverage comparison script

[Alex Bennée]

This is a very rough and ready first pass at comparing gcovr's json
output between two different runs. At the moment it will give you a
file level diff between two runs but hopefully it wont be too hard to
extend to give better insight.

After generating the coverage results you run with something like:

  ./scripts/coverage/compare_gcov_json.py \
    -a ./builds/gcov.config1/coverage.json \
    -b ./builds/gcov.config2/coverage.json

My hope is we can use this to remove some redundancy from testing as
well as evaluate if new tests are actually providing additional
coverage or just burning our precious CI time.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Kautuk Consul <kconsul@linux.vnet.ibm.com>
Acked-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20230330101141.30199-2-alex.bennee@linaro.org>
---
 MAINTAINERS                           |   5 ++
 scripts/coverage/compare_gcov_json.py | 119 ++++++++++++++++++++++++++
 2 files changed, 124 insertions(+)
 create mode 100755 scripts/coverage/compare_gcov_json.py

diff --git a/MAINTAINERS b/MAINTAINERS
index ef45b5e71e..9e1a60ea24 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3908,3 +3908,8 @@ Performance Tools and Tests
 M: Ahmed Karaman <ahmedkhaledkaraman@gmail.com>
 S: Maintained
 F: scripts/performance/
+
+Code Coverage Tools
+M: Alex Bennée <alex.bennee@linaro.org>
+S: Odd Fixes
+F: scripts/coverage/
diff --git a/scripts/coverage/compare_gcov_json.py b/scripts/coverage/compare_gcov_json.py
new file mode 100755
index 0000000000..1b92dc2c8c
--- /dev/null
+++ b/scripts/coverage/compare_gcov_json.py
@@ -0,0 +1,119 @@
+#!/usr/bin/env python3
+#
+# Compare output of two gcovr JSON reports and report differences. To
+# generate the required output first:
+#   - create two build dirs with --enable-gcov
+#   - run set of tests in each
+#   - run make coverage-html in each
+#   - run gcovr --json --exclude-unreachable-branches \
+#           --print-summary -o coverage.json --root ../../ . *.p
+#
+# Author: Alex Bennée <alex.bennee@linaro.org>
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+#
+
+import argparse
+import json
+import sys
+from pathlib import Path
+
+def create_parser():
+    parser = argparse.ArgumentParser(
+        prog='compare_gcov_json',
+        description='analyse the differences in coverage between two runs')
+
+    parser.add_argument('-a', type=Path, default=None,
+                        help=('First file to check'))
+
+    parser.add_argument('-b', type=Path, default=None,
+                        help=('Second file to check'))
+
+    parser.add_argument('--verbose', action='store_true', default=False,
+                        help=('A minimal verbosity level that prints the '
+                              'overall result of the check/wait'))
+    return parser
+
+
+# See https://gcovr.com/en/stable/output/json.html#json-format-reference
+def load_json(json_file_path: Path, verbose = False) -> dict[str, set[int]]:
+
+    with open(json_file_path) as f:
+        data = json.load(f)
+
+    root_dir = json_file_path.absolute().parent
+    covered_lines = dict()
+
+    for filecov in data["files"]:
+        file_path = Path(filecov["file"])
+
+        # account for generated files - map into src tree
+        resolved_path = Path(file_path).absolute()
+        if resolved_path.is_relative_to(root_dir):
+            file_path = resolved_path.relative_to(root_dir)
+            # print(f"remapped {resolved_path} to {file_path}")
+
+        lines = filecov["lines"]
+
+        executed_lines = set(
+            linecov["line_number"]
+            for linecov in filecov["lines"]
+            if linecov["count"] != 0 and not linecov["gcovr/noncode"]
+        )
+
+        # if this file has any coverage add it to the system
+        if len(executed_lines) > 0:
+            if verbose:
+                print(f"file {file_path} {len(executed_lines)}/{len(lines)}")
+            covered_lines[str(file_path)] = executed_lines
+
+    return covered_lines
+
+def find_missing_files(first, second):
+    """
+    Return a list of files not covered in the second set
+    """
+    missing_files = []
+    for f in sorted(first):
+        file_a = first[f]
+        try:
+            file_b = second[f]
+        except KeyError:
+            missing_files.append(f)
+
+    return missing_files
+
+def main():
+    """
+    Script entry point
+    """
+    parser = create_parser()
+    args = parser.parse_args()
+
+    if not args.a or not args.b:
+        print("We need two files to compare")
+        sys.exit(1)
+
+    first_coverage = load_json(args.a, args.verbose)
+    second_coverage = load_json(args.b, args.verbose)
+
+    first_missing = find_missing_files(first_coverage,
+                                       second_coverage)
+
+    second_missing = find_missing_files(second_coverage,
+                                        first_coverage)
+
+    a_name = args.a.parent.name
+    b_name = args.b.parent.name
+
+    print(f"{b_name} missing coverage in {len(first_missing)} files")
+    for f in first_missing:
+        print(f"  {f}")
+
+    print(f"{a_name} missing coverage in {len(second_missing)} files")
+    for f in second_missing:
+        print(f"  {f}")
+
+
+if __name__ == '__main__':
+    main()
-- 
2.39.2

[PATCH v2 02/11] gdbstub: Only build libgdb_user.fa / libgdb_softmmu.fa if necessary

[Alex Bennée]

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

It is pointless to build libgdb_user.fa in a system-only build
(or libgdb_softmmu.fa in a user-only build). Besides, in some
restricted build configurations, some APIs might be restricted /
not available. Example in a KVM-only builds where TCG is disabled:

  $ ninja qemu-system-x86_64
  [99/2187] Compiling C object gdbstub/libgdb_user.fa.p/user.c.o
  FAILED: gdbstub/libgdb_user.fa.p/user.c.o
  ../../gdbstub/user.c: In function ‘gdb_breakpoint_insert’:
  ../../gdbstub/user.c:438:19: error: implicit declaration of function ‘cpu_breakpoint_insert’; did you mean ‘gdb_breakpoint_insert’? [-Werror=implicit-function-declaration]
    438 |             err = cpu_breakpoint_insert(cpu, addr, BP_GDB, NULL);
        |                   ^~~~~~~~~~~~~~~~~~~~~
        |                   gdb_breakpoint_insert
  ../../gdbstub/user.c:438:19: error: nested extern declaration of ‘cpu_breakpoint_insert’ [-Werror=nested-externs]
  ../../gdbstub/user.c: In function ‘gdb_breakpoint_remove’:
  ../../gdbstub/user.c:459:19: error: implicit declaration of function ‘cpu_breakpoint_remove’; did you mean ‘gdb_breakpoint_remove’? [-Werror=implicit-function-declaration]
    459 |             err = cpu_breakpoint_remove(cpu, addr, BP_GDB);
        |                   ^~~~~~~~~~~~~~~~~~~~~
        |                   gdb_breakpoint_remove
  ../../gdbstub/user.c:459:19: error: nested extern declaration of ‘cpu_breakpoint_remove’ [-Werror=nested-externs]
  cc1: all warnings being treated as errors
  ninja: build stopped: subcommand failed.

Fixes: 61b2e136db ("gdbstub: only compile gdbstub twice for whole build")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20230329161852.84992-1-philmd@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20230330101141.30199-3-alex.bennee@linaro.org>
---
 gdbstub/meson.build | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdbstub/meson.build b/gdbstub/meson.build
index bd5c5cd67d..cdb4d28691 100644
--- a/gdbstub/meson.build
+++ b/gdbstub/meson.build
@@ -20,11 +20,13 @@ gdb_softmmu_ss = gdb_softmmu_ss.apply(config_host, strict: false)
 libgdb_user = static_library('gdb_user',
                              gdb_user_ss.sources() + genh,
                              name_suffix: 'fa',
-                             c_args: '-DCONFIG_USER_ONLY')
+                             c_args: '-DCONFIG_USER_ONLY',
+                             build_by_default: have_user)
 
 libgdb_softmmu = static_library('gdb_softmmu',
                                 gdb_softmmu_ss.sources() + genh,
-                                name_suffix: 'fa')
+                                name_suffix: 'fa',
+                                build_by_default: have_system)
 
 gdb_user = declare_dependency(link_whole: libgdb_user)
 user_ss.add(gdb_user)
-- 
2.39.2

[PATCH v2 03/11] gdbstub: don't report auxv feature unless on Linux

[Alex Bennée]

The later handler if conditionally compiled only for Linux but we
forgot to ensure we don't advertise it lest we confuse our BSD
brethren.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Fixes: 51c623b0de ("gdbstub: add support to Xfer:auxv:read: packet")
Reported-by: Warner Losh <imp@bsdimp.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Warner Losh <imp@bsdimp.com>
Tested-by: Warner Losh <imp@bsdimp.com>
Message-Id: <20230403120250.2071560-1-alex.bennee@linaro.org>
---
 gdbstub/gdbstub.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 2a66371aa5..0760d78685 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1468,7 +1468,7 @@ static void handle_query_supported(GArray *params, void *user_ctx)
             ";ReverseStep+;ReverseContinue+");
     }
 
-#ifdef CONFIG_USER_ONLY
+#if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
     if (gdbserver_state.c_cpu->opaque) {
         g_string_append(gdbserver_state.str_buf, ";qXfer:auxv:read+");
     }
-- 
2.39.2

[PATCH v2 04/11] MAINTAINERS: add a section for policy documents

[Alex Bennée]

We don't update these often but now at least we have a few like minded
individuals keeping reviewers eye out for changes.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Warner Losh <imp@bsdimp.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Kashyap Chamarthy <kchamart@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230330101141.30199-4-alex.bennee@linaro.org>
---
 MAINTAINERS | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9e1a60ea24..2f67894604 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -64,6 +64,20 @@ L: qemu-devel@nongnu.org
 F: *
 F: */
 
+Project policy and developer guides
+R: Alex Bennée <alex.bennee@linaro.org>
+R: Daniel P. Berrangé <berrange@redhat.com>
+R: Thomas Huth <thuth@redhat.com>
+R: Markus Armbruster <armbru@redhat.com>
+R: Philippe Mathieu-Daudé <philmd@linaro.org>
+W: https://www.qemu.org/docs/master/devel/index.html
+S: Odd Fixes
+F: docs/devel/style.rst
+F: docs/devel/code-of-conduct.rst
+F: docs/devel/conflict-resolution.rst
+F: docs/devel/submitting-a-patch.rst
+F: docs/devel/submitting-a-pull-request.rst
+
 Responsible Disclosure, Reporting Security Issues
 -------------------------------------------------
 W: https://wiki.qemu.org/SecurityProcess
-- 
2.39.2

[PATCH v2 05/11] qemu-options: finesse the recommendations around -blockdev

[Alex Bennée]

We are a bit premature in recommending -blockdev/-device as the best
way to configure block devices, especially in the common case.
Improve the language to hopefully make things clearer.

Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20230330101141.30199-5-alex.bennee@linaro.org>
---
 qemu-options.hx | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 59bdf67a2c..9a69ed838e 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1143,10 +1143,14 @@ have gone through several iterations as the feature set and complexity
 of the block layer have grown. Many online guides to QEMU often
 reference older and deprecated options, which can lead to confusion.
 
-The recommended modern way to describe disks is to use a combination of
+The most explicit way to describe disks is to use a combination of
 ``-device`` to specify the hardware device and ``-blockdev`` to
 describe the backend. The device defines what the guest sees and the
-backend describes how QEMU handles the data.
+backend describes how QEMU handles the data. The ``-drive`` option
+combines the device and backend into a single command line options
+which is useful in the majority of cases. Older options like ``-hda``
+bake in a lot of assumptions from the days when QEMU was emulating a
+legacy PC, they are not recommended for modern configurations.
 
 ERST
 
-- 
2.39.2

[PATCH v2 06/11] metadata: add .git-blame-ignore-revs

[Alex Bennée]

Someone mentioned this on IRC so I thought I would try it out with a
few commits that are pure code style fixes.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20230318115657.1345921-1-alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230330101141.30199-6-alex.bennee@linaro.org>

---
v2
  - rm extraneous +
---
 .git-blame-ignore-revs | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 .git-blame-ignore-revs

diff --git a/.git-blame-ignore-revs b/.git-blame-ignore-revs
new file mode 100644
index 0000000000..93718ef425
--- /dev/null
+++ b/.git-blame-ignore-revs
@@ -0,0 +1,21 @@
+#
+# List of code-formatting clean ups the git blame can ignore
+#
+#   git blame --ignore-revs-file .git-blame-ignore-revs
+#
+# or
+#
+#   git config blame.ignoreRevsFile .git-blame-ignore-revs
+#
+
+# gdbstub: clean-up indents
+ad9e4585b3c7425759d3eea697afbca71d2c2082
+
+# e1000e: fix code style
+0eadd56bf53ab196a16d492d7dd31c62e1c24c32
+
+# target/riscv: coding style fixes
+8c7feddddd9218b407792120bcfda0347ed16205
+
+# replace TABs with spaces
+48805df9c22a0700fba4b3b548fafaa21726ca68
-- 
2.39.2

[PATCH v2 07/11] Use hexagon toolchain version 16.0.0

[Alex Bennée]

From: Marco Liebel <quic_mliebel@quicinc.com>

Signed-off-by: Marco Liebel <quic_mliebel@quicinc.com>
Reviewed-by: Brian Cain <bcain@quicinc.com>
Message-Id: <20230329142108.1199509-1-quic_mliebel@quicinc.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20230330101141.30199-7-alex.bennee@linaro.org>
---
 tests/docker/dockerfiles/debian-hexagon-cross.docker | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/dockerfiles/debian-hexagon-cross.docker b/tests/docker/dockerfiles/debian-hexagon-cross.docker
index 5308ccb8fe..b99d99f943 100644
--- a/tests/docker/dockerfiles/debian-hexagon-cross.docker
+++ b/tests/docker/dockerfiles/debian-hexagon-cross.docker
@@ -27,7 +27,7 @@ RUN apt-get update && \
 
 
 ENV TOOLCHAIN_INSTALL /opt
-ENV TOOLCHAIN_RELEASE 15.0.3
+ENV TOOLCHAIN_RELEASE 16.0.0
 ENV TOOLCHAIN_BASENAME "clang+llvm-${TOOLCHAIN_RELEASE}-cross-hexagon-unknown-linux-musl"
 ENV TOOLCHAIN_URL https://codelinaro.jfrog.io/artifactory/codelinaro-toolchain-for-hexagon/v${TOOLCHAIN_RELEASE}/${TOOLCHAIN_BASENAME}.tar.xz
 
-- 
2.39.2

[PATCH v2 08/11] tests/qemu-iotests: explicitly invoke 'check' via 'python'

[Alex Bennée]

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

The 'check' script will use "#!/usr/bin/env python3" by default
to locate python, but this doesn't work in distros which lack a
bare 'python3' binary like NetBSD.

We need to explicitly invoke 'check' by referring to the 'python'
variable in meson, which resolves to the detected python binary
that QEMU intends to use.

This fixes a regression introduced by

  commit 51ab5f8bd795d8980351f8531e54995ff9e6d163
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Wed Mar 15 17:43:23 2023 +0000

    iotests: register each I/O test separately with meson

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230329124539.822022-1-berrange@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20230330101141.30199-8-alex.bennee@linaro.org>
---
 tests/qemu-iotests/meson.build | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/meson.build b/tests/qemu-iotests/meson.build
index a162f683ef..9735071a29 100644
--- a/tests/qemu-iotests/meson.build
+++ b/tests/qemu-iotests/meson.build
@@ -47,19 +47,20 @@ foreach format, speed: qemu_iotests_formats
   endif
 
   rc = run_command(
-      [qemu_iotests_check_cmd] + args + ['-n'],
+      [python, qemu_iotests_check_cmd] + args + ['-n'],
       check: true,
   )
 
   foreach item: rc.stdout().strip().split()
-      args = ['-tap', '-' + format, item,
+      args = [qemu_iotests_check_cmd,
+              '-tap', '-' + format, item,
               '--source-dir', meson.current_source_dir(),
               '--build-dir', meson.current_build_dir()]
       # Some individual tests take as long as 45 seconds
       # Bump the timeout to 3 minutes for some headroom
       # on slow machines to minimize spurious failures
       test('io-' + format + '-' + item,
-           qemu_iotests_check_cmd,
+           python,
            args: args,
            depends: qemu_iotests_binaries,
            env: qemu_iotests_env,
-- 
2.39.2

[PATCH v2 09/11] tests/vm: use the default system python for NetBSD

[Alex Bennée]

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

Currently our NetBSD VM recipe requests instal of the python37 package
and explicitly tells QEMU to use that version of python. Since the
NetBSD base ISO was updated to version 9.3 though, the default system
python version is 3.9 which is sufficiently new for QEMU to rely on.
Rather than requesting an older python, just test against the default
system python which is what most users will have.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230329124601.822209-1-berrange@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20230330101141.30199-9-alex.bennee@linaro.org>
---
 tests/vm/netbsd | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/vm/netbsd b/tests/vm/netbsd
index aa54338dfa..0b9536ca17 100755
--- a/tests/vm/netbsd
+++ b/tests/vm/netbsd
@@ -30,7 +30,6 @@ class NetBSDVM(basevm.BaseVM):
         "git-base",
         "pkgconf",
         "xz",
-        "python37",
         "ninja-build",
 
         # gnu tools
@@ -66,7 +65,7 @@ class NetBSDVM(basevm.BaseVM):
         mkdir src build; cd src;
         tar -xf /dev/rld1a;
         cd ../build
-        ../src/configure --python=python3.7 --disable-opengl {configure_opts};
+        ../src/configure --disable-opengl {configure_opts};
         gmake --output-sync -j{jobs} {target} {verbose};
     """
     poweroff = "/sbin/poweroff"
-- 
2.39.2

[PATCH v2 10/11] gitlab: fix typo

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230330101141.30199-11-alex.bennee@linaro.org>
---
 .gitlab-ci.d/base.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.gitlab-ci.d/base.yml b/.gitlab-ci.d/base.yml
index 0274228de8..2fbb58d2a3 100644
--- a/.gitlab-ci.d/base.yml
+++ b/.gitlab-ci.d/base.yml
@@ -75,5 +75,5 @@
     - if: '$QEMU_CI != "2" && $CI_PROJECT_NAMESPACE != "qemu-project"'
       when: manual
 
-    # Jobs can run if any jobs they depend on were successfull
+    # Jobs can run if any jobs they depend on were successful
     - when: on_success
-- 
2.39.2

[PATCH v2 11/11] tests/avocado: Test Xen guest support under KVM

[Alex Bennée]

From: David Woodhouse <dwmw@amazon.co.uk>

Exercise guests with a few different modes for interrupt delivery. In
particular we want to cover:

 • Xen event channel delivery via GSI to the I/O APIC
 • Xen event channel delivery via GSI to the i8259 PIC
 • MSIs routed to PIRQ event channels
 • GSIs routed to PIRQ event channels

As well as some variants of normal non-Xen stuff like MSI to vAPIC and
PCI INTx going to the I/O APIC and PIC, which ought to still work even
in Xen mode.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20230324160719.1790792-1-alex.bennee@linaro.org>

---
v2
  - catch fail to launch and skip on lack of support
---
 tests/avocado/kvm_xen_guest.py | 170 +++++++++++++++++++++++++++++++++
 1 file changed, 170 insertions(+)
 create mode 100644 tests/avocado/kvm_xen_guest.py

diff --git a/tests/avocado/kvm_xen_guest.py b/tests/avocado/kvm_xen_guest.py
new file mode 100644
index 0000000000..112c955976
--- /dev/null
+++ b/tests/avocado/kvm_xen_guest.py
@@ -0,0 +1,170 @@
+# KVM Xen guest functional tests
+#
+# Copyright © 2021 Red Hat, Inc.
+# Copyright © 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+#
+# Author:
+#  David Woodhouse <dwmw2@infradead.org>
+#  Alex Bennée <alex.bennee@linaro.org>
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import os
+
+from qemu.machine import machine
+
+from avocado_qemu import LinuxSSHMixIn
+from avocado_qemu import QemuSystemTest
+from avocado_qemu import wait_for_console_pattern
+
+class KVMXenGuest(QemuSystemTest, LinuxSSHMixIn):
+    """
+    :avocado: tags=arch:x86_64
+    :avocado: tags=machine:q35
+    :avocado: tags=accel:kvm
+    :avocado: tags=kvm_xen_guest
+    """
+
+    KERNEL_DEFAULT = 'printk.time=0 root=/dev/xvda console=ttyS0'
+
+    kernel_path = None
+    kernel_params = None
+
+    # Fetch assets from the kvm-xen-guest subdir of my shared test
+    # images directory on fileserver.linaro.org where you can find
+    # build instructions for how they where assembled.
+    def get_asset(self, name, sha1):
+        base_url = ('https://fileserver.linaro.org/s/'
+                    'kE4nCFLdQcoBF9t/download?'
+                    'path=%2Fkvm-xen-guest&files=' )
+        url = base_url + name
+        # use explicit name rather than failing to neatly parse the
+        # URL into a unique one
+        return self.fetch_asset(name=name, locations=(url), asset_hash=sha1)
+
+    def common_vm_setup(self):
+        # We also catch lack of KVM_XEN support if we fail to launch
+        self.require_accelerator("kvm")
+
+        self.vm.set_console()
+
+        self.vm.add_args("-accel", "kvm,xen-version=0x4000a,kernel-irqchip=split")
+        self.vm.add_args("-smp", "2")
+
+        self.kernel_path = self.get_asset("bzImage",
+                                          "367962983d0d32109998a70b45dcee4672d0b045")
+        self.rootfs = self.get_asset("rootfs.ext4",
+                                     "f1478401ea4b3fa2ea196396be44315bab2bb5e4")
+
+    def run_and_check(self):
+        self.vm.add_args('-kernel', self.kernel_path,
+                         '-append', self.kernel_params,
+                         '-drive',  f"file={self.rootfs},if=none,format=raw,id=drv0",
+                         '-device', 'xen-disk,drive=drv0,vdev=xvda',
+                         '-device', 'virtio-net-pci,netdev=unet',
+                         '-netdev', 'user,id=unet,hostfwd=:127.0.0.1:0-:22')
+
+        try:
+            self.vm.launch()
+        except machine.VMLaunchFailure as e:
+            if "Xen HVM guest support not present" in e.output:
+                self.cancel("KVM Xen support is not present (need v5.12+ kernel with CONFIG_KVM_XEN)")
+            elif "Property 'kvm-accel.xen-version' not found" in e.output:
+                self.cancel("QEMU not built with CONFIG_XEN_EMU support")
+            else:
+                raise e
+
+        self.log.info('VM launched, waiting for sshd')
+        console_pattern = 'Starting dropbear sshd: OK'
+        wait_for_console_pattern(self, console_pattern, 'Oops')
+        self.log.info('sshd ready')
+        self.ssh_connect('root', '', False)
+
+        self.ssh_command('cat /proc/cmdline')
+        self.ssh_command('dmesg | grep -e "Grant table initialized"')
+
+    def test_kvm_xen_guest(self):
+        """
+        :avocado: tags=kvm_xen_guest
+        """
+
+        self.common_vm_setup()
+
+        self.kernel_params = (self.KERNEL_DEFAULT +
+                              ' xen_emul_unplug=ide-disks')
+        self.run_and_check()
+        self.ssh_command('grep xen-pirq.*msi /proc/interrupts')
+
+    def test_kvm_xen_guest_nomsi(self):
+        """
+        :avocado: tags=kvm_xen_guest_nomsi
+        """
+
+        self.common_vm_setup()
+
+        self.kernel_params = (self.KERNEL_DEFAULT +
+                              ' xen_emul_unplug=ide-disks pci=nomsi')
+        self.run_and_check()
+        self.ssh_command('grep xen-pirq.* /proc/interrupts')
+
+    def test_kvm_xen_guest_noapic_nomsi(self):
+        """
+        :avocado: tags=kvm_xen_guest_noapic_nomsi
+        """
+
+        self.common_vm_setup()
+
+        self.kernel_params = (self.KERNEL_DEFAULT +
+                              ' xen_emul_unplug=ide-disks noapic pci=nomsi')
+        self.run_and_check()
+        self.ssh_command('grep xen-pirq /proc/interrupts')
+
+    def test_kvm_xen_guest_vapic(self):
+        """
+        :avocado: tags=kvm_xen_guest_vapic
+        """
+
+        self.common_vm_setup()
+        self.vm.add_args('-cpu', 'host,+xen-vapic')
+        self.kernel_params = (self.KERNEL_DEFAULT +
+                              ' xen_emul_unplug=ide-disks')
+        self.run_and_check()
+        self.ssh_command('grep xen-pirq /proc/interrupts')
+        self.ssh_command('grep PCI-MSI /proc/interrupts')
+
+    def test_kvm_xen_guest_novector(self):
+        """
+        :avocado: tags=kvm_xen_guest_novector
+        """
+
+        self.common_vm_setup()
+        self.kernel_params = (self.KERNEL_DEFAULT +
+                              ' xen_emul_unplug=ide-disks' +
+                              ' xen_no_vector_callback')
+        self.run_and_check()
+        self.ssh_command('grep xen-platform-pci /proc/interrupts')
+
+    def test_kvm_xen_guest_novector_nomsi(self):
+        """
+        :avocado: tags=kvm_xen_guest_novector_nomsi
+        """
+
+        self.common_vm_setup()
+
+        self.kernel_params = (self.KERNEL_DEFAULT +
+                              ' xen_emul_unplug=ide-disks pci=nomsi' +
+                              ' xen_no_vector_callback')
+        self.run_and_check()
+        self.ssh_command('grep xen-platform-pci /proc/interrupts')
+
+    def test_kvm_xen_guest_novector_noapic(self):
+        """
+        :avocado: tags=kvm_xen_guest_novector_noapic
+        """
+
+        self.common_vm_setup()
+        self.kernel_params = (self.KERNEL_DEFAULT +
+                              ' xen_emul_unplug=ide-disks' +
+                              ' xen_no_vector_callback noapic')
+        self.run_and_check()
+        self.ssh_command('grep xen-platform-pci /proc/interrupts')
-- 
2.39.2

Re: [PATCH v2 09/11] tests/vm: use the default system python for NetBSD

[Philippe Mathieu-Daudé]

On 3/4/23 15:49, Alex Bennée wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> Currently our NetBSD VM recipe requests instal of the python37 package
> and explicitly tells QEMU to use that version of python. Since the
> NetBSD base ISO was updated to version 9.3 though, the default system
> python version is 3.9 which is sufficiently new for QEMU to rely on.
> Rather than requesting an older python, just test against the default
> system python which is what most users will have.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-Id: <20230329124601.822209-1-berrange@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <20230330101141.30199-9-alex.bennee@linaro.org>
> ---
>   tests/vm/netbsd | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)

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

Re: [PATCH v2 05/11] qemu-options: finesse the recommendations around -blockdev

[Kevin Wolf]

Am 03.04.2023 um 15:49 hat Alex Bennée geschrieben:
> We are a bit premature in recommending -blockdev/-device as the best
> way to configure block devices, especially in the common case.
> Improve the language to hopefully make things clearer.
> 
> Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <20230330101141.30199-5-alex.bennee@linaro.org>
> ---
>  qemu-options.hx | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 59bdf67a2c..9a69ed838e 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1143,10 +1143,14 @@ have gone through several iterations as the feature set and complexity
>  of the block layer have grown. Many online guides to QEMU often
>  reference older and deprecated options, which can lead to confusion.
>  
> -The recommended modern way to describe disks is to use a combination of
> +The most explicit way to describe disks is to use a combination of
>  ``-device`` to specify the hardware device and ``-blockdev`` to
>  describe the backend. The device defines what the guest sees and the
> -backend describes how QEMU handles the data.
> +backend describes how QEMU handles the data. The ``-drive`` option
> +combines the device and backend into a single command line options
> +which is useful in the majority of cases. Older options like ``-hda``
> +bake in a lot of assumptions from the days when QEMU was emulating a
> +legacy PC, they are not recommended for modern configurations.

Let's not make the use of -drive look more advisable than it really is.
If you're writing a management tool/script and you're still using -drive
today, you're doing it wrong.

Maybe this is actually the point where we should just clearly define
that -blockdev is the only supported stable API (like QMP), and that
-drive etc. are convenient shortcuts for human users with no
compatibility promise (like HMP).

What stopped us from doing so is that there are certain boards that
don't allow the user to configure the onboard devices, but that look at
-drive. These wouldn't provide any stable API any more after this
change. However, if this hasn't been solved in many years, maybe it's
time to view it as the board's problem, and use this change to motivate
them to implement ways to configure the devices. Or maybe some don't
even want to bother with a stable API, who knows.

Kevin

Re: [PATCH v2 05/11] qemu-options: finesse the recommendations around -blockdev

[Alex Bennée]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 03.04.2023 um 15:49 hat Alex Bennée geschrieben:
>> We are a bit premature in recommending -blockdev/-device as the best
>> way to configure block devices, especially in the common case.
>> Improve the language to hopefully make things clearer.
>> 
>> Suggested-by: Michael Tokarev <mjt@tls.msk.ru>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Message-Id: <20230330101141.30199-5-alex.bennee@linaro.org>
>> ---
>>  qemu-options.hx | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 59bdf67a2c..9a69ed838e 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -1143,10 +1143,14 @@ have gone through several iterations as the feature set and complexity
>>  of the block layer have grown. Many online guides to QEMU often
>>  reference older and deprecated options, which can lead to confusion.
>>  
>> -The recommended modern way to describe disks is to use a combination of
>> +The most explicit way to describe disks is to use a combination of
>>  ``-device`` to specify the hardware device and ``-blockdev`` to
>>  describe the backend. The device defines what the guest sees and the
>> -backend describes how QEMU handles the data.
>> +backend describes how QEMU handles the data. The ``-drive`` option
>> +combines the device and backend into a single command line options
>> +which is useful in the majority of cases. Older options like ``-hda``
>> +bake in a lot of assumptions from the days when QEMU was emulating a
>> +legacy PC, they are not recommended for modern configurations.
>
> Let's not make the use of -drive look more advisable than it really is.
> If you're writing a management tool/script and you're still using -drive
> today, you're doing it wrong.
>
> Maybe this is actually the point where we should just clearly define
> that -blockdev is the only supported stable API (like QMP), and that
> -drive etc. are convenient shortcuts for human users with no
> compatibility promise (like HMP).

OK I'll drop this patch from today's PR and await a better description
in due course.

>
> What stopped us from doing so is that there are certain boards that
> don't allow the user to configure the onboard devices, but that look at
> -drive. These wouldn't provide any stable API any more after this
> change. However, if this hasn't been solved in many years, maybe it's
> time to view it as the board's problem, and use this change to motivate
> them to implement ways to configure the devices. Or maybe some don't
> even want to bother with a stable API, who knows.
>
> Kevin


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 05/11] qemu-options: finesse the recommendations around -blockdev

[Michael Tokarev]

04.04.2023 16:57, Kevin Wolf пишет:
> Let's not make the use of -drive look more advisable than it really is.
> If you're writing a management tool/script and you're still using -drive
> today, you're doing it wrong.

Kevin, maybe I'm wrong here, but what to do with the situation which
started it all, -- with -snapshot?

If anything, I think there should be a bold note that -snapshot is broken
by -blockdev.  Users are learning that the *hard* way, after losing their
data..

/mjt

Re: [PATCH v2 05/11] qemu-options: finesse the recommendations around -blockdev

[Kevin Wolf]

Am 04.04.2023 um 17:07 hat Michael Tokarev geschrieben:
> 04.04.2023 16:57, Kevin Wolf пишет:
> > Let's not make the use of -drive look more advisable than it really is.
> > If you're writing a management tool/script and you're still using -drive
> > today, you're doing it wrong.
> 
> Kevin, maybe I'm wrong here, but what to do with the situation which
> started it all, -- with -snapshot?
> 
> If anything, I think there should be a bold note that -snapshot is
> broken by -blockdev.  Users are learning that the *hard* way, after
> losing their data..

Ah, I missed this context.

Maybe -snapshot should error out if -blockdev is in use. You'd generally
expect that either -blockdev is used primarily and snapshots are done
externally (if the command line is generated by some management tool),
or that -drive is used consistently (by a human who likes the
convenience). In both cases, we wouldn't hit the error path.

There may be some exceptional cases where you have both -drive and
-blockdev (maybe because a human users needs more control for one
specific disk). This is the case where you can get a nasty surprise and
that would error out. If you legitimately want the -drive images
snapshotted, but not the -blockdev ones, you can still use individual
'-drive snapshot=on' options instead of the global '-snapshot' (and the
error message should mention this).

Would you see any problems with such an approach?

Kevin

Re: [PATCH v2 05/11] qemu-options: finesse the recommendations around -blockdev

[Reinoud Zandijk]

On Tue, Apr 04, 2023 at 06:17:45PM +0200, Kevin Wolf wrote:
> Am 04.04.2023 um 17:07 hat Michael Tokarev geschrieben:
> > 04.04.2023 16:57, Kevin Wolf пишет:
> Maybe -snapshot should error out if -blockdev is in use. You'd generally
> expect that either -blockdev is used primarily and snapshots are done
> externally (if the command line is generated by some management tool),
> or that -drive is used consistently (by a human who likes the
> convenience). In both cases, we wouldn't hit the error path.
> 
> There may be some exceptional cases where you have both -drive and
> -blockdev (maybe because a human users needs more control for one
> specific disk). This is the case where you can get a nasty surprise and
> that would error out. If you legitimately want the -drive images
> snapshotted, but not the -blockdev ones, you can still use individual
> '-drive snapshot=on' options instead of the global '-snapshot' (and the
> error message should mention this).

I didn't know that! I normally use the -snapshot as global option. Is there a
reason why -blockdev isn't honouring -snapshot?

With regards,
Reinoud

Re: [PATCH v2 05/11] qemu-options: finesse the recommendations around -blockdev

[Kevin Wolf]

Am 06.04.2023 um 22:23 hat Reinoud Zandijk geschrieben:
> On Tue, Apr 04, 2023 at 06:17:45PM +0200, Kevin Wolf wrote:
> > Am 04.04.2023 um 17:07 hat Michael Tokarev geschrieben:
> > > 04.04.2023 16:57, Kevin Wolf пишет:
> > Maybe -snapshot should error out if -blockdev is in use. You'd generally
> > expect that either -blockdev is used primarily and snapshots are done
> > externally (if the command line is generated by some management tool),
> > or that -drive is used consistently (by a human who likes the
> > convenience). In both cases, we wouldn't hit the error path.
> > 
> > There may be some exceptional cases where you have both -drive and
> > -blockdev (maybe because a human users needs more control for one
> > specific disk). This is the case where you can get a nasty surprise and
> > that would error out. If you legitimately want the -drive images
> > snapshotted, but not the -blockdev ones, you can still use individual
> > '-drive snapshot=on' options instead of the global '-snapshot' (and the
> > error message should mention this).
> 
> I didn't know that! I normally use the -snapshot as global option. Is there a
> reason why -blockdev isn't honouring -snapshot?

The philosophy behind -blockdev is that you're explicit about every
image file (and other block node) you want to use and that QEMU doesn't
magically insert or change things behind your back.

For simple use cases that might not seem necessary, but many of the
newer functions added to the block layer, like the block jobs, are
operations that can work on any node in the block graph (i.e. any of the
open images, including backing files etc.). If QEMU changed something
behind your back, you can easily access the wrong image. Especially for
management software like libvirt this kind of magic that -drive involves
was really hard to work with because it always had to second guess what
the world _really_ looked like on the QEMU side.

For example, imagine you open foo.img with -snapshot. Now you want to
create a backup of your current state, so tell QEMU to backup the block
node for foo.img because that's what your VM is currently running on,
right? Except that nobody told you that the active image is actually a
temporary qcow2 image file that -snapshot created internally. You're
backing up the wrong image without the changes of your running VM.

So it's better to always be explicit, and then it's unambiguous which
image file you really mean in operations.

Kevin

Re: [PATCH v2 05/11] qemu-options: finesse the recommendations around -blockdev

[Alex Bennée]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 06.04.2023 um 22:23 hat Reinoud Zandijk geschrieben:
>> On Tue, Apr 04, 2023 at 06:17:45PM +0200, Kevin Wolf wrote:
>> > Am 04.04.2023 um 17:07 hat Michael Tokarev geschrieben:
>> > > 04.04.2023 16:57, Kevin Wolf пишет:
>> > Maybe -snapshot should error out if -blockdev is in use. You'd generally
>> > expect that either -blockdev is used primarily and snapshots are done
>> > externally (if the command line is generated by some management tool),
>> > or that -drive is used consistently (by a human who likes the
>> > convenience). In both cases, we wouldn't hit the error path.
>> > 
>> > There may be some exceptional cases where you have both -drive and
>> > -blockdev (maybe because a human users needs more control for one
>> > specific disk). This is the case where you can get a nasty surprise and
>> > that would error out. If you legitimately want the -drive images
>> > snapshotted, but not the -blockdev ones, you can still use individual
>> > '-drive snapshot=on' options instead of the global '-snapshot' (and the
>> > error message should mention this).
>> 
>> I didn't know that! I normally use the -snapshot as global option. Is there a
>> reason why -blockdev isn't honouring -snapshot?
>
> The philosophy behind -blockdev is that you're explicit about every
> image file (and other block node) you want to use and that QEMU doesn't
> magically insert or change things behind your back.
>
> For simple use cases that might not seem necessary, but many of the
> newer functions added to the block layer, like the block jobs, are
> operations that can work on any node in the block graph (i.e. any of the
> open images, including backing files etc.). If QEMU changed something
> behind your back, you can easily access the wrong image. Especially for
> management software like libvirt this kind of magic that -drive involves
> was really hard to work with because it always had to second guess what
> the world _really_ looked like on the QEMU side.
>
> For example, imagine you open foo.img with -snapshot. Now you want to
> create a backup of your current state, so tell QEMU to backup the block
> node for foo.img because that's what your VM is currently running on,
> right? Except that nobody told you that the active image is actually a
> temporary qcow2 image file that -snapshot created internally. You're
> backing up the wrong image without the changes of your running VM.
>
> So it's better to always be explicit, and then it's unambiguous which
> image file you really mean in operations.

With that in mind please review:

  Subject: [PATCH v3] qemu-options: finesse the recommendations around -blockdev
  Date: Thu,  6 Apr 2023 10:53:17 +0100
  Message-Id: <20230406095317.3321318-1-alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro