Re: [PATCH v2 04/16] target/ppc: PMU basic cycle count for pseries TCG

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

On 8/25/21 2:19 AM, David Gibson wrote:
> On Tue, Aug 24, 2021 at 01:30:20PM -0300, Daniel Henrique Barboza wrote:
>> This patch adds the barebones of the PMU logic by enabling cycle
>> counting, done via the performance monitor counter 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 the frozen counter bit (MMCR0_FC)
>> is cleared or set;
>>
>> - MMCR0 reg initial value is set to 0x80000000 (MMCR0_FC set) to avoid
>> having to spin the PMU right at system init;
>>
>> - the intended usage is to freeze the counters by setting MMCR0_FC, do
>> any additional setting of events to be counted via MMCR1 (not
>> implemented yet) 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.
> 
> Ok, I like how this is simpler than the previous version.  Since qemu
> is not a cycle-accurate simulator, we basically have a choice in
> emulating the PMU:
>     1) we can maintain the illusion that the cpu clock goes at the
>        advertised speed w.r.t. real time
> or 2) we can maintain the illusion that instructions complete roughly
>        as fast as we expect w.r.t. the cpu clock
> 
> We can't do both at the same time.  Well... in theory we kind of could
> (on a time averaged basis at least) if we decouple the guest's notion
> of "real time" from actual real time.  But that introduces a bunch of
> other complications, so I don't think we want to go there.
> 
> Since it's simpler, I think (1) is probably the way to go, which is
> what you've done here.
> 
>> Since there will be more PMU exclusive code to be added next, put the
>> PMU logic in its own helper to keep all in the same place. The name of
>> the new helper file, power8_pmu.c, is an indicative that the PMU logic
>> has been tested with the IBM POWER chip family, POWER8 being the oldest
>> version tested. This doesn't mean that this PMU logic will break with
>> any other PPC64 chip that implements Book3s, but since we can't assert
>> that this PMU will work with all available Book3s emulated processors
>> we're choosing to be explicit.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   target/ppc/cpu.h        |  6 ++++
>>   target/ppc/cpu_init.c   |  6 ++--
>>   target/ppc/helper.h     |  1 +
>>   target/ppc/meson.build  |  1 +
>>   target/ppc/power8_pmu.c | 74 +++++++++++++++++++++++++++++++++++++++++
>>   target/ppc/spr_tcg.h    |  1 +
>>   target/ppc/translate.c  | 17 +++++++++-
>>   7 files changed, 102 insertions(+), 4 deletions(-)
>>   create mode 100644 target/ppc/power8_pmu.c
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 739005ba29..6878d950de 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1176,6 +1176,12 @@ struct CPUPPCState {
>>       uint32_t tm_vscr;
>>       uint64_t tm_dscr;
>>       uint64_t tm_tar;
>> +
>> +    /*
>> +     * PMU base time value used by the PMU to calculate
>> +     * running cycles.
>> +     */
>> +    uint64_t pmu_base_time;
>>   };
>>   
>>   #define SET_FIT_PERIOD(a_, b_, c_, d_)          \
>> diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
>> index 860716da18..71f052b052 100644
>> --- a/target/ppc/cpu_init.c
>> +++ b/target/ppc/cpu_init.c
>> @@ -6821,8 +6821,8 @@ static void register_book3s_pmu_sup_sprs(CPUPPCState *env)
>>   {
>>       spr_register_kvm(env, SPR_POWER_MMCR0, "MMCR0",
>>                        SPR_NOACCESS, SPR_NOACCESS,
>> -                     &spr_read_generic, &spr_write_generic,
>> -                     KVM_REG_PPC_MMCR0, 0x00000000);
>> +                     &spr_read_generic, &spr_write_MMCR0_generic,
> 
> s/spr_write_MMCR0_generic/spr_write_MMCR0/
> 
> The generic refers to the fact that it's covering any register which
> will just read back whatever value is written to it.  Now that you're
> applying MMCR0 specific logic, it's not generic any more.
> 
>> +                     KVM_REG_PPC_MMCR0, 0x80000000);
>>       spr_register_kvm(env, SPR_POWER_MMCR1, "MMCR1",
>>                        SPR_NOACCESS, SPR_NOACCESS,
>>                        &spr_read_generic, &spr_write_generic,
>> @@ -6870,7 +6870,7 @@ static void register_book3s_pmu_user_sprs(CPUPPCState *env)
>>       spr_register(env, SPR_POWER_UMMCR0, "UMMCR0",
>>                    &spr_read_MMCR0_ureg, &spr_write_MMCR0_ureg,
>>                    &spr_read_ureg, &spr_write_ureg,
>> -                 0x00000000);
>> +                 0x80000000);
>>       spr_register(env, SPR_POWER_UMMCR1, "UMMCR1",
>>                    &spr_read_ureg, SPR_NOACCESS,
>>                    &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..278ce07da9 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',
>> +  'power8_pmu.c',
>>     'timebase_helper.c',
>>     'translate.c',
>>   ))
>> diff --git a/target/ppc/power8_pmu.c b/target/ppc/power8_pmu.c
>> new file mode 100644
>> index 0000000000..47de38a99e
>> --- /dev/null
>> +++ b/target/ppc/power8_pmu.c
>> @@ -0,0 +1,74 @@
>> +/*
>> + * 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 "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"
>> +
>> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>> +
>> +static void update_PMC_PM_CYC(CPUPPCState *env, int sprn,
>> +                              uint64_t time_delta)
>> +{
>> +    /*
>> +     * The pseries and pvn clock runs at 1Ghz, meaning that
>> +     * 1 nanosec equals 1 cycle.
>> +     */
>> +    env->spr[sprn] += time_delta;
>> +}
>> +
>> +static void update_cycles_PMCs(CPUPPCState *env)
>> +{
>> +    uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> +    uint64_t time_delta = now - env->pmu_base_time;
>> +
>> +    update_PMC_PM_CYC(env, SPR_POWER_PMC6, time_delta);
>> +}
>> +
>> +void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
>> +{
>> +    target_ulong curr_value = env->spr[SPR_POWER_MMCR0];
>> +    bool curr_FC = curr_value & MMCR0_FC;
>> +    bool new_FC = value & MMCR0_FC;
>> +
>> +    env->spr[SPR_POWER_MMCR0] = value;
>> +
>> +    /* MMCR0 writes can change HFLAGS_PMCCCLEAR */
>> +    if ((curr_value & MMCR0_PMCC) != (value & MMCR0_PMCC)) {
>> +        hreg_compute_hflags(env);
>> +    }
>> +
>> +    /*
>> +     * 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), set the new base_time for future cycle
>> +     * calculations.
>> +     */
>> +    if (curr_FC != new_FC) {
>> +        if (!curr_FC) {
>> +            update_cycles_PMCs(env);
>> +        } else {
>> +            env->pmu_base_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> +        }
>> +    }
>> +}
>> +
>> +#endif /* defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY) */
>> diff --git a/target/ppc/spr_tcg.h b/target/ppc/spr_tcg.h
>> index 5c383dae3d..2c5b056fc1 100644
>> --- a/target/ppc/spr_tcg.h
>> +++ b/target/ppc/spr_tcg.h
>> @@ -25,6 +25,7 @@
>>   void spr_noaccess(DisasContext *ctx, int gprn, int sprn);
>>   void spr_read_generic(DisasContext *ctx, int gprn, int sprn);
>>   void spr_write_generic(DisasContext *ctx, int sprn, int gprn);
>> +void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn);
>>   void spr_read_xer(DisasContext *ctx, int gprn, int sprn);
>>   void spr_write_xer(DisasContext *ctx, int sprn, int gprn);
>>   void spr_read_lr(DisasContext *ctx, int gprn, int sprn);
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index b48eec83e3..e4f75ba380 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -401,6 +401,19 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
>>       spr_store_dump_spr(sprn);
>>   }
>>   
>> +#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
>> +void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn)
>> +{
>> +    gen_icount_io_start(ctx);
> 
> Since you're not using icount any more, do you still need these?

I believe we do. We're not using icount, but we should support icount guests.
And if an icount guest runs this code we'll have a 'bad icount read' error
because we're doing operations with the virtual clock.

I explained this rationale in patch 14. I should've mentioned that in this patch
instead.



> 
>> +    gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
>> +}
>> +#else
>> +void spr_write_MMCR0_generic(DisasContext *ctx, int sprn, int gprn)
>> +{
>> +    spr_write_generic(ctx, sprn, gprn);
>> +}
>> +#endif
>> +
>>   #if !defined(CONFIG_USER_ONLY)
>>   void spr_write_generic32(DisasContext *ctx, int sprn, int gprn)
>>   {
>> @@ -609,6 +622,8 @@ void spr_write_MMCR0_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,
>> @@ -620,7 +635,7 @@ void spr_write_MMCR0_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(SPR_POWER_MMCR0, t1);
>> +    gen_helper_store_mmcr0(cpu_env, t1);
> 
> Do you need this change?  Won't the gen_store_spr() basically do the
> same thing as the gen_helpersince spr_write_MMCR0() expands to a
> gen_helper anyway?


Never realized that. I'll try it out.


Daniel


> 
>>   
>>       tcg_temp_free(t0);
>>       tcg_temp_free(t1);
>