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

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

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>
>