Re: [PATCH 13/19] target/ppc/translate: PMU: handle setting of PMCs while running

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

On 8/10/21 1:06 AM, David Gibson wrote:
> On Mon, Aug 09, 2021 at 10:10:51AM -0300, Daniel Henrique Barboza wrote:
>> The initial PMU support were made under the assumption that the counters
>> would be set before running the PMU and read after either freezing the
>> PMU manually or via a performance monitor alert.
>>
>> Turns out that some EBB powerpc kernel tests set the counters after
>> unfreezing the counters. Setting a PMC value when the PMU is running
>> means that, at that moment, the baseline for calculating the events (set
>> in env->pmu_base_icount) needs to be updated. Updating this baseline
>> means that we need to update all the PMCs with their actual value at
>> that moment. Any existing counter negative timer needs to be discarded
>> an a new one, with the updated values, must be set again.
>>
>> This patch does that via a new 'helper_store_pmc()' that is called in
>> the mtspr() callbacks of the PMU registers, spr_write_pmu_ureg() and
>> spr_write_pmu_generic() in target/ppc/translate.c
>>
>> With this change, EBB powerpc kernel tests such as  'no_handler_test'
>> are now passing.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   target/ppc/helper.h            |  1 +
>>   target/ppc/pmu_book3s_helper.c | 36 ++++++++++++++++++++++++++++++++--
>>   target/ppc/translate.c         | 27 +++++++++++++++++++++++++
>>   3 files changed, 62 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index 5122632784..757665b360 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -21,6 +21,7 @@ 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)
>> +DEF_HELPER_3(store_pmc, void, env, i32, i64)
>>   #endif
>>   DEF_HELPER_1(check_tlb_flush_local, void, env)
>>   DEF_HELPER_1(check_tlb_flush_global, void, env)
>> diff --git a/target/ppc/pmu_book3s_helper.c b/target/ppc/pmu_book3s_helper.c
>> index 58ae65e22b..e7af273cb6 100644
>> --- a/target/ppc/pmu_book3s_helper.c
>> +++ b/target/ppc/pmu_book3s_helper.c
>> @@ -173,7 +173,7 @@ void cpu_ppc_pmu_timer_init(CPUPPCState *env)
>>       env->pmu_intr_timer = timer;
>>   }
>>   
>> -static bool mmcr0_counter_neg_cond_enabled(uint64_t mmcr0)
>> +static bool counter_negative_cond_enabled(uint64_t mmcr0)
> 
> Can you fold this rename into the patch which introduces the function
> please.
> 
>>   {
>>       return mmcr0 & MMCR0_PMC1CE;
>>   }
>> @@ -219,9 +219,41 @@ void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
>>                * Start performance monitor alert timer for counter negative
>>                * events, if needed.
>>                */
>> -            if (mmcr0_counter_neg_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
>> +            if (counter_negative_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
>>                   set_PMU_excp_timer(env);
>>               }
>>           }
>>       }
>>   }
>> +
>> +void helper_store_pmc(CPUPPCState *env, uint32_t sprn, uint64_t value)
>> +{
>> +    bool pmu_frozen = env->spr[SPR_POWER_MMCR0] & MMCR0_FC;
>> +    uint64_t curr_icount, icount_delta;
>> +
>> +    if (pmu_frozen) {
>> +        env->spr[sprn] = value;
>> +        return;
>> +    }
>> +
>> +    curr_icount = (uint64_t)icount_get_raw();
>> +    icount_delta = curr_icount - env->pmu_base_icount;
>> +
>> +    /* Update the counter with the events counted so far */
>> +    update_PMCs(env, icount_delta);
>> +
>> +    /* Set the counter to the desirable value after update_PMCs() */
>> +    env->spr[sprn] = value;
>> +
>> +    /*
>> +     * Delete the current timer and restart a new one with the
>> +     * updated values.
>> +     */
>> +    timer_del(env->pmu_intr_timer);
>> +
>> +    env->pmu_base_icount = curr_icount;
> 
> I'd expect some of this code to be shared with the unfreeze path using
> a helper.  Is there a reason that's not the case?
> 
> Do you also need to deal with any counter interrupts that have already
> been generated by the old counter?  Are the counter overflow events
> edge-triggered or level-triggered?


I'd say that counter negative overflow are edge triggered - we can set any
starting value for the counters but the counter negative overflow is
triggered always with the counter reaching 0x8000000.

If a counter was about to trigger a c.n overflow and then the user set it
back to 0, that c.n overflow is lost.


Thanks,


Daniel


> 
>> +    if (counter_negative_cond_enabled(env->spr[SPR_POWER_MMCR0])) {
>> +        set_PMU_excp_timer(env);
>> +    }
>> +}
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index afc254a03f..3e890cc4d8 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -409,11 +409,25 @@ void spr_write_generic(DisasContext *ctx, int sprn, int gprn)
>>   
>>   void spr_write_pmu_generic(DisasContext *ctx, int sprn, int gprn)
>>   {
>> +    TCGv_i32 t_sprn;
>> +
>>       switch (sprn) {
>>       case SPR_POWER_MMCR0:
>>           gen_icount_io_start(ctx);
>>           gen_helper_store_mmcr0(cpu_env, cpu_gpr[gprn]);
>>           break;
>> +    case SPR_POWER_PMC1:
>> +    case SPR_POWER_PMC2:
>> +    case SPR_POWER_PMC3:
>> +    case SPR_POWER_PMC4:
>> +    case SPR_POWER_PMC5:
>> +    case SPR_POWER_PMC6:
>> +        gen_icount_io_start(ctx);
>> +
>> +        t_sprn = tcg_const_i32(sprn);
>> +        gen_helper_store_pmc(cpu_env, t_sprn, cpu_gpr[gprn]);
>> +        tcg_temp_free_i32(t_sprn);
>> +        break;
>>       default:
>>           spr_write_generic(ctx, sprn, gprn);
>>       }
>> @@ -585,6 +599,7 @@ void spr_write_ureg(DisasContext *ctx, int sprn, int gprn)
>>   void spr_write_pmu_ureg(DisasContext *ctx, int sprn, int gprn)
>>   {
>>       TCGv t0, t1;
>> +    TCGv_i32 t_sprn;
>>       int effective_sprn = sprn + 0x10;
>>   
>>       if (((ctx->spr[SPR_POWER_MMCR0] & MMCR0_PMCC) >> 18) == 0) {
>> @@ -616,6 +631,18 @@ void spr_write_pmu_ureg(DisasContext *ctx, int sprn, int gprn)
>>           tcg_temp_free(t0);
>>           tcg_temp_free(t1);
>>           break;
>> +    case SPR_POWER_PMC1:
>> +    case SPR_POWER_PMC2:
>> +    case SPR_POWER_PMC3:
>> +    case SPR_POWER_PMC4:
>> +    case SPR_POWER_PMC5:
>> +    case SPR_POWER_PMC6:
>> +        gen_icount_io_start(ctx);
>> +
>> +        t_sprn = tcg_const_i32(effective_sprn);
>> +        gen_helper_store_pmc(cpu_env, t_sprn, cpu_gpr[gprn]);
>> +        tcg_temp_free_i32(t_sprn);
>> +        break;
>>       default:
>>           gen_store_spr(effective_sprn, cpu_gpr[gprn]);
>>           break;
>