Re: [PATCH 15/19] target/ppc/pmu_book3s_helper: enable counter negative for all PMCs

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

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

On Tue, Aug 10, 2021 at 06:02:41PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/10/21 1:11 AM, David Gibson wrote:
> > On Mon, Aug 09, 2021 at 10:10:53AM -0300, Daniel Henrique Barboza wrote:
> > > All performance monitor counters can trigger a counter negative
> > > condition if the proper MMCR0 bits are set. This patch does that by
> > > doing the following:
> > > 
> > > - pmc_counter_negative_enabled() will check whether a given PMC is
> > > eligible to trigger the counter negative alert;
> > > 
> > > - get_counter_neg_timeout() will return the timeout for the counter
> > > negative condition for a given PMC, or -1 if the PMC is not able to
> > > trigger this alert;
> > > 
> > > - the existing counter_negative_cond_enabled() now must consider the
> > > counter negative bit for PMCs 2-6, MMCR0_PMCjCE;
> > > 
> > > - set_PMU_excp_timer() will now search all existing PMCs for the
> > > shortest counter negative timeout. The shortest timeout will be used to
> > > set the PMC interrupt timer.
> > > 
> > > This change makes most EBB powepc kernel tests pass, validating that the
> > > existing EBB logic is consistent. There are a few tests that aren't passing
> > > due to additional PMU bits and perf events that aren't covered yet.
> > > We'll attempt to cover some of those in the next patches.
> > > 
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > ---
> > >   target/ppc/cpu.h               |  1 +
> > >   target/ppc/pmu_book3s_helper.c | 96 ++++++++++++++++++++++++++++++----
> > >   2 files changed, 87 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > index 5c81d459f4..1aa1fd42af 100644
> > > --- a/target/ppc/cpu.h
> > > +++ b/target/ppc/cpu.h
> > > @@ -351,6 +351,7 @@ typedef struct ppc_v3_pate_t {
> > >   #define MMCR0_FCECE PPC_BIT(38)         /* FC on Enabled Cond or Event */
> > >   #define MMCR0_PMCC  PPC_BITMASK(44, 45) /* PMC Control */
> > >   #define MMCR0_PMC1CE PPC_BIT(48)
> > > +#define MMCR0_PMCjCE PPC_BIT(49)
> > >   #define MMCR1_PMC1SEL_SHIFT (63 - 39)
> > >   #define MMCR1_PMC1SEL PPC_BITMASK(32, 39)
> > > diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> > > index 7126e9b3d5..c5c5ab38c9 100644
> > > --- a/target/ppc/pmu_book3s_helper.c
> > > +++ b/target/ppc/pmu_book3s_helper.c
> > > @@ -143,22 +143,98 @@ static int64_t get_CYC_timeout(CPUPPCState *env, int sprn)
> > >       return muldiv64(remaining_cyc, NANOSECONDS_PER_SECOND, PPC_CPU_FREQ);
> > >   }
> > > -static void set_PMU_excp_timer(CPUPPCState *env)
> > > +static bool pmc_counter_negative_enabled(CPUPPCState *env, int sprn)
> > >   {
> > > -    uint64_t timeout, now;
> > > +    switch (sprn) {
> > > +    case SPR_POWER_PMC1:
> > > +        return env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE;
> > > -    if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_PMC1CE)) {
> > > -        return;
> > > +    case SPR_POWER_PMC2:
> > > +    case SPR_POWER_PMC3:
> > > +    case SPR_POWER_PMC4:
> > > +    case SPR_POWER_PMC5:
> > > +    case SPR_POWER_PMC6:
> > > +        return env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE;
> > > +
> > > +    default:
> > > +        break;
> > >       }
> > > -    switch (get_PMC_event(env, SPR_POWER_PMC1)) {
> > > -    case 0x2:
> > > -        timeout = get_INST_CMPL_timeout(env, SPR_POWER_PMC1);
> > > +    return false;
> > > +}
> > > +
> > > +static int64_t get_counter_neg_timeout(CPUPPCState *env, int sprn)
> > > +{
> > > +    int64_t timeout = -1;
> > > +
> > > +    if (!pmc_counter_negative_enabled(env, sprn)) {
> > > +        return -1;
> > > +    }
> > > +
> > > +    if (env->spr[sprn] >= COUNTER_NEGATIVE_VAL) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    switch (sprn) {
> > > +    case SPR_POWER_PMC1:
> > > +    case SPR_POWER_PMC2:
> > > +    case SPR_POWER_PMC3:
> > > +    case SPR_POWER_PMC4:
> > > +        switch (get_PMC_event(env, sprn)) {
> > > +        case 0x2:
> > > +            timeout = get_INST_CMPL_timeout(env, sprn);
> > > +            break;
> > > +        case 0x1E:
> > > +            timeout = get_CYC_timeout(env, sprn);
> > > +            break;
> > > +        }
> > > +
> > >           break;
> > > -    case 0x1e:
> > > -        timeout = get_CYC_timeout(env, SPR_POWER_PMC1);
> > > +    case SPR_POWER_PMC5:
> > > +        timeout = get_INST_CMPL_timeout(env, sprn);
> > > +        break;
> > > +    case SPR_POWER_PMC6:
> > > +        timeout = get_CYC_timeout(env, sprn);
> > >           break;
> > >       default:
> > > +        break;
> > > +    }
> > > +
> > > +    return timeout;
> > > +}
> > > +
> > > +static void set_PMU_excp_timer(CPUPPCState *env)
> > > +{
> > > +    int64_t timeout = -1;
> > > +    uint64_t now;
> > > +    int i;
> > > +
> > > +    /*
> > > +     * Scroll through all PMCs and check which one is closer to a
> > > +     * counter negative timeout.
> > 
> > I'm wondering if it would be simpler to use a separate timer for each
> > PMC: after all the core timer logic must have already implemented this
> > "who fires first" logic.
> 
> The first draft had 6 timers, one for each PMC. It would make this step to
> determine the lowest timeout unnecessary.
> 
> I gave it up because we would need to pause the running timers of the other
> PMCs when the PPC_INTERRUPT_PMC happens with MMCR0_FCECE (frozen counters on
> Enabled Condition or Event) set. Resuming the timers back would require to
> update the PMCs (which would now have different icount_bases).

Ok, that makes sense.  Might be worth putting some of that rationale
into a comment.

> For our current usage this single timer approach seems less complicated. And
> the ISA allows for multiple/concurrent overflows to be reported at the same
> alert:
> 
> "An enabled condition or event causes a Perfor-
> mance Monitor alert if Performance Monitor alerts
> are enabled by the corresponding “Enable” bit in
> MMCR0. (..) A single Performance Monitor alert may
> reflect multiple enabled conditions and events."
> 
> So a single timer can report more than one c.n. overflows that might happen
> at the same time. E.g. in this timeout logic below, if PMC1 is already
> overflown, we will trigger the PMC interrupt. Since we're updating all
> PMCs, if more counters are also overflown the application can read them
> all in the same interrupt/exception.

Uh.. sure.. but I think you could implement that by sharing the
interrupt latch regardless of whether there are separate counters or
not.

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