[PATCH 0/3] plugins: fix endianness for qemu_plugin_mem_get_value()

[PATCH 0/3] plugins: fix endianness for qemu_plugin_mem_get_value()

[Pierrick Bouvier]

This series fixes qemu_plugin_mem_get_value() endianness on non x64 hosts.
As well, it updates documentation to make clear that value returned is in
host-endian order. Finally, it fixes tests/tcg/plugins/mem.c.

Pierrick Bouvier (2):
  include/plugins/qemu-plugin.h: add note about endianness of value
    returned by qemu_plugin_mem_get_value
  tests/tcg/plugins/mem.c: fix endian swap in update_region_info

Richard Henderson (1):
  tcg: Pass host-endian values to plugin_gen_mem_callbacks_*

 include/plugins/qemu-plugin.h |  5 ++--
 tcg/tcg-op-ldst.c             | 52 ++++++++++++++++++-----------------
 tests/tcg/plugins/mem.c       | 12 ++++----
 3 files changed, 36 insertions(+), 33 deletions(-)

-- 
2.47.3

[PATCH 1/3] tcg: Pass host-endian values to plugin_gen_mem_callbacks_*

[Pierrick Bouvier]

From: Richard Henderson <richard.henderson@linaro.org>

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>
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Tested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 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 354d9968f92..22211ccb452 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.47.3

[PATCH 2/3] include/plugins/qemu-plugin.h: add note about endianness of value returned by qemu_plugin_mem_get_value

