contrib/plugins: Add a plugin to generate basic block vectors

SimPoint is a widely used tool to find the ideal microarchitecture
simulation points so Valgrind[2] and Pin[3] support generating basic
block vectors for use with them. Let's add a corresponding plugin to
QEMU too.

Note that this plugin has a different goal with tests/plugin/bb.c.

This plugin creates a vector for each constant interval instead of
counting the execution of basic blocks for the entire run and able to
describe the change of execution behavior. Its output is also
syntactically simple and better suited for parsing, while the output of
tests/plugin/bb.c is more human-readable.

[1] https://cseweb.ucsd.edu/~calder/simpoint/
[2] https://valgrind.org/docs/manual/bbv-manual.html
[3] https://www.intel.com/content/www/us/en/developer/articles/tool/pin-a-dynamic-binary-instrumentation-tool.html

Signed-off-by: Yotaro Nada <yotaro.nada@gmail.com>
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 docs/devel/tcg-plugins.rst |  20 ++++++
 contrib/plugins/bb.c       | 153 +++++++++++++++++++++++++++++++++++++++++++++
 contrib/plugins/Makefile   |   1 +
 3 files changed, 174 insertions(+)


---
base-commit: 74abb45dac6979e7ff76172b7f0a24e869405184
change-id: 20240618-bb-93387ddf765b

Best regards,

Hi Akihiko, and thanks for contributing this new plugin.

Recently, plugins documentation has been modified, and list of plugins 
and their doc is now in "docs/about/emulation.rst". You may want to 
rebase on top of master.

Globally, I'm ok with this plugin and the implementation. Just a few 
fixes are needed for concurrent accesses.

On 8/12/24 23:46, Akihiko Odaki wrote:
> SimPoint is a widely used tool to find the ideal microarchitecture
> simulation points so Valgrind[2] and Pin[3] support generating basic
> block vectors for use with them. Let's add a corresponding plugin to
> QEMU too.
> 
> Note that this plugin has a different goal with tests/plugin/bb.c.
> 
> This plugin creates a vector for each constant interval instead of
> counting the execution of basic blocks for the entire run and able to
> describe the change of execution behavior. Its output is also
> syntactically simple and better suited for parsing, while the output of
> tests/plugin/bb.c is more human-readable.
> 

I think it can be confusing to have two plugins named bb. How about 
simpoint, or bbv?

> [1] https://cseweb.ucsd.edu/~calder/simpoint/
> [2] https://valgrind.org/docs/manual/bbv-manual.html
> [3] https://www.intel.com/content/www/us/en/developer/articles/tool/pin-a-dynamic-binary-instrumentation-tool.html
> 
> Signed-off-by: Yotaro Nada <yotaro.nada@gmail.com>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   docs/devel/tcg-plugins.rst |  20 ++++++
>   contrib/plugins/bb.c       | 153 +++++++++++++++++++++++++++++++++++++++++++++
>   contrib/plugins/Makefile   |   1 +
>   3 files changed, 174 insertions(+)
> 
> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
> index 9cc09d8c3da1..2859eecc13b9 100644
> --- a/docs/devel/tcg-plugins.rst
> +++ b/docs/devel/tcg-plugins.rst
> @@ -332,6 +332,26 @@ run::
>     160          1      0
>     135          1      0
>   
> +- contrib/plugins/bb.c
> +
> +The bb plugin allows you to generates basic block vectors for use with the
> +`SimPoint <https://cseweb.ucsd.edu/~calder/simpoint/>`__ analysis tool.
> +
> +It has two options, ``interval`` and ``outfile``. ``interval`` specifies the
> +interval to generate a basic block vector by the number of instructions. It is
> +optional, and its default value is 100000000. ``outfile`` is the path to
> +output files, and it will be suffixed with ``.N.bb`` where ``N`` is a vCPU
> +index.
> +
> +Example::
> +
> +  $ qemu-aarch64 \
> +    -plugin contrib/plugins/libb.so,interval=100,outfile=sha1 \
> +    tests/tcg/aarch64-linux-user/sha1
> +  SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6
> +  $ du sha1.0.bb
> +  23128   sha1.0.bb
> +
>   - contrib/plugins/hotblocks.c
>   
>   The hotblocks plugin allows you to examine the where hot paths of
> diff --git a/contrib/plugins/bb.c b/contrib/plugins/bb.c
> new file mode 100644
> index 000000000000..4f1266d07ff5
> --- /dev/null
> +++ b/contrib/plugins/bb.c
> @@ -0,0 +1,153 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +

A brief summary and the link to simpoint page can be added as a comment 
here.

> +#include <stdio.h>
> +#include <glib.h>
> +
> +#include <qemu-plugin.h>
> +
> +typedef struct Bb {
> +    struct qemu_plugin_scoreboard *count;
> +    unsigned int index;
> +} Bb;
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
> +static GHashTable *bbs;
> +static GPtrArray *files;
> +static char *filename;
> +static struct qemu_plugin_scoreboard *count;
> +static uint64_t interval = 100000000;
> +
> +static void plugin_exit(qemu_plugin_id_t id, void *p)
> +{
> +    g_hash_table_unref(bbs);
> +    g_ptr_array_unref(files);
> +    g_free(filename);
> +    qemu_plugin_scoreboard_free(count);
> +}
> +
> +static void free_bb(void *data)
> +{
> +    qemu_plugin_scoreboard_free(((Bb *)data)->count);
> +    g_free(data);
> +}
> +
> +static void free_file(void *data)
> +{
> +    fclose(data);
> +}
> + > +static directly count_u64(void)
> +{
> +    return qemu_plugin_scoreboard_u64(count);
> +}
> +
> +static qemu_plugin_u64 bb_count_u64(Bb *bb)
> +{
> +    return qemu_plugin_scoreboard_u64(bb->count);
> +}
> +
> +static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
> +{
> +    g_autofree gchar *vcpu_filename = NULL;
> +
> +    if (vcpu_index >= files->len) {
> +        g_ptr_array_set_size(files, vcpu_index + 1);
> +    } else if (g_ptr_array_index(files, vcpu_index)) {
> +        return;
> +    }
> +

You need a lock for files array for expansion/access.

> +    vcpu_filename = g_strdup_printf("%s.%u.bb", filename, vcpu_index);
> +    g_ptr_array_index(files, vcpu_index) = fopen(vcpu_filename, "w");
> +}
> +
> +static void vcpu_tb_exec(unsigned int vcpu_index, void *udata)

