| Message ID | 7ebb7e43a45137c7607c10b1a78fe7bfc17d186e.1390188951.git.alistair.francis@xilinx.com |
|---|---|
| State | New |
| Headers | show |
Same subject-prefix as commented by Andreas. Should be "target-arm". On Wed, Jan 22, 2014 at 9:40 AM, 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 > 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? > + } > + > + 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. > + } > + > 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? > + 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. 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 > >
Thanks, I'll rename the patch in the next version On Thu, Jan 23, 2014 at 11:56 PM, Peter Crosthwaite <peter.crosthwaite@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.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 >> 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 >> >> >
On Tue, Jan 28, 2014 at 11:31 AM, Alistair Francis <alistair.francis@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.crosthwaite@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.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 >>> 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 >>> >>> >> >
On Tue, Jan 28, 2014 at 12:01 PM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Tue, Jan 28, 2014 at 11:31 AM, Alistair Francis > <alistair.francis@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.crosthwaite@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.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 >>>> 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 >>>> >>>> >>> >> >
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; + } + + 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; + } else { + diff = temp_ticks - env->cp15.c15_ccnt; + } + env->cp15.c15_ccnt += diff; + } + 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) & + 0xFFFFFFFF); + + if (env->cp15.c9_pmcr & PMCRDP) { + /* Increment once every 64 processor clock cycles */ + *value = (total_ticks/64) - env->cp15.c15_ccnt; + } else { + *value = total_ticks - env->cp15.c15_ccnt; + } + + 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), + .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),
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 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(-)