Re: [PATCH 3/3] target/arm: implement v8.1-m PAC support

["Alex Bennée" <alex.bennee@linaro.org>]

Torbjörn SVENSSON <torbjorn.svensson@foss.st.com> writes:

> The algorithm used for hashing is a simple XOR between the pointer
> and the modifier using the "implementation defined" scheme.
>
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com>
> ---
>  target/arm/cpu-features.h  |  6 +++
>  target/arm/internals.h     |  2 +
>  target/arm/tcg/cpu-v7m.c   |  2 +-
>  target/arm/tcg/m_helper.c  | 17 ++++++++
>  target/arm/tcg/translate.c | 98 ++++++++++++++++++++++++++++++++++++++++------
>  5 files changed, 113 insertions(+), 12 deletions(-)
>
> diff --git a/target/arm/cpu-features.h b/target/arm/cpu-features.h
> index 4e44245a8b..60bd7a0765 100644
> --- a/target/arm/cpu-features.h
> +++ b/target/arm/cpu-features.h
> @@ -114,6 +114,7 @@ FIELD(ID_ISAR5, AES, 4, 4)
>  FIELD(ID_ISAR5, SHA1, 8, 4)
>  FIELD(ID_ISAR5, SHA2, 12, 4)
>  FIELD(ID_ISAR5, CRC32, 16, 4)
> +FIELD(ID_ISAR5, PACBTI, 20, 4)
>  FIELD(ID_ISAR5, RDM, 24, 4)
>  FIELD(ID_ISAR5, VCMA, 28, 4)
>  
> @@ -583,6 +584,11 @@ static inline bool isar_feature_aa32_m_sec_state(const ARMISARegisters *id)
>      return FIELD_EX32_IDREG(id, ID_PFR1, SECURITY) >= 3;
>  }
>  
> +static inline bool isar_feature_aa32_m_pacbti(const ARMISARegisters *id)
> +{
> +    return FIELD_EX32_IDREG(id, ID_ISAR5, PACBTI) != 0;
> +}

See isar_feature_aa64_pauth and friends.

