Re: [PATCH v2 00/48] tcg patch queue

[Richard Henderson]

On 8/28/23 23:17, Bastian Koppelmann wrote:
> > I'm seeing a segfault in "make docker-test-tcg@debian-tricore-cross"
> > after this pull request.
>
> git bisect points to:
>
> commit fc15bfb6a6bda8d4d01f1383579d385acae17c0f
> Author: Anton Johansson <anjo@rev.ng>
> Date:   Mon Aug 7 17:57:03 2023 +0200
>
>      include/exec: typedef abi_ptr to vaddr in softmmu
>
>      In system mode, abi_ptr is primarily used for representing addresses
>      when accessing guest memory with cpu_[st|ld]*(). Widening it from
>      target_ulong to vaddr reduces the target dependence of these functions
>      and is step towards building accel/ once for system mode.
>
>      Signed-off-by: Anton Johansson <anjo@rev.ng>
>      Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>      Message-Id: <20230807155706.9580-7-anjo@rev.ng>
>      Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
> diff --git a/include/exec/cpu_ldst.h b/include/exec/cpu_ldst.h
> index da10ba1433..f3ce4eb1d0 100644
> --- a/include/exec/cpu_ldst.h
> +++ b/include/exec/cpu_ldst.h
> @@ -121,8 +121,8 @@ static inline bool guest_range_valid_untagged(abi_ulong 
> start, abi_ulong len)
>       h2g_nocheck(x); \
>   })
>   #else
> -typedef target_ulong abi_ptr;
> -#define TARGET_ABI_FMT_ptr TARGET_FMT_lx
> +typedef vaddr abi_ptr;
> +#define TARGET_ABI_FMT_ptr "%016" VADDR_PRIx
>   #endif
>
>
> Changeing typedef vaddr abi_ptr back to target_ulong fixes it.
>
> The faulting TriCore insn is OPC1_32_BOL_ST_W_LONGOFF, that tries to write to
> guest addr 0xd0000124, which is in a valid memory region.

Right.  The traceback is

#6  0x000055555582c511 in mmu_lookup
    (env=0x555555e98ee0, addr=18446744072904253376, oi=32, ra=0, type=MMU_DATA_STORE, 
l=0x7ffff1888cc0) at ../src/accel/tcg/cputlb.c:1828
#7  0x000055555582f531 in do_st4_mmu
    (env=0x555555e98ee0, addr=18446744072904253376, val=0, oi=32, ra=0)
    at ../src/accel/tcg/cputlb.c:2991
#8  0x000055555582fe59 in cpu_stl_mmu
    (env=0x555555e98ee0, addr=18446744072904253376, val=0, oi=32, retaddr=0)
    at ../src/accel/tcg/cputlb.c:3138
#9  0x000055555583041d in cpu_stl_le_mmuidx_ra
    (env=0x555555e98ee0, addr=18446744072904253376, val=0, mmu_idx=0, ra=0)
    at ../src/accel/tcg/ldst_common.c.inc:118
#10 0x00005555558308b5 in cpu_stl_le_data_ra
    (env=0x555555e98ee0, addr=18446744072904253376, val=0, ra=0)
    at ../src/accel/tcg/ldst_common.c.inc:213
#11 0x0000555555830bf2 in cpu_stl_le_data
    (env=0x555555e98ee0, addr=18446744072904253376, val=0)
    at ../src/accel/tcg/ldst_common.c.inc:301
#12 0x00005555557a2ce2 in save_context_upper
    (env=0x555555e98ee0, ea=-805298240)
    at ../src/target/tricore/op_helper.c:2400
#13 0x00005555557a35ee in helper_call (env=0x555555e98ee0, next_pc=2147484092)
    at ../src/target/tricore/op_helper.c:2519

With

    void save_context_upper(CPUTriCoreState *env, int ea)

providing the signed address variable, which gets extended to 64-bits with the change to 
abi_ptr instead of converted to unsigned 32-bits in cpu_stl_le_data.

For the short-term, let's revert this patch.


r~