[PATCH v1 00/15] testing, docs, tracepoints and cputlb hacks

[PATCH v1 00/15] testing, docs, tracepoints and cputlb hacks

[Alex Bennée]

Hi,

In anticipation of the tree re-opening soon I thought flush the
current state of my maintainer trees. We have a change for testing, a
couple of documentation updates, some refactoring of disassembly
handling and additional tracepoints for diagnosing cputlb issues.
There is also a hack for speeding up UEFI boots which I suspect will
never be up-streamed but may serve as a prompt for further discussion
and more refined approaches.

Currently the following patches need review:

- accel/tcg: add heuristic to invalidate al TBs in a page [hack!]
- tests/tcg: add memory-sve test for aarch64
- accel/tcg: add tb_invalidate_phys_page_range tracepoint
- cputlb: add tracepoints for TB invalidation
- cputlb: add tracepoints for the protect/unprotect helpers
- disas: use result of ->read_memory_func
- disas: generalise plugin_printf and use for monitor_disas
- monitor: expose monitor_puts to rest of code
- accel/tcg: add tb_invalidate_phy_pages_fast tracepoint
- accel/tcg: move trace events to correct location
- docs/devel: drop :hidden: and :includehidden: tags
- docs/devel: add some notes on the binfmt-image-debian targets
- tests/avocado: update aarch64_virt test to exercise -cpu max


Alex Bennée (14):
  tests/avocado: update aarch64_virt test to exercise -cpu max
  docs/devel: add some notes on the binfmt-image-debian targets
  docs/devel: drop :hidden: and :includehidden: tags
  accel/tcg: move trace events to correct location
  accel/tcg: add tb_invalidate_phy_pages_fast tracepoint
  monitor: expose monitor_puts to rest of code
  disas: generalise plugin_printf and use for monitor_disas
  disas: use result of ->read_memory_func
  cputlb: add tracepoints for the protect/unprotect helpers
  cputlb: add tracepoints for TB invalidation
  accel/tcg: add tb_invalidate_phys_page_range tracepoint
  tests/tcg: add memory-sve test for aarch64
  plugins: extend execlog to filter matches
  accel/tcg: add heuristic to invalidate al TBs in a page [hack!]

Richard Henderson (1):
  plugins: Assert mmu_idx in range before use in qemu_plugin_get_hwaddr

 docs/devel/index-api.rst                  |  1 -
 docs/devel/index-build.rst                |  3 +-
 docs/devel/index-internals.rst            |  1 -
 docs/devel/index-process.rst              |  1 -
 docs/devel/index-tcg.rst                  |  1 -
 docs/devel/index.rst                      |  2 -
 docs/devel/qtest.rst                      |  1 -
 docs/devel/tcg-plugins.rst                |  9 ++-
 docs/devel/testing.rst                    | 38 +++++++++
 include/monitor/monitor.h                 |  1 +
 monitor/monitor-internal.h                |  1 -
 accel/tcg/cputlb.c                        | 16 ++--
 accel/tcg/translate-all.c                 | 38 +++++++--
 contrib/plugins/execlog.c                 | 96 +++++++++++++++++++----
 disas.c                                   | 43 +++++-----
 disas/capstone.c                          | 73 ++++++++++-------
 plugins/api.c                             |  2 +
 MAINTAINERS                               |  1 +
 accel/tcg/trace-events                    | 10 +++
 tests/avocado/boot_linux_console.py       | 25 ------
 tests/avocado/machine_aarch64_virt.py     | 51 ++++++++++++
 tests/tcg/aarch64/Makefile.softmmu-target |  7 ++
 tests/tcg/aarch64/system/boot.S           |  3 +-
 trace-events                              |  4 -
 24 files changed, 314 insertions(+), 114 deletions(-)
 create mode 100644 tests/avocado/machine_aarch64_virt.py

-- 
2.30.2

[PATCH v1 01/15] tests/avocado: update aarch64_virt test to exercise -cpu max

[Alex Bennée]

The Fedora 29 kernel is quite old and importantly fails when running
in LPA2 scenarios. As it's not really exercising much of the CPU space
replace it with a custom 5.16.12 kernel with all the architecture
options turned on. There is a minimal buildroot initramfs included in
the kernel which has a few tools for stress testing the memory
subsystem. The userspace also targets the Neoverse N1 processor so
would fail with a v8.0 cpu like cortex-a53.

While we are at it move the test into its own file so it can have an
assigned maintainer.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20220315121251.2280317-3-alex.bennee@linaro.org>

---
v2
  - move test into own machine file
v3
  - minor reword of commit, added tags
---
 MAINTAINERS                           |  1 +
 tests/avocado/boot_linux_console.py   | 25 -------------
 tests/avocado/machine_aarch64_virt.py | 51 +++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 25 deletions(-)
 create mode 100644 tests/avocado/machine_aarch64_virt.py

diff --git a/MAINTAINERS b/MAINTAINERS
index 4ad2451e03..dcedfaed1f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -942,6 +942,7 @@ S: Maintained
 F: hw/arm/virt*
 F: include/hw/arm/virt.h
 F: docs/system/arm/virt.rst
+F: tests/avocado/machine_aarch64_virt.py
 
 Xilinx Zynq
 M: Edgar E. Iglesias <edgar.iglesias@gmail.com>
diff --git a/tests/avocado/boot_linux_console.py b/tests/avocado/boot_linux_console.py
index b40a3abc81..45a2ceda22 100644
--- a/tests/avocado/boot_linux_console.py
+++ b/tests/avocado/boot_linux_console.py
@@ -325,31 +325,6 @@ def test_mips_malta32el_nanomips_64k_dbg(self):
         kernel_hash = '18d1c68f2e23429e266ca39ba5349ccd0aeb7180'
         self.do_test_mips_malta32el_nanomips(kernel_url, kernel_hash)
 
