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

[David Hildenbrand <david@redhat.com>]

On 05.08.2018 20:28, 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: 2

-> word, 4 bytes -> 2 bit

>     FC=1: 3

-> double word, 8 bytes -> 3 bit

>     FC=2: 4

-> quad word, 16 bytes -> 4 bit

> 
> For SC these numbers are:
> 
>     SC=0: 0
>     SC=1: 1
>     SC=2: 2
>     SC=3: 3
>     SC=4: 4

Right, corresponds to the size.

> 
> Signed-off-by: Pavel Zbitskiy <pavel.zbitskiy@gmail.com>
> ---
>  target/s390x/mem_helper.c | 2 +-
>  target/s390x/translate.c  | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> 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 efdc88e227..f318fb6e4e 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -2050,9 +2050,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);

Indeed, only addr1 is filled.

(did this ever work?)

>      }
>      tcg_temp_free_i32(t_r3);
>  
> 

Are you running some test case or how did you find this? (PoP review?)


Haven't tested it yet, but looks sane to me

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

-- 

Thanks,

David / dhildenb