| Message ID | 1413910544-20150-10-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: Fabian Aggeler <aggelerf@ethz.ch> > > If EL3 is in AArch32 state certain cp registers are banked (secure and > non-secure instance). When reading or writing to coprocessor registers > the following macros can be used. > > - A32_BANKED macros are used for choosing the banked register based on provided > input security argument. This macro is used to choose the bank during > translation of MRC/MCR instructions that are dependent on something other > than the current secure state. > - A32_BANKED_CURRENT macros are used for choosing the banked register based on > current secure state. This is NOT to be used for choosing the bank used > during translation as it breaks monitor mode. > > If EL3 is operating in AArch64 state coprocessor registers are not > banked anymore. The macros use the non-secure instance (_ns) in this > case, which is architecturally mapped to the AArch64 EL register. > > 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> > > ========== Can you make these separators the standard '---', please? Otherwise when I apply these patches I'll have to go through them all manually editing the version changes out... > v5 -> v6 > - Converted macro USE_SECURE_REG() into inlince function use_secure_reg() > - Globally replace Aarch# with AArch# > > v4 -> v5 > - Cleaned-up macros to try and alleviate misuse. Made A32_BANKED macros take > secure arg indicator rather than relying on USE_SECURE_REG. Incorporated the > A32_BANKED macros into the A32_BANKED_CURRENT. CURRENT is now the only one > that automatically chooses based on current secure state. ...and as you can see your own patch tooling isn't handling them right, because you now have two signed-off-by lines :-) > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > --- > target-arm/cpu.h | 40 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 40 insertions(+) > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index 1a564b9..b48b81a 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -817,6 +817,46 @@ static inline bool arm_el_is_aa64(CPUARMState *env, int el) > return arm_feature(env, ARM_FEATURE_AARCH64); > } > > +/* Function for determing whether to use the secure or non-secure bank of a CP > + * register. When EL3 is operating in AArch32 state, the NS-bit determines > + * whether the secure instance of a cp-register should be used. > + */ > +static inline bool use_secure_reg(CPUARMState *env) > +{ > + bool ret = (arm_feature(env, ARM_FEATURE_EL3) && > + !arm_el_is_aa64(env, 3) && > + !(env->cp15.scr_el3 & SCR_NS)); > + > + return ret; > +} This function is a bit misnamed, and it's actually only used for setting the TBFLAG_NS bit, so: * name it access_secure_reg() * change the comment: /* Function for determing whether guest cp register reads and writes should * access the secure or non-secure bank of a cp register. When EL3 is * operating in AArch32 state, the NS-bit determines whether the secure * instance of a cp register should be used. When EL3 is AArch64 (or if * it doesn't exist at all) then there is no register banking, and all * accesses are to the non-secure version. */ * move it into patch 10 (the one which adds the TBFLAG_NS bit). If you do that, then you can mark what's left in this patch with my Reviewed-by tag. > + > +/* Macros for accessing a specified CP register bank */ > +#define A32_BANKED_REG_GET(_env, _regname, _secure) \ > + ((_secure) ? (_env)->cp15._regname##_s : (_env)->cp15._regname##_ns) > + > +#define A32_BANKED_REG_SET(_env, _regname, _secure, _val) \ > + do { \ > + if (_secure) { \ > + (_env)->cp15._regname##_s = (_val); \ > + } else { \ > + (_env)->cp15._regname##_ns = (_val); \ > + } \ > + } while (0) > + > +/* Macros for automatically accessing a specific CP register bank depending on > + * the current secure state of the system. These macros are not intended for > + * supporting instruction translation reads/writes as these are dependent > + * solely on the SCR.NS bit and not the mode. > + */ > +#define A32_BANKED_CURRENT_REG_GET(_env, _regname) \ > + A32_BANKED_REG_GET((_env), _regname, \ > + ((!arm_el_is_aa64((_env), 3) && arm_is_secure(_env)))) > + > +#define A32_BANKED_CURRENT_REG_SET(_env, _regname, _val) \ > + A32_BANKED_REG_SET((_env), _regname, \ > + ((!arm_el_is_aa64((_env), 3) && arm_is_secure(_env))), \ > + (_val)) > + > void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf); > unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx); > > -- > 1.8.3.2 thanks -- PMM
On 24 October 2014 11:37, Peter Maydell <peter.maydell@linaro.org> wrote: > On 21 October 2014 17:55, Greg Bellows <greg.bellows@linaro.org> wrote: > > From: Fabian Aggeler <aggelerf@ethz.ch> > > > > If EL3 is in AArch32 state certain cp registers are banked (secure and > > non-secure instance). When reading or writing to coprocessor registers > > the following macros can be used. > > > > - A32_BANKED macros are used for choosing the banked register based on > provided > > input security argument. This macro is used to choose the bank during > > translation of MRC/MCR instructions that are dependent on something > other > > than the current secure state. > > - A32_BANKED_CURRENT macros are used for choosing the banked register > based on > > current secure state. This is NOT to be used for choosing the bank > used > > during translation as it breaks monitor mode. > > > > If EL3 is operating in AArch64 state coprocessor registers are not > > banked anymore. The macros use the non-secure instance (_ns) in this > > case, which is architecturally mapped to the AArch64 EL register. > > > > 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> > > > > ========== > > Can you make these separators the standard '---', please? Otherwise > when I apply these patches I'll have to go through them all manually > editing the version changes out... > I did not know there were standard separators. I have fixed this in all the patches. > > v5 -> v6 > > - Converted macro USE_SECURE_REG() into inlince function use_secure_reg() > > - Globally replace Aarch# with AArch# > > > > v4 -> v5 > > - Cleaned-up macros to try and alleviate misuse. Made A32_BANKED macros > take > > secure arg indicator rather than relying on USE_SECURE_REG. > Incorporated the > > A32_BANKED macros into the A32_BANKED_CURRENT. CURRENT is now the > only one > > that automatically chooses based on current secure state. > > ...and as you can see your own patch tooling isn't handling them > right, because you now have two signed-off-by lines :-) > > > Signed-off-by: Greg Bellows <greg.bellows@linaro.org> > > --- > > target-arm/cpu.h | 40 ++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 40 insertions(+) > > > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > > index 1a564b9..b48b81a 100644 > > --- a/target-arm/cpu.h > > +++ b/target-arm/cpu.h > > @@ -817,6 +817,46 @@ static inline bool arm_el_is_aa64(CPUARMState *env, > int el) > > return arm_feature(env, ARM_FEATURE_AARCH64); > > } > > > > +/* Function for determing whether to use the secure or non-secure bank > of a CP > > + * register. When EL3 is operating in AArch32 state, the NS-bit > determines > > + * whether the secure instance of a cp-register should be used. > > + */ > > +static inline bool use_secure_reg(CPUARMState *env) > > +{ > > + bool ret = (arm_feature(env, ARM_FEATURE_EL3) && > > + !arm_el_is_aa64(env, 3) && > > + !(env->cp15.scr_el3 & SCR_NS)); > > + > > + return ret; > > +} > > This function is a bit misnamed, and it's actually only used for > setting the TBFLAG_NS bit, so: > * name it access_secure_reg() > * change the comment: > /* Function for determing whether guest cp register reads and writes > should > * access the secure or non-secure bank of a cp register. When EL3 is > * operating in AArch32 state, the NS-bit determines whether the secure > * instance of a cp register should be used. When EL3 is AArch64 (or if > * it doesn't exist at all) then there is no register banking, and all > * accesses are to the non-secure version. > */ > * move it into patch 10 (the one which adds the TBFLAG_NS bit). > > If you do that, then you can mark what's left in this patch > with my Reviewed-by tag. > Done in v8 > > > + > > +/* Macros for accessing a specified CP register bank */ > > +#define A32_BANKED_REG_GET(_env, _regname, _secure) \ > > + ((_secure) ? (_env)->cp15._regname##_s : (_env)->cp15._regname##_ns) > > + > > +#define A32_BANKED_REG_SET(_env, _regname, _secure, _val) \ > > + do { \ > > + if (_secure) { \ > > + (_env)->cp15._regname##_s = (_val); \ > > + } else { \ > > + (_env)->cp15._regname##_ns = (_val); \ > > + } \ > > + } while (0) > > + > > +/* Macros for automatically accessing a specific CP register bank > depending on > > + * the current secure state of the system. These macros are not > intended for > > + * supporting instruction translation reads/writes as these are > dependent > > + * solely on the SCR.NS bit and not the mode. > > + */ > > +#define A32_BANKED_CURRENT_REG_GET(_env, _regname) \ > > + A32_BANKED_REG_GET((_env), _regname, \ > > + ((!arm_el_is_aa64((_env), 3) && > arm_is_secure(_env)))) > > + > > +#define A32_BANKED_CURRENT_REG_SET(_env, _regname, _val) > \ > > + A32_BANKED_REG_SET((_env), _regname, > \ > > + ((!arm_el_is_aa64((_env), 3) && > arm_is_secure(_env))), \ > > + (_val)) > > + > > void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf); > > unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx); > > > > -- > > 1.8.3.2 > > thanks > -- PMM >
========== v5 -> v6 - Converted macro USE_SECURE_REG() into inlince function use_secure_reg() - Globally replace Aarch# with AArch# v4 -> v5 - Cleaned-up macros to try and alleviate misuse. Made A32_BANKED macros take secure arg indicator rather than relying on USE_SECURE_REG. Incorporated the A32_BANKED macros into the A32_BANKED_CURRENT. CURRENT is now the only one that automatically chooses based on current secure state. Signed-off-by: Greg Bellows <greg.bellows@linaro.org> --- target-arm/cpu.h | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/target-arm/cpu.h b/target-arm/cpu.h index 1a564b9..b48b81a 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -817,6 +817,46 @@ static inline bool arm_el_is_aa64(CPUARMState *env, int el) return arm_feature(env, ARM_FEATURE_AARCH64); } +/* Function for determing whether to use the secure or non-secure bank of a CP + * register. When EL3 is operating in AArch32 state, the NS-bit determines + * whether the secure instance of a cp-register should be used. + */ +static inline bool use_secure_reg(CPUARMState *env) +{ + bool ret = (arm_feature(env, ARM_FEATURE_EL3) && + !arm_el_is_aa64(env, 3) && + !(env->cp15.scr_el3 & SCR_NS)); + + return ret; +} + +/* Macros for accessing a specified CP register bank */ +#define A32_BANKED_REG_GET(_env, _regname, _secure) \ + ((_secure) ? (_env)->cp15._regname##_s : (_env)->cp15._regname##_ns) + +#define A32_BANKED_REG_SET(_env, _regname, _secure, _val) \ + do { \ + if (_secure) { \ + (_env)->cp15._regname##_s = (_val); \ + } else { \ + (_env)->cp15._regname##_ns = (_val); \ + } \ + } while (0) + +/* Macros for automatically accessing a specific CP register bank depending on + * the current secure state of the system. These macros are not intended for + * supporting instruction translation reads/writes as these are dependent + * solely on the SCR.NS bit and not the mode. + */ +#define A32_BANKED_CURRENT_REG_GET(_env, _regname) \ + A32_BANKED_REG_GET((_env), _regname, \ + ((!arm_el_is_aa64((_env), 3) && arm_is_secure(_env)))) + +#define A32_BANKED_CURRENT_REG_SET(_env, _regname, _val) \ + A32_BANKED_REG_SET((_env), _regname, \ + ((!arm_el_is_aa64((_env), 3) && arm_is_secure(_env))), \ + (_val)) + void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf); unsigned int arm_excp_target_el(CPUState *cs, unsigned int excp_idx);