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

From: Aaron Lindsay <aaron@os.amperecomputing.com>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org,
	"Vincent Dehors" <vincent.dehors@smile.fr>,
	"Alex Bennée" <alex.bennee@linaro.org>,
	"Peter Maydell" <peter.maydell@linaro.org>
Subject: Re: [PATCH v2 7/7] target/arm: Add CPU properties for most v8.3 PAC features
Date: Wed, 22 Mar 2023 16:36:39 -0400	[thread overview]
Message-ID: <ZBtm15EMqUZHRx3a@strawberry.localdomain> (raw)
In-Reply-To: <c79aa318-0fa9-e93e-56dc-c084de47f645@linaro.org>

On Feb 22 12:14, Richard Henderson wrote:
> On 2/22/23 09:35, Aaron Lindsay wrote:
> > +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.

I've removed these properties, and epac's as well. It now defaults to
the equivalent of prop_pauth_fpac_combine==true in my previous patch.

> 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);
>     }
> }

I have not played around with this further. Do you feel this is
important to look into prior to merging this patchset (since QARMA3
isn't the default)?

-Aaron