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 <danielhb...@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
