Re: [PATCH v4 3/9] target/arm: Add feature detection for FEAT_Pauth2 and extensions

[Peter Maydell]

On Tue, 22 Aug 2023 at 05:27, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> From: Aaron Lindsay <aaron@os.amperecomputing.com>
>
> Rename isar_feature_aa64_pauth_arch to isar_feature_aa64_pauth_qarma5
> to distinguish the other architectural algorithm qarma3.
>
> Add ARMPauthFeature and isar_feature_pauth_feature to cover the
> other pauth conditions.
>
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> Message-Id: <20230609172324.982888-3-aaron@os.amperecomputing.com>
> [rth: Add ARMPauthFeature and eliminate most other predicates]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h              | 49 +++++++++++++++++++++++++++++------
>  target/arm/tcg/pauth_helper.c |  2 +-
>  2 files changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index fbdbf2df7f..e9fe268453 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3794,28 +3794,61 @@ static inline bool isar_feature_aa64_fcma(const 
> ARMISARegisters *id)
>      return FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, FCMA) != 0;
>  }
>
> +/*
> + * These are the values from APA/API/APA3.
> + *
> + * They must be compared '>=', except EPAC should use '=='.
> + * In the ARM pseudocode, EPAC is treated as not being implemented
> + * by larger values.
> + */

Yeah, but we use PauthFeat_EPAC in exactly one place and
deliberately in a way where it doesn't matter if we use >=...
(I think the pseudocode would be clearer if it was adjusted
to do the same, personally. This ID register field follows
the standard ID scheme which means that increasing values
are supersets, so the "only if equal" part is weird.)

> +typedef enum {
> +    PauthFeat_None         = 0,
> +    PauthFeat_1            = 1,
> +    PauthFeat_EPAC         = 2,
> +    PauthFeat_2            = 3,
> +    PauthFeat_FPAC         = 4,
> +    PauthFeat_FPACCOMBINED = 5,
> +} ARMPauthFeature;
> +
> +static inline ARMPauthFeature
> +isar_feature_pauth_feature(const ARMISARegisters *id)
> +{
> +    /*
> +     * Architecturally, only one of {APA,API,APA3} may be active (non-zero)
> +     * and the other two must be zero.  Thus we may avoid conditionals.
> +     */
> +    return (FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, APA) |
> +            FIELD_EX64(id->id_aa64isar1, ID_AA64ISAR1, API) |
> +            FIELD_EX64(id->id_aa64isar2, ID_AA64ISAR2, APA3));
> +}
> +
>  static inline bool isar_feature_aa64_pauth(const ARMISARegisters *id)
>  {
>      /*
>       * Return true if any form of pauth is enabled, as this
>       * predicate controls migration of the 128-bit keys.
>       */
> -    return (id->id_aa64isar1 &
> -            (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;
> +    return isar_feature_pauth_feature(id) != PauthFeat_None;

Having said "must be compared >=" you then use a != comparison here :-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM