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