On 10/22/24 03:56, Alex Bennée wrote:
> From: Ilya Leoshkevich <iii@linux.ibm.com>
>
> commit f025692c992c ("accel/tcg: Clear PAGE_WRITE before translation")
> fixed cross-modifying code handling, but did not add a test. The
> changed code was further improved recently [1], and I was not sure
> whether these modifications were safe (spoiler: they were fine).
>
> Add a test to make sure there are no regressions.
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg00034.html
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Message-Id: <20241001150617.9977-1-iii@linux.ibm.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> tests/tcg/x86_64/cross-modifying-code.c | 80 +++++++++++++++++++++++++
> tests/tcg/x86_64/Makefile.target | 4 ++
> 2 files changed, 84 insertions(+)
> create mode 100644 tests/tcg/x86_64/cross-modifying-code.c
>
> diff --git a/tests/tcg/x86_64/cross-modifying-code.c b/tests/tcg/x86_64/cross-modifying-code.c
> new file mode 100644
> index 0000000000..2704df6061
> --- /dev/null
> +++ b/tests/tcg/x86_64/cross-modifying-code.c
> @@ -0,0 +1,80 @@
> +/*
> + * Test patching code, running in one thread, from another thread.
> + *
> + * Intel SDM calls this "cross-modifying code" and recommends a special
> + * sequence, which requires both threads to cooperate.
> + *
> + * Linux kernel uses a different sequence that does not require cooperation and
> + * involves patching the first byte with int3.
> + *
> + * Finally, there is user-mode software out there that simply uses atomics, and
> + * that seems to be good enough in practice. Test that QEMU has no problems
> + * with this as well.
> + */
> +
> +#include <assert.h>
> +#include <pthread.h>
> +#include <stdbool.h>
> +#include <stdlib.h>
> +
> +void add1_or_nop(long *x);
> +asm(".pushsection .rwx,\"awx\",@progbits\n"
> + ".globl add1_or_nop\n"
> + /* addq $0x1,(%rdi) */
> + "add1_or_nop: .byte 0x48, 0x83, 0x07, 0x01\n"
> + "ret\n"
> + ".popsection\n");
> +
> +#define THREAD_WAIT 0
> +#define THREAD_PATCH 1
> +#define THREAD_STOP 2
> +
> +static void *thread_func(void *arg)
> +{
> + int val = 0x0026748d; /* nop */
> +
> + while (true) {
> + switch (__atomic_load_n((int *)arg, __ATOMIC_SEQ_CST)) {
> + case THREAD_WAIT:
> + break;
> + case THREAD_PATCH:
> + val = __atomic_exchange_n((int *)&add1_or_nop, val,
> + __ATOMIC_SEQ_CST);
> + break;
> + case THREAD_STOP:
> + return NULL;
> + default:
> + assert(false);
> + __builtin_unreachable();
Use g_assert_not_reached() instead.
checkpatch emits an error for it now.
> + }
> + }
> +}
> +
> +#define INITIAL 42
> +#define COUNT 1000000
> +
> +int main(void)
> +{
> + int command = THREAD_WAIT;
> + pthread_t thread;
> + long x = 0;
> + int err;
> + int i;
> +
> + err = pthread_create(&thread, NULL, &thread_func, &command);
> + assert(err == 0);
> +
> + __atomic_store_n(&command, THREAD_PATCH, __ATOMIC_SEQ_CST);
> + for (i = 0; i < COUNT; i++) {
> + add1_or_nop(&x);
> + }
> + __atomic_store_n(&command, THREAD_STOP, __ATOMIC_SEQ_CST);
> +
> + err = pthread_join(thread, NULL);
> + assert(err == 0);
> +
> + assert(x >= INITIAL);
> + assert(x <= INITIAL + COUNT);
> +
> + return EXIT_SUCCESS;
> +}
> diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
> index 783ab5b21a..d6dff559c7 100644
> --- a/tests/tcg/x86_64/Makefile.target
> +++ b/tests/tcg/x86_64/Makefile.target
> @@ -17,6 +17,7 @@ X86_64_TESTS += cmpxchg
> X86_64_TESTS += adox
> X86_64_TESTS += test-1648
> X86_64_TESTS += test-2175
> +X86_64_TESTS += cross-modifying-code
> TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
> else
> TESTS=$(MULTIARCH_TESTS)
> @@ -27,6 +28,9 @@ adox: CFLAGS=-O2
> run-test-i386-ssse3: QEMU_OPTS += -cpu max
> run-plugin-test-i386-ssse3-%: QEMU_OPTS += -cpu max
>
> +cross-modifying-code: CFLAGS+=-pthread
> +cross-modifying-code: LDFLAGS+=-pthread
> +
> test-x86_64: LDFLAGS+=-lm -lc
> test-x86_64: test-i386.c test-i386.h test-i386-shift.h test-i386-muldiv.h
> $(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
With this change,
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>