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

[Peter Crosthwaite]

On Sun, Jan 19, 2014 at 8:01 AM, Peter Maydell <address@hidden> wrote:
> 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.)
>

It means something slightly different doesnt it? It's the emulation
time last time the ccnt was synced. Looking below its always updated
syncronously with ccnt itself and never on its own. Perhaps a rename
to make that clearer.

>> +
>>      /* 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".
>

Yes out of scope change, please revert.

>>      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.
>

Is the state minimisation really worth that pain? If only reset was
being implemented, it could be done as a simple case of snapping
clock-virtual as "clock_when_ccnt_reset" then removing the CCNT state
entirely but the problem is it does make the implementation of the
disable feature somewhat difficult. If the timer is disabled this
"diff" needs to start changing as emulation time progresses. If I
disable, wait for 1s CLOCK_VIRTUAL time then renable, i need to
recalulate this diff -= 1. The only way that calculation can be done
is with a second variable, which snapshots CLOCK_VIRTUAL when disabled
happened so this diff can be recalculated. Its also awkward as if you
disable for long enough (or immediately on emulation start),
"clock_when_ccnt_reset" can go negative.

What's missing from this patch is there is no sense of syncing the two
variables when you disable, re-enable or change the freqency scaler
(which needs to be fixed). All of these features can cause the timer
to change frequency while it is at a non-zero value, which means this
diff variable would need continual warping every time one of these
frequency affecting settings changed.

Regards,
Peter

>> +    }
>> +
>>      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
>