| Message ID | 20220803182328.2438598-4-ricarkol@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: pmu: Fixes for bare metal | expand |
On Wed, Aug 03, 2022 at 11:23:28AM -0700, Ricardo Koller 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 checking for > overflow. Note that this test fails in KVM without the respective fix. > > 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 7c5bc259..258780f4 100644 > --- a/arm/pmu.c > +++ b/arm/pmu.c > @@ -583,7 +583,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 */ > > @@ -595,7 +595,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); > @@ -603,7 +603,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) > @@ -629,8 +629,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)); > > @@ -648,10 +649,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"); > + "overflow on even and odd counters, and expected values after 100 SW_INCR/CHAIN"); Besides the extra space, this doesn't read well (to me). Thanks, drew
Hi Ricardo, On 8/3/22 20:23, Ricardo Koller 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 checking for > overflow. Note that this test fails in KVM without the respective fix. > > 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 7c5bc259..258780f4 100644 > --- a/arm/pmu.c > +++ b/arm/pmu.c > @@ -583,7 +583,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 */ > > @@ -595,7 +595,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); > @@ -603,7 +603,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) > @@ -629,8 +629,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)); > > @@ -648,10 +649,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"); > + "overflow on even and odd counters, and expected 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)); > } > @@ -731,8 +732,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)); > @@ -759,8 +761,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)); > @@ -867,8 +870,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(); > @@ -876,8 +879,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 > Besides Drew's comment and assuming Marc's fix in sync, Reviewed-by: Eric Auger <eric.auger@redhat.com> I definitively missed that spec detail when originally writing the test, thank you for fixing. Eric
On Thu, Aug 04, 2022 at 10:18:32AM +0200, Andrew Jones wrote: > On Wed, Aug 03, 2022 at 11:23:28AM -0700, Ricardo Koller 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 checking for > > overflow. Note that this test fails in KVM without the respective fix. > > > > 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 7c5bc259..258780f4 100644 > > --- a/arm/pmu.c > > +++ b/arm/pmu.c > > @@ -583,7 +583,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 */ > > > > @@ -595,7 +595,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); > > @@ -603,7 +603,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) > > @@ -629,8 +629,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)); > > > > @@ -648,10 +649,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"); > > + "overflow on even and odd counters, and expected values after 100 SW_INCR/CHAIN"); > > Besides the extra space, this doesn't read well (to me). Replaced the sentence with something simpler and hopefully nicer (in v3). Thanks, Ricardo > > Thanks, > drew
On Thu, 04 Aug 2022 19:21:49 +0100, Eric Auger <eric.auger@redhat.com> wrote: > > I definitively missed that spec detail when originally writing the test, > thank you for fixing. To be fair, it is only in a very recent version of the ARM ARM that this detail came to light, and we collectively misunderstood the spec for a few years, resulting in a over-complicated KVM infrastructure (the new emulation code is so much simpler). BTW, I think I forgot to Cc you and Andrew on the KVM rework at [1], sorry about that. Thanks, M. [1] https://lore.kernel.org/kvm/20220805135813.2102034-1-maz@kernel.org
diff --git a/arm/pmu.c b/arm/pmu.c index 7c5bc259..258780f4 100644 --- a/arm/pmu.c +++ b/arm/pmu.c @@ -583,7 +583,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 */ @@ -595,7 +595,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); @@ -603,7 +603,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) @@ -629,8 +629,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)); @@ -648,10 +649,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"); + "overflow on even and odd counters, and expected 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)); } @@ -731,8 +732,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)); @@ -759,8 +761,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)); @@ -867,8 +870,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(); @@ -876,8 +879,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
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. Signed-off-by: Ricardo Koller <ricarkol@google.com> --- arm/pmu.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)