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

[Richard Henderson <richard.henderson@linaro.org>]

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~