[RFC PATCH 5/8] RFC: accel/tcg: allow cross-page goto_tb chaining in user-only builds

Matt Turner <[email protected]>
Newsgroups gmane.comp.emulators.qemu
Message-ID <[email protected]>
translator_use_goto_tb() refuses to chain unless the destination is on the
same page as the start of the TB. For guests whose text is much larger than
a page this is expensive: an emulated alpha gcc compiling a 255k line
translation unit takes the indirect dispatch path for 8.4 billion of its
34.2 billion TB exits, and a large share of those are ordinary direct
branches that simply crossed an 8 KiB page boundary.

The restriction was made unconditional by d3a2a1d803 ("accel/tcg:
Introduce translator_use_goto_tb"), whose rationale was:

    Various targets avoid the page crossing test for CONFIG_USER_ONLY,
    but that is wrong: mmap and mprotect can change page permissions.

That is true, but in user-only builds the invalidation path already covers
it. There are no page tables: every mmap, mprotect and munmap reaches
page_set_flags(), which calls tb_invalidate_phys_range() whenever the flags
actually change, and tb_phys_invalidate() calls tb_jmp_unlink() to reset
incoming jumps. A chained cross-page jump is therefore broken whenever the
destination page's permissions change. This is not true in system mode,
where TBs are keyed by physical address and a page table change invalidates
nothing, so the restriction is kept there.

Add tests/tcg/alpha/test-xpage-chain.c to cover the hazard directly. It
places a direct branch near the end of one page targeting the next page,
runs it 200000 times so the chain is established, then checks that
mprotect(PROT_NONE) makes the next call fault, and that remapping the page
with different code runs the new code rather than a stale translation.

The test detects the hazard it is meant to detect: with the
tb_invalidate_phys_range() call in page_set_flags() commented out, it fails
both phases, executing page B after PROT_NONE and returning the stale
result.

Measured with qemu-alpha running an emulated alpha gcc 16.2.0 compiling the
SQLite 3.45.1 amalgamation on an x86-64 host, LTO build, on top of the
preceding patches:

    before: 890,713,633,237 instructions
    after:  869,178,598,378 instructions   -2.42%

    before: 84.44s wall clock
    after:  80.49s wall clock              -4.68%

Note that this is worth more in time than in instructions, the reverse of
the preceding patch: a chained jump replaces a cache probe whose loads can
miss, so the instructions it removes are more expensive than average.

Measured before the inline jump cache probe, when a missed chain cost a
helper call rather than an inline probe, the same change was worth -7.9%.

RFC because this reverses a deliberate decision and the reasoning above
wants review from someone who knows the invalidation paths better than I
do.

Signed-off-by: Matt Turner <[email protected]>
---
 accel/tcg/translator.c             |  12 ++++
 tests/tcg/alpha/Makefile.target    |   2 +-
 tests/tcg/alpha/test-xpage-chain.c | 111 +++++++++++++++++++++++++++++
 3 files changed, 124 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/alpha/test-xpage-chain.c

diff --git ./accel/tcg/translator.c ./accel/tcg/translator.c
index 29e609b2ec..4921bf978c 100644
--- ./accel/tcg/translator.c
+++ ./accel/tcg/translator.c
@@ -117,8 +117,20 @@ bool translator_use_goto_tb(DisasContextBase *db, vaddr dest)
         return false;
     }
 
+#ifdef CONFIG_USER_ONLY
+    /*
+     * There are no page tables in user-only mode.  Every mmap, mprotect and
+     * munmap goes through page_set_flags(), which calls
+     * tb_invalidate_phys_range() whenever the flags actually change, and
+     * tb_phys_invalidate() unlinks incoming jumps.  A chained cross-page
+     * jump is therefore broken whenever the destination page's permissions
+     * change, so the same-page restriction is not needed here.
+     */
+    return true;
+#else
     /* Check for the dest on the same page as the start of the TB.  */
     return translator_is_same_page(db, dest);
