| Message ID | 20230704163634.3188465-8-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/11] target/arm: Add raw_writes ops for register whose write induce TLB maintenance | expand |
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~
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 ?
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~
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
> 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
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
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 --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)")