| Message ID | 20230731084354.115015-22-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | plugins: Allow to read registers | expand |
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > It is based on GDB protocol to ensure interface stability. See comments bellow. > The timing of the vcpu init hook is also changed so that the hook will > get called after GDB features are initialized. This might be worth splitting to a separate patch for cleaner bisecting. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > include/qemu/qemu-plugin.h | 65 ++++++++++++++++++++++++++++++++++-- > cpu.c | 11 ------ > hw/core/cpu-common.c | 10 ++++++ > plugins/api.c | 40 ++++++++++++++++++++++ > plugins/qemu-plugins.symbols | 2 ++ > 5 files changed, 114 insertions(+), 14 deletions(-) > > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h > index 50a9957279..214b12bfd6 100644 > --- a/include/qemu/qemu-plugin.h > +++ b/include/qemu/qemu-plugin.h > @@ -11,6 +11,7 @@ > #ifndef QEMU_QEMU_PLUGIN_H > #define QEMU_QEMU_PLUGIN_H > > +#include <glib.h> > #include <inttypes.h> > #include <stdbool.h> > #include <stddef.h> > @@ -51,7 +52,7 @@ typedef uint64_t qemu_plugin_id_t; > > extern QEMU_PLUGIN_EXPORT int qemu_plugin_version; > > -#define QEMU_PLUGIN_VERSION 1 > +#define QEMU_PLUGIN_VERSION 2 > > /** > * struct qemu_info_t - system information for plugins > @@ -218,8 +219,8 @@ struct qemu_plugin_insn; > * @QEMU_PLUGIN_CB_R_REGS: callback reads the CPU's regs > * @QEMU_PLUGIN_CB_RW_REGS: callback reads and writes the CPU's regs > * > - * Note: currently unused, plugins cannot read or change system > - * register state. > + * Note: currently QEMU_PLUGIN_CB_RW_REGS is unused, plugins cannot change > + * system register state. > */ > enum qemu_plugin_cb_flags { > QEMU_PLUGIN_CB_NO_REGS, > @@ -664,4 +665,62 @@ uint64_t qemu_plugin_end_code(void); > */ > uint64_t qemu_plugin_entry_code(void); > > +/** > + * struct qemu_plugin_register_file_t - register information > + * > + * This structure identifies registers. The identifiers included in this > + * structure are identical with names used in GDB's standard target features > + * with some extensions. For details, see: > + * > https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html I'm not super keen on baking GDB-isms into the plugin register interface. > + * > + * A register is uniquely identified with the combination of a feature name > + * and a register name or a register number. It is recommended to derive > + * register numbers from feature names and register names each time a new vcpu > + * starts. Do you have examples of clashing register names from different feature sets? > + * > + * To derive the register number from a feature name and a register name, > + * first look up qemu_plugin_register_file_t with the feature name, and then > + * look up the register name in its @regs. The sum of the @base_reg and the > + * index in the @reg is the register number. > + * > + * Note that @regs may have holes; some elements of @regs may be NULL. > + */ > +typedef struct qemu_plugin_register_file_t { > + /** @name: feature name */ > + const char *name; > + /** @regs: register names */ > + const char * const *regs; > + /** @base_reg: the base identified number */ > + int base_reg; > + /** @num_regs: the number of elements in @regs */ > + int num_regs; > +} qemu_plugin_register_file_t; > + > +/** > + * qemu_plugin_get_register_files() - returns register information > + * > + * @vcpu_index: the index of the vcpu context > + * @size: the pointer to the variable to hold the number of returned elements > + * > + * Returns an array of qemu_plugin_register_file_t. The user should g_free() > + * the array once no longer needed. > + */ > +qemu_plugin_register_file_t * > +qemu_plugin_get_register_files(unsigned int vcpu_index, int *size); I think I'd rather have a simpler interface that returns an anonymous handle to the plugin. For example: struct qemu_plugin_register; struct qemu_plugin_register qemu_plugin_find_register(const char *name); > + > +/** > + * qemu_plugin_read_register() - read register > + * > + * @buf: the byte array to append the read register content to. > + * @reg: the register identifier determined with > + * qemu_plugin_get_register_files(). > + * > + * This function is only available in a context that register read access is > + * explicitly requested. > + * > + * Returns the size of the read register. The content of @buf is in target byte > + * order. > + */ > +int qemu_plugin_read_register(GByteArray *buf, int reg); and this then becomes: int qemu_plugin_read_register(GByteArray *buf, struct qemu_plugin_register); in practice these can become anonymous pointers which hide the implementation details from the plugin itself. Then the details of mapping the register to a gdb regnum can be kept in the plugin code keeping us free to further re-factor the code as we go. The plugin code works quite hard to try and avoid leaking implementation details to plugins so as not to tie QEMU's hands in re-factoring. While the interface provided is technically GDB's, not QEMUs I don't think its a particularly nice one to expose. <snip>
On 2023/08/15 0:05, Alex Bennée wrote: > > Akihiko Odaki <akihiko.odaki@daynix.com> writes: > >> It is based on GDB protocol to ensure interface stability. > > See comments bellow. > >> The timing of the vcpu init hook is also changed so that the hook will >> get called after GDB features are initialized. > > This might be worth splitting to a separate patch for cleaner bisecting. > >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> include/qemu/qemu-plugin.h | 65 ++++++++++++++++++++++++++++++++++-- >> cpu.c | 11 ------ >> hw/core/cpu-common.c | 10 ++++++ >> plugins/api.c | 40 ++++++++++++++++++++++ >> plugins/qemu-plugins.symbols | 2 ++ >> 5 files changed, 114 insertions(+), 14 deletions(-) >> >> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h >> index 50a9957279..214b12bfd6 100644 >> --- a/include/qemu/qemu-plugin.h >> +++ b/include/qemu/qemu-plugin.h >> @@ -11,6 +11,7 @@ >> #ifndef QEMU_QEMU_PLUGIN_H >> #define QEMU_QEMU_PLUGIN_H >> >> +#include <glib.h> >> #include <inttypes.h> >> #include <stdbool.h> >> #include <stddef.h> >> @@ -51,7 +52,7 @@ typedef uint64_t qemu_plugin_id_t; >> >> extern QEMU_PLUGIN_EXPORT int qemu_plugin_version; >> >> -#define QEMU_PLUGIN_VERSION 1 >> +#define QEMU_PLUGIN_VERSION 2 >> >> /** >> * struct qemu_info_t - system information for plugins >> @@ -218,8 +219,8 @@ struct qemu_plugin_insn; >> * @QEMU_PLUGIN_CB_R_REGS: callback reads the CPU's regs >> * @QEMU_PLUGIN_CB_RW_REGS: callback reads and writes the CPU's regs >> * >> - * Note: currently unused, plugins cannot read or change system >> - * register state. >> + * Note: currently QEMU_PLUGIN_CB_RW_REGS is unused, plugins cannot change >> + * system register state. >> */ >> enum qemu_plugin_cb_flags { >> QEMU_PLUGIN_CB_NO_REGS, >> @@ -664,4 +665,62 @@ uint64_t qemu_plugin_end_code(void); >> */ >> uint64_t qemu_plugin_entry_code(void); >> >> +/** >> + * struct qemu_plugin_register_file_t - register information >> + * >> + * This structure identifies registers. The identifiers included in this >> + * structure are identical with names used in GDB's standard target features >> + * with some extensions. For details, see: >> + * >> https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html > > I'm not super keen on baking GDB-isms into the plugin register > interface. I used GDB names here because I just didn't want to invent feature register names again. We can use any other register name dictionary if desired. > >> + * >> + * A register is uniquely identified with the combination of a feature name >> + * and a register name or a register number. It is recommended to derive >> + * register numbers from feature names and register names each time a new vcpu >> + * starts. > > Do you have examples of clashing register names from different feature > sets? No. A possible situation that the feature name matters is that a vendor extension becomes a standard extension with some modifications. In such a case, plugins written for the ratified standard extension may reject to work with the processor with the original vendor extension. While such a situation has not happened for QEMU and it's unlikely to happen, I'm advising to use feature name for register identification just for caution. > >> + * >> + * To derive the register number from a feature name and a register name, >> + * first look up qemu_plugin_register_file_t with the feature name, and then >> + * look up the register name in its @regs. The sum of the @base_reg and the >> + * index in the @reg is the register number. >> + * >> + * Note that @regs may have holes; some elements of @regs may be NULL. >> + */ >> +typedef struct qemu_plugin_register_file_t { >> + /** @name: feature name */ >> + const char *name; >> + /** @regs: register names */ >> + const char * const *regs; >> + /** @base_reg: the base identified number */ >> + int base_reg; >> + /** @num_regs: the number of elements in @regs */ >> + int num_regs; >> +} qemu_plugin_register_file_t; >> + >> +/** >> + * qemu_plugin_get_register_files() - returns register information >> + * >> + * @vcpu_index: the index of the vcpu context >> + * @size: the pointer to the variable to hold the number of returned elements >> + * >> + * Returns an array of qemu_plugin_register_file_t. The user should g_free() >> + * the array once no longer needed. >> + */ >> +qemu_plugin_register_file_t * >> +qemu_plugin_get_register_files(unsigned int vcpu_index, int *size); > > I think I'd rather have a simpler interface that returns an anonymous > handle to the plugin. For example: > > struct qemu_plugin_register; > struct qemu_plugin_register qemu_plugin_find_register(const char *name); > >> + >> +/** >> + * qemu_plugin_read_register() - read register >> + * >> + * @buf: the byte array to append the read register content to. >> + * @reg: the register identifier determined with >> + * qemu_plugin_get_register_files(). >> + * >> + * This function is only available in a context that register read access is >> + * explicitly requested. >> + * >> + * Returns the size of the read register. The content of @buf is in target byte >> + * order. >> + */ >> +int qemu_plugin_read_register(GByteArray *buf, int reg); > > and this then becomes: > > int qemu_plugin_read_register(GByteArray *buf, struct qemu_plugin_register); > > in practice these can become anonymous pointers which hide the > implementation details from the plugin itself. Then the details of > mapping the register to a gdb regnum can be kept in the plugin code > keeping us free to further re-factor the code as we go. > > The plugin code works quite hard to try and avoid leaking implementation > details to plugins so as not to tie QEMU's hands in re-factoring. While > the interface provided is technically GDB's, not QEMUs I don't think its > a particularly nice one to expose. Unfortunately "struct qemu_plugin_register" will not work. C requires the size of the struct to be known before a declaration of a function that uses the struct as a return value or parameter. The best thing we can do is to add typedef just in a manner similar to qemu_plugin_id_t. 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). I'm just using GDB regnum here because I just needed some numbers. The numbers are actually arbitrary and I intentionally did not assure that the numbers are identical with what GDB use in the documentation comments. We may use any other arbitrary numbers as we want in the future. > > <snip> >
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h index 50a9957279..214b12bfd6 100644 --- a/include/qemu/qemu-plugin.h +++ b/include/qemu/qemu-plugin.h @@ -11,6 +11,7 @@ #ifndef QEMU_QEMU_PLUGIN_H #define QEMU_QEMU_PLUGIN_H +#include <glib.h> #include <inttypes.h> #include <stdbool.h> #include <stddef.h> @@ -51,7 +52,7 @@ typedef uint64_t qemu_plugin_id_t; extern QEMU_PLUGIN_EXPORT int qemu_plugin_version; -#define QEMU_PLUGIN_VERSION 1 +#define QEMU_PLUGIN_VERSION 2 /** * struct qemu_info_t - system information for plugins @@ -218,8 +219,8 @@ struct qemu_plugin_insn; * @QEMU_PLUGIN_CB_R_REGS: callback reads the CPU's regs * @QEMU_PLUGIN_CB_RW_REGS: callback reads and writes the CPU's regs * - * Note: currently unused, plugins cannot read or change system - * register state. + * Note: currently QEMU_PLUGIN_CB_RW_REGS is unused, plugins cannot change + * system register state. */ enum qemu_plugin_cb_flags { QEMU_PLUGIN_CB_NO_REGS, @@ -664,4 +665,62 @@ uint64_t qemu_plugin_end_code(void); */ uint64_t qemu_plugin_entry_code(void); +/** + * struct qemu_plugin_register_file_t - register information + * + * This structure identifies registers. The identifiers included in this + * structure are identical with names used in GDB's standard target features + * with some extensions. For details, see: + * https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html + * + * A register is uniquely identified with the combination of a feature name + * and a register name or a register number. It is recommended to derive + * register numbers from feature names and register names each time a new vcpu + * starts. + * + * To derive the register number from a feature name and a register name, + * first look up qemu_plugin_register_file_t with the feature name, and then + * look up the register name in its @regs. The sum of the @base_reg and the + * index in the @reg is the register number. + * + * Note that @regs may have holes; some elements of @regs may be NULL. + */ +typedef struct qemu_plugin_register_file_t { + /** @name: feature name */ + const char *name; + /** @regs: register names */ + const char * const *regs; + /** @base_reg: the base identified number */ + int base_reg; + /** @num_regs: the number of elements in @regs */ + int num_regs; +} qemu_plugin_register_file_t; + +/** + * qemu_plugin_get_register_files() - returns register information + * + * @vcpu_index: the index of the vcpu context + * @size: the pointer to the variable to hold the number of returned elements + * + * Returns an array of qemu_plugin_register_file_t. The user should g_free() + * the array once no longer needed. + */ +qemu_plugin_register_file_t * +qemu_plugin_get_register_files(unsigned int vcpu_index, int *size); + +/** + * qemu_plugin_read_register() - read register + * + * @buf: the byte array to append the read register content to. + * @reg: the register identifier determined with + * qemu_plugin_get_register_files(). + * + * This function is only available in a context that register read access is + * explicitly requested. + * + * Returns the size of the read register. The content of @buf is in target byte + * order. + */ +int qemu_plugin_read_register(GByteArray *buf, int reg); + #endif /* QEMU_QEMU_PLUGIN_H */ diff --git a/cpu.c b/cpu.c index 1c948d1161..2552c85249 100644 --- a/cpu.c +++ b/cpu.c @@ -42,7 +42,6 @@ #include "hw/core/accel-cpu.h" #include "trace/trace-root.h" #include "qemu/accel.h" -#include "qemu/plugin.h" uintptr_t qemu_host_page_size; intptr_t qemu_host_page_mask; @@ -148,11 +147,6 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp) /* Wait until cpu initialization complete before exposing cpu. */ cpu_list_add(cpu); - /* Plugin initialization must wait until cpu_index assigned. */ - if (tcg_enabled()) { - qemu_plugin_vcpu_init_hook(cpu); - } - #ifdef CONFIG_USER_ONLY assert(qdev_get_vmsd(DEVICE(cpu)) == NULL || qdev_get_vmsd(DEVICE(cpu))->unmigratable); @@ -179,11 +173,6 @@ void cpu_exec_unrealizefn(CPUState *cpu) } #endif - /* Call the plugin hook before clearing cpu->cpu_index in cpu_list_remove */ - if (tcg_enabled()) { - qemu_plugin_vcpu_exit_hook(cpu); - } - cpu_list_remove(cpu); /* * Now that the vCPU has been removed from the RCU list, we can call diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c index 549f52f46f..e06a70007a 100644 --- a/hw/core/cpu-common.c +++ b/hw/core/cpu-common.c @@ -211,6 +211,11 @@ static void cpu_common_realizefn(DeviceState *dev, Error **errp) cpu_resume(cpu); } + /* Plugin initialization must wait until the cpu is fully realized. */ + if (tcg_enabled()) { + qemu_plugin_vcpu_init_hook(cpu); + } + /* NOTE: latest generic point where the cpu is fully realized */ } @@ -218,6 +223,11 @@ static void cpu_common_unrealizefn(DeviceState *dev) { CPUState *cpu = CPU(dev); + /* Call the plugin hook before clearing the cpu is fully unrealized */ + if (tcg_enabled()) { + qemu_plugin_vcpu_exit_hook(cpu); + } + /* NOTE: latest generic point before the cpu is fully unrealized */ cpu_exec_unrealizefn(cpu); } diff --git a/plugins/api.c b/plugins/api.c index 2078b16edb..dd7ff5067b 100644 --- a/plugins/api.c +++ b/plugins/api.c @@ -35,6 +35,7 @@ */ #include "qemu/osdep.h" +#include "qemu/main-loop.h" #include "qemu/plugin.h" #include "qemu/log.h" #include "tcg/tcg.h" @@ -442,3 +443,42 @@ uint64_t qemu_plugin_entry_code(void) #endif return entry; } + +static void count_gdb_feature(void *opaque, const GDBFeature *feature, + int base_reg) +{ + (*(int *)opaque)++; +} + +static void map_gdb_feature(void *opaque, const GDBFeature *feature, + int base_reg) +{ + qemu_plugin_register_file_t **cursor = opaque; + (*cursor)->name = feature->name; + (*cursor)->regs = feature->regs; + (*cursor)->base_reg = base_reg; + (*cursor)->num_regs = feature->num_regs; + (*cursor)++; +} + +qemu_plugin_register_file_t * +qemu_plugin_get_register_files(unsigned int vcpu_index, int *size) +{ + QEMU_IOTHREAD_LOCK_GUARD(); + + *size = 0; + gdb_foreach_feature(qemu_get_cpu(vcpu_index), count_gdb_feature, size); + + qemu_plugin_register_file_t *files = + g_new(qemu_plugin_register_file_t, *size); + + qemu_plugin_register_file_t *cursor = files; + gdb_foreach_feature(qemu_get_cpu(vcpu_index), map_gdb_feature, &cursor); + + return files; +} + +int qemu_plugin_read_register(GByteArray *buf, int reg) +{ + return gdb_read_register(current_cpu, buf, reg, true); +} diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols index 71f6c90549..4ed9e70e47 100644 --- a/plugins/qemu-plugins.symbols +++ b/plugins/qemu-plugins.symbols @@ -42,4 +42,6 @@ qemu_plugin_tb_vaddr; qemu_plugin_uninstall; qemu_plugin_vcpu_for_each; + qemu_plugin_get_register_files; + qemu_plugin_read_register; };
It is based on GDB protocol to ensure interface stability. The timing of the vcpu init hook is also changed so that the hook will get called after GDB features are initialized. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- include/qemu/qemu-plugin.h | 65 ++++++++++++++++++++++++++++++++++-- cpu.c | 11 ------ hw/core/cpu-common.c | 10 ++++++ plugins/api.c | 40 ++++++++++++++++++++++ plugins/qemu-plugins.symbols | 2 ++ 5 files changed, 114 insertions(+), 14 deletions(-)