* [PATCH] tests/tcg/x86_64: add cross-modifying code test
@ 2022-09-02 17:46 Ilya Leoshkevich
2022-09-03 9:13 ` Alex Bennée
0 siblings, 1 reply; 3+ messages in thread
From: Ilya Leoshkevich @ 2022-09-02 17:46 UTC (permalink / raw)
To: Paolo Bonzini, Richard Henderson, Eduardo Habkost
Cc: qemu-devel, Ilya Leoshkevich
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)
+
+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;
+}
--
2.37.2
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] tests/tcg/x86_64: add cross-modifying code test
2022-09-02 17:46 [PATCH] tests/tcg/x86_64: add cross-modifying code test Ilya Leoshkevich
@ 2022-09-03 9:13 ` Alex Bennée
2022-09-05 15:44 ` Ilya Leoshkevich
0 siblings, 1 reply; 3+ messages in thread
From: Alex Bennée @ 2022-09-03 9:13 UTC (permalink / raw)
To: Ilya Leoshkevich
Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost, qemu-devel
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;
> +}
--
Alex Bennée
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] tests/tcg/x86_64: add cross-modifying code test
2022-09-03 9:13 ` Alex Bennée
@ 2022-09-05 15:44 ` Ilya Leoshkevich
0 siblings, 0 replies; 3+ messages in thread
From: Ilya Leoshkevich @ 2022-09-05 15:44 UTC (permalink / raw)
To: Alex Bennée
Cc: Paolo Bonzini, Richard Henderson, Eduardo Habkost, qemu-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-09-05 15:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-02 17:46 [PATCH] tests/tcg/x86_64: add cross-modifying code test Ilya Leoshkevich
2022-09-03 9:13 ` Alex Bennée
2022-09-05 15:44 ` Ilya Leoshkevich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).