[PATCH 00/11] tweaks and fixes for 8.0-rc1 (tests, plugins, docs)

[PATCH 00/11] tweaks and fixes for 8.0-rc1 (tests, plugins, docs)

[Alex Bennée]

As usual for softfreeze I switch from my usual maintainer trees to
collecting up miscellaneous fixes and tweaks as we stabilise the
build. Currently these are still mostly testing focused although there
is a plugins fix I forgot to send a PR for earlier.

The following need review:

 - tests/avocado: don't use tags to define drive
 - include/exec: fix kerneldoc definition
 - tests/tcg: disable pauth for aarch64 gdb tests
 - tests/tcg: add some help output for running individual tests
 - include/qemu: add documentation for memory callbacks
 - gitlab: update centos-8-stream job
 - scripts/ci: update gitlab-runner playbook to handle CentOS
 - tests/docker: all add DOCKER_BUILDKIT to RUNC environment

Alex Bennée (9):
  tests/docker: all add DOCKER_BUILDKIT to RUNC environment
  scripts/ci: add libslirp-devel to build-environment
  scripts/ci: update gitlab-runner playbook to handle CentOS
  gitlab: update centos-8-stream job
  include/qemu: add documentation for memory callbacks
  tests/tcg: add some help output for running individual tests
  tests/tcg: disable pauth for aarch64 gdb tests
  include/exec: fix kerneldoc definition
  tests/avocado: don't use tags to define drive

Marcin Juszkiewicz (1):
  tests/avocado: update AArch64 tests to Alpine 3.17.2

Richard Henderson (1):
  tcg: Clear plugin_mem_cbs on TB exit

 include/exec/memory.h                         |  2 +-
 include/qemu/qemu-plugin.h                    | 47 +++++++++++++--
 accel/tcg/cpu-exec-common.c                   |  3 +
 accel/tcg/cpu-exec.c                          |  4 +-
 .../custom-runners/centos-stream-8-x86_64.yml | 18 ++----
 .../org.centos/stream/8/build-environment.yml |  1 +
 scripts/ci/setup/gitlab-runner.yml            | 20 ++++++-
 tests/avocado/machine_aarch64_virt.py         |  8 +--
 tests/avocado/tuxrun_baselines.py             | 60 ++++++++-----------
 tests/docker/Makefile.include                 |  2 +-
 tests/tcg/Makefile.target                     |  7 +++
 tests/tcg/aarch64/Makefile.target             |  2 +
 12 files changed, 112 insertions(+), 62 deletions(-)

-- 
2.39.2

[PATCH 01/11] tests/avocado: update AArch64 tests to Alpine 3.17.2

[Alex Bennée]

From: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>

To test Alpine boot on SBSA-Ref target we need Alpine Linux
'standard' image as 'virt' one lacks kernel modules.

So to minimalize Avocado cache I move test to 'standard' image.

Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-Id: <20230302191146.1790560-1-marcin.juszkiewicz@linaro.org>
---
 tests/avocado/machine_aarch64_virt.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/avocado/machine_aarch64_virt.py b/tests/avocado/machine_aarch64_virt.py
