[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH v3 05/22] target/arm: Reorganize PMCCNTR read, wri
From: |
Aaron Lindsay |
Subject: |
Re: [Qemu-arm] [PATCH v3 05/22] target/arm: Reorganize PMCCNTR read, write, sync |
Date: |
Fri, 13 Apr 2018 09:51:58 -0400 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Apr 12 17:18, Peter Maydell wrote:
> On 16 March 2018 at 20:31, Aaron Lindsay <address@hidden> 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 <address@hidden>
> > ---
> > 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 <address@hidden>
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.