Re: [RFC PATCH 21/24] plugins: Allow to read registers

[Akihiko Odaki]

On 2023/08/15 0:05, Alex Bennée wrote:
> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>
> > It is based on GDB protocol to ensure interface stability.
>
> See comments bellow.
>
> > The timing of the vcpu init hook is also changed so that the hook will
> > get called after GDB features are initialized.
>
> This might be worth splitting to a separate patch for cleaner bisecting.
>
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >   include/qemu/qemu-plugin.h   | 65 ++++++++++++++++++++++++++++++++++--
> >   cpu.c                        | 11 ------
> >   hw/core/cpu-common.c         | 10 ++++++
> >   plugins/api.c                | 40 ++++++++++++++++++++++
> >   plugins/qemu-plugins.symbols |  2 ++
> >   5 files changed, 114 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> > index 50a9957279..214b12bfd6 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>
> > @@ -51,7 +52,7 @@ typedef uint64_t qemu_plugin_id_t;
> >   
> >   extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
> >   
> > -#define QEMU_PLUGIN_VERSION 1
> > +#define QEMU_PLUGIN_VERSION 2
> >   
> >   /**
> >    * struct qemu_info_t - system information for plugins
> > @@ -218,8 +219,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,
> > @@ -664,4 +665,62 @@ uint64_t qemu_plugin_end_code(void);
> >    */
> >   uint64_t qemu_plugin_entry_code(void);
> >   
> > +/**
> > + * struct qemu_plugin_register_file_t - register information
> > + *
> > + * This structure identifies registers. The identifiers included in this
> > + * structure are identical with names used in GDB's standard target features
> > + * with some extensions. For details, see:
> > + *
> > https://sourceware.org/gdb/onlinedocs/gdb/Standard-Target-Features.html
>
> I'm not super keen on baking GDB-isms into the plugin register
> interface.

I used GDB names here because I just didn't want to invent feature 
register names again. We can use any other register name dictionary if 
desired.

> > + *
> > + * A register is uniquely identified with the combination of a feature name
> > + * and a register name or a register number. It is recommended to derive
> > + * register numbers from feature names and register names each time a new vcpu
> > + * starts.
>
> Do you have examples of clashing register names from different feature
> sets?

No. A possible situation that the feature name matters is that a vendor 
extension becomes a standard extension with some modifications. In such 
a case, plugins written for the ratified standard extension may reject 
to work with the processor with the original vendor extension. While 
such a situation has not happened for QEMU and it's unlikely to happen, 
I'm advising to use feature name for register identification just for 
caution.

> > + *
> > + * To derive the register number from a feature name and a register name,
> > + * first look up qemu_plugin_register_file_t with the feature name, and then
> > + * look up the register name in its @regs. The sum of the @base_reg and the
> > + * index in the @reg is the register number.
> > + *
> > + * Note that @regs may have holes; some elements of @regs may be NULL.
> > + */
> > +typedef struct qemu_plugin_register_file_t {
> > +    /** @name: feature name */
> > +    const char *name;
> > +    /** @regs: register names */
> > +    const char * const *regs;
> > +    /** @base_reg: the base identified number */
> > +    int base_reg;
> > +    /** @num_regs: the number of elements in @regs */
> > +    int num_regs;
> > +} qemu_plugin_register_file_t;
> > +
> > +/**
> > + * qemu_plugin_get_register_files() - returns register information
> > + *
> > + * @vcpu_index: the index of the vcpu context
> > + * @size: the pointer to the variable to hold the number of returned elements
> > + *
> > + * Returns an array of qemu_plugin_register_file_t. The user should g_free()
> > + * the array once no longer needed.
> > + */
> > +qemu_plugin_register_file_t *
> > +qemu_plugin_get_register_files(unsigned int vcpu_index, int *size);
>
> I think I'd rather have a simpler interface that returns an anonymous
> handle to the plugin. For example:
>
>    struct qemu_plugin_register;
>    struct qemu_plugin_register qemu_plugin_find_register(const char *name);
>
> > +
> > +/**
> > + * qemu_plugin_read_register() - read register
> > + *
> > + * @buf: the byte array to append the read register content to.
> > + * @reg: the register identifier determined with
> > + *       qemu_plugin_get_register_files().
> > + *
> > + * 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.
> > + */
> > +int qemu_plugin_read_register(GByteArray *buf, int reg);
>
> and this then becomes:
>
>    int qemu_plugin_read_register(GByteArray *buf, struct qemu_plugin_register);
>
> in practice these can become anonymous pointers which hide the
> implementation details from the plugin itself. Then the details of
> mapping the register to a gdb regnum can be kept in the plugin code
> keeping us free to further re-factor the code as we go.
>
> The plugin code works quite hard to try and avoid leaking implementation
> details to plugins so as not to tie QEMU's hands in re-factoring. While
> the interface provided is technically GDB's, not QEMUs I don't think its
> a particularly nice one to expose.

Unfortunately "struct qemu_plugin_register" will not work. C requires 
the size of the struct to be known before a declaration of a function 
that uses the struct as a return value or parameter.

The best thing we can do is to add typedef just in a manner similar to 
qemu_plugin_id_t.

I added a function that returns all register information instead of a 
function that looks up a register so that a plugin can enumerate 
registers. Such capability is useful for a plugin that dumps all 
registers or a plugin that simulates processor (such a plugin may want 
to warn if there are unknown registers).

I'm just using GDB regnum here because I just needed some numbers. The 
numbers are actually arbitrary and I intentionally did not assure that 
the numbers are identical with what GDB use in the documentation 
comments. We may use any other arbitrary numbers as we want in the future.

> <snip>