Re: [PATCH v8 06/10] target/ppc: enable PMU instruction count

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

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

On Tue, Nov 30, 2021 at 07:24:04PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 11/29/21 01:36, David Gibson wrote:
> > On Thu, Nov 25, 2021 at 12:08:13PM -0300, Daniel Henrique Barboza wrote:
> > > The PMU is already counting cycles by calculating time elapsed in
> > > nanoseconds. Counting instructions is a different matter and requires
> > > another approach.
> > > 
> > > This patch adds the capability of counting completed instructions
> > > (Perf event PM_INST_CMPL) by counting the amount of instructions
> > > translated in each translation block right before exiting it.
> > > 
> > > A new pmu_count_insns() helper in translation.c was added to do that.
> > > After verifying that the PMU is running (MMCR0_FC bit not set), call
> > > helper_insns_inc(). This new helper from power8-pmu.c will add the
> > > instructions to the relevant counters. It'll also be responsible for
> > > triggering counter negative overflows as it is already being done with
> > > cycles.
> > > 
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > ---
> > >   target/ppc/cpu.h                 |  1 +
> > >   target/ppc/helper.h              |  1 +
> > >   target/ppc/helper_regs.c         |  4 +++
> > >   target/ppc/power8-pmu-regs.c.inc |  6 +++++
> > >   target/ppc/power8-pmu.c          | 38 ++++++++++++++++++++++++++
> > >   target/ppc/translate.c           | 46 ++++++++++++++++++++++++++++++++
> > >   6 files changed, 96 insertions(+)
> > > 
> > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > index 9b41b022e2..38cd2b5c43 100644
> > > --- a/target/ppc/cpu.h
> > > +++ b/target/ppc/cpu.h
> > > @@ -656,6 +656,7 @@ enum {
> > >       HFLAGS_PR = 14,  /* MSR_PR */
> > >       HFLAGS_PMCC0 = 15,  /* MMCR0 PMCC bit 0 */
> > >       HFLAGS_PMCC1 = 16,  /* MMCR0 PMCC bit 1 */
> > > +    HFLAGS_MMCR0FC = 17, /* MMCR0 FC bit */
> > 
> > Now that the event stuff is a bit more refined, you could narrow this
> > down to specifically marking if any counters are actively counting
> > instructions (not frozen by MMCR0[FC] and not frozen by
> > MMCR0[FC14|FC56] *and* have the right event selected).
> > 
> > Since I suspect the instruction counting instrumentation could be
> > quite expensive (helper call on every tb), that might be worthwhile.
> 
> That was worthwhile. The performance increase is substantial with this
> change, in particular with tests that exercises only cycle events.

Good to know.

> > >       HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
> > >       HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
> > > diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> > > index 94b4690375..d8a23e054a 100644
> > > --- a/target/ppc/helper.h
> > > +++ b/target/ppc/helper.h
> > > @@ -24,6 +24,7 @@ DEF_HELPER_2(store_mmcr0, void, env, tl)
> > >   DEF_HELPER_2(store_mmcr1, void, env, tl)
> > >   DEF_HELPER_3(store_pmc, void, env, i32, i64)
> > >   DEF_HELPER_2(read_pmc, tl, env, i32)
> > > +DEF_HELPER_2(insns_inc, void, env, i32)
> > >   #endif
> > >   DEF_HELPER_1(check_tlb_flush_local, void, env)
> > >   DEF_HELPER_1(check_tlb_flush_global, void, env)
> > > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> > > index 99562edd57..875c2fdfc6 100644
> > > --- a/target/ppc/helper_regs.c
> > > +++ b/target/ppc/helper_regs.c
> > > @@ -115,6 +115,10 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
> > >       if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC1) {
> > >           hflags |= 1 << HFLAGS_PMCC1;
> > >       }
> > > +    if (env->spr[SPR_POWER_MMCR0] & MMCR0_FC) {
> > > +        hflags |= 1 << HFLAGS_MMCR0FC;
> > > +    }
> > > +
> > >   #ifndef CONFIG_USER_ONLY
> > >       if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
> > > diff --git a/target/ppc/power8-pmu-regs.c.inc b/target/ppc/power8-pmu-regs.c.inc
> > > index 25b13ad564..580e4e41b2 100644
> > > --- a/target/ppc/power8-pmu-regs.c.inc
> > > +++ b/target/ppc/power8-pmu-regs.c.inc
> > > @@ -113,6 +113,12 @@ static void write_MMCR0_common(DisasContext *ctx, TCGv val)
> > >        */
> > >       gen_icount_io_start(ctx);
> > >       gen_helper_store_mmcr0(cpu_env, val);
> > > +
> > > +    /*
> > > +     * End the translation block because MMCR0 writes can change
> > > +     * ctx->pmu_frozen.
> > > +     */
> > > +    ctx->base.is_jmp = DISAS_EXIT_UPDATE;
> > >   }
> > >   void spr_write_MMCR0_ureg(DisasContext *ctx, int sprn, int gprn)
> > > diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> > > index 01e0b9b8fc..59d0def79d 100644
> > > --- a/target/ppc/power8-pmu.c
> > > +++ b/target/ppc/power8-pmu.c
> > > @@ -112,6 +112,30 @@ static PMUEventType pmc_get_event(CPUPPCState *env, int sprn)
> > >       return evt_type;
> > >   }
> > > +static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
> > > +{
> > > +    bool overflow_triggered = false;
> > > +    int sprn;
> > > +
> > > +    /* PMC6 never counts instructions */
> > > +    for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC5; sprn++) {
> > > +        if (pmc_get_event(env, sprn) != PMU_EVENT_INSTRUCTIONS) {
> > > +            continue;
> > > +        }
> > > +
> > > +        env->spr[sprn] += num_insns;
> > > +
> > > +        if (env->spr[sprn] >= PMC_COUNTER_NEGATIVE_VAL &&
> > > +            pmc_has_overflow_enabled(env, sprn)) {
> > > +
> > > +            overflow_triggered = true;
> > > +            env->spr[sprn] = PMC_COUNTER_NEGATIVE_VAL;
> > 
> > Does the hardware PMU actually guarantee that the event will happen
> > exactly on the overflow?  Or could you count a few into the negative
> > zone before the event is delivered?
> 
> My understand reading the ISA and from testing with the a real PMU is that yes,
> it'll guarantee that the overflow will happen when the counter reaches exactly
> 0x80000000.

Ok.  We can't quite achieve that in TCG, which makes forcing the
counter to 0x8000000 a reasonable way of faking it.  Might be worth
commenting that that's what this is, though.

> > > +        }
> > > +    }
> > > +
> > > +    return overflow_triggered;
> > > +}
> > > +
> > >   static void pmu_update_cycles(CPUPPCState *env)
> > >   {
> > >       uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > > @@ -258,6 +282,20 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
> > >       return;
> > >   }
> > > +/* This helper assumes that the PMC is running. */
> > > +void helper_insns_inc(CPUPPCState *env, uint32_t num_insns)
> > > +{
> > > +    bool overflow_triggered;
> > > +    PowerPCCPU *cpu;
> > > +
> > > +    overflow_triggered = pmu_increment_insns(env, num_insns);
> > > +
> > > +    if (overflow_triggered) {
> > > +        cpu = env_archcpu(env);
> > > +        fire_PMC_interrupt(cpu);
> > > +    }
> > > +}
> > > +
> > >   static void cpu_ppc_pmu_timer_cb(void *opaque)
> > >   {
> > >       PowerPCCPU *cpu = opaque;
> > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > > index 9960df6e18..ccc83d0603 100644
> > > --- a/target/ppc/translate.c
> > > +++ b/target/ppc/translate.c
> > > @@ -177,6 +177,7 @@ struct DisasContext {
> > >       bool hr;
> > >       bool mmcr0_pmcc0;
> > >       bool mmcr0_pmcc1;
> > > +    bool pmu_frozen;
> > >       ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
> > >       int singlestep_enabled;
> > >       uint32_t flags;
> > > @@ -4170,6 +4171,31 @@ static inline void gen_update_cfar(DisasContext *ctx, target_ulong nip)
> > >   #endif
> > >   }
> > > +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> > 
> > Should this actually be !CONFIG_USER_ONLY?  IIUC there are
> > circumstances where userspace could access the PMU, including
> > instruction counting.
> 
> The user mode will not be able to use the PMU properly because the MMCR1
> reg, used to define the events to be sampled, isn't writable by userpace
> under any circunstance.

Couldn't they use PMC5 without writing MMCR1?

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