Re: [PATCH v4 05/15] target/ppc: introduce PMU events

[David Gibson]

On Sun, Oct 17, 2021 at 10:01:23PM -0300, Daniel Henrique Barboza wrote:
> This patch starts an IBM Power8+ compatible PMU implementation by adding
> the representation of PMU events that we are going to sample, PMUEvent.
> This struct represents a Perf event, determined by the PMUEventType
> enum, that is being sampled by a specific counter 'sprn'. PMUEvent also
> contains an overflow timer that will be used to trigger cycle overflows
> when cycle events are being sampled. This timer will call
> cpu_ppc_pmu_timer_cb(), which in turn calls fire_PMC_interrupt(). Both
> functions are stubs that will be implemented later on when EBB support
> is added.
> 
> The PMU has 6 PMUEvents all the time, one for each counter. Events that
> aren't available (i.e. the counter isn't running) will be of type
> 'PMU_EVENT_INVALID'. Other types added in this patch are
> PMU_EVENT_CYCLES and PMU_EVENT_INSTRUCTIONS. More types will be added
> later on.
> 
> Two new helper files are created to host this new logic.
> cpu_ppc_pmu_init() will init all PMUEvents during CPU init time.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Ah, sorry, this isn't quite what I had in mind.

> ---
>  hw/ppc/spapr_cpu_core.c |  6 ++++
>  target/ppc/cpu.h        | 22 ++++++++++++
>  target/ppc/meson.build  |  1 +
>  target/ppc/power8-pmu.c | 75 +++++++++++++++++++++++++++++++++++++++++
>  target/ppc/power8-pmu.h | 25 ++++++++++++++
>  5 files changed, 129 insertions(+)
>  create mode 100644 target/ppc/power8-pmu.c
>  create mode 100644 target/ppc/power8-pmu.h
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 58e7341cb7..45abffd891 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -20,6 +20,7 @@
>  #include "target/ppc/kvm_ppc.h"
>  #include "hw/ppc/ppc.h"
>  #include "target/ppc/mmu-hash64.h"
> +#include "target/ppc/power8-pmu.h"
>  #include "sysemu/numa.h"
>  #include "sysemu/reset.h"
>  #include "sysemu/hw_accel.h"
> @@ -266,6 +267,11 @@ static bool spapr_realize_vcpu(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>          return false;
>      }
>  
> +    /* Init PMU interrupt timer (TCG only) */
> +    if (!kvm_enabled()) {
> +        cpu_ppc_pmu_init(env);
> +    }
> +
>      if (!sc->pre_3_0_migration) {
>          vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
>                           cpu->machine_data);
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 33e3a91f6f..21591ec725 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -296,6 +296,26 @@ typedef struct ppc_v3_pate_t {
>      uint64_t dw1;
>  } ppc_v3_pate_t;
>  
> +/* PMU related structs and defines */
> +#define PMU_EVENTS_NUM 6
> +typedef enum {
> +    PMU_EVENT_INVALID = 0,
> +    PMU_EVENT_CYCLES,
> +    PMU_EVENT_INSTRUCTIONS,
> +} PMUEventType;
> +

PMUEventType *is* basically what I had in mind..

> +typedef struct PMUEvent {
> +    int sprn;
> +    PMUEventType type;
> +
> +    /*
> +     * Timer used to fire performance monitor alerts
> +     * when counting cycles.
> +     */
> +    QEMUTimer *cyc_overflow_timer;
> +
> +} PMUEvent;

.. but I don't think the PMUEvent structure is particularly useful.

What I was thinking was essentially a function which takes PMC number
and returns PMUEventType.  That will be pretty complex and messy.  It
would always return CYCLES or INSTRUCTIONS for PMC 5 & 6, for PMC 1..4
it will look at MMCR*, apply whatever sprn specific log it needs to
and come up with an answer (or INVALID, of course).

The messy sprn specific logic is inevitable given the hardware, but
the idea is that this will localize it to one place.  Once you have
that function you can for example just loop through each PMC, get its
event type and do the right things based on that.

