diff mbox series

[PULL,07/11] tests/tcg/aarch64: Add testcases for IC IVAU and dual-mapped code

Message ID 20230704163634.3188465-8-peter.maydell@linaro.org
State Not Applicable
Headers show
Series [PULL,01/11] target/arm: Add raw_writes ops for register whose write induce TLB maintenance | expand

Commit Message

Peter Maydell July 4, 2023, 4:36 p.m. UTC
From: John Högberg <john.hogberg@ericsson.com>

https://gitlab.com/qemu-project/qemu/-/issues/1034

Signed-off-by: John Högberg <john.hogberg@ericsson.com>
Message-id: 168778890374.24232.3402138851538068785-2@git.sr.ht
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
[PMM: fixed typo in comment]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/tcg/aarch64/icivau.c        | 189 ++++++++++++++++++++++++++++++
 tests/tcg/aarch64/Makefile.target |   3 +-
 2 files changed, 191 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/aarch64/icivau.c

Comments

Richard Henderson July 5, 2023, 4:53 a.m. UTC | #1
On 7/4/23 18:36, Peter Maydell wrote:
> +int main(int argc, char **argv)
> +{
> +    const char *shm_name = "qemu-test-tcg-aarch64-icivau";
> +    int fd;
> +
> +    fd = shm_open(shm_name, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);

Build failures:

https://gitlab.com/qemu-project/qemu/-/jobs/4592433393#L3958
https://gitlab.com/qemu-project/qemu/-/jobs/4592433395#L4149
https://gitlab.com/qemu-project/qemu/-/jobs/4592433400#L3694


/usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: 
/usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/lib/../lib/librt.a(shm_open.o): 
in function `shm_open':
(.text+0x3c): undefined reference to `__shm_directory'
/usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: 
(.text+0xcc): undefined reference to `pthread_setcancelstate'
/usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: 
(.text+0xfc): undefined reference to `pthread_setcancelstate'
/usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: 
/usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/lib/../lib/librt.a(shm_unlink.o): 
in function `shm_unlink':
(.text+0x30): undefined reference to `__shm_directory'
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:119: icivau] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [/builds/qemu-project/qemu/tests/Makefile.include:50: 
build-tcg-tests-aarch64-linux-user] Error 2

It looks like this test needs something else.



r~
Philippe Mathieu-Daudé July 5, 2023, 9:25 a.m. UTC | #2
Cc'ing John.

