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

[Alistair Francis]

On Tue, Jan 28, 2014 at 12:01 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Tue, Jan 28, 2014 at 11:31 AM, Alistair Francis
> <address@hidden> wrote:
>> Thanks, I'll rename the patch in the next version
>>
>> On Thu, Jan 23, 2014 at 11:56 PM, Peter Crosthwaite
>> <address@hidden> wrote:
>>> Same subject-prefix as commented by Andreas. Should be "target-arm".
>>>
>>> On Wed, Jan 22, 2014 at 9:40 AM, 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
>>>> V3: Fixed up incorrect reset, disable and enable handling that
>>>> was submitted in V2. The patch should now also correctly handle
>>>> on the fly changing of the clock scaling factor.
>>>> 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 |   70 
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>  2 files changed, 71 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 */
>>>> +        uint32_t c15_ccnt;
>>>>      } cp15;
>>>>
>>>>      /* System registers (AArch64) */
>>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>>> index c708f15..ad87136 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,44 @@ 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, diff;
>>>> +
>>>>      if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
>>>>          return EXCP_UDEF;
>>>>      }
>>>> +
>>>> +    temp_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
>>>> +                                   get_ticks_per_sec())/1000000000) >> 8) 
>>>> &
>>>> +                                   0xFFFFFFFF);
>>>> +
>>>> +    /* 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 */
>>>> +        env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
>>>
>>> Should this be frequency scalar aware? I think its calculating and
>>> saving the current timer value but assuming the X64 prescale is
>>> disabled. But since the intent is to save the current timer value to
>>> c15_ccnt, is it as simple as just calling pmccntr_read() here to get
>>> the desired to-be-saved value?
>>>
>>
>> Yes, it should be frequency scalar aware. I have fixed that up
>>
>>>> +    }
>>>> +
>>>> +    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 */
>>>> +            diff = (temp_ticks/64) - env->cp15.c15_ccnt;
>>>
>>> I think you can simplify with just
>>>
>>> diff = (temp_ticks/64);
>>>
>>> and ...
>>>
>>>> +        } else {
>>>> +            diff = temp_ticks - env->cp15.c15_ccnt;
>>>> +        }
>>>> +        env->cp15.c15_ccnt += diff;
>>>
>>> ... env->cp15.c15_ccnt = diff;
>>>
>>> Although it's probably even simpler to just drop this and assign
>>> env->cp15.c15_ccnt in the two if-else branches directly.
>>>
>>
>> Done!
>>
>>>> +    }
>>>> +
>>>>      return 0;
>>>>  }
>>>>
>>>> @@ -584,6 +622,33 @@ 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 = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
>>>> +                                 get_ticks_per_sec())/1000000000) >> 8) &
>>>
>>> Theres an un-needed parenthesis of multiplication before division.
>>>
>>> Whats the shift by 8 for?
>>
>> The shift by 8 is because qemu_clock_get_ns() returns a 64-bit number.
>>
>> I have always though that the values look too big when using the
>> bottom half. The register jumps from: 4289154 to 3339447850 after a 1
>> second sleep.
>
> Virtual or real time? Perhaps a worthwhile experiment is to see if the
> qemu_get_clock_ns values are correlating to real time (separately from
> this timer). It could be that qemu_clock_get_ns is running too fast.

I'm using the virtual timer, so it should be an accurate time of the
emulated guest.

I just did a test then and the raw nano second value does seem to be a
bit fast. It incremented three seconds after my test 1s wait (which
probably isn't very accurate, but still shouldn't be that bad).

qemu_clock_get_ns() is the most accurate timer that I could find for
the guests running time. So you think I should remove the bit shift
and just use the top half of qemu_clock_get_ns()?

>
>  That's why I have been using the top half, do you think
>> I should use the bottom half of the number?
>>
>>>
>>>> +                                 0xFFFFFFFF);
>>>
>>> The mask-to-32bit is un-needed I think.
>>>
>>>> +
>>>> +    if (env->cp15.c9_pmcr & PMCRDP) {
>>>> +        /* Increment once every 64 processor clock cycles */
>>>> +        *value = (total_ticks/64) - env->cp15.c15_ccnt;
>>>
>>> You can save on a common sub-express by just
>>>
>>> total_ticks /= 64;
>>>
>>> Then ...
>>>
>>>> +    } else {
>>>> +        *value = total_ticks - env->cp15.c15_ccnt;
>>>
>>> .. you can do this unconditionally.
>>>
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>  static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>>>>                         uint64_t *value)
>>>>  {
>>>> @@ -644,9 +709,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),
>>>
>>> You don't implement write, so I'm not sure exactly what you would do
>>> to implement readfn with UNIMP writefn. But this as-is will allow the
>>> guest to overwrite the "diff" value with an absolute, which would
>>> cause very strange behaviour.
>>
>> I have changed the permissions to make it a write only register. I
>> have the fieldoffset attribute as that is required if there is no
>> write function. Would you rather I remove fieldoffset and have an
>> empty write function?
>>
>
> That will cause the CP Register interface to throw an exception on
> write, whereas original implementation will just NOP it so the guest
> happily moves on. Although I would suggest that what you have done is
> more accurate, as a silent NOP due to non-implementation is much
> harder to debug than a EXCP_UDEF. But someone will have added that
> dummy for a reason so I think you need to preserve the NOP write.

Ok, I will change it to a NOP write (Just having it as a read only
register causes problems with my test case as well). Do you think I
should put a debug print in the NOP write, or just leave it empty?

>
> Regards,
> Peter
>
>>>
>>> Regards,
>>> Peter
>>>
>>>> +      .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),
>>>> --
>>>> 1.7.1
>>>>
>>>>
>>>
>>
>