Re: [Qemu-devel] [PATCH 1/3] target-arm: Infrastucture changes to enable handling of tagged address loading into PC

[Peter Maydell <peter.maydell@linaro.org>]

On 16 September 2016 at 10:34, Thomas Hanson <thomas.hanson@linaro.org> wrote:

This patch is mostly good; minor comments below.

> New arm_regime_tbi0() and arm_regime_tbi0() to extract the TBI values from
> the correct TCR for the current EL.
>
> New shift, mask and accessor macro definitions needed to add TBI flag bits
> to the TB flags field.
>
> cpu_get_tb_cpu_state() inserst the TBI values into 'flags' parameter
> so that they show up in the TB flags field.
>
> tbi0, tbi1 fields added to definition of DisasContext structure.

This is just describing the "what" of the change (which you
can find fairly easily by just looking at the patch). Commit
messages should concentrate on the "why" (and the 'body' part
should read OK standalone without having to read the 'subject
line' part too).

Put another way, this reads like a GCC changelog message :-)

> Signed-off-by: Thomas Hanson <thomas.hanson@linaro.org>
> ---
>  target-arm/cpu.h       | 20 ++++++++++++++++++--
>  target-arm/helper.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
>  target-arm/translate.h |  3 +++
>  3 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 76d824d..c2d6e75 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -2191,7 +2191,11 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
>  #define ARM_TBFLAG_BE_DATA_SHIFT    20
>  #define ARM_TBFLAG_BE_DATA_MASK     (1 << ARM_TBFLAG_BE_DATA_SHIFT)
>
> -/* Bit usage when in AArch64 state: currently we have no A64 specific bits */
> +/* Bit usage when in AArch64 state */
> +#define ARM_TBFLAG_TBI0_SHIFT 0        /* TBI0 for EL0/1 or TBI for EL2/3 */
> +#define ARM_TBFLAG_TBI0_MASK (0x1ull << ARM_TBFLAG_TBI0_SHIFT)
> +#define ARM_TBFLAG_TBI1_SHIFT 1        /* TBI1 for EL0/1  */
> +#define ARM_TBFLAG_TBI1_MASK (0x1ull << ARM_TBFLAG_TBI1_SHIFT)
>
>  /* some convenience accessor macros */
>  #define ARM_TBFLAG_AARCH64_STATE(F) \
> @@ -2222,6 +2226,10 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
>      (((F) & ARM_TBFLAG_NS_MASK) >> ARM_TBFLAG_NS_SHIFT)
>  #define ARM_TBFLAG_BE_DATA(F) \
>      (((F) & ARM_TBFLAG_BE_DATA_MASK) >> ARM_TBFLAG_BE_DATA_SHIFT)
> +#define ARM_TBFLAG_TBI0(F) \
> +    (((F) & ARM_TBFLAG_TBI0_MASK) >> ARM_TBFLAG_TBI0_SHIFT)
> +#define ARM_TBFLAG_TBI1(F) \
> +    (((F) & ARM_TBFLAG_TBI1_MASK) >> ARM_TBFLAG_TBI1_SHIFT)
>
>  static inline bool bswap_code(bool sctlr_b)
>  {
> @@ -2319,12 +2327,19 @@ static inline bool arm_cpu_bswap_data(CPUARMState *env)
>  }
>  #endif
>
> +long arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx);
> +long arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx);

Why "long" (very rarely the right type in CPU emulation code
because its width depends on the host CPU architecture) ?

New global-scope function prototypes should have a standard-form
doc comment (we have a lot without, but I'm trying to improve
by the ratchet mechanism of insisting on prototypes for newly
added or modified ones).

