Messages in this thread Patch in this message | | | Date | Fri, 19 Jul 2019 08:54:50 +0100 | From | Will Deacon <> | 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.
> 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 >
| |