[kvm-unit-tests PATCH 0/6] arm: pmu: Fix random failures of pmu-chain-promotion

[kvm-unit-tests PATCH 0/6] arm: pmu: Fix random failures of pmu-chain-promotion

[Eric Auger]

On some HW (ThunderXv2), some random failures of
pmu-chain-promotion test can be observed.

pmu-chain-promotion is composed of several subtests
which run 2 mem_access loops. The initial value of
the counter is set so that no overflow is expected on
the first loop run and overflow is expected on the second.
However it is observed that sometimes we get an overflow
on the first run. It looks related to some variability of
the mem_acess count. This variability is observed on all
HW I have access to, with different span though. On
ThunderX2 HW it looks the margin that is currently taken
is too small and we regularly hit failure.

although the first goal of this series is to increase
the count/margin used in those tests, it also attempts
to improve the pmu-chain-promotion logs, add some barriers
in the mem-access loop, clarify the chain counter
enable/disable sequence.

A new 'pmu-memaccess-reliability' is also introduced to
detect issues with MEM_ACCESS event variability and make
the debug easier.

Obviously one can wonder if this variability is something normal
and does not hide any other bug. I hope this series will raise
additional discussions about this.

https://github.com/eauger/kut/tree/pmu-chain-promotion-fixes-v1

Eric Auger (6):
  arm: pmu: pmu-chain-promotion: Improve debug messages
  arm: pmu: pmu-chain-promotion: Introduce defines for count and margin
    values
  arm: pmu: Add extra DSB barriers in the mem_access loop
  arm: pmu: Fix chain counter enable/disable sequences
  arm: pmu: Add pmu-memaccess-reliability test
  arm: pmu-chain-promotion: Increase the count and margin values

 arm/pmu.c         | 189 +++++++++++++++++++++++++++++++++-------------
 arm/unittests.cfg |   6 ++
 2 files changed, 141 insertions(+), 54 deletions(-)

-- 
2.38.1

[kvm-unit-tests PATCH 1/6] arm: pmu: pmu-chain-promotion: Improve debug messages

[Eric Auger]

The pmu-chain-promotion test is composed of several subtests.
In case of failures, the current logs are really dificult to
analyze since they look very similar and sometimes duplicated
for each subtest. Add prefixes for each subtest and introduce
a macro that prints the registers we are mostly interested in,
namerly the 2 first counters and the overflow counter.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arm/pmu.c | 63 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index f6e95012..dad7d4b4 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -715,6 +715,11 @@ static void test_chained_sw_incr(bool unused)
 	report_info("overflow=0x%lx, #0=0x%lx #1=0x%lx", read_sysreg(pmovsclr_el0),
 		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
 }
+#define PRINT_REGS(__s) \
+	report_info("%s #1=0x%lx #0=0x%lx overflow=0x%lx", __s, \
+		    read_regn_el0(pmevcntr, 1), \
+		    read_regn_el0(pmevcntr, 0), \
+		    read_sysreg(pmovsclr_el0))
 
 static void test_chain_promotion(bool unused)
 {
@@ -725,6 +730,7 @@ static void test_chain_promotion(bool unused)
 		return;
 
 	/* Only enable CHAIN counter */
+	report_prefix_push("subtest1");
 	pmu_reset();
 	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
 	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
@@ -732,83 +738,81 @@ static void test_chain_promotion(bool unused)
 	isb();
 
 	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	PRINT_REGS("post");
 	report(!read_regn_el0(pmevcntr, 0),
 		"chain counter not counting if even counter is disabled");
+	report_prefix_pop();
 
 	/* Only enable even counter */
+	report_prefix_push("subtest2");
 	pmu_reset();
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW_32);
 	write_sysreg_s(0x1, PMCNTENSET_EL0);
 	isb();
 
 	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	PRINT_REGS("post");
 	report(!read_regn_el0(pmevcntr, 1) && (read_sysreg(pmovsclr_el0) == 0x1),
 		"odd counter did not increment on overflow if disabled");
-	report_info("MEM_ACCESS counter #0 has value 0x%lx",
-		    read_regn_el0(pmevcntr, 0));
-	report_info("CHAIN counter #1 has value 0x%lx",
-		    read_regn_el0(pmevcntr, 1));
-	report_info("overflow counter 0x%lx", read_sysreg(pmovsclr_el0));
+	report_prefix_pop();
 
 	/* start at 0xFFFFFFDC, +20 with CHAIN enabled, +20 with CHAIN disabled */
+	report_prefix_push("subtest3");
 	pmu_reset();
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
 	isb();
+	PRINT_REGS("init");
 
 	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
-	report_info("MEM_ACCESS counter #0 has value 0x%lx",
-		    read_regn_el0(pmevcntr, 0));
+	PRINT_REGS("After 1st loop");
 
 	/* disable the CHAIN event */
 	write_sysreg_s(0x2, PMCNTENCLR_EL0);
 	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
-	report_info("MEM_ACCESS counter #0 has value 0x%lx",
-		    read_regn_el0(pmevcntr, 0));
+	PRINT_REGS("After 2d loop");
 	report(read_sysreg(pmovsclr_el0) == 0x1,
 		"should have triggered an overflow on #0");
 	report(!read_regn_el0(pmevcntr, 1),
 		"CHAIN counter #1 shouldn't have incremented");
+	report_prefix_pop();
 
 	/* start at 0xFFFFFFDC, +20 with CHAIN disabled, +20 with CHAIN enabled */
 
+	report_prefix_push("subtest4");
 	pmu_reset();
 	write_sysreg_s(0x1, PMCNTENSET_EL0);
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
 	isb();
-	report_info("counter #0 = 0x%lx, counter #1 = 0x%lx overflow=0x%lx",
-		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1),
-		    read_sysreg(pmovsclr_el0));
+	PRINT_REGS("init");
 
 	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
-	report_info("MEM_ACCESS counter #0 has value 0x%lx",
-		    read_regn_el0(pmevcntr, 0));
+	PRINT_REGS("After 1st loop");
 
 	/* enable the CHAIN event */
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	isb();
 	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
-	report_info("MEM_ACCESS counter #0 has value 0x%lx",
-		    read_regn_el0(pmevcntr, 0));
+
+	PRINT_REGS("After 2d loop");
 
 	report((read_regn_el0(pmevcntr, 1) == 1) &&
 		(read_sysreg(pmovsclr_el0) == 0x1),
 		"CHAIN counter enabled: CHAIN counter was incremented and overflow");
-
-	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
-		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
+	report_prefix_pop();
 
 	/* start as MEM_ACCESS/CPU_CYCLES and move to CHAIN/MEM_ACCESS */
+	report_prefix_push("subtest5");
 	pmu_reset();
 	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
 	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
 	isb();
+	PRINT_REGS("init");
 
 	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
-	report_info("MEM_ACCESS counter #0 has value 0x%lx",
-		    read_regn_el0(pmevcntr, 0));
+	PRINT_REGS("After 1st loop");
 
 	/* 0 becomes CHAINED */
 	write_sysreg_s(0x0, PMCNTENSET_EL0);
@@ -817,37 +821,34 @@ static void test_chain_promotion(bool unused)
 	write_regn_el0(pmevcntr, 1, 0x0);
 
 	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
-	report_info("MEM_ACCESS counter #0 has value 0x%lx",
-		    read_regn_el0(pmevcntr, 0));
+	PRINT_REGS("After 2d loop");
 
 	report((read_regn_el0(pmevcntr, 1) == 1) &&
 		(read_sysreg(pmovsclr_el0) == 0x1),
 		"32b->64b: CHAIN counter incremented and overflow");
-
-	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
-		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
+	report_prefix_pop();
 
 	/* start as CHAIN/MEM_ACCESS and move to MEM_ACCESS/CPU_CYCLES */
+	report_prefix_push("subtest6");
 	pmu_reset();
 	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
 	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	PRINT_REGS("init");
 
 	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
-	report_info("counter #0=0x%lx, counter #1=0x%lx",
-			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
+	PRINT_REGS("After 1st loop");
 
 	write_sysreg_s(0x0, PMCNTENSET_EL0);
 	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 
 	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	PRINT_REGS("After 2d loop");
 	report(read_sysreg(pmovsclr_el0) == 1,
 		"overflow is expected on counter 0");
-	report_info("counter #0=0x%lx, counter #1=0x%lx overflow=0x%lx",
-			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1),
-			read_sysreg(pmovsclr_el0));
+	report_prefix_pop();
 }
 
 static bool expect_interrupts(uint32_t bitmap)
-- 
2.38.1

[kvm-unit-tests PATCH 2/6] arm: pmu: pmu-chain-promotion: Introduce defines for count and margin values

[Eric Auger]

The pmu-chain-promotion test is composed of separate subtests.

Some of them apply some settings on a first MEM_ACCESS loop
iterations, change the settings and run another MEM_ACCESS loop.

The PRE_OVERFLOW2 MEM_ACCESS counter init value is defined so that
the first loop does not overflow and the second loop overflows.

At the moment the MEM_ACCESS count number is hardcoded to 20 and
PRE_OVERFLOW2 is set to UINT32_MAX - 20 - 15 where 15 acts as a
margin.

Introduce defines for the count number and the margin so that it
becomes easier to change them.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arm/pmu.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index dad7d4b4..b88366a8 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -55,11 +55,18 @@
 #define EXT_COMMON_EVENTS_LOW	0x4000
 #define EXT_COMMON_EVENTS_HIGH	0x403F
 
-#define ALL_SET_32			0x00000000FFFFFFFFULL
+#define ALL_SET_32		0x00000000FFFFFFFFULL
 #define ALL_CLEAR		0x0000000000000000ULL
 #define PRE_OVERFLOW_32		0x00000000FFFFFFF0ULL
-#define PRE_OVERFLOW2_32	0x00000000FFFFFFDCULL
 #define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
+#define COUNT 20
+#define MARGIN 15
+/*
+ * PRE_OVERFLOW2 is set so that 1st COUNT iterations do not
+ * produce 32b overflow and 2d COUNT iterations do. To accommodate
+ * for some observed variability we take into account a given @MARGIN
+ */
+#define PRE_OVERFLOW2_32		(ALL_SET_32 - COUNT - MARGIN)
 
 #define PRE_OVERFLOW(__overflow_at_64bits)				\
 	(__overflow_at_64bits ? PRE_OVERFLOW_64 : PRE_OVERFLOW_32)
@@ -737,7 +744,7 @@ static void test_chain_promotion(bool unused)
 	write_sysreg_s(0x2, PMCNTENSET_EL0);
 	isb();
 
-	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("post");
 	report(!read_regn_el0(pmevcntr, 0),
 		"chain counter not counting if even counter is disabled");
@@ -750,13 +757,13 @@ static void test_chain_promotion(bool unused)
 	write_sysreg_s(0x1, PMCNTENSET_EL0);
 	isb();
 
-	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("post");
 	report(!read_regn_el0(pmevcntr, 1) && (read_sysreg(pmovsclr_el0) == 0x1),
 		"odd counter did not increment on overflow if disabled");
 	report_prefix_pop();
 
-	/* start at 0xFFFFFFDC, +20 with CHAIN enabled, +20 with CHAIN disabled */
+	/* 1st COUNT with CHAIN enabled, next COUNT with CHAIN disabled */
 	report_prefix_push("subtest3");
 	pmu_reset();
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
@@ -764,12 +771,12 @@ static void test_chain_promotion(bool unused)
 	isb();
 	PRINT_REGS("init");
 
-	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("After 1st loop");
 
 	/* disable the CHAIN event */
 	write_sysreg_s(0x2, PMCNTENCLR_EL0);
-	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("After 2d loop");
 	report(read_sysreg(pmovsclr_el0) == 0x1,
 		"should have triggered an overflow on #0");
@@ -777,7 +784,7 @@ static void test_chain_promotion(bool unused)
 		"CHAIN counter #1 shouldn't have incremented");
 	report_prefix_pop();
 
-	/* start at 0xFFFFFFDC, +20 with CHAIN disabled, +20 with CHAIN enabled */
+	/* 1st COUNT with CHAIN disabled, next COUNT with CHAIN enabled */
 
 	report_prefix_push("subtest4");
 	pmu_reset();
@@ -786,13 +793,13 @@ static void test_chain_promotion(bool unused)
 	isb();
 	PRINT_REGS("init");
 
-	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("After 1st loop");
 
 	/* enable the CHAIN event */
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	isb();
-	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 
 	PRINT_REGS("After 2d loop");
 
@@ -811,7 +818,7 @@ static void test_chain_promotion(bool unused)
 	isb();
 	PRINT_REGS("init");
 
-	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("After 1st loop");
 
 	/* 0 becomes CHAINED */
@@ -820,7 +827,7 @@ static void test_chain_promotion(bool unused)
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	write_regn_el0(pmevcntr, 1, 0x0);
 
