On 03/03/2026 20:46, Pierrick Bouvier wrote: > On 3/3/26 5:07 AM, Florian Hofhammer wrote: >> The test plugin intercepts execution in different contexts. Without the >> plugin, any of the implemented test functions would trigger an assert >> and fail. With the plugin, control flow is redirected to skip the assert >> and return cleanly via the qemu_plugin_set_pc() API. >> >> Signed-off-by: Florian Hofhammer <[email protected]> >> --- >> MAINTAINERS | 1 + >> tests/tcg/arm/Makefile.target | 6 + >> tests/tcg/multiarch/Makefile.target | 17 ++- >> .../multiarch/{ => plugin}/check-plugin-output.sh | 0 >> .../{ => plugin}/test-plugin-mem-access.c | 0 >> tests/tcg/multiarch/plugin/test-plugin-set-pc.c | 140 >> +++++++++++++++++++++ >> tests/tcg/plugins/meson.build | 1 + >> tests/tcg/plugins/setpc.c | 120 ++++++++++++++++++ >> 8 files changed, 282 insertions(+), 3 deletions(-) >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 6698e5ff69..63c0af4d86 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -4104,6 +4104,7 @@ S: Maintained >> F: docs/devel/tcg-plugins.rst >> F: plugins/ >> F: tests/tcg/plugins/ >> +F: tests/tcg/multiarch/plugin/ >> F: tests/functional/aarch64/test_tcg_plugins.py >> F: contrib/plugins/ >> F: scripts/qemu-plugin-symbols.py >> diff --git a/tests/tcg/arm/Makefile.target b/tests/tcg/arm/Makefile.target >> index 6189d7a0e2..613bbf0939 100644 >> --- a/tests/tcg/arm/Makefile.target >> +++ b/tests/tcg/arm/Makefile.target >> @@ -78,4 +78,10 @@ sha512-vector: sha512.c >> ARM_TESTS += sha512-vector >> +ifeq ($(CONFIG_PLUGIN),y) >> +# Require emitting arm32 instructions, otherwise the vCPU might accidentally >> +# try to execute Thumb instructions in arm32 mode after qemu_plugin_set_pc() >> +test-plugin-set-pc: CFLAGS+=-marm >> +endif >> + > > Is that still needed?
Yes, unfortunately. The compiler emits Thumb2 instructions where it deems it appropriate but the addresses of functions/labels are still at least 2-byte aligned and therefore don't have the bottom bit set. Setting the PC to an even address therefore makes the vCPU think the target is in arm32 mode, and it tries to execute the Thumb2 instructions in arm32 mode. I'd either need to ensure that the targets are always in Thumb2 mode and set the bottom bit myself, or ensure that everything is in arm32 mode from the start. >> TESTS += $(ARM_TESTS) >> diff --git a/tests/tcg/multiarch/Makefile.target >> b/tests/tcg/multiarch/Makefile.target >> index 07d0b27bdd..a347efbadf 100644 >> --- a/tests/tcg/multiarch/Makefile.target >> +++ b/tests/tcg/multiarch/Makefile.target >> @@ -14,6 +14,10 @@ ifeq ($(filter %-linux-user, $(TARGET)),$(TARGET)) >> VPATH += $(MULTIARCH_SRC)/linux >> MULTIARCH_SRCS += $(notdir $(wildcard $(MULTIARCH_SRC)/linux/*.c)) >> endif >> +ifeq ($(CONFIG_PLUGIN),y) >> +VPATH += $(MULTIARCH_SRC)/plugin >> +MULTIARCH_SRCS += $(notdir $(wildcard $(MULTIARCH_SRC)/plugin/*.c)) >> +endif >> MULTIARCH_TESTS = $(MULTIARCH_SRCS:.c=) >> # >> @@ -200,13 +204,20 @@ run-plugin-test-plugin-mem-access-with-libmem.so: \ >> PLUGIN_ARGS=$(COMMA)print-accesses=true >> run-plugin-test-plugin-mem-access-with-libmem.so: \ >> CHECK_PLUGIN_OUTPUT_COMMAND= \ >> - $(SRC_PATH)/tests/tcg/multiarch/check-plugin-output.sh \ >> + $(SRC_PATH)/tests/tcg/multiarch/plugin/check-plugin-output.sh \ >> $(QEMU) $< >> run-plugin-test-plugin-syscall-filter-with-libsyscall.so: >> +run-plugin-test-plugin-set-pc-with-libsetpc.so: >> EXTRA_RUNS_WITH_PLUGIN += >> run-plugin-test-plugin-mem-access-with-libmem.so \ >> - run-plugin-test-plugin-syscall-filter-with-libsyscall.so >> -else >> + run-plugin-test-plugin-syscall-filter-with-libsyscall.so \ >> + run-plugin-test-plugin-set-pc-with-libsetpc.so >> + >> +else # CONFIG_PLUGIN=n >> +# Do not build the syscall skipping test if it's not tested with the setpc >> +# plugin because it will simply fail the test. >> +MULTIARCH_TESTS := $(filter-out test-plugin-set-pc, $(MULTIARCH_TESTS)) >> + >> # test-plugin-syscall-filter needs syscall plugin to succeed >> test-plugin-syscall-filter: CFLAGS+=-DSKIP >> endif >> diff --git a/tests/tcg/multiarch/check-plugin-output.sh >> b/tests/tcg/multiarch/plugin/check-plugin-output.sh >> similarity index 100% >> rename from tests/tcg/multiarch/check-plugin-output.sh >> rename to tests/tcg/multiarch/plugin/check-plugin-output.sh >> diff --git a/tests/tcg/multiarch/test-plugin-mem-access.c >> b/tests/tcg/multiarch/plugin/test-plugin-mem-access.c >> similarity index 100% >> rename from tests/tcg/multiarch/test-plugin-mem-access.c >> rename to tests/tcg/multiarch/plugin/test-plugin-mem-access.c >> diff --git a/tests/tcg/multiarch/plugin/test-plugin-set-pc.c >> b/tests/tcg/multiarch/plugin/test-plugin-set-pc.c >> new file mode 100644 >> index 0000000000..40d9a9e8f0 >> --- /dev/null >> +++ b/tests/tcg/multiarch/plugin/test-plugin-set-pc.c >> @@ -0,0 +1,140 @@ >> +/* >> + * SPDX-License-Identifier: GPL-2.0-or-later >> + * >> + * Copyright (C) 2026, Florian Hofhammer <[email protected]> >> + * >> + * This test set exercises the qemu_plugin_set_pc() function in four >> different >> + * contexts: >> + * 1. in a syscall callback, >> + * 2. in an instruction callback during normal execution, >> + * 3. in an instruction callback during signal handling, >> + * 4. in a memory access callback. >> + * Note: using the volatile guards is necessary to prevent the compiler from >> + * doing dead code elimination even on -O0, which would cause everything >> after >> + * the asserts and thus also the target labels to be optimized away. >> + */ >> +#include <assert.h> >> +#include <signal.h> >> +#include <stdint.h> >> +#include <stdlib.h> >> +#include <stdio.h> >> +#include <unistd.h> >> +#include <setjmp.h> >> + >> +#define NOINLINE __attribute__((noinline)) > > Test being compiled in O0, it should not need any noinline attribute. Ack, removed. >> +#define NORETURN __attribute__((noreturn)) > > It's used in a single place, simply move the the attribute there, no need for > another define. Ditto. >> + >> +static int signal_handled; >> +/* >> + * The volatile variable is used as a guard to prevent the compiler from >> + * optimizing away "unreachable" labels. >> + */ >> +static volatile uint32_t guard = 1; >> + > > As mentioned on v4, you can simply use an external function for failing the > test, which won't be tainted with noreturn attribute. > This way, you don't need any guard at all or assert(0), and resulting code is > linear and easier to read. > It test completes, it all worked. If it crashes, something was wrong. > > void panic(void) > { > g_assert_not_reached(); > } > > void test_...() { > ... > on_panic: > panic(); > after_panic: > printf("Hello World\n"); > } I've reworked it to look similar to this (without g_assert_not_reached() though, because glib is not available in the test itself). Still required some shuffling of stuff to make sure the compiler doesn't optimize things away then :) Especially on hexagon, which is the only target (by default) built with clang instead of gcc. Clang/LLVM seems to be way more aggressive even at -O0 to do deadcode elimination and inlining, so I had to add extra CFLAGS for the hexagon target. >> + * This test executes a magic syscall which communicates two addresses to >> the >> + * plugin via the syscall arguments. Whenever we reach the "bad" instruction >> + * during normal execution, the plugin should redirect control flow to the >> + * "good" instruction instead. >> + */ >> +NOINLINE void test_insn(void) >> +{ >> + long ret = syscall(4095, &&bad_insn, &&good_insn, NULL); >> + assert(ret == 0 && "Syscall filter did not return expected value"); >> + if (guard) { >> +bad_insn: >> + assert(0 && "PC redirection in instruction callback failed"); >> + } else { >> +good_insn: >> + return; >> + } >> +} >> + >> +/* >> + * This signal handler communicates a "bad" and a "good" address to the >> plugin >> + * similar to the previous test, and skips to the "good" address when the >> "bad" >> + * one is reached. This serves to test whether PC redirection via >> + * qemu_plugin_set_pc() also works properly in a signal handler context. >> + */ >> +NOINLINE void usr1_handler(int signum) >> +{ >> + long ret = syscall(4095, &&bad_signal, &&good_signal, NULL); >> + assert(ret == 0 && "Syscall filter did not return expected value"); >> + if (guard) { >> +bad_signal: >> + assert(0 && "PC redirection in instruction callback failed"); >> + } else { >> +good_signal: >> + signal_handled = 1; >> + return; >> + } >> +} >> + >> +/* >> + * This test sends a signal to the process, which should trigger the above >> + * signal handler. The signal handler should then exercise the PC >> redirection >> + * functionality in the context of a signal handler, which behaves a bit >> + * differently from normal execution. >> + */ >> +NOINLINE void test_sighandler(void) >> +{ >> + struct sigaction sa = {0}; >> + sa.sa_handler = usr1_handler; >> + sigaction(SIGUSR1, &sa, NULL); >> + pid_t pid = getpid(); >> + kill(pid, SIGUSR1); >> + assert(signal_handled == 1 && "Signal handler was not executed >> properly"); >> +} >> + >> +/* >> + * This test communicates a "good" address and the address of a local >> variable >> + * to the plugin. Upon accessing the local variable, the plugin should then >> + * redirect control flow to the "good" address via qemu_plugin_set_pc(). >> + */ >> +NOINLINE void test_mem(void) >> +{ >> + long ret = syscall(4095, NULL, &&good_mem, &guard); > > Since you use two different syscall numbers, it's worth adding two defines, > that you will duplicate in test and plugin. > Alternatively, you can use the first parameter of syscall to identify which > kind of "service" you want from plugin, with defines duplicated between test > and plugin again. > Feel free to pick the solution you prefer. > > Reading 4095 the first time here felt like it was a typo mistake. Ack, reworked this to be clearer. > >> + assert(ret == 0 && "Syscall filter did not return expected value"); >> + if (guard) { >> + assert(0 && "PC redirection in memory access callback failed"); >> + } else { >> +good_mem: >> + return; >> + } >> +} >> + >> +/* >> + * This test executes a magic syscall which is intercepted and its actual >> + * execution skipped via the qemu_plugin_set_pc() API. In a proper plugin, >> + * syscall skipping would rather be implemented via the syscall filtering >> + * callback, but we want to make sure qemu_plugin_set_pc() works in >> different >> + * contexts. >> + */ >> +NOINLINE NORETURN >> +void test_syscall(void) >> +{ >> + syscall(4096, &&good_syscall); >> + if (guard) { >> + assert(0 && "PC redirection in syscall callback failed"); >> + } else { >> +good_syscall: >> + /* >> + * Note: we execute this test last and exit straight from here >> because >> + * when the plugin redirects control flow upon syscall, the stack >> frame >> + * for the syscall function (and potential other functions in the >> call >> + * chain in libc) is still live and the stack is not unwound >> properly. >> + * Thus, returning from here is risky and breaks on some >> architectures, >> + * so we just exit directly from this test. >> + */ >> + _exit(EXIT_SUCCESS); >> + } >> +} >> + >> + >> +int main(int argc, char *argv[]) >> +{ >> + test_insn(); >> + test_sighandler(); >> + test_mem(); >> + test_syscall(); >> +} >> diff --git a/tests/tcg/plugins/meson.build b/tests/tcg/plugins/meson.build >> index c5e49753fd..b3e3a9a6d0 100644 >> --- a/tests/tcg/plugins/meson.build >> +++ b/tests/tcg/plugins/meson.build >> @@ -7,6 +7,7 @@ test_plugins = [ >> 'mem.c', >> 'patch.c', >> 'reset.c', >> +'setpc.c', >> 'syscall.c', >> ] >> diff --git a/tests/tcg/plugins/setpc.c b/tests/tcg/plugins/setpc.c >> new file mode 100644 >> index 0000000000..72ae31a0ef >> --- /dev/null >> +++ b/tests/tcg/plugins/setpc.c >> @@ -0,0 +1,120 @@ >> +/* >> + * SPDX-License-Identifier: GPL-2.0-or-later >> + * >> + * Copyright (C) 2026, Florian Hofhammer <[email protected]> >> + */ >> +#include <assert.h> >> +#include <glib.h> >> +#include <inttypes.h> >> +#include <unistd.h> >> + >> +#include <qemu-plugin.h> >> + >> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION; >> + >> +static uint64_t source_pc; >> +static uint64_t target_pc; >> +static uint64_t target_vaddr; >> + >> +static void vcpu_syscall(qemu_plugin_id_t id, unsigned int vcpu_index, >> + int64_t num, uint64_t a1, uint64_t a2, >> + uint64_t a3, uint64_t a4, uint64_t a5, >> + uint64_t a6, uint64_t a7, uint64_t a8) >> +{ >> + if (num == 4096) { >> + qemu_plugin_outs("Marker syscall detected, jump to clean return\n"); >> + qemu_plugin_set_pc(a1); >> + } >> +} >> + >> +static bool vcpu_syscall_filter(qemu_plugin_id_t id, unsigned int >> vcpu_index, >> + int64_t num, uint64_t a1, uint64_t a2, >> + uint64_t a3, uint64_t a4, uint64_t a5, >> + uint64_t a6, uint64_t a7, uint64_t a8, >> + uint64_t *sysret) >> +{ >> + if (num == 4095) { >> + qemu_plugin_outs("Communication syscall detected, set target_pc / " >> + "target_vaddr\n"); >> + source_pc = a1; >> + target_pc = a2; >> + target_vaddr = a3; >> + if (source_pc >> 63 || target_pc >> 63 || target_vaddr >> 63) { >> + /* >> + * Some architectures (e.g., m68k) use 32-bit addresses with the >> + * top bit set, which causes them to get sign-extended >> somewhere in >> + * the chain to this callback. We mask the top bits off here to >> get >> + * the actual addresses. >> + */ > > This looks like an actual bug. Using a backtrace here, can you find which > function extended it? > Parameter should be treated as unsigned everywhere, else something is really > going wrong. tl;dr: register values are considered signed in the syscall handling code, which seems incorrect to me. Details below. This seems to be a bigger issue in the syscall handling code and I'm surprised this never surfaced before. Generally, the CPUArchState struct in target/*/cpu.h defines the registers as unsigned types. When a syscall is encountered, the main cpu loop calls do_syscall() from linux-user/syscall.c, which takes and returns abi_long values. abi_long is defined as either int32_t or target_long in include/user/abitypes.h and therefore a signed type, so the register values get converted from unsigned to signed types here. do_syscall() passes those abi_long values on, e.g., to record_syscall_start() and send_through_syscall_filters(), which still take the register values in as abi_long. Those in turn however pass the values on to qemu_plugin_vcpu_syscall() and qemu_plugin_vcpu_syscall_filter(), respectively. Those two functions now take uint64_t values for the arguments, so for 32-bit architectures the conversion chain is uint32_t (CPUArchState) -> int32_t (do_syscall) -> uint64_t (callbacks). This seems to be some bigger refactoring changing all those abi_longs to abi_ulongs. Would you like me to prepare a separate patch series for this? > >> + qemu_plugin_outs("High bit in addresses detected: possible sign >> " >> + "extension in syscall, masking off top >> bits\n"); >> + source_pc &= UINT32_MAX; >> + target_pc &= UINT32_MAX; >> + target_vaddr &= UINT32_MAX; >> + } >> + *sysret = 0; >> + return true; >> + } >> + return false; >> +} >> + >> +static void vcpu_insn_exec(unsigned int vcpu_index, void *userdata) >> +{ >> + uint64_t vaddr = (uint64_t)userdata; >> + if (vaddr == source_pc) { >> + g_assert(target_pc != 0); >> + g_assert(target_vaddr == 0); >> + >> + qemu_plugin_outs("Marker instruction detected, jump to clean >> return\n"); >> + qemu_plugin_set_pc(target_pc); >> + } >> +} >> + >> +static void vcpu_mem_access(unsigned int vcpu_index, >> + qemu_plugin_meminfo_t info, >> + uint64_t vaddr, void *userdata) >> +{ >> + if (vaddr != 0 && vaddr == target_vaddr) { >> + g_assert(source_pc == 0); >> + g_assert(target_pc != 0); >> + qemu_plugin_mem_value val = qemu_plugin_mem_get_value(info); >> + /* target_vaddr points to our volatile guard ==> should always be 1 >> */ >> + g_assert(val.type == QEMU_PLUGIN_MEM_VALUE_U32); >> + g_assert(val.data.u32 == 1); >> + >> + qemu_plugin_outs("Marker mem access detected, jump to clean >> return\n"); >> + qemu_plugin_set_pc(target_pc); >> + } > > Thanks for adding this case also! > So you'll definitely need a read for this precise test, to be able to keep a > load. If it would work with a local variable, that's good, else, try with a > local static (eventually marked volatile if it helps), and in last case, use > a global variable. Works fine with a local variable! > >> +} >> + >> +static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb) >> +{ >> + size_t insns = qemu_plugin_tb_n_insns(tb); >> + for (size_t i = 0; i < insns; i++) { >> + struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i); >> + uint64_t insn_vaddr = qemu_plugin_insn_vaddr(insn); >> + /* >> + * Note: we cannot only register the callbacks if the instruction is >> + * in one of the functions of interest, because symbol lookup for >> + * filtering does not work for all architectures (e.g., ppc64). >> + */ > > Too sad :) > It's not a problem to instrument all instructions though. > >> + qemu_plugin_register_vcpu_insn_exec_cb(insn, vcpu_insn_exec, >> + QEMU_PLUGIN_CB_RW_REGS_PC, >> + (void *)insn_vaddr); >> + qemu_plugin_register_vcpu_mem_cb(insn, vcpu_mem_access, >> + QEMU_PLUGIN_CB_RW_REGS_PC, >> + QEMU_PLUGIN_MEM_R, NULL); >> + } >> +} >> + >> + >> +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id, >> + const qemu_info_t *info, >> + int argc, char **argv) >> +{ >> + >> + qemu_plugin_register_vcpu_syscall_cb(id, vcpu_syscall); >> + qemu_plugin_register_vcpu_syscall_filter_cb(id, vcpu_syscall_filter); >> + qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans); >> + return 0; >> +} >> > > Thanks for the update. > Test looks good, and minus the style nits reported, it looks quite ready. > It would be nice if we could sort the signed parameter extension also to > avoid a hack on this. > > Regards, > Pierrick Best regards, Florian
smime.p7s
Description: S/MIME Cryptographic Signature
