[Qemu-devel] [PATCH 0/2] target/arm: Use MVFR feature bits to gate some insns

[Qemu-devel] [PATCH 0/2] target/arm: Use MVFR feature bits to gate some insns

[Peter Maydell]

This patchset switches some of the A32/T32 VFP decode to
look at the relevant MVFR feature bits rather than using
ARM_FEATURE_* feature flags:
 * use MVFR1.FPHP and .SIMDHP to gate the FP16 conversion insns
 * use MVFR2.FPMISC to gate the various insns in disas_vfp_v8_insn()

This is a bit of preparatory work for v7M/v8M floating point
support: the v7M FPU has the "misc" insns that only arrived
in A-profile in v8, and also the FP16 to/from double precision
conversion insns which are v8-only. So feature checks that look
at ARM_FEATURE_V8 won't work there, and we need to look at the
MVFR* fields instead.

This should have no behavioural changes for current CPUs.

thanks
-- PMM

Peter Maydell (2):
  target/arm: Use MVFR1 feature bits to gate A32/T32 FP16 instructions
  target/arm: Gate "miscellaneous FP" insns by ID register field

 target/arm/cpu.h       | 57 +++++++++++++++++++++++++++++++++++++++++-
 target/arm/cpu.c       |  2 --
 target/arm/kvm32.c     |  3 ---
 target/arm/translate.c | 47 ++++++++++++++++++++++------------
 4 files changed, 87 insertions(+), 22 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] target/arm: Use MVFR1 feature bits to gate A32/T32 FP16 instructions

[Peter Maydell]

Instead of gating the A32/T32 FP16 conversion instructions on
the ARM_FEATURE_VFP_FP16 flag, switch to our new approach of
looking at ID register bits. In this case MVFR1 fields FPHP
and SIMDHP indicate the presence of these insns.

This change doesn't alter behaviour for any of our CPUs.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h       | 37 ++++++++++++++++++++++++++++++++++++-
 target/arm/cpu.c       |  2 --
 target/arm/kvm32.c     |  3 ---
 target/arm/translate.c | 26 ++++++++++++++++++--------
 4 files changed, 54 insertions(+), 14 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 1eea1a408b8..36ea3b58567 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1730,6 +1730,27 @@ FIELD(ID_DFR0, MPROFDBG, 20, 4)
 FIELD(ID_DFR0, PERFMON, 24, 4)
 FIELD(ID_DFR0, TRACEFILT, 28, 4)
 
+FIELD(MVFR0, SIMDREG, 0, 4)
+FIELD(MVFR0, FPSP, 4, 4)
+FIELD(MVFR0, FPDP, 8, 4)
+FIELD(MVFR0, FPTRAP, 12, 4)
+FIELD(MVFR0, FPDIVIDE, 16, 4)
+FIELD(MVFR0, FPSQRT, 20, 4)
+FIELD(MVFR0, FPSHVEC, 24, 4)
+FIELD(MVFR0, FPROUND, 28, 4)
+
+FIELD(MVFR1, FPFTZ, 0, 4)
+FIELD(MVFR1, FPDNAN, 4, 4)
+FIELD(MVFR1, SIMDLS, 8, 4)
+FIELD(MVFR1, SIMDINT, 12, 4)
+FIELD(MVFR1, SIMDSP, 16, 4)
+FIELD(MVFR1, SIMDHP, 20, 4)
+FIELD(MVFR1, FPHP, 24, 4)
+FIELD(MVFR1, SIMDFMAC, 28, 4)
+
+FIELD(MVFR2, SIMDMISC, 0, 4)
+FIELD(MVFR2, FPMISC, 4, 4)
+
 QEMU_BUILD_BUG_ON(ARRAY_SIZE(((ARMCPU *)0)->ccsidr) <= R_V7M_CSSELR_INDEX_MASK);
 
 /* If adding a feature bit which corresponds to a Linux ELF
@@ -1747,7 +1768,6 @@ enum arm_features {
     ARM_FEATURE_THUMB2,
     ARM_FEATURE_PMSA,   /* no MMU; may have Memory Protection Unit */
     ARM_FEATURE_VFP3,
-    ARM_FEATURE_VFP_FP16,
     ARM_FEATURE_NEON,
     ARM_FEATURE_M, /* Microcontroller profile.  */
     ARM_FEATURE_OMAPCP, /* OMAP specific CP15 ops handling.  */
@@ -3293,6 +3313,21 @@ static inline bool isar_feature_aa32_fp16_arith(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64pfr0, ID_AA64PFR0, FP) == 1;
 }
 
+/*
+ * We always set the FP and SIMD FP16 fields to indicate identical
+ * levels of support (assuming SIMD is implemented at all), so
+ * we only need one set of accessors.
+ */
+static inline bool isar_feature_aa32_fp16_spconv(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->mvfr1, MVFR1, FPHP) > 0;
+}
+
+static inline bool isar_feature_aa32_fp16_dpconv(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->mvfr1, MVFR1, FPHP) > 1;
+}
+
 /*
  * 64-bit feature tests via id registers.
  */
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 4d7f6a3bc0c..a3baf4eeed1 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1014,7 +1014,6 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     }
     if (arm_feature(env, ARM_FEATURE_VFP4)) {
         set_feature(env, ARM_FEATURE_VFP3);
-        set_feature(env, ARM_FEATURE_VFP_FP16);
     }
     if (arm_feature(env, ARM_FEATURE_VFP3)) {
         set_feature(env, ARM_FEATURE_VFP);
@@ -1675,7 +1674,6 @@ static void cortex_a9_initfn(Object *obj)
     cpu->dtb_compatible = "arm,cortex-a9";
     set_feature(&cpu->env, ARM_FEATURE_V7);
     set_feature(&cpu->env, ARM_FEATURE_VFP3);
-    set_feature(&cpu->env, ARM_FEATURE_VFP_FP16);
     set_feature(&cpu->env, ARM_FEATURE_NEON);
     set_feature(&cpu->env, ARM_FEATURE_THUMB2EE);
     set_feature(&cpu->env, ARM_FEATURE_EL3);
diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index a75e04cc8f3..327375f6252 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -125,9 +125,6 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     if (extract32(id_pfr0, 12, 4) == 1) {
         set_feature(&features, ARM_FEATURE_THUMB2EE);
     }
-    if (extract32(ahcf->isar.mvfr1, 20, 4) == 1) {
-        set_feature(&features, ARM_FEATURE_VFP_FP16);
-    }
     if (extract32(ahcf->isar.mvfr1, 12, 4) == 1) {
         set_feature(&features, ARM_FEATURE_NEON);
     }
diff --git a/target/arm/translate.c b/target/arm/translate.c
index c1175798ac9..b7702fb49f7 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -3663,17 +3663,27 @@ static int disas_vfp_insn(DisasContext *s, uint32_t insn)
                      * UNPREDICTABLE if bit 8 is set prior to ARMv8
                      * (we choose to UNDEF)
                      */
-                    if ((dp && !arm_dc_feature(s, ARM_FEATURE_V8)) ||
-                        !arm_dc_feature(s, ARM_FEATURE_VFP_FP16)) {
-                        return 1;
+                    if (dp) {
+                        if (!dc_isar_feature(aa32_fp16_dpconv, s)) {
+                            return 1;
+                        }
+                    } else {
+                        if (!dc_isar_feature(aa32_fp16_spconv, s)) {
+                            return 1;
+                        }
                     }
                     rm_is_dp = false;
                     break;
                 case 0x06: /* vcvtb.f16.f32, vcvtb.f16.f64 */
                 case 0x07: /* vcvtt.f16.f32, vcvtt.f16.f64 */
-                    if ((dp && !arm_dc_feature(s, ARM_FEATURE_V8)) ||
-                        !arm_dc_feature(s, ARM_FEATURE_VFP_FP16)) {
-                        return 1;
+                    if (dp) {
+                        if (!dc_isar_feature(aa32_fp16_dpconv, s)) {
+                            return 1;
+                        }
+                    } else {
+                        if (!dc_isar_feature(aa32_fp16_spconv, s)) {
+                            return 1;
+                        }
                     }
                     rd_is_dp = false;
                     break;
@@ -7876,7 +7886,7 @@ static int disas_neon_data_insn(DisasContext *s, uint32_t insn)
                     TCGv_ptr fpst;
                     TCGv_i32 ahp;
 
-                    if (!arm_dc_feature(s, ARM_FEATURE_VFP_FP16) ||
+                    if (!dc_isar_feature(aa32_fp16_spconv, s) ||
                         q || (rm & 1)) {
                         return 1;
                     }
@@ -7908,7 +7918,7 @@ static int disas_neon_data_insn(DisasContext *s, uint32_t insn)
                 {
                     TCGv_ptr fpst;
                     TCGv_i32 ahp;
-                    if (!arm_dc_feature(s, ARM_FEATURE_VFP_FP16) ||
+                    if (!dc_isar_feature(aa32_fp16_spconv, s) ||
                         q || (rd & 1)) {
                         return 1;
                     }
-- 
2.20.1

[Qemu-devel] [PATCH 2/2] target/arm: Gate "miscellaneous FP" insns by ID register field

[Peter Maydell]

There is a set of VFP instructions which we implement in
disas_vfp_v8_insn() and gate on the ARM_FEATURE_V8 bit.
These were all first introduced in v8 for A-profile, but in
M-profile they appeared in v7M. Gate them on the MVFR2
FPMisc field instead, and rename the function appropriately.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h       | 20 ++++++++++++++++++++
 target/arm/translate.c | 21 +++++++++++++--------
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 36ea3b58567..ff9ba387b42 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3328,6 +3328,26 @@ static inline bool isar_feature_aa32_fp16_dpconv(const ARMISARegisters *id)
     return FIELD_EX64(id->mvfr1, MVFR1, FPHP) > 1;
 }
 
+static inline bool isar_feature_aa32_vsel(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->mvfr2, MVFR2, FPMISC) >= 1;
+}
+
+static inline bool isar_feature_aa32_vcvt_dr(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->mvfr2, MVFR2, FPMISC) >= 2;
+}
+
+static inline bool isar_feature_aa32_vrint(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->mvfr2, MVFR2, FPMISC) >= 3;
+}
+
+static inline bool isar_feature_aa32_vminmaxnm(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->mvfr2, MVFR2, FPMISC) >= 4;
+}
+
 /*
  * 64-bit feature tests via id registers.
  */
diff --git a/target/arm/translate.c b/target/arm/translate.c
index b7702fb49f7..af8f9e669b8 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -3357,7 +3357,7 @@ static const uint8_t fp_decode_rm[] = {
     FPROUNDING_NEGINF,
 };
 
-static int disas_vfp_v8_insn(DisasContext *s, uint32_t insn)
+static int disas_vfp_misc_insn(DisasContext *s, uint32_t insn)
 {
     uint32_t rd, rn, rm, dp = extract32(insn, 8, 1);
 
@@ -3375,15 +3375,18 @@ static int disas_vfp_v8_insn(DisasContext *s, uint32_t insn)
         rm = VFP_SREG_M(insn);
     }
 
-    if ((insn & 0x0f800e50) == 0x0e000a00) {
+    if ((insn & 0x0f800e50) == 0x0e000a00 && dc_isar_feature(aa32_vsel, s)) {
         return handle_vsel(insn, rd, rn, rm, dp);
-    } else if ((insn & 0x0fb00e10) == 0x0e800a00) {
+    } else if ((insn & 0x0fb00e10) == 0x0e800a00 &&
+               dc_isar_feature(aa32_vminmaxnm, s)) {
         return handle_vminmaxnm(insn, rd, rn, rm, dp);
-    } else if ((insn & 0x0fbc0ed0) == 0x0eb80a40) {
+    } else if ((insn & 0x0fbc0ed0) == 0x0eb80a40 &&
+               dc_isar_feature(aa32_vrint, s)) {
         /* VRINTA, VRINTN, VRINTP, VRINTM */
         int rounding = fp_decode_rm[extract32(insn, 16, 2)];
         return handle_vrint(insn, rd, rm, dp, rounding);
-    } else if ((insn & 0x0fbc0e50) == 0x0ebc0a40) {
+    } else if ((insn & 0x0fbc0e50) == 0x0ebc0a40 &&
+               dc_isar_feature(aa32_vcvt_dr, s)) {
         /* VCVTA, VCVTN, VCVTP, VCVTM */
         int rounding = fp_decode_rm[extract32(insn, 16, 2)];
         return handle_vcvt(insn, rd, rm, dp, rounding);
@@ -3427,10 +3430,12 @@ static int disas_vfp_insn(DisasContext *s, uint32_t insn)
     }
 
     if (extract32(insn, 28, 4) == 0xf) {
-        /* Encodings with T=1 (Thumb) or unconditional (ARM):
-         * only used in v8 and above.
+        /*
+         * Encodings with T=1 (Thumb) or unconditional (ARM):
+         * only used for the "miscellaneous VFP features" added in v8A
+         * and v7M (and gated on the MVFR2.FPMisc field).
          */
-        return disas_vfp_v8_insn(s, insn);
+        return disas_vfp_misc_insn(s, insn);
     }
 
     dp = ((insn & 0xf00) == 0xb00);
-- 
2.20.1

Re: [Qemu-devel] [PATCH 2/2] target/arm: Gate "miscellaneous FP" insns by ID register field

[Peter Maydell]

On Fri, 22 Feb 2019 at 17:09, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> There is a set of VFP instructions which we implement in
> disas_vfp_v8_insn() and gate on the ARM_FEATURE_V8 bit.
> These were all first introduced in v8 for A-profile, but in
> M-profile they appeared in v7M. Gate them on the MVFR2
> FPMisc field instead, and rename the function appropriately.

> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index b7702fb49f7..af8f9e669b8 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -3357,7 +3357,7 @@ static const uint8_t fp_decode_rm[] = {
>      FPROUNDING_NEGINF,
>  };
>
> -static int disas_vfp_v8_insn(DisasContext *s, uint32_t insn)
> +static int disas_vfp_misc_insn(DisasContext *s, uint32_t insn)
>  {
>      uint32_t rd, rn, rm, dp = extract32(insn, 8, 1);
>

Oops, I forgot to commit this part of the change:

@@ -3361,10 +3361,6 @@ static int disas_vfp_misc_insn(DisasContext *s,
uint32_t insn)
 {
     uint32_t rd, rn, rm, dp = extract32(insn, 8, 1);

-    if (!arm_dc_feature(s, ARM_FEATURE_V8)) {
-        return 1;
-    }
-
     if (dp) {
         VFP_DREG_D(rd, insn);
         VFP_DREG_N(rn, insn);

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/2] target/arm: Use MVFR1 feature bits to gate A32/T32 FP16 instructions

[Richard Henderson]

On 2/22/19 9:09 AM, Peter Maydell wrote:
> Instead of gating the A32/T32 FP16 conversion instructions on
> the ARM_FEATURE_VFP_FP16 flag, switch to our new approach of
> looking at ID register bits. In this case MVFR1 fields FPHP
> and SIMDHP indicate the presence of these insns.
> 
> This change doesn't alter behaviour for any of our CPUs.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h       | 37 ++++++++++++++++++++++++++++++++++++-
>  target/arm/cpu.c       |  2 --
>  target/arm/kvm32.c     |  3 ---
>  target/arm/translate.c | 26 ++++++++++++++++++--------
>  4 files changed, 54 insertions(+), 14 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 2/2] target/arm: Gate "miscellaneous FP" insns by ID register field

[Richard Henderson]

On 2/22/19 9:36 AM, Peter Maydell wrote:
> On Fri, 22 Feb 2019 at 17:09, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> There is a set of VFP instructions which we implement in
>> disas_vfp_v8_insn() and gate on the ARM_FEATURE_V8 bit.
>> These were all first introduced in v8 for A-profile, but in
>> M-profile they appeared in v7M. Gate them on the MVFR2
>> FPMisc field instead, and rename the function appropriately.
> 
>> diff --git a/target/arm/translate.c b/target/arm/translate.c
>> index b7702fb49f7..af8f9e669b8 100644
>> --- a/target/arm/translate.c
>> +++ b/target/arm/translate.c
>> @@ -3357,7 +3357,7 @@ static const uint8_t fp_decode_rm[] = {
>>      FPROUNDING_NEGINF,
>>  };
>>
>> -static int disas_vfp_v8_insn(DisasContext *s, uint32_t insn)
>> +static int disas_vfp_misc_insn(DisasContext *s, uint32_t insn)
>>  {
>>      uint32_t rd, rn, rm, dp = extract32(insn, 8, 1);
>>
> 
> Oops, I forgot to commit this part of the change:
> 
> @@ -3361,10 +3361,6 @@ static int disas_vfp_misc_insn(DisasContext *s,
> uint32_t insn)
>  {
>      uint32_t rd, rn, rm, dp = extract32(insn, 8, 1);
> 
> -    if (!arm_dc_feature(s, ARM_FEATURE_V8)) {
> -        return 1;
> -    }
> -
>      if (dp) {
>          VFP_DREG_D(rd, insn);
>          VFP_DREG_N(rn, insn);

With the addition,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~