On 5/7/23 06:53, Richard Henderson wrote:
> On 7/4/23 18:36, Peter Maydell wrote:
>> +int main(int argc, char **argv)
>> +{
>> +    const char *shm_name = "qemu-test-tcg-aarch64-icivau";
>> +    int fd;
>> +
>> +    fd = shm_open(shm_name, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);
> 
> Build failures:
> 
> https://gitlab.com/qemu-project/qemu/-/jobs/4592433393#L3958
> https://gitlab.com/qemu-project/qemu/-/jobs/4592433395#L4149
> https://gitlab.com/qemu-project/qemu/-/jobs/4592433400#L3694
> 
> 
> /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/lib/../lib/librt.a(shm_open.o): in function `shm_open':
> (.text+0x3c): undefined reference to `__shm_directory'
> /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: (.text+0xcc): undefined reference to `pthread_setcancelstate'
> /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: (.text+0xfc): undefined reference to `pthread_setcancelstate'
> /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/lib/../lib/librt.a(shm_unlink.o): in function `shm_unlink':
> (.text+0x30): undefined reference to `__shm_directory'
> collect2: error: ld returned 1 exit status
> make[1]: *** [Makefile:119: icivau] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [/builds/qemu-project/qemu/tests/Makefile.include:50: 
> build-tcg-tests-aarch64-linux-user] Error 2
> 
> It looks like this test needs something else.

Maybe:

icivau: LDFLAGS+=-lrt -pthread

?
Richard Henderson July 5, 2023, 1:36 p.m. UTC | #3
On 7/5/23 11:25, Philippe Mathieu-Daudé wrote:
>> /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: 
>> /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/lib/../lib/librt.a(shm_open.o): in function `shm_open':
>> (.text+0x3c): undefined reference to `__shm_directory'
>> /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: 
>> (.text+0xcc): undefined reference to `pthread_setcancelstate'
>> /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: 
>> (.text+0xfc): undefined reference to `pthread_setcancelstate'
>> /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: 
>> /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/lib/../lib/librt.a(shm_unlink.o): in function `shm_unlink':
>> (.text+0x30): undefined reference to `__shm_directory'
>> collect2: error: ld returned 1 exit status
>> make[1]: *** [Makefile:119: icivau] Error 1
>> make[1]: *** Waiting for unfinished jobs....
>> make: *** [/builds/qemu-project/qemu/tests/Makefile.include:50: 
>> build-tcg-tests-aarch64-linux-user] Error 2
>>
>> It looks like this test needs something else.
> 
> Maybe:
> 
> icivau: LDFLAGS+=-lrt -pthread

Yes, that's the ticket I'm sure.
Today is a travel day, so I'll still let someone else re-test.


r~
Peter Maydell July 6, 2023, 12:26 p.m. UTC | #4
On Wed, 5 Jul 2023 at 10:25, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Cc'ing John.
>
> On 5/7/23 06:53, Richard Henderson wrote:
> > On 7/4/23 18:36, Peter Maydell wrote:
> >> +int main(int argc, char **argv)
> >> +{
> >> +    const char *shm_name = "qemu-test-tcg-aarch64-icivau";
> >> +    int fd;
> >> +
> >> +    fd = shm_open(shm_name, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);
> >
> > Build failures:
> >
> > https://gitlab.com/qemu-project/qemu/-/jobs/4592433393#L3958
> > https://gitlab.com/qemu-project/qemu/-/jobs/4592433395#L4149
> > https://gitlab.com/qemu-project/qemu/-/jobs/4592433400#L3694
> >
> >
> > /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/lib/../lib/librt.a(shm_open.o): in function `shm_open':
> > (.text+0x3c): undefined reference to `__shm_directory'
> > /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: (.text+0xcc): undefined reference to `pthread_setcancelstate'
> > /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: (.text+0xfc): undefined reference to `pthread_setcancelstate'
> > /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/bin/ld: /usr/lib/gcc-cross/aarch64-linux-gnu/10/../../../../aarch64-linux-gnu/lib/../lib/librt.a(shm_unlink.o): in function `shm_unlink':
> > (.text+0x30): undefined reference to `__shm_directory'
> > collect2: error: ld returned 1 exit status
> > make[1]: *** [Makefile:119: icivau] Error 1
> > make[1]: *** Waiting for unfinished jobs....
> > make: *** [/builds/qemu-project/qemu/tests/Makefile.include:50:
> > build-tcg-tests-aarch64-linux-user] Error 2
> >
> > It looks like this test needs something else.
>
> Maybe:
>
> icivau: LDFLAGS+=-lrt -pthread

That is enough to get it to build, but then in the CI the test
consistently fails:

https://gitlab.com/pm215/qemu/-/jobs/4606447875

TEST icivau on aarch64
make[1]: *** [Makefile:178: run-icivau] Error 1

I'm going to drop this patch from the pullreq until we
can figure out what's going on...

thanks
-- PMM
John Högberg July 6, 2023, 12:45 p.m. UTC | #5
> That is enough to get it to build, but then in the CI the test
> consistently fails:
>
> https://gitlab.com/pm215/qemu/-/jobs/4606447875
>
> TEST icivau on aarch64
> make[1]: *** [Makefile:178: run-icivau] Error 1
>
> I'm going to drop this patch from the pullreq until we
> can figure out what's going on...

Oops, -pthread wasn't required on my machine.

I'm unable to reproduce the failure locally. Is it possible to strace it
and see whether it was caused by failing to set up dual-mapped code?

/John
Peter Maydell July 6, 2023, 12:54 p.m. UTC | #6
On Thu, 6 Jul 2023 at 13:45, John Högberg <john.hogberg@ericsson.com> wrote:
>
> > That is enough to get it to build, but then in the CI the test
> > consistently fails:
> >
> > https://gitlab.com/pm215/qemu/-/jobs/4606447875
> >
> > TEST icivau on aarch64
> > make[1]: *** [Makefile:178: run-icivau] Error 1
> >
> > I'm going to drop this patch from the pullreq until we
> > can figure out what's going on...
>
> Oops, -pthread wasn't required on my machine.
>
> I'm unable to reproduce the failure locally. Is it possible to strace it
> and see whether it was caused by failing to set up dual-mapped code?

On the CI machines all you get is what the test case (and
whatever the makefile chooses to wrap it in) writes
to stdout/stderr.

thanks
-- PMM
John Högberg July 7, 2023, 7:50 a.m. UTC | #7
Alright, thanks. Where should I go from here? Should I send in another
patch that tries to debug this?

/John

-----Original Message-----
From: Peter Maydell <peter.maydell@linaro.org>
To: John Högberg <john.hogberg@ericsson.com>
Cc: philmd@linaro.org <philmd@linaro.org>, richard.henderson@linaro.org
<richard.henderson@linaro.org>, qemu-devel@nongnu.org
<qemu-devel@nongnu.org>
Subject: Re: [PULL 07/11] tests/tcg/aarch64: Add testcases for IC IVAU
and dual-mapped code
Date: Thu, 06 Jul 2023 13:54:35 +0100

On Thu, 6 Jul 2023 at 13:45, John Högberg <john.hogberg@ericsson.com>
wrote:
> 
> > That is enough to get it to build, but then in the CI the test
> > consistently fails:
> > 
> > https://gitlab.com/pm215/qemu/-/jobs/4606447875
> > 
> > TEST icivau on aarch64
> > make[1]: *** [Makefile:178: run-icivau] Error 1
> > 
> > I'm going to drop this patch from the pullreq until we
> > can figure out what's going on...
> 
> Oops, -pthread wasn't required on my machine.
> 
> I'm unable to reproduce the failure locally. Is it possible to strace
> it
> and see whether it was caused by failing to set up dual-mapped code?

On the CI machines all you get is what the test case (and
whatever the makefile chooses to wrap it in) writes
to stdout/stderr.

thanks
-- PMM
diff mbox series

Patch

diff --git a/tests/tcg/aarch64/icivau.c b/tests/tcg/aarch64/icivau.c
new file mode 100644
index 00000000000..77b9e98d5e5
--- /dev/null
+++ b/tests/tcg/aarch64/icivau.c
@@ -0,0 +1,189 @@ 
+/*
+ * Tests the IC IVAU-driven workaround for catching changes made to dual-mapped
+ * code that would otherwise go unnoticed in user mode.
+ *
+ * Copyright (c) 2023 Ericsson AB
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <string.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <fcntl.h>
+
+#define MAX_CODE_SIZE 128
+
+typedef int (SelfModTest)(uint32_t, uint32_t*);
+typedef int (BasicTest)(int);
+
+static void mark_code_modified(const uint32_t *exec_data, size_t length)
+{
+    int dc_required, ic_required;
+    unsigned long ctr_el0;
+
+    /*
+     * Clear the data/instruction cache, as indicated by the CTR_ELO.{DIC,IDC}
+     * flags.
+     *
+     * For completeness we might be tempted to assert that we should fail when
+     * the whole code update sequence is omitted, but that would make the test
+     * flaky as it can succeed by coincidence on actual hardware.
+     */
+    asm ("mrs %0, ctr_el0\n" : "=r"(ctr_el0));
+
+    /* CTR_EL0.IDC */
+    dc_required = !((ctr_el0 >> 28) & 1);
+
+    /* CTR_EL0.DIC */
+    ic_required = !((ctr_el0 >> 29) & 1);
+
+    if (dc_required) {
+        size_t dcache_stride, i;
+
+        /*
+         * Step according to the minimum cache size, as the cache maintenance
+         * instructions operate on the cache line of the given address.
+         *
+         * We assume that exec_data is properly aligned.
+         */
+        dcache_stride = (4 << ((ctr_el0 >> 16) & 0xF));
+
+        for (i = 0; i < length; i += dcache_stride) {
+            const char *dc_addr = &((const char *)exec_data)[i];
+            asm volatile ("dc cvau, %x[dc_addr]\n"
+                          : /* no outputs */
+                          : [dc_addr] "r"(dc_addr)
+                          : "memory");
+        }
+
+        asm volatile ("dmb ish\n");
+    }
+
+    if (ic_required) {
+        size_t icache_stride, i;
+
+        icache_stride = (4 << (ctr_el0 & 0xF));
+
+        for (i = 0; i < length; i += icache_stride) {
+            const char *ic_addr = &((const char *)exec_data)[i];
+            asm volatile ("ic ivau, %x[ic_addr]\n"
+                          : /* no outputs */
+                          : [ic_addr] "r"(ic_addr)
+                          : "memory");
+        }
+
+        asm volatile ("dmb ish\n");
+    }
+
+    asm volatile ("isb sy\n");
+}
+
+static int basic_test(uint32_t *rw_data, const uint32_t *exec_data)
+{
+    /*
+     * As user mode only misbehaved for dual-mapped code when previously
+     * translated code had been changed, we'll start off with this basic test
+     * function to ensure that there's already some translated code at
+     * exec_data before the next test. This should cause the next test to fail
+     * if `mark_code_modified` fails to invalidate the code.
+     *
+     * Note that the payload is in binary form instead of inline assembler
+     * because we cannot use __attribute__((naked)) on this platform and the
+     * workarounds are at least as ugly as this is.
+     */
+    static const uint32_t basic_payload[] = {
+        0xD65F03C0 /* 0x00: RET */
+    };
+
+    BasicTest *copied_ptr = (BasicTest *)exec_data;
+
+    memcpy(rw_data, basic_payload, sizeof(basic_payload));
+    mark_code_modified(exec_data, sizeof(basic_payload));
+
+    return copied_ptr(1234) == 1234;
+}
+
+static int self_modification_test(uint32_t *rw_data, const uint32_t *exec_data)
+{
+    /*
+     * This test is self-modifying in an attempt to cover an edge case where
+     * the IC IVAU instruction invalidates itself.
+     *
+     * Note that the IC IVAU instruction is 16 bytes into the function, in what
+     * will be the same cache line as the modified instruction on machines with
+     * a cache line size >= 16 bytes.
+     */
+    static const uint32_t self_mod_payload[] = {
+        /* Overwrite the placeholder instruction with the new one. */
+        0xB9001C20, /* 0x00: STR w0, [x1, 0x1C] */
+
+        /* Get the executable address of the modified instruction. */
+        0x100000A8, /* 0x04: ADR x8, <0x1C> */
+
+        /* Mark the modified instruction as updated. */
+        0xD50B7B28, /* 0x08: DC CVAU x8 */
+        0xD5033BBF, /* 0x0C: DMB ISH */
+        0xD50B7528, /* 0x10: IC IVAU x8 */
+        0xD5033BBF, /* 0x14: DMB ISH */
+        0xD5033FDF, /* 0x18: ISB */
+
+        /* Placeholder instruction, overwritten above. */
+        0x52800000, /* 0x1C: MOV w0, 0 */
+
+        0xD65F03C0  /* 0x20: RET */
+    };
+
+    SelfModTest *copied_ptr = (SelfModTest *)exec_data;
+    int i;
+
+    memcpy(rw_data, self_mod_payload, sizeof(self_mod_payload));
+    mark_code_modified(exec_data, sizeof(self_mod_payload));
+
+    for (i = 1; i < 10; i++) {
+        /* Replace the placeholder instruction with `MOV w0, i` */
+        uint32_t new_instr = 0x52800000 | (i << 5);
+
+        if (copied_ptr(new_instr, rw_data) != i) {
+            return 0;
+        }
+    }
+
+    return 1;
+}
+
+int main(int argc, char **argv)
+{
+    const char *shm_name = "qemu-test-tcg-aarch64-icivau";
+    int fd;
+
+    fd = shm_open(shm_name, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);
+
+    if (fd < 0) {
+        return EXIT_FAILURE;
+    }
+
+    /* Unlink early to avoid leaving garbage in case the test crashes. */
+    shm_unlink(shm_name);
+
+    if (ftruncate(fd, MAX_CODE_SIZE) == 0) {
+        const uint32_t *exec_data;
+        uint32_t *rw_data;
+
+        rw_data = mmap(0, MAX_CODE_SIZE, PROT_READ | PROT_WRITE,
+                       MAP_SHARED, fd, 0);
+        exec_data = mmap(0, MAX_CODE_SIZE, PROT_READ | PROT_EXEC,
+                         MAP_SHARED, fd, 0);
+
+        if (rw_data && exec_data) {
+            if (basic_test(rw_data, exec_data) &&
+                self_modification_test(rw_data, exec_data)) {
+                return EXIT_SUCCESS;
+            }
+        }
+    }
+
+    return EXIT_FAILURE;
+}
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 97cfc43600a..bf9d21d72fb 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -9,9 +9,10 @@  AARCH64_SRC=$(SRC_PATH)/tests/tcg/aarch64
 VPATH 		+= $(AARCH64_SRC)
 
 # Base architecture tests
-AARCH64_TESTS=fcvt pcalign-a64
+AARCH64_TESTS=fcvt pcalign-a64 icivau
 
 fcvt: LDFLAGS+=-lm
+icivau: LDFLAGS+=-lrt
 
 run-fcvt: fcvt
 	$(call run-test,$<,$(QEMU) $<, "$< on $(TARGET_NAME)")