qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] target-arm: implement CPACR register logic


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH] target-arm: implement CPACR register logic
Date: Fri, 16 May 2014 17:14:11 +0000

On Fri, May 16, 2014 at 10:01 AM, Fabian Aggeler <address@hidden> wrote:
> From: Sergey Fedorov <address@hidden>
>
> CPACR register allows to control access rights to coprocessor 0-13
> interfaces. Bits corresponding to unimplemented coprocessors should be
> RAZ/WI. QEMU implements only VFP coprocessor on ARMv6+ targets. So only
> cp10 & cp11 bits are writable.
>
> Signed-off-by: Sergey Fedorov <address@hidden>
> Signed-off-by: Fabian Aggeler <address@hidden>
> ---
>
> This patch was previously part of the Security Extensions patchset and
> got separated because it is not Sec-Ext specific.
> In addition to a style fix of the comment I added qemu_log_mask() as
> suggested.
>
>  target-arm/helper.c    |  6 ++++++
>  target-arm/translate.c | 29 ++++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 417161e..3cb1ac0 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -477,6 +477,12 @@ static const ARMCPRegInfo not_v7_cp_reginfo[] = {
>  static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                          uint64_t value)
>  {
> +    uint32_t mask = 0;
> +
> +    if (arm_feature(env, ARM_FEATURE_VFP)) {
> +        mask |= 0x00f00000; /* VFP coprocessor: cp10 & cp11 */
> +    }
> +    value &= mask;
>      if (env->cp15.c1_coproc != value) {
>          env->cp15.c1_coproc = value;
>          /* ??? Is this safe when called from within a TB?  */
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index a4d920b..9b298d0 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -6864,9 +6864,32 @@ static int disas_coproc_insn(CPUARMState * env, 
> DisasContext *s, uint32_t insn)
>      const ARMCPRegInfo *ri;
>
>      cpnum = (insn >> 8) & 0xf;
> -    if (arm_feature(env, ARM_FEATURE_XSCALE)
> -           && ((env->cp15.c15_cpar ^ 0x3fff) & (1 << cpnum)))
> -       return 1;
> +    if (cpnum < 14) {
> +        if (arm_feature(env, ARM_FEATURE_XSCALE)) {
> +            if (~env->cp15.c15_cpar & (1 << cpnum)) {
> +                return 1;
> +            }
> +        } else {
> +            /* Bits [20:21] of CPACR control access to cp10
> +             * Bits [23:22] of CPACR control access to cp11
> +             */

Thinking about this a little more, there's nothing CP10/CP11 specific
about this logic. The comment should be more general than this (or
dropped). It would go stale if someone patched in a new CP. Perhaps
add a "..." to indicate that you are illustrating a repeating pattern

+            /* Bits [20:21] of CPACR control access to cp10
+             * Bits [23:22] of CPACR control access to cp11
+             * ...
+             */

With droppage or comment generalisation:

Reviewed-by: Peter Crosthwaite <address@hidden>

> +            switch ((env->cp15.c1_coproc >> (cpnum * 2)) & 3) {
> +            case 0: /* access denied */
> +                return 1;
> +            case 1: /* privileged mode access only */
> +                if (IS_USER(s)) {
> +                    return 1;
> +                }
> +                break;
> +            case 2: /* reserved */
> +                qemu_log_mask(LOG_GUEST_ERROR, "CPACR bits for cp%d set to "
> +                                               "0b10 (unpredictable)\n", 
> cpnum);
> +                return 1;
> +            case 3: /* privileged and user mode access */
> +                break;
> +            }
> +        }
> +    }
>
>      /* First check for coprocessor space used for actual instructions */
>      switch (cpnum) {
> --
> 1.8.3.2
>
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]