| Message ID | 20230811144017.491628-3-james.clark@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf vendor events arm64: Update N2 and V2 metrics and events using Arm telemetry repo | expand |
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 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? > +{ > + 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
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 >
On 14/08/2023 15:15, James Clark wrote: > > 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 > ok, fine, so we seems that we would be following x86 on this in terms of using strcmp_cpuid_str(). It would be good to mention that there is already a weak version of strcmp_cpuid_str() for !x86 in your commit message. Let me check your code again... Thanks, John
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() > - > 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? > + * > + * 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; > +}
On 14/08/2023 15:43, John Garry wrote: > On 14/08/2023 15:15, James Clark wrote: >> >> 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 >> > > ok, fine, so we seems that we would be following x86 on this in terms of > using strcmp_cpuid_str(). It would be good to mention that there is > already a weak version of strcmp_cpuid_str() for !x86 in your commit > message. > Yep I can add this. > Let me check your code again... > > Thanks, > John
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; >> +} >
On 16/08/2023 10:12, James Clark wrote: > > > 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? Right, I don't think that the strtoul() is required. > >>> - >>> 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. ok, fine. > >>> + * >>> + * 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; >>> +} >>
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) +{ + 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; +}
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. Signed-off-by: James Clark <james.clark@arm.com> --- tools/perf/arch/arm64/util/header.c | 64 ++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 14 deletions(-)