[kvm-unit-tests PATCH v4 0/4] arm: pmu: Fixes for bare metal

[kvm-unit-tests PATCH v4 0/4] arm: pmu: Fixes for bare metal

[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.

Addressed all comments from the previous version:
https://lore.kernel.org/kvmarm/YvPsBKGbHHQP+0oS@google.com/T/#mb077998e2eb9fb3e15930b3412fd7ba2fb4103ca
- add pmu_reset() for 32-bit arm [Andrew]
- collect r-b from Alexandru

Thanks!
Ricardo

Ricardo Koller (4):
  arm: pmu: Add missing isb()'s after sys register writing
  arm: pmu: Add reset_pmu() for 32-bit arm
  arm: pmu: Reset the pmu registers before starting some tests
  arm: pmu: Check for overflow in the low counter in chained counters
    tests

 arm/pmu.c | 72 ++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 55 insertions(+), 17 deletions(-)

-- 
2.37.1.559.g78731f0fdb-goog

[kvm-unit-tests PATCH v4 1/4] arm: pmu: Add missing isb()'s after sys register writing

[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 seem
required:
- after clearing the overflow signal in the IRQ handler to make spurious
  interrupts less likely.
- after direct writes to PMSWINC_EL0 for software to read the correct
  value for PMEVNCTR0_EL0 (from ARM DDI 0487H.a, page D13-5237).

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>
Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arm/pmu.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index 15c542a2..4c601b05 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,10 +535,12 @@ 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);
 
+	isb();
 	report_info("SW_INCR counter #0 has value %ld", read_regn_el0(pmevcntr, 0));
 	report(read_regn_el0(pmevcntr, 0) == PRE_OVERFLOW,
 		"PWSYNC does not increment if PMCR.E is unset");
@@ -547,10 +550,12 @@ 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);
 
+	isb();
 	report(read_regn_el0(pmevcntr, 0)  == 84, "counter #1 after + 100 SW_INCR");
 	report(read_regn_el0(pmevcntr, 1)  == 100,
 		"counter #0 after + 100 SW_INCR");
@@ -618,9 +623,12 @@ 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);
 
+	isb();
 	report(!read_sysreg(pmovsclr_el0) && (read_regn_el0(pmevcntr, 1) == 1),
 		"no overflow and chain counter incremented after 100 SW_INCR/CHAIN");
 	report_info("overflow=0x%lx, #0=%ld #1=%ld", read_sysreg(pmovsclr_el0),
@@ -634,9 +642,12 @@ 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);
 
+	isb();
 	report((read_sysreg(pmovsclr_el0) == 0x2) &&
 		(read_regn_el0(pmevcntr, 1) == 0) &&
 		(read_regn_el0(pmevcntr, 0) == 84),
@@ -821,10 +832,14 @@ 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);
 
+	isb();
 	set_pmcr(pmu.pmcr_ro);
+	isb();
 	report(expect_interrupts(0), "no overflow interrupt after counting");
 
 	/* enable interrupts */
@@ -879,6 +894,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 +910,7 @@ static bool check_cycles_increase(void)
 	}
 
 	set_pmcr(get_pmcr() & ~PMU_PMCR_E);
+	isb();
 
 	return success;
 }
-- 
2.37.1.559.g78731f0fdb-goog

[kvm-unit-tests PATCH v4 2/4] arm: pmu: Add reset_pmu() for 32-bit arm

[Ricardo Koller]

Add a 32-bit arm version of reset_pmu(). Add all the necessary register
definitions as well.

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

diff --git a/arm/pmu.c b/arm/pmu.c
index 4c601b05..a5260178 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -93,7 +93,10 @@ static struct pmu pmu;
 #define PMSELR       __ACCESS_CP15(c9, 0, c12, 5)
 #define PMXEVTYPER   __ACCESS_CP15(c9, 0, c13, 1)
 #define PMCNTENSET   __ACCESS_CP15(c9, 0, c12, 1)
+#define PMCNTENCLR   __ACCESS_CP15(c9, 0, c12, 2)
+#define PMOVSR       __ACCESS_CP15(c9, 0, c12, 3)
 #define PMCCNTR32    __ACCESS_CP15(c9, 0, c13, 0)
+#define PMINTENCLR   __ACCESS_CP15(c9, 0, c14, 2)
 #define PMCCNTR64    __ACCESS_CP15_64(0, c9)
 
 static inline uint32_t get_id_dfr0(void) { return read_sysreg(ID_DFR0); }
@@ -145,6 +148,19 @@ static inline void precise_instrs_loop(int loop, uint32_t pmcr)
 	: "cc");
 }
 
