Re: [Qemu-devel] [PATCH target-arm v6 1/1] target-arm: Implements the ARM PMCCNTR register

[Peter Maydell]

On 17 February 2014 01:20, 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
> V6: Rebase to include Peter Maydell's 'Convert performance monitor
> reginfo to accesfn' patch. Remove the raw_fn's as the read/write
> functions already do what is required.
> V5: Implement the actual write function to make sure that
> migration works correctly. Also includes the raw_read/write as
> the normal read/write functions depend on the pmcr register. So
> they don't allow for the pmccntr register to be written first.
> V4: Some bug fixes pointed out by Peter Crosthwaite. Including
> increasing the accuracy of the timer.
> V3: Fixed up incorrect reset, disable and enable handling that
> was submitted in V2. The patch should now also handle changing
> of the clock scaling.
> V2: Incorporated the comments that Peter Maydell and Peter
> Crosthwaite had. Now the implementation only requires one
> CPU state
>
>  target-arm/cpu.h    |    4 ++
>  target-arm/helper.c |   86 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 3c8a2db..14fd1ae 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -215,6 +215,10 @@ typedef struct CPUARMState {
>          uint32_t c15_diagnostic; /* diagnostic register */
>          uint32_t c15_power_diagnostic;
>          uint32_t c15_power_control; /* power control */
> +        /* If the counter is enabled, this stores the last time the counter
> +         * was reset. Otherwise it stores the counter value
> +         */
> +        uint32_t c15_ccnt;
>      } cp15;
>
>      struct {
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b547f04..abc2eb0 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -13,6 +13,12 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t 
> address,
>                                  target_ulong *page_size);
>  #endif
>
> +/* Definitions for the PMCCNTR and PMCR registers */
> +#define PMCRDP  0x20
> +#define PMCRD   0x8
> +#define PMCRC   0x4
> +#define PMCRE   0x1
> +
>  static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
>  {
>      int nregs;
> @@ -478,9 +484,41 @@ static CPAccessResult pmreg_access(CPUARMState *env, 
> const ARMCPRegInfo *ri)
>  static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                         uint64_t value)
>  {
> +    uint32_t temp_ticks;
> +
> +    temp_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) *
> +                  get_ticks_per_sec() / 1000000;
> +
> +    /* This assumes that non-invasive debugging is not permitted */
> +    if (!(env->cp15.c9_pmcr & PMCRDP) ||
> +        env->cp15.c9_pmcr & PMCRE) {
> +        /* If the counter is enabled */

This condition looks wrong. PMCRDP set means "disable
the counter only in periods when non-invasive debug is
not permitted", not "disable it always". And your
logic is enabling the counter if PMCRDP is clear, even
if PMCRE is clear, which is also wrong.

Your assumption is also wrong: we don't implement TrustZone,
and only on CPUs which implement TZ can non-invasive debug
be set to 'not permitted'. (This is distinct from whether
NID is enabled or not, see the ARM ARM section on non-invasive
debug authentication).

So I think the correct logic here is to ignore PMCRDP
completely and just use PMCRE as the enable bit.

> +        if (env->cp15.c9_pmcr & PMCRDP) {
> +            /* Increment once every 64 processor clock cycles */

This should be PMCRD, surely?

> +            env->cp15.c15_ccnt = (temp_ticks/64) - env->cp15.c15_ccnt;
> +        } else {
> +            env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
> +        }
> +    }
> +
> +    if (value & PMCRC) {
> +        /* The counter has been reset */
> +        env->cp15.c15_ccnt = 0;
> +    }
> +
>      /* only the DP, X, D and E bits are writable */
>      env->cp15.c9_pmcr &= ~0x39;
>      env->cp15.c9_pmcr |= (value & 0x39);
> +
> +    /* This assumes that non-invasive debugging is not permitted */
> +    if (!(env->cp15.c9_pmcr & PMCRDP) ||
> +        env->cp15.c9_pmcr & PMCRE) {
> +        if (env->cp15.c9_pmcr & PMCRDP) {
> +            /* Increment once every 64 processor clock cycles */
> +            temp_ticks /= 64;
> +        }
> +        env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
> +    }
>  }
>
>  static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -536,6 +574,50 @@ static void vbar_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>      env->cp15.c12_vbar = value & ~0x1Ful;
>  }
>
> +static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    uint32_t total_ticks;
> +
> +    /* This assumes that non-invasive debugging is not permitted */
> +    if (env->cp15.c9_pmcr & PMCRDP ||
> +        !(env->cp15.c9_pmcr & PMCRE)) {

Remarks about enables apply here too.

> +        /* Counter is disabled, do not change value */
> +        return env->cp15.c15_ccnt;
> +    }
> +
> +    total_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) *
> +                  get_ticks_per_sec() / 1000000;
> +
> +    if (env->cp15.c9_pmcr & PMCRDP) {
> +        /* Increment once every 64 processor clock cycles */

Wrong bit again.

> +        total_ticks /= 64;
> +    }
> +    return total_ticks - env->cp15.c15_ccnt;
> +}
> +
> +static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                        uint64_t value)
> +{
> +    uint32_t total_ticks;
> +
> +    /* This assumes that non-invasive debugging is not permitted */
> +    if (env->cp15.c9_pmcr & PMCRDP ||
> +        !(env->cp15.c9_pmcr & PMCRE)) {
> +        /* Counter is disabled, set the absolute value */
> +        env->cp15.c15_ccnt = value;
> +        return;
> +    }
> +
> +    total_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) *
> +                  get_ticks_per_sec() / 1000000;
> +
> +    if (env->cp15.c9_pmcr & PMCRDP) {
> +        /* Increment once every 64 processor clock cycles */
> +        total_ticks /= 64;
> +    }

Ditto, ditto.

> +    env->cp15.c15_ccnt = total_ticks - value;
> +}
> +
>  static uint64_t ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
>      ARMCPU *cpu = arm_env_get_cpu(env);
> @@ -595,9 +677,9 @@ 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,
>        .accessfn = pmreg_access },
> -    /* Unimplemented, RAZ/WI. */
>      { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
> +      .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
> +      .readfn = pmccntr_read, .writefn = pmccntr_write,
>        .accessfn = pmreg_access },
>      { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 
> = 1,
>        .access = PL0_RW,
> --
> 1.7.1

thanks
-- PMM