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

[Alistair Francis]

I agree with your comments. I can't believe I missed the PMCRDP/PMCRD
mistake. Submitting V7 now

On Wed, Feb 19, 2014 at 2:08 AM, Peter Maydell <address@hidden> wrote:
> 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
>