Re: [Qemu-devel] [PATCH v2 11/35] target-arm: Split cpreg access checks out from read/write functions

[Peter Crosthwaite]

On Sat, Feb 1, 2014 at 1:45 AM, Peter Maydell <address@hidden> 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 <address@hidden>

Reviewed-by: Peter Crosthwaite <address@hidden>

> ---
>  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
>
>