Re: [PATCH v5 2/6] perf arm64: Allow version comparisons of CPU IDs

[James Clark <james.clark@arm.com>]

On 15/08/2023 10:35, John Garry wrote:
> On 11/08/2023 15:39, James Clark wrote:
>> Currently variant and revision fields are masked out of the MIDR so
>> it's not possible to compare different versions of the same CPU.
>> In a later commit a workaround will be removed just for N2 r0p3, so
>> enable comparisons on version.
>>
>> This has the side effect of changing the MIDR stored in the header of
>> the perf.data file to no longer have masked version fields. It also
>> affects the lookups in mapfile.csv, but as that currently only has
>> zeroed version fields, it has no actual effect. The mapfile.csv
>> documentation also states to zero the version fields, so unless this
>> isn't done it will continue to have no effect.
>>
> 
> This looks ok apart from a couple of comments, below.
> 
> Thanks,
> John
> 
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>>   tools/perf/arch/arm64/util/header.c | 64 ++++++++++++++++++++++-------
>>   1 file changed, 50 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/perf/arch/arm64/util/header.c
>> b/tools/perf/arch/arm64/util/header.c
>> index 80b9f6287fe2..8f74e801e1ab 100644
>> --- a/tools/perf/arch/arm64/util/header.c
>> +++ b/tools/perf/arch/arm64/util/header.c
>> @@ -1,3 +1,6 @@
>> +#include <linux/kernel.h>
>> +#include <linux/bits.h>
>> +#include <linux/bitfield.h>
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   #include <perf/cpumap.h>
>> @@ -10,14 +13,12 @@
>>     #define MIDR "/regs/identification/midr_el1"
>>   #define MIDR_SIZE 19
>> -#define MIDR_REVISION_MASK      0xf
>> -#define MIDR_VARIANT_SHIFT      20
>> -#define MIDR_VARIANT_MASK       (0xf << MIDR_VARIANT_SHIFT)
>> +#define MIDR_REVISION_MASK      GENMASK(3, 0)
>> +#define MIDR_VARIANT_MASK    GENMASK(23, 20)
>>     static int _get_cpuid(char *buf, size_t sz, struct perf_cpu_map
>> *cpus)
>>   {
>>       const char *sysfs = sysfs__mountpoint();
>> -    u64 midr = 0;
>>       int cpu;
>>         if (!sysfs || sz < MIDR_SIZE)
>> @@ -44,21 +45,11 @@ static int _get_cpuid(char *buf, size_t sz, struct
>> perf_cpu_map *cpus)
>>           }
>>           fclose(file);
>>   -        /* Ignore/clear Variant[23:20] and
>> -         * Revision[3:0] of MIDR
>> -         */
>> -        midr = strtoul(buf, NULL, 16);
>> -        midr &= (~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK));
>> -        scnprintf(buf, MIDR_SIZE, "0x%016lx", midr);
>>           /* got midr break loop */
>>           break;
>>       }
>>         perf_cpu_map__put(cpus);
>> -
>> -    if (!midr)
>> -        return EINVAL;
> 
> Is there a reason to drop this check?
> 
> As I see, it is still checked in perf_pmu__getcpudid() ->
> get_cpuid_str() -> _get_cpuid(), and we don't zero the buf allocated in
> _get_cpuid()
> 

Ah yes, now if all the files fail to open or read then buf will be
uninitialized. I make it so that it will return EINVAL unless the
fgets() succeeds, but I don't think we need to add the strtoul() back in?

>> -
>>       return 0;
>>   }
>>   @@ -99,3 +90,48 @@ char *get_cpuid_str(struct perf_pmu *pmu)
>>         return buf;
>>   }
>> +
>> +/*
>> + * Return 0 if idstr is a higher or equal to version of the same part as
>> + * mapcpuid.
> 
> And what other values may be returned? If just 0/1, then can we have a
> bool return value?
> 

I don't think that's best for consistency. All the other CPU ID
comparison functions return the strcmp style return values which is the
reverse of booleans. We could change them all to bool, but it would be a
big change, and they'd still have strcmp in the name which suggests
-1/0/1 return values (although -1 is never used here).

I will add to the comment that 1 is returned for a comparison failure
thought. That is missing.

>> + *
>> + * Therefore, if mapcpuid has 0 for revision and variant then any
>> version of
>> + * idstr will match as long as it's the same CPU type.
>> + */
>> +int strcmp_cpuid_str(const char *mapcpuid, const char *idstr)
>> +{
>> +    u64 map_id = strtoull(mapcpuid, NULL, 16);
>> +    char map_id_variant = FIELD_GET(MIDR_VARIANT_MASK, map_id);
>> +    char map_id_revision = FIELD_GET(MIDR_REVISION_MASK, map_id);
>> +    u64 id = strtoull(idstr, NULL, 16);
>> +    char id_variant = FIELD_GET(MIDR_VARIANT_MASK, id);
>> +    char id_revision = FIELD_GET(MIDR_REVISION_MASK, id);
>> +    u64 id_fields = ~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK);
>> +
>> +    /* Compare without version first */
>> +    if ((map_id & id_fields) != (id & id_fields))
>> +        return 1;
>> +
>> +    /*
>> +     * ID matches, now compare version.
>> +     *
>> +     * Arm revisions (like r0p0) are compared here like two digit semver
>> +     * values eg. 1.3 < 2.0 < 2.1 < 2.2. The events json file with the
>> +     * highest matching version is used.
>> +     *
>> +     *  r = high value = 'Variant' field in MIDR
>> +     *  p = low value  = 'Revision' field in MIDR
>> +     *
>> +     */
>> +    if (id_variant > map_id_variant)
>> +        return 0;
>> +
>> +    if (id_variant == map_id_variant && id_revision >= map_id_revision)
>> +        return 0;
>> +
>> +    /*
>> +     * variant is less than mapfile variant or variants are the same but
>> +     * the revision doesn't match. Return no match.
>> +     */
>> +    return 1;
>> +}
>