[1/7] target/arm: v8.3 PAC ID_AA64ISAR[12] feature-detection

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(-)

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

On Thu, 2 Feb 2023 at 21:13, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
>
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>


> 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

Just noticed -- this should I guess be in a later patch, because
it's added an extra argument to the function call without changing
the function definition or declaration, so I think this patch
won't compile as-is.

-- PMM

On Feb 13 16:01, Peter Maydell wrote:
> On Thu, 2 Feb 2023 at 21:13, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
> > +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.

FYI - I did make this `>= 0b0010` after changing the logic around elsewhere to
be compatible as you suggested. I'm also planning to add a comment like this:

 /*
  * 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.
  */


> 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.

Thanks for your review!

I've made a number of your (simpler) suggested changes already, and will
target getting a new patchset out in the next couple weeks after I spend
more time withi the the remaining GETPC() changes that need a little
more thought/rework, and we sort out what the command-line options
should look like.

-Aaron