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

[Peter Maydell]

On 16 May 2014 11:01, 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.

Last time I looked at this register I decided that we
were OK for v8 (since v8 RES0 behaviour allows reads-as-written
as a possible implementation choice) and that I didn't care about
whether we were precisely architecturally correct for v7.
However if you want to tighten up our handling for v7 I
don't object.

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

This looks wrong -- you're not accounting for the ASEDIS and
TRCDIS bits, or D32DIS in v7. If you're deliberately taking
advantage of the IMPDEF choice to have these bits all be RAZ/WI
you should document that. (But note that v8 doesn't allow this
liberty for TRCDIS.)

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

This is all pointless because VFP instructions don't
get decoded via disas_coproc_insn(). So you'll only get
here for coprocessors which aren't 10, 11, 14 or 15,
and we know that there aren't any of those, so we'll
just undef anyway because the reginfo lookup will fail.

Honouring the CPACR bits for VFP/Neon access is done via
the CPACR_FPEN bit in the TB flags. (In fact we should
really stop flushing the tb for CPACR writes, since all
the bits are ignored except ones which we deal with via
TB flags anyway.)

thanks
-- PMM