On Tue, Jan 28, 2014 at 12:01 PM, Peter Crosthwaite <peter.crosthwa...@xilinx.com> wrote: > On Tue, Jan 28, 2014 at 11:31 AM, Alistair Francis > <alistair.fran...@xilinx.com> wrote: >> 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. > > 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 >>>> >>>> >>> >> >