All of lore.kernel.org
 help / color / mirror / Atom feed
* [kvm-unit-tests PATCH 0/3] arm: pmu: Fixes for bare metal
@ 2022-07-18 15:49 ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-18 15:49 UTC (permalink / raw)
  To: kvm, kvmarm, drjones
  Cc: maz, alexandru.elisei, eric.auger, oliver.upton, reijiw, Ricardo Koller

There are some tests that fail when running on bare metal (including a
passthrough prototype).  There are three issues with the tests.  The
first one is that there are some missing isb()'s between enabling event
counting and the actual counting. This wasn't an issue on KVM as
trapping on registers served as context synchronization events. The
second issue is that some tests assume that registers reset to 0.  And
finally, the third issue is that overflowing the low counter of a
chained event sets the overflow flag in PMVOS and some tests fail by
checking for it not being set.

I believe the third fix also requires a KVM change, but would like to
double check with others first.  The only reference I could find in the
ARM ARM is the AArch64.IncrementEventCounter() pseudocode (DDI 0487H.a,
J1.1.1 "aarch64/debug") that unconditionally sets the PMOVS bit on
overflow.

Ricardo Koller (3):
  arm: pmu: Add missing isb()'s after sys register writing
  arm: pmu: Reset the pmu registers before starting some tests
  arm: pmu: Remove checks for !overflow in chained counters tests

 arm/pmu.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

-- 
2.37.0.170.g444d1eabd0-goog


^ permalink raw reply	[flat|nested] 48+ messages in thread
* [kvm-unit-tests PATCH 0/3] arm: pmu: Fixes for bare metal
@ 2022-07-18 15:49 ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-18 15:49 UTC (permalink / raw)
  To: kvm, kvmarm, drjones; +Cc: maz, oliver.upton

There are some tests that fail when running on bare metal (including a
passthrough prototype).  There are three issues with the tests.  The
first one is that there are some missing isb()'s between enabling event
counting and the actual counting. This wasn't an issue on KVM as
trapping on registers served as context synchronization events. The
second issue is that some tests assume that registers reset to 0.  And
finally, the third issue is that overflowing the low counter of a
chained event sets the overflow flag in PMVOS and some tests fail by
checking for it not being set.

I believe the third fix also requires a KVM change, but would like to
double check with others first.  The only reference I could find in the
ARM ARM is the AArch64.IncrementEventCounter() pseudocode (DDI 0487H.a,
J1.1.1 "aarch64/debug") that unconditionally sets the PMOVS bit on
overflow.

Ricardo Koller (3):
  arm: pmu: Add missing isb()'s after sys register writing
  arm: pmu: Reset the pmu registers before starting some tests
  arm: pmu: Remove checks for !overflow in chained counters tests

 arm/pmu.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

-- 
2.37.0.170.g444d1eabd0-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing
  2022-07-18 15:49 ` Ricardo Koller
@ 2022-07-18 15:49   ` Ricardo Koller
  -1 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-18 15:49 UTC (permalink / raw)
  To: kvm, kvmarm, drjones
  Cc: maz, alexandru.elisei, eric.auger, oliver.upton, reijiw, Ricardo Koller

There are various pmu tests that require an isb() between enabling
counting and the actual counting. This can lead to count registers
reporting less events than expected; the actual enabling happens after
some events have happened.  For example, some missing isb()'s in the
pmu-sw-incr test lead to the following errors on bare-metal:

	INFO: pmu: pmu-sw-incr: SW_INCR counter #0 has value 4294967280
        PASS: pmu: pmu-sw-incr: PWSYNC does not increment if PMCR.E is unset
        FAIL: pmu: pmu-sw-incr: counter #1 after + 100 SW_INCR
        FAIL: pmu: pmu-sw-incr: counter #0 after + 100 SW_INCR
        INFO: pmu: pmu-sw-incr: counter values after 100 SW_INCR #0=82 #1=98
        PASS: pmu: pmu-sw-incr: overflow on counter #0 after 100 SW_INCR
        SUMMARY: 4 tests, 2 unexpected failures

Add the missing isb()'s on all failing tests, plus some others that are
not currently required but might in the future (like an isb() after
clearing the overflow signal in the IRQ handler).

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arm/pmu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index 15c542a2..fd838392 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -307,6 +307,7 @@ static void irq_handler(struct pt_regs *regs)
 			}
 		}
 		write_sysreg(ALL_SET, pmovsclr_el0);
+		isb();
 	} else {
 		pmu_stats.unexpected = true;
 	}
@@ -534,6 +535,7 @@ static void test_sw_incr(void)
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
+	isb();
 
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x1, pmswinc_el0);
@@ -547,6 +549,7 @@ static void test_sw_incr(void)
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	isb();
 
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x3, pmswinc_el0);
@@ -618,6 +621,8 @@ static void test_chained_sw_incr(void)
 
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
 	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	isb();
+
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x1, pmswinc_el0);
 
@@ -634,6 +639,8 @@ static void test_chained_sw_incr(void)
 	write_regn_el0(pmevcntr, 1, ALL_SET);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	isb();
+
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x1, pmswinc_el0);
 
@@ -821,6 +828,8 @@ static void test_overflow_interrupt(void)
 	report(expect_interrupts(0), "no overflow interrupt after preset");
 
 	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	isb();
+
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x2, pmswinc_el0);
 
@@ -879,6 +888,7 @@ static bool check_cycles_increase(void)
 	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
 
 	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
+	isb();
 
 	for (int i = 0; i < NR_SAMPLES; i++) {
 		uint64_t a, b;
@@ -894,6 +904,7 @@ static bool check_cycles_increase(void)
 	}
 
 	set_pmcr(get_pmcr() & ~PMU_PMCR_E);
+	isb();
 
 	return success;
 }
-- 
2.37.0.170.g444d1eabd0-goog


^ permalink raw reply related	[flat|nested] 48+ messages in thread
* [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing
@ 2022-07-18 15:49   ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-18 15:49 UTC (permalink / raw)
  To: kvm, kvmarm, drjones; +Cc: maz, oliver.upton

There are various pmu tests that require an isb() between enabling
counting and the actual counting. This can lead to count registers
reporting less events than expected; the actual enabling happens after
some events have happened.  For example, some missing isb()'s in the
pmu-sw-incr test lead to the following errors on bare-metal:

	INFO: pmu: pmu-sw-incr: SW_INCR counter #0 has value 4294967280
        PASS: pmu: pmu-sw-incr: PWSYNC does not increment if PMCR.E is unset
        FAIL: pmu: pmu-sw-incr: counter #1 after + 100 SW_INCR
        FAIL: pmu: pmu-sw-incr: counter #0 after + 100 SW_INCR
        INFO: pmu: pmu-sw-incr: counter values after 100 SW_INCR #0=82 #1=98
        PASS: pmu: pmu-sw-incr: overflow on counter #0 after 100 SW_INCR
        SUMMARY: 4 tests, 2 unexpected failures

Add the missing isb()'s on all failing tests, plus some others that are
not currently required but might in the future (like an isb() after
clearing the overflow signal in the IRQ handler).

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arm/pmu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index 15c542a2..fd838392 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -307,6 +307,7 @@ static void irq_handler(struct pt_regs *regs)
 			}
 		}
 		write_sysreg(ALL_SET, pmovsclr_el0);
+		isb();
 	} else {
 		pmu_stats.unexpected = true;
 	}
@@ -534,6 +535,7 @@ static void test_sw_incr(void)
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
+	isb();
 
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x1, pmswinc_el0);
@@ -547,6 +549,7 @@ static void test_sw_incr(void)
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	isb();
 
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x3, pmswinc_el0);
@@ -618,6 +621,8 @@ static void test_chained_sw_incr(void)
 
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
 	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	isb();
+
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x1, pmswinc_el0);
 
@@ -634,6 +639,8 @@ static void test_chained_sw_incr(void)
 	write_regn_el0(pmevcntr, 1, ALL_SET);
 	write_sysreg_s(0x3, PMCNTENSET_EL0);
 	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	isb();
+
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x1, pmswinc_el0);
 
@@ -821,6 +828,8 @@ static void test_overflow_interrupt(void)
 	report(expect_interrupts(0), "no overflow interrupt after preset");
 
 	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
+	isb();
+
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x2, pmswinc_el0);
 
@@ -879,6 +888,7 @@ static bool check_cycles_increase(void)
 	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
 
 	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
+	isb();
 
 	for (int i = 0; i < NR_SAMPLES; i++) {
 		uint64_t a, b;
@@ -894,6 +904,7 @@ static bool check_cycles_increase(void)
 	}
 
 	set_pmcr(get_pmcr() & ~PMU_PMCR_E);
+	isb();
 
 	return success;
 }
-- 
2.37.0.170.g444d1eabd0-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 48+ messages in thread
* [kvm-unit-tests PATCH 2/3] arm: pmu: Reset the pmu registers before starting some tests
  2022-07-18 15:49 ` Ricardo Koller
@ 2022-07-18 15:49   ` Ricardo Koller
  -1 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-18 15:49 UTC (permalink / raw)
  To: kvm, kvmarm, drjones
  Cc: maz, alexandru.elisei, eric.auger, oliver.upton, reijiw, Ricardo Koller

Some registers like the PMOVS reset to an architecturally UNKNOWN value.
Most tests expect them to be reset (mostly zeroed) using pmu_reset().
Add a pmu_reset() on all the tests that need one.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arm/pmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index fd838392..a7899c3c 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -884,6 +884,7 @@ static bool check_cycles_increase(void)
 	bool success = true;
 
 	/* init before event access, this test only cares about cycle count */
+	pmu_reset();
 	set_pmcntenset(1 << PMU_CYCLE_IDX);
 	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
 
@@ -938,6 +939,7 @@ static bool check_cpi(int cpi)
 	uint32_t pmcr = get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E;
 
 	/* init before event access, this test only cares about cycle count */
+	pmu_reset();
 	set_pmcntenset(1 << PMU_CYCLE_IDX);
 	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
 
-- 
2.37.0.170.g444d1eabd0-goog


^ permalink raw reply related	[flat|nested] 48+ messages in thread
* [kvm-unit-tests PATCH 2/3] arm: pmu: Reset the pmu registers before starting some tests
@ 2022-07-18 15:49   ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-18 15:49 UTC (permalink / raw)
  To: kvm, kvmarm, drjones; +Cc: maz, oliver.upton

Some registers like the PMOVS reset to an architecturally UNKNOWN value.
Most tests expect them to be reset (mostly zeroed) using pmu_reset().
Add a pmu_reset() on all the tests that need one.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arm/pmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index fd838392..a7899c3c 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -884,6 +884,7 @@ static bool check_cycles_increase(void)
 	bool success = true;
 
 	/* init before event access, this test only cares about cycle count */
+	pmu_reset();
 	set_pmcntenset(1 << PMU_CYCLE_IDX);
 	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
 
@@ -938,6 +939,7 @@ static bool check_cpi(int cpi)
 	uint32_t pmcr = get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E;
 
 	/* init before event access, this test only cares about cycle count */
+	pmu_reset();
 	set_pmcntenset(1 << PMU_CYCLE_IDX);
 	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
 
-- 
2.37.0.170.g444d1eabd0-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 48+ messages in thread
* [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-18 15:49 ` Ricardo Koller
@ 2022-07-18 15:49   ` Ricardo Koller
  -1 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-18 15:49 UTC (permalink / raw)
  To: kvm, kvmarm, drjones
  Cc: maz, alexandru.elisei, eric.auger, oliver.upton, reijiw, Ricardo Koller

A chained event overflowing on the low counter can set the overflow flag
in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
(DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
overflow.

The pmu chain tests fail on bare metal when checking the overflow flag
of the low counter _not_ being set on overflow.  Fix by removing the
checks.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arm/pmu.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index a7899c3c..4f2c5096 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -581,7 +581,6 @@ static void test_chained_counters(void)
 	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
 
 	report(read_regn_el0(pmevcntr, 1) == 1, "CHAIN counter #1 incremented");
-	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #1");
 
 	/* test 64b overflow */
 
@@ -593,7 +592,7 @@ static void test_chained_counters(void)
 	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
 	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
 	report(read_regn_el0(pmevcntr, 1) == 2, "CHAIN counter #1 set to 2");
-	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #2");
+	report((read_sysreg(pmovsclr_el0) & 0x2) == 0, "no overflow recorded for chained incr #2");
 
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
 	write_regn_el0(pmevcntr, 1, ALL_SET);
@@ -601,7 +600,7 @@ static void test_chained_counters(void)
 	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
 	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
 	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
-	report(read_sysreg(pmovsclr_el0) == 0x2, "overflow on chain counter");
+	report(read_sysreg(pmovsclr_el0) & 0x2, "overflow on chain counter");
 }
 
 static void test_chained_sw_incr(void)
@@ -626,10 +625,10 @@ static void test_chained_sw_incr(void)
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x1, pmswinc_el0);
 
-	report(!read_sysreg(pmovsclr_el0) && (read_regn_el0(pmevcntr, 1) == 1),
-		"no overflow and chain counter incremented after 100 SW_INCR/CHAIN");
+	report(read_regn_el0(pmevcntr, 1) == 1,
+			"no chain counter incremented after 100 SW_INCR/CHAIN");
 	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
-		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
+			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
 
 	/* 64b SW_INCR and overflow on CHAIN counter*/
 	pmu_reset();
@@ -644,7 +643,7 @@ static void test_chained_sw_incr(void)
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x1, pmswinc_el0);
 
-	report((read_sysreg(pmovsclr_el0) == 0x2) &&
+	report((read_sysreg(pmovsclr_el0) & 0x2) &&
 		(read_regn_el0(pmevcntr, 1) == 0) &&
 		(read_regn_el0(pmevcntr, 0) == 84),
 		"overflow on chain counter and expected values after 100 SW_INCR/CHAIN");
@@ -727,8 +726,8 @@ static void test_chain_promotion(void)
 	report_info("MEM_ACCESS counter #0 has value 0x%lx",
 		    read_regn_el0(pmevcntr, 0));
 
-	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
-		"CHAIN counter enabled: CHAIN counter was incremented and no overflow");
+	report((read_regn_el0(pmevcntr, 1) == 1),
+		"CHAIN counter enabled: CHAIN counter was incremented");
 
 	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
 		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
@@ -755,8 +754,8 @@ static void test_chain_promotion(void)
 	report_info("MEM_ACCESS counter #0 has value 0x%lx",
 		    read_regn_el0(pmevcntr, 0));
 
-	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
-		"32b->64b: CHAIN counter incremented and no overflow");
+	report((read_regn_el0(pmevcntr, 1) == 1),
+		"32b->64b: CHAIN counter incremented");
 
 	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
 		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
-- 
2.37.0.170.g444d1eabd0-goog


^ permalink raw reply related	[flat|nested] 48+ messages in thread
* [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-18 15:49   ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-18 15:49 UTC (permalink / raw)
  To: kvm, kvmarm, drjones; +Cc: maz, oliver.upton

A chained event overflowing on the low counter can set the overflow flag
in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
(DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
overflow.

The pmu chain tests fail on bare metal when checking the overflow flag
of the low counter _not_ being set on overflow.  Fix by removing the
checks.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arm/pmu.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index a7899c3c..4f2c5096 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -581,7 +581,6 @@ static void test_chained_counters(void)
 	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
 
 	report(read_regn_el0(pmevcntr, 1) == 1, "CHAIN counter #1 incremented");
-	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #1");
 
 	/* test 64b overflow */
 
@@ -593,7 +592,7 @@ static void test_chained_counters(void)
 	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
 	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
 	report(read_regn_el0(pmevcntr, 1) == 2, "CHAIN counter #1 set to 2");
-	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #2");
+	report((read_sysreg(pmovsclr_el0) & 0x2) == 0, "no overflow recorded for chained incr #2");
 
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
 	write_regn_el0(pmevcntr, 1, ALL_SET);
@@ -601,7 +600,7 @@ static void test_chained_counters(void)
 	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
 	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
 	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
-	report(read_sysreg(pmovsclr_el0) == 0x2, "overflow on chain counter");
+	report(read_sysreg(pmovsclr_el0) & 0x2, "overflow on chain counter");
 }
 
 static void test_chained_sw_incr(void)
@@ -626,10 +625,10 @@ static void test_chained_sw_incr(void)
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x1, pmswinc_el0);
 
-	report(!read_sysreg(pmovsclr_el0) && (read_regn_el0(pmevcntr, 1) == 1),
-		"no overflow and chain counter incremented after 100 SW_INCR/CHAIN");
+	report(read_regn_el0(pmevcntr, 1) == 1,
+			"no chain counter incremented after 100 SW_INCR/CHAIN");
 	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
-		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
+			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
 
 	/* 64b SW_INCR and overflow on CHAIN counter*/
 	pmu_reset();
@@ -644,7 +643,7 @@ static void test_chained_sw_incr(void)
 	for (i = 0; i < 100; i++)
 		write_sysreg(0x1, pmswinc_el0);
 
-	report((read_sysreg(pmovsclr_el0) == 0x2) &&
+	report((read_sysreg(pmovsclr_el0) & 0x2) &&
 		(read_regn_el0(pmevcntr, 1) == 0) &&
 		(read_regn_el0(pmevcntr, 0) == 84),
 		"overflow on chain counter and expected values after 100 SW_INCR/CHAIN");
@@ -727,8 +726,8 @@ static void test_chain_promotion(void)
 	report_info("MEM_ACCESS counter #0 has value 0x%lx",
 		    read_regn_el0(pmevcntr, 0));
 
-	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
-		"CHAIN counter enabled: CHAIN counter was incremented and no overflow");
+	report((read_regn_el0(pmevcntr, 1) == 1),
+		"CHAIN counter enabled: CHAIN counter was incremented");
 
 	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
 		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
@@ -755,8 +754,8 @@ static void test_chain_promotion(void)
 	report_info("MEM_ACCESS counter #0 has value 0x%lx",
 		    read_regn_el0(pmevcntr, 0));
 
-	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
-		"32b->64b: CHAIN counter incremented and no overflow");
+	report((read_regn_el0(pmevcntr, 1) == 1),
+		"32b->64b: CHAIN counter incremented");
 
 	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
 		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
-- 
2.37.0.170.g444d1eabd0-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing
  2022-07-18 15:49   ` Ricardo Koller
@ 2022-07-18 16:38     ` Alexandru Elisei
  -1 siblings, 0 replies; 48+ messages in thread
