Re: [Qemu-devel] [PATCH v2 7/9] arm: Track M profile handler mode state in TB flags

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

Hi Peter,

On 04/10/2017 02:13 PM, Peter Maydell wrote:
> For M profile exception-return handling we'd like to generate different
> code for some instructions depending on whether we are in Handler
> mode or Thread mode. This isn't the same as "are we privileged
> or user", so we need an extra bit in the TB flags to distinguish.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h       | 9 +++++++++
>  target/arm/translate.h | 1 +
>  target/arm/translate.c | 1 +
>  3 files changed, 11 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index e6f05e2..7d26357 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2291,6 +2291,9 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
>  #define ARM_TBFLAG_NS_MASK          (1 << ARM_TBFLAG_NS_SHIFT)
>  #define ARM_TBFLAG_BE_DATA_SHIFT    20
>  #define ARM_TBFLAG_BE_DATA_MASK     (1 << ARM_TBFLAG_BE_DATA_SHIFT)
> +/* For M profile only, Handler (ie not Thread) mode */
> +#define ARM_TBFLAG_HANDLER_SHIFT    21
> +#define ARM_TBFLAG_HANDLER_MASK     (1 << ARM_TBFLAG_HANDLER_SHIFT)
>
>  /* Bit usage when in AArch64 state */
>  #define ARM_TBFLAG_TBI0_SHIFT 0        /* TBI0 for EL0/1 or TBI for EL2/3 */
> @@ -2327,6 +2330,8 @@ 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_HANDLER(F) \
> +    (((F) & ARM_TBFLAG_HANDLER_MASK) >> ARM_TBFLAG_HANDLER_SHIFT)
>  #define ARM_TBFLAG_TBI0(F) \
>      (((F) & ARM_TBFLAG_TBI0_MASK) >> ARM_TBFLAG_TBI0_SHIFT)
>  #define ARM_TBFLAG_TBI1(F) \
> @@ -2517,6 +2522,10 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>      }
>      *flags |= fp_exception_el(env) << ARM_TBFLAG_FPEXC_EL_SHIFT;
>
> +    if (env->v7m.exception != 0) {
> +        *flags |= ARM_TBFLAG_HANDLER_MASK;
> +    }
> +
>      *cs_base = 0;
>  }
>
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index abb0760..f955174 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -31,6 +31,7 @@ typedef struct DisasContext {
>      bool vfp_enabled; /* FP enabled via FPSCR.EN */
>      int vec_len;
>      int vec_stride;
> +    bool handler; /* v7M Handler mode */

What about a more descriptive name like 'is_v7m_handler_mode' or 
'v7m_mode_handler'? It makes your next patch easier to understand:

     if (s->handler && arm_dc_feature(s, ARM_FEATURE_M)) {

>      /* Immediate value in AArch32 SVC insn; must be set if is_jmp == DISAS_SWI
>       * so that top level loop can generate correct syndrome information.
>       */
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index f28c4ca..f980cda 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11780,6 +11780,7 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
>      dc->vec_len = ARM_TBFLAG_VECLEN(tb->flags);
>      dc->vec_stride = ARM_TBFLAG_VECSTRIDE(tb->flags);
>      dc->c15_cpar = ARM_TBFLAG_XSCALE_CPAR(tb->flags);
> +    dc->handler = ARM_TBFLAG_HANDLER(tb->flags);
>      dc->cp_regs = cpu->cp_regs;
>      dc->features = env->features;
>
>