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

[Alex Bennée]

Thomas Huth <thuth@redhat.com> writes:

> 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.
>
> 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
> @@ -0,0 +1,50 @@
> +/*
> + * vf: vector facility tests
> + */
> +#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,
> diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
> index c830313e67..f8e71a9439 100644
> --- a/tests/tcg/s390x/Makefile.target
> +++ b/tests/tcg/s390x/Makefile.target
> @@ -18,6 +18,12 @@ TESTS+=signals-s390x
>  TESTS+=branch-relative-long
>  TESTS+=noexec
>  
> +Z13_TESTS=vf
> +vf: LDFLAGS+=-lm
> +$(Z13_TESTS): CFLAGS+=-march=z13 -O2
> +TESTS+=$(if $(shell $(CC) -march=z13 -S -o /dev/null -xc /dev/null \
> +                      >/dev/null 2>&1 && echo OK),$(Z13_TESTS))
> +

I didn't realise there where a bunch of compile time tests in the s390x
makefiles. The best practice (since Paolo's refactoring) is to emit a
config-cc.mak to set some variables once, e.g.:

  config-cc.mak: Makefile
          $(quiet-@)( \
              $(call cc-option,-march=armv8.1-a+sve,          
CROSS_CC_HAS_SVE); \
              $(call cc-option,-march=armv8.1-a+sve2,         
CROSS_CC_HAS_SVE2); \
              $(call cc-option,-march=armv8.3-a,              
CROSS_CC_HAS_ARMV8_3); \
              $(call cc-option,-mbranch-protection=standard,  
CROSS_CC_HAS_ARMV8_BTI); \
              $(call cc-option,-march=armv8.5-a+memtag,       
CROSS_CC_HAS_ARMV8_MTE)) 3> config-cc.mak
  -include config-cc.mak


>  Z14_TESTS=vfminmax
>  vfminmax: LDFLAGS+=-lm
>  $(Z14_TESTS): CFLAGS+=-march=z14 -O2


-- 
Alex Bennée