Re: [PATCH v6 2/3] target/arm: Add cpu properties to control pauth

[Andrew Jones]

On Mon, Jan 11, 2021 at 01:11:07PM -1000, 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.
> 
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> v2: Use boolean properties instead of an enum (drjones).
> v3: Add tests (drjones).
> v6: Add documentation (pmm).
> ---
>  docs/system/arm/cpu-features.rst | 21 +++++++++++++++++
>  target/arm/cpu.h                 | 10 ++++++++
>  target/arm/cpu.c                 | 13 +++++++++++
>  target/arm/cpu64.c               | 40 ++++++++++++++++++++++++++++----
>  target/arm/monitor.c             |  3 ++-
>  tests/qtest/arm-cpu-features.c   | 13 +++++++++++
>  6 files changed, 95 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/system/arm/cpu-features.rst 
> b/docs/system/arm/cpu-features.rst
> index 35196a6b75..70e0e4ef78 100644
> --- a/docs/system/arm/cpu-features.rst
> +++ b/docs/system/arm/cpu-features.rst
> @@ -211,6 +211,27 @@ the list of KVM VCPU features and their descriptions.
>                             influence the guest scheduler behavior and/or be
>                             exposed to the guest userspace.
>  
> +TCG VCPU Features
> +=================
> +
> +TCG VCPU features are CPU features that are specific to TCG.
> +Below is the list of TCG VCPU features and their descriptions.
> +
> +  pauth                    Enable or disable `FEAT_Pauth`, pointer
> +                           authentication.  By default, the feature is
> +                           enabled with `-cpu max`.
> +
> +  pauth-impdef             When `FEAT_Pauth` is enabled, either the
> +                           *impdef* (Implementation Definined) algorithm
> +                           is enabled or the *architected* QARMA algorithm
> +                           is enabled.  By default the impdef algorithm
> +                           is disabled, and QARMA is enabled.
> +
> +                           The architected QARMA algorithm has good
> +                           cryptographic properties, but can be quite slow
> +                           to emulate.  The impdef algorithm is
> +                           non-cryptographic but significantly faster.
> +

Doc updates look good.

>  SVE CPU Properties
>  ==================
>  
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 70e9618d13..06f5169f45 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -197,9 +197,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 {
> @@ -947,6 +949,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 8387e94b94..be18df5464 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1320,6 +1320,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;
> +            }
> +        }
>      }
>  
>      if (kvm_enabled()) {
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 7cf9fc4bc6..d9feaa9cdb 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);
> @@ -720,6 +748,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 198b14e95e..1fdb965eb8 100644
> --- a/target/arm/monitor.c
> +++ b/target/arm/monitor.c
> @@ -94,7 +94,8 @@ static const char *cpu_model_advertised_features[] = {
>      "sve128", "sve256", "sve384", "sve512",
>      "sve640", "sve768", "sve896", "sve1024", "sve1152", "sve1280",
>      "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
> -    "kvm-no-adjvtime", "kvm-steal-time",
> +    "kvm-no-adjvtime",

Looks like a rebase error; kvm-steal-time is getting dropped.

> +    "pauth", "pauth-impdef",
>      NULL
>  };
>  
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index bc681a95d5..8252b85bb8 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;
> @@ -462,6 +474,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",
> -- 
> 2.25.1
> 
> 

Thanks,
drew