| Message ID | 20241022105614.839199-8-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | maintainer updates (testing, gdbstub, plugins) | expand |
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>
On Tue, 2024-10-22 at 13:36 -0700, Pierrick Bouvier wrote: > 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. Is there an easy way to include glib from testcases? It's located using meson, and I can't immediately see how to push the respective compiler flags to the test Makefiles - this seems to be currently handled by configure writing to $config_target_mak. [...]
On 10/22/24 17:16, Ilya Leoshkevich wrote: > On Tue, 2024-10-22 at 13:36 -0700, Pierrick Bouvier wrote: >> 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. > > Is there an easy way to include glib from testcases? > It's located using meson, and I can't immediately see how to push the > respective compiler flags to the test Makefiles - this seems to be > currently handled by configure writing to $config_target_mak. > > [...] > Sorry you're right, I missed the fact tests don't have the deps we have in QEMU itself. I don't think any test case include any extra dependency for now (and would make it hard to cross compile them too), so it's not worth trying to get the right glib header for this. I don't now if it will be a problem when merging the series regarding checkpatch, but if it is, we can always replace this by abort, or exit. > As it is, Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > On 10/22/24 17:16, Ilya Leoshkevich wrote: >> On Tue, 2024-10-22 at 13:36 -0700, Pierrick Bouvier wrote: >>> 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. >> Is there an easy way to include glib from testcases? >> It's located using meson, and I can't immediately see how to push the >> respective compiler flags to the test Makefiles - this seems to be >> currently handled by configure writing to $config_target_mak. >> [...] >> > > Sorry you're right, I missed the fact tests don't have the deps we > have in QEMU itself. > I don't think any test case include any extra dependency for now (and > would make it hard to cross compile them too), so it's not worth > trying to get the right glib header for this. No we only have glibc for test cases. > > I don't now if it will be a problem when merging the series regarding > checkpatch, but if it is, we can always replace this by abort, or > exit. Its a false positive in this case. We could tech checkpatch not to care about glib-isms in tests/tcg but that would probaly make keeping it in sync with the kernel version harder. > >> > > As it is, > Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
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; +} 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)