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

[Alex Bennée]

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);
  }



-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro