Re: [PATCH v6 01/10] target/ppc: introduce PMUEventType and PMU overflow timers

[David Gibson]

On Sat, Nov 13, 2021 at 04:02:02PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 11/12/21 00:02, David Gibson wrote:
> > On Mon, Nov 08, 2021 at 07:50:38PM -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,
> > > PMUEventType. This enum represents a Perf event that is being sampled by
> > > a specific counter 'sprn'. Events that aren't available (i.e. no event
> > > was set in MMCR1) 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.
> > > 
> > > Let's also add the required PMU cycle overflow timers. They 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.
> > > 
> > > Two new helper files are created to host this new logic.
> > > cpu_ppc_pmu_init() will init all overflow timers during CPU init time.
> > > 
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > ---
> > >   hw/ppc/spapr_cpu_core.c |  6 +++++
> > >   target/ppc/cpu.h        | 15 +++++++++++
> > >   target/ppc/meson.build  |  1 +
> > >   target/ppc/power8-pmu.c | 57 +++++++++++++++++++++++++++++++++++++++++
> > >   target/ppc/power8-pmu.h | 25 ++++++++++++++++++
> > >   5 files changed, 104 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);
> > 
> > AFAIK, the PMU should be universal in the cpu model, not tied to spapr
> > specifically, so this looks like the wrong place to trigger the
> > initialization.
> 
> 
> Do you mean something like this?

More or less, yes.  However given that this is specifically the POWER8
(and later) PMU I'd name and call the functions accordingly, rather
than "common".

> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 881f39ff76..7503fd76d7 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -45,6 +45,7 @@
>  #include "helper_regs.h"
>  #include "internal.h"
>  #include "spr_tcg.h"
> +#include "power8-pmu.h"
>  /* #define PPC_DEBUG_SPR */
>  /* #define USE_APPLE_GDB */
> @@ -7377,6 +7378,20 @@ static void register_power9_mmu_sprs(CPUPPCState *env)
>  #endif
>  }
> +/*
> + * Initialize PMU counter overflow timers for Power8 and
> + * newer Power chips when using TCG.
> + */
> +static void init_tcg_pmu_common(CPUPPCState *env)
> +{
> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> +    /* Init PMU overflow timers */
> +    if (!kvm_enabled()) {
> +        cpu_ppc_pmu_init(env);
> +    }
> +#endif
> +}
> +
>  static void init_proc_book3s_common(CPUPPCState *env)
>  {
>      register_ne_601_sprs(env);
> @@ -7694,6 +7709,9 @@ static void init_proc_POWER8(CPUPPCState *env)
>      register_sdr1_sprs(env);
>      register_book3s_207_dbg_sprs(env);
> +    /* Common TCG PMU */
> +    init_tcg_pmu_common(env);
> +
>      /* POWER8 Specific Registers */
>      register_book3s_ids_sprs(env);
>      register_rmor_sprs(env);
> @@ -7888,6 +7906,9 @@ static void init_proc_POWER9(CPUPPCState *env)
>      init_proc_book3s_common(env);
>      register_book3s_207_dbg_sprs(env);
> +    /* Common TCG PMU */
> +    init_tcg_pmu_common(env);
> +
>      /* POWER8 Specific Registers */
>      register_book3s_ids_sprs(env);
>      register_amr_sprs(env);
> @@ -8104,6 +8125,9 @@ static void init_proc_POWER10(CPUPPCState *env)
>      init_proc_book3s_common(env);
>      register_book3s_207_dbg_sprs(env);
> +    /* Common TCG PMU */
> +    init_tcg_pmu_common(env);
> +
>      /* POWER8 Specific Registers */
>      register_book3s_ids_sprs(env);
>      register_amr_sprs(env);
> 
> 
> 
> Daniel
> 
> > 
> > > +    }
> > > +
> > >       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 e946da5f3a..04ef9300af 100644
> > > --- a/target/ppc/cpu.h
> > > +++ b/target/ppc/cpu.h
> > > @@ -296,6 +296,15 @@ typedef struct ppc_v3_pate_t {
> > >       uint64_t dw1;
> > >   } ppc_v3_pate_t;
> > > +/* PMU related structs and defines */
> > > +#define PMU_COUNTERS_NUM 6
> > > +#define PMU_TIMERS_NUM   (PMU_COUNTERS_NUM - 1) /* PMC5 doesn't count 
> > > cycles */
> > > +typedef enum {
> > > +    PMU_EVENT_INVALID = 0,
> > > +    PMU_EVENT_CYCLES,
> > > +    PMU_EVENT_INSTRUCTIONS,
> > > +} PMUEventType;
> > > +
> > >   
> > > /*****************************************************************************/
> > >   /* Machine state register bits definition                               
> > >      */
> > >   #define MSR_SF   63 /* Sixty-four-bit mode                            
> > > hflags */
> > > @@ -1191,6 +1200,12 @@ struct CPUPPCState {
> > >       uint32_t tm_vscr;
> > >       uint64_t tm_dscr;
> > >       uint64_t tm_tar;
> > > +
> > > +    /*
> > > +     * Timers used to fire performance monitor alerts
> > > +     * when counting cycles.
> > > +     */
> > > +    QEMUTimer *pmu_cyc_overflow_timers[PMU_TIMERS_NUM];
> > >   };
> > >   #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..3c2f73896f
> > > --- /dev/null
> > > +++ b/target/ppc/power8-pmu.c
> > > @@ -0,0 +1,57 @@
> > > +/*
> > > + * 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;
> > > +
> > > +    for (i = 0; i < PMU_TIMERS_NUM; i++) {
> > > +        env->pmu_cyc_overflow_timers[i] = 
> > > 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