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

["Alex Bennée" <alex.bennee@linaro.org>]

Julian Ganz <neither@nut.email> writes:

> 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.
>
> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Signed-off-by: Julian Ganz <neither@nut.email>
> ---
>  tests/tcg/plugins/discons.c   | 210 ++++++++++++++++++++++++++++++++++
>  tests/tcg/plugins/meson.build |   2 +-
>  2 files changed, 211 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..f185e3948b
> --- /dev/null
> +++ b/tests/tcg/plugins/discons.c
> @@ -0,0 +1,210 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + * Copyright (C) 2025, Julian Ganz <neither@nut.email>
> + *
> + * 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 bool addr_eq(uint64_t a, uint64_t b)
> +{
> +    if (a == b) {
> +        return true;
> +    }
> +
> +    uint64_t a_hw;
> +    uint64_t b_hw;
> +    if (!qemu_plugin_translate_vaddr(a, &a_hw) ||
> +        !qemu_plugin_translate_vaddr(b, &b_hw))
> +    {
> +        return false;
> +    }
> +
> +    return a_hw == b_hw;
> +}
> +
> +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;

This could be:

  g_autoptr(GString) buf = g_string_new(NULL);

> +    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);

I think we might want to flush here because

> +    if (abort_on_mismatch) {
> +        g_abort();
> +    }

This is firing on:

  🕙17:35:50 alex@draig:tests/tcg/i386-linux-user  on  review/tcg-discon-v6 [$!?] 
  ➜  make run-plugin-catch-syscalls-with-libdiscons.so V=1
  timeout -s KILL --foreground 120  env QEMU=/home/alex/lsrc/qemu.git/builds/sanitisers/qemu-i386 /home/alex/lsrc/qemu.git/builds/sanitisers/qemu-i386  -plugin ../plugins/libdiscons.so -d plugin -D catch-syscalls-with-libdiscons.so.pout catch-syscalls >  run-plugin-catch-syscalls-with-libdiscons.so.out
  Aborted
  make: *** [Makefile:226: run-plugin-catch-syscalls-with-libdiscons.so] Error 134
  🕙17:35:52 alex@draig:tests/tcg/i386-linux-user  on  review/tcg-discon-v6 [$!?] [🔴 USAGE] 
  ✗

although it never gets to the point of reporting what failed:

  Thread 1 "qemu-i386" hit Breakpoint 1, __GI_abort () at ./stdlib/abort.c:72
  warning: 72     ./stdlib/abort.c: No such file or directory
  (gdb) bt
  #0  __GI_abort () at ./stdlib/abort.c:72
  #1  0x00007ffff630874d in report_mismatch (pc_name=0x7ffff630a220 "target", vcpu_index=0, type=QEMU_PLUGIN_DISCON_EXCEPTION, last=134574955, expected=134574953, 
      encountered=134574955) at ../../tests/tcg/plugins/discons.c:89
  #2  0x00007ffff6308c0d in insn_exec (vcpu_index=0, userdata=0x0) at ../../tests/tcg/plugins/discons.c:132
  #3  0x00007fffea431114 in code_gen_buffer ()
  #4  0x000055555577b0a6 in cpu_tb_exec (cpu=0x529000005200, itb=0x7fffea431000 <code_gen_buffer+200659>, tb_exit=0x7ffff49c9530) at ../../accel/tcg/cpu-exec.c:438
  #5  0x000055555577c92f in cpu_loop_exec_tb (cpu=0x529000005200, tb=0x7fffea431000 <code_gen_buffer+200659>, pc=134574955, last_tb=0x7ffff49c9540, tb_exit=0x7ffff49c9530)
      at ../../accel/tcg/cpu-exec.c:871
  #6  0x000055555577d151 in cpu_exec_loop (cpu=0x529000005200, sc=0x7ffff483a740) at ../../accel/tcg/cpu-exec.c:981
  #7  0x000055555577d2fe in cpu_exec_setjmp (cpu=0x529000005200, sc=0x7ffff483a740) at ../../accel/tcg/cpu-exec.c:998
  #8  0x000055555577d4c8 in cpu_exec (cpu=0x529000005200) at ../../accel/tcg/cpu-exec.c:1024
  #9  0x00005555557bfc83 in cpu_loop (env=0x529000007dd0) at ../../linux-user/i386/cpu_loop.c:215
  #10 0x00005555558ee3e1 in main (argc=4, argv=0x7fffffffe688, envp=0x7fffffffe6b0) at ../../linux-user/main.c:1038
  (gdb) f 1
  #1  0x00007ffff630874d in report_mismatch (pc_name=0x7ffff630a220 "target", vcpu_index=0, type=QEMU_PLUGIN_DISCON_EXCEPTION, last=134574955, expected=134574953, 
      encountered=134574955) at ../../tests/tcg/plugins/discons.c:89
  89              g_abort();
  (gdb) p report
  $1 = (GString *) 0x50300002bf00
  (gdb) p report->Str
  There is no member named Str.
  (gdb) p report->str
  $2 = (gchar *) 0x51100001fbc0 "Discon target PC mismatch on VCPU 0\nExpected:      8057369\nEncountered:   805736b\nExecuted Last: 805736b\nEvent type:    exception\n"
  (gdb) 

I think this is where it is going wrong:

  IN: _dl_early_allocate
  0x0805736b:  89 c2                    movl     %eax, %edx
  0x0805736d:  8d 1c 28                 leal     (%eax, %ebp), %ebx
  0x08057370:  89 c8                    movl     %ecx, %eax
  0x08057372:  cd 80                    int      $0x80

> +    g_string_free(report, true);

so we could drop this... or..


> +}
> +
> +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;
> +
> +    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 {
> +            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 61a007d9e7..561584159e 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', 'patch']
> +  foreach i : ['bb', 'discons', 'empty', 'inline', 'insn', 'mem', 'reset', 'syscall', 'patch']
>      if host_os == 'windows'
>        t += shared_module(i, files(i + '.c') + '../../../contrib/plugins/win32_linker.c',
>                          include_directories: '../../../include/qemu',

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro