| Message ID | 20220803182328.2438598-2-ricarkol@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: pmu: Fixes for bare metal | expand |
Hi, On Wed, Aug 03, 2022 at 11:23:26AM -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 seem > required: > - after clearing the overflow signal in the IRQ handler to avoid > spurious interrupts. Nitpick, but it doesn't avoid (eliminates) spurious interrupts, it makes them 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). > > 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 15c542a2..76156f78 100644 > --- a/arm/pmu.c > +++ b/arm/pmu.c > [..] > @@ -821,10 +832,13 @@ 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); > > set_pmcr(pmu.pmcr_ro); > + isb(); A context synchronization event affects system register writes that come before the context synchronization event in program order, but if there are multiple system register writes, it doesn't perform them in program order (if that makes sense). So it might happen that the CPU decides to perform the write to PMCR_EL1 which disables the PMU *before* the writes to PMSWINC_EL0. Which means that even if PMINTENSET_EL1 allows the PMU to assert interrupts when it shouldn't (thus causing the test to fail), those interrupt won't be asserted by the PMU because the PMU is disabled and the test would pass. You need an ISB after the PMSWINC_EL0 writes and before disabling the PMU. Thanks, Alex > report(expect_interrupts(0), "no overflow interrupt after counting");
On Thu, Aug 04, 2022 at 09:55:01AM +0100, Alexandru Elisei wrote: > Hi, > > On Wed, Aug 03, 2022 at 11:23:26AM -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 seem > > required: > > - after clearing the overflow signal in the IRQ handler to avoid > > spurious interrupts. > > Nitpick, but it doesn't avoid (eliminates) spurious interrupts, it makes > them 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). > > > > 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 15c542a2..76156f78 100644 > > --- a/arm/pmu.c > > +++ b/arm/pmu.c > > [..] > > @@ -821,10 +832,13 @@ 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); > > > > set_pmcr(pmu.pmcr_ro); > > + isb(); > > A context synchronization event affects system register writes that come > before the context synchronization event in program order, but if there are > multiple system register writes, it doesn't perform them in program order > (if that makes sense). Good point, didn't think of that case. Added the missing isb() in v3. Thanks, Ricardo > > So it might happen that the CPU decides to perform the write to PMCR_EL1 > which disables the PMU *before* the writes to PMSWINC_EL0. Which means that > even if PMINTENSET_EL1 allows the PMU to assert interrupts when it > shouldn't (thus causing the test to fail), those interrupt won't be > asserted by the PMU because the PMU is disabled and the test would pass. > > You need an ISB after the PMSWINC_EL0 writes and before disabling the PMU. > > Thanks, > Alex > > > report(expect_interrupts(0), "no overflow interrupt after counting");
diff --git a/arm/pmu.c b/arm/pmu.c index 15c542a2..76156f78 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,13 @@ 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); set_pmcr(pmu.pmcr_ro); + isb(); report(expect_interrupts(0), "no overflow interrupt after counting"); /* enable interrupts */ @@ -879,6 +893,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 +909,7 @@ static bool check_cycles_increase(void) } set_pmcr(get_pmcr() & ~PMU_PMCR_E); + isb(); return success; }
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 avoid spurious interrupts. - after direct writes to PMSWINC_EL0 for software to read the correct value for PMEVNCTR0_EL0 (from ARM DDI 0487H.a, page D13-5237). Signed-off-by: Ricardo Koller <ricarkol@google.com> --- arm/pmu.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)