Re: [Qemu-devel] [PATCH v7 02/12] target/arm: Reorganize PMCCNTR accesses

[Aaron Lindsay <aaron@os.amperecomputing.com>]

On Nov 16 14:50, Peter Maydell wrote:
> On 5 November 2018 at 18:51, Aaron Lindsay <aaron@os.amperecomputing.com> wrote:
> > pmccntr_read and pmccntr_write contained duplicate code that was already
> > being handled by pmccntr_sync. Consolidate the duplicated code into two
> > functions: pmccntr_op_start and pmccntr_op_finish. Add a companion to
> > c15_ccnt in CPUARMState so that we can simultaneously save both the
> > architectural register value and the last underlying cycle count - this
> > ensures time isn't lost and will also allow us to access the 'old'
> > architectural register value in order to detect overflows in later
> > patches.
> >
> > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> > Signed-off-by: Aaron Lindsay <aclindsa@gmail.com>
> 
> 
> >  /**
> > - * pmccntr_sync
> > + * pmccntr_op_start/finish
> > + * @env: CPUARMState
> > + *
> > + * Convert the counter in the PMCCNTR between its delta form (the typical mode
> > + * when it's enabled) and the guest-visible value. These two calls must always
> > + * surround any action which might affect the counter.
> > + */
> > +void pmccntr_op_start(CPUARMState *env);
> > +void pmccntr_op_finish(CPUARMState *env);
> > +
> > +/**
> > + * pmu_op_start/finish
> >   * @env: CPUARMState
> >   *
> > - * Synchronises the counter in the PMCCNTR. This must always be called twice,
> > - * once before any action that might affect the timer and again afterwards.
> > - * The function is used to swap the state of the register if required.
> > - * This only happens when not in user mode (!CONFIG_USER_ONLY)
> > + * Convert all PMU counters between their delta form (the typical mode when
> > + * they are enabled) and the guest-visible values. These two calls must
> > + * surround any action which might affect the counters, and the return value
> > + * from pmu_op_start must be supplied as the second argument to pmu_op_finish.
> >   */
> > -void pmccntr_sync(CPUARMState *env);
> > +void pmu_op_start(CPUARMState *env);
> > +void pmu_op_finish(CPUARMState *env);
> 
> The comment says that pmu_op_start returns a value and pmu_op_finish
> has a second argument, but the prototypes disagree...

Doh, I updated the comment for pmccntr_op_* but missed pmu_op_*. The
last sentence should end after the comma:

> > + * Convert all PMU counters between their delta form (the typical mode when
> > + * they are enabled) and the guest-visible values. These two calls must
> > + * surround any action which might affect the counters.
> >   */

> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> so if this turns out to be the only problem I can fix it up when
> I apply it to my tree, if you provide suitable new comment text.

I've also updated the comment text in my tree, so if/when there is a v8
of this patchset, it won't be lost.

-Aaron