From: Alexandru Elisei @ 2022-07-18 16:38 UTC (permalink / raw)
  To: Ricardo Koller
  Cc: kvm, kvmarm, drjones, maz, eric.auger, oliver.upton, reijiw

Hi,

On Mon, Jul 18, 2022 at 08:49:08AM -0700, Ricardo Koller wrote:
> There are various pmu tests that require an isb() between enabling
> counting and the actual counting. This can lead to count registers
> reporting less events than expected; the actual enabling happens after
> some events have happened.  For example, some missing isb()'s in the
> pmu-sw-incr test lead to the following errors on bare-metal:
> 
> 	INFO: pmu: pmu-sw-incr: SW_INCR counter #0 has value 4294967280
>         PASS: pmu: pmu-sw-incr: PWSYNC does not increment if PMCR.E is unset
>         FAIL: pmu: pmu-sw-incr: counter #1 after + 100 SW_INCR
>         FAIL: pmu: pmu-sw-incr: counter #0 after + 100 SW_INCR
>         INFO: pmu: pmu-sw-incr: counter values after 100 SW_INCR #0=82 #1=98
>         PASS: pmu: pmu-sw-incr: overflow on counter #0 after 100 SW_INCR
>         SUMMARY: 4 tests, 2 unexpected failures
> 
> Add the missing isb()'s on all failing tests, plus some others that are
> not currently required but might in the future (like an isb() after
> clearing the overflow signal in the IRQ handler).

That's rather cryptic. What might require those hypothetical ISBs and why? Why
should a test add code for some hypothetical requirement that might, or might
not, be implemented?

This is pure speculation on my part, were you seeing spurious interrupts that
went away after adding the ISB in irq_handler()?

A couple of comments below.

> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arm/pmu.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 15c542a2..fd838392 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -307,6 +307,7 @@ static void irq_handler(struct pt_regs *regs)
>  			}
>  		}
>  		write_sysreg(ALL_SET, pmovsclr_el0);
> +		isb();
>  	} else {
>  		pmu_stats.unexpected = true;
>  	}
> @@ -534,6 +535,7 @@ static void test_sw_incr(void)
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> +	isb();
>  
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);

Since your patch adds needed synchronization, from ARM DDI 0487H.a, page
D13-5237:

"Where a direct write to one register causes a bit or field in a different
register [..] to be updated as a side-effect of that direct write [..], the
change to the different register [..] is defined to be an indirect write. In
this case, the indirect write is only guaranteed to be visible to subsequent
direct or indirect reads or writes if synchronization is performed after the
direct write and before the subsequent direct or indirect reads or writes."

I think that says that you need an ISB after the direct writes to PMSWINC_EL0
for software to read the correct value for PMEVNCTR0_EL0.

> @@ -547,6 +549,7 @@ static void test_sw_incr(void)
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
>  
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x3, pmswinc_el0);

Same as above, might be worth checking in other places.

Will come back with more review comments.

Thanks,
Alex

> @@ -618,6 +621,8 @@ static void test_chained_sw_incr(void)
>  
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
> +
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
>  
> @@ -634,6 +639,8 @@ static void test_chained_sw_incr(void)
>  	write_regn_el0(pmevcntr, 1, ALL_SET);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
> +
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
>  
> @@ -821,6 +828,8 @@ static void test_overflow_interrupt(void)
>  	report(expect_interrupts(0), "no overflow interrupt after preset");
>  
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
> +
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x2, pmswinc_el0);
>  
> @@ -879,6 +888,7 @@ static bool check_cycles_increase(void)
>  	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
>  
>  	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
> +	isb();
>  
>  	for (int i = 0; i < NR_SAMPLES; i++) {
>  		uint64_t a, b;
> @@ -894,6 +904,7 @@ static bool check_cycles_increase(void)
>  	}
>  
>  	set_pmcr(get_pmcr() & ~PMU_PMCR_E);
> +	isb();
>  
>  	return success;
>  }
> -- 
> 2.37.0.170.g444d1eabd0-goog
> 

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing
@ 2022-07-18 16:38     ` Alexandru Elisei
  0 siblings, 0 replies; 48+ messages in thread
From: Alexandru Elisei @ 2022-07-18 16:38 UTC (permalink / raw)
  To: Ricardo Koller; +Cc: drjones, kvm, maz, oliver.upton, kvmarm

Hi,

On Mon, Jul 18, 2022 at 08:49:08AM -0700, Ricardo Koller wrote:
> There are various pmu tests that require an isb() between enabling
> counting and the actual counting. This can lead to count registers
> reporting less events than expected; the actual enabling happens after
> some events have happened.  For example, some missing isb()'s in the
> pmu-sw-incr test lead to the following errors on bare-metal:
> 
> 	INFO: pmu: pmu-sw-incr: SW_INCR counter #0 has value 4294967280
>         PASS: pmu: pmu-sw-incr: PWSYNC does not increment if PMCR.E is unset
>         FAIL: pmu: pmu-sw-incr: counter #1 after + 100 SW_INCR
>         FAIL: pmu: pmu-sw-incr: counter #0 after + 100 SW_INCR
>         INFO: pmu: pmu-sw-incr: counter values after 100 SW_INCR #0=82 #1=98
>         PASS: pmu: pmu-sw-incr: overflow on counter #0 after 100 SW_INCR
>         SUMMARY: 4 tests, 2 unexpected failures
> 
> Add the missing isb()'s on all failing tests, plus some others that are
> not currently required but might in the future (like an isb() after
> clearing the overflow signal in the IRQ handler).

That's rather cryptic. What might require those hypothetical ISBs and why? Why
should a test add code for some hypothetical requirement that might, or might
not, be implemented?

This is pure speculation on my part, were you seeing spurious interrupts that
went away after adding the ISB in irq_handler()?

A couple of comments below.

> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arm/pmu.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 15c542a2..fd838392 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -307,6 +307,7 @@ static void irq_handler(struct pt_regs *regs)
>  			}
>  		}
>  		write_sysreg(ALL_SET, pmovsclr_el0);
> +		isb();
>  	} else {
>  		pmu_stats.unexpected = true;
>  	}
> @@ -534,6 +535,7 @@ static void test_sw_incr(void)
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> +	isb();
>  
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);

Since your patch adds needed synchronization, from ARM DDI 0487H.a, page
D13-5237:

"Where a direct write to one register causes a bit or field in a different
register [..] to be updated as a side-effect of that direct write [..], the
change to the different register [..] is defined to be an indirect write. In
this case, the indirect write is only guaranteed to be visible to subsequent
direct or indirect reads or writes if synchronization is performed after the
direct write and before the subsequent direct or indirect reads or writes."

I think that says that you need an ISB after the direct writes to PMSWINC_EL0
for software to read the correct value for PMEVNCTR0_EL0.

> @@ -547,6 +549,7 @@ static void test_sw_incr(void)
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
>  
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x3, pmswinc_el0);

Same as above, might be worth checking in other places.

Will come back with more review comments.

Thanks,
Alex

> @@ -618,6 +621,8 @@ static void test_chained_sw_incr(void)
>  
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
> +
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
>  
> @@ -634,6 +639,8 @@ static void test_chained_sw_incr(void)
>  	write_regn_el0(pmevcntr, 1, ALL_SET);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
> +
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
>  
> @@ -821,6 +828,8 @@ static void test_overflow_interrupt(void)
>  	report(expect_interrupts(0), "no overflow interrupt after preset");
>  
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
> +
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x2, pmswinc_el0);
>  
> @@ -879,6 +888,7 @@ static bool check_cycles_increase(void)
>  	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
>  
>  	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
> +	isb();
>  
>  	for (int i = 0; i < NR_SAMPLES; i++) {
>  		uint64_t a, b;
> @@ -894,6 +904,7 @@ static bool check_cycles_increase(void)
>  	}
>  
>  	set_pmcr(get_pmcr() & ~PMU_PMCR_E);
> +	isb();
>  
>  	return success;
>  }
> -- 
> 2.37.0.170.g444d1eabd0-goog
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 0/3] arm: pmu: Fixes for bare metal
  2022-07-18 15:49 ` Ricardo Koller
@ 2022-07-18 16:42   ` Alexandru Elisei
  -1 siblings, 0 replies; 48+ messages in thread
From: Alexandru Elisei @ 2022-07-18 16:42 UTC (permalink / raw)
  To: Ricardo Koller
  Cc: kvm, kvmarm, maz, eric.auger, oliver.upton, reijiw, andrew.jones

Hi,

I believe you're missing the updated email for the arm maintainer. Added it.

Thanks,
Alex

On Mon, Jul 18, 2022 at 08:49:07AM -0700, Ricardo Koller wrote:
> There are some tests that fail when running on bare metal (including a
> passthrough prototype).  There are three issues with the tests.  The
> first one is that there are some missing isb()'s between enabling event
> counting and the actual counting. This wasn't an issue on KVM as
> trapping on registers served as context synchronization events. The
> second issue is that some tests assume that registers reset to 0.  And
> finally, the third issue is that overflowing the low counter of a
> chained event sets the overflow flag in PMVOS and some tests fail by
> checking for it not being set.
> 
> I believe the third fix also requires a KVM change, but would like to
> double check with others first.  The only reference I could find in the
> ARM ARM is the AArch64.IncrementEventCounter() pseudocode (DDI 0487H.a,
> J1.1.1 "aarch64/debug") that unconditionally sets the PMOVS bit on
> overflow.
> 
> Ricardo Koller (3):
>   arm: pmu: Add missing isb()'s after sys register writing
>   arm: pmu: Reset the pmu registers before starting some tests
>   arm: pmu: Remove checks for !overflow in chained counters tests
> 
>  arm/pmu.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> -- 
> 2.37.0.170.g444d1eabd0-goog
> 

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 0/3] arm: pmu: Fixes for bare metal
@ 2022-07-18 16:42   ` Alexandru Elisei
  0 siblings, 0 replies; 48+ messages in thread
From: Alexandru Elisei @ 2022-07-18 16:42 UTC (permalink / raw)
  To: Ricardo Koller; +Cc: kvm, maz, oliver.upton, andrew.jones, kvmarm

Hi,

I believe you're missing the updated email for the arm maintainer. Added it.

Thanks,
Alex

On Mon, Jul 18, 2022 at 08:49:07AM -0700, Ricardo Koller wrote:
> There are some tests that fail when running on bare metal (including a
> passthrough prototype).  There are three issues with the tests.  The
> first one is that there are some missing isb()'s between enabling event
> counting and the actual counting. This wasn't an issue on KVM as
> trapping on registers served as context synchronization events. The
> second issue is that some tests assume that registers reset to 0.  And
> finally, the third issue is that overflowing the low counter of a
> chained event sets the overflow flag in PMVOS and some tests fail by
> checking for it not being set.
> 
> I believe the third fix also requires a KVM change, but would like to
> double check with others first.  The only reference I could find in the
> ARM ARM is the AArch64.IncrementEventCounter() pseudocode (DDI 0487H.a,
> J1.1.1 "aarch64/debug") that unconditionally sets the PMOVS bit on
> overflow.
> 
> Ricardo Koller (3):
>   arm: pmu: Add missing isb()'s after sys register writing
>   arm: pmu: Reset the pmu registers before starting some tests
>   arm: pmu: Remove checks for !overflow in chained counters tests
> 
>  arm/pmu.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
> 
> -- 
> 2.37.0.170.g444d1eabd0-goog
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 0/3] arm: pmu: Fixes for bare metal
  2022-07-18 16:42   ` Alexandru Elisei
@ 2022-07-18 17:18     ` Ricardo Koller
  -1 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-18 17:18 UTC (permalink / raw)
  To: Alexandru Elisei
  Cc: kvm, kvmarm, maz, eric.auger, oliver.upton, reijiw, andrew.jones

On Mon, Jul 18, 2022 at 05:42:08PM +0100, Alexandru Elisei wrote:
> Hi,
> 
> I believe you're missing the updated email for the arm maintainer. Added it.
> 
> Thanks,
> Alex
> 
> On Mon, Jul 18, 2022 at 08:49:07AM -0700, Ricardo Koller wrote:
> > There are some tests that fail when running on bare metal (including a
> > passthrough prototype).  There are three issues with the tests.  The
> > first one is that there are some missing isb()'s between enabling event
> > counting and the actual counting. This wasn't an issue on KVM as
> > trapping on registers served as context synchronization events. The
> > second issue is that some tests assume that registers reset to 0.  And
> > finally, the third issue is that overflowing the low counter of a
> > chained event sets the overflow flag in PMVOS and some tests fail by
> > checking for it not being set.
> > 
> > I believe the third fix also requires a KVM change, but would like to
> > double check with others first.  The only reference I could find in the
> > ARM ARM is the AArch64.IncrementEventCounter() pseudocode (DDI 0487H.a,
> > J1.1.1 "aarch64/debug") that unconditionally sets the PMOVS bit on
> > overflow.
> > 
> > Ricardo Koller (3):
> >   arm: pmu: Add missing isb()'s after sys register writing
> >   arm: pmu: Reset the pmu registers before starting some tests
> >   arm: pmu: Remove checks for !overflow in chained counters tests
> > 
> >  arm/pmu.c | 34 +++++++++++++++++++++++-----------
> >  1 file changed, 23 insertions(+), 11 deletions(-)
> > 
> > -- 
> > 2.37.0.170.g444d1eabd0-goog
> > 

Right, I forgot about the new email. Thanks for the forwarding!

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 0/3] arm: pmu: Fixes for bare metal
@ 2022-07-18 17:18     ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-18 17:18 UTC (permalink / raw)
  To: Alexandru Elisei; +Cc: kvm, maz, oliver.upton, andrew.jones, kvmarm

On Mon, Jul 18, 2022 at 05:42:08PM +0100, Alexandru Elisei wrote:
> Hi,
> 
> I believe you're missing the updated email for the arm maintainer. Added it.
> 
> Thanks,
> Alex
> 
> On Mon, Jul 18, 2022 at 08:49:07AM -0700, Ricardo Koller wrote:
> > There are some tests that fail when running on bare metal (including a
> > passthrough prototype).  There are three issues with the tests.  The
> > first one is that there are some missing isb()'s between enabling event
> > counting and the actual counting. This wasn't an issue on KVM as
> > trapping on registers served as context synchronization events. The
> > second issue is that some tests assume that registers reset to 0.  And
> > finally, the third issue is that overflowing the low counter of a
> > chained event sets the overflow flag in PMVOS and some tests fail by
> > checking for it not being set.
> > 
> > I believe the third fix also requires a KVM change, but would like to
> > double check with others first.  The only reference I could find in the
> > ARM ARM is the AArch64.IncrementEventCounter() pseudocode (DDI 0487H.a,
> > J1.1.1 "aarch64/debug") that unconditionally sets the PMOVS bit on
> > overflow.
> > 
> > Ricardo Koller (3):
> >   arm: pmu: Add missing isb()'s after sys register writing
> >   arm: pmu: Reset the pmu registers before starting some tests
> >   arm: pmu: Remove checks for !overflow in chained counters tests
> > 
> >  arm/pmu.c | 34 +++++++++++++++++++++++-----------
> >  1 file changed, 23 insertions(+), 11 deletions(-)
> > 
> > -- 
> > 2.37.0.170.g444d1eabd0-goog
> > 

Right, I forgot about the new email. Thanks for the forwarding!
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing
  2022-07-18 16:38     ` Alexandru Elisei
@ 2022-07-18 17:48       ` Ricardo Koller
  -1 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-18 17:48 UTC (permalink / raw)
  To: Alexandru Elisei
  Cc: kvm, kvmarm, drjones, maz, eric.auger, oliver.upton, reijiw

On Mon, Jul 18, 2022 at 05:38:23PM +0100, Alexandru Elisei wrote:
> Hi,
> 
> On Mon, Jul 18, 2022 at 08:49:08AM -0700, Ricardo Koller wrote:
> > There are various pmu tests that require an isb() between enabling
> > counting and the actual counting. This can lead to count registers
> > reporting less events than expected; the actual enabling happens after
> > some events have happened.  For example, some missing isb()'s in the
> > pmu-sw-incr test lead to the following errors on bare-metal:
> > 
> > 	INFO: pmu: pmu-sw-incr: SW_INCR counter #0 has value 4294967280
> >         PASS: pmu: pmu-sw-incr: PWSYNC does not increment if PMCR.E is unset
> >         FAIL: pmu: pmu-sw-incr: counter #1 after + 100 SW_INCR
> >         FAIL: pmu: pmu-sw-incr: counter #0 after + 100 SW_INCR
> >         INFO: pmu: pmu-sw-incr: counter values after 100 SW_INCR #0=82 #1=98
> >         PASS: pmu: pmu-sw-incr: overflow on counter #0 after 100 SW_INCR
> >         SUMMARY: 4 tests, 2 unexpected failures
> > 
> > Add the missing isb()'s on all failing tests, plus some others that are
> > not currently required but might in the future (like an isb() after
> > clearing the overflow signal in the IRQ handler).
> 
> That's rather cryptic. What might require those hypothetical ISBs and why? Why
> should a test add code for some hypothetical requirement that might, or might
> not, be implemented?

Good point, this wasn't very clear. Will add something more specific.

> 
> This is pure speculation on my part, were you seeing spurious interrupts that
> went away after adding the ISB in irq_handler()?

I didn't see any. But I think it could happen: multiple spurious
interrupts until the line finally gets cleared.

> 
> A couple of comments below.
> 
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arm/pmu.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 15c542a2..fd838392 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -307,6 +307,7 @@ static void irq_handler(struct pt_regs *regs)
> >  			}
> >  		}
> >  		write_sysreg(ALL_SET, pmovsclr_el0);
> > +		isb();
> >  	} else {
> >  		pmu_stats.unexpected = true;
> >  	}
> > @@ -534,6 +535,7 @@ static void test_sw_incr(void)
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > +	isb();
> >  
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> 
> Since your patch adds needed synchronization, from ARM DDI 0487H.a, page
> D13-5237:
> 
> "Where a direct write to one register causes a bit or field in a different
> register [..] to be updated as a side-effect of that direct write [..], the
> change to the different register [..] is defined to be an indirect write. In
> this case, the indirect write is only guaranteed to be visible to subsequent
> direct or indirect reads or writes if synchronization is performed after the
> direct write and before the subsequent direct or indirect reads or writes."
> 
> I think that says that you need an ISB after the direct writes to PMSWINC_EL0
> for software to read the correct value for PMEVNCTR0_EL0.
> 
> > @@ -547,6 +549,7 @@ static void test_sw_incr(void)
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> >  
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x3, pmswinc_el0);
> 
> Same as above, might be worth checking in other places.
> 
> Will come back with more review comments.
> 
> Thanks,
> Alex
> 
> > @@ -618,6 +621,8 @@ static void test_chained_sw_incr(void)
> >  
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> > +
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> > @@ -634,6 +639,8 @@ static void test_chained_sw_incr(void)
> >  	write_regn_el0(pmevcntr, 1, ALL_SET);
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> > +
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> > @@ -821,6 +828,8 @@ static void test_overflow_interrupt(void)
> >  	report(expect_interrupts(0), "no overflow interrupt after preset");
> >  
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> > +
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x2, pmswinc_el0);
> >  
> > @@ -879,6 +888,7 @@ static bool check_cycles_increase(void)
> >  	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
> >  
> >  	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
> > +	isb();
> >  
> >  	for (int i = 0; i < NR_SAMPLES; i++) {
> >  		uint64_t a, b;
> > @@ -894,6 +904,7 @@ static bool check_cycles_increase(void)
> >  	}
> >  
> >  	set_pmcr(get_pmcr() & ~PMU_PMCR_E);
> > +	isb();
> >  
> >  	return success;
> >  }
> > -- 
> > 2.37.0.170.g444d1eabd0-goog
> > 

Thanks!
Ricardo

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing
@ 2022-07-18 17:48       ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-18 17:48 UTC (permalink / raw)
  To: Alexandru Elisei; +Cc: drjones, kvm, maz, oliver.upton, kvmarm

On Mon, Jul 18, 2022 at 05:38:23PM +0100, Alexandru Elisei wrote:
> Hi,
> 
> On Mon, Jul 18, 2022 at 08:49:08AM -0700, Ricardo Koller wrote:
> > There are various pmu tests that require an isb() between enabling
> > counting and the actual counting. This can lead to count registers
> > reporting less events than expected; the actual enabling happens after
> > some events have happened.  For example, some missing isb()'s in the
> > pmu-sw-incr test lead to the following errors on bare-metal:
> > 
> > 	INFO: pmu: pmu-sw-incr: SW_INCR counter #0 has value 4294967280
> >         PASS: pmu: pmu-sw-incr: PWSYNC does not increment if PMCR.E is unset
> >         FAIL: pmu: pmu-sw-incr: counter #1 after + 100 SW_INCR
> >         FAIL: pmu: pmu-sw-incr: counter #0 after + 100 SW_INCR
> >         INFO: pmu: pmu-sw-incr: counter values after 100 SW_INCR #0=82 #1=98
> >         PASS: pmu: pmu-sw-incr: overflow on counter #0 after 100 SW_INCR
> >         SUMMARY: 4 tests, 2 unexpected failures
> > 
> > Add the missing isb()'s on all failing tests, plus some others that are
> > not currently required but might in the future (like an isb() after
> > clearing the overflow signal in the IRQ handler).
> 
> That's rather cryptic. What might require those hypothetical ISBs and why? Why
> should a test add code for some hypothetical requirement that might, or might
> not, be implemented?

Good point, this wasn't very clear. Will add something more specific.

> 
> This is pure speculation on my part, were you seeing spurious interrupts that
> went away after adding the ISB in irq_handler()?

I didn't see any. But I think it could happen: multiple spurious
interrupts until the line finally gets cleared.

> 
> A couple of comments below.
> 
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arm/pmu.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 15c542a2..fd838392 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -307,6 +307,7 @@ static void irq_handler(struct pt_regs *regs)
> >  			}
> >  		}
> >  		write_sysreg(ALL_SET, pmovsclr_el0);
> > +		isb();
> >  	} else {
> >  		pmu_stats.unexpected = true;
> >  	}
> > @@ -534,6 +535,7 @@ static void test_sw_incr(void)
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > +	isb();
> >  
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> 
> Since your patch adds needed synchronization, from ARM DDI 0487H.a, page
> D13-5237:
> 
> "Where a direct write to one register causes a bit or field in a different
> register [..] to be updated as a side-effect of that direct write [..], the
> change to the different register [..] is defined to be an indirect write. In
> this case, the indirect write is only guaranteed to be visible to subsequent
> direct or indirect reads or writes if synchronization is performed after the
> direct write and before the subsequent direct or indirect reads or writes."
> 
> I think that says that you need an ISB after the direct writes to PMSWINC_EL0
> for software to read the correct value for PMEVNCTR0_EL0.
> 
> > @@ -547,6 +549,7 @@ static void test_sw_incr(void)
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> >  
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x3, pmswinc_el0);
> 
> Same as above, might be worth checking in other places.
> 
> Will come back with more review comments.
> 
> Thanks,
> Alex
> 
> > @@ -618,6 +621,8 @@ static void test_chained_sw_incr(void)
> >  
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> > +
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> > @@ -634,6 +639,8 @@ static void test_chained_sw_incr(void)
> >  	write_regn_el0(pmevcntr, 1, ALL_SET);
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> > +
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> > @@ -821,6 +828,8 @@ static void test_overflow_interrupt(void)
> >  	report(expect_interrupts(0), "no overflow interrupt after preset");
> >  
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> > +
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x2, pmswinc_el0);
> >  
> > @@ -879,6 +888,7 @@ static bool check_cycles_increase(void)
> >  	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
> >  
> >  	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
> > +	isb();
> >  
> >  	for (int i = 0; i < NR_SAMPLES; i++) {
> >  		uint64_t a, b;
> > @@ -894,6 +904,7 @@ static bool check_cycles_increase(void)
> >  	}
> >  
> >  	set_pmcr(get_pmcr() & ~PMU_PMCR_E);
> > +	isb();
> >  
> >  	return success;
> >  }
> > -- 
> > 2.37.0.170.g444d1eabd0-goog
> > 

Thanks!
Ricardo
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing
  2022-07-18 15:49   ` Ricardo Koller
@ 2022-07-19 11:14     ` Alexandru Elisei
  -1 siblings, 0 replies; 48+ messages in thread
From: Alexandru Elisei @ 2022-07-19 11:14 UTC (permalink / raw)
  To: Ricardo Koller
  Cc: kvm, kvmarm, drjones, maz, eric.auger, oliver.upton, reijiw

Hi,

Since you're touching the PMU tests, I took the liberty to suggest changes
somewhat related to this patch. If you don't want to implement them, let me
know and I'll try to make a patch/series out of them.

On Mon, Jul 18, 2022 at 08:49:08AM -0700, Ricardo Koller wrote:
> There are various pmu tests that require an isb() between enabling
> counting and the actual counting. This can lead to count registers
> reporting less events than expected; the actual enabling happens after
> some events have happened.  For example, some missing isb()'s in the
> pmu-sw-incr test lead to the following errors on bare-metal:
> 
> 	INFO: pmu: pmu-sw-incr: SW_INCR counter #0 has value 4294967280
>         PASS: pmu: pmu-sw-incr: PWSYNC does not increment if PMCR.E is unset
>         FAIL: pmu: pmu-sw-incr: counter #1 after + 100 SW_INCR
>         FAIL: pmu: pmu-sw-incr: counter #0 after + 100 SW_INCR
>         INFO: pmu: pmu-sw-incr: counter values after 100 SW_INCR #0=82 #1=98
>         PASS: pmu: pmu-sw-incr: overflow on counter #0 after 100 SW_INCR
>         SUMMARY: 4 tests, 2 unexpected failures
> 
> Add the missing isb()'s on all failing tests, plus some others that are
> not currently required but might in the future (like an isb() after
> clearing the overflow signal in the IRQ handler).
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arm/pmu.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 15c542a2..fd838392 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -307,6 +307,7 @@ static void irq_handler(struct pt_regs *regs)
>  			}
>  		}
>  		write_sysreg(ALL_SET, pmovsclr_el0);
> +		isb();
>  	} else {
>  		pmu_stats.unexpected = true;
>  	}
> @@ -534,6 +535,7 @@ static void test_sw_incr(void)
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> +	isb();
>  
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
> @@ -547,6 +549,7 @@ static void test_sw_incr(void)
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
>  
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x3, pmswinc_el0);
> @@ -618,6 +621,8 @@ static void test_chained_sw_incr(void)
>  
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
> +
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
>  
> @@ -634,6 +639,8 @@ static void test_chained_sw_incr(void)
>  	write_regn_el0(pmevcntr, 1, ALL_SET);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
> +
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
>  
> @@ -821,6 +828,8 @@ static void test_overflow_interrupt(void)
>  	report(expect_interrupts(0), "no overflow interrupt after preset");
>  
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
> +
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x2, pmswinc_el0);

You missed the set_pmcr(pmu.pmcr_ro) call on the next line.

Also the comment "enable interrupts" below:

[..]
        report(expect_interrupts(0), "no overflow interrupt after preset");

        set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
        for (i = 0; i < 100; i++)
                write_sysreg(0x2, pmswinc_el0);

        set_pmcr(pmu.pmcr_ro);
        report(expect_interrupts(0), "no overflow interrupt after counting");

        /* enable interrupts */

        pmu_reset_stats();
[..]

is misleading, because pmu_reset_stats() doesn't enable the PMU. Unless the
intention was to call pmu_reset(), in which case the comment is correct and
the code is wrong. My guess is that the comment is incorrect, the test
seems to be working fine when the PMU is enabled in the mem_access_loop()
call.

>  
> @@ -879,6 +888,7 @@ static bool check_cycles_increase(void)
>  	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
>  
>  	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
> +	isb();
>  
>  	for (int i = 0; i < NR_SAMPLES; i++) {
>  		uint64_t a, b;
> @@ -894,6 +904,7 @@ static bool check_cycles_increase(void)
>  	}
>  
>  	set_pmcr(get_pmcr() & ~PMU_PMCR_E);
> +	isb();

Those look good to me.

Thanks,
Alex

>  
>  	return success;
>  }
> -- 
> 2.37.0.170.g444d1eabd0-goog
> 

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing
@ 2022-07-19 11:14     ` Alexandru Elisei
  0 siblings, 0 replies; 48+ messages in thread
From: Alexandru Elisei @ 2022-07-19 11:14 UTC (permalink / raw)
  To: Ricardo Koller; +Cc: drjones, kvm, maz, oliver.upton, kvmarm

Hi,

Since you're touching the PMU tests, I took the liberty to suggest changes
somewhat related to this patch. If you don't want to implement them, let me
know and I'll try to make a patch/series out of them.

On Mon, Jul 18, 2022 at 08:49:08AM -0700, Ricardo Koller wrote:
> There are various pmu tests that require an isb() between enabling
> counting and the actual counting. This can lead to count registers
> reporting less events than expected; the actual enabling happens after
> some events have happened.  For example, some missing isb()'s in the
> pmu-sw-incr test lead to the following errors on bare-metal:
> 
> 	INFO: pmu: pmu-sw-incr: SW_INCR counter #0 has value 4294967280
>         PASS: pmu: pmu-sw-incr: PWSYNC does not increment if PMCR.E is unset
>         FAIL: pmu: pmu-sw-incr: counter #1 after + 100 SW_INCR
>         FAIL: pmu: pmu-sw-incr: counter #0 after + 100 SW_INCR
>         INFO: pmu: pmu-sw-incr: counter values after 100 SW_INCR #0=82 #1=98
>         PASS: pmu: pmu-sw-incr: overflow on counter #0 after 100 SW_INCR
>         SUMMARY: 4 tests, 2 unexpected failures
> 
> Add the missing isb()'s on all failing tests, plus some others that are
> not currently required but might in the future (like an isb() after
> clearing the overflow signal in the IRQ handler).
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arm/pmu.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 15c542a2..fd838392 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -307,6 +307,7 @@ static void irq_handler(struct pt_regs *regs)
>  			}
>  		}
>  		write_sysreg(ALL_SET, pmovsclr_el0);
> +		isb();
>  	} else {
>  		pmu_stats.unexpected = true;
>  	}
> @@ -534,6 +535,7 @@ static void test_sw_incr(void)
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> +	isb();
>  
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
> @@ -547,6 +549,7 @@ static void test_sw_incr(void)
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
>  
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x3, pmswinc_el0);
> @@ -618,6 +621,8 @@ static void test_chained_sw_incr(void)
>  
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
> +
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
>  
> @@ -634,6 +639,8 @@ static void test_chained_sw_incr(void)
>  	write_regn_el0(pmevcntr, 1, ALL_SET);
>  	write_sysreg_s(0x3, PMCNTENSET_EL0);
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
> +
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
>  
> @@ -821,6 +828,8 @@ static void test_overflow_interrupt(void)
>  	report(expect_interrupts(0), "no overflow interrupt after preset");
>  
>  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> +	isb();
> +
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x2, pmswinc_el0);

You missed the set_pmcr(pmu.pmcr_ro) call on the next line.

Also the comment "enable interrupts" below:

[..]
        report(expect_interrupts(0), "no overflow interrupt after preset");

        set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
        for (i = 0; i < 100; i++)
                write_sysreg(0x2, pmswinc_el0);

        set_pmcr(pmu.pmcr_ro);
        report(expect_interrupts(0), "no overflow interrupt after counting");

        /* enable interrupts */

        pmu_reset_stats();
[..]

is misleading, because pmu_reset_stats() doesn't enable the PMU. Unless the
intention was to call pmu_reset(), in which case the comment is correct and
the code is wrong. My guess is that the comment is incorrect, the test
seems to be working fine when the PMU is enabled in the mem_access_loop()
call.

>  
> @@ -879,6 +888,7 @@ static bool check_cycles_increase(void)
>  	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
>  
>  	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
> +	isb();
>  
>  	for (int i = 0; i < NR_SAMPLES; i++) {
>  		uint64_t a, b;
> @@ -894,6 +904,7 @@ static bool check_cycles_increase(void)
>  	}
>  
>  	set_pmcr(get_pmcr() & ~PMU_PMCR_E);
> +	isb();

Those look good to me.

Thanks,
Alex

>  
>  	return success;
>  }
> -- 
> 2.37.0.170.g444d1eabd0-goog
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing
  2022-07-18 17:48       ` Ricardo Koller
@ 2022-07-19 11:26         ` Alexandru Elisei
  -1 siblings, 0 replies; 48+ messages in thread
From: Alexandru Elisei @ 2022-07-19 11:26 UTC (permalink / raw)
  To: Ricardo Koller
  Cc: kvm, kvmarm, drjones, maz, eric.auger, oliver.upton, reijiw

Hi,

On Mon, Jul 18, 2022 at 10:48:29AM -0700, Ricardo Koller wrote:
> On Mon, Jul 18, 2022 at 05:38:23PM +0100, Alexandru Elisei wrote:
> > Hi,
> > 
> > On Mon, Jul 18, 2022 at 08:49:08AM -0700, Ricardo Koller wrote:
> > > There are various pmu tests that require an isb() between enabling
> > > counting and the actual counting. This can lead to count registers
> > > reporting less events than expected; the actual enabling happens after
> > > some events have happened.  For example, some missing isb()'s in the
> > > pmu-sw-incr test lead to the following errors on bare-metal:
> > > 
> > > 	INFO: pmu: pmu-sw-incr: SW_INCR counter #0 has value 4294967280
> > >         PASS: pmu: pmu-sw-incr: PWSYNC does not increment if PMCR.E is unset
> > >         FAIL: pmu: pmu-sw-incr: counter #1 after + 100 SW_INCR
> > >         FAIL: pmu: pmu-sw-incr: counter #0 after + 100 SW_INCR
> > >         INFO: pmu: pmu-sw-incr: counter values after 100 SW_INCR #0=82 #1=98
> > >         PASS: pmu: pmu-sw-incr: overflow on counter #0 after 100 SW_INCR
> > >         SUMMARY: 4 tests, 2 unexpected failures
> > > 
> > > Add the missing isb()'s on all failing tests, plus some others that are
> > > not currently required but might in the future (like an isb() after
> > > clearing the overflow signal in the IRQ handler).
> > 
> > That's rather cryptic. What might require those hypothetical ISBs and why? Why
> > should a test add code for some hypothetical requirement that might, or might
> > not, be implemented?
> 
> Good point, this wasn't very clear. Will add something more specific.
> 
> > 
> > This is pure speculation on my part, were you seeing spurious interrupts that
> > went away after adding the ISB in irq_handler()?
> 
> I didn't see any. But I think it could happen: multiple spurious
> interrupts until the line finally gets cleared.

I agree with you, it takes a finite time for any interrupt controller (in
our case, the GIC) to deassert the interrupt line to the CPU after a device
has deasserted the interrupt line to the interrupt controller. That's why
device drivers are usually robust in dealing with spurious interrupts.

It looks to me that the way irq_handler() treats spurious interrupts might
lead to tests being incorrectly treated as failed, which is going to be a
pain to reproduce and diagnose.

@Eric, was there a particular reason for this approach?

Thanks,
Alex

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing
@ 2022-07-19 11:26         ` Alexandru Elisei
  0 siblings, 0 replies; 48+ messages in thread
From: Alexandru Elisei @ 2022-07-19 11:26 UTC (permalink / raw)
  To: Ricardo Koller; +Cc: drjones, kvm, maz, oliver.upton, kvmarm

Hi,