+static void pmu_reset(void)
+{
+	/* reset all counters, counting disabled at PMCR level*/
+	set_pmcr(pmu.pmcr_ro | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_P);
+	/* Disable all counters */
+	write_sysreg(ALL_SET, PMCNTENCLR);
+	/* clear overflow reg */
+	write_sysreg(ALL_SET, PMOVSR);
+	/* disable overflow interrupts on all counters */
+	write_sysreg(ALL_SET, PMINTENCLR);
+	isb();
+}
+
 /* event counter tests only implemented for aarch64 */
 static void test_event_introspection(void) {}
 static void test_event_counter_config(void) {}
-- 
2.37.1.559.g78731f0fdb-goog

[kvm-unit-tests PATCH v4 3/4] arm: pmu: Reset the pmu registers before starting some tests

[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.

As a bonus, fix a couple of comments related to the register state
before a sub-test.

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

diff --git a/arm/pmu.c b/arm/pmu.c
index a5260178..756e0d26 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -842,7 +842,7 @@ static void test_overflow_interrupt(void)
 	write_regn_el0(pmevcntr, 1, PRE_OVERFLOW);
 	isb();
 
-	/* interrupts are disabled */
+	/* interrupts are disabled (PMINTENSET_EL1 == 0) */
 
 	mem_access_loop(addr, 200, pmu.pmcr_ro | PMU_PMCR_E);
 	report(expect_interrupts(0), "no overflow interrupt after preset");
@@ -858,7 +858,7 @@ static void test_overflow_interrupt(void)
 	isb();
 	report(expect_interrupts(0), "no overflow interrupt after counting");
 
-	/* enable interrupts */
+	/* enable interrupts (PMINTENSET_EL1 <= ALL_SET) */
 
 	pmu_reset_stats();
 
@@ -906,6 +906,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 */
 
@@ -960,6 +961,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.1.559.g78731f0fdb-goog

[kvm-unit-tests PATCH v4 4/4] arm: pmu: Check for overflow in the low counter in chained counters tests

[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 checking for
overflow. Note that this test fails in KVM without the respective fix.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arm/pmu.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index 756e0d26..cd47b14b 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -599,7 +599,7 @@ 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");
+	report(read_sysreg(pmovsclr_el0) == 0x1, "overflow recorded for chained incr #1");
 
 	/* test 64b overflow */
 
@@ -611,7 +611,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) == 0x1, "overflow recorded for chained incr #2");
 
 	write_regn_el0(pmevcntr, 0, PRE_OVERFLOW);
 	write_regn_el0(pmevcntr, 1, ALL_SET);
@@ -619,7 +619,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) == 0x3, "overflow on even and odd counters");
 }
 
 static void test_chained_sw_incr(void)
@@ -645,8 +645,9 @@ static void test_chained_sw_incr(void)
 		write_sysreg(0x1, pmswinc_el0);
 
 	isb();
-	report(!read_sysreg(pmovsclr_el0) && (read_regn_el0(pmevcntr, 1) == 1),
-		"no overflow and chain counter incremented after 100 SW_INCR/CHAIN");
+	report((read_sysreg(pmovsclr_el0) == 0x1) &&
+		(read_regn_el0(pmevcntr, 1) == 1),
+		"overflow and 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));
 
@@ -664,10 +665,10 @@ static void test_chained_sw_incr(void)
 		write_sysreg(0x1, pmswinc_el0);
 
 	isb();
-	report((read_sysreg(pmovsclr_el0) == 0x2) &&
+	report((read_sysreg(pmovsclr_el0) == 0x3) &&
 		(read_regn_el0(pmevcntr, 1) == 0) &&
 		(read_regn_el0(pmevcntr, 0) == 84),
-		"overflow on chain counter and expected values after 100 SW_INCR/CHAIN");
+		"expected overflows and values 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));
 }
@@ -747,8 +748,9 @@ 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) &&
+		(read_sysreg(pmovsclr_el0) == 0x1),
+		"CHAIN counter enabled: CHAIN counter was incremented and overflow");
 
 	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
 		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
@@ -775,8 +777,9 @@ 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) &&
+		(read_sysreg(pmovsclr_el0) == 0x1),
+		"32b->64b: CHAIN counter incremented and overflow");
 
 	report_info("CHAIN counter #1 = 0x%lx, overflow=0x%lx",
 		read_regn_el0(pmevcntr, 1), read_sysreg(pmovsclr_el0));
