Re: [qemu-s390x] [PATCH v3 8/9] s390x/tcg: fix FP register pair checks

[Thomas Huth]

On 2018-09-17 11:34, David Hildenbrand wrote:
> Valid register pairs are 0/2, 1/3, 4/6, 5/7, 8/10, 9/11, 12/14, 13/15.
> 
> R1/R2 always selects the lower number, so the current checks are not
> correct as e.g. 2/4 could be selected as a pair.
> 
> Reviewed-by: Richard Henderson <address@hidden>
> Signed-off-by: David Hildenbrand <address@hidden>
> ---
>  target/s390x/translate.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index f9a78c4304..5cc65b0840 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -1110,7 +1110,7 @@ typedef struct {
>  #define IF_HFP3     0x0004      /* r3 points at fp reg for HFP instructions 
> */
>  #define IF_BFP      0x0008      /* binary floating point instruction */
>  #define IF_DFP      0x0010      /* decimal floating point instruction */
> -#define IF_PRIV     0x0020      /* priviledged instruction */
> +#define IF_PRIV     0x0020      /* privileged instruction */

Ah, well, merge this with the previous patch, please.

>  struct DisasInsn {
>      unsigned opc:16;
> @@ -5985,6 +5985,12 @@ static bool is_afp_reg(int reg)
>      return reg % 2 || reg > 6;
>  }
>  
> +static bool is_fp_pair(int reg)
> +{
> +    /* 0,1,4,5,8,9,12,13: to exclude the others, check for single bit */
> +    return !(reg & 0x2);
> +}
> +
>  static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>  {
>      const DisasInsn *insn;
> @@ -6067,17 +6073,11 @@ static DisasJumpType translate_one(CPUS390XState 
> *env, DisasContext *s)
>                  excp = PGM_SPECIFICATION;
>              }
>          }
> -        if (spec & SPEC_r1_f128) {
> -            r = get_field(&f, r1);
> -            if (r > 13) {
> -                excp = PGM_SPECIFICATION;
> -            }
> +        if (spec & SPEC_r1_f128 && !is_fp_pair(get_field(&f, r1))) {
> +            excp = PGM_SPECIFICATION;
>          }
> -        if (spec & SPEC_r2_f128) {
> -            r = get_field(&f, r2);
> -            if (r > 13) {
> -                excp = PGM_SPECIFICATION;
> -            }
> +        if (spec & SPEC_r2_f128 && !is_fp_pair(get_field(&f, r2))) {
> +            excp = PGM_SPECIFICATION;
>          }
>          if (excp) {
>              gen_program_exception(s, excp);
> 

Without the "priviledged" hunk:

Reviewed-by: Thomas Huth <address@hidden>