* [PATCH v6 0/7] plugins: access values during a memory read/write
@ 2024-07-06 19:13 Pierrick Bouvier
2024-07-06 19:13 ` [PATCH v6 1/7] plugins: fix mem callback array size Pierrick Bouvier
` (6 more replies)
0 siblings, 7 replies; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-06 19:13 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson,
Marcel Apfelbaum, Mahmoud Mandour, Alexandre Iooss,
Alex Bennée, Eduardo Habkost, Yanan Wang, Pierrick Bouvier
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 tracking specific values in
memory.
Needs review:
Patch 7: tests/tcg/x86_64: add test for plugin memory access
v6
- fix big endian offset for plugin_gen_mem_callbacks_i32
v5
- fixed width output for mem values in mem plugin
- move plugin_mem_value to CPUNegativeOffset
- tcg/tcg-op-ldst.c: only store word size mem access (do not set upper bits)
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/hw/core/cpu.h | 4 +
include/qemu/plugin.h | 4 +
include/qemu/qemu-plugin.h | 32 ++++++++
accel/tcg/plugin-gen.c | 3 +-
plugins/api.c | 33 ++++++++
plugins/core.c | 6 ++
tcg/tcg-op-ldst.c | 66 +++++++++++++--
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 +++++++++++
16 files changed, 455 insertions(+), 34 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] 23+ messages in thread
* [PATCH v6 1/7] plugins: fix mem callback array size
2024-07-06 19:13 [PATCH v6 0/7] plugins: access values during a memory read/write Pierrick Bouvier
@ 2024-07-06 19:13 ` Pierrick Bouvier
2024-07-08 9:27 ` Alex Bennée
2024-07-16 13:53 ` Alex Bennée
2024-07-06 19:13 ` [PATCH v6 2/7] plugins: save value during memory accesses Pierrick Bouvier
` (5 subsequent siblings)
6 siblings, 2 replies; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-06 19:13 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson,
Marcel Apfelbaum, Mahmoud Mandour, Alexandre Iooss,
Alex Bennée, Eduardo Habkost, Yanan Wang, Pierrick Bouvier,
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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
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] 23+ messages in thread
* [PATCH v6 2/7] plugins: save value during memory accesses
2024-07-06 19:13 [PATCH v6 0/7] plugins: access values during a memory read/write Pierrick Bouvier
2024-07-06 19:13 ` [PATCH v6 1/7] plugins: fix mem callback array size Pierrick Bouvier
@ 2024-07-06 19:13 ` Pierrick Bouvier
2024-07-08 10:55 ` Alex Bennée
2024-07-06 19:13 ` [PATCH v6 3/7] plugins: extend API to get latest memory value accessed Pierrick Bouvier
` (4 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-06 19:13 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson,
Marcel Apfelbaum, Mahmoud Mandour, Alexandre Iooss,
Alex Bennée, Eduardo Habkost, Yanan Wang, Pierrick Bouvier
Different code paths handle memory accesses:
- tcg generated code
- load/store helpers
- atomic helpers
This value is saved in cpu->neg.plugin_mem_value_{high,low}. Values are
written only for accessed word size (upper bits are not set).
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.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
accel/tcg/atomic_template.h | 66 ++++++++++++++++++++++++++++++-----
include/hw/core/cpu.h | 4 +++
include/qemu/plugin.h | 4 +++
plugins/core.c | 6 ++++
tcg/tcg-op-ldst.c | 66 +++++++++++++++++++++++++++++++----
accel/tcg/atomic_common.c.inc | 13 ++++++-
accel/tcg/ldst_common.c.inc | 38 ++++++++++++--------
7 files changed, 167 insertions(+), 30 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/hw/core/cpu.h b/include/hw/core/cpu.h
index a2c8536943f..e92cba049fd 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -350,6 +350,8 @@ typedef union IcountDecr {
* from CPUArchState, via small negative offsets.
* @can_do_io: True if memory-mapped IO is allowed.
* @plugin_mem_cbs: active plugin memory callbacks
+ * @plugin_mem_value_low: 64 lower bits of latest accessed mem value.
+ * @plugin_mem_value_high: 64 higher bits of latest accessed mem value.
*/
typedef struct CPUNegativeOffsetState {
CPUTLB tlb;
@@ -358,6 +360,8 @@ typedef struct CPUNegativeOffsetState {
* The callback pointer are accessed via TCG (see gen_empty_mem_helper).
*/
GArray *plugin_mem_cbs;
+ uint64_t plugin_mem_value_low;
+ uint64_t plugin_mem_value_high;
#endif
IcountDecr icount_decr;
bool can_do_io;
diff --git a/include/qemu/plugin.h b/include/qemu/plugin.h
index af5f9db4692..9726a9ebf36 100644
--- a/include/qemu/plugin.h
+++ b/include/qemu/plugin.h
@@ -167,6 +167,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);
@@ -251,6 +253,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 12c67b4b4eb..d353d38b8da 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -583,6 +583,8 @@ 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;
@@ -591,6 +593,10 @@ void qemu_plugin_vcpu_mem_cb(CPUState *cpu, uint64_t vaddr,
if (arr == NULL) {
return;
}
+
+ cpu->neg.plugin_mem_value_low = value_low;
+ cpu->neg.plugin_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..23dc807f119 100644
--- a/tcg/tcg-op-ldst.c
+++ b/tcg/tcg-op-ldst.c
@@ -148,11 +148,11 @@ 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,
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);
@@ -172,6 +172,54 @@ 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) {
+ tcg_gen_st_i32(val, tcg_env,
+ offsetof(CPUState, neg.plugin_mem_value_low) -
+ sizeof(CPUState) + (HOST_BIG_ENDIAN * 4));
+ plugin_gen_mem_callbacks(copy_addr, orig_addr, oi, rw);
+ }
+#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) {
+ tcg_gen_st_i64(val, tcg_env,
+ offsetof(CPUState, neg.plugin_mem_value_low) -
+ sizeof(CPUState));
+ plugin_gen_mem_callbacks(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) {
+ tcg_gen_st_i64(TCGV128_LOW(val), tcg_env,
+ offsetof(CPUState, neg.plugin_mem_value_low) -
+ sizeof(CPUState));
+ tcg_gen_st_i64(TCGV128_HIGH(val), tcg_env,
+ offsetof(CPUState, neg.plugin_mem_value_high) -
+ sizeof(CPUState));
+ plugin_gen_mem_callbacks(copy_addr, orig_addr, oi, rw);
+ }
#endif
}
@@ -203,7 +251,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 +320,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 +373,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 +446,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 +656,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 +773,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] 23+ messages in thread
* [PATCH v6 3/7] plugins: extend API to get latest memory value accessed
2024-07-06 19:13 [PATCH v6 0/7] plugins: access values during a memory read/write Pierrick Bouvier
2024-07-06 19:13 ` [PATCH v6 1/7] plugins: fix mem callback array size Pierrick Bouvier
2024-07-06 19:13 ` [PATCH v6 2/7] plugins: save value during memory accesses Pierrick Bouvier
@ 2024-07-06 19:13 ` Pierrick Bouvier
2024-07-08 10:56 ` Alex Bennée
2024-07-06 19:13 ` [PATCH v6 4/7] tests/tcg: add mechanism to run specific tests with plugins Pierrick Bouvier
` (3 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-06 19:13 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson,
Marcel Apfelbaum, Mahmoud Mandour, Alexandre Iooss,
Alex Bennée, Eduardo Habkost, Yanan Wang, Pierrick Bouvier,
Xingtao Yao
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
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
include/qemu/qemu-plugin.h | 32 ++++++++++++++++++++++++++++++++
plugins/api.c | 33 +++++++++++++++++++++++++++++++++
plugins/qemu-plugins.symbols | 1 +
3 files changed, 66 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..3316d4a04d4 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -351,6 +351,39 @@ 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->neg.plugin_mem_value_low;
+ 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 = current_cpu->neg.plugin_mem_value_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] 23+ messages in thread
* [PATCH v6 4/7] tests/tcg: add mechanism to run specific tests with plugins
2024-07-06 19:13 [PATCH v6 0/7] plugins: access values during a memory read/write Pierrick Bouvier
` (2 preceding siblings ...)
2024-07-06 19:13 ` [PATCH v6 3/7] plugins: extend API to get latest memory value accessed Pierrick Bouvier
@ 2024-07-06 19:13 ` Pierrick Bouvier
2024-07-08 11:00 ` Alex Bennée
2024-07-06 19:13 ` [PATCH v6 5/7] tests/tcg: allow to check output of plugins Pierrick Bouvier
` (2 subsequent siblings)
6 siblings, 1 reply; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-06 19:13 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson,
Marcel Apfelbaum, Mahmoud Mandour, Alexandre Iooss,
Alex Bennée, Eduardo Habkost, Yanan Wang, Pierrick Bouvier,
Xingtao Yao
Only multiarch tests are run with plugins, and we want to be able to run
per-arch test with plugins too.
Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
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 cb8cfeb6dac..52616544d52 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] 23+ messages in thread
* [PATCH v6 5/7] tests/tcg: allow to check output of plugins
2024-07-06 19:13 [PATCH v6 0/7] plugins: access values during a memory read/write Pierrick Bouvier
` (3 preceding siblings ...)
2024-07-06 19:13 ` [PATCH v6 4/7] tests/tcg: add mechanism to run specific tests with plugins Pierrick Bouvier
@ 2024-07-06 19:13 ` Pierrick Bouvier
2024-07-08 16:06 ` Alex Bennée
2024-07-06 19:13 ` [PATCH v6 6/7] tests/plugin/mem: add option to print memory accesses Pierrick Bouvier
2024-07-06 19:13 ` [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access Pierrick Bouvier
6 siblings, 1 reply; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-06 19:13 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson,
Marcel Apfelbaum, Mahmoud Mandour, Alexandre Iooss,
Alex Bennée, Eduardo Habkost, Yanan Wang, Pierrick Bouvier,
Xingtao Yao
A specific plugin test can now read and check a plugin output, to ensure
it contains expected values.
Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
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 52616544d52..6f50b0176ea 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] 23+ messages in thread
* [PATCH v6 6/7] tests/plugin/mem: add option to print memory accesses
2024-07-06 19:13 [PATCH v6 0/7] plugins: access values during a memory read/write Pierrick Bouvier
` (4 preceding siblings ...)
2024-07-06 19:13 ` [PATCH v6 5/7] tests/tcg: allow to check output of plugins Pierrick Bouvier
@ 2024-07-06 19:13 ` Pierrick Bouvier
2024-07-06 19:13 ` [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access Pierrick Bouvier
6 siblings, 0 replies; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-06 19:13 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson,
Marcel Apfelbaum, Mahmoud Mandour, Alexandre Iooss,
Alex Bennée, Eduardo Habkost, Yanan Wang, Pierrick Bouvier,
Xingtao Yao
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.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
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..086e6f5bdfc 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%02"PRIx8, value.data.u8);
+ break;
+ case QEMU_PLUGIN_MEM_VALUE_U16:
+ g_string_append_printf(out, "0x%04"PRIx16, value.data.u16);
+ break;
+ case QEMU_PLUGIN_MEM_VALUE_U32:
+ g_string_append_printf(out, "0x%08"PRIx32, value.data.u32);
+ break;
+ case QEMU_PLUGIN_MEM_VALUE_U64:
+ g_string_append_printf(out, "0x%016"PRIx64, value.data.u64);
+ break;
+ case QEMU_PLUGIN_MEM_VALUE_U128:
+ g_string_append_printf(out, "0x%016"PRIx64"%016"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] 23+ messages in thread
* [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access
2024-07-06 19:13 [PATCH v6 0/7] plugins: access values during a memory read/write Pierrick Bouvier
` (5 preceding siblings ...)
2024-07-06 19:13 ` [PATCH v6 6/7] tests/plugin/mem: add option to print memory accesses Pierrick Bouvier
@ 2024-07-06 19:13 ` Pierrick Bouvier
2024-07-07 18:25 ` Richard Henderson
2024-07-08 19:15 ` Alex Bennée
6 siblings, 2 replies; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-06 19:13 UTC (permalink / raw)
To: qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Richard Henderson,
Marcel Apfelbaum, Mahmoud Mandour, Alexandre Iooss,
Alex Bennée, Eduardo Habkost, Yanan Wang, Pierrick Bouvier,
Xingtao Yao
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
Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
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 eda9bd7396c..3edc29b924d 100644
--- a/tests/tcg/x86_64/Makefile.target
+++ b/tests/tcg/x86_64/Makefile.target
@@ -16,6 +16,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)
@@ -26,6 +27,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..163f1cfad34
--- /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,0x0042
+,atomic_op_u16,.*,16,store,0xf123
+,load_u16,.*,16,load,0xf123
+,store_u32,.*,32,store,0xff112233
+,atomic_op_u32,.*,32,load,0x00000042
+,atomic_op_u32,.*,32,store,0xff112233
+,load_u32,.*,32,load,0xff112233
+,store_u64,.*,64,store,0xf123456789abcdef
+,atomic_op_u64,.*,64,load,0x0000000000000042
+,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] 23+ messages in thread
* Re: [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access
2024-07-06 19:13 ` [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access Pierrick Bouvier
@ 2024-07-07 18:25 ` Richard Henderson
2024-07-12 0:41 ` Pierrick Bouvier
2024-07-08 19:15 ` Alex Bennée
1 sibling, 1 reply; 23+ messages in thread
From: Richard Henderson @ 2024-07-07 18:25 UTC (permalink / raw)
To: Pierrick Bouvier, qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Marcel Apfelbaum,
Mahmoud Mandour, Alexandre Iooss, Alex Bennée,
Eduardo Habkost, Yanan Wang, Xingtao Yao
On 7/6/24 12:13, Pierrick Bouvier wrote:
> +++ b/tests/tcg/x86_64/test-plugin-mem-access.c
> @@ -0,0 +1,89 @@
> +#include <emmintrin.h>
> +#include <pthread.h>
All new files should have license boilerplate and description.
You can use spdx to limit to just a couple of lines.
r~
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 1/7] plugins: fix mem callback array size
2024-07-06 19:13 ` [PATCH v6 1/7] plugins: fix mem callback array size Pierrick Bouvier
@ 2024-07-08 9:27 ` Alex Bennée
2024-07-16 13:53 ` Alex Bennée
1 sibling, 0 replies; 23+ messages in thread
From: Alex Bennée @ 2024-07-08 9:27 UTC (permalink / raw)
To: Pierrick Bouvier
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang, Xingtao Yao
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/7] plugins: save value during memory accesses
2024-07-06 19:13 ` [PATCH v6 2/7] plugins: save value during memory accesses Pierrick Bouvier
@ 2024-07-08 10:55 ` Alex Bennée
0 siblings, 0 replies; 23+ messages in thread
From: Alex Bennée @ 2024-07-08 10:55 UTC (permalink / raw)
To: Pierrick Bouvier
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> Different code paths handle memory accesses:
> - tcg generated code
> - load/store helpers
> - atomic helpers
>
> This value is saved in cpu->neg.plugin_mem_value_{high,low}. Values are
> written only for accessed word size (upper bits are not set).
>
> 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.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 3/7] plugins: extend API to get latest memory value accessed
2024-07-06 19:13 ` [PATCH v6 3/7] plugins: extend API to get latest memory value accessed Pierrick Bouvier
@ 2024-07-08 10:56 ` Alex Bennée
0 siblings, 0 replies; 23+ messages in thread
From: Alex Bennée @ 2024-07-08 10:56 UTC (permalink / raw)
To: Pierrick Bouvier
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang, Xingtao Yao
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 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
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 4/7] tests/tcg: add mechanism to run specific tests with plugins
2024-07-06 19:13 ` [PATCH v6 4/7] tests/tcg: add mechanism to run specific tests with plugins Pierrick Bouvier
@ 2024-07-08 11:00 ` Alex Bennée
2024-07-12 0:24 ` Pierrick Bouvier
0 siblings, 1 reply; 23+ messages in thread
From: Alex Bennée @ 2024-07-08 11:00 UTC (permalink / raw)
To: Pierrick Bouvier
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang, Xingtao Yao
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> Only multiarch tests are run with plugins, and we want to be able to run
> per-arch test with plugins too.
>
> Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
> 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 cb8cfeb6dac..52616544d52 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
I have no particular objection to adding this (except a minor nit of
maybe the name should be ADDITIONAL_PLUGIN_TESTS). However the use of
this later is for the test:
tests/tcg/x86_64/test-plugin-mem-access.c
and aside from the inline asm I don't see why this couldn't be a
multi-arch test. Could we not use the atomic primitives to make it multiarch?
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 5/7] tests/tcg: allow to check output of plugins
2024-07-06 19:13 ` [PATCH v6 5/7] tests/tcg: allow to check output of plugins Pierrick Bouvier
@ 2024-07-08 16:06 ` Alex Bennée
2024-07-12 0:36 ` Pierrick Bouvier
0 siblings, 1 reply; 23+ messages in thread
From: Alex Bennée @ 2024-07-08 16:06 UTC (permalink / raw)
To: Pierrick Bouvier
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang, Xingtao Yao
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> A specific plugin test can now read and check a plugin output, to ensure
> it contains expected values.
>
> Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 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 52616544d52..6f50b0176ea 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,$<))
No need to have a null test, we can wrap the call in an if:
--8<---------------cut here---------------start------------->8---
modified tests/tcg/Makefile.target
@@ -90,7 +90,7 @@ CFLAGS=
LDFLAGS=
QEMU_OPTS=
-CHECK_PLUGIN_OUTPUT_COMMAND=true
+CHECK_PLUGIN_OUTPUT_COMMAND=
# If TCG debugging, or TCI is enabled things are a lot slower
@@ -181,9 +181,10 @@ 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,$<))
+ $(if $(CHECK_PLUGIN_OUTPUT_COMMAND), \
+ $(call quiet-command, $(CHECK_PLUGIN_OUTPUT_COMMAND) $*.pout, \
+ TEST, check plugin $(call extract-plugin,$@) output \
+ with $(call strip-plugin,$<)))
else
run-%: %
$(call run-test, $<, \
@@ -198,9 +199,10 @@ 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,$<))
+ $(if $(CHECK_PLUGIN_OUTPUT_COMMAND), \
+ $(call quiet-command, $(CHECK_PLUGIN_OUTPUT_COMMAND) $*.pout, \
+ TEST, check plugin $(call extract-plugin,$@) output \
+ with $(call strip-plugin,$<)))
--8<---------------cut here---------------end--------------->8---
> endif
>
> gdb-%: %
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access
2024-07-06 19:13 ` [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access Pierrick Bouvier
2024-07-07 18:25 ` Richard Henderson
@ 2024-07-08 19:15 ` Alex Bennée
2024-07-12 0:48 ` Pierrick Bouvier
1 sibling, 1 reply; 23+ messages in thread
From: Alex Bennée @ 2024-07-08 19:15 UTC (permalink / raw)
To: Pierrick Bouvier
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang, Xingtao Yao
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 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
>
> Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
> 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); \
Should we exercise the other compare and swap ops? Do they all come
through the same rwm path?
> +}
> +
> +#define DEFINE_LOAD(name, type) \
> +static void load_##name(void) \
> +{ \
> + register type var asm("eax") = *((type *) data); \
> + (void)var; \
This is a bit weird. It's the only inline asm needed that makes this a
non-multiarch test. Why?
> +}
> +
> +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 eda9bd7396c..3edc29b924d 100644
> --- a/tests/tcg/x86_64/Makefile.target
> +++ b/tests/tcg/x86_64/Makefile.target
> @@ -16,6 +16,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)
> @@ -26,6 +27,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..163f1cfad34
> --- /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,0x0042
> +,atomic_op_u16,.*,16,store,0xf123
> +,load_u16,.*,16,load,0xf123
> +,store_u32,.*,32,store,0xff112233
> +,atomic_op_u32,.*,32,load,0x00000042
> +,atomic_op_u32,.*,32,store,0xff112233
> +,load_u32,.*,32,load,0xff112233
> +,store_u64,.*,64,store,0xf123456789abcdef
> +,atomic_op_u64,.*,64,load,0x0000000000000042
> +,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
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 4/7] tests/tcg: add mechanism to run specific tests with plugins
2024-07-08 11:00 ` Alex Bennée
@ 2024-07-12 0:24 ` Pierrick Bouvier
0 siblings, 0 replies; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-12 0:24 UTC (permalink / raw)
To: Alex Bennée
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang, Xingtao Yao
On 7/8/24 04:00, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>
>> Only multiarch tests are run with plugins, and we want to be able to run
>> per-arch test with plugins too.
>>
>> Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
>> 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 cb8cfeb6dac..52616544d52 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
>
> I have no particular objection to adding this (except a minor nit of
> maybe the name should be ADDITIONAL_PLUGIN_TESTS). However the use of
> this later is for the test:
>
I'll rename it to ADDITIONAL_PLUGIN_TESTS.
> tests/tcg/x86_64/test-plugin-mem-access.c
>
> and aside from the inline asm I don't see why this couldn't be a
> multi-arch test. Could we not use the atomic primitives to make it multiarch?
>
Will answer on related patch.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 5/7] tests/tcg: allow to check output of plugins
2024-07-08 16:06 ` Alex Bennée
@ 2024-07-12 0:36 ` Pierrick Bouvier
0 siblings, 0 replies; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-12 0:36 UTC (permalink / raw)
To: Alex Bennée
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang, Xingtao Yao
On 7/8/24 09:06, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>
>> A specific plugin test can now read and check a plugin output, to ensure
>> it contains expected values.
>>
>> Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> 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 52616544d52..6f50b0176ea 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,$<))
>
> No need to have a null test, we can wrap the call in an if:
>
> --8<---------------cut here---------------start------------->8---
> modified tests/tcg/Makefile.target
> @@ -90,7 +90,7 @@ CFLAGS=
> LDFLAGS=
>
> QEMU_OPTS=
> -CHECK_PLUGIN_OUTPUT_COMMAND=true
> +CHECK_PLUGIN_OUTPUT_COMMAND=
>
>
> # If TCG debugging, or TCI is enabled things are a lot slower
> @@ -181,9 +181,10 @@ 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,$<))
> + $(if $(CHECK_PLUGIN_OUTPUT_COMMAND), \
> + $(call quiet-command, $(CHECK_PLUGIN_OUTPUT_COMMAND) $*.pout, \
> + TEST, check plugin $(call extract-plugin,$@) output \
> + with $(call strip-plugin,$<)))
> else
> run-%: %
> $(call run-test, $<, \
> @@ -198,9 +199,10 @@ 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,$<))
> + $(if $(CHECK_PLUGIN_OUTPUT_COMMAND), \
> + $(call quiet-command, $(CHECK_PLUGIN_OUTPUT_COMMAND) $*.pout, \
> + TEST, check plugin $(call extract-plugin,$@) output \
> + with $(call strip-plugin,$<)))
> --8<---------------cut here---------------end--------------->8---
>
>> endif
>>
>> gdb-%: %
>
Thanks, I was looking exactly for this construction initially! Will
change to that.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access
2024-07-07 18:25 ` Richard Henderson
@ 2024-07-12 0:41 ` Pierrick Bouvier
0 siblings, 0 replies; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-12 0:41 UTC (permalink / raw)
To: Richard Henderson, qemu-devel
Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Marcel Apfelbaum,
Mahmoud Mandour, Alexandre Iooss, Alex Bennée,
Eduardo Habkost, Yanan Wang, Xingtao Yao
On 7/7/24 11:25, Richard Henderson wrote:
> On 7/6/24 12:13, Pierrick Bouvier wrote:
>> +++ b/tests/tcg/x86_64/test-plugin-mem-access.c
>> @@ -0,0 +1,89 @@
>> +#include <emmintrin.h>
>> +#include <pthread.h>
>
> All new files should have license boilerplate and description.
> You can use spdx to limit to just a couple of lines.
>
Added this.
>
> r~
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access
2024-07-08 19:15 ` Alex Bennée
@ 2024-07-12 0:48 ` Pierrick Bouvier
2024-07-12 14:51 ` Alex Bennée
0 siblings, 1 reply; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-12 0:48 UTC (permalink / raw)
To: Alex Bennée
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang, Xingtao Yao
On 7/8/24 12:15, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>
>> 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
>>
>> Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
>> 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); \
>
> Should we exercise the other compare and swap ops? Do they all come
> through the same rwm path?
>
There are definitely several paths depending on the generated atomic op.
However, the code is pretty straightforward (and implemented in a single
function), so my thought was that one way to trigger this was enough.
>> +}
>> +
>> +#define DEFINE_LOAD(name, type) \
>> +static void load_##name(void) \
>> +{ \
>> + register type var asm("eax") = *((type *) data); \
>> + (void)var; \
>
> This is a bit weird. It's the only inline asm needed that makes this a
> non-multiarch test. Why?
>
I'll answer here about why this test is arch specific, and not a multi arch.
The problem I met is that all target architecture do not have native
64/128 bits types, and depending how code is compiled with gcc, you may
or not generated expected vector instructions for 128bits operation.
Same for atomic operations.
Thus, I chose to specialize this test for x86_64, and use sse2 directly
for 128 bits integers.
You might say "How about adding ifdef for this". Yes sure, but the check
script would become complicated too, and I just wanted to keep it
simple. Our interest here is not to check that memory accesses are
correctly implemented in QEMU, but to check that a specific behavior on
one arch is the one expected.
Does it make more sense for you?
>> +}
>> +
>> +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 eda9bd7396c..3edc29b924d 100644
>> --- a/tests/tcg/x86_64/Makefile.target
>> +++ b/tests/tcg/x86_64/Makefile.target
>> @@ -16,6 +16,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)
>> @@ -26,6 +27,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..163f1cfad34
>> --- /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,0x0042
>> +,atomic_op_u16,.*,16,store,0xf123
>> +,load_u16,.*,16,load,0xf123
>> +,store_u32,.*,32,store,0xff112233
>> +,atomic_op_u32,.*,32,load,0x00000042
>> +,atomic_op_u32,.*,32,store,0xff112233
>> +,load_u32,.*,32,load,0xff112233
>> +,store_u64,.*,64,store,0xf123456789abcdef
>> +,atomic_op_u64,.*,64,load,0x0000000000000042
>> +,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
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access
2024-07-12 0:48 ` Pierrick Bouvier
@ 2024-07-12 14:51 ` Alex Bennée
2024-07-12 17:14 ` Pierrick Bouvier
0 siblings, 1 reply; 23+ messages in thread
From: Alex Bennée @ 2024-07-12 14:51 UTC (permalink / raw)
To: Pierrick Bouvier
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang, Xingtao Yao
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> On 7/8/24 12:15, Alex Bennée wrote:
>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>
>>> 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
>>>
>>> Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
>>> 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); \
>> Should we exercise the other compare and swap ops? Do they all come
>> through the same rwm path?
>>
>
> There are definitely several paths depending on the generated atomic op.
> However, the code is pretty straightforward (and implemented in a
> single function), so my thought was that one way to trigger this was
> enough.
If they all come through the same path I guess that's OK.
>>> +}
>>> +
>>> +#define DEFINE_LOAD(name, type) \
>>> +static void load_##name(void) \
>>> +{ \
>>> + register type var asm("eax") = *((type *) data); \
>>> + (void)var; \
>> This is a bit weird. It's the only inline asm needed that makes this
>> a
>> non-multiarch test. Why?
>>
>
> I'll answer here about why this test is arch specific, and not a multi arch.
>
> The problem I met is that all target architecture do not have native
> 64/128 bits types, and depending how code is compiled with gcc, you
> may or not generated expected vector instructions for 128bits
> operation. Same for atomic operations.
So we do handle this with the sha512 test, building variants of it with
various compiler flags to trigger the use of vectors. So the code is
multiarch but we have arch specific variants as dictated by the
Makefiles, i.e.:
sha512-sve: CFLAGS=-O3 -march=armv8.1-a+sve
sha512-sve: sha512.c
$(CC) $(CFLAGS) $(EXTRA_CFLAGS) $< -o $@ $(LDFLAGS)
TESTS += sha512-sve
> Thus, I chose to specialize this test for x86_64, and use sse2
> directly for 128 bits integers.
>
> You might say "How about adding ifdef for this". Yes sure, but the
> check script would become complicated too, and I just wanted to keep
> it simple.
We can keep the check-script per arch if we have to.
> Our interest here is not to check that memory accesses are
> correctly implemented in QEMU, but to check that a specific behavior
> on one arch is the one expected.
So the problem with not being multiarch is we don't build all targets in
all combinations. To limit CI time we often build a subset and now this
particular subset won't test the plugin paths.
>
> Does it make more sense for you?
>
<snip>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access
2024-07-12 14:51 ` Alex Bennée
@ 2024-07-12 17:14 ` Pierrick Bouvier
2024-07-19 1:01 ` Pierrick Bouvier
0 siblings, 1 reply; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-12 17:14 UTC (permalink / raw)
To: Alex Bennée
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang, Xingtao Yao
On 7/12/24 07:51, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>
>> On 7/8/24 12:15, Alex Bennée wrote:
>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>
>>>> 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
>>>>
>>>> Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
>>>> 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); \
>>> Should we exercise the other compare and swap ops? Do they all come
>>> through the same rwm path?
>>>
>>
>> There are definitely several paths depending on the generated atomic op.
>> However, the code is pretty straightforward (and implemented in a
>> single function), so my thought was that one way to trigger this was
>> enough.
>
> If they all come through the same path I guess that's OK.
>
>>>> +}
>>>> +
>>>> +#define DEFINE_LOAD(name, type) \
>>>> +static void load_##name(void) \
>>>> +{ \
>>>> + register type var asm("eax") = *((type *) data); \
>>>> + (void)var; \
>>> This is a bit weird. It's the only inline asm needed that makes this
>>> a
>>> non-multiarch test. Why?
>>>
>>
>> I'll answer here about why this test is arch specific, and not a multi arch.
>>
>> The problem I met is that all target architecture do not have native
>> 64/128 bits types, and depending how code is compiled with gcc, you
>> may or not generated expected vector instructions for 128bits
>> operation. Same for atomic operations.
>
> So we do handle this with the sha512 test, building variants of it with
> various compiler flags to trigger the use of vectors. So the code is
> multiarch but we have arch specific variants as dictated by the
> Makefiles, i.e.:
>
> sha512-sve: CFLAGS=-O3 -march=armv8.1-a+sve
> sha512-sve: sha512.c
> $(CC) $(CFLAGS) $(EXTRA_CFLAGS) $< -o $@ $(LDFLAGS)
>
> TESTS += sha512-sve
>
I suspect this is gonna need several iterations to try all arch, and see
which ones have native 64/128 bits and which ones have atomic
instructions. Is that really where we want to spend our (precious) time?
I'm not convinced of the value of that. We try to test plugins
implementation, not how QEMU handles memory accesses in general.
The specificity of this test, is what we don't test the correct output
of a program, but we observe an expected behavior, via the plugins
trace. So it's a bit different from sha512 example.
>> Thus, I chose to specialize this test for x86_64, and use sse2
>> directly for 128 bits integers.
>>
>> You might say "How about adding ifdef for this". Yes sure, but the
>> check script would become complicated too, and I just wanted to keep
>> it simple.
>
> We can keep the check-script per arch if we have to.
>
I would add a target arch param, but not duplicate this to be honest.
Will be a pain to update if needed.
My goal was to replace this with LLVM filecheck in a following series.
>> Our interest here is not to check that memory accesses are
>> correctly implemented in QEMU, but to check that a specific behavior
>> on one arch is the one expected.
>
> So the problem with not being multiarch is we don't build all targets in
> all combinations. To limit CI time we often build a subset and now this
> particular subset won't test the plugin paths.
>
Ok. Is linux-user-x86_64 frequently skipped?
I could add support for aarch64 too, if you think it's worth it. I
suspect we always have at least one of the two arch that is running in CI.
>>
>> Does it make more sense for you?
>>
> <snip>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 1/7] plugins: fix mem callback array size
2024-07-06 19:13 ` [PATCH v6 1/7] plugins: fix mem callback array size Pierrick Bouvier
2024-07-08 9:27 ` Alex Bennée
@ 2024-07-16 13:53 ` Alex Bennée
1 sibling, 0 replies; 23+ messages in thread
From: Alex Bennée @ 2024-07-16 13:53 UTC (permalink / raw)
To: Pierrick Bouvier
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang, Xingtao Yao
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
I'm queuing this patch to plugins/next as it is a fix.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access
2024-07-12 17:14 ` Pierrick Bouvier
@ 2024-07-19 1:01 ` Pierrick Bouvier
0 siblings, 0 replies; 23+ messages in thread
From: Pierrick Bouvier @ 2024-07-19 1:01 UTC (permalink / raw)
To: Alex Bennée
Cc: qemu-devel, Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson, Marcel Apfelbaum, Mahmoud Mandour,
Alexandre Iooss, Eduardo Habkost, Yanan Wang, Xingtao Yao
On 7/12/24 10:14, Pierrick Bouvier wrote:
> On 7/12/24 07:51, Alex Bennée wrote:
>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>
>>> On 7/8/24 12:15, Alex Bennée wrote:
>>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>>
>>>>> 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
>>>>>
>>>>> Tested-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>
>>>>> 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); \
>>>> Should we exercise the other compare and swap ops? Do they all come
>>>> through the same rwm path?
>>>>
>>>
>>> There are definitely several paths depending on the generated atomic op.
>>> However, the code is pretty straightforward (and implemented in a
>>> single function), so my thought was that one way to trigger this was
>>> enough.
>>
>> If they all come through the same path I guess that's OK.
>>
>>>>> +}
>>>>> +
>>>>> +#define DEFINE_LOAD(name, type) \
>>>>> +static void load_##name(void) \
>>>>> +{ \
>>>>> + register type var asm("eax") = *((type *) data); \
>>>>> + (void)var; \
>>>> This is a bit weird. It's the only inline asm needed that makes this
>>>> a
>>>> non-multiarch test. Why?
>>>>
>>>
>>> I'll answer here about why this test is arch specific, and not a multi arch.
>>>
>>> The problem I met is that all target architecture do not have native
>>> 64/128 bits types, and depending how code is compiled with gcc, you
>>> may or not generated expected vector instructions for 128bits
>>> operation. Same for atomic operations.
>>
>> So we do handle this with the sha512 test, building variants of it with
>> various compiler flags to trigger the use of vectors. So the code is
>> multiarch but we have arch specific variants as dictated by the
>> Makefiles, i.e.:
>>
>> sha512-sve: CFLAGS=-O3 -march=armv8.1-a+sve
>> sha512-sve: sha512.c
>> $(CC) $(CFLAGS) $(EXTRA_CFLAGS) $< -o $@ $(LDFLAGS)
>>
>> TESTS += sha512-sve
>>
>
> I suspect this is gonna need several iterations to try all arch, and see
> which ones have native 64/128 bits and which ones have atomic
> instructions. Is that really where we want to spend our (precious) time?
> I'm not convinced of the value of that. We try to test plugins
> implementation, not how QEMU handles memory accesses in general.
>
> The specificity of this test, is what we don't test the correct output
> of a program, but we observe an expected behavior, via the plugins
> trace. So it's a bit different from sha512 example.
>
>>> Thus, I chose to specialize this test for x86_64, and use sse2
>>> directly for 128 bits integers.
>>>
>>> You might say "How about adding ifdef for this". Yes sure, but the
>>> check script would become complicated too, and I just wanted to keep
>>> it simple.
>>
>> We can keep the check-script per arch if we have to.
>>
>
> I would add a target arch param, but not duplicate this to be honest.
> Will be a pain to update if needed.
> My goal was to replace this with LLVM filecheck in a following series.
>
>>> Our interest here is not to check that memory accesses are
>>> correctly implemented in QEMU, but to check that a specific behavior
>>> on one arch is the one expected.
>>
>> So the problem with not being multiarch is we don't build all targets in
>> all combinations. To limit CI time we often build a subset and now this
>> particular subset won't test the plugin paths.
>>
>
> Ok. Is linux-user-x86_64 frequently skipped?
> I could add support for aarch64 too, if you think it's worth it. I
> suspect we always have at least one of the two arch that is running in CI.
>
>>>
>>> Does it make more sense for you?
>>>
>> <snip>
>>
After discussing with Alex, our goal will be to have a multiarch test.
I'll do this for the next version.
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2024-07-19 1:02 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-06 19:13 [PATCH v6 0/7] plugins: access values during a memory read/write Pierrick Bouvier
2024-07-06 19:13 ` [PATCH v6 1/7] plugins: fix mem callback array size Pierrick Bouvier
2024-07-08 9:27 ` Alex Bennée
2024-07-16 13:53 ` Alex Bennée
2024-07-06 19:13 ` [PATCH v6 2/7] plugins: save value during memory accesses Pierrick Bouvier
2024-07-08 10:55 ` Alex Bennée
2024-07-06 19:13 ` [PATCH v6 3/7] plugins: extend API to get latest memory value accessed Pierrick Bouvier
2024-07-08 10:56 ` Alex Bennée
2024-07-06 19:13 ` [PATCH v6 4/7] tests/tcg: add mechanism to run specific tests with plugins Pierrick Bouvier
2024-07-08 11:00 ` Alex Bennée
2024-07-12 0:24 ` Pierrick Bouvier
2024-07-06 19:13 ` [PATCH v6 5/7] tests/tcg: allow to check output of plugins Pierrick Bouvier
2024-07-08 16:06 ` Alex Bennée
2024-07-12 0:36 ` Pierrick Bouvier
2024-07-06 19:13 ` [PATCH v6 6/7] tests/plugin/mem: add option to print memory accesses Pierrick Bouvier
2024-07-06 19:13 ` [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access Pierrick Bouvier
2024-07-07 18:25 ` Richard Henderson
2024-07-12 0:41 ` Pierrick Bouvier
2024-07-08 19:15 ` Alex Bennée
2024-07-12 0:48 ` Pierrick Bouvier
2024-07-12 14:51 ` Alex Bennée
2024-07-12 17:14 ` Pierrick Bouvier
2024-07-19 1:01 ` 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).