* [PATCH for-11.0] tcg: Pass host-endian values to plugin_gen_mem_callbacks_* @ 2026-03-25 0:40 Richard Henderson 2026-03-25 1:39 ` Pierrick Bouvier 2026-03-25 2:45 ` Pierrick Bouvier 0 siblings, 2 replies; 5+ messages in thread From: Richard Henderson @ 2026-03-25 0:40 UTC (permalink / raw) To: qemu-devel; +Cc: pierrick.bouvier 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH for-11.0] tcg: Pass host-endian values to plugin_gen_mem_callbacks_* 2026-03-25 0:40 [PATCH for-11.0] tcg: Pass host-endian values to plugin_gen_mem_callbacks_* Richard Henderson @ 2026-03-25 1:39 ` Pierrick Bouvier 2026-03-25 4:59 ` Richard Henderson 2026-03-25 2:45 ` Pierrick Bouvier 1 sibling, 1 reply; 5+ messages in thread From: Pierrick Bouvier @ 2026-03-25 1:39 UTC (permalink / raw) To: Richard Henderson, qemu-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for-11.0] tcg: Pass host-endian values to plugin_gen_mem_callbacks_* 2026-03-25 1:39 ` Pierrick Bouvier @ 2026-03-25 4:59 ` Richard Henderson 2026-03-25 15:15 ` Pierrick Bouvier 0 siblings, 1 reply; 5+ messages in thread From: Richard Henderson @ 2026-03-25 4:59 UTC (permalink / raw) To: Pierrick Bouvier, qemu-devel 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~ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for-11.0] tcg: Pass host-endian values to plugin_gen_mem_callbacks_* 2026-03-25 4:59 ` Richard Henderson @ 2026-03-25 15:15 ` Pierrick Bouvier 0 siblings, 0 replies; 5+ messages in thread From: Pierrick Bouvier @ 2026-03-25 15:15 UTC (permalink / raw) To: Richard Henderson, qemu-devel 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~ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for-11.0] tcg: Pass host-endian values to plugin_gen_mem_callbacks_* 2026-03-25 0:40 [PATCH for-11.0] tcg: Pass host-endian values to plugin_gen_mem_callbacks_* Richard Henderson 2026-03-25 1:39 ` Pierrick Bouvier @ 2026-03-25 2:45 ` Pierrick Bouvier 1 sibling, 0 replies; 5+ messages in thread From: Pierrick Bouvier @ 2026-03-25 2:45 UTC (permalink / raw) To: Richard Henderson, qemu-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-03-25 15:16 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-03-25 0:40 [PATCH for-11.0] tcg: Pass host-endian values to plugin_gen_mem_callbacks_* Richard Henderson 2026-03-25 1:39 ` Pierrick Bouvier 2026-03-25 4:59 ` Richard Henderson 2026-03-25 15:15 ` Pierrick Bouvier 2026-03-25 2:45 ` Pierrick Bouvier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox