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

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

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

On Tue, Aug 10, 2021 at 05:44:18PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/10/21 1:06 AM, David Gibson wrote:
> > 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?
> 
> 
> I'd say that counter negative overflow are edge triggered - we can set any
> starting value for the counters but the counter negative overflow is
> triggered always with the counter reaching 0x8000000.
> 
> If a counter was about to trigger a c.n overflow and then the user set it
> back to 0, that c.n overflow is lost.

Hm.. those aren't actually the edge cases I'm unsure about.  The ones
I'm unclear on are:

1) If you directly set the PMC to a negative value (especially one
that's not 0x80000000 specifically) will that immediately trigger the
interrupt?  In other words is the interrupt triggered by the PMC being
negative for any reason, or only by it transitioning from positive to
negative as a result of counter increments?

2) If a PMC negative interrupt has *already* been generated, but not
yet delivered, will setting the PMC to 0 clear the pending interrupt,
or will it still be delivered?

> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> > 
> > > +    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

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