Re: [Qemu-devel] [PATCH v2 9/9] target/arm: Convert v8.2-fp16 from feature bit to pfr0 test

[Philippe Mathieu-Daudé]

On 9/30/18 9:29 PM, Richard Henderson wrote:
> On 9/28/18 11:34 AM, Philippe Mathieu-Daudé wrote:
>> On 27/09/2018 23:13, Richard Henderson wrote:
>>> Signed-off-by: Richard Henderson <address@hidden>
>>> ---
>>>  target/arm/cpu.h           | 17 +++++++++++++++-
>>>  target/arm/translate-a64.h |  1 +
>>>  target/arm/translate.h     |  1 +
>>>  linux-user/elfload.c       |  6 +-----
>>>  target/arm/cpu64.c         | 13 ++++++-------
>>>  target/arm/helper.c        |  2 +-
>>>  target/arm/translate-a64.c | 40 +++++++++++++++++++-------------------
>>>  target/arm/translate.c     |  6 +++---
>>>  8 files changed, 49 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>> index 152a558a94..bca4ee4281 100644
>>> --- a/target/arm/cpu.h
>>> +++ b/target/arm/cpu.h
>>> @@ -1566,7 +1566,6 @@ enum arm_features {
>>>      ARM_FEATURE_PMU, /* has PMU support */
>>>      ARM_FEATURE_VBAR, /* has cp15 VBAR */
>>>      ARM_FEATURE_M_SECURITY, /* M profile Security Extension */
>>> -    ARM_FEATURE_V8_FP16, /* implements v8.2 half-precision float */
>>>      ARM_FEATURE_M_MAIN, /* M profile Main Extension */
>>>  };
>>>  
>>> @@ -3176,6 +3175,16 @@ static inline bool aa32_feature_dp(ARMCPU *cpu)
>>>      return FIELD_EX32(cpu->id_isar6, ID_ISAR6, DP) != 0;
>>>  }
>>>  
>>> +static inline bool aa32_feature_fp16_arith(ARMCPU *cpu)
>>> +{
>>> +    /*
>>> +     * This is a placeholder for use by VCMA until the rest of
>>> +     * the ARMv8.2-FP16 extension is implemented for aa32 mode.
>>> +     * At which point we can properly set and check MVFR1.FPHP.
>>> +     */
>>> +    return FIELD_EX64(cpu->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
>>> +}
>>> +
>>>  /*
>>>   * 64-bit feature tests via id registers.
>>>   */
>>> @@ -3244,6 +3253,12 @@ static inline bool aa64_feature_fcma(ARMCPU *cpu)
>>>      return FIELD_EX64(cpu->id_aa64isar1, ID_AA64ISAR1, FCMA) != 0;
>>>  }
>>>  
>>> +static inline bool aa64_feature_fp16(ARMCPU *cpu)
>>> +{
>>> +    /* We always set the AdvSIMD and FP fields identically wrt FP16.  */
>>> +    return FIELD_EX64(cpu->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
>>> +}
>>> +
>>>  static inline bool aa64_feature_sve(ARMCPU *cpu)
>>>  {
>>>      return FIELD_EX64(cpu->id_aa64pfr0, ID_AA64PFR0, SVE) != 0;
>>> diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h
>>> index 636f3fded3..e122cef242 100644
>>> --- a/target/arm/translate-a64.h
>>> +++ b/target/arm/translate-a64.h
>>> @@ -140,6 +140,7 @@ FORWARD_FEATURE(sm3)
>>>  FORWARD_FEATURE(sm4)
>>>  FORWARD_FEATURE(dp)
>>>  FORWARD_FEATURE(fcma)
>>> +FORWARD_FEATURE(fp16)
>>>  FORWARD_FEATURE(sve)
>>>  
>>>  #undef FORWARD_FEATURE
>>> diff --git a/target/arm/translate.h b/target/arm/translate.h
>>> index d8eafbe88d..e022d6d4e6 100644
>>> --- a/target/arm/translate.h
>>> +++ b/target/arm/translate.h
>>> @@ -205,6 +205,7 @@ FORWARD_FEATURE(crc32)
>>>  FORWARD_FEATURE(rdm)
>>>  FORWARD_FEATURE(vcma)
>>>  FORWARD_FEATURE(dp)
>>> +FORWARD_FEATURE(fp16_arith)
>>>  
>>>  #undef FORWARD_FEATURE
>>>  
>>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>>> index c4969f163e..bcb2c9928c 100644
>>> --- a/linux-user/elfload.c
>>> +++ b/linux-user/elfload.c
>>> @@ -573,8 +573,6 @@ static uint32_t get_elf_hwcap(void)
>>>      hwcaps |= ARM_HWCAP_A64_ASIMD;
>>>  
>>>      /* probe for the extra features */
>>> -#define GET_FEATURE(feat, hwcap) \
>>> -    do { if (arm_feature(&cpu->env, feat)) { hwcaps |= hwcap; } } while (0)
>>>  #define GET_FEATURE_ID(feat, hwcap) \
>>>      do { if (aa64_feature_##feat(cpu)) { hwcaps |= hwcap; } } while (0)
>>>  
>>> @@ -587,15 +585,13 @@ static uint32_t get_elf_hwcap(void)
>>>      GET_FEATURE_ID(sha3, ARM_HWCAP_A64_SHA3);
>>>      GET_FEATURE_ID(sm3, ARM_HWCAP_A64_SM3);
>>>      GET_FEATURE_ID(sm4, ARM_HWCAP_A64_SM4);
>>> -    GET_FEATURE(ARM_FEATURE_V8_FP16,
>>> -                ARM_HWCAP_A64_FPHP | ARM_HWCAP_A64_ASIMDHP);
>>> +    GET_FEATURE_ID(fp16, ARM_HWCAP_A64_FPHP | ARM_HWCAP_A64_ASIMDHP);
>>>      GET_FEATURE_ID(atomics, ARM_HWCAP_A64_ATOMICS);
>>>      GET_FEATURE_ID(rdm, ARM_HWCAP_A64_ASIMDRDM);
>>>      GET_FEATURE_ID(dp, ARM_HWCAP_A64_ASIMDDP);
>>>      GET_FEATURE_ID(fcma, ARM_HWCAP_A64_FCMA);
>>>      GET_FEATURE_ID(sve, ARM_HWCAP_A64_SVE);
>>>  
>>> -#undef GET_FEATURE
>>>  #undef GET_FEATURE_ID
>>>  
>>>      return hwcaps;
>>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>>> index 8f95de677a..c734e2b2cd 100644
>>> --- a/target/arm/cpu64.c
>>> +++ b/target/arm/cpu64.c
>>> @@ -259,6 +259,8 @@ static void aarch64_max_initfn(Object *obj)
>>>          FIELD_DP64(cpu->id_aa64isar1, ID_AA64ISAR1, FCMA, 1);
>>>  
>>>          FIELD_DP64(cpu->id_aa64pfr0, ID_AA64PFR0, SVE, 1);
>>> +        FIELD_DP64(cpu->id_aa64pfr0, ID_AA64PFR0, FP, 1);
>>> +        FIELD_DP64(cpu->id_aa64pfr0, ID_AA64PFR0, ADVSIMD, 1);
>>>  
>>>          /* Replicate the same data to the 32-bit id registers.  */
>>>          FIELD_DP32(cpu->id_isar5, ID_ISAR5, AES, 2); /* AES + PMULL */
>>> @@ -268,15 +270,12 @@ static void aarch64_max_initfn(Object *obj)
>>>          FIELD_DP32(cpu->id_isar5, ID_ISAR5, RDM, 1);
>>>          FIELD_DP32(cpu->id_isar5, ID_ISAR5, VCMA, 1);
>>>          FIELD_DP32(cpu->id_isar6, ID_ISAR6, DP, 1);
>>> +        /*
>>> +         * FIXME: ARMv8.2-FP16 is not implemented for aa32,
>>> +         * so do not set MVFR1.FPHP and MVFR.SIMDHP.
>>> +         */
>>
>> Shouldn't we use this then?
>>
>>            FIELD_DP64(cpu->id_aa64pfr0, ID_AA64PFR0, FP, 0b1111);
> 
> 
> No, that would indicate no floating point unit at all in aa64 mode.

Indeed.

> What made you think this?

I misread this was aarch64_max_initfn() ;)

Regards,

Phil.