On Mon, Jul 18, 2022 at 10:48:29AM -0700, Ricardo Koller wrote:
> On Mon, Jul 18, 2022 at 05:38:23PM +0100, Alexandru Elisei wrote:
> > Hi,
> > 
> > On Mon, Jul 18, 2022 at 08:49:08AM -0700, Ricardo Koller wrote:
> > > There are various pmu tests that require an isb() between enabling
> > > counting and the actual counting. This can lead to count registers
> > > reporting less events than expected; the actual enabling happens after
> > > some events have happened.  For example, some missing isb()'s in the
> > > pmu-sw-incr test lead to the following errors on bare-metal:
> > > 
> > > 	INFO: pmu: pmu-sw-incr: SW_INCR counter #0 has value 4294967280
> > >         PASS: pmu: pmu-sw-incr: PWSYNC does not increment if PMCR.E is unset
> > >         FAIL: pmu: pmu-sw-incr: counter #1 after + 100 SW_INCR
> > >         FAIL: pmu: pmu-sw-incr: counter #0 after + 100 SW_INCR
> > >         INFO: pmu: pmu-sw-incr: counter values after 100 SW_INCR #0=82 #1=98
> > >         PASS: pmu: pmu-sw-incr: overflow on counter #0 after 100 SW_INCR
> > >         SUMMARY: 4 tests, 2 unexpected failures
> > > 
> > > Add the missing isb()'s on all failing tests, plus some others that are
> > > not currently required but might in the future (like an isb() after
> > > clearing the overflow signal in the IRQ handler).
> > 
> > That's rather cryptic. What might require those hypothetical ISBs and why? Why
> > should a test add code for some hypothetical requirement that might, or might
> > not, be implemented?
> 
> Good point, this wasn't very clear. Will add something more specific.
> 
> > 
> > This is pure speculation on my part, were you seeing spurious interrupts that
> > went away after adding the ISB in irq_handler()?
> 
> I didn't see any. But I think it could happen: multiple spurious
> interrupts until the line finally gets cleared.

I agree with you, it takes a finite time for any interrupt controller (in
our case, the GIC) to deassert the interrupt line to the CPU after a device
has deasserted the interrupt line to the interrupt controller. That's why
device drivers are usually robust in dealing with spurious interrupts.

It looks to me that the way irq_handler() treats spurious interrupts might
lead to tests being incorrectly treated as failed, which is going to be a
pain to reproduce and diagnose.

@Eric, was there a particular reason for this approach?

Thanks,
Alex
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-18 15:49   ` Ricardo Koller
@ 2022-07-19 11:34     ` Marc Zyngier
  -1 siblings, 0 replies; 48+ messages in thread
From: Marc Zyngier @ 2022-07-19 11:34 UTC (permalink / raw)
  To: Ricardo Koller
  Cc: kvm, kvmarm, drjones, alexandru.elisei, eric.auger, oliver.upton, reijiw

On Mon, 18 Jul 2022 16:49:10 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> A chained event overflowing on the low counter can set the overflow flag
> in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> overflow.

Isn't this indicative of a bug in the KVM emulation? To be honest, the
pseudocode looks odd. It says:

<quote>
	if old_value<64:ovflw> != new_value<64:ovflw> then
	    PMOVSSET_EL0<idx> = '1';
	    PMOVSCLR_EL0<idx> = '1';
</quote>

which I find remarkably ambiguous. Is this setting and clearing the
overflow bit? Or setting it in the single register that backs the two
accessors in whatever way it can?

If it is the second interpretation that is correct, then KVM
definitely needs fixing (though this looks pretty involved for
anything that isn't a SWINC event).

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-19 11:34     ` Marc Zyngier
  0 siblings, 0 replies; 48+ messages in thread
From: Marc Zyngier @ 2022-07-19 11:34 UTC (permalink / raw)
  To: Ricardo Koller; +Cc: drjones, kvm, oliver.upton, kvmarm

On Mon, 18 Jul 2022 16:49:10 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> A chained event overflowing on the low counter can set the overflow flag
> in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> overflow.

Isn't this indicative of a bug in the KVM emulation? To be honest, the
pseudocode looks odd. It says:

<quote>
	if old_value<64:ovflw> != new_value<64:ovflw> then
	    PMOVSSET_EL0<idx> = '1';
	    PMOVSCLR_EL0<idx> = '1';
</quote>

which I find remarkably ambiguous. Is this setting and clearing the
overflow bit? Or setting it in the single register that backs the two
accessors in whatever way it can?

If it is the second interpretation that is correct, then KVM
definitely needs fixing (though this looks pretty involved for
anything that isn't a SWINC event).

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-19 11:34     ` Marc Zyngier
@ 2022-07-20  8:40       ` Ricardo Koller
  -1 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-20  8:40 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvm, kvmarm, drjones, alexandru.elisei, eric.auger, oliver.upton, reijiw

On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> On Mon, 18 Jul 2022 16:49:10 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > A chained event overflowing on the low counter can set the overflow flag
> > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > overflow.
> 
> Isn't this indicative of a bug in the KVM emulation? To be honest, the
> pseudocode looks odd. It says:
> 
> <quote>
> 	if old_value<64:ovflw> != new_value<64:ovflw> then
> 	    PMOVSSET_EL0<idx> = '1';
> 	    PMOVSCLR_EL0<idx> = '1';
> </quote>
> 
> which I find remarkably ambiguous. Is this setting and clearing the
> overflow bit? Or setting it in the single register that backs the two
> accessors in whatever way it can?
> 
> If it is the second interpretation that is correct, then KVM
> definitely needs fixing

I think it's the second, as those two "= '1'" apply to the non-chained
counters case as well, which should definitely set the bit in PMOVSSET.

> (though this looks pretty involved for
> anything that isn't a SWINC event).

Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
SWINC, but a non-SWINC event is implemented as a single 64-bit perf
event.

Thanks,
Ricardo

> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-20  8:40       ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-20  8:40 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: drjones, kvm, oliver.upton, kvmarm

On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> On Mon, 18 Jul 2022 16:49:10 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > A chained event overflowing on the low counter can set the overflow flag
> > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > overflow.
> 
> Isn't this indicative of a bug in the KVM emulation? To be honest, the
> pseudocode looks odd. It says:
> 
> <quote>
> 	if old_value<64:ovflw> != new_value<64:ovflw> then
> 	    PMOVSSET_EL0<idx> = '1';
> 	    PMOVSCLR_EL0<idx> = '1';
> </quote>
> 
> which I find remarkably ambiguous. Is this setting and clearing the
> overflow bit? Or setting it in the single register that backs the two
> accessors in whatever way it can?
> 
> If it is the second interpretation that is correct, then KVM
> definitely needs fixing

I think it's the second, as those two "= '1'" apply to the non-chained
counters case as well, which should definitely set the bit in PMOVSSET.

> (though this looks pretty involved for
> anything that isn't a SWINC event).

Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
SWINC, but a non-SWINC event is implemented as a single 64-bit perf
event.

Thanks,
Ricardo

> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-20  8:40       ` Ricardo Koller
@ 2022-07-20  9:45         ` Marc Zyngier
  -1 siblings, 0 replies; 48+ messages in thread
From: Marc Zyngier @ 2022-07-20  9:45 UTC (permalink / raw)
  To: Ricardo Koller
  Cc: kvm, kvmarm, drjones, alexandru.elisei, eric.auger, oliver.upton, reijiw

On Wed, 20 Jul 2022 09:40:01 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> > On Mon, 18 Jul 2022 16:49:10 +0100,
> > Ricardo Koller <ricarkol@google.com> wrote:
> > > 
> > > A chained event overflowing on the low counter can set the overflow flag
> > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > overflow.
> > 
> > Isn't this indicative of a bug in the KVM emulation? To be honest, the
> > pseudocode looks odd. It says:
> > 
> > <quote>
> > 	if old_value<64:ovflw> != new_value<64:ovflw> then
> > 	    PMOVSSET_EL0<idx> = '1';
> > 	    PMOVSCLR_EL0<idx> = '1';
> > </quote>
> > 
> > which I find remarkably ambiguous. Is this setting and clearing the
> > overflow bit? Or setting it in the single register that backs the two
> > accessors in whatever way it can?
> > 
> > If it is the second interpretation that is correct, then KVM
> > definitely needs fixing
> 
> I think it's the second, as those two "= '1'" apply to the non-chained
> counters case as well, which should definitely set the bit in PMOVSSET.
> 
> > (though this looks pretty involved for
> > anything that isn't a SWINC event).
> 
> Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
> SWINC, but a non-SWINC event is implemented as a single 64-bit perf
> event.

Indeed. Which means we need to de-optimise chained counters to being
32bit events, which is pretty annoying (for rapidly firing events, the
interrupt rate is going to be significantly higher).

I guess we should also investigate the support for FEAT_PMUv3p5 and
native 64bit counters. Someone is bound to build it at some point.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-20  9:45         ` Marc Zyngier
  0 siblings, 0 replies; 48+ messages in thread
From: Marc Zyngier @ 2022-07-20  9:45 UTC (permalink / raw)
  To: Ricardo Koller; +Cc: drjones, kvm, oliver.upton, kvmarm

On Wed, 20 Jul 2022 09:40:01 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> > On Mon, 18 Jul 2022 16:49:10 +0100,
> > Ricardo Koller <ricarkol@google.com> wrote:
> > > 
> > > A chained event overflowing on the low counter can set the overflow flag
> > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > overflow.
> > 
> > Isn't this indicative of a bug in the KVM emulation? To be honest, the
> > pseudocode looks odd. It says:
> > 
> > <quote>
> > 	if old_value<64:ovflw> != new_value<64:ovflw> then
> > 	    PMOVSSET_EL0<idx> = '1';
> > 	    PMOVSCLR_EL0<idx> = '1';
> > </quote>
> > 
> > which I find remarkably ambiguous. Is this setting and clearing the
> > overflow bit? Or setting it in the single register that backs the two
> > accessors in whatever way it can?
> > 
> > If it is the second interpretation that is correct, then KVM
> > definitely needs fixing
> 
> I think it's the second, as those two "= '1'" apply to the non-chained
> counters case as well, which should definitely set the bit in PMOVSSET.
> 
> > (though this looks pretty involved for
> > anything that isn't a SWINC event).
> 
> Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
> SWINC, but a non-SWINC event is implemented as a single 64-bit perf
> event.

Indeed. Which means we need to de-optimise chained counters to being
32bit events, which is pretty annoying (for rapidly firing events, the
interrupt rate is going to be significantly higher).

I guess we should also investigate the support for FEAT_PMUv3p5 and
native 64bit counters. Someone is bound to build it at some point.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-20  9:45         ` Marc Zyngier
@ 2022-07-20 21:17           ` Ricardo Koller
  -1 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-20 21:17 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvm, kvmarm, drjones, alexandru.elisei, eric.auger, oliver.upton, reijiw

On Wed, Jul 20, 2022 at 10:45:20AM +0100, Marc Zyngier wrote:
> On Wed, 20 Jul 2022 09:40:01 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> > > On Mon, 18 Jul 2022 16:49:10 +0100,
> > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > 
> > > > A chained event overflowing on the low counter can set the overflow flag
> > > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > > overflow.
> > > 
> > > Isn't this indicative of a bug in the KVM emulation? To be honest, the
> > > pseudocode looks odd. It says:
> > > 
> > > <quote>
> > > 	if old_value<64:ovflw> != new_value<64:ovflw> then
> > > 	    PMOVSSET_EL0<idx> = '1';
> > > 	    PMOVSCLR_EL0<idx> = '1';
> > > </quote>
> > > 
> > > which I find remarkably ambiguous. Is this setting and clearing the
> > > overflow bit? Or setting it in the single register that backs the two
> > > accessors in whatever way it can?
> > > 
> > > If it is the second interpretation that is correct, then KVM
> > > definitely needs fixing
> > 
> > I think it's the second, as those two "= '1'" apply to the non-chained
> > counters case as well, which should definitely set the bit in PMOVSSET.
> > 
> > > (though this looks pretty involved for
> > > anything that isn't a SWINC event).
> > 
> > Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
> > SWINC, but a non-SWINC event is implemented as a single 64-bit perf
> > event.
> 
> Indeed. Which means we need to de-optimise chained counters to being
> 32bit events, which is pretty annoying (for rapidly firing events, the
> interrupt rate is going to be significantly higher).
> 
> I guess we should also investigate the support for FEAT_PMUv3p5 and
> native 64bit counters. Someone is bound to build it at some point.

The kernel perf event is implementing 64-bit counters using chained
counters. I assume this is already firing an interrupt for the low
counter overflow; we might need to just hook into that, investigating...

> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-20 21:17           ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-20 21:17 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: drjones, kvm, oliver.upton, kvmarm

On Wed, Jul 20, 2022 at 10:45:20AM +0100, Marc Zyngier wrote:
> On Wed, 20 Jul 2022 09:40:01 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> > > On Mon, 18 Jul 2022 16:49:10 +0100,
> > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > 
> > > > A chained event overflowing on the low counter can set the overflow flag
> > > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > > overflow.
> > > 
> > > Isn't this indicative of a bug in the KVM emulation? To be honest, the
> > > pseudocode looks odd. It says:
> > > 
> > > <quote>
> > > 	if old_value<64:ovflw> != new_value<64:ovflw> then
> > > 	    PMOVSSET_EL0<idx> = '1';
> > > 	    PMOVSCLR_EL0<idx> = '1';
> > > </quote>
> > > 
> > > which I find remarkably ambiguous. Is this setting and clearing the
> > > overflow bit? Or setting it in the single register that backs the two
> > > accessors in whatever way it can?
> > > 
> > > If it is the second interpretation that is correct, then KVM
> > > definitely needs fixing
> > 
> > I think it's the second, as those two "= '1'" apply to the non-chained
> > counters case as well, which should definitely set the bit in PMOVSSET.
> > 
> > > (though this looks pretty involved for
> > > anything that isn't a SWINC event).
> > 
> > Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
> > SWINC, but a non-SWINC event is implemented as a single 64-bit perf
> > event.
> 
> Indeed. Which means we need to de-optimise chained counters to being
> 32bit events, which is pretty annoying (for rapidly firing events, the
> interrupt rate is going to be significantly higher).
> 
> I guess we should also investigate the support for FEAT_PMUv3p5 and
> native 64bit counters. Someone is bound to build it at some point.

The kernel perf event is implementing 64-bit counters using chained
counters. I assume this is already firing an interrupt for the low
counter overflow; we might need to just hook into that, investigating...

> 
> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing
  2022-07-19 11:14     ` Alexandru Elisei
@ 2022-07-20 21:20       ` Ricardo Koller
  -1 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-20 21:20 UTC (permalink / raw)
  To: Alexandru Elisei
  Cc: kvm, kvmarm, drjones, maz, eric.auger, oliver.upton, reijiw

On Tue, Jul 19, 2022 at 12:14:37PM +0100, Alexandru Elisei wrote:
> Hi,
> 
> Since you're touching the PMU tests, I took the liberty to suggest changes
> somewhat related to this patch. If you don't want to implement them, let me
> know and I'll try to make a patch/series out of them.
> 
> On Mon, Jul 18, 2022 at 08:49:08AM -0700, Ricardo Koller wrote:
> > There are various pmu tests that require an isb() between enabling
> > counting and the actual counting. This can lead to count registers
> > reporting less events than expected; the actual enabling happens after
> > some events have happened.  For example, some missing isb()'s in the
> > pmu-sw-incr test lead to the following errors on bare-metal:
> > 
> > 	INFO: pmu: pmu-sw-incr: SW_INCR counter #0 has value 4294967280
> >         PASS: pmu: pmu-sw-incr: PWSYNC does not increment if PMCR.E is unset
> >         FAIL: pmu: pmu-sw-incr: counter #1 after + 100 SW_INCR
> >         FAIL: pmu: pmu-sw-incr: counter #0 after + 100 SW_INCR
> >         INFO: pmu: pmu-sw-incr: counter values after 100 SW_INCR #0=82 #1=98
> >         PASS: pmu: pmu-sw-incr: overflow on counter #0 after 100 SW_INCR
> >         SUMMARY: 4 tests, 2 unexpected failures
> > 
> > Add the missing isb()'s on all failing tests, plus some others that are
> > not currently required but might in the future (like an isb() after
> > clearing the overflow signal in the IRQ handler).
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arm/pmu.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 15c542a2..fd838392 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -307,6 +307,7 @@ static void irq_handler(struct pt_regs *regs)
> >  			}
> >  		}
> >  		write_sysreg(ALL_SET, pmovsclr_el0);
> > +		isb();
> >  	} else {
> >  		pmu_stats.unexpected = true;
> >  	}
> > @@ -534,6 +535,7 @@ static void test_sw_incr(void)
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > +	isb();
> >  
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> > @@ -547,6 +549,7 @@ static void test_sw_incr(void)
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> >  
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x3, pmswinc_el0);
> > @@ -618,6 +621,8 @@ static void test_chained_sw_incr(void)
> >  
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> > +
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> > @@ -634,6 +639,8 @@ static void test_chained_sw_incr(void)
> >  	write_regn_el0(pmevcntr, 1, ALL_SET);
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> > +
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> > @@ -821,6 +828,8 @@ static void test_overflow_interrupt(void)
> >  	report(expect_interrupts(0), "no overflow interrupt after preset");
> >  
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> > +
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x2, pmswinc_el0);
> 
> You missed the set_pmcr(pmu.pmcr_ro) call on the next line.

Will add this in V2.

> 
> Also the comment "enable interrupts" below:
> 
> [..]
>         report(expect_interrupts(0), "no overflow interrupt after preset");
> 
>         set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
>         for (i = 0; i < 100; i++)
>                 write_sysreg(0x2, pmswinc_el0);
> 
>         set_pmcr(pmu.pmcr_ro);
>         report(expect_interrupts(0), "no overflow interrupt after counting");
> 
>         /* enable interrupts */
> 
>         pmu_reset_stats();
> [..]
> 
> is misleading, because pmu_reset_stats() doesn't enable the PMU. Unless the
> intention was to call pmu_reset(), in which case the comment is correct and
> the code is wrong. My guess is that the comment is incorrect, the test
> seems to be working fine when the PMU is enabled in the mem_access_loop()
> call.

Yes, it seems that the comment is incorrect. Will fix this in V2.

> 
> >  
> > @@ -879,6 +888,7 @@ static bool check_cycles_increase(void)
> >  	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
> >  
> >  	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
> > +	isb();
> >  
> >  	for (int i = 0; i < NR_SAMPLES; i++) {
> >  		uint64_t a, b;
> > @@ -894,6 +904,7 @@ static bool check_cycles_increase(void)
> >  	}
> >  
> >  	set_pmcr(get_pmcr() & ~PMU_PMCR_E);
> > +	isb();
> 
> Those look good to me.
> 
> Thanks,
> Alex

Thanks for the reviews,
Ricardo

> 
> >  
> >  	return success;
> >  }
> > -- 
> > 2.37.0.170.g444d1eabd0-goog
> > 

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing
@ 2022-07-20 21:20       ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-20 21:20 UTC (permalink / raw)
  To: Alexandru Elisei; +Cc: drjones, kvm, maz, oliver.upton, kvmarm

On Tue, Jul 19, 2022 at 12:14:37PM +0100, Alexandru Elisei wrote:
> Hi,
> 
> Since you're touching the PMU tests, I took the liberty to suggest changes
> somewhat related to this patch. If you don't want to implement them, let me
> know and I'll try to make a patch/series out of them.
> 
> On Mon, Jul 18, 2022 at 08:49:08AM -0700, Ricardo Koller wrote:
> > There are various pmu tests that require an isb() between enabling
> > counting and the actual counting. This can lead to count registers
> > reporting less events than expected; the actual enabling happens after
> > some events have happened.  For example, some missing isb()'s in the
> > pmu-sw-incr test lead to the following errors on bare-metal:
> > 
> > 	INFO: pmu: pmu-sw-incr: SW_INCR counter #0 has value 4294967280
> >         PASS: pmu: pmu-sw-incr: PWSYNC does not increment if PMCR.E is unset
> >         FAIL: pmu: pmu-sw-incr: counter #1 after + 100 SW_INCR
> >         FAIL: pmu: pmu-sw-incr: counter #0 after + 100 SW_INCR
> >         INFO: pmu: pmu-sw-incr: counter values after 100 SW_INCR #0=82 #1=98
> >         PASS: pmu: pmu-sw-incr: overflow on counter #0 after 100 SW_INCR
> >         SUMMARY: 4 tests, 2 unexpected failures
> > 
> > Add the missing isb()'s on all failing tests, plus some others that are
> > not currently required but might in the future (like an isb() after
> > clearing the overflow signal in the IRQ handler).
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arm/pmu.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 15c542a2..fd838392 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -307,6 +307,7 @@ static void irq_handler(struct pt_regs *regs)
> >  			}
> >  		}
> >  		write_sysreg(ALL_SET, pmovsclr_el0);
> > +		isb();
> >  	} else {
> >  		pmu_stats.unexpected = true;
> >  	}
> > @@ -534,6 +535,7 @@ static void test_sw_incr(void)
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > +	isb();
> >  
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> > @@ -547,6 +549,7 @@ static void test_sw_incr(void)
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> >  
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x3, pmswinc_el0);
> > @@ -618,6 +621,8 @@ static void test_chained_sw_incr(void)
> >  
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> > +
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> > @@ -634,6 +639,8 @@ static void test_chained_sw_incr(void)
> >  	write_regn_el0(pmevcntr, 1, ALL_SET);
> >  	write_sysreg_s(0x3, PMCNTENSET_EL0);
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> > +
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> > @@ -821,6 +828,8 @@ static void test_overflow_interrupt(void)
> >  	report(expect_interrupts(0), "no overflow interrupt after preset");
> >  
> >  	set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
> > +	isb();
> > +
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x2, pmswinc_el0);
> 
> You missed the set_pmcr(pmu.pmcr_ro) call on the next line.

Will add this in V2.

> 
> Also the comment "enable interrupts" below:
> 
> [..]
>         report(expect_interrupts(0), "no overflow interrupt after preset");
> 
>         set_pmcr(pmu.pmcr_ro | PMU_PMCR_E);
>         for (i = 0; i < 100; i++)
>                 write_sysreg(0x2, pmswinc_el0);
> 
>         set_pmcr(pmu.pmcr_ro);
>         report(expect_interrupts(0), "no overflow interrupt after counting");
> 
>         /* enable interrupts */
> 
>         pmu_reset_stats();
> [..]
> 
> is misleading, because pmu_reset_stats() doesn't enable the PMU. Unless the
> intention was to call pmu_reset(), in which case the comment is correct and
> the code is wrong. My guess is that the comment is incorrect, the test
> seems to be working fine when the PMU is enabled in the mem_access_loop()
> call.

Yes, it seems that the comment is incorrect. Will fix this in V2.

> 
> >  
> > @@ -879,6 +888,7 @@ static bool check_cycles_increase(void)
> >  	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
> >  
> >  	set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
> > +	isb();
> >  
> >  	for (int i = 0; i < NR_SAMPLES; i++) {
> >  		uint64_t a, b;
> > @@ -894,6 +904,7 @@ static bool check_cycles_increase(void)
> >  	}
> >  
> >  	set_pmcr(get_pmcr() & ~PMU_PMCR_E);
> > +	isb();
> 
> Those look good to me.
> 
> Thanks,
> Alex

Thanks for the reviews,
Ricardo

> 
> >  
> >  	return success;
> >  }
> > -- 
> > 2.37.0.170.g444d1eabd0-goog
> > 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-20 21:17           ` Ricardo Koller
@ 2022-07-20 21:26             ` Ricardo Koller
  -1 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-20 21:26 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvm, kvmarm, drjones, alexandru.elisei, eric.auger, oliver.upton, reijiw

