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@xxxxxxxxxx>
> > ---
> > 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
> >
