| Message ID | 20220103185332.117878-5-danielhb413@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Reorg ppc64 pmu insn counting | expand |
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. r~
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~
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: env->pmc_ins_cnt = ins_cnt; env->pmc_cyc_cnt = cyc_cnt; env->hflags = deposit32(env->hflags, HFLAGS_INSN_CNT, 1, ins_cnt != 0);
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(+)