From: Harry van Haaren <[email protected]> The existing code in execlog was never upgraded to the Scoreboard API, resulting in a bespoke implementation of per-vCPU datastructure handling. This had some race-conditions, and causes segfaults with a simple multi-threaded program and two instances of execlog running.
The patch here refactors the custom GArray and GRWLock code away, and uses the scoreboard APIs like the other plugins. This solves the "printing while expanding" race-condition of two plugins with multiple threads in the guest, and hence fixes a segfault. Output remains atomic per instruction by building the full line (including the trailing newline) in the per-vCPU GString before making a single qemu_plugin_outs() call, relying on QEMU's own log locking rather than an additional mutex. Signed-off-by: Harry van Haaren <[email protected]> --- contrib/plugins/execlog.c | 68 +++++++++++++-------------------------- 1 file changed, 23 insertions(+), 45 deletions(-) diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c index 74325495cc..dfe00bf836 100644 --- a/contrib/plugins/execlog.c +++ b/contrib/plugins/execlog.c @@ -31,8 +31,12 @@ typedef struct CPU { QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; -static GArray *cpus; -static GRWLock expand_array_lock; +/* + * Per-vCPU state stored in a qemu_plugin_scoreboard. The scoreboard manages + * per-vCPU storage automatically, eliminating the need for manual array + * growth, locks, or pointer-stability workarounds. + */ +static struct qemu_plugin_scoreboard *cpus; static GPtrArray *imatches; static GArray *amatches; @@ -41,23 +45,13 @@ static bool disas_assist; static GMutex add_reg_name_lock; static GPtrArray *all_reg_names; -static CPU *get_cpu(int vcpu_index) -{ - CPU *c; - g_rw_lock_reader_lock(&expand_array_lock); - c = &g_array_index(cpus, CPU, vcpu_index); - g_rw_lock_reader_unlock(&expand_array_lock); - - return c; -} - /** * Add memory read or write information to current instruction log */ static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t info, uint64_t vaddr, void *udata) { - CPU *c = get_cpu(cpu_index); + CPU *c = qemu_plugin_scoreboard_find(cpus, cpu_index); GString *s = c->last_exec; /* Find vCPU in array */ @@ -117,7 +111,7 @@ static void insn_check_regs(CPU *cpu) /* Log last instruction while checking registers */ static void vcpu_insn_exec_with_regs(unsigned int cpu_index, void *udata) { - CPU *cpu = get_cpu(cpu_index); + CPU *cpu = qemu_plugin_scoreboard_find(cpus, cpu_index); /* Print previous instruction in cache */ if (cpu->last_exec->len) { @@ -125,8 +119,8 @@ static void vcpu_insn_exec_with_regs(unsigned int cpu_index, void *udata) insn_check_regs(cpu); } + g_string_append_c(cpu->last_exec, '\n'); qemu_plugin_outs(cpu->last_exec->str); - qemu_plugin_outs("\n"); } /* Store new instruction in cache */ @@ -138,7 +132,7 @@ static void vcpu_insn_exec_with_regs(unsigned int cpu_index, void *udata) /* Log last instruction while checking registers, ignore next */ static void vcpu_insn_exec_only_regs(unsigned int cpu_index, void *udata) { - CPU *cpu = get_cpu(cpu_index); + CPU *cpu = qemu_plugin_scoreboard_find(cpus, cpu_index); /* Print previous instruction in cache */ if (cpu->last_exec->len) { @@ -146,8 +140,8 @@ static void vcpu_insn_exec_only_regs(unsigned int cpu_index, void *udata) insn_check_regs(cpu); } + g_string_append_c(cpu->last_exec, '\n'); qemu_plugin_outs(cpu->last_exec->str); - qemu_plugin_outs("\n"); } /* reset */ @@ -157,12 +151,12 @@ static void vcpu_insn_exec_only_regs(unsigned int cpu_index, void *udata) /* Log last instruction without checking regs, setup next */ static void vcpu_insn_exec(unsigned int cpu_index, void *udata) { - CPU *cpu = get_cpu(cpu_index); + CPU *cpu = qemu_plugin_scoreboard_find(cpus, cpu_index); /* Print previous instruction in cache */ if (cpu->last_exec->len) { + g_string_append_c(cpu->last_exec, '\n'); qemu_plugin_outs(cpu->last_exec->str); - qemu_plugin_outs("\n"); } /* Store new instruction in cache */ @@ -378,40 +372,28 @@ static GPtrArray *registers_init(int vcpu_index) * - last_exec tracking data * - list of tracked registers * - initial value of registers - * - * As we could have multiple threads trying to do this we need to - * serialise the expansion under a lock. */ static void vcpu_init(unsigned int vcpu_index, void *userdata) { - CPU *c; - - g_rw_lock_writer_lock(&expand_array_lock); - if (vcpu_index >= cpus->len) { - g_array_set_size(cpus, vcpu_index + 1); - } - g_rw_lock_writer_unlock(&expand_array_lock); - - c = get_cpu(vcpu_index); + CPU *c = qemu_plugin_scoreboard_find(cpus, vcpu_index); c->last_exec = g_string_new(NULL); c->registers = registers_init(vcpu_index); } /** - * On plugin exit, print last instruction in cache + * On plugin exit, flush any remaining cached instructions and free state. */ static void plugin_exit(void *p) { - guint i; - g_rw_lock_reader_lock(&expand_array_lock); - for (i = 0; i < cpus->len; i++) { - CPU *c = get_cpu(i); - if (c->last_exec && c->last_exec->str) { + int n = qemu_plugin_num_vcpus(); + for (int i = 0; i < n; i++) { + CPU *c = qemu_plugin_scoreboard_find(cpus, i); + if (c->last_exec && c->last_exec->len) { + g_string_append_c(c->last_exec, '\n'); qemu_plugin_outs(c->last_exec->str); - qemu_plugin_outs("\n"); } } - g_rw_lock_reader_unlock(&expand_array_lock); + qemu_plugin_scoreboard_free(cpus); } /* Add a match to the array of matches */ @@ -452,12 +434,8 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, const qemu_info_t *info, int argc, char **argv) { - /* - * Initialize dynamic array to cache vCPU instruction. In user mode - * we don't know the size before emulation. - */ - cpus = g_array_sized_new(true, true, sizeof(CPU), - info->system_emulation ? info->system.max_vcpus : 1); + /* Initialize scoreboard to cache per-vCPU instruction state. */ + cpus = qemu_plugin_scoreboard_new(sizeof(CPU)); for (int i = 0; i < argc; i++) { char *opt = argv[i]; -- 2.54.0
