On Fri, 09 Dec 2022 17:47:14 +0000,
Alexandru Elisei <alexandru.elisei@xxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
M.
--
Without deviation from the norm, progress is not possible.
