Re: [qemu-s390x] [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 21:19, Alex Bennée wrote:
>>
>> 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:
>
> I have to jump over the actual broken instruction and want to avoid
> patching it. Otherwise I'll loop forever ...

So from:

  Subject: [PATCH  v2 6/6] tests/tcg/aarch64: userspace system register test
  Date: Tue,  5 Feb 2019 19:02:24 +0000
  Message-Id: <address@hidden>

Where I had to do something similar:

  bool should_fail;
  int should_fail_count;
  int should_not_fail_count;
  uintptr_t failed_pc[10];

  void sigill_handler(int signo, siginfo_t *si, void *data)
  {
      ucontext_t *uc = (ucontext_t *)data;

      if (should_fail) {
          should_fail_count++;
      } else {
          uintptr_t pc = (uintptr_t) uc->uc_mcontext.pc;
          failed_pc[should_not_fail_count++] =  pc;
      }
      uc->uc_mcontext.pc += 4;
  }

But that does depend on arch making pc hackable:

  /* Hook in a SIGILL handler */
  memset(&sa, 0, sizeof(struct sigaction));
  sa.sa_flags = SA_SIGINFO;
  sa.sa_sigaction = &sigill_handler;
  sigemptyset(&sa.sa_mask);

And crucially have nice regular sized instructions. Is that not an option?

--
Alex Bennée