On 2026/10/09 11:30, Pierrick Bouvier wrote:
On 10/8/2026 5:57 PM, Akihiko Odaki wrote:On 2026/10/09 1:40, Pierrick Bouvier wrote:On 10/7/2026 11:11 PM, Akihiko Odaki wrote:On 2026/10/08 10:08, Pierrick Bouvier wrote:On 10/6/2026 11:01 PM, Akihiko Odaki wrote:A region of interest (ROI) often excludes the program startup and exit. For example, a benchmark may parse a large input and then run a computation kernel and the former may take longer than the latter; basic block vectors generated for the whole execution will not properly represent the latter.Add options to specify the beginning and the end of the ROI to bound basic block vector generation to the ROI. Signed-off-by: Akihiko Odaki <[email protected]> --- docs/about/emulation.rst | 8 +++++ contrib/plugins/bbv.c | 80 +++++++++++++++++++++++++++++++++++++ +++++++++-- 2 files changed, 86 insertions(+), 2 deletions(-) diff --git a/docs/about/emulation.rst b/docs/about/emulation.rst index c58149086cdd..0566171b517b 100644 --- a/docs/about/emulation.rst +++ b/docs/about/emulation.rst @@ -284,6 +284,14 @@ The bbv plugin allows you to generate basic block vectors for use with the * - Option - Description + * - begin=A + - The virtual address of the instruction that starts basic block vector + generation. Generation starts from the beginning of the execution if + not specified. + * - end=A + - The virtual address of the instruction that ends basic block vector + generation. Generation continues until the endo of the execution ifsmall typo here: s/endo/endThanks for catching the typo.+ not specified. * - interval=N - The interval to generate a basic block vector specified by the number of instructions (Default: N = 100000000) diff --git a/contrib/plugins/bbv.c b/contrib/plugins/bbv.c index e5caf5d9543b..a2a779f7639e 100644 --- a/contrib/plugins/bbv.c +++ b/contrib/plugins/bbv.c @@ -5,6 +5,8 @@ * SPDX-License-Identifier: GPL-2.0-or-later */ +#include <stdbool.h> +#include <stdint.h> #include <stdio.h> #include <glib.h> @@ -26,6 +28,10 @@ static GHashTable *bbs; static GRWLock bbs_lock; static char *filename; static struct qemu_plugin_scoreboard *vcpus; +static bool begin_present; +static uint64_t begin; +static bool end_present; +static uint64_t end; static int64_t interval = 100000000; static void plugin_exit(void *p) @@ -66,6 +72,7 @@ static void vcpu_init(unsigned int vcpu_index, void *userdata) Vcpu *vcpu = qemu_plugin_scoreboard_find(vcpus, vcpu_index); vcpu_filename = g_strdup_printf("%s.%u.bb", filename, vcpu_index); + vcpu->count = begin_present ? INT64_MIN : -interval;Not sure which semantic we are trying to achieve with -interval here. The original logic was to increment this count, until reaching interval, after what we would reset it by using vcpu_count -= interval, to keep the extra insn that went in after the threshold.The counter now starts at -interval and counts up to zero. Subtracting interval after emitting a vector still preserves the extra instructions. For example, with interval=100: initial count: -100 after 130 insns: 30 -> emit a vector subtract interval: -70 -> 70 insns until the next vectorvcpu->file = fopen(vcpu_filename, "w"); } @@ -102,6 +109,22 @@ static void vcpu_interval_exec(unsigned int vcpu_index, void *udata) fputc('\n', vcpu->file); } +static void vcpu_begin_insn_exec(unsigned int vcpu_index, void *userdata) +{ + GHashTableIter iter; + void *value; + + g_rw_lock_reader_lock(&bbs_lock); + g_hash_table_iter_init(&iter, bbs); + + while (g_hash_table_iter_next(&iter, NULL, &value)) { + qemu_plugin_u64_set(bb_count_u64(value), vcpu_index, 0); + } + + g_rw_lock_reader_unlock(&bbs_lock); + qemu_plugin_u64_set(count_u64(), vcpu_index, -interval); +} + static void vcpu_tb_trans(struct qemu_plugin_tb *tb, void *userdata) { uint64_t n_insns = qemu_plugin_tb_n_insns(tb); @@ -119,6 +142,19 @@ static void vcpu_tb_trans(struct qemu_plugin_tb *tb, void *userdata) } g_rw_lock_writer_unlock(&bbs_lock); + for (size_t idx = 0; idx < n_insns; idx++) { + struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, idx); + uint64_t insn_vaddr = qemu_plugin_insn_vaddr(insn); + + if (begin_present && insn_vaddr == begin) { + qemu_plugin_register_vcpu_insn_exec_cb( + insn, vcpu_begin_insn_exec, QEMU_PLUGIN_CB_NO_REGS, NULL); + } else if (end_present && insn_vaddr == end) { + qemu_plugin_register_vcpu_insn_exec_inline_per_vcpu( + insn, QEMU_PLUGIN_INLINE_STORE_U64, count_u64(), INT64_MIN); + } + } + qemu_plugin_register_vcpu_tb_exec_inline_per_vcpu( tb, QEMU_PLUGIN_INLINE_ADD_U64, count_u64(), n_insns); @@ -127,7 +163,32 @@ static void vcpu_tb_trans(struct qemu_plugin_tb *tb, void *userdata) qemu_plugin_register_vcpu_tb_exec_cond_cb( tb, vcpu_interval_exec, QEMU_PLUGIN_CB_NO_REGS, - QEMU_PLUGIN_COND_GE, count_u64(), interval, NULL); + QEMU_PLUGIN_COND_LT, count_u64(), INT64_MIN, NULL); +}is INT64_MIN used as an arbitrary large value when interpreted as unsigned immediate? If yes, that's confusing to the reader.The unsigned comparison count < (uint64_t)INT64_MIN is equivalent with the signed comparison (int64_t)count >= 0. That is why this particular value is used. Setting count to INT64_MIN suppresses output for the next 2^63 counted instructions, which is effectively disabled for this use. The begin callback clears the BB counters and sets count to -interval; end sets it back to INT64_MIN. The requested interval stays unchanged.Ok I see better. Even though it works, it's pretty confusing to be honest. Inline operatons works with unsigned values, so with conversion and wrap around, it's safe, but really not intuitive. I would prefer if you could refactor this to keep a zero initialized count value, and add a specific instrumentation path for begin, end logic (COND_LT, UINT64_MAX - N) to keep the same behavior of "infinite interval".We cannot choose a separate instrumentation path based on whether execution is inside the ROI. Instrumentation is installed when a TB is translated, but the same TB may execute both inside and outside the ROI. I will preserve this logic and add code comments for clarification.I took some time to read again how bbv works. Indeed, my idea to modify interval was a non sense, and not what is expected. The current instrumentation per tb is always done unconditionally, and the only need thing to do is to snapshot bbs hashtable to keep track of the correct data only when being inside the ROI. The rest of existing logic stays the same. - On enter, set vcpu->count to 0 (fresh start) and restore bb table from previous exit (empty table if there was no exit yet). - On exit, duplicate the bb table to allow to restore it next time we enter in ROI. Key idea is that we keep on adding things in bb table, but save/restore when leaving/entering ROI. I think for robustness you should also add detection of those expected transitions: Out of ROI -> Enter ROI -> Exit ROI -> Enter ROI -> ... Nothing is currently done to detect if if we meet enter/exit address multiple times, without having the correct sequence. I don't know if we want to report an error or handle reentrance correctly in this case. Also, should count be reset on each entrance, or do we want to keep track of it also, and save/restore that on exit/enter? I tend to think the idea is create a "virtual" interval than only is alive when being inside the ROI. In this case, we should keep track of this value also.
The current design that resets the bb table is intentional. A "virtual" interval spanning multiple ROIs would be misleading because it mixes different sections of execution.
I don't think we need to add a special handling to enforce a particular order of transitions. This patch performs the same action for the begin and end addresses and that makes the behavior predictable.
Perhaps we can extend the documentation to clarify the "begin" marker to tell that it resets the bb table, making the behavior clear regardless of transition order.
Regards, Akihiko Odaki
