Re: [Qemu-devel] [PATCH arm-ccnt v1 1/1] ARM-CCNT: Implements the ARM PMCCNTR register

[Peter Maydell]

On 16 January 2014 04:31, Alistair Francis <address@hidden> wrote:
> This patch implements the ARM PMCCNTR register including
> the disable and reset components of the PMCR register.
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> This patch assumes that non-invasive debugging is not permitted
> when determining if the counter is disabled
>
>  target-arm/cpu.c    |    2 ++
>  target-arm/cpu.h    |    3 +++
>  target-arm/helper.c |   43 ++++++++++++++++++++++++++++++++++++++++---
>  target-arm/helper.h |    9 +++++++++
>  4 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 52efd5d..a185959 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -76,6 +76,8 @@ static void arm_cpu_reset(CPUState *s)
>
>      acc->parent_reset(s);
>
> +    env->emm_time = 0;
> +
>      memset(env, 0, offsetof(CPUARMState, breakpoints));
>      g_hash_table_foreach(cpu->cp_regs, cp_reg_reset, cpu);
>      env->vfp.xregs[ARM_VFP_FPSID] = cpu->reset_fpsid;
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 198b6b8..8c73a56 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -215,8 +215,11 @@ typedef struct CPUARMState {
>          uint32_t c15_diagnostic; /* diagnostic register */
>          uint32_t c15_power_diagnostic;
>          uint32_t c15_power_control; /* power control */
> +        uint32_t c15_ccnt; /* Performance Monitors Cycle Count */
>      } cp15;
>
> +    int64_t emm_time; /* Used to hold the total emmulation time - in ns */

This looks bogus. You don't need to save the total emulation
time, because you can always get it by calling
qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL).
(Also it won't get migrated, but the correct fix for that is to get
rid of it, see below.)

> +
>      /* System registers (AArch64) */
>      struct {
>          uint64_t tpidr_el0;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index c708f15..72404cf 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -505,9 +505,16 @@ static int pmcr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>      if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
>          return EXCP_UDEF;
>      }
> -    /* only the DP, X, D and E bits are writable */
> +    /* only the DP, X, D and E bits are writeable */

"writable" is a valid spelling and a quick grep shows it's
actually used much more in qemu than "writeable".

>      env->cp15.c9_pmcr &= ~0x39;
>      env->cp15.c9_pmcr |= (value & 0x39);
> +
> +    if (value & PMCRC) {
> +        /* Reset the counter */
> +        env->emm_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        env->cp15.c15_ccnt = 0;

This is the wrong way to do this kind of thing. Don't store
the actual value of a resettable count register; store the
difference between the CLOCK_VIRTUAL count and
the guest-visible value. Then you don't have to separately
keep both emm_time and c15_ccnt as state.

> +    }
> +
>      return 0;
>  }
>
> @@ -584,6 +591,35 @@ static int vbar_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>      return 0;
>  }
>
> +static int pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
> +                       uint64_t *value)
> +{
> +    int64_t clock_value;
> +
> +    /* This assumes that non-invasive debugging is not permitted */
> +    if (env->cp15.c9_pmcr & PMCRDP ||
> +       !(env->cp15.c9_pmcr & PMCRE)) {
> +        /* Counter is disabled, do not change value */
> +        *value = env->cp15.c15_ccnt;
> +        return 0;
> +    }
> +
> +    clock_value = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +
> +    if (env->cp15.c9_pmcr & PMCRDP) {
> +        /* Increment once every 64 processor clock cycles */
> +        env->cp15.c15_ccnt += (uint32_t) CLOCKNSTOTICKS((clock_value - \
> +                                                       env->emm_time))/64;

Why the line continuation character?

> +    } else {
> +        env->cp15.c15_ccnt += (uint32_t) CLOCKNSTOTICKS((clock_value - \
> +                                                       env->emm_time));
> +    }
> +
> +    env->emm_time = clock_value;
> +    *value = env->cp15.c15_ccnt;
> +    return 0;
> +}
> +
>  static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>                         uint64_t *value)
>  {
> @@ -644,9 +680,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>       */
>      { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
>        .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> -    /* Unimplemented, RAZ/WI. XXX PMUSERENR */
>      { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +      .access = PL1_RW, .readfn = pmccntr_read,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt),
> +      .resetvalue = 0, .type = ARM_CP_IO },
>      { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 
> = 1,
>        .access = PL0_RW,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index 70872df..5b5dc0a 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -492,6 +492,15 @@ DEF_HELPER_3(neon_qzip32, void, env, i32, i32)
>  DEF_HELPER_4(crypto_aese, void, env, i32, i32, i32)
>  DEF_HELPER_4(crypto_aesmc, void, env, i32, i32, i32)
>
> +/* Definitions for the PMCCNTR and PMCR registers */
> +#define PMCRDP  0x20
> +#define PMCRD   0x8
> +#define PMCRC   0x4
> +#define PMCRE   0x1
> +
> +#define CLOCKNSTOTICKS(input) ((((input * get_ticks_per_sec())/1000000000) 
> >> 8) & \
> +                              0xFFFFFFFF)
> +

This is absolutely the wrong header for this kind of thing. Also that
macro is rather misleadingly named, looks pretty ugly and is not
really necessary given it's basically used in just one place.

thanks
-- PMM