Re: [Qemu-devel] [PATCH 3/7] target/s390x: fix CSST decoding and runtime alignment check

[David Hildenbrand]

On 21.08.2018 04:51, Pavel Zbitskiy wrote:
> CSST is defined as:
> 
>     C(0xc802, CSST,    SSF,   CASS, la1, a2, 0, 0, csst, 0)
> 
> It means that the first parameter is handled by in1_la1().
> in1_la1() fills addr1 field, and not in1.
> 
> Furthermore, when extract32() is used for the alignment check, the
> third parameter should specify the number of trailing bits that must
> be 0. For FC these numbers are:
> 
>     FC=0 (word, 4 bytes):        2
>     FC=1 (double word, 8 bytes): 3
>     FC=2 (quad word, 16 bytes):  4
> 
> For SC these numbers correspond to the size:
> 
>     SC=0: 0
>     SC=1: 1
>     SC=2: 2
>     SC=3: 3
>     SC=4: 4
> 
> Signed-off-by: Pavel Zbitskiy <address@hidden>
> ---
>  target/s390x/mem_helper.c       |  2 +-
>  target/s390x/translate.c        |  4 +--
>  tests/tcg/s390x/Makefile.target |  1 +
>  tests/tcg/s390x/csst.c          | 43 +++++++++++++++++++++++++++++++++
>  4 files changed, 47 insertions(+), 3 deletions(-)
>  create mode 100644 tests/tcg/s390x/csst.c
> 
> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
> index e21a47fb4d..c94dbf3fcb 100644
> --- a/target/s390x/mem_helper.c
> +++ b/target/s390x/mem_helper.c
> @@ -1442,7 +1442,7 @@ static uint32_t do_csst(CPUS390XState *env, uint32_t 
> r3, uint64_t a1,
>      }
>  
>      /* Sanity check the alignments.  */
> -    if (extract32(a1, 0, 4 << fc) || extract32(a2, 0, 1 << sc)) {
> +    if (extract32(a1, 0, fc + 2) || extract32(a2, 0, sc)) {
>          goto spec_exception;
>      }
>  
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 111d575c41..929fc2db28 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -2059,9 +2059,9 @@ static DisasJumpType op_csst(DisasContext *s, DisasOps 
> *o)
>      TCGv_i32 t_r3 = tcg_const_i32(r3);
>  
>      if (tb_cflags(s->base.tb) & CF_PARALLEL) {
> -        gen_helper_csst_parallel(cc_op, cpu_env, t_r3, o->in1, o->in2);
> +        gen_helper_csst_parallel(cc_op, cpu_env, t_r3, o->addr1, o->in2);
>      } else {
> -        gen_helper_csst(cc_op, cpu_env, t_r3, o->in1, o->in2);
> +        gen_helper_csst(cc_op, cpu_env, t_r3, o->addr1, o->in2);
>      }
>      tcg_temp_free_i32(t_r3);
>  
> diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
> index 9f4076901f..f62f950d8e 100644
> --- a/tests/tcg/s390x/Makefile.target
> +++ b/tests/tcg/s390x/Makefile.target
> @@ -1,3 +1,4 @@
>  VPATH+=$(SRC_PATH)/tests/tcg/s390x
>  CFLAGS+=-march=zEC12 -m64
>  TESTS+=hello-s390x
> +TESTS+=csst
> diff --git a/tests/tcg/s390x/csst.c b/tests/tcg/s390x/csst.c
> new file mode 100644
> index 0000000000..1dae9071fb
> --- /dev/null
> +++ b/tests/tcg/s390x/csst.c
> @@ -0,0 +1,43 @@
> +#include <stdint.h>
> +#include <unistd.h>
> +
> +int main(void)
> +{
> +    uint64_t parmlist[] = {
> +        0xfedcba9876543210ull,
> +        0,
> +        0x7777777777777777ull,
> +        0,
> +    };
> +    uint64_t op1 = 0x0123456789abcdefull;
> +    uint64_t op2 = 0;
> +    uint64_t op3 = op1;
> +    uint64_t cc;
> +
> +    asm volatile(
> +        "    lghi %%r0,%[flags]\n"
> +        "    la %%r1,%[parmlist]\n"
> +        "    csst %[op1],%[op2],%[op3]\n"
> +        "    ipm %[cc]\n"
> +        : [op1] "+m" (op1),
> +          [op2] "+m" (op2),
> +          [op3] "+r" (op3),
> +          [cc] "=r" (cc)
> +        : [flags] "K" (0x0301),
> +          [parmlist] "m" (parmlist)
> +        : "r0", "r1", "cc", "memory");
> +    cc = (cc >> 28) & 3;
> +    if (cc) {
> +        write(1, "bad cc\n", 7);
> +        return 1;
> +    }
> +    if (op1 != parmlist[0]) {
> +        write(1, "bad op1\n", 8);
> +        return 1;
> +    }
> +    if (op2 != parmlist[2]) {
> +        write(1, "bad op2\n", 8);
> +        return 1;
> +    }
> +    return 0;
> +}
> 

Reviewed-by: David Hildenbrand <address@hidden>

-- 

Thanks,

David / dhildenb