[PULL 0/4] last minute tcg fixes

[PULL 0/4] last minute tcg fixes

[Richard Henderson]

The following changes since commit 51a6dc9d394098e8f4141fad869a1ee9585f54f8:

  Merge tag 'pull-target-arm-20230403' of https://git.linaro.org/people/pmaydell/qemu-arm into staging (2023-04-03 17:01:47 +0100)

are available in the Git repository at:

  https://gitlab.com/rth7680/qemu.git tags/pull-tcg-20230404

for you to fetch changes up to 1ffbe5d681b06ea95b1728fc556899f63834553a:

  tcg/sparc64: Disable direct linking for goto_tb (2023-04-04 08:43:04 -0700)

----------------------------------------------------------------
Revert "linux-user/arm: Take more care allocating commpage"
accel/tcg: Fix jump cache set in cpu_exec_loop
accel/tcg: Fix initialization of CF_PCREL in tcg_cflags
tcg/sparc64: Disable direct jumps from goto_tb

----------------------------------------------------------------
Richard Henderson (3):
      Revert "linux-user/arm: Take more care allocating commpage"
      accel/tcg: Fix jump cache set in cpu_exec_loop
      tcg/sparc64: Disable direct linking for goto_tb

Weiwei Li (1):
      accel/tcg: Fix overwrite problems of tcg_cflags

 accel/tcg/cpu-exec.c         | 17 +++++++++++++----
 accel/tcg/tcg-accel-ops.c    |  2 +-
 linux-user/elfload.c         | 37 ++++++++++---------------------------
 tcg/sparc64/tcg-target.c.inc | 30 ++++--------------------------
 4 files changed, 28 insertions(+), 58 deletions(-)

[PULL 1/4] Revert "linux-user/arm: Take more care allocating commpage"

[Richard Henderson]

This reverts commit 4f5c67f8df7f26e559509c68c45e652709edd23f.

