Re: [Qemu-devel] [PATCH v3 10/10] target/arm: Convert v8.2-fp16 from feature bit to aa64pfr0 test

2018-10-16 Thread Richard Henderson
On 10/16/18 3:36 AM, Peter Maydell wrote:
> On 8 October 2018 at 22:22, Richard Henderson
>  wrote:
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Signed-off-by: Richard Henderson 
>> ---
>>  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 |  9 ++---
>>  target/arm/helper.c|  2 +-
>>  target/arm/translate-a64.c | 40 +++---
>>  target/arm/translate.c |  6 +++---
>>  8 files changed, 45 insertions(+), 37 deletions(-)
>> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
>> index ee2c04a627..38e9afef3b 100644
>> --- a/target/arm/cpu64.c
>> +++ b/target/arm/cpu64.c
>> @@ -266,6 +266,8 @@ static void aarch64_max_initfn(Object *obj)
>>
>>  t = cpu->id_aa64pfr0;
>>  t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1);
>> +t = FIELD_DP64(t, ID_AA64PFR0, FP, 1);
>> +t = FIELD_DP64(t, ID_AA64PFR0, ADVSIMD, 1);
>>  cpu->id_aa64pfr0 = t;
>>
>>  /* Replicate the same data to the 32-bit id registers.  */
>> @@ -283,13 +285,6 @@ static void aarch64_max_initfn(Object *obj)
>>  cpu->id_isar6 = u;
>>
>>  #ifdef CONFIG_USER_ONLY
>> -/* We don't set these in system emulation mode for the moment,
>> - * since we don't correctly set the ID registers to advertise them,
>> - * and in some cases they're only available in AArch64 and not 
>> AArch32,
>> - * whereas the architecture requires them to be present in both if
>> - * present in either.
>> - */
>> -set_feature(&cpu->env, ARM_FEATURE_V8_FP16);
> 
> FP16 is the feature that this comment refers to about not having the
> AArch32 support present yet. So previously we only set that feature
> bit in the user-only mode. Doesn't that mean we need to only
> set the equivalent PFR0 bits in the ID register in user-only mode now?

If we do that, then we violate the SVE rule that FP16 must be present.  I think
it's more valuable to have SVE available in system mode than the more obscure
AArch64 <-> AArch32 feature correspondence.


r~



Re: [Qemu-devel] [PATCH v3 10/10] target/arm: Convert v8.2-fp16 from feature bit to aa64pfr0 test

2018-10-16 Thread Peter Maydell
On 8 October 2018 at 22:22, Richard Henderson
 wrote:
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Richard Henderson 
> ---
>  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 |  9 ++---
>  target/arm/helper.c|  2 +-
>  target/arm/translate-a64.c | 40 +++---
>  target/arm/translate.c |  6 +++---
>  8 files changed, 45 insertions(+), 37 deletions(-)
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index ee2c04a627..38e9afef3b 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -266,6 +266,8 @@ static void aarch64_max_initfn(Object *obj)
>
>  t = cpu->id_aa64pfr0;
>  t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1);
> +t = FIELD_DP64(t, ID_AA64PFR0, FP, 1);
> +t = FIELD_DP64(t, ID_AA64PFR0, ADVSIMD, 1);
>  cpu->id_aa64pfr0 = t;
>
>  /* Replicate the same data to the 32-bit id registers.  */
> @@ -283,13 +285,6 @@ static void aarch64_max_initfn(Object *obj)
>  cpu->id_isar6 = u;
>
>  #ifdef CONFIG_USER_ONLY
> -/* We don't set these in system emulation mode for the moment,
> - * since we don't correctly set the ID registers to advertise them,
> - * and in some cases they're only available in AArch64 and not 
> AArch32,
> - * whereas the architecture requires them to be present in both if
> - * present in either.
> - */
> -set_feature(&cpu->env, ARM_FEATURE_V8_FP16);

FP16 is the feature that this comment refers to about not having the
AArch32 support present yet. So previously we only set that feature
bit in the user-only mode. Doesn't that mean we need to only
set the equivalent PFR0 bits in the ID register in user-only mode now?

thanks
-- PMM



[Qemu-devel] [PATCH v3 10/10] target/arm: Convert v8.2-fp16 from feature bit to aa64pfr0 test

2018-10-08 Thread Richard Henderson
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Richard Henderson 
---
 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 |  9 ++---
 target/arm/helper.c|  2 +-
 target/arm/translate-a64.c | 40 +++---
 target/arm/translate.c |  6 +++---
 8 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index a97b471fff..1c880b0c29 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1589,7 +1589,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 */
 };
 
@@ -3204,6 +3203,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.
  */
@@ -3272,6 +3281,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 bd394bdf69..ad911de98c 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -206,6 +206,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 e3585f4cb6..d041ef9d49 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 ee2c04a627..38e9afef3b 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -266,6 +266,8 @@ static void aarch64_max_initfn(Object *obj)
 
 t = cpu->id_aa64pfr0;
 t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1);
+t = FIELD_DP64(t, ID_AA64PFR0, FP, 1);
+t = FIELD_DP64(t, ID_AA64PFR0, ADVSIMD, 1);
 cpu->id_aa64pfr0 = t;
 
 /* Replicate the same data to the 32-bit id registers.  */
@@ -283,13 +285,6 @@ static void aarch64_max_initfn(Object *obj)
 cpu->id_isar6 = u;
 
 #ifdef CONFIG_USER_ONLY
-/* We don't set these in system emulation mode for the moment,
- * since we don't correctly set the ID registers to advertise them,
- * and in some cases they're only available in AArch64 and not AArch32,
- * whereas the architecture requires them to be present in both if
- * present in either.
- */
-set_feature(&cpu->env, ARM_FEATURE_V8_FP16);
 /* For usermode -cpu max we can use a larger and more efficient DCZ
  * blocksize since we don't have to follow what the hardware does.
  */
diff --git a/target/arm/helper.c b/target/arm/h