| Message ID | 20220902174637.1174258-1-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tests/tcg/x86_64: add cross-modifying code test | expand |
Ilya Leoshkevich <iii@linux.ibm.com> writes: > 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> > --- > tests/tcg/x86_64/Makefile.target | 6 +- > tests/tcg/x86_64/cross-modifying-code.c | 80 +++++++++++++++++++++++++ > 2 files changed, 85 insertions(+), 1 deletion(-) > create mode 100644 tests/tcg/x86_64/cross-modifying-code.c > > diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target > index b71a6bcd5e..58e7bfd681 100644 > --- a/tests/tcg/x86_64/Makefile.target > +++ b/tests/tcg/x86_64/Makefile.target > @@ -10,6 +10,7 @@ include $(SRC_PATH)/tests/tcg/i386/Makefile.target > > ifeq ($(filter %-linux-user, $(TARGET)),$(TARGET)) > X86_64_TESTS += vsyscall > +X86_64_TESTS += cross-modifying-code > TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64 > else > TESTS=$(MULTIARCH_TESTS) > @@ -20,5 +21,8 @@ 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) > > -vsyscall: $(SRC_PATH)/tests/tcg/x86_64/vsyscall.c > +%: $(SRC_PATH)/tests/tcg/x86_64/%.c > $(CC) $(CFLAGS) $< -o $@ $(LDFLAGS) You shouldn't need to redefine the default rule when you can tweak the flags > + > +smc: CFLAGS+=-pthread > +smc: LDFLAGS+=-pthread I think this must be from an old iteration because: make[1]: Entering directory '/home/alex/lsrc/qemu.git/builds/all/tests/tcg/x86_64-linux-user' cc -Wall -Werror -O0 -g -fno-strict-aliasing /home/alex/lsrc/qemu.git/tests/tcg/x86_64/cross-modifying-code.c -o cross-modifying-code -static /usr/bin/ld: /tmp/ccK05RAk.o: in function `main': /home/alex/lsrc/qemu.git/tests/tcg/x86_64/cross-modifying-code.c:64: undefined reference to `pthread_create' /usr/bin/ld: /home/alex/lsrc/qemu.git/tests/tcg/x86_64/cross-modifying-code.c:73: undefined reference to `pthread_join' collect2: error: ld returned 1 exit status make[1]: *** [/home/alex/lsrc/qemu.git/tests/tcg/x86_64/Makefile.target:25: cross-modifying-code] Error 1 make[1]: Leaving directory '/home/alex/lsrc/qemu.git/builds/all/tests/tcg/x86_64-linux-user' make: *** [/home/alex/lsrc/qemu.git/tests/Makefile.include:52: build-tcg-tests-x86_64-linux-user] Error 2 > 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(); > + } > + } > +} > + > +#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; > +}
On Sat, 2022-09-03 at 10:13 +0100, Alex Bennée wrote: > > Ilya Leoshkevich <iii@linux.ibm.com> writes: > > > 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> > > --- > > tests/tcg/x86_64/Makefile.target | 6 +- > > tests/tcg/x86_64/cross-modifying-code.c | 80 > > +++++++++++++++++++++++++ > > 2 files changed, 85 insertions(+), 1 deletion(-) > > create mode 100644 tests/tcg/x86_64/cross-modifying-code.c > > > > diff --git a/tests/tcg/x86_64/Makefile.target > > b/tests/tcg/x86_64/Makefile.target > > index b71a6bcd5e..58e7bfd681 100644 > > --- a/tests/tcg/x86_64/Makefile.target > > +++ b/tests/tcg/x86_64/Makefile.target > > @@ -10,6 +10,7 @@ include > > $(SRC_PATH)/tests/tcg/i386/Makefile.target > > > > ifeq ($(filter %-linux-user, $(TARGET)),$(TARGET)) > > X86_64_TESTS += vsyscall > > +X86_64_TESTS += cross-modifying-code > > TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64 > > else > > TESTS=$(MULTIARCH_TESTS) > > @@ -20,5 +21,8 @@ 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) > > > > -vsyscall: $(SRC_PATH)/tests/tcg/x86_64/vsyscall.c > > +%: $(SRC_PATH)/tests/tcg/x86_64/%.c > > $(CC) $(CFLAGS) $< -o $@ $(LDFLAGS) > > You shouldn't need to redefine the default rule when you can tweak > the flags Without this rule, I get: make[1]: *** No rule to make target 'vsyscall', needed by 'all'. Stop. I think this is because the default rule has %.c as a dependency, and we run from a different directory here. > > + > > +smc: CFLAGS+=-pthread > > +smc: LDFLAGS+=-pthread > > I think this must be from an old iteration because: > > make[1]: Entering directory > '/home/alex/lsrc/qemu.git/builds/all/tests/tcg/x86_64-linux-user' > cc -Wall -Werror -O0 -g -fno-strict-aliasing > /home/alex/lsrc/qemu.git/tests/tcg/x86_64/cross-modifying-code.c -o > cross-modifying-code -static > /usr/bin/ld: /tmp/ccK05RAk.o: in function `main': > /home/alex/lsrc/qemu.git/tests/tcg/x86_64/cross-modifying-code.c:64: > undefined reference to `pthread_create' > /usr/bin/ld: /home/alex/lsrc/qemu.git/tests/tcg/x86_64/cross- > modifying-code.c:73: undefined reference to `pthread_join' > collect2: error: ld returned 1 exit status > make[1]: *** > [/home/alex/lsrc/qemu.git/tests/tcg/x86_64/Makefile.target:25: cross- > modifying-code] Error 1 > make[1]: Leaving directory > '/home/alex/lsrc/qemu.git/builds/all/tests/tcg/x86_64-linux-user' > make: *** [/home/alex/lsrc/qemu.git/tests/Makefile.include:52: build- > tcg-tests-x86_64-linux-user] Error 2 Sorry about that, I should have tested a clean build. I will send a v2. Best regards, Ilya
diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target index b71a6bcd5e..58e7bfd681 100644 --- a/tests/tcg/x86_64/Makefile.target +++ b/tests/tcg/x86_64/Makefile.target @@ -10,6 +10,7 @@ include $(SRC_PATH)/tests/tcg/i386/Makefile.target ifeq ($(filter %-linux-user, $(TARGET)),$(TARGET)) X86_64_TESTS += vsyscall +X86_64_TESTS += cross-modifying-code TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64 else TESTS=$(MULTIARCH_TESTS) @@ -20,5 +21,8 @@ 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) -vsyscall: $(SRC_PATH)/tests/tcg/x86_64/vsyscall.c +%: $(SRC_PATH)/tests/tcg/x86_64/%.c $(CC) $(CFLAGS) $< -o $@ $(LDFLAGS) + +smc: CFLAGS+=-pthread +smc: LDFLAGS+=-pthread 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(); + } + } +} + +#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; +}
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> --- tests/tcg/x86_64/Makefile.target | 6 +- tests/tcg/x86_64/cross-modifying-code.c | 80 +++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 tests/tcg/x86_64/cross-modifying-code.c