This exposes bugs in target_mmap et al with respect to overflow
with the final page of the guest address space.  To be fixed in
the next development cycle.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/elfload.c | 37 ++++++++++---------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index b96b3e566b..f1370a7a8b 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -423,32 +423,12 @@ enum {
 
 static bool init_guest_commpage(void)
 {
-    ARMCPU *cpu = ARM_CPU(thread_cpu);
-    abi_ptr want = HI_COMMPAGE & TARGET_PAGE_MASK;
-    abi_ptr addr;
+    abi_ptr commpage = HI_COMMPAGE & -qemu_host_page_size;
+    void *want = g2h_untagged(commpage);
+    void *addr = mmap(want, qemu_host_page_size, PROT_READ | PROT_WRITE,
+                      MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
 
-    /*
-     * M-profile allocates maximum of 2GB address space, so can never
-     * allocate the commpage.  Skip it.
-     */
-    if (arm_feature(&cpu->env, ARM_FEATURE_M)) {
-        return true;
-    }
-
-    /*
-     * If reserved_va does not cover the commpage, we get an assert
-     * in page_set_flags.  Produce an intelligent error instead.
-     */
-    if (reserved_va != 0 && want + TARGET_PAGE_SIZE - 1 > reserved_va) {
-        error_report("Allocating guest commpage: -R 0x%" PRIx64 " too small",
-                     (uint64_t)reserved_va + 1);
-        exit(EXIT_FAILURE);
-    }
-
-    addr = target_mmap(want, TARGET_PAGE_SIZE, PROT_READ | PROT_WRITE,
-                       MAP_ANONYMOUS | MAP_PRIVATE | MAP_FIXED, -1, 0);
-
-    if (addr == -1) {
+    if (addr == MAP_FAILED) {
         perror("Allocating guest commpage");
         exit(EXIT_FAILURE);
     }
@@ -457,12 +437,15 @@ static bool init_guest_commpage(void)
     }
 
     /* Set kernel helper versions; rest of page is 0.  */
-    put_user_u32(5, 0xffff0ffcu);
+    __put_user(5, (uint32_t *)g2h_untagged(0xffff0ffcu));
 
-    if (target_mprotect(addr, qemu_host_page_size, PROT_READ | PROT_EXEC)) {
+    if (mprotect(addr, qemu_host_page_size, PROT_READ)) {
         perror("Protecting guest commpage");
         exit(EXIT_FAILURE);
     }
+
+    page_set_flags(commpage, commpage | ~qemu_host_page_mask,
+                   PAGE_READ | PAGE_EXEC | PAGE_VALID);
     return true;
 }
 
-- 
2.34.1

[PULL 2/4] accel/tcg: Fix overwrite problems of tcg_cflags

[Richard Henderson]

From: Weiwei Li <liweiwei@iscas.ac.cn>

CPUs often set CF_PCREL in tcg_cflags before qemu_init_vcpu(), in which
tcg_cflags will be overwrited by tcg_cpu_init_cflags().

Fixes: 4be790263ffc ("accel/tcg: Replace `TARGET_TB_PCREL` with `CF_PCREL`")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
Message-Id: <20230331150609.114401-6-liweiwei@iscas.ac.cn>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/tcg-accel-ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
index af35e0d092..58c8e64096 100644
--- a/accel/tcg/tcg-accel-ops.c
+++ b/accel/tcg/tcg-accel-ops.c
@@ -59,7 +59,7 @@ void tcg_cpu_init_cflags(CPUState *cpu, bool parallel)
 
     cflags |= parallel ? CF_PARALLEL : 0;
     cflags |= icount_enabled() ? CF_USE_ICOUNT : 0;
-    cpu->tcg_cflags = cflags;
+    cpu->tcg_cflags |= cflags;
 }
 
 void tcg_cpus_destroy(CPUState *cpu)
-- 
2.34.1

[PULL 3/4] accel/tcg: Fix jump cache set in cpu_exec_loop

[Richard Henderson]

Assign pc and use store_release to assign tb.

Fixes: 2dd5b7a1b91 ("accel/tcg: Move jmp-cache `CF_PCREL` checks to caller")
Reported-by: Weiwei Li <liweiwei@iscas.ac.cn>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cpu-exec.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index c815f2dbfd..8370c92c05 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -257,7 +257,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
 
     if (cflags & CF_PCREL) {
         /* Use acquire to ensure current load of pc from jc. */
-        tb =  qatomic_load_acquire(&jc->array[hash].tb);
+        tb = qatomic_load_acquire(&jc->array[hash].tb);
 
         if (likely(tb &&
                    jc->array[hash].pc == pc &&
@@ -272,7 +272,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
             return NULL;
         }
         jc->array[hash].pc = pc;
-        /* Use store_release on tb to ensure pc is written first. */
+        /* Ensure pc is written first. */
         qatomic_store_release(&jc->array[hash].tb, tb);
     } else {
         /* Use rcu_read to ensure current load of pc from *tb. */
@@ -971,18 +971,27 @@ cpu_exec_loop(CPUState *cpu, SyncClocks *sc)
 
             tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
             if (tb == NULL) {
+                CPUJumpCache *jc;
                 uint32_t h;
 
                 mmap_lock();
                 tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
                 mmap_unlock();
+
                 /*
                  * We add the TB in the virtual pc hash table
                  * for the fast lookup
                  */
                 h = tb_jmp_cache_hash_func(pc);
-                /* Use the pc value already stored in tb->pc. */
-                qatomic_set(&cpu->tb_jmp_cache->array[h].tb, tb);
+                jc = cpu->tb_jmp_cache;
+                if (cflags & CF_PCREL) {
+                    jc->array[h].pc = pc;
+                    /* Ensure pc is written first. */
+                    qatomic_store_release(&jc->array[h].tb, tb);
+                } else {
+                    /* Use the pc value already stored in tb->pc. */
+                    qatomic_set(&jc->array[h].tb, tb);
+                }
             }
 
 #ifndef CONFIG_USER_ONLY
-- 
2.34.1

[PULL 4/4] tcg/sparc64: Disable direct linking for goto_tb

[Richard Henderson]

Something is wrong with this code, and also wrong with gdb on the
sparc systems to which I have access, so I cannot debug it either.
Disable for now, so the release is not broken.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/sparc64/tcg-target.c.inc | 30 ++++--------------------------
 1 file changed, 4 insertions(+), 26 deletions(-)

diff --git a/tcg/sparc64/tcg-target.c.inc b/tcg/sparc64/tcg-target.c.inc
index ccc4144f7c..694f2b9dd4 100644
--- a/tcg/sparc64/tcg-target.c.inc
+++ b/tcg/sparc64/tcg-target.c.inc
@@ -1445,12 +1445,12 @@ static void tcg_out_goto_tb(TCGContext *s, int which)
 {
     ptrdiff_t off = tcg_tbrel_diff(s, (void *)get_jmp_target_addr(s, which));
 
-    /* Direct branch will be patched by tb_target_set_jmp_target. */
+    /* Load link and indirect branch. */
     set_jmp_insn_offset(s, which);
-    tcg_out32(s, CALL);
-    /* delay slot */
-    tcg_debug_assert(check_fit_ptr(off, 13));
     tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_TB, TCG_REG_TB, off);
+    tcg_out_arithi(s, TCG_REG_G0, TCG_REG_TB, 0, JMPL);
+    /* delay slot */
+    tcg_out_nop(s);
     set_jmp_reset_offset(s, which);
 
     /*
@@ -1469,28 +1469,6 @@ static void tcg_out_goto_tb(TCGContext *s, int which)
 void tb_target_set_jmp_target(const TranslationBlock *tb, int n,
                               uintptr_t jmp_rx, uintptr_t jmp_rw)
 {
-    uintptr_t addr = tb->jmp_target_addr[n];
-    intptr_t br_disp = (intptr_t)(addr - jmp_rx) >> 2;
-    tcg_insn_unit insn;
-
-    br_disp >>= 2;
-    if (check_fit_ptr(br_disp, 19)) {
-        /* ba,pt %icc, addr */
-        insn = deposit32(INSN_OP(0) | INSN_OP2(1) | INSN_COND(COND_A)
-                         | BPCC_ICC | BPCC_PT, 0, 19, br_disp);
-    } else if (check_fit_ptr(br_disp, 22)) {
-        /* ba addr */
-        insn = deposit32(INSN_OP(0) | INSN_OP2(2) | INSN_COND(COND_A),
-                         0, 22, br_disp);
-    } else {
-        /* The code_gen_buffer can't be larger than 2GB.  */
-        tcg_debug_assert(check_fit_ptr(br_disp, 30));
-        /* call addr */
-        insn = deposit32(CALL, 0, 30, br_disp);
-    }
-
-    qatomic_set((uint32_t *)jmp_rw, insn);
-    flush_idcache_range(jmp_rx, jmp_rw, 4);
 }
 
 static void tcg_out_op(TCGContext *s, TCGOpcode opc,
-- 
2.34.1

Re: [PULL 0/4] last minute tcg fixes

[Peter Maydell]

On Tue, 4 Apr 2023 at 16:52, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The following changes since commit 51a6dc9d394098e8f4141fad869a1ee9585f54f8:
>
>   Merge tag 'pull-target-arm-20230403' of https://git.linaro.org/people/pmaydell/qemu-arm into staging (2023-04-03 17:01:47 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/rth7680/qemu.git tags/pull-tcg-20230404
>
> for you to fetch changes up to 1ffbe5d681b06ea95b1728fc556899f63834553a:
>
>   tcg/sparc64: Disable direct linking for goto_tb (2023-04-04 08:43:04 -0700)
>
> ----------------------------------------------------------------
> Revert "linux-user/arm: Take more care allocating commpage"
> accel/tcg: Fix jump cache set in cpu_exec_loop
> accel/tcg: Fix initialization of CF_PCREL in tcg_cflags
> tcg/sparc64: Disable direct jumps from goto_tb
>
> ----------------------------------------------------------------


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/8.0
for any user-visible changes.

-- PMM