Re: [kvm-unit-tests PATCH 1/3] arm: pmu: Fix overflow checks for PMUv3p5 long counters

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Marc,

On Sat, Dec 10, 2022 at 11:01:53AM +0000, Marc Zyngier wrote:
> On Fri, 09 Dec 2022 17:47:14 +0000,
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> > 
> > Hi,
> > 
> > On Fri, Dec 02, 2022 at 04:55:25AM +0000, Ricardo Koller wrote:
> > > PMUv3p5 uses 64-bit counters irrespective of whether the PMU is configured
> > > for overflowing at 32 or 64-bits. The consequence is that tests that check
> > > the counter values after overflowing should not assume that values will be
> > > wrapped around 32-bits: they overflow into the other half of the 64-bit
> > > counters on PMUv3p5.
> > > 
> > > Fix tests by correctly checking overflowing-counters against the expected
> > > 64-bit value.
> > > 
> > > Signed-off-by: Ricardo Koller <ricarkol@google.com>
> > > ---
> > >  arm/pmu.c | 29 ++++++++++++++++++-----------
> > >  1 file changed, 18 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/arm/pmu.c b/arm/pmu.c
> > > index cd47b14..eeac984 100644
> > > --- a/arm/pmu.c
> > > +++ b/arm/pmu.c
> > > @@ -54,10 +54,10 @@
> > >  #define EXT_COMMON_EVENTS_LOW	0x4000
> > >  #define EXT_COMMON_EVENTS_HIGH	0x403F
> > >  
> > > -#define ALL_SET			0xFFFFFFFF
> > > -#define ALL_CLEAR		0x0
> > > -#define PRE_OVERFLOW		0xFFFFFFF0
> > > -#define PRE_OVERFLOW2		0xFFFFFFDC
> > > +#define ALL_SET			0x00000000FFFFFFFFULL
> > > +#define ALL_CLEAR		0x0000000000000000ULL
> > > +#define PRE_OVERFLOW		0x00000000FFFFFFF0ULL
> > > +#define PRE_OVERFLOW2		0x00000000FFFFFFDCULL
> > >  
> > >  #define PMU_PPI			23
> > >  
> > > @@ -538,6 +538,7 @@ static void test_mem_access(void)
> > >  static void test_sw_incr(void)
> > >  {
> > >  	uint32_t events[] = {SW_INCR, SW_INCR};
> > > +	uint64_t cntr0;
> > >  	int i;
> > >  
> > >  	if (!satisfy_prerequisites(events, ARRAY_SIZE(events)))
> > > @@ -572,9 +573,9 @@ static void test_sw_incr(void)
> > >  		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");
> > > +	cntr0 = (pmu.version < ID_DFR0_PMU_V3_8_5) ? 84 : PRE_OVERFLOW + 100;
> > 
> > Hm... in the Arm ARM it says that counters are 64-bit if PMUv3p5 is
> > implemented.  But it doesn't say anywhere that versions newer than p5 are
> > required to implement PMUv3p5.
> 
> And I don't think it needs to say it, because there is otherwise no
> way for SW to discover whether 64bit counters are implemented or not.
> 
> > 
> > For example, for PMUv3p7, it says that the feature is mandatory in Arm8.7
> > implementations. My interpretation of that is that it is not forbidden for
> > an implementer to cherry-pick this version on older versions of the
> > architecture where PMUv3p5 is not implemented.
> 
> I'm sorry to have to say that, but I find your suggestion that PMUv3p7
> could be implemented without supporting the full gamut of PMUv3p5
> ludicrous.
> 
> Please look back at the ARM ARM, specially at the tiny section titled
> "Alternative ID scheme used for the Performance Monitors Extension
> version" (DDI0487I.a, D17.1.3, page 5553), and the snipped of C code
> that performs exactly this check:
> 
> <quote>
>   if (value != 0xF and value >= number) {
>   	// do something that relies on version 'number' of the feature
>   }
> </quote>
> 
> Replace 'value' with 7 (PMUv3p7), 'number' with 6 (PMUv3p5), and you
> get the exact property that you pretend doesn't exist, allowing you to
> rely on PMUv3p5 to be implemented when the HW has PMUv3p7.
> 
> > Maybe the check should be pmu.version == ID_DFR0_PMU_V3_8_5, to match the
> > counter definitions in the architecture?
> 
> No, that'd be totally wrong. You need to check your understanding of
> how the ID registers work.

A simple "hey, you're wrong here, the PMU extensions do not follow the
principles of the ID scheme for fields in ID registers" would have
sufficed. Guess you never made a silly mistake ever, right?

Otherwise, good job encouraging people to help review KVM/arm64 patches ;)

Thanks,
Alex