On 2024/02/27 23:43, 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>
>
> ---
> 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
> v7
> - minor style fixes
> ---
> include/qemu/qemu-plugin.h | 48 +++++++++++++++++++++++++++++--
> plugins/api.c | 55 ++++++++++++++++++++++++++++++++++++
> plugins/qemu-plugins.symbols | 2 ++
> 3 files changed, 103 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 93981f8f89f..6c5580f4428 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 potentially empty GArray of qemu_plugin_reg_descriptor.
> + * 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.
> + */
Qualify with QEMU_PLUGIN_API, which was apparently added after this
patch was authored.
> +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
Add a period after -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..908fe7e6fa3 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,55 @@ 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
s/gdb_regnum/gdb_reg/
With all comments fixed,
Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> + * 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(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);
> + qemu_plugin_reg_descriptor desc;
> +
> + /* skip "un-named" regs */
> + if (!grd->name) {
> + continue;
> + }
> +
> + /* Create a record for the plugin */
> + desc.handle = GINT_TO_POINTER(grd->gdb_reg);
> + desc.name = g_intern_string(grd->name);
> + desc.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 create_register_handles(regs);
> +}
> +
> +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;