[PATCH v5 23/25] tests: add plugin asserting correctness of discon event's to_pc

Julian Ganz posted 25 patches 6 months ago
Maintainers: "Alex Bennée" <alex.bennee@linaro.org>, Alexandre Iooss <erdnaxe@crans.org>, Mahmoud Mandour <ma.mandourr@gmail.com>, Pierrick Bouvier <pierrick.bouvier@linaro.org>, Richard Henderson <richard.henderson@linaro.org>, Peter Maydell <peter.maydell@linaro.org>, Michael Rolnik <mrolnik@gmail.com>, Helge Deller <deller@gmx.de>, Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Song Gao <gaosong@loongson.cn>, Laurent Vivier <laurent@vivier.eu>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Aurelien Jarno <aurelien@aurel32.net>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Aleksandar Rikalo <arikalo@gmail.com>, Stafford Horne <shorne@gmail.com>, Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Yoshinori Sato <ysato@users.sourceforge.jp>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Max Filippov <jcmvbkbc@gmail.com>
There is a newer version of this series
[PATCH v5 23/25] tests: add plugin asserting correctness of discon event's to_pc
Posted by Julian Ganz 6 months ago
We recently introduced plugin API for the registration of callbacks for
discontinuity events, specifically for interrupts, exceptions and host
call events. The callback receives various bits of information,
including the VCPU index and PCs.

This change introduces a test plugin asserting the correctness of that
behaviour in cases where this is possible with reasonable effort. Since
instruction PCs are recorded at translation blocks translation time and
a TB may be used in multiple processes running in distinct virtual
memory, the plugin allows comparing not full addresses but a subset of
address bits via the `compare-addr-bits` option.

Signed-off-by: Julian Ganz <neither@nut.email>
---

Pierrick: I did implement the changes you requested, but I did not add
your Reviewed-By because I felt the changes were not trivial enough to
not require a new, coarse review.

 tests/tcg/plugins/discons.c   | 216 ++++++++++++++++++++++++++++++++++
 tests/tcg/plugins/meson.build |   2 +-
 2 files changed, 217 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/plugins/discons.c