-	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("After 2d loop");
 
 	report((read_regn_el0(pmevcntr, 1) == 1) &&
@@ -837,14 +844,14 @@ static void test_chain_promotion(bool unused)
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	PRINT_REGS("init");
 
-	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("After 1st loop");
 
 	write_sysreg_s(0x0, PMCNTENSET_EL0);
 	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 
-	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
+	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("After 2d loop");
 	report(read_sysreg(pmovsclr_el0) == 1,
 		"overflow is expected on counter 0");
-- 
2.38.1

[kvm-unit-tests PATCH 3/6] arm: pmu: Add extra DSB barriers in the mem_access loop

[Eric Auger]

The mem access loop currently features ISB barriers only. However
the mem_access loop counts the number of accesses to memory. ISB
do not garantee the PE cannot reorder memory access. Let's
add a DSB ISH before the write to PMCR_EL0 that enables the PMU
and after the last iteration, before disabling the PMU.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com>

---

This was discussed in https://lore.kernel.org/all/YzxmHpV2rpfaUdWi@monolith.localdoman/
---
 arm/pmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index b88366a8..dde399e2 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -301,6 +301,7 @@ static void mem_access_loop(void *addr, long loop, uint32_t pmcr)
 {
 	uint64_t pmcr64 = pmcr;
 asm volatile(
+	"       dsb     ish\n"
 	"       msr     pmcr_el0, %[pmcr]\n"
 	"       isb\n"
 	"       mov     x10, %[loop]\n"
@@ -308,6 +309,7 @@ asm volatile(
 	"       ldr	x9, [%[addr]]\n"
 	"       cmp     x10, #0x0\n"
 	"       b.gt    1b\n"
+	"       dsb     ish\n"
 	"       msr     pmcr_el0, xzr\n"
 	"       isb\n"
 	:
-- 
2.38.1

[kvm-unit-tests PATCH 4/6] arm: pmu: Fix chain counter enable/disable sequences

[Eric Auger]

In some ARM ARM ddi0487 revisions it is said that
disabling/enabling a pair of counters that are paired
by a CHAIN event should follow a given sequence:

Disable the low counter first, isb, disable the high counter
Enable the high counter first, isb, enable low counter

This was the case in Fc. However this is not written anymore
in Ia revision.

Introduce 2 helpers to execute those sequences and replace
the existing PMCNTENCLR/ENSET calls.

Also fix 2 write_sysreg_s(0x0, PMCNTENSET_EL0) in subtest 5 & 6
and replace them by PMCNTENCLR writes since writing 0 in
PMCNTENSET_EL0 has no effect.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arm/pmu.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index dde399e2..af679667 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -730,6 +730,22 @@ static void test_chained_sw_incr(bool unused)
 		    read_regn_el0(pmevcntr, 0), \
 		    read_sysreg(pmovsclr_el0))
 
+static void enable_chain_counter(int even)
+{
+	write_sysreg_s(BIT(even), PMCNTENSET_EL0); /* Enable the high counter first */
+	isb();
+	write_sysreg_s(BIT(even + 1), PMCNTENSET_EL0); /* Enable the low counter */
+	isb();
+}
+
+static void disable_chain_counter(int even)
+{
+	write_sysreg_s(BIT(even + 1), PMCNTENCLR_EL0); /* Disable the low counter first*/
+	isb();
+	write_sysreg_s(BIT(even), PMCNTENCLR_EL0); /* Disable the high counter */
+	isb();
+}
+
 static void test_chain_promotion(bool unused)
 {
 	uint32_t events[] = {MEM_ACCESS, CHAIN};
@@ -768,16 +784,17 @@ static void test_chain_promotion(bool unused)
 	/* 1st COUNT with CHAIN enabled, next COUNT with CHAIN disabled */
 	report_prefix_push("subtest3");
 	pmu_reset();
-	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
-	isb();
+	enable_chain_counter(0);
 	PRINT_REGS("init");
 
 	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("After 1st loop");
 
 	/* disable the CHAIN event */
-	write_sysreg_s(0x2, PMCNTENCLR_EL0);
+	disable_chain_counter(0);
+	write_sysreg_s(0x1, PMCNTENSET_EL0); /* Enable the low counter */
+	isb();
 	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("After 2d loop");
 	report(read_sysreg(pmovsclr_el0) == 0x1,
@@ -798,9 +815,11 @@ static void test_chain_promotion(bool unused)
 	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("After 1st loop");
 
-	/* enable the CHAIN event */
-	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	/* Disable the even counter and enable the chain counter */
+	write_sysreg_s(0x1, PMCNTENCLR_EL0); /* Disable the low counter first */
 	isb();
+	enable_chain_counter(0);
+
 	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 
 	PRINT_REGS("After 2d loop");
@@ -824,10 +843,10 @@ static void test_chain_promotion(bool unused)
 	PRINT_REGS("After 1st loop");
 
 	/* 0 becomes CHAINED */
-	write_sysreg_s(0x0, PMCNTENSET_EL0);
+	write_sysreg_s(0x3, PMCNTENCLR_EL0);
 	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
-	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	write_regn_el0(pmevcntr, 1, 0x0);
+	enable_chain_counter(0);
 
 	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("After 2d loop");
@@ -843,13 +862,13 @@ static void test_chain_promotion(bool unused)
 	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
 	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
-	write_sysreg_s(0x3, PMCNTENSET_EL0);
+	enable_chain_counter(0);
 	PRINT_REGS("init");
 
 	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
 	PRINT_REGS("After 1st loop");
 
-	write_sysreg_s(0x0, PMCNTENSET_EL0);
+	disable_chain_counter(0);
 	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 
-- 
2.38.1

[kvm-unit-tests PATCH 5/6] arm: pmu: Add pmu-memaccess-reliability test

[Eric Auger]

Add a new basic test that runs MEM_ACCESS loop over
100 iterations and make sure the number of measured
MEM_ACCESS never overflows the margin. Some other
pmu tests rely on this pattern and if the MEM_ACCESS
measurement is not reliable, it is better to report
it beforehand and not confuse the user any further.

Without the subsequent patch, this typically fails on
ThunderXv2 with the following logs:

INFO: pmu: pmu-memaccess-reliability: 32-bit overflows:
overflow=1 min=21 max=41 COUNT=20 MARGIN=15
FAIL: pmu: pmu-memaccess-reliability: 32-bit overflows:
memaccess is reliable

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arm/pmu.c         | 52 +++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg |  6 ++++++
 2 files changed, 58 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index af679667..c3d2a428 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -56,6 +56,7 @@
 #define EXT_COMMON_EVENTS_HIGH	0x403F
 
 #define ALL_SET_32		0x00000000FFFFFFFFULL
+#define ALL_SET_64		0xFFFFFFFFFFFFFFFFULL
 #define ALL_CLEAR		0x0000000000000000ULL
 #define PRE_OVERFLOW_32		0x00000000FFFFFFF0ULL
 #define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
@@ -67,6 +68,10 @@
  * for some observed variability we take into account a given @MARGIN
  */
 #define PRE_OVERFLOW2_32		(ALL_SET_32 - COUNT - MARGIN)
+#define PRE_OVERFLOW2_64		(ALL_SET_64 - COUNT - MARGIN)
+
+#define PRE_OVERFLOW2(__overflow_at_64bits)				\
+	(__overflow_at_64bits ? PRE_OVERFLOW2_64 : PRE_OVERFLOW2_32)
 
 #define PRE_OVERFLOW(__overflow_at_64bits)				\
 	(__overflow_at_64bits ? PRE_OVERFLOW_64 : PRE_OVERFLOW_32)
@@ -746,6 +751,50 @@ static void disable_chain_counter(int even)
 	isb();
 }
 
+static void test_memaccess_reliability(bool overflow_at_64bits)
+{
+	uint32_t events[] = {MEM_ACCESS};
+	void *addr = malloc(PAGE_SIZE);
+	uint64_t count, max = 0, min = pmevcntr_mask();
+	uint64_t pre_overflow2 = PRE_OVERFLOW2(overflow_at_64bits);
+	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
+	bool overflow = false;
+
+	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)) ||
+	    !check_overflow_prerequisites(overflow_at_64bits))
+		return;
+
+	pmu_reset();
+	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
+	for (int i = 0; i < 100; i++) {
+		pmu_reset();
+		write_regn_el0(pmevcntr, 0, pre_overflow2);
+		write_sysreg_s(0x1, PMCNTENSET_EL0);
+		isb();
+		mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
+		count = read_regn_el0(pmevcntr, 0);
+		if (count < pre_overflow2) {
+			count += COUNT + MARGIN;
+			if (count > max)
+				max = count;
+			if (count < min)
+				min = count;
+			overflow = true;
+			report_info("iter=%d count=%ld min=%ld max=%ld overflow!!!",
+				    i, count, min, max);
+			continue;
+		}
+		count -= pre_overflow2;
+		if (count > max)
+			max = count;
+		if (count < min)
+			min = count;
+	}
+	report_info("overflow=%d min=%ld max=%ld COUNT=%d MARGIN=%d",
+		    overflow, min, max, COUNT, MARGIN);
+	report(!overflow, "memaccess is reliable");
+}
+
 static void test_chain_promotion(bool unused)
 {
 	uint32_t events[] = {MEM_ACCESS, CHAIN};
@@ -1203,6 +1252,9 @@ int main(int argc, char *argv[])
 	} else if (strcmp(argv[1], "pmu-basic-event-count") == 0) {
 		run_event_test(argv[1], test_basic_event_count, false);
 		run_event_test(argv[1], test_basic_event_count, true);
+	} else if (strcmp(argv[1], "pmu-memaccess-reliability") == 0) {
+		run_event_test(argv[1], test_memaccess_reliability, false);
+		run_event_test(argv[1], test_memaccess_reliability, true);
 	} else if (strcmp(argv[1], "pmu-mem-access") == 0) {
 		run_event_test(argv[1], test_mem_access, false);
 		run_event_test(argv[1], test_mem_access, true);
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 5e67b558..301261aa 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -90,6 +90,12 @@ groups = pmu
 arch = arm64
 extra_params = -append 'pmu-mem-access'
 
+[pmu-memaccess-reliability]
+file = pmu.flat
+groups = pmu
+arch = arm64
+extra_params = -append 'pmu-memaccess-reliability'
+
 [pmu-sw-incr]
 file = pmu.flat
 groups = pmu
-- 
2.38.1

[kvm-unit-tests PATCH 6/6] arm: pmu-chain-promotion: Increase the count and margin values

[Eric Auger]

Let's increase the mem_access loop count by defining COUNT=250
(instead of 20) and define a more reasonable margin (100 instead
of 15 previously) so that it gives better chance to accommodate
for HW implementation variance. Those values were chosen arbitrarily
higher. Those values fix the random failures on ThunderX2 machines.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 arm/pmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index c3d2a428..d897d8d3 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -60,8 +60,8 @@
 #define ALL_CLEAR		0x0000000000000000ULL
 #define PRE_OVERFLOW_32		0x00000000FFFFFFF0ULL
 #define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
-#define COUNT 20
-#define MARGIN 15
+#define COUNT 250
+#define MARGIN 100
 /*
  * PRE_OVERFLOW2 is set so that 1st COUNT iterations do not
  * produce 32b overflow and 2d COUNT iterations do. To accommodate
-- 
2.38.1

Re: [kvm-unit-tests PATCH 0/6] arm: pmu: Fix random failures of pmu-chain-promotion

[Eric Auger]

Hi,

On 3/15/23 12:07, Eric Auger wrote:
> On some HW (ThunderXv2), some random failures of
> pmu-chain-promotion test can be observed.
>
> pmu-chain-promotion is composed of several subtests
> which run 2 mem_access loops. The initial value of
> the counter is set so that no overflow is expected on
> the first loop run and overflow is expected on the second.
> However it is observed that sometimes we get an overflow
> on the first run. It looks related to some variability of
> the mem_acess count. This variability is observed on all
> HW I have access to, with different span though. On
> ThunderX2 HW it looks the margin that is currently taken
> is too small and we regularly hit failure.
>
> although the first goal of this series is to increase
> the count/margin used in those tests, it also attempts
> to improve the pmu-chain-promotion logs, add some barriers
> in the mem-access loop, clarify the chain counter
> enable/disable sequence.
>
> A new 'pmu-memaccess-reliability' is also introduced to
> detect issues with MEM_ACCESS event variability and make
> the debug easier.
>
> Obviously one can wonder if this variability is something normal
> and does not hide any other bug. I hope this series will raise
> additional discussions about this.
>
> https://github.com/eauger/kut/tree/pmu-chain-promotion-fixes-v1

Gentle ping.

Thanks

Eric
>
> Eric Auger (6):
>   arm: pmu: pmu-chain-promotion: Improve debug messages
>   arm: pmu: pmu-chain-promotion: Introduce defines for count and margin
>     values
>   arm: pmu: Add extra DSB barriers in the mem_access loop
>   arm: pmu: Fix chain counter enable/disable sequences
>   arm: pmu: Add pmu-memaccess-reliability test
>   arm: pmu-chain-promotion: Increase the count and margin values
>
>  arm/pmu.c         | 189 +++++++++++++++++++++++++++++++++-------------
>  arm/unittests.cfg |   6 ++
>  2 files changed, 141 insertions(+), 54 deletions(-)
>

Re: [kvm-unit-tests PATCH 0/6] arm: pmu: Fix random failures of pmu-chain-promotion

[Andrew Jones]

On Tue, Apr 04, 2023 at 08:23:15AM +0200, Eric Auger wrote:
> Hi,
> 
> On 3/15/23 12:07, Eric Auger wrote:
> > On some HW (ThunderXv2), some random failures of
> > pmu-chain-promotion test can be observed.
> >
> > pmu-chain-promotion is composed of several subtests
> > which run 2 mem_access loops. The initial value of
> > the counter is set so that no overflow is expected on
> > the first loop run and overflow is expected on the second.
> > However it is observed that sometimes we get an overflow
> > on the first run. It looks related to some variability of
> > the mem_acess count. This variability is observed on all
> > HW I have access to, with different span though. On
> > ThunderX2 HW it looks the margin that is currently taken
> > is too small and we regularly hit failure.
> >
> > although the first goal of this series is to increase
> > the count/margin used in those tests, it also attempts
> > to improve the pmu-chain-promotion logs, add some barriers
> > in the mem-access loop, clarify the chain counter
> > enable/disable sequence.
> >
> > A new 'pmu-memaccess-reliability' is also introduced to
> > detect issues with MEM_ACCESS event variability and make
> > the debug easier.
> >
> > Obviously one can wonder if this variability is something normal
> > and does not hide any other bug. I hope this series will raise
> > additional discussions about this.
> >
> > https://github.com/eauger/kut/tree/pmu-chain-promotion-fixes-v1
> 
> Gentle ping.

I'd be happy to take this, but I was hoping to see some r-b's and/or t-b's
from some of the others.

Thanks,
drew

Re: [kvm-unit-tests PATCH 0/6] arm: pmu: Fix random failures of pmu-chain-promotion

[Andrew Jones]

On Tue, Apr 04, 2023 at 02:47:49PM +0200, Andrew Jones wrote:
> On Tue, Apr 04, 2023 at 08:23:15AM +0200, Eric Auger wrote:
> > Hi,
> > 
> > On 3/15/23 12:07, Eric Auger wrote:
> > > On some HW (ThunderXv2), some random failures of
> > > pmu-chain-promotion test can be observed.
> > >
> > > pmu-chain-promotion is composed of several subtests
> > > which run 2 mem_access loops. The initial value of
> > > the counter is set so that no overflow is expected on
> > > the first loop run and overflow is expected on the second.
> > > However it is observed that sometimes we get an overflow
> > > on the first run. It looks related to some variability of
> > > the mem_acess count. This variability is observed on all
> > > HW I have access to, with different span though. On
> > > ThunderX2 HW it looks the margin that is currently taken
> > > is too small and we regularly hit failure.
> > >
> > > although the first goal of this series is to increase
> > > the count/margin used in those tests, it also attempts
> > > to improve the pmu-chain-promotion logs, add some barriers
> > > in the mem-access loop, clarify the chain counter
> > > enable/disable sequence.
> > >
> > > A new 'pmu-memaccess-reliability' is also introduced to
> > > detect issues with MEM_ACCESS event variability and make
> > > the debug easier.
> > >
> > > Obviously one can wonder if this variability is something normal
> > > and does not hide any other bug. I hope this series will raise
> > > additional discussions about this.
> > >
> > > https://github.com/eauger/kut/tree/pmu-chain-promotion-fixes-v1
> > 
> > Gentle ping.
> 
> I'd be happy to take this, but I was hoping to see some r-b's and/or t-b's
> from some of the others.

Any takers? Ricardo? Alexandru?

Thanks,
drew

Re: [kvm-unit-tests PATCH 0/6] arm: pmu: Fix random failures of pmu-chain-promotion

[Mark Rutland]

On Tue, Apr 04, 2023 at 02:47:47PM +0200, Andrew Jones wrote:
> On Tue, Apr 04, 2023 at 08:23:15AM +0200, Eric Auger wrote:
> > Hi,
> > 
> > On 3/15/23 12:07, Eric Auger wrote:
> > > On some HW (ThunderXv2), some random failures of
> > > pmu-chain-promotion test can be observed.
> > >
> > > pmu-chain-promotion is composed of several subtests
> > > which run 2 mem_access loops. The initial value of
> > > the counter is set so that no overflow is expected on
> > > the first loop run and overflow is expected on the second.
> > > However it is observed that sometimes we get an overflow
> > > on the first run. It looks related to some variability of
> > > the mem_acess count. This variability is observed on all
> > > HW I have access to, with different span though. On
> > > ThunderX2 HW it looks the margin that is currently taken
> > > is too small and we regularly hit failure.
> > >
> > > although the first goal of this series is to increase
> > > the count/margin used in those tests, it also attempts
> > > to improve the pmu-chain-promotion logs, add some barriers
> > > in the mem-access loop, clarify the chain counter
> > > enable/disable sequence.
> > >
> > > A new 'pmu-memaccess-reliability' is also introduced to
> > > detect issues with MEM_ACCESS event variability and make
> > > the debug easier.

As a minor nit, 'pmu-mem-access-reliability' would be more consistent with
'pmu-mem-access'. The lack of a dash in 'memaccess' tripped me up while I was
trying to run those two tests.

> > > Obviously one can wonder if this variability is something normal
> > > and does not hide any other bug. I hope this series will raise
> > > additional discussions about this.
> > >
> > > https://github.com/eauger/kut/tree/pmu-chain-promotion-fixes-v1
> > 
> > Gentle ping.
> 
> I'd be happy to take this, but I was hoping to see some r-b's and/or t-b's
> from some of the others.

I gave this a spin on my ThunderX2, and it seems to fix the intermittent
failures I was seeing.

FWIW:

Tested-by: Mark Rutland <mark.rutland@arm.com>

Before (on commit 4ba7058c61e8922f9c8397cfa1095fac325f809b):

Test results below.

| [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL= useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-chain-promotion'
| timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-chain-promotion # -initrd /tmp/tmp.nl1i6S0EIY
| INFO: PMU version: 0x4
| INFO: PMU implementer/ID code: 0(" ")/0
| INFO: Implements 6 event counters
| PASS: pmu: pmu-chain-promotion: 32-bit overflows: chain counter not counting if even counter is disabled
| PASS: pmu: pmu-chain-promotion: 32-bit overflows: odd counter did not increment on overflow if disabled
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x7
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 has value 0x0
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: overflow counter 0x1
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x4
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x1b
| PASS: pmu: pmu-chain-promotion: 32-bit overflows: should have triggered an overflow on #0
| FAIL: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 shouldn't have incremented
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: counter #0 = 0xffffffdc, counter #1 = 0x0 overflow=0x0
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x4
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x1b
| FAIL: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter enabled: CHAIN counter was incremented and overflow
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 = 0x0, overflow=0x1
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x4
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x1b
| FAIL: pmu: pmu-chain-promotion: 32-bit overflows: 32b->64b: CHAIN counter incremented and overflow
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 = 0x0, overflow=0x1
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: counter #0=0xfffffff3, counter #1=0x0
| PASS: pmu: pmu-chain-promotion: 32-bit overflows: overflow is expected on counter 0
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: counter #0=0xa, counter #1=0xf9 overflow=0x1
| SUMMARY: 7 tests, 3 unexpected failures

After:

| [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL=kvm useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-chain-promotion'
| timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-chain-promotion # -initrd /tmp/tmp.pahLyg1F3s
| INFO: PMU version: 0x4
| INFO: PMU implementer/ID code: 0(" ")/0
| INFO: Implements 6 event counters
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest1: post #1=0x0 #0=0x0 overflow=0x0
| PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest1: chain counter not counting if even counter is disabled
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest2: post #1=0x0 #0=0xf3 overflow=0x1
| PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest2: odd counter did not increment on overflow if disabled
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: init #1=0x0 #0=0xfffffea1 overflow=0x0
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: After 1st loop #1=0x0 #0=0xffffffa0 overflow=0x0
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: After 2d loop #1=0x0 #0=0xc0 overflow=0x1
| PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: should have triggered an overflow on #0
| PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: CHAIN counter #1 shouldn't have incremented
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: init #1=0x0 #0=0xfffffea1 overflow=0x0
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: After 1st loop #1=0x0 #0=0xffffffb7 overflow=0x0
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: After 2d loop #1=0x1 #0=0xbc overflow=0x1
| PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: CHAIN counter enabled: CHAIN counter was incremented and overflow
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: init #1=0x0 #0=0xfffffea1 overflow=0x0
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: After 1st loop #1=0x22c #0=0xffffff9f overflow=0x0
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: After 2d loop #1=0x1 #0=0x9d overflow=0x1
| PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: 32b->64b: CHAIN counter incremented and overflow
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: init #1=0x0 #0=0xfffffea1 overflow=0x0
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: After 1st loop #1=0x0 #0=0xffffff9f overflow=0x0
| INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: After 2d loop #1=0x1f9 #0=0x9c overflow=0x1
| PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: overflow is expected on counter 0
| SUMMARY: 7 tests

As a bonus, the mem-access and memaccess-reliability results:

| [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL=kvm useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-mem-access'     
| timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-mem-access # -initrd /tmp/tmp.84AeEp8Tiw
| INFO: PMU version: 0x4
| INFO: PMU implementer/ID code: 0(" ")/0
| INFO: Implements 6 event counters
| INFO: pmu: pmu-mem-access: 32-bit overflows: counter #0 is 0x15 (MEM_ACCESS)
| INFO: pmu: pmu-mem-access: 32-bit overflows: counter #1 is 0x15 (MEM_ACCESS)
| PASS: pmu: pmu-mem-access: 32-bit overflows: Ran 20 mem accesses
| PASS: pmu: pmu-mem-access: 32-bit overflows: Ran 20 mem accesses with expected overflows on both counters
| INFO: pmu: pmu-mem-access: 32-bit overflows: cnt#0=0x8 cnt#1=0x8 overflow=0x3
| SKIP: pmu: pmu-mem-access: 64-bit overflows: Skip test as 64 overflows need FEAT_PMUv3p5
| SUMMARY: 3 tests, 1 skipped
| [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL=kvm useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-memaccess-reliability'
| timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-memaccess-reliability # -initrd /tmp/tmp.ZToqwencZR
| INFO: PMU version: 0x4
| INFO: PMU implementer/ID code: 0(" ")/0
| INFO: Implements 6 event counters
| INFO: pmu: pmu-memaccess-reliability: 32-bit overflows: overflow=0 min=251 max=283 COUNT=250 MARGIN=100
| PASS: pmu: pmu-memaccess-reliability: 32-bit overflows: memaccess is reliable
| SKIP: pmu: pmu-memaccess-reliability: 64-bit overflows: Skip test as 64 overflows need FEAT_PMUv3p5
| SUMMARY: 2 tests, 1 skipped

Thanks,
Mark.

Re: [kvm-unit-tests PATCH 0/6] arm: pmu: Fix random failures of pmu-chain-promotion

[Alexandru Elisei]

Hi,

On Wed, Apr 12, 2023 at 09:34:36AM +0200, Andrew Jones wrote:
> On Tue, Apr 04, 2023 at 02:47:49PM +0200, Andrew Jones wrote:
> > On Tue, Apr 04, 2023 at 08:23:15AM +0200, Eric Auger wrote:
> > > Hi,
> > > 
> > > On 3/15/23 12:07, Eric Auger wrote:
> > > > On some HW (ThunderXv2), some random failures of
> > > > pmu-chain-promotion test can be observed.
> > > >
> > > > pmu-chain-promotion is composed of several subtests
> > > > which run 2 mem_access loops. The initial value of
> > > > the counter is set so that no overflow is expected on
> > > > the first loop run and overflow is expected on the second.
> > > > However it is observed that sometimes we get an overflow
> > > > on the first run. It looks related to some variability of
> > > > the mem_acess count. This variability is observed on all
> > > > HW I have access to, with different span though. On
> > > > ThunderX2 HW it looks the margin that is currently taken
> > > > is too small and we regularly hit failure.
> > > >
> > > > although the first goal of this series is to increase
> > > > the count/margin used in those tests, it also attempts
> > > > to improve the pmu-chain-promotion logs, add some barriers
> > > > in the mem-access loop, clarify the chain counter
> > > > enable/disable sequence.
> > > >
> > > > A new 'pmu-memaccess-reliability' is also introduced to
> > > > detect issues with MEM_ACCESS event variability and make
> > > > the debug easier.
> > > >
> > > > Obviously one can wonder if this variability is something normal
> > > > and does not hide any other bug. I hope this series will raise
> > > > additional discussions about this.
> > > >
> > > > https://github.com/eauger/kut/tree/pmu-chain-promotion-fixes-v1
> > > 
> > > Gentle ping.
> > 
> > I'd be happy to take this, but I was hoping to see some r-b's and/or t-b's
> > from some of the others.
> 
> Any takers? Ricardo? Alexandru?

Will try to have a look as soon as I can, and run some tests, am hoping this
week (but no promises!).

Thanks,
Alex

> 
> Thanks,
> drew

Re: [kvm-unit-tests PATCH 0/6] arm: pmu: Fix random failures of pmu-chain-promotion

[Eric Auger]

Hi,

On 4/12/23 10:47, Mark Rutland wrote:
> On Tue, Apr 04, 2023 at 02:47:47PM +0200, Andrew Jones wrote:
>> On Tue, Apr 04, 2023 at 08:23:15AM +0200, Eric Auger wrote:
>>> Hi,
>>>
>>> On 3/15/23 12:07, Eric Auger wrote:
>>>> On some HW (ThunderXv2), some random failures of
>>>> pmu-chain-promotion test can be observed.
>>>>
>>>> pmu-chain-promotion is composed of several subtests
>>>> which run 2 mem_access loops. The initial value of
>>>> the counter is set so that no overflow is expected on
>>>> the first loop run and overflow is expected on the second.
>>>> However it is observed that sometimes we get an overflow
>>>> on the first run. It looks related to some variability of
>>>> the mem_acess count. This variability is observed on all
>>>> HW I have access to, with different span though. On
>>>> ThunderX2 HW it looks the margin that is currently taken
>>>> is too small and we regularly hit failure.
>>>>
>>>> although the first goal of this series is to increase
>>>> the count/margin used in those tests, it also attempts
>>>> to improve the pmu-chain-promotion logs, add some barriers
>>>> in the mem-access loop, clarify the chain counter
>>>> enable/disable sequence.
>>>>
>>>> A new 'pmu-memaccess-reliability' is also introduced to
>>>> detect issues with MEM_ACCESS event variability and make
>>>> the debug easier.
> As a minor nit, 'pmu-mem-access-reliability' would be more consistent with
> 'pmu-mem-access'. The lack of a dash in 'memaccess' tripped me up while I was
> trying to run those two tests.

I can easily respin with that renaming. thanks for the feedback. Waiting
a little bit more if somebody has any other comment.

Eric
>
>>>> Obviously one can wonder if this variability is something normal
>>>> and does not hide any other bug. I hope this series will raise
>>>> additional discussions about this.
>>>>
>>>> https://github.com/eauger/kut/tree/pmu-chain-promotion-fixes-v1
>>> Gentle ping.
>> I'd be happy to take this, but I was hoping to see some r-b's and/or t-b's
>> from some of the others.
> I gave this a spin on my ThunderX2, and it seems to fix the intermittent
> failures I was seeing.
>
> FWIW:
>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
>
> Before (on commit 4ba7058c61e8922f9c8397cfa1095fac325f809b):
>
> Test results below.
>
> | [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL= useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-chain-promotion'
> | timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-chain-promotion # -initrd /tmp/tmp.nl1i6S0EIY
> | INFO: PMU version: 0x4
> | INFO: PMU implementer/ID code: 0(" ")/0
> | INFO: Implements 6 event counters
> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: chain counter not counting if even counter is disabled
> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: odd counter did not increment on overflow if disabled
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x7
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 has value 0x0
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: overflow counter 0x1
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x4
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x1b
> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: should have triggered an overflow on #0
> | FAIL: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 shouldn't have incremented
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: counter #0 = 0xffffffdc, counter #1 = 0x0 overflow=0x0
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x4
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x1b
> | FAIL: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter enabled: CHAIN counter was incremented and overflow
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 = 0x0, overflow=0x1
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x4
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x1b
> | FAIL: pmu: pmu-chain-promotion: 32-bit overflows: 32b->64b: CHAIN counter incremented and overflow
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 = 0x0, overflow=0x1
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: counter #0=0xfffffff3, counter #1=0x0
> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: overflow is expected on counter 0
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: counter #0=0xa, counter #1=0xf9 overflow=0x1
> | SUMMARY: 7 tests, 3 unexpected failures
>
> After:
>
> | [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL=kvm useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-chain-promotion'
> | timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-chain-promotion # -initrd /tmp/tmp.pahLyg1F3s
> | INFO: PMU version: 0x4
> | INFO: PMU implementer/ID code: 0(" ")/0
> | INFO: Implements 6 event counters
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest1: post #1=0x0 #0=0x0 overflow=0x0
> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest1: chain counter not counting if even counter is disabled
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest2: post #1=0x0 #0=0xf3 overflow=0x1
> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest2: odd counter did not increment on overflow if disabled
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: init #1=0x0 #0=0xfffffea1 overflow=0x0
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: After 1st loop #1=0x0 #0=0xffffffa0 overflow=0x0
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: After 2d loop #1=0x0 #0=0xc0 overflow=0x1
> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: should have triggered an overflow on #0
> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: CHAIN counter #1 shouldn't have incremented
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: init #1=0x0 #0=0xfffffea1 overflow=0x0
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: After 1st loop #1=0x0 #0=0xffffffb7 overflow=0x0
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: After 2d loop #1=0x1 #0=0xbc overflow=0x1
> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: CHAIN counter enabled: CHAIN counter was incremented and overflow
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: init #1=0x0 #0=0xfffffea1 overflow=0x0
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: After 1st loop #1=0x22c #0=0xffffff9f overflow=0x0
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: After 2d loop #1=0x1 #0=0x9d overflow=0x1
> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: 32b->64b: CHAIN counter incremented and overflow
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: init #1=0x0 #0=0xfffffea1 overflow=0x0
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: After 1st loop #1=0x0 #0=0xffffff9f overflow=0x0
> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: After 2d loop #1=0x1f9 #0=0x9c overflow=0x1
> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: overflow is expected on counter 0
> | SUMMARY: 7 tests
>
> As a bonus, the mem-access and memaccess-reliability results:
>
> | [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL=kvm useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-mem-access'     
> | timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-mem-access # -initrd /tmp/tmp.84AeEp8Tiw
> | INFO: PMU version: 0x4
> | INFO: PMU implementer/ID code: 0(" ")/0
> | INFO: Implements 6 event counters
> | INFO: pmu: pmu-mem-access: 32-bit overflows: counter #0 is 0x15 (MEM_ACCESS)
> | INFO: pmu: pmu-mem-access: 32-bit overflows: counter #1 is 0x15 (MEM_ACCESS)
> | PASS: pmu: pmu-mem-access: 32-bit overflows: Ran 20 mem accesses
> | PASS: pmu: pmu-mem-access: 32-bit overflows: Ran 20 mem accesses with expected overflows on both counters
> | INFO: pmu: pmu-mem-access: 32-bit overflows: cnt#0=0x8 cnt#1=0x8 overflow=0x3
> | SKIP: pmu: pmu-mem-access: 64-bit overflows: Skip test as 64 overflows need FEAT_PMUv3p5
> | SUMMARY: 3 tests, 1 skipped
> | [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL=kvm useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-memaccess-reliability'
> | timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-memaccess-reliability # -initrd /tmp/tmp.ZToqwencZR
> | INFO: PMU version: 0x4
> | INFO: PMU implementer/ID code: 0(" ")/0
> | INFO: Implements 6 event counters
> | INFO: pmu: pmu-memaccess-reliability: 32-bit overflows: overflow=0 min=251 max=283 COUNT=250 MARGIN=100
> | PASS: pmu: pmu-memaccess-reliability: 32-bit overflows: memaccess is reliable
> | SKIP: pmu: pmu-memaccess-reliability: 64-bit overflows: Skip test as 64 overflows need FEAT_PMUv3p5
> | SUMMARY: 2 tests, 1 skipped
>
> Thanks,
> Mark.
>

Re: [kvm-unit-tests PATCH 0/6] arm: pmu: Fix random failures of pmu-chain-promotion

[Alexandru Elisei]

Hi,

On Wed, Apr 19, 2023 at 09:32:10AM +0200, Eric Auger wrote:
> Hi,
> 
> On 4/12/23 10:47, Mark Rutland wrote:
> > On Tue, Apr 04, 2023 at 02:47:47PM +0200, Andrew Jones wrote:
> >> On Tue, Apr 04, 2023 at 08:23:15AM +0200, Eric Auger wrote:
> >>> Hi,
> >>>
> >>> On 3/15/23 12:07, Eric Auger wrote:
> >>>> On some HW (ThunderXv2), some random failures of
> >>>> pmu-chain-promotion test can be observed.
> >>>>
> >>>> pmu-chain-promotion is composed of several subtests
> >>>> which run 2 mem_access loops. The initial value of
> >>>> the counter is set so that no overflow is expected on
> >>>> the first loop run and overflow is expected on the second.
> >>>> However it is observed that sometimes we get an overflow
> >>>> on the first run. It looks related to some variability of
> >>>> the mem_acess count. This variability is observed on all
> >>>> HW I have access to, with different span though. On
> >>>> ThunderX2 HW it looks the margin that is currently taken
> >>>> is too small and we regularly hit failure.
> >>>>
> >>>> although the first goal of this series is to increase
> >>>> the count/margin used in those tests, it also attempts
> >>>> to improve the pmu-chain-promotion logs, add some barriers
> >>>> in the mem-access loop, clarify the chain counter
> >>>> enable/disable sequence.
> >>>>
> >>>> A new 'pmu-memaccess-reliability' is also introduced to
> >>>> detect issues with MEM_ACCESS event variability and make
> >>>> the debug easier.
> > As a minor nit, 'pmu-mem-access-reliability' would be more consistent with
> > 'pmu-mem-access'. The lack of a dash in 'memaccess' tripped me up while I was
> > trying to run those two tests.
> 
> I can easily respin with that renaming. thanks for the feedback. Waiting
> a little bit more if somebody has any other comment.

Started reviewing the series last week, something came up and I had to stop
after the first patch (very nice improvement, by the way). I will finish
the review by the end of this week.

Thanks,
Alex

> 
> Eric
> >
> >>>> Obviously one can wonder if this variability is something normal
> >>>> and does not hide any other bug. I hope this series will raise
> >>>> additional discussions about this.
> >>>>
> >>>> https://github.com/eauger/kut/tree/pmu-chain-promotion-fixes-v1
> >>> Gentle ping.
> >> I'd be happy to take this, but I was hoping to see some r-b's and/or t-b's
> >> from some of the others.
> > I gave this a spin on my ThunderX2, and it seems to fix the intermittent
> > failures I was seeing.
> >
> > FWIW:
> >
> > Tested-by: Mark Rutland <mark.rutland@arm.com>
> >
> > Before (on commit 4ba7058c61e8922f9c8397cfa1095fac325f809b):
> >
> > Test results below.
> >
> > | [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL= useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-chain-promotion'
> > | timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-chain-promotion # -initrd /tmp/tmp.nl1i6S0EIY
> > | INFO: PMU version: 0x4
> > | INFO: PMU implementer/ID code: 0(" ")/0
> > | INFO: Implements 6 event counters
> > | PASS: pmu: pmu-chain-promotion: 32-bit overflows: chain counter not counting if even counter is disabled
> > | PASS: pmu: pmu-chain-promotion: 32-bit overflows: odd counter did not increment on overflow if disabled
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x7
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 has value 0x0
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: overflow counter 0x1
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x4
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x1b
> > | PASS: pmu: pmu-chain-promotion: 32-bit overflows: should have triggered an overflow on #0
> > | FAIL: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 shouldn't have incremented
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: counter #0 = 0xffffffdc, counter #1 = 0x0 overflow=0x0
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x4
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x1b
> > | FAIL: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter enabled: CHAIN counter was incremented and overflow
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 = 0x0, overflow=0x1
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x4
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x1b
> > | FAIL: pmu: pmu-chain-promotion: 32-bit overflows: 32b->64b: CHAIN counter incremented and overflow
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 = 0x0, overflow=0x1
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: counter #0=0xfffffff3, counter #1=0x0
> > | PASS: pmu: pmu-chain-promotion: 32-bit overflows: overflow is expected on counter 0
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: counter #0=0xa, counter #1=0xf9 overflow=0x1
> > | SUMMARY: 7 tests, 3 unexpected failures
> >
> > After:
> >
> > | [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL=kvm useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-chain-promotion'
> > | timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-chain-promotion # -initrd /tmp/tmp.pahLyg1F3s
> > | INFO: PMU version: 0x4
> > | INFO: PMU implementer/ID code: 0(" ")/0
> > | INFO: Implements 6 event counters
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest1: post #1=0x0 #0=0x0 overflow=0x0
> > | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest1: chain counter not counting if even counter is disabled
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest2: post #1=0x0 #0=0xf3 overflow=0x1
> > | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest2: odd counter did not increment on overflow if disabled
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: init #1=0x0 #0=0xfffffea1 overflow=0x0
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: After 1st loop #1=0x0 #0=0xffffffa0 overflow=0x0
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: After 2d loop #1=0x0 #0=0xc0 overflow=0x1
> > | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: should have triggered an overflow on #0
> > | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: CHAIN counter #1 shouldn't have incremented
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: init #1=0x0 #0=0xfffffea1 overflow=0x0
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: After 1st loop #1=0x0 #0=0xffffffb7 overflow=0x0
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: After 2d loop #1=0x1 #0=0xbc overflow=0x1
> > | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: CHAIN counter enabled: CHAIN counter was incremented and overflow
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: init #1=0x0 #0=0xfffffea1 overflow=0x0
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: After 1st loop #1=0x22c #0=0xffffff9f overflow=0x0
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: After 2d loop #1=0x1 #0=0x9d overflow=0x1
> > | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: 32b->64b: CHAIN counter incremented and overflow
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: init #1=0x0 #0=0xfffffea1 overflow=0x0
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: After 1st loop #1=0x0 #0=0xffffff9f overflow=0x0
> > | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: After 2d loop #1=0x1f9 #0=0x9c overflow=0x1
> > | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: overflow is expected on counter 0
> > | SUMMARY: 7 tests
> >
> > As a bonus, the mem-access and memaccess-reliability results:
> >
> > | [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL=kvm useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-mem-access'     
> > | timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-mem-access # -initrd /tmp/tmp.84AeEp8Tiw
> > | INFO: PMU version: 0x4
> > | INFO: PMU implementer/ID code: 0(" ")/0
> > | INFO: Implements 6 event counters
> > | INFO: pmu: pmu-mem-access: 32-bit overflows: counter #0 is 0x15 (MEM_ACCESS)
> > | INFO: pmu: pmu-mem-access: 32-bit overflows: counter #1 is 0x15 (MEM_ACCESS)
> > | PASS: pmu: pmu-mem-access: 32-bit overflows: Ran 20 mem accesses
> > | PASS: pmu: pmu-mem-access: 32-bit overflows: Ran 20 mem accesses with expected overflows on both counters
> > | INFO: pmu: pmu-mem-access: 32-bit overflows: cnt#0=0x8 cnt#1=0x8 overflow=0x3
> > | SKIP: pmu: pmu-mem-access: 64-bit overflows: Skip test as 64 overflows need FEAT_PMUv3p5
> > | SUMMARY: 3 tests, 1 skipped
> > | [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL=kvm useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-memaccess-reliability'
> > | timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-memaccess-reliability # -initrd /tmp/tmp.ZToqwencZR
> > | INFO: PMU version: 0x4
> > | INFO: PMU implementer/ID code: 0(" ")/0
> > | INFO: Implements 6 event counters
> > | INFO: pmu: pmu-memaccess-reliability: 32-bit overflows: overflow=0 min=251 max=283 COUNT=250 MARGIN=100
> > | PASS: pmu: pmu-memaccess-reliability: 32-bit overflows: memaccess is reliable
> > | SKIP: pmu: pmu-memaccess-reliability: 64-bit overflows: Skip test as 64 overflows need FEAT_PMUv3p5
> > | SUMMARY: 2 tests, 1 skipped
> >
> > Thanks,
> > Mark.
> >
> 

Re: [kvm-unit-tests PATCH 0/6] arm: pmu: Fix random failures of pmu-chain-promotion

[Eric Auger]

Hi Alexandru,
On 4/19/23 11:39, Alexandru Elisei wrote:
> Hi,
>
> On Wed, Apr 19, 2023 at 09:32:10AM +0200, Eric Auger wrote:
>> Hi,
>>
>> On 4/12/23 10:47, Mark Rutland wrote:
>>> On Tue, Apr 04, 2023 at 02:47:47PM +0200, Andrew Jones wrote:
>>>> On Tue, Apr 04, 2023 at 08:23:15AM +0200, Eric Auger wrote:
>>>>> Hi,
>>>>>
>>>>> On 3/15/23 12:07, Eric Auger wrote:
>>>>>> On some HW (ThunderXv2), some random failures of
>>>>>> pmu-chain-promotion test can be observed.
>>>>>>
>>>>>> pmu-chain-promotion is composed of several subtests
>>>>>> which run 2 mem_access loops. The initial value of
>>>>>> the counter is set so that no overflow is expected on
>>>>>> the first loop run and overflow is expected on the second.
>>>>>> However it is observed that sometimes we get an overflow
>>>>>> on the first run. It looks related to some variability of
>>>>>> the mem_acess count. This variability is observed on all
>>>>>> HW I have access to, with different span though. On
>>>>>> ThunderX2 HW it looks the margin that is currently taken
>>>>>> is too small and we regularly hit failure.
>>>>>>
>>>>>> although the first goal of this series is to increase
>>>>>> the count/margin used in those tests, it also attempts
>>>>>> to improve the pmu-chain-promotion logs, add some barriers
>>>>>> in the mem-access loop, clarify the chain counter
>>>>>> enable/disable sequence.
>>>>>>
>>>>>> A new 'pmu-memaccess-reliability' is also introduced to
>>>>>> detect issues with MEM_ACCESS event variability and make
>>>>>> the debug easier.
>>> As a minor nit, 'pmu-mem-access-reliability' would be more consistent with
>>> 'pmu-mem-access'. The lack of a dash in 'memaccess' tripped me up while I was
>>> trying to run those two tests.
>> I can easily respin with that renaming. thanks for the feedback. Waiting
>> a little bit more if somebody has any other comment.
> Started reviewing the series last week, something came up and I had to stop
> after the first patch (very nice improvement, by the way). I will finish
> the review by the end of this week.

OK no problem. Thanks for your time.

Eric
>
> Thanks,
> Alex
>
>> Eric
>>>>>> Obviously one can wonder if this variability is something normal
>>>>>> and does not hide any other bug. I hope this series will raise
>>>>>> additional discussions about this.
>>>>>>
>>>>>> https://github.com/eauger/kut/tree/pmu-chain-promotion-fixes-v1
>>>>> Gentle ping.
>>>> I'd be happy to take this, but I was hoping to see some r-b's and/or t-b's
>>>> from some of the others.
>>> I gave this a spin on my ThunderX2, and it seems to fix the intermittent
>>> failures I was seeing.
>>>
>>> FWIW:
>>>
>>> Tested-by: Mark Rutland <mark.rutland@arm.com>
>>>
>>> Before (on commit 4ba7058c61e8922f9c8397cfa1095fac325f809b):
>>>
>>> Test results below.
>>>
>>> | [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL= useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-chain-promotion'
>>> | timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-chain-promotion # -initrd /tmp/tmp.nl1i6S0EIY
>>> | INFO: PMU version: 0x4
>>> | INFO: PMU implementer/ID code: 0(" ")/0
>>> | INFO: Implements 6 event counters
>>> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: chain counter not counting if even counter is disabled
>>> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: odd counter did not increment on overflow if disabled
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x7
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 has value 0x0
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: overflow counter 0x1
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x4
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x1b
>>> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: should have triggered an overflow on #0
>>> | FAIL: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 shouldn't have incremented
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: counter #0 = 0xffffffdc, counter #1 = 0x0 overflow=0x0
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x4
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x1b
>>> | FAIL: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter enabled: CHAIN counter was incremented and overflow
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 = 0x0, overflow=0x1
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x4
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: MEM_ACCESS counter #0 has value 0x1b
>>> | FAIL: pmu: pmu-chain-promotion: 32-bit overflows: 32b->64b: CHAIN counter incremented and overflow
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: CHAIN counter #1 = 0x0, overflow=0x1
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: counter #0=0xfffffff3, counter #1=0x0
>>> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: overflow is expected on counter 0
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: counter #0=0xa, counter #1=0xf9 overflow=0x1
>>> | SUMMARY: 7 tests, 3 unexpected failures
>>>
>>> After:
>>>
>>> | [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL=kvm useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-chain-promotion'
>>> | timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-chain-promotion # -initrd /tmp/tmp.pahLyg1F3s
>>> | INFO: PMU version: 0x4
>>> | INFO: PMU implementer/ID code: 0(" ")/0
>>> | INFO: Implements 6 event counters
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest1: post #1=0x0 #0=0x0 overflow=0x0
>>> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest1: chain counter not counting if even counter is disabled
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest2: post #1=0x0 #0=0xf3 overflow=0x1
>>> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest2: odd counter did not increment on overflow if disabled
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: init #1=0x0 #0=0xfffffea1 overflow=0x0
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: After 1st loop #1=0x0 #0=0xffffffa0 overflow=0x0
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: After 2d loop #1=0x0 #0=0xc0 overflow=0x1
>>> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: should have triggered an overflow on #0
>>> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest3: CHAIN counter #1 shouldn't have incremented
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: init #1=0x0 #0=0xfffffea1 overflow=0x0
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: After 1st loop #1=0x0 #0=0xffffffb7 overflow=0x0
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: After 2d loop #1=0x1 #0=0xbc overflow=0x1
>>> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest4: CHAIN counter enabled: CHAIN counter was incremented and overflow
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: init #1=0x0 #0=0xfffffea1 overflow=0x0
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: After 1st loop #1=0x22c #0=0xffffff9f overflow=0x0
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: After 2d loop #1=0x1 #0=0x9d overflow=0x1
>>> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest5: 32b->64b: CHAIN counter incremented and overflow
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: init #1=0x0 #0=0xfffffea1 overflow=0x0
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: After 1st loop #1=0x0 #0=0xffffff9f overflow=0x0
>>> | INFO: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: After 2d loop #1=0x1f9 #0=0x9c overflow=0x1
>>> | PASS: pmu: pmu-chain-promotion: 32-bit overflows: subtest6: overflow is expected on counter 0
>>> | SUMMARY: 7 tests
>>>
>>> As a bonus, the mem-access and memaccess-reliability results:
>>>
>>> | [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL=kvm useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-mem-access'     
>>> | timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-mem-access # -initrd /tmp/tmp.84AeEp8Tiw
>>> | INFO: PMU version: 0x4
>>> | INFO: PMU implementer/ID code: 0(" ")/0
>>> | INFO: Implements 6 event counters
>>> | INFO: pmu: pmu-mem-access: 32-bit overflows: counter #0 is 0x15 (MEM_ACCESS)
>>> | INFO: pmu: pmu-mem-access: 32-bit overflows: counter #1 is 0x15 (MEM_ACCESS)
>>> | PASS: pmu: pmu-mem-access: 32-bit overflows: Ran 20 mem accesses
>>> | PASS: pmu: pmu-mem-access: 32-bit overflows: Ran 20 mem accesses with expected overflows on both counters
>>> | INFO: pmu: pmu-mem-access: 32-bit overflows: cnt#0=0x8 cnt#1=0x8 overflow=0x3
>>> | SKIP: pmu: pmu-mem-access: 64-bit overflows: Skip test as 64 overflows need FEAT_PMUv3p5
>>> | SUMMARY: 3 tests, 1 skipped
>>> | [mark@gravadlaks:~/src/kvm-unit-tests]% TESTNAME=pmu-chain-promotion TIMEOUT=90s ACCEL=kvm useapp qemu ./arm/run arm/pmu.flat -smp 1 -append 'pmu-memaccess-reliability'
>>> | timeout -k 1s --foreground 90s /home/mark/.opt/apps/qemu/bin/qemu-system-aarch64 -nodefaults -machine virt,gic-version=host -accel kvm -cpu host -device virtio-serial-device -device virtconsole,chardev=ctd -chardev testdev,id=ctd -device pci-testdev -display none -serial stdio -kernel arm/pmu.flat -smp 1 -append pmu-memaccess-reliability # -initrd /tmp/tmp.ZToqwencZR
>>> | INFO: PMU version: 0x4
>>> | INFO: PMU implementer/ID code: 0(" ")/0
>>> | INFO: Implements 6 event counters
>>> | INFO: pmu: pmu-memaccess-reliability: 32-bit overflows: overflow=0 min=251 max=283 COUNT=250 MARGIN=100
>>> | PASS: pmu: pmu-memaccess-reliability: 32-bit overflows: memaccess is reliable
>>> | SKIP: pmu: pmu-memaccess-reliability: 64-bit overflows: Skip test as 64 overflows need FEAT_PMUv3p5
>>> | SUMMARY: 2 tests, 1 skipped
>>>
>>> Thanks,
>>> Mark.
>>>

Re: [kvm-unit-tests PATCH 1/6] arm: pmu: pmu-chain-promotion: Improve debug messages

[Alexandru Elisei]

Hi,

On Wed, Mar 15, 2023 at 12:07:20PM +0100, Eric Auger wrote:
> The pmu-chain-promotion test is composed of several subtests.
> In case of failures, the current logs are really dificult to
> analyze since they look very similar and sometimes duplicated
> for each subtest. Add prefixes for each subtest and introduce
> a macro that prints the registers we are mostly interested in,
> namerly the 2 first counters and the overflow counter.

One possible typo below.

Ran pmu-chain-promotion with and without this patch applied, the
improvement is very noticeable, it makes it very easy to match the debug
message with the subtest being run:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arm/pmu.c | 63 ++++++++++++++++++++++++++++---------------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index f6e95012..dad7d4b4 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -715,6 +715,11 @@ static void test_chained_sw_incr(bool unused)
>  	report_info("overflow=0x%lx, #0=0x%lx #1=0x%lx", read_sysreg(pmovsclr_el0),
>  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
>  }
> +#define PRINT_REGS(__s) \
> +	report_info("%s #1=0x%lx #0=0x%lx overflow=0x%lx", __s, \
> +		    read_regn_el0(pmevcntr, 1), \
> +		    read_regn_el0(pmevcntr, 0), \
> +		    read_sysreg(pmovsclr_el0))
>  
>  static void test_chain_promotion(bool unused)
>  {
> @@ -725,6 +730,7 @@ static void test_chain_promotion(bool unused)
>  		return;
>  
>  	/* Only enable CHAIN counter */
> +	report_prefix_push("subtest1");
>  	pmu_reset();
>  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
>  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
> @@ -732,83 +738,81 @@ static void test_chain_promotion(bool unused)
>  	isb();
>  
>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	PRINT_REGS("post");
>  	report(!read_regn_el0(pmevcntr, 0),
>  		"chain counter not counting if even counter is disabled");
> +	report_prefix_pop();
>  
>  	/* Only enable even counter */
> +	report_prefix_push("subtest2");
>  	pmu_reset();
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW_32);
>  	write_sysreg_s(0x1, PMCNTENSET_EL0);
>  	isb();
>  
>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	PRINT_REGS("post");
>  	report(!read_regn_el0(pmevcntr, 1) && (read_sysreg(pmovsclr_el0) == 0x1),
>  		"odd counter did not increment on overflow if disabled");
> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> -		    read_regn_el0(pmevcntr, 0));
> -	report_info("CHAIN counter #1 has value 0x%lx",
> -		    read_regn_el0(pmevcntr, 1));
> -	report_info("overflow counter 0x%lx", read_sysreg(pmovsclr_el0));
> +	report_prefix_pop();
>  
>  	/* start at 0xFFFFFFDC, +20 with CHAIN enabled, +20 with CHAIN disabled */
> +	report_prefix_push("subtest3");
>  	pmu_reset();
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
>  	isb();
> +	PRINT_REGS("init");
>  
>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> -		    read_regn_el0(pmevcntr, 0));
> +	PRINT_REGS("After 1st loop");
>  
>  	/* disable the CHAIN event */
>  	write_sysreg_s(0x2, PMCNTENCLR_EL0);
>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> -		    read_regn_el0(pmevcntr, 0));
> +	PRINT_REGS("After 2d loop");

Hmm.. was that supposed to be "after 2**n**d loop" (matches the "after 1st
loop" message)? A few more instances below.

Thanks,
Alex

>  	report(read_sysreg(pmovsclr_el0) == 0x1,
>  		"should have triggered an overflow on #0");
>  	report(!read_regn_el0(pmevcntr, 1),
>  		"CHAIN counter #1 shouldn't have incremented");
> +	report_prefix_pop();
>  
>  	/* start at 0xFFFFFFDC, +20 with CHAIN disabled, +20 with CHAIN enabled */
>  
> +	report_prefix_push("subtest4");
>  	pmu_reset();
>  	write_sysreg_s(0x1, PMCNTENSET_EL0);
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
>  	isb();
> -	report_info("counter #0 = 0x%lx, counter #1 = 0x%lx overflow=0x%lx",
> -		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1),
> -		    read_sysreg(pmovsclr_el0));
> +	PRINT_REGS("init");
>  
>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> -		    read_regn_el0(pmevcntr, 0));
> +	PRINT_REGS("After 1st loop");
>  
>  	/* enable the CHAIN event */
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	isb();
>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> -		    read_regn_el0(pmevcntr, 0));
> +
> +	PRINT_REGS("After 2d loop");
>  
>  	report((read_regn_el0(pmevcntr, 1) == 1) &&
>  		(read_sysreg(pmovsclr_el0) == 0x1),
>  		"CHAIN counter enabled: CHAIN counter was incremented and overflow");
> -
> -	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> -		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> +	report_prefix_pop();
>  
>  	/* start as MEM_ACCESS/CPU_CYCLES and move to CHAIN/MEM_ACCESS */
> +	report_prefix_push("subtest5");
>  	pmu_reset();
>  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
>  	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
>  	isb();
> +	PRINT_REGS("init");
>  
>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> -		    read_regn_el0(pmevcntr, 0));
> +	PRINT_REGS("After 1st loop");
>  
>  	/* 0 becomes CHAINED */
>  	write_sysreg_s(0x0, PMCNTENSET_EL0);
> @@ -817,37 +821,34 @@ static void test_chain_promotion(bool unused)
>  	write_regn_el0(pmevcntr, 1, 0x0);
>  
>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> -		    read_regn_el0(pmevcntr, 0));
> +	PRINT_REGS("After 2d loop");
>  
>  	report((read_regn_el0(pmevcntr, 1) == 1) &&
>  		(read_sysreg(pmovsclr_el0) == 0x1),
>  		"32b->64b: CHAIN counter incremented and overflow");
> -
> -	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> -		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> +	report_prefix_pop();
>  
>  	/* start as CHAIN/MEM_ACCESS and move to MEM_ACCESS/CPU_CYCLES */
> +	report_prefix_push("subtest6");
>  	pmu_reset();
>  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
>  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	PRINT_REGS("init");
>  
>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> -	report_info("counter #0=0x%lx, counter #1=0x%lx",
> -			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> +	PRINT_REGS("After 1st loop");
>  
>  	write_sysreg_s(0x0, PMCNTENSET_EL0);
>  	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  
>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	PRINT_REGS("After 2d loop");
>  	report(read_sysreg(pmovsclr_el0) == 1,
>  		"overflow is expected on counter 0");
> -	report_info("counter #0=0x%lx, counter #1=0x%lx overflow=0x%lx",
> -			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1),
> -			read_sysreg(pmovsclr_el0));
> +	report_prefix_pop();
>  }
>  
>  static bool expect_interrupts(uint32_t bitmap)
> -- 
> 2.38.1
> 

Re: [kvm-unit-tests PATCH 2/6] arm: pmu: pmu-chain-promotion: Introduce defines for count and margin values

[Alexandru Elisei]

Hi,

On Wed, Mar 15, 2023 at 12:07:21PM +0100, Eric Auger wrote:
> The pmu-chain-promotion test is composed of separate subtests.
> 
> Some of them apply some settings on a first MEM_ACCESS loop
> iterations, change the settings and run another MEM_ACCESS loop.
> 
> The PRE_OVERFLOW2 MEM_ACCESS counter init value is defined so that
> the first loop does not overflow and the second loop overflows.
> 
> At the moment the MEM_ACCESS count number is hardcoded to 20 and
> PRE_OVERFLOW2 is set to UINT32_MAX - 20 - 15 where 15 acts as a
> margin.
> 
> Introduce defines for the count number and the margin so that it
> becomes easier to change them.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arm/pmu.c | 35 +++++++++++++++++++++--------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index dad7d4b4..b88366a8 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -55,11 +55,18 @@
>  #define EXT_COMMON_EVENTS_LOW	0x4000
>  #define EXT_COMMON_EVENTS_HIGH	0x403F
>  
> -#define ALL_SET_32			0x00000000FFFFFFFFULL
> +#define ALL_SET_32		0x00000000FFFFFFFFULL
>  #define ALL_CLEAR		0x0000000000000000ULL
>  #define PRE_OVERFLOW_32		0x00000000FFFFFFF0ULL
> -#define PRE_OVERFLOW2_32	0x00000000FFFFFFDCULL
>  #define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
> +#define COUNT 20

test_mem_access (from the test "pmu-mem-access") also uses 20 for
mem_access_loop, in case you want to change the define there too.

I realize I'm bikeshedding here, but it might also help if the define name
held some clue to what is being counted (like ACCESS_COUNT, or something
like that).

> +#define MARGIN 15
> +/*
> + * PRE_OVERFLOW2 is set so that 1st COUNT iterations do not
> + * produce 32b overflow and 2d COUNT iterations do. To accommodate

2**nd** COUNT iterations?

> + * for some observed variability we take into account a given @MARGIN

Some inconsistency here, this variable is referred to with @MARGIN, but
COUNT isn't (missing "@").

> + */
> +#define PRE_OVERFLOW2_32		(ALL_SET_32 - COUNT - MARGIN)

This is much better, I would have been hard pressed to figure out where the
previous value of 0x00000000FFFFFFDCULL came from.

The patch looks good to me (with or without the comments above):

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks,
Alex

>  
>  #define PRE_OVERFLOW(__overflow_at_64bits)				\
>  	(__overflow_at_64bits ? PRE_OVERFLOW_64 : PRE_OVERFLOW_32)
> @@ -737,7 +744,7 @@ static void test_chain_promotion(bool unused)
>  	write_sysreg_s(0x2, PMCNTENSET_EL0);
>  	isb();
>  
> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("post");
>  	report(!read_regn_el0(pmevcntr, 0),
>  		"chain counter not counting if even counter is disabled");
> @@ -750,13 +757,13 @@ static void test_chain_promotion(bool unused)
>  	write_sysreg_s(0x1, PMCNTENSET_EL0);
>  	isb();
>  
> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("post");
>  	report(!read_regn_el0(pmevcntr, 1) && (read_sysreg(pmovsclr_el0) == 0x1),
>  		"odd counter did not increment on overflow if disabled");
>  	report_prefix_pop();
>  
> -	/* start at 0xFFFFFFDC, +20 with CHAIN enabled, +20 with CHAIN disabled */
> +	/* 1st COUNT with CHAIN enabled, next COUNT with CHAIN disabled */
>  	report_prefix_push("subtest3");
>  	pmu_reset();
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> @@ -764,12 +771,12 @@ static void test_chain_promotion(bool unused)
>  	isb();
>  	PRINT_REGS("init");
>  
> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 1st loop");
>  
>  	/* disable the CHAIN event */
>  	write_sysreg_s(0x2, PMCNTENCLR_EL0);
> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 2d loop");
>  	report(read_sysreg(pmovsclr_el0) == 0x1,
>  		"should have triggered an overflow on #0");
> @@ -777,7 +784,7 @@ static void test_chain_promotion(bool unused)
>  		"CHAIN counter #1 shouldn't have incremented");
>  	report_prefix_pop();
>  
> -	/* start at 0xFFFFFFDC, +20 with CHAIN disabled, +20 with CHAIN enabled */
> +	/* 1st COUNT with CHAIN disabled, next COUNT with CHAIN enabled */
>  
>  	report_prefix_push("subtest4");
>  	pmu_reset();
> @@ -786,13 +793,13 @@ static void test_chain_promotion(bool unused)
>  	isb();
>  	PRINT_REGS("init");
>  
> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 1st loop");
>  
>  	/* enable the CHAIN event */
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	isb();
> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  
>  	PRINT_REGS("After 2d loop");
>  
> @@ -811,7 +818,7 @@ static void test_chain_promotion(bool unused)
>  	isb();
>  	PRINT_REGS("init");
>  
> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 1st loop");
>  
>  	/* 0 becomes CHAINED */
> @@ -820,7 +827,7 @@ static void test_chain_promotion(bool unused)
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	write_regn_el0(pmevcntr, 1, 0x0);
>  
> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 2d loop");
>  
>  	report((read_regn_el0(pmevcntr, 1) == 1) &&
> @@ -837,14 +844,14 @@ static void test_chain_promotion(bool unused)
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	PRINT_REGS("init");
>  
> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 1st loop");
>  
>  	write_sysreg_s(0x0, PMCNTENSET_EL0);
>  	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  
> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 2d loop");
>  	report(read_sysreg(pmovsclr_el0) == 1,
>  		"overflow is expected on counter 0");
> -- 
> 2.38.1
> 
> 

Re: [kvm-unit-tests PATCH 3/6] arm: pmu: Add extra DSB barriers in the mem_access loop

[Alexandru Elisei]

Hi,

On Wed, Mar 15, 2023 at 12:07:22PM +0100, Eric Auger wrote:
> The mem access loop currently features ISB barriers only. However
> the mem_access loop counts the number of accesses to memory. ISB
> do not garantee the PE cannot reorder memory access. Let's
> add a DSB ISH before the write to PMCR_EL0 that enables the PMU
> and after the last iteration, before disabling the PMU.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com>
> 
> ---
> 
> This was discussed in https://lore.kernel.org/all/YzxmHpV2rpfaUdWi@monolith.localdoman/
> ---
>  arm/pmu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index b88366a8..dde399e2 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -301,6 +301,7 @@ static void mem_access_loop(void *addr, long loop, uint32_t pmcr)
>  {
>  	uint64_t pmcr64 = pmcr;
>  asm volatile(
> +	"       dsb     ish\n"

I think it might still be possible to reorder memory accesses which are
part of the loop after the DSB above and before the PMU is enabled below.
But the DSB above is needed to make sure previous memory accesses, which
shouldn't be counted as part of the loop, are completed.

I would put another DSB after the ISB which enables the PMU, that way all
memory accesses are neatly sandwitches between two DSBs.

Having 3 DSBs might look like overdoing it, but I reason it to be correct.
What do you think?

Thanks,
Alex

>  	"       msr     pmcr_el0, %[pmcr]\n"
>  	"       isb\n"
>  	"       mov     x10, %[loop]\n"
> @@ -308,6 +309,7 @@ asm volatile(
>  	"       ldr	x9, [%[addr]]\n"
>  	"       cmp     x10, #0x0\n"
>  	"       b.gt    1b\n"
> +	"       dsb     ish\n"
>  	"       msr     pmcr_el0, xzr\n"
>  	"       isb\n"
>  	:
> -- 
> 2.38.1
> 
> 

Re: [kvm-unit-tests PATCH 4/6] arm: pmu: Fix chain counter enable/disable sequences

[Alexandru Elisei]

Hi,

On Wed, Mar 15, 2023 at 12:07:23PM +0100, Eric Auger wrote:
> In some ARM ARM ddi0487 revisions it is said that
> disabling/enabling a pair of counters that are paired
> by a CHAIN event should follow a given sequence:
> 
> Disable the low counter first, isb, disable the high counter
> Enable the high counter first, isb, enable low counter
> 
> This was the case in Fc. However this is not written anymore
> in Ia revision.
> 
> Introduce 2 helpers to execute those sequences and replace
> the existing PMCNTENCLR/ENSET calls.
> 
> Also fix 2 write_sysreg_s(0x0, PMCNTENSET_EL0) in subtest 5 & 6
> and replace them by PMCNTENCLR writes since writing 0 in
> PMCNTENSET_EL0 has no effect.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arm/pmu.c | 37 ++++++++++++++++++++++++++++---------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index dde399e2..af679667 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -730,6 +730,22 @@ static void test_chained_sw_incr(bool unused)
>  		    read_regn_el0(pmevcntr, 0), \
>  		    read_sysreg(pmovsclr_el0))
>  
> +static void enable_chain_counter(int even)
> +{
> +	write_sysreg_s(BIT(even), PMCNTENSET_EL0); /* Enable the high counter first */
> +	isb();
> +	write_sysreg_s(BIT(even + 1), PMCNTENSET_EL0); /* Enable the low counter */
> +	isb();
> +}

In ARM DDI 0487F.b, at the bottom of page D7-2727:

"When enabling a pair of counters that are paired by a CHAIN event,
software must:

1. Enable the high counter, by setting PMCNTENCLR_EL0[n+1] to 0 and, if
necessary, setting PMCR_EL0.E to 1.
2. Execute an ISB instruction, or perform another Context synchronization
event.
3. Enable the low counter by setting PMCNTENCLR_EL0[n] to 0."

Which matches the commit message, but not the code above. Am I
misunderstanding what is the high and low counter? In the example from the
Arm ARM, just before the snippet above, the odd numbered countered is
called the high counter.

CHAIN is also defined as:

[..] the odd-numbered event counter n+1 increments when an event increments
the preceding even-numbered counter n on the same PE and causes an unsigned
overflow of bits [31:0] of event counter n.

So it would make sense to enable the odd counter first, then the even, so
no overflows are missed if the sequence was the other way around (even
counter enabled; overflow missed because odd counter disabled; odd counter
enabled).

Same observation with disable_chain_counter().

> +
> +static void disable_chain_counter(int even)
> +{
> +	write_sysreg_s(BIT(even + 1), PMCNTENCLR_EL0); /* Disable the low counter first*/
> +	isb();
> +	write_sysreg_s(BIT(even), PMCNTENCLR_EL0); /* Disable the high counter */
> +	isb();
> +}
> +
>  static void test_chain_promotion(bool unused)
>  {
>  	uint32_t events[] = {MEM_ACCESS, CHAIN};
> @@ -768,16 +784,17 @@ static void test_chain_promotion(bool unused)
>  	/* 1st COUNT with CHAIN enabled, next COUNT with CHAIN disabled */
>  	report_prefix_push("subtest3");
>  	pmu_reset();
> -	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
> -	isb();
> +	enable_chain_counter(0);
>  	PRINT_REGS("init");
>  
>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 1st loop");
>  
>  	/* disable the CHAIN event */
> -	write_sysreg_s(0x2, PMCNTENCLR_EL0);
> +	disable_chain_counter(0);
> +	write_sysreg_s(0x1, PMCNTENSET_EL0); /* Enable the low counter */
> +	isb();
>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 2d loop");
>  	report(read_sysreg(pmovsclr_el0) == 0x1,
> @@ -798,9 +815,11 @@ static void test_chain_promotion(bool unused)
>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 1st loop");
>  
> -	/* enable the CHAIN event */
> -	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	/* Disable the even counter and enable the chain counter */
> +	write_sysreg_s(0x1, PMCNTENCLR_EL0); /* Disable the low counter first */

The comment says disable the even counter, but the odd counter is disabled.
Which Arm ARM refers to as the high counter. I'm properly confused about
the naming.

Thanks,
Alex

>  	isb();
> +	enable_chain_counter(0);
> +
>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  
>  	PRINT_REGS("After 2d loop");
> @@ -824,10 +843,10 @@ static void test_chain_promotion(bool unused)
>  	PRINT_REGS("After 1st loop");
>  
>  	/* 0 becomes CHAINED */
> -	write_sysreg_s(0x0, PMCNTENSET_EL0);
> +	write_sysreg_s(0x3, PMCNTENCLR_EL0);
>  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
> -	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	write_regn_el0(pmevcntr, 1, 0x0);
> +	enable_chain_counter(0);
>  
>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 2d loop");
> @@ -843,13 +862,13 @@ static void test_chain_promotion(bool unused)
>  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
>  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
> -	write_sysreg_s(0x3, PMCNTENSET_EL0);
> +	enable_chain_counter(0);
>  	PRINT_REGS("init");
>  
>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>  	PRINT_REGS("After 1st loop");
>  
> -	write_sysreg_s(0x0, PMCNTENSET_EL0);
> +	disable_chain_counter(0);
>  	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  
> -- 
> 2.38.1
> 
> 

Re: [kvm-unit-tests PATCH 5/6] arm: pmu: Add pmu-memaccess-reliability test

[Alexandru Elisei]

Hi,

On Wed, Mar 15, 2023 at 12:07:24PM +0100, Eric Auger wrote:
> Add a new basic test that runs MEM_ACCESS loop over
> 100 iterations and make sure the number of measured
> MEM_ACCESS never overflows the margin. Some other
> pmu tests rely on this pattern and if the MEM_ACCESS
> measurement is not reliable, it is better to report
> it beforehand and not confuse the user any further.
> 
> Without the subsequent patch, this typically fails on
> ThunderXv2 with the following logs:
> 
> INFO: pmu: pmu-memaccess-reliability: 32-bit overflows:
> overflow=1 min=21 max=41 COUNT=20 MARGIN=15
> FAIL: pmu: pmu-memaccess-reliability: 32-bit overflows:
> memaccess is reliable
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  arm/pmu.c         | 52 +++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg |  6 ++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index af679667..c3d2a428 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -56,6 +56,7 @@
>  #define EXT_COMMON_EVENTS_HIGH	0x403F
>  
>  #define ALL_SET_32		0x00000000FFFFFFFFULL
> +#define ALL_SET_64		0xFFFFFFFFFFFFFFFFULL
>  #define ALL_CLEAR		0x0000000000000000ULL
>  #define PRE_OVERFLOW_32		0x00000000FFFFFFF0ULL
>  #define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
> @@ -67,6 +68,10 @@
>   * for some observed variability we take into account a given @MARGIN
>   */
>  #define PRE_OVERFLOW2_32		(ALL_SET_32 - COUNT - MARGIN)
> +#define PRE_OVERFLOW2_64		(ALL_SET_64 - COUNT - MARGIN)
> +
> +#define PRE_OVERFLOW2(__overflow_at_64bits)				\
> +	(__overflow_at_64bits ? PRE_OVERFLOW2_64 : PRE_OVERFLOW2_32)
>  
>  #define PRE_OVERFLOW(__overflow_at_64bits)				\
>  	(__overflow_at_64bits ? PRE_OVERFLOW_64 : PRE_OVERFLOW_32)
> @@ -746,6 +751,50 @@ static void disable_chain_counter(int even)
>  	isb();
>  }
>  
> +static void test_memaccess_reliability(bool overflow_at_64bits)
> +{
> +	uint32_t events[] = {MEM_ACCESS};
> +	void *addr = malloc(PAGE_SIZE);
> +	uint64_t count, max = 0, min = pmevcntr_mask();
> +	uint64_t pre_overflow2 = PRE_OVERFLOW2(overflow_at_64bits);
> +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
> +	bool overflow = false;
> +
> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)) ||
> +	    !check_overflow_prerequisites(overflow_at_64bits))
> +		return;
> +
> +	pmu_reset();
> +	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
> +	for (int i = 0; i < 100; i++) {
> +		pmu_reset();
> +		write_regn_el0(pmevcntr, 0, pre_overflow2);
> +		write_sysreg_s(0x1, PMCNTENSET_EL0);
> +		isb();
> +		mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> +		count = read_regn_el0(pmevcntr, 0);
> +		if (count < pre_overflow2) {
> +			count += COUNT + MARGIN;
> +			if (count > max)
> +				max = count;
> +			if (count < min)
> +				min = count;
> +			overflow = true;
> +			report_info("iter=%d count=%ld min=%ld max=%ld overflow!!!",
> +				    i, count, min, max);
> +			continue;
> +		}
> +		count -= pre_overflow2;
> +		if (count > max)
> +			max = count;
> +		if (count < min)
> +			min = count;

I'm having difficulties following the above maze of conditions. That's not going
to be easy to maintain.

If I understand the commit message correctly, the point of this test is to check
that PRE_OVERFLOW2 + COUNT doesn't overflow, but PRE_OVERFLOW2 + 2 * COUNT does.
How about this simpler approach instead:

	for (int i = 0; i < 100; i++) {
		pmu_reset();
		write_regn_el0(pmevcntr, 0, pre_overflow2);
		write_sysreg_s(0x1, PMCNTENSET_EL0);
		isb();

		mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
		count = read_regn_el0(pmevcntr, 0);
		/* Counter overflowed when it shouldn't. */
		if (count < pre_overflow2) {
			report_fail("reliable memaccess loop");
			return;
		}

		mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
		count = read_regn_el0(pmevcntr, 0);
		/* Counter didn't overflow when it should. */
		if (count >= pre_overflow2) {
			report_fail("reliable memaccess loop");
			return;
		}
	}

	report_success("reliable memaccess loop");

Thanks,
Alex

>  static void test_chain_promotion(bool unused)
>  {
>  	uint32_t events[] = {MEM_ACCESS, CHAIN};
> @@ -1203,6 +1252,9 @@ int main(int argc, char *argv[])
>  	} else if (strcmp(argv[1], "pmu-basic-event-count") == 0) {
>  		run_event_test(argv[1], test_basic_event_count, false);
>  		run_event_test(argv[1], test_basic_event_count, true);
> +	} else if (strcmp(argv[1], "pmu-memaccess-reliability") == 0) {
> +		run_event_test(argv[1], test_memaccess_reliability, false);
> +		run_event_test(argv[1], test_memaccess_reliability, true);
>  	} else if (strcmp(argv[1], "pmu-mem-access") == 0) {
>  		run_event_test(argv[1], test_mem_access, false);
>  		run_event_test(argv[1], test_mem_access, true);
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index 5e67b558..301261aa 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -90,6 +90,12 @@ groups = pmu
>  arch = arm64
>  extra_params = -append 'pmu-mem-access'
>  
> +[pmu-memaccess-reliability]
> +file = pmu.flat
> +groups = pmu
> +arch = arm64
> +extra_params = -append 'pmu-memaccess-reliability'
> +
>  [pmu-sw-incr]
>  file = pmu.flat
>  groups = pmu
> -- 
> 2.38.1
> 

Re: [kvm-unit-tests PATCH 4/6] arm: pmu: Fix chain counter enable/disable sequences

[Marc Zyngier]

On 2023-04-21 11:52, Alexandru Elisei wrote:
> Hi,
> 
> On Wed, Mar 15, 2023 at 12:07:23PM +0100, Eric Auger wrote:
>> In some ARM ARM ddi0487 revisions it is said that
>> disabling/enabling a pair of counters that are paired
>> by a CHAIN event should follow a given sequence:
>> 
>> Disable the low counter first, isb, disable the high counter
>> Enable the high counter first, isb, enable low counter
>> 
>> This was the case in Fc. However this is not written anymore
>> in Ia revision.
>> 
>> Introduce 2 helpers to execute those sequences and replace
>> the existing PMCNTENCLR/ENSET calls.
>> 
>> Also fix 2 write_sysreg_s(0x0, PMCNTENSET_EL0) in subtest 5 & 6
>> and replace them by PMCNTENCLR writes since writing 0 in
>> PMCNTENSET_EL0 has no effect.
>> 
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  arm/pmu.c | 37 ++++++++++++++++++++++++++++---------
>>  1 file changed, 28 insertions(+), 9 deletions(-)
>> 
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index dde399e2..af679667 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -730,6 +730,22 @@ static void test_chained_sw_incr(bool unused)
>>  		    read_regn_el0(pmevcntr, 0), \
>>  		    read_sysreg(pmovsclr_el0))
>> 
>> +static void enable_chain_counter(int even)
>> +{
>> +	write_sysreg_s(BIT(even), PMCNTENSET_EL0); /* Enable the high 
>> counter first */
>> +	isb();
>> +	write_sysreg_s(BIT(even + 1), PMCNTENSET_EL0); /* Enable the low 
>> counter */
>> +	isb();
>> +}
> 
> In ARM DDI 0487F.b, at the bottom of page D7-2727:
> 
> "When enabling a pair of counters that are paired by a CHAIN event,
> software must:
> 
> 1. Enable the high counter, by setting PMCNTENCLR_EL0[n+1] to 0 and, if
> necessary, setting PMCR_EL0.E to 1.
> 2. Execute an ISB instruction, or perform another Context 
> synchronization
> event.
> 3. Enable the low counter by setting PMCNTENCLR_EL0[n] to 0."

This particular text seems to have been removed from the H.a and I.a
revisions of the ARM ARM, and I cannot spot any equivalent requirement.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [kvm-unit-tests PATCH 1/6] arm: pmu: pmu-chain-promotion: Improve debug messages

[Eric Auger]

Hi Alexandru,

On 4/21/23 11:25, Alexandru Elisei wrote:
> Hi,
>
> On Wed, Mar 15, 2023 at 12:07:20PM +0100, Eric Auger wrote:
>> The pmu-chain-promotion test is composed of several subtests.
>> In case of failures, the current logs are really dificult to
>> analyze since they look very similar and sometimes duplicated
>> for each subtest. Add prefixes for each subtest and introduce
>> a macro that prints the registers we are mostly interested in,
>> namerly the 2 first counters and the overflow counter.
> One possible typo below.
renamed 2d into 2nd as suggested.
>
> Ran pmu-chain-promotion with and without this patch applied, the
> improvement is very noticeable, it makes it very easy to match the debug
> message with the subtest being run:
>
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Thanks!

Eric
>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  arm/pmu.c | 63 ++++++++++++++++++++++++++++---------------------------
>>  1 file changed, 32 insertions(+), 31 deletions(-)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index f6e95012..dad7d4b4 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -715,6 +715,11 @@ static void test_chained_sw_incr(bool unused)
>>  	report_info("overflow=0x%lx, #0=0x%lx #1=0x%lx", read_sysreg(pmovsclr_el0),
>>  		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
>>  }
>> +#define PRINT_REGS(__s) \
>> +	report_info("%s #1=0x%lx #0=0x%lx overflow=0x%lx", __s, \
>> +		    read_regn_el0(pmevcntr, 1), \
>> +		    read_regn_el0(pmevcntr, 0), \
>> +		    read_sysreg(pmovsclr_el0))
>>  
>>  static void test_chain_promotion(bool unused)
>>  {
>> @@ -725,6 +730,7 @@ static void test_chain_promotion(bool unused)
>>  		return;
>>  
>>  	/* Only enable CHAIN counter */
>> +	report_prefix_push("subtest1");
>>  	pmu_reset();
>>  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
>>  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
>> @@ -732,83 +738,81 @@ static void test_chain_promotion(bool unused)
>>  	isb();
>>  
>>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	PRINT_REGS("post");
>>  	report(!read_regn_el0(pmevcntr, 0),
>>  		"chain counter not counting if even counter is disabled");
>> +	report_prefix_pop();
>>  
>>  	/* Only enable even counter */
>> +	report_prefix_push("subtest2");
>>  	pmu_reset();
>>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW_32);
>>  	write_sysreg_s(0x1, PMCNTENSET_EL0);
>>  	isb();
>>  
>>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	PRINT_REGS("post");
>>  	report(!read_regn_el0(pmevcntr, 1) && (read_sysreg(pmovsclr_el0) == 0x1),
>>  		"odd counter did not increment on overflow if disabled");
>> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
>> -		    read_regn_el0(pmevcntr, 0));
>> -	report_info("CHAIN counter #1 has value 0x%lx",
>> -		    read_regn_el0(pmevcntr, 1));
>> -	report_info("overflow counter 0x%lx", read_sysreg(pmovsclr_el0));
>> +	report_prefix_pop();
>>  
>>  	/* start at 0xFFFFFFDC, +20 with CHAIN enabled, +20 with CHAIN disabled */
>> +	report_prefix_push("subtest3");
>>  	pmu_reset();
>>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
>>  	isb();
>> +	PRINT_REGS("init");
>>  
>>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
>> -		    read_regn_el0(pmevcntr, 0));
>> +	PRINT_REGS("After 1st loop");
>>  
>>  	/* disable the CHAIN event */
>>  	write_sysreg_s(0x2, PMCNTENCLR_EL0);
>>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
>> -		    read_regn_el0(pmevcntr, 0));
>> +	PRINT_REGS("After 2d loop");
> Hmm.. was that supposed to be "after 2**n**d loop" (matches the "after 1st
> loop" message)? A few more instances below.
>
> Thanks,
> Alex
>
>>  	report(read_sysreg(pmovsclr_el0) == 0x1,
>>  		"should have triggered an overflow on #0");
>>  	report(!read_regn_el0(pmevcntr, 1),
>>  		"CHAIN counter #1 shouldn't have incremented");
>> +	report_prefix_pop();
>>  
>>  	/* start at 0xFFFFFFDC, +20 with CHAIN disabled, +20 with CHAIN enabled */
>>  
>> +	report_prefix_push("subtest4");
>>  	pmu_reset();
>>  	write_sysreg_s(0x1, PMCNTENSET_EL0);
>>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
>>  	isb();
>> -	report_info("counter #0 = 0x%lx, counter #1 = 0x%lx overflow=0x%lx",
>> -		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1),
>> -		    read_sysreg(pmovsclr_el0));
>> +	PRINT_REGS("init");
>>  
>>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
>> -		    read_regn_el0(pmevcntr, 0));
>> +	PRINT_REGS("After 1st loop");
>>  
>>  	/* enable the CHAIN event */
>>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>>  	isb();
>>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
>> -		    read_regn_el0(pmevcntr, 0));
>> +
>> +	PRINT_REGS("After 2d loop");
>>  
>>  	report((read_regn_el0(pmevcntr, 1) == 1) &&
>>  		(read_sysreg(pmovsclr_el0) == 0x1),
>>  		"CHAIN counter enabled: CHAIN counter was incremented and overflow");
>> -
>> -	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
>> -		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
>> +	report_prefix_pop();
>>  
>>  	/* start as MEM_ACCESS/CPU_CYCLES and move to CHAIN/MEM_ACCESS */
>> +	report_prefix_push("subtest5");
>>  	pmu_reset();
>>  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
>>  	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
>>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
>>  	isb();
>> +	PRINT_REGS("init");
>>  
>>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
>> -		    read_regn_el0(pmevcntr, 0));
>> +	PRINT_REGS("After 1st loop");
>>  
>>  	/* 0 becomes CHAINED */
>>  	write_sysreg_s(0x0, PMCNTENSET_EL0);
>> @@ -817,37 +821,34 @@ static void test_chain_promotion(bool unused)
>>  	write_regn_el0(pmevcntr, 1, 0x0);
>>  
>>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> -	report_info("MEM_ACCESS counter #0 has value 0x%lx",
>> -		    read_regn_el0(pmevcntr, 0));
>> +	PRINT_REGS("After 2d loop");
>>  
>>  	report((read_regn_el0(pmevcntr, 1) == 1) &&
>>  		(read_sysreg(pmovsclr_el0) == 0x1),
>>  		"32b->64b: CHAIN counter incremented and overflow");
>> -
>> -	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
>> -		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
>> +	report_prefix_pop();
>>  
>>  	/* start as CHAIN/MEM_ACCESS and move to MEM_ACCESS/CPU_CYCLES */
>> +	report_prefix_push("subtest6");
>>  	pmu_reset();
>>  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
>>  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
>>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
>>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>> +	PRINT_REGS("init");
>>  
>>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> -	report_info("counter #0=0x%lx, counter #1=0x%lx",
>> -			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
>> +	PRINT_REGS("After 1st loop");
>>  
>>  	write_sysreg_s(0x0, PMCNTENSET_EL0);
>>  	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
>>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>>  
>>  	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	PRINT_REGS("After 2d loop");
>>  	report(read_sysreg(pmovsclr_el0) == 1,
>>  		"overflow is expected on counter 0");
>> -	report_info("counter #0=0x%lx, counter #1=0x%lx overflow=0x%lx",
>> -			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1),
>> -			read_sysreg(pmovsclr_el0));
>> +	report_prefix_pop();
>>  }
>>  
>>  static bool expect_interrupts(uint32_t bitmap)
>> -- 
>> 2.38.1
>>