+#endif
 }
 
 void translator_loop(CPUState *cpu, TranslationBlock *tb, int *max_insns,
diff --git ./tests/tcg/alpha/Makefile.target ./tests/tcg/alpha/Makefile.target
index 36d8ed1eae..eee986bab6 100644
--- ./tests/tcg/alpha/Makefile.target
+++ ./tests/tcg/alpha/Makefile.target
@@ -5,7 +5,7 @@
 ALPHA_SRC=$(SRC_PATH)/tests/tcg/alpha
 VPATH+=$(ALPHA_SRC)
 
-ALPHA_TESTS=hello-alpha test-cond test-cmov test-ovf test-cvttq
+ALPHA_TESTS=hello-alpha test-cond test-cmov test-ovf test-cvttq test-xpage-chain
 TESTS+=$(ALPHA_TESTS)
 
 test-cmov: EXTRA_CFLAGS=-DTEST_CMOV
diff --git ./tests/tcg/alpha/test-xpage-chain.c ./tests/tcg/alpha/test-xpage-chain.c
new file mode 100644
index 0000000000..7916d544af
--- /dev/null
+++ ./tests/tcg/alpha/test-xpage-chain.c
@@ -0,0 +1,111 @@
+/*
+ * Cross-page TB chaining hazard test.
+ *
+ * Phase 1: a direct branch (br) near the end of page A targets page B.
+ *          Run it enough times that QEMU chains TB_A -> TB_B.
+ * Phase 2: mprotect page B away. Re-running must fault.
+ * Phase 3: remap page B with different code. Re-running must execute the
+ *          NEW code, not a stale chained translation of the old code.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <setjmp.h>
+#include <signal.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+#define PS 8192
+
+static sigjmp_buf jb;
+/*
+ * Written by the SIGSEGV handler and read by main(), so it must not be
+ * cached in a register across the faulting call.
+ */
+static volatile sig_atomic_t caught;
+
+static void segv(int sig)
+{
+    caught = 1;
+    siglongjmp(jb, 1);
+}
+
+/* lda $0, imm($31)  -> v0 = imm */
+static unsigned int lda_v0(int imm)
+{
+    return 0x201F0000u | (unsigned short)imm;
+}
+
+int main(void)
+{
+    struct sigaction sa;
+    int rc = 0;
+    unsigned char *m = mmap(NULL, 2 * PS, PROT_READ | PROT_WRITE | PROT_EXEC,
+                            MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    if (m == MAP_FAILED) {
+        perror("mmap");
+        return 2;
+    }
+
+    unsigned char *pa = m, *pb = m + PS;
+    unsigned int *entry = (unsigned int *)(pa + PS - 64);
+    unsigned int *tgt = (unsigned int *)(pb + 16);
+
+    entry[0] = lda_v0(1);
+    long disp = ((long)tgt - ((long)&entry[1] + 4)) / 4;
+    entry[1] = 0xC3E00000u | (unsigned int)(disp & 0x1FFFFF);  /* br $31,tgt */
+    tgt[0] = 0x6BFA8001u;                                      /* ret        */
+    __builtin___clear_cache((char *)m, (char *)m + 2 * PS);
+
+    long (*fn)(void) = (long (*)(void))entry;
+
+    for (int i = 0; i < 200000; i++) {
+        if (fn() != 1) {
+            printf("FAIL: phase 1 wrong result\n");
+            return 1;
+        }
+    }
+    printf("phase 1 ok (chained)\n");
+
+    memset(&sa, 0, sizeof(sa));
+    sa.sa_handler = segv;
+    sigemptyset(&sa.sa_mask);
+    if (sigaction(SIGSEGV, &sa, NULL) != 0) {
+        perror("sigaction");
+        return 2;
+    }
+    if (mprotect(pb, PS, PROT_NONE) != 0) {
+        perror("mprotect");
+        return 2;
+    }
+    if (sigsetjmp(jb, 1) == 0) {
+        fn();
+        printf("FAIL: phase 2 executed page B after mprotect(PROT_NONE)\n");
+        rc = 1;
+    } else if (!caught) {
+        printf("FAIL: phase 2 longjmp without entering the handler\n");
+        rc = 1;
+    } else {
+        printf("phase 2 ok (faulted)\n");
+    }
+
+    /* Phase 3: remap with different code, expect the new code to run. */
+    if (mprotect(pb, PS, PROT_READ | PROT_WRITE | PROT_EXEC) != 0) {
+        perror("mprotect back");
+        return 2;
+    }
+    tgt[0] = lda_v0(2);
+    tgt[1] = 0x6BFA8001u;
+    __builtin___clear_cache((char *)pb, (char *)pb + PS);
+
+    long r = fn();
+    if (r != 2) {
+        printf("FAIL: phase 3 returned %ld, expected 2 (stale chain)\n", r);
+        rc = 1;
+    } else {
+        printf("phase 3 ok (new code ran)\n");
+    }
+    return rc;
+}
-- 
2.54.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.