Re: [Qemu-devel] [PATCH RFC 2/2] tests/tcg: target/s390: Add test for VECTOR LOAD GR FROM VR ELEMENT

[Alex Bennée]

David Hildenbrand <address@hidden> writes:

> On 27.02.19 12:14, David Hildenbrand wrote:
>> We want to make use of vectors, so use -march=z13. To make it compile,
>> use a reasonable optimization level (-O2), which seems to work just fine
>> with all tests.
>>
>> Add some infrastructure for checking if SIGILL will be properly
>> triggered.
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  tests/tcg/s390x/Makefile.target     |  3 ++-
>>  tests/tcg/s390x/helper.h            | 28 +++++++++++++++++++++
>>  tests/tcg/s390x/signal_helper.inc.c | 39 +++++++++++++++++++++++++++++
>>  tests/tcg/s390x/vlgv.c              | 37 +++++++++++++++++++++++++++
>>  4 files changed, 106 insertions(+), 1 deletion(-)
>>  create mode 100644 tests/tcg/s390x/helper.h
>>  create mode 100644 tests/tcg/s390x/signal_helper.inc.c
>>  create mode 100644 tests/tcg/s390x/vlgv.c
>>
>> diff --git a/tests/tcg/s390x/Makefile.target 
>> b/tests/tcg/s390x/Makefile.target
>> index 151dc075aa..d1ae755ab9 100644
>> --- a/tests/tcg/s390x/Makefile.target
>> +++ b/tests/tcg/s390x/Makefile.target
>> @@ -1,8 +1,9 @@
>>  VPATH+=$(SRC_PATH)/tests/tcg/s390x
>> -CFLAGS+=-march=zEC12 -m64
>> +CFLAGS+=-march=z13 -m64 -O2
>>  TESTS+=hello-s390x
>>  TESTS+=csst
>>  TESTS+=ipm
>>  TESTS+=exrl-trt
>>  TESTS+=exrl-trtr
>>  TESTS+=pack
>> +TESTS+=vlgv
>> diff --git a/tests/tcg/s390x/helper.h b/tests/tcg/s390x/helper.h
>> new file mode 100644
>> index 0000000000..845b8bb504
>> --- /dev/null
>> +++ b/tests/tcg/s390x/helper.h
>> @@ -0,0 +1,28 @@
>> +#ifndef TEST_TCG_S390x_VECTOR_H
>> +#define TEST_TCG_S390x_VECTOR_H
>> +
>> +#include <stdint.h>
>> +
>> +#define ES_8    0
>> +#define ES_16   1
>> +#define ES_32   2
>> +#define ES_64   3
>> +#define ES_128  4
>> +
>> +typedef union S390Vector {
>> +    __uint128_t v;
>> +    uint64_t q[2];
>> +    uint32_t d[4];
>> +    uint16_t w[8];
>> +    uint8_t h[16];
>> +} S390Vector;
>> +
>> +static inline void check(const char *s, bool cond)
>> +{
>> +    if (!cond) {
>> +        fprintf(stderr, "Check failed: %s\n", s);
>> +        exit(-1);
>> +    }
>> +}
>> +
>> +#endif /* TEST_TCG_S390x_VECTOR_H */
>> diff --git a/tests/tcg/s390x/signal_helper.inc.c 
>> b/tests/tcg/s390x/signal_helper.inc.c
>> new file mode 100644
>> index 0000000000..5bd69ca76a
>> --- /dev/null
>> +++ b/tests/tcg/s390x/signal_helper.inc.c
>> @@ -0,0 +1,39 @@
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <stdbool.h>
>> +#include <signal.h>
>> +#include <setjmp.h>
>> +#include "helper.h"
>> +
>> +jmp_buf jmp_env;
>> +
>> +static void sig_sigill(int sig)
>> +{
>> +    if (sig != SIGILL) {
>> +        check("Wrong signal received", false);
>> +    }
>> +    longjmp(jmp_env, 1);
>> +}
>> +
>> +#define CHECK_SIGILL(STATEMENT)                             \
>> +do {                                                        \
>> +    struct sigaction act;                                   \
>> +                                                            \
>> +    act.sa_handler = sig_sigill;                            \
>> +    act.sa_flags = 0;                                       \
>> +    if (sigaction(SIGILL, &act, NULL)) {                    \
>> +        check("SIGILL handler not registered", false);      \
>> +    }                                                       \
>> +                                                            \
>> +    if (setjmp(jmp_env) == 0) {                             \
>> +        STATEMENT;                                          \
>> +        check("SIGILL not triggered", false);               \
>> +    }                                                       \
>> +                                                            \
>> +    act.sa_handler = SIG_DFL;                               \
>> +    sigemptyset(&act.sa_mask);                              \
>> +    act.sa_flags = 0;                                       \
>> +    if (sigaction(SIGILL, &act, NULL)) {                    \
>> +        check("SIGILL handler not unregistered", false);    \
>> +    }                                                       \
>> +} while (0)
>
> Now this is some interesting hackery.
>
> I changed it to
>
> jmp_buf jmp_env;
>
> static void sig_sigill(int sig)
> {
>     if (sig != SIGILL) {
>         check("Wrong signal received", false);
>     }
>     longjmp(jmp_env, 1);
> }
>
> #define CHECK_SIGILL(STATEMENT)                  \
> do {                                             \
>     if (signal(SIGILL, sig_sigill) == SIG_ERR) { \
>         check("SIGILL not registered", false);   \
>     }                                            \
>     if (setjmp(jmp_env) == 0) {                  \
>         STATEMENT;                               \
>         check("SIGILL not triggered", false);    \
>     }                                            \
>     if (signal(SIGILL, SIG_DFL) == SIG_ERR) {    \
>         check("SIGILL not registered", false);   \
>     }                                            \
> } while (0)
>
>
> However it only works once. During the second signal, QEMU decides to
> set the default handler.
>
> 1. In a signal handler that signal is blocked. We leave that handler via
> a longjump. So after the first signal, the signal is blocked.
>
> 2. In QEMU, we decide that synchronous signals cannot be blocked and
> simply override the signal handler to default handler. So on the second
> signal, we execute the default handler and not our defined handler.
>
> Multiple ways to fix:
>
> 1. We have to manually unblock the signal in that hackery above.
> 2. In QEMU, don't block synchronous signals.
> 3. In QEMU, don't set the signal handler to the default handler in case
> a synchronous signal is blocked.


This all seems a little over-engineered. This is a single-threaded test
so what's wrong with a couple of flags:

bool should_get_sigill;

and the handler doing:

if (!should_get_sigill) {
   unexpected_sigills++;
}

Tests don't have to be pretty, just reliable.

>
>
> Trying to run the test on a real s390x linux indicates that 1. should be
> the right approach.
>
> address@hidden ~]$ ./vge
> Illegal instruction (core dumped)


--
Alex Bennée