Re: [kvm-unit-tests PATCH 2/6] arm: pmu: pmu-chain-promotion: Introduce defines for count and margin values

[Eric Auger]

Hi Alexandru,
On 4/21/23 11:55, Alexandru Elisei wrote:
> Hi,
>
> On Wed, Mar 15, 2023 at 12:07:21PM +0100, Eric Auger wrote:
>> The pmu-chain-promotion test is composed of separate subtests.
>>
>> Some of them apply some settings on a first MEM_ACCESS loop
>> iterations, change the settings and run another MEM_ACCESS loop.
>>
>> The PRE_OVERFLOW2 MEM_ACCESS counter init value is defined so that
>> the first loop does not overflow and the second loop overflows.
>>
>> At the moment the MEM_ACCESS count number is hardcoded to 20 and
>> PRE_OVERFLOW2 is set to UINT32_MAX - 20 - 15 where 15 acts as a
>> margin.
>>
>> Introduce defines for the count number and the margin so that it
>> becomes easier to change them.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  arm/pmu.c | 35 +++++++++++++++++++++--------------
>>  1 file changed, 21 insertions(+), 14 deletions(-)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index dad7d4b4..b88366a8 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -55,11 +55,18 @@
>>  #define EXT_COMMON_EVENTS_LOW	0x4000
>>  #define EXT_COMMON_EVENTS_HIGH	0x403F
>>  
>> -#define ALL_SET_32			0x00000000FFFFFFFFULL
>> +#define ALL_SET_32		0x00000000FFFFFFFFULL
>>  #define ALL_CLEAR		0x0000000000000000ULL
>>  #define PRE_OVERFLOW_32		0x00000000FFFFFFF0ULL
>> -#define PRE_OVERFLOW2_32	0x00000000FFFFFFDCULL
>>  #define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
>> +#define COUNT 20
> test_mem_access (from the test "pmu-mem-access") also uses 20 for
> mem_access_loop, in case you want to change the define there too.
I hesitated to change this but in fact the mem-access test does not
suffer the same flaw and there is no risk we don't overflow when we set
to PRE_OVERFLOW init value as the measure is always larger than 20. so I
decided to keep the hardcoded value in that case.
>
> I realize I'm bikeshedding here, but it might also help if the define name
> held some clue to what is being counted (like ACCESS_COUNT, or something
> like that).
the event which is tested is MEM_ACCESS, that's whence the current name
stems from.
>
>> +#define MARGIN 15
>> +/*
>> + * PRE_OVERFLOW2 is set so that 1st COUNT iterations do not
>> + * produce 32b overflow and 2d COUNT iterations do. To accommodate
> 2**nd** COUNT iterations?
OK
>
>> + * for some observed variability we take into account a given @MARGIN
> Some inconsistency here, this variable is referred to with @MARGIN, but
> COUNT isn't (missing "@").
OK
>
>> + */
>> +#define PRE_OVERFLOW2_32		(ALL_SET_32 - COUNT - MARGIN)
> This is much better, I would have been hard pressed to figure out where the
> previous value of 0x00000000FFFFFFDCULL came from.
>
> The patch looks good to me (with or without the comments above):
>
> Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
thanks

