Now the get_cpuid__str function returns the MIDR string of the first
online cpu from the range of cpus asscociated with the PMU CORE device.
It can work when pass a perf_pmu entity to get_cpuid_str. However, it
will pass NULL via perf_pmu__find_map from metricgroup.c if we want to add
metric group for arm64. When pass NULL to get_cpuid_str, it can't return
the MIDR string.
There are three methods from userspace getting MIDR string for arm64:
1. parse sysfs(/sys/devices/system/cpu/cpu?/regs/identification/midr_el1)
2. parse procfs(/proc/cpuinfo)
3. read the hwcaps(MIDR register) with getauxval(AT_HWCAP)
Perfer to select #3 as it is more simple and direct.
Signed-off-by: Joakim Zhang <[email protected]>
---
tools/perf/arch/arm64/util/header.c | 68 +++++++++++------------------
1 file changed, 25 insertions(+), 43 deletions(-)
diff --git a/tools/perf/arch/arm64/util/header.c b/tools/perf/arch/arm64/util/header.c
index 534cd2507d83..f58f08af0be8 100644
--- a/tools/perf/arch/arm64/util/header.c
+++ b/tools/perf/arch/arm64/util/header.c
@@ -2,64 +2,46 @@
#include <stdlib.h>
#include <api/fs/fs.h>
#include "header.h"
+#include <asm/hwcap.h>
+#include <sys/auxv.h>
-#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)
-char *get_cpuid_str(struct perf_pmu *pmu)
+#define get_cpuid(id) ({ \
+ unsigned long __val; \
+ asm("mrs %0, "#id : "=r" (__val)); \
+ __val; \
+ })
+
+char *get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
{
char *buf = NULL;
- char path[PATH_MAX];
- const char *sysfs = sysfs__mountpoint();
- int cpu;
- u64 midr = 0;
- struct cpu_map *cpus;
- FILE *file;
+ unsigned long midr = 0;
- if (!sysfs || !pmu || !pmu->cpus)
+ if (!(getauxval(AT_HWCAP) & HWCAP_CPUID)) {
+ fputs("CPUID registers unavailable\n", stderr);
return NULL;
+ }
- buf = malloc(MIDR_SIZE);
- if (!buf)
+ midr = get_cpuid(MIDR_EL1);
+ if (!midr) {
+ fputs("Failed to get cpuid string\n", stderr);
return NULL;
+ }
- /* read midr from list of cpus mapped to this pmu */
- cpus = cpu_map__get(pmu->cpus);
- for (cpu = 0; cpu < cpus->nr; cpu++) {
- scnprintf(path, PATH_MAX, "%s/devices/system/cpu/cpu%d"MIDR,
- sysfs, cpus->map[cpu]);
-
- file = fopen(path, "r");
- if (!file) {
- pr_debug("fopen failed for file %s\n", path);
- continue;
- }
-
- if (!fgets(buf, MIDR_SIZE, file)) {
- fclose(file);
- continue;
- }
- fclose(file);
+ /* Ignore/clear Variant[23:20] and
+ * Revision[3:0] of MIDR
+ */
+ midr &= (~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK));
- /* 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;
- }
+ buf = malloc(MIDR_SIZE);
+ if (!buf)
+ return NULL;
- if (!midr) {
- pr_err("failed to get cpuid string for PMU %s\n", pmu->name);
- free(buf);
- buf = NULL;
- }
+ scnprintf(buf, MIDR_SIZE, "0x%016lx", midr);
- cpu_map__put(cpus);
return buf;
}
--
2.17.1
On Thu, Jul 18, 2019 at 06:21:30AM +0000, Joakim Zhang wrote:
> Now the get_cpuid__str function returns the MIDR string of the first
> online cpu from the range of cpus asscociated with the PMU CORE device.
>
> It can work when pass a perf_pmu entity to get_cpuid_str. However, it
> will pass NULL via perf_pmu__find_map from metricgroup.c if we want to add
> metric group for arm64. When pass NULL to get_cpuid_str, it can't return
> the MIDR string.
Why is this code passing a NULL PMU? What information does it actually
need? Other functions, such as print_pmu_events(), iterate over all PMUs.
> There are three methods from userspace getting MIDR string for arm64:
> 1. parse sysfs(/sys/devices/system/cpu/cpu?/regs/identification/midr_el1)
> 2. parse procfs(/proc/cpuinfo)
> 3. read the hwcaps(MIDR register) with getauxval(AT_HWCAP)
>
> Perfer to select #3 as it is more simple and direct.
That's probably terminally broken for heterogeneous systems, so I'm not
at all happy with this patch.
Will
> diff --git a/tools/perf/arch/arm64/util/header.c b/tools/perf/arch/arm64/util/header.c
> index 534cd2507d83..f58f08af0be8 100644
> --- a/tools/perf/arch/arm64/util/header.c
> +++ b/tools/perf/arch/arm64/util/header.c
> @@ -2,64 +2,46 @@
> #include <stdlib.h>
> #include <api/fs/fs.h>
> #include "header.h"
> +#include <asm/hwcap.h>
> +#include <sys/auxv.h>
>
> -#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)
>
> -char *get_cpuid_str(struct perf_pmu *pmu)
> +#define get_cpuid(id) ({ \
> + unsigned long __val; \
> + asm("mrs %0, "#id : "=r" (__val)); \
> + __val; \
> + })
> +
> +char *get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
> {
> char *buf = NULL;
> - char path[PATH_MAX];
> - const char *sysfs = sysfs__mountpoint();
> - int cpu;
> - u64 midr = 0;
> - struct cpu_map *cpus;
> - FILE *file;
> + unsigned long midr = 0;
>
> - if (!sysfs || !pmu || !pmu->cpus)
> + if (!(getauxval(AT_HWCAP) & HWCAP_CPUID)) {
> + fputs("CPUID registers unavailable\n", stderr);
> return NULL;
> + }
>
> - buf = malloc(MIDR_SIZE);
> - if (!buf)
> + midr = get_cpuid(MIDR_EL1);
> + if (!midr) {
> + fputs("Failed to get cpuid string\n", stderr);
> return NULL;
> + }
>
> - /* read midr from list of cpus mapped to this pmu */
> - cpus = cpu_map__get(pmu->cpus);
> - for (cpu = 0; cpu < cpus->nr; cpu++) {
> - scnprintf(path, PATH_MAX, "%s/devices/system/cpu/cpu%d"MIDR,
> - sysfs, cpus->map[cpu]);
> -
> - file = fopen(path, "r");
> - if (!file) {
> - pr_debug("fopen failed for file %s\n", path);
> - continue;
> - }
> -
> - if (!fgets(buf, MIDR_SIZE, file)) {
> - fclose(file);
> - continue;
> - }
> - fclose(file);
> + /* Ignore/clear Variant[23:20] and
> + * Revision[3:0] of MIDR
> + */
> + midr &= (~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK));
>
> - /* 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;
> - }
> + buf = malloc(MIDR_SIZE);
> + if (!buf)
> + return NULL;
>
> - if (!midr) {
> - pr_err("failed to get cpuid string for PMU %s\n", pmu->name);
> - free(buf);
> - buf = NULL;
> - }
> + scnprintf(buf, MIDR_SIZE, "0x%016lx", midr);
>
> - cpu_map__put(cpus);
> return buf;
> }
> --
> 2.17.1
>
> -----Original Message-----
> From: Will Deacon <[email protected]>
> Sent: 2019??7??19?? 15:55
> To: Joakim Zhang <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Frank Li <[email protected]>;
> dl-linux-imx <[email protected]>; [email protected];
> [email protected]
> Subject: Re: [PATCH] perf tool: arch: arm64: change the way for get_cpuid_str()
> function
>
> On Thu, Jul 18, 2019 at 06:21:30AM +0000, Joakim Zhang wrote:
> > Now the get_cpuid__str function returns the MIDR string of the first
> > online cpu from the range of cpus asscociated with the PMU CORE device.
> >
> > It can work when pass a perf_pmu entity to get_cpuid_str. However, it
> > will pass NULL via perf_pmu__find_map from metricgroup.c if we want to
> > add metric group for arm64. When pass NULL to get_cpuid_str, it can't
> > return the MIDR string.
>
> Why is this code passing a NULL PMU? What information does it actually need?
> Other functions, such as print_pmu_events(), iterate over all PMUs.
Hi Will,
tools/perf/utils/metricgroup.c:
metricgroup__add_metric()/metricgroup__has_metric()/metricgroup__print()---->perf_pmu__find_map(NULL)------>perf_pmu__getcpuid(NULL)----->get_cpuid_str(NULL)
So, it will eventually pass NULL to get_cpuid_str() function for arm64, and now get_cpuid_str() for arm64 need *struct perf_pmu* entity to return MIDR string.
And the declaration for get_cpuid_str() is char *get_cpuid_str(struct perf_pmu *pmu __maybe_unused) in tools/perf/utils/header.h file.
So, it can return MIDR string without passing *struct perf_pmu* entity. Arch PowerPC/x86/s390 which implement metricgroup all add __maybe_unused.
> > There are three methods from userspace getting MIDR string for arm64:
> > 1. parse
> > sysfs(/sys/devices/system/cpu/cpu?/regs/identification/midr_el1)
> > 2. parse procfs(/proc/cpuinfo)
> > 3. read the hwcaps(MIDR register) with getauxval(AT_HWCAP)
> >
> > Perfer to select #3 as it is more simple and direct.
>
> That's probably terminally broken for heterogeneous systems, so I'm not at all
> happy with this patch.
The implement of get_cpuid_str() for arm64 now only can return the MIDR string of the first online cpu. I think it is also broken for heterogeneous systems.
Will, do you know how to fix this issue more reasonable?
Best Regards,
Joakim Zhang
> Will
>
> > diff --git a/tools/perf/arch/arm64/util/header.c
> > b/tools/perf/arch/arm64/util/header.c
> > index 534cd2507d83..f58f08af0be8 100644
> > --- a/tools/perf/arch/arm64/util/header.c
> > +++ b/tools/perf/arch/arm64/util/header.c
> > @@ -2,64 +2,46 @@
> > #include <stdlib.h>
> > #include <api/fs/fs.h>
> > #include "header.h"
> > +#include <asm/hwcap.h>
> > +#include <sys/auxv.h>
> >
> > -#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)
> >
> > -char *get_cpuid_str(struct perf_pmu *pmu)
> > +#define get_cpuid(id) ({ \
> > + unsigned long __val; \
> > + asm("mrs %0, "#id : "=r" (__val)); \
> > + __val; \
> > + })
> > +
> > +char *get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
> > {
> > char *buf = NULL;
> > - char path[PATH_MAX];
> > - const char *sysfs = sysfs__mountpoint();
> > - int cpu;
> > - u64 midr = 0;
> > - struct cpu_map *cpus;
> > - FILE *file;
> > + unsigned long midr = 0;
> >
> > - if (!sysfs || !pmu || !pmu->cpus)
> > + if (!(getauxval(AT_HWCAP) & HWCAP_CPUID)) {
> > + fputs("CPUID registers unavailable\n", stderr);
> > return NULL;
> > + }
> >
> > - buf = malloc(MIDR_SIZE);
> > - if (!buf)
> > + midr = get_cpuid(MIDR_EL1);
> > + if (!midr) {
> > + fputs("Failed to get cpuid string\n", stderr);
> > return NULL;
> > + }
> >
> > - /* read midr from list of cpus mapped to this pmu */
> > - cpus = cpu_map__get(pmu->cpus);
> > - for (cpu = 0; cpu < cpus->nr; cpu++) {
> > - scnprintf(path, PATH_MAX, "%s/devices/system/cpu/cpu%d"MIDR,
> > - sysfs, cpus->map[cpu]);
> > -
> > - file = fopen(path, "r");
> > - if (!file) {
> > - pr_debug("fopen failed for file %s\n", path);
> > - continue;
> > - }
> > -
> > - if (!fgets(buf, MIDR_SIZE, file)) {
> > - fclose(file);
> > - continue;
> > - }
> > - fclose(file);
> > + /* Ignore/clear Variant[23:20] and
> > + * Revision[3:0] of MIDR
> > + */
> > + midr &= (~(MIDR_VARIANT_MASK | MIDR_REVISION_MASK));
> >
> > - /* 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;
> > - }
> > + buf = malloc(MIDR_SIZE);
> > + if (!buf)
> > + return NULL;
> >
> > - if (!midr) {
> > - pr_err("failed to get cpuid string for PMU %s\n", pmu->name);
> > - free(buf);
> > - buf = NULL;
> > - }
> > + scnprintf(buf, MIDR_SIZE, "0x%016lx", midr);
> >
> > - cpu_map__put(cpus);
> > return buf;
> > }
> > --
> > 2.17.1
> >
On Fri, Jul 19, 2019 at 08:29:12AM +0000, Joakim Zhang wrote:
> > On Thu, Jul 18, 2019 at 06:21:30AM +0000, Joakim Zhang wrote:
> > > Now the get_cpuid__str function returns the MIDR string of the first
> > > online cpu from the range of cpus asscociated with the PMU CORE device.
> > >
> > > It can work when pass a perf_pmu entity to get_cpuid_str. However, it
> > > will pass NULL via perf_pmu__find_map from metricgroup.c if we want to
> > > add metric group for arm64. When pass NULL to get_cpuid_str, it can't
> > > return the MIDR string.
> >
> > Why is this code passing a NULL PMU? What information does it actually need?
> > Other functions, such as print_pmu_events(), iterate over all PMUs.
>
> tools/perf/utils/metricgroup.c:
> metricgroup__add_metric()/metricgroup__has_metric()/metricgroup__print()---->perf_pmu__find_map(NULL)------>perf_pmu__getcpuid(NULL)----->get_cpuid_str(NULL)
> So, it will eventually pass NULL to get_cpuid_str() function for arm64, and now get_cpuid_str() for arm64 need *struct perf_pmu* entity to return MIDR string.
>
> And the declaration for get_cpuid_str() is char *get_cpuid_str(struct perf_pmu *pmu __maybe_unused) in tools/perf/utils/header.h file.
> So, it can return MIDR string without passing *struct perf_pmu* entity. Arch PowerPC/x86/s390 which implement metricgroup all add __maybe_unused.
>
> > > There are three methods from userspace getting MIDR string for arm64:
> > > 1. parse
> > > sysfs(/sys/devices/system/cpu/cpu?/regs/identification/midr_el1)
> > > 2. parse procfs(/proc/cpuinfo)
> > > 3. read the hwcaps(MIDR register) with getauxval(AT_HWCAP)
> > >
> > > Perfer to select #3 as it is more simple and direct.
> >
> > That's probably terminally broken for heterogeneous systems, so I'm not at all
> > happy with this patch.
>
> The implement of get_cpuid_str() for arm64 now only can return the MIDR
> string of the first online cpu. I think it is also broken for
> heterogeneous systems.
I disagree: the current code returns the MIDR string for the first online
CPU *corresponding to the PMU* (e.g. pmu->cpus) and therefore handles
heterogeneous systems like that.
> Will, do you know how to fix this issue more reasonable?
I still don't know what the metricgroup coce is trying to achieve, but my
previous suggestion about looking at print_pmu_events() is probably still a
good place to start.
Will