Re: [PATCH 4/7] target/arm: Implement v8.3 Pauth2

From: Peter Maydell <peter.maydell@linaro.org>
To: Aaron Lindsay <aaron@os.amperecomputing.com>
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Vincent Dehors" <vincent.dehors@smile.fr>,
	"Alex Bennée" <alex.bennee@linaro.org>
Subject: Re: [PATCH 4/7] target/arm: Implement v8.3 Pauth2
Date: Mon, 13 Feb 2023 16:49:08 +0000	[thread overview]
Message-ID: <CAFEAcA9ohkUpSbgwW=GdZBuMP4ArbnVa5hrOaxGzmXsRdAAVRg@mail.gmail.com> (raw)
In-Reply-To: <20230202211129.984060-5-aaron@os.amperecomputing.com>

On Thu, 2 Feb 2023 at 21:13, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
>
> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com>
> ---
>  target/arm/pauth_helper.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
> index a83956652f..6ebf6df75c 100644
> --- a/target/arm/pauth_helper.c
> +++ b/target/arm/pauth_helper.c
> @@ -349,7 +349,7 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
>      if (test != 0 && test != -1) {
>          if (cpu_isar_feature(aa64_pauth_epac, env_archcpu(env))) {
>              pac = 0;
> -        } else {
> +        } else if (! cpu_isar_feature(aa64_pauth2, env_archcpu(env))) {

I think we should write this set of conditions as:

     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 {
         /* Note that etc etc */
         pac ^= MAKE_64BIT_MASK(top_bit - 2, 1);
     }

I know this isn't the way round the pseudocode does it, but if we do
it this way it means we don't need to have the weird special case
where we do an == check instead of >= in the epac isar_feature test
function.

>              /*
>               * Note that our top_bit is one greater than the pseudocode's
>               * version, hence "- 2" here.
> @@ -362,6 +362,8 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
>       * Preserve the determination between upper and lower at bit 55,
>       * and insert pointer authentication code.
>       */
> +    if (cpu_isar_feature(aa64_pauth2, env_archcpu(env)))
> +        pac ^= ptr;

All if statements need braces, even one-line ones.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM