Re: [PULL 17/22] plugins: add an API to read registers

[Alex Bennée]

Akihiko Odaki <akihiko.odaki@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.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>
>> 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