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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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>