> +
>  
> /*****************************************************************************/
>  /* Machine state register bits definition                                    
> */
>  #define MSR_SF   63 /* Sixty-four-bit mode                            hflags 
> */
> @@ -1190,6 +1210,8 @@ struct CPUPPCState {
>      uint32_t tm_vscr;
>      uint64_t tm_dscr;
>      uint64_t tm_tar;
> +
> +    PMUEvent pmu_events[PMU_EVENTS_NUM];

Nor storing this information here persistently.

>  };
>  
>  #define SET_FIT_PERIOD(a_, b_, c_, d_)          \
> diff --git a/target/ppc/meson.build b/target/ppc/meson.build
> index b85f295703..a49a8911e0 100644
> --- a/target/ppc/meson.build
> +++ b/target/ppc/meson.build
> @@ -51,6 +51,7 @@ ppc_softmmu_ss.add(when: 'TARGET_PPC64', if_true: files(
>    'mmu-book3s-v3.c',
>    'mmu-hash64.c',
>    'mmu-radix64.c',
> +  'power8-pmu.c',
>  ))
>  
>  target_arch += {'ppc': ppc_ss}
> diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> new file mode 100644
> index 0000000000..42452b5870
> --- /dev/null
> +++ b/target/ppc/power8-pmu.c
> @@ -0,0 +1,75 @@
> +/*
> + * PMU emulation helpers for TCG IBM POWER chips
> + *
> + *  Copyright IBM Corp. 2021
> + *
> + * Authors:
> + *  Daniel Henrique Barboza      <danielhb413@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "power8-pmu.h"
> +#include "cpu.h"
> +#include "helper_regs.h"
> +#include "exec/exec-all.h"
> +#include "exec/helper-proto.h"
> +#include "qemu/error-report.h"
> +#include "qemu/main-loop.h"
> +#include "hw/ppc/ppc.h"
> +
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +
> +static void fire_PMC_interrupt(PowerPCCPU *cpu)
> +{
> +    CPUPPCState *env = &cpu->env;
> +
> +    if (!(env->spr[SPR_POWER_MMCR0] & MMCR0_EBE)) {
> +        return;
> +    }
> +
> +    /* PMC interrupt not implemented yet */
> +    return;
> +}
> +
> +static void cpu_ppc_pmu_timer_cb(void *opaque)
> +{
> +    PowerPCCPU *cpu = opaque;
> +
> +    fire_PMC_interrupt(cpu);
> +}
> +
> +void cpu_ppc_pmu_init(CPUPPCState *env)
> +{
> +    PowerPCCPU *cpu = env_archcpu(env);
> +    int i;
> +
> +    /*
> +     * PMC1 event first, PMC2 second and so on. PMC5 and PMC6
> +     * PMUEvent are always the same regardless of MMCR1.
> +     */
> +    for (i = 0; i < PMU_EVENTS_NUM; i++) {
> +        PMUEvent *event = &env->pmu_events[i];
> +
> +        event->sprn = SPR_POWER_PMC1 + i;
> +        event->type = PMU_EVENT_INVALID;
> +
> +        if (event->sprn == SPR_POWER_PMC5) {
> +            event->type = PMU_EVENT_INSTRUCTIONS;
> +            continue;
> +        }
> +
> +        if (event->sprn == SPR_POWER_PMC6) {
> +            event->type = PMU_EVENT_CYCLES;
> +        }
> +
> +        event->cyc_overflow_timer =  timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                                  &cpu_ppc_pmu_timer_cb,
> +                                                  cpu);
> +    }
> +}
> +
> +#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
> diff --git a/target/ppc/power8-pmu.h b/target/ppc/power8-pmu.h
> new file mode 100644
> index 0000000000..49a813a443
> --- /dev/null
> +++ b/target/ppc/power8-pmu.h
> @@ -0,0 +1,25 @@
> +/*
> + * PMU emulation helpers for TCG IBM POWER chips
> + *
> + *  Copyright IBM Corp. 2021
> + *
> + * Authors:
> + *  Daniel Henrique Barboza      <danielhb413@gmail.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef POWER8_PMU
> +#define POWER8_PMU
> +
> +#include "qemu/osdep.h"
> +#include "cpu.h"
> +#include "exec/exec-all.h"
> +#include "exec/helper-proto.h"
> +#include "qemu/error-report.h"
> +#include "qemu/main-loop.h"
> +
> +void cpu_ppc_pmu_init(CPUPPCState *env);
> +
> +#endif

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