* [PATCH v4 0/7] plugins: access values during a memory read/write
@ 2024-07-02 18:44 Pierrick Bouvier
2024-07-02 18:44 ` [PATCH v4 1/7] plugins: fix mem callback array size Pierrick Bouvier
` (7 more replies)
0 siblings, 8 replies; 21+ messages in thread
From: Pierrick Bouvier @ 2024-07-02 18:44 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Pierrick Bouvier,
Alexandre Iooss, Philippe Mathieu-Daudé, Paolo Bonzini,
Richard Henderson, Eduardo Habkost
This series allows plugins to know which value is read/written during a memory
access.
For every memory access, we know copy this value before calling mem callbacks,
and those can query it using new API function:
- qemu_plugin_mem_get_value
Mem plugin was extended to print accesses, and a new test was added to check
functionality work as expected. A bug was found where callbacks were not
called as expected.
This will open new use cases for plugins, such as following specific values in
memory.
v4
- fix prototype for stubs qemu_plugin_vcpu_mem_cb (inverted low/high parameters
names)
- link gitlab bugs resolved (thanks @Anton Kochkov for reporting)
https://gitlab.com/qemu-project/qemu/-/issues/1719
https://gitlab.com/qemu-project/qemu/-/issues/2152
v3
- simplify API: return an algebraic data type for value accessed
this can be easily extended when QEMU will support wider accesses
- fix Makefile test (use quiet-command instead of manually run the command)
- rename upper/lower to high/low
- reorder functions parameters and code to low/high instead of high/low, to
follow current convention in QEMU codebase
v2
- fix compilation on aarch64 (missing undef in accel/tcg/atomic_template.h)
v3
- add info when printing memory accesses (insn_vaddr,mem_vaddr,mem_hwaddr)
Pierrick Bouvier (7):
plugins: fix mem callback array size
plugins: save value during memory accesses
plugins: extend API to get latest memory value accessed
tests/tcg: add mechanism to run specific tests with plugins
tests/tcg: allow to check output of plugins
tests/plugin/mem: add option to print memory accesses
tests/tcg/x86_64: add test for plugin memory access
accel/tcg/atomic_template.h | 66 +++++++++++++--
include/qemu/plugin.h | 8 ++
include/qemu/qemu-plugin.h | 32 ++++++++
accel/tcg/plugin-gen.c | 3 +-
plugins/api.c | 34 ++++++++
plugins/core.c | 7 ++
tcg/tcg-op-ldst.c | 72 +++++++++++++++--
tests/plugin/mem.c | 69 +++++++++++++++-
tests/tcg/x86_64/test-plugin-mem-access.c | 89 +++++++++++++++++++++
accel/tcg/atomic_common.c.inc | 13 ++-
accel/tcg/ldst_common.c.inc | 38 +++++----
plugins/qemu-plugins.symbols | 1 +
tests/tcg/Makefile.target | 10 ++-
tests/tcg/x86_64/Makefile.target | 7 ++
tests/tcg/x86_64/check-plugin-mem-access.sh | 48 +++++++++++
15 files changed, 462 insertions(+), 35 deletions(-)
create mode 100644 tests/tcg/x86_64/test-plugin-mem-access.c
create mode 100755 tests/tcg/x86_64/check-plugin-mem-access.sh
--
2.39.2
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 1/7] plugins: fix mem callback array size
2024-07-02 18:44 [PATCH v4 0/7] plugins: access values during a memory read/write Pierrick Bouvier
@ 2024-07-02 18:44 ` Pierrick Bouvier
2024-07-03 18:39 ` Richard Henderson
2024-07-02 18:44 ` [PATCH v4 2/7] plugins: save value during memory accesses Pierrick Bouvier
` (6 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Pierrick Bouvier @ 2024-07-02 18:44 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Pierrick Bouvier,
Alexandre Iooss, Philippe Mathieu-Daudé, Paolo Bonzini,
Richard Henderson, Eduardo Habkost, Xingtao Yao
data was correctly copied, but size of array was not set
(g_array_sized_new only reserves memory, but does not set size).
As a result, callbacks were not called for code path relying on
plugin_register_vcpu_mem_cb().
Found when trying to trigger mem access callbacks for atomic
instructions.
Reviewed-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
accel/tcg/plugin-gen.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index b6bae32b997..ec89a085b43 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -85,8 +85,7 @@ static void gen_enable_mem_helper(struct qemu_plugin_tb *ptb,
len = insn->mem_cbs->len;
arr = g_array_sized_new(false, false,
sizeof(struct qemu_plugin_dyn_cb), len);
- memcpy(arr->data, insn->mem_cbs->data,
- len * sizeof(struct qemu_plugin_dyn_cb));
+ g_array_append_vals(arr, insn->mem_cbs->data, len);
qemu_plugin_add_dyn_cb_arr(arr);
tcg_gen_st_ptr(tcg_constant_ptr((intptr_t)arr), tcg_env,
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 2/7] plugins: save value during memory accesses
2024-07-02 18:44 [PATCH v4 0/7] plugins: access values during a memory read/write Pierrick Bouvier
2024-07-02 18:44 ` [PATCH v4 1/7] plugins: fix mem callback array size Pierrick Bouvier
@ 2024-07-02 18:44 ` Pierrick Bouvier
2024-07-03 18:58 ` Richard Henderson
2024-07-02 18:44 ` [PATCH v4 3/7] plugins: extend API to get latest memory value accessed Pierrick Bouvier
` (5 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Pierrick Bouvier @ 2024-07-02 18:44 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Pierrick Bouvier,
Alexandre Iooss, Philippe Mathieu-Daudé, Paolo Bonzini,
Richard Henderson, Eduardo Habkost
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.bouvier@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(-)
diff --git a/accel/tcg/atomic_template.h b/accel/tcg/atomic_template.h
index 1dc2151dafd..89593b2502f 100644
--- a/accel/tcg/atomic_template.h
+++ b/accel/tcg/atomic_template.h
@@ -53,6 +53,14 @@
# error unsupported data size
#endif
+#if DATA_SIZE == 16
+# define VALUE_LOW(val) int128_getlo(val)
+# define VALUE_HIGH(val) int128_gethi(val)
+#else
+# define VALUE_LOW(val) val
+# define VALUE_HIGH(val) 0
+#endif
+
#if DATA_SIZE >= 4
# define ABI_TYPE DATA_TYPE
#else
@@ -83,7 +91,12 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, abi_ptr addr,
ret = qatomic_cmpxchg__nocheck(haddr, cmpv, newv);
#endif
ATOMIC_MMU_CLEANUP;
- atomic_trace_rmw_post(env, addr, oi);
+ atomic_trace_rmw_post(env, addr,
+ VALUE_LOW(ret),
+ VALUE_HIGH(ret),
+ VALUE_LOW(newv),
+ VALUE_HIGH(newv),
+ oi);
return ret;
}
@@ -97,7 +110,12 @@ ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, abi_ptr addr, ABI_TYPE val,
ret = qatomic_xchg__nocheck(haddr, val);
ATOMIC_MMU_CLEANUP;
- atomic_trace_rmw_post(env, addr, oi);
+ atomic_trace_rmw_post(env, addr,
+ VALUE_LOW(ret),
+ VALUE_HIGH(ret),
+ VALUE_LOW(val),
+ VALUE_HIGH(val),
+ oi);
return ret;
}
@@ -109,7 +127,12 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, abi_ptr addr, \
haddr = atomic_mmu_lookup(env_cpu(env), addr, oi, DATA_SIZE, retaddr); \
ret = qatomic_##X(haddr, val); \
ATOMIC_MMU_CLEANUP; \
- atomic_trace_rmw_post(env, addr, oi); \
+ atomic_trace_rmw_post(env, addr, \
+ VALUE_LOW(ret), \
+ VALUE_HIGH(ret), \
+ VALUE_LOW(val), \
+ VALUE_HIGH(val), \
+ oi); \
return ret; \
}
@@ -145,7 +168,12 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, abi_ptr addr, \
cmp = qatomic_cmpxchg__nocheck(haddr, old, new); \
} while (cmp != old); \
ATOMIC_MMU_CLEANUP; \
- atomic_trace_rmw_post(env, addr, oi); \
+ atomic_trace_rmw_post(env, addr, \
+ VALUE_LOW(old), \
+ VALUE_HIGH(old), \
+ VALUE_LOW(xval), \
+ VALUE_HIGH(xval), \
+ oi); \
return RET; \
}
@@ -188,7 +216,12 @@ ABI_TYPE ATOMIC_NAME(cmpxchg)(CPUArchState *env, abi_ptr addr,
ret = qatomic_cmpxchg__nocheck(haddr, BSWAP(cmpv), BSWAP(newv));
#endif
ATOMIC_MMU_CLEANUP;
- atomic_trace_rmw_post(env, addr, oi);
+ atomic_trace_rmw_post(env, addr,
+ VALUE_LOW(ret),
+ VALUE_HIGH(ret),
+ VALUE_LOW(newv),
+ VALUE_HIGH(newv),
+ oi);
return BSWAP(ret);
}
@@ -202,7 +235,12 @@ ABI_TYPE ATOMIC_NAME(xchg)(CPUArchState *env, abi_ptr addr, ABI_TYPE val,
ret = qatomic_xchg__nocheck(haddr, BSWAP(val));
ATOMIC_MMU_CLEANUP;
- atomic_trace_rmw_post(env, addr, oi);
+ atomic_trace_rmw_post(env, addr,
+ VALUE_LOW(ret),
+ VALUE_HIGH(ret),
+ VALUE_LOW(val),
+ VALUE_HIGH(val),
+ oi);
return BSWAP(ret);
}
@@ -214,7 +252,12 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, abi_ptr addr, \
haddr = atomic_mmu_lookup(env_cpu(env), addr, oi, DATA_SIZE, retaddr); \
ret = qatomic_##X(haddr, BSWAP(val)); \
ATOMIC_MMU_CLEANUP; \
- atomic_trace_rmw_post(env, addr, oi); \
+ atomic_trace_rmw_post(env, addr, \
+ VALUE_LOW(ret), \
+ VALUE_HIGH(ret), \
+ VALUE_LOW(val), \
+ VALUE_HIGH(val), \
+ oi); \
return BSWAP(ret); \
}
@@ -247,7 +290,12 @@ ABI_TYPE ATOMIC_NAME(X)(CPUArchState *env, abi_ptr addr, \
ldn = qatomic_cmpxchg__nocheck(haddr, ldo, BSWAP(new)); \
} while (ldo != ldn); \
ATOMIC_MMU_CLEANUP; \
- atomic_trace_rmw_post(env, addr, oi); \
+ atomic_trace_rmw_post(env, addr, \
+ VALUE_LOW(old), \
+ VALUE_HIGH(old), \
+ VALUE_LOW(xval), \
+ VALUE_HIGH(xval), \
+ oi); \
return RET; \
}
@@ -281,3 +329,5 @@ GEN_ATOMIC_HELPER_FN(add_fetch, ADD, DATA_TYPE, new)
#undef SUFFIX
#undef DATA_SIZE
#undef SHIFT
+#undef VALUE_LOW
+#undef VALUE_HIGH
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index bc5aef979e7..0081991dab0 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -142,9 +142,13 @@ struct qemu_plugin_tb {
/**
* struct CPUPluginState - per-CPU state for plugins
* @event_mask: plugin event bitmap. Modified only via async work.
+ * @mem_value_low: 64 lower bits of latest accessed mem value.
+ * @mem_value_high: 64 higher bits of latest accessed mem value.
*/
struct CPUPluginState {
DECLARE_BITMAP(event_mask, QEMU_PLUGIN_EV_MAX);
+ uint64_t mem_value_low;
+ uint64_t mem_value_high;
};
/**
@@ -164,6 +168,8 @@ qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num, uint64_t a1,
void qemu_plugin_vcpu_syscall_ret(CPUState *cpu, int64_t num, int64_t ret);
void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
+ uint64_t value_low,
+ uint64_t value_high,
MemOpIdx oi, enum qemu_plugin_mem_rw rw);
void qemu_plugin_flush_cb(void);
@@ -248,6 +254,8 @@ void qemu_plugin_vcpu_syscall_ret(CPUState *cpu, int64_t num, int64_t ret)
{ }
static inline void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
+ uint64_t value_low,
+ uint64_t value_high,
MemOpIdx oi,
enum qemu_plugin_mem_rw rw)
{ }
diff --git a/plugins/core.c b/plugins/core.c
index 9d737d82787..a899eaf37af 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -575,14 +575,21 @@ void exec_inline_op(enum plugin_dyn_cb_type type,
}
void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
+ uint64_t value_low,
+ uint64_t value_high,
MemOpIdx oi, enum qemu_plugin_mem_rw rw)
{
GArray *arr = cpu->neg.plugin_mem_cbs;
+ CPUPluginState *plugin_state = cpu->plugin_state;
size_t i;
if (arr == NULL) {
return;
}
+
+ plugin_state->mem_value_low = value_low;
+ plugin_state->mem_value_high = value_high;
+
for (i = 0; i < arr->len; i++) {
struct qemu_plugin_dyn_cb *cb =
&g_array_index(arr, struct qemu_plugin_dyn_cb, i);
diff --git a/tcg/tcg-op-ldst.c b/tcg/tcg-op-ldst.c
index 85101602581..ea7f4fa8ded 100644
--- 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));
+ tcg_temp_free_ptr(plugin_state);
+
if (tcg_ctx->addr_type == TCG_TYPE_I32) {
if (!copy_addr) {
copy_addr = tcg_temp_ebb_new_i64();
@@ -172,6 +182,48 @@ plugin_gen_mem_callbacks(TCGv_i64 copy_addr, TCGTemp *orig_addr, MemOpIdx oi,
}
}
}
+}
+#endif
+
+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),
+ copy_addr, orig_addr, oi, rw);
+ tcg_temp_free_i64(ext_val);
+ }
+#endif
+}
+
+static void
+plugin_gen_mem_callbacks_i64(TCGv_i64 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) {
+ plugin_gen_mem_callbacks(val, tcg_constant_i64(0),
+ copy_addr, orig_addr, oi, rw);
+ }
+#endif
+}
+
+static void
+plugin_gen_mem_callbacks_i128(TCGv_i128 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) {
+ plugin_gen_mem_callbacks(TCGV128_LOW(val), TCGV128_HIGH(val),
+ copy_addr, orig_addr, oi, rw);
+ }
#endif
}
@@ -203,7 +255,8 @@ static void tcg_gen_qemu_ld_i32_int(TCGv_i32 val, TCGTemp *addr,
opc = INDEX_op_qemu_ld_a64_i32;
}
gen_ldst(opc, tcgv_i32_temp(val), NULL, addr, oi);
- plugin_gen_mem_callbacks(copy_addr, addr, orig_oi, QEMU_PLUGIN_MEM_R);
+ plugin_gen_mem_callbacks_i32(val, copy_addr, addr, orig_oi,
+ QEMU_PLUGIN_MEM_R);
if ((orig_memop ^ memop) & MO_BSWAP) {
switch (orig_memop & MO_SIZE) {
@@ -271,7 +324,7 @@ static void tcg_gen_qemu_st_i32_int(TCGv_i32 val, TCGTemp *addr,
}
}
gen_ldst(opc, tcgv_i32_temp(val), NULL, addr, oi);
- plugin_gen_mem_callbacks(NULL, addr, orig_oi, QEMU_PLUGIN_MEM_W);
+ plugin_gen_mem_callbacks_i32(val, NULL, addr, orig_oi, QEMU_PLUGIN_MEM_W);
if (swap) {
tcg_temp_free_i32(swap);
@@ -324,7 +377,8 @@ static void tcg_gen_qemu_ld_i64_int(TCGv_i64 val, TCGTemp *addr,
opc = INDEX_op_qemu_ld_a64_i64;
}
gen_ldst_i64(opc, val, addr, oi);
- plugin_gen_mem_callbacks(copy_addr, addr, orig_oi, QEMU_PLUGIN_MEM_R);
+ plugin_gen_mem_callbacks_i64(val, copy_addr, addr, orig_oi,
+ QEMU_PLUGIN_MEM_R);
if ((orig_memop ^ memop) & MO_BSWAP) {
int flags = (orig_memop & MO_SIGN
@@ -396,7 +450,7 @@ static void tcg_gen_qemu_st_i64_int(TCGv_i64 val, TCGTemp *addr,
opc = INDEX_op_qemu_st_a64_i64;
}
gen_ldst_i64(opc, val, addr, oi);
- plugin_gen_mem_callbacks(NULL, addr, orig_oi, QEMU_PLUGIN_MEM_W);
+ plugin_gen_mem_callbacks_i64(val, NULL, addr, orig_oi, QEMU_PLUGIN_MEM_W);
if (swap) {
tcg_temp_free_i64(swap);
@@ -606,7 +660,8 @@ static void tcg_gen_qemu_ld_i128_int(TCGv_i128 val, TCGTemp *addr,
tcg_constant_i32(orig_oi));
}
- plugin_gen_mem_callbacks(ext_addr, addr, orig_oi, QEMU_PLUGIN_MEM_R);
+ plugin_gen_mem_callbacks_i128(val, ext_addr, addr, orig_oi,
+ QEMU_PLUGIN_MEM_R);
}
void tcg_gen_qemu_ld_i128_chk(TCGv_i128 val, TCGTemp *addr, TCGArg idx,
@@ -722,7 +777,8 @@ static void tcg_gen_qemu_st_i128_int(TCGv_i128 val, TCGTemp *addr,
tcg_constant_i32(orig_oi));
}
- plugin_gen_mem_callbacks(ext_addr, addr, orig_oi, QEMU_PLUGIN_MEM_W);
+ plugin_gen_mem_callbacks_i128(val, ext_addr, addr, orig_oi,
+ QEMU_PLUGIN_MEM_W);
}
void tcg_gen_qemu_st_i128_chk(TCGv_i128 val, TCGTemp *addr, TCGArg idx,
diff --git a/accel/tcg/atomic_common.c.inc b/accel/tcg/atomic_common.c.inc
index 95a5c5ff12d..6056598c23d 100644
--- a/accel/tcg/atomic_common.c.inc
+++ b/accel/tcg/atomic_common.c.inc
@@ -14,9 +14,20 @@
*/
static void atomic_trace_rmw_post(CPUArchState *env, uint64_t addr,
+ uint64_t read_value_low,
+ uint64_t read_value_high,
+ uint64_t write_value_low,
+ uint64_t write_value_high,
MemOpIdx oi)
{
- qemu_plugin_vcpu_mem_cb(env_cpu(env), addr, oi, QEMU_PLUGIN_MEM_RW);
+ if (cpu_plugin_mem_cbs_enabled(env_cpu(env))) {
+ qemu_plugin_vcpu_mem_cb(env_cpu(env), addr,
+ read_value_low, read_value_high,
+ oi, QEMU_PLUGIN_MEM_R);
+ qemu_plugin_vcpu_mem_cb(env_cpu(env), addr,
+ write_value_low, write_value_high,
+ oi, QEMU_PLUGIN_MEM_W);
+ }
}
/*
diff --git a/accel/tcg/ldst_common.c.inc b/accel/tcg/ldst_common.c.inc
index 87ceb954873..ebbf380d767 100644
--- a/accel/tcg/ldst_common.c.inc
+++ b/accel/tcg/ldst_common.c.inc
@@ -123,10 +123,15 @@ void helper_st_i128(CPUArchState *env, uint64_t addr, Int128 val, MemOpIdx oi)
* Load helpers for cpu_ldst.h
*/
-static void plugin_load_cb(CPUArchState *env, abi_ptr addr, MemOpIdx oi)
+static void plugin_load_cb(CPUArchState *env, abi_ptr addr,
+ uint64_t value_low,
+ uint64_t value_high,
+ MemOpIdx oi)
{
if (cpu_plugin_mem_cbs_enabled(env_cpu(env))) {
- qemu_plugin_vcpu_mem_cb(env_cpu(env), addr, oi, QEMU_PLUGIN_MEM_R);
+ qemu_plugin_vcpu_mem_cb(env_cpu(env), addr,
+ value_low, value_high,
+ oi, QEMU_PLUGIN_MEM_R);
}
}
@@ -136,7 +141,7 @@ uint8_t cpu_ldb_mmu(CPUArchState *env, abi_ptr addr, MemOpIdx oi, uintptr_t ra)
tcg_debug_assert((get_memop(oi) & MO_SIZE) == MO_UB);
ret = do_ld1_mmu(env_cpu(env), addr, oi, ra, MMU_DATA_LOAD);
- plugin_load_cb(env, addr, oi);
+ plugin_load_cb(env, addr, ret, 0, oi);
return ret;
}
@@ -147,7 +152,7 @@ uint16_t cpu_ldw_mmu(CPUArchState *env, abi_ptr addr,
tcg_debug_assert((get_memop(oi) & MO_SIZE) == MO_16);
ret = do_ld2_mmu(env_cpu(env), addr, oi, ra, MMU_DATA_LOAD);
- plugin_load_cb(env, addr, oi);
+ plugin_load_cb(env, addr, ret, 0, oi);
return ret;
}
@@ -158,7 +163,7 @@ uint32_t cpu_ldl_mmu(CPUArchState *env, abi_ptr addr,
tcg_debug_assert((get_memop(oi) & MO_SIZE) == MO_32);
ret = do_ld4_mmu(env_cpu(env), addr, oi, ra, MMU_DATA_LOAD);
- plugin_load_cb(env, addr, oi);
+ plugin_load_cb(env, addr, ret, 0, oi);
return ret;
}
@@ -169,7 +174,7 @@ uint64_t cpu_ldq_mmu(CPUArchState *env, abi_ptr addr,
tcg_debug_assert((get_memop(oi) & MO_SIZE) == MO_64);
ret = do_ld8_mmu(env_cpu(env), addr, oi, ra, MMU_DATA_LOAD);
- plugin_load_cb(env, addr, oi);
+ plugin_load_cb(env, addr, ret, 0, oi);
return ret;
}
@@ -180,7 +185,7 @@ Int128 cpu_ld16_mmu(CPUArchState *env, abi_ptr addr,
tcg_debug_assert((get_memop(oi) & MO_SIZE) == MO_128);
ret = do_ld16_mmu(env_cpu(env), addr, oi, ra);
- plugin_load_cb(env, addr, oi);
+ plugin_load_cb(env, addr, int128_getlo(ret), int128_gethi(ret), oi);
return ret;
}
@@ -188,10 +193,15 @@ Int128 cpu_ld16_mmu(CPUArchState *env, abi_ptr addr,
* Store helpers for cpu_ldst.h
*/
-static void plugin_store_cb(CPUArchState *env, abi_ptr addr, MemOpIdx oi)
+static void plugin_store_cb(CPUArchState *env, abi_ptr addr,
+ uint64_t value_low,
+ uint64_t value_high,
+ MemOpIdx oi)
{
if (cpu_plugin_mem_cbs_enabled(env_cpu(env))) {
- qemu_plugin_vcpu_mem_cb(env_cpu(env), addr, oi, QEMU_PLUGIN_MEM_W);
+ qemu_plugin_vcpu_mem_cb(env_cpu(env), addr,
+ value_low, value_high,
+ oi, QEMU_PLUGIN_MEM_W);
}
}
@@ -199,7 +209,7 @@ void cpu_stb_mmu(CPUArchState *env, abi_ptr addr, uint8_t val,
MemOpIdx oi, uintptr_t retaddr)
{
helper_stb_mmu(env, addr, val, oi, retaddr);
- plugin_store_cb(env, addr, oi);
+ plugin_store_cb(env, addr, val, 0, oi);
}
void cpu_stw_mmu(CPUArchState *env, abi_ptr addr, uint16_t val,
@@ -207,7 +217,7 @@ void cpu_stw_mmu(CPUArchState *env, abi_ptr addr, uint16_t val,
{
tcg_debug_assert((get_memop(oi) & MO_SIZE) == MO_16);
do_st2_mmu(env_cpu(env), addr, val, oi, retaddr);
- plugin_store_cb(env, addr, oi);
+ plugin_store_cb(env, addr, val, 0, oi);
}
void cpu_stl_mmu(CPUArchState *env, abi_ptr addr, uint32_t val,
@@ -215,7 +225,7 @@ void cpu_stl_mmu(CPUArchState *env, abi_ptr addr, uint32_t val,
{
tcg_debug_assert((get_memop(oi) & MO_SIZE) == MO_32);
do_st4_mmu(env_cpu(env), addr, val, oi, retaddr);
- plugin_store_cb(env, addr, oi);
+ plugin_store_cb(env, addr, val, 0, oi);
}
void cpu_stq_mmu(CPUArchState *env, abi_ptr addr, uint64_t val,
@@ -223,7 +233,7 @@ void cpu_stq_mmu(CPUArchState *env, abi_ptr addr, uint64_t val,
{
tcg_debug_assert((get_memop(oi) & MO_SIZE) == MO_64);
do_st8_mmu(env_cpu(env), addr, val, oi, retaddr);
- plugin_store_cb(env, addr, oi);
+ plugin_store_cb(env, addr, val, 0, oi);
}
void cpu_st16_mmu(CPUArchState *env, abi_ptr addr, Int128 val,
@@ -231,7 +241,7 @@ void cpu_st16_mmu(CPUArchState *env, abi_ptr addr, Int128 val,
{
tcg_debug_assert((get_memop(oi) & MO_SIZE) == MO_128);
do_st16_mmu(env_cpu(env), addr, val, oi, retaddr);
- plugin_store_cb(env, addr, oi);
+ plugin_store_cb(env, addr, int128_getlo(val), int128_gethi(val), oi);
}
/*
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 3/7] plugins: extend API to get latest memory value accessed
2024-07-02 18:44 [PATCH v4 0/7] plugins: access values during a memory read/write Pierrick Bouvier
2024-07-02 18:44 ` [PATCH v4 1/7] plugins: fix mem callback array size Pierrick Bouvier
2024-07-02 18:44 ` [PATCH v4 2/7] plugins: save value during memory accesses Pierrick Bouvier
@ 2024-07-02 18:44 ` Pierrick Bouvier
2024-07-04 16:27 ` Richard Henderson
2024-07-02 18:44 ` [PATCH v4 4/7] tests/tcg: add mechanism to run specific tests with plugins Pierrick Bouvier
` (4 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Pierrick Bouvier @ 2024-07-02 18:44 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Pierrick Bouvier,
Alexandre Iooss, Philippe Mathieu-Daudé, Paolo Bonzini,
Richard Henderson, Eduardo Habkost
This value can be accessed only during a memory callback, using
new qemu_plugin_mem_get_value function.
Returned value can be extended when QEMU will support accesses wider
than 128 bits.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1719
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2152
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
include/qemu/qemu-plugin.h | 32 ++++++++++++++++++++++++++++++++
plugins/api.c | 34 ++++++++++++++++++++++++++++++++++
plugins/qemu-plugins.symbols | 1 +
3 files changed, 67 insertions(+)
diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index c71c705b699..649ce89815f 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -262,6 +262,29 @@ enum qemu_plugin_mem_rw {
QEMU_PLUGIN_MEM_RW,
};
+enum qemu_plugin_mem_value_type {
+ QEMU_PLUGIN_MEM_VALUE_U8,
+ QEMU_PLUGIN_MEM_VALUE_U16,
+ QEMU_PLUGIN_MEM_VALUE_U32,
+ QEMU_PLUGIN_MEM_VALUE_U64,
+ QEMU_PLUGIN_MEM_VALUE_U128,
+};
+
+/* typedef qemu_plugin_mem_value - value accessed during a load/store */
+typedef struct {
+ enum qemu_plugin_mem_value_type type;
+ union {
+ uint8_t u8;
+ uint16_t u16;
+ uint32_t u32;
+ uint64_t u64;
+ struct {
+ uint64_t low;
+ uint64_t high;
+ } u128;
+ } data;
+} qemu_plugin_mem_value;
+
/**
* enum qemu_plugin_cond - condition to enable callback
*
@@ -551,6 +574,15 @@ bool qemu_plugin_mem_is_big_endian(qemu_plugin_meminfo_t info);
QEMU_PLUGIN_API
bool qemu_plugin_mem_is_store(qemu_plugin_meminfo_t info);
+/**
+ * qemu_plugin_mem_get_mem_value() - return last value loaded/stored
+ * @info: opaque memory transaction handle
+ *
+ * Returns: memory value
+ */
+QEMU_PLUGIN_API
+qemu_plugin_mem_value qemu_plugin_mem_get_value(qemu_plugin_meminfo_t info);
+
/**
* qemu_plugin_get_hwaddr() - return handle for memory operation
* @info: opaque memory info structure
diff --git a/plugins/api.c b/plugins/api.c
index 2ff13d09de6..e9c07610052 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -351,6 +351,40 @@ bool qemu_plugin_mem_is_store(qemu_plugin_meminfo_t info)
return get_plugin_meminfo_rw(info) & QEMU_PLUGIN_MEM_W;
}
+qemu_plugin_mem_value qemu_plugin_mem_get_value(qemu_plugin_meminfo_t info)
+{
+ uint64_t low = current_cpu->plugin_state->mem_value_low;
+ uint64_t high = current_cpu->plugin_state->mem_value_high;
+ qemu_plugin_mem_value value;
+
+ switch (qemu_plugin_mem_size_shift(info)) {
+ case 0:
+ value.type = QEMU_PLUGIN_MEM_VALUE_U8;
+ value.data.u8 = (uint8_t)low;
+ break;
+ case 1:
+ value.type = QEMU_PLUGIN_MEM_VALUE_U16;
+ value.data.u16 = (uint16_t)low;
+ break;
+ case 2:
+ value.type = QEMU_PLUGIN_MEM_VALUE_U32;
+ value.data.u32 = (uint32_t)low;
+ break;
+ case 3:
+ value.type = QEMU_PLUGIN_MEM_VALUE_U64;
+ value.data.u64 = low;
+ break;
+ case 4:
+ value.type = QEMU_PLUGIN_MEM_VALUE_U128;
+ value.data.u128.low = low;
+ value.data.u128.high = high;
+ break;
+ default:
+ g_assert_not_reached();
+ }
+ return value;
+}
+
/*
* Virtual Memory queries
*/
diff --git a/plugins/qemu-plugins.symbols b/plugins/qemu-plugins.symbols
index ca773d8d9fe..eed9d8abd90 100644
--- a/plugins/qemu-plugins.symbols
+++ b/plugins/qemu-plugins.symbols
@@ -13,6 +13,7 @@
qemu_plugin_insn_size;
qemu_plugin_insn_symbol;
qemu_plugin_insn_vaddr;
+ qemu_plugin_mem_get_value;
qemu_plugin_mem_is_big_endian;
qemu_plugin_mem_is_sign_extended;
qemu_plugin_mem_is_store;
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 4/7] tests/tcg: add mechanism to run specific tests with plugins
2024-07-02 18:44 [PATCH v4 0/7] plugins: access values during a memory read/write Pierrick Bouvier
` (2 preceding siblings ...)
2024-07-02 18:44 ` [PATCH v4 3/7] plugins: extend API to get latest memory value accessed Pierrick Bouvier
@ 2024-07-02 18:44 ` Pierrick Bouvier
2024-07-03 2:26 ` Xingtao Yao (Fujitsu) via
2024-07-02 18:44 ` [PATCH v4 5/7] tests/tcg: allow to check output of plugins Pierrick Bouvier
` (3 subsequent siblings)
7 siblings, 1 reply; 21+ messages in thread
From: Pierrick Bouvier @ 2024-07-02 18:44 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Pierrick Bouvier,
Alexandre Iooss, Philippe Mathieu-Daudé, Paolo Bonzini,
Richard Henderson, Eduardo Habkost
Only multiarch tests are run with plugins, and we want to be able to run
per-arch test with plugins too.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
tests/tcg/Makefile.target | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
index f21be50d3b2..dc5c8b7a3b4 100644
--- a/tests/tcg/Makefile.target
+++ b/tests/tcg/Makefile.target
@@ -152,10 +152,11 @@ PLUGINS=$(patsubst %.c, lib%.so, $(notdir $(wildcard $(PLUGIN_SRC)/*.c)))
# only expand MULTIARCH_TESTS which are common on most of our targets
# to avoid an exponential explosion as new tests are added. We also
# add some special helpers the run-plugin- rules can use below.
+# In more, extra tests can be added using PLUGINS_TESTS variable.
ifneq ($(MULTIARCH_TESTS),)
$(foreach p,$(PLUGINS), \
- $(foreach t,$(MULTIARCH_TESTS),\
+ $(foreach t,$(MULTIARCH_TESTS) $(PLUGINS_TESTS),\
$(eval run-plugin-$(t)-with-$(p): $t $p) \
$(eval RUN_TESTS+=run-plugin-$(t)-with-$(p))))
endif # MULTIARCH_TESTS
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 5/7] tests/tcg: allow to check output of plugins
2024-07-02 18:44 [PATCH v4 0/7] plugins: access values during a memory read/write Pierrick Bouvier
` (3 preceding siblings ...)
2024-07-02 18:44 ` [PATCH v4 4/7] tests/tcg: add mechanism to run specific tests with plugins Pierrick Bouvier
@ 2024-07-02 18:44 ` Pierrick Bouvier
2024-07-03 2:30 ` Xingtao Yao (Fujitsu) via
2024-07-04 16:28 ` Richard Henderson
2024-07-02 18:44 ` [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses Pierrick Bouvier
` (2 subsequent siblings)
7 siblings, 2 replies; 21+ messages in thread
From: Pierrick Bouvier @ 2024-07-02 18:44 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Pierrick Bouvier,
Alexandre Iooss, Philippe Mathieu-Daudé, Paolo Bonzini,
Richard Henderson, Eduardo Habkost
A specific plugin test can now read and check a plugin output, to ensure
it contains expected values.
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
tests/tcg/Makefile.target | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
index dc5c8b7a3b4..b78fd99c337 100644
--- a/tests/tcg/Makefile.target
+++ b/tests/tcg/Makefile.target
@@ -90,6 +90,7 @@ CFLAGS=
LDFLAGS=
QEMU_OPTS=
+CHECK_PLUGIN_OUTPUT_COMMAND=true
# If TCG debugging, or TCI is enabled things are a lot slower
@@ -180,6 +181,9 @@ run-plugin-%:
-plugin $(PLUGIN_LIB)/$(call extract-plugin,$@)$(PLUGIN_ARGS) \
-d plugin -D $*.pout \
$(call strip-plugin,$<))
+ $(call quiet-command, $(CHECK_PLUGIN_OUTPUT_COMMAND) $*.pout, \
+ TEST, check plugin $(call extract-plugin,$@) output \
+ with $(call strip-plugin,$<))
else
run-%: %
$(call run-test, $<, \
@@ -194,6 +198,9 @@ run-plugin-%:
-plugin $(PLUGIN_LIB)/$(call extract-plugin,$@)$(PLUGIN_ARGS) \
-d plugin -D $*.pout \
$(QEMU_OPTS) $(call strip-plugin,$<))
+ $(call quiet-command, $(CHECK_PLUGIN_OUTPUT_COMMAND) $*.pout, \
+ TEST, check plugin $(call extract-plugin,$@) output \
+ with $(call strip-plugin,$<))
endif
gdb-%: %
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses
2024-07-02 18:44 [PATCH v4 0/7] plugins: access values during a memory read/write Pierrick Bouvier
` (4 preceding siblings ...)
2024-07-02 18:44 ` [PATCH v4 5/7] tests/tcg: allow to check output of plugins Pierrick Bouvier
@ 2024-07-02 18:44 ` Pierrick Bouvier
2024-07-03 1:56 ` Xingtao Yao (Fujitsu) via
2024-07-04 16:30 ` Richard Henderson
2024-07-02 18:44 ` [PATCH v4 7/7] tests/tcg/x86_64: add test for plugin memory access Pierrick Bouvier
2024-07-05 0:42 ` [PATCH v4 0/7] plugins: access values during a memory read/write Pierrick Bouvier
7 siblings, 2 replies; 21+ messages in thread
From: Pierrick Bouvier @ 2024-07-02 18:44 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Pierrick Bouvier,
Alexandre Iooss, Philippe Mathieu-Daudé, Paolo Bonzini,
Richard Henderson, Eduardo Habkost
By using "print-accesses=true" option, mem plugin will now print every
value accessed, with associated size, type (store vs load), symbol,
instruction address and phys/virt address accessed.
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
tests/plugin/mem.c | 69 +++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 68 insertions(+), 1 deletion(-)
diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
index b650dddcce1..aecbad8e68d 100644
--- a/tests/plugin/mem.c
+++ b/tests/plugin/mem.c
@@ -21,10 +21,15 @@ typedef struct {
uint64_t io_count;
} CPUCount;
+typedef struct {
+ uint64_t vaddr;
+ const char *sym;
+} InsnInfo;
+
static struct qemu_plugin_scoreboard *counts;
static qemu_plugin_u64 mem_count;
static qemu_plugin_u64 io_count;
-static bool do_inline, do_callback;
+static bool do_inline, do_callback, do_print_accesses;
static bool do_haddr;
static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
@@ -60,6 +65,44 @@ static void vcpu_mem(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
}
}
+static void print_access(unsigned int cpu_index, qemu_plugin_meminfo_t meminfo,
+ uint64_t vaddr, void *udata)
+{
+ InsnInfo *insn_info = udata;
+ unsigned size = 8 << qemu_plugin_mem_size_shift(meminfo);
+ const char *type = qemu_plugin_mem_is_store(meminfo) ? "store" : "load";
+ qemu_plugin_mem_value value = qemu_plugin_mem_get_value(meminfo);
+ uint64_t hwaddr =
+ qemu_plugin_hwaddr_phys_addr(qemu_plugin_get_hwaddr(meminfo, vaddr));
+ g_autoptr(GString) out = g_string_new("");
+ g_string_printf(out,
+ "0x%"PRIx64",%s,0x%"PRIx64",0x%"PRIx64",%d,%s,",
+ insn_info->vaddr, insn_info->sym,
+ vaddr, hwaddr, size, type);
+ switch (value.type) {
+ case QEMU_PLUGIN_MEM_VALUE_U8:
+ g_string_append_printf(out, "0x%"PRIx8, value.data.u8);
+ break;
+ case QEMU_PLUGIN_MEM_VALUE_U16:
+ g_string_append_printf(out, "0x%"PRIx16, value.data.u16);
+ break;
+ case QEMU_PLUGIN_MEM_VALUE_U32:
+ g_string_append_printf(out, "0x%"PRIx32, value.data.u32);
+ break;
+ case QEMU_PLUGIN_MEM_VALUE_U64:
+ g_string_append_printf(out, "0x%"PRIx64, value.data.u64);
+ break;
+ case QEMU_PLUGIN_MEM_VALUE_U128:
+ g_string_append_printf(out, "0x%.0"PRIx64"%"PRIx64,
+ value.data.u128.high, value.data.u128.low);
+ break;
+ default:
+ g_assert_not_reached();
+ }
+ g_string_append_printf(out, "\n");
+ qemu_plugin_outs(out->str);
+}
+
static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
{
size_t n = qemu_plugin_tb_n_insns(tb);
@@ -79,6 +122,16 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
QEMU_PLUGIN_CB_NO_REGS,
rw, NULL);
}
+ if (do_print_accesses) {
+ /* we leak this pointer, to avoid locking to keep track of it */
+ InsnInfo *insn_info = g_malloc(sizeof(InsnInfo));
+ const char *sym = qemu_plugin_insn_symbol(insn);
+ insn_info->sym = sym ? sym : "";
+ insn_info->vaddr = qemu_plugin_insn_vaddr(insn);
+ qemu_plugin_register_vcpu_mem_cb(insn, print_access,
+ QEMU_PLUGIN_CB_NO_REGS,
+ rw, (void *) insn_info);
+ }
}
}
@@ -117,6 +170,12 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
return -1;
}
+ } else if (g_strcmp0(tokens[0], "print-accesses") == 0) {
+ if (!qemu_plugin_bool_parse(tokens[0], tokens[1],
+ &do_print_accesses)) {
+ fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
+ return -1;
+ }
} else {
fprintf(stderr, "option parsing failed: %s\n", opt);
return -1;
@@ -129,6 +188,14 @@ QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
return -1;
}
+ if (do_print_accesses) {
+ g_autoptr(GString) out = g_string_new("");
+ g_string_printf(out,
+ "insn_vaddr,insn_symbol,mem_vaddr,mem_hwaddr,"
+ "access_size,access_type,mem_value\n");
+ qemu_plugin_outs(out->str);
+ }
+
counts = qemu_plugin_scoreboard_new(sizeof(CPUCount));
mem_count = qemu_plugin_scoreboard_u64_in_struct(
counts, CPUCount, mem_count);
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 7/7] tests/tcg/x86_64: add test for plugin memory access
2024-07-02 18:44 [PATCH v4 0/7] plugins: access values during a memory read/write Pierrick Bouvier
` (5 preceding siblings ...)
2024-07-02 18:44 ` [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses Pierrick Bouvier
@ 2024-07-02 18:44 ` Pierrick Bouvier
2024-07-03 1:42 ` Xingtao Yao (Fujitsu) via
2024-07-05 0:42 ` [PATCH v4 0/7] plugins: access values during a memory read/write Pierrick Bouvier
7 siblings, 1 reply; 21+ messages in thread
From: Pierrick Bouvier @ 2024-07-02 18:44 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Pierrick Bouvier,
Alexandre Iooss, Philippe Mathieu-Daudé, Paolo Bonzini,
Richard Henderson, Eduardo Habkost
Add an explicit test to check expected memory values are read/written.
For sizes 8, 16, 32, 64 and 128, we generate a load/store operation.
For size 8 -> 64, we generate an atomic __sync_val_compare_and_swap too.
For 128bits memory access, we rely on SSE2 instructions.
By default, atomic accesses are non atomic if a single cpu is running,
so we force creation of a second one by creating a new thread first.
load/store helpers code path can't be triggered easily in user mode (no
softmmu), so we can't test it here.
Can be run with:
make -C build/tests/tcg/x86_64-linux-user run-plugin-test-plugin-mem-access-with-libmem.so
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
tests/tcg/x86_64/test-plugin-mem-access.c | 89 +++++++++++++++++++++
tests/tcg/x86_64/Makefile.target | 7 ++
tests/tcg/x86_64/check-plugin-mem-access.sh | 48 +++++++++++
3 files changed, 144 insertions(+)
create mode 100644 tests/tcg/x86_64/test-plugin-mem-access.c
create mode 100755 tests/tcg/x86_64/check-plugin-mem-access.sh
diff --git a/tests/tcg/x86_64/test-plugin-mem-access.c b/tests/tcg/x86_64/test-plugin-mem-access.c
new file mode 100644
index 00000000000..7fdd6a55829
--- /dev/null
+++ b/tests/tcg/x86_64/test-plugin-mem-access.c
@@ -0,0 +1,89 @@
+#include <emmintrin.h>
+#include <pthread.h>
+#include <stdint.h>
+#include <stdlib.h>
+
+static void *data;
+
+#define DEFINE_STORE(name, type, value) \
+static void store_##name(void) \
+{ \
+ *((type *)data) = value; \
+}
+
+#define DEFINE_ATOMIC_OP(name, type, value) \
+static void atomic_op_##name(void) \
+{ \
+ *((type *)data) = 0x42; \
+ __sync_val_compare_and_swap((type *)data, 0x42, value); \
+}
+
+#define DEFINE_LOAD(name, type) \
+static void load_##name(void) \
+{ \
+ register type var asm("eax") = *((type *) data); \
+ (void)var; \
+}
+
+DEFINE_STORE(u8, uint8_t, 0xf1)
+DEFINE_ATOMIC_OP(u8, uint8_t, 0xf1)
+DEFINE_LOAD(u8, uint8_t)
+DEFINE_STORE(u16, uint16_t, 0xf123)
+DEFINE_ATOMIC_OP(u16, uint16_t, 0xf123)
+DEFINE_LOAD(u16, uint16_t)
+DEFINE_STORE(u32, uint32_t, 0xff112233)
+DEFINE_ATOMIC_OP(u32, uint32_t, 0xff112233)
+DEFINE_LOAD(u32, uint32_t)
+DEFINE_STORE(u64, uint64_t, 0xf123456789abcdef)
+DEFINE_ATOMIC_OP(u64, uint64_t, 0xf123456789abcdef)
+DEFINE_LOAD(u64, uint64_t)
+
+static void store_u128(void)
+{
+ _mm_store_si128(data, _mm_set_epi32(0xf1223344, 0x55667788,
+ 0xf1234567, 0x89abcdef));
+}
+
+static void load_u128(void)
+{
+ __m128i var = _mm_load_si128(data);
+ (void)var;
+}
+
+static void *f(void *p)
+{
+ return NULL;
+}
+
+int main(void)
+{
+ /*
+ * We force creation of a second thread to enable cpu flag CF_PARALLEL.
+ * This will generate atomic operations when needed.
+ */
+ pthread_t thread;
+ pthread_create(&thread, NULL, &f, NULL);
+ pthread_join(thread, NULL);
+
+ data = malloc(sizeof(__m128i));
+ atomic_op_u8();
+ store_u8();
+ load_u8();
+
+ atomic_op_u16();
+ store_u16();
+ load_u16();
+
+ atomic_op_u32();
+ store_u32();
+ load_u32();
+
+ atomic_op_u64();
+ store_u64();
+ load_u64();
+
+ store_u128();
+ load_u128();
+
+ free(data);
+}
diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
index 5fedf221174..5f7015fd8b4 100644
--- a/tests/tcg/x86_64/Makefile.target
+++ b/tests/tcg/x86_64/Makefile.target
@@ -14,6 +14,7 @@ X86_64_TESTS += noexec
X86_64_TESTS += cmpxchg
X86_64_TESTS += adox
X86_64_TESTS += test-1648
+PLUGINS_TESTS += test-plugin-mem-access
TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
else
TESTS=$(MULTIARCH_TESTS)
@@ -24,6 +25,12 @@ adox: CFLAGS=-O2
run-test-i386-ssse3: QEMU_OPTS += -cpu max
run-plugin-test-i386-ssse3-%: QEMU_OPTS += -cpu max
+run-plugin-test-plugin-mem-access-with-libmem.so: \
+ PLUGIN_ARGS=$(COMMA)print-accesses=true
+run-plugin-test-plugin-mem-access-with-libmem.so: \
+ CHECK_PLUGIN_OUTPUT_COMMAND= \
+ $(SRC_PATH)/tests/tcg/x86_64/check-plugin-mem-access.sh
+
test-x86_64: LDFLAGS+=-lm -lc
test-x86_64: test-i386.c test-i386.h test-i386-shift.h test-i386-muldiv.h
$(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
diff --git a/tests/tcg/x86_64/check-plugin-mem-access.sh b/tests/tcg/x86_64/check-plugin-mem-access.sh
new file mode 100755
index 00000000000..92008e25a93
--- /dev/null
+++ b/tests/tcg/x86_64/check-plugin-mem-access.sh
@@ -0,0 +1,48 @@
+#!/usr/bin/env bash
+
+set -euo pipefail
+
+die()
+{
+ echo "$@" 1>&2
+ exit 1
+}
+
+check()
+{
+ file=$1
+ pattern=$2
+ grep "$pattern" "$file" > /dev/null || die "\"$pattern\" not found in $file"
+}
+
+[ $# -eq 1 ] || die "usage: plugin_out_file"
+
+plugin_out=$1
+
+expected()
+{
+ cat << EOF
+,store_u8,.*,8,store,0xf1
+,atomic_op_u8,.*,8,load,0x42
+,atomic_op_u8,.*,8,store,0xf1
+,load_u8,.*,8,load,0xf1
+,store_u16,.*,16,store,0xf123
+,atomic_op_u16,.*,16,load,0x42
+,atomic_op_u16,.*,16,store,0xf123
+,load_u16,.*,16,load,0xf123
+,store_u32,.*,32,store,0xff112233
+,atomic_op_u32,.*,32,load,0x42
+,atomic_op_u32,.*,32,store,0xff112233
+,load_u32,.*,32,load,0xff112233
+,store_u64,.*,64,store,0xf123456789abcdef
+,atomic_op_u64,.*,64,load,0x42
+,atomic_op_u64,.*,64,store,0xf123456789abcdef
+,load_u64,.*,64,load,0xf123456789abcdef
+,store_u128,.*,128,store,0xf122334455667788f123456789abcdef
+,load_u128,.*,128,load,0xf122334455667788f123456789abcdef
+EOF
+}
+
+expected | while read line; do
+ check "$plugin_out" "$line"
+done
--
2.39.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* RE: [PATCH v4 7/7] tests/tcg/x86_64: add test for plugin memory access
2024-07-02 18:44 ` [PATCH v4 7/7] tests/tcg/x86_64: add test for plugin memory access Pierrick Bouvier
@ 2024-07-03 1:42 ` Xingtao Yao (Fujitsu) via
0 siblings, 0 replies; 21+ messages in thread
From: Xingtao Yao (Fujitsu) via @ 2024-07-03 1:42 UTC (permalink / raw)
To: Pierrick Bouvier, qemu-devel@nongnu.org
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
> -----Original Message-----
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org
> <qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org> On Behalf Of
> Pierrick Bouvier
> Sent: Wednesday, July 3, 2024 2:45 AM
> To: qemu-devel@nongnu.org
> Cc: Alex Bennée <alex.bennee@linaro.org>; Mahmoud Mandour
> <ma.mandourr@gmail.com>; Pierrick Bouvier <pierrick.bouvier@linaro.org>;
> Alexandre Iooss <erdnaxe@crans.org>; Philippe Mathieu-Daudé
> <philmd@linaro.org>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson
> <richard.henderson@linaro.org>; Eduardo Habkost <eduardo@habkost.net>
> Subject: [PATCH v4 7/7] tests/tcg/x86_64: add test for plugin memory access
>
> Add an explicit test to check expected memory values are read/written.
> For sizes 8, 16, 32, 64 and 128, we generate a load/store operation.
> For size 8 -> 64, we generate an atomic __sync_val_compare_and_swap too.
> For 128bits memory access, we rely on SSE2 instructions.
>
> By default, atomic accesses are non atomic if a single cpu is running,
> so we force creation of a second one by creating a new thread first.
>
> load/store helpers code path can't be triggered easily in user mode (no
> softmmu), so we can't test it here.
>
> Can be run with:
> make -C build/tests/tcg/x86_64-linux-user
> run-plugin-test-plugin-mem-access-with-libmem.so
>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
> tests/tcg/x86_64/test-plugin-mem-access.c | 89
> +++++++++++++++++++++
> tests/tcg/x86_64/Makefile.target | 7 ++
> tests/tcg/x86_64/check-plugin-mem-access.sh | 48 +++++++++++
> 3 files changed, 144 insertions(+)
> create mode 100644 tests/tcg/x86_64/test-plugin-mem-access.c
> create mode 100755 tests/tcg/x86_64/check-plugin-mem-access.sh
>
> diff --git a/tests/tcg/x86_64/test-plugin-mem-access.c
> b/tests/tcg/x86_64/test-plugin-mem-access.c
> new file mode 100644
> index 00000000000..7fdd6a55829
> --- /dev/null
> +++ b/tests/tcg/x86_64/test-plugin-mem-access.c
> @@ -0,0 +1,89 @@
> +#include <emmintrin.h>
> +#include <pthread.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +
> +static void *data;
> +
> +#define DEFINE_STORE(name, type, value) \
> +static void store_##name(void) \
> +{ \
> + *((type *)data) = value; \
> +}
> +
> +#define DEFINE_ATOMIC_OP(name, type, value) \
> +static void atomic_op_##name(void) \
> +{ \
> + *((type *)data) = 0x42; \
> + __sync_val_compare_and_swap((type *)data, 0x42, value); \
> +}
> +
> +#define DEFINE_LOAD(name, type) \
> +static void load_##name(void) \
> +{ \
> + register type var asm("eax") = *((type *) data); \
> + (void)var; \
> +}
> +
> +DEFINE_STORE(u8, uint8_t, 0xf1)
> +DEFINE_ATOMIC_OP(u8, uint8_t, 0xf1)
> +DEFINE_LOAD(u8, uint8_t)
> +DEFINE_STORE(u16, uint16_t, 0xf123)
> +DEFINE_ATOMIC_OP(u16, uint16_t, 0xf123)
> +DEFINE_LOAD(u16, uint16_t)
> +DEFINE_STORE(u32, uint32_t, 0xff112233)
> +DEFINE_ATOMIC_OP(u32, uint32_t, 0xff112233)
> +DEFINE_LOAD(u32, uint32_t)
> +DEFINE_STORE(u64, uint64_t, 0xf123456789abcdef)
> +DEFINE_ATOMIC_OP(u64, uint64_t, 0xf123456789abcdef)
> +DEFINE_LOAD(u64, uint64_t)
> +
> +static void store_u128(void)
> +{
> + _mm_store_si128(data, _mm_set_epi32(0xf1223344, 0x55667788,
> + 0xf1234567, 0x89abcdef));
> +}
> +
> +static void load_u128(void)
> +{
> + __m128i var = _mm_load_si128(data);
> + (void)var;
> +}
> +
> +static void *f(void *p)
> +{
> + return NULL;
> +}
> +
> +int main(void)
> +{
> + /*
> + * We force creation of a second thread to enable cpu flag CF_PARALLEL.
> + * This will generate atomic operations when needed.
> + */
> + pthread_t thread;
> + pthread_create(&thread, NULL, &f, NULL);
> + pthread_join(thread, NULL);
> +
> + data = malloc(sizeof(__m128i));
> + atomic_op_u8();
> + store_u8();
> + load_u8();
> +
> + atomic_op_u16();
> + store_u16();
> + load_u16();
> +
> + atomic_op_u32();
> + store_u32();
> + load_u32();
> +
> + atomic_op_u64();
> + store_u64();
> + load_u64();
> +
> + store_u128();
> + load_u128();
> +
> + free(data);
> +}
> diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
> index 5fedf221174..5f7015fd8b4 100644
> --- a/tests/tcg/x86_64/Makefile.target
> +++ b/tests/tcg/x86_64/Makefile.target
> @@ -14,6 +14,7 @@ X86_64_TESTS += noexec
> X86_64_TESTS += cmpxchg
> X86_64_TESTS += adox
> X86_64_TESTS += test-1648
> +PLUGINS_TESTS += test-plugin-mem-access
> TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
> else
> TESTS=$(MULTIARCH_TESTS)
> @@ -24,6 +25,12 @@ adox: CFLAGS=-O2
> run-test-i386-ssse3: QEMU_OPTS += -cpu max
> run-plugin-test-i386-ssse3-%: QEMU_OPTS += -cpu max
>
> +run-plugin-test-plugin-mem-access-with-libmem.so: \
> + PLUGIN_ARGS=$(COMMA)print-accesses=true
> +run-plugin-test-plugin-mem-access-with-libmem.so: \
> + CHECK_PLUGIN_OUTPUT_COMMAND= \
> + $(SRC_PATH)/tests/tcg/x86_64/check-plugin-mem-access.sh
> +
> test-x86_64: LDFLAGS+=-lm -lc
> test-x86_64: test-i386.c test-i386.h test-i386-shift.h test-i386-muldiv.h
> $(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
> diff --git a/tests/tcg/x86_64/check-plugin-mem-access.sh
> b/tests/tcg/x86_64/check-plugin-mem-access.sh
> new file mode 100755
> index 00000000000..92008e25a93
> --- /dev/null
> +++ b/tests/tcg/x86_64/check-plugin-mem-access.sh
> @@ -0,0 +1,48 @@
> +#!/usr/bin/env bash
> +
> +set -euo pipefail
> +
> +die()
> +{
> + echo "$@" 1>&2
> + exit 1
> +}
> +
> +check()
> +{
> + file=$1
> + pattern=$2
> + grep "$pattern" "$file" > /dev/null || die "\"$pattern\" not found in $file"
> +}
> +
> +[ $# -eq 1 ] || die "usage: plugin_out_file"
> +
> +plugin_out=$1
> +
> +expected()
> +{
> + cat << EOF
> +,store_u8,.*,8,store,0xf1
> +,atomic_op_u8,.*,8,load,0x42
> +,atomic_op_u8,.*,8,store,0xf1
> +,load_u8,.*,8,load,0xf1
> +,store_u16,.*,16,store,0xf123
> +,atomic_op_u16,.*,16,load,0x42
> +,atomic_op_u16,.*,16,store,0xf123
> +,load_u16,.*,16,load,0xf123
> +,store_u32,.*,32,store,0xff112233
> +,atomic_op_u32,.*,32,load,0x42
> +,atomic_op_u32,.*,32,store,0xff112233
> +,load_u32,.*,32,load,0xff112233
> +,store_u64,.*,64,store,0xf123456789abcdef
> +,atomic_op_u64,.*,64,load,0x42
> +,atomic_op_u64,.*,64,store,0xf123456789abcdef
> +,load_u64,.*,64,load,0xf123456789abcdef
> +,store_u128,.*,128,store,0xf122334455667788f123456789abcdef
> +,load_u128,.*,128,load,0xf122334455667788f123456789abcdef
> +EOF
> +}
> +
> +expected | while read line; do
> + check "$plugin_out" "$line"
> +done
> --
> 2.39.2
>
Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses
2024-07-02 18:44 ` [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses Pierrick Bouvier
@ 2024-07-03 1:56 ` Xingtao Yao (Fujitsu) via
2024-07-04 23:30 ` Pierrick Bouvier
2024-07-04 16:30 ` Richard Henderson
1 sibling, 1 reply; 21+ messages in thread
From: Xingtao Yao (Fujitsu) via @ 2024-07-03 1:56 UTC (permalink / raw)
To: Pierrick Bouvier, qemu-devel@nongnu.org
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
one small suggestion:
Keeping the addresses or values of fixed size in output message can improve the readability of logs.
like:
> + case QEMU_PLUGIN_MEM_VALUE_U8:
> + g_string_append_printf(out, "0x%"PRIx8, value.data.u8);
> + break;
case QEMU_PLUGIN_MEM_VALUE_U8:
g_string_append_printf(out, "0x02%"PRIx8, value.data.u8);
break;
> -----Original Message-----
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org
> <qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org> On Behalf Of
> Pierrick Bouvier
> Sent: Wednesday, July 3, 2024 2:45 AM
> To: qemu-devel@nongnu.org
> Cc: Alex Bennée <alex.bennee@linaro.org>; Mahmoud Mandour
> <ma.mandourr@gmail.com>; Pierrick Bouvier <pierrick.bouvier@linaro.org>;
> Alexandre Iooss <erdnaxe@crans.org>; Philippe Mathieu-Daudé
> <philmd@linaro.org>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson
> <richard.henderson@linaro.org>; Eduardo Habkost <eduardo@habkost.net>
> Subject: [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses
>
> By using "print-accesses=true" option, mem plugin will now print every
> value accessed, with associated size, type (store vs load), symbol,
> instruction address and phys/virt address accessed.
>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
> tests/plugin/mem.c | 69
> +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
> index b650dddcce1..aecbad8e68d 100644
> --- a/tests/plugin/mem.c
> +++ b/tests/plugin/mem.c
> @@ -21,10 +21,15 @@ typedef struct {
> uint64_t io_count;
> } CPUCount;
>
> +typedef struct {
> + uint64_t vaddr;
> + const char *sym;
> +} InsnInfo;
> +
> static struct qemu_plugin_scoreboard *counts;
> static qemu_plugin_u64 mem_count;
> static qemu_plugin_u64 io_count;
> -static bool do_inline, do_callback;
> +static bool do_inline, do_callback, do_print_accesses;
> static bool do_haddr;
> static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
>
> @@ -60,6 +65,44 @@ static void vcpu_mem(unsigned int cpu_index,
> qemu_plugin_meminfo_t meminfo,
> }
> }
>
> +static void print_access(unsigned int cpu_index, qemu_plugin_meminfo_t
> meminfo,
> + uint64_t vaddr, void *udata)
> +{
> + InsnInfo *insn_info = udata;
> + unsigned size = 8 << qemu_plugin_mem_size_shift(meminfo);
> + const char *type = qemu_plugin_mem_is_store(meminfo) ? "store" : "load";
> + qemu_plugin_mem_value value = qemu_plugin_mem_get_value(meminfo);
> + uint64_t hwaddr =
> + qemu_plugin_hwaddr_phys_addr(qemu_plugin_get_hwaddr(meminfo,
> vaddr));
> + g_autoptr(GString) out = g_string_new("");
> + g_string_printf(out,
> + "0x%"PRIx64",%s,0x%"PRIx64",0x%"PRIx64",%d,%s,",
> + insn_info->vaddr, insn_info->sym,
> + vaddr, hwaddr, size, type);
> + switch (value.type) {
> + case QEMU_PLUGIN_MEM_VALUE_U8:
> + g_string_append_printf(out, "0x%"PRIx8, value.data.u8);
> + break;
> + case QEMU_PLUGIN_MEM_VALUE_U16:
> + g_string_append_printf(out, "0x%"PRIx16, value.data.u16);
> + break;
> + case QEMU_PLUGIN_MEM_VALUE_U32:
> + g_string_append_printf(out, "0x%"PRIx32, value.data.u32);
> + break;
> + case QEMU_PLUGIN_MEM_VALUE_U64:
> + g_string_append_printf(out, "0x%"PRIx64, value.data.u64);
> + break;
> + case QEMU_PLUGIN_MEM_VALUE_U128:
> + g_string_append_printf(out, "0x%.0"PRIx64"%"PRIx64,
> + value.data.u128.high, value.data.u128.low);
> + break;
> + default:
> + g_assert_not_reached();
> + }
> + g_string_append_printf(out, "\n");
> + qemu_plugin_outs(out->str);
> +}
> +
> static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
> {
> size_t n = qemu_plugin_tb_n_insns(tb);
> @@ -79,6 +122,16 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct
> qemu_plugin_tb *tb)
> QEMU_PLUGIN_CB_NO_REGS,
> rw, NULL);
> }
> + if (do_print_accesses) {
> + /* we leak this pointer, to avoid locking to keep track of it */
> + InsnInfo *insn_info = g_malloc(sizeof(InsnInfo));
> + const char *sym = qemu_plugin_insn_symbol(insn);
> + insn_info->sym = sym ? sym : "";
> + insn_info->vaddr = qemu_plugin_insn_vaddr(insn);
> + qemu_plugin_register_vcpu_mem_cb(insn, print_access,
> + QEMU_PLUGIN_CB_NO_REGS,
> + rw, (void *) insn_info);
> + }
> }
> }
>
> @@ -117,6 +170,12 @@ QEMU_PLUGIN_EXPORT int
> qemu_plugin_install(qemu_plugin_id_t id,
> fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
> return -1;
> }
> + } else if (g_strcmp0(tokens[0], "print-accesses") == 0) {
> + if (!qemu_plugin_bool_parse(tokens[0], tokens[1],
> + &do_print_accesses)) {
> + fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
> + return -1;
> + }
> } else {
> fprintf(stderr, "option parsing failed: %s\n", opt);
> return -1;
> @@ -129,6 +188,14 @@ QEMU_PLUGIN_EXPORT int
> qemu_plugin_install(qemu_plugin_id_t id,
> return -1;
> }
>
> + if (do_print_accesses) {
> + g_autoptr(GString) out = g_string_new("");
> + g_string_printf(out,
> + "insn_vaddr,insn_symbol,mem_vaddr,mem_hwaddr,"
> + "access_size,access_type,mem_value\n");
> + qemu_plugin_outs(out->str);
> + }
> +
> counts = qemu_plugin_scoreboard_new(sizeof(CPUCount));
> mem_count = qemu_plugin_scoreboard_u64_in_struct(
> counts, CPUCount, mem_count);
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v4 4/7] tests/tcg: add mechanism to run specific tests with plugins
2024-07-02 18:44 ` [PATCH v4 4/7] tests/tcg: add mechanism to run specific tests with plugins Pierrick Bouvier
@ 2024-07-03 2:26 ` Xingtao Yao (Fujitsu) via
0 siblings, 0 replies; 21+ messages in thread
From: Xingtao Yao (Fujitsu) via @ 2024-07-03 2:26 UTC (permalink / raw)
To: Pierrick Bouvier, qemu-devel@nongnu.org
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
> -----Original Message-----
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org
> <qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org> On Behalf Of
> Pierrick Bouvier
> Sent: Wednesday, July 3, 2024 2:45 AM
> To: qemu-devel@nongnu.org
> Cc: Alex Bennée <alex.bennee@linaro.org>; Mahmoud Mandour
> <ma.mandourr@gmail.com>; Pierrick Bouvier <pierrick.bouvier@linaro.org>;
> Alexandre Iooss <erdnaxe@crans.org>; Philippe Mathieu-Daudé
> <philmd@linaro.org>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson
> <richard.henderson@linaro.org>; Eduardo Habkost <eduardo@habkost.net>
> Subject: [PATCH v4 4/7] tests/tcg: add mechanism to run specific tests with
> plugins
>
> Only multiarch tests are run with plugins, and we want to be able to run
> per-arch test with plugins too.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
> tests/tcg/Makefile.target | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
> index f21be50d3b2..dc5c8b7a3b4 100644
> --- a/tests/tcg/Makefile.target
> +++ b/tests/tcg/Makefile.target
> @@ -152,10 +152,11 @@ PLUGINS=$(patsubst %.c, lib%.so, $(notdir $(wildcard
> $(PLUGIN_SRC)/*.c)))
> # only expand MULTIARCH_TESTS which are common on most of our targets
> # to avoid an exponential explosion as new tests are added. We also
> # add some special helpers the run-plugin- rules can use below.
> +# In more, extra tests can be added using PLUGINS_TESTS variable.
>
> ifneq ($(MULTIARCH_TESTS),)
> $(foreach p,$(PLUGINS), \
> - $(foreach t,$(MULTIARCH_TESTS),\
> + $(foreach t,$(MULTIARCH_TESTS) $(PLUGINS_TESTS),\
> $(eval run-plugin-$(t)-with-$(p): $t $p) \
> $(eval RUN_TESTS+=run-plugin-$(t)-with-$(p))))
> endif # MULTIARCH_TESTS
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* RE: [PATCH v4 5/7] tests/tcg: allow to check output of plugins
2024-07-02 18:44 ` [PATCH v4 5/7] tests/tcg: allow to check output of plugins Pierrick Bouvier
@ 2024-07-03 2:30 ` Xingtao Yao (Fujitsu) via
2024-07-04 16:28 ` Richard Henderson
1 sibling, 0 replies; 21+ messages in thread
From: Xingtao Yao (Fujitsu) via @ 2024-07-03 2:30 UTC (permalink / raw)
To: Pierrick Bouvier, qemu-devel@nongnu.org
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
> -----Original Message-----
> From: qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org
> <qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org> On Behalf Of
> Pierrick Bouvier
> Sent: Wednesday, July 3, 2024 2:45 AM
> To: qemu-devel@nongnu.org
> Cc: Alex Bennée <alex.bennee@linaro.org>; Mahmoud Mandour
> <ma.mandourr@gmail.com>; Pierrick Bouvier <pierrick.bouvier@linaro.org>;
> Alexandre Iooss <erdnaxe@crans.org>; Philippe Mathieu-Daudé
> <philmd@linaro.org>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson
> <richard.henderson@linaro.org>; Eduardo Habkost <eduardo@habkost.net>
> Subject: [PATCH v4 5/7] tests/tcg: allow to check output of plugins
>
> A specific plugin test can now read and check a plugin output, to ensure
> it contains expected values.
>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
> tests/tcg/Makefile.target | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/tests/tcg/Makefile.target b/tests/tcg/Makefile.target
> index dc5c8b7a3b4..b78fd99c337 100644
> --- a/tests/tcg/Makefile.target
> +++ b/tests/tcg/Makefile.target
> @@ -90,6 +90,7 @@ CFLAGS=
> LDFLAGS=
>
> QEMU_OPTS=
> +CHECK_PLUGIN_OUTPUT_COMMAND=true
>
>
> # If TCG debugging, or TCI is enabled things are a lot slower
> @@ -180,6 +181,9 @@ run-plugin-%:
> -plugin $(PLUGIN_LIB)/$(call
> extract-plugin,$@)$(PLUGIN_ARGS) \
> -d plugin -D $*.pout \
> $(call strip-plugin,$<))
> + $(call quiet-command, $(CHECK_PLUGIN_OUTPUT_COMMAND)
> $*.pout, \
> + TEST, check plugin $(call extract-plugin,$@) output \
> + with $(call strip-plugin,$<))
> else
> run-%: %
> $(call run-test, $<, \
> @@ -194,6 +198,9 @@ run-plugin-%:
> -plugin $(PLUGIN_LIB)/$(call
> extract-plugin,$@)$(PLUGIN_ARGS) \
> -d plugin -D $*.pout \
> $(QEMU_OPTS) $(call strip-plugin,$<))
> + $(call quiet-command, $(CHECK_PLUGIN_OUTPUT_COMMAND)
> $*.pout, \
> + TEST, check plugin $(call extract-plugin,$@) output \
> + with $(call strip-plugin,$<))
> endif
>
> gdb-%: %
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 1/7] plugins: fix mem callback array size
2024-07-02 18:44 ` [PATCH v4 1/7] plugins: fix mem callback array size Pierrick Bouvier
@ 2024-07-03 18:39 ` Richard Henderson
0 siblings, 0 replies; 21+ messages in thread
From: Richard Henderson @ 2024-07-03 18:39 UTC (permalink / raw)
To: Pierrick Bouvier, qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Eduardo Habkost,
Xingtao Yao
On 7/2/24 11:44, Pierrick Bouvier wrote:
> data was correctly copied, but size of array was not set
> (g_array_sized_new only reserves memory, but does not set size).
>
> As a result, callbacks were not called for code path relying on
> plugin_register_vcpu_mem_cb().
>
> Found when trying to trigger mem access callbacks for atomic
> instructions.
>
> Reviewed-by: Xingtao Yao<yaoxt.fnst@fujitsu.com>
> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> ---
> accel/tcg/plugin-gen.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/7] plugins: save value during memory accesses
2024-07-02 18:44 ` [PATCH v4 2/7] plugins: save value during memory accesses Pierrick Bouvier
@ 2024-07-03 18:58 ` Richard Henderson
2024-07-05 0:33 ` Pierrick Bouvier
0 siblings, 1 reply; 21+ messages in thread
From: Richard Henderson @ 2024-07-03 18:58 UTC (permalink / raw)
To: Pierrick Bouvier, qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Eduardo Habkost
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.bouvier@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.henderson@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.
> +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?
(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.
That appears to be what you're doing anyway with qemu_plugin_mem_value in the next patch.
r~
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 3/7] plugins: extend API to get latest memory value accessed
2024-07-02 18:44 ` [PATCH v4 3/7] plugins: extend API to get latest memory value accessed Pierrick Bouvier
@ 2024-07-04 16:27 ` Richard Henderson
0 siblings, 0 replies; 21+ messages in thread
From: Richard Henderson @ 2024-07-04 16:27 UTC (permalink / raw)
To: Pierrick Bouvier, qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Eduardo Habkost
On 7/2/24 11:44, Pierrick Bouvier wrote:
> This value can be accessed only during a memory callback, using
> new qemu_plugin_mem_get_value function.
>
> Returned value can be extended when QEMU will support accesses wider
> than 128 bits.
>
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/1719
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/2152
> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> ---
> include/qemu/qemu-plugin.h | 32 ++++++++++++++++++++++++++++++++
> plugins/api.c | 34 ++++++++++++++++++++++++++++++++++
> plugins/qemu-plugins.symbols | 1 +
> 3 files changed, 67 insertions(+)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 5/7] tests/tcg: allow to check output of plugins
2024-07-02 18:44 ` [PATCH v4 5/7] tests/tcg: allow to check output of plugins Pierrick Bouvier
2024-07-03 2:30 ` Xingtao Yao (Fujitsu) via
@ 2024-07-04 16:28 ` Richard Henderson
1 sibling, 0 replies; 21+ messages in thread
From: Richard Henderson @ 2024-07-04 16:28 UTC (permalink / raw)
To: Pierrick Bouvier, qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Eduardo Habkost
On 7/2/24 11:44, Pierrick Bouvier wrote:
> A specific plugin test can now read and check a plugin output, to ensure
> it contains expected values.
>
> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> ---
> tests/tcg/Makefile.target | 7 +++++++
> 1 file changed, 7 insertions(+)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses
2024-07-02 18:44 ` [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses Pierrick Bouvier
2024-07-03 1:56 ` Xingtao Yao (Fujitsu) via
@ 2024-07-04 16:30 ` Richard Henderson
2024-07-04 23:29 ` Pierrick Bouvier
1 sibling, 1 reply; 21+ messages in thread
From: Richard Henderson @ 2024-07-04 16:30 UTC (permalink / raw)
To: Pierrick Bouvier, qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Eduardo Habkost
On 7/2/24 11:44, Pierrick Bouvier wrote:
> + case QEMU_PLUGIN_MEM_VALUE_U128:
> + g_string_append_printf(out, "0x%.0"PRIx64"%"PRIx64,
> + value.data.u128.high, value.data.u128.low);
PRIx64 does not pad.
You need %016"PRIx64 for the low value.
Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses
2024-07-04 16:30 ` Richard Henderson
@ 2024-07-04 23:29 ` Pierrick Bouvier
0 siblings, 0 replies; 21+ messages in thread
From: Pierrick Bouvier @ 2024-07-04 23:29 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Eduardo Habkost
On 7/4/24 09:30, Richard Henderson wrote:
> On 7/2/24 11:44, Pierrick Bouvier wrote:
>> + case QEMU_PLUGIN_MEM_VALUE_U128:
>> + g_string_append_printf(out, "0x%.0"PRIx64"%"PRIx64,
>> + value.data.u128.high, value.data.u128.low);
>
> PRIx64 does not pad.
> You need %016"PRIx64 for the low value.
>
Oops, indeed. I'll output all values with fixed width.
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
>
> r~
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses
2024-07-03 1:56 ` Xingtao Yao (Fujitsu) via
@ 2024-07-04 23:30 ` Pierrick Bouvier
0 siblings, 0 replies; 21+ messages in thread
From: Pierrick Bouvier @ 2024-07-04 23:30 UTC (permalink / raw)
To: Xingtao Yao (Fujitsu), qemu-devel@nongnu.org
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
On 7/2/24 18:56, Xingtao Yao (Fujitsu) wrote:
> Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
>
> one small suggestion:
> Keeping the addresses or values of fixed size in output message can improve the readability of logs.
Ok, I'll do it for every size.
> like:
>> + case QEMU_PLUGIN_MEM_VALUE_U8:
>> + g_string_append_printf(out, "0x%"PRIx8, value.data.u8);
>> + break;
> case QEMU_PLUGIN_MEM_VALUE_U8:
> g_string_append_printf(out, "0x02%"PRIx8, value.data.u8);
> break;
>
>
>> -----Original Message-----
>> From: qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org
>> <qemu-devel-bounces+yaoxt.fnst=fujitsu.com@nongnu.org> On Behalf Of
>> Pierrick Bouvier
>> Sent: Wednesday, July 3, 2024 2:45 AM
>> To: qemu-devel@nongnu.org
>> Cc: Alex Bennée <alex.bennee@linaro.org>; Mahmoud Mandour
>> <ma.mandourr@gmail.com>; Pierrick Bouvier <pierrick.bouvier@linaro.org>;
>> Alexandre Iooss <erdnaxe@crans.org>; Philippe Mathieu-Daudé
>> <philmd@linaro.org>; Paolo Bonzini <pbonzini@redhat.com>; Richard Henderson
>> <richard.henderson@linaro.org>; Eduardo Habkost <eduardo@habkost.net>
>> Subject: [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses
>>
>> By using "print-accesses=true" option, mem plugin will now print every
>> value accessed, with associated size, type (store vs load), symbol,
>> instruction address and phys/virt address accessed.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>> tests/plugin/mem.c | 69
>> +++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 68 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/plugin/mem.c b/tests/plugin/mem.c
>> index b650dddcce1..aecbad8e68d 100644
>> --- a/tests/plugin/mem.c
>> +++ b/tests/plugin/mem.c
>> @@ -21,10 +21,15 @@ typedef struct {
>> uint64_t io_count;
>> } CPUCount;
>>
>> +typedef struct {
>> + uint64_t vaddr;
>> + const char *sym;
>> +} InsnInfo;
>> +
>> static struct qemu_plugin_scoreboard *counts;
>> static qemu_plugin_u64 mem_count;
>> static qemu_plugin_u64 io_count;
>> -static bool do_inline, do_callback;
>> +static bool do_inline, do_callback, do_print_accesses;
>> static bool do_haddr;
>> static enum qemu_plugin_mem_rw rw = QEMU_PLUGIN_MEM_RW;
>>
>> @@ -60,6 +65,44 @@ static void vcpu_mem(unsigned int cpu_index,
>> qemu_plugin_meminfo_t meminfo,
>> }
>> }
>>
>> +static void print_access(unsigned int cpu_index, qemu_plugin_meminfo_t
>> meminfo,
>> + uint64_t vaddr, void *udata)
>> +{
>> + InsnInfo *insn_info = udata;
>> + unsigned size = 8 << qemu_plugin_mem_size_shift(meminfo);
>> + const char *type = qemu_plugin_mem_is_store(meminfo) ? "store" : "load";
>> + qemu_plugin_mem_value value = qemu_plugin_mem_get_value(meminfo);
>> + uint64_t hwaddr =
>> + qemu_plugin_hwaddr_phys_addr(qemu_plugin_get_hwaddr(meminfo,
>> vaddr));
>> + g_autoptr(GString) out = g_string_new("");
>> + g_string_printf(out,
>> + "0x%"PRIx64",%s,0x%"PRIx64",0x%"PRIx64",%d,%s,",
>> + insn_info->vaddr, insn_info->sym,
>> + vaddr, hwaddr, size, type);
>> + switch (value.type) {
>> + case QEMU_PLUGIN_MEM_VALUE_U8:
>> + g_string_append_printf(out, "0x%"PRIx8, value.data.u8);
>> + break;
>> + case QEMU_PLUGIN_MEM_VALUE_U16:
>> + g_string_append_printf(out, "0x%"PRIx16, value.data.u16);
>> + break;
>> + case QEMU_PLUGIN_MEM_VALUE_U32:
>> + g_string_append_printf(out, "0x%"PRIx32, value.data.u32);
>> + break;
>> + case QEMU_PLUGIN_MEM_VALUE_U64:
>> + g_string_append_printf(out, "0x%"PRIx64, value.data.u64);
>> + break;
>> + case QEMU_PLUGIN_MEM_VALUE_U128:
>> + g_string_append_printf(out, "0x%.0"PRIx64"%"PRIx64,
>> + value.data.u128.high, value.data.u128.low);
>> + break;
>> + default:
>> + g_assert_not_reached();
>> + }
>> + g_string_append_printf(out, "\n");
>> + qemu_plugin_outs(out->str);
>> +}
>> +
>> static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
>> {
>> size_t n = qemu_plugin_tb_n_insns(tb);
>> @@ -79,6 +122,16 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct
>> qemu_plugin_tb *tb)
>> QEMU_PLUGIN_CB_NO_REGS,
>> rw, NULL);
>> }
>> + if (do_print_accesses) {
>> + /* we leak this pointer, to avoid locking to keep track of it */
>> + InsnInfo *insn_info = g_malloc(sizeof(InsnInfo));
>> + const char *sym = qemu_plugin_insn_symbol(insn);
>> + insn_info->sym = sym ? sym : "";
>> + insn_info->vaddr = qemu_plugin_insn_vaddr(insn);
>> + qemu_plugin_register_vcpu_mem_cb(insn, print_access,
>> + QEMU_PLUGIN_CB_NO_REGS,
>> + rw, (void *) insn_info);
>> + }
>> }
>> }
>>
>> @@ -117,6 +170,12 @@ QEMU_PLUGIN_EXPORT int
>> qemu_plugin_install(qemu_plugin_id_t id,
>> fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
>> return -1;
>> }
>> + } else if (g_strcmp0(tokens[0], "print-accesses") == 0) {
>> + if (!qemu_plugin_bool_parse(tokens[0], tokens[1],
>> + &do_print_accesses)) {
>> + fprintf(stderr, "boolean argument parsing failed: %s\n", opt);
>> + return -1;
>> + }
>> } else {
>> fprintf(stderr, "option parsing failed: %s\n", opt);
>> return -1;
>> @@ -129,6 +188,14 @@ QEMU_PLUGIN_EXPORT int
>> qemu_plugin_install(qemu_plugin_id_t id,
>> return -1;
>> }
>>
>> + if (do_print_accesses) {
>> + g_autoptr(GString) out = g_string_new("");
>> + g_string_printf(out,
>> + "insn_vaddr,insn_symbol,mem_vaddr,mem_hwaddr,"
>> + "access_size,access_type,mem_value\n");
>> + qemu_plugin_outs(out->str);
>> + }
>> +
>> counts = qemu_plugin_scoreboard_new(sizeof(CPUCount));
>> mem_count = qemu_plugin_scoreboard_u64_in_struct(
>> counts, CPUCount, mem_count);
>> --
>> 2.39.2
>>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/7] plugins: save value during memory accesses
2024-07-03 18:58 ` Richard Henderson
@ 2024-07-05 0:33 ` Pierrick Bouvier
0 siblings, 0 replies; 21+ messages in thread
From: Pierrick Bouvier @ 2024-07-05 0:33 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Eduardo Habkost
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.bouvier@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.henderson@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~
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/7] plugins: access values during a memory read/write
2024-07-02 18:44 [PATCH v4 0/7] plugins: access values during a memory read/write Pierrick Bouvier
` (6 preceding siblings ...)
2024-07-02 18:44 ` [PATCH v4 7/7] tests/tcg/x86_64: add test for plugin memory access Pierrick Bouvier
@ 2024-07-05 0:42 ` Pierrick Bouvier
7 siblings, 0 replies; 21+ messages in thread
From: Pierrick Bouvier @ 2024-07-05 0:42 UTC (permalink / raw)
To: qemu-devel
Cc: Alex Bennée, Mahmoud Mandour, Alexandre Iooss,
Philippe Mathieu-Daudé, Paolo Bonzini, Richard Henderson,
Eduardo Habkost
Posted v5.
On 7/2/24 11:44, Pierrick Bouvier wrote:
> This series allows plugins to know which value is read/written during a memory
> access.
>
> For every memory access, we know copy this value before calling mem callbacks,
> and those can query it using new API function:
> - qemu_plugin_mem_get_value
>
> Mem plugin was extended to print accesses, and a new test was added to check
> functionality work as expected. A bug was found where callbacks were not
> called as expected.
>
> This will open new use cases for plugins, such as following specific values in
> memory.
>
> v4
> - fix prototype for stubs qemu_plugin_vcpu_mem_cb (inverted low/high parameters
> names)
> - link gitlab bugs resolved (thanks @Anton Kochkov for reporting)
> https://gitlab.com/qemu-project/qemu/-/issues/1719
> https://gitlab.com/qemu-project/qemu/-/issues/2152
>
> v3
> - simplify API: return an algebraic data type for value accessed
> this can be easily extended when QEMU will support wider accesses
> - fix Makefile test (use quiet-command instead of manually run the command)
> - rename upper/lower to high/low
> - reorder functions parameters and code to low/high instead of high/low, to
> follow current convention in QEMU codebase
>
> v2
> - fix compilation on aarch64 (missing undef in accel/tcg/atomic_template.h)
>
> v3
> - add info when printing memory accesses (insn_vaddr,mem_vaddr,mem_hwaddr)
>
> Pierrick Bouvier (7):
> plugins: fix mem callback array size
> plugins: save value during memory accesses
> plugins: extend API to get latest memory value accessed
> tests/tcg: add mechanism to run specific tests with plugins
> tests/tcg: allow to check output of plugins
> tests/plugin/mem: add option to print memory accesses
> tests/tcg/x86_64: add test for plugin memory access
>
> accel/tcg/atomic_template.h | 66 +++++++++++++--
> include/qemu/plugin.h | 8 ++
> include/qemu/qemu-plugin.h | 32 ++++++++
> accel/tcg/plugin-gen.c | 3 +-
> plugins/api.c | 34 ++++++++
> plugins/core.c | 7 ++
> tcg/tcg-op-ldst.c | 72 +++++++++++++++--
> tests/plugin/mem.c | 69 +++++++++++++++-
> tests/tcg/x86_64/test-plugin-mem-access.c | 89 +++++++++++++++++++++
> accel/tcg/atomic_common.c.inc | 13 ++-
> accel/tcg/ldst_common.c.inc | 38 +++++----
> plugins/qemu-plugins.symbols | 1 +
> tests/tcg/Makefile.target | 10 ++-
> tests/tcg/x86_64/Makefile.target | 7 ++
> tests/tcg/x86_64/check-plugin-mem-access.sh | 48 +++++++++++
> 15 files changed, 462 insertions(+), 35 deletions(-)
> create mode 100644 tests/tcg/x86_64/test-plugin-mem-access.c
> create mode 100755 tests/tcg/x86_64/check-plugin-mem-access.sh
>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2024-07-05 0:43 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-02 18:44 [PATCH v4 0/7] plugins: access values during a memory read/write Pierrick Bouvier
2024-07-02 18:44 ` [PATCH v4 1/7] plugins: fix mem callback array size Pierrick Bouvier
2024-07-03 18:39 ` Richard Henderson
2024-07-02 18:44 ` [PATCH v4 2/7] plugins: save value during memory accesses Pierrick Bouvier
2024-07-03 18:58 ` Richard Henderson
2024-07-05 0:33 ` Pierrick Bouvier
2024-07-02 18:44 ` [PATCH v4 3/7] plugins: extend API to get latest memory value accessed Pierrick Bouvier
2024-07-04 16:27 ` Richard Henderson
2024-07-02 18:44 ` [PATCH v4 4/7] tests/tcg: add mechanism to run specific tests with plugins Pierrick Bouvier
2024-07-03 2:26 ` Xingtao Yao (Fujitsu) via
2024-07-02 18:44 ` [PATCH v4 5/7] tests/tcg: allow to check output of plugins Pierrick Bouvier
2024-07-03 2:30 ` Xingtao Yao (Fujitsu) via
2024-07-04 16:28 ` Richard Henderson
2024-07-02 18:44 ` [PATCH v4 6/7] tests/plugin/mem: add option to print memory accesses Pierrick Bouvier
2024-07-03 1:56 ` Xingtao Yao (Fujitsu) via
2024-07-04 23:30 ` Pierrick Bouvier
2024-07-04 16:30 ` Richard Henderson
2024-07-04 23:29 ` Pierrick Bouvier
2024-07-02 18:44 ` [PATCH v4 7/7] tests/tcg/x86_64: add test for plugin memory access Pierrick Bouvier
2024-07-03 1:42 ` Xingtao Yao (Fujitsu) via
2024-07-05 0:42 ` [PATCH v4 0/7] plugins: access values during a memory read/write Pierrick Bouvier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).