Re: [PATCH v2 03/42] accel/tcg: Fix cpu_ld*_code_mmu for user mode

[Pierrick Bouvier]

On 3/18/25 14:31, Richard Henderson wrote:
These routines are buggy in multiple ways:
   - Use of target-endian loads, then a bswap that
     depends on the host endiannness.

The code is very similar to do_ld*_mmu functions, so it's subtle to notice.

Was the endianness bug due to the fact we use oi (MemOpIdx) directly 
instead of get_memop(oi) (MemOp)?
It seems to be the main difference with do_ld*_mmu functions.

   - A non-unwinding code load must set_helper_retaddr 1,
     which is magic within adjust_signal_pc.
   - cpu_ldq_code_mmu used MMU_DATA_LOAD

The bugs are hidden because all current uses of cpu_ld*_code_mmu
are from system mode.

Fixes: 2899062614a ("accel/tcg: Add cpu_ld*_code_mmu")
Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
---
  accel/tcg/user-exec.c | 41 ++++-------------------------------------
  1 file changed, 4 insertions(+), 37 deletions(-)

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 2322181b15..629a1c9ce6 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -1257,58 +1257,25 @@ uint64_t cpu_ldq_code(CPUArchState *env, abi_ptr ptr)
  uint8_t cpu_ldb_code_mmu(CPUArchState *env, abi_ptr addr,
                           MemOpIdx oi, uintptr_t ra)
  {
-    void *haddr;
-    uint8_t ret;
-
-    haddr = cpu_mmu_lookup(env_cpu(env), addr, oi, ra, MMU_INST_FETCH);
-    ret = ldub_p(haddr);
-    clear_helper_retaddr();
-    return ret;
+    return do_ld1_mmu(env_cpu(env), addr, oi, ra ? ra : 1, MMU_INST_FETCH);
  }
  
  uint16_t cpu_ldw_code_mmu(CPUArchState *env, abi_ptr addr,
                            MemOpIdx oi, uintptr_t ra)
  {
-    void *haddr;
-    uint16_t ret;
-
-    haddr = cpu_mmu_lookup(env_cpu(env), addr, oi, ra, MMU_INST_FETCH);
-    ret = lduw_p(haddr);
-    clear_helper_retaddr();
-    if (get_memop(oi) & MO_BSWAP) {
-        ret = bswap16(ret);
-    }
-    return ret;
+    return do_ld2_mmu(env_cpu(env), addr, oi, ra ? ra : 1, MMU_INST_FETCH);
  }
  
  uint32_t cpu_ldl_code_mmu(CPUArchState *env, abi_ptr addr,
                            MemOpIdx oi, uintptr_t ra)
  {
-    void *haddr;
-    uint32_t ret;
-
-    haddr = cpu_mmu_lookup(env_cpu(env), addr, oi, ra, MMU_INST_FETCH);
-    ret = ldl_p(haddr);
-    clear_helper_retaddr();
-    if (get_memop(oi) & MO_BSWAP) {
-        ret = bswap32(ret);
-    }
-    return ret;
+    return do_ld4_mmu(env_cpu(env), addr, oi, ra ? ra : 1, MMU_INST_FETCH);
  }
  
  uint64_t cpu_ldq_code_mmu(CPUArchState *env, abi_ptr addr,
                            MemOpIdx oi, uintptr_t ra)
  {
-    void *haddr;
-    uint64_t ret;
-
-    haddr = cpu_mmu_lookup(env_cpu(env), addr, oi, ra, MMU_DATA_LOAD);
-    ret = ldq_p(haddr);
-    clear_helper_retaddr();
-    if (get_memop(oi) & MO_BSWAP) {
-        ret = bswap64(ret);
-    }
-    return ret;
+    return do_ld8_mmu(env_cpu(env), addr, oi, ra ? ra : 1, MMU_INST_FETCH);
  }
  
  #include "ldst_common.c.inc"

Reviewed-by: Pierrick Bouvier <pierrick.bouv...@linaro.org>