[Qemu-devel] [PATCH RFC 0/2] tests/tcg: Vector instruction tests for target/s390x

[Qemu-devel] [PATCH RFC 0/2] tests/tcg: Vector instruction tests for target/s390x

[David Hildenbrand]

As I currently work on vector instruction support for s390x/tcg, for now
I wrote my tests for kvm-unit-tests, but tests/tcg seems to be a better fit.
The only tricky part is testing interrupt handling, but that also seems to
be possible using some signal hackery.

This is only one test to discuss if the approach make sense. These patches
only work when applied on top of:
    https://github.com/davidhildenbrand/qemu/tree/vx

David Hildenbrand (2):
  tests/tcg: Allow targets to set the optimization level
  tests/tcg: target/s390: Add test for VECTOR LOAD GR FROM VR ELEMENT

 tests/tcg/Makefile                  |  4 +--
 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 +++++++++++++++++++++++++++
 5 files changed, 108 insertions(+), 3 deletions(-)
 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

-- 
2.17.2

[Qemu-devel] [PATCH RFC 1/2] tests/tcg: Allow targets to set the optimization level

[David Hildenbrand]

We sometimes want basic optimizations, e.g. for constant propagation
into inlined functions.

Especially for inline asm with immediates;

static inline void some_instr(uint8_t imm)
{
    asm volatile("...", :: "i" (imm));
}

static void test()
{
    some_instr(1);
}

Which is impossible with -O0. So make -O0 the default but let
targets override it.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 tests/tcg/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/tcg/Makefile b/tests/tcg/Makefile
index bf06415390..bd0bace0d5 100644
--- a/tests/tcg/Makefile
+++ b/tests/tcg/Makefile
@@ -47,7 +47,7 @@ skip-test = @printf "  SKIPPED %s on $(TARGET_NAME) because %s\n" $1 $2
 TESTS=
 
 # Start with a blank slate, the build targets get to add stuff first
-CFLAGS=
+CFLAGS=-O0
 QEMU_CFLAGS=
 LDFLAGS=
 
@@ -70,7 +70,7 @@ ifneq ($(TARGET_BASE_ARCH),$(TARGET_NAME))
 endif
 
 # Add the common build options
-CFLAGS+=-Wall -O0 -g -fno-strict-aliasing
+CFLAGS+=-Wall -g -fno-strict-aliasing
 ifeq ($(BUILD_STATIC),y)
 LDFLAGS+=-static
 endif
-- 
2.17.2

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

[David Hildenbrand]

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 <david@redhat.com>
---
 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)
diff --git a/tests/tcg/s390x/vlgv.c b/tests/tcg/s390x/vlgv.c
new file mode 100644
index 0000000000..3c37ee2035
--- /dev/null
+++ b/tests/tcg/s390x/vlgv.c
@@ -0,0 +1,37 @@
+#include <stdint.h>
+#include <unistd.h>
+#include "signal_helper.inc.c"
+
+static inline void vlgv(uint64_t *r1, S390Vector *v3, const void *a2,
+                        uint8_t m4)
+{
+    asm volatile("vlgv %[r1], %[v3], 0(%[a2]), %[m4]\n"
+                 : [r1] "+d" (*r1),
+                   [v3] "+v" (v3->v)
+                 : [a2] "d" (a2),
+                   [m4] "i" (m4));
+}
+
+int main(void)
+{
+    S390Vector v3 = {
+        .q[0] = 0x0011223344556677ull,
+        .q[1] = 0x8899aabbccddeeffull,
+    };
+    uint64_t r1 = 0;
+
+    /* Directly set all ignored bits to  */
+    vlgv(&r1, &v3, (void *)(7 | ~0xf), ES_8);
+    check("8 bit", r1 == 0x77);
+    vlgv(&r1, &v3, (void *)(4 | ~0x7), ES_16);
+    check("16 bit", r1 == 0x8899);
+    vlgv(&r1, &v3, (void *)(3 | ~0x3), ES_32);
+    check("32 bit", r1 == 0xccddeeff);
+    vlgv(&r1, &v3, (void *)(1 | ~0x1), ES_64);
+    check("64 bit", r1 == 0x8899aabbccddeeffull);
+    check("v3 not modified", v3.q[0] == 0x0011223344556677ull &&
+                             v3.q[1] == 0x8899aabbccddeeffull);
+
+    CHECK_SIGILL(vlgv(&r1, &v3, NULL, ES_128));
+    return 0;
+}
-- 
2.17.2

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

[David Hildenbrand]

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.

Just double-checked, at least exrl-trt will need a fixup to work with -O2.