diff --git a/tests/tcg/plugins/discons.c b/tests/tcg/plugins/discons.c
new file mode 100644
index 0000000000..afbd1b400d
--- /dev/null
+++ b/tests/tcg/plugins/discons.c
@@ -0,0 +1,216 @@
+/*
+ * Copyright (C) 2025, Julian Ganz <neither@nut.email>
+ *
+ * License: GNU GPL, version 2 or later.
+ *   See the COPYING file in the top-level directory.
+ *
+ * This plugin exercises the discontinuity plugin API and asserts some
+ * of its behaviour regarding reported program counters.
+ */
+#include <stdio.h>
+
+#include <qemu-plugin.h>
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+struct cpu_state {
+    uint64_t last_pc;
+    uint64_t from_pc;
+    uint64_t next_pc;
+    uint64_t has_from;
+    bool has_next;
+    enum qemu_plugin_discon_type next_type;
+};
+
+struct insn_data {
+    uint64_t addr;
+    uint64_t next_pc;
+    bool next_valid;
+};
+
+static struct qemu_plugin_scoreboard *states;
+
+static qemu_plugin_u64 last_pc;
+static qemu_plugin_u64 from_pc;
+static qemu_plugin_u64 has_from;
+
+static bool abort_on_mismatch;
+static bool trace_all_insns;
+static uint64_t compare_addr_mask;
+
+static bool addr_eq(uint64_t a, uint64_t b)
+{
+    return ((a ^ b) & compare_addr_mask) == 0;
+}
+
+static void report_mismatch(const char *pc_name, unsigned int vcpu_index,
+                            enum qemu_plugin_discon_type type, uint64_t last,
+                            uint64_t expected, uint64_t encountered)
+{
+    GString *report;
+    const char *discon_type_name = "unknown";
+
+    if (addr_eq(expected, encountered)) {
+        return;
+    }
+
+    switch (type) {
+    case QEMU_PLUGIN_DISCON_INTERRUPT:
+        discon_type_name = "interrupt";
+        break;
+    case QEMU_PLUGIN_DISCON_EXCEPTION:
+        discon_type_name = "exception";
+        break;
+    case QEMU_PLUGIN_DISCON_HOSTCALL:
+        discon_type_name = "hostcall";
+        break;
+    default:
+        break;
+    }
+
+    report = g_string_new(NULL);
+    g_string_append_printf(report,
+                           "Discon %s PC mismatch on VCPU %d\nExpected:      %"
+                           PRIx64"\nEncountered:   %"PRIx64"\nExecuted Last: %"
+                           PRIx64"\nEvent type:    %s\n",
+                           pc_name, vcpu_index, expected, encountered, last,
+                           discon_type_name);
+    qemu_plugin_outs(report->str);
+    if (abort_on_mismatch) {
+        g_abort();
+    }
+    g_string_free(report, true);
+}
+
+static void vcpu_discon(qemu_plugin_id_t id, unsigned int vcpu_index,
+                        enum qemu_plugin_discon_type type, uint64_t from_pc,
+                        uint64_t to_pc)
+{
+    struct cpu_state *state = qemu_plugin_scoreboard_find(states, vcpu_index);
+
+    if (type == QEMU_PLUGIN_DISCON_EXCEPTION &&
+        addr_eq(state->last_pc, from_pc))
+    {
+        /*
+         * For some types of exceptions, insn_exec will be called for the
+         * instruction that caused the exception. This is valid behaviour and
+         * does not need to be reported.
+         */
+    } else if (state->has_next) {
+        /*
+         * We may encounter discontinuity chains without any instructions
+         * being executed in between.
+         */
+        report_mismatch("source", vcpu_index, type, state->last_pc,
+                        state->next_pc, from_pc);
+    } else if (state->has_from) {
+        report_mismatch("source", vcpu_index, type, state->last_pc,
+                        state->from_pc, from_pc);
+    }
+
+    state->has_from = false;
+
+    state->next_pc = to_pc;
+    state->next_type = type;
+    state->has_next = true;
+}
+
+static void insn_exec(unsigned int vcpu_index, void *userdata)
+{
+    struct cpu_state *state = qemu_plugin_scoreboard_find(states, vcpu_index);
+
+    if (state->has_next) {
+        report_mismatch("target", vcpu_index, state->next_type, state->last_pc,
+                        state->next_pc, state->last_pc);
+        state->has_next = false;
+    }
+
+    if (trace_all_insns) {
+        g_autoptr(GString) report = g_string_new(NULL);
+        g_string_append_printf(report, "Exec insn at %"PRIx64" on VCPU %d\n",
+                               state->last_pc, vcpu_index);
+        qemu_plugin_outs(report->str);
+    }
+}
+
+static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
+{
+    size_t n_insns = qemu_plugin_tb_n_insns(tb);
+    for (size_t i = 0; i < n_insns; i++) {
+        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
+        uint64_t pc = qemu_plugin_insn_vaddr(insn);
+        uint64_t next_pc = pc + qemu_plugin_insn_size(insn);
+        uint64_t has_next = (i + 1) < n_insns;
+
+        qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(insn,
+                                                            QEMU_PLUGIN_INLINE_STORE_U64,
+                                                            last_pc, pc);
+        qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(insn,
+                                                            QEMU_PLUGIN_INLINE_STORE_U64,
+                                                            from_pc, next_pc);
+        qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu(insn,
+                                                            QEMU_PLUGIN_INLINE_STORE_U64,
+                                                            has_from, has_next);
+        qemu_plugin_register_vcpu_insn_exec_cb(insn, insn_exec,
+                                               QEMU_PLUGIN_CB_NO_REGS, NULL);
+    }
+}
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
+                                           const qemu_info_t *info,
+                                           int argc, char **argv)
+{
+    /* Set defaults */
+    abort_on_mismatch = true;
+    trace_all_insns = false;
+    compare_addr_mask = -1;
+
+    for (int i = 0; i < argc; i++) {
+        char *opt = argv[i];
+        g_auto(GStrv) tokens = g_strsplit(opt, "=", 2);
+        if (g_strcmp0(tokens[0], "abort") == 0) {
+            if (!qemu_plugin_bool_parse(tokens[0], tokens[1],
+                                        &abort_on_mismatch)) {
+                fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
+                return -1;
+            }
+        } else if (g_strcmp0(tokens[0], "trace-all") == 0) {
+            if (!qemu_plugin_bool_parse(tokens[0], tokens[1],
+                                        &trace_all_insns)) {
+                fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
+                return -1;
+            }
+        } else if (g_strcmp0(tokens[0], "compare-addr-bits") == 0) {
+            if (g_strcmp0(tokens[1], "full") == 0) {
+                compare_addr_mask = -1;
+            } else {
+                char *end = tokens[1];
+                guint64 bits = g_ascii_strtoull(tokens[1], &end, 10);
+                if (bits == 0 || bits > 64 || *end) {
+                    fprintf(stderr,
+                            "integer parsing failed or out of range: %s\n",
+                            opt);
+                    return -1;
+                }
+                compare_addr_mask = ~(((uint64_t) -1) << bits);
+            }
+        } else {
+            fprintf(stderr, "option parsing failed: %s\n", opt);
+            return -1;
+        }
+    }
+
+    states = qemu_plugin_scoreboard_new(sizeof(struct cpu_state));
+    last_pc = qemu_plugin_scoreboard_u64_in_struct(states, struct cpu_state,
+                                                   last_pc);
+    from_pc = qemu_plugin_scoreboard_u64_in_struct(states, struct cpu_state,
+                                                   from_pc);
+    has_from = qemu_plugin_scoreboard_u64_in_struct(states, struct cpu_state,
+                                                    has_from);
+
+    qemu_plugin_register_vcpu_discon_cb(id, QEMU_PLUGIN_DISCON_ALL,
+                                        vcpu_discon);
+    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
+
+    return 0;
+}
diff --git a/tests/tcg/plugins/meson.build b/tests/tcg/plugins/meson.build
index 41f02f2c7f..1b13d6e614 100644
--- a/tests/tcg/plugins/meson.build
+++ b/tests/tcg/plugins/meson.build
@@ -1,6 +1,6 @@
 t = []
 if get_option('plugins')
