Re: [PATCH v9 09/10] target/ppc: PMU Event-Based exception support

["Cédric Le Goater" <clg@kaod.org>]

Hello,

On 12/1/21 16:17, Daniel Henrique Barboza wrote:
> From: Gustavo Romero <gromero@linux.ibm.com>
> 
> Following up the rfebb implementation, this patch adds the EBB exception
> support that are triggered by Performance Monitor alerts. This exception
> occurs when an enabled PMU condition or event happens and both MMCR0_EBE
> and BESCR_PME are set.
> 
> The supported PM alerts will consist of counter negative conditions of
> the PMU counters. This will be achieved by a timer mechanism that will
> predict when a counter becomes negative. The PMU timer callback will set
> the appropriate bits in MMCR0 and fire a PMC interrupt. The EBB
> exception code will then set the appropriate BESCR bits, set the next
> instruction pointer to the address pointed by the return register
> (SPR_EBBRR), and redirect execution to the handler (pointed by
> SPR_EBBHR).
> 
> CC: Gustavo Romero <gustavo.romero@linaro.org>
> Signed-off-by: Gustavo Romero <gromero@linux.ibm.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>   target/ppc/cpu.h         |  5 ++++-
>   target/ppc/excp_helper.c | 29 +++++++++++++++++++++++++++++
>   target/ppc/power8-pmu.c  | 40 ++++++++++++++++++++++++++++++++++++++--
>   3 files changed, 71 insertions(+), 3 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 741b8baf4c..8e0e6319ee 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -129,8 +129,10 @@ enum {
>       /* ISA 3.00 additions */
>       POWERPC_EXCP_HVIRT    = 101,
>       POWERPC_EXCP_SYSCALL_VECTORED = 102, /* scv exception                     */
> +    POWERPC_EXCP_EBB = 103, /* Event-based branch exception                  */
> +
>       /* EOL                                                                   */
> -    POWERPC_EXCP_NB       = 103,
> +    POWERPC_EXCP_NB       = 104,
>       /* QEMU exceptions: special cases we want to stop translation            */
>       POWERPC_EXCP_SYSCALL_USER = 0x203, /* System call in user mode only      */
>   };
> @@ -2452,6 +2454,7 @@ enum {
>       PPC_INTERRUPT_HMI,            /* Hypervisor Maintenance interrupt    */
>       PPC_INTERRUPT_HDOORBELL,      /* Hypervisor Doorbell interrupt        */
>       PPC_INTERRUPT_HVIRT,          /* Hypervisor virtualization interrupt  */
> +    PPC_INTERRUPT_PMC,            /* PMU interrupt  */

This is referred as a Performance Monitor event-based exception in ISA.
PPC_INTERRUPT_PM_EBB would be a better name I think.

We also have :

   Load Monitored event-based exceptions
   External event-based exceptions



>   };
>   
>   /* Processor Compatibility mask (PCR) */
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 7ead32279c..a26d266fe6 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -799,6 +799,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>           cpu_abort(cs, "Non maskable external exception "
>                     "is not implemented yet !\n");
>           break;
> +    case POWERPC_EXCP_EBB:       /* Event-based branch exception             */
> +        if ((env->spr[SPR_FSCR] & (1ull << FSCR_EBB)) &&
> +            (env->spr[SPR_BESCR] & BESCR_GE) &&
> +            (env->spr[SPR_BESCR] & BESCR_PME)) {


Shouldn't we check these conditions to gate the EBB before sending it ?
What about the HFSCR_EBB case ?


> +            target_ulong nip;
> +
> +            env->spr[SPR_BESCR] &= ~BESCR_GE;   /* Clear GE */
> +            env->spr[SPR_BESCR] |= BESCR_PMEO;  /* Set PMEO */
> +            env->spr[SPR_EBBRR] = env->nip;     /* Save NIP for rfebb insn */
> +            nip = env->spr[SPR_EBBHR];          /* EBB handler */
> +            powerpc_set_excp_state(cpu, nip, env->msr);
> +        }


I am in favor of not adding anything more under the powerpc_excp() routine.
Can we add a helper to isolate the EBB case ? I have the feeling it could
become big.


> +        /*
> +         * This interrupt is handled by userspace. No need
> +         * to proceed.
> +         */
> +        return;
>       default:
>       excp_invalid:
>           cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
> @@ -1046,6 +1063,18 @@ static void ppc_hw_interrupt(CPUPPCState *env)
>               powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_THERM);
>               return;
>           }
> +        /* PMC -> Event-based branch exception */
> +        if (env->pending_interrupts & (1 << PPC_INTERRUPT_PMC)) {
> +            /*
> +             * Performance Monitor event-based exception can only
> +             * occur in problem state.
> +             */

Yes. is the PM EBB exception gated by EE also ?

Thanks,

C.


> +            if (msr_pr == 1) {
> +                env->pending_interrupts &= ~(1 << PPC_INTERRUPT_PMC);
> +                powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_EBB);
> +                return;
> +            }
> +        }
>       }
>   
>       if (env->resume_as_sreset) {
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> index 08d1902cd5..279b824c3f 100644
> --- a/target/ppc/power8-pmu.c
> +++ b/target/ppc/power8-pmu.c
> @@ -297,6 +297,20 @@ void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
>       pmc_update_overflow_timer(env, sprn);
>   }
>   
> +static void pmu_delete_timers(CPUPPCState *env)
> +{
> +    QEMUTimer *pmc_overflow_timer;
> +    int sprn;
> +
> +    for (sprn = SPR_POWER_PMC1; sprn <= SPR_POWER_PMC6; sprn++) {
> +        pmc_overflow_timer = get_cyc_overflow_timer(env, sprn);
> +
> +        if (pmc_overflow_timer) {
> +            timer_del(pmc_overflow_timer);
> +        }
> +    }
> +}
> +
>   static void fire_PMC_interrupt(PowerPCCPU *cpu)
>   {
>       CPUPPCState *env = &cpu->env;
> @@ -305,8 +319,30 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
>           return;
>       }
>   
> -    /* PMC interrupt not implemented yet */
> -    return;
> +    pmu_update_cycles(env);
> +
> +    if (env->spr[SPR_POWER_MMCR0] & MMCR0_FCECE) {
> +        env->spr[SPR_POWER_MMCR0] &= ~MMCR0_FCECE;
> +        env->spr[SPR_POWER_MMCR0] |= MMCR0_FC;
> +
> +        /* Changing MMCR0_FC demands a new hflags compute */
> +        hreg_compute_hflags(env);
> +
> +        /*
> +         * Delete all pending timers if we need to freeze
> +         * the PMC. We'll restart them when the PMC starts
> +         * running again.
> +         */
> +        pmu_delete_timers(env);
> +    }
> +
> +    if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMAE) {
> +        env->spr[SPR_POWER_MMCR0] &= ~MMCR0_PMAE;
> +        env->spr[SPR_POWER_MMCR0] |= MMCR0_PMAO;
> +    }
> +
> +    /* Fire the PMC hardware exception */
> +    ppc_set_irq(cpu, PPC_INTERRUPT_PMC, 1);
>   }
>   
>   /* This helper assumes that the PMC is running. */
>