> 
> Add some infrastructure for checking if SIGILL will be properly
> triggered.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  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)
> diff --git a/tests/tcg/s390x/vlgv.c b/tests/tcg/s390x/vlgv.c
> new file mode 100644
> index 0000000000..3c37ee2035
> --- /dev/null
> +++ b/tests/tcg/s390x/vlgv.c
> @@ -0,0 +1,37 @@
> +#include <stdint.h>
> +#include <unistd.h>
> +#include "signal_helper.inc.c"
> +
> +static inline void vlgv(uint64_t *r1, S390Vector *v3, const void *a2,
> +                        uint8_t m4)
> +{
> +    asm volatile("vlgv %[r1], %[v3], 0(%[a2]), %[m4]\n"
> +                 : [r1] "+d" (*r1),
> +                   [v3] "+v" (v3->v)
> +                 : [a2] "d" (a2),
> +                   [m4] "i" (m4));
> +}
> +
> +int main(void)
> +{
> +    S390Vector v3 = {
> +        .q[0] = 0x0011223344556677ull,
> +        .q[1] = 0x8899aabbccddeeffull,
> +    };
> +    uint64_t r1 = 0;
> +
> +    /* Directly set all ignored bits to  */
> +    vlgv(&r1, &v3, (void *)(7 | ~0xf), ES_8);
> +    check("8 bit", r1 == 0x77);
> +    vlgv(&r1, &v3, (void *)(4 | ~0x7), ES_16);
> +    check("16 bit", r1 == 0x8899);
> +    vlgv(&r1, &v3, (void *)(3 | ~0x3), ES_32);
> +    check("32 bit", r1 == 0xccddeeff);
> +    vlgv(&r1, &v3, (void *)(1 | ~0x1), ES_64);
> +    check("64 bit", r1 == 0x8899aabbccddeeffull);
> +    check("v3 not modified", v3.q[0] == 0x0011223344556677ull &&
> +                             v3.q[1] == 0x8899aabbccddeeffull);
> +
> +    CHECK_SIGILL(vlgv(&r1, &v3, NULL, ES_128));
> +    return 0;
> +}
> 


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC 1/2] tests/tcg: Allow targets to set the optimization level

[Alex Bennée]

David Hildenbrand <david@redhat.com> writes:

> We sometimes want basic optimizations, e.g. for constant propagation
> into inlined functions.
>
> Especially for inline asm with immediates;

I agree you need this, but...

<snip>
>  # Start with a blank slate, the build targets get to add stuff first
> -CFLAGS=
> +CFLAGS=-O0
>  QEMU_CFLAGS=
>  LDFLAGS=
>
> @@ -70,7 +70,7 @@ ifneq ($(TARGET_BASE_ARCH),$(TARGET_NAME))
>  endif
>
>  # Add the common build options
> -CFLAGS+=-Wall -O0 -g -fno-strict-aliasing
> +CFLAGS+=-Wall -g -fno-strict-aliasing


You don't need to do this - we already have EXTRA_CFLAGS which will
always be at the end of the compile invocation:

  # enable vectors and optimisation to vectorise for this test
  vglv: CFLAGS+=-march=z13 -m64
  vglv: EXTRA_CFLAGS+=-O2


>  ifeq ($(BUILD_STATIC),y)
>  LDFLAGS+=-static
>  endif


--
Alex Bennée

Re: [Qemu-devel] [PATCH RFC 1/2] tests/tcg: Allow targets to set the optimization level

[David Hildenbrand]

On 27.02.19 12:46, Alex Bennée wrote:
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> We sometimes want basic optimizations, e.g. for constant propagation
>> into inlined functions.
>>
>> Especially for inline asm with immediates;
> 
> I agree you need this, but...
> 
> <snip>
>>  # Start with a blank slate, the build targets get to add stuff first
>> -CFLAGS=
>> +CFLAGS=-O0
>>  QEMU_CFLAGS=
>>  LDFLAGS=
>>
>> @@ -70,7 +70,7 @@ ifneq ($(TARGET_BASE_ARCH),$(TARGET_NAME))
>>  endif
>>
>>  # Add the common build options
>> -CFLAGS+=-Wall -O0 -g -fno-strict-aliasing
>> +CFLAGS+=-Wall -g -fno-strict-aliasing
> 
> 
> You don't need to do this - we already have EXTRA_CFLAGS which will
> always be at the end of the compile invocation:
> 
>   # enable vectors and optimisation to vectorise for this test
>   vglv: CFLAGS+=-march=z13 -m64
>   vglv: EXTRA_CFLAGS+=-O2
> 

Cool, missed that - thanks!


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC 1/2] tests/tcg: Allow targets to set the optimization level

[David Hildenbrand]

On 27.02.19 12:46, Alex Bennée wrote:
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>> We sometimes want basic optimizations, e.g. for constant propagation
>> into inlined functions.
>>
>> Especially for inline asm with immediates;
> 
> I agree you need this, but...
> 
> <snip>
>>  # Start with a blank slate, the build targets get to add stuff first
>> -CFLAGS=
>> +CFLAGS=-O0
>>  QEMU_CFLAGS=
>>  LDFLAGS=
>>
>> @@ -70,7 +70,7 @@ ifneq ($(TARGET_BASE_ARCH),$(TARGET_NAME))
>>  endif
>>
>>  # Add the common build options
>> -CFLAGS+=-Wall -O0 -g -fno-strict-aliasing
>> +CFLAGS+=-Wall -g -fno-strict-aliasing
> 
> 
> You don't need to do this - we already have EXTRA_CFLAGS which will
> always be at the end of the compile invocation:
> 
>   # enable vectors and optimisation to vectorise for this test
>   vglv: CFLAGS+=-march=z13 -m64
>   vglv: EXTRA_CFLAGS+=-O2

For whatever reason I can't get this to work. EXTRA_CFLAGS+=-O2 is
always silently dropped:

make SHELL='sh -x' run-tcg-tests-s390x-linux-user

+ /home/dhildenb/git/qemu/tests/docker/docker.py cc --user 100813 --cc
s390x-linux-gnu-gcc -i qemu:debian-s390x-cross -s
/home/dhildenb/git/qemu -- -march=zEC12 -m64 -Wall -O0 -g
-fno-strict-aliasing -march=z13
/home/dhildenb/git/qemu/tests/tcg/s390x/vlgv.c -o vlgv -static
/home/dhildenb/git/qemu/tests/tcg/s390x/vlgv.c: In function 'vlgv':
/home/dhildenb/git/qemu/tests/tcg/s390x/vlgv.c:8:5: warning: asm operand
3 probably doesn't match constraints
     asm volatile("vlgv %[r1], %[v3], 0(%[a2]), %[m4]\n"
     ^~~
/home/dhildenb/git/qemu/tests/tcg/s390x/vlgv.c:8:5: error: impossible
constraint in 'asm'



-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH RFC 1/2] tests/tcg: Allow targets to set the optimization level

[David Hildenbrand]

On 27.02.19 13:42, David Hildenbrand wrote:
> On 27.02.19 12:46, Alex Bennée wrote:
>>
>> David Hildenbrand <david@redhat.com> writes:
>>
>>> We sometimes want basic optimizations, e.g. for constant propagation
>>> into inlined functions.
>>>
>>> Especially for inline asm with immediates;
>>
>> I agree you need this, but...
>>
>> <snip>
>>>  # Start with a blank slate, the build targets get to add stuff first
>>> -CFLAGS=
>>> +CFLAGS=-O0
>>>  QEMU_CFLAGS=
>>>  LDFLAGS=
>>>
>>> @@ -70,7 +70,7 @@ ifneq ($(TARGET_BASE_ARCH),$(TARGET_NAME))
>>>  endif
>>>
>>>  # Add the common build options
>>> -CFLAGS+=-Wall -O0 -g -fno-strict-aliasing
>>> +CFLAGS+=-Wall -g -fno-strict-aliasing
>>
>>
>> You don't need to do this - we already have EXTRA_CFLAGS which will
>> always be at the end of the compile invocation:
>>
>>   # enable vectors and optimisation to vectorise for this test
>>   vglv: CFLAGS+=-march=z13 -m64
>>   vglv: EXTRA_CFLAGS+=-O2
> 
> For whatever reason I can't get this to work. EXTRA_CFLAGS+=-O2 is
> always silently dropped:
> 
> make SHELL='sh -x' run-tcg-tests-s390x-linux-user
> 
> + /home/dhildenb/git/qemu/tests/docker/docker.py cc --user 100813 --cc
> s390x-linux-gnu-gcc -i qemu:debian-s390x-cross -s
> /home/dhildenb/git/qemu -- -march=zEC12 -m64 -Wall -O0 -g
> -fno-strict-aliasing -march=z13
> /home/dhildenb/git/qemu/tests/tcg/s390x/vlgv.c -o vlgv -static
> /home/dhildenb/git/qemu/tests/tcg/s390x/vlgv.c: In function 'vlgv':
> /home/dhildenb/git/qemu/tests/tcg/s390x/vlgv.c:8:5: warning: asm operand
> 3 probably doesn't match constraints
>      asm volatile("vlgv %[r1], %[v3], 0(%[a2]), %[m4]\n"
>      ^~~
> /home/dhildenb/git/qemu/tests/tcg/s390x/vlgv.c:8:5: error: impossible
> constraint in 'asm'
> 

vlgv: CFLAGS+=-march=z13 -O2

However works. I am very confused now. I guess this is about evaluation
order or something like that.

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

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 <david@redhat.com>
> ---
>  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.


Trying to run the test on a real s390x linux indicates that 1. should be
the right approach.

[linux1@redhat-kvm ~]$ ./vge
Illegal instruction (core dumped)

-- 

Thanks,

David / dhildenb

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 <david@redhat.com> 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 <david@redhat.com>
>> ---
>>  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.
>
> [linux1@redhat-kvm ~]$ ./vge
> Illegal instruction (core dumped)


--
Alex Bennée

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

[David Hildenbrand]

On 27.02.19 21:19, Alex Bennée wrote:
> 
> David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
>>> ---
>>>  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 ...


-- 

Thanks,

David / dhildenb

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 <david@redhat.com> writes:

> On 27.02.19 21:19, Alex Bennée wrote:
>>
>> David Hildenbrand <david@redhat.com> 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 <david@redhat.com>
>>>> ---
>>>>  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: <20190205190224.2198-7-alex.bennee@linaro.org>

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

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

[Richard Henderson]

On 2/27/19 11:37 AM, David Hildenbrand wrote:
> #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.

And this is why we use sigaction not signal.

You can set action.sa_flags |= SA_RESETHAND to avoid needing the explicit reset
(or leave it clear to avoid the reset action that you are seeing).

You can set action.sa_flags |= SA_NODEFER to avoid having the signal added to
the signal mask of the thread, to avoid neeing to use sigsetjmp/siglongjmp.  Or
you can just use sigsetjmp/siglongjmp.  ;-)


r~

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

[Richard Henderson]

On 2/27/19 1:40 PM, Alex Bennée wrote:
> And crucially have nice regular sized instructions. Is that not an option?

s390 insns are {2,4,6} bytes.  I don't think that there's an easy way to pick
out the hw status codes that would give the ilen of the faulting insn.


r~

Re: [Qemu-devel] [PATCH RFC 1/2] tests/tcg: Allow targets to set the optimization level

[Richard Henderson]

On 2/27/19 3:14 AM, David Hildenbrand wrote:
> Especially for inline asm with immediates;
> 
> static inline void some_instr(uint8_t imm)
> {
>     asm volatile("...", :: "i" (imm));
> }
> 
> static void test()
> {
>     some_instr(1);
> }

FWIW, __attribute__((always_inline)) should work even with -O0.


r~

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

[David Hildenbrand]

On 28.02.19 01:24, Richard Henderson wrote:
> On 2/27/19 1:40 PM, Alex Bennée wrote:
>> And crucially have nice regular sized instructions. Is that not an option?
> 
> s390 insns are {2,4,6} bytes.  I don't think that there's an easy way to pick
> out the hw status codes that would give the ilen of the faulting insn.
> 

We would have to read the faulting instruction to determine based on the
two leftmost bits the ilen. But I don't consider that approach much
cleaner compared to signal + setjmp (signal + sigsetjmp after I fixed it
;) )

Thanks for the pointer though, Alex

> 
> r~
> 


-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 28.02.19 01:17, Richard Henderson wrote:
> On 2/27/19 11:37 AM, David Hildenbrand wrote:
>> #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.
> 
> And this is why we use sigaction not signal.
> 
> You can set action.sa_flags |= SA_RESETHAND to avoid needing the explicit reset
> (or leave it clear to avoid the reset action that you are seeing).
> 
> You can set action.sa_flags |= SA_NODEFER to avoid having the signal added to
> the signal mask of the thread, to avoid neeing to use sigsetjmp/siglongjmp.  Or
> you can just use sigsetjmp/siglongjmp.  ;-)

You might tell at this point that it is my first time hacking on signals
and I already learned a lot, thanks ;)

I guess signal + sigsetjmp/siglongjmp is the one with the smalles LOC?
Will give it a try.

BTW: I got the idea of using setjml/longjmp from
tests/tcg/multiarch/linux-test.c - we're also not caring about blocked
signals there, but I guess it doesn't matter as we trigger SISEGV only once.

-- 

Thanks,

David / dhildenb

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

[Richard Henderson]

On 2/27/19 11:14 PM, David Hildenbrand wrote:
> I guess signal + sigsetjmp/siglongjmp is the one with the smalles LOC?
> Will give it a try.

I suppose it depends on how you write it.

    struct sigaction sa = {
        .sa_mask = SA_RESETHAND | SA_NODEFER,
        .sa_handler = sig_sigill,
    };
    check("SIGILL not registered", sigaction(SIGILL, &sa, NULL) == 0);

is 5 lines to the 6 you currently use for registering and unregistering the
signal handler.  ;-)


r~