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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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.