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

[Peter Maydell]

On Wed, 22 Feb 2023 at 20:27, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/22/23 09:35, Aaron Lindsay wrote:
> > +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.

Yeah, we structure the one place the check is used (patch 4) so that
we only check the pauth_epac feature if we already tested pauth2:

+        if (cpu_isar_feature(aa64_pauth2, env_archcpu(env))) {
+            /* No action required */
+        } else if (cpu_isar_feature(aa64_pauth_epac, env_archcpu(env))) {
             pac = 0;
         } else {

where the pseudocode currently has:
         if HaveEnhancedPAC() then
             pac = 0;
         elsif !HaveEnhancedPAC2() then
             old stuff;
and is relying on anything with PAuth2 not returning true for HaveEnhancedPAC().

It is of course possible that the pseudocode might be rephrased in future;
I think the way they've done it at the moment is kind of confusing.

thanks
-- PMM