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 <danielhb...@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
signature.asc
Description: PGP signature
