On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> Several of the system registers handled via the ARMCPRegInfo
> mechanism have access trap control bits controlling whether the
> registers are accessible to lower privilege levels. Replace
> the existing mechanism (allowing the read and write functions
> to return EXCP_UDEF if access is denied) with a dedicated
> "check access rights" function pointer in the ARMCPRegInfo.
> This will allow us to simplify some of the register definitions,
> which no longer need read/write functions purely to handle
> the access checks.
>
> We take the opportunity to define the return value from the
> access checking function in a way that allows us to set the
> correct exception syndrome information for exceptions taken
> to AArch64 (which may need to distinguish access failures due
> to a configurable trap or enable from other kinds of access
> failure).
>
> This commit defines the new mechanism but does not move any
> of the registers across to use it.
>
> Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
Reviewed-by: Peter Crosthwaite <peter.crosthwa...@xilinx.com>
> ---
> target-arm/cpu.h | 29 +++++++++++++++++++++++++----
> target-arm/helper.h | 1 +
> target-arm/op_helper.c | 18 ++++++++++++++++++
> target-arm/translate-a64.c | 11 +++++++++++
> target-arm/translate.c | 11 +++++++++++
> 5 files changed, 66 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index e66d464..30b1a1c 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -812,14 +812,29 @@ static inline int arm_current_pl(CPUARMState *env)
>
> typedef struct ARMCPRegInfo ARMCPRegInfo;
>
> -/* Access functions for coprocessor registers. These should return
> - * 0 on success, or one of the EXCP_* constants if access should cause
> - * an exception (in which case *value is not written).
> - */
> +typedef enum CPAccessResult {
> + /* Access is permitted */
> + CP_ACCESS_OK = 0,
> + /* Access fails due to a configurable trap or enable which would
> + * result in a categorized exception syndrome giving information about
> + * the failing instruction (ie syndrome category 0x3, 0x4, 0x5, 0x6,
> + * 0xc or 0x18).
> + */
> + CP_ACCESS_TRAP = 1,
> + /* Access fails and results in an exception syndrome 0x0
> ("uncategorized").
> + * Note that this is not a catch-all case -- the set of cases which may
> + * result in this failure is specifically defined by the architecture.
> + */
> + CP_ACCESS_TRAP_UNCATEGORIZED = 2,
> +} CPAccessResult;
> +
> +/* Access functions for coprocessor registers. These should always succeed.
> */
> typedef int CPReadFn(CPUARMState *env, const ARMCPRegInfo *opaque,
> uint64_t *value);
> typedef int CPWriteFn(CPUARMState *env, const ARMCPRegInfo *opaque,
> uint64_t value);
> +/* Access permission check functions for coprocessor registers. */
> +typedef CPAccessResult CPAccessFn(CPUARMState *env, const ARMCPRegInfo
> *opaque);
> /* Hook function for register reset */
> typedef void CPResetFn(CPUARMState *env, const ARMCPRegInfo *opaque);
>
> @@ -873,6 +888,12 @@ struct ARMCPRegInfo {
> * 2. both readfn and writefn are specified
> */
> ptrdiff_t fieldoffset; /* offsetof(CPUARMState, field) */
> + /* Function for making any access checks for this register in addition to
> + * those specified by the 'access' permissions bits. If NULL, no extra
> + * checks required. The access check is performed at runtime, not at
> + * translate time.
> + */
> + CPAccessFn *accessfn;
> /* Function for handling reads of this register. If NULL, then reads
> * will be done by loading from the offset into CPUARMState specified
> * by fieldoffset.
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index 93a27ce..1d83293 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -57,6 +57,7 @@ DEF_HELPER_1(cpsr_read, i32, env)
> DEF_HELPER_3(v7m_msr, void, env, i32, i32)
> DEF_HELPER_2(v7m_mrs, i32, env, i32)
>
> +DEF_HELPER_2(access_check_cp_reg, void, env, ptr)
> DEF_HELPER_3(set_cp_reg, void, env, ptr, i32)
> DEF_HELPER_2(get_cp_reg, i32, env, ptr)
> DEF_HELPER_3(set_cp_reg64, void, env, ptr, i64)
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index c812a9f..89b0978 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -273,6 +273,24 @@ void HELPER(set_user_reg)(CPUARMState *env, uint32_t
> regno, uint32_t val)
> }
> }
>
> +void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip)
> +{
> + const ARMCPRegInfo *ri = rip;
> + switch (ri->accessfn(env, ri)) {
> + case CP_ACCESS_OK:
> + return;
> + case CP_ACCESS_TRAP:
> + case CP_ACCESS_TRAP_UNCATEGORIZED:
> + /* These cases will eventually need to generate different
> + * syndrome information.
> + */
> + break;
> + default:
> + g_assert_not_reached();
> + }
> + raise_exception(env, EXCP_UDEF);
> +}
> +
> void HELPER(set_cp_reg)(CPUARMState *env, void *rip, uint32_t value)
> {
> const ARMCPRegInfo *ri = rip;
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index a942609..d90ffd1 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1210,6 +1210,17 @@ static void handle_sys(DisasContext *s, uint32_t insn,
> bool isread,
> return;
> }
>
> + if (ri->accessfn) {
> + /* Emit code to perform further access permissions checks at
> + * runtime; this may result in an exception.
> + */
> + TCGv_ptr tmpptr;
> + gen_a64_set_pc_im(s->pc - 4);
> + tmpptr = tcg_const_ptr(ri);
> + gen_helper_access_check_cp_reg(cpu_env, tmpptr);
> + tcg_temp_free_ptr(tmpptr);
> + }
> +
> /* Handle special cases first */
> switch (ri->type & ~(ARM_CP_FLAG_MASK & ~ARM_CP_SPECIAL)) {
> case ARM_CP_NOP:
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 45886f9..2713081 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -6798,6 +6798,17 @@ static int disas_coproc_insn(CPUARMState * env,
> DisasContext *s, uint32_t insn)
> return 1;
> }
>
> + if (ri->accessfn) {
> + /* Emit code to perform further access permissions checks at
> + * runtime; this may result in an exception.
> + */
> + TCGv_ptr tmpptr;
> + gen_set_pc_im(s, s->pc);
> + tmpptr = tcg_const_ptr(ri);
> + gen_helper_access_check_cp_reg(cpu_env, tmpptr);
> + tcg_temp_free_ptr(tmpptr);
> + }
> +
> /* Handle special cases first */
> switch (ri->type & ~(ARM_CP_FLAG_MASK & ~ARM_CP_SPECIAL)) {
> case ARM_CP_NOP:
> --
> 1.8.5
>
>
