On Apr 12 17:18, Peter Maydell wrote: > On 16 March 2018 at 20:31, Aaron Lindsay <alind...@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 <alind...@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.mayd...@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 -- 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.