On 04/03/2026 16:35, Florian Hofhammer wrote: > 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?
As a follow-up on this, changing this seems to be a bit more complicated
than I initially thought. The arguments could be simply made unsigned,
but the return value (which is also just a register content and should
therefore be unsigned if I understand correctly) can't easily be made
unsigned. The per-target main loop relies on the return value of
do_syscall() to be signed to determine whether one of the QEMU-specific
errors (e.g., QEMU_ERESTARTSYS) was returned.
>>
>>> + 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
