Akihiko Odaki <akihiko.od...@daynix.com> writes:
> On 2024/02/17 1:30, 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.od...@daynix.com>
>> Message-Id: <20240103173349.398526-39-alex.ben...@linaro.org>
>> Based-on: <20231025093128.33116-18-akihiko.od...@daynix.com>
>> Signed-off-by: Alex Bennée <alex.ben...@linaro.org>
>> Reviewed-by: Pierrick Bouvier <pierrick.bouv...@linaro.org>
<snip>
>> +/*
>> + * 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.
>> + *
>> + * 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.
>> + */
>> +
>> +static QemuMutex reg_handle_lock;
>> +
>> +struct qemu_plugin_register {
>> + const char *name;
>> + int gdb_reg_num;
>> +};
>> +
>> +static GHashTable *reg_handles; /* hash table of PluginReg */
>> +
>> +/* Generate a stable key - would xxhash be overkill? */
>> +static gpointer cpu_plus_reg_to_key(CPUState *cs, int gdb_regnum)
>> +{
>> + uintptr_t key = (uintptr_t) cs->cc;
>> + key ^= gdb_regnum;
>> + return GUINT_TO_POINTER(key);
>> +}
>
> I have pointed out this is theoretically prone to collisions and
> unsafe.
How is it unsafe? The aim is to share handles for the same CPUClass
rather than having a unique handle per register/cpu combo.
Indeed if I add the following:
--8<---------------cut here---------------start------------->8---
plugins/api.c
@@ -482,6 +482,9 @@ static GArray *create_register_handles(CPUState *cs, GArray
*gdbstub_regs)
val->name = g_intern_string(grd->name);
g_hash_table_insert(reg_handles, key, val);
+ } else {
+ /* make sure its not a clash */
+ g_assert(val->gdb_reg_num == grd->gdb_reg);
}
/* Create a record for the plugin */
modified tests/plugin/insn.c
@@ -46,6 +46,25 @@ typedef struct {
char *disas;
} Instruction;
+/*
+ * Initialise a new vcpu with reading the register list
+ */
+static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
+{
+ g_autoptr(GArray) reg_list = qemu_plugin_get_registers();
+ g_autoptr(GByteArray) reg_value = g_byte_array_new();
+
+ if (reg_list) {
+ for (int i = 0; i < reg_list->len; i++) {
+ qemu_plugin_reg_descriptor *rd = &g_array_index(
+ reg_list, qemu_plugin_reg_descriptor, i);
+ int count = qemu_plugin_read_register(rd->handle, reg_value);
+ g_assert(count > 0);
+ }
+ }
+}
+
+
static void vcpu_insn_exec_before(unsigned int cpu_index, void *udata)
{
unsigned int i = cpu_index % MAX_CPUS;
@@ -212,6 +231,8 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t
id,
sizes = g_array_new(true, true, sizeof(unsigned long));
}
+ /* Register init, translation block and exit callbacks */
+ qemu_plugin_register_vcpu_init_cb(id, vcpu_init);
qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
return 0;
--8<---------------cut here---------------end--------------->8---
Nothing trips up during check-tcg (after I fixed "gdbstub: Infer number
of core registers from XML" to remove the microblaze check on
cc->gdb_num_core_regs).
>
>> +
>> +/*
>> + * Create register handles.
>> + *
>> + * We need to create a handle for each register so the plugin
>> + * infrastructure can call gdbstub to read a register. We also
>> + * construct a result array with those handles and some ancillary data
>> + * the plugin might find useful.
>> + */
>> +
>> +static GArray *create_register_handles(CPUState *cs, GArray *gdbstub_regs)
>> +{
>> + GArray *find_data = g_array_new(true, true,
>> + sizeof(qemu_plugin_reg_descriptor));
>> +
>> + WITH_QEMU_LOCK_GUARD(®_handle_lock) {
>> +
>> + if (!reg_handles) {
>> + reg_handles = g_hash_table_new(g_direct_hash, g_direct_equal);
>> + }
>> +
>> + for (int i = 0; i < gdbstub_regs->len; i++) {
>> + GDBRegDesc *grd = &g_array_index(gdbstub_regs, GDBRegDesc, i);
>> + gpointer key = cpu_plus_reg_to_key(cs, grd->gdb_reg);
>> + struct qemu_plugin_register *val =
>> g_hash_table_lookup(reg_handles,
>> + key);
>> +
>> + /* skip "un-named" regs */
>> + if (!grd->name) {
>> + continue;
>> + }
>> +
>> + /* Doesn't exist, create one */
>> + if (!val) {
>> + val = g_new0(struct qemu_plugin_register, 1);
>> + val->gdb_reg_num = grd->gdb_reg;
>> + val->name = g_intern_string(grd->name);
>> +
>> + g_hash_table_insert(reg_handles, key, val);
>> + }
>> +
>> + /* Create a record for the plugin */
>> + qemu_plugin_reg_descriptor desc = {
>> + .handle = val,
>> + .name = val->name,
>> + .feature = g_intern_string(grd->feature_name)
>> + };
>> + g_array_append_val(find_data, desc);
>> + }
>> + }
>> +
>> + return find_data;
>> +}
>> +
>> +GArray *qemu_plugin_get_registers(void)
>> +{
>> + g_assert(current_cpu);
>> +
>> + g_autoptr(GArray) regs = gdb_get_register_list(current_cpu);
>> + return regs->len ? create_register_handles(current_cpu, regs) : NULL;
>> +}
>> +
>> +int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray
>> *buf)
>> +{
>> + g_assert(current_cpu);
>> +
>> + return gdb_read_register(current_cpu, buf, reg->gdb_reg_num);
>> +}
>> +
>> +static void __attribute__((__constructor__)) qemu_api_init(void)
>> +{
>> + qemu_mutex_init(®_handle_lock);
>> +
>> +}
>> diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
>> index adb67608598..27fe97239be 100644
>> --- a/plugins/qemu-plugins.symbols
>> +++ b/plugins/qemu-plugins.symbols
>> @@ -3,6 +3,7 @@
>> qemu_plugin_end_code;
>> qemu_plugin_entry_code;
>> qemu_plugin_get_hwaddr;
>> + qemu_plugin_get_registers;
>> qemu_plugin_hwaddr_device_name;
>> qemu_plugin_hwaddr_is_io;
>> qemu_plugin_hwaddr_phys_addr;
>> @@ -19,6 +20,7 @@
>> qemu_plugin_num_vcpus;
>> qemu_plugin_outs;
>> qemu_plugin_path_to_binary;
>> + qemu_plugin_read_register;
>> qemu_plugin_register_atexit_cb;
>> qemu_plugin_register_flush_cb;
>> qemu_plugin_register_vcpu_exit_cb;
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
