| Message ID | 9c5dd63d5eace8dc88ba0459772364ac28114eb7.1392599296.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 17 February 2014 01:20, Alistair Francis <alistair.francis@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.francis@xilinx.com> > --- > 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
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 <peter.maydell@linaro.org> wrote: > On 17 February 2014 01:20, Alistair Francis <alistair.francis@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.francis@xilinx.com> >> --- >> 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 >
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 */ + if (env->cp15.c9_pmcr & PMCRDP) { + /* Increment once every 64 processor clock cycles */ + 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)) { + /* 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 */ + 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; + } + 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,
This patch implements the ARM PMCCNTR register including the disable and reset components of the PMCR register. Signed-off-by: Alistair Francis <alistair.francis@xilinx.com> --- 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(-)