Re: [PATCH for-11.0] tcg: Pass host-endian values to plugin_gen_mem_callbacks_*

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

Hi Richard,

On 3/24/26 5:40 PM, Richard Henderson wrote:
> If the host does not support swapped-endian loads and stores,
> then we emulate those within the tcg expanders with explicit
> bswap operations.
> 
> However, we were passing values to the plugin interface in
> the middle of those bswap operations, which meant that we
> would pass values of the wrong endianness to plugins when
> running on hosts without swapped-endian loads and stores.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3351
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> Hi Pierrick,
> 
> On IRC I expressed the opinion that there was an additional
> big-endian bug with how we treat neg.plugin_mem_value_low,
> but I now see that isn't true, because of how we adjust the
> store address in plugin_gen_mem_callbacks_i32.
>

Yes, we store the value directly swapped, so there is no further 
transformation needed.

> 
> r~
> ---
>   tcg/tcg-op-ldst.c | 52 ++++++++++++++++++++++++-----------------------
>   1 file changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/tcg/tcg-op-ldst.c b/tcg/tcg-op-ldst.c
> index 354d9968f9..22211ccb45 100644
> --- a/tcg/tcg-op-ldst.c
> +++ b/tcg/tcg-op-ldst.c
> @@ -262,9 +262,6 @@ static void tcg_gen_qemu_ld_i32_int(TCGv_i32 val, TCGTemp *addr,
>       addr_new = tci_extend_addr(addr);
>       copy_addr = plugin_maybe_preserve_addr(addr);
>       gen_ldst1(INDEX_op_qemu_ld, TCG_TYPE_I32, tcgv_i32_temp(val), addr_new, oi);
> -    plugin_gen_mem_callbacks_i32(val, copy_addr, addr, orig_oi,
> -                                 QEMU_PLUGIN_MEM_R);
> -    maybe_free_addr(addr, addr_new);
>   
>       if ((orig_memop ^ memop) & MO_BSWAP) {
>           switch (orig_memop & MO_SIZE) {
> @@ -280,6 +277,10 @@ static void tcg_gen_qemu_ld_i32_int(TCGv_i32 val, TCGTemp *addr,
>               g_assert_not_reached();
>           }
>       }
> +
> +    plugin_gen_mem_callbacks_i32(val, copy_addr, addr, orig_oi,
> +                                 QEMU_PLUGIN_MEM_R);
> +    maybe_free_addr(addr, addr_new);
>   }
>   
>   void tcg_gen_qemu_ld_i32_chk(TCGv_i32 val, TCGTemp *addr, TCGArg idx,
> @@ -290,10 +291,10 @@ void tcg_gen_qemu_ld_i32_chk(TCGv_i32 val, TCGTemp *addr, TCGArg idx,
>       tcg_gen_qemu_ld_i32_int(val, addr, idx, memop);
>   }
>   
> -static void tcg_gen_qemu_st_i32_int(TCGv_i32 val, TCGTemp *addr,
> +static void tcg_gen_qemu_st_i32_int(TCGv_i32 orig_val, TCGTemp *addr,
>                                       TCGArg idx, MemOp memop)
>   {
> -    TCGv_i32 swap = NULL;
> +    TCGv_i32 val = orig_val;
>       MemOpIdx orig_oi, oi;
>       TCGTemp *addr_new;
>   
> @@ -302,29 +303,29 @@ static void tcg_gen_qemu_st_i32_int(TCGv_i32 val, TCGTemp *addr,
>       orig_oi = oi = make_memop_idx(memop, idx);
>   
>       if ((memop & MO_BSWAP) && !tcg_target_has_memory_bswap(memop)) {
> -        swap = tcg_temp_ebb_new_i32();
> +        val = tcg_temp_ebb_new_i32();
>           switch (memop & MO_SIZE) {
>           case MO_16:
> -            tcg_gen_bswap16_i32(swap, val, 0);
> +            tcg_gen_bswap16_i32(val, orig_val, 0);
>               break;
>           case MO_32:
> -            tcg_gen_bswap32_i32(swap, val);
> +            tcg_gen_bswap32_i32(val, orig_val);
>               break;
>           default:
>               g_assert_not_reached();
>           }
> -        val = swap;
>           memop &= ~MO_BSWAP;
>           oi = make_memop_idx(memop, idx);
>       }
>   
>       addr_new = tci_extend_addr(addr);
>       gen_ldst1(INDEX_op_qemu_st, TCG_TYPE_I32, tcgv_i32_temp(val), addr_new, oi);
> -    plugin_gen_mem_callbacks_i32(val, NULL, addr, orig_oi, QEMU_PLUGIN_MEM_W);
> +    plugin_gen_mem_callbacks_i32(orig_val, NULL, addr, orig_oi,
> +                                 QEMU_PLUGIN_MEM_W);

It works, but it is still not clear to me why the value orig_val (which 
is not swapped and in target-endian order) gets stored swapped correctly 
in neg.plugin_mem_value_low.

I would expect it to be in target-endian order, since 
plugin_gen_mem_callbacks_i32 uses tcg_gen_st_i32(val, env, ... 
neg.plugin_mem_value_low) which does not perform any swap.

>       maybe_free_addr(addr, addr_new);
>   
> -    if (swap) {
> -        tcg_temp_free_i32(swap);
> +    if (val != orig_val) {
> +        tcg_temp_free_i32(val);
>       }
>   }
>   
> @@ -360,9 +361,6 @@ static void tcg_gen_qemu_ld_i64_int(TCGv_i64 val, TCGTemp *addr,
>       addr_new = tci_extend_addr(addr);
>       copy_addr = plugin_maybe_preserve_addr(addr);
>       gen_ld_i64(val, addr_new, oi);
> -    plugin_gen_mem_callbacks_i64(val, copy_addr, addr, orig_oi,
> -                                 QEMU_PLUGIN_MEM_R);
> -    maybe_free_addr(addr, addr_new);
>   
>       if ((orig_memop ^ memop) & MO_BSWAP) {
>           int flags = (orig_memop & MO_SIGN
> @@ -382,6 +380,10 @@ static void tcg_gen_qemu_ld_i64_int(TCGv_i64 val, TCGTemp *addr,
>               g_assert_not_reached();
>           }
>       }
> +
> +    plugin_gen_mem_callbacks_i64(val, copy_addr, addr, orig_oi,
> +                                 QEMU_PLUGIN_MEM_R);
> +    maybe_free_addr(addr, addr_new);
>   }
>   
>   void tcg_gen_qemu_ld_i64_chk(TCGv_i64 val, TCGTemp *addr, TCGArg idx,
> @@ -392,10 +394,10 @@ void tcg_gen_qemu_ld_i64_chk(TCGv_i64 val, TCGTemp *addr, TCGArg idx,
>       tcg_gen_qemu_ld_i64_int(val, addr, idx, memop);
>   }
>   
> -static void tcg_gen_qemu_st_i64_int(TCGv_i64 val, TCGTemp *addr,
> +static void tcg_gen_qemu_st_i64_int(TCGv_i64 orig_val, TCGTemp *addr,
>                                       TCGArg idx, MemOp memop)
>   {
> -    TCGv_i64 swap = NULL;
> +    TCGv_i64 val = orig_val;
>       MemOpIdx orig_oi, oi;
>       TCGTemp *addr_new;
>   
> @@ -404,32 +406,32 @@ static void tcg_gen_qemu_st_i64_int(TCGv_i64 val, TCGTemp *addr,
>       orig_oi = oi = make_memop_idx(memop, idx);
>   
>       if ((memop & MO_BSWAP) && !tcg_target_has_memory_bswap(memop)) {
> -        swap = tcg_temp_ebb_new_i64();
> +        val = tcg_temp_ebb_new_i64();
>           switch (memop & MO_SIZE) {
>           case MO_16:
> -            tcg_gen_bswap16_i64(swap, val, 0);
> +            tcg_gen_bswap16_i64(val, orig_val, 0);
>               break;
>           case MO_32:
> -            tcg_gen_bswap32_i64(swap, val, 0);
> +            tcg_gen_bswap32_i64(val, orig_val, 0);
>               break;
>           case MO_64:
> -            tcg_gen_bswap64_i64(swap, val);
> +            tcg_gen_bswap64_i64(val, orig_val);
>               break;
>           default:
>               g_assert_not_reached();
>           }
> -        val = swap;
>           memop &= ~MO_BSWAP;
>           oi = make_memop_idx(memop, idx);
>       }
>   
>       addr_new = tci_extend_addr(addr);
>       gen_st_i64(val, addr_new, oi);
> -    plugin_gen_mem_callbacks_i64(val, NULL, addr, orig_oi, QEMU_PLUGIN_MEM_W);
> +    plugin_gen_mem_callbacks_i64(orig_val, NULL, addr, orig_oi,
> +                                 QEMU_PLUGIN_MEM_W);
>       maybe_free_addr(addr, addr_new);
>   
> -    if (swap) {
> -        tcg_temp_free_i64(swap);
> +    if (val != orig_val) {
> +        tcg_temp_free_i64(val);
>       }
>   }
>   

This solves the problem, thanks!

I'll probably add another patch to document that better in plugin api, 
and remove the additional swapping done in tests/tcg/plugins/mem.c.

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

Thanks for the patch,
Pierrick