| Message ID | 20230222193544.3392713-2-aaron@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement Most ARMv8.3 Pointer Authentication Features | expand |
On 2/22/23 09:35, Aaron Lindsay wrote: > +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. > + */ This language isn't quite right, since GPA3 only defines values 0 and 1. Perhaps "to the same result"? > +static inline uint8_t isar_feature_pauth_get_features(const ARMISARegisters *id) 'int' is a better generic result, as 'uint8_t' is 'unsigned char' to the debugger and generally printed as such. > + 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); Braces with if+else, always. That said, exactly one of these fields is allowed to be non-zero, so we can just unconditionally OR them all together. > +static inline bool isar_feature_aa64_pauth_epac(const ARMISARegisters *id) > +{ > + /* > + * Note that unlike most AArch64 features, EPAC is treated (in the ARM > + * psedocode, at least) as not being implemented by larger values of this > + * field. Our usage of '>=' rather than '==' here causes our implementation > + * of PAC logic to diverge slightly from ARM pseudocode. > + */ I find this comment scary -- "diverge slightly"? All I need is once sentence to indicate how this is mitigated (by testing pauth2 first where required?), or "See function_foo" (where there is more commentary), or something. > 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 }, All the code adding aa64isar2 should be a separate patch. You've missed initializing it in kvm_arm_get_host_cpu_features and hvf_arm_get_host_cpu_features. r~
On Wed, 22 Feb 2023 at 20:27, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/22/23 09:35, Aaron Lindsay wrote: > > +static inline bool isar_feature_aa64_pauth_epac(const ARMISARegisters *id) > > +{ > > + /* > > + * Note that unlike most AArch64 features, EPAC is treated (in the ARM > > + * psedocode, at least) as not being implemented by larger values of this > > + * field. Our usage of '>=' rather than '==' here causes our implementation > > + * of PAC logic to diverge slightly from ARM pseudocode. > > + */ > > I find this comment scary -- "diverge slightly"? > > All I need is once sentence to indicate how this is mitigated (by testing pauth2 first > where required?), or "See function_foo" (where there is more commentary), or something. Yeah, we structure the one place the check is used (patch 4) so that we only check the pauth_epac feature if we already tested pauth2: + 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 { where the pseudocode currently has: if HaveEnhancedPAC() then pac = 0; elsif !HaveEnhancedPAC2() then old stuff; and is relying on anything with PAuth2 not returning true for HaveEnhancedPAC(). It is of course possible that the pseudocode might be rephrased in future; I think the way they've done it at the moment is kind of confusing. thanks -- PMM
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 8cf70693be..9c3cbc9a29 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,72 @@ 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) +{ + /* + * 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. + */ + return isar_feature_pauth_get_features(id) >= 0b0010; +} + +static inline bool isar_feature_aa64_pauth2(const ARMISARegisters *id) +{ + return isar_feature_pauth_get_features(id) >= 0b0011; +} + +static inline bool isar_feature_aa64_fpac(const ARMISARegisters *id) +{ + return isar_feature_pauth_get_features(id) >= 0b0100; +} + +static inline bool isar_feature_aa64_fpac_combine(const ARMISARegisters *id) +{ + return isar_feature_pauth_get_features(id) >= 0b0101; +} + static inline bool isar_feature_aa64_tlbirange(const ARMISARegisters *id) { return FIELD_EX64(id->id_aa64isar0, ID_AA64ISAR0, TLB) == 2; diff --git a/target/arm/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..e5206453f6 100644 --- a/target/arm/pauth_helper.c +++ b/target/arm/pauth_helper.c @@ -282,7 +282,7 @@ static uint64_t pauth_computepac_impdef(uint64_t data, uint64_t modifier, static uint64_t pauth_computepac(CPUARMState *env, uint64_t data, uint64_t modifier, ARMPACKey key) { - if (cpu_isar_feature(aa64_pauth_arch, env_archcpu(env))) { + if (cpu_isar_feature(aa64_pauth_arch_qarma5, env_archcpu(env))) { return pauth_computepac_architected(data, modifier, key); } else { return pauth_computepac_impdef(data, modifier, key);
Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> --- target/arm/cpu.h | 61 +++++++++++++++++++++++++++++++++++++-- target/arm/helper.c | 4 +-- target/arm/pauth_helper.c | 2 +- 3 files changed, 61 insertions(+), 6 deletions(-)