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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 5/11/25 6:14 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>
> ---
>   tests/tcg/plugins/discons.c   | 219 ++++++++++++++++++++++++++++++++++
>   tests/tcg/plugins/meson.build |   2 +-
>   2 files changed, 220 insertions(+), 1 deletion(-)
>   create mode 100644 tests/tcg/plugins/discons.c
> 

[...]

> +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);
> +
> +    switch (type) {
> +    case QEMU_PLUGIN_DISCON_EXCEPTION:
> +        /*
> +         * For some types of exceptions, insn_exec will be called for the
> +         * instruction that caused the exception.
> +         */
> +        if (addr_eq(state->last_pc, from_pc)) {
> +            break;
> +        }
> +        __attribute__((fallthrough));

It's a bit hard to follow the codepath with the switch and the 
fallthrough. Maybe we can simply use an empty if for that.

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, so we don't report this
    * case.
    */
} else if (state->has_next) {
   ...
} else if (state->has_from) {
   ...
}

...
set state
...

> +    default:
> +        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);
> +    struct insn_data* insn = (struct insn_data *) userdata;
> +
> +    state->last_pc = insn->addr;
> +    state->has_last = true;
> +
> +    if (insn->next_valid) {
> +        state->from_pc = insn->next_pc;
> +    }
> +    state->has_from = insn->next_valid;
> +
> +    if (state->has_next) {
> +        report_mismatch("target", vcpu_index, state->next_type, state->last_pc,
> +                        state->next_pc, insn->addr);
> +        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",
> +                               insn->addr, vcpu_index);
> +        qemu_plugin_outs(report->str);
> +    }
> +}
> +
> +static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
> +{
> +    size_t i;
> +    size_t n_insns = qemu_plugin_tb_n_insns(tb);
> +    struct insn_data *udata = calloc(n_insns, sizeof(struct insn_data));
> +

With this, for every TB translated, we'll perform an allocation, and 
then lose track of the pointer. It's usually a pain to pass this kind of 
"dynamic" information through udata.

A more elegant solution is to perform a QEMU_PLUGIN_INLINE_STORE_U64 to 
store this information under a new cpu_state.current_insn field directly.
Callbacks are installed in the order you register them, so by storing 
information inline *before* the insn_exec callback, it will work as 
expected, as cpu_static.current_insn will be already updated.
You can find some other plugins which use this trick.

> +    for (i = 0; i < n_insns; i++) {

Feel free to declare i in the loop directly.

> +        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
> +        uint64_t pc = qemu_plugin_insn_vaddr(insn);
> +        udata[i].addr = pc;
> +        udata[i].next_pc = pc + qemu_plugin_insn_size(insn);
> +        udata[i].next_valid = true;
> +        qemu_plugin_register_vcpu_insn_exec_cb(insn, insn_exec,
> +                                               QEMU_PLUGIN_CB_NO_REGS,
> +                                               &udata[i]);
> +    }
> +
> +    udata[n_insns - 1].next_valid = false;
> +}

[...]

Otherwise, the logic of the plugin is ok for me.

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