[target-arm,v5,1/1] target-arm: Implements the ARM PMCCNTR register

This patch implements the ARM PMCCNTR register including
the disable and reset components of the PMCR register.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
This patch assumes that non-invasive debugging is not permitted
when determining if the counter is disabled
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 |   95 +++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 97 insertions(+), 2 deletions(-)

Ping

On Fri, Jan 31, 2014 at 2:44 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> This patch implements the ARM PMCCNTR register including
> the disable and reset components of the PMCR register.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> This patch assumes that non-invasive debugging is not permitted
> when determining if the counter is disabled
> 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 |   95 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 198b6b8..5f96a4d 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;
>
>      /* System registers (AArch64) */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index c708f15..090953a 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;
> @@ -502,12 +508,46 @@ static int pmreg_read(CPUARMState *env, const ARMCPRegInfo *ri,
>  static int pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                        uint64_t value)
>  {
> +    uint32_t temp_ticks;
> +
>      if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
>          return EXCP_UDEF;
>      }
> +
> +    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 */
> +        if (env->cp15.c9_pmcr & PMCRDP) {
> +            /* Increment once every 64 processor clock cycles */
> +            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;
> +    }
> +
>      return 0;
>  }
>
> @@ -584,6 +624,56 @@ static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      return 0;
>  }
>
> +static int pmccntr_read(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, do not change value */
> +        *value = env->cp15.c15_ccnt;
> +        return 0;
> +    }
> +
> +    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;
> +    }
> +    *value = total_ticks - env->cp15.c15_ccnt;
> +
> +    return 0;
> +}
> +
> +static int 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 0;
> +    }
> +
> +    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;
> +    }
> +    env->cp15.c15_ccnt = total_ticks - value;
> +
> +    return 0;
> +}
> +
>  static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>                         uint64_t *value)
>  {
> @@ -644,9 +734,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, .resetvalue = 0, .type = ARM_CP_IO,
> +      .readfn = pmccntr_read, .writefn = pmccntr_write,
> +      .raw_readfn = raw_read, .raw_writefn = raw_write },
>      { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
>        .access = PL0_RW,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
> --
> 1.7.1
>

On 31 January 2014 04:44, Alistair Francis <alistair.francis@xilinx.com> wrote:
> This patch implements the ARM PMCCNTR register including
> the disable and reset components of the PMCR register.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>

I'm afraid you'll need to respin this against my target-arm.next
branch, because I've landed the patches which update the
API for read/write functions.
  git://git.linaro.org/people/peter.maydell/qemu-arm.git target-arm.next

> +
>  static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>                         uint64_t *value)
>  {
> @@ -644,9 +734,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 */

Your patch has dropped this XXX comment without fixing the
issue. Luckily you'll find that in target-arm.next it's been fixed,
by providing a suitable accessfn for the register.

>      { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +      .access = PL1_RW, .resetvalue = 0, .type = ARM_CP_IO,

This changes the access rights, incorrectly.

> +      .readfn = pmccntr_read, .writefn = pmccntr_write,
> +      .raw_readfn = raw_read, .raw_writefn = raw_write },

This is wrong, because raw_read/write need to return the same
value as the actual register, just without odd side effects.

>      { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
>        .access = PL0_RW,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),

Thanks, and sorry for the inconvenience resulting from
the clashing patchset.

-- PMM

On Thu, Feb 13, 2014 at 5:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 31 January 2014 04:44, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> This patch implements the ARM PMCCNTR register including
>> the disable and reset components of the PMCR register.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>
> I'm afraid you'll need to respin this against my target-arm.next
> branch, because I've landed the patches which update the
> API for read/write functions.
>   git://git.linaro.org/people/peter.maydell/qemu-arm.git target-arm.next
>
>> +
>>  static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>>                         uint64_t *value)
>>  {
>> @@ -644,9 +734,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 */
>
> Your patch has dropped this XXX comment without fixing the
> issue. Luckily you'll find that in target-arm.next it's been fixed,
> by providing a suitable accessfn for the register.
>
>>      { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
>> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +      .access = PL1_RW, .resetvalue = 0, .type = ARM_CP_IO,
>
> This changes the access rights, incorrectly.
>
>> +      .readfn = pmccntr_read, .writefn = pmccntr_write,
>> +      .raw_readfn = raw_read, .raw_writefn = raw_write },
>
> This is wrong, because raw_read/write need to return the same
> value as the actual register, just without odd side effects.
>

My understanding is that the _read and _write functions as you have
implemented are this exact semantic. So you should be able to just
drop the raw_'s.

Regards,
Peter

>>      { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
>>        .access = PL0_RW,
>>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
>
> Thanks, and sorry for the inconvenience resulting from
> the clashing patchset.
>
> -- PMM
>