vcpu_tb_exec is a confusing name, as it is called when interval of insn 
is executed. vcpu_interval_exec would be more clear.

> +{
> +    FILE *file = g_ptr_array_index(files, vcpu_index);

Need lock when accessing this array.

> +    uint64_t count = qemu_plugin_u64_get(count_u64(), vcpu_index) - interval;
> +    GHashTableIter iter;
> +    void *value;
> +
> +    if (!file) {
> +        return;
> +    }
> +
> +    qemu_plugin_u64_set(count_u64(), vcpu_index, count);
> + > +    fputc('T', file);

Maybe you can add a comment with a link to 
https://valgrind.org/docs/manual/bbv-manual.html at this point. It's 
already mentioned in the commit message, but IMHO, it's convenient to 
have it here, so we know which data is written exactly.

> +
> +    g_hash_table_iter_init(&iter, bbs);
> +
The access to this hashtable should be under a dedicated lock.

> +    while (g_hash_table_iter_next(&iter, NULL, &value)) {
> +        Bb *bb = value;
> +        uint64_t bb_count = qemu_plugin_u64_get(bb_count_u64(bb), vcpu_index);
> +
> +        if (!bb_count) {
> +            continue;
> +        }
> +
> +        fprintf(file, ":%u:%" PRIu64 " ", bb->index, bb_count);
> +        qemu_plugin_u64_set(bb_count_u64(bb), vcpu_index, 0);
> +    }
> +
> +    fputc('\n', file);
> +}
> +
> +static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
> +{
> +    uint64_t n_insns = qemu_plugin_tb_n_insns(tb);
> +    uint64_t vaddr = qemu_plugin_tb_vaddr(tb);

Need lock for hashtable access.

> +    Bb *bb = g_hash_table_lookup(bbs, &vaddr);
> +
> +    if (!bb) {
> +        uint64_t *key = g_new(uint64_t, 1);
> +
> +        *key = vaddr;
> +        bb = g_new(Bb, 1);
> +        g_hash_table_insert(bbs, key, bb);
> +        bb->count = qemu_plugin_scoreboard_new(sizeof(uint64_t));
> +        bb->index = g_hash_table_size(bbs);
> +    }
> +
> +    qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
> +        tb, QEMU_PLUGIN_INLINE_ADD_U64, count_u64(), n_insns);
> +
> +    qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu(
> +        tb, QEMU_PLUGIN_INLINE_ADD_U64, bb_count_u64(bb), n_insns);
> +
> +    qemu_plugin_register_vcpu_tb_exec_cond_cb(
> +        tb, vcpu_tb_exec, QEMU_PLUGIN_CB_NO_REGS,
> +        QEMU_PLUGIN_COND_GE, count_u64(), interval, NULL);
> +}
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
> +                                           const qemu_info_t *info,
> +                                           int argc, char **argv)
> +{
> +    for (int i = 0; i < argc; i++) {
> +        char *opt = argv[i];
> +        g_auto(GStrv) tokens = g_strsplit(opt, "=", 2);
> +        if (g_strcmp0(tokens[0], "interval") == 0) {
> +            interval = g_ascii_strtoull(tokens[1], NULL, 10);
> +        } else if (g_strcmp0(tokens[0], "outfile") == 0) {
> +            filename = tokens[1];
> +            tokens[1] = NULL;
> +        } else {
> +            fprintf(stderr, "option parsing failed: %s\n", opt);
> +            return -1;
> +        }
> +    }
> +
> +    if (!filename) {
> +        fputs("outfile unspecified\n", stderr);
> +        return -1;
> +    }
> +
> +    bbs = g_hash_table_new_full(g_int64_hash, g_int64_equal, g_free, free_bb);
> +    files = g_ptr_array_new_with_free_func(free_file);
> +    count = qemu_plugin_scoreboard_new(sizeof(uint64_t));
> +    qemu_plugin_register_atexit_cb(id, plugin_exit, NULL);
> +    qemu_plugin_register_vcpu_init_cb(id, vcpu_init);
> +    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
> +
> +    return 0;
> +}
> diff --git a/contrib/plugins/Makefile b/contrib/plugins/Makefile
> index 0b64d2c1e3a9..14bc88bb4f86 100644
> --- a/contrib/plugins/Makefile
> +++ b/contrib/plugins/Makefile
> @@ -13,6 +13,7 @@ TOP_SRC_PATH = $(SRC_PATH)/../..
>   VPATH += $(SRC_PATH)
>   
>   NAMES :=
> +NAMES += bb
>   NAMES += execlog
>   NAMES += hotblocks
>   NAMES += hotpages
> 
> ---
> base-commit: 74abb45dac6979e7ff76172b7f0a24e869405184
> change-id: 20240618-bb-93387ddf765b
> 
> Best regards,

Regards,
Pierrick

On 2024/08/14 4:20, Pierrick Bouvier wrote:
> Hi Akihiko, and thanks for contributing this new plugin.

Hi,

Thanks for reviewing

> 
> Recently, plugins documentation has been modified, and list of plugins 
> and their doc is now in "docs/about/emulation.rst". You may want to 
> rebase on top of master.

I see. I'll rebase and update the documentation with v2.

