Message ID | 20240226165646.425600-22-alex.bennee@linaro.org |
---|---|
State | Superseded |
Headers | show |
Series | maintainer updates for 9.0 pre-PR (tests, plugin register support) | expand |
On 2024/02/27 1:56, Alex Bennée wrote: > We can only request a list of registers once the vCPU has been > initialised so the user needs to use either call the get function on > vCPU initialisation or during the translation phase. > > We don't expose the reg number to the plugin instead hiding it behind > an opaque handle. For now this is just the gdb_regnum encapsulated in > an anonymous GPOINTER but in future as we add more state for plugins > to track we can expand it. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706 > Cc: Akihiko Odaki <akihiko.odaki@daynix.com> > Message-Id: <20240103173349.398526-39-alex.bennee@linaro.org> > Based-on: <20231025093128.33116-18-akihiko.odaki@daynix.com> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> Hi, Mostly looks good. I have a few trivial comments so please have a look at them. > > --- > v4 > - the get/read_registers functions are now implicitly for current > vCPU only to accidental cpu != current_cpu uses. > v5 > - make reg_handles as per-CPUPluginState variable. > v6 > - for now just wrap gdb_regnum > --- > include/qemu/qemu-plugin.h | 48 +++++++++++++++++++++++++++++-- > plugins/api.c | 56 ++++++++++++++++++++++++++++++++++++ > plugins/qemu-plugins.symbols | 2 ++ > 3 files changed, 104 insertions(+), 2 deletions(-) > > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h > index 93981f8f89f..3b6b18058d2 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> > @@ -229,8 +230,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, > @@ -707,4 +708,47 @@ uint64_t qemu_plugin_end_code(void); > QEMU_PLUGIN_API > uint64_t qemu_plugin_entry_code(void); > > +/** struct qemu_plugin_register - Opaque handle for register access */ > +struct qemu_plugin_register; > + > +/** > + * typedef qemu_plugin_reg_descriptor - register descriptions > + * > + * @handle: opaque handle for retrieving value with qemu_plugin_read_register > + * @name: register name > + * @feature: optional feature descriptor, can be NULL > + */ > +typedef struct { > + struct qemu_plugin_register *handle; > + const char *name; > + const char *feature; > +} qemu_plugin_reg_descriptor; > + > +/** > + * qemu_plugin_get_registers() - return register list for current vCPU > + * > + * Returns a GArray of qemu_plugin_reg_descriptor or NULL. Caller > + * frees the array (but not the const strings). > + * > + * Should be used from a qemu_plugin_register_vcpu_init_cb() callback > + * after the vCPU is initialised, i.e. in the vCPU context. > + */ > +GArray *qemu_plugin_get_registers(void); > + > +/** > + * qemu_plugin_read_register() - read register for current vCPU > + * > + * @handle: a @qemu_plugin_reg_handle handle > + * @buf: A GByteArray for the data owned by the plugin > + * > + * This function is only available in a context that register read access is > + * explicitly requested via the QEMU_PLUGIN_CB_R_REGS flag. > + * > + * Returns the size of the read register. The content of @buf is in target byte > + * order. On failure returns -1 > + */ > +int qemu_plugin_read_register(struct qemu_plugin_register *handle, > + GByteArray *buf); > + > + > #endif /* QEMU_QEMU_PLUGIN_H */ > diff --git a/plugins/api.c b/plugins/api.c > index 54df72c1c00..03412598047 100644 > --- a/plugins/api.c > +++ b/plugins/api.c > @@ -8,6 +8,7 @@ > * > * qemu_plugin_tb > * qemu_plugin_insn > + * qemu_plugin_register > * > * Which can then be passed back into the API to do additional things. > * As such all the public functions in here are exported in > @@ -35,10 +36,12 @@ > */ > > #include "qemu/osdep.h" > +#include "qemu/main-loop.h" > #include "qemu/plugin.h" > #include "qemu/log.h" > #include "tcg/tcg.h" > #include "exec/exec-all.h" > +#include "exec/gdbstub.h" > #include "exec/ram_addr.h" > #include "disas/disas.h" > #include "plugin.h" > @@ -410,3 +413,56 @@ uint64_t qemu_plugin_entry_code(void) > #endif > return entry; > } > + > +/* > + * Create register handles. > + * > + * We need to create a handle for each register so the plugin > + * infrastructure can call gdbstub to read a register. They are > + * currently just a pointer encapsulation of the gdb_regnum but in > + * future may hold internal plugin state so its important plugin > + * authors are not tempted to treat them as numbers. > + * > + * We also construct a result array with those handles and some > + * ancillary data the plugin might find useful. > + */ > + > +static GArray *create_register_handles(CPUState *cs, GArray *gdbstub_regs) > +{ cs is unused. > + GArray *find_data = g_array_new(true, true, > + sizeof(qemu_plugin_reg_descriptor)); > + > + for (int i = 0; i < gdbstub_regs->len; i++) { > + GDBRegDesc *grd = &g_array_index(gdbstub_regs, GDBRegDesc, i); > + > + /* skip "un-named" regs */ > + if (!grd->name) { > + continue; > + } > + > + /* Create a record for the plugin */ > + qemu_plugin_reg_descriptor desc = { > + .handle = GINT_TO_POINTER(grd->gdb_reg), > + .name = g_intern_string(grd->name), > + .feature = g_intern_string(grd->feature_name) > + }; Please remove a mixed declaration; see: docs/devel/style.rst > + g_array_append_val(find_data, desc); > + } > + > + return find_data; > +} > + > +GArray *qemu_plugin_get_registers(void) > +{ > + g_assert(current_cpu); > + > + g_autoptr(GArray) regs = gdb_get_register_list(current_cpu); > + return regs->len ? create_register_handles(current_cpu, regs) : NULL; Why do you need regs->len check? > +} > + > +int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf) > +{ > + g_assert(current_cpu); > + > + return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg)); > +} > diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols > index adb67608598..27fe97239be 100644 > --- a/plugins/qemu-plugins.symbols > +++ b/plugins/qemu-plugins.symbols > @@ -3,6 +3,7 @@ > qemu_plugin_end_code; > qemu_plugin_entry_code; > qemu_plugin_get_hwaddr; > + qemu_plugin_get_registers; > qemu_plugin_hwaddr_device_name; > qemu_plugin_hwaddr_is_io; > qemu_plugin_hwaddr_phys_addr; > @@ -19,6 +20,7 @@ > qemu_plugin_num_vcpus; > qemu_plugin_outs; > qemu_plugin_path_to_binary; > + qemu_plugin_read_register; > qemu_plugin_register_atexit_cb; > qemu_plugin_register_flush_cb; > qemu_plugin_register_vcpu_exit_cb;
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > On 2024/02/27 1:56, Alex Bennée wrote: >> We can only request a list of registers once the vCPU has been >> initialised so the user needs to use either call the get function on >> vCPU initialisation or during the translation phase. >> We don't expose the reg number to the plugin instead hiding it >> behind >> an opaque handle. For now this is just the gdb_regnum encapsulated in >> an anonymous GPOINTER but in future as we add more state for plugins >> to track we can expand it. >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706 >> Cc: Akihiko Odaki <akihiko.odaki@daynix.com> >> Message-Id: <20240103173349.398526-39-alex.bennee@linaro.org> >> Based-on: <20231025093128.33116-18-akihiko.odaki@daynix.com> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > > Hi, > > Mostly looks good. I have a few trivial comments so please have a look > at them. Done <snip> >> + g_array_append_val(find_data, desc); >> + } >> + >> + return find_data; >> +} >> + >> +GArray *qemu_plugin_get_registers(void) >> +{ >> + g_assert(current_cpu); >> + >> + g_autoptr(GArray) regs = gdb_get_register_list(current_cpu); >> + return regs->len ? create_register_handles(current_cpu, regs) : NULL; > > Why do you need regs->len check? Not all guests expose register to gdb so we need to catch that: TEST catch-syscalls-with-libinsn.so on alpha ** ERROR:../../plugins/api.c:459:qemu_plugin_get_registers: assertion failed: (regs->len) Aborted <snip>
On 2024/02/27 19:08, Alex Bennée wrote: > Akihiko Odaki <akihiko.odaki@daynix.com> writes: > >> On 2024/02/27 1:56, Alex Bennée wrote: >>> We can only request a list of registers once the vCPU has been >>> initialised so the user needs to use either call the get function on >>> vCPU initialisation or during the translation phase. >>> We don't expose the reg number to the plugin instead hiding it >>> behind >>> an opaque handle. For now this is just the gdb_regnum encapsulated in >>> an anonymous GPOINTER but in future as we add more state for plugins >>> to track we can expand it. >>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706 >>> Cc: Akihiko Odaki <akihiko.odaki@daynix.com> >>> Message-Id: <20240103173349.398526-39-alex.bennee@linaro.org> >>> Based-on: <20231025093128.33116-18-akihiko.odaki@daynix.com> >>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> >> Hi, >> >> Mostly looks good. I have a few trivial comments so please have a look >> at them. > > Done > > <snip> >>> + g_array_append_val(find_data, desc); >>> + } >>> + >>> + return find_data; >>> +} >>> + >>> +GArray *qemu_plugin_get_registers(void) >>> +{ >>> + g_assert(current_cpu); >>> + >>> + g_autoptr(GArray) regs = gdb_get_register_list(current_cpu); >>> + return regs->len ? create_register_handles(current_cpu, regs) : NULL; >> >> Why do you need regs->len check? > > Not all guests expose register to gdb so we need to catch that: > > TEST catch-syscalls-with-libinsn.so on alpha > ** > ERROR:../../plugins/api.c:459:qemu_plugin_get_registers: assertion failed: (regs->len) > Aborted Certainly regs->len can be 0, but why do you need to return NULL in that case? Can't qemu_plugin_get_registers() return an empty array just as gdb_get_register_list() does?
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > On 2024/02/27 19:08, Alex Bennée wrote: >> Akihiko Odaki <akihiko.odaki@daynix.com> writes: >> >>> On 2024/02/27 1:56, Alex Bennée wrote: >>>> We can only request a list of registers once the vCPU has been >>>> initialised so the user needs to use either call the get function on >>>> vCPU initialisation or during the translation phase. >>>> We don't expose the reg number to the plugin instead hiding it >>>> behind >>>> an opaque handle. For now this is just the gdb_regnum encapsulated in >>>> an anonymous GPOINTER but in future as we add more state for plugins >>>> to track we can expand it. >>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706 >>>> Cc: Akihiko Odaki <akihiko.odaki@daynix.com> >>>> Message-Id: <20240103173349.398526-39-alex.bennee@linaro.org> >>>> Based-on: <20231025093128.33116-18-akihiko.odaki@daynix.com> >>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>> >>> Hi, >>> >>> Mostly looks good. I have a few trivial comments so please have a look >>> at them. >> Done >> <snip> >>>> + g_array_append_val(find_data, desc); >>>> + } >>>> + >>>> + return find_data; >>>> +} >>>> + >>>> +GArray *qemu_plugin_get_registers(void) >>>> +{ >>>> + g_assert(current_cpu); >>>> + >>>> + g_autoptr(GArray) regs = gdb_get_register_list(current_cpu); >>>> + return regs->len ? create_register_handles(current_cpu, regs) : NULL; >>> >>> Why do you need regs->len check? >> Not all guests expose register to gdb so we need to catch that: >> TEST catch-syscalls-with-libinsn.so on alpha >> ** >> ERROR:../../plugins/api.c:459:qemu_plugin_get_registers: assertion failed: (regs->len) >> Aborted > Certainly regs->len can be 0, but why do you need to return NULL in > that case? Can't qemu_plugin_get_registers() return an empty array > just as gdb_get_register_list() does? That seems more troublesome to handle the other end. NULL is nothing is a fairly common pattern here which makes short-circuiting the array iteration simple.
On 2024/02/27 19:29, Alex Bennée wrote: > Akihiko Odaki <akihiko.odaki@daynix.com> writes: > >> On 2024/02/27 19:08, Alex Bennée wrote: >>> Akihiko Odaki <akihiko.odaki@daynix.com> writes: >>> >>>> On 2024/02/27 1:56, Alex Bennée wrote: >>>>> We can only request a list of registers once the vCPU has been >>>>> initialised so the user needs to use either call the get function on >>>>> vCPU initialisation or during the translation phase. >>>>> We don't expose the reg number to the plugin instead hiding it >>>>> behind >>>>> an opaque handle. For now this is just the gdb_regnum encapsulated in >>>>> an anonymous GPOINTER but in future as we add more state for plugins >>>>> to track we can expand it. >>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706 >>>>> Cc: Akihiko Odaki <akihiko.odaki@daynix.com> >>>>> Message-Id: <20240103173349.398526-39-alex.bennee@linaro.org> >>>>> Based-on: <20231025093128.33116-18-akihiko.odaki@daynix.com> >>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >>>>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>>> >>>> Hi, >>>> >>>> Mostly looks good. I have a few trivial comments so please have a look >>>> at them. >>> Done >>> <snip> >>>>> + g_array_append_val(find_data, desc); >>>>> + } >>>>> + >>>>> + return find_data; >>>>> +} >>>>> + >>>>> +GArray *qemu_plugin_get_registers(void) >>>>> +{ >>>>> + g_assert(current_cpu); >>>>> + >>>>> + g_autoptr(GArray) regs = gdb_get_register_list(current_cpu); >>>>> + return regs->len ? create_register_handles(current_cpu, regs) : NULL; >>>> >>>> Why do you need regs->len check? >>> Not all guests expose register to gdb so we need to catch that: >>> TEST catch-syscalls-with-libinsn.so on alpha >>> ** >>> ERROR:../../plugins/api.c:459:qemu_plugin_get_registers: assertion failed: (regs->len) >>> Aborted >> Certainly regs->len can be 0, but why do you need to return NULL in >> that case? Can't qemu_plugin_get_registers() return an empty array >> just as gdb_get_register_list() does? > > That seems more troublesome to handle the other end. NULL is nothing is > a fairly common pattern here which makes short-circuiting the array > iteration simple. I'm not sure why you would want to short-circuiting array iteration. Iterating an empty array is often simpler. In any case, the same logic also applies to gdb_get_register_list(), so you should change gdb_get_register_list() to return NULL instead of an empty array if you think that's more reasonable.
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h index 93981f8f89f..3b6b18058d2 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> @@ -229,8 +230,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, @@ -707,4 +708,47 @@ uint64_t qemu_plugin_end_code(void); QEMU_PLUGIN_API uint64_t qemu_plugin_entry_code(void); +/** struct qemu_plugin_register - Opaque handle for register access */ +struct qemu_plugin_register; + +/** + * typedef qemu_plugin_reg_descriptor - register descriptions + * + * @handle: opaque handle for retrieving value with qemu_plugin_read_register + * @name: register name + * @feature: optional feature descriptor, can be NULL + */ +typedef struct { + struct qemu_plugin_register *handle; + const char *name; + const char *feature; +} qemu_plugin_reg_descriptor; + +/** + * qemu_plugin_get_registers() - return register list for current vCPU + * + * Returns a GArray of qemu_plugin_reg_descriptor or NULL. Caller + * frees the array (but not the const strings). + * + * Should be used from a qemu_plugin_register_vcpu_init_cb() callback + * after the vCPU is initialised, i.e. in the vCPU context. + */ +GArray *qemu_plugin_get_registers(void); + +/** + * qemu_plugin_read_register() - read register for current vCPU + * + * @handle: a @qemu_plugin_reg_handle handle + * @buf: A GByteArray for the data owned by the plugin + * + * This function is only available in a context that register read access is + * explicitly requested via the QEMU_PLUGIN_CB_R_REGS flag. + * + * Returns the size of the read register. The content of @buf is in target byte + * order. On failure returns -1 + */ +int qemu_plugin_read_register(struct qemu_plugin_register *handle, + GByteArray *buf); + + #endif /* QEMU_QEMU_PLUGIN_H */ diff --git a/plugins/api.c b/plugins/api.c index 54df72c1c00..03412598047 100644 --- a/plugins/api.c +++ b/plugins/api.c @@ -8,6 +8,7 @@ * * qemu_plugin_tb * qemu_plugin_insn + * qemu_plugin_register * * Which can then be passed back into the API to do additional things. * As such all the public functions in here are exported in @@ -35,10 +36,12 @@ */ #include "qemu/osdep.h" +#include "qemu/main-loop.h" #include "qemu/plugin.h" #include "qemu/log.h" #include "tcg/tcg.h" #include "exec/exec-all.h" +#include "exec/gdbstub.h" #include "exec/ram_addr.h" #include "disas/disas.h" #include "plugin.h" @@ -410,3 +413,56 @@ uint64_t qemu_plugin_entry_code(void) #endif return entry; } + +/* + * Create register handles. + * + * We need to create a handle for each register so the plugin + * infrastructure can call gdbstub to read a register. They are + * currently just a pointer encapsulation of the gdb_regnum but in + * future may hold internal plugin state so its important plugin + * authors are not tempted to treat them as numbers. + * + * We also construct a result array with those handles and some + * ancillary data the plugin might find useful. + */ + +static GArray *create_register_handles(CPUState *cs, GArray *gdbstub_regs) +{ + GArray *find_data = g_array_new(true, true, + sizeof(qemu_plugin_reg_descriptor)); + + for (int i = 0; i < gdbstub_regs->len; i++) { + GDBRegDesc *grd = &g_array_index(gdbstub_regs, GDBRegDesc, i); + + /* skip "un-named" regs */ + if (!grd->name) { + continue; + } + + /* Create a record for the plugin */ + qemu_plugin_reg_descriptor desc = { + .handle = GINT_TO_POINTER(grd->gdb_reg), + .name = g_intern_string(grd->name), + .feature = g_intern_string(grd->feature_name) + }; + g_array_append_val(find_data, desc); + } + + return find_data; +} + +GArray *qemu_plugin_get_registers(void) +{ + g_assert(current_cpu); + + g_autoptr(GArray) regs = gdb_get_register_list(current_cpu); + return regs->len ? create_register_handles(current_cpu, regs) : NULL; +} + +int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf) +{ + g_assert(current_cpu); + + return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg)); +} diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols index adb67608598..27fe97239be 100644 --- a/plugins/qemu-plugins.symbols +++ b/plugins/qemu-plugins.symbols @@ -3,6 +3,7 @@ qemu_plugin_end_code; qemu_plugin_entry_code; qemu_plugin_get_hwaddr; + qemu_plugin_get_registers; qemu_plugin_hwaddr_device_name; qemu_plugin_hwaddr_is_io; qemu_plugin_hwaddr_phys_addr; @@ -19,6 +20,7 @@ qemu_plugin_num_vcpus; qemu_plugin_outs; qemu_plugin_path_to_binary; + qemu_plugin_read_register; qemu_plugin_register_atexit_cb; qemu_plugin_register_flush_cb; qemu_plugin_register_vcpu_exit_cb;