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.hender...@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 <drjo...@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",