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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 7/5/19 9:24 AM, Laurent Desnogues wrote:
> Hello,
> 
> On Tue, Jul 2, 2019 at 4:18 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>> 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é <philmd@redhat.com>
>> Message-id: 20190701132516.26392-16-philmd@redhat.com
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  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.

Oops.

> 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.

OK, thanks,

Phil.