On 6 October 2014 13:10, Sergey Fedorov <serge.f...@gmail.com> wrote:
> On 06.10.2014 09:13, Peter Maydell wrote:
> > On 30 September 2014 22:49, Greg Bellows <greg.bell...@linaro.org>
> wrote:
> >> From: Sergey Fedorov <s.fedo...@samsung.com>
> >>
> >> This patch is based on idea found in patch at
> >> git://github.com/jowinter/qemu-trustzone.git
> >> f3d955c6c0ed8c46bc0eb10b634201032a651dd2 by
> >> Johannes Winter <johannes.win...@iaik.tugraz.at>.
> >>
> >> 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 <s.fedo...@samsung.com>
> >> Signed-off-by: Fabian Aggeler <aggel...@ethz.ch>
> >> Signed-off-by: Greg Bellows <greg.bell...@linaro.org>
> >>
> >> ----------
> >> 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
>
>
