Re: [PATCH v2 02/14] plugins: scoreboard API

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 1/26/24 19:14, Alex Bennée wrote:
>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>> 
>>> We introduce a cpu local storage, automatically managed (and extended)
>>> by QEMU itself. Plugin allocate a scoreboard, and don't have to deal
>>> with how many cpus are launched.
>>>
>>> This API will be used by new inline functions but callbacks can benefit
>>> from this as well. This way, they can operate without a global lock for
>>> simple operations.
>>>
>>> New functions:
>>> - qemu_plugin_scoreboard_free
>>> - qemu_plugin_scoreboard_get
>>> - qemu_plugin_scoreboard_new
>>> - qemu_plugin_scoreboard_size
>>>
>>> In more, we define a qemu_plugin_u64_t, which is a simple struct holding
>>> a pointer to a scoreboard, and a given offset.
>>> This allows to have a scoreboard containing structs, without having to
>>> bring offset for all operations on a specific field.
>>>
>>> Since most of the plugins are simply collecting a sum of per-cpu values,
>>> qemu_plugin_u64_t directly support this operation as well.
>>>
>>> New functions:
>>> - qemu_plugin_u64_get
>>> - qemu_plugin_u64_sum
>>> New macros:
>>> - qemu_plugin_u64
>>> - qemu_plugin_u64_struct
>>>
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>   include/qemu/plugin.h        |  7 +++
>>>   include/qemu/qemu-plugin.h   | 75 ++++++++++++++++++++++++++++
>>>   plugins/api.c                | 39 +++++++++++++++
>>>   plugins/core.c               | 97 ++++++++++++++++++++++++++++++++++++
>>>   plugins/plugin.h             |  8 +++
>>>   plugins/qemu-plugins.symbols |  6 +++
>>>   6 files changed, 232 insertions(+)
>>>
>>> diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
>>> index 9346249145d..5f340192e56 100644
>>> --- a/include/qemu/plugin.h
>>> +++ b/include/qemu/plugin.h
>>> @@ -115,6 +115,13 @@ struct qemu_plugin_insn {
>>>       bool mem_only;
>>>   };
>>>   +/* A scoreboard is an array of values, indexed by vcpu_index */
>>> +struct qemu_plugin_scoreboard {
>>> +    GArray *data;
>>> +    size_t size;
>> Can we get size from GArray->len itself?
>> 
>
> GArray->len matches the allocated size, which is different from
> "semantic" size. I'll answer to why it was implemented this way in a
> next comment.

Does it? The documentation states:

  guint len;

  the number of elements in the GArray not including the possible terminating 
zero element.

I think the actual allocated size is hidden from you by glib as an
implementation detail.

>>> +    size_t element_size;
>>> +};
>>> +
>>>   /*
>>>    * qemu_plugin_insn allocate and cleanup functions. We don't expect to
>>>    * cleanup many of these structures. They are reused for each fresh
>>> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
>>> index 2c1930e7e45..934059d64c2 100644
>>> --- a/include/qemu/qemu-plugin.h
>>> +++ b/include/qemu/qemu-plugin.h
>>> @@ -220,6 +220,23 @@ void 
>>> qemu_plugin_register_vcpu_resume_cb(qemu_plugin_id_t id,
>>>   struct qemu_plugin_tb;
>>>   /** struct qemu_plugin_insn - Opaque handle for a translated instruction 
>>> */
>>>   struct qemu_plugin_insn;
>>> +/**
>>> + * struct qemu_plugin_scoreboard - Opaque handle for a scoreboard
>>> + *
>>> + * A scoreboard is an array of data, indexed by vcpu_index.
>>> + **/
>> stray *'s - I think this is what trips up kdoc.
>> 
>
> Thanks, fixed that in a new version. I now build with documentation
> enabled, so should not happen again, sorry.
>
>>> +struct qemu_plugin_scoreboard;
>>> +
>>> +/**
>>> + * qemu_plugin_u64_t - uint64_t member of an entry in a scoreboard
>> We generally reserve lower_case_with_underscores_ending_with_a_t for
>> scalar types. Its also a little generic. Maybe qemu_plugin_scoreboard_ref?
>>
>
> For _t suffix, I hesitated on this, and followed the qemu_info_t
> struct example in the same header.

We are nothing if not inconsistent in the code base ;-) There are a
number of _t types that in retrospect shouldn't have been.

> In the beginning, I picked qemu_plugin_scoreboard_u64, but realized
> the name was far too long. qemu_plugin_u64 seemed like the right spot
> between length/meaning.
> I would like to defend the u64 meaning, because it was explicitely
> made to express u64 type, instead of a generic scoreboard entry/ref.
>
> In more, based on other comments made, maybe it was not clear that a
> qemu_plugin_u64 does not necessarily point to a "whole" entry in
> scoreboard, but it can target a struct member (u64) in a given entry
> as well. (i.e. scoreboard[i].member). I'll give a more detailed answer
> below in this message.

If we just return values rather than pointers we can stick with simpler
types.

> This way, scoreboard can be composed of struct values, and a
> qemu_plugin_u64 allows to work on a specific member of that struct
> without having to manipulate offset everywhere. Thus the get and sum
> operation on this type.

I would have to see an example of why giving this indirection to the
plugin makes things easier.

>
> Does it makes more sense to you given this?
>
>>> + *
>>> + * This field allows to access a specific uint64_t member in one given 
>>> entry,
>>> + * located at a specified offset. Inline operations expect this as entry.
>>> + */
>>> +typedef struct qemu_plugin_u64 {
>>  > I don't think you need the forward declaration here (which clashes later
>> on).
>>
>
> Initially wrote this without a typedef, and left this afterwards. Will
> remove it, thanks!
>
>>> +    struct qemu_plugin_scoreboard *score;
>>> +    size_t offset;
>>> +} qemu_plugin_u64_t;
>>>     /**
>>>    * enum qemu_plugin_cb_flags - type of callback
>>> @@ -754,5 +771,63 @@ int qemu_plugin_read_register(unsigned int vcpu,
>>>                                 struct qemu_plugin_register *handle,
>>>                                 GByteArray *buf);
>>>   +/**
>>> + * qemu_plugin_scoreboard_new() - alloc a new scoreboard
>>> + *
>>> + * Returns a pointer to a new scoreboard. It must be freed using
>>> + * qemu_plugin_scoreboard_free.
>>> + */
>>> +QEMU_PLUGIN_API
>>> +struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t 
>>> element_size);
>>> +
>>> +/**
>>> + * qemu_plugin_scoreboard_free() - free a scoreboard
>>> + * @score: scoreboard to free
>>> + */
>>> +QEMU_PLUGIN_API
>>> +void qemu_plugin_scoreboard_free(struct qemu_plugin_scoreboard *score);
>>> +
>>> +/**
>>> + * qemu_plugin_scoreboard_size() - return size of a scoreboard
>>> + * @score: scoreboard to query
>>> + */
>>> +QEMU_PLUGIN_API
>>> +size_t qemu_plugin_scoreboard_size(struct qemu_plugin_scoreboard
>>> *score);
>> Is this actually host memory size related? The use case in the
>> plugins
>> seems to be a proxy for num_cpus and I'm note sure we want it used that
>> way. We are making the contract that it will always be big enough for
>> the number of vcpus created - maybe that is the helper we need?
>> 
>
> The current API has a limitation in user mode, where you can't know
> how many cpus were run overall. The point of scoreboard was to
> abstract this away, by removing the need for plugins to track this
> (and use lock, manual memory allocation, and glue code in general).

