Rowan Hart <rowanbh...@gmail.com> writes:
> From: novafacing <rowanbh...@gmail.com>
>
> This patch adds a plugin that exercises the virtual and hardware memory
> read-write API functions added in a previous patch. The plugin takes a
> target and patch byte sequence, and will overwrite any instruction
> matching the target byte sequence with the patch.
>
> Signed-off-by: Rowan Hart <rowanbh...@gmail.com>
> ---
> tests/tcg/Makefile.target | 1 +
> tests/tcg/plugins/meson.build | 2 +-
> tests/tcg/plugins/patch.c | 241 ++++++++++++++++++++++
> tests/tcg/x86_64/Makefile.softmmu-target | 32 ++-
> tests/tcg/x86_64/system/patch-target.c | 27 +++
> tests/tcg/x86_64/system/validate-patch.py | 39 ++++
> 6 files changed, 336 insertions(+), 6 deletions(-)
> create mode 100644 tests/tcg/plugins/patch.c
> create mode 100644 tests/tcg/x86_64/system/patch-target.c
> create mode 100755 tests/tcg/x86_64/system/validate-patch.py
>
> diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
> index 95ff76ea44..4b709a9d18 100644
> --- a/tests/tcg/Makefile.target
> +++ b/tests/tcg/Makefile.target
> @@ -176,6 +176,7 @@ RUN_TESTS+=$(EXTRA_RUNS)
> # Some plugins need additional arguments above the default to fully
> # exercise things. We can define them on a per-test basis here.
> run-plugin-%-with-libmem.so: PLUGIN_ARGS=$(COMMA)inline=true
> +run-plugin-%-with-libpatch.so:
> PLUGIN_ARGS=$(COMMA)target=ffffffff$(COMMA)patch=00000000
>
I think we need to manually add this to the x86_64 specific tests because...
> ifeq ($(filter %-softmmu, $(TARGET)),)
> run-%: %
> diff --git a/tests/tcg/plugins/meson.build b/tests/tcg/plugins/meson.build
> index 41f02f2c7f..163042e601 100644
> --- a/tests/tcg/plugins/meson.build
> +++ b/tests/tcg/plugins/meson.build
> @@ -1,6 +1,6 @@
> t = []
> if get_option('plugins')
> - foreach i : ['bb', 'empty', 'inline', 'insn', 'mem', 'reset', 'syscall']
> + foreach i : ['bb', 'empty', 'inline', 'insn', 'mem', 'reset', 'syscall',
> 'patch']
> if host_os == 'windows'
> t += shared_module(i, files(i + '.c') +
> '../../../contrib/plugins/win32_linker.c',
> include_directories: '../../../include/qemu',
... the problem with adding test patches into tests/tcg is we balloon the
number of test cases. Whats worse we are running on linux-user tests
where we don't exercise anything.
So I think we should filter out the test from the general testing by
fixing up:
PLUGINS=$(patsubst %.c, lib%.so, $(notdir $(wildcard $(PLUGIN_SRC)/*.c)))
<snip>
> +
> +static void usage(void)
> +{
> + fprintf(stderr, "Usage: <lib>,target=<bytes>,patch=<new_bytes>"
> + "[,use_hwaddr=true|false]");
> +}
> +
> +/*
> + * Called when the plugin is installed
> + */
> +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
> + const qemu_info_t *info, int argc,
> + char **argv)
> +{
> +
> + use_hwaddr = true;
> + target_data = NULL;
> + patch_data = NULL;
> +
> + if (argc > 4) {
> + usage();
> + return -1;
> + }
> +
> + for (size_t i = 0; i < argc; i++) {
> + char *opt = argv[i];
> + g_auto(GStrv) tokens = g_strsplit(opt, "=", 2);
> + if (g_strcmp0(tokens[0], "use_hwaddr") == 0) {
> + if (!qemu_plugin_bool_parse(tokens[0], tokens[1], &use_hwaddr)) {
> + fprintf(stderr,
> + "Failed to parse boolean argument use_hwaddr\n");
> + return -1;
> + }
> + } else if (g_strcmp0(tokens[0], "target") == 0) {
> + target_data = str_to_bytes(tokens[1]);
> + if (!target_data) {
> + fprintf(stderr,
> + "Failed to parse target bytes.\n");
> + return -1;
> + }
> + } else if (g_strcmp0(tokens[0], "patch") == 0) {
> + patch_data = str_to_bytes(tokens[1]);
> + if (!patch_data) {
> + fprintf(stderr, "Failed to parse patch bytes.\n");
> + return -1;
> + }
> + } else {
> + fprintf(stderr, "Unknown argument: %s\n", tokens[0]);
> + usage();
> + return -1;
> + }
> + }
> +
> + if (!target_data) {
> + fprintf(stderr, "target argument is required\n");
> + usage();
> + return -1;
> + }
> +
> + if (!patch_data) {
> + fprintf(stderr, "patch argument is required\n");
> + usage();
> + return -1;
> + }
> +
> + if (target_data->len != patch_data->len) {
> + fprintf(stderr, "Target and patch data must be the same length\n");
> + return -1;
> + }
> +
> + qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans_cb);
> +
> + return 0;
> +}
> diff --git a/tests/tcg/x86_64/Makefile.softmmu-target
> b/tests/tcg/x86_64/Makefile.softmmu-target
> index ef6bcb4dc7..154910ab72 100644
> --- a/tests/tcg/x86_64/Makefile.softmmu-target
> +++ b/tests/tcg/x86_64/Makefile.softmmu-target
> @@ -7,18 +7,27 @@
> #
>
> I386_SYSTEM_SRC=$(SRC_PATH)/tests/tcg/i386/system
> -X64_SYSTEM_SRC=$(SRC_PATH)/tests/tcg/x86_64/system
> +X86_64_SYSTEM_SRC=$(SRC_PATH)/tests/tcg/x86_64/system
Can we have symbol renaming in a separate patch as it makes diffs messy
to follow otherwise.
>
> # These objects provide the basic boot code and helper functions for all
> tests
> CRT_OBJS=boot.o
>
> -CRT_PATH=$(X64_SYSTEM_SRC)
> -LINK_SCRIPT=$(X64_SYSTEM_SRC)/kernel.ld
> +X86_64_TEST_C_SRCS=$(wildcard $(X86_64_SYSTEM_SRC)/*.c)
> +X86_64_TEST_S_SRCS=
> +
> +X86_64_C_TESTS = $(patsubst $(X86_64_SYSTEM_SRC)/%.c, %,
> $(X86_64_TEST_C_SRCS))
> +X86_64_S_TESTS = $(patsubst $(X86_64_SYSTEM_SRC)/%.S, %,
> $(X86_64_TEST_S_SRCS))
> +
> +X86_64_TESTS = $(X86_64_C_TESTS)
> +X86_64_TESTS += $(X86_64_S_TESTS)
> +
> +CRT_PATH=$(X86_64_SYSTEM_SRC)
> +LINK_SCRIPT=$(X86_64_SYSTEM_SRC)/kernel.ld
> LDFLAGS=-Wl,-T$(LINK_SCRIPT) -Wl,-melf_x86_64
> CFLAGS+=-nostdlib -ggdb -O0 $(MINILIB_INC)
> LDFLAGS+=-static -nostdlib $(CRT_OBJS) $(MINILIB_OBJS) -lgcc
>
> -TESTS+=$(MULTIARCH_TESTS)
> +TESTS+=$(X86_64_TESTS) $(MULTIARCH_TESTS)
> EXTRA_RUNS+=$(MULTIARCH_RUNS)
>
> # building head blobs
> @@ -27,11 +36,24 @@ EXTRA_RUNS+=$(MULTIARCH_RUNS)
> %.o: $(CRT_PATH)/%.S
> $(CC) $(CFLAGS) $(EXTRA_CFLAGS) -Wa,--noexecstack -c $< -o $@
>
> -# Build and link the tests
> +# Build and link the multiarch tests
> %: %.c $(LINK_SCRIPT) $(CRT_OBJS) $(MINILIB_OBJS)
> $(CC) $(CFLAGS) $(EXTRA_CFLAGS) $< -o $@ $(LDFLAGS)
>
> +# Build and link the arch tests
> +%: $(X86_64_SYSTEM_SRC)/%.c $(LINK_SCRIPT) $(CRT_OBJS) $(MINILIB_OBJS)
> + $(CC) $(CFLAGS) $(EXTRA_CFLAGS) $< -o $@ $(LDFLAGS)
> +
Is this needed? The aarch64 vtimer system test didn't need a new build stanza.
> memory: CFLAGS+=-DCHECK_UNALIGNED=1
> +patch-target: CFLAGS+=-O0
>
> # Running
> QEMU_OPTS+=-device isa-debugcon,chardev=output -device
> isa-debug-exit,iobase=0xf4,iosize=0x4 -kernel
> +
> +# Add patch-target to ADDITIONAL_PLUGINS_TESTS
> +ADDITIONAL_PLUGINS_TESTS += patch-target
> +
> +run-plugin-patch-target-with-libpatch.so: \
> + PLUGIN_ARGS=$(COMMA)target=ffc0$(COMMA)patch=9090$(COMMA)use_hwaddr=true
> +run-plugin-patch-target-with-libpatch.so: \
> + CHECK_PLUGIN_OUTPUT_COMMAND=$(X86_64_SYSTEM_SRC)/validate-patch.py
> $@.out
> \ No newline at end of file
> diff --git a/tests/tcg/x86_64/system/patch-target.c
> b/tests/tcg/x86_64/system/patch-target.c
> new file mode 100644
> index 0000000000..8a7c0a0ae8
> --- /dev/null
> +++ b/tests/tcg/x86_64/system/patch-target.c
> @@ -0,0 +1,27 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This test target increments a value 100 times. The patcher converts the
> + * inc instruction to a nop, so it only increments the value once.
> + *
> + */
> +#include <minilib.h>
> +
> +int main(void)
> +{
> + ml_printf("Running test...\n");
> +#if defined(__x86_64__)
> + ml_printf("Testing insn memory read/write...\n");
> + unsigned int x = 0;
> + for (int i = 0; i < 100; i++) {
> + asm volatile (
> + "inc %[x]"
> + : [x] "+a" (x)
> + );
> + }
> + ml_printf("Value: %d\n", x);
> +#else
> + #error "This test is only valid for x86_64 architecture."
> +#endif
This is a bit redundant given the test is in tests/tcg/x86_64/system.
> + return 0;
> +}
> diff --git a/tests/tcg/x86_64/system/validate-patch.py
> b/tests/tcg/x86_64/system/validate-patch.py
> new file mode 100755
> index 0000000000..700950eae5
> --- /dev/null
> +++ b/tests/tcg/x86_64/system/validate-patch.py
> @@ -0,0 +1,39 @@
> +#!/usr/bin/env python3
> +#
> +# validate-patch.py: check the patch applies
> +#
> +# This program takes two inputs:
> +# - the plugin output
> +# - the binary output
> +#
> +# Copyright (C) 2024
> +#
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +import sys
> +from argparse import ArgumentParser
> +
> +def main() -> None:
> + """
> + Process the arguments, injest the program and plugin out and
> + verify they match up and report if they do not.
> + """
> + parser = ArgumentParser(description="Validate patch")
> + parser.add_argument('test_output',
> + help="The output from the test itself")
> + parser.add_argument('plugin_output',
> + help="The output from plugin")
> + args = parser.parse_args()
> +
> + with open(args.test_output, 'r') as f:
> + test_data = f.read()
> + with open(args.plugin_output, 'r') as f:
> + plugin_data = f.read()
> + if "Value: 1" in test_data:
> + sys.exit(0)
> + else:
> + sys.exit(1)
> +
> +if __name__ == "__main__":
> + main()
> +
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