Eric
>
> Thanks,
> Alex
>
>>  
>>  #define PRE_OVERFLOW(__overflow_at_64bits)				\
>>  	(__overflow_at_64bits ? PRE_OVERFLOW_64 : PRE_OVERFLOW_32)
>> @@ -737,7 +744,7 @@ static void test_chain_promotion(bool unused)
>>  	write_sysreg_s(0x2, PMCNTENSET_EL0);
>>  	isb();
>>  
>> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("post");
>>  	report(!read_regn_el0(pmevcntr, 0),
>>  		"chain counter not counting if even counter is disabled");
>> @@ -750,13 +757,13 @@ static void test_chain_promotion(bool unused)
>>  	write_sysreg_s(0x1, PMCNTENSET_EL0);
>>  	isb();
>>  
>> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("post");
>>  	report(!read_regn_el0(pmevcntr, 1) && (read_sysreg(pmovsclr_el0) == 0x1),
>>  		"odd counter did not increment on overflow if disabled");
>>  	report_prefix_pop();
>>  
>> -	/* start at 0xFFFFFFDC, +20 with CHAIN enabled, +20 with CHAIN disabled */
>> +	/* 1st COUNT with CHAIN enabled, next COUNT with CHAIN disabled */
>>  	report_prefix_push("subtest3");
>>  	pmu_reset();
>>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>> @@ -764,12 +771,12 @@ static void test_chain_promotion(bool unused)
>>  	isb();
>>  	PRINT_REGS("init");
>>  
>> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("After 1st loop");
>>  
>>  	/* disable the CHAIN event */
>>  	write_sysreg_s(0x2, PMCNTENCLR_EL0);
>> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("After 2d loop");
>>  	report(read_sysreg(pmovsclr_el0) == 0x1,
>>  		"should have triggered an overflow on #0");
>> @@ -777,7 +784,7 @@ static void test_chain_promotion(bool unused)
>>  		"CHAIN counter #1 shouldn't have incremented");
>>  	report_prefix_pop();
>>  
>> -	/* start at 0xFFFFFFDC, +20 with CHAIN disabled, +20 with CHAIN enabled */
>> +	/* 1st COUNT with CHAIN disabled, next COUNT with CHAIN enabled */
>>  
>>  	report_prefix_push("subtest4");
>>  	pmu_reset();
>> @@ -786,13 +793,13 @@ static void test_chain_promotion(bool unused)
>>  	isb();
>>  	PRINT_REGS("init");
>>  
>> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("After 1st loop");
>>  
>>  	/* enable the CHAIN event */
>>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>>  	isb();
>> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  
>>  	PRINT_REGS("After 2d loop");
>>  
>> @@ -811,7 +818,7 @@ static void test_chain_promotion(bool unused)
>>  	isb();
>>  	PRINT_REGS("init");
>>  
>> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("After 1st loop");
>>  
>>  	/* 0 becomes CHAINED */
>> @@ -820,7 +827,7 @@ static void test_chain_promotion(bool unused)
>>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>>  	write_regn_el0(pmevcntr, 1, 0x0);
>>  
>> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("After 2d loop");
>>  
>>  	report((read_regn_el0(pmevcntr, 1) == 1) &&
>> @@ -837,14 +844,14 @@ static void test_chain_promotion(bool unused)
>>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>>  	PRINT_REGS("init");
>>  
>> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("After 1st loop");
>>  
>>  	write_sysreg_s(0x0, PMCNTENSET_EL0);
>>  	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
>>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>>  
>> -	mem_access_loop(addr, 20, pmu.pmcr_ro | PMU_PMCR_E);
>> +	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("After 2d loop");
>>  	report(read_sysreg(pmovsclr_el0) == 1,
>>  		"overflow is expected on counter 0");
>> -- 
>> 2.38.1
>>
>>

Re: [kvm-unit-tests PATCH 3/6] arm: pmu: Add extra DSB barriers in the mem_access loop

[Eric Auger]

Hi Alexandru,

On 4/21/23 12:25, Alexandru Elisei wrote:
> Hi,
>
> On Wed, Mar 15, 2023 at 12:07:22PM +0100, Eric Auger wrote:
>> The mem access loop currently features ISB barriers only. However
>> the mem_access loop counts the number of accesses to memory. ISB
>> do not garantee the PE cannot reorder memory access. Let's
>> add a DSB ISH before the write to PMCR_EL0 that enables the PMU
>> and after the last iteration, before disabling the PMU.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>
>> ---
>>
>> This was discussed in https://lore.kernel.org/all/YzxmHpV2rpfaUdWi@monolith.localdoman/
>> ---
>>  arm/pmu.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index b88366a8..dde399e2 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -301,6 +301,7 @@ static void mem_access_loop(void *addr, long loop, uint32_t pmcr)
>>  {
>>  	uint64_t pmcr64 = pmcr;
>>  asm volatile(
>> +	"       dsb     ish\n"
> I think it might still be possible to reorder memory accesses which are
> part of the loop after the DSB above and before the PMU is enabled below.
> But the DSB above is needed to make sure previous memory accesses, which
> shouldn't be counted as part of the loop, are completed.
>
> I would put another DSB after the ISB which enables the PMU, that way all
> memory accesses are neatly sandwitches between two DSBs.
>
> Having 3 DSBs might look like overdoing it, but I reason it to be correct.
> What do you think?
I need more time to investigate this. I will come back to you next week
as I am OoO this week. Sorry for the inconvenience.
Thank you for the review!

Eric
>
> Thanks,
> Alex
>
>>  	"       msr     pmcr_el0, %[pmcr]\n"
>>  	"       isb\n"
>>  	"       mov     x10, %[loop]\n"
>> @@ -308,6 +309,7 @@ asm volatile(
>>  	"       ldr	x9, [%[addr]]\n"
>>  	"       cmp     x10, #0x0\n"
>>  	"       b.gt    1b\n"
>> +	"       dsb     ish\n"
>>  	"       msr     pmcr_el0, xzr\n"
>>  	"       isb\n"
>>  	:
>> -- 
>> 2.38.1
>>
>>

Re: [kvm-unit-tests PATCH 3/6] arm: pmu: Add extra DSB barriers in the mem_access loop

[Alexandru Elisei]

Hi,

On Mon, Apr 24, 2023 at 10:11:11PM +0200, Eric Auger wrote:
> Hi Alexandru,
> 
> On 4/21/23 12:25, Alexandru Elisei wrote:
> > Hi,
> >
> > On Wed, Mar 15, 2023 at 12:07:22PM +0100, Eric Auger wrote:
> >> The mem access loop currently features ISB barriers only. However
> >> the mem_access loop counts the number of accesses to memory. ISB
> >> do not garantee the PE cannot reorder memory access. Let's
> >> add a DSB ISH before the write to PMCR_EL0 that enables the PMU
> >> and after the last iteration, before disabling the PMU.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >>
> >> ---
> >>
> >> This was discussed in https://lore.kernel.org/all/YzxmHpV2rpfaUdWi@monolith.localdoman/
> >> ---
> >>  arm/pmu.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/arm/pmu.c b/arm/pmu.c
> >> index b88366a8..dde399e2 100644
> >> --- a/arm/pmu.c
> >> +++ b/arm/pmu.c
> >> @@ -301,6 +301,7 @@ static void mem_access_loop(void *addr, long loop, uint32_t pmcr)
> >>  {
> >>  	uint64_t pmcr64 = pmcr;
> >>  asm volatile(
> >> +	"       dsb     ish\n"
> > I think it might still be possible to reorder memory accesses which are
> > part of the loop after the DSB above and before the PMU is enabled below.
> > But the DSB above is needed to make sure previous memory accesses, which
> > shouldn't be counted as part of the loop, are completed.
> >
> > I would put another DSB after the ISB which enables the PMU, that way all
> > memory accesses are neatly sandwitches between two DSBs.
> >
> > Having 3 DSBs might look like overdoing it, but I reason it to be correct.
> > What do you think?
> I need more time to investigate this. I will come back to you next week
> as I am OoO this week. Sorry for the inconvenience.

That's fine, I'm swamped too with other things, so don't expect a quick reply
:)

Thanks,
Alex

> Thank you for the review!
> 
> Eric
> >
> > Thanks,
> > Alex
> >
> >>  	"       msr     pmcr_el0, %[pmcr]\n"
> >>  	"       isb\n"
> >>  	"       mov     x10, %[loop]\n"
> >> @@ -308,6 +309,7 @@ asm volatile(
> >>  	"       ldr	x9, [%[addr]]\n"
> >>  	"       cmp     x10, #0x0\n"
> >>  	"       b.gt    1b\n"
> >> +	"       dsb     ish\n"
> >>  	"       msr     pmcr_el0, xzr\n"
> >>  	"       isb\n"
> >>  	:
> >> -- 
> >> 2.38.1
> >>
> >>
> 

Re: [kvm-unit-tests PATCH 3/6] arm: pmu: Add extra DSB barriers in the mem_access loop

[Eric Auger]

Hi Alexandru,
On 4/25/23 15:00, Alexandru Elisei wrote:
> Hi,
>
> On Mon, Apr 24, 2023 at 10:11:11PM +0200, Eric Auger wrote:
>> Hi Alexandru,
>>
>> On 4/21/23 12:25, Alexandru Elisei wrote:
>>> Hi,
>>>
>>> On Wed, Mar 15, 2023 at 12:07:22PM +0100, Eric Auger wrote:
>>>> The mem access loop currently features ISB barriers only. However
>>>> the mem_access loop counts the number of accesses to memory. ISB
>>>> do not garantee the PE cannot reorder memory access. Let's
>>>> add a DSB ISH before the write to PMCR_EL0 that enables the PMU
>>>> and after the last iteration, before disabling the PMU.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> Suggested-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>>>
>>>> ---
>>>>
>>>> This was discussed in https://lore.kernel.org/all/YzxmHpV2rpfaUdWi@monolith.localdoman/
>>>> ---
>>>>  arm/pmu.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arm/pmu.c b/arm/pmu.c
>>>> index b88366a8..dde399e2 100644
>>>> --- a/arm/pmu.c
>>>> +++ b/arm/pmu.c
>>>> @@ -301,6 +301,7 @@ static void mem_access_loop(void *addr, long loop, uint32_t pmcr)
>>>>  {
>>>>  	uint64_t pmcr64 = pmcr;
>>>>  asm volatile(
>>>> +	"       dsb     ish\n"
>>> I think it might still be possible to reorder memory accesses which are
>>> part of the loop after the DSB above and before the PMU is enabled below.
>>> But the DSB above is needed to make sure previous memory accesses, which
>>> shouldn't be counted as part of the loop, are completed.
>>>
>>> I would put another DSB after the ISB which enables the PMU, that way all
>>> memory accesses are neatly sandwitches between two DSBs.
>>>
>>> Having 3 DSBs might look like overdoing it, but I reason it to be correct.
>>> What do you think?
>> I need more time to investigate this. I will come back to you next week
>> as I am OoO this week. Sorry for the inconvenience.
> That's fine, I'm swamped too with other things, so don't expect a quick reply
> :)

My apologies for following up so late. I took your suggestion into
account and added this new DSB.

Thanks

Eric
>
> Thanks,
> Alex
>
>> Thank you for the review!
>>
>> Eric
>>> Thanks,
>>> Alex
>>>
>>>>  	"       msr     pmcr_el0, %[pmcr]\n"
>>>>  	"       isb\n"
>>>>  	"       mov     x10, %[loop]\n"
>>>> @@ -308,6 +309,7 @@ asm volatile(
>>>>  	"       ldr	x9, [%[addr]]\n"
>>>>  	"       cmp     x10, #0x0\n"
>>>>  	"       b.gt    1b\n"
>>>> +	"       dsb     ish\n"
>>>>  	"       msr     pmcr_el0, xzr\n"
>>>>  	"       isb\n"
>>>>  	:
>>>> -- 
>>>> 2.38.1
>>>>
>>>>

Re: [kvm-unit-tests PATCH 4/6] arm: pmu: Fix chain counter enable/disable sequences

[Eric Auger]

Hi Alexandru,

On 4/21/23 12:52, Alexandru Elisei wrote:
> Hi,
>
> On Wed, Mar 15, 2023 at 12:07:23PM +0100, Eric Auger wrote:
>> In some ARM ARM ddi0487 revisions it is said that
>> disabling/enabling a pair of counters that are paired
>> by a CHAIN event should follow a given sequence:
>>
>> Disable the low counter first, isb, disable the high counter
>> Enable the high counter first, isb, enable low counter
>>
>> This was the case in Fc. However this is not written anymore
>> in Ia revision.
>>
>> Introduce 2 helpers to execute those sequences and replace
>> the existing PMCNTENCLR/ENSET calls.
>>
>> Also fix 2 write_sysreg_s(0x0, PMCNTENSET_EL0) in subtest 5 & 6
>> and replace them by PMCNTENCLR writes since writing 0 in
>> PMCNTENSET_EL0 has no effect.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  arm/pmu.c | 37 ++++++++++++++++++++++++++++---------
>>  1 file changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index dde399e2..af679667 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -730,6 +730,22 @@ static void test_chained_sw_incr(bool unused)
>>  		    read_regn_el0(pmevcntr, 0), \
>>  		    read_sysreg(pmovsclr_el0))
>>  
>> +static void enable_chain_counter(int even)
>> +{
>> +	write_sysreg_s(BIT(even), PMCNTENSET_EL0); /* Enable the high counter first */
>> +	isb();
>> +	write_sysreg_s(BIT(even + 1), PMCNTENSET_EL0); /* Enable the low counter */
>> +	isb();
>> +}
> In ARM DDI 0487F.b, at the bottom of page D7-2727:
>
> "When enabling a pair of counters that are paired by a CHAIN event,
> software must:
>
> 1. Enable the high counter, by setting PMCNTENCLR_EL0[n+1] to 0 and, if
> necessary, setting PMCR_EL0.E to 1.
> 2. Execute an ISB instruction, or perform another Context synchronization
> event.
> 3. Enable the low counter by setting PMCNTENCLR_EL0[n] to 0."
>
> Which matches the commit message, but not the code above. Am I
> misunderstanding what is the high and low counter? In the example from the
> Arm ARM, just before the snippet above, the odd numbered countered is
> called the high counter.
>
> CHAIN is also defined as:
>
> [..] the odd-numbered event counter n+1 increments when an event increments
> the preceding even-numbered counter n on the same PE and causes an unsigned
> overflow of bits [31:0] of event counter n.
>
> So it would make sense to enable the odd counter first, then the even, so
> no overflows are missed if the sequence was the other way around (even
> counter enabled; overflow missed because odd counter disabled; odd counter
> enabled).
>
> Same observation with disable_chain_counter().

yeah you're right, I mixed up. Comments were right but does not match
the code.
I corrected this.

As for Marc's comment that this is not documented anymore that's correct
and was mentionned in the commit msg. Introducing those helpers make the
code a little bit simpler and I guess that executing those 'arbitrary'
sequences cannot do any harm so I kept them.

>
>> +
>> +static void disable_chain_counter(int even)
>> +{
>> +	write_sysreg_s(BIT(even + 1), PMCNTENCLR_EL0); /* Disable the low counter first*/
>> +	isb();
>> +	write_sysreg_s(BIT(even), PMCNTENCLR_EL0); /* Disable the high counter */
>> +	isb();
>> +}
>> +
>>  static void test_chain_promotion(bool unused)
>>  {
>>  	uint32_t events[] = {MEM_ACCESS, CHAIN};
>> @@ -768,16 +784,17 @@ static void test_chain_promotion(bool unused)
>>  	/* 1st COUNT with CHAIN enabled, next COUNT with CHAIN disabled */
>>  	report_prefix_push("subtest3");
>>  	pmu_reset();
>> -	write_sysreg_s(0x3, PMCNTENSET_EL0);
>>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
>> -	isb();
>> +	enable_chain_counter(0);
>>  	PRINT_REGS("init");
>>  
>>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("After 1st loop");
>>  
>>  	/* disable the CHAIN event */
>> -	write_sysreg_s(0x2, PMCNTENCLR_EL0);
>> +	disable_chain_counter(0);
>> +	write_sysreg_s(0x1, PMCNTENSET_EL0); /* Enable the low counter */
>> +	isb();
>>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("After 2d loop");
>>  	report(read_sysreg(pmovsclr_el0) == 0x1,
>> @@ -798,9 +815,11 @@ static void test_chain_promotion(bool unused)
>>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("After 1st loop");
>>  
>> -	/* enable the CHAIN event */
>> -	write_sysreg_s(0x3, PMCNTENSET_EL0);
>> +	/* Disable the even counter and enable the chain counter */
>> +	write_sysreg_s(0x1, PMCNTENCLR_EL0); /* Disable the low counter first */
> The comment says disable the even counter, but the odd counter is disabled.
> Which Arm ARM refers to as the high counter. I'm properly confused about
> the naming.

fixed by using the 'low' terminology

Thanks

Eric
>
> Thanks,
> Alex
>
>>  	isb();
>> +	enable_chain_counter(0);
>> +
>>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  
>>  	PRINT_REGS("After 2d loop");
>> @@ -824,10 +843,10 @@ static void test_chain_promotion(bool unused)
>>  	PRINT_REGS("After 1st loop");
>>  
>>  	/* 0 becomes CHAINED */
>> -	write_sysreg_s(0x0, PMCNTENSET_EL0);
>> +	write_sysreg_s(0x3, PMCNTENCLR_EL0);
>>  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
>> -	write_sysreg_s(0x3, PMCNTENSET_EL0);
>>  	write_regn_el0(pmevcntr, 1, 0x0);
>> +	enable_chain_counter(0);
>>  
>>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("After 2d loop");
>> @@ -843,13 +862,13 @@ static void test_chain_promotion(bool unused)
>>  	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
>>  	write_regn_el0(pmevtyper, 1, CHAIN | PMEVTYPER_EXCLUDE_EL0);
>>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW2_32);
>> -	write_sysreg_s(0x3, PMCNTENSET_EL0);
>> +	enable_chain_counter(0);
>>  	PRINT_REGS("init");
>>  
>>  	mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E);
>>  	PRINT_REGS("After 1st loop");
>>  
>> -	write_sysreg_s(0x0, PMCNTENSET_EL0);
>> +	disable_chain_counter(0);
>>  	write_regn_el0(pmevtyper, 1, CPU_CYCLES | PMEVTYPER_EXCLUDE_EL0);
>>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>>  
>> -- 
>> 2.38.1
>>
>>

Re: [kvm-unit-tests PATCH 5/6] arm: pmu: Add pmu-memaccess-reliability test

[Eric Auger]

Hi Alexandru,

On 4/21/23 13:13, Alexandru Elisei wrote:
> Hi,
>
> On Wed, Mar 15, 2023 at 12:07:24PM +0100, Eric Auger wrote:
>> Add a new basic test that runs MEM_ACCESS loop over
>> 100 iterations and make sure the number of measured
>> MEM_ACCESS never overflows the margin. Some other
>> pmu tests rely on this pattern and if the MEM_ACCESS
>> measurement is not reliable, it is better to report
>> it beforehand and not confuse the user any further.
>>
>> Without the subsequent patch, this typically fails on
>> ThunderXv2 with the following logs:
>>
>> INFO: pmu: pmu-memaccess-reliability: 32-bit overflows:
>> overflow=1 min=21 max=41 COUNT=20 MARGIN=15
>> FAIL: pmu: pmu-memaccess-reliability: 32-bit overflows:
>> memaccess is reliable
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  arm/pmu.c         | 52 +++++++++++++++++++++++++++++++++++++++++++++++
>>  arm/unittests.cfg |  6 ++++++
>>  2 files changed, 58 insertions(+)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index af679667..c3d2a428 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -56,6 +56,7 @@
>>  #define EXT_COMMON_EVENTS_HIGH	0x403F
>>  
>>  #define ALL_SET_32		0x00000000FFFFFFFFULL
>> +#define ALL_SET_64		0xFFFFFFFFFFFFFFFFULL
>>  #define ALL_CLEAR		0x0000000000000000ULL
>>  #define PRE_OVERFLOW_32		0x00000000FFFFFFF0ULL
>>  #define PRE_OVERFLOW_64		0xFFFFFFFFFFFFFFF0ULL
>> @@ -67,6 +68,10 @@
>>   * for some observed variability we take into account a given @MARGIN
>>   */
>>  #define PRE_OVERFLOW2_32		(ALL_SET_32 - COUNT - MARGIN)
>> +#define PRE_OVERFLOW2_64		(ALL_SET_64 - COUNT - MARGIN)
>> +
>> +#define PRE_OVERFLOW2(__overflow_at_64bits)				\
>> +	(__overflow_at_64bits ? PRE_OVERFLOW2_64 : PRE_OVERFLOW2_32)
>>  
>>  #define PRE_OVERFLOW(__overflow_at_64bits)				\
>>  	(__overflow_at_64bits ? PRE_OVERFLOW_64 : PRE_OVERFLOW_32)
>> @@ -746,6 +751,50 @@ static void disable_chain_counter(int even)
>>  	isb();
>>  }
>>  
>> +static void test_memaccess_reliability(bool overflow_at_64bits)
>> +{
>> +	uint32_t events[] = {MEM_ACCESS};
>> +	void *addr = malloc(PAGE_SIZE);
>> +	uint64_t count, max = 0, min = pmevcntr_mask();
>> +	uint64_t pre_overflow2 = PRE_OVERFLOW2(overflow_at_64bits);
>> +	uint64_t pmcr_lp = overflow_at_64bits ? PMU_PMCR_LP : 0;
>> +	bool overflow = false;
>> +
>> +	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)) ||
>> +	    !check_overflow_prerequisites(overflow_at_64bits))
>> +		return;
>> +
>> +	pmu_reset();
>> +	write_regn_el0(pmevtyper, 0, MEM_ACCESS | PMEVTYPER_EXCLUDE_EL0);
>> +	for (int i = 0; i < 100; i++) {
>> +		pmu_reset();
>> +		write_regn_el0(pmevcntr, 0, pre_overflow2);
>> +		write_sysreg_s(0x1, PMCNTENSET_EL0);
>> +		isb();
>> +		mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
>> +		count = read_regn_el0(pmevcntr, 0);
>> +		if (count < pre_overflow2) {
>> +			count += COUNT + MARGIN;
>> +			if (count > max)
>> +				max = count;
>> +			if (count < min)
>> +				min = count;
>> +			overflow = true;
>> +			report_info("iter=%d count=%ld min=%ld max=%ld overflow!!!",
>> +				    i, count, min, max);
>> +			continue;
>> +		}
>> +		count -= pre_overflow2;
>> +		if (count > max)
>> +			max = count;
>> +		if (count < min)
>> +			min = count;
> I'm having difficulties following the above maze of conditions. That's not going
> to be easy to maintain.
>
> If I understand the commit message correctly, the point of this test is to check
> that PRE_OVERFLOW2 + COUNT doesn't overflow, but PRE_OVERFLOW2 + 2 * COUNT does.
> How about this simpler approach instead:
>
> 	for (int i = 0; i < 100; i++) {
> 		pmu_reset();
> 		write_regn_el0(pmevcntr, 0, pre_overflow2);
> 		write_sysreg_s(0x1, PMCNTENSET_EL0);
> 		isb();
>
> 		mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> 		count = read_regn_el0(pmevcntr, 0);
> 		/* Counter overflowed when it shouldn't. */
> 		if (count < pre_overflow2) {
> 			report_fail("reliable memaccess loop");
> 			return;
> 		}
>
> 		mem_access_loop(addr, COUNT, pmu.pmcr_ro | PMU_PMCR_E | pmcr_lp);
> 		count = read_regn_el0(pmevcntr, 0);
> 		/* Counter didn't overflow when it should. */
> 		if (count >= pre_overflow2) {
> 			report_fail("reliable memaccess loop");
> 			return;
> 		}
> 	}
>
> 	report_success("reliable memaccess loop");
The test only checks that loop with PRE_OVERFLOW2 init value does not
overflow (in which case the mem access count is considered as 'reliable'
for subsequent tests using the same kind of loop). Besides doing that
check, the test also records the min/max mem access count values over
100 iterations to have an idea about how much the counting is [un]reliable.
I rearranged the logic and added comments. Hopefully this will be easier
to read.

Thank you for the review!

Eric
>
> Thanks,
> Alex
>
>>  static void test_chain_promotion(bool unused)
>>  {
>>  	uint32_t events[] = {MEM_ACCESS, CHAIN};
>> @@ -1203,6 +1252,9 @@ int main(int argc, char *argv[])
>>  	} else if (strcmp(argv[1], "pmu-basic-event-count") == 0) {
>>  		run_event_test(argv[1], test_basic_event_count, false);
>>  		run_event_test(argv[1], test_basic_event_count, true);
>> +	} else if (strcmp(argv[1], "pmu-memaccess-reliability") == 0) {
>> +		run_event_test(argv[1], test_memaccess_reliability, false);
>> +		run_event_test(argv[1], test_memaccess_reliability, true);
>>  	} else if (strcmp(argv[1], "pmu-mem-access") == 0) {
>>  		run_event_test(argv[1], test_mem_access, false);
>>  		run_event_test(argv[1], test_mem_access, true);
>> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
>> index 5e67b558..301261aa 100644
>> --- a/arm/unittests.cfg
>> +++ b/arm/unittests.cfg
>> @@ -90,6 +90,12 @@ groups = pmu
>>  arch = arm64
>>  extra_params = -append 'pmu-mem-access'
>>  
>> +[pmu-memaccess-reliability]
>> +file = pmu.flat
>> +groups = pmu
>> +arch = arm64
>> +extra_params = -append 'pmu-memaccess-reliability'
>> +
>>  [pmu-sw-incr]
>>  file = pmu.flat
>>  groups = pmu
>> -- 
>> 2.38.1
>>