Re: [PATCH v2 7/7] target/arm: Add CPU properties for most v8.3 PAC features

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

On 2/22/23 09:35, Aaron Lindsay wrote:
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> ---
>   target/arm/cpu.h   |  5 +++
>   target/arm/cpu64.c | 81 ++++++++++++++++++++++++++++++++++++++--------
>   2 files changed, 72 insertions(+), 14 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9c3cbc9a29..40b4631f11 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1039,6 +1039,11 @@ struct ArchCPU {
>        */
>       bool prop_pauth;
>       bool prop_pauth_impdef;
> +    bool prop_pauth_qarma3;
> +    bool prop_pauth_epac;
> +    bool prop_pauth2; // also known as EnhancedPAC2/EPAC2

No c++ comments.

> +    if (cpu->prop_pauth_epac &&
> +            (cpu->prop_pauth2 ||
> +             cpu->prop_pauth_fpac ||
> +             cpu->prop_pauth_fpac_combine)) {

Indentation.

> +        if (address_auth == 0)
> +            address_auth = 0b0001;

Missing braces.

> +static Property arm_cpu_pauth2_property =
> +    DEFINE_PROP_BOOL("pauth2", ARMCPU, prop_pauth2, false);
> +static Property arm_cpu_pauth_fpac_property =
> +    DEFINE_PROP_BOOL("pauth-fpac", ARMCPU, prop_pauth_fpac, false);
> +static Property arm_cpu_pauth_fpac_combine_property =
> +    DEFINE_PROP_BOOL("pauth-fpac-combine", ARMCPU, prop_pauth_fpac_combine, false);

For -cpu max, I would expect these to default on.
Or perhaps not expose these or epac as properties at all.

> @@ -646,6 +694,11 @@ static void aarch64_add_pauth_properties(Object *obj)
>           cpu->prop_pauth = cpu_isar_feature(aa64_pauth, cpu);
>       } else {
>           qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_impdef_property);
> +        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_qarma3_property);
> +        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_epac_property);
> +        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth2_property);
> +        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_fpac_property);
> +        qdev_property_add_static(DEVICE(obj), &arm_cpu_pauth_fpac_combine_property);

I think the *only* property that makes sense for KVM is pauth=on/off, which controls if 
KVM exposes the key registers at all (and if off, APA/GPA/etc all get zeroed). There is 
certainly no way to adjust the algorithm exposed by the hardware.

The primary reason we have a property for pauth at all is speed of emulation.  When we 
first enabled qarma5, we saw a major slowdown, with pauth_computepac consuming nearly 50% 
of the entire runtime.  Later we added impdef, as a way of doing *some* testing of pauth 
without the extreme overhead of qarma5.

I see that qarma3 does about half the work of qarma5, so it would be interesting to 
measure the relative speed of the 3 implementations on a boot of kernel + selftests.

You may want to look a the code generated and play with flatten and noinline attributes 
around pauth_computepac and subroutines.  E.g.

static uint64_t __attribute__((flatten, noinline))
pauth_computepac_qarma5(uint64_t data, uint64_t modifier, ARMPACKey key)
{
     return pauth_computepac_architected(data, modifier, key, false);
}

static uint64_t __attribute__((flatten, noinline))
pauth_computepac_qarma3(uint64_t data, uint64_t modifier, ARMPACKey key)
{
     return pauth_computepac_architected(data, modifier, key, true);
}

static uint64_t __attribute__((flatten, noinline))
pauth_computepac_impdef(uint64_t data, uint64_t modifier, ARMPACKey key)
{
     return qemu_xxhash64_4(data, modifier, key.lo, key.hi);
}

static uint64_t pauth_computepac(CPUARMState *env, uint64_t data,
                                  uint64_t modifier, ARMPACKey key)
{
     if (cpu_isar_feature(aa64_pauth_arch_qarma5, env_archcpu(env))) {
         return pauth_computepac_qarma5(data, modifier, key);
     } else if (cpu_isar_feature(aa64_pauth_arch_qarma3, env_archcpu(env))) {
         return pauth_computepac_qarma3(data, modifier, key);
     } else {
         return pauth_computepac_impdef(data, modifier, key);
     }
}


r~