Re: [PATCH 18/23] plugins: add an API to read registers

[Alex Bennée]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2024/02/20 23:14, Alex Bennée wrote:
>> Akihiko Odaki <akihiko.odaki@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.odaki@daynix.com>
>>>> Message-Id: <20240103173349.398526-39-alex.bennee@linaro.org>
>>>> Based-on: <20231025093128.33116-18-akihiko.odaki@daynix.com>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@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.
>
> THe intention is legitimate, but the implementation is not safe. It
> assumes (uintptr)cs->cc ^ gdb_regnum is unique, but there is no such
> guarantee. The key of GHashTable must be unique; generating hashes of
> keys should be done with hash_func given to g_hash_table_new().

This isn't a hash its a non-unique key. It is however unique for
the same register on the same class of CPU so for each vCPU in a system
can share the same opaque handles.

The hashing is done internally by glib. We would assert if there was a
duplicate key referring to a different register.

I'm unsure what you want here? Do you have a suggestion for the key
generation algorithm? As the comment notes I did consider a more complex
mixing algorithm using xxhash but that wouldn't guarantee no clash
either.


>
>> 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).
<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro