diff mbox series

[v3,05/22] target/arm: Reorganize PMCCNTR read, write, sync

Message ID 1521232280-13089-6-git-send-email-alindsay@codeaurora.org
State New
Headers show
Series More fully implement ARM PMUv3 | expand

Commit Message

Aaron Lindsay March 16, 2018, 8:31 p.m. UTC 
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(-)

Comments

Peter Maydell April 12, 2018, 4:18 p.m. UTC | #1 
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
Aaron Lindsay April 13, 2018, 1:51 p.m. UTC | #2 
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 mbox series

Patch

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,