On Mon, Aug 09, 2021 at 10:10:42AM -0300, Daniel Henrique Barboza wrote: > The PMCC (PMC Control) bit in the MMCR0 register controls whether the > counters PMC5 and PMC6 are being part of the performance monitor > facility in a specific time. If PMCC allows it, PMC5 and PMC6 will > always be used to measure instructions completed and cycles, > respectively. > > This patch adds the barebones of the Book3s PMU logic by enabling > instruction counting, using the icount framework, using the performance > monitor counters 5 and 6. The overall logic goes as follows: > > - a helper is added to control the PMU state on each MMCR0 write. This > allows for the PMU to start/stop as quick as possible;
Um.. why does a helper accomplish that goal?
>
> - only PMC5 and PMC6 are being set. PMC6 (cycles) is default to 4*insns
> (for cycles per instruction) for now;
What's the justification for that value? With a superscalar core, I'd
expect average (median) cycles per instruction to be < 1 a lot of the
time. Mean cycles per instruction could be higher since certain
instructions could take a lot.
> - the intended usage is to freeze the counters by setting MMCR0_FC, do
> any additional setting via MMCR1 (not implemented yet) and setting
> initial counter values, and enable the PMU by zeroing MMCR0_FC. Software
> must freeze counters to read the results - on the fly reading of the PMCs
> will return the starting value of each one.
Is that the way hardware behaves? Or is it just a limitation of this
software implementation? Either is fine, we should just be clear on
what it is.
>
> Since there will be more PMU exclusive code to be added next, let's also
> put the PMU logic in its own helper to keep all in the same place. The
> code is also repetitive and not really extensible to add more PMCs, but
> we'll handle this in the next patches.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb...@gmail.com>
> ---
> target/ppc/cpu.h | 4 ++
> target/ppc/cpu_init.c | 4 +-
> target/ppc/helper.h | 1 +
> target/ppc/meson.build | 1 +
> target/ppc/pmu_book3s_helper.c | 78 ++++++++++++++++++++++++++++++++++
> target/ppc/translate.c | 14 ++++--
> 6 files changed, 97 insertions(+), 5 deletions(-)
> create mode 100644 target/ppc/pmu_book3s_helper.c
>
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 4d96015f81..229abfe7ee 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1175,6 +1175,10 @@ struct CPUPPCState {
> uint32_t tm_vscr;
> uint64_t tm_dscr;
> uint64_t tm_tar;
> +
> + /* PMU registers icount state */
> + uint64_t pmc5_base_icount;
> + uint64_t pmc6_base_icount;
> };
>
> #define SET_FIT_PERIOD(a_, b_, c_, d_) \
> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> index 71062809c8..fce89ee994 100644
> --- a/target/ppc/cpu_init.c
> +++ b/target/ppc/cpu_init.c
> @@ -6822,7 +6822,7 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState
> *env)
> spr_register_kvm(env, SPR_POWER_MMCR0, "MMCR0",
> SPR_NOACCESS, SPR_NOACCESS,
> &spr_read_pmu_generic, &spr_write_pmu_generic,
> - KVM_REG_PPC_MMCR0, 0x00000000);
> + KVM_REG_PPC_MMCR0, 0x80000000);
> spr_register_kvm(env, SPR_POWER_MMCR1, "MMCR1",
> SPR_NOACCESS, SPR_NOACCESS,
> &spr_read_pmu_generic, &spr_write_pmu_generic,
> @@ -6870,7 +6870,7 @@ static void register_book3s_pmu_user_sprs(CPUPPCState
> *env)
> spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
> &spr_read_pmu_ureg, &spr_write_pmu_ureg,
> &spr_read_ureg, &spr_write_ureg,
> - 0x00000000);
> + 0x80000000);
> spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
> &spr_read_pmu_ureg, &spr_write_pmu_ureg,
> &spr_read_ureg, &spr_write_ureg,
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 4076aa281e..5122632784 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -20,6 +20,7 @@ DEF_HELPER_1(rfscv, void, env)
> DEF_HELPER_1(hrfid, void, env)
> DEF_HELPER_2(store_lpcr, void, env, tl)
> DEF_HELPER_2(store_pcr, void, env, tl)
> +DEF_HELPER_2(store_mmcr0, void, env, tl)
> #endif
> DEF_HELPER_1(check_tlb_flush_local, void, env)
> DEF_HELPER_1(check_tlb_flush_global, void, env)
> diff --git a/target/ppc/meson.build b/target/ppc/meson.build
> index b85f295703..bf252ca3ac 100644
> --- a/target/ppc/meson.build
> +++ b/target/ppc/meson.build
> @@ -14,6 +14,7 @@ ppc_ss.add(when: 'CONFIG_TCG', if_true: files(
> 'int_helper.c',
> 'mem_helper.c',
> 'misc_helper.c',
> + 'pmu_book3s_helper.c',
> 'timebase_helper.c',
> 'translate.c',
> ))
> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
> new file mode 100644
> index 0000000000..fe16fcfce0
> --- /dev/null
> +++ b/target/ppc/pmu_book3s_helper.c
I'd prefer to call this just book3s_pmu.c. Or better yet
"powerX_pmu.c", where X is the specific PMU model you're implementing
since IIRC the particulars of the PMU vary quite a lot from POWER7
through to POWER10.
> @@ -0,0 +1,78 @@
> +/*
> + * PowerPC Book3s PMU emulation helpers for QEMU TCG
> + *
> + * Copyright IBM Corp. 2021
> + *
> + * Authors:
> + * Daniel Henrique Barboza <danielhb...@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 "cpu.h"
> +#include "exec/exec-all.h"
> +#include "exec/helper-proto.h"
> +#include "qemu/error-report.h"
> +#include "qemu/main-loop.h"
> +
> +static uint64_t get_insns(void)
> +{
> + return (uint64_t)icount_get_raw();
> +}
> +
> +static uint64_t get_cycles(uint64_t insns)
> +{
> + /* Placeholder value */
> + return insns * 4;
> +}
> +
> +/* PMC5 always count instructions */
> +static void freeze_PMC5_value(CPUPPCState *env)
> +{
> + uint64_t insns = get_insns() - env->pmc5_base_icount;
> +
> + env->spr[SPR_POWER_PMC5] += insns;
> + env->pmc5_base_icount += insns;
> +}
> +
> +/* PMC6 always count cycles */
> +static void freeze_PMC6_value(CPUPPCState *env)
> +{
> + uint64_t insns = get_insns() - env->pmc6_base_icount;
> +
> + env->spr[SPR_POWER_PMC6] += get_cycles(insns);
> + env->pmc6_base_icount += insns;
> +}
> +
> +void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
> +{
> + bool curr_FC = env->spr[SPR_POWER_MMCR0] & MMCR0_FC;
> + bool new_FC = value & MMCR0_FC;
> +
> + /*
> + * In an frozen count (FC) bit change:
> + *
> + * - if PMCs were running (curr_FC = false) and we're freezing
> + * them (new_FC = true), save the PMCs values in the registers.
> + *
> + * - if PMCs were frozen (curr_FC = true) and we're activating
> + * them (new_FC = false), calculate the current icount for each
> + * register to allow for subsequent reads to calculate the insns
> + * passed.
> + */
> + if (curr_FC != new_FC) {
> + if (!curr_FC) {
> + freeze_PMC5_value(env);
> + freeze_PMC6_value(env);
> + } else {
> + uint64_t curr_icount = get_insns();
> +
> + env->pmc5_base_icount = curr_icount;
> + env->pmc6_base_icount = curr_icount;
> + }
> + }
> +
> + env->spr[SPR_POWER_MMCR0] = value;
> +}
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 29b0a340a9..62356cfadf 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -409,8 +409,14 @@ void spr_write_generic(DisasContext *ctx, int sprn, int
> gprn)
>
> void spr_write_pmu_generic(DisasContext *ctx, int sprn, int gprn)
> {
> - /* For now it's just a call to spr_write_generic() */
> - spr_write_generic(ctx, sprn, gprn);
> + switch (sprn) {
> + case SPR_POWER_MMCR0:
> + gen_icount_io_start(ctx);
> + gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
> + break;
> + default:
> + spr_write_generic(ctx, sprn, gprn);
> + }
> }
>
> #if !defined(CONFIG_USER_ONLY)
> @@ -592,6 +598,8 @@ void spr_write_pmu_ureg(DisasContext *ctx, int sprn, int
> gprn)
> t0 = tcg_temp_new();
> t1 = tcg_temp_new();
>
> + gen_icount_io_start(ctx);
> +
> /*
> * Filter out all bits but FC, PMAO, and PMAE, according
> * to ISA v3.1, in 10.4.4 Monitor Mode Control Register 0,
> @@ -603,7 +611,7 @@ void spr_write_pmu_ureg(DisasContext *ctx, int sprn, int
> gprn)
> tcg_gen_andi_tl(t1, t1, ~(MMCR0_FC | MMCR0_PMAO | MMCR0_PMAE));
> /* Keep all other bits intact */
> tcg_gen_or_tl(t1, t1, t0);
> - gen_store_spr(effective_sprn, t1);
> + gen_helper_store_mmcr0(cpu_env, t1);
>
> tcg_temp_free(t0);
> tcg_temp_free(t1);
--
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
