Re: [PATCH v6 7/7] tests/tcg/x86_64: add test for plugin memory access

[Pierrick Bouvier]

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