Re: [PATCH 05/19] target/ppc/pmu_book3s_helper.c: eliminate code repetition

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3536 bytes --]

On Mon, Aug 09, 2021 at 10:10:43AM -0300, Daniel Henrique Barboza wrote:
> We don't need a base_icount value in CPUPPCState for each PMC. All the
> calculation done after freeze will use the same base start value. Use a
> single 'pmu_base_icount' attribute that can be use to all PMCs.
> 
> Likewise, the freeze count operations are going to be done for all
> available PMCs, so eliminate both freeze_PMC5_value() and
> freeze_PMC6_value() and use the new update_PMCs_on_freeze() that will
> update all PMCs.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Please fold this simplification into the initial patch.

> ---
>  target/ppc/cpu.h               |  8 +++++---
>  target/ppc/pmu_book3s_helper.c | 33 +++++++++++++--------------------
>  2 files changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 229abfe7ee..8cea8f2aca 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1176,9 +1176,11 @@ struct CPUPPCState {
>      uint64_t tm_dscr;
>      uint64_t tm_tar;
>  
> -    /* PMU registers icount state */
> -    uint64_t pmc5_base_icount;
> -    uint64_t pmc6_base_icount;
> +    /*
> +     * PMU icount base value used by the PMU to calculate
> +     * instructions and cycles.
> +     */
> +    uint64_t pmu_base_icount;
>  };
>  
>  #define SET_FIT_PERIOD(a_, b_, c_, d_)          \
> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> index fe16fcfce0..0994531f65 100644
> --- a/target/ppc/pmu_book3s_helper.c
> +++ b/target/ppc/pmu_book3s_helper.c
> @@ -28,22 +28,19 @@ static uint64_t get_cycles(uint64_t insns)
>      return insns * 4;
>  }
>  
> -/* PMC5 always count instructions */
> -static void freeze_PMC5_value(CPUPPCState *env)
> -{
> -    uint64_t insns = get_insns() - env->pmc5_base_icount;
> -
> -    env->spr[SPR_POWER_PMC5] += insns;
> -    env->pmc5_base_icount += insns;
> -}
> -
> -/* PMC6 always count cycles */
> -static void freeze_PMC6_value(CPUPPCState *env)
> +/*
> + * Set all PMCs values after a PMU freeze via MMCR0_FC.
> + *
> + * There is no need to update the base icount of each PMC since
> + * the PMU is not running.
> + */
> +static void update_PMCs_on_freeze(CPUPPCState *env)
>  {
> -    uint64_t insns = get_insns() - env->pmc6_base_icount;
> +    uint64_t curr_icount = get_insns();
>  
> -    env->spr[SPR_POWER_PMC6] += get_cycles(insns);
> -    env->pmc6_base_icount += insns;
> +    env->spr[SPR_POWER_PMC5] += curr_icount - env->pmu_base_icount;
> +    env->spr[SPR_POWER_PMC6] += get_cycles(curr_icount -
> +                                           env->pmu_base_icount);
>  }
>  
>  void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> @@ -64,13 +61,9 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
>       */
>      if (curr_FC != new_FC) {
>          if (!curr_FC) {
> -            freeze_PMC5_value(env);
> -            freeze_PMC6_value(env);
> +            update_PMCs_on_freeze(env);
>          } else {
> -            uint64_t curr_icount = get_insns();
> -
> -            env->pmc5_base_icount = curr_icount;
> -            env->pmc6_base_icount = curr_icount;
> +            env->pmu_base_icount = get_insns();
>          }
>      }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]