Re: [PATCH 13/19] target/ppc/translate: PMU: handle setting of PMCs while running

[David Gibson]

On Mon, Aug 09, 2021 at 10:10:51AM -0300, Daniel Henrique Barboza wrote:
> The initial PMU support were made under the assumption that the counters
> would be set before running the PMU and read after either freezing the
> PMU manually or via a performance monitor alert.
> 
> Turns out that some EBB powerpc kernel tests set the counters after
> unfreezing the counters. Setting a PMC value when the PMU is running
> means that, at that moment, the baseline for calculating the events (set
> in env->pmu_base_icount) needs to be updated. Updating this baseline
> means that we need to update all the PMCs with their actual value at
> that moment. Any existing counter negative timer needs to be discarded
> an a new one, with the updated values, must be set again.
> 
> This patch does that via a new 'helper_store_pmc()' that is called in
> the mtspr() callbacks of the PMU registers, spr_write_pmu_ureg() and
> spr_write_pmu_generic() in target/ppc/translate.c
> 
> With this change, EBB powerpc kernel tests such as  'no_handler_test'
> are now passing.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  target/ppc/helper.h            |  1 +
>  target/ppc/pmu_book3s_helper.c | 36 ++++++++++++++++++++++++++++++++--
>  target/ppc/translate.c         | 27 +++++++++++++++++++++++++
>  3 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 5122632784..757665b360 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -21,6 +21,7 @@ DEF_HELPER_1(hrfid, void, env)
>  DEF_HELPER_2(store_lpcr, void, env, tl)
>  DEF_HELPER_2(store_pcr, void, env, tl)
>  DEF_HELPER_2(store_mmcr0, void, env, tl)
> +DEF_HELPER_3(store_pmc, void, env, i32, i64)
>  #endif
>  DEF_HELPER_1(check_tlb_flush_local, void, env)
>  DEF_HELPER_1(check_tlb_flush_global, void, env)
> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> index 58ae65e22b..e7af273cb6 100644
> --- a/target/ppc/pmu_book3s_helper.c
> +++ b/target/ppc/pmu_book3s_helper.c
> @@ -173,7 +173,7 @@ void cpu_ppc_pmu_timer_init(CPUPPCState *env)
>      env->pmu_intr_timer = timer;
>  }
>  
> -static bool mmcr0_counter_neg_cond_enabled(uint64_t mmcr0)
> +static bool counter_negative_cond_enabled(uint64_t mmcr0)

Can you fold this rename into the patch which introduces the function
please.

>  {
>      return mmcr0 & MMCR0_PMC1CE;
>  }
> @@ -219,9 +219,41 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong 
> value)
>               * Start performance monitor alert timer for counter negative
>               * events, if needed.
>               */
> -            if (mmcr0_counter_neg_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
> +            if (counter_negative_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
>                  set_PMU_excp_timer(env);
>              }
>          }
>      }
>  }
> +
> +void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
> +{
> +    bool pmu_frozen = env->spr[SPR_POWER_MMCR0] & MMCR0_FC;
> +    uint64_t curr_icount, icount_delta;
> +
> +    if (pmu_frozen) {
> +        env->spr[sprn] = value;
> +        return;
> +    }
> +
> +    curr_icount = (uint64_t)icount_get_raw();
> +    icount_delta = curr_icount - env->pmu_base_icount;
> +
> +    /* Update the counter with the events counted so far */
> +    update_PMCs(env, icount_delta);
> +
> +    /* Set the counter to the desirable value after update_PMCs() */
> +    env->spr[sprn] = value;
> +
> +    /*
> +     * Delete the current timer and restart a new one with the
> +     * updated values.
> +     */
> +    timer_del(env->pmu_intr_timer);
> +
> +    env->pmu_base_icount = curr_icount;

I'd expect some of this code to be shared with the unfreeze path using
a helper.  Is there a reason that's not the case?

Do you also need to deal with any counter interrupts that have already
been generated by the old counter?  Are the counter overflow events
edge-triggered or level-triggered?

> +    if (counter_negative_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
> +        set_PMU_excp_timer(env);
> +    }
> +}
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index afc254a03f..3e890cc4d8 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -409,11 +409,25 @@ void spr_write_generic(DisasContext *ctx, int sprn, int 
> gprn)
>  
>  void spr_write_pmu_generic(DisasContext *ctx, int sprn, int gprn)
>  {
> +    TCGv_i32 t_sprn;
> +
>      switch (sprn) {
>      case SPR_POWER_MMCR0:
>          gen_icount_io_start(ctx);
>          gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
>          break;
> +    case SPR_POWER_PMC1:
> +    case SPR_POWER_PMC2:
> +    case SPR_POWER_PMC3:
> +    case SPR_POWER_PMC4:
> +    case SPR_POWER_PMC5:
> +    case SPR_POWER_PMC6:
> +        gen_icount_io_start(ctx);
> +
> +        t_sprn = tcg_const_i32(sprn);
> +        gen_helper_store_pmc(cpu_env, t_sprn, cpu_gpr[gprn]);
> +        tcg_temp_free_i32(t_sprn);
> +        break;
>      default:
>          spr_write_generic(ctx, sprn, gprn);
>      }
> @@ -585,6 +599,7 @@ void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
>  void spr_write_pmu_ureg(DisasContext *ctx, int sprn, int gprn)
>  {
>      TCGv t0, t1;
> +    TCGv_i32 t_sprn;
>      int effective_sprn = sprn + 0x10;
>  
>      if (((ctx->spr[SPR_POWER_MMCR0] & MMCR0_PMCC) >> 18) == 0) {
> @@ -616,6 +631,18 @@ void spr_write_pmu_ureg(DisasContext *ctx, int sprn, int 
> gprn)
>          tcg_temp_free(t0);
>          tcg_temp_free(t1);
>          break;
> +    case SPR_POWER_PMC1:
> +    case SPR_POWER_PMC2:
> +    case SPR_POWER_PMC3:
> +    case SPR_POWER_PMC4:
> +    case SPR_POWER_PMC5:
> +    case SPR_POWER_PMC6:
> +        gen_icount_io_start(ctx);
> +
> +        t_sprn = tcg_const_i32(effective_sprn);
> +        gen_helper_store_pmc(cpu_env, t_sprn, cpu_gpr[gprn]);
> +        tcg_temp_free_i32(t_sprn);
> +        break;
>      default:
>          gen_store_spr(effective_sprn, cpu_gpr[gprn]);
>          break;

-- 
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

signature.asc
Description: PGP signature