>  static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>                                          target_ulong *cs_base, uint32_t *flags)
>  {
> +    ARMMMUIdx mmu_idx = cpu_mmu_index(env, false);
>      if (is_a64(env)) {
>          *pc = env->pc;
>          *flags = ARM_TBFLAG_AARCH64_STATE_MASK;
> +        /* Get control bits for tagged addresses */
> +        *flags |= (arm_regime_tbi0(env, mmu_idx) << ARM_TBFLAG_TBI0_SHIFT);
> +        *flags |= (arm_regime_tbi1(env, mmu_idx) << ARM_TBFLAG_TBI1_SHIFT);
>      } else {
>          *pc = env->regs[15];
>          *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT)
> @@ -2343,7 +2358,8 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>                     << ARM_TBFLAG_XSCALE_CPAR_SHIFT);
>      }
>
> -    *flags |= (cpu_mmu_index(env, false) << ARM_TBFLAG_MMUIDX_SHIFT);
> +    *flags |= (mmu_idx << ARM_TBFLAG_MMUIDX_SHIFT);
> +
>      /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine
>       * states defined in the ARM ARM for software singlestep:
>       *  SS_ACTIVE   PSTATE.SS   State
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index bdb842c..526306e 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6720,6 +6720,48 @@ static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
>      return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
>  }
>
> +/* Returns TBI0 value for current regime el */
> +long arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    TCR    *tcr;

Weird spacing.

> +    uint32_t el;
> +
> +    /* regime_el fails for mmu_idx = ARMMMUIdx_S12NSE0 or ARMMMUIdx_S12NSE1 */

Better comment:
   /* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
    * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
    */

> +    if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> +        mmu_idx += ARMMMUIdx_S1NSE0;
> +    }
> +
> +    tcr = regime_tcr(env, mmu_idx);
> +    el = regime_el(env, mmu_idx);
> +
> +    if (el > 1) {
> +        return extract64(tcr->raw_tcr, 20, 1);
> +    } else {
> +        return extract64(tcr->raw_tcr, 37, 1);
> +    }
> +}
> +
> +/* Returns TBI0 value for current regime el */

Comment doesn't match function name :-)

> +long arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    TCR    *tcr;
> +    uint32_t el;
> +
> +    /* regime_el fails for mmu_idx = ARMMMUIdx_S12NSE0 or ARMMMUIdx_S12NSE1 */
> +    if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
> +        mmu_idx += ARMMMUIdx_S1NSE0;
> +    }
> +
> +    tcr = regime_tcr(env, mmu_idx);
> +    el = regime_el(env, mmu_idx);
> +
> +    if (el > 1) {
> +        return 0;
> +    } else {
> +        return extract64(tcr->raw_tcr, 38, 1);
> +    }
> +}
> +
>  /* Return the TTBR associated with this translation regime */
>  static inline uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
>                                     int ttbrn)
> diff --git a/target-arm/translate.h b/target-arm/translate.h
> index dbd7ac8..5dfd394 100644
> --- a/target-arm/translate.h
> +++ b/target-arm/translate.h
> @@ -22,6 +22,8 @@ typedef struct DisasContext {
>      int user;
>  #endif
>      ARMMMUIdx mmu_idx; /* MMU index to use for normal loads/stores */
> +    bool tbi0;         /* TBI0 for EL0/1 or TBI for EL2/3 */
> +    bool tbi1;         /* TBI1 for EL0/1  */

Say explicitly "or 0 for EL2/3" ?

(Also stray extra space at end.)

>      bool ns;        /* Use non-secure CPREG bank on access */
>      int fp_excp_el; /* FP exception EL or 0 if enabled */
>      /* Flag indicating that exceptions from secure mode are routed to EL3. */
> @@ -127,6 +129,7 @@ static inline int default_exception_el(DisasContext *s)
>  void a64_translate_init(void);
>  void gen_intermediate_code_a64(ARMCPU *cpu, TranslationBlock *tb);
>  void gen_a64_set_pc_im(uint64_t val);
> +void gen_a64_set_pc_reg(DisasContext *s, unsigned int rn);

This should be in a different patch in the series I think.

>  void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
>                              fprintf_function cpu_fprintf, int flags);
>  #else
> --
> 1.9.1

thanks
-- PMM