Pierrick Bouvier <pierrick.bouvier@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxxxxx>
>>>> Message-Id: <20241001150617.9977-1-iii@xxxxxxxxxxxxx>
>>>> Signed-off-by: Alex Bennée <alex.bennee@xxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxx>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