index 25dab8dc00..a90dc6ff4b 100644
--- a/tests/avocado/machine_aarch64_virt.py
+++ b/tests/avocado/machine_aarch64_virt.py
@@ -38,11 +38,11 @@ def test_alpine_virt_tcg_gic_max(self):
         :avocado: tags=accel:tcg
         """
         iso_url = ('https://dl-cdn.alpinelinux.org/'
-                   'alpine/v3.16/releases/aarch64/'
-                   'alpine-virt-3.16.3-aarch64.iso')
+                   'alpine/v3.17/releases/aarch64/'
+                   'alpine-standard-3.17.2-aarch64.iso')
 
         # Alpine use sha256 so I recalculated this myself
-        iso_sha1 = '0683bc089486d55c91bf6607d5ecb93925769bc0'
+        iso_sha1 = '76284fcd7b41fe899b0c2375ceb8470803eea839'
         iso_path = self.fetch_asset(iso_url, asset_hash=iso_sha1)
 
         self.vm.set_console()
@@ -65,7 +65,7 @@ def test_alpine_virt_tcg_gic_max(self):
         self.vm.add_args('-object', 'rng-random,id=rng0,filename=/dev/urandom')
 
         self.vm.launch()
-        self.wait_for_console_pattern('Welcome to Alpine Linux 3.16')
+        self.wait_for_console_pattern('Welcome to Alpine Linux 3.17')
 
 
     def common_aarch64_virt(self, machine):
-- 
2.39.2

[PATCH 02/11] tests/docker: all add DOCKER_BUILDKIT to RUNC environment

[Alex Bennée]

It seems we also need to pass DOCKER_BUILDKIT as an argument to docker
itself to get the full benefit of caching.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Suggested-by: Fabiano Rosas <farosas@suse.de>
---
 tests/docker/Makefile.include | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 54ed77f671..9401525325 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -39,7 +39,7 @@ docker-qemu-src: $(DOCKER_SRC_COPY)
 # General rule for building docker images.
 docker-image-%: $(DOCKER_FILES_DIR)/%.docker
 	  $(call quiet-command,			\
-		$(RUNC) build				\
+		DOCKER_BUILDKIT=1 $(RUNC) build		\
 		$(if $V,,--quiet)			\
 		$(if $(NOCACHE),--no-cache,		\
 			$(if $(DOCKER_REGISTRY),--cache-from $(DOCKER_REGISTRY)/qemu/$*)) \
-- 
2.39.2

[PATCH 03/11] scripts/ci: add libslirp-devel to build-environment

[Alex Bennée]

Without libslip enabled we won't have user networking which means the
KVM tests won't run.

Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 scripts/ci/org.centos/stream/8/build-environment.yml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/ci/org.centos/stream/8/build-environment.yml b/scripts/ci/org.centos/stream/8/build-environment.yml
index 0d094d70c3..1ead77e2cb 100644
--- a/scripts/ci/org.centos/stream/8/build-environment.yml
+++ b/scripts/ci/org.centos/stream/8/build-environment.yml
@@ -55,6 +55,7 @@
           - librados-devel
           - librbd-devel
           - libseccomp-devel
+          - libslirp-devel
           - libssh-devel
           - libxkbcommon-devel
           - lzo-devel
-- 
2.39.2

[PATCH 04/11] scripts/ci: update gitlab-runner playbook to handle CentOS

[Alex Bennée]

This was broken when we moved to using the pre-built packages as we
didn't take care to ensure we used RPMs where required.

NB: I could never get this to complete on my test setup but I suspect
this was down to network connectivity and timeouts while downloading.

Fixes: 69c4befba1 (scripts/ci: update gitlab-runner playbook to use latest runner)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 scripts/ci/setup/gitlab-runner.yml | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/scripts/ci/setup/gitlab-runner.yml b/scripts/ci/setup/gitlab-runner.yml
index 95d4199c03..1a1b270ff2 100644
--- a/scripts/ci/setup/gitlab-runner.yml
+++ b/scripts/ci/setup/gitlab-runner.yml
@@ -48,13 +48,29 @@
     - debug:
         msg: gitlab-runner arch is {{ gitlab_runner_arch }}
 
-    - name: Download the matching gitlab-runner
+    - name: Download the matching gitlab-runner (DEB)
       get_url:
         dest: "/root/"
         url: "https://gitlab-runner-downloads.s3.amazonaws.com/latest/deb/gitlab-runner_{{ gitlab_runner_arch }}.deb"
+      when:
+        - ansible_facts['distribution'] == 'Ubuntu'
+
+    - name: Download the matching gitlab-runner (RPM)
+      get_url:
+        dest: "/root/"
+        url: "https://gitlab-runner-downloads.s3.amazonaws.com/latest/rpm/gitlab-runner_{{ gitlab_runner_arch }}.rpm"
+      when:
+        - ansible_facts['distribution'] == 'CentOS'
 
-    - name: Install gitlab-runner via package manager
+    - name: Install gitlab-runner via package manager (DEB)
       apt: deb="/root/gitlab-runner_{{ gitlab_runner_arch }}.deb"
+      when:
+        - ansible_facts['distribution'] == 'Ubuntu'
+
+    - name: Install gitlab-runner via package manager (RPM)
+      yum: name="/root/gitlab-runner_{{ gitlab_runner_arch }}.rpm"
+      when:
+        - ansible_facts['distribution'] == 'CentOS'
 
     - name: Register the gitlab-runner
       command: "/usr/bin/gitlab-runner register --non-interactive --url {{ gitlab_runner_server_url }} --registration-token {{ gitlab_runner_registration_token }} --executor shell --tag-list {{ ansible_facts[\"architecture\"] }},{{ ansible_facts[\"distribution\"]|lower }}_{{ ansible_facts[\"distribution_version\"] }} --description '{{ ansible_facts[\"distribution\"] }} {{ ansible_facts[\"distribution_version\"] }} {{ ansible_facts[\"architecture\"] }} ({{ ansible_facts[\"os_family\"] }})'"
-- 
2.39.2

[PATCH 05/11] gitlab: update centos-8-stream job

[Alex Bennée]

A couple of clean-ups here:

  - inherit from the custom runners job for artefacts
  - call check-avocado directly
  - add some comments to the top about setup

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 .../custom-runners/centos-stream-8-x86_64.yml  | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/.gitlab-ci.d/custom-runners/centos-stream-8-x86_64.yml b/.gitlab-ci.d/custom-runners/centos-stream-8-x86_64.yml
index 068b0c4335..367424db78 100644
--- a/.gitlab-ci.d/custom-runners/centos-stream-8-x86_64.yml
+++ b/.gitlab-ci.d/custom-runners/centos-stream-8-x86_64.yml
@@ -1,4 +1,9 @@
+# All centos-stream-8 jobs should run successfully in an environment
+# setup by the scripts/ci/setup/stream/8/build-environment.yml task
+# "Installation of extra packages to build QEMU"
+
 centos-stream-8-x86_64:
+ extends: .custom_runner_template
  allow_failure: true
  needs: []
  stage: build
@@ -8,15 +13,6 @@ centos-stream-8-x86_64:
  rules:
  - if: '$CI_PROJECT_NAMESPACE == "qemu-project" && $CI_COMMIT_BRANCH =~ /^staging/'
  - if: "$CENTOS_STREAM_8_x86_64_RUNNER_AVAILABLE"
- artifacts:
-   name: "$CI_JOB_NAME-$CI_COMMIT_REF_SLUG"
-   when: on_failure
-   expire_in: 7 days
-   paths:
-     - build/tests/results/latest/results.xml
-     - build/tests/results/latest/test-results
-   reports:
-     junit: build/tests/results/latest/results.xml
  before_script:
  - JOBS=$(expr $(nproc) + 1)
  script:
@@ -25,6 +21,4 @@ centos-stream-8-x86_64:
  - ../scripts/ci/org.centos/stream/8/x86_64/configure
    || { cat config.log meson-logs/meson-log.txt; exit 1; }
  - make -j"$JOBS"
- - make NINJA=":" check
-   || { cat meson-logs/testlog.txt; exit 1; } ;
- - ../scripts/ci/org.centos/stream/8/x86_64/test-avocado
+ - make NINJA=":" check check-avocado
-- 
2.39.2

[PATCH 06/11] include/qemu: add documentation for memory callbacks

[Alex Bennée]

Some API documentation was missed, rectify that.

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1497
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/qemu-plugin.h | 47 ++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 4 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index d0e9d03adf..50a9957279 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -481,17 +481,56 @@ uint64_t qemu_plugin_hwaddr_phys_addr(const struct qemu_plugin_hwaddr *haddr);
  */
 const char *qemu_plugin_hwaddr_device_name(const struct qemu_plugin_hwaddr *h);
 
-typedef void
-(*qemu_plugin_vcpu_mem_cb_t)(unsigned int vcpu_index,
-                             qemu_plugin_meminfo_t info, uint64_t vaddr,
-                             void *userdata);
+/**
+ * typedef qemu_plugin_vcpu_mem_cb_t - memory callback function type
+ * @vcpu_index: the executing vCPU
+ * @info: an opaque handle for further queries about the memory
+ * @vaddr: the virtual address of the transaction
+ * @userdata: any user data attached to the callback
+ */
+typedef void (*qemu_plugin_vcpu_mem_cb_t) (unsigned int vcpu_index,
+                                           qemu_plugin_meminfo_t info,
+                                           uint64_t vaddr,
+                                           void *userdata);
 
+/**
+ * qemu_plugin_register_vcpu_mem_cb() - register memory access callback
+ * @insn: handle for instruction to instrument
+ * @cb: callback of type qemu_plugin_vcpu_mem_cb_t
+ * @flags: (currently unused) callback flags
+ * @rw: monitor reads, writes or both
+ * @userdata: opaque pointer for userdata
+ *
+ * This registers a full callback for every memory access generated by
+ * an instruction. If the instruction doesn't access memory no
+ * callback will be made.
+ *
+ * The callback reports the vCPU the access took place on, the virtual
+ * address of the access and a handle for further queries. The user
+ * can attach some userdata to the callback for additional purposes.
+ *
+ * Other execution threads will continue to execute during the
+ * callback so the plugin is responsible for ensuring it doesn't get
+ * confused by making appropriate use of locking if required.
+ */
 void qemu_plugin_register_vcpu_mem_cb(struct qemu_plugin_insn *insn,
                                       qemu_plugin_vcpu_mem_cb_t cb,
                                       enum qemu_plugin_cb_flags flags,
                                       enum qemu_plugin_mem_rw rw,
                                       void *userdata);
 
+/**
+ * qemu_plugin_register_vcpu_mem_inline() - register an inline op to any memory access
+ * @insn: handle for instruction to instrument
+ * @rw: apply to reads, writes or both
+ * @op: the op, of type qemu_plugin_op
+ * @ptr: pointer memory for the op
+ * @imm: immediate data for @op
+ *
+ * This registers a inline op every memory access generated by the
+ * instruction. This provides for a lightweight but not thread-safe
+ * way of counting the number of operations done.
+ */
 void qemu_plugin_register_vcpu_mem_inline(struct qemu_plugin_insn *insn,
                                           enum qemu_plugin_mem_rw rw,
                                           enum qemu_plugin_op op, void *ptr,
-- 
2.39.2

[PATCH 07/11] tcg: Clear plugin_mem_cbs on TB exit

[Alex Bennée]

From: Richard Henderson <richard.henderson@linaro.org>

Do this in cpu_tb_exec (normal exit) and cpu_loop_exit (exception),
adjacent to where we reset can_do_io.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1381
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20230301024737.1210851-2-richard.henderson@linaro.org>
[AJB: use plugin_gen_disable_mem_helpers()]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/cpu-exec-common.c | 3 +++
 accel/tcg/cpu-exec.c        | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/cpu-exec-common.c b/accel/tcg/cpu-exec-common.c
index c7bc8c6efa..e5847e9731 100644
--- a/accel/tcg/cpu-exec-common.c
+++ b/accel/tcg/cpu-exec-common.c
@@ -21,6 +21,7 @@
 #include "sysemu/cpus.h"
 #include "sysemu/tcg.h"
 #include "exec/exec-all.h"
+#include "exec/plugin-gen.h"
 
 bool tcg_allowed;
 
@@ -65,6 +66,8 @@ void cpu_loop_exit(CPUState *cpu)
 {
     /* Undo the setting in cpu_tb_exec.  */
     cpu->can_do_io = 1;
+    /* Undo any setting in generated code. */
+    plugin_gen_disable_mem_helpers();
     siglongjmp(cpu->jmp_env, 1);
 }
 
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 56aaf58b9d..e8a48dbd93 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -39,6 +39,7 @@
 #include "exec/replay-core.h"
 #include "sysemu/tcg.h"
 #include "exec/helper-proto.h"
+#include "exec/plugin-gen.h"
 #include "tb-jmp-cache.h"
 #include "tb-hash.h"
 #include "tb-context.h"
@@ -459,6 +460,7 @@ cpu_tb_exec(CPUState *cpu, TranslationBlock *itb, int *tb_exit)
     qemu_thread_jit_execute();
     ret = tcg_qemu_tb_exec(env, tb_ptr);
     cpu->can_do_io = 1;
+    plugin_gen_disable_mem_helpers();
     /*
      * TODO: Delay swapping back to the read-write region of the TB
      * until we actually need to modify the TB.  The read-only copy,
@@ -526,7 +528,6 @@ static void cpu_exec_exit(CPUState *cpu)
     if (cc->tcg_ops->cpu_exec_exit) {
         cc->tcg_ops->cpu_exec_exit(cpu);
     }
-    QEMU_PLUGIN_ASSERT(cpu->plugin_mem_cbs == NULL);
 }
 
 void cpu_exec_step_atomic(CPUState *cpu)
@@ -1004,7 +1005,6 @@ cpu_exec_loop(CPUState *cpu, SyncClocks *sc)
 
             cpu_loop_exec_tb(cpu, tb, pc, &last_tb, &tb_exit);
 
-            QEMU_PLUGIN_ASSERT(cpu->plugin_mem_cbs == NULL);
             /* Try to align the host and virtual clocks
                if the guest is in advance */
             align_clocks(sc, cpu);
-- 
2.39.2

Re: [PATCH 07/11] tcg: Clear plugin_mem_cbs on TB exit

[Richard Henderson]

On 3/10/23 02:31, Alex Bennée wrote:
> From: Richard Henderson <richard.henderson@linaro.org>
> 
> Do this in cpu_tb_exec (normal exit) and cpu_loop_exit (exception),
> adjacent to where we reset can_do_io.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1381
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> Message-Id: <20230301024737.1210851-2-richard.henderson@linaro.org>
> [AJB: use plugin_gen_disable_mem_helpers()]
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

This is missing Emilio's comment to remove the two existing calls to 
qemu_plugin_disable_mem_helpers which become duplicate with this change.


r~

> ---
>   accel/tcg/cpu-exec-common.c | 3 +++
>   accel/tcg/cpu-exec.c        | 4 ++--
>   2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/accel/tcg/cpu-exec-common.c b/accel/tcg/cpu-exec-common.c
> index c7bc8c6efa..e5847e9731 100644
> --- a/accel/tcg/cpu-exec-common.c
> +++ b/accel/tcg/cpu-exec-common.c
> @@ -21,6 +21,7 @@
>   #include "sysemu/cpus.h"
>   #include "sysemu/tcg.h"
>   #include "exec/exec-all.h"
> +#include "exec/plugin-gen.h"
>   
>   bool tcg_allowed;
>   
> @@ -65,6 +66,8 @@ void cpu_loop_exit(CPUState *cpu)
>   {
>       /* Undo the setting in cpu_tb_exec.  */
>       cpu->can_do_io = 1;
> +    /* Undo any setting in generated code. */
> +    plugin_gen_disable_mem_helpers();
>       siglongjmp(cpu->jmp_env, 1);
>   }
>   
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 56aaf58b9d..e8a48dbd93 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -39,6 +39,7 @@
>   #include "exec/replay-core.h"
>   #include "sysemu/tcg.h"
>   #include "exec/helper-proto.h"
> +#include "exec/plugin-gen.h"
>   #include "tb-jmp-cache.h"
>   #include "tb-hash.h"
>   #include "tb-context.h"
> @@ -459,6 +460,7 @@ cpu_tb_exec(CPUState *cpu, TranslationBlock *itb, int *tb_exit)
>       qemu_thread_jit_execute();
>       ret = tcg_qemu_tb_exec(env, tb_ptr);
>       cpu->can_do_io = 1;
> +    plugin_gen_disable_mem_helpers();
>       /*
>        * TODO: Delay swapping back to the read-write region of the TB
>        * until we actually need to modify the TB.  The read-only copy,
> @@ -526,7 +528,6 @@ static void cpu_exec_exit(CPUState *cpu)
>       if (cc->tcg_ops->cpu_exec_exit) {
>           cc->tcg_ops->cpu_exec_exit(cpu);
>       }
> -    QEMU_PLUGIN_ASSERT(cpu->plugin_mem_cbs == NULL);
>   }
>   
>   void cpu_exec_step_atomic(CPUState *cpu)
> @@ -1004,7 +1005,6 @@ cpu_exec_loop(CPUState *cpu, SyncClocks *sc)
>   
>               cpu_loop_exec_tb(cpu, tb, pc, &last_tb, &tb_exit);
>   
> -            QEMU_PLUGIN_ASSERT(cpu->plugin_mem_cbs == NULL);
>               /* Try to align the host and virtual clocks
>                  if the guest is in advance */
>               align_clocks(sc, cpu);

Re: [PATCH 07/11] tcg: Clear plugin_mem_cbs on TB exit

[Richard Henderson]

On 3/10/23 09:39, Richard Henderson wrote:
> +    /* Undo any setting in generated code. */
> +    plugin_gen_disable_mem_helpers();

Oh!  And this is the wrong function.  Should be qemu_plugin_disable_mem_helpers.


r~

Re: [PATCH 07/11] tcg: Clear plugin_mem_cbs on TB exit

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 3/10/23 09:39, Richard Henderson wrote:
>> +    /* Undo any setting in generated code. */
>> +    plugin_gen_disable_mem_helpers();
>
> Oh!  And this is the wrong function.  Should be
> qemu_plugin_disable_mem_helpers.

Did I miss a newer version of the patches?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 07/11] tcg: Clear plugin_mem_cbs on TB exit

[Richard Henderson]

On 3/10/23 09:56, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> On 3/10/23 09:39, Richard Henderson wrote:
>>> +    /* Undo any setting in generated code. */
>>> +    plugin_gen_disable_mem_helpers();
>>
>> Oh!  And this is the wrong function.  Should be
>> qemu_plugin_disable_mem_helpers.
> 
> Did I miss a newer version of the patches?

No, I never posted one, since you already said you picked it up with the requested changes.


r~

[PATCH 08/11] tests/tcg: add some help output for running individual tests

[Alex Bennée]

So you can do:

  cd tests/tcg/aarch64-linux-user
  make -f ../Makefile.target help

To see the list of tests. You can then run each one individually.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/Makefile.target | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
index a3b0aaf8af..8318caf924 100644
--- a/tests/tcg/Makefile.target
+++ b/tests/tcg/Makefile.target
@@ -201,3 +201,10 @@ clean:
 
 distclean:
 	rm -f config-cc.mak config-target.mak ../config-$(TARGET).mak
+
+.PHONY: help
+help:
+	@echo "TCG tests help $(TARGET_NAME)"
+	@echo "Built with $(CC)"
+	@echo "Available tests:"
+	@$(foreach t,$(RUN_TESTS),echo "  $t";)
-- 
2.39.2

[PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Alex Bennée]

You need a very new gdb to be able to run with pauth support otherwise
your likely to hit asserts and aborts. Disable pauth for now until we
can properly probe support in gdb.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/aarch64/Makefile.target | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 9e91a20b0d..8ffde3b0ed 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -84,6 +84,8 @@ TESTS += sha512-vector
 ifeq ($(HOST_GDB_SUPPORTS_ARCH),y)
 GDB_SCRIPT=$(SRC_PATH)/tests/guest-debug/run-test.py
 
+run-gdbstub-%: QEMU_OPTS=-cpu max,pauth=off
+
 run-gdbstub-sysregs: sysregs
 	$(call run-test, $@, $(GDB_SCRIPT) \
 		--gdb $(HAVE_GDB_BIN) \
-- 
2.39.2

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Richard Henderson]

On 3/10/23 02:31, Alex Bennée wrote:
> You need a very new gdb to be able to run with pauth support otherwise
> your likely to hit asserts and aborts. Disable pauth for now until we
> can properly probe support in gdb.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   tests/tcg/aarch64/Makefile.target | 2 ++
>   1 file changed, 2 insertions(+)

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

r~

> 
> diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
> index 9e91a20b0d..8ffde3b0ed 100644
> --- a/tests/tcg/aarch64/Makefile.target
> +++ b/tests/tcg/aarch64/Makefile.target
> @@ -84,6 +84,8 @@ TESTS += sha512-vector
>   ifeq ($(HOST_GDB_SUPPORTS_ARCH),y)
>   GDB_SCRIPT=$(SRC_PATH)/tests/guest-debug/run-test.py
>   
> +run-gdbstub-%: QEMU_OPTS=-cpu max,pauth=off
> +
>   run-gdbstub-sysregs: sysregs
>   	$(call run-test, $@, $(GDB_SCRIPT) \
>   		--gdb $(HAVE_GDB_BIN) \

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Peter Maydell]

On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> You need a very new gdb to be able to run with pauth support otherwise
> your likely to hit asserts and aborts. Disable pauth for now until we
> can properly probe support in gdb.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

If it makes gdb fall over, then shouldn't we be disabling
the pauth gdbstub stuff entirely ? Otherwise even if our
tests are fine our users will not be...

-- PMM

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Fabiano Rosas]

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

> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> You need a very new gdb to be able to run with pauth support otherwise
>> your likely to hit asserts and aborts. Disable pauth for now until we
>> can properly probe support in gdb.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> If it makes gdb fall over, then shouldn't we be disabling
> the pauth gdbstub stuff entirely ? Otherwise even if our
> tests are fine our users will not be...
>

Have you seem my message on IRC about changing the feature name in the
XML? I think the issue is that we're putting the .xml in a "namespace"
where GDB expects to only find stuff which it has code to
support. Changing from "org.gnu.gdb.aarch64.pauth" to
"org.qemu.aarch64.pauth" made it stop crashing and I can read the
registers just fine.

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Peter Maydell]

On Fri, 10 Mar 2023 at 18:00, Fabiano Rosas <farosas@suse.de> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> You need a very new gdb to be able to run with pauth support otherwise
> >> your likely to hit asserts and aborts. Disable pauth for now until we
> >> can properly probe support in gdb.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >
> > If it makes gdb fall over, then shouldn't we be disabling
> > the pauth gdbstub stuff entirely ? Otherwise even if our
> > tests are fine our users will not be...
> >
>
> Have you seem my message on IRC about changing the feature name in the
> XML? I think the issue is that we're putting the .xml in a "namespace"
> where GDB expects to only find stuff which it has code to
> support. Changing from "org.gnu.gdb.aarch64.pauth" to
> "org.qemu.aarch64.pauth" made it stop crashing and I can read the
> registers just fine.

But then presumably a pauth-aware GDB won't actually know
the values it needs to be able to convert between with-PAC
and without-PAC addresses for backtracing?

Luis, how is this intended to work? Is there some way the
stub can check with the gdb that's connected whether the
gdb is able to cope with the pauth XML, so it can avoid
sending it to a gdb that is going to crash if it sees it ?

thanks
-- PMM

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Fabiano Rosas]

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

> On Fri, 10 Mar 2023 at 18:00, Fabiano Rosas <farosas@suse.de> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>> > On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>> >>
>> >> You need a very new gdb to be able to run with pauth support otherwise
>> >> your likely to hit asserts and aborts. Disable pauth for now until we
>> >> can properly probe support in gdb.
>> >>
>> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> >
>> > If it makes gdb fall over, then shouldn't we be disabling
>> > the pauth gdbstub stuff entirely ? Otherwise even if our
>> > tests are fine our users will not be...
>> >
>>
>> Have you seem my message on IRC about changing the feature name in the
>> XML? I think the issue is that we're putting the .xml in a "namespace"
>> where GDB expects to only find stuff which it has code to
>> support. Changing from "org.gnu.gdb.aarch64.pauth" to
>> "org.qemu.aarch64.pauth" made it stop crashing and I can read the
>> registers just fine.
>
> But then presumably a pauth-aware GDB won't actually know
> the values it needs to be able to convert between with-PAC
> and without-PAC addresses for backtracing?
>

Good question. Although that feels to me more like a GDB feature. If we
don't break it even worse by doing that, the QEMU side which is more
about reading the registers should be fine. Note that we already have
other .xml files using a .qemu namespace in the codebase. As I
understand it, gdb simply treats these as extra registers not tied to
any specific feature.

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Luis Machado]

On 3/10/23 18:07, Peter Maydell wrote:
> On Fri, 10 Mar 2023 at 18:00, Fabiano Rosas <farosas@suse.de> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>
>>>> You need a very new gdb to be able to run with pauth support otherwise
>>>> your likely to hit asserts and aborts. Disable pauth for now until we
>>>> can properly probe support in gdb.
>>>>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>
>>> If it makes gdb fall over, then shouldn't we be disabling
>>> the pauth gdbstub stuff entirely ? Otherwise even if our
>>> tests are fine our users will not be...
>>>
>>
>> Have you seem my message on IRC about changing the feature name in the
>> XML? I think the issue is that we're putting the .xml in a "namespace"
>> where GDB expects to only find stuff which it has code to
>> support. Changing from "org.gnu.gdb.aarch64.pauth" to
>> "org.qemu.aarch64.pauth" made it stop crashing and I can read the
>> registers just fine.

It might be a better option to emit a pauth feature in the qemu namespace to dodge the crashing bug from older
gdb's (a latent pauth-related bug in gdb that is triggered by having gdb identify that a target supports
pauth and at the same time having a target description containing system registers gdb doesn't
care about).

>
> But then presumably a pauth-aware GDB won't actually know
> the values it needs to be able to convert between with-PAC
> and without-PAC addresses for backtracing?
>
> Luis, how is this intended to work? Is there some way the
> stub can check with the gdb that's connected whether the
> gdb is able to cope with the pauth XML, so it can avoid
> sending it to a gdb that is going to crash if it sees it ?

There isn't a probing mechanism unfortunately, and gdb isn't supposed to crash in this case.

With the changes from commit 6d0020873deb2f2c4e0965dc2ebf227bc1db3140, gdb now unmasks signed
addresses using the additional pauth registers. If gdb doesn't detect the pauth feature, it will
still mask out the top bits using a default mask of 0xff80000000000000.

While that should be enough for user addresses, it won't help with "kernel" addresses (when the VA select bit is 1).

To dodge the crashing bug of older gdb's, I can adjust gdb to also look for the pauth registers in the qemu namespace and
document that accordingly.

>
> thanks
> -- PMM

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Alex Bennée]

(adding some more gdb types to CC)

Fabiano Rosas <farosas@suse.de> writes:

> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>> You need a very new gdb to be able to run with pauth support otherwise
>>> your likely to hit asserts and aborts. Disable pauth for now until we
>>> can properly probe support in gdb.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>
>> If it makes gdb fall over, then shouldn't we be disabling
>> the pauth gdbstub stuff entirely ? Otherwise even if our
>> tests are fine our users will not be...
>>
>
> Have you seem my message on IRC about changing the feature name in the
> XML? I think the issue is that we're putting the .xml in a "namespace"
> where GDB expects to only find stuff which it has code to
> support. Changing from "org.gnu.gdb.aarch64.pauth" to
> "org.qemu.aarch64.pauth" made it stop crashing and I can read the
> registers just fine.

That would work, although I would prefer to probe support so we can use
the official namespace. We went through something similar with SVE
until:

  797920b952 (target/arm: use official org.gnu.gdb.aarch64.sve layout for registers)

which required:

  b1863ccc95 (configure: gate our use of GDB to 8.3.1 or above)

Since then we've introduced:

 ./scripts/probe-gdb-support.py

which given the runes to check for pauth support in gdb could expose a
symbol and we get the best of both worlds. Of course if this keeps
happening we could throw up our hands and just use custom XML for all
the extra register sets.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Peter Maydell]

On Fri, 10 Mar 2023 at 18:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> (adding some more gdb types to CC)
>
> Fabiano Rosas <farosas@suse.de> writes:
>
> > Peter Maydell <peter.maydell@linaro.org> writes:
> >
> >> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>>
> >>> You need a very new gdb to be able to run with pauth support otherwise
> >>> your likely to hit asserts and aborts. Disable pauth for now until we
> >>> can properly probe support in gdb.
> >>>
> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >>
> >> If it makes gdb fall over, then shouldn't we be disabling
> >> the pauth gdbstub stuff entirely ? Otherwise even if our
> >> tests are fine our users will not be...
> >>
> >
> > Have you seem my message on IRC about changing the feature name in the
> > XML? I think the issue is that we're putting the .xml in a "namespace"
> > where GDB expects to only find stuff which it has code to
> > support. Changing from "org.gnu.gdb.aarch64.pauth" to
> > "org.qemu.aarch64.pauth" made it stop crashing and I can read the
> > registers just fine.
>
> That would work, although I would prefer to probe support so we can use
> the official namespace.

I don't think there's a way to probe for this problem. I spoke
to Luis about this, and apparently it's a bug in how gdb handles
the pauth XML description (fixed in gdb commit 1ba3a3222039eb25).
A gdb without any pauth support at all will be fine; a gdb with
the bug will report that it has pauth support but will crash
if you feed it the whole set of XML that QEMU has; a gdb
with the bug fixed will also report pauth support but won't crash.
(The bug only manifests if the full XML includes registers that GDB
doesn't care about, like the system registers; if the stub sends
only registers GDB knows about then it won't crash.)

Luis and I came up with two options:

(1) leave QEMU outputting the pauth xml as-is, and tell people
whose gdb 12 crashes that they should upgrade to a newer gdb

(2) make QEMU output the pauth info under a different XML namespace,
and tell people who need backtraces when pauth is enabled
that they should upgrade to a newer gdb

Neither of these feel great, but on balance I guess 2 is better?

Luis: I think that rather than doing (2) with a QEMU namespace,
we should define a gdb namespace for this. That makes it clear
that this is still a gdb-upstream-sanctioned way of exposing
the pauth registers.

thanks
-- PMM

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Luis Machado]

On 3/13/23 11:22, Peter Maydell via Gdb wrote:
> On Fri, 10 Mar 2023 at 18:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> (adding some more gdb types to CC)
>>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>
>>>> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>>
>>>>> You need a very new gdb to be able to run with pauth support otherwise
>>>>> your likely to hit asserts and aborts. Disable pauth for now until we
>>>>> can properly probe support in gdb.
>>>>>
>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>>
>>>> If it makes gdb fall over, then shouldn't we be disabling
>>>> the pauth gdbstub stuff entirely ? Otherwise even if our
>>>> tests are fine our users will not be...
>>>>
>>>
>>> Have you seem my message on IRC about changing the feature name in the
>>> XML? I think the issue is that we're putting the .xml in a "namespace"
>>> where GDB expects to only find stuff which it has code to
>>> support. Changing from "org.gnu.gdb.aarch64.pauth" to
>>> "org.qemu.aarch64.pauth" made it stop crashing and I can read the
>>> registers just fine.
>>
>> That would work, although I would prefer to probe support so we can use
>> the official namespace.
>
> I don't think there's a way to probe for this problem. I spoke
> to Luis about this, and apparently it's a bug in how gdb handles
> the pauth XML description (fixed in gdb commit 1ba3a3222039eb25).
> A gdb without any pauth support at all will be fine; a gdb with
> the bug will report that it has pauth support but will crash
> if you feed it the whole set of XML that QEMU has; a gdb
> with the bug fixed will also report pauth support but won't crash.
> (The bug only manifests if the full XML includes registers that GDB
> doesn't care about, like the system registers; if the stub sends
> only registers GDB knows about then it won't crash.)
>
> Luis and I came up with two options:
>
> (1) leave QEMU outputting the pauth xml as-is, and tell people
> whose gdb 12 crashes that they should upgrade to a newer gdb
>
> (2) make QEMU output the pauth info under a different XML namespace,
> and tell people who need backtraces when pauth is enabled
> that they should upgrade to a newer gdb
>
> Neither of these feel great, but on balance I guess 2 is better?
>
> Luis: I think that rather than doing (2) with a QEMU namespace,
> we should define a gdb namespace for this. That makes it clear
> that this is still a gdb-upstream-sanctioned way of exposing
> the pauth registers.

That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.

We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop using the original
"org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant pauth_v2.

FYI, I've pushed a better documentation for the arm/aarch64 xml descriptions here:

https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=d7001b29e9f256dfc60acb481d9df8f91f2ee623
https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=e0994165d1b8469dfc27b09b62ac74862d535812

>
> thanks
> -- PMM

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Richard Henderson]

On 3/13/23 04:44, Luis Machado wrote:
>> Luis: I think that rather than doing (2) with a QEMU namespace,
>> we should define a gdb namespace for this. That makes it clear
>> that this is still a gdb-upstream-sanctioned way of exposing
>> the pauth registers.
> 
> That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.
> 
> We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop 
> using the original
> "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant 
> pauth_v2.

What if we leave the original two registers, pauth_[cd]mask, in org.gnu.gdb.aarch64.pauth 
and move the new *_high registers into a different feature?  That would maximize the set 
of gdb version for which the original user-only support is functional.


r~

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Peter Maydell]

On Mon, 13 Mar 2023 at 19:21, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/13/23 04:44, Luis Machado wrote:
> >> Luis: I think that rather than doing (2) with a QEMU namespace,
> >> we should define a gdb namespace for this. That makes it clear
> >> that this is still a gdb-upstream-sanctioned way of exposing
> >> the pauth registers.
> >
> > That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.
> >
> > We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop
> > using the original
> > "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant
> > pauth_v2.
>
> What if we leave the original two registers, pauth_[cd]mask, in org.gnu.gdb.aarch64.pauth
> and move the new *_high registers into a different feature?  That would maximize the set
> of gdb version for which the original user-only support is functional.

If that avoids the gdb crash, sure. But I had the impression from
Luis' description of it that that would not help (i.e. that it was
the not-used-by-gdb registers in other XML sections like sysregs
that resulted in it getting confused about the register number
for its internal pauth-related register).

thanks
-- PMM

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Luis Machado]

On 3/13/23 19:21, Richard Henderson wrote:
> On 3/13/23 04:44, Luis Machado wrote:
>>> Luis: I think that rather than doing (2) with a QEMU namespace,
>>> we should define a gdb namespace for this. That makes it clear
>>> that this is still a gdb-upstream-sanctioned way of exposing
>>> the pauth registers.
>>
>> That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.
>>
>> We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop using the original
>> "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant pauth_v2.
>
> What if we leave the original two registers, pauth_[cd]mask, in org.gnu.gdb.aarch64.pauth and move the new *_high registers into a different feature?  That would maximize the set of gdb version for which the original user-only support is functional.

 From what I recall from the gdb bug, I don't think that will help. gdb will detect pauth support, will add the ra_sign_state pseudo-register internally with the incorrect numbering and will also see the additional system registers from QEMU, resulting in a crash.

>
>
> r~
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Luis Machado]

Hi,

On 3/13/23 11:44, Luis Machado wrote:
> On 3/13/23 11:22, Peter Maydell via Gdb wrote:
>> On Fri, 10 Mar 2023 at 18:20, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>
>>>
>>> (adding some more gdb types to CC)
>>>
>>> Fabiano Rosas <farosas@suse.de> writes:
>>>
>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>
>>>>> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>>>>>>
>>>>>> You need a very new gdb to be able to run with pauth support otherwise
>>>>>> your likely to hit asserts and aborts. Disable pauth for now until we
>>>>>> can properly probe support in gdb.
>>>>>>
>>>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>>>
>>>>> If it makes gdb fall over, then shouldn't we be disabling
>>>>> the pauth gdbstub stuff entirely ? Otherwise even if our
>>>>> tests are fine our users will not be...
>>>>>
>>>>
>>>> Have you seem my message on IRC about changing the feature name in the
>>>> XML? I think the issue is that we're putting the .xml in a "namespace"
>>>> where GDB expects to only find stuff which it has code to
>>>> support. Changing from "org.gnu.gdb.aarch64.pauth" to
>>>> "org.qemu.aarch64.pauth" made it stop crashing and I can read the
>>>> registers just fine.
>>>
>>> That would work, although I would prefer to probe support so we can use
>>> the official namespace.
>>
>> I don't think there's a way to probe for this problem. I spoke
>> to Luis about this, and apparently it's a bug in how gdb handles
>> the pauth XML description (fixed in gdb commit 1ba3a3222039eb25).
>> A gdb without any pauth support at all will be fine; a gdb with
>> the bug will report that it has pauth support but will crash
>> if you feed it the whole set of XML that QEMU has; a gdb
>> with the bug fixed will also report pauth support but won't crash.
>> (The bug only manifests if the full XML includes registers that GDB
>> doesn't care about, like the system registers; if the stub sends
>> only registers GDB knows about then it won't crash.)
>>
>> Luis and I came up with two options:
>>
>> (1) leave QEMU outputting the pauth xml as-is, and tell people
>> whose gdb 12 crashes that they should upgrade to a newer gdb
>>
>> (2) make QEMU output the pauth info under a different XML namespace,
>> and tell people who need backtraces when pauth is enabled
>> that they should upgrade to a newer gdb
>>
>> Neither of these feel great, but on balance I guess 2 is better?
>>
>> Luis: I think that rather than doing (2) with a QEMU namespace,
>> we should define a gdb namespace for this. That makes it clear
>> that this is still a gdb-upstream-sanctioned way of exposing
>> the pauth registers.
>
> That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.
>
> We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop using the original
> "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant pauth_v2.
>
> FYI, I've pushed a better documentation for the arm/aarch64 xml descriptions here:
>
> https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=d7001b29e9f256dfc60acb481d9df8f91f2ee623
> https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=e0994165d1b8469dfc27b09b62ac74862d535812
>
>>
>> thanks
>> -- PMM
>

Just an update on this. I had a chat with Richard Henderson yesterday, and it might actually be easier and more convenient to backport
fixes to older gdb versions (at least gdb-12 and gdb-11, but gdb-10 and gdb-9 are also affected). This will ensure those won't crash when
they connect to a qemu that advertises the pauth feature.

It also means we won't need qemu-side changes. My understanding is that we're close to the 8.0.0 release, and the code is already in place.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Peter Maydell]

On Wed, 15 Mar 2023 at 09:51, Luis Machado <luis.machado@arm.com> wrote:
> On 3/13/23 11:44, Luis Machado wrote:
> > On 3/13/23 11:22, Peter Maydell via Gdb wrote:
> >> Luis and I came up with two options:
> >>
> >> (1) leave QEMU outputting the pauth xml as-is, and tell people
> >> whose gdb 12 crashes that they should upgrade to a newer gdb
> >>
> >> (2) make QEMU output the pauth info under a different XML namespace,
> >> and tell people who need backtraces when pauth is enabled
> >> that they should upgrade to a newer gdb
> >>
> >> Neither of these feel great, but on balance I guess 2 is better?
> >>
> >> Luis: I think that rather than doing (2) with a QEMU namespace,
> >> we should define a gdb namespace for this. That makes it clear
> >> that this is still a gdb-upstream-sanctioned way of exposing
> >> the pauth registers.
> >
> > That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.
> >
> > We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop using the original
> > "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant pauth_v2.
> >
> > FYI, I've pushed a better documentation for the arm/aarch64 xml descriptions here:
> >
> > https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=d7001b29e9f256dfc60acb481d9df8f91f2ee623
> > https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=e0994165d1b8469dfc27b09b62ac74862d535812

> Just an update on this. I had a chat with Richard Henderson yesterday, and it might actually be easier and more convenient to backport
> fixes to older gdb versions (at least gdb-12 and gdb-11, but gdb-10 and gdb-9 are also affected). This will ensure those won't crash when
> they connect to a qemu that advertises the pauth feature.
>
> It also means we won't need qemu-side changes. My understanding is that we're close to the 8.0.0 release, and the code is already in place.

Having run into this problem in another couple of situations, one of
which involved gdb 10, I think I'm increasingly favouring option
2 here. The affected gdbs seem to be quite widely deployed, and
the bug results in crashes even for users who didn't really
care about pauth. So I'd rather we didn't release a QEMU 8.0
which crashes these affected deployed gdbs.

So:
 (a) if on the gdb side you can define (within the next week) a
suitable new XML name you want QEMU to expose, we can commit a
change to switch to that before we do the 8.0 release
 (b) if that's too tight a timescale, we can commit a patch which
just stops QEMU from exposing the pauth.xml, and we can come up
with a better solution after 8.0 releases

In fact, I think I'm going to submit a patch to do (b) for
now and we can follow up with a patch for (a) if we want.

thanks
-- PMM

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Luis Machado]

On 3/17/23 16:37, Peter Maydell wrote:
> On Wed, 15 Mar 2023 at 09:51, Luis Machado <luis.machado@arm.com> wrote:
>> On 3/13/23 11:44, Luis Machado wrote:
>>> On 3/13/23 11:22, Peter Maydell via Gdb wrote:
>>>> Luis and I came up with two options:
>>>>
>>>> (1) leave QEMU outputting the pauth xml as-is, and tell people
>>>> whose gdb 12 crashes that they should upgrade to a newer gdb
>>>>
>>>> (2) make QEMU output the pauth info under a different XML namespace,
>>>> and tell people who need backtraces when pauth is enabled
>>>> that they should upgrade to a newer gdb
>>>>
>>>> Neither of these feel great, but on balance I guess 2 is better?
>>>>
>>>> Luis: I think that rather than doing (2) with a QEMU namespace,
>>>> we should define a gdb namespace for this. That makes it clear
>>>> that this is still a gdb-upstream-sanctioned way of exposing
>>>> the pauth registers.
>>>
>>> That should be fine as well, and would work to side-step the gdb 12 bug so it doesn't crash.
>>>
>>> We could name the feature "org.gnu.gdb.aarch64.pauth_v2" or somesuch, and slowly stop using the original
>>> "org.gnu.gdb.aarch64.pauth" feature. I can document the requirements for a compliant pauth_v2.
>>>
>>> FYI, I've pushed a better documentation for the arm/aarch64 xml descriptions here:
>>>
>>> https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=d7001b29e9f256dfc60acb481d9df8f91f2ee623
>>> https://sourceware.org/git?p=binutils-gdb.git;a=commit;h=e0994165d1b8469dfc27b09b62ac74862d535812
>
>> Just an update on this. I had a chat with Richard Henderson yesterday, and it might actually be easier and more convenient to backport
>> fixes to older gdb versions (at least gdb-12 and gdb-11, but gdb-10 and gdb-9 are also affected). This will ensure those won't crash when
>> they connect to a qemu that advertises the pauth feature.
>>
>> It also means we won't need qemu-side changes. My understanding is that we're close to the 8.0.0 release, and the code is already in place.
>
> Having run into this problem in another couple of situations, one of
> which involved gdb 10, I think I'm increasingly favouring option
> 2 here. The affected gdbs seem to be quite widely deployed, and
> the bug results in crashes even for users who didn't really
> care about pauth. So I'd rather we didn't release a QEMU 8.0
> which crashes these affected deployed gdbs.
>

Are the affected gdb's packaged by distros? If so, a backport the distros can pick up
will solve this in a quick package update.

If we decide qemu should now emit a different xml for pauth, it will fix the crashes, but it also
means older gdb's (9/10/11/12) will not be able to backtrace properly through pauth-signed frames.

Maybe that's a reasonable drawback for qemu users?

If someone decides to implement a debugging stub that reports pauth (fast models, for example), it will
also crash gdb, so I still plan to do the backport anyway.

> So:
>   (a) if on the gdb side you can define (within the next week) a
> suitable new XML name you want QEMU to expose, we can commit a
> change to switch to that before we do the 8.0 release

pauth_v2 sounds about as good as any other for me.

>   (b) if that's too tight a timescale, we can commit a patch which
> just stops QEMU from exposing the pauth.xml, and we can come up
> with a better solution after 8.0 releases
>
> In fact, I think I'm going to submit a patch to do (b) for
> now and we can follow up with a patch for (a) if we want.
>
> thanks
> -- PMM

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Peter Maydell]

On Fri, 17 Mar 2023 at 16:55, Luis Machado <luis.machado@arm.com> wrote:
> On 3/17/23 16:37, Peter Maydell wrote:
> > Having run into this problem in another couple of situations, one of
> > which involved gdb 10, I think I'm increasingly favouring option
> > 2 here. The affected gdbs seem to be quite widely deployed, and
> > the bug results in crashes even for users who didn't really
> > care about pauth. So I'd rather we didn't release a QEMU 8.0
> > which crashes these affected deployed gdbs.
> >
>
> Are the affected gdb's packaged by distros? If so, a backport the distros can pick up
> will solve this in a quick package update.

Yes, it's exactly because these gdbs are distro-packaged
that I don't want QEMU to make them crash. I think it's
going to take a long time for the fix to go into gdb branches
and gdb to make a point release and distros to pick up that
point release, and in the meantime that's a lot of crashing
gdb bug reports that we're going to have to field.

> If we decide qemu should now emit a different xml for pauth, it will fix the crashes, but it also
> means older gdb's (9/10/11/12) will not be able to backtrace properly through pauth-signed frames.
>
> Maybe that's a reasonable drawback for qemu users?

"No backtrace through pauth frames" is the situation we've
been in ever since we implemented pauth in 2019, so I think
that's fine. It's not regressing something that used to work.

> If someone decides to implement a debugging stub that reports pauth (fast models, for example), it will
> also crash gdb, so I still plan to do the backport anyway.

If you're backporting the fix, you could also backport the
(hopefully tiny) change that says "treat pauth_v2 the same
way we do pauth", and then users with an updated older
gdb will also get working backtraces.

thanks
-- PMM

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Luis Machado]

On 3/17/23 17:07, Peter Maydell wrote:
> On Fri, 17 Mar 2023 at 16:55, Luis Machado <luis.machado@arm.com> wrote:
>> On 3/17/23 16:37, Peter Maydell wrote:
>>> Having run into this problem in another couple of situations, one of
>>> which involved gdb 10, I think I'm increasingly favouring option
>>> 2 here. The affected gdbs seem to be quite widely deployed, and
>>> the bug results in crashes even for users who didn't really
>>> care about pauth. So I'd rather we didn't release a QEMU 8.0
>>> which crashes these affected deployed gdbs.
>>>
>>
>> Are the affected gdb's packaged by distros? If so, a backport the distros can pick up
>> will solve this in a quick package update.
>
> Yes, it's exactly because these gdbs are distro-packaged
> that I don't want QEMU to make them crash. I think it's
> going to take a long time for the fix to go into gdb branches
> and gdb to make a point release and distros to pick up that
> point release, and in the meantime that's a lot of crashing
> gdb bug reports that we're going to have to field.

Just to clarify, there won't be any point releases for gdb's 9/10/11/12.  We might have a bug fix
release for gdb 13 though (which isn't affected).

>
>> If we decide qemu should now emit a different xml for pauth, it will fix the crashes, but it also
>> means older gdb's (9/10/11/12) will not be able to backtrace properly through pauth-signed frames.
>>
>> Maybe that's a reasonable drawback for qemu users?
>
> "No backtrace through pauth frames" is the situation we've
> been in ever since we implemented pauth in 2019, so I think
> that's fine. It's not regressing something that used to work.
>

Fair enough.

>> If someone decides to implement a debugging stub that reports pauth (fast models, for example), it will
>> also crash gdb, so I still plan to do the backport anyway.
>
> If you're backporting the fix, you could also backport the
> (hopefully tiny) change that says "treat pauth_v2 the same
> way we do pauth", and then users with an updated older
> gdb will also get working backtraces.

I can negotiate that as well, though it borders being a new feature.

>
> thanks
> -- PMM

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Luis Machado]

On 3/17/23 17:12, Luis Machado wrote:
> On 3/17/23 17:07, Peter Maydell wrote:
>> On Fri, 17 Mar 2023 at 16:55, Luis Machado <luis.machado@arm.com> wrote:
>>> On 3/17/23 16:37, Peter Maydell wrote:
>>>> Having run into this problem in another couple of situations, one of
>>>> which involved gdb 10, I think I'm increasingly favouring option
>>>> 2 here. The affected gdbs seem to be quite widely deployed, and
>>>> the bug results in crashes even for users who didn't really
>>>> care about pauth. So I'd rather we didn't release a QEMU 8.0
>>>> which crashes these affected deployed gdbs.
>>>>
>>>
>>> Are the affected gdb's packaged by distros? If so, a backport the distros can pick up
>>> will solve this in a quick package update.
>>
>> Yes, it's exactly because these gdbs are distro-packaged
>> that I don't want QEMU to make them crash. I think it's
>> going to take a long time for the fix to go into gdb branches
>> and gdb to make a point release and distros to pick up that
>> point release, and in the meantime that's a lot of crashing
>> gdb bug reports that we're going to have to field.
>
> Just to clarify, there won't be any point releases for gdb's 9/10/11/12.  We might have a bug fix
> release for gdb 13 though (which isn't affected).
>

Just to complement, my plan is to make the backports available (via stable branch commits) so distro package
maintainers can pick those up easily. No new releases will be made for older gdb's, so the package maintainers
can pick the backport up as soon as they are pushed. There won't be waiting for a new release of gdb.

>>
>>> If we decide qemu should now emit a different xml for pauth, it will fix the crashes, but it also
>>> means older gdb's (9/10/11/12) will not be able to backtrace properly through pauth-signed frames.
>>>
>>> Maybe that's a reasonable drawback for qemu users?
>>
>> "No backtrace through pauth frames" is the situation we've
>> been in ever since we implemented pauth in 2019, so I think
>> that's fine. It's not regressing something that used to work.
>>
>
> Fair enough.
>
>>> If someone decides to implement a debugging stub that reports pauth (fast models, for example), it will
>>> also crash gdb, so I still plan to do the backport anyway.
>>
>> If you're backporting the fix, you could also backport the
>> (hopefully tiny) change that says "treat pauth_v2 the same
>> way we do pauth", and then users with an updated older
>> gdb will also get working backtraces.
>
> I can negotiate that as well, though it borders being a new feature.
>
>>
>> thanks
>> -- PMM
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 09/11] tests/tcg: disable pauth for aarch64 gdb tests

[Richard Henderson]

On 3/10/23 09:47, Peter Maydell wrote:
> On Fri, 10 Mar 2023 at 10:31, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> You need a very new gdb to be able to run with pauth support otherwise
>> your likely to hit asserts and aborts. Disable pauth for now until we
>> can properly probe support in gdb.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> If it makes gdb fall over, then shouldn't we be disabling
> the pauth gdbstub stuff entirely ? Otherwise even if our
> tests are fine our users will not be...

It is, annoyingly, a bug in gdb 12 alone.

Before gdb 12, the pauth extension isn't recognized and so it gets treated as non-special 
registers.  From gdb 13, whatever lead to the internal_error() is fixed and the extension 
works swimmingly.


r~

[PATCH 10/11] include/exec: fix kerneldoc definition

[Alex Bennée]

The kerneldoc processor complains about the mismatched variable name.
Fix it.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/memory.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 6fa0b071f0..15ade918ba 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1738,7 +1738,7 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier,
  *
  * @notifier: the notifier to be notified
  */
-void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *n);
+void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *notifier);
 
 
 /**
-- 
2.39.2

Re: [PATCH 10/11] include/exec: fix kerneldoc definition

[Philippe Mathieu-Daudé]

On 10/3/23 11:31, Alex Bennée wrote:
> The kerneldoc processor complains about the mismatched variable name.
> Fix it.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/exec/memory.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH 10/11] include/exec: fix kerneldoc definition

[Thomas Huth]

On 10/03/2023 11.31, Alex Bennée wrote:
> The kerneldoc processor complains about the mismatched variable name.
> Fix it.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/exec/memory.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 6fa0b071f0..15ade918ba 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1738,7 +1738,7 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier,
>    *
>    * @notifier: the notifier to be notified
>    */
> -void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *n);
> +void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *notifier);

I also keep running into this problem ... I wonder whether we should run 
sphinx with "-W" to turn warnings into errors when configure has been run 
with --enable-werror ...?

Anyway, for this patch here:

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

Re: [PATCH 10/11] include/exec: fix kerneldoc definition

[Peter Maydell]

On Mon, 13 Mar 2023 at 17:00, Thomas Huth <thuth@redhat.com> wrote:
>
> On 10/03/2023 11.31, Alex Bennée wrote:
> > The kerneldoc processor complains about the mismatched variable name.
> > Fix it.
> >
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> > ---
> >   include/exec/memory.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 6fa0b071f0..15ade918ba 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1738,7 +1738,7 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier,
> >    *
> >    * @notifier: the notifier to be notified
> >    */
> > -void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *n);
> > +void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *notifier);
>
> I also keep running into this problem ... I wonder whether we should run
> sphinx with "-W" to turn warnings into errors when configure has been run
> with --enable-werror ...?

We certainly try to do that: docs/meson.build says:

  # If we're making warnings fatal, apply this to Sphinx runs as well
  if get_option('werror')
    SPHINX_ARGS += [ '-W' ]
  endif

Has that broken ?

-- PMM

Re: [PATCH 10/11] include/exec: fix kerneldoc definition

[Thomas Huth]

On 13/03/2023 18.03, Peter Maydell wrote:
> On Mon, 13 Mar 2023 at 17:00, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 10/03/2023 11.31, Alex Bennée wrote:
>>> The kerneldoc processor complains about the mismatched variable name.
>>> Fix it.
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> ---
>>>    include/exec/memory.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>> index 6fa0b071f0..15ade918ba 100644
>>> --- a/include/exec/memory.h
>>> +++ b/include/exec/memory.h
>>> @@ -1738,7 +1738,7 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier,
>>>     *
>>>     * @notifier: the notifier to be notified
>>>     */
>>> -void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *n);
>>> +void memory_region_unmap_iommu_notifier_range(IOMMUNotifier *notifier);
>>
>> I also keep running into this problem ... I wonder whether we should run
>> sphinx with "-W" to turn warnings into errors when configure has been run
>> with --enable-werror ...?
> 
> We certainly try to do that: docs/meson.build says:
> 
>    # If we're making warnings fatal, apply this to Sphinx runs as well
>    if get_option('werror')
>      SPHINX_ARGS += [ '-W' ]
>    endif
> 
> Has that broken ?

It apparently does not work in our CI, see e.g.:

https://gitlab.com/qemu-project/qemu/-/jobs/3922732898#L1420

... there is a warning here, but the job succeeded happily.

  Thomas

Re: [PATCH 10/11] include/exec: fix kerneldoc definition

[Peter Maydell]

On Mon, 13 Mar 2023 at 17:14, Thomas Huth <thuth@redhat.com> wrote:
>
> On 13/03/2023 18.03, Peter Maydell wrote:
> > On Mon, 13 Mar 2023 at 17:00, Thomas Huth <thuth@redhat.com> wrote:
> >> I also keep running into this problem ... I wonder whether we should run
> >> sphinx with "-W" to turn warnings into errors when configure has been run
> >> with --enable-werror ...?
> >
> > We certainly try to do that: docs/meson.build says:
> >
> >    # If we're making warnings fatal, apply this to Sphinx runs as well
> >    if get_option('werror')
> >      SPHINX_ARGS += [ '-W' ]
> >    endif
> >
> > Has that broken ?
>
> It apparently does not work in our CI, see e.g.:
>
> https://gitlab.com/qemu-project/qemu/-/jobs/3922732898#L1420
>
> ... there is a warning here, but the job succeeded happily.

Specifically:

/builds/qemu-project/qemu/docs/../include/exec/memory.h:1741: warning:
Function parameter or member 'n' not described in
'memory_region_unmap_iommu_notifier_range'
/builds/qemu-project/qemu/docs/../include/exec/memory.h:1741: warning:
Excess function parameter 'notifier' description in
'memory_region_unmap_iommu_notifier_range'
ninja: bad depfile: multiple outputs:
/builds/qemu-project/qemu/docs/devel/secure-coding-practices.rst !=
docs/docs.stamp

Also, what's that 'bad depfile' warning from ninja about ??

I looked at the build.ninja file (which you can fish out of
the artifacts for this build), and it shows that we are
passing -W to sphinx-build:

build docs/docs.stamp: CUSTOM_COMMAND_DEP ../docs/conf.py |
/usr/bin/env /usr/bin/sphinx-build
 DEPFILE = docs/docs.d
 DEPFILE_UNQUOTED = docs/docs.d
 COMMAND = /usr/bin/env CONFDIR=etc/qemu /usr/bin/sphinx-build -q -W
-Dversion=7.2.50 -Drelease= -Ddepfile=docs/docs.d
-Ddepfile_stamp=docs/docs.stamp -b html -d
/builds/qemu-project/qemu/build/docs/manual.p
/builds/qemu-project/qemu/docs
/builds/qemu-project/qemu/build/docs/manual
 description = Generating$ docs/QEMU$ manual$ with$ a$ custom$ command

So I think the problem here is not with Sphinx, but with the
kernel-doc script. That script has an option "-Werror" which
turns its warnings into errors, but our Sphinx extension
docs/sphinx/kerneldoc.py does not set it. I think we need to
have the extension say "if Sphinx was run with -W then
pass this flag along" (hopefully Sphinx lets us find out...)

thanks
-- PMM

Re: [PATCH 10/11] include/exec: fix kerneldoc definition

[Peter Maydell]

On Mon, 13 Mar 2023 at 17:30, Peter Maydell <peter.maydell@linaro.org> wrote:
> So I think the problem here is not with Sphinx, but with the
> kernel-doc script. That script has an option "-Werror" which
> turns its warnings into errors, but our Sphinx extension
> docs/sphinx/kerneldoc.py does not set it. I think we need to
> have the extension say "if Sphinx was run with -W then
> pass this flag along" (hopefully Sphinx lets us find out...)

This works:

--- a/docs/sphinx/kerneldoc.py
+++ b/docs/sphinx/kerneldoc.py
@@ -74,6 +74,10 @@ def run(self):
         # Sphinx versions
         cmd += ['-sphinx-version', sphinx.__version__]

+        # Pass through the warnings-as-errors flag if appropriate
+        if env.app.warningiserror:
+            cmd += ['-Werror']
+
         filename = env.config.kerneldoc_srctree + '/' + self.arguments[0]
         export_file_patterns = []


but I think it's prodding undocumented Sphinx internals, so
I'm going to check whether there's a better way to do this.
It might be more robust to have meson create a commandline
with a -Dkerneldoc_werror option that we then pick up in
the extension code, rather than trying to find out whether
-W was passed.

-- PMM

[PATCH 11/11] tests/avocado: don't use tags to define drive

[Alex Bennée]

We are abusing the avocado tags which are intended to provide test
selection metadata to provide parameters to our test. This works OK up
until the point you need to have ,'s in the field as this is the tag
separator character which is the case for a number of the drive
parameters. Fix this by making drive a parameter to the common helper
function.

Fixes: 267fe57c23 (tests: add tuxrun baseline test to avocado)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: David Woodhouse <dwmw2@infradead.org>
---
 tests/avocado/tuxrun_baselines.py | 60 +++++++++++++------------------
 1 file changed, 24 insertions(+), 36 deletions(-)

diff --git a/tests/avocado/tuxrun_baselines.py b/tests/avocado/tuxrun_baselines.py
index 30aaefc1d3..c3fb67f5dc 100644
--- a/tests/avocado/tuxrun_baselines.py
+++ b/tests/avocado/tuxrun_baselines.py
@@ -67,9 +67,6 @@ def setUp(self):
         # The name of the kernel Image file
         self.image = self.get_tag('image', "Image")
 
-        # The block device drive type
-        self.drive = self.get_tag('drive', "virtio-blk-device")
-
         self.root = self.get_tag('root', "vda")
 
         # Occasionally we need extra devices to hook things up
@@ -99,7 +96,7 @@ def fetch_tuxrun_assets(self, dt=None):
 
         return (kernel_image, self.workdir + "/rootfs.ext4", dtb)
 
-    def prepare_run(self, kernel, disk, dtb=None, console_index=0):
+    def prepare_run(self, kernel, disk, drive, dtb=None, console_index=0):
         """
         Setup to run and add the common parameters to the system
         """
@@ -121,10 +118,8 @@ def prepare_run(self, kernel, disk, dtb=None, console_index=0):
         if self.extradev:
             self.vm.add_args('-device', self.extradev)
 
-        # Some machines already define a drive device
-        if self.drive != "none":
-            self.vm.add_args('-device',
-                             f"{self.drive},drive=hd0")
+        self.vm.add_args('-device',
+                         f"{drive},drive=hd0")
 
         # Some machines need an explicit DTB
         if dtb:
@@ -154,7 +149,9 @@ def run_tuxtest_tests(self, haltmsg):
         else:
             self.vm.wait()
 
-    def common_tuxrun(self, dt=None, haltmsg="reboot: System halted",
+    def common_tuxrun(self, dt=None,
+                      drive="virtio-blk-device",
+                      haltmsg="reboot: System halted",
                       console_index=0):
         """
         Common path for LKFT tests. Unless we need to do something
