| Message ID | 1521232280-13089-6-git-send-email-alindsay@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote: > pmccntr_read and pmccntr_write contained duplicate code that was already > being handled by pmccntr_sync. Split pmccntr_sync into pmccntr_op_start > and pmccntr_op_finish, passing the clock value between the two, to avoid > losing time between the two calls. > > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> > --- > target/arm/helper.c | 101 +++++++++++++++++++++++++++++----------------------- > 1 file changed, 56 insertions(+), 45 deletions(-) > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 5634561..6480b80 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -1000,28 +1000,58 @@ static inline bool arm_ccnt_enabled(CPUARMState *env) > > return true; > } > - > -void pmccntr_sync(CPUARMState *env) If you configure your git to use the 'histogram' diff algorithm ("git config --global diff.algorithm histogram", or edit ~/.gitconfig equivalently), does it make git format-patch make less of a mess of this commit ? > +/* > + * Ensure c15_ccnt is the guest-visible count so that operations such as > + * enabling/disabling the counter or filtering, modifying the count itself, > + * etc. can be done logically. This is essentially a no-op if the counter is > + * not enabled at the time of the call. > + * > + * The current cycle count is returned so that it can be passed into the paired > + * pmccntr_op_finish() call which must follow each call to pmccntr_op_start(). > + */ > +uint64_t pmccntr_op_start(CPUARMState *env) > { > - uint64_t temp_ticks; > - > - temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > + uint64_t cycles = 0; > +#ifndef CONFIG_USER_ONLY > + cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > ARM_CPU_FREQ, NANOSECONDS_PER_SECOND); > +#endif Is this ifdef necessary? You have a do-nothing version of pmccntr_op_start() for CONFIG_USER_ONLY later on, so presumably this one is already inside a suitable ifndef. Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Apr 12 17:18, Peter Maydell wrote: > On 16 March 2018 at 20:31, Aaron Lindsay <alindsay@codeaurora.org> wrote: > > pmccntr_read and pmccntr_write contained duplicate code that was already > > being handled by pmccntr_sync. Split pmccntr_sync into pmccntr_op_start > > and pmccntr_op_finish, passing the clock value between the two, to avoid > > losing time between the two calls. > > > > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> > > --- > > target/arm/helper.c | 101 +++++++++++++++++++++++++++++----------------------- > > 1 file changed, 56 insertions(+), 45 deletions(-) > > > > diff --git a/target/arm/helper.c b/target/arm/helper.c > > index 5634561..6480b80 100644 > > --- a/target/arm/helper.c > > +++ b/target/arm/helper.c > > @@ -1000,28 +1000,58 @@ static inline bool arm_ccnt_enabled(CPUARMState *env) > > > > return true; > > } > > - > > -void pmccntr_sync(CPUARMState *env) > > If you configure your git to use the 'histogram' diff algorithm > ("git config --global diff.algorithm histogram", or edit ~/.gitconfig > equivalently), does it make git format-patch make less of a mess > of this commit ? No, it doesn't seem to make much of a difference. > > +/* > > + * Ensure c15_ccnt is the guest-visible count so that operations such as > > + * enabling/disabling the counter or filtering, modifying the count itself, > > + * etc. can be done logically. This is essentially a no-op if the counter is > > + * not enabled at the time of the call. > > + * > > + * The current cycle count is returned so that it can be passed into the paired > > + * pmccntr_op_finish() call which must follow each call to pmccntr_op_start(). > > + */ > > +uint64_t pmccntr_op_start(CPUARMState *env) > > { > > - uint64_t temp_ticks; > > - > > - temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > > + uint64_t cycles = 0; > > +#ifndef CONFIG_USER_ONLY > > + cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > > ARM_CPU_FREQ, NANOSECONDS_PER_SECOND); > > +#endif > > Is this ifdef necessary? You have a do-nothing version of > pmccntr_op_start() for CONFIG_USER_ONLY later on, so presumably > this one is already inside a suitable ifndef. You're right that it is unnecessary as of this patch. A later patch removes the surrounding `#ifndef CONFIG_USER_ONLY` when it is no longer necessary at that level. It would be cleaner to add the #ifndef with smaller scope at the same time - I'll make that change. > Otherwise > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Because I've modified how pmccntr_op_start/finish keep track of the cycles so that they store the counter values differently for v4, I don't feel comfortable adding your Reviewed-by. I apologize for the churn. -Aaron
diff --git a/target/arm/helper.c b/target/arm/helper.c index 5634561..6480b80 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -1000,28 +1000,58 @@ static inline bool arm_ccnt_enabled(CPUARMState *env) return true; } - -void pmccntr_sync(CPUARMState *env) +/* + * Ensure c15_ccnt is the guest-visible count so that operations such as + * enabling/disabling the counter or filtering, modifying the count itself, + * etc. can be done logically. This is essentially a no-op if the counter is + * not enabled at the time of the call. + * + * The current cycle count is returned so that it can be passed into the paired + * pmccntr_op_finish() call which must follow each call to pmccntr_op_start(). + */ +uint64_t pmccntr_op_start(CPUARMState *env) { - uint64_t temp_ticks; - - temp_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), + uint64_t cycles = 0; +#ifndef CONFIG_USER_ONLY + cycles = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), ARM_CPU_FREQ, NANOSECONDS_PER_SECOND); +#endif + + if (arm_ccnt_enabled(env)) { - if (env->cp15.c9_pmcr & PMCRD) { - /* Increment once every 64 processor clock cycles */ - temp_ticks /= 64; + uint64_t eff_cycles = cycles; + if (env->cp15.c9_pmcr & PMCRD) { + /* Increment once every 64 processor clock cycles */ + eff_cycles /= 64; + } + + env->cp15.c15_ccnt = eff_cycles - env->cp15.c15_ccnt; } + return cycles; +} +/* + * If enabled, convert c15_ccnt back into the delta between the clock and the + * guest-visible count. A call to pmccntr_op_finish should follow every call to + * pmccntr_op_start. + */ +void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles) +{ if (arm_ccnt_enabled(env)) { - env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt; + + if (env->cp15.c9_pmcr & PMCRD) { + /* Increment once every 64 processor clock cycles */ + prev_cycles /= 64; + } + + env->cp15.c15_ccnt = prev_cycles - env->cp15.c15_ccnt; } } static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) { - pmccntr_sync(env); + uint64_t saved_cycles = pmccntr_op_start(env); if (value & PMCRC) { /* The counter has been reset */ @@ -1032,26 +1062,16 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri, env->cp15.c9_pmcr &= ~0x39; env->cp15.c9_pmcr |= (value & 0x39); - pmccntr_sync(env); + pmccntr_op_finish(env, saved_cycles); } static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri) { - uint64_t total_ticks; - - if (!arm_ccnt_enabled(env)) { - /* Counter is disabled, do not change value */ - return env->cp15.c15_ccnt; - } - - total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), - ARM_CPU_FREQ, NANOSECONDS_PER_SECOND); - - if (env->cp15.c9_pmcr & PMCRD) { - /* Increment once every 64 processor clock cycles */ - total_ticks /= 64; - } - return total_ticks - env->cp15.c15_ccnt; + uint64_t ret; + uint64_t saved_cycles = pmccntr_op_start(env); + ret = env->cp15.c15_ccnt; + pmccntr_op_finish(env, saved_cycles); + return ret; } static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri, @@ -1068,22 +1088,9 @@ static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri, static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) { - uint64_t total_ticks; - - if (!arm_ccnt_enabled(env)) { - /* Counter is disabled, set the absolute value */ - env->cp15.c15_ccnt = value; - return; - } - - total_ticks = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), - ARM_CPU_FREQ, NANOSECONDS_PER_SECOND); - - if (env->cp15.c9_pmcr & PMCRD) { - /* Increment once every 64 processor clock cycles */ - total_ticks /= 64; - } - env->cp15.c15_ccnt = total_ticks - value; + uint64_t saved_cycles = pmccntr_op_start(env); + env->cp15.c15_ccnt = value; + pmccntr_op_finish(env, saved_cycles); } static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri, @@ -1096,7 +1103,11 @@ static void pmccntr_write32(CPUARMState *env, const ARMCPRegInfo *ri, #else /* CONFIG_USER_ONLY */ -void pmccntr_sync(CPUARMState *env) +uint64_t pmccntr_op_start(CPUARMState *env) +{ +} + +void pmccntr_op_finish(CPUARMState *env, uint64_t prev_cycles) { } @@ -1105,9 +1116,9 @@ void pmccntr_sync(CPUARMState *env) static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) { - pmccntr_sync(env); + uint64_t saved_cycles = pmccntr_op_start(env); env->cp15.pmccfiltr_el0 = value & 0x7E000000; - pmccntr_sync(env); + pmccntr_op_finish(env, saved_cycles); } static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
pmccntr_read and pmccntr_write contained duplicate code that was already being handled by pmccntr_sync. Split pmccntr_sync into pmccntr_op_start and pmccntr_op_finish, passing the clock value between the two, to avoid losing time between the two calls. Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> --- target/arm/helper.c | 101 +++++++++++++++++++++++++++++----------------------- 1 file changed, 56 insertions(+), 45 deletions(-)