Re: [Qemu-devel] [PATCH v7 10/32] target-arm: add non-secure Translation Block flag

[Peter Maydell]

On 21 October 2014 17:55, Greg Bellows <address@hidden> wrote:
> From: Sergey Fedorov <address@hidden>
>
> This patch is based on idea found in patch at
> git://github.com/jowinter/qemu-trustzone.git
> f3d955c6c0ed8c46bc0eb10b634201032a651dd2 by
> Johannes Winter <address@hidden>.
>
> The TBFLAG captures the SCR NS secure state at the time when a TB is created 
> so
> the correct bank is accessed on system register accesses.  It also allows to
> generate different TCG code depending on CPU secure state.

This last sentence is wrong and should just be deleted.

> Signed-off-by: Sergey Fedorov <address@hidden>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ==========
>
> v5 -> v6
> - Removed 64-bit NS TBFLAG macros as they are not needed
> - Added comment on DisasContext ns field
> - Replaced use of USE_SECURE_REG with use_secure_reg
>
> v4 -> v5
> - Merge changes
> - Fixed issue where TB secure state flag was incorrectly being set based on
>   secure state rather than NS setting.  This caused an issue where monitor 
> mode
>   MRC/MCR accesses were always secure rather than being based on NS bit
>   setting.
> - Added separate 64/32 TB secure state flags
> - Unconditionalized the setting of the DC ns bit
> - Removed IS_NS macro and replaced with direct usage.
>
> Signed-off-by: Greg Bellows <address@hidden>
> ---
>  target-arm/cpu.h       | 7 +++++++
>  target-arm/translate.c | 1 +
>  target-arm/translate.h | 1 +
>  3 files changed, 9 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index b48b81a..e041437 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1551,6 +1551,8 @@ static inline bool arm_singlestep_active(CPUARMState 
> *env)
>   */
>  #define ARM_TBFLAG_XSCALE_CPAR_SHIFT 20
>  #define ARM_TBFLAG_XSCALE_CPAR_MASK (3 << ARM_TBFLAG_XSCALE_CPAR_SHIFT)

/* Indicates whether cp register reads and writes by guest code should access
 * the secure or nonsecure bank of banked registers; note that this is not
 * the same thing as the current security state of the processor!
 */

If you add this comment and move and rename the use_secure_reg()
function from the previous patch as I suggest, you can add my
reviewed-by tag.

> +#define ARM_TBFLAG_NS_SHIFT         22
> +#define ARM_TBFLAG_NS_MASK          (1 << ARM_TBFLAG_NS_SHIFT)
>
>  /* Bit usage when in AArch64 state */
>  #define ARM_TBFLAG_AA64_EL_SHIFT    0
> @@ -1595,6 +1597,8 @@ static inline bool arm_singlestep_active(CPUARMState 
> *env)
>      (((F) & ARM_TBFLAG_AA64_SS_ACTIVE_MASK) >> 
> ARM_TBFLAG_AA64_SS_ACTIVE_SHIFT)
>  #define ARM_TBFLAG_AA64_PSTATE_SS(F) \
>      (((F) & ARM_TBFLAG_AA64_PSTATE_SS_MASK) >> 
> ARM_TBFLAG_AA64_PSTATE_SS_SHIFT)
> +#define ARM_TBFLAG_NS(F) \
> +    (((F) & ARM_TBFLAG_NS_MASK) >> ARM_TBFLAG_NS_SHIFT)
>
>  static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>                                          target_ulong *cs_base, int *flags)
> @@ -1644,6 +1648,9 @@ static inline void cpu_get_tb_cpu_state(CPUARMState 
> *env, target_ulong *pc,
>          if (privmode) {
>              *flags |= ARM_TBFLAG_PRIV_MASK;
>          }
> +        if (!(use_secure_reg(env))) {
> +            *flags |= ARM_TBFLAG_NS_MASK;
> +        }
>          if (env->vfp.xregs[ARM_VFP_FPEXC] & (1 << 30)
>              || arm_el_is_aa64(env, 1)) {
>              *flags |= ARM_TBFLAG_VFPEN_MASK;
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 91958b6..aa17a20 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -11002,6 +11002,7 @@ static inline void 
> gen_intermediate_code_internal(ARMCPU *cpu,
>  #if !defined(CONFIG_USER_ONLY)
>      dc->user = (ARM_TBFLAG_PRIV(tb->flags) == 0);
>  #endif
> +    dc->ns = ARM_TBFLAG_NS(tb->flags);
>      dc->cpacr_fpen = ARM_TBFLAG_CPACR_FPEN(tb->flags);
>      dc->vfp_enabled = ARM_TBFLAG_VFPEN(tb->flags);
>      dc->vec_len = ARM_TBFLAG_VECLEN(tb->flags);
> diff --git a/target-arm/translate.h b/target-arm/translate.h
> index 41a9071..f6ee789 100644
> --- a/target-arm/translate.h
> +++ b/target-arm/translate.h
> @@ -20,6 +20,7 @@ typedef struct DisasContext {
>  #if !defined(CONFIG_USER_ONLY)
>      int user;
>  #endif
> +    bool ns;        /* Use non-secure CPREG bank on access */
>      bool cpacr_fpen; /* FP enabled via CPACR.FPEN */
>      bool vfp_enabled; /* FP enabled via FPSCR.EN */
>      int vec_len;
> --
> 1.8.3.2
>


thanks
-- PMM