> +
>  static inline bool isar_feature_aa32_fp16_arith(const ARMISARegisters *id)
>  {
>      /* Sadly this is encoded differently for A-profile and M-profile */
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 00830b1724..cbb0a1d8fc 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -90,6 +90,8 @@ FIELD(V7M_CONTROL, NPRIV, 0, 1)
>  FIELD(V7M_CONTROL, SPSEL, 1, 1)
>  FIELD(V7M_CONTROL, FPCA, 2, 1)
>  FIELD(V7M_CONTROL, SFPA, 3, 1)
> +FIELD(V7M_CONTROL, PAC_EN, 6, 1)
> +FIELD(V7M_CONTROL, UPAC_EN, 7, 1)

Hmm my copy of the v7m Arm ARM doesn't include these bits...

But I can see it online:

  https://developer.arm.com/documentation/109576/0100/Pointer-Authentication-Code/Enabling-pointer-authentication?lang=en

>  
>  /* Bit definitions for v7M exception return payload */
>  FIELD(V7M_EXCRET, ES, 0, 1)
> diff --git a/target/arm/tcg/cpu-v7m.c b/target/arm/tcg/cpu-v7m.c
> index 5cfda232cd..3beb2b23fa 100644
> --- a/target/arm/tcg/cpu-v7m.c
> +++ b/target/arm/tcg/cpu-v7m.c
> @@ -269,7 +269,7 @@ static void cortex_m85_initfn(Object *obj)
>      SET_IDREG(isar, ID_ISAR2, 0x20232232);
>      SET_IDREG(isar, ID_ISAR3, 0x01111131);
>      SET_IDREG(isar, ID_ISAR4, 0x01310132);
> -    SET_IDREG(isar, ID_ISAR5, 0x00000000);
> +    SET_IDREG(isar, ID_ISAR5, 0x00200000); /* PACBTI=implementation defined */
>      SET_IDREG(isar, ID_ISAR6, 0x00000000);
>      SET_IDREG(isar, CLIDR, 0x00000000); /* caches not implemented */
>      cpu->ctr = 0x8303c003;
> diff --git a/target/arm/tcg/m_helper.c b/target/arm/tcg/m_helper.c
> index f2059ed8b0..1160fe8d87 100644
> --- a/target/arm/tcg/m_helper.c
> +++ b/target/arm/tcg/m_helper.c
> @@ -2658,6 +2658,15 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>                  env->v7m.control[M_REG_S] &= ~R_V7M_CONTROL_FPCA_MASK;
>                  env->v7m.control[M_REG_S] |= val & R_V7M_CONTROL_FPCA_MASK;
>              }
> +
> +            /* Only update PAC_EN / UPAC_EN if PACBTI is implemented. */
> +            if (cpu_isar_feature(aa32_m_pacbti, env_archcpu(env))) {
> +                uint32_t enable_mask =
> +                    R_V7M_CONTROL_PAC_EN_MASK | R_V7M_CONTROL_UPAC_EN_MASK;
> +                env->v7m.control[M_REG_NS] &= ~enable_mask;
> +                env->v7m.control[M_REG_NS] |= val & enable_mask;

Hmm I was going to suggest looking at FIELD_DP32 but it looks like this
is following the existing style.

> +            }
> +
>              return;
>          case 0x98: /* SP_NS */
>          {
> @@ -2784,6 +2793,14 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>                  env->v7m.control[M_REG_S] |= val & R_V7M_CONTROL_FPCA_MASK;
>              }
>          }
> +
> +        /* Only update PAC_EN / UPAC_EN if PACBTI is implemented. */
> +        if (cpu_isar_feature(aa32_m_pacbti, env_archcpu(env))) {
> +            uint32_t enable_mask =
> +                R_V7M_CONTROL_PAC_EN_MASK | R_V7M_CONTROL_UPAC_EN_MASK;
> +            env->v7m.control[env->v7m.secure] &= ~enable_mask;
> +            env->v7m.control[env->v7m.secure] |= val & enable_mask;
> +        }
>          break;
>      default:
>      bad_reg:
> diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
> index ae1351ef03..e13119b33b 100644
> --- a/target/arm/tcg/translate.c
> +++ b/target/arm/tcg/translate.c
> @@ -5012,26 +5012,80 @@ static bool trans_SMMLSR(DisasContext *s, arg_rrrr *a)
>      return op_smmla(s, a, true, true);
>  }
>  
> +static void arm_gen_test_pac_enabled(DisasContext *s, TCGLabel *label)
> +{
> +    int bank = s->v8m_secure ? M_REG_S : M_REG_NS;
> +    int mask = IS_USER(s)
> +        ?  R_V7M_CONTROL_UPAC_EN_MASK
> +        : R_V7M_CONTROL_PAC_EN_MASK;
> +    TCGv_i32 temp = load_cpu_field(v7m.control[bank]);
> +    tcg_gen_brcondi_i32(TCG_COND_TSTEQ, temp, mask, label);
> +}
> +
> +static TCGv_i32 op_create_pac_hash(DisasContext *s, int rn, int rm)
> +{
> +    TCGv_i32 res = tcg_temp_new_i32();
> +    TCGv_i64 ext_ptr = tcg_temp_new_i64();
> +    TCGv_i64 modifier = tcg_temp_new_i64();
> +    TCGv_i64 temp = tcg_temp_new_i64();
> +
> +    tcg_gen_extu_i32_i64(ext_ptr, load_reg(s, rn));
> +    tcg_gen_extu_i32_i64(modifier, load_reg(s, rm));
> +
> +    /*
> +     * This a very simple implementation that just xor the two
> +     * inputs. The goal is not to replicate any of the predefined
> +     * hashing functions, but use a simple check.
> +     */
> +    tcg_gen_xor_i64(temp, ext_ptr, modifier);
> +
> +    /* Return the lower word */
> +    tcg_gen_extrl_i64_i32(res, temp);
> +    return res;

Is it really needed here? Could you not create an equivalent of
pauth_computepac for m-profile and use the architected helpers or
fall-back to the xxhash impl?

Is does mean a helper call but it would keep more in common with the
A-profile code.

> +}
> +
> +static bool op_pacg(DisasContext *s, arg_rrr *a)
> +{
> +    TCGv_i32 temp;
> +    TCGLabel *done = gen_new_label();
> +
> +    arm_gen_test_pac_enabled(s, done);
> +
> +    temp = op_create_pac_hash(s, a->rn, a->rm);
> +    store_reg(s, a->rd, temp);
> +
> +    gen_set_label(done);
> +    return true;
> +}
> +
>  static bool trans_PAC(DisasContext *s, arg_empty *a)
>  {
> +    arg_rrr arg;
> +
>      if (!arm_dc_feature(s, ARM_FEATURE_V8_1M)) {
>          return false;
>      }
>  
> -    /* Handle as if PACBTI is disabled. */
> -    return true;
> +    arg.rd = 0xc; /* R12 */
> +    arg.rn = 0xe; /* LR */
> +    arg.rm = 0xd; /* SP */
> +    return op_pacg(s, &arg);
>  }
>  
>  static bool trans_PACBTI(DisasContext *s, arg_empty *a)
>  {
> +    arg_rrr arg;
> +
>      if (!arm_dc_feature(s, ARM_FEATURE_V8_1M)) {
>          return false;
>      }
>  
>      /* todo: reset EPSR.B to 0 */
>  
> -    /* Handle as if PACBTI is disabled. */
> -    return true;
> +    arg.rd = 0xc; /* R12 */
> +    arg.rn = 0xe; /* LR */
> +    arg.rm = 0xd; /* SP */
> +    return op_pacg(s, &arg);
>  }
>  
>  static bool trans_PACG(DisasContext *s, arg_rrr *a)
> @@ -5040,7 +5094,26 @@ static bool trans_PACG(DisasContext *s, arg_rrr *a)
>          return false;
>      }
>  
> -    /* Handle as if PACBTI is disabled. */
> +    return op_pacg(s, a);
> +}
> +
> +static bool op_autg(DisasContext *s, arg_rrrr *a, int set_pc_from_reg)
> +{
> +    TCGv_i32 expected_pac_hash, actual_pac_hash;
> +    TCGLabel *done = gen_new_label();
> +    TCGLabel *fail = delay_exception(s, EXCP_INVSTATE, syn_uncategorized());
> +
> +    arm_gen_test_pac_enabled(s, done);
> +
> +    expected_pac_hash = load_reg(s, a->ra);
> +    actual_pac_hash = op_create_pac_hash(s, a->rn, a->rm);
> +    tcg_gen_brcond_i32(TCG_COND_NE, expected_pac_hash, actual_pac_hash, fail);
> +
> +    gen_set_label(done);
> +    if (set_pc_from_reg >= 0) {
> +        gen_bx_excret(s, load_reg(s, set_pc_from_reg));
> +    }
> +
>      return true;
>  }
>  
> @@ -5050,18 +5123,22 @@ static bool trans_BXAUT(DisasContext *s, arg_rrrr *a)
>          return false;
>      }
>  
> -    /* Handle as if PACBTI is disabled. */
> -    return true;
> +    return op_autg(s, a, a->rn);
>  }
>  
>  static bool trans_AUT(DisasContext *s, arg_empty *a)
>  {
> +    arg_rrrr arg;
> +
>      if (!arm_dc_feature(s, ARM_FEATURE_V8_1M)) {
>          return false;
>      }
>  
> -    /* Handle as if PACBTI is disabled. */
> -    return true;
> +    arg.rd = 0; /* unused */
> +    arg.ra = 0xc; /* R12 */
> +    arg.rn = 0xe; /* LR */
> +    arg.rm = 0xd; /* SP */
> +    return op_autg(s, &arg, -1);
>  }
>  
>  static bool trans_AUTG(DisasContext *s, arg_rrrr *a)
> @@ -5070,8 +5147,7 @@ static bool trans_AUTG(DisasContext *s, arg_rrrr *a)
>          return false;
>      }
>  
> -    /* Handle as if PACBTI is disabled. */
> -    return true;
> +    return op_autg(s, a, -1);
>  }
>  
>  static bool op_div(DisasContext *s, arg_rrr *a, bool u)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro