| Message ID | 1413910544-20150-11-git-send-email-greg.bellows@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 21 October 2014 17:55, Greg Bellows <greg.bellows@linaro.org> wrote: > From: Sergey Fedorov <s.fedorov@samsung.com> > > This patch is based on idea found in patch at > git://github.com/jowinter/qemu-trustzone.git > f3d955c6c0ed8c46bc0eb10b634201032a651dd2 by > Johannes Winter <johannes.winter@iaik.tugraz.at>. > > 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 <s.fedorov@samsung.com> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > ========== > > 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 <greg.bellows@linaro.org> > --- > 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
On 24 October 2014 11:40, Peter Maydell <peter.maydell@linaro.org> wrote: > On 21 October 2014 17:55, Greg Bellows <greg.bellows@linaro.org> wrote: > > From: Sergey Fedorov <s.fedorov@samsung.com> > > > > This patch is based on idea found in patch at > > git://github.com/jowinter/qemu-trustzone.git > > f3d955c6c0ed8c46bc0eb10b634201032a651dd2 by > > Johannes Winter <johannes.winter@iaik.tugraz.at>. > > > > 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. > Done. > > > Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> > > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch> > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > > > ========== > > > > 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 <greg.bellows@linaro.org> > > --- > > 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. > > Function moved and renamed and comment added in v8. > > +#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 >
========== 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 <greg.bellows@linaro.org> --- 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) +#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;