| Message ID | 20220718154910.3923412-2-ricarkol@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: pmu: Fixes for bare metal | expand |
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 >
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
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 >
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
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 > >
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; }
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(+)