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. Signed-off-by: Harry van Haaren <[email protected]> --- Hi Qemu-Devel! This is my first contribution to QEMU itself, but I have worked on DPDK, OVS, and QEMU-RS a bit in the past. I have experience with mailing lists for patches and some QEMU internals. I've read the 'submitting a patch' guide, and ran checkpatch... so basics should be OK I hope! Please note that LLMs were used to investigate and fix this bug, but all code in this patch has been reviewed by me, and I believe it to be a good solution. --- contrib/plugins/execlog.c | 65 +++++++++++++++------------------------ 1 file changed, 25 insertions(+), 40 deletions(-) diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c index d00d9c4ff3..e51af9f2df 100644 --- a/contrib/plugins/execlog.c +++ b/contrib/plugins/execlog.c @@ -31,33 +31,28 @@ 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; static GPtrArray *rmatches; static bool disas_assist; static GMutex add_reg_name_lock; +static GMutex execlog_output_mutex; 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 +112,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 +120,10 @@ static void vcpu_insn_exec_with_regs(unsigned int cpu_index, void *udata) insn_check_regs(cpu); } + g_mutex_lock(&execlog_output_mutex); qemu_plugin_outs(cpu->last_exec->str); qemu_plugin_outs("\n"); + g_mutex_unlock(&execlog_output_mutex); } /* Store new instruction in cache */ @@ -138,7 +135,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 +143,10 @@ static void vcpu_insn_exec_only_regs(unsigned int cpu_index, void *udata) insn_check_regs(cpu); } + g_mutex_lock(&execlog_output_mutex); qemu_plugin_outs(cpu->last_exec->str); qemu_plugin_outs("\n"); + g_mutex_unlock(&execlog_output_mutex); } /* reset */ @@ -157,12 +156,14 @@ 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_mutex_lock(&execlog_output_mutex); qemu_plugin_outs(cpu->last_exec->str); qemu_plugin_outs("\n"); + g_mutex_unlock(&execlog_output_mutex); } /* Store new instruction in cache */ @@ -378,21 +379,10 @@ 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(qemu_plugin_id_t id, unsigned int vcpu_index) { - 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); } @@ -402,16 +392,15 @@ static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index) */ static void plugin_exit(qemu_plugin_id_t id, 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) { 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 +441,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 The content, data, and any attached documents to this email are addressed exclusively to the addressee and are confidential and/or may be subject to a non-disclosure agreement. Any use, forwarding, disclosure, and/or copying, in whole or in part, without authorization is prohibited. If you have received this email in error, we apologize and, please notify the sender or Openchip immediately, and delete it from your system. El contenido, los datos y cualquier documento adjunto a este correo electrónico están dirigidos exclusivamente al destinatario y son confidenciales y/o pueden estar sujetas a un acuerdo de no revelación. Está prohibido cualquier uso, reenvío, divulgación o copia, total o parcial, sin autorización. Si has recibido este correo por error, te pedimos disculpas y agradecemos que lo notifiques de inmediato al remitente o a Openchip, y lo elimines de tu sistema. El contingut, les dades i qualsevol document adjunt a aquest correu electrònic estan dirigits exclusivament al destinatari i són confidencials i/o poden estar subjectes a un acord de no revelació. Està prohibit qualsevol ús, reenviament, divulgació o còpia, total o parcial, sense autorització. Si has rebut aquest correu per error, et demanem disculpes i agraïm que ho notifiquis d'immediat al remitent o a Openchip, i l'eliminis del teu sistema