-  foreach i : ['bb', 'empty', 'inline', 'insn', 'mem', 'reset', 'syscall']
+  foreach i : ['bb', 'discons', 'empty', 'inline', 'insn', 'mem', 'reset', 'syscall']
     if host_os == 'windows'
       t += shared_module(i, files(i + '.c') + '../../../contrib/plugins/win32_linker.c',
                         include_directories: '../../../include/qemu',
-- 
2.49.0
Re: [PATCH v5 23/25] tests: add plugin asserting correctness of discon event's to_pc
Posted by Pierrick Bouvier 5 months, 4 weeks ago
On 5/19/25 8:24 AM, Julian Ganz wrote:
> We recently introduced plugin API for the registration of callbacks for
> discontinuity events, specifically for interrupts, exceptions and host
> call events. The callback receives various bits of information,
> including the VCPU index and PCs.
> 
> This change introduces a test plugin asserting the correctness of that
> behaviour in cases where this is possible with reasonable effort. Since
> instruction PCs are recorded at translation blocks translation time and
> a TB may be used in multiple processes running in distinct virtual
> memory, the plugin allows comparing not full addresses but a subset of
> address bits via the `compare-addr-bits` option.
> 
> Signed-off-by: Julian Ganz <neither@nut.email>
> ---
> 
> Pierrick: I did implement the changes you requested, but I did not add
> your Reviewed-By because I felt the changes were not trivial enough to
> not require a new, coarse review.
>

Looks good to me, thanks.

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Regarding the issue with the same tb being mapped at different virtual 
addresses, I'm ok with the current solution of comparing only page bits.

That said, a better solution could be to compare physical addresses when 
a discon is detected (on plugin side), and confirm it's really a 
discontinuity or just a different mapping. With this approach, it's not 
even needed to have a dedicated option, and there would be no false 
positive in the plugin. It's just a suggestion though.

Regards,
Pierrick
Re: [PATCH v5 23/25] tests: add plugin asserting correctness of discon event's to_pc
Posted by Julian Ganz 5 months, 4 weeks ago
Hi Pierrick,

May 20, 2025 at 10:01 PM, Pierrick Bouvier wrote:
> Regarding the issue with the same tb being mapped at different virtual addresses, I'm ok with the current solution of comparing only page bits.
> 
> That said, a better solution could be to compare physical addresses when a discon is detected (on plugin side), and confirm it's really a discontinuity or just a different mapping. With this approach, it's not even needed to have a dedicated option, and there would be no false positive in the plugin. It's just a suggestion though.

I actually tried to do this before resorting to the current appraoch.
However, there is only API for querying an instruction's or TB's
hardware address and none that would let me translate the virtual
addresses we receive in the discon callback, which we need to compare
against.

I considered also passing the hardware address to the callback (do the
translation in the `plugin_vcpu_cb__discon` hook), but that turned out
to be not straight forward and not something we'd want to do in the
hook, either.

Regards,
Julian
Re: [PATCH v5 23/25] tests: add plugin asserting correctness of discon event's to_pc
Posted by Pierrick Bouvier 5 months, 4 weeks ago
On 5/20/25 1:44 PM, Julian Ganz wrote:
> Hi Pierrick,
> 
> May 20, 2025 at 10:01 PM, Pierrick Bouvier wrote:
>> Regarding the issue with the same tb being mapped at different virtual addresses, I'm ok with the current solution of comparing only page bits.
>>
>> That said, a better solution could be to compare physical addresses when a discon is detected (on plugin side), and confirm it's really a discontinuity or just a different mapping. With this approach, it's not even needed to have a dedicated option, and there would be no false positive in the plugin. It's just a suggestion though.
> 
> I actually tried to do this before resorting to the current appraoch.
> However, there is only API for querying an instruction's or TB's
> hardware address and none that would let me translate the virtual
> addresses we receive in the discon callback, which we need to compare
> against.
>

It would be acceptable to add such a function allowing to query physical 
address for a virtual address (using cpu_get_phys_page_debug behind the 
hoods), as it's not leaking any QEMU implementation detail.

We can implement this later if you don't want to extend your series with 
this.

> I considered also passing the hardware address to the callback (do the
> translation in the `plugin_vcpu_cb__discon` hook), but that turned out
> to be not straight forward and not something we'd want to do in the
> hook, either.
>

Yes, in some cases, people will want virtual addresses, and sometimes 
physical ones. So passing physical ones only is too restrictive.

This plugin is a bit specific, as it's explicitely tracking all 
transitions between instructions, where a "normal" plugin will just work 
with discontinuities. That said, the use case to get physical address 
from a virtual one is a real need.

> Regards,
> Julian

Regards,
Pierrick
Re: [PATCH v5 23/25] tests: add plugin asserting correctness of discon event's to_pc
Posted by Pierrick Bouvier 5 months, 4 weeks ago
On 5/20/25 2:09 PM, Pierrick Bouvier wrote:
> On 5/20/25 1:44 PM, Julian Ganz wrote:
>> Hi Pierrick,
>>
>> May 20, 2025 at 10:01 PM, Pierrick Bouvier wrote:
>>> Regarding the issue with the same tb being mapped at different virtual addresses, I'm ok with the current solution of comparing only page bits.
>>>
>>> That said, a better solution could be to compare physical addresses when a discon is detected (on plugin side), and confirm it's really a discontinuity or just a different mapping. With this approach, it's not even needed to have a dedicated option, and there would be no false positive in the plugin. It's just a suggestion though.
>>
>> I actually tried to do this before resorting to the current appraoch.
>> However, there is only API for querying an instruction's or TB's
>> hardware address and none that would let me translate the virtual
>> addresses we receive in the discon callback, which we need to compare
>> against.
>>
> 
> It would be acceptable to add such a function allowing to query physical
> address for a virtual address (using cpu_get_phys_page_debug behind the
> hoods), as it's not leaking any QEMU implementation detail.
> 
> We can implement this later if you don't want to extend your series with
> this.
>

An happy coincidence, this was just posted:
https://lore.kernel.org/qemu-devel/20250521094333.4075796-6-rowanbhart@gmail.com/

>> I considered also passing the hardware address to the callback (do the
>> translation in the `plugin_vcpu_cb__discon` hook), but that turned out
>> to be not straight forward and not something we'd want to do in the
>> hook, either.
>>
> 
> Yes, in some cases, people will want virtual addresses, and sometimes
> physical ones. So passing physical ones only is too restrictive.
> 
> This plugin is a bit specific, as it's explicitely tracking all
> transitions between instructions, where a "normal" plugin will just work
> with discontinuities. That said, the use case to get physical address
> from a virtual one is a real need.
> 
>> Regards,
>> Julian
> 
> Regards,
> Pierrick