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

[Greg Bellows]

On 6 October 2014 13:10, Sergey Fedorov <address@hidden> wrote:

On 06.10.2014 09:13, Peter Maydell wrote:
> On 30 September 2014 22:49, 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>.
>>
>> This flag prevents QEMU from executing TCG code generated for other CPU
>> security state. It also allows to generate different TCG code depending on
>> CPU secure state.
> This doesn't quite seem to line up with the code:
> the commit message says the flag is for the CPU's
> current security state, but the code is using the
> "which register bank" setting.

Right, the original patch used "!arm_is_secure(env)" as a condition for
setting this flag. But that was changed at some point without correcting
commit message.

 

Yeah, that was my bad.  This had to be fixed as code generated for monitor mode would always use secure banks.

Comment fixed in v6.

 

>
>> Signed-off-by: Sergey Fedorov <address@hidden>
>> Signed-off-by: Fabian Aggeler <address@hidden>
>> Signed-off-by: Greg Bellows <address@hidden>
>>
>> ----------
>> 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.
>> ---
>>  target-arm/cpu.h           | 14 ++++++++++++++
>>  target-arm/translate-a64.c |  1 +
>>  target-arm/translate.c     |  1 +
>>  target-arm/translate.h     |  1 +
>>  4 files changed, 17 insertions(+)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index c58fdf5..1700676 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -1528,6 +1528,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)
>> +#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
>> @@ -1538,6 +1540,8 @@ static inline bool arm_singlestep_active(CPUARMState *env)
>>  #define ARM_TBFLAG_AA64_SS_ACTIVE_MASK (1 << ARM_TBFLAG_AA64_SS_ACTIVE_SHIFT)
>>  #define ARM_TBFLAG_AA64_PSTATE_SS_SHIFT 4
>>  #define ARM_TBFLAG_AA64_PSTATE_SS_MASK (1 << ARM_TBFLAG_AA64_PSTATE_SS_SHIFT)
>> +#define ARM_TBFLAG_AA64_NS_SHIFT    5
>> +#define ARM_TBFLAG_AA64_NS_MASK     (1 << ARM_TBFLAG_AA64_NS_SHIFT)
>>
>>  /* some convenience accessor macros */
>>  #define ARM_TBFLAG_AARCH64_STATE(F) \
>> @@ -1572,6 +1576,10 @@ 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)
>> +#define ARM_TBFLAG_AA64_NS(F) \
>> +    (((F) & ARM_TBFLAG_AA64_NS_MASK) >> ARM_TBFLAG_AA64_NS_SHIFT)
>>
>>  static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>>                                          target_ulong *cs_base, int *flags)
>> @@ -1605,6 +1613,9 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>>                  *flags |= ARM_TBFLAG_AA64_PSTATE_SS_MASK;
>>              }
>>          }
>> +        if (!(USE_SECURE_REG(env))) {
>> +            *flags |= ARM_TBFLAG_AA64_NS_MASK;
>> +        }
> What's this for? If we're in AArch64 mode then we know that
> EL3 (if it exists) must also be AArch64, and so USE_SECURE_REG
> always returns false...

True, this was a leftover change from when it was a call to arm_is_secure(), which also was likely unneeded..  Plus if we are AA64 then we don't care about banks, so we can probably just remove the 64-bit macros.  

Removed from v6

 

>
>>      } else {
>>          int privmode;
>>          *pc = env->regs[15];
>> @@ -1621,6 +1632,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-a64.c b/target-arm/translate-a64.c
>> index f53dc0f..dfc8c58 100644
>> --- a/target-arm/translate-a64.c
>> +++ b/target-arm/translate-a64.c
>> @@ -10926,6 +10926,7 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>>  #if !defined(CONFIG_USER_ONLY)
>>      dc->user = (ARM_TBFLAG_AA64_EL(tb->flags) == 0);
>>  #endif
>> +    dc->ns = ARM_TBFLAG_AA64_NS(tb->flags);
>>      dc->cpacr_fpen = ARM_TBFLAG_AA64_FPEN(tb->flags);
>>      dc->vec_len = 0;
>>      dc->vec_stride = 0;
>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>> index 3f3ddfb..5e1d677 100644
>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -10958,6 +10958,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 85c6f9d..4f9892b 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;
> This could use a brief comment explaining what it indicates.

Commented added in v6

 

>
>>      bool cpacr_fpen; /* FP enabled via CPACR.FPEN */
>>      bool vfp_enabled; /* FP enabled via FPSCR.EN */
>>      int vec_len;
>> --
>> 1.8.3.2