On Wed, Jul 20, 2022 at 02:17:09PM -0700, Ricardo Koller wrote:
> On Wed, Jul 20, 2022 at 10:45:20AM +0100, Marc Zyngier wrote:
> > On Wed, 20 Jul 2022 09:40:01 +0100,
> > Ricardo Koller <ricarkol@google.com> wrote:
> > > 
> > > On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> > > > On Mon, 18 Jul 2022 16:49:10 +0100,
> > > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > > 
> > > > > A chained event overflowing on the low counter can set the overflow flag
> > > > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > > > overflow.
> > > > 
> > > > Isn't this indicative of a bug in the KVM emulation? To be honest, the
> > > > pseudocode looks odd. It says:
> > > > 
> > > > <quote>
> > > > 	if old_value<64:ovflw> != new_value<64:ovflw> then
> > > > 	    PMOVSSET_EL0<idx> = '1';
> > > > 	    PMOVSCLR_EL0<idx> = '1';
> > > > </quote>
> > > > 
> > > > which I find remarkably ambiguous. Is this setting and clearing the
> > > > overflow bit? Or setting it in the single register that backs the two
> > > > accessors in whatever way it can?
> > > > 
> > > > If it is the second interpretation that is correct, then KVM
> > > > definitely needs fixing
> > > 
> > > I think it's the second, as those two "= '1'" apply to the non-chained
> > > counters case as well, which should definitely set the bit in PMOVSSET.
> > > 
> > > > (though this looks pretty involved for
> > > > anything that isn't a SWINC event).
> > > 
> > > Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
> > > SWINC, but a non-SWINC event is implemented as a single 64-bit perf
> > > event.
> > 
> > Indeed. Which means we need to de-optimise chained counters to being
> > 32bit events, which is pretty annoying (for rapidly firing events, the
> > interrupt rate is going to be significantly higher).
> > 
> > I guess we should also investigate the support for FEAT_PMUv3p5 and
> > native 64bit counters. Someone is bound to build it at some point.
> 
> The kernel perf event is implementing 64-bit counters using chained
> counters. I assume this is already firing an interrupt for the low
> counter overflow; we might need to just hook into that, investigating...
> 

Additionally, given that the kernel is already emulating 64-bit
counters, can KVM just expose FEAT_PMUv3p5? Assuming all the other new
features can be emulated.

Thanks,
Ricardo

> > 
> > Thanks,
> > 
> > 	M.
> > 
> > -- 
> > Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-20 21:26             ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-20 21:26 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: drjones, kvm, oliver.upton, kvmarm

On Wed, Jul 20, 2022 at 02:17:09PM -0700, Ricardo Koller wrote:
> On Wed, Jul 20, 2022 at 10:45:20AM +0100, Marc Zyngier wrote:
> > On Wed, 20 Jul 2022 09:40:01 +0100,
> > Ricardo Koller <ricarkol@google.com> wrote:
> > > 
> > > On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> > > > On Mon, 18 Jul 2022 16:49:10 +0100,
> > > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > > 
> > > > > A chained event overflowing on the low counter can set the overflow flag
> > > > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > > > overflow.
> > > > 
> > > > Isn't this indicative of a bug in the KVM emulation? To be honest, the
> > > > pseudocode looks odd. It says:
> > > > 
> > > > <quote>
> > > > 	if old_value<64:ovflw> != new_value<64:ovflw> then
> > > > 	    PMOVSSET_EL0<idx> = '1';
> > > > 	    PMOVSCLR_EL0<idx> = '1';
> > > > </quote>
> > > > 
> > > > which I find remarkably ambiguous. Is this setting and clearing the
> > > > overflow bit? Or setting it in the single register that backs the two
> > > > accessors in whatever way it can?
> > > > 
> > > > If it is the second interpretation that is correct, then KVM
> > > > definitely needs fixing
> > > 
> > > I think it's the second, as those two "= '1'" apply to the non-chained
> > > counters case as well, which should definitely set the bit in PMOVSSET.
> > > 
> > > > (though this looks pretty involved for
> > > > anything that isn't a SWINC event).
> > > 
> > > Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
> > > SWINC, but a non-SWINC event is implemented as a single 64-bit perf
> > > event.
> > 
> > Indeed. Which means we need to de-optimise chained counters to being
> > 32bit events, which is pretty annoying (for rapidly firing events, the
> > interrupt rate is going to be significantly higher).
> > 
> > I guess we should also investigate the support for FEAT_PMUv3p5 and
> > native 64bit counters. Someone is bound to build it at some point.
> 
> The kernel perf event is implementing 64-bit counters using chained
> counters. I assume this is already firing an interrupt for the low
> counter overflow; we might need to just hook into that, investigating...
> 

Additionally, given that the kernel is already emulating 64-bit
counters, can KVM just expose FEAT_PMUv3p5? Assuming all the other new
features can be emulated.

Thanks,
Ricardo

> > 
> > Thanks,
> > 
> > 	M.
> > 
> > -- 
> > Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-20 21:26             ` Ricardo Koller
@ 2022-07-21 13:43               ` Marc Zyngier
  -1 siblings, 0 replies; 48+ messages in thread
From: Marc Zyngier @ 2022-07-21 13:43 UTC (permalink / raw)
  To: Ricardo Koller; +Cc: drjones, kvm, kvmarm

On Wed, 20 Jul 2022 22:26:09 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Wed, Jul 20, 2022 at 02:17:09PM -0700, Ricardo Koller wrote:
> > On Wed, Jul 20, 2022 at 10:45:20AM +0100, Marc Zyngier wrote:
> > > On Wed, 20 Jul 2022 09:40:01 +0100,
> > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > 
> > > > On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> > > > > On Mon, 18 Jul 2022 16:49:10 +0100,
> > > > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > > > 
> > > > > > A chained event overflowing on the low counter can set the overflow flag
> > > > > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > > > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > > > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > > > > overflow.
> > > > > 
> > > > > Isn't this indicative of a bug in the KVM emulation? To be honest, the
> > > > > pseudocode looks odd. It says:
> > > > > 
> > > > > <quote>
> > > > > 	if old_value<64:ovflw> != new_value<64:ovflw> then
> > > > > 	    PMOVSSET_EL0<idx> = '1';
> > > > > 	    PMOVSCLR_EL0<idx> = '1';
> > > > > </quote>
> > > > > 
> > > > > which I find remarkably ambiguous. Is this setting and clearing the
> > > > > overflow bit? Or setting it in the single register that backs the two
> > > > > accessors in whatever way it can?
> > > > > 
> > > > > If it is the second interpretation that is correct, then KVM
> > > > > definitely needs fixing
> > > > 
> > > > I think it's the second, as those two "= '1'" apply to the non-chained
> > > > counters case as well, which should definitely set the bit in PMOVSSET.
> > > > 
> > > > > (though this looks pretty involved for
> > > > > anything that isn't a SWINC event).
> > > > 
> > > > Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
> > > > SWINC, but a non-SWINC event is implemented as a single 64-bit perf
> > > > event.
> > > 
> > > Indeed. Which means we need to de-optimise chained counters to being
> > > 32bit events, which is pretty annoying (for rapidly firing events, the
> > > interrupt rate is going to be significantly higher).
> > > 
> > > I guess we should also investigate the support for FEAT_PMUv3p5 and
> > > native 64bit counters. Someone is bound to build it at some point.
> > 
> > The kernel perf event is implementing 64-bit counters using chained
> > counters. I assume this is already firing an interrupt for the low
> > counter overflow; we might need to just hook into that, investigating...

We probably only enable the overflow interrupt on the odd counter, and
not the even one (which is why you request chained counters the first
place).

And perf wouldn't call us back anyway, as we described the counter as
64bit.

> Additionally, given that the kernel is already emulating 64-bit
> counters, can KVM just expose FEAT_PMUv3p5? Assuming all the other new
> features can be emulated.

This is what I suggested above. Although it can only happen on a
system that already supports FEAT_PMU3p4, as PMMIR_EL1 is not defined
before that (and FEAT_PMU3p5 implies 3p4).

It also remains that we need to *properly* emulate chained counters,
which means not handling them as 64bit counters in perf, but as a
32bit counter and a carry (exactly like the pseudocode does).

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-21 13:43               ` Marc Zyngier
  0 siblings, 0 replies; 48+ messages in thread
From: Marc Zyngier @ 2022-07-21 13:43 UTC (permalink / raw)
  To: Ricardo Koller
  Cc: kvm, kvmarm, drjones, alexandru.elisei, eric.auger, oliver.upton, reijiw

On Wed, 20 Jul 2022 22:26:09 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Wed, Jul 20, 2022 at 02:17:09PM -0700, Ricardo Koller wrote:
> > On Wed, Jul 20, 2022 at 10:45:20AM +0100, Marc Zyngier wrote:
> > > On Wed, 20 Jul 2022 09:40:01 +0100,
> > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > 
> > > > On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> > > > > On Mon, 18 Jul 2022 16:49:10 +0100,
> > > > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > > > 
> > > > > > A chained event overflowing on the low counter can set the overflow flag
> > > > > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > > > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > > > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > > > > overflow.
> > > > > 
> > > > > Isn't this indicative of a bug in the KVM emulation? To be honest, the
> > > > > pseudocode looks odd. It says:
> > > > > 
> > > > > <quote>
> > > > > 	if old_value<64:ovflw> != new_value<64:ovflw> then
> > > > > 	    PMOVSSET_EL0<idx> = '1';
> > > > > 	    PMOVSCLR_EL0<idx> = '1';
> > > > > </quote>
> > > > > 
> > > > > which I find remarkably ambiguous. Is this setting and clearing the
> > > > > overflow bit? Or setting it in the single register that backs the two
> > > > > accessors in whatever way it can?
> > > > > 
> > > > > If it is the second interpretation that is correct, then KVM
> > > > > definitely needs fixing
> > > > 
> > > > I think it's the second, as those two "= '1'" apply to the non-chained
> > > > counters case as well, which should definitely set the bit in PMOVSSET.
> > > > 
> > > > > (though this looks pretty involved for
> > > > > anything that isn't a SWINC event).
> > > > 
> > > > Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
> > > > SWINC, but a non-SWINC event is implemented as a single 64-bit perf
> > > > event.
> > > 
> > > Indeed. Which means we need to de-optimise chained counters to being
> > > 32bit events, which is pretty annoying (for rapidly firing events, the
> > > interrupt rate is going to be significantly higher).
> > > 
> > > I guess we should also investigate the support for FEAT_PMUv3p5 and
> > > native 64bit counters. Someone is bound to build it at some point.
> > 
> > The kernel perf event is implementing 64-bit counters using chained
> > counters. I assume this is already firing an interrupt for the low
> > counter overflow; we might need to just hook into that, investigating...

We probably only enable the overflow interrupt on the odd counter, and
not the even one (which is why you request chained counters the first
place).

And perf wouldn't call us back anyway, as we described the counter as
64bit.

> Additionally, given that the kernel is already emulating 64-bit
> counters, can KVM just expose FEAT_PMUv3p5? Assuming all the other new
> features can be emulated.

This is what I suggested above. Although it can only happen on a
system that already supports FEAT_PMU3p4, as PMMIR_EL1 is not defined
before that (and FEAT_PMU3p5 implies 3p4).

It also remains that we need to *properly* emulate chained counters,
which means not handling them as 64bit counters in perf, but as a
32bit counter and a carry (exactly like the pseudocode does).

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-21 13:43               ` Marc Zyngier
@ 2022-07-22 21:53                 ` Ricardo Koller
  -1 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-22 21:53 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvm, kvmarm, andrew.jones, alexandru.elisei, eric.auger,
	oliver.upton, reijiw

On Thu, Jul 21, 2022 at 02:43:50PM +0100, Marc Zyngier wrote:
> On Wed, 20 Jul 2022 22:26:09 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Wed, Jul 20, 2022 at 02:17:09PM -0700, Ricardo Koller wrote:
> > > On Wed, Jul 20, 2022 at 10:45:20AM +0100, Marc Zyngier wrote:
> > > > On Wed, 20 Jul 2022 09:40:01 +0100,
> > > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > > 
> > > > > On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> > > > > > On Mon, 18 Jul 2022 16:49:10 +0100,
> > > > > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > > > > 
> > > > > > > A chained event overflowing on the low counter can set the overflow flag
> > > > > > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > > > > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > > > > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > > > > > overflow.
> > > > > > 
> > > > > > Isn't this indicative of a bug in the KVM emulation? To be honest, the
> > > > > > pseudocode looks odd. It says:
> > > > > > 
> > > > > > <quote>
> > > > > > 	if old_value<64:ovflw> != new_value<64:ovflw> then
> > > > > > 	    PMOVSSET_EL0<idx> = '1';
> > > > > > 	    PMOVSCLR_EL0<idx> = '1';
> > > > > > </quote>
> > > > > > 
> > > > > > which I find remarkably ambiguous. Is this setting and clearing the
> > > > > > overflow bit? Or setting it in the single register that backs the two
> > > > > > accessors in whatever way it can?
> > > > > > 
> > > > > > If it is the second interpretation that is correct, then KVM
> > > > > > definitely needs fixing
> > > > > 
> > > > > I think it's the second, as those two "= '1'" apply to the non-chained
> > > > > counters case as well, which should definitely set the bit in PMOVSSET.
> > > > > 
> > > > > > (though this looks pretty involved for
> > > > > > anything that isn't a SWINC event).
> > > > > 
> > > > > Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
> > > > > SWINC, but a non-SWINC event is implemented as a single 64-bit perf
> > > > > event.
> > > > 
> > > > Indeed. Which means we need to de-optimise chained counters to being
> > > > 32bit events, which is pretty annoying (for rapidly firing events, the
> > > > interrupt rate is going to be significantly higher).
> > > > 
> > > > I guess we should also investigate the support for FEAT_PMUv3p5 and
> > > > native 64bit counters. Someone is bound to build it at some point.
> > > 
> > > The kernel perf event is implementing 64-bit counters using chained
> > > counters. I assume this is already firing an interrupt for the low
> > > counter overflow; we might need to just hook into that, investigating...
> 
> We probably only enable the overflow interrupt on the odd counter, and
> not the even one (which is why you request chained counters the first
> place).
> 
> And perf wouldn't call us back anyway, as we described the counter as
> 64bit.
> 
> > Additionally, given that the kernel is already emulating 64-bit
> > counters, can KVM just expose FEAT_PMUv3p5? Assuming all the other new
> > features can be emulated.
> 
> This is what I suggested above. Although it can only happen on a
> system that already supports FEAT_PMU3p4, as PMMIR_EL1 is not defined
> before that (and FEAT_PMU3p5 implies 3p4).
> 
> It also remains that we need to *properly* emulate chained counters,
> which means not handling them as 64bit counters in perf, but as a
> 32bit counter and a carry (exactly like the pseudocode does).
> 

Got it, thanks.

Which brings me to what to do with this test. Should it be fixed for
bare-metal by ignoring the PMOVSSET check? or should it actually check
for PMOVSSET=1 and fail on KVM until KVM gets fixed?

Thanks,
Ricardo

> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-22 21:53                 ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-22 21:53 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kvm, andrew.jones, kvmarm

On Thu, Jul 21, 2022 at 02:43:50PM +0100, Marc Zyngier wrote:
> On Wed, 20 Jul 2022 22:26:09 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > On Wed, Jul 20, 2022 at 02:17:09PM -0700, Ricardo Koller wrote:
> > > On Wed, Jul 20, 2022 at 10:45:20AM +0100, Marc Zyngier wrote:
> > > > On Wed, 20 Jul 2022 09:40:01 +0100,
> > > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > > 
> > > > > On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> > > > > > On Mon, 18 Jul 2022 16:49:10 +0100,
> > > > > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > > > > 
> > > > > > > A chained event overflowing on the low counter can set the overflow flag
> > > > > > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > > > > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > > > > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > > > > > overflow.
> > > > > > 
> > > > > > Isn't this indicative of a bug in the KVM emulation? To be honest, the
> > > > > > pseudocode looks odd. It says:
> > > > > > 
> > > > > > <quote>
> > > > > > 	if old_value<64:ovflw> != new_value<64:ovflw> then
> > > > > > 	    PMOVSSET_EL0<idx> = '1';
> > > > > > 	    PMOVSCLR_EL0<idx> = '1';
> > > > > > </quote>
> > > > > > 
> > > > > > which I find remarkably ambiguous. Is this setting and clearing the
> > > > > > overflow bit? Or setting it in the single register that backs the two
> > > > > > accessors in whatever way it can?
> > > > > > 
> > > > > > If it is the second interpretation that is correct, then KVM
> > > > > > definitely needs fixing
> > > > > 
> > > > > I think it's the second, as those two "= '1'" apply to the non-chained
> > > > > counters case as well, which should definitely set the bit in PMOVSSET.
> > > > > 
> > > > > > (though this looks pretty involved for
> > > > > > anything that isn't a SWINC event).
> > > > > 
> > > > > Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
> > > > > SWINC, but a non-SWINC event is implemented as a single 64-bit perf
> > > > > event.
> > > > 
> > > > Indeed. Which means we need to de-optimise chained counters to being
> > > > 32bit events, which is pretty annoying (for rapidly firing events, the
> > > > interrupt rate is going to be significantly higher).
> > > > 
> > > > I guess we should also investigate the support for FEAT_PMUv3p5 and
> > > > native 64bit counters. Someone is bound to build it at some point.
> > > 
> > > The kernel perf event is implementing 64-bit counters using chained
> > > counters. I assume this is already firing an interrupt for the low
> > > counter overflow; we might need to just hook into that, investigating...
> 
> We probably only enable the overflow interrupt on the odd counter, and
> not the even one (which is why you request chained counters the first
> place).
> 
> And perf wouldn't call us back anyway, as we described the counter as
> 64bit.
> 
> > Additionally, given that the kernel is already emulating 64-bit
> > counters, can KVM just expose FEAT_PMUv3p5? Assuming all the other new
> > features can be emulated.
> 
> This is what I suggested above. Although it can only happen on a
> system that already supports FEAT_PMU3p4, as PMMIR_EL1 is not defined
> before that (and FEAT_PMU3p5 implies 3p4).
> 
> It also remains that we need to *properly* emulate chained counters,
> which means not handling them as 64bit counters in perf, but as a
> 32bit counter and a carry (exactly like the pseudocode does).
> 

Got it, thanks.

Which brings me to what to do with this test. Should it be fixed for
bare-metal by ignoring the PMOVSSET check? or should it actually check
for PMOVSSET=1 and fail on KVM until KVM gets fixed?

Thanks,
Ricardo

> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-22 21:53                 ` Ricardo Koller
@ 2022-07-23  7:59                   ` Andrew Jones
  -1 siblings, 0 replies; 48+ messages in thread
From: Andrew Jones @ 2022-07-23  7:59 UTC (permalink / raw)
  To: Ricardo Koller; +Cc: Marc Zyngier, kvm, kvmarm

On Fri, Jul 22, 2022 at 02:53:20PM -0700, Ricardo Koller wrote:
> 
> Which brings me to what to do with this test. Should it be fixed for
> bare-metal by ignoring the PMOVSSET check? or should it actually check
> for PMOVSSET=1 and fail on KVM until KVM gets fixed?
>

Hi Ricardo,

Please write the test per the spec. Failures pointed out in kvm-unit-tests
are great, when the tests are written correctly, since it means it's
doing its job :-)

If some CI somewhere starts blocking builds due to the failure, then there
are ways to skip the test. Unfortunately the easiest way is usually the
oversized hammer of skipping every unittests.cfg entry that fails. To do
better, either the CI needs to be taught about all the subtest failures it
can ignore or the test code needs some work to allow silencing known
failures. For the test code, refactoring to isolate the test into it's own
unittests.cfg entry and then skipping that entry is one way, but probably
won't work in this case, since the overflow checks are scattered. Another
way is to guard all the overflow checks with a variable which can be set
with a command line parameter or environment variable. Eventually, when
the KVM bug is fixed, the guard variable could be forced off for kernel
versions >= the version the fix is merged. The kernel version can be
detected in the unit test by looking at the KERNEL_* environment
variables.

Thanks,
drew

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-23  7:59                   ` Andrew Jones
  0 siblings, 0 replies; 48+ messages in thread
From: Andrew Jones @ 2022-07-23  7:59 UTC (permalink / raw)
  To: Ricardo Koller; +Cc: Marc Zyngier, kvmarm, kvm

On Fri, Jul 22, 2022 at 02:53:20PM -0700, Ricardo Koller wrote:
> 
> Which brings me to what to do with this test. Should it be fixed for
> bare-metal by ignoring the PMOVSSET check? or should it actually check
> for PMOVSSET=1 and fail on KVM until KVM gets fixed?
>

Hi Ricardo,

Please write the test per the spec. Failures pointed out in kvm-unit-tests
are great, when the tests are written correctly, since it means it's
doing its job :-)

If some CI somewhere starts blocking builds due to the failure, then there
are ways to skip the test. Unfortunately the easiest way is usually the
oversized hammer of skipping every unittests.cfg entry that fails. To do
better, either the CI needs to be taught about all the subtest failures it
can ignore or the test code needs some work to allow silencing known
failures. For the test code, refactoring to isolate the test into it's own
unittests.cfg entry and then skipping that entry is one way, but probably
won't work in this case, since the overflow checks are scattered. Another
way is to guard all the overflow checks with a variable which can be set
with a command line parameter or environment variable. Eventually, when
the KVM bug is fixed, the guard variable could be forced off for kernel
versions >= the version the fix is merged. The kernel version can be
detected in the unit test by looking at the KERNEL_* environment
variables.

Thanks,
drew
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-23  7:59                   ` Andrew Jones
@ 2022-07-24  9:40                     ` Marc Zyngier
  -1 siblings, 0 replies; 48+ messages in thread
From: Marc Zyngier @ 2022-07-24  9:40 UTC (permalink / raw)
  To: Andrew Jones; +Cc: Ricardo Koller, kvm, kvmarm

On Sat, 23 Jul 2022 08:59:55 +0100,
Andrew Jones <andrew.jones@linux.dev> wrote:
> 
> On Fri, Jul 22, 2022 at 02:53:20PM -0700, Ricardo Koller wrote:
> > 
> > Which brings me to what to do with this test. Should it be fixed for
> > bare-metal by ignoring the PMOVSSET check? or should it actually check
> > for PMOVSSET=1 and fail on KVM until KVM gets fixed?
> >
> 
> Hi Ricardo,
> 
> Please write the test per the spec. Failures pointed out in kvm-unit-tests
> are great, when the tests are written correctly, since it means it's
> doing its job :-)

Agreed. This is a bug, and bugs are to be fixed. The fact that it will
flare up and annoy people is a good incentive to fix the kernel!

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-24  9:40                     ` Marc Zyngier
  0 siblings, 0 replies; 48+ messages in thread
From: Marc Zyngier @ 2022-07-24  9:40 UTC (permalink / raw)
  To: Andrew Jones; +Cc: kvmarm, kvm

On Sat, 23 Jul 2022 08:59:55 +0100,
Andrew Jones <andrew.jones@linux.dev> wrote:
> 
> On Fri, Jul 22, 2022 at 02:53:20PM -0700, Ricardo Koller wrote:
> > 
> > Which brings me to what to do with this test. Should it be fixed for
> > bare-metal by ignoring the PMOVSSET check? or should it actually check
> > for PMOVSSET=1 and fail on KVM until KVM gets fixed?
> >
> 
> Hi Ricardo,
> 
> Please write the test per the spec. Failures pointed out in kvm-unit-tests
> are great, when the tests are written correctly, since it means it's
> doing its job :-)

Agreed. This is a bug, and bugs are to be fixed. The fact that it will
flare up and annoy people is a good incentive to fix the kernel!

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-24  9:40                     ` Marc Zyngier
@ 2022-07-27  2:29                       ` Ricardo Koller
  -1 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-27  2:29 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: Andrew Jones, kvm, kvmarm

On Sun, Jul 24, 2022 at 10:40:20AM +0100, Marc Zyngier wrote:
> On Sat, 23 Jul 2022 08:59:55 +0100,
> Andrew Jones <andrew.jones@linux.dev> wrote:
> > 
> > On Fri, Jul 22, 2022 at 02:53:20PM -0700, Ricardo Koller wrote:
> > > 
> > > Which brings me to what to do with this test. Should it be fixed for
> > > bare-metal by ignoring the PMOVSSET check? or should it actually check
> > > for PMOVSSET=1 and fail on KVM until KVM gets fixed?
> > >
> > 
> > Hi Ricardo,
> > 
> > Please write the test per the spec. Failures pointed out in kvm-unit-tests
> > are great, when the tests are written correctly, since it means it's
> > doing its job :-)
> 
> Agreed. This is a bug, and bugs are to be fixed. The fact that it will
> flare up and annoy people is a good incentive to fix the kernel!
> 

Sounds good, thank you both. Will send V2 with the proper behavior then.

Thanks,
Ricardo

> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-27  2:29                       ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-07-27  2:29 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kvmarm, kvm, Andrew Jones

On Sun, Jul 24, 2022 at 10:40:20AM +0100, Marc Zyngier wrote:
> On Sat, 23 Jul 2022 08:59:55 +0100,
> Andrew Jones <andrew.jones@linux.dev> wrote:
> > 
> > On Fri, Jul 22, 2022 at 02:53:20PM -0700, Ricardo Koller wrote:
> > > 
> > > Which brings me to what to do with this test. Should it be fixed for
> > > bare-metal by ignoring the PMOVSSET check? or should it actually check
> > > for PMOVSSET=1 and fail on KVM until KVM gets fixed?
> > >
> > 
> > Hi Ricardo,
> > 
> > Please write the test per the spec. Failures pointed out in kvm-unit-tests
> > are great, when the tests are written correctly, since it means it's
> > doing its job :-)
> 
> Agreed. This is a bug, and bugs are to be fixed. The fact that it will
> flare up and annoy people is a good incentive to fix the kernel!
> 

Sounds good, thank you both. Will send V2 with the proper behavior then.

Thanks,
Ricardo

> Thanks,
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-18 15:49   ` Ricardo Koller
@ 2022-07-30 12:47     ` Marc Zyngier
  -1 siblings, 0 replies; 48+ messages in thread
From: Marc Zyngier @ 2022-07-30 12:47 UTC (permalink / raw)
  To: Ricardo Koller
  Cc: kvm, kvmarm, drjones, alexandru.elisei, eric.auger, oliver.upton, reijiw

Hi Ricardo,

On Mon, 18 Jul 2022 16:49:10 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> A chained event overflowing on the low counter can set the overflow flag
> in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> overflow.
> 
> The pmu chain tests fail on bare metal when checking the overflow flag
> of the low counter _not_ being set on overflow.  Fix by removing the
> checks.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arm/pmu.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index a7899c3c..4f2c5096 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -581,7 +581,6 @@ static void test_chained_counters(void)
>  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
>  
>  	report(read_regn_el0(pmevcntr, 1) == 1, "CHAIN counter #1 incremented");
> -	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #1");
>  
>  	/* test 64b overflow */
>  
> @@ -593,7 +592,7 @@ static void test_chained_counters(void)
>  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
>  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
>  	report(read_regn_el0(pmevcntr, 1) == 2, "CHAIN counter #1 set to 2");
> -	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #2");
> +	report((read_sysreg(pmovsclr_el0) & 0x2) == 0, "no overflow recorded for chained incr #2");
>  
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
>  	write_regn_el0(pmevcntr, 1, ALL_SET);
> @@ -601,7 +600,7 @@ static void test_chained_counters(void)
>  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
>  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
>  	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> -	report(read_sysreg(pmovsclr_el0) == 0x2, "overflow on chain counter");
> +	report(read_sysreg(pmovsclr_el0) & 0x2, "overflow on chain counter");
>  }
>  
>  static void test_chained_sw_incr(void)
> @@ -626,10 +625,10 @@ static void test_chained_sw_incr(void)
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
>  
> -	report(!read_sysreg(pmovsclr_el0) && (read_regn_el0(pmevcntr, 1) == 1),
> -		"no overflow and chain counter incremented after 100 SW_INCR/CHAIN");
> +	report(read_regn_el0(pmevcntr, 1) == 1,
> +			"no chain counter incremented after 100 SW_INCR/CHAIN");
>  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> -		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> +			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
>  
>  	/* 64b SW_INCR and overflow on CHAIN counter*/
>  	pmu_reset();
> @@ -644,7 +643,7 @@ static void test_chained_sw_incr(void)
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
>  
> -	report((read_sysreg(pmovsclr_el0) == 0x2) &&
> +	report((read_sysreg(pmovsclr_el0) & 0x2) &&
>  		(read_regn_el0(pmevcntr, 1) == 0) &&
>  		(read_regn_el0(pmevcntr, 0) == 84),
>  		"overflow on chain counter and expected values after 100 SW_INCR/CHAIN");
> @@ -727,8 +726,8 @@ static void test_chain_promotion(void)
>  	report_info("MEM_ACCESS counter #0 has value 0x%lx",
>  		    read_regn_el0(pmevcntr, 0));
>  
> -	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> -		"CHAIN counter enabled: CHAIN counter was incremented and no overflow");
> +	report((read_regn_el0(pmevcntr, 1) == 1),
> +		"CHAIN counter enabled: CHAIN counter was incremented");
>  
>  	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
>  		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> @@ -755,8 +754,8 @@ static void test_chain_promotion(void)
>  	report_info("MEM_ACCESS counter #0 has value 0x%lx",
>  		    read_regn_el0(pmevcntr, 0));
>  
> -	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> -		"32b->64b: CHAIN counter incremented and no overflow");
> +	report((read_regn_el0(pmevcntr, 1) == 1),
> +		"32b->64b: CHAIN counter incremented");
>  
>  	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
>  		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));

I'm looking at fixing KVM to match this (see the binch of hacks at
[1]), and still getting a couple of failures in the PMU overflow tests
despite my best effort to fix the code:

$ ./arm-run  arm/pmu.flat --append pmu-overflow-interrupt
/usr/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 --append pmu-overflow-interrupt # -initrd /tmp/tmp.RQ6FmkvXay
INFO: PMU version: 0x1
INFO: PMU implementer/ID code: 0x41("A")/0x3
INFO: Implements 6 event counters
PASS: pmu: pmu-overflow-interrupt: no overflow interrupt after preset
PASS: pmu: pmu-overflow-interrupt: no overflow interrupt after counting
INFO: pmu: pmu-overflow-interrupt: overflow=0x0
PASS: pmu: pmu-overflow-interrupt: overflow interrupts expected on #0 and #1
FAIL: pmu: pmu-overflow-interrupt: no overflow interrupt expected on 32b boundary
FAIL: pmu: pmu-overflow-interrupt: expect overflow interrupt on odd counter
SUMMARY: 5 tests, 2 unexpected failures

Looking at the kut code, I'm wondering whether you're still missing a
couple of extra fixes such as:

diff --git a/arm/pmu.c b/arm/pmu.c
index 4f2c5096..e0b9f71a 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -861,8 +861,8 @@ static void test_overflow_interrupt(void)
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
 	isb();
 	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
-	report(expect_interrupts(0),
-		"no overflow interrupt expected on 32b boundary");
+	report(expect_interrupts(1),
+		"expect overflow interrupt on 32b counter");
 
 	/* overflow on odd counter */
 	pmu_reset_stats();
@@ -870,8 +870,8 @@ static void test_overflow_interrupt(void)
 	write_regn_el0(pmevcntr, 1, ALL_SET);
 	isb();
 	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
-	report(expect_interrupts(0x2),
-		"expect overflow interrupt on odd counter");
+	report(expect_interrupts(0x3),
+		"expect overflow interrupt on even+odd counters");
 }
 #endif
 
With that, all PMU tests pass. Am I missing something? Did you notice
these failing on HW?

Thanks,

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu-chained

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply related	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-30 12:47     ` Marc Zyngier
  0 siblings, 0 replies; 48+ messages in thread
From: Marc Zyngier @ 2022-07-30 12:47 UTC (permalink / raw)
  To: Ricardo Koller; +Cc: drjones, kvm, kvmarm

Hi Ricardo,

On Mon, 18 Jul 2022 16:49:10 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> A chained event overflowing on the low counter can set the overflow flag
> in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> overflow.
> 
> The pmu chain tests fail on bare metal when checking the overflow flag
> of the low counter _not_ being set on overflow.  Fix by removing the
> checks.
> 
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arm/pmu.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index a7899c3c..4f2c5096 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -581,7 +581,6 @@ static void test_chained_counters(void)
>  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
>  
>  	report(read_regn_el0(pmevcntr, 1) == 1, "CHAIN counter #1 incremented");
> -	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #1");
>  
>  	/* test 64b overflow */
>  
> @@ -593,7 +592,7 @@ static void test_chained_counters(void)
>  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
>  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
>  	report(read_regn_el0(pmevcntr, 1) == 2, "CHAIN counter #1 set to 2");
> -	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #2");
> +	report((read_sysreg(pmovsclr_el0) & 0x2) == 0, "no overflow recorded for chained incr #2");
>  
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
>  	write_regn_el0(pmevcntr, 1, ALL_SET);
> @@ -601,7 +600,7 @@ static void test_chained_counters(void)
>  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
>  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
>  	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> -	report(read_sysreg(pmovsclr_el0) == 0x2, "overflow on chain counter");
> +	report(read_sysreg(pmovsclr_el0) & 0x2, "overflow on chain counter");
>  }
>  
>  static void test_chained_sw_incr(void)
> @@ -626,10 +625,10 @@ static void test_chained_sw_incr(void)
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
>  
> -	report(!read_sysreg(pmovsclr_el0) && (read_regn_el0(pmevcntr, 1) == 1),
> -		"no overflow and chain counter incremented after 100 SW_INCR/CHAIN");
> +	report(read_regn_el0(pmevcntr, 1) == 1,
> +			"no chain counter incremented after 100 SW_INCR/CHAIN");
>  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> -		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> +			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
>  
>  	/* 64b SW_INCR and overflow on CHAIN counter*/
>  	pmu_reset();
> @@ -644,7 +643,7 @@ static void test_chained_sw_incr(void)
>  	for (i = 0; i < 100; i++)
>  		write_sysreg(0x1, pmswinc_el0);
>  
> -	report((read_sysreg(pmovsclr_el0) == 0x2) &&
> +	report((read_sysreg(pmovsclr_el0) & 0x2) &&
>  		(read_regn_el0(pmevcntr, 1) == 0) &&
>  		(read_regn_el0(pmevcntr, 0) == 84),
>  		"overflow on chain counter and expected values after 100 SW_INCR/CHAIN");
> @@ -727,8 +726,8 @@ static void test_chain_promotion(void)
>  	report_info("MEM_ACCESS counter #0 has value 0x%lx",
>  		    read_regn_el0(pmevcntr, 0));
>  
> -	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> -		"CHAIN counter enabled: CHAIN counter was incremented and no overflow");
> +	report((read_regn_el0(pmevcntr, 1) == 1),
> +		"CHAIN counter enabled: CHAIN counter was incremented");
>  
>  	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
>  		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> @@ -755,8 +754,8 @@ static void test_chain_promotion(void)
>  	report_info("MEM_ACCESS counter #0 has value 0x%lx",
>  		    read_regn_el0(pmevcntr, 0));
>  
> -	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> -		"32b->64b: CHAIN counter incremented and no overflow");
> +	report((read_regn_el0(pmevcntr, 1) == 1),
> +		"32b->64b: CHAIN counter incremented");
>  
>  	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
>  		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));

I'm looking at fixing KVM to match this (see the binch of hacks at
[1]), and still getting a couple of failures in the PMU overflow tests
despite my best effort to fix the code:

$ ./arm-run  arm/pmu.flat --append pmu-overflow-interrupt
/usr/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 --append pmu-overflow-interrupt # -initrd /tmp/tmp.RQ6FmkvXay
INFO: PMU version: 0x1
INFO: PMU implementer/ID code: 0x41("A")/0x3
INFO: Implements 6 event counters
PASS: pmu: pmu-overflow-interrupt: no overflow interrupt after preset
PASS: pmu: pmu-overflow-interrupt: no overflow interrupt after counting
INFO: pmu: pmu-overflow-interrupt: overflow=0x0
PASS: pmu: pmu-overflow-interrupt: overflow interrupts expected on #0 and #1
FAIL: pmu: pmu-overflow-interrupt: no overflow interrupt expected on 32b boundary
FAIL: pmu: pmu-overflow-interrupt: expect overflow interrupt on odd counter
SUMMARY: 5 tests, 2 unexpected failures

Looking at the kut code, I'm wondering whether you're still missing a
couple of extra fixes such as:

diff --git a/arm/pmu.c b/arm/pmu.c
index 4f2c5096..e0b9f71a 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -861,8 +861,8 @@ static void test_overflow_interrupt(void)
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
 	isb();
 	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
-	report(expect_interrupts(0),
-		"no overflow interrupt expected on 32b boundary");
+	report(expect_interrupts(1),
+		"expect overflow interrupt on 32b counter");
 
 	/* overflow on odd counter */
 	pmu_reset_stats();
@@ -870,8 +870,8 @@ static void test_overflow_interrupt(void)
 	write_regn_el0(pmevcntr, 1, ALL_SET);
 	isb();
 	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
-	report(expect_interrupts(0x2),
-		"expect overflow interrupt on odd counter");
+	report(expect_interrupts(0x3),
+		"expect overflow interrupt on even+odd counters");
 }
 #endif
 
With that, all PMU tests pass. Am I missing something? Did you notice
these failing on HW?

Thanks,

	M.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu-chained

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-30 12:47     ` Marc Zyngier
@ 2022-07-30 12:52       ` Marc Zyngier
  -1 siblings, 0 replies; 48+ messages in thread
From: Marc Zyngier @ 2022-07-30 12:52 UTC (permalink / raw)
  To: Ricardo Koller
  Cc: kvm, kvmarm, Andrew Jones, alexandru.elisei, eric.auger,
	oliver.upton, reijiw

Crumbs... With Drew's new email this time.

On Sat, 30 Jul 2022 13:47:14 +0100,
Marc Zyngier <maz@kernel.org> wrote:
> 
> Hi Ricardo,
> 
> On Mon, 18 Jul 2022 16:49:10 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > A chained event overflowing on the low counter can set the overflow flag
> > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > overflow.
> > 
> > The pmu chain tests fail on bare metal when checking the overflow flag
> > of the low counter _not_ being set on overflow.  Fix by removing the
> > checks.
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arm/pmu.c | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index a7899c3c..4f2c5096 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -581,7 +581,6 @@ static void test_chained_counters(void)
> >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> >  
> >  	report(read_regn_el0(pmevcntr, 1) == 1, "CHAIN counter #1 incremented");
> > -	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #1");
> >  
> >  	/* test 64b overflow */
> >  
> > @@ -593,7 +592,7 @@ static void test_chained_counters(void)
> >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> >  	report(read_regn_el0(pmevcntr, 1) == 2, "CHAIN counter #1 set to 2");
> > -	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #2");
> > +	report((read_sysreg(pmovsclr_el0) & 0x2) == 0, "no overflow recorded for chained incr #2");
> >  
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >  	write_regn_el0(pmevcntr, 1, ALL_SET);
> > @@ -601,7 +600,7 @@ static void test_chained_counters(void)
> >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> >  	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> > -	report(read_sysreg(pmovsclr_el0) == 0x2, "overflow on chain counter");
> > +	report(read_sysreg(pmovsclr_el0) & 0x2, "overflow on chain counter");
> >  }
> >  
> >  static void test_chained_sw_incr(void)
> > @@ -626,10 +625,10 @@ static void test_chained_sw_incr(void)
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> > -	report(!read_sysreg(pmovsclr_el0) && (read_regn_el0(pmevcntr, 1) == 1),
> > -		"no overflow and chain counter incremented after 100 SW_INCR/CHAIN");
> > +	report(read_regn_el0(pmevcntr, 1) == 1,
> > +			"no chain counter incremented after 100 SW_INCR/CHAIN");
> >  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> > -		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > +			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> >  
> >  	/* 64b SW_INCR and overflow on CHAIN counter*/
> >  	pmu_reset();
> > @@ -644,7 +643,7 @@ static void test_chained_sw_incr(void)
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> > -	report((read_sysreg(pmovsclr_el0) == 0x2) &&
> > +	report((read_sysreg(pmovsclr_el0) & 0x2) &&
> >  		(read_regn_el0(pmevcntr, 1) == 0) &&
> >  		(read_regn_el0(pmevcntr, 0) == 84),
> >  		"overflow on chain counter and expected values after 100 SW_INCR/CHAIN");
> > @@ -727,8 +726,8 @@ static void test_chain_promotion(void)
> >  	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> >  		    read_regn_el0(pmevcntr, 0));
> >  
> > -	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> > -		"CHAIN counter enabled: CHAIN counter was incremented and no overflow");
> > +	report((read_regn_el0(pmevcntr, 1) == 1),
> > +		"CHAIN counter enabled: CHAIN counter was incremented");
> >  
> >  	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> >  		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> > @@ -755,8 +754,8 @@ static void test_chain_promotion(void)
> >  	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> >  		    read_regn_el0(pmevcntr, 0));
> >  
> > -	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> > -		"32b->64b: CHAIN counter incremented and no overflow");
> > +	report((read_regn_el0(pmevcntr, 1) == 1),
> > +		"32b->64b: CHAIN counter incremented");
> >  
> >  	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> >  		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> 
> I'm looking at fixing KVM to match this (see the binch of hacks at
> [1]), and still getting a couple of failures in the PMU overflow tests
> despite my best effort to fix the code:
> 
> $ ./arm-run  arm/pmu.flat --append pmu-overflow-interrupt
> /usr/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 --append pmu-overflow-interrupt # -initrd /tmp/tmp.RQ6FmkvXay
> INFO: PMU version: 0x1
> INFO: PMU implementer/ID code: 0x41("A")/0x3
> INFO: Implements 6 event counters
> PASS: pmu: pmu-overflow-interrupt: no overflow interrupt after preset
> PASS: pmu: pmu-overflow-interrupt: no overflow interrupt after counting
> INFO: pmu: pmu-overflow-interrupt: overflow=0x0
> PASS: pmu: pmu-overflow-interrupt: overflow interrupts expected on #0 and #1
> FAIL: pmu: pmu-overflow-interrupt: no overflow interrupt expected on 32b boundary
> FAIL: pmu: pmu-overflow-interrupt: expect overflow interrupt on odd counter
> SUMMARY: 5 tests, 2 unexpected failures
> 
> Looking at the kut code, I'm wondering whether you're still missing a
> couple of extra fixes such as:
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 4f2c5096..e0b9f71a 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -861,8 +861,8 @@ static void test_overflow_interrupt(void)
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
>  	isb();
>  	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> -	report(expect_interrupts(0),
> -		"no overflow interrupt expected on 32b boundary");
> +	report(expect_interrupts(1),
> +		"expect overflow interrupt on 32b counter");
>  
>  	/* overflow on odd counter */
>  	pmu_reset_stats();
> @@ -870,8 +870,8 @@ static void test_overflow_interrupt(void)
>  	write_regn_el0(pmevcntr, 1, ALL_SET);
>  	isb();
>  	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
> -	report(expect_interrupts(0x2),
> -		"expect overflow interrupt on odd counter");
> +	report(expect_interrupts(0x3),
> +		"expect overflow interrupt on even+odd counters");
>  }
>  #endif
>  
> With that, all PMU tests pass. Am I missing something? Did you notice
> these failing on HW?
> 
> Thanks,
> 
> 	M.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu-chained
> 
> -- 
> Without deviation from the norm, progress is not possible.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-07-30 12:52       ` Marc Zyngier
  0 siblings, 0 replies; 48+ messages in thread
From: Marc Zyngier @ 2022-07-30 12:52 UTC (permalink / raw)
  To: Ricardo Koller; +Cc: kvm, Andrew Jones, kvmarm

Crumbs... With Drew's new email this time.

On Sat, 30 Jul 2022 13:47:14 +0100,
Marc Zyngier <maz@kernel.org> wrote:
> 
> Hi Ricardo,
> 
> On Mon, 18 Jul 2022 16:49:10 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> > 
> > A chained event overflowing on the low counter can set the overflow flag
> > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > overflow.
> > 
> > The pmu chain tests fail on bare metal when checking the overflow flag
> > of the low counter _not_ being set on overflow.  Fix by removing the
> > checks.
> > 
> > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > ---
> >  arm/pmu.c | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index a7899c3c..4f2c5096 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -581,7 +581,6 @@ static void test_chained_counters(void)
> >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> >  
> >  	report(read_regn_el0(pmevcntr, 1) == 1, "CHAIN counter #1 incremented");
> > -	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #1");
> >  
> >  	/* test 64b overflow */
> >  
> > @@ -593,7 +592,7 @@ static void test_chained_counters(void)
> >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> >  	report(read_regn_el0(pmevcntr, 1) == 2, "CHAIN counter #1 set to 2");
> > -	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #2");
> > +	report((read_sysreg(pmovsclr_el0) & 0x2) == 0, "no overflow recorded for chained incr #2");
> >  
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >  	write_regn_el0(pmevcntr, 1, ALL_SET);
> > @@ -601,7 +600,7 @@ static void test_chained_counters(void)
> >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> >  	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> > -	report(read_sysreg(pmovsclr_el0) == 0x2, "overflow on chain counter");
> > +	report(read_sysreg(pmovsclr_el0) & 0x2, "overflow on chain counter");
> >  }
> >  
> >  static void test_chained_sw_incr(void)
> > @@ -626,10 +625,10 @@ static void test_chained_sw_incr(void)
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> > -	report(!read_sysreg(pmovsclr_el0) && (read_regn_el0(pmevcntr, 1) == 1),
> > -		"no overflow and chain counter incremented after 100 SW_INCR/CHAIN");
> > +	report(read_regn_el0(pmevcntr, 1) == 1,
> > +			"no chain counter incremented after 100 SW_INCR/CHAIN");
> >  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> > -		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > +			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> >  
> >  	/* 64b SW_INCR and overflow on CHAIN counter*/
> >  	pmu_reset();
> > @@ -644,7 +643,7 @@ static void test_chained_sw_incr(void)
> >  	for (i = 0; i < 100; i++)
> >  		write_sysreg(0x1, pmswinc_el0);
> >  
> > -	report((read_sysreg(pmovsclr_el0) == 0x2) &&
> > +	report((read_sysreg(pmovsclr_el0) & 0x2) &&
> >  		(read_regn_el0(pmevcntr, 1) == 0) &&
> >  		(read_regn_el0(pmevcntr, 0) == 84),
> >  		"overflow on chain counter and expected values after 100 SW_INCR/CHAIN");
> > @@ -727,8 +726,8 @@ static void test_chain_promotion(void)
> >  	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> >  		    read_regn_el0(pmevcntr, 0));
> >  
> > -	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> > -		"CHAIN counter enabled: CHAIN counter was incremented and no overflow");
> > +	report((read_regn_el0(pmevcntr, 1) == 1),
> > +		"CHAIN counter enabled: CHAIN counter was incremented");
> >  
> >  	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> >  		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> > @@ -755,8 +754,8 @@ static void test_chain_promotion(void)
> >  	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> >  		    read_regn_el0(pmevcntr, 0));
> >  
> > -	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> > -		"32b->64b: CHAIN counter incremented and no overflow");
> > +	report((read_regn_el0(pmevcntr, 1) == 1),
> > +		"32b->64b: CHAIN counter incremented");
> >  
> >  	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> >  		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> 
> I'm looking at fixing KVM to match this (see the binch of hacks at
> [1]), and still getting a couple of failures in the PMU overflow tests
> despite my best effort to fix the code:
> 
> $ ./arm-run  arm/pmu.flat --append pmu-overflow-interrupt
> /usr/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 --append pmu-overflow-interrupt # -initrd /tmp/tmp.RQ6FmkvXay
> INFO: PMU version: 0x1
> INFO: PMU implementer/ID code: 0x41("A")/0x3
> INFO: Implements 6 event counters
> PASS: pmu: pmu-overflow-interrupt: no overflow interrupt after preset
> PASS: pmu: pmu-overflow-interrupt: no overflow interrupt after counting
> INFO: pmu: pmu-overflow-interrupt: overflow=0x0
> PASS: pmu: pmu-overflow-interrupt: overflow interrupts expected on #0 and #1
> FAIL: pmu: pmu-overflow-interrupt: no overflow interrupt expected on 32b boundary
> FAIL: pmu: pmu-overflow-interrupt: expect overflow interrupt on odd counter
> SUMMARY: 5 tests, 2 unexpected failures
> 
> Looking at the kut code, I'm wondering whether you're still missing a
> couple of extra fixes such as:
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 4f2c5096..e0b9f71a 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -861,8 +861,8 @@ static void test_overflow_interrupt(void)
>  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
>  	isb();
>  	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> -	report(expect_interrupts(0),
> -		"no overflow interrupt expected on 32b boundary");
> +	report(expect_interrupts(1),
> +		"expect overflow interrupt on 32b counter");
>  
>  	/* overflow on odd counter */
>  	pmu_reset_stats();
> @@ -870,8 +870,8 @@ static void test_overflow_interrupt(void)
>  	write_regn_el0(pmevcntr, 1, ALL_SET);
>  	isb();
>  	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
> -	report(expect_interrupts(0x2),
> -		"expect overflow interrupt on odd counter");
> +	report(expect_interrupts(0x3),
> +		"expect overflow interrupt on even+odd counters");
>  }
>  #endif
>  
> With that, all PMU tests pass. Am I missing something? Did you notice
> these failing on HW?
> 
> Thanks,
> 
> 	M.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu-chained
> 
> -- 
> Without deviation from the norm, progress is not possible.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
  2022-07-30 12:52       ` Marc Zyngier