@@ -884,8 +887,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(0x1),
+		"expect overflow interrupt on 32b boundary");
 
 	/* overflow on odd counter */
 	pmu_reset_stats();
@@ -893,8 +896,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 and odd counter");
 }
 #endif
 
-- 
2.37.1.559.g78731f0fdb-goog

Re: [kvm-unit-tests PATCH v4 0/4] arm: pmu: Fixes for bare metal

[Andrew Jones]

On Thu, Aug 11, 2022 at 11:52:06AM -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.
> 
> Addressed all comments from the previous version:
> https://lore.kernel.org/kvmarm/YvPsBKGbHHQP+0oS@google.com/T/#mb077998e2eb9fb3e15930b3412fd7ba2fb4103ca
> - add pmu_reset() for 32-bit arm [Andrew]
> - collect r-b from Alexandru

You forgot to pick up Oliver's r-b's and his Link suggestion. I can do
that again, though.

Thanks,
drew

Re: [kvm-unit-tests PATCH v4 0/4] arm: pmu: Fixes for bare metal

[Ricardo Koller]

On Fri, Aug 12, 2022 at 08:33:00AM +0200, Andrew Jones wrote:
> On Thu, Aug 11, 2022 at 11:52:06AM -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.
> > 
> > Addressed all comments from the previous version:
> > https://lore.kernel.org/kvmarm/YvPsBKGbHHQP+0oS@google.com/T/#mb077998e2eb9fb3e15930b3412fd7ba2fb4103ca
> > - add pmu_reset() for 32-bit arm [Andrew]
> > - collect r-b from Alexandru
> 
> You forgot to pick up Oliver's r-b's and his Link suggestion.

Ahh, yes, sorry Oliver.

> I can do that again, though.

Thanks Andrew

> 
> Thanks,
> drew

Re: [kvm-unit-tests PATCH v4 0/4] arm: pmu: Fixes for bare metal

[Marc Zyngier]

On 2022-08-11 19:52, 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.
> 
> Addressed all comments from the previous version:
> https://lore.kernel.org/kvmarm/YvPsBKGbHHQP+0oS@google.com/T/#mb077998e2eb9fb3e15930b3412fd7ba2fb4103ca
> - add pmu_reset() for 32-bit arm [Andrew]
> - collect r-b from Alexandru
> 
> Thanks!
> Ricardo
> 
> Ricardo Koller (4):
>   arm: pmu: Add missing isb()'s after sys register writing
>   arm: pmu: Add reset_pmu() for 32-bit arm
>   arm: pmu: Reset the pmu registers before starting some tests
>   arm: pmu: Check for overflow in the low counter in chained counters
>     tests
> 
>  arm/pmu.c | 72 ++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 55 insertions(+), 17 deletions(-)

For the series:

Acked-by: Marc Zyngier <maz@kernel.org>

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

Re: [kvm-unit-tests PATCH v4 0/4] arm: pmu: Fixes for bare metal

[Andrew Jones]

On Thu, Aug 11, 2022 at 11:52:06AM -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.
> 
> Addressed all comments from the previous version:
> https://lore.kernel.org/kvmarm/YvPsBKGbHHQP+0oS@google.com/T/#mb077998e2eb9fb3e15930b3412fd7ba2fb4103ca
> - add pmu_reset() for 32-bit arm [Andrew]
> - collect r-b from Alexandru
> 
> Thanks!
> Ricardo
> 
> Ricardo Koller (4):
>   arm: pmu: Add missing isb()'s after sys register writing
>   arm: pmu: Add reset_pmu() for 32-bit arm
>   arm: pmu: Reset the pmu registers before starting some tests
>   arm: pmu: Check for overflow in the low counter in chained counters
>     tests
> 
>  arm/pmu.c | 72 ++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 55 insertions(+), 17 deletions(-)
>

Hi all,

Please refresh my memory. Does this series work on current platforms? Or
was it introducing new test failures which may be in the test, as opposed
to KVM? If they work on most platforms, but not on every platform, then
have we identified what triggers them to fail and whether that should be
fixed or just worked-around? I'm sorry I still can't help out with the
testing as I haven't yet had time to setup the Rpi that Mark Rutland gave
me in Dublin.

I know this series has been rotting on arm/queue for months, so I'll be
happy to merge it if the consensus is to do so. I can also drop it, or
some of the patches, if that's the consensus.

Thanks,
drew

Re: [kvm-unit-tests PATCH v4 0/4] arm: pmu: Fixes for bare metal

[Marc Zyngier]

Hi Drew,

