Re: [PATCH v12 6/7] plugins: Add patcher plugin and test

["Alex Bennée" <alex.bennee@linaro.org>]

Rowan Hart <rowanbhart@gmail.com> writes:

> From: novafacing <rowanbhart@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 <rowanbhart@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