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

[Ricardo Koller <ricarkol@google.com>]

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");