| Message ID | 20230818033648.8326-25-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | plugins: Allow to read registers | expand |
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > This demonstrates how a register can be read from a plugin. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > docs/devel/tcg-plugins.rst | 10 ++- > contrib/plugins/execlog.c | 140 ++++++++++++++++++++++++++++--------- > 2 files changed, 117 insertions(+), 33 deletions(-) > > diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst > index 81dcd43a61..c9f8b27590 100644 > --- a/docs/devel/tcg-plugins.rst > +++ b/docs/devel/tcg-plugins.rst > @@ -497,6 +497,15 @@ arguments if required:: > $ qemu-system-arm $(QEMU_ARGS) \ > -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin > > +This plugin can also dump a specified register. The specification of register > +follows `GDB standard target features <https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html>`__. > + > +Specify the name of the feature that contains the register and the name of the > +register with ``rfile`` and ``reg`` options, respectively:: > + > + $ qemu-system-arm $(QEMU_ARGS) \ > + -plugin ./contrib/plugins/libexeclog.so,rfile=org.gnu.gdb.arm.core,reg=sp -d plugin > + > - contrib/plugins/cache.c > > Cache modelling plugin that measures the performance of a given L1 cache > @@ -583,4 +592,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 82dc2f584e..aa05840fd0 100644 > --- a/contrib/plugins/execlog.c > +++ b/contrib/plugins/execlog.c > @@ -15,27 +15,43 @@ > > #include <qemu-plugin.h> > > +typedef struct CPU { > + /* Store last executed instruction on each vCPU as a GString */ > + GString *last_exec; > + GByteArray *reg_history[2]; > + > + int reg; > +} CPU; > + > QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; > <snip> > > /* 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(cpus[cpu_index].last_exec, "%u, ", cpu_index); > + g_string_append(cpus[cpu_index].last_exec, (char *)udata); > + > + g_rw_lock_reader_unlock(&expand_array_lock); > } > > /** > @@ -167,8 +197,10 @@ 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); > + qemu_plugin_register_vcpu_insn_exec_cb( > + insn, vcpu_insn_exec, > + rfile_name ? QEMU_PLUGIN_CB_R_REGS : QEMU_PLUGIN_CB_NO_REGS, > + output); > > /* reset skip */ > skip = (imatches || amatches); > @@ -177,17 +209,53 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) > } > } > > +static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index) > +{ > + int reg = 0; > + bool found = false; > + > + expand_cpu(vcpu_index); > + > + if (rfile_name) { > + int i; > + int j; > + int n; > + > + qemu_plugin_register_file_t *rfiles = > + qemu_plugin_get_register_files(vcpu_index, &n); > + > + for (i = 0; i < n; i++) { > + if (g_strcmp0(rfiles[i].name, rfile_name) == 0) { > + for (j = 0; j < rfiles[i].num_regs; j++) { > + if (g_strcmp0(rfiles[i].regs[j], reg_name) == 0) { > + reg += j; > + found = true; > + break; > + } > + } > + break; > + } > + > + reg += rfiles[i].num_regs; > + } > + > + g_free(rfiles); > + } This makes me question the value of exposing the register file directly to the plugin. I would much rather have a lookup utility function with an optional tag. Something like: plugin_reg_t qemu_plugin_find_register(const char *name, const char *tag); And make tag optional. I think in the general case "name" should be enough. > + > + g_rw_lock_writer_lock(&expand_array_lock); > + cpus[vcpu_index].reg = found ? reg : -1; > + g_rw_lock_writer_unlock(&expand_array_lock); > +} > + > /** > * 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); > + for (i = 0; i < num_cpus; i++) { > + if (cpus[i].last_exec->str) { > + qemu_plugin_outs(cpus[i].last_exec->str); > qemu_plugin_outs("\n"); > } > } > @@ -224,9 +292,7 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, > * 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_new(CPU, info->system.max_vcpus); > } > > for (int i = 0; i < argc; i++) { > @@ -236,13 +302,23 @@ 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], "rfile") == 0) { > + rfile_name = g_strdup(tokens[1]); > + } else if (g_strcmp0(tokens[0], "reg") == 0) { > + reg_name = g_strdup(tokens[1]); And then instead of having the rfile/reg distinction support a command line like: qemu-aarch64 -plugin contrib/plugins/libexeclog.so,reg=sp,reg=x1,reg=sve:p1 so if the user specifies a reg of the form TAG:REG we can pass that as the option tag string. It also avoids exposing all the details of gdb to plugins while still allowing the utility function to search by rname internally (even if only a substring match?), > } else { > fprintf(stderr, "option parsing failed: %s\n", opt); > return -1; > } > } > > + if ((!rfile_name) != (!reg_name)) { > + fputs("file and reg need to be set at the same time\n", stderr); > + return -1; > + } > + > /* Register 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);
On 2023/08/31 0:08, Alex Bennée wrote: > > Akihiko Odaki <akihiko.odaki@daynix.com> writes: > >> This demonstrates how a register can be read from a plugin. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> docs/devel/tcg-plugins.rst | 10 ++- >> contrib/plugins/execlog.c | 140 ++++++++++++++++++++++++++++--------- >> 2 files changed, 117 insertions(+), 33 deletions(-) >> >> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst >> index 81dcd43a61..c9f8b27590 100644 >> --- a/docs/devel/tcg-plugins.rst >> +++ b/docs/devel/tcg-plugins.rst >> @@ -497,6 +497,15 @@ arguments if required:: >> $ qemu-system-arm $(QEMU_ARGS) \ >> -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin >> >> +This plugin can also dump a specified register. The specification of register >> +follows `GDB standard target features <https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html>`__. >> + >> +Specify the name of the feature that contains the register and the name of the >> +register with ``rfile`` and ``reg`` options, respectively:: >> + >> + $ qemu-system-arm $(QEMU_ARGS) \ >> + -plugin ./contrib/plugins/libexeclog.so,rfile=org.gnu.gdb.arm.core,reg=sp -d plugin >> + >> - contrib/plugins/cache.c >> >> Cache modelling plugin that measures the performance of a given L1 cache >> @@ -583,4 +592,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 82dc2f584e..aa05840fd0 100644 >> --- a/contrib/plugins/execlog.c >> +++ b/contrib/plugins/execlog.c >> @@ -15,27 +15,43 @@ >> >> #include <qemu-plugin.h> >> >> +typedef struct CPU { >> + /* Store last executed instruction on each vCPU as a GString */ >> + GString *last_exec; >> + GByteArray *reg_history[2]; >> + >> + int reg; >> +} CPU; >> + >> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; >> > <snip> >> >> /* 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(cpus[cpu_index].last_exec, "%u, ", cpu_index); >> + g_string_append(cpus[cpu_index].last_exec, (char *)udata); >> + >> + g_rw_lock_reader_unlock(&expand_array_lock); >> } >> >> /** >> @@ -167,8 +197,10 @@ 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); >> + qemu_plugin_register_vcpu_insn_exec_cb( >> + insn, vcpu_insn_exec, >> + rfile_name ? QEMU_PLUGIN_CB_R_REGS : QEMU_PLUGIN_CB_NO_REGS, >> + output); >> >> /* reset skip */ >> skip = (imatches || amatches); >> @@ -177,17 +209,53 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) >> } >> } >> >> +static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index) >> +{ >> + int reg = 0; >> + bool found = false; >> + >> + expand_cpu(vcpu_index); >> + >> + if (rfile_name) { >> + int i; >> + int j; >> + int n; >> + >> + qemu_plugin_register_file_t *rfiles = >> + qemu_plugin_get_register_files(vcpu_index, &n); >> + >> + for (i = 0; i < n; i++) { >> + if (g_strcmp0(rfiles[i].name, rfile_name) == 0) { >> + for (j = 0; j < rfiles[i].num_regs; j++) { >> + if (g_strcmp0(rfiles[i].regs[j], reg_name) == 0) { >> + reg += j; >> + found = true; >> + break; >> + } >> + } >> + break; >> + } >> + >> + reg += rfiles[i].num_regs; >> + } >> + >> + g_free(rfiles); >> + } > > This makes me question the value of exposing the register file directly > to the plugin. I would much rather have a lookup utility function with > an optional tag. Something like: > > plugin_reg_t qemu_plugin_find_register(const char *name, const char *tag); > > And make tag optional. I think in the general case "name" should be enough. I have explained the reason why I introduced register file abstraction instead of adding a function to look up a register for an earlier version of this series: > I added a function that returns all register information instead of a > function that looks up a register so that a plugin can enumerate > registers. Such capability is useful for a plugin that dumps all > registers or a plugin that simulates processor (such a plugin may want > to warn if there are unknown registers). How would you define name and tag? They are something we currently do not have, and I'm trying to add new types of identifiers since such identifiers will be needed to be defined for different architectures and require documentation and extra work to avoid name conflicts and ensure interface stability. > >> + >> + g_rw_lock_writer_lock(&expand_array_lock); >> + cpus[vcpu_index].reg = found ? reg : -1; >> + g_rw_lock_writer_unlock(&expand_array_lock); >> +} >> + >> /** >> * 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); >> + for (i = 0; i < num_cpus; i++) { >> + if (cpus[i].last_exec->str) { >> + qemu_plugin_outs(cpus[i].last_exec->str); >> qemu_plugin_outs("\n"); >> } >> } >> @@ -224,9 +292,7 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, >> * 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_new(CPU, info->system.max_vcpus); >> } >> >> for (int i = 0; i < argc; i++) { >> @@ -236,13 +302,23 @@ 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], "rfile") == 0) { >> + rfile_name = g_strdup(tokens[1]); >> + } else if (g_strcmp0(tokens[0], "reg") == 0) { >> + reg_name = g_strdup(tokens[1]); > > And then instead of having the rfile/reg distinction support a command > line like: > > qemu-aarch64 -plugin contrib/plugins/libexeclog.so,reg=sp,reg=x1,reg=sve:p1 > > so if the user specifies a reg of the form TAG:REG we can pass that as > the option tag string. It also avoids exposing all the details of gdb to > plugins while still allowing the utility function to search by rname > internally (even if only a substring match?), That implicitly assumes TAG does not contain a colon. I'm avoiding to make such an implicit assumption because it is a reference for plugin writers who may create out-of-tree plugins. We should retrain ourselves to tell the plugin writers not to make such an assumption that may not hold in the future version of QEMU. I consider a substring match harmful for a similar reason. There is no guarantee that a future version of QEMU will not introduce a new register that match with the existing substring and break interface stability. It is not necessary that identifiers are consistent with ones GDB use. What matters here is that the identifiers are documented, stable and immune from conflicts. > > >> } else { >> fprintf(stderr, "option parsing failed: %s\n", opt); >> return -1; >> } >> } >> >> + if ((!rfile_name) != (!reg_name)) { >> + fputs("file and reg need to be set at the same time\n", stderr); >> + return -1; >> + } >> + >> /* Register 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); > >
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > On 2023/08/31 0:08, Alex Bennée wrote: >> Akihiko Odaki <akihiko.odaki@daynix.com> writes: >> >>> This demonstrates how a register can be read from a plugin. >>> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> --- >>> docs/devel/tcg-plugins.rst | 10 ++- >>> contrib/plugins/execlog.c | 140 ++++++++++++++++++++++++++++--------- >>> 2 files changed, 117 insertions(+), 33 deletions(-) >>> >>> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst >>> index 81dcd43a61..c9f8b27590 100644 >>> --- a/docs/devel/tcg-plugins.rst >>> +++ b/docs/devel/tcg-plugins.rst >>> @@ -497,6 +497,15 @@ arguments if required:: >>> $ qemu-system-arm $(QEMU_ARGS) \ >>> -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin >>> +This plugin can also dump a specified register. The >>> specification of register >>> +follows `GDB standard target features <https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html>`__. >>> + >>> +Specify the name of the feature that contains the register and the name of the >>> +register with ``rfile`` and ``reg`` options, respectively:: >>> + >>> + $ qemu-system-arm $(QEMU_ARGS) \ >>> + -plugin ./contrib/plugins/libexeclog.so,rfile=org.gnu.gdb.arm.core,reg=sp -d plugin >>> + >>> - contrib/plugins/cache.c >>> Cache modelling plugin that measures the performance of a given >>> L1 cache >>> @@ -583,4 +592,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 82dc2f584e..aa05840fd0 100644 >>> --- a/contrib/plugins/execlog.c >>> +++ b/contrib/plugins/execlog.c >>> @@ -15,27 +15,43 @@ >>> #include <qemu-plugin.h> >>> +typedef struct CPU { >>> + /* Store last executed instruction on each vCPU as a GString */ >>> + GString *last_exec; >>> + GByteArray *reg_history[2]; >>> + >>> + int reg; >>> +} CPU; >>> + >>> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; >>> >> <snip> >>> /* 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(cpus[cpu_index].last_exec, "%u, ", cpu_index); >>> + g_string_append(cpus[cpu_index].last_exec, (char *)udata); >>> + >>> + g_rw_lock_reader_unlock(&expand_array_lock); >>> } >>> /** >>> @@ -167,8 +197,10 @@ 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); >>> + qemu_plugin_register_vcpu_insn_exec_cb( >>> + insn, vcpu_insn_exec, >>> + rfile_name ? QEMU_PLUGIN_CB_R_REGS : QEMU_PLUGIN_CB_NO_REGS, >>> + output); >>> /* reset skip */ >>> skip = (imatches || amatches); >>> @@ -177,17 +209,53 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) >>> } >>> } >>> +static void vcpu_init(qemu_plugin_id_t id, unsigned int >>> vcpu_index) >>> +{ >>> + int reg = 0; >>> + bool found = false; >>> + >>> + expand_cpu(vcpu_index); >>> + >>> + if (rfile_name) { >>> + int i; >>> + int j; >>> + int n; >>> + >>> + qemu_plugin_register_file_t *rfiles = >>> + qemu_plugin_get_register_files(vcpu_index, &n); >>> + >>> + for (i = 0; i < n; i++) { >>> + if (g_strcmp0(rfiles[i].name, rfile_name) == 0) { >>> + for (j = 0; j < rfiles[i].num_regs; j++) { >>> + if (g_strcmp0(rfiles[i].regs[j], reg_name) == 0) { >>> + reg += j; >>> + found = true; >>> + break; >>> + } >>> + } >>> + break; >>> + } >>> + >>> + reg += rfiles[i].num_regs; >>> + } >>> + >>> + g_free(rfiles); >>> + } >> This makes me question the value of exposing the register file >> directly >> to the plugin. I would much rather have a lookup utility function with >> an optional tag. Something like: >> plugin_reg_t qemu_plugin_find_register(const char *name, const >> char *tag); >> And make tag optional. I think in the general case "name" should be >> enough. > > I have explained the reason why I introduced register file abstraction > instead of adding a function to look up a register for an earlier > version of this series: >> I added a function that returns all register information instead of a >> function that looks up a register so that a plugin can enumerate >> registers. Such capability is useful for a plugin that dumps all >> registers or a plugin that simulates processor (such a plugin may want >> to warn if there are unknown registers). Fair enough. However I think a simple search interface will also be useful for the more common case. > How would you define name and tag? They are something we currently do > not have, and I'm trying to add new types of identifiers since such > identifiers will be needed to be defined for different architectures > and require documentation and extra work to avoid name conflicts and > ensure interface stability. The name would be the register name which AFAICT are unique across the system. If you have examples of clashes I'm curious as to what they are. I'm still conflicted about baking gdb-isms into this ABI because they aren't as stable as they could be either. Either way we do state: This is a new feature for QEMU and it does allow people to develop out-of-tree plugins that can be dynamically linked into a running QEMU process. However the project reserves the right to change or break the API should it need to do so. The best way to avoid this is to submit your plugin upstream so they can be updated if/when the API changes. So I'm not overly concerned about formalising a stable ABI for now. > >> >>> + >>> + g_rw_lock_writer_lock(&expand_array_lock); >>> + cpus[vcpu_index].reg = found ? reg : -1; >>> + g_rw_lock_writer_unlock(&expand_array_lock); >>> +} >>> + >>> /** >>> * 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); >>> + for (i = 0; i < num_cpus; i++) { >>> + if (cpus[i].last_exec->str) { >>> + qemu_plugin_outs(cpus[i].last_exec->str); >>> qemu_plugin_outs("\n"); >>> } >>> } >>> @@ -224,9 +292,7 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, >>> * 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_new(CPU, info->system.max_vcpus); >>> } >>> for (int i = 0; i < argc; i++) { >>> @@ -236,13 +302,23 @@ 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], "rfile") == 0) { >>> + rfile_name = g_strdup(tokens[1]); >>> + } else if (g_strcmp0(tokens[0], "reg") == 0) { >>> + reg_name = g_strdup(tokens[1]); >> And then instead of having the rfile/reg distinction support a >> command >> line like: >> qemu-aarch64 -plugin >> contrib/plugins/libexeclog.so,reg=sp,reg=x1,reg=sve:p1 >> so if the user specifies a reg of the form TAG:REG we can pass that >> as >> the option tag string. It also avoids exposing all the details of gdb to >> plugins while still allowing the utility function to search by rname >> internally (even if only a substring match?), > > That implicitly assumes TAG does not contain a colon. I'm avoiding to > make such an implicit assumption because it is a reference for plugin > writers who may create out-of-tree plugins. We should retrain > ourselves to tell the plugin writers not to make such an assumption > that may not hold in the future version of QEMU. > > I consider a substring match harmful for a similar reason. There is no > guarantee that a future version of QEMU will not introduce a new > register that match with the existing substring and break interface > stability. > > It is not necessary that identifiers are consistent with ones GDB use. > What matters here is that the identifiers are documented, stable and > immune from conflicts. We've a couple of cases of GDB having to issue new XML interface names to handle cases where the previous definition missed important bits. Hence my unease at exposing them to the plugin interface. The plugin interface shouldn't (yet?) be regarded as a stable interface (c.f. above). > >> >>> } else { >>> fprintf(stderr, "option parsing failed: %s\n", opt); >>> return -1; >>> } >>> } >>> + if ((!rfile_name) != (!reg_name)) { >>> + fputs("file and reg need to be set at the same time\n", stderr); >>> + return -1; >>> + } >>> + >>> /* Register 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); >>
On 2023/09/05 0:30, Alex Bennée wrote: > > Akihiko Odaki <akihiko.odaki@daynix.com> writes: > >> On 2023/08/31 0:08, Alex Bennée wrote: >>> Akihiko Odaki <akihiko.odaki@daynix.com> writes: >>> >>>> This demonstrates how a register can be read from a plugin. >>>> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>> --- >>>> docs/devel/tcg-plugins.rst | 10 ++- >>>> contrib/plugins/execlog.c | 140 ++++++++++++++++++++++++++++--------- >>>> 2 files changed, 117 insertions(+), 33 deletions(-) >>>> >>>> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst >>>> index 81dcd43a61..c9f8b27590 100644 >>>> --- a/docs/devel/tcg-plugins.rst >>>> +++ b/docs/devel/tcg-plugins.rst >>>> @@ -497,6 +497,15 @@ arguments if required:: >>>> $ qemu-system-arm $(QEMU_ARGS) \ >>>> -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin >>>> +This plugin can also dump a specified register. The >>>> specification of register >>>> +follows `GDB standard target features <https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html>`__. >>>> + >>>> +Specify the name of the feature that contains the register and the name of the >>>> +register with ``rfile`` and ``reg`` options, respectively:: >>>> + >>>> + $ qemu-system-arm $(QEMU_ARGS) \ >>>> + -plugin ./contrib/plugins/libexeclog.so,rfile=org.gnu.gdb.arm.core,reg=sp -d plugin >>>> + >>>> - contrib/plugins/cache.c >>>> Cache modelling plugin that measures the performance of a given >>>> L1 cache >>>> @@ -583,4 +592,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 82dc2f584e..aa05840fd0 100644 >>>> --- a/contrib/plugins/execlog.c >>>> +++ b/contrib/plugins/execlog.c >>>> @@ -15,27 +15,43 @@ >>>> #include <qemu-plugin.h> >>>> +typedef struct CPU { >>>> + /* Store last executed instruction on each vCPU as a GString */ >>>> + GString *last_exec; >>>> + GByteArray *reg_history[2]; >>>> + >>>> + int reg; >>>> +} CPU; >>>> + >>>> QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; >>>> >>> <snip> >>>> /* 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(cpus[cpu_index].last_exec, "%u, ", cpu_index); >>>> + g_string_append(cpus[cpu_index].last_exec, (char *)udata); >>>> + >>>> + g_rw_lock_reader_unlock(&expand_array_lock); >>>> } >>>> /** >>>> @@ -167,8 +197,10 @@ 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); >>>> + qemu_plugin_register_vcpu_insn_exec_cb( >>>> + insn, vcpu_insn_exec, >>>> + rfile_name ? QEMU_PLUGIN_CB_R_REGS : QEMU_PLUGIN_CB_NO_REGS, >>>> + output); >>>> /* reset skip */ >>>> skip = (imatches || amatches); >>>> @@ -177,17 +209,53 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) >>>> } >>>> } >>>> +static void vcpu_init(qemu_plugin_id_t id, unsigned int >>>> vcpu_index) >>>> +{ >>>> + int reg = 0; >>>> + bool found = false; >>>> + >>>> + expand_cpu(vcpu_index); >>>> + >>>> + if (rfile_name) { >>>> + int i; >>>> + int j; >>>> + int n; >>>> + >>>> + qemu_plugin_register_file_t *rfiles = >>>> + qemu_plugin_get_register_files(vcpu_index, &n); >>>> + >>>> + for (i = 0; i < n; i++) { >>>> + if (g_strcmp0(rfiles[i].name, rfile_name) == 0) { >>>> + for (j = 0; j < rfiles[i].num_regs; j++) { >>>> + if (g_strcmp0(rfiles[i].regs[j], reg_name) == 0) { >>>> + reg += j; >>>> + found = true; >>>> + break; >>>> + } >>>> + } >>>> + break; >>>> + } >>>> + >>>> + reg += rfiles[i].num_regs; >>>> + } >>>> + >>>> + g_free(rfiles); >>>> + } >>> This makes me question the value of exposing the register file >>> directly >>> to the plugin. I would much rather have a lookup utility function with >>> an optional tag. Something like: >>> plugin_reg_t qemu_plugin_find_register(const char *name, const >>> char *tag); >>> And make tag optional. I think in the general case "name" should be >>> enough. >> >> I have explained the reason why I introduced register file abstraction >> instead of adding a function to look up a register for an earlier >> version of this series: >>> I added a function that returns all register information instead of a >>> function that looks up a register so that a plugin can enumerate >>> registers. Such capability is useful for a plugin that dumps all >>> registers or a plugin that simulates processor (such a plugin may want >>> to warn if there are unknown registers). > > Fair enough. However I think a simple search interface will also be > useful for the more common case. I'll add one in a future version. > >> How would you define name and tag? They are something we currently do >> not have, and I'm trying to add new types of identifiers since such >> identifiers will be needed to be defined for different architectures >> and require documentation and extra work to avoid name conflicts and >> ensure interface stability. > > The name would be the register name which AFAICT are unique across the > system. If you have examples of clashes I'm curious as to what they are. Here I'm talking about creating and maintaining a set of identifiers independent of GDB. I'm suggesting to reuse the identifiers GDB use since they are already designed so that there are no clashes. > I'm still conflicted about baking gdb-isms into this ABI because they > aren't as stable as they could be either. Either way we do state: > > This is a new feature for QEMU and it does allow people to develop > out-of-tree plugins that can be dynamically linked into a running QEMU > process. However the project reserves the right to change or break the > API should it need to do so. The best way to avoid this is to submit > your plugin upstream so they can be updated if/when the API changes. > > So I'm not overly concerned about formalising a stable ABI for now. > >> >>> >>>> + >>>> + g_rw_lock_writer_lock(&expand_array_lock); >>>> + cpus[vcpu_index].reg = found ? reg : -1; >>>> + g_rw_lock_writer_unlock(&expand_array_lock); >>>> +} >>>> + >>>> /** >>>> * 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); >>>> + for (i = 0; i < num_cpus; i++) { >>>> + if (cpus[i].last_exec->str) { >>>> + qemu_plugin_outs(cpus[i].last_exec->str); >>>> qemu_plugin_outs("\n"); >>>> } >>>> } >>>> @@ -224,9 +292,7 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, >>>> * 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_new(CPU, info->system.max_vcpus); >>>> } >>>> for (int i = 0; i < argc; i++) { >>>> @@ -236,13 +302,23 @@ 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], "rfile") == 0) { >>>> + rfile_name = g_strdup(tokens[1]); >>>> + } else if (g_strcmp0(tokens[0], "reg") == 0) { >>>> + reg_name = g_strdup(tokens[1]); >>> And then instead of having the rfile/reg distinction support a >>> command >>> line like: >>> qemu-aarch64 -plugin >>> contrib/plugins/libexeclog.so,reg=sp,reg=x1,reg=sve:p1 >>> so if the user specifies a reg of the form TAG:REG we can pass that >>> as >>> the option tag string. It also avoids exposing all the details of gdb to >>> plugins while still allowing the utility function to search by rname >>> internally (even if only a substring match?), >> >> That implicitly assumes TAG does not contain a colon. I'm avoiding to >> make such an implicit assumption because it is a reference for plugin >> writers who may create out-of-tree plugins. We should retrain >> ourselves to tell the plugin writers not to make such an assumption >> that may not hold in the future version of QEMU. >> >> I consider a substring match harmful for a similar reason. There is no >> guarantee that a future version of QEMU will not introduce a new >> register that match with the existing substring and break interface >> stability. >> >> It is not necessary that identifiers are consistent with ones GDB use. >> What matters here is that the identifiers are documented, stable and >> immune from conflicts. > > We've a couple of cases of GDB having to issue new XML interface names > to handle cases where the previous definition missed important bits. > Hence my unease at exposing them to the plugin interface. Why you had to issue new feature names when adding missing definitions? I expect the reasoning for changing GDB interface names should also apply for TCG plugins: when not changing interface names will break GDB, it should be also likely to break TCG plugins using the affected features. I consider names used in GDB stable. GDB versions will break if they are not. > > The plugin interface shouldn't (yet?) be regarded as a stable interface > (c.f. above). The plugin interface has been for almost 5 years. If it is not stable yet, when it will be? In any case, the interface stability should be considered as an eventual goal; otherwise the existing infrastructure for plugin API versioning will make no sense.
diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst index 81dcd43a61..c9f8b27590 100644 --- a/docs/devel/tcg-plugins.rst +++ b/docs/devel/tcg-plugins.rst @@ -497,6 +497,15 @@ arguments if required:: $ qemu-system-arm $(QEMU_ARGS) \ -plugin ./contrib/plugins/libexeclog.so,ifilter=st1w,afilter=0x40001808 -d plugin +This plugin can also dump a specified register. The specification of register +follows `GDB standard target features <https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html>`__. + +Specify the name of the feature that contains the register and the name of the +register with ``rfile`` and ``reg`` options, respectively:: + + $ qemu-system-arm $(QEMU_ARGS) \ + -plugin ./contrib/plugins/libexeclog.so,rfile=org.gnu.gdb.arm.core,reg=sp -d plugin + - contrib/plugins/cache.c Cache modelling plugin that measures the performance of a given L1 cache @@ -583,4 +592,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 82dc2f584e..aa05840fd0 100644 --- a/contrib/plugins/execlog.c +++ b/contrib/plugins/execlog.c @@ -15,27 +15,43 @@ #include <qemu-plugin.h> +typedef struct CPU { + /* Store last executed instruction on each vCPU as a GString */ + GString *last_exec; + GByteArray *reg_history[2]; + + int reg; +} 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 CPU *cpus; +static int num_cpus; static GRWLock expand_array_lock; static GPtrArray *imatches; static GArray *amatches; +static char *rfile_name; +static char *reg_name; + /* - * Expand last_exec array. + * Expand cpu 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 void expand_cpu(int cpu_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); + if (cpu_index >= num_cpus) { + cpus = g_realloc_n(cpus, cpu_index + 1, sizeof(*cpus)); + while (cpu_index >= num_cpus) { + cpus[num_cpus].last_exec = g_string_new(NULL); + cpus[num_cpus].reg_history[0] = g_byte_array_new(); + cpus[num_cpus].reg_history[1] = g_byte_array_new(); + num_cpus++; + } } g_rw_lock_writer_unlock(&expand_array_lock); } @@ -50,8 +66,8 @@ static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t info, /* 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_assert(cpu_index < num_cpus); + s = cpus[cpu_index].last_exec; g_rw_lock_reader_unlock(&expand_array_lock); /* Indicate type of memory access */ @@ -77,28 +93,42 @@ static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t info, */ static void vcpu_insn_exec(unsigned int cpu_index, void *udata) { - GString *s; + int n; + int i; - /* 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); - } - s = g_ptr_array_index(last_exec, cpu_index); - g_rw_lock_reader_unlock(&expand_array_lock); /* Print previous instruction in cache */ - if (s->len) { - qemu_plugin_outs(s->str); + if (cpus[cpu_index].last_exec->len) { + if (cpus[cpu_index].reg >= 0) { + GByteArray *current = cpus[cpu_index].reg_history[0]; + GByteArray *last = cpus[cpu_index].reg_history[1]; + + g_byte_array_set_size(current, 0); + n = qemu_plugin_read_register(current, cpus[cpu_index].reg); + + if (n != last->len || memcmp(current->data, last->data, n)) { + g_string_append(cpus[cpu_index].last_exec, ", reg,"); + for (i = 0; i < n; i++) { + g_string_append_printf(cpus[cpu_index].last_exec, " %02x", + current->data[i]); + } + } + + cpus[cpu_index].reg_history[0] = last; + cpus[cpu_index].reg_history[1] = current; + } + + qemu_plugin_outs(cpus[cpu_index].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(cpus[cpu_index].last_exec, "%u, ", cpu_index); + g_string_append(cpus[cpu_index].last_exec, (char *)udata); + + g_rw_lock_reader_unlock(&expand_array_lock); } /** @@ -167,8 +197,10 @@ 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); + qemu_plugin_register_vcpu_insn_exec_cb( + insn, vcpu_insn_exec, + rfile_name ? QEMU_PLUGIN_CB_R_REGS : QEMU_PLUGIN_CB_NO_REGS, + output); /* reset skip */ skip = (imatches || amatches); @@ -177,17 +209,53 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) } } +static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index) +{ + int reg = 0; + bool found = false; + + expand_cpu(vcpu_index); + + if (rfile_name) { + int i; + int j; + int n; + + qemu_plugin_register_file_t *rfiles = + qemu_plugin_get_register_files(vcpu_index, &n); + + for (i = 0; i < n; i++) { + if (g_strcmp0(rfiles[i].name, rfile_name) == 0) { + for (j = 0; j < rfiles[i].num_regs; j++) { + if (g_strcmp0(rfiles[i].regs[j], reg_name) == 0) { + reg += j; + found = true; + break; + } + } + break; + } + + reg += rfiles[i].num_regs; + } + + g_free(rfiles); + } + + g_rw_lock_writer_lock(&expand_array_lock); + cpus[vcpu_index].reg = found ? reg : -1; + g_rw_lock_writer_unlock(&expand_array_lock); +} + /** * 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); + for (i = 0; i < num_cpus; i++) { + if (cpus[i].last_exec->str) { + qemu_plugin_outs(cpus[i].last_exec->str); qemu_plugin_outs("\n"); } } @@ -224,9 +292,7 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, * 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_new(CPU, info->system.max_vcpus); } for (int i = 0; i < argc; i++) { @@ -236,13 +302,23 @@ 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], "rfile") == 0) { + rfile_name = g_strdup(tokens[1]); + } else if (g_strcmp0(tokens[0], "reg") == 0) { + reg_name = g_strdup(tokens[1]); } else { fprintf(stderr, "option parsing failed: %s\n", opt); return -1; } } + if ((!rfile_name) != (!reg_name)) { + fputs("file and reg need to be set at the same time\n", stderr); + return -1; + } + /* Register 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);
This demonstrates how a register can be read from a plugin. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- docs/devel/tcg-plugins.rst | 10 ++- contrib/plugins/execlog.c | 140 ++++++++++++++++++++++++++++--------- 2 files changed, 117 insertions(+), 33 deletions(-)