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

[Ricardo Koller <ricarkol@google.com>]

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
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm