[target-arm,v4,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
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    |    3 ++
 target-arm/helper.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 78 insertions(+), 2 deletions(-)

On 28 January 2014 06:25, 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
> 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    |    3 ++
>  target-arm/helper.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 78 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 198b6b8..2fdab58 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -215,6 +215,9 @@ 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 */

Newline before the "*/" please.

> +        uint32_t c15_ccnt;
>      } cp15;
>
>      /* System registers (AArch64) */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index c708f15..f6c57c8 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;
> +    }
> +
>      return 0;
>  }
>
> @@ -584,6 +624,39 @@ 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)
> +{
> +    /* A NOP write */
> +    qemu_log_mask(LOG_UNIMP, "CCNT: Write not implemented\n");
> +    return 0;
> +}

This will break migration. You must provide a mechanism for the
migration to do a "read register on source end; write value to register
at destination". (You also in this case need to make sure migration
works whether the migration process writes the control register
first and the counter second or the other way around.)

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

thanks
-- PMM

On Wed, Jan 29, 2014 at 10:28 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 28 January 2014 06:25, 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
>> 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    |    3 ++
>>  target-arm/helper.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 78 insertions(+), 2 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 198b6b8..2fdab58 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -215,6 +215,9 @@ 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 */
>
> Newline before the "*/" please.
>
>> +        uint32_t c15_ccnt;
>>      } cp15;
>>
>>      /* System registers (AArch64) */
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index c708f15..f6c57c8 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;
>> +    }
>> +
>>      return 0;
>>  }
>>
>> @@ -584,6 +624,39 @@ 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)
>> +{
>> +    /* A NOP write */
>> +    qemu_log_mask(LOG_UNIMP, "CCNT: Write not implemented\n");
>> +    return 0;
>> +}
>
> This will break migration. You must provide a mechanism for the
> migration to do a "read register on source end; write value to register
> at destination". (You also in this case need to make sure migration
> works whether the migration process writes the control register
> first and the counter second or the other way around.)
>

Is this as simple as hooking up the default raw_write/raw_read
handlers? From a migration perspective is should be a case of simply
saving and loading the state value with no fancyness. The vm timer
should migrate so there should be no need for any of the syncing
effects on either end of a migration.

Regards,
Peter

>> +
>>  static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>>                         uint64_t *value)
>>  {
>> @@ -644,9 +717,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 },
>> -    /* 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, .writefn = pmccntr_write,
>> +      .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),
>
> thanks
> -- PMM
>

On 30 January 2014 07:00, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Wed, Jan 29, 2014 at 10:28 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> This will break migration. You must provide a mechanism for the
>> migration to do a "read register on source end; write value to register
>> at destination". (You also in this case need to make sure migration
>> works whether the migration process writes the control register
>> first and the counter second or the other way around.)
>>
>
> Is this as simple as hooking up the default raw_write/raw_read
> handlers? From a migration perspective is should be a case of simply
> saving and loading the state value with no fancyness. The vm timer
> should migrate so there should be no need for any of the syncing
> effects on either end of a migration.

You also have to consider KVM<->TCG migration, so the value on
the wire should be the actual value of the register, not the
value of TCG's underlying state. So you need a raw read/write
fn (and also to fix up the one for the enable register) but it's
not as simple as just using the raw_read/write functions.

thanks
-- PMM

On Thu, Jan 30, 2014 at 8:22 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 30 January 2014 07:00, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> On Wed, Jan 29, 2014 at 10:28 PM, Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>> This will break migration. You must provide a mechanism for the
>>> migration to do a "read register on source end; write value to register
>>> at destination". (You also in this case need to make sure migration
>>> works whether the migration process writes the control register
>>> first and the counter second or the other way around.)
>>>
>>
>> Is this as simple as hooking up the default raw_write/raw_read
>> handlers? From a migration perspective is should be a case of simply
>> saving and loading the state value with no fancyness. The vm timer
>> should migrate so there should be no need for any of the syncing
>> effects on either end of a migration.
>
> You also have to consider KVM<->TCG migration, so the value on
> the wire should be the actual value of the register, not the
> value of TCG's underlying state. So you need a raw read/write
> fn (and also to fix up the one for the enable register) but it's
> not as simple as just using the raw_read/write functions.
>

At the point, you're better off just implementing the actual write
functionality, considering the semantic of the write exactly matches a
"raw write" - i.e. update the register value. Once the the proper
write fn is implement these migration concerns will just come out in
the wash.

Regards,
Peter

> thanks
> -- PMM
>