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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 11/8/21 17:34, Matheus K. Ferst wrote:
> On 08/11/2021 17:03, Daniel Henrique Barboza wrote:
>> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI.
>>
>> On 11/8/21 16:48, Matheus K. Ferst wrote:
>>> On 01/11/2021 20:56, 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 | 28 ++++++++++++++++++++++++++++
>>>>   target/ppc/power8-pmu.c  | 26 ++++++++++++++++++++++++--
>>>>   3 files changed, 56 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>>>> index 8f545ff482..592031ce54 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      */
>>>>   };
>>>> @@ -2455,6 +2457,7 @@ enum {
>>>>       PPC_INTERRUPT_HMI,            /* Hypervisor Maintenance interrupt    */
>>>>       PPC_INTERRUPT_HDOORBELL,      /* Hypervisor Doorbell interrupt        */
>>>>       PPC_INTERRUPT_HVIRT,          /* Hypervisor virtualization interrupt  */
>>>> +    PPC_INTERRUPT_PMC,            /* Hypervisor virtualization interrupt  */
>>>>   };
>>>>
>>>>   /* Processor Compatibility mask (PCR) */
>>>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>>>> index 7be334e007..88aa0a84f8 100644
>>>> --- a/target/ppc/excp_helper.c
>>>> +++ b/target/ppc/excp_helper.c
>>>> @@ -797,6 +797,22 @@ 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_BESCR] & BESCR_GE) &&
>>>> +            (env->spr[SPR_BESCR] & BESCR_PME)) {
>>>
>>> Do we need to check FSCR[EBB] here?
>>
>> FSCR[EBB] is being checked in the spr_read_ebb* and spr_write_ebb* callbacks
>> in translate.c. These are the read/write callbacks of EBB sprs
>> (register_power8_ebb_sprs() in target/ppc/cpu_init.c).
>>
> 
> I'm not sure if these checks are enough. Looking at section 4.4 of book III, it seems that we need to check again before delivering the exception. E.g., if the timer expires with MSR[PR]=0 and supervisor code disables the facility before it returns to problem state, we would have a spurious exception.

Hmmm yeah, I guess it's not worth assuming that this bit will be intact.

I'll go with your suggestion and add a check for FSCR[EBB] before firing the
interrupt.


Thanks,

Daniel


> 
>>
>> Thanks,
>>
>>
>> Daniel
>>
>>
>>>
>>>> +            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);
>>>> +        }
>>>> +        /*
>>>> +         * This interrupt is handled by userspace. No need
>>>> +         * to proceed.
>>>> +         */
>>>> +        return;
>>>>       default:
>>>>       excp_invalid:
>>>>           cpu_abort(cs, "Invalid PowerPC exception %d. Aborting\n", excp);
>>>> @@ -1044,6 +1060,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.
>>>> +             */
>>>> +            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 aa10233b29..ca3954ff0e 100644
>>>> --- a/target/ppc/power8-pmu.c
>>>> +++ b/target/ppc/power8-pmu.c
>>>> @@ -323,8 +323,30 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
>>>>           return;
>>>>       }
>>>>
>>>> -    /* PMC interrupt not implemented yet */
>>>> -    return;
>>>> +    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);
>>>> +    }
>>>> +
>>>> +    pmu_update_cycles(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. */
>>>> -- 
>>>> 2.31.1
>>>>
>>>
>>>
> 
> 
> Thanks,
> Matheus K. Ferst
> Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
> Analista de Software
> Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>