@@ -163,7 +160,7 @@ def common_tuxrun(self, dt=None, haltmsg="reboot: System halted",
         """
         (kernel, disk, dtb) = self.fetch_tuxrun_assets(dt)
 
-        self.prepare_run(kernel, disk, dtb, console_index)
+        self.prepare_run(kernel, disk, drive, dtb, console_index)
         self.vm.launch()
         self.run_tuxtest_tests(haltmsg)
 
@@ -206,11 +203,11 @@ def test_armv5(self):
         :avocado: tags=machine:versatilepb
         :avocado: tags=tuxboot:armv5
         :avocado: tags=image:zImage
-        :avocado: tags=drive:virtio-blk-pci
         :avocado: tags=console:ttyAMA0
         :avocado: tags=shutdown:nowait
         """
-        self.common_tuxrun(dt="versatile-pb.dtb")
+        self.common_tuxrun(drive="virtio-blk-pci",
+                           dt="versatile-pb.dtb")
 
     def test_armv7(self):
         """
@@ -244,10 +241,9 @@ def test_i386(self):
         :avocado: tags=machine:q35
         :avocado: tags=tuxboot:i386
         :avocado: tags=image:bzImage
-        :avocado: tags=drive:virtio-blk-pci
         :avocado: tags=shutdown:nowait
         """
-        self.common_tuxrun()
+        self.common_tuxrun(drive="virtio-blk-pci")
 
     def test_mips32(self):
         """
@@ -257,11 +253,10 @@ def test_mips32(self):
         :avocado: tags=endian:big
         :avocado: tags=tuxboot:mips32
         :avocado: tags=image:vmlinux
-        :avocado: tags=drive:driver=ide-hd,bus=ide.0,unit=0
         :avocado: tags=root:sda
         :avocado: tags=shutdown:nowait
         """
-        self.common_tuxrun()
+        self.common_tuxrun(drive="driver=ide-hd,bus=ide.0,unit=0")
 
     def test_mips32el(self):
         """
@@ -270,11 +265,10 @@ def test_mips32el(self):
         :avocado: tags=cpu:mips32r6-generic
         :avocado: tags=tuxboot:mips32el
         :avocado: tags=image:vmlinux
-        :avocado: tags=drive:driver=ide-hd,bus=ide.0,unit=0
         :avocado: tags=root:sda
         :avocado: tags=shutdown:nowait
         """
-        self.common_tuxrun()
+        self.common_tuxrun(drive="driver=ide-hd,bus=ide.0,unit=0")
 
     @skip("QEMU currently broken") # regression against stable QEMU
     def test_mips64(self):
@@ -284,11 +278,10 @@ def test_mips64(self):
         :avocado: tags=tuxboot:mips64
         :avocado: tags=endian:big
         :avocado: tags=image:vmlinux
-        :avocado: tags=drive:driver=ide-hd,bus=ide.0,unit=0
         :avocado: tags=root:sda
         :avocado: tags=shutdown:nowait
         """
-        self.common_tuxrun()
+        self.common_tuxrun(drive="driver=ide-hd,bus=ide.0,unit=0")
 
     def test_mips64el(self):
         """
@@ -296,11 +289,10 @@ def test_mips64el(self):
         :avocado: tags=machine:malta
         :avocado: tags=tuxboot:mips64el
         :avocado: tags=image:vmlinux
-        :avocado: tags=drive:driver=ide-hd,bus=ide.0,unit=0
         :avocado: tags=root:sda
         :avocado: tags=shutdown:nowait
         """
-        self.common_tuxrun()
+        self.common_tuxrun(drive="driver=ide-hd,bus=ide.0,unit=0")
 
     def test_ppc32(self):
         """
@@ -309,10 +301,9 @@ def test_ppc32(self):
         :avocado: tags=cpu:e500mc
         :avocado: tags=tuxboot:ppc32
         :avocado: tags=image:uImage
-        :avocado: tags=drive:virtio-blk-pci
         :avocado: tags=shutdown:nowait
         """
-        self.common_tuxrun()
+        self.common_tuxrun(drive="virtio-blk-pci")
 
     def test_ppc64(self):
         """
@@ -324,10 +315,9 @@ def test_ppc64(self):
         :avocado: tags=tuxboot:ppc64
         :avocado: tags=image:vmlinux
         :avocado: tags=extradev:driver=spapr-vscsi
-        :avocado: tags=drive:scsi-hd
         :avocado: tags=root:sda
         """
-        self.common_tuxrun()
+        self.common_tuxrun(drive="scsi-hd")
 
     def test_ppc64le(self):
         """
@@ -338,10 +328,9 @@ def test_ppc64le(self):
         :avocado: tags=tuxboot:ppc64le
         :avocado: tags=image:vmlinux
         :avocado: tags=extradev:driver=spapr-vscsi
-        :avocado: tags=drive:scsi-hd
         :avocado: tags=root:sda
         """
-        self.common_tuxrun()
+        self.common_tuxrun(drive="scsi-hd")
 
     def test_riscv32(self):
         """
@@ -365,10 +354,10 @@ def test_s390(self):
         :avocado: tags=endian:big
         :avocado: tags=tuxboot:s390
         :avocado: tags=image:bzImage
-        :avocado: tags=drive:virtio-blk-ccw
         :avocado: tags=shutdown:nowait
         """
-        self.common_tuxrun(haltmsg="Requesting system halt")
+        self.common_tuxrun(drive="virtio-blk-ccw",
+                           haltmsg="Requesting system halt")
 
     # Note: some segfaults caused by unaligned userspace access
     @skipIf(os.getenv('GITLAB_CI'), 'Skipping unstable test on GitLab')
@@ -380,7 +369,6 @@ def test_sh4(self):
         :avocado: tags=tuxboot:sh4
         :avocado: tags=image:zImage
         :avocado: tags=root:sda
-        :avocado: tags=drive:driver=ide-hd,bus=ide.0,unit=0
         :avocado: tags=console:ttySC1
         """
         # The test is currently too unstable to do much in userspace
@@ -388,7 +376,9 @@ def test_sh4(self):
         (kernel, disk, dtb) = self.fetch_tuxrun_assets()
 
         # the console comes on the second serial port
-        self.prepare_run(kernel, disk, console_index=1)
+        self.prepare_run(kernel, disk,
+                         "driver=ide-hd,bus=ide.0,unit=0",
+                         console_index=1)
         self.vm.launch()
 
         self.wait_for_console_pattern("Welcome to TuxTest")
@@ -404,10 +394,9 @@ def test_sparc64(self):
         :avocado: tags=tuxboot:sparc64
         :avocado: tags=image:vmlinux
         :avocado: tags=root:sda
-        :avocado: tags=drive:driver=ide-hd,bus=ide.0,unit=0
         :avocado: tags=shutdown:nowait
         """
-        self.common_tuxrun()
+        self.common_tuxrun(drive="driver=ide-hd,bus=ide.0,unit=0")
 
     def test_x86_64(self):
         """
@@ -417,7 +406,6 @@ def test_x86_64(self):
         :avocado: tags=tuxboot:x86_64
         :avocado: tags=image:bzImage
         :avocado: tags=root:sda
-        :avocado: tags=drive:driver=ide-hd,bus=ide.0,unit=0
         :avocado: tags=shutdown:nowait
         """
-        self.common_tuxrun()
+        self.common_tuxrun(drive="driver=ide-hd,bus=ide.0,unit=0")
-- 
2.39.2

Re: [PATCH 11/11] tests/avocado: don't use tags to define drive

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2733 bytes --]

On Fri, 2023-03-10 at 10:31 +0000, Alex Bennée wrote:
> We are abusing the avocado tags which are intended to provide test
> selection metadata to provide parameters to our test. This works OK
> up
> until the point you need to have ,'s in the field as this is the tag
> separator character which is the case for a number of the drive
> parameters. Fix this by making drive a parameter to the common helper
> function.
> 
> Fixes: 267fe57c23 (tests: add tuxrun baseline test to avocado)
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: David Woodhouse <dwmw2@infradead.org>


Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>

Thanks! Now this works (and I should update the commit message)...


From 8b9e04d1c7c942f51b575b94fd280bd2353f76b6 Mon Sep 17 00:00:00 2001
From: David Woodhouse <dwmw@amazon.co.uk>
Date: Thu, 9 Mar 2023 17:34:44 +0000
Subject: [PATCH] Boot KVM/Xen guest with tuxrun

The drive tag doesn't work right, and drops extra args. And we're
using a temporary kernel build.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 tests/avocado/tuxrun_baselines.py | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/tests/avocado/tuxrun_baselines.py b/tests/avocado/tuxrun_baselines.py
index c3fb67f5dc..0abb77023d 100644
--- a/tests/avocado/tuxrun_baselines.py
+++ b/tests/avocado/tuxrun_baselines.py
@@ -83,7 +83,8 @@ def fetch_tuxrun_assets(self, dt=None):
         use the per-test tags to fetch details.
         """
         base_url = f"https://storage.tuxboot.com/{self.tuxboot}/"
-        kernel_image =  self.fetch_asset(base_url + self.image)
+        #kernel_image =  self.fetch_asset(base_url + self.image)
+        kernel_image = self.fetch_asset('https://storage.tuxsuite.com/public/linaro/alex/builds/2MmnfXeo73oWy3oaJx1bxliw8z4/bzImage')
         disk_image_zst = self.fetch_asset(base_url + "rootfs.ext4.zst")
 
         cmd = f"{self.zstd} -d {disk_image_zst} -o {self.workdir}/rootfs.ext4"
@@ -409,3 +410,17 @@ def test_x86_64(self):
         :avocado: tags=shutdown:nowait
         """
         self.common_tuxrun(drive="driver=ide-hd,bus=ide.0,unit=0")
+
+    def test_x86_64_kvm_xen(self):
+        """
+        :avocado: tags=arch:x86_64
+        :avocado: tags=machine:q35
+        :avocado: tags=cpu:Nehalem
+        :avocado: tags=tuxboot:x86_64
+        :avocado: tags=image:bzImage
+        :avocado: tags=root:xvda
+        :avocado: tags=shutdown:nowait
+        """
+        self.require_accelerator("kvm")
+        self.vm.add_args("-accel", "kvm,xen-version=0x4000a,kernel-irqchip=split")
+        self.common_tuxrun(drive="driver=xen-disk,vdev=xvda")
-- 
2.34.1



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH 11/11] tests/avocado: don't use tags to define drive

[Philippe Mathieu-Daudé]

On 10/3/23 11:31, Alex Bennée wrote:
> We are abusing the avocado tags which are intended to provide test
> selection metadata to provide parameters to our test.

Oh I missed that, good point.

> This works OK up
> until the point you need to have ,'s in the field as this is the tag
> separator character which is the case for a number of the drive
> parameters. Fix this by making drive a parameter to the common helper
> function.
> 
> Fixes: 267fe57c23 (tests: add tuxrun baseline test to avocado)
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> ---
>   tests/avocado/tuxrun_baselines.py | 60 +++++++++++++------------------
>   1 file changed, 24 insertions(+), 36 deletions(-)

These also need to be cleaned:

>           :avocado: tags=tuxboot:armv5
>           :avocado: tags=image:zImage
>           :avocado: tags=console:ttyAMA0
>           :avocado: tags=root:sda
>           :avocado: tags=shutdown:nowait
>           :avocado: tags=extradev:driver=spapr-vscsi

Can be done on top, so:

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