I agree with your comments. I can't believe I missed the PMCRDP/PMCRD mistake. Submitting V7 now
On Wed, Feb 19, 2014 at 2:08 AM, Peter Maydell <[email protected]> wrote: > On 17 February 2014 01:20, Alistair Francis <[email protected]> > wrote: >> This patch implements the ARM PMCCNTR register including >> the disable and reset components of the PMCR register. >> >> Signed-off-by: Alistair Francis <[email protected]> >> --- >> This patch assumes that non-invasive debugging is not permitted >> when determining if the counter is disabled >> V6: Rebase to include Peter Maydell's 'Convert performance monitor >> reginfo to accesfn' patch. Remove the raw_fn's as the read/write >> functions already do what is required. >> V5: Implement the actual write function to make sure that >> migration works correctly. Also includes the raw_read/write as >> the normal read/write functions depend on the pmcr register. So >> they don't allow for the pmccntr register to be written first. >> V4: Some bug fixes pointed out by Peter Crosthwaite. Including >> increasing the accuracy of the timer. >> V3: Fixed up incorrect reset, disable and enable handling that >> was submitted in V2. The patch should now also handle changing >> of the clock scaling. >> V2: Incorporated the comments that Peter Maydell and Peter >> Crosthwaite had. Now the implementation only requires one >> CPU state >> >> target-arm/cpu.h | 4 ++ >> target-arm/helper.c | 86 >> +++++++++++++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 88 insertions(+), 2 deletions(-) >> >> diff --git a/target-arm/cpu.h b/target-arm/cpu.h >> index 3c8a2db..14fd1ae 100644 >> --- a/target-arm/cpu.h >> +++ b/target-arm/cpu.h >> @@ -215,6 +215,10 @@ 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; >> >> struct { >> diff --git a/target-arm/helper.c b/target-arm/helper.c >> index b547f04..abc2eb0 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; >> @@ -478,9 +484,41 @@ static CPAccessResult pmreg_access(CPUARMState *env, >> const ARMCPRegInfo *ri) >> static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri, >> uint64_t value) >> { >> + uint32_t temp_ticks; >> + >> + temp_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) * >> + get_ticks_per_sec() / 1000000; >> + >> + /* 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 */ > > This condition looks wrong. PMCRDP set means "disable > the counter only in periods when non-invasive debug is > not permitted", not "disable it always". And your > logic is enabling the counter if PMCRDP is clear, even > if PMCRE is clear, which is also wrong. > > Your assumption is also wrong: we don't implement TrustZone, > and only on CPUs which implement TZ can non-invasive debug > be set to 'not permitted'. (This is distinct from whether > NID is enabled or not, see the ARM ARM section on non-invasive > debug authentication). > > So I think the correct logic here is to ignore PMCRDP > completely and just use PMCRE as the enable bit. > >> + if (env->cp15.c9_pmcr & PMCRDP) { >> + /* Increment once every 64 processor clock cycles */ > > This should be PMCRD, surely? > >> + env->cp15.c15_ccnt = (temp_ticks/64) - env->cp15.c15_ccnt; >> + } else { >> + env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt; >> + } >> + } >> + >> + 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 */ >> + temp_ticks /= 64; >> + } >> + env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt; >> + } >> } >> >> static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri, >> @@ -536,6 +574,50 @@ static void vbar_write(CPUARMState *env, const >> ARMCPRegInfo *ri, >> env->cp15.c12_vbar = value & ~0x1Ful; >> } >> >> +static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri) >> +{ >> + uint32_t total_ticks; >> + >> + /* This assumes that non-invasive debugging is not permitted */ >> + if (env->cp15.c9_pmcr & PMCRDP || >> + !(env->cp15.c9_pmcr & PMCRE)) { > > Remarks about enables apply here too. > >> + /* Counter is disabled, do not change value */ >> + return env->cp15.c15_ccnt; >> + } >> + >> + total_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) * >> + get_ticks_per_sec() / 1000000; >> + >> + if (env->cp15.c9_pmcr & PMCRDP) { >> + /* Increment once every 64 processor clock cycles */ > > Wrong bit again. > >> + total_ticks /= 64; >> + } >> + return total_ticks - env->cp15.c15_ccnt; >> +} >> + >> +static void pmccntr_write(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, set the absolute value */ >> + env->cp15.c15_ccnt = value; >> + return; >> + } >> + >> + total_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) * >> + get_ticks_per_sec() / 1000000; >> + >> + if (env->cp15.c9_pmcr & PMCRDP) { >> + /* Increment once every 64 processor clock cycles */ >> + total_ticks /= 64; >> + } > > Ditto, ditto. > >> + env->cp15.c15_ccnt = total_ticks - value; >> +} >> + >> static uint64_t ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri) >> { >> ARMCPU *cpu = arm_env_get_cpu(env); >> @@ -595,9 +677,9 @@ 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, >> .accessfn = pmreg_access }, >> - /* Unimplemented, RAZ/WI. */ >> { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = >> 0, >> - .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0, >> + .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO, >> + .readfn = pmccntr_read, .writefn = pmccntr_write, >> .accessfn = pmreg_access }, >> { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 >> = 1, >> .access = PL0_RW, >> -- >> 1.7.1 > > thanks > -- PMM >
