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

[David Gibson]

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.

> +     */
> +    for (i = SPR_POWER_PMC1; i <= SPR_POWER_PMC6; i++) {
> +        int64_t curr_timeout = get_counter_neg_timeout(env, i);
> +
> +        if (curr_timeout == -1) {
> +            continue;
> +        }
> +
> +        if (curr_timeout == 0) {
> +            timeout = 0;
> +            break;
> +        }
> +
> +        if (timeout == -1 || timeout > curr_timeout) {
> +            timeout = curr_timeout;
> +        }
> +    }
> +
> +    /*
> +     * This can happen if counter negative conditions were enabled
> +     * without any events to be sampled.
> +     */
> +    if (timeout == -1) {
>          return;
>      }
>  
> @@ -204,7 +280,7 @@ void cpu_ppc_pmu_timer_init(CPUPPCState *env)
>  
>  static bool counter_negative_cond_enabled(uint64_t mmcr0)
>  {
> -    return mmcr0 & MMCR0_PMC1CE;
> +    return mmcr0 & (MMCR0_PMC1CE | MMCR0_PMCjCE);
>  }
>  
>  void helper_store_mmcr0(CPUPPCState *env, target_ulong value)

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