| Message ID | 20250318213209.2579218-4-richard.henderson@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | accel/tcg, codebase: Build once patches | expand |
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~
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>
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"
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(-)