From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43534) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XbCjv-0003yo-8V for qemu-devel@nongnu.org; Mon, 06 Oct 2014 14:10:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XbCjr-0008Bh-55 for qemu-devel@nongnu.org; Mon, 06 Oct 2014 14:10:31 -0400 Received: from mail-pd0-x22c.google.com ([2607:f8b0:400e:c02::22c]:42385) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XbCjq-0008B9-OE for qemu-devel@nongnu.org; Mon, 06 Oct 2014 14:10:27 -0400 Received: by mail-pd0-f172.google.com with SMTP id ft15so3697210pdb.3 for ; Mon, 06 Oct 2014 11:10:25 -0700 (PDT) Message-ID: <5432DB0E.9020608@gmail.com> Date: Mon, 06 Oct 2014 11:10:22 -0700 From: Sergey Fedorov MIME-Version: 1.0 References: <1412113785-21525-1-git-send-email-greg.bellows@linaro.org> <1412113785-21525-11-git-send-email-greg.bellows@linaro.org> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 10/33] target-arm: add non-secure Translation Block flag List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , Greg Bellows Cc: "Edgar E. Iglesias" , Sergey Fedorov , QEMU Developers , Fabian Aggeler On 06.10.2014 09:13, Peter Maydell wrote: > On 30 September 2014 22:49, Greg Bellows wrote: >> From: Sergey Fedorov >> >> This patch is based on idea found in patch at >> git://github.com/jowinter/qemu-trustzone.git >> f3d955c6c0ed8c46bc0eb10b634201032a651dd2 by >> Johannes Winter . >> >> 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. > >> Signed-off-by: Sergey Fedorov >> Signed-off-by: Fabian Aggeler >> Signed-off-by: Greg Bellows >> >> ---------- >> 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... > >> } 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. > >> bool cpacr_fpen; /* FP enabled via CPACR.FPEN */ >> bool vfp_enabled; /* FP enabled via FPSCR.EN */ >> int vec_len; >> -- >> 1.8.3.2