Re: [PATCH v2 07/20] tests/tcg/x86_64: Add cross-modifying code test

On 10/22/24 03:56, Alex Bennée wrote: > From: Ilya Leoshkevich <iii(a)linux.ibm.com&gt; > > 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(a)linux.ibm.com&gt; > Message-Id: <20241001150617.9977-1-iii(a)linux.ibm.com&gt; > Signed-off-by: Alex Bennée <alex.bennee(a)linaro.org&gt; > --- >   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.