[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 18/23] plugins: add an API to read registers
From: |
Alex Bennée |
Subject: |
Re: [PATCH 18/23] plugins: add an API to read registers |
Date: |
Fri, 23 Feb 2024 11:44:30 +0000 |
User-agent: |
mu4e 1.11.28; emacs 29.1 |
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
- Re: [PATCH 18/23] plugins: add an API to read registers, (continued)
- Re: [PATCH 18/23] plugins: add an API to read registers, Alex Bennée, 2024/02/20
- Re: [PATCH 18/23] plugins: add an API to read registers, Akihiko Odaki, 2024/02/20
- Re: [PATCH 18/23] plugins: add an API to read registers, Alex Bennée, 2024/02/21
- Re: [PATCH 18/23] plugins: add an API to read registers, Akihiko Odaki, 2024/02/21
- Re: [PATCH 18/23] plugins: add an API to read registers, Alex Bennée, 2024/02/21
- Re: [PATCH 18/23] plugins: add an API to read registers, Akihiko Odaki, 2024/02/22
- Re: [PATCH 18/23] plugins: add an API to read registers, Alex Bennée, 2024/02/22
- Re: [PATCH 18/23] plugins: add an API to read registers, Akihiko Odaki, 2024/02/22
- Re: [PATCH 18/23] plugins: add an API to read registers, Alex Bennée, 2024/02/22
- Re: [PATCH 18/23] plugins: add an API to read registers, Akihiko Odaki, 2024/02/23
- Re: [PATCH 18/23] plugins: add an API to read registers,
Alex Bennée <=
- Re: [PATCH 18/23] plugins: add an API to read registers, Alex Bennée, 2024/02/23
[PATCH 22/23] docs/devel: document some plugin assumptions, Alex Bennée, 2024/02/16
[PATCH 20/23] contrib/plugins: extend execlog to track register changes, Alex Bennée, 2024/02/16
[PATCH 17/23] gdbstub: expose api to find registers, Alex Bennée, 2024/02/16
[PATCH 21/23] docs/devel: lift example and plugin API sections up, Alex Bennée, 2024/02/16
[PATCH 15/23] cpu: call plugin init hook asynchronously, Alex Bennée, 2024/02/16
[PATCH 14/23] plugins: fix order of init/idle/resume callback, Alex Bennée, 2024/02/16
[PATCH 16/23] plugins: Use different helpers when reading registers, Alex Bennée, 2024/02/16
[PATCH 19/23] contrib/plugins: fix imatch, Alex Bennée, 2024/02/16