Re: [Qemu-devel] [PATCH v5 09/33] target-arm: add macros to access banked registers

[Greg Bellows]

Right, we need the macros to do string concatenation so they have to be macros.  That combination occurs 3 times from a quick look.  I agree that it may be cumbersome to try and invent a name.

Anything to do on this?

On 6 October 2014 11:09, Peter Maydell <address@hidden> wrote:

On 30 September 2014 22:49, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ----------
> 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.
> ---
>  target-arm/cpu.h | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 601f8fe..c58fdf5 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -807,6 +807,42 @@ static inline bool arm_el_is_aa64(CPUARMState *env, int el)
>      return arm_feature(env, ARM_FEATURE_AARCH64);
>  }
>
> +/* Macro 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.
> + */
> +#define USE_SECURE_REG(_env) (                                   \
> +                        arm_feature((_env), ARM_FEATURE_EL3) &&    \
> +                        !arm_el_is_aa64((_env), 3) &&              \
> +                        !((_env)->cp15.scr_el3 & SCR_NS))

Better to use an inline function for this rather than a macro,
I think.

> +
> +/* 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))
> +

...though these all have to be macros because of the regname handling.

(Do we use "!arm_el_is_aa64((env), 3) && arm_is_secure(env)"
often enough to make it worth a utility function? I can't
think of a good name though, so maybe not...)

thanks
-- PMM