> 
> Globally, I'm ok with this plugin and the implementation. Just a few 
> fixes are needed for concurrent accesses.
> 
> On 8/12/24 23:46, Akihiko Odaki wrote:
>> SimPoint is a widely used tool to find the ideal microarchitecture
>> simulation points so Valgrind[2] and Pin[3] support generating basic
>> block vectors for use with them. Let's add a corresponding plugin to
>> QEMU too.
>>
>> Note that this plugin has a different goal with tests/plugin/bb.c.
>>
>> This plugin creates a vector for each constant interval instead of
>> counting the execution of basic blocks for the entire run and able to
>> describe the change of execution behavior. Its output is also
>> syntactically simple and better suited for parsing, while the output of
>> tests/plugin/bb.c is more human-readable.
>>
> 
> I think it can be confusing to have two plugins named bb. How about 
> simpoint, or bbv?

How about renaming tests/tcg/plugins/bb.c to simple-bb.c instead and 
keep the name of contrib/plugins/bb.c concise?

tests/tcg/plugins/bb.c is simple and good as a sample, but less useful 
in practice than this plugin due to differences described earlier. On 
the other hand, this plugin is designed to be utilized for practical 
purpose so I want to keep its name short and save typing.

> 
>> [1] https://cseweb.ucsd.edu/~calder/simpoint/
>> [2] https://valgrind.org/docs/manual/bbv-manual.html
>> [3] 
>> https://www.intel.com/content/www/us/en/developer/articles/tool/pin-a-dynamic-binary-instrumentation-tool.html
>>
>> Signed-off-by: Yotaro Nada <yotaro.nada@gmail.com>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   docs/devel/tcg-plugins.rst |  20 ++++++
>>   contrib/plugins/bb.c       | 153 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>   contrib/plugins/Makefile   |   1 +
>>   3 files changed, 174 insertions(+)
>>
>> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
>> index 9cc09d8c3da1..2859eecc13b9 100644
>> --- a/docs/devel/tcg-plugins.rst
>> +++ b/docs/devel/tcg-plugins.rst
>> @@ -332,6 +332,26 @@ run::
>>     160          1      0
>>     135          1      0
>> +- contrib/plugins/bb.c
>> +
>> +The bb plugin allows you to generates basic block vectors for use 
>> with the
>> +`SimPoint <https://cseweb.ucsd.edu/~calder/simpoint/>`__ analysis tool.
>> +
>> +It has two options, ``interval`` and ``outfile``. ``interval`` 
>> specifies the
>> +interval to generate a basic block vector by the number of 
>> instructions. It is
>> +optional, and its default value is 100000000. ``outfile`` is the path to
>> +output files, and it will be suffixed with ``.N.bb`` where ``N`` is a 
>> vCPU
>> +index.
>> +
>> +Example::
>> +
>> +  $ qemu-aarch64 \
>> +    -plugin contrib/plugins/libb.so,interval=100,outfile=sha1 \
>> +    tests/tcg/aarch64-linux-user/sha1
>> +  SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6
>> +  $ du sha1.0.bb
>> +  23128   sha1.0.bb
>> +
>>   - contrib/plugins/hotblocks.c
>>   The hotblocks plugin allows you to examine the where hot paths of
>> diff --git a/contrib/plugins/bb.c b/contrib/plugins/bb.c
>> new file mode 100644
>> index 000000000000..4f1266d07ff5
>> --- /dev/null
>> +++ b/contrib/plugins/bb.c
>> @@ -0,0 +1,153 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +
> 
> A brief summary and the link to simpoint page can be added as a comment 
> here.

I'll add them with v2.

> 
>> +#include <stdio.h>
>> +#include <glib.h>
>> +
>> +#include <qemu-plugin.h>
>> +
>> +typedef struct Bb {
>> +    struct qemu_plugin_scoreboard *count;
>> +    unsigned int index;
>> +} Bb;
>> +
>> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>> +static GHashTable *bbs;
>> +static GPtrArray *files;
>> +static char *filename;
>> +static struct qemu_plugin_scoreboard *count;
>> +static uint64_t interval = 100000000;
>> +
>> +static void plugin_exit(qemu_plugin_id_t id, void *p)
>> +{
>> +    g_hash_table_unref(bbs);
>> +    g_ptr_array_unref(files);
>> +    g_free(filename);
>> +    qemu_plugin_scoreboard_free(count);
>> +}
>> +
>> +static void free_bb(void *data)
>> +{
>> +    qemu_plugin_scoreboard_free(((Bb *)data)->count);
>> +    g_free(data);
>> +}
>> +
>> +static void free_file(void *data)
>> +{
>> +    fclose(data);
>> +}
>> + > +static directly count_u64(void)
>> +{
>> +    return qemu_plugin_scoreboard_u64(count);
>> +}
>> +
>> +static qemu_plugin_u64 bb_count_u64(Bb *bb)
>> +{
>> +    return qemu_plugin_scoreboard_u64(bb->count);
>> +}
>> +
>> +static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
>> +{
>> +    g_autofree gchar *vcpu_filename = NULL;
>> +
>> +    if (vcpu_index >= files->len) {
>> +        g_ptr_array_set_size(files, vcpu_index + 1);
>> +    } else if (g_ptr_array_index(files, vcpu_index)) {
>> +        return;
>> +    }
>> +
> 
> You need a lock for files array for expansion/access.

I will replace GPtrArray with scoreboard instead.

> 
>> +    vcpu_filename = g_strdup_printf("%s.%u.bb", filename, vcpu_index);
>> +    g_ptr_array_index(files, vcpu_index) = fopen(vcpu_filename, "w");
>> +}
>> +
>> +static void vcpu_tb_exec(unsigned int vcpu_index, void *udata)
> 
> vcpu_tb_exec is a confusing name, as it is called when interval of insn 
> is executed. vcpu_interval_exec would be more clear.

That makes sense. I will do so with the next version.

