[PATCH 0/7] Implement Most ARMv8.3 Pointer Authentication Features

[PATCH 0/7] Implement Most ARMv8.3 Pointer Authentication Features

[Aaron Lindsay]

Hello,

I've taken a first pass at implementing many of the ARMv8.3 Pointer
Authentication features and welcome your review.

Thanks!

-Aaron

Aaron Lindsay (7):
  target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection
  target/arm: Implement v8.3 QARMA3 PAC cipher
  target/arm: Implement v8.3 EnhancedPAC
  target/arm: Implement v8.3 Pauth2
  targer/arm: Inform helpers whether a PAC instruction is 'combined'
  target/arm: Implement v8.3 FPAC and FPACCOMBINE
  target/arm: Add CPU properties for most v8.3 PAC features

 target/arm/cpu.h           |  62 ++++++++++++-
 target/arm/cpu64.c         |  81 ++++++++++++++---
 target/arm/helper-a64.h    |   4 +
 target/arm/helper.c        |   4 +-
 target/arm/pauth_helper.c  | 182 ++++++++++++++++++++++++++++++-------
 target/arm/syndrome.h      |   6 ++
 target/arm/translate-a64.c |  20 ++--
 7 files changed, 296 insertions(+), 63 deletions(-)

-- 
2.25.1

[PATCH 1/7] target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
---
 target/arm/cpu.h          | 57 ++++++++++++++++++++++++++++++++++++---
 target/arm/helper.c       |  4 +--
 target/arm/pauth_helper.c |  4 +--
 3 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8cf70693be..9be59163ff 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1001,6 +1001,7 @@ struct ArchCPU {
         uint32_t dbgdevid1;
         uint64_t id_aa64isar0;
         uint64_t id_aa64isar1;
+        uint64_t id_aa64isar2;
         uint64_t id_aa64pfr0;
         uint64_t id_aa64pfr1;
         uint64_t id_aa64mmfr0;
@@ -3902,18 +3903,68 @@ 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 value.
+     */
+    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 uint8_t 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)
+{
+    return isar_feature_pauth_get_features(id) == 0b0010;
+}
+
+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_fpac(const ARMISARegisters *id)
+{
+    return isar_feature_pauth_get_features(id) == 0b0100 ||
+        isar_feature_aa64_fpac_combine(id);
+}
+
+static inline bool isar_feature_aa64_pauth2(const ARMISARegisters *id)
+{
+    return isar_feature_pauth_get_features(id) == 0b0011 ||
+        isar_feature_aa64_fpac(id);
+}
+
 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/helper.c b/target/arm/helper.c
index 72b37b7cf1..448ebf8301 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8028,11 +8028,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .access = PL1_R, .type = ARM_CP_CONST,
               .accessfn = access_aa64_tid3,
               .resetvalue = cpu->isar.id_aa64isar1 },
-            { .name = "ID_AA64ISAR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
+            { .name = "ID_AA64ISAR2_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 2,
               .access = PL1_R, .type = ARM_CP_CONST,
               .accessfn = access_aa64_tid3,
-              .resetvalue = 0 },
+              .resetvalue = cpu->isar.id_aa64isar2 },
             { .name = "ID_AA64ISAR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 3,
               .access = PL1_R, .type = ARM_CP_CONST,
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index d0483bf051..a0c9bea06b 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -282,8 +282,8 @@ 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))) {
-        return pauth_computepac_architected(data, modifier, key);
+    if (cpu_isar_feature(aa64_pauth_arch_qarma5, env_archcpu(env))) {
+        return pauth_computepac_architected(data, modifier, key, false);
     } else {
         return pauth_computepac_impdef(data, modifier, key);
     }
-- 
2.25.1

Re: [PATCH 1/7] target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection

[Peter Maydell]

On Thu, 2 Feb 2023 at 21:13, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
>
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> ---
>  target/arm/cpu.h          | 57 ++++++++++++++++++++++++++++++++++++---
>  target/arm/helper.c       |  4 +--
>  target/arm/pauth_helper.c |  4 +--
>  3 files changed, 58 insertions(+), 7 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 8cf70693be..9be59163ff 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1001,6 +1001,7 @@ struct ArchCPU {
>          uint32_t dbgdevid1;
>          uint64_t id_aa64isar0;
>          uint64_t id_aa64isar1;
> +        uint64_t id_aa64isar2;
>          uint64_t id_aa64pfr0;
>          uint64_t id_aa64pfr1;
>          uint64_t id_aa64mmfr0;
> @@ -3902,18 +3903,68 @@ 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 value.
> +     */
> +    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 uint8_t 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)
> +{
> +    return isar_feature_pauth_get_features(id) == 0b0010;

This should ideally be ">= 0b0010", but it depends a bit on
where we call it.

This field, like most ID register fields, follows the "standard
scheme", where the value increases and larger numbers always
imply "all of the functionality from the lower values, plus
some more". In QEMU we implement this by doing a >= type
comparison on the field value.

The PAC related ID fields are documented slightly confusingly,
but they do work this way. The documentation suggests it might not
be quite that way for FEAT_EPAC because it says that
HaveEnhancedPAC() returns TRUE for 2 and FALSE for 3 and up.
However this is more because the definition of the architectural
features means that FEAT_EPAC is irrelevant, and it's an accident
of the way the pseudocode was written that means that
HaveEnhancedPAC() ever gets called when FEAT_PAuth2 is present.

Other than EPAC, the rest of the values in these fields are
straightforward and we can implement the isar_feature functions
with a single >= comparison.

> +}
> +
> +static inline bool isar_feature_aa64_fpac_combine(const ARMISARegisters *id)
> +{
> +    return isar_feature_pauth_get_features(id) == 0b0101;

Should be ">= 0b0101".

> +}
> +
> +static inline bool isar_feature_aa64_fpac(const ARMISARegisters *id)
> +{
> +    return isar_feature_pauth_get_features(id) == 0b0100 ||
> +        isar_feature_aa64_fpac_combine(id);

Should be ">= 0b0100", no need to || with fpac_combine().

> +}
> +
> +static inline bool isar_feature_aa64_pauth2(const ARMISARegisters *id)
> +{
> +    return isar_feature_pauth_get_features(id) == 0b0011 ||
> +        isar_feature_aa64_fpac(id);

Should be ">= 0b0011", no need to || with fpac().

> +}
> +
>  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/helper.c b/target/arm/helper.c
> index 72b37b7cf1..448ebf8301 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8028,11 +8028,11 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .access = PL1_R, .type = ARM_CP_CONST,
>                .accessfn = access_aa64_tid3,
>                .resetvalue = cpu->isar.id_aa64isar1 },
> -            { .name = "ID_AA64ISAR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
> +            { .name = "ID_AA64ISAR2_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 2,
>                .access = PL1_R, .type = ARM_CP_CONST,
>                .accessfn = access_aa64_tid3,
> -              .resetvalue = 0 },
> +              .resetvalue = cpu->isar.id_aa64isar2 },
>              { .name = "ID_AA64ISAR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 3,
>                .access = PL1_R, .type = ARM_CP_CONST,
> diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
> index d0483bf051..a0c9bea06b 100644
> --- a/target/arm/pauth_helper.c
> +++ b/target/arm/pauth_helper.c
> @@ -282,8 +282,8 @@ 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))) {
> -        return pauth_computepac_architected(data, modifier, key);
> +    if (cpu_isar_feature(aa64_pauth_arch_qarma5, env_archcpu(env))) {
> +        return pauth_computepac_architected(data, modifier, key, false);
>      } else {
>          return pauth_computepac_impdef(data, modifier, key);
>      }
> --
> 2.25.1

thanks
-- PMM

Re: [PATCH 1/7] target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection

[Aaron Lindsay]

On Feb 13 16:01, Peter Maydell wrote:
> On Thu, 2 Feb 2023 at 21:13, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
> > +static inline bool isar_feature_aa64_pauth_epac(const ARMISARegisters *id)
> > +{
> > +    return isar_feature_pauth_get_features(id) == 0b0010;
> 
> This should ideally be ">= 0b0010", but it depends a bit on
> where we call it.

FYI - I did make this `>= 0b0010` after changing the logic around elsewhere to
be compatible as you suggested. I'm also planning to add a comment like this:

 /*
  * 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 slightly from ARM pseudocode.
  */


> This field, like most ID register fields, follows the "standard
> scheme", where the value increases and larger numbers always
> imply "all of the functionality from the lower values, plus
> some more". In QEMU we implement this by doing a >= type
> comparison on the field value.
> 
> The PAC related ID fields are documented slightly confusingly,
> but they do work this way. The documentation suggests it might not
> be quite that way for FEAT_EPAC because it says that
> HaveEnhancedPAC() returns TRUE for 2 and FALSE for 3 and up.
> However this is more because the definition of the architectural
> features means that FEAT_EPAC is irrelevant, and it's an accident
> of the way the pseudocode was written that means that
> HaveEnhancedPAC() ever gets called when FEAT_PAuth2 is present.
> 
> Other than EPAC, the rest of the values in these fields are
> straightforward and we can implement the isar_feature functions
> with a single >= comparison.

Thanks for your review!

I've made a number of your (simpler) suggested changes already, and will
target getting a new patchset out in the next couple weeks after I spend
more time withi the the remaining GETPC() changes that need a little
more thought/rework, and we sort out what the command-line options
should look like.

-Aaron

Re: [PATCH 1/7] target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection

[Peter Maydell]

On Thu, 2 Feb 2023 at 21:13, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
>
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>


> diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
> index d0483bf051..a0c9bea06b 100644
> --- a/target/arm/pauth_helper.c
> +++ b/target/arm/pauth_helper.c
> @@ -282,8 +282,8 @@ 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))) {
> -        return pauth_computepac_architected(data, modifier, key);
> +    if (cpu_isar_feature(aa64_pauth_arch_qarma5, env_archcpu(env))) {
> +        return pauth_computepac_architected(data, modifier, key, false);
>      } else {
>          return pauth_computepac_impdef(data, modifier, key);
>      }
> --
> 2.25.1

Just noticed -- this should I guess be in a later patch, because
it's added an extra argument to the function call without changing
the function definition or declaration, so I think this patch
won't compile as-is.

-- PMM

[PATCH 2/7] target/arm: Implement v8.3 QARMA3 PAC cipher

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
---
 target/arm/pauth_helper.c | 48 +++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index a0c9bea06b..f525ef7fad 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -96,6 +96,21 @@ static uint64_t pac_sub(uint64_t i)
     return o;
 }
 
+static uint64_t pac_sub1(uint64_t i)
+{
+    static const uint8_t sub1[16] = {
+        0xa, 0xd, 0xe, 0x6, 0xf, 0x7, 0x3, 0x5,
+        0x9, 0x8, 0x0, 0xc, 0xb, 0x1, 0x2, 0x4,
+    };
+    uint64_t o = 0;
+    int b;
+
+    for (b = 0; b < 64; b += 4) {
+        o |= (uint64_t)sub1[(i >> b) & 0xf] << b;
+    }
+    return o;
+}
+
 static uint64_t pac_inv_sub(uint64_t i)
 {
     static const uint8_t inv_sub[16] = {
@@ -209,7 +224,7 @@ static uint64_t tweak_inv_shuffle(uint64_t i)
 }
 
 static uint64_t pauth_computepac_architected(uint64_t data, uint64_t modifier,
-                                             ARMPACKey key)
+                                             ARMPACKey key, bool isqarma3)
 {
     static const uint64_t RC[5] = {
         0x0000000000000000ull,
@@ -219,6 +234,7 @@ static uint64_t pauth_computepac_architected(uint64_t data, uint64_t modifier,
         0x452821E638D01377ull,
     };
     const uint64_t alpha = 0xC0AC29B7C97C50DDull;
+    int iterations = isqarma3 ? 2 : 4;
     /*
      * Note that in the ARM pseudocode, key0 contains bits <127:64>
      * and key1 contains bits <63:0> of the 128-bit key.
@@ -231,7 +247,7 @@ static uint64_t pauth_computepac_architected(uint64_t data, uint64_t modifier,
     runningmod = modifier;
     workingval = data ^ key0;
 
-    for (i = 0; i <= 4; ++i) {
+    for (i = 0; i <= iterations; ++i) {
         roundkey = key1 ^ runningmod;
         workingval ^= roundkey;
         workingval ^= RC[i];
@@ -239,32 +255,44 @@ static uint64_t pauth_computepac_architected(uint64_t data, uint64_t modifier,
             workingval = pac_cell_shuffle(workingval);
             workingval = pac_mult(workingval);
         }
-        workingval = pac_sub(workingval);
+        if (isqarma3)
+            workingval = pac_sub1(workingval);
+        else
+            workingval = pac_sub(workingval);
         runningmod = tweak_shuffle(runningmod);
     }
     roundkey = modk0 ^ runningmod;
     workingval ^= roundkey;
     workingval = pac_cell_shuffle(workingval);
     workingval = pac_mult(workingval);
-    workingval = pac_sub(workingval);
+    if (isqarma3)
+        workingval = pac_sub1(workingval);
+    else
+        workingval = pac_sub(workingval);
     workingval = pac_cell_shuffle(workingval);
     workingval = pac_mult(workingval);
     workingval ^= key1;
     workingval = pac_cell_inv_shuffle(workingval);
-    workingval = pac_inv_sub(workingval);
+    if (isqarma3)
+        workingval = pac_sub1(workingval);
+    else
+        workingval = pac_inv_sub(workingval);
     workingval = pac_mult(workingval);
     workingval = pac_cell_inv_shuffle(workingval);
     workingval ^= key0;
     workingval ^= runningmod;
-    for (i = 0; i <= 4; ++i) {
-        workingval = pac_inv_sub(workingval);
-        if (i < 4) {
+    for (i = 0; i <= iterations; ++i) {
+        if (isqarma3)
+            workingval = pac_sub1(workingval);
+        else
+            workingval = pac_inv_sub(workingval);
+        if (i < iterations) {
             workingval = pac_mult(workingval);
             workingval = pac_cell_inv_shuffle(workingval);
         }
         runningmod = tweak_inv_shuffle(runningmod);
         roundkey = key1 ^ runningmod;
-        workingval ^= RC[4 - i];
+        workingval ^= RC[iterations - i];
         workingval ^= roundkey;
         workingval ^= alpha;
     }
@@ -284,6 +312,8 @@ static uint64_t pauth_computepac(CPUARMState *env, uint64_t data,
 {
     if (cpu_isar_feature(aa64_pauth_arch_qarma5, env_archcpu(env))) {
         return pauth_computepac_architected(data, modifier, key, false);
+    } else if (cpu_isar_feature(aa64_pauth_arch_qarma3, env_archcpu(env))) {
+        return pauth_computepac_architected(data, modifier, key, true);
     } else {
         return pauth_computepac_impdef(data, modifier, key);
     }
-- 
2.25.1

Re: [PATCH 2/7] target/arm: Implement v8.3 QARMA3 PAC cipher

[Peter Maydell]

On Thu, 2 Feb 2023 at 21:13, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
>
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> ---
>  target/arm/pauth_helper.c | 48 +++++++++++++++++++++++++++++++--------
>  1 file changed, 39 insertions(+), 9 deletions(-)

If you move the stray bit from patch 1 that should be in this patch
into here, then

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 3/7] target/arm: Implement v8.3 EnhancedPAC

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
---
 target/arm/pauth_helper.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index f525ef7fad..a83956652f 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -347,11 +347,15 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
      */
     test = sextract64(ptr, bot_bit, top_bit - bot_bit);
     if (test != 0 && test != -1) {
-        /*
-         * Note that our top_bit is one greater than the pseudocode's
-         * version, hence "- 2" here.
-         */
-        pac ^= MAKE_64BIT_MASK(top_bit - 2, 1);
+        if (cpu_isar_feature(aa64_pauth_epac, env_archcpu(env))) {
+            pac = 0;
+        } else {
+            /*
+             * Note that our top_bit is one greater than the pseudocode's
+             * version, hence "- 2" here.
+             */
+            pac ^= MAKE_64BIT_MASK(top_bit - 2, 1);
+        }
     }
 
     /*
-- 
2.25.1

Re: [PATCH 3/7] target/arm: Implement v8.3 EnhancedPAC

[Peter Maydell]

On Thu, 2 Feb 2023 at 21:12, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
>
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> ---
>  target/arm/pauth_helper.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
> index f525ef7fad..a83956652f 100644
> --- a/target/arm/pauth_helper.c
> +++ b/target/arm/pauth_helper.c
> @@ -347,11 +347,15 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
>       */
>      test = sextract64(ptr, bot_bit, top_bit - bot_bit);
>      if (test != 0 && test != -1) {
> -        /*
> -         * Note that our top_bit is one greater than the pseudocode's
> -         * version, hence "- 2" here.
> -         */
> -        pac ^= MAKE_64BIT_MASK(top_bit - 2, 1);
> +        if (cpu_isar_feature(aa64_pauth_epac, env_archcpu(env))) {
> +            pac = 0;
> +        } else {
> +            /*
> +             * Note that our top_bit is one greater than the pseudocode's
> +             * version, hence "- 2" here.
> +             */
> +            pac ^= MAKE_64BIT_MASK(top_bit - 2, 1);
> +        }
>      }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 4/7] target/arm: Implement v8.3 Pauth2

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
---
 target/arm/pauth_helper.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index a83956652f..6ebf6df75c 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -349,7 +349,7 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
     if (test != 0 && test != -1) {
         if (cpu_isar_feature(aa64_pauth_epac, env_archcpu(env))) {
             pac = 0;
-        } else {
+        } else if (! cpu_isar_feature(aa64_pauth2, env_archcpu(env))) {
             /*
              * Note that our top_bit is one greater than the pseudocode's
              * version, hence "- 2" here.
@@ -362,6 +362,8 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
      * Preserve the determination between upper and lower at bit 55,
      * and insert pointer authentication code.
      */
+    if (cpu_isar_feature(aa64_pauth2, env_archcpu(env)))
+        pac ^= ptr;
     if (param.tbi) {
         ptr &= ~MAKE_64BIT_MASK(bot_bit, 55 - bot_bit + 1);
         pac &= MAKE_64BIT_MASK(bot_bit, 54 - bot_bit + 1);
@@ -389,23 +391,30 @@ static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
     ARMMMUIdx mmu_idx = arm_stage1_mmu_idx(env);
     ARMVAParameters param = aa64_va_parameters(env, ptr, mmu_idx, data);
     int bot_bit, top_bit;
-    uint64_t pac, orig_ptr, test;
+    uint64_t pac, orig_ptr, test, result;
 
     orig_ptr = pauth_original_ptr(ptr, param);
     pac = pauth_computepac(env, orig_ptr, modifier, *key);
     bot_bit = 64 - param.tsz;
     top_bit = 64 - 8 * param.tbi;
 
-    test = (pac ^ ptr) & ~MAKE_64BIT_MASK(55, 1);
-    if (unlikely(extract64(test, bot_bit, top_bit - bot_bit))) {
-        int error_code = (keynumber << 1) | (keynumber ^ 1);
-        if (param.tbi) {
-            return deposit64(orig_ptr, 53, 2, error_code);
-        } else {
-            return deposit64(orig_ptr, 61, 2, error_code);
+    if (cpu_isar_feature(aa64_pauth2, env_archcpu(env))) {
+        uint64_t xor_mask = MAKE_64BIT_MASK(bot_bit, top_bit - bot_bit + 1) &
+            ~MAKE_64BIT_MASK(55, 1);
+        result = ((ptr ^ pac) & xor_mask) | (ptr & ~xor_mask);
+    } else {
+        test = (pac ^ ptr) & ~MAKE_64BIT_MASK(55, 1);
+        if (unlikely(extract64(test, bot_bit, top_bit - bot_bit))) {
+            int error_code = (keynumber << 1) | (keynumber ^ 1);
+            if (param.tbi) {
+                return deposit64(orig_ptr, 53, 2, error_code);
+            } else {
+                return deposit64(orig_ptr, 61, 2, error_code);
+            }
         }
+        result = orig_ptr;
     }
-    return orig_ptr;
+    return result;
 }
 
 static uint64_t pauth_strip(CPUARMState *env, uint64_t ptr, bool data)
-- 
2.25.1

Re: [PATCH 4/7] target/arm: Implement v8.3 Pauth2

[Peter Maydell]

On Thu, 2 Feb 2023 at 21:13, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
>
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> ---
>  target/arm/pauth_helper.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
> index a83956652f..6ebf6df75c 100644
> --- a/target/arm/pauth_helper.c
> +++ b/target/arm/pauth_helper.c
> @@ -349,7 +349,7 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
>      if (test != 0 && test != -1) {
>          if (cpu_isar_feature(aa64_pauth_epac, env_archcpu(env))) {
>              pac = 0;
> -        } else {
> +        } else if (! cpu_isar_feature(aa64_pauth2, env_archcpu(env))) {

I think we should write this set of conditions as:

     if (cpu_isar_feature(aa64_pauth2, env_archcpu(env))) {
         /* No action required */
     } else if (cpu_isar_feature(aa64_pauth_epac, env_archcpu(env))) {
         pac = 0;
     } else {
         /* Note that etc etc */
         pac ^= MAKE_64BIT_MASK(top_bit - 2, 1);
     }

I know this isn't the way round the pseudocode does it, but if we do
it this way it means we don't need to have the weird special case
where we do an == check instead of >= in the epac isar_feature test
function.

>              /*
>               * Note that our top_bit is one greater than the pseudocode's
>               * version, hence "- 2" here.
> @@ -362,6 +362,8 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
>       * Preserve the determination between upper and lower at bit 55,
>       * and insert pointer authentication code.
>       */
> +    if (cpu_isar_feature(aa64_pauth2, env_archcpu(env)))
> +        pac ^= ptr;

All if statements need braces, even one-line ones.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 5/7] targer/arm: Inform helpers whether a PAC instruction is 'combined'

[Aaron Lindsay]

An instruction is a 'combined' Pointer Authentication instruction if it
does something in addition to PAC - for instance, branching to or
loading an address from the authenticated pointer. Knowing whether a PAC
operation is 'combined' is needed to implement the FPACCOMBINE feature
for ARMv8.3.

Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
---
 target/arm/helper-a64.h    |  4 +++
 target/arm/pauth_helper.c  | 63 ++++++++++++++++++++++++++++++++------
 target/arm/translate-a64.c | 20 ++++++------
 3 files changed, 68 insertions(+), 19 deletions(-)

diff --git a/target/arm/helper-a64.h b/target/arm/helper-a64.h
index 7b706571bb..829aaf4919 100644
--- a/target/arm/helper-a64.h
+++ b/target/arm/helper-a64.h
@@ -98,9 +98,13 @@ DEF_HELPER_FLAGS_3(pacda, TCG_CALL_NO_WG, i64, env, i64, i64)
 DEF_HELPER_FLAGS_3(pacdb, TCG_CALL_NO_WG, i64, env, i64, i64)
 DEF_HELPER_FLAGS_3(pacga, TCG_CALL_NO_WG, i64, env, i64, i64)
 DEF_HELPER_FLAGS_3(autia, TCG_CALL_NO_WG, i64, env, i64, i64)
+DEF_HELPER_FLAGS_3(autia_combined, TCG_CALL_NO_WG, i64, env, i64, i64)
 DEF_HELPER_FLAGS_3(autib, TCG_CALL_NO_WG, i64, env, i64, i64)
+DEF_HELPER_FLAGS_3(autib_combined, TCG_CALL_NO_WG, i64, env, i64, i64)
 DEF_HELPER_FLAGS_3(autda, TCG_CALL_NO_WG, i64, env, i64, i64)
+DEF_HELPER_FLAGS_3(autda_combined, TCG_CALL_NO_WG, i64, env, i64, i64)
 DEF_HELPER_FLAGS_3(autdb, TCG_CALL_NO_WG, i64, env, i64, i64)
+DEF_HELPER_FLAGS_3(autdb_combined, TCG_CALL_NO_WG, i64, env, i64, i64)
 DEF_HELPER_FLAGS_2(xpaci, TCG_CALL_NO_RWG_SE, i64, env, i64)
 DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, i64)
 
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index 6ebf6df75c..66dc90a289 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -386,7 +386,8 @@ static uint64_t pauth_original_ptr(uint64_t ptr, ARMVAParameters param)
 }
 
 static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
-                           ARMPACKey *key, bool data, int keynumber)
+                           ARMPACKey *key, bool data, int keynumber,
+                           bool is_combined)
 {
     ARMMMUIdx mmu_idx = arm_stage1_mmu_idx(env);
     ARMVAParameters param = aa64_va_parameters(env, ptr, mmu_idx, data);
@@ -507,44 +508,88 @@ uint64_t HELPER(pacga)(CPUARMState *env, uint64_t x, uint64_t y)
     return pac & 0xffffffff00000000ull;
 }
 
-uint64_t HELPER(autia)(CPUARMState *env, uint64_t x, uint64_t y)
+static uint64_t pauth_autia(CPUARMState *env, uint64_t x, uint64_t y,
+                            bool is_combined)
 {
     int el = arm_current_el(env);
     if (!pauth_key_enabled(env, el, SCTLR_EnIA)) {
         return x;
     }
     pauth_check_trap(env, el, GETPC());
-    return pauth_auth(env, x, y, &env->keys.apia, false, 0);
+    return pauth_auth(env, x, y, &env->keys.apia, false, 0, is_combined);
 }
 
-uint64_t HELPER(autib)(CPUARMState *env, uint64_t x, uint64_t y)
+uint64_t HELPER(autia)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+    return pauth_autia(env, x, y, false);
+}
+
+uint64_t HELPER(autia_combined)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+    return pauth_autia(env, x, y, true);
+}
+
+static uint64_t pauth_autib(CPUARMState *env, uint64_t x, uint64_t y,
+                            bool is_combined)
 {
     int el = arm_current_el(env);
     if (!pauth_key_enabled(env, el, SCTLR_EnIB)) {
         return x;
     }
     pauth_check_trap(env, el, GETPC());
-    return pauth_auth(env, x, y, &env->keys.apib, false, 1);
+    return pauth_auth(env, x, y, &env->keys.apib, false, 1, is_combined);
 }
 
-uint64_t HELPER(autda)(CPUARMState *env, uint64_t x, uint64_t y)
+uint64_t HELPER(autib)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+    return pauth_autib(env, x, y, false);
+}
+
+uint64_t HELPER(autib_combined)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+    return pauth_autib(env, x, y, true);
+}
+
+static uint64_t pauth_autda(CPUARMState *env, uint64_t x, uint64_t y,
+                            bool is_combined)
 {
     int el = arm_current_el(env);
     if (!pauth_key_enabled(env, el, SCTLR_EnDA)) {
         return x;
     }
     pauth_check_trap(env, el, GETPC());
-    return pauth_auth(env, x, y, &env->keys.apda, true, 0);
+    return pauth_auth(env, x, y, &env->keys.apda, true, 0, is_combined);
 }
 
-uint64_t HELPER(autdb)(CPUARMState *env, uint64_t x, uint64_t y)
+uint64_t HELPER(autda)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+    return pauth_autda(env, x, y, false);
+}
+
+uint64_t HELPER(autda_combined)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+    return pauth_autda(env, x, y, true);
+}
+
+static uint64_t pauth_autdb(CPUARMState *env, uint64_t x, uint64_t y,
+                            bool is_combined)
 {
     int el = arm_current_el(env);
     if (!pauth_key_enabled(env, el, SCTLR_EnDB)) {
         return x;
     }
     pauth_check_trap(env, el, GETPC());
-    return pauth_auth(env, x, y, &env->keys.apdb, true, 1);
+    return pauth_auth(env, x, y, &env->keys.apdb, true, 1, is_combined);
+}
+
+uint64_t HELPER(autdb)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+    return pauth_autdb(env, x, y, false);
+}
+
+uint64_t HELPER(autdb_combined)(CPUARMState *env, uint64_t x, uint64_t y)
+{
+    return pauth_autdb(env, x, y, true);
 }
 
 uint64_t HELPER(xpaci)(CPUARMState *env, uint64_t a)
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 52b1b8a1f0..37cccfda8a 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -2318,9 +2318,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
             if (s->pauth_active) {
                 dst = new_tmp_a64(s);
                 if (op3 == 2) {
-                    gen_helper_autia(dst, cpu_env, cpu_reg(s, rn), modifier);
+                    gen_helper_autia_combined(dst, cpu_env, cpu_reg(s, rn), modifier);
                 } else {
-                    gen_helper_autib(dst, cpu_env, cpu_reg(s, rn), modifier);
+                    gen_helper_autib_combined(dst, cpu_env, cpu_reg(s, rn), modifier);
                 }
             } else {
                 dst = cpu_reg(s, rn);
@@ -2356,9 +2356,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
             dst = new_tmp_a64(s);
             modifier = cpu_reg_sp(s, op4);
             if (op3 == 2) {
-                gen_helper_autia(dst, cpu_env, cpu_reg(s, rn), modifier);
+                gen_helper_autia_combined(dst, cpu_env, cpu_reg(s, rn), modifier);
             } else {
-                gen_helper_autib(dst, cpu_env, cpu_reg(s, rn), modifier);
+                gen_helper_autib_combined(dst, cpu_env, cpu_reg(s, rn), modifier);
             }
         } else {
             dst = cpu_reg(s, rn);
@@ -2404,9 +2404,9 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
             if (s->pauth_active) {
                 modifier = cpu_X[31];
                 if (op3 == 2) {
-                    gen_helper_autia(dst, cpu_env, dst, modifier);
+                    gen_helper_autia_combined(dst, cpu_env, dst, modifier);
                 } else {
-                    gen_helper_autib(dst, cpu_env, dst, modifier);
+                    gen_helper_autib_combined(dst, cpu_env, dst, modifier);
                 }
             }
             break;
@@ -3583,11 +3583,11 @@ static void disas_ldst_pac(DisasContext *s, uint32_t insn,
 
     if (s->pauth_active) {
         if (use_key_a) {
-            gen_helper_autda(dirty_addr, cpu_env, dirty_addr,
-                             new_tmp_a64_zero(s));
+            gen_helper_autda_combined(dirty_addr, cpu_env, dirty_addr,
+                                      new_tmp_a64_zero(s));
         } else {
-            gen_helper_autdb(dirty_addr, cpu_env, dirty_addr,
-                             new_tmp_a64_zero(s));
+            gen_helper_autdb_combined(dirty_addr, cpu_env, dirty_addr,
+                                      new_tmp_a64_zero(s));
         }
     }
 
-- 
2.25.1

Re: [PATCH 5/7] targer/arm: Inform helpers whether a PAC instruction is 'combined'

[Peter Maydell]

On Thu, 2 Feb 2023 at 21:12, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
>
> An instruction is a 'combined' Pointer Authentication instruction if it
> does something in addition to PAC - for instance, branching to or
> loading an address from the authenticated pointer. Knowing whether a PAC
> operation is 'combined' is needed to implement the FPACCOMBINE feature
> for ARMv8.3.
>
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> ---


> -uint64_t HELPER(autia)(CPUARMState *env, uint64_t x, uint64_t y)
> +static uint64_t pauth_autia(CPUARMState *env, uint64_t x, uint64_t y,
> +                            bool is_combined)
>  {
>      int el = arm_current_el(env);
>      if (!pauth_key_enabled(env, el, SCTLR_EnIA)) {
>          return x;
>      }
>      pauth_check_trap(env, el, GETPC());

You can't move a GETPC() into a sub-function like this : it has
to remain in the top level helper function. If you need the
value in a sub-function, you need to pass it down. This is why
pauth_check_trap() has its 'ra' argument. (See patch 6 review
comment for more explanation.)

> -    return pauth_auth(env, x, y, &env->keys.apia, false, 0);
> +    return pauth_auth(env, x, y, &env->keys.apia, false, 0, is_combined);
>  }

thanks
-- PMM

[PATCH 6/7] target/arm: Implement v8.3 FPAC and FPACCOMBINE

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
---
 target/arm/pauth_helper.c | 26 ++++++++++++++++++++++++++
 target/arm/syndrome.h     |  6 ++++++
 2 files changed, 32 insertions(+)

diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index 66dc90a289..3a2772de0e 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -385,6 +385,21 @@ static uint64_t pauth_original_ptr(uint64_t ptr, ARMVAParameters param)
     return deposit64(ptr, bot_pac_bit, top_pac_bit - bot_pac_bit, extfield);
 }
 
+static G_NORETURN
+void pauth_fail_exception(CPUARMState *env, int error_code)
+{
+    int target_el = arm_current_el(env);
+    if (target_el == 0) {
+        uint64_t hcr = arm_hcr_el2_eff(env);
+        if (arm_is_el2_enabled(env) && (hcr & HCR_TGE))
+            target_el = 2;
+        else
+            target_el = 1;
+    }
+
+    raise_exception_ra(env, EXCP_UDEF, syn_pacfail(error_code), target_el, GETPC());
+}
+
 static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
                            ARMPACKey *key, bool data, int keynumber,
                            bool is_combined)
@@ -403,6 +418,17 @@ static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
         uint64_t xor_mask = MAKE_64BIT_MASK(bot_bit, top_bit - bot_bit + 1) &
             ~MAKE_64BIT_MASK(55, 1);
         result = ((ptr ^ pac) & xor_mask) | (ptr & ~xor_mask);
+        if (cpu_isar_feature(aa64_fpac_combine, env_archcpu(env)) ||
+                (cpu_isar_feature(aa64_fpac, env_archcpu(env)) &&
+                 !is_combined)) {
+            int fpac_top = param.tbi ? 55 : 64;
+            uint64_t fpac_mask = MAKE_64BIT_MASK(bot_bit, fpac_top - bot_bit);
+            test = (result ^ sextract64(result, 55, 1)) & fpac_mask;
+            if (unlikely(test)) {
+                int error_code = ((data ? 1 : 0) << 1) | (keynumber);
+                pauth_fail_exception(env, error_code);
+            }
+        }
     } else {
         test = (pac ^ ptr) & ~MAKE_64BIT_MASK(55, 1);
         if (unlikely(extract64(test, bot_bit, top_bit - bot_bit))) {
diff --git a/target/arm/syndrome.h b/target/arm/syndrome.h
index 73df5e3793..885a85735c 100644
--- a/target/arm/syndrome.h
+++ b/target/arm/syndrome.h
@@ -48,6 +48,7 @@ enum arm_exception_class {
     EC_AA64_SMC               = 0x17,
     EC_SYSTEMREGISTERTRAP     = 0x18,
     EC_SVEACCESSTRAP          = 0x19,
+    EC_PACFAIL                = 0x1c,
     EC_SMETRAP                = 0x1d,
     EC_INSNABORT              = 0x20,
     EC_INSNABORT_SAME_EL      = 0x21,
@@ -221,6 +222,11 @@ static inline uint32_t syn_smetrap(SMEExceptionType etype, bool is_16bit)
         | (is_16bit ? 0 : ARM_EL_IL) | etype;
 }
 
+static inline uint32_t syn_pacfail(int error_code)
+{
+    return (EC_PACFAIL << ARM_EL_EC_SHIFT) | error_code;
+}
+
 static inline uint32_t syn_pactrap(void)
 {
     return EC_PACTRAP << ARM_EL_EC_SHIFT;
-- 
2.25.1

Re: [PATCH 6/7] target/arm: Implement v8.3 FPAC and FPACCOMBINE

[Peter Maydell]

On Thu, 2 Feb 2023 at 21:13, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
>
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> ---
>  target/arm/pauth_helper.c | 26 ++++++++++++++++++++++++++
>  target/arm/syndrome.h     |  6 ++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
> index 66dc90a289..3a2772de0e 100644
> --- a/target/arm/pauth_helper.c
> +++ b/target/arm/pauth_helper.c
> @@ -385,6 +385,21 @@ static uint64_t pauth_original_ptr(uint64_t ptr, ARMVAParameters param)
>      return deposit64(ptr, bot_pac_bit, top_pac_bit - bot_pac_bit, extfield);
>  }
>
> +static G_NORETURN
> +void pauth_fail_exception(CPUARMState *env, int error_code)
> +{
> +    int target_el = arm_current_el(env);
> +    if (target_el == 0) {
> +        uint64_t hcr = arm_hcr_el2_eff(env);
> +        if (arm_is_el2_enabled(env) && (hcr & HCR_TGE))
> +            target_el = 2;
> +        else
> +            target_el = 1;
> +    }
> +
> +    raise_exception_ra(env, EXCP_UDEF, syn_pacfail(error_code), target_el, GETPC());

This won't work -- you must call GETPC() from the top-level helper
function directly called from JITted code, so that it can get the
PC of the callsite in the JITted code. Otherwise you'll get a PC
somewhere inside QEMU's C code, which won't do the right thing.
This is why pauth_check_trap() takes an 'ra' argument (for
'return address') and all the top level helper functions call
GETPC() to get the value to pass.

> +}
> +
>  static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
>                             ARMPACKey *key, bool data, int keynumber,
>                             bool is_combined)
> @@ -403,6 +418,17 @@ static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
>          uint64_t xor_mask = MAKE_64BIT_MASK(bot_bit, top_bit - bot_bit + 1) &
>              ~MAKE_64BIT_MASK(55, 1);
>          result = ((ptr ^ pac) & xor_mask) | (ptr & ~xor_mask);
> +        if (cpu_isar_feature(aa64_fpac_combine, env_archcpu(env)) ||
> +                (cpu_isar_feature(aa64_fpac, env_archcpu(env)) &&
> +                 !is_combined)) {
> +            int fpac_top = param.tbi ? 55 : 64;
> +            uint64_t fpac_mask = MAKE_64BIT_MASK(bot_bit, fpac_top - bot_bit);
> +            test = (result ^ sextract64(result, 55, 1)) & fpac_mask;
> +            if (unlikely(test)) {
> +                int error_code = ((data ? 1 : 0) << 1) | (keynumber);
> +                pauth_fail_exception(env, error_code);
> +            }
> +        }
>      } else {
>          test = (pac ^ ptr) & ~MAKE_64BIT_MASK(55, 1);
>          if (unlikely(extract64(test, bot_bit, top_bit - bot_bit))) {
> diff --git a/target/arm/syndrome.h b/target/arm/syndrome.h
> index 73df5e3793..885a85735c 100644
> --- a/target/arm/syndrome.h
> +++ b/target/arm/syndrome.h
> @@ -48,6 +48,7 @@ enum arm_exception_class {
>      EC_AA64_SMC               = 0x17,
>      EC_SYSTEMREGISTERTRAP     = 0x18,
>      EC_SVEACCESSTRAP          = 0x19,
> +    EC_PACFAIL                = 0x1c,
>      EC_SMETRAP                = 0x1d,
>      EC_INSNABORT              = 0x20,
>      EC_INSNABORT_SAME_EL      = 0x21,
> @@ -221,6 +222,11 @@ static inline uint32_t syn_smetrap(SMEExceptionType etype, bool is_16bit)
>          | (is_16bit ? 0 : ARM_EL_IL) | etype;
>  }
>
> +static inline uint32_t syn_pacfail(int error_code)
> +{
> +    return (EC_PACFAIL << ARM_EL_EC_SHIFT) | error_code;

You need ARM_EL_IL here too, I think.

I would suggest that you make the syn_pacfail() function take
two arguments (bool data and int keynumber), and put them in to
bits 0 and 1 in this function. That avoids the need to
construct an error code at the callsite.

> +}
> +
>  static inline uint32_t syn_pactrap(void)
>  {
>      return EC_PACTRAP << ARM_EL_EC_SHIFT;
> --
> 2.25.1

thanks
-- PMM

[PATCH 7/7] target/arm: Add CPU properties for most v8.3 PAC features

[Aaron Lindsay]

Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
---
 target/arm/cpu.h   |  5 +++
 target/arm/cpu64.c | 81 ++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 72 insertions(+), 14 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9be59163ff..a9420bae67 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1039,6 +1039,11 @@ struct ArchCPU {
      */
     bool prop_pauth;
     bool prop_pauth_impdef;
+    bool prop_pauth_qarma3;
+    bool prop_pauth_epac;
+    bool prop_pauth2; // also known as EnhancedPAC2/EPAC2
+    bool prop_pauth_fpac;
+    bool prop_pauth_fpac_combine;
     bool prop_lpa2;
 
     /* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 0e021960fb..315acabbe2 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -590,8 +590,7 @@ static void aarch64_add_sme_properties(Object *obj)
 
 void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
 {
-    int arch_val = 0, impdef_val = 0;
-    uint64_t t;
+    int address_auth = 0, generic_auth = 0;
 
     /* Exit early if PAuth is enabled, and fall through to disable it */
     if ((kvm_enabled() || hvf_enabled()) && cpu->prop_pauth) {
@@ -603,30 +602,79 @@ void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
         return;
     }
 
-    /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */
+    if (cpu->prop_pauth_epac &&
+            (cpu->prop_pauth2 ||
+             cpu->prop_pauth_fpac ||
+             cpu->prop_pauth_fpac_combine)) {
+        error_setg(errp, "'pauth-epac' feature not compatible with any of "
+                   "'pauth-2', 'pauth-fpac', or 'pauth-fpac-combine'");
+        return;
+    }
+
+    /* Determine the PAC features independently of the algorithm */
+    if (cpu->prop_pauth_fpac_combine) {
+        address_auth = 0b0101;
+    } else if (cpu->prop_pauth_fpac) {
+        address_auth = 0b0100;
+    } else if (cpu->prop_pauth2) {
+        address_auth = 0b0011;
+    } else if (cpu->prop_pauth_epac) {
+        address_auth = 0b0010;
+    }
+
+    /* Write the features into the correct field for the algorithm in use */
     if (cpu->prop_pauth) {
+        uint64_t t;
+
+        if (cpu->prop_pauth_impdef && cpu->prop_pauth_qarma3) {
+            error_setg(errp, "Cannot set both qarma3 ('pauth-qarma3') and "
+                    "impdef ('pauth-impdef') pointer authentication ciphers");
+            return;
+        }
+
+        if (address_auth == 0)
+            address_auth = 0b0001;
+        generic_auth = 1;
+
         if (cpu->prop_pauth_impdef) {
-            impdef_val = 1;
+            t = cpu->isar.id_aa64isar1;
+            t = FIELD_DP64(t, ID_AA64ISAR1, API, address_auth);
+            t = FIELD_DP64(t, ID_AA64ISAR1, GPI, generic_auth);
+            cpu->isar.id_aa64isar1 = t;
+        } else if (cpu->prop_pauth_qarma3) {
+            t = cpu->isar.id_aa64isar2;
+            t = FIELD_DP64(t, ID_AA64ISAR2, APA3, address_auth);
+            t = FIELD_DP64(t, ID_AA64ISAR2, GPA3, generic_auth);
+            cpu->isar.id_aa64isar2 = t;
         } else {
-            arch_val = 1;
+            t = cpu->isar.id_aa64isar1;
+            t = FIELD_DP64(t, ID_AA64ISAR1, APA, address_auth);
+            t = FIELD_DP64(t, ID_AA64ISAR1, GPA, generic_auth);
+            cpu->isar.id_aa64isar1 = t;
         }
-    } else if (cpu->prop_pauth_impdef) {
-        error_setg(errp, "cannot enable pauth-impdef without pauth");
+    } else if (cpu->prop_pauth_impdef || cpu->prop_pauth_qarma3) {
+        error_setg(errp, "cannot enable pauth-impdef or pauth-qarma3 without pauth");
+        error_append_hint(errp, "Add pauth=on to the CPU property list.\n");
+    } else if (address_auth != 0) {
+        error_setg(errp, "cannot enable any pauth* features without pauth");
         error_append_hint(errp, "Add pauth=on to the CPU property list.\n");
     }
-
-    t = cpu->isar.id_aa64isar1;
-    t = FIELD_DP64(t, ID_AA64ISAR1, APA, arch_val);
-    t = FIELD_DP64(t, ID_AA64ISAR1, GPA, arch_val);
-    t = FIELD_DP64(t, ID_AA64ISAR1, API, impdef_val);
-    t = FIELD_DP64(t, ID_AA64ISAR1, GPI, impdef_val);
-    cpu->isar.id_aa64isar1 = t;
 }
 
 static Property arm_cpu_pauth_property =
     DEFINE_PROP_BOOL("pauth", ARMCPU, prop_pauth, true);
 static Property arm_cpu_pauth_impdef_property =
     DEFINE_PROP_BOOL("pauth-impdef", ARMCPU, prop_pauth_impdef, false);
+static Property arm_cpu_pauth_qarma3_property =
+    DEFINE_PROP_BOOL("pauth-qarma3", ARMCPU, prop_pauth_qarma3, false);
+static Property arm_cpu_pauth_epac_property =
+    DEFINE_PROP_BOOL("pauth-epac", ARMCPU, prop_pauth_epac, false);
+static Property arm_cpu_pauth2_property =
+    DEFINE_PROP_BOOL("pauth2", ARMCPU, prop_pauth2, false);
+static Property arm_cpu_pauth_fpac_property =
+    DEFINE_PROP_BOOL("pauth-fpac", ARMCPU, prop_pauth_fpac, false);
+static Property arm_cpu_pauth_fpac_combine_property =
+    DEFINE_PROP_BOOL("pauth-fpac-combine", ARMCPU, prop_pauth_fpac_combine, false);
 
 static void aarch64_add_pauth_properties(Object *obj)
 {
@@ -646,6 +694,11 @@ static void aarch64_add_pauth_properties(Object *obj)
         cpu->prop_pauth = cpu_isar_feature(aa64_pauth, cpu);
     } else {
         qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_qarma3_property);
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_epac_property);
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth2_property);
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_fpac_property);
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_fpac_combine_property);
     }
 }
 
-- 
2.25.1

Re: [PATCH 7/7] target/arm: Add CPU properties for most v8.3 PAC features

[Peter Maydell]

On Thu, 2 Feb 2023 at 21:12, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
>
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> ---
>  target/arm/cpu.h   |  5 +++
>  target/arm/cpu64.c | 81 ++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 72 insertions(+), 14 deletions(-)

Do we really need all these properties ? Generally we don't
add CPU properties unless there's a good reason for the
user (or the board/SoC code) to want to flip them. The
more usual case is that we simply enable them on the 'max'
CPU by setting the ID register fields appropriately.

Somewhere in this series you need to add documentation of
the features being implemented to docs/system/arm/emulation.rst
(just a one-liner per FEAT_whatever).

thanks
-- PMM

Re: [PATCH 7/7] target/arm: Add CPU properties for most v8.3 PAC features

[Aaron Lindsay]

On Feb 13 17:11, Peter Maydell wrote:
> On Thu, 2 Feb 2023 at 21:12, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
> >
> > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> > ---
> >  target/arm/cpu.h   |  5 +++
> >  target/arm/cpu64.c | 81 ++++++++++++++++++++++++++++++++++++++--------
> >  2 files changed, 72 insertions(+), 14 deletions(-)
> 
> Do we really need all these properties ? Generally we don't
> add CPU properties unless there's a good reason for the
> user (or the board/SoC code) to want to flip them. The
> more usual case is that we simply enable them on the 'max'
> CPU by setting the ID register fields appropriately.

Honestly, I wasn't sure where to draw the line... so I didn't. Though I
won't claim to have perfect knowledge of the evolution of this feature,
it felt like there were 4 distinct levels that I could imagine might be
wanted - I've starred those 4 below:

* 1) no PAC           (APA/API=0b0000)
* 2) PAC without EPAC/Pauth2, QEMU's highest PAC implementation previous
     to this patchset (APA/API=0b0001)
* 3) EPAC             (APA/API=0b0010)
  4) Pauth2           (APA/API=0b0011) 
  5) FPAC             (APA/API=0b0100) 
* 6) FPACCombined     (APA/API=0b0101)

And I am not sure how likely 4) and 5) are to be implemented, but after
I was already up to 4, adding the last two more didn't feel like much
more!

I half-considered trying to make `pauth` a single option which took a
string instead of a handful of separate boolean arguments. The possible
options might be `pauth=off`, `pauth=no-epac` (no EPAC), `pauth=epac`,
`pauth=pauth2`, `pauth=fpac-combine`.

All this to say: I'm more than happy to take guidance here!

> Somewhere in this series you need to add documentation of
> the features being implemented to docs/system/arm/emulation.rst
> (just a one-liner per FEAT_whatever).

Will do in my next patchset based on what we decide upon above.

Thanks!

-Aaron