Re: [PATCH v2 38/43] plugins: add an API to read registers

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 1/3/24 21:33, 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. This allows for a bit of future proofing should the
> internals need to be changed while also being hashed against the
> CPUClass so we can handle different register sets per-vCPU in
> hetrogenous situations.
> 
> Having an internal state within the plugins also allows us to expand
> the interface in future (for example providing callbacks on register
> change if the translator can track changes).
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1706
> Cc: Akihiko Odaki <akihiko.odaki@daynix.com>
> Based-on: <20231025093128.33116-18-akihiko.odaki@daynix.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v3
>    - also g_intern_string the register name
>    - make get_registers documentation a bit less verbose
> v2
>    - use new get whole list api, and expose upwards
> 
> vAJB:
> 
> The main difference to Akikio's version is hiding the gdb register
> detail from the plugin for the reasons described above.
> ---
>   include/qemu/qemu-plugin.h   |  51 +++++++++++++++++-
>   plugins/api.c                | 102 +++++++++++++++++++++++++++++++++++
>   plugins/qemu-plugins.symbols |   2 +
>   3 files changed, 153 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 4daab6efd29..95380895f81 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>
> @@ -227,8 +228,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,
> @@ -708,4 +709,50 @@ 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 vCPU
> + * @vcpu_index: vcpu to query
> + *
> + * 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.
> + */
> +GArray * qemu_plugin_get_registers(unsigned int vcpu_index);
> +
> +/**
> + * qemu_plugin_read_register() - read register
> + *
> + * @vcpu: vcpu index
> + * @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.
> + *
> + * 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(unsigned int vcpu,
> +                              struct qemu_plugin_register *handle,
> +                              GByteArray *buf);
> +
> +
>   #endif /* QEMU_QEMU_PLUGIN_H */
> diff --git a/plugins/api.c b/plugins/api.c
> index ac39cdea0b3..f8905325c43 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"
> @@ -435,3 +438,102 @@ uint64_t qemu_plugin_entry_code(void)
>   #endif
>       return entry;
>   }
> +
> +/*
> + * Register handles
> + *
> + * The plugin infrastructure keeps hold of these internal data
> + * structures which are presented to plugins as opaque handles. They
> + * are global to the system and therefor additions to the hash table
> + * must be protected by the @reg_handle_lock.
> + *
> + * In order to future proof for up-coming heterogeneous work we want
> + * different entries for each CPU type while sharing them in the
> + * common case of multiple cores of the same type.
> + */
> +
> +static QemuMutex reg_handle_lock;
> +
> +struct qemu_plugin_register {
> +    const char *name;
> +    int gdb_reg_num;
> +};
> +
> +static GHashTable *reg_handles; /* hash table of PluginReg */
> +
> +/* Generate a stable key - would xxhash be overkill? */
> +static gpointer cpu_plus_reg_to_key(CPUState *cs, int gdb_regnum)
> +{
> +    uintptr_t key = (uintptr_t) cs->cc;
> +    key ^= gdb_regnum;
> +    return GUINT_TO_POINTER(key);
> +}
> +
> +/*
> + * Create register handles.
> + *
> + * We need to create a handle for each register so the plugin
> + * infrastructure can call gdbstub to read a register. 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));
> +
> +    WITH_QEMU_LOCK_GUARD(&reg_handle_lock) {
> +
> +        if (!reg_handles) {
> +            reg_handles = g_hash_table_new(g_direct_hash, g_direct_equal);
> +        }
> +
> +        for (int i=0; i < gdbstub_regs->len; i++) {
> +            GDBRegDesc *grd = &g_array_index(gdbstub_regs, GDBRegDesc, i);
> +            gpointer key = cpu_plus_reg_to_key(cs, grd->gdb_reg);
> +            struct qemu_plugin_register *val = g_hash_table_lookup(reg_handles, key);
> +
> +            /* Doesn't exist, create one */
> +            if (!val) {
> +                val = g_new0(struct qemu_plugin_register, 1);
> +                val->gdb_reg_num = grd->gdb_reg;
> +                val->name = g_intern_string(grd->name);
> +
> +                g_hash_table_insert(reg_handles, key, val);
> +            }
> +
> +            /* Create a record for the plugin */
> +            qemu_plugin_reg_descriptor desc = {
> +                .handle = val,
> +                .name = val->name,
> +                .feature = g_intern_string(grd->feature_name)
> +            };
> +            g_array_append_val(find_data, desc);
> +        }
> +    }
> +
> +    return find_data;
> +}
> +
> +GArray * qemu_plugin_get_registers(unsigned int vcpu)
> +{
> +    CPUState *cs = qemu_get_cpu(vcpu);
> +    if (cs) {
> +        g_autoptr(GArray) regs = gdb_get_register_list(cs);
> +        return regs->len ? create_register_handles(cs, regs) : NULL;
> +    } else {
> +        return NULL;
> +    }
> +}
> +
> +int qemu_plugin_read_register(unsigned int vcpu, struct qemu_plugin_register *reg, GByteArray *buf)
> +{
> +    CPUState *cs = qemu_get_cpu(vcpu);
> +    /* assert with debugging on? */
> +    return gdb_read_register(cs, buf, reg->gdb_reg_num);
> +}
> +
> +static void __attribute__((__constructor__)) qemu_api_init(void)
> +{
> +    qemu_mutex_init(&reg_handle_lock);
> +
> +}
> diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
> index 71f6c90549d..6963585c1ea 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;
> @@ -20,6 +21,7 @@
>     qemu_plugin_n_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;

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>