Re: [qemu-s390x] [Qemu-devel] [PATCH v1 12/15] s390x/tcg: Implement XxC and checks for most FP instructions

[Richard Henderson]

On 2/12/19 3:03 AM, David Hildenbrand wrote:
> -uint64_t HELPER(cegb)(CPUS390XState *env, int64_t v2, uint32_t m3)
> +uint64_t HELPER(cegb)(CPUS390XState *env, int64_t v2, uint32_t m)
>  {
> -    int old_mode = s390_swap_bfp_rounding_mode(env, m3);
> +    int old_mode = s390_swap_bfp_rounding_mode(env, m & 0xf);
>  
>      float32 ret = int64_to_float32(v2, &env->fpu_status);
>      s390_restore_bfp_rounding_mode(env, old_mode);
> -    handle_exceptions(env, false, GETPC());
> +    handle_exceptions(env, (m >> 8) & 1, GETPC());
>      return ret;
>  }

It might be helpful to have inlines for these extractions.  E.g

static inline uint32_t round_from_m34(uint32_t m);
static inline bool xxc_from_m34(uint32_t m);

>  static DisasJumpType op_cfeb(DisasContext *s, DisasOps *o)
>  {
> -    TCGv_i32 m3 = tcg_const_i32(get_field(s->fields, m3));
> -    gen_helper_cfeb(o->out, cpu_env, o->in2, m3);
> -    tcg_temp_free_i32(m3);
> +    TCGv_i32 m;
> +
> +    if (!valid_bfp_rounding_mode(get_field(s->fields, m3))) {
> +        gen_program_exception(s, PGM_SPECIFICATION);
> +        return DISAS_NORETURN;
> +    }
> +    m = tcg_const_i32(get_fields2(s->fields, m3, m4));
> +    gen_helper_cfeb(o->out, cpu_env, o->in2, m);
> +    tcg_temp_free_i32(m);
>      gen_set_cc_nz_f32(s, o->in2);
>      return DISAS_NEXT;
>  }

The m4 field does not exist unless fp extension facility is enabled.  You
should ignore the field if not installed.

It would be good to split this out to a helper, since there are so many copies.
 E.g.

static bool extract_m3_m4(DisasContext *s, uint32_t *m34)
{
    int m3 = get_field(s->fields, m3);
    int m4 = get_field(s->fields, m4);

    if (!valid_bfp_rounding_mode(m3)) {
        gen_program_exception(s, PGM_SPECIFICATION);
        return false;
    }
    if (feature) {
        return deposit32(m3, 8, 1, m4);
    }
    return m3;
}

Hmm..  Except that I see we don't have enough stored in DisasContext to allow
you to look up the proper feature.  So perhaps just a FIXME for now?


r~