@ 2022-08-01 19:15         ` Ricardo Koller
  -1 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-08-01 19:15 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvm, kvmarm, Andrew Jones, alexandru.elisei, eric.auger,
	oliver.upton, reijiw

On Sat, Jul 30, 2022 at 01:52:38PM +0100, Marc Zyngier wrote:
> Crumbs... With Drew's new email this time.
> 
> On Sat, 30 Jul 2022 13:47:14 +0100,
> Marc Zyngier <maz@kernel.org> wrote:
> > 
> > Hi Ricardo,
> > 
> > On Mon, 18 Jul 2022 16:49:10 +0100,
> > Ricardo Koller <ricarkol@google.com> wrote:
> > > 
> > > A chained event overflowing on the low counter can set the overflow flag
> > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > overflow.
> > > 
> > > The pmu chain tests fail on bare metal when checking the overflow flag
> > > of the low counter _not_ being set on overflow.  Fix by removing the
> > > checks.
> > > 
> > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > ---
> > >  arm/pmu.c | 21 ++++++++++-----------
> > >  1 file changed, 10 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/arm/pmu.c b/arm/pmu.c
> > > index a7899c3c..4f2c5096 100644
> > > --- a/arm/pmu.c
> > > +++ b/arm/pmu.c
> > > @@ -581,7 +581,6 @@ static void test_chained_counters(void)
> > >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > >  
> > >  	report(read_regn_el0(pmevcntr, 1) == 1, "CHAIN counter #1 incremented");
> > > -	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #1");
> > >  
> > >  	/* test 64b overflow */
> > >  
> > > @@ -593,7 +592,7 @@ static void test_chained_counters(void)
> > >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > >  	report(read_regn_el0(pmevcntr, 1) == 2, "CHAIN counter #1 set to 2");
> > > -	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #2");
> > > +	report((read_sysreg(pmovsclr_el0) & 0x2) == 0, "no overflow recorded for chained incr #2");
> > >  
> > >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > >  	write_regn_el0(pmevcntr, 1, ALL_SET);
> > > @@ -601,7 +600,7 @@ static void test_chained_counters(void)
> > >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > >  	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> > > -	report(read_sysreg(pmovsclr_el0) == 0x2, "overflow on chain counter");
> > > +	report(read_sysreg(pmovsclr_el0) & 0x2, "overflow on chain counter");
> > >  }
> > >  
> > >  static void test_chained_sw_incr(void)
> > > @@ -626,10 +625,10 @@ static void test_chained_sw_incr(void)
> > >  	for (i = 0; i < 100; i++)
> > >  		write_sysreg(0x1, pmswinc_el0);
> > >  
> > > -	report(!read_sysreg(pmovsclr_el0) && (read_regn_el0(pmevcntr, 1) == 1),
> > > -		"no overflow and chain counter incremented after 100 SW_INCR/CHAIN");
> > > +	report(read_regn_el0(pmevcntr, 1) == 1,
> > > +			"no chain counter incremented after 100 SW_INCR/CHAIN");
> > >  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> > > -		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > > +			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > >  
> > >  	/* 64b SW_INCR and overflow on CHAIN counter*/
> > >  	pmu_reset();
> > > @@ -644,7 +643,7 @@ static void test_chained_sw_incr(void)
> > >  	for (i = 0; i < 100; i++)
> > >  		write_sysreg(0x1, pmswinc_el0);
> > >  
> > > -	report((read_sysreg(pmovsclr_el0) == 0x2) &&
> > > +	report((read_sysreg(pmovsclr_el0) & 0x2) &&
> > >  		(read_regn_el0(pmevcntr, 1) == 0) &&
> > >  		(read_regn_el0(pmevcntr, 0) == 84),
> > >  		"overflow on chain counter and expected values after 100 SW_INCR/CHAIN");
> > > @@ -727,8 +726,8 @@ static void test_chain_promotion(void)
> > >  	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> > >  		    read_regn_el0(pmevcntr, 0));
> > >  
> > > -	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> > > -		"CHAIN counter enabled: CHAIN counter was incremented and no overflow");
> > > +	report((read_regn_el0(pmevcntr, 1) == 1),
> > > +		"CHAIN counter enabled: CHAIN counter was incremented");
> > >  
> > >  	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> > >  		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> > > @@ -755,8 +754,8 @@ static void test_chain_promotion(void)
> > >  	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> > >  		    read_regn_el0(pmevcntr, 0));
> > >  
> > > -	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> > > -		"32b->64b: CHAIN counter incremented and no overflow");
> > > +	report((read_regn_el0(pmevcntr, 1) == 1),
> > > +		"32b->64b: CHAIN counter incremented");
> > >  
> > >  	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> > >  		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> > 
> > I'm looking at fixing KVM to match this (see the binch of hacks at
> > [1]), and still getting a couple of failures in the PMU overflow tests
> > despite my best effort to fix the code:
> > 
> > $ ./arm-run  arm/pmu.flat --append pmu-overflow-interrupt
> > /usr/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 --append pmu-overflow-interrupt # -initrd /tmp/tmp.RQ6FmkvXay
> > INFO: PMU version: 0x1
> > INFO: PMU implementer/ID code: 0x41("A")/0x3
> > INFO: Implements 6 event counters
> > PASS: pmu: pmu-overflow-interrupt: no overflow interrupt after preset
> > PASS: pmu: pmu-overflow-interrupt: no overflow interrupt after counting
> > INFO: pmu: pmu-overflow-interrupt: overflow=0x0
> > PASS: pmu: pmu-overflow-interrupt: overflow interrupts expected on #0 and #1
> > FAIL: pmu: pmu-overflow-interrupt: no overflow interrupt expected on 32b boundary
> > FAIL: pmu: pmu-overflow-interrupt: expect overflow interrupt on odd counter
> > SUMMARY: 5 tests, 2 unexpected failures
> > 
> > Looking at the kut code, I'm wondering whether you're still missing a
> > couple of extra fixes such as:
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 4f2c5096..e0b9f71a 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -861,8 +861,8 @@ static void test_overflow_interrupt(void)
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >  	isb();
> >  	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > -	report(expect_interrupts(0),
> > -		"no overflow interrupt expected on 32b boundary");
> > +	report(expect_interrupts(1),
> > +		"expect overflow interrupt on 32b counter");
> >  
> >  	/* overflow on odd counter */
> >  	pmu_reset_stats();
> > @@ -870,8 +870,8 @@ static void test_overflow_interrupt(void)
> >  	write_regn_el0(pmevcntr, 1, ALL_SET);
> >  	isb();
> >  	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
> > -	report(expect_interrupts(0x2),
> > -		"expect overflow interrupt on odd counter");
> > +	report(expect_interrupts(0x3),
> > +		"expect overflow interrupt on even+odd counters");
> >  }
> >  #endif
> >  
> > With that, all PMU tests pass. Am I missing something? Did you notice
> > these failing on HW?
> > 

Actually, yes. But, I wasn't 100% sure. I tried a PMU passthrough
prototype on both real HW and the fast-model, and with
test_overflow_interrupt() I see an interrupt overflow hitting EL2. I
wasn't sure whether I should be forwarding it to the guest.

Thanks,
Ricardo

> > Thanks,
> > 
> > 	M.
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu-chained
> > 
> > -- 
> > Without deviation from the norm, progress is not possible.
> 
> -- 
> Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 48+ messages in thread
* Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
@ 2022-08-01 19:15         ` Ricardo Koller
  0 siblings, 0 replies; 48+ messages in thread
From: Ricardo Koller @ 2022-08-01 19:15 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: kvm, Andrew Jones, kvmarm

On Sat, Jul 30, 2022 at 01:52:38PM +0100, Marc Zyngier wrote:
> Crumbs... With Drew's new email this time.
> 
> On Sat, 30 Jul 2022 13:47:14 +0100,
> Marc Zyngier <maz@kernel.org> wrote:
> > 
> > Hi Ricardo,
> > 
> > On Mon, 18 Jul 2022 16:49:10 +0100,
> > Ricardo Koller <ricarkol@google.com> wrote:
> > > 
> > > A chained event overflowing on the low counter can set the overflow flag
> > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > overflow.
> > > 
> > > The pmu chain tests fail on bare metal when checking the overflow flag
> > > of the low counter _not_ being set on overflow.  Fix by removing the
> > > checks.
> > > 
> > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > ---
> > >  arm/pmu.c | 21 ++++++++++-----------
> > >  1 file changed, 10 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/arm/pmu.c b/arm/pmu.c
> > > index a7899c3c..4f2c5096 100644
> > > --- a/arm/pmu.c
> > > +++ b/arm/pmu.c
> > > @@ -581,7 +581,6 @@ static void test_chained_counters(void)
> > >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > >  
> > >  	report(read_regn_el0(pmevcntr, 1) == 1, "CHAIN counter #1 incremented");
> > > -	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #1");
> > >  
> > >  	/* test 64b overflow */
> > >  
> > > @@ -593,7 +592,7 @@ static void test_chained_counters(void)
> > >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > >  	report(read_regn_el0(pmevcntr, 1) == 2, "CHAIN counter #1 set to 2");
> > > -	report(!read_sysreg(pmovsclr_el0), "no overflow recorded for chained incr #2");
> > > +	report((read_sysreg(pmovsclr_el0) & 0x2) == 0, "no overflow recorded for chained incr #2");
> > >  
> > >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> > >  	write_regn_el0(pmevcntr, 1, ALL_SET);
> > > @@ -601,7 +600,7 @@ static void test_chained_counters(void)
> > >  	precise_instrs_loop(22, pmu.pmcr_ro | PMU_PMCR_E);
> > >  	report_info("overflow reg = 0x%lx", read_sysreg(pmovsclr_el0));
> > >  	report(!read_regn_el0(pmevcntr, 1), "CHAIN counter #1 wrapped");
> > > -	report(read_sysreg(pmovsclr_el0) == 0x2, "overflow on chain counter");
> > > +	report(read_sysreg(pmovsclr_el0) & 0x2, "overflow on chain counter");
> > >  }
> > >  
> > >  static void test_chained_sw_incr(void)
> > > @@ -626,10 +625,10 @@ static void test_chained_sw_incr(void)
> > >  	for (i = 0; i < 100; i++)
> > >  		write_sysreg(0x1, pmswinc_el0);
> > >  
> > > -	report(!read_sysreg(pmovsclr_el0) && (read_regn_el0(pmevcntr, 1) == 1),
> > > -		"no overflow and chain counter incremented after 100 SW_INCR/CHAIN");
> > > +	report(read_regn_el0(pmevcntr, 1) == 1,
> > > +			"no chain counter incremented after 100 SW_INCR/CHAIN");
> > >  	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
> > > -		    read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > > +			read_regn_el0(pmevcntr, 0), read_regn_el0(pmevcntr, 1));
> > >  
> > >  	/* 64b SW_INCR and overflow on CHAIN counter*/
> > >  	pmu_reset();
> > > @@ -644,7 +643,7 @@ static void test_chained_sw_incr(void)
> > >  	for (i = 0; i < 100; i++)
> > >  		write_sysreg(0x1, pmswinc_el0);
> > >  
> > > -	report((read_sysreg(pmovsclr_el0) == 0x2) &&
> > > +	report((read_sysreg(pmovsclr_el0) & 0x2) &&
> > >  		(read_regn_el0(pmevcntr, 1) == 0) &&
> > >  		(read_regn_el0(pmevcntr, 0) == 84),
> > >  		"overflow on chain counter and expected values after 100 SW_INCR/CHAIN");
> > > @@ -727,8 +726,8 @@ static void test_chain_promotion(void)
> > >  	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> > >  		    read_regn_el0(pmevcntr, 0));
> > >  
> > > -	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> > > -		"CHAIN counter enabled: CHAIN counter was incremented and no overflow");
> > > +	report((read_regn_el0(pmevcntr, 1) == 1),
> > > +		"CHAIN counter enabled: CHAIN counter was incremented");
> > >  
> > >  	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> > >  		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> > > @@ -755,8 +754,8 @@ static void test_chain_promotion(void)
> > >  	report_info("MEM_ACCESS counter #0 has value 0x%lx",
> > >  		    read_regn_el0(pmevcntr, 0));
> > >  
> > > -	report((read_regn_el0(pmevcntr, 1) == 1) && !read_sysreg(pmovsclr_el0),
> > > -		"32b->64b: CHAIN counter incremented and no overflow");
> > > +	report((read_regn_el0(pmevcntr, 1) == 1),
> > > +		"32b->64b: CHAIN counter incremented");
> > >  
> > >  	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
> > >  		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
> > 
> > I'm looking at fixing KVM to match this (see the binch of hacks at
> > [1]), and still getting a couple of failures in the PMU overflow tests
> > despite my best effort to fix the code:
> > 
> > $ ./arm-run  arm/pmu.flat --append pmu-overflow-interrupt
> > /usr/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 --append pmu-overflow-interrupt # -initrd /tmp/tmp.RQ6FmkvXay
> > INFO: PMU version: 0x1
> > INFO: PMU implementer/ID code: 0x41("A")/0x3
> > INFO: Implements 6 event counters
> > PASS: pmu: pmu-overflow-interrupt: no overflow interrupt after preset
> > PASS: pmu: pmu-overflow-interrupt: no overflow interrupt after counting
> > INFO: pmu: pmu-overflow-interrupt: overflow=0x0
> > PASS: pmu: pmu-overflow-interrupt: overflow interrupts expected on #0 and #1
> > FAIL: pmu: pmu-overflow-interrupt: no overflow interrupt expected on 32b boundary
> > FAIL: pmu: pmu-overflow-interrupt: expect overflow interrupt on odd counter
> > SUMMARY: 5 tests, 2 unexpected failures
> > 
> > Looking at the kut code, I'm wondering whether you're still missing a
> > couple of extra fixes such as:
> > 
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 4f2c5096..e0b9f71a 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -861,8 +861,8 @@ static void test_overflow_interrupt(void)
> >  	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
> >  	isb();
> >  	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
> > -	report(expect_interrupts(0),
> > -		"no overflow interrupt expected on 32b boundary");
> > +	report(expect_interrupts(1),
> > +		"expect overflow interrupt on 32b counter");
> >  
> >  	/* overflow on odd counter */
> >  	pmu_reset_stats();
> > @@ -870,8 +870,8 @@ static void test_overflow_interrupt(void)
> >  	write_regn_el0(pmevcntr, 1, ALL_SET);
> >  	isb();
> >  	mem_access_loop(addr, 400, pmu.pmcr_ro | PMU_PMCR_E);
> > -	report(expect_interrupts(0x2),
> > -		"expect overflow interrupt on odd counter");
> > +	report(expect_interrupts(0x3),
> > +		"expect overflow interrupt on even+odd counters");
> >  }
> >  #endif
> >  
> > With that, all PMU tests pass. Am I missing something? Did you notice
> > these failing on HW?
> > 

Actually, yes. But, I wasn't 100% sure. I tried a PMU passthrough
prototype on both real HW and the fast-model, and with
test_overflow_interrupt() I see an interrupt overflow hitting EL2. I
wasn't sure whether I should be forwarding it to the guest.

Thanks,
Ricardo

> > Thanks,
> > 
> > 	M.
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/pmu-chained
> > 
> > -- 
> > Without deviation from the norm, progress is not possible.
> 
> -- 
> Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 48+ messages in thread
end of thread, other threads:[~2022-08-01 19:16 UTC | newest]

Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-18 15:49 [kvm-unit-tests PATCH 0/3] arm: pmu: Fixes for bare metal Ricardo Koller
2022-07-18 15:49 ` Ricardo Koller
2022-07-18 15:49 ` [kvm-unit-tests PATCH 1/3] arm: pmu: Add missing isb()'s after sys register writing Ricardo Koller
2022-07-18 15:49   ` Ricardo Koller
2022-07-18 16:38   ` Alexandru Elisei
2022-07-18 16:38     ` Alexandru Elisei
2022-07-18 17:48     ` Ricardo Koller
2022-07-18 17:48       ` Ricardo Koller
2022-07-19 11:26       ` Alexandru Elisei
2022-07-19 11:26         ` Alexandru Elisei
2022-07-19 11:14   ` Alexandru Elisei
2022-07-19 11:14     ` Alexandru Elisei
2022-07-20 21:20     ` Ricardo Koller
2022-07-20 21:20       ` Ricardo Koller
2022-07-18 15:49 ` [kvm-unit-tests PATCH 2/3] arm: pmu: Reset the pmu registers before starting some tests Ricardo Koller
2022-07-18 15:49   ` Ricardo Koller
2022-07-18 15:49 ` [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests Ricardo Koller
2022-07-18 15:49   ` Ricardo Koller
2022-07-19 11:34   ` Marc Zyngier
2022-07-19 11:34     ` Marc Zyngier
2022-07-20  8:40     ` Ricardo Koller
2022-07-20  8:40       ` Ricardo Koller
2022-07-20  9:45       ` Marc Zyngier
2022-07-20  9:45         ` Marc Zyngier
2022-07-20 21:17         ` Ricardo Koller
2022-07-20 21:17           ` Ricardo Koller
2022-07-20 21:26           ` Ricardo Koller
2022-07-20 21:26             ` Ricardo Koller
2022-07-21 13:43             ` Marc Zyngier
2022-07-21 13:43               ` Marc Zyngier
2022-07-22 21:53               ` Ricardo Koller
2022-07-22 21:53                 ` Ricardo Koller
2022-07-23  7:59                 ` Andrew Jones
2022-07-23  7:59                   ` Andrew Jones
2022-07-24  9:40                   ` Marc Zyngier
2022-07-24  9:40                     ` Marc Zyngier
2022-07-27  2:29                     ` Ricardo Koller
2022-07-27  2:29                       ` Ricardo Koller
2022-07-30 12:47   ` Marc Zyngier
2022-07-30 12:47     ` Marc Zyngier
2022-07-30 12:52     ` Marc Zyngier
2022-07-30 12:52       ` Marc Zyngier
2022-08-01 19:15       ` Ricardo Koller
2022-08-01 19:15         ` Ricardo Koller
2022-07-18 16:42 ` [kvm-unit-tests PATCH 0/3] arm: pmu: Fixes for bare metal Alexandru Elisei
2022-07-18 16:42   ` Alexandru Elisei
2022-07-18 17:18   ` Ricardo Koller
2022-07-18 17:18     ` Ricardo Koller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.