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.n...@gmail.com>
Signed-off-by: Akihiko Odaki <akihiko.od...@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
