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

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

On 2024/02/21 19:02, Alex Bennée wrote:
> 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.

I suggest using a struct that holds both of cs->cc and gdb_regnum, and 
pass g_direct_equal() and g_direct_hash() to g_hash_table_new().