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

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

[Richard Henderson]

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.


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);
     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);
     }
 }
 
-- 
2.43.0

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

[Pierrick Bouvier]

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

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

[Pierrick Bouvier]

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.
> 
> 
> r~
> ---
>   tcg/tcg-op-ldst.c | 52 ++++++++++++++++++++++++-----------------------
>   1 file changed, 27 insertions(+), 25 deletions(-)
> 

I integrated this commit in a broader series on the same topic:
https://lore.kernel.org/qemu-devel/20260325024252.3369186-1-pierrick.bouvier@linaro.org/T/#t

Regards,
Pierrick

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

[Richard Henderson]

On 3/25/26 11:39, Pierrick Bouvier wrote:
> 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.

It's not about storing the value swapped, it's about storing the i32 to the low part of 
the uint64_t for the host endianness.

> 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.

Again, it is *not* swapped: orig_val is host-endian order, and so is neg.plugin_mem_value_low.

> 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.

What swapping is that?

There's a variable called "swapped_value", but it appears to be mis-named, or a remnant of 
swapping that used to occur.


r~

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

[Pierrick Bouvier]

On 3/24/26 9:59 PM, Richard Henderson wrote:
> On 3/25/26 11:39, Pierrick Bouvier wrote:
>> 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.
>
> It's not about storing the value swapped, it's about storing the i32 to the low part of
> the uint64_t for the host endianness.
>

I finally realized the orig_value we had is already in host-endian 
order, while I was assuming it was in target-endian order. Thus the 
confusion in our conversion. All good now.

>> 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.
> 
> Again, it is *not* swapped: orig_val is host-endian order, and so is neg.plugin_mem_value_low.
> 
>> 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.
> 
> What swapping is that?
>

It tracks memory regions we accessed, so which byte was written matters 
in this case, thus the need to swap to target-endian order.

> There's a variable called "swapped_value", but it appears to be mis-named, or a remnant of
> swapping that used to occur.
>

It's basically the value in target-endian order. FROM_BE should to TO_BE 
as well, even though both macros do exactly the same (FROM or TO acting 
as documentation to express the intent).

> 
> r~