On 14/08/2023 14:07, 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.
Did you consider adding a raw version of _get_cpuid(), which returns the full MIDR just for the purpose of caller strcmp_cpuid_str()?
I did, but I thought that seeing as it would only be used in one place, and that changing the existing one didn't break anything, that it was better to not fragment the CPU ID interface. I thought it might also have repercussions for the other architectures as well. It would also mean that the MIDR that's stored in the header wouldn't have the version information, which if we're starting to do things with that could be bad.
There are already callers of strcmp_cpuid_str() so it's probably best to keep it using the same get_cpuid() string. Unless there is a reason _not_ to do it? There isn't really anything that can't be done with it accepting/returning the full unmasked MIDR. If you want the old behavior, you just set the version fields to 0, which I've also used in a later patch and is already done in mapfile.csv
I can't comment on how it will be called in relation to strcmp_cpuid_str(), as I am unsure whether strcmp_cpuid_str() should be just in arch arm64 code - see comment on later patch.
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.
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;
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.
- 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)
should we check implementator and other fields as a sanity check?
I'm not sure what we could check them against? There are no existing sanity checks around the MIDR stuff, it just takes whatever MIDR is in mapfile.csv and from the system. Unless there is something specific to this change that would benefit from it? Otherwise it sounds like it could be a separate additional change to what's already in Perf.
+{ + 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; +}
Thanks, John