Re: [PATCH 6/7] target/arm: Implement v8.3 FPAC and FPACCOMBINE

[Peter Maydell]

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 | 26 ++++++++++++++++++++++++++
>  target/arm/syndrome.h     |  6 ++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
> index 66dc90a289..3a2772de0e 100644
> --- a/target/arm/pauth_helper.c
> +++ b/target/arm/pauth_helper.c
> @@ -385,6 +385,21 @@ static uint64_t pauth_original_ptr(uint64_t ptr, 
> ARMVAParameters param)
>      return deposit64(ptr, bot_pac_bit, top_pac_bit - bot_pac_bit, extfield);
>  }
>
> +static G_NORETURN
> +void pauth_fail_exception(CPUARMState *env, int error_code)
> +{
> +    int target_el = arm_current_el(env);
> +    if (target_el == 0) {
> +        uint64_t hcr = arm_hcr_el2_eff(env);
> +        if (arm_is_el2_enabled(env) && (hcr & HCR_TGE))
> +            target_el = 2;
> +        else
> +            target_el = 1;
> +    }
> +
> +    raise_exception_ra(env, EXCP_UDEF, syn_pacfail(error_code), target_el, 
> GETPC());

This won't work -- you must call GETPC() from the top-level helper
function directly called from JITted code, so that it can get the
PC of the callsite in the JITted code. Otherwise you'll get a PC
somewhere inside QEMU's C code, which won't do the right thing.
This is why pauth_check_trap() takes an 'ra' argument (for
'return address') and all the top level helper functions call
GETPC() to get the value to pass.

> +}
> +
>  static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
>                             ARMPACKey *key, bool data, int keynumber,
>                             bool is_combined)
> @@ -403,6 +418,17 @@ static uint64_t pauth_auth(CPUARMState *env, uint64_t 
> ptr, uint64_t modifier,
>          uint64_t xor_mask = MAKE_64BIT_MASK(bot_bit, top_bit - bot_bit + 1) &
>              ~MAKE_64BIT_MASK(55, 1);
>          result = ((ptr ^ pac) & xor_mask) | (ptr & ~xor_mask);
> +        if (cpu_isar_feature(aa64_fpac_combine, env_archcpu(env)) ||
> +                (cpu_isar_feature(aa64_fpac, env_archcpu(env)) &&
> +                 !is_combined)) {
> +            int fpac_top = param.tbi ? 55 : 64;
> +            uint64_t fpac_mask = MAKE_64BIT_MASK(bot_bit, fpac_top - 
> bot_bit);
> +            test = (result ^ sextract64(result, 55, 1)) & fpac_mask;
> +            if (unlikely(test)) {
> +                int error_code = ((data ? 1 : 0) << 1) | (keynumber);
> +                pauth_fail_exception(env, error_code);
> +            }
> +        }
>      } else {
>          test = (pac ^ ptr) & ~MAKE_64BIT_MASK(55, 1);
>          if (unlikely(extract64(test, bot_bit, top_bit - bot_bit))) {
> diff --git a/target/arm/syndrome.h b/target/arm/syndrome.h
> index 73df5e3793..885a85735c 100644
> --- a/target/arm/syndrome.h
> +++ b/target/arm/syndrome.h
> @@ -48,6 +48,7 @@ enum arm_exception_class {
>      EC_AA64_SMC               = 0x17,
>      EC_SYSTEMREGISTERTRAP     = 0x18,
>      EC_SVEACCESSTRAP          = 0x19,
> +    EC_PACFAIL                = 0x1c,
>      EC_SMETRAP                = 0x1d,
>      EC_INSNABORT              = 0x20,
>      EC_INSNABORT_SAME_EL      = 0x21,
> @@ -221,6 +222,11 @@ static inline uint32_t syn_smetrap(SMEExceptionType 
> etype, bool is_16bit)
>          | (is_16bit ? 0 : ARM_EL_IL) | etype;
>  }
>
> +static inline uint32_t syn_pacfail(int error_code)
> +{
> +    return (EC_PACFAIL << ARM_EL_EC_SHIFT) | error_code;

You need ARM_EL_IL here too, I think.

I would suggest that you make the syn_pacfail() function take
two arguments (bool data and int keynumber), and put them in to
bits 0 and 1 in this function. That avoids the need to
construct an error code at the callsite.

> +}
> +
>  static inline uint32_t syn_pactrap(void)
>  {
>      return EC_PACTRAP << ARM_EL_EC_SHIFT;
> --
> 2.25.1

thanks
-- PMM