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

[Alex Bennée]

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

> On 2024/02/23 2:27, Alex Bennée wrote:
>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>> 
>>> On 2024/02/22 19:20, Alex Bennée wrote:
>>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>>>
>>>>> On 2024/02/21 23:14, Alex Bennée wrote:
>>>>>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>>>>>>
>>>>>>> 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().
>>>>>> We already do:
>>>>>>            if (!reg_handles) {
>>>>>>                reg_handles = g_hash_table_new(g_direct_hash, 
>>>>>> g_direct_equal);
>>>>>>            }
>>>>>> But we can't use g_direct_equal with something that exceeds the
>>>>>> width of
>>>>>> gpointer as it is a straight equality test of the key. What you are
>>>>>> suggesting requires allocating memory for each key and de-referencing
>>>>>> with a custom GEqualFunc.
>>>>>
>>>>> My bad. I wrongly remembered g_direct_equal() and g_direct_hash(). It
>>>>> indeed seems to need a more complicated solution.
>>>>>
>>>>> It is possible to write a GEqualFunc and a GHashFunc that consumes a
>>>>> struct but it is a chore. How about having a two-level GHashTable?
>>>>> reg_handles will be a GHashTable keyed with cs->cc, and another
>>>>> GHashTable will be keyed with gdb_regnum.
>>>> That still seems overkill for a clash that can't happen. What do you
>>>> think about the following:
>>>>     /*
>>>>      * Generate a stable key shared across CPUs of the same class
>>>>      *
>>>>      * 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. This makes the
>>>>      * assumption you won't see two CPUClass pointers that are similar
>>>>      * enough that the low bits mixed with different registers numbers
>>>>      * will give you the same key.
>>>>      *
>>>>      * The build time assert will fire if CPUClass goes on a sudden diet
>>>>      * and we assert further down if we detect two keys representing
>>>>      * different regnums. In practice allocations of CPUClass are much
>>>>      * farther apart making clashes practically impossible.
>>>>      */
>>>>     static gpointer cpu_plus_reg_to_key(CPUState *cs, int gdb_regnum)
>>>>     {
>>>>         uintptr_t key = (uintptr_t) cs->cc;
>>>>         /* this protects some of the assumptions above */
>>>>         qemu_build_assert(sizeof(*cs->cc) >= 256);
>>>>         key ^= gdb_regnum;
>>>>         return GUINT_TO_POINTER(key);
>>>>     }
>>>
>>>
>>> I think the assertion and comments are overkill. Doesn't having a
>>> nested GHashTable save some words you have to wrote for the
>>> assumption?
>> A nested hash table for a single entry is overkill.
>
> You mean that the first level will be indexed by only one CPUClass (or
> few if we support a heterogeneous system).
>
> I think it's still OK though. It's not like we will need more code
> when having few entries.
>
>> 
>>> I'm also not quite convinced that the comments and assertions are
>>> enough to say this hack is safe; what if some sort of pointer
>>> authentication is added and shuffles bits? Will this hack be
>>> compatible with static and dynamic checkers we may have in the future?
>> We are not using the value as a pointer so that should be irrelevant
>> although generally those bits tend to be at the top of pointers so they
>> can be masked off.
>> I'm not sure what we are trying to achieve here. I've got something
>> that
>> works, doesn't fail any tests and has some guards in for potential
>> future problems. At the same time I'm not prepared to over-engineer the
>> solution for a theoretical future problem we haven't got yet.
>> What about if I just key based of gdb_regnum and we accept that that
>> might break the one heterogeneous system we model today?
>> 
>
> That's the best option in my opinion. gdbstub won't work well with
> such a system anyway, and fixing it will need something similar to
> GHashTable. But if I would fix gdbstub for a heterogeneous system, I
> would add a field to CPUClass instead of having a GHashTable keyed
> with tuples of CPUClass pointers and register numbers. It should be
> fine considering that CPUState already has gdbstub-specific fields
> like gdb_regs.

It would be nice to move all register code into CPUClass to avoid
repeating ourselves but I suspect that is quite an invasive change for a
later series. Currently all the CPUClass values are set on init and
shouldn't really be changed after that otherwise we'll have to start
messing with locking. 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro