Akihiko Odaki <akihiko.od...@daynix.com> writes:
> On 2024/01/16 19:48, 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.od...@daynix.com>
>> Message-Id: <20240103173349.398526-39-alex.ben...@linaro.org>
>> Based-on: <20231025093128.33116-18-akihiko.od...@daynix.com>
>> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
>> Reviewed-by: Pierrick Bouvier <pierrick.bouv...@linaro.org>
>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>> index 4daab6efd29..2c1930e7e45 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..8d5cca53295 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,111 @@ 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.
>
> The BQL should be used instead. This lock only serializes the plugin
> access, but the whole gdbstub code needs to be serialized to ensure
> the correct behaving of e.g., gdb_get_register_list().
Why does gdb_get_register_list need to take the BQL? It only works
through per-cpu structures. The reg_handle_lock only protects the hash
table itself.
>
>> + *
>> + * 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.
>
> I don't think such an effort should be done in the plugin code, but it
> should be done in the common gdbstub code.
Sure - we can always move it later.
> GDB assumes all threads have the same set of registers, so gdbstub
> will need to take care of them by running distinct GDB servers for
> each processor type, for example. There is a good chance that gdbstub
> will duplicate similar logic.
Which logic?
>
>> + */
>> +
>> +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);
>> +}
>
> This is, theoretically, prone to collisions and unsafe.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
