Re: [PATCH 04/19] target/ppc: PMU Book3s basic insns count for pseries TCG

[Richard Henderson]

On 8/9/21 3:10 AM, 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;
>
> - only PMC5 and PMC6 are being set. PMC6 (cycles) is default to 4*insns
> (for cycles per instruction) for now;
>
> - 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.
>
> 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 <danielhb413@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
> @@ -0,0 +1,78 @@
> +/*
> + * PowerPC Book3s PMU emulation helpers for QEMU TCG
> + *
> + *  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 "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();
> +}

So... this will just abort for user-only, which I suppose is fair since you won't be 
setting the other control bits there.

But I don't know what it will do if !icount_enabled().  Certainly not provide you with any 
meaningful value.  Given that icount affects all TCG translation, and is only enabled from 
the qemu command-line, do you really want to rely on that for a niche feature?

> +/* 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;
> +}
...
> +            freeze_PMC5_value(env);
> +            freeze_PMC6_value(env);

Why would you read get_insns() for PMC5 and 6 separately?


r~