Re: [Qemu-devel] [PULL 41/46] target/arm/vfp_helper: Extract vfp_set_fpscr_to_host()

[Laurent Desnogues]

Hello,

On Tue, Jul 2, 2019 at 4:18 AM Peter Maydell <address@hidden> wrote:
>
> From: Philippe Mathieu-Daudé <address@hidden>
>
> The vfp_set_fpscr() helper contains code specific to the host
> floating point implementation (here the SoftFloat library).
> Extract this code to vfp_set_fpscr_to_host().
>
> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
> Message-id: address@hidden
> Reviewed-by: Peter Maydell <address@hidden>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  target/arm/vfp_helper.c | 127 +++++++++++++++++++++-------------------
>  1 file changed, 66 insertions(+), 61 deletions(-)
>
> diff --git a/target/arm/vfp_helper.c b/target/arm/vfp_helper.c
> index d54e3253240..b19a395b67d 100644
> --- a/target/arm/vfp_helper.c
> +++ b/target/arm/vfp_helper.c
> @@ -81,71 +81,11 @@ static inline int vfp_exceptbits_to_host(int target_bits)
>      return host_bits;
>  }
>
> -uint32_t HELPER(vfp_get_fpscr)(CPUARMState *env)
> -{
> -    uint32_t i, fpscr;
> -
> -    fpscr = env->vfp.xregs[ARM_VFP_FPSCR]
> -            | (env->vfp.vec_len << 16)
> -            | (env->vfp.vec_stride << 20);
> -
> -    i = get_float_exception_flags(&env->vfp.fp_status);
> -    i |= get_float_exception_flags(&env->vfp.standard_fp_status);
> -    /* FZ16 does not generate an input denormal exception.  */
> -    i |= (get_float_exception_flags(&env->vfp.fp_status_f16)
> -          & ~float_flag_input_denormal);
> -    fpscr |= vfp_exceptbits_from_host(i);
> -
> -    i = env->vfp.qc[0] | env->vfp.qc[1] | env->vfp.qc[2] | env->vfp.qc[3];
> -    fpscr |= i ? FPCR_QC : 0;
> -
> -    return fpscr;
> -}
> -
> -uint32_t vfp_get_fpscr(CPUARMState *env)
> -{
> -    return HELPER(vfp_get_fpscr)(env);
> -}
> -
> -void HELPER(vfp_set_fpscr)(CPUARMState *env, uint32_t val)
> +static void vfp_set_fpscr_to_host(CPUARMState *env, uint32_t val)
>  {
>      int i;
>      uint32_t changed = env->vfp.xregs[ARM_VFP_FPSCR];
>
> -    /* When ARMv8.2-FP16 is not supported, FZ16 is RES0.  */
> -    if (!cpu_isar_feature(aa64_fp16, env_archcpu(env))) {
> -        val &= ~FPCR_FZ16;
> -    }
> -
> -    if (arm_feature(env, ARM_FEATURE_M)) {
> -        /*
> -         * M profile FPSCR is RES0 for the QC, STRIDE, FZ16, LEN bits
> -         * and also for the trapped-exception-handling bits IxE.
> -         */
> -        val &= 0xf7c0009f;
> -    }
> -
> -    /*
> -     * We don't implement trapped exception handling, so the
> -     * trap enable bits, IDE|IXE|UFE|OFE|DZE|IOE are all RAZ/WI (not RES0!)
> -     *
> -     * If we exclude the exception flags, IOC|DZC|OFC|UFC|IXC|IDC
> -     * (which are stored in fp_status), and the other RES0 bits
> -     * in between, then we clear all of the low 16 bits.
> -     */
> -    env->vfp.xregs[ARM_VFP_FPSCR] = val & 0xf7c80000;
> -    env->vfp.vec_len = (val >> 16) & 7;
> -    env->vfp.vec_stride = (val >> 20) & 3;
> -
> -    /*
> -     * The bit we set within fpscr_q is arbitrary; the register as a
> -     * whole being zero/non-zero is what counts.
> -     */
> -    env->vfp.qc[0] = val & FPCR_QC;
> -    env->vfp.qc[1] = 0;
> -    env->vfp.qc[2] = 0;
> -    env->vfp.qc[3] = 0;
> -
>      changed ^= val;
>      if (changed & (3 << 22)) {
>          i = (val >> 22) & 3;
> @@ -193,6 +133,71 @@ void HELPER(vfp_set_fpscr)(CPUARMState *env, uint32_t 
> val)
>      set_float_exception_flags(0, &env->vfp.standard_fp_status);
>  }
>
> +uint32_t HELPER(vfp_get_fpscr)(CPUARMState *env)
> +{
> +    uint32_t i, fpscr;
> +
> +    fpscr = env->vfp.xregs[ARM_VFP_FPSCR]
> +            | (env->vfp.vec_len << 16)
> +            | (env->vfp.vec_stride << 20);
> +
> +    i = get_float_exception_flags(&env->vfp.fp_status);
> +    i |= get_float_exception_flags(&env->vfp.standard_fp_status);
> +    /* FZ16 does not generate an input denormal exception.  */
> +    i |= (get_float_exception_flags(&env->vfp.fp_status_f16)
> +          & ~float_flag_input_denormal);
> +    fpscr |= vfp_exceptbits_from_host(i);
> +
> +    i = env->vfp.qc[0] | env->vfp.qc[1] | env->vfp.qc[2] | env->vfp.qc[3];
> +    fpscr |= i ? FPCR_QC : 0;
> +
> +    return fpscr;
> +}
> +
> +uint32_t vfp_get_fpscr(CPUARMState *env)
> +{
> +    return HELPER(vfp_get_fpscr)(env);
> +}
> +
> +void HELPER(vfp_set_fpscr)(CPUARMState *env, uint32_t val)
> +{
> +    /* When ARMv8.2-FP16 is not supported, FZ16 is RES0.  */
> +    if (!cpu_isar_feature(aa64_fp16, env_archcpu(env))) {
> +        val &= ~FPCR_FZ16;
> +    }
> +
> +    if (arm_feature(env, ARM_FEATURE_M)) {
> +        /*
> +         * M profile FPSCR is RES0 for the QC, STRIDE, FZ16, LEN bits
> +         * and also for the trapped-exception-handling bits IxE.
> +         */
> +        val &= 0xf7c0009f;
> +    }
> +
> +    /*
> +     * We don't implement trapped exception handling, so the
> +     * trap enable bits, IDE|IXE|UFE|OFE|DZE|IOE are all RAZ/WI (not RES0!)
> +     *
> +     * If we exclude the exception flags, IOC|DZC|OFC|UFC|IXC|IDC
> +     * (which are stored in fp_status), and the other RES0 bits
> +     * in between, then we clear all of the low 16 bits.
> +     */
> +    env->vfp.xregs[ARM_VFP_FPSCR] = val & 0xf7c80000;
> +    env->vfp.vec_len = (val >> 16) & 7;
> +    env->vfp.vec_stride = (val >> 20) & 3;
> +
> +    /*
> +     * The bit we set within fpscr_q is arbitrary; the register as a
> +     * whole being zero/non-zero is what counts.
> +     */
> +    env->vfp.qc[0] = val & FPCR_QC;
> +    env->vfp.qc[1] = 0;
> +    env->vfp.qc[2] = 0;
> +    env->vfp.qc[3] = 0;
> +
> +    vfp_set_fpscr_to_host(env, val);
> +}
> +
>  void vfp_set_fpscr(CPUARMState *env, uint32_t val)
>  {
>      HELPER(vfp_set_fpscr)(env, val);

This patch breaks flag settings because at the point where
vfp_set_fpscr_to_host is called, the value in
env->vfp.xregs[ARM_VFP_FPSCR] has already been changed.

A possible fix to that issue to is to save FPCR value when entering
the helper and passing it to vfp_set_fpscr_to_host.

Thanks,

Laurent

> --
> 2.20.1
>
>