Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters

[Aurelien Jarno]

On Thu, Apr 21, 2011 at 05:01:48PM +0100, Peter Maydell wrote:
> Newer Linux kernels assume the existence of the performance counter
> cp15 registers. Provide a minimal implementation of these registers.
> We support no events. This should be compliant with the ARM ARM,
> except that we don't implement the cycle counter.
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> This is the last fix required to be able to boot a stock Linaro
> versatile express image on upstream QEMU...
> 
>  target-arm/cpu.h       |    8 ++-
>  target-arm/helper.c    |  159 
> ++++++++++++++++++++++++++++++++++++++++++++----
>  target-arm/machine.c   |   12 ++++
>  target-arm/translate.c |   20 ++++++-
>  4 files changed, 183 insertions(+), 16 deletions(-)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index d5af644..b8e7419 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -129,6 +129,12 @@ typedef struct CPUARMState {
>          uint32_t c7_par;  /* Translation result. */
>          uint32_t c9_insn; /* Cache lockdown registers.  */
>          uint32_t c9_data;
> +        uint32_t c9_pmcr; /* performance monitor control register */
> +        uint32_t c9_pmcnten; /* perf monitor counter enables */
> +        uint32_t c9_pmovsr; /* perf monitor overflow status */
> +        uint32_t c9_pmxevtyper; /* perf monitor event type */
> +        uint32_t c9_pmuserenr; /* perf monitor user enable */
> +        uint32_t c9_pminten; /* perf monitor interrupt enables */
>          uint32_t c13_fcse; /* FCSE PID.  */
>          uint32_t c13_context; /* Context ID.  */
>          uint32_t c13_tls1; /* User RW Thread register.  */
> @@ -434,7 +440,7 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
>  #define cpu_signal_handler cpu_arm_signal_handler
>  #define cpu_list arm_cpu_list
>  
> -#define CPU_SAVE_VERSION 3
> +#define CPU_SAVE_VERSION 4
>  
>  /* MMU modes definitions */
>  #define MMU_MODE0_SUFFIX _kernel
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 62ae72e..b051b8c 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -270,6 +270,10 @@ void cpu_reset(CPUARMState *env)
>      }
>      env->vfp.xregs[ARM_VFP_FPEXC] = 0;
>      env->cp15.c2_base_mask = 0xffffc000u;
> +    /* v7 performance monitor control register: same implementor
> +     * field as main ID register, and we implement no event counters.
> +     */
> +    env->cp15.c9_pmcr = (id & 0xff000000);
>  #endif
>      set_flush_to_zero(1, &env->vfp.standard_fp_status);
>      set_flush_inputs_to_zero(1, &env->vfp.standard_fp_status);
> @@ -1587,6 +1591,81 @@ void HELPER(set_cp15)(CPUState *env, uint32_t insn, 
> uint32_t val)
>          case 1: /* TCM memory region registers.  */
>              /* Not implemented.  */
>              goto bad_reg;
> +        case 12: /* Performance monitor control */
> +            /* Performance monitors are implementation defined in v7,
> +             * but with an ARM recommended set of registers, which we
> +             * follow (although we don't actually implement any counters)
> +             */
> +            if (!arm_feature(env, ARM_FEATURE_V7)) {
> +                goto bad_reg;
> +            }
> +            switch (op2) {
> +            case 0: /* performance monitor control register */
> +                /* only the DP, X, D and E bits are writable */
> +                env->cp15.c9_pmcr &= ~0x39;
> +                env->cp15.c9_pmcr |= (val & 0x39);
> +                break;
> +            case 1: /* Count enable set register */
> +                val &= (1 << 31);
> +                env->cp15.c9_pmcnten |= val;
> +                break;
> +            case 2: /* Count enable clear */
> +                val &= (1 << 31);
> +                env->cp15.c9_pmcnten &= ~val;
> +                break;
> +            case 3: /* Overflow flag status */
> +                env->cp15.c9_pmovsr &= ~val;
> +                break;
> +            case 4: /* Software increment */
> +                /* RAZ/WI since we don't implement the software-count event 
> */
> +                break;
> +            case 5: /* Event counter selection register */
> +                /* Since we don't implement any events, writing to this 
> register
> +                 * is actually UNPREDICTABLE. So we choose to RAZ/WI.
> +                 */
> +                break;
> +            default:
> +                goto bad_reg;
> +            }
> +            break;
> +        case 13: /* Performance counters */
> +            if (!arm_feature(env, ARM_FEATURE_V7)) {
> +                goto bad_reg;
> +            }
> +            switch (op2) {
> +            case 0: /* Cycle count register: not implemented, so RAZ/WI */
> +                break;
> +            case 1: /* Event type select */
> +                env->cp15.c9_pmxevtyper = val & 0xff;
> +                break;
> +            case 2: /* Event count register */
> +                /* Unimplemented (we have no events), RAZ/WI */
> +                break;
> +            default:
> +                goto bad_reg;
> +            }
> +            break;
> +        case 14: /* Performance monitor control */
> +            if (!arm_feature(env, ARM_FEATURE_V7)) {
> +                goto bad_reg;
> +            }
> +            switch (op2) {
> +            case 0: /* user enable */
> +                env->cp15.c9_pmuserenr = val & 1;
> +                /* changes access rights for cp registers, so flush tbs */
> +                tb_flush(env);

If you flush all tbs, you also have to ensure that on the translate.c
side, this is the last instruction of the tb. Otherwise, the rest of the
TB will be executed with the wrong access rights.

> +                break;
> +            case 1: /* interrupt enable set */
> +                /* We have no event counters so only the C bit can be 
> changed */
> +                val &= (1 << 31);
> +                env->cp15.c9_pminten |= val;
> +                break;
> +            case 2: /* interrupt enable clear */
> +                val &= (1 << 31);
> +                env->cp15.c9_pminten &= ~val;
> +                break;
> +            }
> +            break;
>          default:
>              goto bad_reg;
>          }
> @@ -1878,27 +1957,81 @@ uint32_t HELPER(get_cp15)(CPUState *env, uint32_t 
> insn)
>          return 0;
>      case 8: /* MMU TLB control.  */
>          goto bad_reg;
> -    case 9: /* Cache lockdown.  */
> -        switch (op1) {
> -        case 0: /* L1 cache.  */
> -         if (arm_feature(env, ARM_FEATURE_OMAPCP))
> -             return 0;
> +    case 9:
> +        switch (crm) {
> +        case 0: /* Cache lockdown */
> +            switch (op1) {
> +            case 0: /* L1 cache.  */
> +                if (arm_feature(env, ARM_FEATURE_OMAPCP)) {
> +                    return 0;
> +                }
> +                switch (op2) {
> +                case 0:
> +                    return env->cp15.c9_data;
> +                case 1:
> +                    return env->cp15.c9_insn;
> +                default:
> +                    goto bad_reg;
> +                }
> +            case 1: /* L2 cache */
> +                if (crm != 0) {
> +                    goto bad_reg;
> +                }
> +                /* L2 Lockdown and Auxiliary control.  */
> +                return 0;
> +            default:
> +                goto bad_reg;
> +            }
> +            break;
> +        case 12: /* Performance monitor control */
> +            if (!arm_feature(env, ARM_FEATURE_V7)) {
> +                goto bad_reg;
> +            }
>              switch (op2) {
> -            case 0:
> -                return env->cp15.c9_data;
> -            case 1:
> -                return env->cp15.c9_insn;
> +            case 0: /* performance monitor control register */
> +                return env->cp15.c9_pmcr;
> +            case 1: /* count enable set */
> +            case 2: /* count enable clear */
> +                return env->cp15.c9_pmcnten;
> +            case 3: /* overflow flag status */
> +                return env->cp15.c9_pmovsr;
> +            case 4: /* software increment */
> +            case 5: /* event counter selection register */
> +                return 0; /* Unimplemented, RAZ/WI */
>              default:
>                  goto bad_reg;
>              }
> -        case 1: /* L2 cache */
> -            if (crm != 0)
> +        case 13: /* Performance counters */
> +            if (!arm_feature(env, ARM_FEATURE_V7)) {
> +                goto bad_reg;
> +            }
> +            switch (op2) {
> +            case 1: /* Event type select */
> +                return env->cp15.c9_pmxevtyper;
> +            case 0: /* Cycle count register */
> +            case 2: /* Event count register */
> +                /* Unimplemented, so RAZ/WI */
> +                return 0;
> +            default:
>                  goto bad_reg;
> -            /* L2 Lockdown and Auxiliary control.  */
> -            return 0;
> +            }
> +        case 14: /* Performance monitor control */
> +            if (!arm_feature(env, ARM_FEATURE_V7)) {
> +                goto bad_reg;
> +            }
> +            switch (op2) {
> +            case 0: /* user enable */
> +                return env->cp15.c9_pmuserenr;
> +            case 1: /* interrupt enable set */
> +            case 2: /* interrupt enable clear */
> +                return env->cp15.c9_pminten;
> +            default:
> +                goto bad_reg;
> +            }
>          default:
>              goto bad_reg;
>          }
> +        break;
>      case 10: /* MMU TLB lockdown.  */
>          /* ??? TLB lockdown not implemented.  */
>          return 0;
> diff --git a/target-arm/machine.c b/target-arm/machine.c
> index a18b7dc..7d4fc54 100644
> --- a/target-arm/machine.c
> +++ b/target-arm/machine.c
> @@ -44,6 +44,12 @@ void cpu_save(QEMUFile *f, void *opaque)
>      qemu_put_be32(f, env->cp15.c7_par);
>      qemu_put_be32(f, env->cp15.c9_insn);
>      qemu_put_be32(f, env->cp15.c9_data);
> +    qemu_put_be32(f, env->cp15.c9_pmcr);
> +    qemu_put_be32(f, env->cp15.c9_pmcnten);
> +    qemu_put_be32(f, env->cp15.c9_pmovsr);
> +    qemu_put_be32(f, env->cp15.c9_pmxevtyper);
> +    qemu_put_be32(f, env->cp15.c9_pmuserenr);
> +    qemu_put_be32(f, env->cp15.c9_pminten);
>      qemu_put_be32(f, env->cp15.c13_fcse);
>      qemu_put_be32(f, env->cp15.c13_context);
>      qemu_put_be32(f, env->cp15.c13_tls1);
> @@ -152,6 +158,12 @@ int cpu_load(QEMUFile *f, void *opaque, int version_id)
>      env->cp15.c7_par = qemu_get_be32(f);
>      env->cp15.c9_insn = qemu_get_be32(f);
>      env->cp15.c9_data = qemu_get_be32(f);
> +    env->cp15.c9_pmcr = qemu_get_be32(f);
> +    env->cp15.c9_pmcnten = qemu_get_be32(f);
> +    env->cp15.c9_pmovsr = qemu_get_be32(f);
> +    env->cp15.c9_pmxevtyper = qemu_get_be32(f);
> +    env->cp15.c9_pmuserenr = qemu_get_be32(f);
> +    env->cp15.c9_pminten = qemu_get_be32(f);
>      env->cp15.c13_fcse = qemu_get_be32(f);
>      env->cp15.c13_context = qemu_get_be32(f);
>      env->cp15.c13_tls1 = qemu_get_be32(f);
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index e1bda57..ea9cc30 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -2438,12 +2438,28 @@ static int disas_cp_insn(CPUState *env, DisasContext 
> *s, uint32_t insn)
>      return 0;
>  }
>  
> -static int cp15_user_ok(uint32_t insn)
> +static int cp15_user_ok(CPUState *env, uint32_t insn)
>  {
>      int cpn = (insn >> 16) & 0xf;
>      int cpm = insn & 0xf;
>      int op = ((insn >> 5) & 7) | ((insn >> 18) & 0x38);
>  
> +    if (arm_feature(env, ARM_FEATURE_V7) && cpn == 9) {
> +        /* Performance monitor registers fall into three categories:
> +         *  (a) always UNDEF in usermode
> +         *  (b) UNDEF only if PMUSERENR.EN is 0
> +         *  (c) always read OK and UNDEF on write (PMUSERENR only)
> +         */
> +        if ((cpm == 12 && (op < 6)) ||
> +            (cpm == 13 && (op < 3))) {
> +            return env->cp15.c9_pmuserenr;
> +        } else if (cpm == 14 && op == 0 && (insn & ARM_CP_RW_BIT)) {
> +            /* PMUSERENR, read only */
> +            return 1;
> +        }
> +        return 0;
> +    }
> +

Instead of having this complex test for all cp15 access, but only for
catching a few access to performance registers, wouldn't it make more
sense to have this test and an exception triggering directly in 
helper.c?

>      if (cpn == 13 && cpm == 0) {
>          /* TLS register.  */
>          if (op == 2 || (op == 3 && (insn & ARM_CP_RW_BIT)))
> @@ -2530,7 +2546,7 @@ static int disas_cp15_insn(CPUState *env, DisasContext 
> *s, uint32_t insn)
>          /* cdp */
>          return 1;
>      }
> -    if (IS_USER(s) && !cp15_user_ok(insn)) {
> +    if (IS_USER(s) && !cp15_user_ok(env, insn)) {
>          return 1;
>      }
>  
> -- 
> 1.7.1
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net