Message ID | 20240228115701.1416107-27-alex.bennee@linaro.org |
---|---|
State | New |
Headers | show |
Series | [PULL,01/29] tests/tcg: update licenses to GPLv2 as intended | expand |
Hi Alex, I hit the following warnings (with "./configure --enable-werror"): /qemu/contrib/plugins/execlog.c: In function ‘registers_init’: /qemu/contrib/plugins/execlog.c:330:17: warning: ‘g_pattern_match_string’ is deprecated: Use 'g_pattern_spec_match_string' instead [-Wdeprecated-declarations] 330 | if (g_pattern_match_string(pat, rd->name) || | ^~ In file included from /usr/include/glib-2.0/glib.h:65, from /qemu/contrib/plugins/execlog.c:9: /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here 55 | gboolean g_pattern_match_string (GPatternSpec *pspec, | ^~~~~~~~~~~~~~~~~~~~~~ /qemu/contrib/plugins/execlog.c:331:21: warning: ‘g_pattern_match_string’ is deprecated: Use 'g_pattern_spec_match_string' instead [-Wdeprecated-declarations] 331 | g_pattern_match_string(pat, rd_lower)) { | ^~~~~~~~~~~~~~~~~~~~~~ In file included from /usr/include/glib-2.0/glib.h:65, from /qemu/contrib/plugins/execlog.c:9: /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here 55 | gboolean g_pattern_match_string (GPatternSpec *pspec, | ^~~~~~~~~~~~~~~~~~~~~~ /qemu/contrib/plugins/execlog.c:339:63: warning: passing argument 2 of ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] 339 | g_ptr_array_add(all_reg_names, reg->name); | ~~~^~~~~~ In file included from /usr/include/glib-2.0/glib.h:31, from /qemu/contrib/plugins/execlog.c:9: /usr/include/glib-2.0/glib/garray.h:192:62: note: expected ‘gpointer’ {aka ‘void *’} but argument is of type ‘const char *’ 192 | gpointer data); | ~~~~~~~~~~~~~~~~~~^~~~ In addition, I checked my glic version: $ldd --version ldd (Ubuntu GLIBC 2.35-0ubuntu3.5) 2.35 I think it's v2.35. Are these three warning reports valid? I also noticed in target/arm/helper.c, there's another g_pattern_match_string() but I haven't met the warning. Regards, Zhao On Wed, Feb 28, 2024 at 11:56:58AM +0000, Alex Bennée wrote: > Date: Wed, 28 Feb 2024 11:56:58 +0000 > From: Alex Bennée <alex.bennee@linaro.org> > Subject: [PULL 26/29] contrib/plugins: extend execlog to track register > changes > X-Mailer: git-send-email 2.39.2 > > With the new plugin register API we can now track changes to register > values. Currently the implementation is fairly dumb which will slow > down if a large number of register values are being tracked. This > could be improved by only instrumenting instructions which mention > registers we are interested in tracking. > > Example usage: > > ./qemu-aarch64 -D plugin.log -d plugin \ > -cpu max,sve256=on \ > -plugin contrib/plugins/libexeclog.so,reg=sp,reg=z\* \ > ./tests/tcg/aarch64-linux-user/sha512-sve > > will display in the execlog any changes to the stack pointer (sp) and > the SVE Z registers. > > As testing registers every instruction will be quite a heavy operation > there is an additional flag which attempts to optimise the register > tracking by only instrumenting instructions which are likely to change > its value. This relies on the QEMU disassembler showing up the register > names in disassembly so is an explicit opt-in. > > Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > Cc: Akihiko Odaki <akihiko.odaki@daynix.com> > Based-On: <20231025093128.33116-19-akihiko.odaki@daynix.com> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Message-Id: <20240227144335.1196131-27-alex.bennee@linaro.org> > > diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst > index 81dcd43a612..fa7421279f5 100644 > --- a/docs/devel/tcg-plugins.rst > +++ b/docs/devel/tcg-plugins.rst > @@ -497,6 +497,22 @@ arguments if required:: > $ qemu-system-arm $(QEMU_ARGS) \ > -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin > > +This plugin can also dump registers when they change value. Specify the name of the > +registers with multiple ``reg`` options. You can also use glob style matching if you wish:: > + > + $ qemu-system-arm $(QEMU_ARGS) \ > + -plugin ./contrib/plugins/libexeclog.so,reg=\*_el2,reg=sp -d plugin > + > +Be aware that each additional register to check will slow down > +execution quite considerably. You can optimise the number of register > +checks done by using the rdisas option. This will only instrument > +instructions that mention the registers in question in disassembly. > +This is not foolproof as some instructions implicitly change > +instructions. You can use the ifilter to catch these cases: > + > + $ qemu-system-arm $(QEMU_ARGS) \ > + -plugin ./contrib/plugins/libexeclog.so,ifilter=msr,ifilter=blr,reg=x30,reg=\*_el1,rdisas=on > + > - contrib/plugins/cache.c > > Cache modelling plugin that measures the performance of a given L1 cache > @@ -583,4 +599,3 @@ The following API is generated from the inline documentation in > include the full kernel-doc annotations. > > .. kernel-doc:: include/qemu/qemu-plugin.h > - > diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c > index f262e5555eb..a1dfd59ab71 100644 > --- a/contrib/plugins/execlog.c > +++ b/contrib/plugins/execlog.c > @@ -1,7 +1,7 @@ > /* > * Copyright (C) 2021, Alexandre Iooss <erdnaxe@crans.org> > * > - * Log instruction execution with memory access. > + * Log instruction execution with memory access and register changes > * > * License: GNU GPL, version 2 or later. > * See the COPYING file in the top-level directory. > @@ -15,29 +15,40 @@ > > #include <qemu-plugin.h> > > +typedef struct { > + struct qemu_plugin_register *handle; > + GByteArray *last; > + GByteArray *new; > + const char *name; > +} Register; > + > +typedef struct CPU { > + /* Store last executed instruction on each vCPU as a GString */ > + GString *last_exec; > + /* Ptr array of Register */ > + GPtrArray *registers; > +} CPU; > + > QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; > > -/* Store last executed instruction on each vCPU as a GString */ > -static GPtrArray *last_exec; > +static GArray *cpus; > static GRWLock expand_array_lock; > > static GPtrArray *imatches; > static GArray *amatches; > +static GPtrArray *rmatches; > +static bool disas_assist; > +static GMutex add_reg_name_lock; > +static GPtrArray *all_reg_names; > > -/* > - * Expand last_exec array. > - * > - * As we could have multiple threads trying to do this we need to > - * serialise the expansion under a lock. > - */ > -static void expand_last_exec(int cpu_index) > +static CPU *get_cpu(int vcpu_index) > { > - g_rw_lock_writer_lock(&expand_array_lock); > - while (cpu_index >= last_exec->len) { > - GString *s = g_string_new(NULL); > - g_ptr_array_add(last_exec, s); > - } > - g_rw_lock_writer_unlock(&expand_array_lock); > + 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; > } > > /** > @@ -46,13 +57,10 @@ static void expand_last_exec(int cpu_index) > static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t info, > uint64_t vaddr, void *udata) > { > - GString *s; > + CPU *c = get_cpu(cpu_index); > + GString *s = c->last_exec; > > /* Find vCPU in array */ > - g_rw_lock_reader_lock(&expand_array_lock); > - g_assert(cpu_index < last_exec->len); > - s = g_ptr_array_index(last_exec, cpu_index); > - g_rw_lock_reader_unlock(&expand_array_lock); > > /* Indicate type of memory access */ > if (qemu_plugin_mem_is_store(info)) { > @@ -73,32 +81,91 @@ static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t info, > } > > /** > - * Log instruction execution > + * Log instruction execution, outputting the last one. > + * > + * vcpu_insn_exec() is a copy and paste of vcpu_insn_exec_with_regs() > + * without the checking of register values when we've attempted to > + * optimise with disas_assist. > */ > -static void vcpu_insn_exec(unsigned int cpu_index, void *udata) > +static void insn_check_regs(CPU *cpu) > { > - GString *s; > + for (int n = 0; n < cpu->registers->len; n++) { > + Register *reg = cpu->registers->pdata[n]; > + int sz; > > - /* Find or create vCPU in array */ > - g_rw_lock_reader_lock(&expand_array_lock); > - if (cpu_index >= last_exec->len) { > - g_rw_lock_reader_unlock(&expand_array_lock); > - expand_last_exec(cpu_index); > - g_rw_lock_reader_lock(&expand_array_lock); > + g_byte_array_set_size(reg->new, 0); > + sz = qemu_plugin_read_register(reg->handle, reg->new); > + g_assert(sz == reg->last->len); > + > + if (memcmp(reg->last->data, reg->new->data, sz)) { > + GByteArray *temp = reg->last; > + g_string_append_printf(cpu->last_exec, ", %s -> 0x", reg->name); > + /* TODO: handle BE properly */ > + for (int i = sz; i >= 0; i--) { > + g_string_append_printf(cpu->last_exec, "%02x", > + reg->new->data[i]); > + } > + reg->last = reg->new; > + reg->new = temp; > + } > + } > +} > + > +/* 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); > + > + /* Print previous instruction in cache */ > + if (cpu->last_exec->len) { > + if (cpu->registers) { > + insn_check_regs(cpu); > + } > + > + qemu_plugin_outs(cpu->last_exec->str); > + qemu_plugin_outs("\n"); > + } > + > + /* Store new instruction in cache */ > + /* vcpu_mem will add memory access information to last_exec */ > + g_string_printf(cpu->last_exec, "%u, ", cpu_index); > + g_string_append(cpu->last_exec, (char *)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); > + > + /* Print previous instruction in cache */ > + if (cpu->last_exec->len) { > + if (cpu->registers) { > + insn_check_regs(cpu); > + } > + > + qemu_plugin_outs(cpu->last_exec->str); > + qemu_plugin_outs("\n"); > } > - s = g_ptr_array_index(last_exec, cpu_index); > - g_rw_lock_reader_unlock(&expand_array_lock); > + > + /* reset */ > + cpu->last_exec->len = 0; > +} > + > +/* 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); > > /* Print previous instruction in cache */ > - if (s->len) { > - qemu_plugin_outs(s->str); > + if (cpu->last_exec->len) { > + qemu_plugin_outs(cpu->last_exec->str); > qemu_plugin_outs("\n"); > } > > /* Store new instruction in cache */ > /* vcpu_mem will add memory access information to last_exec */ > - g_string_printf(s, "%u, ", cpu_index); > - g_string_append(s, (char *)udata); > + g_string_printf(cpu->last_exec, "%u, ", cpu_index); > + g_string_append(cpu->last_exec, (char *)udata); > } > > /** > @@ -111,6 +178,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) > { > struct qemu_plugin_insn *insn; > bool skip = (imatches || amatches); > + bool check_regs_this = rmatches; > + bool check_regs_next = false; > > size_t n = qemu_plugin_tb_n_insns(tb); > for (size_t i = 0; i < n; i++) { > @@ -131,7 +200,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) > /* > * If we are filtering we better check out if we have any > * hits. The skip "latches" so we can track memory accesses > - * after the instruction we care about. > + * after the instruction we care about. Also enable register > + * checking on the next instruction. > */ > if (skip && imatches) { > int j; > @@ -139,6 +209,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) > char *m = g_ptr_array_index(imatches, j); > if (g_str_has_prefix(insn_disas, m)) { > skip = false; > + check_regs_next = rmatches; > } > } > } > @@ -153,8 +224,39 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) > } > } > > + /* > + * Check the disassembly to see if a register we care about > + * will be affected by this instruction. This relies on the > + * dissembler doing something sensible for the registers we > + * care about. > + */ > + if (disas_assist && rmatches) { > + check_regs_next = false; > + gchar *args = g_strstr_len(insn_disas, -1, " "); > + for (int n = 0; n < all_reg_names->len; n++) { > + gchar *reg = g_ptr_array_index(all_reg_names, n); > + if (g_strrstr(args, reg)) { > + check_regs_next = true; > + skip = false; > + } > + } > + } > + > + /* > + * We now have 3 choices: > + * > + * - Log insn > + * - Log insn while checking registers > + * - Don't log this insn but check if last insn changed registers > + */ > + > if (skip) { > - g_free(insn_disas); > + if (check_regs_this) { > + qemu_plugin_register_vcpu_insn_exec_cb(insn, > + vcpu_insn_exec_only_regs, > + QEMU_PLUGIN_CB_R_REGS, > + NULL); > + } > } else { > uint32_t insn_opcode; > insn_opcode = *((uint32_t *)qemu_plugin_insn_data(insn)); > @@ -167,30 +269,124 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) > QEMU_PLUGIN_MEM_RW, NULL); > > /* Register callback on instruction */ > - qemu_plugin_register_vcpu_insn_exec_cb(insn, vcpu_insn_exec, > - QEMU_PLUGIN_CB_NO_REGS, output); > + if (check_regs_this) { > + qemu_plugin_register_vcpu_insn_exec_cb( > + insn, vcpu_insn_exec_with_regs, > + QEMU_PLUGIN_CB_R_REGS, > + output); > + } else { > + qemu_plugin_register_vcpu_insn_exec_cb( > + insn, vcpu_insn_exec, > + QEMU_PLUGIN_CB_NO_REGS, > + output); > + } > > /* reset skip */ > skip = (imatches || amatches); > } > > + /* set regs for next */ > + if (disas_assist && rmatches) { > + check_regs_this = check_regs_next; > + } > + > + g_free(insn_disas); > } > } > > +static Register *init_vcpu_register(qemu_plugin_reg_descriptor *desc) > +{ > + Register *reg = g_new0(Register, 1); > + g_autofree gchar *lower = g_utf8_strdown(desc->name, -1); > + int r; > + > + reg->handle = desc->handle; > + reg->name = g_intern_string(lower); > + reg->last = g_byte_array_new(); > + reg->new = g_byte_array_new(); > + > + /* read the initial value */ > + r = qemu_plugin_read_register(reg->handle, reg->last); > + g_assert(r > 0); > + return reg; > +} > + > +static GPtrArray *registers_init(int vcpu_index) > +{ > + g_autoptr(GPtrArray) registers = g_ptr_array_new(); > + g_autoptr(GArray) reg_list = qemu_plugin_get_registers(); > + > + if (rmatches && reg_list->len) { > + /* > + * Go through each register in the complete list and > + * see if we want to track it. > + */ > + for (int r = 0; r < reg_list->len; r++) { > + qemu_plugin_reg_descriptor *rd = &g_array_index( > + reg_list, qemu_plugin_reg_descriptor, r); > + for (int p = 0; p < rmatches->len; p++) { > + g_autoptr(GPatternSpec) pat = g_pattern_spec_new(rmatches->pdata[p]); > + g_autofree gchar *rd_lower = g_utf8_strdown(rd->name, -1); > + if (g_pattern_match_string(pat, rd->name) || > + g_pattern_match_string(pat, rd_lower)) { > + Register *reg = init_vcpu_register(rd); > + g_ptr_array_add(registers, reg); > + > + /* we need a list of regnames at TB translation time */ > + if (disas_assist) { > + g_mutex_lock(&add_reg_name_lock); > + if (!g_ptr_array_find(all_reg_names, reg->name, NULL)) { > + g_ptr_array_add(all_reg_names, reg->name); > + } > + g_mutex_unlock(&add_reg_name_lock); > + } > + } > + } > + } > + } > + > + return registers->len ? g_steal_pointer(®isters) : NULL; > +} > + > +/* > + * Initialise a new vcpu/thread with: > + * - 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); > + c->last_exec = g_string_new(NULL); > + c->registers = registers_init(vcpu_index); > +} > + > /** > * On plugin exit, print last instruction in cache > */ > static void plugin_exit(qemu_plugin_id_t id, void *p) > { > guint i; > - GString *s; > - for (i = 0; i < last_exec->len; i++) { > - s = g_ptr_array_index(last_exec, i); > - if (s->str) { > - qemu_plugin_outs(s->str); > + 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) { > + qemu_plugin_outs(c->last_exec->str); > qemu_plugin_outs("\n"); > } > } > + g_rw_lock_reader_unlock(&expand_array_lock); > } > > /* Add a match to the array of matches */ > @@ -212,6 +408,18 @@ static void parse_vaddr_match(char *match) > g_array_append_val(amatches, v); > } > > +/* > + * We have to wait until vCPUs are started before we can check the > + * patterns find anything. > + */ > +static void add_regpat(char *regpat) > +{ > + if (!rmatches) { > + rmatches = g_ptr_array_new(); > + } > + g_ptr_array_add(rmatches, g_strdup(regpat)); > +} > + > /** > * Install the plugin > */ > @@ -223,11 +431,8 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, > * Initialize dynamic array to cache vCPU instruction. In user mode > * we don't know the size before emulation. > */ > - if (info->system_emulation) { > - last_exec = g_ptr_array_sized_new(info->system.max_vcpus); > - } else { > - last_exec = g_ptr_array_new(); > - } > + cpus = g_array_sized_new(true, true, sizeof(CPU), > + info->system_emulation ? info->system.max_vcpus : 1); > > for (int i = 0; i < argc; i++) { > char *opt = argv[i]; > @@ -236,13 +441,22 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, > parse_insn_match(tokens[1]); > } else if (g_strcmp0(tokens[0], "afilter") == 0) { > parse_vaddr_match(tokens[1]); > + } else if (g_strcmp0(tokens[0], "reg") == 0) { > + add_regpat(tokens[1]); > + } else if (g_strcmp0(tokens[0], "rdisas") == 0) { > + if (!qemu_plugin_bool_parse(tokens[0], tokens[1], &disas_assist)) { > + fprintf(stderr, "boolean argument parsing failed: %s\n", opt); > + return -1; > + } > + all_reg_names = g_ptr_array_new(); > } else { > fprintf(stderr, "option parsing failed: %s\n", opt); > return -1; > } > } > > - /* Register translation block and exit callbacks */ > + /* Register init, translation block and exit callbacks */ > + qemu_plugin_register_vcpu_init_cb(id, vcpu_init); > qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans); > qemu_plugin_register_atexit_cb(id, plugin_exit, NULL); > > -- > 2.39.2 > >
Zhao Liu <zhao1.liu@intel.com> writes: > Hi Alex, > > I hit the following warnings (with "./configure --enable-werror"): > > /qemu/contrib/plugins/execlog.c: In function ‘registers_init’: > /qemu/contrib/plugins/execlog.c:330:17: warning: ‘g_pattern_match_string’ is deprecated: Use 'g_pattern_spec_match_string' instead [-Wdeprecated-declarations] > 330 | if (g_pattern_match_string(pat, rd->name) || > | ^~ > In file included from /usr/include/glib-2.0/glib.h:65, > from /qemu/contrib/plugins/execlog.c:9: > /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here > 55 | gboolean g_pattern_match_string (GPatternSpec *pspec, > | ^~~~~~~~~~~~~~~~~~~~~~ > /qemu/contrib/plugins/execlog.c:331:21: warning: ‘g_pattern_match_string’ is deprecated: Use 'g_pattern_spec_match_string' instead [-Wdeprecated-declarations] > 331 | g_pattern_match_string(pat, rd_lower)) { > | ^~~~~~~~~~~~~~~~~~~~~~ > In file included from /usr/include/glib-2.0/glib.h:65, > from /qemu/contrib/plugins/execlog.c:9: > /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here > 55 | gboolean g_pattern_match_string (GPatternSpec *pspec, > | ^~~~~~~~~~~~~~~~~~~~~~ > /qemu/contrib/plugins/execlog.c:339:63: warning: passing argument 2 of ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] > 339 | g_ptr_array_add(all_reg_names, > reg->name); Hmm I missed that. Not sure what the neatest solution is in this case - g_ptr_array_new() doesn't have a destroy func so we shouldn't ever attempt to free it. We can manually re-add the const qualifier at the other end for completeness and I guess comment and cast? > | ~~~^~~~~~ > In file included from /usr/include/glib-2.0/glib.h:31, > from /qemu/contrib/plugins/execlog.c:9: > /usr/include/glib-2.0/glib/garray.h:192:62: note: expected ‘gpointer’ {aka ‘void *’} but argument is of type ‘const char *’ > 192 | gpointer data); > | ~~~~~~~~~~~~~~~~~~^~~~ > > In addition, I checked my glic version: > > $ldd --version > ldd (Ubuntu GLIBC 2.35-0ubuntu3.5) 2.35 > > I think it's v2.35. Are these three warning reports valid? It's the glib (not glibc) version that matters here. g_pattern_match_string was deprecated in 2.70 when the suggested alternative was added. However our baseline for glib is still: # When bumping glib minimum version, please check also whether to increase # the _WIN32_WINNT setting in osdep.h according to the value from glib glib_req_ver = '>=2.56.0' glib_pc = dependency('glib-2.0', version: glib_req_ver, required: true, method: 'pkg-config') The usual solution for this is to throw in a compat wrapper in glib-compat.h: --8<---------------cut here---------------start------------->8--- modified include/glib-compat.h @@ -105,6 +105,24 @@ static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size) } #define g_memdup2(m, s) g_memdup2_qemu(m, s) +/* + * g_pattern_match_string has been deprecated in Glib since 2.70 and + * will complain about it if you try to use it. Fortunately the + * signature of both functions is the same making it easy to work + * around. + */ +static inline +gboolean g_pattern_spec_match_string_qemu(GPatternSpec *pspec, + const gchar *string) +{ +#if GLIB_CHECK_VERSION(2, 70, 0) + return g_pattern_spec_match_string(pspec, string); +#else + return g_pattern_match_string(pspec, string); +#endif +}; +#define g_pattern_spec_match_string(p, s) g_pattern_spec_match_string_qemu(p, s) + #if defined(G_OS_UNIX) /* * Note: The fallback implementation is not MT-safe, and it returns a copy of modified contrib/plugins/execlog.c @@ -334,8 +334,8 @@ static void registers_init(int vcpu_index) for (int p = 0; p < rmatches->len; p++) { g_autoptr(GPatternSpec) pat = g_pattern_spec_new(rmatches->pdata[p]); g_autofree gchar *rd_lower = g_utf8_strdown(rd->name, -1); - if (g_pattern_match_string(pat, rd->name) || - g_pattern_match_string(pat, rd_lower)) { + if (g_pattern_spec_match_string(pat, rd->name) || + g_pattern_spec_match_string(pat, rd_lower)) { Register *reg = init_vcpu_register(vcpu_index, rd); g_ptr_array_add(registers, reg); --8<---------------cut here---------------end--------------->8--- but I hesitated to add it for this case as plugins shouldn't assume they have access to QEMU's internals. Maybe the glib-compat.h header could be treated as a special case. > > I also noticed in target/arm/helper.c, there's another > g_pattern_match_string() but I haven't met the warning. Hmm that's weird. I suspect glib suppresses the warnings with: /* Ask for warnings for anything that was marked deprecated in * the defined version, or before. It is a candidate for rewrite. */ #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_56 > > Regards, > Zhao > > On Wed, Feb 28, 2024 at 11:56:58AM +0000, Alex Bennée wrote: >> Date: Wed, 28 Feb 2024 11:56:58 +0000 >> From: Alex Bennée <alex.bennee@linaro.org> >> Subject: [PULL 26/29] contrib/plugins: extend execlog to track register >> changes >> X-Mailer: git-send-email 2.39.2 >> >> With the new plugin register API we can now track changes to register >> values. Currently the implementation is fairly dumb which will slow >> down if a large number of register values are being tracked. This >> could be improved by only instrumenting instructions which mention >> registers we are interested in tracking. >> >> Example usage: >> >> ./qemu-aarch64 -D plugin.log -d plugin \ >> -cpu max,sve256=on \ >> -plugin contrib/plugins/libexeclog.so,reg=sp,reg=z\* \ >> ./tests/tcg/aarch64-linux-user/sha512-sve >> >> will display in the execlog any changes to the stack pointer (sp) and >> the SVE Z registers. >> >> As testing registers every instruction will be quite a heavy operation >> there is an additional flag which attempts to optimise the register >> tracking by only instrumenting instructions which are likely to change >> its value. This relies on the QEMU disassembler showing up the register >> names in disassembly so is an explicit opt-in. >> >> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> Cc: Akihiko Odaki <akihiko.odaki@daynix.com> >> Based-On: <20231025093128.33116-19-akihiko.odaki@daynix.com> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Message-Id: <20240227144335.1196131-27-alex.bennee@linaro.org> >> >> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst >> index 81dcd43a612..fa7421279f5 100644 >> --- a/docs/devel/tcg-plugins.rst >> +++ b/docs/devel/tcg-plugins.rst >> @@ -497,6 +497,22 @@ arguments if required:: >> $ qemu-system-arm $(QEMU_ARGS) \ >> -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin >> >> +This plugin can also dump registers when they change value. Specify the name of the >> +registers with multiple ``reg`` options. You can also use glob style matching if you wish:: >> + >> + $ qemu-system-arm $(QEMU_ARGS) \ >> + -plugin ./contrib/plugins/libexeclog.so,reg=\*_el2,reg=sp -d plugin >> + >> +Be aware that each additional register to check will slow down >> +execution quite considerably. You can optimise the number of register >> +checks done by using the rdisas option. This will only instrument >> +instructions that mention the registers in question in disassembly. >> +This is not foolproof as some instructions implicitly change >> +instructions. You can use the ifilter to catch these cases: >> + >> + $ qemu-system-arm $(QEMU_ARGS) \ >> + -plugin ./contrib/plugins/libexeclog.so,ifilter=msr,ifilter=blr,reg=x30,reg=\*_el1,rdisas=on >> + >> - contrib/plugins/cache.c >> >> Cache modelling plugin that measures the performance of a given L1 cache >> @@ -583,4 +599,3 @@ The following API is generated from the inline documentation in >> include the full kernel-doc annotations. >> >> .. kernel-doc:: include/qemu/qemu-plugin.h >> - >> diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c >> index f262e5555eb..a1dfd59ab71 100644 >> --- a/contrib/plugins/execlog.c >> +++ b/contrib/plugins/execlog.c >> @@ -1,7 +1,7 @@ >> /* >> * Copyright (C) 2021, Alexandre Iooss <erdnaxe@crans.org> >> * >> - * Log instruction execution with memory access. >> + * Log instruction execution with memory access and register changes >> * >> * License: GNU GPL, version 2 or later. >> * See the COPYING file in the top-level directory. >> @@ -15,29 +15,40 @@ >> >> #include <qemu-plugin.h> >> >> +typedef struct { >> + struct qemu_plugin_register *handle; >> + GByteArray *last; >> + GByteArray *new; >> + const char *name; >> +} Register; >> + >> +typedef struct CPU { >> + /* Store last executed instruction on each vCPU as a GString */ >> + GString *last_exec; >> + /* Ptr array of Register */ >> + GPtrArray *registers; >> +} CPU; >> + >> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; >> >> -/* Store last executed instruction on each vCPU as a GString */ >> -static GPtrArray *last_exec; >> +static GArray *cpus; >> static GRWLock expand_array_lock; >> >> static GPtrArray *imatches; >> static GArray *amatches; >> +static GPtrArray *rmatches; >> +static bool disas_assist; >> +static GMutex add_reg_name_lock; >> +static GPtrArray *all_reg_names; >> >> -/* >> - * Expand last_exec array. >> - * >> - * As we could have multiple threads trying to do this we need to >> - * serialise the expansion under a lock. >> - */ >> -static void expand_last_exec(int cpu_index) >> +static CPU *get_cpu(int vcpu_index) >> { >> - g_rw_lock_writer_lock(&expand_array_lock); >> - while (cpu_index >= last_exec->len) { >> - GString *s = g_string_new(NULL); >> - g_ptr_array_add(last_exec, s); >> - } >> - g_rw_lock_writer_unlock(&expand_array_lock); >> + 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; >> } >> >> /** >> @@ -46,13 +57,10 @@ static void expand_last_exec(int cpu_index) >> static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t info, >> uint64_t vaddr, void *udata) >> { >> - GString *s; >> + CPU *c = get_cpu(cpu_index); >> + GString *s = c->last_exec; >> >> /* Find vCPU in array */ >> - g_rw_lock_reader_lock(&expand_array_lock); >> - g_assert(cpu_index < last_exec->len); >> - s = g_ptr_array_index(last_exec, cpu_index); >> - g_rw_lock_reader_unlock(&expand_array_lock); >> >> /* Indicate type of memory access */ >> if (qemu_plugin_mem_is_store(info)) { >> @@ -73,32 +81,91 @@ static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t info, >> } >> >> /** >> - * Log instruction execution >> + * Log instruction execution, outputting the last one. >> + * >> + * vcpu_insn_exec() is a copy and paste of vcpu_insn_exec_with_regs() >> + * without the checking of register values when we've attempted to >> + * optimise with disas_assist. >> */ >> -static void vcpu_insn_exec(unsigned int cpu_index, void *udata) >> +static void insn_check_regs(CPU *cpu) >> { >> - GString *s; >> + for (int n = 0; n < cpu->registers->len; n++) { >> + Register *reg = cpu->registers->pdata[n]; >> + int sz; >> >> - /* Find or create vCPU in array */ >> - g_rw_lock_reader_lock(&expand_array_lock); >> - if (cpu_index >= last_exec->len) { >> - g_rw_lock_reader_unlock(&expand_array_lock); >> - expand_last_exec(cpu_index); >> - g_rw_lock_reader_lock(&expand_array_lock); >> + g_byte_array_set_size(reg->new, 0); >> + sz = qemu_plugin_read_register(reg->handle, reg->new); >> + g_assert(sz == reg->last->len); >> + >> + if (memcmp(reg->last->data, reg->new->data, sz)) { >> + GByteArray *temp = reg->last; >> + g_string_append_printf(cpu->last_exec, ", %s -> 0x", reg->name); >> + /* TODO: handle BE properly */ >> + for (int i = sz; i >= 0; i--) { >> + g_string_append_printf(cpu->last_exec, "%02x", >> + reg->new->data[i]); >> + } >> + reg->last = reg->new; >> + reg->new = temp; >> + } >> + } >> +} >> + >> +/* 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); >> + >> + /* Print previous instruction in cache */ >> + if (cpu->last_exec->len) { >> + if (cpu->registers) { >> + insn_check_regs(cpu); >> + } >> + >> + qemu_plugin_outs(cpu->last_exec->str); >> + qemu_plugin_outs("\n"); >> + } >> + >> + /* Store new instruction in cache */ >> + /* vcpu_mem will add memory access information to last_exec */ >> + g_string_printf(cpu->last_exec, "%u, ", cpu_index); >> + g_string_append(cpu->last_exec, (char *)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); >> + >> + /* Print previous instruction in cache */ >> + if (cpu->last_exec->len) { >> + if (cpu->registers) { >> + insn_check_regs(cpu); >> + } >> + >> + qemu_plugin_outs(cpu->last_exec->str); >> + qemu_plugin_outs("\n"); >> } >> - s = g_ptr_array_index(last_exec, cpu_index); >> - g_rw_lock_reader_unlock(&expand_array_lock); >> + >> + /* reset */ >> + cpu->last_exec->len = 0; >> +} >> + >> +/* 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); >> >> /* Print previous instruction in cache */ >> - if (s->len) { >> - qemu_plugin_outs(s->str); >> + if (cpu->last_exec->len) { >> + qemu_plugin_outs(cpu->last_exec->str); >> qemu_plugin_outs("\n"); >> } >> >> /* Store new instruction in cache */ >> /* vcpu_mem will add memory access information to last_exec */ >> - g_string_printf(s, "%u, ", cpu_index); >> - g_string_append(s, (char *)udata); >> + g_string_printf(cpu->last_exec, "%u, ", cpu_index); >> + g_string_append(cpu->last_exec, (char *)udata); >> } >> >> /** >> @@ -111,6 +178,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) >> { >> struct qemu_plugin_insn *insn; >> bool skip = (imatches || amatches); >> + bool check_regs_this = rmatches; >> + bool check_regs_next = false; >> >> size_t n = qemu_plugin_tb_n_insns(tb); >> for (size_t i = 0; i < n; i++) { >> @@ -131,7 +200,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) >> /* >> * If we are filtering we better check out if we have any >> * hits. The skip "latches" so we can track memory accesses >> - * after the instruction we care about. >> + * after the instruction we care about. Also enable register >> + * checking on the next instruction. >> */ >> if (skip && imatches) { >> int j; >> @@ -139,6 +209,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) >> char *m = g_ptr_array_index(imatches, j); >> if (g_str_has_prefix(insn_disas, m)) { >> skip = false; >> + check_regs_next = rmatches; >> } >> } >> } >> @@ -153,8 +224,39 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) >> } >> } >> >> + /* >> + * Check the disassembly to see if a register we care about >> + * will be affected by this instruction. This relies on the >> + * dissembler doing something sensible for the registers we >> + * care about. >> + */ >> + if (disas_assist && rmatches) { >> + check_regs_next = false; >> + gchar *args = g_strstr_len(insn_disas, -1, " "); >> + for (int n = 0; n < all_reg_names->len; n++) { >> + gchar *reg = g_ptr_array_index(all_reg_names, n); >> + if (g_strrstr(args, reg)) { >> + check_regs_next = true; >> + skip = false; >> + } >> + } >> + } >> + >> + /* >> + * We now have 3 choices: >> + * >> + * - Log insn >> + * - Log insn while checking registers >> + * - Don't log this insn but check if last insn changed registers >> + */ >> + >> if (skip) { >> - g_free(insn_disas); >> + if (check_regs_this) { >> + qemu_plugin_register_vcpu_insn_exec_cb(insn, >> + vcpu_insn_exec_only_regs, >> + QEMU_PLUGIN_CB_R_REGS, >> + NULL); >> + } >> } else { >> uint32_t insn_opcode; >> insn_opcode = *((uint32_t *)qemu_plugin_insn_data(insn)); >> @@ -167,30 +269,124 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) >> QEMU_PLUGIN_MEM_RW, NULL); >> >> /* Register callback on instruction */ >> - qemu_plugin_register_vcpu_insn_exec_cb(insn, vcpu_insn_exec, >> - QEMU_PLUGIN_CB_NO_REGS, output); >> + if (check_regs_this) { >> + qemu_plugin_register_vcpu_insn_exec_cb( >> + insn, vcpu_insn_exec_with_regs, >> + QEMU_PLUGIN_CB_R_REGS, >> + output); >> + } else { >> + qemu_plugin_register_vcpu_insn_exec_cb( >> + insn, vcpu_insn_exec, >> + QEMU_PLUGIN_CB_NO_REGS, >> + output); >> + } >> >> /* reset skip */ >> skip = (imatches || amatches); >> } >> >> + /* set regs for next */ >> + if (disas_assist && rmatches) { >> + check_regs_this = check_regs_next; >> + } >> + >> + g_free(insn_disas); >> } >> } >> >> +static Register *init_vcpu_register(qemu_plugin_reg_descriptor *desc) >> +{ >> + Register *reg = g_new0(Register, 1); >> + g_autofree gchar *lower = g_utf8_strdown(desc->name, -1); >> + int r; >> + >> + reg->handle = desc->handle; >> + reg->name = g_intern_string(lower); >> + reg->last = g_byte_array_new(); >> + reg->new = g_byte_array_new(); >> + >> + /* read the initial value */ >> + r = qemu_plugin_read_register(reg->handle, reg->last); >> + g_assert(r > 0); >> + return reg; >> +} >> + >> +static GPtrArray *registers_init(int vcpu_index) >> +{ >> + g_autoptr(GPtrArray) registers = g_ptr_array_new(); >> + g_autoptr(GArray) reg_list = qemu_plugin_get_registers(); >> + >> + if (rmatches && reg_list->len) { >> + /* >> + * Go through each register in the complete list and >> + * see if we want to track it. >> + */ >> + for (int r = 0; r < reg_list->len; r++) { >> + qemu_plugin_reg_descriptor *rd = &g_array_index( >> + reg_list, qemu_plugin_reg_descriptor, r); >> + for (int p = 0; p < rmatches->len; p++) { >> + g_autoptr(GPatternSpec) pat = g_pattern_spec_new(rmatches->pdata[p]); >> + g_autofree gchar *rd_lower = g_utf8_strdown(rd->name, -1); >> + if (g_pattern_match_string(pat, rd->name) || >> + g_pattern_match_string(pat, rd_lower)) { >> + Register *reg = init_vcpu_register(rd); >> + g_ptr_array_add(registers, reg); >> + >> + /* we need a list of regnames at TB translation time */ >> + if (disas_assist) { >> + g_mutex_lock(&add_reg_name_lock); >> + if (!g_ptr_array_find(all_reg_names, reg->name, NULL)) { >> + g_ptr_array_add(all_reg_names, reg->name); >> + } >> + g_mutex_unlock(&add_reg_name_lock); >> + } >> + } >> + } >> + } >> + } >> + >> + return registers->len ? g_steal_pointer(®isters) : NULL; >> +} >> + >> +/* >> + * Initialise a new vcpu/thread with: >> + * - 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); >> + c->last_exec = g_string_new(NULL); >> + c->registers = registers_init(vcpu_index); >> +} >> + >> /** >> * On plugin exit, print last instruction in cache >> */ >> static void plugin_exit(qemu_plugin_id_t id, void *p) >> { >> guint i; >> - GString *s; >> - for (i = 0; i < last_exec->len; i++) { >> - s = g_ptr_array_index(last_exec, i); >> - if (s->str) { >> - qemu_plugin_outs(s->str); >> + 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) { >> + qemu_plugin_outs(c->last_exec->str); >> qemu_plugin_outs("\n"); >> } >> } >> + g_rw_lock_reader_unlock(&expand_array_lock); >> } >> >> /* Add a match to the array of matches */ >> @@ -212,6 +408,18 @@ static void parse_vaddr_match(char *match) >> g_array_append_val(amatches, v); >> } >> >> +/* >> + * We have to wait until vCPUs are started before we can check the >> + * patterns find anything. >> + */ >> +static void add_regpat(char *regpat) >> +{ >> + if (!rmatches) { >> + rmatches = g_ptr_array_new(); >> + } >> + g_ptr_array_add(rmatches, g_strdup(regpat)); >> +} >> + >> /** >> * Install the plugin >> */ >> @@ -223,11 +431,8 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, >> * Initialize dynamic array to cache vCPU instruction. In user mode >> * we don't know the size before emulation. >> */ >> - if (info->system_emulation) { >> - last_exec = g_ptr_array_sized_new(info->system.max_vcpus); >> - } else { >> - last_exec = g_ptr_array_new(); >> - } >> + cpus = g_array_sized_new(true, true, sizeof(CPU), >> + info->system_emulation ? info->system.max_vcpus : 1); >> >> for (int i = 0; i < argc; i++) { >> char *opt = argv[i]; >> @@ -236,13 +441,22 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, >> parse_insn_match(tokens[1]); >> } else if (g_strcmp0(tokens[0], "afilter") == 0) { >> parse_vaddr_match(tokens[1]); >> + } else if (g_strcmp0(tokens[0], "reg") == 0) { >> + add_regpat(tokens[1]); >> + } else if (g_strcmp0(tokens[0], "rdisas") == 0) { >> + if (!qemu_plugin_bool_parse(tokens[0], tokens[1], &disas_assist)) { >> + fprintf(stderr, "boolean argument parsing failed: %s\n", opt); >> + return -1; >> + } >> + all_reg_names = g_ptr_array_new(); >> } else { >> fprintf(stderr, "option parsing failed: %s\n", opt); >> return -1; >> } >> } >> >> - /* Register translation block and exit callbacks */ >> + /* Register init, translation block and exit callbacks */ >> + qemu_plugin_register_vcpu_init_cb(id, vcpu_init); >> qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans); >> qemu_plugin_register_atexit_cb(id, plugin_exit, NULL); >> >> -- >> 2.39.2 >> >>
On Fri, Mar 01, 2024 at 10:22:08AM +0000, Alex Bennée wrote: > Date: Fri, 01 Mar 2024 10:22:08 +0000 > From: Alex Bennée <alex.bennee@linaro.org> > Subject: Re: [PULL 26/29] contrib/plugins: extend execlog to track register > changes > > Zhao Liu <zhao1.liu@intel.com> writes: > > > Hi Alex, > > > > I hit the following warnings (with "./configure --enable-werror"): > > > > /qemu/contrib/plugins/execlog.c: In function ‘registers_init’: > > /qemu/contrib/plugins/execlog.c:330:17: warning: ‘g_pattern_match_string’ is deprecated: Use 'g_pattern_spec_match_string' instead [-Wdeprecated-declarations] > > 330 | if (g_pattern_match_string(pat, rd->name) || > > | ^~ > > In file included from /usr/include/glib-2.0/glib.h:65, > > from /qemu/contrib/plugins/execlog.c:9: > > /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here > > 55 | gboolean g_pattern_match_string (GPatternSpec *pspec, > > | ^~~~~~~~~~~~~~~~~~~~~~ > > /qemu/contrib/plugins/execlog.c:331:21: warning: ‘g_pattern_match_string’ is deprecated: Use 'g_pattern_spec_match_string' instead [-Wdeprecated-declarations] > > 331 | g_pattern_match_string(pat, rd_lower)) { > > | ^~~~~~~~~~~~~~~~~~~~~~ > > In file included from /usr/include/glib-2.0/glib.h:65, > > from /qemu/contrib/plugins/execlog.c:9: > > /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here > > 55 | gboolean g_pattern_match_string (GPatternSpec *pspec, > > | ^~~~~~~~~~~~~~~~~~~~~~ > > /qemu/contrib/plugins/execlog.c:339:63: warning: passing argument 2 of ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] > > 339 | g_ptr_array_add(all_reg_names, > > reg->name); > > Hmm I missed that. Not sure what the neatest solution is in this case - > g_ptr_array_new() doesn't have a destroy func so we shouldn't ever > attempt to free it. We can manually re-add the const qualifier at the > other end for completeness and I guess comment and cast? I find other palces use 2 ways: * Use g_strdup() to create a copy (e.g., net/net.c, add_nic_model_help()). But I'm not sure if this is OK since you said we shouldn't attempt to free it. May I ask if the free issue you mentioned will affect the use of g_strdup() here? * Another way is the forced conversion to gpointer (also e.g., in net/net.c, qemu_get_nic_models()). Which way do you like? ;-) > > > > | ~~~^~~~~~ > > In file included from /usr/include/glib-2.0/glib.h:31, > > from /qemu/contrib/plugins/execlog.c:9: > > /usr/include/glib-2.0/glib/garray.h:192:62: note: expected ‘gpointer’ {aka ‘void *’} but argument is of type ‘const char *’ > > 192 | gpointer data); > > | ~~~~~~~~~~~~~~~~~~^~~~ > > > > In addition, I checked my glic version: > > > > $ldd --version > > ldd (Ubuntu GLIBC 2.35-0ubuntu3.5) 2.35 > > > > I think it's v2.35. Are these three warning reports valid? > > It's the glib (not glibc) version that matters here. > g_pattern_match_string was deprecated in 2.70 when the suggested > alternative was added. However our baseline for glib is still: > > # When bumping glib minimum version, please check also whether to increase > # the _WIN32_WINNT setting in osdep.h according to the value from glib > glib_req_ver = '>=2.56.0' > glib_pc = dependency('glib-2.0', version: glib_req_ver, required: true, > method: 'pkg-config') > > The usual solution for this is to throw in a compat wrapper in > glib-compat.h: > > --8<---------------cut here---------------start------------->8--- > modified include/glib-compat.h > @@ -105,6 +105,24 @@ static inline gpointer g_memdup2_qemu(gconstpointer mem, gsize byte_size) > } > #define g_memdup2(m, s) g_memdup2_qemu(m, s) > > +/* > + * g_pattern_match_string has been deprecated in Glib since 2.70 and > + * will complain about it if you try to use it. Fortunately the > + * signature of both functions is the same making it easy to work > + * around. > + */ > +static inline > +gboolean g_pattern_spec_match_string_qemu(GPatternSpec *pspec, > + const gchar *string) > +{ > +#if GLIB_CHECK_VERSION(2, 70, 0) > + return g_pattern_spec_match_string(pspec, string); > +#else > + return g_pattern_match_string(pspec, string); > +#endif > +}; > +#define g_pattern_spec_match_string(p, s) g_pattern_spec_match_string_qemu(p, s) > + > #if defined(G_OS_UNIX) > /* > * Note: The fallback implementation is not MT-safe, and it returns a copy of > modified contrib/plugins/execlog.c > @@ -334,8 +334,8 @@ static void registers_init(int vcpu_index) > for (int p = 0; p < rmatches->len; p++) { > g_autoptr(GPatternSpec) pat = g_pattern_spec_new(rmatches->pdata[p]); > g_autofree gchar *rd_lower = g_utf8_strdown(rd->name, -1); > - if (g_pattern_match_string(pat, rd->name) || > - g_pattern_match_string(pat, rd_lower)) { > + if (g_pattern_spec_match_string(pat, rd->name) || > + g_pattern_spec_match_string(pat, rd_lower)) { > Register *reg = init_vcpu_register(vcpu_index, rd); > g_ptr_array_add(registers, reg); > --8<---------------cut here---------------end--------------->8--- > > but I hesitated to add it for this case as plugins shouldn't assume they > have access to QEMU's internals. Maybe the glib-compat.h header could be > treated as a special case. Thanks! This works on my side! I support to fix the compatibility as the above, after all it's always confusing when we allow users to use newer glib and see warnings at compile time! > > > > I also noticed in target/arm/helper.c, there's another > > g_pattern_match_string() but I haven't met the warning. > > Hmm that's weird. I suspect glib suppresses the warnings with: > > /* Ask for warnings for anything that was marked deprecated in > * the defined version, or before. It is a candidate for rewrite. > */ > #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_56 > I'm not too familiar with the QEMU build framework, but based on this, a natural question is, can this rule be applied to plugins code as well? If so, this would also avoid warning. Thanks, Zhao
Zhao Liu <zhao1.liu@intel.com> writes: > On Fri, Mar 01, 2024 at 10:22:08AM +0000, Alex Bennée wrote: >> Date: Fri, 01 Mar 2024 10:22:08 +0000 >> From: Alex Bennée <alex.bennee@linaro.org> >> Subject: Re: [PULL 26/29] contrib/plugins: extend execlog to track register >> changes >> >> Zhao Liu <zhao1.liu@intel.com> writes: >> >> > Hi Alex, >> > >> > I hit the following warnings (with "./configure --enable-werror"): >> > >> > /qemu/contrib/plugins/execlog.c: In function ‘registers_init’: >> > /qemu/contrib/plugins/execlog.c:330:17: warning: ‘g_pattern_match_string’ is deprecated: Use 'g_pattern_spec_match_string' instead [-Wdeprecated-declarations] >> > 330 | if (g_pattern_match_string(pat, rd->name) || >> > | ^~ >> > In file included from /usr/include/glib-2.0/glib.h:65, >> > from /qemu/contrib/plugins/execlog.c:9: >> > /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here >> > 55 | gboolean g_pattern_match_string (GPatternSpec *pspec, >> > | ^~~~~~~~~~~~~~~~~~~~~~ >> > /qemu/contrib/plugins/execlog.c:331:21: warning: ‘g_pattern_match_string’ is deprecated: Use 'g_pattern_spec_match_string' instead [-Wdeprecated-declarations] >> > 331 | g_pattern_match_string(pat, rd_lower)) { >> > | ^~~~~~~~~~~~~~~~~~~~~~ >> > In file included from /usr/include/glib-2.0/glib.h:65, >> > from /qemu/contrib/plugins/execlog.c:9: >> > /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here >> > 55 | gboolean g_pattern_match_string (GPatternSpec *pspec, >> > | ^~~~~~~~~~~~~~~~~~~~~~ >> > /qemu/contrib/plugins/execlog.c:339:63: warning: passing argument 2 of ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] >> > 339 | g_ptr_array_add(all_reg_names, >> > reg->name); >> >> Hmm I missed that. Not sure what the neatest solution is in this case - >> g_ptr_array_new() doesn't have a destroy func so we shouldn't ever >> attempt to free it. We can manually re-add the const qualifier at the >> other end for completeness and I guess comment and cast? > > I find other palces use 2 ways: > * Use g_strdup() to create a copy (e.g., net/net.c, > add_nic_model_help()). But I'm not sure if this is OK since you said > we shouldn't attempt to free it. May I ask if the free issue you > mentioned will affect the use of g_strdup() here? If we g_strdup we have to remember to free later and ensure the g_ptr_array has a free func. > * Another way is the forced conversion to gpointer (also e.g., in > net/net.c, qemu_get_nic_models()). I think this is ok, but with a comment on all_reg_names just to remind any potential users that the strings are const and allocated by QEMU so are not to be modified or freed. > > Which way do you like? ;-) > <snip> >> >> but I hesitated to add it for this case as plugins shouldn't assume they >> have access to QEMU's internals. Maybe the glib-compat.h header could be >> treated as a special case. > > Thanks! This works on my side! > > I support to fix the compatibility as the above, after all it's always > confusing when we allow users to use newer glib and see warnings at > compile time! > >> > >> > I also noticed in target/arm/helper.c, there's another >> > g_pattern_match_string() but I haven't met the warning. >> >> Hmm that's weird. I suspect glib suppresses the warnings with: >> >> /* Ask for warnings for anything that was marked deprecated in >> * the defined version, or before. It is a candidate for rewrite. >> */ >> #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_56 >> > > I'm not too familiar with the QEMU build framework, but based on this, a > natural question is, can this rule be applied to plugins code as well? > If so, this would also avoid warning. I think the simplest solution would be to add: #include "glib-compat.h" into qemu-plugin.h so plugins have the same deprecation rules as the QEMU they come from. I checked with Michael on IRC and Debian currently doesn't package any header files but if anyone starts shipping a qemu-dev package we'll need to make sure we include glib-compat.h in the same directory and qemu-plugin.h. > > Thanks, > Zhao
On Fri, Mar 01, 2024 at 04:30:51PM +0000, Alex Bennée wrote: > Date: Fri, 01 Mar 2024 16:30:51 +0000 > From: Alex Bennée <alex.bennee@linaro.org> > Subject: Re: [PULL 26/29] contrib/plugins: extend execlog to track register > changes > > Zhao Liu <zhao1.liu@intel.com> writes: > > > On Fri, Mar 01, 2024 at 10:22:08AM +0000, Alex Bennée wrote: > >> Date: Fri, 01 Mar 2024 10:22:08 +0000 > >> From: Alex Bennée <alex.bennee@linaro.org> > >> Subject: Re: [PULL 26/29] contrib/plugins: extend execlog to track register > >> changes > >> > >> Zhao Liu <zhao1.liu@intel.com> writes: > >> > >> > Hi Alex, > >> > > >> > I hit the following warnings (with "./configure --enable-werror"): > >> > > >> > /qemu/contrib/plugins/execlog.c: In function ‘registers_init’: > >> > /qemu/contrib/plugins/execlog.c:330:17: warning: ‘g_pattern_match_string’ is deprecated: Use 'g_pattern_spec_match_string' instead [-Wdeprecated-declarations] > >> > 330 | if (g_pattern_match_string(pat, rd->name) || > >> > | ^~ > >> > In file included from /usr/include/glib-2.0/glib.h:65, > >> > from /qemu/contrib/plugins/execlog.c:9: > >> > /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here > >> > 55 | gboolean g_pattern_match_string (GPatternSpec *pspec, > >> > | ^~~~~~~~~~~~~~~~~~~~~~ > >> > /qemu/contrib/plugins/execlog.c:331:21: warning: ‘g_pattern_match_string’ is deprecated: Use 'g_pattern_spec_match_string' instead [-Wdeprecated-declarations] > >> > 331 | g_pattern_match_string(pat, rd_lower)) { > >> > | ^~~~~~~~~~~~~~~~~~~~~~ > >> > In file included from /usr/include/glib-2.0/glib.h:65, > >> > from /qemu/contrib/plugins/execlog.c:9: > >> > /usr/include/glib-2.0/glib/gpattern.h:55:15: note: declared here > >> > 55 | gboolean g_pattern_match_string (GPatternSpec *pspec, > >> > | ^~~~~~~~~~~~~~~~~~~~~~ > >> > /qemu/contrib/plugins/execlog.c:339:63: warning: passing argument 2 of ‘g_ptr_array_add’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers] > >> > 339 | g_ptr_array_add(all_reg_names, > >> > reg->name); > >> > >> Hmm I missed that. Not sure what the neatest solution is in this case - > >> g_ptr_array_new() doesn't have a destroy func so we shouldn't ever > >> attempt to free it. We can manually re-add the const qualifier at the > >> other end for completeness and I guess comment and cast? > > > > I find other palces use 2 ways: > > * Use g_strdup() to create a copy (e.g., net/net.c, > > add_nic_model_help()). But I'm not sure if this is OK since you said > > we shouldn't attempt to free it. May I ask if the free issue you > > mentioned will affect the use of g_strdup() here? > > If we g_strdup we have to remember to free later and ensure the > g_ptr_array has a free func. Got it. Thanks! > > * Another way is the forced conversion to gpointer (also e.g., in > > net/net.c, qemu_get_nic_models()). > > I think this is ok, but with a comment on all_reg_names just to remind > any potential users that the strings are const and allocated by QEMU so > are not to be modified or freed. > Yes, this makes sense. > > Which way do you like? ;-) > > > <snip> > >> > >> but I hesitated to add it for this case as plugins shouldn't assume they > >> have access to QEMU's internals. Maybe the glib-compat.h header could be > >> treated as a special case. > > > > Thanks! This works on my side! > > > > I support to fix the compatibility as the above, after all it's always > > confusing when we allow users to use newer glib and see warnings at > > compile time! > > > >> > > >> > I also noticed in target/arm/helper.c, there's another > >> > g_pattern_match_string() but I haven't met the warning. > >> > >> Hmm that's weird. I suspect glib suppresses the warnings with: > >> > >> /* Ask for warnings for anything that was marked deprecated in > >> * the defined version, or before. It is a candidate for rewrite. > >> */ > >> #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_56 > >> > > > > I'm not too familiar with the QEMU build framework, but based on this, a > > natural question is, can this rule be applied to plugins code as well? > > If so, this would also avoid warning. > > I think the simplest solution would be to add: > > #include "glib-compat.h" > > into qemu-plugin.h so plugins have the same deprecation rules as the > QEMU they come from. I checked with Michael on IRC and Debian currently > doesn't package any header files but if anyone starts shipping a qemu-dev > package we'll need to make sure we include glib-compat.h in the same > directory and qemu-plugin.h. I teseted this way, and met these error and warnings: $ make -j16 changing dir to build for make ""... make[1]: Entering directory '/qemu/build' In file included from /qemu/contrib/plugins/execlog.c:16: /qemu/contrib/plugins/../../include/qemu/qemu-plugin.h:19:10: fatal error: glib-compat.h: No such file or directory 19 | #include "glib-compat.h" | ^~~~~~~~~~~~~~~ compilation terminated. make[2]: *** [Makefile:49: execlog.o] Error 1 make[1]: *** [Makefile:182: contrib/plugins/all] Error 2 make[1]: *** Waiting for unfinished jobs.... [1/2014] Compiling C object tests/plugin/libempty.so.p/empty.c.o FAILED: tests/plugin/libempty.so.p/empty.c.o cc -m64 -mcx16 -Itests/plugin/libempty.so.p -Itests/plugin -I../tests/plugin -I../include/qemu -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -fstack-protector-strong -Wempty-body -Wendif-labels -Wexpansion-to-defined -Wformat-security -Wformat-y2k -Wignored-qualifiers -Wimplicit-fallthrough=2 -Winit-self -Wmissing-format-attribute -Wmissing-prototypes -Wnested-externs -Wold-style-declaration -Wold-style-definition -Wredundant-decls -Wshadow=local -Wstrict-prototypes -Wtype-limits -Wundef -Wvla -Wwrite-strings -Wno-missing-include-dirs -Wno-psabi -Wno-shift-negative-value -isystem /qemu/linux-headers -isystem linux-headers -iquote . -iquote /qemu -iquote /qemu/include -iquote /qemu/host/include/x86_64 -iquote /qemu/host/include/generic -iquote /qemu/tcg/i386 -pthread -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -fzero-call-used-regs=used-gpr -fPIC -MD -MQ tests/plugin/libempty.so.p/empty.c.o -MF tests/plugin/libempty.so.p/empty.c.o.d -o tests/plugin/libempty.so.p/empty.c.o -c ../tests/plugin/empty.c In file included from ../include/qemu/qemu-plugin.h:19, from ../tests/plugin/empty.c:14: /qemu/include/glib-compat.h:22: error: "GLIB_VERSION_MIN_REQUIRED" redefined [-Werror] 22 | #define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_56 | In file included from /usr/include/glib-2.0/glib/gtypes.h:34, from /usr/include/glib-2.0/glib/galloca.h:32, from /usr/include/glib-2.0/glib.h:30, from ../include/qemu/qemu-plugin.h:14, from ../tests/plugin/empty.c:14: /usr/include/glib-2.0/glib/gversionmacros.h:335: note: this is the location of the previous definition 335 | # define GLIB_VERSION_MIN_REQUIRED (GLIB_VERSION_CUR_STABLE) | In file included from ../include/qemu/qemu-plugin.h:19, from ../tests/plugin/empty.c:14: /qemu/include/glib-compat.h:27: error: "GLIB_VERSION_MAX_ALLOWED" redefined [-Werror] 27 | #define GLIB_VERSION_MAX_ALLOWED GLIB_VERSION_2_56 | In file included from /usr/include/glib-2.0/glib/gtypes.h:34, from /usr/include/glib-2.0/glib/galloca.h:32, from /usr/include/glib-2.0/glib.h:30, from ../include/qemu/qemu-plugin.h:14, from ../tests/plugin/empty.c:14: /usr/include/glib-2.0/glib/gversionmacros.h:364: note: this is the location of the previous definition 364 | # define GLIB_VERSION_MAX_ALLOWED (GLIB_VERSION_CUR_STABLE) | cc1: all warnings being treated as errors For redefination warings, I think it's because qemu-plugin.h has included <glib.h>...so it seems this way is still not clean. As a comparison, it may be that we also have to go with g_pattern_spec_match_string_qemu(), which also avoids the potential problems you mentioned with the possible qemu-dev package. Will you send the cleanups for those warnings? If you're pressed for time, I'd be happy to help you send them. ;-) Thanks, Zhao
On Wed, 28 Feb 2024 at 12:00, Alex Bennée <alex.bennee@linaro.org> wrote: > > With the new plugin register API we can now track changes to register > values. Currently the implementation is fairly dumb which will slow > down if a large number of register values are being tracked. This > could be improved by only instrumenting instructions which mention > registers we are interested in tracking. > Hi; Coverity complains about a possible NULL dereference in this code (CID 1534929): > @@ -153,8 +224,39 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) > } > } > > + /* > + * Check the disassembly to see if a register we care about > + * will be affected by this instruction. This relies on the > + * dissembler doing something sensible for the registers we > + * care about. > + */ > + if (disas_assist && rmatches) { > + check_regs_next = false; > + gchar *args = g_strstr_len(insn_disas, -1, " "); g_strstr_len() can return NULL if it doesn't find the string being searched for... > + for (int n = 0; n < all_reg_names->len; n++) { > + gchar *reg = g_ptr_array_index(all_reg_names, n); > + if (g_strrstr(args, reg)) { ...but it's not valid to pass NULL as the argument to g_strrstr(). > + check_regs_next = true; > + skip = false; > + } > + } > + } thanks -- PMM
diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst index 81dcd43a612..fa7421279f5 100644 --- a/docs/devel/tcg-plugins.rst +++ b/docs/devel/tcg-plugins.rst @@ -497,6 +497,22 @@ arguments if required:: $ qemu-system-arm $(QEMU_ARGS) \ -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin +This plugin can also dump registers when they change value. Specify the name of the +registers with multiple ``reg`` options. You can also use glob style matching if you wish:: + + $ qemu-system-arm $(QEMU_ARGS) \ + -plugin ./contrib/plugins/libexeclog.so,reg=\*_el2,reg=sp -d plugin + +Be aware that each additional register to check will slow down +execution quite considerably. You can optimise the number of register +checks done by using the rdisas option. This will only instrument +instructions that mention the registers in question in disassembly. +This is not foolproof as some instructions implicitly change +instructions. You can use the ifilter to catch these cases: + + $ qemu-system-arm $(QEMU_ARGS) \ + -plugin ./contrib/plugins/libexeclog.so,ifilter=msr,ifilter=blr,reg=x30,reg=\*_el1,rdisas=on + - contrib/plugins/cache.c Cache modelling plugin that measures the performance of a given L1 cache @@ -583,4 +599,3 @@ The following API is generated from the inline documentation in include the full kernel-doc annotations. .. kernel-doc:: include/qemu/qemu-plugin.h - diff --git a/contrib/plugins/execlog.c b/contrib/plugins/execlog.c index f262e5555eb..a1dfd59ab71 100644 --- a/contrib/plugins/execlog.c +++ b/contrib/plugins/execlog.c @@ -1,7 +1,7 @@ /* * Copyright (C) 2021, Alexandre Iooss <erdnaxe@crans.org> * - * Log instruction execution with memory access. + * Log instruction execution with memory access and register changes * * License: GNU GPL, version 2 or later. * See the COPYING file in the top-level directory. @@ -15,29 +15,40 @@ #include <qemu-plugin.h> +typedef struct { + struct qemu_plugin_register *handle; + GByteArray *last; + GByteArray *new; + const char *name; +} Register; + +typedef struct CPU { + /* Store last executed instruction on each vCPU as a GString */ + GString *last_exec; + /* Ptr array of Register */ + GPtrArray *registers; +} CPU; + QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; -/* Store last executed instruction on each vCPU as a GString */ -static GPtrArray *last_exec; +static GArray *cpus; static GRWLock expand_array_lock; static GPtrArray *imatches; static GArray *amatches; +static GPtrArray *rmatches; +static bool disas_assist; +static GMutex add_reg_name_lock; +static GPtrArray *all_reg_names; -/* - * Expand last_exec array. - * - * As we could have multiple threads trying to do this we need to - * serialise the expansion under a lock. - */ -static void expand_last_exec(int cpu_index) +static CPU *get_cpu(int vcpu_index) { - g_rw_lock_writer_lock(&expand_array_lock); - while (cpu_index >= last_exec->len) { - GString *s = g_string_new(NULL); - g_ptr_array_add(last_exec, s); - } - g_rw_lock_writer_unlock(&expand_array_lock); + 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; } /** @@ -46,13 +57,10 @@ static void expand_last_exec(int cpu_index) static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t info, uint64_t vaddr, void *udata) { - GString *s; + CPU *c = get_cpu(cpu_index); + GString *s = c->last_exec; /* Find vCPU in array */ - g_rw_lock_reader_lock(&expand_array_lock); - g_assert(cpu_index < last_exec->len); - s = g_ptr_array_index(last_exec, cpu_index); - g_rw_lock_reader_unlock(&expand_array_lock); /* Indicate type of memory access */ if (qemu_plugin_mem_is_store(info)) { @@ -73,32 +81,91 @@ static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t info, } /** - * Log instruction execution + * Log instruction execution, outputting the last one. + * + * vcpu_insn_exec() is a copy and paste of vcpu_insn_exec_with_regs() + * without the checking of register values when we've attempted to + * optimise with disas_assist. */ -static void vcpu_insn_exec(unsigned int cpu_index, void *udata) +static void insn_check_regs(CPU *cpu) { - GString *s; + for (int n = 0; n < cpu->registers->len; n++) { + Register *reg = cpu->registers->pdata[n]; + int sz; - /* Find or create vCPU in array */ - g_rw_lock_reader_lock(&expand_array_lock); - if (cpu_index >= last_exec->len) { - g_rw_lock_reader_unlock(&expand_array_lock); - expand_last_exec(cpu_index); - g_rw_lock_reader_lock(&expand_array_lock); + g_byte_array_set_size(reg->new, 0); + sz = qemu_plugin_read_register(reg->handle, reg->new); + g_assert(sz == reg->last->len); + + if (memcmp(reg->last->data, reg->new->data, sz)) { + GByteArray *temp = reg->last; + g_string_append_printf(cpu->last_exec, ", %s -> 0x", reg->name); + /* TODO: handle BE properly */ + for (int i = sz; i >= 0; i--) { + g_string_append_printf(cpu->last_exec, "%02x", + reg->new->data[i]); + } + reg->last = reg->new; + reg->new = temp; + } + } +} + +/* 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); + + /* Print previous instruction in cache */ + if (cpu->last_exec->len) { + if (cpu->registers) { + insn_check_regs(cpu); + } + + qemu_plugin_outs(cpu->last_exec->str); + qemu_plugin_outs("\n"); + } + + /* Store new instruction in cache */ + /* vcpu_mem will add memory access information to last_exec */ + g_string_printf(cpu->last_exec, "%u, ", cpu_index); + g_string_append(cpu->last_exec, (char *)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); + + /* Print previous instruction in cache */ + if (cpu->last_exec->len) { + if (cpu->registers) { + insn_check_regs(cpu); + } + + qemu_plugin_outs(cpu->last_exec->str); + qemu_plugin_outs("\n"); } - s = g_ptr_array_index(last_exec, cpu_index); - g_rw_lock_reader_unlock(&expand_array_lock); + + /* reset */ + cpu->last_exec->len = 0; +} + +/* 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); /* Print previous instruction in cache */ - if (s->len) { - qemu_plugin_outs(s->str); + if (cpu->last_exec->len) { + qemu_plugin_outs(cpu->last_exec->str); qemu_plugin_outs("\n"); } /* Store new instruction in cache */ /* vcpu_mem will add memory access information to last_exec */ - g_string_printf(s, "%u, ", cpu_index); - g_string_append(s, (char *)udata); + g_string_printf(cpu->last_exec, "%u, ", cpu_index); + g_string_append(cpu->last_exec, (char *)udata); } /** @@ -111,6 +178,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) { struct qemu_plugin_insn *insn; bool skip = (imatches || amatches); + bool check_regs_this = rmatches; + bool check_regs_next = false; size_t n = qemu_plugin_tb_n_insns(tb); for (size_t i = 0; i < n; i++) { @@ -131,7 +200,8 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) /* * If we are filtering we better check out if we have any * hits. The skip "latches" so we can track memory accesses - * after the instruction we care about. + * after the instruction we care about. Also enable register + * checking on the next instruction. */ if (skip && imatches) { int j; @@ -139,6 +209,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) char *m = g_ptr_array_index(imatches, j); if (g_str_has_prefix(insn_disas, m)) { skip = false; + check_regs_next = rmatches; } } } @@ -153,8 +224,39 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) } } + /* + * Check the disassembly to see if a register we care about + * will be affected by this instruction. This relies on the + * dissembler doing something sensible for the registers we + * care about. + */ + if (disas_assist && rmatches) { + check_regs_next = false; + gchar *args = g_strstr_len(insn_disas, -1, " "); + for (int n = 0; n < all_reg_names->len; n++) { + gchar *reg = g_ptr_array_index(all_reg_names, n); + if (g_strrstr(args, reg)) { + check_regs_next = true; + skip = false; + } + } + } + + /* + * We now have 3 choices: + * + * - Log insn + * - Log insn while checking registers + * - Don't log this insn but check if last insn changed registers + */ + if (skip) { - g_free(insn_disas); + if (check_regs_this) { + qemu_plugin_register_vcpu_insn_exec_cb(insn, + vcpu_insn_exec_only_regs, + QEMU_PLUGIN_CB_R_REGS, + NULL); + } } else { uint32_t insn_opcode; insn_opcode = *((uint32_t *)qemu_plugin_insn_data(insn)); @@ -167,30 +269,124 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) QEMU_PLUGIN_MEM_RW, NULL); /* Register callback on instruction */ - qemu_plugin_register_vcpu_insn_exec_cb(insn, vcpu_insn_exec, - QEMU_PLUGIN_CB_NO_REGS, output); + if (check_regs_this) { + qemu_plugin_register_vcpu_insn_exec_cb( + insn, vcpu_insn_exec_with_regs, + QEMU_PLUGIN_CB_R_REGS, + output); + } else { + qemu_plugin_register_vcpu_insn_exec_cb( + insn, vcpu_insn_exec, + QEMU_PLUGIN_CB_NO_REGS, + output); + } /* reset skip */ skip = (imatches || amatches); } + /* set regs for next */ + if (disas_assist && rmatches) { + check_regs_this = check_regs_next; + } + + g_free(insn_disas); } } +static Register *init_vcpu_register(qemu_plugin_reg_descriptor *desc) +{ + Register *reg = g_new0(Register, 1); + g_autofree gchar *lower = g_utf8_strdown(desc->name, -1); + int r; + + reg->handle = desc->handle; + reg->name = g_intern_string(lower); + reg->last = g_byte_array_new(); + reg->new = g_byte_array_new(); + + /* read the initial value */ + r = qemu_plugin_read_register(reg->handle, reg->last); + g_assert(r > 0); + return reg; +} + +static GPtrArray *registers_init(int vcpu_index) +{ + g_autoptr(GPtrArray) registers = g_ptr_array_new(); + g_autoptr(GArray) reg_list = qemu_plugin_get_registers(); + + if (rmatches && reg_list->len) { + /* + * Go through each register in the complete list and + * see if we want to track it. + */ + for (int r = 0; r < reg_list->len; r++) { + qemu_plugin_reg_descriptor *rd = &g_array_index( + reg_list, qemu_plugin_reg_descriptor, r); + for (int p = 0; p < rmatches->len; p++) { + g_autoptr(GPatternSpec) pat = g_pattern_spec_new(rmatches->pdata[p]); + g_autofree gchar *rd_lower = g_utf8_strdown(rd->name, -1); + if (g_pattern_match_string(pat, rd->name) || + g_pattern_match_string(pat, rd_lower)) { + Register *reg = init_vcpu_register(rd); + g_ptr_array_add(registers, reg); + + /* we need a list of regnames at TB translation time */ + if (disas_assist) { + g_mutex_lock(&add_reg_name_lock); + if (!g_ptr_array_find(all_reg_names, reg->name, NULL)) { + g_ptr_array_add(all_reg_names, reg->name); + } + g_mutex_unlock(&add_reg_name_lock); + } + } + } + } + } + + return registers->len ? g_steal_pointer(®isters) : NULL; +} + +/* + * Initialise a new vcpu/thread with: + * - 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); + c->last_exec = g_string_new(NULL); + c->registers = registers_init(vcpu_index); +} + /** * On plugin exit, print last instruction in cache */ static void plugin_exit(qemu_plugin_id_t id, void *p) { guint i; - GString *s; - for (i = 0; i < last_exec->len; i++) { - s = g_ptr_array_index(last_exec, i); - if (s->str) { - qemu_plugin_outs(s->str); + 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) { + qemu_plugin_outs(c->last_exec->str); qemu_plugin_outs("\n"); } } + g_rw_lock_reader_unlock(&expand_array_lock); } /* Add a match to the array of matches */ @@ -212,6 +408,18 @@ static void parse_vaddr_match(char *match) g_array_append_val(amatches, v); } +/* + * We have to wait until vCPUs are started before we can check the + * patterns find anything. + */ +static void add_regpat(char *regpat) +{ + if (!rmatches) { + rmatches = g_ptr_array_new(); + } + g_ptr_array_add(rmatches, g_strdup(regpat)); +} + /** * Install the plugin */ @@ -223,11 +431,8 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, * Initialize dynamic array to cache vCPU instruction. In user mode * we don't know the size before emulation. */ - if (info->system_emulation) { - last_exec = g_ptr_array_sized_new(info->system.max_vcpus); - } else { - last_exec = g_ptr_array_new(); - } + cpus = g_array_sized_new(true, true, sizeof(CPU), + info->system_emulation ? info->system.max_vcpus : 1); for (int i = 0; i < argc; i++) { char *opt = argv[i]; @@ -236,13 +441,22 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, parse_insn_match(tokens[1]); } else if (g_strcmp0(tokens[0], "afilter") == 0) { parse_vaddr_match(tokens[1]); + } else if (g_strcmp0(tokens[0], "reg") == 0) { + add_regpat(tokens[1]); + } else if (g_strcmp0(tokens[0], "rdisas") == 0) { + if (!qemu_plugin_bool_parse(tokens[0], tokens[1], &disas_assist)) { + fprintf(stderr, "boolean argument parsing failed: %s\n", opt); + return -1; + } + all_reg_names = g_ptr_array_new(); } else { fprintf(stderr, "option parsing failed: %s\n", opt); return -1; } } - /* Register translation block and exit callbacks */ + /* Register init, translation block and exit callbacks */ + qemu_plugin_register_vcpu_init_cb(id, vcpu_init); qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans); qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);