These routines are buggy in multiple ways:
  - Use of target-endian loads, then a bswap that
    depends on the host endiannness.
  - 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.henderson@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"
-- 
2.43.0

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.
>    - 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.henderson@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"

Already given with my first question, but more clear to repeat it here:

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

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.henderson@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.bouvier@linaro.org>

On 3/18/25 16:52, Pierrick Bouvier wrote:
> 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)?

No, it was due to ...

>> -    ret = lduw_p(haddr);
>> -    ret = ldl_p(haddr);
>> -    ret = ldq_p(haddr);

... these being target-endian macros.

What was intended, once upon a time, was ldl_he_p etc,
so that the load was host-endian.  But using the atomicity
routines is even better.


r~

On 3/18/25 18:05, Richard Henderson wrote:
> On 3/18/25 16:52, Pierrick Bouvier wrote:
>> 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)?
> 
> No, it was due to ...
> 
>>> -    ret = lduw_p(haddr);
>>> -    ret = ldl_p(haddr);
>>> -    ret = ldq_p(haddr);
> 
> ... these being target-endian macros.
> 
> What was intended, once upon a time, was ldl_he_p etc,
> so that the load was host-endian.  But using the atomicity
> routines is even better.
> 

Oh right, I missed the load_atom_* for size > 1, as I was looking at 
do_ld1_mmu, which uses ldub_p.

Thanks

> 
> r~