On 7/3/24 11:58, Richard Henderson wrote:
On 7/2/24 11:44, Pierrick Bouvier wrote:
Different code paths handle memory accesses:
- tcg generated code
- load/store helpers
- atomic helpers
This value is saved in cpu->plugin_state.
Atomic operations are doing read/write at the same time, so we generate
two memory callbacks instead of one, to allow plugins to access distinct
values.
For now, we can have access only up to 128 bits, thus split this in two
64 bits words. When QEMU will support wider operations, we'll be able to
reconsider this.
Signed-off-by: Pierrick Bouvier <pierrick.bouv...@linaro.org>
---
accel/tcg/atomic_template.h | 66 ++++++++++++++++++++++++++++----
include/qemu/plugin.h | 8 ++++
plugins/core.c | 7 ++++
tcg/tcg-op-ldst.c | 72 +++++++++++++++++++++++++++++++----
accel/tcg/atomic_common.c.inc | 13 ++++++-
accel/tcg/ldst_common.c.inc | 38 +++++++++++-------
6 files changed, 173 insertions(+), 31 deletions(-)
It looks correct so,
Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
Possibilities for follow-up improvement:
--- a/tcg/tcg-op-ldst.c
+++ b/tcg/tcg-op-ldst.c
@@ -148,14 +148,24 @@ static TCGv_i64 plugin_maybe_preserve_addr(TCGTemp *addr)
return NULL;
}
+#ifdef CONFIG_PLUGIN
static void
-plugin_gen_mem_callbacks(TCGv_i64 copy_addr, TCGTemp *orig_addr, MemOpIdx oi,
+plugin_gen_mem_callbacks(TCGv_i64 value_low, TCGv_i64 value_high,
+ TCGv_i64 copy_addr, TCGTemp *orig_addr, MemOpIdx oi,
enum qemu_plugin_mem_rw rw)
{
-#ifdef CONFIG_PLUGIN
if (tcg_ctx->plugin_insn != NULL) {
qemu_plugin_meminfo_t info = make_plugin_meminfo(oi, rw);
+ TCGv_ptr plugin_state = tcg_temp_ebb_new_ptr();
+ tcg_gen_ld_ptr(plugin_state, tcg_env,
+ offsetof(CPUState, plugin_state) - sizeof(CPUState));
+ tcg_gen_st_i64(value_low, plugin_state,
+ offsetof(CPUPluginState, mem_value_low));
+ tcg_gen_st_i64(value_high, plugin_state,
+ offsetof(CPUPluginState, mem_value_high));
Maybe better to place this data at the beginning of CPUNegativeOffsetState?
This would eliminate a load dependency and most hosts would be able to use
(relatively)
small negative offset efficiently.
That's a good suggestion. Moved it here for v5.
+static void
+plugin_gen_mem_callbacks_i32(TCGv_i32 val,
+ TCGv_i64 copy_addr, TCGTemp *orig_addr,
+ MemOpIdx oi, enum qemu_plugin_mem_rw rw)
+{
+#ifdef CONFIG_PLUGIN
+ if (tcg_ctx->plugin_insn != NULL) {
+ TCGv_i64 ext_val = tcg_temp_ebb_new_i64();
+ tcg_gen_extu_i32_i64(ext_val, val);
+ plugin_gen_mem_callbacks(ext_val, tcg_constant_i64(0),
This zero extension got me to thinking:
(1) why zero extension and not sign-extension based on MO_SIGN from oi?
This was to truncate upper value, but as you mentioned, I simply clip it
later, so it's not important.
(2) given that the callback will have oi, do we really need any extension
at all here? We could allow the bits outside oi be garbage.
This would eliminate the store to value_high entirely for most ops,
and would allow this i32 op to avoid the extension -- simply perform
a 32-bit store into the low half of value_low.
I implemented selective store for plugin_gen_mem_callbacks function for
next version.
However, for helpers based memory accesses, I prefer to keep existing
version. It would require to create several small functions
(atomic_trace_rmw_post, plugin_load/store_cb and qemu_plugin_vcpu_mem_cb
for every size). It's something that could be desirable later when we'll
introduce bigger tcg word size though.
That appears to be what you're doing anyway with qemu_plugin_mem_value in the
next patch.
r~