Agreed.

> I thought it was more clear to write:
> for (int i = 0; i < qemu_plugin_scoreboard_size(s); ++i) {
>       void *val = qemu_plugin_scoreboard_get(s, i);
> }
> than:
> for (int i = 0; i < qemu_plugin_max_cpus(s); ++i) {
>       void *val = qemu_plugin_scoreboard_get(s, i);
> }
>
> We can always replace qemu_plugin_scoreboard_size by
> qemu_plugin_max_cpus (or another name you prefer), which would return
> the max number of cpus that were used during all the execution, if
> that works for you.

I think that works, do we know it even if we haven't allocated any
scoreboards?


>
>>> +
>>> +/**
>>> + * qemu_plugin_scoreboard_get() - access value from a scoreboard
>>> + * @score: scoreboard to query
>>> + * @vcpu_index: entry index
>>> + *
>>> + * Returns address of entry of a scoreboard matching a given vcpu_index. 
>>> This
>>> + * address can be modified later if scoreboard is resized.
>>> + */
>>> +QEMU_PLUGIN_API
>>> +void *qemu_plugin_scoreboard_get(struct qemu_plugin_scoreboard *score,
>>> +                                 unsigned int vcpu_index);
>>> +
>>> +/* Macros to define a qemu_plugin_u64_t */
>>> +#define qemu_plugin_u64(score) \
>>> +    (qemu_plugin_u64_t){score, 0}
>>> +#define qemu_plugin_u64_struct(score, type, member) \
>>> +    (qemu_plugin_u64_t){score, offsetof(type, member)}
>> qemu_plugin_val_ref and qemu_plugin_struct_ref?
>> 
>
> Same comment as before, it would be more clear to keep the u64 in the
> name if possible, so we know which data type we manipulate.
>
>>> +
>>> +/**
>>> + * qemu_plugin_u64_get() - access specific uint64_t in a scoreboard
>>> + * @entry: entry to query
>>> + * @vcpu_index: entry index
>>> + *
>>> + * Returns address of a specific member in a scoreboard entry, matching a 
>>> given
>>> + * vcpu_index.
>>> + */
>>> +QEMU_PLUGIN_API
>>> +uint64_t *qemu_plugin_u64_get(qemu_plugin_u64_t entry, unsigned int 
>>> vcpu_index);
>>> +
>>> +/**
>>> + * qemu_plugin_u64_sum() - return sum of all values in a scoreboard
>>> + * @entry: entry to sum
>>> + */
>>> +QEMU_PLUGIN_API
>>> +uint64_t qemu_plugin_u64_sum(qemu_plugin_u64_t entry);
>>>     #endif /* QEMU_QEMU_PLUGIN_H */
>>> diff --git a/plugins/api.c b/plugins/api.c
>>> index e777eb4e9fc..4de94e798c6 100644
>>> --- a/plugins/api.c
>>> +++ b/plugins/api.c
>>> @@ -547,3 +547,42 @@ static void __attribute__((__constructor__)) 
>>> qemu_api_init(void)
>>>       qemu_mutex_init(&reg_handle_lock);
>>>     }
>>> +
>>> +struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t 
>>> element_size)
>>> +{
>>> +    return plugin_scoreboard_new(element_size);
>>> +}
>>> +
>>> +void qemu_plugin_scoreboard_free(struct qemu_plugin_scoreboard *score)
>>> +{
>>> +    plugin_scoreboard_free(score);
>>> +}
>>> +
>>> +size_t qemu_plugin_scoreboard_size(struct qemu_plugin_scoreboard *score)
>>> +{
>>> +    return score->size;
>> So this would be score->data->len
>> 
>>> +}
>>> +
>>> +void *qemu_plugin_scoreboard_get(struct qemu_plugin_scoreboard *score,
>>> +                                 unsigned int vcpu_index)
>>> +{
>>> +    g_assert(vcpu_index < qemu_plugin_scoreboard_size(score));
>>> +    char *ptr = score->data->data;
>>> +    return ptr + vcpu_index * score->element_size;
>> GArray has accesses functions for this:
>>         return &g_array_index(score->data,
>>                               g_array_get_element_size(score->data),
>>                               vcpu_index);
>>
>
> Yes, I'll update this.
>
>>> +}
>>> +
>>> +uint64_t *qemu_plugin_u64_get(qemu_plugin_u64_t entry,
>>> +                                         unsigned int vcpu_index)
>>> +{
>>> +    char *ptr = (char *) qemu_plugin_scoreboard_get(entry.score, 
>>> vcpu_index);
>>> +    return (uint64_t *)(ptr + entry.offset);
>> Not sure whats going with the casting here but I wonder if we are
>> giving
>> the user too much rope. Why not to return the value itself? In fact why
>> do we treat things as an offset rather than an index of uint64_t.
>> The qemu_plugin_scoreboard_get can return uint64_t * and the
>> individual
>> get becomes:
>>    uint64_t *qemu_plugin_scoreboard_get(struct
>> qemu_plugin_scoreboard *score,
>>                                         unsigned int vcpu_index)
>>    {
>>        g_assert(vcpu_index < qemu_plugin_scoreboard_size(score));
>>        return &g_array_index(score->data, score->element_size, vcpu_index);
>>    }
>>    uint64_t qemu_plugin_u64_get(struct qemu_plugin_scoreboard
>> *score,
>>                                  unsigned int vcpu_index, unsigned int 
>> score_index)
>>    {
>>        g_assert(score_index < score->num_elements);
>>        uint64_t *sb = qemu_plugin_scoreboard_get(score, vcpu_index);
>>        return sb[score_index];
>>    }
>> 
>
> As said above, maybe it was not clear in the commit (though expressed
> in commit message) what was a distinction between a scoreboard, and a
> plugin_u64.
>
> The latter is an abstraction of a specific member in one scoreboard
> entry, while a scoreboard holds any type.
>
> Let me write a concrete example, which will be clearer:
>
> typedef struct {
>       char data1;
>       bool data2;
>       void* data3;
>       enum ...;
>       uint64_t counter;
> } CPUData;
>
> score = qemu_plugin_scoreboard_new(sizeof(CPUData));
> qemu_plugin_u64_t counter =
>       qemu_plugin_u64_struct(score, CPUData, counter));
>
> From there:
> qemu_plugin_u64_get(counter, 2) returns &score[2].counter.
> qemu_plugin_u64_sum(counter) returns
>       sum(i: [0:max_cpus[, score[i].counter).
>
> With this, a plugin can simply have an abstract representation of cpu
> local value, than can be get or set (since we return a pointer with
> get) and easily summed, which is what most of the plugins want to do
> in the end (at least from what I observed in existing ones).
>
> If we stick to a void* scoreboard, we end up writing the sum function
> in every plugin. In more, every API function "inline" operating on a
> scoreboard would need the offsetof(CPUData, counter) passed in
> parameter, which is error prone and repetitive.
>
> With this explaination, do you see more the value to have a scoreboard
> *and* a plugin_u64 struct to access it?
>
>>> +}
>>> +
>>> +uint64_t qemu_plugin_u64_sum(qemu_plugin_u64_t entry)
>>> +{
>>> +    uint64_t total = 0;
>>> +    for (int i = 0; i < qemu_plugin_scoreboard_size(entry.score); ++i) {
>>> +        total += *qemu_plugin_u64_get(entry, i);
>>> +    }
>>> +    return total;
>>> +}
>>> diff --git a/plugins/core.c b/plugins/core.c
>>> index e07b9cdf229..0286a127810 100644
>>> --- a/plugins/core.c
>>> +++ b/plugins/core.c
>>> @@ -209,6 +209,71 @@ plugin_register_cb_udata(qemu_plugin_id_t id, enum 
>>> qemu_plugin_event ev,
>>>       do_plugin_register_cb(id, ev, func, udata);
>>>   }
>>>   +struct resize_scoreboard_args {
>>> +    size_t new_alloc_size;
>>> +    size_t new_size;
>>> +};
>>> +
>>> +static void plugin_resize_one_scoreboard(gpointer key,
>>> +                                         gpointer value,
>>> +                                         gpointer user_data)
>>> +{
>>> +    struct qemu_plugin_scoreboard *score =
>>> +        (struct qemu_plugin_scoreboard *) value;
>>> +    struct resize_scoreboard_args *args =
>>> +        (struct resize_scoreboard_args *) user_data;
>>> +    if (score->data->len != args->new_alloc_size) {
>>> +        g_array_set_size(score->data, args->new_alloc_size);
>>> +    }
>>> +    score->size = args->new_size;
>> I think you have this in len so if you skip the extra field you can
>> just
>> do:
>>    new_size = GPOINTER_TO_UINT(user_data);
>> 
>
> If we can avoid managing allocated vs real size, yes.
>
>>> +}
>>> +
>>> +static void plugin_grow_scoreboards__locked(CPUState *cpu)
>>> +{
>>> +    if (cpu->cpu_index < plugin.scoreboard_size) {
>>> +        return;
>>> +    }
>>> +
>>> +    bool need_realloc = FALSE;
>>> +    while (cpu->cpu_index >= plugin.scoreboard_alloc_size) {
>>> +        plugin.scoreboard_alloc_size *= 2;
>> I suspect for USER this makes sense, if we every end up doing system
>> expansion then it would just be +1 at a time. In fact given glib does:
>>        gsize want_alloc = g_nearest_pow (g_array_elt_len (array,
>> want_len));
>> maybe we just do a simple increment and leave it up glib to handle
>> the
>> exponential growth?
>> 
>
> Initially, I wanted to go this way, but it seems like glib does not
> offer alloc_size information about a GArray. I looked for this in
> documentation and doc, and maybe missed it.
>
> If you have an idea to detect this, I would love to just use it, and
> it would made the code much more simple, avoiding the need to keep
> track of two sizes.
>
> The problem we try to solve is to detect when the pointer will be
> reallocated, with the consequence that all tb must be flushed and
> retranslated to use the new one instead.
>
>>> +        need_realloc = TRUE;
>>> +    }
>>> +    plugin.scoreboard_size = cpu->cpu_index + 1;
>>> +    g_assert(plugin.scoreboard_size <= plugin.scoreboard_alloc_size);
>>> +
>>> +    if (g_hash_table_size(plugin.scoreboards) == 0) {
>>> +        /* nothing to do, we just updated sizes for future scoreboards */
>>> +        return;
>>> +    }
>>> +
>>> +    if (need_realloc) {
>>> +#ifdef CONFIG_USER_ONLY
>>> +        /**
>>> +         * cpus must be stopped, as some tb might still use an existing
>>> +         * scoreboard.
>>> +         */
>>> +        start_exclusive();
>>> +#endif
>> Hmm this seems wrong to be USER_ONLY. While we don't expect to
>> resize in
>> system mode if we did we certainly want to do it during exclusive
>> periods.
>> 
>
> I agree. Initially did this for both (system and user), but in system,
> we hit a segfault.
>
> $ ./build/qemu-system-x86_64 -plugin ./build/tests/plugin/libinline.so
> -smp 17 # > 16 will trigger a reallocation of scoreboard
> ../cpu-common.c:196:20: runtime error: member access within null
> pointer of type 'struct CPUState'
>
> It seems like something is not set yet at this time (current_cpu TLS
> var), which result in this problem. Maybe this code simply reveals an
> existing problem, what do you think?
>
>>> +    }
>>> +
>>> +    struct resize_scoreboard_args args = {
>>> +        .new_alloc_size = plugin.scoreboard_alloc_size,
>>> +        .new_size = plugin.scoreboard_size
>>> +    };
>>> +    g_hash_table_foreach(plugin.scoreboards,
>>> +                         &plugin_resize_one_scoreboard,
>>> +                         &args);
>> This can just be g_hash_table_foreach(plugin.scoreboards,
>>                                        &plugin_resize_one_scoreboard,
>>                                        
>> GUINT_TO_POINTER(plugin.scoreboard_alloc_size))
>> 
>
> If we use a single size, yes.
>
>>> +
>>> +    if (need_realloc) {
>>> +        /* force all tb to be flushed, as scoreboard pointers were 
>>> changed. */
>>> +        tb_flush(cpu);
>>> +#ifdef CONFIG_USER_ONLY
>>> +        end_exclusive();
>>> +#endif
>>> +    }
>>> +}
>>> +
>>>   void qemu_plugin_vcpu_init_hook(CPUState *cpu)
>>>   {
>>>       bool success;
>>> @@ -218,6 +283,7 @@ void qemu_plugin_vcpu_init_hook(CPUState *cpu)
>>>       success = g_hash_table_insert(plugin.cpu_ht, &cpu->cpu_index,
>>>                                     &cpu->cpu_index);
>>>       g_assert(success);
>>> +    plugin_grow_scoreboards__locked(cpu);
>>>       qemu_rec_mutex_unlock(&plugin.lock);
>>>         plugin_vcpu_cb__simple(cpu, QEMU_PLUGIN_EV_VCPU_INIT);
>>> @@ -576,8 +642,39 @@ static void __attribute__((__constructor__)) 
>>> plugin_init(void)
>>>       qemu_rec_mutex_init(&plugin.lock);
>>>       plugin.id_ht = g_hash_table_new(g_int64_hash, g_int64_equal);
>>>       plugin.cpu_ht = g_hash_table_new(g_int_hash, g_int_equal);
>>> +    plugin.scoreboards = g_hash_table_new(g_int64_hash, g_int64_equal);
>>> +    plugin.scoreboard_size = 1;
>>> +    plugin.scoreboard_alloc_size = 16; /* avoid frequent reallocation */
>>>       QTAILQ_INIT(&plugin.ctxs);
>>>       qht_init(&plugin.dyn_cb_arr_ht, plugin_dyn_cb_arr_cmp, 16,
>>>                QHT_MODE_AUTO_RESIZE);
>>>       atexit(qemu_plugin_atexit_cb);
>>>   }
>>> +
>>> +struct qemu_plugin_scoreboard *plugin_scoreboard_new(size_t element_size)
>>> +{
>>> +    struct qemu_plugin_scoreboard *score =
>>> +        g_malloc0(sizeof(struct qemu_plugin_scoreboard));
>>> +    score->data = g_array_new(FALSE, TRUE, element_size);
>>> +    g_array_set_size(score->data, plugin.scoreboard_alloc_size);
>>> +    score->size = plugin.scoreboard_size;
>>> +    score->element_size = element_size;
>>> +
>>> +    qemu_rec_mutex_lock(&plugin.lock);
>>> +    bool inserted = g_hash_table_insert(plugin.scoreboards, score, score);
>>> +    g_assert(inserted);
>>> +    qemu_rec_mutex_unlock(&plugin.lock);
>>> +
>>> +    return score;
>>> +}
>>> +
>>> +void plugin_scoreboard_free(struct qemu_plugin_scoreboard *score)
>>> +{
>>> +    qemu_rec_mutex_lock(&plugin.lock);
>>> +    bool removed = g_hash_table_remove(plugin.scoreboards, score);
>>> +    g_assert(removed);
>>> +    qemu_rec_mutex_unlock(&plugin.lock);
>>> +
>>> +    g_array_free(score->data, TRUE);
>>> +    g_free(score);
>>> +}
>>> diff --git a/plugins/plugin.h b/plugins/plugin.h
>>> index 2c278379b70..99829c40886 100644
>>> --- a/plugins/plugin.h
>>> +++ b/plugins/plugin.h
>>> @@ -31,6 +31,10 @@ struct qemu_plugin_state {
>>>        * but with the HT we avoid adding a field to CPUState.
>>>        */
>>>       GHashTable *cpu_ht;
>>> +    /* Scoreboards, indexed by their addresses. */
>>> +    GHashTable *scoreboards;
>>> +    size_t scoreboard_size;
>>> +    size_t scoreboard_alloc_size;
>>>       DECLARE_BITMAP(mask, QEMU_PLUGIN_EV_MAX);
>>>       /*
>>>        * @lock protects the struct as well as ctx->uninstalling.
>>> @@ -99,4 +103,8 @@ void plugin_register_vcpu_mem_cb(GArray **arr,
>>>     void exec_inline_op(struct qemu_plugin_dyn_cb *cb, int
>>> cpu_index);
>>>   +struct qemu_plugin_scoreboard *plugin_scoreboard_new(size_t
>>> element_size);
>>> +
>>> +void plugin_scoreboard_free(struct qemu_plugin_scoreboard *score);
>>> +
>>>   #endif /* PLUGIN_H */
>>> diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
>>> index 6963585c1ea..93866d1b5f2 100644
>>> --- a/plugins/qemu-plugins.symbols
>>> +++ b/plugins/qemu-plugins.symbols
>>> @@ -38,10 +38,16 @@
>>>     qemu_plugin_register_vcpu_tb_exec_inline;
>>>     qemu_plugin_register_vcpu_tb_trans_cb;
>>>     qemu_plugin_reset;
>>> +  qemu_plugin_scoreboard_free;
>>> +  qemu_plugin_scoreboard_get;
>>> +  qemu_plugin_scoreboard_new;
>>> +  qemu_plugin_scoreboard_size;
>>>     qemu_plugin_start_code;
>>>     qemu_plugin_tb_get_insn;
>>>     qemu_plugin_tb_n_insns;
>>>     qemu_plugin_tb_vaddr;
>>> +  qemu_plugin_u64_get;
>>> +  qemu_plugin_u64_sum;
>>>     qemu_plugin_uninstall;
>>>     qemu_plugin_vcpu_for_each;
>>>   };
>> 
>
> Thanks for your complete and detailed review!

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro