[PATCH v2 0/3] target/arm: Implement an IMPDEF pauth algorithm

[PATCH v2 0/3] target/arm: Implement an IMPDEF pauth algorithm

[Richard Henderson]

The architected pauth algorithm is quite slow without
hardware support, and boot times for kernels that enable
use of the feature have been significantly impacted.

Version 1 blurb at
  https://lists.nongnu.org/archive/html/qemu-devel/2020-08/msg02172.html
which contains larger study of the tradeoffs.

Version 2 changes:
  * Use boolean properties, for qmp_query_cpu_model_expansion (drjones).
  * Move XXH64 implementation to xxhash.h (ajb).
  * Include a small cleanup to parsing the "sve" property
    that I noticed along the way.


r~


Richard Henderson (3):
  target/arm: Add cpu properties to control pauth
  target/arm: Implement an IMPDEF pauth algorithm
  target/arm: Use object_property_add_bool for "sve" property

 include/qemu/xxhash.h     | 82 +++++++++++++++++++++++++++++++++++++++
 target/arm/cpu.h          | 25 ++++++++++--
 target/arm/cpu.c          | 13 +++++++
 target/arm/cpu64.c        | 64 +++++++++++++++++++++---------
 target/arm/monitor.c      |  1 +
 target/arm/pauth_helper.c | 41 +++++++++++++++++---
 6 files changed, 199 insertions(+), 27 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] target/arm: Add cpu properties to control pauth

[Richard Henderson]

The crypto overhead of emulating pauth can be significant for
some workloads.  Add two boolean properties that allows the
feature to be turned off, on with the architected algorithm,
or on with an implementation defined algorithm.

We need two intermediate booleans to control the state while
parsing properties lest we clobber ID_AA64ISAR1 into an invalid
intermediate state.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
v2: Use boolean properties instead of an enum (drjones).
---
 target/arm/cpu.h     | 10 ++++++++++
 target/arm/cpu.c     | 13 +++++++++++++
 target/arm/cpu64.c   | 40 ++++++++++++++++++++++++++++++++++++----
 target/arm/monitor.c |  1 +
 4 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9e8ed423ea..44901923c8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -196,9 +196,11 @@ typedef struct {
 #ifdef TARGET_AARCH64
 # define ARM_MAX_VQ    16
 void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
+void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
 #else
 # define ARM_MAX_VQ    1
 static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
+static inline void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) { }
 #endif
 
 typedef struct ARMVectorReg {
@@ -938,6 +940,14 @@ struct ARMCPU {
     uint64_t reset_cbar;
     uint32_t reset_auxcr;
     bool reset_hivecs;
+
+    /*
+     * Intermediate values used during property parsing.
+     * Once finalized, the values should be read from ID_AA64ISAR1.
+     */
+    bool prop_pauth;
+    bool prop_pauth_impdef;
+
     /* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */
     uint32_t dcz_blocksize;
     uint64_t rvbar;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 111579554f..c719562d3d 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1307,6 +1307,19 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
             error_propagate(errp, local_err);
             return;
         }
+
+        /*
+         * KVM does not support modifications to this feature.
+         * We have not registered the cpu properties when KVM
+         * is in use, so the user will not be able to set them.
+         */
+        if (!kvm_enabled()) {
+            arm_cpu_pauth_finalize(cpu, &local_err);
+            if (local_err != NULL) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
     }
 }
 
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index dd696183df..0227862d39 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -28,6 +28,8 @@
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
 #include "qapi/visitor.h"
+#include "hw/qdev-properties.h"
+
 
 #ifndef CONFIG_USER_ONLY
 static uint64_t a57_a53_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
@@ -572,6 +574,36 @@ void aarch64_add_sve_properties(Object *obj)
     }
 }
 
+void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
+{
+    int arch_val = 0, impdef_val = 0;
+    uint64_t t;
+
+    /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */
+    if (cpu->prop_pauth) {
+        if (cpu->prop_pauth_impdef) {
+            impdef_val = 1;
+        } else {
+            arch_val = 1;
+        }
+    } else if (cpu->prop_pauth_impdef) {
+        error_setg(errp, "cannot enable pauth-impdef 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);
+
 /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
  * otherwise, a CPU with as many features enabled as our emulation supports.
  * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
@@ -627,10 +659,6 @@ static void aarch64_max_initfn(Object *obj)
         t = FIELD_DP64(t, ID_AA64ISAR1, DPB, 2);
         t = FIELD_DP64(t, ID_AA64ISAR1, JSCVT, 1);
         t = FIELD_DP64(t, ID_AA64ISAR1, FCMA, 1);
-        t = FIELD_DP64(t, ID_AA64ISAR1, APA, 1); /* PAuth, architected only */
-        t = FIELD_DP64(t, ID_AA64ISAR1, API, 0);
-        t = FIELD_DP64(t, ID_AA64ISAR1, GPA, 1);
-        t = FIELD_DP64(t, ID_AA64ISAR1, GPI, 0);
         t = FIELD_DP64(t, ID_AA64ISAR1, SB, 1);
         t = FIELD_DP64(t, ID_AA64ISAR1, SPECRES, 1);
         t = FIELD_DP64(t, ID_AA64ISAR1, FRINTTS, 1);
@@ -718,6 +746,10 @@ static void aarch64_max_initfn(Object *obj)
         cpu->ctr = 0x80038003; /* 32 byte I and D cacheline size, VIPT icache */
         cpu->dcz_blocksize = 7; /*  512 bytes */
 #endif
+
+        /* Default to PAUTH on, with the architected algorithm. */
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
+        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
     }
 
     aarch64_add_sve_properties(obj);
diff --git a/target/arm/monitor.c b/target/arm/monitor.c
index ba6e01abd0..2c7be32b33 100644
--- a/target/arm/monitor.c
+++ b/target/arm/monitor.c
@@ -104,6 +104,7 @@ static const char *cpu_model_advertised_features[] = {
     "sve640", "sve768", "sve896", "sve1024", "sve1152", "sve1280",
     "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
     "kvm-no-adjvtime",
+    "pauth", "pauth-impdef",
     NULL
 };
 
-- 
2.25.1

Re: [PATCH v2 1/3] target/arm: Add cpu properties to control pauth

[Andrew Jones]

On Thu, Aug 13, 2020 at 01:02:41PM -0700, Richard Henderson wrote:
> The crypto overhead of emulating pauth can be significant for
> some workloads.  Add two boolean properties that allows the
> feature to be turned off, on with the architected algorithm,
> or on with an implementation defined algorithm.
> 
> We need two intermediate booleans to control the state while
> parsing properties lest we clobber ID_AA64ISAR1 into an invalid
> intermediate state.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> v2: Use boolean properties instead of an enum (drjones).
> ---
>  target/arm/cpu.h     | 10 ++++++++++
>  target/arm/cpu.c     | 13 +++++++++++++
>  target/arm/cpu64.c   | 40 ++++++++++++++++++++++++++++++++++++----
>  target/arm/monitor.c |  1 +
>  4 files changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9e8ed423ea..44901923c8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -196,9 +196,11 @@ typedef struct {
>  #ifdef TARGET_AARCH64
>  # define ARM_MAX_VQ    16
>  void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
> +void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
>  #else
>  # define ARM_MAX_VQ    1
>  static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
> +static inline void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) { }
>  #endif
>  
>  typedef struct ARMVectorReg {
> @@ -938,6 +940,14 @@ struct ARMCPU {
>      uint64_t reset_cbar;
>      uint32_t reset_auxcr;
>      bool reset_hivecs;
> +
> +    /*
> +     * Intermediate values used during property parsing.
> +     * Once finalized, the values should be read from ID_AA64ISAR1.
> +     */
> +    bool prop_pauth;
> +    bool prop_pauth_impdef;
> +
>      /* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */
>      uint32_t dcz_blocksize;
>      uint64_t rvbar;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 111579554f..c719562d3d 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1307,6 +1307,19 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
>              error_propagate(errp, local_err);
>              return;
>          }
> +
> +        /*
> +         * KVM does not support modifications to this feature.
> +         * We have not registered the cpu properties when KVM
> +         * is in use, so the user will not be able to set them.
> +         */
> +        if (!kvm_enabled()) {
> +            arm_cpu_pauth_finalize(cpu, &local_err);
> +            if (local_err != NULL) {
> +                error_propagate(errp, local_err);
> +                return;
> +            }
> +        }
>      }
>  }
>  
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index dd696183df..0227862d39 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -28,6 +28,8 @@
>  #include "sysemu/kvm.h"
>  #include "kvm_arm.h"
>  #include "qapi/visitor.h"
> +#include "hw/qdev-properties.h"
> +
>  
>  #ifndef CONFIG_USER_ONLY
>  static uint64_t a57_a53_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> @@ -572,6 +574,36 @@ void aarch64_add_sve_properties(Object *obj)
>      }
>  }
>  
> +void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
> +{
> +    int arch_val = 0, impdef_val = 0;
> +    uint64_t t;
> +
> +    /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */
> +    if (cpu->prop_pauth) {
> +        if (cpu->prop_pauth_impdef) {
> +            impdef_val = 1;
> +        } else {
> +            arch_val = 1;
> +        }
> +    } else if (cpu->prop_pauth_impdef) {
> +        error_setg(errp, "cannot enable pauth-impdef 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);
> +
>  /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
>   * otherwise, a CPU with as many features enabled as our emulation supports.
>   * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
> @@ -627,10 +659,6 @@ static void aarch64_max_initfn(Object *obj)
>          t = FIELD_DP64(t, ID_AA64ISAR1, DPB, 2);
>          t = FIELD_DP64(t, ID_AA64ISAR1, JSCVT, 1);
>          t = FIELD_DP64(t, ID_AA64ISAR1, FCMA, 1);
> -        t = FIELD_DP64(t, ID_AA64ISAR1, APA, 1); /* PAuth, architected only */
> -        t = FIELD_DP64(t, ID_AA64ISAR1, API, 0);
> -        t = FIELD_DP64(t, ID_AA64ISAR1, GPA, 1);
> -        t = FIELD_DP64(t, ID_AA64ISAR1, GPI, 0);
>          t = FIELD_DP64(t, ID_AA64ISAR1, SB, 1);
>          t = FIELD_DP64(t, ID_AA64ISAR1, SPECRES, 1);
>          t = FIELD_DP64(t, ID_AA64ISAR1, FRINTTS, 1);
> @@ -718,6 +746,10 @@ static void aarch64_max_initfn(Object *obj)
>          cpu->ctr = 0x80038003; /* 32 byte I and D cacheline size, VIPT icache */
>          cpu->dcz_blocksize = 7; /*  512 bytes */
>  #endif
> +
> +        /* Default to PAUTH on, with the architected algorithm. */
> +        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
> +        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);

Many of our CPU properties have descriptions added with
object_property_set_description(), maybe we should add
descriptions for these as well. And, maybe I should look
into generating descriptions for each of the sve* properties
too.

>      }
>  
>      aarch64_add_sve_properties(obj);
> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> index ba6e01abd0..2c7be32b33 100644
> --- a/target/arm/monitor.c
> +++ b/target/arm/monitor.c
> @@ -104,6 +104,7 @@ static const char *cpu_model_advertised_features[] = {
>      "sve640", "sve768", "sve896", "sve1024", "sve1152", "sve1280",
>      "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
>      "kvm-no-adjvtime",
> +    "pauth", "pauth-impdef",
>      NULL
>  };
>  
> -- 
> 2.25.1
> 
> 

With the qtest change below I tested the property setting. Maybe we should
add that diff to this patch.

In any case,

Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew


diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index f7e062c1891e..bfe08b9b84f0 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -427,6 +427,18 @@ static void sve_tests_sve_off_kvm(const void *data)
     qtest_quit(qts);
 }
 
+static void pauth_tests_default(QTestState *qts, const char *cpu_type)
+{
+    assert_has_feature_enabled(qts, "max", "pauth");
+    assert_has_feature_disabled(qts, "max", "pauth-impdef");
+    assert_set_feature(qts, "max", "pauth", false);
+    assert_set_feature(qts, "max", "pauth", true);
+    assert_set_feature(qts, "max", "pauth-impdef", true);
+    assert_set_feature(qts, "max", "pauth-impdef", false);
+    assert_error(qts, "max", "cannot enable pauth-impdef without pauth",
+                 "{ 'pauth': false, 'pauth-impdef': true }");
+}
+
 static void test_query_cpu_model_expansion(const void *data)
 {
     QTestState *qts;
@@ -461,6 +473,7 @@ static void test_query_cpu_model_expansion(const void *data)
         assert_has_feature_enabled(qts, "cortex-a57", "aarch64");
 
         sve_tests_default(qts, "max");
+        pauth_tests_default(qts, "max");
 
         /* Test that features that depend on KVM generate errors without. */
         assert_error(qts, "max",

Re: [PATCH v2 1/3] target/arm: Add cpu properties to control pauth

[Andrew Jones]

On Fri, Aug 14, 2020 at 11:17:30AM +0200, Andrew Jones wrote:
> On Thu, Aug 13, 2020 at 01:02:41PM -0700, Richard Henderson wrote:
> > The crypto overhead of emulating pauth can be significant for
> > some workloads.  Add two boolean properties that allows the
> > feature to be turned off, on with the architected algorithm,
> > or on with an implementation defined algorithm.
> > 
> > We need two intermediate booleans to control the state while
> > parsing properties lest we clobber ID_AA64ISAR1 into an invalid
> > intermediate state.
> > 
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> > v2: Use boolean properties instead of an enum (drjones).
> > ---
> >  target/arm/cpu.h     | 10 ++++++++++
> >  target/arm/cpu.c     | 13 +++++++++++++
> >  target/arm/cpu64.c   | 40 ++++++++++++++++++++++++++++++++++++----
> >  target/arm/monitor.c |  1 +
> >  4 files changed, 60 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 9e8ed423ea..44901923c8 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -196,9 +196,11 @@ typedef struct {
> >  #ifdef TARGET_AARCH64
> >  # define ARM_MAX_VQ    16
> >  void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
> > +void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
> >  #else
> >  # define ARM_MAX_VQ    1
> >  static inline void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp) { }
> > +static inline void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp) { }
> >  #endif
> >  
> >  typedef struct ARMVectorReg {
> > @@ -938,6 +940,14 @@ struct ARMCPU {
> >      uint64_t reset_cbar;
> >      uint32_t reset_auxcr;
> >      bool reset_hivecs;
> > +
> > +    /*
> > +     * Intermediate values used during property parsing.
> > +     * Once finalized, the values should be read from ID_AA64ISAR1.
> > +     */
> > +    bool prop_pauth;
> > +    bool prop_pauth_impdef;
> > +
> >      /* DCZ blocksize, in log_2(words), ie low 4 bits of DCZID_EL0 */
> >      uint32_t dcz_blocksize;
> >      uint64_t rvbar;
> > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > index 111579554f..c719562d3d 100644
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -1307,6 +1307,19 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
> >              error_propagate(errp, local_err);
> >              return;
> >          }
> > +
> > +        /*
> > +         * KVM does not support modifications to this feature.
> > +         * We have not registered the cpu properties when KVM
> > +         * is in use, so the user will not be able to set them.
> > +         */
> > +        if (!kvm_enabled()) {
> > +            arm_cpu_pauth_finalize(cpu, &local_err);
> > +            if (local_err != NULL) {
> > +                error_propagate(errp, local_err);
> > +                return;
> > +            }
> > +        }
> >      }
> >  }
> >  
> > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> > index dd696183df..0227862d39 100644
> > --- a/target/arm/cpu64.c
> > +++ b/target/arm/cpu64.c
> > @@ -28,6 +28,8 @@
> >  #include "sysemu/kvm.h"
> >  #include "kvm_arm.h"
> >  #include "qapi/visitor.h"
> > +#include "hw/qdev-properties.h"
> > +
> >  
> >  #ifndef CONFIG_USER_ONLY
> >  static uint64_t a57_a53_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> > @@ -572,6 +574,36 @@ void aarch64_add_sve_properties(Object *obj)
> >      }
> >  }
> >  
> > +void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp)
> > +{
> > +    int arch_val = 0, impdef_val = 0;
> > +    uint64_t t;
> > +
> > +    /* TODO: Handle HaveEnhancedPAC, HaveEnhancedPAC2, HaveFPAC. */
> > +    if (cpu->prop_pauth) {
> > +        if (cpu->prop_pauth_impdef) {
> > +            impdef_val = 1;
> > +        } else {
> > +            arch_val = 1;
> > +        }
> > +    } else if (cpu->prop_pauth_impdef) {
> > +        error_setg(errp, "cannot enable pauth-impdef 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);
> > +
> >  /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
> >   * otherwise, a CPU with as many features enabled as our emulation supports.
> >   * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
> > @@ -627,10 +659,6 @@ static void aarch64_max_initfn(Object *obj)
> >          t = FIELD_DP64(t, ID_AA64ISAR1, DPB, 2);
> >          t = FIELD_DP64(t, ID_AA64ISAR1, JSCVT, 1);
> >          t = FIELD_DP64(t, ID_AA64ISAR1, FCMA, 1);
> > -        t = FIELD_DP64(t, ID_AA64ISAR1, APA, 1); /* PAuth, architected only */
> > -        t = FIELD_DP64(t, ID_AA64ISAR1, API, 0);
> > -        t = FIELD_DP64(t, ID_AA64ISAR1, GPA, 1);
> > -        t = FIELD_DP64(t, ID_AA64ISAR1, GPI, 0);
> >          t = FIELD_DP64(t, ID_AA64ISAR1, SB, 1);
> >          t = FIELD_DP64(t, ID_AA64ISAR1, SPECRES, 1);
> >          t = FIELD_DP64(t, ID_AA64ISAR1, FRINTTS, 1);
> > @@ -718,6 +746,10 @@ static void aarch64_max_initfn(Object *obj)
> >          cpu->ctr = 0x80038003; /* 32 byte I and D cacheline size, VIPT icache */
> >          cpu->dcz_blocksize = 7; /*  512 bytes */
> >  #endif
> > +
> > +        /* Default to PAUTH on, with the architected algorithm. */
> > +        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_property);
> > +        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
> 
> Many of our CPU properties have descriptions added with
> object_property_set_description(), maybe we should add
> descriptions for these as well. And, maybe I should look
> into generating descriptions for each of the sve* properties
> too.
> 
> >      }
> >  
> >      aarch64_add_sve_properties(obj);
> > diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> > index ba6e01abd0..2c7be32b33 100644
> > --- a/target/arm/monitor.c
> > +++ b/target/arm/monitor.c
> > @@ -104,6 +104,7 @@ static const char *cpu_model_advertised_features[] = {
> >      "sve640", "sve768", "sve896", "sve1024", "sve1152", "sve1280",
> >      "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
> >      "kvm-no-adjvtime",
> > +    "pauth", "pauth-impdef",
> >      NULL
> >  };
> >  
> > -- 
> > 2.25.1
> > 
> > 
> 
> With the qtest change below I tested the property setting. Maybe we should
> add that diff to this patch.
> 
> In any case,
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> Thanks,
> drew
> 
> 
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index f7e062c1891e..bfe08b9b84f0 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -427,6 +427,18 @@ static void sve_tests_sve_off_kvm(const void *data)
>      qtest_quit(qts);
>  }
>  
> +static void pauth_tests_default(QTestState *qts, const char *cpu_type)
> +{
> +    assert_has_feature_enabled(qts, "max", "pauth");
> +    assert_has_feature_disabled(qts, "max", "pauth-impdef");
> +    assert_set_feature(qts, "max", "pauth", false);
> +    assert_set_feature(qts, "max", "pauth", true);
> +    assert_set_feature(qts, "max", "pauth-impdef", true);
> +    assert_set_feature(qts, "max", "pauth-impdef", false);
> +    assert_error(qts, "max", "cannot enable pauth-impdef without pauth",
> +                 "{ 'pauth': false, 'pauth-impdef': true }");

Eh, all the '"max"' uses above should be 'cpu_type', like below.

Thanks,
drew


diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index f7e062c1891e..176b7f2fdd05 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -427,6 +427,18 @@ static void sve_tests_sve_off_kvm(const void *data)
     qtest_quit(qts);
 }
 
+static void pauth_tests_default(QTestState *qts, const char *cpu_type)
+{
+    assert_has_feature_enabled(qts, cpu_type, "pauth");
+    assert_has_feature_disabled(qts, cpu_type, "pauth-impdef");
+    assert_set_feature(qts, cpu_type, "pauth", false);
+    assert_set_feature(qts, cpu_type, "pauth", true);
+    assert_set_feature(qts, cpu_type, "pauth-impdef", true);
+    assert_set_feature(qts, cpu_type, "pauth-impdef", false);
+    assert_error(qts, cpu_type, "cannot enable pauth-impdef without pauth",
+                 "{ 'pauth': false, 'pauth-impdef': true }");
+}
+
 static void test_query_cpu_model_expansion(const void *data)
 {
     QTestState *qts;
@@ -461,6 +473,7 @@ static void test_query_cpu_model_expansion(const void *data)
         assert_has_feature_enabled(qts, "cortex-a57", "aarch64");
 
         sve_tests_default(qts, "max");
+        pauth_tests_default(qts, "max");
 
         /* Test that features that depend on KVM generate errors without. */
         assert_error(qts, "max",

[PATCH v2 2/3] target/arm: Implement an IMPDEF pauth algorithm

[Richard Henderson]

Without hardware acceleration, a cryptographically strong
algorithm is too expensive for pauth_computepac.

Even with hardware accel, we are not currently expecting
to link the linux-user binaries to any crypto libraries,
and doing so would generally make the --static build fail.

So choose XXH64 as a reasonably quick and decent hash.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
v2: Move the XXH64 bits to xxhash.h (ajb).
    Create isar_feature_aa64_pauth_arch and fixup a comment
    in isar_feature_aa64_pauth that no longer applies.
---
 include/qemu/xxhash.h     | 82 +++++++++++++++++++++++++++++++++++++++
 target/arm/cpu.h          | 15 +++++--
 target/arm/pauth_helper.c | 41 +++++++++++++++++---
 3 files changed, 129 insertions(+), 9 deletions(-)

diff --git a/include/qemu/xxhash.h b/include/qemu/xxhash.h
index 076f1f6054..93ba1a0425 100644
--- a/include/qemu/xxhash.h
+++ b/include/qemu/xxhash.h
@@ -119,4 +119,86 @@ static inline uint32_t qemu_xxhash6(uint64_t ab, uint64_t cd, uint32_t e,
     return qemu_xxhash7(ab, cd, e, f, 0);
 }
 
+/*
+ * Component parts of the XXH64 algorithm from
+ * https://github.com/Cyan4973/xxHash/blob/v0.8.0/xxhash.h
+ *
+ * The complete algorithm looks like
+ *
+ *  i = 0;
+ *  if (len >= 32) {
+ *      v1 = seed + PRIME64_1 + PRIME64_2;
+ *      v2 = seed + PRIME64_2;
+ *      v3 = seed + 0;
+ *      v4 = seed - XXH_PRIME64_1;
+ *      do {
+ *          v1 = XXH64_round(v1, get64bits(input + i));
+ *          v2 = XXH64_round(v2, get64bits(input + i + 8));
+ *          v3 = XXH64_round(v3, get64bits(input + i + 16));
+ *          v4 = XXH64_round(v4, get64bits(input + i + 24));
+ *      } while ((i += 32) <= len);
+ *      h64 = XXH64_mergerounds(v1, v2, v3, v4);
+ *  } else {
+ *      h64 = seed + PRIME64_5;
+ *  }
+ *  h64 += len;
+ *
+ *  for (; i + 8 <= len; i += 8) {
+ *      h64 ^= XXH64_round(0, get64bits(input + i));
+ *      h64 = rol64(h64, 27) * PRIME64_1 + PRIME64_4;
+ *  }
+ *  for (; i + 4 <= len; i += 4) {
+ *      h64 ^= get32bits(input + i) * PRIME64_1;
+ *      h64 = rol64(h64, 23) * PRIME64_2 + PRIME64_3;
+ *  }
+ *  for (; i < len; i += 1) {
+ *      h64 ^= get8bits(input + i) * PRIME64_5;
+ *      h64 = rol64(h64, 11) * PRIME64_1;
+ *  }
+ *
+ *  return XXH64_avalanche(h64)
+ *
+ * Exposing the pieces instead allows for simplified usage when
+ * the length is a known constant and the inputs are in registers.
+ */
+#define PRIME64_1   0x9E3779B185EBCA87ULL
+#define PRIME64_2   0xC2B2AE3D27D4EB4FULL
+#define PRIME64_3   0x165667B19E3779F9ULL
+#define PRIME64_4   0x85EBCA77C2B2AE63ULL
+#define PRIME64_5   0x27D4EB2F165667C5ULL
+
+static inline uint64_t XXH64_round(uint64_t acc, uint64_t input)
+{
+    return rol64(acc + input * PRIME64_2, 31) * PRIME64_1;
+}
+
+static inline uint64_t XXH64_mergeround(uint64_t acc, uint64_t val)
+{
+    return (acc ^ XXH64_round(0, val)) * PRIME64_1 + PRIME64_4;
+}
+
+static inline uint64_t XXH64_mergerounds(uint64_t v1, uint64_t v2,
+                                         uint64_t v3, uint64_t v4)
+{
+    uint64_t h64;
+
+    h64 = rol64(v1, 1) + rol64(v2, 7) + rol64(v3, 12) + rol64(v4, 18);
+    h64 = XXH64_mergeround(h64, v1);
+    h64 = XXH64_mergeround(h64, v2);
+    h64 = XXH64_mergeround(h64, v3);
+    h64 = XXH64_mergeround(h64, v4);
+
+    return h64;
+}
+
+static inline uint64_t XXH64_avalanche(uint64_t h64)
+{
+    h64 ^= h64 >> 33;
+    h64 *= PRIME64_2;
+    h64 ^= h64 >> 29;
+    h64 *= PRIME64_3;
+    h64 ^= h64 >> 32;
+    return h64;
+}
+
 #endif /* QEMU_XXHASH_H */
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 44901923c8..776bf30cbc 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -3767,10 +3767,8 @@ static inline bool isar_feature_aa64_fcma(const ARMISARegisters *id)
 static inline bool isar_feature_aa64_pauth(const ARMISARegisters *id)
 {
     /*
-     * Note that while QEMU will only implement the architected algorithm
-     * QARMA, and thus APA+GPA, the host cpu for kvm may use implementation
-     * defined algorithms, and thus API+GPI, and this predicate controls
-     * migration of the 128-bit keys.
+     * Return true if any form of pauth is enabled, as this
+     * predicate controls migration of the 128-bit keys.
      */
     return (id->id_aa64isar1 &
             (FIELD_DP64(0, ID_AA64ISAR1, APA, 0xf) |
@@ -3779,6 +3777,15 @@ static inline bool isar_feature_aa64_pauth(const ARMISARegisters *id)
              FIELD_DP64(0, ID_AA64ISAR1, GPI, 0xf))) != 0;
 }
 
+static inline bool isar_feature_aa64_pauth_arch(const ARMISARegisters *id)
+{
+    /*
+     * Return true if pauth is enabled with the architected QARMA 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_sb(const ARMISARegisters *id)
 {
     return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, SB) != 0;
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index 6dbab03768..6ec4f83ff0 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -24,6 +24,7 @@
 #include "exec/cpu_ldst.h"
 #include "exec/helper-proto.h"
 #include "tcg/tcg-gvec-desc.h"
+#include "qemu/xxhash.h"
 
 
 static uint64_t pac_cell_shuffle(uint64_t i)
@@ -207,8 +208,8 @@ static uint64_t tweak_inv_shuffle(uint64_t i)
     return o;
 }
 
-static uint64_t pauth_computepac(uint64_t data, uint64_t modifier,
-                                 ARMPACKey key)
+static uint64_t __attribute__((noinline))
+pauth_computepac_architected(uint64_t data, uint64_t modifier, ARMPACKey key)
 {
     static const uint64_t RC[5] = {
         0x0000000000000000ull,
@@ -272,6 +273,36 @@ static uint64_t pauth_computepac(uint64_t data, uint64_t modifier,
     return workingval;
 }
 
+/*
+ * The XXH64 algorithm from
+ * https://github.com/Cyan4973/xxHash/blob/v0.8.0/xxhash.h
+ */
+static uint64_t __attribute__((noinline))
+pauth_computepac_impdef(uint64_t data, uint64_t modifier, ARMPACKey key)
+{
+    uint64_t v1 = QEMU_XXHASH_SEED + PRIME64_1 + PRIME64_2;
+    uint64_t v2 = QEMU_XXHASH_SEED + PRIME64_2;
+    uint64_t v3 = QEMU_XXHASH_SEED + 0;
+    uint64_t v4 = QEMU_XXHASH_SEED - PRIME64_1;
+
+    v1 = XXH64_round(v1, data);
+    v2 = XXH64_round(v2, modifier);
+    v3 = XXH64_round(v3, key.lo);
+    v4 = XXH64_round(v4, key.hi);
+
+    return XXH64_avalanche(XXH64_mergerounds(v1, v2, v3, v4));
+}
+
+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);
+    } else {
+        return pauth_computepac_impdef(data, modifier, key);
+    }
+}
+
 static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
                              ARMPACKey *key, bool data)
 {
@@ -292,7 +323,7 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
     bot_bit = 64 - param.tsz;
     ext_ptr = deposit64(ptr, bot_bit, top_bit - bot_bit, ext);
 
-    pac = pauth_computepac(ext_ptr, modifier, *key);
+    pac = pauth_computepac(env, ext_ptr, modifier, *key);
 
     /*
      * Check if the ptr has good extension bits and corrupt the
@@ -341,7 +372,7 @@ static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
     uint64_t pac, orig_ptr, test;
 
     orig_ptr = pauth_original_ptr(ptr, param);
-    pac = pauth_computepac(orig_ptr, modifier, *key);
+    pac = pauth_computepac(env, orig_ptr, modifier, *key);
     bot_bit = 64 - param.tsz;
     top_bit = 64 - 8 * param.tbi;
 
@@ -442,7 +473,7 @@ uint64_t HELPER(pacga)(CPUARMState *env, uint64_t x, uint64_t y)
     uint64_t pac;
 
     pauth_check_trap(env, arm_current_el(env), GETPC());
-    pac = pauth_computepac(x, y, env->keys.apga);
+    pac = pauth_computepac(env, x, y, env->keys.apga);
 
     return pac & 0xffffffff00000000ull;
 }
-- 
2.25.1

Re: [PATCH v2 2/3] target/arm: Implement an IMPDEF pauth algorithm

[Andrew Jones]

On Thu, Aug 13, 2020 at 01:02:42PM -0700, Richard Henderson wrote:
> Without hardware acceleration, a cryptographically strong
> algorithm is too expensive for pauth_computepac.
> 
> Even with hardware accel, we are not currently expecting
> to link the linux-user binaries to any crypto libraries,
> and doing so would generally make the --static build fail.
> 
> So choose XXH64 as a reasonably quick and decent hash.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> v2: Move the XXH64 bits to xxhash.h (ajb).
>     Create isar_feature_aa64_pauth_arch and fixup a comment
>     in isar_feature_aa64_pauth that no longer applies.
> ---
>  include/qemu/xxhash.h     | 82 +++++++++++++++++++++++++++++++++++++++
>  target/arm/cpu.h          | 15 +++++--
>  target/arm/pauth_helper.c | 41 +++++++++++++++++---
>  3 files changed, 129 insertions(+), 9 deletions(-)
> 
> diff --git a/include/qemu/xxhash.h b/include/qemu/xxhash.h
> index 076f1f6054..93ba1a0425 100644
> --- a/include/qemu/xxhash.h
> +++ b/include/qemu/xxhash.h
> @@ -119,4 +119,86 @@ static inline uint32_t qemu_xxhash6(uint64_t ab, uint64_t cd, uint32_t e,
>      return qemu_xxhash7(ab, cd, e, f, 0);
>  }
>  
> +/*
> + * Component parts of the XXH64 algorithm from
> + * https://github.com/Cyan4973/xxHash/blob/v0.8.0/xxhash.h
> + *
> + * The complete algorithm looks like
> + *
> + *  i = 0;
> + *  if (len >= 32) {
> + *      v1 = seed + PRIME64_1 + PRIME64_2;
> + *      v2 = seed + PRIME64_2;
> + *      v3 = seed + 0;
> + *      v4 = seed - XXH_PRIME64_1;
> + *      do {
> + *          v1 = XXH64_round(v1, get64bits(input + i));
> + *          v2 = XXH64_round(v2, get64bits(input + i + 8));
> + *          v3 = XXH64_round(v3, get64bits(input + i + 16));
> + *          v4 = XXH64_round(v4, get64bits(input + i + 24));
> + *      } while ((i += 32) <= len);
> + *      h64 = XXH64_mergerounds(v1, v2, v3, v4);
> + *  } else {
> + *      h64 = seed + PRIME64_5;
> + *  }
> + *  h64 += len;
> + *
> + *  for (; i + 8 <= len; i += 8) {
> + *      h64 ^= XXH64_round(0, get64bits(input + i));
> + *      h64 = rol64(h64, 27) * PRIME64_1 + PRIME64_4;
> + *  }
> + *  for (; i + 4 <= len; i += 4) {
> + *      h64 ^= get32bits(input + i) * PRIME64_1;
> + *      h64 = rol64(h64, 23) * PRIME64_2 + PRIME64_3;
> + *  }
> + *  for (; i < len; i += 1) {
> + *      h64 ^= get8bits(input + i) * PRIME64_5;
> + *      h64 = rol64(h64, 11) * PRIME64_1;
> + *  }
> + *
> + *  return XXH64_avalanche(h64)
> + *
> + * Exposing the pieces instead allows for simplified usage when
> + * the length is a known constant and the inputs are in registers.
> + */
> +#define PRIME64_1   0x9E3779B185EBCA87ULL
> +#define PRIME64_2   0xC2B2AE3D27D4EB4FULL
> +#define PRIME64_3   0x165667B19E3779F9ULL
> +#define PRIME64_4   0x85EBCA77C2B2AE63ULL
> +#define PRIME64_5   0x27D4EB2F165667C5ULL
> +
> +static inline uint64_t XXH64_round(uint64_t acc, uint64_t input)
> +{
> +    return rol64(acc + input * PRIME64_2, 31) * PRIME64_1;
> +}
> +
> +static inline uint64_t XXH64_mergeround(uint64_t acc, uint64_t val)
> +{
> +    return (acc ^ XXH64_round(0, val)) * PRIME64_1 + PRIME64_4;
> +}
> +
> +static inline uint64_t XXH64_mergerounds(uint64_t v1, uint64_t v2,
> +                                         uint64_t v3, uint64_t v4)
> +{
> +    uint64_t h64;
> +
> +    h64 = rol64(v1, 1) + rol64(v2, 7) + rol64(v3, 12) + rol64(v4, 18);
> +    h64 = XXH64_mergeround(h64, v1);
> +    h64 = XXH64_mergeround(h64, v2);
> +    h64 = XXH64_mergeround(h64, v3);
> +    h64 = XXH64_mergeround(h64, v4);
> +
> +    return h64;
> +}
> +
> +static inline uint64_t XXH64_avalanche(uint64_t h64)
> +{
> +    h64 ^= h64 >> 33;
> +    h64 *= PRIME64_2;
> +    h64 ^= h64 >> 29;
> +    h64 *= PRIME64_3;
> +    h64 ^= h64 >> 32;
> +    return h64;
> +}
> +
>  #endif /* QEMU_XXHASH_H */
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 44901923c8..776bf30cbc 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3767,10 +3767,8 @@ static inline bool isar_feature_aa64_fcma(const ARMISARegisters *id)
>  static inline bool isar_feature_aa64_pauth(const ARMISARegisters *id)
>  {
>      /*
> -     * Note that while QEMU will only implement the architected algorithm
> -     * QARMA, and thus APA+GPA, the host cpu for kvm may use implementation
> -     * defined algorithms, and thus API+GPI, and this predicate controls
> -     * migration of the 128-bit keys.
> +     * Return true if any form of pauth is enabled, as this
> +     * predicate controls migration of the 128-bit keys.
>       */
>      return (id->id_aa64isar1 &
>              (FIELD_DP64(0, ID_AA64ISAR1, APA, 0xf) |
> @@ -3779,6 +3777,15 @@ static inline bool isar_feature_aa64_pauth(const ARMISARegisters *id)
>               FIELD_DP64(0, ID_AA64ISAR1, GPI, 0xf))) != 0;
>  }
>  
> +static inline bool isar_feature_aa64_pauth_arch(const ARMISARegisters *id)
> +{
> +    /*
> +     * Return true if pauth is enabled with the architected QARMA 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_sb(const ARMISARegisters *id)
>  {
>      return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, SB) != 0;
> diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
> index 6dbab03768..6ec4f83ff0 100644
> --- a/target/arm/pauth_helper.c
> +++ b/target/arm/pauth_helper.c
> @@ -24,6 +24,7 @@
>  #include "exec/cpu_ldst.h"
>  #include "exec/helper-proto.h"
>  #include "tcg/tcg-gvec-desc.h"
> +#include "qemu/xxhash.h"
>  
>  
>  static uint64_t pac_cell_shuffle(uint64_t i)
> @@ -207,8 +208,8 @@ static uint64_t tweak_inv_shuffle(uint64_t i)
>      return o;
>  }
>  
> -static uint64_t pauth_computepac(uint64_t data, uint64_t modifier,
> -                                 ARMPACKey key)
> +static uint64_t __attribute__((noinline))
> +pauth_computepac_architected(uint64_t data, uint64_t modifier, ARMPACKey key)
>  {
>      static const uint64_t RC[5] = {
>          0x0000000000000000ull,
> @@ -272,6 +273,36 @@ static uint64_t pauth_computepac(uint64_t data, uint64_t modifier,
>      return workingval;
>  }
>  
> +/*
> + * The XXH64 algorithm from
> + * https://github.com/Cyan4973/xxHash/blob/v0.8.0/xxhash.h
> + */
> +static uint64_t __attribute__((noinline))
> +pauth_computepac_impdef(uint64_t data, uint64_t modifier, ARMPACKey key)

Out of curiosity, why do we need to make these computepac functions
noinline?

> +{
> +    uint64_t v1 = QEMU_XXHASH_SEED + PRIME64_1 + PRIME64_2;
> +    uint64_t v2 = QEMU_XXHASH_SEED + PRIME64_2;
> +    uint64_t v3 = QEMU_XXHASH_SEED + 0;
> +    uint64_t v4 = QEMU_XXHASH_SEED - PRIME64_1;
> +
> +    v1 = XXH64_round(v1, data);
> +    v2 = XXH64_round(v2, modifier);
> +    v3 = XXH64_round(v3, key.lo);
> +    v4 = XXH64_round(v4, key.hi);
> +
> +    return XXH64_avalanche(XXH64_mergerounds(v1, v2, v3, v4));
> +}
> +
> +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);
> +    } else {
> +        return pauth_computepac_impdef(data, modifier, key);
> +    }
> +}

I think this patch should come before the last one. As it stands, when
bisecting between the last one and this one a user could attempt to
enable pauth-imdef, but it wouldn't do anything, or it would potentially
break things. However, this patch shouldn't change anything if it comes
first.

> +
>  static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
>                               ARMPACKey *key, bool data)
>  {
> @@ -292,7 +323,7 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
>      bot_bit = 64 - param.tsz;
>      ext_ptr = deposit64(ptr, bot_bit, top_bit - bot_bit, ext);
>  
> -    pac = pauth_computepac(ext_ptr, modifier, *key);
> +    pac = pauth_computepac(env, ext_ptr, modifier, *key);
>  
>      /*
>       * Check if the ptr has good extension bits and corrupt the
> @@ -341,7 +372,7 @@ static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
>      uint64_t pac, orig_ptr, test;
>  
>      orig_ptr = pauth_original_ptr(ptr, param);
> -    pac = pauth_computepac(orig_ptr, modifier, *key);
> +    pac = pauth_computepac(env, orig_ptr, modifier, *key);
>      bot_bit = 64 - param.tsz;
>      top_bit = 64 - 8 * param.tbi;
>  
> @@ -442,7 +473,7 @@ uint64_t HELPER(pacga)(CPUARMState *env, uint64_t x, uint64_t y)
>      uint64_t pac;
>  
>      pauth_check_trap(env, arm_current_el(env), GETPC());
> -    pac = pauth_computepac(x, y, env->keys.apga);
> +    pac = pauth_computepac(env, x, y, env->keys.apga);
>  
>      return pac & 0xffffffff00000000ull;
>  }
> -- 
> 2.25.1
> 
>

Thanks,
drew 

Re: [PATCH v2 2/3] target/arm: Implement an IMPDEF pauth algorithm

[Richard Henderson]

On 8/14/20 2:26 AM, Andrew Jones wrote:
>> +static uint64_t __attribute__((noinline))
>> +pauth_computepac_impdef(uint64_t data, uint64_t modifier, ARMPACKey key)
> 
> Out of curiosity, why do we need to make these computepac functions
> noinline?

Oh, heh.  Left over from profiling.  Will remove.

> I think this patch should come before the last one. As it stands, when
> bisecting between the last one and this one a user could attempt to
> enable pauth-imdef, but it wouldn't do anything, or it would potentially
> break things. However, this patch shouldn't change anything if it comes
> first.

The current patch ordering would enable impdef but implement that with the
architected algorithm.  Which is ok.

But you're right that the other ordering makes more sense.


r~

[PATCH v2 3/3] target/arm: Use object_property_add_bool for "sve" property

[Richard Henderson]

The interface for object_property_add_bool is simpler,
making the code easier to understand.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu64.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 0227862d39..cce0da0b90 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -488,6 +488,12 @@ static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
     cpu->sve_max_vq = max_vq;
 }
 
+/*
+ * Note that cpu_arm_get/set_sve_vq cannot use the simpler
+ * object_property_add_bool interface because they make use
+ * of the contents of "name" to determine which bit on which
+ * to operate.
+ */
 static void cpu_arm_get_sve_vq(Object *obj, Visitor *v, const char *name,
                                void *opaque, Error **errp)
 {
@@ -529,26 +535,17 @@ static void cpu_arm_set_sve_vq(Object *obj, Visitor *v, const char *name,
     set_bit(vq - 1, cpu->sve_vq_init);
 }
 
-static void cpu_arm_get_sve(Object *obj, Visitor *v, const char *name,
-                            void *opaque, Error **errp)
+static bool cpu_arm_get_sve(Object *obj, Error **errp)
 {
     ARMCPU *cpu = ARM_CPU(obj);
-    bool value = cpu_isar_feature(aa64_sve, cpu);
-
-    visit_type_bool(v, name, &value, errp);
+    return cpu_isar_feature(aa64_sve, cpu);
 }
 
-static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name,
-                            void *opaque, Error **errp)
+static void cpu_arm_set_sve(Object *obj, bool value, Error **errp)
 {
     ARMCPU *cpu = ARM_CPU(obj);
-    bool value;
     uint64_t t;
 
-    if (!visit_type_bool(v, name, &value, errp)) {
-        return;
-    }
-
     if (value && kvm_enabled() && !kvm_arm_sve_supported()) {
         error_setg(errp, "'sve' feature not supported by KVM on this host");
         return;
@@ -563,8 +560,7 @@ void aarch64_add_sve_properties(Object *obj)
 {
     uint32_t vq;
 
-    object_property_add(obj, "sve", "bool", cpu_arm_get_sve,
-                        cpu_arm_set_sve, NULL, NULL);
+    object_property_add_bool(obj, "sve", cpu_arm_get_sve, cpu_arm_set_sve);
 
     for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
         char name[8];
-- 
2.25.1

Re: [PATCH v2 3/3] target/arm: Use object_property_add_bool for "sve" property

[Andrew Jones]

On Thu, Aug 13, 2020 at 01:02:43PM -0700, Richard Henderson wrote:
> The interface for object_property_add_bool is simpler,
> making the code easier to understand.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu64.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 0227862d39..cce0da0b90 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -488,6 +488,12 @@ static void cpu_max_set_sve_max_vq(Object *obj, Visitor *v, const char *name,
>      cpu->sve_max_vq = max_vq;
>  }
>  
> +/*
> + * Note that cpu_arm_get/set_sve_vq cannot use the simpler
> + * object_property_add_bool interface because they make use
> + * of the contents of "name" to determine which bit on which
> + * to operate.
> + */
>  static void cpu_arm_get_sve_vq(Object *obj, Visitor *v, const char *name,
>                                 void *opaque, Error **errp)
>  {
> @@ -529,26 +535,17 @@ static void cpu_arm_set_sve_vq(Object *obj, Visitor *v, const char *name,
>      set_bit(vq - 1, cpu->sve_vq_init);
>  }
>  
> -static void cpu_arm_get_sve(Object *obj, Visitor *v, const char *name,
> -                            void *opaque, Error **errp)
> +static bool cpu_arm_get_sve(Object *obj, Error **errp)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
> -    bool value = cpu_isar_feature(aa64_sve, cpu);
> -
> -    visit_type_bool(v, name, &value, errp);
> +    return cpu_isar_feature(aa64_sve, cpu);
>  }
>  
> -static void cpu_arm_set_sve(Object *obj, Visitor *v, const char *name,
> -                            void *opaque, Error **errp)
> +static void cpu_arm_set_sve(Object *obj, bool value, Error **errp)
>  {
>      ARMCPU *cpu = ARM_CPU(obj);
> -    bool value;
>      uint64_t t;
>  
> -    if (!visit_type_bool(v, name, &value, errp)) {
> -        return;
> -    }
> -
>      if (value && kvm_enabled() && !kvm_arm_sve_supported()) {
>          error_setg(errp, "'sve' feature not supported by KVM on this host");
>          return;
> @@ -563,8 +560,7 @@ void aarch64_add_sve_properties(Object *obj)
>  {
>      uint32_t vq;
>  
> -    object_property_add(obj, "sve", "bool", cpu_arm_get_sve,
> -                        cpu_arm_set_sve, NULL, NULL);
> +    object_property_add_bool(obj, "sve", cpu_arm_get_sve, cpu_arm_set_sve);
>  
>      for (vq = 1; vq <= ARM_MAX_VQ; ++vq) {
>          char name[8];
> -- 
> 2.25.1
> 
>

Reviewed-by: Andrew Jones <drjones@redhat.com>