Thanks, I'll rename the patch in the next version
On Thu, Jan 23, 2014 at 11:56 PM, Peter Crosthwaite
<peter.crosthwa...@xilinx.com> wrote:
> Same subject-prefix as commented by Andreas. Should be "target-arm".
>
> On Wed, Jan 22, 2014 at 9:40 AM, Alistair Francis
> <alistair.fran...@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.fran...@xilinx.com>
>> ---
>> 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. 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?
>
> 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
>>
>>
>