On Fri, 28 Oct 2022 12:40:41 +0100,
Andrew Jones <andrew.jones@linux.dev> wrote:
> 
> On Thu, Aug 11, 2022 at 11:52:06AM -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.
> > 
> > Addressed all comments from the previous version:
> > https://lore.kernel.org/kvmarm/YvPsBKGbHHQP+0oS@google.com/T/#mb077998e2eb9fb3e15930b3412fd7ba2fb4103ca
> > - add pmu_reset() for 32-bit arm [Andrew]
> > - collect r-b from Alexandru
> > 
> > Thanks!
> > Ricardo
> > 
> > Ricardo Koller (4):
> >   arm: pmu: Add missing isb()'s after sys register writing
> >   arm: pmu: Add reset_pmu() for 32-bit arm
> >   arm: pmu: Reset the pmu registers before starting some tests
> >   arm: pmu: Check for overflow in the low counter in chained counters
> >     tests
> > 
> >  arm/pmu.c | 72 ++++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 55 insertions(+), 17 deletions(-)
> >
> 
> Hi all,
> 
> Please refresh my memory. Does this series work on current platforms? Or
> was it introducing new test failures which may be in the test, as opposed
> to KVM? If they work on most platforms, but not on every platform, then
> have we identified what triggers them to fail and whether that should be
> fixed or just worked-around? I'm sorry I still can't help out with the
> testing as I haven't yet had time to setup the Rpi that Mark Rutland gave
> me in Dublin.

This series does show that KVM is buggy, and I have patches out to fix
it [1]. The patches should work on anything, really.

> I know this series has been rotting on arm/queue for months, so I'll be
> happy to merge it if the consensus is to do so. I can also drop it, or
> some of the patches, if that's the consensus.

I'd be very happy to see these patches being merged.

Thanks,

	M.

[1] https://lore.kernel.org/r/20221028105402.2030192-1-maz@kernel.org

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

Re: [kvm-unit-tests PATCH v4 0/4] arm: pmu: Fixes for bare metal

[Andrew Jones]

On Fri, Oct 28, 2022 at 04:01:50PM +0100, Marc Zyngier wrote:
> Hi Drew,
> 
> On Fri, 28 Oct 2022 12:40:41 +0100,
> Andrew Jones <andrew.jones@linux.dev> wrote:
> > 
> > On Thu, Aug 11, 2022 at 11:52:06AM -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.
> > > 
> > > Addressed all comments from the previous version:
> > > https://lore.kernel.org/kvmarm/YvPsBKGbHHQP+0oS@google.com/T/#mb077998e2eb9fb3e15930b3412fd7ba2fb4103ca
> > > - add pmu_reset() for 32-bit arm [Andrew]
> > > - collect r-b from Alexandru
> > > 
> > > Thanks!
> > > Ricardo
> > > 
> > > Ricardo Koller (4):
> > >   arm: pmu: Add missing isb()'s after sys register writing
> > >   arm: pmu: Add reset_pmu() for 32-bit arm
> > >   arm: pmu: Reset the pmu registers before starting some tests
> > >   arm: pmu: Check for overflow in the low counter in chained counters
> > >     tests
> > > 
> > >  arm/pmu.c | 72 ++++++++++++++++++++++++++++++++++++++++++-------------
> > >  1 file changed, 55 insertions(+), 17 deletions(-)
> > >
> > 
> > Hi all,
> > 
> > Please refresh my memory. Does this series work on current platforms? Or
> > was it introducing new test failures which may be in the test, as opposed
> > to KVM? If they work on most platforms, but not on every platform, then
> > have we identified what triggers them to fail and whether that should be
> > fixed or just worked-around? I'm sorry I still can't help out with the
> > testing as I haven't yet had time to setup the Rpi that Mark Rutland gave
> > me in Dublin.
> 
> This series does show that KVM is buggy, and I have patches out to fix
> it [1]. The patches should work on anything, really.
> 
> > I know this series has been rotting on arm/queue for months, so I'll be
> > happy to merge it if the consensus is to do so. I can also drop it, or
> > some of the patches, if that's the consensus.
> 
> I'd be very happy to see these patches being merged.

Thanks for the information, Marc. I've gone ahead and merged the tests.
What's the worst than can happen :-)  Anyway, I agree that when the tests
start failing in CIs, then they're doing their job. If your pending series
can't be applied right away, then the CIs can likely be taught to
temporarily ignore the known failures.

Thanks,
drew

> 
> Thanks,
> 
> 	M.
> 
> [1] https://lore.kernel.org/r/20221028105402.2030192-1-maz@kernel.org
> 
> -- 
> Without deviation from the norm, progress is not possible.