[Pierrick Bouvier]

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/plugins/qemu-plugin.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/plugins/qemu-plugin.h b/include/plugins/qemu-plugin.h
index 2ce7a7fb6c3..4eb1d2cd85c 100644
--- a/include/plugins/qemu-plugin.h
+++ b/include/plugins/qemu-plugin.h
@@ -374,7 +374,8 @@ enum qemu_plugin_mem_value_type {
  *
  * @type: the memory access size
  * @data: the value accessed during the memory operation (value after
- *        read/write)
+ *        read/write). It's directly stored following host endianness, so no
+ *        further swap is needed.
  */
 typedef struct {
     enum qemu_plugin_mem_value_type type;
@@ -682,7 +683,7 @@ bool qemu_plugin_mem_is_store(qemu_plugin_meminfo_t info);
  * qemu_plugin_mem_get_value() - return last value loaded/stored
  * @info: opaque memory transaction handle
  *
- * Returns: memory value
+ * Returns: memory value in host-endian order (no further swap is necessary).
  */
 QEMU_PLUGIN_API
 qemu_plugin_mem_value qemu_plugin_mem_get_value(qemu_plugin_meminfo_t info);
-- 
2.47.3

Re: [PATCH 2/3] include/plugins/qemu-plugin.h: add note about endianness of value returned by qemu_plugin_mem_get_value

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>  include/plugins/qemu-plugin.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/include/plugins/qemu-plugin.h b/include/plugins/qemu-plugin.h
> index 2ce7a7fb6c3..4eb1d2cd85c 100644
> --- a/include/plugins/qemu-plugin.h
> +++ b/include/plugins/qemu-plugin.h
> @@ -374,7 +374,8 @@ enum qemu_plugin_mem_value_type {
>   *
>   * @type: the memory access size
>   * @data: the value accessed during the memory operation (value after
> - *        read/write)
> + *        read/write). It's directly stored following host endianness, so no
> + *        further swap is needed.
>   */
>  typedef struct {
>      enum qemu_plugin_mem_value_type type;
> @@ -682,7 +683,7 @@ bool qemu_plugin_mem_is_store(qemu_plugin_meminfo_t info);
>   * qemu_plugin_mem_get_value() - return last value loaded/stored
>   * @info: opaque memory transaction handle
>   *
> - * Returns: memory value
> + * Returns: memory value in host-endian order (no further swap is necessary).
>   */
>  QEMU_PLUGIN_API
>  qemu_plugin_mem_value qemu_plugin_mem_get_value(qemu_plugin_meminfo_t info);

I'd be tempted to fold this into the change that makes it happen in 1/3.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 2/3] include/plugins/qemu-plugin.h: add note about endianness of value returned by qemu_plugin_mem_get_value

[Pierrick Bouvier]

On 3/25/26 8:17 AM, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>   include/plugins/qemu-plugin.h | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/plugins/qemu-plugin.h b/include/plugins/qemu-plugin.h
>> index 2ce7a7fb6c3..4eb1d2cd85c 100644
>> --- a/include/plugins/qemu-plugin.h
>> +++ b/include/plugins/qemu-plugin.h
>> @@ -374,7 +374,8 @@ enum qemu_plugin_mem_value_type {
>>    *
>>    * @type: the memory access size
>>    * @data: the value accessed during the memory operation (value after
>> - *        read/write)
>> + *        read/write). It's directly stored following host endianness, so no
>> + *        further swap is needed.
>>    */
>>   typedef struct {
>>       enum qemu_plugin_mem_value_type type;
>> @@ -682,7 +683,7 @@ bool qemu_plugin_mem_is_store(qemu_plugin_meminfo_t info);
>>    * qemu_plugin_mem_get_value() - return last value loaded/stored
>>    * @info: opaque memory transaction handle
>>    *
>> - * Returns: memory value
>> + * Returns: memory value in host-endian order (no further swap is necessary).
>>    */
>>   QEMU_PLUGIN_API
>>   qemu_plugin_mem_value qemu_plugin_mem_get_value(qemu_plugin_meminfo_t info);
> 
> I'd be tempted to fold this into the change that makes it happen in 1/3.
> 

It was already supposed to be the behavior (x64 host did the right thing 
by accident since it skips swap operation and rely on moveb), and other 
host architectures had a bug that went unnoticed until recently.

Regards,
Pierrick

[PATCH 3/3] tests/tcg/plugins/mem.c: fix endian swap in update_region_info

[Pierrick Bouvier]

value returned by qemu_plugin_mem_get_value() is always in host-endian
order, so we need to convert TO target endianness and not FROM it.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 tests/tcg/plugins/mem.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/tcg/plugins/mem.c b/tests/tcg/plugins/mem.c
index 1ee257f855b..6b0732dc0fd 100644
--- a/tests/tcg/plugins/mem.c
+++ b/tests/tcg/plugins/mem.c
@@ -154,20 +154,20 @@ static void update_region_info(uint64_t region, uint64_t offset,
         val_size = 1;
         break;
     case QEMU_PLUGIN_MEM_VALUE_U16:
-        swapped_value.data.u16 = be ? GUINT16_FROM_BE(value.data.u16) :
-            GUINT16_FROM_LE(value.data.u16);
+        swapped_value.data.u16 = be ? GUINT16_TO_BE(value.data.u16) :
+            GUINT16_TO_LE(value.data.u16);
         val_ptr = &swapped_value.data.u16;
         val_size = 2;
         break;
     case QEMU_PLUGIN_MEM_VALUE_U32:
-        swapped_value.data.u32 = be ? GUINT32_FROM_BE(value.data.u32) :
-            GUINT32_FROM_LE(value.data.u32);
+        swapped_value.data.u32 = be ? GUINT32_TO_BE(value.data.u32) :
+            GUINT32_TO_LE(value.data.u32);
         val_ptr = &swapped_value.data.u32;
         val_size = 4;
         break;
     case QEMU_PLUGIN_MEM_VALUE_U64:
-        swapped_value.data.u64 = be ? GUINT64_FROM_BE(value.data.u64) :
-            GUINT64_FROM_LE(value.data.u64);
+        swapped_value.data.u64 = be ? GUINT64_TO_BE(value.data.u64) :
+            GUINT64_TO_LE(value.data.u64);
         val_ptr = &swapped_value.data.u64;
         val_size = 8;
         break;
-- 
2.47.3

Re: [PATCH 3/3] tests/tcg/plugins/mem.c: fix endian swap in update_region_info

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> value returned by qemu_plugin_mem_get_value() is always in host-endian
> order, so we need to convert TO target endianness and not FROM it.
>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  tests/tcg/plugins/mem.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tests/tcg/plugins/mem.c b/tests/tcg/plugins/mem.c
> index 1ee257f855b..6b0732dc0fd 100644
> --- a/tests/tcg/plugins/mem.c
> +++ b/tests/tcg/plugins/mem.c
> @@ -154,20 +154,20 @@ static void update_region_info(uint64_t region, uint64_t offset,
>          val_size = 1;
>          break;
>      case QEMU_PLUGIN_MEM_VALUE_U16:
> -        swapped_value.data.u16 = be ? GUINT16_FROM_BE(value.data.u16) :
> -            GUINT16_FROM_LE(value.data.u16);
> +        swapped_value.data.u16 = be ? GUINT16_TO_BE(value.data.u16) :
> +            GUINT16_TO_LE(value.data.u16);
>          val_ptr = &swapped_value.data.u16;
>          val_size = 2;
>          break;
>      case QEMU_PLUGIN_MEM_VALUE_U32:
> -        swapped_value.data.u32 = be ? GUINT32_FROM_BE(value.data.u32) :
> -            GUINT32_FROM_LE(value.data.u32);
> +        swapped_value.data.u32 = be ? GUINT32_TO_BE(value.data.u32) :
> +            GUINT32_TO_LE(value.data.u32);
>          val_ptr = &swapped_value.data.u32;
>          val_size = 4;
>          break;
>      case QEMU_PLUGIN_MEM_VALUE_U64:
> -        swapped_value.data.u64 = be ? GUINT64_FROM_BE(value.data.u64) :
> -            GUINT64_FROM_LE(value.data.u64);
> +        swapped_value.data.u64 = be ? GUINT64_TO_BE(value.data.u64) :
> +            GUINT64_TO_LE(value.data.u64);
>          val_ptr = &swapped_value.data.u64;
>          val_size = 8;
>          break;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 3/3] tests/tcg/plugins/mem.c: fix endian swap in update_region_info

[Pierrick Bouvier]

On 3/25/26 8:20 AM, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> value returned by qemu_plugin_mem_get_value() is always in host-endian
>> order, so we need to convert TO target endianness and not FROM it.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 
>> ---
>>   tests/tcg/plugins/mem.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>

As a complement, GUINT*_FROM_* and GUINT*_TO_* do the same thing (they 
both swap value). The FROM/TO name simply acts as a documentation when 
reading the code. That's why it was working before as expected, even 
though it was sending the wrong hint for the reader.

Regards,
Pierrick

Re: [PATCH 0/3] plugins: fix endianness for qemu_plugin_mem_get_value()

[Pierrick Bouvier]

On 3/24/26 7:42 PM, Pierrick Bouvier wrote:
> This series fixes qemu_plugin_mem_get_value() endianness on non x64 hosts.
> As well, it updates documentation to make clear that value returned is in
> host-endian order. Finally, it fixes tests/tcg/plugins/mem.c.
> 
> Pierrick Bouvier (2):
>    include/plugins/qemu-plugin.h: add note about endianness of value
>      returned by qemu_plugin_mem_get_value
>    tests/tcg/plugins/mem.c: fix endian swap in update_region_info
> 
> Richard Henderson (1):
>    tcg: Pass host-endian values to plugin_gen_mem_callbacks_*
> 
>   include/plugins/qemu-plugin.h |  5 ++--
>   tcg/tcg-op-ldst.c             | 52 ++++++++++++++++++-----------------
>   tests/tcg/plugins/mem.c       | 12 ++++----
>   3 files changed, 36 insertions(+), 33 deletions(-)
> 

This series is a good candidate for stable also.

Thanks,
Pierrick