Re: [Qemu-devel] [PATCH v3 05/22] target/arm: Reorganize PMCCNTR read, write, sync
On Apr 12 17:18, Peter Maydell wrote: > On 16 March 2018 at 20:31, Aaron Lindsaywrote: > > 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 > > --- > > 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 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 -- Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [Qemu-devel] [PATCH v3 05/22] target/arm: Reorganize PMCCNTR read, write, sync
On 16 March 2018 at 20:31, Aaron Lindsaywrote: > 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 > --- > 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 thanks -- PMM
[Qemu-devel] [PATCH v3 05/22] target/arm: Reorganize PMCCNTR read, write, sync
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--- 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) +/* + * 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,