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

["Alex Bennée" <alex.bennee@linaro.org>]

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.

> + *
> + * 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? 

> + *
> + * 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.

<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro