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