Re: [PATCH] target/s390x: Fix emulation of the VISTR instruction

[David Hildenbrand <david@redhat.com>]

On 11.10.22 12:14, Thomas Huth wrote:
> The element size is encoded in the M3 field, not in the M4
> field. Let's also add a TCG test that shows the failing
> behavior without this fix.
> 

I'd suggest moving the test to a separate patch and adding a Fixes: tag 
to the fix.

Should be

Fixes: be6324c6b734 ("s390x/tcg: Implement VECTOR ISOLATE STRING")

> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1248
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/tcg/s390x/vf.c                | 50 +++++++++++++++++++++++++++++
>   target/s390x/tcg/translate_vx.c.inc |  2 +-
>   tests/tcg/s390x/Makefile.target     |  6 ++++
>   3 files changed, 57 insertions(+), 1 deletion(-)
>   create mode 100644 tests/tcg/s390x/vf.c
> 
> diff --git a/tests/tcg/s390x/vf.c b/tests/tcg/s390x/vf.c
> new file mode 100644
> index 0000000000..fdc424ce7c
> --- /dev/null
> +++ b/tests/tcg/s390x/vf.c

In general, we use "vx" when talking about vector extension. Maybe name 
this vx.c

...

> @@ -0,0 +1,50 @@
> +/*
> + * vf: vector facility tests

And adjust it here as well.

> + */
> +#include <stdint.h>
> +#include <stdio.h>
> +#include "vx.h"
> +
> +static inline void vistr(S390Vector *v1, S390Vector *v2,
> +                         const uint8_t m3, const uint8_t m5)
> +{
> +    asm volatile("vistr %[v1], %[v2], %[m3], %[m5]\n"
> +                : [v1] "=v" (v1->v)
> +                : [v2]  "v" (v2->v)
> +                , [m3]  "i" (m3)
> +                , [m5]  "i" (m5)
> +                : "cc");
> +}
> +
> +static int test_vistr(void)
> +{
> +    S390Vector vd = {};
> +    S390Vector vs16 = {
> +        .h[0] = 0x1234, .h[1] = 0x0056, .h[2] = 0x7800, .h[3] = 0x0000,
> +        .h[4] = 0x0078, .h[5] = 0x0000, .h[6] = 0x6543, .h[7] = 0x2100
> +    };
> +    S390Vector vs32 = {
> +        .w[0] = 0x12340000, .w[1] = 0x78654300,
> +        .w[2] = 0x0, .w[3] = 0x12,
> +    };
> +
> +    vistr(&vd, &vs16, 1, 0);
> +    if (vd.h[0] != 0x1234 || vd.h[1] != 0x0056 || vd.h[2] != 0x7800 ||
> +        vd.h[3] || vd.h[4] || vd.h[5] || vd.h[6] || vd.h[7]) {
> +        puts("ERROR: vitrh failed!");
> +        return 1;
> +    }
> +
> +    vistr(&vd, &vs32, 2, 0);
> +    if (vd.w[0] != 0x12340000 || vd.w[1] != 0x78654300 || vd.w[2] || vd.w[3]) {
> +        puts("ERROR: vitrf failed!");
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +    return test_vistr();
> +}
> diff --git a/target/s390x/tcg/translate_vx.c.inc b/target/s390x/tcg/translate_vx.c.inc
> index 3526ba3e3b..b69c1a111c 100644
> --- a/target/s390x/tcg/translate_vx.c.inc
> +++ b/target/s390x/tcg/translate_vx.c.inc
> @@ -2723,7 +2723,7 @@ static DisasJumpType op_vfene(DisasContext *s, DisasOps *o)
>   
>   static DisasJumpType op_vistr(DisasContext *s, DisasOps *o)
>   {
> -    const uint8_t es = get_field(s, m4);
> +    const uint8_t es = get_field(s, m3);
>       const uint8_t m5 = get_field(s, m5);
>       static gen_helper_gvec_2 * const g[3] = {
>           gen_helper_gvec_vistr8,

Apart from that, LGTM.

-- 
Thanks,

David / dhildenb