-    def test_aarch64_virt(self):
-        """
-        :avocado: tags=arch:aarch64
-        :avocado: tags=machine:virt
-        :avocado: tags=accel:tcg
-        :avocado: tags=cpu:cortex-a53
-        """
-        kernel_url = ('https://archives.fedoraproject.org/pub/archive/fedora'
-                      '/linux/releases/29/Everything/aarch64/os/images/pxeboot'
-                      '/vmlinuz')
-        kernel_hash = '8c73e469fc6ea06a58dc83a628fc695b693b8493'
-        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
-
-        self.vm.set_console()
-        kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
-                               'console=ttyAMA0')
-        self.require_accelerator("tcg")
-        self.vm.add_args('-cpu', 'cortex-a53',
-                         '-accel', 'tcg',
-                         '-kernel', kernel_path,
-                         '-append', kernel_command_line)
-        self.vm.launch()
-        console_pattern = 'Kernel command line: %s' % kernel_command_line
-        self.wait_for_console_pattern(console_pattern)
-
     def test_aarch64_xlnx_versal_virt(self):
         """
         :avocado: tags=arch:aarch64
diff --git a/tests/avocado/machine_aarch64_virt.py b/tests/avocado/machine_aarch64_virt.py
new file mode 100644
index 0000000000..21848cba70
--- /dev/null
+++ b/tests/avocado/machine_aarch64_virt.py
@@ -0,0 +1,51 @@
+# Functional test that boots a Linux kernel and checks the console
+#
+# Copyright (c) 2022 Linaro Ltd.
+#
+# Author:
+#  Alex Bennée <alex.bennee@linaro.org>
+#
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+import time
+
+from avocado_qemu import QemuSystemTest
+from avocado_qemu import wait_for_console_pattern
+from avocado_qemu import exec_command
+
+class Aarch64VirtMachine(QemuSystemTest):
+    KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
+
+    def wait_for_console_pattern(self, success_message, vm=None):
+        wait_for_console_pattern(self, success_message,
+                                 failure_message='Kernel panic - not syncing',
+                                 vm=vm)
+
+    def test_aarch64_virt(self):
+        """
+        :avocado: tags=arch:aarch64
+        :avocado: tags=machine:virt
+        :avocado: tags=accel:tcg
+        :avocado: tags=cpu:max
+        """
+        kernel_url = ('https://fileserver.linaro.org/s/'
+                      'z6B2ARM7DQT3HWN/download')
+
+        kernel_hash = 'ed11daab50c151dde0e1e9c9cb8b2d9bd3215347'
+        kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+
+        self.vm.set_console()
+        kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
+                               'console=ttyAMA0')
+        self.require_accelerator("tcg")
+        self.vm.add_args('-cpu', 'max,pauth-impdef=on',
+                         '-accel', 'tcg',
+                         '-kernel', kernel_path,
+                         '-append', kernel_command_line)
+        self.vm.launch()
+        self.wait_for_console_pattern('Welcome to Buildroot')
+        time.sleep(0.1)
+        exec_command(self, 'root')
+        time.sleep(0.1)
+        exec_command(self, 'cat /proc/self/maps')
+        time.sleep(0.1)
-- 
2.30.2

[PATCH v1 02/15] docs/devel: add some notes on the binfmt-image-debian targets

[Alex Bennée]

We document some of this on the wiki but lets move it into our
official developer notes documentation.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>

---
v2
  - fix some trailing ws
---
 docs/devel/testing.rst | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/docs/devel/testing.rst b/docs/devel/testing.rst
index 92d40cdd19..5b60a31807 100644
--- a/docs/devel/testing.rst
+++ b/docs/devel/testing.rst
@@ -637,6 +637,44 @@ The full set of annotations can be found here:
 
 https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp
 
+docker-binfmt-image-debian-% targets
+------------------------------------
+
+It is possible to combine Debian's bootstrap scripts with a configured
+``binfmt_misc`` to bootstrap a number of Debian's distros including
+experimental ports not yet supported by a released OS. This can
+simplify setting up a rootfs by using docker to contain the foreign
+rootfs rather than manually invoking chroot.
+
+Setting up ``binfmt_misc``
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+You can use the script ``qemu-binfmt-conf.sh`` to configure a QEMU
+user binary to automatically run binaries for the foreign
+architecture. While the scripts will try their best to work with
+dynamically linked QEMU's a statically linked one will present less
+potential complications when copying into the docker image. Modern
+kernels support the ``F`` (fix binary) flag which will open the QEMU
+executable on setup and avoids the need to find and re-open in the
+chroot environment. This is triggered with the ``--persistent`` flag.
+
+Example invocation
+~~~~~~~~~~~~~~~~~~
+
+For example to setup the HPPA ports builds of Debian::
+
+  make docker-binfmt-image-debian-sid-hppa \
+    DEB_TYPE=sid DEB_ARCH=hppa \
+    DEB_URL=http://ftp.ports.debian.org/debian-ports/ \
+    DEB_KEYRING=/usr/share/keyrings/debian-ports-archive-keyring.gpg \
+    EXECUTABLE=(pwd)/qemu-hppa V=1
+
+The ``DEB_`` variables are substitutions used by
+``debian-boostrap.pre`` which is called to do the initial debootstrap
+of the rootfs before it is copied into the container. The second stage
+is run as part of the build. The final image will be tagged as
+``qemu/debian-sid-hppa``.
+
 VM testing
 ----------
 
-- 
2.30.2

[PATCH v1 03/15] docs/devel: drop :hidden: and :includehidden: tags

[Alex Bennée]

This was confusing and in the case of qtest was hiding the details of
the qgraph sub-document in the qtest pages.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/devel/index-api.rst       | 1 -
 docs/devel/index-build.rst     | 3 +--
 docs/devel/index-internals.rst | 1 -
 docs/devel/index-process.rst   | 1 -
 docs/devel/index-tcg.rst       | 1 -
 docs/devel/index.rst           | 2 --
 docs/devel/qtest.rst           | 1 -
 7 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/docs/devel/index-api.rst b/docs/devel/index-api.rst
index b749240272..60c0d7459d 100644
--- a/docs/devel/index-api.rst
+++ b/docs/devel/index-api.rst
@@ -6,7 +6,6 @@ generated from in-code annotations to function prototypes.
 
 .. toctree::
    :maxdepth: 2
-   :includehidden:
 
    bitops
    loads-stores
diff --git a/docs/devel/index-build.rst b/docs/devel/index-build.rst
index d96894f07c..1002a533a6 100644
--- a/docs/devel/index-build.rst
+++ b/docs/devel/index-build.rst
@@ -6,8 +6,7 @@ into our testing infrastructure. You will need to understand some of
 the basics if you are adding new files and targets to the build.
 
 .. toctree::
-   :maxdepth: 2
-   :includehidden:
+   :maxdepth: 3
 
    build-system
    kconfig
diff --git a/docs/devel/index-internals.rst b/docs/devel/index-internals.rst
index bb118b8eaf..a50889c556 100644
--- a/docs/devel/index-internals.rst
+++ b/docs/devel/index-internals.rst
@@ -5,7 +5,6 @@ Details about QEMU's various subsystems including how to add features to them.
 
 .. toctree::
    :maxdepth: 2
-   :includehidden:
 
    qom
    atomics
diff --git a/docs/devel/index-process.rst b/docs/devel/index-process.rst
index 314e9e94cc..d0d7a200fd 100644
--- a/docs/devel/index-process.rst
+++ b/docs/devel/index-process.rst
@@ -5,7 +5,6 @@ Notes about how to interact with the community and how and where to submit patch
 
 .. toctree::
    :maxdepth: 2
-   :includehidden:
 
    code-of-conduct
    conflict-resolution
diff --git a/docs/devel/index-tcg.rst b/docs/devel/index-tcg.rst
index 3acbd95d36..0b0ad12c22 100644
--- a/docs/devel/index-tcg.rst
+++ b/docs/devel/index-tcg.rst
@@ -7,7 +7,6 @@ are only implementing things for HW accelerated hypervisors.
 
 .. toctree::
    :maxdepth: 2
-   :includehidden:
 
    tcg
    decodetree
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index a68207052d..09cfb322be 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -8,8 +8,6 @@ modifying QEMU's source code.
 
 .. toctree::
    :maxdepth: 1
-   :includehidden:
-
 
    index-process
    index-build
diff --git a/docs/devel/qtest.rst b/docs/devel/qtest.rst
index c3dceb6c8a..add293d397 100644
--- a/docs/devel/qtest.rst
+++ b/docs/devel/qtest.rst
@@ -3,7 +3,6 @@ QTest Device Emulation Testing Framework
 ========================================
 
 .. toctree::
-   :hidden:
 
    qgraph
 
-- 
2.30.2

[PATCH v1 04/15] accel/tcg: move trace events to correct location

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/cputlb.c     | 2 +-
 accel/tcg/trace-events | 4 ++++
 trace-events           | 4 ----
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 2035b2ac0a..35797cea49 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -33,7 +33,7 @@
 #include "qemu/atomic.h"
 #include "qemu/atomic128.h"
 #include "exec/translate-all.h"
-#include "trace/trace-root.h"
+#include "trace.h"
 #include "tb-hash.h"
 #include "internal.h"
 #ifdef CONFIG_PLUGIN
diff --git a/accel/tcg/trace-events b/accel/tcg/trace-events
index 59eab96f26..4e9b450520 100644
--- a/accel/tcg/trace-events
+++ b/accel/tcg/trace-events
@@ -6,5 +6,9 @@ exec_tb(void *tb, uintptr_t pc) "tb:%p pc=0x%"PRIxPTR
 exec_tb_nocache(void *tb, uintptr_t pc) "tb:%p pc=0x%"PRIxPTR
 exec_tb_exit(void *last_tb, unsigned int flags) "tb:%p flags=0x%x"
 
+# cputlb.c
+memory_notdirty_write_access(uint64_t vaddr, uint64_t ram_addr, unsigned size) "0x%" PRIx64 " ram_addr 0x%" PRIx64 " size %u"
+memory_notdirty_set_dirty(uint64_t vaddr) "0x%" PRIx64
+
 # translate-all.c
 translate_block(void *tb, uintptr_t pc, const void *tb_code) "tb:%p, pc:0x%"PRIxPTR", tb_code:%p"
diff --git a/trace-events b/trace-events
index bc71006675..841c2ca7c5 100644
--- a/trace-events
+++ b/trace-events
@@ -42,10 +42,6 @@ find_ram_offset(uint64_t size, uint64_t offset) "size: 0x%" PRIx64 " @ 0x%" PRIx
 find_ram_offset_loop(uint64_t size, uint64_t candidate, uint64_t offset, uint64_t next, uint64_t mingap) "trying size: 0x%" PRIx64 " @ 0x%" PRIx64 ", offset: 0x%" PRIx64" next: 0x%" PRIx64 " mingap: 0x%" PRIx64
 ram_block_discard_range(const char *rbname, void *hva, size_t length, bool need_madvise, bool need_fallocate, int ret) "%s@%p + 0x%zx: madvise: %d fallocate: %d ret: %d"
 
-# accel/tcg/cputlb.c
-memory_notdirty_write_access(uint64_t vaddr, uint64_t ram_addr, unsigned size) "0x%" PRIx64 " ram_addr 0x%" PRIx64 " size %u"
-memory_notdirty_set_dirty(uint64_t vaddr) "0x%" PRIx64
-
 # gdbstub.c
 gdbstub_op_start(const char *device) "Starting gdbstub using device %s"
 gdbstub_op_exiting(uint8_t code) "notifying exit with code=0x%02x"
-- 
2.30.2

[PATCH v1 05/15] accel/tcg: add tb_invalidate_phy_pages_fast tracepoint

[Alex Bennée]

These events can be very expensive for the translator so lets add a
tracepoint to help with debugging what might be causing them. Clean up
the comments while we are at it.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/translate-all.c | 15 +++++++++++----
 accel/tcg/trace-events    |  1 +
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 5971cd53ab..e9f1cc179d 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1807,21 +1807,28 @@ void tb_invalidate_phys_range(target_ulong start, target_ulong end)
 }
 
 #ifdef CONFIG_SOFTMMU
-/* len must be <= 8 and start must be a multiple of len.
+/*
+ * len must be <= 8 and start must be a multiple of len.
  * Called via softmmu_template.h when code areas are written to with
  * iothread mutex not held.
  *
- * Call with all @pages in the range [@start, @start + len[ locked.
+ * Call with all @pages in the range [@start, @start + len] locked.
  */
 void tb_invalidate_phys_page_fast(struct page_collection *pages,
                                   tb_page_addr_t start, int len,
                                   uintptr_t retaddr)
 {
-    PageDesc *p;
+    PageDesc *p = page_find(start >> TARGET_PAGE_BITS);
+
+    if (trace_event_get_state_backends(TRACE_TB_INVALIDATE_PHYS_PAGE_FAST)) {
+        TranslationBlock *tb = tcg_tb_lookup(retaddr);
+        g_assert(tb);
+        trace_tb_invalidate_phys_page_fast(start, len, tb->pc,
+                                           p->code_write_count, p->code_bitmap);
+    }
 
     assert_memory_lock();
 
-    p = page_find(start >> TARGET_PAGE_BITS);
     if (!p) {
         return;
     }
diff --git a/accel/tcg/trace-events b/accel/tcg/trace-events
index 4e9b450520..f4d1321b1b 100644
--- a/accel/tcg/trace-events
+++ b/accel/tcg/trace-events
@@ -11,4 +11,5 @@ memory_notdirty_write_access(uint64_t vaddr, uint64_t ram_addr, unsigned size) "
 memory_notdirty_set_dirty(uint64_t vaddr) "0x%" PRIx64
 
 # translate-all.c
+tb_invalidate_phys_page_fast(uint64_t page_start, int len, uint64_t ret, int wc, void *bmap) "addr:0x%" PRIx64 "/%d pc:0x%" PRIx64" wc:%d/%p"
 translate_block(void *tb, uintptr_t pc, const void *tb_code) "tb:%p, pc:0x%"PRIxPTR", tb_code:%p"
-- 
2.30.2

[PATCH v1 06/15] monitor: expose monitor_puts to rest of code

[Alex Bennée]

This helps us construct strings elsewhere before echoing to the
monitor. It avoids having to jump through hoops like:

  monitor_printf(mon, "%s", s->str);

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

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index cc4cc6c6ad..8e291a2588 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -31,6 +31,7 @@ void monitor_resume(Monitor *mon);
 int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp);
 int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp);
 
+int monitor_puts(Monitor *mon, const char *str);
 int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
     G_GNUC_PRINTF(2, 0);
 int monitor_printf(Monitor *mon, const char *fmt, ...) G_GNUC_PRINTF(2, 3);
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index caa2e90ef2..a2cdbbf646 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -174,7 +174,6 @@ extern int mon_refcount;
 
 extern HMPCommand hmp_cmds[];
 
-int monitor_puts(Monitor *mon, const char *str);
 void monitor_data_init(Monitor *mon, bool is_qmp, bool skip_flush,
                        bool use_io_thread);
 void monitor_data_destroy(Monitor *mon);
-- 
2.30.2

[PATCH v1 07/15] disas: generalise plugin_printf and use for monitor_disas

[Alex Bennée]

Rather than assembling our output piecemeal lets use the same approach
as the plugin disas interface to build the disassembly string before
printing it.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 disas.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/disas.c b/disas.c
index 3dab4482d1..9b52b2fa93 100644
--- a/disas.c
+++ b/disas.c
@@ -248,7 +248,7 @@ void target_disas(FILE *out, CPUState *cpu, target_ulong code,
     }
 }
 
-static int plugin_printf(FILE *stream, const char *fmt, ...)
+static int gstring_printf(FILE *stream, const char *fmt, ...)
 {
     /* We abuse the FILE parameter to pass a GString. */
     GString *s = (GString *)stream;
@@ -279,7 +279,7 @@ char *plugin_disas(CPUState *cpu, uint64_t addr, size_t size)
     GString *ds = g_string_new(NULL);
 
     initialize_debug_target(&s, cpu);
-    s.info.fprintf_func = plugin_printf;
+    s.info.fprintf_func = gstring_printf;
     s.info.stream = (FILE *)ds;  /* abuse this slot */
     s.info.buffer_vma = addr;
     s.info.buffer_length = size;
@@ -367,15 +367,19 @@ void monitor_disas(Monitor *mon, CPUState *cpu,
 {
     int count, i;
     CPUDebug s;
+    g_autoptr(GString) ds = g_string_new("");
 
     initialize_debug_target(&s, cpu);
-    s.info.fprintf_func = qemu_fprintf;
+    s.info.fprintf_func = gstring_printf;
+    s.info.stream = (FILE *)ds;  /* abuse this slot */
+
     if (is_physical) {
         s.info.read_memory_func = physical_read_memory;
     }
     s.info.buffer_vma = pc;
 
     if (s.info.cap_arch >= 0 && cap_disas_monitor(&s.info, pc, nb_insn)) {
+        monitor_puts(mon, ds->str);
         return;
     }
 
@@ -385,13 +389,16 @@ void monitor_disas(Monitor *mon, CPUState *cpu,
         return;
     }
 
-    for(i = 0; i < nb_insn; i++) {
-	monitor_printf(mon, "0x" TARGET_FMT_lx ":  ", pc);
+    for (i = 0; i < nb_insn; i++) {
+        g_string_append_printf(ds, "0x" TARGET_FMT_lx ":  ", pc);
         count = s.info.print_insn(pc, &s.info);
-	monitor_printf(mon, "\n");
-	if (count < 0)
-	    break;
+        g_string_append_c(ds, '\n');
+        if (count < 0) {
+            break;
+        }
         pc += count;
     }
+
+    monitor_puts(mon, ds->str);
 }
 #endif
-- 
2.30.2

[PATCH v1 08/15] disas: use result of ->read_memory_func

[Alex Bennée]

This gets especially confusing if you start plugging in host addresses
from a trace and you wonder why the output keeps changing. Report when
read_memory_func fails instead of blindly disassembling the buffer
contents.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 disas.c          | 20 ++++++-------
 disas/capstone.c | 73 ++++++++++++++++++++++++++++--------------------
 2 files changed, 53 insertions(+), 40 deletions(-)

diff --git a/disas.c b/disas.c
index 9b52b2fa93..04329aae36 100644
--- a/disas.c
+++ b/disas.c
@@ -83,18 +83,18 @@ static int print_insn_objdump(bfd_vma pc, disassemble_info *info,
                               const char *prefix)
 {
     int i, n = info->buffer_length;
-    uint8_t *buf = g_malloc(n);
-
-    info->read_memory_func(pc, buf, n, info);
-
-    for (i = 0; i < n; ++i) {
-        if (i % 32 == 0) {
-            info->fprintf_func(info->stream, "\n%s: ", prefix);
+    g_autofree uint8_t *buf = g_malloc(n);
+
+    if (info->read_memory_func(pc, buf, n, info) == 0) {
+        for (i = 0; i < n; ++i) {
+            if (i % 32 == 0) {
+                info->fprintf_func(info->stream, "\n%s: ", prefix);
+            }
+            info->fprintf_func(info->stream, "%02x", buf[i]);
         }
-        info->fprintf_func(info->stream, "%02x", buf[i]);
+    } else {
+        info->fprintf_func(info->stream, "unable to read memory");
     }
-
-    g_free(buf);
     return n;
 }
 
diff --git a/disas/capstone.c b/disas/capstone.c
index 20bc8f9669..fe3efb0d3c 100644
--- a/disas/capstone.c
+++ b/disas/capstone.c
@@ -191,37 +191,43 @@ bool cap_disas_target(disassemble_info *info, uint64_t pc, size_t size)
         size_t tsize = MIN(sizeof(cap_buf) - csize, size);
         const uint8_t *cbuf = cap_buf;
 
-        info->read_memory_func(pc + csize, cap_buf + csize, tsize, info);
-        csize += tsize;
-        size -= tsize;
+        if (info->read_memory_func(pc + csize, cap_buf + csize, tsize, info) == 0) {
+            csize += tsize;
+            size -= tsize;
 
-        while (cs_disasm_iter(handle, &cbuf, &csize, &pc, insn)) {
-            cap_dump_insn(info, insn);
-        }
+            while (cs_disasm_iter(handle, &cbuf, &csize, &pc, insn)) {
+                cap_dump_insn(info, insn);
+            }
+
+            /* If the target memory is not consumed, go back for more... */
+            if (size != 0) {
+                /*
+                 * ... taking care to move any remaining fractional insn
+                 * to the beginning of the buffer.
+                 */
+                if (csize != 0) {
+                    memmove(cap_buf, cbuf, csize);
+                }
+                continue;
+            }
 
-        /* If the target memory is not consumed, go back for more... */
-        if (size != 0) {
             /*
-             * ... taking care to move any remaining fractional insn
-             * to the beginning of the buffer.
+             * Since the target memory is consumed, we should not have
+             * a remaining fractional insn.
              */
             if (csize != 0) {
-                memmove(cap_buf, cbuf, csize);
+                info->fprintf_func(info->stream,
+                                   "Disassembler disagrees with translator "
+                                   "over instruction decoding\n"
+                                   "Please report this to qemu-devel@nongnu.org\n");
             }
-            continue;
-        }
+            break;
 
-        /*
-         * Since the target memory is consumed, we should not have
-         * a remaining fractional insn.
-         */
-        if (csize != 0) {
+        } else {
             info->fprintf_func(info->stream,
-                "Disassembler disagrees with translator "
-                "over instruction decoding\n"
-                "Please report this to qemu-devel@nongnu.org\n");
+                               "0x%08" PRIx64 ": unable to read memory\n", pc);
+            break;
         }
-        break;
     }
 
     cs_close(&handle);
@@ -286,16 +292,23 @@ bool cap_disas_monitor(disassemble_info *info, uint64_t pc, int count)
 
         /* Make certain that we can make progress.  */
         assert(tsize != 0);
-        info->read_memory_func(pc + csize, cap_buf + csize, tsize, info);
-        csize += tsize;
-
-        if (cs_disasm_iter(handle, &cbuf, &csize, &pc, insn)) {
-            cap_dump_insn(info, insn);
-            if (--count <= 0) {
-                break;
+        if (info->read_memory_func(pc + csize, cap_buf + csize,
+                                   tsize, info) == 0)
+        {
+            csize += tsize;
+
+            if (cs_disasm_iter(handle, &cbuf, &csize, &pc, insn)) {
+                cap_dump_insn(info, insn);
+                if (--count <= 0) {
+                    break;
+                }
             }
+            memmove(cap_buf, cbuf, csize);
+        } else {
+            info->fprintf_func(info->stream,
+                               "0x%08" PRIx64 ": unable to read memory\n", pc);
+            break;
         }
-        memmove(cap_buf, cbuf, csize);
     }
 
     cs_close(&handle);
-- 
2.30.2

[PATCH v1 09/15] cputlb: add tracepoints for the protect/unprotect helpers

[Alex Bennée]

This helps track when pages are tagged for detecting code changes.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/cputlb.c     | 14 ++++++++++----
 accel/tcg/trace-events |  3 +++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 35797cea49..f2983cb2fc 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -947,18 +947,24 @@ void tlb_flush_page_bits_by_mmuidx_all_cpus_synced(CPUState *src_cpu,
                                               idxmap, bits);
 }
 
-/* update the TLBs so that writes to code in the virtual page 'addr'
-   can be detected */
+/*
+ * Update the TLBs so that writes to code in the virtual page 'ram_addr'
+ * can be detected.
+ */
 void tlb_protect_code(ram_addr_t ram_addr)
 {
+    trace_tlb_protect_code(ram_addr);
     cpu_physical_memory_test_and_clear_dirty(ram_addr, TARGET_PAGE_SIZE,
                                              DIRTY_MEMORY_CODE);
 }
 
-/* update the TLB so that writes in physical page 'phys_addr' are no longer
-   tested for self modifying code */
+/*
+ * Update the TLB so that writes in physical page 'ram_addr' are no longer
+ * tested for self modifying code
+ */
 void tlb_unprotect_code(ram_addr_t ram_addr)
 {
+    trace_tlb_unprotect_code(ram_addr);
     cpu_physical_memory_set_dirty_flag(ram_addr, DIRTY_MEMORY_CODE);
 }
 
diff --git a/accel/tcg/trace-events b/accel/tcg/trace-events
index f4d1321b1b..e11e59492f 100644
--- a/accel/tcg/trace-events
+++ b/accel/tcg/trace-events
@@ -7,6 +7,9 @@ exec_tb_nocache(void *tb, uintptr_t pc) "tb:%p pc=0x%"PRIxPTR
 exec_tb_exit(void *last_tb, unsigned int flags) "tb:%p flags=0x%x"
 
 # cputlb.c
+tlb_protect_code(uint64_t ram_addr) "0x%" PRIx64""
+tlb_unprotect_code(uint64_t ram_addr) "0x%" PRIx64""
+
 memory_notdirty_write_access(uint64_t vaddr, uint64_t ram_addr, unsigned size) "0x%" PRIx64 " ram_addr 0x%" PRIx64 " size %u"
 memory_notdirty_set_dirty(uint64_t vaddr) "0x%" PRIx64
 
-- 
2.30.2

[PATCH v1 10/15] cputlb: add tracepoints for TB invalidation

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/translate-all.c | 2 ++
 accel/tcg/trace-events    | 1 +
 2 files changed, 3 insertions(+)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index e9f1cc179d..b0009177b9 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1180,6 +1180,8 @@ static void do_tb_phys_invalidate(TranslationBlock *tb, bool rm_from_page_list)
 
     assert_memory_lock();
 
+    trace_tb_phys_invalidate(tb, tb->pc);
+
     /* make sure no further incoming jumps will be chained to this TB */
     qemu_spin_lock(&tb->jmp_lock);
     qatomic_set(&tb->cflags, tb->cflags | CF_INVALID);
diff --git a/accel/tcg/trace-events b/accel/tcg/trace-events
index e11e59492f..c034649dc2 100644
--- a/accel/tcg/trace-events
+++ b/accel/tcg/trace-events
@@ -14,5 +14,6 @@ memory_notdirty_write_access(uint64_t vaddr, uint64_t ram_addr, unsigned size) "
 memory_notdirty_set_dirty(uint64_t vaddr) "0x%" PRIx64
 
 # translate-all.c
+tb_phys_invalidate(void *tb, uintptr_t pc) "tb:%p, pc:0x%"PRIxPTR""
 tb_invalidate_phys_page_fast(uint64_t page_start, int len, uint64_t ret, int wc, void *bmap) "addr:0x%" PRIx64 "/%d pc:0x%" PRIx64" wc:%d/%p"
 translate_block(void *tb, uintptr_t pc, const void *tb_code) "tb:%p, pc:0x%"PRIxPTR", tb_code:%p"
-- 
2.30.2

[PATCH v1 11/15] accel/tcg: add tb_invalidate_phys_page_range tracepoint

[Alex Bennée]

This gives a little more insight into what is going on as we
invalidate a range of TBs.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/translate-all.c | 9 +++++++++
 accel/tcg/trace-events    | 1 +
 2 files changed, 10 insertions(+)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index b0009177b9..625c46dd9b 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1671,6 +1671,7 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
     TranslationBlock *tb;
     tb_page_addr_t tb_start, tb_end;
     int n;
+    int checked = 0, removed = 0;
 #ifdef TARGET_HAS_PRECISE_SMC
     CPUState *cpu = current_cpu;
     CPUArchState *env = NULL;
@@ -1695,6 +1696,7 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
        the code */
     PAGE_FOR_EACH_TB(p, tb, n) {
         assert_page_locked(p);
+        checked++;
         /* NOTE: this is subtle as a TB may span two physical pages */
         if (n == 0) {
             /* NOTE: tb_end may be after the end of the page, but
@@ -1728,13 +1730,20 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
             }
 #endif /* TARGET_HAS_PRECISE_SMC */
             tb_phys_invalidate__locked(tb);
+            removed++;
         }
     }
+
+
 #if !defined(CONFIG_USER_ONLY)
     /* if no code remaining, no need to continue to use slow writes */
     if (!p->first_tb) {
         invalidate_page_bitmap(p);
         tlb_unprotect_code(start);
+        trace_tb_invalidate_phys_page_range(checked, removed, 0);
+    } else {
+        TranslationBlock *tb = (TranslationBlock *) p->first_tb;
+        trace_tb_invalidate_phys_page_range(checked, removed, tb->pc);
     }
 #endif
 #ifdef TARGET_HAS_PRECISE_SMC
diff --git a/accel/tcg/trace-events b/accel/tcg/trace-events
index c034649dc2..fdc64e1423 100644
--- a/accel/tcg/trace-events
+++ b/accel/tcg/trace-events
@@ -15,5 +15,6 @@ memory_notdirty_set_dirty(uint64_t vaddr) "0x%" PRIx64
 
 # translate-all.c
 tb_phys_invalidate(void *tb, uintptr_t pc) "tb:%p, pc:0x%"PRIxPTR""
+tb_invalidate_phys_page_range(int checked, int removed, uintptr_t ftb_pc) "checked:%d removed:%d first_tb_pc:0x%"PRIxPTR""
 tb_invalidate_phys_page_fast(uint64_t page_start, int len, uint64_t ret, int wc, void *bmap) "addr:0x%" PRIx64 "/%d pc:0x%" PRIx64" wc:%d/%p"
 translate_block(void *tb, uintptr_t pc, const void *tb_code) "tb:%p, pc:0x%"PRIxPTR", tb_code:%p"
-- 
2.30.2

[PATCH v1 12/15] tests/tcg: add memory-sve test for aarch64

[Alex Bennée]

This will be helpful in debugging problems with tracking SVE memory
accesses via the TCG plugins system.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Robert Henry <robhenry@microsoft.com>
Cc: Aaron Lindsay <aaron@os.amperecomputing.com>
---
 tests/tcg/aarch64/Makefile.softmmu-target | 7 +++++++
 tests/tcg/aarch64/system/boot.S           | 3 ++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target
index a7286ac295..cc02814065 100644
--- a/tests/tcg/aarch64/Makefile.softmmu-target
+++ b/tests/tcg/aarch64/Makefile.softmmu-target
@@ -31,6 +31,13 @@ LDFLAGS+=-static -nostdlib $(CRT_OBJS) $(MINILIB_OBJS) -lgcc
 
 memory: CFLAGS+=-DCHECK_UNALIGNED=1
 
+memory-sve: memory.c $(LINK_SCRIPT) $(CRT_OBJS) $(MINILIB_OBJS)
+	$(CC) $(CFLAGS) $(EXTRA_CFLAGS) $< -o $@ $(LDFLAGS)
+
+memory-sve: CFLAGS+=-DCHECK_UNALIGNED=1 -march=armv8.1-a+sve -O3 -fno-tree-loop-distribute-patterns
+
+TESTS+=memory-sve
+
 # Running
 QEMU_BASE_MACHINE=-M virt -cpu max -display none
 QEMU_OPTS+=$(QEMU_BASE_MACHINE) -semihosting-config enable=on,target=native,chardev=output -kernel
diff --git a/tests/tcg/aarch64/system/boot.S b/tests/tcg/aarch64/system/boot.S
index e190b1efa6..f136363d2a 100644
--- a/tests/tcg/aarch64/system/boot.S
+++ b/tests/tcg/aarch64/system/boot.S
@@ -179,12 +179,13 @@ __start:
 	isb
 
 	/*
-	 * Enable FP registers. The standard C pre-amble will be
+	 * Enable FP/SVE registers. The standard C pre-amble will be
 	 * saving these and A-profile compilers will use AdvSIMD
 	 * registers unless we tell it not to.
 	*/
 	mrs	x0, cpacr_el1
 	orr	x0, x0, #(3 << 20)
+	orr	x0, x0, #(3 << 16)
 	msr	cpacr_el1, x0
 
 	/* Setup some stack space and enter the test code.
-- 
2.30.2

[PATCH v1 13/15] plugins: extend execlog to filter matches

[Alex Bennée]

Sometimes the whole execlog is just two much so add the ability to
filter by instruction opcode or address.

[AJB: this shows for example

 .qemu-system-aarch64 -display none -serial mon:stdio \
   -M virt -cpu max \
   -semihosting-config enable=on \
   -kernel ./tests/tcg/aarch64-softmmu/memory-sve \
   -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin -D plugin.out

the st1w SVE instruction is not instrumenting its stores.]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Alexandre Iooss <erdnaxe@crans.org>
Cc: Robert Henry <robhenry@microsoft.com>
Cc: Aaron Lindsay <aaron@os.amperecomputing.com>
---
 docs/devel/tcg-plugins.rst |  9 +++-
 contrib/plugins/execlog.c  | 96 ++++++++++++++++++++++++++++++++------
 2 files changed, 90 insertions(+), 15 deletions(-)

diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
index a7cc44aa20..a503d44cee 100644
--- a/docs/devel/tcg-plugins.rst
+++ b/docs/devel/tcg-plugins.rst
@@ -346,7 +346,7 @@ The execlog tool traces executed instructions with memory access. It can be used
 for debugging and security analysis purposes.
 Please be aware that this will generate a lot of output.
 
-The plugin takes no argument::
+The plugin needs default argument::
 
   qemu-system-arm $(QEMU_ARGS) \
     -plugin ./contrib/plugins/libexeclog.so -d plugin
@@ -364,6 +364,13 @@ which will output an execution trace following this structure::
   0, 0xd34, 0xf9c8f000, "bl #0x10c8"
   0, 0x10c8, 0xfff96c43, "ldr r3, [r0, #0x44]", load, 0x200000e4, RAM
 
+the output can be filtered to only track certain instructions or
+addresses using the `ifilter` or `afilter` options. You can stack the
+arguments if required::
+
+  qemu-system-arm $(QEMU_ARGS) \
+    -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin
+
 - contrib/plugins/cache.c
 
 Cache modelling plugin that measures the performance of a given L1 cache
diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c
index a5275dcc15..e659ac9cbb 100644
--- a/contrib/plugins/execlog.c
+++ b/contrib/plugins/execlog.c
@@ -20,6 +20,9 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
 /* Store last executed instruction on each vCPU as a GString */
 GArray *last_exec;
 
+static GPtrArray *imatches;
+static GArray *amatches;
+
 /**
  * Add memory read or write information to current instruction log
  */
@@ -85,12 +88,13 @@ static void vcpu_insn_exec(unsigned int cpu_index, void *udata)
 static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
 {
     struct qemu_plugin_insn *insn;
-    uint64_t insn_vaddr;
-    uint32_t insn_opcode;
-    char *insn_disas;
+    bool skip = (imatches || amatches) ? true : false;
 
     size_t n = qemu_plugin_tb_n_insns(tb);
     for (size_t i = 0; i < n; i++) {
+        char *insn_disas;
+        uint64_t insn_vaddr;
+
         /*
          * `insn` is shared between translations in QEMU, copy needed data here.
          * `output` is never freed as it might be used multiple times during
@@ -99,20 +103,52 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
          * a limitation for CISC architectures.
          */
         insn = qemu_plugin_tb_get_insn(tb, i);
-        insn_vaddr = qemu_plugin_insn_vaddr(insn);
-        insn_opcode = *((uint32_t *)qemu_plugin_insn_data(insn));
         insn_disas = qemu_plugin_insn_disas(insn);
-        char *output = g_strdup_printf("0x%"PRIx64", 0x%"PRIx32", \"%s\"",
-                                       insn_vaddr, insn_opcode, insn_disas);
+        insn_vaddr = qemu_plugin_insn_vaddr(insn);
+
+        /*
+         * If we are filtering we better check out if we have any
+         * hits. The skip "latches" so we can track memory accesses
+         * after the instruction we care about.
+         */
+        if (skip && imatches) {
+            int j;
+            for (j = 0; j < imatches->len && skip; j++) {
+                char *m = g_ptr_array_index(imatches, j);
+                if (g_str_has_prefix(insn_disas, m)) {
+                    skip = false;
+                }
+            }
+        }
+
+        if (skip && amatches) {
+            int j;
+            for (j = 0; j < amatches->len && skip; j++) {
+                uint64_t v = g_array_index(amatches, uint64_t, j);
+                if (v == insn_vaddr) {
+                    skip = false;
+                }
+            }
+        }
 
-        /* Register callback on memory read or write */
-        qemu_plugin_register_vcpu_mem_cb(insn, vcpu_mem,
-                                         QEMU_PLUGIN_CB_NO_REGS,
-                                         QEMU_PLUGIN_MEM_RW, NULL);
+        if (skip) {
+            g_free(insn_disas);
+        } else {
+            uint32_t insn_opcode;
+            insn_opcode = *((uint32_t *)qemu_plugin_insn_data(insn));
+            char *output = g_strdup_printf("0x%"PRIx64", 0x%"PRIx32", \"%s\"",
+                                           insn_vaddr, insn_opcode, insn_disas);
+
+            /* Register callback on memory read or write */
+            qemu_plugin_register_vcpu_mem_cb(insn, vcpu_mem,
+                                             QEMU_PLUGIN_CB_NO_REGS,
+                                             QEMU_PLUGIN_MEM_RW, NULL);
+
+            /* Register callback on instruction */
+            qemu_plugin_register_vcpu_insn_exec_cb(insn, vcpu_insn_exec,
+                                                   QEMU_PLUGIN_CB_NO_REGS, output);
+        }
 
-        /* Register callback on instruction */
-        qemu_plugin_register_vcpu_insn_exec_cb(insn, vcpu_insn_exec,
-                                               QEMU_PLUGIN_CB_NO_REGS, output);
     }
 }
 
@@ -132,6 +168,25 @@ static void plugin_exit(qemu_plugin_id_t id, void *p)
     }
 }
 
+/* Add a match to the array of matches */
+static void parse_insn_match(char *match)
+{
+    if (!imatches) {
+        imatches = g_ptr_array_new();
+    }
+    g_ptr_array_add(imatches, match);
+}
+
+static void parse_vaddr_match(char *match)
+{
+    uint64_t v = g_ascii_strtoull(match, NULL, 16);
+
+    if (!amatches) {
+        amatches = g_array_new(false, true, sizeof(uint64_t));
+    }
+    g_array_append_val(amatches, v);
+}
+
 /**
  * Install the plugin
  */
@@ -145,6 +200,19 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
      */
     last_exec = g_array_new(FALSE, FALSE, sizeof(GString *));
 
+    for (int i = 0; i < argc; i++) {
+        char *opt = argv[i];
+        g_autofree char **tokens = g_strsplit(opt, "=", 2);
+        if (g_strcmp0(tokens[0], "ifilter") == 0) {
+            parse_insn_match(tokens[1]);
+        } else if (g_strcmp0(tokens[0], "afilter") == 0) {
+            parse_vaddr_match(tokens[1]);
+        } else {
+            fprintf(stderr, "option parsing failed: %s\n", opt);
+            return -1;
+        }
+    }
+
     /* Register translation block and exit callbacks */
     qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
     qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
-- 
2.30.2

[PATCH v1 14/15] plugins: Assert mmu_idx in range before use in qemu_plugin_get_hwaddr

[Alex Bennée]

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

Coverity reports out-of-bound accesses here.  This should be a
false positive due to how the index is decoded from MemOpIdx.

Fixes: Coverity CID 1487201
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>
Message-Id: <20220401190233.329360-1-richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 plugins/api.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/plugins/api.c b/plugins/api.c
index 7bf71b189d..2078b16edb 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -289,6 +289,8 @@ struct qemu_plugin_hwaddr *qemu_plugin_get_hwaddr(qemu_plugin_meminfo_t info,
     enum qemu_plugin_mem_rw rw = get_plugin_meminfo_rw(info);
     hwaddr_info.is_store = (rw & QEMU_PLUGIN_MEM_W) != 0;
 
+    assert(mmu_idx < NB_MMU_MODES);
+
     if (!tlb_plugin_lookup(cpu, vaddr, mmu_idx,
                            hwaddr_info.is_store, &hwaddr_info)) {
         error_report("invalid use of qemu_plugin_get_hwaddr");
-- 
2.30.2

[PATCH v1 15/15] accel/tcg: add heuristic to invalidate al TBs in a page [hack!]

[Alex Bennée]

This is a dumb attempt to reduce to the execution time of UEFI booted
kernels and almost certainly not what the final solution should be.

The problem is if we have generated JIT code from running firmware we
can spend a lot of time carefully invalidating every TB in a page when
the kernel that eventually boots and clears all the pages in RAM.

This fix simply assumes any write to the start of the page should
invalidate everything in the page. Once that is done following writes
to the page should all follow the fast path.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 accel/tcg/translate-all.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 625c46dd9b..67884260fa 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1830,6 +1830,16 @@ void tb_invalidate_phys_page_fast(struct page_collection *pages,
                                   uintptr_t retaddr)
 {
     PageDesc *p = page_find(start >> TARGET_PAGE_BITS);
+    unsigned int nr = start & ~TARGET_PAGE_MASK;
+
+    /*
+     * Assume any write to the start of the page is start of clearing
+     * the whole page. To avoid coming back multiple times lets just
+     * invalidate everything first.
+     */
+    if (nr == 0) {
+        len = TARGET_PAGE_SIZE;
+    }
 
     if (trace_event_get_state_backends(TRACE_TB_INVALIDATE_PHYS_PAGE_FAST)) {
         TranslationBlock *tb = tcg_tb_lookup(retaddr);
@@ -1850,10 +1860,8 @@ void tb_invalidate_phys_page_fast(struct page_collection *pages,
         build_page_bitmap(p);
     }
     if (p->code_bitmap) {
-        unsigned int nr;
         unsigned long b;
 
-        nr = start & ~TARGET_PAGE_MASK;
         b = p->code_bitmap[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG - 1));
         if (b & ((1 << len) - 1)) {
             goto do_invalidate;
-- 
2.30.2

Re: [PATCH v1 04/15] accel/tcg: move trace events to correct location

[Richard Henderson]

On 4/8/22 09:47, Alex Bennée wrote:
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   accel/tcg/cputlb.c     | 2 +-
>   accel/tcg/trace-events | 4 ++++
>   trace-events           | 4 ----
>   3 files changed, 5 insertions(+), 5 deletions(-)

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

r~

Re: [PATCH v1 05/15] accel/tcg: add tb_invalidate_phy_pages_fast tracepoint

[Richard Henderson]

On 4/8/22 09:47, Alex Bennée wrote:
> These events can be very expensive for the translator so lets add a
> tracepoint to help with debugging what might be causing them. Clean up
> the comments while we are at it.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   accel/tcg/translate-all.c | 15 +++++++++++----
>   accel/tcg/trace-events    |  1 +
>   2 files changed, 12 insertions(+), 4 deletions(-)

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

r~

Re: [PATCH v1 06/15] monitor: expose monitor_puts to rest of code

[Richard Henderson]

On 4/8/22 09:47, Alex Bennée wrote:
> This helps us construct strings elsewhere before echoing to the
> monitor. It avoids having to jump through hoops like:
> 
>    monitor_printf(mon, "%s", s->str);
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   include/monitor/monitor.h  | 1 +
>   monitor/monitor-internal.h | 1 -
>   2 files changed, 1 insertion(+), 1 deletion(-)

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

r~

Re: [PATCH v1 07/15] disas: generalise plugin_printf and use for monitor_disas

[Richard Henderson]

On 4/8/22 09:47, Alex Bennée wrote:
> Rather than assembling our output piecemeal lets use the same approach
> as the plugin disas interface to build the disassembly string before
> printing it.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   disas.c | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)

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

r~

Re: [PATCH v1 08/15] disas: use result of ->read_memory_func

[Richard Henderson]

On 4/8/22 09:47, Alex Bennée wrote:
> This gets especially confusing if you start plugging in host addresses
> from a trace and you wonder why the output keeps changing. Report when
> read_memory_func fails instead of blindly disassembling the buffer
> contents.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   disas.c          | 20 ++++++-------
>   disas/capstone.c | 73 ++++++++++++++++++++++++++++--------------------
>   2 files changed, 53 insertions(+), 40 deletions(-)

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

r~

Re: [PATCH v1 09/15] cputlb: add tracepoints for the protect/unprotect helpers

[Richard Henderson]

On 4/8/22 09:47, Alex Bennée wrote:
> This helps track when pages are tagged for detecting code changes.
> 
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   accel/tcg/cputlb.c     | 14 ++++++++++----
>   accel/tcg/trace-events |  3 +++
>   2 files changed, 13 insertions(+), 4 deletions(-)

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

r~

Re: [PATCH v1 10/15] cputlb: add tracepoints for TB invalidation

[Richard Henderson]

On 4/8/22 09:47, Alex Bennée wrote:
> Signed-off-by: Alex Bennée<alex.bennee@linaro.org>
> ---
>   accel/tcg/translate-all.c | 2 ++
>   accel/tcg/trace-events    | 1 +
>   2 files changed, 3 insertions(+)

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

r~

Re: [PATCH v1 11/15] accel/tcg: add tb_invalidate_phys_page_range tracepoint

[Richard Henderson]

On 4/8/22 09:47, Alex Bennée wrote:
> This gives a little more insight into what is going on as we
> invalidate a range of TBs.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   accel/tcg/translate-all.c | 9 +++++++++
>   accel/tcg/trace-events    | 1 +
>   2 files changed, 10 insertions(+)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index b0009177b9..625c46dd9b 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1671,6 +1671,7 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
>       TranslationBlock *tb;
>       tb_page_addr_t tb_start, tb_end;
>       int n;
> +    int checked = 0, removed = 0;
>   #ifdef TARGET_HAS_PRECISE_SMC
>       CPUState *cpu = current_cpu;
>       CPUArchState *env = NULL;
> @@ -1695,6 +1696,7 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
>          the code */
>       PAGE_FOR_EACH_TB(p, tb, n) {
>           assert_page_locked(p);
> +        checked++;
>           /* NOTE: this is subtle as a TB may span two physical pages */
>           if (n == 0) {
>               /* NOTE: tb_end may be after the end of the page, but
> @@ -1728,13 +1730,20 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
>               }
>   #endif /* TARGET_HAS_PRECISE_SMC */
>               tb_phys_invalidate__locked(tb);
> +            removed++;
>           }
>       }
> +
> +
>   #if !defined(CONFIG_USER_ONLY)

Spacing.

>       /* if no code remaining, no need to continue to use slow writes */
>       if (!p->first_tb) {
>           invalidate_page_bitmap(p);
>           tlb_unprotect_code(start);
> +        trace_tb_invalidate_phys_page_range(checked, removed, 0);
> +    } else {
> +        TranslationBlock *tb = (TranslationBlock *) p->first_tb;
> +        trace_tb_invalidate_phys_page_range(checked, removed, tb->pc);

Is this going to get us set without use warnings on CONFIG_USER_ONLY?


r~
r~

Re: [PATCH v1 06/15] monitor: expose monitor_puts to rest of code

[Markus Armbruster]

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

> This helps us construct strings elsewhere before echoing to the
> monitor. It avoids having to jump through hoops like:
>
>   monitor_printf(mon, "%s", s->str);
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

There's just one caller other than monitor_vprintf().  Even if you add a
few more, I'm not sure keeping it in the interface is worthwhile.

If we conclude it is, then this patch should come with a conversion of
existing uses of monitor_printf(MON, "%s", STR) to monitor_puts(MON,
STR).

If we conclude it isn't, switch the lone caller to monitor_printf() and
make monitor_puts() static in monitor.c.