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

2022-01-03 Thread 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 
---
  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~

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

2022-01-03 Thread Richard Henderson 

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 
---
  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~

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

2022-01-03 Thread Daniel Henrique Barboza 
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 
---
 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:
 env->pmc_ins_cnt = ins_cnt;
 env->pmc_cyc_cnt = cyc_cnt;
 env->hflags = deposit32(env->hflags, HFLAGS_INSN_CNT, 1, ins_cnt != 0);
-- 
2.33.1