On 24 October 2014 11:37, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 21 October 2014 17:55, Greg Bellows <greg.bell...@linaro.org> wrote:
> > From: Fabian Aggeler <aggel...@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.fedo...@samsung.com>
> > Signed-off-by: Fabian Aggeler <aggel...@ethz.ch>
> > Signed-off-by: Greg Bellows <greg.bell...@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.bell...@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
>
