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