Re: [PATCH v2 4/5] target/ppc: keep ins_cnt/cyc_cnt cleared if MMCR0_FC is set

[Daniel Henrique Barboza]

On 1/3/22 18:38, Richard Henderson wrote:
> On 1/3/22 10:53 AM, Daniel Henrique Barboza wrote:
> > pmu_update_summaries() is not considering the case where the PMU can be
> > turned off (i.e. stop counting all events) if MMCR0_FC is set,
> > regardless of the other frozen counter bits state. This use case was
> > covered in the late pmc_get_event(), via the also gone pmc_is_inactive(),
> > that would return an invalid event if MMCR0_FC was set.
> >
> > This use case is exercised by the back_to_back_ebbs_test Linux kernel
> > selftests [1]. As it is today, after enabling EBB exceptions, the test
> > will report an additional event-based branch being taken and will fail.
> > Other tests, such as cycles_test.c, will report additional cycles being
> > calculated in the counters because we're not freezing the PMU quick
> > enough.
> >
> > Fix pmu_update_summaries() by keeping env->ins_cnt and env->cyc_cnt
> > cleared when MMCR0_FC is set.
> >
> > [1] tools/testing/selftests/powerpc/pmu/ebb/back_to_back_ebbs_test.c
> >
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >   target/ppc/power8-pmu.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/target/ppc/power8-pmu.c b/target/ppc/power8-pmu.c
> > index 7fc7d91109..73713ca2a3 100644
> > --- a/target/ppc/power8-pmu.c
> > +++ b/target/ppc/power8-pmu.c
> > @@ -40,6 +40,10 @@ void pmu_update_summaries(CPUPPCState *env)
> >       int ins_cnt = 0;
> >       int cyc_cnt = 0;
> > +    if (mmcr0 & MMCR0_FC) {
> > +        goto hflags_calc;
> > +    }
> > +
> >       if (!(mmcr0 & MMCR0_FC14) && mmcr1 != 0) {
> >           target_ulong sel;
> > @@ -71,6 +75,7 @@ void pmu_update_summaries(CPUPPCState *env)
> >       ins_cnt |= !(mmcr0 & MMCR0_FC56) << 5;
> >       cyc_cnt |= !(mmcr0 & MMCR0_FC56) << 6;
> > + hflags_calc:
>
> Good catch, but should be folded into patch 1 to avoid bisection breakage.

Fair point. Given that you have a suggestion in patch 5 as well I'll send a v3.


Thanks,


Daniel



> r~