> 
>> +{
>> +    FILE *file = g_ptr_array_index(files, vcpu_index);
> 
> Need lock when accessing this array.
> 
>> +    uint64_t count = qemu_plugin_u64_get(count_u64(), vcpu_index) - 
>> interval;
>> +    GHashTableIter iter;
>> +    void *value;
>> +
>> +    if (!file) {
>> +        return;
>> +    }
>> +
>> +    qemu_plugin_u64_set(count_u64(), vcpu_index, count);
>> + > +    fputc('T', file);
> 
> Maybe you can add a comment with a link to 
> https://valgrind.org/docs/manual/bbv-manual.html at this point. It's 
> already mentioned in the commit message, but IMHO, it's convenient to 
> have it here, so we know which data is written exactly.

SimPoint should be referred to know the data format. I cited Valgrind 
only to show the popularity of the format and to motivate its 
implementation for QEMU.

> 
>> +
>> +    g_hash_table_iter_init(&iter, bbs);
>> +
> The access to this hashtable should be under a dedicated lock.

I will add a lock with v2.

Regards,
Akihiko Odaki

On 8/13/24 21:56, Akihiko Odaki wrote:
> On 2024/08/14 4:20, Pierrick Bouvier wrote:
>> Hi Akihiko, and thanks for contributing this new plugin.
> 
> Hi,
> 
> Thanks for reviewing
> 
>>
>> Recently, plugins documentation has been modified, and list of plugins
>> and their doc is now in "docs/about/emulation.rst". You may want to
>> rebase on top of master.
> 
> I see. I'll rebase and update the documentation with v2.
> 
>>
>> Globally, I'm ok with this plugin and the implementation. Just a few
>> fixes are needed for concurrent accesses.
>>
>> On 8/12/24 23:46, Akihiko Odaki wrote:
>>> SimPoint is a widely used tool to find the ideal microarchitecture
>>> simulation points so Valgrind[2] and Pin[3] support generating basic
>>> block vectors for use with them. Let's add a corresponding plugin to
>>> QEMU too.
>>>
>>> Note that this plugin has a different goal with tests/plugin/bb.c.
>>>
>>> This plugin creates a vector for each constant interval instead of
>>> counting the execution of basic blocks for the entire run and able to
>>> describe the change of execution behavior. Its output is also
>>> syntactically simple and better suited for parsing, while the output of
>>> tests/plugin/bb.c is more human-readable.
>>>
>>
>> I think it can be confusing to have two plugins named bb. How about
>> simpoint, or bbv?
> 
> How about renaming tests/tcg/plugins/bb.c to simple-bb.c instead and
> keep the name of contrib/plugins/bb.c concise?
> 
> tests/tcg/plugins/bb.c is simple and good as a sample, but less useful
> in practice than this plugin due to differences described earlier. On
> the other hand, this plugin is designed to be utilized for practical
> purpose so I want to keep its name short and save typing.
> 

I would recommend using the <tab> key to save typing. I'm kidding :)

More seriously, I get your point, but I don't think this new plugin is 
generic enough to be qualified as "the" bb plugin (nor the 
tests/tcg/plugins/bb neither, but it predates this new plugin). It 
outputs format for SimPoint, so how about simpoint (accessible after 
typing si<tab>)?

>>
>>> [1] https://cseweb.ucsd.edu/~calder/simpoint/
>>> [2] https://valgrind.org/docs/manual/bbv-manual.html
>>> [3]
>>> https://www.intel.com/content/www/us/en/developer/articles/tool/pin-a-dynamic-binary-instrumentation-tool.html
>>>
>>> Signed-off-by: Yotaro Nada <yotaro.nada@gmail.com>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>>    docs/devel/tcg-plugins.rst |  20 ++++++
>>>    contrib/plugins/bb.c       | 153
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>    contrib/plugins/Makefile   |   1 +
>>>    3 files changed, 174 insertions(+)
>>>
>>> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
>>> index 9cc09d8c3da1..2859eecc13b9 100644
>>> --- a/docs/devel/tcg-plugins.rst
>>> +++ b/docs/devel/tcg-plugins.rst
>>> @@ -332,6 +332,26 @@ run::
>>>      160          1      0
>>>      135          1      0
>>> +- contrib/plugins/bb.c
>>> +
>>> +The bb plugin allows you to generates basic block vectors for use
>>> with the
>>> +`SimPoint <https://cseweb.ucsd.edu/~calder/simpoint/>`__ analysis tool.
>>> +
>>> +It has two options, ``interval`` and ``outfile``. ``interval``
>>> specifies the
>>> +interval to generate a basic block vector by the number of
>>> instructions. It is
>>> +optional, and its default value is 100000000. ``outfile`` is the path to
>>> +output files, and it will be suffixed with ``.N.bb`` where ``N`` is a
>>> vCPU
>>> +index.
>>> +
>>> +Example::
>>> +
>>> +  $ qemu-aarch64 \
>>> +    -plugin contrib/plugins/libb.so,interval=100,outfile=sha1 \
>>> +    tests/tcg/aarch64-linux-user/sha1
>>> +  SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6
>>> +  $ du sha1.0.bb
>>> +  23128   sha1.0.bb
>>> +
>>>    - contrib/plugins/hotblocks.c
>>>    The hotblocks plugin allows you to examine the where hot paths of
>>> diff --git a/contrib/plugins/bb.c b/contrib/plugins/bb.c
>>> new file mode 100644
>>> index 000000000000..4f1266d07ff5
>>> --- /dev/null
>>> +++ b/contrib/plugins/bb.c
>>> @@ -0,0 +1,153 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +
>>
>> A brief summary and the link to simpoint page can be added as a comment
>> here.
> 
> I'll add them with v2.
> 
>>
>>> +#include <stdio.h>
>>> +#include <glib.h>
>>> +
>>> +#include <qemu-plugin.h>
>>> +
>>> +typedef struct Bb {
>>> +    struct qemu_plugin_scoreboard *count;
>>> +    unsigned int index;
>>> +} Bb;
>>> +
>>> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>>> +static GHashTable *bbs;
>>> +static GPtrArray *files;
>>> +static char *filename;
>>> +static struct qemu_plugin_scoreboard *count;
>>> +static uint64_t interval = 100000000;
>>> +
>>> +static void plugin_exit(qemu_plugin_id_t id, void *p)
>>> +{
>>> +    g_hash_table_unref(bbs);
>>> +    g_ptr_array_unref(files);
>>> +    g_free(filename);
>>> +    qemu_plugin_scoreboard_free(count);
>>> +}
>>> +
>>> +static void free_bb(void *data)
>>> +{
>>> +    qemu_plugin_scoreboard_free(((Bb *)data)->count);
>>> +    g_free(data);
>>> +}
>>> +
>>> +static void free_file(void *data)
>>> +{
>>> +    fclose(data);
>>> +}
>>> + > +static directly count_u64(void)
>>> +{
>>> +    return qemu_plugin_scoreboard_u64(count);
>>> +}
>>> +
>>> +static qemu_plugin_u64 bb_count_u64(Bb *bb)
>>> +{
>>> +    return qemu_plugin_scoreboard_u64(bb->count);
>>> +}
>>> +
>>> +static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
>>> +{
>>> +    g_autofree gchar *vcpu_filename = NULL;
>>> +
>>> +    if (vcpu_index >= files->len) {
>>> +        g_ptr_array_set_size(files, vcpu_index + 1);
>>> +    } else if (g_ptr_array_index(files, vcpu_index)) {
>>> +        return;
>>> +    }
>>> +
>>
>> You need a lock for files array for expansion/access.
> 
> I will replace GPtrArray with scoreboard instead.
> 

Good idea. You'll have to cleanup files at the end manually, as there is 
no support for custom destructor.

>>
>>> +    vcpu_filename = g_strdup_printf("%s.%u.bb", filename, vcpu_index);
>>> +    g_ptr_array_index(files, vcpu_index) = fopen(vcpu_filename, "w");
>>> +}
>>> +
>>> +static void vcpu_tb_exec(unsigned int vcpu_index, void *udata)
>>
>> vcpu_tb_exec is a confusing name, as it is called when interval of insn
>> is executed. vcpu_interval_exec would be more clear.
> 
> That makes sense. I will do so with the next version.
> 
>>
>>> +{
>>> +    FILE *file = g_ptr_array_index(files, vcpu_index);
>>
>> Need lock when accessing this array.
>>
>>> +    uint64_t count = qemu_plugin_u64_get(count_u64(), vcpu_index) -
>>> interval;
>>> +    GHashTableIter iter;
>>> +    void *value;
>>> +
>>> +    if (!file) {
>>> +        return;
>>> +    }
>>> +
>>> +    qemu_plugin_u64_set(count_u64(), vcpu_index, count);
>>> + > +    fputc('T', file);
>>
>> Maybe you can add a comment with a link to
>> https://valgrind.org/docs/manual/bbv-manual.html at this point. It's
>> already mentioned in the commit message, but IMHO, it's convenient to
>> have it here, so we know which data is written exactly.
> 
> SimPoint should be referred to know the data format. I cited Valgrind
> only to show the popularity of the format and to motivate its
> implementation for QEMU.
> 

I understood that, but it happens that I didn't find a simple 
description of the format on SimPoint website (maybe I missed it?), 
while valgrind doc page is clear and concise. Feel free to point the 
right page if it exists.

>>
>>> +
>>> +    g_hash_table_iter_init(&iter, bbs);
>>> +
>> The access to this hashtable should be under a dedicated lock.
> 
> I will add a lock with v2.
> 
> Regards,
> Akihiko Odaki

On 2024/08/14 14:41, Pierrick Bouvier wrote:
> On 8/13/24 21:56, Akihiko Odaki wrote:
>> On 2024/08/14 4:20, Pierrick Bouvier wrote:
>>> Hi Akihiko, and thanks for contributing this new plugin.
>>
>> Hi,
>>
>> Thanks for reviewing
>>
>>>
>>> Recently, plugins documentation has been modified, and list of plugins
>>> and their doc is now in "docs/about/emulation.rst". You may want to
>>> rebase on top of master.
>>
>> I see. I'll rebase and update the documentation with v2.
>>
>>>
>>> Globally, I'm ok with this plugin and the implementation. Just a few
>>> fixes are needed for concurrent accesses.
>>>
>>> On 8/12/24 23:46, Akihiko Odaki wrote:
>>>> SimPoint is a widely used tool to find the ideal microarchitecture
>>>> simulation points so Valgrind[2] and Pin[3] support generating basic
>>>> block vectors for use with them. Let's add a corresponding plugin to
>>>> QEMU too.
>>>>
>>>> Note that this plugin has a different goal with tests/plugin/bb.c.
>>>>
>>>> This plugin creates a vector for each constant interval instead of
>>>> counting the execution of basic blocks for the entire run and able to
>>>> describe the change of execution behavior. Its output is also
>>>> syntactically simple and better suited for parsing, while the output of
>>>> tests/plugin/bb.c is more human-readable.
>>>>
>>>
>>> I think it can be confusing to have two plugins named bb. How about
>>> simpoint, or bbv?
>>
>> How about renaming tests/tcg/plugins/bb.c to simple-bb.c instead and
>> keep the name of contrib/plugins/bb.c concise?
>>
>> tests/tcg/plugins/bb.c is simple and good as a sample, but less useful
>> in practice than this plugin due to differences described earlier. On
>> the other hand, this plugin is designed to be utilized for practical
>> purpose so I want to keep its name short and save typing.
>>
> 
> I would recommend using the <tab> key to save typing. I'm kidding :)
> 
> More seriously, I get your point, but I don't think this new plugin is 
> generic enough to be qualified as "the" bb plugin (nor the 
> tests/tcg/plugins/bb neither, but it predates this new plugin). It 
> outputs format for SimPoint, so how about simpoint (accessible after 
> typing si<tab>)?

Perhaps bbv is a better naming. I expect there are more use cases of 
basic block vectors than feeding it for SimPoint. Searching for "basic 
block vectors" with Google Scholar show this terminology has a 
consistent meaning and used in several researches.

> 
>>>
>>>> [1] https://cseweb.ucsd.edu/~calder/simpoint/
>>>> [2] https://valgrind.org/docs/manual/bbv-manual.html
>>>> [3]
>>>> https://www.intel.com/content/www/us/en/developer/articles/tool/pin-a-dynamic-binary-instrumentation-tool.html
>>>>
>>>> Signed-off-by: Yotaro Nada <yotaro.nada@gmail.com>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>>    docs/devel/tcg-plugins.rst |  20 ++++++
>>>>    contrib/plugins/bb.c       | 153
>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>    contrib/plugins/Makefile   |   1 +
>>>>    3 files changed, 174 insertions(+)
>>>>
>>>> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
>>>> index 9cc09d8c3da1..2859eecc13b9 100644
>>>> --- a/docs/devel/tcg-plugins.rst
>>>> +++ b/docs/devel/tcg-plugins.rst
>>>> @@ -332,6 +332,26 @@ run::
>>>>      160          1      0
>>>>      135          1      0
>>>> +- contrib/plugins/bb.c
>>>> +
>>>> +The bb plugin allows you to generates basic block vectors for use
>>>> with the
>>>> +`SimPoint <https://cseweb.ucsd.edu/~calder/simpoint/>`__ analysis 
>>>> tool.
>>>> +
>>>> +It has two options, ``interval`` and ``outfile``. ``interval``
>>>> specifies the
>>>> +interval to generate a basic block vector by the number of
>>>> instructions. It is
>>>> +optional, and its default value is 100000000. ``outfile`` is the 
>>>> path to
>>>> +output files, and it will be suffixed with ``.N.bb`` where ``N`` is a
>>>> vCPU
>>>> +index.
>>>> +
>>>> +Example::
>>>> +
>>>> +  $ qemu-aarch64 \
>>>> +    -plugin contrib/plugins/libb.so,interval=100,outfile=sha1 \
>>>> +    tests/tcg/aarch64-linux-user/sha1
>>>> +  SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6
>>>> +  $ du sha1.0.bb
>>>> +  23128   sha1.0.bb
>>>> +
>>>>    - contrib/plugins/hotblocks.c
>>>>    The hotblocks plugin allows you to examine the where hot paths of
>>>> diff --git a/contrib/plugins/bb.c b/contrib/plugins/bb.c
>>>> new file mode 100644
>>>> index 000000000000..4f1266d07ff5
>>>> --- /dev/null
>>>> +++ b/contrib/plugins/bb.c
>>>> @@ -0,0 +1,153 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +
>>>
>>> A brief summary and the link to simpoint page can be added as a comment
>>> here.
>>
>> I'll add them with v2.
>>
>>>
>>>> +#include <stdio.h>
>>>> +#include <glib.h>
>>>> +
>>>> +#include <qemu-plugin.h>
>>>> +
>>>> +typedef struct Bb {
>>>> +    struct qemu_plugin_scoreboard *count;
>>>> +    unsigned int index;
>>>> +} Bb;
>>>> +
>>>> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>>>> +static GHashTable *bbs;
>>>> +static GPtrArray *files;
>>>> +static char *filename;
>>>> +static struct qemu_plugin_scoreboard *count;
>>>> +static uint64_t interval = 100000000;
>>>> +
>>>> +static void plugin_exit(qemu_plugin_id_t id, void *p)
>>>> +{
>>>> +    g_hash_table_unref(bbs);
>>>> +    g_ptr_array_unref(files);
>>>> +    g_free(filename);
>>>> +    qemu_plugin_scoreboard_free(count);
>>>> +}
>>>> +
>>>> +static void free_bb(void *data)
>>>> +{
>>>> +    qemu_plugin_scoreboard_free(((Bb *)data)->count);
>>>> +    g_free(data);
>>>> +}
>>>> +
>>>> +static void free_file(void *data)
>>>> +{
>>>> +    fclose(data);
>>>> +}
>>>> + > +static directly count_u64(void)
>>>> +{
>>>> +    return qemu_plugin_scoreboard_u64(count);
>>>> +}
>>>> +
>>>> +static qemu_plugin_u64 bb_count_u64(Bb *bb)
>>>> +{
>>>> +    return qemu_plugin_scoreboard_u64(bb->count);
>>>> +}
>>>> +
>>>> +static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
>>>> +{
>>>> +    g_autofree gchar *vcpu_filename = NULL;
>>>> +
>>>> +    if (vcpu_index >= files->len) {
>>>> +        g_ptr_array_set_size(files, vcpu_index + 1);
>>>> +    } else if (g_ptr_array_index(files, vcpu_index)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>
>>> You need a lock for files array for expansion/access.
>>
>> I will replace GPtrArray with scoreboard instead.
>>
> 
> Good idea. You'll have to cleanup files at the end manually, as there is 
> no support for custom destructor.
> 
>>>
>>>> +    vcpu_filename = g_strdup_printf("%s.%u.bb", filename, vcpu_index);
>>>> +    g_ptr_array_index(files, vcpu_index) = fopen(vcpu_filename, "w");
>>>> +}
>>>> +
>>>> +static void vcpu_tb_exec(unsigned int vcpu_index, void *udata)
>>>
>>> vcpu_tb_exec is a confusing name, as it is called when interval of insn
>>> is executed. vcpu_interval_exec would be more clear.
>>
>> That makes sense. I will do so with the next version.
>>
>>>
>>>> +{
>>>> +    FILE *file = g_ptr_array_index(files, vcpu_index);
>>>
>>> Need lock when accessing this array.
>>>
>>>> +    uint64_t count = qemu_plugin_u64_get(count_u64(), vcpu_index) -
>>>> interval;
>>>> +    GHashTableIter iter;
>>>> +    void *value;
>>>> +
>>>> +    if (!file) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    qemu_plugin_u64_set(count_u64(), vcpu_index, count);
>>>> + > +    fputc('T', file);
>>>
>>> Maybe you can add a comment with a link to
>>> https://valgrind.org/docs/manual/bbv-manual.html at this point. It's
>>> already mentioned in the commit message, but IMHO, it's convenient to
>>> have it here, so we know which data is written exactly.
>>
>> SimPoint should be referred to know the data format. I cited Valgrind
>> only to show the popularity of the format and to motivate its
>> implementation for QEMU.
>>
> 
> I understood that, but it happens that I didn't find a simple 
> description of the format on SimPoint website (maybe I missed it?), 
> while valgrind doc page is clear and concise. Feel free to point the 
> right page if it exists.

The authoritative reference would be README.txt included in SimPoint 
3.2. The Valgrind documentation is more accessible, but anyone who tries 
to generate or parse it should refer to SimPoint.

Regards,
Akihiko Odaki

On 8/13/24 23:32, Akihiko Odaki wrote:
> On 2024/08/14 14:41, Pierrick Bouvier wrote:
>> On 8/13/24 21:56, Akihiko Odaki wrote:
>>> On 2024/08/14 4:20, Pierrick Bouvier wrote:
>>>> Hi Akihiko, and thanks for contributing this new plugin.
>>>
>>> Hi,
>>>
>>> Thanks for reviewing
>>>
>>>>
>>>> Recently, plugins documentation has been modified, and list of plugins
>>>> and their doc is now in "docs/about/emulation.rst". You may want to
>>>> rebase on top of master.
>>>
>>> I see. I'll rebase and update the documentation with v2.
>>>
>>>>
>>>> Globally, I'm ok with this plugin and the implementation. Just a few
>>>> fixes are needed for concurrent accesses.
>>>>
>>>> On 8/12/24 23:46, Akihiko Odaki wrote:
>>>>> SimPoint is a widely used tool to find the ideal microarchitecture
>>>>> simulation points so Valgrind[2] and Pin[3] support generating basic
>>>>> block vectors for use with them. Let's add a corresponding plugin to
>>>>> QEMU too.
>>>>>
>>>>> Note that this plugin has a different goal with tests/plugin/bb.c.
>>>>>
>>>>> This plugin creates a vector for each constant interval instead of
>>>>> counting the execution of basic blocks for the entire run and able to
>>>>> describe the change of execution behavior. Its output is also
>>>>> syntactically simple and better suited for parsing, while the output of
>>>>> tests/plugin/bb.c is more human-readable.
>>>>>
>>>>
>>>> I think it can be confusing to have two plugins named bb. How about
>>>> simpoint, or bbv?
>>>
>>> How about renaming tests/tcg/plugins/bb.c to simple-bb.c instead and
>>> keep the name of contrib/plugins/bb.c concise?
>>>
>>> tests/tcg/plugins/bb.c is simple and good as a sample, but less useful
>>> in practice than this plugin due to differences described earlier. On
>>> the other hand, this plugin is designed to be utilized for practical
>>> purpose so I want to keep its name short and save typing.
>>>
>>
>> I would recommend using the <tab> key to save typing. I'm kidding :)
>>
>> More seriously, I get your point, but I don't think this new plugin is
>> generic enough to be qualified as "the" bb plugin (nor the
>> tests/tcg/plugins/bb neither, but it predates this new plugin). It
>> outputs format for SimPoint, so how about simpoint (accessible after
>> typing si<tab>)?
> 
> Perhaps bbv is a better naming. I expect there are more use cases of
> basic block vectors than feeding it for SimPoint. Searching for "basic
> block vectors" with Google Scholar show this terminology has a
> consistent meaning and used in several researches.
> 

bbv works for me 👍, thanks.

>>
>>>>
>>>>> [1] https://cseweb.ucsd.edu/~calder/simpoint/
>>>>> [2] https://valgrind.org/docs/manual/bbv-manual.html
>>>>> [3]
>>>>> https://www.intel.com/content/www/us/en/developer/articles/tool/pin-a-dynamic-binary-instrumentation-tool.html
>>>>>
>>>>> Signed-off-by: Yotaro Nada <yotaro.nada@gmail.com>
>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>> ---
>>>>>     docs/devel/tcg-plugins.rst |  20 ++++++
>>>>>     contrib/plugins/bb.c       | 153
>>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>>     contrib/plugins/Makefile   |   1 +
>>>>>     3 files changed, 174 insertions(+)
>>>>>
>>>>> diff --git a/docs/devel/tcg-plugins.rst b/docs/devel/tcg-plugins.rst
>>>>> index 9cc09d8c3da1..2859eecc13b9 100644
>>>>> --- a/docs/devel/tcg-plugins.rst
>>>>> +++ b/docs/devel/tcg-plugins.rst
>>>>> @@ -332,6 +332,26 @@ run::
>>>>>       160          1      0
>>>>>       135          1      0
>>>>> +- contrib/plugins/bb.c
>>>>> +
>>>>> +The bb plugin allows you to generates basic block vectors for use
>>>>> with the
>>>>> +`SimPoint <https://cseweb.ucsd.edu/~calder/simpoint/>`__ analysis
>>>>> tool.
>>>>> +
>>>>> +It has two options, ``interval`` and ``outfile``. ``interval``
>>>>> specifies the
>>>>> +interval to generate a basic block vector by the number of
>>>>> instructions. It is
>>>>> +optional, and its default value is 100000000. ``outfile`` is the
>>>>> path to
>>>>> +output files, and it will be suffixed with ``.N.bb`` where ``N`` is a
>>>>> vCPU
>>>>> +index.
>>>>> +
>>>>> +Example::
>>>>> +
>>>>> +  $ qemu-aarch64 \
>>>>> +    -plugin contrib/plugins/libb.so,interval=100,outfile=sha1 \
>>>>> +    tests/tcg/aarch64-linux-user/sha1
>>>>> +  SHA1=15dd99a1991e0b3826fede3deffc1feba42278e6
>>>>> +  $ du sha1.0.bb
>>>>> +  23128   sha1.0.bb
>>>>> +
>>>>>     - contrib/plugins/hotblocks.c
>>>>>     The hotblocks plugin allows you to examine the where hot paths of
>>>>> diff --git a/contrib/plugins/bb.c b/contrib/plugins/bb.c
>>>>> new file mode 100644
>>>>> index 000000000000..4f1266d07ff5
>>>>> --- /dev/null
>>>>> +++ b/contrib/plugins/bb.c
>>>>> @@ -0,0 +1,153 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>> +
>>>>
>>>> A brief summary and the link to simpoint page can be added as a comment
>>>> here.
>>>
>>> I'll add them with v2.
>>>
>>>>
>>>>> +#include <stdio.h>
>>>>> +#include <glib.h>
>>>>> +
>>>>> +#include <qemu-plugin.h>
>>>>> +
>>>>> +typedef struct Bb {
>>>>> +    struct qemu_plugin_scoreboard *count;
>>>>> +    unsigned int index;
>>>>> +} Bb;
>>>>> +
>>>>> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
>>>>> +static GHashTable *bbs;
>>>>> +static GPtrArray *files;
>>>>> +static char *filename;
>>>>> +static struct qemu_plugin_scoreboard *count;
>>>>> +static uint64_t interval = 100000000;
>>>>> +
>>>>> +static void plugin_exit(qemu_plugin_id_t id, void *p)
>>>>> +{
>>>>> +    g_hash_table_unref(bbs);
>>>>> +    g_ptr_array_unref(files);
>>>>> +    g_free(filename);
>>>>> +    qemu_plugin_scoreboard_free(count);
>>>>> +}
>>>>> +
>>>>> +static void free_bb(void *data)
>>>>> +{
>>>>> +    qemu_plugin_scoreboard_free(((Bb *)data)->count);
>>>>> +    g_free(data);
>>>>> +}
>>>>> +
>>>>> +static void free_file(void *data)
>>>>> +{
>>>>> +    fclose(data);
>>>>> +}
>>>>> + > +static directly count_u64(void)
>>>>> +{
>>>>> +    return qemu_plugin_scoreboard_u64(count);
>>>>> +}
>>>>> +
>>>>> +static qemu_plugin_u64 bb_count_u64(Bb *bb)
>>>>> +{
>>>>> +    return qemu_plugin_scoreboard_u64(bb->count);
>>>>> +}
>>>>> +
>>>>> +static void vcpu_init(qemu_plugin_id_t id, unsigned int vcpu_index)
>>>>> +{
>>>>> +    g_autofree gchar *vcpu_filename = NULL;
>>>>> +
>>>>> +    if (vcpu_index >= files->len) {
>>>>> +        g_ptr_array_set_size(files, vcpu_index + 1);
>>>>> +    } else if (g_ptr_array_index(files, vcpu_index)) {
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>
>>>> You need a lock for files array for expansion/access.
>>>
>>> I will replace GPtrArray with scoreboard instead.
>>>
>>
>> Good idea. You'll have to cleanup files at the end manually, as there is
>> no support for custom destructor.
>>
>>>>
>>>>> +    vcpu_filename = g_strdup_printf("%s.%u.bb", filename, vcpu_index);
>>>>> +    g_ptr_array_index(files, vcpu_index) = fopen(vcpu_filename, "w");
>>>>> +}
>>>>> +
>>>>> +static void vcpu_tb_exec(unsigned int vcpu_index, void *udata)
>>>>
>>>> vcpu_tb_exec is a confusing name, as it is called when interval of insn
>>>> is executed. vcpu_interval_exec would be more clear.
>>>
>>> That makes sense. I will do so with the next version.
>>>
>>>>
>>>>> +{
>>>>> +    FILE *file = g_ptr_array_index(files, vcpu_index);
>>>>
>>>> Need lock when accessing this array.
>>>>
>>>>> +    uint64_t count = qemu_plugin_u64_get(count_u64(), vcpu_index) -
>>>>> interval;
>>>>> +    GHashTableIter iter;
>>>>> +    void *value;
>>>>> +
>>>>> +    if (!file) {
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    qemu_plugin_u64_set(count_u64(), vcpu_index, count);
>>>>> + > +    fputc('T', file);
>>>>
>>>> Maybe you can add a comment with a link to
>>>> https://valgrind.org/docs/manual/bbv-manual.html at this point. It's
>>>> already mentioned in the commit message, but IMHO, it's convenient to
>>>> have it here, so we know which data is written exactly.
>>>
>>> SimPoint should be referred to know the data format. I cited Valgrind
>>> only to show the popularity of the format and to motivate its
>>> implementation for QEMU.
>>>
>>
>> I understood that, but it happens that I didn't find a simple
>> description of the format on SimPoint website (maybe I missed it?),
>> while valgrind doc page is clear and concise. Feel free to point the
>> right page if it exists.
> 
> The authoritative reference would be README.txt included in SimPoint
> 3.2. The Valgrind documentation is more accessible, but anyone who tries
> to generate or parse it should refer to SimPoint.
> 

Thanks. I found this link that works, but it's not an official mirror:
https://github.com/hanhwi/SimPoint/blob/9c42d14fbfb90207dde915d705e11101b0e12e9f/README.txt#L153

> Regards,
> Akihiko Odaki