On Wed, 22 Feb 2023 at 20:27, Richard Henderson
<richard.hender...@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
