Re: [Qemu-devel] [Qemu-arm] [PATCH v5 02/17] target/arm: Split out rebuild_hflags_a64

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

Richard Henderson <richard.henderson@linaro.org> writes:

> Create a function to compute the values of the TBFLAG_A64 bits
> that will be cached.  For now, the env->hflags variable is not
> used, and the results are fed back to cpu_get_tb_cpu_state.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/helper.c | 131 +++++++++++++++++++++++---------------------
>  1 file changed, 69 insertions(+), 62 deletions(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index f2c6419369..02cb43cf58 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11032,6 +11032,71 @@ static uint32_t rebuild_hflags_common(CPUARMState *env, int fp_el,
>      return flags;
>  }
>
> +static uint32_t rebuild_hflags_a64(CPUARMState *env, int el, int fp_el,
> +                                   ARMMMUIdx mmu_idx)
> +{
<snip>
> +
> +    if (cpu_isar_feature(aa64_bti, env_archcpu(env))) {
> +        /* Note that SCTLR_EL[23].BT == SCTLR_BT1.  */
> +        if (sctlr & (el == 0 ? SCTLR_BT0 : SCTLR_BT1)) {
> +            flags = FIELD_DP32(flags, TBFLAG_A64, BT, 1);
> +        }
> +    }
> +
> +    return rebuild_hflags_common(env, fp_el, mmu_idx, flags);
> +}
> +
>  void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>                            target_ulong *cs_base, uint32_t *pflags)
>  {
> @@ -11041,67 +11106,9 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>      uint32_t flags = 0;
>
>      if (is_a64(env)) {
<snip>
> -
> -        if (cpu_isar_feature(aa64_bti, cpu)) {
> -            /* Note that SCTLR_EL[23].BT == SCTLR_BT1.  */
> -            if (sctlr & (current_el == 0 ? SCTLR_BT0 : SCTLR_BT1)) {
> -                flags = FIELD_DP32(flags, TBFLAG_A64, BT, 1);
> -            }
> +        flags = rebuild_hflags_a64(env, current_el, fp_el, mmu_idx);
> +        if (cpu_isar_feature(aa64_bti, env_archcpu(env))) {
>              flags = FIELD_DP32(flags, TBFLAG_A64, BTYPE, env->btype);

It seems off to only hoist part of the BTI flag check into the helper,
was it just missed or is there a reason? If so it could probably do with
an additional comment.

>          }
>      } else {
> @@ -11121,9 +11128,9 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>              flags = FIELD_DP32(flags, TBFLAG_A32,
>                                 XSCALE_CPAR, env->cp15.c15_cpar);
>          }
> -    }
>
> -    flags = rebuild_hflags_common(env, fp_el, mmu_idx, flags);
> +        flags = rebuild_hflags_common(env, fp_el, mmu_idx, flags);
> +    }
>
>      /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine
>       * states defined in the ARM ARM for software singlestep:


--
Alex Bennée