[PATCH 1/2] target/s390x: Use a 16-bit immediate in VREP

[PATCH 1/2] target/s390x: Use a 16-bit immediate in VREP

[Ilya Leoshkevich]

Unlike most other instructions that contain an immediate element index,
VREP's one is 16-bit, and not 4-bit. The code uses only 8 bits, so
using, e.g., 0x101 does not lead to a specification exception.

Fix by checking all 16 bits.

Cc: qemu-stable@nongnu.org
Fixes: 28d08731b1d8 ("s390x/tcg: Implement VECTOR REPLICATE")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/s390x/tcg/translate_vx.c.inc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/s390x/tcg/translate_vx.c.inc b/target/s390x/tcg/translate_vx.c.inc
index f8df121d3d3..a6d840d4065 100644
--- a/target/s390x/tcg/translate_vx.c.inc
+++ b/target/s390x/tcg/translate_vx.c.inc
@@ -57,7 +57,7 @@
 #define FPF_LONG        3
 #define FPF_EXT         4
 
-static inline bool valid_vec_element(uint8_t enr, MemOp es)
+static inline bool valid_vec_element(uint16_t enr, MemOp es)
 {
     return !(enr & ~(NUM_VEC_ELEMENTS(es) - 1));
 }
@@ -964,7 +964,7 @@ static DisasJumpType op_vpdi(DisasContext *s, DisasOps *o)
 
 static DisasJumpType op_vrep(DisasContext *s, DisasOps *o)
 {
-    const uint8_t enr = get_field(s, i2);
+    const uint16_t enr = get_field(s, i2);
     const uint8_t es = get_field(s, m4);
 
     if (es > ES_64 || !valid_vec_element(enr, es)) {
-- 
2.41.0

[PATCH 2/2] tests/tcg/s390x: Test VREP

[Ilya Leoshkevich]

Add a small test to prevent regressions.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/s390x/Makefile.target |  1 +
 tests/tcg/s390x/vrep.c          | 81 +++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+)
 create mode 100644 tests/tcg/s390x/vrep.c

diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 1fc98099070..9548b6d4f51 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -59,6 +59,7 @@ Z13_TESTS=vistr
 Z13_TESTS+=lcbb
 Z13_TESTS+=locfhr
 Z13_TESTS+=vcksm
+Z13_TESTS+=vrep
 $(Z13_TESTS): CFLAGS+=-march=z13 -O2
 TESTS+=$(Z13_TESTS)
 
diff --git a/tests/tcg/s390x/vrep.c b/tests/tcg/s390x/vrep.c
new file mode 100644
index 00000000000..d5a3bd8eb20
--- /dev/null
+++ b/tests/tcg/s390x/vrep.c
@@ -0,0 +1,81 @@
+/*
+ * Test the VREP instruction.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+#include <assert.h>
+#include <signal.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include "vx.h"
+
+static void handle_sigill(int sig, siginfo_t *info, void *ucontext)
+{
+    mcontext_t *mcontext = &((ucontext_t *)ucontext)->uc_mcontext;
+    char *insn = (char *)info->si_addr;
+
+    if (insn[0] != 0xe7 || insn[5] != 0x4d) {
+        _exit(EXIT_FAILURE);
+    }
+
+    mcontext->gregs[2] = SIGILL;
+}
+
+static inline __attribute__((__always_inline__)) unsigned long
+vrep(S390Vector *v1, const S390Vector *v3, const uint16_t i2, const uint8_t m4)
+{
+    register unsigned long sig asm("r2") = -1;
+
+    asm("vrep %[v1],%[v3],%[i2],%[m4]\n"
+        : [v1] "=v" (v1->v)
+        , [sig] "+r" (sig)
+        : [v3] "v" (v3->v)
+        , [i2] "i" (i2)
+        , [m4] "i" (m4));
+
+    return sig;
+}
+
+int main(int argc, char *argv[])
+{
+    S390Vector v3 = {.d[0] = 1, .d[1] = 2};
+    struct sigaction act;
+    S390Vector v1;
+    int err;
+
+    memset(&act, 0, sizeof(act));
+    act.sa_sigaction = handle_sigill;
+    act.sa_flags = SA_SIGINFO;
+    err = sigaction(SIGILL, &act, NULL);
+    assert(err == 0);
+
+    assert(vrep(&v1, &v3, 7, 0) == -1);
+    assert(v1.d[0] == 0x0101010101010101ULL);
+    assert(v1.d[1] == 0x0101010101010101ULL);
+
+    assert(vrep(&v1, &v3, 7, 1) == -1);
+    assert(v1.d[0] == 0x0002000200020002ULL);
+    assert(v1.d[1] == 0x0002000200020002ULL);
+
+    assert(vrep(&v1, &v3, 1, 2) == -1);
+    assert(v1.d[0] == 0x0000000100000001ULL);
+    assert(v1.d[1] == 0x0000000100000001ULL);
+
+    assert(vrep(&v1, &v3, 1, 3) == -1);
+    assert(v1.d[0] == 2);
+    assert(v1.d[1] == 2);
+
+    assert(vrep(&v1, &v3, 0x10, 0) == SIGILL);
+    assert(vrep(&v1, &v3, 0x101, 0) == SIGILL);
+    assert(vrep(&v1, &v3, 0x8, 1) == SIGILL);
+    assert(vrep(&v1, &v3, 0x108, 1) == SIGILL);
+    assert(vrep(&v1, &v3, 0x4, 2) == SIGILL);
+    assert(vrep(&v1, &v3, 0x104, 2) == SIGILL);
+    assert(vrep(&v1, &v3, 0x2, 3) == SIGILL);
+    assert(vrep(&v1, &v3, 0x102, 3) == SIGILL);
+
+    return EXIT_SUCCESS;
+}
-- 
2.41.0

Re: [PATCH 1/2] target/s390x: Use a 16-bit immediate in VREP

[David Hildenbrand]

On 07.08.23 18:34, Ilya Leoshkevich wrote:
> Unlike most other instructions that contain an immediate element index,
> VREP's one is 16-bit, and not 4-bit. The code uses only 8 bits, so
> using, e.g., 0x101 does not lead to a specification exception.
> 
> Fix by checking all 16 bits.
> 
> Cc: qemu-stable@nongnu.org

Just curious, why stable? Are there valid programs that set invalid 
element size and they are fixed by this?

LGTM

Reviewed-by: David Hildenbrand <david@redhat.com>

> Fixes: 28d08731b1d8 ("s390x/tcg: Implement VECTOR REPLICATE")
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/s390x/tcg/translate_vx.c.inc | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/tcg/translate_vx.c.inc b/target/s390x/tcg/translate_vx.c.inc
> index f8df121d3d3..a6d840d4065 100644
> --- a/target/s390x/tcg/translate_vx.c.inc
> +++ b/target/s390x/tcg/translate_vx.c.inc
> @@ -57,7 +57,7 @@
>   #define FPF_LONG        3
>   #define FPF_EXT         4
>   
> -static inline bool valid_vec_element(uint8_t enr, MemOp es)
> +static inline bool valid_vec_element(uint16_t enr, MemOp es)
>   {
>       return !(enr & ~(NUM_VEC_ELEMENTS(es) - 1));
>   }
> @@ -964,7 +964,7 @@ static DisasJumpType op_vpdi(DisasContext *s, DisasOps *o)
>   
>   static DisasJumpType op_vrep(DisasContext *s, DisasOps *o)
>   {
> -    const uint8_t enr = get_field(s, i2);
> +    const uint16_t enr = get_field(s, i2);
>       const uint8_t es = get_field(s, m4);
>   
>       if (es > ES_64 || !valid_vec_element(enr, es)) {

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] target/s390x: Use a 16-bit immediate in VREP

[Ilya Leoshkevich]

On Mon, 2023-08-07 at 19:00 +0200, David Hildenbrand wrote:
> On 07.08.23 18:34, Ilya Leoshkevich wrote:
> > Unlike most other instructions that contain an immediate element
> > index,
> > VREP's one is 16-bit, and not 4-bit. The code uses only 8 bits, so
> > using, e.g., 0x101 does not lead to a specification exception.
> > 
> > Fix by checking all 16 bits.
> > 
> > Cc: qemu-stable@nongnu.org
> 
> Just curious, why stable? Are there valid programs that set invalid 
> element size and they are fixed by this?

None that I know of, but I thought this was still nice to have, and at
the same time small enough to not cause any trouble.

> LGTM
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>

[...]
> 

Re: [PATCH 1/2] target/s390x: Use a 16-bit immediate in VREP

[David Hildenbrand]

On 07.08.23 19:02, Ilya Leoshkevich wrote:
> On Mon, 2023-08-07 at 19:00 +0200, David Hildenbrand wrote:
>> On 07.08.23 18:34, Ilya Leoshkevich wrote:
>>> Unlike most other instructions that contain an immediate element
>>> index,
>>> VREP's one is 16-bit, and not 4-bit. The code uses only 8 bits, so
>>> using, e.g., 0x101 does not lead to a specification exception.
>>>
>>> Fix by checking all 16 bits.
>>>
>>> Cc: qemu-stable@nongnu.org
>>
>> Just curious, why stable? Are there valid programs that set invalid
>> element size and they are fixed by this?
> 
> None that I know of, but I thought this was still nice to have, and at
> the same time small enough to not cause any trouble.

Yes, I was just curious. From my recollection, we didn't backport all 
specification exception checks.

-- 
Cheers,

David / dhildenb

Re: [PATCH 1/2] target/s390x: Use a 16-bit immediate in VREP

[Michael Tokarev]

> On 07.08.23 19:02, Ilya Leoshkevich wrote:
..>> None that I know of, but I thought this was still nice to have, and at
>> the same time small enough to not cause any trouble.

Ilya, do you have an idea why your messages don't reach qemu-stable@?

I see e.g. David's replies to yuor messages in qemu-stable@ - this way
I know you sent something, and copy that message from qemu-devel@.

For example, this thread is shown in the archive with your messages
unavailable:

https://mail.gnu.org/archive/html/qemu-stable/2023-08/threads.html#00118

Re: [PATCH 1/2] target/s390x: Use a 16-bit immediate in VREP

[Ilya Leoshkevich]

On Mon, 2023-08-07 at 20:27 +0300, Michael Tokarev wrote:
> > On 07.08.23 19:02, Ilya Leoshkevich wrote:
> ..>> None that I know of, but I thought this was still nice to have,
> and at
> > > the same time small enough to not cause any trouble.
> 
> Ilya, do you have an idea why your messages don't reach qemu-stable@?
> 
> I see e.g. David's replies to yuor messages in qemu-stable@ - this
> way
> I know you sent something, and copy that message from qemu-devel@.
> 
> For example, this thread is shown in the archive with your messages
> unavailable:
> 
> https://mail.gnu.org/archive/html/qemu-stable/2023-08/threads.html#00118

No, unfortunately I don't know. Everything looks normal on my end,
qemu-stable@ is properly Cc:ed and I don't see any typos.
I've sent a question about this to the mailing list owner.