[v3,2/8] target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection

Commit Message

Aaron Lindsay June 9, 2023, 5:23 p.m. UTC

Signed-off-by: Aaron Lindsay <[e-mail]>
---
 target/arm/cpu.h              | 65 +++++++++++++++++++++++++++++++++--
 target/arm/tcg/pauth_helper.c |  2 +-
 2 files changed, 63 insertions(+), 4 deletions(-)

Comments

Richard Henderson June 9, 2023, 8:51 p.m. UTC | #1

On 6/9/23 10:23, Aaron Lindsay wrote:
> +static inline int isar_feature_pauth_get_features(const ARMISARegisters *id)
> +{
> +    if (isar_feature_aa64_pauth_arch_qarma5(id)) {
> +        return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, APA);
> +    } else if (isar_feature_aa64_pauth_arch_qarma3(id)) {
> +        return FIELD_EX64(id->id_aa64isar2, ID_AA64ISAR2, APA3);
> +    } else {
> +        return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, API);
> +    }
> +}

As I mentioned in previous review, exactly one of these fields will be non-zero, so you 
can just OR them all together without the conditionals.


r~

Aaron Lindsay June 12, 2023, 1:19 p.m. UTC | #2

On Jun 09 13:51, Richard Henderson wrote:
> On 6/9/23 10:23, Aaron Lindsay wrote:
> > +static inline int isar_feature_pauth_get_features(const ARMISARegisters *id)
> > +{
> > +    if (isar_feature_aa64_pauth_arch_qarma5(id)) {
> > +        return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, APA);
> > +    } else if (isar_feature_aa64_pauth_arch_qarma3(id)) {
> > +        return FIELD_EX64(id->id_aa64isar2, ID_AA64ISAR2, APA3);
> > +    } else {
> > +        return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, API);
> > +    }
> > +}
> 
> As I mentioned in previous review, exactly one of these fields will be
> non-zero, so you can just OR them all together without the conditionals.

Sorry I missed this last time around - I've queued this change for v4.

Thanks!

-Aaron

Patch

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index df04c9a9ab..22dd898577 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3714,18 +3714,77 @@  static inline bool isar_feature_aa64_pauth(const ARMISARegisters *id)
             (FIELD_DP64(0, ID_AA64ISAR1, APA, 0xf) |
              FIELD_DP64(0, ID_AA64ISAR1, API, 0xf) |
              FIELD_DP64(0, ID_AA64ISAR1, GPA, 0xf) |
-             FIELD_DP64(0, ID_AA64ISAR1, GPI, 0xf))) != 0;
+             FIELD_DP64(0, ID_AA64ISAR1, GPI, 0xf))) != 0 ||
+           (id->id_aa64isar2 &
+            (FIELD_DP64(0, ID_AA64ISAR2, APA3, 0xf) |
+             FIELD_DP64(0, ID_AA64ISAR2, GPA3, 0xf))) != 0;
 }
 
-static inline bool isar_feature_aa64_pauth_arch(const ARMISARegisters *id)
+static inline bool isar_feature_aa64_pauth_arch_qarma5(const ARMISARegisters *id)
 {
     /*
-     * Return true if pauth is enabled with the architected QARMA algorithm.
+     * Return true if pauth is enabled with the architected QARMA5 algorithm.
      * QEMU will always set APA+GPA to the same value.
      */
     return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, APA) != 0;
 }
 
+static inline bool isar_feature_aa64_pauth_arch_qarma3(const ARMISARegisters *id)
+{
+    /*
+     * Return true if pauth is enabled with the architected QARMA3 algorithm.
+     * QEMU will always set APA3+GPA3 to the same result.
+     */
+    return FIELD_EX64(id->id_aa64isar2, ID_AA64ISAR2, APA3) != 0;
+}
+
+static inline bool isar_feature_aa64_pauth_arch(const ARMISARegisters *id)
+{
+    return isar_feature_aa64_pauth_arch_qarma5(id) ||
+        isar_feature_aa64_pauth_arch_qarma3(id);
+}
+
+static inline int isar_feature_pauth_get_features(const ARMISARegisters *id)
+{
+    if (isar_feature_aa64_pauth_arch_qarma5(id)) {
+        return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, APA);
+    } else if (isar_feature_aa64_pauth_arch_qarma3(id)) {
+        return FIELD_EX64(id->id_aa64isar2, ID_AA64ISAR2, APA3);
+    } else {
+        return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, API);
+    }
+}
+
+static inline bool isar_feature_aa64_pauth_epac(const ARMISARegisters *id)
+{
+    /*
+     * Note that unlike most AArch64 features, EPAC is treated (in the ARM
+     * psedocode, at least) as not being implemented by larger values of this
+     * field. Our usage of '>=' rather than '==' here causes our implementation
+     * of PAC logic to diverge from ARM pseudocode - we must check that
+     * isar_feature_aa64_pauth2() returns false AND
+     * isar_feature_aa64_pauth_epac() returns true, where the pseudocode reads
+     * as if EPAC is not implemented if the value of this register is > 0b10.
+     * See the implementation of pauth_addpac() for an example.
+     */
+    return isar_feature_pauth_get_features(id) >= 0b0010;
+}
+
+static inline bool isar_feature_aa64_pauth2(const ARMISARegisters *id)
+{
+    return isar_feature_pauth_get_features(id) >= 0b0011;
+}
+
+static inline bool isar_feature_aa64_fpac(const ARMISARegisters *id)
+{
+    return isar_feature_pauth_get_features(id) >= 0b0100;
+}
+
+static inline bool isar_feature_aa64_fpac_combine(const ARMISARegisters *id)
+{
+    return isar_feature_pauth_get_features(id) >= 0b0101;
+}
+
 static inline bool isar_feature_aa64_tlbirange(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64isar0, ID_AA64ISAR0, TLB) == 2;
diff --git a/target/arm/tcg/pauth_helper.c b/target/arm/tcg/pauth_helper.c
index 62af569341..3ff4610a26 100644
--- a/target/arm/tcg/pauth_helper.c
+++ b/target/arm/tcg/pauth_helper.c
@@ -282,7 +282,7 @@  static uint64_t pauth_computepac_impdef(uint64_t data, uint64_t modifier,
 static uint64_t pauth_computepac(CPUARMState *env, uint64_t data,
                                  uint64_t modifier, ARMPACKey key)
 {
-    if (cpu_isar_feature(aa64_pauth_arch, env_archcpu(env))) {
+    if (cpu_isar_feature(aa64_pauth_arch_qarma5, env_archcpu(env))) {
         return pauth_computepac_architected(data, modifier, key);
     } else {
         return pauth_computepac_impdef(data, modifier, key);