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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2024/01/18 20:38, Alex Bennée wrote:
> 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?
reg_handles, a GHashTable, is used to detect the duplicate definitions 
of a register in processors with same type, but such a logic is probably 
better off to be implemented in gdbstub; gdbstub should have one 
definition of register set for each type of processors in a system, and 
gdbstub can use it to start a distinct GDB server for the type. This 
way, gdbstub can ensure it exposes a homogeneous view for each GDB 
server as GDB requires.

If gdbstub already has such a logic, the plugin infrastructure can just 
query gdbstub for a common, shared register set used for a processor 
type; it will no longer have to check for duplicate definitions with a 
hash table.